Make TabContents own its infobar delegates....

Make TabContents own its infobar delegates.

Also does a bunch of random cleanup of container access.

BUG=62154
TEST=Infobars don't leak

[Peter Kasting, 2010-11-10 02:08:39]

Here's a shot at making TabContents own its infobar delegates and close them
synchronously on infobar removal.  This also enforces that the TabContents
infobar APIs must be called with non-NULL delegates that (where appropriate)
exist in the delegate list.

I haven't tested this.  It might break the universe.  But it's conceptually
simpler than changing the infobar container.

This also doesn't have the Mac and GTK side changes.

Sorry about the vector iterator stuff, I got kinda carried away.  I was hoping
to kill the public accessor functions but they seem to be used in a number of
places right now...

[Peter Kasting, 2010-11-10 02:33:37]

New snap attempts to support Mac and GTK and fix some problems in the first
take.

I do not know what the Mac awakeFromNib() function is called, though, so I don't
know whether I need to worry about the problem that Views and GTK can have where
the layout functionality is run after the delegate is gone.

[tfarina, 2010-11-10 13:11:57]

http://codereview.chromium.org/4767001/diff/7001/chrome/browser/gtk/infobar_g...
File chrome/browser/gtk/infobar_gtk.cc (right):

http://codereview.chromium.org/4767001/diff/7001/chrome/browser/gtk/infobar_g...
chrome/browser/gtk/infobar_gtk.cc:358: if (link_info_bar->delegate_ &&
Why the check here and else here? Ah, OK because in AnimateClose you are
assigning the delegate to NULL.

http://codereview.chromium.org/4767001/diff/7001/chrome/browser/gtk/infobar_g...
chrome/browser/gtk/infobar_gtk.cc:360:
LinkClicked(gtk_util::DispositionForCurrentButtonPressEvent()))
Please, do not remove {} around the if clause.

http://codereview.chromium.org/4767001/diff/7001/chrome/browser/gtk/infobar_g...
chrome/browser/gtk/infobar_gtk.cc:440:
gtk_util::DispositionForCurrentButtonPressEvent()))
Please, do not remove the {} around the if clause here too. Also keep the 4
spaces indentation that had before.

http://codereview.chromium.org/4767001/diff/7001/chrome/browser/gtk/infobar_g...
File chrome/browser/gtk/infobar_gtk.h (right):

http://codereview.chromium.org/4767001/diff/7001/chrome/browser/gtk/infobar_g...
chrome/browser/gtk/infobar_gtk.h:1: // Copyright (c) 2009 The Chromium Authors.
All rights reserved.
nit: 2010

http://codereview.chromium.org/4767001/diff/7001/chrome/browser/tab_contents/...
File chrome/browser/tab_contents/tab_contents.cc (right):

http://codereview.chromium.org/4767001/diff/7001/chrome/browser/tab_contents/...
chrome/browser/tab_contents/tab_contents.cc:1177: return
(render_manager_.dom_ui() == NULL) ?
This change to ternary operator just make it *less* readable. I really don't
like the change to ternary operator when it does not fit into one line. The same
thing below. Also if the check is not wrong, why change it at all? It's just
increasing the size of the diff.

http://codereview.chromium.org/4767001/diff/7001/chrome/browser/tab_contents/...
chrome/browser/tab_contents/tab_contents.cc:2409: while
(!infobar_delegates_.empty())
What is wrong with the for loop? I guess this is superfluous.

http://codereview.chromium.org/4767001/diff/7001/chrome/browser/tab_contents/...
File chrome/browser/tab_contents/tab_contents.h (right):

http://codereview.chromium.org/4767001/diff/7001/chrome/browser/tab_contents/...
chrome/browser/tab_contents/tab_contents.h:473: return
infobar_delegates_[index];
hum? Why? What is the benefit?

http://codereview.chromium.org/4767001/diff/7001/chrome/browser/ui/views/info...
File chrome/browser/ui/views/infobars/infobar_container.cc (right):

http://codereview.chromium.org/4767001/diff/7001/chrome/browser/ui/views/info...
chrome/browser/ui/views/infobars/infobar_container.cc:107: typedef
std::pair<InfoBarDelegate*, InfoBarDelegate*> Delegates;
Was this change really necessary? What is wrong with the later code?

[Peter Kasting, 2010-11-10 19:03:42]

tfarina, please do not lecture me, or anyone else, about Chromium style unless
you have something that is explicitly disallowed by the style guide.  None of
the things you complain about are wrong.

Plus, if you can't see why typedefs improve readability, and don't know the
difference between std::vector::at() and std::vector::operator[](), you should
probably not complain about those things at all.

[Ben Goodger (Google), 2010-11-10 21:30:59]

I am not the best person to review cocoa/gtk changes so rohit/estade/erg should
take a look.

Otherwise LGTM on the other bits.

http://codereview.chromium.org/4767001/diff/7001/chrome/browser/tab_contents/...
File chrome/browser/tab_contents/tab_contents.h (right):

http://codereview.chromium.org/4767001/diff/7001/chrome/browser/tab_contents/...
chrome/browser/tab_contents/tab_contents.h:461: virtual void
AddInfoBar(InfoBarDelegate* delegate);
Can you add a note here about ownership/lifetime now that you've made it
consistent?

[Peter Kasting, 2010-11-11 22:23:24]

Added a number of reviewers.  Also Ben see below.

* Rohit: Mac code.  Please note my question above (second message in this
review's thread) about awakeFromNib() and whether layout can run after the
delegate is gone.

* Evan: GTK code

* Marcus: Geolocation code.  The main change was to switch from iterators to
indexes in a few cases where synchronous erasure makes iterators hard (or
impossible) to use safely.  I also fixed a few style issues like improper
function argument wrapping and "<<" alignment.  The unittest code had to change
how it handled infobar delegate destruction; hopefully the new approach, which
uses a simple listener, is also a bit simpler and safer.

* Ben: Please re-review the stuff not covered above; I've made some changes to
TabContents since the last patch to fix some bugs that cropped up in testing.

[Evan Stade, 2010-11-11 22:39:47]

*gtk* lgtm

http://codereview.chromium.org/4767001/diff/38001/chrome/browser/gtk/infobar_...
File chrome/browser/gtk/infobar_container_gtk.cc (right):

http://codereview.chromium.org/4767001/diff/38001/chrome/browser/gtk/infobar_...
chrome/browser/gtk/infobar_container_gtk.cc:127: Delegates* delegates =
Details<Delegates>(details).ptr();
nit: can you call it DelegatePair?

http://codereview.chromium.org/4767001/diff/38001/chrome/browser/gtk/infobar_...
File chrome/browser/gtk/infobar_gtk.cc (left):

http://codereview.chromium.org/4767001/diff/38001/chrome/browser/gtk/infobar_...
chrome/browser/gtk/infobar_gtk.cc:359:
LinkClicked(gtk_util::DispositionForCurrentButtonPressEvent())) {
I understand that leaving off the {} in this case is not "wrong" according to
the style guide, but I'd appreciate it if you refrained changing this from one
not-wrong style to another not-wrong style to suit your personal preference.

[Peter Kasting, 2010-11-11 22:47:57]

http://codereview.chromium.org/4767001/diff/38001/chrome/browser/gtk/infobar_...
File chrome/browser/gtk/infobar_container_gtk.cc (right):

http://codereview.chromium.org/4767001/diff/38001/chrome/browser/gtk/infobar_...
chrome/browser/gtk/infobar_container_gtk.cc:127: Delegates* delegates =
Details<Delegates>(details).ptr();
On 2010/11/11 22:39:47, Evan Stade wrote:
> nit: can you call it DelegatePair?

Good idea.  Changed typedef and variable names here and elsewhere in the patch.

http://codereview.chromium.org/4767001/diff/38001/chrome/browser/gtk/infobar_...
File chrome/browser/gtk/infobar_gtk.cc (left):

http://codereview.chromium.org/4767001/diff/38001/chrome/browser/gtk/infobar_...
chrome/browser/gtk/infobar_gtk.cc:359:
LinkClicked(gtk_util::DispositionForCurrentButtonPressEvent())) {
On 2010/11/11 22:39:47, Evan Stade wrote:
> I understand that leaving off the {} in this case is not "wrong" according to
> the style guide, but I'd appreciate it if you refrained changing this from one
> not-wrong style to another not-wrong style to suit your personal preference.

Sorry!  Reverted here and below.

[bulach, 2010-11-12 12:03:10]

geolocation LGTM, and indeed the tests are much simpler.

thanks, Peter!

[rohitrao (ping after 24h), 2010-11-12 13:47:38]

Mac changes LGTM

awakeFromNib is called exactly once per infobar, when the view is first loaded
from the nib file.  In concrete terms, this will happen at some point during
-[InfoBarContainerController addInfoBar].

[Ben Goodger (Google), 2010-11-12 18:56:43]

OK. I think your changes to RemoveInfoBar are valid. LGTM.

http://codereview.chromium.org/4767001/diff/59001/chrome/browser/tab_contents...
File chrome/browser/tab_contents/tab_contents.cc (right):

http://codereview.chromium.org/4767001/diff/59001/chrome/browser/tab_contents...
chrome/browser/tab_contents/tab_contents.cc:1566: // iterators, as the
RemvoeInfoBar() call synchronously modifies our delegate
Remove

[Peter Kasting, 2010-11-15 21:26:03]

Yet more reviewers:

* Finnur: Windows/GTK extension infobars
* Andrew: Mac extension infobars
* Jay: Windows/GTK translate infobars
* Michael: Making the Mac translate infobar changes daunted me.  Do you think
you could help me figure out how to do them?

For all of you: Please do not assume I know what I'm doing -- this needs real
review.  The context here is that the ownership model for InfoBarDelegates is
changing, and they will be synchronously deleted when the infobar is told to
close, meaning that that various infobar views (which animate close) will be
outliving their delegates.  Thus everything has to be made safe in a world in
which InfoBar::delegate() can return NULL.

[Finnur, 2010-11-15 23:08:41]

Looks like you have some test failures, but here are my comments.

http://codereview.chromium.org/4767001/diff/118001/chrome/browser/ui/views/in...
File chrome/browser/ui/views/infobars/extension_infobar.cc (left):

http://codereview.chromium.org/4767001/diff/118001/chrome/browser/ui/views/in...
chrome/browser/ui/views/infobars/extension_infobar.cc:77: return;
So, given the new model, there is no chance of it being in a closing state when
we get notified here by the ExtensionView?

http://codereview.chromium.org/4767001/diff/118001/chrome/browser/ui/views/in...
chrome/browser/ui/views/infobars/extension_infobar.cc:113: return;  // The
delegate can go away while we asynchronously load images.
This should be OK I guess given that it is guaranteed to outlive the view in the
new world order.

http://codereview.chromium.org/4767001/diff/118001/chrome/browser/ui/views/in...
File chrome/browser/ui/views/infobars/extension_infobar.cc (right):

http://codereview.chromium.org/4767001/diff/118001/chrome/browser/ui/views/in...
chrome/browser/ui/views/infobars/extension_infobar.cc:64: DCHECK(delegate());
nit: This is redundant and just adds cruft to the debug build. You'll notice it
is NULL on the next line anyway.

http://codereview.chromium.org/4767001/diff/118001/chrome/browser/ui/views/in...
chrome/browser/ui/views/infobars/extension_infobar.cc:150: DCHECK(delegate());
Same here, redundant.

[Peter Kasting, 2010-11-16 01:36:28]

Sigh.  Belay this review.  The latest test failures are not easily solvable with
the current design.  I need to either move to refcounted delegates or weak
pointers.

%)&*(^#