Add WebUSB bindings and client interface [part 1]

Add WebUSB bindings and client interface [part 1]

This is part 1 of a multipart change set to add WebUSB
bindings to Blink. In this CL, a basic client interface
is added for device enumeration.

The original full CL is here: 
https://codereview.chromium.org/1245363002/

And the WebUSB draft spec is here:
http://reillyeon.github.io/webusb/

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

[Ken Rockot(use gerrit already), 2015-07-29 17:57:07]

rockot@chromium.org changed reviewers:
+ haraken@chromium.org, reillyg@chromium.org

[Ken Rockot(use gerrit already), 2015-07-29 17:57:08]

Please let me know if this is still too big :)  Lots of files but fairly small
ones.

[haraken, 2015-07-29 18:02:43]

I just want to confirm, but when and where will tests be added? I cannot find
tests in the original CL: https://codereview.chromium.org/1245363002/

[haraken, 2015-07-29 18:08:03]

I'd be happy if you could land things more incrementally, but it's hard, I can
take care of this size (I'll take a detailed look tomorrow morning).

[Ken Rockot(use gerrit already), 2015-07-29 19:05:19]

On 2015/07/29 18:02:43, haraken wrote:
> I just want to confirm, but when and where will tests be added? I cannot find
> tests in the original CL: https://codereview.chromium.org/1245363002/

I could add layout tests now but they wouldn't effectively be run until we
landed stuff in content right? If that's the case, I can add layout tests along
with chrome patches to content.

[haraken, 2015-07-29 19:30:50]

On 2015/07/29 19:05:19, Ken Rockot wrote:
> On 2015/07/29 18:02:43, haraken wrote:
> > I just want to confirm, but when and where will tests be added? I cannot
find
> > tests in the original CL: https://codereview.chromium.org/1245363002/
> 
> I could add layout tests now but they wouldn't effectively be run until we
> landed stuff in content right? If that's the case, I can add layout tests
along
> with chrome patches to content.

Then it would make sense to add the tests when they run effectively.

[haraken, 2015-07-30 11:26:52]

Mostly looks good.

https://codereview.chromium.org/1262163003/diff/1/Source/modules/webusb/Navig...
File Source/modules/webusb/NavigatorUSB.cpp (right):

https://codereview.chromium.org/1262163003/diff/1/Source/modules/webusb/Navig...
Source/modules/webusb/NavigatorUSB.cpp:13: // static

Drop the comment.

https://codereview.chromium.org/1262163003/diff/1/Source/modules/webusb/Navig...
Source/modules/webusb/NavigatorUSB.cpp:24: // static

Drop the comment.

https://codereview.chromium.org/1262163003/diff/1/Source/modules/webusb/Navig...
Source/modules/webusb/NavigatorUSB.cpp:32: return m_usb.get();

.get() won't be needed.

https://codereview.chromium.org/1262163003/diff/1/Source/modules/webusb/Navig...
Source/modules/webusb/NavigatorUSB.cpp:42: :
m_usb(USB::create(*navigator.frame()))

Is it guaranteed that navigator.frame() returns a non-null frame?

https://codereview.chromium.org/1262163003/diff/1/Source/modules/webusb/Navig...
File Source/modules/webusb/NavigatorUSB.h (right):

https://codereview.chromium.org/1262163003/diff/1/Source/modules/webusb/Navig...
Source/modules/webusb/NavigatorUSB.h:9: #include "platform/heap/Heap.h"

Heap.h => Handle.h (We normally include it.)

https://codereview.chromium.org/1262163003/diff/1/Source/modules/webusb/Navig...
File Source/modules/webusb/NavigatorUSB.idl (right):

https://codereview.chromium.org/1262163003/diff/1/Source/modules/webusb/Navig...
Source/modules/webusb/NavigatorUSB.idl:4: 

Would you add a spec link to each IDL file?

https://codereview.chromium.org/1262163003/diff/1/Source/modules/webusb/USB.cpp
File Source/modules/webusb/USB.cpp (right):

https://codereview.chromium.org/1262163003/diff/1/Source/modules/webusb/USB.c...
Source/modules/webusb/USB.cpp:12: #include
"public/platform/modules/webusb/WebUSBDeviceFilter.h"

Some of the header files won't be necessary in this CL, but I'm ok with adding
them in this CL.

https://codereview.chromium.org/1262163003/diff/1/Source/modules/webusb/USB.c...
Source/modules/webusb/USB.cpp:28: #if ENABLE(OILPAN)

This wouldn't be necessary.

https://codereview.chromium.org/1262163003/diff/1/Source/modules/webusb/USBCo...
File Source/modules/webusb/USBController.cpp (right):

https://codereview.chromium.org/1262163003/diff/1/Source/modules/webusb/USBCo...
Source/modules/webusb/USBController.cpp:24: USBController*
USBController::from(LocalFrame& frame)

Can we add an ASSERT to verify that the from doesn't return a nullptr? (i.e.,
provideTo should have been called before we reach here.)

https://codereview.chromium.org/1262163003/diff/1/Source/modules/webusb/USBDe...
File Source/modules/webusb/USBDevice.cpp (right):

https://codereview.chromium.org/1262163003/diff/1/Source/modules/webusb/USBDe...
Source/modules/webusb/USBDevice.cpp:13: return new USBDevice;

You can inline these simple methods.

https://codereview.chromium.org/1262163003/diff/1/Source/modules/webusb/USBDe...
Source/modules/webusb/USBDevice.cpp:18: }

Ditto.

[Ken Rockot(use gerrit already), 2015-07-30 18:06:55]

https://codereview.chromium.org/1262163003/diff/1/Source/modules/webusb/Navig...
File Source/modules/webusb/NavigatorUSB.cpp (right):

https://codereview.chromium.org/1262163003/diff/1/Source/modules/webusb/Navig...
Source/modules/webusb/NavigatorUSB.cpp:13: // static
On 2015/07/30 11:26:52, haraken wrote:
> 
> Drop the comment.

Done.

https://codereview.chromium.org/1262163003/diff/1/Source/modules/webusb/Navig...
Source/modules/webusb/NavigatorUSB.cpp:24: // static
On 2015/07/30 11:26:51, haraken wrote:
> 
> Drop the comment.

Done.

https://codereview.chromium.org/1262163003/diff/1/Source/modules/webusb/Navig...
Source/modules/webusb/NavigatorUSB.cpp:32: return m_usb.get();
On 2015/07/30 11:26:52, haraken wrote:
> 
> .get() won't be needed.

Done.

https://codereview.chromium.org/1262163003/diff/1/Source/modules/webusb/Navig...
Source/modules/webusb/NavigatorUSB.cpp:42: :
m_usb(USB::create(*navigator.frame()))
On 2015/07/30 11:26:52, haraken wrote:
> 
> Is it guaranteed that navigator.frame() returns a non-null frame?

That assumption seems to be made in a lot of places - whether or not it's
correct probably depends on context. It looks like frame() can only be null
after DOMWindowProperty::willDestroyGlobalObjectInFrame is called on the
Navigator instance.

In any case I've changed this so that m_usb will be null if navigator.frame() is
null upon NavigatorUSB construction. I think that should be OK.

https://codereview.chromium.org/1262163003/diff/1/Source/modules/webusb/Navig...
File Source/modules/webusb/NavigatorUSB.h (right):

https://codereview.chromium.org/1262163003/diff/1/Source/modules/webusb/Navig...
Source/modules/webusb/NavigatorUSB.h:9: #include "platform/heap/Heap.h"
On 2015/07/30 11:26:52, haraken wrote:
> 
> Heap.h => Handle.h (We normally include it.)

Done.

https://codereview.chromium.org/1262163003/diff/1/Source/modules/webusb/Navig...
File Source/modules/webusb/NavigatorUSB.idl (right):

https://codereview.chromium.org/1262163003/diff/1/Source/modules/webusb/Navig...
Source/modules/webusb/NavigatorUSB.idl:4: 
On 2015/07/30 11:26:52, haraken wrote:
> 
> Would you add a spec link to each IDL file?

Done.

https://codereview.chromium.org/1262163003/diff/1/Source/modules/webusb/USB.cpp
File Source/modules/webusb/USB.cpp (right):

https://codereview.chromium.org/1262163003/diff/1/Source/modules/webusb/USB.c...
Source/modules/webusb/USB.cpp:12: #include
"public/platform/modules/webusb/WebUSBDeviceFilter.h"
On 2015/07/30 11:26:52, haraken wrote:
> 
> Some of the header files won't be necessary in this CL, but I'm ok with adding
> them in this CL.

Acknowledged.

https://codereview.chromium.org/1262163003/diff/1/Source/modules/webusb/USB.c...
Source/modules/webusb/USB.cpp:28: #if ENABLE(OILPAN)
On 2015/07/30 11:26:52, haraken wrote:
> 
> This wouldn't be necessary.

Done.

https://codereview.chromium.org/1262163003/diff/1/Source/modules/webusb/USBCo...
File Source/modules/webusb/USBController.cpp (right):

https://codereview.chromium.org/1262163003/diff/1/Source/modules/webusb/USBCo...
Source/modules/webusb/USBController.cpp:24: USBController*
USBController::from(LocalFrame& frame)
On 2015/07/30 11:26:52, haraken wrote:
> 
> Can we add an ASSERT to verify that the from doesn't return a nullptr? (i.e.,
> provideTo should have been called before we reach here.)

Done.

https://codereview.chromium.org/1262163003/diff/1/Source/modules/webusb/USBDe...
File Source/modules/webusb/USBDevice.cpp (right):

https://codereview.chromium.org/1262163003/diff/1/Source/modules/webusb/USBDe...
Source/modules/webusb/USBDevice.cpp:13: return new USBDevice;
On 2015/07/30 11:26:52, haraken wrote:
> 
> You can inline these simple methods.

Done.

https://codereview.chromium.org/1262163003/diff/1/Source/modules/webusb/USBDe...
Source/modules/webusb/USBDevice.cpp:18: }
On 2015/07/30 11:26:52, haraken wrote:
> 
> Ditto.

Done.

[haraken, 2015-07-30 20:18:00]

LGTM

You need to get an approval from public/owner.

https://codereview.chromium.org/1262163003/diff/20001/Source/modules/webusb/N...
File Source/modules/webusb/NavigatorUSB.cpp (right):

https://codereview.chromium.org/1262163003/diff/20001/Source/modules/webusb/N...
Source/modules/webusb/NavigatorUSB.cpp:41: if (navigator.frame())

Actually I'm not sure if we should expect that NavigatorUSB::from() gets called
for a frame that is already detached. If you think that is not an expected
situation, you can change this to ASSERT(navigator.frame()).

https://codereview.chromium.org/1262163003/diff/20001/Source/modules/webusb/U...
File Source/modules/webusb/USB.h (right):

https://codereview.chromium.org/1262163003/diff/20001/Source/modules/webusb/U...
Source/modules/webusb/USB.h:29: USB(LocalFrame& frame);

Add explicit.

https://codereview.chromium.org/1262163003/diff/20001/Source/modules/webusb/U...
File Source/modules/webusb/USBController.cpp (right):

https://codereview.chromium.org/1262163003/diff/20001/Source/modules/webusb/U...
Source/modules/webusb/USBController.cpp:24: USBController*
USBController::from(LocalFrame& frame)

Given that from() shouldn't return a nullptr, the return type could be
USBController&.

[haraken, 2015-07-30 20:20:19]

Also please add more information to the CL description so that others can follow
(the original CL, the link to the intent-to-implement etc).

[Ken Rockot(use gerrit already), 2015-07-30 21:09:50]

Patchset #3 (id:40001) has been deleted

[Ken Rockot(use gerrit already), 2015-07-30 21:10:34]

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

[Ken Rockot(use gerrit already), 2015-07-30 21:10:34]

+dcheng could you please take a look as public/ OWNER?

https://codereview.chromium.org/1262163003/diff/20001/Source/modules/webusb/N...
File Source/modules/webusb/NavigatorUSB.cpp (right):

https://codereview.chromium.org/1262163003/diff/20001/Source/modules/webusb/N...
Source/modules/webusb/NavigatorUSB.cpp:41: if (navigator.frame())
On 2015/07/30 20:17:59, haraken wrote:
> 
> Actually I'm not sure if we should expect that NavigatorUSB::from() gets
called
> for a frame that is already detached. If you think that is not an expected
> situation, you can change this to ASSERT(navigator.frame()).

OK thought maybe this was true. Done.

https://codereview.chromium.org/1262163003/diff/20001/Source/modules/webusb/U...
File Source/modules/webusb/USB.h (right):

https://codereview.chromium.org/1262163003/diff/20001/Source/modules/webusb/U...
Source/modules/webusb/USB.h:29: USB(LocalFrame& frame);
On 2015/07/30 20:17:59, haraken wrote:
> 
> Add explicit.

Done.

https://codereview.chromium.org/1262163003/diff/20001/Source/modules/webusb/U...
File Source/modules/webusb/USBController.cpp (right):

https://codereview.chromium.org/1262163003/diff/20001/Source/modules/webusb/U...
Source/modules/webusb/USBController.cpp:24: USBController*
USBController::from(LocalFrame& frame)
On 2015/07/30 20:18:00, haraken wrote:
> 
> Given that from() shouldn't return a nullptr, the return type could be
> USBController&.

Done.

[dcheng, 2015-07-30 21:18:10]

https://codereview.chromium.org/1262163003/diff/60001/public/platform/modules...
File public/platform/modules/webusb/WebUSBClient.h (right):

https://codereview.chromium.org/1262163003/diff/60001/public/platform/modules...
public/platform/modules/webusb/WebUSBClient.h:17: typedef
WebCallbacks<WebVector<WebUSBDevice*>*, WebUSBError*>
WebUSBClientGetDevicesCallbacks;
Prefer 'using' to 'typedef' for consistency with the C++11 guidelines that
Chrome uses?

https://codereview.chromium.org/1262163003/diff/60001/public/web/WebFrameClie...
File public/web/WebFrameClient.h (right):

https://codereview.chromium.org/1262163003/diff/60001/public/web/WebFrameClie...
public/web/WebFrameClient.h:674: virtual WebUSBClient* usbClient() { return 0; }
nullptr

[Ken Rockot(use gerrit already), 2015-07-30 21:20:14]

https://codereview.chromium.org/1262163003/diff/60001/public/platform/modules...
File public/platform/modules/webusb/WebUSBClient.h (right):

https://codereview.chromium.org/1262163003/diff/60001/public/platform/modules...
public/platform/modules/webusb/WebUSBClient.h:17: typedef
WebCallbacks<WebVector<WebUSBDevice*>*, WebUSBError*>
WebUSBClientGetDevicesCallbacks;
On 2015/07/30 21:18:10, dcheng wrote:
> Prefer 'using' to 'typedef' for consistency with the C++11 guidelines that
> Chrome uses?

Done.

https://codereview.chromium.org/1262163003/diff/60001/public/web/WebFrameClie...
File public/web/WebFrameClient.h (right):

https://codereview.chromium.org/1262163003/diff/60001/public/web/WebFrameClie...
public/web/WebFrameClient.h:674: virtual WebUSBClient* usbClient() { return 0; }
On 2015/07/30 21:18:10, dcheng wrote:
> nullptr

Done.

[dcheng, 2015-07-30 21:20:37]

lgtm

[Ken Rockot(use gerrit already), 2015-07-30 23:23:05]

https://codereview.chromium.org/1262163003/diff/20001/Source/modules/webusb/N...
File Source/modules/webusb/NavigatorUSB.cpp (right):

https://codereview.chromium.org/1262163003/diff/20001/Source/modules/webusb/N...
Source/modules/webusb/NavigatorUSB.cpp:41: if (navigator.frame())
On 2015/07/30 21:10:34, Ken Rockot wrote:
> On 2015/07/30 20:17:59, haraken wrote:
> > 
> > Actually I'm not sure if we should expect that NavigatorUSB::from() gets
> called
> > for a frame that is already detached. If you think that is not an expected
> > situation, you can change this to ASSERT(navigator.frame()).
> 
> OK thought maybe this was true. Done.

So actually there are layout tests which seem to exercise this possibility
(fast/dom/navigator-detached-no-crash.html for one). I've gone back to not
ASSERTing, and letting m_usb remain null in that case.

[Ken Rockot(use gerrit already), 2015-07-30 23:23:52]

The CQ bit was checked by rockot@chromium.org

[Ken Rockot(use gerrit already), 2015-07-30 23:23:52]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/1262163003/#ps160001
(title: "Handle null navigator.frame() without ASSERT")

[commit-bot: I haz the power, 2015-07-30 23:24:04]

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

[commit-bot: I haz the power, 2015-07-30 23:50:59]

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

[commit-bot: I haz the power, 2015-07-30 23:50:59]

Try jobs failed on following builders:
  win_blink_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/72104)

[Ken Rockot(use gerrit already), 2015-07-31 01:09:51]

The CQ bit was checked by rockot@chromium.org

[commit-bot: I haz the power, 2015-07-31 01:10:05]

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

[commit-bot: I haz the power, 2015-07-31 02:16:12]

Committed patchset #8 (id:160001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=199777