Split InfoBarService core code into InfoBarManager

chrome/browser/ui/gtk/browser_window_gtk.cc

 // Copyright 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/ui/gtk/browser_window_gtk.h"
 
 #include <gdk/gdkkeysyms.h>
 
 #include <algorithm>
 #include <string>
@@ skipping 784 matching lines @@
 void BrowserWindowGtk::OnActiveTabChanged(WebContents* old_contents,
                                           WebContents* new_contents,
                                           int index,
                                           int reason) {
   TRACE_EVENT0("ui::gtk", "BrowserWindowGtk::ActiveTabChanged");
   if (old_contents && !old_contents->IsBeingDestroyed())
     old_contents->GetView()->StoreFocus();
 
   // Update various elements that are interested in knowing the current
   // WebContents.
-  infobar_container_->ChangeInfoBarService(
-      InfoBarService::FromWebContents(new_contents));
+  InfoBarManager* infobar_manager = NULL;

[blundell, 2014/03/31 08:29:22]

Optional thought for a follow-up CL: You use this code pattern several places in
this CL. It might be worth creating a static
InfoBarService::InfoBarManagerFromWebContents() method.

[Peter Kasting, 2014/03/31 20:05:34]

This comment made me glance at this code (which I didn't review originally).

Are you sure your new NULL-check is appropriate?  Can |infobar_service| ever
actually be NULL below?  I wouldn't expect that to normally be true... maybe
during shutdown?

In general, make sure you don't add any NULL-checks to the existing code unless
they're truly necessary, and when they are necessary, please leave a comment
about when the variable can be NULL.

[droger, 2014/04/01 10:48:34]

InfoBarService::FromWebContents() indeed returns NULL during shutdown
(InfoBarService removes itself from WebContents in WebContentsDestroyed).
I don't know if this code can be called during shutdown, but the existing code
was supporting the case where the InfoBarService was NULL (because
ChangeInfoBarService() has a NULL check inside).

I applied Colin's suggestion and factored this code in a new
InfoBarManagerFromWebContents() method, and added a comment explaining why it
can be NULL there.

+  InfoBarService* infobar_service =
+      InfoBarService::FromWebContents(new_contents);
+  if (infobar_service)
+    infobar_manager = infobar_service->infobar_manager();
+  infobar_container_->ChangeInfoBarManager(infobar_manager);
   contents_container_->SetTab(new_contents);
   UpdateDevToolsForContents(new_contents);
 
   // TODO(estade): after we manage browser activation, add a check to make sure
   // we are the active browser before calling RestoreFocus().
   if (!browser_->tab_strip_model()->closing_all()) {
     new_contents->GetView()->RestoreFocus();
     FindTabHelper* find_tab_helper =
         FindTabHelper::FromWebContents(new_contents);
     if (find_tab_helper->find_ui_active())
@@ skipping 461 matching lines @@
   // The profile avatar icon may have changed.
   gtk_util::SetWindowIcon(window_, browser_->profile());
 }
 
 void BrowserWindowGtk::TabDetachedAt(WebContents* contents, int index) {
   // We use index here rather than comparing |contents| because by this time
   // the model has already removed |contents| from its list, so
   // browser_->tab_strip_model()->GetActiveWebContents() will return NULL or
   // something else.
   if (index == browser_->tab_strip_model()->active_index()) {
-    infobar_container_->ChangeInfoBarService(NULL);
+    infobar_container_->ChangeInfoBarManager(NULL);
     UpdateDevToolsForContents(NULL);
   }
   contents_container_->DetachTab(contents);
 }
 
 void BrowserWindowGtk::ActiveWindowChanged(GdkWindow* active_window) {
   // Do nothing if we're in the process of closing the browser window.
   if (!window_)
     return;
 
@@ skipping 1076 matching lines @@
   BrowserWindowGtk* browser_window_gtk = new BrowserWindowGtk(browser);
   browser_window_gtk->Init();
   return browser_window_gtk;
 }
 
 // static
 chrome::HostDesktopType BrowserWindow::AdjustHostDesktopType(
     chrome::HostDesktopType desktop_type) {
   return desktop_type;
 }