This is used in tests, IE migrator and Safari migrator. Can be replaced with PlacesUtils.asyncHistory.updatePlaces, the migrators rewrite is ongoing, so what's left to do here is updating tests and removing the API.

This replaces addPageWithDetails with updatePlaces in cases where the call is not nested in a way that requires too much changes in the surrounding code or test framework. In other cases, the call to addPageWithDetails is temporarily replaced with the synchronous addVisit, sometimes followed by setPageTitle. addPageWithDetails is also marked deprecated in preparation for its removal.

Comment on attachment 613299 [details] [diff] [review] First patch Review of attachment 613299 [details] [diff] [review]: ----------------------------------------------------------------- nothing major (Apart the suckiness of some of the tests, but those are quite old), though in many cases you don't need to pass an array to updatePlaces, you need a SR, and the Sync part should be also reviewed by a Sync peer. A try run with a couple retriggers may help confirming this doesn't introduce some subtle random failure. ::: browser/base/content/test/browser_sanitize-timespans.js @@ +296,5 @@ > + addPlace(makeURI("http://1hour10minutes.com/"), "1 hour 10 minutes ago", now_uSec - 70*60*1000000); > + addPlace(makeURI("http://2hour.com/"), "Less than 2 hours ago", now_uSec - 90*60*1000000); > + addPlace(makeURI("http://2hour10minutes.com/"), "2 hours 10 minutes ago", now_uSec - 130*60*1000000); > + addPlace(makeURI("http://4hour.com/"), "Less than 4 hours ago", now_uSec - 180*60*1000000); > + addPlace(makeURI("http://4hour10minutes.com/"), "4 hours 10 minutesago", now_uSec - 250*60*1000000); assign 60 * 1000000 to a const kUsecPerMin and use the const, should help readability @@ +302,5 @@ > let today = new Date(); > today.setHours(0); > today.setMinutes(0); > today.setSeconds(1); > + addPlace(makeURI("http://today.com/"), "Today", today.valueOf() * 1000); is not .valueOf() implicit when you multiply? @@ +307,4 @@ > > let lastYear = new Date(); > lastYear.setFullYear(lastYear.getFullYear() - 1); > + addPlace(makeURI("http://before-today.com/"), "Before Today", lastYear.valueOf() * 1000); ditto @@ +311,5 @@ > > + PlacesUtils.asyncHistory.updatePlaces(places, { > + handleError: function () ok(false, "Unexpected error in adding visit."), > + handleResult: function () { }, > + handleCompletion: function () { actually, for sanity may be nice to check that handleResult has been called at least once. @@ +312,5 @@ > + PlacesUtils.asyncHistory.updatePlaces(places, { > + handleError: function () ok(false, "Unexpected error in adding visit."), > + handleResult: function () { }, > + handleCompletion: function () { > + confirmSetupHistory(); Otherwise, if we properly count the number of handleResult calls, we may even kill this additional check. ::: browser/base/content/test/newtab/browser_newtab_bug722273.js @@ +43,5 @@ > + } > + PlacesUtils.asyncHistory.updatePlaces(places, { > + handleError: function () do_throw("Unexpected error in adding visit."), > + handleResult: function () { }, > + handleCompletion: function () TestRunner.next() put 59 in a const, and check handleResult is called that many times. ::: browser/components/privatebrowsing/test/unit/do_test_placesTitleNoUpdate.js @@ +58,5 @@ > + visits: [{ > + visitDate: Date.now() * 1000, > + transitionType: Ci.nsINavHistoryService.TRANSITION_LINK > + }] > + }], just for readability, make a separate places obj (doesn't need to be an array, fwiw, if it's just a single place) @@ +75,1 @@ > do_check_eq(PlacesUtils.history.getPageTitle(TEST_URI), TITLE_1); may we rather check this in handleResult based on the place we get back? @@ +83,5 @@ > + visits: [{ > + visitDate: Date.now() * 2000, > + transitionType: Ci.nsINavHistoryService.TRANSITION_LINK > + }] > + }], ditto @@ +103,1 @@ > do_check_eq(PlacesUtils.history.getPageTitle(TEST_URI), TITLE_1); as well as this one. ::: browser/components/privatebrowsing/test/unit/do_test_removeDataFromDomain.js @@ +90,5 @@ > check_visited(aURI, false); > + let history = Cc["@mozilla.org/browser/nav-history-service;1"] > + .getService(Ci.nsINavHistoryService); > + history.addVisit(aURI, Date.now() * 1000, null, > + Ci.nsINavHistoryService.TRANSITION_LINK, false, 0); use PlacesUtils.history (if not available already just import it) ::: services/sync/tests/unit/test_history_store.js @@ +85,2 @@ > > + PlacesUtils.asyncHistory.updatePlaces( for the changes to Sync tests you want an additional review from rnewman... I suggest splitting these to a separate patch. @@ +90,5 @@ > + visits: [{ > + visitDate: TIMESTAMP1, > + transitionType: Ci.nsINavHistoryService.TRANSITION_LINK > + }] > + }], doesn't need to be an array ::: services/sync/tests/unit/test_places_guid_downgrade.js @@ +101,5 @@ > + visitDate: Date.now() * 1000, > + transitionType: Ci.nsINavHistoryService.TRANSITION_LINK > + }] > + } > + ], nit: could compress a bit the indentation and define a separate var for readability @@ +116,5 @@ > + let fxid = PlacesUtils.bookmarks.insertBookmark( > + PlacesUtils.bookmarks.toolbarFolder, > + uri, > + PlacesUtils.bookmarks.DEFAULT_INDEX, > + "Mozilla"); not sure what this Hack is meant to do, if (as I suppose) it was meant to do a Flush when we had temp tables to read the guid from the disk table, it can likely die. Richard may know more. ::: toolkit/components/places/nsIBrowserHistory.idl @@ +60,2 @@ > */ > + [deprecated] needs SR ::: toolkit/components/places/tests/bookmarks/test_savedsearches.js @@ +70,5 @@ > > +// a search term that matches a default bookmark > +var searchTerm = "about"; > + > +var testRoot; man, this test sucks... ::: toolkit/components/places/tests/queries/head_queries.js @@ +112,5 @@ > + PlacesUtils.history.addVisit( > + uri(qdata.uri), > + qdata.lastVisit, > + null, > + Ci.nsINavHistoryService.TRANSITION_LINK, I don't mind if you go a bit over the 80 chars, provided you don't use fancy indentation :) Btw notice that head_common.js has shortcuts for transitions, so you can just use TRANSITION_LINK in any Places xpcshell test. ::: toolkit/components/places/tests/unit/test_resolveNullBookmarkTitles.js @@ +69,5 @@ > + var query = PlacesUtils.history.getNewQuery(); > + // if we don't set a tag folder, RESULTS_AS_TAG_CONTENTS will return all > + // tagged URIs > + var result = PlacesUtils.history.executeQuery(query, options); > + var root = result.root; coalesce to root = PlacesUtils.history.executeQuery(query, options).root; since you don't need result. btw, since your practically rewriting this test, please convert it to use let

Comment on attachment 618083 [details] [diff] [review] Updated patch, excluding Sync parts Review of attachment 618083 [details] [diff] [review]: ----------------------------------------------------------------- Will need SR once comments are addressed. ::: browser/base/content/test/browser_sanitize-timespans.js @@ +19,5 @@ > + setupFormHistory(); > + setupHistory(afterSetupHistory); > +} > + > +function afterSetupHistory() { nit: onHistoryReady may be clearer, globally using onXReady than afterXDone would be better. ::: browser/base/content/test/newtab/browser_newtab_bug722273.js @@ +32,5 @@ > let uri = makeURI(URL); > + let places = []; > + for (let i = 59; i > 0; i--) { > + places.push({ > + uri: uri, if the uri is the same, you should rather add a single place object and generate its visits in a loop, that's how updatePlaces is supposed to be used (to avoid adding multiple times the same page). ::: browser/components/privatebrowsing/test/unit/do_test_removeDataFromDomain.js @@ +362,5 @@ > pb.removeDataFromDomain("mozilla.org"); > check_visited(TEST_URI, true); > > // Clear history since we left something there from this test. > + PlacesUtils.bhistory.removeAllPages(); I hope no test after this one uses history, cause would likely generate a random failure... ::: toolkit/components/places/nsIBrowserHistory.idl @@ +62,3 @@ > void addPageWithDetails(in nsIURI aURI, > in wstring aTitle, > in long long aLastVisited); This actually hurts us from a Snappy point of view (blocking other snappy work), so I'd prefer if we'd directly remove it, please send a mail to Jorge to notify add-ons using it about the removal and the alternative. I can blog post about the removal of AddPageWithDetails and AddVisit too, the latter may take more time but it's ok to start with a heads-up. ::: toolkit/components/places/tests/bookmarks/test_savedsearches.js @@ +68,5 @@ > // get bookmarks root id > var root = bmsvc.bookmarksMenuFolder; > > +// a search term that matches a default bookmark > +var searchTerm = "about"; looks like a const

Removing addPageWithDetails entirely as per comment 4.

(In reply to Marco Bonardo [:mak] from comment #4) > This actually hurts us from a Snappy point of view (blocking other snappy > work), so I'd prefer if we'd directly remove it, please send a mail to Jorge > to notify add-ons using it about the removal and the alternative. Jorge, can you take the appropriate actions, if needed, to notify add-on developers that the addPageWithDetails API will be removed in Firefox 15? The suggested alternative is the updatePlaces asyncronous API, though for the moment there is a temporary alternative using addVisit and setPageTitle, if making the calling code asynchronous is difficult.

(In reply to Paolo Amadini [:paolo] from comment #6) > though for the moment there is a temporary alternative using addVisit and > setPageTitle, if making the calling code asynchronous is difficult. How long do we expect that solution to be present? Seems dangerous to even mention it as an alternative if we're actively trying to get rid of those synchronous APIs.

(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #7) > (In reply to Paolo Amadini [:paolo] from comment #6) > > though for the moment there is a temporary alternative using addVisit and > > setPageTitle, if making the calling code asynchronous is difficult. > > How long do we expect that solution to be present? Seems dangerous to even > mention it as an alternative if we're actively trying to get rid of those > synchronous APIs. Right, I didn't think that, even if it requires some time, it is probably going away in the same release cycle, so no need to mention it.

well probably not the same reelase cycle, addVisit is trickier to remove, but I agree we should only suggest updatePlaces.

Unbitrotted and pushed to inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/4b199744d601

Docs updated: https://developer.mozilla.org/en/XPCOM_Interface_Reference/nsIBrowserHistory Mentioned on Firefox 15 for developers.