Workaround to remove command line flag on ChromeOS.

Workaround to remove command line flag on ChromeOS.
When bookmarks experiment is enabled existing code will add a command line flag that will allow opt-out to be shown on a chrome:flags page.
On Chrome OS at time of login if they see command line flag from any experiment they restart browser. It creates delay and user will see a black screen for a couple seconds. It happens on every login, not only the first one.
I talked to CrOS devs and they strongly discourage from using command line flags.
So for Chrome OS only do not add flag to command line but still keep it in a flag_storage. Use flag storage when need to check if flag is set.

BUG=

Committed: https://crrev.com/ff0c52fb645f56ebdee15b256d73997184174257
Cr-Commit-Position: refs/heads/master@{#296504}

[yefimt, 2014-09-22 20:54:20]

yefim@chromium.org changed reviewers:
+ sky@chromium.org

[sky, 2014-09-22 21:40:03]

Your description isn't clear. I don't understand why we need this hack? What is
the exact problem?

[yefimt, 2014-09-22 22:01:44]

Updated description

[sky, 2014-09-22 23:19:53]

"When bookmarks experiment is enabled existing code will add a command line
flag" What is different about the enhanced bookmark experiment as compared to
other experiments we have? Do they all trigger something bad like this?

[yefimt, 2014-09-23 02:11:25]

On 2014/09/22 23:19:53, sky wrote:
> "When bookmarks experiment is enabled existing code will add a command line
> flag" What is different about the enhanced bookmark experiment as compared to
> other experiments we have? Do they all trigger something bad like this?

The only difference is that only small percentage of users changes settings on
chrome:flags page and it is initiated by them. In case of enhanced bookmarks
experiment is pushed from a server and eventually will go to all users.

And yes if you change settings on flags page to non default value, chrome will
be restarted on every login. That is true for not owner accounts only.

[sky, 2014-09-23 03:04:27]

I still don't get why this is specific to enhanced bookmarks and not a
general problem that should be addressed more centrally? Presumably
we'll see the same bug in the future when a new experiment is
triggered like this, right? Can't we fix more centrally?

  -Scott

On Mon, Sep 22, 2014 at 7:11 PM,  <yefim@chromium.org> wrote:
> On 2014/09/22 23:19:53, sky wrote:
>>
>> "When bookmarks experiment is enabled existing code will add a command
>> line
>> flag" What is different about the enhanced bookmark experiment as compared
>> to
>> other experiments we have? Do they all trigger something bad like this?
>
>
> The only difference is that only small percentage of users changes settings
> on
> chrome:flags page and it is initiated by them. In case of enhanced bookmarks
> experiment is pushed from a server and eventually will go to all users.
>
> And yes if you change settings on flags page to non default value, chrome
> will
> be restarted on every login. That is true for not owner accounts only.
>
> https://codereview.chromium.org/578333003/

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

[yefimt, 2014-09-23 17:29:58]

Ideally yes, and I believe CrOS people are looking for a long term solution,
+nkostylev@ to comment.
Now I need to fix enhanced bookmark experiment before branch point, as it is in
a middle of dogfood and this issue is blocking for CrOS roll-out.

[sky, 2014-09-23 22:25:56]

I would like to hear about why we can't fix this correctly now from
Nikita. If that isn't possible in a timely manner than I'll approve.

  -Scott

On Tue, Sep 23, 2014 at 10:29 AM,  <yefim@chromium.org> wrote:
> Ideally yes, and I believe CrOS people are looking for a long term solution,
> +nkostylev@ to comment.
> Now I need to fix enhanced bookmark experiment before branch point, as it is
> in
> a middle of dogfood and this issue is blocking for CrOS roll-out.
>
> https://codereview.chromium.org/578333003/

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

[yefimt, 2014-09-24 16:49:30]

+ asvitkine@ for finch related part

[Alexei Svitkine (slow), 2014-09-24 16:55:05]

asvitkine@chromium.org changed reviewers:
+ asvitkine@chromium.org

[Alexei Svitkine (slow), 2014-09-24 16:55:05]

I had a chat with Yefim and after understanding the issue, I don't think it
makes sense to generalize at this time.

Right now the FieldTrial system does not support changing state without a
restart and this was an explicit decision that needs a lot of discussion (in
addition to framework changes) to change.

Specifically, right now experiment groups are assigned on process start up,
which includes evaluating the Finch config and the base FieldTrial API doesn't
support (by design) experiments changing groups after the initial group has been
decided. Both of those would need to change if we were to support this.

But, enhanced bookmarks is currently the only experiment that's using Finch this
way (the approach they took is basically nonstandard for various reasons), so I
don't think it makes sense to try to generalize this solution at this time
(until we better understand if there's a broader demand for this sort of
experiment rollouts than just enhanced bookmarks).

[sky, 2014-09-24 17:22:10]

https://codereview.chromium.org/578333003/diff/1/chrome/browser/about_flags.cc
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/578333003/diff/1/chrome/browser/about_flags.c...
chrome/browser/about_flags.cc:2374: if (experiment_name ==
"manual-enhanced-bookmarks") {
nit: no {}
And add a description as to why this is special cased.

https://codereview.chromium.org/578333003/diff/1/chrome/browser/bookmarks/DEPS
File chrome/browser/bookmarks/DEPS (right):

https://codereview.chromium.org/578333003/diff/1/chrome/browser/bookmarks/DEP...
chrome/browser/bookmarks/DEPS:37: # Allow to include flags_storage into
enhanced_bookmarks_features.cc.
This just documents what you're doing, and isn't useful. Document why.

https://codereview.chromium.org/578333003/diff/1/chrome/browser/bookmarks/enh...
File chrome/browser/bookmarks/enhanced_bookmarks_features.cc (right):

https://codereview.chromium.org/578333003/diff/1/chrome/browser/bookmarks/enh...
chrome/browser/bookmarks/enhanced_bookmarks_features.cc:196: #if
defined(OS_CHROMEOS)
Again, add a comment.

[yefimt, 2014-09-24 18:46:12]

https://codereview.chromium.org/578333003/diff/1/chrome/browser/about_flags.cc
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/578333003/diff/1/chrome/browser/about_flags.c...
chrome/browser/about_flags.cc:2374: if (experiment_name ==
"manual-enhanced-bookmarks") {
On 2014/09/24 17:22:09, sky wrote:
> nit: no {}
> And add a description as to why this is special cased.

Done.

https://codereview.chromium.org/578333003/diff/1/chrome/browser/bookmarks/DEPS
File chrome/browser/bookmarks/DEPS (right):

https://codereview.chromium.org/578333003/diff/1/chrome/browser/bookmarks/DEP...
chrome/browser/bookmarks/DEPS:37: # Allow to include flags_storage into
enhanced_bookmarks_features.cc.
On 2014/09/24 17:22:09, sky wrote:
> This just documents what you're doing, and isn't useful. Document why.

Done.

https://codereview.chromium.org/578333003/diff/1/chrome/browser/bookmarks/enh...
File chrome/browser/bookmarks/enhanced_bookmarks_features.cc (right):

https://codereview.chromium.org/578333003/diff/1/chrome/browser/bookmarks/enh...
chrome/browser/bookmarks/enhanced_bookmarks_features.cc:196: #if
defined(OS_CHROMEOS)
On 2014/09/24 17:22:09, sky wrote:
> Again, add a comment.

Done.

[sky, 2014-09-24 19:52:53]

LGTM

[yefimt, 2014-09-24 19:53:44]

The CQ bit was checked by yefim@chromium.org

[commit-bot: I haz the power, 2014-09-24 19:54:15]

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

[commit-bot: I haz the power, 2014-09-24 20:20:00]

Committed patchset #2 (id:20001) as f117f6b0b7bac3887199088c45f56b0d324d6b34

[commit-bot: I haz the power, 2014-09-24 20:20:42]

Patchset 2 (id:??) landed as
https://crrev.com/ff0c52fb645f56ebdee15b256d73997184174257
Cr-Commit-Position: refs/heads/master@{#296504}