Separate RenderViewHost from RenderWidgetHost, part 4: delegate

chrome/browser/renderer_host/chrome_render_widget_host_view_mac_delegate.mm

 // Copyright (c) 2012 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.
 
 #import "chrome/browser/renderer_host/chrome_render_widget_host_view_mac_delegate.h"
 
 #include <cmath>
 
 #include "base/prefs/pref_service.h"
 #include "base/strings/sys_string_conversions.h"
@@ skipping 56 matching lines @@
 };
 
 }  // namespace ChromeRenderWidgetHostViewMacDelegateInternal
 
 @implementation ChromeRenderWidgetHostViewMacDelegate
 
 - (id)initWithRenderWidgetHost:(content::RenderWidgetHost*)renderWidgetHost {
   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]);
   }
   return self;
 }
 
 - (void)dealloc {
   [historySwiper_ setDelegate:nil];
   [super dealloc];
@@ skipping 39 matching lines @@
   return [historySwiper_ canRubberbandLeft:view];
 }
 
 - (BOOL)canRubberbandRight:(NSView*)view {
   return [historySwiper_ canRubberbandRight:view];
 }
 
 // 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. :\

   if (webContents && DevToolsWindow::IsDevToolsWindow(webContents)) {
     return NO;
   }
 
   return YES;
 }
 
 - (NSView*)viewThatWantsHistoryOverlay {
   return renderWidgetHost_->GetView()->GetNativeView();
 }
 
 - (BOOL)validateUserInterfaceItem:(id<NSValidatedUserInterfaceItem>)item
                       isValidItem:(BOOL*)valid {
   SEL action = [item action];
 
   // For now, this action is always enabled for render view;
   // 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;
   }
 
   // TODO(groby): Clarify who sends this and if toggleContinuousSpellChecking:
   // is still necessary.
   if (action == @selector(toggleContinuousSpellChecking:)) {
     if ([(id)item respondsToSelector:@selector(setState:)]) {
       content::RenderProcessHost* host = renderWidgetHost_->GetProcess();
       Profile* profile = Profile::FromBrowserContext(host->GetBrowserContext());
       DCHECK(profile);
@@ skipping 72 matching lines @@
 }
 
 - (void)spellCheckEnabled:(BOOL)enabled checked:(BOOL)checked {
   spellcheckEnabled_ = enabled;
   spellcheckChecked_ = checked;
 }
 
 // END Spellchecking methods
 
 @end