For our current haptic feedback in Fennec we use our isClickable function. Instead of implementing that in JS like we currently do, I'd like to share with the code being used for the new platform touch/mouse "fluffing". Rather than exposing that to JS, I think its easiest to just move the haptic feedback into the platform code. I'd like to also do this (via a separate pref) on long press context menus, but we don't currently use that EventStateManager code (we use platform long tap detection in Fennec). In bug 788073 I'm trying to move us to use the platform touch "fluffing". I had to remove our haptic feedback there, so this is required to restore functionality.

Trying to think of a good way to test this.

Comment on attachment 661919 [details] [diff] [review] Patch v1 >+ if (mCurrentTarget && IsElementClickable(mCurrentTarget) && >+ aEvent->clickCount == 1 && mClickHapticFeedback) { >+ nsCOMPtr<nsIHapticFeedback> haptic = >+ do_CreateInstance("@mozilla.org/widget/hapticfeedback;1"); >+ if (haptic) { >+ haptic->PerformSimpleAction(haptic->ShortPress); >+ } >+ } You really want to create a new instance of nsIHapticFeedback for each click (when haptics is enabled)? I assume you want do_GetService How slow can PerformSimpleAction be? Does it block the calling thread? We must not block the main Gecko thread.

Fixed the service piece. (In reply to Olli Pettay [:smaug] from comment #2) > How slow can PerformSimpleAction be? Does it block the calling thread? We > must not block the main Gecko thread. For the Android implementation, PerformSimpleAction calls into Android's View.performHapticFeedback, which (eventually) winds up calling into the VibratorService through Vibrator.java: http://developer.android.com/reference/android/os/Vibrator.html That's a system service, supposedly running in its own thread inside the system server process (i.e. it shouldn't block). Are you worried about other implementations?

(In reply to Wesley Johnston (:wesj) from comment #3) > That's a system service, supposedly running in its own thread inside the > system server process (i.e. it shouldn't block). Are you worried about other > implementations? Yes, and I'm also worried about "supposedly". We must not block main Gecko thread if just possible. I think I'd prefer approach where we had only one implementation of nsIHapticFeedback. It would then call platform specific stuff using a helper thread (assuming we can't ensure that OS level haptics stuff doesn't block).

I tested this with some simple Vibrator v = (Vibrator) getSystemService(VIBRATOR_SERVICE); Log.i("XXX", "XXX - before " + SystemClock.currentThreadTimeMillis()); v.vibrate(5000); Log.i("XXX", "XXX - after " + SystemClock.currentThreadTimeMillis()); and it doesn't block (2ms between the two log statements), but I'll put together something to move these calls to a helper thread.

Only Android and Meego implement nsIHaptic. I think we're better to just kill those and move to using hal? I stole a bit from Gonk. This doesn't work yet, but mwu, does this look like the right track to go to ensure that haptic feedback is off the main thread?

(In reply to Wesley Johnston (:wesj) from comment #6) > Created attachment 664328 [details] [diff] [review] > OMT Patch > > Only Android and Meego implement nsIHaptic. I think we're better to just > kill those and move to using hal? > > I stole a bit from Gonk. This doesn't work yet, but mwu, does this look like > the right track to go to ensure that haptic feedback is off the main thread? Haptic feedback/vibration is already off the main thread on gonk so no additional work is necessary. I think we should assume that hal backends should implement things in a non-blocking fashion, which appears to be the case on Android too?

Additionally, I think there may be a case to be made for leaving haptic feedback to the gaia frontend to implement since that's where all the policy is.

(In reply to Michael Wu [:mwu] from comment #7) > Haptic feedback/vibration is already off the main thread on gonk so no > additional work is necessary. I think we should assume that hal backends > should implement things in a non-blocking fashion, which appears to be the > case on Android too? hal/android/AndroidHal::Vibrate isn't off main thread explicitly that I see? (the Android VibrationService happens to be, but that's apparently not guaranteed). I... have no strong preference here. Smaug wants us to move things off main thread in one place so that every implementation doesn't have to. Is there a better way to do that?

(In reply to Wesley Johnston (:wesj) from comment #9) > hal/android/AndroidHal::Vibrate isn't off main thread explicitly that I see? > (the Android VibrationService happens to be, but that's apparently not > guaranteed). I... have no strong preference here. Smaug wants us to move > things off main thread in one place so that every implementation doesn't > have to. Is there a better way to do that? Doing |adb shell "echo 1000 > /sys/class/timed_output/vibrator/enable"| shows that the Android kernel interface for vibration is inherently non-blocking. (enable takes millisecond values) That command returns instantly. I don't think we should waste time engineering around a problem that doesn't exist on the platforms we're interested in.

At least nsIHapticFeedback::performSimpleAction needs some comment saying that the implementation of the method *must not* block the caller.

(In reply to Michael Wu [:mwu] from comment #10) > (enable takes millisecond values) Just curious, how many milliseconds? Since even milliseconds sound quite high.

(In reply to Olli Pettay [:smaug] from comment #12) > (In reply to Michael Wu [:mwu] from comment #10) > > (enable takes millisecond values) > Just curious, how many milliseconds? Since even milliseconds sound quite > high. I meant the file itself takes millisecond values for how long the vibrator should vibrate. So in the example above, 1000 means 1000 milliseconds.

oh, I misinterpreted :)

Added a comment to the idl. Sounds like that's enough to make people happy for now.

Comment on attachment 671506 [details] [diff] [review] Patch Talked to mfinkle today. We're just going to nix this feature entirely.

(In reply to Wesley Johnston (:wesj) from comment #16) > Comment on attachment 671506 [details] [diff] [review] > Patch > > Talked to mfinkle today. We're just going to nix this feature entirely. Ah, nice. This is the way to handle my reviews :)