Closed Bug 1447619 Opened 6 years ago Closed 6 years ago

_onLazyResize fires (and triggers sync reflow) for resize events that don't affect the browser window size

Categories

(Firefox :: Toolbars and Customization, defect, P1)

Product:
Firefox
Component:
Toolbars and Customization
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 61
Tracking Flags:
Tracking Status
firefox61 --- verified

People

(Reporter: Gijs, Assigned: Gijs)

References

(Depends on 1 open bug)

Details

(Whiteboard: [fxperf:p2])

Attachments

(1 file)

Bug 1447619 - avoid reflowing when returning items to the toolbar from the overflow menu,
59 bytes, text/x-review-board-request
florian
: review+
Details 
STR:

0. install ohnoreflow
1. open browser window
2. open devtools
3. resize devtools like mad for 5 seconds
4. dump reflows

ER:
nothing from CUI, as browser width / main window size hasn't changed

AR:
bunch of sync reflows.
Whiteboard: [fxperf]
Somehow this reminds me https://hg.mozilla.org/integration/mozilla-inbound/rev/3145f2c95016, maybe a similar fix is all we need.
Whiteboard: [fxperf] → [fxperf:p2]
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Priority: -- → P1
Comment on attachment 8969662 [details]
Bug 1447619 - avoid reflowing when returning items to the toolbar from the overflow menu,

https://reviewboard.mozilla.org/r/238470/#review244218

::: commit-message-789e3:1
(Diff revision 1)
> +Bug 1447619 - avoid reflowing when returning items to the toolbar from the overflow menu, r?florian

As may be obvious, I slightly scope-creeped this from the bug report as written, and then scope-creeped it some more from this commit message which I forgot to update. Sorry.

Anyway, the upshot of this is that the only overflow-related case where we will still sync reflow is if you use customize mode in 1 window and have overflowed stuff in another window. Or something like that. I don't really think we need to care too much about that, and I don't know what stuff relies on that being handled sync, so it feels like the effort/reward balance doesn't justify tackling that here.

I verified by using ohnoreflow and resizing the window like a maniac. :-)
If I had remembered, I would have fixed bug 1438490 first, but it's too late now. I'll do it after this lands, assuming I remember...
Blocks: 1438490
I removed the rangeParent check because it triggers sync reflow and (in my testing) is actually not effective at what it does, which means now the browser_resize test has no more remaining sync reflows.

If you put the bookmarks in the navbar and drag them from there, you can still trip this code (and make the bookmark toolbars end up in the overflow menu), but for me at least, the extant code is also not effective in preventing that from happening, so rather than having code that doesn't do anything useful but does cause sync reflow, I just removed it.

This problem might go away when we stop using XBL bindings for some of this stuff, which are causing all of the *Target properties on the event to be useless at detecting where the overflow comes from, but it might also help if we add end-margin on the bookmark toolbar items to avoid the drop indicator causing reflow in the parent (which I suspect it's doing today). Anyway, I didn't really want to fix that in this bug, we already have several bugs on file about it (e.g. bug 1403326 and others linked from there). I added back a half-baked attempt to stop overflow inside the bm toolbar from propagating, but as I said, it's not fully effective...
Blocks: 1358388
Comment on attachment 8969662 [details]
Bug 1447619 - avoid reflowing when returning items to the toolbar from the overflow menu,

https://reviewboard.mozilla.org/r/238470/#review244224

This avoids sync layout flushes at the cost of introducing slightly more flicker when opening small windows (I could see it when pressing Cmd+n for a few seconds on my fast macbook pro... it may be visible on very slow or overloaded machines), and causing us to repaint the toolbar a bit more. But I think the responsiveness win when resizing is worth it, so I'm leaning toward r+. There's just two questions below that I would like you to answer :-).

::: browser/components/customizableui/CustomizableUI.jsm:4508
(Diff revision 1)
>      while (this._list.firstChild) {
>        let child = this._list.firstChild;
>        let minSize = this._collapsed.get(child.id);
>  
>        if (!shouldMoveAllItems &&
> -          minSize &&
> +          minSize) {

nit: merge these 2 lines of the test.

Or back to the previous style with:

(targetWidth || this._target.clientWidth) <= minSize

::: browser/base/content/test/performance/browser_window_resize.js:15
(Diff revision 2)
>   * be modifying your code to avoid the reflow.
>   *
>   * See https://developer.mozilla.org/en-US/Firefox/Performance_best_practices_for_Firefox_fe_engineers
>   * for tips on how to do that.
>   */
> -const EXPECTED_REFLOWS = [
> +const EXPECTED_REFLOWS = [];

Nice :). Our other reflow tests typically have a comment urging people to not add any new whitelist entry when it's empty.

::: browser/components/customizableui/CustomizableUI.jsm:4415
(Diff revision 2)
> -  onOverflow(aEvent) {
> -    // The rangeParent check is here because of bug 1111986 and ensuring that
> -    // overflow events from the bookmarks toolbar items or similar things that
> -    // manage their own overflow don't trigger an overflow on the entire toolbar
> -    if (!this._enabled ||
> -        (aEvent && aEvent.target != this._toolbar.customizationTarget) ||
> +  /**
> +   * Avoid re-entrancy in the overflow handling by keeping track of invocations:
> +   */
> +  _lastOverflowCounter: 0,
> +
> +  /**

Can you explain why the (aEvent && aEvent.target != this._toolbar.customizationTarget) check is no longer needed?

::: browser/components/customizableui/CustomizableUI.jsm:4664
(Diff revision 2)
>        let minSize = this._collapsed.get(prevId);
>        this._collapsed.set(aNode.id, minSize);
>      } else {
>        // If it's now the first item in the overflow list,
>        // maybe we can return it:
> -      this._moveItemsBackToTheirOrigin();
> +      this._moveItemsBackToTheirOrigin(false, this._target.clientWidth);

Why is this better than letting the _moveItemsBackToTheirOrigin call the clientWidth getter itself?
(In reply to Florian Quèze [:florian] from comment #8)
> Or back to the previous style with:
> 
> (targetWidth || this._target.clientWidth) <= minSize

Hmm. I changed it to avoid fetching it multiple times. If the check against minSize passes, we will break out of the loop so then it doesn't matter, but I guess it's possible that it doesn't at first, and does after we iterate several more times, each time putting items into the toolbar's customizationTarget, which dirties the DOM which then probably causes a layout flush (even though the _target's width won't really change so hopefully layout can optimize that some... but I digress).

So I guess I'll put the other bit on 1 line but keep the separate "lazy" assignment for `targetWidth` for when it's not passed. I could write `(targetWidth || (targetWidth = this._target.clientWidth)) <= minSize)` of course but that doesn't really make things neater, IMO...
 
> ::: browser/components/customizableui/CustomizableUI.jsm:4415
> (Diff revision 2)
> > -  onOverflow(aEvent) {
> > -    // The rangeParent check is here because of bug 1111986 and ensuring that
> > -    // overflow events from the bookmarks toolbar items or similar things that
> > -    // manage their own overflow don't trigger an overflow on the entire toolbar
> > -    if (!this._enabled ||
> > -        (aEvent && aEvent.target != this._toolbar.customizationTarget) ||
> > +  /**
> > +   * Avoid re-entrancy in the overflow handling by keeping track of invocations:
> > +   */
> > +  _lastOverflowCounter: 0,
> > +
> > +  /**
> 
> Can you explain why the (aEvent && aEvent.target !=
> this._toolbar.customizationTarget) check is no longer needed?

You added the same check to the actual listener, in toolbar.xml, in bug 1434945. :-)
So we already check this before the event gets here. Hence me referencing bug 1438490 - it'll be a lot easier to follow this stuff once it's all in 1 place, but either way there's no point checking the same thing twice...

> ::: browser/components/customizableui/CustomizableUI.jsm:4664
> (Diff revision 2)
> >        let minSize = this._collapsed.get(prevId);
> >        this._collapsed.set(aNode.id, minSize);
> >      } else {
> >        // If it's now the first item in the overflow list,
> >        // maybe we can return it:
> > -      this._moveItemsBackToTheirOrigin();
> > +      this._moveItemsBackToTheirOrigin(false, this._target.clientWidth);
> 
> Why is this better than letting the _moveItemsBackToTheirOrigin call the
> clientWidth getter itself?

It's not, I forgot to delete it when I was wavering about where to put definitive responsibility for having this value (caller or callee).

Originally I intended to make *all* the callers pass this, but then found that it doesn't make sense for the callers who pass `true` for `shouldMoveAllItems` to pass it, because it's never used, and if we made them fetch it those callers add extra sync reflows, which seems dumb. But it felt fragile to have some places pass it and not others, without having a fallback in the method itself, even if today that wouldn't be hit. So then I added a fallback. But anyway with the fallback obviously there's no point for this callsite to pass it as it doesn't fetch it without avoiding a reflow (and I don't want to change this particular callsite for the reasons in comment #3).


It also seems like there's try orange for browser/components/customizableui/test/browser_976792_insertNodeInWindow.js on windows and macOS debug. Probably a race condition of sorts. I'll look into this on Monday if not before...
(In reply to :Gijs (he/him) from comment #9)
> (In reply to Florian Quèze [:florian] from comment #8)
> > Or back to the previous style with:
> > 
> > (targetWidth || this._target.clientWidth) <= minSize
> 
> Hmm. I changed it to avoid fetching it multiple times. If the check against
> minSize passes, we will break out of the loop so then it doesn't matter, but
> I guess it's possible that it doesn't at first, and does after we iterate
> several more times, each time putting items into the toolbar's
> customizationTarget, which dirties the DOM which then probably causes a
> layout flush (even though the _target's width won't really change so
> hopefully layout can optimize that some... but I digress).

Hmm. Though in fact, maybe we can just use dwu.getBoundsWithoutFlushing instead of .clientWidth to get the width of the customizationTarget, and that would avoid a flush here, too.
Comment on attachment 8969662 [details]
Bug 1447619 - avoid reflowing when returning items to the toolbar from the overflow menu,

https://reviewboard.mozilla.org/r/238470/#review245734

Thanks for the answers in the previous comments. r=me

::: browser/components/customizableui/CustomizableUI.jsm:4507
(Diff revision 3)
>        let child = this._list.firstChild;
>        let minSize = this._collapsed.get(child.id);
>  
> -      if (!shouldMoveAllItems &&
> -          minSize &&
> -          this._target.clientWidth <= minSize) {
> +      if (!shouldMoveAllItems && minSize) {
> +        if (!targetWidth) {
> +          let dwu = win.QueryInterface(Ci.nsIInterfaceRequestor).getInterface(Ci.nsIDOMWindowUtils);

nit: Looks like this is the only place where the 'win' variable is used, so it could be declared just above this line.
Attachment #8969662 - Flags: review?(florian) → review+
(In reply to Florian Quèze [:florian] from comment #12)
> Comment on attachment 8969662 [details]
> Bug 1447619 - avoid reflowing when returning items to the toolbar from the
> overflow menu,
> 
> https://reviewboard.mozilla.org/r/238470/#review245734
> 
> Thanks for the answers in the previous comments. r=me
> 
> ::: browser/components/customizableui/CustomizableUI.jsm:4507
> (Diff revision 3)
> >        let child = this._list.firstChild;
> >        let minSize = this._collapsed.get(child.id);
> >  
> > -      if (!shouldMoveAllItems &&
> > -          minSize &&
> > -          this._target.clientWidth <= minSize) {
> > +      if (!shouldMoveAllItems && minSize) {
> > +        if (!targetWidth) {
> > +          let dwu = win.QueryInterface(Ci.nsIInterfaceRequestor).getInterface(Ci.nsIDOMWindowUtils);
> 
> nit: Looks like this is the only place where the 'win' variable is used, so
> it could be declared just above this line.

I moved it from just before:

    win.UpdateUrlbarSearchSplitterState();

at the bottom of the method, so unfortunately I can't do that because it'd be out of scope (let) or perhaps undefined (var, if the loop doesn't run at all). :-)
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/1ce48405e58a
avoid reflowing when returning items to the toolbar from the overflow menu, r=florian
https://hg.mozilla.org/mozilla-central/rev/1ce48405e58a
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox61: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
status-firefox61: fixed → ---
Target Milestone: Firefox 61 → ---
Flags: needinfo?(gijskruitbosch+bugs)
Flags: needinfo?(gijskruitbosch+bugs)
Keywords: leave-open
Comment on attachment 8969662 [details]
Bug 1447619 - avoid reflowing when returning items to the toolbar from the overflow menu,

https://reviewboard.mozilla.org/r/238470/#review246386


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: browser/components/customizableui/test/browser_901207_searchbar_in_panel.js:63
(Diff revision 4)
>    Services.prefs.setBoolPref("browser.search.widget.inNavBar", true);
>  
>    window.resizeTo(kForceOverflowWidthPx, window.outerHeight);
> -  await waitForCondition(() => navbar.getAttribute("overflowing") == "true");
> +  await waitForCondition(() => {
> +    return navbar.getAttribute("overflowing") == "true" &&
> +      !navbar.querySelector("#search-container"));

Error: Parsing error: unexpected token ) [eslint]
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/4525b4e9b22d
avoid reflowing when returning items to the toolbar from the overflow menu, r=florian
https://hg.mozilla.org/mozilla-central/rev/4525b4e9b22d
Status: REOPENED → RESOLVED
Closed: 6 years ago → 6 years ago
status-firefox61: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
Flags: qe-verify+
I reproduced this issue on Windows 10 x64, Ubuntu 17.04 x64 and Mac OS X 10.12 using Nightly 61.0a1 (2018-03-21).

I could not reproduce it on the latest Nightly 61.0a1 (2018-05-06) on Windows 10 x64, Ubuntu 17.04 x64 and Mac OS X 10.12, but I found the following error in the Browser Console "Error: browser.reflows is undefined".

Gijs, could you please take a look?
Flags: needinfo?(gijskruitbosch+bugs)
I suspect that this is because the latest versions of Firefox Nightly require you to set extensions.legacy.enabled to true in order to use Oh no! Reflow.

I have independently confirmed that this bug is fixed, fwiw. I feel comfortable marking it as VERIFIED.
Status: RESOLVED → VERIFIED
Flags: needinfo?(gijskruitbosch+bugs)
Based on comment 25, I will set the tracking flag for firefox61: verified.
status-firefox61: fixed → verified
Flags: qe-verify+
Depends on: 1528656
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: