1267438 - Make the representation of clips in the Layers API more flexible

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Botond Ballo [:botond]]

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.

Comment 1

[Botond Ballo [:botond]]

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

Comment 2

[Botond Ballo [:botond]]

Attachment: 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/

Comment 3

[Botond Ballo [:botond]]

Attachment: 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/

Comment 4

[Botond Ballo [:botond]]

Attachment: 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/

Comment 5

[Botond Ballo [:botond]]

Attachment: 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/

Comment 6

[Botond Ballo [:botond]]

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

Comment 7

[Botond Ballo [:botond]]

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

Comment 8

[Botond Ballo [:botond]]

Attachment: 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/

Comment 9

[Botond Ballo [:botond]]

Attachment: 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/

Comment 10

[Botond Ballo [:botond]]

Attachment: 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/

Comment 11

[Botond Ballo [:botond]]

Attachment: 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/

Comment 12

[Botond Ballo [:botond]]

Attachment: 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/

Comment 13

[Botond Ballo [:botond]]

Attachment: 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/

Comment 14

[Botond Ballo [:botond]]

Attachment: 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/

Comment 15

[Botond Ballo [:botond]]

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/

Comment 16

[Botond Ballo [:botond]]

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/

Comment 17

[Botond Ballo [:botond]]

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/

Comment 18

[Botond Ballo [:botond]]

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/

Comment 19

[Botond Ballo [:botond]]

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.

Comment 20

[Botond Ballo [:botond]]

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

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

Comment 21

[Botond Ballo [:botond]]

There are also some async-scrolling reftest failures that I need to investigate.

Comment 22

[Botond Ballo [:botond]]

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

Comment 23

[Botond Ballo [:botond]]

Attachment: 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/

Comment 24

[Botond Ballo [:botond]]

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/

Comment 25

[Botond Ballo [:botond]]

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/

Comment 26

[Botond Ballo [:botond]]

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/

Comment 27

[Botond Ballo [:botond]]

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/

Comment 28

[Botond Ballo [:botond]]

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/

Comment 29

[Botond Ballo [:botond]]

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/

Comment 30

[Botond Ballo [:botond]]

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/

Comment 31

[Botond Ballo [:botond]]

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/

Comment 32

[Botond Ballo [:botond]]

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.

Comment 33

[Botond Ballo [:botond]]

Try pushes:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1026455f7f5c
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f4931e652d5a

(There are two because TaskCluster broke T-pushes (bug 1247343)).

Comment 34

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

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 35

[Markus Stange [:mstange]]

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

https://reviewboard.mozilla.org/r/49825/#review48181

Comment 36

[Markus Stange [:mstange]]

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

Comment 37

[Markus Stange [:mstange]]

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

Comment 38

[Markus Stange [:mstange]]

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

Comment 39

[Markus Stange [:mstange]]

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 40

[Markus Stange [:mstange]]

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.

Comment 41

[Markus Stange [:mstange]]

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

https://reviewboard.mozilla.org/r/50623/#review48227

Comment 42

[Markus Stange [:mstange]]

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 43

[Markus Stange [:mstange]]

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.

Comment 44

[Markus Stange [:mstange]]

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

Comment 45

[Markus Stange [:mstange]]

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

Comment 46

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1065434be718
https://hg.mozilla.org/integration/mozilla-inbound/rev/b802e4ef2099
https://hg.mozilla.org/integration/mozilla-inbound/rev/5bf9a7962ad4
https://hg.mozilla.org/integration/mozilla-inbound/rev/54cb97cee681
https://hg.mozilla.org/integration/mozilla-inbound/rev/6618e805fce8
https://hg.mozilla.org/integration/mozilla-inbound/rev/68b48bf17f7b
https://hg.mozilla.org/integration/mozilla-inbound/rev/14ee768229fa
https://hg.mozilla.org/integration/mozilla-inbound/rev/641eef893097
https://hg.mozilla.org/integration/mozilla-inbound/rev/1c8e4840f8b4
https://hg.mozilla.org/integration/mozilla-inbound/rev/4aa07089ce57
https://hg.mozilla.org/integration/mozilla-inbound/rev/2bf1247f5743
https://hg.mozilla.org/integration/mozilla-inbound/rev/63c9e4fbc1be

Comment 47

[Botond Ballo [:botond]]

Thanks for the reviews! Addressed review comments, rebased, and landed.

Comment 48

[Ryan VanderMeulen [:RyanVM]]

Android R2 has been permafailing since this landed:
https://treeherder.mozilla.org/logviewer.html#?job_id=27546122&repo=mozilla-inbound

Comment 49

[Botond Ballo [:botond]]

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

Comment 50

[Botond Ballo [:botond]]

(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

Comment 51

[Botond Ballo [:botond]]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/39d2501ceed3

Comment 52

[Botond Ballo [:botond]]

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

Comment 53

[Botond Ballo [:botond]]

(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

Comment 54

[Botond Ballo [:botond]]

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

Comment 55

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/aff26040d9f9
https://hg.mozilla.org/integration/mozilla-inbound/rev/4e627bf7ba98
https://hg.mozilla.org/integration/mozilla-inbound/rev/3e9e4731148b
https://hg.mozilla.org/integration/mozilla-inbound/rev/cc5e63dc19c5
https://hg.mozilla.org/integration/mozilla-inbound/rev/83ed4951ebab
https://hg.mozilla.org/integration/mozilla-inbound/rev/8713dd0cf44e
https://hg.mozilla.org/integration/mozilla-inbound/rev/cce3a0906b07
https://hg.mozilla.org/integration/mozilla-inbound/rev/33d67d476ec6
https://hg.mozilla.org/integration/mozilla-inbound/rev/2b70efbc6c3a
https://hg.mozilla.org/integration/mozilla-inbound/rev/3b75e87bc3c9
https://hg.mozilla.org/integration/mozilla-inbound/rev/2ccaac8616e4
https://hg.mozilla.org/integration/mozilla-inbound/rev/ad59be79e6ed

Comment 56

[Botond Ballo [:botond]]

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

Comment 57

[Botond Ballo [:botond]]

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

Comment 58

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

Comment on attachment 8751514 [details] [diff] [review]
Some minor follow-up cleanup

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

Seems straightforward enough.

Comment 59

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/aff26040d9f9
https://hg.mozilla.org/mozilla-central/rev/4e627bf7ba98
https://hg.mozilla.org/mozilla-central/rev/3e9e4731148b
https://hg.mozilla.org/mozilla-central/rev/cc5e63dc19c5
https://hg.mozilla.org/mozilla-central/rev/83ed4951ebab
https://hg.mozilla.org/mozilla-central/rev/8713dd0cf44e
https://hg.mozilla.org/mozilla-central/rev/cce3a0906b07
https://hg.mozilla.org/mozilla-central/rev/33d67d476ec6
https://hg.mozilla.org/mozilla-central/rev/2b70efbc6c3a
https://hg.mozilla.org/mozilla-central/rev/3b75e87bc3c9
https://hg.mozilla.org/mozilla-central/rev/2ccaac8616e4
https://hg.mozilla.org/mozilla-central/rev/ad59be79e6ed

Comment 60

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1d4e393524e3

Comment 61

[Botond Ballo [:botond]]

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

Comment 62

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/1d4e393524e3