1406488 - Use a set to store current visits in `_recordToPlaceInfo`

Status: RESOLVED FIXED

(Firefox :: Sync, enhancement, P3)

Description

[Lina Butler [:lina]]

We currently use an array to store `curVisits`: https://searchfox.org/mozilla-central/rev/8efd128b48cdae3a6c6862795ce618aa1ca1c0b4/services/sync/modules/engines/history.js#248,255-258,285,293 Using a set here would be more efficient, as we'd avoid a linear search to look up every visit key.

Comment 1

[Kevin Pellet (Ilphrin)]

Hi! Can I take this bug? It is my very first bug. I have built Firefox and am testing some code modifications

Comment 2

[Mark Hammond [:markh] [:mhammond]]

(In reply to Kevin Pellet (Ilphrin) from comment #1)
> Hi! Can I take this bug? It is my very first bug. I have built Firefox and
> am testing some code modifications

Yes, that would be great, and thanks for wanting to get involved! Please upload a first version of a patch, and we'll then both assign the bug to you and let you know what changes, if any, need to be made.

Comment 3

[Kevin Pellet (Ilphrin)]

Attachment: bug1406488.patch

So here is my first proposal for fixing this bug!

Comment 4

[Lina Butler [:lina]]

Comment on attachment 8917049 [details] [diff] [review]
bug1406488.patch

Review of attachment 8917049 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks very much for your first patch, Kevin! Please fix up the `fetchVisitsForURL` issue, and run `./mach xpcshell-test services/sync/tests/unit/test_history_store.js` to make sure the tests still pass. Once that's fixed, I'll r+ and we can land. \o/

::: services/sync/modules/engines/history.js
@@ +251,1 @@
>      } catch (e) {

This won't work as written, unfortunately. `fetchVisitsForURL` returns an array of `{ date, type }` tuples, but `curVisits` needs to be a set of string keys. You'll want to iterate over the visits, change them to `visit.date + "," + visit.type`, and add those to the set instead, like the old loop did.

Comment 5

[Kevin Pellet (Ilphrin)]

Attachment: bug1406488.patch

Oh! I thought the point was to remove this loop when replacing by a Set, but after testing with jsFiddle I understood that using objects for a Set is... meh. I understand more now. I've uploaded another patch which iterates over the array to generate the Set.

All the tests are successful.

Comment 6

[Lina Butler [:lina]]

Comment on attachment 8917229 [details] [diff] [review]
bug1406488.patch

Review of attachment 8917229 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good! Please squash the two commits into a single patch, add "r=kitcambridge" to the commit message, and then we can get this checked in.

Comment 7

[Kevin Pellet (Ilphrin)]

Attachment: bug1406588_publish.patch

Cool =D I had some difficulties handling hg commands, but here is the final patch with the two commits merged.

Comment 8

[Kevin Pellet (Ilphrin)]

Ah, bad file name, the file should be named bug1406488_publish.patch, sorry

Comment 9

[Lina Butler [:lina]]

Comment on attachment 8917605 [details] [diff] [review]
bug1406588_publish.patch

Awesome, thanks!

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f160ec9ff8bd
Use a set instead of array to store current visits in `_recordToPlaceInfo` r=kitcambridge

Comment 11

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/f160ec9ff8bd