904533 - APZ panning on about:start tab is wonky

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Jim Mathies [:jimm]]

Spin off from bug 902937.

We get weird scrollTo values when the browser we are panning has an offset set on it. 50% of the padding value. This causes the page to jump around a lot.

> Seeing some weird scrollto values here with apz enabled. The scrollTo.y
> value is always -34. 
> 
> MetroWidget::SendAsyncScrollDOMEvent
> MetroWidget::SendAsyncScrollDOMEvent
> MetroWidget::RequestContentRepaint
> APZC scrollId: 6 
> APZC scrollTo.x: 302, scrollTo.y: -34 
> APZC setResolution: 1 
> APZC setDisplayPortForElement: displayPort.x: -302, displayPort.y: 0,
> displayPort.width: 2042, displayort.height: 774 

We can work around this by giving up the scrollbar on the bottom of the browser when about:start is open. I think this would be a acceptable, although this is not the only case where we add padding on the browser element. We also do this for form input and for find bar results, so there may be other problems lurking for certain web pages.

Comment 1

[Jim Mathies [:jimm]]

I've tracked down the cause of this, still trying to figure out how to address it.

The start page sits in a browser and fills the screen, but there's a bottom section which is obscured by a navbar overlay. In order to get the browser scrollbars visible above the navbar, we add some padding on the bottom that pushes the base of the browser up so that it aligns with the top of the navbar.

In frame metrics we have mCompositionBounds which is pulled from the widget's bounds - 

http://mxr.mozilla.org/mozilla-central/source/layout/base/nsDisplayList.cpp#677

and we have mScrollableRect which is pulled from the scrollable frame - 

http://mxr.mozilla.org/mozilla-central/source/layout/base/nsDisplayList.cpp#642

When we're tracking drag movement down in the apz in either DoFling or TrackTouch we calculate a scrollby values which call Axis GetDisplacementForDuration which subtracts GetCompositionEnd from GetPageEnd (mCompositionBounds - mScrollableRect). But these two rects are not equal in height, mScrollableRect is short the padding we add to the browser.

With this difference, scrollby ends up with a wonky y value that flips back and forth resulting in jitter up and down really fast as you scroll.

Comment 2

[Jim Mathies [:jimm]]

Attachment: adjust overscroll for scroll frame bounds v.1

My first foray into apz code, lets see how I do. :) Comment 1 explains the issue.

Comment 3

[Jim Mathies [:jimm]]

ack, that had some front end gunk in it, ignore the apzc.js snippet.

Comment 4

[Jim Mathies [:jimm]]

Attachment: adjust overscroll for scroll frame bounds v.1

breaking that up.

Comment 5

[Jim Mathies [:jimm]]

Attachment: apzc.js debug

Comment 6

[Kartikaya Gupta (email:[e-mail])]

So based on the description you have in comment 1, it sounds like the correct fix is to shorten the mCompositionBounds to exclude the padding. The mCompositionBounds is not very well defined, but it's supposed to be the screen area that the scrollable rect is displayed in, which in your case should exclude the navbar area.

Interestingly, billm was running into a very similar problem a couple of days ago and has a patch to shorten the mCompositionBounds by the size of the scrollbar (see bug 907495). I think we should do something similar. Or even better, instead of getting the mCompositionBounds from the widget bounds, actually get it from the nsIFrame that it corresponds to. Do you (a) think that would work, and (b) want to try implementing that approach instead?

For (a) you could do a quick check by hard-coding the padding amount and subtracting it off the mCompositionBounds height in nsDisplayList where it's populated.

Comment 7

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 794181 [details] [diff] [review]
adjust overscroll for scroll frame bounds v.1

Review of attachment 794181 [details] [diff] [review]:
-----------------------------------------------------------------

Minusing this for now.

For the record, there are some IRC logs at http://logbot.glob.com.au/?c=mozilla%23gfx&s=20%20Aug%202013&e=20%20Aug%202013#c82185 where we discussed the problem with billm. In general we assume that mCompositionBounds is smaller than or equal to mScrollableRect so if that's not the case then we should make it so rather than fixing Axis to deal with it.

Comment 8

[Jim Mathies [:jimm]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #7)
> Comment on attachment 794181 [details] [diff] [review]
> adjust overscroll for scroll frame bounds v.1
> 
> Review of attachment 794181 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Minusing this for now.
> 
> For the record, there are some IRC logs at
> http://logbot.glob.com.au/
> ?c=mozilla%23gfx&s=20%20Aug%202013&e=20%20Aug%202013#c82185 where we
> discussed the problem with billm. In general we assume that
> mCompositionBounds is smaller than or equal to mScrollableRect so if that's
> not the case then we should make it so rather than fixing Axis to deal with
> it.

Ok sounds good. I wasn't 100% familiar with how mCompositionBounds plays into all this so assumed it needed to be the size of the surface we render too (layer?) which would include the padding at the browser level in layout.

The bug billm is seeing is similar, although we use overlay scrollbars so that wouldn't effect us. In his patch all that is included is the scrollbar size, so it looks like we'll need to do some additional adjusting on top of his work.

Comment 9

[Brian R. Bondy [:bbondy]]

Comment on attachment 794184 [details] [diff] [review]
apzc.js debug

Review of attachment 794184 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/metro/base/content/apzc.js
@@ +109,5 @@
>        // When we're panning, hide the main scrollbars by setting imprecise
>        // input (which sets a property on the browser which hides the scrollbar
>        // via CSS).  This reduces jittering from left to right. We may be able
>        // to get rid of this once we implement axis locking in /gfx APZC.
> +      if (this._debugEvents) Util.dumpLn("APZC pan-begin");

nit: 2 lines.

@@ +115,5 @@
>          InputSourceHelper._imprecise();
>        }
>  
>      } else if (aTopic == "apzc-handle-pan-end") {
> +      if (this._debugEvents) Util.dumpLn("APZC pan-end");

nit: 2 lines.

Comment 10

[Jim Mathies [:jimm]]

Attachment: overlay scrollbar fix

Comment 11

[Jim Mathies [:jimm]]

Attachment: scrollable composition rect v.1

A couple notes - 

- I moved the scrollbar adjustment up into the widget block since we only set mCompositionBounds when we have a valid widget.
- aForFrame->GetScreenRectInAppUnits() can return a rect that's offset from the origin and offscreen. It didn't seem to break when mCompositionBounds had a negative origin, but I still clamped this to the widget bounds to be safe.

Tested with various padding settings on a start tab browser [1], everything looked good.

Something like this probably deserves a test. Not sure how we are handling testing for apz code though. I could probably put something together up in browser chrome if need be, although that's not the best place for it obviously.

[1] http://mxr.mozilla.org/mozilla-central/source/browser/metro/theme/browser.css#213

Comment 12

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 796009 [details] [diff] [review]
scrollable composition rect v.1

Review of attachment 796009 [details] [diff] [review]:
-----------------------------------------------------------------

This is.. interesting. It looks like it could be right but honestly I don't know. I will need to test this patch on B2G to make sure it doesn't break anything. If it doesn't I don't have a problem with it. It will need review by somebody in layout though. I'll r+ once I do some testing with it.

Comment 13

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 796009 [details] [diff] [review]
scrollable composition rect v.1

Review of attachment 796009 [details] [diff] [review]:
-----------------------------------------------------------------

Ok, this doesn't seem to break anything on B2G from my (somewhat limited) testing. I'm ok with it landing. The code still has an unexplained conversion from LayoutDevice to Screen pixels, but I believe it is correct and will clean that up and comment it properly over in bug 909281.

Tagging :tn for review since somebody in layout should also look at this.

Comment 14

[Timothy Nikkel (:tnikkel)]

Comment on attachment 796009 [details] [diff] [review]
scrollable composition rect v.1

I think we should be deflating by the actual scrollbar sizes first and then clamping to the widget rect after.

Comment 15

[Jim Mathies [:jimm]]

Attachment: scrollable composition rect v.2

updated per comments, and cleaned up the units.

Comment 16

[Kartikaya Gupta (email:[e-mail])]

FWIW I was discussing this with botond today and we might have a follow-up patch for this that removes the clamping and uses a different frame object's screen rect, so that the composition bounds are well-defined and consistent. I don't know if you want to land this right away or wait until we figure out what else needs to be done here.

Comment 17

[Timothy Nikkel (:tnikkel)]

Comment on attachment 797882 [details] [diff] [review]
scrollable composition rect v.2

I meant deflate the size of the scrollable frame by the scroll bar sizes, not the widget. Because the scrollbars are applied to the frame, not the widget. Sorry for not being clearer.

Comment 18

[Botond Ballo [:botond]]

Attachment: Compute mCompositionBounds correctly for subframes

This patch combines jimm's patch, kats' suggestions to compute mCompositionBounds correctly for subframes, and tn's review comment.

Summary of changes:
  - Base the composition bounds on the outer frame's size. The inner frame represents
    the content being scrolled, so its size isn't what we wanted.
  - Don't clamp to the widget bounds. This is undesirable for non-toplevel frames,
    and it's unnecessary for the toplevel frame now that we are using the outer
    frame's size because the outer frame's size should be equal to the widget bounds.
  - Only deflate the composition bounds by the scroll bar sizes if the scroll bars
    are not overlay.

jimm, could you check to see that this works ok on metro? Thanks!

Comment 20

[Timothy Nikkel (:tnikkel)]

Comment on attachment 798020 [details] [diff] [review]
Compute mCompositionBounds correctly for subframes

>   // Adjust for the size of scroll bars.
>-  if (scrollableFrame) {
>+  if (scrollableFrame && !LookAndFeel::GetInt(LookAndFeel::eIntID_UseOverlayScrollbars)) {
>     nsMargin sizes = scrollableFrame->GetActualScrollbarSizes();
>     ScreenIntMargin boundMargins(nsPresContext::AppUnitsToIntCSSPixels(sizes.top),
>                                  nsPresContext::AppUnitsToIntCSSPixels(sizes.right),
>                                  nsPresContext::AppUnitsToIntCSSPixels(sizes.bottom),
>                                  nsPresContext::AppUnitsToIntCSSPixels(sizes.left));
>     metrics.mCompositionBounds.Deflate(boundMargins);
>   }

AppUnitsToIntCSSPixels for converting to a screen pixel? Shouldn't this be AppUnitsToDevPixels?

Comment 21

[Botond Ballo [:botond]]

(In reply to Timothy Nikkel (:tn) from comment #20)
> Comment on attachment 798020 [details] [diff] [review]
> Compute mCompositionBounds correctly for subframes
> 
> >   // Adjust for the size of scroll bars.
> >-  if (scrollableFrame) {
> >+  if (scrollableFrame && !LookAndFeel::GetInt(LookAndFeel::eIntID_UseOverlayScrollbars)) {
> >     nsMargin sizes = scrollableFrame->GetActualScrollbarSizes();
> >     ScreenIntMargin boundMargins(nsPresContext::AppUnitsToIntCSSPixels(sizes.top),
> >                                  nsPresContext::AppUnitsToIntCSSPixels(sizes.right),
> >                                  nsPresContext::AppUnitsToIntCSSPixels(sizes.bottom),
> >                                  nsPresContext::AppUnitsToIntCSSPixels(sizes.left));
> >     metrics.mCompositionBounds.Deflate(boundMargins);
> >   }
> 
> AppUnitsToIntCSSPixels for converting to a screen pixel? Shouldn't this be
> AppUnitsToDevPixels?

According to kats in https://bugzilla.mozilla.org/show_bug.cgi?id=907495#c4, the scrollbars are not subject to scaling.

Comment 22

[Timothy Nikkel (:tnikkel)]

(In reply to Botond Ballo [:botond] from comment #21)
> According to kats in https://bugzilla.mozilla.org/show_bug.cgi?id=907495#c4,
> the scrollbars are not subject to scaling.

Ah, good point. Please add a comment so that it's not confusing.

Also, I checked and on mac the overlay scrollbars (so not applicable here) do change size when you change the pref layout.css.devPixelsPerPx. But that might be something specific for retina macs and not a general thing (windows and linux don't change size).

Comment 23

[Botond Ballo [:botond]]

Attachment: Compute mCompositionBounds correctly for subframes

Updated patch to add a comment as per comment #22. Carrying r+.

Comment 24

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #23)
> Created attachment 798047 [details] [diff] [review]
> Compute mCompositionBounds correctly for subframes
> 
> Updated patch to add a comment as per comment #22. Carrying r+.

Hmm, I'm getting a crash on metro after pulling and applying this patch. Not sure if it's caused by this patch or something I pulled. Will investigate.

Comment 25

[Jim Mathies [:jimm]]

Comment on attachment 798047 [details] [diff] [review]
Compute mCompositionBounds correctly for subframes

Looks like you need to wrap this in a if (aScrollFrame) {} since aScrollFrame can be null. With that change in place it appears this is working.

Comment 26

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 798047 [details] [diff] [review]
Compute mCompositionBounds correctly for subframes

Review of attachment 798047 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Jim Mathies [:jimm] from comment #25)
> Looks like you need to wrap this in a if (aScrollFrame) {} since
> aScrollFrame can be null. With that change in place it appears this is
> working.

If this is the case then we should figure out under what circumstances aScrollFrame can be null, and if it is, what the mCompositionBounds should be. I suspect it will be null only in cases where we have a non-scrollable container layer, in which case it doesn't really matter what we put in mCompositionBounds. In this case I think we might be able to just set it 0, but it might be more backwards-compatible to walk up the frame tree until we find a scroll frame and use that instead. I would go with just setting it to 0 first and see if that breaks anything.

::: layout/base/nsDisplayList.cpp
@@ +671,5 @@
>      (float)nsPresContext::AppUnitsPerCSSPixel() / auPerDevPixel);
>  
>    metrics.mMayHaveTouchListeners = aMayHaveTouchListeners;
>  
> +  metrics.mCompositionBounds = ScreenPixel::FromAppUnitsToNearest(

nit: use ScreenIntRect:: here rather than ScreenPixel:: since that's what we're getting out of it.

Comment 27

[Timothy Nikkel (:tnikkel)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #26)
> (In reply to Jim Mathies [:jimm] from comment #25)
> > Looks like you need to wrap this in a if (aScrollFrame) {} since
> > aScrollFrame can be null. With that change in place it appears this is
> > working.
> 
> If this is the case then we should figure out under what circumstances
> aScrollFrame can be null, and if it is, what the mCompositionBounds should
> be. I suspect it will be null only in cases where we have a non-scrollable
> container layer, in which case it doesn't really matter what we put in
> mCompositionBounds. In this case I think we might be able to just set it 0,
> but it might be more backwards-compatible to walk up the frame tree until we
> find a scroll frame and use that instead. I would go with just setting it to
> 0 first and see if that breaks anything.

Looking at the two call sites for RecordFrameMetrics I think the only case it can be null is when the root scroll frame of the document is null, which means it is a xul document.

Comment 28

[Botond Ballo [:botond]]

This patch messes up the scrolling of some websites that don't involve subframes, such as http://people.mozilla.org/~kgupta/tmp/long.html: such pages can now be overscrolled to the left, and scrolling them to the right causes the screen to go blank.

Comment 29

[Botond Ballo [:botond]]

Attachment: Compute mCompositionBounds correctly for subframes

Comment 30

[Botond Ballo [:botond]]

Comment on attachment 799115 [details] [diff] [review]
Compute mCompositionBounds correctly for subframes

The problem was that we were forcing a conversion from layout device coordinates into screen coordinates without multiplying by the appropriate scale factor (in this case, mResolution * LayerToScreenScale(1)). The updated patch fixes that.

Comment 31

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 799115 [details] [diff] [review]
Compute mCompositionBounds correctly for subframes

Review of attachment 799115 [details] [diff] [review]:
-----------------------------------------------------------------

Would have been better to split the refactoring into a separate patch from the actual change, but I don't know if it's worth the additional work at this point.

::: layout/base/nsDisplayList.cpp
@@ +682,5 @@
> +  if (aScrollFrame) {
> +    metrics.mCompositionBounds = RoundedToInt(LayoutDeviceRect::FromAppUnits(
> +        aScrollFrame->GetScreenRectInAppUnits(), auPerDevPixel)
> +                               * metrics.mResolution
> +                               * LayerToScreenScale(1.0f));

the 1.0 should get a comment.

Comment 32

[Botond Ballo [:botond]]

Attachment: Compute mCompositionBounds correctly for subframes

Added a comment as per Kats' feedback.

Comment 33

[Timothy Nikkel (:tnikkel)]

Comment on attachment 799130 [details] [diff] [review]
Compute mCompositionBounds correctly for subframes

We should probably try to set the composition bounds to something reasonable if aScrollFrame is null. That can be a follow up though.

Comment 34

[Botond Ballo [:botond]]

(In reply to Timothy Nikkel (:tn) from comment #33)
> Comment on attachment 799130 [details] [diff] [review]
> Compute mCompositionBounds correctly for subframes
> 
> We should probably try to set the composition bounds to something reasonable
> if aScrollFrame is null. That can be a follow up though.

Kats suggested in comment #26 that 0 (which is what mCompositionBounds is initialized to when the FrameMetrics is constructed) might be a reasonable value if the layer is not scrollable. Will that be the case when aScrollFrame is null?

Comment 35

[Botond Ballo [:botond]]

Try server run: https://tbpl.mozilla.org/?tree=Try&rev=cbe36811afd8

Comment 36

[Timothy Nikkel (:tnikkel)]

(In reply to Botond Ballo [:botond] from comment #34)
> (In reply to Timothy Nikkel (:tn) from comment #33)
> > Comment on attachment 799130 [details] [diff] [review]
> > Compute mCompositionBounds correctly for subframes
> > 
> > We should probably try to set the composition bounds to something reasonable
> > if aScrollFrame is null. That can be a follow up though.
> 
> Kats suggested in comment #26 that 0 (which is what mCompositionBounds is
> initialized to when the FrameMetrics is constructed) might be a reasonable
> value if the layer is not scrollable. Will that be the case when
> aScrollFrame is null?

I think the case when aScrollFrame is null is when we are in a document without a root scroll frame. These are xul documents. In that case I think it would make more sense to use the size of the viewport frame, which I think should be aForFrame.

Comment 37

[Kartikaya Gupta (email:[e-mail])]

That sounds reasonable to me.

Comment 38

[Botond Ballo [:botond]]

Attachment: Compute mCompositionBounds correctly for subframes

Updated patch to set mCompositionBounds to aForFrame's size if aScrollFrame is null.

Comment 39

[Botond Ballo [:botond]]

(And carried r+.)

Comment 40

[Botond Ballo [:botond]]

Attachment: Compute mCompositionBounds correctly for subframes

Updated patch to add a comment about the case where aScrollFrame is null. Carrying r+.

Comment 41

[Botond Ballo [:botond]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4db34d255ed0

(My first commit to inbound!)

Comment 42

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/4db34d255ed0

Comment 43

[Matt Brubeck (:mbrubeck)]

It looks like this regressed several performance benchmarks on Linux (tpaint, tsvg, tart, tresize):
https://groups.google.com/d/topic/mozilla.dev.tree-management/MUbnYXShSP4/discussion

For example:

Regression: Mozilla-Inbound-Non-PGO - SVG-ASAP - Ubuntu HW 12.04 x64 - 10.7% increase
-------------------------------------------------------------------------------------
    Previous: avg 488.291 stddev 6.621 of 12 runs up to revision 7ccebf05488c
    New     : avg 540.636 stddev 11.483 of 12 runs since revision 5a86fde943e6
    Change  : +52.345 (10.7% / z=7.905)
    Graph   : http://mzl.la/14lyAtB

Changeset range: http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=7ccebf05488c&tochange=5a86fde943e6

Try push that does not include the other changes in this range:
https://tbpl.mozilla.org/?tree=Try&rev=d1bad1790560

Compare-Talos of that Try push versus the pre-regression changeset:
http://perf.snarkfest.net/compare-talos/index.html?oldRevs=7ccebf05488,b5d62f5c733c,365e150efda0&newRev=d1bad1790560&submit=true

Comment 44

[Timothy Nikkel (:tnikkel)]

My guess is because GetScreenRectInAppUnits calls WidgetToScreen offset on the widget, which is usually a system call, whereas GetBounds (before this patch) is usually just a getter of an already stored rect.

Is it really important to get the x/y offset of the composition area relative to the screen? If the x/y offset is important at all I would think the x/y offset relative to the widget would be what we want.

On plain old linux we probably don't need to be calling this code at all, can we not run this code for not reason maybe?

Comment 45

[Avi Halachmi (:avih)]

It should be noted that TART enforces OMTC off (layers.offmainthreadcomposition.enabled=false) due to currently broken frame intervals recording with OMTC. I don't know if this fact helps in analysis.

tsvgx (ASAP) and other talos tests neither require nor enforce OMTC off (they run with whatever the default is).

Comment 46

[Botond Ballo [:botond]]

(In reply to Timothy Nikkel (:tn) from comment #44)
> My guess is because GetScreenRectInAppUnits calls WidgetToScreen offset on
> the widget, which is usually a system call, whereas GetBounds (before this
> patch) is usually just a getter of an already stored rect.
> 
> Is it really important to get the x/y offset of the composition area
> relative to the screen? If the x/y offset is important at all I would think
> the x/y offset relative to the widget would be what we want.

You're right. I filed bug 913205 as a follow-up.

Comment 47

[Brad Lassey [:blassey] (use needinfo?)]

this fixed the regression in bug 913013, what do we think of uplifting to 25?

Comment 48

[Kartikaya Gupta (email:[e-mail])]

For the record this also fixed the regression in bug 923969 and the two dupes. FWIW I think the risk to uplift is significant but it might be worth it.

Comment 49

[Botond Ballo [:botond]]

If we want to uplift this to 25, we'll need to also uplift the dependency bug 907945, and the fixes for the regressions it caused (bug 913205, bug 914825, bug 918682). We are currently testing whether this series of 5 patches applies cleanly to mozilla-beta, and then we'll do a try run for them.

Comment 50

[Botond Ballo [:botond]]

Note that all these patches are just a few lines of change to RecordFrameMetrics, so in spite of there being 5 patches, it's very little code that would be uplifted.

Comment 51

[Kartikaya Gupta (email:[e-mail])]

I pushed a try run to beta at https://tbpl.mozilla.org/?tree=Try&rev=9299a59f7836 but I don't know if there need to be branding changes for non-android platforms too. I looked at the mozconfigs and didn't see anything beta-specific like there in android but the try run might blow up.

I also had to pull in the changes from bug Bug 907754 for the patches listed in comment 49 to apply "cleanly" (i.e. without major rebasing pain - there was still some minor rebasing pain).

On the one hand I do feel that these regressions are pretty important and should be fixed, but on the other I feel like it's easy to miss something and end up in a situation worse than we are now.

Comment 52

[Kartikaya Gupta (email:[e-mail])]

A more useful try run, since I screwed up the try syntax before:

https://tbpl.mozilla.org/?tree=Try&rev=4a24f1d6e877

Comment 53

[Kartikaya Gupta (email:[e-mail])]

That try build failed because of missing some other patches; I did a couple more pushes to try with some fixes but it still fails to build. I'm going to try building it locally, but it's looking increasingly like there are a bunch of other changes (e.g. changing the type of FrameMetrics.mZoom) that landed in 26 interleaved with these bugs that makes it much trickier to rebase.

Comment 54

[Botond Ballo [:botond]]

Attachment: Backport corrections to composition bounds calculations to beta

The attached patch applies cleanly to beta and contains approximately the content of the patches for the following bugs:

Corrections to composition bounds calculation:
  bug 904533
  bug 913205
  bug 914825
  bug 918682

Changes to Rect.h, BaseRect.h, and Units.h that the above patches depend on:
  bug 907901
  bug 907906  
  bug 909281

I did my best to include as little as possible. The combined patch is fairly small.

Comment 55

[Botond Ballo [:botond]]

Kats suggested that I do try pushes, one with just my patch and one that also changes the branding from 'beta' to 'nightly':

https://tbpl.mozilla.org/?tree=Try&rev=4818e394636f  (no branding changes)
https://tbpl.mozilla.org/?tree=Try&rev=b89fbc9a2f82  (with branding changes)

Comment 56

[Alex Keybl [:akeybl]]

(In reply to Brad Lassey [:blassey] (use needinfo?) from comment #47)
> this fixed the regression in bug 913013, what do we think of uplifting to 25?

Deeming this issue critical enough to land in the last two weeks of the beta seems unlikely, since we haven't been tracking a related bug since 25 has been with prerelease populations since 8/9.

What's the risk involved here?

Comment 57

[Matt Brubeck (:mbrubeck)]

(In reply to Alex Keybl [:akeybl] from comment #56)
> Deeming this issue critical enough to land in the last two weeks of the beta
> seems unlikely, since we haven't been tracking a related bug since 25 has
> been with prerelease populations since 8/9.

This fixes bug 913013, bug 890910, and bug 890910 (the latter two were duped to bug 923969) which are all tracking-25+.

Comment 58

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 816033 [details] [diff] [review]
Backport corrections to composition bounds calculations to beta

Review of attachment 816033 [details] [diff] [review]:
-----------------------------------------------------------------

The code changes look sane to me, but when I tested with the try build (from the branding-changed version) it didn't fix bug 926124 or bug 923969. It did fix bug 890910, bug 913013, and bug 903473. I'm not sure if this is safe to uplift unless we understand why it fixed some but not others. (All the ones it fixed are tracking 25+ whereas the ones it didn't fix are not).

Comment 59

[Kartikaya Gupta (email:[e-mail])]

(In reply to Alex Keybl [:akeybl] from comment #56)
> (In reply to Brad Lassey [:blassey] (use needinfo?) from comment #47)
> What's the risk involved here?

The risk is that (1) there have been many changes to the code in 26 so we have to manually pick the right bits to uplift to 25 which is easy to do incorrectly and (2) the exact nature of the problem is not well-understood - it's some sort of interaction between the fixed-position code and the RecordFrameMetrics code but we're not sure exactly what. Therefore we're doing a bit of guesswork to try and come up with the right pieces to uplift (and based on comment 58 we did a not-perfect job) and there's a significant risk of it breaking other fixed-position content. Any regressions should be limited to fixed-position content at least; the only other code that would be affected by this is B2G-only and 25 is irrelevant for them.

Comment 60

[Alex Keybl [:akeybl]]

(In reply to Matt Brubeck (:mbrubeck) from comment #57)
> (In reply to Alex Keybl [:akeybl] from comment #56)
> > Deeming this issue critical enough to land in the last two weeks of the beta
> > seems unlikely, since we haven't been tracking a related bug since 25 has
> > been with prerelease populations since 8/9.
> 
> This fixes bug 913013, bug 890910, and bug 890910 (the latter two were duped
> to bug 923969) which are all tracking-25+.

None of those are tracking Firefox 25 from the Release perspective. That's some engineering flag I ignore.

Too late for Firefox 25 based on comment 59.