Last Comment Bug 679966 - WebVibrator
: WebVibrator
Status: RESOLVED FIXED
[secr:curtisk]
: dev-doc-complete
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- enhancement (vote)
: mozilla11
Assigned To: Justin Lebar (not reading bugmail)
:
:
Mentors:
http://www.youtube.com/watch?v=VpFITa...
Depends on: 698057 792891 672352 690056 690670 698782 701716 706958 769571
Blocks: webapi 647541 694862
  Show dependency treegraph
 
Reported: 2011-08-17 19:00 PDT by Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
Modified: 2012-11-07 04:24 PST (History)
46 users (show)
dveditz: sec‑review+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
unaffected


Attachments
patch, incomplete (16.54 KB, patch)
2011-08-17 22:56 PDT, Andreas Gal :gal
no flags Details | Diff | Splinter Review
pong (20.58 KB, patch)
2011-08-18 00:48 PDT, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
no flags Details | Diff | Splinter Review
functional WIP (46.52 KB, patch)
2011-09-29 02:17 PDT, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
no flags Details | Diff | Splinter Review
part 1: Add vibrator support for android (12.58 KB, patch)
2011-09-30 00:01 PDT, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
blassey.bugs: review+
Details | Diff | Splinter Review
DOM API. WIP (3.46 KB, patch)
2011-09-30 00:03 PDT, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
no flags Details | Diff | Splinter Review
Part 2, v1: DOM API (10.71 KB, patch)
2011-10-16 08:26 PDT, Justin Lebar (not reading bugmail)
bzbarsky: review-
Details | Diff | Splinter Review
Part 1, v2: Add vibrator support for Android. (13.47 KB, patch)
2011-10-16 08:28 PDT, Justin Lebar (not reading bugmail)
blassey.bugs: review+
Details | Diff | Splinter Review
Manual testcase (3.17 KB, text/html)
2011-10-16 08:31 PDT, Justin Lebar (not reading bugmail)
no flags Details
Part 2, v2: DOM API (13.94 KB, patch)
2011-10-18 10:19 PDT, Justin Lebar (not reading bugmail)
no flags Details | Diff | Splinter Review
Part 2, v3: DOM API (14.08 KB, patch)
2011-10-18 14:06 PDT, Justin Lebar (not reading bugmail)
no flags Details | Diff | Splinter Review
Manual testcase, v2 (4.15 KB, text/html)
2011-10-18 14:06 PDT, Justin Lebar (not reading bugmail)
no flags Details
Part 2, v4: DOM API (13.71 KB, patch)
2011-10-21 14:30 PDT, Justin Lebar (not reading bugmail)
no flags Details | Diff | Splinter Review
Part 1, v3: Vibrator support (29.96 KB, patch)
2011-10-21 14:33 PDT, Justin Lebar (not reading bugmail)
no flags Details | Diff | Splinter Review
Part 1, v3.1: Vibrator support (29.09 KB, patch)
2011-10-21 14:47 PDT, Justin Lebar (not reading bugmail)
no flags Details | Diff | Splinter Review
Interdiff part 1 v2 --> v3.1 (2.11 KB, patch)
2011-10-21 14:49 PDT, Justin Lebar (not reading bugmail)
no flags Details | Diff | Splinter Review
Part 1, v4 (64.72 KB, patch)
2011-10-28 09:39 PDT, Justin Lebar (not reading bugmail)
no flags Details | Diff | Splinter Review
Part 2, v5 (11.67 KB, patch)
2011-10-28 09:40 PDT, Justin Lebar (not reading bugmail)
no flags Details | Diff | Splinter Review
Part 1, v4.1 (64.75 KB, patch)
2011-10-28 11:03 PDT, Justin Lebar (not reading bugmail)
no flags Details | Diff | Splinter Review
Part 2, v5.1 (11.66 KB, patch)
2011-10-28 11:04 PDT, Justin Lebar (not reading bugmail)
no flags Details | Diff | Splinter Review
Part 1, v5 (39.29 KB, patch)
2011-11-01 15:19 PDT, Justin Lebar (not reading bugmail)
cjones.bugs: review+
blassey.bugs: review+
justin.lebar+bug: checkin+
Details | Diff | Splinter Review
Part 2, v6 (14.27 KB, patch)
2011-11-01 15:21 PDT, Justin Lebar (not reading bugmail)
no flags Details | Diff | Splinter Review
Empty placeholder patch - back out part 1 from Aurora (see comment 143) (49 bytes, patch)
2011-11-08 19:43 PST, Justin Lebar (not reading bugmail)
christian: approval‑mozilla‑aurora+
Details | Diff | Splinter Review
Manual testcase, v3 (4.28 KB, text/html)
2011-11-09 10:25 PST, Justin Lebar (not reading bugmail)
no flags Details
Part 2, v7 (13.70 KB, patch)
2011-11-09 13:04 PST, Justin Lebar (not reading bugmail)
no flags Details | Diff | Splinter Review
Part 2, v8 (13.91 KB, patch)
2011-11-11 08:07 PST, Justin Lebar (not reading bugmail)
no flags Details | Diff | Splinter Review
Part 2, v9 (16.71 KB, patch)
2011-12-02 10:55 PST, Justin Lebar (not reading bugmail)
bzbarsky: review+
cjones.bugs: review+
Details | Diff | Splinter Review
Manual testcase, v4 (4.02 KB, text/html)
2011-12-02 10:55 PST, Justin Lebar (not reading bugmail)
no flags Details

Description Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-08-17 19:00:07 PDT
(Why use the working title WebVibrator?  Because I can.)

With bug 595441, we'll provide haptic feedback automatically on button presses and so forth.  That's not the goal of this bug.  The goal of this bug is to let web devs control the vibrator as directly as possible, for the sake of games etc.

W3C and WAC don't have proposals for this.  Here's the android one

http://developer.android.com/reference/android/os/Vibrator.html

It would be cool to allow changing vibration intensity too, for an effect like a tank driving through your field of view in a game.
Comment 1 Andreas Gal :gal 2011-08-17 19:05:57 PDT
Where should we stick this? navigator.Vibrator? or window.Vibrator?

Also, I don't like hasVibrator(). Thats an identifying bit and a privacy concern.

How about simply:

navigator.vibrate(pattern, [repeat])

pattern can be a number (ms), or an array of numbers (ms).

repeat is optional and 1 by default.

navigator.vibrate(0) turns off the vibrator.

If the device has no vibrator, nothing happens.
Comment 2 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-08-17 19:09:12 PDT
I agree about no-op'ing when the vibrator is absent.

That API seems OK except for the varying-intensity use case.  I'm OK putting that off for now.  If we expose a device setting for intensity, then authors can use that to hack something up.

Oh, one thing I neglected: I think this API can be allowed by default, with an option given to the user to turn it off if it's too annoying.  Anyone know of security concerns other than battery drain?
Comment 3 Andrew McCreight [:mccr8] 2011-08-17 19:13:56 PDT
Can you infer the presence of a vibrator using a motion sensor?  I guess it is probably not that sensitive, especially if somebody is holding the device.
Comment 4 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-08-17 19:15:13 PDT
(In reply to Chris Jones [:cjones] [:warhammer] from comment #2)
> That API seems OK except for the varying-intensity use case.  I'm OK putting
> that off for now.  If we expose a device setting for intensity, then authors
> can use that to hack something up.

I guess authors can simulate that well enough with modulation patterns.  Ignore me.
Comment 5 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-08-17 19:17:13 PDT
(In reply to Andrew McCreight [:mccr8] from comment #3)
> Can you infer the presence of a vibrator using a motion sensor?  I guess it
> is probably not that sensitive, especially if somebody is holding the device.

It's possible, e.g. I've had vibration shake my phone off of precarious perches.  I wonder though if there are many phones that have accelerometers but not vibrators.
Comment 6 Doug Turner (:dougt) 2011-08-17 22:07:30 PDT
we have the plumbing for haptic feedback in chrome code.

also see:  https://bugzilla.mozilla.org/show_bug.cgi?id=518266#c10
Comment 7 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-08-17 22:19:45 PDT
I'm not sure there's much of a use case for web content performing stereotyped actions ("longtap" etc.).  If content uses the right HTML controls etc., those will happen automatically right?
Comment 8 Doug Turner (:dougt) 2011-08-17 22:22:40 PDT
cjones, probably.  being able to provide a pattern like comment #1 would fucking rock.
Comment 9 Andreas Gal :gal 2011-08-17 22:56:17 PDT
Created attachment 554000 [details] [diff] [review]
patch, incomplete
Comment 10 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-08-18 00:48:10 PDT
Created attachment 554010 [details] [diff] [review]
pong

 - mostly builds, erroring out in gal code in AndroidBridge
 - won't work: needs a path from nsNavigation to an nsIWidget
 - might be some quibbling over org, went with what I know
Comment 11 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-08-18 00:51:24 PDT
Oh and to be clear,

> navigator.vibrate(pattern, [repeat])
> 
> repeat is optional and 1 by default.

The code treats the default as repeat=0, and I prefer that now.  By default, the pattern is vibrated once, but not repeated.  So repeat=1 would vibrated the pattern twice.
Comment 12 Brendan Eich [:brendan] 2011-08-18 11:10:25 PDT
Why are you guys injecting new APIs into navigator? We need a more principled and extensible approach.

/be
Comment 13 Andreas Gal :gal 2011-08-18 11:14:06 PDT
Open to suggestions. We could follow W3C and make it window.devices.Vibrator.vibrate(). Post suggestions and lets pick a winner. I really don't care much.
Comment 14 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-08-18 11:15:09 PDT
Seconded.  This is not something I want to even have to think about.
Comment 15 Brendan Eich [:brendan] 2011-08-18 11:19:58 PDT
Someone here had better think about two things, and think hard:

1. Developer ergonomics, usability.

2. Security, including DOS/annoyance controls.

/be
Comment 16 Andreas Gal :gal 2011-08-18 11:24:20 PDT
(2) reminds me that we should cancel vibration when the user leaves the site.
Comment 17 Jonas Sicking (:sicking) No longer reading bugmail consistently 2011-08-18 11:46:22 PDT
Brendan, where do you propose that is better than navigator? A lot of new APIs have been proposed there lately.

I wish we could use ECMA modules, but my understanding was that they are still some ways out?
Comment 18 Dave Herman [:dherman] 2011-08-18 12:03:48 PDT
> I wish we could use ECMA modules, but my understanding was that they are
> still some ways out?

Just this week Jason has begun work on implementing ES6 modules in SpiderMonkey (bug 568953). We don't yet have an ETA though.

Dave
Comment 19 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-08-18 13:25:05 PDT
Are any other engines/browsers signed on to ES6 modules yet?
Comment 20 K. Gadd (:kael) 2011-08-18 13:25:34 PDT
I'll just mention that many modern joysticks/game controllers have built in vibration motors, and none of the current API proposals for reading input from those controllers allow you to control the motor.

It might be worth considering that use case in the design of this API, since that would make content that uses it testable on desktop/laptop computers, given a game controller plugged in. I think the biggest potential consideration there is what you do when there are multiple motors: Do you expose the motors as devices, accept a motor parameter to the vibrate method, or just autoselect one motor and use it for everything?
Comment 21 Dave Herman [:dherman] 2011-08-18 13:32:53 PDT
> Are any other engines/browsers signed on to ES6 modules yet?

There's broad consensus on TC39 that modules are very important, but nobody has yet made any promises about implementing. There's still more spec work to be done, but we're far enough along that implementation work can proceed. I know that there are people working on other engines who speak very positively about ES6 modules, but nobody has made commitments yet.

Dave
Comment 22 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-08-18 13:37:19 PDT
OK.  I'm strongly in favor of us implementing modules ASAP, but would register a concern about tying new APIs we're trying to standardize through other channels to ES6 modules.  That raises a much higher barrier to other implementations and thus standardization.
Comment 23 Jonas Sicking (:sicking) No longer reading bugmail consistently 2011-08-18 14:38:49 PDT
I share that same concern. Unless we know that at the very least one, ideally a couple, other JS implementation out there is about to implement modules, I think it'll be hard to rely on them.
Comment 24 Dave Herman [:dherman] 2011-08-18 15:34:22 PDT
Modules will happen if it kills me. I'm callin' this shot like Babe Ruth.

But I'm not predicting when. I was only answering people's questions, not advocating. This already came up in bug 674718, and people already agreed that we aren't using modules for b2g-inspired API's, at least for the time being. Eventually, all standard API's should have a standard home in an ES6 module.

Dave
Comment 25 Paul Bakaus 2011-08-19 00:40:02 PDT
Thanks for adding me to the discussion.

I agree to Brendan's concerns for extensibility, it's hard to say if we can change the path of device APIs all together though.

Compare http://dev.w3.org/geo/api/spec-source-orientation.html with http://dev.w3.org/geo/api/spec-source.html. There's clearly work to be done to align them.

IMHO, navigator is closest to the actual device, so I'd say introduce navigator.device as a namespace, allow it to take events and put everything in there.

one of the following:
navigator.addEventListener("orientationchange", fn, false)
navigator.orientation.addEventListener("change", fn, false)

for vibrating, something like this:

navigator.vibration.vibrate()
navigator.vibration.addEventListener("vibrate", fn, false)

I also have to admit that I'm in favor of exposing wether the device can vibrate or not. As a game designer, I would probably try to highlight features differently when there's no vibrating controller.
Comment 26 Andreas Gal :gal 2011-08-19 01:16:53 PDT
I feel pretty strongly about not exposing additional identifying information. I think game developers can guess from the device type (mobile -> yes, desktop -> no). Thats a close enough approximation IMO. I might be wrong.

As for navigator.vibration, the main reason I put the function directly on navigator because if we add an object that navigator.vibration returns, WebIDL requires that we add its constructor to the global namespace, so I would have to add window.Vibration and navigator.vibration and Vibration.prototype.vibrate(). That seems a lot more effort than what I propose, but I don't feel strongly about this. Its just a slight extra namespace pollution (Vibration). If you and Brendan agree that it should look like this, I am happy to change it.

Also, thanks a lot for chiming in. Your opinion is _very_ welcome here.
Comment 27 John Drinkwater (:beta) 2011-08-23 07:05:44 PDT
Future portable devices are likely to get force‐feedback / rumble support, maybe this API should be comprehensive enough to cover simple *vibrate* means and additional positional/material feedback for Games and accessibility.
Comment 28 AWAY Tom Schuster [:evilpie] 2011-08-25 08:28:28 PDT
Please don't forget to implement HTML 11 <vibration>, for a in depth specification see http://html11.org/vibration.html. Also we should partner with the porn industry and ask for their specific needs of such a feature.
Comment 29 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-09-29 02:17:12 PDT
Created attachment 563345 [details] [diff] [review]
functional WIP

Includes some gunk that will be split into another bug.

This doesn't disable vibrator access for inactive docshells.  We'll probably want that, though I'm not 100% sure how something like an invisible SMS app would silently notify on incoming SMS otherwise.  Maybe that's something we need to extend the Notifications spec to cover.  This also doesn't implement an "annoyance detector", but if we disable vibrator on docshell inactivity then navigating away or closing the offending tab makes the problem go away.  Maybe that's enough?

It's going to be tricky to spec behavior if multiple active pages can access the vibrator concurrently.
Comment 30 Robin Berjon 2011-09-29 03:07:36 PDT
I don't think that an invisible SMS app is a use case for direct vibrator access. SMS apps (and others similar) should really not make the decision as to whether they should use sound, light, dialogs, beeps, vibration, or whatever else to inform the user. They should trigger a notification, and it's up to the notification system to know how to inform the user and serves as the single entry point for the user to configure this — otherwise you just get a completely unusable mess!

This API ought to be useful for the likes of haptic feedback, games, etc. Restricting it to active docshells seems to be most sensible to me considering the use cases.
Comment 31 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-09-30 00:01:35 PDT
Created attachment 563673 [details] [diff] [review]
part 1: Add vibrator support for android
Comment 32 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-09-30 00:03:07 PDT
Created attachment 563674 [details] [diff] [review]
DOM API.  WIP
Comment 33 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-10-04 17:44:00 PDT
bz (or whoever else), I'd like to implement disable-vibrator-access-on-hidden-or-disabled.  Here's what I can think of so far

 - access should be disabled when !docShell.isActive
 - on transitioning from docShell.isActive->!isActive, any in-progress vibration should be canceled
 - ... and similarly for every sub-window of an outer window

Any other relevant state that should disable vibrator?

How should my code listen for these state changes and react to them?  What's the best place to store any tracking state I might need?

One issue is that it would be nice to cancel any pending vibrations for a window and all its sub-windows before enabling vibrator access for the next "active" window, to avoid race conditions that would accidentally lead to vibrations from the newly activated window being canceled.  Is it true that |oldDocShell.isActive<-false| and |newDocShell.isActive<-true| are always sequenced?  (I would imagine not.)  If not, I might not care about this race condition enough to fix it up front.
Comment 34 Boris Zbarsky [:bz] (still a bit busy) 2011-10-04 19:58:46 PDT
> Is it true that |oldDocShell.isActive<-false| and |newDocShell.isActive<-true| are always
> sequenced?

No.  We could try to ensure it in the tabbrowser, and it may be true by accident now, but nothing enforces it at the moment.

Does a given vibration know which document it's associated with?  Seems like if it did it could just add a visibilitychange listener on that document and cancel itself when the visibility changes to hidden.  This won't happen sync under the isActive set (instead, it will take a trip through the event loop), but I'm not sure that's a problem...  That would make sure, in particular, that you only disable the things whose document went inactive.
Comment 35 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-10-04 20:18:59 PDT
(In reply to Boris Zbarsky (:bz) from comment #34)
> Does a given vibration know which document it's associated with?

The vibration is scheduled from window.navigator.mozVibrator.  Would window.document give me the "right" document?

> Seems like
> if it did it could just add a visibilitychange listener on that document and
> cancel itself when the visibility changes to hidden.  This won't happen sync
> under the isActive set (instead, it will take a trip through the event
> loop), but I'm not sure that's a problem...  That would make sure, in
> particular, that you only disable the things whose document went inactive.

OK, I think this makes sense.  That can all be done from C++, I assume?
Comment 36 Boris Zbarsky [:bz] (still a bit busy) 2011-10-04 20:31:24 PDT
> Would window.document give me the "right" document?

Yes.

> That can all be done from C++, I assume?

Sure.  A bit more painful than from JS (e.g. need an nsIDOMEventListener subclass), but quite doable.
Comment 37 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-10-04 20:38:57 PDT
(In reply to Boris Zbarsky (:bz) from comment #36)
> > That can all be done from C++, I assume?
> 
> Sure.  A bit more painful than from JS (e.g. need an nsIDOMEventListener
> subclass), but quite doable.

I suspect we'll need to use this mechanism a lot with upcoming web API work, so an initial investment in goop is worth it.  Sucks for me ;).
Comment 38 Ted Mielczarek [:ted.mielczarek] 2011-10-06 04:54:33 PDT
I mentioned in bug 680289, there are some game controllers that have more than one vibration motor. The Xbox 360 controller has two: a low-frequency and high-frequency, and you can control them separately:
http://msdn.microsoft.com/en-us/library/microsoft.directx_sdk.reference.xinput_vibration%28v=vs.85%29.aspx

I'm not sure if this is something that exists in mobile devices, but it's something to keep in mind when designing APIs. (Clearly the Android APIs have not worried about it.)
Comment 39 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-10-06 16:41:39 PDT
Vibration in game controllers is a different problem.  This API (not finalized) is explicitly for "default system vibrator".  If a use case arises where support for multiple system vibrators is needed (doesn't seem likely), then we can add on to this API.

FWIW, not just android but also iOS, WP7, and maemo assume one system vibrator.
Comment 40 Justin Lebar (not reading bugmail) 2011-10-13 15:34:38 PDT
cjones, for the purposes of B2G, we're not going to vibrate by going through the Java bridge, right?  Can you point me to code which does the appropriate thing in that case?
Comment 41 Andreas Gal :gal 2011-10-13 15:37:11 PDT
jlebar, correct, and that code needs to be investigated, look at the source in glue/gonk
Comment 42 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-10-13 16:10:53 PDT
(In reply to Justin Lebar [:jlebar] from comment #40)
> cjones, for the purposes of B2G, we're not going to vibrate by going through
> the Java bridge, right?  Can you point me to code which does the appropriate
> thing in that case?

Sure, but I would recommend doing the work in the following order
 (1) Finish the DOM impl by canceling vibration when the page is hidden
 (2) Tests.  They won't be incredibly useful at first, don't worry.  Need a human for functional testing right now.
 (2) Land the new API and android impl
 (3) Work on gonk backend.  Probably better as a followup.

FTR, the C interface to the library the vibrator lives in is vibrator_on/vibrator_off() in libhardware_legacy.h, and the implementation of that API lives in libhardware_legacy/vibrator/vibrator.c.  Initially we can target the libhardware_legacy interface and later decide which abstraction level makes sense in the long run (with feedback from appropriate folks).  It's not a big jump between the two levels.
Comment 43 Jose Manuel Cantera 2011-10-14 00:06:15 PDT
Be careful. vibrator_on, vibrator_off is not going to work properly on certain devices, like Samsung Galaxy S. The thing is that they use Immersion technology to deal with Haptic feedback. Basically, vibration is done through the functions provided by the libImmVibeJ.so library. In addition there is a daemon immived that controls haptic feedback generation (and if you kill it the device stops working properly). Also, if you see the source code of the Galaxy S kernel there is a module related to this functionality and which controls the vibration motor.
Comment 44 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-10-14 01:16:02 PDT
Thanks for the heads-up.
Comment 45 Justin Lebar (not reading bugmail) 2011-10-16 06:54:22 PDT
We can cap the length of the array sent to mozVibrate() and the vibration lengths within the array.  But it's much harder to keep a determined page from vibrating the phone like crazy, since you can always call mozVibrate from within a setInterval.  I'm not sure we care...
Comment 46 Justin Lebar (not reading bugmail) 2011-10-16 08:26:40 PDT
Created attachment 567341 [details] [diff] [review]
Part 2, v1: DOM API
Comment 47 Justin Lebar (not reading bugmail) 2011-10-16 08:28:11 PDT
Created attachment 567342 [details] [diff] [review]
Part 1, v2: Add vibrator support for Android.
Comment 48 Justin Lebar (not reading bugmail) 2011-10-16 08:30:39 PDT
Comment on attachment 567342 [details] [diff] [review]
Part 1, v2: Add vibrator support for Android.

Brad, I made one change to the patch you r+'ed.  In AndroidBridge::Vibrate, we now stick a 0 at the beginning of the array, since the first element of the array is the delay before we start vibrating.
Comment 49 Justin Lebar (not reading bugmail) 2011-10-16 08:31:35 PDT
Created attachment 567343 [details]
Manual testcase

I'm going to get this manual testcase added to Litmus.
Comment 50 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-10-16 14:47:30 PDT
(In reply to Justin Lebar [:jlebar] from comment #45)
> We can cap the length of the array sent to mozVibrate() and the vibration
> lengths within the array.  But it's much harder to keep a determined page
> from vibrating the phone like crazy, since you can always call mozVibrate
> from within a setInterval.  I'm not sure we care...

Yes, pages will be able to find ways to keep the vibrator on constantly.  (Just like they can find ways to fire timers constantly, even though we throttle the timer interval.)  I don't care about defending against that in the API since there are use cases in games in which the vibrator would want to fire like crazy for periods of time.  What we *do* care about is making sure that users can I DON'T LIKE IT MAKE IT STOP.  We'll do that initially by ensuring that putting the page into the background and closing or navigating away from the page turns off the vibrator.  We can look into cleverer things down the road if the need arises, like having a "vibrator volume" settable per page.
Comment 51 Boris Zbarsky [:bz] (still a bit busy) 2011-10-17 10:17:42 PDT
Comment on attachment 567341 [details] [diff] [review]
Part 2, v1: DOM API

So passing 5 as a pattern will work, but passing 5.0 won't work?

Worse yet, if you get 5 as a result of some arithmetic it may or may not work depending on the arithmetic and the type-specialization hoops the jit jumped through.

Or put another way, I think your JSVAL_IS_INT test should allow doubles as well.

As far as that goes, what about passing "5"?  Think a page storing a user preference for how long to vibrate in some string-only store like localStorage.  I've seen web pages not even realize that they had a string, not a number, in some cases...

If what you really want here is to convert the arg to integer (after checking for null and undefined?), then maybe you can reuse the code quickstubs uses for that?

As far as that goes, maybe 0 should simply stop the vibrator?  That way null and undefined will Just Work when converted to integer....

Calling MozVibrate N times will add N listeners to the document.  This seems undesirable.  Should the listener not be removed when the vibration completes or is canceled?

The IDL should probably document what the argument means, given that there is no spec for this.
Comment 52 Justin Lebar (not reading bugmail) 2011-10-17 10:31:24 PDT
> So passing 5 as a pattern will work, but passing 5.0 won't work?

This must be my strong-typing background corrupting my API design.  :)

These changes seem right to me.
Comment 53 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-10-17 10:35:53 PDT
(In reply to Justin Lebar [:jlebar] from comment #52)
> > So passing 5 as a pattern will work, but passing 5.0 won't work?
> 
> This must be my strong-typing background corrupting my API design.  :)

You say that like it's a bad thing.
Comment 54 Justin Lebar (not reading bugmail) 2011-10-17 13:13:44 PDT
> Calling MozVibrate N times will add N listeners to the document.  This seems undesirable.  Should 
> the listener not be removed when the vibration completes or is canceled?

Maybe we can just make sure we never add the listener more than once?

Where should that boolean go?  On the document, on the global window, on the navigator?  I'm not clear as to which of these things out live the others.
Comment 55 Boris Zbarsky [:bz] (still a bit busy) 2011-10-17 13:26:25 PDT
> Maybe we can just make sure we never add the listener more than once?

Wouldn't that make it cancel vibrate if this document is switched from visible to hidden in the future even if this document wasn't the one that started the vibrate?
Comment 56 Justin Lebar (not reading bugmail) 2011-10-17 13:32:45 PDT
I guess I've been operating under the assumption that there's only one visible document at a time, so that we always blame the current vibration (if any) on the currently-visible document.  That's bogus on desktop; is it also on mobile?
Comment 57 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-10-17 13:35:49 PDT
Yes, iframes can use vibrate().  You need to track the last window ID that triggered a vibrate in the chrome-side impl and when navigating away and sending a cancel, do the window-ID check to ensure there's not a bogus cancellation.
Comment 58 Boris Zbarsky [:bz] (still a bit busy) 2011-10-17 13:43:30 PDT
I believe that's bogus on mobile with iframes, yeah....
Comment 59 Olli Pettay [:smaug] (reviewing overload) 2011-10-17 15:34:18 PDT
Comment on attachment 567341 [details] [diff] [review]
Part 2, v1: DOM API


>+  // Add a listener to cancel the vibration if the document becomes hidden.
>+  nsCOMPtr<VibrateWindowListener> listener = new VibrateWindowListener();
>+  doc->AddEventListener(NS_LITERAL_STRING("mozvisibilitychange"), listener, true);
If you're adding a listener to web page, you sure want to listen for only trusted events and
only in system event group.
Comment 60 Justin Lebar (not reading bugmail) 2011-10-18 10:19:14 PDT
Created attachment 567785 [details] [diff] [review]
Part 2, v2: DOM API
Comment 61 Boris Zbarsky [:bz] (still a bit busy) 2011-10-18 10:24:50 PDT
I still don't understand the logic.  We want to remove the listener when the vibration is done, not when the document that started the vibration is hidden, no?
Comment 62 Justin Lebar (not reading bugmail) 2011-10-18 10:46:52 PDT
(In reply to Boris Zbarsky (:bz) from comment #61)
> I still don't understand the logic.  We want to remove the listener when the
> vibration is done, not when the document that started the vibration is
> hidden, no?

The problem is that it's kind of tricky to say when the vibration is done.  We can add up all the numbers in the array we pass to hal::vibrate, but there's no guarantee that the device will vibrate for precisely that long.

The reason we want to remove the listener when the vibration is done is because if another document calls vibrate() while I'm still vibrating, we don't want my listener to cancel the other document's vibration.  But if another document calls vibrate() while I'm vibrating, my vibration is immediately canceled.  So then I no longer need to listen for visibility changes.

The code makes sure that there's only one visibilitychange listener and that it corresponds to the document which most recently called vibrate().  I guess it would be sensible for the listener to delete itself after it cancels the vibration once; I'll change this, if the rest looks OK.
Comment 63 Boris Zbarsky [:bz] (still a bit busy) 2011-10-18 10:54:27 PDT
> The reason we want to remove the listener when the vibration is done is because if
> another document calls vibrate() while I'm still vibrating, we don't want my listener to
> cancel the other document's vibration.

No, the reason is that if another document calls vibrate() and then while that vibration is happening this document becomes hidden we will cancel that other document's vibration with the patch as currently attached, no?

> The code makes sure that there's only one visibilitychange listener

I don't see where that's happening.  If that were the case, things would work...
Comment 64 Justin Lebar (not reading bugmail) 2011-10-18 11:07:26 PDT
> No, the reason is that if another document calls vibrate() and then while that vibration is 
> happening this document becomes hidden we will cancel that other document's vibration

This is what I meant, yes.

>> The code makes sure that there's only one visibilitychange listener
> I don't see where that's happening.

> +  // Add a listener to cancel the vibration if the document becomes hidden,
> +  // and remove the old mozvisibility listener, if there was one.
> +  gVibrateWindowListener = new VibrateWindowListener(doc);

The comment should be more explicit.
Comment 65 Boris Zbarsky [:bz] (still a bit busy) 2011-10-18 12:16:25 PDT
> The comment should be more explicit.

My issue is that the comment is _wrong_ as far as I can tell.  Where is the old listener being removed, for example?
Comment 66 Justin Lebar (not reading bugmail) 2011-10-18 12:25:01 PDT
> Where is the old listener being removed?

In its destructor, which gets called when we assign to that global nsRefPtr?
Comment 67 Boris Zbarsky [:bz] (still a bit busy) 2011-10-18 12:40:47 PDT
The event listener manager holds strong refs to event listeners last I checked.  So I would not expect the dtor to run when you reset gVibrateWindowListener.  Have you tested that it actually does?
Comment 68 Justin Lebar (not reading bugmail) 2011-10-18 13:42:35 PDT
> The event listener manager holds strong refs to event listeners last I checked. 

Ah, of course.

I've added a failing testcase to my manual test, and I'll fix the patch.
Comment 69 Justin Lebar (not reading bugmail) 2011-10-18 14:06:04 PDT
Created attachment 567874 [details] [diff] [review]
Part 2, v3: DOM API
Comment 70 Justin Lebar (not reading bugmail) 2011-10-18 14:06:28 PDT
Created attachment 567875 [details]
Manual testcase, v2
Comment 71 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-10-18 16:57:31 PDT
I haven't looked closely at the new patch, but if/how does it avoid the cancellation of a vibrate() call from a tab being in hidden in one content process stomping on the vibrate() in a newly visible tab in another content process?
Comment 72 Justin Lebar (not reading bugmail) 2011-10-18 17:05:56 PDT
(In reply to Chris Jones [:cjones] [:warhammer] from comment #71)
> I haven't looked closely at the new patch, but if/how does it avoid the
> cancellation of a vibrate() call from a tab being in hidden in one content
> process stomping on the vibrate() in a newly visible tab in another content
> process?

It doesn't.  But unless we can run in this configuration now, should we be designing for it now?
Comment 73 Jonas Sicking (:sicking) No longer reading bugmail consistently 2011-10-18 18:23:06 PDT
The plan is to have multi-process firefox desktop as soon as 5 months from now. Seems nice if we didn't have to add this API to the to-do list of things that needs to be fixed before then.

That said, I don't know if we have the infrastructure to send messages to the "chrome process" such that it transparently goes to the same process now, but to the real chrome process once we have one.
Comment 74 Justin Lebar (not reading bugmail) 2011-10-18 18:39:02 PDT
Do we really want to block this feature on getting it to work with an in-development feature on desktop?  This code won't break when you turn on multi-content-process on desktop.

I promise to fix it to make it work as things change, but this seems premature.
Comment 75 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-10-18 20:47:36 PDT
We can run multiple content processes now: dom.ipc.processCount > 1.  (I hope there are tests that do this...)

I haven't been following the latest patches so I don't know why you don't have a similar stomping problem when navigating from a page of say 3 iframes to another page.  But it shouldn't be hard to implement don't-stomp-on-hide; in chrome, track the last window ID that turned on the vibrator.  When canceling vibration from window ID x because of change-to-hidden, check that x == lastVibratorUser.  If that's hard to implement, we can worry about the cost/benefit analysis (like I said, I assumed it would be very easy).
Comment 76 Justin Lebar (not reading bugmail) 2011-10-18 20:55:33 PDT
> We can run multiple content processes now: dom.ipc.processCount > 1.

This works on mobile?
Comment 77 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-10-18 20:58:09 PDT
If it works anywhere.
Comment 78 Justin Lebar (not reading bugmail) 2011-10-19 07:51:15 PDT
Matt, do you know if multiple content processes work on mobile, and if so, how I can force two tabs to load into two different processes?
Comment 79 Justin Lebar (not reading bugmail) 2011-10-19 08:14:38 PDT
It seems to work if you set the pref.  mbrubeck showed me that so long as I open fewer tabs than the max number of content processes, each tab will get its own process.

I'll see what I can do about fixing up the patch...
Comment 80 Justin Lebar (not reading bugmail) 2011-10-20 12:49:05 PDT
AIUI, the window ID is per-content-process, so we can't use that by itself.  We need an identifier per process.  And pid doesn't cut it, because you might reuse a pid.

I'm reading the IPC code to figure out how I can add an ID to each content process, but I'm new to the code.  Pointers would be appreciated!
Comment 81 Justin Lebar (not reading bugmail) 2011-10-20 15:02:39 PDT
Okay, I think I got it...
Comment 82 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-10-20 15:14:25 PDT
Mmm yuck.  That's a general problem that's going to bite elsewhere.  I hoped it had been addressed already.  Landing what's here is high priority, and it's not the best use of your time to head down a rathole fixing things like this that should already have been fixed.  Let's go ahead and move that work to a followup.

FTR: we would want to add a 64-bit counter to content processes (ContentParent.cpp) and send it across just before here (http://mxr.mozilla.org/mozilla-central/source/dom/ipc/ContentParent.cpp#429).  Then we would make |struct WindowId { int64 process; int64 id;}| or something like that.
Comment 83 Justin Lebar (not reading bugmail) 2011-10-20 20:52:31 PDT
I have something which kind of works, but I just realized that it only matters if you can have multiple tabs open at the same time.  If you can have only one tab open at once (and if all the content in a tab is rendered by a single content process), then when you go to switch tabs, you'll clear any active vibrations.

So we won't be able to test this code, short of putting printfs in FallbackHal.cpp and running on desktop...
Comment 84 Justin Lebar (not reading bugmail) 2011-10-20 21:26:26 PDT
I should s/open/active/ above.

Now I get the following linker error:

/usr/bin/ld.gold.real: error: /home/jlebar/code/moz/ff-1/release/toolkit/library/../../hal/Hal.o: requires dynamic R_X86_64_PC32 reloc against 'mozilla::hal_sandbox::Vibrate(nsTArray<unsigned int, nsTArrayDefaultAllocator> const&, unsigned long)' which may overflow at runtime; recompile with -fPIC

I get this even when I add FORCE_USE_PIC = 1 to hal/Makefile.in and verify that the object files under hal are being compiled with -fPIC.  Not sure what this means...
Comment 85 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-10-21 07:27:50 PDT
(In reply to Justin Lebar [:jlebar] from comment #83)
> I have something which kind of works, but I just realized that it only
> matters if you can have multiple tabs open at the same time.  If you can
> have only one tab open at once (and if all the content in a tab is rendered
> by a single content process), then when you go to switch tabs, you'll clear
> any active vibrations.
> 

No, it also matters when switching between tabs.  (And when navigating back/fore, but I didn't look at how you're preventing stomping there.)
Comment 86 Justin Lebar (not reading bugmail) 2011-10-21 07:40:26 PDT
(In reply to Chris Jones [:cjones] [:warhammer] from comment #85)
> No, it also matters when switching between tabs.  (And when navigating
> back/fore, but I didn't look at how you're preventing stomping there.)

Because we don't wait for the visibilitychange event on the tab-to-be-hidden to finish running before showing the new tab?  I believe that.
Comment 87 Justin Lebar (not reading bugmail) 2011-10-21 11:20:04 PDT
Here's a race condition we're not currently handling:

 * You have windows A and B, in different content processes.  A is active.
 * You switch from window A to window B.  Chrome *asynchronously* notifies A that it's no longer active and notifies B that it's now active.
 * B starts vibrating.
 * Before A receives the notification that it's no longer active, it calls vibrate(), canceling B's vibration pattern.
 * A notices that it's now no longer active, and cancels the vibration it just started.

In general, the problem is that two content processes may think that they're both active at the same time.  bz's suggestion is to track visibility in the ContentParent, since that can be updated synchronously, and cancel vibration requests when they reach the chrome process if the window is hidden.
Comment 88 Justin Lebar (not reading bugmail) 2011-10-21 14:30:19 PDT
Created attachment 568777 [details] [diff] [review]
Part 2, v4: DOM API
Comment 89 Justin Lebar (not reading bugmail) 2011-10-21 14:33:12 PDT
Created attachment 568779 [details] [diff] [review]
Part 1, v3: Vibrator support

I think this gets rid of the race conditions.  I haven't figured out how to test it, because on my phone, when I set dom.ipc.processCount > 1, the browser unloads pages when I leave them.

Also, it's super-ugly.
Comment 90 Justin Lebar (not reading bugmail) 2011-10-21 14:47:28 PDT
Created attachment 568784 [details] [diff] [review]
Part 1, v3.1: Vibrator support

Got rid of an extra hunk.
Comment 91 Justin Lebar (not reading bugmail) 2011-10-21 14:49:43 PDT
Created attachment 568786 [details] [diff] [review]
Interdiff part 1 v2 --> v3.1
Comment 92 Justin Lebar (not reading bugmail) 2011-10-21 14:53:19 PDT
Comment on attachment 568786 [details] [diff] [review]
Interdiff part 1 v2 --> v3.1

This interdiff only captures the changes between files in both patches.  But v3.1 also touches files not in v2:

diff <(diffstat -l <(curl https://bug679966.bugzilla.mozilla.org/attachment.cgi?id=567342) | sort) <(diffstat -l vibrator-1 | sort)

> dom/ipc/ContentChild.cpp
> dom/ipc/ContentChild.h
> dom/ipc/ContentParent.cpp
> dom/ipc/ContentParent.h
> dom/ipc/PContent.ipdl
> dom/ipc/TabParent.cpp
> hal/fallback/FallbackHal.cpp
> hal/Hal.h
> hal/sandbox/PHal.ipdl
> hal/sandbox/SandboxHal.cpp
Comment 93 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-10-21 17:04:05 PDT
(In reply to Justin Lebar [:jlebar] from comment #87)
> Here's a race condition we're not currently handling:
> 
>  * You have windows A and B, in different content processes.  A is active.
>  * You switch from window A to window B.  Chrome *asynchronously* notifies A
> that it's no longer active and notifies B that it's now active.
>  * B starts vibrating.
>  * Before A receives the notification that it's no longer active, it calls
> vibrate(), canceling B's vibration pattern.
>  * A notices that it's now no longer active, and cancels the vibration it
> just started.
> 

This is the same problem as having iframes A and B within an active tab, B vibrate(), A stomp B, then A being removed from doc and cancelling.  Right?  I'm OK with that.

The big problem I want to avoid is say A vibrate()ing a short time in onload(), we set a "needToCancel" bit for A, then 10 minutes later the user switches to B and the cancellation for A stomps a vibrate for B.  That's a very long-lived race condition that's relatively easy to fix.  The others that arise when several windows are trying to vibrate at once and/or they're being switched between I don't care quite as much about.  We can do something cleverer if that's an important use case.
Comment 94 Justin Lebar (not reading bugmail) 2011-10-24 08:35:22 PDT
> The big problem I want to avoid is say A vibrate()ing a short time in onload(), we set a 
> "needToCancel" bit for A, then 10 minutes later the user switches to B and the cancellation for A 
> stomps a vibrate for B.  That's a very long-lived race condition that's relatively easy to fix.

Yes, solving just this race would be simpler than the current iteration of patches, although it does involve the trickiness of estimating how long a vibration will last.  (In theory, we can calculate this by adding the values in the vibrate pattern, but in practice, I think we'd have to overestimate, since there's no real guarantee the vibration will take exactly that long.)

But the current patches are free of all known races (?), so maybe we should consider the approach there?
Comment 95 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-10-24 17:28:12 PDT
(In reply to Justin Lebar [:jlebar] from comment #94)
> > The big problem I want to avoid is say A vibrate()ing a short time in onload(), we set a 
> > "needToCancel" bit for A, then 10 minutes later the user switches to B and the cancellation for A 
> > stomps a vibrate for B.  That's a very long-lived race condition that's relatively easy to fix.
> 
> Yes, solving just this race would be simpler than the current iteration of
> patches, although it does involve the trickiness of estimating how long a
> vibration will last.  (In theory, we can calculate this by adding the values
> in the vibrate pattern, but in practice, I think we'd have to overestimate,
> since there's no real guarantee the vibration will take exactly that long.)
> 

No, you don't need to calculate vibration duration for the simplest solution.  The first time a page calls vibrate(), just set a mayBeVibrating bit somewhere, and never unset it.

> But the current patches are free of all known races (?), so maybe we should
> consider the approach there?

I'll get to the reviews as soon as I can, but in the meantime, is there a high-level comment summarizing your approach somewhere?  It'd be great if it's in the patches.  That would help us review faster.
Comment 96 Justin Lebar (not reading bugmail) 2011-10-24 20:09:12 PDT
> No, you don't need to calculate vibration duration for the simplest solution.  The first time a page 
> calls vibrate(), just set a mayBeVibrating bit somewhere, and never unset it.

I'm not sure how this works; I think this discussion would be more productive if we had it in the context of code.

> is there a high-level comment summarizing your approach somewhere? 

At a high level: push as little logic into hal as possible.  Hal only knows about processes, not windows.  Within a process, you make sure that only the active window is vibrating.

When you make a request to hal, it figures out your processID (distinct from your pid).  Hal remembers the processID which started the last vibration.  When a content process proxies a vibration request to the chrome process, chrome checks whether that content process is in the foreground; if it's not, we cancel the request (we have to check in chrome to be race-free).

A non-foreground content process may cancel a vibration (but not start one), so long as it started the last vibration.  When a process goes into the background, it will eventually cancel its outstanding vibrations, but we can't expect it to do so immediately.
Comment 97 Justin Lebar (not reading bugmail) 2011-10-25 11:02:36 PDT
One assumption in this patch is that a tab we're hiding runs its mozvisibilitychange event before a tab we're showing has its visibility status set to shown.  Is this true?
Comment 98 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-10-25 16:14:57 PDT
When bz and I discussed this, he implied that was not the case in general.  The example we discussed was, tab A is visible but has DOM events disabled (say it's in a sync XHR), but then A is hidden and tab B is shown.  Since we can't fire DOM events in A, then there's no way to fire visibilitychange in A before B.  (That example actually makes me wonder whether listening to visibilitychange in C++ is the best way to cancel various hardware-interaction thingies that visible tabs might be doing ...)  There's also the problem of ensuring that visibilitychange fires in all sub-frames of a being-hidden tab before it's fired in the newly-visible one.  Maybe that doesn't complicate things, maybe it does.

If that's true, then a general approach to avoid race stomps is needed.  The implementation I had in mind would just use window IDs (unique across processes) and not care about processes at all.  I think that's a similar approach to what you have, but maybe a simpler impl.
Comment 99 Justin Lebar (not reading bugmail) 2011-10-25 16:56:53 PDT
Sounds like I need to rework this again.  I'll get something new up ASAP.
Comment 100 Curtis Koenig [:curtisk-use curtis.koenig+bzATgmail.com]] 2011-10-25 18:20:14 PDT
marking sec-review-needed, marking myself as secr and sched review for 2011.10.27
Comment 101 Boris Zbarsky [:bz] (still a bit busy) 2011-10-25 18:35:32 PDT
> When bz and I discussed this, he implied that was not the case in general.

And in particular, the UI is the only thing that would guarantee that.

As far as sync XHR, we don't suppress visibility change events for it now, but we might in the future....
Comment 102 Olli Pettay [:smaug] (reviewing overload) 2011-10-26 03:38:53 PDT
(In reply to Chris Jones [:cjones] [:warhammer] from comment #98)
> When bz and I discussed this, he implied that was not the case in general. 
> The example we discussed was, tab A is visible but has DOM events disabled
There is no such thing as "DOM events disabled".
User initiated event handling is suppressed in certain cases.

But, as bz indicated, we may want to change that so that all event handling is suppressed
(if someone figures out how to do that).
Comment 103 Curtis Koenig [:curtisk-use curtis.koenig+bzATgmail.com]] 2011-10-27 10:21:39 PDT
sec-review-complete: https://wiki.mozilla.org/Security/Firefox/Fennec/WebVibrator
Comment 104 Justin Lebar (not reading bugmail) 2011-10-27 13:07:09 PDT
> But, as bz indicated, we may want to change that so that all event handling is suppressed
> (if someone figures out how to do that).

So I can rely on visibilitychange being fired soon-ish, if not in the right order?  Or do we want to use some other mechanism to detect a window blurring?
Comment 105 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-10-27 13:24:34 PDT
bz told me to use visibility events, so I would keep going down that path until there's a better idea floated (which we should do in a followup).
Comment 106 Justin Lebar (not reading bugmail) 2011-10-28 09:39:56 PDT
Created attachment 570288 [details] [diff] [review]
Part 1, v4
Comment 107 Justin Lebar (not reading bugmail) 2011-10-28 09:40:59 PDT
Created attachment 570289 [details] [diff] [review]
Part 2, v5
Comment 108 Justin Lebar (not reading bugmail) 2011-10-28 09:43:25 PDT
Comment on attachment 570288 [details] [diff] [review]
Part 1, v4

I pushed the visibility monitoring down into Hal.  I figure it's not out of place there, and it's likely we'll want to do visibility monitoring for other things in hal, like the screen lock.

That part will probably need a content person's review.
Comment 109 Justin Lebar (not reading bugmail) 2011-10-28 10:09:34 PDT
Comment on attachment 570288 [details] [diff] [review]
Part 1, v4

Hm, this actually doesn't work just right.  There was a bug in my manual test.
Comment 110 Justin Lebar (not reading bugmail) 2011-10-28 10:57:21 PDT
One problem I just discovered is that, although we'll cancel any outstanding vibrations when the device's screen is turned off, a document can start a new vibration even after the screen is turned off.

This appears to mean that the document's MozHiddden state becomes true when the display turns off, and then becomes false again soon thereafter!

I'll keep poking around.
Comment 111 Justin Lebar (not reading bugmail) 2011-10-28 11:03:35 PDT
Created attachment 570307 [details] [diff] [review]
Part 1, v4.1
Comment 112 Justin Lebar (not reading bugmail) 2011-10-28 11:04:43 PDT
Created attachment 570309 [details] [diff] [review]
Part 2, v5.1

Call hal::VibrateWithAutoCancel instead of hal::Vibrate
Comment 113 Justin Lebar (not reading bugmail) 2011-10-28 11:05:05 PDT
Comment on attachment 570307 [details] [diff] [review]
Part 1, v4.1

One-line change from v4, fixes cancel-on-hide.
Comment 114 Justin Lebar (not reading bugmail) 2011-10-28 11:29:46 PDT
Okay.  It looks like Android itself is canceling the vibration when I turn off the screen.  The document is never marked as hidden.  I'll file a separate bug.
Comment 115 Justin Lebar (not reading bugmail) 2011-10-28 11:35:07 PDT
Filed bug 698057 on the issue with turning off the screen.  Marking these patches for reviews; when that bug is fixed, this code should Just Work.
Comment 116 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-10-28 19:52:19 PDT
Comment on attachment 570307 [details] [diff] [review]
Part 1, v4.1

New patch forthcoming per discussion.
Comment 117 Justin Lebar (not reading bugmail) 2011-10-28 19:54:56 PDT
cjones and I talked about this on IRC.

He thinks it's important to maintain the same API for hal, hal_sandbox, and hal_impl.  So we're going to pass around a struct containing the parameters (window*, id, tabparent) to all the functions.  In hal_impl, this struct will be ignored.  (In fact, I'm going to pass empty data to hal_impl so it's not tempted to read it.)  The struct will have an automatic conversion from nsIDOMWindow*, so nsGlobalWindow can call hal just as it does currently, by passing a pattern and a window.

This means that the flow for a call coming from the sandbox will be

  hal -> hal_sandbox (content process) -> hal_sandbox (chrome process) -> hal (chrome process) -> hal_impl.

We're going to move the vibration-cancellation code out of hal and somewhere else.  I think I'm just going to move it back into nsGlobalWindow; when we need it for something else, we can generalize it.

We're going to make PHal managed by PContent, rather than by PBrowser.  This is important for the battery patches.
Comment 118 Justin Lebar (not reading bugmail) 2011-10-28 19:57:47 PDT
> The struct will have an automatic conversion from nsIDOMWindow*, so nsGlobalWindow can call hal just 
> as it does currently, by passing a pattern and a window.

bz, most of the forthcoming changes are to part 1, but things are moving around too much for me to be comfortable leaving the r? on part 2.  The DOM stuff is pretty small; let's wait until I get an r+ from Chris.
Comment 119 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-10-28 20:01:50 PDT
(In reply to Justin Lebar [:jlebar] from comment #117)
> cjones and I talked about this on IRC.
> 
> He thinks it's important to maintain the same API for hal, hal_sandbox, and
> hal_impl.  So we're going to pass around a struct containing the parameters
> (window*, id, tabparent) to all the functions.

We just need (window*, id) AFAICT.  hal_sandbox gets a TabParent automatically when a content process sends it an IPDL message with a TabChild argument.  But I may be missing something.
Comment 120 Mounir Lamouri (:mounir) 2011-10-29 12:05:38 PDT
(In reply to Robin Berjon from comment #30)
> I don't think that an invisible SMS app is a use case for direct vibrator
> access. SMS apps (and others similar) should really not make the decision as
> to whether they should use sound, light, dialogs, beeps, vibration, or
> whatever else to inform the user. They should trigger a notification, and
> it's up to the notification system to know how to inform the user and serves
> as the single entry point for the user to configure this — otherwise you
> just get a completely unusable mess!

How would that work? Is the Notification API able to have that kind of information added?
Comment 121 Justin Lebar (not reading bugmail) 2011-10-29 12:37:40 PDT
(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #120)
> (In reply to Robin Berjon from comment #30)
> > I don't think that an invisible SMS app is a use case for direct vibrator
> > access. SMS apps (and others similar) should really not make the decision as
> > to whether they should use sound, light, dialogs, beeps, vibration, or
> > whatever else to inform the user. They should trigger a notification, and
> > it's up to the notification system to know how to inform the user and serves
> > as the single entry point for the user to configure this — otherwise you
> > just get a completely unusable mess!
> 
> How would that work? Is the Notification API able to have that kind of
> information added?

Can we discuss this elsewhere, please?  :)
Comment 122 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-10-30 16:54:07 PDT
Yes, let's discuss elsewhere, and no, the current Notifications API isn't enough.
Comment 123 Justin Lebar (not reading bugmail) 2011-11-01 15:19:53 PDT
Created attachment 571159 [details] [diff] [review]
Part 1, v5
Comment 124 Justin Lebar (not reading bugmail) 2011-11-01 15:21:35 PDT
Created attachment 571161 [details] [diff] [review]
Part 2, v6

I'll hold off asking for review on this until we get part 1 in an agreeable state.
Comment 125 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-11-01 16:34:12 PDT
Comment on attachment 571159 [details] [diff] [review]
Part 1, v5

>diff --git a/content/base/src/Makefile.in b/content/base/src/Makefile.in

>-		$(NULL)
>+		nsFrameMessageManager.h \
>+		$(NULL)
> 

Does this need to be exported?

>diff --git a/dom/ipc/ContentParent.cpp b/dom/ipc/ContentParent.cpp

>     Open(mSubprocess->GetChannel(), mSubprocess->GetChildProcessHandle());
>+    SendSetID(gContentChildID++);
> 

unused << SendSetID(...) or you'll get a gcc warning.

>diff --git a/dom/ipc/PContent.ipdl b/dom/ipc/PContent.ipdl
 
>+    SetID(PRUint64 id);
>+

The content-process ID needs to be documented somewhere.
ContentChild.h:mId is probably the best place.  Please add a note here
about when this message is expected to be sent.

>diff --git a/dom/ipc/TabParent.h b/dom/ipc/TabParent.h

>+    bool Active();

Please document at least Active().

>diff --git a/hal/Hal.cpp b/hal/Hal.cpp

>+using mozilla::dom::ContentChild;
>+using mozilla::dom::TabChild;
>+using mozilla::dom::PBrowserChild;
> 

using namespace mozilla::dom;

>+nsTArray<uint64> *gLastIDToVibrate;
>+

When I see nsTArray<uint64>, I don't immediately think of an ID.  Can
we make this a typedef or struct and define how the ID is interpreted?

Please use nsAutoPtr.

>+NS_IMPL_ISUPPORTS1(ShutdownObserver, nsIObserver);
>+
>+void InitLastIDToVibrate()
>+{
>+  MOZ_ASSERT(!gLastIDToVibrate);
>+  gLastIDToVibrate = new nsTArray<uint64>();
>+
>+  nsCOMPtr<nsIObserverService> observerService =
>+    mozilla::services::GetObserverService();

|using namespace mozilla::services;| above, plz.

>+TabChild*
>+WindowIdentifier::GetTabChild() const
>+{
>+  MOZ_ASSERT(!mIsEmpty);
>+  nsCOMPtr<nsIWebNavigation> webnav = do_GetInterface(mWindow);
>+  nsCOMPtr<nsIDocShell> docshell = do_QueryInterface(webnav);
>+  nsCOMPtr<nsITabChild> iTabChild = do_GetInterface(docshell);
>+  nsRefPtr<TabChild> tabChild = static_cast<TabChild*>(iTabChild.get());
>+  return tabChild.get();
>+}
>+

Do you mind adding this as another helper in TabChild.h?  See the
|inline TabChild* GetTabChildFrom(...)| code near the bottom of the
file.  I'd like to keep all this goop centralized instead of littering
across the codebase.  You should be able to go Window->WebNav then
reuse the docshell helper.

>+void
>+Vibrate(const nsTArray<uint32>& pattern, const WindowIdentifier &id)

>+    // hal_impl doesn't need |id|. Send it an empty id, so it's not tempted to
>+    // use it.
>+    WindowIdentifier emptyID;
>+    hal_impl::Vibrate(pattern, emptyID);

|hal_impl::Vibrate(pattern, WindowIdentifier())| works too.  Your call.

>diff --git a/hal/Hal.h b/hal/Hal.h

>+  WindowIdentifier(nsIDOMWindow* window);
>+  WindowIdentifier(nsCOMPtr<nsIDOMWindow> &window);
>+

I'm not sure why we need the second ctor.

>+  bool mIsEmpty;

Can mIsEmpty be determined from the other fields?

>diff --git a/hal/sandbox/SandboxHal.cpp b/hal/sandbox/SandboxHal.cpp

> void
>+Vibrate(const nsTArray<uint32>& pattern, const WindowIdentifier &id)
> {

Just to be sure: the DOM code does a visibility check before calling
the hal:: API, right?

>diff --git a/widget/src/android/AndroidBridge.cpp b/widget/src/android/AndroidBridge.cpp

>+    // XXX not really clear if this worth special caseing, but creates

casing

>+    mJNIEnv->CallStaticVoidMethod(mGeckoAppShellClass, jVibrateA,
>+                                  array, -1/*don't repeat*/);
>+    // GC owns |array| now?

I'm pretty sure this is the case.  Let's find out for sure before landing :).

This is mostly nit-y stuff except for the JNI array ownership
question.  r=me with that answered and nits picked.
Comment 126 Justin Lebar (not reading bugmail) 2011-11-01 16:51:53 PDT
> >diff --git a/content/base/src/Makefile.in b/content/base/src/Makefile.in
> 
> >-		$(NULL)
> >+		nsFrameMessageManager.h \
> >+		$(NULL)
> > 
> 
> Does this need to be exported?

Otherwise I can't include a header I need (TabParent.h, I think).

> >+nsTArray<uint64> *gLastIDToVibrate;
> >+
> 
> When I see nsTArray<uint64>, I don't immediately think of an ID.  Can
> we make this a typedef or struct and define how the ID is interpreted?

Will C++ be happy if I make the signature 

I can do that, but I eventually need to pass this into an IPDL-defined
function, which takes an nsTArray<uint64>.  So it's kind of lame either way.

> >+  WindowIdentifier(nsIDOMWindow* window);
> >+  WindowIdentifier(nsCOMPtr<nsIDOMWindow> &window);
> >+
> 
> I'm not sure why we need the second ctor.

DOM calls it and passes an nsCOMPtr<nsIDOMWindow>.  If we don't have the
explicit COMPtr constructor, you'd have to call .get() on the COMPtr, which
would break the illusion that the function actually takes an nsIDOMWindow*.

> 
> >+  bool mIsEmpty;
> 
> Can mIsEmpty be determined from the other fields?
> 
> >diff --git a/hal/sandbox/SandboxHal.cpp b/hal/sandbox/SandboxHal.cpp
> 
> > void
> >+Vibrate(const nsTArray<uint32>& pattern, const WindowIdentifier &id)
> > {
> 
> Just to be sure: the DOM code does a visibility check before calling
> the hal:: API, right?

Yes, but sicking tells me that we need to do it here again, or at least, we
need to check after we've read the values out of the vibrate pattern JS array,
since reading those values may run script.

> 
> >diff --git a/widget/src/android/AndroidBridge.cpp b/widget/src/android/AndroidBridge.cpp
> 
> >+    // XXX not really clear if this worth special caseing, but creates
> 
> casing

It should be "special-casing", I think.  :)
Comment 127 Justin Lebar (not reading bugmail) 2011-11-01 16:57:53 PDT
> Will C++ be happy if I make the signature 
>
> I can do that, but I eventually need to pass this into an IPDL-defined
> function, which takes an nsTArray<uint64>.  So it's kind of lame either way.

er, didn't finish editing that.

I meant to wonder if C++ will appreciate taking a function declared as

  RecvVibrate(const nsTArray<uint64> &id)

and defining it as

  typedef WindowIDArray nsTArray<uint64>;
  RecvVibrate(const WindowIDArray &id) { ... }
Comment 128 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-11-01 17:03:06 PDT
(In reply to Justin Lebar [:jlebar] from comment #126)
> > >+nsTArray<uint64> *gLastIDToVibrate;
> > >+
> > 
> > When I see nsTArray<uint64>, I don't immediately think of an ID.  Can
> > we make this a typedef or struct and define how the ID is interpreted?
> 
> Will C++ be happy if I make the signature 
> 
> I can do that, but I eventually need to pass this into an IPDL-defined
> function, which takes an nsTArray<uint64>.  So it's kind of lame either way.
> 

Typedef is fine.

> > > void
> > >+Vibrate(const nsTArray<uint32>& pattern, const WindowIdentifier &id)
> > > {
> > 
> > Just to be sure: the DOM code does a visibility check before calling
> > the hal:: API, right?
> 
> Yes, but sicking tells me that we need to do it here again, or at least, we
> need to check after we've read the values out of the vibrate pattern JS
> array,
> since reading those values may run script.
> 

Sigh, JS.  The check in the leaf content process isn't critically important, it's just a perf benefit.  Moving the check in DOM to after reading the values seems OK.

> > 
> > >diff --git a/widget/src/android/AndroidBridge.cpp b/widget/src/android/AndroidBridge.cpp
> > 
> > >+    // XXX not really clear if this worth special caseing, but creates
> > 
> > casing
> 
> It should be "special-casing", I think.  :)

Yes, that's right.
Comment 129 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-11-01 17:05:07 PDT
(In reply to Justin Lebar [:jlebar] from comment #127)
> > Will C++ be happy if I make the signature 
> >
> > I can do that, but I eventually need to pass this into an IPDL-defined
> > function, which takes an nsTArray<uint64>.  So it's kind of lame either way.
> 
> er, didn't finish editing that.
> 
> I meant to wonder if C++ will appreciate taking a function declared as
> 
>   RecvVibrate(const nsTArray<uint64> &id)
> 
> and defining it as
> 
>   typedef WindowIDArray nsTArray<uint64>;
>   RecvVibrate(const WindowIDArray &id) { ... }

That's perfectly fine, except the order of the types in your typedef needs to be the other way around.  "typdef" makes the defined type a synonym to the source type in every respect (modulo some C++ syntactic corner cases), so you can use it anywhere you could use the source type.
Comment 130 Justin Lebar (not reading bugmail) 2011-11-01 17:31:12 PDT
> >+  bool mIsEmpty;
> 
> Can mIsEmpty be determined from the other fields?

Not really.  You'd have to assert(mWindow), but that can legitimately be null.  (At the very least, a caller could pass a null window to hal.)
Comment 131 Justin Lebar (not reading bugmail) 2011-11-01 21:00:06 PDT
> The check in the leaf content process isn't critically important, it's just a perf benefit.  Moving 
> the check in DOM to after reading the values seems OK.

Since hal checks that the tab is active, I think it makes sense for hal to also check that the window is active.  (I'm not sure if you mean that the window visibility check is redundant with the tab visibility check.  I think you can have an inactive window in an active tab, so both checks are necessary.)
Comment 132 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-11-01 21:03:51 PDT
I think you want

  if (!id.HasTraveledThroughIPC() && !WindowIsActive(id.GetWindow())) {
     ^^^

then.
Comment 133 Justin Lebar (not reading bugmail) 2011-11-01 21:14:05 PDT
(In reply to Chris Jones [:cjones] [:warhammer] from comment #132)
> I think you want
> 
>   if (!id.HasTraveledThroughIPC() && !WindowIsActive(id.GetWindow())) {
>      ^^^

Yes!!  Thanks.
Comment 134 Justin Lebar (not reading bugmail) 2011-11-02 07:58:59 PDT
> This is mostly nit-y stuff except for the JNI array ownership
> question.  r=me with that answered and nits picked.

I spent a while looking through JNI docs.  It's not entirely clear to me what's going on, but afaict, this jLongArray type is rooting, so we don't need to explicitly free the array at the end of the method.  In fact, I don't see a method *to* free the array.

So I suspect the code is right, but maybe we should get someone who actually knows JNI to look at it.
Comment 135 Justin Lebar (not reading bugmail) 2011-11-02 08:18:30 PDT
Comment on attachment 571159 [details] [diff] [review]
Part 1, v5

Brad, would you mind double-checking that AndroidBridge::Vibrate is right?  The GC owns the array at the end of the function -- we don't need to have a AutoLocalJNIFrame or other magic?
Comment 136 Brad Lassey [:blassey] (use needinfo?) 2011-11-03 13:41:36 PDT
Comment on attachment 571159 [details] [diff] [review]
Part 1, v5

Review of attachment 571159 [details] [diff] [review]:
-----------------------------------------------------------------

bridge code looks good
Comment 137 Boris Zbarsky [:bz] (still a bit busy) 2011-11-07 13:00:28 PST
I'm not going to get to this review today....
Comment 138 Justin Lebar (not reading bugmail) 2011-11-07 18:14:15 PST
Checked part 1 in.  Since part 2 isn't in yet, this bug shouldn't be closed when it's merged to m-c.

https://hg.mozilla.org/integration/mozilla-inbound/rev/ebb4fe2d4c83
Comment 139 Ed Morley [:emorley] 2011-11-08 01:11:37 PST
Part 1:
https://hg.mozilla.org/mozilla-central/rev/ebb4fe2d4c83

(Leaving open per comment 138)
Comment 140 Mounir Lamouri (:mounir) 2011-11-08 02:33:55 PST
Justin, with part 1 pushed and in Gecko 10, we are going to ask for VIBRATE permissions on Android but we are not going to expose an API to use it which means we are going to ask for the permissions for nothing (which is not a good idea in general).
I believe it might be better to whether push part 2 to Gecko 10 (in Aurora then) or to not ask for this permission in Gecko 10 (by backing out or just removing the line in the Manifest).
Comment 141 Ed Morley [:emorley] 2011-11-08 02:37:51 PST
Requesting tracking Firefox 10 due to comment 140.
Comment 142 Justin Lebar (not reading bugmail) 2011-11-08 06:28:20 PST
> Justin, with part 1 pushed and in Gecko 10, we are going to ask for VIBRATE permissions on Android 
> but we are not going to expose an API to use it which means we are going to ask for the permissions 
> for nothing (which is not a good idea in general).

Thanks for catching this, Mounir.  Do you know if "vibrate" will be one of the features that Android prompts about?  Android doesn't list every permission we ask for...

I guess we should remove the permission from the manifest.
Comment 143 Justin Lebar (not reading bugmail) 2011-11-08 06:59:16 PST
Mounir's suggestion, which I agree with, is to back out part 1 from aurora.  This way, we won't end up asking vibrator permission in there.  (Vibrator permission comes up when you install Fennec, but you have to open up the "other permissions" list to see it.)
Comment 144 Justin Lebar (not reading bugmail) 2011-11-08 19:43:44 PST
Created attachment 573065 [details] [diff] [review]
Empty placeholder patch - back out part 1 from Aurora (see comment 143)
Comment 145 Justin Lebar (not reading bugmail) 2011-11-09 10:25:02 PST
Created attachment 573245 [details]
Manual testcase, v3
Comment 146 Justin Lebar (not reading bugmail) 2011-11-09 13:04:03 PST
Created attachment 573296 [details] [diff] [review]
Part 2, v7

Unbitrotting.
Comment 147 Justin Lebar (not reading bugmail) 2011-11-09 13:38:13 PST
If you want to play around with it, a build is at http://people.mozilla.org/~jlebar/fennec-vibrator.apk
Comment 148 Martijn Wargers [:mwargers] (not working for Mozilla) 2011-11-09 14:13:49 PST
A long vibrator seems to kill that Fennec instance from comment 147:
http://people.mozilla.org/~mwargers/tests/vibrator_long.htm
Comment 149 Justin Lebar (not reading bugmail) 2011-11-09 14:21:38 PST
Awesome.

W/dalvikvm(16350): ReferenceTable overflow (max=512)
W/dalvikvm(16350): Last 10 entries in JNI local reference table:
W/dalvikvm(16350):   502: 0x40612028 cls=[J (820 bytes)
W/dalvikvm(16350):   503: 0x40612360 cls=[J (820 bytes)
W/dalvikvm(16350):   504: 0x40612698 cls=[J (820 bytes)
W/dalvikvm(16350):   505: 0x406129d0 cls=[J (820 bytes)
W/dalvikvm(16350):   506: 0x40612d08 cls=[J (820 bytes)
W/dalvikvm(16350):   507: 0x40613040 cls=[J (820 bytes)
W/dalvikvm(16350):   508: 0x40613378 cls=[J (820 bytes)
W/dalvikvm(16350):   509: 0x406136b0 cls=[J (820 bytes)
W/dalvikvm(16350):   510: 0x406139e8 cls=[J (820 bytes)
W/dalvikvm(16350):   511: 0x40613d20 cls=[J (820 bytes)
W/dalvikvm(16350): JNI local reference table summary (512 entries):
W/dalvikvm(16350):   511 of [J 820B (511 unique)
W/dalvikvm(16350):     1 of Landroid/view/Surface; 44B
W/dalvikvm(16350): Memory held directly by tracked refs is 419064 bytes
E/dalvikvm(16350): Failed adding to JNI local ref table (has 512 entries)
I/dalvikvm(16350): "Thread-12" prio=5 tid=11 RUNNABLE
I/dalvikvm(16350):   | group="main" sCount=0 dsCount=0 obj=0x4059ff88 self=0x2a4dd0
I/dalvikvm(16350):   | sysTid=16360 nice=0 sched=0/0 cgrp=default handle=2745880
I/dalvikvm(16350):   at org.mozilla.gecko.GeckoAppShell.nativeRun(Native Method)
I/dalvikvm(16350):   at org.mozilla.gecko.GeckoAppShell.runGecko(GeckoAppShell.java:411)
I/dalvikvm(16350):   at org.mozilla.gecko.GeckoApp$2.run(GeckoApp.java:329)
I/dalvikvm(16350): 
E/dalvikvm(16350): VM aborting
I/DEBUG   (16326): debuggerd committing suicide to free the zombie!
I/DEBUG   (16902): debuggerd: Sep 20 2011 11:11:25
Comment 150 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-11-09 15:57:48 PST
Um.  JNI fail?
Comment 151 Justin Lebar (not reading bugmail) 2011-11-09 16:51:32 PST
"cls=[J" means we're leaking an array of longs, I think [1].  It appears that this would happen if we called GetLongArrayElements but didn't have a coresponding ReleaseLongArrayElements [2].  But I don't see how that could happen in the patch, so I'm not sure what's going on.

[1] http://java.sun.com/docs/books/jni/html/types.html
[2] http://stackoverflow.com/questions/4959495/jni-reference-table-overflow
Comment 152 Justin Lebar (not reading bugmail) 2011-11-10 13:25:40 PST
I think the array it's mentioning in the reference table is the array we're passing to vibrate():

In comment 149, the array has length 99 and size 820 bytes.  In my testing, passing an array of length 10 also causes an error, with size 108 bytes.

820 = 99 * 8 + [overhead]
108 = 10 * 8 + [overhead]

gives overhead in both cases to be 28 bytes.

I'm still not sure what's going on.  One possibility is that Android is hanging on to the pattern until the pattern expires.  But because we created the array in JNI, we're getting blamed for this.
Comment 153 Justin Lebar (not reading bugmail) 2011-11-10 13:51:04 PST
The following loop kills the process right quick:

while(true) {
  navigator.mozVibrate([100, 100]);
}
Comment 154 Justin Lebar (not reading bugmail) 2011-11-11 08:07:47 PST
Created attachment 573814 [details] [diff] [review]
Part 2, v8

Adding test for bug 701716
Comment 155 christian 2011-11-15 14:34:03 PST
Comment on attachment 573065 [details] [diff] [review]
Empty placeholder patch - back out part 1 from Aurora (see comment 143)

[triage comment]
Approved for aurora. Please back out when you can and set status-firefox10 unaffected when you do.
Comment 156 Justin Lebar (not reading bugmail) 2011-11-16 18:30:37 PST
Thanks, Christian.

https://hg.mozilla.org/releases/mozilla-aurora/rev/e1190dc6f7da
Comment 157 Justin Lebar (not reading bugmail) 2011-11-30 13:49:21 PST
Comment on attachment 573814 [details] [diff] [review]
Part 2, v8

bz, do you have an ETA for reviewing this patch?  I can ask someone else, if you're swamped, but you've already looked at it once.
Comment 158 Boris Zbarsky [:bz] (still a bit busy) 2011-11-30 14:03:18 PST
I can probably manage it by Friday or Monday...  Seem ok?
Comment 159 Justin Lebar (not reading bugmail) 2011-11-30 14:04:54 PST
Yes.  Thanks!
Comment 160 Justin Lebar (not reading bugmail) 2011-12-01 12:07:37 PST
The w3c discussion seems to have settled on disallowing vibrate() and vibrate(null).  I'll modify the patch and test to reflect this.
Comment 161 Justin Lebar (not reading bugmail) 2011-12-02 10:55:39 PST
Created attachment 578644 [details] [diff] [review]
Part 2, v9

Updated to the latest version of the spec.

I found a bug in AndroidHal; Chris, would you mind reviewing the change there?
Comment 162 Justin Lebar (not reading bugmail) 2011-12-02 10:55:57 PST
Created attachment 578645 [details]
Manual testcase, v4
Comment 163 Justin Lebar (not reading bugmail) 2011-12-02 10:57:11 PST
Comment on attachment 578644 [details] [diff] [review]
Part 2, v9

Note that this depends on bug 706958.
Comment 164 Boris Zbarsky [:bz] (still a bit busy) 2011-12-06 21:58:16 PST
Comment on attachment 578644 [details] [diff] [review]
Part 2, v9

>+++ b/dom/base/Navigator.cpp
>+  VibrateWindowListener(nsIDOMWindow *aWindow)
>+    nsCOMPtr<nsIEventListenerService> els =
>+      do_GetService("@mozilla.org/eventlistenerservice;1");

Why?  You can just call nsIDOMEventTarget::AddSystemEventListener.  It's noscript, so JS code has to jump through the event listener service hoop, but you're not in JS.

>+already_AddRefed<nsIDOMEventTarget>
>+VibrateWindowListener::GetEventTarget()
>+{
>+  nsCOMPtr<nsIDOMWindow> window = do_QueryReferent(mWindow);
>+  nsCOMPtr<nsIWebNavigation> webNav = do_GetInterface(window);
>+  nsCOMPtr<nsIDocument> doc = do_GetInterface(webNav);
>+  nsCOMPtr<nsIDOMEventTarget> target = do_QueryInterface(doc);
>+  return target.forget();

This is fragile at best, because the document for an nsIWebNavigation is not invariant.

Why not just store a weak reference to the document you added yourself as an event target to directly instead?

>+VibrateWindowListener::HandleEvent(nsIDOMEvent* aEvent)

When the document goes hidden, should this listener remove itself?

>+    hal::CancelVibrate(window);

I assume CancelVibrate is null-safe?

>+VibrateWindowListener::RemoveListener()
>+  nsCOMPtr<nsIEventListenerService> els =

Again, no need for that.

>+  nsCOMPtr<nsIDocument> doc = do_QueryInterface(win->GetExtantDocument());
>+  nsCOMPtr<nsIDOMDocument> domDoc = do_QueryInterface(doc);

GetExtantDocument returns nsIDOMDocument, so you don't need that second QI.  Does domDoc still need to be a strong ref at that point?

And |doc| is unused, a far as I can tell.  Just remove it.

>+      if (JS_GetElement(cx, obj, i, &v) && JS_ValueToInt32(cx, v, &pv) &&
>+          pv >= 0 && static_cast<PRUint32>(pv) <= sMaxVibrateMS) {

Might be worth factoring out those last three checks into a ValueOK helper that's called both here and for the primitive case.

>+  nsCOMPtr<nsIDOMWindow> domWindow = do_QueryInterface(win);

nsPIDOMWindow is a subclass of nsIDOMWindow, so you don't need this QI.

>+++ b/dom/interfaces/base/nsIDOMNavigator.idl
>+   * - 0, the empty list, or a list containing entirely 0s, we cancel any
>+   *   outstanding vibration pattern; that is, we stop the device from
>+   *   vibrating.

Except if the window doesn't have focus?  Or is the comment in VibrateWindowListener::HandleEvent about focus out of date?

>+   *   converting it to a number and then rounding.  If we cannot convert any
>+   *   element to an integer, or if any of the integers are negative, we throw
>+   *   NS_ERROR_DOM_NOT_SUPPORTED_ERR.

"If there is some element that cannot be converted to an integer" is clearer, I think.

r=me with the above nits fixed.
Comment 165 Justin Lebar (not reading bugmail) 2011-12-07 01:59:48 PST
Thanks for the review, Boris.

>+      if (JS_GetElement(cx, obj, i, &v) && JS_ValueToInt32(cx, v, &pv) &&
>+          pv >= 0 && static_cast<PRUint32>(pv) <= sMaxVibrateMS) {

> Might be worth factoring out those last three checks into a ValueOK helper that's called both here and for the 
> primitive case.

I just tried, but I think it's messier, because you have to check for validity twice (that JS_GetElement succeeds, and then that converting the element to an int succeeds).

>>+  nsCOMPtr<nsIDOMWindow> domWindow = do_QueryInterface(win);
> nsPIDOMWindow is a subclass of nsIDOMWindow, so you don't need this QI.

Unfortunately I need an nsIDOMWindow*.  hal::Vibrate appears to take an nsIDOMWindow*, but it actually takes a hal::WindowIdentifier, which has an implicit cast from nsIDOMWindow*.  Yes, it's awful.

I put in a static cast.
Comment 166 Boris Zbarsky [:bz] (still a bit busy) 2011-12-07 02:05:23 PST
> because you have to check for validity twice

I was thinking something like:

  if (JS_GetElement(cx, obj, i, &v) && ValueOK(cx, v, &pv)) {

where ValueOK would call JS_ValueToInt32 and then check >= 0 and <= sMaxVibrateMS.
Comment 167 Justin Lebar (not reading bugmail) 2011-12-07 02:16:38 PST
That's cleaner.  Thanks.
Comment 168 Justin Lebar (not reading bugmail) 2011-12-07 20:35:52 PST
I had to make the pref caches static to the file, rather than static to Navigator, so that the anonymous function could access sMaxVibrateMS.  Hope that doesn't bother anyone.
Comment 169 Justin Lebar (not reading bugmail) 2011-12-15 10:32:57 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/44777be25060
Comment 170 Ed Morley [:emorley] 2011-12-16 06:16:51 PST
https://hg.mozilla.org/mozilla-central/rev/44777be25060
Comment 171 Eric Shepherd [:sheppy] 2012-03-24 09:12:29 PDT
Listed on:

https://developer.mozilla.org/en/DOM/window.navigator

Documented:

https://developer.mozilla.org/en/DOM/window.navigator.mozVibrate

Mentioned on Firefox 11 for developers.

Note You need to log in before you can comment on or make changes to this bug.