Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1152393002:
bluetooth: Blink side implementation of getPrimaryService (Closed)
Descriptionbluetooth: Blink side implementation of getPrimaryService
This adds the corresponding Blink-side implementation for getPrimaryService.
This change also adds layout tests for getPrimaryService.
This is the last of three patches to implement getPrimaryService:
[1] http://crrev.com/1150253004
[2] http://crrev.com/1159523002
[3] This patch.
BUG=491399
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=196420
Patch Set 1 #Patch Set 2 : 1. getPrimaryService impl #
Total comments: 7
Patch Set 3 : Add explicit to BluetoothGATTRemoteService constructor #Patch Set 4 : Fixed layout test #
Total comments: 10
Patch Set 5 : Address scheib's comments #Patch Set 6 : Test cleanup #
Total comments: 10
Patch Set 7 : Address jyasskin's comments #Patch Set 8 : Address scheib's comments #
Total comments: 5
Patch Set 9 : Add BluetoothGATTService to webexposed layout test #Messages
Total messages: 36 (9 generated)
ortuno@chromium.org changed reviewers: + haraken@chromium.org
@haraken: PTAL
haraken@chromium.org changed reviewers: + yhirano@chromium.org
+yhirano for ScriptPromise changes https://codereview.chromium.org/1152393002/diff/20001/Source/modules/bluetoot... File Source/modules/bluetooth/BluetoothGATTService.cpp (right): https://codereview.chromium.org/1152393002/diff/20001/Source/modules/bluetoot... Source/modules/bluetooth/BluetoothGATTService.cpp:23: OwnPtr<WebBluetoothGATTService> webService = adoptPtr(webServiceRawPointer); We should avoid the adoptPtr(raw_pointer) pattern since it is error-prone. adoptPtr needs to be used in the adoptPtr(new WebBluetoothGATTService) pattern. See the comment below as well. https://codereview.chromium.org/1152393002/diff/20001/Source/modules/bluetoot... Source/modules/bluetooth/BluetoothGATTService.cpp:29: delete webService; Manually deleting an object looks terrible :) Can you make BluetoothGATTService hold an OwnPtr<WebBluetoothGATTService>? Then you can just call m_webService.clear() here. https://codereview.chromium.org/1152393002/diff/20001/Source/modules/bluetoot... File Source/modules/bluetooth/BluetoothGATTService.h (right): https://codereview.chromium.org/1152393002/diff/20001/Source/modules/bluetoot... Source/modules/bluetooth/BluetoothGATTService.h:30: BluetoothGATTService(const WebBluetoothGATTService&); Add explicit.
https://codereview.chromium.org/1152393002/diff/20001/Source/modules/bluetoot... File Source/modules/bluetooth/BluetoothGATTService.cpp (right): https://codereview.chromium.org/1152393002/diff/20001/Source/modules/bluetoot... Source/modules/bluetooth/BluetoothGATTService.cpp:23: OwnPtr<WebBluetoothGATTService> webService = adoptPtr(webServiceRawPointer); On 2015/05/24 00:13:50, haraken wrote: > > We should avoid the adoptPtr(raw_pointer) pattern since it is error-prone. > adoptPtr needs to be used in the adoptPtr(new WebBluetoothGATTService) pattern. > See the comment below as well. To: haraken@ Yeah, I agree that this is ugly, but this is enforced by CallbackPromiseAdapter design. Please see its class comment. https://codereview.chromium.org/1152393002/diff/20001/Source/modules/bluetoot... Source/modules/bluetooth/BluetoothGATTService.cpp:29: delete webService; On 2015/05/24 00:13:51, haraken wrote: > > Manually deleting an object looks terrible :) > > Can you make BluetoothGATTService hold an OwnPtr<WebBluetoothGATTService>? Then > you can just call m_webService.clear() here. To: haraken@ ditto
https://codereview.chromium.org/1152393002/diff/20001/Source/modules/bluetoot... File Source/modules/bluetooth/BluetoothGATTService.cpp (right): https://codereview.chromium.org/1152393002/diff/20001/Source/modules/bluetoot... Source/modules/bluetooth/BluetoothGATTService.cpp:23: OwnPtr<WebBluetoothGATTService> webService = adoptPtr(webServiceRawPointer); On 2015/05/25 02:30:00, yhirano wrote: > On 2015/05/24 00:13:50, haraken wrote: > > > > We should avoid the adoptPtr(raw_pointer) pattern since it is error-prone. > > adoptPtr needs to be used in the adoptPtr(new WebBluetoothGATTService) > pattern. > > See the comment below as well. > > To: haraken@ > Yeah, I agree that this is ugly, but this is enforced by CallbackPromiseAdapter > design. Please see its class comment. I'm curious why we can't make CallbackPromiseAdapter hold an OwnPtr to BluetoothGATTService. That OwnPtr could be cleared when the CallbackPromiseAdapter disposes the BluetoothGATTService...?
On 2015/05/26 11:45:34, haraken wrote: > https://codereview.chromium.org/1152393002/diff/20001/Source/modules/bluetoot... > File Source/modules/bluetooth/BluetoothGATTService.cpp (right): > > https://codereview.chromium.org/1152393002/diff/20001/Source/modules/bluetoot... > Source/modules/bluetooth/BluetoothGATTService.cpp:23: > OwnPtr<WebBluetoothGATTService> webService = adoptPtr(webServiceRawPointer); > On 2015/05/25 02:30:00, yhirano wrote: > > On 2015/05/24 00:13:50, haraken wrote: > > > > > > We should avoid the adoptPtr(raw_pointer) pattern since it is error-prone. > > > adoptPtr needs to be used in the adoptPtr(new WebBluetoothGATTService) > > pattern. > > > See the comment below as well. > > > > To: haraken@ > > Yeah, I agree that this is ugly, but this is enforced by > CallbackPromiseAdapter > > design. Please see its class comment. > > I'm curious why we can't make CallbackPromiseAdapter hold an OwnPtr to > BluetoothGATTService. That OwnPtr could be cleared when the > CallbackPromiseAdapter disposes the BluetoothGATTService...? +nhiroki@, falken@ It is requested by ServiceWorker developers: Please see the below code. https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
On 2015/05/26 12:15:29, yhirano wrote: > On 2015/05/26 11:45:34, haraken wrote: > > > https://codereview.chromium.org/1152393002/diff/20001/Source/modules/bluetoot... > > File Source/modules/bluetooth/BluetoothGATTService.cpp (right): > > > > > https://codereview.chromium.org/1152393002/diff/20001/Source/modules/bluetoot... > > Source/modules/bluetooth/BluetoothGATTService.cpp:23: > > OwnPtr<WebBluetoothGATTService> webService = adoptPtr(webServiceRawPointer); > > On 2015/05/25 02:30:00, yhirano wrote: > > > On 2015/05/24 00:13:50, haraken wrote: > > > > > > > > We should avoid the adoptPtr(raw_pointer) pattern since it is error-prone. > > > > adoptPtr needs to be used in the adoptPtr(new WebBluetoothGATTService) > > > pattern. > > > > See the comment below as well. > > > > > > To: haraken@ > > > Yeah, I agree that this is ugly, but this is enforced by > > CallbackPromiseAdapter > > > design. Please see its class comment. > > > > I'm curious why we can't make CallbackPromiseAdapter hold an OwnPtr to > > BluetoothGATTService. That OwnPtr could be cleared when the > > CallbackPromiseAdapter disposes the BluetoothGATTService...? > > +nhiroki@, falken@ > > It is requested by ServiceWorker developers: Please see the below code. > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... Thanks for the context! I understand you want to customize the timing when the object gets deleted, but it seems nasty to do that by manually calling delete... If you don't want to delete the registration object as long as registration->proxy() exists, I guess we can introduce an indirection object to handle the logic. - CallbackPromiseAdapter holds an OwnPtr to X. CallbackPromiseAdapter clears the OwnPtr when disposing X. - X holds a RefPtr to the registration object. - The proxy object holds a RefPtr to the registration object. The proxy objects clears the RefPtr when the registration object loses the proxy. ^^^ I don't think the above suggestion makes sense but I think we can refactor the code so that we don't need to rely on manual delete.
On 2015/05/26 12:29:11, haraken wrote: > On 2015/05/26 12:15:29, yhirano wrote: > > On 2015/05/26 11:45:34, haraken wrote: > > > > > > https://codereview.chromium.org/1152393002/diff/20001/Source/modules/bluetoot... > > > File Source/modules/bluetooth/BluetoothGATTService.cpp (right): > > > > > > > > > https://codereview.chromium.org/1152393002/diff/20001/Source/modules/bluetoot... > > > Source/modules/bluetooth/BluetoothGATTService.cpp:23: > > > OwnPtr<WebBluetoothGATTService> webService = adoptPtr(webServiceRawPointer); > > > On 2015/05/25 02:30:00, yhirano wrote: > > > > On 2015/05/24 00:13:50, haraken wrote: > > > > > > > > > > We should avoid the adoptPtr(raw_pointer) pattern since it is > error-prone. > > > > > adoptPtr needs to be used in the adoptPtr(new WebBluetoothGATTService) > > > > pattern. > > > > > See the comment below as well. > > > > > > > > To: haraken@ > > > > Yeah, I agree that this is ugly, but this is enforced by > > > CallbackPromiseAdapter > > > > design. Please see its class comment. > > > > > > I'm curious why we can't make CallbackPromiseAdapter hold an OwnPtr to > > > BluetoothGATTService. That OwnPtr could be cleared when the > > > CallbackPromiseAdapter disposes the BluetoothGATTService...? > > > > +nhiroki@, falken@ > > > > It is requested by ServiceWorker developers: Please see the below code. > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > Thanks for the context! > > I understand you want to customize the timing when the object gets deleted, but > it seems nasty to do that by manually calling delete... > > If you don't want to delete the registration object as long as > registration->proxy() exists, I guess we can introduce an indirection object to > handle the logic. > > - CallbackPromiseAdapter holds an OwnPtr to X. CallbackPromiseAdapter clears the > OwnPtr when disposing X. > - X holds a RefPtr to the registration object. > - The proxy object holds a RefPtr to the registration object. The proxy objects > clears the RefPtr when the registration object loses the proxy. > > ^^^ I don't think the above suggestion makes sense but I think we can refactor > the code so that we don't need to rely on manual delete. I'm not fully understand yet how CallbackPromiseAdapter collaborates with X, but eliminating the manual delete sounds good. This is not related to this CL, so how about leaving this CL as is for now? I or someone else will work on it later (sorry, I don't have enough bandwidth to work on it right now).
On 2015/05/27 at 10:03:27, nhiroki wrote: > On 2015/05/26 12:29:11, haraken wrote: > > On 2015/05/26 12:15:29, yhirano wrote: > > > On 2015/05/26 11:45:34, haraken wrote: > > > > > > > > > https://codereview.chromium.org/1152393002/diff/20001/Source/modules/bluetoot... > > > > File Source/modules/bluetooth/BluetoothGATTService.cpp (right): > > > > > > > > > > > > > https://codereview.chromium.org/1152393002/diff/20001/Source/modules/bluetoot... > > > > Source/modules/bluetooth/BluetoothGATTService.cpp:23: > > > > OwnPtr<WebBluetoothGATTService> webService = adoptPtr(webServiceRawPointer); > > > > On 2015/05/25 02:30:00, yhirano wrote: > > > > > On 2015/05/24 00:13:50, haraken wrote: > > > > > > > > > > > > We should avoid the adoptPtr(raw_pointer) pattern since it is > > error-prone. > > > > > > adoptPtr needs to be used in the adoptPtr(new WebBluetoothGATTService) > > > > > pattern. > > > > > > See the comment below as well. > > > > > > > > > > To: haraken@ > > > > > Yeah, I agree that this is ugly, but this is enforced by > > > > CallbackPromiseAdapter > > > > > design. Please see its class comment. > > > > > > > > I'm curious why we can't make CallbackPromiseAdapter hold an OwnPtr to > > > > BluetoothGATTService. That OwnPtr could be cleared when the > > > > CallbackPromiseAdapter disposes the BluetoothGATTService...? > > > > > > +nhiroki@, falken@ > > > > > > It is requested by ServiceWorker developers: Please see the below code. > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > > > Thanks for the context! > > > > I understand you want to customize the timing when the object gets deleted, but > > it seems nasty to do that by manually calling delete... > > > > If you don't want to delete the registration object as long as > > registration->proxy() exists, I guess we can introduce an indirection object to > > handle the logic. > > > > - CallbackPromiseAdapter holds an OwnPtr to X. CallbackPromiseAdapter clears the > > OwnPtr when disposing X. > > - X holds a RefPtr to the registration object. > > - The proxy object holds a RefPtr to the registration object. The proxy objects > > clears the RefPtr when the registration object loses the proxy. > > > > ^^^ I don't think the above suggestion makes sense but I think we can refactor > > the code so that we don't need to rely on manual delete. > > I'm not fully understand yet how CallbackPromiseAdapter collaborates with X, but eliminating the manual delete sounds good. This is not related to this CL, so how about leaving this CL as is for now? I or someone else will work on it later (sorry, I don't have enough bandwidth to work on it right now). @haraken, @yhirano, @nhiroki: Thanks for the info! Please let me know if there any changes to CallbackPromiseAdapter and I will implement the changes on this side.
https://codereview.chromium.org/1152393002/diff/20001/Source/modules/bluetoot... File Source/modules/bluetooth/BluetoothGATTService.h (right): https://codereview.chromium.org/1152393002/diff/20001/Source/modules/bluetoot... Source/modules/bluetooth/BluetoothGATTService.h:30: BluetoothGATTService(const WebBluetoothGATTService&); On 2015/05/24 at 00:13:51, haraken wrote: > Add explicit. Done.
ortuno@chromium.org changed reviewers: + scheib@chromium.org
@scheib: PTAL
https://codereview.chromium.org/1152393002/diff/60001/LayoutTests/bluetooth/g... File LayoutTests/bluetooth/getPrimaryService.html (right): https://codereview.chromium.org/1152393002/diff/60001/LayoutTests/bluetooth/g... LayoutTests/bluetooth/getPrimaryService.html:45: assert_equals(service.uuid, generic_access, test that the device matches as well. https://codereview.chromium.org/1152393002/diff/60001/LayoutTests/bluetooth/g... LayoutTests/bluetooth/getPrimaryService.html:49: }); An additional call to getPrimaryService must return the same object: https://webbluetoothcg.github.io/web-bluetooth/#information-model "The cache must not contain two entities that are for the same attribute." We don't pass this test yet, but we should file an issue and write the test (adjusted to handle the current incorrect behavior) here so it's not forgotten. https://codereview.chromium.org/1152393002/diff/60001/Source/modules/bluetoot... File Source/modules/bluetooth/BluetoothGATTRemoteServer.cpp (right): https://codereview.chromium.org/1152393002/diff/60001/Source/modules/bluetoot... Source/modules/bluetooth/BluetoothGATTRemoteServer.cpp:39: { TODO: BluetoothUUID.getService(serviceUUID) crbug.com/00000 https://codereview.chromium.org/1152393002/diff/60001/Source/modules/bluetoot... File Source/modules/bluetooth/BluetoothGATTRemoteServer.h (right): https://codereview.chromium.org/1152393002/diff/60001/Source/modules/bluetoot... Source/modules/bluetooth/BluetoothGATTRemoteServer.h:42: ScriptPromise getPrimaryService(ScriptState*, String /* service_uuid */); Use parameter name, not comment.
I'm not intending to block this CL, but the manual delete should be fixed. I'm ok with doing that in a follow up if this CL needs to be landed soon, but we should have a concrete plan to fix it.
On 2015/05/29 at 01:35:45, haraken wrote: > I'm not intending to block this CL, but the manual delete should be fixed. I'm ok with doing that in a follow up if this CL needs to be landed soon, but we should have a concrete plan to fix it. @haraken: I thought changes needed to be made in CallbackPromiseAdapter before we could get rid of the manual delete?
On 2015/05/29 01:38:30, ortuno wrote: > On 2015/05/29 at 01:35:45, haraken wrote: > > I'm not intending to block this CL, but the manual delete should be fixed. I'm > ok with doing that in a follow up if this CL needs to be landed soon, but we > should have a concrete plan to fix it. > > @haraken: I thought changes needed to be made in CallbackPromiseAdapter before > we could get rid of the manual delete? Yes, we need to fix CallbackPromiseAdapter to remove it. (So I'm not intending to force you to fix the problem in this CL.)
On 2015/05/29 01:43:12, haraken wrote: > On 2015/05/29 01:38:30, ortuno wrote: > > On 2015/05/29 at 01:35:45, haraken wrote: > > > I'm not intending to block this CL, but the manual delete should be fixed. > I'm > > ok with doing that in a follow up if this CL needs to be landed soon, but we > > should have a concrete plan to fix it. > > > > @haraken: I thought changes needed to be made in CallbackPromiseAdapter before > > we could get rid of the manual delete? > > Yes, we need to fix CallbackPromiseAdapter to remove it. (So I'm not intending > to force you to fix the problem in this CL.) Filled an issue: http://crbug.com/493531. Probably I'll be able to work on it next week.
https://codereview.chromium.org/1152393002/diff/60001/LayoutTests/bluetooth/g... File LayoutTests/bluetooth/getPrimaryService.html (right): https://codereview.chromium.org/1152393002/diff/60001/LayoutTests/bluetooth/g... LayoutTests/bluetooth/getPrimaryService.html:45: assert_equals(service.uuid, generic_access, On 2015/05/29 00:17:32, scheib wrote: > test that the device matches as well. There is no reference to device yet. https://codereview.chromium.org/1152393002/diff/60001/LayoutTests/bluetooth/g... LayoutTests/bluetooth/getPrimaryService.html:49: }); On 2015/05/29 00:17:32, scheib wrote: > An additional call to getPrimaryService must return the same object: > https://webbluetoothcg.github.io/web-bluetooth/#information-model "The cache > must not contain two entities that are for the same attribute." > > We don't pass this test yet, but we should file an issue and write the test > (adjusted to handle the current incorrect behavior) here so it's not forgotten. Done. https://codereview.chromium.org/1152393002/diff/60001/Source/modules/bluetoot... File Source/modules/bluetooth/BluetoothGATTRemoteServer.cpp (right): https://codereview.chromium.org/1152393002/diff/60001/Source/modules/bluetoot... Source/modules/bluetooth/BluetoothGATTRemoteServer.cpp:39: { On 2015/05/29 00:17:32, scheib wrote: > TODO: BluetoothUUID.getService(serviceUUID) crbug.com/00000 Done. https://codereview.chromium.org/1152393002/diff/60001/Source/modules/bluetoot... File Source/modules/bluetooth/BluetoothGATTRemoteServer.h (right): https://codereview.chromium.org/1152393002/diff/60001/Source/modules/bluetoot... Source/modules/bluetooth/BluetoothGATTRemoteServer.h:42: ScriptPromise getPrimaryService(ScriptState*, String /* service_uuid */); On 2015/05/29 00:17:32, scheib wrote: > Use parameter name, not comment. Done.
ortuno@chromium.org changed reviewers: + jyasskin@chromium.org
@jyasskin: PTAL
lgtm https://codereview.chromium.org/1152393002/diff/100001/LayoutTests/bluetooth/... File LayoutTests/bluetooth/getPrimaryService.html (right): https://codereview.chromium.org/1152393002/diff/100001/LayoutTests/bluetooth/... LayoutTests/bluetooth/getPrimaryService.html:15: return device.connectGATT().then(function(gattServer) { I think you can collapse this to: return navigator.bluetooth.requestDevice().then(function(device) { return device.connectGATT(); }).then(function(gattServer) { testRunner.setBluetoothMockDataSet('EmptyAdapter'); return gattServer.getPrimaryService(generic_access).then(function() { assert_unreached('Should return error if device not in adapter.'); }, function(e) { assert_equals(e.name, "NetworkError"); }); }); https://codereview.chromium.org/1152393002/diff/100001/LayoutTests/bluetooth/... LayoutTests/bluetooth/getPrimaryService.html:63: assert_not_equals(services[0], services[1], Maybe be clearer that this assertion is going to change senses. There's also another aspect of this that we should test: if you add a property to the first service you get, then let it go out of scope and be garbage collected, and then retrieve the same service again and look at that property, the thing you assigned should still be there. https://codereview.chromium.org/1152393002/diff/100001/Source/modules/bluetoo... File Source/modules/bluetooth/BluetoothGATTRemoteServer.cpp (right): https://codereview.chromium.org/1152393002/diff/100001/Source/modules/bluetoo... Source/modules/bluetooth/BluetoothGATTRemoteServer.cpp:38: String serviceUUID) IIRC, Blink likes keeping arguments all on one line? https://codereview.chromium.org/1152393002/diff/100001/Source/modules/bluetoo... Source/modules/bluetooth/BluetoothGATTRemoteServer.cpp:44: return ScriptPromise::rejectWithDOMException(scriptState, DOMException::create(NotSupportedError)); How would this happen? I'd assume that JS couldn't get a GATTRemoteServer in the first place if there's no bluetooth system. I've filed https://github.com/WebBluetoothCG/web-bluetooth/issues/127 to figure out what to do in the spec.
@scheib: PTAL https://codereview.chromium.org/1152393002/diff/100001/LayoutTests/bluetooth/... File LayoutTests/bluetooth/getPrimaryService.html (right): https://codereview.chromium.org/1152393002/diff/100001/LayoutTests/bluetooth/... LayoutTests/bluetooth/getPrimaryService.html:15: return device.connectGATT().then(function(gattServer) { On 2015/06/02 20:28:07, Jeffrey Yasskin wrote: > I think you can collapse this to: > > return navigator.bluetooth.requestDevice().then(function(device) { > return device.connectGATT(); > }).then(function(gattServer) { > testRunner.setBluetoothMockDataSet('EmptyAdapter'); > return gattServer.getPrimaryService(generic_access).then(function() { > assert_unreached('Should return error if device not in adapter.'); > }, function(e) { > assert_equals(e.name, "NetworkError"); > }); > }); Done. https://codereview.chromium.org/1152393002/diff/100001/LayoutTests/bluetooth/... LayoutTests/bluetooth/getPrimaryService.html:63: assert_not_equals(services[0], services[1], On 2015/06/02 20:28:07, Jeffrey Yasskin wrote: > Maybe be clearer that this assertion is going to change senses. Done. > There's also another aspect of this that we should test: if you add a property > to the first service you get, then let it go out of scope and be garbage > collected, and then retrieve the same service again and look at that property, > the thing you assigned should still be there. Ok. Need to think about how to implement that. https://codereview.chromium.org/1152393002/diff/100001/Source/modules/bluetoo... File Source/modules/bluetooth/BluetoothGATTRemoteServer.cpp (right): https://codereview.chromium.org/1152393002/diff/100001/Source/modules/bluetoo... Source/modules/bluetooth/BluetoothGATTRemoteServer.cpp:38: String serviceUUID) On 2015/06/02 20:28:08, Jeffrey Yasskin wrote: > IIRC, Blink likes keeping arguments all on one line? Done. https://codereview.chromium.org/1152393002/diff/100001/Source/modules/bluetoo... Source/modules/bluetooth/BluetoothGATTRemoteServer.cpp:44: return ScriptPromise::rejectWithDOMException(scriptState, DOMException::create(NotSupportedError)); On 2015/06/02 20:28:08, Jeffrey Yasskin wrote: > How would this happen? I'd assume that JS couldn't get a GATTRemoteServer in the > first place if there's no bluetooth system. > > I've filed https://github.com/WebBluetoothCG/web-bluetooth/issues/127 to figure > out what to do in the spec. Ah you're right. I shouldn't be copy pasting code.
LGTM https://codereview.chromium.org/1152393002/diff/60001/LayoutTests/bluetooth/g... File LayoutTests/bluetooth/getPrimaryService.html (right): https://codereview.chromium.org/1152393002/diff/60001/LayoutTests/bluetooth/g... LayoutTests/bluetooth/getPrimaryService.html:45: assert_equals(service.uuid, generic_access, On 2015/06/01 21:21:34, ortuno wrote: > On 2015/05/29 00:17:32, scheib wrote: > > test that the device matches as well. > > There is no reference to device yet. Add a TODO in the BluetoothGATTService.idl then for the device attribute, pointing to this test and that it should be included.
Thanks! https://codereview.chromium.org/1152393002/diff/60001/LayoutTests/bluetooth/g... File LayoutTests/bluetooth/getPrimaryService.html (right): https://codereview.chromium.org/1152393002/diff/60001/LayoutTests/bluetooth/g... LayoutTests/bluetooth/getPrimaryService.html:45: assert_equals(service.uuid, generic_access, On 2015/06/03 03:28:24, scheib wrote: > On 2015/06/01 21:21:34, ortuno wrote: > > On 2015/05/29 00:17:32, scheib wrote: > > > test that the device matches as well. > > > > There is no reference to device yet. > > Add a TODO in the BluetoothGATTService.idl then for the device attribute, > pointing to this test and that it should be included. Done.
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 Link to the patchset: https://codereview.chromium.org/1152393002/#ps140001 (title: "Address scheib's comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1152393002/140001
https://codereview.chromium.org/1152393002/diff/140001/Source/modules/bluetoo... File Source/modules/bluetooth/BluetoothGATTService.cpp (right): https://codereview.chromium.org/1152393002/diff/140001/Source/modules/bluetoo... Source/modules/bluetooth/BluetoothGATTService.cpp:14: : m_webService(webService) We're deep-copying the webService here. Is this intentional? https://codereview.chromium.org/1152393002/diff/140001/Source/modules/bluetoo... Source/modules/bluetooth/BluetoothGATTService.cpp:24: return new BluetoothGATTService(*webService); Maybe you want to pass in webService.learPtr() here and let BluetoothGATTService keep a raw pointer to the webService. Then the webService is explicitly deleted in the following dispose(). (As discussed above, the mis-design of the lifetime management needs to be fixed soon.)
The CQ bit was unchecked by ortuno@chromium.org
https://codereview.chromium.org/1152393002/diff/100001/LayoutTests/bluetooth/... File LayoutTests/bluetooth/getPrimaryService.html (right): https://codereview.chromium.org/1152393002/diff/100001/LayoutTests/bluetooth/... LayoutTests/bluetooth/getPrimaryService.html:63: assert_not_equals(services[0], services[1], On 2015/06/03 03:05:16, ortuno wrote: > On 2015/06/02 20:28:07, Jeffrey Yasskin wrote: > > There's also another aspect of this that we should test: if you add a property > > to the first service you get, then let it go out of scope and be garbage > > collected, and then retrieve the same service again and look at that property, > > the thing you assigned should still be there. > > Ok. Need to think about how to implement that. Ask esprehn or another DOM expert when you get around to it. Apparently there's a known technique. https://codereview.chromium.org/1152393002/diff/140001/Source/modules/bluetoo... File Source/modules/bluetooth/BluetoothGATTService.cpp (right): https://codereview.chromium.org/1152393002/diff/140001/Source/modules/bluetoo... Source/modules/bluetooth/BluetoothGATTService.cpp:24: return new BluetoothGATTService(*webService); On 2015/06/03 03:45:38, haraken wrote: > > Maybe you want to pass in webService.learPtr() here and let BluetoothGATTService > keep a raw pointer to the webService. Then the webService is explicitly deleted > in the following dispose(). > > (As discussed above, the mis-design of the lifetime management needs to be fixed > soon.) I mentioned this in https://codereview.chromium.org/1146163005/diff/20001/Source/modules/bluetoot...: if we want to avoid the copy (which may not matter: the contents are all primitives or refcounted WebStrings), the right thing to do seems to be to have BluetoothGATTService store an OwnPtr, not need an explicit destructor to destroy the raw pointer. According to https://code.google.com/p/chromium/codesearch/#chromium/src/third_party/WebKi..., we never call both take() and dispose(), so there is no "following dispose()" if take() was called.
The CQ bit was checked by ortuno@chromium.org
https://codereview.chromium.org/1152393002/diff/100001/LayoutTests/bluetooth/... File LayoutTests/bluetooth/getPrimaryService.html (right): https://codereview.chromium.org/1152393002/diff/100001/LayoutTests/bluetooth/... LayoutTests/bluetooth/getPrimaryService.html:63: assert_not_equals(services[0], services[1], On 2015/06/03 at 15:10:04, Jeffrey Yasskin wrote: > On 2015/06/03 03:05:16, ortuno wrote: > > On 2015/06/02 20:28:07, Jeffrey Yasskin wrote: > > > There's also another aspect of this that we should test: if you add a property > > > to the first service you get, then let it go out of scope and be garbage > > > collected, and then retrieve the same service again and look at that property, > > > the thing you assigned should still be there. > > > > Ok. Need to think about how to implement that. > > Ask esprehn or another DOM expert when you get around to it. Apparently there's a known technique. Will do. https://codereview.chromium.org/1152393002/diff/140001/Source/modules/bluetoo... File Source/modules/bluetooth/BluetoothGATTService.cpp (right): https://codereview.chromium.org/1152393002/diff/140001/Source/modules/bluetoo... Source/modules/bluetooth/BluetoothGATTService.cpp:14: : m_webService(webService) On 2015/06/03 at 03:45:38, haraken wrote: > We're deep-copying the webService here. Is this intentional? No. This pattern slipped under in all of our classes. I will submit a follow up patch to fix all instances. https://codereview.chromium.org/1152393002/diff/140001/Source/modules/bluetoo... Source/modules/bluetooth/BluetoothGATTService.cpp:24: return new BluetoothGATTService(*webService); On 2015/06/03 at 15:10:04, Jeffrey Yasskin wrote: > On 2015/06/03 03:45:38, haraken wrote: > > > > Maybe you want to pass in webService.learPtr() here and let BluetoothGATTService > > keep a raw pointer to the webService. Then the webService is explicitly deleted > > in the following dispose(). > > > > (As discussed above, the mis-design of the lifetime management needs to be fixed > > soon.) > > I mentioned this in https://codereview.chromium.org/1146163005/diff/20001/Source/modules/bluetoot...: if we want to avoid the copy (which may not matter: the contents are all primitives or refcounted WebStrings), the right thing to do seems to be to have BluetoothGATTService store an OwnPtr, not need an explicit destructor to destroy the raw pointer. > > According to https://code.google.com/p/chromium/codesearch/#chromium/src/third_party/WebKi..., we never call both take() and dispose(), so there is no "following dispose()" if take() was called. We've been mistakenly following this pattern in all our Blink classes. I plan to submit a follow up patch to change all of them to what jyasskin described.
The patchset sent to the CQ was uploaded after l-g-t-m from scheib@chromium.org, jyasskin@chromium.org Link to the patchset: https://codereview.chromium.org/1152393002/#ps160001 (title: "Add BluetoothGATTService to webexposed layout test")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1152393002/160001
Message was sent while issue was closed.
Committed patchset #9 (id:160001) as https://src.chromium.org/viewvc/blink?view=rev&revision=196420 |
