The history engine has a hand-rolled wrapper around `mozIAsyncHistory.updatePlaces`: http://searchfox.org/mozilla-central/rev/5bb6dfcb3355a912c20383578bc435c1d4ecf575/services/sync/modules/engines/history.js#293-321 Let's replace this wrapper with the nicer, promise-based `PlacesUtils.history.insertMany`: http://searchfox.org/mozilla-central/rev/5bb6dfcb3355a912c20383578bc435c1d4ecf575/toolkit/components/places/History.jsm#212-255 This is a great first bug, but we should wait for bug 1210296 to land before working on this.

Description says good-first-bug, but is it good-second-bug too? Let's find out. Mind if I pick this up? What are the relevant tests for this change?

I started to work yesterday in this bug and i found so far two test: https://developer.mozilla.org/en-US/docs/Mozilla/QA/Writing_xpcshell-based_unit_tests with xpcshell, you can test: PlaceUtils.history.insertMany and the last one: https://developer.mozilla.org/en-US/docs/Mozilla/Projects/TPS_Tests And here you can test test_history.js but i found troubles with tps test. i don't know if i'm doing something wrong or documentation is outdated but my test always fail. (example test too!)

(In reply to Luciano I from comment #2) > I started to work yesterday in this bug and i found so far two test: > https://developer.mozilla.org/en-US/docs/Mozilla/QA/Writing_xpcshell- > based_unit_tests > > with xpcshell, you can test: PlaceUtils.history.insertMany Note that this bug isn't about testing .insertMany, it's about shifting the core of the sync code to use that. It would be fine if tests (including TPS) didn't move to using this - and in some ways may be preferred, as keeping the tests the same helps build confidence that the semantics of the history engine haven't changed) - getting the tests moved over eventually might make sense, but I think it's fine to ignore for now. (In reply to Nicolas Ouellet-Payeur from comment #1) > Description says good-first-bug, but is it good-second-bug too? Let's find > out. > > Mind if I pick this up? What are the relevant tests for this change? I think we should give Luciano a chance to upload a patch - if there's been no movement for a while I think it's OK to take. Another bug that might interest you and might be suitable is bug 1295510 (and maybe bug 1135943, bug 1346072, bug 1353217, bug 1371746 or bug 1375223 :) Thanks for you interest!

Comment on attachment 8891590 [details] Bug 1377944 - Converts the history engine to use 'PlacesUtils.history.insertMany'. https://reviewboard.mozilla.org/r/162702/#review168116 Thanks very much for the patch, Luciano, it's a good first cut! r- because this won't work for multiple visits yet, and needs some changes to the `VisitInfo` property names. Please fix up the issues, make sure the tests are green (`./mach xpcshell-test services/sync/test_history` will run just the history tests; if those pass, you can run `./mach xpcshell-test services/sync` to make sure the others pass, too), and that `./mach eslint services/sync` doesn't report any issues. Then, amend the commit, push a new version to MozReview, and we can get it landed. Thanks again for working on this. ::: services/sync/modules/engines/history.js:272 (Diff revision 1) > > try { > if (record.deleted) { > let promise = this.remove(record); > promise = promise.catch(ex => failed.push(record.id)); > blockers.push(promise); While you're here, let's remove the `blockers` array entirely. Our storage layer serializes all SQLite statements, so accumulating the promises into an array and then `await`-ing them at the end doesn't improve throughput. Let's simplify this block to just `await this.remove(record)`. The outer `catch` already adds the ID to `failed` if this throws, so we can also drop the `promise.catch` line. ::: services/sync/modules/engines/history.js:296 (Diff revision 1) > records.length = k; // truncate array > > + // Need it for PageInfo Validation > + for(i=0; i<records.length; i++) { > + let record = records[i]; > + let record_visit = record.visits[0]; Unfortunately, this only handles the first visit. `insertMany` will still throw if we have more than one visit for a URL. However, I think `_recordToPlaceInfo` is a better place for this logic, anyway, since we already rewrite the record's properties there. See my comments below. ::: services/sync/modules/engines/history.js:299 (Diff revision 1) > + for(i=0; i<records.length; i++) { > + let record = records[i]; > + let record_visit = record.visits[0]; > + > + record.url = record.histUri; > + record_visit.date = PlacesUtils.toDate(record_visit.date); Nice! These two lines are correct, but let's move them to `_recordToPlaceInfo`. ::: services/sync/modules/engines/history.js:300 (Diff revision 1) > + let record = records[i]; > + let record_visit = record.visits[0]; > + > + record.url = record.histUri; > + record_visit.date = PlacesUtils.toDate(record_visit.date); > + record_visit.visitDate = PlacesUtils.toDate(record_visit.visitDate); I don't think this is necessary, though...`visitDate` is a legacy `mozIVisitInfo` property, but `insertMany` takes plain JavaScript `VisitInfo` objects. ::: services/sync/modules/engines/history.js:304 (Diff revision 1) > + record_visit.date = PlacesUtils.toDate(record_visit.date); > + record_visit.visitDate = PlacesUtils.toDate(record_visit.visitDate); > + } > + > if (records.length) { > - blockers.push(new Promise(resolve => { > + blockers.push( As above, let's drop `blockers` and just do `await PlacesUtils.history.insertMany(records)`. ::: services/sync/modules/engines/history.js:325 (Diff revision 1) > * returns true if the record is to be applied, false otherwise > * (no visits to add, etc.), > */ > _recordToPlaceInfo: function _recordToPlaceInfo(record) { > // Sort out invalid URIs and ones Places just simply doesn't want. > record.uri = Utils.makeURI(record.histUri); `VisitInfo` calls this `url`, not `uri`. Let's change this line to read `record.url = PlacesUtils.normalizeToURLOrGUID(record.histUri)`, and remove the `if (!record.uri) { ... }` block below. `normalizeToURLOrGUID` will throw if `histUri` isn't valid, and avoid an unnecessary `nsIURI` conversion. ::: services/sync/modules/engines/history.js:380 (Diff revision 1) > // Visit is a dupe, don't increment 'k' so the element will be > // overwritten. > continue; > } > > visit.visitDate = visit.date; Let's replace this line with the one you added to `applyIncomingBatch`: `visit.date = PlacesUtils.toDate(visit.date)`. ::: services/sync/modules/engines/history.js:381 (Diff revision 1) > // overwritten. > continue; > } > > visit.visitDate = visit.date; > visit.transitionType = visit.type; According to the list of visit properties (https://searchfox.org/mozilla-central/rev/09c065976fd4f18d4ad764d7cb4bbc684bf56714/toolkit/components/places/History.jsm#46-57), I think this should be `transition`, not `transitionType`.

Comment on attachment 8891590 [details] Bug 1377944 - Converts the history engine to use 'PlacesUtils.history.insertMany'. https://reviewboard.mozilla.org/r/162702/#review168238 Thanks! Try looks green: https://treeherder.mozilla.org/#/jobs?repo=try&revision=96f1aa1211e2 Fix up the two remaining issues, and then we can land this. \o/ ::: services/sync/modules/engines/history.js:307 (Diff revision 2) > * (no visits to add, etc.), > */ > _recordToPlaceInfo: function _recordToPlaceInfo(record) { > // Sort out invalid URIs and ones Places just simply doesn't want. > + record.url = PlacesUtils.normalizeToURLOrGUID(record.histUri); > record.uri = Utils.makeURI(record.histUri); Drop the `record.uri` line. It's not needed now that you're setting `record.url` above, and `insertMany` will ignore it. ::: services/sync/modules/engines/history.js:359 (Diff revision 2) > // overwritten. > continue; > } > > + visit.date = PlacesUtils.toDate(visit.date); > visit.visitDate = visit.date; Drop `visitDate`, too.

Comment on attachment 8891590 [details] Bug 1377944 - Converts the history engine to use 'PlacesUtils.history.insertMany'. https://reviewboard.mozilla.org/r/162702/#review168376 ::: services/sync/modules/engines/history.js:307 (Diff revision 2) > * (no visits to add, etc.), > */ > _recordToPlaceInfo: function _recordToPlaceInfo(record) { > // Sort out invalid URIs and ones Places just simply doesn't want. > + record.url = PlacesUtils.normalizeToURLOrGUID(record.histUri); > record.uri = Utils.makeURI(record.histUri); if i drop that line, this test no pass: http://searchfox.org/mozilla-central/source/services/sync/tests/unit/test_history_store.js#64

Comment on attachment 8891590 [details] Bug 1377944 - Converts the history engine to use 'PlacesUtils.history.insertMany'. https://reviewboard.mozilla.org/r/162702/#review168376 > if i drop that line, this test no pass: > http://searchfox.org/mozilla-central/source/services/sync/tests/unit/test_history_store.js#64 Right, you'll want to remove the `canAddURI` call in that function, and other references to `record.uri`. But...I see that `insertMany` will throw if none of the incoming history records can be stored, which will cause the "javascript:" URL test to fail. I filed bug 1385989 to see if we can change `insertMany` to behave differently here. For now, let's keep `record.uri` and `canAddURI`, and remove them in a follow-up. Thanks for double-checking!

Comment on attachment 8891590 [details] Bug 1377944 - Converts the history engine to use 'PlacesUtils.history.insertMany'. https://reviewboard.mozilla.org/r/162702/#review168482 Thanks!

Pushed by kcambridge@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/26831e829c5a Converts the history engine to use 'PlacesUtils.history.insertMany'. r=kitcambridge

Forgot the part where I was supposed to comment about what that was, but that was a backout from beta at markh's request. It hasn't yet been backed out on the trunk.

And backed out of 57 in https://hg.mozilla.org/integration/mozilla-inbound/rev/9b4b07fd0391

also backedout from m-c https://hg.mozilla.org/mozilla-central/rev/9b4b07fd0391

It appears the symptoms in bug 1388024 aren't actually due to this bug. Kit, do you think we should just reland this?

Sure, let's reland this in 57. Good catch on bug 1390178!

Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/5322ba58976c Converts the history engine to use 'PlacesUtils.history.insertMany'. r=kitcambridge

ni myself to look at telemetry for the history engine once this has baked for a while.

(In reply to Mark Hammond [:markh] from comment #21) > ni myself to look at telemetry for the history engine once this has baked > for a while. I'm glad I did - bug 1405566.