We make every effort that before the first paint, a displayport will be set on the root scrollable document[0][1], but unfortunately, the root scrollable document isn't always the main scrollable area. Most unfortunately, this is the case for all Gaia apps, which use a subdocument for their scrolling (this was cited to be a UX issue, as using the root scrollable document will mean the scrollbar will be overlapped by the application header). We should figure out some way of making sure these elements also get a displayport set. Suggestions: - Find the element in the centre of the screen, and if it's scrollable, set a displayport on that - Recurse through the DOM and set a displayport on the first scrollable element - Recurse through the DOM and set a displayport on the element with the largest scrollable rect (that's scrollable) [0] http://hg.mozilla.org/mozilla-central/file/67485526e241/dom/ipc/TabChild.cpp#l359 [1] http://hg.mozilla.org/mozilla-central/file/67485526e241/dom/ipc/TabChild.cpp#l525

I'll have a look at this.

Nominating for 1.4. Based on Chris' description of this being "our worst and most obvious performance bug at the moment from a user perspective", I'd say we want this to meet our APZ perf target.

I discussed this with Timothy and came up with the following plan: In TabChild's before-first-paint handler, - Perform a depth-first search over the frame tree for nsGfxScrollFrames. - Find the first one that has a nonempty scroll range and isn't overflow:hidden. - If it's not the root scroll frame, give it a display port. (We give the root scroll frame a display port elsewhere). I'll give this a try.

Note that the depth-first search implements Chris' second suggestion ("first scrollable element", for a particular definition of "first"), but we can vary the heuristic easily by changing how we search.

Comment on attachment 8390047 [details] [diff] [review] Part 1 - Expose the logic used to decide whether a scrollable frame should be async scrollable Doing this seems fine.

(In reply to Botond Ballo [:botond] from comment #2) > Nominating for 1.4. Based on Chris' description of this being "our worst and > most obvious performance bug at the moment from a user perspective", I'd say > we want this to meet our APZ perf target. Considering this we are going to block on this issue.

Note: I changed my mind about taking individual fields of FrameMetrics here.

Whoops, just realized we already have a way of telling if this is the first paint in TabChild::HandlePossibleViewportChange(). Updated Part 6 patch to just use the existing way.

I have tested this in the Settings app, and I indeed see a scrollable layer being created right away for the <div> which is the primary scrollable element on the page.

In the Contacts app this still doesn't give the primary scrollable <div> an initial displayport because on first paint, the contacts list isn't populated yet, and so the <div> doesn't have a scroll range yet. I discussed this with Timothy, and decided to try an alternate approach where the setting of the displayport is done in ScrollFrameHelper::BuildDisplayList instead (which is called for every scrollable frame on every paint), but only if there isn't one set already. This change in approach invalidates the Part 3 and Part 6 patches, and I will clear their review flags accordingly. The other patches will be used as-is in the new approach and can proceed to be reviewed.

After some more discussion with Timothy, we realized that setting a displayport mid-paint (which is when ScrollFrameHelper::BuildDisplayList is called) is not a great idea. Instead, we do the same traversal that we would have done in TabChild's before-first-paint handler, on every paint (specifically, in nsLayoutUtils::PaintFrame(), before display lists are build), but we only set a displayport if one hasn't already been set. The new Part 3 and Part 6 patches implement this. I have tested this and now the contacts list in the Contacts app gets a displayport as soon as enough contacts are added. Try push: https://tbpl.mozilla.org/?tree=Try&rev=ca8586af0f45

Comment on attachment 8390948 [details] [diff] [review] Part 6 - Make sure the primary async-scrollable frame has a displayport It would probably be better to factor this out into a little static helper function that we call.

Comment on attachment 8390678 [details] [diff] [review] Part 4 - Refactor MaybeAlignAndClampDisplayport to separate scroll offset adjustment from aligning/clamping Review of attachment 8390678 [details] [diff] [review]: ----------------------------------------------------------------- ::: widget/xpwidgets/APZCCallbackHelper.cpp @@ +79,5 @@ > > // Finally, clamp the display port to the expanded scrollable rect. > + CSSRect scrollableRect = aMetrics.GetExpandedScrollableRect(); > + displayPort = scrollableRect.Intersect(aMetrics.mDisplayPort + aMetrics.GetScrollOffset()) > + - aMetrics.GetScrollOffset(); Might be worth pulling out GetScrollOffset to a local var since it's used a bunch of times in this function. Not a big deal though, it's only for readability.

Comment on attachment 8390680 [details] [diff] [review] Part 5 - Introduce APZCCallbackHelper::AlignAndSetDisplayPort Review of attachment 8390680 [details] [diff] [review]: ----------------------------------------------------------------- These changes make the code much cleaner, thanks! ::: widget/xpwidgets/APZCCallbackHelper.cpp @@ +222,5 @@ > > + aMetrics.mDisplayPort = AlignAndSetDisplayPort(aContent, aMetrics); > +} > + > +/* static */ CSSRect For consistency, remove the /*static*/ here. All the functions in this class are static.

Comment on attachment 8390948 [details] [diff] [review] Part 6 - Make sure the primary async-scrollable frame has a displayport Review of attachment 8390948 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/base/nsLayoutUtils.cpp @@ +2284,5 @@ > } else { > visibleRegion = aDirtyRegion; > } > > +#ifdef MOZ_WIDGET_GONK It might be better to make this a pref so that we can more easily disable it to isolate problems. Also so that we can more easily enable it on other platforms, and so that it gets compiled the same everywhere. However if you choose to keep it an ifdef, then it would be better to also wrap in the new #include statements in the same #ifdef. I also agree with tn that this should be refactored to a helper function, so that we can use early-exit conditions instead of nested if statements. @@ +2295,5 @@ > + // before-first-paint handler, so if that's primary async scrollable frame > + // then we don't need to do anything here. > + if (primaryAsyncScrollableFrame != presShell->GetRootScrollFrameAsScrollable()) { > + nsIFrame* frame = do_QueryFrame(primaryAsyncScrollableFrame); > + nsIContent* content = frame->GetContent(); Multiple frames can be associated with the same piece of content. Timothy, do you know if it's worth checking here that the frame is the content's primary frame? @@ +2304,5 @@ > + FrameMetrics subframeMetrics; > + > + // Subframes cannot be zoomed, but they are affected by the zoom on > + // the root frame. > + subframeMetrics.SetZoom(CSSToScreenScale(presShellResolution)); Should this be using the cumulative resolution? And multiplying by the CSS->LD pixel ratio?

Comment on attachment 8390948 [details] [diff] [review] Part 6 - Make sure the primary async-scrollable frame has a displayport Review of attachment 8390948 [details] [diff] [review]: ----------------------------------------------------------------- Minusing until comments are addressed.

(In reply to (PTO|Mar15-Mar19) Kartikaya Gupta (email:kats@mozilla.com) from comment #22) > @@ +2304,5 @@ > > + FrameMetrics subframeMetrics; > > + > > + // Subframes cannot be zoomed, but they are affected by the zoom on > > + // the root frame. > > + subframeMetrics.SetZoom(CSSToScreenScale(presShellResolution)); > > Should this be using the cumulative resolution? And multiplying by the > CSS->LD pixel ratio? It should definitely be multiplying by the CSS->LD pixel ration. I'll fix that. Regarding the cumulative resolution, technically yes, but on B2G I believe the only pres shell that should have a resolution is the one belonging to the display root document, i.e. this one. One of the reasons I'd like to keep this functionality B2G-only for now is so that I can assume this.

(In reply to Botond Ballo [:botond] from comment #24) > One of the reasons I'd like to > keep this functionality B2G-only for now is so that I can assume this. What advantage do you get from assuming this? There should already be a GetCumulativeResolution function on the presshell, so using that would be just as easy.

Updated Part 6 patch to address review comments. After discussing with Kats I decided to keep the #ifdef for B2G for now. (In reply to (PTO|Mar15-Mar19) Kartikaya Gupta (email:kats@mozilla.com) from comment #25) > (In reply to Botond Ballo [:botond] from comment #24) > > One of the reasons I'd like to > > keep this functionality B2G-only for now is so that I can assume this. > > What advantage do you get from assuming this? There should already be a > GetCumulativeResolution function on the presshell, so using that would be > just as easy. Good point; the cost to making this more general was very low so I did so.

Comment on attachment 8391279 [details] [diff] [review] Part 6 - Make sure the primary async-scrollable frame has a displayport Review of attachment 8391279 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/base/nsLayoutUtils.cpp @@ +2250,5 @@ > + // the root scroll frame's display port having been set already (on B2G > + // this happens in TabChild's before-first-paint handler). > +#ifdef MOZ_WIDGET_GONK > + if (nsIScrollableFrame* primaryAsyncScrollableFrame = > + FindPrimaryAsyncScrollableFrame(aPresShell->GetRootFrame())) { I'd prefer to invert some of these checks and return; if they fail, to reduce the nesting level of this function.

(In reply to (PTO|Mar15-Mar19) Kartikaya Gupta (email:kats@mozilla.com) from comment #20) > > // Finally, clamp the display port to the expanded scrollable rect. > > + CSSRect scrollableRect = aMetrics.GetExpandedScrollableRect(); > > + displayPort = scrollableRect.Intersect(aMetrics.mDisplayPort + aMetrics.GetScrollOffset()) > > + - aMetrics.GetScrollOffset(); > > Might be worth pulling out GetScrollOffset to a local var since it's used a > bunch of times in this function. Not a big deal though, it's only for > readability. Done. I also realized there was a bug in this patch - 'aMetrics.mDisplayPort' on that line should have been 'displayPort'. I also took this opportunity to clean this function up a little more. Re-requesting review just in case.

(In reply to (PTO|Mar15-Mar19) Kartikaya Gupta (email:kats@mozilla.com) from comment #21) > ::: widget/xpwidgets/APZCCallbackHelper.cpp > @@ +222,5 @@ > > > > + aMetrics.mDisplayPort = AlignAndSetDisplayPort(aContent, aMetrics); > > +} > > + > > +/* static */ CSSRect > > For consistency, remove the /*static*/ here. All the functions in this class > are static. Done. Carrying r+.

(In reply to (PTO|Mar15-Mar19) Kartikaya Gupta (email:kats@mozilla.com) from comment #27) > ::: layout/base/nsLayoutUtils.cpp > @@ +2250,5 @@ > > + // the root scroll frame's display port having been set already (on B2G > > + // this happens in TabChild's before-first-paint handler). > > +#ifdef MOZ_WIDGET_GONK > > + if (nsIScrollableFrame* primaryAsyncScrollableFrame = > > + FindPrimaryAsyncScrollableFrame(aPresShell->GetRootFrame())) { > > I'd prefer to invert some of these checks and return; if they fail, to > reduce the nesting level of this function. I like the brevity of 'if (T* x = func())', which you can't use if you invert. But I can see the argument for avoiding nesting, so done.

Comment on attachment 8390047 [details] [diff] [review] Part 1 - Expose the logic used to decide whether a scrollable frame should be async scrollable IsAsyncScrollable makes it sound like we are asking if this frame is able to be async scrolled, when really I think the semantics are more like it is desirable to async scroll this frame (nothing prevents these frames from being async scrollable). So something like WantAsyncScroll or ShouldAsyncScroll?

Comment on attachment 8390673 [details] [diff] [review] Part 2 - Factor out scrollable rect calculation into nsLayoutUtils The overflow behaviour might seem a little surprising just based on reading the comment in the header.

(In reply to Timothy Nikkel (:tn) from comment #31) > Comment on attachment 8390047 [details] [diff] [review] > Part 1 - Expose the logic used to decide whether a scrollable frame should > be async scrollable > > IsAsyncScrollable makes it sound like we are asking if this frame is able to > be async scrolled, when really I think the semantics are more like it is > desirable to async scroll this frame (nothing prevents these frames from > being async scrollable). So something like WantAsyncScroll or > ShouldAsyncScroll? Renamed to WantAsyncScroll. Carrying r+.

(In reply to Timothy Nikkel (:tn) from comment #32) > Comment on attachment 8390673 [details] [diff] [review] > Part 2 - Factor out scrollable rect calculation into nsLayoutUtils > > The overflow behaviour might seem a little surprising just based on reading > the comment in the header. Adjusted comment to describe what happens if the element is overflow/:hidden. Carrying r+.

Updated Part 3 patch to reflect the change from IsAsyncScrollable() to WantAsyncScroll().

Update on the 1.4+ status of this bug: This bug fixes a very user-noticeable delay in many apps between the user starting to pan, and the content starting to move on the screen. This affects almost all Gaia apps (comment 0) and has been described as one of our most obvious performance bugs (comment 2). I have patches for this bug which have been tested and work. A couple of them are still under review, with Timothy and I actively iterating on them. I believe we should continue blocking on this.

Timothy and I discussed the Part 3 and Part 6 patches some more. We think it would be good to avoid doing a pass over the frame tree every paint, and instead just the pass already being done during display list building (essentially the approach described in comment 15, with the important modification that for subdocuments we'd do checking in nsSubDocumentFrame::BuildDisplayList rather than ScrollFrameHelper::BuildDisplayList).

Dropping review requests for Part 3 and Part 6 patches until I revise them as per comment 37.

This code is common to the various approaches we've tried, so I thought I'd put it into its own patch so it doesn't keep getting reposted for review.

One thing I realized while testing the latest approach is that we don't want this creation of displayports to happen in the B2G parent process, where APZ is not enabled to begin with. To help facilitate reasoning about this, I moved the 'wantSubAPZC' logic from ScrollFrameHelper::BuildDisplayList() into nsLayoutUtils.

This contains the bulk of the code for the new approach.

(In reply to Botond Ballo [:botond] from comment #41) > Created attachment 8393645 [details] [diff] [review] > Part 7 - Make sure the primary async-scrollable frame has a displayport > > This contains the bulk of the code for the new approach. There is one thing in this patch which I'm unsure about, which is the call to SetIgnoreViewportScrolling() I added to nsSubDocumentFrame::BuildDisplayList(). Please let me know if this is OK.

Not blocking, as we don't know this is the only way to get the performance, but please ask for the uplift when fixed, and we should continue working on this as it is a good candidate for giving us the improvements we need.

(In reply to Milan Sreckovic [:milan] from comment #43) > Not blocking, as we don't know this is the only way to get the performance, > but please ask for the uplift when fixed, and we should continue working on > this as it is a good candidate for giving us the improvements we need. Setting a displayport on a scrollable element is the only decent way we can not have janky first scrolls, beyond massively (unrealistically) improving our layout and render performance (at which point, there'd be almost no point in having displayports). The other thing we could do is have Gaia use the root scrollable document for all main scrollable areas, but I assume that's an even bigger and more awkward change at this point (also a bit of an annoying limitation for app developers).

Comment on attachment 8393645 [details] [diff] [review] Part 7 - Make sure the primary async-scrollable frame has a displayport Ah, so I realized that setting a display port in nsDOMWindowUtils will schedule a paint, thus giving us a useless paint that we have carefully tried to avoid. I guess you could make a c++ only set display port function that let's the caller specify no schedule paint call.

> Comment on attachment 8393645 [details] [diff] [review] > Part 7 - Make sure the primary async-scrollable frame has a displayport > > Ah, so I realized that setting a display port in nsDOMWindowUtils will > schedule a paint, thus giving us a useless paint that we have carefully > tried to avoid. I guess you could make a c++ only set display port function > that let's the caller specify no schedule paint call. Good point! Updated patches to address this.

These patches need to be updated now that bug 957668 has landed, as that changed a lot of code related to displayports.

Rebased to apply to latest trunk. Carrying r+ from Timothy.

Note that this applies on top of the patch for bug 988882, which is currently backed out. I expect to be able to reland it by the time this is ready to land, but if not it's straightforward to adjust this patch to apply on top of the previous code.

A previous version of this has been reviewed, but things have changed enough that I'd like a re-review.

Again, requesting a re-review as things changed a bit since last time.

Comment on attachment 8401872 [details] [diff] [review] Part 3 - Calculate frame metrics for a display port calculation Review of attachment 8401872 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/base/nsLayoutUtils.cpp @@ +2445,5 @@ > + metrics.mScrollableRect = CSSRect::FromAppUnits( > + nsLayoutUtils::CalculateScrollableRectForFrame(aScrollFrameAsScrollable, nullptr)); > + > + return metrics; > + nit: remove the blank line at the end of the function

Comment on attachment 8401872 [details] [diff] [review] Part 3 - Calculate frame metrics for a display port calculation >+ CSSToLayoutDeviceScale deviceScale(nsPresContext::AppUnitsPerCSSPixel() >+ / presContext->AppUnitsPerDevPixel()); I think this will be an int / int division, so it will truncate. We want float here. >+ // Only the size of the composition bounds is relevant to the >+ // displayport calculation, not its origin. >+ metrics.mCompositionBounds >+ = RoundedToInt(LayoutDeviceRect::FromAppUnits(nsRect(nsPoint(0, 0), aScrollFrame->GetSize()), >+ presContext->AppUnitsPerDevPixel()) >+ * (cumulativeResolution / resolution)); >+ >+ // This function is used for setting a display port for subframes, so >+ // aScrollFrame will not be the root content document's root scroll frame. >+ metrics.SetRootCompositionSize( >+ nsLayoutUtils::CalculateRootCompositionSize(aScrollFrame, false, metrics)); I looked forward in this patch series and this function is called for all scroll frames include root ones, and root root ones. Which also means that the calculation of composition bounds here is wrong.

Comment on attachment 8401876 [details] [diff] [review] Part 5 - Introduce nsLayoutUtils::SetDisplayPortMargins, with an option to not repaint >+ MOZ_BEGIN_ENUM_CLASS(RepaintMode, uint8_t) >+ Repaint, >+ DoNotRepaint >+ MOZ_END_ENUM_CLASS(PixelCastJustification) Is putting PixelCastJustification not a compile error? > /** > * Set the display port base rect for given element to be used with display > * port margins. > */ >- static void SetDisplayPortBase(nsIContent* aContent, const nsRect& aBase); >+ static void SetDisplayPortBase(nsIContent* aContent, const nsRect& aBase); Fix the comment indenting too while you are here.

Comment on attachment 8401878 [details] [diff] [review] Part 6 - Make sure the primary async-scrollable frame has a displayport set I don't know how I like the name primary async-scrollable. How about we just keep track if we've found a displayport? So call it "mFoundADisplayPort"? I'm not a stickler, but some of your lines go quite far beyond 80 chars. >+bool >+nsLayoutUtils::GetOrMaybeCreateDisplayPort(nsDisplayListBuilder& aBuilder, >+ nsIFrame* aScrollFrame, >+ nsRect aDisplayPortBase, >+ nsRect* aOutDisplayport) { >+ nsIContent* content = aScrollFrame->GetContent(); >+ nsIScrollableFrame* scrollableFrame = do_QueryFrame(aScrollFrame); >+ if (!content || !aScrollFrame) { >+ return false; >+ } We don't need scrollableFrame until further down, so move it there. >+ FrameMetrics metrics = CalculateFrameMetricsForDisplayPort(aScrollFrame, scrollableFrame); >+ LayerMargin displayportMargins = AsyncPanZoomController::CalculatePendingDisplayPort( >+ metrics, ScreenPoint(0.0f, 0.0f), 0.0); >+ nsIPresShell* presShell = aScrollFrame->PresContext()->GetPresShell(); >+ uint32_t alignment = gfxPrefs::LayersTilesEnabled() ? TILEDLAYERBUFFER_TILE_SIZE : 1; >+ nsLayoutUtils::SetDisplayPortMargins(content, presShell, displayportMargins, alignment, 0, >+ nsLayoutUtils::RepaintMode::DoNotRepaint); I think we want to use 0 for alignment if we aren't tiling, because if alignment is non-zero we will inflate the rect by 1 and then align to alignment. >+ /** >+ * Get the display port for |aScrollFrame|'s content. If |aScrollFrame} >+ * WantsAsyncScroll() and we don't have a displayport for the primary >+ * async-scrollable frame yet (as tracked by |aBuilder|), calculate and set >+ * a display port. Returns true if there is (now) a displayport, and if so >+ * the displayport is returned in |aOutDisplayport|. >+ * >+ * Note that a displayport can either be stored as a rect, or as a base >+ * rect + margins. If it is stored as a base rect + margins, the base rect >+ * is updated to |aDisplayPortBaseRect|, and the rect assembled from the >+ * base rect and margins is returned. If this function computes a displayport, >+ * it computes margins and sotres |aDisplayPortBase| as the base rect. >+ * >+ * This is intended to be called during display list building. >+ */ Fix typos: |aScrollFrame}, sotres How about "If this function creates a displayport" instead of "If this function computes a displayport"?

Comment on attachment 8401878 [details] [diff] [review] Part 6 - Make sure the primary async-scrollable frame has a displayport set Talked to Botond on irc about how to fix up the review comments.

(In reply to Timothy Nikkel (:tn) from comment #57) > Comment on attachment 8401872 [details] [diff] [review] > Part 3 - Calculate frame metrics for a display port calculation > > >+ CSSToLayoutDeviceScale deviceScale(nsPresContext::AppUnitsPerCSSPixel() > >+ / presContext->AppUnitsPerDevPixel()); > > I think this will be an int / int division, so it will truncate. We want > float here. Good catch! Fixed. > > >+ // Only the size of the composition bounds is relevant to the > >+ // displayport calculation, not its origin. > >+ metrics.mCompositionBounds > >+ = RoundedToInt(LayoutDeviceRect::FromAppUnits(nsRect(nsPoint(0, 0), aScrollFrame->GetSize()), > >+ presContext->AppUnitsPerDevPixel()) > >+ * (cumulativeResolution / resolution)); > >+ > >+ // This function is used for setting a display port for subframes, so > >+ // aScrollFrame will not be the root content document's root scroll frame. > >+ metrics.SetRootCompositionSize( > >+ nsLayoutUtils::CalculateRootCompositionSize(aScrollFrame, false, metrics)); > > I looked forward in this patch series and this function is called for all > scroll frames include root ones, and root root ones. Which also means that > the calculation of composition bounds here is wrong. Summarizing discussion on IRC: this function shouldn't be called for the RCD-RSF because it's only called from GetOrMaybeCreateDisplayPort() if the frame doesn't already have a displayport, but the RCD-RSF should already have a displayport. However, it doesn't hurt for this function to do the right thing even if that's not the case for some reason. Passing isRootContentDocRootScrollFrame=false to CalculateRootCompositionSize() is still OK because it will still return the correct answer, it'll just recompute it instead of just returning aScrollFrame.mCompositionBounds. However, I updated the calculation of mCompositionBounds here to use CalculateRootCompositionSize() (which gives the correct answer for the RCD-RSF too) rather than aScrollFrame->GetSize() (which does not). (In reply to Timothy Nikkel (:tn) from comment #58) > Comment on attachment 8401876 [details] [diff] [review] > Part 5 - Introduce nsLayoutUtils::SetDisplayPortMargins, with an option to > not repaint > > >+ MOZ_BEGIN_ENUM_CLASS(RepaintMode, uint8_t) > >+ Repaint, > >+ DoNotRepaint > >+ MOZ_END_ENUM_CLASS(PixelCastJustification) > > Is putting PixelCastJustification not a compile error? On platforms that support enum classes, MOZ_END_ENUM_CLASS(Name) just expands to a closing brace. On other platforms it will be a compiler error, though. Thanks for catching it; fixed. > > /** > > * Set the display port base rect for given element to be used with display > > * port margins. > > */ > >- static void SetDisplayPortBase(nsIContent* aContent, const nsRect& aBase); > >+ static void SetDisplayPortBase(nsIContent* aContent, const nsRect& aBase); > > Fix the comment indenting too while you are here. Done. (In reply to Timothy Nikkel (:tn) from comment #59) > Comment on attachment 8401878 [details] [diff] [review] > Part 6 - Make sure the primary async-scrollable frame has a displayport set > > I don't know how I like the name primary async-scrollable. How about we just > keep track if we've found a displayport? So call it "mFoundADisplayPort"? After discussing this on IRC, we landed on mHaveScrollableDisplayPort. mHaveDisplayPort would be inaccurate as the RCD-RSF always has a displayport but if it's not actually scrollable it doesn't count for purposes of this flag. > I'm not a stickler, but some of your lines go quite far beyond 80 chars. I toned it down a bit. The patch still doesn't strictly respect 80, but then neither does a lot of other code in the same files :) > >+bool > >+nsLayoutUtils::GetOrMaybeCreateDisplayPort(nsDisplayListBuilder& aBuilder, > >+ nsIFrame* aScrollFrame, > >+ nsRect aDisplayPortBase, > >+ nsRect* aOutDisplayport) { > >+ nsIContent* content = aScrollFrame->GetContent(); > >+ nsIScrollableFrame* scrollableFrame = do_QueryFrame(aScrollFrame); > >+ if (!content || !aScrollFrame) { > >+ return false; > >+ } > > We don't need scrollableFrame until further down, so move it there. I meant the null check here to be '!scrollableFrame' rather than '!aScrollFrame'. aScrollFrame already has to be non-null since we call GetContent() on it above. I fixed the null check. > >+ FrameMetrics metrics = CalculateFrameMetricsForDisplayPort(aScrollFrame, scrollableFrame); > >+ LayerMargin displayportMargins = AsyncPanZoomController::CalculatePendingDisplayPort( > >+ metrics, ScreenPoint(0.0f, 0.0f), 0.0); > >+ nsIPresShell* presShell = aScrollFrame->PresContext()->GetPresShell(); > >+ uint32_t alignment = gfxPrefs::LayersTilesEnabled() ? TILEDLAYERBUFFER_TILE_SIZE : 1; > >+ nsLayoutUtils::SetDisplayPortMargins(content, presShell, displayportMargins, alignment, 0, > >+ nsLayoutUtils::RepaintMode::DoNotRepaint); > > I think we want to use 0 for alignment if we aren't tiling, because if > alignment is non-zero we will inflate the rect by 1 and then align to > alignment. The is not specific to this call site of SetDisplayPortMargins; the ones in APZCCallbackHelper::UpdateRootFrame and APZCCallbackHelper::UpdateSubFrame also pass 1. As dicussed on IRC, I'll file a follow-up a fixing all three call sites. > >+ /** > >+ * Get the display port for |aScrollFrame|'s content. If |aScrollFrame} > >+ * WantsAsyncScroll() and we don't have a displayport for the primary > >+ * async-scrollable frame yet (as tracked by |aBuilder|), calculate and set > >+ * a display port. Returns true if there is (now) a displayport, and if so > >+ * the displayport is returned in |aOutDisplayport|. > >+ * > >+ * Note that a displayport can either be stored as a rect, or as a base > >+ * rect + margins. If it is stored as a base rect + margins, the base rect > >+ * is updated to |aDisplayPortBaseRect|, and the rect assembled from the > >+ * base rect and margins is returned. If this function computes a displayport, > >+ * it computes margins and sotres |aDisplayPortBase| as the base rect. > >+ * > >+ * This is intended to be called during display list building. > >+ */ > > Fix typos: |aScrollFrame}, sotres > > How about "If this function creates a displayport" instead of "If this > function computes a displayport"? Fixed. I will post updated patches shortly.

Rebased and addressed review comments.

Rebased and addressed review comments. Carrying r+.

Rebased and addressed review comments. Carrying r+.

(In reply to Botond Ballo [:botond] from comment #61) > > >+ FrameMetrics metrics = CalculateFrameMetricsForDisplayPort(aScrollFrame, scrollableFrame); > > >+ LayerMargin displayportMargins = AsyncPanZoomController::CalculatePendingDisplayPort( > > >+ metrics, ScreenPoint(0.0f, 0.0f), 0.0); > > >+ nsIPresShell* presShell = aScrollFrame->PresContext()->GetPresShell(); > > >+ uint32_t alignment = gfxPrefs::LayersTilesEnabled() ? TILEDLAYERBUFFER_TILE_SIZE : 1; > > >+ nsLayoutUtils::SetDisplayPortMargins(content, presShell, displayportMargins, alignment, 0, > > >+ nsLayoutUtils::RepaintMode::DoNotRepaint); > > > > I think we want to use 0 for alignment if we aren't tiling, because if > > alignment is non-zero we will inflate the rect by 1 and then align to > > alignment. > > The is not specific to this call site of SetDisplayPortMargins; the ones in > APZCCallbackHelper::UpdateRootFrame and APZCCallbackHelper::UpdateSubFrame > also pass 1. As dicussed on IRC, I'll file a follow-up a fixing all three > call sites. I filed bug 994816.

https://hg.mozilla.org/integration/mozilla-inbound/rev/95b341d26f7b https://hg.mozilla.org/integration/mozilla-inbound/rev/6642718ad3bb https://hg.mozilla.org/integration/mozilla-inbound/rev/ffe51d96e86e https://hg.mozilla.org/integration/mozilla-inbound/rev/af219bb49c06 https://hg.mozilla.org/integration/mozilla-inbound/rev/2ad9ce9d904e https://hg.mozilla.org/integration/mozilla-inbound/rev/16540ab2d2cb

Backed out for Windows and OSX bustage. Please verify this cross-platform codes builds on all platforms before pushing again. https://hg.mozilla.org/integration/mozilla-inbound/rev/f2b8543b60ab https://tbpl.mozilla.org/php/getParsedLog.php?id=37589069&tree=Mozilla-Inbound https://tbpl.mozilla.org/php/getParsedLog.php?id=37588889&tree=Mozilla-Inbound

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #67) > Backed out for Windows and OSX bustage. Please verify this cross-platform > codes builds on all platforms before pushing again. > https://hg.mozilla.org/integration/mozilla-inbound/rev/f2b8543b60ab > > https://tbpl.mozilla.org/php/getParsedLog.php?id=37589069&tree=Mozilla- > Inbound > https://tbpl.mozilla.org/php/getParsedLog.php?id=37588889&tree=Mozilla- > Inbound Sorry about that, I should have Tried first! The patches had a few problems: - The static namespace-scope function CalculateFrameMetricsForDisplayPort() was only called on B2G, and this made some non-B2G compilers complain about an unused static function. Solution: guard its declaration with #ifdef MOZ_WIDGET_GONK. - MOZ_[BEGIN_END]_ENUM_CLASS cannot be used at class scope. There is a MOZ_[BEGIN|END]_NESTED_ENUM_CLASS macro for this purpose. - nsSubDocumentFrame::BuildDisplayList() needed to check that rootScrollFrame != nullptr before calling GetOrMaybeCreateDisplayPort(). Tryish with these changes before relanding: https://tbpl.mozilla.org/?tree=Try&rev=3afe02a1ed92

(In reply to Botond Ballo [:botond] from comment #68) > - nsSubDocumentFrame::BuildDisplayList() needed to check that > rootScrollFrame != nullptr before calling GetOrMaybeCreateDisplayPort(). > > Tryish with these changes before relanding: > https://tbpl.mozilla.org/?tree=Try&rev=3afe02a1ed92 Wasn't aggressive enough with the null checks, got many crashes still. ReTrying: https://tbpl.mozilla.org/?tree=Try&rev=3b0654023687

(In reply to Botond Ballo [:botond] from comment #69) > (In reply to Botond Ballo [:botond] from comment #68) > > - nsSubDocumentFrame::BuildDisplayList() needed to check that > > rootScrollFrame != nullptr before calling GetOrMaybeCreateDisplayPort(). > > > > Tryish with these changes before relanding: > > https://tbpl.mozilla.org/?tree=Try&rev=3afe02a1ed92 > > Wasn't aggressive enough with the null checks, got many crashes still. > > ReTrying: https://tbpl.mozilla.org/?tree=Try&rev=3b0654023687 Looks clean except for Linux debug bc1, which looks unrelated and which I saw failing on many other Try pushes too. Inbound is closed, marking as checkin-needed.

https://hg.mozilla.org/integration/mozilla-inbound/rev/5f35c0238eda https://hg.mozilla.org/integration/mozilla-inbound/rev/6f65fda604bc https://hg.mozilla.org/integration/mozilla-inbound/rev/6e4f4b4d730d https://hg.mozilla.org/integration/mozilla-inbound/rev/2afd32b60dfd https://hg.mozilla.org/integration/mozilla-inbound/rev/656f96723134 https://hg.mozilla.org/integration/mozilla-inbound/rev/a9e22794850e

https://hg.mozilla.org/mozilla-central/rev/5f35c0238eda https://hg.mozilla.org/mozilla-central/rev/6f65fda604bc https://hg.mozilla.org/mozilla-central/rev/6e4f4b4d730d https://hg.mozilla.org/mozilla-central/rev/2afd32b60dfd https://hg.mozilla.org/mozilla-central/rev/656f96723134 https://hg.mozilla.org/mozilla-central/rev/a9e22794850e

[Approval Request Comment] Bug caused by (feature/regressing bug #): APZ User impact if declined: For almost all Gaia apps with scrollable content, there is a noticeable lag between the user first panning on the content, and the content moving. Has been described as "our worst and most obvious performance bug at the moment from a user perspective" (comment 2). Web pages can be similarly affected as well, though that's less common. Testing completed (on m-c, etc.): Tested locally, and has been baking on m-c since Monday. Risk to taking this patch (and alternatives if risky): The patch causes the primary scrollable frame to be layerized on startup instead of when the user first pans. Layerization uses memory. Therefore, this patch can increase memory usage unnecessarily in cases where the user opens an app but does not pan its scrollable content. However, I don't think this is a concern, as typically when a user opens an app with scrollable content, they subsequently start panning it. String or IDL/UUID changes made by this patch: None Note: As per the whiteboard, please wait until Monday before actually uplifting. Thanks!

> User impact if declined: > For almost all Gaia apps with scrollable content, there is a noticeable > lag > between the user first panning on the content, and the content moving. > Has been described as "our worst and most obvious performance bug at the > moment > from a user perspective" (comment 2). > Web pages can be similarly affected as well, though that's less common. > > Testing completed (on m-c, etc.): > Tested locally, and has been baking on m-c since Monday. > > Risk to taking this patch (and alternatives if risky): > The patch causes the primary scrollable frame to be layerized on startup > instead of when the user first pans. Layerization uses memory. Therefore, > this patch can increase memory usage unnecessarily in cases where the user > opens an app but does not pan its scrollable content. However, I don't > think this is a concern, as typically when a user opens an app with > scrollable content, they subsequently start panning it. I should also mention that this patch has no effect on non-B2G platforms.

Triage drive-by: waiting until monday to approve uplift.