Closed Bug 857730 Opened 12 years ago Closed 11 years ago

Implement Contacts API for Android

Categories

(Firefox for Android Graveyard :: General, defect)

Product:
Firefox for Android Graveyard
Component:
General
Platform:
ARM
Android
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox25 disabled, fennecNightly+)

Status:
RESOLVED FIXED
Milestone:
Firefox 25
Tracking Flags:
Tracking Status
firefox25 --- disabled
fennec Nightly+ ---

People

(Reporter: elan, Assigned: stully)

References

Details

(Keywords: dev-doc-needed, feature, user-doc-needed)

Attachments

(5 files, 26 obsolete files)

3/5 Add contacts API Javascript to Android
17.90 KB, patch
cpeterson
: review+
Details | Diff | Splinter Review
2/5 Add permissions doorhanger
5.95 KB, patch
cpeterson
: review+
Details | Diff | Splinter Review
1/5 Add contacts permissions to manifest
1.31 KB, patch
cpeterson
: review+
Details | Diff | Splinter Review
4/5 Integrate contacts API Javascript with Java and Android contact API
92.94 KB, patch
cpeterson
: review+
Details | Diff | Splinter Review
5/5 - Disable contact unit tests on Android for non-nightly channels
924 bytes, patch
cpeterson
: review+
Details | Diff | Splinter Review
Assignee: nobody → stully
Add contacts API Javascript to Android
Attachment #757722 - Flags: feedback?(cpeterson)
Temporaily disable security checks until permission prompts are hashed out
Attachment #757724 - Flags: feedback?(cpeterson)
Add read and write contacts Android permissions to Android manifest file
Attachment #757725 - Flags: feedback?(cpeterson)
Attachment #757725 - Attachment description: Add read and write contacts Android permissions to Android manifest file → 3/5 Add read and write contacts Android permissions to Android manifest file
Add default value for "key" field The API calls for a "key" field, but it is not present in ContactManager.js. This causes problems on the Java end where is it expecting a field called "key" in the JSON string. I attempted to add it to ContactManager.js, but I'm missing something because the field is still not showing up in the JSON string.
Attachment #757726 - Flags: feedback?(cpeterson)
Integrate contacts API Javascript with Java and Android contact API The save function is mostly complete except for the photo field. It also needs to check for existing contacts before saving them.
Attachment #757727 - Flags: feedback?(cpeterson)
Comment on attachment 757724 [details] [diff] [review] 2/5 Temporaily disable security checks until permission prompts are hashed out Review of attachment 757724 [details] [diff] [review]: ----------------------------------------------------------------- We should determine why ContactManager.js relies on PermissionPromptHelper.jsm and how to use door hanger permission popups.
Attachment #757724 - Flags: feedback?(cpeterson) → feedback+
Comment on attachment 757725 [details] [diff] [review] 3/5 Add read and write contacts Android permissions to Android manifest file Review of attachment 757725 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/AndroidManifest.xml.in @@ +48,5 @@ > + <uses-permission android:name="android.permission.READ_CONTACTS"/> > + <uses-permission android:name="android.permission.WRITE_CONTACTS"/> > + <uses-permission android:name="android.permission.GET_ACCOUNTS"/> > + > + Use only one blank line here.
Attachment #757725 - Flags: feedback?(cpeterson) → feedback+
Comment on attachment 757726 [details] [diff] [review] 4/5 Add default value for "key" field Review of attachment 757726 [details] [diff] [review]: ----------------------------------------------------------------- LGTM, but this issue is not specific to Android, so I think it should be addressed by existing bug 807688. That bug has been quiet for a couple months, so feel free to add a comment asking ddahl if he's still working on it. :) You might want to also ask whether the field should be called `key` (as per Tantek's Contacts API spec) or `publicKey` (in ddahl's patch).
Attachment #757726 - Flags: feedback?(cpeterson) → feedback+
Comment on attachment 757722 [details] [diff] [review] 1/5 Add contacts API Javascript to Android Review of attachment 757722 [details] [diff] [review]: ----------------------------------------------------------------- Looking good. ::: mobile/android/modules/AndroidContactService.jsm @@ +39,5 @@ > + > + var idbManager = Components.classes["@mozilla.org/dom/indexeddb/manager;1"].getService(Ci.nsIIndexedDatabaseManager); > + idbManager.initWindowless(myGlobal); > + this._db = new ContactDB(myGlobal); > + this._db.init(myGlobal); I don't think we want any of this idbManager, this._db, or ContactDB code. It's only used for B2G's Contacts database, but we're using Android's. @@ +44,5 @@ > + > + Services.obs.addObserver(this, "Contacts:Find:Return:OK", false); > + Services.obs.addObserver(this, "Contact:Save:Return:OK", false); > + Services.obs.addObserver(this, "Contact:Remove:Return:OK", false); > + Services.obs.addObserver(this, "Contacts:Clear:Return:OK", false); These repetitive addObserver() calls should probably use the arrayOfStrings.forEach() pattern used above for "Contacts:" messages array. @@ +55,5 @@ > + Services.obs.addObserver(this, "Contacts:GetAll:Next", false); > + Services.obs.addObserver(this, "profile-before-change", false); > + }, > + > + _sendMessageToJava: function (aMsg) { Remove whitespace between `function` keyword and paren for anonymous functions. @@ +62,5 @@ > + > + observe: function(aSubject, aTopic, aData) { > + switch (aTopic) { > + case "Contacts:Find:Return:OK": { > + let aMessage = this._requestMessages[aData]; You should rename `aMessage` to just `message`. The 'a' prefix is Hungarian notation for function arguments. @@ +70,5 @@ > + > + } case "Contacts:Find:Return:KO": { > + let aMessage = this._requestMessages[aData]; > + aMessage.target.sendAsyncMessage("Contacts:Find:Return:KO", {requestID: aMessage.data.requestID, errorMsg: aErrorMsg}); > + delete this._requestMessages[aData]; I think you can extract this code into helper function that looks something like: sendReturnMessage: function(aTopic, aData, aResult) { let aMessage = this._requestMessages[aData]; aMessage.target.sendAsyncMessage(aTopic, aResult); delete this._requestMessages[aData]; } @@ +149,5 @@ > + let msg = aMessage.data; > + > + switch (aMessage.name) { > + case "Contacts:Find": > + if (this.findContacts(aMessage) == false) { Comparing an expression for equality to `true` or `false` is usually a bad idea, especially in JavaScript because of the language's many "truthy" and "falsy" values. You could just use `if (!this.findContacts(...`. That said, I think we can safely ignore the return value of `this.findContacts(aMessage)`. @@ +150,5 @@ > + > + switch (aMessage.name) { > + case "Contacts:Find": > + if (this.findContacts(aMessage) == false) { > + return null; Does returning null from receiveMessage() actually do anything? I know fallback/ContactService.jsm's receiveMessage() returns null in some code paths, but it also returns nothing in other code paths. @@ +221,5 @@ > + } > + > + this._sendMessageToJava({ > + type: "Contacts:Find", > + message: aMessage.data We have so many fields and variables called "message", I think we should rename this field to `data` because we are assigning it aMessage.data (not aMessage). @@ +222,5 @@ > + > + this._sendMessageToJava({ > + type: "Contacts:Find", > + message: aMessage.data > + }); Can you squeeze this _sendMessageToJava() parameters onto just one line of code (that is <= 80 characters)? @@ +280,5 @@ > + // Send the request to the Java ContactService for interfacing with the Android API > + this._sendMessageToJava({ > + type: "Contact:Save", > + message: aMessage.data > + }); Can you squeeze this _sendMessageToJava() parameters onto just one line of code (that is <= 80 characters)?
Attachment #757722 - Flags: feedback?(cpeterson) → feedback+
Blocks: 862853
Comment on attachment 757727 [details] [diff] [review] 5/5 Integrate contacts API Javascript with Java and Android contact API Review of attachment 757727 [details] [diff] [review]: ----------------------------------------------------------------- Looking good. This list of suggestions looks long, but they are mostly minor issues. <:) ::: mobile/android/base/ContactService.java @@ +6,5 @@ > + > +import org.mozilla.gecko.gfx.Layer; > +import org.mozilla.gecko.gfx.LayerView; > +import org.mozilla.gecko.util.EventDispatcher; > +import org.mozilla.gecko.util.FloatUtils; The FloatUtils import is unused and can be removed. @@ +21,5 @@ > +import android.database.Cursor; > +import android.provider.ContactsContract; > + > +import android.accounts.Account; > +import android.accounts.AccountManager; Please remove blank lines between android.* imports and sort them alphabetically. @@ +28,5 @@ > + > +import android.os.Build; > +import android.os.RemoteException; > +import java.util.ArrayList; > +import android.util.Log; java.* imports should be listed after android.* imports and be separated from them by a blank line. Our Java coding style is to organize imports by blocks separated by empty line: org.mozilla.*, com.*, net.*, org.*, android.*, then java.*: https://developer.mozilla.org/en-US/docs/Developer_Guide/Coding_Style#Java_practices @@ +39,5 @@ > + private final static int GROUP_ACCOUNT_TYPE = 1; > + private final static int GROUP_ID = 2; > + private final static int GROUP_TITLE = 3; > + private final static int GROUP_AUTO_ADD = 4; > + private final static String PRE_HONEYCOMB_DEFULT_GROUP = "System Group: My Contacts"; 1. s/DEFULT/DEFAULT/ 2. Please add a comment explaining why this magic group name is need on pre-Honeycomb and not recent Android versions. @@ +45,5 @@ > + private final static String MIMETYPE_SEX = "org.mozilla.gecko/sex"; > + private final static String MIMETYPE_GENDER_IDENTITY = "org.mozilla.gecko/gender_identity"; > + private final static String MIMETYPE_KEY = "org.mozilla.gecko/key"; > + > + private final boolean debug = true; `debug` should be static and renamed `DEBUG`. Our Java coding style is that instance variables have an 'm' prefix, static class variables have an 's' prefix, and constants are all uppercase. This is just a rule of thumb and some teams (e.g. the Mozilla Services team) use a different style in their files. @@ +60,5 @@ > + > + ContactService(EventDispatcher eventDispatcher, GeckoApp activity) { > + mEventDispatcher = eventDispatcher; > + mActivity = activity; > + mWasInit = false; You don't need to initialize instance variables to false/0/null values in the constructor because that is the default value. @@ +66,5 @@ > + registerEventListener("Contacts:Find"); > + registerEventListener("Contacts:GetAll"); > + registerEventListener("Contacts:Clear"); > + registerEventListener("Contact:Save"); > + registerEventListener("Contact:Remove"); Please sort these alphabetically. @@ +82,5 @@ > + unregisterEventListener("Contacts:Find"); > + unregisterEventListener("Contacts:GetAll"); > + unregisterEventListener("Contacts:Clear"); > + unregisterEventListener("Contact:Save"); > + unregisterEventListener("Contact:Remove"); Please sort these alphabetically. @@ +95,5 @@ > + init(); > + } > + > + try { > + if (debug) Log.d(LOGTAG, "Event: " + event + "\nMessage: " + message.toString(3)); Gecko coding style forbids single-line if statements. Wrap the Log.d() onto the next line and add parens. @@ +98,5 @@ > + try { > + if (debug) Log.d(LOGTAG, "Event: " + event + "\nMessage: " + message.toString(3)); > + > + final String requestID = message.getJSONObject("message").getString("requestID"); > + final JSONObject contactOptions = message.getJSONObject("message") 1. "message" should be changed to "data", if you implement my suggestions for AndroidContactService.jsm. 2. Save the message.getJSONObject("data") return value in a local variable to avoid extra calls to getJSONObject(). This will also reduce the line length. @@ +111,5 @@ > + } else if ("Contact:Save".equals(event)) { > + saveContact(contactOptions, requestID); > + } else if ("Contact:Remove".equals(event)) { > + removeContact(contactOptions, requestID); > + } Please add an else block that throws an IllegalArgumentException("Unexpected event=" + event) because this would indicate a bug in our code. @@ +113,5 @@ > + } else if ("Contact:Remove".equals(event)) { > + removeContact(contactOptions, requestID); > + } > + } catch (JSONException e) { > + Log.e(LOGTAG, "JSON exception", e); I think this catch block should throw an IllegalArgumentException("message=" + message.toString()) because any JSONExceptions we catch here indicate a bug in our code. @@ +137,5 @@ > + cursor.close();*/ > + } > + > + private void getAllContacts(final JSONObject contactOptions, final String requestID) { > + // TOOD s/TOOD/TODO/ @@ +150,5 @@ > + try { > + JSONObject contactProperties = contactOptions.getJSONObject("contact") > + .getJSONObject("properties"); > + > + ArrayList<ContentValues> newContactValues = new ArrayList<ContentValues>(); Declare newContactValues' type as just List<ContentValues>. This will hide the ArrayList implementation details and make the line shorter. @@ +162,5 @@ > + // Create all the values that will be inserted into the new contact > + // Display names > + getDisplayNamesValues(newContactValues, contactProperties.getJSONArray("name")); > + // Given names > + getGivenNamesValues(newContactValues, contactProperties.getJSONArray("givenName")); I think the comments between each call to the get*NamesValues() methods makes the code harder to read. Plus the method names are already self-explanatory. @@ +202,5 @@ > + getGenderIdentityValues(newContactValues, contactProperties.getString("genderIdentity")); > + // Key -- commented out until key field is added in the javascript files > + //getKeysValues(newContactValues, contactProperties.getJSONArray("key")); > + > + ArrayList<ContentProviderOperation> newContact = new ArrayList<ContentProviderOperation>(); Use List<ContentProviderOperations>. @@ +224,5 @@ > + wasSuccessful = true; > + } catch (RemoteException e) { > + Log.e(LOGTAG, "RemoteException", e); > + } catch (OperationApplicationException e) { > + Log.e(LOGTAG, "OperationApplicationException", e); Do these exceptions indicate a bug in our code? If so, then we should rethrow some exception such as IllegalStateException. @@ +227,5 @@ > + } catch (OperationApplicationException e) { > + Log.e(LOGTAG, "OperationApplicationException", e); > + } > + } catch (JSONException e) { > + Log.e(LOGTAG, "JSON exception", e); Would a JSONException here indicate a bug in our code? If so, then we should rethrow an IllegalArgumentException. @@ +229,5 @@ > + } > + } catch (JSONException e) { > + Log.e(LOGTAG, "JSON exception", e); > + } finally { > + if (debug) Log.d(LOGTAG, "Sending return message: " + (wasSuccessful ? "OK" : "KO")); No single-line if. @@ +234,5 @@ > + sendCallbackToJavascript("Contact:Save:Return:" + (wasSuccessful ? "OK" : "KO"), requestID); > + } > + } > + > + private void getDisplayNamesValues(ArrayList<ContentValues> newContactValues, final JSONArray displayNames) throws JSONException { 1. Use List<ContentValues>. 2. I think it is customary for "out" parameters to be the last parameter, so this method signature should be something like `getDisplayNamesValues(JSONArray displayNames, List<ContentValues> newContentValues)`. @@ +235,5 @@ > + } > + } > + > + private void getDisplayNamesValues(ArrayList<ContentValues> newContactValues, final JSONArray displayNames) throws JSONException { > + for (int i=0; i<displayNames.length(); i++) { Use whitespace around the = and < operators here and in the other get*NamesValues() methods below. @@ +248,5 @@ > + for (int i=0; i<givenNames.length(); i++) { > + ContentValues contentValues = new ContentValues(); > + contentValues.put(ContactsContract.Data.MIMETYPE, ContactsContract.CommonDataKinds.StructuredName.CONTENT_ITEM_TYPE); > + contentValues.put(ContactsContract.CommonDataKinds.StructuredName.GIVEN_NAME, givenNames.getString(i)); > + newContactValues.add(contentValues); I think you could extract some of the common code from these get*NamesValues() methods into some helper methods. For example, something like: private static ContentValues createContentValue(int mimeType, int dataType, String dataValue) { ContentValues contentValues = new ContentValues(); contentValues.put(ContactsContract.Data.MIMETYPE, mimeType_ADDITIONAL_NAME); contentValues.put(dataType, dataValue); return contactValues; } and: private static void addContentValues(int mimeType, int dataType, JSONArray dataValues, List<ContentValues> newContentValues) { for (int i=0; i<familyNames.length(); i++) { ContentValues contentValues = new ContentValues(); contentValues.put(ContactsContract.Data.MIMETYPE, mimeType_ADDITIONAL_NAME); contentValues.put(dataType, dataValue); newContactValues.add(contentValues); } } @@ +256,5 @@ > + private void getFamilyNamesValues(ArrayList<ContentValues> newContactValues, final JSONArray familyNames) throws JSONException { > + for (int i=0; i<familyNames.length(); i++) { > + ContentValues contentValues = new ContentValues(); > + contentValues.put(ContactsContract.Data.MIMETYPE, ContactsContract.CommonDataKinds.StructuredName.CONTENT_ITEM_TYPE); > + contentValues.put(ContactsContract.CommonDataKinds.StructuredName.FAMILY_NAME, familyNames.getString(i)); Can you add more imports to reduce the repeated code in these methods? For example, can you `import android.provider.ContactsContract.CommonDataKinds.StructuredName` and then just reference StructuredName.CONTENT_ITEM_TYPE here? @@ +305,5 @@ > + > + JSONArray addressTypes = address.getJSONArray("type"); > + for (int j=0; j<addressTypes.length(); j++) { > + // Translate the website type string to an integer constant provided by the ContactsContract API > + int addressType = getAddressType(addressTypes.getString(j)); Save the addressTypes.getString(j) return value in a local variable so you don't need to call addressTypes.getString(j) again below. @@ +566,5 @@ > + Log.w(LOGTAG, "No accounts available"); > + } > + > + // For now, just grab the first account > + mAccountName = accounts[0].name; If getAccounts() returns a zero-length array, you should return early because this code will crash. @@ +582,5 @@ > + String groupAccountType = cursor.getString(GROUP_ACCOUNT_TYPE); > + > + // Check if the account name and type for the group match the account name and type of > + // the account we're working with > + if (groupAccountName.equals(mAccountName) && groupAccountType.equals(mAccountType)) { If you move the groupAccountName.equals(mAccountName) check before cursor.getString(GROUP_ACCOUNT_TYPE), you can continue the loop early (to avoid the extra getString() call) and avoid the extra indentation of the rest of the loop. @@ +584,5 @@ > + // Check if the account name and type for the group match the account name and type of > + // the account we're working with > + if (groupAccountName.equals(mAccountName) && groupAccountType.equals(mAccountType)) { > + // For all honeycomb and up, the default group is the first one which has the AUTO_ADD flag set. > + // For everything below honeycomb, use the default "System Group: My Contacts" group I think getDefaultGroupId() will be clearer if you extract the pre- and post-Honeycomb group checks into an isAutoAddGroup(cursor) helper method. @@ +585,5 @@ > + // the account we're working with > + if (groupAccountName.equals(mAccountName) && groupAccountType.equals(mAccountType)) { > + // For all honeycomb and up, the default group is the first one which has the AUTO_ADD flag set. > + // For everything below honeycomb, use the default "System Group: My Contacts" group > + if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.HONEYCOMB) { Our coding style is to hard code API Level numbers (e.g. 11) instead of the VERSION_CODES.HONEYCOMB constants. Magic numbers are usually a bad design "smell", but our reasoning is: * API Level numbers are more precise, reducing confusion about names, e.g. HONEYCOMB vs HONEYCOMB_MR1 vs HONEYCOMB_MR2. * Android's API documentation lists API Levels, not names, so comparing code versions checks to the documentation is easier. * The numbers are shorter than the long names. @@ +589,5 @@ > + if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.HONEYCOMB) { > + // The default group is the one which new contacts are assigned to by default (AUTO_ADD) > + if (!cursor.isNull(GROUP_AUTO_ADD) && cursor.getInt(GROUP_AUTO_ADD) != 0) { > + mGroupTitle = cursor.getString(GROUP_TITLE); > + mGroupId = cursor.getLong(GROUP_ID); Can we break out of the while loop when we find the first AUTO_ADD group? This code is going to continue checking all the groups for AUTO_ADD, so mGroupId will be the ID of the *last* AUTO_ADD group. @@ +592,5 @@ > + mGroupTitle = cursor.getString(GROUP_TITLE); > + mGroupId = cursor.getLong(GROUP_ID); > + } > + } else { > + if (PRE_HONEYCOMB_DEFULT_GROUP.equals(cursor.getString(GROUP_TITLE))) { You should just collapse this `} else { if` into `} else if` on one line to avoid unnecessary indentation. @@ +644,5 @@ > + > + private long createGroup(String groupName) { > + if (debug) Log.d(LOGTAG, "Creating group: " + groupName); > + > + ArrayList<ContentProviderOperation> newGroup = new ArrayList<ContentProviderOperation>(); Declare newGroup as List<ContentProviderOperation>. @@ +658,5 @@ > + mContentResolver.applyBatch(ContactsContract.AUTHORITY, newGroup); > + } catch (RemoteException e) { > + Log.e(LOGTAG, "RemoteException", e); > + } catch (OperationApplicationException e) { > + Log.e(LOGTAG, "OperationApplicationException", e); Do these exception indicate a bug in our code? If so, then we should rethrow an appropriate subclass of RuntimeException. @@ +665,5 @@ > + // Return the ID of the newly created group > + return getGroupId(groupName); > + } > + > + private boolean isPreferred(JSONObject contactField) { isPreferred() and many of the getter methods could be made static. @@ +670,5 @@ > + // Check if the JSON object has the field pref set to 1 > + // If not, or if it doesn't exist (throws a JSON expcetion), return false, otherwise true > + try { > + return "1".equals(contactField.getString("pref")); > + } catch (JSONException e) { I think you can avoid dealing with JSONException here by using something like: String pref = contactField.optString("pref", null); return "1".equals(pref); @@ +686,5 @@ > + } > + } > + > + private int getPhoneType(String phoneType) { > + if ("home".equalsIgnoreCase(phoneType)) { To avoid the performance overhead and long method name of equalsIgnoreCase(), create a local variable (or just overwrite the phoneType parameter) with a phoneType.toLowercase() string. @@ +688,5 @@ > + > + private int getPhoneType(String phoneType) { > + if ("home".equalsIgnoreCase(phoneType)) { > + return ContactsContract.CommonDataKinds.Phone.TYPE_HOME; > + } else if ("mobile".equalsIgnoreCase(phoneType)) { You can remove all these else keywords because the preceding if blocks always return.
Attachment #757727 - Flags: feedback?(cpeterson) → feedback+
Add read and write contacts Android permissions to Android manifest file Revised
Attachment #757725 - Attachment is obsolete: true
Attachment #758333 - Flags: feedback?(cpeterson)
Add contacts API Javascript to Android Revised. Note that I'm slowly replacing the old fallback DB code as the relevant functions are replaced with their new Java counterparts.
Attachment #757722 - Attachment is obsolete: true
Attachment #758335 - Flags: feedback?(cpeterson)
Comment on attachment 758333 [details] [diff] [review] 1/4 Add read and write contacts Android permissions to Android manifest file Review of attachment 758333 [details] [diff] [review]: ----------------------------------------------------------------- LGTM!
Attachment #758333 - Flags: feedback?(cpeterson) → review+
Comment on attachment 758335 [details] [diff] [review] 1/5 Add contacts API Javascript to Android Review of attachment 758335 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/modules/AndroidContactService.jsm @@ +39,5 @@ > + > + var idbManager = Components.classes["@mozilla.org/dom/indexeddb/manager;1"].getService(Ci.nsIIndexedDatabaseManager); > + idbManager.initWindowless(myGlobal); > + this._db = new ContactDB(myGlobal); > + this._db.init(myGlobal); As you noted, we no longer need any of this _db or ContactDB code. @@ +65,5 @@ > + observe: function(aSubject, aTopic, aData) { > + switch (aTopic) { > + case "Contacts:Find:Return:OK": { > + let message = this._requestMessages[aData]; > + this._sendAndDeleteReturnMessage("Contacts:Find:Return:OK", aData, {requestID: message.data.requestID, contacts: result}); Gecko's JS (and C++) coding style limits line lengths to <= 80 characters. In our Java code, we allow lines <= 100 characters. @@ +68,5 @@ > + let message = this._requestMessages[aData]; > + this._sendAndDeleteReturnMessage("Contacts:Find:Return:OK", aData, {requestID: message.data.requestID, contacts: result}); > + break; > + > + } case "Contacts:Find:Return:KO": { I think having the } on the same line as case is confusing. I would just move the } to its own line after the break;
Attachment #758335 - Flags: feedback?(cpeterson) → feedback+
I landed patch 3/5 ("Add Contacts permissions to AndroidManifest.xml.in") so we can batch multiple permission changes in the same release (Firefox 24): https://hg.mozilla.org/integration/mozilla-inbound/rev/12cdc8931e48
Whiteboard: [leave open]
5/5 Integrate contacts API Javascript with Java and Android contact API Revised. clearAllContacts() function implemented and started find() function.
Attachment #757727 - Attachment is obsolete: true
Attachment #758906 - Flags: feedback?(cpeterson)
Comment on attachment 758906 [details] [diff] [review] 5/5 Integrate contacts API Javascript with Java and Android contact API Review of attachment 758906 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/ContactService.java @@ +727,5 @@ > + if ("name".equals(field)) { > + return StructuredName.DISPLAY_NAME; > + } > + if ("givenName".equals(field)) { > + return StructuredName.GIVEN_NAME; These methods to map column names have a lot of repeated code. You might want investigate a more functional approach like the sInputMethodBlocklist code in FormAssistPopup.java: https://hg.mozilla.org/mozilla-central/file/a47f4e36197f/mobile/android/base/FormAssistPopup.java#l60 We could have Map<String,String> collections that map the JS attribute names to the Android column names. The map definitions are a little cumbersome, but maintaining them should be easier and the lookup method should be much shorter. :D
Attachment #758906 - Flags: feedback?(cpeterson) → feedback+
Should have the newly added permissions on mozilla-central land behind a confvar?
The actual implementation of the contacts API isn't ready yet so there's nothing to confvar. The permissions patch was landed so it would be with the other new permissions for the next update.
I backed out the Contacts permissions until the API implementation is ready to use them: https://hg.mozilla.org/integration/mozilla-inbound/rev/b6652771e70f
Another note for later: Requesting read/write CONTACTS permission will implicitly request CALL_LOG permissions too. Several people have been asking why Firefox Nightly needs access to the CALL_LOG permissions. Technically, we don't. Android docs say we can bump our targeted SDK to 16 to avoid the implicit CALL_LOG permission requests: http://developer.android.com/reference/android/Manifest.permission.html#READ_CALL_LOG
Should I make the target SDK bump a patch for this bug or should that be taken care of elsewhere?
Depends on: 807688
Depends on: 882442
No longer depends on: 882442
Depends on: 832960
Depends on: 882495
Attachment #757726 - Attachment is obsolete: true
Attached patch 2/4 Add permissions doorhanger (obsolete) — — Splinter Review
Attachment #757724 - Attachment is obsolete: true
Attachment #762396 - Flags: feedback?(cpeterson)
Still WIP, but it's getting there.
Attachment #758335 - Attachment is obsolete: true
Attachment #762397 - Flags: feedback?(cpeterson)
Attachment #762396 - Attachment description: Add permissions doorhanger → 2/5 Add permissions doorhanger
Attachment #758333 - Attachment description: 3/5 Add read and write contacts Android permissions to Android manifest file → 1/5 Add read and write contacts Android permissions to Android manifest file
Attachment #762397 - Attachment description: 2/5 Add contacts API Javascript to Android → 3/5 Add contacts API Javascript to Android
WIP, some parts are a little rough still, but it's coming along.
Attachment #758906 - Attachment is obsolete: true
Attachment #762406 - Flags: feedback?(cpeterson)
Attachment #762397 - Attachment description: 3/5 Add contacts API Javascript to Android → 3/4 Add contacts API Javascript to Android
Attachment #762396 - Attachment description: 2/5 Add permissions doorhanger → 2/4 Add permissions doorhanger
Attachment #758333 - Attachment description: 1/5 Add read and write contacts Android permissions to Android manifest file → 1/4 Add read and write contacts Android permissions to Android manifest file
Comment on attachment 762397 [details] [diff] [review] 3/5 Add contacts API Javascript to Android Review of attachment 762397 [details] [diff] [review]: ----------------------------------------------------------------- Looking good. ::: mobile/android/modules/AndroidContactService.jsm @@ +36,5 @@ > + // Add listeners for all messages from ContactService.java > + this._returnMessages = ["Contacts:Find:Return:OK", "Contact:Save:Return:OK", > + "Contact:Remove:Return:OK", "Contacts:Clear:Return:OK", > + "Contacts:Find:Return:KO", "Contact:Save:Return:KO", > + "Contact:Remove:Return:KO", "Contacts:Clear:Return:KO", These message names might be easier to read if each message type's OK and KO was listed on the same line: this._returnMessages = ["Contacts:Clear:Return:OK", "Contact:Clear:Return:KO", "Contact:Find:Return:OK", "Contacts:Find:Return:KO", "Contacts:Remove:Return:KO", "Contact:Remove:Return:KO", "Contact:Save:Return:KO", "Contacts:Save:Return:KO", @@ +63,5 @@ > + let requestID = message.requestID; > + > + switch (aTopic) { > + case "Contacts:Find:Return:OK": > + this._sendAndDeleteReturnMessage(aTopic, requestID, {requestID: requestID, contacts: result}); You could extract a lot of this duplicated code by making the `switch (aTopic)` statement only construct the `result` object and then call `this._sendAndDeleteReturnMessage(aTopic, requestID, result)` only once at the end of the function. @@ +118,5 @@ > + return true; > + }, > + > + receiveMessage: function(aMessage) { > + if (DEBUG) debug("receiveMessage " + aMessage.name); If you plan to leave this debug code, you should add curly braces and newlines. @@ +121,5 @@ > + receiveMessage: function(aMessage) { > + if (DEBUG) debug("receiveMessage " + aMessage.name); > + > + // GetAll uses a cursor ID instead of a request ID, but they can be treated the same from here > + let requestID; The `requestID` local variable is unused. @@ +124,5 @@ > + // GetAll uses a cursor ID instead of a request ID, but they can be treated the same from here > + let requestID; > + if (!aMessage.data.requestID && aMessage.data.cursorId) { > + aMessage.data.requestID = aMessage.data.cursorId; > + } To avoid repeating `aMessage.data.requestID` multiple times in this function, you could cache the value in `requestID`. Was that your intention for the unused variable?
Attachment #762397 - Attachment description: 3/4 Add contacts API Javascript to Android → 3/5 Add contacts API Javascript to Android
Attachment #762397 - Flags: feedback?(cpeterson) → feedback+
Comment on attachment 762396 [details] [diff] [review] 2/4 Add permissions doorhanger Review of attachment 762396 [details] [diff] [review]: ----------------------------------------------------------------- LGTM!
Attachment #762396 - Attachment description: 2/4 Add permissions doorhanger → 2/5 Add permissions doorhanger
Attachment #762396 - Flags: feedback?(cpeterson) → review+
Comment on attachment 762406 [details] [diff] [review] 4/4 Integrate contacts API Javascript with Java and Android contact API Review of attachment 762406 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/ContactService.java @@ +89,5 @@ > + registerEventListener("Contacts:Find"); > + registerEventListener("Contacts:GetAll"); > + registerEventListener("Contact:Remove"); > + registerEventListener("Contact:Save"); > + registerEventListener("Contacts:GetCount"); Please alphabetize these event listener names. @@ +106,5 @@ > + unregisterEventListener("Contacts:Find"); > + unregisterEventListener("Contacts:GetAll"); > + unregisterEventListener("Contact:Remove"); > + unregisterEventListener("Contact:Save"); > + unregisterEventListener("Contacts:GetCount"); Please alphabetize these event listener names. @@ +111,5 @@ > + } > + > + @Override > + public void handleMessage(final String event, final JSONObject message) { > + mActivity.runOnUiThread(new Runnable() { * Why does this code need to run on the UI thread? Please add a short comment explaining why. * Can you move some of the run() code that doesn't need to run on the UI thread before the call to runOnUiThread()? The JSON parsing could probably be done on the Gecko thread. @@ +149,5 @@ > + throw new IllegalArgumentException("Unexpected event: " + event); > + } > + } catch (JSONException e) { > + Log.e(LOGTAG, "JSON exception", e); > + throw new IllegalArgumentException("Message: " + message.toString()); You don't need to log the JSONException `e` if you call the IllegalArgumentException constructor that takes two parameters: a String message and another exception. @@ +166,5 @@ > + try { > + callbackMessage.put("requestID", requestID); > + callbackMessage.put("contacts", getContactsAsJSONArray(rawContactIds)); > + sendCallbackToJavascript("Contacts:Find:Return:OK", callbackMessage); > + } catch (JSONException e) { Does sendCallbackToJavascript() throw a JSONException? Can we move the sendCallbackToJavascript() after the try/catch? These questions apply to other methods below, too. @@ +207,5 @@ > + long rawContactId = 0; > + boolean wasSuccessful = false; > + try { > + ContentProviderResult[] removeResults = mContentResolver.applyBatch(ContactsContract.AUTHORITY, > + (ArrayList<ContentProviderOperation>)deleteOptionsList); * You can ignore my previous suggestion to make the local variables like deleteOptionsList have type List<> (instead of ArrayList<>) if you are calling (lame) Android APIs that use ArrayList<>s. * Every applyBatch() call contains some duplicate code. Can you write your own applyBatch() helper method that includes the ContractsContract.AUTHORITY parameter and the annoying exception handling? private ContentProviderResult[] applyBatch(ArrayList<ContentProviderOperation>) operations) { try { return mContentResolver.applyBatch(ContactsContract.AUTHORITY, operations); } catch (RemoteException e) { Log.e(LOGTAG, "RemoteException", e); } catch (OperationApplicationException e) { Log.e(LOGTAG, "OperationApplicationException", e); } return null; } @@ +222,5 @@ > + } catch (RemoteException e) { > + Log.e(LOGTAG, "RemoteException", e); > + } catch (OperationApplicationException e) { > + Log.e(LOGTAG, "OperationApplicationException", e); > + } catch (NumberFormatException e) { This try/catch is catching a lot of different exceptions. Can you split it up so the RemoteException and OperationApplicationException are only around the applyBatch() call? @@ +990,5 @@ > + return Long.parseLong(uri.substring(uri.lastIndexOf("/") + 1)); > + } > + } > + > + return -1; Could -1 ever be a real raw content ID? Maybe instead of return -1 for one code path or throwing NumberFormatException in another, you could return a boxed `Long` object that can have a long value or be null (to indicate missing or bad field)? @@ +997,5 @@ > + private static String getColumnNameConstant(String field) { > + if ("name".equals(field)) { > + return StructuredName.DISPLAY_NAME; > + } > + if ("givenName".equals(field)) { My suggestion in comment 18 still applies. I think these mapping functions could be greatly simplified by using Map<String,String> collections that map the JS attribute names to the Android column names
Attachment #762406 - Flags: feedback?(cpeterson) → feedback+
Attachment #762397 - Attachment is obsolete: true
Attachment #764492 - Flags: feedback?(cpeterson)
Still to do: * Save contact photos * Add prompt & pref for handling multiple accounts
Attachment #762406 - Attachment is obsolete: true
Attachment #764494 - Flags: feedback?(cpeterson)
Attachment #762396 - Attachment description: 2/5 Add permissions doorhanger → 2/4 Add permissions doorhanger
Comment on attachment 764492 [details] [diff] [review] 3/4 Add contacts API Javascript to Android Review of attachment 764492 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/contacts/ContactManager.js @@ +922,5 @@ > "Contact:Remove:Return:OK", "Contact:Remove:Return:KO", > "Contact:Changed", > "PermissionPromptHelper:AskPermission:OK", > + "Contacts:GetAll:Next", "Contacts:Count", > + "Contacts:Revision", "Contacts:Revision:NotImplemented",]); Could we return a fake revision number, such as a counter variable in a AndroidContactService.jsm? We could call the new event "Contacts:Revision:KO" (and include an error message?) to make the name more consistent with the other event names. You might want to ask Gregor or someone if they have an opinion. ::: mobile/android/chrome/content/browser.js @@ +16,5 @@ > Cu.import("resource://gre/modules/FileUtils.jsm"); > Cu.import("resource://gre/modules/JNI.jsm"); > Cu.import('resource://gre/modules/Payment.jsm'); > Cu.import("resource://gre/modules/PermissionPromptHelper.jsm"); > +Cu.import("resource://gre/modules/AndroidContactService.jsm"); I've been told by a reviewer that the Firefox Desktop team prefers modules not clutter the global namespace and instead use components to boot themselves. See bug 871372 for an example from PushService for Firefox Desktop. Just something to think about. I would not spend any time worrying about this now. We can investigate this after you have landed the AndroidContactService.
Attachment #764492 - Flags: feedback?(cpeterson) → feedback+
> Could we return a fake revision number, such as a counter variable in a AndroidContactService.jsm? (In reply to Chris Peterson (:cpeterson) from comment #33) > Comment on attachment 764492 [details] [diff] [review] > 3/4 Add contacts API Javascript to Android > > Review of attachment 764492 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/contacts/ContactManager.js > @@ +922,5 @@ > > "Contact:Remove:Return:OK", "Contact:Remove:Return:KO", > > "Contact:Changed", > > "PermissionPromptHelper:AskPermission:OK", > > + "Contacts:GetAll:Next", "Contacts:Count", > > + "Contacts:Revision", "Contacts:Revision:NotImplemented",]); > > Could we return a fake revision number, such as a counter variable in a > AndroidContactService.jsm? > > We could call the new event "Contacts:Revision:KO" (and include an error > message?) to make the name more consistent with the other event names. You > might want to ask Gregor or someone if they have an opinion. > Actually, returning a not implemented message is what Gregor recommended.
Comment on attachment 764494 [details] [diff] [review] 4/4 Integrate contacts API Javascript with Java and Android contact API Review of attachment 764494 [details] [diff] [review]: ----------------------------------------------------------------- ConstactService.java is getting very long. Maybe you could extract the JS-to-Android field maps and/or the ContentProvider/ContentResolver/accounts glue into separate files? ::: mobile/android/base/ContactService.java @@ +61,5 @@ > + private final static int GROUP_ACCOUNT_TYPE = 1; > + private final static int GROUP_ID = 2; > + private final static int GROUP_TITLE = 3; > + private final static int GROUP_AUTO_ADD = 4; > + // Pre-Honeycomb versions of Android have a "My Contacts" system group that all contacts are Please add a blank line between GROUP_AUTO_ADD and this comment. @@ +83,5 @@ > + private HashMap<String, Integer> mAddressTypesMap; > + private HashMap<String, Integer> mPhoneTypesMap; > + private HashMap<String, Integer> mEmailTypesMap; > + private HashMap<String, Integer> mWebsiteTypesMap; > + private HashMap<String, Integer> mImTypesMap; Declare these instance variables as `final Map<>` types and then initialize them right here with = new HashMap<>. @@ +197,5 @@ > + String filterValue = findOptions.getString("filterValue"); > + final String filterOp = findOptions.getString("filterOp"); > + final String sortBy = findOptions.getString("sortBy"); > + final String sortOrder = findOptions.getString("sortOrder"); > + final long filterLimit = findOptions.getLong("filterLimit"); You might be able to simplify findContactsRawIds() be splitting the findOptions JSON parsing and the ContentResolver querying into separate helper methods. @@ +280,5 @@ > + break; > + } > + } > + } > + cursor.close(); Move cursor.close() to a finally {} block. @@ +1446,5 @@ > + mPhoneTypesMap.put("ttd", Phone.TYPE_TTY_TDD); > + mPhoneTypesMap.put("work mobile", Phone.TYPE_WORK_MOBILE); > + mPhoneTypesMap.put("work pager", Phone.TYPE_WORK_PAGER); > + mPhoneTypesMap.put("assistant", Phone.TYPE_ASSISTANT); > + mPhoneTypesMap.put("mms", Phone.TYPE_MMS); I guess the Map initialization is not as elegant as I was imagining. :\ If you think your previous if/else/if/else code was easier to read or maintain, please feel free to revert to that style. Your original implementation would probably be faster and would definitely use less memory. You wouldn't have to allocate a bunch of objects for the HashMaps or box the TYPE ints into Integer objects so they can be inserted into the Map. If you would like to keep the Map implementation, let's make these init*Map() functions lazy. initPhoneTypesMap() could become getPhoneTypesMap() and only initialize mPhoneTypesMap the first time getPhoneTypesMap() is called (if ever).
Attachment #764494 - Flags: feedback?(cpeterson) → feedback+
Depends on: 889673
Comment on attachment 764492 [details] [diff] [review] 3/4 Add contacts API Javascript to Android Review of attachment 764492 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/contacts/ContactManager.js @@ +668,5 @@ > Services.DOMRequest.fireSuccess(req, msg.revision); > } > break; > + // Android does not support the revision function > + case "Contacts:Revision:NotImplemented": Make it "Contacts:Revision:KO", and pass the error message in msg.errorMsg, that way you can simply add |case "Contacts:Revision:KO"| to the existing error block. ::: mobile/android/modules/AndroidContactService.jsm @@ +45,5 @@ > + }.bind(this)); > + }, > + > + _sendMessageToJava: function(aMsg) { > + Cc["@mozilla.org/android/bridge;1"].getService(Ci.nsIAndroidBridge).handleGeckoMessage(JSON.stringify(aMsg)); JSON.stringify is a lossy operation and will happily destroy Blobs in the data without saying anything. You'll notice that we use |aMessage.data| and not |aMessage.json| in ContactService.jsm. I suspect that's why you have the Blob code commented out in your Java code? We probably can't send a structured clone over to Java code (can we?), so it sounds like you'll have to resort to converting the Blobs to base64 strings before sending them or something equally horrible :( Actually, even if we could do a structured clone there, you're using nsIObserverService to receive messages back from Java, so you'd also lose information on the way back. @@ +223,5 @@ > + > + this._sendMessageToJava({type: "Contacts:GetCount", data: aMessage.data}); > + }, > + > + _convertJSONArrayToContactsArray: function(jsonContacts) { Is this dead code? Doesn't seem to get called anywhere.
Comment on attachment 764494 [details] [diff] [review] 4/4 Integrate contacts API Javascript with Java and Android contact API Review of attachment 764494 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/ContactService.java @@ +54,5 @@ > +import java.util.Map; > + > +public class ContactService implements GeckoEventListener { > + private static final String LOGTAG = "GeckoContactService"; > + private static final boolean DEBUG = true; For what is worth, we only do the |if (DEBUG) debug()| dance in our JS code because of performance reasons (it turns out if you put the check inside of the function we still waste a ton of time building strings, calling JSON.stringify, etc). You should avoid doing that here if possible, unless that's the standard practice for Java code (I hope not).
Reuben, yes, it looks like I'm going to have to do pass a base64 string back and forth. I'm working on this now. These patches are pretty out of date by now. I was going to update them when I have the photos implmented. I did change the revision message and debug function though. The "convertJSONArrayToContactsArray" function was removed a while ago.
Attachment #764492 - Attachment is obsolete: true
Attachment #771124 - Flags: review?(cpeterson)
Woo! Almost done! (except for what are hopefully minor corrections :)
Attachment #764494 - Attachment is obsolete: true
Attachment #771125 - Flags: review?(cpeterson)
Changed message for error on revision function.
Attachment #771124 - Attachment is obsolete: true
Attachment #771124 - Flags: review?(cpeterson)
Attachment #772158 - Flags: review?(cpeterson)
Comment on attachment 772158 [details] [diff] [review] 3/4 Add contacts API Javascript to Android Review of attachment 772158 [details] [diff] [review]: ----------------------------------------------------------------- Looks good! Some minor feedback: ::: dom/contacts/ContactManager.js @@ +3,5 @@ > * You can obtain one at http://mozilla.org/MPL/2.0/. */ > > "use strict"; > > +const DEBUG = true; Don't forget to change DEBUG back to false! I recommend keeping debug changes like this in a separate patch in your patch queue. @@ +540,5 @@ > > ContactManager.prototype = { > __proto__: DOMRequestIpcHelper.prototype, > _oncontactchange: null, > + _cachedContacts: new Array(), Can you just use an array literal like `_cachedContacts: []`? @@ +631,5 @@ > } > break; > case "Contact:Save:Return:OK": > + // If a cached contact was saved and a new contact ID was returned, update the contact's ID > + if (msg.contactID != undefined && this._cachedContacts[msg.requestID] != undefined) { Comparing variables to null or undefined is not very JavaScripty. The preferred style is to rely on those values being "falsy", unless you really need to differentiate between null and undefined values. if (msg.contactID && this._cachedContacts[msg.requestID]) { @@ +649,3 @@ > req = this.getRequest(msg.requestID); > + if (req) { > + if (req.request !== undefined) { if (req.request) { ::: mobile/android/modules/ContactService.jsm @@ +3,5 @@ > + * You can obtain one at http://mozilla.org/MPL/2.0/. */ > + > +"use strict"; > + > +const DEBUG = true; DEBUG = false @@ +25,5 @@ > + if (DEBUG) debug("Init"); > + this._requestMessages = {}; > + > + // Add listeners for all messages from ContactManager.js > + this._messages = ["Contacts:Find", "Contacts:GetAll", "Contacts:GetAll:SendNow", this._messages can just be a local variable; it's not used in any other function. @@ +27,5 @@ > + > + // Add listeners for all messages from ContactManager.js > + this._messages = ["Contacts:Find", "Contacts:GetAll", "Contacts:GetAll:SendNow", > + "Contacts:Clear", "Contact:Save", "Contacts:GetCount", > + "Contact:Remove", "Contacts:GetRevision",]; Please alphabetize this list. @@ +33,5 @@ > + ppmm.addMessageListener(msgName, this); > + }.bind(this)); > + > + // Add listeners for all messages from ContactService.java > + this._returnMessages = ["Android:Contacts:Find:Return:OK", "Android:Contact:Save:Return:OK", this._returnMessages can just be a local variable; it's not used in any other function. @@ +38,5 @@ > + "Android:Contact:Remove:Return:OK", "Android:Contacts:Clear:Return:OK", > + "Android:Contacts:Find:Return:KO", "Android:Contact:Save:Return:KO", > + "Android:Contact:Remove:Return:KO", "Android:Contacts:Clear:Return:KO", > + "Android:Contacts:GetAll:Next", "Android:Contacts:Count", > + "Android:Contacts:RegisterForMessages",]; Please alphabetize this list. I think having the "XYZ:Return:KO" and "XYZ:Return:OK" is more useful than grouping all the OKs and KOs with each other. @@ +69,5 @@ > + > + // The return message topic is the same as the current topic, but without the "Android:" prefix > + let returnMessageTopic = aTopic.substring(8); > + > + switch (aTopic) { Please add a default case that throws an error message for unexpected values of aTopic. @@ +178,5 @@ > + errorMsg: "Android does not support the revision function."}); > + break; > + > + default: > + if (DEBUG) debug("WRONG MESSAGE NAME: " + aMessage.name); If receiveMessage() receives an unexpected message name, is that a bug in ContactManager.js? I think we should just throw an error message, instead of just logging. @@ +192,5 @@ > + > + let countryName = PhoneNumberUtils.getCountryName(); > + let substringmatchingValue = 0; > + if (Services.prefs.getPrefType("dom.phonenumber.substringmatching." + countryName) == Ci.nsIPrefBranch.PREF_INT) { > + substringmatchingValue = Services.prefs.getIntPref("dom.phonenumber.substringmatching." + countryName); Move the "dom.phonenumber.substringmatching." + countryName expression into a local variable (substringmatchingPref?) to avoid extra string operations and reduce code duplication. @@ +234,5 @@ > + } > + > + this._sendMessageToJava({type: "Android:Contacts:Clear", data: aMessage.data}); > + > + return true; Why does clearContacts() return a boolean value? The caller receiveMessage() does not check the return value and none of the other contact functions called from receiveMessage() return a value. @@ +239,5 @@ > + }, > + > + getContactsCount: function(aMessage) { > + if (!this.assertPermission(aMessage, "contacts-read")) { > + return null; Just return;
Attachment #772158 - Flags: review?(cpeterson) → feedback+
Revised with suggested changes.
Attachment #772158 - Attachment is obsolete: true
Attachment #773619 - Flags: review?(cpeterson)
Comment on attachment 773619 [details] [diff] [review] 3/4 Add contacts API Javascript to Android Review of attachment 773619 [details] [diff] [review]: ----------------------------------------------------------------- LGTM! Just two tiny nits: ::: dom/contacts/ContactManager.js @@ +634,5 @@ > + // If a cached contact was saved and a new contact ID was returned, update the contact's ID > + // If the request does not have a new contact ID, remove the contact from cache > + if (msg.contactID && this._cachedContacts[msg.requestID]) { > + this._cachedContacts[msg.requestID].id = msg.contactID; > + } else if (this._cachedContacts[msg.requestID]) { If you check `if (this._cachedContacts[msg.requestID])` first, then you only have to check it once. You can then just check `if (msg.contact)` to know whether to cache or delete requestID. ::: mobile/android/modules/ContactService.jsm @@ +35,5 @@ > + > + // Add listeners for all messages from ContactService.java > + let returnMessages = ["Android:Contacts:Count", > + "Android:Contacts:Clear:Return:OK", "Android:Contacts:Clear:Return:KO", > + "Android:Contacts:Find:Return:OK", "Android:Contacts:Find:Return:KO", Fix indentation.
Attachment #773619 - Flags: review?(cpeterson) → review+
Revised.
Attachment #773619 - Attachment is obsolete: true
Attachment #773666 - Flags: review?(cpeterson)
Removed delete contacts from cache
Attachment #773666 - Attachment is obsolete: true
Attachment #773666 - Flags: review?(cpeterson)
Attachment #773683 - Flags: review?(cpeterson)
Comment on attachment 771125 [details] [diff] [review] 4/4 Integrate contacts API Javascript with Java and Android contact API Review of attachment 771125 [details] [diff] [review]: ----------------------------------------------------------------- Looking good with some small changes: ::: mobile/android/base/ContactService.java @@ +52,5 @@ > +import android.view.View; > + > +import java.lang.IllegalArgumentException; > +import java.lang.NumberFormatException; > +import java.lang.Runnable; I don't think you you need to import java.lang.* classes. @@ +63,5 @@ > +import java.util.Map.Entry; > + > +public class ContactService implements GeckoEventListener { > + private static final String LOGTAG = "GeckoContactService"; > + private static final boolean DEBUG = true; DEBUG = false; @@ +132,5 @@ > + } > + > + @Override > + public void handleMessage(final String event, final JSONObject message) { > + Runnable handleMessage = new Runnable() { Please add a short comment explaining which thread is calling handleMessage() and why we need to post a Runnable to another thread. @@ +146,5 @@ > + > + // Options may not exist for all operations > + JSONObject contactOptions = null; > + if (messageData.has("options")) { > + contactOptions = messageData.getJSONObject("options"); Can you just call messageData.optJSONObject("options") without the has("options") check? JSONObject's opt*() methods return null for missing properties instead of throwing a JSONException. @@ +171,5 @@ > + } > + }; > + > + if (!mWasInit) { > + init(handleMessage); Can you replace mWasInit with a check for `mAccountName == null`? getDeviceAccount() seems to be only thing that init() really does (other than initializing mContentResolver, which I assume you could do in the ContactService() constructor). @@ +195,5 @@ > + } > + > + private void getAllContacts(final JSONObject contactOptions, final String requestID) { > + long[] rawContactIds = getAllRawContactIds(); > + Log.i(LOGTAG, "Got " + rawContactIds.length + " raw contact IDs"); Fix indentation. @@ +233,5 @@ > + (!"ascending".equals(sortOrder) && !"descending".equals(sortOrder))) { > + return null; > + } > + } catch (JSONException e) { > + Log.e(LOGTAG, "JSON Exception", e); I think a JSONException here would indicate a bug in our code, so we should `throw new IllegalArgumentException(e)`. This comment applies to all other methods that catch and log JSONExceptions, too. :) @@ +241,5 @@ > + String[] sortOptions = new String[2]; > + sortOptions[0] = sortBy; > + sortOptions[1] = sortOrder; > + > + return sortOptions; Can you can simplify this code to something like `return new String[] { sortBy, sortOrder };`. @@ +1481,5 @@ > + } > + > + private void getContactsCount(final String requestID) { > + sendCallbackToJavascript("Android:Contacts:Count", requestID, new String[] {"count"}, > + new Object[] {(Integer)(getAllRawContactIds().length)}); Is there a cheaper way to query the ContentResolver for the raw contact count without return the every contact DB just to get the array length? Also, please add one or two local variables to simplify this complicated getAllRawContactIds() expression. @@ +1484,5 @@ > + sendCallbackToJavascript("Android:Contacts:Count", requestID, new String[] {"count"}, > + new Object[] {(Integer)(getAllRawContactIds().length)}); > + } > + > + private void sendCallbackToJavascript(final String subject, final String requestID, final String[] argNames, final Object[] args) { `argValues` might be a clearer name than `args` because the name will pair well with `argNames`. @@ +1486,5 @@ > + } > + > + private void sendCallbackToJavascript(final String subject, final String requestID, final String[] argNames, final Object[] args) { > + // Check the same number of argument names and arguments were given > + if (argNames != null && args != null && argNames.length != args.length) { Is it valid to have a non-null argNames with a null args? You can probably just check argNames != null and let this code NPE if args == null. @@ +1487,5 @@ > + > + private void sendCallbackToJavascript(final String subject, final String requestID, final String[] argNames, final Object[] args) { > + // Check the same number of argument names and arguments were given > + if (argNames != null && args != null && argNames.length != args.length) { > + throw new IllegalArgumentException("argument names and arguments length do not match"); You should include the mismatched names and arguments lengths in the exception message. @@ +1494,5 @@ > + try { > + JSONObject callbackMessage = new JSONObject(); > + callbackMessage.put("requestID", requestID); > + > + if (argNames != null && args != null) { IIUC, you can remove the args != null check. @@ +1551,5 @@ > + accountNames[i] = accounts[i].name; > + } > + > + final AlertDialog.Builder builder = new AlertDialog.Builder(mActivity); > + builder.setTitle("Share contacts from...") UI strings should be loaded from resources so they can be localized. @@ +1648,5 @@ > + // Check if the account name and type for the group match the account name and type of > + // the account we're working with > + if (groupAccountName.equals(mAccountName) && groupAccountType.equals(mAccountType)) { > + if (groupName.equals(cursor.getString(GROUP_TITLE))) { > + groupId = cursor.getLong(GROUP_ID); Can there be more than one groupId match? Can we break out of the while loop here when we find the first GROUP_TITLE match? @@ +1769,5 @@ > + > + return false; > + } > + > + private static long[] convertLongListToArray(List<Long> list) { Can you replace convertLongListToArray() with List.toArray()? @@ +1782,5 @@ > + > + private static boolean doesJSONArrayContainString(final JSONArray array, final String value) { > + try { > + for (int i = 0; i < array.length(); i++) { > + if (value.equals(array.getString(i))) { If you replace array.getString() with optString() (which returns null), then you can remove the ugly try/catch JSONException.
Attachment #771125 - Flags: review?(cpeterson) → feedback+
Revised
Attachment #771125 - Attachment is obsolete: true
Attachment #774097 - Flags: review?(cpeterson)
Comment on attachment 773683 [details] [diff] [review] 3/4 Add contacts API Javascript to Android Review of attachment 773683 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/contacts/ContactManager.js @@ +632,5 @@ > break; > case "Contact:Save:Return:OK": > + // If a cached contact was saved and a new contact ID was returned, update the contact's ID > + if (msg.contactID && this._cachedContacts[msg.requestID]) { > + this._cachedContacts[msg.requestID].id = msg.contactID; Do the _cachedContacts entries ever get cleared?
Comment on attachment 774097 [details] [diff] [review] 4/4 Integrate contacts API Javascript with Java and Android contact API Review of attachment 774097 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/ContactService.java @@ +74,5 @@ > + > + // Pre-Honeycomb versions of Android have a "My Contacts" system group that all contacts are > + // assigned to by default for a given account. After Honeycomb, an AUTO_ADD database column > + // was added to denote groups that contacts are automatically added to > + private final static String PRE_HONEYCOMB_DEFUALT_GROUP = "System Group: My Contacts"; "DEFAULT" is misspelled in "PRE_HONEYCOMB_DEFUALT_GROUP". @@ +167,5 @@ > + }; > + > + // If the account name is null, we need to initalize some member variables > + if (mAccountName == null) { > + init(handleMessage); Can you remove the init() method and just call getDeviceAccount(handleMessage) here? I think the code would make more sense. All init() does is initialize mContentResolver and call getDeviceAccount(). Can you initialize mContentResolver in ContactService's constructor? @@ +1468,5 @@ > + return contentValues; > + } > + > + private void getContactsCount(final String requestID) { > + Integer numContacts = (Integer)(getAllRawContactIds().length); Querying all raw contacts just to get the contact count seems unnecessarily expensive. I think you can query the ContentResolver for just the count. Stack Overflow has the following example: http://stackoverflow.com/a/5935434/61352 Cursor countCursor = getContentResolver().query(CONTENT_URI, new String[] {"count(*) AS count"}, null, null, null); countCursor.moveToFirst(); int count = countCursor.getInt(0); @@ +1763,5 @@ > + > + return false; > + } > + > + private static long[] convertLongListToArray(List<Long> list) { Can you replace calls to convertLongListToArray() with List.toArray()? The convertLongListToArray() method seems to be doing the same thing.
Attachment #774097 - Flags: review?(cpeterson) → feedback+
The getContactsCount() function has not changed. The Contacts table is the aggregate contacts so Android will try to merge contacts together there giving a false number of contacts as far as the contacts WebAPI is concerned (which makes the unit tests fail). The Data table contains multiple rows for a given contact ID so it's necessary to get all the IDs from this table and then filter out the duplicates, which is what the getAllRawContactIds() function does so we use the length of the array that that function returns as the number of contacts. This seems to be the "cleanest" solution, despite the added memory and time complexity.
Attachment #774097 - Attachment is obsolete: true
Attachment #774808 - Flags: review?(cpeterson)
Comment on attachment 774808 [details] [diff] [review] 4/4 Integrate contacts API Javascript with Java and Android contact API Review of attachment 774808 [details] [diff] [review]: ----------------------------------------------------------------- LGTM!
Attachment #774808 - Flags: review?(cpeterson) → review+
Blocks: 862157
Attachment #773683 - Attachment is obsolete: true
Attachment #773683 - Flags: review?(cpeterson)
Attachment #778092 - Flags: review?(cpeterson)
Attachment #774808 - Attachment is obsolete: true
Attachment #778093 - Flags: review?(cpeterson)
Attachment #778095 - Flags: review?(cpeterson)
Comment on attachment 778092 [details] [diff] [review] 3/5 Add contacts API Javascript to Android Review of attachment 778092 [details] [diff] [review]: ----------------------------------------------------------------- LGTM!
Attachment #778092 - Flags: review?(cpeterson) → review+
Comment on attachment 778093 [details] [diff] [review] 4/5 Integrate contacts API Javascript with Java and Android contact API Review of attachment 778093 [details] [diff] [review]: ----------------------------------------------------------------- LGTM
Attachment #778093 - Flags: review?(cpeterson) → review+
Comment on attachment 778095 [details] [diff] [review] 5/5 - Disable contact unit tests on Android Review of attachment 778095 [details] [diff] [review]: ----------------------------------------------------------------- CC'ing khuey LGTM, but (most) build config changes should be reviewed by a build peer.
Attachment #778095 - Flags: review?(khuey)
Attachment #778095 - Flags: review?(cpeterson)
Attachment #778095 - Flags: feedback+
Comment on attachment 778095 [details] [diff] [review] 5/5 - Disable contact unit tests on Android Review of attachment 778095 [details] [diff] [review]: ----------------------------------------------------------------- Uh, you're disabling these tests on everything but Android, no?
Attachment #778095 - Flags: review?(khuey) → review-
Comment on attachment 778095 [details] [diff] [review] 5/5 - Disable contact unit tests on Android Review of attachment 778095 [details] [diff] [review]: ----------------------------------------------------------------- Nevermind, I can't read.
Attachment #778095 - Flags: review- → review+
Blocks: 896003
No longer blocks: 896003
Pref'ed off contact permissions.
Attachment #758333 - Attachment is obsolete: true
Attachment #779829 - Flags: review?(cpeterson)
Attachment #778095 - Attachment is obsolete: true
Fixed permissions test failure
Attachment #779885 - Flags: review?(cpeterson)
Attachment #762396 - Attachment is obsolete: true
Changed to use NIGHTLY_BUILD
Attachment #779829 - Attachment is obsolete: true
Attachment #779829 - Flags: review?(cpeterson)
Attachment #779969 - Flags: review?(cpeterson)
Rebased patch.
Attachment #778093 - Attachment is obsolete: true
Attachment #779976 - Flags: review?(cpeterson)
Comment on attachment 779969 [details] [diff] [review] 1/5 Add contacts permissions to manifest Review of attachment 779969 [details] [diff] [review]: ----------------------------------------------------------------- LGTM
Attachment #779969 - Flags: review?(cpeterson) → review+
Comment on attachment 779976 [details] [diff] [review] 4/5 Integrate contacts API Javascript with Java and Android contact API Review of attachment 779976 [details] [diff] [review]: ----------------------------------------------------------------- LGTM
Attachment #779976 - Flags: review?(cpeterson) → review+
Attachment #779972 - Attachment is obsolete: true
Attachment #779972 - Flags: review?(cpeterson)
Attachment #780070 - Flags: review?(cpeterson)
Removed extraneous 'if'
Attachment #780070 - Attachment is obsolete: true
Attachment #780070 - Flags: review?(cpeterson)
Attachment #780071 - Flags: review?(cpeterson)
Comment on attachment 779885 [details] [diff] [review] 2/5 Add permissions doorhanger Review of attachment 779885 [details] [diff] [review]: ----------------------------------------------------------------- LGTM. Since you are adding new UI strings, you should ask around to find out whether we need to alert the localization volunteers.
Attachment #779885 - Flags: review?(cpeterson) → review+
Comment on attachment 780071 [details] [diff] [review] 5/5 - Disable contact unit tests on Android for non-nightly channels Review of attachment 780071 [details] [diff] [review]: ----------------------------------------------------------------- LGTM
Attachment #780071 - Flags: review?(cpeterson) → review+
Status: NEW → ASSIGNED
Whiteboard: [leave open]
I believe these are set by mcMerge when the patch hits central, and we wouldn't want to make our sheriff's lives harder :)
status-firefox24: wontfix → ---
status-firefox25: fixed → ---
The Inbound Rules wiki says to leave the Target Milestone unset, but doesn't mention the status flags. https://wiki.mozilla.org/Tree_Rules/Inbound#Please_do_the_following_after_pushing_to_inbound
(In reply to Shane Tully (:stully: from comment #3) > Created attachment 757725 [details] [diff] [review] > 3/5 Add read and write contacts Android permissions to Android manifest file These new permissions will need to be documented on this page: https://support.mozilla.org/en-US/kb/how-firefox-android-use-permissions-it-requests
Blocks: 672352
relnote-firefox: --- → ?
Keywords: user-doc-needed
Depends on: 897657
Nominating this for tracking to make sure I remember to review our permissions bumps in each upcoming version
tracking-firefox25: --- → ?
relnote-firefox: ? → 25+
Disabled in Aurora 25 because this is only built if NIGHTLY_BUILD is defined. Relnote and user docs can wait until this is ready for uplift.
status-firefox25: --- → disabled
relnote-firefox: 25+ → ---
No longer blocks: 862157
Depends on: 1040442
Is there a bug for getting to a point where this can be re-enabled? If so, that bug needs dev-doc-needed and this one should have it removed.
mfinkle: can the Android Contacts WebAPI ever be enabled for web content? Or will it be restricted to web apps?
Flags: needinfo?(mark.finkle)
(In reply to Chris Peterson (:cpeterson) from comment #80) > mfinkle: can the Android Contacts WebAPI ever be enabled for web content? Or > will it be restricted to web apps? We have no plans to enable for general web content. The API is currently enabled for webapps on Nightly, but we don't have a release yet where we want to let it ride the trains. We should file a bug for doing that.
Flags: needinfo?(mark.finkle)
tracking-fennec: --- → Nightly+
Keywords: feature
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: