Closed Bug 904533 Opened 6 years ago Closed 6 years ago

APZ panning on about:start tab is wonky

Categories

(Core :: Layout, defect)

x86_64
Windows 8.1
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla26
Tracking Status
firefox25 - wontfix
fennec 25+ ---

People

(Reporter: jimm, Assigned: botond)

References

Details

(Whiteboard: [preview])

Attachments

(2 files, 11 obsolete files)

7.06 KB, patch
botond
: review+
Details | Diff | Splinter Review
22.55 KB, patch
kats
: review+
Details | Diff | Splinter Review
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.
Blocks: metro-apzc
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.
My first foray into apz code, lets see how I do. :) Comment 1 explains the issue.
Assignee: nobody → jmathies
Attachment #794180 - Flags: review?(bugmail.mozilla)
ack, that had some front end gunk in it, ignore the apzc.js snippet.
breaking that up.
Attachment #794180 - Attachment is obsolete: true
Attachment #794180 - Flags: review?(bugmail.mozilla)
Attachment #794181 - Flags: review?(bugmail.mozilla)
Attached patch apzc.js debug (obsolete) — Splinter Review
Attachment #794184 - Flags: review?(netzen)
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 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.
Attachment #794181 - Flags: review?(bugmail.mozilla) → review-
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.
Depends on: 907495
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.
Attachment #794184 - Flags: review?(netzen) → review+
Depends on: 908748
Attached patch overlay scrollbar fix (obsolete) — Splinter Review
Attachment #794181 - Attachment is obsolete: true
Attached patch scrollable composition rect v.1 (obsolete) — Splinter Review
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
Attachment #795644 - Attachment is obsolete: true
Attachment #796009 - Flags: review?(bugmail.mozilla)
Component: Pan and Zoom → Layout
Product: Firefox for Metro → Core
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.
Attachment #796009 - Flags: feedback+
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.
Attachment #796009 - Flags: review?(tnikkel)
Attachment #796009 - Flags: review?(bugmail.mozilla)
Attachment #796009 - Flags: review+
Attachment #796009 - Flags: feedback+
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.
Attached patch scrollable composition rect v.2 (obsolete) — Splinter Review
updated per comments, and cleaned up the units.
Attachment #797882 - Flags: review?(tnikkel)
Attachment #796009 - Attachment is obsolete: true
Attachment #796009 - Flags: review?(tnikkel)
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 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.
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!
Assignee: jmathies → botond
Attachment #797882 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #797882 - Flags: review?(tnikkel)
Attachment #798020 - Flags: review?(tnikkel)
Attachment #798020 - Flags: feedback?(jmathies)
Attachment #798020 - Flags: feedback?(bugmail.mozilla)
Duplicate of this bug: 908958
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?
(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.
(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).
Attachment #798020 - Flags: review?(tnikkel) → review+
Updated patch to add a comment as per comment #22. Carrying r+.
Attachment #798020 - Attachment is obsolete: true
Attachment #798020 - Flags: feedback?(jmathies)
Attachment #798020 - Flags: feedback?(bugmail.mozilla)
Attachment #798047 - Flags: review+
Attachment #798047 - Flags: feedback?(jmathies)
Attachment #798047 - Flags: feedback?(bugmail.mozilla)
(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 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.
Attachment #798047 - Flags: feedback?(jmathies) → feedback+
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.
Attachment #798047 - Flags: feedback?(bugmail.mozilla) → feedback+
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.
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.
Attachment #799115 - Flags: review?(tnikkel)
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.
Attachment #799115 - Attachment description: bug904533.patch → Compute mCompositionBounds correctly for subframes
Attachment #799115 - Flags: feedback?(bugmail.mozilla)
Attachment #798047 - Attachment is obsolete: true
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.
Attachment #799115 - Flags: feedback?(bugmail.mozilla) → feedback+
Added a comment as per Kats' feedback.
Attachment #799115 - Attachment is obsolete: true
Attachment #799115 - Flags: review?(tnikkel)
Attachment #799130 - Flags: review?(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.
Attachment #799130 - Flags: review?(tnikkel) → review+
(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?
Blocks: 898055
(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.
Updated patch to set mCompositionBounds to aForFrame's size if aScrollFrame is null.
Attachment #799130 - Attachment is obsolete: true
Attachment #799735 - Flags: review+
Attachment #799735 - Flags: feedback?(bugmail.mozilla)
(And carried r+.)
Attachment #799735 - Flags: feedback?(bugmail.mozilla) → feedback+
Updated patch to add a comment about the case where aScrollFrame is null. Carrying r+.
Attachment #799735 - Attachment is obsolete: true
Attachment #799742 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/4db34d255ed0
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
Attachment #794184 - Attachment is obsolete: true
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
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?
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).
Depends on: 913205
(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.
Depends on: 914825
Depends on: 918682
this fixed the regression in bug 913013, what do we think of uplifting to 25?
tracking-fennec: --- → 25+
Flags: needinfo?(botond)
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.
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.
Flags: needinfo?(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.
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.
A more useful try run, since I screwed up the try syntax before:

https://tbpl.mozilla.org/?tree=Try&rev=4a24f1d6e877
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.
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.
Attachment #816033 - Flags: review?(bugmail.mozilla)
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)
(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?
(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 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).
Attachment #816033 - Flags: review?(bugmail.mozilla) → review+
(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.
(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.
OS: Windows 8 Metro → Windows 8.1
You need to log in before you can comment on or make changes to this bug.