Remove ScrollableArea::GetFrameViewBase and move ScheduleAnimation into subclasses.

Remove ScrollableArea::GetFrameViewBase and move
ScrollableArea::ScheduleAnimation into subclasses.

This is part of an overall effort to remove the FrameViewBase
(old Widget) class.  This change helps to remove an unneeded reference.

ScrollableArea implemented ScheduleAnimation by defining GetFrameViewBase
that subclasses could implement.  The alternative implementation is
for subclasses to implement ScheduleAnimation and then there is no need
for the awkward reference to FrameViewBase.

I have added ScheduleAnimation into LocalFrame which is the core class
required by HostWindow/ChromeClientImpl.  All subclasses of ScrollableArea
now implement ScheduleAnimation by calling it on their local reference to
LocalFrame.


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

Review-Url: https://codereview.chromium.org/2826893003
Cr-Commit-Position: refs/heads/master@{#465546}
Committed: https://chromium.googlesource.com/chromium/src/+/c00bae0bd50204fc323a37c28f9e77b39c8dc376

[joelhockey, 2017-04-19 05:57:41]

Description was changed from

==========
Remove ScrollableArea::GetFrameViewBase and move
ScrollableArea::ScheduleAnimation into subclass FrameView.

BUG=637460
==========

to

==========
Remove ScrollableArea::GetFrameViewBase and move
ScrollableArea::ScheduleAnimation into subclass FrameView.

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

[joelhockey, 2017-04-19 05:57:49]

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

[commit-bot: I haz the power, 2017-04-19 05:58:10]

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, 2017-04-19 06:01:52]

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

[commit-bot: I haz the power, 2017-04-19 06:01:53]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_layout_tests_slimming_paint_v2 on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_layout_te...)

[joelhockey, 2017-04-19 06:08:38]

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

[commit-bot: I haz the power, 2017-04-19 06:08:54]

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

[joelhockey, 2017-04-19 06:40:42]

Description was changed from

==========
Remove ScrollableArea::GetFrameViewBase and move
ScrollableArea::ScheduleAnimation into subclass FrameView.

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

to

==========
Remove ScrollableArea::GetFrameViewBase and move
ScrollableArea::ScheduleAnimation into subclasses.

This is part of an overall effort to remove the FrameViewBase
(old Widget) class.  This change helps to remove an unneeded reference.

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

[joelhockey, 2017-04-19 06:40:42]

joelhockey@chromium.org changed reviewers:
+ slangley@chromium.org

[joelhockey, 2017-04-19 06:41:04]

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

[commit-bot: I haz the power, 2017-04-19 06:41:21]

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

[joelhockey, 2017-04-19 06:47:54]

Description was changed from

==========
Remove ScrollableArea::GetFrameViewBase and move
ScrollableArea::ScheduleAnimation into subclasses.

This is part of an overall effort to remove the FrameViewBase
(old Widget) class.  This change helps to remove an unneeded reference.

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

to

==========
Remove ScrollableArea::GetFrameViewBase and move
ScrollableArea::ScheduleAnimation into subclasses.

This is part of an overall effort to remove the FrameViewBase
(old Widget) class.  This change helps to remove an unneeded reference.

ScrollableArea implemented ScheduleAnimation by defining GetFrameViewBase
that subclasses could implement.  The alternative implementation is
for subclasses to implement ScheduleAnimation and then there is no need
for the awkward reference to FrameViewBase.

I have added ScheduleAnimation into LocalFrame which is the core class
required by HostWindow/ChromeClientImpl.  All subclasses of ScrollableArea
now implement ScheduleAnimation by calling it on their local reference to
LocalFrame.


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

[slangley, 2017-04-19 07:15:45]

lgtm

https://codereview.chromium.org/2826893003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/FrameViewTest.cpp (right):

https://codereview.chromium.org/2826893003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/FrameViewTest.cpp:48: }
add vertical whitespace?

https://codereview.chromium.org/2826893003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/LocalFrame.cpp (right):

https://codereview.chromium.org/2826893003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/LocalFrame.cpp:878:
window->ScheduleAnimation(this);
seems weird not to have HostWindow::ScheduleAnimation return a bool as well and
then plumb that result out through here ... but i guess that's the way it's
always been?

[joelhockey, 2017-04-19 07:20:47]

https://codereview.chromium.org/2826893003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/FrameViewTest.cpp (right):

https://codereview.chromium.org/2826893003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/FrameViewTest.cpp:48: }
On 2017/04/19 at 07:15:45, slangley wrote:
> add vertical whitespace?

I was going to add it, but then I looked at the rest of the file and it looks
like there is a general lack of whitespace, so I don't really want to start
anything that I'm not keen to follow through on.

https://codereview.chromium.org/2826893003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/LocalFrame.cpp (right):

https://codereview.chromium.org/2826893003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/LocalFrame.cpp:878:
window->ScheduleAnimation(this);
On 2017/04/19 at 07:15:45, slangley wrote:
> seems weird not to have HostWindow::ScheduleAnimation return a bool as well
and then plumb that result out through here ... but i guess that's the way it's
always been?

Yes, it's kind of hard to know what the bool even means.  There does appear to
be one place where it is checked.
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/scrol...

[joelhockey, 2017-04-19 07:22:51]

joelhockey@chromium.org changed reviewers:
+ dcheng@chromium.org, haraken@chromium.org

[joelhockey, 2017-04-19 07:22:52]

dcheng@, haraken@, ptal

[commit-bot: I haz the power, 2017-04-19 08:47:12]

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

[commit-bot: I haz the power, 2017-04-19 08:47:13]

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

[joelhockey, 2017-04-19 09:16:17]

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

[commit-bot: I haz the power, 2017-04-19 09:16:44]

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

[haraken, 2017-04-19 09:44:07]

LGTM with comments. You can address the comments in follow-up CLs.

https://codereview.chromium.org/2826893003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/2826893003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/FrameView.cpp:2750: bool
FrameView::ScheduleAnimation() {

It seems like we have too many proxy methods to call ScheduleAnimation. Can we
try to remove FrameView::ScheduleAnimation and Frame::ScheduleAnimation?

The caller sites can directly call HostWindow::ScheduleAnimation.

(Not related to this CL, HostWindow should be renamed to PlatformChromeClient.
It's never related to a window.)

https://codereview.chromium.org/2826893003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/LocalFrame.cpp (right):

https://codereview.chromium.org/2826893003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/LocalFrame.cpp:878:
window->ScheduleAnimation(this);
On 2017/04/19 07:20:47, joelhockey wrote:
> On 2017/04/19 at 07:15:45, slangley wrote:
> > seems weird not to have HostWindow::ScheduleAnimation return a bool as well
> and then plumb that result out through here ... but i guess that's the way
it's
> always been?
> 
> Yes, it's kind of hard to know what the bool even means.  There does appear to
> be one place where it is checked.
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/scrol...

Yeah, would you fix it in a follow-up?

[commit-bot: I haz the power, 2017-04-19 10:12:01]

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

[commit-bot: I haz the power, 2017-04-19 10:12:01]

Dry run: This issue passed the CQ dry run.

[joelhockey, 2017-04-19 10:16:48]

The CQ bit was checked by joelhockey@chromium.org

[commit-bot: I haz the power, 2017-04-19 10:17:19]

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, 2017-04-19 10:21:51]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1492597008457360,
"parent_rev": "be0a6071e24bc1e0f432533042d3d813bbe6ed3f", "commit_rev":
"c00bae0bd50204fc323a37c28f9e77b39c8dc376"}

[commit-bot: I haz the power, 2017-04-19 10:22:02]

Description was changed from

==========
Remove ScrollableArea::GetFrameViewBase and move
ScrollableArea::ScheduleAnimation into subclasses.

This is part of an overall effort to remove the FrameViewBase
(old Widget) class.  This change helps to remove an unneeded reference.

ScrollableArea implemented ScheduleAnimation by defining GetFrameViewBase
that subclasses could implement.  The alternative implementation is
for subclasses to implement ScheduleAnimation and then there is no need
for the awkward reference to FrameViewBase.

I have added ScheduleAnimation into LocalFrame which is the core class
required by HostWindow/ChromeClientImpl.  All subclasses of ScrollableArea
now implement ScheduleAnimation by calling it on their local reference to
LocalFrame.


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

to

==========
Remove ScrollableArea::GetFrameViewBase and move
ScrollableArea::ScheduleAnimation into subclasses.

This is part of an overall effort to remove the FrameViewBase
(old Widget) class.  This change helps to remove an unneeded reference.

ScrollableArea implemented ScheduleAnimation by defining GetFrameViewBase
that subclasses could implement.  The alternative implementation is
for subclasses to implement ScheduleAnimation and then there is no need
for the awkward reference to FrameViewBase.

I have added ScheduleAnimation into LocalFrame which is the core class
required by HostWindow/ChromeClientImpl.  All subclasses of ScrollableArea
now implement ScheduleAnimation by calling it on their local reference to
LocalFrame.


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

Review-Url: https://codereview.chromium.org/2826893003
Cr-Commit-Position: refs/heads/master@{#465546}
Committed:
https://chromium.googlesource.com/chromium/src/+/c00bae0bd50204fc323a37c28f9e...
==========

[commit-bot: I haz the power, 2017-04-19 10:22:04]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/c00bae0bd50204fc323a37c28f9e...

[joelhockey, 2017-04-19 10:27:54]

https://codereview.chromium.org/2826893003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/2826893003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/FrameView.cpp:2750: bool
FrameView::ScheduleAnimation() {
On 2017/04/19 at 09:44:07, haraken wrote:
> It seems like we have too many proxy methods to call ScheduleAnimation. Can we
try to remove FrameView::ScheduleAnimation and Frame::ScheduleAnimation?
> 
> The caller sites can directly call HostWindow::ScheduleAnimation.
> 
> (Not related to this CL, HostWindow should be renamed to PlatformChromeClient.
It's never related to a window.)

2 CLs on the wey soon for this.

https://codereview.chromium.org/2826893003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/LocalFrame.cpp (right):

https://codereview.chromium.org/2826893003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/LocalFrame.cpp:878:
window->ScheduleAnimation(this);
On 2017/04/19 at 09:44:07, haraken wrote:
> On 2017/04/19 07:20:47, joelhockey wrote:
> > On 2017/04/19 at 07:15:45, slangley wrote:
> > > seems weird not to have HostWindow::ScheduleAnimation return a bool as
well
> > and then plumb that result out through here ... but i guess that's the way
it's
> > always been?
> > 
> > Yes, it's kind of hard to know what the bool even means.  There does appear
to
> > be one place where it is checked.
> >
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/scrol...
> 
> Yeah, would you fix it in a follow-up?

I've looked a little, but I'm not sure that I can really improve this much. 
I've made a note to look at it more later.

[dcheng, 2017-04-19 11:26:25]

https://codereview.chromium.org/2826893003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/LocalFrame.cpp (right):

https://codereview.chromium.org/2826893003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/LocalFrame.cpp:876: bool
LocalFrame::ScheduleAnimation(HostWindow* window) {
Can we avoid plumbing HostWindow* through here?

https://codereview.chromium.org/2826893003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/HostWindow.h (right):

https://codereview.chromium.org/2826893003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/HostWindow.h:59: virtual void
ScheduleAnimation(LocalFrame*) = 0;
The reason we didn't plumb through Frame to begin with is because HostWindow is
in platform and shouldn't know about frames.

https://codereview.chromium.org/2826893003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/InspectorOverlay.cpp (right):

https://codereview.chromium.org/2826893003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/InspectorOverlay.cpp:406: // TODO(joelhockey):
Remove this check once all tests pass.
I don't think I understand this comment. Presumably all the tests pass if this
CL landed?

[joelhockey, 2017-04-19 11:33:18]

https://codereview.chromium.org/2826893003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/LocalFrame.cpp (right):

https://codereview.chromium.org/2826893003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/LocalFrame.cpp:876: bool
LocalFrame::ScheduleAnimation(HostWindow* window) {
On 2017/04/19 at 11:26:25, dcheng (OOO through May 2) wrote:
> Can we avoid plumbing HostWindow* through here?

Yes, I've fixed this in https://codereview.chromium.org/2829613002

https://codereview.chromium.org/2826893003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/HostWindow.h (right):

https://codereview.chromium.org/2826893003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/HostWindow.h:59: virtual void
ScheduleAnimation(LocalFrame*) = 0;
On 2017/04/19 at 11:26:25, dcheng (OOO through May 2) wrote:
> The reason we didn't plumb through Frame to begin with is because HostWindow
is in platform and shouldn't know about frames.

Yes, that makes sense.

Could there be an empty PlatformFrame class in platform that
core/frame/LocalFrame extends?

https://codereview.chromium.org/2826893003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/InspectorOverlay.cpp (right):

https://codereview.chromium.org/2826893003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/InspectorOverlay.cpp:406: // TODO(joelhockey):
Remove this check once all tests pass.
On 2017/04/19 at 11:26:25, dcheng (OOO through May 2) wrote:
> I don't think I understand this comment. Presumably all the tests pass if this
CL landed?

Thanks for picking this up.  I'll send a fix now.