Add Media button (play, pause, stop, forward, backward) support. In order to enable Bluetooth AVRCP (Audio / Video Remote Control Profile) profile. It required to enable key mapping. This method of overriding VK_F15-F16, VK_U-Z and sending a system message.

Hi Eric, Similar bug is Bug 805744. I just added at the same place.

Comment on attachment 706296 [details] [diff] [review] Add support for media button Review of attachment 706296 [details] [diff] [review]: ----------------------------------------------------------------- Just curious, where's the keymappings come from? Are you just using unused keys?

Hi Kyle, It's because BlueZ AVRCP uses these key for mapping. In AVRCP.kl key 200 MEDIA_PLAY WAKE key 201 MEDIA_PAUSE WAKE key 166 MEDIA_STOP WAKE key 163 MEDIA_NEXT WAKE key 165 MEDIA_PREVIOUS WAKE key 168 MEDIA_REWIND WAKE key 208 MEDIA_FAST_FORWARD WAKE In BlueZ stack audio/control.c } key_map[] = { { "PLAY", PLAY_OP, KEY_PLAYCD }, { "STOP", STOP_OP, KEY_STOPCD }, { "PAUSE", PAUSE_OP, KEY_PAUSECD }, { "FORWARD", FORWARD_OP, KEY_NEXTSONG }, { "BACKWARD", BACKWARD_OP, KEY_PREVIOUSSONG }, { "REWIND", REWIND_OP, KEY_REWIND }, { "FAST FORWARD", FAST_FORWARD_OP, KEY_FASTFORWARD }, { NULL } };

Ah, ok, figured it was that, just wanted to make sure in case anyone asks me (I still get lots of bluetooth questions, heh).

Comment on attachment 706296 [details] [diff] [review] Add support for media button Review of attachment 706296 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me.

Try run for ff15ad52ecb8 is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=ff15ad52ecb8 Results (out of 18 total builds): success: 17 warnings: 1 Builds (or logs if builds failed) available at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/shuang@mozilla.com-ff15ad52ecb8

Try run for f9f3b50e8191 is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=f9f3b50e8191 Results (out of 14 total builds): success: 14 Builds (or logs if builds failed) available at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/shuang@mozilla.com-f9f3b50e8191

Update patch title and reviewer

Comment on attachment 716392 [details] [diff] [review] Add Media button support for Bluetooth AVRCP 1.3 Review of attachment 716392 [details] [diff] [review]: ----------------------------------------------------------------- ::: b2g/chrome/content/shell.js @@ +410,5 @@ > this.lastHardwareButtonEventType = type; > gSystemMessenger.broadcastMessage('headset-button', type); > return; > } > + if (((evt.keyCode >= evt.DOM_VK_U && evt.keyCode <= evt.DOM_VK_Z) || Nit: blank line before the if() and add a comment explaining why we do that. ::: widget/gonk/GonkKeyMapping.h @@ +107,5 @@ > + NS_VK_V, // MEDIA_STOP > + NS_VK_W, // MEDIA_NEXT > + NS_VK_X, // MEDIA_PREVIOUS > + NS_VK_Y, // MEDIA_REWIND > + NS_VK_Z, // MEDIA_FAST_FORWARD So if I hit "V" on a real keyboard this will trigger a media-stop-button system message?

I checked nsIDOMKeyEvent.idl, DOM_VK_V defines as 0x56, which is Key code constant: Stop media key. In the original GonkKeyMapping.h, NS_VK_U commented as MEDIA_PLAY_PAUSE. NS_VK_V looks weired but it maps to Media key instead of "V".

Sorry, now I understand your question. I think I'm wrong about mapping. I will change it.

Hi Fabrice, I misunderstood key code and virtual key code, this is why I misused NS_VK_V. I think the proper way is to add NS_VK_MEDIA_STOP virtual key. I found actually there is no Media key relative in nsIDOMKeyEvent.idl. I wonder it is ok to add media button definition into nsIDOMKeyEvent.idl. If it is NOT proper to add media button in nsIDOMKeyEvent.idl. Can we use other keys like Bug 805744 (use F1)? Can you provide any suggestion?

Shawn, we need input from people that are doing DOM3 Events here I think, see bug 674739.

Please wait the fix of bug 842927. Then, you can get rid of such hacky implementation.

Hi Masayuki, any target schedule regarding the fix of bug 842927? Will it be part of v1.1?

(In reply to Shawn Huang from comment #18) > Hi Masayuki, any target schedule regarding the fix of bug 842927? Will it be > part of v1.1? I'd like to land them ASAP, but the schedule depends on the reviewers. First, smaug will check all of them. After that, each widget reviewer will review them.

Due to schedule difficulty for A2DP/AVRCP, can we accept hacky implementation first?

I changed to other function keys from F15~F22

Comment on attachment 730596 [details] [diff] [review] Bug 834554-Add media button support for AVRCP 1.3 feature Review of attachment 730596 [details] [diff] [review]: ----------------------------------------------------------------- I'm sorry but I don't see any reason to rush this before bug 842927 lands.

Now, these keys are available with KeyboardEvent.key or nsKeyEvent::mKeyNameIndex. See https://developer.mozilla.org/en-US/docs/DOM/KeyboardEvent#keyname_table_android for the mapping.

OK. I will follow the new way to implement AVRCP for Android, thank you.

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) (Offline 5/28 - 6/1) from comment #23) > Now, these keys are available with KeyboardEvent.key or > nsKeyEvent::mKeyNameIndex. See > https://developer.mozilla.org/en-US/docs/DOM/ > KeyboardEvent#keyname_table_android for the mapping. I'm not an expert for this keyboard event. There are a few questions I want to know further based on my original patch. In b2g/chrome/content/shell.js, we use keyCode to send system message, which binds android keycode in widget/gonk/GonkKeyMapping.h. My question is from current implementation, I still cannot use virtual key of media button series keycode. I checked dom/interfaces/events/nsIDOMKeyEvent.idl, I don't see extra virtual key definition defined. Would you mind to provide me some more advices here?

Are you trying to improve here? http://mxr.mozilla.org/mozilla-central/source/b2g/chrome/content/shell.js#360 If so, use KeyboardEvent.key instead of KeyboardEvent.keyCode. http://dev.w3.org/2006/webapi/DOM-Level-3-Events/html/DOM3-Events.html#events-KeyboardEvent-key

Yes. It works fine on Android. Thanks a lot.

Hi masayuki, When trying to map all media-related keys in shell.js, and I found something interesting. All media-related keys are mapping to a name starting with MediaXxxxx. For example, MediaPlay, MediaPause, and MediaRewind. Just out of curiosity. There's one exception: FastFwd. In widget/shared/NativeKeyToDOMKeyName.h:1048:KEY_MAP_ANDROID (FastFwd, AKEYCODE_MEDIA_FAST_FORWARD), are we mapping |AKEYCODE_MEDIA_FAST_FORWARD| to |FastFwd| rather than to |MediaFastFwd| for other reason?

I'd like to steal it from Shawn :)

(In reply to Gina Yeh [:gyeh] [:ginayeh] from comment #28) > Hi masayuki, > > When trying to map all media-related keys in shell.js, and I found something > interesting. All media-related keys are mapping to a name starting with > MediaXxxxx. For example, MediaPlay, MediaPause, and MediaRewind. > > Just out of curiosity. There's one exception: FastFwd. In > widget/shared/NativeKeyToDOMKeyName.h:1048:KEY_MAP_ANDROID (FastFwd, > AKEYCODE_MEDIA_FAST_FORWARD), are we mapping |AKEYCODE_MEDIA_FAST_FORWARD| > to |FastFwd| rather than to |MediaFastFwd| for other reason? We just use the key names which defined by D3E draft. If the names will be changed, we need to change them. FYI: https://www.w3.org/Bugs/Public/show_bug.cgi?id=22084

Thanks for your information, masayuki. :)

Comment on attachment 758487 [details] [diff] [review] Patch 1(v1): Support AVRCP 1.0 Review of attachment 758487 [details] [diff] [review]: ----------------------------------------------------------------- I'm ok with the overall idea, though I'm not the most suitable person to do this since there's not much related to Bluetooth. ;) Please redirect to Fabrice after revision. Thanks! ::: b2g/chrome/content/shell.js @@ +382,5 @@ > break; > + } > + > + // Handle media-related keys and set mediaKey to true > + var isMediaKey = false; To avoid adding "isMediaKey = true;" in each 'case' block, you could set it to false in the 'default' case. @@ +384,5 @@ > + > + // Handle media-related keys and set mediaKey to true > + var isMediaKey = false; > + switch (evt.key) { > + case 'MediaNextTrack': // Media backward button Weird comment. Shouldn't it be 'forward' button? @@ +409,5 @@ > + type = 'media-stop-button'; > + isMediaKey = true; > + break; > + case 'MediaRewind': // Media rewind button > + type = 'media-rewind'; To be consistent, please use 'media-rewind-button' instead. @@ +416,5 @@ > + case 'FastFwd': // Media fast forward button > + type = 'media-fast-forward-button'; > + isMediaKey = true; > + break; > + } About those comments at the end of each 'case' statement, since the name of these keys are clear enough and almost the same as what you're trying to explain in comments, we don't really need them. @@ +418,5 @@ > + isMediaKey = true; > + break; > + } > + > + // A real key. Don't filter it al all; let it propagate to Gaia Please refine the comment. @@ +420,5 @@ > + } > + > + // A real key. Don't filter it al all; let it propagate to Gaia > + if (!type) { > + return; We can move this to 'default' of above switch block.

Comment on attachment 758487 [details] [diff] [review] Patch 1(v1): Support AVRCP 1.0 Review of attachment 758487 [details] [diff] [review]: ----------------------------------------------------------------- Thanks. Will update patch soon. ::: b2g/chrome/content/shell.js @@ +382,5 @@ > break; > + } > + > + // Handle media-related keys and set mediaKey to true > + var isMediaKey = false; Good point. Thanks. @@ +384,5 @@ > + > + // Handle media-related keys and set mediaKey to true > + var isMediaKey = false; > + switch (evt.key) { > + case 'MediaNextTrack': // Media backward button Oops! Will fix in the next patch. @@ +388,5 @@ > + case 'MediaNextTrack': // Media backward button > + type = 'media-next-track-button'; > + isMediaKey = true; > + break; > + case 'MediaPreviousTrack': // Media forward button Should be "backward" button @@ +420,5 @@ > + } > + > + // A real key. Don't filter it al all; let it propagate to Gaia > + if (!type) { > + return; I'm afraid we couldn't move this to the default case of above switch since hardware key like "HOME" shouldn't be ignored.

Updated based on feedback from echou.

Comment on attachment 761846 [details] [diff] [review] Patch 1(v2): Support AVRCP 1.0 Fabrice, I'm not sure who should be the reviewer of this part. Could you help to review this patch or re-direct to to right reviewer. Thanks.

Comment on attachment 761846 [details] [diff] [review] Patch 1(v2): Support AVRCP 1.0 Review of attachment 761846 [details] [diff] [review]: ----------------------------------------------------------------- ::: b2g/chrome/content/shell.js @@ +381,5 @@ > type = 'headset-button'; > break; > + } > + > + var isMediaKey; Nit: s/var/let @@ +440,5 @@ > gSystemMessenger.broadcastMessage('headset-button', type); > return; > } > > + if (isMediaKey) { I don't see any place where isMediaKey is set to true.

Fabrice, thanks for your review. I update the patch and please check it.

Sorry for annoying review request. v2 and v3 don't work though :( v4 has been tested and verified. Please help to check it. Thanks.

Comment on attachment 762450 [details] [diff] [review] Patch 1(v4): Support AVRCP 1.0 Review of attachment 762450 [details] [diff] [review]: ----------------------------------------------------------------- ::: b2g/chrome/content/shell.js @@ +415,5 @@ > + case 'FastFwd': > + type = 'media-fast-forward-button'; > + isMediaKey = true; > + break; > + } I think that we could something a bit nicer: let mediaKeys = {"MediaNextTrack": "media-next-track-button", "MediaPreviousTrack": "media-previous-track-button", ...}; let isMediaKey = false; if (mediaKeys[evt.key) { type = mediaKeys[evt.key; isMediaKey = true; } What do you think?

(In reply to Fabrice Desré [:fabrice] from comment #40) > I think that we could something a bit nicer: > let mediaKeys = {"MediaNextTrack": "media-next-track-button", > "MediaPreviousTrack": "media-previous-track-button", > ...}; > let isMediaKey = false; > if (mediaKeys[evt.key) { > type = mediaKeys[evt.key; > isMediaKey = true; > } > > What do you think? It's much much better. Let me try it and update patch soon.

Thanks for your feedback. It looks neater :)