1191773 - Read-only History data adapter

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Sync, defect)

Description

[Fernando Jiménez Moreno [:ferjm]]

We are going to be getting history records from Firefox Sync, but the data model will be different from the one that we currently have for FirefoxOS history. We need to create a data adapter.

Comment 1

[Michiel de Jong [:michielbdejong]]

Synchronizer can expose a kinto.js collection to the adapter, which the adapter can query with for instance `db().collection('history').list()`.

For the two-way merge, it probably makes sense to take all history entries from synchronizer's database, add all the entries from the device's DataStore for history, and take the union of those two sets. For each entry that was not in the DataStore, add it to the DataStore. For each entry that was not in synchronizer's database, add it to synchronizer's database.

If both synchronizer's database and the DataStore have a history entry for http://example.com/, look at the visits arrays, and merge those. So for instance if `db().collection('history').list()` contains an entry:

{ url: 'http://example.com', visits: [1234567890123, 1234567890124] }

And DataStore has an entry:

{ url: 'http://example.com', visits: [1234567890123, 1234567890125] }

Then update both those entries to:

{ url: 'http://example.com', visits: [1234567890123, 1234567890124, 1234567890125] }

Does that make sense?

Comment 2

[Mauricio Navarro Miranda]

Can I help with this?
I'm not yet a ninja programmer, but If you can point me in the right direction, I'll do my best on it. 

:)

Comment 3

[Sean Lee [:seanlee][:weilonge]]

Hey Mauricio,

In this bug, history adapter is the first one that we have to implement, and there is still some direction have to be discussed.
We will have some other adapters needed to be implemented as well, so probably you can join us for these adapters.
Thanks for your passion and do not hesitate to give us any suggestions! :)

Comment 4

[Michiel de Jong [:michielbdejong]]

@seanlee Do you think the history adapter can be done as a kinto.js custom database adapter? 

http://kintojs.readthedocs.org/en/latest/extending/#custom-database-adapters

Then we can skip the extra step of storing data in IDB first, see https://github.com/Kinto/kinto.js/issues/116

Comment 5

[Sean Lee [:seanlee][:weilonge]]

Hi Michiel,

Do you agree that bug 1195647 blocks this bug?
When the synchronizer is almost done, the adapter would be easier to implement based on the latest synchronizer.
Another idea is that we can treat this bug as history adapter's POC, and we've done it at POC demo.

Could you give some suggestions? Thank you.

Comment 6

[Michiel de Jong [:michielbdejong]]

Let me create an example adapter in the synchronizer code I have so far, for you to work with.

Comment 7

[Sean Lee [:seanlee][:weilonge]]

Attachment: Use the new adapter design for browser histroy.

Hi Michiel,

The attachment is the new adapter design for browser history based on your example code[1].

The history adapter is in three parts:
  apps/settings/js/datasync/history_adapter.js
  apps/system/js/sync/sync_iac_api.js 
  apps/system/js/places.js

places.js is a library for accessing Places Data Store.
Based on places.js, sync_iac_api.js will handle the records from history_adapter.js and merging the records [2].

Please give me a feedback for this patch. Thank you! :)
[1] https://github.com/michielbdejong/synchronizer-app/commit/230d8879eec902749711f17a4abfb986dffaed08
[2] https://github.com/weilonge/gaia/commit/2192fb5e32cf6514ccd67098b1916c50d3abb1ef#diff-64d5ca74ec66e7f6125ea7657110e2dfL57

Comment 8

[Michiel de Jong [:michielbdejong]]

Great stuff! I commented on github.

Comment 9

[Sean Lee [:seanlee][:weilonge]]

Attachment: Use the new adapter design for browser histroy.

Hey Michiel,

Please help to give a feedback for this updated one. Thank you!

Comment 10

[Michiel de Jong [:michielbdejong]]

Sean and I brainstormed about this today, using Vidyo and https://etherpad.mozilla.org/ntqQBU1yOF

We discovered that:
* We can iterate over the DataStore using the DataStore.sync() cursor. This gives us a change log in reverse order.
* We can iterate over the Kinto collection using collection.list(). This gives us the history entries in reverse last_modified order (so not the same thing).

We drafted an algorithm to iterate over these two lists side-by-side, and update in both directions, until we hit the point where sync was last run.

Then we discovered that the history data stored by FxDesktop is not a simple collection. Some entries have a 'deleted: true' field instead of the normal 'histUri, visits, title, ...' fields. They do have an 'id' field, though. So for these, we need to find the matching entry to know which URL this is about, before this delete action can be applied to the DataStore.

At this point, it's probably best to see how FxDesktop and Fennec have implemented their 'HistoryAdapter', and learn from that.

Comment 11

[Michiel de Jong [:michielbdejong]]

Also, the format of https://github.com/weilonge/gaia/blob/syncto.poc/apps/system/js/places.js#L327-L330 is different from the format of https://github.com/michielbdejong/fxsync-webcrypto/blob/initial-implementation/test/fixtures/fxsync-webcrypto-test.js#L36-L39 so that requires some thought as well.

All in all, this bug is a lot more work than we thought a few days ago! :)

Comment 12

[Michiel de Jong [:michielbdejong]]

(trying to unset 'feedback' flag by replying directly to comment 5)

Comment 13

[Sean Lee [:seanlee][:weilonge]]

Thanks for Michiel's review, and we had a discuss last Friday. For the current status, attachment 8650712 [details] [review] provides a better design for Data Sync architecture, but 'sync up' and merging records are needed more work to finish it. Here are the next steps for these new work:
1. 'sync up' history data from FxSync and FxOS. We thought they are data records, actually they are commit records (FxSync collection and DataStore.sync). First, the last merged record should be found, and collect the commit records between the latest one and the last merged one (the latest difference of commit record). All the data should be merged correctly if the latest difference is replayed to FxSync and FxOS. The sub-task is :
    1.1 history URL is missing when it's a 'deleted' record from FxSync. We have to find the exact URL for a deleting record.
    1.2 When there are two or more visit records should be merged into one record, we have to merge and convert between DataStore[1] and SyncCollection[2]

2. In the current patch, it uses the Places Service in System app to store the new history records from FxSync. We have two options to access the history data in FxOS via Places Service in System app or Data Store directly:
  * If we use DataStore directly, it's nicer from an architecture/permissions point of view, but it also means that top-sites will not get updated with sites from Desktop until you visit them with phone at least once after that.
  * find out what writeInProgress boolean is for, and if we break system app when we write directly to the DataStore.

[1] https://github.com/weilonge/gaia/blob/syncto.poc/apps/system/js/places.js#L327-L330
[2 ]https://github.com/michielbdejong/fxsync-webcrypto/blob/initial-implementation/test/fixtures/fxsync-webcrypto-test.js#L36-L39

Comment 14

[Sean Lee [:seanlee][:weilonge]]

Hi Dale, Fernando,

Places Service is placed in System app, and the history adapter uses IAC to push the history from Firefox Sync in my current design. Places Service accesses Places Data Store directly. How do you think if the history adapter accesses the data store directly? The below comments are the detail of our concern.

Thank you.

(In reply to Sean Lee [:seanlee] from comment #13)
> 2. In the current patch, it uses the Places Service in System app to store
> the new history records from FxSync. We have two options to access the
> history data in FxOS via Places Service in System app or Data Store directly:
>   * If we use DataStore directly, it's nicer from an
> architecture/permissions point of view, but it also means that top-sites
> will not get updated with sites from Desktop until you visit them with phone
> at least once after that.
>   * find out what writeInProgress boolean is for, and if we break system app
> when we write directly to the DataStore.
>

Comment 15

[Fernando Jiménez Moreno [:ferjm]]

(In reply to Sean Lee [:seanlee] from comment #14)
> Hi Dale, Fernando,
> 
> Places Service is placed in System app, and the history adapter uses IAC to
> push the history from Firefox Sync in my current design. Places Service
> accesses Places Data Store directly. How do you think if the history adapter
> accesses the data store directly? The below comments are the detail of our
> concern.
> 
> Thank you.
> 
> (In reply to Sean Lee [:seanlee] from comment #13)
> > 2. In the current patch, it uses the Places Service in System app to store
> > the new history records from FxSync. We have two options to access the
> > history data in FxOS via Places Service in System app or Data Store directly:
> >   * If we use DataStore directly, it's nicer from an
> > architecture/permissions point of view, but it also means that top-sites
> > will not get updated with sites from Desktop until you visit them with phone
> > at least once after that.

I don't understand this. Could you elaborate more, please?

> >   * find out what writeInProgress boolean is for, and if we break system app
> > when we write directly to the DataStore.
> >

Yes, please :)

The one that should be writing Firefox Sync history data into the Places DataStore should be the history adapter living in the Synchronizer app. The model I have in mind is something like this:

- The Synchronizer app needs to listen for Search DataStore changes. This can be done by listening to the "datastore-update-search" system message. This way we will be able to update our Firefox Sync data (via kinto.js + Syncto) once a new url is browsed from Firefox OS.

- The Synchronizer app needs to be able to read/write on the Search DataStore. This can be done by adding the "search" DataStore entry into the "datastores-access" manifest field of the Synchronizer app with "readwrite" access.

- The flows:

-- A URL is browsed/removed from Firefox OS. This will update the Search DataStore. Because the Synchronizer app is listening for Search DataStore changes, it will be woken up. Once it is up, it will need to update it's local history kinto collection and sync the changes with the Syncto server so they are available in other Sync enabled products (Desktop, Android, iOS).

-- A new URL is browsed/removed from a Sync enabled product other than Firefox OS. This will update the Firefox Sync server history collection. On Firefox OS the a sync request is received (because the requestSync timer fires or the user requests a sync on demand via Settings). The State Machine opens the Synchronizer app via IAC. The Synchronizer app gets the changes in the Firefox Sync server history collection via kinto.js + Syncto and writes the changes in the local history kinto collection and the Search DataStore.

Does that make sense?

Comment 16

[Michiel de Jong [:michielbdejong]]

> Because the Synchronizer app is listening for Search DataStore changes, it will be woken up. Once it is
> up, it will need to update it's local history kinto collection and sync the changes with the Syncto
> server so they are available in other Sync enabled products (Desktop, Android, iOS).

I would use https://developer.mozilla.org/en-US/docs/Web/API/DataStore/sync instead of https://developer.mozilla.org/en-US/docs/Web/API/DataStore/onchange, then we are not waking up the Synchronizer app on every page load, but rather every 10 minutes, to sync up the changes from the last 10 minutes in a batch.

Comment 17

[Sean Lee [:seanlee][:weilonge]]

Hey Fernando,

Thanks a lot for your suggestions. :)
I reply your comments as below.

(In reply to Fernando Jiménez Moreno [:ferjm] from comment #15)
> (In reply to Sean Lee [:seanlee] from comment #14)
> > Hi Dale, Fernando,
> > 
> > Places Service is placed in System app, and the history adapter uses IAC to
> > push the history from Firefox Sync in my current design. Places Service
> > accesses Places Data Store directly. How do you think if the history adapter
> > accesses the data store directly? The below comments are the detail of our
> > concern.
> > 
> > Thank you.
> > 
> > (In reply to Sean Lee [:seanlee] from comment #13)
> > > 2. In the current patch, it uses the Places Service in System app to store
> > > the new history records from FxSync. We have two options to access the
> > > history data in FxOS via Places Service in System app or Data Store directly:
> > >   * If we use DataStore directly, it's nicer from an
> > > architecture/permissions point of view, but it also means that top-sites
> > > will not get updated with sites from Desktop until you visit them with phone
> > > at least once after that.
> 
> I don't understand this. Could you elaborate more, please?
> 

Places Service[1] in System app is to manage the browser history stored in
'places' Data Store[2].
[1] https://github.com/weilonge/gaia/blob/syncto.poc/apps/system/js/places.js
[2] https://github.com/weilonge/gaia/blob/syncto.poc/apps/system/js/places.js#L34

Search app accesses places as well, but it only has 'readonly' permission [3].
[3] https://github.com/weilonge/gaia/blob/syncto.poc/apps/search/manifest.webapp#L48

So I suppose that Places Service is the only entry to access places data store.

For history adapter's design, I wonder that history adapter is to access places
data store directly, or use IAC to access it.

|  Synchronizer app  | sync_iac_api |    System app     |
| history adapter[4] |      ->      | places adapter[5] |
|                    |              | places service[6] |

[4] https://github.com/weilonge/gaia/pull/2/files#diff-ab4ff88f363d13281c373660010c233aR21
[5] https://github.com/weilonge/gaia/pull/2/files#diff-8a2bb9b312066a5a859bf2cb42f1c06aR12
[6] https://github.com/weilonge/gaia/pull/2/files#diff-64d5ca74ec66e7f6125ea7657110e2dfR59

> > >   * find out what writeInProgress boolean is for, and if we break system app
> > > when we write directly to the DataStore.
> > >

I suppose that all variables used by places.js should be considered including
writeInProgress and others.
https://github.com/weilonge/gaia/blob/syncto.poc/apps/system/js/places.js#L48-L79

> 
> Yes, please :)

I wish Dale can give us some suggestions here. He did some changes for places.js. :)

> 
> The one that should be writing Firefox Sync history data into the Places
> DataStore should be the history adapter living in the Synchronizer app. The
> model I have in mind is something like this:
> 
> - The Synchronizer app needs to listen for Search DataStore changes. This
> can be done by listening to the "datastore-update-search" system message.
> This way we will be able to update our Firefox Sync data (via kinto.js +
> Syncto) once a new url is browsed from Firefox OS.

Is Search DataStore the same with browser history(places data store)?
I suppose that Synchronizer app will handle the request only from Scheduler
(system app), but "datastore-update-search" system message is an important
information for system app to decide the synchronization timing.
(Don't have to do polling things.)

> 
> - The Synchronizer app needs to be able to read/write on the Search
> DataStore. This can be done by adding the "search" DataStore entry into the
> "datastores-access" manifest field of the Synchronizer app with "readwrite"
> access.

That's why I wonder above and would discuss Places Service above. 

> 
> - The flows:
> 
> -- A URL is browsed/removed from Firefox OS. This will update the Search
> DataStore. Because the Synchronizer app is listening for Search DataStore
> changes, it will be woken up. Once it is up, it will need to update it's
> local history kinto collection and sync the changes with the Syncto server
> so they are available in other Sync enabled products (Desktop, Android, iOS).
> 
> -- A new URL is browsed/removed from a Sync enabled product other than
> Firefox OS. This will update the Firefox Sync server history collection. On
> Firefox OS the a sync request is received (because the requestSync timer
> fires or the user requests a sync on demand via Settings). The State Machine
> opens the Synchronizer app via IAC. The Synchronizer app gets the changes in
> the Firefox Sync server history collection via kinto.js + Syncto and writes
> the changes in the local history kinto collection and the Search DataStore.
> 
> Does that make sense?

Comment 18

[Fernando Jiménez Moreno [:ferjm]]

Yes, of course! I was referring to the Places DataStore. Forget about the Search DS. Sorry about the confusion.

Also, we shouldn't be using IAC between Synchronizer and System to write in the Places DS. That's why we have DataStore for. We can directly write into the DS from Synchronizer.

Comment 19

[Fernando Jiménez Moreno [:ferjm]]

(In reply to Michiel de Jong [:michielbdejong] from comment #16)
> > Because the Synchronizer app is listening for Search DataStore changes, it will be woken up. Once it is
> > up, it will need to update it's local history kinto collection and sync the changes with the Syncto
> > server so they are available in other Sync enabled products (Desktop, Android, iOS).
> 
> I would use https://developer.mozilla.org/en-US/docs/Web/API/DataStore/sync
> instead of
> https://developer.mozilla.org/en-US/docs/Web/API/DataStore/onchange, then we
> are not waking up the Synchronizer app on every page load, but rather every
> 10 minutes, to sync up the changes from the last 10 minutes in a batch.

Good point.

We'd have to use DS.sync anyway. The question is if we want to wake the Synchronizer app (now renamed to Sync :)) up every time a page is loaded or we can defer to the requestSync timer to wake it up to update the Places DS. And you are probably right that we can wait for the requestSync timer to fire. In any case, the user can always request a sync on demand from Settings.

Comment 20

[Dale Harvey (:daleharvey)]

> >   * find out what writeInProgress boolean is for, and if we break system app
> > when we write directly to the DataStore.

So the |writeInProgress| is pretty relevant, its a hacky solution for transactions to ensure we get consistent writes, DataStore has no transactions so if we have a bunch of read/update/writes happening at the same time we can lose data, that will be possible if the synchroniser is talking to the datastore as well.

However after implementing that DataStore has added the abilty to specify a revision when writing (https://developer.mozilla.org/en-US/docs/Web/API/DataStore/put)  which can offer the same guarantees, so please make sure if you ever do a |.put()|, use the |revisionId| parameter.

And the rest looks good, as for how to write directly to the datastore, I would like a common interface to be used for both the remote and the local visits, so an update from the sync data results in a call to |https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/places.js#L235| This have a single storage API and can change the format, do common validation (and transaction) logic in one place, if we write directly to the store in 2 places it seems fairly possible for them to drift out of sync when we change things

Comment 21

[Sean Lee [:seanlee][:weilonge]]

Hi Dale,

Thanks for your comments. :) You point out what I concerned at comment 13, and the idea of writing DataStore directly should be discussed more.

How do you think if we move places.js to shared folder?

IMHO, moving places.js to shared folder can resolve the issue that drifting out of sync, but two Places Services instance at runtime might cause data conflict problem if an instance has their own local data/variable, e.g. writeInProgress, topSites, etc. (local data/variable doesn't include Places DataStore.)

Could you give some suggestions?

Thank you.

Comment 22

[Michiel de Jong [:michielbdejong]]

> move places.js to shared folder?

I would think that if writeInProgress is a variable, its value would not be shared between apps? So then that approach would not work? Sounds to me like it would make most sense to use DataStore as the safe concurrent interface between apps that share data (after all, that's one of the things it's designed for, right?), so that would mean changing the system app so that it uses |.put()| with the |revisionId| parameter, and then the history adapter can safely use the DataStore interface.

I also see an advantage in this approach for all the other adapters we will be writing in the future (I guess we eventually want one adapter for each DataStore that exists on the device?), so that's nice for code maintainability if all the adapters work in the same way.

My 2ct.

Comment 23

[Michiel de Jong [:michielbdejong]]

Handling records like `{ id: 'abcdefghijk', deleted: true }` will now be done automatically by the SyncEngine, using a custom kinto.js IdSchema, so removing dependency on bug 1200156.

Comment 24

[Aaron Wu]

Thanks Michiel for providing more info. Sean will keep working on that and complete by Sprint 7.

Comment 25

[Dale Harvey (:daleharvey)]

> How do you think if we move places.js to shared folder?

Sorry I missed this needinfo, I definitely think putting the API in /shared/ is a good idea if it will be used across different application, and that is the approach I would recommend, Cheers

Comment 26

[Sean Lee [:seanlee][:weilonge]]

Attachment: Histroy Adapter based on latest Sync Engine

Comment 27

[Sean Lee [:seanlee][:weilonge]]

Hi Fernando, Michiel,

I have some progress for HistoryAdapter. It can merge every history record when the sync request is triggered. The push feature is implemented in this patch as well, but I got an error that syncto seems no put/post operation support when I try to create a record to History collection. The error object is here:
error: {
  code: 405,
  errno: 999,
  error: "Method Not Allowed",
  message: "Failed batch subrequest"
}

Probably I use the incorrect version or miss something. Could you give me some suggestions about the error? Thank you!

Comment 28

[Michiel de Jong [:michielbdejong]]

(In reply to Sean Lee [:seanlee] from comment #27)
> Hi Fernando, Michiel,
> 
> I have some progress for HistoryAdapter. It can merge every history record
> when the sync request is triggered. The push feature is implemented in this
> patch as well

Wow, awesome!! \o/

> Probably I use the incorrect version or miss something. Could you give me
> some suggestions about the error? Thank you!

Which version of Syncto are you using, the 6-original-ids branch? That makes sense then that you get that error, wait for it to be merged in master (probably today or tomorrow), and then use the master branch of Syncto.

Comment 29

[Sean Lee [:seanlee][:weilonge]]

Hey Michiel,

(In reply to Michiel de Jong [:michielbdejong] from comment #28)
> Which version of Syncto are you using, the 6-original-ids branch? That makes
> sense then that you get that error, wait for it to be merged in master
> (probably today or tomorrow), and then use the master branch of Syncto.

Yes, I do use 6-original-ids branch. I will test it again when Syncto updates.

Another question, bug 1196239 is landed at master. May I integrate History Adapter to Sync app in master? Eventually, I suppose the adapter should be placed in apps/sync/js/adapters

Comment 30

[Michiel de Jong [:michielbdejong]]

Yes! Please do so with the following syntax:

/* global DataAdapters */

DataAdapters.history = {
  update(kintoCollection) {
    // Your code ...
    return Promise.resolve();
  },

  handleConflict(conflict) {
    // Your code ...
    return Promise.resolve(conflict.local);
  }
};

That way I can easily import it in the launcher code (more easily than with /* exports HistoryAdapter */).

Comment 31

[Michiel de Jong [:michielbdejong]]

Actually, small adjustment - can you return true from update if there were writes to kintoCollection, and false if not? That way, we can avoid doing the second sync (syncing changes up) if it's not necessary. So then it would be:

/* global DataAdapters */

DataAdapters.history = {
  update(kintoCollection) {
    // Your code ...
    return Promise.resolve(false);
  },

  handleConflict(conflict) {
    // Your code ...
    return Promise.resolve(conflict.local);
  }
};

Comment 32

[GitHub Autolander Bot]

Attachment: [gaia] weilonge:seanlee/DataSync/master/Bug1191773 > mozilla-b2g:master

Comment 33

[Sean Lee [:seanlee][:weilonge]]

Comment on attachment 8662667 [details] [review]
[gaia] weilonge:seanlee/DataSync/master/Bug1191773 > mozilla-b2g:master

Hi Fernando, Michiel,

Could you help to review my PR for Read-only History Adapter?
Thank you.

Comment 34

[Sean Lee [:seanlee][:weilonge]]

Even a read-only history adapter is in reviewing, the two-way sync feature is implemented on attachment 8661664 [details] [review].

Comment 35

[Michiel de Jong [:michielbdejong]]

Comment on attachment 8662667 [details] [review]
[gaia] weilonge:seanlee/DataSync/master/Bug1191773 > mozilla-b2g:master

Great work! My main concern is about using IAC instead of shared code accessing the places DataStore (and using put() with a revisionId as described in https://bugzilla.mozilla.org/show_bug.cgi?id=1191773#c20).

Rest of the feedback is on github! :)

Comment 36

[Fernando Jiménez Moreno [:ferjm]]

Comment on attachment 8662667 [details] [review]
[gaia] weilonge:seanlee/DataSync/master/Bug1191773 > mozilla-b2g:master

While this is a great piece of work, I feel that it is not ready to land yet. 

Apart from all the things I commented on GitHub, I can see two major blockers:

1. Maybe I missed it on the code, but I can't see how remotely deleted records are also deleted locally.

2. The usage of IAC to modify the Places DataStore is wrong in this case. I know that I said that we could land this using IAC initially to not block other work and then move to directly use DataStore in a follow up, but after looking at the code, I don't think it's worth doing that. Using IAC makes things quite messy. Basically, we shouldn't be adding any code into the System app related to the history adapter. Feel free to ping me on IRC or needinfo me if you need any help using DataStore directly.

Thanks for all the effort your putting here!

Comment 37

[Sean Lee [:seanlee][:weilonge]]

Comment on attachment 8662667 [details] [review]
[gaia] weilonge:seanlee/DataSync/master/Bug1191773 > mozilla-b2g:master

Hi Fernando, Michiel,

Thanks a lot for your comments and suggestions. Could you help to give it a feedback again?

The PR is updated based on couple points:
1. nit fix.
2. Use DataStore rather than IAC to places service in system app.

The following parts will be fixed as soon:
1. deleting records
2. unit test
3. comments

Comment 38

[Michiel de Jong [:michielbdejong]]

Comment on attachment 8662667 [details] [review]
[gaia] weilonge:seanlee/DataSync/master/Bug1191773 > mozilla-b2g:master

I suggested some changes in https://github.com/weilonge/gaia/pull/4

Comment 39

[Sean Lee [:seanlee][:weilonge]]

Deleting records feature is updated to attachment 8662667 [details] [review].

Comment 40

[Sean Lee [:seanlee][:weilonge]]

Hey Michiel,

Thanks for your PR! It has merged into History Adapter.

Comment 41

[Fernando Jiménez Moreno [:ferjm]]

Comment on attachment 8662667 [details] [review]
[gaia] weilonge:seanlee/DataSync/master/Bug1191773 > mozilla-b2g:master

Thank you Sean! This is looking better.

As we just spoke, the deletion code is a bit hard to follow and it doesn't scale very well, so I'd prefer to keep a local record of a KintoRecordID <-> DataStoreRecordID so we can do dataStore.delete(matchingID) and avoid traversing the whole DataStore everytime we need to delete one record. It's ok if you want to do it in a follow up.

Also, now that we are directly modifying the Places DataStore, did you check that this doesn't affect the other clients of this DataStore? I am completely unfamiliar to this code, but for example, is top sites updated when we store something in the Places DataStore from the history adapter?

Dale already worked with the Places DS so I'd like to hear his feedback as well if possible.

Comment 42

[Fernando Jiménez Moreno [:ferjm]]

Actually, we might be able to avoid creating the local match between KintoRecordID <-> DataStoreRecordID if we could mark kinto.js record properties as non-removable.

Let's say that we store history records in kinto like:

"id": "zMgfGkRinh92",
"sortindex": 2000,
"last_modified": 1442247272150,
"payload": {
  "id": "zMgfGkRinh92",
  "datastoreId: "whatever",
  "histUri": "http://mozilla.org/",
  "title": "Mozilla",
  "visits": [
     { "date": 1442247252490018, "type": 2 },
     { "date": 1442247250001234, "type": 2 }
  ]
},
"_status": "synced"

Right now, when we delete a record, we get a payload like:

"id": "_Avscjx5srFy",
"sortindex": 100,
"last_modified": 1441985077970,
"payload": {
  "id": "_Avscjx5srFy",
  "deleted": true
},
"_status": "synced"

Michiel, do you think that we can tell kinto to give us also the "datastoreId" property along with the "id" and "deleted" ones?

Comment 43

[Michiel de Jong [:michielbdejong]]

(In reply to Fernando Jiménez Moreno [:ferjm] from comment #42)
> Actually, we might be able to avoid creating the local match between
> KintoRecordID <-> DataStoreRecordID if we could mark kinto.js record
> properties as non-removable.
> 
> Let's say that we store history records in kinto like:
> 
> "id": "zMgfGkRinh92",
> "sortindex": 2000,
> "last_modified": 1442247272150,
> "payload": {
>   "id": "zMgfGkRinh92",
>   "datastoreId: "whatever",
>   "histUri": "http://mozilla.org/",
>   "title": "Mozilla",
>   "visits": [
>      { "date": 1442247252490018, "type": 2 },
>      { "date": 1442247250001234, "type": 2 }
>   ]
> },
> "_status": "synced"
> 
> Right now, when we delete a record, we get a payload like:
> 
> "id": "_Avscjx5srFy",
> "sortindex": 100,
> "last_modified": 1441985077970,
> "payload": {
>   "id": "_Avscjx5srFy",
>   "deleted": true
> },
> "_status": "synced"
> 
> Michiel, do you think that we can tell kinto to give us also the
> "datastoreId" property along with the "id" and "deleted" ones?

If we want to store meta-data about the DataStore ID on FxSync, then we would have to give it a name so that other FxSync clients know that this is coming from FxOS. It's a bit ugly, but would work as long as a record is not deleted.

However, if another client deletes a history record, it will not leave our fxosDataStoreId in place (unless we change the code on all other clients).

If we want to store it in the Kinto.js local copy but not on the FxSync server, then we would have to add a "local-only meta data" feature to Kinto.js which would maybe be possible, but I don't see a big win compared to storing it in asyncStorage.

However, if we reverse the thinking, we could easily store the FxSync Id in the DataStore (just make sure all our apps ignore this field), and then if a deletion comes in, we can find its corresponding record back in the DataStore.

Comment 44

[Fernando Jiménez Moreno [:ferjm]]

(In reply to Michiel de Jong [:michielbdejong] from comment #43)
> (In reply to Fernando Jiménez Moreno [:ferjm] from comment #42)
> > Actually, we might be able to avoid creating the local match between
> > KintoRecordID <-> DataStoreRecordID if we could mark kinto.js record
> > properties as non-removable.
> > 
> > Let's say that we store history records in kinto like:
> > 
> > "id": "zMgfGkRinh92",
> > "sortindex": 2000,
> > "last_modified": 1442247272150,
> > "payload": {
> >   "id": "zMgfGkRinh92",
> >   "datastoreId: "whatever",
> >   "histUri": "http://mozilla.org/",
> >   "title": "Mozilla",
> >   "visits": [
> >      { "date": 1442247252490018, "type": 2 },
> >      { "date": 1442247250001234, "type": 2 }
> >   ]
> > },
> > "_status": "synced"
> > 
> > Right now, when we delete a record, we get a payload like:
> > 
> > "id": "_Avscjx5srFy",
> > "sortindex": 100,
> > "last_modified": 1441985077970,
> > "payload": {
> >   "id": "_Avscjx5srFy",
> >   "deleted": true
> > },
> > "_status": "synced"
> > 
> > Michiel, do you think that we can tell kinto to give us also the
> > "datastoreId" property along with the "id" and "deleted" ones?
> 
> If we want to store meta-data about the DataStore ID on FxSync, then we
> would have to give it a name so that other FxSync clients know that this is
> coming from FxOS. It's a bit ugly, but would work as long as a record is not
> deleted.
> 
> However, if another client deletes a history record, it will not leave our
> fxosDataStoreId in place (unless we change the code on all other clients).

This is a good point. Maybe storing it locally is a better option.
 
> If we want to store it in the Kinto.js local copy but not on the FxSync
> server, then we would have to add a "local-only meta data" feature to
> Kinto.js which would maybe be possible, but I don't see a big win compared
> to storing it in asyncStorage.
> 

The advantage of adding it to the Kinto.js store is that we will be saving some storage space. An empty IndexedDB already takes almost 2Mb, so reusing the one that we have for the collection would be great.

> However, if we reverse the thinking, we could easily store the FxSync Id in
> the DataStore (just make sure all our apps ignore this field), and then if a
> deletion comes in, we can find its corresponding record back in the
> DataStore.

This is what the code already does. However it performs terribly bad. Because DataStore only allows to do operations given a record ID (it has no search functionalities), we have to traverse the whole DataStore looking for the element to be removed. If we have 1000 elements to remove, that means 1000 loops.

Comment 45

[Fernando Jiménez Moreno [:ferjm]]

(In reply to Fernando Jiménez Moreno [:ferjm] from comment #44)
>  
> > If we want to store it in the Kinto.js local copy but not on the FxSync
> > server, then we would have to add a "local-only meta data" feature to
> > Kinto.js which would maybe be possible, but I don't see a big win compared
> > to storing it in asyncStorage.
> > 
> 
> The advantage of adding it to the Kinto.js store is that we will be saving
> some storage space. An empty IndexedDB already takes almost 2Mb, so reusing
> the one that we have for the collection would be great.

Michiel, do you think that you could work on adding this feature to kinto.js, please? Thanks!

Comment 46

[Sean Lee [:seanlee][:weilonge]]

Comment on attachment 8662667 [details] [review]
[gaia] weilonge:seanlee/DataSync/master/Bug1191773 > mozilla-b2g:master

Hey Fernando,

All the comments in PR have been fixed, and it's ready to review after these changes fixed again:
* Comments updated
* Use DataStore directly to access PlacesDS
* Handle deleted: true records.
* Improve the performance of deleting records.

Could you help to review it again? Thank you. :)

Comment 47

[Michiel de Jong [:michielbdejong]]

(In reply to Fernando Jiménez Moreno [:ferjm] from comment #45)
> > The advantage of adding it to the Kinto.js store is that we will be saving
> > some storage space. An empty IndexedDB already takes almost 2Mb, so reusing
> > the one that we have for the collection would be great.

Yes, also storing data and local-only meta-data in the same place is simpler, and means you only have to write to disk once, not twice.

> 
> Michiel, do you think that you could work on adding this feature to
> kinto.js, please? Thanks!

I created https://github.com/Kinto/kinto.js/issues/173 about this.
I still think this is a bit of an ugly feature for Kinto.js, but maybe we can somehow make it nice. :)

Comment 48

[Sean Lee [:seanlee][:weilonge]]

(In reply to Michiel de Jong [:michielbdejong] from comment #47)
> (In reply to Fernando Jiménez Moreno [:ferjm] from comment #45)
> > > The advantage of adding it to the Kinto.js store is that we will be saving
> > > some storage space. An empty IndexedDB already takes almost 2Mb, so reusing
> > > the one that we have for the collection would be great.
> 
> Yes, also storing data and local-only meta-data in the same place is
> simpler, and means you only have to write to disk once, not twice.

Hey Michiel,

https://github.com/mozilla-b2g/gaia/pull/31896/files#diff-4d73ff90f2479c9b5d1f53457e85e25dR59
The latest implementation of History Adapter uses IndexedDB to store SynctoID -> DataStore ID mapping meta.

Could you help to give a comment here? Thank you :)

Comment 49

[Michiel de Jong [:michielbdejong]]

I created a follow-up bug to address the Id-mapping issue: bug 1207468

Comment 50

[Fernando Jiménez Moreno [:ferjm]]

Comment on attachment 8662667 [details] [review]
[gaia] weilonge:seanlee/DataSync/master/Bug1191773 > mozilla-b2g:master

Thank you Sean. It looks great, I left a few comments on the PR, mostly minor stuff.

This is almost ready, but I'd like to see the final version. Thanks!

Comment 51

[Dale Harvey (:daleharvey)]

Comment on attachment 8662667 [details] [review]
[gaia] weilonge:seanlee/DataSync/master/Bug1191773 > mozilla-b2g:master

So this is looking great thanks, I had a few comments in github, they were all very minor.

The top sites will be calculated correctly within the browser app, they wont receive screenshots until they are visited in fxos but that is ok (there are lots of reasons to not have a screenshot right now)

I do have one concern, we talked previously about moving places.js into shared, my reason for that is I am worried about having one of the writers drift out of sync with the other and having it cause bugs by storing unexpected data. A basic example would be the |localRecord.sort| which is very easy to forget about, also the previously mentioned bug with multiple processes + |writeInProgress|

There is stuff in places.js that definitely doesnt belong in the sync app, anything to deal with app_window.js etc, however I would really like it if we could extract the parts that actually touch the datastore in history.js and places.js and put them in a common file and share that so we have a common abstraction for the actual writing for the data.

It should hopefully be a relatively small refactor from what you already have, apologies for not getting to it earlier, thanks a lot

Comment 52

[Sean Lee [:seanlee][:weilonge]]

Hi Dale,

Thanks for your suggestions.
I will create a follow-up bug to improve the refactor part that you mentioned due to the schedule. I hope you don't mind. :)

Comment 53

[Sean Lee [:seanlee][:weilonge]]

Comment on attachment 8662667 [details] [review]
[gaia] weilonge:seanlee/DataSync/master/Bug1191773 > mozilla-b2g:master

Hi Fernando,

Could you help to review it again? :)

Comment 54

[Fernando Jiménez Moreno [:ferjm]]

Comment on attachment 8662667 [details] [review]
[gaia] weilonge:seanlee/DataSync/master/Bug1191773 > mozilla-b2g:master

r=me with the comments addressed, please.

Thank you!

Comment 55

[Fernando Jiménez Moreno [:ferjm]]

(In reply to Sean Lee [:seanlee] from comment #52)
> Hi Dale,
> 
> Thanks for your suggestions.
> I will create a follow-up bug to improve the refactor part that you
> mentioned due to the schedule. I hope you don't mind. :)

Could you make a reference to this follow-up in the code and here, please? Thanks!

Comment 56

[Sean Lee [:seanlee][:weilonge]]

Bug 1208352 is the follow-up bug based on comment 51.

Comment 57

[Fernando Jiménez Moreno [:ferjm]]

https://github.com/mozilla-b2g/gaia/commit/5a0a1bb1a863b07528ebc1a04652ed656d8333b3

Comment 58

[Sean Lee [:seanlee][:weilonge]]

gaia-test: https://treeherder.mozilla.org/#/jobs?repo=gaia&revision=063537968d4a85f45a198b14cd73d8db23bc7d90