Don't allow profile deletion in Metro mode.

Don't allow profile deletion in Metro mode.

Bad things happen if you delete the profile that started the Metro
process (at best, you crash), since relaunching Metro with a new user
is not supported. Hiding the button selectively depending on the
profile selected is a bit weird, so in Metro mode, hide it for all
profiles.

BUG=448352
Committed: https://crrev.com/9a4fcc927bf745d6aab08440e22344b393246492
Cr-Commit-Position: refs/heads/master@{#313449}

[noms (inactive), 2015-01-23 19:29:22]

Patchset #1 (id:1) has been deleted

[noms (inactive), 2015-01-23 19:29:31]

Patchset #1 (id:20001) has been deleted

[noms (inactive), 2015-01-23 21:12:34]

noms@chromium.org changed reviewers:
+ tapted@chromium.org

[noms (inactive), 2015-01-23 21:12:35]

Hi Trent,

Before I send it out for owners review, does this look okay from the Metro mode
side?

[noms (inactive), 2015-01-26 15:19:20]

noms@chromium.org changed reviewers:
+ estade@chromium.org

[noms (inactive), 2015-01-26 15:19:21]

Hi Evan,

This CL disables profile deletion in Win 8 metro mode. Please take a look.
Thanks!

[Evan Stade, 2015-01-26 17:55:26]

https://codereview.chromium.org/863063002/diff/60001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/options/browser_options_handler.cc (right):

https://codereview.chromium.org/863063002/diff/60001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/browser_options_handler.cc:613:
values->SetBoolean("allowProfileDeletion", !base::win::IsMetroProcess());
don't you need some ifdefs

[noms (inactive), 2015-01-26 22:28:00]

Patchset #3 (id:80001) has been deleted

[noms (inactive), 2015-01-26 22:30:10]

Patchset #3 (id:100001) has been deleted

[noms (inactive), 2015-01-26 23:08:14]

https://codereview.chromium.org/863063002/diff/60001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/options/browser_options_handler.cc (right):

https://codereview.chromium.org/863063002/diff/60001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/browser_options_handler.cc:613:
values->SetBoolean("allowProfileDeletion", !base::win::IsMetroProcess());
On 2015/01/26 17:55:26, Evan Stade wrote:
> don't you need some ifdefs

Gah, sorry about that. Dropped that ball. Done.

[tapted, 2015-01-26 23:36:07]

Sorry for the delay! (long weekend over here..).

https://codereview.chromium.org/863063002/diff/60001/chrome/browser/resources...
File chrome/browser/resources/options/browser_options.js (right):

https://codereview.chromium.org/863063002/diff/60001/chrome/browser/resources...
chrome/browser/resources/options/browser_options.js:327:
profilesList.canDeleteItems = true;
I'm not a webui guru, but this is probably unnecessary, since it's done by an
~initializer in browser_options_profile_list.js  (and if it _was_ necessary the
profileIsSupervised clause would be broken :).

https://codereview.chromium.org/863063002/diff/60001/chrome/browser/resources...
chrome/browser/resources/options/browser_options.js:1420:
$('profiles-delete').disabled = !profilesList.canDeleteItems_ ||
Does this need an accessor (i.e. trailing underscore => "private")? (It's also
@private, and I think dbeam's working on running all this through closure)

https://codereview.chromium.org/863063002/diff/60001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/options/browser_options_handler.cc (right):

https://codereview.chromium.org/863063002/diff/60001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/browser_options_handler.cc:613:
values->SetBoolean("allowProfileDeletion", !base::win::IsMetroProcess());
I'm pretty sure base::win::IsMetroProcess() only returns true for the viewer
process, not the browser process. aura::RemoteWindowTreeHostWin::IsValid() is
probably the right way to this. Or ash::Shell::HasInstance(), since you can't
delete profiles this way in ChromeOS either.

ash::Shell::HasInstance() has a nice ring to it, since the comment above could
say something like. ~"If the ash desktop is instantiated (e.g. Windows Metro
Mode), the user might delete the Profile that it's using, which is not
supported."

(note the settings webui can still show in Desktop mode while the Chrome browser
is in Metro mode, so we wouldn't want that allowing profile deletion either).

There are still a disturbing number of callers of IsMetroProcess in c/b/ui
though. It's likely they were missed in the great win-aura purges. Probably
there should be a DEPS rule for metro.h to prevent non-viewer-process code using
it :/

https://codereview.chromium.org/863063002/diff/60001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/signin/user_manager_screen_handler.cc (right):

https://codereview.chromium.org/863063002/diff/60001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/signin/user_manager_screen_handler.cc:638: bool
can_remove = !base::win::IsMetroProcess();
here too, but this might be shared with ChromeOS, so perhaps guard
ash::Shell::HasInstance() differently (or use viewerprocess:IsValid

[Evan Stade, 2015-01-27 00:06:58]

https://codereview.chromium.org/863063002/diff/120001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options/browser_options_handler.cc (right):

https://codereview.chromium.org/863063002/diff/120001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/browser_options_handler.cc:615: allow_deletion =
!base::win::IsMetroProcess();
you don't care about isSupervised any more?

[noms (inactive), 2015-01-27 00:36:10]

https://codereview.chromium.org/863063002/diff/120001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options/browser_options_handler.cc (right):

https://codereview.chromium.org/863063002/diff/120001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/browser_options_handler.cc:615: allow_deletion =
!base::win::IsMetroProcess();
On 2015/01/27 00:06:58, Evan Stade wrote:
> you don't care about isSupervised any more?

This is more like an extra check on top of isSupervised (which works in the same
way in browser_options.js) Basically, even if all the rules let you delete a
profile, if you're in Metro mode, you don't get to.

Would it be better if I renamed the variable to a metro-specific name?

[Evan Stade, 2015-01-27 01:59:36]

https://codereview.chromium.org/863063002/diff/120001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options/browser_options_handler.cc (right):

https://codereview.chromium.org/863063002/diff/120001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/browser_options_handler.cc:615: allow_deletion =
!base::win::IsMetroProcess();
On 2015/01/27 00:36:10, Monica Dinculescu wrote:
> On 2015/01/27 00:06:58, Evan Stade wrote:
> > you don't care about isSupervised any more?
> 
> This is more like an extra check on top of isSupervised (which works in the
same
> way in browser_options.js) Basically, even if all the rules let you delete a
> profile, if you're in Metro mode, you don't get to.
> 
> Would it be better if I renamed the variable to a metro-specific name?

I would rather the logic all be in one place.

  bool allow_deletion = !Profile::FromWebUI(web_ui())->IsSupervised();
#if defined(OS_WIN)
  // also check for metro
#endif

[noms (inactive), 2015-01-28 02:20:14]

https://codereview.chromium.org/863063002/diff/60001/chrome/browser/resources...
File chrome/browser/resources/options/browser_options.js (right):

https://codereview.chromium.org/863063002/diff/60001/chrome/browser/resources...
chrome/browser/resources/options/browser_options.js:327:
profilesList.canDeleteItems = true;
On 2015/01/26 23:36:07, tapted wrote:
> I'm not a webui guru, but this is probably unnecessary, since it's done by an
> ~initializer in browser_options_profile_list.js  (and if it _was_ necessary
the
> profileIsSupervised clause would be broken :).

Done.

https://codereview.chromium.org/863063002/diff/60001/chrome/browser/resources...
chrome/browser/resources/options/browser_options.js:1420:
$('profiles-delete').disabled = !profilesList.canDeleteItems_ ||
On 2015/01/26 23:36:07, tapted wrote:
> Does this need an accessor (i.e. trailing underscore => "private")? (It's also
> @private, and I think dbeam's working on running all this through closure)

Done.

https://codereview.chromium.org/863063002/diff/60001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/options/browser_options_handler.cc (right):

https://codereview.chromium.org/863063002/diff/60001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/browser_options_handler.cc:613:
values->SetBoolean("allowProfileDeletion", !base::win::IsMetroProcess());
On 2015/01/26 23:36:07, tapted wrote:
> I'm pretty sure base::win::IsMetroProcess() only returns true for the viewer
> process, not the browser process. aura::RemoteWindowTreeHostWin::IsValid() is
> probably the right way to this. Or ash::Shell::HasInstance(), since you can't
> delete profiles this way in ChromeOS either.
> 
> ash::Shell::HasInstance() has a nice ring to it, since the comment above could
> say something like. ~"If the ash desktop is instantiated (e.g. Windows Metro
> Mode), the user might delete the Profile that it's using, which is not
> supported."
> 
> (note the settings webui can still show in Desktop mode while the Chrome
browser
> is in Metro mode, so we wouldn't want that allowing profile deletion either).
> 
> There are still a disturbing number of callers of IsMetroProcess in c/b/ui
> though. It's likely they were missed in the great win-aura purges. Probably
> there should be a DEPS rule for metro.h to prevent non-viewer-process code
using
> it :/

Sigh. It had such a promising name. I changed it to the one that actually checks
the right thing. Thanks!

https://codereview.chromium.org/863063002/diff/60001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/signin/user_manager_screen_handler.cc (right):

https://codereview.chromium.org/863063002/diff/60001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/signin/user_manager_screen_handler.cc:638: bool
can_remove = !base::win::IsMetroProcess();
On 2015/01/26 23:36:07, tapted wrote:
> here too, but this might be shared with ChromeOS, so perhaps guard
> ash::Shell::HasInstance() differently (or use viewerprocess:IsValid

This is not shared with ChromeOS. This is desktop-only. Done.

https://codereview.chromium.org/863063002/diff/120001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options/browser_options_handler.cc (right):

https://codereview.chromium.org/863063002/diff/120001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/browser_options_handler.cc:615: allow_deletion =
!base::win::IsMetroProcess();
On 2015/01/27 01:59:36, Evan Stade wrote:
> On 2015/01/27 00:36:10, Monica Dinculescu wrote:
> > On 2015/01/27 00:06:58, Evan Stade wrote:
> > > you don't care about isSupervised any more?
> > 
> > This is more like an extra check on top of isSupervised (which works in the
> same
> > way in browser_options.js) Basically, even if all the rules let you delete a
> > profile, if you're in Metro mode, you don't get to.
> > 
> > Would it be better if I renamed the variable to a metro-specific name?
> 
> I would rather the logic all be in one place.
> 
>   bool allow_deletion = !Profile::FromWebUI(web_ui())->IsSupervised();
> #if defined(OS_WIN)
>   // also check for metro
> #endif

Done.

[Evan Stade, 2015-01-28 02:23:00]

lgtm

[tapted, 2015-01-28 02:28:27]

lgtm

[noms (inactive), 2015-01-28 03:23:28]

Woohoo! Thanks!

[noms (inactive), 2015-01-28 03:23:31]

Woohoo! Thanks!

[noms (inactive), 2015-01-28 03:23:37]

The CQ bit was checked by noms@chromium.org

[commit-bot: I haz the power, 2015-01-28 03:24:49]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/863063002/160001

[commit-bot: I haz the power, 2015-01-28 04:57:31]

Committed patchset #5 (id:160001)

[commit-bot: I haz the power, 2015-01-28 04:58:33]

Patchset 5 (id:??) landed as
https://crrev.com/9a4fcc927bf745d6aab08440e22344b393246492
Cr-Commit-Position: refs/heads/master@{#313449}