bluetooth: Initial WebBluetooth & WebBluetoothError.

bluetooth: Initial WebBluetooth & WebBluetoothError.

In three patches the initial mock implementation of bluetooth.requestDevice
is implemented across blink & content. This allows layout tests to specify the
data responses the mock implemented in content should return via
testRunner.SetBluetoothMockDataSet and call bluetooth.requestDevice with the
expected results.

crrev.com/650613005 blink::WebBluetooth & WebBluetoothError interfaces.
crrev.com/702593002 content::WebBluetoothImpl & testRunner.SetBluetoothMockDataSet.
crrev.com/686813003 blink::BluetoothDiscovery::requestDevice implemented.

BUG=420284
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=184839

[scheib, 2014-10-17 21:16:58]

Patchset #1 (id:1) has been deleted

[scheib, 2014-10-17 22:59:35]

Patchset #3 (id:60001) has been deleted

[scheib, 2014-10-17 22:59:42]

Patchset #1 (id:20001) has been deleted

[scheib, 2014-10-17 22:59:51]

Patchset #1 (id:40001) has been deleted

[scheib, 2014-10-17 23:05:18]

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

[haraken, 2014-10-18 11:25:56]

I'm not really happy about adding Internals APIs to test a public/platform/
change. Instead of testing it in layout tests, can you add a chromium-side
unittests?

https://codereview.chromium.org/650613005/diff/80001/Source/modules/bluetooth...
File Source/modules/bluetooth/BluetoothDiscovery.cpp (right):

https://codereview.chromium.org/650613005/diff/80001/Source/modules/bluetooth...
Source/modules/bluetooth/BluetoothDiscovery.cpp:21: WebBluetooth* webbluetooth =
m_bluetoothMock ? m_bluetoothMock : Platform::current()->bluetooth();

Help me understand: What is this doing? Who sets the platform's bluetooth?

https://codereview.chromium.org/650613005/diff/80001/Source/modules/bluetooth...
File Source/modules/bluetooth/BluetoothDiscovery.h (right):

https://codereview.chromium.org/650613005/diff/80001/Source/modules/bluetooth...
Source/modules/bluetooth/BluetoothDiscovery.h:17: class BluetoothDiscovery

Add final.

https://codereview.chromium.org/650613005/diff/80001/Source/modules/bluetooth...
Source/modules/bluetooth/BluetoothDiscovery.h:25: BluetoothMock*
getBluetoothMockForTesting() { return m_bluetoothMock.get(); }

Blink normally doesn't use "get".

getBluetoothMockForTesting => bluetoothMockForTesting

https://codereview.chromium.org/650613005/diff/80001/Source/modules/bluetooth...
File Source/modules/bluetooth/BluetoothError.cpp (right):

https://codereview.chromium.org/650613005/diff/80001/Source/modules/bluetooth...
Source/modules/bluetooth/BluetoothError.cpp:29: delete webErrorRaw;

Why do we need to manually call 'delete'?

https://codereview.chromium.org/650613005/diff/80001/Source/modules/bluetooth...
File Source/modules/bluetooth/testing/InternalsBluetooth.h (right):

https://codereview.chromium.org/650613005/diff/80001/Source/modules/bluetooth...
Source/modules/bluetooth/testing/InternalsBluetooth.h:18: static void
bluetoothRequestDeviceWillReject(Internals&, Navigator*, const String& error);

const String& => String (Blink normally uses String.)

[haraken, 2014-10-18 11:26:28]

haraken@chromium.org changed reviewers:
+ tkent@chromium.org, yhirano@chromium.org

[haraken, 2014-10-18 11:26:29]

+tkent-san for public APIs.

+hirano-san for Promise changes.

[scheib, 2014-10-19 03:54:21]

Thank you, advice on best testing Blink practices welcome!

We are adding Bluetooth support here, and must have a way to expose Bluetooth
functionality of a platform. Blink should not know the specifics, and will
interface with abstracted platform concepts via WebBluetooth, and in the future
WebBluetoothDevice, WebBluetoothService, et cetera as needed.

Layout tests can not presume the platform support exists. Instead a mock
implementation should be used that can be configured. In the near term, such as
"return a bluetooth device" or "fail with no bluetooth device". And soon,
"return a device with a heart rate monitor service and a battery service, report
some specific values when the characteristics (attributes) are queried)". This
will be necessary to ensure that the IDL binding and Blink module correctly
routes information from the blink platform api WebBluetooth* interfaces.

If there are other approaches, please let me know.

https://codereview.chromium.org/650613005/diff/80001/Source/modules/bluetooth...
File Source/modules/bluetooth/BluetoothDiscovery.cpp (right):

https://codereview.chromium.org/650613005/diff/80001/Source/modules/bluetooth...
Source/modules/bluetooth/BluetoothDiscovery.cpp:21: WebBluetooth* webbluetooth =
m_bluetoothMock ? m_bluetoothMock : Platform::current()->bluetooth();
On 2014/10/18 11:25:56, haraken wrote:
> 
> Help me understand: What is this doing? Who sets the platform's bluetooth?

The platform should provide the WebBluetooth interface, which implements the
platform's support of bluetooth device access (later, also additional interfaces
for platform representations of devices and their services).

For layout tests of the Blink module, the platform is mocked out, and the
m_bluetoothMock member will be set to provide a mock implementation of what the
platform would do. The mock is configurable by windows.internals.methods.

https://codereview.chromium.org/650613005/diff/80001/Source/modules/bluetooth...
File Source/modules/bluetooth/BluetoothError.cpp (right):

https://codereview.chromium.org/650613005/diff/80001/Source/modules/bluetooth...
Source/modules/bluetooth/BluetoothError.cpp:29: delete webErrorRaw;
On 2014/10/18 11:25:56, haraken wrote:
> 
> Why do we need to manually call 'delete'?

This is an object for use by CallbackPromiseAdapter, doc here:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

I will add a reference to that in the class comment.

https://codereview.chromium.org/650613005/diff/80001/Source/modules/bluetooth...
File Source/modules/bluetooth/BluetoothError.h (right):

https://codereview.chromium.org/650613005/diff/80001/Source/modules/bluetooth...
Source/modules/bluetooth/BluetoothError.h:15: 
I will add a class comment here:

// BluetoothError is used with CallbackPromiseAdapter to receive
// WebBluetoothError responses. See CallbackPromiseAdapter class comments.

[haraken, 2014-10-20 02:16:04]

> We are adding Bluetooth support here, and must have a way to expose Bluetooth
> functionality of a platform. Blink should not know the specifics, and will
> interface with abstracted platform concepts via WebBluetooth, and in the
future
> WebBluetoothDevice, WebBluetoothService, et cetera as needed.
> 
> Layout tests can not presume the platform support exists. Instead a mock
> implementation should be used that can be configured. In the near term, such
as
> "return a bluetooth device" or "fail with no bluetooth device". And soon,
> "return a device with a heart rate monitor service and a battery service,
report
> some specific values when the characteristics (attributes) are queried)". This
> will be necessary to ensure that the IDL binding and Blink module correctly
> routes information from the blink platform api WebBluetooth* interfaces.
> 
> If there are other approaches, please let me know.

Given that you're going to expose WebBluetooth to chromium, wouldn't it be
possible to test the behavior in chromium-side unittests (e.g., see
src/content/renderer/media/*-unittest.cc)?

(1) Land this CL without a test
(2) After the Blink is rolled, add a chromium-side unittest to test the
WebBluetooth behavior.

(Of course, I want to see a CL for (2) before landing a CL for (1) :-)

[tkent, 2014-10-20 04:34:42]

Bluetooth API is not shipped yet.  You don't need to involve an API owner for
each of changes.

[scheib, 2014-10-20 04:56:48]

On 2014/10/20 02:16:04, haraken wrote:
> > We are adding Bluetooth support here, and must have a way to expose
Bluetooth
> > functionality of a platform. Blink should not know the specifics, and will
> > interface with abstracted platform concepts via WebBluetooth, and in the
> future
> > WebBluetoothDevice, WebBluetoothService, et cetera as needed.
> > 
> > Layout tests can not presume the platform support exists. Instead a mock
> > implementation should be used that can be configured. In the near term, such
> as
> > "return a bluetooth device" or "fail with no bluetooth device". And soon,
> > "return a device with a heart rate monitor service and a battery service,
> report
> > some specific values when the characteristics (attributes) are queried)".
This
> > will be necessary to ensure that the IDL binding and Blink module correctly
> > routes information from the blink platform api WebBluetooth* interfaces.
> > 
> > If there are other approaches, please let me know.
> 
> Given that you're going to expose WebBluetooth to chromium, wouldn't it be
> possible to test the behavior in chromium-side unittests (e.g., see
> src/content/renderer/media/*-unittest.cc)?
> 
> (1) Land this CL without a test
> (2) After the Blink is rolled, add a chromium-side unittest to test the
> WebBluetooth behavior.
> 
> (Of course, I want to see a CL for (2) before landing a CL for (1) :-)

I'm a concerned I am not communicating correctly, or maybe I'm not understanding
your concern.

I believe tests are appropriate both in content and in blink.
public/platform/WebBluetooth.h is an interface Blink expects the content layer
to implement.

Content should test that WebBluetoothImpl produces the correct information given
mocked lower level components, and calls to the WebBluetooth interface.

Blink should presume that WebBluetooth interface works, and not test its
implementation. It should mock that interface to cause deterministic behavior
when testing the implementation of the code in modules/bluetooth providing the
javascript interface. This patch does this - it does not use the
yet-to-be-written WebBluetoothImpl, it provides a Mock implementation of that
interface so that LayoutTests/bluetooth/requestDevice.html can test
navigator.bluetooth.requestDevice and control what the 'platform' will respond
with.

As more complexity of the API is added, the Mock will gain more complexity to
control it. So that Javascript can requestDevice("(handwaving)heartrate
service"), receive a BluetoothDevice object back, and then query that it has the
expected service on it.

This is similar perhaps to how
WebKit/Source/modules/geolocation/testing/GeolocationClientMock.h works, though
it jumps through additional classes
third_party/WebKit/Source/web/GeolocationClientProxy.h to swap in the mock where
in this patch a simpler member for testing is used.

In your suggestion (1, 2), I don't think I can easily add chromium-side tests
that verify the javascript interfaces function as the specification expects them
to. The unit tests there deal with the C++ interfaces. Layout tests seem most
appropriate.

[haraken, 2014-10-20 06:12:13]

On 2014/10/20 04:56:48, scheib wrote:
> On 2014/10/20 02:16:04, haraken wrote:
> > > We are adding Bluetooth support here, and must have a way to expose
> Bluetooth
> > > functionality of a platform. Blink should not know the specifics, and will
> > > interface with abstracted platform concepts via WebBluetooth, and in the
> > future
> > > WebBluetoothDevice, WebBluetoothService, et cetera as needed.
> > > 
> > > Layout tests can not presume the platform support exists. Instead a mock
> > > implementation should be used that can be configured. In the near term,
such
> > as
> > > "return a bluetooth device" or "fail with no bluetooth device". And soon,
> > > "return a device with a heart rate monitor service and a battery service,
> > report
> > > some specific values when the characteristics (attributes) are queried)".
> This
> > > will be necessary to ensure that the IDL binding and Blink module
correctly
> > > routes information from the blink platform api WebBluetooth* interfaces.
> > > 
> > > If there are other approaches, please let me know.
> > 
> > Given that you're going to expose WebBluetooth to chromium, wouldn't it be
> > possible to test the behavior in chromium-side unittests (e.g., see
> > src/content/renderer/media/*-unittest.cc)?
> > 
> > (1) Land this CL without a test
> > (2) After the Blink is rolled, add a chromium-side unittest to test the
> > WebBluetooth behavior.
> > 
> > (Of course, I want to see a CL for (2) before landing a CL for (1) :-)
> 
> I'm a concerned I am not communicating correctly, or maybe I'm not
understanding
> your concern.
> 
> I believe tests are appropriate both in content and in blink.
> public/platform/WebBluetooth.h is an interface Blink expects the content layer
> to implement.
> 
> Content should test that WebBluetoothImpl produces the correct information
given
> mocked lower level components, and calls to the WebBluetooth interface.
> 
> Blink should presume that WebBluetooth interface works, and not test its
> implementation. It should mock that interface to cause deterministic behavior
> when testing the implementation of the code in modules/bluetooth providing the
> javascript interface. This patch does this - it does not use the
> yet-to-be-written WebBluetoothImpl, it provides a Mock implementation of that
> interface so that LayoutTests/bluetooth/requestDevice.html can test
> navigator.bluetooth.requestDevice and control what the 'platform' will respond
> with.
> 
> As more complexity of the API is added, the Mock will gain more complexity to
> control it. So that Javascript can requestDevice("(handwaving)heartrate
> service"), receive a BluetoothDevice object back, and then query that it has
the
> expected service on it.
> 
> This is similar perhaps to how
> WebKit/Source/modules/geolocation/testing/GeolocationClientMock.h works,
though
> it jumps through additional classes
> third_party/WebKit/Source/web/GeolocationClientProxy.h to swap in the mock
where
> in this patch a simpler member for testing is used.
> 
> In your suggestion (1, 2), I don't think I can easily add chromium-side tests
> that verify the javascript interfaces function as the specification expects
them
> to. The unit tests there deal with the C++ interfaces. Layout tests seem most
> appropriate.

OK, your point makes sense. Let's go that way :)

(I agree with your point but just thought that it would be enough to test it in
the content layer (we're doing this for most platform things), given the
complexity we need to add to the Blink code base to mock the platform object.)

[yhirano, 2014-10-20 10:42:45]

lgtm.

> This will be necessary to ensure that the IDL binding and Blink module
correctly routes information from the blink platform api WebBluetooth*
interfaces.
You can write Blink unittests to test the Blink module.

https://codereview.chromium.org/650613005/diff/80001/public/platform/Platform.h
File public/platform/Platform.h (right):

https://codereview.chromium.org/650613005/diff/80001/public/platform/Platform...
public/platform/Platform.h:646: virtual WebBluetooth* bluetooth() { return 0; }
Using nullptr is fine.

[scheib, 2014-10-20 21:39:50]

Thank you haraken.

After discussions with dglazkov and dpranke I will not land this patch.

Instead I will pursue having content shell in layout test mode provide stubbed
out bluetooth data responses. These should integrate from JS code down through
content and to the src/device bluetooth mock stub implementations.

[scheib, 2014-11-03 23:44:23]

Patchset #2 (id:100001) has been deleted

[scheib, 2014-11-04 00:07:27]

scheib@chromium.org changed reviewers:
+ japhet@chromium.org
- haraken@chromium.org, tkent@chromium.org, yhirano@chromium.org

[scheib, 2014-11-04 00:11:51]

I've taken the new approach for the initial Mock implementation of
bluetooth.requestDevice by moving the mock implementation into content,
requiring the patch to be split into three.

As the initial portion is only in public, +japhet, a local OWNER (others feel
free to chime in).

[haraken, 2014-11-04 02:55:01]

LGTM, but needs an approval from a public/ owner.

[Nate Chapin, 2014-11-04 21:44:05]

public/ LGTM

[scheib, 2014-11-04 21:58:07]

The CQ bit was checked by scheib@chromium.org

[commit-bot: I haz the power, 2014-11-04 21:58:41]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/650613005/120001

[commit-bot: I haz the power, 2014-11-04 22:24:00]

Committed patchset #2 (id:120001) as 184839