Add chrome://usb-internals page for adding and removing test devices.

Add chrome://usb-internals page for adding and removing test devices.

This patch adds a new WebUI page, chrome://usb-internals with controls
for simulating the connection and disconnection of virtual WebUSB
devices. This will be useful for doing UI testing without the need for
real hardware.

This page will be expanded in the future to add other features useful
for debugging WebUSB such as viewing device information as it has been
parsed by Chrome and overriding descriptors during development.

BUG=625019
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/3903a69e08972bf282615d725b918109e5cb7a44
Cr-Commit-Position: refs/heads/master@{#411091}

[Reilly Grant (use Gerrit), 2016-08-04 01:31:23]

Description was changed from

==========
Add chrome://usb-internals page for adding and removing test devices.

This patch adds a new WebUI page, chrome://usb-internals with controls
for simulating the connection and disconnection of virtual WebUSB
devices. This will be useful for doing UI testing without the need for
real hardware.

This page will be expanded in the future to add other features useful
for debugging WebUSB such as viewing device information as it has been
parsed by Chrome and overriding descriptors during development.

BUG=625019
==========

to

==========
Add chrome://usb-internals page for adding and removing test devices.

This patch adds a new WebUI page, chrome://usb-internals with controls
for simulating the connection and disconnection of virtual WebUSB
devices. This will be useful for doing UI testing without the need for
real hardware.

This page will be expanded in the future to add other features useful
for debugging WebUSB such as viewing device information as it has been
parsed by Chrome and overriding descriptors during development.

BUG=625019
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[Reilly Grant (use Gerrit), 2016-08-04 01:33:11]

reillyg@chromium.org changed reviewers:
+ dbeam@chromium.org

[Reilly Grant (use Gerrit), 2016-08-04 01:33:11]

Please take a look.

[Reilly Grant (use Gerrit), 2016-08-04 01:33:20]

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

[commit-bot: I haz the power, 2016-08-04 01:33:46]

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

[Dan Beam, 2016-08-04 02:54:32]

dbeam@chromium.org changed reviewers:
+ dpapad@chromium.org

[Dan Beam, 2016-08-04 02:54:33]

I think dpapad@ might be a better reviewer of mojo-based webui pages

[commit-bot: I haz the power, 2016-08-04 03:08:59]

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

[commit-bot: I haz the power, 2016-08-04 03:09:00]

Dry run: This issue passed the CQ dry run.

[dpapad, 2016-08-04 20:55:35]

https://codereview.chromium.org/2204713006/diff/1/chrome/browser/resources/us...
File chrome/browser/resources/usb_internals/usb_internals.html (right):

https://codereview.chromium.org/2204713006/diff/1/chrome/browser/resources/us...
chrome/browser/resources/usb_internals/usb_internals.html:54: <button
type="submit">Add</button> <span id="add-test-device-result">
This <span> is missing closing tag, </span>

https://codereview.chromium.org/2204713006/diff/1/chrome/browser/resources/us...
File chrome/browser/resources/usb_internals/usb_internals.js (right):

https://codereview.chromium.org/2204713006/diff/1/chrome/browser/resources/us...
chrome/browser/resources/usb_internals/usb_internals.js:100:
initializeProxies().then(initialize);
The |initialize| method is too small, and only called once, so not really
justifying its existence. Can we inline it here?

https://codereview.chromium.org/2204713006/diff/1/chrome/browser/ui/webui/usb...
File chrome/browser/ui/webui/usb_internals/usb_internals.mojom (right):

https://codereview.chromium.org/2204713006/diff/1/chrome/browser/ui/webui/usb...
chrome/browser/ui/webui/usb_internals/usb_internals.mojom:20: string
allowed_origin) => (bool success, string message);
Ideally we would want the Promise that JS holds to be rejected instead of
resolved, in the case of an error.

addDeviceForTesting(...).then(
   function(message) { // handles only the success case },
   function(error) { // handles only the error case }); // no need for |success|
boolean.

Instead of passing a |success| boolean and handling both success/error in the
success callback. I am not sure if there is currently a way to reject a Promise
from the Mojo-based C++ handler. Perhaps worth asking on chromium-mojo, since
this seems it will come up a lot in the future.

https://codereview.chromium.org/2204713006/diff/1/chrome/browser/ui/webui/usb...
chrome/browser/ui/webui/usb_internals/usb_internals.mojom:23:
RemoveDeviceForTesting(string guid) => ();
It is not clear where the guid is found. Based on the comment here, I was
expecting that |guid| is returned from a call to AddDeviceForTesting, and then
it can be used to remove that device. Looking at the JS code, I am realizing
that AddDeviceForTesting, does not return the |guid|, instead a call to
GetTestDevices() is necessary.

How about changing the signature of AddDeviceForTesting as follows?

AddDeviceForTesting(string name,
                    string serial_number,
                    string landing_page,
                    string allowed_origin) => (bool success, TestDeviceInfo
device);

Or if we figure out the reject/resolve issue mentioned in the previous comment,
this could be even simpler

AddDeviceForTesting(string name,
                    string serial_number,
                    string landing_page,
                    string allowed_origin) => (TestDeviceInfo device);

[Reilly Grant (use Gerrit), 2016-08-05 01:30:55]

https://codereview.chromium.org/2204713006/diff/1/chrome/browser/resources/us...
File chrome/browser/resources/usb_internals/usb_internals.html (right):

https://codereview.chromium.org/2204713006/diff/1/chrome/browser/resources/us...
chrome/browser/resources/usb_internals/usb_internals.html:54: <button
type="submit">Add</button> <span id="add-test-device-result">
On 2016/08/04 at 20:55:34, dpapad wrote:
> This <span> is missing closing tag, </span>

Done.

https://codereview.chromium.org/2204713006/diff/1/chrome/browser/resources/us...
File chrome/browser/resources/usb_internals/usb_internals.js (right):

https://codereview.chromium.org/2204713006/diff/1/chrome/browser/resources/us...
chrome/browser/resources/usb_internals/usb_internals.js:100:
initializeProxies().then(initialize);
On 2016/08/04 at 20:55:34, dpapad wrote:
> The |initialize| method is too small, and only called once, so not really
justifying its existence. Can we inline it here?

Inlined.

https://codereview.chromium.org/2204713006/diff/1/chrome/browser/ui/webui/usb...
File chrome/browser/ui/webui/usb_internals/usb_internals.mojom (right):

https://codereview.chromium.org/2204713006/diff/1/chrome/browser/ui/webui/usb...
chrome/browser/ui/webui/usb_internals/usb_internals.mojom:20: string
allowed_origin) => (bool success, string message);
On 2016/08/04 at 20:55:34, dpapad wrote:
> Ideally we would want the Promise that JS holds to be rejected instead of
resolved, in the case of an error.
> 
> addDeviceForTesting(...).then(
>    function(message) { // handles only the success case },
>    function(error) { // handles only the error case }); // no need for
|success| boolean.
> 
> Instead of passing a |success| boolean and handling both success/error in the
success callback. I am not sure if there is currently a way to reject a Promise
from the Mojo-based C++ handler. Perhaps worth asking on chromium-mojo, since
this seems it will come up a lot in the future.

There is not, because it doesn't translate well to C++ there is no concept of
failure in Mojo calls.

https://codereview.chromium.org/2204713006/diff/1/chrome/browser/ui/webui/usb...
chrome/browser/ui/webui/usb_internals/usb_internals.mojom:23:
RemoveDeviceForTesting(string guid) => ();
On 2016/08/04 at 20:55:34, dpapad wrote:
> It is not clear where the guid is found. Based on the comment here, I was
expecting that |guid| is returned from a call to AddDeviceForTesting, and then
it can be used to remove that device. Looking at the JS code, I am realizing
that AddDeviceForTesting, does not return the |guid|, instead a call to
GetTestDevices() is necessary.
> 
> How about changing the signature of AddDeviceForTesting as follows?
> 
> AddDeviceForTesting(string name,
>                     string serial_number,
>                     string landing_page,
>                     string allowed_origin) => (bool success, TestDeviceInfo
device);
> 
> Or if we figure out the reject/resolve issue mentioned in the previous
comment, this could be even simpler
> 
> AddDeviceForTesting(string name,
>                     string serial_number,
>                     string landing_page,
>                     string allowed_origin) => (TestDeviceInfo device);

It simplifies the JS code if it doesn't have to handle adding and removing
individual entries and just receives the full list via Mojo each time.

[Reilly Grant (use Gerrit), 2016-08-05 01:31:06]

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

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

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 02:38:49]

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

[commit-bot: I haz the power, 2016-08-05 02:38:50]

Dry run: This issue passed the CQ dry run.

[dpapad, 2016-08-05 17:25:11]

LGTM for chrome/browser/*, with some nits.

https://codereview.chromium.org/2204713006/diff/20001/chrome/browser/resource...
File chrome/browser/resources/usb_internals/usb_internals.js (right):

https://codereview.chromium.org/2204713006/diff/20001/chrome/browser/resource...
chrome/browser/resources/usb_internals/usb_internals.js:17: while
(tableBody.firstChild)
Nit: I think you can clear the tableBody element in an easier way as follows,

tableBody.innerHTML = '';

https://codereview.chromium.org/2204713006/diff/20001/chrome/browser/resource...
chrome/browser/resources/usb_internals/usb_internals.js:31:
removeButton.addEventListener('click', function(e) {
Nit: The |e| parameter is not used, so it can be omitted it.

https://codereview.chromium.org/2204713006/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/usb_internals/OWNERS (right):

https://codereview.chromium.org/2204713006/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/usb_internals/OWNERS:3: 
Revert this one?

[Reilly Grant (use Gerrit), 2016-08-05 17:59:53]

reillyg@chromium.org changed reviewers:
+ jam@chromium.org, rsesek@chromium.org
- dbeam@chromium.org

[Reilly Grant (use Gerrit), 2016-08-05 18:08:35]

jam@, please take a look at the chrome/browser and chrome/browser/ui BUILD.gn
changes.
rsesek@, please do an IPC review of the new Mojo interface.

https://codereview.chromium.org/2204713006/diff/20001/chrome/browser/resource...
File chrome/browser/resources/usb_internals/usb_internals.js (right):

https://codereview.chromium.org/2204713006/diff/20001/chrome/browser/resource...
chrome/browser/resources/usb_internals/usb_internals.js:17: while
(tableBody.firstChild)
On 2016/08/05 at 17:25:11, dpapad wrote:
> Nit: I think you can clear the tableBody element in an easier way as follows,
> 
> tableBody.innerHTML = '';

Done.

https://codereview.chromium.org/2204713006/diff/20001/chrome/browser/resource...
chrome/browser/resources/usb_internals/usb_internals.js:31:
removeButton.addEventListener('click', function(e) {
On 2016/08/05 at 17:25:11, dpapad wrote:
> Nit: The |e| parameter is not used, so it can be omitted it.

Done.

https://codereview.chromium.org/2204713006/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/usb_internals/OWNERS (right):

https://codereview.chromium.org/2204713006/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/usb_internals/OWNERS:3: 
On 2016/08/05 at 17:25:11, dpapad wrote:
> Revert this one?

Done.

[Reilly Grant (use Gerrit), 2016-08-05 18:09:02]

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

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

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 18:17:07]

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

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

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

[jam, 2016-08-05 19:55:35]

On 2016/08/05 18:08:35, Reilly Grant wrote:
> jam@, please take a look at the chrome/browser and chrome/browser/ui BUILD.gn
> changes.
lgtm

[Robert Sesek, 2016-08-08 19:11:36]

https://codereview.chromium.org/2204713006/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/usb_internals/usb_internals.mojom (right):

https://codereview.chromium.org/2204713006/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/usb_internals/usb_internals.mojom:11: string
landing_page;
Should this be a GURL?

https://codereview.chromium.org/2204713006/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/usb_internals/usb_internals.mojom:12: string
allowed_origin;
And should this be a url::Origin?

https://codereview.chromium.org/2204713006/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/usb_internals/usb_internals_page_handler.cc
(right):

https://codereview.chromium.org/2204713006/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/usb_internals/usb_internals_page_handler.cc:30: };
DISALLOW_COPY_AND_ASSIGN

https://codereview.chromium.org/2204713006/diff/40001/device/usb/usb_service.cc
File device/usb/usb_service.cc (right):

https://codereview.chromium.org/2204713006/diff/40001/device/usb/usb_service....
device/usb/usb_service.cc:95: DCHECK(!ContainsKey(devices_, device->guid()));
In production, this would allow a testing device to override a real device. Is
that OK?

[Reilly Grant (use Gerrit), 2016-08-08 20:48:25]

https://codereview.chromium.org/2204713006/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/usb_internals/usb_internals.mojom (right):

https://codereview.chromium.org/2204713006/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/usb_internals/usb_internals.mojom:11: string
landing_page;
On 2016/08/08 at 19:11:35, Robert Sesek wrote:
> Should this be a GURL?

It should but since this interface is being called with user input from
Javascript where a proper URL validator is not available (let me know if I'm
wrong about this) it is easier to do the validation in the browser process.

https://codereview.chromium.org/2204713006/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/usb_internals/usb_internals_page_handler.cc
(right):

https://codereview.chromium.org/2204713006/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/usb_internals/usb_internals_page_handler.cc:30: };
On 2016/08/08 at 19:11:36, Robert Sesek wrote:
> DISALLOW_COPY_AND_ASSIGN

Done.

https://codereview.chromium.org/2204713006/diff/40001/device/usb/usb_service.cc
File device/usb/usb_service.cc (right):

https://codereview.chromium.org/2204713006/diff/40001/device/usb/usb_service....
device/usb/usb_service.cc:95: DCHECK(!ContainsKey(devices_, device->guid()));
On 2016/08/08 at 19:11:36, Robert Sesek wrote:
> In production, this would allow a testing device to override a real device. Is
that OK?

The GUIDs are generated randomly so there's no practical way for override a
device. The DCHECK is more of a sanity test in case you a) try to add the same
UsbDevice instance more than once or b) break GUID generation horribly.

[Dan Beam, 2016-08-08 22:40:27]

Description was changed from

==========
Add chrome://usb-internals page for adding and removing test devices.

This patch adds a new WebUI page, chrome://usb-internals with controls
for simulating the connection and disconnection of virtual WebUSB
devices. This will be useful for doing UI testing without the need for
real hardware.

This page will be expanded in the future to add other features useful
for debugging WebUSB such as viewing device information as it has been
parsed by Chrome and overriding descriptors during development.

BUG=625019
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Add chrome://usb-internals page for adding and removing test devices.

This patch adds a new WebUI page, chrome://usb-internals with controls
for simulating the connection and disconnection of virtual WebUSB
devices. This will be useful for doing UI testing without the need for
real hardware.

This page will be expanded in the future to add other features useful
for debugging WebUSB such as viewing device information as it has been
parsed by Chrome and overriding descriptors during development.

BUG=625019
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[Robert Sesek, 2016-08-09 20:42:38]

https://codereview.chromium.org/2204713006/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/usb_internals/usb_internals.mojom (right):

https://codereview.chromium.org/2204713006/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/usb_internals/usb_internals.mojom:11: string
landing_page;
On 2016/08/08 20:48:24, Reilly Grant wrote:
> On 2016/08/08 at 19:11:35, Robert Sesek wrote:
> > Should this be a GURL?
> 
> It should but since this interface is being called with user input from
> Javascript where a proper URL validator is not available (let me know if I'm
> wrong about this) it is easier to do the validation in the browser process.

Is it? It looks like the user input is only for AddDeviceForTesting() and
GetTestDevices() returns this.

[Reilly Grant (use Gerrit), 2016-08-10 01:02:46]

https://codereview.chromium.org/2204713006/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/usb_internals/usb_internals.mojom (right):

https://codereview.chromium.org/2204713006/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/usb_internals/usb_internals.mojom:11: string
landing_page;
On 2016/08/09 at 20:42:37, Robert Sesek wrote:
> On 2016/08/08 20:48:24, Reilly Grant wrote:
> > On 2016/08/08 at 19:11:35, Robert Sesek wrote:
> > > Should this be a GURL?
> > 
> > It should but since this interface is being called with user input from
> > Javascript where a proper URL validator is not available (let me know if I'm
> > wrong about this) it is easier to do the validation in the browser process.
> 
> Is it? It looks like the user input is only for AddDeviceForTesting() and
GetTestDevices() returns this.

Ah, I see what you mean. I was only looking at AddDeviceForTesting().

[Reilly Grant (use Gerrit), 2016-08-10 01:02:50]

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

[commit-bot: I haz the power, 2016-08-10 01:03:45]

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-10 02:39:48]

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

[commit-bot: I haz the power, 2016-08-10 02:39:49]

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...)

[Robert Sesek, 2016-08-10 17:31:07]

lgtm

[Reilly Grant (use Gerrit), 2016-08-10 17:40:01]

The CQ bit was checked by reillyg@chromium.org

[Reilly Grant (use Gerrit), 2016-08-10 17:40:01]

The patchset sent to the CQ was uploaded after l-g-t-m from dpapad@chromium.org,
jam@chromium.org
Link to the patchset: https://codereview.chromium.org/2204713006/#ps80001
(title: "Use typemaped types in the TestDeviceInfo struct.")

[commit-bot: I haz the power, 2016-08-10 17:41:21]

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

Description was changed from

==========
Add chrome://usb-internals page for adding and removing test devices.

This patch adds a new WebUI page, chrome://usb-internals with controls
for simulating the connection and disconnection of virtual WebUSB
devices. This will be useful for doing UI testing without the need for
real hardware.

This page will be expanded in the future to add other features useful
for debugging WebUSB such as viewing device information as it has been
parsed by Chrome and overriding descriptors during development.

BUG=625019
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Add chrome://usb-internals page for adding and removing test devices.

This patch adds a new WebUI page, chrome://usb-internals with controls
for simulating the connection and disconnection of virtual WebUSB
devices. This will be useful for doing UI testing without the need for
real hardware.

This page will be expanded in the future to add other features useful
for debugging WebUSB such as viewing device information as it has been
parsed by Chrome and overriding descriptors during development.

BUG=625019
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[commit-bot: I haz the power, 2016-08-10 18:34:33]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-08-10 18:36:35]

Description was changed from

==========
Add chrome://usb-internals page for adding and removing test devices.

This patch adds a new WebUI page, chrome://usb-internals with controls
for simulating the connection and disconnection of virtual WebUSB
devices. This will be useful for doing UI testing without the need for
real hardware.

This page will be expanded in the future to add other features useful
for debugging WebUSB such as viewing device information as it has been
parsed by Chrome and overriding descriptors during development.

BUG=625019
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Add chrome://usb-internals page for adding and removing test devices.

This patch adds a new WebUI page, chrome://usb-internals with controls
for simulating the connection and disconnection of virtual WebUSB
devices. This will be useful for doing UI testing without the need for
real hardware.

This page will be expanded in the future to add other features useful
for debugging WebUSB such as viewing device information as it has been
parsed by Chrome and overriding descriptors during development.

BUG=625019
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/3903a69e08972bf282615d725b918109e5cb7a44
Cr-Commit-Position: refs/heads/master@{#411091}
==========

[commit-bot: I haz the power, 2016-08-10 18:36:37]

Patchset 5 (id:??) landed as
https://crrev.com/3903a69e08972bf282615d725b918109e5cb7a44
Cr-Commit-Position: refs/heads/master@{#411091}