Closed Bug 679966 Opened 13 years ago Closed 12 years ago

WebVibrator

Categories

(Core :: DOM: Core & HTML, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla11
Tracking Status
firefox10 + unaffected

People

(Reporter: cjones, Assigned: justin.lebar+bug)

References

()

Details

(Keywords: dev-doc-complete, Whiteboard: [secr:curtisk])

Attachments

(4 files, 23 obsolete files)

39.29 KB, patch
cjones
: review+
blassey
: review+
justin.lebar+bug
: checkin+
Details | Diff | Splinter Review
49 bytes, patch
Details | Diff | Splinter Review
16.71 KB, patch
bzbarsky
: review+
cjones
: review+
Details | Diff | Splinter Review
4.02 KB, text/html
Details
(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.
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.
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?
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.
(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.
Assignee: nobody → gal
(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.
we have the plumbing for haptic feedback in chrome code.

also see:  https://bugzilla.mozilla.org/show_bug.cgi?id=518266#c10
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?
cjones, probably.  being able to provide a pattern like comment #1 would fucking rock.
Attached patch patch, incomplete (obsolete) — Splinter Review
Attached patch pong (obsolete) — Splinter Review
- 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
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.
Why are you guys injecting new APIs into navigator? We need a more principled and extensible approach.

/be
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.
Seconded.  This is not something I want to even have to think about.
Someone here had better think about two things, and think hard:

1. Developer ergonomics, usability.

2. Security, including DOS/annoyance controls.

/be
(2) reminds me that we should cancel vibration when the user leaves the site.
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?
> 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
Are any other engines/browsers signed on to ES6 modules yet?
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?
> 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
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.
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.
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
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.
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.
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.
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.
Whiteboard: [html11]
Attached patch functional WIP (obsolete) — Splinter Review
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.
Attachment #554000 - Attachment is obsolete: true
Attachment #554010 - Attachment is obsolete: true
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.
Assignee: gal → jones.chris.g
Attachment #563345 - Attachment is obsolete: true
Attachment #563673 - Flags: review?(blassey.bugs)
Attachment #563673 - Flags: review?(blassey.bugs) → review+
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.
> 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.
(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?
> 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.
(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 ;).
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.)
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.
Assignee: jones.chris.g → justin.lebar+bug
Whiteboard: [html11]
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?
jlebar, correct, and that code needs to be investigated, look at the source in glue/gonk
(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.
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.
Thanks for the heads-up.
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...
Attached patch Part 2, v1: DOM API (obsolete) — Splinter Review
Attachment #567341 - Flags: review?(bzbarsky)
Attachment #563673 - Attachment is obsolete: true
Attachment #563674 - Attachment is obsolete: true
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.
Attachment #567342 - Flags: review?(blassey.bugs)
Attached file Manual testcase (obsolete) —
I'm going to get this manual testcase added to Litmus.
Blocks: 694862
(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.
Attachment #567342 - Flags: review?(blassey.bugs) → review+
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.
Attachment #567341 - Flags: review?(bzbarsky) → review-
> 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.
(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.
> 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.
> 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?
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?
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.
I believe that's bogus on mobile with iframes, yeah....
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.
Attached patch Part 2, v2: DOM API (obsolete) — Splinter Review
Attachment #567785 - Flags: review?(bzbarsky)
Attachment #567341 - Attachment is obsolete: true
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?
(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.
> 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...
> 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.
> 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?
> Where is the old listener being removed?

In its destructor, which gets called when we assign to that global nsRefPtr?
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?
> 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.
Attached patch Part 2, v3: DOM API (obsolete) — Splinter Review
Attachment #567874 - Flags: review?(bzbarsky)
Attachment #567785 - Attachment is obsolete: true
Attachment #567785 - Flags: review?(bzbarsky)
Attached file Manual testcase, v2 (obsolete) —
Attachment #567343 - Attachment is obsolete: true
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?
(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?
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.
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.
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).
> We can run multiple content processes now: dom.ipc.processCount > 1.

This works on mobile?
If it works anywhere.
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?
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...
Attachment #567874 - Flags: review?(bzbarsky)
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!
Okay, I think I got it...
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.
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...
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...
(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.)
(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.
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.
Attached patch Part 2, v4: DOM API (obsolete) — Splinter Review
Attachment #568777 - Flags: review?(bzbarsky)
Attachment #567874 - Attachment is obsolete: true
Attached patch Part 1, v3: Vibrator support (obsolete) — Splinter Review
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.
Attachment #568779 - Flags: review?(jones.chris.g)
Attachment #567342 - Attachment is obsolete: true
Attached patch Part 1, v3.1: Vibrator support (obsolete) — Splinter Review
Got rid of an extra hunk.
Attachment #568784 - Flags: review?(jones.chris.g)
Attachment #568779 - Attachment is obsolete: true
Attachment #568779 - Flags: review?(jones.chris.g)
Attached patch Interdiff part 1 v2 --> v3.1 (obsolete) — Splinter Review
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
(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.
> 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?
(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.
> 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.
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?
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.
Sounds like I need to rework this again.  I'll get something new up ASAP.
Attachment #568777 - Flags: review?(bzbarsky)
Attachment #568784 - Flags: review?(jones.chris.g)
Attachment #568786 - Attachment is obsolete: true
marking sec-review-needed, marking myself as secr and sched review for 2011.10.27
Whiteboard: [secr:curtisk]
> 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....
(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).
> 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?
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).
Attached patch Part 1, v4 (obsolete) — Splinter Review
Attachment #570288 - Flags: review?(jones.chris.g)
Attachment #568784 - Attachment is obsolete: true
Attached patch Part 2, v5 (obsolete) — Splinter Review
Attachment #570289 - Flags: review?(bzbarsky)
Attachment #568777 - Attachment is obsolete: true
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.
Attachment #570289 - Flags: review?(bzbarsky)
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.
Attachment #570288 - Flags: review?(jones.chris.g)
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.
Attached patch Part 1, v4.1 (obsolete) — Splinter Review
Attachment #570288 - Attachment is obsolete: true
Attached patch Part 2, v5.1 (obsolete) — Splinter Review
Call hal::VibrateWithAutoCancel instead of hal::Vibrate
Attachment #570289 - Attachment is obsolete: true
Comment on attachment 570307 [details] [diff] [review]
Part 1, v4.1

One-line change from v4, fixes cancel-on-hide.
Attachment #570307 - Attachment description: One-line change from v4, fixes cancel-on-hide. → Part 1, v4.1
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.
Depends on: 698057
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.
Attachment #570307 - Flags: review?(jones.chris.g)
Attachment #570309 - Flags: review?(bzbarsky)
Comment on attachment 570307 [details] [diff] [review]
Part 1, v4.1

New patch forthcoming per discussion.
Attachment #570307 - Flags: review?(jones.chris.g)
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.
> 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.
Attachment #570309 - Flags: review?(bzbarsky)
(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.
(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?
(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?  :)
Yes, let's discuss elsewhere, and no, the current Notifications API isn't enough.
Assignee: justin.lebar+bug → nobody
Depends on: 698782
Assignee: nobody → justin.lebar+bug
Attached patch Part 1, v5Splinter Review
Attachment #571159 - Flags: review?(jones.chris.g)
Attachment #570307 - Attachment is obsolete: true
Attached patch Part 2, v6 (obsolete) — Splinter Review
I'll hold off asking for review on this until we get part 1 in an agreeable state.
Attachment #570309 - Attachment is obsolete: true
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.
Attachment #571159 - Flags: review?(jones.chris.g) → review+
> >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.  :)
> 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) { ... }
(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.
(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.
> >+  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.)
> 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.)
I think you want

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

then.
Attachment #571161 - Flags: review?(bzbarsky)
(In reply to Chris Jones [:cjones] [:warhammer] from comment #132)
> I think you want
> 
>   if (!id.HasTraveledThroughIPC() && !WindowIsActive(id.GetWindow())) {
>      ^^^

Yes!!  Thanks.
> 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 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?
Attachment #571159 - Flags: review?(blassey.bugs)
Keywords: dev-doc-needed
Comment on attachment 571159 [details] [diff] [review]
Part 1, v5

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

bridge code looks good
Attachment #571159 - Flags: review?(blassey.bugs) → review+
I'm not going to get to this review today....
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
Part 1:
https://hg.mozilla.org/mozilla-central/rev/ebb4fe2d4c83

(Leaving open per comment 138)
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla10
Target Milestone: mozilla10 → ---
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).
Requesting tracking Firefox 10 due to comment 140.
> 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.
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.)
Attached file Manual testcase, v3 (obsolete) —
Attachment #567875 - Attachment is obsolete: true
Attachment #571159 - Flags: checkin+
Attached patch Part 2, v7 (obsolete) — Splinter Review
Unbitrotting.
Attachment #571161 - Attachment is obsolete: true
Attachment #571161 - Flags: review?(bzbarsky)
Attachment #573296 - Flags: review?(bzbarsky)
If you want to play around with it, a build is at http://people.mozilla.org/~jlebar/fennec-vibrator.apk
A long vibrator seems to kill that Fennec instance from comment 147:
http://people.mozilla.org/~mwargers/tests/vibrator_long.htm
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
"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
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.
The following loop kills the process right quick:

while(true) {
  navigator.mozVibrate([100, 100]);
}
Depends on: 701716
Attached patch Part 2, v8 (obsolete) — Splinter Review
Adding test for bug 701716
Attachment #573814 - Flags: review?(bzbarsky)
Attachment #573296 - Attachment is obsolete: true
Attachment #573296 - Flags: review?(bzbarsky)
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.
Attachment #573065 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Depends on: 672352
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.
I can probably manage it by Friday or Monday...  Seem ok?
Yes.  Thanks!
The w3c discussion seems to have settled on disallowing vibrate() and vibrate(null).  I'll modify the patch and test to reflect this.
Depends on: 706958
Attached patch Part 2, v9Splinter Review
Updated to the latest version of the spec.

I found a bug in AndroidHal; Chris, would you mind reviewing the change there?
Attachment #578644 - Flags: review?(bzbarsky)
Attachment #573814 - Attachment is obsolete: true
Attachment #573814 - Flags: review?(bzbarsky)
Attached file Manual testcase, v4
Attachment #573245 - Attachment is obsolete: true
Comment on attachment 578644 [details] [diff] [review]
Part 2, v9

Note that this depends on bug 706958.
Attachment #578644 - Flags: review?(jones.chris.g)
Attachment #578644 - Flags: review?(jones.chris.g) → review+
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.
Attachment #578644 - Flags: review?(bzbarsky) → review+
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.
> 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.
That's cleaner.  Thanks.
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.
Whiteboard: [secr:curtisk] → [secr:curtisk][needs dependency]
Whiteboard: [secr:curtisk][needs dependency] → [secr:curtisk][needs landing]
https://hg.mozilla.org/integration/mozilla-inbound/rev/44777be25060
Whiteboard: [secr:curtisk][needs landing] → [secr:curtisk]
https://hg.mozilla.org/mozilla-central/rev/44777be25060
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla11
No longer depends on: 696295
Depends on: 769571
Flags: sec-review+
Depends on: 792891
Safari will not implement this feature. https://webkit.org/status/#?search=vibration%20api
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.