Closed Bug 518266 Opened 10 years ago Closed 9 years ago

Implement mechanism to provide haptic feed back

Categories

(Firefox for Android Graveyard :: General, defect)

defect
Not set

Tracking

(fennec2.0+)

RESOLVED FIXED
Tracking Status
fennec 2.0+ ---

People

(Reporter: blassey, Assigned: blassey)

References

Details

(Keywords: dev-doc-complete)

Attachments

(4 files, 4 obsolete files)

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.
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?
Attached patch patch (obsolete) — Splinter Review
Assignee: nobody → blassey.bugs
Status: NEW → ASSIGNED
Attachment #487834 - Flags: review?(mwu)
Attachment #487834 - Flags: review?(doug.turner)
tracking-fennec: --- → ?
Blocks: 595441
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.
Attachment #487834 - Flags: review?(doug.turner) → review-
(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?
Attachment #487834 - Flags: review?(mwu)
Attached patch idl patch (obsolete) — Splinter Review
Attachment #487834 - Attachment is obsolete: true
Attachment #488097 - Flags: superreview?(benjamin)
Attachment #488097 - Flags: review?(doug.turner)
Attached patch ipc patch (obsolete) — Splinter Review
Attachment #488098 - Flags: superreview?(roc)
Attachment #488098 - Flags: review?(doug.turner)
Attached patch android patch (obsolete) — Splinter Review
Attachment #488099 - Flags: review?(mwu)
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.
Attachment #488097 - Flags: superreview?(benjamin)
Attachment #488097 - Flags: review?(doug.turner)
Attachment #488097 - Flags: review-
(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.
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
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.
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 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.
(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)
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?
(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
Attachment #488098 - Flags: superreview?(roc)
Attachment #488098 - Flags: superreview-
Attachment #488098 - Flags: review?(doug.turner)
Attachment #488098 - Flags: review-
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.
Attachment #488099 - Flags: review?(mwu)
tracking-fennec: ? → 2.0+
Attached patch idl patchSplinter Review
Attachment #488097 - Attachment is obsolete: true
Attachment #491423 - Flags: review?(doug.turner)
Attached patch android patchSplinter Review
Attachment #488099 - Attachment is obsolete: true
Attachment #491424 - Flags: review?(doug.turner)
Attached patch ipc patchSplinter Review
Attachment #488098 - Attachment is obsolete: true
Attachment #491425 - Flags: review?(doug.turner)
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.
Attachment #491423 - Flags: review?(doug.turner) → review+
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.
Attachment #491424 - Flags: review?(doug.turner) → review+
Comment on attachment 491425 [details] [diff] [review]
ipc patch

patch is probably okay, but it sounds like it isn't needed atm.
Attachment #491425 - Flags: review?(doug.turner) → review-
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
Attached patch maemo patchSplinter Review
Attachment #492396 - Flags: review?(doug.turner)
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.
Attachment #492396 - Flags: review?(doug.turner) → review+
(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.
Flags: in-testsuite?
Flags: in-litmus?
Depends on: 614751
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
With a little input from mfinkle, the doc is now finished.
You need to log in before you can comment on or make changes to this bug.