Closed Bug 975931 Opened 11 years ago Closed 10 years ago

Give reftests the ability to test async scrolling

Categories

(Core :: Graphics: Layers, defect)

Product:
Core
Component:
Graphics: Layers
Platform:
x86_64
Linux
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla30

People

(Reporter: roc, Assigned: roc)

References

Details

(Whiteboard: [rr])

Attachments

(10 files, 4 obsolete files)

Part 2: Update README with complete documentation of all attributes, and put the attribute name in the section heading
(deleted), patch
dbaron
: review+
Details | Diff | Splinter Review
Part 3: When the reftest-async-scroll attribute is present, allow reftest-displayport to be set on any element
(deleted), patch
tnikkel
: review+
Details | Diff | Splinter Review
Part 4: Support reftest-async-scroll attributes
(deleted), patch
dbaron
: review+
Details | Diff | Splinter Review
Part 5: Propagate scrolling behavior for non-app non-browser remote <iframe>s
(deleted), patch
khuey
: review+
Details | Diff | Splinter Review
Part 7: Enable APZC in reftests
(deleted), patch
dbaron
: review+
Details | Diff | Splinter Review
Part 8: When we reduce a layer's shadow-visible-region due to opaque covering content, restore it after compositing to ensure it's corr ect if the opaque content isn't there next time
(deleted), patch
mattwoodrow
: review+
Details | Diff | Splinter Review
Part 9: Add basic reftests for async scrolling
(deleted), patch
tnikkel
: review+
Details | Diff | Splinter Review
Part 1 v2
(deleted), patch
tnikkel
: review+
Details | Diff | Splinter Review
Part 10: Refactor nsDOMWindowUtils to use a shared GetDocument method
(deleted), patch
MatsPalmgren_bugz
: review+
Details | Diff | Splinter Review
Part 11: Make nsDOMWindowUtils::SetDisplayPortForElement take a priority parameter, and set that parameter to 1 in reftests to override automatic displayport selection
(deleted), patch
kats
: review+
Details | Diff | Splinter Review 
I have more tests, but one of them fails due to bug 976412, so I'll attach more tests there.
Comment on attachment 8381302 [details] [diff] [review]
Part 6: We need to take the normal viewport scrolling path to ensure the right layers are created

This actually is much more complicated then it seems.

In order to solve bug 935219 we initially tried to do this in bug 951936, but we abandoned it (I don't recall the details of why off the top of my head right now) and we went with the opposite approach of heeding the "ignore scroll frame" even on subdocuments to make the behaviour consistent. I'm told that the basic problem is that AZPC isn't written to cope with the zoom (resolution) being on one layer (the container layer for the document) and the pan (scrolling) on another layer (the scroll layer created from a scroll layer item), and it would take a lot of work to change that.
Botond, see comment 11, do you remember why the approach from bug 951936 was abandoned?
(In reply to Timothy Nikkel (:tn) from comment #11)
> ... we initially tried to do this in bug 951936,
> but we abandoned it ... and we went with the opposite approach of heeding the
> "ignore scroll frame" even on subdocuments ...

The opposite approach is bug 959847.
Comment on attachment 8381295 [details] [diff] [review]
Part 1: Add nsDOMWindowUtils API to add an extra scroll offset in the AsyncPanZoomController when compositing

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

::: gfx/layers/ipc/AsyncPanZoomController.h
@@ +732,5 @@
>  
>    // Specifies whether mPreventDefault property is set for current touch events block.
>    bool mPreventDefaultSet;
>  
> +  // Extra offset to add in SampleContentTransformForFrame for testing 

nit: trailing ws

::: gfx/layers/ipc/LayerTransactionParent.cpp
@@ +613,5 @@
> +  AsyncPanZoomController* controller = layer->GetAsyncPanZoomController();
> +  if (!controller) {
> +    return true;
> +  }
> +  controller->SetTestAsyncScrollOffset(CSSPoint(aX, aY));  

nit: trailing ws
Attachment #8381295 - Flags: review?(bugmail.mozilla) → review+
(In reply to Timothy Nikkel (:tn) from comment #12)
> Botond, see comment 11, do you remember why the approach from bug 951936 was
> abandoned?

It's basically what you said in comment #11. Bug 951936 would have made things so that when a scrollable layer is zoomed, the zoom ends up not in the layer's own resolution, but in a parent layer's resolution. This is not good for APZC, because in APZC we need to be able to distinguish between our own layer being zoomed and a parent layer being zoomed. We could have made it work but it was looking more complicated than necessary, and the alternative approach (bug 959847) was looking simpler.

I'm not opposed to reevaluating this if the bug 959847 is turning out to have complications, but I'd rather do it as a follow-up rather than holding back bug 935219 any further because it fixes many things.
Attachment #8381304 - Flags: review?(matt.woodrow) → review+
Comment on attachment 8381296 [details] [diff] [review]
Part 2: Update README with complete documentation of all attributes, and put the attribute name in the section heading

Could you actually make the syntax snippets look even more like the syntax, like so:

  class="reftest-wait"
  "MozReftestInvalidate" (leave as-is)
  reftest-zoom="<float>"

since otherwise it's easy to miss the distinction between things that are classes and things that are attributes (especially, for, say, reftest-no-sync-layers, which is a boolean in an attribute rather than a class).

r=dbaron with that
Attachment #8381296 - Flags: review?(dbaron) → review+
Comment on attachment 8381300 [details] [diff] [review]
Part 4: Support reftest-async-scroll attributes

>+Testing Async Scrolling: "reftest-async-scroll-x/y"

see comment 16

also mention the "reftest-async-scroll" attribute in the title

>+        var sx = attrOrDefault(element, "reftest-async-scroll-x", 0);
>+        var sy = attrOrDefault(element, "reftest-async-scroll-y", 0);

put Number() around these, before you do != comparisons with them

>+        for (var c = element.firstChild; c; c = c.nextSibling) {
>+            if (c.nodeType == c.ELEMENT_NODE) {

Just use firstElementChild and nextElementSibling.

>+                // This might fail when called from RecordResult since layers
>+                // may not have been constructed yet

Maybe have an "allow failure" boolean so that you don't allow this
failure in the SynchronizeForSnapshot case?

>+    // Setup async scroll offsets now in case SynchronizeForSnapshot is not
>+    // called.
...
>+    // Set up async scroll offsets now, because any scrollable layers should
>+    // have had their AsyncPanZoomControllers created.

In both of these comments I think you should mention
reftest-no-sync-layers explicitly.  Or are there other reasons for the
need to have this in 2 places?  If so, please explain.

r=dbaron with that
Attachment #8381300 - Flags: review?(dbaron) → review+
Comment on attachment 8381303 [details] [diff] [review]
Part 7: Enable APZC in reftests

It seems a little bad to have this enabled even on platforms where we don't have APZC enabled.  Or does this patch not actually do that?  Or is there something that guarantees that we don't lose the test coverage of no-APZC, or that that coverage isn't worthwhile?
Flags: needinfo?(roc)
(In reply to David Baron [:dbaron] (needinfo? me) (UTC-8) from comment #18)
> It seems a little bad to have this enabled even on platforms where we don't
> have APZC enabled.  Or does this patch not actually do that?  Or is there
> something that guarantees that we don't lose the test coverage of no-APZC,
> or that that coverage isn't worthwhile?

These patches have no effect when OMTC is not enabled. They do eliminate coverage of OMTC non-APZC configurations, but I'm not worried about that because the existence of APZCs is most unlikely to change behavior for any reftest that's not using reftest-async-scroll. For example our scrolling reftests will not be affected since the scrolling is driven by the main thread.

Setting these prefs when the browser still defaulting OMTC non-APZC is good because it means desktop reftest-ipc can catch regressions. That's easier to test and easier to debug than having to use an APZC-enabled mobile device.
Flags: needinfo?(roc)
(In reply to Timothy Nikkel (:tn) from comment #11)
> In order to solve bug 935219 we initially tried to do this in bug 951936,
> but we abandoned it (I don't recall the details of why off the top of my
> head right now) and we went with the opposite approach of heeding the
> "ignore scroll frame" even on subdocuments to make the behaviour consistent.
> I'm told that the basic problem is that AZPC isn't written to cope with the
> zoom (resolution) being on one layer (the container layer for the document)
> and the pan (scrolling) on another layer (the scroll layer created from a
> scroll layer item), and it would take a lot of work to change that.

OK. How does async scrolling of the viewport work then, since ScrollFrameHelper::BuildDisplayList bails out when ignoring the root scroll frame without creating an nsDisplayScrollLayer?
Flags: needinfo?(tnikkel)
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #20)
> OK. How does async scrolling of the viewport work then, since
> ScrollFrameHelper::BuildDisplayList bails out when ignoring the root scroll
> frame without creating an nsDisplayScrollLayer?

Through a lucky coincidence ignoring viewport scrolling only has an effect when painting a display root document (it's mostly ignored otherewise, and definitely ignored on subdocuments). And we just so happen to call RecordFrameMetrics in nsDisplayList::PaintForFrame and expand the dirty rect to the displayport in nsLayoutUtils::PaintFrame and nsDisplayList::PaintForFrame. AZPC doesn't care much if the container layer it's scrolling came from an nsDisplayScrollLayer or from somewhere else, as long as it has a properly filled in FrameMetrics and the correct content painted to the layer.

b2g is the only thing that does it this way, I don't think it was planned, the code just worked and nobody noticed or cared because of that. Root scroll frames are stacking contexts so they don't have to deal with some of the acrobatics that nsDisplayScrollLayer does so it works.
Flags: needinfo?(tnikkel)
Comment on attachment 8381303 [details] [diff] [review]
Part 7: Enable APZC in reftests

ok, r=dbaron

(following comment 18 and comment 19)
Attachment #8381303 - Flags: review?(dbaron) → review+
(In reply to Timothy Nikkel (:tn) from comment #21)
> Through a lucky coincidence ignoring viewport scrolling only has an effect
> when painting a display root document (it's mostly ignored otherewise, and
> definitely ignored on subdocuments). And we just so happen to call
> RecordFrameMetrics in nsDisplayList::PaintForFrame and expand the dirty rect
> to the displayport in nsLayoutUtils::PaintFrame and
> nsDisplayList::PaintForFrame. AZPC doesn't care much if the container layer
> it's scrolling came from an nsDisplayScrollLayer or from somewhere else, as
> long as it has a properly filled in FrameMetrics and the correct content
> painted to the layer.

OK thanks. The reason for the ignore-viewport-scrolling removal patch was that my code needed to find the layer with the APZC on it, and I was looking for the layer created by nsDisplayScrollLayer, so it wasn't there when ignoring viewport scrolling. So I should be able to fix this by dropping the ignore-viewport-removal patch and instead special-casing finding the scrolling layer for the root element.
Attached patch Part 1 v2 (deleted) — — Splinter Review 
Part 6 is no longer needed.
Attachment #8381295 - Attachment is obsolete: true
Attachment #8381302 - Attachment is obsolete: true
Attachment #8381302 - Flags: review?(tnikkel)
Comment on attachment 8382672 [details] [diff] [review]
Part 1 v2

Perhaps before using the root layer of the widget's layer manager we could check that we are the root document in the widget?
Attachment #8382672 - Flags: review?(tnikkel) → review+
Attachment #8381297 - Flags: review?(tnikkel) → review+
Comment on attachment 8381305 [details] [diff] [review]
Part 9: Add basic reftests for async scrolling

>+++ b/layout/reftests/reftest-sanity/async-scroll-1b.html
>+<body style="background:red; margin:0; overflow:hidden">
>+  <div style="position:absolute; background:lime; top:50px; left:0; width:800px; height:1000px"></div>
>+  <div style="position:absolute; background:blue; top:1000px; left:50px; width:50px; height:50px"></div>
>+</body>

blue should be yellow here I think. You've probably fixed this already I'm guessing.
Attachment #8381305 - Flags: review?(tnikkel) → review+
The problem is that TabChild responds to before-first-paint by setting the displayport on the root element. This can wipe out the displayport set by the reftest harness. I'm not sure how to fix this.
Whiteboard: [rr]
One option is to add another nsDOMWindowUtils API, say LockDisplayPortForElement, which sets a property on the element which causes TabChild to refuse to overwrite the displayport. reftest-content could then call that API when it sets the displayport.
But that doesn't sound very good...

Maybe what we need is an nsDOMWindowUtils API to disable setting of viewport metadata via TabChild, giving it to whoever's calling setDisplayPortForElement etc manually.
Comment on attachment 8386002 [details] [diff] [review]
Part 10: Refactor nsDOMWindowUtils to use a shared GetDocument method

'mWindow' is a nsWeakPtr so please consider whether removing the strong ref
on the stack matters to any of these methods.

> getScrollXYAppUnits(nsWeakPtr aWindow, bool aFlushLayout, nsPoint& aScrollPos) {

While you're here, please move the '{' to the next line
and s/get/Get/
Attachment #8386002 - Flags: review?(matspal) → review+
Comment on attachment 8386016 [details] [diff] [review]
Part 11: Add nsDOMWindowUtils::configureViewportAutomatically

Having to guard every callsite seems like it is going to be error prone in the long term.
But we don't want to guard every call site, just the "automatic" ones.

We could introduce new nsDOMWindowUtils APIs, or flags to existing APIs, to indicate which calls are "automatic", but I don't see that as much better.
Flags: needinfo?(tnikkel)
Frankly I'll do whatever you like here, just tell me which way you want it :-)
Comment on attachment 8386016 [details] [diff] [review]
Part 11: Add nsDOMWindowUtils::configureViewportAutomatically

>+nsDOMWindowUtils::SetConfigureViewportAutomatically(bool aValue)
>+    doc->SetProperty(nsGkAtoms::configureViewportManually,
>+                     nsGkAtoms::configureViewportManually);

Can we set it to something simple like 1 or true?

>@@ -381,18 +381,22 @@ TabChild::Observe(nsISupports *aSubject,
>+          bool configureAutomatically;
>+          utils->GetConfigureViewportAutomatically(&configureAutomatically);
>+          if (configureAutomatically) {
>+                utils->SetResolution(mLastRootMetrics.mResolution.scale,
>+                                     mLastRootMetrics.mResolution.scale);
>+          }

The indentation of this file isn't consistent it seems, but right here it's using 2 space indents and this looks out of place.

>@@ -154,16 +154,22 @@ APZCCallbackHelper::UpdateRootFrame(nsID
>+    bool configureAutomatically;
>+    aUtils->GetConfigureViewportAutomatically(&configureAutomatically);
>+    if (!configureAutomatically) {
>+        return;
>+    }

Normally I like early exits, but this one makes me hope no one adds anything after it that should still be done regardless of the configureAutomatically setting. So maybe make it a big if?
Attachment #8386016 - Flags: review?(tnikkel) → review+
Flags: needinfo?(tnikkel)
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #29)
> The problem is that TabChild responds to before-first-paint by setting the
> displayport on the root element. This can wipe out the displayport set by
> the reftest harness.

Do we know why this wasn't a problem before these patches? There are already reftests which set a displayport on the root element, and that's the only thing TabChild "automatically" clobbers.

(Also for the record I don't like the configureAutomatically flag solution and would like to come up with a better solution).
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #37)
> But we don't want to guard every call site, just the "automatic" ones.

Also I'm not entirely sure what qualifies as a non-automatic site. The stuff in APZCCallbackHelper is invoked on APZC-driven scrolling too. I assume that qualifies as non-automatic so if in the future we have tests that trigger APZC scrolling and also set a displayport then that won't work? I think ideally I would like to not have to do any special handling for "automatic" vs "other" call sites, because that way the test is more representative of what happens in production code.

I would lean towards a different approach, where TabChild checks to see if properties are set in the DOM and updates the metrics accordingly before it calls into APZCCallbackHelper or anything else.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #40)
> (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #29)
> > The problem is that TabChild responds to before-first-paint by setting the
> > displayport on the root element. This can wipe out the displayport set by
> > the reftest harness.
> 
> Do we know why this wasn't a problem before these patches? There are already
> reftests which set a displayport on the root element, and that's the only
> thing TabChild "automatically" clobbers.

On Linux Ripc, it was because IsAsyncPanZoomEnabled was false. I don't know about B2G.

> (Also for the record I don't like the configureAutomatically flag solution
> and would like to come up with a better solution).

OK.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #41)
> Also I'm not entirely sure what qualifies as a non-automatic site. The stuff
> in APZCCallbackHelper is invoked on APZC-driven scrolling too. I assume that
> qualifies as non-automatic

No, that's automatic.

> so if in the future we have tests that trigger
> APZC scrolling and also set a displayport then that won't work? I think
> ideally I would like to not have to do any special handling for "automatic"
> vs "other" call sites, because that way the test is more representative of
> what happens in production code.

reftests need to set a particular displayport and have that not be changed by APZC or TabChild. That isn't compatible with what happens in production code.

To make this more representative of what happens in production code, we could try to throw out the explicit reftest-displayport attributes and set <meta viewport> and other data so that APZC and TabChild configure the displayport we want. But that would make reftests dependent on the displayport sizing heuristics used by APZC which seems bad.

> I would lean towards a different approach, where TabChild checks to see if
> properties are set in the DOM and updates the metrics accordingly before it
> calls into APZCCallbackHelper or anything else.

I'm not sure which properties you mean. You mean TabChild should check for the reftest-async-scroll attribute? Can you give me a more detailed suggestion?
Flags: needinfo?(bugmail.mozilla)
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #43)
> > I would lean towards a different approach, where TabChild checks to see if
> > properties are set in the DOM and updates the metrics accordingly before it
> > calls into APZCCallbackHelper or anything else.
> 
> I'm not sure which properties you mean. You mean TabChild should check for
> the reftest-async-scroll attribute? Can you give me a more detailed
> suggestion?

Yeah, I was thinking that TabChild could check for reftest-async-scroll. The obvious problem is that random web content might start manipulating the displayport by setting this property, so I was thinking of a layer of abstraction where the reftest harness reads the property like it does now, but sets it somewhere in DOMWindowUtils and then TabChild picks it up from there.

However, thinking about it a bit more, an alternative approach might be for the reftest harness to set an "override displayport" instead of setting the "displayport". So instead of calling setDisplayPortForElement(...) it would call setOverrideDisplayPortForElement(...) which behaves identically but sets a nsGkAtoms::OverrideDisplayPort property instead of nsGkAtoms::DisplayPort. And then, in nsLayoutUtils::GetDisplayPort, we read the override property and, if it exists, return that instead of the regular displayport. That way the call sites are left untouched but layout will always use the reftest-specified displayport.
Flags: needinfo?(bugmail.mozilla)
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #44)
> Yeah, I was thinking that TabChild could check for reftest-async-scroll. The
> obvious problem is that random web content might start manipulating the
> displayport by setting this property, so I was thinking of a layer of
> abstraction where the reftest harness reads the property like it does now,
> but sets it somewhere in DOMWindowUtils and then TabChild picks it up from
> there.

Isn't that what I've done here, except that the property's meaning is inverted and it doesn't have a reftest-specific name?

> However, thinking about it a bit more, an alternative approach might be for
> the reftest harness to set an "override displayport" instead of setting the
> "displayport". So instead of calling setDisplayPortForElement(...) it would
> call setOverrideDisplayPortForElement(...) which behaves identically but
> sets a nsGkAtoms::OverrideDisplayPort property instead of
> nsGkAtoms::DisplayPort. And then, in nsLayoutUtils::GetDisplayPort, we read
> the override property and, if it exists, return that instead of the regular
> displayport. That way the call sites are left untouched but layout will
> always use the reftest-specified displayport.

I think that is a little more complex than necessary, especially if we later need to add "override zoom" and other stuff.

How about this: extend setDisplayPortForElement with an extra parameter that indicates the "priority level". Normal users set it with priority level 0. Other users (reftests) set a higher priority level. Setting a lower priority value when a higher-priority value is in place is silently ignored. Store the priority level in the property value alongside the displayport's nsRect. Use the same pattern for other properties later if needed.
Flags: needinfo?(bugmail.mozilla)
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #45)
> (In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #44)
> > Yeah, I was thinking that TabChild could check for reftest-async-scroll. The
> > obvious problem is that random web content might start manipulating the
> > displayport by setting this property, so I was thinking of a layer of
> > abstraction where the reftest harness reads the property like it does now,
> > but sets it somewhere in DOMWindowUtils and then TabChild picks it up from
> > there.
> 
> Isn't that what I've done here, except that the property's meaning is
> inverted and it doesn't have a reftest-specific name?

Yeah I guess so. I was thinking that we could just update the FrameMetrics in TabChild before calling APZCCallbackHelper::UpdateRootFrame with the reftest-specified values. That would be marginally better because APZCCallbackHelper would not need to be modified, but overall that may not be much better because of the call sites to SetResolution and SetCSSViewport in TabChild itself.

> How about this: extend setDisplayPortForElement with an extra parameter that
> indicates the "priority level". Normal users set it with priority level 0.
> Other users (reftests) set a higher priority level. Setting a lower priority
> value when a higher-priority value is in place is silently ignored. Store
> the priority level in the property value alongside the displayport's nsRect.
> Use the same pattern for other properties later if needed.

This sounds fine to me. We might eventually need a way to un-set a high priority value to allow lower-priority users to control it again but for now we can probably get by without it.
Flags: needinfo?(bugmail.mozilla)
Comment on attachment 8388048 [details] [diff] [review]
Part 11: Make nsDOMWindowUtils::SetDisplayPortForElement take a priority parameter, and set that parameter to 1 in reftests to override automatic displayport selection

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

r=me with comment fixed

::: dom/base/nsDOMWindowUtils.cpp
@@ +398,5 @@
>    }
>  
> +  DisplayPortPropertyData* currentData =
> +    static_cast<DisplayPortPropertyData*>(content->GetProperty(nsGkAtoms::DisplayPort));
> +  if (currentData && currentData->mPriority < aPriority) {

Shouldn't this be > instead of < ? If aPriority is big you don't want to early-exit.
Attachment #8388048 - Flags: review?(bugmail.mozilla) → review+
I realize my review of part 7 was insufficient, and it should have changed bootstrap.js as well (which I believe affects Android and B2G).  I'm a little afraid to "just fix" this; I suspect try server use is probably in order.
Flags: needinfo?(roc)
(I also filed bug 986807 on the lists of prefs being out-of-sync.)
... where it turns out flipping the prefs doesn't appear to introduce new failures.
Flags: needinfo?(roc)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: