1121298 - Add URI Helper for MozNDEFRecord

Status: RESOLVED FIXED

(Firefox OS Graveyard :: NFC, defect)

Description

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

Add some helper function to encode/decode URI in MozNDEFRecord.

Comment 1

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

Attachment: Part 1: refactor cstor in MozNDEFRecord.

Comment 2

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

Attachment: Part 2: Add createURI in MozNDEFRecord.

Comment 3

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

Attachment: Part 3: Add parseURI in MozNDEFRecord.

Comment 4

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

Attachment: Part 4: Add MOZ_COUNT_CTOR/DTOR for MozNDEFRecord.

Comment 5

[Dimi Lee [:dimi]]

Comment on attachment 8561248 [details] [diff] [review]
Part 2: Add createURI in MozNDEFRecord.

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

::: dom/nfc/MozNDEFRecord.cpp
@@ +126,5 @@
> +MozNDEFRecord::CreateURI(const GlobalObject& aGlobal,
> +                         const nsAString& aUri,
> +                         ErrorResult& aRv)
> +{
> +  if (aUri.IsVoid()) {

Also need to check IsEmpty

@@ +200,5 @@
>    IncSizeForPayload(payload.Length());
>  }
>  
>  void
> +MozNDEFRecord::InitPayload(JSContext* aCx, const nsAString& aUri, ErrorResult& aRv)

I think we might have other cases passing nsAString also in the future, Ex. text, mime
So i will suggest rename this function

@@ +203,5 @@
>  void
> +MozNDEFRecord::InitPayload(JSContext* aCx, const nsAString& aUri, ErrorResult& aRv)
> +{
> +  using namespace mozilla::dom::WellKnownURIPrefixValues;
> +

Do you think we should convert URI to lower case ?
However, I am fine either way.

@@ +240,5 @@
> +
> +  const char* uri = aUri.get();
> +  // strings[0] is "", so we start from 1.
> +  for (uint8_t i = 1; i < ArrayLength(strings); i++) {
> +    if (!strncmp(uri, strings[i].value, strings[i].length)) {

The length of EndGuard_ is 0 so it will always match the last comparison.
Should check if reach EndGuard_ instead

Comment 6

[Dimi Lee [:dimi]]

Comment on attachment 8561249 [details] [diff] [review]
Part 3: Add parseURI in MozNDEFRecord.

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

::: dom/nfc/MozNDEFRecord.cpp
@@ +175,5 @@
> +  }
> +
> +  using namespace mozilla::dom::WellKnownURIPrefixValues;
> +  const nsCString prefix = nsCString(strings[id].value, strings[id].length);
> +  nsCString payload = nsCString((const char*)&payloadData[1], payloadLen - 1);

const nsCString

Comment 7

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

(In reply to Dimi Lee[:dimi][:dlee] from comment #5)
> Comment on attachment 8561248 [details] [diff] [review]
> Part 2: Add createURI in MozNDEFRecord.
> 
> Review of attachment 8561248 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/nfc/MozNDEFRecord.cpp
> @@ +126,5 @@
> > +MozNDEFRecord::CreateURI(const GlobalObject& aGlobal,
> > +                         const nsAString& aUri,
> > +                         ErrorResult& aRv)
> > +{
> > +  if (aUri.IsVoid()) {
> 
> Also need to check IsEmpty

I don't see anywhere specified that uri cannot be "".

> 
> @@ +200,5 @@
> >    IncSizeForPayload(payload.Length());
> >  }
> >  
> >  void
> > +MozNDEFRecord::InitPayload(JSContext* aCx, const nsAString& aUri, ErrorResult& aRv)
> 
> I think we might have other cases passing nsAString also in the future, Ex.
> text, mime
> So i will suggest rename this function
> 
No, for Text we also need to add 'encoding' as parameter, and for Mime the payload will be of Uint8Array.
They should be handled separately by overloading.

Comment 8

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

Attachment: Part 2: Add createURI in MozNDEFRecord. v2.

Comment 9

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

Attachment: Part 3: Add parseURI in MozNDEFRecord. v2.

add const

Comment 10

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

Comment on attachment 8561250 [details] [diff] [review]
Part 4: Add MOZ_COUNT_CTOR/DTOR for MozNDEFRecord.

This shouldn't be needed. MozNDEFRecord implements AddRef/Release using the normal macros, so those should log this stuff well enough for leak detection.

Is there any particular reason why you want count ctor/dtor?

(But the patch as such is ok)

Comment 11

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

Comment on attachment 8564877 [details] [diff] [review]
Part 2: Add createURI in MozNDEFRecord. v2.

>+MozNDEFRecord::CreateURI(const GlobalObject& aGlobal,
>+                         const nsAString& aUri,
>+                         ErrorResult& aRv)
>+{
>+  if (aUri.IsVoid()) {
>+    aRv.Throw(NS_ERROR_DOM_INVALID_STATE_ERR);
>+    return nullptr;
>+  }
>+
>+  nsCOMPtr<nsPIDOMWindow> win = do_QueryInterface(aGlobal.GetAsSupports());
>+  if (!win) {
>+    aRv.Throw(NS_ERROR_FAILURE);
>+    return nullptr;
>+  }
>+
>+  nsRefPtr<MozNDEFRecord> ndefRecord = new MozNDEFRecord(win, TNF::Well_known);
>+  ndefRecord->InitType(aGlobal.Context(), RTD::U);
>+  ndefRecord->InitPayload(aGlobal.Context(), aUri, aRv);
>+  return ndefRecord.forget();
>+}
Why you want to create all these infallible Init* methods? Why not just constructor which takes the
needed data as param.

>+MozNDEFRecord::GetURIIdentifier(const nsCString& aUri)
>+{
>+  using namespace mozilla::dom::WellKnownURIPrefixValues;
>+
>+  const char* uri = aUri.get();
>+  // strings[0] is "", so we start from 1.
>+  // And the last element in strings is EndGuard_, so minus 1 to prevent
>+  // comparing with it.
>+  for (uint8_t i = 1; i < ArrayLength(strings) - 1; i++) {
>+    if (!strncmp(uri, strings[i].value, strings[i].length)) {
>+      return i;
>+    }
>+  }
I would do
MozNDEFRecord::GetURIIdentifier(const nsCString& aUri)
for (uint32_t i = 1; i < WellKnownURIPrefix::EndGuard_; ++i) {
  if (aUri.EqualsASCII(WellKnownURIPrefixValues::strings[i])) {
    return i;
  }
}
>+/**
>+ * Record Type Description.
>+ *
>+ * Record Types from well-known NDEF Records.
>+ * @see NFCForum-TS-RTD
>+ */
>+enum RTD {
>+  "U", // URI
>+};
It is not clear to me why we need this

Comment 12

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

(In reply to Olli Pettay [:smaug] from comment #11)
>   if (aUri.EqualsASCII(WellKnownURIPrefixValues::strings[i])) {
.value is missing there.

Comment 13

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

Comment on attachment 8564877 [details] [diff] [review]
Part 2: Add createURI in MozNDEFRecord. v2.

createURI is also a bit odd name here.
You don't create an URI, but NDEFRecord.
Maybe createNDEFFromURI()?

Comment 14

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

(In reply to Olli Pettay [:smaug] from comment #11)
> I would do
> MozNDEFRecord::GetURIIdentifier(const nsCString& aUri)
> for (uint32_t i = 1; i < WellKnownURIPrefix::EndGuard_; ++i) {
>   if (aUri.EqualsASCII(WellKnownURIPrefixValues::strings[i])) {
>     return i;

Ah, that would miss the 'n' in strncmp.
Use StringBeginsWith, and not .EqualsASCII

Comment 15

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

Comment on attachment 8561247 [details] [diff] [review]
Part 1: refactor cstor in MozNDEFRecord.

so why we want all these init methods?

Comment 16

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

Comment on attachment 8564878 [details] [diff] [review]
Part 3: Add parseURI in MozNDEFRecord. v2.

>+
>+  using namespace mozilla::dom::WellKnownURIPrefixValues;
>+  const nsCString prefix = nsCString(strings[id].value, strings[id].length);
>+  const nsCString payload = nsCString((const char*)&payloadData[1], payloadLen - 1);
>+
>+  aRetVal.Assign(NS_ConvertUTF8toUTF16(prefix + payload));


Why not
aRetVal.AssignASCII(strings[id].value);
aRetVal.Append(NS_ConvertUTF8toUTF16(nsDependentCString(static_cast<char*>(&payloadData[1]), payloadLen - 1)));


>+  /**
>+   * Parse this NDEF Record as URI, return null if this record cannot be parsed
returns

>+   * as a well-known URI record.
>+   */
>+  DOMString? parseURI();
Sounds a bit odd method. No URI parsing is happening. More like serializing to URI.
Would 'DOMString? getAsURI()' work here?

Comment 17

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

Comment on attachment 8561250 [details] [diff] [review]
Part 4: Add MOZ_COUNT_CTOR/DTOR for MozNDEFRecord.

obsolete since it's no needed.

Comment 18

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

(In reply to Olli Pettay [:smaug] from comment #11)
> Comment on attachment 8564877 [details] [diff] [review]
> Part 2: Add createURI in MozNDEFRecord. v2.
> 
> >+MozNDEFRecord::CreateURI(const GlobalObject& aGlobal,
> >+
> >+  nsRefPtr<MozNDEFRecord> ndefRecord = new MozNDEFRecord(win, TNF::Well_known);
> >+  ndefRecord->InitType(aGlobal.Context(), RTD::U);
> >+  ndefRecord->InitPayload(aGlobal.Context(), aUri, aRv);
> >+  return ndefRecord.forget();
> >+}
> Why you want to create all these infallible Init* methods? Why not just
> constructor which takes the
> needed data as param.
> 
(In reply to Olli Pettay [:smaug] from comment #15)
> Comment on attachment 8561247 [details] [diff] [review]
> Part 1: refactor cstor in MozNDEFRecord.
> 
> so why we want all these init methods?

Hi smaug
Yeah, I could remove those Init methods and add more constructors in MozNDEFRecord.h.

It's just that NDEF can be used to store many kinds of format of data, like URI (with UTF-8 encoding),
Text Message(with language code, also use utf-8 encoding), and binary data with specified
mime-type, .. etc.

Also I plan to add more static methods in MozNDEFRecord.webidl

createNDEFFromURI(DOMString uri);
createNDEFFromText(DOMString languageCode, DOMString text)
createNDEFFromMime(DOMString mime, Uint8Array data)
...

so instead of creating more constructors in MozNDEFRecord.h, 
I create more Init methods to initiate those fields to get more re-usability.
For example,
URI and Text could share the InitType(RTD) method added in Part 2.

Do you prefer adding more cstors instead of adding these Init methods?
I could remove those Init* if you think cstors is better.

Thanks

Comment 19

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

Ok, so you want to reuse those init methods in the near future. Then those are fine.

Comment 20

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

...assuming it leads to simpler code.

Comment 21

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

(In reply to Olli Pettay [:smaug] from comment #11)
> >+/**
> >+ * Record Type Description.
> >+ *
> >+ * Record Types from well-known NDEF Records.
> >+ * @see NFCForum-TS-RTD
> >+ */
> >+enum RTD {
> >+  "U", // URI
> >+};
> It is not clear to me why we need this

From the spec it mentions if the tnf of the NDEF Record is 'well-known', the 'type' field of the NDEF will be one of the following byte array
0x55, ("U") for URI
0x54, ("T") For Text
...
So I added RTD into WebIDL, and I have filed Bug 1138266 to use RTD in the constructors.

Comment 22

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

(In reply to Olli Pettay [:smaug] from comment #11)
> >+MozNDEFRecord::GetURIIdentifier(const nsCString& aUri)
> >+{
> >+  using namespace mozilla::dom::WellKnownURIPrefixValues;
> >+
> >+  const char* uri = aUri.get();
> >+  // strings[0] is "", so we start from 1.
> >+  // And the last element in strings is EndGuard_, so minus 1 to prevent
> >+  // comparing with it.
> >+  for (uint8_t i = 1; i < ArrayLength(strings) - 1; i++) {
> >+    if (!strncmp(uri, strings[i].value, strings[i].length)) {
> >+      return i;
> >+    }
> >+  }
> I would do
> MozNDEFRecord::GetURIIdentifier(const nsCString& aUri)
> for (uint32_t i = 1; i < WellKnownURIPrefix::EndGuard_; ++i) {
Add static_cast<uint32_t>(...)

  for (uint32_t i = 1; i < static_cast<iunt32_t>(WellKnownURIPrefix::EndGuard_); i++) {

For meeting error
../../../gecko/dom/nfc/MozNDEFRecord.cpp: In static member function 'static uint8_t mozilla::dom::MozNDEFRecord::GetURIIdentifier(const nsCString&)':
../../../gecko/dom/nfc/MozNDEFRecord.cpp:270:57: error: no match for 'operator<' in 'i < (mozilla::dom::WellKnownURIPrefix)36u'




(In reply to Olli Pettay [:smaug] from comment #16)
> >+
> >+  using namespace mozilla::dom::WellKnownURIPrefixValues;
> >+  const nsCString prefix = nsCString(strings[id].value, strings[id].length);
> >+  const nsCString payload = nsCString((const char*)&payloadData[1], payloadLen - 1);
> >+
> >+  aRetVal.Assign(NS_ConvertUTF8toUTF16(prefix + payload));
> 
> 
> Why not
> aRetVal.AssignASCII(strings[id].value);
> aRetVal.
> Append(NS_ConvertUTF8toUTF16(nsDependentCString(static_cast<char*>(&payloadDa
> ta[1]), payloadLen - 1)));
> 
Change the last line to
  aRetVal.Append(NS_ConvertUTF8toUTF16(
    nsDependentCSubstring(reinterpret_cast<char*>(&payloadData[1]), payloadLen - 1)));

For 
1: fix compilation error 
error: invalid static_cast from type 'uint8_t* {aka unsigned char*}' to type 'char*'
2. Use nsDependentCSubstring since the payloadData is not null-terminated.

Comment 23

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

(In reply to Olli Pettay [:smaug] from comment #13)
> Comment on attachment 8564877 [details] [diff] [review]
> Part 2: Add createURI in MozNDEFRecord. v2.
> 
> createURI is also a bit odd name here.
> You don't create an URI, but NDEFRecord.
> Maybe createNDEFFromURI()?
Hi Smaug
Since this is a static method so the 'NDEF' seems unneccesary to me, and I check the pattern in m-c, I'll rename it to
'createFromURI'.

Comment 24

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

Attachment: Part 2: Add createFromURI in MozNDEFRecord. v3.

Addressed smaug's comments.

Comment 25

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

Attachment: Part 3: Add getAsURI in MozNDEFRecord. v3.

addressed smaug's comments.

Comment 26

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

Comment on attachment 8561247 [details] [diff] [review]
Part 1: refactor cstor in MozNDEFRecord.

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

re-request r? again for in Bug 1138886 we also reuse the pattern to seperate type/id/payload from cstor.

Comment 27

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

Comment on attachment 8571278 [details] [diff] [review]
Part 2: Add createFromURI in MozNDEFRecord. v3.

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

Addressed Smaug's comments,
please also see my comment 21, comment 22 and comment 23 to see some slight change for smaug's previous comment.

Comment 28

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

Comment on attachment 8571281 [details] [diff] [review]
Part 3: Add getAsURI in MozNDEFRecord. v3.

Addressed Smaug's comment
Please see Comment 22 for the slight change of smaug's previous comment.

Comment 29

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

Comment on attachment 8571281 [details] [diff] [review]
Part 3: Add getAsURI in MozNDEFRecord. v3.

Shouldn't you set aRetVal to void string in case a valid value isn't returned.
So, perhaps initialize aRetVal.SetIsVoid(true); right at the beginning of the method.

Comment 30

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

Comment on attachment 8571278 [details] [diff] [review]
Part 2: Add createFromURI in MozNDEFRecord. v3.

Actually, why not just use Constructor for this? (why didn't I think of this earlier.)

Constructor(optional (MozNDEFRecordOptions or DOMString) optionsOrUrl)

Comment 31

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

(In reply to Olli Pettay [:smaug] (no new review request before March 8, please) from comment #30)
> Comment on attachment 8571278 [details] [diff] [review]
> Part 2: Add createFromURI in MozNDEFRecord. v3.
> 
> Actually, why not just use Constructor for this? (why didn't I think of this
> earlier.)
> 
> Constructor(optional (MozNDEFRecordOptions or DOMString) optionsOrUrl)

Thanks for this great suggestion, :)
I'll update it.

Also the url shouldn't be null/undefined, so I'll do it as two seperate constructors.

Thanks

Comment 32

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

Attachment: Part 2: Add Constructor(uri) for MozNDEFRecord. v4

smaug's comment addressed.
will ask for review next week since he's busy. :P

Comment 33

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

Attachment: Part 3: Add getAsURI in MozNDEFRecord. v4

added aRetVal.SetIsVoid(true);

Comment 34

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

Comment on attachment 8573105 [details] [diff] [review]
Part 2: Add Constructor(uri) for MozNDEFRecord. v4

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

Hi, Smaug
See my Comment 31 for why I made it two constructors, instead of using 'or' in your previous comments.

Thanks

Comment 35

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

Comment on attachment 8573105 [details] [diff] [review]
Part 2: Add Constructor(uri) for MozNDEFRecord. v4

So we don't want to throw if a random string is passed to the new
constructor?

It is tiny bit odd to create a record using a random UTF16 string, and then
payload gives that back as an array containing the data in UTF8. 

+  uint8_t *data
* goes with the type, not with variable name.

Comment 36

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

Smaug, thanks for your review.

(In reply to Olli Pettay [:smaug] from comment #35)
> Comment on attachment 8573105 [details] [diff] [review]
> Part 2: Add Constructor(uri) for MozNDEFRecord. v4
> 
> So we don't want to throw if a random string is passed to the new
> constructor?
> 
From the spec it mentions about the string should be as per RFC 3987.
I'll file a follow-up bug to verify this.

> It is tiny bit odd to create a record using a random UTF16 string, and then
> payload gives that back as an array containing the data in UTF8. 
>

From the spec it also mentions MUST use UTF8 encoding.
Or you have some other concerns?
 
> +  uint8_t *data
> * goes with the type, not with variable name.
I'll update the nits.

Thanks

Comment 37

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

https://hg.mozilla.org/integration/b2g-inbound/rev/34967f6af126
https://hg.mozilla.org/integration/b2g-inbound/rev/84773d851be6
https://hg.mozilla.org/integration/b2g-inbound/rev/e45e1dc34fb0

Comment 38

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

https://hg.mozilla.org/mozilla-central/rev/34967f6af126
https://hg.mozilla.org/mozilla-central/rev/84773d851be6
https://hg.mozilla.org/mozilla-central/rev/e45e1dc34fb0