Add support for Hands Free Profile to Firefox for Android

NEW
Unassigned

Status

()

Firefox for Android
General
5 years ago
5 years ago

People

(Reporter: blassey, Unassigned)

Tracking

(Depends on: 1 bug)

Trunk
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 6 obsolete attachments)

Comment hidden (empty)
Created attachment 706465 [details] [diff] [review]
WIP patch
Comment on attachment 706465 [details] [diff] [review]
WIP patch

What does this feature give us?

Comment 3

5 years ago
It gives headset functionality (transmit/receive) of audio from the headset.  Currently A2DP is being used which provides receive-only audio.

Comment 4

5 years ago
Created attachment 707336 [details] [diff] [review]
Patch to add xevents to fennec

Comment 5

5 years ago
Created attachment 707854 [details] [diff] [review]
Added support for static configuration of XEvent broadcast receiver
Attachment #707336 - Attachment is obsolete: true
(Reporter)

Updated

5 years ago
Attachment #707854 - Attachment is patch: true
Comment on attachment 707854 [details] [diff] [review]
Added support for static configuration of XEvent broadcast receiver

Review of attachment 707854 [details] [diff] [review]:
-----------------------------------------------------------------

I'm getting xevents with this patch and I see they're being translated into key events.

Comment 7

5 years ago
Will be working next on patch for HFP support
However, I'm not seeing key events in content according to my test page http://dump.lassey.us/key_test.html

Jim, Chris, any thoughts?
(In reply to Brad Lassey [:blassey] from comment #8)
> However, I'm not seeing key events in content according to my test page
> http://dump.lassey.us/key_test.html
> 
> Jim, Chris, any thoughts?

My guess is you need to add Android to Gecko key mappings here, http://mxr.mozilla.org/mozilla-central/source/widget/android/nsWindow.cpp#1406
looks like we need bug 674739 first
Depends on: 674739
Turns out volume up and volume down key codes are already handled by nsWindow, but they're turned into command events (https://mxr.mozilla.org/mozilla-central/source/widget/android/nsWindow.cpp#1589).

Unfortunately blame doesn't tell us why since it was part of the initial check in of android widget code. Vlad, mwu, do you remember why we did this?

Also, does anyone know how a command event can be handled by content?
Created attachment 709061 [details] [diff] [review]
patch for volume key events

since app commands are chrome only, it looks like we're going to want to use key events. This patch uses the key events added by the patch on bug 674739
(Reporter)

Updated

5 years ago
Depends on: 839446

Comment 13

5 years ago
Created attachment 715848 [details] [diff] [review]
Patch to add xevents to fennec
Attachment #707854 - Attachment is obsolete: true

Updated

5 years ago
Attachment #715848 - Flags: review?(nchen)

Updated

5 years ago
Attachment #715848 - Flags: review?(nchen)

Comment 14

5 years ago
Created attachment 716099 [details] [diff] [review]
Patch for XEvent support for MWC demo

This patch is for adding headset button press support for a Fennec WebRTC demo
Attachment #716099 - Flags: review?(nchen)
Comment on attachment 716099 [details] [diff] [review]
Patch for XEvent support for MWC demo

Review of attachment 716099 [details] [diff] [review]:
-----------------------------------------------------------------

I think blassey should review most of this patch.

::: mobile/android/base/GeckoAppShell.java
@@ +605,5 @@
> +    public static void sendKeyEvent(KeyEvent event) {
> +        if (mEditableClient != null)
> +            mEditableClient.sendEvent(GeckoEvent.createKeyEvent(event));
> +    }
> +

For sending a key event, please use,

> View v = GeckoApp.mAppContext.getLayerView();
> if (v != null) {
>     v.dispatchKeyEvent(event);
> }

And you don't even have to add a method to GeckoAppShell; you can use this anywhere.
Attachment #716099 - Flags: review?(nchen)
Attachment #716099 - Flags: review?(blassey.bugs)
Attachment #716099 - Flags: feedback+
Comment on attachment 716099 [details] [diff] [review]
Patch for XEvent support for MWC demo

Review of attachment 716099 [details] [diff] [review]:
-----------------------------------------------------------------

r=blassey with the cleanups that jim and I pointed out. You'll have to attach another patch for checkin anyway and I can double check it then.

::: mobile/android/base/AndroidManifest.xml.in
@@ +26,5 @@
>  
>      <uses-permission android:name="android.permission.WAKE_LOCK"/>
>      <uses-permission android:name="android.permission.VIBRATE"/>
>      <uses-permission android:name="android.permission.SET_WALLPAPER"/>
> +    <uses-permission android:name="android.permission.BLUETOOTH"/>  

please remove the trailing whitespace

@@ +156,5 @@
> +                <action android:name="android.bluetooth.device.action.ACL_CONNECTED" />
> +                <action android:name="android.bluetooth.device.action.ACL_DISCONNECTED"/>
> +                <action android:name="android.bluetooth.headset.profile.action.AUDIO_STATE_CHANGED"/>
> +                <action android:name="android.bluetooth.headset.profile.action.CONNECTION_STATE_CHANGED"/>
> +                <action android:name="android.bluetooth.headset.action.VENDOR_SPECIFIC_HEADSET_EVENT"/>                 

another trailing whitespace

::: mobile/android/base/BluetoothControlsReceiver.java
@@ +15,5 @@
> +
> +public class BluetoothControlsReceiver extends BroadcastReceiver
> +{
> +	
> +    //plantronics XEvent button ids - TODO: extract to another class w vendor specific code?

yes, this list will get long as we add vendors, let's pull it out now.

@@ +35,5 @@
> +        if(BluetoothHeadset.ACTION_VENDOR_SPECIFIC_HEADSET_EVENT.equals(intent.getAction())){
> +        	
> +        	if(intent.hasCategory(BluetoothHeadset.VENDOR_SPECIFIC_HEADSET_EVENT_COMPANY_ID_CATEGORY + "." + BluetoothAssignedNumbers.PLANTRONICS)){
> +        		handlePlantronicsEvent(context, intent);	    
> +		

couple more whitespace issues here, it might be helpful for you to click on the "splinter review" link next to the patch in bugzilla. Anything highlighted in pink is a suspected whitespace issue.

@@ +51,5 @@
> +    }
> +    
> +    
> +    private void handlePlantronicsEvent(Context context, Intent intent){
> +    	    

delete the blank line

@@ +73,5 @@
> +		case PLT_KEYCODE_POWER:
> +			keyCode = KeyEvent.KEYCODE_POWER;
> +			break;
> +		case PLT_KEYCODE_HOOK:
> +			keyCode = KeyEvent.KEYCODE_SPACE; /* TODO: replace with a more suitable event when available */

add the bug number for DOM3 media key events to the comment
Attachment #716099 - Flags: review?(blassey.bugs) → review+

Comment 17

5 years ago
Created attachment 716322 [details] [diff] [review]
Patch for XEvent support for MWC demo

Added changes suggested by review - tested
Attachment #716099 - Attachment is obsolete: true
Attachment #715848 - Attachment is obsolete: true
Attachment #716322 - Flags: review?(nchen)
Comment on attachment 716322 [details] [diff] [review]
Patch for XEvent support for MWC demo

Review of attachment 716322 [details] [diff] [review]:
-----------------------------------------------------------------

Looks pretty good! Just some more small changes.

::: mobile/android/base/BluetoothControlsReceiver.java
@@ +14,5 @@
> +import android.view.KeyEvent;
> +import android.view.View;
> +
> +public class BluetoothControlsReceiver extends BroadcastReceiver
> +{	

There are still whitespace issues in BluetoothControlsReceiver.java. Can you click on "Splinter Review" next to the patch in Bugzilla, and delete all the extra whitespaces highlighted in pink?

@@ +26,5 @@
> +    private static final int PLT_KEYCODE_FORWARD = 9;
> +    private static final int PLT_KEYCODE_REWIND = 10;
> +    private static final int PLT_KEYCODE_NEXT = 11;
> +    private static final int PLT_KEYCODE_PREVIOUS = 12;
> +    private static final int PLT_KEYCODE_STOP = 13;

I think blassey was suggesting that you move these to another class now, instead of doing it later. Maybe move all Plantronics-specific code to a separate class in this file? For example,

> class PlantronicsEventHandler
> {
>     private static final int PLT_KEYCODE_POWER = 1;
>     ...
>     public static handleEvent(Context context, Intent intent) {
>         ...
>     }
>     private static sendKeyEvent(int keyCode) {
>         ...
>         BluetoothControlsReceiver.sendKeyEvent(...);
>         ...
>     }
> }
> 
> public class BluetoothControlsReceiver extends BroadcastReceiver
> {
>     ...

Then do,

> if (intent.hasCategory(BluetoothHeadset.VENDOR_SPECIFIC_HEADSET_EVENT_COMPANY_ID_CATEGORY +
                         "." + BluetoothAssignedNumbers.PLANTRONICS)) {
>     PlantronicsEventHandler.handleEvent(context, intent);
> } else {
>     ...

@@ +31,5 @@
> +    
> +    @Override
> +    public void onReceive(Context context, Intent intent) {
> +        if(BluetoothHeadset.ACTION_VENDOR_SPECIFIC_HEADSET_EVENT.equals(intent.getAction())){    	
> +        	if(intent.hasCategory(BluetoothHeadset.VENDOR_SPECIFIC_HEADSET_EVENT_COMPANY_ID_CATEGORY + "." + BluetoothAssignedNumbers.PLANTRONICS)){

Long line; generally we try to limit lines to 100 characters

@@ +73,5 @@
> +		case PLT_KEYCODE_POWER:
> +			keyCode = KeyEvent.KEYCODE_POWER;
> +			break;
> +		case PLT_KEYCODE_HOOK:
> +			keyCode = KeyEvent.KEYCODE_SPACE; /* TODO: Replace with a more suitable event when Bug https://bugzilla.mozilla.org/show_bug.cgi?id=839446 is fixed */

Long line. Also just the bug number is fine; no need for URL.

::: mobile/android/base/GeckoAppShell.java
@@ +598,5 @@
>  
>      // Tell the Gecko event loop that an event is available.
>      public static native void notifyGeckoOfEvent(GeckoEvent event);
>  
> +

Only thing changed in GeckoAppShell.java is an empty line; let's remove it.
Attachment #716322 - Flags: review?(nchen) → feedback+

Comment 19

5 years ago
Created attachment 716838 [details] [diff] [review]
Patch for XEvent support for MWC demo

Refactored out the Plantronics specific stuff into it's own class
Attachment #716322 - Attachment is obsolete: true
Attachment #716838 - Flags: review?(nchen)
Comment on attachment 716838 [details] [diff] [review]
Patch for XEvent support for MWC demo

Review of attachment 716838 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks Cary! I suggest keeping PlantronicsEventHandler inside BluetoothControlsReceiver.java because it's only used by BluetoothControlsReceiver (can even be a child class of BluetoothControlsReceiver). But I think I'm just nit-picking now :)  I'm okay with either way.

Also your patch says the author is blassey, but I think you can take the credit here :)
Attachment #716838 - Flags: review?(nchen) → review+

Comment 21

5 years ago
Thanks Jim - making a small change to the patch to include my name and resumbitting

Comment 22

5 years ago
Created attachment 716874 [details] [diff] [review]
Support for XEvents over bluetooth

Just a name change on the patch
Attachment #716838 - Attachment is obsolete: true
Attachment #716874 - Flags: review?(blassey.bugs)

Comment 23

5 years ago
Comment on attachment 716874 [details] [diff] [review]
Support for XEvents over bluetooth

Same as the approved patch but has my name on it instead of Brad's
Attachment #716874 - Attachment description: Same as the approved patch but has my name on it instead of Brad's → Support for XEvents over bluetooth
(Reporter)

Updated

5 years ago
Attachment #716874 - Flags: review?(blassey.bugs) → review+
You need to log in before you can comment on or make changes to this bug.