Closed Bug 1428191 Opened 6 years ago Closed 6 years ago

The devedition devtools toolbar icon shouldn't flicker at startup

Categories

(DevTools :: General, enhancement, P2)

Product:
DevTools
Component:
General
Type:
enhancement

Tracking

(firefox59 verified)

Status:
VERIFIED FIXED
Milestone:
Firefox 59
Tracking Flags:
Tracking Status
firefox59 --- verified

People

(Reporter: florian, Assigned: jdescottes)

References

Details

Attachments

(2 files)

Proof of concept patch
3.66 KB, patch
Details | Diff | Splinter Review
Bug 1428191 - create developer toggle widget before first paint in DevEdition;
59 bytes, 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.
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
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.
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.
(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.
See Also: → 1248603
See Also: → 1418377
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
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?
(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.
Assignee: nobody → jdescottes
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+
Priority: -- → P2
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+
Thanks for the review, updated to use getElementById("devtoolsKeyset").

New try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3e2cad0e16bcd5d90da71442f10f103074b2f457
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)
Pushed by jdescottes@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/024c63779e3b
create developer toggle widget before first paint in DevEdition;r=ochameau
https://hg.mozilla.org/mozilla-central/rev/024c63779e3b
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox59: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59
Status: RESOLVED → VERIFIED
status-firefox59: fixed → verified
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: