Re-enable webusb feature #2574
Re-enable webusb feature #2574
Conversation
simonhong
force-pushed
the
dont_prevent_webusb
branch
from
d4e7e86
to
3af7350
Compare
|
Thanks for considering adding WebUSB back! We (and our users) would love to have it enabled in Brave. What was the rationale for removing it in the first place? From the thread in #13, it seems it was done out of privacy concerns. Does the need to pair devices in order for websites to talk to them mitigate that? |
|
@keepkeyjon it was disabled due to a security issue in brave/browser-laptop#13374. |
|
according to https://www.yubico.com/2018/06/webusb-and-responsible-disclosure/, the original security issue was fixed in chromium 67 so we should be ok to re-enable webusb now. @simonhong please make sure all necessary wiki pages (linked in the PR template) that mention webusb are updated once this is merged. thanks! |
|
@diracdeltas two document are updated. |
| void BraveContentRendererClient::SetRuntimeFeaturesDefaultsBeforeBlinkInitialization() { | ||
| blink::WebRuntimeFeatures::EnableWebUsb(false); | ||
| void BraveContentRendererClient:: | ||
| SetRuntimeFeaturesDefaultsBeforeBlinkInitialization() { | ||
| blink::WebRuntimeFeatures::EnableSharedArrayBuffer(false); |
There was a problem hiding this comment.
Not your issue, but shouldn't we be calling the base class here?
void ChromeContentRendererClient::
SetRuntimeFeaturesDefaultsBeforeBlinkInitialization() {
// The performance manager service interfaces are provided by the chrome
// embedder only.
blink::WebRuntimeFeatures::EnablePerformanceManagerInstrumentation(true);
// Web Share is shipped on Android, experimental otherwise. It is enabled here,
// in chrome/, to avoid it being made available in other clients of content/
// that do not have a Web Share Mojo implementation.
#if defined(OS_ANDROID)
blink::WebRuntimeFeatures::EnableWebShare(true);
blink::WebRuntimeFeatures::EnableWebShareV2(true);
#endif
if (base::FeatureList::IsEnabled(subresource_filter::kAdTagging))
blink::WebRuntimeFeatures::EnableAdTagging(true);
}
There was a problem hiding this comment.
@bbondy Yup, we should call base class' method also if we don't want to prevent all.
I assume that when @jumde pushed #114, base class methods would be like below.(Only it had android stuff)
void ChromeContentRendererClient::
SetRuntimeFeaturesDefaultsBeforeBlinkInitialization() {
// Web Share is shipped on Android, experimental otherwise. It is enabled here,
// in chrome/, to avoid it being made available in other clients of content/
// that do not have a Web Share Mojo implementation.
#if defined(OS_ANDROID)
blink::WebRuntimeFeatures::EnableWebShare(true);
#endif
}
So, @jumde might skip calling it.
Added.
There was a problem hiding this comment.
I think maybe it is safest to always call it and just undo anything we don't want to do. But probably there is nothing else we don't want to do. We'll have android support soon btw.
|
Just to surface something that came up in #privacy today Clearing site data does not currently clear suite permissions to access USB devices. Whether thats part of this issue, or its own issue, that seems like something important to work out before enabling WebUSB |
Please post a different issue for that @snyderp , I think we want to have better webcompat sooner than later. |
simonhong
force-pushed
the
dont_prevent_webusb
branch
2 times, most recently
from
fde0861
to
34fb5d5
Compare
|
Only unrelated one step in CI was failed - |
simonhong
force-pushed
the
dont_prevent_webusb
branch
from
34fb5d5
to
5897901
Compare
|
Passed in CI. Any more comments on this? |
|
Went through the following verifications:
Examples:
|
@snyderp did you end up creating an issue for the above? Just double checking to make sure it doesn't end up falling through the cracks. |
|
@kjozwiak the issue was that I'm a dingbat and was confused by the "Clear data" dialog. I didn't realize the tabs on the "clear data" dialog control the behavior of the "clear" button at the bottom (instead of just showing more information about the same action). TL;DR: i'm a dumb-dumb and no issue needed. I appreciate you double checking though :) |
|
@snyderp haha no worries! Appreciate the update 👍 |
This api was disabled by #114
Fix brave/brave-browser#4669
Submitter Checklist:
npm test brave_unit_tests && npm test brave_browser_tests && npm run test-security) onnpm run lint)git rebase master(if needed).git rebase -ito squash commits (if needed).Test Plan:
Check
navigator.usbdoesn't returnundefinedin console.Reviewer Checklist:
After-merge Checklist:
changes has landed on.