Closed
Bug 1428191
Opened 7 years ago
Closed 7 years ago
The devedition devtools toolbar icon shouldn't flicker at startup
Categories
(DevTools :: General, enhancement, P2)
DevTools
General
Tracking
(firefox59 verified)
VERIFIED
FIXED
Firefox 59
Tracking | Status | |
---|---|---|
firefox59 | --- | verified |
People
(Reporter: florian, Assigned: jdescottes)
References
Details
Attachments
(2 files)
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
text/x-review-board-request
|
ochameau
:
review+
|
Details |
browser/base/content/test/performance/browser_startup_flicker.js shows the devtools toolbar icon flickering at startup.
In bug 1426559, we are adding an exception in the test.
Assignee | ||
Comment 1•7 years ago
|
||
Currently, the developer-button is dynamically created in devtools-startup.js when receiving the browser-delayed-startup-finished event for a given window [1].
[1] https://searchfox.org/mozilla-central/rev/652fbd6270de0d3ec424d2b88f8375ff546f949f/devtools/shim/devtools-startup.js#331-392
Assignee | ||
Comment 2•7 years ago
|
||
My previous comment was vague -> this is only done for the first browser-delayed-startup-finished event we receive.
As discussed on IRC, we should find a way to add this button to the UI before the first paint for dev edition. In other channels, as the button is not visible by default, it's fine to delay its creation until the first window is initialized (even though it means there will still be a flickr for users who customized the button to be in their navbar manually?)
Potential spot to create the button could be https://searchfox.org/mozilla-central/source/browser/components/nsBrowserGlue.js#668
Could:
- add a method to create the button
- if dev-edition: call it in _beforeUIStartup()
- otherwise: call it in _onFirstWindowLoaded()
The only dependency on DevTools code from this button is a call to initDevTools() in onViewShowing.
Reporter | ||
Comment 3•7 years ago
|
||
This is enough to avoid the flicker.
The
if (AppConstants.MOZ_DEV_EDITION) {
item.defaultArea = CustomizableUI.AREA_NAVBAR;
}
code that I'm removing races with https://searchfox.org/mozilla-central/rev/652fbd6270de0d3ec424d2b88f8375ff546f949f/browser/components/customizableui/CustomizableUI.jsm#202 and causes the icon to be added at the end of the toolbar instead of at its expected position.
The shortcutId is commented out because the key is created later by https://searchfox.org/mozilla-central/rev/652fbd6270de0d3ec424d2b88f8375ff546f949f/devtools/shim/devtools-startup.js#71
This causes this error during startup:
console.error: CustomizableUI:
Key element with id 'key_toggleToolbox' for widget 'developer-button' not found!
I don't know if the wrappedJSObject hack is good enough or if a cleaner solution needs to be implemented, that'll be up to whoever reviews the code I guess.
Reporter | ||
Comment 4•7 years ago
|
||
(In reply to Florian Quèze [:florian] from comment #0)
> browser/base/content/test/performance/browser_startup_flicker.js shows the
> devtools toolbar icon flickering at startup.
>
> In bug 1426559, we are adding an exception in the test.
I forgot to mention that in bug 1426559 we had to change our plan: on my fast local machine it was only the devtools icon that flickered and it was possible to add an exception, but on try the icon was added after first paint, causing most of the toolbar content to shift. Adding an exception that broad would make the test pretty much useless. So let's fix the flicker (ie. this bug) instead of adding an exception for it in the test.
Updated•7 years ago
|
Updated•7 years ago
|
Comment hidden (mozreview-request) |
Assignee | ||
Comment 6•7 years ago
|
||
Thanks for the POC!
Slightly modified it to keep the code constrained to devtools-startup. I think creating the widget from the handle() method of devtools-startup achieves the same goal without having to link browserglue to devtools. At least it also fixes the flicker test in dev-edition. handle() is called after the final-ui-startup event is fired, but still before the first paint.
I have to hook keyshortcuts before the creation of the widget, so I'm adding a map of windows to allow hookKeyShortcuts to be safely called several times for the same target.
Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=03cfa199c80654bb0ce1e2f50bd238717e607b1b
Reporter | ||
Comment 7•7 years ago
|
||
mozreview-review |
Comment on attachment 8940205 [details]
Bug 1428191 - create developer toggle widget before first paint in DevEdition;
https://reviewboard.mozilla.org/r/210518/#review216158
::: devtools/shim/devtools-startup.js:383
(Diff revision 1)
> // it right away.
> this.onBeforeCreated(anchor.ownerDocument);
> },
> - onBeforeCreated(doc) {
> + onBeforeCreated: (doc) => {
> + // The developer toggle needs the "key_toggleToolbox" <key> element.
> + // In DEV EDITION, the toggle is added before 1st paint in onDevEditionUiStartup()
Is onDevEditionUiStartup a function that exists somewhere?
::: devtools/shim/devtools-startup.js:526
(Diff revision 1)
> + _hookKeyShortcutsWindows: new WeakMap(),
> +
> hookKeyShortcuts(window) {
> + // hookKeyShortcuts can be called both from hookWindow and from the developer toggle
> + // onBeforeCreated. Make sure shortcuts are only added once per window.
> + if (this._hookKeyShortcutsWindows.get(window)) {
Isn't a null check on doc.getElementById("devtoolsKeyset") enough?
Assignee | ||
Comment 8•7 years ago
|
||
(In reply to Florian Quèze [:florian] from comment #7)
> Comment on attachment 8940205 [details]
> Bug 1428191 - create developer toggle widget before first paint in
> DevEdition;
>
> https://reviewboard.mozilla.org/r/210518/#review216158
>
> ::: devtools/shim/devtools-startup.js:383
> (Diff revision 1)
> > // it right away.
> > this.onBeforeCreated(anchor.ownerDocument);
> > },
> > - onBeforeCreated(doc) {
> > + onBeforeCreated: (doc) => {
> > + // The developer toggle needs the "key_toggleToolbox" <key> element.
> > + // In DEV EDITION, the toggle is added before 1st paint in onDevEditionUiStartup()
>
> Is onDevEditionUiStartup a function that exists somewhere?
Thanks for catching this, I got rid of it and forgot to update my comment.
>
> ::: devtools/shim/devtools-startup.js:526
> (Diff revision 1)
> > + _hookKeyShortcutsWindows: new WeakMap(),
> > +
> > hookKeyShortcuts(window) {
> > + // hookKeyShortcuts can be called both from hookWindow and from the developer toggle
> > + // onBeforeCreated. Make sure shortcuts are only added once per window.
> > + if (this._hookKeyShortcutsWindows.get(window)) {
>
> Isn't a null check on doc.getElementById("devtoolsKeyset") enough?
It would. I did this based on the assumption that getElementById would have a higher cost here. I might be wrong.
Comment hidden (mozreview-request) |
Updated•7 years ago
|
Assignee: nobody → jdescottes
Comment 10•7 years ago
|
||
Comment on attachment 8940205 [details]
Bug 1428191 - create developer toggle widget before first paint in DevEdition;
I included this patch in my last round of 59-as-Beta Try simulations and it appears to work nicely :)
Attachment #8940205 -
Flags: feedback+
Updated•7 years ago
|
Priority: -- → P2
Comment 11•7 years ago
|
||
mozreview-review |
Comment on attachment 8940205 [details]
Bug 1428191 - create developer toggle widget before first paint in DevEdition;
https://reviewboard.mozilla.org/r/210518/#review216710
Looks good, thanks for looking into this!
::: devtools/shim/devtools-startup.js:528
(Diff revision 2)
> hookKeyShortcuts(window) {
> + // hookKeyShortcuts can be called both from hookWindow and from the developer toggle
> + // onBeforeCreated. Make sure shortcuts are only added once per window.
> + if (this._hookKeyShortcutsWindows.get(window)) {
> + return;
> + }
Rather than introducing a WeakMap, I would check for "devtoolsKeyset" existence.
Attachment #8940205 -
Flags: review?(poirot.alex) → review+
Comment hidden (mozreview-request) |
Assignee | ||
Comment 13•7 years ago
|
||
Thanks for the review, updated to use getElementById("devtoolsKeyset").
New try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3e2cad0e16bcd5d90da71442f10f103074b2f457
Comment 14•7 years ago
|
||
That latest version of the patch appears to have a lot of issues :(
https://treeherder.mozilla.org/logviewer.html#?job_id=154852911&repo=try
https://treeherder.mozilla.org/logviewer.html#?job_id=154853032&repo=try
https://treeherder.mozilla.org/logviewer.html#?job_id=154852927&repo=try
https://treeherder.mozilla.org/logviewer.html#?job_id=154853026&repo=try
I saw these same failures in my latest simulation run today too after adding the patch to my queue.
Flags: needinfo?(jdescottes)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 16•7 years ago
|
||
Sorry, forgot to remove a reference to the previous implementation. New patch should be good to go.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=aa4ff5b28dd53161382179437b01705ce8222824
Flags: needinfo?(jdescottes)
Comment 17•7 years ago
|
||
Pushed by jdescottes@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/024c63779e3b
create developer toggle widget before first paint in DevEdition;r=ochameau
Comment 18•7 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59
Updated•7 years ago
|
Status: RESOLVED → VERIFIED
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•