InfoBarService inherits from InfoBarManager

chrome/browser/infobars/infobar_delegate.cc

 // 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.
 
 #include "chrome/browser/infobars/infobar_delegate.h"
 
 #include "base/logging.h"
 #include "build/build_config.h"
 #include "chrome/browser/infobars/infobar.h"
 #include "chrome/browser/infobars/infobar_service.h"
 #include "content/public/browser/navigation_controller.h"
 #include "content/public/browser/navigation_details.h"
 #include "content/public/browser/navigation_entry.h"
 #include "content/public/browser/web_contents.h"
 #include "ui/base/resource/resource_bundle.h"
 
+namespace {
+content::WebContents* WebContentsFromInfoBar(InfoBar* infobar) {

[droger, 2014/04/08 15:00:11]

The plan would be to make this helper function public so that any infobar
delegate that needs it can use it instead of InfoBarDelegate::web_contents().

[Peter Kasting, 2014/04/08 20:39:56]

I didn't go this route for two reasons:
(1) Callsite verbosity -- web_contents() is a lot simpler than
WebContentsFromInfoBar(this).
(2) Type-checking -- ensuring web_contents() is available due to an inheritance
hierarchy means that the only delegates that can call it are those that inherit
from the appropriate base class.  If this is public static, then any infobar
could theoretically call it, which we don't want.

[droger, 2014/04/08 20:54:46]

About (2), I don't think we still need to have different base classes for the
infobar delegates anymore (i.e. ContentConfirmInfoBar). The only point of these
base class was to add the web_contents() method, but this is no longer needed
with this design.

[Peter Kasting, 2014/04/08 21:41:35]

Right, I was suggesting that the base class route was preferable to the
no-base-class route.

I might be wrong.  This way might be better.  What really worries me is the
assumption here that infobar->owner() can be safely static_casted to
InfoBarService*.  Nothing in our current design really enforces this.  I was
hoping by use of inheritance to try and make it more mandatory that this holds. 
Maybe that doesn't actually improve, though.

In any case, it seems like you can't declare this in infobar_delegate.h if you
want that file to live in a component.  So where were you planning to move the
declaration?

+  if (!infobar || !infobar->owner())
+    return NULL;
+
+  InfoBarService* infobar_service =
+      static_cast<InfoBarService*>(infobar->owner());
+  return infobar_service->web_contents();
+}
+}  // namespace
+
 using content::NavigationEntry;
 
 // InfoBarDelegate ------------------------------------------------------------
 
 const int InfoBarDelegate::kNoIconID = 0;
 
 InfoBarDelegate::~InfoBarDelegate() {
 }
 
 InfoBarDelegate::InfoBarAutomationType
@@ skipping 74 matching lines @@
   contents_unique_id_ = active_entry ? active_entry->GetUniqueID() : 0;
 }
 
 gfx::Image InfoBarDelegate::GetIcon() const {
   int icon_id = GetIconID();
   return (icon_id == kNoIconID) ? gfx::Image() :
       ResourceBundle::GetSharedInstance().GetNativeImageNamed(icon_id);
 }
 
 content::WebContents* InfoBarDelegate::web_contents() {
-  return (infobar_ && infobar_->owner()) ?
-      infobar_->owner()->web_contents() : NULL;
+  return WebContentsFromInfoBar(infobar_);
 }
 
 InfoBarDelegate::InfoBarDelegate() : contents_unique_id_(0) {
 }
 
 bool InfoBarDelegate::ShouldExpireInternal(
     const NavigationDetails& details) const {
   // NOTE: If you change this, be sure to check and adjust the behavior of
   // anyone who overrides this as necessary!
   return (contents_unique_id_ != details.entry_id) || details.is_reload;
 }