Fix the System Profile with extensions.

Fix the System Profile to not load extensions. It could otherwise be forced to by GPO.

A few releases ago I introduced a System Profile to back the User Manager (https://codereview.chromium.org/847733005). This profile, however, is causing some problems. I'm trying to reduce those by making clearer the contract of what it's for: A profile that doesn't take a browser window, has no extensions, writes little to disk, etc.

This certainly addresses issue #4 raised within the bug, and address the other issues as well (I can't repro them). This CL generally hardens Chrome for the System Profile in a lot of the same ways we do the Guest Profile.

BUG=482176
TBR=jcivelli@chromium.org (a function, called from chrome/test that lives in c/b/profiles, was renamed).

Committed: https://crrev.com/7831f57d5f56e9cafe77388b20a83a90d6eb7918
Cr-Commit-Position: refs/heads/master@{#331276}

[Mike Lerman, 2015-05-20 18:39:41]

mlerman@chromium.org changed reviewers:
+ msw@chromium.org, rockot@chromium.org

[Mike Lerman, 2015-05-20 18:39:42]

Hi msw@, rockot@,

A few releases ago I introduced a System Profile to back the User Manager
(https://codereview.chromium.org/847733005). This profile, however, is causing
some problems. I'm trying to reduce those by making clearer the contract of what
it's for: A profile that doesn't take a browser window, has no extensions,
writes little to disk, etc.

msw - can you look at c/b/profiles and c/b/ui?
rockot - can you look at c/b/extensions?

Thanks!

[Ken Rockot(use gerrit already), 2015-05-20 21:27:52]

extensions lgtm

I'm wondering how common it will now be to pair the condition "is guest" with
"is system profile" - and is there maybe a conditional abstraction that deserves
its own method on profile (like IsBasicProfile or something?) as a union of the
two conditions. maybe, maybe not

[Mike Lerman, 2015-05-21 00:21:40]

On 2015/05/20 21:27:52, Ken Rockot wrote:
> extensions lgtm
> 
> I'm wondering how common it will now be to pair the condition "is guest" with
> "is system profile" - and is there maybe a conditional abstraction that
deserves
> its own method on profile (like IsBasicProfile or something?) as a union of
the
> two conditions. maybe, maybe not

I thought about making a common method. I'm leery of doing so, because there's
no hard & fast rule about when to pair the two conditions. Having a simple
abstraction might encourage people to use it unnecessarily or unwisely, whereas
specifying both makes people think. At least, that's my reasoning ;)

I'm also following a personal rule of trying not too hard to find patterns or
refactorings when there are only two items, rather than having three yet. Maybe
if we end up with a third type of profile, it will make sense (and be more
obvious how) to make an abstraction.

Although I'm curious for msw@ to weigh in, he may have a different opinion. I'm
also willing to bring in noms@ for a second opinion.

[Ken Rockot(use gerrit already), 2015-05-21 00:24:15]

Cool, I'm not too worried about it either way. Just something I thought
about. My expectation is that you're right and there isn't in fact a useful
joint abstraction for these conditions.

On Wed, May 20, 2015 at 5:21 PM, <mlerman@chromium.org> wrote:

> On 2015/05/20 21:27:52, Ken Rockot wrote:
>
>> extensions lgtm
>>
>
>  I'm wondering how common it will now be to pair the condition "is guest"
>> with
>> "is system profile" - and is there maybe a conditional abstraction that
>>
> deserves
>
>> its own method on profile (like IsBasicProfile or something?) as a union
>> of
>>
> the
>
>> two conditions. maybe, maybe not
>>
>
> I thought about making a common method. I'm leery of doing so, because
> there's
> no hard & fast rule about when to pair the two conditions. Having a simple
> abstraction might encourage people to use it unnecessarily or unwisely,
> whereas
> specifying both makes people think. At least, that's my reasoning ;)
>
> I'm also following a personal rule of trying not too hard to find patterns
> or
> refactorings when there are only two items, rather than having three yet.
> Maybe
> if we end up with a third type of profile, it will make sense (and be more
> obvious how) to make an abstraction.
>
> Although I'm curious for msw@ to weigh in, he may have a different
> opinion. I'm
> also willing to bring in noms@ for a second opinion.
>
> https://codereview.chromium.org/1129293002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[msw, 2015-05-21 04:34:43]

I don't think a helper like IsGuestOrSystemProfile() would be too useful yet.

https://codereview.chromium.org/1129293002/diff/100001/chrome/browser/app_con...
File chrome/browser/app_controller_mac.mm (right):

https://codereview.chromium.org/1129293002/diff/100001/chrome/browser/app_con...
chrome/browser/app_controller_mac.mm:1257: // If the last profile was locked, we
have to open the User Manager, as the
nit: update comment for system profile.

https://codereview.chromium.org/1129293002/diff/100001/chrome/browser/profile...
File chrome/browser/profiles/profile_manager.cc (right):

https://codereview.chromium.org/1129293002/diff/100001/chrome/browser/profile...
chrome/browser/profiles/profile_manager.cc:441: // SystemProfile also needs
characteristics of being off the record, such
nit: "The system profile"

https://codereview.chromium.org/1129293002/diff/100001/chrome/browser/profile...
chrome/browser/profiles/profile_manager.cc:444: profile->GetPath() ==
ProfileManager::GetSystemProfilePath()) {
nit: use profile->IsSystemProfile() instead? (ditto for IsGuestSession() above)
(or maybe this should use ProfileManager::ShouldGoOffTheRecord()?)

https://codereview.chromium.org/1129293002/diff/100001/chrome/browser/profile...
chrome/browser/profiles/profile_manager.cc:445: SetGuestProfilePrefs(profile);
nit: maybe this should be renamed?

https://codereview.chromium.org/1129293002/diff/100001/chrome/browser/profile...
chrome/browser/profiles/profile_manager.cc:1009: // If this was the guest
profile, finish setting its special status.
nit: update comment.

https://codereview.chromium.org/1129293002/diff/100001/chrome/browser/profile...
chrome/browser/profiles/profile_manager.cc:1010: if (profile->GetPath() ==
ProfileManager::GetGuestProfilePath() ||
ditto nit: use IsSystemProfile() and IsGuestSession() or ShouldGoOffTheRecord()

https://codereview.chromium.org/1129293002/diff/100001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc (right):

https://codereview.chromium.org/1129293002/diff/100001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc:375: // record
profile, unless in guest session (where off the record profile is
nit/q: Did you mean to use the system profile and not its original profile (if
that really exists)? Is the system profile an instance of ProfileImpl (or a
subclass instance, like OffTheRecordProfileImpl)? If the former, wouldn't
GetOriginalProfile return itself, and then isn't this change unnecessary?

https://codereview.chromium.org/1129293002/diff/100001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc:378: profile_ =
nit: consider:
  profile_ = ProfileManager::GetActiveUserProfile();
  if (!profile_->IsGuestSession() && !profile_->IsSystemProfile())
    profile_ = profile_->GetOriginalProfile();

https://codereview.chromium.org/1129293002/diff/100001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc:2058:
!profile->IsSystemProfile();
nit: should this only be indented 4 spaces without parens around the conditions?

https://codereview.chromium.org/1129293002/diff/100001/chrome/browser/ui/brow...
File chrome/browser/ui/browser.cc (right):

https://codereview.chromium.org/1129293002/diff/100001/chrome/browser/ui/brow...
chrome/browser/ui/browser.cc:368: DCHECK(!profile_->IsSystemProfile());
nit: maybe use CHECK instead of DCHECK? Then eliminate the UMA entry?

https://codereview.chromium.org/1129293002/diff/100001/chrome/browser/ui/brow...
chrome/browser/ui/browser.cc:369: // The SystemProfile should never open a real
Browser.
nit: maybe print this comment as a warning with the [D]CHECK like above?

https://codereview.chromium.org/1129293002/diff/100001/chrome/browser/ui/brow...
File chrome/browser/ui/browser_command_controller.cc (right):

https://codereview.chromium.org/1129293002/diff/100001/chrome/browser/ui/brow...
chrome/browser/ui/browser_command_controller.cc:958:
profile()->IsSystemProfile();
Should this code instead [D]CHECK against this if no browser instances should be
created for the system profile? (BrowserCommandController instances are only
created for Browser instances)

https://codereview.chromium.org/1129293002/diff/100001/chrome/browser/ui/brow...
chrome/browser/ui/browser_command_controller.cc:1197:
!profile()->IsGuestSession() &&
ditto

https://codereview.chromium.org/1129293002/diff/100001/chrome/browser/ui/star...
File chrome/browser/ui/startup/startup_browser_creator.cc (right):

https://codereview.chromium.org/1129293002/diff/100001/chrome/browser/ui/star...
chrome/browser/ui/startup/startup_browser_creator.cc:693:
last_used_profile->GetOffTheRecordProfile()) > 0;
Is GetOffTheRecordProfile safe to call for the system profile?

[Mike Lerman, 2015-05-21 15:27:32]

https://codereview.chromium.org/1129293002/diff/100001/chrome/browser/app_con...
File chrome/browser/app_controller_mac.mm (right):

https://codereview.chromium.org/1129293002/diff/100001/chrome/browser/app_con...
chrome/browser/app_controller_mac.mm:1257: // If the last profile was locked, we
have to open the User Manager, as the
On 2015/05/21 04:34:43, msw wrote:
> nit: update comment for system profile.

Done.

https://codereview.chromium.org/1129293002/diff/100001/chrome/browser/profile...
File chrome/browser/profiles/profile_manager.cc (right):

https://codereview.chromium.org/1129293002/diff/100001/chrome/browser/profile...
chrome/browser/profiles/profile_manager.cc:441: // SystemProfile also needs
characteristics of being off the record, such
On 2015/05/21 04:34:43, msw wrote:
> nit: "The system profile"

Done.

https://codereview.chromium.org/1129293002/diff/100001/chrome/browser/profile...
chrome/browser/profiles/profile_manager.cc:444: profile->GetPath() ==
ProfileManager::GetSystemProfilePath()) {
On 2015/05/21 04:34:43, msw wrote:
> nit: use profile->IsSystemProfile() instead? (ditto for IsGuestSession()
above)
> (or maybe this should use ProfileManager::ShouldGoOffTheRecord()?)

This works now, as these items are set during profile_impl's constructor. Done.

https://codereview.chromium.org/1129293002/diff/100001/chrome/browser/profile...
chrome/browser/profiles/profile_manager.cc:445: SetGuestProfilePrefs(profile);
On 2015/05/21 04:34:43, msw wrote:
> nit: maybe this should be renamed?

I tried "SetNonPersonalProfilePrefs" since the profiles aren't for a single
person or created by any person, they're created by chromium. Up for a better
name :) AutoProfile? SharedOTRProfile? PredefinedProfile? There isn't really a
good word that unifies these.

https://codereview.chromium.org/1129293002/diff/100001/chrome/browser/profile...
chrome/browser/profiles/profile_manager.cc:1009: // If this was the guest
profile, finish setting its special status.
On 2015/05/21 04:34:43, msw wrote:
> nit: update comment.

Done.

https://codereview.chromium.org/1129293002/diff/100001/chrome/browser/profile...
chrome/browser/profiles/profile_manager.cc:1010: if (profile->GetPath() ==
ProfileManager::GetGuestProfilePath() ||
On 2015/05/21 04:34:43, msw wrote:
> ditto nit: use IsSystemProfile() and IsGuestSession() or
ShouldGoOffTheRecord()

Done.

https://codereview.chromium.org/1129293002/diff/100001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc (right):

https://codereview.chromium.org/1129293002/diff/100001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc:375: // record
profile, unless in guest session (where off the record profile is
On 2015/05/21 04:34:43, msw wrote:
> nit/q: Did you mean to use the system profile and not its original profile (if
> that really exists)? Is the system profile an instance of ProfileImpl (or a
> subclass instance, like OffTheRecordProfileImpl)? If the former, wouldn't
> GetOriginalProfile return itself, and then isn't this change unnecessary?

SystemProfile is OTRProfileImpl. There is an original that was created, but we
don't want to use it. (this is similar to guest)
So yes, intentionally want to do what Guest does, and stay with the OTR version.

https://codereview.chromium.org/1129293002/diff/100001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc:378: profile_ =
On 2015/05/21 04:34:43, msw wrote:
> nit: consider:
>   profile_ = ProfileManager::GetActiveUserProfile();
>   if (!profile_->IsGuestSession() && !profile_->IsSystemProfile())
>     profile_ = profile_->GetOriginalProfile();

Done.

https://codereview.chromium.org/1129293002/diff/100001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc:2058:
!profile->IsSystemProfile();
On 2015/05/21 04:34:43, msw wrote:
> nit: should this only be indented 4 spaces without parens around the
conditions?

I tried it your way, but apparently not - I did that, and then PRESUBMIT yelled
at me, and git cl format confirms this is the correct indentation.

https://codereview.chromium.org/1129293002/diff/100001/chrome/browser/ui/brow...
File chrome/browser/ui/browser.cc (right):

https://codereview.chromium.org/1129293002/diff/100001/chrome/browser/ui/brow...
chrome/browser/ui/browser.cc:368: DCHECK(!profile_->IsSystemProfile());
On 2015/05/21 04:34:43, msw wrote:
> nit: maybe use CHECK instead of DCHECK? Then eliminate the UMA entry?

I'm glad you brought this up. We talked about this on the team. I'm not
confident enough that there aren't users or use cases where this is happening
(browsers with System Profile), and they're living in weird but generally usable
configurations, and forcing CHECKs on them suddenly would be very harsh. Rather,
I propose we go with this for now, and perhaps leave a TODO: to check the
Metrics once they've been gathered in stable, and perhaps change DCHECK to
CHECK.

Also, if we do want to make this a CHECK, I'd like to make it easily revertable;
I would land this CL as is, and then land another CL that removes the D.

https://codereview.chromium.org/1129293002/diff/100001/chrome/browser/ui/brow...
chrome/browser/ui/browser.cc:369: // The SystemProfile should never open a real
Browser.
On 2015/05/21 04:34:43, msw wrote:
> nit: maybe print this comment as a warning with the [D]CHECK like above?

Done.

https://codereview.chromium.org/1129293002/diff/100001/chrome/browser/ui/brow...
File chrome/browser/ui/browser_command_controller.cc (right):

https://codereview.chromium.org/1129293002/diff/100001/chrome/browser/ui/brow...
chrome/browser/ui/browser_command_controller.cc:958:
profile()->IsSystemProfile();
On 2015/05/21 04:34:43, msw wrote:
> Should this code instead [D]CHECK against this if no browser instances should
be
> created for the system profile? (BrowserCommandController instances are only
> created for Browser instances)

What happens on Mac (or Linux I think) when there's still a Menu even if there's
no Browser, such as if I Close the last browser or have User Manager open?

https://codereview.chromium.org/1129293002/diff/100001/chrome/browser/ui/brow...
chrome/browser/ui/browser_command_controller.cc:1197:
!profile()->IsGuestSession() &&
On 2015/05/21 04:34:43, msw wrote:
> ditto

changed to system

https://codereview.chromium.org/1129293002/diff/100001/chrome/browser/ui/star...
File chrome/browser/ui/startup/startup_browser_creator.cc (right):

https://codereview.chromium.org/1129293002/diff/100001/chrome/browser/ui/star...
chrome/browser/ui/startup/startup_browser_creator.cc:693:
last_used_profile->GetOffTheRecordProfile()) > 0;
On 2015/05/21 04:34:43, msw wrote:
> Is GetOffTheRecordProfile safe to call for the system profile?

Don't see why not? the function just iterates over the BrowserList looking for
matching profiles, and ideally won't find any.

[Mike Lerman, 2015-05-21 15:28:39]

mlerman@chromium.org changed reviewers:
+ isherman@chromium.org

[Mike Lerman, 2015-05-21 15:28:39]

Hi Ilya,

Can you take a look at tools/metrics/actions for me, please? Thanks!

Mike

[msw, 2015-05-21 17:48:12]

https://codereview.chromium.org/1129293002/diff/100001/chrome/browser/profile...
File chrome/browser/profiles/profile_manager.cc (right):

https://codereview.chromium.org/1129293002/diff/100001/chrome/browser/profile...
chrome/browser/profiles/profile_manager.cc:445: SetGuestProfilePrefs(profile);
On 2015/05/21 15:27:30, Mike Lerman wrote:
> On 2015/05/21 04:34:43, msw wrote:
> > nit: maybe this should be renamed?
> 
> I tried "SetNonPersonalProfilePrefs" since the profiles aren't for a single
> person or created by any person, they're created by chromium. Up for a better
> name :) AutoProfile? SharedOTRProfile? PredefinedProfile? There isn't really a
> good word that unifies these.

SetNonPersonalProfilePrefs sgtm. SetSystemProfilePrefs might also be okay, if
one might consider the Guest Profile a "system profile"...

https://codereview.chromium.org/1129293002/diff/100001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc (right):

https://codereview.chromium.org/1129293002/diff/100001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc:375: // record
profile, unless in guest session (where off the record profile is
On 2015/05/21 15:27:31, Mike Lerman wrote:
> On 2015/05/21 04:34:43, msw wrote:
> > nit/q: Did you mean to use the system profile and not its original profile
(if
> > that really exists)? Is the system profile an instance of ProfileImpl (or a
> > subclass instance, like OffTheRecordProfileImpl)? If the former, wouldn't
> > GetOriginalProfile return itself, and then isn't this change unnecessary?
> 
> SystemProfile is OTRProfileImpl. There is an original that was created, but we
> don't want to use it. (this is similar to guest)
> So yes, intentionally want to do what Guest does, and stay with the OTR
version.

Acknowledged.

https://codereview.chromium.org/1129293002/diff/100001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc:2058:
!profile->IsSystemProfile();
On 2015/05/21 15:27:31, Mike Lerman wrote:
> On 2015/05/21 04:34:43, msw wrote:
> > nit: should this only be indented 4 spaces without parens around the
> conditions?
> 
> I tried it your way, but apparently not - I did that, and then PRESUBMIT
yelled
> at me, and git cl format confirms this is the correct indentation.

Acknowledged.

https://codereview.chromium.org/1129293002/diff/100001/chrome/browser/ui/brow...
File chrome/browser/ui/browser.cc (right):

https://codereview.chromium.org/1129293002/diff/100001/chrome/browser/ui/brow...
chrome/browser/ui/browser.cc:368: DCHECK(!profile_->IsSystemProfile());
On 2015/05/21 15:27:31, Mike Lerman wrote:
> On 2015/05/21 04:34:43, msw wrote:
> > nit: maybe use CHECK instead of DCHECK? Then eliminate the UMA entry?
> 
> I'm glad you brought this up. We talked about this on the team. I'm not
> confident enough that there aren't users or use cases where this is happening
> (browsers with System Profile), and they're living in weird but generally
usable
> configurations, and forcing CHECKs on them suddenly would be very harsh.
Rather,
> I propose we go with this for now, and perhaps leave a TODO: to check the
> Metrics once they've been gathered in stable, and perhaps change DCHECK to
> CHECK.
> 
> Also, if we do want to make this a CHECK, I'd like to make it easily
revertable;
> I would land this CL as is, and then land another CL that removes the D.

Hmm, that sounds ok, I hope we find some way forward for those users.

https://codereview.chromium.org/1129293002/diff/100001/chrome/browser/ui/brow...
File chrome/browser/ui/browser_command_controller.cc (right):

https://codereview.chromium.org/1129293002/diff/100001/chrome/browser/ui/brow...
chrome/browser/ui/browser_command_controller.cc:958:
profile()->IsSystemProfile();
On 2015/05/21 15:27:31, Mike Lerman wrote:
> On 2015/05/21 04:34:43, msw wrote:
> > Should this code instead [D]CHECK against this if no browser instances
should
> be
> > created for the system profile? (BrowserCommandController instances are only
> > created for Browser instances)
> 
> What happens on Mac (or Linux I think) when there's still a Menu even if
there's
> no Browser, such as if I Close the last browser or have User Manager open?

I'm not entirely sure. Outside tests, it looks like the only instantiation is in
Browser, but I see that chrome/browser/app_controller_mac.mm uses its
CommandUpdater, |menuState_| in some static BrowserCommandController functions.
Maybe treat this like your Browser instance DCHECKs against using the system
profile for now?

https://codereview.chromium.org/1129293002/diff/100001/chrome/browser/ui/star...
File chrome/browser/ui/startup/startup_browser_creator.cc (right):

https://codereview.chromium.org/1129293002/diff/100001/chrome/browser/ui/star...
chrome/browser/ui/startup/startup_browser_creator.cc:693:
last_used_profile->GetOffTheRecordProfile()) > 0;
On 2015/05/21 15:27:31, Mike Lerman wrote:
> On 2015/05/21 04:34:43, msw wrote:
> > Is GetOffTheRecordProfile safe to call for the system profile?
> 
> Don't see why not? the function just iterates over the BrowserList looking for
> matching profiles, and ideally won't find any.

Acknowledged.

https://codereview.chromium.org/1129293002/diff/120001/chrome/browser/profile...
File chrome/browser/profiles/profile_manager.cc (right):

https://codereview.chromium.org/1129293002/diff/120001/chrome/browser/profile...
chrome/browser/profiles/profile_manager.cc:441: // System Profile also needs
characteristics of being off the record, such
nit: I think this should be "The system profile", to match the general
terminology and capitalization of (most/all) other comment references.

https://codereview.chromium.org/1129293002/diff/120001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc (right):

https://codereview.chromium.org/1129293002/diff/120001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc:378: if
(!active_profile->IsGuestSession() && !active_profile->IsSystemProfile())
Careful, this no longer sets |profile_|! Take another look at my comment.

https://codereview.chromium.org/1129293002/diff/120001/chrome/browser/ui/brow...
File chrome/browser/ui/browser.cc (right):

https://codereview.chromium.org/1129293002/diff/120001/chrome/browser/ui/brow...
chrome/browser/ui/browser.cc:369: << "System profile should never have a real
browser.";
nit: "The system profile"

https://codereview.chromium.org/1129293002/diff/120001/chrome/browser/ui/brow...
chrome/browser/ui/browser.cc:370: // The SystemProfile should never open a real
Browser.
nit: remove this comment, in lieu of the printed warning above.

[Mike Lerman, 2015-05-21 19:24:32]

mlerman@chromium.org changed reviewers:
+ rsesek@chromium.org

[Mike Lerman, 2015-05-21 19:24:34]

Thanks Mike - comments addressed below.

Robert, can you take a look at chrome/browser/app_controller_mac.mm please?
Thanks!

https://codereview.chromium.org/1129293002/diff/100001/chrome/browser/ui/brow...
File chrome/browser/ui/browser.cc (right):

https://codereview.chromium.org/1129293002/diff/100001/chrome/browser/ui/brow...
chrome/browser/ui/browser.cc:368: DCHECK(!profile_->IsSystemProfile());
On 2015/05/21 17:48:12, msw wrote:
> On 2015/05/21 15:27:31, Mike Lerman wrote:
> > On 2015/05/21 04:34:43, msw wrote:
> > > nit: maybe use CHECK instead of DCHECK? Then eliminate the UMA entry?
> > 
> > I'm glad you brought this up. We talked about this on the team. I'm not
> > confident enough that there aren't users or use cases where this is
happening
> > (browsers with System Profile), and they're living in weird but generally
> usable
> > configurations, and forcing CHECKs on them suddenly would be very harsh.
> Rather,
> > I propose we go with this for now, and perhaps leave a TODO: to check the
> > Metrics once they've been gathered in stable, and perhaps change DCHECK to
> > CHECK.
> > 
> > Also, if we do want to make this a CHECK, I'd like to make it easily
> revertable;
> > I would land this CL as is, and then land another CL that removes the D.
> 
> Hmm, that sounds ok, I hope we find some way forward for those users.

I hope so - knowing how many users there are will help. Added the TODO.

https://codereview.chromium.org/1129293002/diff/100001/chrome/browser/ui/brow...
File chrome/browser/ui/browser_command_controller.cc (right):

https://codereview.chromium.org/1129293002/diff/100001/chrome/browser/ui/brow...
chrome/browser/ui/browser_command_controller.cc:958:
profile()->IsSystemProfile();
On 2015/05/21 17:48:12, msw wrote:
> On 2015/05/21 15:27:31, Mike Lerman wrote:
> > On 2015/05/21 04:34:43, msw wrote:
> > > Should this code instead [D]CHECK against this if no browser instances
> should
> > be
> > > created for the system profile? (BrowserCommandController instances are
only
> > > created for Browser instances)
> > 
> > What happens on Mac (or Linux I think) when there's still a Menu even if
> there's
> > no Browser, such as if I Close the last browser or have User Manager open?
> 
> I'm not entirely sure. Outside tests, it looks like the only instantiation is
in
> Browser, but I see that chrome/browser/app_controller_mac.mm uses its
> CommandUpdater, |menuState_| in some static BrowserCommandController
functions.
> Maybe treat this like your Browser instance DCHECKs against using the system
> profile for now?

DCHECK added.

https://codereview.chromium.org/1129293002/diff/120001/chrome/browser/profile...
File chrome/browser/profiles/profile_manager.cc (right):

https://codereview.chromium.org/1129293002/diff/120001/chrome/browser/profile...
chrome/browser/profiles/profile_manager.cc:441: // System Profile also needs
characteristics of being off the record, such
On 2015/05/21 17:48:12, msw wrote:
> nit: I think this should be "The system profile", to match the general
> terminology and capitalization of (most/all) other comment references.

Done.

https://codereview.chromium.org/1129293002/diff/120001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc (right):

https://codereview.chromium.org/1129293002/diff/120001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc:378: if
(!active_profile->IsGuestSession() && !active_profile->IsSystemProfile())
On 2015/05/21 17:48:12, msw wrote:
> Careful, this no longer sets |profile_|! Take another look at my comment.

Doh! Done.

https://codereview.chromium.org/1129293002/diff/120001/chrome/browser/ui/brow...
File chrome/browser/ui/browser.cc (right):

https://codereview.chromium.org/1129293002/diff/120001/chrome/browser/ui/brow...
chrome/browser/ui/browser.cc:369: << "System profile should never have a real
browser.";
On 2015/05/21 17:48:12, msw wrote:
> nit: "The system profile"

Done.

https://codereview.chromium.org/1129293002/diff/120001/chrome/browser/ui/brow...
chrome/browser/ui/browser.cc:370: // The SystemProfile should never open a real
Browser.
On 2015/05/21 17:48:12, msw wrote:
> nit: remove this comment, in lieu of the printed warning above.

Done.

[Ilya Sherman, 2015-05-21 19:29:13]

https://codereview.chromium.org/1129293002/diff/160001/tools/metrics/actions/...
File tools/metrics/actions/actions.xml (right):

https://codereview.chromium.org/1129293002/diff/160001/tools/metrics/actions/...
tools/metrics/actions/actions.xml:1787: </action>
What is your plan for understanding this action?  Typically, I'd recommend using
a histogram, and building in a baseline to compare against.  Histograms are more
space-efficient, and have better dashboard support to boot.

https://codereview.chromium.org/1129293002/diff/160001/tools/metrics/actions/...
tools/metrics/actions/actions.xml:2687: <description>Please enter the
description of the metric.</description>
Could you please upload a separate CL to fill in the missing pieces of
actions.xml?  You can TBR me on it if you'd like, but it would be nice not to
muddy this CL with all of these unrelated changes.  Also, could you please file
a bug tagged with "Internals-Metrics" to investigate why our presubmit scripts
didn't catch these in the first place?  Thanks.

[Robert Sesek, 2015-05-21 19:36:27]

app_controller_mac.mm stamp lgtm

[msw, 2015-05-21 19:36:48]

lgtm modulo Ilya's UMA comments.

[Mike Lerman, 2015-05-21 20:38:09]

Comments addressed, Ilya. Thanks!

https://codereview.chromium.org/1129293002/diff/160001/tools/metrics/actions/...
File tools/metrics/actions/actions.xml (right):

https://codereview.chromium.org/1129293002/diff/160001/tools/metrics/actions/...
tools/metrics/actions/actions.xml:1787: </action>
On 2015/05/21 19:29:12, Ilya Sherman wrote:
> What is your plan for understanding this action?  Typically, I'd recommend
using
> a histogram, and building in a baseline to compare against.  Histograms are
more
> space-efficient, and have better dashboard support to boot.

Anything non-zero tells us that browsers are being opened for the system
profile. We expect and want this number to be zero, otherwise wonky things are
happening that should be followed up.

I expanded the description somewhat.

https://codereview.chromium.org/1129293002/diff/160001/tools/metrics/actions/...
tools/metrics/actions/actions.xml:2687: <description>Please enter the
description of the metric.</description>
On 2015/05/21 19:29:12, Ilya Sherman wrote:
> Could you please upload a separate CL to fill in the missing pieces of
> actions.xml?  You can TBR me on it if you'd like, but it would be nice not to
> muddy this CL with all of these unrelated changes.  Also, could you please
file
> a bug tagged with "Internals-Metrics" to investigate why our presubmit scripts
> didn't catch these in the first place?  Thanks.

Sure.

[Ilya Sherman, 2015-05-21 21:18:14]

https://codereview.chromium.org/1129293002/diff/160001/tools/metrics/actions/...
File tools/metrics/actions/actions.xml (right):

https://codereview.chromium.org/1129293002/diff/160001/tools/metrics/actions/...
tools/metrics/actions/actions.xml:1787: </action>
On 2015/05/21 20:38:09, Mike Lerman wrote:
> On 2015/05/21 19:29:12, Ilya Sherman wrote:
> > What is your plan for understanding this action?  Typically, I'd recommend
> using
> > a histogram, and building in a baseline to compare against.  Histograms are
> more
> > space-efficient, and have better dashboard support to boot.
> 
> Anything non-zero tells us that browsers are being opened for the system
> profile. We expect and want this number to be zero, otherwise wonky things are
> happening that should be followed up.
> 
> I expanded the description somewhat.

I'd still recommend a histogram -- probably associated with the BooleanHit enum
-- rather than an action.  However, if you really prefer to use an action, it's
okay if you do so, in which case LGTM.

[Mike Lerman, 2015-05-25 10:41:30]

https://codereview.chromium.org/1129293002/diff/160001/tools/metrics/actions/...
File tools/metrics/actions/actions.xml (right):

https://codereview.chromium.org/1129293002/diff/160001/tools/metrics/actions/...
tools/metrics/actions/actions.xml:1787: </action>
On 2015/05/21 21:18:14, Ilya Sherman wrote:
> On 2015/05/21 20:38:09, Mike Lerman wrote:
> > On 2015/05/21 19:29:12, Ilya Sherman wrote:
> > > What is your plan for understanding this action?  Typically, I'd recommend
> > using
> > > a histogram, and building in a baseline to compare against.  Histograms
are
> > more
> > > space-efficient, and have better dashboard support to boot.
> > 
> > Anything non-zero tells us that browsers are being opened for the system
> > profile. We expect and want this number to be zero, otherwise wonky things
are
> > happening that should be followed up.
> > 
> > I expanded the description somewhat.
> 
> I'd still recommend a histogram -- probably associated with the BooleanHit
enum
> -- rather than an action.  However, if you really prefer to use an action,
it's
> okay if you do so, in which case LGTM.

I just need to know if there's a non-zero number of triggers or not, and the
filter based off OS and Milestone that's present is all I need. I'll go with
this - thanks!

[Mike Lerman, 2015-05-25 10:41:36]

The CQ bit was checked by mlerman@chromium.org

[Mike Lerman, 2015-05-25 10:41:37]

The patchset sent to the CQ was uploaded after l-g-t-m from rockot@chromium.org,
rsesek@chromium.org, msw@chromium.org
Link to the patchset: https://codereview.chromium.org/1129293002/#ps200001
(title: "remove autogenerated actions")

[commit-bot: I haz the power, 2015-05-25 10:41:45]

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

[commit-bot: I haz the power, 2015-05-25 11:40:28]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2015-05-25 11:41:23]

Patchset 11 (id:??) landed as
https://crrev.com/7831f57d5f56e9cafe77388b20a83a90d6eb7918
Cr-Commit-Position: refs/heads/master@{#331276}