Owner flags storage should not save the flags names but the actual switches.

Owner flags storage should not save the flags names but the actual switches.

These are directly applied by the session manager which can not resolve them. 

It fixed an issue with the flags stored in the device settings being the flag names and not the actual switches as is required by the session manager causing the flags not to be applied to the login screen if the flag and the switch don't happen to coincide and to browser restarts even for the owner which should not happen.

BUG=280935
TEST=Unit tests + manual testing as described in the bug.

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

[pastarmovj, 2013-09-12 13:13:33]

Hi Dmitry and Nico,
can you please review this CL? 
It fixed an issue with the flags stored in the device settings being the flag
names and not the actual switches as is required by the session manager causing
the flags not to be applied to the login screen if the flag and the switch don't
happen to coincide and to browser restarts even for the owner which should not
happen.
Thanks,
Julian

[pastarmovj, 2013-09-12 13:44:44]

On 2013/09/12 13:13:33, pastarmovj wrote:
> Hi Dmitry and Nico,
> can you please review this CL? 
> It fixed an issue with the flags stored in the device settings being the flag
> names and not the actual switches as is required by the session manager
causing
> the flags not to be applied to the login screen if the flag and the switch
don't
> happen to coincide and to browser restarts even for the owner which should not
> happen.
> Thanks,
> Julian

Forgot to say that I added Nico for the flags part and Dmitry for the c/b/c
part. Sorry.

[Dmitry Polukhin, 2013-09-12 19:45:18]

LGTM c/b/c

[Nico, 2013-09-12 21:40:24]

My feedback is the same as last time when you added a bool parameter: enums read
better at the calling site. (e.g. kAddSentinels, kNoSentinels)

[pastarmovj, 2013-09-13 13:29:09]

Thanks for the reviews guys!

Nico PTAL.

[Nico, 2013-09-13 22:16:33]

lgtm

https://codereview.chromium.org/23983031/diff/10001/chrome/browser/about_flags.h
File chrome/browser/about_flags.h (right):

https://codereview.chromium.org/23983031/diff/10001/chrome/browser/about_flag...
chrome/browser/about_flags.h:109: // whether it should add or not the sentinel
switches around flags or not.
s/or not//g

https://codereview.chromium.org/23983031/diff/10001/chrome/browser/about_flag...
chrome/browser/about_flags.h:110: enum SentinelsMode { kDontAddSentinels,
kAddSentinels };
No contractions in names, and "DoNotAdd" is kind of link. I'd go with
kNoSentinels for the first one

[pastarmovj, 2013-09-16 13:28:14]

Thanks!

https://codereview.chromium.org/23983031/diff/10001/chrome/browser/about_flags.h
File chrome/browser/about_flags.h (right):

https://codereview.chromium.org/23983031/diff/10001/chrome/browser/about_flag...
chrome/browser/about_flags.h:109: // whether it should add or not the sentinel
switches around flags or not.
On 2013/09/13 22:16:33, Nico wrote:
> s/or not//g

Done.

https://codereview.chromium.org/23983031/diff/10001/chrome/browser/about_flag...
chrome/browser/about_flags.h:110: enum SentinelsMode { kDontAddSentinels,
kAddSentinels };
On 2013/09/13 22:16:33, Nico wrote:
> No contractions in names, and "DoNotAdd" is kind of link. I'd go with
> kNoSentinels for the first one

Done.

[commit-bot: I haz the power, 2013-09-16 13:28:39]

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

[commit-bot: I haz the power, 2013-09-16 13:35:57]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-09-16 14:46:11]

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

[commit-bot: I haz the power, 2013-09-16 17:16:23]

Change committed as 223353