Issue 1334763002: bluetooth: Subscribe to notifications

ortuno

Patchset #1 (id:1) has been deleted

5 years, 3 months ago (2015-09-10 18:15:00 UTC) #1

ortuno

ortuno@chromium.org changed reviewers: + jyasskin@chromium.org

5 years, 3 months ago (2015-09-10 18:15:32 UTC) #2

Jeffrey Yasskin

You won't be able to test the "2 frames" problem without either events or 'Characteristic.value', ...

5 years, 3 months ago (2015-09-12 02:36:05 UTC) #4

Jeffrey Yasskin

https://codereview.chromium.org/1334763002/diff/20001/content/browser/bluetooth/bluetooth_dispatcher_host.cc File content/browser/bluetooth/bluetooth_dispatcher_host.cc (right): https://codereview.chromium.org/1334763002/diff/20001/content/browser/bluetooth/bluetooth_dispatcher_host.cc#newcode784 content/browser/bluetooth/bluetooth_dispatcher_host.cc:784: notify_session_iter->second->Stop( On 2015/09/12 02:36:05, Jeffrey Yasskin wrote: > If ...

5 years, 3 months ago (2015-09-12 02:50:22 UTC) #5

ortuno

https://codereview.chromium.org/1334763002/diff/20001/content/browser/bluetooth/bluetooth_dispatcher_host.cc File content/browser/bluetooth/bluetooth_dispatcher_host.cc (right): https://codereview.chromium.org/1334763002/diff/20001/content/browser/bluetooth/bluetooth_dispatcher_host.cc#newcode784 content/browser/bluetooth/bluetooth_dispatcher_host.cc:784: notify_session_iter->second->Stop( On 2015/09/12 at 02:50:22, Jeffrey Yasskin wrote: > ...

5 years, 3 months ago (2015-09-13 18:35:59 UTC) #6

Jeffrey Yasskin

2 tabs on the same site can share the same process, and there's a global ...

5 years, 3 months ago (2015-09-13 18:53:41 UTC) #7

chromium-reviews

Argh. Forget I said this until tomorrow. Go have a weekend. On Sep 13, 2015 ...

5 years, 3 months ago (2015-09-13 18:58:44 UTC) #8

Argh. Forget I said this until tomorrow. Go have a weekend.
On Sep 13, 2015 11:53 AM, "Jeffrey Yasskin" <jyasskin@chromium.org> wrote:

> 2 tabs on the same site can share the same process, and there's a global
> process limit that causes separate sites to share. Still, it makes sense to
> get the minimum working before making it resilient. Add a to-do with a bug?
> On Sep 13, 2015 11:36 AM, <ortuno@chromium.org> wrote:
>
>>
>>
>>
https://codereview.chromium.org/1334763002/diff/20001/content/browser/bluetoo...
>> File content/browser/bluetooth/bluetooth_dispatcher_host.cc (right):
>>
>>
>>
https://codereview.chromium.org/1334763002/diff/20001/content/browser/bluetoo...
>> content/browser/bluetooth/bluetooth_dispatcher_host.cc:784:
>> notify_session_iter->second->Stop(
>> On 2015/09/12 at 02:50:22, Jeffrey Yasskin wrote:
>>
>>> On 2015/09/12 02:36:05, Jeffrey Yasskin wrote:
>>> > If two different frames in the same process register for
>>>
>> notifications on the
>>
>>> > same characteristic, and then one of them unregisters, I believe
>>>
>> this will stop
>>
>>> > notifications for both. We need to somehow record which execution
>>>
>> contexts are
>>
>>> > actively watching.
>>>
>>
>>
>> I thought about that. I couldn't find any examples about two frames
>> sharing the same process, except for iframes but I thought we were not
>> supporting bluetooth inside iframes http://crbug.com/518042
>>
>> esprehn says we should make something on the renderer side an
>>>
>> ActiveDOMObject, and refcount how many of those are keeping active
>> notification sessions. Ojan warns that ActiveDOMObject is a wonky API
>>
>> Hmm yeah I didn't make BluetoothCharacteristic an ActiveDOMObject
>> because I was trying to go for the smallest CL to unlock the rolling
>> spider demo. The CL that adds the events will make our object
>> ActiveDOMObject.
>>
>> https://codereview.chromium.org/1334763002/
>>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

chromium-reviews

I thought that was before site isolation and RenderFrame(Host): "We will have a full RenderFrame ...

5 years, 3 months ago (2015-09-13 19:00:20 UTC) #9

I thought that was before site isolation and RenderFrame(Host): "We will
have a full RenderFrame for every active frame, regardless of process"

On Sun, Sep 13, 2015 at 11:53 AM Jeffrey Yasskin <jyasskin@chromium.org>
wrote:

> 2 tabs on the same site can share the same process, and there's a global
> process limit that causes separate sites to share. Still, it makes sense to
> get the minimum working before making it resilient. Add a to-do with a bug?
> On Sep 13, 2015 11:36 AM, <ortuno@chromium.org> wrote:
>
>>
>>
>>
https://codereview.chromium.org/1334763002/diff/20001/content/browser/bluetoo...
>> File content/browser/bluetooth/bluetooth_dispatcher_host.cc (right):
>>
>>
>>
https://codereview.chromium.org/1334763002/diff/20001/content/browser/bluetoo...
>> content/browser/bluetooth/bluetooth_dispatcher_host.cc:784:
>> notify_session_iter->second->Stop(
>> On 2015/09/12 at 02:50:22, Jeffrey Yasskin wrote:
>>
>>> On 2015/09/12 02:36:05, Jeffrey Yasskin wrote:
>>> > If two different frames in the same process register for
>>>
>> notifications on the
>>
>>> > same characteristic, and then one of them unregisters, I believe
>>>
>> this will stop
>>
>>> > notifications for both. We need to somehow record which execution
>>>
>> contexts are
>>
>>> > actively watching.
>>>
>>
>>
>> I thought about that. I couldn't find any examples about two frames
>> sharing the same process, except for iframes but I thought we were not
>> supporting bluetooth inside iframes http://crbug.com/518042
>>
>> esprehn says we should make something on the renderer side an
>>>
>> ActiveDOMObject, and refcount how many of those are keeping active
>> notification sessions. Ojan warns that ActiveDOMObject is a wonky API
>>
>> Hmm yeah I didn't make BluetoothCharacteristic an ActiveDOMObject
>> because I was trying to go for the smallest CL to unlock the rolling
>> spider demo. The CL that adds the events will make our object
>> ActiveDOMObject.
>>
>> https://codereview.chromium.org/1334763002/
>>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

Jeffrey Yasskin

Before site isolation, it's worse: 2 frames (a tab is a kind of frame, here) ...

5 years, 3 months ago (2015-09-14 07:02:01 UTC) #10

Before site isolation, it's worse: 2 frames (a tab is a kind of frame,
here) from different sites can share a process, even before running into
the limit.

I'm not following what that quote implies here, sorry.

On Sun, Sep 13, 2015 at 12:00 PM, Giovanni Ortuño <ortuno@google.com> wrote:

> I thought that was before site isolation and RenderFrame(Host): "We will
> have a full RenderFrame for every active frame, regardless of process"
>
> On Sun, Sep 13, 2015 at 11:53 AM Jeffrey Yasskin <jyasskin@chromium.org>
> wrote:
>
>> 2 tabs on the same site can share the same process, and there's a global
>> process limit that causes separate sites to share. Still, it makes sense to
>> get the minimum working before making it resilient. Add a to-do with a bug?
>> On Sep 13, 2015 11:36 AM, <ortuno@chromium.org> wrote:
>>
>>>
>>>
>>>
https://codereview.chromium.org/1334763002/diff/20001/content/browser/bluetoo...
>>> File content/browser/bluetooth/bluetooth_dispatcher_host.cc (right):
>>>
>>>
>>>
https://codereview.chromium.org/1334763002/diff/20001/content/browser/bluetoo...
>>> content/browser/bluetooth/bluetooth_dispatcher_host.cc:784:
>>> notify_session_iter->second->Stop(
>>> On 2015/09/12 at 02:50:22, Jeffrey Yasskin wrote:
>>>
>>>> On 2015/09/12 02:36:05, Jeffrey Yasskin wrote:
>>>> > If two different frames in the same process register for
>>>>
>>> notifications on the
>>>
>>>> > same characteristic, and then one of them unregisters, I believe
>>>>
>>> this will stop
>>>
>>>> > notifications for both. We need to somehow record which execution
>>>>
>>> contexts are
>>>
>>>> > actively watching.
>>>>
>>>
>>>
>>> I thought about that. I couldn't find any examples about two frames
>>> sharing the same process, except for iframes but I thought we were not
>>> supporting bluetooth inside iframes http://crbug.com/518042
>>>
>>> esprehn says we should make something on the renderer side an
>>>>
>>> ActiveDOMObject, and refcount how many of those are keeping active
>>> notification sessions. Ojan warns that ActiveDOMObject is a wonky API
>>>
>>> Hmm yeah I didn't make BluetoothCharacteristic an ActiveDOMObject
>>> because I was trying to go for the smallest CL to unlock the rolling
>>> spider demo. The CL that adds the events will make our object
>>> ActiveDOMObject.
>>>
>>> https://codereview.chromium.org/1334763002/
>>>
>>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

Jeffrey Yasskin

https://codereview.chromium.org/1334763002/diff/40001/content/renderer/bluetooth/bluetooth_dispatcher.cc File content/renderer/bluetooth/bluetooth_dispatcher.cc (right): https://codereview.chromium.org/1334763002/diff/40001/content/renderer/bluetooth/bluetooth_dispatcher.cc#newcode77 content/renderer/bluetooth/bluetooth_dispatcher.cc:77: blink::WebBluetoothGATTCharacteristicDelegate* delegate; Can you comment how we know |delegate| ...

5 years, 3 months ago (2015-09-15 03:45:38 UTC) #11

ortuno

https://codereview.chromium.org/1334763002/diff/20001/content/common/bluetooth/bluetooth_messages.h File content/common/bluetooth/bluetooth_messages.h (right): https://codereview.chromium.org/1334763002/diff/20001/content/common/bluetooth/bluetooth_messages.h#newcode199 content/common/bluetooth/bluetooth_messages.h:199: // Informs the renderer that On 2015/09/12 at 02:36:05, ...

5 years, 3 months ago (2015-09-15 21:38:20 UTC) #12

https://codereview.chromium.org/1334763002/diff/20001/content/common/bluetoot...
File content/common/bluetooth/bluetooth_messages.h (right):

https://codereview.chromium.org/1334763002/diff/20001/content/common/bluetoot...
content/common/bluetooth/bluetooth_messages.h:199: // Informs the renderer that
On 2015/09/12 at 02:36:05, Jeffrey Yasskin wrote:
> This sentence is missing its

Done.

https://codereview.chromium.org/1334763002/diff/40001/content/renderer/blueto...
File content/renderer/bluetooth/bluetooth_dispatcher.cc (right):

https://codereview.chromium.org/1334763002/diff/40001/content/renderer/blueto...
content/renderer/bluetooth/bluetooth_dispatcher.cc:77:
blink::WebBluetoothGATTCharacteristicDelegate* delegate;
On 2015/09/15 at 03:45:37, Jeffrey Yasskin wrote:
> Can you comment how we know |delegate| will stay alive long enough? (what
object owns it?)

Well the execution context on the blink side owns the object and it can destroy
it at any point. But it does tell us when it's destroying the delegate so we can
remove any references to it. Added a note explaining that.

https://codereview.chromium.org/1334763002/diff/40001/content/renderer/blueto...
content/renderer/bluetooth/bluetooth_dispatcher.cc:266: Send(new
BluetoothHostMsg_StartNotifications(
On 2015/09/15 at 03:45:37, Jeffrey Yasskin wrote:
> Since you're creating the active_notifications entry when this IPC comes back,
make sure that if a concurrent call to startNotifications() comes in before this
IPC returns, it doesn't cause a second IPC to be sent. You'll probably need a
queue of callbacks that gets flushed when the IPC returns.

That seems like an unnecessary optimization. Sending another IPC doesn't really
cost us much and keeps the code simple.

https://codereview.chromium.org/1334763002/diff/40001/content/renderer/blueto...
content/renderer/bluetooth/bluetooth_dispatcher.cc:275: if
(!IsActiveCharacteristicNotification(characteristic_instance_id)) {
On 2015/09/15 at 03:45:37, Jeffrey Yasskin wrote:
> I think you don't need this check. Removing the element will be a no-op if the
notification wasn't active.

Reworked this part. Calling stopNotifications twice in a row would result in one
IPC being sent and the second call to stopNotifications succeeding immediately.

https://codereview.chromium.org/1334763002/diff/40001/content/renderer/blueto...
content/renderer/bluetooth/bluetooth_dispatcher.cc:333: if
(char_iter->second.size() == 0) {
On 2015/09/15 at 03:45:37, Jeffrey Yasskin wrote:
> We should make sure that we remove elements from the map when their set gets
empty, rather than leaving empty mappings around using up memory.

Done.

https://codereview.chromium.org/1334763002/diff/40001/content/renderer/blueto...
content/renderer/bluetooth/bluetooth_dispatcher.cc:342: auto char_iter =
active_characteristic_notifications.find(
On 2015/09/15 at 03:45:37, Jeffrey Yasskin wrote:
> This is a good place to use operator[]:
> 
> std::set<blink::WebBluetoothGATTCharacteristicDelegate*>& active_notifications
=
>   active_characteristic_notifications[characteristic_instance_id.utf8()];
> 
> bool should_register = active_notifications.empty();
> 
> active_notifications.insert(delegate);
> 
> return should_register;  // Avoid needing a double-lookup in the caller.

I don't like adding the delegates to the map before the ipc returns. Makes it
trickier to handle concurrent calls to start/stop.

https://codereview.chromium.org/1334763002/diff/40001/content/renderer/blueto...
content/renderer/bluetooth/bluetooth_dispatcher.cc:367:
char_iter->second.erase(delegate);
On 2015/09/15 at 03:45:37, Jeffrey Yasskin wrote:
> If the set became empty, remove it from the map here.

Done.

https://codereview.chromium.org/1334763002/diff/40001/content/renderer/blueto...
content/renderer/bluetooth/bluetooth_dispatcher.cc:537: if (request->delegate !=
nullptr) {
On 2015/09/15 at 03:45:37, Jeffrey Yasskin wrote:
> Could you comment that characteristicDelegateRemoved() nulls this out?

Done.

https://codereview.chromium.org/1334763002/diff/40001/content/renderer/blueto...
content/renderer/bluetooth/bluetooth_dispatcher.cc:565:
RemoveFromActiveNotificationsMap(request->characteristic_instance_id,
On 2015/09/15 at 03:45:37, Jeffrey Yasskin wrote:
> You already called this before sending the stop IPC.

No longer calling this from stopNotifications.

https://codereview.chromium.org/1334763002/diff/40001/content/renderer/blueto...
File content/renderer/bluetooth/bluetooth_dispatcher.h (right):

https://codereview.chromium.org/1334763002/diff/40001/content/renderer/blueto...
content/renderer/bluetooth/bluetooth_dispatcher.h:93: bool
IsActiveCharacteristicNotification(
On 2015/09/15 at 03:45:37, Jeffrey Yasskin wrote:
> Maybe s/Is/Has/?

Done.

https://codereview.chromium.org/1334763002/diff/40001/content/renderer/blueto...
content/renderer/bluetooth/bluetooth_dispatcher.h:150: // Map that keeps track
of which characteristic are subscribed to
On 2015/09/15 at 03:45:37, Jeffrey Yasskin wrote:
> Mention that the keys are characteristic UUIDs.

You mean instance IDs right? Multiple characteristic could have the same UUID.

ortuno

https://codereview.chromium.org/1334763002/diff/60001/content/renderer/bluetooth/bluetooth_dispatcher.cc File content/renderer/bluetooth/bluetooth_dispatcher.cc (right): https://codereview.chromium.org/1334763002/diff/60001/content/renderer/bluetooth/bluetooth_dispatcher.cc#newcode561 content/renderer/bluetooth/bluetooth_dispatcher.cc:561: if (request->delegate != nullptr) { Argh. This doesn't work. ...

5 years, 3 months ago (2015-09-16 16:30:08 UTC) #14

ortuno

The CQ bit was checked by ortuno@chromium.org to run a CQ dry run

5 years, 2 months ago (2015-09-25 19:41:15 UTC) #15

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1334763002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1334763002/160001

5 years, 2 months ago (2015-09-25 19:42:28 UTC) #16

Jeffrey Yasskin

Sorry, I didn't get through everything. Vince will have to finish up. https://codereview.chromium.org/1334763002/diff/180001/content/browser/bluetooth/bluetooth_dispatcher_host.cc File content/browser/bluetooth/bluetooth_dispatcher_host.cc ...

5 years, 2 months ago (2015-09-25 23:03:20 UTC) #18

ortuno

ortuno@chromium.org changed reviewers: + scheib@chromium.org

5 years, 2 months ago (2015-09-29 22:47:35 UTC) #19

ortuno

scheib: PTAL. Design doc and cases: https://docs.google.com/document/d/1WJgglgqvZnrt37iKf3YR4ULL6DkzzT64VGBL4ijTjr4 https://codereview.chromium.org/1334763002/diff/180001/content/browser/bluetooth/bluetooth_dispatcher_host.cc File content/browser/bluetooth/bluetooth_dispatcher_host.cc (right): https://codereview.chromium.org/1334763002/diff/180001/content/browser/bluetooth/bluetooth_dispatcher_host.cc#newcode720 content/browser/bluetooth/bluetooth_dispatcher_host.cc:720: Send(new BluetoothMsg_StartNotificationsSuccess(thread_id, ...

5 years, 2 months ago (2015-09-29 22:47:37 UTC) #20

scheib: PTAL. Design doc and cases:
https://docs.google.com/document/d/1WJgglgqvZnrt37iKf3YR4ULL6DkzzT64VGBL4ijTjr4

https://codereview.chromium.org/1334763002/diff/180001/content/browser/blueto...
File content/browser/bluetooth/bluetooth_dispatcher_host.cc (right):

https://codereview.chromium.org/1334763002/diff/180001/content/browser/blueto...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:720: Send(new
BluetoothMsg_StartNotificationsSuccess(thread_id, request_id));
On 2015/09/25 at 23:03:19, Jeffrey Yasskin wrote:
> This is vulnerable to races if a second BluetoothHostMsg_StartNotifications
comes in while the first is still registering. I assume that's handled in two
ways: 1) BluetoothDispatcher won't send 2 Start() requests concurrently (and we
don't support multiple thread_ids yet), and 2) OnStartNotifySessionSuccess()
will destroy its notification session if it can't insert it.
> 
> Comment this, please. If I've missed a real problem that'll require code to
fix, but it won't make the API unusable, probably file a bug and add a TODO
rather than fixing it in this CL.

This code was wrote before I implemented the queues in Dispatcher.
BluetoothDispatcher should never send a request for a characteristic already
subscribed to notifications. Added a DCHECK.

https://codereview.chromium.org/1334763002/diff/180001/content/browser/blueto...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:724: // TODO(ortuno):
Check if notify/indicate bit is set.
On 2015/09/25 at 23:03:19, Jeffrey Yasskin wrote:
> Add a blank line below this, since it's not connected to looking up the
characteristic.

Done.

https://codereview.chromium.org/1334763002/diff/180001/content/browser/blueto...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:1029: Send(new
BluetoothMsg_StartNotificationsSuccess(thread_id, request_id));
On 2015/09/25 at 23:03:19, Jeffrey Yasskin wrote:
> Call RecordStartNotificationsOutcome() here, right?

Done.

https://codereview.chromium.org/1334763002/diff/180001/content/browser/blueto...
File content/browser/bluetooth/bluetooth_dispatcher_host.h (right):

https://codereview.chromium.org/1334763002/diff/180001/content/browser/blueto...
content/browser/bluetooth/bluetooth_dispatcher_host.h:205:
base::ScopedPtrMap<std::string,
On 2015/09/25 at 23:03:19, Jeffrey Yasskin wrote:
> We'll probably need an additional |thread_id| key in this map, but since we
don't support workers yet, you can leave it as a TODO(crbug/NNNN).

Done. http://crbug.com/537372

https://codereview.chromium.org/1334763002/diff/180001/content/common/bluetoo...
File content/common/bluetooth/bluetooth_messages.h (right):

https://codereview.chromium.org/1334763002/diff/180001/content/common/bluetoo...
content/common/bluetooth/bluetooth_messages.h:194: 
On 2015/09/25 at 23:03:19, Jeffrey Yasskin wrote:
> Remove the extra blank line.

Done.

https://codereview.chromium.org/1334763002/diff/180001/content/renderer/bluet...
File content/renderer/bluetooth/bluetooth_dispatcher.cc (right):

https://codereview.chromium.org/1334763002/diff/180001/content/renderer/bluet...
content/renderer/bluetooth/bluetooth_dispatcher.cc:68: struct
BluetoothNotificationsRequest {
On 2015/09/25 at 23:03:19, Jeffrey Yasskin wrote:
> Maybe "BluetoothNotificationStateChangeRequest"? Or comment what the class
means, since it's not just requesting the set of notifications.

Done.

https://codereview.chromium.org/1334763002/diff/180001/content/renderer/bluet...
content/renderer/bluetooth/bluetooth_dispatcher.cc:83: // object implements
ActiveDOMObject we do get notify when it's being
On 2015/09/25 at 23:03:19, Jeffrey Yasskin wrote:
> Instead of "we get notified", can you mention the function that carries the
notification and nulls |characteristic|? You also don't need "Note that".
Otherwise, looks good.

Done.

https://codereview.chromium.org/1334763002/diff/180001/content/renderer/bluet...
content/renderer/bluetooth/bluetooth_dispatcher.cc:311: //   1. The subscription
was innactive already.
On 2015/09/25 at 23:03:19, Jeffrey Yasskin wrote:
> sp: innactive, and on the next line

Done.

https://codereview.chromium.org/1334763002/diff/180001/content/renderer/bluet...
content/renderer/bluetooth/bluetooth_dispatcher.cc:339:
pending_notifications_requests_.Add(new BluetoothNotificationsRequest(
On 2015/09/25 at 23:03:19, Jeffrey Yasskin wrote:
> It looks like you don't actually need the switch here and can just
unconditionally add the BluetoothNotificationsRequest?

Done.

https://codereview.chromium.org/1334763002/diff/180001/content/renderer/bluet...
content/renderer/bluetooth/bluetooth_dispatcher.cc:357: auto queue_iter =
queued_notification_requests_.find(
On 2015/09/25 at 23:03:19, Jeffrey Yasskin wrote:
> Also CHECK(queue_iter != queued_notification_requests_.end()).

Done.

https://codereview.chromium.org/1334763002/diff/180001/content/renderer/bluet...
content/renderer/bluetooth/bluetooth_dispatcher.cc:360:
DCHECK(queue_iter->second.front() == request_id);
On 2015/09/25 at 23:03:20, Jeffrey Yasskin wrote:
> Alias std::queue<int>& request_queue = queue_iter->second;

Done.

https://codereview.chromium.org/1334763002/diff/180001/content/renderer/bluet...
content/renderer/bluetooth/bluetooth_dispatcher.cc:393: return
active_notification_subscriptions_.find(characteristic_instance_id) !=
On 2015/09/25 at 23:03:19, Jeffrey Yasskin wrote:
> You can use ContainsKey() for this
(https://code.google.com/p/chromium/codesearch/#chromium/src/base/stl_util.h&s...)

Done.

https://codereview.chromium.org/1334763002/diff/180001/content/renderer/bluet...
content/renderer/bluetooth/bluetooth_dispatcher.cc:471: if (callbacks !=
nullptr) {
On 2015/09/25 at 23:03:19, Jeffrey Yasskin wrote:
> Do you mean callbacks or characteristic here? If this is wrong, add a
TODO(crbug/NNNN) for a test that would have caught it.

callbacks is correct. Added:

An object is destroyed 1) during a start request or 2) after a start
request. If 1), then OnStartNotificationsSuccess will queue a stop with
callback = nullptr and characteristic = nullptr which will eventually call
processStopNotificationsRequest. If 2), characteristic needs to be removed
from active_notifications_subscriptions so onCharacteristicObjectRemoved
will call processStopNotification with callback = nullptr only.

https://codereview.chromium.org/1334763002/diff/180001/content/renderer/bluet...
File content/renderer/bluetooth/bluetooth_dispatcher.h (right):

https://codereview.chromium.org/1334763002/diff/180001/content/renderer/bluet...
content/renderer/bluetooth/bluetooth_dispatcher.h:95: // To avoid races and
sending innecessary IPC messages we implement
On 2015/09/25 at 23:03:20, Jeffrey Yasskin wrote:
> s/innecessary/unnecessary/

Done.

https://codereview.chromium.org/1334763002/diff/180001/content/renderer/bluet...
content/renderer/bluetooth/bluetooth_dispatcher.h:99: // When an characteristic
object gets destroyed BluetoothDispatcher
On 2015/09/25 at 23:03:20, Jeffrey Yasskin wrote:
> s/an/a/

Done.

https://codereview.chromium.org/1334763002/diff/180001/content/renderer/bluet...
content/renderer/bluetooth/bluetooth_dispatcher.h:101: // a stop request should
be issued if the characterisic was susbscribed
On 2015/09/25 at 23:03:20, Jeffrey Yasskin wrote:
> sp: characterisic, susbscribed

Done.

https://codereview.chromium.org/1334763002/diff/180001/content/renderer/bluet...
content/renderer/bluetooth/bluetooth_dispatcher.h:103: std::map<std::string,
std::queue<int>> queued_notification_requests_;
On 2015/09/25 at 23:03:20, Jeffrey Yasskin wrote:
> Move this down to the section of data members. We generally don't interleave
member variables with member functions.

Done.

> Comment what the std::string and int mean.

Done.

https://codereview.chromium.org/1334763002/diff/180001/content/renderer/bluet...
content/renderer/bluetooth/bluetooth_dispatcher.h:207: std::map<std::string,
std::set<blink::WebBluetoothGATTCharacteristic*>>
On 2015/09/25 at 23:03:20, Jeffrey Yasskin wrote:
> You lost the comment saying what these strings mean. Browser-side
device::BluetoothGattCharacteristic::GetIdentifier(), right?

Done.

scheib

Link design doc from change description. https://codereview.chromium.org/1334763002/diff/200001/content/browser/bluetooth/bluetooth_dispatcher_host.cc File content/browser/bluetooth/bluetooth_dispatcher_host.cc (right): https://codereview.chromium.org/1334763002/diff/200001/content/browser/bluetooth/bluetooth_dispatcher_host.cc#newcode718 content/browser/bluetooth/bluetooth_dispatcher_host.cc:718: DCHECK( It's a ...

5 years, 2 months ago (2015-10-01 22:05:24 UTC) #21

ortuno

https://codereview.chromium.org/1334763002/diff/200001/content/browser/bluetooth/bluetooth_dispatcher_host.cc File content/browser/bluetooth/bluetooth_dispatcher_host.cc (right): https://codereview.chromium.org/1334763002/diff/200001/content/browser/bluetooth/bluetooth_dispatcher_host.cc#newcode718 content/browser/bluetooth/bluetooth_dispatcher_host.cc:718: DCHECK( On 2015/10/01 at 22:05:22, scheib wrote: > It's ...

5 years, 2 months ago (2015-10-03 04:03:04 UTC) #22

https://codereview.chromium.org/1334763002/diff/200001/content/browser/blueto...
File content/browser/bluetooth/bluetooth_dispatcher_host.cc (right):

https://codereview.chromium.org/1334763002/diff/200001/content/browser/blueto...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:718: DCHECK(
On 2015/10/01 at 22:05:22, scheib wrote:
> It's a compromised renderer then, right? So this should be 
bad_message::ReceivedBadMessage or CHECK.

Done.

https://codereview.chromium.org/1334763002/diff/200001/content/browser/blueto...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:722: // TODO(ortuno):
Check if notify/indicate bit is set.
On 2015/10/01 at 22:05:22, scheib wrote:
> Have an issue so we track to follow up on this.

Done.

https://codereview.chromium.org/1334763002/diff/200001/content/browser/blueto...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:726: // A
characteristic_instance_id not in the map implies a hostile renderer
On 2015/10/01 at 22:05:22, scheib wrote:
> 3rd time we're copying this block, so let's move explanation to top of file
as:
> // ID Not In Map Note:
> // A service, characteristic, or descriptor ID not in the corresponding
> // BluetoothDispatcherHost map 
> // [service_to_device_, characteristic_to_service_,
descriptor_to_characteristic_]
> // implies a hostile renderer because a renderer obtains the service id
> // from this class and it will be added to the map at that time.
> 
> 
> Then at these BadMessage sites 
> // Kill the renderer, see "ID Not In Map Note" above.
> 
> (you decide if you want to include the descriptors...)

Done.

https://codereview.chromium.org/1334763002/diff/200001/content/browser/blueto...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:740:
device::BluetoothDevice* device =
On 2015/10/01 at 22:05:22, scheib wrote:
> In a later clean up patch we should refactor out these blocks into something
similar to TranslateGATTError...And...RecordGATTOperationOutcome.

I already have a patch :P

https://codereview.chromium.org/1315573004

Not ready for review. Needs clean up.

https://codereview.chromium.org/1334763002/diff/200001/content/browser/blueto...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:1037: //
TranslateGATTError calls RecordGATTOperationOutcome.
On 2015/10/01 at 22:05:22, scheib wrote:
> In a clean up CL we should remove comments and rename to
TranslateGATTErrorAndRecordGATTOperationOutcome.

http://crbug.com/538876

https://codereview.chromium.org/1334763002/diff/200001/content/renderer/bluet...
File content/renderer/bluetooth/bluetooth_dispatcher.cc (right):

https://codereview.chromium.org/1334763002/diff/200001/content/renderer/bluet...
content/renderer/bluetooth/bluetooth_dispatcher.cc:315: //   2. The subscription
will become inactive.
On 2015/10/01 at 22:05:23, scheib wrote:
> Case 2 is only if another frame holds the subscription and releases, and can
only happen if !HasActiveNotificationSubscription, because there are no pending
notification requests and thus no action from this frame. I think just update
comments. Below I think #2 would go with the first if.

Not sure I follow: Case 2 happens when this is the last frame holding the
notification.

https://codereview.chromium.org/1334763002/diff/200001/content/renderer/bluet...
content/renderer/bluetooth/bluetooth_dispatcher.cc:323: // care of stopping the
notifications if the subscription becomes inactive.
On 2015/10/01 at 22:05:23, scheib wrote:
> Perhaps:
> For 3 calling processStopNotificationsRequest ensures the notification
subscription is released.

Done.

https://codereview.chromium.org/1334763002/diff/200001/content/renderer/bluet...
content/renderer/bluetooth/bluetooth_dispatcher.cc:325:
characteristic_instance_id.utf8(), characteristic,
On 2015/10/01 at 22:05:23, scheib wrote:
> characteristic should be nullptr

We still need to remove the characteristic from the set of active notifications.

https://codereview.chromium.org/1334763002/diff/200001/content/renderer/bluet...
content/renderer/bluetooth/bluetooth_dispatcher.cc:426:
blink::WebBluetoothGATTCharacteristic* characteristic =
On 2015/10/01 at 22:05:23, scheib wrote:
> characteristic may be nullptr due to characteristicObjectRemoved, in which
case we shouldn't Send a 'Start' right? For the 'Stop' we should.

That's tricky and seems like a question for Jeffrey: If the object gets
destroyed before we get to send a Start request should we even send the start
request?

https://codereview.chromium.org/1334763002/diff/200001/content/renderer/bluet...
content/renderer/bluetooth/bluetooth_dispatcher.cc:460: characteristic)) {
On 2015/10/01 at 22:05:23, scheib wrote:
> May be removing from here with characteristic == nullptr, when it was added
with a valid pointer. 

That's why we pass the characteristic and not nullptr in
characteristicObjectRemoved

Perhaps simplest solution is to change active_notification_subscriptions_ to
std::map<std::string, int>

Would int be the number of references? That seems plausible. We would still need
another set to store the WebBluetoothCharacteristics though.

https://codereview.chromium.org/1334763002/diff/200001/content/renderer/bluet...
content/renderer/bluetooth/bluetooth_dispatcher.cc:465: // An object is
destroyed 1) during a start request or 2) after a start
On 2015/10/01 at 22:05:23, scheib wrote:
> Reform comment more like this for readability:
> 
> Possibly arrived here after two characteristic object destruction cases:
> 1) Characteristic is destroyed with a Start request outstanding: 
>     Then OnStartNotificationsSuccess will queue a stop with 
>     callback = nullptr and characteristic = nullptr which will
>     eventually call processStopNotificationsRequest.
> 2) Characteristic is destroyed after Start request completes:
>     The characteristic needs to be removed from
>     active_notifications_subscriptions so
>     characteristicObjectRemoved will call
>     processStopNotification with callback = nullptr only.
> 
>     Comment didn't use "characteristicObjectRemoved"
>     
> After chat -- this can probably be simplified to much less text. Something
like:
> May be processing a request with nullptr callbacks due to
>   1) Outstanding start request handed in OnStartNotificationsSuccess
>       after characteristicObjectRemoved.
>   2) Characteristic destroyed with no pending requests in
>       characteristicObjectRemoved.

Done.

https://codereview.chromium.org/1334763002/diff/200001/content/renderer/bluet...
content/renderer/bluetooth/bluetooth_dispatcher.cc:654: // Since there could be
other characteristics waiting to subscribe, we queue
On 2015/10/01 at 22:05:23, scheib wrote:
> I understand sending a Stop when the object is destroyed. And it seems that we
could do it here or in characteristicRemoved. I don't understand "Since there
could be other characteristics waiting to subscribe". Which others? How is
Stopping helping them? If no others just trim comment.

// Frame 1
char.startNotifications()
// char gets destroyed
// Frame 2
char.startNotifications()

It's a small optimization that lets us keep the subscription alive by queuing
the Stop request after the start request from frame 2. By the time the Stop
request is processed frame 2 will hold the subscription.

https://codereview.chromium.org/1334763002/diff/200001/content/renderer/bluet...
File content/renderer/bluetooth/bluetooth_dispatcher.h (right):

https://codereview.chromium.org/1334763002/diff/200001/content/renderer/bluet...
content/renderer/bluetooth/bluetooth_dispatcher.h:95: // To avoid races and
sending unnecessary IPC messages we implement
On 2015/10/01 at 22:05:23, scheib wrote:
> Title this block so for readability and so it can be reference. 
> // Notifications Queueing Note:

Done.

https://codereview.chromium.org/1334763002/diff/200001/content/renderer/bluet...
content/renderer/bluetooth/bluetooth_dispatcher.h:104: // Helper functions for
notification requests queue.
On 2015/10/01 at 22:05:23, scheib wrote:
> End in a colon and blank line to show that this applies to the group.

Done.

https://codereview.chromium.org/1334763002/diff/200001/content/renderer/bluet...
content/renderer/bluetooth/bluetooth_dispatcher.h:112: void
ProcessQueuedNotificationRequests(int request_id);
On 2015/10/01 at 22:05:23, scheib wrote:
> ProcessQueuedNotificationRequests -> PopNotificationRequestQueueAndProcessNext

Done.

https://codereview.chromium.org/1334763002/diff/200001/content/renderer/bluet...
content/renderer/bluetooth/bluetooth_dispatcher.h:113: // Check if there is more
than one request in the queue i.e. if there
On 2015/10/01 at 22:05:23, scheib wrote:
> "Checks if ..." (from guide, see 'descriptive')
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Function_Comm...

> Also a few methods below.

Done.

https://codereview.chromium.org/1334763002/diff/200001/content/renderer/bluet...
content/renderer/bluetooth/bluetooth_dispatcher.h:115: bool
PendingNotificationRequest(
On 2015/10/01 at 22:05:23, scheib wrote:
> PendingNotificationRequest -> HasUnsentNotificationRequest

Unset doesn't reflect the state of the notification request. What about
HasNotificationRequestResponsePending?

https://codereview.chromium.org/1334763002/diff/200001/content/renderer/bluet...
content/renderer/bluetooth/bluetooth_dispatcher.h:127: // characteristic's
notifications. Returns if the subscription
On 2015/10/01 at 22:05:23, scheib wrote:
> Returns true if

Done.

https://codereview.chromium.org/1334763002/diff/200001/content/renderer/bluet...
content/renderer/bluetooth/bluetooth_dispatcher.h:135: // You should never call
this functions if PendingNotificationRequest
On 2015/10/01 at 22:05:23, scheib wrote:
> 'call these functions'

Done.

https://codereview.chromium.org/1334763002/diff/200001/content/renderer/bluet...
content/renderer/bluetooth/bluetooth_dispatcher.h:137: void
processStartNotificationsRequest(int request_id);
On 2015/10/01 at 22:05:23, scheib wrote:
> processStartNotificationsRequest -> SendStartNotificationsRequestIfNeeded

bikeshedding: ResolveOrSendStartNotificationsRequest()?

https://codereview.chromium.org/1334763002/diff/200001/content/renderer/bluet...
content/renderer/bluetooth/bluetooth_dispatcher.h:185: // See note about why we
queue notifications above.
On 2015/10/01 at 22:05:23, scheib wrote:
> Refer to note by name (so that it can be text searched easily), and location.
See "Notifications Queueing Note" above.
> 

Done.

> Also, this is a map of queues, but the name makes it seem like a single queue
of notification requests. Consider name: notification_queues_ or
notification_request_queues_.

Done.

https://codereview.chromium.org/1334763002/diff/200001/content/shell/browser/...
File content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc
(right):

https://codereview.chromium.org/1334763002/diff/200001/content/shell/browser/...
content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc:390:
LayoutTestBluetoothAdapterProvider::GetBaseGATTNotifySession(
On 2015/10/01 at 22:05:23, scheib wrote:
> After ::GetBaseGATTCharacteristic,
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Declaration_O...

Moved it after GetErrorCharacteristic since GetBaseGATTCharacteristic and
GetErrorCharacteristic belong under "Characteristics"

https://codereview.chromium.org/1334763002/diff/200001/content/shell/browser/...
File content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.h
(right):

https://codereview.chromium.org/1334763002/diff/200001/content/shell/browser/...
content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.h:163: 
On 2015/10/01 at 22:05:23, scheib wrote:
> Changing test data should have corresponding test spec changes, link in change
description.

There is an issue here:
https://github.com/WebBluetoothCG/web-bluetooth/issues/162

https://codereview.chromium.org/1334763002/diff/200001/content/shell/browser/...
content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.h:164:
// |HeartRateAdapter|
On 2015/10/01 at 22:05:23, scheib wrote:
> Why not use GlucoseHeartRateAdapter? For a 'normal' functioning device it
seems it can reuse an existing adapter.

This boils down to a design decision when refactoring this code. The Get*Adapter
methods are the ones that create the functioning device. Get*Device creates the
most basic device.

https://codereview.chromium.org/1334763002/diff/200001/content/shell/browser/...
content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.h:305:
// Inherits from |BaseDevice|(adapter, "Heart Rate Device", uuids,
On 2015/10/01 at 22:05:23, scheib wrote:
> from |ConnectableDevice|

Done.

https://codereview.chromium.org/1334763002/diff/200001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/bluetooth/notifications.html (right):

https://codereview.chromium.org/1334763002/diff/200001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/bluetooth/notifications.html:126: </script>
On 2015/10/01 at 22:05:23, scheib wrote:
> No tests here for object deletion. Can it be accomplished in LayoutTests? How
are other ActiveDOMObjects tested?

I checked battery, geolocation, midi, web sockets, indexeddb, none have tests.

Theoretically the characteristics should get deleted as soon as they are out of
scope but in practice I've seen them get deleted during the last tests.

https://codereview.chromium.org/1334763002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/bluetooth/BluetoothSupplement.cpp
(right):

https://codereview.chromium.org/1334763002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/bluetooth/BluetoothSupplement.cpp:39:
WebBluetooth* BluetoothSupplement::from(ScriptState* scriptState)
On 2015/10/01 at 22:05:23, scheib wrote:
> Ordering, here or in .h should adjust:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Declaration_O...

Done.

scheib

I've edited the change description with the design doc https://codereview.chromium.org/1334763002/diff/200001/content/renderer/bluetooth/bluetooth_dispatcher.cc File content/renderer/bluetooth/bluetooth_dispatcher.cc (right): https://codereview.chromium.org/1334763002/diff/200001/content/renderer/bluetooth/bluetooth_dispatcher.cc#newcode315 content/renderer/bluetooth/bluetooth_dispatcher.cc:315: ...

5 years, 2 months ago (2015-10-04 01:35:58 UTC) #23

I've edited the change description with the design doc

https://codereview.chromium.org/1334763002/diff/200001/content/renderer/bluet...
File content/renderer/bluetooth/bluetooth_dispatcher.cc (right):

https://codereview.chromium.org/1334763002/diff/200001/content/renderer/bluet...
content/renderer/bluetooth/bluetooth_dispatcher.cc:315: //   2. The subscription
will become inactive.
On 2015/10/03 04:03:03, ortuno wrote:
> On 2015/10/01 at 22:05:23, scheib wrote:
> > Case 2 is only if another frame holds the subscription and releases, and can
> only happen if !HasActiveNotificationSubscription, because there are no
pending
> notification requests and thus no action from this frame. I think just update
> comments. Below I think #2 would go with the first if.
> 
> Not sure I follow: Case 2 happens when this is the last frame holding the
> notification.

Add detail to the comments somehow explaining case 2. Why will the subscription
become inactive? There's no pending request for it to become inactive. Oh, is it
because this object is the last object keeping it active and we must take action
to release it?

https://codereview.chromium.org/1334763002/diff/200001/content/renderer/bluet...
content/renderer/bluetooth/bluetooth_dispatcher.cc:325:
characteristic_instance_id.utf8(), characteristic,
On 2015/10/03 04:03:03, ortuno wrote:
> On 2015/10/01 at 22:05:23, scheib wrote:
> > characteristic should be nullptr
> 
> We still need to remove the characteristic from the set of active
notifications.

OK, yeah, this is complicated enough that's not obvious. Perhaps update that
comment above adding "is released for this characteristic object" to help hint
further? :(

https://codereview.chromium.org/1334763002/diff/200001/content/renderer/bluet...
content/renderer/bluetooth/bluetooth_dispatcher.cc:426:
blink::WebBluetoothGATTCharacteristic* characteristic =
On 2015/10/03 04:03:03, ortuno wrote:
> On 2015/10/01 at 22:05:23, scheib wrote:
> > characteristic may be nullptr due to characteristicObjectRemoved, in which
> case we shouldn't Send a 'Start' right? For the 'Stop' we should.
> 
> That's tricky and seems like a question for Jeffrey: If the object gets
> destroyed before we get to send a Start request should we even send the start
> request?

Can wait for J if you like, but below on Start success and the characteristic
being nullptr we then send a stop right away. Seems pretty likely to me that we
shouldn't send the initial request here if the characteristic is already gone.
If another characteristic object needed it then it would exist and be in queue
to make it start.

https://codereview.chromium.org/1334763002/diff/200001/content/renderer/bluet...
content/renderer/bluetooth/bluetooth_dispatcher.cc:460: characteristic)) {
On 2015/10/03 04:03:03, ortuno wrote:
> On 2015/10/01 at 22:05:23, scheib wrote:
> > May be removing from here with characteristic == nullptr, when it was added
> with a valid pointer. 
> 
> That's why we pass the characteristic and not nullptr in
> characteristicObjectRemoved
> 
> Perhaps simplest solution is to change active_notification_subscriptions_ to
> std::map<std::string, int>
> 
> Would int be the number of references? That seems plausible. We would still
need
> another set to store the WebBluetoothCharacteristics though.

We do? I don't see WebBluetoothCharacteristics accessed from
active_notification_subscriptions_, only stored into it.

https://codereview.chromium.org/1334763002/diff/200001/content/renderer/bluet...
content/renderer/bluetooth/bluetooth_dispatcher.cc:465: // An object is
destroyed 1) during a start request or 2) after a start
On 2015/10/03 04:03:03, ortuno wrote:
> On 2015/10/01 at 22:05:23, scheib wrote:
> > Reform comment more like this for readability:
> > 
> > Possibly arrived here after two characteristic object destruction cases:
> > 1) Characteristic is destroyed with a Start request outstanding: 
> >     Then OnStartNotificationsSuccess will queue a stop with 
> >     callback = nullptr and characteristic = nullptr which will
> >     eventually call processStopNotificationsRequest.
> > 2) Characteristic is destroyed after Start request completes:
> >     The characteristic needs to be removed from
> >     active_notifications_subscriptions so
> >     characteristicObjectRemoved will call
> >     processStopNotification with callback = nullptr only.
> > 
> >     Comment didn't use "characteristicObjectRemoved"
> >     
> > After chat -- this can probably be simplified to much less text. Something
> like:
> > May be processing a request with nullptr callbacks due to
> >   1) Outstanding start request handed in OnStartNotificationsSuccess
> >       after characteristicObjectRemoved.
> >   2) Characteristic destroyed with no pending requests in
> >       characteristicObjectRemoved.
> 
> Done.

I think you can delete the first 7 comment lines from this block.

https://codereview.chromium.org/1334763002/diff/200001/content/renderer/bluet...
content/renderer/bluetooth/bluetooth_dispatcher.cc:654: // Since there could be
other characteristics waiting to subscribe, we queue
On 2015/10/03 04:03:03, ortuno wrote:
> On 2015/10/01 at 22:05:23, scheib wrote:
> > I understand sending a Stop when the object is destroyed. And it seems that
we
> could do it here or in characteristicRemoved. I don't understand "Since there
> could be other characteristics waiting to subscribe". Which others? How is
> Stopping helping them? If no others just trim comment.
> 
> // Frame 1
> char.startNotifications()
> // char gets destroyed
> // Frame 2
> char.startNotifications()
> 
> It's a small optimization that lets us keep the subscription alive by queuing
> the Stop request after the start request from frame 2. By the time the Stop
> request is processed frame 2 will hold the subscription.

Improve comments in code to address the uncertainty. It sound like the comment
wants to explain why a Stop is queued instead of being executed immediately. So,
comment along lines of:

// The object requesting the notification could have been destroyed while
waiting for the subscription. A STOP must be issued as the subscription is no
longer needed. The STOP is added to the end of the queue in case another START
exists in the queue from another characteristic object, which would result in
the subscription continuing.

https://codereview.chromium.org/1334763002/diff/200001/content/renderer/bluet...
File content/renderer/bluetooth/bluetooth_dispatcher.h (right):

https://codereview.chromium.org/1334763002/diff/200001/content/renderer/bluet...
content/renderer/bluetooth/bluetooth_dispatcher.h:115: bool
PendingNotificationRequest(
On 2015/10/03 04:03:04, ortuno wrote:
> On 2015/10/01 at 22:05:23, scheib wrote:
> > PendingNotificationRequest -> HasUnsentNotificationRequest
> 
> Unset doesn't reflect the state of the notification request. What about
> HasNotificationRequestResponsePending?

SG

https://codereview.chromium.org/1334763002/diff/200001/content/renderer/bluet...
content/renderer/bluetooth/bluetooth_dispatcher.h:137: void
processStartNotificationsRequest(int request_id);
On 2015/10/03 04:03:04, ortuno wrote:
> On 2015/10/01 at 22:05:23, scheib wrote:
> > processStartNotificationsRequest -> SendStartNotificationsRequestIfNeeded
> 
> bikeshedding: ResolveOrSendStartNotificationsRequest()?

SG

https://codereview.chromium.org/1334763002/diff/200001/content/shell/browser/...
File content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.h
(right):

https://codereview.chromium.org/1334763002/diff/200001/content/shell/browser/...
content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.h:164:
// |HeartRateAdapter|
On 2015/10/03 04:03:04, ortuno wrote:
> On 2015/10/01 at 22:05:23, scheib wrote:
> > Why not use GlucoseHeartRateAdapter? For a 'normal' functioning device it
> seems it can reuse an existing adapter.
> 
> This boils down to a design decision when refactoring this code. The
Get*Adapter
> methods are the ones that create the functioning device. Get*Device creates
the
> most basic device.

I mean, why not add this implementation to GlucoseHeartRateAdapter? The
additional functionality in GlucoseHeartRateAdapter shouldn't hurt any tests
that use it, and it will reduce the total number of adapters.

https://codereview.chromium.org/1334763002/diff/220001/content/browser/blueto...
File content/browser/bluetooth/bluetooth_dispatcher_host.cc (right):

https://codereview.chromium.org/1334763002/diff/220001/content/browser/blueto...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:736: // Kill the renderer
Pick one place for the single line comment, either before the if or inside it. I
think inside the if is fine. (update all copies).

https://codereview.chromium.org/1334763002/diff/220001/content/renderer/bluet...
File content/renderer/bluetooth/bluetooth_dispatcher.cc (right):

https://codereview.chromium.org/1334763002/diff/220001/content/renderer/bluet...
content/renderer/bluetooth/bluetooth_dispatcher.cc:301: //    characteristic.
ResolverOrSendStartNotificationRequest will make sure
Resolver -> Resolve

https://codereview.chromium.org/1334763002/diff/220001/content/renderer/bluet...
content/renderer/bluetooth/bluetooth_dispatcher.cc:307: //    we null the
characteristic object. startNotificationsSuccess will take
startNotificationsSuccess -> OnStartNotificationsSuccess

https://codereview.chromium.org/1334763002/diff/220001/content/renderer/bluet...
content/renderer/bluetooth/bluetooth_dispatcher.cc:317:
iter.GetCurrentValue()->characteristic = nullptr;
RemoveFromActiveNotificationSubscriptions must also be called for these. It is
not called as a result of OnStartNotificationsSuccess because nullptr is used
there.

https://codereview.chromium.org/1334763002/diff/220001/content/renderer/bluet...
File content/renderer/bluetooth/bluetooth_dispatcher.h (right):

https://codereview.chromium.org/1334763002/diff/220001/content/renderer/bluet...
content/renderer/bluetooth/bluetooth_dispatcher.h:188: std::map<std::string,
std::queue<int>> notification_requests_queues;
suffix with '_'

scheib

https://codereview.chromium.org/1334763002/diff/200001/third_party/WebKit/LayoutTests/bluetooth/notifications.html File third_party/WebKit/LayoutTests/bluetooth/notifications.html (right): https://codereview.chromium.org/1334763002/diff/200001/third_party/WebKit/LayoutTests/bluetooth/notifications.html#newcode126 third_party/WebKit/LayoutTests/bluetooth/notifications.html:126: </script> On 2015/10/03 04:03:04, ortuno wrote: > On 2015/10/01 ...

5 years, 2 months ago (2015-10-04 01:39:05 UTC) #24

ortuno

Ready for review again. https://codereview.chromium.org/1334763002/diff/200001/content/renderer/bluetooth/bluetooth_dispatcher.cc File content/renderer/bluetooth/bluetooth_dispatcher.cc (right): https://codereview.chromium.org/1334763002/diff/200001/content/renderer/bluetooth/bluetooth_dispatcher.cc#newcode315 content/renderer/bluetooth/bluetooth_dispatcher.cc:315: // 2. The subscription will ...

5 years, 2 months ago (2015-10-06 02:38:43 UTC) #27

Ready for review again.

https://codereview.chromium.org/1334763002/diff/200001/content/renderer/bluet...
File content/renderer/bluetooth/bluetooth_dispatcher.cc (right):

https://codereview.chromium.org/1334763002/diff/200001/content/renderer/bluet...
content/renderer/bluetooth/bluetooth_dispatcher.cc:315: //   2. The subscription
will become inactive.
On 2015/10/04 01:35:58, scheib wrote:
> On 2015/10/03 04:03:03, ortuno wrote:
> > On 2015/10/01 at 22:05:23, scheib wrote:
> > > Case 2 is only if another frame holds the subscription and releases, and
can
> > only happen if !HasActiveNotificationSubscription, because there are no
> pending
> > notification requests and thus no action from this frame. I think just
update
> > comments. Below I think #2 would go with the first if.
> > 
> > Not sure I follow: Case 2 happens when this is the last frame holding the
> > notification.
> 
> Add detail to the comments somehow explaining case 2. Why will the
subscription
> become inactive? There's no pending request for it to become inactive. Oh, is
it
> because this object is the last object keeping it active and we must take
action
> to release it?

Done.

https://codereview.chromium.org/1334763002/diff/200001/content/renderer/bluet...
content/renderer/bluetooth/bluetooth_dispatcher.cc:325:
characteristic_instance_id.utf8(), characteristic,
On 2015/10/04 01:35:58, scheib wrote:
> On 2015/10/03 04:03:03, ortuno wrote:
> > On 2015/10/01 at 22:05:23, scheib wrote:
> > > characteristic should be nullptr
> > 
> > We still need to remove the characteristic from the set of active
> notifications.
> 
> OK, yeah, this is complicated enough that's not obvious. Perhaps update that
> comment above adding "is released for this characteristic object" to help hint
> further? :(

Done.

https://codereview.chromium.org/1334763002/diff/200001/content/renderer/bluet...
content/renderer/bluetooth/bluetooth_dispatcher.cc:426:
blink::WebBluetoothGATTCharacteristic* characteristic =
On 2015/10/04 01:35:58, scheib wrote:
> On 2015/10/03 04:03:03, ortuno wrote:
> > On 2015/10/01 at 22:05:23, scheib wrote:
> > > characteristic may be nullptr due to characteristicObjectRemoved, in which
> > case we shouldn't Send a 'Start' right? For the 'Stop' we should.
> > 
> > That's tricky and seems like a question for Jeffrey: If the object gets
> > destroyed before we get to send a Start request should we even send the
start
> > request?
> 
> Can wait for J if you like, but below on Start success and the characteristic
> being nullptr we then send a stop right away. Seems pretty likely to me that
we
> shouldn't send the initial request here if the characteristic is already gone.
> If another characteristic object needed it then it would exist and be in queue
> to make it start.

Chatted offline. We will send the request for now and open an issue on the spec.
https://github.com/WebBluetoothCG/web-bluetooth/issues/170

https://codereview.chromium.org/1334763002/diff/200001/content/renderer/bluet...
content/renderer/bluetooth/bluetooth_dispatcher.cc:460: characteristic)) {
On 2015/10/04 01:35:58, scheib wrote:
> On 2015/10/03 04:03:03, ortuno wrote:
> > On 2015/10/01 at 22:05:23, scheib wrote:
> > > May be removing from here with characteristic == nullptr, when it was
added
> > with a valid pointer. 
> > 
> > That's why we pass the characteristic and not nullptr in
> > characteristicObjectRemoved
> > 
> > Perhaps simplest solution is to change active_notification_subscriptions_ to
> > std::map<std::string, int>
> > 
> > Would int be the number of references? That seems plausible. We would still
> need
> > another set to store the WebBluetoothCharacteristics though.
> 
> We do? I don't see WebBluetoothCharacteristics accessed from
> active_notification_subscriptions_, only stored into it.

It's not used yet. WebBluetoothCharacteristics is how we are going dispatch the
events to blink.

https://codereview.chromium.org/1334763002/diff/200001/content/renderer/bluet...
content/renderer/bluetooth/bluetooth_dispatcher.cc:465: // An object is
destroyed 1) during a start request or 2) after a start
On 2015/10/04 01:35:58, scheib wrote:
> On 2015/10/03 04:03:03, ortuno wrote:
> > On 2015/10/01 at 22:05:23, scheib wrote:
> > > Reform comment more like this for readability:
> > > 
> > > Possibly arrived here after two characteristic object destruction cases:
> > > 1) Characteristic is destroyed with a Start request outstanding: 
> > >     Then OnStartNotificationsSuccess will queue a stop with 
> > >     callback = nullptr and characteristic = nullptr which will
> > >     eventually call processStopNotificationsRequest.
> > > 2) Characteristic is destroyed after Start request completes:
> > >     The characteristic needs to be removed from
> > >     active_notifications_subscriptions so
> > >     characteristicObjectRemoved will call
> > >     processStopNotification with callback = nullptr only.
> > > 
> > >     Comment didn't use "characteristicObjectRemoved"
> > >     
> > > After chat -- this can probably be simplified to much less text. Something
> > like:
> > > May be processing a request with nullptr callbacks due to
> > >   1) Outstanding start request handed in OnStartNotificationsSuccess
> > >       after characteristicObjectRemoved.
> > >   2) Characteristic destroyed with no pending requests in
> > >       characteristicObjectRemoved.
> > 
> > Done.
> 
> I think you can delete the first 7 comment lines from this block.

Done.

https://codereview.chromium.org/1334763002/diff/200001/content/renderer/bluet...
content/renderer/bluetooth/bluetooth_dispatcher.cc:654: // Since there could be
other characteristics waiting to subscribe, we queue
On 2015/10/04 01:35:58, scheib wrote:
> On 2015/10/03 04:03:03, ortuno wrote:
> > On 2015/10/01 at 22:05:23, scheib wrote:
> > > I understand sending a Stop when the object is destroyed. And it seems
that
> we
> > could do it here or in characteristicRemoved. I don't understand "Since
there
> > could be other characteristics waiting to subscribe". Which others? How is
> > Stopping helping them? If no others just trim comment.
> > 
> > // Frame 1
> > char.startNotifications()
> > // char gets destroyed
> > // Frame 2
> > char.startNotifications()
> > 
> > It's a small optimization that lets us keep the subscription alive by
queuing
> > the Stop request after the start request from frame 2. By the time the Stop
> > request is processed frame 2 will hold the subscription.
> 
> Improve comments in code to address the uncertainty. It sound like the comment
> wants to explain why a Stop is queued instead of being executed immediately.
So,
> comment along lines of:
> 
> // The object requesting the notification could have been destroyed while
> waiting for the subscription. A STOP must be issued as the subscription is no
> longer needed. The STOP is added to the end of the queue in case another START
> exists in the queue from another characteristic object, which would result in
> the subscription continuing.

Done.

https://codereview.chromium.org/1334763002/diff/200001/content/shell/browser/...
File content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.h
(right):

https://codereview.chromium.org/1334763002/diff/200001/content/shell/browser/...
content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.h:164:
// |HeartRateAdapter|
On 2015/10/04 01:35:58, scheib wrote:
> On 2015/10/03 04:03:04, ortuno wrote:
> > On 2015/10/01 at 22:05:23, scheib wrote:
> > > Why not use GlucoseHeartRateAdapter? For a 'normal' functioning device it
> > seems it can reuse an existing adapter.
> > 
> > This boils down to a design decision when refactoring this code. The
> Get*Adapter
> > methods are the ones that create the functioning device. Get*Device creates
> the
> > most basic device.
> 
> I mean, why not add this implementation to GlucoseHeartRateAdapter? The
> additional functionality in GlucoseHeartRateAdapter shouldn't hurt any tests
> that use it, and it will reduce the total number of adapters.

Added a note to the bug http://crbug.com/529975. I didn't do it on this CL since
I will need to change other tests.

https://codereview.chromium.org/1334763002/diff/200001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/bluetooth/notifications.html (right):

https://codereview.chromium.org/1334763002/diff/200001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/bluetooth/notifications.html:126: </script>
On 2015/10/04 01:39:05, scheib wrote:
> On 2015/10/03 04:03:04, ortuno wrote:
> > On 2015/10/01 at 22:05:23, scheib wrote:
> > > No tests here for object deletion. Can it be accomplished in LayoutTests?
> How
> > are other ActiveDOMObjects tested?
> > 
> > I checked battery, geolocation, midi, web sockets, indexeddb, none have
tests.
> > 
> > Theoretically the characteristics should get deleted as soon as they are out
> of
> > scope but in practice I've seen them get deleted during the last tests.
> 
> I'd feel better if we have an issue for this and ask on blink-dev if it's
> reasonable to try to have a hook for forcing this. Perhaps we just need to
force
> a garbage collection? There's a lot of logic we have to handle this that we're
> not stressing in tests.

Done. Added GC tests.

scheib

LGTM with an issue to track adding more characteristic deleted tests. https://codereview.chromium.org/1334763002/diff/200001/third_party/WebKit/LayoutTests/bluetooth/notifications.html File third_party/WebKit/LayoutTests/bluetooth/notifications.html (right): ...

5 years, 2 months ago (2015-10-06 05:35:24 UTC) #28

ortuno

https://codereview.chromium.org/1334763002/diff/200001/third_party/WebKit/LayoutTests/bluetooth/notifications.html File third_party/WebKit/LayoutTests/bluetooth/notifications.html (right): https://codereview.chromium.org/1334763002/diff/200001/third_party/WebKit/LayoutTests/bluetooth/notifications.html#newcode126 third_party/WebKit/LayoutTests/bluetooth/notifications.html:126: </script> On 2015/10/06 05:35:24, scheib wrote: > On 2015/10/06 ...

5 years, 2 months ago (2015-10-06 17:14:43 UTC) #29

ortuno

ortuno@chromium.org changed reviewers: + dpranke@chromium.org, isherman@chromium.org, nick@chromium.org, palmer@chromium.org

5 years, 2 months ago (2015-10-06 17:57:39 UTC) #30

ortuno

palmer: Please review changes in content/common/bluetooth/bluetooth_messages.h nick: Please review changes in content/browser/bad_message.h isherman: Please review ...

5 years, 2 months ago (2015-10-06 17:57:39 UTC) #31

ncarter (slow)

On 2015/10/06 17:57:39, ortuno wrote: > palmer: Please review changes in content/common/bluetooth/bluetooth_messages.h > > nick: ...

5 years, 2 months ago (2015-10-06 18:56:19 UTC) #32

ortuno

ortuno@chromium.org changed reviewers: + haraken@chromium.org

5 years, 2 months ago (2015-10-06 20:58:09 UTC) #33

haraken

https://codereview.chromium.org/1334763002/diff/320001/third_party/WebKit/Source/modules/bluetooth/BluetoothGATTCharacteristic.cpp File third_party/WebKit/Source/modules/bluetooth/BluetoothGATTCharacteristic.cpp (right): https://codereview.chromium.org/1334763002/diff/320001/third_party/WebKit/Source/modules/bluetooth/BluetoothGATTCharacteristic.cpp#newcode25 third_party/WebKit/Source/modules/bluetooth/BluetoothGATTCharacteristic.cpp:25: suspendIfNeeded(); Nit: Can we move suspendIfNeeded() into the take ...

5 years, 2 months ago (2015-10-07 01:22:33 UTC) #36

ortuno

https://codereview.chromium.org/1334763002/diff/320001/third_party/WebKit/Source/modules/bluetooth/BluetoothGATTCharacteristic.cpp File third_party/WebKit/Source/modules/bluetooth/BluetoothGATTCharacteristic.cpp (right): https://codereview.chromium.org/1334763002/diff/320001/third_party/WebKit/Source/modules/bluetooth/BluetoothGATTCharacteristic.cpp#newcode25 third_party/WebKit/Source/modules/bluetooth/BluetoothGATTCharacteristic.cpp:25: suspendIfNeeded(); On 2015/10/07 at 01:22:33, haraken wrote: > Nit: ...

5 years, 2 months ago (2015-10-07 17:03:55 UTC) #37

https://codereview.chromium.org/1334763002/diff/320001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/bluetooth/BluetoothGATTCharacteristic.cpp
(right):

https://codereview.chromium.org/1334763002/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/bluetooth/BluetoothGATTCharacteristic.cpp:25:
suspendIfNeeded();
On 2015/10/07 at 01:22:33, haraken wrote:
> Nit: Can we move suspendIfNeeded() into the take method? We normally call
suspendIfNeeded() after finishing constructing the object.

Done. Is there any reason for that? I see a couple classes that do it in the
constructor.

https://codereview.chromium.org/1334763002/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/bluetooth/BluetoothGATTCharacteristic.cpp:48:
// again.
On 2015/10/07 at 01:22:33, haraken wrote:
> Is this comment correct? If the parent document is detached,
ActiveDOMObject::stop() should have been called.

Moved things around a bit to make our intention clearer.

https://codereview.chromium.org/1334763002/diff/320001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/bluetooth/BluetoothGATTCharacteristic.h
(right):

https://codereview.chromium.org/1334763002/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/bluetooth/BluetoothGATTCharacteristic.h:39:
WILL_BE_USING_GARBAGE_COLLECTED_MIXIN(BluetoothGATTCharacteristic);
On 2015/10/07 at 01:22:33, haraken wrote:
> You don't need this. (WebBluetoothGATTCharacteristic is not a GC mixin.)

I tried that but the compiler complains when Oilpan is on:

../../third_party/WebKit/Source/modules/bluetooth/BluetoothGATTCharacteristic.h:33:7:
error: abstract class is marked 'final' [-Werror,-Wabstract-final-class]
class BluetoothGATTCharacteristic final
      ^
../../third_party/WebKit/Source/platform/heap/GarbageCollected.h:125:18: note:
unimplemented pure virtual method 'adjustAndMark' in
'BluetoothGATTCharacteristic'
    virtual void adjustAndMark(Visitor*) const = 0;
                 ^
../../third_party/WebKit/Source/platform/heap/GarbageCollected.h:127:18: note:
unimplemented pure virtual method 'adjustAndMark' in
'BluetoothGATTCharacteristic'
    virtual void adjustAndMark(InlinedGlobalMarkingVisitor) const = 0;
                 ^
../../third_party/WebKit/Source/platform/heap/GarbageCollected.h:129:18: note:
unimplemented pure virtual method 'isHeapObjectAlive' in
'BluetoothGATTCharacteristic'
    virtual bool isHeapObjectAlive() const = 0;
                 ^
../../third_party/WebKit/Source/modules/bluetooth/BluetoothGATTCharacteristic.cpp:33:55:
error: allocating an object of abstract class type
'blink::BluetoothGATTCharacteristic'
    BluetoothGATTCharacteristic* characteristic = new
BluetoothGATTCharacteristic(resolver->executionContext(), webCharacteristic);

https://codereview.chromium.org/1334763002/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/bluetooth/BluetoothGATTCharacteristic.h:52: //
Called when the object is getting garbage collected.
On 2015/10/07 at 01:22:33, haraken wrote:
> Called _before_

Done.

https://codereview.chromium.org/1334763002/diff/320001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/bluetooth/BluetoothSupplement.h (right):

https://codereview.chromium.org/1334763002/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/bluetooth/BluetoothSupplement.h:31: static
WebBluetooth* from(LocalDOMWindow*);
On 2015/10/07 at 01:22:33, haraken wrote:
> This looks wrong. BluetoothSupplement is a supplement of LocalFrame, so it
must be extracted from LocalFrame (shouldn't be extracted from ExecutionContext,
LocalDOMWindow etc, which has a different lifetime than LocalFrame).

In all three cases we end up getting the supplement from the frame:

scriptState->domWindow()->frame();
executionContext->executingWindow()->frame();
window->frame();

Should we do it differently? Is there another class we should implement to get
the local frame?

haraken

https://codereview.chromium.org/1334763002/diff/320001/third_party/WebKit/Source/modules/bluetooth/BluetoothGATTCharacteristic.h File third_party/WebKit/Source/modules/bluetooth/BluetoothGATTCharacteristic.h (right): https://codereview.chromium.org/1334763002/diff/320001/third_party/WebKit/Source/modules/bluetooth/BluetoothGATTCharacteristic.h#newcode39 third_party/WebKit/Source/modules/bluetooth/BluetoothGATTCharacteristic.h:39: WILL_BE_USING_GARBAGE_COLLECTED_MIXIN(BluetoothGATTCharacteristic); On 2015/10/07 17:03:55, ortuno wrote: > On 2015/10/07 ...

5 years, 2 months ago (2015-10-08 00:51:52 UTC) #38

https://codereview.chromium.org/1334763002/diff/320001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/bluetooth/BluetoothGATTCharacteristic.h
(right):

https://codereview.chromium.org/1334763002/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/bluetooth/BluetoothGATTCharacteristic.h:39:
WILL_BE_USING_GARBAGE_COLLECTED_MIXIN(BluetoothGATTCharacteristic);
On 2015/10/07 17:03:55, ortuno wrote:
> On 2015/10/07 at 01:22:33, haraken wrote:
> > You don't need this. (WebBluetoothGATTCharacteristic is not a GC mixin.)
> 
> I tried that but the compiler complains when Oilpan is on:
> 
>
../../third_party/WebKit/Source/modules/bluetooth/BluetoothGATTCharacteristic.h:33:7:
> error: abstract class is marked 'final' [-Werror,-Wabstract-final-class]
> class BluetoothGATTCharacteristic final
>       ^
> ../../third_party/WebKit/Source/platform/heap/GarbageCollected.h:125:18: note:
> unimplemented pure virtual method 'adjustAndMark' in
> 'BluetoothGATTCharacteristic'
>     virtual void adjustAndMark(Visitor*) const = 0;
>                  ^
> ../../third_party/WebKit/Source/platform/heap/GarbageCollected.h:127:18: note:
> unimplemented pure virtual method 'adjustAndMark' in
> 'BluetoothGATTCharacteristic'
>     virtual void adjustAndMark(InlinedGlobalMarkingVisitor) const = 0;
>                  ^
> ../../third_party/WebKit/Source/platform/heap/GarbageCollected.h:129:18: note:
> unimplemented pure virtual method 'isHeapObjectAlive' in
> 'BluetoothGATTCharacteristic'
>     virtual bool isHeapObjectAlive() const = 0;
>                  ^
>
../../third_party/WebKit/Source/modules/bluetooth/BluetoothGATTCharacteristic.cpp:33:55:
> error: allocating an object of abstract class type
> 'blink::BluetoothGATTCharacteristic'
>     BluetoothGATTCharacteristic* characteristic = new
> BluetoothGATTCharacteristic(resolver->executionContext(), webCharacteristic);

ah, sorry; you're right.

 WILL_BE_USING_GARBAGE_COLLECTED_MIXIN is needed because ActiveDOMObject is a
WillBeGCMixin.

https://codereview.chromium.org/1334763002/diff/340001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/bluetooth/BluetoothSupplement.cpp
(right):

https://codereview.chromium.org/1334763002/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/bluetooth/BluetoothSupplement.cpp:46: return
Platform::current()->bluetooth();

We sometimes return a bluetooth of the frame and sometimes a bluetooth of the
platform, which seems undeterministic. Why is it ok?

In other words, what happens if:

- we return nullptr if the frame is already gone; or
- we unconditionally return Platform::current()->bluetooth()

?

ortuno

https://codereview.chromium.org/1334763002/diff/340001/third_party/WebKit/Source/modules/bluetooth/BluetoothSupplement.cpp File third_party/WebKit/Source/modules/bluetooth/BluetoothSupplement.cpp (right): https://codereview.chromium.org/1334763002/diff/340001/third_party/WebKit/Source/modules/bluetooth/BluetoothSupplement.cpp#newcode46 third_party/WebKit/Source/modules/bluetooth/BluetoothSupplement.cpp:46: return Platform::current()->bluetooth(); On 2015/10/08 at 00:51:52, haraken wrote: > ...

5 years, 2 months ago (2015-10-08 16:07:45 UTC) #39

haraken

On 2015/10/08 16:07:45, ortuno wrote: > https://codereview.chromium.org/1334763002/diff/340001/third_party/WebKit/Source/modules/bluetooth/BluetoothSupplement.cpp > File third_party/WebKit/Source/modules/bluetooth/BluetoothSupplement.cpp > (right): > > https://codereview.chromium.org/1334763002/diff/340001/third_party/WebKit/Source/modules/bluetooth/BluetoothSupplement.cpp#newcode46 ...

5 years, 2 months ago (2015-10-09 00:09:04 UTC) #40

On 2015/10/08 16:07:45, ortuno wrote:
>
https://codereview.chromium.org/1334763002/diff/340001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/modules/bluetooth/BluetoothSupplement.cpp
> (right):
> 
>
https://codereview.chromium.org/1334763002/diff/340001/third_party/WebKit/Sou...
> third_party/WebKit/Source/modules/bluetooth/BluetoothSupplement.cpp:46: return
> Platform::current()->bluetooth();
> On 2015/10/08 at 00:51:52, haraken wrote:
> > We sometimes return a bluetooth of the frame and sometimes a bluetooth of
the
> platform, which seems undeterministic. Why is it ok?
> > 
> > In other words, what happens if:
> > 
> > - we return nullptr if the frame is already gone; or
> 
> If the frame is gone then window->frame() would be false and we would return
> Platform::current()->bluetooth()
> 
> > - we unconditionally return Platform::current()->bluetooth()
> > 
> 
> There is actually no implementation of bluetooth() so that would return
nullptr.
> 
> > ?
> 
> 
> We moved web_bluetooth from child/ to renderer/ here:
> 
> https://codereview.chromium.org/1228283003
> 
> I think the intention was the following:
> 
> If there is a frame available then get the supplement from there. If frame is
> not available then we can assume we are in a worker and get the supplement
from
> platform. Since we don't support workers yet we return nullptr in this case.
Do
> you think this can be done in a better way?

I think we need to distinguish the following three scenarios:

1) Main thread. Frame is available.
2) Main thread. Frame is not available.
3) Worker thread. Frame is not available.

Currently we're mixing 2) and 3) behind the fact that Platform::current()
returns nullptr at the moment.

What we normally do for 2) is:

Bar* Supplement::foo() {
  if (!executionContext() || !executionContext()->frame())
    return nullptr;
  return Bar::from(executionContext()->frame());
}

That way we can avoid introducing WebBluetooth* from(LocalDOMWindow*),
WebBluetooth* from(ExecutionContext*) etc. Given that BluetoothSupplement is a
supplement of LocalFrame, it is wrong to introduce other than from(LocalFrame*).

(Sorry about the review delay...)

ortuno

The CQ bit was checked by ortuno@chromium.org to run a CQ dry run

5 years, 2 months ago (2015-10-09 17:52:21 UTC) #42

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1334763002/360001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1334763002/360001

5 years, 2 months ago (2015-10-09 17:52:36 UTC) #43

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

5 years, 2 months ago (2015-10-09 18:10:40 UTC) #44

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/android_arm64_dbg_recipe/builds/129950) android_chromium_gn_compile_dbg on ...

5 years, 2 months ago (2015-10-09 18:10:41 UTC) #45

haraken

https://codereview.chromium.org/1334763002/diff/380001/third_party/WebKit/Source/modules/bluetooth/BluetoothGATTCharacteristic.cpp File third_party/WebKit/Source/modules/bluetooth/BluetoothGATTCharacteristic.cpp (right): https://codereview.chromium.org/1334763002/diff/380001/third_party/WebKit/Source/modules/bluetooth/BluetoothGATTCharacteristic.cpp#newcode22 third_party/WebKit/Source/modules/bluetooth/BluetoothGATTCharacteristic.cpp:22: , m_webCharacteristic(webCharacteristic) Shall we initialize m_stopped in the constructor? ...

5 years, 2 months ago (2015-10-10 14:19:35 UTC) #46

ortuno

https://codereview.chromium.org/1334763002/diff/380001/third_party/WebKit/Source/modules/bluetooth/BluetoothGATTCharacteristic.cpp File third_party/WebKit/Source/modules/bluetooth/BluetoothGATTCharacteristic.cpp (right): https://codereview.chromium.org/1334763002/diff/380001/third_party/WebKit/Source/modules/bluetooth/BluetoothGATTCharacteristic.cpp#newcode22 third_party/WebKit/Source/modules/bluetooth/BluetoothGATTCharacteristic.cpp:22: , m_webCharacteristic(webCharacteristic) On 2015/10/10 at 14:19:35, haraken wrote: > ...

5 years, 2 months ago (2015-10-12 17:46:07 UTC) #47

haraken

On 2015/10/12 23:22:06, haraken wrote: > Thanks for being persistent! LGMT. I meant LGTM :)

5 years, 2 months ago (2015-10-12 23:22:21 UTC) #49

ortuno

dpranke: ping for third_party/WebKit/public/blink_headers.gypi

5 years, 2 months ago (2015-10-13 18:28:54 UTC) #50

ortuno

palmer: ping for content/common/bluetooth/bluetooth_messages.h

5 years, 2 months ago (2015-10-13 18:59:13 UTC) #52

ortuno

The CQ bit was checked by ortuno@chromium.org to run a CQ dry run

5 years, 2 months ago (2015-10-13 19:27:30 UTC) #54

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1334763002/420001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1334763002/420001

5 years, 2 months ago (2015-10-13 19:30:28 UTC) #55

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

5 years, 2 months ago (2015-10-13 20:42:11 UTC) #56

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_ng/builds/120020)

5 years, 2 months ago (2015-10-13 20:42:13 UTC) #57

ortuno

The patchset sent to the CQ was uploaded after l-g-t-m from scheib@chromium.org, nick@chromium.org, isherman@chromium.org, dpranke@chromium.org, ...

5 years, 2 months ago (2015-10-14 18:25:11 UTC) #59

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1334763002/440001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1334763002/440001

5 years, 2 months ago (2015-10-14 18:26:08 UTC) #60

commit-bot: I haz the power

5 years, 2 months ago (2015-10-14 19:12:45 UTC) #62

Message was sent while issue was closed.

Patchset 20 (id:??) landed as
https://crrev.com/73a7a6aa56f705aabfa9f2bf371f34a09d79d62e
Cr-Commit-Position: refs/heads/master@{#354080}

Issue 1334763002: bluetooth: Subscribe to notifications (Closed)

Description

Patch Set 1 : #

Patch Set 2 : Keep track of objects on the renderer side #

Patch Set 3 : Address jyasskin's comments #

Patch Set 4 : Queue startNotifications calls #

Patch Set 5 : #

Patch Set 6 : Pre merge #

Patch Set 7 : Redesign #

Patch Set 8 : Clean up #

Patch Set 9 : Add a comment about queueing system #

Patch Set 10 : Address jyasskin's comments #

Patch Set 11 : Address scheib's comments and add dispose #

Patch Set 12 : Final clean up #

Patch Set 13 : Stop request pending test #

Patch Set 14 : Wait for stop request to finish #

Patch Set 15 : Address haraken's comments #

Patch Set 16 : Use the new BluetoothSupplement::from(LocalFrame*) #

Patch Set 17 : Include map #

Patch Set 18 : Using execution context directly to get the frame. #

Patch Set 19 : New line #

Patch Set 20 : Fix global interface test #

Messages