Fix perfbotnot feature param setting from trial testing config.

Fix perfbotnot feature param setting from trial testing config.

Previously, params that were queried via their associated features
would not get correctly picked by perfbots using the field trial
testing config.

This was caused by the generated command-line not using the
--enable-features=FeatureName<TrialName syntax. This syntax is
needed in order to estable the link between a feature and its
associated trial, so that when a field-trial param is looked up
by feature, it's able to find it from the associated trial.

Adds quotations to these params since "<" characters would
otherwise get interpreted by some shells.

PERF SHERIFFS: This CL may cause perf bots to move - if so, it
means that one of the entries in the field trial testing config
that uses both params and features is causing the change but was
previously not being correctly tested. If so, we will need to
narrow down which one.

BUG=706414
Review-Url: https://codereview.chromium.org/2778313003
Cr-Commit-Position: refs/heads/master@{#460498}
Committed: https://chromium.googlesource.com/chromium/src/+/d47c7180be6c7531200d034d36c470fb5980f0e4

[Alexei Svitkine (slow), 2017-03-29 18:29:00]

The CQ bit was checked by asvitkine@chromium.org to run a CQ dry run

[Alexei Svitkine (slow), 2017-03-29 18:29:13]

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

[commit-bot: I haz the power, 2017-03-29 18:29:40]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Alexei Svitkine (slow), 2017-03-29 18:56:56]

Description was changed from

==========
Fix perfbotnot feature param setting from trial testing config.

Previously, params that were queried via their associated features
would not get correctly picked by perfbots using the field trial
testing config.

This was caused by the generated command-line not using the
--enable-features=FeatureName<TrialName syntax. This syntax is
needed in order to estable the link between a feature and its
associated trial, so that when a field-trial param is looked up
by feature, it's able to find it from the associated trial.

Adds quotations to these params since "<" characters would
otherwise get interpreted by some shells.

Note: This CL may cause perf bots to move - if so, it means that
one of the entries in the field trial testing config that uses
both params and features is causing the change but was previously
not being correctly tested. If so, we will need to narrow down
which one.

BUG=706414
==========

to

==========
Fix perfbotnot feature param setting from trial testing config.

Previously, params that were queried via their associated features
would not get correctly picked by perfbots using the field trial
testing config.

This was caused by the generated command-line not using the
--enable-features=FeatureName<TrialName syntax. This syntax is
needed in order to estable the link between a feature and its
associated trial, so that when a field-trial param is looked up
by feature, it's able to find it from the associated trial.

Adds quotations to these params since "<" characters would
otherwise get interpreted by some shells.

PERF SHEERIFFS: This CL may cause perf bots to move - if so, it
means that one of the entries in the field trial testing config that uses
both params and features is causing the change but was previously
not being correctly tested. If so, we will need to narrow down
which one.

BUG=706414
==========

[Alexei Svitkine (slow), 2017-03-29 18:57:20]

Description was changed from

==========
Fix perfbotnot feature param setting from trial testing config.

Previously, params that were queried via their associated features
would not get correctly picked by perfbots using the field trial
testing config.

This was caused by the generated command-line not using the
--enable-features=FeatureName<TrialName syntax. This syntax is
needed in order to estable the link between a feature and its
associated trial, so that when a field-trial param is looked up
by feature, it's able to find it from the associated trial.

Adds quotations to these params since "<" characters would
otherwise get interpreted by some shells.

PERF SHEERIFFS: This CL may cause perf bots to move - if so, it
means that one of the entries in the field trial testing config that uses
both params and features is causing the change but was previously
not being correctly tested. If so, we will need to narrow down
which one.

BUG=706414
==========

to

==========
Fix perfbotnot feature param setting from trial testing config.

Previously, params that were queried via their associated features
would not get correctly picked by perfbots using the field trial
testing config.

This was caused by the generated command-line not using the
--enable-features=FeatureName<TrialName syntax. This syntax is
needed in order to estable the link between a feature and its
associated trial, so that when a field-trial param is looked up
by feature, it's able to find it from the associated trial.

Adds quotations to these params since "<" characters would
otherwise get interpreted by some shells.

PERF SHERIFFS: This CL may cause perf bots to move - if so, it
means that one of the entries in the field trial testing config that
uses both params and features is causing the change but was
previously not being correctly tested. If so, we will need to narrow
down which one.

BUG=706414
==========

[Alexei Svitkine (slow), 2017-03-29 18:57:41]

Description was changed from

==========
Fix perfbotnot feature param setting from trial testing config.

Previously, params that were queried via their associated features
would not get correctly picked by perfbots using the field trial
testing config.

This was caused by the generated command-line not using the
--enable-features=FeatureName<TrialName syntax. This syntax is
needed in order to estable the link between a feature and its
associated trial, so that when a field-trial param is looked up
by feature, it's able to find it from the associated trial.

Adds quotations to these params since "<" characters would
otherwise get interpreted by some shells.

PERF SHERIFFS: This CL may cause perf bots to move - if so, it
means that one of the entries in the field trial testing config that
uses both params and features is causing the change but was
previously not being correctly tested. If so, we will need to narrow
down which one.

BUG=706414
==========

to

==========
Fix perfbotnot feature param setting from trial testing config.

Previously, params that were queried via their associated features
would not get correctly picked by perfbots using the field trial
testing config.

This was caused by the generated command-line not using the
--enable-features=FeatureName<TrialName syntax. This syntax is
needed in order to estable the link between a feature and its
associated trial, so that when a field-trial param is looked up
by feature, it's able to find it from the associated trial.

Adds quotations to these params since "<" characters would
otherwise get interpreted by some shells.

PERF SHERIFFS: This CL may cause perf bots to move - if so, it
means that one of the entries in the field trial testing config
that uses both params and features is causing the change but
was previously not being correctly tested. If so, we will need to
narrow down which one.

BUG=706414
==========

[Alexei Svitkine (slow), 2017-03-29 18:57:57]

Description was changed from

==========
Fix perfbotnot feature param setting from trial testing config.

Previously, params that were queried via their associated features
would not get correctly picked by perfbots using the field trial
testing config.

This was caused by the generated command-line not using the
--enable-features=FeatureName<TrialName syntax. This syntax is
needed in order to estable the link between a feature and its
associated trial, so that when a field-trial param is looked up
by feature, it's able to find it from the associated trial.

Adds quotations to these params since "<" characters would
otherwise get interpreted by some shells.

PERF SHERIFFS: This CL may cause perf bots to move - if so, it
means that one of the entries in the field trial testing config
that uses both params and features is causing the change but
was previously not being correctly tested. If so, we will need to
narrow down which one.

BUG=706414
==========

to

==========
Fix perfbotnot feature param setting from trial testing config.

Previously, params that were queried via their associated features
would not get correctly picked by perfbots using the field trial
testing config.

This was caused by the generated command-line not using the
--enable-features=FeatureName<TrialName syntax. This syntax is
needed in order to estable the link between a feature and its
associated trial, so that when a field-trial param is looked up
by feature, it's able to find it from the associated trial.

Adds quotations to these params since "<" characters would
otherwise get interpreted by some shells.

PERF SHERIFFS: This CL may cause perf bots to move - if so, it
means that one of the entries in the field trial testing config
that uses both params and features is causing the change but was
previously not being correctly tested. If so, we will need to
narrow down which one.

BUG=706414
==========

[jwd, 2017-03-29 19:26:09]

lgtm

[commit-bot: I haz the power, 2017-03-29 19:37:14]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-29 19:37:15]

Dry run: This issue passed the CQ dry run.

[Alexei Svitkine (slow), 2017-03-29 19:37:38]

The CQ bit was checked by asvitkine@chromium.org

[commit-bot: I haz the power, 2017-03-29 19:38:40]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-29 19:45:02]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1490816258782560, "parent_rev":
"5135ba101f2a688c8b90f9df43df5abca4279a54", "commit_rev":
"d47c7180be6c7531200d034d36c470fb5980f0e4"}

[commit-bot: I haz the power, 2017-03-29 19:45:53]

Description was changed from

==========
Fix perfbotnot feature param setting from trial testing config.

Previously, params that were queried via their associated features
would not get correctly picked by perfbots using the field trial
testing config.

This was caused by the generated command-line not using the
--enable-features=FeatureName<TrialName syntax. This syntax is
needed in order to estable the link between a feature and its
associated trial, so that when a field-trial param is looked up
by feature, it's able to find it from the associated trial.

Adds quotations to these params since "<" characters would
otherwise get interpreted by some shells.

PERF SHERIFFS: This CL may cause perf bots to move - if so, it
means that one of the entries in the field trial testing config
that uses both params and features is causing the change but was
previously not being correctly tested. If so, we will need to
narrow down which one.

BUG=706414
==========

to

==========
Fix perfbotnot feature param setting from trial testing config.

Previously, params that were queried via their associated features
would not get correctly picked by perfbots using the field trial
testing config.

This was caused by the generated command-line not using the
--enable-features=FeatureName<TrialName syntax. This syntax is
needed in order to estable the link between a feature and its
associated trial, so that when a field-trial param is looked up
by feature, it's able to find it from the associated trial.

Adds quotations to these params since "<" characters would
otherwise get interpreted by some shells.

PERF SHERIFFS: This CL may cause perf bots to move - if so, it
means that one of the entries in the field trial testing config
that uses both params and features is causing the change but was
previously not being correctly tested. If so, we will need to
narrow down which one.

BUG=706414

Review-Url: https://codereview.chromium.org/2778313003
Cr-Commit-Position: refs/heads/master@{#460498}
Committed:
https://chromium.googlesource.com/chromium/src/+/d47c7180be6c7531200d034d36c4...
==========

[commit-bot: I haz the power, 2017-03-29 19:46:02]

Committed patchset #1 (id:1) as
https://chromium.googlesource.com/chromium/src/+/d47c7180be6c7531200d034d36c4...

[perezju, 2017-03-30 08:16:05]

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

[perezju, 2017-03-30 08:16:06]

https://codereview.chromium.org/2778313003/diff/1/tools/variations/fieldtrial...
File tools/variations/fieldtrial_util_unittest.py (right):

https://codereview.chromium.org/2778313003/diff/1/tools/variations/fieldtrial...
tools/variations/fieldtrial_util_unittest.py:70: '--disable-features="y<c"'],
result)
Are we sure that those quotes are needed there?

The command line flags are not passed through the shell, so there should be no
need of any special quoting.

[Alexei Svitkine (slow), 2017-03-30 14:28:30]

https://codereview.chromium.org/2778313003/diff/1/tools/variations/fieldtrial...
File tools/variations/fieldtrial_util_unittest.py (right):

https://codereview.chromium.org/2778313003/diff/1/tools/variations/fieldtrial...
tools/variations/fieldtrial_util_unittest.py:70: '--disable-features="y<c"'],
result)
On 2017/03/30 08:16:06, perezju wrote:
> Are we sure that those quotes are needed there?
> 
> The command line flags are not passed through the shell, so there should be no
> need of any special quoting.

You're right that it might not be necessary, though I think it should work
either way, no?

[perezju, 2017-03-30 14:57:44]

On 2017/03/30 14:28:30, Alexei Svitkine (slow) wrote:
>
https://codereview.chromium.org/2778313003/diff/1/tools/variations/fieldtrial...
> File tools/variations/fieldtrial_util_unittest.py (right):
> 
>
https://codereview.chromium.org/2778313003/diff/1/tools/variations/fieldtrial...
> tools/variations/fieldtrial_util_unittest.py:70: '--disable-features="y<c"'],
> result)
> On 2017/03/30 08:16:06, perezju wrote:
> > Are we sure that those quotes are needed there?
> > 
> > The command line flags are not passed through the shell, so there should be
no
> > need of any special quoting.
> 
> You're right that it might not be necessary, though I think it should work
> either way, no?

It depends on what Chrome does while parsing those args, I'm not familiar with
that code. I have the suspicion that this may not work on Android. I suggest to
double check whether Chrome is understanding these correctly on the bots.

[Alexei Svitkine (slow), 2017-03-30 15:05:59]

Looking at the relevant code (
https://cs.chromium.org/chromium/src/base/command_line.cc?rcl=9a50f192a882b65...)
it doesn't look like there's special handling of unquoting things. So seems
like you're right - will send a CL to fix.

On Thu, Mar 30, 2017 at 10:57 AM, <perezju@chromium.org> wrote:

> On 2017/03/30 14:28:30, Alexei Svitkine (slow) wrote:
> >
> https://codereview.chromium.org/2778313003/diff/1/tools/
> variations/fieldtrial_util_unittest.py
> > File tools/variations/fieldtrial_util_unittest.py (right):
> >
> >
> https://codereview.chromium.org/2778313003/diff/1/tools/
> variations/fieldtrial_util_unittest.py#newcode70
> > tools/variations/fieldtrial_util_unittest.py:70:
> '--disable-features="y<c"'],
> > result)
> > On 2017/03/30 08:16:06, perezju wrote:
> > > Are we sure that those quotes are needed there?
> > >
> > > The command line flags are not passed through the shell, so there
> should be
> no
> > > need of any special quoting.
> >
> > You're right that it might not be necessary, though I think it should
> work
> > either way, no?
>
> It depends on what Chrome does while parsing those args, I'm not familiar
> with
> that code. I have the suspicion that this may not work on Android. I
> suggest to
> double check whether Chrome is understanding these correctly on the bots.
>
> https://codereview.chromium.org/2778313003/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Alexei Svitkine (slow), 2017-03-30 15:17:54]

Sent https://codereview.chromium.org/2789583002/ to fix. Thanks for
catching!

On Thu, Mar 30, 2017 at 11:05 AM, Alexei Svitkine <asvitkine@chromium.org>
wrote:

> Looking at the relevant code (https://cs.chromium.org/
> chromium/src/base/command_line.cc?rcl=9a50f192a882b65f021c0c13345762
> c3a4d7a499&l=57) it doesn't look like there's special handling of
> unquoting things. So seems like you're right - will send a CL to fix.
>
> On Thu, Mar 30, 2017 at 10:57 AM, <perezju@chromium.org> wrote:
>
>> On 2017/03/30 14:28:30, Alexei Svitkine (slow) wrote:
>> >
>> https://codereview.chromium.org/2778313003/diff/1/tools/vari
>> ations/fieldtrial_util_unittest.py
>> > File tools/variations/fieldtrial_util_unittest.py (right):
>> >
>> >
>> https://codereview.chromium.org/2778313003/diff/1/tools/vari
>> ations/fieldtrial_util_unittest.py#newcode70
>> > tools/variations/fieldtrial_util_unittest.py:70:
>> '--disable-features="y<c"'],
>> > result)
>> > On 2017/03/30 08:16:06, perezju wrote:
>> > > Are we sure that those quotes are needed there?
>> > >
>> > > The command line flags are not passed through the shell, so there
>> should be
>> no
>> > > need of any special quoting.
>> >
>> > You're right that it might not be necessary, though I think it should
>> work
>> > either way, no?
>>
>> It depends on what Chrome does while parsing those args, I'm not familiar
>> with
>> that code. I have the suspicion that this may not work on Android. I
>> suggest to
>> double check whether Chrome is understanding these correctly on the bots.
>>
>> https://codereview.chromium.org/2778313003/
>>
>
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[perezju, 2017-03-30 15:23:36]

On 2017/03/30 15:17:54, Alexei Svitkine (slow) wrote:
> Sent https://codereview.chromium.org/2789583002/ to fix. Thanks for
> catching!
> 

np! I recently re-wrote the flag changer we use to set flags Android, so I had
this kind of pitfalls recent in mind.

[Alexei Svitkine (slow), 2017-05-02 21:29:48]

A revert of this CL (patchset #1 id:1) has been created in
https://codereview.chromium.org/2861493002/ by asvitkine@chromium.org.

The reason for reverting is: Creating revert CL for testing..