RemoteMacViews: Unify views::View to content::WebContentsView interfaces

There exist two interface through which a views::View is stitched to
a content::WebContentsView
- the ui::Layer is attached via a cr_setParentUiLayer method
- accessibility is attached via the accesibilityHostable protocol

Lifetimes of the embedding are slightly unclear because the attached
WebContentsView may be destroyed the NativeViewHost detaches it.

Replace the AccessibilityHostable protocol with a ViewsHostable
protocol, and add a C++ interface, ui::ViewsHostableView, which provides
a single place through which a views::View and a content::WebContentsView
are stitched together
- have content::WebContentsViewMac implement ui::ViewsHostableView
- have views::NativeWidgetHost implement ui::ViewsHostableView::Host

Use this explicit interface to ensure that the views::NativeWidgetHost
detaches from the content::WebContentsViewMac before it is destroyed.

Additional parameters and method will be added to the
ui::ViewsHostableView interface to enable stitching together
out-of-process NSViews.

R=lgrey
TBR=avi (content/ OWNERship)

Bug: 821561
Change-Id: Iddc46278cd06b9e27d3f12aa5a9e830273f3b562
Reviewed-on: https://chromium-review.googlesource.com/1252641
Commit-Queue: ccameron <ccameron@chromium.org>
Reviewed-by: Leonard Grey <lgrey@chromium.org>
Cr-Commit-Position: refs/heads/master@{#595521}

content/browser/web_contents/web_contents_view_mac.h

@@ -19,8 +19,8 @@
 #include "content/browser/web_contents/web_contents_view.h"
 #include "content/common/content_export.h"
 #include "content/common/drag_event_source_info.h"
-#import "ui/base/cocoa/accessibility_hostable.h"
 #import "ui/base/cocoa/base_view.h"
+#import "ui/base/cocoa/views_hostable.h"
 #include "ui/gfx/geometry/size.h"
 
 @class WebDragDest;
@@ -42,7 +42,7 @@ class Layer;
 }
 
 CONTENT_EXPORT
-@interface WebContentsViewCocoa : BaseView<AccessibilityHostable> {
+@interface WebContentsViewCocoa : BaseView<ViewsHostable> {
  @private
   // Instances of this class are owned by both webContentsView_ and AppKit. It
   // is possible for an instance to outlive its webContentsView_. The
@@ -56,6 +56,12 @@ CONTENT_EXPORT
 
 - (void)setMouseDownCanMoveWindow:(BOOL)canMove;
 
+// Sets |accessibilityParent| as the object returned when the
+// receiver is queried for its accessibility parent.
+// TODO(lgrey/ellyjones): Remove this in favor of setAccessibilityParent:
+// when we switch to the new accessibility API.
+- (void)setAccessibilityParentElement:(id)accessibilityParent;
+
 // Returns the available drag operations. This is a required method for
 // NSDraggingSource. It is supposedly deprecated, but the non-deprecated API
 // -[NSWindow dragImage:...] still relies on it.
@@ -69,7 +75,8 @@ namespace content {
 // contains all of the contents of the tab and associated child views.
 class WebContentsViewMac : public WebContentsView,
                            public RenderViewHostDelegateView,
-                           public PopupMenuHelper::Delegate {
+                           public PopupMenuHelper::Delegate,
+                           public ui::ViewsHostableView {
  public:
   // The corresponding WebContentsImpl is passed in the constructor, and manages
   // our lifetime. This doesn't need to be the case, but is this way currently
@@ -132,12 +139,14 @@ class WebContentsViewMac : public WebContentsView,
   // PopupMenuHelper::Delegate:
   void OnMenuClosed() override;
 
+  // ViewsHostableView:
+  void OnViewsHostableAttached(ViewsHostableView::Host* host) override;
+  void OnViewsHostableDetached() override;
+
   // A helper method for closing the tab in the
   // CloseTabAfterEventTracking() implementation.
   void CloseTab();
 
-  void SetParentUiLayer(ui::Layer* parent_ui_layer);
-
   WebContentsImpl* web_contents() { return web_contents_; }
   WebContentsViewDelegate* delegate() { return delegate_.get(); }
 
@@ -149,6 +158,8 @@ class WebContentsViewMac : public WebContentsView,
       RenderWidgetHostViewCreateFunction create_render_widget_host_view);
 
  private:
+  void SetParentUiLayer(ui::Layer* parent_ui_layer);
+
   // Returns the fullscreen view, if one exists; otherwise, returns the content
   // native view. This ensures that the view currently attached to a NSWindow is
   // being used to query or set first responder state.
@@ -170,7 +181,8 @@ class WebContentsViewMac : public WebContentsView,
   // destroyed.
   std::list<base::WeakPtr<RenderWidgetHostViewBase>> child_views_;
 
-  ui::Layer* parent_ui_layer_ = nullptr;
+  // Interface to the views::View host of this view.
+  ViewsHostableView::Host* views_host_ = nullptr;
 
   std::unique_ptr<PopupMenuHelper> popup_menu_helper_;
 

content/browser/web_contents/web_contents_view_mac.mm

@@ -108,6 +108,9 @@ WebContentsViewMac::WebContentsViewMac(WebContentsImpl* web_contents,
     : web_contents_(web_contents), delegate_(delegate) {}
 
 WebContentsViewMac::~WebContentsViewMac() {
+  if (views_host_)
+    views_host_->OnHostableViewDestroying();
+  DCHECK(!views_host_);
   // This handles the case where a renderer close call was deferred
   // while the user was operating a UI control which resulted in a
   // close.  In that case, the Cocoa view outlives the
@@ -354,7 +357,8 @@ RenderWidgetHostViewBase* WebContentsViewMac::CreateViewForWidget(
 
   // Add the RenderWidgetHostView to the ui::Layer heirarchy.
   child_views_.push_back(view->GetWeakPtr());
-  SetParentUiLayer(parent_ui_layer_);
+  if (views_host_)
+    SetParentUiLayer(views_host_->GetUiLayer());
 
   // Fancy layout comes later; for now just make it our size and resize it
   // with us. In case there are other siblings of the content area, we want
@@ -431,7 +435,6 @@ void WebContentsViewMac::CloseTab() {
 }
 
 void WebContentsViewMac::SetParentUiLayer(ui::Layer* parent_ui_layer) {
-  parent_ui_layer_ = parent_ui_layer;
   // Remove any child NSViews that have been destroyed.
   for (auto iter = child_views_.begin(); iter != child_views_.end();) {
     if (*iter)
@@ -441,6 +444,26 @@ void WebContentsViewMac::SetParentUiLayer(ui::Layer* parent_ui_layer) {
   }
 }
 
+////////////////////////////////////////////////////////////////////////////////
+// WebContentsViewMac, ViewsHostableView:
+
+void WebContentsViewMac::OnViewsHostableAttached(
+    ViewsHostableView::Host* host) {
+  views_host_ = host;
+
+  SetParentUiLayer(views_host_->GetUiLayer());
+  [cocoa_view_
+      setAccessibilityParentElement:views_host_->GetAccessibilityElement()];
+}
+
+void WebContentsViewMac::OnViewsHostableDetached() {
+  DCHECK(views_host_);
+  views_host_ = nullptr;
+
+  SetParentUiLayer(nullptr);
+  [cocoa_view_ setAccessibilityParentElement:nil];
+}
+
 }  // namespace content
 
 @implementation WebContentsViewCocoa
@@ -655,11 +678,6 @@ void WebContentsViewMac::SetParentUiLayer(ui::Layer* parent_ui_layer) {
       FocusThroughTabTraversal(direction == NSSelectingPrevious);
 }
 
-- (void)cr_setParentUiLayer:(ui::Layer*)parentUiLayer {
-  if (webContentsView_)
-    webContentsView_->SetParentUiLayer(parentUiLayer);
-}
-
 - (void)updateWebContentsVisibility {
   WebContentsImpl* webContents = [self webContents];
   if (!webContents || webContents->IsBeingDestroyed())
@@ -724,11 +742,15 @@ void WebContentsViewMac::SetParentUiLayer(ui::Layer* parent_ui_layer) {
   [self updateWebContentsVisibility];
 }
 
-// AccessibilityHostable protocol implementation.
 - (void)setAccessibilityParentElement:(id)accessibilityParent {
   accessibilityParent_.reset([accessibilityParent retain]);
 }
 
+// ViewsHostable protocol implementation.
+- (ui::ViewsHostableView*)viewsHostableView {
+  return webContentsView_;
+}
+
 // NSAccessibility informal protocol implementation.
 - (id)accessibilityAttributeValue:(NSString*)attribute {
   if (accessibilityParent_ &&

ui/base/BUILD.gn

@@ -89,7 +89,6 @@ jumbo_component("base") {
     "clipboard/clipboard_win.cc",
     "clipboard/clipboard_win.h",
     "clipboard/custom_data_helper_mac.mm",
-    "cocoa/accessibility_hostable.h",
     "cocoa/animation_utils.h",
     "cocoa/appkit_utils.h",
     "cocoa/appkit_utils.mm",
@@ -152,6 +151,7 @@ jumbo_component("base") {
     "cocoa/user_interface_item_command_handler.h",
     "cocoa/view_description.h",
     "cocoa/view_description.mm",
+    "cocoa/views_hostable.h",
     "cocoa/weak_ptr_nsobject.h",
     "cocoa/weak_ptr_nsobject.mm",
     "cocoa/window_size_constants.h",

ui/base/cocoa/accessibility_hostable.h

-// Copyright 2018 The Chromium Authors. All rights reserved.
-// Use of this source code is governed by a BSD-style license that can be
-// found in the LICENSE file.
-
-#ifndef UI_BASE_COCOA_ACCESSIBILITY_HOSTABLE_H_
-#define UI_BASE_COCOA_ACCESSIBILITY_HOSTABLE_H_
-
-#import <objc/objc.h>
-
-// An object that can be hosted in another accessibility hierarchy.
-// This allows for stitching together heterogenous accessibility
-// hierarchies, for example the AXPlatformNodeCocoa-based views
-// toolkit hierarchy and the BrowserAccessibilityCocoa-based
-// web content hierarchy.
-@protocol AccessibilityHostable
-
-// Sets |accessibilityParent| as the object returned when the
-// receiver is queried for its accessibility parent.
-// TODO(lgrey/ellyjones): Remove this in favor of setAccessibilityParent:
-// when we switch to the new accessibility API.
-- (void)setAccessibilityParentElement:(id)accessibilityParent;
-
-@end
-
-#endif  // UI_BASE_COCOA_ACCESSIBILITY_HOSTABLE_H_

ui/base/cocoa/views_hostable.h

+// Copyright 2018 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef UI_BASE_COCOA_VIEWS_HOSTABLE_H_
+#define UI_BASE_COCOA_VIEWS_HOSTABLE_H_
+
+#import <objc/objc.h>
+
+#include "ui/base/ui_base_export.h"
+#include "ui/gfx/geometry/rect.h"
+
+namespace ui {
+
+class Layer;
+
+// Interface that it used to stitch a content::WebContentsView into a
+// views::View.
+class ViewsHostableView {
+ public:
+  // Host interface through which the WebContentsView may indicate that its C++
+  // object is destroying.
+  class Host {
+   public:
+    // Query the ui::Layer of the host.
+    virtual ui::Layer* GetUiLayer() const = 0;
+
+    // Query the parent accessibility element of the host.
+    virtual id GetAccessibilityElement() const = 0;
+
+    // Called when the hostable view will be destroyed.
+    virtual void OnHostableViewDestroying() = 0;
+  };
+
+  // Called when the content::WebContentsView's NSView is added as a subview of
+  // the views::View's NSView (note that these are the browser-side NSViews).
+  // This is responsible for:
+  // - Adding the WebContentsView's ui::Layer to the parent's ui::Layer tree.
+  // - Stitching together the accessibility tree between the views::View and
+  //   the WebContentsView.
+  // - Stitching together any app-shim-side NSViews.
+  virtual void OnViewsHostableAttached(Host* host) = 0;
+  // Called when the WebContentsView's NSView has been removed from the
+  // views::View's NSView. This is responsible for un-doing all of the actions
+  // taken when attaching.
+  virtual void OnViewsHostableDetached() = 0;
+};
+
+}  // namespace ui
+
+// The protocol through which an NSView indicates support for the
+// ViewsHostableView interface.
+@protocol ViewsHostable
+
+- (ui::ViewsHostableView*)viewsHostableView;
+
+@end
+
+#endif  // UI_BASE_COCOA_VIEWS_HOSTABLE_H_

ui/views/controls/native/native_view_host_mac.h

@@ -7,24 +7,32 @@
 
 #include "base/mac/scoped_nsobject.h"
 #include "base/macros.h"
+#include "ui/base/cocoa/views_hostable.h"
 #include "ui/views/controls/native/native_view_host_wrapper.h"
 #include "ui/views/views_export.h"
 
 namespace ui {
 class LayerOwner;
-}
+class ViewsHostableView;
+}  // namespace ui
 
 namespace views {
 
 class NativeViewHost;
 
 // Mac implementation of NativeViewHostWrapper.
-class NativeViewHostMac : public NativeViewHostWrapper {
+class NativeViewHostMac : public NativeViewHostWrapper,
+                          public ui::ViewsHostableView::Host {
  public:
   explicit NativeViewHostMac(NativeViewHost* host);
   ~NativeViewHostMac() override;
 
-  // Overridden from NativeViewHostWrapper:
+  // ViewsHostableView::Host:
+  ui::Layer* GetUiLayer() const override;
+  id GetAccessibilityElement() const override;
+  void OnHostableViewDestroying() override;
+
+  // NativeViewHostWrapper:
   void AttachNativeView() override;
   void NativeViewDetaching(bool destroyed) override;
   void AddedToWidget() override;
@@ -48,6 +56,11 @@ class NativeViewHostMac : public NativeViewHostWrapper {
   // Retain the native view as it may be destroyed at an unpredictable time.
   base::scoped_nsobject<NSView> native_view_;
 
+  // If |native_view| supports the ViewsHostable protocol, then this is the
+  // the corresponding ViewsHostableView interface (which is implemeted only
+  // by WebContents and tests).
+  ui::ViewsHostableView* native_view_hostable_ = nullptr;
+
   DISALLOW_COPY_AND_ASSIGN(NativeViewHostMac);
 };
 

ui/views/controls/native/native_view_host_mac.mm

@@ -8,19 +8,11 @@
 
 #include "base/mac/foundation_util.h"
 #import "ui/accessibility/platform/ax_platform_node_mac.h"
-#import "ui/base/cocoa/accessibility_hostable.h"
 #import "ui/views/cocoa/bridged_native_widget_host_impl.h"
 #include "ui/views/controls/native/native_view_host.h"
 #include "ui/views/widget/native_widget_mac.h"
 #include "ui/views/widget/widget.h"
 
-// NSViews that can be drawn as a ui::Layer directly will implement this
-// interface. Calling cr_setParentLayer will embed the ui::Layer of the NSView
-// under |parentUiLayer|.
-@interface NSView (UICompositor)
-- (void)cr_setParentUiLayer:(ui::Layer*)parentUiLayer;
-@end
-
 namespace views {
 namespace {
 
@@ -59,6 +51,40 @@ NativeViewHostMac::NativeViewHostMac(NativeViewHost* host) : host_(host) {
 NativeViewHostMac::~NativeViewHostMac() {
 }
 
+////////////////////////////////////////////////////////////////////////////////
+// NativeViewHostMac, ViewsHostableView::Host implementation:
+
+ui::Layer* NativeViewHostMac::GetUiLayer() const {
+  return host_->layer();
+}
+
+id NativeViewHostMac::GetAccessibilityElement() const {
+  // Find the closest ancestor view that participates in the views toolkit
+  // accessibility hierarchy and set its element as the native view's parent.
+  // This is necessary because a closer ancestor might already be attaching
+  // to the NSView/content hierarchy.
+  // For example, web content is currently embedded into the views hierarchy
+  // roughly like this:
+  // BrowserView (views)
+  // |_  WebView (views)
+  //   |_  NativeViewHost (views)
+  //     |_  WebContentView (Cocoa, is |native_view_| in this scenario,
+  //         |               accessibility ignored).
+  //         |_ RenderWidgetHostView (Cocoa)
+  // WebView specifies either the RenderWidgetHostView or the native view as
+  // its accessibility element. That means that if we were to set it as
+  // |native_view_|'s parent, the RenderWidgetHostView would be its own
+  // accessibility parent! Instead, we want to find the browser view and
+  // attach to its node.
+  return ClosestPlatformAncestorNode(host_->parent());
+}
+
+void NativeViewHostMac::OnHostableViewDestroying() {
+  DCHECK(native_view_hostable_);
+  host_->NativeViewDestroyed();
+  DCHECK(!native_view_hostable_);
+}
+
 ////////////////////////////////////////////////////////////////////////////////
 // NativeViewHostMac, NativeViewHostWrapper implementation:
 
@@ -66,46 +92,25 @@ void NativeViewHostMac::AttachNativeView() {
   DCHECK(host_->native_view());
   DCHECK(!native_view_);
   native_view_.reset([host_->native_view() retain]);
-
-  if ([native_view_ respondsToSelector:@selector(cr_setParentUiLayer:)])
-    [native_view_ cr_setParentUiLayer:host_->layer()];
-  if ([native_view_ conformsToProtocol:@protocol(AccessibilityHostable)]) {
-    // Find the closest ancestor view that participates in the views toolkit
-    // accessibility hierarchy and set its element as the native view's parent.
-    // This is necessary because a closer ancestor might already be attaching
-    // to the NSView/content hierarchy.
-    // For example, web content is currently embedded into the views hierarchy
-    // roughly like this:
-    // BrowserView (views)
-    // |_  WebView (views)
-    //   |_  NativeViewHost (views)
-    //     |_  WebContentView (Cocoa, is |native_view_| in this scenario,
-    //         |               accessibility ignored).
-    //         |_ RenderWidgetHostView (Cocoa)
-    // WebView specifies either the RenderWidgetHostView or the native view as
-    // its accessibility element. That means that if we were to set it as
-    // |native_view_|'s parent, the RenderWidgetHostView would be its own
-    // accessibility parent! Instead, we want to find the browser view and
-    // attach to its node.
-    id hostable = native_view_;
-    [hostable setAccessibilityParentElement:ClosestPlatformAncestorNode(
-                                                host_->parent())];
-  }
-
   EnsureNativeViewHasNoChildWidgets(native_view_);
   BridgedNativeWidgetHostImpl* bridge_host =
       BridgedNativeWidgetHostImpl::GetFromNativeWindow(
           host_->GetWidget()->GetNativeWindow());
   DCHECK(bridge_host);
   bridge_host->SetAssociationForView(host_, native_view_);
+
+  if ([native_view_ conformsToProtocol:@protocol(ViewsHostable)]) {
+    id hostable = native_view_;
+    native_view_hostable_ = [hostable viewsHostableView];
+    if (native_view_hostable_)
+      native_view_hostable_->OnViewsHostableAttached(this);
+  }
 }
 
 void NativeViewHostMac::NativeViewDetaching(bool destroyed) {
-  // |destroyed| is only true if this class calls host_->NativeViewDestroyed().
-  // Aura does this after observing an aura OnWindowDestroying, but NSViews
-  // are reference counted so there isn't a reliable signal. Instead, a
-  // reference is retained until the NativeViewHost is detached.
-  DCHECK(!destroyed);
+  // |destroyed| is only true if this class calls host_->NativeViewDestroyed(),
+  // which is called if a hosted WebContentsView about to be destroyed (note
+  // that its corresponding NSView may still exist).
 
   // |native_view_| can be nil here if RemovedFromWidget() is called before
   // NativeViewHost::Detach().
@@ -118,11 +123,9 @@ void NativeViewHostMac::NativeViewDetaching(bool destroyed) {
   [host_->native_view() setHidden:YES];
   [host_->native_view() removeFromSuperview];
 
-  if ([native_view_ respondsToSelector:@selector(cr_setParentUiLayer:)])
-    [native_view_ cr_setParentUiLayer:nullptr];
-  if ([native_view_ conformsToProtocol:@protocol(AccessibilityHostable)]) {
-    id hostable = native_view_;
-    [hostable setAccessibilityParentElement:nil];
+  if (native_view_hostable_) {
+    native_view_hostable_->OnViewsHostableDetached();
+    native_view_hostable_ = nullptr;
   }
 
   EnsureNativeViewHasNoChildWidgets(host_->native_view());

ui/views/controls/native/native_view_host_mac_unittest.mm

@@ -12,17 +12,35 @@
 #import "base/mac/scoped_nsobject.h"
 #include "base/macros.h"
 #import "testing/gtest_mac.h"
-#import "ui/base/cocoa/accessibility_hostable.h"
+#import "ui/base/cocoa/views_hostable.h"
 #include "ui/views/controls/native/native_view_host.h"
 #include "ui/views/controls/native/native_view_host_test_base.h"
 #include "ui/views/view.h"
 #include "ui/views/widget/widget.h"
 
-@interface TestAccessibilityHostableView : NSView<AccessibilityHostable>
-@property(nonatomic, assign) id accessibilityParentElement;
+class TestViewsHostable : public ui::ViewsHostableView {
+ public:
+  id parent_accessibility_element() const {
+    return parent_accessibility_element_;
+  }
+
+ private:
+  // ui::ViewsHostableView:
+  void OnViewsHostableAttached(ui::ViewsHostableView::Host* host) override {
+    parent_accessibility_element_ = host->GetAccessibilityElement();
+  }
+  void OnViewsHostableDetached() override {
+    parent_accessibility_element_ = nil;
+  }
+
+  id parent_accessibility_element_ = nil;
+};
+
+@interface TestViewsHostableView : NSView<ViewsHostable>
+@property(nonatomic, assign) ui::ViewsHostableView* viewsHostableView;
 @end
-@implementation TestAccessibilityHostableView
-@synthesize accessibilityParentElement = accessibilityParentElement_;
+@implementation TestViewsHostableView
+@synthesize viewsHostableView = viewsHostableView_;
 @end
 
 namespace views {
@@ -115,15 +133,18 @@ TEST_F(NativeViewHostMacTest, AccessibilityParent) {
   CreateHost();
   host()->Detach();
 
-  base::scoped_nsobject<TestAccessibilityHostableView> view(
-      [[TestAccessibilityHostableView alloc] init]);
+  base::scoped_nsobject<TestViewsHostableView> view(
+      [[TestViewsHostableView alloc] init]);
+  TestViewsHostable views_hostable;
+  [view setViewsHostableView:&views_hostable];
+
   host()->Attach(view);
-  EXPECT_NSEQ([view accessibilityParentElement],
+  EXPECT_NSEQ(views_hostable.parent_accessibility_element(),
               toplevel()->GetRootView()->GetNativeViewAccessible());
 
   host()->Detach();
   DestroyHost();
-  EXPECT_FALSE([view accessibilityParentElement]);
+  EXPECT_FALSE(views_hostable.parent_accessibility_element());
 }
 
 // Test that the content windows' bounds are set to the correct values while the