Allow normal users to change per session flags.

Allow normal users to change per session flags.

This is the first step towards better support for about:flags on ChromeOS.

It allows users to specify flags that will be applied only after they log in thus they will not influence the login profile.

No non-owner  flags will be applied until chromium-os:39249 is implemented. 

BUG=39248
TEST=Open about flags as a non-owner and the flags should be editable but will not have any effect yet.


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

[pastarmovj, 2013-02-26 15:53:10]

Hi Mattias,
Please review this first step towards per user flags. As already obvious from
the bug split - this CL does not intend to get chrome reboots in only to enable
the pref system and the UI to handle per user flags.
Thanks!

[Mattias Nissler (ping if slow), 2013-02-26 16:27:35]

A couple of comments, but you should get somebody who feels responsible for
flags_ui.{h,cc} to review this.

https://codereview.chromium.org/12330137/diff/1010/chrome/app/generated_resou...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/12330137/diff/1010/chrome/app/generated_resou...
chrome/app/generated_resources.grd:6139: +          Flags that apply system-wide
can only be set by the owner:
can we finally use a proper placeholder here? ;)

https://codereview.chromium.org/12330137/diff/1010/chrome/browser/chromeos/lo...
File chrome/browser/chromeos/login/login_utils.cc (right):

https://codereview.chromium.org/12330137/diff/1010/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/login_utils.cc:287: 
Add a TODO here that indicates we want to re-launch the browser here if there's
a flag change.

https://codereview.chromium.org/12330137/diff/1010/chrome/browser/resources/f...
File chrome/browser/resources/flags.html (right):

https://codereview.chromium.org/12330137/diff/1010/chrome/browser/resources/f...
chrome/browser/resources/flags.html:26: <div jsdisplay="ownerUserId"
id="blurb-container">
blurb-container is now a duplicate ID...

https://codereview.chromium.org/12330137/diff/1010/chrome/browser/ui/webui/fl...
File chrome/browser/ui/webui/flags_ui.cc (right):

https://codereview.chromium.org/12330137/diff/1010/chrome/browser/ui/webui/fl...
chrome/browser/ui/webui/flags_ui.cc:178: if
(!chromeos::UserManager::Get()->IsCurrentUserOwner()) {
Do we run into issues here with racing against the ownership check? Same for
first sign-in when the owner key gets generated?

[pastarmovj, 2013-02-26 17:23:47]

Adding James and Ivan for OWNER reviews.

@Mattias: PTAL.

https://codereview.chromium.org/12330137/diff/1010/chrome/app/generated_resou...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/12330137/diff/1010/chrome/app/generated_resou...
chrome/app/generated_resources.grd:6139: +          Flags that apply system-wide
can only be set by the owner:
On 2013/02/26 16:27:35, Mattias Nissler wrote:
> can we finally use a proper placeholder here? ;)

Done.

https://codereview.chromium.org/12330137/diff/1010/chrome/browser/chromeos/lo...
File chrome/browser/chromeos/login/login_utils.cc (right):

https://codereview.chromium.org/12330137/diff/1010/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/login_utils.cc:287: 
On 2013/02/26 16:27:35, Mattias Nissler wrote:
> Add a TODO here that indicates we want to re-launch the browser here if
there's
> a flag change.

Done.

https://codereview.chromium.org/12330137/diff/1010/chrome/browser/resources/f...
File chrome/browser/resources/flags.html (right):

https://codereview.chromium.org/12330137/diff/1010/chrome/browser/resources/f...
chrome/browser/resources/flags.html:26: <div jsdisplay="ownerUserId"
id="blurb-container">
On 2013/02/26 16:27:35, Mattias Nissler wrote:
> blurb-container is now a duplicate ID...

Done.

https://codereview.chromium.org/12330137/diff/1010/chrome/browser/ui/webui/fl...
File chrome/browser/ui/webui/flags_ui.cc (right):

https://codereview.chromium.org/12330137/diff/1010/chrome/browser/ui/webui/fl...
chrome/browser/ui/webui/flags_ui.cc:178: if
(!chromeos::UserManager::Get()->IsCurrentUserOwner()) {
On 2013/02/26 16:27:35, Mattias Nissler wrote:
> Do we run into issues here with racing against the ownership check? Same for
> first sign-in when the owner key gets generated?

On first owner sign-in maybe but for the rest of tries I think that with our
lockbox cache chances are low we'll see this problem again.

[Ivan Korotkov, 2013-02-26 17:45:31]

chromeos LGTM

[pastarmovj, 2013-02-28 12:32:12]

@Nico: I saw you touched a lot of the about:flags code so I though it might be
useful if you could have a look at this CL if you spot any major issues with our
approach.

@Mattias: PTAL.

@James: Polite ping :)

Thanks all!

[Nico, 2013-02-28 12:37:41]

Can you describe the flow how this is going to work more?

Right now it's:

1. Change stuff on about:flags
2. Restart chrome
3. Flags get converted to flags at chrome restart and are active after that

From the CL description it kind of sounds like this will no longer be the model?

(For perspective: We do not want to support flags that can change at runtime
without a chrome restart.)

[pastarmovj, 2013-03-01 12:36:35]

On 2013/02/28 12:37:41, Nico wrote:
> Can you describe the flow how this is going to work more?
> 
> Right now it's:
> 
> 1. Change stuff on about:flags
> 2. Restart chrome
> 3. Flags get converted to flags at chrome restart and are active after that
> 
> From the CL description it kind of sounds like this will no longer be the
model?
> 
> (For perspective: We do not want to support flags that can change at runtime
> without a chrome restart.)

@Nico, Mattias: PTAL.

[Mattias Nissler (ping if slow), 2013-03-02 22:43:49]

https://codereview.chromium.org/12330137/diff/23001/chrome/browser/chromeos/l...
File chrome/browser/chromeos/login/login_utils.cc (right):

https://codereview.chromium.org/12330137/diff/23001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/login_utils.cc:288: // TODO(pastarmovj): Restart
the browser and apply any flags have been set
grammar

https://codereview.chromium.org/12330137/diff/23001/chrome/browser/ui/webui/f...
File chrome/browser/ui/webui/flags_ui.cc (right):

https://codereview.chromium.org/12330137/diff/23001/chrome/browser/ui/webui/f...
chrome/browser/ui/webui/flags_ui.cc:181: weak_factory_.GetWeakPtr(), profile));
Is there a race here now? What if the Javascript part calls a handler before
FinishInitialization runs? Also, what if the ownership status changes? A better
approach might be to observer DeviceSettingsService for OwnershipStatusChanged
and switching out the PrefService accordingly on that event.

https://codereview.chromium.org/12330137/diff/23001/chrome/browser/ui/webui/f...
chrome/browser/ui/webui/flags_ui.cc:207: void
FlagsUI::RegisterUserPrefs(PrefRegistrySyncable* registry) {
should this be OS_CHROMEOS only as well?

https://codereview.chromium.org/12330137/diff/23001/chrome/browser/ui/webui/f...
chrome/browser/ui/webui/flags_ui.cc:225: // because it will never be accessible
anymore.
nit: a the

https://codereview.chromium.org/12330137/diff/23001/chrome/browser/ui/webui/f...
chrome/browser/ui/webui/flags_ui.cc:231: content::WebUIDataSource::Add(profile,
CreateFlagsUIHTMLSource());
wouldn't this also be needed on desktop?

[pastarmovj, 2013-03-04 14:31:51]

https://codereview.chromium.org/12330137/diff/23001/chrome/browser/chromeos/l...
File chrome/browser/chromeos/login/login_utils.cc (right):

https://codereview.chromium.org/12330137/diff/23001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/login_utils.cc:288: // TODO(pastarmovj): Restart
the browser and apply any flags have been set
On 2013/03/02 22:43:49, Mattias Nissler wrote:
> grammar

Done.

https://codereview.chromium.org/12330137/diff/23001/chrome/browser/ui/webui/f...
File chrome/browser/ui/webui/flags_ui.cc (right):

https://codereview.chromium.org/12330137/diff/23001/chrome/browser/ui/webui/f...
chrome/browser/ui/webui/flags_ui.cc:181: weak_factory_.GetWeakPtr(), profile));
On 2013/03/02 22:43:50, Mattias Nissler wrote:
> Is there a race here now? What if the Javascript part calls a handler before
> FinishInitialization runs? Also, what if the ownership status changes? A
better
> approach might be to observer DeviceSettingsService for OwnershipStatusChanged
> and switching out the PrefService accordingly on that event.

There is no race because we never call content::WebUIDataSource::Add(profile,
CreateFlagsUIHTMLSource()); before we are done with checking that which means
the UI will not be built at all until we are done with this check.

https://codereview.chromium.org/12330137/diff/23001/chrome/browser/ui/webui/f...
chrome/browser/ui/webui/flags_ui.cc:207: void
FlagsUI::RegisterUserPrefs(PrefRegistrySyncable* registry) {
On 2013/03/02 22:43:50, Mattias Nissler wrote:
> should this be OS_CHROMEOS only as well?

Done.

https://codereview.chromium.org/12330137/diff/23001/chrome/browser/ui/webui/f...
chrome/browser/ui/webui/flags_ui.cc:225: // because it will never be accessible
anymore.
On 2013/03/02 22:43:50, Mattias Nissler wrote:
> nit: a the

Done.

https://codereview.chromium.org/12330137/diff/23001/chrome/browser/ui/webui/f...
chrome/browser/ui/webui/flags_ui.cc:231: content::WebUIDataSource::Add(profile,
CreateFlagsUIHTMLSource());
On 2013/03/02 22:43:50, Mattias Nissler wrote:
> wouldn't this also be needed on desktop?

Yes and it is called for the desktop on line 187. We need to delay calling this
for ChromeOS exactly to avoid the race problem you pointed out on line 181.

[pastarmovj, 2013-03-05 15:05:59]

This time adding James correctly with his Chromium.org address. 

@James: Please review this as owner of the touched UI code. Thanks!

[Nico, 2013-03-05 15:11:08]

Looks fine. Have you talked to the ux leads about the open UI question we
discussed? ("How are flags that aren't modifiable by non-owners presented?")

Does the CL description still match what the patch does? If not, update it
please.

https://codereview.chromium.org/12330137/diff/28001/chrome/browser/ui/webui/f...
File chrome/browser/ui/webui/flags_ui.h (right):

https://codereview.chromium.org/12330137/diff/28001/chrome/browser/ui/webui/f...
chrome/browser/ui/webui/flags_ui.h:38: void FinishInitialization(
Needs a comment about what this does and why it's only needed on CrOs.

[pastarmovj, 2013-03-05 15:23:13]

I started a mail thread with Glen Murphy about that today. Offering him the two
options - add a new category of admin only flags or simply having the message on
top that says only the owner can change some flags.

https://codereview.chromium.org/12330137/diff/28001/chrome/browser/ui/webui/f...
File chrome/browser/ui/webui/flags_ui.h (right):

https://codereview.chromium.org/12330137/diff/28001/chrome/browser/ui/webui/f...
chrome/browser/ui/webui/flags_ui.h:38: void FinishInitialization(
On 2013/03/05 15:11:08, Nico wrote:
> Needs a comment about what this does and why it's only needed on CrOs.

Done.

[Nico, 2013-03-05 15:25:20]

ok, lgtm

Are you planning to add any test coverage for the cros-specific path?

[James Hawkins, 2013-03-06 00:16:56]

LGTM with nit.

https://codereview.chromium.org/12330137/diff/33002/chrome/browser/ui/webui/f...
File chrome/browser/ui/webui/flags_ui.cc (right):

https://codereview.chromium.org/12330137/diff/33002/chrome/browser/ui/webui/f...
chrome/browser/ui/webui/flags_ui.cc:65: // Set the strings to show which user
can actually change the flags
nit: Add period at end of sentence.

[pastarmovj, 2013-03-06 13:45:44]

Thanks for the reviews everyone. The CL is good rebased to ToT and going for CQ
now.

https://codereview.chromium.org/12330137/diff/33002/chrome/browser/ui/webui/f...
File chrome/browser/ui/webui/flags_ui.cc (right):

https://codereview.chromium.org/12330137/diff/33002/chrome/browser/ui/webui/f...
chrome/browser/ui/webui/flags_ui.cc:65: // Set the strings to show which user
can actually change the flags
On 2013/03/06 00:16:56, James Hawkins wrote:
> nit: Add period at end of sentence.

Done.

[commit-bot: I haz the power, 2013-03-06 13:49:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pastarmovj@chromium.org/12330137/43001

[commit-bot: I haz the power, 2013-03-06 14:14:07]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_rel.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-03-06 14:15:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pastarmovj@chromium.org/12330137/46003

[commit-bot: I haz the power, 2013-03-06 16:26:58]

Change committed as 186447