Move click haptic feedback into platform

RESOLVED WONTFIX

Status

()

Firefox for Android
General
RESOLVED WONTFIX
6 years ago
6 years ago

People

(Reporter: wesj, Assigned: wesj)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

6 years ago
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.
(Assignee)

Comment 1

6 years ago
Created attachment 661919 [details] [diff] [review]
Patch v1

Trying to think of a good way to test this.
Assignee: nobody → wjohnston
Attachment #661919 - Flags: review?(bugs)
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.
Attachment #661919 - Flags: review?(bugs) → review-
(Assignee)

Comment 3

6 years ago
Created attachment 662404 [details] [diff] [review]
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?
Attachment #661919 - Attachment is obsolete: true
(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).
(Assignee)

Comment 5

6 years ago
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.
(Assignee)

Comment 6

6 years ago
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?
Attachment #664328 - Flags: feedback?(mwu)

Comment 7

6 years ago
(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

6 years ago
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.

Updated

6 years ago
Attachment #664328 - Flags: feedback?(mwu)
(Assignee)

Comment 9

6 years ago
(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

6 years ago
(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.

Comment 13

6 years ago
(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 :)
(Assignee)

Comment 15

6 years ago
Created attachment 671506 [details] [diff] [review]
Patch

Added a comment to the idl. Sounds like that's enough to make people happy for now.
Attachment #662404 - Attachment is obsolete: true
Attachment #671506 - Flags: review?(bugs)
(Assignee)

Comment 16

6 years ago
Comment on attachment 671506 [details] [diff] [review]
Patch

Talked to mfinkle today. We're just going to nix this feature entirely.
Attachment #671506 - Flags: review?(bugs)
(Assignee)

Updated

6 years ago
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → WONTFIX
(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 :)
You need to log in before you can comment on or make changes to this bug.