1120268 - [Stingray] Remove mozChromeEvents for hardware keys

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Gina Yeh [:gyeh] [:ginayeh]]

https://bugzilla.mozilla.org/show_bug.cgi?id=989198#c161

As I mentioned in bug989198, keyboard events are propogating to Gaia Apps, so no need to send mozChromeEvents for hardward keys.

Comment 1

[Kevin Chen[:kechen] (UTC + 8)]

Hi Gina, I change the assignee to myself first and will upload the patch asap. Thank you.

Comment 2

[Kevin Chen[:kechen] (UTC + 8)]

Attachment: 0001-Bug-1120268-Remove-mozChromeEvent-sent-from-hardware.patch

Hi Gina, could you please help me to check this patch. Thank you.

In this patch, I modied the code in filterHardwareKeys. The mainly modify is the removal of the send mozChromeEvvent code and deleting the redundent hardware key recognition code.

Comment 3

[Gina Yeh [:gyeh] [:ginayeh]]

Comment on attachment 8547428 [details] [diff] [review]
0001-Bug-1120268-Remove-mozChromeEvent-sent-from-hardware.patch

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

Kevin, please check my comments and questions below. Thanks.

::: b2g/chrome/content/shell.js
@@ +1,1 @@
> +/* -*- in/ent-tabs-mode: nil; js-indent-level: 2 -*- /

I think you accidently change the word 'indent' here. Please remove this part.

@@ +372,2 @@
>    filterHardwareKeys: function shell_filterHardwareKeys(evt) {
> +

Please rewrite the comments above and rename the function.

Also, remove the blank line here.

@@ +431,5 @@
>      switch (evt.type) {
>        case 'keydown':
>        case 'keyup':
> +      case 'mozbrowserbeforekeydown':
> +      case 'mozbrowserbeforekeyup':

No event listeners of the above two events are added, so we never receive these events.

Question: Why we need to handle 'mozbrowserbeforekeydown' and 'mozbrowserbeforekeyup' events?

Comment 4

[[:fabrice] Fabrice Desré]

Where are these changes landing?

Comment 5

[Gina Yeh [:gyeh] [:ginayeh]]

I think it should be m-c, Fabrice.

Comment 6

[Gina Yeh [:gyeh] [:ginayeh]]

Btw, I cannot review this patch but I can provide some feedbacks. So, after comments are addressed, Kevin will send another review request to Fabrice, thanks.

Comment 7

[Kevin Chen[:kechen] (UTC + 8)]

Attachment: 0001-Bug-1120268-Remove-mozChromeEvent-sent-from-hardware.patch

Hi Gina, I change the function name and the comment. 

       case 'keydown':
       case 'keyup':
+      case 'mozbrowserbeforekeydown':
+      case 'mozbrowserbeforekeyup':
         this.filterHardwareKeys(evt);
         break;

And the two addition events above is for the code testing when I modified this code. Sorry for forgetting to remove them.

Please help me to check the new patch. Thank you.

Comment 8

[Gina Yeh [:gyeh] [:ginayeh]]

Comment on attachment 8548029 [details] [diff] [review]
0001-Bug-1120268-Remove-mozChromeEvent-sent-from-hardware.patch

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

The function name makes sense to me now.

One more question, can we merge multiple if conditions into one? It increases readability of this function. :)

::: b2g/chrome/content/shell.js
@@ +365,5 @@
>      UserAgentOverrides.uninit();
>      IndexedDBPromptHelper.uninit();
>    },
>  
> +  // If this key event represents a hardware button which need to be send as

nit: s/need/needs/g

@@ +373,3 @@
>      var type;
> +
> +    if(evt.keyCode==evt.DOM_VK_F1){

nit: please insert spaces before and after '=='.

Comment 9

[Kevin Chen[:kechen] (UTC + 8)]

Attachment: 0001-Bug-1023837-Move-LightType-LightMode-FlashMode-from-.patch

Hi Gina, I compacted the code with removing some redundant if condition, and renamed some variables.

Please check if these changes are make sense. Thank you.

Comment 10

[Gina Yeh [:gyeh] [:ginayeh]]

Comment on attachment 8548087 [details] [diff] [review]
0001-Bug-1023837-Move-LightType-LightMode-FlashMode-from-.patch

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

Kevin, it seems like you put wrong patch here.

Comment 11

[Kevin Chen[:kechen] (UTC + 8)]

Attachment: 0001-Bug-1120268-Remove-mozChromeEvent-sent-from-hardware.patch

Sorry about the mistake! Attachment is the correct one.

Comment 12

[Gina Yeh [:gyeh] [:ginayeh]]

Comment on attachment 8548113 [details] [diff] [review]
0001-Bug-1120268-Remove-mozChromeEvent-sent-from-hardware.patch

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

Great work! f+ with nit addressed. :)

::: b2g/chrome/content/shell.js
@@ +403,4 @@
>          break;
>      }
>  
> +    // Let applications receive the headset button and media key press/release event.

nit: Please use 'message' rather than 'event'.

@@ +404,5 @@
>      }
>  
> +    // Let applications receive the headset button and media key press/release event.
> +    if (message !== this.lastHardwareButtonEventMessage) {
> +      this.lastHardwareButtonEventMessage = message;

nit: I suggest to remove 'Event' in the property name.

@@ +410,2 @@
>      }
> +    return;

nit: redundant line.

Comment 13

[Kevin Chen[:kechen] (UTC + 8)]

Attachment: 0001-Bug-1120268-Remove-mozChromeEvent-sent-from-hardware.patch

Hi Fabrice, could you help me to review this patch? Thank you.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=cefc27ad2486

Comment 14

[[:fabrice] Fabrice Desré]

Comment on attachment 8548597 [details] [diff] [review]
0001-Bug-1120268-Remove-mozChromeEvent-sent-from-hardware.patch

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

Looks good to me, but look at my comments.

::: b2g/chrome/content/shell.js
@@ +366,5 @@
>      IndexedDBPromptHelper.uninit();
>    },
>  
> +  // If this key event represents a hardware button which needs to be send as
> +  // a message. Broadcast it with the message set to 'xxx-button-press' or

nit: that should all be a single sentence: "...needs to be send as a message, broadcast it with..."

@@ +373,2 @@
>      var type;
> +    var message;

while we are touching this code, please do s/var/let for type and message.

@@ +410,2 @@
>      }
> +    return;

that useless, right?

Comment 15

[Kevin Chen[:kechen] (UTC + 8)]

Attachment: 0001-Bug-1120268-Remove-mozChromeEvent-sent-from-hardware.patch

Comment 16

[Kevin Chen[:kechen] (UTC + 8)]

Attachment: 0001-Bug-1120268-Remove-mozChromeEvent-sent-from-hardware.patch

Comment 17

[Gina Yeh [:gyeh] [:ginayeh]]

https://hg.mozilla.org/integration/b2g-inbound/rev/22a8b1ad4e2d

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/22a8b1ad4e2d