Remove TODOs that are out of date.

We will not update the cached characteristic value when writing, nor will we call the observers' GattCharacteristicValueChanged method.

Update the documentation for WriteRemoteCharacteristic and change the name of the "new_value" parameter to "value".

See http://crbug.com/614534

BUG=551634
Committed: https://crrev.com/73e0d4d0673f5b84699da8e4b07bd40ef7c83fce
Cr-Commit-Position: refs/heads/master@{#416028}

[tommyt, 2016-08-29 08:58:07]

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

[tommyt, 2016-08-29 08:58:07]

This change addresses the final remaining TODO for http://crbug.com/551634.

Please take a look.

[tommyt, 2016-08-29 08:58:18]

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

[commit-bot: I haz the power, 2016-08-29 08:58:39]

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-29 10:10:18]

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

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

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-29 11:26:20]

Description was changed from

==========
Update the characteristic value upon successful write.

A successful write operation now causes the value_ field of the remote
characteristic to be updated with the written value, and all observers
to be notified of the change.

BUG=551634
==========

to

==========
Update the characteristic value upon successful write.

A successful write operation now causes the value_ field of the remote
characteristic to be updated with the written value, and all observers
to be notified of the change.

BUG=551634
==========

[ortuno, 2016-08-29 16:02:53]

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

[ortuno, 2016-08-29 16:02:53]

https://codereview.chromium.org/2287273002/diff/1/device/bluetooth/bluetooth_...
File device/bluetooth/bluetooth_remote_gatt_characteristic_android.cc (right):

https://codereview.chromium.org/2287273002/diff/1/device/bluetooth/bluetooth_...
device/bluetooth/bluetooth_remote_gatt_characteristic_android.cc:218:
adapter_->NotifyGattCharacteristicValueChanged(this, value_);
I don't think we want to do this, see http://crbug.com/614534#c1

[tommyt, 2016-08-29 17:44:36]

https://codereview.chromium.org/2287273002/diff/1/device/bluetooth/bluetooth_...
File device/bluetooth/bluetooth_remote_gatt_characteristic_android.cc (right):

https://codereview.chromium.org/2287273002/diff/1/device/bluetooth/bluetooth_...
device/bluetooth/bluetooth_remote_gatt_characteristic_android.cc:218:
adapter_->NotifyGattCharacteristicValueChanged(this, value_);
On 2016/08/29 16:02:53, ortuno wrote:
> I don't think we want to do this, see http://crbug.com/614534#c1

If we don't want to do this, what is the intention of the two TODOs that I've
removed with this change? Are they no longer relevant/wanted? If so, I can
upload a patch that only removes the TODOs and makes no other change to the
code.

[ortuno, 2016-08-29 18:05:23]

https://codereview.chromium.org/2287273002/diff/1/device/bluetooth/bluetooth_...
File device/bluetooth/bluetooth_remote_gatt_characteristic_android.cc (right):

https://codereview.chromium.org/2287273002/diff/1/device/bluetooth/bluetooth_...
device/bluetooth/bluetooth_remote_gatt_characteristic_android.cc:218:
adapter_->NotifyGattCharacteristicValueChanged(this, value_);
On 2016/08/29 at 17:44:36, tommyt wrote:
> On 2016/08/29 16:02:53, ortuno wrote:
> > I don't think we want to do this, see http://crbug.com/614534#c1
> 
> If we don't want to do this, what is the intention of the two TODOs that I've
removed with this change? Are they no longer relevant/wanted? If so, I can
upload a patch that only removes the TODOs and makes no other change to the
code.

I believe the first one is out of date. The patch that added that TODO didn't
call NotifyGattCharacteristicValueChanged once notifications were enabled:
http://crrev.com/1422463012

Regarding this TODO. I would just remove it. The current implementation matches
the bluez and mac implementations so there isn't really anything left to do. We
have http://crbug.com/614534 remind us to decide if we want to change the API to
update value on writes. scheib: WDYT?

[scheib, 2016-08-29 22:36:39]

https://codereview.chromium.org/2287273002/diff/1/device/bluetooth/bluetooth_...
File device/bluetooth/bluetooth_remote_gatt_characteristic_android.cc (right):

https://codereview.chromium.org/2287273002/diff/1/device/bluetooth/bluetooth_...
device/bluetooth/bluetooth_remote_gatt_characteristic_android.cc:218:
adapter_->NotifyGattCharacteristicValueChanged(this, value_);
On 2016/08/29 18:05:23, ortuno wrote:
> On 2016/08/29 at 17:44:36, tommyt wrote:
> > On 2016/08/29 16:02:53, ortuno wrote:
> > > I don't think we want to do this, see http://crbug.com/614534#c1
> > 
> > If we don't want to do this, what is the intention of the two TODOs that
I've
> removed with this change? Are they no longer relevant/wanted? If so, I can
> upload a patch that only removes the TODOs and makes no other change to the
> code.
> 
> I believe the first one is out of date. The patch that added that TODO didn't
> call NotifyGattCharacteristicValueChanged once notifications were enabled:
> http://crrev.com/1422463012
> 
> Regarding this TODO. I would just remove it. The current implementation
matches
> the bluez and mac implementations so there isn't really anything left to do.
We
> have http://crbug.com/614534 remind us to decide if we want to change the API
to
> update value on writes. scheib: WDYT?

I agree, let's not update the value cache. tommyt, would you take a look at
https://bugs.chromium.org/p/chromium/issues/detail?id=614534#c2 to update the
comments and parameter name as well to clean up the confusion? Perhaps even
explicitly call out that write doesn't cache the value?

[tommyt, 2016-08-30 08:11:42]

Description was changed from

==========
Update the characteristic value upon successful write.

A successful write operation now causes the value_ field of the remote
characteristic to be updated with the written value, and all observers
to be notified of the change.

BUG=551634
==========

to

==========
We will not update the cached characteristic value when writing, nor will we
call the observers' GattCharacteristicValueChanged method.

Update the documentation for WriteRemoteCharacteristic and change the name of
the "new_value" parameter to "value".

See http://crbug.com/614534

BUG=551634
==========

[tommyt, 2016-08-30 08:17:17]

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

[commit-bot: I haz the power, 2016-08-30 08:17:42]

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-30 08:41:08]

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

[commit-bot: I haz the power, 2016-08-30 08:41:08]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[tommyt, 2016-08-30 08:45:35]

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

[commit-bot: I haz the power, 2016-08-30 08:45: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-08-30 11:49:24]

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

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

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

[tommyt, 2016-08-31 07:02:43]

I think the latest patchset addresses your comments.

Today is my last day in the office before going on vacation, so if you are happy
with this change, maybe you can commit it and close this bug:
https://bugs.chromium.org/p/chromium/issues/detail?id=551634 ?

If you're not happy with these changes, feel free to take over in any way you
wish. Or leave it until I'm back on the 26th.

[scheib, 2016-08-31 15:36:50]

Thanks, LGTM, I'll wait a few hours to see if ortuno chimes in though he's
supposed to be out of office ;) , then I'll commit queue.

[ortuno, 2016-09-01 15:58:13]

lgtm

[scheib, 2016-09-01 16:54:43]

The CQ bit was checked by scheib@chromium.org

[commit-bot: I haz the power, 2016-09-01 16:55:04]

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-09-01 20:06:46]

Description was changed from

==========
We will not update the cached characteristic value when writing, nor will we
call the observers' GattCharacteristicValueChanged method.

Update the documentation for WriteRemoteCharacteristic and change the name of
the "new_value" parameter to "value".

See http://crbug.com/614534

BUG=551634
==========

to

==========
We will not update the cached characteristic value when writing, nor will we
call the observers' GattCharacteristicValueChanged method.

Update the documentation for WriteRemoteCharacteristic and change the name of
the "new_value" parameter to "value".

See http://crbug.com/614534

BUG=551634
==========

[commit-bot: I haz the power, 2016-09-01 20:06:48]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-09-01 20:08:50]

Description was changed from

==========
We will not update the cached characteristic value when writing, nor will we
call the observers' GattCharacteristicValueChanged method.

Update the documentation for WriteRemoteCharacteristic and change the name of
the "new_value" parameter to "value".

See http://crbug.com/614534

BUG=551634
==========

to

==========
We will not update the cached characteristic value when writing, nor will we
call the observers' GattCharacteristicValueChanged method.

Update the documentation for WriteRemoteCharacteristic and change the name of
the "new_value" parameter to "value".

See http://crbug.com/614534

BUG=551634

Committed: https://crrev.com/73e0d4d0673f5b84699da8e4b07bd40ef7c83fce
Cr-Commit-Position: refs/heads/master@{#416028}
==========

[commit-bot: I haz the power, 2016-09-01 20:08:51]

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