Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1611773002:
bluetooth: Disconnect if the page becomes hidden or is closed. (Closed)
Descriptionbluetooth: Disconnect if the page becomes hidden or is closed.
This means closing connection when:
1. BluetoothGATTRemoteServer is destroyed.
2. The parent document is detached.
3. The page becomes hidden.
BUG=579746
Committed: https://crrev.com/2eec1c80c538f99ac3538a40c4a6584079ff7d47
Cr-Commit-Position: refs/heads/master@{#371542}
Patch Set 1 #Patch Set 2 : Clean up #Patch Set 3 : Disconnect if page hidden when connected. Clean up #
Total comments: 16
Patch Set 4 : Address jyasskin's comments #
Total comments: 8
Patch Set 5 : Address jyasskin's comments #2 #
Total comments: 8
Patch Set 6 : Address haraken's comments #Messages
Total messages: 31 (7 generated)
ortuno@chromium.org changed reviewers: + jyasskin@chromium.org
jyasskin: PTAL
Found an issue with this approach. If you call connectGATT() and then change tabs before the connection is finished you end up connected to the device. I think the short term solution is to check if the page is visible when the BluetoothGATTRemoteServer object is constructed and disconnect if the page is hidden. The long term solution would be to cancel the connection when the page becomes hidden, but we need to immediately return the BluetoothGATTRemoteServer when calling connectGATT() so we can call disconnect on it. Is the short term solution acceptable?
On 2016/01/21 18:33:57, ortuno wrote: > Found an issue with this approach. If you call connectGATT() and then change > tabs before the connection is finished you end up connected to the device. I > think the short term solution is to check if the page is visible when the > BluetoothGATTRemoteServer object is constructed and disconnect if the page is > hidden. The long term solution would be to cancel the connection when the page > becomes hidden, but we need to immediately return the BluetoothGATTRemoteServer > when calling connectGATT() so we can call disconnect on it. Is the short term > solution acceptable? Yep, the short-term solution sounds fine for shipping to the experimental framework. Does that mean you don't want me to start reviewing this yet?
Implemented proposed solution.
This looks great! Once you've gone through the numerous but small comments below, LGTM. https://codereview.chromium.org/1611773002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/bluetooth/connectGATT.html (right): https://codereview.chromium.org/1611773002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/bluetooth/connectGATT.html:136: }).then(() => testRunner.setPageVisibility('visible')); Do we also need to run this if the promise rejects? For example, if there's a typo in the exception message, does that cause all subsequent tests to also fail? https://codereview.chromium.org/1611773002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/bluetooth/disconnect-tab.html (right): https://codereview.chromium.org/1611773002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/bluetooth/disconnect-tab.html:36: }, 'Test object gets garbage collected before tab becomes hidden'); This is checking that Chrome doesn't crash? Would it make sense to request the same device again and assert that it's not connected? (I don't think this'll check anything right now, but we'll eventually return the same object from a redundant call to requestDevice(), and then we'll want to make sure we have a consistent behavior. Although, just garbage collecting should maybe be enough to disconnect the device if no notifications are set up...) https://codereview.chromium.org/1611773002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/bluetooth/disconnect-tab.html:45: }, 'Test object gets garbage collected after tab becomes hidden'); Is this also a crash test? https://codereview.chromium.org/1611773002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/bluetooth/BluetoothDevice.cpp (right): https://codereview.chromium.org/1611773002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/bluetooth/BluetoothDevice.cpp:21: PageVisibilityState getPageVisibilityState(ScriptState* scriptState) Either add a blank line after the "namespace {" or remove the one before "} // namespace". Probably add it. https://codereview.chromium.org/1611773002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/bluetooth/BluetoothGATTRemoteServer.cpp (right): https://codereview.chromium.org/1611773002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/bluetooth/BluetoothGATTRemoteServer.cpp:22: bool isPageVisible(Page* page) Add a blank line after "namespace {". (Ditto from the other anonymous namespace blank line comment.) https://codereview.chromium.org/1611773002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/bluetooth/BluetoothGATTRemoteServer.cpp:68: m_webGATT->connected = false; FWIW (not for this CL), I'm uncomfortable having 'connected' represented as a boolean that we have to manually keep in sync with the actual connected state. If you ever see a way to calculate it from something else, perhaps an object that represents the connection itself, let's switch to that. https://codereview.chromium.org/1611773002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/bluetooth/BluetoothGATTRemoteServer.h (right): https://codereview.chromium.org/1611773002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/bluetooth/BluetoothGATTRemoteServer.h:46: // 1. When the object gets GarbageCollected e.g. it went out of scope. We should allow folks to turn on notifications, listen for events on navigator.bluetooth, and then drop the BluetoothGATTRemoteServer and the BluetoothDevice, without getting disconnected. Maybe not yet, since we don't have event bubbling, but could you add a TODO for that and link to https://crbug.com/570083? https://codereview.chromium.org/1611773002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/bluetooth/BluetoothGATTRemoteServer.h:50: // 3. When the tab is no longer in the foreground e.g. change tabs. I'd phrase this something like "For now (https://crbug.com/579746), when the tab is no longer ..." so it's clear from the start that this isn't part of the long-term behavior.
PTAL. Also added more tests. https://codereview.chromium.org/1611773002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/bluetooth/connectGATT.html (right): https://codereview.chromium.org/1611773002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/bluetooth/connectGATT.html:136: }).then(() => testRunner.setPageVisibility('visible')); On 2016/01/21 at 23:44:33, Jeffrey Yasskin wrote: > Do we also need to run this if the promise rejects? For example, if there's a typo in the exception message, does that cause all subsequent tests to also fail? Added catch. https://codereview.chromium.org/1611773002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/bluetooth/disconnect-tab.html (right): https://codereview.chromium.org/1611773002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/bluetooth/disconnect-tab.html:36: }, 'Test object gets garbage collected before tab becomes hidden'); On 2016/01/21 at 23:44:33, Jeffrey Yasskin wrote: > This is checking that Chrome doesn't crash? > Yup. > Would it make sense to request the same device again and assert that it's not connected? (I don't think this'll check anything right now, but we'll eventually return the same object from a redundant call to requestDevice(), and then we'll want to make sure we have a consistent behavior. Although, just garbage collecting should maybe be enough to disconnect the device if no notifications are set up...) Not sure how we could check if the device is connected. The 'connected' member is in BluetoothGATTRemoteServer which is only available after you call connect on the device. https://codereview.chromium.org/1611773002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/bluetooth/disconnect-tab.html:45: }, 'Test object gets garbage collected after tab becomes hidden'); On 2016/01/21 at 23:44:33, Jeffrey Yasskin wrote: > Is this also a crash test? Yup. Added comment to clarify. https://codereview.chromium.org/1611773002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/bluetooth/BluetoothDevice.cpp (right): https://codereview.chromium.org/1611773002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/bluetooth/BluetoothDevice.cpp:21: PageVisibilityState getPageVisibilityState(ScriptState* scriptState) On 2016/01/21 at 23:44:33, Jeffrey Yasskin wrote: > Either add a blank line after the "namespace {" or remove the one before "} // namespace". Probably add it. Done. https://codereview.chromium.org/1611773002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/bluetooth/BluetoothGATTRemoteServer.cpp (right): https://codereview.chromium.org/1611773002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/bluetooth/BluetoothGATTRemoteServer.cpp:22: bool isPageVisible(Page* page) On 2016/01/21 at 23:44:33, Jeffrey Yasskin wrote: > Add a blank line after "namespace {". (Ditto from the other anonymous namespace blank line comment.) Done. https://codereview.chromium.org/1611773002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/bluetooth/BluetoothGATTRemoteServer.cpp:68: m_webGATT->connected = false; On 2016/01/21 at 23:44:33, Jeffrey Yasskin wrote: > FWIW (not for this CL), I'm uncomfortable having 'connected' represented as a boolean that we have to manually keep in sync with the actual connected state. If you ever see a way to calculate it from something else, perhaps an object that represents the connection itself, let's switch to that. I agree. I think things will become clearer once we resolve the spec issues regarding connection/disconnection. For now this just respects what the spec says. https://codereview.chromium.org/1611773002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/bluetooth/BluetoothGATTRemoteServer.h (right): https://codereview.chromium.org/1611773002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/bluetooth/BluetoothGATTRemoteServer.h:46: // 1. When the object gets GarbageCollected e.g. it went out of scope. On 2016/01/21 at 23:44:33, Jeffrey Yasskin wrote: > We should allow folks to turn on notifications, listen for events on navigator.bluetooth, and then drop the BluetoothGATTRemoteServer and the BluetoothDevice, without getting disconnected. Maybe not yet, since we don't have event bubbling, but could you add a TODO for that and link to https://crbug.com/570083? Done. https://codereview.chromium.org/1611773002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/bluetooth/BluetoothGATTRemoteServer.h:50: // 3. When the tab is no longer in the foreground e.g. change tabs. On 2016/01/21 at 23:44:33, Jeffrey Yasskin wrote: > I'd phrase this something like "For now (https://crbug.com/579746), when the tab is no longer ..." so it's clear from the start that this isn't part of the long-term behavior. Done.
https://codereview.chromium.org/1611773002/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/bluetooth/disconnect-frame-detached/detach-gc.html (right): https://codereview.chromium.org/1611773002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/bluetooth/disconnect-frame-detached/detach-gc.html:21: assert_unreached('iframe send invalid data: ' + messageEvent.data); s/send/sent/ I think https://codereview.chromium.org/1611773002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/bluetooth/disconnect-frame-detached/detach-gc.html:26: iframe.src = '../resources/disconnect-tab-in-iframe.html'; I don't see this file. Did you forget to add it? https://codereview.chromium.org/1611773002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/bluetooth/disconnect-frame-detached/detach-gc.html:28: }, 'Detach frame then garbage collect. We shouln\'t crash.'); spelling: shouln't https://codereview.chromium.org/1611773002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/bluetooth/BluetoothGATTRemoteServer.h (right): https://codereview.chromium.org/1611773002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/bluetooth/BluetoothGATTRemoteServer.h:59: // https://crbug.com/579746 You can probably remove this line.
https://codereview.chromium.org/1611773002/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/bluetooth/disconnect-frame-detached/detach-gc.html (right): https://codereview.chromium.org/1611773002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/bluetooth/disconnect-frame-detached/detach-gc.html:21: assert_unreached('iframe send invalid data: ' + messageEvent.data); On 2016/01/22 21:59:01, Jeffrey Yasskin wrote: > s/send/sent/ I think Done. https://codereview.chromium.org/1611773002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/bluetooth/disconnect-frame-detached/detach-gc.html:26: iframe.src = '../resources/disconnect-tab-in-iframe.html'; On 2016/01/22 21:59:01, Jeffrey Yasskin wrote: > I don't see this file. Did you forget to add it? It should be connectGATT-iframe.html https://codereview.chromium.org/1611773002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/bluetooth/disconnect-frame-detached/detach-gc.html:28: }, 'Detach frame then garbage collect. We shouln\'t crash.'); On 2016/01/22 21:59:01, Jeffrey Yasskin wrote: > spelling: shouln't Done. Also fixed in the other files. https://codereview.chromium.org/1611773002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/bluetooth/BluetoothGATTRemoteServer.h (right): https://codereview.chromium.org/1611773002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/bluetooth/BluetoothGATTRemoteServer.h:59: // https://crbug.com/579746 On 2016/01/22 21:59:02, Jeffrey Yasskin wrote: > You can probably remove this line. Done.
lgtm
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/1611773002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1611773002/80001
The CQ bit was unchecked by ortuno@chromium.org
ortuno@chromium.org changed reviewers: + haraken@chromium.org
haraken: We want to send a message to the browser process to disconnect from a Bluetooth device whenever the page becomes hidden, the document is detached or the object is garbage collected. Who would be the best person to review this change?
https://codereview.chromium.org/1611773002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/bluetooth/BluetoothDevice.cpp (right): https://codereview.chromium.org/1611773002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/bluetooth/BluetoothDevice.cpp:22: PageVisibilityState getPageVisibilityState(ScriptState* scriptState) As commented in the other file, once we have Page::isVisible(), this helper function wouldn't be needed. https://codereview.chromium.org/1611773002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/bluetooth/BluetoothGATTRemoteServer.cpp (right): https://codereview.chromium.org/1611773002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/bluetooth/BluetoothGATTRemoteServer.cpp:23: bool isPageVisible(Page* page) Can we add this helper function to Page? (i.e., Page::isVisible()) https://codereview.chromium.org/1611773002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/bluetooth/BluetoothGATTRemoteServer.cpp:70: WebBluetooth* webbluetooth = BluetoothSupplement::fromExecutionContext(executionContext()); I think we've discussed this before, but it's a bit nasty to have BluetoothSupplement::fromExecutionContext and BluetoothSupplement::fromScriptState. Given that BluetoothSupplement is a supplement of a Frame, it should provide only BluetoothSupplement::from(LocalFrame*). The caller side (e.g., BluetoothGATTRemoteServer) should hold a Member<Frame> and use BluetoothSupplement::from(m_frame) to retrieve the supplement. (In the case of BluetoothGATTRemoteServer, it doesn't need to hold m_frame because it can retrieve the frame from executionContext(). It can just provide BluetoothGATTRemoteServer::frame() which returns the frame associated with executionContext(). Then it can call BluetoothSupplement::from(frame())). (You don't need to address this in this CL.) https://codereview.chromium.org/1611773002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/bluetooth/BluetoothGATTRemoteServer.cpp:84: WebBluetooth* webbluetooth = BluetoothSupplement::fromScriptState(scriptState); This is not really nice for the same reason (although it's not wrong). We want to write this like BluetoothSupplement::from(frame())). Then you don't need to pass in ScriptState. You don't need to address this in this CL but consider removing BluetoothSupplement::fromScriptState and BluetoothSupplement::fromExecutionContext in a follow-up.
https://codereview.chromium.org/1611773002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/bluetooth/BluetoothGATTRemoteServer.cpp (right): https://codereview.chromium.org/1611773002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/bluetooth/BluetoothGATTRemoteServer.cpp:70: WebBluetooth* webbluetooth = BluetoothSupplement::fromExecutionContext(executionContext()); On 2016/01/23 02:02:02, haraken wrote: > > I think we've discussed this before, but it's a bit nasty to have > BluetoothSupplement::fromExecutionContext and > BluetoothSupplement::fromScriptState. Given that BluetoothSupplement is a > supplement of a Frame, it should provide only > BluetoothSupplement::from(LocalFrame*). > > The caller side (e.g., BluetoothGATTRemoteServer) should hold a Member<Frame> > and use BluetoothSupplement::from(m_frame) to retrieve the supplement. > > (In the case of BluetoothGATTRemoteServer, it doesn't need to hold m_frame > because it can retrieve the frame from executionContext(). It can just provide > BluetoothGATTRemoteServer::frame() which returns the frame associated with > executionContext(). Then it can call BluetoothSupplement::from(frame())). > > (You don't need to address this in this CL.) FWIW, I disagree. There are uses of BluetoothSupplement in 5 different files, none of which have a frame immediately available. They all have either a ScriptState or an ExecutionContext, and accepting these in methods on BluetoothSupplement allows us to centralize the non-trivial knowledge of how to get from the object we have to the Frame. I don't want to duplicate this code.
On 2016/01/23 02:33:36, Jeffrey Yasskin wrote: > https://codereview.chromium.org/1611773002/diff/80001/third_party/WebKit/Sour... > File third_party/WebKit/Source/modules/bluetooth/BluetoothGATTRemoteServer.cpp > (right): > > https://codereview.chromium.org/1611773002/diff/80001/third_party/WebKit/Sour... > third_party/WebKit/Source/modules/bluetooth/BluetoothGATTRemoteServer.cpp:70: > WebBluetooth* webbluetooth = > BluetoothSupplement::fromExecutionContext(executionContext()); > On 2016/01/23 02:02:02, haraken wrote: > > > > I think we've discussed this before, but it's a bit nasty to have > > BluetoothSupplement::fromExecutionContext and > > BluetoothSupplement::fromScriptState. Given that BluetoothSupplement is a > > supplement of a Frame, it should provide only > > BluetoothSupplement::from(LocalFrame*). > > > > The caller side (e.g., BluetoothGATTRemoteServer) should hold a Member<Frame> > > and use BluetoothSupplement::from(m_frame) to retrieve the supplement. > > > > (In the case of BluetoothGATTRemoteServer, it doesn't need to hold m_frame > > because it can retrieve the frame from executionContext(). It can just provide > > BluetoothGATTRemoteServer::frame() which returns the frame associated with > > executionContext(). Then it can call BluetoothSupplement::from(frame())). > > > > (You don't need to address this in this CL.) > > FWIW, I disagree. There are uses of BluetoothSupplement in 5 different files, > none of which have a frame immediately available. They all have either a > ScriptState or an ExecutionContext, and accepting these in methods on > BluetoothSupplement allows us to centralize the non-trivial knowledge of how to > get from the object we have to the Frame. I don't want to duplicate this code. Why can't you cache the Frame on the object when the object is created? My point is that (it's not wrong but) it's a bit nasty to rely on a JS to give the object a ScriptState and try to retrieve a Frame from there. (I'm not complained about the helper method itself. My point is that which Frame the object should use should be managed in the DOM side without relying on JS.) BluotoothSupplement is the only supplement that wants the fromExecutionContext and fromScriptState version (as far as I quickly grep).
On 2016/01/23 02:42:38, haraken wrote: > On 2016/01/23 02:33:36, Jeffrey Yasskin wrote: > > > https://codereview.chromium.org/1611773002/diff/80001/third_party/WebKit/Sour... > > File third_party/WebKit/Source/modules/bluetooth/BluetoothGATTRemoteServer.cpp > > (right): > > > > > https://codereview.chromium.org/1611773002/diff/80001/third_party/WebKit/Sour... > > third_party/WebKit/Source/modules/bluetooth/BluetoothGATTRemoteServer.cpp:70: > > WebBluetooth* webbluetooth = > > BluetoothSupplement::fromExecutionContext(executionContext()); > > On 2016/01/23 02:02:02, haraken wrote: > > > > > > I think we've discussed this before, but it's a bit nasty to have > > > BluetoothSupplement::fromExecutionContext and > > > BluetoothSupplement::fromScriptState. Given that BluetoothSupplement is a > > > supplement of a Frame, it should provide only > > > BluetoothSupplement::from(LocalFrame*). > > > > > > The caller side (e.g., BluetoothGATTRemoteServer) should hold a > Member<Frame> > > > and use BluetoothSupplement::from(m_frame) to retrieve the supplement. > > > > > > (In the case of BluetoothGATTRemoteServer, it doesn't need to hold m_frame > > > because it can retrieve the frame from executionContext(). It can just > provide > > > BluetoothGATTRemoteServer::frame() which returns the frame associated with > > > executionContext(). Then it can call BluetoothSupplement::from(frame())). > > > > > > (You don't need to address this in this CL.) > > > > FWIW, I disagree. There are uses of BluetoothSupplement in 5 different files, > > none of which have a frame immediately available. They all have either a > > ScriptState or an ExecutionContext, and accepting these in methods on > > BluetoothSupplement allows us to centralize the non-trivial knowledge of how > to > > get from the object we have to the Frame. I don't want to duplicate this code. > > Why can't you cache the Frame on the object when the object is created? > > My point is that (it's not wrong but) it's a bit nasty to rely on a JS to give > the object a ScriptState and try to retrieve a Frame from there. (I'm not > complained about the helper method itself. My point is that which Frame the > object should use should be managed in the DOM side without relying on JS.) > BluotoothSupplement is the only supplement that wants the fromExecutionContext > and fromScriptState version (as far as I quickly grep). Ah, that makes more sense, thanks. Also, by asking the bindings layer to give us "the" ScriptState, are we going to get the wrong one for cross-iframe calls? However, these objects are created by a CallbackPromiseAdapter created from a ScriptPromiseResolver::create(scriptState), where that scriptState is _also_ provided by JS. ... We'll need to have BluetoothDispatcher look up its RenderFrame and use that to get a WebFrame to pass into the WebBluetoothGATTRemoteServer, etc., rather than using the ScriptState passed to BluetoothGATTRemoteServer::take(). Looks totally doable, but if we'd done the simplest thing that would remove the helper functions, we'd have done the wrong thing.
On 2016/01/23 03:10:42, Jeffrey Yasskin wrote: > On 2016/01/23 02:42:38, haraken wrote: > > On 2016/01/23 02:33:36, Jeffrey Yasskin wrote: > > > > > > https://codereview.chromium.org/1611773002/diff/80001/third_party/WebKit/Sour... > > > File > third_party/WebKit/Source/modules/bluetooth/BluetoothGATTRemoteServer.cpp > > > (right): > > > > > > > > > https://codereview.chromium.org/1611773002/diff/80001/third_party/WebKit/Sour... > > > > third_party/WebKit/Source/modules/bluetooth/BluetoothGATTRemoteServer.cpp:70: > > > WebBluetooth* webbluetooth = > > > BluetoothSupplement::fromExecutionContext(executionContext()); > > > On 2016/01/23 02:02:02, haraken wrote: > > > > > > > > I think we've discussed this before, but it's a bit nasty to have > > > > BluetoothSupplement::fromExecutionContext and > > > > BluetoothSupplement::fromScriptState. Given that BluetoothSupplement is a > > > > supplement of a Frame, it should provide only > > > > BluetoothSupplement::from(LocalFrame*). > > > > > > > > The caller side (e.g., BluetoothGATTRemoteServer) should hold a > > Member<Frame> > > > > and use BluetoothSupplement::from(m_frame) to retrieve the supplement. > > > > > > > > (In the case of BluetoothGATTRemoteServer, it doesn't need to hold m_frame > > > > because it can retrieve the frame from executionContext(). It can just > > provide > > > > BluetoothGATTRemoteServer::frame() which returns the frame associated with > > > > executionContext(). Then it can call BluetoothSupplement::from(frame())). > > > > > > > > (You don't need to address this in this CL.) > > > > > > FWIW, I disagree. There are uses of BluetoothSupplement in 5 different > files, > > > none of which have a frame immediately available. They all have either a > > > ScriptState or an ExecutionContext, and accepting these in methods on > > > BluetoothSupplement allows us to centralize the non-trivial knowledge of how > > to > > > get from the object we have to the Frame. I don't want to duplicate this > code. > > > > Why can't you cache the Frame on the object when the object is created? > > > > My point is that (it's not wrong but) it's a bit nasty to rely on a JS to give > > the object a ScriptState and try to retrieve a Frame from there. (I'm not > > complained about the helper method itself. My point is that which Frame the > > object should use should be managed in the DOM side without relying on JS.) > > BluotoothSupplement is the only supplement that wants the fromExecutionContext > > and fromScriptState version (as far as I quickly grep). > > Ah, that makes more sense, thanks. Also, by asking the bindings layer to give us > "the" ScriptState, are we going to get the wrong one for cross-iframe calls? It won't happen (so this wouldn't be a big deal -- this is just not nice :-). > > However, these objects are created by a CallbackPromiseAdapter created from a > ScriptPromiseResolver::create(scriptState), where that scriptState is _also_ > provided by JS. ... We'll need to have BluetoothDispatcher look up its > RenderFrame and use that to get a WebFrame to pass into the > WebBluetoothGATTRemoteServer, etc., rather than using the ScriptState passed to > BluetoothGATTRemoteServer::take(). Looks totally doable, but if we'd done the > simplest thing that would remove the helper functions, we'd have done the wrong > thing. As far as I understand, the conceptual model is: - BluetoothGATTRemoteServer should have an associated frame. When the BluetoothGATTRemoteServer needs to get a Bluetooth object, it should ask the frame to give the Bluetooth object. So it would make sense for the BluetoothGATTRemoteServer's constructor to get the frame from the passed-in ScriptState. What looks not nice is that you're adding [CallWith=ScriptState] to all methods in BluetoothGATTRemoteServer.idl so that each method can get a frame from the ScriptState. Alternately, we should just remove [CallWith=ScriptState] and use the frame (directly or indirectly) recorded on the BluetoothGATTRemoteServer's object. (Indirectly means it can get the frame via ActiveDOMObject::executionContext() etc.) (Again, this is just a nit-picking about the coding style. You don't need to address it in this CL.)
FWIW this is leftover for when we were targeting workers. Now that we are not immediately targeting workers and have a frame it makes sense to change it. On Fri, Jan 22, 2016, 7:33 PM <haraken@chromium.org> wrote: > On 2016/01/23 03:10:42, Jeffrey Yasskin wrote: > > On 2016/01/23 02:42:38, haraken wrote: > > > On 2016/01/23 02:33:36, Jeffrey Yasskin wrote: > > > > > > > > > > https://codereview.chromium.org/1611773002/diff/80001/third_party/WebKit/Sour... > > > > File > > third_party/WebKit/Source/modules/bluetooth/BluetoothGATTRemoteServer.cpp > > > > (right): > > > > > > > > > > > > > > https://codereview.chromium.org/1611773002/diff/80001/third_party/WebKit/Sour... > > > > > > > third_party/WebKit/Source/modules/bluetooth/BluetoothGATTRemoteServer.cpp:70: > > > > WebBluetooth* webbluetooth = > > > > BluetoothSupplement::fromExecutionContext(executionContext()); > > > > On 2016/01/23 02:02:02, haraken wrote: > > > > > > > > > > I think we've discussed this before, but it's a bit nasty to have > > > > > BluetoothSupplement::fromExecutionContext and > > > > > BluetoothSupplement::fromScriptState. Given that > > BluetoothSupplement is > a > > > > > supplement of a Frame, it should provide only > > > > > BluetoothSupplement::from(LocalFrame*). > > > > > > > > > > The caller side (e.g., BluetoothGATTRemoteServer) should hold a > > > Member<Frame> > > > > > and use BluetoothSupplement::from(m_frame) to retrieve the > > supplement. > > > > > > > > > > (In the case of BluetoothGATTRemoteServer, it doesn't need to hold > m_frame > > > > > because it can retrieve the frame from executionContext(). It can > > just > > > provide > > > > > BluetoothGATTRemoteServer::frame() which returns the frame > > associated > with > > > > > executionContext(). Then it can call > BluetoothSupplement::from(frame())). > > > > > > > > > > (You don't need to address this in this CL.) > > > > > > > > FWIW, I disagree. There are uses of BluetoothSupplement in 5 > different > > files, > > > > none of which have a frame immediately available. They all have > > either a > > > > ScriptState or an ExecutionContext, and accepting these in methods on > > > > BluetoothSupplement allows us to centralize the non-trivial knowledge > > of > how > > > to > > > > get from the object we have to the Frame. I don't want to duplicate > > this > > code. > > > > > > Why can't you cache the Frame on the object when the object is created? > > > > > > My point is that (it's not wrong but) it's a bit nasty to rely on a JS > > to > give > > > the object a ScriptState and try to retrieve a Frame from there. (I'm > > not > > > complained about the helper method itself. My point is that which Frame > > the > > > object should use should be managed in the DOM side without relying on > > JS.) > > > BluotoothSupplement is the only supplement that wants the > fromExecutionContext > > > and fromScriptState version (as far as I quickly grep). > > > Ah, that makes more sense, thanks. Also, by asking the bindings layer to > > give > us > > "the" ScriptState, are we going to get the wrong one for cross-iframe > > calls? > > It won't happen (so this wouldn't be a big deal -- this is just not > nice :-). > > > > However, these objects are created by a CallbackPromiseAdapter created > > from a > > ScriptPromiseResolver::create(scriptState), where that scriptState is > > _also_ > > provided by JS. ... We'll need to have BluetoothDispatcher look up its > > RenderFrame and use that to get a WebFrame to pass into the > > WebBluetoothGATTRemoteServer, etc., rather than using the ScriptState > > passed > to > > BluetoothGATTRemoteServer::take(). Looks totally doable, but if we'd done > > the > > simplest thing that would remove the helper functions, we'd have done the > wrong > > thing. > > As far as I understand, the conceptual model is: > > - BluetoothGATTRemoteServer should have an associated frame. When the > BluetoothGATTRemoteServer needs to get a Bluetooth object, it should ask > the > frame to give the Bluetooth object. > > So it would make sense for the BluetoothGATTRemoteServer's constructor to > get > the frame from the passed-in ScriptState. What looks not nice is that > you're > adding [CallWith=ScriptState] to all methods in > BluetoothGATTRemoteServer.idl so > that each method can get a frame from the ScriptState. Alternately, we > should > just remove [CallWith=ScriptState] and use the frame (directly or > indirectly) > recorded on the BluetoothGATTRemoteServer's object. (Indirectly means it > can get > the frame via ActiveDOMObject::executionContext() etc.) > > (Again, this is just a nit-picking about the coding style. You don't need > to > address it in this CL.) > > > https://codereview.chromium.org/1611773002/ > -- You received this message because you are subscribed to the Google Groups "Blink Reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
FWIW this is leftover for when we were targeting workers. Now that we are not immediately targeting workers and have a frame it makes sense to change it. On Fri, Jan 22, 2016, 7:33 PM <haraken@chromium.org> wrote: > On 2016/01/23 03:10:42, Jeffrey Yasskin wrote: > > On 2016/01/23 02:42:38, haraken wrote: > > > On 2016/01/23 02:33:36, Jeffrey Yasskin wrote: > > > > > > > > > > https://codereview.chromium.org/1611773002/diff/80001/third_party/WebKit/Sour... > > > > File > > third_party/WebKit/Source/modules/bluetooth/BluetoothGATTRemoteServer.cpp > > > > (right): > > > > > > > > > > > > > > https://codereview.chromium.org/1611773002/diff/80001/third_party/WebKit/Sour... > > > > > > > third_party/WebKit/Source/modules/bluetooth/BluetoothGATTRemoteServer.cpp:70: > > > > WebBluetooth* webbluetooth = > > > > BluetoothSupplement::fromExecutionContext(executionContext()); > > > > On 2016/01/23 02:02:02, haraken wrote: > > > > > > > > > > I think we've discussed this before, but it's a bit nasty to have > > > > > BluetoothSupplement::fromExecutionContext and > > > > > BluetoothSupplement::fromScriptState. Given that > > BluetoothSupplement is > a > > > > > supplement of a Frame, it should provide only > > > > > BluetoothSupplement::from(LocalFrame*). > > > > > > > > > > The caller side (e.g., BluetoothGATTRemoteServer) should hold a > > > Member<Frame> > > > > > and use BluetoothSupplement::from(m_frame) to retrieve the > > supplement. > > > > > > > > > > (In the case of BluetoothGATTRemoteServer, it doesn't need to hold > m_frame > > > > > because it can retrieve the frame from executionContext(). It can > > just > > > provide > > > > > BluetoothGATTRemoteServer::frame() which returns the frame > > associated > with > > > > > executionContext(). Then it can call > BluetoothSupplement::from(frame())). > > > > > > > > > > (You don't need to address this in this CL.) > > > > > > > > FWIW, I disagree. There are uses of BluetoothSupplement in 5 > different > > files, > > > > none of which have a frame immediately available. They all have > > either a > > > > ScriptState or an ExecutionContext, and accepting these in methods on > > > > BluetoothSupplement allows us to centralize the non-trivial knowledge > > of > how > > > to > > > > get from the object we have to the Frame. I don't want to duplicate > > this > > code. > > > > > > Why can't you cache the Frame on the object when the object is created? > > > > > > My point is that (it's not wrong but) it's a bit nasty to rely on a JS > > to > give > > > the object a ScriptState and try to retrieve a Frame from there. (I'm > > not > > > complained about the helper method itself. My point is that which Frame > > the > > > object should use should be managed in the DOM side without relying on > > JS.) > > > BluotoothSupplement is the only supplement that wants the > fromExecutionContext > > > and fromScriptState version (as far as I quickly grep). > > > Ah, that makes more sense, thanks. Also, by asking the bindings layer to > > give > us > > "the" ScriptState, are we going to get the wrong one for cross-iframe > > calls? > > It won't happen (so this wouldn't be a big deal -- this is just not > nice :-). > > > > However, these objects are created by a CallbackPromiseAdapter created > > from a > > ScriptPromiseResolver::create(scriptState), where that scriptState is > > _also_ > > provided by JS. ... We'll need to have BluetoothDispatcher look up its > > RenderFrame and use that to get a WebFrame to pass into the > > WebBluetoothGATTRemoteServer, etc., rather than using the ScriptState > > passed > to > > BluetoothGATTRemoteServer::take(). Looks totally doable, but if we'd done > > the > > simplest thing that would remove the helper functions, we'd have done the > wrong > > thing. > > As far as I understand, the conceptual model is: > > - BluetoothGATTRemoteServer should have an associated frame. When the > BluetoothGATTRemoteServer needs to get a Bluetooth object, it should ask > the > frame to give the Bluetooth object. > > So it would make sense for the BluetoothGATTRemoteServer's constructor to > get > the frame from the passed-in ScriptState. What looks not nice is that > you're > adding [CallWith=ScriptState] to all methods in > BluetoothGATTRemoteServer.idl so > that each method can get a frame from the ScriptState. Alternately, we > should > just remove [CallWith=ScriptState] and use the frame (directly or > indirectly) > recorded on the BluetoothGATTRemoteServer's object. (Indirectly means it > can get > the frame via ActiveDOMObject::executionContext() etc.) > > (Again, this is just a nit-picking about the coding style. You don't need > to > address it in this CL.) > > > https://codereview.chromium.org/1611773002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2016/01/23 03:37:34, chromium-reviews wrote: > FWIW this is leftover for when we were targeting workers. Now that we are > not immediately targeting workers and have a frame it makes sense to change > it. That sounds nice. Regarding this CL, the usage of PageLifecycleObserver, ActiveDOMObject and pre-finalizer looks good. Once the comment in #16 (other than the ScriptState's one) is addressed, LG.
https://codereview.chromium.org/1611773002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/bluetooth/BluetoothDevice.cpp (right): https://codereview.chromium.org/1611773002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/bluetooth/BluetoothDevice.cpp:22: PageVisibilityState getPageVisibilityState(ScriptState* scriptState) On 2016/01/23 at 02:02:02, haraken wrote: > As commented in the other file, once we have Page::isVisible(), this helper function wouldn't be needed. Done. https://codereview.chromium.org/1611773002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/bluetooth/BluetoothGATTRemoteServer.cpp (right): https://codereview.chromium.org/1611773002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/bluetooth/BluetoothGATTRemoteServer.cpp:23: bool isPageVisible(Page* page) On 2016/01/23 at 02:02:02, haraken wrote: > Can we add this helper function to Page? (i.e., Page::isVisible()) done. https://codereview.chromium.org/1611773002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/bluetooth/BluetoothGATTRemoteServer.cpp:84: WebBluetooth* webbluetooth = BluetoothSupplement::fromScriptState(scriptState); On 2016/01/23 at 02:02:02, haraken wrote: > This is not really nice for the same reason (although it's not wrong). > > We want to write this like BluetoothSupplement::from(frame())). Then you don't need to pass in ScriptState. > > You don't need to address this in this CL but consider removing BluetoothSupplement::fromScriptState and BluetoothSupplement::fromExecutionContext in a follow-up. Definitely in my TODO list. We used to be targeting workers that's why we couldn't store the frame. Ever since we changed that I've been meaning to refactor this pattern.
Thanks, 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 Link to the patchset: https://codereview.chromium.org/1611773002/#ps100001 (title: "Address haraken's comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1611773002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1611773002/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== bluetooth: Disconnect if the page becomes hidden or is closed. This means closing connection when: 1. BluetoothGATTRemoteServer is destroyed. 2. The parent document is detached. 3. The page becomes hidden. BUG=579746 ========== to ========== bluetooth: Disconnect if the page becomes hidden or is closed. This means closing connection when: 1. BluetoothGATTRemoteServer is destroyed. 2. The parent document is detached. 3. The page becomes hidden. BUG=579746 Committed: https://crrev.com/2eec1c80c538f99ac3538a40c4a6584079ff7d47 Cr-Commit-Position: refs/heads/master@{#371542} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/2eec1c80c538f99ac3538a40c4a6584079ff7d47 Cr-Commit-Position: refs/heads/master@{#371542} |
