Bug 720934 - Part 1: Collected Android Sync 0.4 code drop (Bug 721305, Bug 709348, Bug 719693, Bug 709660, Bug 718703). a=mobile

Includes:
3ad57f4 Bug 718703 - Pair a Device crashes. r=rnewman
3c530d1 Bug 709660 - Fixed blurry icons used in SyncAdapter. r=rnewman
bde287a Bug 719693 - Compatibility for JDK < 6.
91c5e23 Bug 719693 - Effect review comments on 6ffb7d72f.
6ffb7d7 Bug 719693 - Handle date-format HTTP Retry-After headers.
6df289e Bug 709348 - Trace logging and fixed logging of visit equality.
71580df Bug 709348 - Include Fennec visits and counts in HistoryRecord androidID equivalence.
8f6a106 Bug 709348 - Equality work.
f40bf4a Bug 709348 - Part 5: Rework handling of certain cases of incoming deletion, including history visits.
0dd004d Bug 709348 - Part 4: Reworking equality tests between records.
2210717 Bug 709348 - Part 3: Better commenting for ignored bookmark records.
c05236b Bug 709348 - Part 2: Introduce trace log method.
a34f4aa Bug 709348 - Part 1: Introduce copyWithIDs to clone records.
796b15f Bug 721305 - sync.link.advancedsetup.label should use …, not ....
adf7586 Bump AndroidSync User-Agent version.
This commit is contained in:
Richard Newman 2012-02-03 13:09:28 -08:00
parent ee4c971fd9
commit 8ea74e7f50
12 changed files with 406 additions and 75 deletions

View File

@ -17,7 +17,7 @@
<!ENTITY sync.pin.default.label '...\n...\n...\n'>
<!ENTITY sync.pin.oneline.label '...'>
<!ENTITY sync.link.show.label 'Show me how.'>
<!ENTITY sync.link.advancedsetup.label 'Advanced setup...'>
<!ENTITY sync.link.advancedsetup.label 'Advanced setup'>
<!ENTITY sync.link.nodevice.label 'I don\&apos;t have the device with me…'>
<!-- J-PAKE Waiting Screen -->

View File

@ -130,6 +130,17 @@ public class CryptoRecord extends Record {
super(source.guid, source.collection, source.lastModified, source.deleted);
}
@Override
public Record copyWithIDs(String guid, long androidID) {
CryptoRecord out = new CryptoRecord(this);
out.guid = guid;
out.androidID = androidID;
out.sortIndex = this.sortIndex;
out.payload = (this.payload == null) ? null : new ExtendedJSONObject(this.payload.object);
out.keyBundle = this.keyBundle; // TODO: copy me?
return out;
}
/**
* Take a whole record as JSON -- i.e., something like
*

View File

@ -185,8 +185,7 @@ public class GlobalSession implements CredentialsSource, PrefsSource {
config.syncKeyBundle = syncKeyBundle;
// clusterURL and syncID are set through `persisted`, or fetched from the server.
// TODO: populate saved configurations. We'll amend these after processing meta/global.
this.synchronizerConfigurations = new SynchronizerConfigurations(persisted);
assert(null == persisted);
prepareStages();
}
@ -696,23 +695,4 @@ public class GlobalSession implements CredentialsSource, PrefsSource {
}
return this.config.metaGlobal.engines.get(engineName) != null;
}
/**
* Return enough information to be able to reconstruct a Synchronizer.
*
* @param engineName
* @return
*/
public SynchronizerConfiguration configForEngine(String engineName) {
// TODO: we need an altogether better way of handling empty configs.
SynchronizerConfiguration stored = this.getSynchronizerConfigurations().forEngine(engineName);
if (stored == null) {
return new SynchronizerConfiguration(engineName, new RepositorySessionBundle(0), new RepositorySessionBundle(0));
}
return stored;
}
private SynchronizerConfigurations synchronizerConfigurations;
private SynchronizerConfigurations getSynchronizerConfigurations() {
return this.synchronizerConfigurations;
}
}

View File

@ -91,15 +91,6 @@ public class SynchronizerConfigurations {
engines = new HashMap<String, SynchronizerConfiguration>();
}
public void fillBundle(Bundle bundle) {
Bundle contents = new Bundle();
for (Entry<String, SynchronizerConfiguration> entry : engines.entrySet()) {
contents.putStringArray(entry.getKey(), entry.getValue().toStringValues());
}
contents.putInt("version", CONFIGURATION_VERSION);
bundle.putBundle("engines", contents);
}
public SynchronizerConfiguration forEngine(String engineName) {
return engines.get(engineName);
}

View File

@ -20,6 +20,7 @@
*
* Contributor(s):
* Richard Newman <rnewman@mozilla.com>
* Nick Alexander <nalexander@mozilla.com>
*
* Alternatively, the contents of this file may be used under the terms of
* either the GNU General Public License Version 2 or later (the "GPL"), or
@ -47,11 +48,16 @@ import org.mozilla.gecko.sync.ExtendedJSONObject;
import org.mozilla.gecko.sync.NonObjectJSONException;
import org.mozilla.gecko.sync.Utils;
import android.util.Log;
import ch.boye.httpclientandroidlib.Header;
import ch.boye.httpclientandroidlib.HttpEntity;
import ch.boye.httpclientandroidlib.HttpResponse;
import ch.boye.httpclientandroidlib.impl.cookie.DateParseException;
import ch.boye.httpclientandroidlib.impl.cookie.DateUtils;
public class SyncResponse {
private static final String HEADER_RETRY_AFTER = "retry-after";
private static final String LOG_TAG = "SyncResponse";
protected HttpResponse response;
@ -123,10 +129,20 @@ public class SyncResponse {
return this.response.containsHeader(h);
}
private int getIntegerHeader(String h) {
private static boolean missingHeader(String value) {
return value == null ||
value.trim().length() == 0;
}
private int getIntegerHeader(String h) throws NumberFormatException {
if (this.hasHeader(h)) {
Header header = this.response.getFirstHeader(h);
return Integer.parseInt(header.getValue(), 10);
String value = header.getValue();
if (missingHeader(value)) {
Log.w(LOG_TAG, h + " header present but empty.");
return -1;
}
return Integer.parseInt(value, 10);
}
return -1;
}
@ -135,7 +151,31 @@ public class SyncResponse {
* @return A number of seconds, or -1 if the header was not present.
*/
public int retryAfter() throws NumberFormatException {
return this.getIntegerHeader("retry-after");
if (!this.hasHeader(HEADER_RETRY_AFTER)) {
return -1;
}
Header header = this.response.getFirstHeader(HEADER_RETRY_AFTER);
String retryAfter = header.getValue();
if (missingHeader(retryAfter)) {
Log.w(LOG_TAG, "Retry-After header present but empty.");
return -1;
}
try {
return Integer.parseInt(retryAfter, 10);
} catch (NumberFormatException e) {
// Fall through to try date format.
}
try {
final long then = DateUtils.parseDate(retryAfter).getTime();
final long now = System.currentTimeMillis();
return (int)((then - now) / 1000); // Convert milliseconds to seconds.
} catch (DateParseException e) {
Log.w(LOG_TAG, "Retry-After header neither integer nor date: " + retryAfter);
return -1;
}
}
public int weaveBackoff() throws NumberFormatException {
@ -174,5 +214,4 @@ public class SyncResponse {
}
return null;
}
}

View File

@ -173,7 +173,7 @@ public class SyncStorageRequest implements Resource {
}
}
public static String USER_AGENT = "Firefox AndroidSync 0.3";
public static String USER_AGENT = "Firefox AndroidSync 0.4";
protected SyncResourceDelegate resourceDelegate;
public SyncStorageRequestDelegate delegate;
protected BaseResource resource;

View File

@ -56,7 +56,6 @@ import android.content.Context;
import android.database.Cursor;
import android.util.Log;
public class AndroidBrowserBookmarksRepositorySession extends AndroidBrowserRepositorySession {
// TODO: synchronization for these.

View File

@ -41,6 +41,7 @@ package org.mozilla.gecko.sync.repositories.android;
import java.util.ArrayList;
import java.util.HashMap;
import org.mozilla.gecko.sync.Utils;
import org.mozilla.gecko.sync.repositories.InactiveSessionException;
import org.mozilla.gecko.sync.repositories.InvalidRequestException;
import org.mozilla.gecko.sync.repositories.InvalidSessionTransitionException;
@ -342,6 +343,15 @@ public abstract class AndroidBrowserRepositorySession extends RepositorySession
this.fetchSince(0, delegate);
}
private void trace(String m) {
if (Utils.ENABLE_TRACE_LOGGING) {
if (Utils.LOG_TO_STDOUT) {
System.out.println(LOG_TAG + "::TRACE " + m);
}
Log.d(LOG_TAG, m);
}
}
@Override
public void store(final Record record) throws NoStoreDelegateException {
if (delegate == null) {
@ -363,9 +373,11 @@ public abstract class AndroidBrowserRepositorySession extends RepositorySession
return;
}
// Check that the record is a valid type
// TODO Currently for bookmarks we only take care of folders
// and bookmarks, all other types are ignored and thrown away
// Check that the record is a valid type.
// Fennec only supports bookmarks and folders. All other types of records,
// including livemarks and queries, are simply ignored.
// See Bug 708149. This might be resolved by Fennec changing its database
// schema, or by Sync storing non-applied records in its own private database.
if (!checkRecordType(record)) {
Log.d(LOG_TAG, "Ignoring record " + record.guid + " due to unknown record type.");
@ -386,11 +398,18 @@ public abstract class AndroidBrowserRepositorySession extends RepositorySession
record.androidID = insert(record);
} else if (existingRecord != null) {
// If the incoming record is marked deleted and
// our existing record has a newer timestamp, then
// discard the incoming record.
if (record.deleted && existingRecord.lastModified > record.lastModified) {
delegate.onRecordStoreSucceeded(record);
return;
}
// Now's a great time to do expensive additions.
existingRecord = transformRecord(existingRecord);
dbHelper.delete(existingRecord);
// Or clause: We won't store a remotely deleted record ever, but if it is marked deleted
// and our existing record has a newer timestamp, we will restore the existing record
if (!record.deleted || (record.deleted && existingRecord.lastModified > record.lastModified)) {
// Record exists already, need to figure out what to store
if (!record.deleted) {
// Record exists already, need to figure out what to store.
Record store = reconcileRecords(existingRecord, record);
record.androidID = insert(store);
}

View File

@ -95,6 +95,51 @@ public class BookmarkRecord extends Record {
parentID + "/" + androidParentID + "/" + parentName + ">";
}
// Oh God, this is terribly thread-unsafe. These record objects should be immutable.
@SuppressWarnings("unchecked")
protected JSONArray copyChildren() {
if (this.children == null) {
return null;
}
JSONArray children = new JSONArray();
children.addAll(this.children);
return children;
}
@SuppressWarnings("unchecked")
protected JSONArray copyTags() {
if (this.tags == null) {
return null;
}
JSONArray tags = new JSONArray();
tags.addAll(this.tags);
return tags;
}
@Override
public Record copyWithIDs(String guid, long androidID) {
BookmarkRecord out = new BookmarkRecord(guid, this.collection, this.lastModified, this.deleted);
out.androidID = androidID;
out.sortIndex = this.sortIndex;
// Copy BookmarkRecord fields.
out.title = this.title;
out.bookmarkURI = this.bookmarkURI;
out.description = this.description;
out.keyword = this.keyword;
out.parentID = this.parentID;
out.parentName = this.parentName;
out.androidParentID = this.androidParentID;
out.type = this.type;
out.pos = this.pos;
out.androidPosition = this.androidPosition;
out.children = this.copyChildren();
out.tags = this.copyTags();
return out;
}
@Override
public void initFromPayload(CryptoRecord payload) {
ExtendedJSONObject p = payload.payload;
@ -185,15 +230,14 @@ public class BookmarkRecord extends Record {
}
@Override
public boolean equals(Object o) {
trace("Calling BookmarkRecord.equals.");
if (!(o instanceof BookmarkRecord)) {
public boolean equalPayloads(Object o) {
trace("Calling BookmarkRecord.equalPayloads.");
if (o == null || !(o instanceof BookmarkRecord)) {
return false;
}
BookmarkRecord other = (BookmarkRecord) o;
if (!super.equals(other)) {
if (!super.equalPayloads(other)) {
return false;
}
@ -236,8 +280,15 @@ public class BookmarkRecord extends Record {
&& RepoUtils.stringsEqual(this.keyword, other.keyword)
&& jsonArrayStringsEqual(this.tags, other.tags);
}
// Converts to JSONArrays to strings and checks if they are the same.
// TODO: two records can be congruent if their child lists are different.
@Override
public boolean congruentWith(Object o) {
return this.equalPayloads(o) &&
super.congruentWith(o);
}
// Converts two JSONArrays to strings and checks if they are the same.
// This is only useful for stuff like tags where we aren't actually
// touching the data there (and therefore ordering won't change)
private boolean jsonArrayStringsEqual(JSONArray a, JSONArray b) {
@ -247,7 +298,6 @@ public class BookmarkRecord extends Record {
if (a != null && b == null) return false;
return RepoUtils.stringsEqual(a.toJSONString(), b.toJSONString());
}
}

View File

@ -83,6 +83,32 @@ public class HistoryRecord extends Record {
public long fennecDateVisited;
public long fennecVisitCount;
@SuppressWarnings("unchecked")
private JSONArray copyVisits() {
if (this.visits == null) {
return null;
}
JSONArray out = new JSONArray();
out.addAll(this.visits);
return out;
}
@Override
public Record copyWithIDs(String guid, long androidID) {
HistoryRecord out = new HistoryRecord(guid, this.collection, this.lastModified, this.deleted);
out.androidID = androidID;
out.sortIndex = this.sortIndex;
// Copy HistoryRecord fields.
out.title = this.title;
out.histURI = this.histURI;
out.fennecDateVisited = this.fennecDateVisited;
out.fennecVisitCount = this.fennecVisitCount;
out.visits = this.copyVisits();
return out;
}
@Override
public void initFromPayload(CryptoRecord payload) {
ExtendedJSONObject p = payload.payload;
@ -115,32 +141,62 @@ public class HistoryRecord extends Record {
return rec;
}
public boolean equalsExceptVisits(Object o) {
if (!(o instanceof HistoryRecord)) {
/**
* We consider two history records to be congruent if they represent the
* same history record regardless of visits.
*/
@Override
public boolean congruentWith(Object o) {
if (o == null || !(o instanceof HistoryRecord)) {
return false;
}
HistoryRecord other = (HistoryRecord) o;
return super.equals(other) &&
RepoUtils.stringsEqual(this.title, other.title) &&
if (!super.congruentWith(other)) {
return false;
}
return RepoUtils.stringsEqual(this.title, other.title) &&
RepoUtils.stringsEqual(this.histURI, other.histURI);
}
public boolean equalsIncludingVisits(Object o) {
@Override
public boolean equalPayloads(Object o) {
if (o == null || !(o instanceof HistoryRecord)) {
Log.d(LOG_TAG, "Not a HistoryRecord: " + o);
return false;
}
HistoryRecord other = (HistoryRecord) o;
return equalsExceptVisits(other) && this.checkVisitsEquals(other);
if (!super.equalPayloads(other)) {
Log.d(LOG_TAG, "super.equalPayloads returned false.");
return false;
}
return RepoUtils.stringsEqual(this.title, other.title) &&
RepoUtils.stringsEqual(this.histURI, other.histURI) &&
checkVisitsEquals(other);
}
@Override
/**
* We consider two history records to be equal if they represent the
* same history record regardless of visits.
*/
public boolean equals(Object o) {
return equalsExceptVisits(o);
public boolean equalAndroidIDs(Record other) {
return super.equalAndroidIDs(other) &&
this.equalFennecVisits(other);
}
private boolean equalFennecVisits(Record other) {
if (!(other instanceof HistoryRecord)) {
return false;
}
HistoryRecord h = (HistoryRecord) other;
return this.fennecDateVisited == h.fennecDateVisited &&
this.fennecVisitCount == h.fennecVisitCount;
}
private boolean checkVisitsEquals(HistoryRecord other) {
Log.d(LOG_TAG, "Checking visits.");
if (Utils.ENABLE_TRACE_LOGGING) {
Log.d(LOG_TAG, ">> Mine: " + ((this.visits == null) ? "null" : this.visits.toJSONString()));
Log.d(LOG_TAG, ">> Theirs: " + ((other.visits == null) ? "null" : other.visits.toJSONString()));
}
// Handle nulls.
if (this.visits == other.visits) {
return true;

View File

@ -75,6 +75,28 @@ public class PasswordRecord extends Record {
public long timeLastUsed;
public long timesUsed;
@Override
public Record copyWithIDs(String guid, long androidID) {
PasswordRecord out = new PasswordRecord(guid, this.collection, this.lastModified, this.deleted);
out.androidID = androidID;
out.sortIndex = this.sortIndex;
// Copy HistoryRecord fields.
out.hostname = this.hostname;
out.formSubmitURL = this.formSubmitURL;
out.httpRealm = this.httpRealm;
out.username = this.username;
out.password = this.password;
out.usernameField = this.usernameField;
out.passwordField = this.passwordField;
out.encType = this.encType;
out.timeLastUsed = this.timeLastUsed;
out.timesUsed = this.timesUsed;
return out;
}
@Override
public void initFromPayload(CryptoRecord payload) {
// TODO Auto-generated method stub
@ -88,10 +110,33 @@ public class PasswordRecord extends Record {
}
@Override
public boolean equals(Object o) {
if (!o.getClass().equals(PasswordRecord.class)) return false;
public boolean congruentWith(Object o) {
if (o == null || !(o instanceof PasswordRecord)) {
return false;
}
PasswordRecord other = (PasswordRecord) o;
if (!super.equals(other)) return false;
if (!super.congruentWith(other)) {
return false;
}
return RepoUtils.stringsEqual(this.hostname, other.hostname)
&& RepoUtils.stringsEqual(this.formSubmitURL, other.formSubmitURL)
&& RepoUtils.stringsEqual(this.httpRealm, other.httpRealm)
&& RepoUtils.stringsEqual(this.username, other.username)
&& RepoUtils.stringsEqual(this.password, other.password)
&& RepoUtils.stringsEqual(this.usernameField, other.usernameField)
&& RepoUtils.stringsEqual(this.passwordField, other.passwordField)
&& RepoUtils.stringsEqual(this.encType, other.encType);
}
@Override
public boolean equalPayloads(Object o) {
if (o == null || !(o instanceof PasswordRecord)) {
return false;
}
PasswordRecord other = (PasswordRecord) o;
if (!super.equalPayloads(other)) {
return false;
}
return RepoUtils.stringsEqual(this.hostname, other.hostname)
&& RepoUtils.stringsEqual(this.formSubmitURL, other.formSubmitURL)
&& RepoUtils.stringsEqual(this.httpRealm, other.httpRealm)

View File

@ -43,8 +43,62 @@ import java.io.UnsupportedEncodingException;
import org.mozilla.gecko.sync.CryptoRecord;
import org.mozilla.gecko.sync.ExtendedJSONObject;
/**
* Record is the abstract base class for all entries that Sync processes:
* bookmarks, passwords, history, and such.
*
* A Record can be initialized from or serialized to a CryptoRecord for
* submission to an encrypted store.
*
* Records should be considered to be conventionally immutable: modifications
* should be completed before the new record object escapes its constructing
* scope. Note that this is a critically important part of equality. As Rich
* Hickey notes:
*
* the only things you can really compare for equality are immutable things,
* because if you compare two things for equality that are mutable, and ever
* say true, and they're ever not the same thing, you are wrong. Or you will
* become wrong at some point in the future.
*
* Records have a layered definition of equality. Two records can be said to be
* "equal" if:
*
* * They have the same GUID and collection. Two crypto/keys records are in some
* way "the same".
* This is `equalIdentifiers`.
*
* * Their most significant fields are the same. That is to say, they share a
* GUID, a collection, deletion, and domain-specific fields. Two copies of
* crypto/keys, neither deleted, with the same encrypted data but different
* modified times and sortIndex are in a stronger way "the same".
* This is `equalPayloads`.
*
* * Their most significant fields are the same, and their local fields (e.g.,
* the androidID to which we have decided that this record maps) are congruent.
* A record with the same androidID, or one whose androidID has not been set,
* can be considered "the same".
* This concept can be extended by Record subclasses. The key point is that
* reconciling should be applied to the contents of these records. For example,
* two history records with the same URI and GUID, but different visit arrays,
* can be said to be congruent.
* This is `congruentWith`.
*
* * They are strictly identical. Every field that is persisted, including
* lastModified and androidID, is equal.
* This is `equals`.
*
* Different parts of the codebase have use for different layers of this
* comparison hierarchy. For instance, lastModified times change every time a
* record is stored; a store followed by a retrieval will return a Record that
* shares its most significant fields with the input, but has a later
* lastModified time and might not yet have values set for others. Reconciling
* will thus ignore the modification time of a record.
*
* @author rnewman
*
*/
public abstract class Record {
// TODO: consider immutability, effective immutability, and thread-safety.
public String guid;
public String collection;
public long lastModified;
@ -58,16 +112,22 @@ public abstract class Record {
this.lastModified = lastModified;
this.deleted = deleted;
this.sortIndex = 0;
this.androidID = -1;
}
@Override
public boolean equals(Object o) {
if (o == null) {
/**
* Return true iff the input is a Record and has the same
* collection and guid as this object.
*
* @param o
* @return
*/
public boolean equalIdentifiers(Object o) {
if (o == null || !(o instanceof Record)) {
return false;
}
Record other = (Record) o;
if (this.guid == null) {
if (other.guid != null) {
return false;
@ -77,7 +137,6 @@ public abstract class Record {
return false;
}
}
if (this.collection == null) {
if (other.collection != null) {
return false;
@ -87,13 +146,84 @@ public abstract class Record {
return false;
}
}
return true;
}
if (this.deleted != other.deleted) {
/**
* Return true iff the input is a Record which is substantially the
* same as this object.
*
* @param o
* @return
*/
public boolean equalPayloads(Object o) {
if (!this.equalIdentifiers(o)) {
return false;
}
Record other = (Record) o;
return this.deleted == other.deleted;
}
/**
* Return true iff the input is a Record which is substantially the
* same as this object, considering the ability and desire two
* reconcile the two objects if possible.
*
* @param o
* @return
*/
public boolean congruentWith(Object o) {
if (!this.equalIdentifiers(o)) {
return false;
}
Record other = (Record) o;
return congruentAndroidIDs(other) &&
(this.deleted == other.deleted);
}
public boolean congruentAndroidIDs(Record other) {
// We treat -1 as "unset", and treat this as
// congruent with any other value.
if (this.androidID != -1 &&
other.androidID != -1 &&
this.androidID != other.androidID) {
return false;
}
return true;
}
/**
* Return true iff the input is both equal in terms of payload,
* and also shares transient values such as timestamps.
*/
@Override
public boolean equals(Object o) {
if (o == null || !(o instanceof Record)) {
return false;
}
Record other = (Record) o;
return equalTimestamps(other) &&
equalSortIndices(other) &&
equalAndroidIDs(other) &&
equalPayloads(o);
}
public boolean equalAndroidIDs(Record other) {
return this.androidID == other.androidID;
}
public boolean equalSortIndices(Record other) {
return this.sortIndex == other.sortIndex;
}
public boolean equalTimestamps(Object o) {
if (o == null || !(o instanceof Record)) {
return false;
}
return ((Record) o).lastModified == this.lastModified;
}
public abstract void initFromPayload(CryptoRecord payload);
public abstract CryptoRecord getPayload();
@ -122,4 +252,15 @@ public abstract class Record {
throw new IllegalStateException(detailMessage);
}
}
/**
* Return an identical copy of this record with the provided two values.
*
* Oh for persistent data structures.
*
* @param guid
* @param androidID
* @return
*/
public abstract Record copyWithIDs(String guid, long androidID);
}