Closed Bug 1368208 Opened 7 years ago Closed 7 years ago

Don't flush layout to determine whether scrollbox scroll buttons should be enabled/disabled

Categories

(Toolkit :: UI Widgets, defect, P1)

Product:
Toolkit
Component:
UI Widgets
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla56
Iteration:
56.1 - Jun 26
Project Flags:
Performance Impact high
Tracking Flags:
Tracking Status
firefox56 --- fixed

People

(Reporter: dao, Assigned: dao)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

(Keywords: perf, Whiteboard: [ohnoreflow][photon-performance])

Attachments

(1 file)

Bug 1368208 - Don't flush layout to determine whether scrollbox scroll buttons should be enabled/disabled
59 bytes, text/x-review-board-request
mconley
: review+
Details 
+++ This bug was initially created as a clone of Bug #1358453 +++

Following up from bug 1358453...

It should be possible to use MozAfterPaint and getBoundsWithoutFlushing to reliably update the scroll buttons.
Flags: qe-verify-
No longer blocks: 1356153
Iteration: 55.6 - May 29 → ---
Blocks: 1356153
Comment on attachment 8872029 [details]
Bug 1368208 - Don't flush layout to determine whether scrollbox scroll buttons should be enabled/disabled

https://reviewboard.mozilla.org/r/143550/#review147512

::: toolkit/content/widgets/scrollbox.xml:188
(Diff revision 1)
>            innerRect.right = innerRect.left + this._scrollbox.scrollWidth;
>            innerRect.bottom = innerRect.top + this._scrollbox.scrollHeight;
>            return innerRect;
>          ]]></getter>
>        </property>
> -      <property name="scrollboxPaddingStart" readonly="true">
> +      <field name="scrollboxPaddingStart"><![CDATA[

If I remember correctly, field values are computed lazily the first time they are used; do we need to invalidate this when the theme changes?

::: toolkit/content/widgets/scrollbox.xml:616
(Diff revision 1)
>          ]]></body>
>        </method>
>  
> +      <field name="_scrollButtonUpdatePending">false</field>
>        <method name="_updateScrollButtonsDisabledState">
>          <parameter name="aScrolling"/>

nit: aScrolling is no longer used.

::: toolkit/content/widgets/scrollbox.xml:618
(Diff revision 1)
>  
> +      <field name="_scrollButtonUpdatePending">false</field>
>        <method name="_updateScrollButtonsDisabledState">
>          <parameter name="aScrolling"/>
>          <body><![CDATA[
> -          let scrolledToStart;
> +          if (this._scrollButtonUpdatePending) {

Just the use of the _scrollButtonUpdatePending boolean here to avoid updating the buttons several times in the same frame probably saves us a lot of time when closing several tabs at once (eg. bug 1356153).

::: toolkit/content/widgets/scrollbox.xml:635
(Diff revision 1)
> +            return;
> +          }
> +
> +          // Wait until after the next paint to get current layout data without
> +          // flushing layout. Use nested requestAnimationFrame rather than
> +          // MozAfterPaint to invalidate layout as late as possible.

Can you expand a bit on what you mean by "Use nested requestAnimationFrame rather than MozAfterPaint to invalidate layout as late as possible".

If I understood Mike's explanations correctly, we should change the dom in requestAnimationFrame but avoid doing things that flush there. And I guess the MozAfterPaint event would be a reasonable place to get computed layout values.

So based on this understanding I would expect something like:
function onMozAfterPaint() {
  /* do all the stuff that flushes */
  requestAnimationFrame(() => {
    this.setAttribute(...
  });
}

::: toolkit/content/widgets/scrollbox.xml:657
(Diff revision 1)
> -            scrolledToEnd = this._isRTLScrollbox;
> -            scrolledToStart = !this._isRTLScrollbox;
> -          } else if (this.scrollClientSize + this.scrollPosition == this.scrollSize) {
> -            // In the RTL case, this means the _first_ element in the
> -            // scrollbox is visible
> -            scrolledToStart = this._isRTLScrollbox;
> +                  scrollboxPaddingStart = -scrollboxPaddingStart;
> +                  scrollboxPaddingEnd = -scrollboxPaddingEnd;
> +                }
> +                let windowUtils = window.QueryInterface(Ci.nsIInterfaceRequestor)
> +                                        .getInterface(Ci.nsIDOMWindowUtils);
> +                let rect = ele => { return windowUtils.getBoundsWithoutFlushing(ele); };

nit: you can omit '{ return ' and '};' here.

::: toolkit/content/widgets/scrollbox.xml:659
(Diff revision 1)
> -          } else if (this.scrollClientSize + this.scrollPosition == this.scrollSize) {
> -            // In the RTL case, this means the _first_ element in the
> -            // scrollbox is visible
> -            scrolledToStart = this._isRTLScrollbox;
> -            scrolledToEnd = !this._isRTLScrollbox;
> +                }
> +                let windowUtils = window.QueryInterface(Ci.nsIInterfaceRequestor)
> +                                        .getInterface(Ci.nsIDOMWindowUtils);
> +                let rect = ele => { return windowUtils.getBoundsWithoutFlushing(ele); };
> +                let scrollboxRect = rect(this._scrollbox);
> +                let elements = this._getScrollableElements();

Seeing the getBoundsWithoutFlushing usage makes me assume you intend this function to not flush... but _getScrollableElements calls _canScrollToElement which is implemented as 'window.getComputedStyle(element).display != "none"'.
(In reply to Florian Quèze [:florian] [:flo] (away until Monday May 29th) from comment #2)

> ::: toolkit/content/widgets/scrollbox.xml:635
> (Diff revision 1)
> > +            return;
> > +          }
> > +
> > +          // Wait until after the next paint to get current layout data without
> > +          // flushing layout. Use nested requestAnimationFrame rather than
> > +          // MozAfterPaint to invalidate layout as late as possible.
> 
> Can you expand a bit on what you mean by "Use nested requestAnimationFrame
> rather than MozAfterPaint to invalidate layout as late as possible".
> 
> If I understood Mike's explanations correctly, we should change the dom in
> requestAnimationFrame but avoid doing things that flush there. And I guess
> the MozAfterPaint event would be a reasonable place to get computed layout
> values.
> 
> So based on this understanding I would expect something like:
> function onMozAfterPaint() {
>   /* do all the stuff that flushes */
>   requestAnimationFrame(() => {
>     this.setAttribute(...
>   });
> }

needinfo on Mike who may have interesting input here too.
Flags: needinfo?(mconley)
(In reply to Florian Quèze [:florian] [:flo] (away until Monday May 29th) from comment #2)
> ::: toolkit/content/widgets/scrollbox.xml:188
> (Diff revision 1)
> >            innerRect.right = innerRect.left + this._scrollbox.scrollWidth;
> >            innerRect.bottom = innerRect.top + this._scrollbox.scrollHeight;
> >            return innerRect;
> >          ]]></getter>
> >        </property>
> > -      <property name="scrollboxPaddingStart" readonly="true">
> > +      <field name="scrollboxPaddingStart"><![CDATA[
> 
> If I remember correctly, field values are computed lazily the first time
> they are used; do we need to invalidate this when the theme changes?

This property is only relevant for the tab strip, and it shouldn't change when applying a lightweight theme.

> ::: toolkit/content/widgets/scrollbox.xml:635
> (Diff revision 1)
> > +            return;
> > +          }
> > +
> > +          // Wait until after the next paint to get current layout data without
> > +          // flushing layout. Use nested requestAnimationFrame rather than
> > +          // MozAfterPaint to invalidate layout as late as possible.
> 
> Can you expand a bit on what you mean by "Use nested requestAnimationFrame
> rather than MozAfterPaint to invalidate layout as late as possible".
> 
> If I understood Mike's explanations correctly, we should change the dom in
> requestAnimationFrame but avoid doing things that flush there. And I guess
> the MozAfterPaint event would be a reasonable place to get computed layout
> values.
> 
> So based on this understanding I would expect something like:
> function onMozAfterPaint() {
>   /* do all the stuff that flushes */
>   requestAnimationFrame(() => {
>     this.setAttribute(...
>   });
> }

Yes, MozAfterPaint -> requestAnimationFrame works too. I guess if _updateScrollButtonsDisabledState is called after the last rAF tick and before that frame is painted, this would make a difference, but I'm not sure this is possible in practice.

> ::: toolkit/content/widgets/scrollbox.xml:659
> (Diff revision 1)
> > -          } else if (this.scrollClientSize + this.scrollPosition == this.scrollSize) {
> > -            // In the RTL case, this means the _first_ element in the
> > -            // scrollbox is visible
> > -            scrolledToStart = this._isRTLScrollbox;
> > -            scrolledToEnd = !this._isRTLScrollbox;
> > +                }
> > +                let windowUtils = window.QueryInterface(Ci.nsIInterfaceRequestor)
> > +                                        .getInterface(Ci.nsIDOMWindowUtils);
> > +                let rect = ele => { return windowUtils.getBoundsWithoutFlushing(ele); };
> > +                let scrollboxRect = rect(this._scrollbox);
> > +                let elements = this._getScrollableElements();
> 
> Seeing the getBoundsWithoutFlushing usage makes me assume you intend this
> function to not flush... but _getScrollableElements calls
> _canScrollToElement which is implemented as
> 'window.getComputedStyle(element).display != "none"'.

Tabbrowser overrides this with a method that doesn't use getComputedStyle. The generic case that the scrollbox tries to handle is more tricky...
(In reply to Dão Gottwald [::dao] from comment #4)
> Yes, MozAfterPaint -> requestAnimationFrame works too. I guess if
> _updateScrollButtonsDisabledState is called after the last rAF tick and
> before that frame is painted, this would make a difference, but I'm not sure
> this is possible in practice.

By make a difference I mean MozAfterPaint -> requestAnimationFrame would catch an earlier frame than requestAnimationFrame -> requestAnimationFrame. Usually they should end up with the same frame.
MozAfterPaint is a little tricky because of what it means - it means "layers in this window have been painted, and _received_ by the compositor thread". This is the closest, I believe, we can get to "has been presented to the user".

The problem with that, is that a MozAfterPaint event handler can be called for a paint that is not related to what you care about - it might fire, for example, for the _last_ paint and composite. I wrote about this in [1], and added some workarounds to nsIDOMWindowUtils to help with this. Also see the example code in [2].

So it's possible that MozAfterPaint is a fine place to put your computed style and layout values, but after talking to the folks in #content, what they suggest instead is to either:

a) Queue a runnable in requestAnimationFrame
b) Use a setTimeout in requestAnimationFrame

The runnable / setTimeout functions will be run "asap" after the paint has occurred, which is the best time to get the computed style and layout information.

[1]: https://groups.google.com/forum/#!searchin/mozilla.dev.platform/MozAfterPaint/mozilla.dev.platform/pCLwWdYc_GY/j9A-vWm3AgAJ
[2]: https://developer.mozilla.org/en-US/docs/Web/Events/MozAfterPaint
Flags: needinfo?(mconley)
Comment on attachment 8872029 [details]
Bug 1368208 - Don't flush layout to determine whether scrollbox scroll buttons should be enabled/disabled

https://reviewboard.mozilla.org/r/143550/#review147600

::: toolkit/content/widgets/scrollbox.xml:644
(Diff revision 2)
> +          // Wait until after the next paint to get current layout data without
> +          // flushing layout.
> +          window.addEventListener("MozAfterPaint", () => {

This relates to my previous comment, I guess - this MozAfterPaint event handler might fire for the _last_ paint that occurred.
(In reply to Mike Conley (:mconley) from comment #8)
> Comment on attachment 8872029 [details]
> Bug 1368208 - Don't flush layout to determine whether scrollbox scroll
> buttons should be enabled/disabled.
> 
> https://reviewboard.mozilla.org/r/143550/#review147600
> 
> ::: toolkit/content/widgets/scrollbox.xml:644
> (Diff revision 2)
> > +          // Wait until after the next paint to get current layout data without
> > +          // flushing layout.
> > +          window.addEventListener("MozAfterPaint", () => {
> 
> This relates to my previous comment, I guess - this MozAfterPaint event
> handler might fire for the _last_ paint that occurred.

In other words, layout data may be stale here from the perspective of _updateScrollButtonsDisabledState's caller? So I guess I'll go back to nesting requestAnimationFrame.
Comment on attachment 8872029 [details]
Bug 1368208 - Don't flush layout to determine whether scrollbox scroll buttons should be enabled/disabled

Forwarding the review request to Mike who has more expertise here than I do.
Attachment #8872029 - Flags: review?(florian) → review?(mconley)
Comment on attachment 8872029 [details]
Bug 1368208 - Don't flush layout to determine whether scrollbox scroll buttons should be enabled/disabled

https://reviewboard.mozilla.org/r/143550/#review147612

Hey Dao,

This looks great! Thanks so much for working on this stuff. Just a few notes below - let me know if you have any questions.

::: toolkit/content/widgets/scrollbox.xml:644
(Diff revision 3)
> +          // Wait until after the next paint to get current layout data without
> +          // flushing layout.
> +          window.requestAnimationFrame(() => {
> +            window.requestAnimationFrame(() => {

So what we're doing here is moving this JS to run not at the end of the original tick (A), but at the end of the _next_ tick (B). This means that, yes, any DOM changes you make in (A) will not cause us to flush layout within (B), but any DOM changes that occurred between (A) or (B) might.

What I'm saying, is that we run the risk here of having too big a gap in time, where the DOM might be dirtied, and inside your inner-most rAF, we'll still cause a layout flush.

We can reduce this probability, by running inner-most JS earlier than (B). Yes, after the (A) tick, but as close to the _start_ of the (B) tick as possible.

So what I suggest instead is either:

```js
window.requestAnimationFrame(() => {
  setTimeout(() => {
    this._scrollButtonUpdatePending = false;
    // ...
  }, 0);
});
```

or

```js
window.requestAnimationFrame(() => {
  Services.tm.dispatchToMain(() => {
    this._scrollButtonUpdatePending = false;
    // ...
  });
});
```

if you want to avoid the timer overhead.

::: toolkit/content/widgets/scrollbox.xml:661
(Diff revision 3)
> -          } else if (aScrolling) {
> -            scrolledToStart = false;
> -            scrolledToEnd = false;
> -          } else if (this.scrollPosition == 0) {
> -            // In the RTL case, this means the _last_ element in the
> -            // scrollbox is visible
> +              } else {
> +                let scrollboxPaddingStart = Math.round(this.scrollboxPaddingStart);
> +                let scrollboxPaddingEnd = Math.round(this.scrollboxPaddingEnd);
> +                let [start, end] = this._startEndProps;
> +                if (this._isRTLScrollbox) {
> +                  [start, end] = [end, start];

Neat. :) Never thought of swapping that way.

::: toolkit/content/widgets/scrollbox.xml:682
(Diff revision 3)
> -          if (scrolledToEnd)
> +              if (scrolledToEnd) {
> -            this.setAttribute("scrolledtoend", "true");
> +                this.setAttribute("scrolledtoend", "true");
> -          else
> +              } else {
> -            this.removeAttribute("scrolledtoend");
> +                this.removeAttribute("scrolledtoend");
> +              }
>  
> -          if (scrolledToStart)
> +              if (scrolledToStart) {
> -            this.setAttribute("scrolledtostart", "true");
> +                this.setAttribute("scrolledtostart", "true");
> -          else
> +              } else {
> -            this.removeAttribute("scrolledtostart");
> +                this.removeAttribute("scrolledtostart");
> +              }

We might as well do this inside a requestAnimationFrame as well, if we're going to all of the trouble to avoid flushes - that'll make it so that other code that runs in this tick has less probability of causing a layout flush.
Attachment #8872029 - Flags: review?(mconley) → review-
(In reply to Mike Conley (:mconley) from comment #12)
> What I'm saying, is that we run the risk here of having too big a gap in
> time, where the DOM might be dirtied, and inside your inner-most rAF, we'll
> still cause a layout flush.

Where are we flushing layout?
Flags: needinfo?(mconley)
(In reply to Dão Gottwald [::dao] from comment #13)
> Where are we flushing layout?

Bah, sorry, I meant flushing styles. We'll, I believe, be flushing styles inside your scrollboxPaddingStart and scrollboxPaddingEnd calls.
Flags: needinfo?(mconley)
(In reply to Mike Conley (:mconley) from comment #14)
> (In reply to Dão Gottwald [::dao] from comment #13)
> > Where are we flushing layout?
> 
> Bah, sorry, I meant flushing styles. We'll, I believe, be flushing styles
> inside your scrollboxPaddingStart and scrollboxPaddingEnd calls.

XBL fields have lazy getters, they run once and then the value is kept.
(We'll also get rid of scrollboxPaddingStart and scrollboxPaddingEnd in bug 1349555.)
Comment on attachment 8872029 [details]
Bug 1368208 - Don't flush layout to determine whether scrollbox scroll buttons should be enabled/disabled

https://reviewboard.mozilla.org/r/143550/#review147612

> We might as well do this inside a requestAnimationFrame as well, if we're going to all of the trouble to avoid flushes - that'll make it so that other code that runs in this tick has less probability of causing a layout flush.

Of course this is already inside a rAF. Ignore this.
Comment on attachment 8872029 [details]
Bug 1368208 - Don't flush layout to determine whether scrollbox scroll buttons should be enabled/disabled

https://reviewboard.mozilla.org/r/143550/#review147652

::: toolkit/content/widgets/scrollbox.xml:644
(Diff revision 3)
> +          // Wait until after the next paint to get current layout data without
> +          // flushing layout.

Ah, okay! I understand! You're not somehow avoiding flushes with those rAF's - you're avoiding _stale information inside the getBoundsWithoutFlushing calls_. Okay, I understand now.

In that case, I think the above comment can probably be made more clear. Instead of saying "get current layout data without flushing layout", perhaps we can directly reference getBoundsWithoutFlushing, and say that we're waiting until after the next paint to avoid stale layout data.
Attachment #8872029 - Flags: review- → review+
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c0c635e938fa
Don't flush layout to determine whether scrollbox scroll buttons should be enabled/disabled. r=mconley
(In reply to Phil Ringnalda (:philor) from comment #21)
> Backed out in https://hg.mozilla.org/integration/autoland/rev/2d19d1d3f8aa
> for eslint
> (https://treeherder.mozilla.org/logviewer.
> html#?job_id=102879886&repo=autoland)

4618 TEST-UNEXPECTED-ERROR | /home/worker/checkouts/gecko/toolkit/content/widgets/scrollbox.xml:189:12 | Parsing error: Unexpected token paddingStart (eslint)

I don't understand what this is complaining about... I guess I'll try removing the paddingStart and paddingEnd variables in the XBL fields.

> and reftest
> (https://treeherder.mozilla.org/logviewer.
> html#?job_id=102882864&repo=autoland) failures.

Looks like this is running unprivileged and then we get this:

JavaScript error: chrome://global/content/bindings/scrollbox.xml, line 237: TypeError: window.QueryInterface(...).getInterface is not a function
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8901a598369f
Don't flush layout to determine whether scrollbox scroll buttons should be enabled/disabled. r=mconley
eslint, you're really annoying.
Flags: needinfo?(dao+bmo)
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9fc7556f5386
Don't flush layout to determine whether scrollbox scroll buttons should be enabled/disabled. r=mconley
backed out for eslint failure like https://treeherder.mozilla.org/logviewer.html#?job_id=103030382&repo=autoland
Flags: needinfo?(dao+bmo)
Backout by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7d03173f5828
Backed out changeset 9fc7556f5386 for eslint failures
(In reply to Carsten Book [:Tomcat] from comment #30)
> backed out for eslint failure like
> https://treeherder.mozilla.org/logviewer.html#?job_id=103030382&repo=autoland

TEST-UNEXPECTED-ERROR | /home/worker/checkouts/gecko/toolkit/content/widgets/scrollbox.xml:196:10 | Do not nest ternary expressions. (no-nested-ternary)

I'm nesting ternary expressions to work around the obscure failure from comment 22. FWIW, the code actually worked fine despite eslint failing to parse it. Same for comment 26. This is killing my productivity. Mark, please advise how I should proceed here.
Flags: needinfo?(dao+bmo) → needinfo?(standard8)
The fact this didn't show on try is due to bug 1368733. I'm looking at the actual issue in a bit.
(In reply to Dão Gottwald [::dao] from comment #32)
> (In reply to Carsten Book [:Tomcat] from comment #30)
> > backed out for eslint failure like
> > https://treeherder.mozilla.org/logviewer.html#?job_id=103030382&repo=autoland
> 
> TEST-UNEXPECTED-ERROR |
> /home/worker/checkouts/gecko/toolkit/content/widgets/scrollbox.xml:196:10 |
> Do not nest ternary expressions. (no-nested-ternary)
> 
> I'm nesting ternary expressions to work around the obscure failure from
> comment 22.

Actually this isn't true, the nested ternary expression was in my earlier patch but masked by the parsing error. Ideally I'd like to keep this expression, as I think it's quite readable.
I'm going to drop 'this.orient == "vertical" ? "paddingTop" :' from the expression and attempt to land this a fourth time. This means that scrollboxPaddingStart and scrollboxPaddingEnd will continue to be wrong for vertical scrollboxes, but I don't think we hit this case in practice.
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9bda0f33f7b5
Don't flush layout to determine whether scrollbox scroll buttons should be enabled/disabled. r=mconley
The parser that was written for xbl provides a modified version to ESLint. For field elements, it pretends it is a getter and wraps the code segment in a return(), e.g.:

      get scrollboxPaddingStart() { // eslint-disable-line
        return ( // eslint-disable-line
          parseFloat(window.getComputedStyle(this._scrollbox)[
            this.orient == "vertical" ? "paddingTop" :
              (this._isRTLScrollbox ? "paddingRight" : "paddingLeft")
          ])
        ); // eslint-disable-line
      }, // eslint-disable-line

The cases in comment 21 and comment 26 are therefore found to be errors due to the return wrapping of what is seen as multiple statements.

I took a look at https://developer.mozilla.org/docs/Mozilla/Tech/XBL/XBL_1.0_Reference/Elements#field and https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/Tutorial/Adding_Properties_to_XBL-defined_Elements#Fields

From my understanding of those, it seems like the field's initializer can only have one line of code that is expected to return the value. So I think these are possibly invalid xml, although it is a bit strange that the tests seemed to pass. I'm guessing we don't have other instances of field elements with multiple statements in the tree, as we'd be seeing failures for those as well.
Flags: needinfo?(standard8)
Could the semi-colon at the end of the field value be part of the problem?
(In reply to Florian Quèze [:florian] [:flo] from comment #41)
> Could the semi-colon at the end of the field value be part of the problem?

The preprocessor automatically strips that.
(In reply to Mark Banner (:standard8) from comment #40)
> From my understanding of those, it seems like the field's initializer can
> only have one line of code that is expected to return the value.

No, this is not correct.

> So I think
> these are possibly invalid xml, although it is a bit strange that the tests
> seemed to pass.

I tested this manually too.
(In reply to Ryan VanderMeulen [:RyanVM] from comment #39)
> Backed out for 508816-1-ref.xul failures. Scroll button highlighting
> differences from the looks of it.
> https://hg.mozilla.org/integration/autoland/rev/
> 02c2ade8e3ece2b1fd39df5bac82e78bb62cb3d1
> 
> https://treeherder.mozilla.org/logviewer.html#?job_id=103146668&repo=autoland

There appears to be a race condition exposed by that reftest. In _updateScrollButtonsDisabledState, the early this.hasAttribute("notoverflowing") check + requestAnimationFrame + _scrollButtonUpdatePending makes us miss the initial overflow event from 508816-1-ref.xul's arrowscrollbox that would remove the notoverflowing attribute.
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/17792fad126f
Don't flush layout to determine whether scrollbox scroll buttons should be enabled/disabled. r=mconley
had to back this out again for the reftest failures like https://treeherder.mozilla.org/logviewer.html#?job_id=103350489&repo=autoland in the same test as mentioned in #comment 44
Flags: needinfo?(dao+bmo)
Backout by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d840ee711bc8
Backed out changeset 17792fad126f for reftestfailures in 508816-1.xul
(In reply to Carsten Book [:Tomcat] from comment #47)
> had to back this out again for the reftest failures like
> https://treeherder.mozilla.org/logviewer.
> html#?job_id=103350489&repo=autoland in the same test as mentioned in
> #comment 44

Green on Linux and Mac, orange on Windows. Sigh.
Flags: needinfo?(dao+bmo)
I don't understand what's going on with 508816-1.xul on Windows, but queuing at least three requestAnimationFrame calls in both 508816-1.xul and 508816-1-ref.xul before doing the comparison seems to fix it... none of this seems to be needed on Linux and Mac.
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4b82f58570e4
Don't flush layout to determine whether scrollbox scroll buttons should be enabled/disabled. r=mconley
Still failing on QR builds. Can we please get a fully green Try push across all platforms before this lands and burns the tree again? |try: -b do -p all -u reftest[-10.6,-Windows XP] -t none| should get you what you want.
https://hg.mozilla.org/integration/autoland/rev/cd807be171322fd50de8709de13f240121259933

https://treeherder.mozilla.org/logviewer.html#?job_id=103529725&repo=autoland
https://treeherder.mozilla.org/logviewer.html#?job_id=103531016&repo=autoland
Whiteboard: [ohnoreflow][qf][photon-performance] → [ohnoreflow][qf:p1][photon-performance]
Depends on: 1371604
Depends on: 1371620
I think I'll land this with 508816-1.xul disabled. The scroll buttons seem to suffer from some weird invalidation bug that I can reproduce even in nightly (e.g. in the all tabs list or in the bookmarks menu). My patch doesn't seem to make this worse but changes the timing such the reftest now exposes this bug.
Depends on: 1375012
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ea7d29871244
Don't flush layout to determine whether scrollbox scroll buttons should be enabled/disabled r=mconley
https://hg.mozilla.org/mozilla-central/rev/ea7d29871244
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Iteration: --- → 56.1 - Jun 26
Depends on: 1376397
Depends on: 1465730
Performance Impact: --- → P1
Whiteboard: [ohnoreflow][qf:p1][photon-performance] → [ohnoreflow][photon-performance]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: