1423399 - Sync won't dedupe visits older than your most recent 20 for a given uri

Status: RESOLVED FIXED

(Firefox :: Sync, enhancement, P2)

Description

[Thom Chiovoloni [:tcsc] (ex-moco)]

This call [0] only returns the most recent 20 history visits, and so we won't dedupe against items older than that. This doesn't seem like that big of a deal, but it's probably reasonably common...

[0] https://searchfox.org/mozilla-central/rev/2e08acdf8862e68b13166970e17809a3b5d6a555/services/sync/modules/engines/history.js#291

Comment 1

[Thom Chiovoloni [:tcsc] (ex-moco)]

Attachment: Bug 1423399 - Avoid duplicating synced history visits older than your most recent 20

Review commit: https://reviewboard.mozilla.org/r/208266/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/208266/

Comment 2

[Thom Chiovoloni [:tcsc] (ex-moco)]

This patch applies on top of the patch in bug 1423395

Comment 3

[Thom Chiovoloni [:tcsc] (ex-moco)]

Comment on attachment 8937588 [details]
Bug 1423399 - Avoid duplicating synced history visits older than your most recent 20

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/208266/diff/1-2/

Comment 4

[Thom Chiovoloni [:tcsc] (ex-moco)]

Comment on attachment 8937588 [details]
Bug 1423399 - Avoid duplicating synced history visits older than your most recent 20

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/208266/diff/2-3/

Comment 5

[Lina Butler [:lina]]

Comment on attachment 8937588 [details]
Bug 1423399 - Avoid duplicating synced history visits older than your most recent 20

https://reviewboard.mozilla.org/r/208266/#review214020

Thanks! I've a few questions, so I'd like to see the patch again, but this is looking good.

::: services/sync/modules/engines/history.js:315
(Diff revision 3)
>        curVisitsAsArray = await PlacesSyncUtils.history.fetchVisitsForURL(record.histUri);
>      } catch (e) {
>        this._log.error("Error while fetching visits for URL ${record.histUri}", record.histUri);
>      }
> +    let oldestAllowed = PlacesSyncUtils.bookmarks.EARLIEST_BOOKMARK_TIMESTAMP;
> +    if (curVisitsAsArray.length == 20) {

I wonder if we should have `fetchVisitsForURL` clamp or filter out old visits instead. Hard-coding the limit of 20 visits here and in the query in `fetchVisitsForURL` makes me wary, but `PlacesSyncUtils` doesn't currently clamp any dates, so maybe this is OK.

::: services/sync/modules/engines/history.js:316
(Diff revision 3)
>      } catch (e) {
>        this._log.error("Error while fetching visits for URL ${record.histUri}", record.histUri);
>      }
> +    let oldestAllowed = PlacesSyncUtils.bookmarks.EARLIEST_BOOKMARK_TIMESTAMP;
> +    if (curVisitsAsArray.length == 20) {
> +      oldestAllowed = Math.min(...curVisitsAsArray.map(v =>

Nit: `fetchVisitsForURL` already sorts; can you simplify to just clamping `curVisitsAsArray[curVisitsAsArray.length - 1].date`?

::: services/sync/modules/engines/history.js:356
(Diff revision 3)
>        visit.date = PlacesSyncUtils.history.clampVisitDate(originalVisitDate);
>  
> +      if (visit.date.getTime() < oldestAllowed) {
> +        // Visit is older than the oldest visit we have, and we have so many
> +        // visits for this uri that we hit our limit when inserting.
> +        continue;

I wonder if we can break early, since visits are (in theory) sorted newest first. Then again, it's probably not very expensive to leave as-is...

::: services/sync/tests/unit/test_history_engine.js:231
(Diff revision 3)
> +
> +  allVisits = (await PlacesUtils.history.fetch("https://www.example.com", {
> +    includeVisits: true
> +  })).visits;
> +
> +  equal(allVisits.length, 25);

Should we add some newer server visits, too (and multiple old visits), and make sure we take those while ignoring all the old ones?

Comment 6

[Thom Chiovoloni [:tcsc] (ex-moco)]

Comment on attachment 8937588 [details]
Bug 1423399 - Avoid duplicating synced history visits older than your most recent 20

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/208266/diff/3-4/

Comment 7

[Lina Butler [:lina]]

Comment on attachment 8937588 [details]
Bug 1423399 - Avoid duplicating synced history visits older than your most recent 20

https://reviewboard.mozilla.org/r/208266/#review214354

Awesome, thanks!

Comment 8

[Thom Chiovoloni [:tcsc] (ex-moco)]

Comment on attachment 8937588 [details]
Bug 1423399 - Avoid duplicating synced history visits older than your most recent 20

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/208266/diff/4-5/

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1e3dc1249201
Avoid duplicating synced history visits older than your most recent 20 r=kitcambridge

Comment 10

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/1e3dc1249201