1055560 - B2G NFC: enum TNF in MozNDEFRecord.webidl

Status: RESOLVED FIXED

(Firefox OS Graveyard :: NFC, defect)

Description

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

TNF byte right now is 0x00 ~ 0x07, if it's reasonable we could try to make it as enum.

Comment 1

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

Attachment: Part 1: DOM Change

Comment 2

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

Attachment: Part 2: NfcService change

Comment 3

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

Attachment: Part 3: Update tests.

Comment 4

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

Attachment: Part 2: NfcService change v2

Comment 5

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

Comment on attachment 8475850 [details] [diff] [review]
Part 1: DOM Change

Hi Smaug
I use different style as opposed to W3C NFC API, http://w3c.github.io/nfc/proposals/common/nfc.html#idl-def-TNF, 
and the reason is in our dom/webidl, most enums start with lower case.

Also for the 'uri'(all lower case, W3C NFC API uses all upper case), current;y in our WebIDL there are 'document-uri', and 'blocked-uri' from CSPReport.webidl, so I use the same naming rule.

Could you help to review this for me ?

Thank you

Comment 6

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

Comment on attachment 8479820 [details] [diff] [review]
Part 2: NfcService change v2

>+    record.mTnf = static_cast<TNF>(tnf);
Could you assert that tnf is < TNF::EndGuard_, if possible.
(I'm not sure what kind of code MOZ_BEGIN_ENUM_CLASS ends up creating for Webidl enums.)

Comment 7

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

Attachment: Part 2: NfcService change v3

add assert for tnf.

Comment 8

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

Attachment: Gaia Pull Request

Hi, Alive
This patch is base on Bug 1053732, so this PR contains 3 commits from Bug 1053732.

Send r? to you to review the commit 'Bug 1055560 - enum TNF in MozNDEFRecord.'

Thanks

Comment 9

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

Comment on attachment 8475850 [details] [diff] [review]
Part 1: DOM Change

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

::: dom/webidl/MozNDEFRecord.webidl
@@ +12,5 @@
> +  "absolute-uri",
> +  "external",
> +  "unknown",
> +  "unchanged",
> +  "reserved"

Just start working on Bug 1007724, From clause 3.3, NDEF spec 1.0.
there's a requirement that states:

"The TNF value MUST NOT be 0x07.(reserved)"

So I'll remove 'reserved' from this enum.

Comment 10

[(inactive after 6/18) Alive Kuo [:alive]]

Comment on attachment 8480428 [details] [review]
Gaia Pull Request

Defer review..

Comment 11

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

Attachment: Part 1: DOM Change. v2

removed 'reserved'

Comment 12

[Greg Weng [:snowmantw][:gweng][:λ]]

Comment on attachment 8480428 [details] [review]
Gaia Pull Request

Thanks, bug I've some questions left on GitHub page. So set review again if the patch get ready again. And please remember to squash those commits to one commit before you land the patch.

Comment 13

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

Comment on attachment 8480428 [details] [review]
Gaia Pull Request

Sorry Greg,
I have mentioned in Comment 8 to ask review the commit 'Bug 1055560 - enum TNF in MozNDEFRecord.'

Your review is on Bug 1053732, which is not this bug.

Can you review the commit again?
(https://github.com/allstarschh/gaia/commit/0a3a1a6748835d576608a2e939e06202306afe50)

Thank you

Comment 14

[Greg Weng [:snowmantw][:gweng][:λ]]

Comment on attachment 8480428 [details] [review]
Gaia Pull Request

Oh okay. The patch comes without issues I think.

Comment 15

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

Comment on attachment 8480317 [details] [diff] [review]
Part 2: NfcService change v3

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

::: dom/nfc/gonk/NfcMessageHandler.cpp
@@ +308,5 @@
>    for (int i = 0; i < recordCount; i++) {
>      int32_t tnf = aParcel.readInt32();
>      NDEFRecordStruct record;
> +    record.mTnf = static_cast<TNF>(tnf);
> +    MOZ_ASSERT(record.mTnf < TNF::EndGuard_);

Will move this assert to NfcService since in Bug 1059168 will do the assert check in NfcService because the techList is contained in the byte array and not checkable in NfcMessageHandler.

Comment 16

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

Attachment: Part 2: NfcService change v4

Move MOZ_ASSSERT to NfcService

Comment 17

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

https://hg.mozilla.org/integration/b2g-inbound/rev/1559d39a2d78
https://hg.mozilla.org/integration/b2g-inbound/rev/8c6dac528337
https://hg.mozilla.org/integration/b2g-inbound/rev/26d497fdb8e9

Try:
https://tbpl.mozilla.org/?tree=Try&rev=178e3d4db84e

Comment 18

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

Gaia try:
https://tbpl.mozilla.org/?rev=f0137a15208c6fa84528ed9fbb83250637c11a73&tree=Gaia-Try

Comment 19

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

Gaia merged in master. https://github.com/mozilla-b2g/gaia/commit/0357f1dcc45fc63ec32018a9925960b2318ca02b

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1559d39a2d78
https://hg.mozilla.org/mozilla-central/rev/8c6dac528337
https://hg.mozilla.org/mozilla-central/rev/26d497fdb8e9