Separate RenderViewHost from RenderWidgetHost, part 4: delegate

chrome/browser/renderer_host/chrome_render_widget_host_view_mac_delegate.mm

Index: chrome/browser/renderer_host/chrome_render_widget_host_view_mac_delegate.mm
diff --git a/chrome/browser/renderer_host/chrome_render_widget_host_view_mac_delegate.mm b/chrome/browser/renderer_host/chrome_render_widget_host_view_mac_delegate.mm
index f320ff62dac393b6a905a4d2e305ba27683054a4..0196b9bd492792dd5a76c5b59f3b4a65d42402c0 100644
--- a/chrome/browser/renderer_host/chrome_render_widget_host_view_mac_delegate.mm
+++ b/chrome/browser/renderer_host/chrome_render_widget_host_view_mac_delegate.mm
@@ -74,10 +74,11 @@ class SpellCheckObserver : public content::WebContentsObserver {
   self = [super init];
   if (self) {
     renderWidgetHost_ = renderWidgetHost;
-    if (renderWidgetHost_->IsRenderView()) {
+    RenderViewHost* rvh = RenderViewHost::From(renderWidgetHost_);
+    if (rvh) {
       spellingObserver_.reset(
           new ChromeRenderWidgetHostViewMacDelegateInternal::SpellCheckObserver(
-              RenderViewHost::From(renderWidgetHost_), self));
+              rvh, self));
     }
 
     historySwiper_.reset([[HistorySwiper alloc] initWithDelegate:self]);
@@ -137,7 +138,7 @@ class SpellCheckObserver : public content::WebContentsObserver {
 // HistorySwiperDelegate methods
 
 - (BOOL)shouldAllowHistorySwiping {
-  if (!renderWidgetHost_ || !renderWidgetHost_->IsRenderView())
+  if (!renderWidgetHost_)
     return NO;
   content::WebContents* webContents = content::WebContents::FromRenderViewHost(
       RenderViewHost::From(renderWidgetHost_));

[ncarter (slow), 2015/10/26 18:35:02]

In the case where renderWidgetHost_ is not a for a view, seems like you've
changed the return value here from 'NO' to 'YES'. Is that intentional?

[Avi (use Gerrit), 2015/10/26 19:41:28]

No. :\

@@ -160,7 +161,8 @@ class SpellCheckObserver : public content::WebContentsObserver {
   // this is sub-optimal.
   // TODO(suzhe): Plumb the "can*" methods up from WebCore.
   if (action == @selector(checkSpelling:)) {
-    *valid = renderWidgetHost_->IsRenderView();
+    *valid =
+        renderWidgetHost_ && RenderViewHost::From(renderWidgetHost_) != nullptr;

[ncarter (slow), 2015/10/26 18:35:02]

This seems to be the only place in this class where we handle a null rWH_ (and
the old code deref'd it unconditionally). Unless this fixes a bug, I'd drop it. 

[Avi (use Gerrit), 2015/10/26 19:41:28]

Well, there are a few other places (note the method -shouldAllowHistorySwiping
in which you commented). But point taken.

     return YES;
   }