813722 - Create a layer for opacity:0 display items if opacity is animated

Status: RESOLVED FIXED

(Core :: Layout, defect)

Comment 1

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix

Comment 2

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 683730 [details] [diff] [review]
fix

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

::: layout/base/nsDisplayList.cpp
@@ +2687,5 @@
>  nsDisplayOpacity::BuildLayer(nsDisplayListBuilder* aBuilder,
>                               LayerManager* aManager,
>                               const ContainerParameters& aContainerParameters) {
> +  if (mFrame->GetStyleDisplay()->mOpacity == 0 && mFrame->GetContent() &&
> +      !nsLayoutUtils::HasAnimationsForCompositor(mFrame->GetContent(), eCSSProperty_opacity)) {

(!mFrame->GetContent() || !nsLayout.....)

I think not having a nsIContent* means that we definitely don't have animations, so we can skip building the layer.

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

We can never have nsDisplayOpacity on the viewport frame (null mContent), so no point in optimizing for it. I'm only checking for sanity's sake, and the simple thing to do is to not use this optimization if mContent happens to be null

Comment 4

[Florian Bender]

Does this patch handle Animations as well as Transitions? I don't know if Firefox handles them separately internally. 

Plus, this bug might have to be uplifted to the branch wherever Bug 811831 landed (looks like that one made it into Gecko 19 while this one seems to land in Gecko 20).

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Florian Bender from comment #4)
> Does this patch handle Animations as well as Transitions? I don't know if
> Firefox handles them separately internally.

The function I'm calling does handle both, yes.

> Plus, this bug might have to be uplifted to the branch wherever Bug 811831
> landed (looks like that one made it into Gecko 19 while this one seems to
> land in Gecko 20).

You are correct.

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6152b934d1c7

Comment 7

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/6152b934d1c7

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 683730 [details] [diff] [review]
fix

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 811831
User impact if declined: Could possibly have a situation where off-main-thread animations of opacity start jerkily in FF19
Testing completed (on m-c, etc.): just landed on m-c
Risk to taking this patch (and alternatives if risky): Very low-risk patch. We just disable an optimization in an another case
String or UUID changes made by this patch: None

Comment 9

[Alex Keybl [:akeybl]]

Not tracking since this doesn't have a demonstrated user impact at this point, but approving for Aurora given where we are in the cycle.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/9e255aaa583c

Comment 11

[Mihai Morar, (:MihaiMorar)]

Is there any way QA can verify this fix?