Integrate WebUSB with Feature Policy

Integrate WebUSB with Feature Policy

This patch uses the new Feature Policy framework to control access to           
WebUSB from embedded frames. When Feature Policy is available WebUSB's          
default policy ["self"] is enforced. When it isn't available WebUSB is          
simply disallowed in embedded frames.

A follow-up patch will remove the parser for the "allowed origins"
descriptor as they are no longer used and have been removed from the
specification.

BUG=711443
Review-Url: https://codereview.chromium.org/2815003005
Cr-Commit-Position: refs/heads/master@{#471487}
Committed: https://chromium.googlesource.com/chromium/src/+/aee98b5c7aebe3c263f2150357d36901efbf0d53

[Reilly Grant (use Gerrit), 2017-04-13 01:20:30]

Description was changed from

==========
Use Feature Policy to control access to WebUSB in iframes

The updates Chrome to use Feature Policy to control access to WebUSB
from iframes as was recently added to the specification in this pull
request:

https://github.com/WICG/webusb/pull/86

Allowed Origins descriptors have been removed from the specification and
so the code to parse and interpret them is also removed.

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

to

==========
Use Feature Policy to control access to WebUSB in iframes

The updates Chrome to use Feature Policy to control access to WebUSB
from iframes as was recently added to the specification in this pull
request:

https://github.com/WICG/webusb/pull/86

Allowed Origins descriptors have been removed from the specification and
so the code to parse and interpret them is also removed.

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[Reilly Grant (use Gerrit), 2017-04-13 01:20:31]

Description was changed from

==========
Use Feature Policy to control access to WebUSB in iframes

The updates Chrome to use Feature Policy to control access to WebUSB
from iframes as was recently added to the specification in this pull
request:

https://github.com/WICG/webusb/pull/86

Allowed Origins descriptors have been removed from the specification and
so the code to parse and interpret them is also removed.

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Use Feature Policy to control access to WebUSB in iframes

The updates Chrome to use Feature Policy to control access to WebUSB
from iframes as was recently added to the specification in this pull
request:

https://github.com/WICG/webusb/pull/86

Allowed Origins descriptors have been removed from the specification and
so the code to parse and interpret them is also removed.

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation;master.tryserver.chromium.linux:linux_site_isolation
==========

[Reilly Grant (use Gerrit), 2017-04-14 21:46:04]

rebased

[Reilly Grant (use Gerrit), 2017-05-01 22:51:59]

Description was changed from

==========
Use Feature Policy to control access to WebUSB in iframes

The updates Chrome to use Feature Policy to control access to WebUSB
from iframes as was recently added to the specification in this pull
request:

https://github.com/WICG/webusb/pull/86

Allowed Origins descriptors have been removed from the specification and
so the code to parse and interpret them is also removed.

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation;master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Integrate WebUSB with Feature Policy

This patch uses the new Feature Policy framework to control access to           
WebUSB from embedded frames. When Feature Policy is available WebUSB's          
default policy ["self"] is enforced. When it isn't available WebUSB is          
simply disallowed in embedded frames.

A follow-up patch will remove the parser for the "allowed origins"
descriptor as they are no longer used and have been removed from the
specification.

BUG=711443
==========

[Reilly Grant (use Gerrit), 2017-05-01 22:51:59]

reillyg@chromium.org changed reviewers:
+ iclelland@chromium.org

[Reilly Grant (use Gerrit), 2017-05-01 23:06:24]

Ready for review

[Reilly Grant (use Gerrit), 2017-05-01 23:08:29]

reillyg@chromium.org changed reviewers:
+ rbyers@chromium.org

[Reilly Grant (use Gerrit), 2017-05-01 23:10:33]

iclelland@, please do a general review.
rbyers@, please do an API_OWNERS review.

[Reilly Grant (use Gerrit), 2017-05-01 23:10:41]

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

[commit-bot: I haz the power, 2017-05-01 23:11:59]

Dry run: 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-05-02 01:23:44]

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

[commit-bot: I haz the power, 2017-05-02 01:23:45]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Rick Byers, 2017-05-02 13:26:56]

On 2017/05/01 23:10:33, Reilly Grant wrote:
> iclelland@, please do a general review.
> rbyers@, please do an API_OWNERS review.

There's nothing required for API_OWNERS here (since you're not yet shipping a
new API).  But I've looked over all the WebKit code and it seems reasonable at a
high-level to me, so I can give a WebKit OWNERS stamp once iclelland has
reviewed the feature policy details.

[Reilly Grant (use Gerrit), 2017-05-02 14:33:25]

On 2017/05/02 13:26:56, Rick Byers wrote:
> On 2017/05/01 23:10:33, Reilly Grant wrote:
> > iclelland@, please do a general review.
> > rbyers@, please do an API_OWNERS review.
> 
> There's nothing required for API_OWNERS here (since you're not yet shipping a
> new API).  But I've looked over all the WebKit code and it seems reasonable at
a
> high-level to me, so I can give a WebKit OWNERS stamp once iclelland has
> reviewed the feature policy details.

Thanks for taking a look. RuntimeEnabledFeatures.json5 is owned by API_OWNERS,
which is what I meant here.

[iclelland.google, 2017-05-02 14:42:51]

iclelland@google.com changed reviewers:
+ iclelland@google.com

[iclelland.google, 2017-05-02 14:42:52]

FP integration looks pretty good -- I can see some opportunities to change the
API to make it easier (there's a lot of "If FP Enabled { If Feature Enabled }
else { if main frame }" going on that we could probably extract into a common
pattern, but it's okay here for now.

https://codereview.chromium.org/2815003005/diff/40001/chrome/browser/usb/usb_...
File chrome/browser/usb/usb_tab_helper.cc (right):

https://codereview.chromium.org/2815003005/diff/40001/chrome/browser/usb/usb_...
chrome/browser/usb/usb_tab_helper.cc:66: if
(base::FeatureList::IsEnabled(features::kFeaturePolicy)) {
Could this block, and the identical one below, be extracted into a single
boolean "is USB enabled in RFH" method?

https://codereview.chromium.org/2815003005/diff/40001/device/usb/mojo/device_...
File device/usb/mojo/device_impl.cc (right):

https://codereview.chromium.org/2815003005/diff/40001/device/usb/mojo/device_...
device/usb/mojo/device_impl.cc:167: return true;
Drive-by: I'm not really looking at this part of the CL, but all of these
'return true' statements look redundant.

https://codereview.chromium.org/2815003005/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/external/wpt/webusb/usbFeaturePolicy.https.sub.html
(right):

https://codereview.chromium.org/2815003005/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/external/wpt/webusb/usbFeaturePolicy.https.sub.html:8:
<script src=/resources/testharnessreport.js></script>
@lunalu, you've been looking that the use of testharnessreport.js in the other
Feature Policy tests, for crbug.com/716085 -- can you take a look at this test
to see if it's going to need to be changed as well?

https://codereview.chromium.org/2815003005/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp
(right):

https://codereview.chromium.org/2815003005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp:137:
default_feature_name_map.Set("usb", WebFeaturePolicyFeature::kUsb);
Can you make this line conditional on RuntimeEnabledFeatures::WebUSB? WebUSB
implies FP, but not the other way around, and we shouldn't be recognizing the
feature in policy declarations if WebUSB is disabled in the browser.

[iclelland.google, 2017-05-02 14:43:42]

iclelland@google.com changed reviewers:
+ lunalu@chromium.org

[iclelland.google, 2017-05-02 14:43:43]

+lunalu to take a look at the layout tests

[Reilly Grant (use Gerrit), 2017-05-02 19:28:26]

Addressed iclelland@'s comments

[Reilly Grant (use Gerrit), 2017-05-02 19:28:29]

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

[commit-bot: I haz the power, 2017-05-02 19:29:01]

Dry run: CQ is trying da patch.

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

[Reilly Grant (use Gerrit), 2017-05-02 19:29:06]

https://codereview.chromium.org/2815003005/diff/40001/chrome/browser/usb/usb_...
File chrome/browser/usb/usb_tab_helper.cc (right):

https://codereview.chromium.org/2815003005/diff/40001/chrome/browser/usb/usb_...
chrome/browser/usb/usb_tab_helper.cc:66: if
(base::FeatureList::IsEnabled(features::kFeaturePolicy)) {
On 2017/05/02 14:42:52, iclelland.google wrote:
> Could this block, and the identical one below, be extracted into a single
> boolean "is USB enabled in RFH" method?

Done.

https://codereview.chromium.org/2815003005/diff/40001/device/usb/mojo/device_...
File device/usb/mojo/device_impl.cc (right):

https://codereview.chromium.org/2815003005/diff/40001/device/usb/mojo/device_...
device/usb/mojo/device_impl.cc:167: return true;
On 2017/05/02 14:42:52, iclelland.google wrote:
> Drive-by: I'm not really looking at this part of the CL, but all of these
> 'return true' statements look redundant.

Done.

https://codereview.chromium.org/2815003005/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp
(right):

https://codereview.chromium.org/2815003005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp:137:
default_feature_name_map.Set("usb", WebFeaturePolicyFeature::kUsb);
On 2017/05/02 14:42:52, iclelland.google wrote:
> Can you make this line conditional on RuntimeEnabledFeatures::WebUSB? WebUSB
> implies FP, but not the other way around, and we shouldn't be recognizing the
> feature in policy declarations if WebUSB is disabled in the browser.

WebUSB is an Origin Trial so RuntimeEnabledFeature::webUSBEnabled() may return
false even if the API is exposed to the web. It's confusing but that's what
we're stuck with.

[commit-bot: I haz the power, 2017-05-02 20:03:11]

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

[commit-bot: I haz the power, 2017-05-02 20:03:12]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[lunalu1, 2017-05-03 15:01:22]

Hi Reilly,

Thank you for writing this CL. 
Sorry for my delayed reply. I left a few comments on the web platform test part.
Please ignore me if what I mentioned does not apply.

Cheers,
Luna

https://codereview.chromium.org/2815003005/diff/40001/device/usb/mojo/device_...
File device/usb/mojo/device_impl.cc (right):

https://codereview.chromium.org/2815003005/diff/40001/device/usb/mojo/device_...
device/usb/mojo/device_impl.cc:194: if (granted) {
If I recall correctly, you don't need bracket here for else statement.
Can we do something similar to DeviceImpl::SetConfiguration:

if (granted) {
    device_->Open(
        base::Bind(&DeviceImpl::OnOpen, weak_factory_.GetWeakPtr(), callback));
    return;
}
callback.Run(mojom::UsbOpenDeviceError::ACCESS_DENIED);

https://codereview.chromium.org/2815003005/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/external/wpt/webusb/usbFeaturePolicy.https.sub.html
(right):

https://codereview.chromium.org/2815003005/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/external/wpt/webusb/usbFeaturePolicy.https.sub.html:3:
<head>
>>>Please correct me if I am wrong. According to the WPT guide
(http://web-platform-tests.org/writing-tests/general-guidelines.html):
"File names should generally be somewhat descriptive of what is being tested;
very generic names like 001.html are discouraged. A common format is
test-topic-001.html, where test-topic is a short identifier that describes the
test."

Maybe we can consider rename this test to be usb-feature-policy.sub.html

>>>Question, why do we need ".https" here?

So please ignore me if I am wrong.

https://codereview.chromium.org/2815003005/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/external/wpt/webusb/usbFeaturePolicy.https.sub.html:4:
<meta charset="utf-8">
We also need more tests for this with feature policy:
case 1: disabled in the http header
So create a test file named usb-disabled-by-feature-policy.sub.html 
with header policy usb-disabled-by-feature-policy.sub.html.headers:
      Feature-Policy: {"usb": []}
case 2: enabled for all (this is the default policy for usb), same as above.
      Feature-Policy: {"usb": ['*']}
case 3: enabled for self, same as above.
      Feature-Policy: {"usb": ['self']}

In all 3 cases, we should test if usb is enabled/disabled in same-origin and
cross-origin which you have done in your 2 async_tests.

Ideally, there should also be test where usb is enabled through iframe allow
attribute with relocation (with and without reloading the document) too. I am
working on some examples of how to write those tests. So maybe you can add those
tests later once I commit the examples.

https://codereview.chromium.org/2815003005/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/external/wpt/webusb/usbFeaturePolicy.https.sub.html:6:
<link rel="help" href="https://wicg.github.io/webusb/">
By the way, I assume you tested this file following the instructions
(http://web-platform-tests.org/running-tests/index.html). If not, I encourage
you to do so.

https://codereview.chromium.org/2815003005/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/external/wpt/webusb/usbFeaturePolicy.https.sub.html:18:
async_test(t => {
The 3 async_tests look very similar in format. We could write one generic test
method and call it 3 times. But given this is just a test file. It doesn't have
to be done.

https://codereview.chromium.org/2815003005/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/external/wpt/webusb/usbFeaturePolicy.https.sub.html:65:
</html>
I don't think <head> <html> and <body> are needed for layout tests. But I could
be wrong.

[lunalu1, 2017-05-03 15:01:26]

Hi Reilly,

Thank you for writing this CL. 
Sorry for my delayed reply. I left a few comments on the web platform test part.
Please ignore me if what I mentioned does not apply.

Cheers,
Luna

[lunalu1, 2017-05-03 21:40:10]

Hopefully this CL can give you a better idea of what needs to be tested for a
feature policy feature.
https://codereview.chromium.org/2855133004/

[Reilly Grant (use Gerrit), 2017-05-04 01:16:49]

reillyg@chromium.org changed reviewers:
- iclelland@google.com

[Reilly Grant (use Gerrit), 2017-05-04 01:16:50]

I've fleshed out the web platform tests some more however
usb-allowed-by-feature-policy.https.sub.html is not passing. Did I get
usb-allowed-by-feature-policy.https.sub.html.headers wrong?

https://codereview.chromium.org/2815003005/diff/40001/device/usb/mojo/device_...
File device/usb/mojo/device_impl.cc (right):

https://codereview.chromium.org/2815003005/diff/40001/device/usb/mojo/device_...
device/usb/mojo/device_impl.cc:194: if (granted) {
On 2017/05/03 15:01:21, loonybear wrote:
> If I recall correctly, you don't need bracket here for else statement.
> Can we do something similar to DeviceImpl::SetConfiguration:
> 
> if (granted) {
>     device_->Open(
>         base::Bind(&DeviceImpl::OnOpen, weak_factory_.GetWeakPtr(),
callback));
>     return;
> }
> callback.Run(mojom::UsbOpenDeviceError::ACCESS_DENIED);

Done, though I put the error case first which is more common.

https://codereview.chromium.org/2815003005/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/external/wpt/webusb/usbFeaturePolicy.https.sub.html
(right):

https://codereview.chromium.org/2815003005/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/external/wpt/webusb/usbFeaturePolicy.https.sub.html:3:
<head>
On 2017/05/03 15:01:21, loonybear wrote:
> >>>Please correct me if I am wrong. According to the WPT guide
> (http://web-platform-tests.org/writing-tests/general-guidelines.html):
> "File names should generally be somewhat descriptive of what is being tested;
> very generic names like 001.html are discouraged. A common format is
> test-topic-001.html, where test-topic is a short identifier that describes the
> test."
> 
> Maybe we can consider rename this test to be usb-feature-policy.sub.html

Done.

> Question, why do we need ".https" here?

This API is only available in secure contexts.

https://codereview.chromium.org/2815003005/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/external/wpt/webusb/usbFeaturePolicy.https.sub.html:4:
<meta charset="utf-8">
On 2017/05/03 15:01:21, loonybear wrote:
> We also need more tests for this with feature policy:
> case 1: disabled in the http header
> So create a test file named usb-disabled-by-feature-policy.sub.html 
> with header policy usb-disabled-by-feature-policy.sub.html.headers:
>       Feature-Policy: {"usb": []}
> case 2: enabled for all (this is the default policy for usb), same as above.
>       Feature-Policy: {"usb": ['*']}
> case 3: enabled for self, same as above.
>       Feature-Policy: {"usb": ['self']}
> 
> In all 3 cases, we should test if usb is enabled/disabled in same-origin and
> cross-origin which you have done in your 2 async_tests.
> 
> Ideally, there should also be test where usb is enabled through iframe allow
> attribute with relocation (with and without reloading the document) too. I am
> working on some examples of how to write those tests. So maybe you can add
those
> tests later once I commit the examples.

Done.

https://codereview.chromium.org/2815003005/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/external/wpt/webusb/usbFeaturePolicy.https.sub.html:6:
<link rel="help" href="https://wicg.github.io/webusb/">
On 2017/05/03 15:01:21, loonybear wrote:
> By the way, I assume you tested this file following the instructions
> (http://web-platform-tests.org/running-tests/index.html). If not, I encourage
> you to do so.

Yes.

https://codereview.chromium.org/2815003005/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/external/wpt/webusb/usbFeaturePolicy.https.sub.html:65:
</html>
On 2017/05/03 15:01:21, loonybear wrote:
> I don't think <head> <html> and <body> are needed for layout tests. But I
could
> be wrong.

<body> is required but the rest are not.

[lunalu1, 2017-05-04 14:36:58]

On 2017/05/04 01:16:50, Reilly Grant wrote:
> I've fleshed out the web platform tests some more however
> usb-allowed-by-feature-policy.https.sub.html is not passing. Did I get
> usb-allowed-by-feature-policy.https.sub.html.headers wrong?
> 
>
https://codereview.chromium.org/2815003005/diff/40001/device/usb/mojo/device_...
> File device/usb/mojo/device_impl.cc (right):
> 
>
https://codereview.chromium.org/2815003005/diff/40001/device/usb/mojo/device_...
> device/usb/mojo/device_impl.cc:194: if (granted) {
> On 2017/05/03 15:01:21, loonybear wrote:
> > If I recall correctly, you don't need bracket here for else statement.
> > Can we do something similar to DeviceImpl::SetConfiguration:
> > 
> > if (granted) {
> >     device_->Open(
> >         base::Bind(&DeviceImpl::OnOpen, weak_factory_.GetWeakPtr(),
> callback));
> >     return;
> > }
> > callback.Run(mojom::UsbOpenDeviceError::ACCESS_DENIED);
> 
> Done, though I put the error case first which is more common.
> 
>
https://codereview.chromium.org/2815003005/diff/40001/third_party/WebKit/Layo...
> File
>
third_party/WebKit/LayoutTests/external/wpt/webusb/usbFeaturePolicy.https.sub.html
> (right):
> 
>
https://codereview.chromium.org/2815003005/diff/40001/third_party/WebKit/Layo...
>
third_party/WebKit/LayoutTests/external/wpt/webusb/usbFeaturePolicy.https.sub.html:3:
> <head>
> On 2017/05/03 15:01:21, loonybear wrote:
> > >>>Please correct me if I am wrong. According to the WPT guide
> > (http://web-platform-tests.org/writing-tests/general-guidelines.html):
> > "File names should generally be somewhat descriptive of what is being
tested;
> > very generic names like 001.html are discouraged. A common format is
> > test-topic-001.html, where test-topic is a short identifier that describes
the
> > test."
> > 
> > Maybe we can consider rename this test to be usb-feature-policy.sub.html
> 
> Done.
> 
> > Question, why do we need ".https" here?
> 
> This API is only available in secure contexts.
> 
>
https://codereview.chromium.org/2815003005/diff/40001/third_party/WebKit/Layo...
>
third_party/WebKit/LayoutTests/external/wpt/webusb/usbFeaturePolicy.https.sub.html:4:
> <meta charset="utf-8">
> On 2017/05/03 15:01:21, loonybear wrote:
> > We also need more tests for this with feature policy:
> > case 1: disabled in the http header
> > So create a test file named usb-disabled-by-feature-policy.sub.html 
> > with header policy usb-disabled-by-feature-policy.sub.html.headers:
> >       Feature-Policy: {"usb": []}
> > case 2: enabled for all (this is the default policy for usb), same as above.
> >       Feature-Policy: {"usb": ['*']}
> > case 3: enabled for self, same as above.
> >       Feature-Policy: {"usb": ['self']}
> > 
> > In all 3 cases, we should test if usb is enabled/disabled in same-origin and
> > cross-origin which you have done in your 2 async_tests.
> > 
> > Ideally, there should also be test where usb is enabled through iframe allow
> > attribute with relocation (with and without reloading the document) too. I
am
> > working on some examples of how to write those tests. So maybe you can add
> those
> > tests later once I commit the examples.
> 
> Done.
> 
>
https://codereview.chromium.org/2815003005/diff/40001/third_party/WebKit/Layo...
>
third_party/WebKit/LayoutTests/external/wpt/webusb/usbFeaturePolicy.https.sub.html:6:
> <link rel="help" href="https://wicg.github.io/webusb/">
> On 2017/05/03 15:01:21, loonybear wrote:
> > By the way, I assume you tested this file following the instructions
> > (http://web-platform-tests.org/running-tests/index.html). If not, I
encourage
> > you to do so.
> 
> Yes.
> 
>
https://codereview.chromium.org/2815003005/diff/40001/third_party/WebKit/Layo...
>
third_party/WebKit/LayoutTests/external/wpt/webusb/usbFeaturePolicy.https.sub.html:65:
> </html>
> On 2017/05/03 15:01:21, loonybear wrote:
> > I don't think <head> <html> and <body> are needed for layout tests. But I
> could
> > be wrong.
> 
> <body> is required but the rest are not.

A quick response here: some tests are expected to fail as feature policy is
still behind a flag. I will look more into it to double confirm.

[iclelland, 2017-05-04 15:24:09]

Feature policy integration lgtm

https://codereview.chromium.org/2815003005/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp
(right):

https://codereview.chromium.org/2815003005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp:137:
default_feature_name_map.Set("usb", WebFeaturePolicyFeature::kUsb);
On 2017/05/02 19:29:05, Reilly Grant wrote:
> On 2017/05/02 14:42:52, iclelland.google wrote:
> > Can you make this line conditional on RuntimeEnabledFeatures::WebUSB? WebUSB
> > implies FP, but not the other way around, and we shouldn't be recognizing
the
> > feature in policy declarations if WebUSB is disabled in the browser.
> 
> WebUSB is an Origin Trial so RuntimeEnabledFeature::webUSBEnabled() may return
> false even if the API is exposed to the web. It's confusing but that's what
> we're stuck with.

Right, of course. (And other than plumbing the execution context down to this
method, that's probably not going to change soon.) I was thinking that this was
part of shipping WebUSB, and removing it from origin trials.

https://codereview.chromium.org/2815003005/diff/80001/device/usb/mojo/device_...
File device/usb/mojo/device_impl.cc (right):

https://codereview.chromium.org/2815003005/diff/80001/device/usb/mojo/device_...
device/usb/mojo/device_impl.cc:168: return interface;
Returning a pointer from a bool method seems wrong (though I can't find anything
specifically prohibiting it in the style guide)  -- Is it clearer to explicitly
return (interface != nullptr)?

At the very least, a comment saying that we're not actually trying to return the
interface itself might be helpful.

https://codereview.chromium.org/2815003005/diff/80001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/external/wpt/webusb/usb-disabled-by-feature-policy.https.sub.html
(right):

https://codereview.chromium.org/2815003005/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/external/wpt/webusb/usb-disabled-by-feature-policy.https.sub.html:23:
}, 'Fea-Policy header {"usb": []} disallows cross-origin iframes.');
nit: typo

[Reilly Grant (use Gerrit), 2017-05-04 20:34:35]

Fixed usb-allowed-by-feature-policy.https.sub.html

[Reilly Grant (use Gerrit), 2017-05-04 20:34:46]

https://codereview.chromium.org/2815003005/diff/80001/device/usb/mojo/device_...
File device/usb/mojo/device_impl.cc (right):

https://codereview.chromium.org/2815003005/diff/80001/device/usb/mojo/device_...
device/usb/mojo/device_impl.cc:168: return interface;
On 2017/05/04 15:24:08, iclelland wrote:
> Returning a pointer from a bool method seems wrong (though I can't find
anything
> specifically prohibiting it in the style guide)  -- Is it clearer to
explicitly
> return (interface != nullptr)?
> 
> At the very least, a comment saying that we're not actually trying to return
the
> interface itself might be helpful.

Done.

https://codereview.chromium.org/2815003005/diff/80001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/external/wpt/webusb/usb-disabled-by-feature-policy.https.sub.html
(right):

https://codereview.chromium.org/2815003005/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/external/wpt/webusb/usb-disabled-by-feature-policy.https.sub.html:23:
}, 'Fea-Policy header {"usb": []} disallows cross-origin iframes.');
On 2017/05/04 15:24:08, iclelland wrote:
> nit: typo

Done.

[Reilly Grant (use Gerrit), 2017-05-04 20:40:04]

reillyg@chromium.org changed reviewers:
+ jochen@chromium.org
- rbyers@chromium.org

[Reilly Grant (use Gerrit), 2017-05-04 20:40:05]

jochen@, please review these files:

chrome/browser/DEPS
third_party/WebKit/Source/platform/RuntimeEnabledFeatures.json5
third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp
third_party/WebKit/public/platform/WebFeaturePolicy.h

[Rick Byers, 2017-05-04 20:52:47]

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

[Rick Byers, 2017-05-04 20:52:48]

WebKit LGTM
(up to you if you still want to ask jochen@ for a review there, seems straight
forward to me).

[jbroman, 2017-05-04 21:18:02]

On 2017/05/02 at 14:33:25, reillyg wrote:
> On 2017/05/02 13:26:56, Rick Byers wrote:
> > On 2017/05/01 23:10:33, Reilly Grant wrote:
> > > iclelland@, please do a general review.
> > > rbyers@, please do an API_OWNERS review.
> > 
> > There's nothing required for API_OWNERS here (since you're not yet shipping
a
> > new API).  But I've looked over all the WebKit code and it seems reasonable
at a
> > high-level to me, so I can give a WebKit OWNERS stamp once iclelland has
> > reviewed the feature policy details.
> 
> Thanks for taking a look. RuntimeEnabledFeatures.json5 is owned by API_OWNERS,
which is what I meant here.

To clarify here, RuntimeEnabledFeatures.json5 is also owned by any owner of
third_party/WebKit/Source/platform/. As the comment in OWNERS indicates, this is
intended to clarify that API_OWNERS is sufficient to approve changes to
RuntimeEnabledFeatures, but doesn't make it necessary (see also disucssion on
https://codereview.chromium.org/2530433004).

(Just want avoid spreading the misconception that all REF features require an
API_OWNERS stamp.)

[Reilly Grant (use Gerrit), 2017-05-04 21:28:07]

On 2017/05/04 20:52:48, Rick Byers wrote:
> WebKit LGTM
> (up to you if you still want to ask jochen@ for a review there, seems straight
> forward to me).

I still need jochen@ for the other files I listed.

[Rick Byers, 2017-05-04 21:40:22]

On 2017/05/04 21:28:07, Reilly Grant wrote:
> On 2017/05/04 20:52:48, Rick Byers wrote:
> > WebKit LGTM
> > (up to you if you still want to ask jochen@ for a review there, seems
straight
> > forward to me).
> 
> I still need jochen@ for the other files I listed.

Yes, sorry - by "there" I meant "for WebKit".

[jochen (gone - plz use gerrit), 2017-05-05 14:28:07]

https://codereview.chromium.org/2815003005/diff/100001/chrome/browser/DEPS
File chrome/browser/DEPS (right):

https://codereview.chromium.org/2815003005/diff/100001/chrome/browser/DEPS#ne...
chrome/browser/DEPS:109:
"+third_party/WebKit/public/platform/WebFeaturePolicy.h",
the browser process must only depend on pure POD / enum headers. Please feature
the enum out into a separate file

[Reilly Grant (use Gerrit), 2017-05-05 22:10:15]

https://codereview.chromium.org/2815003005/diff/100001/chrome/browser/DEPS
File chrome/browser/DEPS (right):

https://codereview.chromium.org/2815003005/diff/100001/chrome/browser/DEPS#ne...
chrome/browser/DEPS:109:
"+third_party/WebKit/public/platform/WebFeaturePolicy.h",
On 2017/05/05 14:28:06, jochen wrote:
> the browser process must only depend on pure POD / enum headers. Please
feature
> the enum out into a separate file

Done.

[jochen (gone - plz use gerrit), 2017-05-08 06:17:09]

lgtm

[lunalu1, 2017-05-08 15:12:12]

wpt lgtm

[Reilly Grant (use Gerrit), 2017-05-08 15:14:31]

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

[commit-bot: I haz the power, 2017-05-08 15:15:01]

Dry run: 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-05-08 17:57:40]

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

[commit-bot: I haz the power, 2017-05-08 17:57:41]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Reilly Grant (use Gerrit), 2017-05-12 18:28:57]

Rebased

[Reilly Grant (use Gerrit), 2017-05-12 18:29:28]

The CQ bit was checked by reillyg@chromium.org

[Reilly Grant (use Gerrit), 2017-05-12 18:29:29]

The patchset sent to the CQ was uploaded after l-g-t-m from
iclelland@chromium.org, rbyers@chromium.org, jochen@chromium.org,
lunalu@chromium.org
Link to the patchset: https://codereview.chromium.org/2815003005/#ps140001
(title: "Rebased")

[commit-bot: I haz the power, 2017-05-12 18:30:35]

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-05-12 20:04:13]

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

[commit-bot: I haz the power, 2017-05-12 20:04:15]

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

[Reilly Grant (use Gerrit), 2017-05-12 20:38:02]

The CQ bit was checked by reillyg@chromium.org

[commit-bot: I haz the power, 2017-05-12 20:38:50]

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-05-12 23:31:15]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1494621482842240,
"parent_rev": "2ae4e4bee29bcb52bc0dd21a2dd59551003cd678", "commit_rev":
"aee98b5c7aebe3c263f2150357d36901efbf0d53"}

[commit-bot: I haz the power, 2017-05-12 23:31:31]

Description was changed from

==========
Integrate WebUSB with Feature Policy

This patch uses the new Feature Policy framework to control access to           
WebUSB from embedded frames. When Feature Policy is available WebUSB's          
default policy ["self"] is enforced. When it isn't available WebUSB is          
simply disallowed in embedded frames.

A follow-up patch will remove the parser for the "allowed origins"
descriptor as they are no longer used and have been removed from the
specification.

BUG=711443
==========

to

==========
Integrate WebUSB with Feature Policy

This patch uses the new Feature Policy framework to control access to           
WebUSB from embedded frames. When Feature Policy is available WebUSB's          
default policy ["self"] is enforced. When it isn't available WebUSB is          
simply disallowed in embedded frames.

A follow-up patch will remove the parser for the "allowed origins"
descriptor as they are no longer used and have been removed from the
specification.

BUG=711443

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

[commit-bot: I haz the power, 2017-05-12 23:31:32]

Committed patchset #8 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/aee98b5c7aebe3c263f2150357d3...

[loonybear, 2017-07-12 19:06:33]

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

[loonybear, 2017-07-12 19:06:34]

Hi Reilly, 

A while ago, we added this method
IsSupportedInFeaturePolicy(WebFeaturePolicyFeature);
in third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.h
to check if feature policy is enabled and |feature| is supported in feature
policy.

Currently the other features in feature policy are using it. Examples
(third_party/WebKit/Source/core/dom/Fullscreen.cpp,
third_party/WebKit/Source/modules/vibration/NavigatorVibration.cpp,
third_party/WebKit/Source/modules/payments/PaymentRequest.cpp)

Not so sure if this would apply to web usb. This is just FYI.

Thanks,
Luna