Implement BluetoothGattNotifySession::Stop on Android, 2nd attempt

Implement BluetoothGattNotifySession::Stop on Android, 2nd attempt

This change enables the stopNotifications function for remote GATT
characteristics on Android. In order to do so, two distinct refactorings have been done;

1) The session classes have been simplified so that platform specific versions are no longer required. All code that previously used BluetoothGattNotifySession[Mac|Win|BlueZ] now simply use the base BluetoothGattNotifySession class.

2) BluetoothRemoteGattCharacteristic::StartNotifySession and StopNotifySession have been rewritten so that all the code for keeping track of the different session objects and the notification state sits in the base class. Inheriting classes should no longer override these two functions, but instead override the much simpler SubscribeToNotifications and UnsubscribeFromNotifications.

BUG=584370
Committed: https://crrev.com/d41af3adfd8500c81c827ab03ed2736909cf1ee8
Committed: https://crrev.com/ad8462b6930b45ec778dc9e3fcedfc74680f964d
Cr-Original-Commit-Position: refs/heads/master@{#412498}
Cr-Commit-Position: refs/heads/master@{#413417}

[tommyt, 2016-06-10 12:57:40]

Description was changed from

==========
Implement BluetoothGattNotifySession::Stop on Android

This change enables the stopNotifications function for remote GATT
characteristics on Android.

BUG=584370
==========

to

==========
Implement BluetoothGattNotifySession::Stop on Android

This change enables the stopNotifications function for remote GATT
characteristics on Android.

BUG=584370
==========

[tommyt, 2016-06-10 12:57:41]

tommyt@opera.com changed reviewers:
+ scheib@chromium.org

[tommyt, 2016-06-10 13:00:51]

Here's some code for stopping notification sessions on Android. Please take a
look when you have time.

[ortuno, 2016-06-10 17:06:14]

fwiw: I wrote a document a while ago with a bunch of edge cases for notification
subscribing. I don't think all of it applies to this but some edge cases might
be relevant:

https://docs.google.com/document/d/1WJgglgqvZnrt37iKf3YR4ULL6DkzzT64VGBL4ijTj...

[tommyt, 2016-06-15 09:11:53]

On 2016/06/10 17:06:14, ortuno wrote:
> fwiw: I wrote a document a while ago with a bunch of edge cases for
notification
> subscribing. I don't think all of it applies to this but some edge cases might
> be relevant:
> 
>
https://docs.google.com/document/d/1WJgglgqvZnrt37iKf3YR4ULL6DkzzT64VGBL4ijTj...

Hi Gio!

I've had a look at the document you linked, and all of the implementation
suggestions take place in the DispatcherHost, which I don't think exists any
more. Would the WebBluetoothServiceImpl be the right place for handling these
edge cases?

Ideally, all of these edge cases should be expressed as unit tests, but I don't
think the regular bluetooth unittests in device/bluetooth are able to cover the
WebBluetoothServiceImpl class. Where would you suggest putting these tests?
Would a web_bluetooth_service_impl_unittests.cc be a good idea?

[ortuno, 2016-06-15 15:43:39]

On 2016/06/15 at 09:11:53, tommyt wrote:
> On 2016/06/10 17:06:14, ortuno wrote:
> > fwiw: I wrote a document a while ago with a bunch of edge cases for
notification
> > subscribing. I don't think all of it applies to this but some edge cases
might
> > be relevant:
> > 
> >
https://docs.google.com/document/d/1WJgglgqvZnrt37iKf3YR4ULL6DkzzT64VGBL4ijTj...
> 
> Hi Gio!
> 
> I've had a look at the document you linked, and all of the implementation
suggestions take place in the DispatcherHost, which I don't think exists any
more. Would the WebBluetoothServiceImpl be the right place for handling these
edge cases?
> 
> Ideally, all of these edge cases should be expressed as unit tests, but I
don't think the regular bluetooth unittests in device/bluetooth are able to
cover the WebBluetoothServiceImpl class. Where would you suggest putting these
tests? Would a web_bluetooth_service_impl_unittests.cc be a good idea?

Sorry for not being clear enough. We've been talking about queuing calls to
char.startNotifications and char.stopNotifications. The document listed the edge
cases that our renderer side implementation had to handle. After some
discussions we thought it would be better if the queuing was done at the
device/bluetooth layer. The tests in the document could server as a starting
point for the queuing tests in the device/bluetooth layer. I revised the doc to
be in terms of device/bluetooth and removed any tests that don't apply to this
layer:
https://docs.google.com/document/d/1cjxpD5aH54QXy6J0l-0XVQOlNEq3QfKL6cQJU2KnR...

That said, I don't think we need the queuing implementation just yet. But we
should write tests for all the tests cases in the document and make sure we
handle them in an expected way.

[tommyt, 2016-06-20 12:59:05]

On 2016/06/15 15:43:39, ortuno wrote:
> On 2016/06/15 at 09:11:53, tommyt wrote:
> > On 2016/06/10 17:06:14, ortuno wrote:
> > > fwiw: I wrote a document a while ago with a bunch of edge cases for
> notification
> > > subscribing. I don't think all of it applies to this but some edge cases
> might
> > > be relevant:
> > > 
> > >
>
https://docs.google.com/document/d/1WJgglgqvZnrt37iKf3YR4ULL6DkzzT64VGBL4ijTj...
> > 
> > Hi Gio!
> > 
> > I've had a look at the document you linked, and all of the implementation
> suggestions take place in the DispatcherHost, which I don't think exists any
> more. Would the WebBluetoothServiceImpl be the right place for handling these
> edge cases?
> > 
> > Ideally, all of these edge cases should be expressed as unit tests, but I
> don't think the regular bluetooth unittests in device/bluetooth are able to
> cover the WebBluetoothServiceImpl class. Where would you suggest putting these
> tests? Would a web_bluetooth_service_impl_unittests.cc be a good idea?
> 
> Sorry for not being clear enough. We've been talking about queuing calls to
> char.startNotifications and char.stopNotifications. The document listed the
edge
> cases that our renderer side implementation had to handle. After some
> discussions we thought it would be better if the queuing was done at the
> device/bluetooth layer. The tests in the document could server as a starting
> point for the queuing tests in the device/bluetooth layer. I revised the doc
to
> be in terms of device/bluetooth and removed any tests that don't apply to this
> layer:
>
https://docs.google.com/document/d/1cjxpD5aH54QXy6J0l-0XVQOlNEq3QfKL6cQJU2KnR...
> 
> That said, I don't think we need the queuing implementation just yet. But we
> should write tests for all the tests cases in the document and make sure we
> handle them in an expected way.

Patchset #7 adds unittests to cover all of the edge cases in the document.
Here's the map of which unittest cover which edge case:

Object gets destroyed before callback is run
    StartNotifySession_AfterDeleted

Object gets destroyed after callback is run
    StartNotifySession_BeforeDeleted

Concurrent StartNotifySessions
    StartNotifySession_Multiple

Concurrent stop requests
    StopNotifySession_Multiple1

Start request before stop request resolves
    StopNotifySession_StopStart

Concurrent subscribe requests
    StartNotifySession_Multiple

Start notification request before stop notification from another frame succeeds
    StopNotifySession_StopStart

Start notification request before stop notifications requests from another frame
succeed
    StopNotifySession_StopStopStart

[scheib, 2016-06-23 20:26:40]

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

[tommyt, 2016-07-20 07:27:02]

The CQ bit was checked by tommyt@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2016-07-20 07:27:26]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-20 07:36:56]

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

[commit-bot: I haz the power, 2016-07-20 07:36:57]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[tommyt, 2016-07-21 09:36:34]

The CQ bit was checked by tommyt@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2016-07-21 09:36:56]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-21 09:43:23]

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

[commit-bot: I haz the power, 2016-07-21 09:43:24]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[tommyt, 2016-07-21 12:18:22]

The CQ bit was checked by tommyt@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2016-07-21 12:18:46]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-21 12:25:25]

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

[commit-bot: I haz the power, 2016-07-21 12:25:26]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[tommyt, 2016-07-22 09:06:29]

The CQ bit was checked by tommyt@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2016-07-22 09:06:44]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-22 09:17:33]

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

[commit-bot: I haz the power, 2016-07-22 09:17:34]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[tommyt, 2016-07-23 06:44:31]

The CQ bit was checked by tommyt@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2016-07-23 06:44:35]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-23 06:57:45]

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

[commit-bot: I haz the power, 2016-07-23 06:57:46]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[tommyt, 2016-07-25 06:40:35]

The CQ bit was checked by tommyt@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2016-07-25 06:40:46]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-25 06:54:20]

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

[commit-bot: I haz the power, 2016-07-25 06:54:21]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[tommyt, 2016-07-25 06:54:28]

The CQ bit was checked by tommyt@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2016-07-25 06:54:41]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-25 07:14:20]

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

[commit-bot: I haz the power, 2016-07-25 07:14:21]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[tommyt, 2016-07-25 11:04:09]

The CQ bit was checked by tommyt@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2016-07-25 11:04:19]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-25 11:39:58]

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

[commit-bot: I haz the power, 2016-07-25 11:39:59]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[tommyt, 2016-07-26 08:38:51]

The CQ bit was checked by tommyt@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2016-07-26 08:39:14]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-26 08:49:32]

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

[commit-bot: I haz the power, 2016-07-26 08:49:33]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[tommyt, 2016-07-26 09:37:30]

The CQ bit was checked by tommyt@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2016-07-26 09:37:48]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-26 10:19:40]

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

[commit-bot: I haz the power, 2016-07-26 10:19:41]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[tommyt, 2016-07-28 11:13:06]

The CQ bit was checked by tommyt@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2016-07-28 11:13:17]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-28 11:53:30]

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

[commit-bot: I haz the power, 2016-07-28 11:53:31]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[tommyt, 2016-07-28 12:26:42]

The CQ bit was checked by tommyt@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2016-07-28 12:26:52]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-28 13:56:02]

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

[commit-bot: I haz the power, 2016-07-28 13:56:03]

Dry run: This issue passed the CQ dry run.

[ortuno, 2016-07-28 21:59:30]

https://codereview.chromium.org/2051333004/diff/420001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_gatt_notify_session.cc (right):

https://codereview.chromium.org/2051333004/diff/420001/device/bluetooth/bluet...
device/bluetooth/bluetooth_gatt_notify_session.cc:20: if (characteristic_.get())
{
optional nit: I think the preferred method for checking that a WeakPtr is still
valid is to use its  !=(..., nullptr) operator:

characteristic_ != nullptr;

Alternative you could use IsActive():

if (IsActive()) {
  Stop(...);
}

https://codereview.chromium.org/2051333004/diff/420001/device/bluetooth/bluet...
device/bluetooth/bluetooth_gatt_notify_session.cc:35: return
characteristic_.get() != nullptr && characteristic_->IsNotifying();
When would characteristic_.get() != nullptr and IsNotifying be false?

https://codereview.chromium.org/2051333004/diff/420001/device/bluetooth/bluet...
device/bluetooth/bluetooth_gatt_notify_session.cc:41: if (characteristic) {
optional nit for clarity: characteristic != nullptr

https://codereview.chromium.org/2051333004/diff/420001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_gatt_notify_session.h (right):

https://codereview.chromium.org/2051333004/diff/420001/device/bluetooth/bluet...
device/bluetooth/bluetooth_gatt_notify_session.h:34: // Returns the associated
characteristic.
Explain when this returns nullptr.

https://codereview.chromium.org/2051333004/diff/420001/device/bluetooth/bluet...
device/bluetooth/bluetooth_gatt_notify_session.h:38: virtual bool IsActive();
Please add a comment explaining what it means for this session to be active.

https://codereview.chromium.org/2051333004/diff/420001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_remote_gatt_characteristic.cc (right):

https://codereview.chromium.org/2051333004/diff/420001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic.cc:46: void
BluetoothRemoteGattCharacteristic::StartNotifySession(
This looks very similar to what I first wrote for the notifications queue in the
renderer and also what I started to do for the getPrimaryServices code. I think
we can simplify this a bit by using closures so that we don't have to create to
a base class and two classes that implement it. What do you think of the
following code? It introduces a NotifySessionTask that takes a closure and a
callback to be called when cancelling tasks. The rest of the code is very
similar to what you have already.


NotifySessionTask(Closure task, ErrorCallback error_callback) 
  : task_(task),
    error_callback_(error_callback) {
}

~NotifySessionTask() {}

NotifySessionTask::Run() {
  task.Run();
}

NotifySessionTask::Cancel() {
  error_callback_.Run(GATT_ERROR);
}

StartNotifySessionImpl(callback, error_callback) {
   if (IsNotifying()) {
    base::ThreadTaskRunnerHandle::Get()->PostTask(
        FROM_HERE, base::Bind(&OnSubscribeToNotificationsSuccess,
                              weak_ptr_factory_.GetWeakPtr(),
                              callback, error_callback));
    return;
  }
  SubscribeToNotifications(base::Bind(OnSubscribeToNotificationsSuccess,
                                      weak_ptr_factory_.GetWeakPtr(),
                                      callback),
                           base::Bind(OnSubscribetoNotificationsFailed,
                                      weak_ptr_factory_.GetWeakPtr(),
                                      error_callback));
}

// TODO: Implement StopNotifySessionImpl


OnSubscribeToNotificationsSuccess(NotifySessionCallback callback) {
  pending_notify_tasks_.pop();
  // TODO: make notify session.
  callback.Run(std::move(notify_session));
  
  if (!pending_notify_tasks_.empty()) {
    pending_notify_tasks_.front().Run();
  }
}

// TODO: Implement OnSubscribeToNotificationsSuccess,
OnSubscribeToNotificationsError,
// OnUnsubscribeToNotificationsSuccess and OnUnsubscribeToNotificationsError.

StartNotifySession(NotifySessionCallback callback, ErrorCallback,
error_callback) {
  pending_notify_tasks_.push(MakeUnique<NotifySessionTask>(
    base::Bind(StartNotifySessionImpl, this, callback, error_callback),
error_callback));
  if (pending_notify_tasks_size() == 1) {
    pending_notify_tasks_.front().Run();
  }
}

StopNotifySession(...) {
  pending_notify_tasks_.push(MakeUnique<NotifySessionTask>(
    base::Bind(StopNotifySessionImpl, callback, error_callback),
error_callback));
  if (pending_notify_tasks_size() == 1) {
    pending_notify_tasks_.fron().Run();
  }
}

https://codereview.chromium.org/2051333004/diff/420001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic.cc:82: if (!hasNotify &&
!hasIndicate) {
I think you can move this logic into SubscribeToNotifications. See comment in
SubscribeToNotifications.

https://codereview.chromium.org/2051333004/diff/420001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic.cc:153: if (next->type_ ==
COMMAND_START) {
Do you do this for any other reason that optimizing?

https://codereview.chromium.org/2051333004/diff/420001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_remote_gatt_characteristic.h (right):

https://codereview.chromium.org/2051333004/diff/420001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic.h:80: virtual bool
IsNotifying() const;
hmm this should really be private. I can't think of any use cases for it. There
are two uses in the codebase: One of them is using it completely wrong and the
other one is working around a bug that was fixed... OK to leave public for now
but can you add the following:

// TODO(crbug.com/632476): Make private since there are no uses for this
function.

https://codereview.chromium.org/2051333004/diff/420001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic.h:85: // ownership belongs
to the caller. |error_callback| is called on errors.
I think we should document this more. Please explain:

- How notifications are delivered e.g. CharacteristicValueChanged is called
when...
- How to stop notifications e.g. calling session->Stop or destroying the
session.

Anything else that you think might be relevant for someone that wants to use
this API.

https://codereview.chromium.org/2051333004/diff/420001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic.h:102: virtual void
StopNotifySession(BluetoothGattNotifySession* session,
I don't think we want to expose another way of stopping sessions. Make this
private and have BluetoothNotifySession be a friend of this class.

https://codereview.chromium.org/2051333004/diff/420001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic.h:126: // Writes to the
Client Characteristic Configuration descriptor to enable
Also could you explain the relationship between these two methods and
Start/StopNotifySessions? No need to go into too much detail, just saying that
these two methods are meant to be called by Start/StopNotifySessions might be
enough.

https://codereview.chromium.org/2051333004/diff/420001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic.h:130: // not present
|error_callback| will be run.
We probably want this behavior, failing on no descriptor, to be cross platform.
Do you think we could include this in StartNotifySession? Or maybe implement
this here and have other implementations override SubscribeToNotificationsImpl?

https://codereview.chromium.org/2051333004/diff/420001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_remote_gatt_characteristic_android.cc (right):

https://codereview.chromium.org/2051333004/diff/420001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic_android.cc:242: void
BluetoothRemoteGattCharacteristicAndroid::SubscribeToNotifications(
I would be OK with keeping this as NOTIMPLEMENTED in this CL. And moving each
platform in a different CL.

https://codereview.chromium.org/2051333004/diff/420001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic_android.cc:263: if
(ccc_descriptor.size() != 1u) {
I think we should move this to the cross platform implementation as well. See
comment in bluetooth_remote_gatt_characteristic.h

https://codereview.chromium.org/2051333004/diff/420001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic_android.cc:284:
std::vector<uint8_t> value(2);
optional nit:

std::vector<uint8_t> value = hasNotify ? {1, 0} : {2, 0};

https://codereview.chromium.org/2051333004/diff/420001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_remote_gatt_characteristic_mac.mm (right):

https://codereview.chromium.org/2051333004/diff/420001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic_mac.mm:229:
NOTIMPLEMENTED();
Can you open an issue that mentions that this needs to be implemented on macOS?

https://codereview.chromium.org/2051333004/diff/420001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc (right):

https://codereview.chromium.org/2051333004/diff/420001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc:1172: #if
defined(OS_ANDROID)
How do you feel about splitting the testing in two parts,
Start/StopNotifySession tests and Subscribe/Unsubscribe tests?

For the Start/StopNotifySession tests you can just construct a mock
characteristic with fake subscribe and unsubscribe, that way you don't need to
worry about all the descriptor stuff.

For Subscribe/Unsubscribe you would need to worry about those but then you don't
need to worry about so many tests.

WDYT?

[tommyt, 2016-08-01 11:24:21]

The CQ bit was checked by tommyt@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-01 11:24:36]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tommyt, 2016-08-01 11:39:22]

I have added a new patchset which addresses most of Gio's review comments.

https://codereview.chromium.org/2051333004/diff/420001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_gatt_notify_session.cc (right):

https://codereview.chromium.org/2051333004/diff/420001/device/bluetooth/bluet...
device/bluetooth/bluetooth_gatt_notify_session.cc:20: if (characteristic_.get())
{
On 2016/07/28 21:59:29, ortuno wrote:
> optional nit: I think the preferred method for checking that a WeakPtr is
still
> valid is to use its  !=(..., nullptr) operator:
> 
> characteristic_ != nullptr;
> 
> Alternative you could use IsActive():
> 
> if (IsActive()) {
>   Stop(...);
> }

Done.

https://codereview.chromium.org/2051333004/diff/420001/device/bluetooth/bluet...
device/bluetooth/bluetooth_gatt_notify_session.cc:35: return
characteristic_.get() != nullptr && characteristic_->IsNotifying();
On 2016/07/28 21:59:30, ortuno wrote:
> When would characteristic_.get() != nullptr and IsNotifying be false?

Currently never. However, if we move towards creating session objects instantly
in StartNotifySession, this will be the case between the time when the session
object is created, until the characteristic successfully starts notifying.

https://codereview.chromium.org/2051333004/diff/420001/device/bluetooth/bluet...
device/bluetooth/bluetooth_gatt_notify_session.cc:41: if (characteristic) {
On 2016/07/28 21:59:30, ortuno wrote:
> optional nit for clarity: characteristic != nullptr 

Done.

https://codereview.chromium.org/2051333004/diff/420001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_gatt_notify_session.h (right):

https://codereview.chromium.org/2051333004/diff/420001/device/bluetooth/bluet...
device/bluetooth/bluetooth_gatt_notify_session.h:34: // Returns the associated
characteristic.
On 2016/07/28 21:59:30, ortuno wrote:
> Explain when this returns nullptr.

Done.

https://codereview.chromium.org/2051333004/diff/420001/device/bluetooth/bluet...
device/bluetooth/bluetooth_gatt_notify_session.h:38: virtual bool IsActive();
On 2016/07/28 21:59:30, ortuno wrote:
> Please add a comment explaining what it means for this session to be active.

Done.

https://codereview.chromium.org/2051333004/diff/420001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_remote_gatt_characteristic.cc (right):

https://codereview.chromium.org/2051333004/diff/420001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic.cc:46: void
BluetoothRemoteGattCharacteristic::StartNotifySession(
On 2016/07/28 21:59:30, ortuno wrote:
> This looks very similar to what I first wrote for the notifications queue in
the
> renderer and also what I started to do for the getPrimaryServices code. I
think
> we can simplify this a bit by using closures so that we don't have to create
to
> a base class and two classes that implement it. What do you think of the
> following code? It introduces a NotifySessionTask that takes a closure and a
> callback to be called when cancelling tasks. The rest of the code is very
> similar to what you have already.
> 
> 
> NotifySessionTask(Closure task, ErrorCallback error_callback) 
>   : task_(task),
>     error_callback_(error_callback) {
> }
> 
> ~NotifySessionTask() {}
> 
> NotifySessionTask::Run() {
>   task.Run();
> }
> 
> NotifySessionTask::Cancel() {
>   error_callback_.Run(GATT_ERROR);
> }
> 
> StartNotifySessionImpl(callback, error_callback) {
>    if (IsNotifying()) {
>     base::ThreadTaskRunnerHandle::Get()->PostTask(
>         FROM_HERE, base::Bind(&OnSubscribeToNotificationsSuccess,
>                               weak_ptr_factory_.GetWeakPtr(),
>                               callback, error_callback));
>     return;
>   }
>   SubscribeToNotifications(base::Bind(OnSubscribeToNotificationsSuccess,
>                                       weak_ptr_factory_.GetWeakPtr(),
>                                       callback),
>                            base::Bind(OnSubscribetoNotificationsFailed,
>                                       weak_ptr_factory_.GetWeakPtr(),
>                                       error_callback));
> }
> 
> // TODO: Implement StopNotifySessionImpl
> 
> 
> OnSubscribeToNotificationsSuccess(NotifySessionCallback callback) {
>   pending_notify_tasks_.pop();
>   // TODO: make notify session.
>   callback.Run(std::move(notify_session));
>   
>   if (!pending_notify_tasks_.empty()) {
>     pending_notify_tasks_.front().Run();
>   }
> }
> 
> // TODO: Implement OnSubscribeToNotificationsSuccess,
> OnSubscribeToNotificationsError,
> // OnUnsubscribeToNotificationsSuccess and OnUnsubscribeToNotificationsError.
> 
> StartNotifySession(NotifySessionCallback callback, ErrorCallback,
> error_callback) {
>   pending_notify_tasks_.push(MakeUnique<NotifySessionTask>(
>     base::Bind(StartNotifySessionImpl, this, callback, error_callback),
> error_callback));
>   if (pending_notify_tasks_size() == 1) {
>     pending_notify_tasks_.front().Run();
>   }
> }
> 
> StopNotifySession(...) {
>   pending_notify_tasks_.push(MakeUnique<NotifySessionTask>(
>     base::Bind(StopNotifySessionImpl, callback, error_callback),
> error_callback));
>   if (pending_notify_tasks_size() == 1) {
>     pending_notify_tasks_.fron().Run();
>   }
> }

I really like this approach, and I agree that the multiple Command classes is
not super-elegant. The only problem is that the success callbacks for
StartNotifySession and StopNotifySession are not the same type (The former takes
a session object as a parameter, the latter does not have any parameters). I
think it might be easier to do this change at the same time as we move to
creating session objects immediately in StartNotifySession without waiting for
success or error. Once we make that change, the success callbacks can be plain
Closures for both start and stop.

https://codereview.chromium.org/2051333004/diff/420001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic.cc:82: if (!hasNotify &&
!hasIndicate) {
On 2016/07/28 21:59:30, ortuno wrote:
> I think you can move this logic into SubscribeToNotifications. See comment in
> SubscribeToNotifications.

Actually, I'll be moving more functionality into this function. This function is
cross-platform, and I think this is an ok place to keep all of this logic.

https://codereview.chromium.org/2051333004/diff/420001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic.cc:153: if (next->type_ ==
COMMAND_START) {
On 2016/07/28 21:59:30, ortuno wrote:
> Do you do this for any other reason that optimizing?

Yes, if we have multiple StartNotifySession commands sitting in the front of the
queue, a single OnError or OnSuccess callback should resolve all of them.

https://codereview.chromium.org/2051333004/diff/420001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_remote_gatt_characteristic.h (right):

https://codereview.chromium.org/2051333004/diff/420001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic.h:80: virtual bool
IsNotifying() const;
On 2016/07/28 21:59:30, ortuno wrote:
> hmm this should really be private. I can't think of any use cases for it.
There
> are two uses in the codebase: One of them is using it completely wrong and the
> other one is working around a bug that was fixed... OK to leave public for now
> but can you add the following:
> 
> // TODO(crbug.com/632476): Make private since there are no uses for this
> function.

Even though there are no good uses of this function at the moment, there could
be in the future. I feel that this function is a potentially useful and
perfectly reasonable part of this class' public api.

https://codereview.chromium.org/2051333004/diff/420001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic.h:85: // ownership belongs
to the caller. |error_callback| is called on errors.
On 2016/07/28 21:59:30, ortuno wrote:
> I think we should document this more. Please explain:
> 
> - How notifications are delivered e.g. CharacteristicValueChanged is called
> when...
> - How to stop notifications e.g. calling session->Stop or destroying the
> session.
> 
> Anything else that you think might be relevant for someone that wants to use
> this API.

Done.

https://codereview.chromium.org/2051333004/diff/420001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic.h:102: virtual void
StopNotifySession(BluetoothGattNotifySession* session,
On 2016/07/28 21:59:30, ortuno wrote:
> I don't think we want to expose another way of stopping sessions. Make this
> private and have BluetoothNotifySession be a friend of this class.

Done.

https://codereview.chromium.org/2051333004/diff/420001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic.h:126: // Writes to the
Client Characteristic Configuration descriptor to enable
On 2016/07/28 21:59:30, ortuno wrote:
> Also could you explain the relationship between these two methods and
> Start/StopNotifySessions? No need to go into too much detail, just saying that
> these two methods are meant to be called by Start/StopNotifySessions might be
> enough.

Done.

https://codereview.chromium.org/2051333004/diff/420001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic.h:130: // not present
|error_callback| will be run.
On 2016/07/28 21:59:30, ortuno wrote:
> We probably want this behavior, failing on no descriptor, to be cross
platform.
> Do you think we could include this in StartNotifySession? Or maybe implement
> this here and have other implementations override
SubscribeToNotificationsImpl?

I have moved this behavior to
BluetoothRemoteGattCharacteristic::NotifySessionCommandStart::Execute().

https://codereview.chromium.org/2051333004/diff/420001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_remote_gatt_characteristic_android.cc (right):

https://codereview.chromium.org/2051333004/diff/420001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic_android.cc:242: void
BluetoothRemoteGattCharacteristicAndroid::SubscribeToNotifications(
On 2016/07/28 21:59:30, ortuno wrote:
> I would be OK with keeping this as NOTIMPLEMENTED in this CL. And moving each
> platform in a different CL.

Would you also be OK with leaving it here? With my latest patchset, there is
very little code left in this method.

https://codereview.chromium.org/2051333004/diff/420001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic_android.cc:263: if
(ccc_descriptor.size() != 1u) {
On 2016/07/28 21:59:30, ortuno wrote:
> I think we should move this to the cross platform implementation as well. See
> comment in bluetooth_remote_gatt_characteristic.h

Done.

https://codereview.chromium.org/2051333004/diff/420001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic_android.cc:284:
std::vector<uint8_t> value(2);
On 2016/07/28 21:59:30, ortuno wrote:
> optional nit:
> 
> std::vector<uint8_t> value = hasNotify ? {1, 0} : {2, 0};

This doesn't seem to compile...

[tommyt, 2016-08-01 12:48:28]

https://codereview.chromium.org/2051333004/diff/420001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_remote_gatt_characteristic_mac.mm (right):

https://codereview.chromium.org/2051333004/diff/420001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic_mac.mm:229:
NOTIMPLEMENTED();
On 2016/07/28 21:59:30, ortuno wrote:
> Can you open an issue that mentions that this needs to be implemented on
macOS?

I have created https://crbug.com/633191

https://codereview.chromium.org/2051333004/diff/420001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc (right):

https://codereview.chromium.org/2051333004/diff/420001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc:1172: #if
defined(OS_ANDROID)
On 2016/07/28 21:59:30, ortuno wrote:
> How do you feel about splitting the testing in two parts,
> Start/StopNotifySession tests and Subscribe/Unsubscribe tests?
> 
> For the Start/StopNotifySession tests you can just construct a mock
> characteristic with fake subscribe and unsubscribe, that way you don't need to
> worry about all the descriptor stuff.
> 
> For Subscribe/Unsubscribe you would need to worry about those but then you
don't
> need to worry about so many tests.
> 
> WDYT?

I don't really have a strong opinion on this. There are advantages to testing
the whole system at once, too, since it means we test something that is more
similar to the code that will actually be run in the various browsers. Do you
think the advantages to splitting up the code outweigh the advantages of keeping
it together?

If we do go the route of splitting this up, maybe we can leave this change as it
is, and do the split in a separate change?

[commit-bot: I haz the power, 2016-08-01 14:23:20]

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

[commit-bot: I haz the power, 2016-08-01 14:23:21]

Dry run: This issue passed the CQ dry run.

[ortuno, 2016-08-02 01:56:34]

https://codereview.chromium.org/2051333004/diff/420001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_remote_gatt_characteristic.cc (right):

https://codereview.chromium.org/2051333004/diff/420001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic.cc:46: void
BluetoothRemoteGattCharacteristic::StartNotifySession(
On 2016/08/01 at 11:39:21, tommyt wrote:
> On 2016/07/28 21:59:30, ortuno wrote:
> > This looks very similar to what I first wrote for the notifications queue in
the
> > renderer and also what I started to do for the getPrimaryServices code. I
think
> > we can simplify this a bit by using closures so that we don't have to create
to
> > a base class and two classes that implement it. What do you think of the
> > following code? It introduces a NotifySessionTask that takes a closure and a
> > callback to be called when cancelling tasks. The rest of the code is very
> > similar to what you have already.
> > 
> > 
> > NotifySessionTask(Closure task, ErrorCallback error_callback) 
> >   : task_(task),
> >     error_callback_(error_callback) {
> > }
> > 
> > ~NotifySessionTask() {}
> > 
> > NotifySessionTask::Run() {
> >   task.Run();
> > }
> > 
> > NotifySessionTask::Cancel() {
> >   error_callback_.Run(GATT_ERROR);
> > }
> > 
> > StartNotifySessionImpl(callback, error_callback) {
> >    if (IsNotifying()) {
> >     base::ThreadTaskRunnerHandle::Get()->PostTask(
> >         FROM_HERE, base::Bind(&OnSubscribeToNotificationsSuccess,
> >                               weak_ptr_factory_.GetWeakPtr(),
> >                               callback, error_callback));
> >     return;
> >   }
> >   SubscribeToNotifications(base::Bind(OnSubscribeToNotificationsSuccess,
> >                                       weak_ptr_factory_.GetWeakPtr(),
> >                                       callback),
> >                            base::Bind(OnSubscribetoNotificationsFailed,
> >                                       weak_ptr_factory_.GetWeakPtr(),
> >                                       error_callback));
> > }
> > 
> > // TODO: Implement StopNotifySessionImpl
> > 
> > 
> > OnSubscribeToNotificationsSuccess(NotifySessionCallback callback) {
> >   pending_notify_tasks_.pop();
> >   // TODO: make notify session.
> >   callback.Run(std::move(notify_session));
> >   
> >   if (!pending_notify_tasks_.empty()) {
> >     pending_notify_tasks_.front().Run();
> >   }
> > }
> > 
> > // TODO: Implement OnSubscribeToNotificationsSuccess,
> > OnSubscribeToNotificationsError,
> > // OnUnsubscribeToNotificationsSuccess and
OnUnsubscribeToNotificationsError.
> > 
> > StartNotifySession(NotifySessionCallback callback, ErrorCallback,
> > error_callback) {
> >   pending_notify_tasks_.push(MakeUnique<NotifySessionTask>(
> >     base::Bind(StartNotifySessionImpl, this, callback, error_callback),
> > error_callback));
> >   if (pending_notify_tasks_size() == 1) {
> >     pending_notify_tasks_.front().Run();
> >   }
> > }
> > 
> > StopNotifySession(...) {
> >   pending_notify_tasks_.push(MakeUnique<NotifySessionTask>(
> >     base::Bind(StopNotifySessionImpl, callback, error_callback),
> > error_callback));
> >   if (pending_notify_tasks_size() == 1) {
> >     pending_notify_tasks_.fron().Run();
> >   }
> > }
> 
> I really like this approach, and I agree that the multiple Command classes is
not super-elegant. The only problem is that the success callbacks for
StartNotifySession and StopNotifySession are not the same type (The former takes
a session object as a parameter, the latter does not have any parameters). I
think it might be easier to do this change at the same time as we move to
creating session objects immediately in StartNotifySession without waiting for
success or error. Once we make that change, the success callbacks can be plain
Closures for both start and stop.

Hmm maybe I'm missing something, but I don't think the different callback type
makes a difference? You aren't storing the actual callback just a closure to run
StartNotifySessionImpl or StopNotifySessionImpl.

https://codereview.chromium.org/2051333004/diff/420001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic.cc:82: if (!hasNotify &&
!hasIndicate) {
On 2016/08/01 at 11:39:21, tommyt wrote:
> On 2016/07/28 21:59:30, ortuno wrote:
> > I think you can move this logic into SubscribeToNotifications. See comment
in
> > SubscribeToNotifications.
> 
> Actually, I'll be moving more functionality into this function. This function
is cross-platform, and I think this is an ok place to keep all of this logic.

I'm a bit hesitant to put low level concepts like properties and descriptors in
a high level function that is meant to coordinate notify sessions.

https://codereview.chromium.org/2051333004/diff/420001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_remote_gatt_characteristic.h (right):

https://codereview.chromium.org/2051333004/diff/420001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic.h:80: virtual bool
IsNotifying() const;
On 2016/08/01 at 11:39:21, tommyt wrote:
> On 2016/07/28 21:59:30, ortuno wrote:
> > hmm this should really be private. I can't think of any use cases for it.
There
> > are two uses in the codebase: One of them is using it completely wrong and
the
> > other one is working around a bug that was fixed... OK to leave public for
now
> > but can you add the following:
> > 
> > // TODO(crbug.com/632476): Make private since there are no uses for this
> > function.
> 
> Even though there are no good uses of this function at the moment, there could
be in the future. I feel that this function is a potentially useful and
perfectly reasonable part of this class' public api.

I think this makes the API a bit confusing and doesn't really provide any useful
information to clients i.e. why would a client care if there are any other
clients that have notify sessions? Anyway I don't feel too strongly about it,
but I would feel better if the documentation is improved a bit. I mainly want to
avoid cases like this one: http://crrev.com/2189183002

https://codereview.chromium.org/2051333004/diff/420001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc (right):

https://codereview.chromium.org/2051333004/diff/420001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc:1172: #if
defined(OS_ANDROID)
On 2016/08/01 at 12:48:27, tommyt wrote:
> On 2016/07/28 21:59:30, ortuno wrote:
> > How do you feel about splitting the testing in two parts,
> > Start/StopNotifySession tests and Subscribe/Unsubscribe tests?
> > 
> > For the Start/StopNotifySession tests you can just construct a mock
> > characteristic with fake subscribe and unsubscribe, that way you don't need
to
> > worry about all the descriptor stuff.
> > 
> > For Subscribe/Unsubscribe you would need to worry about those but then you
don't
> > need to worry about so many tests.
> > 
> > WDYT?
> 
> I don't really have a strong opinion on this. There are advantages to testing
the whole system at once, too, since it means we test something that is more
similar to the code that will actually be run in the various browsers. Do you
think the advantages to splitting up the code outweigh the advantages of keeping
it together?
> 
> If we do go the route of splitting this up, maybe we can leave this change as
it is, and do the split in a separate change?

I'm a fan of unit tests but scheib prefers integration tests. So it's up to you.
This needs a lot of tests so separating might be better.

https://codereview.chromium.org/2051333004/diff/440001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_remote_gatt_characteristic.cc (right):

https://codereview.chromium.org/2051333004/diff/440001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic.cc:101:
std::vector<BluetoothRemoteGattDescriptor*> ccc_descriptor =
Can you surround this with a #if !(defined(OS_CHROMEOS) || defined(OS_LINUX) ||
defined(OS_MACOSX) || defined(OS_WIN)). We can enable each platform as we
migrate it to this model. Also mac doesn't implement descriptors yet.

https://codereview.chromium.org/2051333004/diff/440001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_remote_gatt_characteristic.h (right):

https://codereview.chromium.org/2051333004/diff/440001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic.h:102: // changes. These
notifications are routed up from the platform into the
This layer shouldn't know anything about javascript. Maybe:

When a new notification is received,
BluetoothAdapterObserver::GattCharacteristicValueChanged is called with the
characteristic's new value.

https://codereview.chromium.org/2051333004/diff/440001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc (right):

https://codereview.chromium.org/2051333004/diff/440001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc:1190:
RememberCharacteristicForSubsequentAction(characteristic1_);
nit: Move this after SimulateGattNotifySessionStarted.

https://codereview.chromium.org/2051333004/diff/440001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc:1193:
SimulateGattNotifySessionStarted(/* use remembered characteristic */ nullptr);
nit: the characteristic is still alive so you don't need to pass in a nullptr.

https://codereview.chromium.org/2051333004/diff/440001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc:1197: 
We should also test the state of the notify session i.e. GetCharacteristicId(),
GetCharacteristic(), IsActive(), etc. Same after you stop the session.

https://codereview.chromium.org/2051333004/diff/440001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc:1325: // Tests
StopNotifySession success on a characteristic enabling Notify.
nit: on a characteristic that enabled Notify.

https://codereview.chromium.org/2051333004/diff/440001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc:1326:
TEST_F(BluetoothRemoteGattCharacteristicTest, StopNotifySession) {
Also add a test to make sure notifications are stopped when the notify session
is destroyed.

https://codereview.chromium.org/2051333004/diff/440001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc:1331:
GetStopNotifyCallback(Call::EXPECTED, notify_sessions_[0]));
There are two things you need to make sure right after you call Stop on a notify
session:

1. The right values were written to the descriptor, and
2. The stop session is inactive.

For 1:

You can copy the expects from line 130.

For 2:
EXPECT_FALSE(notify_session_[0]->IsActive());
EXPECT_EQ(nullptr, notify_session_[0]->GetCharacteristic());
...

https://codereview.chromium.org/2051333004/diff/440001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc:1334:
ASSERT_EQ(0u, notify_sessions_.size());
Also add EXPECTS for IsNotifying()

https://codereview.chromium.org/2051333004/diff/440001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc:1339: // Tests
StopNotifySession success on a characteristic enabling Indicate.
nit: on a characteristic that enabled Notify.

https://codereview.chromium.org/2051333004/diff/440001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc:1346:
SimulateGattNotifySessionStopped(characteristic1_);
Same here. Add the same EXPECTS as above.

https://codereview.chromium.org/2051333004/diff/440001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc:1353: // Tests
StopNotifySession success on a characteristic enabling Notify &
nit: on a characteristic that enabled Notify & Indicate.

https://codereview.chromium.org/2051333004/diff/440001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc:1358: /*
properties: INDICATE */ 0x30,
nit: properties: NOTIFY & INDICATE

https://codereview.chromium.org/2051333004/diff/440001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc:1362:
SimulateGattNotifySessionStopped(characteristic1_);
Same here. Please add same EXPECTS as above.

https://codereview.chromium.org/2051333004/diff/440001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc:1375:
GetStopNotifyCallback(Call::EXPECTED, notify_sessions_[0]));
Test the state of the notify session before and after the error as well.

https://codereview.chromium.org/2051333004/diff/440001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc:1379:
ASSERT_EQ(0u, notify_sessions_.size());
Also test IsNotifying()

https://codereview.chromium.org/2051333004/diff/440001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc:1411:
GetStopNotifyCallback(Call::EXPECTED, notify_sessions_[0]));
We need a way to test that callbacks are called in the right order. What do you
think about the following in bluetooth_test:

base::Closure NotifySessionCheckForPrecedingCalls( // Or similar name.
  base::Closure stop_callback,
  int num_of_preceding_calls) {
  
  EXPECT_EQ(num_of_preceding_calls, callback_count_);

  stop_callback.Run();
}

This test would look like this:

ResetEventCounts();
notify_session_[0]->Stop(
    base::Bind(BluetoothTest::NotifySessionCheckForPrecedingCalls,
               base::Unretained(this),
               GetStopNotifyCallback(Call::EXPECTED, notify_sessions_[0]),
               0 /* num_of_preceding_calls */));
    
notify_session_[0]->Stop(
    base::Bind(BluetoothTest::NotifySessionCheckForPrecedingCalls,
               base::Unretained(this),
               GetStopNotifyCallback(Call::EXPECTED, notify_sessions_[0]),
               1 /* num_of_preceding_calls */));

https://codereview.chromium.org/2051333004/diff/440001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc:1416:
ASSERT_EQ(0u, notify_sessions_.size());
optional nit: maybe test IsNotifying(). Not as important since other tests test
it.

https://codereview.chromium.org/2051333004/diff/440001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc:1425:
SimulateGattDescriptor(
Surround this with # !defined(OS_MACOSX) with a todo to remove it once
http://crbug.com/624017 is fixed.

https://codereview.chromium.org/2051333004/diff/440001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc:1432:
characteristic1_->StartNotifySession(
Same here. I think we need a way to test that callbacks are getting called in
the right order.

https://codereview.chromium.org/2051333004/diff/440001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc:1444:
ASSERT_EQ(2u, notify_sessions_.size());
Also test is IsNotifying()

https://codereview.chromium.org/2051333004/diff/440001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc:1456:
ASSERT_EQ(1u, notify_sessions_.size());
Also test IsNotifying() and that the other session is still active.

https://codereview.chromium.org/2051333004/diff/440001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc:1467: // Tests
starting a new notify session before the previous stop request resolves
nit: Period at the end of sentence.

https://codereview.chromium.org/2051333004/diff/440001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc:1477: // Start
notify session
Same here. Please add some code to make sure the callbacks are called in the
right order.

https://codereview.chromium.org/2051333004/diff/440001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc:1505: }
Also test IsNotifying()

https://codereview.chromium.org/2051333004/diff/440001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc:1509: // Tests
starting a new notify session before the previous stop request resolves
nit: Period at the end of sentence.

before previous stop request*s* resolve.

https://codereview.chromium.org/2051333004/diff/440001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc:1510:
TEST_F(BluetoothRemoteGattCharacteristicTest, StopNotifySession_StopStopStart) {
Another similar test but for StartNotifySession to make sure that a success
start doesn't cause other starts to jump ahead.

TEST_F(..., StartNotifySession_StartStopStart) {
  // Make sure the order of calls is correct.
  // Setup
  
  characteristic1_->StartNotifySession(...)

  // Start succeeds.

  characteristic1_->StartNotifySession(
    base::Bind(BluetoothTest::NotifySessionCheckForPrecedingCalls,
               base::Unretained(this),
               1 /* num_of_preceding_calls */),
    GetStartNotifyErrorCallback(...));

  notify_sessions_[0]->Stop(
      base::Bind(BluetoothTest::NotifySessionCheckForPrecedingCalls,
                 base::Unretained(this),
                 2 /* num_of_preceding_calls */));
          
  characteristic1_->StartNotifySession(
      base::Bind(BluetoothTest::NotifySessionCheckForPrecedingCalls,
                 base::Unretained(this),
                 3 /* num_of_preceding_calls */),
      GetStartNotifyErrorCallback(...));
   
  // Simulate necessary events and run loop.

  EXPECT_FALSE(notify_sessions_[0]->IsActive());
  EXPECT_TRUE(notify_sessions_[1]->IsActive());
  EXPECT_TRUE(notify_sessions_[2]->IsActive());

  EXPECT_TRUE(characteristic1_->IsNotifying());
  // EXPECT value of CCC descriptor is 1.
  // EXPECT only one write to the CCC descriptor.

}

https://codereview.chromium.org/2051333004/diff/440001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc:1549: }
Same here we need to make sure that the callbacks are run in the right order and
the right values were written to the descriptor.

https://codereview.chromium.org/2051333004/diff/440001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc:1552: #if
defined(OS_ANDROID) || defined(OS_MACOSX) || defined(OS_WIN)
Also tests for reentrancy i.e. you need to make sure calling StartNotifySession
and Stop() inside a callback are correctly handled. We already have a bunch for
StartNotifySession, but we are missing tests for Stop calls inside
StartNotifySession's callback and StartNotifySession called inside Stop's
callback. The good thing is that now you can use Bind lambdas so the tests
shouldn't be as hard as the already existing tests. I believe there are 3
combinations:

StartSuccess => Stop
Stop => StartSuccess
Stop => StartError

TEST_F(StartNotifySession_Reentrant_Success_Stop) {
  // Setup

  characteristic1_->StartNotifySession(
      base::Bind([](NotifySessionCallback callback,
                    std::unique_ptr<BluetoothGattNotifySession> session) {
                      BluetoothGattNotifySession* s = session.get(); // Or pass
in a reference to the sessions array.
                      callback.Run(std::move(session));
                      s->GetCharacteristic()->Stop(...);
                 }, GetStartNotifySessionCallback(...)),
      GetStartNotifySessionErrorCallback(...));

  // Run loop.
  EXPECT_FALSE(characteristic1_->IsNotifying());
  EXPECT_FALSE(notify_sessions_->IsActvice());
  // EXPECT two calls to write to descriptors.
  // EXPECT final value of descriptor to be 0.
}

If lambdas don't work you might have to add a function for each case in
bluetooth_test.

https://codereview.chromium.org/2051333004/diff/440001/device/bluetooth/test/...
File device/bluetooth/test/bluetooth_test.cc (right):

https://codereview.chromium.org/2051333004/diff/440001/device/bluetooth/test/...
device/bluetooth/test/bluetooth_test.cc:148: for
(ScopedVector<BluetoothGattNotifySession>::iterator i =
If you do this then you never get a chance to test the state of the
NotifySession after the callback is called.

[ortuno, 2016-08-03 16:55:43]

https://codereview.chromium.org/2051333004/diff/420001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_remote_gatt_characteristic.cc (right):

https://codereview.chromium.org/2051333004/diff/420001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic.cc:46: void
BluetoothRemoteGattCharacteristic::StartNotifySession(
On 2016/08/02 at 01:56:31, ortuno wrote:
> On 2016/08/01 at 11:39:21, tommyt wrote:
> > On 2016/07/28 21:59:30, ortuno wrote:
> > > This looks very similar to what I first wrote for the notifications queue
in the
> > > renderer and also what I started to do for the getPrimaryServices code. I
think
> > > we can simplify this a bit by using closures so that we don't have to
create to
> > > a base class and two classes that implement it. What do you think of the
> > > following code? It introduces a NotifySessionTask that takes a closure and
a
> > > callback to be called when cancelling tasks. The rest of the code is very
> > > similar to what you have already.
> > > 
> > > 
> > > NotifySessionTask(Closure task, ErrorCallback error_callback) 
> > >   : task_(task),
> > >     error_callback_(error_callback) {
> > > }
> > > 
> > > ~NotifySessionTask() {}
> > > 
> > > NotifySessionTask::Run() {
> > >   task.Run();
> > > }
> > > 
> > > NotifySessionTask::Cancel() {
> > >   error_callback_.Run(GATT_ERROR);
> > > }
> > > 
> > > StartNotifySessionImpl(callback, error_callback) {
> > >    if (IsNotifying()) {
> > >     base::ThreadTaskRunnerHandle::Get()->PostTask(
> > >         FROM_HERE, base::Bind(&OnSubscribeToNotificationsSuccess,
> > >                               weak_ptr_factory_.GetWeakPtr(),
> > >                               callback, error_callback));
> > >     return;
> > >   }
> > >   SubscribeToNotifications(base::Bind(OnSubscribeToNotificationsSuccess,
> > >                                       weak_ptr_factory_.GetWeakPtr(),
> > >                                       callback),
> > >                            base::Bind(OnSubscribetoNotificationsFailed,
> > >                                       weak_ptr_factory_.GetWeakPtr(),
> > >                                       error_callback));
> > > }
> > > 
> > > // TODO: Implement StopNotifySessionImpl
> > > 
> > > 
> > > OnSubscribeToNotificationsSuccess(NotifySessionCallback callback) {
> > >   pending_notify_tasks_.pop();
> > >   // TODO: make notify session.
> > >   callback.Run(std::move(notify_session));
> > >   
> > >   if (!pending_notify_tasks_.empty()) {
> > >     pending_notify_tasks_.front().Run();
> > >   }
> > > }
> > > 
> > > // TODO: Implement OnSubscribeToNotificationsSuccess,
> > > OnSubscribeToNotificationsError,
> > > // OnUnsubscribeToNotificationsSuccess and
OnUnsubscribeToNotificationsError.
> > > 
> > > StartNotifySession(NotifySessionCallback callback, ErrorCallback,
> > > error_callback) {
> > >   pending_notify_tasks_.push(MakeUnique<NotifySessionTask>(
> > >     base::Bind(StartNotifySessionImpl, this, callback, error_callback),
> > > error_callback));
> > >   if (pending_notify_tasks_size() == 1) {
> > >     pending_notify_tasks_.front().Run();
> > >   }
> > > }
> > > 
> > > StopNotifySession(...) {
> > >   pending_notify_tasks_.push(MakeUnique<NotifySessionTask>(
> > >     base::Bind(StopNotifySessionImpl, callback, error_callback),
> > > error_callback));
> > >   if (pending_notify_tasks_size() == 1) {
> > >     pending_notify_tasks_.fron().Run();
> > >   }
> > > }
> > 
> > I really like this approach, and I agree that the multiple Command classes
is not super-elegant. The only problem is that the success callbacks for
StartNotifySession and StopNotifySession are not the same type (The former takes
a session object as a parameter, the latter does not have any parameters). I
think it might be easier to do this change at the same time as we move to
creating session objects immediately in StartNotifySession without waiting for
success or error. Once we make that change, the success callbacks can be plain
Closures for both start and stop.
> 
> Hmm maybe I'm missing something, but I don't think the different callback type
makes a difference? You aren't storing the actual callback just a closure to run
StartNotifySessionImpl or StopNotifySessionImpl.

Ah I see what you mean now. You could still just save a closure with the error
e.g.:

new NotifySessionTask(base::Bind(StartNotifySessionImpl,...),
base::Bind(error_callback, GATT_ERROR));

And for Stop:

new NotifySessionTask(base::Bind(StopNotifySessionImpl,...), callback);

[tommyt, 2016-08-05 12:21:17]

The CQ bit was checked by tommyt@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-05 12:21:39]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tommyt, 2016-08-05 12:22:30]

I've now added two more patchsets which I believe should address all issues and
comments.

Please take another look :)

https://codereview.chromium.org/2051333004/diff/440001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_remote_gatt_characteristic.cc (right):

https://codereview.chromium.org/2051333004/diff/440001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic.cc:101:
std::vector<BluetoothRemoteGattDescriptor*> ccc_descriptor =
On 2016/08/02 01:56:32, ortuno wrote:
> Can you surround this with a #if !(defined(OS_CHROMEOS) || defined(OS_LINUX)
||
> defined(OS_MACOSX) || defined(OS_WIN)). We can enable each platform as we
> migrate it to this model. Also mac doesn't implement descriptors yet.

Done.

https://codereview.chromium.org/2051333004/diff/440001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_remote_gatt_characteristic.h (right):

https://codereview.chromium.org/2051333004/diff/440001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic.h:102: // changes. These
notifications are routed up from the platform into the
On 2016/08/02 01:56:32, ortuno wrote:
> This layer shouldn't know anything about javascript. Maybe:
> 
> When a new notification is received,
> BluetoothAdapterObserver::GattCharacteristicValueChanged is called with the
> characteristic's new value.

Done.

https://codereview.chromium.org/2051333004/diff/440001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc (right):

https://codereview.chromium.org/2051333004/diff/440001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc:1190:
RememberCharacteristicForSubsequentAction(characteristic1_);
On 2016/08/02 01:56:33, ortuno wrote:
> nit: Move this after SimulateGattNotifySessionStarted.

Done.

https://codereview.chromium.org/2051333004/diff/440001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc:1193:
SimulateGattNotifySessionStarted(/* use remembered characteristic */ nullptr);
On 2016/08/02 01:56:33, ortuno wrote:
> nit: the characteristic is still alive so you don't need to pass in a nullptr.

Done.

https://codereview.chromium.org/2051333004/diff/440001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc:1197: 
On 2016/08/02 01:56:33, ortuno wrote:
> We should also test the state of the notify session i.e.
GetCharacteristicId(),
> GetCharacteristic(), IsActive(), etc. Same after you stop the session.

Done.

https://codereview.chromium.org/2051333004/diff/440001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc:1325: // Tests
StopNotifySession success on a characteristic enabling Notify.
On 2016/08/02 01:56:33, ortuno wrote:
> nit: on a characteristic that enabled Notify.

Done.

https://codereview.chromium.org/2051333004/diff/440001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc:1326:
TEST_F(BluetoothRemoteGattCharacteristicTest, StopNotifySession) {
On 2016/08/02 01:56:32, ortuno wrote:
> Also add a test to make sure notifications are stopped when the notify session
> is destroyed.

Done.

https://codereview.chromium.org/2051333004/diff/440001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc:1331:
GetStopNotifyCallback(Call::EXPECTED, notify_sessions_[0]));
On 2016/08/02 01:56:32, ortuno wrote:
> There are two things you need to make sure right after you call Stop on a
notify
> session:
> 
> 1. The right values were written to the descriptor, and
> 2. The stop session is inactive.
> 
> For 1:
> 
> You can copy the expects from line 130.
> 
> For 2:
> EXPECT_FALSE(notify_session_[0]->IsActive());
> EXPECT_EQ(nullptr, notify_session_[0]->GetCharacteristic());
> ...

Done.

https://codereview.chromium.org/2051333004/diff/440001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc:1334:
ASSERT_EQ(0u, notify_sessions_.size());
On 2016/08/02 01:56:33, ortuno wrote:
> Also add EXPECTS for IsNotifying()

Done.

https://codereview.chromium.org/2051333004/diff/440001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc:1339: // Tests
StopNotifySession success on a characteristic enabling Indicate.
On 2016/08/02 01:56:32, ortuno wrote:
> nit: on a characteristic that enabled Notify.

Done.

https://codereview.chromium.org/2051333004/diff/440001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc:1346:
SimulateGattNotifySessionStopped(characteristic1_);
On 2016/08/02 01:56:33, ortuno wrote:
> Same here. Add the same EXPECTS as above.

Done.

https://codereview.chromium.org/2051333004/diff/440001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc:1353: // Tests
StopNotifySession success on a characteristic enabling Notify &
On 2016/08/02 01:56:33, ortuno wrote:
> nit: on a characteristic that enabled Notify & Indicate.

Done.

https://codereview.chromium.org/2051333004/diff/440001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc:1358: /*
properties: INDICATE */ 0x30,
On 2016/08/02 01:56:33, ortuno wrote:
> nit: properties: NOTIFY & INDICATE

Done.

https://codereview.chromium.org/2051333004/diff/440001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc:1362:
SimulateGattNotifySessionStopped(characteristic1_);
On 2016/08/02 01:56:32, ortuno wrote:
> Same here. Please add same EXPECTS as above.

Done.

https://codereview.chromium.org/2051333004/diff/440001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc:1375:
GetStopNotifyCallback(Call::EXPECTED, notify_sessions_[0]));
On 2016/08/02 01:56:32, ortuno wrote:
> Test the state of the notify session before and after the error as well.

Done.

https://codereview.chromium.org/2051333004/diff/440001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc:1379:
ASSERT_EQ(0u, notify_sessions_.size());
On 2016/08/02 01:56:33, ortuno wrote:
> Also test IsNotifying()

Done.

https://codereview.chromium.org/2051333004/diff/440001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc:1411:
GetStopNotifyCallback(Call::EXPECTED, notify_sessions_[0]));
On 2016/08/02 01:56:33, ortuno wrote:
> We need a way to test that callbacks are called in the right order. What do
you
> think about the following in bluetooth_test:
> 
> base::Closure NotifySessionCheckForPrecedingCalls( // Or similar name.
>   base::Closure stop_callback,
>   int num_of_preceding_calls) {
>   
>   EXPECT_EQ(num_of_preceding_calls, callback_count_);
> 
>   stop_callback.Run();
> }
> 
> This test would look like this:
> 
> ResetEventCounts();
> notify_session_[0]->Stop(
>     base::Bind(BluetoothTest::NotifySessionCheckForPrecedingCalls,
>                base::Unretained(this),
>                GetStopNotifyCallback(Call::EXPECTED, notify_sessions_[0]),
>                0 /* num_of_preceding_calls */));
>     
> notify_session_[0]->Stop(
>     base::Bind(BluetoothTest::NotifySessionCheckForPrecedingCalls,
>                base::Unretained(this),
>                GetStopNotifyCallback(Call::EXPECTED, notify_sessions_[0]),
>                1 /* num_of_preceding_calls */));

Done.

https://codereview.chromium.org/2051333004/diff/440001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc:1416:
ASSERT_EQ(0u, notify_sessions_.size());
On 2016/08/02 01:56:34, ortuno wrote:
> optional nit: maybe test IsNotifying(). Not as important since other tests
test
> it.

Done.

https://codereview.chromium.org/2051333004/diff/440001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc:1425:
SimulateGattDescriptor(
On 2016/08/02 01:56:32, ortuno wrote:
> Surround this with # !defined(OS_MACOSX) with a todo to remove it once
> http://crbug.com/624017 is fixed.

Done.

https://codereview.chromium.org/2051333004/diff/440001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc:1432:
characteristic1_->StartNotifySession(
On 2016/08/02 01:56:32, ortuno wrote:
> Same here. I think we need a way to test that callbacks are getting called in
> the right order.

Done.

https://codereview.chromium.org/2051333004/diff/440001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc:1444:
ASSERT_EQ(2u, notify_sessions_.size());
On 2016/08/02 01:56:33, ortuno wrote:
> Also test is IsNotifying()

Done.

https://codereview.chromium.org/2051333004/diff/440001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc:1456:
ASSERT_EQ(1u, notify_sessions_.size());
On 2016/08/02 01:56:33, ortuno wrote:
> Also test IsNotifying() and that the other session is still active.

Done.

https://codereview.chromium.org/2051333004/diff/440001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc:1467: // Tests
starting a new notify session before the previous stop request resolves
On 2016/08/02 01:56:32, ortuno wrote:
> nit: Period at the end of sentence.

Done.

https://codereview.chromium.org/2051333004/diff/440001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc:1477: // Start
notify session
On 2016/08/02 01:56:32, ortuno wrote:
> Same here. Please add some code to make sure the callbacks are called in the
> right order.

Done.

https://codereview.chromium.org/2051333004/diff/440001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc:1505: }
On 2016/08/02 01:56:32, ortuno wrote:
> Also test IsNotifying()

Done.

https://codereview.chromium.org/2051333004/diff/440001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc:1509: // Tests
starting a new notify session before the previous stop request resolves
On 2016/08/02 01:56:33, ortuno wrote:
> nit: Period at the end of sentence.
> 
> before previous stop request*s* resolve.

Done.

https://codereview.chromium.org/2051333004/diff/440001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc:1510:
TEST_F(BluetoothRemoteGattCharacteristicTest, StopNotifySession_StopStopStart) {
On 2016/08/02 01:56:32, ortuno wrote:
> Another similar test but for StartNotifySession to make sure that a success
> start doesn't cause other starts to jump ahead.
> 
> TEST_F(..., StartNotifySession_StartStopStart) {
>   // Make sure the order of calls is correct.
>   // Setup
>   
>   characteristic1_->StartNotifySession(...)
> 
>   // Start succeeds.
> 
>   characteristic1_->StartNotifySession(
>     base::Bind(BluetoothTest::NotifySessionCheckForPrecedingCalls,
>                base::Unretained(this),
>                1 /* num_of_preceding_calls */),
>     GetStartNotifyErrorCallback(...));
> 
>   notify_sessions_[0]->Stop(
>       base::Bind(BluetoothTest::NotifySessionCheckForPrecedingCalls,
>                  base::Unretained(this),
>                  2 /* num_of_preceding_calls */));
>           
>   characteristic1_->StartNotifySession(
>       base::Bind(BluetoothTest::NotifySessionCheckForPrecedingCalls,
>                  base::Unretained(this),
>                  3 /* num_of_preceding_calls */),
>       GetStartNotifyErrorCallback(...));
>    
>   // Simulate necessary events and run loop.
> 
>   EXPECT_FALSE(notify_sessions_[0]->IsActive());
>   EXPECT_TRUE(notify_sessions_[1]->IsActive());
>   EXPECT_TRUE(notify_sessions_[2]->IsActive());
> 
>   EXPECT_TRUE(characteristic1_->IsNotifying());
>   // EXPECT value of CCC descriptor is 1.
>   // EXPECT only one write to the CCC descriptor.
> 
> }

This test case would have to be added later, after we do the change where
StartNotifySession returns a session object immediately. At the moment, it is
not possible to queue up a start, stop, start, and then simulate all the events
(since the stop depends on the initial start having finished).

https://codereview.chromium.org/2051333004/diff/440001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc:1549: }
On 2016/08/02 01:56:33, ortuno wrote:
> Same here we need to make sure that the callbacks are run in the right order
and
> the right values were written to the descriptor.

Done.

https://codereview.chromium.org/2051333004/diff/440001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc:1552: #if
defined(OS_ANDROID) || defined(OS_MACOSX) || defined(OS_WIN)
On 2016/08/02 01:56:33, ortuno wrote:
> Also tests for reentrancy i.e. you need to make sure calling
StartNotifySession
> and Stop() inside a callback are correctly handled. We already have a bunch
for
> StartNotifySession, but we are missing tests for Stop calls inside
> StartNotifySession's callback and StartNotifySession called inside Stop's
> callback. The good thing is that now you can use Bind lambdas so the tests
> shouldn't be as hard as the already existing tests. I believe there are 3
> combinations:
> 
> StartSuccess => Stop
> Stop => StartSuccess
> Stop => StartError
> 
> TEST_F(StartNotifySession_Reentrant_Success_Stop) {
>   // Setup
> 
>   characteristic1_->StartNotifySession(
>       base::Bind([](NotifySessionCallback callback,
>                     std::unique_ptr<BluetoothGattNotifySession> session) {
>                       BluetoothGattNotifySession* s = session.get(); // Or
pass
> in a reference to the sessions array.
>                       callback.Run(std::move(session));
>                       s->GetCharacteristic()->Stop(...);
>                  }, GetStartNotifySessionCallback(...)),
>       GetStartNotifySessionErrorCallback(...));
> 
>   // Run loop.
>   EXPECT_FALSE(characteristic1_->IsNotifying());
>   EXPECT_FALSE(notify_sessions_->IsActvice());
>   // EXPECT two calls to write to descriptors.
>   // EXPECT final value of descriptor to be 0.
> }
> 
> If lambdas don't work you might have to add a function for each case in
> bluetooth_test.

Done.

https://codereview.chromium.org/2051333004/diff/440001/device/bluetooth/test/...
File device/bluetooth/test/bluetooth_test.cc (right):

https://codereview.chromium.org/2051333004/diff/440001/device/bluetooth/test/...
device/bluetooth/test/bluetooth_test.cc:148: for
(ScopedVector<BluetoothGattNotifySession>::iterator i =
On 2016/08/02 01:56:34, ortuno wrote:
> If you do this then you never get a chance to test the state of the
> NotifySession after the callback is called.

I have removed this and adjusted the unittests.

[commit-bot: I haz the power, 2016-08-05 12:31:06]

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

[commit-bot: I haz the power, 2016-08-05 12:31:08]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)

[tommyt, 2016-08-05 12:39:31]

The CQ bit was checked by tommyt@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-05 12:39:55]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-05 12:53:17]

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

[commit-bot: I haz the power, 2016-08-05 12:53:18]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[ortuno, 2016-08-05 14:47:00]

You forgot the comments on patch 22 :)

https://codereview.chromium.org/2051333004/diff/440001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc (right):

https://codereview.chromium.org/2051333004/diff/440001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc:1510:
TEST_F(BluetoothRemoteGattCharacteristicTest, StopNotifySession_StopStopStart) {
On 2016/08/05 at 12:22:29, tommyt wrote:
> On 2016/08/02 01:56:32, ortuno wrote:
> > Another similar test but for StartNotifySession to make sure that a success
> > start doesn't cause other starts to jump ahead.
> > 
> > TEST_F(..., StartNotifySession_StartStopStart) {
> >   // Make sure the order of calls is correct.
> >   // Setup
> >   
> >   characteristic1_->StartNotifySession(...)
> > 
> >   // Start succeeds.
> > 
> >   characteristic1_->StartNotifySession(
> >     base::Bind(BluetoothTest::NotifySessionCheckForPrecedingCalls,
> >                base::Unretained(this),
> >                1 /* num_of_preceding_calls */),
> >     GetStartNotifyErrorCallback(...));
> > 
> >   notify_sessions_[0]->Stop(
> >       base::Bind(BluetoothTest::NotifySessionCheckForPrecedingCalls,
> >                  base::Unretained(this),
> >                  2 /* num_of_preceding_calls */));
> >           
> >   characteristic1_->StartNotifySession(
> >       base::Bind(BluetoothTest::NotifySessionCheckForPrecedingCalls,
> >                  base::Unretained(this),
> >                  3 /* num_of_preceding_calls */),
> >       GetStartNotifyErrorCallback(...));
> >    
> >   // Simulate necessary events and run loop.
> > 
> >   EXPECT_FALSE(notify_sessions_[0]->IsActive());
> >   EXPECT_TRUE(notify_sessions_[1]->IsActive());
> >   EXPECT_TRUE(notify_sessions_[2]->IsActive());
> > 
> >   EXPECT_TRUE(characteristic1_->IsNotifying());
> >   // EXPECT value of CCC descriptor is 1.
> >   // EXPECT only one write to the CCC descriptor.
> > 
> > }
> 
> This test case would have to be added later, after we do the change where
StartNotifySession returns a session object immediately. At the moment, it is
not possible to queue up a start, stop, start, and then simulate all the events
(since the stop depends on the initial start having finished).

There are three StartNotifications calls. The stop is called on the session that
the first StartNotifications() call produced.

[tommyt, 2016-08-08 13:51:06]

The CQ bit was checked by tommyt@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-08 13:51:31]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tommyt, 2016-08-08 13:55:08]

Today I've pushed another patchset up, which should hopefully address the
remaining review comments.

(There is a windows unittest error with this patchset, which I'll try to get to
the bottom of tomorrow, but this will probably not lead to any substantial
changes.)

https://codereview.chromium.org/2051333004/diff/420001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_remote_gatt_characteristic.cc (right):

https://codereview.chromium.org/2051333004/diff/420001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic.cc:46: void
BluetoothRemoteGattCharacteristic::StartNotifySession(
On 2016/08/03 16:55:42, ortuno wrote:
> On 2016/08/02 at 01:56:31, ortuno wrote:
> > On 2016/08/01 at 11:39:21, tommyt wrote:
> > > On 2016/07/28 21:59:30, ortuno wrote:
> > > > This looks very similar to what I first wrote for the notifications
queue
> in the
> > > > renderer and also what I started to do for the getPrimaryServices code.
I
> think
> > > > we can simplify this a bit by using closures so that we don't have to
> create to
> > > > a base class and two classes that implement it. What do you think of the
> > > > following code? It introduces a NotifySessionTask that takes a closure
and
> a
> > > > callback to be called when cancelling tasks. The rest of the code is
very
> > > > similar to what you have already.
> > > > 
> > > > 
> > > > NotifySessionTask(Closure task, ErrorCallback error_callback) 
> > > >   : task_(task),
> > > >     error_callback_(error_callback) {
> > > > }
> > > > 
> > > > ~NotifySessionTask() {}
> > > > 
> > > > NotifySessionTask::Run() {
> > > >   task.Run();
> > > > }
> > > > 
> > > > NotifySessionTask::Cancel() {
> > > >   error_callback_.Run(GATT_ERROR);
> > > > }
> > > > 
> > > > StartNotifySessionImpl(callback, error_callback) {
> > > >    if (IsNotifying()) {
> > > >     base::ThreadTaskRunnerHandle::Get()->PostTask(
> > > >         FROM_HERE, base::Bind(&OnSubscribeToNotificationsSuccess,
> > > >                               weak_ptr_factory_.GetWeakPtr(),
> > > >                               callback, error_callback));
> > > >     return;
> > > >   }
> > > >   SubscribeToNotifications(base::Bind(OnSubscribeToNotificationsSuccess,
> > > >                                       weak_ptr_factory_.GetWeakPtr(),
> > > >                                       callback),
> > > >                            base::Bind(OnSubscribetoNotificationsFailed,
> > > >                                       weak_ptr_factory_.GetWeakPtr(),
> > > >                                       error_callback));
> > > > }
> > > > 
> > > > // TODO: Implement StopNotifySessionImpl
> > > > 
> > > > 
> > > > OnSubscribeToNotificationsSuccess(NotifySessionCallback callback) {
> > > >   pending_notify_tasks_.pop();
> > > >   // TODO: make notify session.
> > > >   callback.Run(std::move(notify_session));
> > > >   
> > > >   if (!pending_notify_tasks_.empty()) {
> > > >     pending_notify_tasks_.front().Run();
> > > >   }
> > > > }
> > > > 
> > > > // TODO: Implement OnSubscribeToNotificationsSuccess,
> > > > OnSubscribeToNotificationsError,
> > > > // OnUnsubscribeToNotificationsSuccess and
> OnUnsubscribeToNotificationsError.
> > > > 
> > > > StartNotifySession(NotifySessionCallback callback, ErrorCallback,
> > > > error_callback) {
> > > >   pending_notify_tasks_.push(MakeUnique<NotifySessionTask>(
> > > >     base::Bind(StartNotifySessionImpl, this, callback, error_callback),
> > > > error_callback));
> > > >   if (pending_notify_tasks_size() == 1) {
> > > >     pending_notify_tasks_.front().Run();
> > > >   }
> > > > }
> > > > 
> > > > StopNotifySession(...) {
> > > >   pending_notify_tasks_.push(MakeUnique<NotifySessionTask>(
> > > >     base::Bind(StopNotifySessionImpl, callback, error_callback),
> > > > error_callback));
> > > >   if (pending_notify_tasks_size() == 1) {
> > > >     pending_notify_tasks_.fron().Run();
> > > >   }
> > > > }
> > > 
> > > I really like this approach, and I agree that the multiple Command classes
> is not super-elegant. The only problem is that the success callbacks for
> StartNotifySession and StopNotifySession are not the same type (The former
takes
> a session object as a parameter, the latter does not have any parameters). I
> think it might be easier to do this change at the same time as we move to
> creating session objects immediately in StartNotifySession without waiting for
> success or error. Once we make that change, the success callbacks can be plain
> Closures for both start and stop.
> > 
> > Hmm maybe I'm missing something, but I don't think the different callback
type
> makes a difference? You aren't storing the actual callback just a closure to
run
> StartNotifySessionImpl or StopNotifySessionImpl.
> 
> Ah I see what you mean now. You could still just save a closure with the error
> e.g.:
> 
> new NotifySessionTask(base::Bind(StartNotifySessionImpl,...),
> base::Bind(error_callback, GATT_ERROR));
> 
> And for Stop:
> 
> new NotifySessionTask(base::Bind(StopNotifySessionImpl,...), callback);

Done.

https://codereview.chromium.org/2051333004/diff/420001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic.cc:82: if (!hasNotify &&
!hasIndicate) {
On 2016/08/02 01:56:31, ortuno wrote:
> On 2016/08/01 at 11:39:21, tommyt wrote:
> > On 2016/07/28 21:59:30, ortuno wrote:
> > > I think you can move this logic into SubscribeToNotifications. See comment
> in
> > > SubscribeToNotifications.
> > 
> > Actually, I'll be moving more functionality into this function. This
function
> is cross-platform, and I think this is an ok place to keep all of this logic.
> 
> I'm a bit hesitant to put low level concepts like properties and descriptors
in
> a high level function that is meant to coordinate notify sessions.

This has been changed a bit in the latest changeset. This code  is now contained
in BluetoothRemoteGattCharacteristic::ExecuteStartNotifySession().

https://codereview.chromium.org/2051333004/diff/420001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_remote_gatt_characteristic.h (right):

https://codereview.chromium.org/2051333004/diff/420001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic.h:80: virtual bool
IsNotifying() const;
On 2016/08/02 01:56:31, ortuno wrote:
> On 2016/08/01 at 11:39:21, tommyt wrote:
> > On 2016/07/28 21:59:30, ortuno wrote:
> > > hmm this should really be private. I can't think of any use cases for it.
> There
> > > are two uses in the codebase: One of them is using it completely wrong and
> the
> > > other one is working around a bug that was fixed... OK to leave public for
> now
> > > but can you add the following:
> > > 
> > > // TODO(crbug.com/632476): Make private since there are no uses for this
> > > function.
> > 
> > Even though there are no good uses of this function at the moment, there
could
> be in the future. I feel that this function is a potentially useful and
> perfectly reasonable part of this class' public api.
> 
> I think this makes the API a bit confusing and doesn't really provide any
useful
> information to clients i.e. why would a client care if there are any other
> clients that have notify sessions? Anyway I don't feel too strongly about it,
> but I would feel better if the documentation is improved a bit. I mainly want
to
> avoid cases like this one: http://crrev.com/2189183002
> 

I've updated the documentation.

https://codereview.chromium.org/2051333004/diff/440001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc (right):

https://codereview.chromium.org/2051333004/diff/440001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc:1510:
TEST_F(BluetoothRemoteGattCharacteristicTest, StopNotifySession_StopStopStart) {
On 2016/08/05 14:47:00, ortuno wrote:
> On 2016/08/05 at 12:22:29, tommyt wrote:
> > On 2016/08/02 01:56:32, ortuno wrote:
> > > Another similar test but for StartNotifySession to make sure that a
success
> > > start doesn't cause other starts to jump ahead.
> > > 
> > > TEST_F(..., StartNotifySession_StartStopStart) {
> > >   // Make sure the order of calls is correct.
> > >   // Setup
> > >   
> > >   characteristic1_->StartNotifySession(...)
> > > 
> > >   // Start succeeds.
> > > 
> > >   characteristic1_->StartNotifySession(
> > >     base::Bind(BluetoothTest::NotifySessionCheckForPrecedingCalls,
> > >                base::Unretained(this),
> > >                1 /* num_of_preceding_calls */),
> > >     GetStartNotifyErrorCallback(...));
> > > 
> > >   notify_sessions_[0]->Stop(
> > >       base::Bind(BluetoothTest::NotifySessionCheckForPrecedingCalls,
> > >                  base::Unretained(this),
> > >                  2 /* num_of_preceding_calls */));
> > >           
> > >   characteristic1_->StartNotifySession(
> > >       base::Bind(BluetoothTest::NotifySessionCheckForPrecedingCalls,
> > >                  base::Unretained(this),
> > >                  3 /* num_of_preceding_calls */),
> > >       GetStartNotifyErrorCallback(...));
> > >    
> > >   // Simulate necessary events and run loop.
> > > 
> > >   EXPECT_FALSE(notify_sessions_[0]->IsActive());
> > >   EXPECT_TRUE(notify_sessions_[1]->IsActive());
> > >   EXPECT_TRUE(notify_sessions_[2]->IsActive());
> > > 
> > >   EXPECT_TRUE(characteristic1_->IsNotifying());
> > >   // EXPECT value of CCC descriptor is 1.
> > >   // EXPECT only one write to the CCC descriptor.
> > > 
> > > }
> > 
> > This test case would have to be added later, after we do the change where
> StartNotifySession returns a session object immediately. At the moment, it is
> not possible to queue up a start, stop, start, and then simulate all the
events
> (since the stop depends on the initial start having finished).
> 
> There are three StartNotifications calls. The stop is called on the session
that
> the first StartNotifications() call produced.

Right, this approach is possible. The latest patchset adds this unittest.

[commit-bot: I haz the power, 2016-08-08 15:40:09]

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

[commit-bot: I haz the power, 2016-08-08 15:40:10]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[tommyt, 2016-08-09 09:32:27]

The CQ bit was checked by tommyt@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-09 09:32:57]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-09 10:49:11]

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

[commit-bot: I haz the power, 2016-08-09 10:49:13]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[tommyt, 2016-08-09 13:15:16]

The CQ bit was checked by tommyt@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-09 13:15:37]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-09 13:49:53]

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

[commit-bot: I haz the power, 2016-08-09 13:49:54]

Dry run: This issue passed the CQ dry run.

[ortuno, 2016-08-09 21:57:56]

This should be the last set of comments :)

https://codereview.chromium.org/2051333004/diff/440001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc (right):

https://codereview.chromium.org/2051333004/diff/440001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc:1432:
characteristic1_->StartNotifySession(
On 2016/08/05 12:22:29, tommyt wrote:
> On 2016/08/02 01:56:32, ortuno wrote:
> > Same here. I think we need a way to test that callbacks are getting called
in
> > the right order.
> 
> Done.

I think you forgot to change this test :)

https://codereview.chromium.org/2051333004/diff/540001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_gatt_notify_session.cc (right):

https://codereview.chromium.org/2051333004/diff/540001/device/bluetooth/bluet...
device/bluetooth/bluetooth_gatt_notify_session.cc:45: callback.Run();
nit: post a task. Could you also add a test to check that this is called
asynchronously when the notify session has been stopped already?

https://codereview.chromium.org/2051333004/diff/540001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_remote_gatt_characteristic.cc (right):

https://codereview.chromium.org/2051333004/diff/540001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic.cc:67:
base::Unretained(this), callback, error_callback),
Unretained always raises some alarms. Either add a comment explaining why it's
safe to use Unretained or use a weakptr. This applies to all instances of
Unretained. You can add a comment at the top of the file explaining why it's
safe to use Unretained and just reference the comment where necessary.

https://codereview.chromium.org/2051333004/diff/540001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic.cc:103: Properties
properties = GetProperties();
optional nit: Would it make sense to put all this code that checks if
notifications are supported in a separate function? Maybe: std::pair<bool,
BluetoothRemoteGattService::GATT_ERROR> SupportsNotifyOrIndicate()?

https://codereview.chromium.org/2051333004/diff/540001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic.cc:127: #if
defined(OS_CHROMEOS) || defined(OS_LINUX) || defined(OS_MACOSX) || \
Could you add a TODO explaining what needs to be done to remove this #if?
Ideally pointing to an issue for each platform.

https://codereview.chromium.org/2051333004/diff/540001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic.cc:134: #else
nit: //  !(defined(OS_CHROMEOS) && defined(OS_LINUX) && defined(OS_MACOSX) &&
defined(OS_WIN))

https://codereview.chromium.org/2051333004/diff/540001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic.cc:158: #endif
nit: //  defined(OS_CHROMEOS) || defined(OS_LINUX) ...

https://codereview.chromium.org/2051333004/diff/540001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic.cc:200:
previous_command_type_ = NotifySessionCommand::COMMAND_START;
optional nit: Would passing these directly to Execute be cleaner?

https://codereview.chromium.org/2051333004/diff/540001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic.cc:215: callback);
nit: callback /* cancel_callback */);

https://codereview.chromium.org/2051333004/diff/540001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic.cc:224:
BluetoothGattNotifySession* session,
It makes me nervous that we are passing a pointer that could have been deleted.
Do you think it would make sense to keep a count of the sessions rather than a
map? To know if the session belongs in the characteristic you could pass the
session's characteristic identifier.

https://codereview.chromium.org/2051333004/diff/540001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic.cc:249:
std::vector<BluetoothRemoteGattDescriptor*> ccc_descriptor =
Do you need an "#if defined..." here?

https://codereview.chromium.org/2051333004/diff/540001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_remote_gatt_characteristic.h (right):

https://codereview.chromium.org/2051333004/diff/540001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic.h:196: void
StartNotifySessionSuccess(NotifySessionCallback callback);
nit: OnStartNotifySessionSuccess maybe? Same for the other ones.

[tommyt, 2016-08-10 12:05:07]

The CQ bit was checked by tommyt@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-10 12:05:17]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tommyt, 2016-08-10 12:07:19]

Another patchset is up, where I think I have addressed the remaining review
comments.

https://codereview.chromium.org/2051333004/diff/440001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc (right):

https://codereview.chromium.org/2051333004/diff/440001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc:1432:
characteristic1_->StartNotifySession(
On 2016/08/09 21:57:55, ortuno wrote:
> On 2016/08/05 12:22:29, tommyt wrote:
> > On 2016/08/02 01:56:32, ortuno wrote:
> > > Same here. I think we need a way to test that callbacks are getting called
> in
> > > the right order.
> > 
> > Done.
> 
> I think you forgot to change this test :)

Yes, or I did change it but I only added these tests to the Stop calls below. I
have fixed this properly in the latest patchset.

https://codereview.chromium.org/2051333004/diff/540001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_gatt_notify_session.cc (right):

https://codereview.chromium.org/2051333004/diff/540001/device/bluetooth/bluet...
device/bluetooth/bluetooth_gatt_notify_session.cc:45: callback.Run();
On 2016/08/09 21:57:55, ortuno wrote:
> nit: post a task. Could you also add a test to check that this is called
> asynchronously when the notify session has been stopped already?

Done. I've added the StopNotifySession_AfterDeleted unittest to test for this.

https://codereview.chromium.org/2051333004/diff/540001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_remote_gatt_characteristic.cc (right):

https://codereview.chromium.org/2051333004/diff/540001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic.cc:67:
base::Unretained(this), callback, error_callback),
On 2016/08/09 21:57:56, ortuno wrote:
> Unretained always raises some alarms. Either add a comment explaining why it's
> safe to use Unretained or use a weakptr. This applies to all instances of
> Unretained. You can add a comment at the top of the file explaining why it's
> safe to use Unretained and just reference the comment where necessary.

I've replaced all instances of base::Unretained(this) with GetWeakPtr().

https://codereview.chromium.org/2051333004/diff/540001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic.cc:103: Properties
properties = GetProperties();
On 2016/08/09 21:57:56, ortuno wrote:
> optional nit: Would it make sense to put all this code that checks if
> notifications are supported in a separate function? Maybe: std::pair<bool,
> BluetoothRemoteGattService::GATT_ERROR> SupportsNotifyOrIndicate()?

Personally, I only think there is an advantage to doing this if we need to check
this many places in the code. If that's not the case, I actually prefer to have
all the code in one place, as I think it makes it easier to see what happens.

In the latest patchset I have instead added a comment to each section of this
function, to explain what is done.

https://codereview.chromium.org/2051333004/diff/540001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic.cc:127: #if
defined(OS_CHROMEOS) || defined(OS_LINUX) || defined(OS_MACOSX) || \
On 2016/08/09 21:57:56, ortuno wrote:
> Could you add a TODO explaining what needs to be done to remove this #if?
> Ideally pointing to an issue for each platform.

Done

https://codereview.chromium.org/2051333004/diff/540001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic.cc:134: #else
On 2016/08/09 21:57:56, ortuno wrote:
> nit: //  !(defined(OS_CHROMEOS) && defined(OS_LINUX) && defined(OS_MACOSX) &&
> defined(OS_WIN))

Done

https://codereview.chromium.org/2051333004/diff/540001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic.cc:158: #endif
On 2016/08/09 21:57:56, ortuno wrote:
> nit: //  defined(OS_CHROMEOS) || defined(OS_LINUX) ...

Done

https://codereview.chromium.org/2051333004/diff/540001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic.cc:200:
previous_command_type_ = NotifySessionCommand::COMMAND_START;
On 2016/08/09 21:57:56, ortuno wrote:
> optional nit: Would passing these directly to Execute be cleaner?

Maybe. I've done this in my latest patchset, and I think I like this a bit
better.

https://codereview.chromium.org/2051333004/diff/540001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic.cc:215: callback);
On 2016/08/09 21:57:56, ortuno wrote:
> nit: callback /* cancel_callback */);

Done.

https://codereview.chromium.org/2051333004/diff/540001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic.cc:224:
BluetoothGattNotifySession* session,
On 2016/08/09 21:57:56, ortuno wrote:
> It makes me nervous that we are passing a pointer that could have been
deleted.
> Do you think it would make sense to keep a count of the sessions rather than a
> map? To know if the session belongs in the characteristic you could pass the
> session's characteristic identifier.

This session pointer is not dereferenced anywhere, it's just used as a key in
the notify_sessions_ set. Passing the characteristic identifier is not enough,
since we need to be able to remove the pointer from the notify_sessions_ set
when we're done.

The current code should be safe, seeing that we're not dereferencing these
pointers, but I do agree that keeping these raw pointers around is a bit like
leaving a loaded gun under the pillow. For now, I've added the unittest
StopNotifySession_SessionDeleted2 to test that deleting the sessions with
outstanding stop callbacks does not cause problems.

If we want to make this safer, then we may have to start using weak pointers to
the sessions. Some sort of a session ID that would uniquely identify a session
would probably also work.

https://codereview.chromium.org/2051333004/diff/540001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic.cc:249:
std::vector<BluetoothRemoteGattDescriptor*> ccc_descriptor =
On 2016/08/09 21:57:56, ortuno wrote:
> Do you need an "#if defined..." here?

Done.

https://codereview.chromium.org/2051333004/diff/540001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_remote_gatt_characteristic.h (right):

https://codereview.chromium.org/2051333004/diff/540001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic.h:196: void
StartNotifySessionSuccess(NotifySessionCallback callback);
On 2016/08/09 21:57:56, ortuno wrote:
> nit: OnStartNotifySessionSuccess maybe? Same for the other ones.

Done.

[commit-bot: I haz the power, 2016-08-10 13:50:29]

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

[commit-bot: I haz the power, 2016-08-10 13:50:31]

Dry run: This issue passed the CQ dry run.

[ortuno, 2016-08-10 17:21:00]

lgtm! Hopefully I didn't drive you crazy with all the comments :)

[tommyt, 2016-08-11 08:48:25]

The CQ bit was checked by tommyt@opera.com

[commit-bot: I haz the power, 2016-08-11 08:48:39]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-11 08:55:10]

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

[commit-bot: I haz the power, 2016-08-11 08:55:12]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[tommyt, 2016-08-11 09:12:53]

tommyt@opera.com changed reviewers:
+ msarda@chromium.org, rkc@chromium.org

[tommyt, 2016-08-11 09:16:10]

I have added two more reviewers to this commit, as I did not have owner coverage
for all files. The diffs for the missing files are very simple, though;


rkc: Would you mind reviewing the following file, for which you are the owner?

chrome/browser/extensions/api/bluetooth_low_energy/bluetooth_low_energy_apitest.cc


msarda: Would you please review the following two files?

components/proximity_auth/ble/bluetooth_low_energy_connection_unittest.cc

components/proximity_auth/ble/bluetooth_low_energy_weave_client_connection_unittest.cc

[tommyt, 2016-08-11 09:38:04]

Description was changed from

==========
Implement BluetoothGattNotifySession::Stop on Android

This change enables the stopNotifications function for remote GATT
characteristics on Android.

BUG=584370
==========

to

==========
Implement BluetoothGattNotifySession::Stop on Android

This change enables the stopNotifications function for remote GATT
characteristics on Android. In order to do so, two distinct refactorings have
been done;

1) The session classes have been simplified so that platform specific versions
are no longer required. All code that previously used
BluetoothGattNotifySession[Mac|Win|BlueZ] now simply use the base
BluetoothGattNotifySession class.

2) BluetoothRemoteGattCharacteristic::StartNotifySession and StopNotifySession
have been rewritten so that all the code for keeping track of the different
session objects and the notification state sits in the base class. Inheriting
classes should no longer override these two functions, but instead override the
much simpler SubscribeToNotifications and UnsubscribeFromNotifications.

BUG=584370
==========

[rkc, 2016-08-15 22:54:44]

c/b/e/api/bluetooth_low_energy/bluetooth_low_energy_apitest.cc lgtm

https://codereview.chromium.org/2051333004/diff/600001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_gatt_notify_session.cc (right):

https://codereview.chromium.org/2051333004/diff/600001/device/bluetooth/bluet...
device/bluetooth/bluetooth_gatt_notify_session.cc:7: #include "base/bind.h"
nit: #include "base/bind_helpers.h"?

[tommyt, 2016-08-16 07:46:49]

https://codereview.chromium.org/2051333004/diff/600001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_gatt_notify_session.cc (right):

https://codereview.chromium.org/2051333004/diff/600001/device/bluetooth/bluet...
device/bluetooth/bluetooth_gatt_notify_session.cc:7: #include "base/bind.h"
On 2016/08/15 22:54:44, rkc wrote:
> nit: #include "base/bind_helpers.h"?

I could certainly include that file, but may I ask why?

[rkc, 2016-08-16 07:48:08]

https://codereview.chromium.org/2051333004/diff/600001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_gatt_notify_session.cc (right):

https://codereview.chromium.org/2051333004/diff/600001/device/bluetooth/bluet...
device/bluetooth/bluetooth_gatt_notify_session.cc:7: #include "base/bind.h"
On 2016/08/16 07:46:49, tommyt wrote:
> On 2016/08/15 22:54:44, rkc wrote:
> > nit: #include "base/bind_helpers.h"?
> 
> I could certainly include that file, but may I ask why?

base::DoNothing

[tommyt, 2016-08-16 07:54:09]

https://codereview.chromium.org/2051333004/diff/600001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_gatt_notify_session.cc (right):

https://codereview.chromium.org/2051333004/diff/600001/device/bluetooth/bluet...
device/bluetooth/bluetooth_gatt_notify_session.cc:7: #include "base/bind.h"
On 2016/08/16 07:48:08, rkc wrote:
> On 2016/08/16 07:46:49, tommyt wrote:
> > On 2016/08/15 22:54:44, rkc wrote:
> > > nit: #include "base/bind_helpers.h"?
> > 
> > I could certainly include that file, but may I ask why?
> 
> base::DoNothing

Aha, that does make sense. I will add this line in a new patchset.

[tommyt, 2016-08-16 09:27:04]

The CQ bit was checked by tommyt@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-16 09:27:13]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tommyt, 2016-08-16 09:43:11]

ping?

I'm still missing a review for bluetooth_low_energy_connection_unittest.cc and
bluetooth_low_energy_weave_client_connection_unittest.cc.

msarda: Are you able to take a quick look at these files?

[commit-bot: I haz the power, 2016-08-16 13:09:16]

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

[commit-bot: I haz the power, 2016-08-16 13:09:18]

Dry run: This issue passed the CQ dry run.

[tommyt, 2016-08-17 07:52:47]

tommyt@opera.com changed reviewers:
+ sacomoto@chromium.org

[sacomoto, 2016-08-17 08:44:51]

LGTM for bluetooth_low_energy_connection_unittest.cc and
bluetooth_low_energy_weave_client_connection_unittest.cc.

[tommyt, 2016-08-17 08:59:57]

On 2016/08/17 08:44:51, sacomoto wrote:
> LGTM for bluetooth_low_energy_connection_unittest.cc and
> bluetooth_low_energy_weave_client_connection_unittest.cc.

Thank you sacomoto! :)

[tommyt, 2016-08-17 09:00:16]

The CQ bit was checked by tommyt@opera.com

[tommyt, 2016-08-17 09:00:17]

The patchset sent to the CQ was uploaded after l-g-t-m from ortuno@chromium.org,
rkc@chromium.org
Link to the patchset: https://codereview.chromium.org/2051333004/#ps620001
(title: "Add a missing header file")

[commit-bot: I haz the power, 2016-08-17 09:00:27]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-17 09:06:07]

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

[commit-bot: I haz the power, 2016-08-17 09:06:09]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[tommyt, 2016-08-17 09:16:58]

The CQ bit was checked by tommyt@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-17 09:17:18]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-17 11:37:58]

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

[commit-bot: I haz the power, 2016-08-17 11:37:59]

Dry run: This issue passed the CQ dry run.

[tommyt, 2016-08-17 11:39:01]

The CQ bit was checked by tommyt@opera.com

[tommyt, 2016-08-17 11:39:01]

The patchset sent to the CQ was uploaded after l-g-t-m from rkc@chromium.org,
ortuno@chromium.org, sacomoto@chromium.org
Link to the patchset: https://codereview.chromium.org/2051333004/#ps640001
(title: "Rebase and solve conflicts")

[commit-bot: I haz the power, 2016-08-17 11:39:10]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-17 11:43:36]

Description was changed from

==========
Implement BluetoothGattNotifySession::Stop on Android

This change enables the stopNotifications function for remote GATT
characteristics on Android. In order to do so, two distinct refactorings have
been done;

1) The session classes have been simplified so that platform specific versions
are no longer required. All code that previously used
BluetoothGattNotifySession[Mac|Win|BlueZ] now simply use the base
BluetoothGattNotifySession class.

2) BluetoothRemoteGattCharacteristic::StartNotifySession and StopNotifySession
have been rewritten so that all the code for keeping track of the different
session objects and the notification state sits in the base class. Inheriting
classes should no longer override these two functions, but instead override the
much simpler SubscribeToNotifications and UnsubscribeFromNotifications.

BUG=584370
==========

to

==========
Implement BluetoothGattNotifySession::Stop on Android

This change enables the stopNotifications function for remote GATT
characteristics on Android. In order to do so, two distinct refactorings have
been done;

1) The session classes have been simplified so that platform specific versions
are no longer required. All code that previously used
BluetoothGattNotifySession[Mac|Win|BlueZ] now simply use the base
BluetoothGattNotifySession class.

2) BluetoothRemoteGattCharacteristic::StartNotifySession and StopNotifySession
have been rewritten so that all the code for keeping track of the different
session objects and the notification state sits in the base class. Inheriting
classes should no longer override these two functions, but instead override the
much simpler SubscribeToNotifications and UnsubscribeFromNotifications.

BUG=584370
==========

[commit-bot: I haz the power, 2016-08-17 11:43:38]

Committed patchset #33 (id:640001)

[commit-bot: I haz the power, 2016-08-17 11:45:05]

Description was changed from

==========
Implement BluetoothGattNotifySession::Stop on Android

This change enables the stopNotifications function for remote GATT
characteristics on Android. In order to do so, two distinct refactorings have
been done;

1) The session classes have been simplified so that platform specific versions
are no longer required. All code that previously used
BluetoothGattNotifySession[Mac|Win|BlueZ] now simply use the base
BluetoothGattNotifySession class.

2) BluetoothRemoteGattCharacteristic::StartNotifySession and StopNotifySession
have been rewritten so that all the code for keeping track of the different
session objects and the notification state sits in the base class. Inheriting
classes should no longer override these two functions, but instead override the
much simpler SubscribeToNotifications and UnsubscribeFromNotifications.

BUG=584370
==========

to

==========
Implement BluetoothGattNotifySession::Stop on Android

This change enables the stopNotifications function for remote GATT
characteristics on Android. In order to do so, two distinct refactorings have
been done;

1) The session classes have been simplified so that platform specific versions
are no longer required. All code that previously used
BluetoothGattNotifySession[Mac|Win|BlueZ] now simply use the base
BluetoothGattNotifySession class.

2) BluetoothRemoteGattCharacteristic::StartNotifySession and StopNotifySession
have been rewritten so that all the code for keeping track of the different
session objects and the notification state sits in the base class. Inheriting
classes should no longer override these two functions, but instead override the
much simpler SubscribeToNotifications and UnsubscribeFromNotifications.

BUG=584370

Committed: https://crrev.com/d41af3adfd8500c81c827ab03ed2736909cf1ee8
Cr-Commit-Position: refs/heads/master@{#412498}
==========

[commit-bot: I haz the power, 2016-08-17 11:45:07]

Patchset 33 (id:??) landed as
https://crrev.com/d41af3adfd8500c81c827ab03ed2736909cf1ee8
Cr-Commit-Position: refs/heads/master@{#412498}

[Guido Urdaneta, 2016-08-17 16:38:42]

A revert of this CL (patchset #33 id:640001) has been created in
https://codereview.chromium.org/2241263005/ by guidou@chromium.org.

The reason for reverting is: Suspect of breaking
https://build.chromium.org/p/chromium.mac/builders/Mac10.11%20Tests/builds/2450

[345/345] BluetoothRemoteGattCharacteristicTest.GattCharacteristicValueChanged
(TIMED OUT)
6 tests timed out:
    BluetoothRemoteGattCharacteristicTest.GattCharacteristicValueChanged
(../../device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc:1931)
    BluetoothRemoteGattCharacteristicTest.StartNotifySession
(../../device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc:1026)
    BluetoothRemoteGattCharacteristicTest.StartNotifySession_Multiple
(../../device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc:1067)
    BluetoothRemoteGattCharacteristicTest.StartNotifySession_OnIndicate
(../../device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc:1039)
    BluetoothRemoteGattCharacteristicTest.StartNotifySession_OnNotifyAndIndicate
(../../device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc:1054)
   
BluetoothRemoteGattCharacteristicTest.StartNotifySession_Reentrant_Success_Success
(../../device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc:1217).

[tommyt, 2016-08-18 07:08:49]

Description was changed from

==========
Implement BluetoothGattNotifySession::Stop on Android

This change enables the stopNotifications function for remote GATT
characteristics on Android. In order to do so, two distinct refactorings have
been done;

1) The session classes have been simplified so that platform specific versions
are no longer required. All code that previously used
BluetoothGattNotifySession[Mac|Win|BlueZ] now simply use the base
BluetoothGattNotifySession class.

2) BluetoothRemoteGattCharacteristic::StartNotifySession and StopNotifySession
have been rewritten so that all the code for keeping track of the different
session objects and the notification state sits in the base class. Inheriting
classes should no longer override these two functions, but instead override the
much simpler SubscribeToNotifications and UnsubscribeFromNotifications.

BUG=584370

Committed: https://crrev.com/d41af3adfd8500c81c827ab03ed2736909cf1ee8
Cr-Commit-Position: refs/heads/master@{#412498}
==========

to

==========
Implement BluetoothGattNotifySession::Stop on Android

This change enables the stopNotifications function for remote GATT
characteristics on Android. In order to do so, two distinct refactorings have
been done;

1) The session classes have been simplified so that platform specific versions
are no longer required. All code that previously used
BluetoothGattNotifySession[Mac|Win|BlueZ] now simply use the base
BluetoothGattNotifySession class.

2) BluetoothRemoteGattCharacteristic::StartNotifySession and StopNotifySession
have been rewritten so that all the code for keeping track of the different
session objects and the notification state sits in the base class. Inheriting
classes should no longer override these two functions, but instead override the
much simpler SubscribeToNotifications and UnsubscribeFromNotifications.

BUG=584370

Committed: https://crrev.com/d41af3adfd8500c81c827ab03ed2736909cf1ee8
Cr-Commit-Position: refs/heads/master@{#412498}
==========

[tommyt, 2016-08-19 11:12:35]

The CQ bit was checked by tommyt@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-19 11:12:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tommyt, 2016-08-19 11:20:42]

The CQ bit was checked by tommyt@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-19 11:21:05]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-19 11:49:15]

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

[commit-bot: I haz the power, 2016-08-19 11:49:18]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[tommyt, 2016-08-19 11:50:08]

The CQ bit was checked by tommyt@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-19 11:50:37]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-19 12:00:41]

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

[commit-bot: I haz the power, 2016-08-19 12:00:43]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[tommyt, 2016-08-19 12:26:39]

Description was changed from

==========
Implement BluetoothGattNotifySession::Stop on Android

This change enables the stopNotifications function for remote GATT
characteristics on Android. In order to do so, two distinct refactorings have
been done;

1) The session classes have been simplified so that platform specific versions
are no longer required. All code that previously used
BluetoothGattNotifySession[Mac|Win|BlueZ] now simply use the base
BluetoothGattNotifySession class.

2) BluetoothRemoteGattCharacteristic::StartNotifySession and StopNotifySession
have been rewritten so that all the code for keeping track of the different
session objects and the notification state sits in the base class. Inheriting
classes should no longer override these two functions, but instead override the
much simpler SubscribeToNotifications and UnsubscribeFromNotifications.

BUG=584370

Committed: https://crrev.com/d41af3adfd8500c81c827ab03ed2736909cf1ee8
Cr-Commit-Position: refs/heads/master@{#412498}
==========

to

==========
Implement BluetoothGattNotifySession::Stop on Android, 2nd attempt

This change enables the stopNotifications function for remote GATT
characteristics on Android. In order to do so, two distinct refactorings have
been done;

1) The session classes have been simplified so that platform specific versions
are no longer required. All code that previously used
BluetoothGattNotifySession[Mac|Win|BlueZ] now simply use the base
BluetoothGattNotifySession class.

2) BluetoothRemoteGattCharacteristic::StartNotifySession and StopNotifySession
have been rewritten so that all the code for keeping track of the different
session objects and the notification state sits in the base class. Inheriting
classes should no longer override these two functions, but instead override the
much simpler SubscribeToNotifications and UnsubscribeFromNotifications.

BUG=584370

Committed: https://crrev.com/d41af3adfd8500c81c827ab03ed2736909cf1ee8
Cr-Commit-Position: refs/heads/master@{#412498}
==========

[tommyt, 2016-08-19 13:10:23]

A new patchset is ready, which contains a fix for the timeout problems on
Mac10.11. The fix consists of adding a stub for the StopNotifySession function
on mac.

[ortuno, 2016-08-19 19:13:27]

https://codereview.chromium.org/2051333004/diff/700001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_remote_gatt_characteristic_mac.mm (right):

https://codereview.chromium.org/2051333004/diff/700001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic_mac.mm:170:
NOTIMPLEMENTED();
Could you point to an issue to implement this?

https://codereview.chromium.org/2051333004/diff/700001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_remote_gatt_characteristic_win.cc (right):

https://codereview.chromium.org/2051333004/diff/700001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic_win.cc:204:
callback.Run();
Why not have it post a task as well?

[tommyt, 2016-08-22 06:09:43]

The CQ bit was checked by tommyt@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-22 06:10:12]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tommyt, 2016-08-22 06:34:12]

https://codereview.chromium.org/2051333004/diff/700001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_remote_gatt_characteristic_mac.mm (right):

https://codereview.chromium.org/2051333004/diff/700001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic_mac.mm:170:
NOTIMPLEMENTED();
On 2016/08/19 19:13:27, ortuno wrote:
> Could you point to an issue to implement this?

Done.

https://codereview.chromium.org/2051333004/diff/700001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_remote_gatt_characteristic_win.cc (right):

https://codereview.chromium.org/2051333004/diff/700001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic_win.cc:204:
callback.Run();
On 2016/08/19 19:13:27, ortuno wrote:
> Why not have it post a task as well?

Done.

[commit-bot: I haz the power, 2016-08-22 07:49:09]

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

[commit-bot: I haz the power, 2016-08-22 07:49:10]

Dry run: This issue passed the CQ dry run.

[tommyt, 2016-08-22 08:05:14]

The CQ bit was checked by tommyt@opera.com

[tommyt, 2016-08-22 08:05:15]

The patchset sent to the CQ was uploaded after l-g-t-m from rkc@chromium.org,
ortuno@chromium.org, sacomoto@chromium.org
Link to the patchset: https://codereview.chromium.org/2051333004/#ps720001
(title: "Address new review comments")

[commit-bot: I haz the power, 2016-08-22 08:05:28]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-22 08:10:05]

Description was changed from

==========
Implement BluetoothGattNotifySession::Stop on Android, 2nd attempt

This change enables the stopNotifications function for remote GATT
characteristics on Android. In order to do so, two distinct refactorings have
been done;

1) The session classes have been simplified so that platform specific versions
are no longer required. All code that previously used
BluetoothGattNotifySession[Mac|Win|BlueZ] now simply use the base
BluetoothGattNotifySession class.

2) BluetoothRemoteGattCharacteristic::StartNotifySession and StopNotifySession
have been rewritten so that all the code for keeping track of the different
session objects and the notification state sits in the base class. Inheriting
classes should no longer override these two functions, but instead override the
much simpler SubscribeToNotifications and UnsubscribeFromNotifications.

BUG=584370

Committed: https://crrev.com/d41af3adfd8500c81c827ab03ed2736909cf1ee8
Cr-Commit-Position: refs/heads/master@{#412498}
==========

to

==========
Implement BluetoothGattNotifySession::Stop on Android, 2nd attempt

This change enables the stopNotifications function for remote GATT
characteristics on Android. In order to do so, two distinct refactorings have
been done;

1) The session classes have been simplified so that platform specific versions
are no longer required. All code that previously used
BluetoothGattNotifySession[Mac|Win|BlueZ] now simply use the base
BluetoothGattNotifySession class.

2) BluetoothRemoteGattCharacteristic::StartNotifySession and StopNotifySession
have been rewritten so that all the code for keeping track of the different
session objects and the notification state sits in the base class. Inheriting
classes should no longer override these two functions, but instead override the
much simpler SubscribeToNotifications and UnsubscribeFromNotifications.

BUG=584370

Committed: https://crrev.com/d41af3adfd8500c81c827ab03ed2736909cf1ee8
Cr-Commit-Position: refs/heads/master@{#412498}
==========

[commit-bot: I haz the power, 2016-08-22 08:10:08]

Committed patchset #37 (id:720001)

[commit-bot: I haz the power, 2016-08-22 08:11:46]

Description was changed from

==========
Implement BluetoothGattNotifySession::Stop on Android, 2nd attempt

This change enables the stopNotifications function for remote GATT
characteristics on Android. In order to do so, two distinct refactorings have
been done;

1) The session classes have been simplified so that platform specific versions
are no longer required. All code that previously used
BluetoothGattNotifySession[Mac|Win|BlueZ] now simply use the base
BluetoothGattNotifySession class.

2) BluetoothRemoteGattCharacteristic::StartNotifySession and StopNotifySession
have been rewritten so that all the code for keeping track of the different
session objects and the notification state sits in the base class. Inheriting
classes should no longer override these two functions, but instead override the
much simpler SubscribeToNotifications and UnsubscribeFromNotifications.

BUG=584370

Committed: https://crrev.com/d41af3adfd8500c81c827ab03ed2736909cf1ee8
Cr-Commit-Position: refs/heads/master@{#412498}
==========

to

==========
Implement BluetoothGattNotifySession::Stop on Android, 2nd attempt

This change enables the stopNotifications function for remote GATT
characteristics on Android. In order to do so, two distinct refactorings have
been done;

1) The session classes have been simplified so that platform specific versions
are no longer required. All code that previously used
BluetoothGattNotifySession[Mac|Win|BlueZ] now simply use the base
BluetoothGattNotifySession class.

2) BluetoothRemoteGattCharacteristic::StartNotifySession and StopNotifySession
have been rewritten so that all the code for keeping track of the different
session objects and the notification state sits in the base class. Inheriting
classes should no longer override these two functions, but instead override the
much simpler SubscribeToNotifications and UnsubscribeFromNotifications.

BUG=584370

Committed: https://crrev.com/d41af3adfd8500c81c827ab03ed2736909cf1ee8
Committed: https://crrev.com/ad8462b6930b45ec778dc9e3fcedfc74680f964d
Cr-Original-Commit-Position: refs/heads/master@{#412498}
Cr-Commit-Position: refs/heads/master@{#413417}
==========

[commit-bot: I haz the power, 2016-08-22 08:11:49]

Patchset 37 (id:??) landed as
https://crrev.com/ad8462b6930b45ec778dc9e3fcedfc74680f964d
Cr-Commit-Position: refs/heads/master@{#413417}