bluetooth: Update BluetoothCharacteristic.value on readValue

bluetooth: Update BluetoothCharacteristic.value on readValue

BUG=575577
Committed: https://crrev.com/e09e5da1d5b9f11e7955bdb6a4517e9e5da30164
Cr-Commit-Position: refs/heads/master@{#369995}

[fbeaufortchromium, 2016-01-12 09:20:23]

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

[fbeaufortchromium, 2016-01-12 09:20:24]

Hey Giovanni, can you have a look at this CL?

[ortuno, 2016-01-12 19:26:47]

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

[ortuno, 2016-01-12 19:26:48]

Nice! I'm adding yhirano as a reviewer to see if this is the right pattern. If
so this pattern will make implementing similar functions much easier.

[yhirano, 2016-01-13 09:10:23]

Looks good.
I generally prefer using CallbackPromiseAdapter over using WebCallbacks
directly, but in this case the callback does additional things so it's
reasonable to use WebCallbacks.

[ortuno, 2016-01-13 21:14:09]

One small issue. Thanks for doing this by the way!

https://codereview.chromium.org/1573183002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/bluetooth/BluetoothGATTCharacteristic.cpp
(right):

https://codereview.chromium.org/1573183002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/bluetooth/BluetoothGATTCharacteristic.cpp:119:
Persistent<BluetoothGATTCharacteristic> m_webCharacteristic;
Talked to jyasskin about this. We don't need to keep an object alive longer than
necessary so we should use WeakPersistent instead. That would mean you need to
add a null check before resolving the promise:

RefPtr<DOMArrayBuffer> domBuffer = ConvertWebVectorToArrayBuffer(value);
if (m_webCharacteristic != nullptr) {
    m_webCharacteristic->setValue(domBuffer);
}
m_resolver->resolve(domBuffer);

[fbeaufortchromium, 2016-01-14 10:20:53]

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

[commit-bot: I haz the power, 2016-01-14 10:21:05]

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

[commit-bot: I haz the power, 2016-01-14 10:21:06]

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

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

Dry run: No L-G-T-M from a valid reviewer yet. Only full committers are
accepted.
Even if an L-G-T-M may have been provided, it was from a non-committer,
_not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[François Beaufort, 2016-01-14 10:22:28]

Description was changed from

==========
bluetooth: Update BluetoothCharacteristic.value on readValue

WIP

BUG=575577
==========

to

==========
bluetooth: Update BluetoothCharacteristic.value on readValue

BUG=575577
==========

[fbeaufortchromium, 2016-01-14 10:28:22]

Patchset #3 (id:40001) has been deleted

[fbeaufortchromium, 2016-01-15 08:15:08]

Patchset #3 (id:60001) has been deleted

[François Beaufort, 2016-01-15 08:17:35]

On 2016/01/13 21:14:09, ortuno wrote:
> One small issue. Thanks for doing this by the way!
> 
>
https://codereview.chromium.org/1573183002/diff/20001/third_party/WebKit/Sour...
> File
third_party/WebKit/Source/modules/bluetooth/BluetoothGATTCharacteristic.cpp
> (right):
> 
>
https://codereview.chromium.org/1573183002/diff/20001/third_party/WebKit/Sour...
>
third_party/WebKit/Source/modules/bluetooth/BluetoothGATTCharacteristic.cpp:119:
> Persistent<BluetoothGATTCharacteristic> m_webCharacteristic;
> Talked to jyasskin about this. We don't need to keep an object alive longer
than
> necessary so we should use WeakPersistent instead. That would mean you need to
> add a null check before resolving the promise:
> 
> RefPtr<DOMArrayBuffer> domBuffer = ConvertWebVectorToArrayBuffer(value);
> if (m_webCharacteristic != nullptr) {
>     m_webCharacteristic->setValue(domBuffer);
> }
> m_resolver->resolve(domBuffer);

Thank you!
Based on your review, I've used WeakPersistent.

[ortuno, 2016-01-15 17:12:51]

lgtm with one nit.

https://codereview.chromium.org/1573183002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/bluetooth/BluetoothGATTCharacteristic.cpp
(right):

https://codereview.chromium.org/1573183002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/bluetooth/BluetoothGATTCharacteristic.cpp:20:
PassRefPtr<DOMArrayBuffer> ConvertWebVectorToArrayBuffer(
nit: I would put this in an anonymous namespace.

[yhirano, 2016-01-17 23:25:14]

lgtm from my side.

[fbeaufortchromium, 2016-01-18 07:42:17]

The CQ bit was checked by fbeaufort@chromium.org

[fbeaufortchromium, 2016-01-18 07:42:18]

The patchset sent to the CQ was uploaded after l-g-t-m from ortuno@chromium.org,
yhirano@chromium.org
Link to the patchset: https://codereview.chromium.org/1573183002/#ps100001
(title: "Use anonymous namespace")

[commit-bot: I haz the power, 2016-01-18 07:42:31]

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

[commit-bot: I haz the power, 2016-01-18 08:51:39]

Committed patchset #4 (id:100001)

[commit-bot: I haz the power, 2016-01-18 08:52:47]

Description was changed from

==========
bluetooth: Update BluetoothCharacteristic.value on readValue

BUG=575577
==========

to

==========
bluetooth: Update BluetoothCharacteristic.value on readValue

BUG=575577

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

[commit-bot: I haz the power, 2016-01-18 08:52:48]

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