1063525 - [NFC] getEventType in nsNfc.js should be removed

Status: RESOLVED FIXED

(Firefox OS Graveyard :: NFC, defect)

Description

[Krzysztof Mioduszewski[:tauzen]]

getEventType method [1] maps event name (i.e. "peerready") to a const number [2], which is later on used for comparison here [3]. This is not needed and confusing, the const could be removed and regular event string name comparison could used. 

Additionally to avoid typing each time the name of event we can introduce a const string, and replace all string name occurrences with it.

[1] - https://github.com/mozilla/gecko-dev/blob/63adb59ffb694e524f8b4096425f4f1be540eeb5/dom/nfc/nsNfc.js#L288
[2] - https://github.com/mozilla/gecko-dev/blob/63adb59ffb694e524f8b4096425f4f1be540eeb5/dom/nfc/nsNfc.js#L25
[3] - https://github.com/mozilla/gecko-dev/blob/63adb59ffb694e524f8b4096425f4f1be540eeb5/dom/nfc/nsNfc.js#L231

Comment 1

[Krzysztof Mioduszewski[:tauzen]]

Attachment: 0001-Bug-1063525-removal-of-getEventType-method-from-nsNf.patch

Comment 2

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Comment on attachment 8485772 [details] [diff] [review]
0001-Bug-1063525-removal-of-getEventType-method-from-nsNf.patch

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

::: dom/nfc/nsNfc.js
@@ +23,5 @@
>                                     "@mozilla.org/AppsService;1",
>                                     "nsIAppsService");
> +
> +const NFC_PEER_EVENT_READY = "peerready";
> +const NFC_PEER_EVENT_LOST  = "peerlost";

The original event names come with 'on' prefix.
Smaug removed the 'on' prefix in Bug 943613 comment 5.

Also the argument in __DOM__.get/setEventHandler still has 'on' prefix.
Better had asked him for the details before you decide to const these.

Comment 3

[Krzysztof Mioduszewski[:tauzen]]

Hi Olli,

Could you clarify a bit the issue which Yoshi raised in comment 2? What is the reason that __DOM__.get/setEventHandler takes the 'onpeerready' as an argument [1] but the eventListnerWasAdded is fired with only 'peerready' string [2][3]. 
Do you think it makes sense to introduce consts for the string names here? I can prepare the patch without this and just do a plain string comparison, or introduce a const with 'on' prefix also. Although the best solution would be if we could unify this and introduce one const to use in set/getEventHandlers and eventListnerWasAdded.


[1] - https://github.com/mozilla/gecko-dev/blob/63adb59ffb694e524f8b4096425f4f1be540eeb5/dom/nfc/nsNfc.js#L213
[2] - https://github.com/mozilla/gecko-dev/blob/63adb59ffb694e524f8b4096425f4f1be540eeb5/dom/nfc/nsNfc.js#L230
[3] - https://github.com/mozilla/gecko-dev/blob/63adb59ffb694e524f8b4096425f4f1be540eeb5/dom/nfc/nsNfc.js#L290

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Krzysztof Mioduszewski[:kmioduszewski][:tauzen] from comment #3)
> Hi Olli,
> 
> Could you clarify a bit the issue which Yoshi raised in comment 2? What is
> the reason that __DOM__.get/setEventHandler takes the 'onpeerready' as an
> argument [1] but the eventListnerWasAdded is fired with only 'peerready'
> string [2][3]. 
Eventhandler name is 'onfoo', and event listener is added for an event type 'foo'.



> Do you think it makes sense to introduce consts for the string names here? I
> can prepare the patch without this and just do a plain string comparison, or
> introduce a const with 'on' prefix also. 
I don't have strong opinion about const. Whichever makes the code easier to read.

Comment 5

[Krzysztof Mioduszewski[:tauzen]]

Attachment: (v1.1) 0001-Bug-1063525-removal-of-getEventType-method-from-nsNf.patch

Thanks Olli for clarifying this. I've removed the consts and used strings, since it's easier to read the code this way. 

Yoshi could you take a look once more?

Comment 6

[Krzysztof Mioduszewski[:tauzen]]

Try-run: https://tbpl.mozilla.org/?tree=Try&rev=e4a74fc54e72

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/3ae90639bd87

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3ae90639bd87