1137107 - Move RequestType, ResponseType and NotificationTypes into NfcOptions.webidl

Status: RESOLVED FIXED

(Firefox OS Graveyard :: NFC, defect)

Description

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO ~ Feb.7]

We could take advantage of the enum in WebIDL to define those message types sent between nfcd.

Comment 1

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO ~ Feb.7]

Attachment: Part 1: Add NfcRequestType.

Comment 2

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO ~ Feb.7]

Attachment: Part 2: Add NfcResponseType and NfcNotificationType.

Comment 3

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO ~ Feb.7]

Comment on attachment 8569740 [details] [diff] [review]
Part 1: Add NfcRequestType.

Sorry, type too fast
r? to smaug for the WebIDL change in NfcOptions.webidl.

Comment 4

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO ~ Feb.7]

Comment on attachment 8569741 [details] [diff] [review]
Part 2: Add NfcResponseType and NfcNotificationType.

r? to smaug for the WebIDL change in NfcOptions.webidl.

Comment 5

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

Comment on attachment 8569740 [details] [diff] [review]
Part 1: Add NfcRequestType.

I wonder if some other NfcCommandOptions properties could be 'required' too.

Comment 6

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

Comment on attachment 8569741 [details] [diff] [review]
Part 2: Add NfcResponseType and NfcNotificationType.

notiType sounds really odd.

Isn't the API a bit odd if there is rspType and notiType but only value
from one of them is used.

Does webidl support
(NfcResponseType or NfcNotificationType) type;

Comment 7

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO ~ Feb.7]

(In reply to Olli Pettay [:smaug] from comment #6)
> Comment on attachment 8569741 [details] [diff] [review]
> Part 2: Add NfcResponseType and NfcNotificationType.
> 
> notiType sounds really odd.
> 
> Isn't the API a bit odd if there is rspType and notiType but only value
> from one of them is used.
> 
These two properties are in dictionary so they could be optional, so they won't exist at the same time.

Actually my first attempt is to divide the options into two dictionaries, one for response and the other for notification, but it turned out I have to modify a lot of code which using the dictionary, I didn't finish it and turn to this patch.

I'll try if I cound merge these two properties.

> Does webidl support
> (NfcResponseType or NfcNotificationType) type;
No, they are both enums 
   self._parser_results = parser.finish()
  File "/home/allstars/src/github/mozilla-b2g/B2G_FlameKK/gecko/dom/bindings/parser/WebIDL.py", line 5788, in finish
    production.finish(self.globalScope())
  File "/home/allstars/src/github/mozilla-b2g/B2G_FlameKK/gecko/dom/bindings/parser/WebIDL.py", line 1454, in finish
    member.complete(scope)
  File "/home/allstars/src/github/mozilla-b2g/B2G_FlameKK/gecko/dom/bindings/parser/WebIDL.py", line 3553, in complete
    type = self.type.complete(scope)
  File "/home/allstars/src/github/mozilla-b2g/B2G_FlameKK/gecko/dom/bindings/parser/WebIDL.py", line 2178, in complete
    [self.location, t.location, u.location])
WebIDL.WebIDLError: error: Flat member types of a union should be distinguishable, NfcResponseType (Wrapper) is not distinguishable from NfcNotificationType (Wrapper), /home/allstars/src/github/mozilla-b2g/B2G_FlameKK/gecko/dom/webidl/NfcOptions.webidl line 65:2
  (NfcResponseType or NfcNotificationType) type;
  ^
/home/allstars/src/github/mozilla-b2g/B2G_FlameKK/gecko/dom/webidl/NfcOptions.webidl line 65:3
  (NfcResponseType or NfcNotificationType) type;
   ^
/home/allstars/src/github/mozilla-b2g/B2G_FlameKK/gecko/dom/webidl/NfcOptions.webidl line 65:22
  (NfcResponseType or NfcNotificationType) type;
                      ^
make[6]: *** [codegen.pp] Error 1
make[5]: *** [dom/bindings/export] Error 2
make[5]: *** Waiting for unfinished jobs....
make[4]: *** [export] Error 2
make[3]: *** [default] Error 2
make[2]: *** [realbuild] Error 2
make[1]: *** [build] Error 2
make: *** [out/target/product/flame/obj/DATA/gecko_intermediates/gecko] Error 2

Comment 8

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO ~ Feb.7]

(In reply to Olli Pettay [:smaug] from comment #5)
> Comment on attachment 8569740 [details] [diff] [review]
> Part 1: Add NfcRequestType.
> 
> I wonder if some other NfcCommandOptions properties could be 'required' too.
Hi Smaug
No. Only the type and requestId are mandatory.

Comment 9

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO ~ Feb.7]

Hi Smaug
See my comment 7 for your previous question.
The term 'Notification' is from NFC Specification, and the specification uses XXX_RSP and XXX_NTF for the messages. 

So how do you think I rename them to
- ntfType
- rspType

And if you still think the dict seems odd, I'll file a separate bug to split the dict into two, one fo response and the other for notification.

How do you think?

Thanks

Comment 10

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

ntfType and rspType is better.
Should the relevant method throw if both types are used?

Comment 11

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO ~ Feb.7]

Attachment: Part 2: Add NfcResponseType and NfcNotificationType. v2.

rename to ntfType, 
also add MOZ_ASSERT for checking only one of rspType and ntfType should be used.

Comment 12

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO ~ Feb.7]

Attachment: Part 2 interdiff (v1 - v2)

add interdiff for Part 2.

Comment 13

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO ~ Feb.7]

https://hg.mozilla.org/integration/b2g-inbound/rev/16fef4e9b00b
https://hg.mozilla.org/integration/b2g-inbound/rev/80d1a77e6c2a

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/16fef4e9b00b
https://hg.mozilla.org/mozilla-central/rev/80d1a77e6c2a