Make FeatureList override state be plumbed to renderer processes.

Make FeatureList override state be plumbed to renderer processes.

Previously, the browser copied over verbatim the --enable-features
and --disable-features command-line. However, this wouldn't have
captured overrides as a result of field trials. This change updates
the logic to include those in the list. The new logic will continue
to work for plumbing command-lines as well, since those are also
registered using the override mechanism.

Note: There's still some follow-up work that needs to be done to
plumb the FieldTrial association state (so that a FieldTrial that's
not active will get activated in the renderer if the associated
Feature is queried). But this can be supported in a follow-up CL
that builds on top of this one.

BUG=561077
Committed: https://crrev.com/86340191dcd4ad3ab7e6e4c7500ff9a5a8e9c3b0
Cr-Commit-Position: refs/heads/master@{#362294}

[Alexei Svitkine (slow), 2015-11-24 22:38:15]

Patchset #1 (id:1) has been deleted

[Alexei Svitkine (slow), 2015-11-25 18:36:14]

Patchset #2 (id:40001) has been deleted

[Alexei Svitkine (slow), 2015-11-25 18:36:31]

Patchset #1 (id:20001) has been deleted

[Alexei Svitkine (slow), 2015-11-25 18:36:44]

Patchset #1 (id:60001) has been deleted

[Alexei Svitkine (slow), 2015-11-25 18:36:49]

Description was changed from

==========
Make FeatureList override state be plumbed to renderer processes.

Previously, the browser copied over verbatim the --enable-features
and --disable-features command-line. However, this wouldn't have
captured overrides as a result of field trials. This change updates
the logic to include those in the list. The new logic will continue
to work for plumbing command-lines as well, since those are also
registered using the override mechanism.

BUG=
==========

to

==========
Make FeatureList override state be plumbed to renderer processes.

Previously, the browser copied over verbatim the --enable-features
and --disable-features command-line. However, this wouldn't have
captured overrides as a result of field trials. This change updates
the logic to include those in the list. The new logic will continue
to work for plumbing command-lines as well, since those are also
registered using the override mechanism.

BUG=561077
==========

[Alexei Svitkine (slow), 2015-11-25 18:37:50]

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

[Ilya Sherman, 2015-11-25 22:45:16]

Hmm, suppose that a field trial override is registered, but it's only queried
from the renderer process.  Will we correctly detect that the field trial was
queried, and should be reported as active for UMA uploads?

[Alexei Svitkine (slow), 2015-11-26 16:12:55]

On 2015/11/25 22:45:16, Ilya Sherman wrote:
> Hmm, suppose that a field trial override is registered, but it's only queried
> from the renderer process.  Will we correctly detect that the field trial was
> queried, and should be reported as active for UMA uploads?

You're right that this CL doesn't address this. This only propagates behavior
change, but does not correctly attribute activation (it has to be done manually
by querying the field trial at the time they query the feature in the renderer).
This is not ideal, but at least with this CL, it can be worked around and the
behavior change still propagates.

I thought of this problem first, but thought it might be better to split the
fixes into several CLs. After this CL lands, then we can add the missing field
trial association support by expanding the command-line format to have this
information - thus changing what's returned by GetFeatureOverrides() to include
the associated field trial name and updating the parsing code to match. I think
doing it in a follow-up CL makes it a bit cleaner. Are you ok with that?

[Ilya Sherman, 2015-11-26 20:41:53]

On 2015/11/26 16:12:55, Alexei Svitkine (slow) wrote:
> On 2015/11/25 22:45:16, Ilya Sherman wrote:
> > Hmm, suppose that a field trial override is registered, but it's only
queried
> > from the renderer process.  Will we correctly detect that the field trial
was
> > queried, and should be reported as active for UMA uploads?
> 
> You're right that this CL doesn't address this. This only propagates behavior
> change, but does not correctly attribute activation (it has to be done
manually
> by querying the field trial at the time they query the feature in the
renderer).
> This is not ideal, but at least with this CL, it can be worked around and the
> behavior change still propagates.
> 
> I thought of this problem first, but thought it might be better to split the
> fixes into several CLs. After this CL lands, then we can add the missing field
> trial association support by expanding the command-line format to have this
> information - thus changing what's returned by GetFeatureOverrides() to
include
> the associated field trial name and updating the parsing code to match. I
think
> doing it in a follow-up CL makes it a bit cleaner. Are you ok with that?

I'm ok with that if you prefer it.  I'd definitely mention this KI in the CL
description, though.  LGTM with that done.

[Alexei Svitkine (slow), 2015-11-26 20:44:29]

Description was changed from

==========
Make FeatureList override state be plumbed to renderer processes.

Previously, the browser copied over verbatim the --enable-features
and --disable-features command-line. However, this wouldn't have
captured overrides as a result of field trials. This change updates
the logic to include those in the list. The new logic will continue
to work for plumbing command-lines as well, since those are also
registered using the override mechanism.

BUG=561077
==========

to

==========
Make FeatureList override state be plumbed to renderer processes.

Previously, the browser copied over verbatim the --enable-features
and --disable-features command-line. However, this wouldn't have
captured overrides as a result of field trials. This change updates
the logic to include those in the list. The new logic will continue
to work for plumbing command-lines as well, since those are also
registered using the override mechanism.

Note: There's still some follow-up work that needs to be done to
plumb the FieldTrial association state (so that a FieldTrial that's
not active will get activated in the renderer if the associated
Feature is queried). But this can be supported in a follow-up CL
that builds on top of this one. 

BUG=561077
==========

[Alexei Svitkine (slow), 2015-11-26 20:44:45]

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

[Alexei Svitkine (slow), 2015-11-26 20:44:45]

Done.

+jam for content/ OWNERS

[jam, 2015-11-30 21:18:54]

why isn't this done for all child processes, i.e. in
browser_child_process_host_impl.cc as well?

[Alexei Svitkine (slow), 2015-11-30 21:22:15]

On 2015/11/30 21:18:54, jam wrote:
> why isn't this done for all child processes, i.e. in
> browser_child_process_host_impl.cc as well?

Currently, the code to parse this is in renderer_main.cc. I think it's worth
enabling it for all child processes, though I think that would be better done in
a separate CL. Thanks for the pointer to the other file.

[Alexei Svitkine (slow), 2015-11-30 21:25:10]

File http://crbug.com/563705 to do it for all child processes. My preference is
to do that in a separate CL.

[jam, 2015-11-30 21:33:02]

On 2015/11/30 21:25:10, Alexei Svitkine (slow) wrote:
> File http://crbug.com/563705 to do it for all child processes. My preference
is
> to do that in a separate CL.

ok. lgtm

[Alexei Svitkine (slow), 2015-11-30 21:35:40]

Thanks!

[Alexei Svitkine (slow), 2015-11-30 21:35:45]

The CQ bit was checked by asvitkine@chromium.org

[commit-bot: I haz the power, 2015-11-30 21:39:54]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1471693007/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1471693007/100001

[commit-bot: I haz the power, 2015-12-01 00:45:38]

Description was changed from

==========
Make FeatureList override state be plumbed to renderer processes.

Previously, the browser copied over verbatim the --enable-features
and --disable-features command-line. However, this wouldn't have
captured overrides as a result of field trials. This change updates
the logic to include those in the list. The new logic will continue
to work for plumbing command-lines as well, since those are also
registered using the override mechanism.

Note: There's still some follow-up work that needs to be done to
plumb the FieldTrial association state (so that a FieldTrial that's
not active will get activated in the renderer if the associated
Feature is queried). But this can be supported in a follow-up CL
that builds on top of this one. 

BUG=561077
==========

to

==========
Make FeatureList override state be plumbed to renderer processes.

Previously, the browser copied over verbatim the --enable-features
and --disable-features command-line. However, this wouldn't have
captured overrides as a result of field trials. This change updates
the logic to include those in the list. The new logic will continue
to work for plumbing command-lines as well, since those are also
registered using the override mechanism.

Note: There's still some follow-up work that needs to be done to
plumb the FieldTrial association state (so that a FieldTrial that's
not active will get activated in the renderer if the associated
Feature is queried). But this can be supported in a follow-up CL
that builds on top of this one. 

BUG=561077
==========

[commit-bot: I haz the power, 2015-12-01 00:45:39]

Committed patchset #2 (id:100001)

[commit-bot: I haz the power, 2015-12-01 00:47:14]

Description was changed from

==========
Make FeatureList override state be plumbed to renderer processes.

Previously, the browser copied over verbatim the --enable-features
and --disable-features command-line. However, this wouldn't have
captured overrides as a result of field trials. This change updates
the logic to include those in the list. The new logic will continue
to work for plumbing command-lines as well, since those are also
registered using the override mechanism.

Note: There's still some follow-up work that needs to be done to
plumb the FieldTrial association state (so that a FieldTrial that's
not active will get activated in the renderer if the associated
Feature is queried). But this can be supported in a follow-up CL
that builds on top of this one. 

BUG=561077
==========

to

==========
Make FeatureList override state be plumbed to renderer processes.

Previously, the browser copied over verbatim the --enable-features
and --disable-features command-line. However, this wouldn't have
captured overrides as a result of field trials. This change updates
the logic to include those in the list. The new logic will continue
to work for plumbing command-lines as well, since those are also
registered using the override mechanism.

Note: There's still some follow-up work that needs to be done to
plumb the FieldTrial association state (so that a FieldTrial that's
not active will get activated in the renderer if the associated
Feature is queried). But this can be supported in a follow-up CL
that builds on top of this one.

BUG=561077

Committed: https://crrev.com/86340191dcd4ad3ab7e6e4c7500ff9a5a8e9c3b0
Cr-Commit-Position: refs/heads/master@{#362294}
==========

[commit-bot: I haz the power, 2015-12-01 00:47:15]

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