Make the representation of clips in the Layers API more flexible

RESOLVED FIXED in Firefox 49

Status

()

RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: botond, Assigned: botond)

Tracking

(Blocks: 1 bug, {correctness})

Trunk
mozilla49
correctness
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox49 fixed)

Details

(Whiteboard: [gfx-noted])

Attachments

(13 attachments)

58 bytes, text/x-review-board-request
mstange
: review+
Details
58 bytes, text/x-review-board-request
mstange
: review+
Details
58 bytes, text/x-review-board-request
kats
: review+
Details
58 bytes, text/x-review-board-request
mstange
: review+
Details
58 bytes, text/x-review-board-request
mstange
: review+
Details
58 bytes, text/x-review-board-request
mstange
: review+
Details
58 bytes, text/x-review-board-request
mstange
: review+
Details
58 bytes, text/x-review-board-request
mstange
: review+
Details
58 bytes, text/x-review-board-request
mstange
: review+
Details
58 bytes, text/x-review-board-request
mstange
: review+
Details
58 bytes, text/x-review-board-request
mstange
: review+
Details
58 bytes, text/x-review-board-request
mstange
: review+
Details
8.88 KB, patch
kats
: review+
Details | Diff | Splinter Review
(Assignee)

Description

3 years ago
We currently represent scroll clips in the Layers API by each ScrollMetadata having an optional clip rect, called its scroll clip, which is considered to be scrolled by all scroll frames above it.

In addition, a layer can be marked as "fixed" with respect to a scroll frame (to implement position:fixed and background-attachment:fixed), and fixed layers have an additional flag, "is clip fixed", that governs whether the clips are also fixed.

This flag applies to all clips. As a result, we cannot represent layers which are fixed and which have some clips that are fixed and some clips that scroll. Such layers come up when e.g. position:fixed is combined with the "clip" property, as in bug 1214146.

To fix this, we propose changing the representation of clip rects in the Layers API.

We will store, on each Layer, a list of clip rects, and for each clip rect, an associated scroll id (which could be NULL_SCROLL_ID) which represents the bottommost scroll id that scrolls that clip. In additional, we will retain the optional "fixed with respect to scroll id" annotation. However, there will be no "is clip fixed" annotation; whether a clip is fixed will be determined for each clip by its associated scroll id.
(Assignee)

Updated

3 years ago
Keywords: correctness
Whiteboard: [gfx-noted]
(Assignee)

Comment 1

3 years ago
(In reply to Botond Ballo [:botond] from comment #0)
> We will store, on each Layer, a list of clip rects, and for each clip rect,
> an associated scroll id (which could be NULL_SCROLL_ID) which represents the
> bottommost scroll id that scrolls that clip. In additional, we will retain
> the optional "fixed with respect to scroll id" annotation. However, there
> will be no "is clip fixed" annotation; whether a clip is fixed will be
> determined for each clip by its associated scroll id.

It turns out this representation makes the compositor-side code more convoluted than it needs to be, so Markus and I agreed to change tack slightly: we'll keep optional scroll clips stored in ScrollMetadatas, with the (unchanged) interpretation that they apply on top of the corresponding scroll transform, but we'll add a (single) optional scroll clip to the Layer as well, to represent a clip that is scrolled by all scroll frames associated with the layer.

The "is clip fixed" annotation is still going away; if a Layer is fixed, its regular clip will always be fixed, and its scroll clip will always scroll. Layout can populate the two clips as needed to get the desired behaviour.
(Assignee)

Comment 2

3 years ago
Created attachment 8747307 [details]
MozReview Request: Bug 1267438 - Use IntersectMaybeRects() in Layer::GetCombinedClipRect(). r=mstange

Review commit: https://reviewboard.mozilla.org/r/49825/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/49825/
(Assignee)

Comment 3

3 years ago
Created attachment 8747308 [details]
MozReview Request: Bug 1267438 - Group ScrollMetadata's optional clip rect and mask layer index into a LayerClip structure. r=mstange

Review commit: https://reviewboard.mozilla.org/r/49827/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/49827/
(Assignee)

Comment 4

3 years ago
Created attachment 8747309 [details]
MozReview Request: Bug 1267438 - Do not propagate the scroll clip to APZC's copy of ScrollMetadata. r=kats

Review commit: https://reviewboard.mozilla.org/r/49829/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/49829/
(Assignee)

Comment 5

3 years ago
Created attachment 8747310 [details]
MozReview Request: Bug 1267438 - Give layers an optional scrolled clip that is scrolled by all scroll frames associated with the layer. r=mstange

Review commit: https://reviewboard.mozilla.org/r/49831/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/49831/
(Assignee)

Comment 6

3 years ago
Here are some WIP patches; not ready for review yet.

Rough outline of the remaining patches:

  - Use the additional scroll clip on the compositor side (during hit testing 
    tree construction and AsyncCompositionManager)

  - Populate the new scroll clip on the Layout side instead of having a
    regular clip with isClipFixed=false

  - Remove isClipFixed
(Assignee)

Updated

3 years ago
Blocks: 1269537
(Assignee)

Comment 7

3 years ago
After some discussion with Markus today, we decided that "scroll clip" is a misleading name for the clip we're adding. Accordingly, I'm going to rename the type LayerScrollClip to LayerClip, and the new clip we're adding (which is still of type LayerClip) to "scrolled clip".
(Assignee)

Updated

3 years ago
Summary: Make the representation of scroll clips in the Layers API more flexible → Make the representation of clips in the Layers API more flexible
(Assignee)

Comment 8

3 years ago
Created attachment 8748935 [details]
MozReview Request: Bug 1267438 - Use the layer's scrolled clip during compositor hit testing. r=mstange

Review commit: https://reviewboard.mozilla.org/r/50617/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/50617/
Attachment #8747308 - Attachment description: MozReview Request: Bug 1267438 - Group ScrollMetadata's optional clip rect and mask layer index into a LayerScrollClip structure. r=mstange → MozReview Request: Bug 1267438 - Group ScrollMetadata's optional clip rect and mask layer index into a LayerClip structure. r=mstange
Attachment #8747310 - Attachment description: MozReview Request: Bug 1267438 - Give layers an (optional) additional scroll clip that is scrolled by all scroll frames associated with the layer. r=mstange → MozReview Request: Bug 1267438 - Give layers an optional scrolled clip that is scrolled by all scroll frames associated with the layer. r=mstange
(Assignee)

Comment 9

3 years ago
Created attachment 8748936 [details]
MozReview Request: Bug 1267438 - Factor out a helper function to check if a layer is fixed or sticky. r=mstange

Review commit: https://reviewboard.mozilla.org/r/50619/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/50619/
(Assignee)

Comment 10

3 years ago
Created attachment 8748937 [details]
MozReview Request: Bug 1267438 - During AlignFixedAndStickyLayers, only un-adjust the fixed portion of a layer's clip rect. r=mstange

Review commit: https://reviewboard.mozilla.org/r/50621/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/50621/
(Assignee)

Comment 11

3 years ago
Created attachment 8748938 [details]
MozReview Request: Bug 1267438 - Use the scrolled clip in AsyncCompositionManager. r=mstange

Review commit: https://reviewboard.mozilla.org/r/50623/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/50623/
(Assignee)

Comment 12

3 years ago
Created attachment 8748939 [details]
MozReview Request: Bug 1267438 - For fixed backgrounds, use the scrolled clip rather than the isClipFixed=false annotation. r=mstange

Review commit: https://reviewboard.mozilla.org/r/50625/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/50625/
(Assignee)

Comment 13

3 years ago
Created attachment 8748940 [details]
MozReview Request: Bug 1267438 - Remove the (no longer used) isClipFixed=false annotation. r=mstange

Review commit: https://reviewboard.mozilla.org/r/50627/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/50627/
(Assignee)

Comment 14

3 years ago
Created attachment 8748941 [details]
MozReview Request: Bug 1267438 - Remove the redundant aTransformAffectsLayerClip argument to AlignFixedAndStickyLayers. r=mstange

Review commit: https://reviewboard.mozilla.org/r/50629/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/50629/
(Assignee)

Comment 15

3 years ago
Comment on attachment 8747307 [details]
MozReview Request: Bug 1267438 - Use IntersectMaybeRects() in Layer::GetCombinedClipRect(). r=mstange

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49825/diff/1-2/
(Assignee)

Comment 16

3 years ago
Comment on attachment 8747308 [details]
MozReview Request: Bug 1267438 - Group ScrollMetadata's optional clip rect and mask layer index into a LayerClip structure. r=mstange

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49827/diff/1-2/
(Assignee)

Comment 17

3 years ago
Comment on attachment 8747309 [details]
MozReview Request: Bug 1267438 - Do not propagate the scroll clip to APZC's copy of ScrollMetadata. r=kats

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49829/diff/1-2/
(Assignee)

Comment 18

3 years ago
Comment on attachment 8747310 [details]
MozReview Request: Bug 1267438 - Give layers an optional scrolled clip that is scrolled by all scroll frames associated with the layer. r=mstange

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49831/diff/1-2/
(Assignee)

Comment 19

3 years ago
The updated patch series implements the API change.

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9ee9661c8b74

Before posting the series for review, I'd like to see the Try push passing. In addition, the two patches marked "[WIP]" need some cleanup.
(Assignee)

Comment 20

3 years ago
Oops, I used Maybe::emplace() improperly in one place.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b4524431f971
(Assignee)

Comment 21

3 years ago
There are also some async-scrolling reftest failures that I need to investigate.
(Assignee)

Comment 22

3 years ago
(In reply to Botond Ballo [:botond] from comment #21)
> There are also some async-scrolling reftest failures that I need to investigate.

The problem is that there is a second codepath in Layout, added in bug 1216580, that propagates the clip on a fixed background item to a layer, which I wasn't aware of, which also needs to be changed to use the scrolled clip.
(Assignee)

Comment 23

3 years ago
Created attachment 8749898 [details]
MozReview Request: Bug 1267438 - Support adding ancestor mask layers from places other than SetupScrollingMetadata. r=mstange

Review commit: https://reviewboard.mozilla.org/r/51237/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/51237/
Attachment #8748937 - Attachment description: MozReview Request: [WIP] Bug 1267438 - During AlignFixedAndStickyLayers, only un-adjust the fixed portion of a layer's clip rect. r=mstange → MozReview Request: Bug 1267438 - During AlignFixedAndStickyLayers, only un-adjust the fixed portion of a layer's clip rect. r=mstange
Attachment #8748938 - Attachment description: MozReview Request: [WIP] Bug 1267438 - Use the scrolled clip in AsyncCompositionManager. r=mstange → MozReview Request: Bug 1267438 - Use the scrolled clip in AsyncCompositionManager. r=mstange
Attachment #8749898 - Flags: review?(mstange)
Attachment #8747310 - Flags: review?(mstange)
Attachment #8748935 - Flags: review?(mstange)
Attachment #8748936 - Flags: review?(mstange)
Attachment #8748937 - Flags: review?(mstange)
Attachment #8748938 - Flags: review?(mstange)
Attachment #8748939 - Flags: review?(mstange)
Attachment #8748940 - Flags: review?(mstange)
Attachment #8748941 - Flags: review?(mstange)
(Assignee)

Comment 24

3 years ago
Comment on attachment 8747310 [details]
MozReview Request: Bug 1267438 - Give layers an optional scrolled clip that is scrolled by all scroll frames associated with the layer. r=mstange

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49831/diff/2-3/
(Assignee)

Comment 25

3 years ago
Comment on attachment 8748935 [details]
MozReview Request: Bug 1267438 - Use the layer's scrolled clip during compositor hit testing. r=mstange

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50617/diff/1-2/
(Assignee)

Comment 26

3 years ago
Comment on attachment 8748936 [details]
MozReview Request: Bug 1267438 - Factor out a helper function to check if a layer is fixed or sticky. r=mstange

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50619/diff/1-2/
(Assignee)

Comment 27

3 years ago
Comment on attachment 8748937 [details]
MozReview Request: Bug 1267438 - During AlignFixedAndStickyLayers, only un-adjust the fixed portion of a layer's clip rect. r=mstange

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50621/diff/1-2/
(Assignee)

Comment 28

3 years ago
Comment on attachment 8748938 [details]
MozReview Request: Bug 1267438 - Use the scrolled clip in AsyncCompositionManager. r=mstange

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50623/diff/1-2/
(Assignee)

Comment 29

3 years ago
Comment on attachment 8748939 [details]
MozReview Request: Bug 1267438 - For fixed backgrounds, use the scrolled clip rather than the isClipFixed=false annotation. r=mstange

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50625/diff/1-2/
(Assignee)

Comment 30

3 years ago
Comment on attachment 8748940 [details]
MozReview Request: Bug 1267438 - Remove the (no longer used) isClipFixed=false annotation. r=mstange

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50627/diff/1-2/
(Assignee)

Comment 31

3 years ago
Comment on attachment 8748941 [details]
MozReview Request: Bug 1267438 - Remove the redundant aTransformAffectsLayerClip argument to AlignFixedAndStickyLayers. r=mstange

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50629/diff/1-2/
(Assignee)

Updated

3 years ago
Attachment #8747307 - Flags: review?(mstange)
Attachment #8747308 - Flags: review?(mstange)
Attachment #8747309 - Flags: review?(bugmail.mozilla)
(Assignee)

Comment 32

3 years ago
I updated the patches for fix the reftest failures, cleaned them up, and posted for review.

Some commentary:

Part 1
  Refactors Layer::GetCombinedClipRect() to use IntersectMaybeRects(), in
  preparation for eventually combining the scrolled clip there as well.

Part 2
  Groups the optional clip rect and mask layer index in ScrollMetadata
  into a LayerClip structure, in preparation for reusing this structure
  for the layer's scrolled clip. Note that the mask layer index is
  still optional inside the structure, because we can have a clip
  without a mask layer.

Part 3
  Stops propagating ScrollMetadata's scroll clip to APZC. This reflects
  the fact the different layers scrolled by the same scroll frame can
  have different scroll clips, but we have only one APZC per scroll frame,
  so it makes so sense for APZC to use per-layer properties from
  ScrollMetadata. (It already wasn't using the scroll clip.) In the future,
  we'd like to separate the per-layer properties out more clearly.

Part 4
  Introduces the scrolled clip to the Layers API, without using it yet on
  either side of the API.

Part 5
  Uses the scrolled clip during compositor hit testing. Note that as per
  bug 1269537, the treatment of clips during compositor hit testing is
  wrong; this patch ignores that problem for the time being, leaving a
  proper fix for bug 1269537.

Part 6
  Factors out a helper function for determining if a layer is fixed or
  sticky. When I wrote this, I expected to use it later in the patch
  series (to condition the use of ClipParts on the layer being fixed
  or sticky), but ended up not using it (and just using ClipParts
  unconditionally). I kept the refactoring because I think it's still
  nice.

Part 7
  Modifies ApplyAsyncContentTransformToTree to split the shadow clip
  into two parts, a fixed part and a scrolled part, and have
  AlignFixedAndStickyLayers only adjust the fixed part. Technically,
  the old behaviour of adjusting both parts was wrong, but since 
  bug 1231538, layer structures that triggers the wrongness never 
  occur (and before bug 12315387, rendering of such layer structures
  was wrong anyways), so this is just a refactoring. However, not
  adjusting the scrolled part of the clip becomes important once we
  start including the layer's scrolled clip in it.

  I realize that I'm overloading the term "scrolled clip" to mean
  both the layer property, and one of the clip parts; if you have a
  suggestion for another name for the clip part, let me know.

Part 8
  Uses the layer's scrolled clip as part of the scrolled part of
  the shadow clip in AsyncCompositionManager. The corresponding
  mask layer, if present, is handled as well.

  One thing I wasn't sure about is whether the Render phase of
  composition requires any changes for the new mask layer. I don't
  think so, as it's part of mAncestorMaskLayers, and those are
  looped over in the Render phase, but I could be wrong.

Part 9
  Modifies the Layout side of the Layers API to allow adding
  ancestor mask layers from places other than SetupScrollMetadata,
  in anticipation of adding the mask layer for the scrolled clip.

Part 10
  Modifies Layout to represent the clip for a fixed background layer
  as a scrolled clip rather than using a regular clip and setting
  the isClipFixed=false annotation.

Part 11
  Removes the isClipFixed=false annotation, which is no longer used,
  from both layout and compositor code.

Part 12
  Simplifies AlignFixedAndStickyLayers a bit by removing the
  aTransformAffectsLayerClip parameter.

  The rationale here is that, now that isClipFixed is removed, the
  only time this would be false is if we don't have a clip rect
  yet at this point, in which case there's nothing to adjust
  anyways.
Attachment #8747309 - Flags: review?(bugmail.mozilla) → review+
Comment on attachment 8747309 [details]
MozReview Request: Bug 1267438 - Do not propagate the scroll clip to APZC's copy of ScrollMetadata. r=kats

https://reviewboard.mozilla.org/r/49829/#review48093
Comment on attachment 8747307 [details]
MozReview Request: Bug 1267438 - Use IntersectMaybeRects() in Layer::GetCombinedClipRect(). r=mstange

https://reviewboard.mozilla.org/r/49825/#review48181
Attachment #8747307 - Flags: review?(mstange) → review+
Comment on attachment 8747308 [details]
MozReview Request: Bug 1267438 - Group ScrollMetadata's optional clip rect and mask layer index into a LayerClip structure. r=mstange

https://reviewboard.mozilla.org/r/49827/#review48185
Attachment #8747308 - Flags: review?(mstange) → review+
Comment on attachment 8747310 [details]
MozReview Request: Bug 1267438 - Give layers an optional scrolled clip that is scrolled by all scroll frames associated with the layer. r=mstange

https://reviewboard.mozilla.org/r/49831/#review48195
Attachment #8747310 - Flags: review?(mstange) → review+
Comment on attachment 8748935 [details]
MozReview Request: Bug 1267438 - Use the layer's scrolled clip during compositor hit testing. r=mstange

https://reviewboard.mozilla.org/r/50617/#review48197
Attachment #8748935 - Flags: review?(mstange) → review+
Attachment #8748936 - Flags: review?(mstange) → review+
Comment on attachment 8748936 [details]
MozReview Request: Bug 1267438 - Factor out a helper function to check if a layer is fixed or sticky. r=mstange

https://reviewboard.mozilla.org/r/50619/#review48199
Comment on attachment 8748937 [details]
MozReview Request: Bug 1267438 - During AlignFixedAndStickyLayers, only un-adjust the fixed portion of a layer's clip rect. r=mstange

https://reviewboard.mozilla.org/r/50621/#review48223

Looks good. I'd prefer the life cycle of the clip parts cache to be a little more explicit, though. E.g. you could have it as a local variable in TransformShadowTree and it as an argument to the functions that need it.
Otherwise casual readers of AsyncCompositionManager.h might get alarmed about there being raw Layer* pointers in the cache's keys.
Attachment #8748937 - Flags: review?(mstange) → review+
Comment on attachment 8748938 [details]
MozReview Request: Bug 1267438 - Use the scrolled clip in AsyncCompositionManager. r=mstange

https://reviewboard.mozilla.org/r/50623/#review48227
Attachment #8748938 - Flags: review?(mstange) → review+
Attachment #8749898 - Flags: review?(mstange) → review+
Comment on attachment 8749898 [details]
MozReview Request: Bug 1267438 - Support adding ancestor mask layers from places other than SetupScrollingMetadata. r=mstange

https://reviewboard.mozilla.org/r/51237/#review48229

::: layout/base/FrameLayerBuilder.cpp:5103
(Diff revision 1)
>        } else {
>          NS_ASSERTION(oldLayer->GetType() == Layer::TYPE_CONTAINER,
>                       "Wrong layer type");
>          containerLayer = static_cast<ContainerLayer*>(oldLayer);
>          containerLayer->SetMaskLayer(nullptr);
> +        containerLayer->SetAncestorMaskLayers({});

Cool!
Comment on attachment 8748939 [details]
MozReview Request: Bug 1267438 - For fixed backgrounds, use the scrolled clip rather than the isClipFixed=false annotation. r=mstange

https://reviewboard.mozilla.org/r/50625/#review48239

::: layout/base/FrameLayerBuilder.cpp:4048
(Diff revision 2)
> +        if (shouldFixToViewport) {
> +          ownLayer->SetScrolledClip(Nothing());
> +        } else {
> +          ownLayer->SetClipRect(Nothing());
> +        }

I'm not a fan of this. Can we, before we go into the if (layerClip.HasClip()), just set both to Nothing()? We really only want at most one of the two to be non-Nothing. The code you have here would break that invariant if it ever reused an existing ownLayer with a different shouldFixToViewport value. I don't think that can happen, but let's do the safe thing anyway.
Attachment #8748939 - Flags: review?(mstange) → review+
Comment on attachment 8748940 [details]
MozReview Request: Bug 1267438 - Remove the (no longer used) isClipFixed=false annotation. r=mstange

https://reviewboard.mozilla.org/r/50627/#review48243
Attachment #8748940 - Flags: review?(mstange) → review+
Comment on attachment 8748941 [details]
MozReview Request: Bug 1267438 - Remove the redundant aTransformAffectsLayerClip argument to AlignFixedAndStickyLayers. r=mstange

https://reviewboard.mozilla.org/r/50629/#review48245
Attachment #8748941 - Flags: review?(mstange) → review+
(Assignee)

Comment 47

3 years ago
Thanks for the reviews! Addressed review comments, rebased, and landed.
Android R2 has been permafailing since this landed:
https://treeherder.mozilla.org/logviewer.html#?job_id=27546122&repo=mozilla-inbound
Flags: needinfo?(botond)
(Assignee)

Comment 49

3 years ago
(In reply to Ryan VanderMeulen [:RyanVM] from comment #48)
> Android R2 has been permafailing since this landed:
> https://treeherder.mozilla.org/logviewer.html#?job_id=27546122&repo=mozilla-
> inbound

That's definitely caused by my patches. Sorry about that!

I have a theory about what's causing the failure, testing it now.
(Assignee)

Comment 50

3 years ago
(In reply to Botond Ballo [:botond] from comment #49)
> I have a theory about what's causing the failure, testing it now.

I pushed an attempted fix to Try [1], but some Android reftests are still failing. Let's just back this out so the tree can reopen.

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=87b69e5c7727
Flags: needinfo?(botond)
(Assignee)

Comment 52

3 years ago
I'm pretty sure the remaining failures are related to the special-casing of UsesContainerScrolling() (which is only true on Android) here: https://dxr.mozilla.org/mozilla-central/rev/043082cb7bd8490c60815f67fbd1f33323ad7663/gfx/layers/composite/AsyncCompositionManager.cpp#902
(Assignee)

Comment 53

3 years ago
(In reply to Botond Ballo [:botond] from comment #52)
> I'm pretty sure the remaining failures are related to the special-casing of
> UsesContainerScrolling() (which is only true on Android) here:
> https://dxr.mozilla.org/mozilla-central/rev/
> 043082cb7bd8490c60815f67fbd1f33323ad7663/gfx/layers/composite/
> AsyncCompositionManager.cpp#902

I tried removing that special-casing, but not only did that not fix the remaining reftest failures, it broke other things [1], so red herring, I guess.

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=b97e86f1b159
(Assignee)

Comment 54

3 years ago
All right, I debugged this and have things passing locally.

There were two problems:

  - The code in ApplyAsyncContentTransformToTree that detected whether
    the clip has changed, in the sense of a shadow clip distinct from
    the layer clip needing to be set, failed to take into consideration
    the presence of a scrolled clip. (This was the fix I pushed in
    comment 50.)

  - While addressing Markus' review comment about having the 
    ClipPartsCache live in TransformShadowTree() and passing it around
    rather than having it be a field of AsyncCompositionManager, I made 
    the ClipPartsCache argument to AlignFixedAndStickyLayers optional
    (because TransformScrollableLayer() also calls it), defaulting it
    to nullptr, but then forgot to pass in a non-nullptr argument at
    the ApplyAsyncContentTransformToTree call site. Silly me!

Try push for updated patches:

  https://treeherder.mozilla.org/#/jobs?repo=try&revision=cde6eea6a00d
(Assignee)

Comment 56

3 years ago
(In reply to Botond Ballo [:botond] from comment #54)
> Try push for updated patches:
> 
>   https://treeherder.mozilla.org/#/jobs?repo=try&revision=cde6eea6a00d

^ Looking good, relanded.
(Assignee)

Comment 57

3 years ago
Created attachment 8751514 [details] [diff] [review]
Some minor follow-up cleanup

This is some minor cleanup to AsyncCompositionManager, applying on top of bug 1227231, which I intend to land as a follow-up after this and bug 1227231 land:

  - Removes an outdated comment

  - Moves the declaration of the ClipPartsCache object from TransformShadwoTree()
    to ApplyAsyncContentTransformToTree(), since - as of bug 1227231 - the latter
    is no longer recursive.

  - Makes better use of Maybe, assigning directly to the object inside when we
    know there is one rather than assigning to the Maybe.
Attachment #8751514 - Flags: review?(bugmail.mozilla)
Comment on attachment 8751514 [details] [diff] [review]
Some minor follow-up cleanup

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

Seems straightforward enough.
Attachment #8751514 - Flags: review?(bugmail.mozilla) → review+
Depends on: 1272525
(Assignee)

Comment 61

3 years ago
(In reply to Botond Ballo [:botond] from comment #57)
> This is some minor cleanup to AsyncCompositionManager, applying on top of
> bug 1227231, which I intend to land as a follow-up after this and bug
> 1227231 land

Group-landed with bug 1270902.
Blocks: 1278656
No longer blocks: 1278656
Depends on: 1285619
You need to log in before you can comment on or make changes to this bug.