Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2658473002:
Refactor BluetoothAllowedDevicesMap (Closed)
|
Created:
3 years, 11 months ago by juncai Modified:
3 years, 10 months ago Reviewers:
Dirk Pranke, Reilly Grant (use Gerrit), sky, Elliot Glaysher, ortuno, jam, dcheng, scheib, jam1 CC:
chromium-apps-reviews_chromium.org, chromium-reviews, darin-cc_chromium.org, extensions-reviews_chromium.org, jam, jochen+watch_chromium.org, mlamouri+watch-content_chromium.org, ortuno+watch_chromium.org, Peter Beverloo, scheib+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRefactor BluetoothAllowedDevicesMap
This CL makes each browser profile have a BluetoothAllowedDevicesMap. And
in the map it has 1:1 mapping from origin to BluetoothAllowedDevices.
BUG=610343
Review-Url: https://codereview.chromium.org/2658473002
Cr-Commit-Position: refs/heads/master@{#449793}
Committed: https://chromium.googlesource.com/chromium/src/+/f70c51172084e2166d9e56acb3e13ce562a1cc45
Patch Set 1 : refactor BluetoothAllowedDevicesMap #Patch Set 2 : changed --similairty when did git cl upload #Patch Set 3 : set --similarity=20 when did git cl upload #
Total comments: 20
Patch Set 4 : address comments #Patch Set 5 : updated test code #
Total comments: 14
Patch Set 6 : merge master #Patch Set 7 : resolve conflicts #Patch Set 8 : address comments #Patch Set 9 : address comments #Patch Set 10 : address comments #Patch Set 11 : address comments #Patch Set 12 : clean up code #
Total comments: 12
Patch Set 13 : added layout test and address comments #Patch Set 14 : merge master #Patch Set 15 : cleaned up layout test code #
Total comments: 4
Messages
Total messages: 85 (51 generated)
The CQ bit was checked by juncai@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...
Description was changed from ========== Refactor BluetoothAllowedDevicesMap This CL makes each profile have a BluetoothAllowedDevicesMap. And in the map it has 1:1 mapping from origin to BluetoothAllowedDevices. BUG=610343 ========== to ========== Refactor BluetoothAllowedDevicesMap This CL makes each browser profile have a BluetoothAllowedDevicesMap. And in the map it has 1:1 mapping from origin to BluetoothAllowedDevices. BUG=610343 ==========
juncai@chromium.org changed reviewers: + scheib@chromium.org
Please take a look.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks. It's hard to review the files that were renamed because they don't diff
to the old files. git cl upload has an option:
--similarity=SIM Sets the percentage that a pair of files need to match
in order to be considered copies (default 50)
Please set that to a very low value (e.g. 10, or 5) and upload again. Check to
see if the files are matching in codereview.
The CQ bit was checked by juncai@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...
On 2017/01/25 06:18:17, scheib wrote: > Thanks. It's hard to review the files that were renamed because they don't diff > to the old files. git cl upload has an option: > --similarity=SIM Sets the percentage that a pair of files need to match > in order to be considered copies (default 50) > Please set that to a very low value (e.g. 10, or 5) and upload again. Check to > see if the files are matching in codereview. Done. Thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
We need to keep the unique origin CHECK and related tests. https://codereview.chromium.org/2658473002/diff/40001/content/browser/bluetoo... File content/browser/bluetooth/bluetooth_allowed_devices.cc (left): https://codereview.chromium.org/2658473002/diff/40001/content/browser/bluetoo... content/browser/bluetooth/bluetooth_allowed_devices.cc:32: CHECK(!origin.unique()); This must move to BluetoothAllowedDevicesMapBase https://codereview.chromium.org/2658473002/diff/40001/content/browser/bluetoo... File content/browser/bluetooth/bluetooth_allowed_devices.h (right): https://codereview.chromium.org/2658473002/diff/40001/content/browser/bluetoo... content/browser/bluetooth/bluetooth_allowed_devices.h:25: // Keeps track of which devices and their services are allowed to access. Tracks the devices and their services that a site has been granted access to. https://codereview.chromium.org/2658473002/diff/40001/content/browser/bluetoo... content/browser/bluetooth/bluetooth_allowed_devices.h:36: // devices. Generates and returns a device id. a new random device ID so that devices IDs can not be compared between sites. https://codereview.chromium.org/2658473002/diff/40001/content/browser/bluetoo... content/browser/bluetooth/bluetooth_allowed_devices.h:45: // Returns the Bluetooth Device's id if the device is allowed to access. id if it has been added previously with |AddDevice|. https://codereview.chromium.org/2658473002/diff/40001/content/browser/bluetoo... File content/browser/bluetooth/bluetooth_allowed_devices_unittest.cc (left): https://codereview.chromium.org/2658473002/diff/40001/content/browser/bluetoo... content/browser/bluetooth/bluetooth_allowed_devices_unittest.cc:48: TEST_F(BluetoothAllowedDevicesMapTest, UniqueOriginNotSupported) { Testing of unique origins should be handled somewhere. Maybe not in this file as the allowed devices no longer deals with this concern, but maybe here anyway since BluetoothAllowedDevicesMapBase is so tightly related to BluetoothAllowedDevices anyway. https://codereview.chromium.org/2658473002/diff/40001/content/browser/bluetoo... content/browser/bluetooth/bluetooth_allowed_devices_unittest.cc:106: TEST_F(BluetoothAllowedDevicesMapTest, AddTwoDevicesFromTwoOriginsToMap) { Let's find a new home for this test as well, verifying that origins that gain the ID string still can not access the other origin's device. https://codereview.chromium.org/2658473002/diff/40001/content/browser/bluetoo... content/browser/bluetooth/bluetooth_allowed_devices_unittest.cc:359: TEST_F(BluetoothAllowedDevicesMapTest, AllowedServices_TwoOriginsOneDevice) { ditto https://codereview.chromium.org/2658473002/diff/40001/content/browser/bluetoo... File content/browser/bluetooth/web_bluetooth_service_impl.cc (right): https://codereview.chromium.org/2658473002/diff/40001/content/browser/bluetoo... content/browser/bluetooth/web_bluetooth_service_impl.cc:179: DCHECK(delegate); These first 2 DCHECKs can be removed. The last will suffice. If pointers are null we'll get a crash for the use of null pointer and will be able to see it clearly. https://codereview.chromium.org/2658473002/diff/40001/content/public/browser/... File content/public/browser/bluetooth_allowed_devices_map_base.cc (right): https://codereview.chromium.org/2658473002/diff/40001/content/public/browser/... content/public/browser/bluetooth_allowed_devices_map_base.cc:19: iter = origin_to_allowed_devices_map_.insert( The unique origin problem has been moved to here. https://codereview.chromium.org/2658473002/diff/40001/content/public/browser/... File content/public/browser/bluetooth_allowed_devices_map_base.h (right): https://codereview.chromium.org/2658473002/diff/40001/content/public/browser/... content/public/browser/bluetooth_allowed_devices_map_base.h:17: class CONTENT_EXPORT BluetoothAllowedDevicesMapBase { Comment explaining class and relation to other classes.
The CQ bit was checked by juncai@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/2658473002/diff/40001/content/browser/bluetoo... File content/browser/bluetooth/bluetooth_allowed_devices.cc (left): https://codereview.chromium.org/2658473002/diff/40001/content/browser/bluetoo... content/browser/bluetooth/bluetooth_allowed_devices.cc:32: CHECK(!origin.unique()); On 2017/01/26 04:27:36, scheib wrote: > This must move to BluetoothAllowedDevicesMapBase Done. https://codereview.chromium.org/2658473002/diff/40001/content/browser/bluetoo... File content/browser/bluetooth/bluetooth_allowed_devices.h (right): https://codereview.chromium.org/2658473002/diff/40001/content/browser/bluetoo... content/browser/bluetooth/bluetooth_allowed_devices.h:25: // Keeps track of which devices and their services are allowed to access. On 2017/01/26 04:27:36, scheib wrote: > Tracks the devices and their services that a site has been granted > access to. Done. https://codereview.chromium.org/2658473002/diff/40001/content/browser/bluetoo... content/browser/bluetooth/bluetooth_allowed_devices.h:36: // devices. Generates and returns a device id. On 2017/01/26 04:27:36, scheib wrote: > a new random device ID so that devices IDs can not be compared between sites. Done. https://codereview.chromium.org/2658473002/diff/40001/content/browser/bluetoo... content/browser/bluetooth/bluetooth_allowed_devices.h:45: // Returns the Bluetooth Device's id if the device is allowed to access. On 2017/01/26 04:27:36, scheib wrote: > id if it has been added previously with |AddDevice|. Done. https://codereview.chromium.org/2658473002/diff/40001/content/browser/bluetoo... File content/browser/bluetooth/bluetooth_allowed_devices_unittest.cc (left): https://codereview.chromium.org/2658473002/diff/40001/content/browser/bluetoo... content/browser/bluetooth/bluetooth_allowed_devices_unittest.cc:48: TEST_F(BluetoothAllowedDevicesMapTest, UniqueOriginNotSupported) { On 2017/01/26 04:27:36, scheib wrote: > Testing of unique origins should be handled somewhere. Maybe not in this file as > the allowed devices no longer deals with this concern, but maybe here anyway > since BluetoothAllowedDevicesMapBase is so tightly related to > BluetoothAllowedDevices anyway. Done. https://codereview.chromium.org/2658473002/diff/40001/content/browser/bluetoo... content/browser/bluetooth/bluetooth_allowed_devices_unittest.cc:106: TEST_F(BluetoothAllowedDevicesMapTest, AddTwoDevicesFromTwoOriginsToMap) { On 2017/01/26 04:27:36, scheib wrote: > Let's find a new home for this test as well, verifying that origins that gain > the ID string still can not access the other origin's device. Done. https://codereview.chromium.org/2658473002/diff/40001/content/browser/bluetoo... content/browser/bluetooth/bluetooth_allowed_devices_unittest.cc:359: TEST_F(BluetoothAllowedDevicesMapTest, AllowedServices_TwoOriginsOneDevice) { On 2017/01/26 04:27:36, scheib wrote: > ditto Done. https://codereview.chromium.org/2658473002/diff/40001/content/browser/bluetoo... File content/browser/bluetooth/web_bluetooth_service_impl.cc (right): https://codereview.chromium.org/2658473002/diff/40001/content/browser/bluetoo... content/browser/bluetooth/web_bluetooth_service_impl.cc:179: DCHECK(delegate); On 2017/01/26 04:27:36, scheib wrote: > These first 2 DCHECKs can be removed. The last will suffice. If pointers are > null we'll get a crash for the use of null pointer and will be able to see it > clearly. Done. https://codereview.chromium.org/2658473002/diff/40001/content/public/browser/... File content/public/browser/bluetooth_allowed_devices_map_base.cc (right): https://codereview.chromium.org/2658473002/diff/40001/content/public/browser/... content/public/browser/bluetooth_allowed_devices_map_base.cc:19: iter = origin_to_allowed_devices_map_.insert( On 2017/01/26 04:27:36, scheib wrote: > The unique origin problem has been moved to here. Done. https://codereview.chromium.org/2658473002/diff/40001/content/public/browser/... File content/public/browser/bluetooth_allowed_devices_map_base.h (right): https://codereview.chromium.org/2658473002/diff/40001/content/public/browser/... content/public/browser/bluetooth_allowed_devices_map_base.h:17: class CONTENT_EXPORT BluetoothAllowedDevicesMapBase { On 2017/01/26 04:27:36, scheib wrote: > Comment explaining class and relation to other classes. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM
The CQ bit was checked by juncai@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.
juncai@chromium.org changed reviewers: + dpranke@chromium.org, erg@chromium.org, jam@chromium.org, reillyg@chromium.org, sky@chromium.org
reillyg@chromium.org: Please review changes in //chrome/browser/extensions //extensions/browser erg@chromium.org: Please review changes in //chrome/browser/profiles/chrome_browser_main_extra_parts_profiles.cc dpranke@chromium.org: Please review changes in //components/test_runner sky@chromium.org: Please review changes in //chrome/browser/ui jam@chromium.org: Please review changes in //content/public //content/shell
ortuno@chromium.org changed reviewers: + ortuno@chromium.org
This is still missing layout tests...
juncai@chromium.org changed reviewers: + dcheng@chromium.org - ortuno@chromium.org
dcheng@chromium.org: Please review changes in //content/shell/common/shell_messages.h
On 2017/01/31 18:46:02, ortuno wrote: > This is still missing layout tests... Thanks! Working on it, :).
profiles lgtm
c/b/ui LGTM
lgtm https://codereview.chromium.org/2658473002/diff/80001/content/browser/bluetoo... File content/browser/bluetooth/bluetooth_allowed_devices.cc (right): https://codereview.chromium.org/2658473002/diff/80001/content/browser/bluetoo... content/browser/bluetooth/bluetooth_allowed_devices.cc:74: } nit: no braces around single-line if
https://codereview.chromium.org/2658473002/diff/80001/content/public/browser/... File content/public/browser/bluetooth_allowed_devices_map_base.cc (right): https://codereview.chromium.org/2658473002/diff/80001/content/public/browser/... content/public/browser/bluetooth_allowed_devices_map_base.cc:18: BluetoothAllowedDevicesMapBase::GetOrCreateAllowedDevices(url::Origin origin) { See https://www.chromium.org/developers/content-module/content-api for guidelines on content/public. Namely, they should be structs or interfaces. Also why is this a new separate interface, instead of just one method on WebContentsDelegate?
https://codereview.chromium.org/2658473002/diff/80001/content/public/browser/... File content/public/browser/bluetooth_allowed_devices_map_base.cc (right): https://codereview.chromium.org/2658473002/diff/80001/content/public/browser/... content/public/browser/bluetooth_allowed_devices_map_base.cc:18: BluetoothAllowedDevicesMapBase::GetOrCreateAllowedDevices(url::Origin origin) { On 2017/02/01 01:18:24, jam wrote: > See https://www.chromium.org/developers/content-module/content-api for > guidelines on content/public. Namely, they should be structs or interfaces. > > Also why is this a new separate interface, instead of just one method on > WebContentsDelegate? Here are some context: This CL makes each browser profile have one bluetooth allowed devices map, and in the map it has a 1:1 mapping of origin to BluetoothAllowedDevices. So it needs to be a KeyedService and needs to be implemented on chrome side. And the map is created by BluetoothAllowedDevicesMapFactory. Though BluetoothAllowedDevices is implemented at //content/browser/bluetooth, and the chrome side code can not use it, so it has a BluetoothAllowedDevicesMapBase class at //content/public/browser and it actually contains the mapping of origin to BluetoothAllowedDevices. And on chrome side, the BluetoothAllowedDevicesMap just needs to be a subclass of both KeyedService and BluetoothAllowedDevicesMapBase. I am not sure if it can be only a method of WebContentsDelegate. I know it is a workaround, so any suggestions to make this design better are welcome. Thanks a lot!
https://codereview.chromium.org/2658473002/diff/80001/content/browser/bluetoo... File content/browser/bluetooth/web_bluetooth_service_impl.cc (right): https://codereview.chromium.org/2658473002/diff/80001/content/browser/bluetoo... content/browser/bluetooth/web_bluetooth_service_impl.cc:1130: allowed_devices_map->GetOrCreateAllowedDevices(GetOrigin()); Is this expected to be immutable per-origin? https://codereview.chromium.org/2658473002/diff/80001/content/public/browser/... File content/public/browser/bluetooth_allowed_devices_map_base.h (right): https://codereview.chromium.org/2658473002/diff/80001/content/public/browser/... content/public/browser/bluetooth_allowed_devices_map_base.h:29: url::Origin origin); const url::Origin&
https://codereview.chromium.org/2658473002/diff/80001/content/browser/bluetoo... File content/browser/bluetooth/web_bluetooth_service_impl.cc (right): https://codereview.chromium.org/2658473002/diff/80001/content/browser/bluetoo... content/browser/bluetooth/web_bluetooth_service_impl.cc:1130: allowed_devices_map->GetOrCreateAllowedDevices(GetOrigin()); On 2017/02/01 07:54:09, dcheng wrote: > Is this expected to be immutable per-origin? The BluetoothAllowedDevices object that an origin maps to is cached after creation. But the allowed devices that the BluetoothAllowedDevices object contains can be modified, it has AddDevice(), RemoveDevice() methods, etc. to update the devices.
lgtm
https://codereview.chromium.org/2658473002/diff/80001/content/public/browser/... File content/public/browser/bluetooth_allowed_devices_map_base.cc (right): https://codereview.chromium.org/2658473002/diff/80001/content/public/browser/... content/public/browser/bluetooth_allowed_devices_map_base.cc:21: CHECK(!origin.unique()); Why is this a CHECK()? This appears to be directly reachable via a mojo interface (WebBluetoothService::RemoteServerGetPrimaryServices), and isn't explicitly checked for. If a renderer makes this mojo IPC call, won't it kill the browser? https://codereview.chromium.org/2658473002/diff/80001/content/public/browser/... File content/public/browser/bluetooth_allowed_devices_map_base.h (right): https://codereview.chromium.org/2658473002/diff/80001/content/public/browser/... content/public/browser/bluetooth_allowed_devices_map_base.h:29: url::Origin origin); On 2017/02/01 07:54:09, dcheng wrote: > const url::Origin& I think this comment still hasn't been addressed.
https://codereview.chromium.org/2658473002/diff/80001/content/public/browser/... File content/public/browser/bluetooth_allowed_devices_map_base.cc (right): https://codereview.chromium.org/2658473002/diff/80001/content/public/browser/... content/public/browser/bluetooth_allowed_devices_map_base.cc:18: BluetoothAllowedDevicesMapBase::GetOrCreateAllowedDevices(url::Origin origin) { On 2017/02/01 05:19:27, juncai wrote: > On 2017/02/01 01:18:24, jam wrote: > > See https://www.chromium.org/developers/content-module/content-api for > > guidelines on content/public. Namely, they should be structs or interfaces. > > > > Also why is this a new separate interface, instead of just one method on > > WebContentsDelegate? > > Here are some context: > This CL makes each browser profile have one bluetooth allowed devices map, and > in the map it has a 1:1 mapping of origin to BluetoothAllowedDevices. So it > needs to be a KeyedService and needs to be implemented on chrome side. And the > map is created by BluetoothAllowedDevicesMapFactory. Though > BluetoothAllowedDevices is implemented at //content/browser/bluetooth, and the > chrome side code can not use it, so it has a BluetoothAllowedDevicesMapBase > class at //content/public/browser and it actually contains the mapping of origin > to BluetoothAllowedDevices. And on chrome side, the BluetoothAllowedDevicesMap > just needs to be a subclass of both KeyedService and > BluetoothAllowedDevicesMapBase. I am not sure if it can be only a method of > WebContentsDelegate. I know it is a workaround, so any suggestions to make this > design better are welcome. Thanks a lot! This doesn't appear to be used outside of content right? If the intent is to make this per-profile, this can be done inside content by hanging this class off StoragePartitionImpl. See https://cs.chromium.org/chromium/src/content/browser/storage_partition_impl.h... as an example.
The CQ bit was checked by juncai@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_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by juncai@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 juncai@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 juncai@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.
https://codereview.chromium.org/2658473002/diff/80001/content/browser/bluetoo... File content/browser/bluetooth/bluetooth_allowed_devices.cc (right): https://codereview.chromium.org/2658473002/diff/80001/content/browser/bluetoo... content/browser/bluetooth/bluetooth_allowed_devices.cc:74: } On 2017/01/31 23:47:37, Reilly Grant wrote: > nit: no braces around single-line if In the //content/browser/bluetooth directory, the style is single line still has braces. So would like to keep it consistent with other files. https://codereview.chromium.org/2658473002/diff/80001/content/public/browser/... File content/public/browser/bluetooth_allowed_devices_map_base.cc (right): https://codereview.chromium.org/2658473002/diff/80001/content/public/browser/... content/public/browser/bluetooth_allowed_devices_map_base.cc:18: BluetoothAllowedDevicesMapBase::GetOrCreateAllowedDevices(url::Origin origin) { On 2017/02/02 06:13:27, jam wrote: > On 2017/02/01 05:19:27, juncai wrote: > > On 2017/02/01 01:18:24, jam wrote: > > > See https://www.chromium.org/developers/content-module/content-api for > > > guidelines on content/public. Namely, they should be structs or interfaces. > > > > > > Also why is this a new separate interface, instead of just one method on > > > WebContentsDelegate? > > > > Here are some context: > > This CL makes each browser profile have one bluetooth allowed devices map, and > > in the map it has a 1:1 mapping of origin to BluetoothAllowedDevices. So it > > needs to be a KeyedService and needs to be implemented on chrome side. And the > > map is created by BluetoothAllowedDevicesMapFactory. Though > > BluetoothAllowedDevices is implemented at //content/browser/bluetooth, and the > > chrome side code can not use it, so it has a BluetoothAllowedDevicesMapBase > > class at //content/public/browser and it actually contains the mapping of > origin > > to BluetoothAllowedDevices. And on chrome side, the BluetoothAllowedDevicesMap > > just needs to be a subclass of both KeyedService and > > BluetoothAllowedDevicesMapBase. I am not sure if it can be only a method of > > WebContentsDelegate. I know it is a workaround, so any suggestions to make > this > > design better are welcome. Thanks a lot! > > This doesn't appear to be used outside of content right? If the intent is to > make this per-profile, this can be done inside content by hanging this class off > StoragePartitionImpl. See > https://cs.chromium.org/chromium/src/content/browser/storage_partition_impl.h... > as an example. Thanks for the suggestion! Done. https://codereview.chromium.org/2658473002/diff/80001/content/public/browser/... content/public/browser/bluetooth_allowed_devices_map_base.cc:21: CHECK(!origin.unique()); On 2017/02/02 05:26:03, dcheng wrote: > Why is this a CHECK()? This appears to be directly reachable via a mojo > interface (WebBluetoothService::RemoteServerGetPrimaryServices), and isn't > explicitly checked for. If a renderer makes this mojo IPC call, won't it kill > the browser? When the WebBluetooth requestDevice() function is called, the code: https://cs.chromium.org/chromium/src/content/browser/bluetooth/bluetooth_devi... already checks if it is a unique origin. If so, it will display some error message. And the check happens before BluetoothAllowedDevicesMap::GetOrCreateAllowedDevices() is called. https://codereview.chromium.org/2658473002/diff/80001/content/public/browser/... File content/public/browser/bluetooth_allowed_devices_map_base.h (right): https://codereview.chromium.org/2658473002/diff/80001/content/public/browser/... content/public/browser/bluetooth_allowed_devices_map_base.h:29: url::Origin origin); On 2017/02/02 05:26:03, dcheng wrote: > On 2017/02/01 07:54:09, dcheng wrote: > > const url::Origin& > > I think this comment still hasn't been addressed. Done. https://codereview.chromium.org/2658473002/diff/80001/content/public/browser/... content/public/browser/bluetooth_allowed_devices_map_base.h:29: url::Origin origin); On 2017/02/01 07:54:09, dcheng wrote: > const url::Origin& Done.
ortuno@chromium.org changed reviewers: + ortuno@chromium.org
https://codereview.chromium.org/2658473002/diff/210001/content/browser/blueto... File content/browser/bluetooth/web_bluetooth_service_impl.cc (right): https://codereview.chromium.org/2658473002/diff/210001/content/browser/blueto... content/browser/bluetooth/web_bluetooth_service_impl.cc:1194: return allowed_devices_; Can this be null? If so we need to DCHECK that it isn't before using it. If not then just return a reference.
https://codereview.chromium.org/2658473002/diff/210001/content/browser/blueto... File content/browser/bluetooth/web_bluetooth_service_impl.cc (right): https://codereview.chromium.org/2658473002/diff/210001/content/browser/blueto... content/browser/bluetooth/web_bluetooth_service_impl.cc:1185: StoragePartitionImpl* partition = static_cast<StoragePartitionImpl*>( I suppose check with jam@, but whenever I see a static_cast I think something is broken. Why isn't the GetBluetoothAllowedDevicesMap method on the StoragePartition interface? https://codereview.chromium.org/2658473002/diff/210001/content/browser/storag... File content/browser/storage_partition_impl.h (right): https://codereview.chromium.org/2658473002/diff/210001/content/browser/storag... content/browser/storage_partition_impl.h:112: void ClearBluetoothAllowedDevicesMap() override; All the overrides should be together without new-lines between them. https://codereview.chromium.org/2658473002/diff/210001/content/public/browser... File content/public/browser/storage_partition.h (right): https://codereview.chromium.org/2658473002/diff/210001/content/public/browser... content/public/browser/storage_partition.h:170: virtual void ClearBluetoothAllowedDevicesMap() = 0; For test only methods should end with ForTesting https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...
mojo lgtm
lgtm
The CQ bit was checked by juncai@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-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
The CQ bit was checked by juncai@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.
https://codereview.chromium.org/2658473002/diff/210001/content/browser/blueto... File content/browser/bluetooth/web_bluetooth_service_impl.cc (right): https://codereview.chromium.org/2658473002/diff/210001/content/browser/blueto... content/browser/bluetooth/web_bluetooth_service_impl.cc:1185: StoragePartitionImpl* partition = static_cast<StoragePartitionImpl*>( On 2017/02/03 23:28:13, scheib wrote: > I suppose check with jam@, but whenever I see a static_cast I think something is > broken. Why isn't the GetBluetoothAllowedDevicesMap method on the > StoragePartition interface? Hi, jam@, the three functions: https://cs.chromium.org/chromium/src/content/browser/storage_partition_impl.h... https://cs.chromium.org/chromium/src/content/browser/storage_partition_impl.h... https://cs.chromium.org/chromium/src/content/browser/storage_partition_impl.h... are not part of the StoragePartition interface, just wondering why. Since GetBluetoothAllowedDevicesMap uses the similar pattern as the above three functions, and here uses static_cast, similar to: https://cs.chromium.org/chromium/src/content/browser/devtools/protocol/servic... https://codereview.chromium.org/2658473002/diff/210001/content/browser/blueto... content/browser/bluetooth/web_bluetooth_service_impl.cc:1194: return allowed_devices_; On 2017/02/03 20:16:50, ortuno wrote: > Can this be null? If so we need to DCHECK that it isn't before using it. If not > then just return a reference. It can't be null. Changed it to return a reference. Done. https://codereview.chromium.org/2658473002/diff/210001/content/browser/storag... File content/browser/storage_partition_impl.h (right): https://codereview.chromium.org/2658473002/diff/210001/content/browser/storag... content/browser/storage_partition_impl.h:112: void ClearBluetoothAllowedDevicesMap() override; On 2017/02/03 23:28:13, scheib wrote: > All the overrides should be together without new-lines between them. Done. https://codereview.chromium.org/2658473002/diff/210001/content/public/browser... File content/public/browser/storage_partition.h (right): https://codereview.chromium.org/2658473002/diff/210001/content/public/browser... content/public/browser/storage_partition.h:170: virtual void ClearBluetoothAllowedDevicesMap() = 0; On 2017/02/03 23:28:13, scheib wrote: > For test only methods should end with ForTesting > https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... Done.
The CQ bit was checked by juncai@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_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
jam@google.com changed reviewers: + jam@google.com
https://codereview.chromium.org/2658473002/diff/210001/content/browser/blueto... File content/browser/bluetooth/web_bluetooth_service_impl.cc (right): https://codereview.chromium.org/2658473002/diff/210001/content/browser/blueto... content/browser/bluetooth/web_bluetooth_service_impl.cc:1185: StoragePartitionImpl* partition = static_cast<StoragePartitionImpl*>( On 2017/02/08 06:43:49, juncai wrote: > On 2017/02/03 23:28:13, scheib wrote: > > I suppose check with jam@, but whenever I see a static_cast I think something > is > > broken. Why isn't the GetBluetoothAllowedDevicesMap method on the > > StoragePartition interface? > > Hi, jam@, the three functions: > https://cs.chromium.org/chromium/src/content/browser/storage_partition_impl.h... > https://cs.chromium.org/chromium/src/content/browser/storage_partition_impl.h... > https://cs.chromium.org/chromium/src/content/browser/storage_partition_impl.h... > are not part of the StoragePartition interface, just wondering why. Since > GetBluetoothAllowedDevicesMap uses the similar pattern as the above three > functions, and here uses static_cast, similar to: > https://cs.chromium.org/chromium/src/content/browser/devtools/protocol/servic... the public interface is for getters that are exposed outside content. for getters that are only used inside content, we cast to the impl since it's the only implementation.
https://codereview.chromium.org/2658473002/diff/210001/content/browser/blueto... File content/browser/bluetooth/web_bluetooth_service_impl.cc (right): https://codereview.chromium.org/2658473002/diff/210001/content/browser/blueto... content/browser/bluetooth/web_bluetooth_service_impl.cc:1185: StoragePartitionImpl* partition = static_cast<StoragePartitionImpl*>( On 2017/02/10 05:24:39, jam1 wrote: > On 2017/02/08 06:43:49, juncai wrote: > > On 2017/02/03 23:28:13, scheib wrote: > > > I suppose check with jam@, but whenever I see a static_cast I think > something > > is > > > broken. Why isn't the GetBluetoothAllowedDevicesMap method on the > > > StoragePartition interface? > > > > Hi, jam@, the three functions: > > > https://cs.chromium.org/chromium/src/content/browser/storage_partition_impl.h... > > > https://cs.chromium.org/chromium/src/content/browser/storage_partition_impl.h... > > > https://cs.chromium.org/chromium/src/content/browser/storage_partition_impl.h... > > are not part of the StoragePartition interface, just wondering why. Since > > GetBluetoothAllowedDevicesMap uses the similar pattern as the above three > > functions, and here uses static_cast, similar to: > > > https://cs.chromium.org/chromium/src/content/browser/devtools/protocol/servic... > > the public interface is for getters that are exposed outside content. > > for getters that are only used inside content, we cast to the impl since it's > the only implementation. That's a bit unfortunate, as it's not clear that this is the only impl possible without global knowledge of the code base. (BTW, there is a test subclass as well). Perhaps we can at least add a comment at StoragePartition that it's safe for other content code to static_cast it. Or, do something such as: """ // In public headers class Impl; class Interface { public: virtual Impl* GetImpl() = 0; }; // In implementation only headers class Impl : public Interface { public: Impl* GetImpl() override { return this; } std::string ImplOnlyMethod() { return "ImplOnlyMethod\n"; } }; // Elsewhere in implementation { Interface* i = new Impl(); // ... std::cout << i->GetImpl()->ImplOnlyMethod(); } """ Test subclasses would return null. Pro: the pattern is very clear that internals are available to anyone who has access to the Impl class definition. Con: A virtual dispatch.
https://codereview.chromium.org/2658473002/diff/210001/content/browser/blueto... File content/browser/bluetooth/web_bluetooth_service_impl.cc (right): https://codereview.chromium.org/2658473002/diff/210001/content/browser/blueto... content/browser/bluetooth/web_bluetooth_service_impl.cc:1185: StoragePartitionImpl* partition = static_cast<StoragePartitionImpl*>( On 2017/02/10 06:33:59, scheib wrote: > > That's a bit unfortunate, as it's not clear that this is the only impl possible > without global knowledge of the code base. (BTW, there is a test subclass as > well). > > Perhaps we can at least add a comment at StoragePartition that it's safe for > other content code to static_cast it. > > Or, do something such as: > """ > // In public headers > class Impl; > class Interface { > public: > virtual Impl* GetImpl() = 0; > }; > > // In implementation only headers > class Impl : public Interface { > public: > Impl* GetImpl() override { return this; } > > std::string ImplOnlyMethod() { return "ImplOnlyMethod\n"; } > }; > > // Elsewhere in implementation > { > Interface* i = new Impl(); > // ... > std::cout << i->GetImpl()->ImplOnlyMethod(); > } > """ > > Test subclasses would return null. > Pro: the pattern is very clear that internals are available to anyone who has > access to the Impl class definition. > Con: A virtual dispatch. What about adding some comments about this in a separate CL? If so, I can start a CL that does that. Or if we decide to implement them using the approach that scheib@ mentioned, I can also help with that, probably also in a separate CL. What do you think?
https://codereview.chromium.org/2658473002/diff/210001/content/browser/blueto... File content/browser/bluetooth/web_bluetooth_service_impl.cc (right): https://codereview.chromium.org/2658473002/diff/210001/content/browser/blueto... content/browser/bluetooth/web_bluetooth_service_impl.cc:1185: StoragePartitionImpl* partition = static_cast<StoragePartitionImpl*>( On 2017/02/10 22:30:43, juncai wrote: > On 2017/02/10 06:33:59, scheib wrote: > > > > That's a bit unfortunate, as it's not clear that this is the only impl > possible > > without global knowledge of the code base. (BTW, there is a test subclass as > > well). > > > > Perhaps we can at least add a comment at StoragePartition that it's safe for > > other content code to static_cast it. > > > > Or, do something such as: > > """ > > // In public headers > > class Impl; > > class Interface { > > public: > > virtual Impl* GetImpl() = 0; > > }; > > > > // In implementation only headers > > class Impl : public Interface { > > public: > > Impl* GetImpl() override { return this; } > > > > std::string ImplOnlyMethod() { return "ImplOnlyMethod\n"; } > > }; > > > > // Elsewhere in implementation > > { > > Interface* i = new Impl(); > > // ... > > std::cout << i->GetImpl()->ImplOnlyMethod(); > > } > > """ > > > > Test subclasses would return null. > > Pro: the pattern is very clear that internals are available to anyone who has > > access to the Impl class definition. > > Con: A virtual dispatch. > > What about adding some comments about this in a separate CL? If so, I can start > a CL that does that. > > Or if we decide to implement them using the approach that scheib@ mentioned, I > can also help with that, probably also in a separate CL. > > What do you think? Separate patch is OK, need to float to jam@ for thoughts, but using the existing pattern is fine.
The CQ bit was checked by juncai@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from scheib@chromium.org, dpranke@chromium.org, reillyg@chromium.org, sky@chromium.org, erg@chromium.org, jam@chromium.org, dcheng@chromium.org Link to the patchset: https://codereview.chromium.org/2658473002/#ps270001 (title: "cleaned up layout test code")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 270001, "attempt_start_ts": 1486769896355240,
"parent_rev": "31c92fe8317757707c7c910c7d67bc04fb6938be", "commit_rev":
"f70c51172084e2166d9e56acb3e13ce562a1cc45"}
Message was sent while issue was closed.
Description was changed from ========== Refactor BluetoothAllowedDevicesMap This CL makes each browser profile have a BluetoothAllowedDevicesMap. And in the map it has 1:1 mapping from origin to BluetoothAllowedDevices. BUG=610343 ========== to ========== Refactor BluetoothAllowedDevicesMap This CL makes each browser profile have a BluetoothAllowedDevicesMap. And in the map it has 1:1 mapping from origin to BluetoothAllowedDevices. BUG=610343 Review-Url: https://codereview.chromium.org/2658473002 Cr-Commit-Position: refs/heads/master@{#449793} Committed: https://chromium.googlesource.com/chromium/src/+/f70c51172084e2166d9e56acb3e1... ==========
Message was sent while issue was closed.
Committed patchset #15 (id:270001) as https://chromium.googlesource.com/chromium/src/+/f70c51172084e2166d9e56acb3e1...
Message was sent while issue was closed.
You might want to read about promises a bit: https://developers.google.com/web/fundamentals/getting-started/primers/promises https://codereview.chromium.org/2658473002/diff/270001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/resources/bluetooth/heart-rate-two-iframes.html (right): https://codereview.chromium.org/2658473002/diff/270001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/resources/bluetooth/heart-rate-two-iframes.html:29: gattServer.getPrimaryService('heart_rate'); Why are we calling getPrimaryService here? We are not doing anything with the promise so even if it rejects we won't know. https://codereview.chromium.org/2658473002/diff/270001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/resources/bluetooth/heart-rate-two-iframes.html:44: gattServer.getPrimaryService('generic_access'); Same here. The promise could reject and we would still pass the test.
Message was sent while issue was closed.
https://codereview.chromium.org/2658473002/diff/270001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/resources/bluetooth/heart-rate-two-iframes.html (right): https://codereview.chromium.org/2658473002/diff/270001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/resources/bluetooth/heart-rate-two-iframes.html:29: gattServer.getPrimaryService('heart_rate'); On 2017/02/14 02:52:28, ortuno wrote: > Why are we calling getPrimaryService here? We are not doing anything with the > promise so even if it rejects we won't know. Thanks! I'll fix it in a separate CL. https://codereview.chromium.org/2658473002/diff/270001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/resources/bluetooth/heart-rate-two-iframes.html:44: gattServer.getPrimaryService('generic_access'); On 2017/02/14 02:52:28, ortuno wrote: > Same here. The promise could reject and we would still pass the test. ditto. |
