Drop "only composite opacity animations when already in compositing mode".

Drop "only composite opacity animations when already in compositing mode".

This CL fixes a bug caused by incomplete memory optimization made in r162118.
r162118 logic causes a complicated bug. When there is webkit-transform animation as
well as opacity animation, and it is not in compositing mode, it is possible
for Blink to handle opacity animation incorrectly, because Blink expects opacity
animation is accelerated. It can be fixed, but code would be more complicated,
so this CL drops this incomplete memory optimization.

BUG=324685
TEST=transitions/opacity-transform-transitions-inside-iframe.html

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=163136

[dshwang, 2013-12-03 11:03:11]

This CL fixes BUG=324685.
transitions/opacity-transform-transitions-inside-iframe.html is minimal test
cast of BUG=324685

This bug is related to combination of iframe, opacity-animation,
transform-animation and background-color layer.
opacity-transform-transitions-inside-iframe.html stand for the combination.

CL's content is reverting "incomplete opacity animation memory optimization":
https://codereview.chromium.org/63813004
Web Animation CSS also followed this optimization, so I changed the part of
https://codereview.chromium.org/23874019

[dshwang, 2013-12-03 11:08:58]

https://codereview.chromium.org/101623002/diff/1/LayoutTests/transitions/reso...
File
LayoutTests/transitions/resources/opacity-transform-transitions-inside-iframe-inner.html
(right):

https://codereview.chromium.org/101623002/diff/1/LayoutTests/transitions/reso...
LayoutTests/transitions/resources/opacity-transform-transitions-inside-iframe-inner.html:59:
}
Let me explain this test in detail.

- opacity-transform-transitions-inside-iframe.html is in force compositing mode,
while opacity-transform-transitions-inside-iframe-inner.html is not in force
compositing mode, because iframe tag does not have overflow property.
- solid_color_overlay has opacity animation, but when attaching
solid_color_overlay to the dom, opacity animation does not start through
composited animation because we are not in compositing mode.
- popup has transform animation. When attaching popup to the dom, popup's
animation start through composited animation.
- composited animation think solid_color_overlay's opacity animation is run by
compositor.


To fix it, all code related to opacity animation must deal with all edge cases.
However, it will make code more complicated. If it is needed, I think it is
better to do it in the future after all animation refactoring is done.

[ajuma, 2013-12-03 14:19:25]

Note that while r162118 was needed to address a regression on Mac OS 10.6
(caused by r161636, which made us to enter compositing mode for opacity
animations; see crbug.com/317747), force-compositing-mode has since been turned
on by default there (see crbug.com/266186). So  this CL is likely safe to land
now.

https://codereview.chromium.org/101623002/diff/1/LayoutTests/transitions/reso...
File
LayoutTests/transitions/resources/opacity-transform-transitions-inside-iframe-inner.html
(right):

https://codereview.chromium.org/101623002/diff/1/LayoutTests/transitions/reso...
LayoutTests/transitions/resources/opacity-transform-transitions-inside-iframe-inner.html:55:
window.setTimeout(function() {
Can we use window.internals.forceCompositingUpdate instead of setTimeout?

[dshwang, 2013-12-03 14:41:50]

On 2013/12/03 14:19:25, ajuma wrote:
> Note that while r162118 was needed to address a regression on Mac OS 10.6
> (caused by r161636, which made us to enter compositing mode for opacity
> animations; see crbug.com/317747), force-compositing-mode has since been
turned
> on by default there (see crbug.com/266186). So  this CL is likely safe to land
> now.

It's good to know. Thank you for explanation.


>
https://codereview.chromium.org/101623002/diff/1/LayoutTests/transitions/reso...
> File
>
LayoutTests/transitions/resources/opacity-transform-transitions-inside-iframe-inner.html
> (right):
> 
>
https://codereview.chromium.org/101623002/diff/1/LayoutTests/transitions/reso...
>
LayoutTests/transitions/resources/opacity-transform-transitions-inside-iframe-inner.html:55:
> window.setTimeout(function() {
> Can we use window.internals.forceCompositingUpdate instead of setTimeout?

Yeah, you are right. forceCompositingUpdate works well. I updated.

[ajuma, 2013-12-03 16:30:37]

https://codereview.chromium.org/101623002/diff/40001/LayoutTests/transitions/...
File
LayoutTests/transitions/resources/opacity-transform-transitions-inside-iframe-inner.html
(right):

https://codereview.chromium.org/101623002/diff/40001/LayoutTests/transitions/...
LayoutTests/transitions/resources/opacity-transform-transitions-inside-iframe-inner.html:59:
}, 0.5);
Can we do without this setTimeout? (I'm trying to understand where the 0.5 is
coming from here.)

[dshwang, 2013-12-03 16:50:21]

https://codereview.chromium.org/101623002/diff/40001/LayoutTests/transitions/...
File
LayoutTests/transitions/resources/opacity-transform-transitions-inside-iframe-inner.html
(right):

https://codereview.chromium.org/101623002/diff/40001/LayoutTests/transitions/...
LayoutTests/transitions/resources/opacity-transform-transitions-inside-iframe-inner.html:59:
}, 0.5);
On 2013/12/03 16:30:37, ajuma wrote:
> Can we do without this setTimeout? (I'm trying to understand where the 0.5 is
> coming from here.)

Thank you. setTimeout is not needed in here as well as
transitions/opacity-transition-zindex.html
I updated.

I misunderstood what pauseAnimations() is.
I thought pauseAnimations() means to start and then stop at the given time.
I know now pauseAnimations() means to seek at the given time.

[ajuma, 2013-12-03 16:53:18]

Thanks, non-OWNER lgtm.

[dshwang, 2013-12-03 17:10:21]

On 2013/12/03 16:53:18, ajuma wrote:
> Thanks, non-OWNER lgtm.

Thank you for good review. This CL is improved :)

[dstockwell, 2013-12-03 18:49:30]

https://codereview.chromium.org/101623002/diff/60001/LayoutTests/transitions/...
File LayoutTests/transitions/opacity-transition-zindex.html (right):

https://codereview.chromium.org/101623002/diff/60001/LayoutTests/transitions/...
LayoutTests/transitions/opacity-transition-zindex.html:50: if (window.internals
&& window.testRunner) {
Please format this as:

if (window.internals) {
  internals...
}
if (window.testRunner) {
  testRunner...
}

This allows the test to function correctly under --expose-internals-for-testing
where internals is exposed but testRunner is not.

https://codereview.chromium.org/101623002/diff/60001/Source/core/animation/Ac...
File Source/core/animation/ActiveAnimations.cpp (right):

https://codereview.chromium.org/101623002/diff/60001/Source/core/animation/Ac...
Source/core/animation/ActiveAnimations.cpp:48: // FIXME: remove compositing mode
check once compositing is forced on all platforms
remove this FIXME

[dshwang, 2013-12-03 19:17:30]

On 2013/12/03 18:49:30, dstockwell wrote:
>
https://codereview.chromium.org/101623002/diff/60001/LayoutTests/transitions/...
> File LayoutTests/transitions/opacity-transition-zindex.html (right):
> 
>
https://codereview.chromium.org/101623002/diff/60001/LayoutTests/transitions/...
> LayoutTests/transitions/opacity-transition-zindex.html:50: if
(window.internals
> && window.testRunner) {
> Please format this as:
> 
> if (window.internals) {
>   internals...
> }
> if (window.testRunner) {
>   testRunner...
> }
> 
> This allows the test to function correctly under
--expose-internals-for-testing
> where internals is exposed but testRunner is not.

Thank you for review. It's good to know. Done.


>
https://codereview.chromium.org/101623002/diff/60001/Source/core/animation/Ac...
> File Source/core/animation/ActiveAnimations.cpp (right):
> 
>
https://codereview.chromium.org/101623002/diff/60001/Source/core/animation/Ac...
> Source/core/animation/ActiveAnimations.cpp:48: // FIXME: remove compositing
mode
> check once compositing is forced on all platforms
> remove this FIXME

I missed it. Done.

[dstockwell, 2013-12-04 00:57:32]

lgtm

[commit-bot: I haz the power, 2013-12-04 00:58:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dongseong.hwang@intel.com/101623002/80001

[commit-bot: I haz the power, 2013-12-04 01:57:30]

Change committed as 163136

[dshwang, 2013-12-04 06:35:45]

Thank you for review!