Add a kill switch for all of Web Bluetooth.

Add a kill switch for all of Web Bluetooth.

This is temporary until we get Web Bluetooth behind
PermissionContextBase, but that may block on getting some UI done, so I
don't want to bet on it happening by M51.

BUG=598059
Committed: https://crrev.com/c993ce8db282755a85b9671638ba24de0c5ca946
Cr-Commit-Position: refs/heads/master@{#384162}

[Jeffrey Yasskin, 2016-03-29 17:39:42]

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

[commit-bot: I haz the power, 2016-03-29 17:39:55]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1841763002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1841763002/20001

[Jeffrey Yasskin, 2016-03-29 17:41:16]

jyasskin@chromium.org changed reviewers:
+ asvitkine@chromium.org, jam@chromium.org, kcarattini@chromium.org,
ortuno@chromium.org

[ortuno, 2016-03-29 17:48:47]

https://codereview.chromium.org/1841763002/diff/20001/content/browser/bluetoo...
File content/browser/bluetooth/bluetooth_dispatcher_host.cc (right):

https://codereview.chromium.org/1841763002/diff/20001/content/browser/bluetoo...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:1222: switch
(GetContentClient()->browser()->AllowWebBluetooth(
Doing this so late in OnRequestDeviceImpl seems wrong. I think it would be
better if we did it before we try to initialize the adapter, in OnRequestDevice.

https://codereview.chromium.org/1841763002/diff/20001/content/browser/bluetoo...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:1231: case
ContentBrowserClient::AllowWebBluetoothResult::BLOCK_KILL_SWITCH: {
What do you think about checking if the WebBluetoothFlag is on? That way people
that know what they are doing can still use the feature?

[jam, 2016-03-29 18:06:18]

https://codereview.chromium.org/1841763002/diff/20001/content/public/browser/...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/1841763002/diff/20001/content/public/browser/...
content/public/browser/content_browser_client.h:424: BLOCK_KILL_SWITCH,
can we keep the "kill switch" concept out of content, since it's a chrome level
thing? the webkit mapping says "Web Bluetooth permission globally disabled", so
perhaps this can be BLOCK_GLOBALLY_DISABLED?

[Jeffrey Yasskin, 2016-03-29 23:11:46]

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

[commit-bot: I haz the power, 2016-03-29 23:12:10]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1841763002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1841763002/60001

[Jeffrey Yasskin, 2016-03-29 23:13:04]

https://codereview.chromium.org/1841763002/diff/20001/content/browser/bluetoo...
File content/browser/bluetooth/bluetooth_dispatcher_host.cc (right):

https://codereview.chromium.org/1841763002/diff/20001/content/browser/bluetoo...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:1222: switch
(GetContentClient()->browser()->AllowWebBluetooth(
On 2016/03/29 17:48:46, ortuno wrote:
> Doing this so late in OnRequestDeviceImpl seems wrong. I think it would be
> better if we did it before we try to initialize the adapter, in
OnRequestDevice.

It probably makes sense to reconsider a lot of the requestDevice ordering, but
it's not intimately tied to adding the kill switch, so let's do it in a separate
patch? We're going to be forced to rearrange some of this anyway when we
refactor it into the PermissionContextBase structure, so let's think about it
then, unless there's some more urgent problem with this ordering.

https://codereview.chromium.org/1841763002/diff/20001/content/browser/bluetoo...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:1231: case
ContentBrowserClient::AllowWebBluetoothResult::BLOCK_KILL_SWITCH: {
On 2016/03/29 17:48:46, ortuno wrote:
> What do you think about checking if the WebBluetoothFlag is on? That way
people
> that know what they are doing can still use the feature?

For now, I think that prevents me from testing this change. I've filed
https://crbug.com/598890 to track this.

https://codereview.chromium.org/1841763002/diff/20001/content/public/browser/...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/1841763002/diff/20001/content/public/browser/...
content/public/browser/content_browser_client.h:424: BLOCK_KILL_SWITCH,
On 2016/03/29 18:06:18, jam wrote:
> can we keep the "kill switch" concept out of content, since it's a chrome
level
> thing? the webkit mapping says "Web Bluetooth permission globally disabled",
so
> perhaps this can be BLOCK_GLOBALLY_DISABLED?

Sure, unless Kendra wants to argue that it ought to be a content/web-level
thing. I've made this change assuming she won't.

[jam, 2016-03-30 00:20:58]

lgtm

[kcarattini, 2016-03-30 00:45:59]

https://codereview.chromium.org/1841763002/diff/20001/content/public/browser/...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/1841763002/diff/20001/content/public/browser/...
content/public/browser/content_browser_client.h:424: BLOCK_KILL_SWITCH,
On 2016/03/29 23:13:04, Jeffrey Yasskin wrote:
> On 2016/03/29 18:06:18, jam wrote:
> > can we keep the "kill switch" concept out of content, since it's a chrome
> level
> > thing? the webkit mapping says "Web Bluetooth permission globally disabled",
> so
> > perhaps this can be BLOCK_GLOBALLY_DISABLED?
> 
> Sure, unless Kendra wants to argue that it ought to be a content/web-level
> thing. I've made this change assuming she won't.

No argument here.

https://codereview.chromium.org/1841763002/diff/60001/chrome/browser/web_blue...
File chrome/browser/web_bluetooth_browsertest.cc (right):

https://codereview.chromium.org/1841763002/diff/60001/chrome/browser/web_blue...
chrome/browser/web_bluetooth_browsertest.cc:56: // Turn on the global kill
switch.
Can you also test that it works properly without the kill switch on?

[Jeffrey Yasskin, 2016-03-30 01:19:51]

https://codereview.chromium.org/1841763002/diff/60001/chrome/browser/web_blue...
File chrome/browser/web_bluetooth_browsertest.cc (right):

https://codereview.chromium.org/1841763002/diff/60001/chrome/browser/web_blue...
chrome/browser/web_bluetooth_browsertest.cc:56: // Turn on the global kill
switch.
On 2016/03/30 00:45:59, kcarattini wrote:
> Can you also test that it works properly without the kill switch on?

We test that in
https://code.google.com/p/chromium/codesearch/#chromium/src/third_party/WebKi....
I agree it'd be nice to have this test cover the success case too, but we'd need
to drive the chooser dialog from the test, which either implies subclassing
Browser or writing code to click buttons. The layout tests do it by customizing
Shell, which we don't have access to in a browsertest.

[kcarattini, 2016-03-30 01:23:12]

lgtm

[commit-bot: I haz the power, 2016-03-30 01:27:01]

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

[commit-bot: I haz the power, 2016-03-30 01:27:02]

Dry run: This issue passed the CQ dry run.

[Alexei Svitkine (slow), 2016-03-30 15:28:17]

lgtm

[Alexei Svitkine (slow), 2016-03-30 16:48:54]

However, please don't use "Finch" in CL descriptions or code - since it's an
internal codename. You can use terms like "variations" or "server-side
experiments".

[Jeffrey Yasskin, 2016-03-30 16:59:29]

Description was changed from

==========
Add a finch-based kill switch for all of Web Bluetooth.

This is temporary until we get Web Bluetooth behind
PermissionContextBase, but that may block on getting some UI done, so I
don't want to bet on it happening by M51.

BUG=598059
==========

to

==========
Add a kill switch for all of Web Bluetooth.

This is temporary until we get Web Bluetooth behind
PermissionContextBase, but that may block on getting some UI done, so I
don't want to bet on it happening by M51.

BUG=598059
==========

[Jeffrey Yasskin, 2016-03-30 18:42:37]

On 2016/03/30 16:48:54, Alexei Svitkine wrote:
> However, please don't use "Finch" in CL descriptions or code - since it's an
> internal codename. You can use terms like "variations" or "server-side
> experiments".

Ah, sorry. Fixed.

[ortuno, 2016-03-30 19:19:46]

https://codereview.chromium.org/1841763002/diff/60001/content/browser/bluetoo...
File content/browser/bluetooth/bluetooth_dispatcher_host.cc (right):

https://codereview.chromium.org/1841763002/diff/60001/content/browser/bluetoo...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:1400:
WebBluetoothError::ChooserNotShown_BrowserDeniedPermissionToScan));
I think you might have meant to change the error in line 1274. In this case we
do show the chooser; that's where the user can press the "Update Permission"
option and that will request for the permission.

https://codereview.chromium.org/1841763002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/bluetooth/BluetoothError.cpp (right):

https://codereview.chromium.org/1841763002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/bluetooth/BluetoothError.cpp:53:
MAP_ERROR(ChooserNotShown_BrowserDeniedPermissionToScan, NotFoundError, "User
denied the browser permission to scan for Bluetooth devices.");
We have errors where we don't show the chooser i.e. when we can't ask for
permissions but we also have errors where we do show the chooser but the user
decided not to enable the permission.

[Jeffrey Yasskin, 2016-03-30 19:31:24]

https://codereview.chromium.org/1841763002/diff/60001/content/browser/bluetoo...
File content/browser/bluetooth/bluetooth_dispatcher_host.cc (right):

https://codereview.chromium.org/1841763002/diff/60001/content/browser/bluetoo...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:1400:
WebBluetoothError::ChooserNotShown_BrowserDeniedPermissionToScan));
On 2016/03/30 19:19:46, ortuno wrote:
> I think you might have meant to change the error in line 1274. In this case we
> do show the chooser; that's where the user can press the "Update Permission"
> option and that will request for the permission.

Line 1274 is just the chooser event saying that it can't ask for permission to
scan. That event comes down here and closes the chooser with a message to the
website that it can't ask for permission anymore.

It's true that we might have shown the chooser before being denied permission to
ask again, but on future calls we won't show it, since we can't ask, and the
website instead needs to guide users to re-enable the permission.

When we simply don't have permission but can ask, we shouldn't return this
error. Let me know if you see a path where we would.

https://codereview.chromium.org/1841763002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/bluetooth/BluetoothError.cpp (right):

https://codereview.chromium.org/1841763002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/bluetooth/BluetoothError.cpp:53:
MAP_ERROR(ChooserNotShown_BrowserDeniedPermissionToScan, NotFoundError, "User
denied the browser permission to scan for Bluetooth devices.");
On 2016/03/30 19:19:46, ortuno wrote:
> We have errors where we don't show the chooser i.e. when we can't ask for
> permissions but we also have errors where we do show the chooser but the user
> decided not to enable the permission.

Yeah, there's a chance this one did show the chooser, and that's when the user
decided never to let Chrome ask again. Do you have a better idea for the enum
value? It's a separate error message because the website needs to know that
calling requestDevice() either had or is going to have no effect, so the user's
going to think something's broken.

[ortuno, 2016-03-30 19:41:51]

https://codereview.chromium.org/1841763002/diff/60001/content/browser/bluetoo...
File content/browser/bluetooth/bluetooth_dispatcher_host.cc (right):

https://codereview.chromium.org/1841763002/diff/60001/content/browser/bluetoo...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:1400:
WebBluetoothError::ChooserNotShown_BrowserDeniedPermissionToScan));
On 2016/03/30 at 19:31:24, Jeffrey Yasskin wrote:
> On 2016/03/30 19:19:46, ortuno wrote:
> > I think you might have meant to change the error in line 1274. In this case
we
> > do show the chooser; that's where the user can press the "Update Permission"
> > option and that will request for the permission.
> 
> Line 1274 is just the chooser event saying that it can't ask for permission to
scan. That event comes down here and closes the chooser with a message to the
website that it can't ask for permission anymore.
> 
> It's true that we might have shown the chooser before being denied permission
to ask again, but on future calls we won't show it, since we can't ask, and the
website instead needs to guide users to re-enable the permission.
> 
> When we simply don't have permission but can ask, we shouldn't return this
error. Let me know if you see a path where we would.

Ah you're totally right.

https://codereview.chromium.org/1841763002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/bluetooth/BluetoothError.cpp (right):

https://codereview.chromium.org/1841763002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/bluetooth/BluetoothError.cpp:53:
MAP_ERROR(ChooserNotShown_BrowserDeniedPermissionToScan, NotFoundError, "User
denied the browser permission to scan for Bluetooth devices.");
On 2016/03/30 at 19:31:24, Jeffrey Yasskin wrote:
> On 2016/03/30 19:19:46, ortuno wrote:
> > We have errors where we don't show the chooser i.e. when we can't ask for
> > permissions but we also have errors where we do show the chooser but the
user
> > decided not to enable the permission.
> 
> Yeah, there's a chance this one did show the chooser, and that's when the user
decided never to let Chrome ask again. Do you have a better idea for the enum
value? It's a separate error message because the website needs to know that
calling requestDevice() either had or is going to have no effect, so the user's
going to think something's broken.

"UserDeniedPermissionToScan"?

[Jeffrey Yasskin, 2016-03-30 23:11:33]

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

[Jeffrey Yasskin, 2016-03-30 23:11:40]

https://codereview.chromium.org/1841763002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/bluetooth/BluetoothError.cpp (right):

https://codereview.chromium.org/1841763002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/bluetooth/BluetoothError.cpp:53:
MAP_ERROR(ChooserNotShown_BrowserDeniedPermissionToScan, NotFoundError, "User
denied the browser permission to scan for Bluetooth devices.");
On 2016/03/30 19:41:51, ortuno wrote:
> On 2016/03/30 at 19:31:24, Jeffrey Yasskin wrote:
> > On 2016/03/30 19:19:46, ortuno wrote:
> > > We have errors where we don't show the chooser i.e. when we can't ask for
> > > permissions but we also have errors where we do show the chooser but the
> user
> > > decided not to enable the permission.
> > 
> > Yeah, there's a chance this one did show the chooser, and that's when the
user
> decided never to let Chrome ask again. Do you have a better idea for the enum
> value? It's a separate error message because the website needs to know that
> calling requestDevice() either had or is going to have no effect, so the
user's
> going to think something's broken.
> 
> "UserDeniedPermissionToScan"?

Sure, done.

[commit-bot: I haz the power, 2016-03-30 23:11:51]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1841763002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1841763002/80001

[ortuno, 2016-03-30 23:12:15]

lgtm

[Jeffrey Yasskin, 2016-03-30 23:27:12]

The CQ bit was unchecked by jyasskin@chromium.org

[Jeffrey Yasskin, 2016-03-30 23:27:23]

The CQ bit was checked by jyasskin@chromium.org

[Jeffrey Yasskin, 2016-03-30 23:27:24]

The patchset sent to the CQ was uploaded after l-g-t-m from
asvitkine@chromium.org, jam@chromium.org, kcarattini@chromium.org
Link to the patchset: https://codereview.chromium.org/1841763002/#ps80001
(title: "Sync and rename an enum value.")

[commit-bot: I haz the power, 2016-03-30 23:27:59]

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

[commit-bot: I haz the power, 2016-03-31 00:38:45]

Description was changed from

==========
Add a kill switch for all of Web Bluetooth.

This is temporary until we get Web Bluetooth behind
PermissionContextBase, but that may block on getting some UI done, so I
don't want to bet on it happening by M51.

BUG=598059
==========

to

==========
Add a kill switch for all of Web Bluetooth.

This is temporary until we get Web Bluetooth behind
PermissionContextBase, but that may block on getting some UI done, so I
don't want to bet on it happening by M51.

BUG=598059
==========

[commit-bot: I haz the power, 2016-03-31 00:38:46]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-03-31 00:41:17]

Description was changed from

==========
Add a kill switch for all of Web Bluetooth.

This is temporary until we get Web Bluetooth behind
PermissionContextBase, but that may block on getting some UI done, so I
don't want to bet on it happening by M51.

BUG=598059
==========

to

==========
Add a kill switch for all of Web Bluetooth.

This is temporary until we get Web Bluetooth behind
PermissionContextBase, but that may block on getting some UI done, so I
don't want to bet on it happening by M51.

BUG=598059

Committed: https://crrev.com/c993ce8db282755a85b9671638ba24de0c5ca946
Cr-Commit-Position: refs/heads/master@{#384162}
==========

[commit-bot: I haz the power, 2016-03-31 00:41:19]

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