791831 - Move click haptic feedback into platform

Status: RESOLVED WONTFIX

(Firefox for Android Graveyard :: General, defect)

Description

[Wesley Johnston (:wesj)]

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.

Comment 1

[Wesley Johnston (:wesj)]

Attachment: Patch v1

Trying to think of a good way to test this.

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 3

[Wesley Johnston (:wesj)]

Attachment: Patch v2

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?

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

(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).

Comment 5

[Wesley Johnston (:wesj)]

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.

Comment 6

[Wesley Johnston (:wesj)]

Attachment: 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?

Comment 7

[Michael Wu [:mwu]]

(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?

Comment 8

[Michael Wu [:mwu]]

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.

Comment 9

[Wesley Johnston (:wesj)]

(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?

Comment 10

[Michael Wu [:mwu]]

(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.

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 13

[Michael Wu [:mwu]]

(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.

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

oh, I misinterpreted :)

Comment 15

[Wesley Johnston (:wesj)]

Attachment: Patch

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

Comment 16

[Wesley Johnston (:wesj)]

Comment on attachment 671506 [details] [diff] [review]
Patch

Talked to mfinkle today. We're just going to nix this feature entirely.

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

(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 :)