Closed Bug 1137107 Opened 7 years ago Closed 7 years ago

Move RequestType, ResponseType and NotificationTypes into NfcOptions.webidl

Categories

(Firefox OS Graveyard :: NFC, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(firefox39 fixed)

RESOLVED FIXED
2.2 S12 (15may)
Tracking Status
firefox39 --- fixed

People

(Reporter: allstars.chh, Assigned: allstars.chh)

References

Details

(Whiteboard: [p=3])

Attachments

(3 files, 1 obsolete file)

We could take advantage of the enum in WebIDL to define those message types sent between nfcd.
Attachment #8569740 - Flags: review?(dlee) → review+
Attachment #8569741 - Flags: review?(dlee) → review+
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 on attachment 8569741 [details] [diff] [review]
Part 2: Add NfcResponseType and NfcNotificationType.

r? to smaug for the WebIDL change in NfcOptions.webidl.
Attachment #8569741 - Flags: review?(bugs)
Comment on attachment 8569740 [details] [diff] [review]
Part 1: Add NfcRequestType.

I wonder if some other NfcCommandOptions properties could be 'required' too.
Attachment #8569740 - Flags: review?(bugs) → review+
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;
Attachment #8569741 - Flags: review?(bugs) → review-
(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
(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.
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
Flags: needinfo?(bugs)
ntfType and rspType is better.
Should the relevant method throw if both types are used?
Flags: needinfo?(bugs)
rename to ntfType, 
also add MOZ_ASSERT for checking only one of rspType and ntfType should be used.
Attachment #8569741 - Attachment is obsolete: true
add interdiff for Part 2.
Attachment #8572396 - Flags: review?(bugs) → review+
You need to log in before you can comment on or make changes to this bug.