Closed Bug 1336518 Opened 7 years ago Closed 7 years ago

Move Sync history queries into PlacesSyncUtils

Categories

(Firefox :: Sync, defect, P3)

defect

Tracking

()

RESOLVED FIXED
Firefox 56
Tracking Status
firefox56 --- fixed

People

(Reporter: lina, Assigned: tiago, Mentored)

References

(Blocks 1 open bug)

Details

(Whiteboard: [bad first bug][lang=js])

Attachments

(1 file)

The Sync history engine currently executes queries directly against the database: http://searchfox.org/mozilla-central/rev/d20e4431d0cf40311afa797868bc5c58c54790a2/services/sync/modules/engines/history.js#73-106,148-153,170-176,192-200,202-208,210-224

Let's move these queries into new `PlacesSyncUtils.history` methods, add tests (http://searchfox.org/mozilla-central/rev/b1aadb3572eaf7d2c70e19a2ba5413809d9ac698/toolkit/components/places/tests/unit/test_sync_utils.js), and convert the engine to use them. We can then remove the `_getStmt` machinery.

The `PlacesSyncUtils` module lives here: http://searchfox.org/mozilla-central/rev/d20e4431d0cf40311afa797868bc5c58c54790a2/toolkit/components/places/PlacesSyncUtils.jsm Here's what I'm thinking for the conversion:

* `PlacesSyncUtils.history.filterSyncablePlaces(guids)` can replace the query in `pullNewChanges`.
* We can move `setGUID` into `PlacesSyncUtils.history.changeGuid(oldGuid, newGuid)`.
* `_guidStm` can move to `PlacesSyncUtils.history.fetchGuidForURL(url)`.
* `_urlStm` can move to `PlacesSyncUtils.history.fetchURLForGuid(guid)`. Or we can have a more generic lookup method like `PlacesSyncUtils.history.fetch({ guid, url })`, that can search by either. Check out `Bookmarks.fetch` (http://searchfox.org/mozilla-central/rev/b1aadb3572eaf7d2c70e19a2ba5413809d9ac698/toolkit/components/places/Bookmarks.jsm#535-544,571-576) for inspiration.
* `_visitStm` can move to `PlacesSyncUtils.history.fetchVisitsForURL(url)`.

The new history methods should be tasks, and use `PlacesUtils.promiseDBConnection` or `PlacesUtils.withConnectionWrapper`, like `PlacesSyncUtils.bookmarks.*`.

This is a great bug if you've already landed a couple of Firefox patches. Marking it as a bad first bug because it's a bit intimidating if you're just getting started, but I'm very happy to mentor you if you'd like to take it. :-)
Priority: -- → P3
Should we write the 'PlacesSyncUtils.history.*' methods from scratch and then redirect the _getStmt() in each of the _guidStm, _urlStm and pullNewChanges or simply replace(like cut and paste) the existing method definitions from history.js to PlacesSyncUtils.jsm with just these method names differing?
Flags: needinfo?(kit)
(In reply to Bharat Raghunathan from comment #1)
> Should we write the 'PlacesSyncUtils.history.*' methods from scratch and
> then redirect the _getStmt() in each of the _guidStm, _urlStm and
> pullNewChanges or simply replace(like cut and paste) the existing method
> definitions from history.js to PlacesSyncUtils.jsm with just these method
> names differing?

Hi! The SQL query strings can be cut-and-pasted, but you'll need to replace `querySpinningly` with `PlacesUtils.promiseDBConnection()` for read-only queries, and `PlacesUtils.withConnectionWrapper` for the new `changeGuid` method.

After that, you can change `history.js` to call the new methods using `Async.promiseSpinningly`, and remove `_getStmt` and `_stmts` like in bug 1336282.
Flags: needinfo?(kit)
Hi! I wanted to give this bug a try, just submitted a patch with a first attempt at it.

If there aren't many things wrong I'll add tests on the next version of the patch.

I have some questions about it, can I ping you on IRC to ask you about them?

Thanks :)
Flags: needinfo?(kit)
(In reply to Santiago Paez [:tiago] from comment #4)
> Hi! I wanted to give this bug a try, just submitted a patch with a first
> attempt at it.
> 
> If there aren't many things wrong I'll add tests on the next version of the
> patch.
> 
> I have some questions about it, can I ping you on IRC to ask you about them?
> 
> Thanks :)

Also, I ran the tests and xpcshell-tests for services/sync and they are all passing. I didn't run the linter though, I'll do that for the next version of the patch.
Comment on attachment 8876433 [details]
Bug 1336518 - Move Sync history queries into PlacesSyncUtils.

https://reviewboard.mozilla.org/r/147784/#review152416

Hi Tiago, thank you so much for the patch! This is looking excellent. I'm traveling this week, so IRC might be tricky, but I left some general comments. If you'd like to chat, you're welcome to ping eoger, markh, or tcsc this week, or wait until next week. We all work on Desktop Sync. :-)

For the next round, could you please add some tests to http://searchfox.org/mozilla-central/rev/d840ebd5858a61dbc1622487c1fab74ecf235e03/toolkit/components/places/tests/unit/test_sync_utils.js? The Sync xpcshell tests should already cover most of your additions, but these methods are usually called several levels deep, and sometimes it's tricky to make sure the code is actually called.

For `filterSyncablePlaces`, you could probably copy from http://searchfox.org/mozilla-central/rev/d840ebd5858a61dbc1622487c1fab74ecf235e03/services/sync/tests/unit/test_history_tracker.js#212, maybe with different transition types. For the others, you can use `PlacesTestUtils.addVisits` (http://searchfox.org/mozilla-central/rev/d840ebd5858a61dbc1622487c1fab74ecf235e03/toolkit/components/places/tests/PlacesTestUtils.jsm#19-37) to insert some visits, and then fetch and modify them using the new functions.

Thanks again!

::: toolkit/components/places/PlacesSyncUtils.jsm:13
(Diff revision 1)
>  
>  const { classes: Cc, interfaces: Ci, results: Cr, utils: Cu } = Components;
>  
>  Cu.importGlobalProperties(["URL", "URLSearchParams"]);
>  
> +Cu.import("resource://services-sync/constants.js");

Instead of doing this, let's just define `const SQLITE_MAX_VARIABLE_NUMBER = 999` in this file. `Cu.import` is pretty expensive, and code in `toolkit/` usually doesn't depend on code in `services/`.

::: toolkit/components/places/PlacesSyncUtils.jsm:72
(Diff revision 1)
>        { url: canonicalURL.href }
>      );
>      return rows.length ? rows[0].getResultByName("frecency") : -1;
>    },
> +
> +  async filterSyncablePlaces(guids) {

For bonus points: could you add some JSDoc-style comments to these functions, with `@param` and `@returns` annotations?

::: toolkit/components/places/PlacesSyncUtils.jsm:80
(Diff revision 1)
> +    // Filter out hidden pages and `TRANSITION_FRAMED_LINK` visits. These are
> +    // excluded when rendering the history menu, so we use the same constraints
> +    // for Sync. We also don't want to sync `TRANSITION_EMBED` visits, but those
> +    // aren't stored in the database.
> +    let changedGuids = [];
> +    for (let startIndex = 0;

It would be nice to factor the chunking out into a `*chunkArray(array, chunkLength)` generator that yields slices of the `array`.

If you'd rather not do that in this bug, that's totally OK. We can save it for a follow-up.

::: toolkit/components/places/PlacesSyncUtils.jsm:92
(Diff revision 1)
> +      let binds = []
> +      for (let i = 0; i < chunkLength; i++) {
> +        binds.push(guids[startIndex + i]);
> +      }
> +
> +      let rows = await db.executeCached(`

Nit: Let's use `db.execute` here. `executeCached` is mostly useful for static query strings.

::: toolkit/components/places/PlacesSyncUtils.jsm:108
(Diff revision 1)
> +    return changedGuids;
> +  },
> +
> +  async changeGuid(uri, guid) {
> +    return PlacesUtils.withConnectionWrapper("HistorySyncUtils: changeGuid",
> +      async function(db, uri, guid) {

Hmm, this doesn't seem right, I think this function should only take `db` as an argument. This also suggests we need more test coverage. :-)

::: toolkit/components/places/PlacesSyncUtils.jsm:110
(Diff revision 1)
> +
> +  async changeGuid(uri, guid) {
> +    return PlacesUtils.withConnectionWrapper("HistorySyncUtils: changeGuid",
> +      async function(db, uri, guid) {
> +        await db.executeCached("UPDATE moz_places " +
> +          "SET guid = :guid " +

Nit: Let's use a template string instead of a double-quoted string. Here and everywhere else.

::: toolkit/components/places/PlacesSyncUtils.jsm:116
(Diff revision 1)
> +          "WHERE url_hash = hash(:page_url) AND url = :page_url",
> +          {guid: guid, page_url: uri});
> +      });
> +  },
> +
> +  async fetchVisitsForURL(uri) {

Micro-nit: Let's call this `url`. `uri` usually refers to an `nsIURI` object, but this function takes a string.

::: toolkit/components/places/PlacesSyncUtils.jsm:134
(Diff revision 1)
> +      let visitType = row.getResultByName("type");
> +      return { date: visitDate, type: visitType };
> +    });
> +  },
> +
> +  async fetchGUIDForURL(uri){

Nit: GUID => Guid

::: toolkit/components/places/PlacesSyncUtils.jsm:146
(Diff revision 1)
> +    );
> +
> +    return rows[0].getResultByName("guid");
> +  },
> +
> +  async fetchURLForGuid(guid) {

Nit: Let's call this something like `fetchURLInfoForGuid`, since we return the title and frecency, too.

::: toolkit/components/places/PlacesSyncUtils.jsm:152
(Diff revision 1)
> +    let db = await PlacesUtils.promiseDBConnection();
> +
> +    let rows = await db.executeCached("SELECT url, title, frecency " +
> +      "FROM moz_places " +
> +      "WHERE guid = :guid",
> +      { guid: guid }

Nit: `{ guid }`

::: toolkit/components/places/PlacesSyncUtils.jsm:166
(Diff revision 1)
> +      title: rows[0].getResultByName("title"),
> +      frecency: rows[0].getResultByName("frecency"),
> +    };
> +  },
> +
> +  async getAllURLs() {

Let's make this function take a `{ since, limit }` options object, so that we can make this function a bit easier to test and drop the `constants.js` import for `MAX_HISTORY_UPLOAD`.
Attachment #8876433 - Flags: review?(kit)
Assignee: nobody → tiago.paez11
Status: NEW → ASSIGNED
Flags: needinfo?(kit)
Attachment #8876433 - Flags: feedback+
Blocks: 1375223
Comment on attachment 8876433 [details]
Bug 1336518 - Move Sync history queries into PlacesSyncUtils.

https://reviewboard.mozilla.org/r/147784/#review156514

I notice this patch replaces many querySpinningly calls with promiseSpinningly - if it actually converts them all, we should also remove querySpinningly.
See Also: → 1377944
Hi Kit!

I'm so sorry I dropped the ball on this one.

I made the changes you mentioned and I'm going to be around, so next versions (if needed) will be done fast :)
Comment on attachment 8876433 [details]
Bug 1336518 - Move Sync history queries into PlacesSyncUtils.

https://reviewboard.mozilla.org/r/147784/#review163654

Excellent work, Tiago, thanks for picking this up again! No worries on the delay. With the review comments addressed, I think this patch should be ready to land! \o/ In particular, bug 1210296 replaced most of the `Async.promiseSpinningly` calls in Sync with `async/await`. I think we can remove most of those calls from the history engine, now that you've moved the queries into `PlacesSyncUtils`.

::: services/sync/modules/engines/history.js:77
(Diff revision 2)
>      let modifiedGUIDs = Object.keys(this._tracker.changedIDs);
>      if (!modifiedGUIDs.length) {
>        return {};
>      }
>  
> -    let db = PlacesUtils.history.QueryInterface(Ci.nsPIPlacesDatabase)
> +    let guids = Async.promiseSpinningly(PlacesSyncUtils.history.filterSyncablePlaces(modifiedGUIDs));

No need for `promiseSpinningly` here; just `await PlacesSyncUtils.history.filterSyncablePlaces(modifiedGUIDs)`.

::: services/sync/modules/engines/history.js:120
(Diff revision 2)
> -      "SET guid = :guid " +
> -      "WHERE url_hash = hash(:page_url) AND url = :page_url");
> -  },
> -
>    // Some helper functions to handle GUIDs
>    setGUID: function setGUID(uri, guid) {

Let's make this an async function and use `await` here, too, instead of spinning the event loop.

::: services/sync/modules/engines/history.js:131
(Diff revision 2)
> -      "FROM moz_places " +
> -      "WHERE url_hash = hash(:page_url) AND url = :page_url");
> -  },
> -  _guidCols: ["guid"],
> -
>    GUIDForUri: function GUIDForUri(uri, create) {

Ditto, let's make this `async`.

::: services/sync/modules/engines/history.js:133
(Diff revision 2)
>    GUIDForUri: function GUIDForUri(uri, create) {
> -    let stm = this._guidStm;
> -    stm.params.page_url = uri.spec ? uri.spec : uri;
> -
>      // Use the existing GUID if it exists
> -    let result = Async.querySpinningly(stm, this._guidCols)[0];
> +    let url = uri.spec ? uri.spec : uri;

`uri.spec` is only for tests, right? If it's not too much trouble, can we remove those uses and make sure `uri` is always a string? If it's painful to convert all the callers, no worries; keeping it like this is fine.

::: services/sync/modules/engines/history.js:140
(Diff revision 2)
> -      return result.guid;
> +    if (guid) {
> +      return guid;
> +    }
>  
>      // Give the uri a GUID if it doesn't have one
> -    if (create)
> +    if (create) {

Thanks for adding these braces while you're here. :-)

::: services/sync/modules/engines/history.js:145
(Diff revision 2)
> -      LIMIT :max_results`);
> -  },
> -  _allUrlCols: ["url"],
> -
> -  // See bug 320831 for why we use SQL here
>    _getVisits: function HistStore__getVisits(uri) {

Please make this and `_findURLByGUID` async, too.

::: services/sync/modules/engines/history.js:157
(Diff revision 2)
>  
>    async changeItemID(oldID, newID) {
>      this.setGUID(this._findURLByGUID(oldID).url, newID);
>    },
>  
> -
> +  getAllIDs: function HistStore_getAllIDs() {

This should be `async getAllIDs()`, and then you can `await` the calls to `getAllURLs` and `GUIDForUri` in the body.

::: toolkit/components/places/PlacesSyncUtils.jsm:59
(Diff revision 2)
>  XPCOMUtils.defineLazyGetter(this, "ROOTS", () =>
>    Object.keys(ROOT_SYNC_ID_TO_GUID)
>  );
>  
> +/**
> + * Auxiliar generator function that yields an array in chunks

Micro-nit: "Auxiliary".

::: toolkit/components/places/PlacesSyncUtils.jsm:65
(Diff revision 2)
> + *
> + * @param array
> + * @param chunkLength
> + * @yields {Array} New Array with the next chunkLength elements of array. If the array has less than chunkLength elements, yields all of them
> + */
> +let chunkArray = function* (array, chunkLength) {

Nit: `function* chunkArray`.

::: toolkit/components/places/PlacesSyncUtils.jsm:68
(Diff revision 2)
> + * @yields {Array} New Array with the next chunkLength elements of array. If the array has less than chunkLength elements, yields all of them
> + */
> +let chunkArray = function* (array, chunkLength) {
> +  let cpArray = array.slice(0);
> +  while (cpArray.length > 0) {
> +    yield cpArray.splice(0, chunkLength);

Instead of copying the array and splicing, what do you think about keeping track of the start index in a variable, and yielding `array.slice(startIndex, startIndex += chunkLength)`?

::: toolkit/components/places/PlacesSyncUtils.jsm:124
(Diff revision 2)
> +        SELECT DISTINCT p.guid FROM moz_places p
> +        JOIN moz_historyvisits v ON p.id = v.place_id
> +        WHERE p.guid IN (${new Array(chunk.length).fill("?").join(",")}) AND
> +            (p.hidden = 1 OR v.visit_type IN (0,
> +              ${PlacesUtils.history.TRANSITION_FRAMED_LINK}))
> +      `, binds);

Can you pass `chunk` directly here, instead of copying into `binds`?

::: toolkit/components/places/PlacesSyncUtils.jsm:139
(Diff revision 2)
> +   *
> +   * @param uri
> +   * @param guid
> +   */
> +  async changeGuid(uri, guid) {
> +    return PlacesUtils.withConnectionWrapper("HistorySyncUtils: changeGuid",

Let's use the same `let canonicalURL = PlacesUtils.SYNC_BOOKMARK_VALIDATORS.url(uri)` trick here that we use in `fetchURLFrecency`, and let's validate the GUID using `PlacesUtils.SYNC_BOOKMARK_VALIDATORS.guid(guid)`.

::: toolkit/components/places/PlacesSyncUtils.jsm:150
(Diff revision 2)
> +          {guid, page_url: uri});
> +      });
> +  },
> +
> +  /**
> +   * Fetch the visits (date and type of it) corresponding to a given url

Let's mention that we fetch the last 20 visits.

::: toolkit/components/places/PlacesSyncUtils.jsm:156
(Diff revision 2)
> +   *
> +   * @param url
> +   * @returns {Array} Each element of the Array is an object with members: date and type
> +   */
> +  async fetchVisitsForURL(url) {
> +    let db = await PlacesUtils.promiseDBConnection();

Ditto for `canonicalURL`.

::: toolkit/components/places/PlacesSyncUtils.jsm:181
(Diff revision 2)
> +   * @param uri
> +   * @returns {String} The guid of the given uri
> +   */
> +  async fetchGuidForURL(url) {
> +    let db = await PlacesUtils.promiseDBConnection();
> +

Ditto for `canonicalURL` here, too. :-)

::: toolkit/components/places/PlacesSyncUtils.jsm:189
(Diff revision 2)
> +      FROM moz_places
> +      WHERE url_hash = hash(:page_url) AND url = :page_url`,
> +      { page_url: url }
> +    );
> +
> +    return rows[0].getResultByName("guid");

This will throw if `rows` is empty. Let's check for that and return `null` if so.

::: toolkit/components/places/PlacesSyncUtils.jsm:239
(Diff revision 2)
> +            p.hidden = 0 AND
> +            v.visit_type NOT IN (0,
> +              ${PlacesUtils.history.TRANSITION_FRAMED_LINK})
> +      ORDER BY frecency DESC
> +      LIMIT :max_results`,
> +      { cutoff_date: options.since, max_results: options.limit }

Just in case, let's validate `options.limit`, and throw if `!Number.isFinite`.

Also, let's change `options.since` to be a `Date` object instead of a timestamp in microseconds. There's a `PlacesUtils.toPRTime` helper that will convert dates to microseconds for us.

::: toolkit/components/places/tests/unit/test_sync_utils.js:192
(Diff revision 2)
> +
> +  // Remove the visits added during this test.
> +  await PlacesTestUtils.clearHistory();
> +});
> +
> +add_task(async function test_filterSyncablePlaces() {

Nice work on the tests!

::: toolkit/components/places/tests/unit/test_sync_utils.js:216
(Diff revision 2)
> +  let filteredGuids = await PlacesSyncUtils.history.filterSyncablePlaces(guids);
> +
> +  // Check if the filtered visits are of type TRANSITION_FRAMED_LINK.
> +  for (let visit of arrayOfVisits) {
> +    if (visit.transition === TRANSITION_FRAMED_LINK) {
> +      equal(filteredGuids.includes(dictURLGuid[visit.uri]), true);

Nit: We usually use `ok` instead of `equal(..., true)`.
Attachment #8876433 - Flags: review?(kit)
Hi!

I made most of the changes you suggested, thank you for the feedback, it was extremely useful!

I'm not very experienced using async/await, if you see anything weird while reviewing, please let me know :)

The patch is passing all the tests, but I have some questions:

1- I added the validation for the url in a couple of the methods(the canonicalURL trick). This validator throws when it fails, and I don't know what the expected behavior should be in that case.
The methods where this happens are:

changeGuid(uri, guid)  - PlacesSyncUtils:133 
fetchVisitsForURL(url) - PlacesSyncUtils:157
fetchGuidForURL(url)   - PlacesSyncUtils:186

For now I'm catching the exception and returning an 'empty' value for that method. I chose this because some tests try to insert or change visits with malformed url, making the methods throw an exception and fail the test.

One thing I could add to this is to log the exception (I was thinking about adding logging throughout the code I touched).
But I know we could do something different and change the tests. I would like to hear what do you think about it.

2- I didn't understand the uri.spec changes you asked me to make. I can check if it's a string, but you mentioned something about "convert all the callers" that confused me :(

3- I checked into what Mark said about removing querySpinningly, and the method is used in another file and a couple test files. I guess it would be possible to remove it, but I'm wondering if we should do it in another bug.

I'll be waiting for your feedback, thanks!
Flags: needinfo?(kit)
Comment on attachment 8876433 [details]
Bug 1336518 - Move Sync history queries into PlacesSyncUtils.

https://reviewboard.mozilla.org/r/147784/#review165770

Great! Almost there. :-) Thanks for addressing all the feedback, and nice job on the `async/await` conversion. Answering your questions:

1. I elaborated in a comment, but I think the approach we should take is have your new `PlacesSyncUtils` methods throw, then catch and log the exceptions in the history engine. That way, we can remove the logic later once we're sure that other devices aren't uploading malformed history records. In general, it's OK to change or even remove tests that don't add value, and I think the test for malformed record handling is one of those. But we also don't want to break history syncing just because a device uploaded a record with a bad URL or GUID, so let's keep it for now.
2. Sorry, I meant that we should pass `uri.spec` instead of `uri` everywhere. It looks like that's only in `test_history_tracker.js`, so let's change those calls and drop `uri.spec ? uri.spec : uri`. I left more specific review comments about this, too.
3. I agree with you, let's remove `querySpinningly` in bug 1375223 instead of this bug. Thanks for checking!

::: services/sync/modules/engines/history.js:77
(Diff revision 3)
>      let modifiedGUIDs = Object.keys(this._tracker.changedIDs);
>      if (!modifiedGUIDs.length) {
>        return {};
>      }
>  
> -    let db = PlacesUtils.history.QueryInterface(Ci.nsPIPlacesDatabase)
> +    let guids = await PlacesSyncUtils.history.filterSyncablePlaces(modifiedGUIDs);

Micro-nit: `guidsToRemove`.

::: services/sync/modules/engines/history.js:146
(Diff revision 3)
> -    if (create)
> -      return this.setGUID(uri);
> +    if (create) {
> +      return await this.setGUID(uri);
> +    }
>    },
>  
> -  get _visitStm() {
> +  async _getVisits(uri) {

Same as `_findURLByGUID`, please remove `_getVisits` and use `fetchVisitsForURL` directly.

::: services/sync/modules/engines/history.js:150
(Diff revision 3)
> -  _findURLByGUID: function HistStore__findURLByGUID(guid) {
> -    this._urlStm.params.guid = guid;
> -    return Async.querySpinningly(this._urlStm, this._urlCols)[0];
>    },
>  
> -  async changeItemID(oldID, newID) {
> +  async _findURLByGUID(guid) {

Let's remove `_findURLByGUID` and change the callers to use `fetchURLInfoForGuid` directly.

::: services/sync/modules/engines/history.js:160
(Diff revision 3)
> -    // Only get places visited within the last 30 days (30*24*60*60*1000ms)
> +    let urls = await PlacesSyncUtils.history.getAllURLs({ since: new Date((Date.now() - 2592000000)), limit: MAX_HISTORY_UPLOAD });
> -    this._allUrlStm.params.cutoff_date = (Date.now() - 2592000000) * 1000;
> -    this._allUrlStm.params.max_results = MAX_HISTORY_UPLOAD;
> -
> -    let urls = Async.querySpinningly(this._allUrlStm, this._allUrlCols);
>      let self = this;

Micro-nit: Remove `self` and use `this` directly.

::: services/sync/modules/engines/history.js:163
(Diff revision 3)
> -
> -    let urls = Async.querySpinningly(this._allUrlStm, this._allUrlCols);
>      let self = this;
> -    return urls.reduce(function(ids, item) {
> -      ids[self.GUIDForUri(item.url, true)] = item.url;
> -      return ids;
> +
> +    let dict = {};
> +    for (let i=0; i < Object.keys(urls).length; i++) {

`for (let url of urls)`

::: services/sync/modules/engines/history.js:164
(Diff revision 3)
> -    let urls = Async.querySpinningly(this._allUrlStm, this._allUrlCols);
>      let self = this;
> -    return urls.reduce(function(ids, item) {
> -      ids[self.GUIDForUri(item.url, true)] = item.url;
> -      return ids;
> -    }, {});
> +
> +    let dict = {};
> +    for (let i=0; i < Object.keys(urls).length; i++) {
> +      let index = await self.GUIDForUri(urls[i], true);

Naming nit: Let's call this `guid` instead of `index`, and let's call the object `urlsByGUID` instead of `dict`.

::: services/sync/tests/unit/test_history_tracker.js:147
(Diff revision 3)
>    _("Deletions are tracked.");
>  
>    // This isn't present because we weren't tracking when it was visited.
>    await addVisit("track_delete");
>    let uri = Utils.makeURI("http://getfirefox.com/track_delete");
> -  let guid = engine._store.GUIDForUri(uri);
> +  let guid = await engine._store.GUIDForUri(uri);

uri.spec

::: services/sync/tests/unit/test_history_tracker.js:165
(Diff revision 3)
>  });
>  
>  add_task(async function test_dont_track_expiration() {
>    _("Expirations are not tracked.");
>    let uriToRemove = await addVisit("to_remove");
> -  let guidToRemove = engine._store.GUIDForUri(uriToRemove);
> +  let guidToRemove = await engine._store.GUIDForUri(uriToRemove);

uriToRemove.spec

::: services/sync/tests/unit/test_history_tracker.js:219
(Diff revision 3)
>  add_task(async function test_filter_hidden() {
>    await startTracking();
>  
>    _("Add visit; should be hidden by the redirect");
>    let hiddenURI = await addVisit("hidden");
> -  let hiddenGUID = engine._store.GUIDForUri(hiddenURI);
> +  let hiddenGUID = await engine._store.GUIDForUri(hiddenURI);

`hiddenURI.spec`, and same for `trackedURI` and `embedURI` below. Then you can remove `let url = uri.spec ? uri.spec : uri` in `GUIDForUri`.

::: toolkit/components/places/PlacesSyncUtils.jsm:70
(Diff revision 3)
> +function* chunkArray(array, chunkLength) {
> +  let startIndex = 0;
> +  while (startIndex < array.length) {
> +    yield array.slice(startIndex, startIndex += chunkLength);
> +  }
> +};

Micro-nit: No `;` here. I think ESLint will catch that, and other style nits, if you run `./mach eslint toolkit/components/places services/sync`.

::: toolkit/components/places/PlacesSyncUtils.jsm:102
(Diff revision 3)
> +   *
> +   * @param guids
> +   *
> +   * @returns {Array} new Array with the guids that aren't syncable
> +   */
> +  async filterSyncablePlaces(guids) {

I know I suggested the `filterSyncablePlaces` name, but I think that implies this returns the GUIDs we *should* sync, while it actually returns the GUIDs that we shouldn't. Maybe something like `determineNonSyncableGuids` would be clearer. What do you think?

::: toolkit/components/places/PlacesSyncUtils.jsm:109
(Diff revision 3)
> +
> +    // Filter out hidden pages and `TRANSITION_FRAMED_LINK` visits. These are
> +    // excluded when rendering the history menu, so we use the same constraints
> +    // for Sync. We also don't want to sync `TRANSITION_EMBED` visits, but those
> +    // aren't stored in the database.
> +    let changedGuids = [];

Micro-nit: Move this comment to just above `db.execute`, and rename `changedGuids` to `nonSyncableGuids`.

::: toolkit/components/places/PlacesSyncUtils.jsm:134
(Diff revision 3)
> +   *
> +   * @param uri
> +   * @param guid
> +   */
> +  async changeGuid(uri, guid) {
> +    try{

Instead of wrapping your new `PlacesSyncUtils` functions in a `try...catch`, let's instead catch the exceptions in the history engine, and log the error there. So, for example, `setGUID` would look something like this:

    try {
      await PlacesSyncUtils.history.changeGuid(uri, guid);
    } catch (e) {
      this._log.error("Error setting GUID ${guid} for URI ${uri}", guid, uri);

::: toolkit/components/places/PlacesSyncUtils.jsm:244
(Diff revision 3)
> +   *        Options object with two members, since and limit. Both of them must be provided
> +   * @returns {Array} - Up to limit number of URLs starting from the date provided by since
> +   */
> +  async getAllURLs(options) {
> +    // Check that the limit property is finite number.
> +    if (options.limit && !Number.isFinite(options.limit)) {

Just `!Number.isFinite(options.limit)` is fine here. It'll be false if `options.limit` isn't passed.

::: toolkit/components/places/PlacesSyncUtils.jsm:249
(Diff revision 3)
> +    if (options.limit && !Number.isFinite(options.limit)) {
> +      throw new Error("The number provided in options.limit is not finite.");
> +    }
> +
> +    // Check that the since property is of type Date.
> +    if (options.since && !Object.prototype.toString.call(new Date(options.since)) === "[object Date]") {

I think this should be `!options.since || Object.prototype.toString.call(options.since) != "[object Date]"`.

`Object.prototype.toString.call(new Date(...))` will always be `"[object Date]"`, so `!"[object Date]" === false`, and `(false === "[object Date]") === false`.
Attachment #8876433 - Flags: review?(kit)
Blocks: 1384644
Flags: needinfo?(kit)
Hi Kit!

Thanks again for the feedback :)

This time I have only one question, 

In async changeGUID(uri, guid) - PlacesSyncUtils.jsm - line: 126
This method is the only one of PlacesSyncUtils.history that returns PlacesUtils.withConnectionWrapper(...);
I don't know if there is a reason for that or if we should/could change it.

Let me know what you think and I'll be waiting your comments for further improvements.
Thanks!
Flags: needinfo?(kit)
Comment on attachment 8876433 [details]
Bug 1336518 - Move Sync history queries into PlacesSyncUtils.

https://reviewboard.mozilla.org/r/147784/#review168118

Awesome! I pushed your patch to the try server; with the one nit fixed, I think this is ready to land. Thanks for all your work on this, you did a great job!

::: services/sync/modules/engines/history.js:164
(Diff revisions 3 - 4)
>    async changeItemID(oldID, newID) {
> -    this.setGUID(await this._findURLByGUID(oldID).url, newID);
> +    this.setGUID(await PlacesSyncUtils.history.fetchURLInfoForGuid(oldID).url, newID);
>    },
>  
>    async getAllIDs() {
>      let urls = await PlacesSyncUtils.history.getAllURLs({ since: new Date((Date.now() - 2592000000)), limit: MAX_HISTORY_UPLOAD });

Nit: It's not immediately obvious that 2592000000 milliseconds is 30 days. Let's move this into a constant at the top of the file, and add a comment that it's 30 days.
Attachment #8876433 - Flags: review?(kit) → review+
(In reply to Santiago Paez [:tiago] from comment #15)
> In async changeGUID(uri, guid) - PlacesSyncUtils.jsm - line: 126
> This method is the only one of PlacesSyncUtils.history that returns
> PlacesUtils.withConnectionWrapper(...);
> I don't know if there is a reason for that or if we should/could change it.

`PlacesUtils.withConnectionWrapper` returns a read-write connection, while `PlacesUtils.promiseDBConnection` returns a read-only one. `promiseDBConnection` is faster, and usually the better choice if you're not updating any rows, but can return stale data if there are pending writes.

That's a problem for bookmarks, where stale data can introduce inconsistencies, but not for history. Unlike bookmarks, history records don't depend on each other, and change so frequently that missing a visit is OK. We can pick it up on the next sync, anyway.

TL;DR: You did the right thing; no need to change this. :-)
Flags: needinfo?(kit)
Comment on attachment 8876433 [details]
Bug 1336518 - Move Sync history queries into PlacesSyncUtils.

https://reviewboard.mozilla.org/r/147784/#review168120

::: toolkit/components/places/PlacesSyncUtils.jsm:126
(Diff revision 4)
> +   * Change the guid of the given uri
> +   *
> +   * @param uri
> +   * @param guid
> +   */
> +  async changeGuid(uri, guid) {

Oh, also, drop `async` here, since there's no `await` in the function. (https://treeherder.mozilla.org/#/jobs?repo=try&revision=699dae5b76bd6e39f0c8ab374b9ea5ba46845d57&selectedJob=119341978)
Pushed by kcambridge@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9c4690da59fa
Move Sync history queries into PlacesSyncUtils. r=kitcambridge
https://hg.mozilla.org/mozilla-central/rev/9c4690da59fa
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
Depends on: 1389717
You need to log in before you can comment on or make changes to this bug.