Skip to content

Commit

Permalink
Use the new adapter design for browser histroy.
Browse files Browse the repository at this point in the history
  • Loading branch information
weilonge committed Aug 20, 2015
1 parent 3082776 commit 2192fb5
Show file tree
Hide file tree
Showing 5 changed files with 104 additions and 17 deletions.
3 changes: 3 additions & 0 deletions apps/settings/index.html
Expand Up @@ -489,5 +489,8 @@ <h1 data-l10n-id="ums-warning-title">Enable USB storage?</h1>
<script src="js/datasync/hawk_creds.js"></script>
<script src="js/datasync/kinto.dev.js"></script>
<script src="js/datasync/synccrypto.js"></script>
<script src="js/datasync/history_adapter.js"></script>
<script src="js/datasync/sync-engine.js"></script>
<script src="js/datasync/fxsync-webcrypto.js"></script>
</body>
</html>
49 changes: 49 additions & 0 deletions apps/settings/js/datasync/history_adapter.js
@@ -0,0 +1,49 @@
var HistoryAPI = {
addPlace: function(place) {
console.log(place);
return IAC.request('sync-history', {
method: 'addPlace',
args: [place]
});
}
};

var SynctoHistoryAdapter = {
update: function(kintoCollection) {
kintoCollection.list().then(list => {
console.log('history adapter update function called with this local copy of the remote data:', list);

var historyRecords = list.data;
var partialRecoreds = historyRecords.slice(0, 10);

This comment has been minimized.

Copy link
@michielbdejong

michielbdejong Aug 20, 2015

typo Recor[e]ds

historyRecords.forEach(function (decryptedRecord){
var record = decryptedRecord.payload
if(!record.histUri || !record.visits || !record.visits[0]){
//console.log('invalid history: ', record);
return ;
}

console.log('decrypted record', record);

This comment has been minimized.

Copy link
@michielbdejong

michielbdejong Aug 20, 2015

stray console.log statement from debugging

This comment has been minimized.

Copy link
@weilonge

weilonge Aug 20, 2015

Author Owner

shall we remove line 21 or line 25? I would like to remove them both.

This comment has been minimized.

Copy link
@michielbdejong

michielbdejong Aug 20, 2015

yes, both! :)
and also the stray space on line 22, I guess.


var visits = [];
record.visits.forEach(function (elem){
visits.push(elem.date);
});

var place = {
url: record.histUri,
title: record.title,
visits: visits,
visited: record.visits[0]
};
HistoryAPI.addPlace(place).then(function (d){
console.log(d);
}, function (e){
console.log(e);
});
document.querySelector('#sync-time').textContent =

This comment has been minimized.

Copy link
@michielbdejong

michielbdejong Aug 20, 2015

This line is not related to the history adapter, we need to remember to move it somewhere else

This comment has been minimized.

Copy link
@weilonge

weilonge Aug 20, 2015

Author Owner

I think this line would be moved after Settings app can get the last sync time from Scheduler (System app). Could we leave it here currently?

This comment has been minimized.

Copy link
@weilonge

weilonge Aug 20, 2015

Author Owner

update function is implemented again, so we can do DOM operation after its promise.resolve().

new Date().toString();
});

});
}
};
43 changes: 27 additions & 16 deletions apps/settings/js/fxsync.js
Expand Up @@ -3,7 +3,7 @@

'use strict';

const REMOTE = "http://69b5bd29.ngrok.io/v1/"
const REMOTE = "http://localhost:8000/v1/"

function toCamelCase(str) {
var rdashes = /-(.)/g;
Expand Down Expand Up @@ -102,24 +102,21 @@ var SyncCredentials = {
}
};

var HistoryAdapter = {
addPlace: function(place) {
console.log(place);
return IAC.request('sync-history', {
method: 'addPlace',
args: [place]
});
}
};

define('fxsync', ['modules/settings_utils', 'shared/settings_listener'
], function(SettingsUtils, SettingsListener) {
var FxSync = {
_adapters: {},
_credentialCache: {},
init: function fmd_init() {
this.syncButton = document.querySelector('#sync-button');
this.syncButton.
addEventListener('click', FxSync.syncHistory.bind(FxSync));
SyncCrypto.assignApp(FxSync);
this.registerAdapter('history', SynctoHistoryAdapter);
},

registerAdapter: function(collectionName, adapter) {
this._adapters[collectionName] = adapter;
},

ensureDb: function(assertion) {
Expand All @@ -128,6 +125,14 @@ define('fxsync', ['modules/settings_utils', 'shared/settings_listener'
return this._db;
}
return SyncCredentials.getXClientState().then(xClientState => {
this._credentialCache = {
synctoCredentials: {
URL: REMOTE,
assertion: assertion,
xClientState: xClientState
}
};

this._db = new Kinto({
bucket: 'syncto',
remote: REMOTE,
Expand Down Expand Up @@ -263,11 +268,17 @@ define('fxsync', ['modules/settings_utils', 'shared/settings_listener'

syncHistory: function() {
console.log('Retrieving history collection... this may take several minutes on first run');
this.getHistoryCollection().then(history => {
console.log('History ', history);
history.sync().then(result => {
console.log('Sync results ', result);
this.storeHistoryToDS(result);
this.ensureDb().then(db => {
SyncCredentials.getKeys().then(credentials => {
this._credentialCache.kB = credentials.kB;
document.querySelector('#sync-account').textContent = credentials.email;

var se = new SyncEngine(this._credentialCache.synctoCredentials, ['history'], this._credentialCache.kB);
se.connect().then(() => {
return se.syncNow();
}).then(() => {
this._adapters.history.update(se._collections.history);
});
});
});
},
Expand Down
1 change: 1 addition & 0 deletions apps/settings/locales/settings.en-US.properties
Expand Up @@ -844,6 +844,7 @@ fxsync-sync-now=Sync now
fxsync-collections=Collections
fxsync-collections-bookmarks=Bookmarks
fxsync-collections-history=History
fxsync-collections-tabs=Tabs
fxsync-collections-password=Password

#=#=#=#=#=#=#=#=#=#=#=#=#=#=#=#=#=#=#=#=#=#=#=#=#=#=#=#=#=#=#=#=#=#=#=#=#=#=#=#
Expand Down
25 changes: 24 additions & 1 deletion apps/system/js/sync/sync_iac_api.js
Expand Up @@ -10,6 +10,18 @@
* consumed by the System app, so there will be no need to expose anything.
*/

function arrayUnique(array) {
var a = array.concat();

This comment has been minimized.

Copy link
@michielbdejong

michielbdejong Aug 20, 2015

Why .concat()? Is that a way to clone the Array?

This comment has been minimized.

Copy link
@weilonge

weilonge Aug 20, 2015

Author Owner

yes, this is for cloning an array, but it will be removed when the new merging algorithm is adopted.

for(var i=0; i<a.length; ++i) {
for(var j=i+1; j<a.length; ++j) {
if(a[i] === a[j])
a.splice(j--, 1);
}
}

return a;
};

This comment has been minimized.

Copy link
@michielbdejong

michielbdejong Aug 20, 2015

Does this function have a unit test?

This comment has been minimized.

Copy link
@weilonge

weilonge Aug 20, 2015

Author Owner

yes, I will implement the test for new merging algorithm, but it will be removed as well.


(function() {

function sendPortMessage(portName, message) {
Expand Down Expand Up @@ -52,9 +64,20 @@
function syncHistroyRequest(id, method, args){
var url = args[0].url;
var visits = args[0].visits;
var title = args[0].title;

var places = appWindowManager.places;
places.setVisits(url, visits);
places.editPlace(url, (place, cb) => {

This comment has been minimized.

Copy link
@michielbdejong

michielbdejong Aug 20, 2015

does this function also automatically create a new place if no place for url exist yet?

This comment has been minimized.

Copy link
@weilonge

weilonge Aug 20, 2015

Author Owner

yes, editPlace will create a new place if it doesn't exist.
https://github.com/weilonge/gaia/blob/syncto.poc/apps/system/js/places.js#L215

if(place.title === place.url){

This comment has been minimized.

Copy link
@michielbdejong

michielbdejong Aug 20, 2015

why not always update place.title to title? Maybe the title of the place changed since the last visit.

This comment has been minimized.

Copy link
@weilonge

weilonge Aug 20, 2015

Author Owner

okay, I will remove the condition.

place.title = title;
}
place.visits = place.visits || [];

This comment has been minimized.

Copy link
@michielbdejong

michielbdejong Aug 20, 2015

Is this necessary?

This comment has been minimized.

Copy link
@weilonge

weilonge Aug 20, 2015

Author Owner

https://github.com/weilonge/gaia/blob/syncto.poc/apps/system/js/places.js#L237
that's how places library did, so I think that's necessary.

This comment has been minimized.

Copy link
@michielbdejong

michielbdejong Aug 20, 2015

ok!

place.visits = arrayUnique(place.visits.concat(visits));
place.visits.sort((a, b) => {
return b - a;
});

This comment has been minimized.

Copy link
@michielbdejong

michielbdejong Aug 20, 2015

Would be nice to have a unit test for this as well, to confirm that visits get sorted in the right way (is it newest first?)

This comment has been minimized.

Copy link
@weilonge

weilonge Aug 20, 2015

Author Owner

yes, I will add the test for the new merging algorithm.

cb(place);
});
}

/**
Expand Down

5 comments on commit 2192fb5

@michielbdejong
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the way you merge the two sorted arrays place.visits and visits, by first concatenating them, then splicing out the duplicates in a double for-loop, and then sorting again, is probably not the most efficient algorithm. How about this alternative:

for  (var i=0; i<visits.length; i++) {
  if (place.visits.indexOf(visits[i]) === -1) {
    place.visits.push(visits[i]);
  }
}
place.visits.sort(function(a, b) {
  return b - a;
});

Seems simpler and possibly faster?

@michielbdejong
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel we need at least one unit test for the history adapter. Maybe just add it under https://github.com/weilonge/gaia/tree/master/apps/settings/test/unit for now, and then we can move it along with the code once we have the Synchronizer app to land this in.

@michielbdejong
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This adapter 'syncs down' visits to places that exist on the Sync server but not in the places DataStore. But that's only step 1, we also need to 'sync up', so iterate over the places and their visits and check if they all exist in kintoCollection. If there is a place that's missing from the kintoCollection, use kintoCollection.add(). If there is a place which is missing some visits in the kintoCollection, add the missing visits using http://kintojs.readthedocs.org/en/latest/api/#updating-a-record.

Since Kinto.js does not have auto-batching, and batch operations are not supported yet, this is going to very slow when there are hundreds of records to add or update. It's important for performance to do all these add and update operations in a batch operation, so that they go into one IndexedDB transaction instead of hundreds of slow IndexedDB transactions. But for now, let's just not batch them, and add a code comment, referring to Kinto/kinto.js#16, which mentions this important performance issue (we could do some measurements to confirm exactly how slow it is).

@michielbdejong
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, and I forgot, we also need a function HistoryAdapter#handleConflict, so we can do something like this:

var conflict = {
  type: "incoming", // can also be "outgoing"
  local: {
    _status: "created",
    id: "233a018a-fd2b-4d39-ba85-8bf3e13d73ec",
    title: "local title",
  },
  remote: {
    id: "233a018a-fd2b-4d39-ba85-8bf3e13d73ec",
    title: "remote title",
  }
};
myHistoryAdapter.handleConflict(conflict);

The only type of conflict we need to worry about is if one URL was visited both on desktop and on the phone. So you can always just:

  • merge the visits arrays
  • use the title of the one which was visited lasted (title should never be different unless the visits are also different)

So something like:

function handleConflict(conflict) {
  var merged = conflict.local;

  // Merge visits from remote into visits from local:
  for  (var i=0; i<conflict.remote.visits.length; i++) {
    if (merged.visits.indexOf(conflict.remote.visits[i]) === -1) {
      merged.visits.push(conflict.remote.visits[i]);
    }
  }
  merged.visits.sort(function(a, b) {
    return b - a;
  });

  // Use title from remote if more recent:
  if (conflict.remote.visited > merged.visited) {
    merged.title = conflict.remote.title;
  }

  return merged;
}

@weilonge
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey Michiel,

Thanks a lot for your comments, here are my feedback for them:

  1. I think the way you proposed is simpler. The precise performance will be decided after doing benchmark. (Just reading the code, I think that would be faster :-) ) So I will adopt your suggestion and remove the deduplication function.
  2. The test is done in the new PR[1].
  3. For 'sync up' case, I have some ideas to share. Any places modification should be monitored, and [places.js#editPlace] is the only entry point to put a place object as far as I know. We can modify [places.js#editPlace] to mark an attribute or enqueue the new information to a "waiting for sync-up" queue. When sync-up request is coming, upload the places in the queue. I would like to consult with Dale Harvey who is the author of places.js (git blame), so we probably can discuss this issue (correctness, performance, and test-case) in another bug.
  4. May I know where is the conflict object from? (kinto.js?) "last_modified" in a decrypted record is an important property for deciding the merging strategy. I did this in the new PR as well.

Besides, the suggestions of your above comment is implemented in the new PR[1] except item3, please help to review it. let's move the discussion to the new one. Thank you! :)

[1] #2

Please sign in to comment.