Commit 4fc8dcd1 authored by pkasting@chromium.org's avatar pkasting@chromium.org

Fix a number of problems with the GoogleURLTracker, mostly around the...

Fix a number of problems with the GoogleURLTracker, mostly around the multiple-searches-simultaneously use case:

* The old code used members to track things like "what's the search URL".  But since there's one GoogleURLTracker and potentially many simultaneously open tabs doing searches, this caused problems with searches overwriting the data for previous searches, leading to infobars that would redo the wrong search.  The new code stuffs this sort of data into the infobar delegate so it's safe to have many simultaneous infobars, which are now tracked in a map.

* Related to the above issue, we'd call registrar_.RemoveAll() any time an infobar was closed or we got a committed or closed notification.  But since multiple tabs could in theory be listening for notifications, this was wrong.  This could cause infobars to sometimes not appear when doing searches in multiple tabs.  Now we are more surgical with our notifications.

* Made accepting one infobar close all of them and trigger all of them to re-search.  Similarly, canceling any one infobar now cancels them all.

* The unittest code has been made more robust, if a bit confusing. I changed the way the helper code provided mock search domain responses to make it safe to provide a response whether or not a query had actually happened.  This way if something goes wrong and we don't query at the places we think we should, we won't have hangs or data corruption or anything else.  The downside here is that in order to avoid actually needing a WebContents*, NavigationController*, InfoBarTabHelper*, etc. to be constructed, I instead use simple ints that are reinterpret_cast<>()ed to the various types to serve as unique pointers.  The GoogleURLTracker itself doesn't actually deref these pointers, so this is safe, and it neatly sidesteps having to mock out a bunch of different things, but it's fairly unorthodox.

* More unittests for cases that the original code didn't cover, e.g. multiple simultaneous infobars.

This does NOT yet deal with a major cause of bug 110158 that the GoogleURLTracker does not correctly handle instant (and maybe also prerendering).  I wanted to make the underlying framework more sane and robust before trying to fix that.

BUG=54274,110158
TEST=Moving to different countries causes a "you moved" infobar to trigger when the user does a search.  No matter how many tabs have infobars, accepting or rejecting the new Google TLD should close them all, and if the TLD was accepted, redo all their searches on the new TLD.
Review URL: https://chromiumcodereview.appspot.com/4880003

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@133598 0039d316-1c4b-4281-b951-d872f2087c98
parent ac2c63e6
This diff is collapsed.
......@@ -6,6 +6,7 @@
#define CHROME_BROWSER_GOOGLE_GOOGLE_URL_TRACKER_H_
#pragma once
#include <map>
#include <string>
#include "base/gtest_prod_util.h"
......@@ -51,8 +52,8 @@ class GoogleURLTracker : public content::URLFetcherDelegate,
UNIT_TEST_MODE,
};
// Only the main browser process loop should call this, when setting up
// g_browser_process->google_url_tracker_. No code other than the
// Only the main browser process loop and tests should call this, when setting
// up g_browser_process->google_url_tracker_. No code other than the
// GoogleURLTracker itself should actually use
// g_browser_process->google_url_tracker().
explicit GoogleURLTracker(Mode mode);
......@@ -89,14 +90,17 @@ class GoogleURLTracker : public content::URLFetcherDelegate,
friend class GoogleURLTrackerInfoBarDelegate;
friend class GoogleURLTrackerTest;
typedef InfoBarDelegate* (*InfoBarCreator)(InfoBarTabHelper*,
GoogleURLTracker*,
const GURL&);
typedef std::map<const InfoBarTabHelper*,
GoogleURLTrackerInfoBarDelegate*> InfoBarMap;
typedef GoogleURLTrackerInfoBarDelegate* (*InfoBarCreator)(
InfoBarTabHelper* infobar_helper,
const GURL& search_url,
GoogleURLTracker* google_url_tracker,
const GURL& new_google_url);
void AcceptGoogleURL(const GURL& google_url);
void CancelGoogleURL(const GURL& google_url);
void InfoBarClosed();
void RedoSearch();
void InfoBarClosed(const InfoBarTabHelper* infobar_helper);
// Registers consumer interest in getting an updated URL from the server.
// It will be notified as chrome::GOOGLE_URL_UPDATED, so the
......@@ -124,10 +128,15 @@ class GoogleURLTracker : public content::URLFetcherDelegate,
void SearchCommitted();
void OnNavigationPending(const content::NotificationSource& source,
const GURL& pending_url);
void OnNavigationCommittedOrTabClosed(content::WebContents* web_contents,
int type);
void ShowGoogleURLInfoBarIfNecessary(content::WebContents* web_contents);
const content::NotificationSource& contents_source,
InfoBarTabHelper* infobar_helper,
const GURL& search_url);
void OnNavigationCommittedOrTabClosed(
const content::NotificationSource& source,
const content::NotificationSource& contents_source,
const InfoBarTabHelper* infobar_helper,
int type);
void CloseAllInfoBars(bool redo_searches);
content::NotificationRegistrar registrar_;
InfoBarCreator infobar_creator_;
......@@ -151,9 +160,7 @@ class GoogleURLTracker : public content::URLFetcherDelegate,
bool need_to_prompt_; // True if the last fetched Google URL is not
// matched with current user's default Google URL
// nor the last prompted Google URL.
content::NavigationController* controller_;
InfoBarDelegate* infobar_;
GURL search_url_;
InfoBarMap infobar_map_;
DISALLOW_COPY_AND_ASSIGN(GoogleURLTracker);
};
......@@ -164,6 +171,7 @@ class GoogleURLTracker : public content::URLFetcherDelegate,
class GoogleURLTrackerInfoBarDelegate : public ConfirmInfoBarDelegate {
public:
GoogleURLTrackerInfoBarDelegate(InfoBarTabHelper* infobar_helper,
const GURL& search_url,
GoogleURLTracker* google_url_tracker,
const GURL& new_google_url);
......@@ -173,11 +181,18 @@ class GoogleURLTrackerInfoBarDelegate : public ConfirmInfoBarDelegate {
virtual string16 GetLinkText() const OVERRIDE;
virtual bool LinkClicked(WindowOpenDisposition disposition) OVERRIDE;
// These are virtual so test code can override them in a subclass.
virtual void Show();
virtual void Close(bool redo_search);
protected:
virtual ~GoogleURLTrackerInfoBarDelegate();
InfoBarTabHelper* map_key_; // What |google_url_tracker_| uses to track us.
const GURL search_url_;
GoogleURLTracker* google_url_tracker_;
const GURL new_google_url_;
bool showing_; // True if this delegate has been added to a TabContents.
private:
// ConfirmInfoBarDelegate:
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment