Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1119763003:
Animations: Port LinkHighlight to use compositor timelines. (Closed)
|
Created:
5 years, 7 months ago by loyso (OOO) Modified:
5 years, 5 months ago CC:
blink-reviews, jbroman Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Target Ref:
refs/heads/master Project:
blink Visibility:
Public. |
DescriptionAnimations: Port LinkHighlight to use compositor timelines.
Use AnimationPlayer to add animations if new system enabled.
Previous episode: https://codereview.chromium.org/1012583002/
Next episode:
https://codereview.chromium.org/1133513002/
BUG=394777
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=199201
Patch Set 1 #Patch Set 2 : Rebase. #
Total comments: 18
Patch Set 3 : Fix code review issues (add ASSERTs) #
Messages
Total messages: 19 (7 generated)
loyso@chromium.org changed reviewers: + aelias@chromium.org
loyso@chromium.org changed reviewers: + ajuma@chromium.org, vollick@chromium.org - aelias@chromium.org
loyso@chromium.org changed reviewers: + aelias@chromium.org
PTAL! Would be nice to land this part.
loyso@chromium.org changed reviewers: + chrishtr@chromium.org, jbroman@chromium.org - ajuma@chromium.org, shane@chromium.org, vollick@chromium.org
jbroman@chromium.org changed reviewers: - jbroman@chromium.org
Moving myself to cc -- I don't know enough about the animation code to be a useful reviewer.
https://codereview.chromium.org/1119763003/diff/20001/Source/web/LinkHighligh... File Source/web/LinkHighlight.cpp (right): https://codereview.chromium.org/1119763003/diff/20001/Source/web/LinkHighligh... Source/web/LinkHighlight.cpp:84: if (RuntimeEnabledFeatures::compositorAnimationTimelinesEnabled() && Platform::current()->compositorSupport()) { When would compositorSupport() be null? https://codereview.chromium.org/1119763003/diff/20001/Source/web/LinkHighligh... Source/web/LinkHighlight.cpp:308: OwnPtr<WebCompositorAnimation> animation = adoptPtr(compositorSupport->createAnimation(*curve, WebCompositorAnimation::TargetPropertyOpacity)); Why even put it in an OwnPtr if you are going to leak it two lines later? https://codereview.chromium.org/1119763003/diff/20001/Source/web/WebViewImpl.h File Source/web/WebViewImpl.h (right): https://codereview.chromium.org/1119763003/diff/20001/Source/web/WebViewImpl.... Source/web/WebViewImpl.h:743: OwnPtr<WebCompositorAnimationTimeline> m_linkHighlightsTimeline; Why do you need a timeline specific to link highlights?
https://codereview.chromium.org/1119763003/diff/20001/Source/web/LinkHighligh... File Source/web/LinkHighlight.cpp (right): https://codereview.chromium.org/1119763003/diff/20001/Source/web/LinkHighligh... Source/web/LinkHighlight.cpp:84: if (RuntimeEnabledFeatures::compositorAnimationTimelinesEnabled() && Platform::current()->compositorSupport()) { On 2015/07/13 21:20:52, chrishtr wrote: > When would compositorSupport() be null? We can assert that it can't be null here. I'll fix it here and all related CLs. https://codereview.chromium.org/1119763003/diff/20001/Source/web/LinkHighligh... Source/web/LinkHighlight.cpp:308: OwnPtr<WebCompositorAnimation> animation = adoptPtr(compositorSupport->createAnimation(*curve, WebCompositorAnimation::TargetPropertyOpacity)); On 2015/07/13 21:20:52, chrishtr wrote: > Why even put it in an OwnPtr if you are going to leak it two lines later? 1) This is how we mark ownership over this animation (and that we pass the ownership here) 2) That's how blink interact with cc (cc_blink) in existing code base (search for all the calls https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...). We don't use OwnPtr in WebKit/public for some reason. Would be nice to unify it on blink/chromium repositories merge. Any ideas? https://codereview.chromium.org/1119763003/diff/20001/Source/web/WebViewImpl.h File Source/web/WebViewImpl.h (right): https://codereview.chromium.org/1119763003/diff/20001/Source/web/WebViewImpl.... Source/web/WebViewImpl.h:743: OwnPtr<WebCompositorAnimationTimeline> m_linkHighlightsTimeline; On 2015/07/13 21:20:53, chrishtr wrote: > Why do you need a timeline specific to link highlights? Timeline is a group of players, basically. Here we isolate highlight animations in a special group. (Note that we will rename cc::AnimationPlayer to Animation and cc::Animation to cc::KeyframeEffect, like here https://codereview.chromium.org/1113173003/) In general, highlights use animation system in superficial way. And we want animation system to be unified with blink to get rid of redundancies.
https://codereview.chromium.org/1119763003/diff/20001/Source/web/LinkHighligh... File Source/web/LinkHighlight.cpp (right): https://codereview.chromium.org/1119763003/diff/20001/Source/web/LinkHighligh... Source/web/LinkHighlight.cpp:84: if (RuntimeEnabledFeatures::compositorAnimationTimelinesEnabled() && Platform::current()->compositorSupport()) { On 2015/07/14 at 00:59:04, loyso wrote: > On 2015/07/13 21:20:52, chrishtr wrote: > > When would compositorSupport() be null? > We can assert that it can't be null here. I'll fix it here and all related CLs. Also change the interface to return a reference rather than a pointer then? Is that feasible? https://codereview.chromium.org/1119763003/diff/20001/Source/web/LinkHighligh... Source/web/LinkHighlight.cpp:308: OwnPtr<WebCompositorAnimation> animation = adoptPtr(compositorSupport->createAnimation(*curve, WebCompositorAnimation::TargetPropertyOpacity)); On 2015/07/14 at 00:59:04, loyso wrote: > On 2015/07/13 21:20:52, chrishtr wrote: > > Why even put it in an OwnPtr if you are going to leak it two lines later? > 1) This is how we mark ownership over this animation (and that we pass the ownership here) > 2) That's how blink interact with cc (cc_blink) in existing code base (search for all the calls https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...). > We don't use OwnPtr in WebKit/public for some reason. > > Would be nice to unify it on blink/chromium repositories merge. Any ideas? I agree that unifying after the blink/chromium merge would be nice. For now this code is ok. https://codereview.chromium.org/1119763003/diff/20001/Source/web/WebViewImpl.h File Source/web/WebViewImpl.h (right): https://codereview.chromium.org/1119763003/diff/20001/Source/web/WebViewImpl.... Source/web/WebViewImpl.h:743: OwnPtr<WebCompositorAnimationTimeline> m_linkHighlightsTimeline; On 2015/07/14 at 00:59:04, loyso wrote: > On 2015/07/13 21:20:53, chrishtr wrote: > > Why do you need a timeline specific to link highlights? > Timeline is a group of players, basically. Here we isolate highlight animations in a special group. (Note that we will rename cc::AnimationPlayer to Animation and cc::Animation to cc::KeyframeEffect, like here https://codereview.chromium.org/1113173003/) > > In general, highlights use animation system in superficial way. And we want animation system to be unified with blink to get rid of redundancies. Is it actually possible to animate a link highlight? LinkHighlight is an unfortunate source of one-off complexity that we should reduce, not increase, which is why we should try to avoid the complexities in this CL if possible.
https://codereview.chromium.org/1119763003/diff/20001/Source/web/LinkHighligh... File Source/web/LinkHighlight.cpp (right): https://codereview.chromium.org/1119763003/diff/20001/Source/web/LinkHighligh... Source/web/LinkHighlight.cpp:84: if (RuntimeEnabledFeatures::compositorAnimationTimelinesEnabled() && Platform::current()->compositorSupport()) { On 2015/07/14 18:34:00, chrishtr wrote: > On 2015/07/14 at 00:59:04, loyso wrote: > > On 2015/07/13 21:20:52, chrishtr wrote: > > > When would compositorSupport() be null? > > We can assert that it can't be null here. I'll fix it here and all related > CLs. > Also change the interface to return a reference rather than a pointer then? Is > that feasible? We have so many calls to this function throughout the project. Many Mocks/Tests intentionally don't override this virtual function so nullptr is desirable behavior for them. In ideal world, I would go further an would like to get rid of that Platform singleton. I think, this is outside of the scope of this CL. We could set a task in our bug-tracker. https://codereview.chromium.org/1119763003/diff/20001/Source/web/WebViewImpl.h File Source/web/WebViewImpl.h (right): https://codereview.chromium.org/1119763003/diff/20001/Source/web/WebViewImpl.... Source/web/WebViewImpl.h:743: OwnPtr<WebCompositorAnimationTimeline> m_linkHighlightsTimeline; On 2015/07/14 18:34:00, chrishtr wrote: > On 2015/07/14 at 00:59:04, loyso wrote: > > On 2015/07/13 21:20:53, chrishtr wrote: > > > Why do you need a timeline specific to link highlights? > > Timeline is a group of players, basically. Here we isolate highlight > animations in a special group. (Note that we will rename cc::AnimationPlayer to > Animation and cc::Animation to cc::KeyframeEffect, like here > https://codereview.chromium.org/1113173003/) > > > > In general, highlights use animation system in superficial way. And we want > animation system to be unified with blink to get rid of redundancies. > Is it actually possible to animate a link highlight? Yes, that's an opacity animation if you touch a hyper link on android (What's funny is that it didn't support --disable-threaded-animation mode). So, I don't understand the question (given that you started the discussion here: https://groups.google.com/a/chromium.org/d/msg/paint-dev/b6hEgxo6AbY/gH8snoYW... ) > LinkHighlight is an unfortunate source of one-off complexity that we should > reduce, not increase, which is why we should try to avoid the complexities in > this CL if possible. 1) We will delete the old layer-intrusive animation system eventually. All the conditional branches will be gone. 2) Adding animations directly to layers was a source of highly-coupled code in chrome compositor. So we needed some external entity to add animations. It's an AnimationPlayer now. 3) CC animation system is "the least common denominator" for all sub-systems. And we need to keep LinkHighlights in a working shape. 4) I had the idea to have hidden timeline inside the AnimationHost to handle all the "orphaned" AnimationPlayers. But this would introduce an unstructured soup (no grouping == no isolation), which would be hard to debug. So I decided to keep the group explicit. Moreover, probably we will move some properties (?) from individual animations into AnimationTimeline.
https://codereview.chromium.org/1119763003/diff/20001/Source/web/LinkHighligh... File Source/web/LinkHighlight.cpp (right): https://codereview.chromium.org/1119763003/diff/20001/Source/web/LinkHighligh... Source/web/LinkHighlight.cpp:84: if (RuntimeEnabledFeatures::compositorAnimationTimelinesEnabled() && Platform::current()->compositorSupport()) { On 2015/07/14 at 00:59:04, loyso wrote: > On 2015/07/13 21:20:52, chrishtr wrote: > > When would compositorSupport() be null? > We can assert that it can't be null here. I'll fix it here and all related CLs. Ok please add the assert at least then. https://codereview.chromium.org/1119763003/diff/20001/Source/web/WebViewImpl.cpp File Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/1119763003/diff/20001/Source/web/WebViewImpl.... Source/web/WebViewImpl.cpp:4193: if (RuntimeEnabledFeatures::compositorAnimationTimelinesEnabled() && Platform::current()->compositorSupport() && m_layerTreeView) { Add assert for compositorSupport() https://codereview.chromium.org/1119763003/diff/20001/Source/web/WebViewImpl.h File Source/web/WebViewImpl.h (right): https://codereview.chromium.org/1119763003/diff/20001/Source/web/WebViewImpl.... Source/web/WebViewImpl.h:743: OwnPtr<WebCompositorAnimationTimeline> m_linkHighlightsTimeline; On 2015/07/15 at 02:04:36, loyso wrote: > On 2015/07/14 18:34:00, chrishtr wrote: > > On 2015/07/14 at 00:59:04, loyso wrote: > > > On 2015/07/13 21:20:53, chrishtr wrote: > > > > Why do you need a timeline specific to link highlights? > > > Timeline is a group of players, basically. Here we isolate highlight > > animations in a special group. (Note that we will rename cc::AnimationPlayer to > > Animation and cc::Animation to cc::KeyframeEffect, like here > > https://codereview.chromium.org/1113173003/) > > > > > > In general, highlights use animation system in superficial way. And we want > > animation system to be unified with blink to get rid of redundancies. > > Is it actually possible to animate a link highlight? > Yes, that's an opacity animation if you touch a hyper link on android (What's funny is that it didn't support --disable-threaded-animation mode). > So, I don't understand the question (given that you started the discussion here: https://groups.google.com/a/chromium.org/d/msg/paint-dev/b6hEgxo6AbY/gH8snoYW... ) > > > LinkHighlight is an unfortunate source of one-off complexity that we should > > reduce, not increase, which is why we should try to avoid the complexities in > > this CL if possible. > 1) We will delete the old layer-intrusive animation system eventually. All the conditional branches will be gone. > 2) Adding animations directly to layers was a source of highly-coupled code in chrome compositor. So we needed some external entity to add animations. It's an AnimationPlayer now. > 3) CC animation system is "the least common denominator" for all sub-systems. And we need to keep LinkHighlights in a working shape. > 4) I had the idea to have hidden timeline inside the AnimationHost to handle all the "orphaned" AnimationPlayers. But this would introduce an unstructured soup (no grouping == no isolation), which would be hard to debug. So I decided to keep the group explicit. Moreover, probably we will move some properties (?) from individual animations into AnimationTimeline. It looks like each Document has a timeline already. Can you just re-use the timeline for the containing document for link highlight animations?
On 2015/07/15 14:25:15, chrishtr wrote: > > 4) I had the idea to have hidden timeline inside the AnimationHost to handle > all the "orphaned" AnimationPlayers. But this would introduce an unstructured > soup (no grouping == no isolation), which would be hard to debug. So I decided > to keep the group explicit. Moreover, probably we will move some properties (?) > from individual animations into AnimationTimeline. > > It looks like each Document has a timeline already. Can you just re-use the > timeline for the containing document for link highlight animations? The Document's AnimationTimeline contains WebCompositorAnimationTimeline where every WebCompositorAnimationPlayer supposed to have correspondent Animation object (ex-AnimationPlayer). If we add LinkHighlight's WebCompositorAnimationPlayer to the Document's WebCompositorAnimationTimeline then we need extra 1st-class blink::Animation object. In this CL LinkHighlight operates at WebCompositorAnimationTimeline/WebCompositorAnimationPlayer only, without exposing them for the blink core/Animation layer.
lgtm
https://codereview.chromium.org/1119763003/diff/20001/Source/web/LinkHighligh... File Source/web/LinkHighlight.cpp (right): https://codereview.chromium.org/1119763003/diff/20001/Source/web/LinkHighligh... Source/web/LinkHighlight.cpp:84: if (RuntimeEnabledFeatures::compositorAnimationTimelinesEnabled() && Platform::current()->compositorSupport()) { On 2015/07/15 14:25:14, chrishtr wrote: > On 2015/07/14 at 00:59:04, loyso wrote: > > On 2015/07/13 21:20:52, chrishtr wrote: > > > When would compositorSupport() be null? > > We can assert that it can't be null here. I'll fix it here and all related > CLs. > > Ok please add the assert at least then. Done. https://codereview.chromium.org/1119763003/diff/20001/Source/web/LinkHighligh... Source/web/LinkHighlight.cpp:308: OwnPtr<WebCompositorAnimation> animation = adoptPtr(compositorSupport->createAnimation(*curve, WebCompositorAnimation::TargetPropertyOpacity)); On 2015/07/14 18:34:00, chrishtr wrote: > On 2015/07/14 at 00:59:04, loyso wrote: > > On 2015/07/13 21:20:52, chrishtr wrote: > > > Why even put it in an OwnPtr if you are going to leak it two lines later? > > 1) This is how we mark ownership over this animation (and that we pass the > ownership here) > > 2) That's how blink interact with cc (cc_blink) in existing code base (search > for all the calls > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...). > > We don't use OwnPtr in WebKit/public for some reason. > > > > Would be nice to unify it on blink/chromium repositories merge. Any ideas? > > I agree that unifying after the blink/chromium merge would be nice. For now this > code is ok. Acknowledged. https://codereview.chromium.org/1119763003/diff/20001/Source/web/WebViewImpl.cpp File Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/1119763003/diff/20001/Source/web/WebViewImpl.... Source/web/WebViewImpl.cpp:4193: if (RuntimeEnabledFeatures::compositorAnimationTimelinesEnabled() && Platform::current()->compositorSupport() && m_layerTreeView) { On 2015/07/15 14:25:14, chrishtr wrote: > Add assert for compositorSupport() Done. https://codereview.chromium.org/1119763003/diff/20001/Source/web/WebViewImpl.h File Source/web/WebViewImpl.h (right): https://codereview.chromium.org/1119763003/diff/20001/Source/web/WebViewImpl.... Source/web/WebViewImpl.h:743: OwnPtr<WebCompositorAnimationTimeline> m_linkHighlightsTimeline; On 2015/07/15 14:25:14, chrishtr wrote: > On 2015/07/15 at 02:04:36, loyso wrote: > > On 2015/07/14 18:34:00, chrishtr wrote: > > > On 2015/07/14 at 00:59:04, loyso wrote: > > > > On 2015/07/13 21:20:53, chrishtr wrote: > > > > > Why do you need a timeline specific to link highlights? > > > > Timeline is a group of players, basically. Here we isolate highlight > > > animations in a special group. (Note that we will rename cc::AnimationPlayer > to > > > Animation and cc::Animation to cc::KeyframeEffect, like here > > > https://codereview.chromium.org/1113173003/) > > > > > > > > In general, highlights use animation system in superficial way. And we > want > > > animation system to be unified with blink to get rid of redundancies. > > > Is it actually possible to animate a link highlight? > > Yes, that's an opacity animation if you touch a hyper link on android (What's > funny is that it didn't support --disable-threaded-animation mode). > > So, I don't understand the question (given that you started the discussion > here: > https://groups.google.com/a/chromium.org/d/msg/paint-dev/b6hEgxo6AbY/gH8snoYW... > ) > > > > > LinkHighlight is an unfortunate source of one-off complexity that we should > > > reduce, not increase, which is why we should try to avoid the complexities > in > > > this CL if possible. > > 1) We will delete the old layer-intrusive animation system eventually. All the > conditional branches will be gone. > > 2) Adding animations directly to layers was a source of highly-coupled code in > chrome compositor. So we needed some external entity to add animations. It's an > AnimationPlayer now. > > 3) CC animation system is "the least common denominator" for all sub-systems. > And we need to keep LinkHighlights in a working shape. > > 4) I had the idea to have hidden timeline inside the AnimationHost to handle > all the "orphaned" AnimationPlayers. But this would introduce an unstructured > soup (no grouping == no isolation), which would be hard to debug. So I decided > to keep the group explicit. Moreover, probably we will move some properties (?) > from individual animations into AnimationTimeline. > > It looks like each Document has a timeline already. Can you just re-use the > timeline for the containing document for link highlight animations? Acknowledged.
The CQ bit was checked by loyso@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from chrishtr@chromium.org Link to the patchset: https://codereview.chromium.org/1119763003/#ps40001 (title: "Fix code review issues (add ASSERTs)")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1119763003/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://src.chromium.org/viewvc/blink?view=rev&revision=199201 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
