need to actually add them first...

From discussing with sdwilsh, this is what it'll take for Sync to avoid triggering bug 606062, which really just looks like a specific case of bug 563538, which is why it's blocking final releases on desktop/mobile. Still figuring out resources, but one way or another this needs to happen.

Reflect the current state of this bug: so far only mozIAsyncHistory::updatePlaces has been implemented, so we can only do this for history sync for now. Filed bug 626279 for the bookmark stuff (which still requires bug 519514 to be implemented in the first place).

I have outlined a proposal for changing the store/engine API to allow batching in bug 622762 comment 11. With this in place, the history engine will apply incoming records in batches by calling mozIAsyncHistory::updatePlaces with multiple records at a time.

The new places API is much more strict so a bunch of tests have to be hardened to produce valid GUIDs and valid visits (WHY-OH-WHY does test_corrupt_keys use actual engines???). Also added tests for the kinds of records we won't (can't) be accepting in the future. Some of these fail with just this patch applied as the actual checking isn't happening yet.

Comment on attachment 507601 [details] [diff] [review] Preq (v1): Utils.checkGUID Wouldn't it be more efficient to just grab https://hg.mozilla.org/mozilla-central/annotate/e98b94aa64fa/toolkit/components/places/tests/head_common.js#l603? Might want to ask cdleary or someone else on the JS team.

This refactors the history engine to use mozIAsyncHistory::updatePlaces when available. Makes the tests in part 0 pass.

Comment on attachment 507601 [details] [diff] [review] Preq (v1): Utils.checkGUID Any reason not to do: var re = /^[-abcdefghijklmnopqrstuvwxyz0123456789_]{12}$/i; function checkGUID(guid) { return !!guid && re.test(guid); } In informal testing on my machine, this is about 30x faster than trolling over the array calling a JS function.

(In reply to comment #9) > Any reason not to do: > > var re = /^[-abcdefghijklmnopqrstuvwxyz0123456789_]{12}$/i; > function checkGUID(guid) { > return !!guid && re.test(guid); > } My (irrational?) hatred of regular expressions. > In informal testing on my machine, this is about 30x faster than trolling over > the array calling a JS function. Pretty convincing. I'll fix.

For queen and country.

Comment on attachment 507617 [details] [diff] [review] Preq (v2): Utils.checkGUID Make it so!

Try build: http://tbpl.mozilla.org/?tree=MozillaTry&rev=ed45880ddada

Comment on attachment 507603 [details] [diff] [review] Part 0: Improve and harden history sync tests 1. Can you pull out Engines.get("history")._store in lines 176 and 228 onwards, rather than wrapping the lines? 2. In test_history_store.js: please align those dump() calls. (Also, dumpn rather than + "\n"?) 3. These checks: do_check_eq(failed[0], no_date_visit_guid); look like they would fail if we switched representations (internally or externally) and lost the ordering. Is there any reason to assume an ordering in the result here? If not, can you put a .sort() in there before running the checks? Otherwise, looks good, and tests pass with just this patch in my queue.

(In reply to comment #14) > Otherwise, looks good, and tests pass with just this patch in my queue. Scratch that. Did I mention how much I hate mq? Test fails because applyIncomingBatch doesn't exist, of course. Validating with the other patch now.

(In reply to comment #14) > 1. Can you pull out > > Engines.get("history")._store > > in lines 176 and 228 onwards, rather than wrapping the lines? Will do. > 2. In test_history_store.js: please align those dump() calls. (Also, dumpn > rather than + "\n"?) Bah, these aren't supposed to be in there. I just added them for debugging. Will remove. > 3. These checks: > > do_check_eq(failed[0], no_date_visit_guid); > > look like they would fail if we switched representations (internally or > externally) and lost the ordering. Is there any reason to assume an ordering in > the result here? If not, can you put a .sort() in there before running the > checks? Good catch. No reason to assume ordering. In other places I have used .sort(), so will do that.

Comment on attachment 507605 [details] [diff] [review] Part 1: Use mozIAsyncHistory::updatePlaces when available Patch fails to apply on top of tests: applying bug-606353-1.patch patching file services/sync/modules/constants.js Hunk #1 FAILED at 84 1 out of 1 hunks FAILED -- saving rejects to file services/sync/modules/constants.js.rej patching file services/sync/modules/engines/history.js Hunk #2 FAILED at 71 1 out of 5 hunks FAILED -- saving rejects to file services/sync/modules/engines/history.js.rej patch failed, unable to continue (try -v) patch failed, rejects left in working dir errors during apply, please fix and refresh bug-606353-1.patch I believe I'm missing some constants from another patch in your queue. Easy enough to get it to apply by hand, but consequently take this review with a pinch of salt. One error in test_history_store, right after "Let's modify the record and have the store update the database.": [Exception... "Component returned failure code: 0x80004001 (NS_ERROR_NOT_IMPLEMENTED) [mozIAsyncHistory.updatePlaces]" presumably because I'm not running with the necessary Places changes. Comments: 450 }, this).filter(function (placeInfo) { 451 return placeInfo; 452 }); This took me a minute to figure out, and I'm a functional programmer! Please consider defining: function identity(x) x; and using .filter(identity); instead. The functional style is much clearer when the language exposes all the right vocabulary... Typo: 495 this._log.trace("Ignorning record " + record.id + and again in 529. Otherwise, looks good...

(In reply to comment #17) > Patch fails to apply on top of tests: ... > I believe I'm missing some constants from another patch in your queue. Easy > enough to get it to apply by hand, but consequently take this review with a > pinch of salt. Erm, the test patch doesn't even touch history.js or constants.js. This is what my patch queue looks like: 0 A bug622762: Bug 622762 - Add batch API for incoming records, deal with exceptions in SyncEngine's recordHandler 1 A bug606353-tests: Bug 606353 - Improve and harden history sync tests 2 A bug606353-util: Bug 606353 - Add a Utils.checkGUID() helper 3 A bug606353: Bug 606353 - History sync: use mozIAsyncHistory::updatePlaces It applied cleanly on all of my three checkouts (fx-sync, places, mozilla-central). > One error in test_history_store, right after "Let's modify the record and have > the store update the database.": > > [Exception... "Component returned failure code: 0x80004001 > (NS_ERROR_NOT_IMPLEMENTED) [mozIAsyncHistory.updatePlaces]" > > presumably because I'm not running with the necessary Places changes. Update your mozilla-central build? > Comments: > > 450 }, this).filter(function (placeInfo) { > 451 return placeInfo; > 452 }); > > This took me a minute to figure out, and I'm a functional programmer! Please > consider defining: > > function identity(x) x; > > and using > > .filter(identity); > > instead. The functional style is much clearer when the language exposes all the > right vocabulary... Can do.

(In reply to comment #18) > my patch queue looks like: Yup, was just missing a dep. > Update your mozilla-central build? Done. 97 test passes. Thumbs up! (Marking as ASSIGNED, btw.)

Address rnewman's review comments from Comment 14.

Address rnewman's review comments: * Introduce function identity for the filter * Fix typos

Crossweave tests with latest tinderbox build (which has mozIAsyncHistory::updatePlaces) pass. Just needs bug 622762 to land now, otherwise good to go!

New try build: http://tbpl.mozilla.org/?tree=MozillaTry&rev=1eddde320380

Part 0: https://hg.mozilla.org/services/fx-sync/rev/833e18225829 Preq: https://hg.mozilla.org/services/fx-sync/rev/2afdc6b14ad5 Part 1: https://hg.mozilla.org/services/fx-sync/rev/5dc7db5e0192

Verified on fennec nightlies with Mozilla/5.0 (Android; Linux armv71; rv:2.0b11pre) Gecko/20110204 Firefox/4.0b11pre Fennec/4.0b5pre. Tested on a Nexus One, Nexus S. Most of the testing involved: - syncing history only, large records (2.5 MB) - disabling sync logging - strip out form data history records - sync on fennec, and pan over to content pages to resume functional tests (panning, zooming, open a new tab, loading web content, typing into url and search boxes) Result: user performance during sync was noticeably faster than when i did the same scenario on a nightly build prior to this fix. Since this is a large patch, awaiting others to comment before marking verified. Also, looking for feedback on desktop testing.