Deleted Widget/FrameViewBase

Deleted Widget/FrameViewBase.

Scrollbar has no need for a 'parent' concept.  It already has
ScrollableArea which is the container of scrollbars.

Moved FrameViewBase::ConvertFromRootFrame method to ScrollableArea
where it is implemented in FrameView and PaintLayerScrollableArea
along with other similar Convert* methods.

Custom PaintLayerScrollableArea scrollbars must receive
StyleUpdate calls when frame comes in and out of focus.
The scrollbars attached to FrameView already receive these via
RecalculateCustomScrollbarStyle, so they do not need to be
stored in FrameView::scrollbars_.

LayoutScrollbar was relying on overriding and listening for
SetParent(nullptr) to dispose.  This is now done by overriding
Scrollbar::DisconnectFromScrollableArea.

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

Review-Url: https://codereview.chromium.org/2855523002
Cr-Commit-Position: refs/heads/master@{#471494}
Committed: https://chromium.googlesource.com/chromium/src/+/4838cef544c67d342c418d208e52cd8a4cf1a3d2

[joelhockey, 2017-05-01 05:14:38]

Description was changed from

==========
Rename FrameViewBase to ScrollbarParent, remove unneeded methods

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

to

==========
Rename FrameViewBase to ScrollbarParent, remove unneeded methods

Rename FrameViewBase to ScrollbarParent.  I like this name since the
relationship that Scrollbar has with this class is that this must be
the base class of Scrollbar::SetParent.

Removed unneeded Dispose method for ScrollbarParent.

Moved ConvertFromRootFrame methods to FrameView.  Out of the 3
overloaded ConvertFromRootFrame methods (IntRect, IntPoint, FloatPoint),
only IntPoint is used in Scrollbar classes.  So I have renamed
this method to ConvertPointFromRootFrame.

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

[joelhockey, 2017-05-01 05:18:18]

Description was changed from

==========
Rename FrameViewBase to ScrollbarParent, remove unneeded methods

Rename FrameViewBase to ScrollbarParent.  I like this name since the
relationship that Scrollbar has with this class is that this must be
the base class of Scrollbar::SetParent.

Removed unneeded Dispose method for ScrollbarParent.

Moved ConvertFromRootFrame methods to FrameView.  Out of the 3
overloaded ConvertFromRootFrame methods (IntRect, IntPoint, FloatPoint),
only IntPoint is used in Scrollbar classes.  So I have renamed
this method to ConvertPointFromRootFrame.

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

to

==========
Rename FrameViewBase to ScrollbarParent, remove unneeded methods

Rename FrameViewBase to ScrollbarParent.  I like this name since the
relationship that Scrollbar has with this class is that this must be
the base class of Scrollbar::SetParent.

Removed unneeded Dispose method for ScrollbarParent.

Moved ConvertFromRootFrame methods to FrameView.  Out of the 3
overloaded ConvertFromRootFrame methods (IntRect, IntPoint, FloatPoint),
only IntPoint is used in Scrollbar classes.  So I have renamed
this method to ConvertPointFromRootFrame.

Changed PlatformChromeClient::ViewportToScreen to use forward-declared class
FrameView rather than FrameViewBase.  If this change feels like it is
pushing the boundaries of having platform depend on core, then I suggest that
a new class platform/frame/PlatformFrameView be created which can be a base
class of FrameView.

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

[joelhockey, 2017-05-01 05:21:38]

Description was changed from

==========
Rename FrameViewBase to ScrollbarParent, remove unneeded methods

Rename FrameViewBase to ScrollbarParent.  I like this name since the
relationship that Scrollbar has with this class is that this must be
the base class of Scrollbar::SetParent.

Removed unneeded Dispose method for ScrollbarParent.

Moved ConvertFromRootFrame methods to FrameView.  Out of the 3
overloaded ConvertFromRootFrame methods (IntRect, IntPoint, FloatPoint),
only IntPoint is used in Scrollbar classes.  So I have renamed
this method to ConvertPointFromRootFrame.

Changed PlatformChromeClient::ViewportToScreen to use forward-declared class
FrameView rather than FrameViewBase.  If this change feels like it is
pushing the boundaries of having platform depend on core, then I suggest that
a new class platform/frame/PlatformFrameView be created which can be a base
class of FrameView.

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

to

==========
Rename FrameViewBase to ScrollbarParent, remove unneeded methods

Rename FrameViewBase to ScrollbarParent.  I like this name since the
relationship that Scrollbar has with this class is that this must be
the base class of Scrollbar::SetParent.

Removed unneeded Dispose method for ScrollbarParent.

Moved ConvertFromRootFrame methods to FrameView.  Out of the 3
overloaded ConvertFromRootFrame methods (IntRect, IntPoint, FloatPoint),
only IntPoint is used in Scrollbar classes.  So I have renamed
this method to ConvertPointFromRootFrame.

Removed platform/FrameViewBase.h as unused import from a number of files.

Changed PlatformChromeClient::ViewportToScreen to use forward-declared class
FrameView rather than FrameViewBase.  If this change feels like it is
pushing the boundaries of having platform depend on core, then I suggest that
a new class platform/frame/PlatformFrameView be created which can be a base
class of FrameView.

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

[joelhockey, 2017-05-01 05:21:53]

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

[commit-bot: I haz the power, 2017-05-01 05:22:11]

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-05-01 05:26:08]

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

[commit-bot: I haz the power, 2017-05-01 05:26:09]

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)

[joelhockey, 2017-05-01 05:27:52]

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

[commit-bot: I haz the power, 2017-05-01 05:28:06]

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-05-01 05:32:05]

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

[commit-bot: I haz the power, 2017-05-01 05:32:05]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  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_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[joelhockey, 2017-05-01 05:35:21]

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

[commit-bot: I haz the power, 2017-05-01 05:35:36]

Dry run: CQ is trying da patch.

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

[joelhockey, 2017-05-01 05:43:17]

Description was changed from

==========
Rename FrameViewBase to ScrollbarParent, remove unneeded methods

Rename FrameViewBase to ScrollbarParent.  I like this name since the
relationship that Scrollbar has with this class is that this must be
the base class of Scrollbar::SetParent.

Removed unneeded Dispose method for ScrollbarParent.

Moved ConvertFromRootFrame methods to FrameView.  Out of the 3
overloaded ConvertFromRootFrame methods (IntRect, IntPoint, FloatPoint),
only IntPoint is used in Scrollbar classes.  So I have renamed
this method to ConvertPointFromRootFrame.

Removed platform/FrameViewBase.h as unused import from a number of files.

Changed PlatformChromeClient::ViewportToScreen to use forward-declared class
FrameView rather than FrameViewBase.  If this change feels like it is
pushing the boundaries of having platform depend on core, then I suggest that
a new class platform/frame/PlatformFrameView be created which can be a base
class of FrameView.

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

to

==========
Rename FrameViewBase to ScrollbarParent, remove unneeded methods

Rename FrameViewBase to ScrollbarParent.  I like this name since the
relationship that Scrollbar has with this class is that this must be
the base class of Scrollbar::SetParent.

Removed unneeded Dispose method for ScrollbarParent.

Moved ConvertFromRootFrame methods to FrameView.  Out of the 3
overloaded ConvertFromRootFrame methods (IntRect, IntPoint, FloatPoint),
only IntPoint is used in Scrollbar classes.  I have renamed
this method to ConvertPointFromRootFrame so that there is no
collision with other methods required in FrameView.

Removed platform/FrameViewBase.h as unused import from a number of files.

Changed PlatformChromeClient::ViewportToScreen to use forward-declared class
FrameView rather than FrameViewBase.  If this change feels like it is
pushing the boundaries of having platform depend on core, then I suggest that
a new class platform/frame/PlatformFrameView be created which can be a base
class of FrameView.

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

[joelhockey, 2017-05-01 05:55:50]

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

[joelhockey, 2017-05-01 05:55:51]

I'm planning to write up a doc describing where I see the end-state of my
current work being.  I hope to have that very soon.

This CL looked like low-hanging fruit though, since I think this is the
end-state of the Widget class.  If figured it was preferable to send out a
concrete CL for this rather than a doc or email to discuss.

Kentaro had made a suggestion about renaming this class to possibly
ScrollbarLayoutController, but I have gone with ScrollbarParent which I prefer.

This CL is dependent on http://crrev.com/2845583002 which removes FrameViewBase
as base
class of RemoteFrameView.  Once that work is done, then I feel very happy with
this CL how
Widget / FrameViewBase / ScrollbarParent only requires a single
virtual method which is used by Scrollbar and implemented by FrameView.

Next step for me is to document a little about the changes I have made so far,
and what I
think is still worth doing.  It seems the most popular thing is to remove
the widget tree (FrameView::parent_, FrameView::children_).  I'm skeptical that
this
can be done, and unclear about what benefit it provides, but I'm still looking
into it.

[joelhockey, 2017-05-01 06:02:27]

https://codereview.chromium.org/2855523002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/scroll/ScrollbarParent.h (right):

https://codereview.chromium.org/2855523002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scroll/ScrollbarParent.h:1: /*
It appears that git or something thinks that I copied ScrollbarParent.h from
PlatformChromeClient.h.  This is not the case.  I did:

git mv third_party/WebKit/Source/platform/FrameViewBase.h
third_party/WebKit/Source/platform/scroll/ScrollbarParent.h

Please let me know if there is some other hint command I should run to make it
clear that this file is a fork/rename of FrameViewBase.h.

[haraken, 2017-05-01 07:51:58]

https://codereview.chromium.org/2855523002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/FrameView.h (right):

https://codereview.chromium.org/2855523002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/FrameView.h:105: public ScrollbarParent,

Are you planning to remove this inheritance in the end state?

It looks a bit strange that things other than Scrollbar inherits from
ScrollbarParent.

[commit-bot: I haz the power, 2017-05-01 07:59:37]

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

[commit-bot: I haz the power, 2017-05-01 07:59:38]

Dry run: This issue passed the CQ dry run.

[joelhockey, 2017-05-01 09:08:51]

https://codereview.chromium.org/2855523002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/FrameView.h (right):

https://codereview.chromium.org/2855523002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/FrameView.h:105: public ScrollbarParent,
On 2017/05/01 at 07:51:58, haraken wrote:
> Are you planning to remove this inheritance in the end state?
> 
> It looks a bit strange that things other than Scrollbar inherits from
ScrollbarParent.

No, FrameView will remain as Scrollbar's parent.  It is the only class that
implements ScrollbarParent.

btw, Scrollbar does not (any longer) inherit from ScrollbarParent - it is not
its own parent.  Previously, Scrollbar did inherit from Widget, and Widget was
also the parent of scrollbar.  It reminds me of https://youtu.be/eYlJH81dSiw
Now that Scrollbar is a specialized child of FrameView, there is no need for it
to inherit Widget.

[haraken, 2017-05-01 09:16:42]

On 2017/05/01 09:08:51, joelhockey wrote:
>
https://codereview.chromium.org/2855523002/diff/60001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/frame/FrameView.h (right):
> 
>
https://codereview.chromium.org/2855523002/diff/60001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/frame/FrameView.h:105: public ScrollbarParent,
> On 2017/05/01 at 07:51:58, haraken wrote:
> > Are you planning to remove this inheritance in the end state?
> > 
> > It looks a bit strange that things other than Scrollbar inherits from
> ScrollbarParent.
> 
> No, FrameView will remain as Scrollbar's parent.  It is the only class that
> implements ScrollbarParent.
> 
> btw, Scrollbar does not (any longer) inherit from ScrollbarParent - it is not
> its own parent.  Previously, Scrollbar did inherit from Widget, and Widget was
> also the parent of scrollbar.  It reminds me of https://youtu.be/eYlJH81dSiw
> Now that Scrollbar is a specialized child of FrameView, there is no need for
it
> to inherit Widget.

That makes sense.

I'd like to have dcheng@ and szager@ agree on this CL.

[dcheng, 2017-05-01 15:28:36]

The overall approach seems reasonable to me. Just a few minor comments.

https://codereview.chromium.org/2855523002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/scroll/ScrollbarParent.h (right):

https://codereview.chromium.org/2855523002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scroll/ScrollbarParent.h:37: //
ScrollbarParent is the parent of Scrollbar.
Nit: this comment is slightly confusing, because it could be interpreted to read
that this is the parent *class* of Scrollbar (which it isn't)

https://codereview.chromium.org/2855523002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scroll/ScrollbarParent.h:40: class
PLATFORM_EXPORT ScrollbarParent : public GarbageCollectedMixin {
One potential name alternative: ScrollbarContainer?

(I don't feel strongly about this, but per the above comment, 'parent' could
read slightly ambiguously, as one doesn't normally imagine scrollbars as having
a tree hierarchy)

https://codereview.chromium.org/2855523002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scroll/ScrollbarParent.h:43: virtual
~ScrollbarParent(){};
Nit: remove the ;

[dcheng, 2017-05-01 15:29:37]

On 2017/05/01 15:28:36, dcheng (OOO through May 2) wrote:
> The overall approach seems reasonable to me. Just a few minor comments.
> 
>
https://codereview.chromium.org/2855523002/diff/60001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/platform/scroll/ScrollbarParent.h (right):
> 
>
https://codereview.chromium.org/2855523002/diff/60001/third_party/WebKit/Sour...
> third_party/WebKit/Source/platform/scroll/ScrollbarParent.h:37: //
> ScrollbarParent is the parent of Scrollbar.
> Nit: this comment is slightly confusing, because it could be interpreted to
read
> that this is the parent *class* of Scrollbar (which it isn't)
> 
>
https://codereview.chromium.org/2855523002/diff/60001/third_party/WebKit/Sour...
> third_party/WebKit/Source/platform/scroll/ScrollbarParent.h:40: class
> PLATFORM_EXPORT ScrollbarParent : public GarbageCollectedMixin {
> One potential name alternative: ScrollbarContainer?
> 
> (I don't feel strongly about this, but per the above comment, 'parent' could
> read slightly ambiguously, as one doesn't normally imagine scrollbars as
having
> a tree hierarchy)
> 
>
https://codereview.chromium.org/2855523002/diff/60001/third_party/WebKit/Sour...
> third_party/WebKit/Source/platform/scroll/ScrollbarParent.h:43: virtual
> ~ScrollbarParent(){};
> Nit: remove the ;

(I'll try to take a look at https://codereview.chromium.org/2845583002#ps60001
before I get on the plane tomorrow)

[joelhockey, 2017-05-01 22:03:18]

I might add a few comments on
https://codereview.chromium.org/2845583002/#ps60001 which hopefully make it
easier to review.

https://codereview.chromium.org/2855523002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/scroll/ScrollbarParent.h (right):

https://codereview.chromium.org/2855523002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scroll/ScrollbarParent.h:37: //
ScrollbarParent is the parent of Scrollbar.
On 2017/05/01 at 15:28:36, dcheng (OOO through May 2) wrote:
> Nit: this comment is slightly confusing, because it could be interpreted to
read that this is the parent *class* of Scrollbar (which it isn't)

Fair point.  Comment modified

https://codereview.chromium.org/2855523002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scroll/ScrollbarParent.h:40: class
PLATFORM_EXPORT ScrollbarParent : public GarbageCollectedMixin {
On 2017/05/01 at 15:28:36, dcheng (OOO through May 2) wrote:
> One potential name alternative: ScrollbarContainer?
> 
> (I don't feel strongly about this, but per the above comment, 'parent' could
read slightly ambiguously, as one doesn't normally imagine scrollbars as having
a tree hierarchy)

I'm happy to change name if there is consensus or at least majority agreement
with szager and haraken.  My vote is still for ScrollbarParent since this is the
class that is required for Scrollbar::SetParent.  If we rename the class to
ScrollbarContainer then I would suggest to change Scrollbar method to
SetContainer.

https://codereview.chromium.org/2855523002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scroll/ScrollbarParent.h:43: virtual
~ScrollbarParent(){};
On 2017/05/01 at 15:28:36, dcheng (OOO through May 2) wrote:
> Nit: remove the ;

Done

[szager1, 2017-05-02 14:31:36]

https://codereview.chromium.org/2855523002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/scroll/ScrollbarParent.h (right):

https://codereview.chromium.org/2855523002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scroll/ScrollbarParent.h:41: class
PLATFORM_EXPORT ScrollbarParent : public GarbageCollectedMixin {
Are you planning to make PaintLayerScrollableArea inherit from ScrollbarParent?

It's strange to me to have a class called ScrollbarParent which is not an
ancestor of *every* class that owns Scrollbars.

Are you still planning to move paint/ScrollbarManager into platform/?  Or maybe
to get rid of ScrollbarManager in favor of ScrollbarParent?

I really would like to see the semantics for Scrollbar ownership/management
unified into a single class in platform/, whether it's ScrollbarParent or
ScrollbarManager.

[joelhockey, 2017-05-02 22:24:42]

On 2017/05/02 at 14:31:36, szager wrote:
>
https://codereview.chromium.org/2855523002/diff/80001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/platform/scroll/ScrollbarParent.h (right):
> 
>
https://codereview.chromium.org/2855523002/diff/80001/third_party/WebKit/Sour...
> third_party/WebKit/Source/platform/scroll/ScrollbarParent.h:41: class
PLATFORM_EXPORT ScrollbarParent : public GarbageCollectedMixin {
> Are you planning to make PaintLayerScrollableArea inherit from
ScrollbarParent?
> 
> It's strange to me to have a class called ScrollbarParent which is not an
ancestor of *every* class that owns Scrollbars.
> 
> Are you still planning to move paint/ScrollbarManager into platform/?  Or
maybe to get rid of ScrollbarManager in favor of ScrollbarParent?
> 
> I really would like to see the semantics for Scrollbar ownership/management
unified into a single class in platform/, whether it's ScrollbarParent or
ScrollbarManager.

> Are you planning to make PaintLayerScrollableArea inherit from
ScrollbarParent?

No, I'm not planning to change PaintLayerScrollableArea.  The scope of my
current project is cleaning up the Widget class as per
http://go/architecture-backlog > Frames tab > Widget cleanup, and also described
in the attached bug.  I see in http://bit.ly/root-layer-scrolling that skobes@
has a plan to unify FrameView and PLSA.  Once I finish the current work, I'm
open to looking at scrollbars as my next project, but there is also other work
that our team is doing.

I'm not planning to move ScrollbarManager into platform any more.  I started a
CL to do move ScrollbarManager and merge it with FrameViewBase, but it wasn't
working out.  The moving bit would probably be fine, but the merging bit didn't
make sense.  One possibility that I will try right now is to move the single
remaining ScrollbarParent::ConvertFromRootFrame method into ScrollableArea which
is the common ancestor of every class that owns Scrollbars.  The method will
then be implemented in both FrameView and PLSA.  And the Widget class will
totally disappear.

[joelhockey, 2017-05-03 01:15:06]

Description was changed from

==========
Rename FrameViewBase to ScrollbarParent, remove unneeded methods

Rename FrameViewBase to ScrollbarParent.  I like this name since the
relationship that Scrollbar has with this class is that this must be
the base class of Scrollbar::SetParent.

Removed unneeded Dispose method for ScrollbarParent.

Moved ConvertFromRootFrame methods to FrameView.  Out of the 3
overloaded ConvertFromRootFrame methods (IntRect, IntPoint, FloatPoint),
only IntPoint is used in Scrollbar classes.  I have renamed
this method to ConvertPointFromRootFrame so that there is no
collision with other methods required in FrameView.

Removed platform/FrameViewBase.h as unused import from a number of files.

Changed PlatformChromeClient::ViewportToScreen to use forward-declared class
FrameView rather than FrameViewBase.  If this change feels like it is
pushing the boundaries of having platform depend on core, then I suggest that
a new class platform/frame/PlatformFrameView be created which can be a base
class of FrameView.

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

to

==========
Rename FrameViewBase to ScrollbarParent, remove unneeded methods

Rename FrameViewBase to ScrollbarParent.  I like this name since the
relationship that Scrollbar has with this class is that this must be
the base class of Scrollbar::SetParent.

Removed unneeded Dispose method for ScrollbarParent.

Moved ConvertFromRootFrame methods to FrameView.  Out of the 3
overloaded ConvertFromRootFrame methods (IntRect, IntPoint, FloatPoint),
only IntPoint is used in Scrollbar classes.  I have renamed
this method to ConvertPointFromRootFrame so that there is no
collision with other methods required in FrameView.

Removed platform/FrameViewBase.h as unused import from a number of files.

Changed PlatformChromeClient::ViewportToScreen to use forward-declared class
FrameView rather than FrameViewBase.  If this change feels like it is
pushing the boundaries of having platform depend on core, then I suggest that
a new class platform/frame/PlatformFrameView be created which can be a base
class of FrameView.

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

[joelhockey, 2017-05-03 01:34:16]

Description was changed from

==========
Rename FrameViewBase to ScrollbarParent, remove unneeded methods

Rename FrameViewBase to ScrollbarParent.  I like this name since the
relationship that Scrollbar has with this class is that this must be
the base class of Scrollbar::SetParent.

Removed unneeded Dispose method for ScrollbarParent.

Moved ConvertFromRootFrame methods to FrameView.  Out of the 3
overloaded ConvertFromRootFrame methods (IntRect, IntPoint, FloatPoint),
only IntPoint is used in Scrollbar classes.  I have renamed
this method to ConvertPointFromRootFrame so that there is no
collision with other methods required in FrameView.

Removed platform/FrameViewBase.h as unused import from a number of files.

Changed PlatformChromeClient::ViewportToScreen to use forward-declared class
FrameView rather than FrameViewBase.  If this change feels like it is
pushing the boundaries of having platform depend on core, then I suggest that
a new class platform/frame/PlatformFrameView be created which can be a base
class of FrameView.

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

to

==========
Delete FrameViewBase.

Moved ConvertFromRootFrame method to ScrollableArea
where it is implemented in FrameView and PaintLayerScrollableArea.

Changed PlatformChromeClient::ViewportToScreen to use forward-declared class
FrameView rather than FrameViewBase.  If this change feels like it is
pushing the boundaries of having platform depend on core, then I suggest that
a new class platform/frame/PlatformFrameView be created which can be a base
class of FrameView.

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

[joelhockey, 2017-05-03 01:34:59]

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

[commit-bot: I haz the power, 2017-05-03 01:35:37]

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-05-03 01:40:29]

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

[commit-bot: I haz the power, 2017-05-03 01:40:32]

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[joelhockey, 2017-05-03 01:42:43]

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

[commit-bot: I haz the power, 2017-05-03 01:43:49]

Dry run: CQ is trying da patch.

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

[joelhockey, 2017-05-03 02:22:00]

Description was changed from

==========
Delete FrameViewBase.

Moved ConvertFromRootFrame method to ScrollableArea
where it is implemented in FrameView and PaintLayerScrollableArea.

Changed PlatformChromeClient::ViewportToScreen to use forward-declared class
FrameView rather than FrameViewBase.  If this change feels like it is
pushing the boundaries of having platform depend on core, then I suggest that
a new class platform/frame/PlatformFrameView be created which can be a base
class of FrameView.

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

to

==========
Deleted Widget/FrameViewBase.

Scrollbar has no need for a 'parent' concept.  It already has
ScrollableArea which is the container of scrollbars.

Moved FrameViewBase::ConvertFromRootFrame method to ScrollableArea
where it is implemented in FrameView and PaintLayerScrollableArea
along with other similar Convert* methods.

Removed FrameView::scrollbars_ set which is not required.
FrameView already implements ScrollableArea and has access to
HorizontalScrollbar and VerticalScrollbar.  The code in 
InvalidateAllCustomScrollbarsOnActiveChanged that calls StyleChanged
against each scrollbar in scrollbars_ is not required since this is
done already in RecalculateCustomScrollbarStyle which is called
within the same function.

LayoutScrollbar was relying on overriding and listening for
SetParent(nullptr) to dispose.  This is now done by overriding
Scrollbar::DisconnectFromScrollableArea.

Changed PlatformChromeClient::ViewportToScreen to use forward-declared class
FrameView rather than FrameViewBase.  If this change feels like it is
pushing the boundaries of having platform depend on core, then I suggest that
a new class platform/frame/PlatformFrameView be created which can be a base
class of FrameView.

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

[joelhockey, 2017-05-03 02:23:20]

dcheng@, szager@, ptal

The Widget/FrameViewBase class is now removed!

esprehn@ told me that there would be rapturous applause if I could ever achieve
this.

Thanks szager for your persistence to push me to understand scrollbars a bit
better.  Removing this class and the Scrollbar::parent_ field is much better
than what I had.

Reminder that this CL is still dependent on http://crrev.com/2845583002 where I
stop RemoteFrameView from inheriting from FrameViewBase.

[commit-bot: I haz the power, 2017-05-03 03:08:06]

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

[commit-bot: I haz the power, 2017-05-03 03:08:07]

Dry run: Try jobs failed on following builders:
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[joelhockey, 2017-05-03 04:26:19]

Description was changed from

==========
Deleted Widget/FrameViewBase.

Scrollbar has no need for a 'parent' concept.  It already has
ScrollableArea which is the container of scrollbars.

Moved FrameViewBase::ConvertFromRootFrame method to ScrollableArea
where it is implemented in FrameView and PaintLayerScrollableArea
along with other similar Convert* methods.

Removed FrameView::scrollbars_ set which is not required.
FrameView already implements ScrollableArea and has access to
HorizontalScrollbar and VerticalScrollbar.  The code in 
InvalidateAllCustomScrollbarsOnActiveChanged that calls StyleChanged
against each scrollbar in scrollbars_ is not required since this is
done already in RecalculateCustomScrollbarStyle which is called
within the same function.

LayoutScrollbar was relying on overriding and listening for
SetParent(nullptr) to dispose.  This is now done by overriding
Scrollbar::DisconnectFromScrollableArea.

Changed PlatformChromeClient::ViewportToScreen to use forward-declared class
FrameView rather than FrameViewBase.  If this change feels like it is
pushing the boundaries of having platform depend on core, then I suggest that
a new class platform/frame/PlatformFrameView be created which can be a base
class of FrameView.

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

to

==========
Deleted Widget/FrameViewBase.

Scrollbar has no need for a 'parent' concept.  It already has
ScrollableArea which is the container of scrollbars.

Moved FrameViewBase::ConvertFromRootFrame method to ScrollableArea
where it is implemented in FrameView and PaintLayerScrollableArea
along with other similar Convert* methods.

Renamed FrameView::scrollbars_ to paint_scrollbars.
Custom PaintLayerScrollableArea scrollbars must receive
StyleUpdate calls when frame comes in and out of focus.
The scrollbars attached to FrameView already receive these via
RecalculateCustomScrollbarStyle, so they do not need to be
stored in FrameView::scrollbars_.

LayoutScrollbar was relying on overriding and listening for
SetParent(nullptr) to dispose.  This is now done by overriding
Scrollbar::DisconnectFromScrollableArea.

Changed PlatformChromeClient::ViewportToScreen to use forward-declared class
FrameView rather than FrameViewBase.  If this change feels like it is
pushing the boundaries of having platform depend on core, then I suggest that
a new class platform/frame/PlatformFrameView be created which can be a base
class of FrameView.

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

[joelhockey, 2017-05-03 04:26:29]

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

[commit-bot: I haz the power, 2017-05-03 04:26:44]

Dry run: CQ is trying da patch.

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

[joelhockey, 2017-05-03 04:28:16]

Description was changed from

==========
Deleted Widget/FrameViewBase.

Scrollbar has no need for a 'parent' concept.  It already has
ScrollableArea which is the container of scrollbars.

Moved FrameViewBase::ConvertFromRootFrame method to ScrollableArea
where it is implemented in FrameView and PaintLayerScrollableArea
along with other similar Convert* methods.

Renamed FrameView::scrollbars_ to paint_scrollbars.
Custom PaintLayerScrollableArea scrollbars must receive
StyleUpdate calls when frame comes in and out of focus.
The scrollbars attached to FrameView already receive these via
RecalculateCustomScrollbarStyle, so they do not need to be
stored in FrameView::scrollbars_.

LayoutScrollbar was relying on overriding and listening for
SetParent(nullptr) to dispose.  This is now done by overriding
Scrollbar::DisconnectFromScrollableArea.

Changed PlatformChromeClient::ViewportToScreen to use forward-declared class
FrameView rather than FrameViewBase.  If this change feels like it is
pushing the boundaries of having platform depend on core, then I suggest that
a new class platform/frame/PlatformFrameView be created which can be a base
class of FrameView.

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

to

==========
Deleted Widget/FrameViewBase.

Scrollbar has no need for a 'parent' concept.  It already has
ScrollableArea which is the container of scrollbars.

Moved FrameViewBase::ConvertFromRootFrame method to ScrollableArea
where it is implemented in FrameView and PaintLayerScrollableArea
along with other similar Convert* methods.

Renamed FrameView::scrollbars_ to paint_scrollbars_.
Custom PaintLayerScrollableArea scrollbars must receive
StyleUpdate calls when frame comes in and out of focus.
The scrollbars attached to FrameView already receive these via
RecalculateCustomScrollbarStyle, so they do not need to be
stored in FrameView::scrollbars_.

LayoutScrollbar was relying on overriding and listening for
SetParent(nullptr) to dispose.  This is now done by overriding
Scrollbar::DisconnectFromScrollableArea.

Changed PlatformChromeClient::ViewportToScreen to use forward-declared class
FrameView rather than FrameViewBase.  If this change feels like it is
pushing the boundaries of having platform depend on core, then I suggest that
a new class platform/frame/PlatformFrameView be created which can be a base
class of FrameView.

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

[commit-bot: I haz the power, 2017-05-03 06:59:11]

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

[commit-bot: I haz the power, 2017-05-03 06:59:12]

Dry run: This issue passed the CQ dry run.

[dcheng, 2017-05-09 08:05:55]

Regarding the layering violation, I do think that forward-declaring a core class
in platform is a layering violation. I guess I'm OK with PlatformFrameView since
we already have a precedent for that in PlatformChromeClient. haraken@, do you
know of anything comparable?

https://codereview.chromium.org/2855523002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/2855523002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/FrameView.cpp:501: if
(frame_->GetDocument()->GetStyleEngine().UsesWindowInactiveSelector()) {
Ditto: a separate CL for small cleanups like this helps to separate functional
from cleanup changes.

https://codereview.chromium.org/2855523002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/FrameView.h (right):

https://codereview.chromium.org/2855523002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/FrameView.h:501: void
RemovePaintScrollbar(Scrollbar*);
Is the term "PaintScrollbar" used elsewhere in Chrome code as a noun?
It's not clear to me what a custom PaintLayer scrollbar is.

https://codereview.chromium.org/2855523002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLPlugInElement.cpp (right):

https://codereview.chromium.org/2855523002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLPlugInElement.cpp:80: //
needs_plugin_update(!createdByParser) allows HTMLObjectElement to delay
Move these comment fixes to a separate CL? In git, it's pretty to separate
things like this with judicious use of git add -p.

[haraken, 2017-05-09 11:37:31]

On 2017/05/09 08:05:55, dcheng wrote:
> Regarding the layering violation, I do think that forward-declaring a core
class
> in platform is a layering violation. I guess I'm OK with PlatformFrameView
since
> we already have a precedent for that in PlatformChromeClient. haraken@, do you
> know of anything comparable?

I don't know anything else comparable but creating PlatformFrameView makes
sense.

[joelhockey, 2017-05-10 00:23:00]

On 2017/05/09 at 11:37:31, haraken wrote:
> On 2017/05/09 08:05:55, dcheng wrote:
> > Regarding the layering violation, I do think that forward-declaring a core
class
> > in platform is a layering violation. I guess I'm OK with PlatformFrameView
since
> > we already have a precedent for that in PlatformChromeClient. haraken@, do
you
> > know of anything comparable?
> 
> I don't know anything else comparable but creating PlatformFrameView makes
sense.

Created PlatformLocalFrame in http://crrev.com/2868173002.

Callers and implementations of the 2 PlatformChromeClient methods
ViewportToScreen and ScheduleAnimations are interested in LocalFrame rather than
FrameView.

[joelhockey, 2017-05-10 04:17:03]

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

[joelhockey, 2017-05-10 04:17:13]

https://codereview.chromium.org/2855523002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/2855523002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/FrameView.cpp:501: if
(frame_->GetDocument()->GetStyleEngine().UsesWindowInactiveSelector()) {
On 2017/05/09 at 08:05:55, dcheng wrote:
> Ditto: a separate CL for small cleanups like this helps to separate functional
from cleanup changes.

reverted

https://codereview.chromium.org/2855523002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/FrameView.h (right):

https://codereview.chromium.org/2855523002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/FrameView.h:501: void
RemovePaintScrollbar(Scrollbar*);
On 2017/05/09 at 08:05:55, dcheng wrote:
> Is the term "PaintScrollbar" used elsewhere in Chrome code as a noun?
> It's not clear to me what a custom PaintLayer scrollbar is.

No, I don't believe PaintScrollbar is used elsewhere.  PaintLayer Scrollbar is
to Scrollbar as PaintLayerScrollableArea is to ScrollableArea.

PaintLayerScrollableArea (PLSA) and ScrollableArea are parallel classes which
are containers of scrollbars.  See http://bit.ly/root-layer-scrolling for more
info.

It turns out that FrameView has been holding the scrollbars (horizontal and / or
vertical) of both ScrollableArea (itself) and PLSA.  When there are scrollbars
with custom CSS, then all (up to 4) scrollbars need to be notified when focus
changes.  The 2 scrollbars that are part of ScrollableArea already get looked
after in FrameView::RecalculateCustomScrollbarStyle, so the old
FrameView::scrollbars_ container only really needs to hold references to the 2
scrollbars in PLSA.

So these names could be:

AddPaintLayerScrollableAreaScrollbar
RemovePaintLayerScrollableAreaScrollbar

But I went with the shorter:

AddPaintScrollbar
RemovePaintScrollbar

I don't mind changing the name to anything else.

Ideally, this parallel scrollbar container setup wouldn't exist, and there are
plans to remove it.

https://codereview.chromium.org/2855523002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLPlugInElement.cpp (right):

https://codereview.chromium.org/2855523002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLPlugInElement.cpp:80: //
needs_plugin_update(!createdByParser) allows HTMLObjectElement to delay
On 2017/05/09 at 08:05:55, dcheng wrote:
> Move these comment fixes to a separate CL? In git, it's pretty to separate
things like this with judicious use of git add -p.

reverted

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

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-05-10 05:21:44]

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

[commit-bot: I haz the power, 2017-05-10 05:21:45]

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_...)

[dcheng, 2017-05-10 07:27:00]

I assume this will be rebased this on top of the PlatformFrameView CL once that
lands?

https://codereview.chromium.org/2855523002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/FrameView.h (right):

https://codereview.chromium.org/2855523002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/FrameView.h:501: void
RemovePaintScrollbar(Scrollbar*);
On 2017/05/10 04:17:13, joelhockey wrote:
> On 2017/05/09 at 08:05:55, dcheng wrote:
> > Is the term "PaintScrollbar" used elsewhere in Chrome code as a noun?
> > It's not clear to me what a custom PaintLayer scrollbar is.
> 
> No, I don't believe PaintScrollbar is used elsewhere.  PaintLayer Scrollbar is
> to Scrollbar as PaintLayerScrollableArea is to ScrollableArea.
> 
> PaintLayerScrollableArea (PLSA) and ScrollableArea are parallel classes which
> are containers of scrollbars.  See http://bit.ly/root-layer-scrolling for more
> info.
> 
> It turns out that FrameView has been holding the scrollbars (horizontal and /
or
> vertical) of both ScrollableArea (itself) and PLSA.  When there are scrollbars
> with custom CSS, then all (up to 4) scrollbars need to be notified when focus
> changes.  The 2 scrollbars that are part of ScrollableArea already get looked
> after in FrameView::RecalculateCustomScrollbarStyle, so the old
> FrameView::scrollbars_ container only really needs to hold references to the 2
> scrollbars in PLSA.
> 
> So these names could be:
> 
> AddPaintLayerScrollableAreaScrollbar
> RemovePaintLayerScrollableAreaScrollbar
> 
> But I went with the shorter:
> 
> AddPaintScrollbar
> RemovePaintScrollbar
> 
> I don't mind changing the name to anything else.
> 
> Ideally, this parallel scrollbar container setup wouldn't exist, and there are
> plans to remove it.

Unless I'm missing something, I think the original names might be the clearest.
The caller can pass in a scrollbar created by LayoutScrollbar::Create() or
Scrollbar::Create(): neither of these mentions "Paint", so it's not clear how
"Paint" relates (I guess it's an implementation detail in this case, but not a
particularly obvious one?)

[joelhockey, 2017-05-10 09:54:04]

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

[commit-bot: I haz the power, 2017-05-10 09:54:38]

Dry run: CQ is trying da patch.

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

[joelhockey, 2017-05-10 09:55:34]

On 2017/05/10 at 07:27:00, dcheng wrote:
> I assume this will be rebased this on top of the PlatformFrameView CL once
that lands?
yes

 
> Unless I'm missing something, I think the original names might be the
clearest. The caller can pass in a scrollbar created by
LayoutScrollbar::Create() or Scrollbar::Create(): neither of these mentions
"Paint", so it's not clear how "Paint" relates (I guess it's an implementation
detail in this case, but not a particularly obvious one?)

I've reverted the names

[commit-bot: I haz the power, 2017-05-10 09:59:10]

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

[commit-bot: I haz the power, 2017-05-10 09:59:11]

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
  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...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  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_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[joelhockey, 2017-05-10 10:02:49]

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

[commit-bot: I haz the power, 2017-05-10 10:03:37]

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-05-10 11:37:49]

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

[commit-bot: I haz the power, 2017-05-10 11:37:51]

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-05-11 06:54:38]

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

[commit-bot: I haz the power, 2017-05-11 06:55:10]

Dry run: CQ is trying da patch.

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

[joelhockey, 2017-05-11 06:55:37]

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

[commit-bot: I haz the power, 2017-05-11 06:56:11]

Dry run: CQ is trying da patch.

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

[joelhockey, 2017-05-11 07:01:14]

On 2017/05/10 at 09:55:34, joelhockey wrote:
> On 2017/05/10 at 07:27:00, dcheng wrote:
> > I assume this will be rebased this on top of the PlatformFrameView CL once
that lands?
> yes

Now rebased.

dcheng@ ptal

[joelhockey, 2017-05-11 07:04:49]

Description was changed from

==========
Deleted Widget/FrameViewBase.

Scrollbar has no need for a 'parent' concept.  It already has
ScrollableArea which is the container of scrollbars.

Moved FrameViewBase::ConvertFromRootFrame method to ScrollableArea
where it is implemented in FrameView and PaintLayerScrollableArea
along with other similar Convert* methods.

Renamed FrameView::scrollbars_ to paint_scrollbars_.
Custom PaintLayerScrollableArea scrollbars must receive
StyleUpdate calls when frame comes in and out of focus.
The scrollbars attached to FrameView already receive these via
RecalculateCustomScrollbarStyle, so they do not need to be
stored in FrameView::scrollbars_.

LayoutScrollbar was relying on overriding and listening for
SetParent(nullptr) to dispose.  This is now done by overriding
Scrollbar::DisconnectFromScrollableArea.

Changed PlatformChromeClient::ViewportToScreen to use forward-declared class
FrameView rather than FrameViewBase.  If this change feels like it is
pushing the boundaries of having platform depend on core, then I suggest that
a new class platform/frame/PlatformFrameView be created which can be a base
class of FrameView.

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

to

==========
Deleted Widget/FrameViewBase.

Scrollbar has no need for a 'parent' concept.  It already has
ScrollableArea which is the container of scrollbars.

Moved FrameViewBase::ConvertFromRootFrame method to ScrollableArea
where it is implemented in FrameView and PaintLayerScrollableArea
along with other similar Convert* methods.

Custom PaintLayerScrollableArea scrollbars must receive
StyleUpdate calls when frame comes in and out of focus.
The scrollbars attached to FrameView already receive these via
RecalculateCustomScrollbarStyle, so they do not need to be
stored in FrameView::scrollbars_.

LayoutScrollbar was relying on overriding and listening for
SetParent(nullptr) to dispose.  This is now done by overriding
Scrollbar::DisconnectFromScrollableArea.

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

[commit-bot: I haz the power, 2017-05-11 10:58:50]

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

[commit-bot: I haz the power, 2017-05-11 10:58:51]

Dry run: This issue passed the CQ dry run.

[esprehn, 2017-05-11 20:33:46]

esprehn@chromium.org changed reviewers:
- dcheng@chromium.org, haraken@chromium.org, szager@chromium.org

[esprehn, 2017-05-11 20:33:57]

esprehn@chromium.org changed reviewers:
+ dcheng@chromium.org, haraken@chromium.org, szager@chromium.org

[dcheng, 2017-05-11 22:23:30]

LGTM!

[joelhockey, 2017-05-11 23:40:39]

The CQ bit was checked by joelhockey@chromium.org

[commit-bot: I haz the power, 2017-05-11 23:42:07]

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

[commit-bot: I haz the power, 2017-05-11 23:42:09]

This CL has an open dependency (Issue 2845583002 Patch 140001). Please resolve
the dependency and try again.
If you are sure that there is no real dependency, please use one of the options
listed in https://goo.gl/9Es4OR to land the CL.

[joelhockey, 2017-05-11 23:44:13]

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

[commit-bot: I haz the power, 2017-05-11 23:44:54]

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-05-12 00:04:39]

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

[commit-bot: I haz the power, 2017-05-12 00:04:41]

Dry run: This issue passed the CQ dry run.

[joelhockey, 2017-05-12 04:13:46]

haraken@, ptal. I need owners approval for platform.

[joelhockey, 2017-05-12 04:53:06]

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

[commit-bot: I haz the power, 2017-05-12 04:53:25]

Dry run: CQ is trying da patch.

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

[haraken, 2017-05-12 05:10:00]

Sorry about the review delay. LGTM!

[joelhockey, 2017-05-12 05:23:36]

The CQ bit was unchecked by joelhockey@chromium.org

[joelhockey, 2017-05-12 05:23:39]

The CQ bit was checked by joelhockey@chromium.org

[joelhockey, 2017-05-12 05:23:40]

The patchset sent to the CQ was uploaded after l-g-t-m from dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2855523002/#ps260001
(title: "removed FrameViewBase.h file now that dependent CL submitted")

[commit-bot: I haz the power, 2017-05-12 05:24:03]

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-05-12 06:39:25]

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

[commit-bot: I haz the power, 2017-05-12 06:39:27]

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

[joelhockey, 2017-05-12 06:39:59]

The CQ bit was checked by joelhockey@chromium.org

[commit-bot: I haz the power, 2017-05-12 06:40:20]

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-05-12 08:03:09]

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

[commit-bot: I haz the power, 2017-05-12 08:03:11]

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

[joelhockey, 2017-05-12 09:33:59]

The CQ bit was checked by joelhockey@chromium.org

[commit-bot: I haz the power, 2017-05-12 09:34:25]

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-05-12 10:54:26]

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

[commit-bot: I haz the power, 2017-05-12 10:54:28]

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

[joelhockey, 2017-05-12 10:54:40]

The CQ bit was checked by joelhockey@chromium.org

[commit-bot: I haz the power, 2017-05-12 10:54:58]

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-05-12 16:02:52]

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

[commit-bot: I haz the power, 2017-05-12 16:02:54]

Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[joelhockey, 2017-05-12 21:03:06]

The CQ bit was checked by joelhockey@chromium.org

[commit-bot: I haz the power, 2017-05-12 21:04: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, 2017-05-12 23:44:45]

CQ is committing da patch.

Bot data: {"patchset_id": 260001, "attempt_start_ts": 1494622986338070,
"parent_rev": "00627245b117b8e2d83a5272fc26222765882323", "commit_rev":
"4838cef544c67d342c418d208e52cd8a4cf1a3d2"}

[commit-bot: I haz the power, 2017-05-12 23:45:05]

Description was changed from

==========
Deleted Widget/FrameViewBase.

Scrollbar has no need for a 'parent' concept.  It already has
ScrollableArea which is the container of scrollbars.

Moved FrameViewBase::ConvertFromRootFrame method to ScrollableArea
where it is implemented in FrameView and PaintLayerScrollableArea
along with other similar Convert* methods.

Custom PaintLayerScrollableArea scrollbars must receive
StyleUpdate calls when frame comes in and out of focus.
The scrollbars attached to FrameView already receive these via
RecalculateCustomScrollbarStyle, so they do not need to be
stored in FrameView::scrollbars_.

LayoutScrollbar was relying on overriding and listening for
SetParent(nullptr) to dispose.  This is now done by overriding
Scrollbar::DisconnectFromScrollableArea.

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

to

==========
Deleted Widget/FrameViewBase.

Scrollbar has no need for a 'parent' concept.  It already has
ScrollableArea which is the container of scrollbars.

Moved FrameViewBase::ConvertFromRootFrame method to ScrollableArea
where it is implemented in FrameView and PaintLayerScrollableArea
along with other similar Convert* methods.

Custom PaintLayerScrollableArea scrollbars must receive
StyleUpdate calls when frame comes in and out of focus.
The scrollbars attached to FrameView already receive these via
RecalculateCustomScrollbarStyle, so they do not need to be
stored in FrameView::scrollbars_.

LayoutScrollbar was relying on overriding and listening for
SetParent(nullptr) to dispose.  This is now done by overriding
Scrollbar::DisconnectFromScrollableArea.

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

Review-Url: https://codereview.chromium.org/2855523002
Cr-Commit-Position: refs/heads/master@{#471494}
Committed:
https://chromium.googlesource.com/chromium/src/+/4838cef544c67d342c418d208e52...
==========

[commit-bot: I haz the power, 2017-05-12 23:45:08]

Committed patchset #14 (id:260001) as
https://chromium.googlesource.com/chromium/src/+/4838cef544c67d342c418d208e52...