982141 - Only the root scrollable document will get a displayport

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[Chris Lord [:cwiiis]]

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

Comment 1

[Botond Ballo [:botond]]

I'll have a look at this.

Comment 2

[Botond Ballo [:botond]]

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.

Comment 3

[Botond Ballo [:botond]]

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.

Comment 4

[Botond Ballo [:botond]]

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 5

[Botond Ballo [:botond]]

Attachment: Part 1 - Expose the logic used to decide whether a scrollable frame should be async scrollable

Comment 6

[Timothy Nikkel (:tnikkel)]

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.

Comment 7

[bhavana bajaj [:bajaj]]

(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.

Comment 8

[Botond Ballo [:botond]]

Attachment: Part 2 - Factor out scrollable rect calculation into nsLayoutUtils

Comment 9

[Botond Ballo [:botond]]

Attachment: [old approach] Part 3 - Find the primary async-scrollable frame

Comment 10

[Botond Ballo [:botond]]

Attachment: Part 4 - Refactor MaybeAlignAndClampDisplayport to separate scroll offset adjustment from aligning/clamping

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

Comment 11

[Botond Ballo [:botond]]

Attachment: Part 5 - Introduce APZCCallbackHelper::AlignAndSetDisplayPort

Comment 12

[Botond Ballo [:botond]]

Attachment: Part 6 - Make sure the primary async-scrollable frame has an initial displayport

Comment 13

[Botond Ballo [:botond]]

Attachment: [old approach] Part 6 - Make sure the primary async-scrollable frame has an initial displayport

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.

Comment 14

[Botond Ballo [:botond]]

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.

Comment 15

[Botond Ballo [:botond]]

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.

Comment 16

[Botond Ballo [:botond]]

Attachment: Part 3 - Find the primary async-scrollable frame

Comment 17

[Botond Ballo [:botond]]

Attachment: Part 6 - Make sure the primary async-scrollable frame has a displayport

Comment 18

[Botond Ballo [:botond]]

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 19

[Timothy Nikkel (:tnikkel)]

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 20

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

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 21

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

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 22

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

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 23

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

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.

Comment 24

[Botond Ballo [:botond]]

(In reply to (PTO|Mar15-Mar19) Kartikaya Gupta (email:[e-mail]) 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.

Comment 25

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

(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.

Comment 26

[Botond Ballo [:botond]]

Attachment: Part 6 - Make sure the primary async-scrollable frame has a displayport

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:[e-mail]) 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 27

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

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.

Comment 28

[Botond Ballo [:botond]]

Attachment: Part 4 - Refactor MaybeAlignAndClampDisplayport to separate scroll offset adjustment from aligning/clamping

(In reply to (PTO|Mar15-Mar19) Kartikaya Gupta (email:[e-mail]) 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.

Comment 29

[Botond Ballo [:botond]]

Attachment: Part 5 - Introduce APZCCallbackHelper::AlignAndSetDisplayPort

(In reply to (PTO|Mar15-Mar19) Kartikaya Gupta (email:[e-mail]) 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+.

Comment 30

[Botond Ballo [:botond]]

Attachment: Part 6 - Make sure the primary async-scrollable frame has a displayport

(In reply to (PTO|Mar15-Mar19) Kartikaya Gupta (email:[e-mail]) 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 31

[Timothy Nikkel (:tnikkel)]

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 32

[Timothy Nikkel (:tnikkel)]

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.

Comment 33

[Botond Ballo [:botond]]

Attachment: Part 1 - Expose the logic used to decide whether a scrollable frame should be async scrollable

(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+.

Comment 34

[Botond Ballo [:botond]]

Attachment: Part 2 - Factor out scrollable rect calculation into nsLayoutUtils

(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+.

Comment 35

[Botond Ballo [:botond]]

Attachment: Part 3 - Find the primary async-scrollable frame

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

Comment 36

[Botond Ballo [:botond]]

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.

Comment 37

[Botond Ballo [:botond]]

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).

Comment 38

[Botond Ballo [:botond]]

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

Comment 39

[Botond Ballo [:botond]]

Attachment: Part 3 - Calculate a FrameMetrics for a displayport calculation

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.

Comment 40

[Botond Ballo [:botond]]

Attachment: Part 6 - Extract 'wantSubAPZC' to nsLayoutUtils

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.

Comment 41

[Botond Ballo [:botond]]

Attachment: Part 7 - Make sure the primary async-scrollable frame has a displayport

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

Comment 42

[Botond Ballo [:botond]]

(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.

Comment 43

[Milan Sreckovic [:milan] (needinfo for best results)]

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.

Comment 44

[Chris Lord [:cwiiis]]

(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 45

[Timothy Nikkel (:tnikkel)]

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 46

[Botond Ballo [:botond]]

Attachment: Part 7 - Allow setting a displayport without requesting a repaint

Comment 47

[Botond Ballo [:botond]]

Attachment: Part 8 - Make sure the primary async-scrollable frame has a displayport

Comment 48

[Botond Ballo [:botond]]

> 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.

Comment 49

[Botond Ballo [:botond]]

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

Comment 50

[Botond Ballo [:botond]]

Attachment: Part 1 - Expose the logic used to decide whether a scrollable frame should be async scrollable

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

Comment 51

[Botond Ballo [:botond]]

Attachment: Part 2 - Move CalculateRootCompositionSize to nsLayoutUtils and clean it up a bit

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.

Comment 52

[Botond Ballo [:botond]]

Attachment: Part 3 - Calculate frame metrics for a display port calculation

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

Comment 53

[Botond Ballo [:botond]]

Attachment: Part 4 - Factor out 'wantSubAPZC' into nsLayoutUtils

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

Comment 54

[Botond Ballo [:botond]]

Attachment: Part 5 - Introduce nsLayoutUtils::SetDisplayPortMargins, with an option to not repaint

Comment 55

[Botond Ballo [:botond]]

Attachment: Part 6 - Make sure the primary async-scrollable frame has a displayport set

Comment 56

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

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 57

[Timothy Nikkel (:tnikkel)]

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 58

[Timothy Nikkel (:tnikkel)]

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 59

[Timothy Nikkel (:tnikkel)]

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 60

[Timothy Nikkel (:tnikkel)]

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.

Comment 61

[Botond Ballo [:botond]]

(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.

Comment 62

[Botond Ballo [:botond]]

Attachment: Part 3 - Calculate a FrameMetrics for a displayport calculation

Rebased and addressed review comments.

Comment 63

[Botond Ballo [:botond]]

Attachment: Part 5 - Introduce nsLayoutUtils::SetDisplayPortMargins, with an option to not repaint

Rebased and addressed review comments. Carrying r+.

Comment 64

[Botond Ballo [:botond]]

Attachment: Part 6 - Make sure the primary async-scrollable frame has a displayport

Rebased and addressed review comments. Carrying r+.

Comment 65

[Botond Ballo [:botond]]

(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.

Comment 66

[Botond Ballo [:botond]]

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

Comment 67

[Ryan VanderMeulen [:RyanVM]]

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

Comment 68

[Botond Ballo [:botond]]

(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

Comment 69

[Botond Ballo [:botond]]

(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

Comment 70

[Botond Ballo [:botond]]

(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.

Comment 71

[Botond Ballo [:botond]]

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

Comment 72

[Carsten Book [:Tomcat]]

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

Comment 73

[Botond Ballo [:botond]]

Attachment: Rollup patch for aurora uplift

[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!

Comment 74

[Botond Ballo [:botond]]

> 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.

Comment 75

[Lukas Blakk [:lsblakk] use ?needinfo]

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

Comment 76

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/684a7c304b76