1151346 - ActiveLayerTracker::IsOffsetOrMarginStyleAnimated should respect CSS animations

Status: RESOLVED INVALID

(Core :: Layout, defect)

Description

[Markus Stange [:mstange]]

It's what we do for transform and opacity animations, and it makes it easier to write reliable tests.

Comment 1

[Markus Stange [:mstange]]

Attachment: patch

I didn't add will-change support because I didn't want to deal with budgeting.

Comment 2

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

Comment on attachment 8588476 [details] [diff] [review]
patch

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

::: layout/base/ActiveLayerTracker.cpp
@@ +320,5 @@
> +        nsLayoutUtils::HasCurrentAnimationsForProperty(content, eCSSProperty_margin_right) ||
> +        nsLayoutUtils::HasCurrentAnimationsForProperty(content, eCSSProperty_margin_bottom)) {
> +      return true;
> +    }
> +  }

I'm worried that this will make nsDisplayListBuilder::IsAnimatedGeometryRoot significantly slower when called on an element which has a transition or animation --- HasCurrentAnimationsForProperty will do repeated work in that case.

How about we extend nsLayoutUtils::HasCurrentAnimationsForProperty, AnimationPlayerCollection::HasCurrentAnimationsForProperty, and Animation::HasAnimationOfProperty to take a list of properties? Seems like this wouldn't add much complexity.

Comment 3

[Markus Stange [:mstange]]

Attachment: patch

How do you feel about this? I could also make it pointer + length, so that we can skip the array construction. It would be nice to have a type that wraps just that, and std::initializer_list would be such a type, but unfortunately it seems like we can't use that yet.
(It says yes on the "Using C++ in Mozilla code" page, but clang on OS X wants me to #include <initializer_list>, which it doesn't find unless we build with -std=c++11, but currently we build with -std=gnu++0x, I don't know why.)

Comment 4

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

Comment on attachment 8588495 [details] [diff] [review]
patch

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

::: layout/base/ActiveLayerTracker.cpp
@@ +321,5 @@
> +    properties.AppendElement(eCSSProperty_bottom);
> +    properties.AppendElement(eCSSProperty_margin_left);
> +    properties.AppendElement(eCSSProperty_margin_top);
> +    properties.AppendElement(eCSSProperty_margin_right);
> +    properties.AppendElement(eCSSProperty_margin_bottom);

Having to initialize the array this way is rather horrible. Does it optimize to efficient code? If yes, keep it, otherwise let's go with explicit pointer + length.

Comment 5

[Markus Stange [:mstange]]

The generated code was horrible: 8 invocations of EnsureCapacity. I'm going with pointer + length.

Comment 6

[Markus Stange [:mstange]]

Attachment: patch

Comment 7

[Markus Stange [:mstange]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/95b75fff1a03

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/95b75fff1a03

Comment 9

[Nathan Froyd [:froydnj]]

Comment on attachment 8588495 [details] [diff] [review]
patch

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

::: layout/base/ActiveLayerTracker.cpp
@@ +314,5 @@
> +  nsIContent* content = aFrame->GetContent();
> +  if (content) {
> +    // An std::initializer_list would be much better than an nsTArray here.
> +    nsAutoTArray<nsCSSProperty,8> properties;
> +    properties.AppendElement(eCSSProperty_left);

FWIW, you could do:

nsCSSProperty* p = properties.AppendElements(8);
*p++ = eCSSProperty_left;
*p++ = eCSSProperty_top;
...

That would at least only call EnsureCapacity() once.  But that's still more executable code than pointer+length, so...

Comment 10

[Markus Stange [:mstange]]

I'm going to back out the important parts of this again because of bug 1151889. I think that's the right thing to do anyway until bug 1009693 is fixed, because until that happens, the only animations that can benefit from layerization are step() animations that align with layer pixels.

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5e987206ec9c

Comment 12

[Markus Stange [:mstange]]

Backed out: https://hg.mozilla.org/mozilla-central/rev/5e987206ec9c

Comment 13

[Markus Stange [:mstange]]

We don't need this in WR.