Remove FrameViewBase as base class of RemoteFrameView. (issue 2845583002 by joelhockey@chromium.org)

2 views
Skip to first unread message

joelh...@chromium.org

unread,
Apr 26, 2017, 9:26:09 AM4/26/17
to har...@chromium.org, dcheng+...@chromium.org, blink-...@chromium.org, blink-revi...@chromium.org, blink-rev...@chromium.org, blink-revi...@chromium.org, chromium...@chromium.org, dglazko...@chromium.org, eae+bli...@chromium.org, har...@chromium.org, jchaffraix...@chromium.org, leviw+re...@chromium.org, pdr+renderi...@chromium.org, szager+la...@chromium.org, zol...@webkit.org
Reviewers: haraken, dcheng (OOO through May 2)
CL: https://codereview.chromium.org/2845583002/

Message:
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


Description:
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

Affected files (+162, -175 lines):
M third_party/WebKit/Source/core/frame/FrameView.h
M third_party/WebKit/Source/core/frame/FrameView.cpp
M third_party/WebKit/Source/core/frame/RemoteFrame.cpp
M third_party/WebKit/Source/core/frame/RemoteFrameView.h
M third_party/WebKit/Source/core/frame/RemoteFrameView.cpp
M third_party/WebKit/Source/core/html/HTMLFrameElementBase.cpp
M third_party/WebKit/Source/core/html/HTMLFrameOwnerElement.h
M third_party/WebKit/Source/core/html/HTMLFrameOwnerElement.cpp
M third_party/WebKit/Source/core/html/HTMLPlugInElement.h
M third_party/WebKit/Source/core/html/HTMLPlugInElement.cpp
M third_party/WebKit/Source/core/layout/LayoutPart.cpp
M third_party/WebKit/Source/modules/plugins/PluginOcclusionSupport.h
M third_party/WebKit/Source/modules/plugins/PluginOcclusionSupport.cpp


dch...@chromium.org

unread,
Apr 26, 2017, 9:31:23 AM4/26/17
to joelh...@chromium.org, har...@chromium.org, blink-...@chromium.org, blink-revi...@chromium.org, blink-rev...@chromium.org, blink-revi...@chromium.org, chromium...@chromium.org, dglazko...@chromium.org, eae+bli...@chromium.org, har...@chromium.org, jchaffraix...@chromium.org, leviw+re...@chromium.org, pdr+renderi...@chromium.org, szager+la...@chromium.org, zol...@webkit.org
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



sza...@chromium.org

unread,
Apr 26, 2017, 12:15:54 PM4/26/17
to joelh...@chromium.org, dcheng+...@chromium.org, har...@chromium.org, blink-...@chromium.org, blink-revi...@chromium.org, blink-rev...@chromium.org, blink-revi...@chromium.org, chromium...@chromium.org, dglazko...@chromium.org, eae+bli...@chromium.org, har...@chromium.org, jchaffraix...@chromium.org, leviw+re...@chromium.org, pdr+renderi...@chromium.org, szager+la...@chromium.org, zol...@webkit.org
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.

https://codereview.chromium.org/2845583002/

sza...@chromium.org

unread,
Apr 26, 2017, 12:16:18 PM4/26/17
to joelh...@chromium.org, dcheng+...@chromium.org, har...@chromium.org, blink-...@chromium.org, blink-revi...@chromium.org, blink-rev...@chromium.org, blink-revi...@chromium.org, chromium...@chromium.org, dglazko...@chromium.org, eae+bli...@chromium.org, har...@chromium.org, jchaffraix...@chromium.org, leviw+re...@chromium.org, pdr+renderi...@chromium.org, szager+la...@chromium.org, zol...@webkit.org
> 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."

https://codereview.chromium.org/2845583002/

har...@chromium.org

unread,
Apr 26, 2017, 9:11:00 PM4/26/17
to joelh...@chromium.org, dcheng+...@chromium.org, blink-...@chromium.org, blink-revi...@chromium.org, blink-rev...@chromium.org, blink-revi...@chromium.org, chromium...@chromium.org, dglazko...@chromium.org, eae+bli...@chromium.org, jchaffraix...@chromium.org, leviw+re...@chromium.org, pdr+renderi...@chromium.org, szager+la...@chromium.org, zol...@webkit.org
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.)


https://codereview.chromium.org/2845583002/

joelh...@chromium.org

unread,
Apr 28, 2017, 5:33:45 AM4/28/17
to dcheng+...@chromium.org, har...@chromium.org, sza...@chromium.org, blink-...@chromium.org, blink-revi...@chromium.org, blink-rev...@chromium.org, blink-revi...@chromium.org, chromium...@chromium.org, dglazko...@chromium.org, eae+bli...@chromium.org, jchaffraix...@chromium.org, leviw+re...@chromium.org, pdr+renderi...@chromium.org, szager+la...@chromium.org, zol...@webkit.org
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.

https://codereview.chromium.org/2845583002/

har...@chromium.org

unread,
Apr 28, 2017, 6:34:25 AM4/28/17
to joelh...@chromium.org, dcheng+...@chromium.org, sza...@chromium.org, blink-...@chromium.org, blink-revi...@chromium.org, blink-rev...@chromium.org, blink-revi...@chromium.org, chromium...@chromium.org, dglazko...@chromium.org, eae+bli...@chromium.org, jchaffraix...@chromium.org, leviw+re...@chromium.org, pdr+renderi...@chromium.org, szager+la...@chromium.org, zol...@webkit.org
(I'll defer the review to dcheng@ and szager@.)


https://codereview.chromium.org/2845583002/

sza...@chromium.org

unread,
Apr 28, 2017, 7:10:02 AM4/28/17
to joelh...@chromium.org, dcheng+...@chromium.org, har...@chromium.org, blink-...@chromium.org, blink-revi...@chromium.org, blink-rev...@chromium.org, blink-revi...@chromium.org, chromium...@chromium.org, dglazko...@chromium.org, eae+bli...@chromium.org, jchaffraix...@chromium.org, leviw+re...@chromium.org, pdr+renderi...@chromium.org, szager+la...@chromium.org, zol...@webkit.org
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.


https://codereview.chromium.org/2845583002/

joelh...@chromium.org

unread,
Apr 28, 2017, 8:27:19 AM4/28/17
to dcheng+...@chromium.org, har...@chromium.org, sza...@chromium.org, blink-...@chromium.org, blink-revi...@chromium.org, blink-rev...@chromium.org, blink-revi...@chromium.org, chromium...@chromium.org, dglazko...@chromium.org, eae+bli...@chromium.org, jchaffraix...@chromium.org, leviw+re...@chromium.org, pdr+renderi...@chromium.org, szager+la...@chromium.org, zol...@webkit.org
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


https://codereview.chromium.org/2845583002/

joelh...@chromium.org

unread,
Apr 28, 2017, 8:44:32 AM4/28/17
to dcheng+...@chromium.org, har...@chromium.org, sza...@chromium.org, blink-...@chromium.org, blink-revi...@chromium.org, blink-rev...@chromium.org, blink-revi...@chromium.org, chromium...@chromium.org, dglazko...@chromium.org, eae+bli...@chromium.org, jchaffraix...@chromium.org, leviw+re...@chromium.org, pdr+renderi...@chromium.org, szager+la...@chromium.org, zol...@webkit.org
> 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

https://codereview.chromium.org/2845583002/

joelh...@chromium.org

unread,
May 1, 2017, 7:04:35 PM5/1/17
to dcheng+...@chromium.org, har...@chromium.org, sza...@chromium.org, blink-...@chromium.org, blink-revi...@chromium.org, blink-rev...@chromium.org, blink-revi...@chromium.org, chromium...@chromium.org, dglazko...@chromium.org, eae+bli...@chromium.org, jchaffraix...@chromium.org, leviw+re...@chromium.org, pdr+renderi...@chromium.org, szager+la...@chromium.org, zol...@webkit.org
Maybe these comments are helpful when reviewing.


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

https://codereview.chromium.org/2845583002/diff/60001/third_party/WebKit/Source/core/frame/FrameOrPlugin.h#newcode20
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/Source/core/frame/FrameView.h
File third_party/WebKit/Source/core/frame/FrameView.h (left):

https://codereview.chromium.org/2845583002/diff/60001/third_party/WebKit/Source/core/frame/FrameView.h#oldcode503
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/Source/core/frame/FrameView.h
File third_party/WebKit/Source/core/frame/FrameView.h (right):

https://codereview.chromium.org/2845583002/diff/60001/third_party/WebKit/Source/core/frame/FrameView.h#newcode650
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/Source/core/frame/FrameView.h#newcode666
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/Source/core/frame/FrameView.h#newcode1065
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/Source/core/frame/RemoteFrameView.h
File third_party/WebKit/Source/core/frame/RemoteFrameView.h (right):

https://codereview.chromium.org/2845583002/diff/60001/third_party/WebKit/Source/core/frame/RemoteFrameView.h#newcode19
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/Source/core/html/HTMLFrameOwnerElement.h
File third_party/WebKit/Source/core/html/HTMLFrameOwnerElement.h
(right):

https://codereview.chromium.org/2845583002/diff/60001/third_party/WebKit/Source/core/html/HTMLFrameOwnerElement.h#newcode64
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/Source/core/html/HTMLPlugInElement.h
File third_party/WebKit/Source/core/html/HTMLPlugInElement.h (left):

https://codereview.chromium.org/2845583002/diff/60001/third_party/WebKit/Source/core/html/HTMLPlugInElement.h#oldcode169
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/Source/core/html/PluginDocument.h
File third_party/WebKit/Source/core/html/PluginDocument.h (right):

https://codereview.chromium.org/2845583002/diff/60001/third_party/WebKit/Source/core/html/PluginDocument.h#newcode59
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/Source/platform/FrameViewBase.h
File third_party/WebKit/Source/platform/FrameViewBase.h (left):

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

https://codereview.chromium.org/2845583002/

dch...@chromium.org

unread,
May 9, 2017, 3:50:51 AM5/9/17
to joelh...@chromium.org, har...@chromium.org, sza...@chromium.org, blink-...@chromium.org, blink-revi...@chromium.org, blink-rev...@chromium.org, blink-revi...@chromium.org, chromium...@chromium.org, dglazko...@chromium.org, eae+bli...@chromium.org, jchaffraix...@chromium.org, leviw+re...@chromium.org, pdr+renderi...@chromium.org, szager+la...@chromium.org, zol...@webkit.org
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/Source/core/frame/FrameView.h
File third_party/WebKit/Source/core/frame/FrameView.h (right):

https://codereview.chromium.org/2845583002/diff/60001/third_party/WebKit/Source/core/frame/FrameView.h#newcode650
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/Source/core/frame/FrameView.h#newcode666
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/Source/core/html/HTMLFrameOwnerElement.h
File third_party/WebKit/Source/core/html/HTMLFrameOwnerElement.h
(right):

https://codereview.chromium.org/2845583002/diff/60001/third_party/WebKit/Source/core/html/HTMLFrameOwnerElement.h#newcode64
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/Source/core/frame/FrameView.cpp
File third_party/WebKit/Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/2845583002/diff/80001/third_party/WebKit/Source/core/frame/FrameView.cpp#newcode1522
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/Source/core/layout/LayoutPart.cpp
File third_party/WebKit/Source/core/layout/LayoutPart.cpp (right):

https://codereview.chromium.org/2845583002/diff/80001/third_party/WebKit/Source/core/layout/LayoutPart.cpp#newcode68
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/Source/modules/plugins/PluginOcclusionSupport.cpp
File
third_party/WebKit/Source/modules/plugins/PluginOcclusionSupport.cpp
(right):

https://codereview.chromium.org/2845583002/diff/80001/third_party/WebKit/Source/modules/plugins/PluginOcclusionSupport.cpp#newcode156
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.

https://codereview.chromium.org/2845583002/

joelh...@chromium.org

unread,
May 9, 2017, 10:35:44 PM5/9/17
to dcheng+...@chromium.org, har...@chromium.org, sza...@chromium.org, blink-...@chromium.org, blink-revi...@chromium.org, blink-rev...@chromium.org, blink-revi...@chromium.org, chromium...@chromium.org, dglazko...@chromium.org, eae+bli...@chromium.org, jchaffraix...@chromium.org, leviw+re...@chromium.org, pdr+renderi...@chromium.org, szager+la...@chromium.org, zol...@webkit.org

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

https://codereview.chromium.org/2845583002/diff/60001/third_party/WebKit/Source/core/frame/FrameView.h#newcode650
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/RemoteFrameView.cpp?l=145&rcl=67970ee192706eed7c080811732bab47383d636c


https://codereview.chromium.org/2845583002/diff/60001/third_party/WebKit/Source/core/frame/FrameView.h#newcode666
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/Source/core/html/HTMLFrameOwnerElement.h
File third_party/WebKit/Source/core/html/HTMLFrameOwnerElement.h
(right):

https://codereview.chromium.org/2845583002/diff/60001/third_party/WebKit/Source/core/html/HTMLFrameOwnerElement.h#newcode64
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/Source/core/frame/FrameView.cpp
File third_party/WebKit/Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/2845583002/diff/80001/third_party/WebKit/Source/core/frame/FrameView.cpp#newcode1522
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
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.

https://codereview.chromium.org/2845583002/

dch...@chromium.org

unread,
May 10, 2017, 3:08:29 AM5/10/17
to joelh...@chromium.org, har...@chromium.org, sza...@chromium.org, blink-...@chromium.org, blink-revi...@chromium.org, blink-rev...@chromium.org, blink-revi...@chromium.org, chromium...@chromium.org, dglazko...@chromium.org, eae+bli...@chromium.org, jchaffraix...@chromium.org, leviw+re...@chromium.org, pdr+renderi...@chromium.org, szager+la...@chromium.org, zol...@webkit.org

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

https://codereview.chromium.org/2845583002/diff/60001/third_party/WebKit/Source/core/frame/FrameView.h#newcode650
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/RemoteFrameView.cpp?l=145&rcl=67970ee192706eed7c080811732bab47383d636c

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

https://codereview.chromium.org/2845583002/

joelh...@chromium.org

unread,
May 10, 2017, 5:40:47 AM5/10/17
to dcheng+...@chromium.org, har...@chromium.org, sza...@chromium.org, blink-...@chromium.org, blink-revi...@chromium.org, blink-rev...@chromium.org, blink-revi...@chromium.org, chromium...@chromium.org, dglazko...@chromium.org, eae+bli...@chromium.org, jchaffraix...@chromium.org, leviw+re...@chromium.org, pdr+renderi...@chromium.org, szager+la...@chromium.org, zol...@webkit.org

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

https://codereview.chromium.org/2845583002/diff/60001/third_party/WebKit/Source/core/frame/FrameView.h#newcode650
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/RemoteFrameView.cpp?l=145&rcl=67970ee192706eed7c080811732bab47383d636c
>
> 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.
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.

sza...@chromium.org

unread,
May 10, 2017, 5:41:04 AM5/10/17
to joelh...@chromium.org, dcheng+...@chromium.org, har...@chromium.org, blink-...@chromium.org, blink-revi...@chromium.org, blink-rev...@chromium.org, blink-revi...@chromium.org, chromium...@chromium.org, dglazko...@chromium.org, eae+bli...@chromium.org, jchaffraix...@chromium.org, leviw+re...@chromium.org, pdr+renderi...@chromium.org, szager+la...@chromium.org, zol...@webkit.org

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

https://codereview.chromium.org/2845583002/diff/60001/third_party/WebKit/Source/core/html/HTMLFrameOwnerElement.h#newcode64
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.

https://codereview.chromium.org/2845583002/

dch...@chromium.org

unread,
May 11, 2017, 5:57:13 PM5/11/17
to joelh...@chromium.org, har...@chromium.org, sza...@chromium.org, blink-...@chromium.org, blink-revi...@chromium.org, blink-rev...@chromium.org, blink-revi...@chromium.org, chromium...@chromium.org, dglazko...@chromium.org, eae+bli...@chromium.org, jchaffraix...@chromium.org, leviw+re...@chromium.org, pdr+renderi...@chromium.org, szager+la...@chromium.org, zol...@webkit.org

joelh...@chromium.org

unread,
May 11, 2017, 7:34:12 PM5/11/17
to dcheng+...@chromium.org, har...@chromium.org, sza...@chromium.org, blink-...@chromium.org, blink-revi...@chromium.org, blink-rev...@chromium.org, blink-revi...@chromium.org, chromium...@chromium.org, dglazko...@chromium.org, eae+bli...@chromium.org, jchaffraix...@chromium.org, leviw+re...@chromium.org, pdr+renderi...@chromium.org, szager+la...@chromium.org, zol...@webkit.org

commit-bot@chromium.org via codereview.chromium.org

unread,
May 11, 2017, 7:34:55 PM5/11/17
to joelh...@chromium.org, dcheng+...@chromium.org, har...@chromium.org, sza...@chromium.org, commi...@chromium.org, blink-...@chromium.org, blink-revi...@chromium.org, blink-rev...@chromium.org, blink-revi...@chromium.org, chromium...@chromium.org, dglazko...@chromium.org, eae+bli...@chromium.org, jchaffraix...@chromium.org, leviw+re...@chromium.org, pdr+renderi...@chromium.org, szager+la...@chromium.org, zol...@webkit.org

commit-bot@chromium.org via codereview.chromium.org

unread,
May 11, 2017, 8:04:43 PM5/11/17
to joelh...@chromium.org, dcheng+...@chromium.org, har...@chromium.org, sza...@chromium.org, commi...@chromium.org, blink-...@chromium.org, blink-revi...@chromium.org, blink-rev...@chromium.org, blink-revi...@chromium.org, chromium...@chromium.org, dglazko...@chromium.org, eae+bli...@chromium.org, jchaffraix...@chromium.org, leviw+re...@chromium.org, pdr+renderi...@chromium.org, szager+la...@chromium.org, zol...@webkit.org
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_presubmit/builds/434777)

https://codereview.chromium.org/2845583002/

joelh...@chromium.org

unread,
May 12, 2017, 12:13:17 AM5/12/17
to dcheng+...@chromium.org, har...@chromium.org, sza...@chromium.org, blink-...@chromium.org, blink-revi...@chromium.org, blink-rev...@chromium.org, blink-revi...@chromium.org, chromium...@chromium.org, dglazko...@chromium.org, eae+bli...@chromium.org, jchaffraix...@chromium.org, leviw+re...@chromium.org, pdr+renderi...@chromium.org, szager+la...@chromium.org, zol...@webkit.org
haraken@, ptal. I need owners approval for platform.

https://codereview.chromium.org/2845583002/

har...@chromium.org

unread,
May 12, 2017, 12:23:42 AM5/12/17
to joelh...@chromium.org, dcheng+...@chromium.org, sza...@chromium.org, blink-...@chromium.org, blink-revi...@chromium.org, blink-rev...@chromium.org, blink-revi...@chromium.org, chromium...@chromium.org, dglazko...@chromium.org, eae+bli...@chromium.org, jchaffraix...@chromium.org, leviw+re...@chromium.org, pdr+renderi...@chromium.org, szager+la...@chromium.org, zol...@webkit.org
Sorry about the review delay. I'm catching up with things... LGTM.



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

https://codereview.chromium.org/2845583002/diff/140001/third_party/WebKit/Source/core/html/HTMLPlugInElement.h#newcode186
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.)

https://codereview.chromium.org/2845583002/

commit-bot@chromium.org via codereview.chromium.org

unread,
May 12, 2017, 12:40:26 AM5/12/17
to joelh...@chromium.org, dcheng+...@chromium.org, har...@chromium.org, sza...@chromium.org, commi...@chromium.org, blink-...@chromium.org, blink-revi...@chromium.org, blink-rev...@chromium.org, blink-revi...@chromium.org, chromium...@chromium.org, dglazko...@chromium.org, eae+bli...@chromium.org, jchaffraix...@chromium.org, leviw+re...@chromium.org, pdr+renderi...@chromium.org, szager+la...@chromium.org, zol...@webkit.org

commit-bot@chromium.org via codereview.chromium.org

unread,
May 12, 2017, 12:45:55 AM5/12/17
to joelh...@chromium.org, dcheng+...@chromium.org, har...@chromium.org, sza...@chromium.org, commi...@chromium.org, blink-...@chromium.org, blink-revi...@chromium.org, blink-rev...@chromium.org, blink-revi...@chromium.org, chromium...@chromium.org, dglazko...@chromium.org, eae+bli...@chromium.org, jchaffraix...@chromium.org, leviw+re...@chromium.org, pdr+renderi...@chromium.org, szager+la...@chromium.org, zol...@webkit.org
Reply all
Reply to author
Forward
0 new messages