Remove FrameViewBase as base class of RemoteFrameView.

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

 /*
    Copyright (C) 1997 Martin Jones (mjones@kde.org)
              (C) 1998 Waldo Bastian (bastian@kde.org)
              (C) 1998, 1999 Torben Weis (weis@kde.org)
              (C) 1999 Lars Knoll (knoll@kde.org)
              (C) 1999 Antti Koivisto (koivisto@kde.org)
    Copyright (C) 2004, 2005, 2006, 2007, 2008, 2009 Apple Inc. All rights
    reserved.
 
    This library is free software; you can redistribute it and/or
@@ skipping 22 matching lines @@
 #include "core/frame/FrameViewAutoSizeInfo.h"
 #include "core/frame/LayoutSubtreeRootList.h"
 #include "core/frame/RootFrameViewport.h"
 #include "core/layout/MapCoordinatesFlags.h"
 #include "core/layout/ScrollAnchor.h"
 #include "core/paint/FirstMeaningfulPaintDetector.h"
 #include "core/paint/ObjectPaintProperties.h"
 #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"
 #include "platform/animation/CompositorAnimationTimeline.h"
 #include "platform/geometry/IntRect.h"
 #include "platform/geometry/LayoutRect.h"
 #include "platform/graphics/Color.h"
 #include "platform/graphics/CompositorElementId.h"
 #include "platform/graphics/GraphicsLayerClient.h"
 #include "platform/scroll/ScrollTypes.h"
@@ skipping 417 matching lines @@
 
   LayoutRect ScrollIntoView(const LayoutRect& rect_in_content,
                             const ScrollAlignment& align_x,
                             const ScrollAlignment& align_y,
                             ScrollType = kProgrammaticScroll) override;
 
   // The window that hosts the FrameView. The FrameView will communicate scrolls
   // 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.
   bool IsSelfVisible() const {
     return self_visible_;
   }  // Whether or not we have been explicitly marked as visible or not.
   bool IsParentVisible() const {
     return parent_visible_;
   }  // Whether or not our parent is visible.
   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
   // scrollbars.
   Scrollbar* HorizontalScrollbar() const override {
     return scrollbar_manager_.HorizontalScrollbar();
   }
   Scrollbar* VerticalScrollbar() const override {
     return scrollbar_manager_.VerticalScrollbar();
   }
@@ skipping 126 matching lines @@
 
   // Functions for converting to screen coordinates.
   IntRect ContentsToScreen(const IntRect&) const;
 
   // For platforms that need to hit test scrollbars from within the engine's
   // event handlers (like Win32).
   Scrollbar* ScrollbarAtFramePoint(const IntPoint&);
 
   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&,
                      const GlobalPaintFlags,
                      const IntRect& damage_rect) const;
 
   void Show() override;
   void Hide() override;
 
   bool IsPointInScrollbarCorner(const IntPoint&);
   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;
 
   bool IsFrameView() const override { return true; }
 
   DECLARE_VIRTUAL_TRACE();
   void NotifyPageThatContentAreaWillPaint() const;
   FrameView* ParentFrameView() const;
 
   // Returns the scrollable area for the frame. For the root frame, this will
@@ skipping 281 matching lines @@
 
   void RecordDeferredLoadingStats();
 
   DocumentLifecycle& Lifecycle() const;
 
   void ContentsResized() override;
   void ScrollbarExistenceDidChange();
 
   // 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;
 
   void UpdateGeometriesIfNeeded();
 
   bool WasViewportResized();
   void SendResizeEventIfNeeded();
 
   void UpdateParentScrollableAreaSet();
 
@@ skipping 73 matching lines @@
   bool CheckLayoutInvalidationIsAllowed() const;
 
   PaintController* GetPaintController() { return paint_controller_.get(); }
 
   LayoutSize size_;
 
   typedef HashSet<RefPtr<LayoutEmbeddedObject>> EmbeddedObjectSet;
   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_;
 
   IntRect frame_rect_;
   Member<FrameView> parent_;
   bool self_visible_;
   bool parent_visible_;
 
   WebDisplayMode display_mode_;
@@ skipping 59 matching lines @@
   bool has_been_disposed_ = false;
 #endif
 
   ScrollbarMode horizontal_scrollbar_mode_;
   ScrollbarMode vertical_scrollbar_mode_;
 
   bool horizontal_scrollbar_lock_;
   bool vertical_scrollbar_lock_;
 
   ChildrenSet children_;
-  PluginsSet plugins_;
   ScrollbarsSet scrollbars_;
 
   ScrollOffset pending_scroll_delta_;
   ScrollOffset scroll_offset_;
   IntSize contents_size_;
 
   bool scrollbars_suppressed_;
 
   bool in_update_scrollbars_;
 
@@ skipping 10 matching lines @@
 
   // The following members control rendering pipeline throttling for this
   // frame. They are only updated in response to intersection observer
   // notifications, i.e., not in the middle of the lifecycle.
   bool hidden_for_throttling_;
   bool subtree_throttled_;
   bool lifecycle_updates_throttled_;
 
   // 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
   // enabled.
   RefPtr<TransformPaintPropertyNode> pre_translation_;
   RefPtr<TransformPaintPropertyNode> scroll_translation_;
   // The content clip clips the document (= LayoutView) but not the scrollbars.
   // TODO(trchen): This will not be needed once settings->rootLayerScrolls() is
   // enabled.
   RefPtr<ClipPaintPropertyNode> content_clip_;
@@ skipping 72 matching lines @@
   if (visually_non_empty_pixel_count_ > kVisualPixelThreshold)
     SetIsVisuallyNonEmpty();
 }
 
 DEFINE_TYPE_CASTS(FrameView,
                   FrameViewBase,
                   frameViewBase,
                   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
 
 #endif  // FrameView_h