Make infobars ignore button clicks when closing.

Make infobars ignore button clicks when closing.

This also fixes an unfiled bug where clicking the "deny" button on the Mac before translate infobar would double-count the denial.

BUG=54253, 90985
TEST=none
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=103368

[Peter Kasting, 2011-09-22 00:53:26]

sky: views
erg: gtk
rsesek: cocoa

The goal of this patch is that once an infobar is removed from the
TabContentsWrapper, all buttons/links/menus etc. become inactive.  (Ideally,
menus immediately close as well.)  I was not totally sure about menu-handling on
GTK and not really sure about almost anything on Mac so if this doesn't achieve
that please yell.

erg, why is it that GTK's base infobar class seems to have both a "link" and a
"link button" concept, and LinkInfoBarGtk uses one while ConfirmInfoBarGtk uses
the other?

rsesek, why does the translate infobar disable a bunch of menus in
-infobarWillClose, but the extension infobar seemingly does menus via some other
method and doesn't disable them?

[sky, 2011-09-22 16:48:30]

views LGTM

[Robert Sesek, 2011-09-22 16:49:55]

On 2011/09/22 00:53:26, Peter Kasting wrote: 
> rsesek, why does the translate infobar disable a bunch of menus in
> -infobarWillClose, but the extension infobar seemingly does menus via some
other
> method and doesn't disable them?

Probably because we haven't seen crashes related to the extension infobars
(they're still experimental, right?), and so the fix was not applied there. The
translate infobars have given us menu-related crashes, so they were explicitly
disabled on close there.

http://codereview.chromium.org/7981045/diff/1/chrome/browser/ui/cocoa/infobar...
File chrome/browser/ui/cocoa/infobars/infobar_controller.h (left):

http://codereview.chromium.org/7981045/diff/1/chrome/browser/ui/cocoa/infobar...
chrome/browser/ui/cocoa/infobars/infobar_controller.h:58: -
(IBAction)dismiss:(id)sender;
This needs to stay part of the public header because the XIB file (user
interface layout) still calls through this method.

http://codereview.chromium.org/7981045/diff/1/chrome/browser/ui/cocoa/infobar...
File chrome/browser/ui/cocoa/infobars/infobar_controller.h (right):

http://codereview.chromium.org/7981045/diff/1/chrome/browser/ui/cocoa/infobar...
chrome/browser/ui/cocoa/infobars/infobar_controller.h:53: - (bool)owned;
This should be an Obj-C BOOL and be YES/NO instead of true/false.

http://codereview.chromium.org/7981045/diff/1/chrome/browser/ui/cocoa/transla...
File chrome/browser/ui/cocoa/translate/before_translate_infobar_controller.mm
(right):

http://codereview.chromium.org/7981045/diff/1/chrome/browser/ui/cocoa/transla...
chrome/browser/ui/cocoa/translate/before_translate_infobar_controller.mm:110:
return;  // We're closing; don't call anything, it might access the owner.
I'm not sure this copy-pasted comment is really necessary in every place.

[Peter Kasting, 2011-09-22 21:55:25]

erg has a separate CL out to fix the gtk side of this in a cleaner way.  Once he
lands that I'll remove the gtk stuff in this patch and re-upload.


On 2011/09/22 16:49:55, rsesek wrote:
> On 2011/09/22 00:53:26, Peter Kasting wrote: 
> > rsesek, why does the translate infobar disable a bunch of menus in
> > -infobarWillClose, but the extension infobar seemingly does menus via some
> other
> > method and doesn't disable them?
> 
> Probably because we haven't seen crashes related to the extension infobars
> (they're still experimental, right?), and so the fix was not applied there.
The
> translate infobars have given us menu-related crashes, so they were explicitly
> disabled on close there.

OK.  What needs to happen at this point is that all menus need to be closed once
the infobar becomes unowned, and then the user should not be able to reopen them
(clicking the relevant buttons should just do nothing).  Can you either send me
a diff that does that or tell me what I need to change in order to accomplish
it?

http://codereview.chromium.org/7981045/diff/1/chrome/browser/ui/cocoa/infobar...
File chrome/browser/ui/cocoa/infobars/infobar_controller.h (left):

http://codereview.chromium.org/7981045/diff/1/chrome/browser/ui/cocoa/infobar...
chrome/browser/ui/cocoa/infobars/infobar_controller.h:58: -
(IBAction)dismiss:(id)sender;
On 2011/09/22 16:49:56, rsesek wrote:
> This needs to stay part of the public header because the XIB file (user
> interface layout) still calls through this method.

sail informed me that it was safe to move this to private and I wouldn't break
the IB connection.  He also said there was some extra magic that could be done
to make the private method show up correctly in the IB GUI but that he didn't
know it.  I would like to make this private if I can, do you know the right
voodoo?

http://codereview.chromium.org/7981045/diff/1/chrome/browser/ui/cocoa/infobar...
File chrome/browser/ui/cocoa/infobars/infobar_controller.h (right):

http://codereview.chromium.org/7981045/diff/1/chrome/browser/ui/cocoa/infobar...
chrome/browser/ui/cocoa/infobars/infobar_controller.h:53: - (bool)owned;
On 2011/09/22 16:49:56, rsesek wrote:
> This should be an Obj-C BOOL and be YES/NO instead of true/false.

Done.

http://codereview.chromium.org/7981045/diff/1/chrome/browser/ui/cocoa/transla...
File chrome/browser/ui/cocoa/translate/before_translate_infobar_controller.mm
(right):

http://codereview.chromium.org/7981045/diff/1/chrome/browser/ui/cocoa/transla...
chrome/browser/ui/cocoa/translate/before_translate_infobar_controller.mm:110:
return;  // We're closing; don't call anything, it might access the owner.
On 2011/09/22 16:49:56, rsesek wrote:
> I'm not sure this copy-pasted comment is really necessary in every place.

It does seem redundant, but OTOH, I really don't want to just have no comment in
some places either... this stuff is all subtle :(

[Peter Kasting, 2011-09-23 23:32:31]

New snap up.  GTK has been addressed separately so it's no longer in this patch.

rsesek: Still waiting for answers to the previous questions.

[Robert Sesek, 2011-09-24 19:20:16]

On 2011/09/22 21:55:25, Peter Kasting wrote:
> On 2011/09/22 16:49:55, rsesek wrote:
> > On 2011/09/22 00:53:26, Peter Kasting wrote: 
> > > rsesek, why does the translate infobar disable a bunch of menus in
> > > -infobarWillClose, but the extension infobar seemingly does menus via some
> > other
> > > method and doesn't disable them?
> > 
> > Probably because we haven't seen crashes related to the extension infobars
> > (they're still experimental, right?), and so the fix was not applied there.
> The
> > translate infobars have given us menu-related crashes, so they were
explicitly
> > disabled on close there.
> 
> OK.  What needs to happen at this point is that all menus need to be closed
once
> the infobar becomes unowned, and then the user should not be able to reopen
them
> (clicking the relevant buttons should just do nothing).  Can you either send
me
> a diff that does that or tell me what I need to change in order to accomplish
> it?

I'd maybe move the DisablePopupMenu() in translate_infobar_base.mm to be a
protected method on InfoBarController. You can make it "protected" by putting it
in a category like so after the @end for InfoBarController:

@interface InfoBarController (Protected)
- (void)disablePopupMenu:(NSMenu*);
@end

The implementation need not be in a category, though, and can reside in the same
@implementation block as the rest of the methods.

http://codereview.chromium.org/7981045/diff/1/chrome/browser/ui/cocoa/infobar...
File chrome/browser/ui/cocoa/infobars/infobar_controller.h (left):

http://codereview.chromium.org/7981045/diff/1/chrome/browser/ui/cocoa/infobar...
chrome/browser/ui/cocoa/infobars/infobar_controller.h:58: -
(IBAction)dismiss:(id)sender;
On 2011/09/22 21:55:25, Peter Kasting wrote:
> On 2011/09/22 16:49:56, rsesek wrote:
> > This needs to stay part of the public header because the XIB file (user
> > interface layout) still calls through this method.
> 
> sail informed me that it was safe to move this to private and I wouldn't break
> the IB connection.  He also said there was some extra magic that could be done
> to make the private method show up correctly in the IB GUI but that he didn't
> know it.  I would like to make this private if I can, do you know the right
> voodoo?

It's technically safe, but as some other entity is calling it, I'd consider it
part of the public interface and it should live there. You can document what
you'd prefer code to call and that it exists solely for the XIB.

http://codereview.chromium.org/7981045/diff/1/chrome/browser/ui/cocoa/transla...
File chrome/browser/ui/cocoa/translate/before_translate_infobar_controller.mm
(right):

http://codereview.chromium.org/7981045/diff/1/chrome/browser/ui/cocoa/transla...
chrome/browser/ui/cocoa/translate/before_translate_infobar_controller.mm:110:
return;  // We're closing; don't call anything, it might access the owner.
On 2011/09/22 21:55:25, Peter Kasting wrote:
> On 2011/09/22 16:49:56, rsesek wrote:
> > I'm not sure this copy-pasted comment is really necessary in every place.
> 
> It does seem redundant, but OTOH, I really don't want to just have no comment
in
> some places either... this stuff is all subtle :(

How about adding something like it to the declaration of |-owned| instead?

[Peter Kasting, 2011-09-26 22:34:49]

On 2011/09/24 19:20:16, rsesek wrote:
> On 2011/09/22 21:55:25, Peter Kasting wrote:
> > OK.  What needs to happen at this point is that all menus need to be closed
> once
> > the infobar becomes unowned, and then the user should not be able to reopen
> them
> > (clicking the relevant buttons should just do nothing).  Can you either send
> me
> > a diff that does that or tell me what I need to change in order to
accomplish
> > it?
> 
> I'd maybe move the DisablePopupMenu() in translate_infobar_base.mm to be a
> protected method on InfoBarController. You can make it "protected" by putting
it
> in a category like so after the @end for InfoBarController:
> 
> @interface InfoBarController (Protected)
> - (void)disablePopupMenu:(NSMenu*);
> @end
> 
> The implementation need not be in a category, though, and can reside in the
same
> @implementation block as the rest of the methods.

OK.  I tried to do this and hook the extension infobar to it as well.  That
infobar shows its menu using a slightly different method, so please check that I
did the right thing.

http://codereview.chromium.org/7981045/diff/1/chrome/browser/ui/cocoa/infobar...
File chrome/browser/ui/cocoa/infobars/infobar_controller.h (left):

http://codereview.chromium.org/7981045/diff/1/chrome/browser/ui/cocoa/infobar...
chrome/browser/ui/cocoa/infobars/infobar_controller.h:58: -
(IBAction)dismiss:(id)sender;
On 2011/09/24 19:20:16, rsesek wrote:
> It's technically safe, but as some other entity is calling it, I'd consider it
> part of the public interface and it should live there. You can document what
> you'd prefer code to call and that it exists solely for the XIB.

Done.

http://codereview.chromium.org/7981045/diff/1/chrome/browser/ui/cocoa/transla...
File chrome/browser/ui/cocoa/translate/before_translate_infobar_controller.mm
(right):

http://codereview.chromium.org/7981045/diff/1/chrome/browser/ui/cocoa/transla...
chrome/browser/ui/cocoa/translate/before_translate_infobar_controller.mm:110:
return;  // We're closing; don't call anything, it might access the owner.
On 2011/09/24 19:20:16, rsesek wrote:
> How about adding something like it to the declaration of |-owned| instead?

Done.

[Robert Sesek, 2011-09-27 18:21:46]

c/b/u/cocoa/ LGTM with nits

http://codereview.chromium.org/7981045/diff/27001/chrome/browser/ui/cocoa/inf...
File chrome/browser/ui/cocoa/infobars/infobar_controller.h (right):

http://codereview.chromium.org/7981045/diff/27001/chrome/browser/ui/cocoa/inf...
chrome/browser/ui/cocoa/infobars/infobar_controller.h:55: - (BOOL)owned;
nit: this should probably be called -isOwned because it's a boolean getter.

http://codereview.chromium.org/7981045/diff/27001/chrome/browser/ui/cocoa/tra...
File chrome/browser/ui/cocoa/translate/translate_infobar_base.mm (right):

http://codereview.chromium.org/7981045/diff/27001/chrome/browser/ui/cocoa/tra...
chrome/browser/ui/cocoa/translate/translate_infobar_base.mm:504: return;  //
We're closing; don't call anything, it might access the owner.
Remove these copy/pasted comments?

[Peter Kasting, 2011-09-27 18:29:34]

You might want to glance at the spacing in infobar_controller_unittest.mm -- I
had to linebreak a few calls and wasn't sure how best to do it.

http://codereview.chromium.org/7981045/diff/27001/chrome/browser/ui/cocoa/inf...
File chrome/browser/ui/cocoa/infobars/infobar_controller.h (right):

http://codereview.chromium.org/7981045/diff/27001/chrome/browser/ui/cocoa/inf...
chrome/browser/ui/cocoa/infobars/infobar_controller.h:55: - (BOOL)owned;
On 2011/09/27 18:21:46, rsesek wrote:
> nit: this should probably be called -isOwned because it's a boolean getter.

Done.

http://codereview.chromium.org/7981045/diff/27001/chrome/browser/ui/cocoa/tra...
File chrome/browser/ui/cocoa/translate/translate_infobar_base.mm (right):

http://codereview.chromium.org/7981045/diff/27001/chrome/browser/ui/cocoa/tra...
chrome/browser/ui/cocoa/translate/translate_infobar_base.mm:504: return;  //
We're closing; don't call anything, it might access the owner.
On 2011/09/27 18:21:46, rsesek wrote:
> Remove these copy/pasted comments?

Done.

[Robert Sesek, 2011-09-27 18:32:14]

On 2011/09/27 18:29:34, Peter Kasting wrote:
> You might want to glance at the spacing in infobar_controller_unittest.mm -- I
> had to linebreak a few calls and wasn't sure how best to do it.

Yup. That's one of the correct wrapping styles.

LGTM