Vibration API: use runtime flag, change from client to platform.

Vibration API: use runtime flag, change from client to platform.

Intent to implement & ship thread:
https://groups.google.com/a/chromium.org/forum/#!searchin/blink-dev/vibration$20api/blink-dev/hH9bJGWKAbk/AFPov-g5VMMJ

The runtime flag is by default disabled.

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

[Michael van Ouwerkerk, 2013-06-11 10:04:00]

Peter, could you please take a look at this patch? Thanks!

[Peter Beverloo, 2013-06-11 10:19:10]

Cool, thanks!

Could you clarify the CLs description to include:
  * That the Vibration API already passed the Intent to Implement and Ship
e-mails, perhaps with a link to the thread?
  * That the runtime flag will be *disabled* on all platforms?

https://codereview.chromium.org/15724023/diff/1/Source/modules/vibration/Navi...
File Source/modules/vibration/NavigatorVibration.cpp (right):

https://codereview.chromium.org/15724023/diff/1/Source/modules/vibration/Navi...
Source/modules/vibration/NavigatorVibration.cpp:49:
provideVibrationTo(navigator->frame()->page());
navigator->frame()->page() is used five times in this method, can you use a
variable for it instead?

https://codereview.chromium.org/15724023/diff/1/Source/modules/vibration/Navi...
Source/modules/vibration/NavigatorVibration.cpp:63:
provideVibrationTo(navigator->frame()->page());
dito

https://codereview.chromium.org/15724023/diff/1/Source/modules/vibration/Navi...
File Source/modules/vibration/NavigatorVibration.h (right):

https://codereview.chromium.org/15724023/diff/1/Source/modules/vibration/Navi...
Source/modules/vibration/NavigatorVibration.h:46: 
nit: blank line.

https://codereview.chromium.org/15724023/diff/1/Source/modules/vibration/Navi...
File Source/modules/vibration/NavigatorVibration.idl (right):

https://codereview.chromium.org/15724023/diff/1/Source/modules/vibration/Navi...
Source/modules/vibration/NavigatorVibration.idl:23: [RaisesException] void
vibrate(sequence<unsigned long> pattern);
Neither of these methods still raises an exception (we only fired
NOT_SUPPORTED_ERR when no Client had been provided to the Page).  Can you remove
the [RaisesException] annotation?  The latest ED has been updated to not throw
exceptions either.

https://codereview.chromium.org/15724023/diff/1/Source/modules/vibration/Vibr...
File Source/modules/vibration/Vibration.cpp (right):

https://codereview.chromium.org/15724023/diff/1/Source/modules/vibration/Vibr...
Source/modules/vibration/Vibration.cpp:79:
WebKit::Platform::current()->cancelVibration();
To confirm: cancelling the current vibration when the visibility state of the
tab changes will be done in content/, right?

https://codereview.chromium.org/15724023/diff/1/Source/modules/vibration/Vibr...
File Source/modules/vibration/Vibration.h (right):

https://codereview.chromium.org/15724023/diff/1/Source/modules/vibration/Vibr...
Source/modules/vibration/Vibration.h:59: void provideVibrationTo(Page*);
Since we already have three methods related to supplementing on Page on the
Vibration class itself, do you think it may be a good idea to include the
lazy-loading logic in ::from()?  The InternalSettings class already does this. 
Then you can remove the provideVibrationTo() method together with the checks in
Vibration.cpp.

https://codereview.chromium.org/15724023/diff/1/public/platform/Platform.h
File public/platform/Platform.h (right):

https://codereview.chromium.org/15724023/diff/1/public/platform/Platform.h#ne...
public/platform/Platform.h:385: // Vibration
-----------------------------------------------------------
nit: blank line after this one.

https://codereview.chromium.org/15724023/diff/1/public/platform/Platform.h#ne...
public/platform/Platform.h:387: virtual void vibrate(unsigned time) { }
I'd list "vibrate" before "cancelVibration", because cancelling is something you
can only do with an existing vibration (chronologically speaking).

Could you add basic one-line comments as to what these APIs do, despite the
clear naming?  The unit "time" has to be specified in would be useful
information, what happens with existing vibrations would be interesting,
etcetera.

[Michael van Ouwerkerk, 2013-06-11 13:26:51]

Thanks for the quick review! Please take another look.

https://codereview.chromium.org/15724023/diff/1/Source/modules/vibration/Navi...
File Source/modules/vibration/NavigatorVibration.cpp (right):

https://codereview.chromium.org/15724023/diff/1/Source/modules/vibration/Navi...
Source/modules/vibration/NavigatorVibration.cpp:49:
provideVibrationTo(navigator->frame()->page());
On 2013/06/11 10:19:10, Peter Beverloo wrote:
> navigator->frame()->page() is used five times in this method, can you use a
> variable for it instead?

Done.

https://codereview.chromium.org/15724023/diff/1/Source/modules/vibration/Navi...
Source/modules/vibration/NavigatorVibration.cpp:63:
provideVibrationTo(navigator->frame()->page());
On 2013/06/11 10:19:10, Peter Beverloo wrote:
> dito

Done.

https://codereview.chromium.org/15724023/diff/1/Source/modules/vibration/Navi...
File Source/modules/vibration/NavigatorVibration.h (right):

https://codereview.chromium.org/15724023/diff/1/Source/modules/vibration/Navi...
Source/modules/vibration/NavigatorVibration.h:46: 
On 2013/06/11 10:19:10, Peter Beverloo wrote:
> nit: blank line.

Done.

https://codereview.chromium.org/15724023/diff/1/Source/modules/vibration/Navi...
File Source/modules/vibration/NavigatorVibration.idl (right):

https://codereview.chromium.org/15724023/diff/1/Source/modules/vibration/Navi...
Source/modules/vibration/NavigatorVibration.idl:23: [RaisesException] void
vibrate(sequence<unsigned long> pattern);
On 2013/06/11 10:19:10, Peter Beverloo wrote:
> Neither of these methods still raises an exception (we only fired
> NOT_SUPPORTED_ERR when no Client had been provided to the Page).  Can you
remove
> the [RaisesException] annotation?  The latest ED has been updated to not throw
> exceptions either.

Done.

https://codereview.chromium.org/15724023/diff/1/Source/modules/vibration/Vibr...
File Source/modules/vibration/Vibration.cpp (right):

https://codereview.chromium.org/15724023/diff/1/Source/modules/vibration/Vibr...
Source/modules/vibration/Vibration.cpp:79:
WebKit::Platform::current()->cancelVibration();
On 2013/06/11 10:19:10, Peter Beverloo wrote:
> To confirm: cancelling the current vibration when the visibility state of the
> tab changes will be done in content/, right?

That's the plan. We'll see whether I can inline that in Page::setVisibilityState
or need to set up a listener.

https://codereview.chromium.org/15724023/diff/1/Source/modules/vibration/Vibr...
File Source/modules/vibration/Vibration.h (right):

https://codereview.chromium.org/15724023/diff/1/Source/modules/vibration/Vibr...
Source/modules/vibration/Vibration.h:59: void provideVibrationTo(Page*);
On 2013/06/11 10:19:10, Peter Beverloo wrote:
> Since we already have three methods related to supplementing on Page on the
> Vibration class itself, do you think it may be a good idea to include the
> lazy-loading logic in ::from()?  The InternalSettings class already does this.

> Then you can remove the provideVibrationTo() method together with the checks
in
> Vibration.cpp.

Much nicer. Done.

https://codereview.chromium.org/15724023/diff/1/public/platform/Platform.h
File public/platform/Platform.h (right):

https://codereview.chromium.org/15724023/diff/1/public/platform/Platform.h#ne...
public/platform/Platform.h:385: // Vibration
-----------------------------------------------------------
On 2013/06/11 10:19:10, Peter Beverloo wrote:
> nit: blank line after this one.

Done.

https://codereview.chromium.org/15724023/diff/1/public/platform/Platform.h#ne...
public/platform/Platform.h:387: virtual void vibrate(unsigned time) { }
On 2013/06/11 10:19:10, Peter Beverloo wrote:
> I'd list "vibrate" before "cancelVibration", because cancelling is something
you
> can only do with an existing vibration (chronologically speaking).
> 
> Could you add basic one-line comments as to what these APIs do, despite the
> clear naming?  The unit "time" has to be specified in would be useful
> information, what happens with existing vibrations would be interesting,
> etcetera.

Done.

[Peter Beverloo, 2013-06-11 13:50:12]

Please be sure to update the description of this issue, not just in your local
git state!  A few more nits.

https://codereview.chromium.org/15724023/diff/9001/Source/modules/vibration/N...
File Source/modules/vibration/NavigatorVibration.h (right):

https://codereview.chromium.org/15724023/diff/9001/Source/modules/vibration/N...
Source/modules/vibration/NavigatorVibration.h:28: class Uint32Array;
nit: this forward declaration isn't necessary, it's not being used.

https://codereview.chromium.org/15724023/diff/9001/Source/modules/vibration/N...
Source/modules/vibration/NavigatorVibration.h:38: NavigatorVibration();
Do we need the constructor and destructor?  They're both empty and all methods
in this class are static.

https://codereview.chromium.org/15724023/diff/9001/Source/modules/vibration/V...
File Source/modules/vibration/Vibration.cpp (right):

https://codereview.chromium.org/15724023/diff/9001/Source/modules/vibration/V...
Source/modules/vibration/Vibration.cpp:40: PassOwnPtr<Vibration>
Vibration::create()
This method isn't being used anymore, please remove it.

https://codereview.chromium.org/15724023/diff/9001/Source/modules/vibration/V...
File Source/modules/vibration/Vibration.h (right):

https://codereview.chromium.org/15724023/diff/9001/Source/modules/vibration/V...
Source/modules/vibration/Vibration.h:52: Vibration();
nit: newline under this method to visually separate it from the data members?

[Michael van Ouwerkerk, 2013-06-11 14:16:42]

Thanks again for the quick review! Please take another look.

https://codereview.chromium.org/15724023/diff/9001/Source/modules/vibration/N...
File Source/modules/vibration/NavigatorVibration.h (right):

https://codereview.chromium.org/15724023/diff/9001/Source/modules/vibration/N...
Source/modules/vibration/NavigatorVibration.h:28: class Uint32Array;
On 2013/06/11 13:50:12, Peter Beverloo wrote:
> nit: this forward declaration isn't necessary, it's not being used.

Done.

https://codereview.chromium.org/15724023/diff/9001/Source/modules/vibration/N...
Source/modules/vibration/NavigatorVibration.h:38: NavigatorVibration();
On 2013/06/11 13:50:12, Peter Beverloo wrote:
> Do we need the constructor and destructor?  They're both empty and all methods
> in this class are static.

Let's just leave a private constructor then.

https://codereview.chromium.org/15724023/diff/9001/Source/modules/vibration/V...
File Source/modules/vibration/Vibration.cpp (right):

https://codereview.chromium.org/15724023/diff/9001/Source/modules/vibration/V...
Source/modules/vibration/Vibration.cpp:40: PassOwnPtr<Vibration>
Vibration::create()
On 2013/06/11 13:50:12, Peter Beverloo wrote:
> This method isn't being used anymore, please remove it.

Done.

https://codereview.chromium.org/15724023/diff/9001/Source/modules/vibration/V...
File Source/modules/vibration/Vibration.h (right):

https://codereview.chromium.org/15724023/diff/9001/Source/modules/vibration/V...
Source/modules/vibration/Vibration.h:52: Vibration();
On 2013/06/11 13:50:12, Peter Beverloo wrote:
> nit: newline under this method to visually separate it from the data members?

Done.

[Peter Beverloo, 2013-06-11 14:30:36]

lgtm (but not an owner), this looks great, lots of red :-).  One nit to increase
the amount of redness even more.  Thanks!

https://codereview.chromium.org/15724023/diff/13001/Source/modules/vibration/...
File Source/modules/vibration/Vibration.h (right):

https://codereview.chromium.org/15724023/diff/13001/Source/modules/vibration/...
Source/modules/vibration/Vibration.h:47: static bool isActive(Page*);
Sorry, two more nits I missed earlier on.  isActive() can be removed because
it's not being used, and supplementName() can be private.

[Michael van Ouwerkerk, 2013-06-11 14:58:56]

Adam, could you take a look please for ownership? Thanks!

https://codereview.chromium.org/15724023/diff/13001/Source/modules/vibration/...
File Source/modules/vibration/Vibration.h (right):

https://codereview.chromium.org/15724023/diff/13001/Source/modules/vibration/...
Source/modules/vibration/Vibration.h:47: static bool isActive(Page*);
On 2013/06/11 14:30:36, Peter Beverloo wrote:
> Sorry, two more nits I missed earlier on.  isActive() can be removed because
> it's not being used, and supplementName() can be private.

Done.

[abarth-chromium, 2013-06-11 23:55:35]

LGTM with a few questions below.

https://codereview.chromium.org/15724023/diff/12002/Source/core/page/RuntimeE...
File Source/core/page/RuntimeEnabledFeatures.in (right):

https://codereview.chromium.org/15724023/diff/12002/Source/core/page/RuntimeE...
Source/core/page/RuntimeEnabledFeatures.in:69: Vibration
What is the status for this feature?  test?  experimental?

https://codereview.chromium.org/15724023/diff/12002/Source/modules/vibration/...
File Source/modules/vibration/NavigatorVibration.cpp (right):

https://codereview.chromium.org/15724023/diff/12002/Source/modules/vibration/...
Source/modules/vibration/NavigatorVibration.cpp:52: return;
Can you extract this copy/paste code into a helper function?

https://codereview.chromium.org/15724023/diff/12002/public/webpage/WebRuntime...
File public/webpage/WebRuntimeFeatures.h (right):

https://codereview.chromium.org/15724023/diff/12002/public/webpage/WebRuntime...
public/webpage/WebRuntimeFeatures.h:212: WEBKIT_EXPORT static bool
isVibrationEnabled();
Is there a reason this need to be controllable separately?  Do we not just want
to control it with the usual test/experimental/stable flags?

[abarth-chromium, 2013-06-11 23:57:31]

https://codereview.chromium.org/15724023/diff/12002/Source/modules/vibration/...
File Source/modules/vibration/Vibration.h (right):

https://codereview.chromium.org/15724023/diff/12002/Source/modules/vibration/...
Source/modules/vibration/Vibration.h:29: class Vibration : public
Supplement<Page> {
What's the point of having a separate Vibration page supplement?  It seems like
you could do all this work in NavigatorVibration if you made that object a
Supplement<Navigator>.

[Michael van Ouwerkerk, 2013-06-12 16:44:50]

Thanks for the quick review! I've made significant changes, so please take
another look (maybe Peter first).

https://codereview.chromium.org/15724023/diff/12002/Source/core/page/RuntimeE...
File Source/core/page/RuntimeEnabledFeatures.in (right):

https://codereview.chromium.org/15724023/diff/12002/Source/core/page/RuntimeE...
Source/core/page/RuntimeEnabledFeatures.in:69: Vibration
On 2013/06/11 23:55:36, abarth wrote:
> What is the status for this feature?  test?  experimental?

The status is currently blank, intentionally, which I understand is a valid
status. I'm only enabling it locally during development.

https://codereview.chromium.org/15724023/diff/12002/Source/modules/vibration/...
File Source/modules/vibration/NavigatorVibration.cpp (right):

https://codereview.chromium.org/15724023/diff/12002/Source/modules/vibration/...
Source/modules/vibration/NavigatorVibration.cpp:52: return;
On 2013/06/11 23:55:36, abarth wrote:
> Can you extract this copy/paste code into a helper function?

I've changed it so the one vibrate method calls the other.

https://codereview.chromium.org/15724023/diff/12002/Source/modules/vibration/...
File Source/modules/vibration/Vibration.h (right):

https://codereview.chromium.org/15724023/diff/12002/Source/modules/vibration/...
Source/modules/vibration/Vibration.h:29: class Vibration : public
Supplement<Page> {
On 2013/06/11 23:57:31, abarth wrote:
> What's the point of having a separate Vibration page supplement?  It seems
like
> you could do all this work in NavigatorVibration if you made that object a
> Supplement<Navigator>.

You're right, so: done! I was trying to keep this patch small but I understand
now that this code was ripe for a full review. I've rearranged most of it and
updated the algorithm and interface to adhere better to the latest version of
the editor's draft.

https://codereview.chromium.org/15724023/diff/12002/public/webpage/WebRuntime...
File public/webpage/WebRuntimeFeatures.h (right):

https://codereview.chromium.org/15724023/diff/12002/public/webpage/WebRuntime...
public/webpage/WebRuntimeFeatures.h:212: WEBKIT_EXPORT static bool
isVibrationEnabled();
On 2013/06/11 23:55:36, abarth wrote:
> Is there a reason this need to be controllable separately?  Do we not just
want
> to control it with the usual test/experimental/stable flags?

I use these during development to enable the API for Android in
render_thread_impl.cc (AdjustRuntimeFeatureDefaultsForPlatform).

Maybe these should be removed until we find a better use for them. I'll discuss
it with Peter.

[abarth-chromium, 2013-06-12 20:41:27]

This generally looks good.  Do we have LayoutTests that we can enable now that
we're enabling this feature?

https://codereview.chromium.org/15724023/diff/23001/Source/core/page/RuntimeE...
File Source/core/page/RuntimeEnabledFeatures.in (right):

https://codereview.chromium.org/15724023/diff/23001/Source/core/page/RuntimeE...
Source/core/page/RuntimeEnabledFeatures.in:70: Vibration
I suspect the status should be "test", which means the API is turned on only
during testing.  Presumably we'll want to test this feature.  :)

https://codereview.chromium.org/15724023/diff/23001/Source/modules/vibration/...
File Source/modules/vibration/NavigatorVibration.cpp (right):

https://codereview.chromium.org/15724023/diff/23001/Source/modules/vibration/...
Source/modules/vibration/NavigatorVibration.cpp:107:
m_timerStart.startOneShot(0);
Should we ASSERT that m_timerStart isn't active?

https://codereview.chromium.org/15724023/diff/23001/Source/modules/vibration/...
Source/modules/vibration/NavigatorVibration.cpp:114: m_timerStart.stop();
Is this necessary if we only use one-shot timers?

https://codereview.chromium.org/15724023/diff/23001/Source/modules/vibration/...
Source/modules/vibration/NavigatorVibration.cpp:159: return
static_cast<NavigatorVibration*>(Supplement<Navigator>::from(navigator,
supplementName()));
Please use the normal pattern for these "from" functions.  It's a couple lines
longer, but it avoids doing two hash lookups.

[Michael van Ouwerkerk, 2013-06-13 18:23:16]

Thanks for the quick review! Please take another look, Adam or Peter.

https://codereview.chromium.org/15724023/diff/23001/Source/core/page/RuntimeE...
File Source/core/page/RuntimeEnabledFeatures.in (right):

https://codereview.chromium.org/15724023/diff/23001/Source/core/page/RuntimeE...
Source/core/page/RuntimeEnabledFeatures.in:70: Vibration
On 2013/06/12 20:41:27, abarth wrote:
> I suspect the status should be "test", which means the API is turned on only
> during testing.  Presumably we'll want to test this feature.  :)

Done. I've fixed the layout tests.

https://codereview.chromium.org/15724023/diff/23001/Source/modules/vibration/...
File Source/modules/vibration/NavigatorVibration.cpp (right):

https://codereview.chromium.org/15724023/diff/23001/Source/modules/vibration/...
Source/modules/vibration/NavigatorVibration.cpp:107:
m_timerStart.startOneShot(0);
On 2013/06/12 20:41:27, abarth wrote:
> Should we ASSERT that m_timerStart isn't active?

Done.

https://codereview.chromium.org/15724023/diff/23001/Source/modules/vibration/...
Source/modules/vibration/NavigatorVibration.cpp:114: m_timerStart.stop();
On 2013/06/12 20:41:27, abarth wrote:
> Is this necessary if we only use one-shot timers?

It doesn't seem to be. I've removed it here and in timerStopFired.

https://codereview.chromium.org/15724023/diff/23001/Source/modules/vibration/...
Source/modules/vibration/NavigatorVibration.cpp:159: return
static_cast<NavigatorVibration*>(Supplement<Navigator>::from(navigator,
supplementName()));
On 2013/06/12 20:41:27, abarth wrote:
> Please use the normal pattern for these "from" functions.  It's a couple lines
> longer, but it avoids doing two hash lookups.

Done.

[abarth-chromium, 2013-06-13 20:13:20]

lgtm

A better test would mock out this platform API and log the API calls.  We can
leave that for the next CL, however.

https://codereview.chromium.org/15724023/diff/29001/public/platform/Platform.h
File public/platform/Platform.h (right):

https://codereview.chromium.org/15724023/diff/29001/public/platform/Platform....
public/platform/Platform.h:385: virtual void vibrate(unsigned time) { }
time -> durationMS ?

[commit-bot: I haz the power, 2013-06-13 20:13:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mvanouwerkerk@chromium.org/15724023/29001

[commit-bot: I haz the power, 2013-06-13 21:22:36]

Retried try job too often on linux_layout_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_layo...

[abarth-chromium, 2013-06-13 21:32:44]

On 2013/06/13 21:22:36, I haz the power (commit-bot) wrote:
> Retried try job too often on linux_layout_rel for step(s) webkit_tests
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_layo...

Looks like you might need to update navigator-detached-no-crash.html to
understand this new property.

[commit-bot: I haz the power, 2013-06-18 09:52:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mvanouwerkerk@chromium.org/15724023/53001

[commit-bot: I haz the power, 2013-06-18 11:48:08]

Change committed as 152627