Move GeolocationPermissionContextUnittest from subclassing TabContentsWrapper to merely observing...

Move GeolocationPermissionContextUnittest from subclassing TabContentsWrapper to merely observing infobar removal notifications.  (The existing comment about TestTabContents was made obsolete by r85835.)

Keeping a set of closed delegates instead of tracking the last closed delegate for each tab seems slightly clearer.  Also, back when I first wrote this code in http://codereview.chromium.org/4767001/ , I was convinced it was necessary to the infobar refactor, although I can no longer recall why!

This also eliminates TestTabContentsWrapper::SetContentsWrapper().

BUG=62154
TEST=none
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=88360

[Peter Kasting, 2011-06-03 18:58:43]

Marcus, you already reviewed these changes once as part of
http://codereview.chromium.org/4767001/ ; this set is only slightly different.

[bulach, 2011-06-03 19:15:05]

LGTM

thanks, Peter! a few nits, and there's one potential issue with the set (sorry
for not spotting it earlier) :

http://codereview.chromium.org/6993030/diff/1/chrome/browser/geolocation/geol...
File chrome/browser/geolocation/geolocation_permission_context_unittest.cc
(right):

http://codereview.chromium.org/6993030/diff/1/chrome/browser/geolocation/geol...
chrome/browser/geolocation/geolocation_permission_context_unittest.cc:112:
scoped_ptr<ClosedDelegateTracker> closed_delegate_tracker_;
nit: it no longer depends on anything for lazy initialization, you may as well
just have a regular member instead of a scoped_ptr.

http://codereview.chromium.org/6993030/diff/1/chrome/browser/geolocation/geol...
chrome/browser/geolocation/geolocation_permission_context_unittest.cc:125: :
TabContentsWrapperTestHarness(),
nit: unnecessary?

http://codereview.chromium.org/6993030/diff/1/chrome/browser/geolocation/geol...
chrome/browser/geolocation/geolocation_permission_context_unittest.cc:379:
EXPECT_EQ(2U, closed_delegate_tracker_->size());
this may fail: infobar_0 may just get the same address as removed_infobar, and
the set will contain a single entry.
since it's a tiny amount, we may get away with using a vector: size would be 2,
and it wouldn't matter which one we matched..

http://codereview.chromium.org/6993030/diff/1/chrome/browser/geolocation/geol...
chrome/browser/geolocation/geolocation_permission_context_unittest.cc:423:
EXPECT_EQ(2U, closed_delegate_tracker_->size());
ditto

[Peter Kasting, 2011-06-03 20:37:36]

New snap up.

http://codereview.chromium.org/6993030/diff/1/chrome/browser/geolocation/geol...
File chrome/browser/geolocation/geolocation_permission_context_unittest.cc
(right):

http://codereview.chromium.org/6993030/diff/1/chrome/browser/geolocation/geol...
chrome/browser/geolocation/geolocation_permission_context_unittest.cc:125: :
TabContentsWrapperTestHarness(),
On 2011/06/03 19:15:05, bulach wrote:
> nit: unnecessary?

The compiler will do it implicitly, but I prefer to make the construction
explicit.  This also matches the style in TabContentsWrapperTestHarness()
itself.

http://codereview.chromium.org/6993030/diff/1/chrome/browser/geolocation/geol...
chrome/browser/geolocation/geolocation_permission_context_unittest.cc:379:
EXPECT_EQ(2U, closed_delegate_tracker_->size());
On 2011/06/03 19:15:05, bulach wrote:
> this may fail: infobar_0 may just get the same address as removed_infobar, and
> the set will contain a single entry.

I don't see how that's possible; the two infobars are showing simultaneously, so
they can't be the same object.

[Peter Kasting, 2011-06-07 18:29:09]

Ping

[bulach, 2011-06-07 18:42:02]

On 2011/06/07 18:29:09, Peter Kasting wrote:
> Ping

LGTM

(sorry for the delay, I had already LGTM'd and thought you'd land it once the
nits were addressed... go for it!)

[Peter Kasting, 2011-06-07 18:47:44]

On 2011/06/07 18:42:02, bulach wrote:
> On 2011/06/07 18:29:09, Peter Kasting wrote:
> > Ping
> 
> LGTM
> 
> (sorry for the delay, I had already LGTM'd and thought you'd land it once the
> nits were addressed... go for it!)

I normally would, but you had a serious concern that I was confused by.  I'd
been hoping you'd either explain why I was wrong or say you were mistaken...

[bulach, 2011-06-08 14:23:03]

LGTM still

http://codereview.chromium.org/6993030/diff/1/chrome/browser/geolocation/geol...
File chrome/browser/geolocation/geolocation_permission_context_unittest.cc
(right):

http://codereview.chromium.org/6993030/diff/1/chrome/browser/geolocation/geol...
chrome/browser/geolocation/geolocation_permission_context_unittest.cc:379:
EXPECT_EQ(2U, closed_delegate_tracker_->size());
On 2011/06/03 20:37:36, Peter Kasting wrote:
> On 2011/06/03 19:15:05, bulach wrote:
> > this may fail: infobar_0 may just get the same address as removed_infobar,
and
> > the set will contain a single entry.
> 
> I don't see how that's possible; the two infobars are showing simultaneously,
so
> they can't be the same object.

sorry again for the lack of response, I was happy with your explanation :) 
I mistakenly thought that the the infobars would be created sequentially (which
could potentially break the set size) rather than simultaneously (which is safe
as the addresses are never reused).