Last Comment Bug 769245 - Contacts API: Add ContactEmail Type
: Contacts API: Add ContactEmail Type
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM: Device Interfaces (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: mozilla16
Assigned To: Gregor Wagner [:gwagner]
:
Mentors:
https://wiki.mozilla.org/WebAPI/Conta...
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-06-28 06:42 PDT by Gregor Wagner [:gwagner]
Modified: 2012-07-13 02:51 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (14.05 KB, patch)
2012-06-28 07:55 PDT, Gregor Wagner [:gwagner]
no flags Details | Diff | Review
patch (20.27 KB, patch)
2012-06-28 10:40 PDT, Gregor Wagner [:gwagner]
no flags Details | Diff | Review
patch (20.27 KB, patch)
2012-06-28 10:44 PDT, Gregor Wagner [:gwagner]
jonas: review+
Details | Diff | Review

Description Gregor Wagner [:gwagner] 2012-06-28 06:42:07 PDT
Currently we have a DOMString[] for email addresses.
We want to add a ContactEmail type that has a type for home, work, etc and the actual email address.

This is in the v1 requirements and in the UX design.
Comment 1 Gregor Wagner [:gwagner] 2012-06-28 07:55:37 PDT
Created attachment 637512 [details] [diff] [review]
Patch

first version
Comment 2 Gregor Wagner [:gwagner] 2012-06-28 10:40:21 PDT
Created attachment 637583 [details] [diff] [review]
patch

few more fixes
Comment 3 Gregor Wagner [:gwagner] 2012-06-28 10:44:07 PDT
Created attachment 637584 [details] [diff] [review]
patch

and without debug statements
Comment 4 Jonas Sicking (:sicking) PTO Until July 5th 2012-06-28 17:47:56 PDT
Comment on attachment 637584 [details] [diff] [review]
patch

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

::: dom/contacts/ContactManager.js
@@ +161,5 @@
>          return new Array(aField);
> +      } else if (Array.isArray(aField)) {
> +        return aField;
> +      } else if (aField != null) {
> +        return [aField.toString()];

You're handling strings twice here. Just do something like:

if (Array.isArray(aField)) {
  ..convert each member to a string..
}
else if (aField != null) {
  return [String(aField)];
}

::: dom/contacts/fallback/ContactDB.jsm
@@ +116,5 @@
> +        objectStore.openCursor().onsuccess = function(event) {  
> +          let cursor = event.target.result;
> +          if (cursor) {
> +            debug("upgrade email1: " + JSON.stringify(cursor.value));
> +            for (let address in cursor.value.properties.email) {

It's a little scary to be iterating this array this way as you are modifying it. I'm not sure if JS guarantees that that will work.

Possibly doing something like

cursor.value.properties.email =
  cursor.value.properties.email.map(function(address) { return { address: address }; });

would be safer.

@@ +119,5 @@
> +            debug("upgrade email1: " + JSON.stringify(cursor.value));
> +            for (let address in cursor.value.properties.email) {
> +              cursor.value.properties.email[address] = {address: address};
> +            }
> +            cursor.update(cursor.value);

Don't you need to also fill search.email with data? Or is that already the case?
Comment 5 Gregor Wagner [:gwagner] 2012-07-03 09:35:55 PDT
(In reply to Jonas Sicking (:sicking) from comment #4)
> @@ +119,5 @@
> > +            debug("upgrade email1: " + JSON.stringify(cursor.value));
> > +            for (let address in cursor.value.properties.email) {
> > +              cursor.value.properties.email[address] = {address: address};
> > +            }
> > +            cursor.update(cursor.value);
> 
> Don't you need to also fill search.email with data? Or is that already the
> case?

That's already the case.

https://hg.mozilla.org/integration/mozilla-inbound/rev/e29885f8f290
Comment 6 Ed Morley [:emorley] 2012-07-03 10:16:45 PDT
Backed out for M3 failures:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=e29885f8f290

{
12207 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/tests/mochitest/general/test_interfaces.html | Unexpected interface name in global scope: ContactEmail
}

https://hg.mozilla.org/integration/mozilla-inbound/rev/72a742ed3d20
Comment 8 Ryan VanderMeulen [:RyanVM] 2012-07-03 16:06:55 PDT
https://hg.mozilla.org/mozilla-central/rev/c54c4e2f8058
Comment 9 Tantek Çelik 2012-07-03 16:35:45 PDT
https://wiki.mozilla.org/WebAPI/ContactsAPI updated with interface ContactEmail with a DOMString[] type to handle the listed requirements from https://wiki.mozilla.org/Gaia/Contacts#Gaia_v1 in particular:
"Email address - home (primary) and offers the ability to add work and other email addresses"

"(primary)" or favorite/preferred email address is indicated as *one* of the types of an email in vCard/hCard as the string "PREF".

home is "home", and work is "work".

Similarly for interface ContactTelephone and ContactAddress.

This usage of "PREF" for the 'favorite' or 'preferred' phone/email/address of a contact has been verified by exporting vCards from various mobile address books (Android, iOS), and noting the *array* of types that accompanies the tel/email/adr fields, including the string "PREF" for favorite/preferred, and as noted, additional values in the type array for "work", "home", "fax" etc.

Thus these updated interfaces represent a mapping that is compatible both with vCard and interop with the exports of existing implementations, which is a significant consideration for us, as users of B2G phones may be moving data from such devices and the better we preserve things like favorites/preferences and they "just work", the better experience we provide to our users.
Comment 10 alberto.pastor 2012-07-13 02:51:25 PDT
I see in the wiki that tel and email have {type, value} instead {type, number} and {type, address}.

Is that wiki wrong or the code is going to change to use that value field?

Note You need to log in before you can comment on or make changes to this bug.