518266 - Implement mechanism to provide haptic feed back

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Brad Lassey [:blassey] (use needinfo?)]

most phones have at least a vibrator and sometimes more to provide haptic feed back to users.  We should have an abstraction over the various interfaces that xul apps can use to provide appropriate haptic feedback to the users.

Comment 1

[Alex Pakhotin (:alexp)]

Apparently we need this for Android.
Hardware buttons do provide the feedback already, and we need to implement it for a long-click (context menu).

What other events do we want to use haptic feedback for?

Comment 2

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: patch

Comment 3

[Doug Turner (:dougt)]

Comment on attachment 487834 [details] [diff] [review]
patch

are you missing nsIHapticFeedback?

Lets make sure it isn't Android specific. There is no one haptic system, and some system are pretty complicated (like being able to control specific motors, setting their frequency and duration, etc).


it looks like it is pretty basic since RecvPerformHapticFeedback just holds a bool value.  I'd hope we could do something more generic.

Have you investigated other existing haptic APIs that we may want to implement (MeeGo/Qt, Maemo/Gtk, hapi)


Who is going to be calling this?  Nothing in this patch uses it yet.  Is that a follow up?

Separate out the new IPL into a new patch and ask r= from me, and sr= from roc.  He is the module owner and needs to oversee API changes.

Comment 4

[Brad Lassey [:blassey] (use needinfo?)]

(In reply to comment #3)
> Comment on attachment 487834 [details] [diff] [review]
> patch
> 
> are you missing nsIHapticFeedback?

yup, forgot to hg add it.

> 
> Lets make sure it isn't Android specific. There is no one haptic system, and
> some system are pretty complicated (like being able to control specific motors,
> setting their frequency and duration, etc).
> 
> 
> it looks like it is pretty basic since RecvPerformHapticFeedback just holds a
> bool value.  I'd hope we could do something more generic.

this is only intended to deal with button presses and button long presses. We can worry about giving finer grain control later.

> 
> Have you investigated other existing haptic APIs that we may want to implement
> (MeeGo/Qt, Maemo/Gtk, hapi)

I looked at maemo gtk, there is dbus api for press and long press

> Who is going to be calling this?  Nothing in this patch uses it yet.  Is that a
> follow up?

fennec's FE code, in bug 595441
> 
> Separate out the new IPL into a new patch and ask r= from me, and sr= from roc.
>  He is the module owner and needs to oversee API changes.

the new idl as well?

Comment 5

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: idl patch

Comment 6

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: ipc patch

Comment 7

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: android patch

Comment 8

[Doug Turner (:dougt)]

Comment on attachment 488097 [details] [diff] [review]
idl patch

roc is the owner of widget.  this interface lives in xpcom but is implemented in widget; and it really is a widget thing.

I think we want to do more here, don't you?  For example android provides full control over the vibrator:

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

If we want to keep it simple for now, I'd rather you wrap performHapticFeedback directly and provide the 3 enums.

Comment 9

[Brad Lassey [:blassey] (use needinfo?)]

(In reply to comment #8)
> Comment on attachment 488097 [details] [diff] [review]
> idl patch
> 
> roc is the owner of widget.  this interface lives in xpcom but is implemented
> in widget; and it really is a widget thing.
> 
> I think we want to do more here, don't you?  For example android provides full
> control over the vibrator:
> 
> http://developer.android.com/reference/android/os/Vibrator.html
> 
> If we want to keep it simple for now, I'd rather you wrap performHapticFeedback
> directly and provide the 3 enums.

I'm trying to do something that mameo and android can both support.

Besides, I don't see a need for android's keyboard press haptic.

Comment 10

[Doug Turner (:dougt)]

more notes on the API design (from IRC)

18:08 < shaver> dougt: should use a string arg for performHapticAction
18:08 < shaver> dougt: can be "notify" for now, but it means we can add more later without breaking the interface
18:08 < dougt> yeah, true
18:08 < shaver> or string, int, int
18:08 < shaver> so you can parameterize later
18:09 < dougt> there are things like "longtap", "tap", that we'd suppor tnow
18:09 < shaver> "type", arg1, arg2
18:09 < shaver> yeah, that might be enough
18:09 < dougt> but, i'd rather see us do a bit more than what is there.
18:09 < shaver> or you could take an object
18:09 < dougt> for example:
18:09 < dougt> http://developer.android.com/reference/android/os/Vibrator.html
18:09 < shaver> {type:"simple"}
18:09 < dougt> oh, i see what you are saying
18:09 < shaver> {type:"flexible", strength:0.5, duration:8}
18:10 < shaver> you get a nicer API that way
18:10 < shaver> more weblike
18:10 < dougt> imagine if we did more stuff like that.
18:10 < dougt> mind if I copy and paste?
18:10 < shaver> IMAGINE.
18:10 < shaver> no

Comment 11

[Mark Finkle (:mfinkle) (use needinfo?)]

Do we have a straight forward way of passing a JS object to IDL/XPCOM and unpacking it in the C++ impl?

If not, I think the string based parameter is a good way to move forward.

Comment 12

[Mark Finkle (:mfinkle) (use needinfo?)]

Oh, and this is currently not a bug about vibrator control - just simple haptic. We can control the vibrator in a followup to the interface, when needed.

Yes, I said "vibrator control"

Comment 13

[Michael Wu [:mwu]]

Comment on attachment 488099 [details] [diff] [review]
android patch

If this is the interface we want, then this impl mostly looks ok. I'm pretty suspicious of the ipc handling code in android widget though. That seems like something that should be done in some other layer. Also, I worry that having different behavior depending on long press is an androidism that might not fit well with other platforms.

Comment 14

[Brad Lassey [:blassey] (use needinfo?)]

(In reply to comment #13)
> Comment on attachment 488099 [details] [diff] [review]
> android patch

> Also, I worry that having
> different behavior depending on long press is an androidism that might not fit
> well with other platforms.

If another platform uses the same feedback for short and long press they can do so in the implementation (i.e. just ignore the flag)

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Boolean parameters are undesirable in general. Wouldn't you be better off with an enum, even if it only has two values for now?

Also, the service probably shouldn't live in widget. It's just another system service, why not put it in dom/system?

Comment 16

[Brad Lassey [:blassey] (use needinfo?)]

(In reply to comment #15)
> Boolean parameters are undesirable in general. Wouldn't you be better off with
> an enum, even if it only has two values for now?
> 
> Also, the service probably shouldn't live in widget. It's just another system
> service, why not put it in dom/system?

both of those work for me

Comment 17

[Michael Wu [:mwu]]

Comment on attachment 488099 [details] [diff] [review]
android patch

Sounds like we're changing the interface. Should be able to rubberstamp once there's a patch using the new interface.

Comment 18

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: idl patch

Comment 19

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: android patch

Comment 20

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: ipc patch

Comment 21

[Doug Turner (:dougt)]

Comment on attachment 491423 [details] [diff] [review]
idl patch

Brad and I spoke a bit.  This interface is fine and allows us to support perform({}) in the future.

>+interface nsIHapticFeedback : nsISupports {
>+    const long kShortPress = 0;
>+    const long kLongPress  = 1;
>+  /**
>+     * Perform haptic feedback
>+     *
>+     * @param isLongPress
>+     *        indicate wether feedback is for a long press (vs. short press)
>+     */
>+  void performSimpleAction(in long isLongPress);
>+
>+};


1) Space after the interface line and after the const values

2) No need for the kPrefix since you already have the nsIHapticFeedback namespace.  ShortPress and LongPress are fine.

3) typo in comment "wether"

r+ when you make these changes.

Comment 22

[Doug Turner (:dougt)]

Comment on attachment 491424 [details] [diff] [review]
android patch


>         nsAccelerometerSystem.cpp \
>         AndroidLocationProvider.cpp \
>+	nsHapticFeedback.cpp \
>         $(NULL)
> 

Align these.


>+NS_IMETHODIMP
>+nsHapticFeedback::PerformSimpleAction(PRInt32 aType)
>+{
>+    AndroidBridge* bridge = AndroidBridge::Bridge();
>+    if (bridge) {
>+        bridge->PerformHapticFeedback(aType == kLongPress);
>+        return NS_OK;
>+    }
>+    return NS_ERROR_NOT_IMPLEMENTED;
>+}

Wrong error code.  NS_ERROR_FAILURE?

r+ when these changes are made.

Comment 23

[Doug Turner (:dougt)]

Comment on attachment 491425 [details] [diff] [review]
ipc patch

patch is probably okay, but it sounds like it isn't needed atm.

Comment 24

[Michael Wu [:mwu]]

Comment on attachment 491424 [details] [diff] [review]
android patch

>@@ -556,6 +557,13 @@ AndroidBridge::HideProgressDialogOnce()
> }
> 
> void
>+AndroidBridge::PerformHapticFeedback(PRBool aIsLongPress)
>+{
>+    mJNIEnv->CallStaticObjectMethod(mGeckoAppShellClass,
>+                                    jPerformHapticFeedback, aIsLongPress);

CallStaticVoidMethod

Comment 25

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: maemo patch

Comment 26

[Doug Turner (:dougt)]

Comment on attachment 492396 [details] [diff] [review]
maemo patch


>+
> include $(topsrcdir)/config/rules.mk
>\ No newline at end of file

nit: add a new line to this makefile.

>+ *
>+ * The Original Code is Mozilla Android code.
>+ *


Not android code.


>+  DBusError err;
>+  dbus_error_init(&err);
>+  
>+  DBusConnection  *connection;
>+  connection = dbus_bus_get(DBUS_BUS_SYSTEM, &err);
>+  if (dbus_error_is_set(&err)) { 
>+    dbus_error_free(&err); 
>+    return NS_ERROR_FAILURE;
>+  }
>+  if (nsnull == connection) { 
>+    return NS_ERROR_FAILURE; 
>+  }

I do not think you need to also test for the connection.  err should be set if null is returned from dbus_bus_get.

If you do, do you have to also clean up the err?


>+  dbus_connection_set_exit_on_disconnect(connection,false);
>+  

please add a space after the comment, and remove the space on the newline  (there are a few instances of that in this new file).


>+  if (dbus_connection_send(connection, msg, NULL)) {
>+    dbus_connection_flush(connection);

I do not think you should call flush (unless I am missing something).  Basically this requires the calling code to block until the entire message queue is drained.



>+ *
>+ * The Original Code is Mozilla Android code.
>+ *

same


r+ if you fix the nits and explain the flush and the bus_get function.

Comment 27

[Brad Lassey [:blassey] (use needinfo?)]

(In reply to comment #26)
> 
> I do not think you need to also test for the connection.  err should be set if
> null is returned from dbus_bus_get.
the paranoid in me wants to check the return value since the behavior isn't specified in the api docs
> 
> If you do, do you have to also clean up the err?
not sure what you mean here
> 
> 
> >+  dbus_connection_set_exit_on_disconnect(connection,false);
> >+  
> 
> please add a space after the comment, and remove the space on the newline 
> (there are a few instances of that in this new file).
> 
> 
> >+  if (dbus_connection_send(connection, msg, NULL)) {
> >+    dbus_connection_flush(connection);
> 
> I do not think you should call flush (unless I am missing something). 
> Basically this requires the calling code to block until the entire message
> queue is drained.

as I understand it, this only flushes our connection (which only has the one message). I think you need to do this or you're basically racing the message handling vs. closing the connection.

Comment 28

[Brad Lassey [:blassey] (use needinfo?)]

pushed http://hg.mozilla.org/mozilla-central/rev/0f4b3354d426, http://hg.mozilla.org/mozilla-central/rev/3d73b386ac8e and http://hg.mozilla.org/mozilla-central/rev/372e38d12c5e

Comment 29

[Eric Shepherd [:sheppy]]

An initial draft of the reference for nsIHapticFeedback has been posted; however, it needs some details cleared up and we need an example of how to use it:

https://developer.mozilla.org/en/XPCOM_Interface_Reference/nsIHapticFeedback

Comment 30

[Eric Shepherd [:sheppy]]

With a little input from mfinkle, the doc is now finished.