Split the existing GoogleURLTrackerInfoBarDelegate into two classes

Split the existing GoogleURLTrackerInfoBarDelegate into two classes by adding a MapEntry class.  The MapEntry represents all the metadata about a pending search.  This means that the infobar delegate itself can now be created only when it's going to actually be added to a tab, just like other infobars.  This is necessary for some cleanup I'm doing to infobar calling conventions and ownership.

This also makes each individual class simpler than the old unified class.

There are some other miscellaneous changes in here, like switching the test code magic numbers from int to intptr_t (safer for portability).

BUG=none
TEST=none
TBR=sky
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=162298

[tfarina, 2012-10-13 21:30:02]

http://codereview.chromium.org/11114009/diff/20001/chrome/browser/google/goog...
File chrome/browser/google/google_url_tracker_unittest.cc (right):

http://codereview.chromium.org/11114009/diff/20001/chrome/browser/google/goog...
chrome/browser/google/google_url_tracker_unittest.cc:42: GURL search_url() const
{ return search_url_; }
nit: const GURL& ?

http://codereview.chromium.org/11114009/diff/20001/chrome/browser/google/goog...
chrome/browser/google/google_url_tracker_unittest.cc:137: virtual void SetUp();
nit: while here, add OVERRIDE.

[Peter Kasting, 2012-10-16 01:19:50]

isherman: ping

[Ilya Sherman, 2012-10-16 20:16:24]

I like the direction of this cleanup :)

https://chromiumcodereview.appspot.com/11114009/diff/20001/chrome/browser/goo...
File chrome/browser/google/google_url_tracker.cc (right):

https://chromiumcodereview.appspot.com/11114009/diff/20001/chrome/browser/goo...
chrome/browser/google/google_url_tracker.cc:225: }
nit: No longer a need for curly braces for this case

https://chromiumcodereview.appspot.com/11114009/diff/20001/chrome/browser/goo...
File chrome/browser/google/google_url_tracker.h (left):

https://chromiumcodereview.appspot.com/11114009/diff/20001/chrome/browser/goo...
chrome/browser/google/google_url_tracker.h:207: // configure to use https in
search engine templates.
Is this being removed because it's no longer appropriate, because you've
implemented it, or some other reason?

https://chromiumcodereview.appspot.com/11114009/diff/20001/chrome/browser/goo...
File chrome/browser/google/google_url_tracker.h (right):

https://chromiumcodereview.appspot.com/11114009/diff/20001/chrome/browser/goo...
chrome/browser/google/google_url_tracker.h:96: friend class
GoogleURLTrackerInfoBarDelegate;
Whoa, I just noticed this.  How 'bout we expose public getters for the bits of
internals that the infobar delegate needs, and drop this friend declaration?

https://chromiumcodereview.appspot.com/11114009/diff/20001/chrome/browser/goo...
chrome/browser/google/google_url_tracker.h:99: class MapEntry;
How about defining this in a separate header file?

https://chromiumcodereview.appspot.com/11114009/diff/20001/chrome/browser/goo...
chrome/browser/google/google_url_tracker.h:256: InfoBarTabHelper*
infobar_helper_;
nit: Can these be const members?

https://chromiumcodereview.appspot.com/11114009/diff/20001/chrome/browser/goo...
chrome/browser/google/google_url_tracker.h:259: content::NotificationSource
web_contents_source_;
nit: Can these be const members?

https://chromiumcodereview.appspot.com/11114009/diff/20001/chrome/browser/goo...
File chrome/browser/google/google_url_tracker_infobar_delegate.cc (left):

https://chromiumcodereview.appspot.com/11114009/diff/20001/chrome/browser/goo...
chrome/browser/google/google_url_tracker_infobar_delegate.cc:64:
net::StripWWWFromHost(new_google_url));
nit: Is this DCHECK worth keeping around, in the case that the URL is being
updated rather than set for the first time?

https://chromiumcodereview.appspot.com/11114009/diff/20001/chrome/browser/goo...
File chrome/browser/google/google_url_tracker_infobar_delegate.h (right):

https://chromiumcodereview.appspot.com/11114009/diff/20001/chrome/browser/goo...
chrome/browser/google/google_url_tracker_infobar_delegate.h:41: int pending_id_;
nit: Class members should always have private visibility, not protected
visibility.

https://chromiumcodereview.appspot.com/11114009/diff/20001/chrome/browser/goo...
File chrome/browser/google/google_url_tracker_unittest.cc (right):

https://chromiumcodereview.appspot.com/11114009/diff/20001/chrome/browser/goo...
chrome/browser/google/google_url_tracker_unittest.cc:33: GoogleURLTrackerTest*
g_test = NULL;
Wait, what?  How about storing this as a data member in the TestInfoBarDelegate
class instead?

https://chromiumcodereview.appspot.com/11114009/diff/20001/chrome/browser/goo...
chrome/browser/google/google_url_tracker_unittest.cc:55: };
nit: Leave a blank line after this one.

https://chromiumcodereview.appspot.com/11114009/diff/20001/chrome/browser/goo...
chrome/browser/google/google_url_tracker_unittest.cc:204: observer_(new
TestNotificationObserver) {
nit: Why not just store the observer_ by value, rather than wrapped in a
scoped_ptr?

[Peter Kasting, 2012-10-16 23:29:35]

PTAL.

http://codereview.chromium.org/11114009/diff/20001/chrome/browser/google/goog...
File chrome/browser/google/google_url_tracker.cc (right):

http://codereview.chromium.org/11114009/diff/20001/chrome/browser/google/goog...
chrome/browser/google/google_url_tracker.cc:225: }
On 2012/10/16 20:16:24, Ilya Sherman wrote:
> nit: No longer a need for curly braces for this case

True.

http://codereview.chromium.org/11114009/diff/20001/chrome/browser/google/goog...
File chrome/browser/google/google_url_tracker.h (left):

http://codereview.chromium.org/11114009/diff/20001/chrome/browser/google/goog...
chrome/browser/google/google_url_tracker.h:207: // configure to use https in
search engine templates.
On 2012/10/16 20:16:24, Ilya Sherman wrote:
> Is this being removed because it's no longer appropriate, because you've
> implemented it, or some other reason?

We solved this problem in a different way by supporting HTTPS via
searchdomaincheck.

http://codereview.chromium.org/11114009/diff/20001/chrome/browser/google/goog...
File chrome/browser/google/google_url_tracker.h (right):

http://codereview.chromium.org/11114009/diff/20001/chrome/browser/google/goog...
chrome/browser/google/google_url_tracker.h:96: friend class
GoogleURLTrackerInfoBarDelegate;
On 2012/10/16 20:16:24, Ilya Sherman wrote:
> Whoa, I just noticed this.  How 'bout we expose public getters for the bits of
> internals that the infobar delegate needs, and drop this friend declaration?

OK.  I hadn't done this so far because the current public API amounts to a set
of safe static functions that general consumers of the GoogleURLTracker should
use, whereas all the things the infobar needs are non-static methods that no one
but it should be calling; so I left it as a friend because it's something of an
implementation detail.

Publicizing these means other classes can call them, which we don't really want.
 But I share your aversion to friend declarations, and this would mean that the
infobar itself becomes much more limited in what it can access, which is a good
thing.

http://codereview.chromium.org/11114009/diff/20001/chrome/browser/google/goog...
chrome/browser/google/google_url_tracker.h:99: class MapEntry;
On 2012/10/16 20:16:24, Ilya Sherman wrote:
> How about defining this in a separate header file?

:/ I'm less convinced of the benefits of this, but OK.

http://codereview.chromium.org/11114009/diff/20001/chrome/browser/google/goog...
chrome/browser/google/google_url_tracker.h:256: InfoBarTabHelper*
infobar_helper_;
On 2012/10/16 20:16:24, Ilya Sherman wrote:
> nit: Can these be const members?

Added as much const as possible.

http://codereview.chromium.org/11114009/diff/20001/chrome/browser/google/goog...
chrome/browser/google/google_url_tracker.h:259: content::NotificationSource
web_contents_source_;
On 2012/10/16 20:16:24, Ilya Sherman wrote:
> nit: Can these be const members?

Yes.

http://codereview.chromium.org/11114009/diff/20001/chrome/browser/google/goog...
File chrome/browser/google/google_url_tracker_infobar_delegate.cc (left):

http://codereview.chromium.org/11114009/diff/20001/chrome/browser/google/goog...
chrome/browser/google/google_url_tracker_infobar_delegate.cc:64:
net::StripWWWFromHost(new_google_url));
On 2012/10/16 20:16:24, Ilya Sherman wrote:
> nit: Is this DCHECK worth keeping around, in the case that the URL is being
> updated rather than set for the first time?

We can't actually check this anymore.  The URL that we expect to have not
changed (other than scheme) is google_url_tracker_->fetched_google_url_, and the
old value of that member is not preserved when the member is updated, so no code
here can compare the two.

The equivalent bit of code to this DCHECK is the conditional at the end of
GoogleURLTracker::OnURLFetchComplete() that calls CloseAllInfoBars() if the
stripped hosts do not match.

http://codereview.chromium.org/11114009/diff/20001/chrome/browser/google/goog...
File chrome/browser/google/google_url_tracker_infobar_delegate.h (right):

http://codereview.chromium.org/11114009/diff/20001/chrome/browser/google/goog...
chrome/browser/google/google_url_tracker_infobar_delegate.h:41: int pending_id_;
On 2012/10/16 20:16:24, Ilya Sherman wrote:
> nit: Class members should always have private visibility, not protected
> visibility.

Done.

http://codereview.chromium.org/11114009/diff/20001/chrome/browser/google/goog...
File chrome/browser/google/google_url_tracker_unittest.cc (right):

http://codereview.chromium.org/11114009/diff/20001/chrome/browser/google/goog...
chrome/browser/google/google_url_tracker_unittest.cc:33: GoogleURLTrackerTest*
g_test = NULL;
On 2012/10/16 20:16:24, Ilya Sherman wrote:
> Wait, what?  How about storing this as a data member in the
TestInfoBarDelegate
> class instead?

Achieved this by switching GoogleURLTracker from using a typedefed function
pointer over to using base::Callback, so I could use a member of
GoogleURLTrackerTest as the infobar creator function in this file.

I had been thinking of doing something like this before but dismissed it because
I wasn't sure quite how to do it.  You pushed me to figure it out :)

http://codereview.chromium.org/11114009/diff/20001/chrome/browser/google/goog...
chrome/browser/google/google_url_tracker_unittest.cc:42: GURL search_url() const
{ return search_url_; }
On 2012/10/13 21:30:02, tfarina wrote:
> nit: const GURL& ?

These disappeared in favor of methods on the base class, which do return const
refs.

http://codereview.chromium.org/11114009/diff/20001/chrome/browser/google/goog...
chrome/browser/google/google_url_tracker_unittest.cc:55: };
On 2012/10/16 20:16:24, Ilya Sherman wrote:
> nit: Leave a blank line after this one.

Not doing that was purposeful, but OK.

http://codereview.chromium.org/11114009/diff/20001/chrome/browser/google/goog...
chrome/browser/google/google_url_tracker_unittest.cc:137: virtual void SetUp();
On 2012/10/13 21:30:02, tfarina wrote:
> nit: while here, add OVERRIDE.

Done.

http://codereview.chromium.org/11114009/diff/20001/chrome/browser/google/goog...
chrome/browser/google/google_url_tracker_unittest.cc:204: observer_(new
TestNotificationObserver) {
On 2012/10/16 20:16:24, Ilya Sherman wrote:
> nit: Why not just store the observer_ by value, rather than wrapped in a
> scoped_ptr?

Hmm, I dunno.  Fixed.

[Ilya Sherman, 2012-10-17 00:32:41]

LGTM, thanks

https://codereview.chromium.org/11114009/diff/20001/chrome/browser/google/goo...
File chrome/browser/google/google_url_tracker_unittest.cc (right):

https://codereview.chromium.org/11114009/diff/20001/chrome/browser/google/goo...
chrome/browser/google/google_url_tracker_unittest.cc:55: };
On 2012/10/16 23:29:35, Peter Kasting wrote:
> On 2012/10/16 20:16:24, Ilya Sherman wrote:
> > nit: Leave a blank line after this one.
> 
> Not doing that was purposeful, but OK.

Oh, I think I misread the comment a bit before.  Now that I've properly parsed
it, I'm ok with either leaving or omitting the blank line.

https://codereview.chromium.org/11114009/diff/24001/chrome/browser/google/goo...
File chrome/browser/google/google_url_tracker.cc (right):

https://codereview.chromium.org/11114009/diff/24001/chrome/browser/google/goo...
chrome/browser/google/google_url_tracker.cc:29: 
nit: Spurious blank line?

https://codereview.chromium.org/11114009/diff/24001/chrome/browser/google/goo...
File chrome/browser/google/google_url_tracker_unittest.cc (right):

https://codereview.chromium.org/11114009/diff/24001/chrome/browser/google/goo...
chrome/browser/google/google_url_tracker_unittest.cc:31:
TestInfoBarDelegate(GoogleURLTrackerTest* test_harness,
nit: Seems like this should still need to be forward-declared, no?

[Peter Kasting, 2012-10-17 00:34:28]

https://codereview.chromium.org/11114009/diff/24001/chrome/browser/google/goo...
File chrome/browser/google/google_url_tracker.cc (right):

https://codereview.chromium.org/11114009/diff/24001/chrome/browser/google/goo...
chrome/browser/google/google_url_tracker.cc:29: 
On 2012/10/17 00:32:41, Ilya Sherman wrote:
> nit: Spurious blank line?

I like leaving 2 lines between each class or other self-contained block-o-stuff
:)

https://codereview.chromium.org/11114009/diff/24001/chrome/browser/google/goo...
File chrome/browser/google/google_url_tracker_unittest.cc (right):

https://codereview.chromium.org/11114009/diff/24001/chrome/browser/google/goo...
chrome/browser/google/google_url_tracker_unittest.cc:31:
TestInfoBarDelegate(GoogleURLTrackerTest* test_harness,
On 2012/10/17 00:32:41, Ilya Sherman wrote:
> nit: Seems like this should still need to be forward-declared, no?

Yeah, boo to MSVC for letting me get away without it.  The other bots caught
this.

[Peter Kasting, 2012-10-17 01:42:13]

TBR=sky for chrome/chrome_browser.gypi OWNERS