Comment on attachment 640348 [details] [diff] [review] Replacing use of addPage with async code. Review of attachment 640348 [details] [diff] [review]: ----------------------------------------------------------------- The underlying API is synchronous. By changing it to synchronous, you introduce a race condition where the TPS test driver could advance before data has been added to the database. The change to use the async APIs can be done: we just need to wait on the callback before returning from the function. You should be able to Cu.import("resource://services-common/async.js") and do something like: let cb = Async.makeSpinningCallback(); PlacesUtils.asyncHistory.updatePlaces(places, { handleError: function Add_handleError() { cb(new Error("Error adding history entry.")); } handleResult: function Add_handleResult() { cb(null); }, handleCompletion: function Add_handleCompletion() { cb(null); } }); cb.wait(); A proper solution would be to refactor the TPS APIs to be async. But, I don't think you want to tackle that ;)

Here we go. Local tests work, but with TryServer hiccups, I have not been able to get confirmation from TryServer.

(In reply to Gregory Szorc [:gps] from comment #1) > Comment on attachment 640348 [details] [diff] [review] > A proper solution would be to refactor the TPS APIs to be async. But, I > don't think you want to tackle that ;) Not quite yet :) By the way, is there a bug# for this? If you need OS.File features, feel free to ping me.

(In reply to David Rajchenbach Teller [:Yoric] from comment #2) > Created attachment 642229 [details] [diff] [review] > Replacing use of addPage with async code v2 > > Here we go. Local tests work, but with TryServer hiccups, I have not been > able to get confirmation from TryServer. TPS tests don't run on the buildbot infrastructure, so a Try push won't exercise this. There is some documentation at https://developer.mozilla.org/en/TPS_Tests. Not sure if it is current. > By the way, is there a bug# for this? No. We generally don't want to touch TPS unless we have to.

Ah, I just understood that what I was patching is just a test. This patch is probably not very useful, then.

Comment on attachment 642229 [details] [diff] [review] Replacing use of addPage with async code v2 Review of attachment 642229 [details] [diff] [review]: ----------------------------------------------------------------- This looks good. But, I can't verify it because my m-c builds are doing: assertion failure: target, at /Users/gps/src/services-central/storage/src/mozStorageAsyncStatementExecution.cpp:190 If this lands in services-central, TPS will run on the server on checkin. So, we can always try to land. If it breaks, we back out.

Comment on attachment 642229 [details] [diff] [review] Replacing use of addPage with async code v2 Review of attachment 642229 [details] [diff] [review]: ----------------------------------------------------------------- There's something to fix in the callback (trying to interrupt twice), apart that I will be fine with gps final review pass, I don't have further comments. ::: services/sync/tps/extensions/tps/modules/history.jsm @@ +107,5 @@ > Logger.AssertTrue("visits" in item && "uri" in item, > "History entry in test file must have both 'visits' " + > "and 'uri' properties"); > let uri = Services.io.newURI(item.uri, null, null); > + let places = { ideally this is a single "place" @@ +109,5 @@ > "and 'uri' properties"); > let uri = Services.io.newURI(item.uri, null, null); > + let places = { > + uri: uri, > + visits:[] missing space after colon @@ +116,5 @@ > + places.visits.push({ > + visitDate: usSinceEpoch + (visit.date * 60 * 60 * 1000 * 1000), > + transitionType: visit.type, > + referrerURI: null, > + sessionId: null since this is a jsval, you don't have to define null properties, they are optional, so can be omitted. @@ +122,3 @@ > } > + if ("title" in item) { > + places.title = item.title; I suppose (can be verified easily), you may just use title: item.title in the places object definition, if undefined/null the other side should get a void string and ignore it. @@ +131,5 @@ > + handleResult: function Add_handleResult() { > + cb(null); > + }, > + handleCompletion: function Add_handleCompletion() { > + cb(null); calling both in handleResult/Error and handleCompletion is going to notify and continue twice... you should just do one of those, I suggest using completion and track error/success internally.

David, do you plan to update the patch here, are you looking for help completing it?

I can find other addVisit entities in Sync: services/sync/tps/extensions/tps/modules/history.jsm services/sync/tests/unit/test_history_tracker.js

Bear in mind that Sync's history engine does handle visit-level granularity, so -- without yet taking the time to dig into the API; still drinking morning coffee! -- some use of visit-level APIs might be appropriate.

The fact is that nsINavHistoryService::AddVisit will not exist anymore, any visits update must go through mozIAsyncHistory::updatePlaces(), that is the async replacement for AddVisit. Nothing changes regarding granularity, it's just sync API being replaced by an async API.

(In reply to Marco Bonardo [:mak] from comment #11) > The fact is that nsINavHistoryService::AddVisit will not exist anymore, any > visits update must go through mozIAsyncHistory::updatePlaces(), that is the > async replacement for AddVisit. Nothing changes regarding granularity, it's > just sync API being replaced by an async API. Got it. Putting this on the list of things to keep an eye on…

(In reply to Marco Bonardo [:mak] from comment #8) > David, do you plan to update the patch here, are you looking for help > completing it? Sorry, your additional review had been lost in bugspam. I will try and update this soonish. How urgent is this patch?

(In reply to David Rajchenbach Teller [:Yoric] from comment #13) > Sorry, your additional review had been lost in bugspam. I will try and > update this soonish. How urgent is this patch? I'd like to remove addVisit in the Firefox 21 bracket, so by the end of Jan.

Also applied to test_history_tracker.js

Same one with yet another typo removed. Try: https://tbpl.mozilla.org/?tree=Try&rev=73ad8a53ec73

Comment on attachment 695083 [details] [diff] [review] Replacing use of addPage with async code v5 Review of attachment 695083 [details] [diff] [review]: ----------------------------------------------------------------- Stealing gps's review flag, because he has other things to do! Just nits and sadness :D Thanks for taking care of this, David! ::: services/sync/tests/unit/test_history_tracker.js @@ +35,5 @@ > + uri: uri, > + visits: [ { > + visitDate: Date.now() * 1000, > + transitionType: 1 > + } ] Nit: surplus spaces, trailing commas. Should be: let place = { uri: uri, visits: [{ transitionType: 1, visitDate: Date.now() * 1000, }], }; @@ +39,5 @@ > + } ] > + }; > + let cb = Async.makeSpinningCallback(); > + PlacesUtils.asyncHistory.updatePlaces(place, { > + handleError: function Add_handleError() { Nit: lose the "Add_". @@ +47,5 @@ > + cb(null); > + }, > + handleCompletion: function Add_handleCompletion(status) { > + // Nothing to do > + } Nit: correct indentation on these two lines, add period at end of comment, add trailing comma. @@ +50,5 @@ > + // Nothing to do > + } > + }); > + // Spin the event loop to embed this async call in a sync API > + cb.wait(); Given that this is test code, I'd rather not, but meh! We'll fix it when we get rid of trackers. ::: services/sync/tps/extensions/tps/modules/history.jsm @@ +120,5 @@ > } > + if ("title" in item) { > + place.title = item.title; > + } > + let cb = Async.makeSpinningCallback(); Newline before "let cb", please. @@ +130,5 @@ > + cb(null); > + }, > + handleCompletion: function Add_handleCompletion(status) { > + // Nothing to do > + } Same comments apply here. (It's a pity we have this code duplication. Oh well.)

Comment on attachment 695083 [details] [diff] [review] Replacing use of addPage with async code v5 Review of attachment 695083 [details] [diff] [review]: ----------------------------------------------------------------- ::: services/sync/tests/unit/test_history_tracker.js @@ +34,5 @@ > + let place = { > + uri: uri, > + visits: [ { > + visitDate: Date.now() * 1000, > + transitionType: 1 Please replace this hardcoded number with the proper PlacesUtils.history.TRANSITION_LINK @@ +43,5 @@ > + handleError: function Add_handleError() { > + cb(new Error("Error adding history entry")); > + }, > + handleResult: function Add_handleResult() { > + cb(null); I think there's no point in the explicit null argument, just omit it... @@ +46,5 @@ > + handleResult: function Add_handleResult() { > + cb(null); > + }, > + handleCompletion: function Add_handleCompletion(status) { > + // Nothing to do since you don't use status you can avoid defining it ::: services/sync/tps/extensions/tps/modules/history.jsm @@ +120,3 @@ > } > + if ("title" in item) { > + place.title = item.title; much <3 for removing another instance of setPageTitle (bug 752217) :) @@ +126,5 @@ > + handleError: function Add_handleError() { > + cb(new Error("Error adding history entry")); > + }, > + handleResult: function Add_handleResult() { > + cb(null); ditto @@ +129,5 @@ > + handleResult: function Add_handleResult() { > + cb(null); > + }, > + handleCompletion: function Add_handleCompletion(status) { > + // Nothing to do ditto on status

Try: https://tbpl.mozilla.org/?tree=Try&rev=79289967812d

Comment on attachment 699080 [details] [diff] [review] Replacing use of addPage with async code v6 Review of attachment 699080 [details] [diff] [review]: ----------------------------------------------------------------- ::: services/sync/tps/extensions/tps/modules/history.jsm @@ +115,3 @@ > for each (visit in item.visits) { > + place.visits.push({ > + visitDate: usSinceEpoch + (visit.date * 60 * 60 * 1000 * 1000), While you're here, could you lift that constant somewhere and make it meaningful: is it MICROSECONDS_PER_HOUR? Is that even correct?