Replace the virtual InfoBarDelegate::InfoBarClosed() function with a non-virtual one. This is a ...

Replace the virtual InfoBarDelegate::InfoBarClosed() function with a non-virtual one.  This is a step along the way to killing it entirely.  This also adds a lot of OVERRIDE markers and does some other cleanup in a few places.

The original (stupid) design for the delegate class left subclasses great flexibility in how they mapped infobars to delegates.  In practice, no one ever wanted multiple infobars driven off a single delegate, so the mapping was always one-to-one.  As a result, it was always correct for InfoBarClosed() to "delete this", but because the base class did not do so, every subclass needed to.  Most did; the others leaked memory and failed to run their destructors.

This change forces the base class to delete itself.  This fixes the delegate leaks in the couple subclasses that failed to do this.  It also eliminates a lot of copy-and-pasted "delete this" implementations.

Ultimately, we'll be moving to a model where the InfoBar "view" class owns the delegate and deletes it directly, which will eliminate InfoBarClosed() completely.

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

[Peter Kasting, 2011-05-03 23:45:49]

rohitrao: Cocoa stuff
sky: Other

[sky, 2011-05-04 00:03:45]

LGTM

[Peter Kasting, 2011-05-04 21:03:24]

Rohit is OOO.  Nico, could you review the Cocoa bits?

[Nico, 2011-05-04 21:15:52]

LGTM for the changes in b/u/cocoa, I have only nits which you can ignore if you
want.

http://codereview.chromium.org/6926001/diff/2019/chrome/browser/ui/cocoa/info...
File chrome/browser/ui/cocoa/infobars/infobar_container_controller_unittest.mm
(right):

http://codereview.chromium.org/6926001/diff/2019/chrome/browser/ui/cocoa/info...
chrome/browser/ui/cocoa/infobars/infobar_container_controller_unittest.mm:57: //
These infobars own themselves.
This would be more informative if it was "These infobars delete themselves when
XXX happens" (with XXX = "they are closed" (and "happens" removed I guess))

^ Good example of clear writing

Who owns the delegate?

http://codereview.chromium.org/6926001/diff/2019/chrome/browser/ui/cocoa/info...
File chrome/browser/ui/cocoa/infobars/infobar_controller_unittest.mm (right):

http://codereview.chromium.org/6926001/diff/2019/chrome/browser/ui/cocoa/info...
chrome/browser/ui/cocoa/infobars/infobar_controller_unittest.mm:171:
ASSERT_TRUE(delegate_ == NULL);
These would read better if there's a method on the test class named
WasDelegateClosed() that returned delegate == NULL

(also below)

http://codereview.chromium.org/6926001/diff/2019/chrome/browser/ui/cocoa/info...
File chrome/browser/ui/cocoa/infobars/mock_link_infobar_delegate.h (right):

http://codereview.chromium.org/6926001/diff/2019/chrome/browser/ui/cocoa/info...
chrome/browser/ui/cocoa/infobars/mock_link_infobar_delegate.h:16: class
MockLinkInfoBarDelegate : public LinkInfoBarDelegate {
(btw, why are all these mocks mac-specific? that seems like something that can
and should be shared across platforms)

[Peter Kasting, 2011-05-04 22:54:55]

Will land once Mac trybot passes.

http://codereview.chromium.org/6926001/diff/2019/chrome/browser/ui/cocoa/info...
File chrome/browser/ui/cocoa/infobars/infobar_container_controller_unittest.mm
(right):

http://codereview.chromium.org/6926001/diff/2019/chrome/browser/ui/cocoa/info...
chrome/browser/ui/cocoa/infobars/infobar_container_controller_unittest.mm:57: //
These infobars own themselves.
On 2011/05/04 21:15:52, Nico wrote:
> This would be more informative if it was "These infobars delete themselves
when
> XXX happens"

Changed to "These delegates delete themselves when they're told their infobars
have closed."

> Who owns the delegate?

Do you ask this because the old comment said "these infobars" rather than "these
delegates"?  Hopefully now things are clear?

http://codereview.chromium.org/6926001/diff/2019/chrome/browser/ui/cocoa/info...
File chrome/browser/ui/cocoa/infobars/infobar_controller_unittest.mm (right):

http://codereview.chromium.org/6926001/diff/2019/chrome/browser/ui/cocoa/info...
chrome/browser/ui/cocoa/infobars/infobar_controller_unittest.mm:171:
ASSERT_TRUE(delegate_ == NULL);
On 2011/05/04 21:15:52, Nico wrote:
> These would read better if there's a method on the test class named
> WasDelegateClosed() that returned delegate == NULL

Added "delegate_closed()".

http://codereview.chromium.org/6926001/diff/2019/chrome/browser/ui/cocoa/info...
File chrome/browser/ui/cocoa/infobars/mock_link_infobar_delegate.h (right):

http://codereview.chromium.org/6926001/diff/2019/chrome/browser/ui/cocoa/info...
chrome/browser/ui/cocoa/infobars/mock_link_infobar_delegate.h:16: class
MockLinkInfoBarDelegate : public LinkInfoBarDelegate {
On 2011/05/04 21:15:52, Nico wrote:
> (btw, why are all these mocks mac-specific? that seems like something that can
> and should be shared across platforms)

Dunno.  TBH I don't really find infobar_controller_unittest.mm that valuable,
though.

[Nico, 2011-05-04 22:59:08]

Thanks!

On Wed, May 4, 2011 at 3:54 PM,  <pkasting@chromium.org> wrote:
> Will land once Mac trybot passes.
>
>
>
http://codereview.chromium.org/6926001/diff/2019/chrome/browser/ui/cocoa/info...
> File
> chrome/browser/ui/cocoa/infobars/infobar_container_controller_unittest.mm
> (right):
>
>
http://codereview.chromium.org/6926001/diff/2019/chrome/browser/ui/cocoa/info...
> chrome/browser/ui/cocoa/infobars/infobar_container_controller_unittest.mm:57:
> // These infobars own themselves.
> On 2011/05/04 21:15:52, Nico wrote:
>>
>> This would be more informative if it was "These infobars delete
>
> themselves when
>>
>> XXX happens"
>
> Changed to "These delegates delete themselves when they're told their
> infobars have closed."
>
>> Who owns the delegate?
>
> Do you ask this because the old comment said "these infobars" rather
> than "these delegates"?  Hopefully now things are clear?

Yes, and yes.

>
>
http://codereview.chromium.org/6926001/diff/2019/chrome/browser/ui/cocoa/info...
> File chrome/browser/ui/cocoa/infobars/infobar_controller_unittest.mm
> (right):
>
>
http://codereview.chromium.org/6926001/diff/2019/chrome/browser/ui/cocoa/info...
> chrome/browser/ui/cocoa/infobars/infobar_controller_unittest.mm:171:
> ASSERT_TRUE(delegate_ == NULL);
> On 2011/05/04 21:15:52, Nico wrote:
>>
>> These would read better if there's a method on the test class named
>> WasDelegateClosed() that returned delegate == NULL
>
> Added "delegate_closed()".
>
>
http://codereview.chromium.org/6926001/diff/2019/chrome/browser/ui/cocoa/info...
> File chrome/browser/ui/cocoa/infobars/mock_link_infobar_delegate.h
> (right):
>
>
http://codereview.chromium.org/6926001/diff/2019/chrome/browser/ui/cocoa/info...
> chrome/browser/ui/cocoa/infobars/mock_link_infobar_delegate.h:16: class
> MockLinkInfoBarDelegate : public LinkInfoBarDelegate {
> On 2011/05/04 21:15:52, Nico wrote:
>>
>> (btw, why are all these mocks mac-specific? that seems like something
>
> that can
>>
>> and should be shared across platforms)
>
> Dunno.  TBH I don't really find infobar_controller_unittest.mm that
> valuable, though.
>
> http://codereview.chromium.org/6926001/
>