Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1159523002:
bluetooth: Browser side partial implementation of getPrimaryService. (Closed)
|
Created:
5 years, 7 months ago by ortuno Modified:
5 years, 6 months ago CC:
chromium-reviews, darin-cc_chromium.org, jam, jochen+watch_chromium.org, mkwst+moarreviews-shell_chromium.org, mlamouri+watch-content_chromium.org, scheib+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionbluetooth: Browser side partial implementation of getPrimaryService.
This adds the browser side implementation for the getPrimaryService
function:
https://webbluetoothcg.github.io/web-bluetooth/#dom-bluetoothgattremoteserver-getprimaryservice
Updates to the Chrome Implementation Notes corresponding to this patch:
https://github.com/WebBluetoothChrome/web-bluetooth/pull/8
This change also adds the necessary mocks for Layout Tests in the following
Blink-side patch.
This is the second of three patches to implement getPrimaryService:
[1] http://crrev.com/1150253004
[2] This patch.
[3] http://crrev.com/1152393002
BUG=491399
Committed: https://crrev.com/49eb1ee2a0d9b51e39c687d66f8e530b35fe83cf
Cr-Commit-Position: refs/heads/master@{#332335}
Patch Set 1 : #
Total comments: 16
Patch Set 2 : Address jyasskin comments #
Total comments: 1
Patch Set 3 : s/sevice/service #
Total comments: 21
Patch Set 4 : Address scheib's comments #Patch Set 5 : Fixed order of gypi #
Total comments: 8
Patch Set 6 : Address scheib's comments #
Total comments: 2
Patch Set 7 : Added namespace comment and new line #
Total comments: 2
Patch Set 8 : Add const to method. #Patch Set 9 : Remove anonymous namespace #Messages
Total messages: 33 (11 generated)
Patchset #1 (id:1) has been deleted
ortuno@chromium.org changed reviewers: + jyasskin@chromium.org
@jyasskin: PTAL
https://codereview.chromium.org/1159523002/diff/20001/content/browser/bluetoo... File content/browser/bluetooth/bluetooth_dispatcher_host.cc (right): https://codereview.chromium.org/1159523002/diff/20001/content/browser/bluetoo... content/browser/bluetooth/bluetooth_dispatcher_host.cc:21: const int kDelayTime = 5; // 5 seconds for scanning and discovering Maybe add a TODO here that this is supposed to entirely go away by the time we ship, replaced by the chooser dialog and the discovery-finished callbacks? https://codereview.chromium.org/1159523002/diff/20001/content/browser/bluetoo... content/browser/bluetooth/bluetooth_dispatcher_host.cc:255: service->GetUUID().canonical_value())); There can be multiple services in a device with the same UUID, so this message to the renderer isn't enough for it to uniquely identify the instance back to the browser. Send service->GetIdentifier() instead. https://codereview.chromium.org/1159523002/diff/20001/content/browser/bluetoo... File content/browser/bluetooth/bluetooth_dispatcher_host.h (right): https://codereview.chromium.org/1159523002/diff/20001/content/browser/bluetoo... content/browser/bluetooth/bluetooth_dispatcher_host.h:92: // See: http://crbug.com/484504 Nit: Link to https://... https://codereview.chromium.org/1159523002/diff/20001/content/child/bluetooth... File content/child/bluetooth/bluetooth_dispatcher.cc (right): https://codereview.chromium.org/1159523002/diff/20001/content/child/bluetooth... content/child/bluetooth/bluetooth_dispatcher.cc:132: const blink::WebString& service_uuid, Add a TODO to check that the service_uuid is valid (initially https://webbluetoothcg.github.io/web-bluetooth/#dfn-valid-uuid, but eventually https://webbluetoothcg.github.io/web-bluetooth/#dom-bluetoothuuid-getservice). I think this can happen on the renderer side: the browser still needs to be resilient to malformed UUIDs, but it doesn't need to send the right exception back to a malicious renderer. The renderer can be responsible for producing the specified exception for bad user-provided strings. https://codereview.chromium.org/1159523002/diff/20001/content/child/bluetooth... content/child/bluetooth/bluetooth_dispatcher.cc:205: ->onSuccess(new WebBluetoothGATTService( Would putting the instance ID in here be more obvious if the spec included a private [[representedService]] field, like the [[representedDevice]] field for devices? https://codereview.chromium.org/1159523002/diff/20001/content/common/bluetoot... File content/common/bluetooth/bluetooth_messages.h (right): https://codereview.chromium.org/1159523002/diff/20001/content/common/bluetoot... content/common/bluetooth/bluetooth_messages.h:169: std::string /* service_uuid */) In the long run, we're probably going to want to coalesce multiple getPrimaryService() calls that happen in the same message loop turn, into a single request that takes a group of UUIDs. That'll reduce the number of renderer<->browser IPCs and help the browser pick the right GATT procedure to discover those services. Similarly for the other get*() calls. You don't need to make the IPC support that right now. https://codereview.chromium.org/1159523002/diff/20001/device/bluetooth/test/m... File device/bluetooth/test/mock_bluetooth_device.cc (right): https://codereview.chromium.org/1159523002/diff/20001/device/bluetooth/test/m... device/bluetooth/test/mock_bluetooth_device.cc:67: for (auto& service : mock_services_) { s/auto&/BluetoothGattService*/ https://codereview.chromium.org/1159523002/diff/20001/device/bluetooth/test/m... File device/bluetooth/test/mock_bluetooth_device.h (right): https://codereview.chromium.org/1159523002/diff/20001/device/bluetooth/test/m... device/bluetooth/test/mock_bluetooth_device.h:87: // convenience as far testing is concerned and it's possible to write test "…as far as…concerned, and it's…" or "…for convenience in testing, and it's possible…"
@jyasskin: Ready for review again. https://codereview.chromium.org/1159523002/diff/20001/content/browser/bluetoo... File content/browser/bluetooth/bluetooth_dispatcher_host.cc (right): https://codereview.chromium.org/1159523002/diff/20001/content/browser/bluetoo... content/browser/bluetooth/bluetooth_dispatcher_host.cc:21: const int kDelayTime = 5; // 5 seconds for scanning and discovering On 2015/05/27 at 18:36:59, Jeffrey Yasskin wrote: > Maybe add a TODO here that this is supposed to entirely go away by the time we ship, replaced by the chooser dialog and the discovery-finished callbacks? Done. https://codereview.chromium.org/1159523002/diff/20001/content/browser/bluetoo... content/browser/bluetooth/bluetooth_dispatcher_host.cc:255: service->GetUUID().canonical_value())); On 2015/05/27 at 18:36:59, Jeffrey Yasskin wrote: > There can be multiple services in a device with the same UUID, so this message to the renderer isn't enough for it to uniquely identify the instance back to the browser. Send service->GetIdentifier() instead. Ah. Good point. Done. https://codereview.chromium.org/1159523002/diff/20001/content/browser/bluetoo... File content/browser/bluetooth/bluetooth_dispatcher_host.h (right): https://codereview.chromium.org/1159523002/diff/20001/content/browser/bluetoo... content/browser/bluetooth/bluetooth_dispatcher_host.h:92: // See: http://crbug.com/484504 On 2015/05/27 at 18:36:59, Jeffrey Yasskin wrote: > Nit: Link to https://... Done. Someone should fix the extension so that it generates https links instead of http links. https://codereview.chromium.org/1159523002/diff/20001/content/child/bluetooth... File content/child/bluetooth/bluetooth_dispatcher.cc (right): https://codereview.chromium.org/1159523002/diff/20001/content/child/bluetooth... content/child/bluetooth/bluetooth_dispatcher.cc:132: const blink::WebString& service_uuid, On 2015/05/27 at 18:36:59, Jeffrey Yasskin wrote: > Add a TODO to check that the service_uuid is valid (initially https://webbluetoothcg.github.io/web-bluetooth/#dfn-valid-uuid, but eventually https://webbluetoothcg.github.io/web-bluetooth/#dom-bluetoothuuid-getservice). Done. > I think this can happen on the renderer side: the browser still needs to be resilient to malformed UUIDs, but it doesn't need to send the right exception back to a malicious renderer. The renderer can be responsible for producing the specified exception for bad user-provided strings. OK. Once I implement BluetoothUUID I will do the checks in the renderer. https://codereview.chromium.org/1159523002/diff/20001/content/child/bluetooth... content/child/bluetooth/bluetooth_dispatcher.cc:205: ->onSuccess(new WebBluetoothGATTService( On 2015/05/27 at 18:36:59, Jeffrey Yasskin wrote: > Would putting the instance ID in here be more obvious if the spec included a private [[representedService]] field, like the [[representedDevice]] field for devices? I don't think it would make that much of a difference. https://codereview.chromium.org/1159523002/diff/20001/content/common/bluetoot... File content/common/bluetooth/bluetooth_messages.h (right): https://codereview.chromium.org/1159523002/diff/20001/content/common/bluetoot... content/common/bluetooth/bluetooth_messages.h:169: std::string /* service_uuid */) On 2015/05/27 at 18:36:59, Jeffrey Yasskin wrote: > In the long run, we're probably going to want to coalesce multiple getPrimaryService() calls that happen in the same message loop turn, into a single request that takes a group of UUIDs. That'll reduce the number of renderer<->browser IPCs and help the browser pick the right GATT procedure to discover those services. Similarly for the other get*() calls. > > You don't need to make the IPC support that right now. Ack. https://codereview.chromium.org/1159523002/diff/20001/device/bluetooth/test/m... File device/bluetooth/test/mock_bluetooth_device.cc (right): https://codereview.chromium.org/1159523002/diff/20001/device/bluetooth/test/m... device/bluetooth/test/mock_bluetooth_device.cc:67: for (auto& service : mock_services_) { On 2015/05/27 at 18:36:59, Jeffrey Yasskin wrote: > s/auto&/BluetoothGattService*/ Done. https://codereview.chromium.org/1159523002/diff/20001/device/bluetooth/test/m... File device/bluetooth/test/mock_bluetooth_device.h (right): https://codereview.chromium.org/1159523002/diff/20001/device/bluetooth/test/m... device/bluetooth/test/mock_bluetooth_device.h:87: // convenience as far testing is concerned and it's possible to write test On 2015/05/27 at 18:36:59, Jeffrey Yasskin wrote: > "…as far as…concerned, and it's…" or "…for convenience in testing, and it's possible…" Done.
lgtm https://codereview.chromium.org/1159523002/diff/40001/content/child/bluetooth... File content/child/bluetooth/bluetooth_dispatcher.h (right): https://codereview.chromium.org/1159523002/diff/40001/content/child/bluetooth... content/child/bluetooth/bluetooth_dispatcher.h:81: const std::string& sevice_instance_id, sp: sevice->service
ortuno@chromium.org changed reviewers: + scheib@chromium.org
@jyasskin: Thanks! @scheib: PTAL
https://codereview.chromium.org/1159523002/diff/60001/content/browser/bluetoo... File content/browser/bluetooth/bluetooth_dispatcher_host.cc (right): https://codereview.chromium.org/1159523002/diff/60001/content/browser/bluetoo... content/browser/bluetooth/bluetooth_dispatcher_host.cc:138: // TODO(ortuno): Right now it's pointless to check if the domain has access to TODO: Check if device_instance_id is in "allowed devices". crbug.com/00000 TODO: Check if service__uuid is in "allowed services". crbug.com/00000 (This implies also that service_uuid is well-formed). https://codereview.chromium.org/1159523002/diff/60001/content/browser/bluetoo... content/browser/bluetooth/bluetooth_dispatcher_host.cc:142: // For now just wait two seconds and call OnServiceDiscovered. s/wait two seconds/wait a fixed time/. The constant is already changed to 5 and may likely change again. https://codereview.chromium.org/1159523002/diff/60001/content/browser/bluetoo... content/browser/bluetooth/bluetooth_dispatcher_host.cc:251: thread_id, request_id, BluetoothError::NETWORK_ERROR)); We should get the error messages returning error strings before adding more error responses. Let's do it before landing any future patch with error messages. https://codereview.chromium.org/1159523002/diff/60001/content/child/bluetooth... File content/child/bluetooth/bluetooth_dispatcher.cc (right): https://codereview.chromium.org/1159523002/diff/60001/content/child/bluetooth... content/child/bluetooth/bluetooth_dispatcher.cc:134: // TODO(ortuno): Check that service_uuid is a valid UUID. UUIDs per spec should be verified by BluetoothUUID.getService and return those exceptions. For Browser integrity the UUIDs should be re-verified in browser process. You can remove this comment. https://codereview.chromium.org/1159523002/diff/60001/content/child/bluetooth... content/child/bluetooth/bluetooth_dispatcher.cc:218: // Since we couldn't find the service return null. See Step 3 of Let's cross reference from https://github.com/WebBluetoothChrome/web-bluetooth here as well. Please add a pull request for https://github.com/WebBluetoothChrome/web-bluetooth to the change description of this patch. https://codereview.chromium.org/1159523002/diff/60001/content/child/bluetooth... File content/child/bluetooth/bluetooth_dispatcher.h (right): https://codereview.chromium.org/1159523002/diff/60001/content/child/bluetooth... content/child/bluetooth/bluetooth_dispatcher.h:78: void OnGetPrimaryServiceSuccess(int thread_id, Remove blank lines between all of these, they're all covered by comment above. https://codereview.chromium.org/1159523002/diff/60001/content/common/bluetoot... File content/common/bluetooth/bluetooth_messages.h (right): https://codereview.chromium.org/1159523002/diff/60001/content/common/bluetoot... content/common/bluetooth/bluetooth_messages.h:138: std::string /* service_uuid */) service_uuid is redundant, as this response is based on matching a UUID. I'm guessing a possible plan is to use a similar IPC response for multiple queries in the future; e.g. getPrimaryServices. If so, let's just make that message type now, or simplify this message to just what it needs for GetPrimaryServiceSuccess. https://codereview.chromium.org/1159523002/diff/60001/content/shell/browser/l... File content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc (right): https://codereview.chromium.org/1159523002/diff/60001/content/shell/browser/l... content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc:186: ON_CALL(*empty_device, GetGattServices()) This can stay on the empty device, just move the AddMockService calls to a non EmptyDevice. https://codereview.chromium.org/1159523002/diff/60001/content/shell/browser/l... File content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.h (right): https://codereview.chromium.org/1159523002/diff/60001/content/shell/browser/l... content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.h:73: // - |GetGattServices| returns a list with two services "Generic Access" and EmptyDevice doesn't seem so empty if it has Services. Perhaps these should be added to ConnectableDevice? https://codereview.chromium.org/1159523002/diff/60001/device/bluetooth/test/m... File device/bluetooth/test/mock_bluetooth_device.h (right): https://codereview.chromium.org/1159523002/diff/60001/device/bluetooth/test/m... device/bluetooth/test/mock_bluetooth_device.h:88: // cases without using these functions. Document how to use the method in tests with the code snippet: .WillByDefault(Invoke(empty_device.get(), &MockBluetoothDevice::GetMockServices));"
Patchset #4 (id:80001) has been deleted
@scheib: Ready for review again. @jyasskin: Could you take a look at the new struct, BluetoothPrimaryServiceRequest? https://codereview.chromium.org/1159523002/diff/60001/content/browser/bluetoo... File content/browser/bluetooth/bluetooth_dispatcher_host.cc (right): https://codereview.chromium.org/1159523002/diff/60001/content/browser/bluetoo... content/browser/bluetooth/bluetooth_dispatcher_host.cc:138: // TODO(ortuno): Right now it's pointless to check if the domain has access to On 2015/05/28 at 23:09:16, scheib wrote: > TODO: Check if device_instance_id is in "allowed devices". crbug.com/00000 > TODO: Check if service__uuid is in "allowed services". crbug.com/00000 (This implies also that service_uuid is well-formed). Done. https://codereview.chromium.org/1159523002/diff/60001/content/browser/bluetoo... content/browser/bluetooth/bluetooth_dispatcher_host.cc:142: // For now just wait two seconds and call OnServiceDiscovered. On 2015/05/28 at 23:09:16, scheib wrote: > s/wait two seconds/wait a fixed time/. The constant is already changed to 5 and may likely change again. Done. https://codereview.chromium.org/1159523002/diff/60001/content/browser/bluetoo... content/browser/bluetooth/bluetooth_dispatcher_host.cc:251: thread_id, request_id, BluetoothError::NETWORK_ERROR)); On 2015/05/28 at 23:09:16, scheib wrote: > We should get the error messages returning error strings before adding more error responses. Let's do it before landing any future patch with error messages. Ack. https://codereview.chromium.org/1159523002/diff/60001/content/child/bluetooth... File content/child/bluetooth/bluetooth_dispatcher.cc (right): https://codereview.chromium.org/1159523002/diff/60001/content/child/bluetooth... content/child/bluetooth/bluetooth_dispatcher.cc:134: // TODO(ortuno): Check that service_uuid is a valid UUID. On 2015/05/28 at 23:09:16, scheib wrote: > UUIDs per spec should be verified by BluetoothUUID.getService and return those exceptions. For Browser integrity the UUIDs should be re-verified in browser process. You can remove this comment. Done. https://codereview.chromium.org/1159523002/diff/60001/content/child/bluetooth... content/child/bluetooth/bluetooth_dispatcher.cc:218: // Since we couldn't find the service return null. See Step 3 of On 2015/05/28 at 23:09:16, scheib wrote: > Let's cross reference from https://github.com/WebBluetoothChrome/web-bluetooth here as well. Not sure what you mean. You want me to add a reference to the chrome fork of the spec here? Please add a pull request for https://github.com/WebBluetoothChrome/web-bluetooth to the change description of this patch. The description of this patch? https://codereview.chromium.org/1159523002/diff/60001/content/child/bluetooth... File content/child/bluetooth/bluetooth_dispatcher.h (right): https://codereview.chromium.org/1159523002/diff/60001/content/child/bluetooth... content/child/bluetooth/bluetooth_dispatcher.h:78: void OnGetPrimaryServiceSuccess(int thread_id, On 2015/05/28 at 23:09:16, scheib wrote: > Remove blank lines between all of these, they're all covered by comment above. Removed the ones this patch introduces. I will remove the rest in another CL. https://codereview.chromium.org/1159523002/diff/60001/content/common/bluetoot... File content/common/bluetooth/bluetooth_messages.h (right): https://codereview.chromium.org/1159523002/diff/60001/content/common/bluetoot... content/common/bluetooth/bluetooth_messages.h:138: std::string /* service_uuid */) On 2015/05/28 at 23:09:16, scheib wrote: > service_uuid is redundant, as this response is based on matching a UUID. I'm guessing a possible plan is to use a similar IPC response for multiple queries in the future; e.g. getPrimaryServices. If so, let's just make that message type now, or simplify this message to just what it needs for GetPrimaryServiceSuccess. Done. https://codereview.chromium.org/1159523002/diff/60001/content/shell/browser/l... File content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc (right): https://codereview.chromium.org/1159523002/diff/60001/content/shell/browser/l... content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc:186: ON_CALL(*empty_device, GetGattServices()) On 2015/05/28 at 23:09:16, scheib wrote: > This can stay on the empty device, just move the AddMockService calls to a non EmptyDevice. See comment in bluetooth_adapter_provider.h https://codereview.chromium.org/1159523002/diff/60001/content/shell/browser/l... File content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.h (right): https://codereview.chromium.org/1159523002/diff/60001/content/shell/browser/l... content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.h:73: // - |GetGattServices| returns a list with two services "Generic Access" and On 2015/05/28 at 23:09:16, scheib wrote: > EmptyDevice doesn't seem so empty if it has Services. Perhaps these should be added to ConnectableDevice? But it's already advertising two UUIDs. Also these two services are present in all BLE devices. https://codereview.chromium.org/1159523002/diff/60001/device/bluetooth/test/m... File device/bluetooth/test/mock_bluetooth_device.h (right): https://codereview.chromium.org/1159523002/diff/60001/device/bluetooth/test/m... device/bluetooth/test/mock_bluetooth_device.h:88: // cases without using these functions. On 2015/05/28 at 23:09:16, scheib wrote: > Document how to use the method in tests with the code snippet: .WillByDefault(Invoke(empty_device.get(), &MockBluetoothDevice::GetMockServices));" Done.
Still LGTM https://codereview.chromium.org/1159523002/diff/120001/content/child/bluetoot... File content/child/bluetooth/bluetooth_dispatcher.h (right): https://codereview.chromium.org/1159523002/diff/120001/content/child/bluetoot... content/child/bluetooth/bluetooth_dispatcher.h:96: // Owns callback objects. They aren't callback objects anymore. :) https://codereview.chromium.org/1159523002/diff/120001/content/child/bluetoot... File content/child/bluetooth/bluetooth_primary_service_request.h (right): https://codereview.chromium.org/1159523002/diff/120001/content/child/bluetoot... content/child/bluetooth/bluetooth_primary_service_request.h:13: struct BluetoothPrimaryServiceRequest { I'd default to just defining this in the single .cc file that it's used.
https://codereview.chromium.org/1159523002/diff/60001/content/child/bluetooth... File content/child/bluetooth/bluetooth_dispatcher.cc (right): https://codereview.chromium.org/1159523002/diff/60001/content/child/bluetooth... content/child/bluetooth/bluetooth_dispatcher.cc:218: // Since we couldn't find the service return null. See Step 3 of On 2015/05/29 17:53:55, ortuno wrote: > On 2015/05/28 at 23:09:16, scheib wrote: > > Let's cross reference from https://github.com/WebBluetoothChrome/web-bluetooth > here as well. > > Not sure what you mean. You want me to add a reference to the chrome fork of the > spec here? > > Please add a pull request for > https://github.com/WebBluetoothChrome/web-bluetooth to the change description of > this patch. > > The description of this patch? Sorry, more clearly: Please add comments into the chrome implementation notes fork of the spec [1] in the Step 3 section of getPrimaryService algorithm indicating that we've had to create special code in BluetoothDispatcher::OnGetPrimaryServiceError to return NULL. If the spec changes that logic, we will then have a clear reference back to this block of code. Then, in this patch's description [2] please add a statement such as "Updates to the Chrome Implementation Notes corresponding to this patch: https://github.com/WebBluetoothChrome/web-bluetooth/pull/0" [1] https://github.com/WebBluetoothChrome/web-bluetooth [2] https://codereview.chromium.org/1159523002 https://codereview.chromium.org/1159523002/diff/120001/content/child/bluetoot... File content/child/bluetooth/bluetooth_primary_service_request.h (right): https://codereview.chromium.org/1159523002/diff/120001/content/child/bluetoot... content/child/bluetooth/bluetooth_primary_service_request.h:13: struct BluetoothPrimaryServiceRequest { On 2015/05/29 20:31:42, Jeffrey Yasskin wrote: > I'd default to just defining this in the single .cc file that it's used. I suspect this is based on a conversation ortuno and I had where I lamented multiple classes defined in a single .h file. E.g. device/bluetooth/bluetooth_discovery_session.h containing the related BluetoothDiscoveryFilter class. I agree that a class/struct used in only one implementation file is appropriate to be contained at the top of that file unless it becomes rather large and unwieldy. BluetoothPrimaryServiceRequest is fine in just the bluetooth_dispatcher_host.cc. BluetoothDiscoveryFilter, on the other hand is used by multiple other compilation units, and though not required I prefer it to be in bluetooth_discovery_filter.h instead of joined in bluetooth_discovery_session.h. https://codereview.chromium.org/1159523002/diff/120001/device/bluetooth/test/... File device/bluetooth/test/mock_bluetooth_device.h (right): https://codereview.chromium.org/1159523002/diff/120001/device/bluetooth/test/... device/bluetooth/test/mock_bluetooth_device.h:89: // Example: I should have been clearer, include a more complete sample that shows 'GetGattServices' invoking 'GetMockServices': ON_CALL(*device, GetGattServices()).WillByDefault( Invoke(device.get(), &MockBluetoothDevice::GetMockServices));
https://codereview.chromium.org/1159523002/diff/120001/content/child/bluetoot... File content/child/bluetooth/bluetooth_primary_service_request.h (right): https://codereview.chromium.org/1159523002/diff/120001/content/child/bluetoot... content/child/bluetooth/bluetooth_primary_service_request.h:13: struct BluetoothPrimaryServiceRequest { On 2015/05/31 19:48:41, scheib wrote: > On 2015/05/29 20:31:42, Jeffrey Yasskin wrote: > > I'd default to just defining this in the single .cc file that it's used. > > I suspect this is based on a conversation ortuno and I had where I lamented > multiple classes defined in a single .h file. E.g. > device/bluetooth/bluetooth_discovery_session.h > containing the related BluetoothDiscoveryFilter class. > > I agree that a class/struct used in only one implementation file is appropriate > to be contained at the top of that file unless it becomes rather large and > unwieldy. BluetoothPrimaryServiceRequest is fine in just the > bluetooth_dispatcher_host.cc. > > BluetoothDiscoveryFilter, on the other hand is used by multiple other > compilation units, and though not required I prefer it to be in > bluetooth_discovery_filter.h instead of joined in bluetooth_discovery_session.h. SGTM. I think the argument to split classes out into their own files gets stronger as the classes get more widely used and more complicated. If BluetoothPrimaryServiceRequest were its current complexity but its header were included by multiple other .cc files, I'd be on the fence about where to put it, and reasonably happy to see it in its own header in order to make users' lives easier. If it were the complexity of BluetoothDiscoveryFilter but only used in bluetooth_dispatcher.cc, I would similarly be happy to see it defined in a header rather than bluetooth_dispatcher.cc, so it could be unittested. It's only because the class is so simple and can be defined completely in a single .cc that I'm suggesting putting it there.
Also added reference in WebBluetoothChrome https://codereview.chromium.org/1159523002/diff/120001/content/child/bluetoot... File content/child/bluetooth/bluetooth_dispatcher.h (right): https://codereview.chromium.org/1159523002/diff/120001/content/child/bluetoot... content/child/bluetooth/bluetooth_dispatcher.h:96: // Owns callback objects. On 2015/05/29 20:31:42, Jeffrey Yasskin wrote: > They aren't callback objects anymore. :) Done. https://codereview.chromium.org/1159523002/diff/120001/content/child/bluetoot... File content/child/bluetooth/bluetooth_primary_service_request.h (right): https://codereview.chromium.org/1159523002/diff/120001/content/child/bluetoot... content/child/bluetooth/bluetooth_primary_service_request.h:13: struct BluetoothPrimaryServiceRequest { On 2015/05/31 21:11:12, Jeffrey Yasskin wrote: > On 2015/05/31 19:48:41, scheib wrote: > > On 2015/05/29 20:31:42, Jeffrey Yasskin wrote: > > > I'd default to just defining this in the single .cc file that it's used. > > > > I suspect this is based on a conversation ortuno and I had where I lamented > > multiple classes defined in a single .h file. E.g. > > device/bluetooth/bluetooth_discovery_session.h > > containing the related BluetoothDiscoveryFilter class. > > > > I agree that a class/struct used in only one implementation file is > appropriate > > to be contained at the top of that file unless it becomes rather large and > > unwieldy. BluetoothPrimaryServiceRequest is fine in just the > > bluetooth_dispatcher_host.cc. > > > > BluetoothDiscoveryFilter, on the other hand is used by multiple other > > compilation units, and though not required I prefer it to be in > > bluetooth_discovery_filter.h instead of joined in > bluetooth_discovery_session.h. > > SGTM. I think the argument to split classes out into their own files gets > stronger as the classes get more widely used and more complicated. If > BluetoothPrimaryServiceRequest were its current complexity but its header were > included by multiple other .cc files, I'd be on the fence about where to put it, > and reasonably happy to see it in its own header in order to make users' lives > easier. If it were the complexity of BluetoothDiscoveryFilter but only used in > bluetooth_dispatcher.cc, I would similarly be happy to see it defined in a > header rather than bluetooth_dispatcher.cc, so it could be unittested. It's only > because the class is so simple and can be defined completely in a single .cc > that I'm suggesting putting it there. Done. Hm I was having some trouble with the compiler not allowing me to define a "complex" struct in the cc file. But now it works. https://codereview.chromium.org/1159523002/diff/120001/device/bluetooth/test/... File device/bluetooth/test/mock_bluetooth_device.h (right): https://codereview.chromium.org/1159523002/diff/120001/device/bluetooth/test/... device/bluetooth/test/mock_bluetooth_device.h:89: // Example: On 2015/05/31 19:48:41, scheib wrote: > I should have been clearer, include a more complete sample that shows > 'GetGattServices' invoking 'GetMockServices': > ON_CALL(*device, GetGattServices()).WillByDefault( > Invoke(device.get(), &MockBluetoothDevice::GetMockServices)); Done.
LGTM one thing left: https://codereview.chromium.org/1159523002/diff/140001/content/child/bluetoot... File content/child/bluetooth/bluetooth_dispatcher.cc (right): https://codereview.chromium.org/1159523002/diff/140001/content/child/bluetoot... content/child/bluetooth/bluetooth_dispatcher.cc:43: } Blank line. And "Terminate namespaces with comments as shown in the given examples." http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Namespaces
https://codereview.chromium.org/1159523002/diff/140001/content/child/bluetoot... File content/child/bluetooth/bluetooth_dispatcher.cc (right): https://codereview.chromium.org/1159523002/diff/140001/content/child/bluetoot... content/child/bluetooth/bluetooth_dispatcher.cc:43: } On 2015/06/01 19:35:56, scheib wrote: > Blank line. And "Terminate namespaces with comments as shown in the given > examples." > http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Namespaces Done.
ortuno@chromium.org changed reviewers: + armansito@chromium.org, tsepez@chromium.org
@tsepez: Please review changes in bluetooth_messages.h @armansito: Please review changes in mock_bluetooth_device
device/bluetooth lgtm with one nit https://codereview.chromium.org/1159523002/diff/160001/device/bluetooth/test/... File device/bluetooth/test/mock_bluetooth_device.h (right): https://codereview.chromium.org/1159523002/diff/160001/device/bluetooth/test/... device/bluetooth/test/mock_bluetooth_device.h:94: std::vector<BluetoothGattService*> GetMockServices(); nit: make this a const method.
@armansito: Thanks! https://codereview.chromium.org/1159523002/diff/160001/device/bluetooth/test/... File device/bluetooth/test/mock_bluetooth_device.h (right): https://codereview.chromium.org/1159523002/diff/160001/device/bluetooth/test/... device/bluetooth/test/mock_bluetooth_device.h:94: std::vector<BluetoothGattService*> GetMockServices(); On 2015/06/01 21:10:15, armansito wrote: > nit: make this a const method. Done.
Messages LGTM.
The CQ bit was checked by ortuno@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jyasskin@chromium.org, scheib@chromium.org, armansito@chromium.org, tsepez@chromium.org Link to the patchset: https://codereview.chromium.org/1159523002/#ps150012 (title: "Remove anonymous namespace")
The CQ bit was unchecked by ortuno@chromium.org
The CQ bit was checked by ortuno@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1159523002/150012
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)
The CQ bit was checked by ortuno@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1159523002/150012
Message was sent while issue was closed.
Committed patchset #9 (id:150012)
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/49eb1ee2a0d9b51e39c687d66f8e530b35fe83cf Cr-Commit-Position: refs/heads/master@{#332335} |
