Make VibrationMessageFilter available to other platforms.

Make VibrationMessageFilter available to other platforms.

Adds VibrationProvider interface that simplifies adding vibration API handlers for other platforms and moves Android implementation into VibrationProviderAndroid.
Also adds ContentBrowserClient::OverrideVibrationProvider() method that allows embedder application override handling of blink Vibration API.

BUG=222504
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=237101

[Avi (use Gerrit), 2013-09-12 05:05:57]

Link to a bug with BUG line.

Please specify which reviewers you are requesting to review which files.

https://codereview.chromium.org/23496051/diff/1/content/browser/android/brows...
File content/browser/android/browser_jni_registrar.cc (right):

https://codereview.chromium.org/23496051/diff/1/content/browser/android/brows...
content/browser/android/browser_jni_registrar.cc:66: {"VibrationMessageFilter",
content::VibrationMessageFilterAndroid::Register},
80 cols

https://codereview.chromium.org/23496051/diff/1/content/browser/renderer_host...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/23496051/diff/1/content/browser/renderer_host...
content/browser/renderer_host/render_process_host_impl.cc:696:
VibrationMessageFilter* vibra_filter = VibrationMessageFilter::Create();
You can spell out "vibration".

https://codereview.chromium.org/23496051/diff/1/content/browser/vibration_mes...
File content/browser/vibration_message_filter.cc (right):

https://codereview.chromium.org/23496051/diff/1/content/browser/vibration_mes...
content/browser/vibration_message_filter.cc:15:
VibrationMessageFilter::VibrationMessageFilterFactory*
VibrationMessageFilter::factory_ = 0;
80 columns!

And NULL, not 0.

https://codereview.chromium.org/23496051/diff/1/content/browser/vibration_mes...
File content/browser/vibration_message_filter.h (right):

https://codereview.chromium.org/23496051/diff/1/content/browser/vibration_mes...
content/browser/vibration_message_filter.h:12: class CONTENT_EXPORT
VibrationMessageFilter
Comments on the class and functions?

https://codereview.chromium.org/23496051/diff/1/content/browser/vibration_mes...
content/browser/vibration_message_filter.h:20: static void
SetVibrationMessageFilterFactory(VibrationMessageFilterFactory* factory);
80 columns!

[ostap, 2013-09-12 16:14:30]

On 2013/09/12 05:05:57, Avi wrote:
> Please specify which reviewers you are requesting to review which files.

Added to reviewers Peter Beverloo, who've reviewed original commits for this
feature.

[Peter Beverloo, 2013-09-13 16:14:12]

+Michael who should also review this.

I left some general comments.  Could you please clarify which platform you plan
on implementing vibration support for, and whether that will be done in content/
as well?  You also seem to have included some hooks for testing, but I think
these would be better when done in a patch that actually uses them.

https://codereview.chromium.org/23496051/diff/9001/content/browser/android/vi...
File content/browser/android/vibration_message_filter_android.cc (right):

https://codereview.chromium.org/23496051/diff/9001/content/browser/android/vi...
content/browser/android/vibration_message_filter_android.cc:19: const int64
kMinimumVibrationDurationMs = 1;
We'd want a setting like this to apply everywhere.  Can we perhaps make it a
constant in the VibrationMessageFilter class, so all the platform-specific
implementations can use it?

https://codereview.chromium.org/23496051/diff/9001/content/browser/android/vi...
content/browser/android/vibration_message_filter_android.cc:57: if (factory_)
Why do we need a factory function here? Testing I suppose?

I'm not sure what the best convention would be. Personally I think it would be
nicer to only define VibrationMessageFilter::Create() in
vibration_message_filter.cc and use if-defs to return the right implementation
rather than re-defining the same method in different places.

Especially if we need the ability to inject a testing message filter it'd be
better to have that logic in one place rather than in several places.

https://codereview.chromium.org/23496051/diff/9001/content/browser/android/vi...
File content/browser/android/vibration_message_filter_android.h (right):

https://codereview.chromium.org/23496051/diff/9001/content/browser/android/vi...
content/browser/android/vibration_message_filter_android.h:13: class
VibrationMessageFilterAndroid : public VibrationMessageFilter {
Should this be called VibrationServiceAndroid, which implements the
VibrationMessageFilter?  The class which you have now doesn't actually filter
messages.

https://codereview.chromium.org/23496051/diff/9001/content/browser/vibration_...
File content/browser/vibration_message_filter.h (right):

https://codereview.chromium.org/23496051/diff/9001/content/browser/vibration_...
content/browser/vibration_message_filter.h:15: VibrationMessageFilter();
If the Create() method is static, then the constructor shouldn't be public. 
Since we're not doing anything in it anyway, it's fine to remove it.

https://codereview.chromium.org/23496051/diff/9001/content/browser/vibration_...
content/browser/vibration_message_filter.h:17: // Create instance of
VibrationMessageFilter.
"Create a platform-specific instance of the VibrationMessageFilter."

https://codereview.chromium.org/23496051/diff/9001/content/browser/vibration_...
content/browser/vibration_message_filter.h:22: typedef VibrationMessageFilter*
Factory();
I'm not too fussed about this.  Like I asked earlier, is it for testing?  If so,
can we add it later when you actually implement tests?

https://codereview.chromium.org/23496051/diff/9001/content/browser/vibration_...
content/browser/vibration_message_filter.h:26: // Device should start vibrating
for N milliseconds.
nit: I'd prefer it if this could be written as "vibrating for |milliseconds|
ms".

https://codereview.chromium.org/23496051/diff/9001/content/browser/vibration_...
content/browser/vibration_message_filter.h:28: // Cancels vibration of the
device.
nit: empty line above here, perhaps?

https://codereview.chromium.org/23496051/diff/9001/content/renderer/renderer_...
File content/renderer/renderer_webkitplatformsupport_impl.h (right):

https://codereview.chromium.org/23496051/diff/9001/content/renderer/renderer_...
content/renderer/renderer_webkitplatformsupport_impl.h:152: virtual void
cancelVibration();
These should be annotated with OVERRIDE because they already have an
implementation in third_party/WebKit/public/platform/Platform.h.

[ostap, 2013-09-18 15:44:01]

Redesign it similar to LocationProvider.

[ostap, 2013-10-10 21:12:51]

Rebased patch.

[Michael van Ouwerkerk, 2013-10-16 13:21:41]

Please make sure this patch compiles. I ran some try bots, they look unhappy.
Have you addressed the previous comments by the other reviewers? Maybe mark them
as done, or if they are obsolete add a note why.

https://codereview.chromium.org/23496051/diff/21001/content/browser/android/b...
File content/browser/android/browser_jni_registrar.cc (right):

https://codereview.chromium.org/23496051/diff/21001/content/browser/android/b...
content/browser/android/browser_jni_registrar.cc:25: #include
"content/browser/android/vibration_message_filter.h"
Looks like you can delete this now.

https://codereview.chromium.org/23496051/diff/21001/content/browser/vibration...
File content/browser/vibration/vibration_dispatcher_host.h (right):

https://codereview.chromium.org/23496051/diff/21001/content/browser/vibration...
content/browser/vibration/vibration_dispatcher_host.h:13: class
VibrationDispatcherHost
I prefer VibrationMessageFilter in this case (like it was). I think you may have
derived this from the geolocation code. There, the naming scheme helps contrast
and relate GeolocationDispatcher and GeolocationDispatcherHost. For vibration,
this patch does not introduce a counterpart class in the renderer, so no need
for the awkward 'DispatcherHost' naming scheme.

[ostap, 2013-10-16 15:17:00]

On 2013/10/16 13:21:41, Michael van Ouwerkerk wrote:
> Please make sure this patch compiles. I ran some try bots, they look unhappy.

I have to do another rebase.

> Have you addressed the previous comments by the other reviewers? Maybe mark
them
> as done, or if they are obsolete add a note why.

Yes, they all addressed.
Patch was completely rewritten after last previous review.

>
https://codereview.chromium.org/23496051/diff/21001/content/browser/android/b...
> File content/browser/android/browser_jni_registrar.cc (right):
> 
>
https://codereview.chromium.org/23496051/diff/21001/content/browser/android/b...
> content/browser/android/browser_jni_registrar.cc:25: #include
> "content/browser/android/vibration_message_filter.h"
> Looks like you can delete this now.

Right.

>
https://codereview.chromium.org/23496051/diff/21001/content/browser/vibration...
> File content/browser/vibration/vibration_dispatcher_host.h (right):
> 
>
https://codereview.chromium.org/23496051/diff/21001/content/browser/vibration...
> content/browser/vibration/vibration_dispatcher_host.h:13: class
> VibrationDispatcherHost
> I prefer VibrationMessageFilter in this case (like it was). I think you may
have
> derived this from the geolocation code. There, the naming scheme helps
contrast
> and relate GeolocationDispatcher and GeolocationDispatcherHost. For vibration,
> this patch does not introduce a counterpart class in the renderer, so no need
> for the awkward 'DispatcherHost' naming scheme.

Ok

[ostap, 2013-11-06 00:28:06]

Patch rebased after r231091

[Michael van Ouwerkerk, 2013-11-08 14:41:05]

Please add more detail to the patch description.

Please also run the try bots after uploading a new patch by running "git cl
try".

https://codereview.chromium.org/23496051/diff/37001/content/browser/vibration...
File content/browser/vibration/vibration_message_filter.cc (right):

https://codereview.chromium.org/23496051/diff/37001/content/browser/vibration...
content/browser/vibration/vibration_message_filter.cc:22: class
VibrationMessageFilterImpl
It looks like you don't need the a separate Impl class. You could just implement
the methods in VibrationMessageFilter and declare the provider_ field in the
header.

https://codereview.chromium.org/23496051/diff/37001/content/browser/vibration...
content/browser/vibration/vibration_message_filter.cc:87:
VibrationMessageFilter* VibrationMessageFilter::New() {
It looks like you don't need this method, you can make the constructor public.

https://codereview.chromium.org/23496051/diff/37001/content/browser/vibration...
File content/browser/vibration/vibration_message_filter.h (right):

https://codereview.chromium.org/23496051/diff/37001/content/browser/vibration...
content/browser/vibration/vibration_message_filter.h:12: //
VibrationDispatcherHost is a browser filter for Vibration messages.
Update comment.

https://codereview.chromium.org/23496051/diff/37001/content/browser/vibration...
File content/browser/vibration/vibration_provider_android.h (right):

https://codereview.chromium.org/23496051/diff/37001/content/browser/vibration...
content/browser/vibration/vibration_provider_android.h:22: virtual void
OnVibrate(int64 milliseconds);
OVERRIDE

https://codereview.chromium.org/23496051/diff/37001/content/browser/vibration...
content/browser/vibration/vibration_provider_android.h:23: virtual void
OnCancelVibration();
OVERRIDE

https://codereview.chromium.org/23496051/diff/37001/content/port/browser/vibr...
File content/port/browser/vibration_provider.h (right):

https://codereview.chromium.org/23496051/diff/37001/content/port/browser/vibr...
content/port/browser/vibration_provider.h:13: virtual void OnVibrate(int64
milliseconds) = 0;
s/OnVibrate/Vibrate/

The 'On' prefix is only used for methods mapped with IPC_MESSAGE_HANDLER.

https://codereview.chromium.org/23496051/diff/37001/content/port/browser/vibr...
content/port/browser/vibration_provider.h:16: virtual void OnCancelVibration() =
0;
Same idea, s/OnCancelVibration/CancelVibration/

https://codereview.chromium.org/23496051/diff/37001/content/renderer/renderer...
File content/renderer/renderer_webkitplatformsupport_impl.h (right):

https://codereview.chromium.org/23496051/diff/37001/content/renderer/renderer...
content/renderer/renderer_webkitplatformsupport_impl.h:148: virtual void
vibrate(unsigned int milliseconds) OVERRIDE;
Don't add OVERRIDE here. It is generally not used in this file, so that it is
easier to make changes in Blink, without a compicated 3-sided patch.

https://codereview.chromium.org/23496051/diff/37001/content/renderer/renderer...
content/renderer/renderer_webkitplatformsupport_impl.h:149: virtual void
cancelVibration() OVERRIDE;
Also don't add OVERRIDE here.

[ostap, 2013-11-08 20:33:41]

> Please add more detail to the patch description.

Done

> Please also run the try bots after uploading a new patch by running "git cl
try".

Sorry, I'm not committer yet and it doesn't work for me.

https://codereview.chromium.org/23496051/diff/37001/content/browser/vibration...
File content/browser/vibration/vibration_message_filter.cc (right):

https://codereview.chromium.org/23496051/diff/37001/content/browser/vibration...
content/browser/vibration/vibration_message_filter.cc:22: class
VibrationMessageFilterImpl
On 2013/11/08 14:41:07, Michael van Ouwerkerk wrote:
> It looks like you don't need the a separate Impl class. You could just
implement
> the methods in VibrationMessageFilter and declare the provider_ field in the
> header.

Done.

https://codereview.chromium.org/23496051/diff/37001/content/browser/vibration...
File content/browser/vibration/vibration_message_filter.h (right):

https://codereview.chromium.org/23496051/diff/37001/content/browser/vibration...
content/browser/vibration/vibration_message_filter.h:12: //
VibrationDispatcherHost is a browser filter for Vibration messages.
On 2013/11/08 14:41:07, Michael van Ouwerkerk wrote:
> Update comment.

Done.

https://codereview.chromium.org/23496051/diff/37001/content/browser/vibration...
File content/browser/vibration/vibration_provider_android.h (right):

https://codereview.chromium.org/23496051/diff/37001/content/browser/vibration...
content/browser/vibration/vibration_provider_android.h:22: virtual void
OnVibrate(int64 milliseconds);
On 2013/11/08 14:41:07, Michael van Ouwerkerk wrote:
> OVERRIDE

Done.

https://codereview.chromium.org/23496051/diff/37001/content/browser/vibration...
content/browser/vibration/vibration_provider_android.h:23: virtual void
OnCancelVibration();
On 2013/11/08 14:41:07, Michael van Ouwerkerk wrote:
> OVERRIDE

Done.

https://codereview.chromium.org/23496051/diff/37001/content/port/browser/vibr...
File content/port/browser/vibration_provider.h (right):

https://codereview.chromium.org/23496051/diff/37001/content/port/browser/vibr...
content/port/browser/vibration_provider.h:13: virtual void OnVibrate(int64
milliseconds) = 0;
On 2013/11/08 14:41:07, Michael van Ouwerkerk wrote:
> s/OnVibrate/Vibrate/
> 
> The 'On' prefix is only used for methods mapped with IPC_MESSAGE_HANDLER.

Done.

https://codereview.chromium.org/23496051/diff/37001/content/port/browser/vibr...
content/port/browser/vibration_provider.h:16: virtual void OnCancelVibration() =
0;
On 2013/11/08 14:41:07, Michael van Ouwerkerk wrote:
> Same idea, s/OnCancelVibration/CancelVibration/

Done.

https://codereview.chromium.org/23496051/diff/37001/content/renderer/renderer...
File content/renderer/renderer_webkitplatformsupport_impl.h (right):

https://codereview.chromium.org/23496051/diff/37001/content/renderer/renderer...
content/renderer/renderer_webkitplatformsupport_impl.h:148: virtual void
vibrate(unsigned int milliseconds) OVERRIDE;
On 2013/11/08 14:41:07, Michael van Ouwerkerk wrote:
> Don't add OVERRIDE here. It is generally not used in this file, so that it is
> easier to make changes in Blink, without a compicated 3-sided patch.

Done.

https://codereview.chromium.org/23496051/diff/37001/content/renderer/renderer...
content/renderer/renderer_webkitplatformsupport_impl.h:149: virtual void
cancelVibration() OVERRIDE;
On 2013/11/08 14:41:07, Michael van Ouwerkerk wrote:
> Also don't add OVERRIDE here.

Done.

[Michael van Ouwerkerk, 2013-11-11 12:11:19]

https://codereview.chromium.org/23496051/diff/137001/content/browser/vibratio...
File content/browser/vibration/vibration_message_filter.cc (right):

https://codereview.chromium.org/23496051/diff/137001/content/browser/vibratio...
content/browser/vibration/vibration_message_filter.cc:28: if (!provider_)
When using scoped_ptr then checks like this become: if(!provider_.get())

https://codereview.chromium.org/23496051/diff/137001/content/browser/vibratio...
File content/browser/vibration/vibration_message_filter.h (right):

https://codereview.chromium.org/23496051/diff/137001/content/browser/vibratio...
content/browser/vibration/vibration_message_filter.h:16: : public
BrowserMessageFilter {
nit: looks like this can be joined onto line above

https://codereview.chromium.org/23496051/diff/137001/content/browser/vibratio...
content/browser/vibration/vibration_message_filter.h:19: virtual
~VibrationMessageFilter();
This can be private, like it was.

https://codereview.chromium.org/23496051/diff/137001/content/browser/vibratio...
content/browser/vibration/vibration_message_filter.h:29: VibrationProvider*
provider_;
I think this should be a scoped_ptr<VibrationProvider> -
http://www.chromium.org/developers/smart-pointer-guidelines

[ostap, 2013-11-11 18:02:13]

https://codereview.chromium.org/23496051/diff/137001/content/browser/vibratio...
File content/browser/vibration/vibration_message_filter.cc (right):

https://codereview.chromium.org/23496051/diff/137001/content/browser/vibratio...
content/browser/vibration/vibration_message_filter.cc:28: if (!provider_)
On 2013/11/11 12:11:20, Michael van Ouwerkerk wrote:
> When using scoped_ptr then checks like this become: if(!provider_.get())

Done.

https://codereview.chromium.org/23496051/diff/137001/content/browser/vibratio...
File content/browser/vibration/vibration_message_filter.h (right):

https://codereview.chromium.org/23496051/diff/137001/content/browser/vibratio...
content/browser/vibration/vibration_message_filter.h:16: : public
BrowserMessageFilter {
On 2013/11/11 12:11:20, Michael van Ouwerkerk wrote:
> nit: looks like this can be joined onto line above

Done.

https://codereview.chromium.org/23496051/diff/137001/content/browser/vibratio...
content/browser/vibration/vibration_message_filter.h:19: virtual
~VibrationMessageFilter();
On 2013/11/11 12:11:20, Michael van Ouwerkerk wrote:
> This can be private, like it was.

Done.

https://codereview.chromium.org/23496051/diff/137001/content/browser/vibratio...
content/browser/vibration/vibration_message_filter.h:29: VibrationProvider*
provider_;
On 2013/11/11 12:11:20, Michael van Ouwerkerk wrote:
> I think this should be a scoped_ptr<VibrationProvider> -
> http://www.chromium.org/developers/smart-pointer-guidelines

Done.

[Michael van Ouwerkerk, 2013-11-11 18:10:12]

Thanks!

lgtm

[ostap, 2013-11-18 18:26:12]

Please, can somebody review changes in content folder?

Thanks.

[Avi (use Gerrit), 2013-11-18 18:56:30]

When you first sent this CL out, you added a ton of reviewers but never
specified who you wanted to review what. I pointed this out, and asked you to
specify who you wanted to review it.

You failed to do so, and I believe that's why this is two months old.

You asked on the list, "Please, can somebody review changes in content folder?"
That is still not how you do it. You select someone who seems to be a reasonable
reviewer, and you ask them _specifically_ to do it. This way you avoid wasting
peoples' time; what happens if _two_ people respond to your call for review?

I will take the content review, but in the future, please _explicitly name
names_ when you are requesting a review.

https://codereview.chromium.org/23496051/diff/287001/content/browser/vibratio...
File content/browser/vibration/vibration_message_filter.cc (right):

https://codereview.chromium.org/23496051/diff/287001/content/browser/vibratio...
content/browser/vibration/vibration_message_filter.cc:51: if (!provider_.get())
return;
I have never seen the same-line if style in Chrome code. Please move the returns
to a new line.

https://codereview.chromium.org/23496051/diff/287001/content/browser/vibratio...
content/browser/vibration/vibration_message_filter.cc:62: if (!provider_.get())
return;
ditto

https://codereview.chromium.org/23496051/diff/287001/content/public/browser/c...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/23496051/diff/287001/content/public/browser/c...
content/public/browser/content_browser_client.h:577: virtual VibrationProvider*
OverrideVibrationProvider();
What's the rationale here? Are we expecting the embedder to need to override the
vibration provider?

[Avi (use Gerrit), 2013-11-18 18:59:04]

On 2013/09/12 16:14:30, ostap wrote:
> On 2013/09/12 05:05:57, Avi wrote:
> > Please specify which reviewers you are requesting to review which files.
> 
> Added to reviewers Peter Beverloo, who've reviewed original commits for this
> feature.

FYI, I meant something like:

Aaaa, please review xxxx files.
Bbbbb, please review yyy files.
Ccccc, please review zzz files.

This was a request for you to literally, explicitly, name people to review
explicitly-specified files.

[ostap, 2013-11-18 20:01:22]

On 2013/11/18 18:59:04, Avi wrote:
> On 2013/09/12 16:14:30, ostap wrote:
> > On 2013/09/12 05:05:57, Avi wrote:
> > > Please specify which reviewers you are requesting to review which files.
> > 
> > Added to reviewers Peter Beverloo, who've reviewed original commits for this
> > feature.
> 
> FYI, I meant something like:
> 
> Aaaa, please review xxxx files.
> Bbbbb, please review yyy files.
> Ccccc, please review zzz files.
> 
> This was a request for you to literally, explicitly, name people to review
> explicitly-specified files.

I'm sorry, I was really confused who should review this. Peter have asked
Michael van Ouwerkerk and he is not marked as owner or reviewer for affected
folders.
And now, after Michaels review, what extra reviews do I need for this patch?
Sorry, still figuring out how it works here. If there is multiple
reviewers/owners, how can I guess who is available for review?

I'll try to specify the list now:
content/browser/android/*
jochen@chromium.org, please review
content/browser/renderer_host/render_process_host_impl.cc and content/renderer/*

cdn@chromium.org, please review content/common/view_messages.h
joi@chromium.org, please review content/port/browser/vibration_provider.h
yfriedman@chromium.org, please review content/public/android/*
avi@chromium.org, please review content/public/* and content/*

Hope I've done it right this time.

[ostap, 2013-11-18 21:47:18]

jochen@chromium.org, please review
content/browser/renderer_host/render_process_host_impl.cc and content/renderer/*

cdn@chromium.org, please review content/common/view_messages.h
joi@chromium.org, please review content/port/browser/vibration_provider.h and
content/public/*
yfriedman@chromium.org, please review content/public/android/*
avi@chromium.org, please review content/*

https://codereview.chromium.org/23496051/diff/287001/content/browser/vibratio...
File content/browser/vibration/vibration_message_filter.cc (right):

https://codereview.chromium.org/23496051/diff/287001/content/browser/vibratio...
content/browser/vibration/vibration_message_filter.cc:51: if (!provider_.get())
return;
On 2013/11/18 18:56:31, Avi wrote:
> I have never seen the same-line if style in Chrome code. Please move the
returns
> to a new line.

Done.

https://codereview.chromium.org/23496051/diff/287001/content/browser/vibratio...
content/browser/vibration/vibration_message_filter.cc:62: if (!provider_.get())
return;
On 2013/11/18 18:56:31, Avi wrote:
> ditto

Done.

https://codereview.chromium.org/23496051/diff/287001/content/public/browser/c...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/23496051/diff/287001/content/public/browser/c...
content/public/browser/content_browser_client.h:577: virtual VibrationProvider*
OverrideVibrationProvider();
On 2013/11/18 18:56:31, Avi wrote:
> What's the rationale here? Are we expecting the embedder to need to override
the
> vibration provider?

Yes, it adds possibility to override vibration provider.

[Avi (use Gerrit), 2013-11-18 21:49:48]

https://codereview.chromium.org/23496051/diff/287001/content/public/browser/c...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/23496051/diff/287001/content/public/browser/c...
content/public/browser/content_browser_client.h:577: virtual VibrationProvider*
OverrideVibrationProvider();
On 2013/11/18 21:47:18, ostap wrote:
> On 2013/11/18 18:56:31, Avi wrote:
> > What's the rationale here? Are we expecting the embedder to need to override
> the
> > vibration provider?
> 
> Yes, it adds possibility to override vibration provider.

That wasn't my question. Yes, it offers the possibility, but are you expecting
that you'll need it? I'd rather hold off on that until we know it will be
needed, rather than put it in and have an unused override point sitting there.

[Yaron, 2013-11-18 22:52:12]

content/public/android lgtm

[Cris Neckar, 2013-11-18 22:58:07]

IPC changes LGTM

[Jói, 2013-11-19 12:07:48]

Like Avi, I would like to know whether there is already a use for this. If not,
I would prefer indirection not be introduced until necessary.

If this goes ahead, I think it would make sense to have a static factory method
on the VibrationProvider that returns the platform-default implementation. That
helps localize the platform specific bits better (the factory function can be
defined in the platform-specific files, with one default implementation that
returns NULL for any other platforms defined in vibration_provider.cc).

Cheers,
Jói

[jochen (gone - plz use gerrit), 2013-11-19 13:19:15]

I'm deferring to Avi, since he owns all of content/ anyway

[Avi (use Gerrit), 2013-11-19 17:24:50]

On 2013/11/19 12:07:48, Jói wrote:
> Like Avi, I would like to know whether there is already a use for this. If
not,
> I would prefer indirection not be introduced until necessary.
> 
> If this goes ahead, I think it would make sense to have a static factory
method
> on the VibrationProvider that returns the platform-default implementation.
That
> helps localize the platform specific bits better (the factory function can be
> defined in the platform-specific files, with one default implementation that
> returns NULL for any other platforms defined in vibration_provider.cc).
> 
> Cheers,
> Jói

I think the override was inspired by OverrideSystemLocationProvider (based on
the comment above "Redesign it similar to LocationProvider.).
OverrideSystemLocationProvider is unused and we're removing it.

Unless you have explicit plans for needing the override (please share them if
you do!) please remove OverrideVibrationProvider.

[ostap, 2013-11-19 18:07:07]

On 2013/11/19 17:24:50, Avi wrote:
> On 2013/11/19 12:07:48, Jói wrote:
> > Like Avi, I would like to know whether there is already a use for this. If
> not,
> > I would prefer indirection not be introduced until necessary.
> > 
> > If this goes ahead, I think it would make sense to have a static factory
> method
> > on the VibrationProvider that returns the platform-default implementation.
> That
> > helps localize the platform specific bits better (the factory function can
be
> > defined in the platform-specific files, with one default implementation that
> > returns NULL for any other platforms defined in vibration_provider.cc).
> > 
> > Cheers,
> > Jói
> 
> I think the override was inspired by OverrideSystemLocationProvider (based on
> the comment above "Redesign it similar to LocationProvider.).
> OverrideSystemLocationProvider is unused and we're removing it.
> 
> Unless you have explicit plans for needing the override (please share them if
> you do!) please remove OverrideVibrationProvider.

We use VibrationProvider in downstream project which is not opensoursed yet.
LocationProvider is used also, so I'd like to kindly ask not to remove it.

[Avi (use Gerrit), 2013-11-19 18:11:55]

Ah, didn't know that. Carry on.

LGTM once you fix minor formatting nits.

https://codereview.chromium.org/23496051/diff/477001/content/port/browser/vib...
File content/port/browser/vibration_provider.h (right):

https://codereview.chromium.org/23496051/diff/477001/content/port/browser/vib...
content/port/browser/vibration_provider.h:1: // Copyright (c) 2013 The Chromium
Authors. All rights reserved.
New files don't have "(c)", just "Copyright 2013..."

https://codereview.chromium.org/23496051/diff/477001/content/renderer/rendere...
File content/renderer/renderer_webkitplatformsupport_impl.h (right):

https://codereview.chromium.org/23496051/diff/477001/content/renderer/rendere...
content/renderer/renderer_webkitplatformsupport_impl.h:149: 
Drop blank line now that there is no #if.

[ostap, 2013-11-19 19:58:06]

On 2013/11/19 12:07:48, Jói wrote:
> If this goes ahead, I think it would make sense to have a static factory
method
> on the VibrationProvider that returns the platform-default implementation.
That
> helps localize the platform specific bits better (the factory function can be
> defined in the platform-specific files, with one default implementation that
> returns NULL for any other platforms defined in vibration_provider.cc).

Done

[ostap, 2013-11-19 19:58:32]

https://codereview.chromium.org/23496051/diff/477001/content/port/browser/vib...
File content/port/browser/vibration_provider.h (right):

https://codereview.chromium.org/23496051/diff/477001/content/port/browser/vib...
content/port/browser/vibration_provider.h:1: // Copyright (c) 2013 The Chromium
Authors. All rights reserved.
On 2013/11/19 18:11:56, Avi wrote:
> New files don't have "(c)", just "Copyright 2013..."

Done.

https://codereview.chromium.org/23496051/diff/477001/content/renderer/rendere...
File content/renderer/renderer_webkitplatformsupport_impl.h (right):

https://codereview.chromium.org/23496051/diff/477001/content/renderer/rendere...
content/renderer/renderer_webkitplatformsupport_impl.h:149: 
On 2013/11/19 18:11:56, Avi wrote:
> Drop blank line now that there is no #if.

Done.

[Avi (use Gerrit), 2013-11-22 15:36:34]

On 2013/11/19 12:07:48, Jói wrote:
> Like Avi, I would like to know whether there is already a use for this. If
not,
> I would prefer indirection not be introduced until necessary.
> 
> If this goes ahead, I think it would make sense to have a static factory
method
> on the VibrationProvider that returns the platform-default implementation.
That
> helps localize the platform specific bits better (the factory function can be
> defined in the platform-specific files, with one default implementation that
> returns NULL for any other platforms defined in vibration_provider.cc).
> 
> Cheers,
> Jói

Jói, you're the last reviewer. I'm not sure I see what you're asking. This is
modeled after the LocationProvider scheme. What are you looking for?

[ostap, 2013-11-22 15:56:02]

On 2013/11/22 15:36:34, Avi wrote:
> On 2013/11/19 12:07:48, Jói wrote:
> > If this goes ahead, I think it would make sense to have a static factory
> method
> > on the VibrationProvider that returns the platform-default implementation.
> That
> > helps localize the platform specific bits better (the factory function can
be
> > defined in the platform-specific files, with one default implementation that
> > returns NULL for any other platforms defined in vibration_provider.cc).
> 
> Jói, you're the last reviewer. I'm not sure I see what you're asking. This is
> modeled after the LocationProvider scheme. What are you looking for?

I actually implemented the factory method CreateProvider. The default
implementations is in vibration_message_filter.cc and it returns NULL. Android
implementation is in vibration_provider_android.cc .

[Avi (use Gerrit), 2013-11-22 16:08:48]

https://codereview.chromium.org/23496051/diff/607001/content/public/browser/c...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/23496051/diff/607001/content/public/browser/c...
content/public/browser/content_browser_client.h:576: // Return NULL to use the
default one for the platform to be created.
Please add the following line to this comment block and the LocationProvider
comment block above:

// FYI: Used by an external project; please don't remove. Contact xxx at yy
// for more information.

Edit it so that future maintainers have contact information for you and can make
sure they don't unnecessarily break you by removing these two API integration
points.

[ostap, 2013-11-22 17:24:18]

https://codereview.chromium.org/23496051/diff/607001/content/public/browser/c...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/23496051/diff/607001/content/public/browser/c...
content/public/browser/content_browser_client.h:576: // Return NULL to use the
default one for the platform to be created.
On 2013/11/22 16:08:48, Avi wrote:
> Please add the following line to this comment block and the LocationProvider
> comment block above:
> 
> // FYI: Used by an external project; please don't remove. Contact xxx at yy
> // for more information.
> 
> Edit it so that future maintainers have contact information for you and can
make
> sure they don't unnecessarily break you by removing these two API integration
> points.

Done.

[Avi (use Gerrit), 2013-11-22 21:38:57]

On 2013/11/22 17:24:18, ostap wrote:
>
https://codereview.chromium.org/23496051/diff/607001/content/public/browser/c...
> File content/public/browser/content_browser_client.h (right):
> 
>
https://codereview.chromium.org/23496051/diff/607001/content/public/browser/c...
> content/public/browser/content_browser_client.h:576: // Return NULL to use the
> default one for the platform to be created.
> On 2013/11/22 16:08:48, Avi wrote:
> > Please add the following line to this comment block and the LocationProvider
> > comment block above:
> > 
> > // FYI: Used by an external project; please don't remove. Contact xxx at yy
> > // for more information.
> > 
> > Edit it so that future maintainers have contact information for you and can
> make
> > sure they don't unnecessarily break you by removing these two API
integration
> > points.
> 
> Done.

Sweet. I think that will help.

[Jói, 2013-11-25 11:04:54]

LGTM

[commit-bot: I haz the power, 2013-11-25 16:12:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sl.ostapenko@samsung.com/23496051/707001

[commit-bot: I haz the power, 2013-11-25 18:12:57]

Change committed as 237101