Track USB device interface claim state in Blink.

Track USB device interface claim state in Blink.

This patch updates the WebUSB IDL to include a 'claimed' attribute on
USBInterface and the necessary renderer-side logic to keep track of this
state.

The most interesting part of this is continuing to work out what
operations on the device should block other operations because they
change the device state in a way that effects argument validation.

BUG=593164
Committed: https://crrev.com/8953a7b9ffec800ce8a36e44a7c27a5f7725e33d
Cr-Commit-Position: refs/heads/master@{#382407}

[Reilly Grant (use Gerrit), 2016-03-16 23:32:39]

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

[Reilly Grant (use Gerrit), 2016-03-16 23:32:40]

Please take a look.

[juncai, 2016-03-17 01:21:12]

https://codereview.chromium.org/1814603002/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/usb/usbDevice.html (right):

https://codereview.chromium.org/1814603002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/usb/usbDevice.html:255: }, 'a non-existant
interface cannot be claimed or released');
typo:
existant -> existent

https://codereview.chromium.org/1814603002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webusb/USBConfiguration.h (right):

https://codereview.chromium.org/1814603002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webusb/USBConfiguration.h:30: size_t index()
const;
maybe use: configurationIndex() as function name? Similar to other functions
such as: configurationValue() and configurationName().

https://codereview.chromium.org/1814603002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webusb/USBDevice.cpp (right):

https://codereview.chromium.org/1814603002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webusb/USBDevice.cpp:146: class
ClaimInterfacePromiseAdapter : public WebCallbacks<void, const WebUSBError&> {
Is class ClaimInterfacePromiseAdapter used anywhere? I did a search and didn't
find any code use it.

If this class is not used, function onInterfaceClaimedOrUnclaimed can also be
removed since it seems only this class uses it.

https://codereview.chromium.org/1814603002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webusb/USBDevice.cpp:289:
onConfigurationSelected(true, configurationIndex);
nit: add comment for true parameter.

https://codereview.chromium.org/1814603002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webusb/USBDevice.cpp:348:
resolver->reject(DOMException::create(InvalidStateError,
kInterfaceStateChangeInProgress));
USBDevice::open, USBDevice::close and USBDevice::selectConfiguration all use the
above if .. else if:

if (m_deviceStateChangeInProgress) {
    resolver->reject(DOMException::create(InvalidStateError, kDeviceStateCha
     ngeInProgress));
} else if (anyInterfaceChangeInProgress()) {
    resolver->reject(DOMException::create(InvalidStateError, kInterfaceState
     ChangeInProgress));
}

Maybe can create a function to avoid code duplication here.

https://codereview.chromium.org/1814603002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webusb/USBDevice.cpp:407: } else if
(m_interfaceStateChangeInProgress.get(interfaceIndex)) {
Use static_cast<size_t>(interfaceIndex) since: get(size_t bit).

Or maybe it's better to use size_t interfaceIndex here and change the return
type of function: USBDevice::findInterfaceIndex to be size_t instead of int.

Noticed that USBDevice::isInterfaceClaimed function uses size_t interfaceIndex
and USBInterface class has a size_t m_interfaceIndex member variable.

https://codereview.chromium.org/1814603002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webusb/USBDevice.cpp:578: int
USBDevice::findInterfaceIndex(uint8_t interfaceNumber) const
This function may need to return size_t.

https://codereview.chromium.org/1814603002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webusb/USBDevice.h (right):

https://codereview.chromium.org/1814603002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webusb/USBDevice.h:47: void
onInterfaceClaimedOrUnclaimed(bool claimed, int interfaceIndex);
interfaceIndex is int type in this function, but is size_t in function
isInterfaceClaimed(). It is used as a parameter in WTF::BitVector::get(size_t
bit) and set(size_t bit).

[juncai, 2016-03-17 05:59:35]

https://codereview.chromium.org/1814603002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webusb/USBDevice.cpp (right):

https://codereview.chromium.org/1814603002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webusb/USBDevice.cpp:146: class
ClaimInterfacePromiseAdapter : public WebCallbacks<void, const WebUSBError&> {
On 2016/03/17 01:21:11, juncai wrote:
> Is class ClaimInterfacePromiseAdapter used anywhere? I did a search and didn't
> find any code use it.
> 
> If this class is not used, function onInterfaceClaimedOrUnclaimed can also be
> removed since it seems only this class uses it.

ah, please ignore this comment. I just realized that I used
"ClaimInterfacePromiseAdapter" to search it, but the class name is divided into
two lines where this class is used.

[Reilly Grant (use Gerrit), 2016-03-17 17:07:01]

https://codereview.chromium.org/1814603002/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/usb/usbDevice.html (right):

https://codereview.chromium.org/1814603002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/usb/usbDevice.html:255: }, 'a non-existant
interface cannot be claimed or released');
On 2016/03/17 at 01:21:11, juncai wrote:
> typo:
> existant -> existent

Fixed.

https://codereview.chromium.org/1814603002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webusb/USBConfiguration.h (right):

https://codereview.chromium.org/1814603002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webusb/USBConfiguration.h:30: size_t index()
const;
On 2016/03/17 at 01:21:11, juncai wrote:
> maybe use: configurationIndex() as function name? Similar to other functions
such as: configurationValue() and configurationName().

The name configurationValue is verbose because the WebIDL matches the USB
specification's bConfigurationValue field for clarity. I'd rather write
configuration->index() than configuration->configurationIndex() if I have a
choice about it.

https://codereview.chromium.org/1814603002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webusb/USBDevice.cpp (right):

https://codereview.chromium.org/1814603002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webusb/USBDevice.cpp:146: class
ClaimInterfacePromiseAdapter : public WebCallbacks<void, const WebUSBError&> {
On 2016/03/17 at 01:21:11, juncai wrote:
> Is class ClaimInterfacePromiseAdapter used anywhere? I did a search and didn't
find any code use it.
> 
> If this class is not used, function onInterfaceClaimedOrUnclaimed can also be
removed since it seems only this class uses it.

It is used in claimInterface and releaseInterface.

https://codereview.chromium.org/1814603002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webusb/USBDevice.cpp:289:
onConfigurationSelected(true, configurationIndex);
On 2016/03/17 at 01:21:11, juncai wrote:
> nit: add comment for true parameter.

Done.

https://codereview.chromium.org/1814603002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webusb/USBDevice.cpp:348:
resolver->reject(DOMException::create(InvalidStateError,
kInterfaceStateChangeInProgress));
On 2016/03/17 at 01:21:11, juncai wrote:
> USBDevice::open, USBDevice::close and USBDevice::selectConfiguration all use
the above if .. else if:
> 
> if (m_deviceStateChangeInProgress) {
>     resolver->reject(DOMException::create(InvalidStateError, kDeviceStateCha
>      ngeInProgress));
> } else if (anyInterfaceChangeInProgress()) {
>     resolver->reject(DOMException::create(InvalidStateError, kInterfaceState
>      ChangeInProgress));
> }
> 
> Maybe can create a function to avoid code duplication here.

Done.

https://codereview.chromium.org/1814603002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webusb/USBDevice.cpp:407: } else if
(m_interfaceStateChangeInProgress.get(interfaceIndex)) {
On 2016/03/17 at 01:21:11, juncai wrote:
> Use static_cast<size_t>(interfaceIndex) since: get(size_t bit).
> 
> Or maybe it's better to use size_t interfaceIndex here and change the return
type of function: USBDevice::findInterfaceIndex to be size_t instead of int.
> 
> Noticed that USBDevice::isInterfaceClaimed function uses size_t interfaceIndex
and USBInterface class has a size_t m_interfaceIndex member variable.

Done.

https://codereview.chromium.org/1814603002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webusb/USBDevice.cpp:578: int
USBDevice::findInterfaceIndex(uint8_t interfaceNumber) const
On 2016/03/17 at 01:21:11, juncai wrote:
> This function may need to return size_t.

It can't return -1 if it returns size_t.

https://codereview.chromium.org/1814603002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webusb/USBDevice.h (right):

https://codereview.chromium.org/1814603002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webusb/USBDevice.h:47: void
onInterfaceClaimedOrUnclaimed(bool claimed, int interfaceIndex);
On 2016/03/17 at 01:21:12, juncai wrote:
> interfaceIndex is int type in this function, but is size_t in function
isInterfaceClaimed(). It is used as a parameter in WTF::BitVector::get(size_t
bit) and set(size_t bit).

Fixed.

[juncai, 2016-03-17 18:20:38]

https://codereview.chromium.org/1814603002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webusb/USBDevice.cpp (right):

https://codereview.chromium.org/1814603002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webusb/USBDevice.cpp:401: } else if
(m_interfaceStateChangeInProgress.get(interfaceIndex)) {
nit: may need to use static_cast<size_t> here since interfaceIndex is int type.

https://codereview.chromium.org/1814603002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webusb/USBDevice.cpp:403: } else if
(m_claimedInterfaces.get(interfaceIndex)) {
ditto.

https://codereview.chromium.org/1814603002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webusb/USBDevice.cpp:406:
m_interfaceStateChangeInProgress.set(interfaceIndex);
ditto.

https://codereview.chromium.org/1814603002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webusb/USBDevice.cpp:407:
m_device->claimInterface(interfaceNumber, new ClaimInterfacePromiseAdapter(this,
resolver, interfaceIndex, true));
nit: may need to use static_cast<size_t> here since interfaceIndex is int type.

Also need to add comments about the true parameter.

https://codereview.chromium.org/1814603002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webusb/USBDevice.cpp:421: } else if
(m_interfaceStateChangeInProgress.get(interfaceIndex)) {
ditto.

https://codereview.chromium.org/1814603002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webusb/USBDevice.cpp:423: } else if
(!m_claimedInterfaces.get(interfaceIndex)) {
ditto.

https://codereview.chromium.org/1814603002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webusb/USBDevice.cpp:426:
m_interfaceStateChangeInProgress.set(interfaceIndex);
ditto.

https://codereview.chromium.org/1814603002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webusb/USBDevice.cpp:427:
m_device->releaseInterface(interfaceNumber, new
ClaimInterfacePromiseAdapter(this, resolver, interfaceIndex, false));
nit: may need to use static_cast<size_t> here since interfaceIndex is int type.

Also need to add comments for false parameter.

https://codereview.chromium.org/1814603002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webusb/USBDevice.cpp:502: if
(ensureEndpointAvailable(true, endpointNumber, resolver))
nit: add comments for true parameter.

https://codereview.chromium.org/1814603002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webusb/USBDevice.cpp:511: if
(ensureEndpointAvailable(false, endpointNumber, resolver)) {
nit: add comments for false parameter.

https://codereview.chromium.org/1814603002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webusb/USBDevice.cpp:522: if
(ensureEndpointAvailable(true, endpointNumber, resolver))
nit: add comments for true parameter.

https://codereview.chromium.org/1814603002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webusb/USBDevice.cpp:531: if
(ensureEndpointAvailable(false, endpointNumber, resolver)) {
nit: add comments for false parameter.

https://codereview.chromium.org/1814603002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webusb/USBDevice.cpp:578: return i;
nit: static_cast<int>(i) since i is size_t type.

https://codereview.chromium.org/1814603002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webusb/USBDevice.cpp:616: } else if
(m_interfaceStateChangeInProgress.get(interfaceIndex)) {
nit: may need to use static_cast<size_t> here since interfaceIndex is int type.

https://codereview.chromium.org/1814603002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webusb/USBDevice.cpp:618: } else if
(!m_claimedInterfaces.get(interfaceIndex)) {
ditto.

https://codereview.chromium.org/1814603002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webusb/USBInterface.cpp (right):

https://codereview.chromium.org/1814603002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webusb/USBInterface.cpp:21: for (size_t i = 0;
i < configuration->info().interfaces.size(); ++i) {
configuration->info() is a function call, to avoid it in a for loop, maybe can
store it in a variable.

https://codereview.chromium.org/1814603002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webusb/USBInterface.cpp:22: if
(configuration->info().interfaces[i].interfaceNumber == interfaceNumber)
ditto.

https://codereview.chromium.org/1814603002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webusb/USBInterface.cpp:46: for (size_t i = 0;
i < info().alternates.size(); ++i)
ditto.

[Reilly Grant (use Gerrit), 2016-03-18 00:10:22]

https://codereview.chromium.org/1814603002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webusb/USBDevice.cpp (right):

https://codereview.chromium.org/1814603002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webusb/USBDevice.cpp:401: } else if
(m_interfaceStateChangeInProgress.get(interfaceIndex)) {
On 2016/03/17 at 18:20:37, juncai wrote:
> nit: may need to use static_cast<size_t> here since interfaceIndex is int
type.

The compiler is not complaining so I'm going to leave out the static_casts for
the moment.

https://codereview.chromium.org/1814603002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webusb/USBDevice.cpp:407:
m_device->claimInterface(interfaceNumber, new ClaimInterfacePromiseAdapter(this,
resolver, interfaceIndex, true));
On 2016/03/17 at 18:20:37, juncai wrote:
> Also need to add comments about the true parameter.

Done.

https://codereview.chromium.org/1814603002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webusb/USBDevice.cpp:427:
m_device->releaseInterface(interfaceNumber, new
ClaimInterfacePromiseAdapter(this, resolver, interfaceIndex, false));
On 2016/03/17 at 18:20:37, juncai wrote:
> Also need to add comments for false parameter.

Done.

https://codereview.chromium.org/1814603002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webusb/USBDevice.cpp:502: if
(ensureEndpointAvailable(true, endpointNumber, resolver))
On 2016/03/17 at 18:20:38, juncai wrote:
> nit: add comments for true parameter.

Done.

https://codereview.chromium.org/1814603002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webusb/USBDevice.cpp:511: if
(ensureEndpointAvailable(false, endpointNumber, resolver)) {
On 2016/03/17 at 18:20:37, juncai wrote:
> nit: add comments for false parameter.

Done.

https://codereview.chromium.org/1814603002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webusb/USBDevice.cpp:522: if
(ensureEndpointAvailable(true, endpointNumber, resolver))
On 2016/03/17 at 18:20:37, juncai wrote:
> nit: add comments for true parameter.

Done.

https://codereview.chromium.org/1814603002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webusb/USBDevice.cpp:531: if
(ensureEndpointAvailable(false, endpointNumber, resolver)) {
On 2016/03/17 at 18:20:37, juncai wrote:
> nit: add comments for false parameter.

Done.

https://codereview.chromium.org/1814603002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webusb/USBInterface.cpp (right):

https://codereview.chromium.org/1814603002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webusb/USBInterface.cpp:21: for (size_t i = 0;
i < configuration->info().interfaces.size(); ++i) {
On 2016/03/17 at 18:20:38, juncai wrote:
> configuration->info() is a function call, to avoid it in a for loop, maybe can
store it in a variable.

Done.

[juncai, 2016-03-18 00:18:28]

LGTM.

[Reilly Grant (use Gerrit), 2016-03-18 01:12:12]

The CQ bit was checked by reillyg@chromium.org

[commit-bot: I haz the power, 2016-03-18 01:12:41]

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

[commit-bot: I haz the power, 2016-03-18 01:16:32]

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

[commit-bot: I haz the power, 2016-03-18 01:16:33]

Try jobs failed on following builders:
  android_arm64_dbg_recipe on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_chromium_gn_compile_dbg on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...)
  android_chromium_gn_compile_rel on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...)
  android_clang_dbg_recipe on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  cast_shell_android on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  cast_shell_linux on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  chromeos_x86-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
  linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_compile_dbg_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_clobber_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Reilly Grant (use Gerrit), 2016-03-21 20:50:30]

The CQ bit was checked by reillyg@chromium.org

[Reilly Grant (use Gerrit), 2016-03-21 20:50:31]

The patchset sent to the CQ was uploaded after l-g-t-m from juncai@chromium.org
Link to the patchset: https://codereview.chromium.org/1814603002/#ps80001
(title: "Rebased.")

[commit-bot: I haz the power, 2016-03-21 20:50:46]

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

[commit-bot: I haz the power, 2016-03-21 22:02:46]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-03-21 22:04:22]

Description was changed from

==========
Track USB device interface claim state in Blink.

This patch updates the WebUSB IDL to include a 'claimed' attribute on
USBInterface and the necessary renderer-side logic to keep track of this
state.

The most interesting part of this is continuing to work out what
operations on the device should block other operations because they
change the device state in a way that effects argument validation.

BUG=593164
==========

to

==========
Track USB device interface claim state in Blink.

This patch updates the WebUSB IDL to include a 'claimed' attribute on
USBInterface and the necessary renderer-side logic to keep track of this
state.

The most interesting part of this is continuing to work out what
operations on the device should block other operations because they
change the device state in a way that effects argument validation.

BUG=593164

Committed: https://crrev.com/8953a7b9ffec800ce8a36e44a7c27a5f7725e33d
Cr-Commit-Position: refs/heads/master@{#382407}
==========

[commit-bot: I haz the power, 2016-03-21 22:04:23]

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