CC AnimationHost: Fix LinkHighlights detach scope.

CC AnimationHost: Fix LinkHighlights detach scope.

In a new world with external cc::AnimationHost
blink::WebCompositorAnimationPlayer objects may outsurvive the compositor
(LayerTreeView and AnimationHost).
In this case, they become detached.
(See RenderWidget::Close())

See the related fix for ProgrammaticScrollAnimator here:
https://codereview.chromium.org/1420913008

BUG=541069
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/429e65a60baa76cbeefa4451483e44dcf7cbfcf7
Cr-Commit-Position: refs/heads/master@{#357755}

[loyso (OOO), 2015-11-03 04:17:52]

loyso@chromium.org changed reviewers:
+ ajuma@chromium.org, chrishtr@chromium.org, pdr@chromium.org,
piman@chromium.org

[piman, 2015-11-03 04:24:24]

LGTM for content/

[loyso (OOO), 2015-11-03 04:30:39]

Description was changed from

==========
CC AnimationHost: Fix LinkHighlights detach scope.

In a new world with external cc::AnimationHost
blink::WebCompositorAnimationPlayer objects may outsurvive the compositor
(LayerTreeView).
In this case, they become detached.

BUG=541069
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
CC AnimationHost: Fix LinkHighlights detach scope.

In a new world with external cc::AnimationHost
blink::WebCompositorAnimationPlayer objects may outsurvive the compositor
(LayerTreeView).
In this case, they become detached.

See the related fix for ProgrammaticScrollAnimator here:
https://codereview.chromium.org/1420913008

BUG=541069
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[loyso (OOO), 2015-11-03 04:43:17]

Description was changed from

==========
CC AnimationHost: Fix LinkHighlights detach scope.

In a new world with external cc::AnimationHost
blink::WebCompositorAnimationPlayer objects may outsurvive the compositor
(LayerTreeView).
In this case, they become detached.

See the related fix for ProgrammaticScrollAnimator here:
https://codereview.chromium.org/1420913008

BUG=541069
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
CC AnimationHost: Fix LinkHighlights detach scope.

In a new world with external cc::AnimationHost
blink::WebCompositorAnimationPlayer objects may outsurvive the compositor
(LayerTreeView).
In this case, they become detached.
(See RenderWidget::Close())

See the related fix for ProgrammaticScrollAnimator here:
https://codereview.chromium.org/1420913008

BUG=541069
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[loyso (OOO), 2015-11-03 05:08:53]

https://codereview.chromium.org/1413373007/diff/20001/content/test/web_layer_...
File content/test/web_layer_tree_view_impl_for_testing.cc (right):

https://codereview.chromium.org/1413373007/diff/20001/content/test/web_layer_...
content/test/web_layer_tree_view_impl_for_testing.cc:50:
use_compositor_animation_timelines);
Alternatively, we can land it with 'settings.use_compositor_animation_timelines
= false' constant
and switch to a new system globally in this upcoming CL:
https://codereview.chromium.org/1308053006/

It would leave all function signatures unchanged.

[loyso (OOO), 2015-11-03 05:36:20]

Description was changed from

==========
CC AnimationHost: Fix LinkHighlights detach scope.

In a new world with external cc::AnimationHost
blink::WebCompositorAnimationPlayer objects may outsurvive the compositor
(LayerTreeView).
In this case, they become detached.
(See RenderWidget::Close())

See the related fix for ProgrammaticScrollAnimator here:
https://codereview.chromium.org/1420913008

BUG=541069
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
CC AnimationHost: Fix LinkHighlights detach scope.

In a new world with external cc::AnimationHost
blink::WebCompositorAnimationPlayer objects may outsurvive the compositor
(LayerTreeView and AnimationHost).
In this case, they become detached.
(See RenderWidget::Close())

See the related fix for ProgrammaticScrollAnimator here:
https://codereview.chromium.org/1420913008

BUG=541069
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[chrishtr, 2015-11-03 05:43:29]

https://codereview.chromium.org/1413373007/diff/20001/content/test/web_layer_...
File content/test/web_layer_tree_view_impl_for_testing.cc (right):

https://codereview.chromium.org/1413373007/diff/20001/content/test/web_layer_...
content/test/web_layer_tree_view_impl_for_testing.cc:50:
use_compositor_animation_timelines);
On 2015/11/03 at 05:08:53, loyso wrote:
> Alternatively, we can land it with
'settings.use_compositor_animation_timelines = false' constant
> and switch to a new system globally in this upcoming CL:
> https://codereview.chromium.org/1308053006/
> 
> It would leave all function signatures unchanged.

I think that would be better, since you'll want to remove the parameter later
anyway.

[ajuma, 2015-11-03 15:34:04]

cc/ lgtm

[loyso (OOO), 2015-11-04 00:43:18]

On 2015/11/03 05:43:29, chrishtr wrote:
> > Alternatively, we can land it with
> 'settings.use_compositor_animation_timelines = false' constant
> > and switch to a new system globally in this upcoming CL:
> > https://codereview.chromium.org/1308053006/
> > 
> > It would leave all function signatures unchanged.
> 
> I think that would be better, since you'll want to remove the parameter later
> anyway.
Done. PTAL!

[piman, 2015-11-04 00:54:21]

lgtm

[pdr., 2015-11-04 02:53:02]

https://codereview.chromium.org/1413373007/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/1413373007/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebViewImpl.cpp:2684: m_linkHighlights.clear();
Can you help me understand this line? Why do we only clear m_linkHighlights if
there's a highlight timeline?

[loyso (OOO), 2015-11-04 04:16:31]

https://codereview.chromium.org/1413373007/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/1413373007/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebViewImpl.cpp:2684: m_linkHighlights.clear();
On 2015/11/04 02:53:02, pdr wrote:
> Can you help me understand this line? Why do we only clear m_linkHighlights if
> there's a highlight timeline?
Ermmm. We can clear m_linkHighlights in any case, definitely.

Currently two compositor animation systems co-exist in cc.
m_linkHighlightsTimeline presence indicates that we are running in "external
AnimatioHost mode (enable-compositor-animationtimelines).

So i just wanted to keep those behaviors isolated.

[pdr., 2015-11-04 04:22:26]

The CQ bit was checked by pdr@chromium.org

[pdr., 2015-11-04 04:22:26]

lgtm

[pdr., 2015-11-04 04:22:26]

The patchset sent to the CQ was uploaded after l-g-t-m from ajuma@chromium.org
Link to the patchset: https://codereview.chromium.org/1413373007/#ps60001
(title: "GC.")

[commit-bot: I haz the power, 2015-11-04 04:22:45]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1413373007/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1413373007/60001

[commit-bot: I haz the power, 2015-11-04 04:28:02]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-11-04 04:28:49]

Patchset 4 (id:??) landed as
https://crrev.com/429e65a60baa76cbeefa4451483e44dcf7cbfcf7
Cr-Commit-Position: refs/heads/master@{#357755}