Eliminate dependence of GoogleURLTracker et al. on InfoBarService

chrome/browser/google/google_url_tracker_infobar_delegate.cc

Index: chrome/browser/google/google_url_tracker_infobar_delegate.cc
diff --git a/chrome/browser/google/google_url_tracker_infobar_delegate.cc b/chrome/browser/google/google_url_tracker_infobar_delegate.cc
index 585cabdf63b795452e95d9fbb96fd09a0aeea90e..79bafb765a813fb35f221dc017534dc2d7ccf57a 100644
--- a/chrome/browser/google/google_url_tracker_infobar_delegate.cc
+++ b/chrome/browser/google/google_url_tracker_infobar_delegate.cc
@@ -5,12 +5,10 @@
 #include "chrome/browser/google/google_url_tracker_infobar_delegate.h"
 
 #include "chrome/browser/google/google_url_tracker.h"
+#include "chrome/browser/google/google_url_tracker_navigation_helper.h"
 #include "chrome/browser/google/google_util.h"
-#include "chrome/browser/infobars/infobar_service.h"
 #include "components/infobars/core/infobar.h"
-#include "content/public/browser/navigation_entry.h"
-#include "content/public/browser/page_navigator.h"
-#include "content/public/browser/web_contents.h"
+#include "components/infobars/core/infobar_manager.h"
 #include "grit/generated_resources.h"
 #include "net/base/net_util.h"
 #include "ui/base/l10n/l10n_util.h"
@@ -18,12 +16,13 @@
 
 // static
 infobars::InfoBar* GoogleURLTrackerInfoBarDelegate::Create(
-    InfoBarService* infobar_service,
+    infobars::InfoBarManager* infobar_manager,
     GoogleURLTracker* google_url_tracker,
+    GoogleURLTrackerNavigationHelper* navigation_helper,
     const GURL& search_url) {
-  return infobar_service->AddInfoBar(ConfirmInfoBarDelegate::CreateInfoBar(
+  return infobar_manager->AddInfoBar(ConfirmInfoBarDelegate::CreateInfoBar(
       scoped_ptr<ConfirmInfoBarDelegate>(new GoogleURLTrackerInfoBarDelegate(
-          google_url_tracker, search_url))));
+          google_url_tracker, navigation_helper, search_url))));
 }
 
 bool GoogleURLTrackerInfoBarDelegate::Accept() {
@@ -58,23 +57,21 @@ void GoogleURLTrackerInfoBarDelegate::Close(bool redo_search) {
     new_search_url = search_url_.ReplaceComponents(replacements);
   }
 
-  content::WebContents* contents =
-      InfoBarService::WebContentsFromInfoBar(infobar());
+  GoogleURLTrackerNavigationHelper* navigation_helper = navigation_helper_;
   infobar()->RemoveSelf();
   // WARNING: |this| may be deleted at this point!  Do not access any members!
 
-  if (new_search_url.is_valid()) {
-    contents->OpenURL(content::OpenURLParams(
-        new_search_url, content::Referrer(), CURRENT_TAB,
-        content::PAGE_TRANSITION_GENERATED, false));
-  }
+  if (new_search_url.is_valid())
+    navigation_helper->OpenURL(new_search_url, CURRENT_TAB, false);

[Peter Kasting, 2014/05/19 13:59:22]

I don't think this is safe.

RemoveSelf() above can cause GoogleURLTrackerMapEntry::Observe() to be
synchronously notified with NOTIFICATION_TAB_CONTENTS_INFOBAR_REMOVED if the
infobar isn't currently visible (e.g. it's in a background tab).  That in turn
deletes the map entry, which owns the navigation helper you're dereferencing
here.

You probably didn't see this if you manually tested because if the infobar _is_
currently visible, the notification will be asynchronous rather than
synchronous, and everything will work.

Either you need to use refcounting, or you need to change how the navigation
helper is owned, or you need to figure out a different way to plumb this
OpenURL() call.

[blundell, 2014/05/19 14:06:54]

Oof, yes you're right. I'm inclined to go back to the approach of having the
NavigationHelper be a WebContentsUserData. Thoughts?

 On 2014/05/19 13:59:22, Peter Kasting wrote:

[Peter Kasting, 2014/05/19 14:16:25]

Off-the-cuff ideas, you should second-guess me as I don't have all the object
lifetimes paged in to my brain:

Seems like another way would be, instead of being given the navigation helper in
the constructor, instead get a pointer to the map entry; then in this function,
call a map entry function that returns a
scoped_ptr<GoogleURLTrackerNavigationHelper> -- in other words, take the nav
helper away from the map entry, and hold it in a local variable in this
function.  That will keep it alive long enough to call OpenURL() on.

Yet another way might be for the map entry to pass over ownership of the
navigation helper at the time this infobar delegate is created -- from then on,
anyone who wants the nav helper gets it from the infobar.  (Maybe the map entry
has a helper function that does this access if necessary to shield the details
from callers?)  I haven't thought much about who needs to get at the nav helper
when to determine what the simplest model is.

You could, of course, go all the way back to WebContentsUserData, but I hate
stuffing things there -- it feels like a dumping ground, and it sucks having
this long-lived class existing on every web contents just on the off chance you
need a copy here.

 }
 
 GoogleURLTrackerInfoBarDelegate::GoogleURLTrackerInfoBarDelegate(
     GoogleURLTracker* google_url_tracker,
+    GoogleURLTrackerNavigationHelper* navigation_helper,
     const GURL& search_url)
     : ConfirmInfoBarDelegate(),
       google_url_tracker_(google_url_tracker),
+      navigation_helper_(navigation_helper),
       search_url_(search_url),
       pending_id_(0) {
 }
@@ -107,14 +104,12 @@ base::string16 GoogleURLTrackerInfoBarDelegate::GetLinkText() const {
 
 bool GoogleURLTrackerInfoBarDelegate::LinkClicked(
     WindowOpenDisposition disposition) {
-  InfoBarService::WebContentsFromInfoBar(infobar())->OpenURL(
-      content::OpenURLParams(
-          google_util::AppendGoogleLocaleParam(GURL(
-              "https://www.google.com/support/chrome/bin/answer.py?"
-              "answer=1618699")),
-          content::Referrer(),
-          (disposition == CURRENT_TAB) ? NEW_FOREGROUND_TAB : disposition,
-          content::PAGE_TRANSITION_LINK, false));
+  navigation_helper_->OpenURL(
+      google_util::AppendGoogleLocaleParam(GURL(
+          "https://www.google.com/support/chrome/bin/answer.py?"
+          "answer=1618699")),
+      (disposition == CURRENT_TAB) ? NEW_FOREGROUND_TAB : disposition,
+      true);
   return false;
 }