Move infobar handling to a tab helper.

Move infobar handling to a tab helper, part 1.

BUG=94741
TEST=no visible change

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=99187

[Avi (use Gerrit), 2011-08-31 01:03:55]

The change here is the splitting off of infobar stuff to the new infobar tab
helper. The rest is just changes, adjusting includes, etc.

A second sweep will move more infobar base files to the new infobar folder.

[Avi (use Gerrit), 2011-08-31 14:18:05]

Tests I was not expecting came back red. Hold off for a little while.

[Avi (use Gerrit), 2011-08-31 16:26:00]

I must have hit a bad rev, because I rebased, tried again, and everything is
coming back green. Take a look.

[Peter Kasting, 2011-08-31 18:47:31]

The important takeaway from the non-nits below is that I think the whole infobar
system should stop knowing about TabContentsWrapper and deal exclusively in
InfoBarTabHelper.  There is a related issue where various infobar delegates keep
their own TabContents* or TCW* in addition to the private one in the base class,
especially if they need to call OpenURL().  I wonder if these should be removed
and changed to call an OpenURL() method on InfoBarTabHelper, which then will
need to be constructed with an object that can perform that action.  If we can
do this, then all this code is isolated from both TabContents and
TabContentsWrapper.  At this point we can remove the pointers kept by subclasses
and replace them with conditional-checked accesses to the base's owner, although
that's probably a job for me rather than you.

http://codereview.chromium.org/7810002/diff/1042/chrome/browser/autofill/auto...
File chrome/browser/autofill/autofill_browsertest.cc (right):

http://codereview.chromium.org/7810002/diff/1042/chrome/browser/autofill/auto...
chrome/browser/autofill/autofill_browsertest.cc:514:
GetInfoBarDelegateAt(0)->AsTranslateInfoBarDelegate();
Nit: While you're here, fix this line to be indented 4 more instead of 2

http://codereview.chromium.org/7810002/diff/1042/chrome/browser/automation/au...
File chrome/browser/automation/automation_provider_observers.cc (right):

http://codereview.chromium.org/7810002/diff/1042/chrome/browser/automation/au...
chrome/browser/automation/automation_provider_observers.cc:2293: if
(tab_contents_->infobar_tab_helper()->infobar_count() > 0 &&
Nit: Can you pull the infobar_tab_helper() out to a temp?

http://codereview.chromium.org/7810002/diff/1042/chrome/browser/automation/au...
chrome/browser/automation/automation_provider_observers.cc:2325: if (!(infobar =
Nit: While here, remove this conditional entirely and just set the temp directly
-- this call cannot fail.

http://codereview.chromium.org/7810002/diff/1042/chrome/browser/automation/au...
chrome/browser/automation/automation_provider_observers.cc:2336: if
(!(confirm_infobar = infobar->AsConfirmInfoBarDelegate())) {
Nit: While here, can you fix this conditional to assign above the conditional
and then just check the temp inside the conditional?

http://codereview.chromium.org/7810002/diff/1042/chrome/browser/automation/au...
chrome/browser/automation/automation_provider_observers.cc:2345: if
(!(confirm_infobar->Accept())) {
Nit: Extra parens

http://codereview.chromium.org/7810002/diff/1042/chrome/browser/automation/te...
File chrome/browser/automation/testing_automation_provider.cc (right):

http://codereview.chromium.org/7810002/diff/1042/chrome/browser/automation/te...
chrome/browser/automation/testing_automation_provider.cc:1885:
TabContentsWrapper* wrapper =
Nit: Just combine these two lines

http://codereview.chromium.org/7810002/diff/1042/chrome/browser/automation/te...
chrome/browser/automation/testing_automation_provider.cc:1902:
TabContentsWrapper* wrapper =
Nit: Convert this TCW* temp into an InfoBarTabHelper*

http://codereview.chromium.org/7810002/diff/1042/chrome/browser/automation/te...
chrome/browser/automation/testing_automation_provider.cc:2531:
TabContentsWrapper* wrapper =
Nit: Convert this TCW* temp into an InfoBarTabHelper*

http://codereview.chromium.org/7810002/diff/1042/chrome/browser/automation/te...
chrome/browser/automation/testing_automation_provider.cc:2608: if (infobar_index
>= tab_contents->infobar_tab_helper()->infobar_count() ||
Nit: Can you factor this infobar_tab_helper() out into a temp?

http://codereview.chromium.org/7810002/diff/1042/chrome/browser/automation/te...
chrome/browser/automation/testing_automation_provider.cc:2609: !(infobar =
tab_contents->infobar_tab_helper()->
Nit: This assignment can be pulled out of the conditional and moved below it, as
it can't fail if the index check passes.

http://codereview.chromium.org/7810002/diff/1042/chrome/browser/automation/te...
chrome/browser/automation/testing_automation_provider.cc:3901:
TabContentsWrapper* wrapper =
Nit: Convert this TCW* temp into an InfoBarTabHelper*

http://codereview.chromium.org/7810002/diff/1042/chrome/browser/download/down...
File chrome/browser/download/download_request_limiter.cc (right):

http://codereview.chromium.org/7810002/diff/1042/chrome/browser/download/down...
chrome/browser/download/download_request_limiter.cc:78: TabContentsWrapper*
wrapper =
Nit: Just collapse these two lines together

http://codereview.chromium.org/7810002/diff/1042/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_install_ui.cc (right):

http://codereview.chromium.org/7810002/diff/1042/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_install_ui.cc:269: i <
tab_contents->infobar_tab_helper()->infobar_count();
Nit: Can you factor the infobar_tab_helper() out to a temp?

http://codereview.chromium.org/7810002/diff/1042/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_install_ui_browsertest.cc (right):

http://codereview.chromium.org/7810002/diff/1042/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_install_ui_browsertest.cc:22:
TabContentsWrapper* tab = browser()->GetSelectedTabContentsWrapper();
Nit: Convert this TCW* temp into an InfoBarTabHelper*

http://codereview.chromium.org/7810002/diff/1042/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_management_browsertest.cc (right):

http://codereview.chromium.org/7810002/diff/1042/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_management_browsertest.cc:181:
TabContentsWrapper* wrapper = browser()->GetTabContentsWrapperAt(0);
Nit: Convert this TCW* temp into an InfoBarTabHelper*

http://codereview.chromium.org/7810002/diff/1042/chrome/browser/geolocation/c...
File chrome/browser/geolocation/chrome_geolocation_permission_context.cc
(right):

http://codereview.chromium.org/7810002/diff/1042/chrome/browser/geolocation/c...
chrome/browser/geolocation/chrome_geolocation_permission_context.cc:447:
TabContentsWrapper* wrapper = NULL;
Nit: Convert this TCW* temp into an InfoBarTabHelper*

http://codereview.chromium.org/7810002/diff/1042/chrome/browser/geolocation/c...
chrome/browser/geolocation/chrome_geolocation_permission_context.cc:491:
TabContentsWrapper* wrapper =
Nit: Just combine these two lines

http://codereview.chromium.org/7810002/diff/1042/chrome/browser/geolocation/c...
File
chrome/browser/geolocation/chrome_geolocation_permission_context_unittest.cc
(right):

http://codereview.chromium.org/7810002/diff/1042/chrome/browser/geolocation/c...
chrome/browser/geolocation/chrome_geolocation_permission_context_unittest.cc:215:
EXPECT_EQ(0U, contents_wrapper()->infobar_tab_helper()->infobar_count());
Nit: Can you factor out infobar_tab_helper() temps throughout this file?

http://codereview.chromium.org/7810002/diff/1042/chrome/browser/geolocation/g...
File chrome/browser/geolocation/geolocation_browsertest.cc (right):

http://codereview.chromium.org/7810002/diff/1042/chrome/browser/geolocation/g...
chrome/browser/geolocation/geolocation_browsertest.cc:581:
GetSelectedTabContentsWrapper()->infobar_tab_helper()->infobar_count();
Nit: Factor this infobar_tab_helper() out to a temp

http://codereview.chromium.org/7810002/diff/1042/chrome/browser/google/google...
File chrome/browser/google/google_url_tracker.cc (right):

http://codereview.chromium.org/7810002/diff/1042/chrome/browser/google/google...
chrome/browser/google/google_url_tracker.cc:38: TabContentsWrapper* wrapper =
Nit: Just combine these two lines

http://codereview.chromium.org/7810002/diff/1042/chrome/browser/notifications...
File chrome/browser/notifications/desktop_notification_service.cc (right):

http://codereview.chromium.org/7810002/diff/1042/chrome/browser/notifications...
chrome/browser/notifications/desktop_notification_service.cc:508:
TabContentsWrapper* wrapper =
Nit: Move this into the conditional and combine with its usage.

http://codereview.chromium.org/7810002/diff/1042/chrome/browser/plugin_observ...
File chrome/browser/plugin_observer.cc (right):

http://codereview.chromium.org/7810002/diff/1042/chrome/browser/plugin_observ...
chrome/browser/plugin_observer.cc:318:
tab_contents_->infobar_tab_helper()->AddInfoBar(
Nit: Factor this infobar_tab_helper() out to a temp.

http://codereview.chromium.org/7810002/diff/1042/chrome/browser/tab_contents/...
File chrome/browser/tab_contents/infobar.cc (right):

http://codereview.chromium.org/7810002/diff/1042/chrome/browser/tab_contents/...
chrome/browser/tab_contents/infobar.cc:43: InfoBar::InfoBar(TabContentsWrapper*
owner, InfoBarDelegate* delegate)
I think |owner| and |owner_| should be converted to InfoBarTabHelper*s.

http://codereview.chromium.org/7810002/diff/1042/chrome/browser/tab_contents/...
File chrome/browser/tab_contents/infobar_container.cc (right):

http://codereview.chromium.org/7810002/diff/1042/chrome/browser/tab_contents/...
chrome/browser/tab_contents/infobar_container.cc:35: void
InfoBarContainer::ChangeTabContents(TabContentsWrapper* contents) {
I think this function should be changed to:

void ChangeTab(InfoBarTabHelper* new_helper)

Also |tab_contents_| should change to |infobar_tab_helper_| and all the
notifications should use a Source<InfoBarTabHelper> instead of a
Source<TabContentsWrapper>.

http://codereview.chromium.org/7810002/diff/1042/chrome/browser/tab_contents/...
File chrome/browser/tab_contents/infobar_delegate.cc (right):

http://codereview.chromium.org/7810002/diff/1042/chrome/browser/tab_contents/...
chrome/browser/tab_contents/infobar_delegate.cc:80: owner_(contents) {
This |owner_| should also change to being an InfoBarTabHelper*.

http://codereview.chromium.org/7810002/diff/1042/chrome/browser/task_manager/...
File chrome/browser/task_manager/task_manager_browsertest.cc (right):

http://codereview.chromium.org/7810002/diff/1042/chrome/browser/task_manager/...
chrome/browser/task_manager/task_manager_browsertest.cc:377: TabContentsWrapper*
current_tab = browser()->GetSelectedTabContentsWrapper();
Nit: Convert this TCW* to an InfoBarTabHelper*

http://codereview.chromium.org/7810002/diff/1042/chrome/browser/translate/tra...
File chrome/browser/translate/translate_manager.cc (right):

http://codereview.chromium.org/7810002/diff/1042/chrome/browser/translate/tra...
chrome/browser/translate/translate_manager.cc:807: for (size_t i = 0; i <
wrapper->infobar_tab_helper()->infobar_count(); ++i) {
Nit: Factor this infobar_tab_helper() out to a temp

http://codereview.chromium.org/7810002/diff/1042/chrome/browser/translate/tra...
File chrome/browser/translate/translate_manager_browsertest.cc (right):

http://codereview.chromium.org/7810002/diff/1042/chrome/browser/translate/tra...
chrome/browser/translate/translate_manager_browsertest.cc:94: return
(contents_wrapper()->infobar_tab_helper()->infobar_count() == 1) ?
Nit: Factor this infobar_tab_helper() out to a temp

http://codereview.chromium.org/7810002/diff/1042/chrome/browser/translate/tra...
chrome/browser/translate/translate_manager_browsertest.cc:636: EXPECT_EQ(0U,
contents_wrapper()->infobar_tab_helper()->infobar_count());
Nit: Factor this infobar_tab_helper() out to a temp

http://codereview.chromium.org/7810002/diff/1042/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

http://codereview.chromium.org/7810002/diff/1042/chrome/browser/ui/browser.cc...
chrome/browser/ui/browser.cc:3698: TabContentsWrapper* wrapper =
Nit: Collapse these two lines together

http://codereview.chromium.org/7810002/diff/1042/chrome/browser/ui/cocoa/info...
File chrome/browser/ui/cocoa/infobars/infobar_container_controller.mm (right):

http://codereview.chromium.org/7810002/diff/1042/chrome/browser/ui/cocoa/info...
chrome/browser/ui/cocoa/infobars/infobar_container_controller.mm:144: i <
currentTabContents_->infobar_tab_helper()->infobar_count();
Nit: Factor this infobar_tab_helper() out to a temp

http://codereview.chromium.org/7810002/diff/1042/chrome/browser/ui/cocoa/info...
File chrome/browser/ui/cocoa/infobars/infobar_controller.mm (right):

http://codereview.chromium.org/7810002/diff/1042/chrome/browser/ui/cocoa/info...
chrome/browser/ui/cocoa/infobars/infobar_controller.mm:120: owner_ = owner;
Another |owner_| that should probably become an InfoBarTabHelper*.

http://codereview.chromium.org/7810002/diff/1042/chrome/browser/ui/shell_dial...
File chrome/browser/ui/shell_dialogs.cc (right):

http://codereview.chromium.org/7810002/diff/1042/chrome/browser/ui/shell_dial...
chrome/browser/ui/shell_dialogs.cc:56: TabContentsWrapper* wrapper =
Nit: Combine these two lines

http://codereview.chromium.org/7810002/diff/1042/chrome/browser/ui/tab_conten...
File chrome/browser/ui/tab_contents/tab_contents_wrapper.cc (right):

http://codereview.chromium.org/7810002/diff/1042/chrome/browser/ui/tab_conten...
chrome/browser/ui/tab_contents/tab_contents_wrapper.cc:199: in_destructor_ =
true;
You should explicitly reset |infobar_tab_helper_| here so infobars get deleted
before anything else is torn down.

[Avi (use Gerrit), 2011-08-31 20:03:51]

I'm letting the "combine these two lines" comments slide as they don't fit in 80
columns. I think I see where you're going with the bigger picture. I'm reworking
things now; I'll let you know when to take a look.

[Avi (use Gerrit), 2011-08-31 21:18:54]

I've made all the fixed, but none of them that involve changing ownership. This
is already a very large CL. Let me land this; I already have a second CL that
I've started to switch ownership.

http://codereview.chromium.org/7810002/diff/1042/chrome/browser/geolocation/c...
File chrome/browser/geolocation/chrome_geolocation_permission_context.cc
(right):

http://codereview.chromium.org/7810002/diff/1042/chrome/browser/geolocation/c...
chrome/browser/geolocation/chrome_geolocation_permission_context.cc:447:
TabContentsWrapper* wrapper = NULL;
On 2011/08/31 18:47:32, Peter Kasting wrote:
> Nit: Convert this TCW* temp into an InfoBarTabHelper*

Why? It's used once.

[Peter Kasting, 2011-08-31 21:24:55]

Rubber-stamp LGTM (didn't look again)

http://codereview.chromium.org/7810002/diff/1042/chrome/browser/geolocation/c...
File chrome/browser/geolocation/chrome_geolocation_permission_context.cc
(right):

http://codereview.chromium.org/7810002/diff/1042/chrome/browser/geolocation/c...
chrome/browser/geolocation/chrome_geolocation_permission_context.cc:447:
TabContentsWrapper* wrapper = NULL;
On 2011/08/31 21:18:54, Avi wrote:
> On 2011/08/31 18:47:32, Peter Kasting wrote:
> > Nit: Convert this TCW* temp into an InfoBarTabHelper*
> 
> Why? It's used once.

nm, I misread the code.

[Avi (use Gerrit), 2011-08-31 21:28:05]

OK.

There are 27 pending tryjobs, so tomorrow morning I'll try this, land it, and
continue on with part 2.