Give access to browsers by Profile/HostDesktopType.

Give access to browsers by Profile/HostDesktopType.

And fix existing callers of FindBrowserForProfile.

BUG=None
TEST=Make sure that the page cyclers still work (they're one of the users of the API), as well as the download page gets opened properly on the Mac when the user chooses to wait (the other user of the API).

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

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

[robertshield, 2012-08-16 21:05:16]

looking good

https://chromiumcodereview.appspot.com/10855183/diff/1009/chrome/browser/ui/b...
File chrome/browser/ui/browser_finder.cc (right):

https://chromiumcodereview.appspot.com/10855183/diff/1009/chrome/browser/ui/b...
chrome/browser/ui/browser_finder.cc:148: browser = new
Browser(Browser::CreateParams(Browser::TYPE_TABBED, profile));
why do we add TYPE_TABBED here?

https://chromiumcodereview.appspot.com/10855183/diff/1009/chrome/browser/ui/b...
File chrome/browser/ui/browser_finder.h (right):

https://chromiumcodereview.appspot.com/10855183/diff/1009/chrome/browser/ui/b...
chrome/browser/ui/browser_finder.h:63: chrome::HostDesktopType type);
this looks duplicated from line 55

[MAD, 2012-08-17 12:45:50]

Thanks!

BYE
MAD

http://codereview.chromium.org/10855183/diff/1009/chrome/browser/ui/browser_f...
File chrome/browser/ui/browser_finder.cc (right):

http://codereview.chromium.org/10855183/diff/1009/chrome/browser/ui/browser_f...
chrome/browser/ui/browser_finder.cc:148: browser = new
Browser(Browser::CreateParams(Browser::TYPE_TABBED, profile));
On 2012/08/16 21:05:16, robertshield wrote:
> why do we add TYPE_TABBED here?

Because I wanted to be more explicit that it's the same as below... TYPE_TABBED
is the default value when you only pass in a profile... But we don't have one
where TYPE_TABBED is the default and we receive a desktop type...

OK?

http://codereview.chromium.org/10855183/diff/1009/chrome/browser/ui/browser_f...
File chrome/browser/ui/browser_finder.h (right):

http://codereview.chromium.org/10855183/diff/1009/chrome/browser/ui/browser_f...
chrome/browser/ui/browser_finder.h:63: chrome::HostDesktopType type);
On 2012/08/16 21:05:16, robertshield wrote:
> this looks duplicated from line 55
Oupsss... :-)
Done.

[robertshield, 2012-08-17 13:25:02]

lgtm

[MAD, 2012-08-17 13:38:48]

Salut Ben,

   is this the direction you wanted to take with the browser finder?

Thanks!

BYE
MAD...

[MAD, 2012-08-20 14:58:44]

Ping?

I have a few CLs depending on this one. Some Windows 8 patches that need to look
for a browser. And then I'll also use this to fix more issues in John's
spreadsheet.

So please let me know if you would prefer doing this some other way...

Thanks!

BYE
MAD...

[Ben Goodger (Google), 2012-08-20 20:09:40]

Was OOO friday.

http://codereview.chromium.org/10855183/diff/5001/chrome/browser/ui/browser_f...
File chrome/browser/ui/browser_finder.h (right):

http://codereview.chromium.org/10855183/diff/5001/chrome/browser/ui/browser_f...
chrome/browser/ui/browser_finder.h:30: Browser*
FindDesktopTabbedBrowser(Profile* profile,
question I have about all of these are...

are they all needed? or can we get by with fewer of these methods servicing only
the few special cases that really need them?

http://codereview.chromium.org/10855183/diff/5001/chrome/browser/ui/browser_f...
chrome/browser/ui/browser_finder.h:39: Browser*
FindOrCreateDesktopTabbedBrowser(Profile* profile,
also I prefer the original names, and to retire the methods that don't take a
HDT.

[MAD, 2012-08-20 20:28:15]

OK thanks!

I'll try to minimize the change, and keep the old names...

As for removing the exiting ones without a HDT, we need to fix all callers
first...

I was trying to stage this into smaller CLs, but I could also try to go
vertically instead of horizontally, i.e., take one API, add the HDT and
change all the callers at once, and do one CL per API, as opposed to change
all the APIs, and then gradually go after all callers, and once we cover
them all, remove the old HDT-less APIs... I prefer the horizontal approach
(change all APIs and then gradually change all callers), but I can see how
big the vertical CLs for each API would be...

On Mon, Aug 20, 2012 at 4:09 PM, <ben@chromium.org> wrote:

> Was OOO friday.
>
>
> http://codereview.chromium.**org/10855183/diff/5001/chrome/**
>
browser/ui/browser_finder.h<http://codereview.chromium.org/10855183/diff/5001/chrome/browser/ui/browser_finder.h>
> File chrome/browser/ui/browser_**finder.h (right):
>
> http://codereview.chromium.**org/10855183/diff/5001/chrome/**
>
browser/ui/browser_finder.h#**newcode30<http://codereview.chromium.org/10855183/diff/5001/chrome/browser/ui/browser_finder.h#newcode30>
> chrome/browser/ui/browser_**finder.h:30: Browser*
> FindDesktopTabbedBrowser(**Profile* profile,
> question I have about all of these are...
>
> are they all needed? or can we get by with fewer of these methods
> servicing only the few special cases that really need them?
>
> http://codereview.chromium.**org/10855183/diff/5001/chrome/**
>
browser/ui/browser_finder.h#**newcode39<http://codereview.chromium.org/10855183/diff/5001/chrome/browser/ui/browser_finder.h#newcode39>
> chrome/browser/ui/browser_**finder.h:39: Browser*
> FindOrCreateDesktopTabbedBrows**er(Profile* profile,
> also I prefer the original names, and to retire the methods that don't
> take a HDT.
>
>
http://codereview.chromium.**org/10855183/<http://codereview.chromium.org/108...
>

[MAD, 2012-08-21 14:34:26]

Would this be OK then?

Thanks!

BYE
MAD...

[Ben Goodger (Google), 2012-08-21 21:47:20]

lgtm

https://chromiumcodereview.appspot.com/10855183/diff/10006/chrome/browser/ui/...
File chrome/browser/ui/browser_finder.cc (right):

https://chromiumcodereview.appspot.com/10855183/diff/10006/chrome/browser/ui/...
chrome/browser/ui/browser_finder.cc:23: chrome::HostDesktopType
kDefaultHostDesktopType = chrome::HOST_DESKTOP_TYPE_ASH;
my theory is that this will have to go in host_desktop_type.h

maybe not yet.

[MAD, 2012-08-21 21:50:08]

OK, thanks, I'll add a "TODO(mad) eventually move to  host_desktop_type.h."
comment...

On Tue, Aug 21, 2012 at 5:47 PM, <ben@chromium.org> wrote:

> lgtm
>
>
>
>
> https://chromiumcodereview.**appspot.com/10855183/diff/**
>
10006/chrome/browser/ui/**browser_finder.cc<https://chromiumcodereview.appspot.com/10855183/diff/10006/chrome/browser/ui/browser_finder.cc>
> File chrome/browser/ui/browser_**finder.cc (right):
>
> https://chromiumcodereview.**appspot.com/10855183/diff/**
>
10006/chrome/browser/ui/**browser_finder.cc#newcode23<https://chromiumcodereview.appspot.com/10855183/diff/10006/chrome/browser/ui/browser_finder.cc#newcode23>
> chrome/browser/ui/browser_**finder.cc:23: chrome::HostDesktopType
> kDefaultHostDesktopType = chrome::HOST_DESKTOP_TYPE_ASH;
> my theory is that this will have to go in host_desktop_type.h
>
> maybe not yet.
>
>
https://chromiumcodereview.**appspot.com/10855183/<https://chromiumcodereview...
>

[commit-bot: I haz the power, 2012-08-21 21:55:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mad@chromium.org/10855183/8003

[commit-bot: I haz the power, 2012-08-22 01:23:20]

Try job failure for 10855183-8003 (retry) on linux_chromeos for steps
"browser_tests, unit_tests".
It's a second try, previously, steps "browser_tests, unit_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2012-08-22 01:32:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mad@chromium.org/10855183/8003

[commit-bot: I haz the power, 2012-08-22 03:33:06]

Try job failure for 10855183-8003 (retry) on win for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

[commit-bot: I haz the power, 2012-08-22 11:35:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mad@chromium.org/10855183/8003

[commit-bot: I haz the power, 2012-08-22 12:54:54]

Try job failure for 10855183-8003 (retry) on win for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

[commit-bot: I haz the power, 2012-08-22 13:44:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mad@chromium.org/10855183/8003

[commit-bot: I haz the power, 2012-08-22 15:10:39]

Try job failure for 10855183-8003 (retry) on win for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

[commit-bot: I haz the power, 2012-08-22 15:29:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mad@chromium.org/10855183/8003

[commit-bot: I haz the power, 2012-08-22 16:57:04]

Try job failure for 10855183-8003 (retry) on win for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

[MAD, 2012-08-23 12:46:52]

Hey Ben! Let me know if this is what you meant.
Now on OS_CHROMEOS, the Ash HDT is set to Native.

Thanks!

BYE
MAD...

[commit-bot: I haz the power, 2012-08-24 12:23:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mad@chromium.org/10855183/17005

[commit-bot: I haz the power, 2012-08-24 14:45:12]

Change committed as 153207