Remove dependency of infobars component on the embedder

Remove dependency of infobars component on the embedder

Add a virtual method InfoBarManager::CreateConfirmInfoBar that allow the
embedder to use UI specific implementation of ConfirmInfoBar. Port all
client code to use this virtual method instead of the static method
ConfirmInfoBarDelegate::CreateInfoBar.

Implements the InfoBarService::CreateConfirmInfoBar method for the
different UI (views, android, cocoa) and for TestInfoBarManager.

BUG=386171
TBR=jam@chromium.org
TBR=jochen@chromium.org

Committed: https://crrev.com/316da458d4d1ea846f078efaf71ae1f23964f42e
Cr-Commit-Position: refs/heads/master@{#309437}

[sdefresne, 2014-12-17 19:10:44]

sdefresne@chromium.org changed reviewers:
+ pkasting@chromium.org

[sdefresne, 2014-12-17 19:10:45]

pkasting: could you please review this CL. It introduce an abstract singleton
used to implement ConfirmInfoBarDelegate::CreateInfoBar() as you suggested,
provides a chrome implementation and a test helper and register both of them
when required.

This is based on this comment: "I suppose one way would be, instead of having
this be a static method on ConfirmInfoBarDelegate, have it be pure virtual on
some singleton abstract class we expect the embedder to implement.  To preserve
the existing "only confirm infobar delegate subclasses can do this" restriction,
we could make this private, make ConfirmInfoBarDelegate a friend, and keep the
existing static method on that class, but implement it in the components/ code
to call over to the new abstract method.  Doing this would require that such an
embedder class actually exist.  Not sure if one does." on
https://codereview.chromium.org/710453002

[Michael van Ouwerkerk, 2014-12-17 19:23:43]

mvanouwerkerk@chromium.org changed reviewers:
+ mvanouwerkerk@chromium.org

[Michael van Ouwerkerk, 2014-12-17 19:23:44]

https://codereview.chromium.org/812823002/diff/20001/chrome/browser/geolocati...
File chrome/browser/geolocation/geolocation_permission_context_unittest.cc
(right):

https://codereview.chromium.org/812823002/diff/20001/chrome/browser/geolocati...
chrome/browser/geolocation/geolocation_permission_context_unittest.cc:140:
TestConfirmInfoBarDelegateFactory confirm_info_bar_delegate_factory_;
Why is this needed? It appears to be unused.

[Peter Kasting, 2014-12-17 19:32:50]

Can we implement this on some class that already exists?  I'm not terribly
thrilled about the SetInstance() stuff.

I was thinking perhaps the InfoBarManager could declare this, and then we could
implement it in the InfoBarService.  If we do that, I think the components code
probably already has access to InfoBarManager pointers in the places it needs to
create these infobars?

[sdefresne, 2014-12-18 14:34:00]

On 2014/12/17 at 19:32:50, pkasting wrote:
> Can we implement this on some class that already exists?  I'm not terribly
thrilled about the SetInstance() stuff.
> 
> I was thinking perhaps the InfoBarManager could declare this, and then we
could implement it in the InfoBarService.  If we do that, I think the components
code probably already has access to InfoBarManager pointers in the places it
needs to create these infobars?

I've re-implemented it on InfoBarManager as suggested. Can you take another
look?

Note that I've changed the ConfirmInfoBarDelegate::CreateInfoBar() prototype to
take the infobars::InfoBarManager as an additional parameter and changed all the
client code to pass it (it was always available since it is required to add the
newly created InfoBar). I intent to TBR the review of the client code if you
think this implementation is okay.

[sdefresne, 2014-12-18 14:48:58]

sdefresne@chromium.org changed reviewers:
- mvanouwerkerk@chromium.org

[sdefresne, 2014-12-18 14:48:59]

mvanouwerkerk: FYI

https://codereview.chromium.org/812823002/diff/20001/chrome/browser/geolocati...
File chrome/browser/geolocation/geolocation_permission_context_unittest.cc
(right):

https://codereview.chromium.org/812823002/diff/20001/chrome/browser/geolocati...
chrome/browser/geolocation/geolocation_permission_context_unittest.cc:140:
TestConfirmInfoBarDelegateFactory confirm_info_bar_delegate_factory_;
On 2014/12/17 at 19:23:43, Michael - OOO until 2 January wrote:
> Why is this needed? It appears to be unused.

The TestConfirmInfoBarDelegateFactory used to do work in constructor/destructor
and just creating an instance was enough. I've changed the implementation and
this is no longer required though.

[Peter Kasting, 2014-12-18 19:23:02]

OK, this is definitely better.

I didn't think through how ConfirmInfoBarDelegate::CreateInfoBar() would need to
have the manager pointer passed in in order to call it.  It's obviously
necessary, I just wasn't thinking through it enough.

Since we have to do that anyway, I think the gain from trying to have all
callers route their requests through this class is pretty much zero.  Let's do
the simpler route: make the new InfoBarManager method public, eliminate
ConfirmInfoBarDelegate::CreateInfoBar() entirely, and have all callers just call
the InfoBarManager method directly.

I'm sorry to keep giving you directions that require lots of reworking of the
patch.  This may be another case where instead of waving my hands and then
having someone else do it, I should have tried to write up the patch myself, so
I would have run into these sorts of issues and been prompted to think more
deeply about the problem :/

https://codereview.chromium.org/812823002/diff/100001/components/infobars/cor...
File components/infobars/core/infobar_manager.h (right):

https://codereview.chromium.org/812823002/diff/100001/components/infobars/cor...
components/infobars/core/infobar_manager.h:121: // of ConfirmInfoBar.
This comment is super-confused grammatically and content-wise
(CreateConfirmInfoBar must only be called by CreateConfirmInfoBar?).

[sdefresne, 2014-12-19 11:06:15]

Please take another look.

https://codereview.chromium.org/812823002/diff/100001/components/infobars/cor...
File components/infobars/core/infobar_manager.h (right):

https://codereview.chromium.org/812823002/diff/100001/components/infobars/cor...
components/infobars/core/infobar_manager.h:121: // of ConfirmInfoBar.
On 2014/12/18 at 19:23:02, Peter Kasting wrote:
> This comment is super-confused grammatically and content-wise
(CreateConfirmInfoBar must only be called by CreateConfirmInfoBar?).

Ack. English is hard!

[Peter Kasting, 2014-12-19 23:06:23]

LGTM modulo style

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

https://codereview.chromium.org/812823002/diff/140001/chrome/browser/infobars...
chrome/browser/infobars/infobar_service.h:48: // InfoBarManager:
Why make these public instead of private?

Normally I try to default to "as private as possible" for overriding
implementations.

Same comment on the protected methods below.

https://codereview.chromium.org/812823002/diff/140001/chrome/browser/net/spdy...
File chrome/browser/net/spdyproxy/data_reduction_proxy_infobar_delegate.h
(right):

https://codereview.chromium.org/812823002/diff/140001/chrome/browser/net/spdy...
chrome/browser/net/spdyproxy/data_reduction_proxy_infobar_delegate.h:19: }
This forward-decl doesn't seem to be needed?

https://codereview.chromium.org/812823002/diff/140001/chrome/browser/plugins/...
File chrome/browser/plugins/plugin_infobar_delegates.cc (right):

https://codereview.chromium.org/812823002/diff/140001/chrome/browser/plugins/...
chrome/browser/plugins/plugin_infobar_delegates.cc:100: :
IDS_PLUGIN_DOWNLOADING,
Nit: Don't rewrap ?: linebreaking (2 places)

https://codereview.chromium.org/812823002/diff/140001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/keystone_infobar_delegate.mm (right):

https://codereview.chromium.org/812823002/diff/140001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/keystone_infobar_delegate.mm:87: ->GetPrefs()))));
Nit: Old linebreaking here was better

[sdefresne, 2014-12-20 11:30:53]

sdefresne@chromium.org changed reviewers:
+ jam@chromium.org, jochen@chromium.org

[sdefresne, 2014-12-20 11:30:55]

Will TBR OWNERS since this is just mechanical change in the client code.

jam: OWNERS of athena/main/athena_main.gyp
jochen: OWNERS of all the remaining client code

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

https://codereview.chromium.org/812823002/diff/140001/chrome/browser/infobars...
chrome/browser/infobars/infobar_service.h:48: // InfoBarManager:
On 2014/12/19 23:06:23, Peter Kasting wrote:
> Why make these public instead of private?
> 
> Normally I try to default to "as private as possible" for overriding
> implementations.
> 
> Same comment on the protected methods below.

I need to make CreateConfirmInfoBar() public since most of the client code have
InfoBarService* and not infobars::InfoBarManager*. If the method is private in
InfoBarService* then they cannot call it.

I considered leaving the method private and changing all the client code to use
infobars::InfoBarManager* instead but this made the CL even larger. For the
moment, I'll only leave CreateConfirmInfoBar() public, with a comment to turn it
back private once all client code has been fixed.

https://codereview.chromium.org/812823002/diff/140001/chrome/browser/net/spdy...
File chrome/browser/net/spdyproxy/data_reduction_proxy_infobar_delegate.h
(right):

https://codereview.chromium.org/812823002/diff/140001/chrome/browser/net/spdy...
chrome/browser/net/spdyproxy/data_reduction_proxy_infobar_delegate.h:19: }
On 2014/12/19 at 23:06:23, Peter Kasting wrote:
> This forward-decl doesn't seem to be needed?

It is in fact required to allow Android code to provide a system specific
implementation of DataReductionProxyInfoBarDelegate::CreateInfoBar().

https://codereview.chromium.org/812823002/diff/140001/chrome/browser/plugins/...
File chrome/browser/plugins/plugin_infobar_delegates.cc (right):

https://codereview.chromium.org/812823002/diff/140001/chrome/browser/plugins/...
chrome/browser/plugins/plugin_infobar_delegates.cc:100: :
IDS_PLUGIN_DOWNLOADING,
On 2014/12/19 at 23:06:23, Peter Kasting wrote:
> Nit: Don't rewrap ?: linebreaking (2 places)

Sorry, I reformatted everything with clang-format and I just accepted what it
gave me. Reverted.

https://codereview.chromium.org/812823002/diff/140001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/keystone_infobar_delegate.mm (right):

https://codereview.chromium.org/812823002/diff/140001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/keystone_infobar_delegate.mm:87: ->GetPrefs()))));
On 2014/12/19 at 23:06:23, Peter Kasting wrote:
> Nit: Old linebreaking here was better


Sorry, I reformatted everything with clang-format and I just accepted what it
gave me. Reverted.

[sdefresne, 2014-12-22 17:11:44]

The CQ bit was checked by sdefresne@chromium.org

[commit-bot: I haz the power, 2014-12-22 17:12:10]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/812823002/200001

[commit-bot: I haz the power, 2014-12-22 17:31:38]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2014-12-22 17:32:24]

Patchset 11 (id:??) landed as
https://crrev.com/316da458d4d1ea846f078efaf71ae1f23964f42e
Cr-Commit-Position: refs/heads/master@{#309437}

[Peter Kasting, 2014-12-22 19:22:06]

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

https://codereview.chromium.org/812823002/diff/140001/chrome/browser/infobars...
chrome/browser/infobars/infobar_service.h:48: // InfoBarManager:
On 2014/12/20 11:30:55, sdefresne wrote:
> On 2014/12/19 23:06:23, Peter Kasting wrote:
> > Why make these public instead of private?
> > 
> > Normally I try to default to "as private as possible" for overriding
> > implementations.
> > 
> > Same comment on the protected methods below.
> 
> I need to make CreateConfirmInfoBar() public since most of the client code
have
> InfoBarService* and not infobars::InfoBarManager*. If the method is private in
> InfoBarService* then they cannot call it.
> 
> I considered leaving the method private and changing all the client code to
use
> infobars::InfoBarManager* instead but this made the CL even larger. For the
> moment, I'll only leave CreateConfirmInfoBar() public, with a comment to turn
it
> back private once all client code has been fixed.

It may not be good to make all clients use an InfoBarManager*.  I suspect some
of them on the chrome side need to call InfoBarService-only methods.

If that's not true then maybe the conversion you propose would work.

In any case, that explains making CreateConfirmInfoBar() protected, but not the
other methods.

https://codereview.chromium.org/812823002/diff/140001/chrome/browser/net/spdy...
File chrome/browser/net/spdyproxy/data_reduction_proxy_infobar_delegate.h
(right):

https://codereview.chromium.org/812823002/diff/140001/chrome/browser/net/spdy...
chrome/browser/net/spdyproxy/data_reduction_proxy_infobar_delegate.h:19: }
On 2014/12/20 11:30:55, sdefresne wrote:
> On 2014/12/19 at 23:06:23, Peter Kasting wrote:
> > This forward-decl doesn't seem to be needed?
> 
> It is in fact required to allow Android code to provide a system specific
> implementation of DataReductionProxyInfoBarDelegate::CreateInfoBar().

I don't understand why the forward-decl is needed in this file, though.  Isn't
it sufficient to declare or define this type in the file that declares/defines
this CreateInfoBar() override?

Basically, I'm just saying that this type is never referred to in this specific
.h, so presumably it could be defined in whatever file #includes this header and
needs it.

[sdefresne, 2014-12-22 20:48:41]

https://codereview.chromium.org/812823002/diff/140001/chrome/browser/net/spdy...
File chrome/browser/net/spdyproxy/data_reduction_proxy_infobar_delegate.h
(right):

https://codereview.chromium.org/812823002/diff/140001/chrome/browser/net/spdy...
chrome/browser/net/spdyproxy/data_reduction_proxy_infobar_delegate.h:19: }
On 2014/12/22 at 19:22:05, Peter Kasting wrote:
> On 2014/12/20 11:30:55, sdefresne wrote:
> > On 2014/12/19 at 23:06:23, Peter Kasting wrote:
> > > This forward-decl doesn't seem to be needed?
> > 
> > It is in fact required to allow Android code to provide a system specific
> > implementation of DataReductionProxyInfoBarDelegate::CreateInfoBar().
> 
> I don't understand why the forward-decl is needed in this file, though.  Isn't
it sufficient to declare or define this type in the file that declares/defines
this CreateInfoBar() override?
> 
> Basically, I'm just saying that this type is never referred to in this
specific .h, so presumably it could be defined in whatever file #includes this
header and needs it.

Sorry, I was again unclear. I planned to remove this, but then I found that
compilation failed on Android. This is because the class define a static method
CreateInfoBar() that is overridden in
chrome/browser/ui/android/infobars/data_reduction_proxy_infobar.cc. In a later
patch for this CL, I did have to re-introduce this forward-declaration as I
changed the declaration of CreateInfoBar() to take both an
infobars::InfoBarManager* and a scoped_ptr<DataReductionProxyInfoBarDelegate>.

See patchset 11 where the declaration of CreateInfoBar is slightly different and
uses infobars::InfoBarManager.

[Peter Kasting, 2014-12-22 21:24:11]

On 2014/12/22 20:48:41, sdefresne wrote:
>
https://codereview.chromium.org/812823002/diff/140001/chrome/browser/net/spdy...
> File chrome/browser/net/spdyproxy/data_reduction_proxy_infobar_delegate.h
> (right):
> 
>
https://codereview.chromium.org/812823002/diff/140001/chrome/browser/net/spdy...
> chrome/browser/net/spdyproxy/data_reduction_proxy_infobar_delegate.h:19: }
> On 2014/12/22 at 19:22:05, Peter Kasting wrote:
> > On 2014/12/20 11:30:55, sdefresne wrote:
> > > On 2014/12/19 at 23:06:23, Peter Kasting wrote:
> > > > This forward-decl doesn't seem to be needed?
> > > 
> > > It is in fact required to allow Android code to provide a system specific
> > > implementation of DataReductionProxyInfoBarDelegate::CreateInfoBar().
> > 
> > I don't understand why the forward-decl is needed in this file, though. 
Isn't
> it sufficient to declare or define this type in the file that declares/defines
> this CreateInfoBar() override?
> > 
> > Basically, I'm just saying that this type is never referred to in this
> specific .h, so presumably it could be defined in whatever file #includes this
> header and needs it.
> 
> Sorry, I was again unclear. I planned to remove this, but then I found that
> compilation failed on Android. This is because the class define a static
method
> CreateInfoBar() that is overridden in
> chrome/browser/ui/android/infobars/data_reduction_proxy_infobar.cc. In a later
> patch for this CL, I did have to re-introduce this forward-declaration as I
> changed the declaration of CreateInfoBar() to take both an
> infobars::InfoBarManager* and a scoped_ptr<DataReductionProxyInfoBarDelegate>.
> 
> See patchset 11 where the declaration of CreateInfoBar is slightly different
and
> uses infobars::InfoBarManager.

Ah, that looks more reasonable.