Introduce InfoBarTabService API.

Introduce InfoBarTabService API.

The new interface extracts the API bits from InfoBarTabHelper.  This
change switches files under chrome/browser/autofill and
chrome/browser/api to use the API and remove the temporarily-allowed
dependency on InfoBarTabHelper.

TBRing other owners as the changes are just to match the updated API and include paths.
TBR=ben@chromium.org
BUG=140037
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=152338

[Jói, 2012-08-16 10:21:37]

erikwright:  Main reviewer.

pkasting:  Optional reviewer, this is all mechanical except if you want to look
at which bits of InfoBarTabHelper get extracted into the InfoBarTabService API.

Cheers,
Jói

[tfarina, 2012-08-16 12:14:35]

http://codereview.chromium.org/10830353/diff/1090/chrome/browser/api/infobars...
File chrome/browser/api/infobars/infobar_tab_service.h (right):

http://codereview.chromium.org/10830353/diff/1090/chrome/browser/api/infobars...
chrome/browser/api/infobars/infobar_tab_service.h:20: static InfoBarTabService*
GetInstance(TabContents* tab_contents);
According to crbug.com/68682 static methods should be after virtual destructors.
But I agree that separating static from virtuals in interfaces like this, looks
better ;)

[Jói, 2012-08-16 12:23:32]

http://codereview.chromium.org/10830353/diff/1090/chrome/browser/api/infobars...
File chrome/browser/api/infobars/infobar_tab_service.h (right):

http://codereview.chromium.org/10830353/diff/1090/chrome/browser/api/infobars...
chrome/browser/api/infobars/infobar_tab_service.h:20: static InfoBarTabService*
GetInstance(TabContents* tab_contents);
On 2012/08/16 12:14:35, tfarina wrote:
> According to crbug.com/68682 static methods should be after virtual
destructors.
> But I agree that separating static from virtuals in interfaces like this,
looks
> better ;)

Thanks, putting them at the top like this, at least if they are factory or
retrieval methods, has been the convention in content/.

[erikwright (departed), 2012-08-16 17:25:38]

I see lots of cases using the Helper when they should be able to switch to the
Service. Since they are already being touched, can we just make that switch now?

http://codereview.chromium.org/10830353/diff/1090/chrome/browser/api/infobars...
File chrome/browser/api/infobars/infobar_delegate.cc (right):

http://codereview.chromium.org/10830353/diff/1090/chrome/browser/api/infobars...
chrome/browser/api/infobars/infobar_delegate.cc:93:
InfoBarDelegate::InfoBarDelegate(InfoBarTabService* infobar_helper)
rename to infobar_service?

http://codereview.chromium.org/10830353/diff/1090/chrome/browser/api/infobars...
File chrome/browser/api/infobars/infobar_delegate.h (right):

http://codereview.chromium.org/10830353/diff/1090/chrome/browser/api/infobars...
chrome/browser/api/infobars/infobar_delegate.h:70: // the InfoBarTabHelper will
not add the new delegate because it considers
Helper->Service

http://codereview.chromium.org/10830353/diff/1090/chrome/browser/api/infobars...
chrome/browser/api/infobars/infobar_delegate.h:111: explicit
InfoBarDelegate(InfoBarTabService* infobar_helper);
rename

http://codereview.chromium.org/10830353/diff/1090/chrome/browser/api/infobars...
chrome/browser/api/infobars/infobar_delegate.h:116: void
StoreActiveEntryUniqueID(InfoBarTabService* infobar_helper);
rename

http://codereview.chromium.org/10830353/diff/1090/chrome/browser/api/infobars...
chrome/browser/api/infobars/infobar_delegate.h:136: void clear_owner() { owner_
= NULL; }
This implies that one could not simply write an alternative implementation of
InfoBarTabService using this API.

I think it would be better to leave clear_owner public.

http://codereview.chromium.org/10830353/diff/1090/chrome/browser/api/infobars...
File chrome/browser/api/infobars/infobar_tab_service.h (right):

http://codereview.chromium.org/10830353/diff/1090/chrome/browser/api/infobars...
chrome/browser/api/infobars/infobar_tab_service.h:20: static InfoBarTabService*
GetInstance(TabContents* tab_contents);
My comments elsewhere apply here. I don't think that a conforming component
should use this API. Perhaps move it out of /api/.

http://codereview.chromium.org/10830353/diff/1090/chrome/browser/api/infobars...
File chrome/browser/api/infobars/simple_alert_infobar_delegate.cc (right):

http://codereview.chromium.org/10830353/diff/1090/chrome/browser/api/infobars...
chrome/browser/api/infobars/simple_alert_infobar_delegate.cc:10:
InfoBarTabService* infobar_helper,
rename

http://codereview.chromium.org/10830353/diff/1090/chrome/browser/api/infobars...
File chrome/browser/api/infobars/simple_alert_infobar_delegate.h (right):

http://codereview.chromium.org/10830353/diff/1090/chrome/browser/api/infobars...
chrome/browser/api/infobars/simple_alert_infobar_delegate.h:15:
SimpleAlertInfoBarDelegate(InfoBarTabService* infobar_helper,
rename

http://codereview.chromium.org/10830353/diff/1090/chrome/browser/custom_handl...
File chrome/browser/custom_handlers/register_protocol_handler_infobar_delegate.h
(right):

http://codereview.chromium.org/10830353/diff/1090/chrome/browser/custom_handl...
chrome/browser/custom_handlers/register_protocol_handler_infobar_delegate.h:12:
class InfoBarTabHelper;
Use the service instead?

http://codereview.chromium.org/10830353/diff/1090/chrome/browser/download/dow...
File chrome/browser/download/download_request_infobar_delegate.cc (right):

http://codereview.chromium.org/10830353/diff/1090/chrome/browser/download/dow...
chrome/browser/download/download_request_infobar_delegate.cc:16: :
ConfirmInfoBarDelegate(infobar_helper),
The base class now takes a service, so the helper should not be needed, correct?

http://codereview.chromium.org/10830353/diff/1090/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_infobar_delegate.h (right):

http://codereview.chromium.org/10830353/diff/1090/chrome/browser/extensions/e...
chrome/browser/extensions/extension_infobar_delegate.h:15: class
InfoBarTabHelper;
can we just use service here?

[Jói, 2012-08-17 15:51:23]

PTAL

http://codereview.chromium.org/10830353/diff/1090/chrome/browser/api/infobars...
File chrome/browser/api/infobars/infobar_delegate.cc (right):

http://codereview.chromium.org/10830353/diff/1090/chrome/browser/api/infobars...
chrome/browser/api/infobars/infobar_delegate.cc:93:
InfoBarDelegate::InfoBarDelegate(InfoBarTabService* infobar_helper)
On 2012/08/16 17:25:38, erikwright wrote:
> rename to infobar_service?

Done.

http://codereview.chromium.org/10830353/diff/1090/chrome/browser/api/infobars...
File chrome/browser/api/infobars/infobar_delegate.h (right):

http://codereview.chromium.org/10830353/diff/1090/chrome/browser/api/infobars...
chrome/browser/api/infobars/infobar_delegate.h:70: // the InfoBarTabHelper will
not add the new delegate because it considers
On 2012/08/16 17:25:38, erikwright wrote:
> Helper->Service

Done.

http://codereview.chromium.org/10830353/diff/1090/chrome/browser/api/infobars...
chrome/browser/api/infobars/infobar_delegate.h:111: explicit
InfoBarDelegate(InfoBarTabService* infobar_helper);
On 2012/08/16 17:25:38, erikwright wrote:
> rename

Done.

http://codereview.chromium.org/10830353/diff/1090/chrome/browser/api/infobars...
chrome/browser/api/infobars/infobar_delegate.h:116: void
StoreActiveEntryUniqueID(InfoBarTabService* infobar_helper);
On 2012/08/16 17:25:38, erikwright wrote:
> rename

Done.

http://codereview.chromium.org/10830353/diff/1090/chrome/browser/api/infobars...
chrome/browser/api/infobars/infobar_delegate.h:136: void clear_owner() { owner_
= NULL; }
On 2012/08/16 17:25:38, erikwright wrote:
> This implies that one could not simply write an alternative implementation of
> InfoBarTabService using this API.
> 
> I think it would be better to leave clear_owner public.

Moved it back to public.  Removed the comment.

http://codereview.chromium.org/10830353/diff/1090/chrome/browser/api/infobars...
File chrome/browser/api/infobars/infobar_tab_service.h (right):

http://codereview.chromium.org/10830353/diff/1090/chrome/browser/api/infobars...
chrome/browser/api/infobars/infobar_tab_service.h:20: static InfoBarTabService*
GetInstance(TabContents* tab_contents);
On 2012/08/16 17:25:38, erikwright wrote:
> My comments elsewhere apply here. I don't think that a conforming component
> should use this API. Perhaps move it out of /api/.

As per discussion summarized in http://codereview.chromium.org/10828345/ we
decided this is staying and the pattern is allowed in /api/ but we may revisit
this later and apply dependency injection more broadly.

http://codereview.chromium.org/10830353/diff/1090/chrome/browser/api/infobars...
File chrome/browser/api/infobars/simple_alert_infobar_delegate.cc (right):

http://codereview.chromium.org/10830353/diff/1090/chrome/browser/api/infobars...
chrome/browser/api/infobars/simple_alert_infobar_delegate.cc:10:
InfoBarTabService* infobar_helper,
On 2012/08/16 17:25:38, erikwright wrote:
> rename

Done.

http://codereview.chromium.org/10830353/diff/1090/chrome/browser/api/infobars...
File chrome/browser/api/infobars/simple_alert_infobar_delegate.h (right):

http://codereview.chromium.org/10830353/diff/1090/chrome/browser/api/infobars...
chrome/browser/api/infobars/simple_alert_infobar_delegate.h:15:
SimpleAlertInfoBarDelegate(InfoBarTabService* infobar_helper,
On 2012/08/16 17:25:38, erikwright wrote:
> rename

Done.

http://codereview.chromium.org/10830353/diff/1090/chrome/browser/custom_handl...
File chrome/browser/custom_handlers/register_protocol_handler_infobar_delegate.h
(right):

http://codereview.chromium.org/10830353/diff/1090/chrome/browser/custom_handl...
chrome/browser/custom_handlers/register_protocol_handler_infobar_delegate.h:12:
class InfoBarTabHelper;
On 2012/08/16 17:25:38, erikwright wrote:
> Use the service instead?

Done.

http://codereview.chromium.org/10830353/diff/1090/chrome/browser/download/dow...
File chrome/browser/download/download_request_infobar_delegate.cc (right):

http://codereview.chromium.org/10830353/diff/1090/chrome/browser/download/dow...
chrome/browser/download/download_request_infobar_delegate.cc:16: :
ConfirmInfoBarDelegate(infobar_helper),
On 2012/08/16 17:25:38, erikwright wrote:
> The base class now takes a service, so the helper should not be needed,
correct?

Done.

http://codereview.chromium.org/10830353/diff/1090/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_infobar_delegate.h (right):

http://codereview.chromium.org/10830353/diff/1090/chrome/browser/extensions/e...
chrome/browser/extensions/extension_infobar_delegate.h:15: class
InfoBarTabHelper;
On 2012/08/16 17:25:38, erikwright wrote:
> can we just use service here?

Done.

[erikwright (departed), 2012-08-20 05:54:45]

LGTM assuming that I'm right about the 'friend' declaration being unnecessary.
Otherwise some kind of comment is required.

I would prefer to see all of the static_casts removed, but I guess that might
cause this CL to blow up? If it was not too much work I'd do it, but otherwise a
tracking bug should be created for the Infobar API and this should be recorded
there.

http://codereview.chromium.org/10830353/diff/11002/chrome/browser/api/infobar...
File chrome/browser/api/infobars/infobar_delegate.h (right):

http://codereview.chromium.org/10830353/diff/11002/chrome/browser/api/infobar...
chrome/browser/api/infobars/infobar_delegate.h:135: friend class
InfoBarTabHelper;
I assume this is accidentally left over?

http://codereview.chromium.org/10830353/diff/11002/chrome/browser/plugin_info...
File chrome/browser/plugin_infobar_delegates.cc (right):

http://codereview.chromium.org/10830353/diff/11002/chrome/browser/plugin_info...
chrome/browser/plugin_infobar_delegates.cc:296:
static_cast<InfoBarTabHelper*>(owner()),
Perhaps it would make sense to convert PluginInstallerInfoBarDelegate to use the
service API to get rid of these casts?

http://codereview.chromium.org/10830353/diff/11002/chrome/browser/ui/cocoa/in...
File chrome/browser/ui/cocoa/infobars/media_stream_infobar_controller.mm
(right):

http://codereview.chromium.org/10830353/diff/11002/chrome/browser/ui/cocoa/in...
chrome/browser/ui/cocoa/infobars/media_stream_infobar_controller.mm:29:
MediaStreamInfoBarController* infobar_controller =
Again, perhaps converting MediaStreamInfoBarController and removing the cast
would be worthwhile.

http://codereview.chromium.org/10830353/diff/11002/chrome/browser/ui/cocoa/in...
File chrome/browser/ui/cocoa/infobars/translate_infobar_base.mm (right):

http://codereview.chromium.org/10830353/diff/11002/chrome/browser/ui/cocoa/in...
chrome/browser/ui/cocoa/infobars/translate_infobar_base.mm:33: InfoBarTabHelper*
helper = (InfoBarTabHelper*)owner;
remove cast?

http://codereview.chromium.org/10830353/diff/11002/chrome/browser/ui/gtk/info...
File chrome/browser/ui/gtk/infobars/extension_infobar_gtk.cc (right):

http://codereview.chromium.org/10830353/diff/11002/chrome/browser/ui/gtk/info...
chrome/browser/ui/gtk/infobars/extension_infobar_gtk.cc:207: return new
ExtensionInfoBarGtk(static_cast<InfoBarTabHelper*>(owner), this);
cast

[Jói, 2012-08-20 12:30:15]

Thanks Erik.

The InfoBarDelegate subclasses touched by this change now deal only in
InfoBarTabService.  OTOH, InfoBar subclasses still need to deal in the concrete
InfoBarTabHelper implementation, so there are a couple of casts remaining.

It'll take a while to run this through the trybots, in particular the Mac
changes are probably not correct yet, but I'll commit when I'm in the end state
described above.

Cheers,
Jói

http://codereview.chromium.org/10830353/diff/11002/chrome/browser/api/infobar...
File chrome/browser/api/infobars/infobar_delegate.h (right):

http://codereview.chromium.org/10830353/diff/11002/chrome/browser/api/infobar...
chrome/browser/api/infobars/infobar_delegate.h:135: friend class
InfoBarTabHelper;
On 2012/08/20 05:54:46, erikwright wrote:
> I assume this is accidentally left over?

Done.

http://codereview.chromium.org/10830353/diff/11002/chrome/browser/plugin_info...
File chrome/browser/plugin_infobar_delegates.cc (right):

http://codereview.chromium.org/10830353/diff/11002/chrome/browser/plugin_info...
chrome/browser/plugin_infobar_delegates.cc:296:
static_cast<InfoBarTabHelper*>(owner()),
On 2012/08/20 05:54:46, erikwright wrote:
> Perhaps it would make sense to convert PluginInstallerInfoBarDelegate to use
the
> service API to get rid of these casts?

Done.

http://codereview.chromium.org/10830353/diff/11002/chrome/browser/ui/cocoa/in...
File chrome/browser/ui/cocoa/infobars/media_stream_infobar_controller.mm
(right):

http://codereview.chromium.org/10830353/diff/11002/chrome/browser/ui/cocoa/in...
chrome/browser/ui/cocoa/infobars/media_stream_infobar_controller.mm:29:
MediaStreamInfoBarController* infobar_controller =
On 2012/08/20 05:54:46, erikwright wrote:
> Again, perhaps converting MediaStreamInfoBarController and removing the cast
> would be worthwhile.

Done.

http://codereview.chromium.org/10830353/diff/11002/chrome/browser/ui/cocoa/in...
File chrome/browser/ui/cocoa/infobars/translate_infobar_base.mm (right):

http://codereview.chromium.org/10830353/diff/11002/chrome/browser/ui/cocoa/in...
chrome/browser/ui/cocoa/infobars/translate_infobar_base.mm:33: InfoBarTabHelper*
helper = (InfoBarTabHelper*)owner;
On 2012/08/20 05:54:46, erikwright wrote:
> remove cast?

Done.

http://codereview.chromium.org/10830353/diff/11002/chrome/browser/ui/gtk/info...
File chrome/browser/ui/gtk/infobars/extension_infobar_gtk.cc (right):

http://codereview.chromium.org/10830353/diff/11002/chrome/browser/ui/gtk/info...
chrome/browser/ui/gtk/infobars/extension_infobar_gtk.cc:207: return new
ExtensionInfoBarGtk(static_cast<InfoBarTabHelper*>(owner), this);
On 2012/08/20 05:54:46, erikwright wrote:
> cast

Done.

[commit-bot: I haz the power, 2012-08-20 13:53:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joi@chromium.org/10830353/11002

[commit-bot: I haz the power, 2012-08-20 14:06:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joi@chromium.org/10830353/4008

[commit-bot: I haz the power, 2012-08-20 16:34:21]

Change committed as 152338

[Peter Kasting, 2012-08-25 23:09:34]

Is InfoBarTabHelper the only class that will ever implement InfoBarTabService? 
If so, is it possible to move the entire concrete implementation to the api/
directory?

Really I just feel sad if I see more layers added to the class hierarchy when
that could be avoided...

[Jói, 2012-08-27 15:49:44]

Hi Peter,

There was extensive discussion and even a design meeting on the
subject of whether to allow concrete classes in general, when they are
"suitable as APIs" (with InfoBarTabHelper being the concrete example
under scrutiny), see http://codereview.chromium.org/10843071/.

In summary the conclusion of that discussion was that there are
advantages to making the API cleanly abstract, to separate layers more
effectively, allow better unit testing, be able to separate things
more cleanly on linker boundaries, etc., and that we would likely end
up with abstract interfaces for almost everything in the API directory
in the end anyway.  The only exception is to allow simple helper
classes that don't depend on non-API stuff.

Cheers,
Jói


On Sat, Aug 25, 2012 at 11:09 PM,  <pkasting@chromium.org> wrote:
> Is InfoBarTabHelper the only class that will ever implement
> InfoBarTabService?
> If so, is it possible to move the entire concrete implementation to the api/
> directory?
>
> Really I just feel sad if I see more layers added to the class hierarchy
> when
> that could be avoided...
>
> https://chromiumcodereview.appspot.com/10830353/
>
>
> --
> You received this message because you are subscribed to the Google Groups
> "browser-components-watch" group.
> To post to this group, send email to browser-components-watch@chromium.org.
> To unsubscribe from this group, send email to
> browser-components-watch+unsubscribe@chromium.org.
> For more options, visit
> https://groups.google.com/a/chromium.org/groups/opt_out.
>
>

[Peter Kasting, 2012-08-27 18:24:29]

OK, if other folks are convinced this is the best way to go, I don't have the
perspective to disagree.