Consume Mojo services directly in Blink's WebUSB implementation.

Consume Mojo services directly in Blink's WebUSB implementation.

This patch takes advantage of the ability to consume a Mojo service
directly in //third_party/WebKit/Source/modules to remove the
//third_party/WebKit/public/platform/modules/webusb,
//content/renderer/usb and the rest of that intermediate layer.

BUG=None

Committed: https://crrev.com/1b995c58c463593dc156931956a3e4ace76b5e52
Cr-Commit-Position: refs/heads/master@{#388571}

[Reilly Grant (use Gerrit), 2016-04-06 19:50:13]

Patchset #1 (id:1) has been deleted

[Reilly Grant (use Gerrit), 2016-04-06 19:53:17]

Description was changed from

==========
Onion Soup WebUSB

BUG=None
==========

to

==========
Consume Mojo services directly in Blink's WebUSB implementation.

This patch takes advantage of the ability to consume a Mojo service
directly in //third_party/WebKit/Source/modules to remove the
//third_party/WebKit/public/platform/modules/webusb,
//content/renderer/usb and the rest of that intermediate layer.

BUG=None
==========

[Reilly Grant (use Gerrit), 2016-04-06 19:53:17]

reillyg@chromium.org changed reviewers:
+ esprehn@chromium.org, pfeldman@chromium.org

[Reilly Grant (use Gerrit), 2016-04-06 19:53:44]

Please take a look.

[esprehn, 2016-04-06 20:10:45]

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

https://codereview.chromium.org/1850023002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webusb/USB.cpp:114: // a
mojo::Callback::Runnable's const Run() method.
this is really weird, callback interfaces generally don't put const so they
don't impose a restriction like this. Can we fix mojo?

https://codereview.chromium.org/1850023002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webusb/USB.cpp:134: for (size_t i = 0; i <
deviceInfos.size(); ++i) {
range loop?

https://codereview.chromium.org/1850023002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webusb/USB.cpp:186:
m_deviceManager->GetDeviceChanges(adapter->callback());
this seems to just do the exact same thing twice in a row? Why do that?

https://codereview.chromium.org/1850023002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webusb/USB.cpp:217: LocalFrame* frame =
getExecutionContext() ? static_cast<Document*>(getExecutionContext())->frame() :
nullptr;
toDocument() don't static_cast types in blink, our toFoo() macros enforce type
checks

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

https://codereview.chromium.org/1850023002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webusb/USB.h:49:
device::usb::blink::DeviceManager* deviceManager() const { return
m_deviceManager.get(); }
did these get renamed from ::wtf at some point?

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

https://codereview.chromium.org/1850023002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webusb/USBDevice.cpp:156: case
device::usb::blink::OpenDeviceError::OK:
The verbosity of the prefix makes all this code hard to read, can we do using
statements or something?

https://codereview.chromium.org/1850023002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webusb/USBDevice.cpp:460: for (size_t i = 0; i
< mojoPackets.size(); ++i) {
range loop?

[Reilly Grant (use Gerrit), 2016-04-06 22:30:08]

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

https://codereview.chromium.org/1850023002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webusb/USB.cpp:114: // a
mojo::Callback::Runnable's const Run() method.
On 2016/04/06 at 20:10:45, esprehn wrote:
> this is really weird, callback interfaces generally don't put const so they
don't impose a restriction like this. Can we fix mojo?

mojo::Callback<>::Runnable::Run() is const because it needs to be called on
const mojo::Callback&s. This could be fixed by making
mojo::Callback<>::RunnableHolder::runnable volatile but rockot@ is against it.

https://codereview.chromium.org/1850023002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webusb/USB.cpp:134: for (size_t i = 0; i <
deviceInfos.size(); ++i) {
On 2016/04/06 at 20:10:45, esprehn wrote:
> range loop?

Done.

https://codereview.chromium.org/1850023002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webusb/USB.cpp:186:
m_deviceManager->GetDeviceChanges(adapter->callback());
On 2016/04/06 at 20:10:45, esprehn wrote:
> this seems to just do the exact same thing twice in a row? Why do that?

Sorry, there was a comment why in the original code.

https://codereview.chromium.org/1850023002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webusb/USB.cpp:217: LocalFrame* frame =
getExecutionContext() ? static_cast<Document*>(getExecutionContext())->frame() :
nullptr;
On 2016/04/06 at 20:10:45, esprehn wrote:
> toDocument() don't static_cast types in blink, our toFoo() macros enforce type
checks

Done.

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

https://codereview.chromium.org/1850023002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webusb/USBDevice.cpp:156: case
device::usb::blink::OpenDeviceError::OK:
On 2016/04/06 at 20:10:45, esprehn wrote:
> The verbosity of the prefix makes all this code hard to read, can we do using
statements or something?

Added "namespace usb = device::usb::blink;" to shorten the type names while
keeping them distinct from the IDL types.

https://codereview.chromium.org/1850023002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webusb/USBDevice.cpp:460: for (size_t i = 0; i
< mojoPackets.size(); ++i) {
On 2016/04/06 at 20:10:45, esprehn wrote:
> range loop?

Done.

[esprehn, 2016-04-06 22:42:38]

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

[esprehn, 2016-04-06 22:42:38]

rockot@ See comments about mojo

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

https://codereview.chromium.org/1850023002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webusb/USB.cpp:114: // a
mojo::Callback::Runnable's const Run() method.
On 2016/04/06 at 22:30:07, Reilly Grant wrote:
> On 2016/04/06 at 20:10:45, esprehn wrote:
> > this is really weird, callback interfaces generally don't put const so they
don't impose a restriction like this. Can we fix mojo?
> 
> mojo::Callback<>::Runnable::Run() is const because it needs to be called on
const mojo::Callback&s. This could be fixed by making
mojo::Callback<>::RunnableHolder::runnable volatile but rockot@ is against it.

Having to make your members mutable all over the codebase every time you use a
mojo::Callback is pretty broken. Mojo should not force const-ness on consumers
where they then need to magic it away with C++.

[Ken Rockot(use gerrit already), 2016-04-06 23:04:06]

On 2016/04/06 at 22:42:38, esprehn wrote:
> rockot@ See comments about mojo
> 
>
https://codereview.chromium.org/1850023002/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/modules/webusb/USB.cpp (right):
> 
>
https://codereview.chromium.org/1850023002/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/modules/webusb/USB.cpp:114: // a
mojo::Callback::Runnable's const Run() method.
> On 2016/04/06 at 22:30:07, Reilly Grant wrote:
> > On 2016/04/06 at 20:10:45, esprehn wrote:
> > > this is really weird, callback interfaces generally don't put const so
they don't impose a restriction like this. Can we fix mojo?
> > 
> > mojo::Callback<>::Runnable::Run() is const because it needs to be called on
const mojo::Callback&s. This could be fixed by making
mojo::Callback<>::RunnableHolder::runnable volatile but rockot@ is against it.
> 
> Having to make your members mutable all over the codebase every time you use a
mojo::Callback is pretty broken. Mojo should not force const-ness on consumers
where they then need to magic it away with C++.

This is not at all a common usage of mojo::Callback and it is definitely not
something that we would expect to see all over the codebase. In general nobody
should need to override Runnable, and this is the first such case where it is
allegedly necessary to do so. In any case, mojo::Callback should be seen as a
temporary inconvenience, and we should be able to use base::Callback instead
(and ideally, the safe Bind in blink should support binding multiple arguments
so this isn't necessary...)

Talked to Reilly offline though and it sounds like we can just make
Runnable::Run non-const without any issues (i.e. Callback::Run can still be
const).

[Reilly Grant (use Gerrit), 2016-04-06 23:38:07]

On 2016/04/06 at 23:04:06, rockot wrote:
> On 2016/04/06 at 22:42:38, esprehn wrote:
> > rockot@ See comments about mojo
> > 
> >
https://codereview.chromium.org/1850023002/diff/20001/third_party/WebKit/Sour...
> > File third_party/WebKit/Source/modules/webusb/USB.cpp (right):
> > 
> >
https://codereview.chromium.org/1850023002/diff/20001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/modules/webusb/USB.cpp:114: // a
mojo::Callback::Runnable's const Run() method.
> > On 2016/04/06 at 22:30:07, Reilly Grant wrote:
> > > On 2016/04/06 at 20:10:45, esprehn wrote:
> > > > this is really weird, callback interfaces generally don't put const so
they don't impose a restriction like this. Can we fix mojo?
> > > 
> > > mojo::Callback<>::Runnable::Run() is const because it needs to be called
on const mojo::Callback&s. This could be fixed by making
mojo::Callback<>::RunnableHolder::runnable volatile but rockot@ is against it.
> > 
> > Having to make your members mutable all over the codebase every time you use
a mojo::Callback is pretty broken. Mojo should not force const-ness on consumers
where they then need to magic it away with C++.
> 
> This is not at all a common usage of mojo::Callback and it is definitely not
something that we would expect to see all over the codebase. In general nobody
should need to override Runnable, and this is the first such case where it is
allegedly necessary to do so. In any case, mojo::Callback should be seen as a
temporary inconvenience, and we should be able to use base::Callback instead
(and ideally, the safe Bind in blink should support binding multiple arguments
so this isn't necessary...)
> 
> Talked to Reilly offline though and it sounds like we can just make
Runnable::Run non-const without any issues (i.e. Callback::Run can still be
const).

Done.

[Reilly Grant (use Gerrit), 2016-04-12 17:04:04]

Please take another look.

[Reilly Grant (use Gerrit), 2016-04-14 21:09:29]

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

[Reilly Grant (use Gerrit), 2016-04-14 21:09:30]

esprehn@ and rockot@, please review the pattern I've standardized in
MojoPromiseAdapter.h.

juncai@, please review the changes to the WebUSB implementation itself.

[Ken Rockot(use gerrit already), 2016-04-15 05:09:09]

On 2016/04/14 at 21:09:30, reillyg wrote:
> esprehn@ and rockot@, please review the pattern I've standardized in
MojoPromiseAdapter.h.
> 
> juncai@, please review the changes to the WebUSB implementation itself.

We want to delete mojo::Callback, so I'm really hesitant to allow introduction
of more dependencies on it.

Two things to consider:

1. IMHO we should have a general-purpose wrapper for ScriptPromiseResolver which
has explicit ownership semantics and which invokes user-provided default
fulfillment behavior in the case of wrapper-destruction-before-fulfillment
(similar to content::ScopedWebCallbacks but maybe less ugly ;)). This should not
be a Mojo-only solution.

2. If this is really going to be a common enough pattern (I think it is) and we
really can't make sameThreadBindForMojo more generally powerful, then given #1
we could add another limited bind utility like this:

    template <typename Class, typename ReturnType, typename... Args>
    mojo::Callback<ReturnType(Args...)>
    sameThreadBindForMojoWithResolver(ReturnType (Class::*method)(Args...),
                                      Class* instance,
                                      ScriptPromiseResolverWrapper resolver) {
        return base::Bind(method, base::Unretained(instance),
base::Passed(&resolver));
    }

Then implementations can write stuff like:

    class FooClient {
        // ...

        void DoStuff(ScriptPromiseResolver* resolver) {
            ScriptPromiseResolverWrapper wrappedResolver(
                resolver, WTF::bind(&FooClient::OnServiceLost, this));
             service_->DoStuff(
                 sameThreadBindForMojoWithResolver(&FooClient::OnStuffDone,
                                                   this,
                                                   std::move(wrappedResolver));
        }

        void OnServiceLost(ScriptPromiseResolver* resolver) {
            // Called if DoStuff's callback is dropped on the floor.
        }
    };

I'm pretty sure this will result in much less code all around, and it will be
much easier to transition from mojo::Callback to base::Callback. WDYT?

[esprehn, 2016-04-15 05:49:17]

I'm fine with whatever is going to result in the least verbose and
complicated code. Note that base::Bind does not handle garbage collected
objects correctly while wtf's bind does. You're likely to shoot your feet
off using base::Bind if you're not careful so we should be very cautious
about hiding it's usage from you inside templates or wrappers.

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[esprehn, 2016-04-15 05:54:30]

I'm fine with whatever is going to result in the least verbose and
complicated code. Note that base::Bind does not handle garbage collected
objects correctly while wtf's bind does. You're likely to shoot your feet
off using base::Bind if you're not careful so we should be very cautious
about hiding it's usage from you inside templates or wrappers.

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[juncai, 2016-04-15 16:52:47]

https://codereview.chromium.org/1850023002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/webusb/USB.cpp (right):

https://codereview.chromium.org/1850023002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webusb/USB.cpp:33: if
(mojoFilter->has_vendor_id)
maybe can use:
if (mojoFilter->has_vendor_id = filter.hasVendorId())
to save some extra lines.

ditto for the next a few lines.

https://codereview.chromium.org/1850023002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/webusb/USBDevice.cpp (right):

https://codereview.chromium.org/1850023002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webusb/USBDevice.cpp:41: return
DOMException::create(SecurityError, "The transfer was not allowed.");
This error message may be modified to better reflect that it is caused by
permission denied.

https://codereview.chromium.org/1850023002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webusb/USBDevice.cpp:110:
m_device->onDeviceOpenedOrClosed(true);
nit: add comment for true parameter.

https://codereview.chromium.org/1850023002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webusb/USBDevice.cpp:112: break;
can just use return; here.

https://codereview.chromium.org/1850023002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webusb/USBDevice.cpp:114:
m_device->onDeviceOpenedOrClosed(false);
nit: add comment for false parameter.

https://codereview.chromium.org/1850023002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webusb/USBDevice.cpp:116: break;
can just use return; here.

[Reilly Grant (use Gerrit), 2016-04-18 21:29:31]

Patchset #6 (id:120001) has been deleted

[Reilly Grant (use Gerrit), 2016-04-18 21:31:33]

Please take another look. It looks like I can use WTF::bind and have added a
createBaseCallback() function in MojoHelpers.h which will convert a
WTF::Function to a base::Callback. I would use WebFunction for this but because
it is a full class it is more difficult to use.

[Ken Rockot(use gerrit already), 2016-04-18 21:44:06]

LGTM - Perhaps it's worthwhile for our mojom-Blink bindings generator to (in a
future patch) emit utility methods for binding method callbacks. So instead of
calling 

  createBaseCallback(bind<usb::DeviceChangeNotificationPtr>(...))

you could just call

  bindGetDeviceChangesCallback(...)

[juncai, 2016-04-18 22:05:38]

LGTM with nits.

https://codereview.chromium.org/1850023002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/webusb/USBDevice.cpp (right):

https://codereview.chromium.org/1850023002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webusb/USBDevice.cpp:572:
onDeviceOpenedOrClosed(true);
nit: add comment for true parameter.

https://codereview.chromium.org/1850023002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webusb/USBDevice.cpp:574: break;
nit: maybe can just use return here?

https://codereview.chromium.org/1850023002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webusb/USBDevice.cpp:576:
onDeviceOpenedOrClosed(false);
ditto.

https://codereview.chromium.org/1850023002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webusb/USBDevice.cpp:578: break;
ditto.

https://codereview.chromium.org/1850023002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webusb/USBDevice.cpp:588:
onDeviceOpenedOrClosed(false);
ditto.

[Reilly Grant (use Gerrit), 2016-04-20 00:03:13]

pfeldman@ please take a look.

https://codereview.chromium.org/1850023002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/webusb/USBDevice.cpp (right):

https://codereview.chromium.org/1850023002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webusb/USBDevice.cpp:572:
onDeviceOpenedOrClosed(true);
On 2016/04/18 at 22:05:37, juncai wrote:
> nit: add comment for true parameter.

Done.

https://codereview.chromium.org/1850023002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webusb/USBDevice.cpp:574: break;
On 2016/04/18 at 22:05:37, juncai wrote:
> nit: maybe can just use return here?

Done.

https://codereview.chromium.org/1850023002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webusb/USBDevice.cpp:576:
onDeviceOpenedOrClosed(false);
On 2016/04/18 at 22:05:37, juncai wrote:
> ditto.

Done.

https://codereview.chromium.org/1850023002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webusb/USBDevice.cpp:578: break;
On 2016/04/18 at 22:05:37, juncai wrote:
> ditto.

Done.

https://codereview.chromium.org/1850023002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webusb/USBDevice.cpp:588:
onDeviceOpenedOrClosed(false);
On 2016/04/18 at 22:05:37, juncai wrote:
> ditto.

Done.

[pfeldman, 2016-04-20 18:22:38]

code removal lgtm

[Reilly Grant (use Gerrit), 2016-04-20 19:31:57]

The CQ bit was checked by reillyg@chromium.org

[Reilly Grant (use Gerrit), 2016-04-20 19:31:57]

The patchset sent to the CQ was uploaded after l-g-t-m from rockot@chromium.org,
juncai@chromium.org, pfeldman@chromium.org
Link to the patchset: https://codereview.chromium.org/1850023002/#ps180001
(title: "Don't do assignments in if statements.")

[commit-bot: I haz the power, 2016-04-20 19:32:15]

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

[commit-bot: I haz the power, 2016-04-20 20:45:39]

Committed patchset #8 (id:180001)

[commit-bot: I haz the power, 2016-04-22 19:26:16]

Description was changed from

==========
Consume Mojo services directly in Blink's WebUSB implementation.

This patch takes advantage of the ability to consume a Mojo service
directly in //third_party/WebKit/Source/modules to remove the
//third_party/WebKit/public/platform/modules/webusb,
//content/renderer/usb and the rest of that intermediate layer.

BUG=None
==========

to

==========
Consume Mojo services directly in Blink's WebUSB implementation.

This patch takes advantage of the ability to consume a Mojo service
directly in //third_party/WebKit/Source/modules to remove the
//third_party/WebKit/public/platform/modules/webusb,
//content/renderer/usb and the rest of that intermediate layer.

BUG=None

Committed: https://crrev.com/1b995c58c463593dc156931956a3e4ace76b5e52
Cr-Commit-Position: refs/heads/master@{#388571}
==========

[commit-bot: I haz the power, 2016-04-22 19:26:17]

Patchset 8 (id:??) landed as
https://crrev.com/1b995c58c463593dc156931956a3e4ace76b5e52
Cr-Commit-Position: refs/heads/master@{#388571}