909224 - Want to add the phonetic name of the name to mozContacts

Status: RESOLVED FIXED

(Core Graveyard :: DOM: Contacts, defect)

Description

[Teiichiro Fukuda]

User Agent: Mozilla/5.0 (Windows NT 5.1) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/28.0.1500.95 Safari/537.36

Comment 1

[Teiichiro Fukuda]

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.

Comment 2

[Teiichiro Fukuda]

Attachment: patch

Comment 3

[Tantek Çelik]

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.

Comment 4

[Tantek Çelik]

(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

Comment 5

[Teiichiro Fukuda]

(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.

Comment 6

[Teiichiro Fukuda]

(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

Comment 7

[Tantek Çelik]

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.

Comment 8

[Teiichiro Fukuda]

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

Comment 9

[Teiichiro Fukuda]

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

Comment 10

[Teiichiro Fukuda]

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

Comment 11

[Gregor Wagner [:gwagner]]

Is there any update here?

Comment 12

[Teiichiro Fukuda]

There is no schedule of update.

Comment 13

[Teiichiro Fukuda]

I have created a Bugzilla[1] related to this matter.

[1]:https://bugzilla.mozilla.org/show_bug.cgi?id=936386

Comment 14

[Julien Wajsberg [:julienw]]

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!

Comment 15

[Teiichiro Fukuda]

Attachment: bug909224_patch

I made a patch in the latest environment.
Review Is it possible?

Comment 16

[Teiichiro Fukuda]

Hi Gregor,

Should I remake a patch?

Thanks.

Comment 17

[Reuben Morais [:reuben]]

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?

Comment 18

[Gregor Wagner [:gwagner]]

Did the API discussion on the mailing list come to any conclusions?

Comment 19

[Teiichiro Fukuda]

Since the reply about use of "MEDIA-TYPE" is not got from the area director of IETF, a discussion is under continuation.

Comment 20

[Teiichiro Fukuda]

Is the addition of a property impossible, if the conclusion of a discussion is reached and vCard specification is not changed?

Comment 21

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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?

Comment 22

[Teiichiro Fukuda]

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

Comment 23

[Julien Wajsberg [:julienw]]

(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?

Comment 24

[(no longer active)]

(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?

Comment 25

[Julien Wajsberg [:julienw]]

(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.

Comment 26

[Teiichiro Fukuda]

Attachment: bug909224_patch2

I made a patch with reference to the comment 21.

Comment 27

[Reuben Morais [:reuben]]

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.

Comment 28

[Teiichiro Fukuda]

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!

Comment 29

[Reuben Morais [:reuben]]

(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;

Comment 30

[Teiichiro Fukuda]

>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;
                                 ^

Comment 31

[Tantek Çelik]

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.

Comment 32

[Teiichiro Fukuda]

Thank you for adding a phonetic property.
I correct the patch.

Comment 33

[Teiichiro Fukuda]

Attachment: bug909224_patch

I made a patch newly.
Please review it.

Comment 34

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/aa90cb0be515

Comment 35

[Carsten Book [:Tomcat]]

sorry had to backout this patch for testfailures like https://tbpl.mozilla.org/php/getParsedLog.php?id=36988181&tree=Mozilla-Inbound

Comment 36

[Teiichiro Fukuda]

Attachment: bug909224_patch

I have merged again in the source code of the latest mozContacts.

Comment 37

[Reuben Morais [:reuben]]

Let's see if this passes on Try first:

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

Comment 38

[Reuben Morais [:reuben]]

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?

Comment 39

[Teiichiro Fukuda]

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.

Comment 40

[Teiichiro Fukuda]

Attachment: bug909224_new_patch

I changed test data.
Please review it again.

Comment 41

[Reuben Morais [:reuben]]

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 42

[Reuben Morais [:reuben]]

Comment on attachment 8401095 [details] [diff] [review]
bug909224_new_patch

Comment 41 was supposed to go here.

Comment 43

[Teiichiro Fukuda]

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.

Comment 44

[Teiichiro Fukuda]

Attachment: bug909224_new_patch2

I changed the test data to the alphabet.

Comment 45

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fa4b170c71ff

Comment 46

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/fa4b170c71ff