Blink Compositor Animation: Introduce CompositorAnimationHost correctly.

Blink Compositor Animation: Introduce CompositorAnimationHost correctly.

So we get it from WebLayerTreeView (RenderWidgetCompositor), not from
timeline.

BUG=610763
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Committed: https://crrev.com/ffcf86ca01510297c45f67c09de79859d541d98f
Cr-Commit-Position: refs/heads/master@{#439423}

[loyso (OOO), 2016-12-09 04:02:19]

Description was changed from

==========
Blink Compositor Animation: Introduce CompositorAnimationHost correctly.

So we get it from WebLayerTreeView (RenderWidgetCompositor), not from
timeline.

BUG=610763
==========

to

==========
Blink Compositor Animation: Introduce CompositorAnimationHost correctly.

So we get it from WebLayerTreeView (RenderWidgetCompositor), not from
timeline.

BUG=610763
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[loyso (OOO), 2016-12-09 04:02:45]

The CQ bit was checked by loyso@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-09 04:03:07]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-09 04:05:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-09 04:05:10]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[loyso (OOO), 2016-12-09 04:06:49]

loyso@chromium.org changed reviewers:
+ ajuma@chromium.org

[loyso (OOO), 2016-12-09 05:03:47]

The CQ bit was checked by loyso@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-09 05:04:02]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-09 06:20:58]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-09 06:20:59]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[ajuma, 2016-12-09 15:08:23]

lgtm

[loyso (OOO), 2016-12-12 00:08:54]

The CQ bit was checked by loyso@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-12 00:09:05]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[loyso (OOO), 2016-12-12 00:19:59]

loyso@chromium.org changed reviewers:
+ aelias@chromium.org, jbroman@chromium.org

[loyso (OOO), 2016-12-12 00:22:06]

PTAL!
aelias:
  content/renderer/
  Source/web/
  public/platform/WebLayerTreeView.h
jbroman:
  Source/core/
  Source/platform/
Thanks!

[commit-bot: I haz the power, 2016-12-12 02:15:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-12 02:15:10]

Dry run: This issue passed the CQ dry run.

[aelias_OOO_until_Jul13, 2016-12-12 23:27:58]

lgtm

[loyso (OOO), 2016-12-13 03:22:49]

jbroman@, PTAL!

[jbroman, 2016-12-13 16:38:32]

lgtm with a couple optional nits

https://codereview.chromium.org/2559273003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp (right):

https://codereview.chromium.org/2559273003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:1818: if
(LocalFrame* frame = box().frame()) {
OK because it already exists below, but ordinarily I'd prefer an early-return
style to progressively deeper nesting of if statements and ternary operators.

LocalFrame* frame = box().frame();
if (!frame)
  return nullptr;

Page* page = frame->page();
if (!page)
  return nullptr;

ScrollingCoordinator* coordinator = page->scrollingCoordinator();
if (!coordinator)
  return nullptr;

return coordinator->compositorAnimationHost();

https://codereview.chromium.org/2559273003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/animation/CompositorAnimationHost.h
(right):

https://codereview.chromium.org/2559273003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/animation/CompositorAnimationHost.h:28: void
addTimeline(CompositorAnimationTimeline*);
These look like they could take "const CompositorAnimationTimeline&". Any reason
to take non-const pointers?

https://codereview.chromium.org/2559273003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/GraphicsLayerTest.cpp (right):

https://codereview.chromium.org/2559273003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/GraphicsLayerTest.cpp:122: auto host
= WTF::makeUnique<CompositorAnimationHost>(
nit: Why heap-allocate? This should work too:

CompositorAnimationHost host(layerTreeView()->compositorAnimationHost());
host->addTimeline(&host);

https://codereview.chromium.org/2559273003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/WebFrameWidgetImpl.h (right):

https://codereview.chromium.org/2559273003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebFrameWidgetImpl.h:223:
std::unique_ptr<CompositorAnimationHost> m_compositorAnimationHost;
Up to you, but if having a compositor animation host is the usual case then I'd
lean towards using WTF::Optional<> (or making CompositorAnimationHost support
nullability itself, but I gather we're leaning away from that). This is
approximately a pointer-to-a-pointer, which is a little much.

[loyso (OOO), 2016-12-13 22:23:35]

https://codereview.chromium.org/2559273003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/animation/CompositorAnimationHost.h
(right):

https://codereview.chromium.org/2559273003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/animation/CompositorAnimationHost.h:28: void
addTimeline(CompositorAnimationTimeline*);
On 2016/12/13 at 16:38:32, jbroman wrote:
> These look like they could take "const CompositorAnimationTimeline&". Any
reason to take non-const pointers?
Yes, technically we can. Only because 'const' semantics is shallow in C++.
I would be different to cc where we add/remove timelines to a non-const list of
timelines.

https://codereview.chromium.org/2559273003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/WebFrameWidgetImpl.h (right):

https://codereview.chromium.org/2559273003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebFrameWidgetImpl.h:223:
std::unique_ptr<CompositorAnimationHost> m_compositorAnimationHost;
On 2016/12/13 at 16:38:32, jbroman wrote:
> Up to you, but if having a compositor animation host is the usual case then
I'd lean towards using WTF::Optional<> (or making CompositorAnimationHost
support nullability itself, but I gather we're leaning away from that). This is
approximately a pointer-to-a-pointer, which is a little much.
Yup. I was thinking about base::Oprional here. Will do that.

[loyso (OOO), 2016-12-19 04:03:05]

The CQ bit was checked by loyso@chromium.org to run a CQ dry run

[loyso (OOO), 2016-12-19 04:03:12]

https://codereview.chromium.org/2559273003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp (right):

https://codereview.chromium.org/2559273003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:1818: if
(LocalFrame* frame = box().frame()) {

Done.

https://codereview.chromium.org/2559273003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/animation/CompositorAnimationHost.h
(right):

https://codereview.chromium.org/2559273003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/animation/CompositorAnimationHost.h:28: void
addTimeline(CompositorAnimationTimeline*);
Done.

https://codereview.chromium.org/2559273003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/GraphicsLayerTest.cpp (right):

https://codereview.chromium.org/2559273003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/GraphicsLayerTest.cpp:122: auto host
= WTF::makeUnique<CompositorAnimationHost>(
Done.

https://codereview.chromium.org/2559273003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/WebFrameWidgetImpl.h (right):

https://codereview.chromium.org/2559273003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebFrameWidgetImpl.h:223:
std::unique_ptr<CompositorAnimationHost> m_compositorAnimationHost;
Since we want to replace it with a raw weak pointer (similar to
WebLayerTreeView* m_layerTreeView;) I prefer to continue with
std::unique_ptr<CompositorAnimationHost>.
Added a todo note in
https://bugs.chromium.org/p/chromium/issues/detail?id=584551

[commit-bot: I haz the power, 2016-12-19 04:03:21]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[loyso (OOO), 2016-12-19 05:17:33]

The CQ bit was checked by loyso@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-19 05:17:52]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[loyso (OOO), 2016-12-19 06:47:52]

The CQ bit was unchecked by loyso@chromium.org

[loyso (OOO), 2016-12-19 06:48:08]

The CQ bit was checked by loyso@chromium.org

[loyso (OOO), 2016-12-19 06:48:09]

The patchset sent to the CQ was uploaded after l-g-t-m from ajuma@chromium.org,
aelias@chromium.org, jbroman@chromium.org
Link to the patchset: https://codereview.chromium.org/2559273003/#ps80001
(title: "Allocate on stack in unit tests.")

[commit-bot: I haz the power, 2016-12-19 06:48:21]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-19 07:14:44]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1482130088786060,
"parent_rev": "f35547712b90637c661c4456644827b76b9524e1", "commit_rev":
"7386e93e384faf26bbf6fb649044ac8f5676b68b"}

[commit-bot: I haz the power, 2016-12-19 07:15:10]

Description was changed from

==========
Blink Compositor Animation: Introduce CompositorAnimationHost correctly.

So we get it from WebLayerTreeView (RenderWidgetCompositor), not from
timeline.

BUG=610763
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Blink Compositor Animation: Introduce CompositorAnimationHost correctly.

So we get it from WebLayerTreeView (RenderWidgetCompositor), not from
timeline.

BUG=610763
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2559273003
==========

[commit-bot: I haz the power, 2016-12-19 07:15:12]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-12-19 07:17:07]

Description was changed from

==========
Blink Compositor Animation: Introduce CompositorAnimationHost correctly.

So we get it from WebLayerTreeView (RenderWidgetCompositor), not from
timeline.

BUG=610763
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2559273003
==========

to

==========
Blink Compositor Animation: Introduce CompositorAnimationHost correctly.

So we get it from WebLayerTreeView (RenderWidgetCompositor), not from
timeline.

BUG=610763
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Committed: https://crrev.com/ffcf86ca01510297c45f67c09de79859d541d98f
Cr-Commit-Position: refs/heads/master@{#439423}
==========

[commit-bot: I haz the power, 2016-12-19 07:17:09]

Patchset 5 (id:??) landed as
https://crrev.com/ffcf86ca01510297c45f67c09de79859d541d98f
Cr-Commit-Position: refs/heads/master@{#439423}