bluetooth: Create Mojo equivalent of SetBluetoothMockDataSet

bluetooth: Create Mojo equivalent of SetBluetoothMockDataSet

Adds setBluetoothFakeAdapter to testRunner in order to set the
fake adapter on the browser side. Also removes the old SetBluetoothMockDataSet.

Since Web Bluetooth is transitioning to Mojo it can no longer rely
on IPC::Message's ordering to set fake adapters. This is because there are
no order guarantees between IPC::Message and Mojo and even between
Mojo pipes. For this reason we introduce a new function that takes
a callback that is run when the adapter has been set on the browser.

Since IPC::Message is deprecated this new function uses Mojo to communicate
with the browser.

BUG=508771
Committed: https://crrev.com/f78c841627596d492015ffd5ebfd9e3d091522a7
Cr-Commit-Position: refs/heads/master@{#384350}

[ortuno, 2016-03-18 01:25:42]

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

[ortuno, 2016-03-18 01:25:43]

jyasskin: PTAL

[Jeffrey Yasskin, 2016-03-24 23:47:54]

Sorry for taking so long to get to this.

https://codereview.chromium.org/1815483003/diff/40001/components/test_runner/...
File components/test_runner/test_runner.h (right):

https://codereview.chromium.org/1815483003/diff/40001/components/test_runner/...
components/test_runner/test_runner.h:532: void SetBluetoothMockDataSet(const
std::string& name);
Can you remove the old interface in the same patch?

https://codereview.chromium.org/1815483003/diff/40001/content/content_shell.gypi
File content/content_shell.gypi (right):

https://codereview.chromium.org/1815483003/diff/40001/content/content_shell.g...
content/content_shell.gypi:22: 'content_shell_mojo_bindings',
Sort this, I think.

https://codereview.chromium.org/1815483003/diff/40001/content/content_shell.g...
content/content_shell.gypi:649: 'target_name':
'content_shell_mojo_bindings_mojom',
I don't know enough GYP to understand why this has to be 2 extra targets, but
that's what other mojo users seem to be doing, so sure.

https://codereview.chromium.org/1815483003/diff/40001/content/shell/browser/l...
File
content/shell/browser/layout_test/layout_test_bluetooth_fake_adapter_setter_impl.cc
(right):

https://codereview.chromium.org/1815483003/diff/40001/content/shell/browser/l...
content/shell/browser/layout_test/layout_test_bluetooth_fake_adapter_setter_impl.cc:35:
adapter_name.To<std::string>()));
It looks like you can use .get() to return a const std::string& without copying?

https://codereview.chromium.org/1815483003/diff/40001/content/shell/common/la...
File
content/shell/common/layout_test/layout_test_bluetooth_fake_adapter_setter.mojom
(right):

https://codereview.chromium.org/1815483003/diff/40001/content/shell/common/la...
content/shell/common/layout_test/layout_test_bluetooth_fake_adapter_setter.mojom:9:
Set(string adapter_name) => ();
Comment what this does.

https://codereview.chromium.org/1815483003/diff/40001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/bluetooth/writeValue.html (right):

https://codereview.chromium.org/1815483003/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/bluetooth/writeValue.html:12:
testRunner.setBluetoothMockDataSet('HeartRateAdapter');
Should you replace these at the same time?

[ortuno, 2016-03-28 20:45:43]

Description was changed from

==========
bluetooth: Create Mojo equivalent of SetBluetoothMockDataSet

Adds setBluetoothFakeAdapter to testRunner in order to set the
fake adapter on the browser side.

Since Web Bluetooth is transitioning to Mojo it can no longer rely
on IPC::Message's ordering to set fake adapters. This is because there are
no order guarantees between IPC::Message and Mojo and even between
Mojo pipes. For this reason we introduce a new function that takes
a callback that is run when the adapter has been set on the browser.

Since IPC::Message is deprecated this new function uses Mojo to communicate
with the browser.

BUG=508771
==========

to

==========
bluetooth: Create Mojo equivalent of SetBluetoothMockDataSet

Adds setBluetoothFakeAdapter to testRunner in order to set the
fake adapter on the browser side. Also removes the old SetBluetoothMockDataSet.

Since Web Bluetooth is transitioning to Mojo it can no longer rely
on IPC::Message's ordering to set fake adapters. This is because there are
no order guarantees between IPC::Message and Mojo and even between
Mojo pipes. For this reason we introduce a new function that takes
a callback that is run when the adapter has been set on the browser.

Since IPC::Message is deprecated this new function uses Mojo to communicate
with the browser.

BUG=508771
==========

[ortuno, 2016-03-28 23:26:44]

https://codereview.chromium.org/1815483003/diff/40001/components/test_runner/...
File components/test_runner/test_runner.h (right):

https://codereview.chromium.org/1815483003/diff/40001/components/test_runner/...
components/test_runner/test_runner.h:532: void SetBluetoothMockDataSet(const
std::string& name);
On 2016/03/24 at 23:47:53, Jeffrey Yasskin wrote:
> Can you remove the old interface in the same patch?

Done.

https://codereview.chromium.org/1815483003/diff/40001/content/content_shell.gypi
File content/content_shell.gypi (right):

https://codereview.chromium.org/1815483003/diff/40001/content/content_shell.g...
content/content_shell.gypi:22: 'content_shell_mojo_bindings',
On 2016/03/24 at 23:47:54, Jeffrey Yasskin wrote:
> Sort this, I think.

Done.

https://codereview.chromium.org/1815483003/diff/40001/content/content_shell.g...
content/content_shell.gypi:649: 'target_name':
'content_shell_mojo_bindings_mojom',
On 2016/03/24 at 23:47:53, Jeffrey Yasskin wrote:
> I don't know enough GYP to understand why this has to be 2 extra targets, but
that's what other mojo users seem to be doing, so sure.

Yeah, otherwise component builds don't work...

https://codereview.chromium.org/1815483003/diff/40001/content/shell/browser/l...
File
content/shell/browser/layout_test/layout_test_bluetooth_fake_adapter_setter_impl.cc
(right):

https://codereview.chromium.org/1815483003/diff/40001/content/shell/browser/l...
content/shell/browser/layout_test/layout_test_bluetooth_fake_adapter_setter_impl.cc:35:
adapter_name.To<std::string>()));
On 2016/03/24 at 23:47:54, Jeffrey Yasskin wrote:
> It looks like you can use .get() to return a const std::string& without
copying?

Hmm. I might have run into an edge case in another patch and that's why I used
To here. Changed to just adapter_name.

https://codereview.chromium.org/1815483003/diff/40001/content/shell/common/la...
File
content/shell/common/layout_test/layout_test_bluetooth_fake_adapter_setter.mojom
(right):

https://codereview.chromium.org/1815483003/diff/40001/content/shell/common/la...
content/shell/common/layout_test/layout_test_bluetooth_fake_adapter_setter.mojom:9:
Set(string adapter_name) => ();
On 2016/03/24 at 23:47:54, Jeffrey Yasskin wrote:
> Comment what this does.

Done.

https://codereview.chromium.org/1815483003/diff/40001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/bluetooth/writeValue.html (right):

https://codereview.chromium.org/1815483003/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/bluetooth/writeValue.html:12:
testRunner.setBluetoothMockDataSet('HeartRateAdapter');
On 2016/03/24 at 23:47:54, Jeffrey Yasskin wrote:
> Should you replace these at the same time?

We could. I've been doing it patch by patch. I'll do them all at once if you
want.

[Jeffrey Yasskin, 2016-03-29 01:59:13]

lgtm

https://codereview.chromium.org/1815483003/diff/40001/components/test_runner/...
File components/test_runner/test_runner.h (right):

https://codereview.chromium.org/1815483003/diff/40001/components/test_runner/...
components/test_runner/test_runner.h:532: void SetBluetoothMockDataSet(const
std::string& name);
On 2016/03/28 23:26:44, ortuno wrote:
> On 2016/03/24 at 23:47:53, Jeffrey Yasskin wrote:
> > Can you remove the old interface in the same patch?
> 
> Done.

Cool, thanks. Another reasonable answer would have been that it makes the patch
much bigger so you'd rather do it in a couple steps. :)

https://codereview.chromium.org/1815483003/diff/80001/components/test_runner/...
File components/test_runner/test_runner.cc (right):

https://codereview.chromium.org/1815483003/diff/80001/components/test_runner/...
components/test_runner/test_runner.cc:578: .SetMethod("setBluetoothFakeAdapter",
I think keep this below the link to the spec that'll hold its definition, even
though the definition isn't there yet.

https://codereview.chromium.org/1815483003/diff/80001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/bluetooth/connect.html (right):

https://codereview.chromium.org/1815483003/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/bluetooth/connect.html:113: // This test was not
returning th assert_promise_rejects_with_message
s/th/the/ and d'oh.

https://codereview.chromium.org/1815483003/diff/80001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/bluetooth/requestDevice.html (right):

https://codereview.chromium.org/1815483003/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/bluetooth/requestDevice.html:13:
testRunner.setBluetoothManualChooser(true);
Is this going to fail non-deterministically because the IPC ordering isn't
guaranteed, or does the fact that we get the response for a later IPC wind up
guaranteeing that the first one has completed?

[ortuno, 2016-03-29 16:05:17]

https://codereview.chromium.org/1815483003/diff/80001/components/test_runner/...
File components/test_runner/test_runner.cc (right):

https://codereview.chromium.org/1815483003/diff/80001/components/test_runner/...
components/test_runner/test_runner.cc:578: .SetMethod("setBluetoothFakeAdapter",
On 2016/03/29 at 01:59:13, Jeffrey Yasskin wrote:
> I think keep this below the link to the spec that'll hold its definition, even
though the definition isn't there yet.

Done.

https://codereview.chromium.org/1815483003/diff/80001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/bluetooth/connect.html (right):

https://codereview.chromium.org/1815483003/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/bluetooth/connect.html:113: // This test was not
returning th assert_promise_rejects_with_message
On 2016/03/29 at 01:59:13, Jeffrey Yasskin wrote:
> s/th/the/ and d'oh.

Done.

https://codereview.chromium.org/1815483003/diff/80001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/bluetooth/requestDevice.html (right):

https://codereview.chromium.org/1815483003/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/bluetooth/requestDevice.html:13:
testRunner.setBluetoothManualChooser(true);
On 2016/03/29 at 01:59:13, Jeffrey Yasskin wrote:
> Is this going to fail non-deterministically because the IPC ordering isn't
guaranteed, or does the fact that we get the response for a later IPC wind up
guaranteeing that the first one has completed?

For now we are relying on the fact that requestDevice uses IPC, so we know the
chooser will always be set by the time the requestDevice request comes in the
browser. We will have to change this before moving requestDevice to mojo.

I've been thinking about making a BluetoothChooser mojo Service and exposing it
through a window.internals API in modules/bluetooth/testing. Since we can now
use Mojo from Blink, we won't have to go through as many hoops as we do for the
current implementation. I'll look deeper into it (and write a design doc) before
I start moving requestDevice to mojo.

[ortuno, 2016-03-29 16:06:06]

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

[ortuno, 2016-03-29 16:06:07]

rockot: PTAL

[Ken Rockot(use gerrit already), 2016-03-29 20:53:23]

lgtm

https://codereview.chromium.org/1815483003/diff/100001/content/shell/renderer...
File content/shell/renderer/layout_test/blink_test_runner.cc (right):

https://codereview.chromium.org/1815483003/diff/100001/content/shell/renderer...
content/shell/renderer/layout_test/blink_test_runner.cc:953: return
bluetooth_fake_adapter_setter_;
nit: I think it's weird to return a reference to a scoped_ptr instead of simply
returning a raw pointer. You can return bluetooth_fake_adapter_setter_.get(),
which is of type mojom::LayoutTestBluetoothFakeAdapterSetter*. It will work the
same, with the added bonus that the caller can't inadvertently trash the
InterfacePtr.

[ortuno, 2016-03-29 21:17:04]

Thanks!

https://codereview.chromium.org/1815483003/diff/100001/content/shell/renderer...
File content/shell/renderer/layout_test/blink_test_runner.cc (right):

https://codereview.chromium.org/1815483003/diff/100001/content/shell/renderer...
content/shell/renderer/layout_test/blink_test_runner.cc:953: return
bluetooth_fake_adapter_setter_;
On 2016/03/29 at 20:53:23, Ken Rockot wrote:
> nit: I think it's weird to return a reference to a scoped_ptr instead of
simply returning a raw pointer. You can return
bluetooth_fake_adapter_setter_.get(), which is of type
mojom::LayoutTestBluetoothFakeAdapterSetter*. It will work the same, with the
added bonus that the caller can't inadvertently trash the InterfacePtr.

Ha. Makes sense. I saw VR doing the same so I just copied it. Changed to
pointer.

[ortuno, 2016-03-29 21:17:40]

ortuno@chromium.org changed reviewers:
+ jam@chromium.org, mkwst@chromium.org

[ortuno, 2016-03-29 21:17:41]

mkwst@chromium.org: PTAL at components/test_runner

jam@chromium.org: PTAL at content/

[Jeffrey Yasskin, 2016-03-29 22:19:10]

https://codereview.chromium.org/1815483003/diff/100001/content/shell/renderer...
File content/shell/renderer/layout_test/blink_test_runner.cc (right):

https://codereview.chromium.org/1815483003/diff/100001/content/shell/renderer...
content/shell/renderer/layout_test/blink_test_runner.cc:953: return
bluetooth_fake_adapter_setter_;
On 2016/03/29 20:53:23, Ken Rockot wrote:
> nit: I think it's weird to return a reference to a scoped_ptr instead of
simply
> returning a raw pointer. You can return bluetooth_fake_adapter_setter_.get(),
> which is of type mojom::LayoutTestBluetoothFakeAdapterSetter*. It will work
the
> same, with the added bonus that the caller can't inadvertently trash the
> InterfacePtr.

Or, since this can never return null, return the
mojom::LayoutTestBluetoothFakeAdapterSetter& *bluetooth_fake_adapter_setter_?

[ortuno, 2016-03-29 22:30:53]

https://codereview.chromium.org/1815483003/diff/100001/content/shell/renderer...
File content/shell/renderer/layout_test/blink_test_runner.cc (right):

https://codereview.chromium.org/1815483003/diff/100001/content/shell/renderer...
content/shell/renderer/layout_test/blink_test_runner.cc:953: return
bluetooth_fake_adapter_setter_;
On 2016/03/29 at 22:19:10, Jeffrey Yasskin wrote:
> On 2016/03/29 20:53:23, Ken Rockot wrote:
> > nit: I think it's weird to return a reference to a scoped_ptr instead of
simply
> > returning a raw pointer. You can return
bluetooth_fake_adapter_setter_.get(),
> > which is of type mojom::LayoutTestBluetoothFakeAdapterSetter*. It will work
the
> > same, with the added bonus that the caller can't inadvertently trash the
> > InterfacePtr.
> 
> Or, since this can never return null, return the
mojom::LayoutTestBluetoothFakeAdapterSetter& *bluetooth_fake_adapter_setter_?

Done.

[jam, 2016-03-30 00:31:11]

jam@chromium.org changed reviewers:
+ jochen@chromium.org

[jam, 2016-03-30 00:31:12]

deferring to Jochen as I'm not familiar with the test runner

[jochen (gone - plz use gerrit), 2016-03-30 16:35:26]

lgtm

[ortuno, 2016-03-30 22:39:12]

The CQ bit was checked by ortuno@chromium.org

[ortuno, 2016-03-30 22:39:13]

The patchset sent to the CQ was uploaded after l-g-t-m from
jyasskin@chromium.org, rockot@chromium.org
Link to the patchset: https://codereview.chromium.org/1815483003/#ps140001
(title: "Make Getter return a reference")

[ortuno, 2016-03-30 22:39:23]

The CQ bit was unchecked by ortuno@chromium.org

[ortuno, 2016-03-30 23:22:27]

The CQ bit was checked by ortuno@chromium.org

[ortuno, 2016-03-30 23:22:27]

The patchset sent to the CQ was uploaded after l-g-t-m from rockot@chromium.org,
jyasskin@chromium.org, jochen@chromium.org
Link to the patchset: https://codereview.chromium.org/1815483003/#ps160001
(title: "MMerge with ToT")

[commit-bot: I haz the power, 2016-03-30 23:22:53]

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

[commit-bot: I haz the power, 2016-03-30 23:22:55]

This CL has an open dependency (Issue 1775953004 Patch 400001). Please resolve
the dependency and try again.

[ortuno, 2016-03-31 18:50:27]

The CQ bit was checked by ortuno@chromium.org

[commit-bot: I haz the power, 2016-03-31 18:51:22]

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

[commit-bot: I haz the power, 2016-03-31 19:00:09]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-03-31 19:01:45]

Description was changed from

==========
bluetooth: Create Mojo equivalent of SetBluetoothMockDataSet

Adds setBluetoothFakeAdapter to testRunner in order to set the
fake adapter on the browser side. Also removes the old SetBluetoothMockDataSet.

Since Web Bluetooth is transitioning to Mojo it can no longer rely
on IPC::Message's ordering to set fake adapters. This is because there are
no order guarantees between IPC::Message and Mojo and even between
Mojo pipes. For this reason we introduce a new function that takes
a callback that is run when the adapter has been set on the browser.

Since IPC::Message is deprecated this new function uses Mojo to communicate
with the browser.

BUG=508771
==========

to

==========
bluetooth: Create Mojo equivalent of SetBluetoothMockDataSet

Adds setBluetoothFakeAdapter to testRunner in order to set the
fake adapter on the browser side. Also removes the old SetBluetoothMockDataSet.

Since Web Bluetooth is transitioning to Mojo it can no longer rely
on IPC::Message's ordering to set fake adapters. This is because there are
no order guarantees between IPC::Message and Mojo and even between
Mojo pipes. For this reason we introduce a new function that takes
a callback that is run when the adapter has been set on the browser.

Since IPC::Message is deprecated this new function uses Mojo to communicate
with the browser.

BUG=508771

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

[commit-bot: I haz the power, 2016-03-31 19:01:48]

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