Remove FrameViewBase as base class of RemoteFrameView.

Remove FrameViewBase as base class of RemoteFrameView.

FrameView now holds separate children for Scrollbars, and a combined
FrameOrPlugin set of children which includes FrameView, PluginView,
and RemoteFrameView.

HTMLFrameOwnerElement::widget_ is of type FrameOrPlugin and holds either
FrameView, PluginView, or RemoteFrameView.  I have pulled the plugin back
from HTMLPlugInObject to be in a single field which simplifies the code
overall.

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

Review-Url: https://codereview.chromium.org/2845583002
Cr-Commit-Position: refs/heads/master@{#471225}
Committed: https://chromium.googlesource.com/chromium/src/+/0f82aa916a39eb944f388c1ae27435036cade852

[joelhockey, 2017-04-26 12:39:07]

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

[commit-bot: I haz the power, 2017-04-26 12:39:15]

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-26 13:23:41]

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

[commit-bot: I haz the power, 2017-04-26 13:23:42]

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

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

[joelhockey, 2017-04-26 13:26:08]

Previously where FrameViewBase was used for polymorphism between FrameView,
RemoteFrameView and PluginView, now each of the objects are handled separately
within FrameView and HTMLFrameObjectElement/HTMLPlugInElement.

FrameViewBase now serves a focused purpose for Scrollbars to set a parent and
subsequently receive callbacks when style is changed, and to calculate some
geometry.

I think this CL improves the code by making it more explicit about where and how
FrameView, RemoteFrameView and PluginView are different, but it is a cost of
having some amount of duplication.

I don't generally like the style of having a base class with IsXXX methods and
then doing downcasts.  This CL has removed just about all cases of this.  In
particular FrameOrPlugin does not have any IsXXX methods.  Again, this comes at
the cost of having duplication where some similarities between the classes could
make use of polymorphism.

So, I guess I'm trying to say that I feel happy enough about the trade-offs, but
I also see how others might take a different approach, and I'm open to hear any
suggestions.

The next steps from here:
1/ Move ScrollbarManager to platform and replace FrameViewBase with it
2/ Rename of classes to *LayoutController
3/ See how I can remove FrameView::parent and use FrameTree.
4/ About half a dozen other smaller changes or TODOs to fix

[joelhockey, 2017-04-26 13:29:14]

Description was changed from

==========
Remove FrameViewBase as base class of RemoteFrameView.

FrameView now holds separate children for RemoteFrameView.

HTMLFrameOwnerElement holds separate objects for FrameView
(named as widget_, still to be renamed), and RemoveFrameView.

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

to

==========
Remove FrameViewBase as base class of RemoteFrameView.

FrameView now holds separate children for RemoteFrameView.

HTMLFrameOwnerElement holds separate objects for FrameView
(named as widget_, still to be renamed), and RemoteFrameView.

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

[dcheng, 2017-04-26 13:31:22]

On 2017/04/26 13:26:08, joelhockey wrote:
> Previously where FrameViewBase was used for polymorphism between FrameView,
> RemoteFrameView and PluginView, now each of the objects are handled separately
> within FrameView and HTMLFrameObjectElement/HTMLPlugInElement.
> 
> FrameViewBase now serves a focused purpose for Scrollbars to set a parent and
> subsequently receive callbacks when style is changed, and to calculate some
> geometry.
> 
> I think this CL improves the code by making it more explicit about where and
how
> FrameView, RemoteFrameView and PluginView are different, but it is a cost of
> having some amount of duplication.
> 
> I don't generally like the style of having a base class with IsXXX methods and
> then doing downcasts.  This CL has removed just about all cases of this.  In
> particular FrameOrPlugin does not have any IsXXX methods.  Again, this comes
at
> the cost of having duplication where some similarities between the classes
could
> make use of polymorphism.

Well, arguably the way to avoid that is to just use virtual functions, rather
than duplicating code. In particular, I'm opposed to the complication for frame
owner's SetWidget / SetRemoteFrameView. We don't have to distinguish like this
for frames themselves, and we shouldn't need to for FrameView/RemoteFrameView.

> 
> So, I guess I'm trying to say that I feel happy enough about the trade-offs,
but
> I also see how others might take a different approach, and I'm open to hear
any
> suggestions.

I would prefer to just see us wholly remove children/parent for
FrameView/RemoteFrameView and depend on the FrameTree, rather than investing
further effort here.
I think it's fine to handle plugins/scrollbars separately.

> 
> The next steps from here:
> 1/ Move ScrollbarManager to platform and replace FrameViewBase with it
> 2/ Rename of classes to *LayoutController
> 3/ See how I can remove FrameView::parent and use FrameTree.
> 4/ About half a dozen other smaller changes or TODOs to fix

[szager1, 2017-04-26 16:15:54]

On 2017/04/26 13:31:22, dcheng (OOO through May 2) wrote:
> On 2017/04/26 13:26:08, joelhockey wrote:
> > Previously where FrameViewBase was used for polymorphism between FrameView,
> > RemoteFrameView and PluginView, now each of the objects are handled
separately
> > within FrameView and HTMLFrameObjectElement/HTMLPlugInElement.
> > 
> > FrameViewBase now serves a focused purpose for Scrollbars to set a parent
and
> > subsequently receive callbacks when style is changed, and to calculate some
> > geometry.
> > 
> > I think this CL improves the code by making it more explicit about where and
> how
> > FrameView, RemoteFrameView and PluginView are different, but it is a cost of
> > having some amount of duplication.
> > 
> > I don't generally like the style of having a base class with IsXXX methods
and
> > then doing downcasts.  This CL has removed just about all cases of this.  In
> > particular FrameOrPlugin does not have any IsXXX methods.  Again, this comes
> at
> > the cost of having duplication where some similarities between the classes
> could
> > make use of polymorphism.
> 
> Well, arguably the way to avoid that is to just use virtual functions, rather
> than duplicating code. In particular, I'm opposed to the complication for
frame
> owner's SetWidget / SetRemoteFrameView. We don't have to distinguish like this
> for frames themselves, and we shouldn't need to for FrameView/RemoteFrameView.
> 
> > 
> > So, I guess I'm trying to say that I feel happy enough about the trade-offs,
> but
> > I also see how others might take a different approach, and I'm open to hear
> any
> > suggestions.
> 
> I would prefer to just see us wholly remove children/parent for
> FrameView/RemoteFrameView and depend on the FrameTree, rather than investing
> further effort here.
> I think it's fine to handle plugins/scrollbars separately.
> 
> > 
> > The next steps from here:
> > 1/ Move ScrollbarManager to platform and replace FrameViewBase with it
> > 2/ Rename of classes to *LayoutController
> > 3/ See how I can remove FrameView::parent and use FrameTree.
> > 4/ About half a dozen other smaller changes or TODOs to fix

I agree with dcheng here.  The most important thing, I think, is your #3: doing
all parent/child traversals through the Frame tree.

And just my $.02 -- I'd prefer not to rename things to *LayoutController.  It's
verbose and particularly self-documenting; and changing the name of FrameView
will be a lot of churn in the code for (what I consider to be) little benefit.

[szager1, 2017-04-26 16:16:18]

> And just my $.02 -- I'd prefer not to rename things to *LayoutController. 
It's
> verbose and particularly self-documenting; and changing the name of FrameView
> will be a lot of churn in the code for (what I consider to be) little benefit.

I meant "*not* particularly self-documenting."

[haraken, 2017-04-27 01:11:00]

Just to clarify, regarding replacing the parent_ fields of
LocalFrameView/RemoteFrameView with the Frame tree, I'm totally agreeing with
it. (That's actually one of the goals of removing the Widget tree.)

[joelhockey, 2017-04-28 09:26:52]

Description was changed from

==========
Remove FrameViewBase as base class of RemoteFrameView.

FrameView now holds separate children for RemoteFrameView.

HTMLFrameOwnerElement holds separate objects for FrameView
(named as widget_, still to be renamed), and RemoteFrameView.

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

to

==========
Remove FrameViewBase as base class of RemoteFrameView.

FrameView now holds separate children for RemoteFrameView.

HTMLFrameOwnerElement holds separate objects for FrameView
(named as widget_, still to be renamed), and RemoteFrameView.

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

[joelhockey, 2017-04-28 09:33:44]

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

[joelhockey, 2017-04-28 09:33:45]

Thanks for the review dcheng@, szager@ and haraken@.

This CL is still primarily about removing FrameViewBase as parent of
RemoteFrameView.
But this has quite a few knock-on effects.

I think this time, I have been able to combine FrameView, RemoteFrame and
PluginView
as FrameOrPlugin children of FrameView, and I have done similar for widget_ in
HTMLFrameOwnerElement.  I like the result.

Next step is to look at using FrameTree.

I don't love the names FrameViewBase or FrameOrPlugin.  Haraken has made
suggestions for
alternative, but I agree that the suggested names are long and not particularly
descriptive,
so I will leave any renamings until there is broader agreement.

[joelhockey, 2017-04-28 09:33:50]

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

[commit-bot: I haz the power, 2017-04-28 09:34:16]

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-28 10:34:25]

On 2017/04/28 09:33:45, joelhockey wrote:
> Thanks for the review dcheng@, szager@ and haraken@.
> 
> This CL is still primarily about removing FrameViewBase as parent of
> RemoteFrameView.
> But this has quite a few knock-on effects.
> 
> I think this time, I have been able to combine FrameView, RemoteFrame and
> PluginView
> as FrameOrPlugin children of FrameView, and I have done similar for widget_ in
> HTMLFrameOwnerElement.  I like the result.
> 
> Next step is to look at using FrameTree.
> 
> I don't love the names FrameViewBase or FrameOrPlugin.  Haraken has made
> suggestions for
> alternative, but I agree that the suggested names are long and not
particularly
> descriptive,
> so I will leave any renamings until there is broader agreement.

(I'll defer the review to dcheng@ and szager@.)

[szager1, 2017-04-28 11:10:01]

Do you have a design doc or diagram anywhere that shows the intended class
hierarchy of your end state?  It's hard for me to evaluate these incremental
changes without understanding what your end goal is.

My first instinct is that FrameView and RemoteFrameView should have a common
ancestor.  I think that will make OOPIF code easier to write, since some code
will be able to use FrameViewBase without knowing or caring whether it's local
or remote.

[commit-bot: I haz the power, 2017-04-28 11:39:34]

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

[commit-bot: I haz the power, 2017-04-28 11:39:35]

Dry run: 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-04-28 12:27:18]

On 2017/04/28 at 11:10:01, szager wrote:
> Do you have a design doc or diagram anywhere that shows the intended class
hierarchy of your end state?  It's hard for me to evaluate these incremental
changes without understanding what your end goal is.

Sorry, I don't have a design doc or diagram.  I'm also struggling a bit through
this code for lack of a clear end state understanding, but I'm trying to keep
the CLs fairly small and contained to a single change and give some context in
descriptions and messages about what I see as next steps.  This is my starter
project on blink, and I'm only gaining familiarity with the code as I go.  The
clearest description that I am following is in the attached bug.  This was
written by wkorman and mentions a discussion with you.  http://crbug.com/637460.

=====
1) Make Widget::m_parent be type Member<FrameView>
2) Add m_childPlugins, m_childScrollbars to FrameView.
3) Remove Widget inheritance from PluginView and Scrollbar.
4) Remove Widget inheritance from FrameView.
...
N) Remove m_frame from FrameView, delegate to LayoutView
  - Is FrameView::location() ever non-zero?
N+1) Cull extraneous methods from FrameView.
=====

My work originally started as a rename from Widget to FrameViewBase, but after a
chat session with you, and being pointed at the attached bug, I realised that
removing Widget/FrameViewBase inheritance from PluginView, Scrollbar, and
RemoteFrameView would be an improvement.  I have already removed the inheritance
for PluginView, and Scrollbar.  This CL removes inheritance for RemoteFrameView
and completes step 4 (kind of - it turns out that FrameView must inherit from
some class, currently FrameViewBase which lives in platform and acts as parent
for scrollbar, so whilst FrameViewBase/Widget may get another rename, the
inheritance must remain).

> 
> My first instinct is that FrameView and RemoteFrameView should have a common
ancestor.  I think that will make OOPIF code easier to write, since some code
will be able to use FrameViewBase without knowing or caring whether it's local
or remote.

FrameOrPlugin is currently a common ancestor, although it is intentionally a
pure abstract class.  As I've looked at RemoteFrameView I was at first surprised
that it did not extend FrameView, but I see now that it is a small class and has
close to zero code overlap with FrameView, so I think it is good that they do
not share any implementation inheritance.  If things change and the 2 classes
have common code, then I would suggest that FrameView is renamed to
LocalFrameView, and then a new FrameView can be common to both.

Current state:

FrameOrPlugin
 - PluginView
 - FrameView (also extends platform/FrameViewBase)
 - RemoteFrameView

Possible future state:

FrameOrPlugin
 - PluginView
 - FrameView
    - LocalFrameView (also extends platform/FrameViewBase)
    - RemoteFrameView

[joelhockey, 2017-04-28 12:44:32]

> Sorry, I don't have a design doc or diagram.

I just thought to mention again, that for my next steps right now, I see it as
step N (remove widget tree), and maybe N+1 (remove extraneous methods if any). 
I also have some smaller cleanup things to look into that I have kept notes on.

I would offer to write a design doc, but to be honest, I don't really know
things like what the widget tree is, or what FrameTree is, or what "delegate to
LayoutView" really means right now.  I expect to find out next week as I
continue on this.  If I was to write up a little about what I've done so far, it
would be:

Original state:

platform/Widget
 - platform/Scrollbar (has Widget for parent)
 - core/plugins/PluginView
 - core/frame/FrameView (contains Widget children)
 - core/frame/RemoteFrameView

Current state:

platform/Scrollbar
  = Has FrameViewBase as parent field
  = no inheritance to/from other classes mentioned here

* platform/FrameViewBase
  = Renamed from Widget
  = This exists since platform cannot depend on core
  = Some class in core - FrameView - must inherit from this and be parent to
scrollbar.
  = This class is as small as possible - it has half a dozen virtual methods,
    and a single ConvertFromRootFrame method which is required by Scrollbar

* core/frame/FrameOrPlugin (pure interface)
  - core/plugins/PluginView
    = pure interface
    = soon web/WebPluginContainerImpl which is the only implementation
      will be merged into this
  - core/frame/FrameView
    = also extends platform/FrameViewBase
    = contains scrollbar children
    = contains FrameOrPlugin children (FrameView, PluginView, RemoteFrameView)
  - core/frame/RemoteFrameView

[joelhockey, 2017-04-28 12:45:12]

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

[commit-bot: I haz the power, 2017-04-28 12:45:31]

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-28 13:10:47]

Description was changed from

==========
Remove FrameViewBase as base class of RemoteFrameView.

FrameView now holds separate children for RemoteFrameView.

HTMLFrameOwnerElement holds separate objects for FrameView
(named as widget_, still to be renamed), and RemoteFrameView.

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

to

==========
Remove FrameViewBase as base class of RemoteFrameView.

FrameView now holds separate children for Scrollbars, and a combined
FrameOrPlugin set of children which includes FrameView, PluginView,
and RemoteFrameView.

HTMLFrameOwnerElement::widget_ is of type FrameOrPlugin and holds either
FrameView, PluginView, or RemoteFrameView.  I have pulled the plugin back
from HTMLPlugInObject to be in a single field which simplifies the code
overall.

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

[commit-bot: I haz the power, 2017-04-28 16:34:28]

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

[commit-bot: I haz the power, 2017-04-28 16:34:28]

Dry run: This issue passed the CQ dry run.

[joelhockey, 2017-05-01 23:04:35]

Maybe these comments are helpful when reviewing.

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

https://codereview.chromium.org/2845583002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/FrameOrPlugin.h:20: class CORE_EXPORT
FrameOrPlugin : public GarbageCollectedMixin {
Methods SetParent, Parent, SetParentVisible, FrameRectsChanged moved from
FrameViewBase to FrameOrPlugin.  These methods are applicable to FrameView,
RemoteFrameView and PluginView.  Parent is now type FrameView rather than
FrameViewBase.

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

https://codereview.chromium.org/2845583002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/FrameView.h:503: void
RemovePlugin(PluginView*);
Scrollbar is still a special-case child of FrameView, but Plugin is not.  All of
the FrameOrPlugin (FrameView, RemoteFrameView and PluginView) types are combined
into children_

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

https://codereview.chromium.org/2845583002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/FrameView.h:650: IntPoint
ConvertSelfToChild(const IntPoint&, const IntPoint&) const;
The method was only calling FrameViewBase::Location, so it is easy for clients
to provide this IntPoint directly.

https://codereview.chromium.org/2845583002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/FrameView.h:666: IntPoint
ConvertFromContainingFrameViewToScrollbar(
I changed a few Convert...FrameViewBase... methods to Convert...FrameView...
This still doesn't feel like a perfect name, but overall an improvement.

I would be happy to shorten even more to something like:

  ConvertFromContainerToScrollbar

Or in cases like this where Scrollbar is provided as an input type, I also like:

  ConvertFromContainer

https://codereview.chromium.org/2845583002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/FrameView.h:1065: // Member<FrameView>
instead.
I've tried to update comments to be more accurate.  Some changes are a guess,
but even if not perfect, at least hopefully things are not worse.

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

https://codereview.chromium.org/2845583002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/RemoteFrameView.h:19: class RemoteFrameView
final : public GarbageCollectedFinalized<RemoteFrameView>,
The main point of this CL is to stop RemoteFrameView from inheriting from
FrameViewBase.  It is nice to see that overall there is a slight reduction in
lines of code and number of methods in this class as a result.

https://codereview.chromium.org/2845583002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLFrameOwnerElement.h (right):

https://codereview.chromium.org/2845583002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLFrameOwnerElement.h:64: void
SetWidget(FrameOrPlugin*);
Widget type has changed from platform/FrameViewBase to core/frame/FrameOrPlugin.
I still plan to change the name of this method to something other than 'Widget'
once I think of something better.  Right now 'FrameOrPlugin' is the leading
candidate.

Previously I moved PluginView to be stored on HTMLPlugInElement.  I have now
moved it back to be part of the single 'widget_' field in HTMLFrameOwnerElement.
 Having the single field simplifies the overall code, particularly the attach
and disposal.

I still make a distinction in LayoutPart class where callers can request only
the PluginView, or only the FrameView, or for FrameOrPlugin which may be any of
[Frame|RemoteFrame|Plugin]View.

https://codereview.chromium.org/2845583002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLPlugInElement.h (left):

https://codereview.chromium.org/2845583002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLPlugInElement.h:169: PluginView*
ReleasePlugin();
These specialized PluginView methods are no longer needed.  PluginView has gone
back to being stored in HTMLFrameOwnerElement::widget_

https://codereview.chromium.org/2845583002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/PluginDocument.h (right):

https://codereview.chromium.org/2845583002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/PluginDocument.h:59:
Member<HTMLPlugInElement> plugin_node_;
plugin_node_ is always HTMLPlugInElement

https://codereview.chromium.org/2845583002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/FrameViewBase.h (left):

https://codereview.chromium.org/2845583002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/FrameViewBase.h:51: virtual void
SetParent(FrameViewBase*) = 0;
SetParent, SetParentVisible, FrameRectsChanged methods moved to FrameOrPlugin
interface.

[joelhockey, 2017-05-03 01:40: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:41:26]

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 03:39:02]

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

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

Dry run: This issue passed the CQ dry run.

[dcheng, 2017-05-09 07:50:51]

Sorry for the extreme latency. I'm fine with the general direction of this CL

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

https://codereview.chromium.org/2845583002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/FrameView.h:650: IntPoint
ConvertSelfToChild(const IntPoint&, const IntPoint&) const;
On 2017/05/01 23:04:35, joelhockey wrote:
> The method was only calling FrameViewBase::Location, so it is easy for clients
> to provide this IntPoint directly.

I prefer the original: it's hard to get it wrong if we pass in the
FrameViewBase* and call Location().

https://codereview.chromium.org/2845583002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/FrameView.h:666: IntPoint
ConvertFromContainingFrameViewToScrollbar(
On 2017/05/01 23:04:35, joelhockey wrote:
> I changed a few Convert...FrameViewBase... methods to Convert...FrameView...
> This still doesn't feel like a perfect name, but overall an improvement.
> 
> I would be happy to shorten even more to something like:
> 
>   ConvertFromContainerToScrollbar
> 
> Or in cases like this where Scrollbar is provided as an input type, I also
like:
> 
>   ConvertFromContainer

For future CLs, let's separate out renaming changes to keep the CLs easier to
review (since the widget code is not very well understood, it helps to simplify
the CLs for reviewers as much as possible)

https://codereview.chromium.org/2845583002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLFrameOwnerElement.h (right):

https://codereview.chromium.org/2845583002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLFrameOwnerElement.h:64: void
SetWidget(FrameOrPlugin*);
On 2017/05/01 23:04:35, joelhockey wrote:
> Widget type has changed from platform/FrameViewBase to
core/frame/FrameOrPlugin.
> I still plan to change the name of this method to something other than
'Widget'
> once I think of something better.  Right now 'FrameOrPlugin' is the leading
> candidate.

FrameOrPlugin doesn't really help readers understand the purpose of the class
though. Readers only know that it has something do with frames or plugins, but
not really what. We probably should have just left it named "Widget" until we
had a clearer picture of what would be left on FrameOrPlugin, but that ship has
already sailed.

My personal suggestion is something along the lines of of
EmbeddedObject/EmbeddedObjectHost/EmbeddedObjectController, but this isn't great
for several reasons:
- It's kind of confusing since we also have LayoutEmbeddedObject. Maybe the Host
or Controller suffix will help disambiguate.
- At the same time, just adding Host or Controller doesn't address my earlier
question of "what does this class do". But calling it EmbeddedObject makes it a
bit clearer what types of objects it's intended for... I think.
- something else?

At the same time, it's a bit more descriptive than Widget, and more accurate
once Scrollbar is no longer in the picture...

All this is a long-winded way of saying: let's leave the names for now, finish
the code cleanup, and then rename classes as appropriate.

> 
> Previously I moved PluginView to be stored on HTMLPlugInElement.  I have now
> moved it back to be part of the single 'widget_' field in
HTMLFrameOwnerElement.
>  Having the single field simplifies the overall code, particularly the attach
> and disposal.
> 
> I still make a distinction in LayoutPart class where callers can request only
> the PluginView, or only the FrameView, or for FrameOrPlugin which may be any
of
> [Frame|RemoteFrame|Plugin]View.

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

https://codereview.chromium.org/2845583002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/FrameView.cpp:1522: // Tell the DOM element
that it needs a FrameView update.
This reads a bit confusing. It says FrameView here, but this method appears to
be dealing with plugins?

https://codereview.chromium.org/2845583002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutPart.cpp (right):

https://codereview.chromium.org/2845583002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutPart.cpp:68: Node* node = GetNode();
A separate cleanup for this would be nice in the future.

https://codereview.chromium.org/2845583002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/plugins/PluginOcclusionSupport.cpp
(right):

https://codereview.chromium.org/2845583002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/plugins/PluginOcclusionSupport.cpp:156: void
GetPluginOcclusions(Element* element,
Btw, no need to block on the removal CL, this is easy to rebase over.

[joelhockey, 2017-05-10 02:35:43]

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

https://codereview.chromium.org/2845583002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/FrameView.h:650: IntPoint
ConvertSelfToChild(const IntPoint&, const IntPoint&) const;
On 2017/05/09 at 07:50:51, dcheng wrote:
> On 2017/05/01 23:04:35, joelhockey wrote:
> > The method was only calling FrameViewBase::Location, so it is easy for
clients
> > to provide this IntPoint directly.
> 
> I prefer the original: it's hard to get it wrong if we pass in the
FrameViewBase* and call Location().

Sorry, my comment didn't make it clear that the reason for the change is that
RemoteFrameView is no longer a FrameViewBase, but it still needs to call this
method.
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/frame/Rem...

https://codereview.chromium.org/2845583002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/FrameView.h:666: IntPoint
ConvertFromContainingFrameViewToScrollbar(
On 2017/05/09 at 07:50:51, dcheng wrote:
> On 2017/05/01 23:04:35, joelhockey wrote:
> > I changed a few Convert...FrameViewBase... methods to Convert...FrameView...
> > This still doesn't feel like a perfect name, but overall an improvement.
> > 
> > I would be happy to shorten even more to something like:
> > 
> >   ConvertFromContainerToScrollbar
> > 
> > Or in cases like this where Scrollbar is provided as an input type, I also
like:
> > 
> >   ConvertFromContainer
> 
> For future CLs, let's separate out renaming changes to keep the CLs easier to
review (since the widget code is not very well understood, it helps to simplify
the CLs for reviewers as much as possible)

I've backed out the renaming for this CL, and I'll do it in a follow-up.  I know
that it might not have made much difference for this review, but to future
people who may still look through blame at this CL, hopefully it makes it easier
for them if I get this CL as simple as possible.

https://codereview.chromium.org/2845583002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLFrameOwnerElement.h (right):

https://codereview.chromium.org/2845583002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLFrameOwnerElement.h:64: void
SetWidget(FrameOrPlugin*);
> All this is a long-winded way of saying: let's leave the names for now, finish
the code cleanup, and then rename classes as appropriate.

agree.  If I knew before what I know now, I would have left the Widget class
named as Widget since it is to be deleted in any case.

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

https://codereview.chromium.org/2845583002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/FrameView.cpp:1522: // Tell the DOM element
that it needs a FrameView update.
On 2017/05/09 at 07:50:51, dcheng wrote:
> This reads a bit confusing. It says FrameView here, but this method appears to
be dealing with plugins?

comment updated

https://codereview.chromium.org/2845583002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutPart.cpp (right):

https://codereview.chromium.org/2845583002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutPart.cpp:68: Node* node = GetNode();
On 2017/05/09 at 07:50:51, dcheng wrote:
> A separate cleanup for this would be nice in the future.

Do you mean fixing the attach / detach / dispose for Widgets?

The work I am doing right now for the WidgetTree removal also involves changing
all the SetWidget(nullptr) calls and trying to make attach, detach, and dispose
clearer.

[joelhockey, 2017-05-10 02:35:56]

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

[commit-bot: I haz the power, 2017-05-10 02:36:31]

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 03:04:54]

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

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

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

[joelhockey, 2017-05-10 03:09: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 03:11:01]

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 04:58:50]

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

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

Dry run: Try jobs failed on following builders:
  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...)

[dcheng, 2017-05-10 07:08:28]

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

https://codereview.chromium.org/2845583002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/FrameView.h:650: IntPoint
ConvertSelfToChild(const IntPoint&, const IntPoint&) const;
On 2017/05/10 02:35:43, joelhockey wrote:
> On 2017/05/09 at 07:50:51, dcheng wrote:
> > On 2017/05/01 23:04:35, joelhockey wrote:
> > > The method was only calling FrameViewBase::Location, so it is easy for
> clients
> > > to provide this IntPoint directly.
> > 
> > I prefer the original: it's hard to get it wrong if we pass in the
> FrameViewBase* and call Location().
> 
> Sorry, my comment didn't make it clear that the reason for the change is that
> RemoteFrameView is no longer a FrameViewBase, but it still needs to call this
> method.
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/frame/Rem...

I understand that, but maybe this is an argument for having something common on
FrameView/RemoteFrameView. It could be NOTREACHED() on PluginView for now.
There's at least one other place where it'd be useful to have some methods
shared between FrameView/RemoteFrameView.

Having a FrameViewBase* argument and a const IntPoint& argument is a bit clearer
on what's being converted: the new overload just has two sets of IntPoints, and
it's not 100% clear how the arguments are consumed anymore.

https://codereview.chromium.org/2845583002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutPart.cpp (right):

https://codereview.chromium.org/2845583002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutPart.cpp:68: Node* node = GetNode();
On 2017/05/10 02:35:43, joelhockey wrote:
> On 2017/05/09 at 07:50:51, dcheng wrote:
> > A separate cleanup for this would be nice in the future.
> 
> Do you mean fixing the attach / detach / dispose for Widgets?
> 
> The work I am doing right now for the WidgetTree removal also involves
changing
> all the SetWidget(nullptr) calls and trying to make attach, detach, and
dispose
> clearer.

I just meant that the changes on lines 68-70 are a functional no-op, so aren't
really needed as part of this CL. No need to revert at this point in this CL
though.

[joelhockey, 2017-05-10 09:40:46]

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

https://codereview.chromium.org/2845583002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/FrameView.h:650: IntPoint
ConvertSelfToChild(const IntPoint&, const IntPoint&) const;
On 2017/05/10 at 07:08:28, dcheng wrote:
> On 2017/05/10 02:35:43, joelhockey wrote:
> > On 2017/05/09 at 07:50:51, dcheng wrote:
> > > On 2017/05/01 23:04:35, joelhockey wrote:
> > > > The method was only calling FrameViewBase::Location, so it is easy for
> > clients
> > > > to provide this IntPoint directly.
> > > 
> > > I prefer the original: it's hard to get it wrong if we pass in the
> > FrameViewBase* and call Location().
> > 
> > Sorry, my comment didn't make it clear that the reason for the change is
that
> > RemoteFrameView is no longer a FrameViewBase, but it still needs to call
this
> > method.
> >
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/frame/Rem...
> 
> I understand that, but maybe this is an argument for having something common
on FrameView/RemoteFrameView. It could be NOTREACHED() on PluginView for now.
There's at least one other place where it'd be useful to have some methods
shared between FrameView/RemoteFrameView.
> 
> Having a FrameViewBase* argument and a const IntPoint& argument is a bit
clearer on what's being converted: the new overload just has two sets of
IntPoints, and it's not 100% clear how the arguments are consumed anymore.

I've changed to use the FrameOrPlugin class rather than IntPoint.  The calling
code is now the same except for passing a non-const ref rather than pointer.

https://codereview.chromium.org/2845583002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutPart.cpp (right):

https://codereview.chromium.org/2845583002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutPart.cpp:68: Node* node = GetNode();
On 2017/05/10 at 07:08:28, dcheng wrote:
> On 2017/05/10 02:35:43, joelhockey wrote:
> > On 2017/05/09 at 07:50:51, dcheng wrote:
> > > A separate cleanup for this would be nice in the future.
> > 
> > Do you mean fixing the attach / detach / dispose for Widgets?
> > 
> > The work I am doing right now for the WidgetTree removal also involves
changing
> > all the SetWidget(nullptr) calls and trying to make attach, detach, and
dispose
> > clearer.
> 
> I just meant that the changes on lines 68-70 are a functional no-op, so aren't
really needed as part of this CL. No need to revert at this point in this CL
though.

ack

[joelhockey, 2017-05-10 09:40:59]

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

[szager1, 2017-05-10 09:41:04]

https://codereview.chromium.org/2845583002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLFrameOwnerElement.h (right):

https://codereview.chromium.org/2845583002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLFrameOwnerElement.h:64: void
SetWidget(FrameOrPlugin*);
On 2017/05/10 02:35:43, joelhockey wrote:
> > All this is a long-winded way of saying: let's leave the names for now,
finish
> the code cleanup, and then rename classes as appropriate.
> 
> agree.  If I knew before what I know now, I would have left the Widget class
> named as Widget since it is to be deleted in any case.

My $.02: Widget would still have been a bad name, because it's still way too
generic.

I would opt for something like EmbeddedContentView.  "EmbeddedContent" because
it's embedded content; and "View" for the same reason we use the name FrameView,
i.e., because it's the visible manifestation of a Frame.  Similarly, a
PluginView is the visible manifestation of a plugin, and an EmbeddedContentView
is the visible manifestation of embedded content.

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

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 10:54:30]

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

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

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

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

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

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 14:16:13]

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

[commit-bot: I haz the power, 2017-05-10 14:16:15]

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:33]

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:09]

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-11 08:54:22]

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

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

Dry run: This issue passed the CQ dry run.

[dcheng, 2017-05-11 21:57:12]

LGTM

[joelhockey, 2017-05-11 23:34:12]

The CQ bit was checked by joelhockey@chromium.org

[commit-bot: I haz the power, 2017-05-11 23:34:55]

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:42]

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

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

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

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

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

[haraken, 2017-05-12 04:23:42]

Sorry about the review delay. I'm catching up with things... LGTM.

https://codereview.chromium.org/2845583002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLPlugInElement.h (right):

https://codereview.chromium.org/2845583002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLPlugInElement.h:186: // prevent
confusing code which may assume that OwnedWidget() != null

In my understanding, we should now rename OwnedWidget/SetWidget to
OwnedFrameOrPlugin/SetFrameOrPlugin, right? (You don't need to do it in this CL
though.)

[joelhockey, 2017-05-12 04:40:08]

The CQ bit was checked by joelhockey@chromium.org

[commit-bot: I haz the power, 2017-05-12 04:40:26]

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 04:45:16]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1494564008692870,
"parent_rev": "8ddfef7806bcbedb9582efc385088603b200d2d9", "commit_rev":
"9659e530ddd76377c8cc07a24d9a3d56344902a4"}

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

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1494564008692870,
"parent_rev": "9e9c629332801817315bbeda3b2ac0c055da670f", "commit_rev":
"0f82aa916a39eb944f388c1ae27435036cade852"}

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

Description was changed from

==========
Remove FrameViewBase as base class of RemoteFrameView.

FrameView now holds separate children for Scrollbars, and a combined
FrameOrPlugin set of children which includes FrameView, PluginView,
and RemoteFrameView.

HTMLFrameOwnerElement::widget_ is of type FrameOrPlugin and holds either
FrameView, PluginView, or RemoteFrameView.  I have pulled the plugin back
from HTMLPlugInObject to be in a single field which simplifies the code
overall.

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

to

==========
Remove FrameViewBase as base class of RemoteFrameView.

FrameView now holds separate children for Scrollbars, and a combined
FrameOrPlugin set of children which includes FrameView, PluginView,
and RemoteFrameView.

HTMLFrameOwnerElement::widget_ is of type FrameOrPlugin and holds either
FrameView, PluginView, or RemoteFrameView.  I have pulled the plugin back
from HTMLPlugInObject to be in a single field which simplifies the code
overall.

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

Review-Url: https://codereview.chromium.org/2845583002
Cr-Commit-Position: refs/heads/master@{#471225}
Committed:
https://chromium.googlesource.com/chromium/src/+/0f82aa916a39eb944f388c1ae274...
==========

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

Committed patchset #8 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/0f82aa916a39eb944f388c1ae274...