Open Bug 1151346 Opened 5 years ago Updated 1 year ago

ActiveLayerTracker::IsOffsetOrMarginStyleAnimated should respect CSS animations

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

REOPENED
mozilla40
Tracking Status
firefox40 --- fixed

People

(Reporter: mstange, Assigned: mstange)

References

Details

Attachments

(1 file, 2 obsolete files)

It's what we do for transform and opacity animations, and it makes it easier to write reliable tests.
Attached patch patch (obsolete) — Splinter Review
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)
Attached patch patch (obsolete) — Splinter Review
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+
The generated code was horrible: 8 invocations of EnsureCapacity. I'm going with pointer + length.
Attached patch patchSplinter Review
Attachment #8588495 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/95b75fff1a03
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
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...
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.
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.