report: split topbar features #12926
Conversation
| * @param {Node=} target DOM node to fire the event on. | ||
| * @param {*=} detail Custom data to include. | ||
| */ | ||
| fireEventOn(name, target = this._document, detail) { |
There was a problem hiding this comment.
this seems dom-y to me.
Not sure why the first error happens. The second error is this line, and would require us to disable the CDT test check to land: I think it no longer is an error because I removed the |
I'm sure now: I updated the report renderer README with info on the workaround |
| * @param {DOM} dom | ||
| * @param {boolean} [force] | ||
| */ | ||
| export function toggleDarkTheme(dom, force) { |
There was a problem hiding this comment.
Why does this need to be in a separate file? Couldn't this just remain a member of ReportUIFeatures which is passed to the constructor of TopbarFeatures already?
There was a problem hiding this comment.
It could go there, however the eventual goal of this is to remove TopbarFeatures from ReportUIFeatures, and so this function will need to be a dependency of both. So it needs to be defined outside both of them.
Co-authored-by: Adam Raine <[email protected]>
is the intention to then remove the |
|
yea but I will probably forget |
For me, at least, this is the real meat of the issue, and until they are untangled, it does feel a bit like all we can do is bikeshed on which file each function should live in, which will inevitably change as we figure out how we want to rearchitect the report hierarchy. @connorjclark you started with topbar features, but what if we started with some of the non-topbar features that we can address without untangling from other features? e.g. we now want |
|
Where to start splitting didn't seem too consequential, and it wasn't trivial to split up this file, so I am hesitant to throw out this work just to change where we start splitting up
This seems good to me.
I plan to work on this next (haven't yet looked to see where it'd be best to move it to) |
See GoogleChrome/lighthouse#12926 (comment) Bug: 772558 Change-Id: Id3feb625fbc0e1cff1be809b81bd564cc313ef62 Reviewed-on: https://chromium-review.googlesource.com/c/devtools/devtools-frontend/+/3111047 Commit-Queue: Connor Clark <[email protected]> Reviewed-by: Connor Clark <[email protected]>
ref #12254 (comment)
Initially I wanted to split off the topbar features and automatically enable them in
ReportRenderer, but there are numerous issues involving how we use classes (relying on method overrides to provide/change behavior in the viewer, for example) that makes me want to break this up into two steps. This first PR is moving all the topbar features to its own class, and instantiating an instance of it inReportUIFeaturessuch that the API of that class won't change in this PR.There's some shared functionality between "topbar features" and "the other stuff" that necessitates some helper modules:
open-tab.jsandfeature-util.js(I thoughtutil.jsshould probably be free of dom stuff).As for next steps, I'm not sure yet how to untangle the
getReportHtml/saveAsGist/resetUIStatemess we have here.