Split InfoBarService core code into InfoBarManager

Split InfoBarService core code into InfoBarManager

InfoBarService is now responsible for associating an InfoBarManager to a Tab
and handling the interactions with the rest of chrome (notifications, navigation
events).

InfoBarManager has the core logic for infobar management. It owns the list of infobars.

Eventually InfoBarManager will no longer rely on content/, but for now it still
has a reference to a WebContents instance. This reference was kept for now to 
minimize the size of the change, but will be removed in a future CL.

BUG=354379
TBR=jochen

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

[droger, 2014-03-26 16:56:56]

The core of the CL in infobar_manager and infobar_service.
The rest is just updating callsites.

[droger, 2014-03-26 17:44:44]

+pkasting
The core of the CL in infobar_manager and infobar_service.
The rest is just updating callsites.

https://codereview.chromium.org/211273007/diff/400001/chrome/browser/ui/andro...
File chrome/browser/ui/android/infobars/auto_login_infobar_delegate_android.cc
(right):

https://codereview.chromium.org/211273007/diff/400001/chrome/browser/ui/andro...
chrome/browser/ui/android/infobars/auto_login_infobar_delegate_android.cc:113:
infobar_service, IDR_INFOBAR_WARNING,
Note: this particular change is not trivial.

[Peter Kasting, 2014-03-26 23:51:29]

LGTM, but I didn't review anything other than infobar_manager.* and
infobar_service.*.

https://codereview.chromium.org/211273007/diff/400001/chrome/browser/infobars...
File chrome/browser/infobars/infobar_container.cc (right):

https://codereview.chromium.org/211273007/diff/400001/chrome/browser/infobars...
chrome/browser/infobars/infobar_container.cc:22:
top_arrow_target_height_(InfoBar::kDefaultArrowTargetHeight) {}
Nit: Don't modify wrapping here

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

https://codereview.chromium.org/211273007/diff/400001/chrome/browser/infobars...
chrome/browser/infobars/infobar_manager.cc:74: : infobars_enabled_(true),
web_contents_(web_contents) {
Nit: One member per line

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

https://codereview.chromium.org/211273007/diff/400001/chrome/browser/infobars...
chrome/browser/infobars/infobar_manager.h:76: // Removes all the infobars.
Nit: This should be up be RemoveInfoBar().

https://codereview.chromium.org/211273007/diff/400001/chrome/browser/infobars...
chrome/browser/infobars/infobar_manager.h:88: void CleanUp();
Nit: Why is the name of this so vague?  Why isn't this OnWebContentsDestroyed()?

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

https://codereview.chromium.org/211273007/diff/400001/chrome/browser/infobars...
chrome/browser/infobars/infobar_service.cc:47: // The WebContents is going away;
be aggressively paranoid and delete
Nit: Blank line here

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

https://codereview.chromium.org/211273007/diff/400001/chrome/browser/infobars...
chrome/browser/infobars/infobar_service.h:40: InfoBarManager& infobar_manager()
{ return infobar_manager_; }
Do not return non-const refs.  Return a non-const pointer, or a const ref.

[blundell, 2014-03-27 09:40:29]

Mostly nits. I reviewed the whole CL.

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

https://codereview.chromium.org/211273007/diff/400001/chrome/browser/infobars...
chrome/browser/infobars/infobar_container.h:48: // the infobars from
|infobar_service|, and show them all.  |infobar_service|
s/infobar_service/infobar_manager everywhere

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

https://codereview.chromium.org/211273007/diff/400001/chrome/browser/infobars...
chrome/browser/infobars/infobar_manager.cc:99: void InfoBarManager::CleanUp() {
web_contents_ = NULL; }
The long-term impl of this method is to cycle through all the delegates calling
CleanUp() on them, right?

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

https://codereview.chromium.org/211273007/diff/400001/chrome/browser/infobars...
chrome/browser/infobars/infobar_manager.h:81: // TODO(droger): remove this
method. See http://crbug.com/
bug number?

https://codereview.chromium.org/211273007/diff/400001/chrome/browser/infobars...
chrome/browser/infobars/infobar_manager.h:88: void CleanUp();
This method is going to stay in InfoBarManager after InfoBarManager no longer
has knowledge of WebContents, so it can't be called OnWebContentsDestroyed().
We're not tied to this name if there's anything (abstract) that seems better.

On 2014/03/26 23:51:30, Peter Kasting wrote:
> Nit: Why is the name of this so vague?  Why isn't this
OnWebContentsDestroyed()?

https://codereview.chromium.org/211273007/diff/400001/chrome/browser/infobars...
chrome/browser/infobars/infobar_manager.h:105: content::WebContents*
web_contents_;
Put a TODO and bug reference here as well.

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

https://codereview.chromium.org/211273007/diff/400001/chrome/browser/infobars...
chrome/browser/infobars/infobar_service.cc:68: // TODO(droger): Remove the
notifications and use observers instead.
I would say something like "Remove the notifications and have listeners change
to be NavigationManager::Observers instead" for clarity.

https://codereview.chromium.org/211273007/diff/400001/chrome/browser/notifica...
File chrome/browser/notifications/notification_browsertest.cc (right):

https://codereview.chromium.org/211273007/diff/400001/chrome/browser/notifica...
chrome/browser/notifications/notification_browsertest.cc:506: .infobar_count());
This indentation seems odd (and below)

https://codereview.chromium.org/211273007/diff/400001/chrome/browser/ui/gtk/b...
File chrome/browser/ui/gtk/browser_window_gtk.cc (right):

https://codereview.chromium.org/211273007/diff/400001/chrome/browser/ui/gtk/b...
chrome/browser/ui/gtk/browser_window_gtk.cc:806:
&InfoBarService::FromWebContents(new_contents)->infobar_manager());
Do you know the InfoBarService won't be NULL?

https://codereview.chromium.org/211273007/diff/400001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/browser_view.cc (right):

https://codereview.chromium.org/211273007/diff/400001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_view.cc:835:
&InfoBarService::FromWebContents(new_contents)->infobar_manager());
Do we know the infobar service won't be NULL here?

[Peter Kasting, 2014-03-27 17:51:32]

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

https://codereview.chromium.org/211273007/diff/400001/chrome/browser/infobars...
chrome/browser/infobars/infobar_manager.h:88: void CleanUp();
On 2014/03/27 09:40:30, blundell wrote:
> This method is going to stay in InfoBarManager after InfoBarManager no longer
> has knowledge of WebContents, so it can't be called OnWebContentsDestroyed().

What's it going to do at that point?

(Side note: Please always reply-at-bottom in Rietveld comments, it's really
confusing to read the resulting emails if you reply-at-top.)

[droger, 2014-03-27 18:00:12]

On 2014/03/27 17:51:32, Peter Kasting wrote:
>
https://codereview.chromium.org/211273007/diff/400001/chrome/browser/infobars...
> File chrome/browser/infobars/infobar_manager.h (right):
> 
>
https://codereview.chromium.org/211273007/diff/400001/chrome/browser/infobars...
> chrome/browser/infobars/infobar_manager.h:88: void CleanUp();
> On 2014/03/27 09:40:30, blundell wrote:
> > This method is going to stay in InfoBarManager after InfoBarManager no
longer
> > has knowledge of WebContents, so it can't be called
OnWebContentsDestroyed().
> 
> What's it going to do at that point?

It will call delegate()->CleanUp() on each infobar.
Which will in turn set web_contents_ to NULL in some of these delegates ;
ContentConfirmInfobar in particular, see the prototype:
https://codereview.chromium.org/190063006/diff/120002/chrome/browser/infobars...

[Peter Kasting, 2014-03-27 18:11:06]

On 2014/03/27 18:00:12, droger wrote:
> On 2014/03/27 17:51:32, Peter Kasting wrote:
> >
>
https://codereview.chromium.org/211273007/diff/400001/chrome/browser/infobars...
> > File chrome/browser/infobars/infobar_manager.h (right):
> > 
> >
>
https://codereview.chromium.org/211273007/diff/400001/chrome/browser/infobars...
> > chrome/browser/infobars/infobar_manager.h:88: void CleanUp();
> > On 2014/03/27 09:40:30, blundell wrote:
> > > This method is going to stay in InfoBarManager after InfoBarManager no
> longer
> > > has knowledge of WebContents, so it can't be called
> OnWebContentsDestroyed().
> > 
> > What's it going to do at that point?
> 
> It will call delegate()->CleanUp() on each infobar.
> Which will in turn set web_contents_ to NULL in some of these delegates ;
> ContentConfirmInfobar in particular, see the prototype:
>
https://codereview.chromium.org/190063006/diff/120002/chrome/browser/infobars...

I wonder if we're making a mistake plumbing this through InfoBarManager,
InfoBar, and InfoBarDelegate when none of those classes will know about
WebContents.  Maybe the InfoBarService should know directly about the particular
InfoBarDelegates that are aware of WebContents, so it can clear this on them via
a direct call, and we can avoid any footprint in the lower-level classes.

[droger, 2014-03-27 18:25:39]

> I wonder if we're making a mistake plumbing this through InfoBarManager,
> InfoBar, and InfoBarDelegate when none of those classes will know about
> WebContents.  Maybe the InfoBarService should know directly about the
particular
> InfoBarDelegates that are aware of WebContents, so it can clear this on them
via
> a direct call, and we can avoid any footprint in the lower-level classes.

It should be possible to do, but seems more complicated to me.
We'd need to maintain another list of infobars in InfoBarService, and then I
don't see a straightforward way to populate this list.

We could remove the method on InfobarManager though, by inlining it in
InfoBarService::WebContentsDestroyed.
The code would look like:

InfoBarService::OnWebContentsDestroyed() {
  for (size_t i = 0; i < infobar_manager_->infobar_count(); ++i)
    infobar_manager->infobar_at(i)->delegate()->CleanUp();
}

[Peter Kasting, 2014-03-27 18:29:33]

On 2014/03/27 18:25:39, droger wrote:
> > I wonder if we're making a mistake plumbing this through InfoBarManager,
> > InfoBar, and InfoBarDelegate when none of those classes will know about
> > WebContents.  Maybe the InfoBarService should know directly about the
> particular
> > InfoBarDelegates that are aware of WebContents, so it can clear this on them
> via
> > a direct call, and we can avoid any footprint in the lower-level classes.
> 
> It should be possible to do, but seems more complicated to me.
> We'd need to maintain another list of infobars in InfoBarService, and then I
> don't see a straightforward way to populate this list.

If infobars unaware of WebContents were added directly by calls to
InfoBarManager, and infobars aware of WebContents were added by calling a
wrapper method on InfoBarService, that would tell InfoBarService the right set
of infobars to use.  I don't know if that makes a ton of sense, but it seems
sort of logical to me: you call whatever class is highest-level in your
abstraction layer.

> We could remove the method on InfobarManager though, by inlining it in
> InfoBarService::WebContentsDestroyed.
> The code would look like:
> 
> InfoBarService::OnWebContentsDestroyed() {
>   for (size_t i = 0; i < infobar_manager_->infobar_count(); ++i)
>     infobar_manager->infobar_at(i)->delegate()->CleanUp();
> }

Yeah, that probably helps.

[droger, 2014-03-28 14:04:53]

I renamed InfoBarManager::CleanUp as InfoBarManager::OnWebContentsDestroyed for
now, and we'll see later how to handle the cleanup of the infobars when I'll
refactor the corresponding code.

[Peter Kasting, 2014-03-28 17:52:27]

On 2014/03/28 14:04:53, droger wrote:
> I renamed InfoBarManager::CleanUp as InfoBarManager::OnWebContentsDestroyed
for
> now, and we'll see later how to handle the cleanup of the infobars when I'll
> refactor the corresponding code.

OK, that works too.

[droger, 2014-03-31 08:11:45]

blundell: ping

[blundell, 2014-03-31 08:29:21]

LGTM

https://codereview.chromium.org/211273007/diff/520001/chrome/browser/ui/gtk/b...
File chrome/browser/ui/gtk/browser_window_gtk.cc (right):

https://codereview.chromium.org/211273007/diff/520001/chrome/browser/ui/gtk/b...
chrome/browser/ui/gtk/browser_window_gtk.cc:805: InfoBarManager* infobar_manager
= NULL;
Optional thought for a follow-up CL: You use this code pattern several places in
this CL. It might be worth creating a static
InfoBarService::InfoBarManagerFromWebContents() method.

[droger, 2014-03-31 09:50:28]

TBR jochen for everything except infobars, ui and translate

[droger, 2014-03-31 09:51:26]

The CQ bit was checked by droger@chromium.org

[commit-bot: I haz the power, 2014-03-31 09:51:41]

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

[jochen (gone - plz use gerrit), 2014-03-31 09:54:07]

lgtm

[commit-bot: I haz the power, 2014-03-31 15:35:58]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-31 15:35:59]

Retried try job too often on win_rel for step(s) app_list_unittests,
ash_unittests, aura_unittests, cacheinvalidation_unittests, cc_unittests,
check_deps, check_deps2git, chrome_elf_unittests, chromedriver_unittests,
components_unittests, compositor_unittests, content_browsertests,
content_unittests, crypto_unittests, events_unittests, gfx_unittests,
google_apis_unittests, gpu_unittests, installer_util_unittests, ipc_tests,
jingle_unittests, media_unittests, mini_installer_test, nacl_integration,
ppapi_unittests, printing_unittests, remoting_unittests, sql_unittests,
sync_integration_tests, sync_unit_tests, telemetry_perf_unittests,
telemetry_unittests, views_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[droger, 2014-03-31 16:47:30]

The CQ bit was checked by droger@chromium.org

[commit-bot: I haz the power, 2014-03-31 16:49:43]

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

[commit-bot: I haz the power, 2014-03-31 20:02:18]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-31 20:02:18]

Failed to apply patch for
chrome/browser/media/chrome_media_stream_infobar_browsertest.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/media/chrome_media_stream_infobar_browsertest.cc
  Hunk #2 FAILED at 123.
  1 out of 2 hunks FAILED -- saving rejects to file
chrome/browser/media/chrome_media_stream_infobar_browsertest.cc.rej

Patch:       chrome/browser/media/chrome_media_stream_infobar_browsertest.cc
Index: chrome/browser/media/chrome_media_stream_infobar_browsertest.cc
diff --git a/chrome/browser/media/chrome_media_stream_infobar_browsertest.cc
b/chrome/browser/media/chrome_media_stream_infobar_browsertest.cc
index
4e686d5f9b104d3ca363dca3f0a11e81a03ac87c..41dcfb81c559a843b6d22d1a4b6d597e46e1a4a7
100644
--- a/chrome/browser/media/chrome_media_stream_infobar_browsertest.cc
+++ b/chrome/browser/media/chrome_media_stream_infobar_browsertest.cc
@@ -9,6 +9,7 @@
 #include "chrome/browser/chrome_notification_types.h"
 #include "chrome/browser/content_settings/host_content_settings_map.h"
 #include "chrome/browser/infobars/infobar.h"
+#include "chrome/browser/infobars/infobar_manager.h"
 #include "chrome/browser/infobars/infobar_service.h"
 #include "chrome/browser/media/webrtc_browsertest_base.h"
 #include "chrome/browser/media/webrtc_browsertest_common.h"
@@ -122,9 +123,9 @@ IN_PROC_BROWSER_TEST_F(MediaStreamInfoBarTest,
   EXPECT_TRUE(PollingWaitUntil("obtainGetUserMediaResult()",
                                kFailedWithPermissionDeniedError,
                                tab_contents));
-  InfoBarService* infobar_service =
-      InfoBarService::FromWebContents(tab_contents);
-  EXPECT_EQ(0u, infobar_service->infobar_count());
+  InfoBarManager* infobar_manager =
+      InfoBarService::FromWebContents(tab_contents)->infobar_manager();
+  EXPECT_EQ(0u, infobar_manager->infobar_count());
 }
 
 // Failing on Win Aura, so disabling on that.

[Peter Kasting, 2014-03-31 20:05:33]

https://codereview.chromium.org/211273007/diff/520001/chrome/browser/ui/gtk/b...
File chrome/browser/ui/gtk/browser_window_gtk.cc (right):

https://codereview.chromium.org/211273007/diff/520001/chrome/browser/ui/gtk/b...
chrome/browser/ui/gtk/browser_window_gtk.cc:805: InfoBarManager* infobar_manager
= NULL;
On 2014/03/31 08:29:22, blundell wrote:
> Optional thought for a follow-up CL: You use this code pattern several places
in
> this CL. It might be worth creating a static
> InfoBarService::InfoBarManagerFromWebContents() method.

This comment made me glance at this code (which I didn't review originally).

Are you sure your new NULL-check is appropriate?  Can |infobar_service| ever
actually be NULL below?  I wouldn't expect that to normally be true... maybe
during shutdown?

In general, make sure you don't add any NULL-checks to the existing code unless
they're truly necessary, and when they are necessary, please leave a comment
about when the variable can be NULL.

[droger, 2014-04-01 10:48:33]

https://codereview.chromium.org/211273007/diff/520001/chrome/browser/ui/gtk/b...
File chrome/browser/ui/gtk/browser_window_gtk.cc (right):

https://codereview.chromium.org/211273007/diff/520001/chrome/browser/ui/gtk/b...
chrome/browser/ui/gtk/browser_window_gtk.cc:805: InfoBarManager* infobar_manager
= NULL;
On 2014/03/31 20:05:34, Peter Kasting wrote:
> On 2014/03/31 08:29:22, blundell wrote:
> > Optional thought for a follow-up CL: You use this code pattern several
places
> in
> > this CL. It might be worth creating a static
> > InfoBarService::InfoBarManagerFromWebContents() method.
> 
> This comment made me glance at this code (which I didn't review originally).
> 
> Are you sure your new NULL-check is appropriate?  Can |infobar_service| ever
> actually be NULL below?  I wouldn't expect that to normally be true... maybe
> during shutdown?
> 
> In general, make sure you don't add any NULL-checks to the existing code
unless
> they're truly necessary, and when they are necessary, please leave a comment
> about when the variable can be NULL.

InfoBarService::FromWebContents() indeed returns NULL during shutdown
(InfoBarService removes itself from WebContents in WebContentsDestroyed).
I don't know if this code can be called during shutdown, but the existing code
was supporting the case where the InfoBarService was NULL (because
ChangeInfoBarService() has a NULL check inside).

I applied Colin's suggestion and factored this code in a new
InfoBarManagerFromWebContents() method, and added a comment explaining why it
can be NULL there.

[droger, 2014-04-01 10:48:43]

The CQ bit was checked by droger@chromium.org

[commit-bot: I haz the power, 2014-04-01 10:48:47]

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

[droger, 2014-04-01 11:02:17]

I'm trying to land this now, but I'm happy to do a follow up CL if needed to
tweak the NULL checks.

[commit-bot: I haz the power, 2014-04-01 11:44:08]

Change committed as 260847