Closed Bug 909224 Opened 6 years ago Closed 6 years ago

Want to add the phonetic name of the name to mozContacts

Categories

(Core Graveyard :: DOM: Contacts, defect)

All
Gonk (Firefox OS)
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla31

People

(Reporter: te-fukuda, Assigned: te-fukuda)

References

Details

(Whiteboard: [m+])

Attachments

(1 file, 6 obsolete files)

User Agent: Mozilla/5.0 (Windows NT 5.1) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/28.0.1500.95 Safari/537.36
OS: All → Gonk (Firefox OS)
In areas that use Kanji, such as China and Japan, I register the name in Kanji in the address book.
However, Kanji has many readings.
In areas that use Kanji, in order to facilitate the use of the address book, kana reading of Kanji should be added to the address book.
Attached patch patch (obsolete) — Splinter Review
Attachment #795878 - Flags: review?(anygregor)
This has been requested before, and documented as a priority 5 request:
* https://wiki.mozilla.org/ContactsAPI#Priority_5_feature_requests

I requested consideration of phonetic name in vCard (that MozContacts is based on) over a year ago[1], and there has been almost no interest:

[1] http://www.ietf.org/mail-archive/web/vcarddav/current/msg02514.html

If you would like to see this feature added to MozContacts, it must be first added to vCard. 

To do that, please follow-up on the VCARDDAV mailing list[2], preferably by following up on [1], and provide answers and persuasion there to get phonetic name adopted by that group.
(In reply to Tantek Çelik from comment #3)
> To do that, please follow-up on the VCARDDAV mailing list[2], preferably by
> following up on [1], and provide answers and persuasion there to get
> phonetic name adopted by that group.

[2] https://www.ietf.org/mailman/listinfo/vcarddav
(In reply to Tantek Çelik from comment #3)
> This has been requested before, and documented as a priority 5 request:
> * https://wiki.mozilla.org/ContactsAPI#Priority_5_feature_requests
> 
> I requested consideration of phonetic name in vCard (that MozContacts is
> based on) over a year ago[1], and there has been almost no interest:
> 
> [1] http://www.ietf.org/mail-archive/web/vcarddav/current/msg02514.html
> 
> If you would like to see this feature added to MozContacts, it must be first
> added to vCard. 
> 
> To do that, please follow-up on the VCARDDAV mailing list[2], preferably by
> following up on [1], and provide answers and persuasion there to get
> phonetic name adopted by that group.

Hello Tantek,

Thanks for your reply.

I'll take a look at the VCARDDAV mailing list.
(In reply to Tantek Çelik from comment #4)
> (In reply to Tantek Çelik from comment #3)
> > To do that, please follow-up on the VCARDDAV mailing list[2], preferably by
> > following up on [1], and provide answers and persuasion there to get
> > phonetic name adopted by that group.
> 
> [2] https://www.ietf.org/mailman/listinfo/vcarddav

It inquired about the addition of 'Phonetic' property in VCARDDAV mailing list.

http://www.ietf.org/mail-archive/web/vcarddav/current/msg02801.html
Thanks. Looks like there is a good discussion going on. 

If the outcome of the discussion is to endorse a new vCard4 property, then we can look at adding it to the spec. 

Until then we can leave this bug open to track the issue.
Hi Tantek,

I made a draft of phonetic transcription property(phonetic-given-name and phonetic-family-name) of vCard4.0.
I want to hear your comment.

http://www.ietf.org/mail-archive/web/vcarddav/current/msg02824.html
Hi Tantek,

The draft of phonetic-transcription of vCard4.0 was updated.
I want to hear your comment.

http://www.ietf.org/mail-archive/web/vcarddav/current/msg02844.html
Hi Tantek,

The draft of phonetic-transcription of vCard4.0 was updated.
I want to hear your comment.

http://www.ietf.org/mail-archive/web/vcarddav/current/msg02851.html
Is there any update here?
There is no schedule of update.
I have created a Bugzilla[1] related to this matter.

[1]:https://bugzilla.mozilla.org/show_bug.cgi?id=936386
Component: General → DOM: Contacts
Product: Firefox OS → Core
Hey Tantek,

can you please check comment 9 and tell us if now is a good time to add this in the mozContacts API?

Thanks!
Flags: needinfo?(tantek)
Status: UNCONFIRMED → NEW
Ever confirmed: true
Attached patch bug909224_patch (obsolete) — Splinter Review
I made a patch in the latest environment.
Review Is it possible?
Attachment #8373198 - Flags: review?(tantek)
Attachment #8373198 - Flags: review?(tantek) → review?(anygregor)
Attachment #795878 - Attachment is obsolete: true
Attachment #795878 - Flags: review?(anygregor)
Hi Gregor,

Should I remake a patch?

Thanks.
Flags: needinfo?(anygregor)
I don't think Tantek reads bugmail. Can you email him directly and see if the proposed draft is enough for us to add this?
Did the API discussion on the mailing list come to any conclusions?
Flags: needinfo?(anygregor)
Since the reply about use of "MEDIA-TYPE" is not got from the area director of IETF, a discussion is under continuation.
Is the addition of a property impossible, if the conclusion of a discussion is reached and vCard specification is not changed?
Flags: needinfo?(anygregor)
Because we constantly have the need to add things that are not in vcard, and because the Contacts API is slated to eventually be phased out in favor of DataStore which will allow apps to add their own metadata, I think we should add a way to add non-vcard metadata for now.

Something like:

{
  "name": ...
  ...

  "moz": {
    "phoneticGivenName": string/number/blob,
    "phoneticFamilyName": string/number/blob
  }
}

This way it's clear which properties are not vcard properties, and we can reuse existing infrastructure we have for sorting etc.

This means that to get all contacts sorted by phoneticFamilyName you'd do something like contacts.getAll({ sortBy: "moz.phoneticFamilyName"})

Does this sound ok to people?
Flags: needinfo?(anygregor)
Hi Jonas,

Thank you for a good idea!
I think that the property of non-vCard can be added if it is this way.
I'll consider it.

Thanks,

Teiichiro
(In reply to Jonas Sicking (:sicking) from comment #21)
> Because we constantly have the need to add things that are not in vcard, and
> because the Contacts API is slated to eventually be phased out in favor of
> DataStore which will allow apps to add their own metadata, I think we should
> add a way to add non-vcard metadata for now.
> 
> Something like:
> 
> {
>   "name": ...
>   ...
> 
>   "moz": {
>     "phoneticGivenName": string/number/blob,
>     "phoneticFamilyName": string/number/blob
>   }
> }
> 
> This way it's clear which properties are not vcard properties, and we can
> reuse existing infrastructure we have for sorting etc.
> 
> This means that to get all contacts sorted by phoneticFamilyName you'd do
> something like contacts.getAll({ sortBy: "moz.phoneticFamilyName"})
> 
> Does this sound ok to people?

Sounds good, but what happens if/when this eventually move to vCard? What's the migration story?
(In reply to Julien Wajsberg [:julienw] from comment #23)
> (In reply to Jonas Sicking (:sicking) from comment #21)
> > Because we constantly have the need to add things that are not in vcard, and
> > because the Contacts API is slated to eventually be phased out in favor of
> > DataStore which will allow apps to add their own metadata, I think we should
> > add a way to add non-vcard metadata for now.
> > 
> > Something like:
> > 
> > {
> >   "name": ...
> >   ...
> > 
> >   "moz": {
> >     "phoneticGivenName": string/number/blob,
> >     "phoneticFamilyName": string/number/blob
> >   }
> > }
> > 
> > This way it's clear which properties are not vcard properties, and we can
> > reuse existing infrastructure we have for sorting etc.
> > 
> > This means that to get all contacts sorted by phoneticFamilyName you'd do
> > something like contacts.getAll({ sortBy: "moz.phoneticFamilyName"})
> > 
> > Does this sound ok to people?
> 
> Sounds good

Sounds good to me to.

> but what happens if/when this eventually move to vCard? What's
> the migration story?

Do you expect that to happen before we phase out the MozContacts API?
(In reply to :Ehsan Akhgari (needinfo? me!) from comment #24)
> 
> > but what happens if/when this eventually move to vCard? What's
> > the migration story?
> 
> Do you expect that to happen before we phase out the MozContacts API?

I don't expect anything here ;)
We can also say that the migration story is moving to datastore.
Attached patch bug909224_patch2 (obsolete) — Splinter Review
I made a patch with reference to the comment 21.
Attachment #8373198 - Attachment is obsolete: true
Attachment #8373198 - Flags: review?(anygregor)
Attachment #8393972 - Flags: review?(anygregor)
Attachment #8393972 - Flags: review?(reuben.bmo)
Comment on attachment 8393972 [details] [diff] [review]
bug909224_patch2

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

Thank you for the patch, and thank you again for the tests :)

Looks good, but I want to see a new version with the comments addressed.

::: dom/contacts/fallback/ContactDB.jsm
@@ +920,5 @@
>          }
> +        if (field == "moz") {
> +          let val = aContact.properties[field].phoneticGivenName;
> +          if (val && val != "null") {
> +            contact.search[field].phoneticGivenName.push(val.toString().toLowerCase());

nit: Use |contact.moz.phoneticGivenName|… to make this more grep friendly.

@@ +924,5 @@
> +            contact.search[field].phoneticGivenName.push(val.toString().toLowerCase());
> +          }
> +          val = aContact.properties[field].phoneticFamilyName;
> +          if (val && val != "null") {
> +            contact.search[field].phoneticFamilyName.push(val.toString().toLowerCase());

Same here.

@@ +1147,5 @@
> +  getSortByParam: function CDB_getSortByParam(aFindOptions) {
> +    let sortBy;
> +    switch (aFindOptions.sortBy) {
> +      case "familyName":
> +        sortBy = [ "familyName", "givenName" ];

nit: Just return the values, so you can get rid of the breaks.

@@ +1270,4 @@
>  
> +          return sortOrder == "ascending" ? result : -result;
> +        });
> +      }

Duplicating this code is not OK. Add a helper function to get the property from a contact, something like

 function getSortProp(contact, prop) {
   if (prop.startsWith("moz.")) { /* do the right thing */ }
   // etc.
 }

@@ +1274,2 @@
>      }
> +

Whitespace.

::: dom/contacts/tests/shared.js
@@ +18,5 @@
>  };
>  
> +var defaultOptions2 = {
> +  sortBy: "moz.phoneticGivenName",
> +};

defaultOptions2 is only used once, inline it to make the test more easily readable.

@@ +30,5 @@
>    givenName: ["a"],
> +  moz: {
> +    phoneticFamilyName: [""],
> +    phoneticGivenName: [""],
> +  }

This shouldn't be needed after marking the attribute as optional.

Same for all the changes in this file adding an empty moz dict to the objects.

::: dom/contacts/tests/test_contacts_basics2.html
@@ +553,5 @@
>          honorificSuffix: [{foo: "bar"}],
> +        moz: {
> +          phoneticFamilyName: [""],
> +          phoneticGivenName: [""],
> +        },

This shouldn't be needed after marking the attribute as optional.

@@ +579,5 @@
>          honorificSuffix: ["[object Object]"],
> +        moz: {
> +          phoneticFamilyName: [""],
> +          phoneticGivenName: [""],
> +        },

This shouldn't be needed after marking the attribute as optional.

@@ +756,5 @@
>        nickname: [],
> +      moz: {
> +        phoneticFamilyName: [],
> +        phoneticGivenName: []
> +      },

This shouldn't be needed after marking the attribute as optional.

@@ +770,4 @@
>          c[prop].push(properties1[prop][i]);
>        }
>      }
> +    c['moz'] = properties1['moz'];

This shouldn't be needed after marking the attribute as optional.

@@ +900,5 @@
> +    }
> +    req.onerror = onFailure;
> +  },
> +  function () {
> +    ok(true, "Deleting database");

Use clearDatabase from shared.js.

@@ +936,5 @@
> +    }
> +    req.onerror = onFailure;
> +  },
> +  function () {
> +    ok(true, "Retrieving all contacts with limit 10 and sorted");

Changing the sortBy code shouldn't affect the filterLimit functionality. I don't think this test (and the setup for it above) is necessary. (But I could be convinced of the contrary :))

@@ +974,5 @@
> +    }
> +    req.onerror = onFailure;
> +  },
> +  function () {
> +    ok(true, "Deleting database");

Use clearDatabase from shared.js.

@@ +987,5 @@
> +    createResult1 = new mozContact(properties3);
> +    req = mozContacts.save(createResult1);
> +    req.onsuccess = function () {
> +      ok(createResult1.id, "The contact now has an ID.");
> +      checkStrArray(createResult1.name, properties3.name, "Same Name");

Did you mean to check the new fields here?

@@ +1002,5 @@
> +    }
> +    req.onerror = onFailure;
> +  },
> +  function () {
> +    ok(true, "Deleting database");

Use clearDatabase from shared.js.

@@ +1112,5 @@
> +    };
> +    req.onerror = onFailure;
> +  },
> +  function () {
> +    ok(true, "Deleting database");

Use clearDatabase from shared.js.

@@ +1152,5 @@
> +      checkContacts(c16, createResult1);
> +      next();
> +    };
> +    req.onerror = onFailure;
> +  },

The last three steps are setting up for a sorting test, but then you just delete the DB. Did you mean to include an additional sorting test here?

@@ +1154,5 @@
> +    };
> +    req.onerror = onFailure;
> +  },
> +  function () {
> +    ok(true, "Deleting database");

Use clearDatabase from shared.js.

@@ +1169,5 @@
> +    req = mozContacts.save(createResult2);
> +    req.onsuccess = function () {
> +      ok(createResult2.id, "The contact now has an ID.");
> +      sample_id2 = createResult2.id;
> +      next();

What is this testing? Did you mean to call
  checkContacts(createResult2, properties4);
here?

If so, this should probably be the first of these tests in the list, as failing here will likely break everything else that uses the new fields.

@@ +1174,5 @@
> +    };
> +    req.onerror = onFailure;
> +  },
> +  function () {
> +    ok(true, "Deleting database");

Use clearDatabase from shared.js.

::: dom/webidl/Contacts.webidl
@@ +96,4 @@
>    [Cached, Pure] attribute sequence<DOMString>?       jobTitle;
>    [Cached, Pure] attribute sequence<DOMString>?       note;
>    [Cached, Pure] attribute sequence<DOMString>?       key;
> +  [Cached, Pure] attribute MozExtensionContactField   moz;

This field should be optional.
Attachment #8393972 - Flags: review?(reuben.bmo)
Thank you for the review.

There are some questions about a review.

> > +        if (field == "moz") {
> > +          let val = aContact.properties[field].phoneticGivenName;
> > +          if (val && val != "null") {
> > +            contact.search[field].phoneticGivenName.push(val.toString().toLowerCase());
> 
> nit: Use |contact.moz.phoneticGivenName|… to make this more grep friendly.
Is it better to shorten like |contact.phoneticGivenName|?

> >    [Cached, Pure] attribute sequence<DOMString>?       jobTitle;
> >    [Cached, Pure] attribute sequence<DOMString>?       note;
> >    [Cached, Pure] attribute sequence<DOMString>?       key;
> > +  [Cached, Pure] attribute MozExtensionContactField   moz;
> 
> This field should be optional.
Should I define it like other properties as follows?

[Cached, Pure] attribute sequence<MozExtensionContactField>?   moz;

Thanks!
(In reply to Teiichiro Fukuda from comment #28)
> > > +            contact.search[field].phoneticGivenName.push(val.toString().toLowerCase());
> > 
> > nit: Use |contact.moz.phoneticGivenName|… to make this more grep friendly.
> Is it better to shorten like |contact.phoneticGivenName|?

Sorry, I meant |contact.search.moz.phoneticGivenName|. We want to keep the stuff that is created by ContactDB inside |contact.search|.
 
> > >    [Cached, Pure] attribute sequence<DOMString>?       jobTitle;
> > >    [Cached, Pure] attribute sequence<DOMString>?       note;
> > >    [Cached, Pure] attribute sequence<DOMString>?       key;
> > > +  [Cached, Pure] attribute MozExtensionContactField   moz;
> > 
> > This field should be optional.
> Should I define it like other properties as follows?
> 
> [Cached, Pure] attribute sequence<MozExtensionContactField>?   moz;

No, just

  [Cached, Pure] attribute MozExtensionContactField?  moz;
>No, just
>  [Cached, Pure] attribute MozExtensionContactField?  moz;
As a result of a build in the above definition, the following error has occurred.

ContactProperties be modified as follows.

dictionary ContactProperties {
          ・
          ・
          ・
   sequence<DOMString>?           jobTitle;
   sequence<DOMString>?           note;
   sequence<DOMString>?           key;
   MozExtensionContactField?       moz;
 };

Error:
WebIDL.WebIDLError: error: Dictionary ContactProperties has member with nullable dictionary type, 
/home/B2G/gecko/dom/webidl/Contacts.webidl line 61:33
  MozExtensionContactField?      moz;
                                 ^
Thank you for the well-chosen phonetic property names of phoneticGivenName and phoneticFamilyName.

I have reviewed the proposal to VCARDDAV[1], noted no objections, and have added them directly to the mozContact interface adjacent to their parallel fields of givenName and familyName.

https://wiki.mozilla.org/WebAPI/ContactsAPI#mozContact

As we have made *very* few additions/extensions to this interface in several years, and we don't expect to make a lot (if any) more, there is no need for an extra layer "MozExtensionContactField", as it will unlikely get anything else before we presumably phase out the MozContacts API.

[1] http://tools.ietf.org/html/draft-fukuda-vcarddav-phonetic-transcription-02
Note that this draft expires today ("Expires: March 24, 2014").
Teiichiro Fukuda, you may want to submit a new draft to VCARDDAV, even if just to update the dates and extend the expiration by another 6 months. 
Thank you very much for your diligent work on this.
Flags: needinfo?(tantek)
Thank you for adding a phonetic property.
I correct the patch.
Attached patch bug909224_patch (obsolete) — Splinter Review
I made a patch newly.
Please review it.
Attachment #8393972 - Attachment is obsolete: true
Attachment #8393972 - Flags: review?(anygregor)
Attachment #8397699 - Flags: review?(reuben.bmo)
Attachment #8397699 - Flags: review?(reuben.bmo) → review+
Keywords: checkin-needed
Attached patch bug909224_patch (obsolete) — Splinter Review
I have merged again in the source code of the latest mozContacts.
Attachment #8397699 - Attachment is obsolete: true
Attachment #8399846 - Flags: review+
Keywords: checkin-needed
Let's see if this passes on Try first:

https://tbpl.mozilla.org/?tree=Try&rev=d4c7e87978e2
Keywords: checkin-needed
Looks like the same failures are there, but only on B2G Desktop? Could this have something to do with the locales that are available in the test machines? Can we use characters from the Latin alphabet instead in those tests?
It seems that the error has occurred since the Japanese hiragana is probably used as test data.
I change a hiragana into Unicode and make a patch.
Attached patch bug909224_new_patch (obsolete) — Splinter Review
I changed test data.
Please review it again.
Attachment #8399846 - Attachment is obsolete: true
Attachment #8401095 - Flags: review?(reuben.bmo)
That didn't fix the failures. It looks like "あ".localeCompare("い") in B2G desktop is not returning the value your tests expect. We don't treat phonetic{Family,Given}Name differently in the sorting code, can we just use ASCII characters for now and then figure out why B2G desktop is behaving differently in a different bug?
Comment on attachment 8401095 [details] [diff] [review]
bug909224_new_patch

Comment 41 was supposed to go here.
Attachment #8401095 - Flags: review?(reuben.bmo)
Because my test machine is set for Japanese locale, I think that the locale of the test machine has a problem.
About test data, I think that I will unify into the alphabet like other test data.
I changed the test data to the alphabet.
Attachment #8401095 - Attachment is obsolete: true
Attachment #8402564 - Flags: review?(reuben.bmo)
Attachment #8402564 - Flags: review?(reuben.bmo) → review+
https://hg.mozilla.org/mozilla-central/rev/fa4b170c71ff
Status: NEW → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
Whiteboard: [m+]
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.