Infobar Componentization Proof of Concept

Infobar Componentization Proof of Concept

This is not intended for commit.

The CL uses architecture described in the design doc, with the minor difference
that this CL does not actually moves the files to components/. Moving files is
left for a later pass, as this would just add a lot of noise to this already
huge change.

The key classes are in chrome/browser/infobar:
- the core of InfoBarService has been split into InfoBarManager
- InfoBarDelegate, InfoBarContainer, Infobar, ConfirmInfoBarDelegate have been
changed to no longer depend on content/.

A new ContentConfirmInfoBarDelegate has been added. This class is simply a
ConfirmInfoBarDelegate that knows a WebContents.

The rest of the CL is mostly code churn to update existing calls.
In most cases, it is about converting from ConfirmInfoBarDelegate to
ContentConfirmInfoBarDelegate.
You can look at
chrome/browser/custom_handlers/register_protocol_handler_infobar_delegate.h(
https://codereview.chromium.org/190063006/diff/150105/chrome/browser/custom_h...
) 
for one example of this.

[droger, 2014-03-07 16:44:09]

blundell:
This is not finished yet, but sending this to you as a FYI to track my progress.

[droger, 2014-03-18 10:50:55]

The CL now compiles and has all the designed structure and dependencies.
It may still be rough (many comments are missing), but should give an idea.

[droger, 2014-03-18 12:08:26]

Here is a breakdown of the change.

The CL uses architecture described in the design doc, with the minor difference
that this CL does not actually moves the files to components/. Moving files is
left for a later pass, as this would just add a lot of noise to this already
huge change.

The key classes are in chrome/browser/infobar:
- the core of InfoBarService has been split into InfoBarManager
- InfoBarDelegate, InfoBarContainer, Infobar, ConfirmInfoBarDelegate have been
changed to no longer depend on content/.

A new ContentConfirmInfoBarDelegate has been added. This class is simply a
ConfirmInfoBarDelegate that knows a WebContents.

The rest of the CL is mostly code churn to update existing calls.
In most cases, it is about converting from ConfirmInfoBarDelegate to
ContentConfirmInfoBarDelegate.
You can look at
chrome/browser/custom_handlers/register_protocol_handler_infobar_delegate.h(
https://codereview.chromium.org/190063006/diff/150105/chrome/browser/custom_h...
) 
for one example of this.

[tfarina, 2014-03-18 13:47:18]

Infobar? -> +Peter

[droger, 2014-03-18 13:49:11]

On 2014/03/18 13:47:18, tfarina wrote:
> Infobar? -> +Peter

Yes I was planning on including Peter later on. He's aware of this project.
I just wanted some initial feedback from blundell before  reaching out to Peter.

[tfarina, 2014-03-18 13:50:12]

OK, so moving me and Peter back to CC list. Sorry.

[droger, 2014-03-18 15:59:52]

pkasting: this is now ready for you to take a look.


This is not intended for committing. It's only a proof of concept work for
infobar componentization. Once we agree on the high-level design, this CL will
be split in smaller chunks.

You don't need to review many of these files. The important files are in
chrome/browser/infobars.
I tried adding a CL description and some comments inline to help focus on what
is important.

https://codereview.chromium.org/190063006/diff/120002/chrome/browser/infobars...
File chrome/browser/infobars/content_confirm_infobar_delegate.h (right):

https://codereview.chromium.org/190063006/diff/120002/chrome/browser/infobars...
chrome/browser/infobars/content_confirm_infobar_delegate.h:14: class
ContentConfirmInfoBarDelegate : public ConfirmInfoBarDelegate {
ContentConfirmInfoBarDelegate is a ConfirmInfoBarDelegate which knows one
instance of WebContents.
It will eventually live in components/infobar/content/browser/.

The WebContents pointer is reset when the new CleanUp() function is called.
The CleanUp function has been added mainly for this purpose.

https://codereview.chromium.org/190063006/diff/120002/chrome/browser/infobars...
File chrome/browser/infobars/infobar.h (right):

https://codereview.chromium.org/190063006/diff/120002/chrome/browser/infobars...
chrome/browser/infobars/infobar.h:58: InfoBarManager* owner() { return owner_; }
owner() now returns a InfoBarManager instead of InfoBarService.

https://codereview.chromium.org/190063006/diff/120002/chrome/browser/infobars...
chrome/browser/infobars/infobar.h:68: void StoreActiveEntryUniqueID(int
entry_id);
The entry_id, which used to be retrieved from WebContents, now needs to be
passed explicitly.

https://codereview.chromium.org/190063006/diff/120002/chrome/browser/infobars...
File chrome/browser/infobars/infobar_container.h (right):

https://codereview.chromium.org/190063006/diff/120002/chrome/browser/infobars...
chrome/browser/infobars/infobar_container.h:24: class InfoBarContainer :
InfoBarManager::Observer {
InfoBarContained now observes InfoBarManager instead of Notifications.

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

https://codereview.chromium.org/190063006/diff/120002/chrome/browser/infobars...
chrome/browser/infobars/infobar_delegate.cc:47: void InfoBarDelegate::CleanUp()
{}
The default implementation of CleanUp does nothing. It's here to be overridden
by subclasses, and most notably ContentConfirmInfoBarDelegate (but also a few
others).

https://codereview.chromium.org/190063006/diff/120002/chrome/browser/infobars...
File chrome/browser/infobars/infobar_delegate.h (right):

https://codereview.chromium.org/190063006/diff/120002/chrome/browser/infobars...
chrome/browser/infobars/infobar_delegate.h:67: struct NavigationDetails {
This structure is used instead of content::LoadCommittedDetails, as we can no
longer rely on content/.

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

https://codereview.chromium.org/190063006/diff/120002/chrome/browser/infobars...
chrome/browser/infobars/infobar_manager.h:18: class InfoBarManager {
InfoBarManager has the core logic.
It will move in components/infobar/core/browser/

https://codereview.chromium.org/190063006/diff/120002/chrome/browser/infobars...
chrome/browser/infobars/infobar_manager.h:69: class Observer {
Observer class, to replace the notifications.

https://codereview.chromium.org/190063006/diff/120002/chrome/browser/infobars...
File chrome/browser/infobars/infobar_service.cc (right):

https://codereview.chromium.org/190063006/diff/120002/chrome/browser/infobars...
chrome/browser/infobars/infobar_service.cc:109: void
InfoBarService::OnInfoBarAdded(InfoBar* infobar) {
These functions listen for InfoBarManager and emit the corresponding
notifications.

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

https://codereview.chromium.org/190063006/diff/120002/chrome/browser/infobars...
chrome/browser/infobars/infobar_service.h:19: class InfoBarService : public
content::WebContentsObserver,
InfoBarService no longer manages the list of infobars, but rather associates an
InfoBarManager with a WebContents instance.
This file will eventually move to components/infobar/content/browser/ and will
be renamed as infobar_driver to follow the pattern from existing components.

https://codereview.chromium.org/190063006/diff/120002/chrome/browser/infobars...
chrome/browser/infobars/infobar_service.h:21: public InfoBarManager::Observer {
InfoBarService is a InfoBarManager::Observer so that it can emit the
infobar-related notifications.
These notifications won't be used in the component, but rather only in chrome
client code.

https://codereview.chromium.org/190063006/diff/120002/chrome/browser/infobars...
chrome/browser/infobars/infobar_service.h:23: // Helper functions.
The functions are just pass through to call the corresponding functions on
InfoBarManager.
The only thing they do in addition to calling InfoBarManager is to compute the
entry ID.

https://codereview.chromium.org/190063006/diff/120002/chrome/browser/infobars...
chrome/browser/infobars/infobar_service.h:28: static
InfoBarDelegate::NavigationDetails
Converts content::LoadCommittedDetails to InfoBarDelegate::NavigationDetails.

https://codereview.chromium.org/190063006/diff/120002/chrome/browser/infobars...
chrome/browser/infobars/infobar_service.h:39: static int
GetActiveEntryID(content::WebContents* web_contents);
InfoBarService computes the entry_id and explicitly passes it to InfoBarManager.

[Peter Kasting, 2014-03-18 18:29:16]

I don't see any major problems in c/b/infobars/.

https://codereview.chromium.org/190063006/diff/120002/chrome/browser/infobars...
File chrome/browser/infobars/confirm_infobar_delegate.cc (right):

https://codereview.chromium.org/190063006/diff/120002/chrome/browser/infobars...
chrome/browser/infobars/confirm_infobar_delegate.cc:55:
InfoBarDelegate::ShouldExpireInternal(details);
Nit: Old indenting was correct

https://codereview.chromium.org/190063006/diff/120002/chrome/browser/infobars...
File chrome/browser/infobars/infobar_container.h (right):

https://codereview.chromium.org/190063006/diff/120002/chrome/browser/infobars...
chrome/browser/infobars/infobar_container.h:104: virtual void
OnInfoBarReplaced(InfoBar* old_infobar,
Nit: Here and elsewhere, put these in alphabetical order

https://codereview.chromium.org/190063006/diff/120002/chrome/browser/infobars...
File chrome/browser/infobars/infobar_delegate.h (right):

https://codereview.chromium.org/190063006/diff/120002/chrome/browser/infobars...
chrome/browser/infobars/infobar_delegate.h:94: virtual void CleanUp();
Nit: Add comments about when and why this function is called.

https://codereview.chromium.org/190063006/diff/120002/chrome/browser/infobars...
chrome/browser/infobars/infobar_delegate.h:112: // Store the unique id for the
active entry in our WebContents, to be used
Nit: Seems like this shouldn't refer to WebContents?

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

https://codereview.chromium.org/190063006/diff/120002/chrome/browser/infobars...
chrome/browser/infobars/infobar_manager.cc:112: Observer, observer_list_,
OnInfoBarRemoved(infobar, animate));
Tiny nit: I slightly prefer:

  FOR_EACH_OBSERVER(Observer, observer_list_,
                    OnInfoBarRemoved(infobar, animate));

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

https://codereview.chromium.org/190063006/diff/120002/chrome/browser/infobars...
chrome/browser/infobars/infobar_manager.h:67: void CleanUp();
Nit: Again, add comments here (and, frankly, above as well) about when and why
this is called.

https://codereview.chromium.org/190063006/diff/120002/chrome/browser/infobars...
chrome/browser/infobars/infobar_manager.h:69: class Observer {
On 2014/03/18 15:59:53, droger wrote:
> Observer class, to replace the notifications.

Nit: Classes should be declared at the top of the containing class.

https://codereview.chromium.org/190063006/diff/120002/chrome/browser/infobars...
chrome/browser/infobars/infobar_manager.h:77: void AddObserver(Observer* obs) {
observer_list_.AddObserver(obs); }
Nit: Don't inline non-unix_hacker()-style functions.  (2 places)

https://codereview.chromium.org/190063006/diff/120002/chrome/browser/infobars...
File chrome/browser/infobars/infobar_service.cc (right):

https://codereview.chromium.org/190063006/diff/120002/chrome/browser/infobars...
chrome/browser/infobars/infobar_service.cc:43:
InfoBarService::NavigationDetailsFromLoadCommittedDetails(
Nit: Indent 4

https://codereview.chromium.org/190063006/diff/120002/chrome/browser/infobars...
chrome/browser/infobars/infobar_service.cc:109: void
InfoBarService::OnInfoBarAdded(InfoBar* infobar) {
On 2014/03/18 15:59:53, droger wrote:
> These functions listen for InfoBarManager and emit the corresponding
> notifications.

Since we're trying to kill notifications in Chrome anyway, it seems like we
should take the opportunity to clean these up.  Anyone who listens to these
should probably instead just be an InfoBarManager::Observer.

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

https://codereview.chromium.org/190063006/diff/120002/chrome/browser/infobars...
chrome/browser/infobars/infobar_service.h:20: public
content::WebContentsUserData<InfoBarService>,
Nit: Leave the WebContentsUserData<> listed last

https://codereview.chromium.org/190063006/diff/120002/chrome/browser/infobars...
chrome/browser/infobars/infobar_service.h:28: static
InfoBarDelegate::NavigationDetails
On 2014/03/18 15:59:53, droger wrote:
> Converts content::LoadCommittedDetails to InfoBarDelegate::NavigationDetails.

Nit: Do this and GetActiveEntryID() need to be public?

It seems like GetActiveEntryID() is only public because
PluginInstallerInfoBarDelegate::Replace() calls it.  But that function has
access to the WebContents, so it could get at the InfoBarService instead of the
InfoBarManager and thus avoid needing to make that call.

NavigationDetailsFromLoadCommittedDetails() is mainly public because of the
managed_mode_interstitial.cc code, and while I don't have a specific solution
there, something feels wrong about that -- like this expiry ought to be somehow
happening automatically elsewhere instead of needing to be manually triggered. 
Like, maybe we can start by converting the InfoBarDelegate::ShouldExpire() call
to an InfoBarManager::OnNavigation() call, and then perhaps replace that with
some kind of call to InfobarService::NavigationEntryCommitted() or something.

https://codereview.chromium.org/190063006/diff/120002/chrome/browser/infobars...
chrome/browser/infobars/infobar_service.h:59: // InfoBarManager::Observer:
Nit: Put this just below the WebContentsObserver overrides

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

https://codereview.chromium.org/190063006/diff/120002/chrome/browser/infobars...
chrome/browser/infobars/infobars_browsertest.cc:90: .infobar_count());
Nit: Old wrapping was correct (2 places)

[droger, 2014-03-19 15:03:04]

Thanks for the comments. Sorry for the formatting, it was the result of a raw
run of "git cl format", to save time. I'll try to preserve the existing format
when it is valid.

My plan now will be to split this in several smaller CLS.
Once they are landed, I'll move the files to the component.

[blundell, 2014-03-19 15:08:08]

https://codereview.chromium.org/190063006/diff/170001/chrome/browser/infobars...
File chrome/browser/infobars/infobar_container.cc (left):

https://codereview.chromium.org/190063006/diff/170001/chrome/browser/infobars...
chrome/browser/infobars/infobar_container.cc:149: registrar_.RemoveAll();
Presumably we should be removing ourselves as an observer of infobar_manager_
here?

https://codereview.chromium.org/190063006/diff/170001/chrome/browser/infobars...
File chrome/browser/infobars/infobar_delegate.h (right):

https://codereview.chromium.org/190063006/diff/170001/chrome/browser/infobars...
chrome/browser/infobars/infobar_delegate.h:67: struct NavigationDetails {
Add a comment here to make this struct easier to understand.

https://codereview.chromium.org/190063006/diff/170001/chrome/browser/infobars...
chrome/browser/infobars/infobar_delegate.h:94: virtual void CleanUp();
Add a comment here.

https://codereview.chromium.org/190063006/diff/170001/chrome/browser/infobars...
chrome/browser/infobars/infobar_delegate.h:112: // Store the unique id for the
active entry in our WebContents, to be used
This comment should be updated.

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

https://codereview.chromium.org/190063006/diff/170001/chrome/browser/infobars...
chrome/browser/infobars/infobar_manager.h:31: virtual InfoBar*
AddInfoBar(scoped_ptr<InfoBar> infobar, int entry_id);
Add an explanation of what entry_id is?

https://codereview.chromium.org/190063006/diff/120002/chrome/browser/infobars...
File chrome/browser/infobars/infobar_service.cc (right):

https://codereview.chromium.org/190063006/diff/120002/chrome/browser/infobars...
chrome/browser/infobars/infobar_service.cc:109: void
InfoBarService::OnInfoBarAdded(InfoBar* infobar) {
There are 20+ listeners for these notifications currently. As changing
notification listeners to Observers can be time-consuming, would you be OK with
us taking this approach to avoid the work being blocked on the cleanup task and
at the same time incrementally working to change the notification listeners to
be InfoBarManager::Observers?

On 2014/03/18 18:29:17, Peter Kasting wrote:
> On 2014/03/18 15:59:53, droger wrote:
> > These functions listen for InfoBarManager and emit the corresponding
> > notifications.
> 
> Since we're trying to kill notifications in Chrome anyway, it seems like we
> should take the opportunity to clean these up.  Anyone who listens to these
> should probably instead just be an InfoBarManager::Observer.

[Peter Kasting, 2014-03-19 18:11:09]

https://codereview.chromium.org/190063006/diff/120002/chrome/browser/infobars...
File chrome/browser/infobars/infobar_service.cc (right):

https://codereview.chromium.org/190063006/diff/120002/chrome/browser/infobars...
chrome/browser/infobars/infobar_service.cc:109: void
InfoBarService::OnInfoBarAdded(InfoBar* infobar) {
On 2014/03/19 15:08:09, blundell wrote:
> There are 20+ listeners for these notifications currently. As changing
> notification listeners to Observers can be time-consuming, would you be OK
with
> us taking this approach to avoid the work being blocked on the cleanup task
and
> at the same time incrementally working to change the notification listeners to
> be InfoBarManager::Observers?

Splitting work into smaller pieces is always fine as long as we agree on the
ultimate endpoint.  I'm surprised there are that many observers of this.  I
would have expected more like 5.