InfoBarService inherits from InfoBarManager

InfoBarService inherits from InfoBarManager

This allows to change the implementation of InfoBarDelegate::web_contents()
such that it no longer calls InfoBarManager::web_contents() (which will be
removed), but InfoBarService::web_contents() instead.

BUG=354379
TBR=jochen

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=262976

[droger, 2014-04-08 12:10:59]

This CL is work in progress, but core of the change is done.
It implements Peter's design we discussed.

Let me know what you think.

https://codereview.chromium.org/228293004/diff/60001/chrome/browser/infobars/...
File chrome/browser/infobars/infobar_delegate.cc (right):

https://codereview.chromium.org/228293004/diff/60001/chrome/browser/infobars/...
chrome/browser/infobars/infobar_delegate.cc:18: content::WebContents*
WebContentsFromInfoBar(InfoBar* infobar) {
The plan would be to make this helper function public so that any infobar
delegate that needs it can use it instead of InfoBarDelegate::web_contents().

https://codereview.chromium.org/228293004/diff/60001/chrome/browser/infobars/...
File chrome/browser/infobars/infobar_service.h (right):

https://codereview.chromium.org/228293004/diff/60001/chrome/browser/infobars/...
chrome/browser/infobars/infobar_service.h:41: InfoBarManager* infobar_manager()
{ return this; }
This method needs to be removed. I'll do this once we agree on the design, since
it will introduce a lot of noise.

[blundell, 2014-04-08 12:19:03]

Looks fine to me.

https://codereview.chromium.org/228293004/diff/60001/chrome/browser/infobars/...
File chrome/browser/infobars/infobar_manager.cc (right):

https://codereview.chromium.org/228293004/diff/60001/chrome/browser/infobars/...
chrome/browser/infobars/infobar_manager.cc:87: NotifyManagerShuttingDown();
nit: I don't think this one needs to change to be a method.

[droger, 2014-04-08 12:20:53]

https://codereview.chromium.org/228293004/diff/60001/chrome/browser/infobars/...
File chrome/browser/infobars/infobar_manager.cc (right):

https://codereview.chromium.org/228293004/diff/60001/chrome/browser/infobars/...
chrome/browser/infobars/infobar_manager.cc:87: NotifyManagerShuttingDown();
On 2014/04/08 12:19:03, blundell wrote:
> nit: I don't think this one needs to change to be a method.

True. I did it just to be consistent with the other notifications, but it's not
required.

[droger, 2014-04-08 13:39:38]

Actually, the CL does not work in this state because there are calls to virtual
methods during the destructor of InfoBarManager.
These calls do not go through since InfoBarService is already destroyed and the
notification does not fire.

[droger, 2014-04-08 14:52:06]

Alright, bots are happy again.

[droger, 2014-04-08 15:00:11]

pkasting: if you agree with this approach, my plan will be to remove the
InfoBarService::infobar_manager() method as part of this CL, and remove the
InfoBarDelegate::web_contents() method in a follow-up CL.

https://codereview.chromium.org/228293004/diff/80001/chrome/browser/infobars/...
File chrome/browser/infobars/infobar_delegate.cc (right):

https://codereview.chromium.org/228293004/diff/80001/chrome/browser/infobars/...
chrome/browser/infobars/infobar_delegate.cc:18: content::WebContents*
WebContentsFromInfoBar(InfoBar* infobar) {
The plan would be to make this helper function public so that any infobar
delegate that needs it can use it instead of InfoBarDelegate::web_contents().

https://codereview.chromium.org/228293004/diff/80001/chrome/browser/infobars/...
File chrome/browser/infobars/infobar_manager.h (right):

https://codereview.chromium.org/228293004/diff/80001/chrome/browser/infobars/...
chrome/browser/infobars/infobar_manager.h:38: void ShutDown();
This method is needed as there are virtual calls during the destructor
otherwise.
These virtual calls are only needed for notifications, so we should be able to
remove the shutdown method once the notifications are killed.

https://codereview.chromium.org/228293004/diff/80001/chrome/browser/infobars/...
File chrome/browser/infobars/infobar_service.h (right):

https://codereview.chromium.org/228293004/diff/80001/chrome/browser/infobars/...
chrome/browser/infobars/infobar_service.h:41: InfoBarManager* infobar_manager()
{ return this; }
This method should be removed (probably as part of this CL).

[Peter Kasting, 2014-04-08 20:39:56]

https://codereview.chromium.org/228293004/diff/80001/chrome/browser/infobars/...
File chrome/browser/infobars/infobar_delegate.cc (right):

https://codereview.chromium.org/228293004/diff/80001/chrome/browser/infobars/...
chrome/browser/infobars/infobar_delegate.cc:18: content::WebContents*
WebContentsFromInfoBar(InfoBar* infobar) {
On 2014/04/08 15:00:11, droger wrote:
> The plan would be to make this helper function public so that any infobar
> delegate that needs it can use it instead of InfoBarDelegate::web_contents().

I didn't go this route for two reasons:
(1) Callsite verbosity -- web_contents() is a lot simpler than
WebContentsFromInfoBar(this).
(2) Type-checking -- ensuring web_contents() is available due to an inheritance
hierarchy means that the only delegates that can call it are those that inherit
from the appropriate base class.  If this is public static, then any infobar
could theoretically call it, which we don't want.

https://codereview.chromium.org/228293004/diff/80001/chrome/browser/infobars/...
File chrome/browser/infobars/infobar_manager.cc (right):

https://codereview.chromium.org/228293004/diff/80001/chrome/browser/infobars/...
chrome/browser/infobars/infobar_manager.cc:77: did_shut_down_(false),
Nit: Don't add members just for sanity-check purposes.

https://codereview.chromium.org/228293004/diff/80001/chrome/browser/infobars/...
File chrome/browser/infobars/infobar_manager.h (right):

https://codereview.chromium.org/228293004/diff/80001/chrome/browser/infobars/...
chrome/browser/infobars/infobar_manager.h:38: void ShutDown();
On 2014/04/08 15:00:11, droger wrote:
> This method is needed as there are virtual calls during the destructor
> otherwise.
> These virtual calls are only needed for notifications, so we should be able to
> remove the shutdown method once the notifications are killed.

I'd add a comment to that effect.

https://codereview.chromium.org/228293004/diff/80001/chrome/browser/infobars/...
chrome/browser/infobars/infobar_manager.h:85: content::WebContents*
web_contents() { return web_contents_; }
Can this method (and member) be removed too?

[droger, 2014-04-08 20:54:46]

https://codereview.chromium.org/228293004/diff/80001/chrome/browser/infobars/...
File chrome/browser/infobars/infobar_delegate.cc (right):

https://codereview.chromium.org/228293004/diff/80001/chrome/browser/infobars/...
chrome/browser/infobars/infobar_delegate.cc:18: content::WebContents*
WebContentsFromInfoBar(InfoBar* infobar) {
On 2014/04/08 20:39:56, Peter Kasting wrote:
> On 2014/04/08 15:00:11, droger wrote:
> > The plan would be to make this helper function public so that any infobar
> > delegate that needs it can use it instead of
InfoBarDelegate::web_contents().
> 
> I didn't go this route for two reasons:
> (1) Callsite verbosity -- web_contents() is a lot simpler than
> WebContentsFromInfoBar(this).
> (2) Type-checking -- ensuring web_contents() is available due to an
inheritance
> hierarchy means that the only delegates that can call it are those that
inherit
> from the appropriate base class.  If this is public static, then any infobar
> could theoretically call it, which we don't want.

About (2), I don't think we still need to have different base classes for the
infobar delegates anymore (i.e. ContentConfirmInfoBar). The only point of these
base class was to add the web_contents() method, but this is no longer needed
with this design.

https://codereview.chromium.org/228293004/diff/80001/chrome/browser/infobars/...
File chrome/browser/infobars/infobar_manager.h (right):

https://codereview.chromium.org/228293004/diff/80001/chrome/browser/infobars/...
chrome/browser/infobars/infobar_manager.h:85: content::WebContents*
web_contents() { return web_contents_; }
On 2014/04/08 20:39:56, Peter Kasting wrote:
> Can this method (and member) be removed too?

I'd prefer doing it in another CL  if you don't mind, as there are still a few
callers.

[Peter Kasting, 2014-04-08 21:41:35]

https://codereview.chromium.org/228293004/diff/80001/chrome/browser/infobars/...
File chrome/browser/infobars/infobar_delegate.cc (right):

https://codereview.chromium.org/228293004/diff/80001/chrome/browser/infobars/...
chrome/browser/infobars/infobar_delegate.cc:18: content::WebContents*
WebContentsFromInfoBar(InfoBar* infobar) {
On 2014/04/08 20:54:46, droger wrote:
> On 2014/04/08 20:39:56, Peter Kasting wrote:
> > On 2014/04/08 15:00:11, droger wrote:
> > > The plan would be to make this helper function public so that any infobar
> > > delegate that needs it can use it instead of
> InfoBarDelegate::web_contents().
> > 
> > I didn't go this route for two reasons:
> > (1) Callsite verbosity -- web_contents() is a lot simpler than
> > WebContentsFromInfoBar(this).
> > (2) Type-checking -- ensuring web_contents() is available due to an
> inheritance
> > hierarchy means that the only delegates that can call it are those that
> inherit
> > from the appropriate base class.  If this is public static, then any infobar
> > could theoretically call it, which we don't want.
> 
> About (2), I don't think we still need to have different base classes for the
> infobar delegates anymore (i.e. ContentConfirmInfoBar). The only point of
these
> base class was to add the web_contents() method, but this is no longer needed
> with this design.

Right, I was suggesting that the base class route was preferable to the
no-base-class route.

I might be wrong.  This way might be better.  What really worries me is the
assumption here that infobar->owner() can be safely static_casted to
InfoBarService*.  Nothing in our current design really enforces this.  I was
hoping by use of inheritance to try and make it more mandatory that this holds. 
Maybe that doesn't actually improve, though.

In any case, it seems like you can't declare this in infobar_delegate.h if you
want that file to live in a component.  So where were you planning to move the
declaration?

[blundell, 2014-04-08 21:47:27]

On 2014/04/08 21:41:35, Peter Kasting wrote:
>
https://codereview.chromium.org/228293004/diff/80001/chrome/browser/infobars/...
> File chrome/browser/infobars/infobar_delegate.cc (right):
> 
>
https://codereview.chromium.org/228293004/diff/80001/chrome/browser/infobars/...
> chrome/browser/infobars/infobar_delegate.cc:18: content::WebContents*
> WebContentsFromInfoBar(InfoBar* infobar) {
> On 2014/04/08 20:54:46, droger wrote:
> > On 2014/04/08 20:39:56, Peter Kasting wrote:
> > > On 2014/04/08 15:00:11, droger wrote:
> > > > The plan would be to make this helper function public so that any
infobar
> > > > delegate that needs it can use it instead of
> > InfoBarDelegate::web_contents().
> > > 
> > > I didn't go this route for two reasons:
> > > (1) Callsite verbosity -- web_contents() is a lot simpler than
> > > WebContentsFromInfoBar(this).
> > > (2) Type-checking -- ensuring web_contents() is available due to an
> > inheritance
> > > hierarchy means that the only delegates that can call it are those that
> > inherit
> > > from the appropriate base class.  If this is public static, then any
infobar
> > > could theoretically call it, which we don't want.
> > 
> > About (2), I don't think we still need to have different base classes for
the
> > infobar delegates anymore (i.e. ContentConfirmInfoBar). The only point of
> these
> > base class was to add the web_contents() method, but this is no longer
needed
> > with this design.
> 
> Right, I was suggesting that the base class route was preferable to the
> no-base-class route.
> 
> I might be wrong.  This way might be better.  What really worries me is the
> assumption here that infobar->owner() can be safely static_casted to
> InfoBarService*.  Nothing in our current design really enforces this.  I was
> hoping by use of inheritance to try and make it more mandatory that this
holds. 
> Maybe that doesn't actually improve, though.
> 

Unless I'm missing something, the InfoBarDelegate inheritance doesn't help the
"can't enforce the assumption" issue: the ContentConfirmInfoBarDelegate would
still be taking in an InfoBar and passing that InfoBar to the
WebContentsAccessor without any way to check that that InfoBar is actually owned
by an InfoBarService rather than some other kind of InfoBarManager.

The only thing I see the inheritance changing is the brevity of the name of the
method used to access the WebContents.

> In any case, it seems like you can't declare this in infobar_delegate.h if you
> want that file to live in a component.  So where were you planning to move the
> declaration?

If we go this route, this would presumably live in a util file in
//chrome/browser/infobars.

[Peter Kasting, 2014-04-08 21:53:57]

On 2014/04/08 21:47:27, blundell wrote:
> Unless I'm missing something, the InfoBarDelegate inheritance doesn't help the
> "can't enforce the assumption" issue: the ContentConfirmInfoBarDelegate would
> still be taking in an InfoBar and passing that InfoBar to the
> WebContentsAccessor without any way to check that that InfoBar is actually
owned
> by an InfoBarService rather than some other kind of InfoBarManager.

(I think you meant InfoBarDelegate rather than InfoBar here.)

I was hoping that, if you inherited from ContentConfirmInfoBarDelegate, that
would be an obvious sign that you should be using InfoBarService.  But I guess
one could make the same argument about this solution: if you're calling
WebContentsFromInfoBarDelegate(), you'd better be using InfoBarService.

Secretly I was hoping there would be some way to actually make InfoBarManager
invisible/inaccessible when InfoBarService was available, to force people to go
through the latter, but I haven't been able to think of a way to make that
compile.

I do think the verbosity reduction is noteworthy, but perhaps not noteworthy
enough on its own.  If you guys think this way is better, I'm OK with it.

> If we go this route, this would presumably live in a util file in
> //chrome/browser/infobars.

I wonder if we should declare it in infobar_service.h with a note like "This
method is only valid to use on InfoBarDelegates owned by an InfoBarService."  If
your code has to #include infobar_service.h to use this method, you're unlikely
to use InfobarManager wrongly.

[blundell, 2014-04-08 21:56:56]

On 2014/04/08 21:53:57, Peter Kasting wrote:
> On 2014/04/08 21:47:27, blundell wrote:
> > Unless I'm missing something, the InfoBarDelegate inheritance doesn't help
the
> > "can't enforce the assumption" issue: the ContentConfirmInfoBarDelegate
would
> > still be taking in an InfoBar and passing that InfoBar to the
> > WebContentsAccessor without any way to check that that InfoBar is actually
> owned
> > by an InfoBarService rather than some other kind of InfoBarManager.
> 
> (I think you meant InfoBarDelegate rather than InfoBar here.)
> 

I actually meant InfoBar: it's InfoBar::owner() that is statically cast to
InfoBarService.

> I was hoping that, if you inherited from ContentConfirmInfoBarDelegate, that
> would be an obvious sign that you should be using InfoBarService.  But I guess
> one could make the same argument about this solution: if you're calling
> WebContentsFromInfoBarDelegate(), you'd better be using InfoBarService.
> 
> Secretly I was hoping there would be some way to actually make InfoBarManager
> invisible/inaccessible when InfoBarService was available, to force people to
go
> through the latter, but I haven't been able to think of a way to make that
> compile.
> 
> I do think the verbosity reduction is noteworthy, but perhaps not noteworthy
> enough on its own.  If you guys think this way is better, I'm OK with it.
> 
> > If we go this route, this would presumably live in a util file in
> > //chrome/browser/infobars.
> 
> I wonder if we should declare it in infobar_service.h with a note like "This
> method is only valid to use on InfoBarDelegates owned by an InfoBarService." 
If
> your code has to #include infobar_service.h to use this method, you're
unlikely
> to use InfobarManager wrongly.

That SGTM, and actually seems like the best approach to mitigating the chance of
using this method wrongly.

[droger, 2014-04-09 09:29:55]

This is ready for review, please take a look.

[blundell, 2014-04-09 09:41:21]

Can you get rid of InfoBarManager::web_contents() in this CL?

Can you add a TODO re: getting rid of InfoBarDelegate::web_contents()? My
understanding is that you're saving that for a follow-up CL to limit the size of
this CL.

https://codereview.chromium.org/228293004/diff/120001/chrome/browser/infobars...
File chrome/browser/infobars/infobar_delegate.cc (right):

https://codereview.chromium.org/228293004/diff/120001/chrome/browser/infobars...
chrome/browser/infobars/infobar_delegate.cc:18: content::WebContents*
WebContentsFromInfoBar(InfoBar* infobar) {
This should live in InfoBarService per the discussion on this CL, right?

https://codereview.chromium.org/228293004/diff/120001/chrome/browser/infobars...
chrome/browser/infobars/infobar_delegate.cc:22: InfoBarService* infobar_service
=
Maybe a warning about only calling this function if you know that |infobar| is
owned by an InfoBarService?

https://codereview.chromium.org/228293004/diff/120001/chrome/browser/infobars...
File chrome/browser/infobars/infobar_manager.h (right):

https://codereview.chromium.org/228293004/diff/120001/chrome/browser/infobars...
chrome/browser/infobars/infobar_manager.h:99: // Notifies the observer in
|observer_list_|.
Reference the bug here as well.

[droger, 2014-04-09 11:10:49]

On 2014/04/09 09:41:21, blundell wrote:
> Can you get rid of InfoBarManager::web_contents() in this CL?

Done. There was actually only one caller
(chrome/browser/ui/cocoa/infobars/extension_infobar_controller.mm).

[blundell, 2014-04-09 15:18:45]

LGTM

[Peter Kasting, 2014-04-10 00:08:58]

LGTM

https://codereview.chromium.org/228293004/diff/160001/chrome/browser/autofill...
File chrome/browser/autofill/autofill_interactive_uitest.cc (right):

https://codereview.chromium.org/228293004/diff/160001/chrome/browser/autofill...
chrome/browser/autofill/autofill_interactive_uitest.cc:177: ->Accept();
Nit: Urgh, clang-format.  Do this:

    infobar_service_->infobar_at(0)->delegate()->AsConfirmInfoBarDelegate()->
        Accept();

https://codereview.chromium.org/228293004/diff/160001/chrome/browser/extensio...
File chrome/browser/extensions/webstore_startup_installer_browsertest.cc
(right):

https://codereview.chromium.org/228293004/diff/160001/chrome/browser/extensio...
chrome/browser/extensions/webstore_startup_installer_browsertest.cc:204:
EXPECT_EQ(infobar_service->infobar_count(), 0u);
Nit: While here: (expected, actual)

https://codereview.chromium.org/228293004/diff/160001/chrome/browser/infobars...
File chrome/browser/infobars/infobar_manager.h (right):

https://codereview.chromium.org/228293004/diff/160001/chrome/browser/infobars...
chrome/browser/infobars/infobar_manager.h:93: // notifications are removed (see
http://crbug.com/354380).
Nit: I might say something more like "Absorb these methods back into their
callers once virtual overrides are removed".  In the same spirit, I wouldn't add
NotifyManagerShuttingDown().

https://codereview.chromium.org/228293004/diff/160001/chrome/browser/infobars...
File chrome/browser/infobars/infobars_browsertest.cc (right):

https://codereview.chromium.org/228293004/diff/160001/chrome/browser/infobars...
chrome/browser/infobars/infobars_browsertest.cc:97: ->infobar_count());
Nit: Place arrow on end of previous line

https://codereview.chromium.org/228293004/diff/160001/chrome/browser/password...
File chrome/browser/password_manager/password_manager_browsertest.cc (right):

https://codereview.chromium.org/228293004/diff/160001/chrome/browser/password...
chrome/browser/password_manager/password_manager_browsertest.cc:81: ->Accept();
Nit: While here: Fix wrapping:

          infobar_service_->infobar_at(0)->delegate()->
              AsConfirmInfoBarDelegate()->Accept();

https://codereview.chromium.org/228293004/diff/160001/chrome/browser/translat...
File chrome/browser/translate/translate_browsertest.cc (right):

https://codereview.chromium.org/228293004/diff/160001/chrome/browser/translat...
chrome/browser/translate/translate_browsertest.cc:102:
->AsConfirmInfoBarDelegate();
Nit: I'd condense/wrap like this:

      if (infobar_service_->infobar_at(i)->delegate()->
          AsConfirmInfoBarDelegate())
        continue;

      TranslateInfoBarDelegate* translate =
          infobar_service_->infobar_at(i)->delegate()->
              AsTranslateInfoBarDelegate();
      ...

https://codereview.chromium.org/228293004/diff/160001/chrome/browser/translat...
File chrome/browser/translate/translate_manager_render_view_host_unittest.cc
(right):

https://codereview.chromium.org/228293004/diff/160001/chrome/browser/translat...
chrome/browser/translate/translate_manager_render_view_host_unittest.cc:158: :
NULL;
Nit: While here: More to clean up:

    return (infobar_service()->infobar_count() == 1) ?
        infobar_service()->infobar_at(0)->delegate()->
            AsTranslateInfoBarDelegate() :
        NULL;

https://codereview.chromium.org/228293004/diff/160001/chrome/test/gpu/webgl_i...
File chrome/test/gpu/webgl_infobar_browsertest.cc (right):

https://codereview.chromium.org/228293004/diff/160001/chrome/test/gpu/webgl_i...
chrome/test/gpu/webgl_infobar_browsertest.cc:94: ->infobar_count());
Nit: Place arrow on end of previous line

[droger, 2014-04-10 09:18:18]

TBR=jochen for everything that is not in ui/ infobars/ or translate/

[droger, 2014-04-10 09:18:27]

The CQ bit was checked by droger@chromium.org

[commit-bot: I haz the power, 2014-04-10 09:18:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/droger@chromium.org/228293004/180001

[commit-bot: I haz the power, 2014-04-10 13:53:28]

Change committed as 262976