Morris Tseng [:mtseng] [:Morris] (Inactive) Assignee
	Description • 7 years ago

STR:

1. Turn on gfx.webrender.layers-free
2. Open attachment

Expected result:
CSS animation should start when page loaded.

Actual result:
CSS animation didn't start.

	Morris Tseng [:mtseng] [:Morris] (Inactive) Assignee
	Comment 1 • 7 years ago

Something didn't trigger in layers free mode because in layers free mode we early return in [1]. Therefore, we didn't trigger pending animation in [2]. I think we should run through to the end of function instead of early return. I'll write a patch for this.

[1]: https://dxr.mozilla.org/mozilla-central/source/layout/painting/nsDisplayList.cpp#2115
[2]: https://dxr.mozilla.org/mozilla-central/source/layout/painting/nsDisplayList.cpp#2317

	Ethan Lin[:ethlin]
	Comment 3 • 7 years ago
mozreview-review

Comment on attachment 8889763 [details]
Bug 1384000 - Call TriggerPendingAnimations in layers-free mode.

https://reviewboard.mozilla.org/r/160844/#review166136

Currently I know for layers-free, we should add ClearInvalidationStateBits back to make invalidation correct and add TriggerPendingAnimations back to make css animation work. Removing the early return seems to make the code more complicated but this way could reduce some duplicate code. I'm fine with both ways.

::: layout/painting/nsDisplayList.cpp:2313
(Diff revision 1)
>    if (document && widgetTransaction) {
>      TriggerPendingAnimations(document, layerManager->GetAnimationReadyTime());
>    }
>  
>    nsIntRegion invalid;
>    if (props) {

I suppose layers-free mode doesn't need these computation. Could we move these invalidation funciton calls to the line after 'layerBuilder->DidEndTransaction()'? And then we don't need to check if the root is valid.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 4 • 7 years ago
mozreview-review

Comment on attachment 8889763 [details]
Bug 1384000 - Call TriggerPendingAnimations in layers-free mode.

https://reviewboard.mozilla.org/r/160844/#review166240

I'm not comfortable with this change because it looks like it reorders things in the non-WR codepath and I don't know what effects that will have. Your patch pulls the BeginTransaction calls earlier compared to when it was happening previously - now it happens before the FrameLayerBuilder is created and is outside of the AutoProfilerTracing tracing(...) RAII block. I don't know if that's acceptable so to be conservative I'm giving this a r-. I think for now it's safer to use the old code pattern and just add the necessary call to TriggerPendingAnimations in the right spot. If it turns out we are duplicating a lot of stuff to make things work we can revisit this. If there is a lot of duplication then doing it this way should be simpler than it is now, because fewer things will be conditioned on |isLayersFree|.

	Ethan Lin[:ethlin]
	Comment 5 • 7 years ago

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #4)
> Comment on attachment 8889763 [details]
> Bug 1384000 - Don't early return in layers free mode.
> 
> https://reviewboard.mozilla.org/r/160844/#review166240
> 
> I'm not comfortable with this change because it looks like it reorders
> things in the non-WR codepath and I don't know what effects that will have.
> Your patch pulls the BeginTransaction calls earlier compared to when it was
> happening previously - now it happens before the FrameLayerBuilder is
> created and is outside of the AutoProfilerTracing tracing(...) RAII block. I
> don't know if that's acceptable so to be conservative I'm giving this a r-.
> I think for now it's safer to use the old code pattern and just add the
> necessary call to TriggerPendingAnimations in the right spot. If it turns
> out we are duplicating a lot of stuff to make things work we can revisit
> this. If there is a lot of duplication then doing it this way should be
> simpler than it is now, because fewer things will be conditioned on
> |isLayersFree|.

Okay, so we just add what we really need for layers-free mode. I will create another bug for adding ClearInvalidationStateBits to fix invalidation problem.

	Morris Tseng [:mtseng] [:Morris] (Inactive) Assignee
	Comment 6 • 7 years ago

Sure, I'll attach another patch.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 8 • 7 years ago
mozreview-review

Comment on attachment 8889763 [details]
Bug 1384000 - Call TriggerPendingAnimations in layers-free mode.

https://reviewboard.mozilla.org/r/160844/#review166734

Thanks!

	Morris Tseng [:mtseng] [:Morris] (Inactive) Assignee
	Comment 9 • 7 years ago

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

	Pulsebot
	Comment 10 • 7 years ago

Pushed by mtseng@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d4d95cd1fdff
Call TriggerPendingAnimations in layers-free mode. r=ethlin,kats

	Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)
	Comment 11 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/d4d95cd1fdff