Remove FrameViewBase as base class of RemoteFrameView.

third_party/WebKit/Source/core/frame/FrameView.h

Index: third_party/WebKit/Source/core/frame/FrameView.h
diff --git a/third_party/WebKit/Source/core/frame/FrameView.h b/third_party/WebKit/Source/core/frame/FrameView.h
index 8336242eac67d4ddad0c261b2552c75616a6c95a..1f74d0df1e3a09f0554303e1bcaf1cda1e35bbf0 100644
--- a/third_party/WebKit/Source/core/frame/FrameView.h
+++ b/third_party/WebKit/Source/core/frame/FrameView.h
@@ -40,7 +40,6 @@
 #include "core/paint/PaintInvalidationCapableScrollableArea.h"
 #include "core/paint/PaintPhase.h"
 #include "core/paint/ScrollbarManager.h"
-#include "core/plugins/PluginView.h"
 #include "platform/FrameViewBase.h"
 #include "platform/RuntimeEnabledFeatures.h"
 #include "platform/animation/CompositorAnimationHost.h"
@@ -478,8 +477,7 @@ class CORE_EXPORT FrameView final
   // and repaints to the host window in the window's coordinate space.
   PlatformChromeClient* GetChromeClient() const;
 
-  typedef HeapHashSet<Member<FrameViewBase>> ChildrenSet;
-  typedef HeapHashSet<Member<PluginView>> PluginsSet;
+  typedef HeapHashSet<Member<FrameOrPlugin>> ChildrenSet;
   typedef HeapHashSet<Member<Scrollbar>> ScrollbarsSet;
 
   // Functions for child manipulation and inspection.
@@ -492,20 +490,17 @@ class CORE_EXPORT FrameView final
   bool IsVisible() const {
     return self_visible_ && parent_visible_;
   }  // Whether or not we are actually visible.
-  void SetParentVisible(bool);
+  void SetParentVisible(bool) override;
   void SetSelfVisible(bool v) { self_visible_ = v; }
-  void SetParent(FrameViewBase*) override;
-  FrameViewBase* Parent() const override { return parent_; }
+  void SetParent(FrameView*) override;
+  FrameView* Parent() const override { return parent_; }
   FrameView* Root() const;
-  void RemoveChild(FrameViewBase*);
-  void AddChild(FrameViewBase*);
-  const ChildrenSet* Children() const { return &children_; }
-  void RemovePlugin(PluginView*);

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

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_

-  void AddPlugin(PluginView*);
-  const PluginsSet* Plugins() const { return &plugins_; }
+  void RemoveChild(FrameOrPlugin*);
+  void AddChild(FrameOrPlugin*);
+  const ChildrenSet& Children() const { return children_; }
   void RemoveScrollbar(Scrollbar*);
   void AddScrollbar(Scrollbar*);
-  const ScrollbarsSet* Scrollbars() const { return &scrollbars_; }
+  const ScrollbarsSet& Scrollbars() const { return scrollbars_; }
 
   // If the scroll view does not use a native widget, then it will have
   // cross-platform Scrollbars. These functions can be used to obtain those
@@ -652,10 +647,9 @@ class CORE_EXPORT FrameView final
 
   IntRect ConvertToRootFrame(const IntRect&) const;
   IntPoint ConvertToRootFrame(const IntPoint&) const;
-  IntPoint ConvertSelfToChild(const FrameViewBase*, const IntPoint&) const;
+  IntPoint ConvertSelfToChild(const IntPoint&, const IntPoint&) const;

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

The method was only calling FrameViewBase::Location, so it is easy for clients
to provide this IntPoint directly.

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

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

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

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

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

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.

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

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.

 
-  // FrameViewBase override. Handles painting of the contents of the view as
-  // well as the scrollbars.
+  // Handles painting of the contents of the view as well as the scrollbars.
   void Paint(GraphicsContext&, const CullRect&) const override;
   void Paint(GraphicsContext&, const GlobalPaintFlags, const CullRect&) const;
   void PaintContents(GraphicsContext&,
@@ -669,7 +663,7 @@ class CORE_EXPORT FrameView final
   bool ScrollbarCornerPresent() const;
   IntRect ScrollCornerRect() const override;
 
-  IntPoint ConvertFromContainingFrameViewBaseToScrollbar(
+  IntPoint ConvertFromContainingFrameViewToScrollbar(

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

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

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

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)

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

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.

       const Scrollbar&,
       const IntPoint&) const override;
 
@@ -971,10 +965,10 @@ class CORE_EXPORT FrameView final
 
   // Methods to do point conversion via layoutObjects, in order to take
   // transforms into account.
-  IntRect ConvertToContainingFrameViewBase(const IntRect&) const;
-  IntPoint ConvertToContainingFrameViewBase(const IntPoint&) const;
-  IntRect ConvertFromContainingFrameViewBase(const IntRect&) const;
-  IntPoint ConvertFromContainingFrameViewBase(const IntPoint&) const;
+  IntRect ConvertToContainingFrameView(const IntRect&) const;
+  IntPoint ConvertToContainingFrameView(const IntPoint&) const;
+  IntRect ConvertFromContainingFrameView(const IntRect&) const;
+  IntPoint ConvertFromContainingFrameView(const IntPoint&) const;
 
   void DidChangeGlobalRootScroller() override;
 
@@ -1068,7 +1062,7 @@ class CORE_EXPORT FrameView final
   EmbeddedObjectSet part_update_set_;
 
   // FIXME: These are just "children" of the FrameView and should be
-  // Member<FrameViewBase> instead.
+  // Member<FrameView> instead.

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

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. 

   HashSet<RefPtr<LayoutPart>> parts_;
 
   Member<LocalFrame> frame_;
@@ -1148,7 +1142,6 @@ class CORE_EXPORT FrameView final
   bool vertical_scrollbar_lock_;
 
   ChildrenSet children_;
-  PluginsSet plugins_;
   ScrollbarsSet scrollbars_;
 
   ScrollOffset pending_scroll_delta_;
@@ -1179,7 +1172,7 @@ class CORE_EXPORT FrameView final
 
   // Paint properties for SPv2 Only.
   // The hierarchy of transform subtree created by a FrameView.
-  // [ preTranslation ]               The offset from FrameViewBase::frameRect.
+  // [ preTranslation ]               The offset from FrameView::FrameRect.
   //     |                            Establishes viewport.
   //     +---[ scrollTranslation ]    Frame scrolling.
   // TODO(trchen): These will not be needed once settings->rootLayerScrolls() is
@@ -1272,10 +1265,15 @@ DEFINE_TYPE_CASTS(FrameView,
                   frameViewBase->IsFrameView(),
                   frameViewBase.IsFrameView());
 DEFINE_TYPE_CASTS(FrameView,
+                  FrameOrPlugin,
+                  frame_or_plugin,
+                  frame_or_plugin->IsFrameView(),
+                  frame_or_plugin.IsFrameView());
+DEFINE_TYPE_CASTS(FrameView,
                   ScrollableArea,
-                  scrollableArea,
-                  scrollableArea->IsFrameView(),
-                  scrollableArea.IsFrameView());
+                  scrollable_area,
+                  scrollable_area->IsFrameView(),
+                  scrollable_area.IsFrameView());
 
 }  // namespace blink