834772 - Add support for Hands Free Profile to Firefox for Android

Status: RESOLVED INCOMPLETE

(Firefox for Android Graveyard :: General, defect)

Comment 1

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

Attachment: WIP patch

Comment 2

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

Comment on attachment 706465 [details] [diff] [review]
WIP patch

What does this feature give us?

Comment 3

[cary.bran]

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

Comment 4

[cary.bran]

Attachment: Patch to add xevents to fennec

Comment 5

[cary.bran]

Attachment: Added support for static configuration of XEvent broadcast receiver

Comment 6

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

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

[cary.bran]

Will be working next on patch for HFP support

Comment 8

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

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?

Comment 9

[(inactive) Jim Chen [:jchen] [:darchons]]

(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

Comment 10

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

looks like we need bug 674739 first

Comment 11

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

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?

Comment 12

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

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

Comment 13

[cary.bran]

Attachment: Patch to add xevents to fennec

Comment 14

[cary.bran]

Attachment: Patch for XEvent support for MWC demo

This patch is for adding headset button press support for a Fennec WebRTC demo

Comment 15

[(inactive) Jim Chen [:jchen] [:darchons]]

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.

Comment 16

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

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

Comment 17

[cary.bran]

Attachment: Patch for XEvent support for MWC demo

Added changes suggested by review - tested

Comment 18

[(inactive) Jim Chen [:jchen] [:darchons]]

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.

Comment 19

[cary.bran]

Attachment: Patch for XEvent support for MWC demo

Refactored out the Plantronics specific stuff into it's own class

Comment 20

[(inactive) Jim Chen [:jchen] [:darchons]]

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

Comment 21

[cary.bran]

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

Comment 22

[cary.bran]

Attachment: Support for XEvents over bluetooth

Just a name change on the patch

Comment 23

[cary.bran]

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

Comment 24

[BMO Automation]

We have completed our launch of our new Firefox on Android. The development of the new versions use GitHub for issue tracking. If the bug report still reproduces in a current version of [Firefox on Android nightly](https://play.google.com/store/apps/details?id=org.mozilla.fenix) an issue can be reported at the [Fenix GitHub project](https://github.com/mozilla-mobile/fenix/). If you want to discuss your report please use [Mozilla's chat](https://wiki.mozilla.org/Matrix#Connect_to_Matrix) server https://chat.mozilla.org and join the [#fenix](https://chat.mozilla.org/#/room/#fenix:mozilla.org) channel.