Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Description • 7 years ago

Placeholder bug. I'm not yet sure exactly what this will cover, but it's the next stepping stone towards APZ in webrender.

WIP: https://github.com/staktrace/gecko-dev/commits/wr-apz

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 1 • 7 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6c8668a73838569a176212402115fa1442a90a95

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 2 • 7 years ago

The last try push had a hacky change to remove the scrolling transform from the layer tree which didn't really work properly. It was visibly wrong when enabling text layers for example, because it wouldn't remove the scrolling transform when the text layers used the layer transform to convert the bounds. Here's a new try push without that patch, rebased onto graphics tip:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6b232aa30f70d5bd423c4a2765ccf657ddcbb508

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 3 • 7 years ago

Looks like the last patch in that set (pushing a root stacking context) causes some failures. I'm not really sure why, but the upstream servo/webrender#1211 will make that patch unnecessary so I might as well wait for that.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 4 • 7 years ago

and we'll want servo/webrender#1213 fixed as well to get my patchset to scroll upwards as well as downwards.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 5 • 7 years ago

Latest try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=39d58cdd37c59c0fd7cfbd826c27e3cc6c2bd313

If this is green I'll put the patches up for review. Locally if I enable APZ with webrender I also need to apply patches for pushing a root stacking context [1] and stopping WR from clamping the scroll position [2]. Those will be fixed in WR upstream though, so I'm happy to land these patches for now.

[1] https://github.com/staktrace/gecko-dev/commit/2399ec0ab7b23ac8463058ecb38ea7f8a3eba1bd
[2] https://github.com/staktrace/gecko-dev/commit/abc1b6bf68d35e06cac310f0f63ced68048aaa7e

	Jeff Muizelaar [:jrmuizel]
	Comment 11 • 7 years ago
mozreview-review

Comment on attachment 8866484 [details]
Bug 1361497 - Clean up the WR APIs for pushing scrolling clips vs non-scrolling clips.

https://reviewboard.mozilla.org/r/138112/#review142062

	Jeff Muizelaar [:jrmuizel]
	Comment 12 • 7 years ago
mozreview-review

Comment on attachment 8866485 [details]
Bug 1361497 - Override AsyncPanZoomEnabled() in WebRenderLayerManager.

https://reviewboard.mozilla.org/r/138114/#review142064

	Jeff Muizelaar [:jrmuizel]
	Comment 13 • 7 years ago
mozreview-review

Comment on attachment 8866486 [details]
Bug 1361497 - Push scrolling clips for scrollable layers in the layer tree.

https://reviewboard.mozilla.org/r/138116/#review142066

	Jeff Muizelaar [:jrmuizel]
	Comment 14 • 7 years ago
mozreview-review

Comment on attachment 8866487 [details]
Bug 1361497 - Refactoring to simplify getting the root CompositorBridgeParent and APZCTreeManager.

https://reviewboard.mozilla.org/r/138118/#review142068

	Jeff Muizelaar [:jrmuizel]
	Comment 15 • 7 years ago
mozreview-review

Comment on attachment 8866488 [details]
Bug 1361497 - Add a mechanism to push the async scroll data from APZ to WR.

https://reviewboard.mozilla.org/r/138120/#review142070

::: gfx/layers/apz/src/APZCTreeManager.cpp:376
(Diff revision 1)
> +{
> +  APZThreadUtils::AssertOnCompositorThread();
> +  MOZ_ASSERT(aWrApi);
> +
> +  MutexAutoLock lock(mTreeLock);
> +  mTreeLock.AssertCurrentThreadOwns();

Isn't this guaranteed to be tree because we take the lock above?

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 16 • 7 years ago
mozreview-review

Comment on attachment 8866488 [details]
Bug 1361497 - Add a mechanism to push the async scroll data from APZ to WR.

https://reviewboard.mozilla.org/r/138120/#review142074

::: gfx/layers/apz/src/APZCTreeManager.cpp:376
(Diff revision 1)
> +{
> +  APZThreadUtils::AssertOnCompositorThread();
> +  MOZ_ASSERT(aWrApi);
> +
> +  MutexAutoLock lock(mTreeLock);
> +  mTreeLock.AssertCurrentThreadOwns();

Good point, I'll remove it. I think it was copypasta.

	Botond Ballo [:botond]
	Comment 17 • 7 years ago
mozreview-review

Comment on attachment 8866488 [details]
Bug 1361497 - Add a mechanism to push the async scroll data from APZ to WR.

https://reviewboard.mozilla.org/r/138120/#review142136

::: gfx/layers/apz/src/APZCTreeManager.cpp:382
(Diff revision 1)
> +
> +  bool activeAnimations = false;
> +  uint64_t lastLayersId = -1;
> +  WrPipelineId lastPipelineId;
> +
> +  ForEachNode<ReverseIterator>(mRootNode.get(),

This loop is basically intended to be equivalent to ApplyAsyncContentTransformToTree and SamplaAPZAnimations in AsyncCompositionManager.cpp.

The former uses ForEachNode<ForwardIterator>, and the latter, ForEachNodePostOrder<ForwardIterator>. Is there a reason to use a different traversal order here?

::: gfx/layers/apz/src/APZCTreeManager.cpp:385
(Diff revision 1)
> +  WrPipelineId lastPipelineId;
> +
> +  ForEachNode<ReverseIterator>(mRootNode.get(),
> +      [&](HitTestingTreeNode* aNode)
> +      {
> +        if (!aNode->IsPrimaryHolder()) {

Is there a reason SampleAPZAnimations *doesn't* do this check, and we have workarounds like this [1] and this [2] instead?

[1] http://searchfox.org/mozilla-central/rev/cd8c561106d804e26bc09389f18f361846d005eb/gfx/layers/apz/src/AsyncPanZoomController.cpp#3059
[2] http://searchfox.org/mozilla-central/rev/cd8c561106d804e26bc09389f18f361846d005eb/gfx/layers/apz/src/AsyncPanZoomController.cpp#3273

::: gfx/layers/apz/src/APZCTreeManager.cpp:401
(Diff revision 1)
> +          MOZ_ASSERT(state && state->mWrBridge);
> +          lastPipelineId = state->mWrBridge->PipelineId();
> +        }
> +
> +        aWrApi->UpdateScrollPosition(lastPipelineId, apzc->GetGuid().mScrollId,
> +            wr::ToWrPoint(-apzc->GetCurrentAsyncTransform(AsyncPanZoomController::RESPECT_FORCE_DISABLE).mTranslation));

Why the negation? Might be worth a comment.

Also, line length. (I'm not a stickler for line length limits, but this one fails the "is still readable after zooming out sufficiently to get MozReview to not wrap the line" test).

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 18 • 7 years ago
mozreview-review-reply

Comment on attachment 8866488 [details]
Bug 1361497 - Add a mechanism to push the async scroll data from APZ to WR.

https://reviewboard.mozilla.org/r/138120/#review142136

> This loop is basically intended to be equivalent to ApplyAsyncContentTransformToTree and SamplaAPZAnimations in AsyncCompositionManager.cpp.
> 
> The former uses ForEachNode<ForwardIterator>, and the latter, ForEachNodePostOrder<ForwardIterator>. Is there a reason to use a different traversal order here?

I believe that it shouldn't really matter what order we do this in, since we should be processing each APZC exactly once, and there shouldn't be any particular dependencies. However, here we are traversing the HitTestingNodeTree which is intended to be iterated in reverse (it has a GetPrevSibling() but no GetNextSibling(), and GetFirstChild() is implemented with a linear loop). In AsyncCompositionManager we iterate the layer[metrics] tree, which is usually iterated forwards (by convention, I guess?). I suppose we could probably modify AsyncCompositionManager to iterate in reverse as well if we wanted to be consistent. I can add a comment explaining the choice of direction in this code though.

> Is there a reason SampleAPZAnimations *doesn't* do this check, and we have workarounds like this [1] and this [2] instead?
> 
> [1] http://searchfox.org/mozilla-central/rev/cd8c561106d804e26bc09389f18f361846d005eb/gfx/layers/apz/src/AsyncPanZoomController.cpp#3059
> [2] http://searchfox.org/mozilla-central/rev/cd8c561106d804e26bc09389f18f361846d005eb/gfx/layers/apz/src/AsyncPanZoomController.cpp#3273

It's mostly because of the way the mapping works between the layer tree, the APZC instances, and the HitTestingTreeNodes. Recall that there is a many-to-one mapping from the layers to the APZC instances, and a many-to-one mapping from the HTTNs to the APZCs. So for a given APZC we can't obtain "the" HTTN because there are actually many of them. And the HTTN is the one with the primary-holder flag. The existing SampleAPZAnimations code gets to the APZC via the layer tree and there's no primary-holder flag there so it can't deduplicate the APZCs easily. In this new code we *can* do this, so I took advantage of it.

> Why the negation? Might be worth a comment.
> 
> Also, line length. (I'm not a stickler for line length limits, but this one fails the "is still readable after zooming out sufficiently to get MozReview to not wrap the line" test).

Will add a comment and wrap this better.

	Botond Ballo [:botond]
	Comment 19 • 7 years ago

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #18)
> I believe that it shouldn't really matter what order we do this in, since we
> should be processing each APZC exactly once, and there shouldn't be any
> particular dependencies. However, here we are traversing the
> HitTestingNodeTree which is intended to be iterated in reverse (it has a
> GetPrevSibling() but no GetNextSibling(), and GetFirstChild() is implemented
> with a linear loop).

That's a good reason, thanks. Note that, while I agree that forward vs. reverse doesn't matter, if this function later ends up doing the equivalent of AlignFixedAndStickyLayers(), post-order vs. pre-order *will* become important. Might be worth adding a comment to that effect.

> > Is there a reason SampleAPZAnimations *doesn't* do this check
> 
> there's no primary-holder flag there

Doh! Of course :)

	Pulsebot
	Comment 20 • 7 years ago

Pushed by kgupta@mozilla.com:
https://hg.mozilla.org/projects/graphics/rev/e8be507f8d1e
Clean up the WR APIs for pushing scrolling clips vs non-scrolling clips. r=jrmuizel
https://hg.mozilla.org/projects/graphics/rev/c4eb64f48562
Override AsyncPanZoomEnabled() in WebRenderLayerManager. r=jrmuizel
https://hg.mozilla.org/projects/graphics/rev/cafebbb11ef1
Push scrolling clips for scrollable layers in the layer tree. r=jrmuizel
https://hg.mozilla.org/projects/graphics/rev/5dca8ba945a4
Refactoring to simplify getting the root CompositorBridgeParent and APZCTreeManager. r=jrmuizel
https://hg.mozilla.org/projects/graphics/rev/6c717edf5c5c
Add a mechanism to push the async scroll data from APZ to WR. r=jrmuizel,botond

	Pulsebot
	Comment 21 • 7 years ago

Pushed by kgupta@mozilla.com:
https://hg.mozilla.org/projects/graphics/rev/35576958aacb
Follow-up to fix unified build bustage on Windows. r=bustage

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 22 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/e8be507f8d1e
https://hg.mozilla.org/mozilla-central/rev/c4eb64f48562
https://hg.mozilla.org/mozilla-central/rev/cafebbb11ef1
https://hg.mozilla.org/mozilla-central/rev/5dca8ba945a4
https://hg.mozilla.org/mozilla-central/rev/6c717edf5c5c
https://hg.mozilla.org/mozilla-central/rev/35576958aacb