Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2014473002:
bluetooth: Web Bluetooth can filter by empty device names, doesn't leak MACs. (Closed)
|
Created:
4 years, 7 months ago by scheib Modified:
4 years, 4 months ago CC:
blink-reviews, chromium-reviews, darin-cc_chromium.org, jam, jochen+watch_chromium.org, mkwst+moarreviews-renderer_chromium.org, mkwst+moarreviews-shell_chromium.org, mlamouri+watch-content_chromium.org, mlamouri+watch-test-runner_chromium.org, ortuno+watch_chromium.org, Peter Beverloo, scheib+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@bt-GetNameOrEmpty- Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionbluetooth: Web Bluetooth can filter by empty device names, doesn't leak MACs.
Previously BluetoothDevice::GetName was used, which had two side effects:
- Filters for an empty device name did not work.
- Device names returned to web pages included generic descriptions including
the device address.
This patch changes the filter logic and the returned device name to use
BluetoothDevice::GetNameOrEmpty, which will provide the raw name from
devices without modification.
http://crrev.com/2017393002 GetName -> GetNameForDisplay
http://crrev.com/2009753002 New GetName
http://crrev.com/2014473002 Web Bluetooth updated <<< This change.
http://crrev.com/2020923002 Arc Bridge updated
BUG=586438
Committed: https://crrev.com/6d0965788c483ec5377c20943c1ceedfd3669a51
Cr-Commit-Position: refs/heads/master@{#409312}
Patch Set 1 : Merge #
Total comments: 9
Patch Set 2 : Merge TOT #Patch Set 3 : Correct null handling in renderer; Null and Empty Named fakes & tests. #
Total comments: 24
Patch Set 4 : Remove optional from arguments; address other ortuno comments #
Total comments: 8
Patch Set 5 : addressed nits #Depends on Patchset: Dependent Patchsets: Messages
Total messages: 74 (60 generated)
scheib@chromium.org changed reviewers: + ortuno@chromium.org
Description was changed from ========== bluetooth: Web Bluetooth can filter by empty device names, doesn't leak MACs. Previously BluetoothDevice::GetName was used, which had two side effects: - Filters for an empty device name did not work. - Device names returned to web pages included generic descriptions including the device address. This patch changes the filter logic and the returned device name to use BluetoothDevice::GetNameOrEmpty, which will provide the raw name from devices without modification. BUG=586438 ========== to ========== bluetooth: Web Bluetooth can filter by empty device names, doesn't leak MACs. Previously BluetoothDevice::GetName was used, which had two side effects: - Filters for an empty device name did not work. - Device names returned to web pages included generic descriptions including the device address. This patch changes the filter logic and the returned device name to use BluetoothDevice::GetNameOrEmpty, which will provide the raw name from devices without modification. http://crrev.com/2017393002 GetName -> GetNameForDisplay http://crrev.com/2009753002 New GetName http://crrev.com/2014473002 Web Bluetooth updated <<< This change. http://crrev.com/2020923002 Arc Bridge updated BUG=586438 ==========
The CQ bit was checked by scheib@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
The CQ bit was checked by scheib@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org to run a CQ dry run
Dry run: Try jobs failed on following builders: win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
The CQ bit was checked by scheib@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by scheib@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by scheib@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Patchset #6 (id:100001) has been deleted
Patchset #1 (id:1) has been deleted
The CQ bit was checked by scheib@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:60001) has been deleted
Patchset #1 (id:80001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
https://codereview.chromium.org/2014473002/diff/120001/content/browser/blueto... File content/browser/bluetooth/bluetooth_device_chooser_controller.cc (right): https://codereview.chromium.org/2014473002/diff/120001/content/browser/blueto... content/browser/bluetooth/bluetooth_device_chooser_controller.cc:93: if (device.GetName()) { If a device doesn't have a name and name is the only member in our filter, we will match the device. Which I think is wrong. I think the logic should be: if (!filter->name.is_null()) { if (!(device.GetName() && filter->name == device.GetName().value())) { return false; } } if (!filter->namePrefix.is_null()) { if (!(device.GetName() && base::StartsWith(filter->name_prefix.get(), device.GetName().value()))) { return false; } } Which is requires some thinking so I think we need some exhaustive tests for this. https://codereview.chromium.org/2014473002/diff/120001/content/browser/blueto... File content/browser/bluetooth/web_bluetooth_service_impl.cc (right): https://codereview.chromium.org/2014473002/diff/120001/content/browser/blueto... content/browser/bluetooth/web_bluetooth_service_impl.cc:787: device_ptr->name = device->GetName().value(); optional nit to save one line: device_ptr->name = device->GetName() ? device->GetName().value() : nullptr; Also now that this can be null we need to make sure the renderer side (web_bluetooth_impl) handles it correctly. I see two options: 1. Keep web_bluetooth_impl.cc:192 as is. Meaning we rely on std::string::data to be null terminated, which I'm not sure we can rely on[1], to produce a null WebString. 2. Change web_bluetooth_impl.cc:192 to make it explicit that a null name returns a null wtf::String e.g. device->name.is_null() ? blink::WebString() : blink::WebString::fromUTF8(device->name). [1] http://en.cppreference.com/w/cpp/string/basic_string/data https://codereview.chromium.org/2014473002/diff/120001/content/shell/browser/... File content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc (right): https://codereview.chromium.org/2014473002/diff/120001/content/shell/browser/... content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc:448: GetHeartRateDevice(adapter.get(), "")); Well this is not really an Unnamed device is more of a device with an empty name. Which means all the code in which device->GetName() is false is not being tested. https://codereview.chromium.org/2014473002/diff/120001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/bluetooth/requestDevice/unnamed-device.html (right): https://codereview.chromium.org/2014473002/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/bluetooth/requestDevice/unnamed-device.html:43: }, 'An unnamed device can be obtained by empty name.'); Hmm well the spec says that if the device is not present we interpret that as a mismatch so this should not be true.
The CQ bit was checked by scheib@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by scheib@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #3 (id:160001) has been deleted
The CQ bit was checked by scheib@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2014473002/diff/120001/content/browser/blueto... File content/browser/bluetooth/bluetooth_device_chooser_controller.cc (right): https://codereview.chromium.org/2014473002/diff/120001/content/browser/blueto... content/browser/bluetooth/bluetooth_device_chooser_controller.cc:93: if (device.GetName()) { On 2016/07/21 17:42:29, ortuno wrote: > If a device doesn't have a name and name is the only member in our filter, we > will match the device. Which I think is wrong. I think the logic should be: > > if (!filter->name.is_null()) { > if (!(device.GetName() && filter->name == device.GetName().value())) { > return false; > } > } > > if (!filter->namePrefix.is_null()) { > if (!(device.GetName() && base::StartsWith(filter->name_prefix.get(), > device.GetName().value()))) { > return false; > } > } > > Which is requires some thinking so I think we need some exhaustive tests for > this. Done. https://codereview.chromium.org/2014473002/diff/120001/content/browser/blueto... File content/browser/bluetooth/web_bluetooth_service_impl.cc (right): https://codereview.chromium.org/2014473002/diff/120001/content/browser/blueto... content/browser/bluetooth/web_bluetooth_service_impl.cc:787: device_ptr->name = device->GetName().value(); On 2016/07/21 17:42:29, ortuno wrote: > optional nit to save one line: device_ptr->name = device->GetName() ? > device->GetName().value() : nullptr; > > Also now that this can be null we need to make sure the renderer side > (web_bluetooth_impl) handles it correctly. I see two options: > > 1. Keep web_bluetooth_impl.cc:192 as is. Meaning we rely on std::string::data to > be null terminated, which I'm not sure we can rely on[1], to produce a null > WebString. > 2. Change web_bluetooth_impl.cc:192 to make it explicit that a null name returns > a null wtf::String e.g. device->name.is_null() ? blink::WebString() : > blink::WebString::fromUTF8(device->name). > > [1] http://en.cppreference.com/w/cpp/string/basic_string/data Changed to always assign -- to make the assignment clear, but required type casting doesn't save the line. Renderer also updated to correctly handle null. https://codereview.chromium.org/2014473002/diff/120001/content/shell/browser/... File content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc (right): https://codereview.chromium.org/2014473002/diff/120001/content/shell/browser/... content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc:448: GetHeartRateDevice(adapter.get(), "")); On 2016/07/21 17:42:29, ortuno wrote: > Well this is not really an Unnamed device is more of a device with an empty > name. Which means all the code in which device->GetName() is false is not being > tested. Done - created unnamed and empty named versions. https://codereview.chromium.org/2014473002/diff/120001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/bluetooth/requestDevice/unnamed-device.html (right): https://codereview.chromium.org/2014473002/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/bluetooth/requestDevice/unnamed-device.html:43: }, 'An unnamed device can be obtained by empty name.'); On 2016/07/21 17:42:29, ortuno wrote: > Hmm well the spec says that if the device is not present we interpret that as a > mismatch so this should not be true. Renamed adapter to be 'EmptyNameHeartRateAdapter', and this filter should match an empty name.
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
Try jobs complain that we don't have string resources, which the empty name is causing to be required for GetNameForDisplay. ;| I'll have to come back to solving that.
I'm surprised by the resources error. Our mocks should be overriding the GetNameForDisplay function... https://codereview.chromium.org/2014473002/diff/120001/content/browser/blueto... File content/browser/bluetooth/web_bluetooth_service_impl.cc (right): https://codereview.chromium.org/2014473002/diff/120001/content/browser/blueto... content/browser/bluetooth/web_bluetooth_service_impl.cc:787: device_ptr->name = device->GetName().value(); On 2016/07/30 at 02:38:14, scheib wrote: > On 2016/07/21 17:42:29, ortuno wrote: > > optional nit to save one line: device_ptr->name = device->GetName() ? > > device->GetName().value() : nullptr; > > > > Also now that this can be null we need to make sure the renderer side > > (web_bluetooth_impl) handles it correctly. I see two options: > > > > 1. Keep web_bluetooth_impl.cc:192 as is. Meaning we rely on std::string::data to > > be null terminated, which I'm not sure we can rely on[1], to produce a null > > WebString. > > 2. Change web_bluetooth_impl.cc:192 to make it explicit that a null name returns > > a null wtf::String e.g. device->name.is_null() ? blink::WebString() : > > blink::WebString::fromUTF8(device->name). > > > > [1] http://en.cppreference.com/w/cpp/string/basic_string/data > > Changed to always assign -- to make the assignment clear, but required type casting doesn't save the line. > I'm surprised it requires type casting since the code before it directly passed in a std::string. > Renderer also updated to correctly handle null. https://codereview.chromium.org/2014473002/diff/180001/content/browser/blueto... File content/browser/bluetooth/bluetooth_device_chooser_controller.cc (right): https://codereview.chromium.org/2014473002/diff/180001/content/browser/blueto... content/browser/bluetooth/bluetooth_device_chooser_controller.cc:72: if (filter->name.size() > kMaxLengthForDeviceName) I don't think we should remove the is_null. Once we move to base::Optional<std::string> this will become filter->name->size() and will DCHECK when there is no name. https://codereview.chromium.org/2014473002/diff/180001/content/browser/blueto... content/browser/bluetooth/bluetooth_device_chooser_controller.cc:76: if (filter->name_prefix.size() > kMaxLengthForDeviceName) Same here. I don't think we should remove the check of is_null. https://codereview.chromium.org/2014473002/diff/180001/content/browser/blueto... content/browser/bluetooth/bluetooth_device_chooser_controller.cc:92: CHECK(!IsEmptyOrInvalidFilter(filter)); Not sure if we actually need this CHECK. We already crash the renderer on invalid filters. https://codereview.chromium.org/2014473002/diff/180001/content/browser/blueto... content/browser/bluetooth/bluetooth_device_chooser_controller.cc:101: if (filter->name_prefix.size()) { Same here. I don't think we should change the is_null check. https://codereview.chromium.org/2014473002/diff/180001/content/shell/browser/... File content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc (right): https://codereview.chromium.org/2014473002/diff/180001/content/shell/browser/... content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc:454: GetHeartRateDevice(adapter.get(), /* device_name */ std::string(""))); optional nit: you can use base::EmptyString() https://cs.chromium.org/chromium/src/base/strings/string_util.h?q=emptystring... https://codereview.chromium.org/2014473002/diff/180001/content/shell/browser/... File content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.h (right): https://codereview.chromium.org/2014473002/diff/180001/content/shell/browser/... content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.h:413: static std::unique_ptr<testing::NiceMock<device::MockBluetoothDevice>> In the base::Optional documentation it says that we shouldn't use optional as a function parameter[1] and should use a pointer instead. This will also avoid having two variations of many functions. Also perhaps we don't need a default value since all callers of this function pass in a name. [1] https://chromium.googlesource.com/chromium/src/+/master/docs/optional.md#When... https://codereview.chromium.org/2014473002/diff/180001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/bluetooth/requestDevice/name-empty-device.html (right): https://codereview.chromium.org/2014473002/diff/180001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/bluetooth/requestDevice/name-empty-device.html:7: promise_test(() => { Please put each test in a separate file. https://codereview.chromium.org/2014473002/diff/180001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/bluetooth/requestDevice/name-empty-device.html:16: .then(gattServer => gattServer.getPrimaryService('generic_access')) Why do you need to read the characteristic's value? Seems unrelated to this change. https://codereview.chromium.org/2014473002/diff/180001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/bluetooth/requestDevice/name-empty-device.html:35: .then(gattServer => gattServer.getPrimaryService('generic_access')) Same here. Not sure why you need to read the name. https://codereview.chromium.org/2014473002/diff/180001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/bluetooth/requestDevice/name-missing-device.html (right): https://codereview.chromium.org/2014473002/diff/180001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/bluetooth/requestDevice/name-missing-device.html:6: 'use strict'; Please put each test in a separate file. https://codereview.chromium.org/2014473002/diff/180001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/bluetooth/requestDevice/name-missing-device.html:11: optionalServices: ['generic_access']})) nit: No need for optionalServices. https://codereview.chromium.org/2014473002/diff/180001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/bluetooth/requestDevice/name-missing-device.html:45: }, 'An unnamed device can not be obtained by empty namePrefix filter.'); well it's not really an empty namePrefix ;)
The CQ bit was checked by scheib@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by scheib@chromium.org to run a CQ dry run
Patchset #4 (id:200001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2014473002/diff/180001/content/browser/blueto... File content/browser/bluetooth/bluetooth_device_chooser_controller.cc (right): https://codereview.chromium.org/2014473002/diff/180001/content/browser/blueto... content/browser/bluetooth/bluetooth_device_chooser_controller.cc:72: if (filter->name.size() > kMaxLengthForDeviceName) On 2016/07/31 18:00:05, ortuno wrote: > I don't think we should remove the is_null. Once we move to > base::Optional<std::string> this will become filter->name->size() and will > DCHECK when there is no name. Done. https://codereview.chromium.org/2014473002/diff/180001/content/browser/blueto... content/browser/bluetooth/bluetooth_device_chooser_controller.cc:76: if (filter->name_prefix.size() > kMaxLengthForDeviceName) On 2016/07/31 18:00:05, ortuno wrote: > Same here. I don't think we should remove the check of is_null. Done. https://codereview.chromium.org/2014473002/diff/180001/content/browser/blueto... content/browser/bluetooth/bluetooth_device_chooser_controller.cc:92: CHECK(!IsEmptyOrInvalidFilter(filter)); On 2016/07/31 18:00:05, ortuno wrote: > Not sure if we actually need this CHECK. We already crash the renderer on > invalid filters. Ok, removing the DCHECK here. MatchesFilters calls it, as does GetDevice(). https://codereview.chromium.org/2014473002/diff/180001/content/browser/blueto... content/browser/bluetooth/bluetooth_device_chooser_controller.cc:101: if (filter->name_prefix.size()) { On 2016/07/31 18:00:05, ortuno wrote: > Same here. I don't think we should change the is_null check. Done. https://codereview.chromium.org/2014473002/diff/180001/content/shell/browser/... File content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc (right): https://codereview.chromium.org/2014473002/diff/180001/content/shell/browser/... content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc:454: GetHeartRateDevice(adapter.get(), /* device_name */ std::string(""))); On 2016/07/31 18:00:05, ortuno wrote: > optional nit: you can use base::EmptyString() > > https://cs.chromium.org/chromium/src/base/strings/string_util.h?q=emptystring... No longer needed, but also FYI reading the comment they do not encourage using them for arguments as you suggest. https://codereview.chromium.org/2014473002/diff/180001/content/shell/browser/... File content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.h (right): https://codereview.chromium.org/2014473002/diff/180001/content/shell/browser/... content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.h:413: static std::unique_ptr<testing::NiceMock<device::MockBluetoothDevice>> On 2016/07/31 18:00:05, ortuno wrote: > In the base::Optional documentation it says that we shouldn't use optional as a > function parameter[1] and should use a pointer instead. This will also avoid > having two variations of many functions. Also perhaps we don't need a default > value since all callers of this function pass in a name. > > > [1] > https://chromium.googlesource.com/chromium/src/+/master/docs/optional.md#When... Done. https://codereview.chromium.org/2014473002/diff/180001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/bluetooth/requestDevice/name-empty-device.html (right): https://codereview.chromium.org/2014473002/diff/180001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/bluetooth/requestDevice/name-empty-device.html:7: promise_test(() => { On 2016/07/31 18:00:05, ortuno wrote: > Please put each test in a separate file. Done. https://codereview.chromium.org/2014473002/diff/180001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/bluetooth/requestDevice/name-empty-device.html:16: .then(gattServer => gattServer.getPrimaryService('generic_access')) On 2016/07/31 18:00:05, ortuno wrote: > Why do you need to read the characteristic's value? Seems unrelated to this > change. Done. https://codereview.chromium.org/2014473002/diff/180001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/bluetooth/requestDevice/name-empty-device.html:35: .then(gattServer => gattServer.getPrimaryService('generic_access')) On 2016/07/31 18:00:05, ortuno wrote: > Same here. Not sure why you need to read the name. Done. https://codereview.chromium.org/2014473002/diff/180001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/bluetooth/requestDevice/name-missing-device.html (right): https://codereview.chromium.org/2014473002/diff/180001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/bluetooth/requestDevice/name-missing-device.html:6: 'use strict'; On 2016/07/31 18:00:06, ortuno wrote: > Please put each test in a separate file. Done. https://codereview.chromium.org/2014473002/diff/180001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/bluetooth/requestDevice/name-missing-device.html:11: optionalServices: ['generic_access']})) On 2016/07/31 18:00:06, ortuno wrote: > nit: No need for optionalServices. Done. https://codereview.chromium.org/2014473002/diff/180001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/bluetooth/requestDevice/name-missing-device.html:45: }, 'An unnamed device can not be obtained by empty namePrefix filter.'); On 2016/07/31 18:00:05, ortuno wrote: > well it's not really an empty namePrefix ;) Done.
lgtm with some optional nits. https://codereview.chromium.org/2014473002/diff/220001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/bluetooth/requestDevice/name-empty-device-from-name-empty-filter.html (right): https://codereview.chromium.org/2014473002/diff/220001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/bluetooth/requestDevice/name-empty-device-from-name-empty-filter.html:9: .then(() => requestDeviceWithKeyDown({ optional nit: only two spaces. https://codereview.chromium.org/2014473002/diff/220001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/bluetooth/requestDevice/name-empty-device-from-name-empty-filter.html:14: return device.gatt.connect(); Out of curiosity: Why do you connect to the device? https://codereview.chromium.org/2014473002/diff/220001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/bluetooth/requestDevice/name-empty-device-from-service-filter.html (right): https://codereview.chromium.org/2014473002/diff/220001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/bluetooth/requestDevice/name-empty-device-from-service-filter.html:8: return setBluetoothFakeAdapter('EmptyNameHeartRateAdapter') optional nit: only two spaces. https://codereview.chromium.org/2014473002/diff/220001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/bluetooth/requestDevice/name-missing-device-from-service-filter.html (right): https://codereview.chromium.org/2014473002/diff/220001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/bluetooth/requestDevice/name-missing-device-from-service-filter.html:8: return setBluetoothFakeAdapter('NoNameHeartRateAdapter') optional nit: only two spaces.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by scheib@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2014473002/diff/220001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/bluetooth/requestDevice/name-empty-device-from-name-empty-filter.html (right): https://codereview.chromium.org/2014473002/diff/220001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/bluetooth/requestDevice/name-empty-device-from-name-empty-filter.html:9: .then(() => requestDeviceWithKeyDown({ On 2016/08/02 04:13:40, ortuno wrote: > optional nit: only two spaces. Done. https://codereview.chromium.org/2014473002/diff/220001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/bluetooth/requestDevice/name-empty-device-from-name-empty-filter.html:14: return device.gatt.connect(); On 2016/08/02 04:13:40, ortuno wrote: > Out of curiosity: Why do you connect to the device? Done. Tests previously did more, checking the name via gap as well to ensure it was consistent. https://codereview.chromium.org/2014473002/diff/220001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/bluetooth/requestDevice/name-empty-device-from-service-filter.html (right): https://codereview.chromium.org/2014473002/diff/220001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/bluetooth/requestDevice/name-empty-device-from-service-filter.html:8: return setBluetoothFakeAdapter('EmptyNameHeartRateAdapter') On 2016/08/02 04:13:40, ortuno wrote: > optional nit: only two spaces. Done. https://codereview.chromium.org/2014473002/diff/220001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/bluetooth/requestDevice/name-missing-device-from-service-filter.html (right): https://codereview.chromium.org/2014473002/diff/220001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/bluetooth/requestDevice/name-missing-device-from-service-filter.html:8: return setBluetoothFakeAdapter('NoNameHeartRateAdapter') On 2016/08/02 04:13:40, ortuno wrote: > optional nit: only two spaces. Done.
scheib@chromium.org changed reviewers: + tsepez@chromium.org
tsepez: OWNERS check of mojom file, please.
Mojo RS LGTM on making parameter optional.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by scheib@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from ortuno@chromium.org Link to the patchset: https://codereview.chromium.org/2014473002/#ps240001 (title: "addressed nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== bluetooth: Web Bluetooth can filter by empty device names, doesn't leak MACs. Previously BluetoothDevice::GetName was used, which had two side effects: - Filters for an empty device name did not work. - Device names returned to web pages included generic descriptions including the device address. This patch changes the filter logic and the returned device name to use BluetoothDevice::GetNameOrEmpty, which will provide the raw name from devices without modification. http://crrev.com/2017393002 GetName -> GetNameForDisplay http://crrev.com/2009753002 New GetName http://crrev.com/2014473002 Web Bluetooth updated <<< This change. http://crrev.com/2020923002 Arc Bridge updated BUG=586438 ========== to ========== bluetooth: Web Bluetooth can filter by empty device names, doesn't leak MACs. Previously BluetoothDevice::GetName was used, which had two side effects: - Filters for an empty device name did not work. - Device names returned to web pages included generic descriptions including the device address. This patch changes the filter logic and the returned device name to use BluetoothDevice::GetNameOrEmpty, which will provide the raw name from devices without modification. http://crrev.com/2017393002 GetName -> GetNameForDisplay http://crrev.com/2009753002 New GetName http://crrev.com/2014473002 Web Bluetooth updated <<< This change. http://crrev.com/2020923002 Arc Bridge updated BUG=586438 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:240001)
Message was sent while issue was closed.
Description was changed from ========== bluetooth: Web Bluetooth can filter by empty device names, doesn't leak MACs. Previously BluetoothDevice::GetName was used, which had two side effects: - Filters for an empty device name did not work. - Device names returned to web pages included generic descriptions including the device address. This patch changes the filter logic and the returned device name to use BluetoothDevice::GetNameOrEmpty, which will provide the raw name from devices without modification. http://crrev.com/2017393002 GetName -> GetNameForDisplay http://crrev.com/2009753002 New GetName http://crrev.com/2014473002 Web Bluetooth updated <<< This change. http://crrev.com/2020923002 Arc Bridge updated BUG=586438 ========== to ========== bluetooth: Web Bluetooth can filter by empty device names, doesn't leak MACs. Previously BluetoothDevice::GetName was used, which had two side effects: - Filters for an empty device name did not work. - Device names returned to web pages included generic descriptions including the device address. This patch changes the filter logic and the returned device name to use BluetoothDevice::GetNameOrEmpty, which will provide the raw name from devices without modification. http://crrev.com/2017393002 GetName -> GetNameForDisplay http://crrev.com/2009753002 New GetName http://crrev.com/2014473002 Web Bluetooth updated <<< This change. http://crrev.com/2020923002 Arc Bridge updated BUG=586438 Committed: https://crrev.com/6d0965788c483ec5377c20943c1ceedfd3669a51 Cr-Commit-Position: refs/heads/master@{#409312} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/6d0965788c483ec5377c20943c1ceedfd3669a51 Cr-Commit-Position: refs/heads/master@{#409312} |
