Remove last usages of FindTabbedBrowserDeprecated.

Remove last usages of FindTabbedBrowserDeprecated.

BUG=129187
TEST=NONE


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

[robertshield, 2012-12-03 14:56:44]

Greg, please could you sanity check this before I send it to Ben for OWNERS?

[grt (UTC plus 2), 2012-12-03 15:54:39]

https://codereview.chromium.org/11348344/diff/1014/chrome/browser/extensions/...
File chrome/browser/extensions/api/tabs/tabs.cc (right):

https://codereview.chromium.org/11348344/diff/1014/chrome/browser/extensions/...
chrome/browser/extensions/api/tabs/tabs.cc:244: new_window = new
Browser(params);
specify the same host desktop type here as that used just above.  (i don't
recall seeing calls to "new Browser" on the cleanup list.  if they're not there,
they should be, no?)

https://codereview.chromium.org/11348344/diff/1014/chrome/browser/extensions/...
chrome/browser/extensions/api/tabs/tabs.cc:998: // Ensure the selected browser
is tabbed.
i haven't studied when this is called, but since |browser| isn't NULL, does it
make sense to use the same desktop type as |browser| here?

https://codereview.chromium.org/11348344/diff/1014/chrome/browser/extensions/...
chrome/browser/extensions/api/tabs/tabs.cc:1068: chrome::GetActiveDesktop());
along similar lines, why not use the same host desktop type as |browser|?

https://codereview.chromium.org/11348344/diff/1014/chrome/browser/extensions/...
chrome/browser/extensions/api/tabs/tabs.cc:1070: browser = new
Browser(Browser::CreateParams(profile));
specify the same host desktop type here as that used just above.

https://codereview.chromium.org/11348344/diff/1014/chrome/browser/extensions/...
File chrome/browser/extensions/extension_install_ui_default.cc (right):

https://codereview.chromium.org/11348344/diff/1014/chrome/browser/extensions/...
chrome/browser/extensions/extension_install_ui_default.cc:189:
chrome::GetActiveDesktop());
if the user clicked on a link on the web store and then switched desktops,
should the infobar appear in the originating desktop? is it possible to know the
context at the time that the theme (extension) was installed?

https://codereview.chromium.org/11348344/diff/1014/chrome/browser/extensions/...
File chrome/browser/extensions/extension_tab_util.cc (right):

https://codereview.chromium.org/11348344/diff/1014/chrome/browser/extensions/...
chrome/browser/extensions/extension_tab_util.cc:234: Browser* browser =
browser::FindTabbedBrowser(profile,
ExtensionHost::AddNewContents has a |source| WebContents. Is that meaningful? Is
it possible to know what desktop that's on?

https://codereview.chromium.org/11348344/diff/1014/chrome/browser/ui/extensio...
File chrome/browser/ui/extensions/application_launch.cc (right):

https://codereview.chromium.org/11348344/diff/1014/chrome/browser/ui/extensio...
chrome/browser/ui/extensions/application_launch.cc:124: browser = new
Browser(params);
need context here

https://codereview.chromium.org/11348344/diff/1014/chrome/browser/ui/extensio...
chrome/browser/ui/extensions/application_launch.cc:152: browser = new
Browser(Browser::CreateParams(profile));
need context here, too

[robertshield, 2012-12-03 20:25:58]

Thanks, PTAL and let's chat about the downloads stuff.

https://codereview.chromium.org/11348344/diff/1014/chrome/browser/extensions/...
File chrome/browser/extensions/api/tabs/tabs.cc (right):

https://codereview.chromium.org/11348344/diff/1014/chrome/browser/extensions/...
chrome/browser/extensions/api/tabs/tabs.cc:244: new_window = new
Browser(params);
On 2012/12/03 15:54:39, grt wrote:
> specify the same host desktop type here as that used just above.  (i don't
> recall seeing calls to "new Browser" on the cleanup list.  if they're not
there,
> they should be, no?)

ah, good point. We should definitely audit the "new Browser" calls as well.

In this particular case, |params| are being passed in to this function. I'm not
sure that mutating them here to set the host desktop type is the right thing to
do. I think rather I should be using the host desktop type in the given in
|params|. What say you?

https://codereview.chromium.org/11348344/diff/1014/chrome/browser/extensions/...
chrome/browser/extensions/api/tabs/tabs.cc:998: // Ensure the selected browser
is tabbed.
On 2012/12/03 15:54:39, grt wrote:
> i haven't studied when this is called, but since |browser| isn't NULL, does it
> make sense to use the same desktop type as |browser| here?

You're right, I had missed that this call site would only be reached if we had
already found a browser.

https://codereview.chromium.org/11348344/diff/1014/chrome/browser/extensions/...
chrome/browser/extensions/api/tabs/tabs.cc:1068: chrome::GetActiveDesktop());
On 2012/12/03 15:54:39, grt wrote:
> along similar lines, why not use the same host desktop type as |browser|?

Done.

https://codereview.chromium.org/11348344/diff/1014/chrome/browser/extensions/...
chrome/browser/extensions/api/tabs/tabs.cc:1070: browser = new
Browser(Browser::CreateParams(profile));
On 2012/12/03 15:54:39, grt wrote:
> specify the same host desktop type here as that used just above.

Done.

https://codereview.chromium.org/11348344/diff/1014/chrome/browser/extensions/...
File chrome/browser/extensions/extension_install_ui_default.cc (right):

https://codereview.chromium.org/11348344/diff/1014/chrome/browser/extensions/...
chrome/browser/extensions/extension_install_ui_default.cc:189:
chrome::GetActiveDesktop());
On 2012/12/03 15:54:39, grt wrote:
> if the user clicked on a link on the web store and then switched desktops,
> should the infobar appear in the originating desktop? is it possible to know
the
> context at the time that the theme (extension) was installed?

Sometimes. This is called from OnInstallSuccess, which is called in two places.
One of these places is CrxInstaller which is used in a couple of different
places, only some of which have context.

The main place that doesn't have context is in ChromeDownloadManagerDelegate
which is where my search stopped when I made this change. 

The best way to do this is probably to plumb context through the downloads
system so I could revert this line change and do that first. What say you?

https://codereview.chromium.org/11348344/diff/1014/chrome/browser/extensions/...
File chrome/browser/extensions/extension_tab_util.cc (right):

https://codereview.chromium.org/11348344/diff/1014/chrome/browser/extensions/...
chrome/browser/extensions/extension_tab_util.cc:234: Browser* browser =
browser::FindTabbedBrowser(profile,
On 2012/12/03 15:54:39, grt wrote:
> ExtensionHost::AddNewContents has a |source| WebContents. Is that meaningful?
Is
> it possible to know what desktop that's on?

Not 100%. A WebContents doesn't have a desktop context, though it may be
associated with a Browser that does.

There's a FindBrowserWithWebContents function in browser_finder.h, but the rub
is that a WebContents won't always be associated with a Browser (e.g. background
extensions). In those cases, FindBrowserWithWebContents will return NULL.

https://codereview.chromium.org/11348344/diff/1014/chrome/browser/ui/extensio...
File chrome/browser/ui/extensions/application_launch.cc (right):

https://codereview.chromium.org/11348344/diff/1014/chrome/browser/ui/extensio...
chrome/browser/ui/extensions/application_launch.cc:124: browser = new
Browser(params);
On 2012/12/03 15:54:39, grt wrote:
> need context here

Done.

https://codereview.chromium.org/11348344/diff/1014/chrome/browser/ui/extensio...
chrome/browser/ui/extensions/application_launch.cc:152: browser = new
Browser(Browser::CreateParams(profile));
On 2012/12/03 15:54:39, grt wrote:
> need context here, too

Done.

[grt (UTC plus 2), 2012-12-03 21:09:04]

lgtm

[robertshield, 2012-12-03 21:11:05]

Ben, please could you give OWNERS blessing?

[Ben Goodger (Google), 2012-12-04 16:23:15]

lgtm

[commit-bot: I haz the power, 2012-12-04 19:03:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/robertshield@chromium.org/11348344/7002

[commit-bot: I haz the power, 2012-12-04 21:51:26]

Change committed as 171057