Vibration API : migrate to device/vibration using mojo.

Vibration API : migrate to device/vibration using mojo.

Migrate Vibration API to use mojo. This patch does the following:
1. move Vibration API to device/vibration,
2. implement a vibration mojo service which is used from platform.
3. delete old filters and ipc messages.

BUG=437537
Committed: https://crrev.com/8e29c14638adbc5121a1514d041d5287e1116ca2
Cr-Commit-Position: refs/heads/master@{#307176}

[timvolodine, 2014-09-26 16:41:26]

timvolodine@chromium.org changed reviewers:
+ darin@chromium.org, mvanouwerkerk@chromium.org, qsr@chromium.org

[timvolodine, 2014-09-26 16:41:28]

Hi all,
please take a look

mvanouwerkerk@ : vibration API code
qsr@ : java JNI bindings
darin@ : mojo usage
cc:blundell@ : mojo migration similarities.

thanks,
Tim

[qsr, 2014-09-29 08:39:14]

For the JNI code, depending on how ppi@ CLs go, you might be able to directly
register a java implementation of the service instead of even needing JNI.

https://codereview.chromium.org/583663003/diff/320001/content/browser/rendere...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/583663003/diff/320001/content/browser/rendere...
content/browser/renderer_host/render_process_host_impl.cc:2107:
mojo_application_host_->Activate(this, GetHandle());
There is issue at this level. morrita@ has been working on trying to always
enable mojo, and his CL has been reverted because it was crashing. You will need
to sync with him, and only land this CL when the issue is resolved.

https://codereview.chromium.org/583663003/diff/320001/device/vibration/vibrat...
File device/vibration/vibration_manager_impl.cc (right):

https://codereview.chromium.org/583663003/diff/320001/device/vibration/vibrat...
device/vibration/vibration_manager_impl.cc:1: // Copyright 2014 The Chromium
Authors. All rights reserved.
Hum... Do we need this class at all. It seems that its main purpose is to allow
to override the provider. Shouldn't we have a point of extension at registration
time instead? Anyone that would want to override the service would just register
another implementation. The way it works today, the last registration win. So if
the point of extension is the last thing we do, we give the full control on the
set of available service and their implementation, and we do not need this class
at all.

[blundell, 2014-09-29 09:13:48]

blundell@chromium.org changed reviewers:
+ blundell@chromium.org

[blundell, 2014-09-29 09:13:49]

https://codereview.chromium.org/583663003/diff/320001/device/vibration/vibrat...
File device/vibration/vibration_manager_impl.cc (right):

https://codereview.chromium.org/583663003/diff/320001/device/vibration/vibrat...
device/vibration/vibration_manager_impl.cc:1: // Copyright 2014 The Chromium
Authors. All rights reserved.
On 2014/09/29 08:39:14, qsr wrote:
> Hum... Do we need this class at all. It seems that its main purpose is to
allow
> to override the provider. Shouldn't we have a point of extension at
registration
> time instead? Anyone that would want to override the service would just
register
> another implementation. The way it works today, the last registration win. So
if
> the point of extension is the last thing we do, we give the full control on
the
> set of available service and their implementation, and we do not need this
class
> at all.

+1. We should be providing an interface for the content client to register any
services it wants to. This will happen after //content registers its own
services. The downstream project can then just provide their own service impl
and register it.

[Michael van Ouwerkerk, 2014-09-29 12:55:20]

https://codereview.chromium.org/583663003/diff/320001/content/browser/rendere...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/583663003/diff/320001/content/browser/rendere...
content/browser/renderer_host/render_process_host_impl.cc:2106:
//MaybeActivateMojo();
Maybe delete this line?

https://codereview.chromium.org/583663003/diff/320001/content/renderer/render...
File content/renderer/renderer_webkitplatformsupport_impl.cc (right):

https://codereview.chromium.org/583663003/diff/320001/content/renderer/render...
content/renderer/renderer_webkitplatformsupport_impl.cc:1044:
vibration_manager_->Vibrate(milliseconds);
Why did you omit the cast?

https://codereview.chromium.org/583663003/diff/320001/content/renderer/render...
content/renderer/renderer_webkitplatformsupport_impl.cc:1048: if
(vibration_manager_)
Why isn't one instantiated when needed? A vibration could have been started from
another renderer, and this cancellation would stop it. An edge case, but I don't
see why it should not work.

https://codereview.chromium.org/583663003/diff/320001/device/vibration/vibrat...
File device/vibration/vibration_manager_impl.cc (right):

https://codereview.chromium.org/583663003/diff/320001/device/vibration/vibrat...
device/vibration/vibration_manager_impl.cc:1: // Copyright 2014 The Chromium
Authors. All rights reserved.
On 2014/09/29 09:13:49, blundell wrote:
> On 2014/09/29 08:39:14, qsr wrote:
> > Hum... Do we need this class at all. It seems that its main purpose is to
> allow
> > to override the provider. Shouldn't we have a point of extension at
> registration
> > time instead? Anyone that would want to override the service would just
> register
> > another implementation. The way it works today, the last registration win.
So
> if
> > the point of extension is the last thing we do, we give the full control on
> the
> > set of available service and their implementation, and we do not need this
> class
> > at all.
> 
> +1. We should be providing an interface for the content client to register any
> services it wants to. This will happen after //content registers its own
> services. The downstream project can then just provide their own service impl
> and register it.

Why override after content registers its own services? Sound like potentially
redundant work. We should override before.

[qsr, 2014-09-29 12:58:42]

On 2014/09/29 12:55:20, Michael van Ouwerkerk wrote:
>
https://codereview.chromium.org/583663003/diff/320001/content/browser/rendere...
> File content/browser/renderer_host/render_process_host_impl.cc (right):
> 
>
https://codereview.chromium.org/583663003/diff/320001/content/browser/rendere...
> content/browser/renderer_host/render_process_host_impl.cc:2106:
> //MaybeActivateMojo();
> Maybe delete this line?
> 
>
https://codereview.chromium.org/583663003/diff/320001/content/renderer/render...
> File content/renderer/renderer_webkitplatformsupport_impl.cc (right):
> 
>
https://codereview.chromium.org/583663003/diff/320001/content/renderer/render...
> content/renderer/renderer_webkitplatformsupport_impl.cc:1044:
> vibration_manager_->Vibrate(milliseconds);
> Why did you omit the cast?
> 
>
https://codereview.chromium.org/583663003/diff/320001/content/renderer/render...
> content/renderer/renderer_webkitplatformsupport_impl.cc:1048: if
> (vibration_manager_)
> Why isn't one instantiated when needed? A vibration could have been started
from
> another renderer, and this cancellation would stop it. An edge case, but I
don't
> see why it should not work.
> 
>
https://codereview.chromium.org/583663003/diff/320001/device/vibration/vibrat...
> File device/vibration/vibration_manager_impl.cc (right):
> 
>
https://codereview.chromium.org/583663003/diff/320001/device/vibration/vibrat...
> device/vibration/vibration_manager_impl.cc:1: // Copyright 2014 The Chromium
> Authors. All rights reserved.
> On 2014/09/29 09:13:49, blundell wrote:
> > On 2014/09/29 08:39:14, qsr wrote:
> > > Hum... Do we need this class at all. It seems that its main purpose is to
> > allow
> > > to override the provider. Shouldn't we have a point of extension at
> > registration
> > > time instead? Anyone that would want to override the service would just
> > register
> > > another implementation. The way it works today, the last registration win.
> So
> > if
> > > the point of extension is the last thing we do, we give the full control
on
> > the
> > > set of available service and their implementation, and we do not need this
> > class
> > > at all.
> > 
> > +1. We should be providing an interface for the content client to register
any
> > services it wants to. This will happen after //content registers its own
> > services. The downstream project can then just provide their own service
impl
> > and register it.
> 
> Why override after content registers its own services? Sound like potentially
> redundant work. We should override before.

 The registration is just adding a callback to a map. So, overriding is very
cheap.

[blundell, 2014-09-29 13:01:19]

On 2014/09/29 12:58:42, qsr wrote:
> On 2014/09/29 12:55:20, Michael van Ouwerkerk wrote:
> >
>
https://codereview.chromium.org/583663003/diff/320001/content/browser/rendere...
> > File content/browser/renderer_host/render_process_host_impl.cc (right):
> > 
> >
>
https://codereview.chromium.org/583663003/diff/320001/content/browser/rendere...
> > content/browser/renderer_host/render_process_host_impl.cc:2106:
> > //MaybeActivateMojo();
> > Maybe delete this line?
> > 
> >
>
https://codereview.chromium.org/583663003/diff/320001/content/renderer/render...
> > File content/renderer/renderer_webkitplatformsupport_impl.cc (right):
> > 
> >
>
https://codereview.chromium.org/583663003/diff/320001/content/renderer/render...
> > content/renderer/renderer_webkitplatformsupport_impl.cc:1044:
> > vibration_manager_->Vibrate(milliseconds);
> > Why did you omit the cast?
> > 
> >
>
https://codereview.chromium.org/583663003/diff/320001/content/renderer/render...
> > content/renderer/renderer_webkitplatformsupport_impl.cc:1048: if
> > (vibration_manager_)
> > Why isn't one instantiated when needed? A vibration could have been started
> from
> > another renderer, and this cancellation would stop it. An edge case, but I
> don't
> > see why it should not work.
> > 
> >
>
https://codereview.chromium.org/583663003/diff/320001/device/vibration/vibrat...
> > File device/vibration/vibration_manager_impl.cc (right):
> > 
> >
>
https://codereview.chromium.org/583663003/diff/320001/device/vibration/vibrat...
> > device/vibration/vibration_manager_impl.cc:1: // Copyright 2014 The Chromium
> > Authors. All rights reserved.
> > On 2014/09/29 09:13:49, blundell wrote:
> > > On 2014/09/29 08:39:14, qsr wrote:
> > > > Hum... Do we need this class at all. It seems that its main purpose is
to
> > > allow
> > > > to override the provider. Shouldn't we have a point of extension at
> > > registration
> > > > time instead? Anyone that would want to override the service would just
> > > register
> > > > another implementation. The way it works today, the last registration
win.
> > So
> > > if
> > > > the point of extension is the last thing we do, we give the full control
> on
> > > the
> > > > set of available service and their implementation, and we do not need
this
> > > class
> > > > at all.
> > > 
> > > +1. We should be providing an interface for the content client to register
> any
> > > services it wants to. This will happen after //content registers its own
> > > services. The downstream project can then just provide their own service
> impl
> > > and register it.
> > 
> > Why override after content registers its own services? Sound like
potentially
> > redundant work. We should override before.
> 
>  The registration is just adding a callback to a map. So, overriding is very
> cheap.

Additionally, if we had the overrides first, then the content code would need to
look like

if (!ServiceRegistered("foo"))
  // Register "foo"

for every service, which is (1) ugly and (2) introduces the possibility for
breakage if someone forgets to follow the pattern.

[Michael van Ouwerkerk, 2014-09-29 13:02:58]

On 2014/09/29 13:01:19, blundell wrote:
> On 2014/09/29 12:58:42, qsr wrote:
> > On 2014/09/29 12:55:20, Michael van Ouwerkerk wrote:
> > >
> >
>
https://codereview.chromium.org/583663003/diff/320001/content/browser/rendere...
> > > File content/browser/renderer_host/render_process_host_impl.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/583663003/diff/320001/content/browser/rendere...
> > > content/browser/renderer_host/render_process_host_impl.cc:2106:
> > > //MaybeActivateMojo();
> > > Maybe delete this line?
> > > 
> > >
> >
>
https://codereview.chromium.org/583663003/diff/320001/content/renderer/render...
> > > File content/renderer/renderer_webkitplatformsupport_impl.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/583663003/diff/320001/content/renderer/render...
> > > content/renderer/renderer_webkitplatformsupport_impl.cc:1044:
> > > vibration_manager_->Vibrate(milliseconds);
> > > Why did you omit the cast?
> > > 
> > >
> >
>
https://codereview.chromium.org/583663003/diff/320001/content/renderer/render...
> > > content/renderer/renderer_webkitplatformsupport_impl.cc:1048: if
> > > (vibration_manager_)
> > > Why isn't one instantiated when needed? A vibration could have been
started
> > from
> > > another renderer, and this cancellation would stop it. An edge case, but I
> > don't
> > > see why it should not work.
> > > 
> > >
> >
>
https://codereview.chromium.org/583663003/diff/320001/device/vibration/vibrat...
> > > File device/vibration/vibration_manager_impl.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/583663003/diff/320001/device/vibration/vibrat...
> > > device/vibration/vibration_manager_impl.cc:1: // Copyright 2014 The
Chromium
> > > Authors. All rights reserved.
> > > On 2014/09/29 09:13:49, blundell wrote:
> > > > On 2014/09/29 08:39:14, qsr wrote:
> > > > > Hum... Do we need this class at all. It seems that its main purpose is
> to
> > > > allow
> > > > > to override the provider. Shouldn't we have a point of extension at
> > > > registration
> > > > > time instead? Anyone that would want to override the service would
just
> > > > register
> > > > > another implementation. The way it works today, the last registration
> win.
> > > So
> > > > if
> > > > > the point of extension is the last thing we do, we give the full
control
> > on
> > > > the
> > > > > set of available service and their implementation, and we do not need
> this
> > > > class
> > > > > at all.
> > > > 
> > > > +1. We should be providing an interface for the content client to
register
> > any
> > > > services it wants to. This will happen after //content registers its own
> > > > services. The downstream project can then just provide their own service
> > impl
> > > > and register it.
> > > 
> > > Why override after content registers its own services? Sound like
> potentially
> > > redundant work. We should override before.
> > 
> >  The registration is just adding a callback to a map. So, overriding is very
> > cheap.
> 
> Additionally, if we had the overrides first, then the content code would need
to
> look like
> 
> if (!ServiceRegistered("foo"))
>   // Register "foo"
> 
> for every service, which is (1) ugly and (2) introduces the possibility for
> breakage if someone forgets to follow the pattern.

Ah, ok then.

[timvolodine, 2014-10-03 15:57:14]

https://codereview.chromium.org/583663003/diff/320001/content/browser/rendere...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/583663003/diff/320001/content/browser/rendere...
content/browser/renderer_host/render_process_host_impl.cc:2106:
//MaybeActivateMojo();
On 2014/09/29 12:55:19, Michael van Ouwerkerk wrote:
> Maybe delete this line?

Done. :)

https://codereview.chromium.org/583663003/diff/320001/content/browser/rendere...
content/browser/renderer_host/render_process_host_impl.cc:2107:
mojo_application_host_->Activate(this, GetHandle());
On 2014/09/29 08:39:14, qsr wrote:
> There is issue at this level. morrita@ has been working on trying to always
> enable mojo, and his CL has been reverted because it was crashing. You will
need
> to sync with him, and only land this CL when the issue is resolved.

yes, morrita@ is working on a fix.

https://codereview.chromium.org/583663003/diff/320001/content/renderer/render...
File content/renderer/renderer_webkitplatformsupport_impl.cc (right):

https://codereview.chromium.org/583663003/diff/320001/content/renderer/render...
content/renderer/renderer_webkitplatformsupport_impl.cc:1044:
vibration_manager_->Vibrate(milliseconds);
On 2014/09/29 12:55:19, Michael van Ouwerkerk wrote:
> Why did you omit the cast?

I've put it back. Done.

https://codereview.chromium.org/583663003/diff/320001/content/renderer/render...
content/renderer/renderer_webkitplatformsupport_impl.cc:1048: if
(vibration_manager_)
On 2014/09/29 12:55:19, Michael van Ouwerkerk wrote:
> Why isn't one instantiated when needed? A vibration could have been started
from
> another renderer, and this cancellation would stop it. An edge case, but I
don't
> see why it should not work.

good point. Done.

https://codereview.chromium.org/583663003/diff/320001/device/vibration/vibrat...
File device/vibration/vibration_manager_impl.cc (right):

https://codereview.chromium.org/583663003/diff/320001/device/vibration/vibrat...
device/vibration/vibration_manager_impl.cc:1: // Copyright 2014 The Chromium
Authors. All rights reserved.
On 2014/09/29 09:13:49, blundell wrote:
> On 2014/09/29 08:39:14, qsr wrote:
> > Hum... Do we need this class at all. It seems that its main purpose is to
> allow
> > to override the provider. Shouldn't we have a point of extension at
> registration
> > time instead? Anyone that would want to override the service would just
> register
> > another implementation. The way it works today, the last registration win.
So
> if
> > the point of extension is the last thing we do, we give the full control on
> the
> > set of available service and their implementation, and we do not need this
> class
> > at all.
> 
> +1. We should be providing an interface for the content client to register any
> services it wants to. This will happen after //content registers its own
> services. The downstream project can then just provide their own service impl
> and register it.

I agree this looks like a good example since vibration api is currently
java-only and we can probably get rid of this class. I think this is best
handled in a separate follow-up patch though, since we don't have a proper mojo
override interface yet. This patch is intended for migration to mojo + moving to
device/vibration.

[mlamouri (slow - plz ping), 2014-10-05 20:33:39]

mlamouri@chromium.org changed reviewers:
+ mlamouri@chromium.org

[mlamouri (slow - plz ping), 2014-10-05 20:33:40]

https://codereview.chromium.org/583663003/diff/360001/content/public/common/c...
File content/public/common/content_client.h (right):

https://codereview.chromium.org/583663003/diff/360001/content/public/common/c...
content/public/common/content_client.h:55: CONTENT_EXPORT ContentClient*
GetContentClient();
Out of curiosity, why do you need that? It sounds odd that you mark a
CONTENT_IMPLEMENTATION only method as CONTENT_EXPORT.

[blundell, 2014-10-09 09:40:59]

blundell@chromium.org changed reviewers:
+ blundell@chromium.org

[blundell, 2014-10-09 09:41:00]

I filed crbug.com/421796 to track having the embedder of //content be able to
register its own Mojo services.

[timvolodine, 2014-10-16 15:58:20]

Mounir I've addressed your comment below.
Colin thanks for filing the bug!

https://codereview.chromium.org/583663003/diff/360001/content/public/common/c...
File content/public/common/content_client.h (right):

https://codereview.chromium.org/583663003/diff/360001/content/public/common/c...
content/public/common/content_client.h:55: CONTENT_EXPORT ContentClient*
GetContentClient();
On 2014/10/05 20:33:40, Mounir Lamouri wrote:
> Out of curiosity, why do you need that? It sounds odd that you mark a
> CONTENT_IMPLEMENTATION only method as CONTENT_EXPORT.

right, this is not needed anymore since the override is in device/vibration/
now.
Done.

[timvolodine, 2014-12-01 11:37:04]

this is ready for review now [and hopefully lgtms ;)], please take another look.
there do not seem to be any blocking issues anymore.

[ppi, 2014-12-01 12:03:54]

ppi@chromium.org changed reviewers:
+ ppi@chromium.org

[ppi, 2014-12-01 12:03:55]

https://codereview.chromium.org/583663003/diff/400001/content/public/browser/...
File content/public/browser/content_browser_client.h (left):

https://codereview.chromium.org/583663003/diff/400001/content/public/browser/...
content/public/browser/content_browser_client.h:569: // FYI: Used by an external
project; please don't remove.
What's up with that - have we reached out to Viatcheslav about the change?

I can land a patch for Mojo overrides in the client API
(http://crbug.com/421796), but we need a confirmation that it will be indeed
used by someone.

[timvolodine, 2014-12-01 12:11:42]

https://codereview.chromium.org/583663003/diff/400001/content/public/browser/...
File content/public/browser/content_browser_client.h (left):

https://codereview.chromium.org/583663003/diff/400001/content/public/browser/...
content/public/browser/content_browser_client.h:569: // FYI: Used by an external
project; please don't remove.
On 2014/12/01 12:03:55, ppi wrote:
> What's up with that - have we reached out to Viatcheslav about the change?
> 
> I can land a patch for Mojo overrides in the client API
> (http://crbug.com/421796), but we need a confirmation that it will be indeed
> used by someone.

yes I have communicated with Viatcheslav about this change he was ok with it
(more details in crbug.com/416426).
In the longer run we could use the approach in your patch, but I don't think
this should block this patch at this point.

[ppi, 2014-12-01 12:25:13]

https://codereview.chromium.org/583663003/diff/400001/content/public/browser/...
File content/public/browser/content_browser_client.h (left):

https://codereview.chromium.org/583663003/diff/400001/content/public/browser/...
content/public/browser/content_browser_client.h:569: // FYI: Used by an external
project; please don't remove.
On 2014/12/01 12:11:42, timvolodine wrote:
> On 2014/12/01 12:03:55, ppi wrote:
> > What's up with that - have we reached out to Viatcheslav about the change?
> > 
> > I can land a patch for Mojo overrides in the client API
> > (http://crbug.com/421796), but we need a confirmation that it will be indeed
> > used by someone.
> 
> yes I have communicated with Viatcheslav about this change he was ok with it
> (more details in crbug.com/416426).
> In the longer run we could use the approach in your patch, but I don't think
> this should block this patch at this point.

Sounds good. I will put together a standalone CL for the overrides and send it
for review to Viatcheslav.

[timvolodine, 2014-12-01 13:00:05]

timvolodine@chromium.org changed reviewers:
+ tsepez@chromium.org

[timvolodine, 2014-12-01 13:00:07]

+tsepez@ : for content/common/view_messages.h

[qsr, 2014-12-01 16:21:32]

https://codereview.chromium.org/583663003/diff/400001/content/app/android/lib...
File content/app/android/library_loader_hooks.cc (right):

https://codereview.chromium.org/583663003/diff/400001/content/app/android/lib...
content/app/android/library_loader_hooks.cc:25: 
spurious blank line?

[Tom Sepez, 2014-12-01 18:27:17]

Messages/Mojo LGTM.

https://codereview.chromium.org/583663003/diff/400001/device/vibration/androi...
File device/vibration/android/vibration_jni_registrar.cc (right):

https://codereview.chromium.org/583663003/diff/400001/device/vibration/androi...
device/vibration/android/vibration_jni_registrar.cc:15:
base::android::RegistrationMethod kRegisteredMethods[] = {
nit: is this const?

https://codereview.chromium.org/583663003/diff/400001/device/vibration/androi...
device/vibration/android/vibration_jni_registrar.cc:20: bool
RegisterVibrationJni(JNIEnv* env) {
nit: maybe blank line before this function

https://codereview.chromium.org/583663003/diff/400001/device/vibration/vibrat...
File device/vibration/vibration_manager.mojom (right):

https://codereview.chromium.org/583663003/diff/400001/device/vibration/vibrat...
device/vibration/vibration_manager.mojom:8: Vibrate(int64 millis);
nit: maybe spell out "milliseconds".

https://codereview.chromium.org/583663003/diff/400001/device/vibration/vibrat...
File device/vibration/vibration_manager_impl.h (right):

https://codereview.chromium.org/583663003/diff/400001/device/vibration/vibrat...
device/vibration/vibration_manager_impl.h:22: virtual void Vibrate(int64 millis)
override;
nit: ditto

https://codereview.chromium.org/583663003/diff/400001/device/vibration/vibrat...
device/vibration/vibration_manager_impl.h:27: // Contact Viatcheslav Ostapenko
at sl.ostapenko@samsung.com for more
nit: someone else commented on this, I'm assuming its handled.

https://codereview.chromium.org/583663003/diff/400001/device/vibration/vibrat...
File device/vibration/vibration_provider_android.cc (right):

https://codereview.chromium.org/583663003/diff/400001/device/vibration/vibrat...
device/vibration/vibration_provider_android.cc:6: 
nit: no blank line?

https://codereview.chromium.org/583663003/diff/400001/device/vibration/vibrat...
device/vibration/vibration_provider_android.cc:7: #include
"device/vibration/vibration_manager_impl.h"
nit: alphabetize.

https://codereview.chromium.org/583663003/diff/400001/device/vibration/vibrat...
File device/vibration/vibration_provider_android.h (right):

https://codereview.chromium.org/583663003/diff/400001/device/vibration/vibrat...
device/vibration/vibration_provider_android.h:22: virtual void Vibrate(int64
milliseconds) override;
nit: I thought the new style recommendation was to use override omitting
"virtual".

[blundell, 2014-12-02 10:54:07]

Have you given thought to having the Android service impl in Java and getting
rid of the custom JNI as a followup?

[timvolodine, 2014-12-02 19:37:57]

https://codereview.chromium.org/583663003/diff/400001/content/app/android/lib...
File content/app/android/library_loader_hooks.cc (right):

https://codereview.chromium.org/583663003/diff/400001/content/app/android/lib...
content/app/android/library_loader_hooks.cc:25: 
On 2014/12/01 16:21:32, qsr wrote:
> spurious blank line?

Done.

https://codereview.chromium.org/583663003/diff/400001/device/vibration/androi...
File device/vibration/android/vibration_jni_registrar.cc (right):

https://codereview.chromium.org/583663003/diff/400001/device/vibration/androi...
device/vibration/android/vibration_jni_registrar.cc:15:
base::android::RegistrationMethod kRegisteredMethods[] = {
On 2014/12/01 18:27:16, Tom Sepez wrote:
> nit: is this const?

Done.

https://codereview.chromium.org/583663003/diff/400001/device/vibration/androi...
device/vibration/android/vibration_jni_registrar.cc:20: bool
RegisterVibrationJni(JNIEnv* env) {
On 2014/12/01 18:27:16, Tom Sepez wrote:
> nit: maybe blank line before this function

Done.

https://codereview.chromium.org/583663003/diff/400001/device/vibration/vibrat...
File device/vibration/vibration_manager.mojom (right):

https://codereview.chromium.org/583663003/diff/400001/device/vibration/vibrat...
device/vibration/vibration_manager.mojom:8: Vibrate(int64 millis);
On 2014/12/01 18:27:16, Tom Sepez wrote:
> nit: maybe spell out "milliseconds". 

Done.

https://codereview.chromium.org/583663003/diff/400001/device/vibration/vibrat...
File device/vibration/vibration_manager_impl.h (right):

https://codereview.chromium.org/583663003/diff/400001/device/vibration/vibrat...
device/vibration/vibration_manager_impl.h:22: virtual void Vibrate(int64 millis)
override;
On 2014/12/01 18:27:16, Tom Sepez wrote:
> nit: ditto

Done.

https://codereview.chromium.org/583663003/diff/400001/device/vibration/vibrat...
device/vibration/vibration_manager_impl.h:27: // Contact Viatcheslav Ostapenko
at sl.ostapenko@samsung.com for more
On 2014/12/01 18:27:16, Tom Sepez wrote:
> nit: someone else commented on this, I'm assuming its handled.

Done.

https://codereview.chromium.org/583663003/diff/400001/device/vibration/vibrat...
File device/vibration/vibration_provider_android.cc (right):

https://codereview.chromium.org/583663003/diff/400001/device/vibration/vibrat...
device/vibration/vibration_provider_android.cc:6: 
On 2014/12/01 18:27:17, Tom Sepez wrote:
> nit: no blank line?

I believe this is correct: main header include, blank line followed by the other
includes in alphabetical order.

https://codereview.chromium.org/583663003/diff/400001/device/vibration/vibrat...
device/vibration/vibration_provider_android.cc:7: #include
"device/vibration/vibration_manager_impl.h"
On 2014/12/01 18:27:16, Tom Sepez wrote:
> nit: alphabetize.

see above. done.

https://codereview.chromium.org/583663003/diff/400001/device/vibration/vibrat...
File device/vibration/vibration_provider_android.h (right):

https://codereview.chromium.org/583663003/diff/400001/device/vibration/vibrat...
device/vibration/vibration_provider_android.h:22: virtual void Vibrate(int64
milliseconds) override;
On 2014/12/01 18:27:17, Tom Sepez wrote:
> nit: I thought the new style recommendation was to use override omitting
> "virtual".

Done.

[timvolodine, 2014-12-02 19:38:57]

On 2014/12/02 10:54:07, blundell wrote:
> Have you given thought to having the Android service impl in Java and getting
> rid of the custom JNI as a followup?

yes that's definitely on my todo list :)

[timvolodine, 2014-12-03 12:10:51]

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

[timvolodine, 2014-12-03 12:10:52]

ok, think I'll need a high level lgtm to land: darin@ or jam@ could you
rubber-stamp this?

[Michael van Ouwerkerk, 2014-12-03 14:10:12]

lgtm with minor nits

https://codereview.chromium.org/583663003/diff/420001/device/vibration/vibrat...
File device/vibration/vibration_provider_android.h (right):

https://codereview.chromium.org/583663003/diff/420001/device/vibration/vibrat...
device/vibration/vibration_provider_android.h:20: virtual
~VibrationProviderAndroid();
Use override, not virtual.

https://codereview.chromium.org/583663003/diff/420001/device/vibration/vibrat...
device/vibration/vibration_provider_android.h:22: virtual void Vibrate(int64
milliseconds) override;
Omit virtual when using override. Same below.

[Michael van Ouwerkerk, 2014-12-03 14:11:28]

On 2014/12/03 12:10:52, timvolodine wrote:
> ok, think I'll need a high level lgtm to land: darin@ or jam@ could you
> rubber-stamp this?

I don't think jam does reviews for us anymore. You can try avi as content owner
if you feel like adding darin is not enough :-)

[timvolodine, 2014-12-04 18:41:05]

https://codereview.chromium.org/583663003/diff/420001/device/vibration/vibrat...
File device/vibration/vibration_provider_android.h (right):

https://codereview.chromium.org/583663003/diff/420001/device/vibration/vibrat...
device/vibration/vibration_provider_android.h:20: virtual
~VibrationProviderAndroid();
On 2014/12/03 14:10:12, Michael van Ouwerkerk wrote:
> Use override, not virtual.

Done.

https://codereview.chromium.org/583663003/diff/420001/device/vibration/vibrat...
device/vibration/vibration_provider_android.h:22: virtual void Vibrate(int64
milliseconds) override;
On 2014/12/03 14:10:12, Michael van Ouwerkerk wrote:
> Omit virtual when using override. Same below.

Done.

[timvolodine, 2014-12-04 19:30:28]

On 2014/12/03 14:11:28, Michael van Ouwerkerk wrote:
> On 2014/12/03 12:10:52, timvolodine wrote:
> > ok, think I'll need a high level lgtm to land: darin@ or jam@ could you
> > rubber-stamp this?
> 
> I don't think jam does reviews for us anymore. You can try avi as content
owner
> if you feel like adding darin is not enough :-)

maybe, I've seen jam doing mojo related reviews in chromium though.

jam@ or darin@ would you be able to review this patch?

[jam, 2014-12-05 01:13:34]

nice to see the simplification by switching to mojo! just one comment.

ps yep i'm still doing reviews :)

https://codereview.chromium.org/583663003/diff/440001/device/vibration/vibrat...
File device/vibration/vibration_manager_impl.cc (right):

https://codereview.chromium.org/583663003/diff/440001/device/vibration/vibrat...
device/vibration/vibration_manager_impl.cc:36: // trust any values passed from
the renderer.
nit: update "renderer" to "client"?

https://codereview.chromium.org/583663003/diff/440001/device/vibration/vibrat...
File device/vibration/vibration_manager_impl.h (right):

https://codereview.chromium.org/583663003/diff/440001/device/vibration/vibrat...
device/vibration/vibration_manager_impl.h:28: // information.
is this still needed with mojo? can't embedders override this with the
functionality added in https://codereview.chromium.org/772433004/? If so, please
remove VibrationProvider

[timvolodine, 2014-12-05 18:12:48]

jam, thanks for the comments, PTAL.

https://codereview.chromium.org/583663003/diff/440001/device/vibration/vibrat...
File device/vibration/vibration_manager_impl.cc (right):

https://codereview.chromium.org/583663003/diff/440001/device/vibration/vibrat...
device/vibration/vibration_manager_impl.cc:36: // trust any values passed from
the renderer.
On 2014/12/05 01:13:34, jam wrote:
> nit: update "renderer" to "client"?

Done.

https://codereview.chromium.org/583663003/diff/440001/device/vibration/vibrat...
File device/vibration/vibration_manager_impl.h (right):

https://codereview.chromium.org/583663003/diff/440001/device/vibration/vibrat...
device/vibration/vibration_manager_impl.h:28: // information.
On 2014/12/05 01:13:34, jam wrote:
> is this still needed with mojo? can't embedders override this with the
> functionality added in https://codereview.chromium.org/772433004/ If so,
please
> remove VibrationProvider

right, removed VibrationProvider (which required some refactoring though).
Done.

[jam, 2014-12-05 18:44:28]

lgtm

[timvolodine, 2014-12-05 19:41:09]

Patchset #26 (id:500001) has been deleted

[mlamouri (slow - plz ping), 2014-12-06 12:51:20]

The CQ bit was checked by mlamouri@chromium.org

[commit-bot: I haz the power, 2014-12-06 12:51:57]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/583663003/520001

[commit-bot: I haz the power, 2014-12-06 13:49:10]

Committed patchset #26 (id:520001)

[commit-bot: I haz the power, 2014-12-06 13:50:21]

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

[Nico, 2014-12-23 23:32:59]

http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...
is pretty red, with compile errors like

FAILED: /b/build/goma/gomacc
../../third_party/android_tools/ndk/toolchains/arm-linux-androideabi-4.9/prebuilt/linux-x86_64/bin/arm-linux-androideabi-g++
-MMD -MF obj/device/vibration/android/android.vibration_jni_registrar.o.d
-DCHROMIUM_BUILD -DV8_DEPRECATION_WARNINGS -DCLD_VERSION=1
-DENABLE_NOTIFICATIONS -DENABLE_BROWSER_CDMS -DENABLE_PRINTING=1
-DENABLE_BASIC_PRINTING=1 -DUSE_OPENSSL=1 -DUSE_OPENSSL_CERTS=1 -DNO_TCMALLOC
-DDISABLE_NACL -DENABLE_CONFIGURATION_POLICY -DENABLE_SUPERVISED_USERS=1
-DENABLE_AUTOFILL_DIALOG=1 -DV8_USE_EXTERNAL_STARTUP_DATA -D_FILE_OFFSET_BITS=64
-DANDROID -DHAVE_SYS_UIO_H -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS
-DUSE_STLPORT=1 -D_STLP_USE_PTR_SPECIALIZATIONS=1 -D__GNU_SOURCE=1 -D_DEBUG
-DDYNAMIC_ANNOTATIONS_ENABLED=1 -DWTF_USE_DYNAMIC_ANNOTATIONS=1 -I../.. -Igen
-fno-strict-aliasing -march=armv7-a -mfloat-abi=softfp -mtune=generic-armv7-a
-mthumb -mthumb-interwork -fno-tree-sra -fno-caller-saves -funwind-tables -fPIC
-pipe -ffunction-sections -funwind-tables -fno-short-enums -finline-limit=64
-mfpu=vfpv3-d16 -Wall -Wsign-compare -Wendif-labels -Werror
-Wno-missing-field-initializers -Wno-unused-parameter -Wno-psabi -Wno-extra
-Wno-ignored-qualifiers -Wno-type-limits -Wno-unused-local-typedefs
-isystem../../third_party/android_tools/ndk/sources/cxx-stl/stlport/stlport
-fvisibility=hidden
--sysroot=/b/build/slave/android_chromium_gn/build/src/third_party/android_tools/ndk/platforms/android-14/arch-arm
-Os -fdata-sections -ffunction-sections -fomit-frame-pointer -g2
-fno-threadsafe-statics -fvisibility-inlines-hidden -std=gnu++11 -Wno-narrowing
-Wno-literal-suffix -Wno-error=c++0x-compat -Wno-non-virtual-dtor
-Wno-sign-promo -fno-rtti -fno-exceptions -c
../../device/vibration/android/vibration_jni_registrar.cc -o
obj/device/vibration/android/android.vibration_jni_registrar.o
In file included from
../../device/vibration/android/vibration_jni_registrar.cc:9:0:
../../device/vibration/vibration_manager_impl_android.h:9:54: fatal error:
device/vibration/vibration_manager.mojom.h: No such file or directory
 #include "device/vibration/vibration_manager.mojom.h"
                                                      ^
compilation terminated.


Note that this is a GN bot; the gyp bots seem fine. I'm guessing your gn
dependencies are underspecified?

[dcheng, 2014-12-23 23:53:23]

A revert of this CL (patchset #26 id:520001) has been created in
https://codereview.chromium.org/826613003/ by dcheng@chromium.org.

The reason for reverting is: Android GN debug clobber builds broken.

[dcheng, 2014-12-23 23:57:59]

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

[dcheng, 2014-12-23 23:58:00]

(The revert wasn't actually landed, it was just to see how feasible it was.)

[Nico, 2014-12-24 00:35:40]

https://codereview.chromium.org/826593003/ tries to fix the bot.