Closed Bug 982141 Opened 6 years ago Closed 6 years ago

Only the root scrollable document will get a displayport

Categories

(Core :: Panning and Zooming, defect)

All
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla31
Tracking Status
firefox29 --- wontfix
firefox30 --- fixed
firefox31 --- fixed
b2g-v1.3 --- wontfix
b2g-v1.4 --- fixed
b2g-v2.0 --- fixed

People

(Reporter: cwiiis, Assigned: botond)

References

Details

Attachments

(7 files, 24 obsolete files)

8.10 KB, patch
botond
: review+
Details | Diff | Splinter Review
11.98 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
4.03 KB, patch
kats
: review+
tnikkel
: review+
Details | Diff | Splinter Review
3.10 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
7.78 KB, patch
botond
: review+
Details | Diff | Splinter Review
15.78 KB, patch
botond
: review+
Details | Diff | Splinter Review
46.63 KB, patch
Details | Diff | Splinter Review
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.
Assignee: nobody → 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.
blocking-b2g: --- → 1.4?
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.
Attachment #8390047 - Flags: feedback?(tnikkel) → feedback+
(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.
blocking-b2g: 1.4? → 1.4+
Attachment #8390047 - Flags: review?(tnikkel)
Note: I changed my mind about taking individual fields of FrameMetrics here.
Attachment #8390678 - Flags: review?(bugmail.mozilla)
Attachment #8390680 - Flags: review?(bugmail.mozilla)
Attachment #8390683 - Flags: review?(tnikkel)
Attachment #8390683 - Flags: review?(bugmail.mozilla)
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.
Attachment #8390683 - Attachment is obsolete: true
Attachment #8390683 - Flags: review?(tnikkel)
Attachment #8390683 - Flags: review?(bugmail.mozilla)
Attachment #8390692 - Flags: review?(tnikkel)
Attachment #8390692 - Flags: review?(bugmail.mozilla)
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.
Attachment #8390674 - Attachment description: Part 3 - Find the primary async-scrollable frame → [old approach] Part 3 - Find the primary async-scrollable frame
Attachment #8390674 - Flags: review?(tnikkel)
Attachment #8390692 - Attachment description: Part 6 - Make sure the primary async-scrollable frame has an initial displayport → [old approach] Part 6 - Make sure the primary async-scrollable frame has an initial displayport
Attachment #8390692 - Flags: review?(tnikkel)
Attachment #8390692 - Flags: review?(bugmail.mozilla)
Attachment #8390674 - Attachment is obsolete: true
Attachment #8390946 - Flags: review?(tnikkel)
Attachment #8390692 - Attachment is obsolete: true
Attachment #8390948 - Flags: review?(tnikkel)
Attachment #8390948 - Flags: review?(bugmail.mozilla)
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.
Attachment #8390678 - Flags: review?(bugmail.mozilla) → review+
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.
Attachment #8390680 - Flags: review?(bugmail.mozilla) → review+
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.
Attachment #8390948 - Flags: review?(bugmail.mozilla) → review-
(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.
Attachment #8390948 - Attachment is obsolete: true
Attachment #8390948 - Flags: review?(tnikkel)
Attachment #8391279 - Flags: review?(tnikkel)
Attachment #8391279 - Flags: review?(bugmail.mozilla)
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.
Attachment #8391279 - Flags: review?(bugmail.mozilla) → review+
(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.
Attachment #8390678 - Attachment is obsolete: true
Attachment #8391332 - Flags: review?(bugmail.mozilla)
(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+.
Attachment #8390680 - Attachment is obsolete: true
Attachment #8391336 - Flags: review+
(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.
Attachment #8391279 - Attachment is obsolete: true
Attachment #8391279 - Flags: review?(tnikkel)
Attachment #8391339 - Flags: review?(tnikkel)
Attachment #8391332 - Flags: review?(bugmail.mozilla) → review+
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?
Attachment #8390047 - Flags: review?(tnikkel) → review+
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.
Attachment #8390673 - Flags: review?(tnikkel) → review+
(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+.
Attachment #8390047 - Attachment is obsolete: true
Attachment #8392945 - Flags: review+
(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+.
Attachment #8390673 - Attachment is obsolete: true
Attachment #8392946 - Flags: review+
Updated Part 3 patch to reflect the change from IsAsyncScrollable() to WantAsyncScroll().
Attachment #8390946 - Attachment is obsolete: true
Attachment #8390946 - Flags: review?(tnikkel)
Attachment #8392947 - Flags: review?(tnikkel)
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).
Attachment #8391339 - Flags: review?(tnikkel)
Attachment #8392947 - Flags: review?(tnikkel)
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.
Attachment #8392947 - Attachment is obsolete: true
Attachment #8393642 - Flags: review?(tnikkel)
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.
Attachment #8391339 - Attachment is obsolete: true
Attachment #8393644 - Flags: review?(tnikkel)
This contains the bulk of the code for the new approach.
Attachment #8393645 - Flags: review?(tnikkel)
(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.
blocking-b2g: 1.4+ → ---
(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.
Attachment #8393645 - Attachment is obsolete: true
Attachment #8393645 - Flags: review?(tnikkel)
Attachment #8395962 - Flags: review?(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.

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.
Attachment #8392945 - Attachment is obsolete: true
Attachment #8401868 - Flags: review+
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.
Attachment #8392946 - Attachment is obsolete: true
Attachment #8401870 - Flags: review?(tnikkel)
A previous version of this has been reviewed, but things have changed enough that I'd like a re-review.
Attachment #8393642 - Attachment is obsolete: true
Attachment #8393642 - Flags: review?(tnikkel)
Attachment #8401872 - Flags: review?(tnikkel)
Attachment #8401872 - Flags: review?(bugmail.mozilla)
Again, requesting a re-review as things changed a bit since last time.
Attachment #8391332 - Attachment is obsolete: true
Attachment #8391336 - Attachment is obsolete: true
Attachment #8393644 - Attachment is obsolete: true
Attachment #8393644 - Flags: review?(tnikkel)
Attachment #8401874 - Flags: review?(tnikkel)
Attachment #8401874 - Flags: review?(bugmail.mozilla)
Attachment #8395962 - Attachment is obsolete: true
Attachment #8395962 - Flags: review?(tnikkel)
Attachment #8401876 - Flags: review?(tnikkel)
Attachment #8395963 - Attachment is obsolete: true
Attachment #8395963 - Flags: review?(tnikkel)
Attachment #8401878 - Flags: review?(tnikkel)
Attachment #8401874 - Flags: review?(bugmail.mozilla) → review+
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
Attachment #8401872 - Flags: review?(bugmail.mozilla) → review+
Attachment #8401870 - Flags: review?(tnikkel) → review+
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.
Attachment #8401872 - Flags: review?(tnikkel) → review-
Attachment #8401874 - Flags: review?(tnikkel) → review+
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.
Attachment #8401876 - Flags: review?(tnikkel) → review+
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.
Attachment #8401878 - Flags: review?(tnikkel) → review+
(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.
Attachment #8401872 - Attachment is obsolete: true
Attachment #8404824 - Flags: review?(tnikkel)
Rebased and addressed review comments. Carrying r+.
Attachment #8401876 - Attachment is obsolete: true
Attachment #8404826 - Flags: review+
Rebased and addressed review comments. Carrying r+.
Attachment #8401878 - Attachment is obsolete: true
Attachment #8404827 - Flags: review+
(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.
Attachment #8404824 - Flags: review?(tnikkel) → review+
(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.
Keywords: checkin-needed
[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!
Attachment #8407830 - Flags: approval-mozilla-aurora?
Whiteboard: [bake on m-c until 2014-04-21]
> 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.
Whiteboard: [bake on m-c until 2014-04-21]
Attachment #8407830 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Blocks: 945367
Depends on: 1013780
See Also: → 943348
Depends on: 1024595
You need to log in before you can comment on or make changes to this bug.