Create a field trial for Seccomp-BPF on Android.

Create a field trial for Seccomp-BPF on Android.

BUG=477049
Committed: https://crrev.com/a8dcaa96573ff917fe9347609855241224824e1f
Cr-Commit-Position: refs/heads/master@{#361136}

[Robert Sesek, 2015-11-06 21:57:25]

rsesek@chromium.org changed reviewers:
+ gayane@chromium.org

[Alexei Svitkine (slow), 2015-11-09 16:51:42]

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

[Alexei Svitkine (slow), 2015-11-09 16:51:43]

Gayane pointed me at this CL and mentioned you were asking about FeatureList.

I think you should definitely be able to use FeatureList here instead of using
the CommandLine and FieldTrial APIs.

I would imagine the code would be something like:

if (IsSeccompBpfSupportedBySDK() &&
base::FeatureList::IsEnabled(kSeccompBpfFeature)) {
}

The FeatureList check would replace the command-line and FieldTrial API uses. As
part of changing to FeatureList, you can also remove the explicit
--enable/--disable flags for this, since Features can be enabled via
--enable-features=Name and --disable-features=Name syntax. (You can change the
about_flags.cc entry if you have one to be associated with the Feature which
integrates with those flags.)

[gayane -on leave until 09-2017, 2015-11-09 16:53:17]

Description was changed from

==========
Create a field trial for Seccomp-BPF on Android.

BUG=477049
==========

to

==========
Create a field trial for Seccomp-BPF on Android.

BUG=477049
==========

[gayane -on leave until 09-2017, 2015-11-09 16:53:17]

gayane@chromium.org changed reviewers:
- gayane@chromium.org

[Robert Sesek, 2015-11-09 21:26:12]

On 2015/11/09 16:51:43, Alexei Svitkine (slow) wrote:
> The FeatureList check would replace the command-line and FieldTrial API uses.
As
> part of changing to FeatureList, you can also remove the explicit
> --enable/--disable flags for this, since Features can be enabled via
> --enable-features=Name and --disable-features=Name syntax. (You can change the
> about_flags.cc entry if you have one to be associated with the Feature which
> integrates with those flags.)

Can you elaborate on this bit a little more? On Android, it's preferable to be
in about:flags, since you can't set CLI args unless your device is rooted (and
this feature is one that should be tested on a user/production build). If
there's a way to use FeatureList and still have a dedicated entry in about:flags
for enabling this, that's what I'd like.

[Alexei Svitkine (slow), 2015-11-09 21:46:04]

There definitely is! Use the FEATURE_VALUE_TYPE() macro in about_flags.cc
and pass in your feature.

On Mon, Nov 9, 2015 at 4:26 PM, <rsesek@chromium.org> wrote:

> On 2015/11/09 16:51:43, Alexei Svitkine (slow) wrote:
>
>> The FeatureList check would replace the command-line and FieldTrial API
>> uses.
>>
> As
>
>> part of changing to FeatureList, you can also remove the explicit
>> --enable/--disable flags for this, since Features can be enabled via
>> --enable-features=Name and --disable-features=Name syntax. (You can
>> change the
>> about_flags.cc entry if you have one to be associated with the Feature
>> which
>> integrates with those flags.)
>>
>
> Can you elaborate on this bit a little more? On Android, it's preferable
> to be
> in about:flags, since you can't set CLI args unless your device is rooted
> (and
> this feature is one that should be tested on a user/production build). If
> there's a way to use FeatureList and still have a dedicated entry in
> about:flags
> for enabling this, that's what I'd like.
>
> https://codereview.chromium.org/1419083012/
>

-- 
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), 2015-11-12 23:22:29]

LGTM, sorry for the delay - didn't realise you made the about_flags.cc changes
in the new patchset.

[Robert Sesek, 2015-11-13 02:36:50]

On 2015/11/12 23:22:29, Alexei Svitkine (slow) wrote:
> LGTM, sorry for the delay - didn't realise you made the about_flags.cc changes
> in the new patchset.

That's because I didn't send it. Thanks for the review, though!

[Robert Sesek, 2015-11-13 02:37:16]

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

[Robert Sesek, 2015-11-13 02:37:17]

+jam for //content OWNERS

[jam, 2015-11-13 17:20:17]

https://codereview.chromium.org/1419083012/diff/40001/content/public/common/s...
File content/public/common/sandbox_init.h (right):

https://codereview.chromium.org/1419083012/diff/40001/content/public/common/s...
content/public/common/sandbox_init.h:122: CONTENT_EXPORT extern const
base::Feature kSeccompSandboxAndroidFeature;
hmm, as we convert switches to base::Feature, seems like they should go into the
switches file still? i.e. otherwise where will we end up putting all the various
switches? (also seems we want them in one place like switches)

[Alexei Svitkine (slow), 2015-11-13 17:33:02]

https://codereview.chromium.org/1419083012/diff/40001/content/public/common/s...
File content/public/common/sandbox_init.h (right):

https://codereview.chromium.org/1419083012/diff/40001/content/public/common/s...
content/public/common/sandbox_init.h:122: CONTENT_EXPORT extern const
base::Feature kSeccompSandboxAndroidFeature;
On 2015/11/13 17:20:17, jam wrote:
> hmm, as we convert switches to base::Feature, seems like they should go into
the
> switches file still? i.e. otherwise where will we end up putting all the
various
> switches? (also seems we want them in one place like switches)

I am okay with a centralized place for features at each layer (e.g. content/,
chrome/), although some may live in components and therefore may need to be
decentralized. (The base API allows either.)

One idea is to add a new file - e.g. content_features.cc/h similar to
content_switches and same for chrome/. Or do you prefer re-using
content_switches? WDYT?

[Robert Sesek, 2015-11-14 01:16:54]

Patchset #4 (id:60001) has been deleted

[Robert Sesek, 2015-11-14 01:18:03]

https://codereview.chromium.org/1419083012/diff/40001/content/public/common/s...
File content/public/common/sandbox_init.h (right):

https://codereview.chromium.org/1419083012/diff/40001/content/public/common/s...
content/public/common/sandbox_init.h:122: CONTENT_EXPORT extern const
base::Feature kSeccompSandboxAndroidFeature;
On 2015/11/13 17:33:02, Alexei Svitkine (slow) wrote:
> On 2015/11/13 17:20:17, jam wrote:
> > hmm, as we convert switches to base::Feature, seems like they should go into
> the
> > switches file still? i.e. otherwise where will we end up putting all the
> various
> > switches? (also seems we want them in one place like switches)
> 
> I am okay with a centralized place for features at each layer (e.g. content/,
> chrome/), although some may live in components and therefore may need to be
> decentralized. (The base API allows either.)
> 
> One idea is to add a new file - e.g. content_features.cc/h similar to
> content_switches and same for chrome/. Or do you prefer re-using
> content_switches? WDYT?

I agree that it'd be good to centralize these. I added content_features to do so
(it doesn't seem like switches is the right place for these).

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

lgtm

[jam, 2015-11-16 22:19:32]

lgtm

https://codereview.chromium.org/1419083012/diff/80001/content/public/common/c...
File content/public/common/content_features.h (right):

https://codereview.chromium.org/1419083012/diff/80001/content/public/common/c...
content/public/common/content_features.h:5: // This file defines all the
base::FeatureList features for the content module.
nit: add "public base::FeatureList"

then once we have internal features in content, we can create
content/common/content_features_internal.h

[Robert Sesek, 2015-11-21 03:32:47]

https://codereview.chromium.org/1419083012/diff/80001/content/public/common/c...
File content/public/common/content_features.h (right):

https://codereview.chromium.org/1419083012/diff/80001/content/public/common/c...
content/public/common/content_features.h:5: // This file defines all the
base::FeatureList features for the content module.
On 2015/11/16 22:19:32, jam wrote:
> nit: add "public base::FeatureList"
> 
> then once we have internal features in content, we can create
> content/common/content_features_internal.h

Done.

[Robert Sesek, 2015-11-21 03:32:52]

The CQ bit was checked by rsesek@chromium.org

[Robert Sesek, 2015-11-21 03:32:53]

The patchset sent to the CQ was uploaded after l-g-t-m from
asvitkine@chromium.org, jam@chromium.org
Link to the patchset: https://codereview.chromium.org/1419083012/#ps100001
(title: "nit")

[commit-bot: I haz the power, 2015-11-21 03:33:29]

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

[commit-bot: I haz the power, 2015-11-21 04:04:34]

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

[commit-bot: I haz the power, 2015-11-21 04:04:35]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Robert Sesek, 2015-11-23 14:46:47]

The CQ bit was checked by rsesek@chromium.org

[commit-bot: I haz the power, 2015-11-23 14:47:19]

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

[commit-bot: I haz the power, 2015-11-23 17:55:16]

Committed patchset #5 (id:100001)

[commit-bot: I haz the power, 2015-11-23 17:56:04]

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