Add PerAppSettingsService and use it to remember what desktop apps should create windows on.

Add PerAppSettingsService and use it to remember what desktop apps should create windows on.

BUG=316062
TBR=ben@chromium.org

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

[benwells, 2013-12-11 05:27:39]

lgtm with some nits

https://codereview.chromium.org/111553008/diff/20001/chrome/browser/apps/per_...
File chrome/browser/apps/per_app_settings_service.cc (right):

https://codereview.chromium.org/111553008/diff/20001/chrome/browser/apps/per_...
chrome/browser/apps/per_app_settings_service.cc:22: return
chrome::HOST_DESKTOP_TYPE_NATIVE;
Is it valid for this to happen?

https://codereview.chromium.org/111553008/diff/20001/chrome/browser/apps/per_...
File chrome/browser/apps/per_app_settings_service.h (right):

https://codereview.chromium.org/111553008/diff/20001/chrome/browser/apps/per_...
chrome/browser/apps/per_app_settings_service.h:23: void SetDefaultDesktop(
The name 'default' feels strange here. Isn't it more the current desktop?
SetCurrentDesktop would make more sense to me...

https://codereview.chromium.org/111553008/diff/20001/chrome/browser/apps/per_...
File chrome/browser/apps/per_app_settings_service_factory.cc (right):

https://codereview.chromium.org/111553008/diff/20001/chrome/browser/apps/per_...
chrome/browser/apps/per_app_settings_service_factory.cc:42: return false;
nit: this doesn't do anything as ServiceIsCreatedWithBrowserContext is false.

https://codereview.chromium.org/111553008/diff/20001/chrome/browser/apps/per_...
File chrome/browser/apps/per_app_settings_service_factory.h (right):

https://codereview.chromium.org/111553008/diff/20001/chrome/browser/apps/per_...
chrome/browser/apps/per_app_settings_service_factory.h:14: class
PerAppSettingsService;
nit: blank line above.

[tapted, 2013-12-11 06:30:10]

lgtm if you agree with my suggestion for startup_browser_creator_impl.cc &&
GetActiveDesktop in PerAppSettingsService::GetFooDesktop(..). Otherwise mostly
nits.

https://codereview.chromium.org/111553008/diff/20001/chrome/browser/apps/per_...
File chrome/browser/apps/per_app_settings_service.cc (right):

https://codereview.chromium.org/111553008/diff/20001/chrome/browser/apps/per_...
chrome/browser/apps/per_app_settings_service.cc:14: const std::string&
extension_id, chrome::HostDesktopType host_desktop) {
nit: extension_id -> app_id, like the header?

https://codereview.chromium.org/111553008/diff/20001/chrome/browser/apps/per_...
chrome/browser/apps/per_app_settings_service.cc:22: return
chrome::HOST_DESKTOP_TYPE_NATIVE;
On 2013/12/11 05:27:39, benwells wrote:
> Is it valid for this to happen?

I think an App can do things like open a window in a chrome alarm, or registered
in an OnStartup event, and we need a fallback case for that.

For those - I'd probably reluctantly go for chrome::GetActiveDesktop() rather
than native. Note that desktop shortcuts should still open "always" on the
native desktop and for that, I think we need to add `params.desktop_type =
chrome::DESKTOP_TYPE_NATIVE` in startup_browser_creator_impl.cc:376, where it's
checking switches::kAppId. Otherwise, I link that code can still select the
wrong desktop type there, and set the wrong one when the app gets launched.
#yaymetro

https://codereview.chromium.org/111553008/diff/20001/chrome/browser/apps/per_...
File chrome/browser/apps/per_app_settings_service.h (right):

https://codereview.chromium.org/111553008/diff/20001/chrome/browser/apps/per_...
chrome/browser/apps/per_app_settings_service.h:23: void SetDefaultDesktop(
On 2013/12/11 05:27:39, benwells wrote:
> The name 'default' feels strange here. Isn't it more the current desktop?
> SetCurrentDesktop would make more sense to me...

perhaps `SetInitialDesktop`, since subsequently opened windows will not change
the desktop if an app already has currently open windows (even if going through
an OnLaunched).

https://codereview.chromium.org/111553008/diff/20001/chrome/browser/apps/per_...
chrome/browser/apps/per_app_settings_service.h:25: chrome::HostDesktopType
GetDefaultDesktop(const std::string& app_id);
nit: declare function const?

https://codereview.chromium.org/111553008/diff/20001/chrome/browser/apps/per_...
chrome/browser/apps/per_app_settings_service.h:30: };
nit: DISALLOW_COPY_AND_ASSIGN?

https://codereview.chromium.org/111553008/diff/20001/chrome/browser/apps/per_...
File chrome/browser/apps/per_app_settings_service_factory.cc (right):

https://codereview.chromium.org/111553008/diff/20001/chrome/browser/apps/per_...
chrome/browser/apps/per_app_settings_service_factory.cc:24:
PerAppSettingsService* PerAppSettingsServiceFactory::GetForBrowserContext(
nit: move up with the other static function?

https://codereview.chromium.org/111553008/diff/20001/chrome/browser/apps/per_...
File chrome/browser/apps/per_app_settings_service_factory.h (right):

https://codereview.chromium.org/111553008/diff/20001/chrome/browser/apps/per_...
chrome/browser/apps/per_app_settings_service_factory.h:8: #include
"base/memory/singleton.h"
nit: I typically see things forward-declaring singleton, like

template <typename T> struct DefaultSingletonTraits;

see comments at ~singleton.h:149

https://codereview.chromium.org/111553008/diff/20001/chrome/browser/apps/per_...
chrome/browser/apps/per_app_settings_service_factory.h:18: static
PerAppSettingsServiceFactory* GetInstance();
nit: maybe make this private?

[koz (OOO until 15th September), 2013-12-11 23:26:43]

https://codereview.chromium.org/111553008/diff/20001/chrome/browser/apps/per_...
File chrome/browser/apps/per_app_settings_service.cc (right):

https://codereview.chromium.org/111553008/diff/20001/chrome/browser/apps/per_...
chrome/browser/apps/per_app_settings_service.cc:14: const std::string&
extension_id, chrome::HostDesktopType host_desktop) {
On 2013/12/11 06:30:10, tapted wrote:
> nit: extension_id -> app_id, like the header?

Done.

https://codereview.chromium.org/111553008/diff/20001/chrome/browser/apps/per_...
chrome/browser/apps/per_app_settings_service.cc:22: return
chrome::HOST_DESKTOP_TYPE_NATIVE;
On 2013/12/11 06:30:10, tapted wrote:
> On 2013/12/11 05:27:39, benwells wrote:
> > Is it valid for this to happen?
> 
> I think an App can do things like open a window in a chrome alarm, or
registered
> in an OnStartup event, and we need a fallback case for that.
> 
> For those - I'd probably reluctantly go for chrome::GetActiveDesktop() rather
> than native. Note that desktop shortcuts should still open "always" on the
> native desktop and for that, I think we need to add `params.desktop_type =
> chrome::DESKTOP_TYPE_NATIVE` in startup_browser_creator_impl.cc:376, where
it's
> checking switches::kAppId. Otherwise, I link that code can still select the
> wrong desktop type there, and set the wrong one when the app gets launched.
> #yaymetro

In that case, I think it'd be better to have the caller decide on that policy
rather than doing it here. I've moved that logic to the call site.

And you're right about the startup_browser_creator_impl.cc bit, I've added that,
too.

https://codereview.chromium.org/111553008/diff/20001/chrome/browser/apps/per_...
File chrome/browser/apps/per_app_settings_service.h (right):

https://codereview.chromium.org/111553008/diff/20001/chrome/browser/apps/per_...
chrome/browser/apps/per_app_settings_service.h:23: void SetDefaultDesktop(
On 2013/12/11 06:30:10, tapted wrote:
> On 2013/12/11 05:27:39, benwells wrote:
> > The name 'default' feels strange here. Isn't it more the current desktop?
> > SetCurrentDesktop would make more sense to me...
> 
> perhaps `SetInitialDesktop`, since subsequently opened windows will not change
> the desktop if an app already has currently open windows (even if going
through
> an OnLaunched).

I used the word default because if there's a better reason to choose another
desktop then we'll do that - the desktop specified here is only the backup, ie:
the default.

However, I've renamed this to be more explicit - it's the last desktop that an
app was launched from. I think this makes it easier to understand what it means,
and makes the call sites easier to read.

https://codereview.chromium.org/111553008/diff/20001/chrome/browser/apps/per_...
chrome/browser/apps/per_app_settings_service.h:25: chrome::HostDesktopType
GetDefaultDesktop(const std::string& app_id);
On 2013/12/11 06:30:10, tapted wrote:
> nit: declare function const?

Done.

https://codereview.chromium.org/111553008/diff/20001/chrome/browser/apps/per_...
chrome/browser/apps/per_app_settings_service.h:30: };
On 2013/12/11 06:30:10, tapted wrote:
> nit: DISALLOW_COPY_AND_ASSIGN?

Done.

https://codereview.chromium.org/111553008/diff/20001/chrome/browser/apps/per_...
File chrome/browser/apps/per_app_settings_service_factory.cc (right):

https://codereview.chromium.org/111553008/diff/20001/chrome/browser/apps/per_...
chrome/browser/apps/per_app_settings_service_factory.cc:24:
PerAppSettingsService* PerAppSettingsServiceFactory::GetForBrowserContext(
On 2013/12/11 06:30:10, tapted wrote:
> nit: move up with the other static function?

Done.

https://codereview.chromium.org/111553008/diff/20001/chrome/browser/apps/per_...
chrome/browser/apps/per_app_settings_service_factory.cc:42: return false;
On 2013/12/11 05:27:39, benwells wrote:
> nit: this doesn't do anything as ServiceIsCreatedWithBrowserContext is false.

Done.

https://codereview.chromium.org/111553008/diff/20001/chrome/browser/apps/per_...
File chrome/browser/apps/per_app_settings_service_factory.h (right):

https://codereview.chromium.org/111553008/diff/20001/chrome/browser/apps/per_...
chrome/browser/apps/per_app_settings_service_factory.h:8: #include
"base/memory/singleton.h"
On 2013/12/11 06:30:10, tapted wrote:
> nit: I typically see things forward-declaring singleton, like
> 
> template <typename T> struct DefaultSingletonTraits;
> 
> see comments at ~singleton.h:149

Done.

https://codereview.chromium.org/111553008/diff/20001/chrome/browser/apps/per_...
chrome/browser/apps/per_app_settings_service_factory.h:14: class
PerAppSettingsService;
On 2013/12/11 05:27:39, benwells wrote:
> nit: blank line above.

Done.

https://codereview.chromium.org/111553008/diff/20001/chrome/browser/apps/per_...
chrome/browser/apps/per_app_settings_service_factory.h:18: static
PerAppSettingsServiceFactory* GetInstance();
On 2013/12/11 06:30:10, tapted wrote:
> nit: maybe make this private?

The example in singleton.h has this public, and it means less friend stuff
(simply making it private causes compile errors).

[koz (OOO until 15th September), 2013-12-11 23:27:31]

+ben for startup_browser_creator stuff

[benwells, 2013-12-13 01:39:09]

On 2013/12/11 23:27:31, koz wrote:
> +ben for startup_browser_creator stuff

btw I'm pretty sure the doc we did for win/ash and apps specifies which desktop
apps launched from events should be shown on. I can't remember what it was
though.

[koz (OOO until 15th September), 2013-12-13 04:15:56]

On 2013/12/13 01:39:09, benwells wrote:
> On 2013/12/11 23:27:31, koz wrote:
> > +ben for startup_browser_creator stuff
> 
> btw I'm pretty sure the doc we did for win/ash and apps specifies which
desktop
> apps launched from events should be shown on. I can't remember what it was
> though.

The doc says to run in the environment that Chrome runs in, which you point out
in comments is probably equivalent to the last active desktop (ie: what's
implemented here).

[koz (OOO until 15th September), 2013-12-16 23:05:51]

TBRing ben@, I think the startup_browser_creator_impl.cc change is pretty
trivial.

[commit-bot: I haz the power, 2013-12-16 23:13:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/koz@chromium.org/111553008/60001

[commit-bot: I haz the power, 2013-12-17 04:21:03]

Retried try job too often on linux_chromeos for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2013-12-17 04:38:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/koz@chromium.org/111553008/60001

[commit-bot: I haz the power, 2013-12-17 07:17:44]

Retried try job too often on linux_chromeos for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2013-12-17 07:29:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/koz@chromium.org/111553008/60001

[commit-bot: I haz the power, 2013-12-17 10:41:39]

Retried try job too often on linux_chromeos for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2013-12-17 23:03:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/koz@chromium.org/111553008/60001

[commit-bot: I haz the power, 2013-12-18 02:35:00]

Retried try job too often on linux_chromeos for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2013-12-18 02:50:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/koz@chromium.org/111553008/60001

[commit-bot: I haz the power, 2013-12-18 05:45:31]

Retried try job too often on linux_chromeos for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2013-12-18 05:47:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/koz@chromium.org/111553008/60001

[commit-bot: I haz the power, 2013-12-18 09:33:48]

Retried try job too often on linux_chromeos for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2013-12-18 23:21:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/koz@chromium.org/111553008/60001

[commit-bot: I haz the power, 2013-12-19 00:25:15]

Retried try job too often on win_rel for step(s) chrome_elf_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-12-19 04:41:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/koz@chromium.org/111553008/80001

[commit-bot: I haz the power, 2013-12-19 06:12:39]

Retried try job too often on linux_chromeos for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2013-12-19 23:24:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/koz@chromium.org/111553008/100001

[commit-bot: I haz the power, 2013-12-20 00:53:36]

Retried try job too often on win_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2014-01-07 09:54:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/koz@chromium.org/111553008/100001

[commit-bot: I haz the power, 2014-01-07 10:50:57]

Retried try job too often on mac_rel for step(s) base_unittests, browser_tests,
interactive_ui_tests, net_unittests, unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2014-01-07 23:22:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/koz@chromium.org/111553008/360001

[commit-bot: I haz the power, 2014-01-08 03:43:46]

Retried try job too often on linux_chromeos for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2014-01-12 23:55:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/koz@chromium.org/111553008/640001

[commit-bot: I haz the power, 2014-01-13 06:58:56]

Change committed as 244466