ActiveLayerTracker::IsOffsetOrMarginStyleAnimated should respect CSS animations

REOPENED
Assigned to

Status

()

Core
Layout
REOPENED
3 years ago
3 years ago

People

(Reporter: mstange, Assigned: mstange)

Tracking

Trunk
mozilla40
Points:
---

Firefox Tracking Flags

(firefox40 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

3 years ago
It's what we do for transform and opacity animations, and it makes it easier to write reliable tests.
(Assignee)

Comment 1

3 years ago
Created attachment 8588476 [details] [diff] [review]
patch

I didn't add will-change support because I didn't want to deal with budgeting.
Attachment #8588476 - Flags: review?(roc)
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.
Attachment #8588476 - Flags: review?(roc)
(Assignee)

Comment 3

3 years ago
Created attachment 8588495 [details] [diff] [review]
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.)
Attachment #8588476 - Attachment is obsolete: true
Attachment #8588495 - Flags: review?(roc)
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.
Attachment #8588495 - Flags: review?(roc) → review+
(Assignee)

Comment 5

3 years ago
The generated code was horrible: 8 invocations of EnsureCapacity. I'm going with pointer + length.
(Assignee)

Comment 6

3 years ago
Created attachment 8588497 [details] [diff] [review]
patch
Attachment #8588495 - Attachment is obsolete: true
(Assignee)

Comment 7

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/95b75fff1a03
https://hg.mozilla.org/mozilla-central/rev/95b75fff1a03
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox40: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Blocks: 1151889
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...
(Assignee)

Comment 10

3 years ago
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

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/5e987206ec9c
(Assignee)

Comment 12

3 years ago
Backed out: https://hg.mozilla.org/mozilla-central/rev/5e987206ec9c
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
You need to log in before you can comment on or make changes to this bug.