We are deprecating AddVisit() in Places, and we plan to remove it, so any call to it should be removed. We already fixed most of toolkit, though any browser change should be ported, especially check which test changes in bug 820764 will have to be ported.

So I suggest you wait we fix all the browser tests in m-c, and then you port the new version of the tests. At that point any remaining call should be fixed.

so basically, port bug 820764 (once it's done) and then fix anything left.

This can now be worked on, we removed all the calls from FF tests, most of shared tests can probably be just ported

looks like the tests that should be fixed (or just ported from mozilla-central) are: /suite/common/places/tests/browser/browser_library_infoBox.js /suite/common/places/tests/chrome/test_treeview_date.xul /suite/common/places/tests/chrome/test_bug549192.xul /suite/common/places/tests/chrome/test_bug549491.xul /suite/common/places/tests/unit/test_clearHistory_shutdown.js /suite/modules/test/browser_sanitizer.js

We are quite near removal of the APIs on central, is any SM contributor interested in working on this bug?

directly blocking the meta bug than the deprecation bug.

(In reply to Marco Bonardo [:mak] from comment #5) > We are quite near removal of the APIs on central, is any SM contributor > interested in working on this bug? From the usual meaning of "quite near", probably not me. After looking at my agenda, and considering that I don't know anything about writing tests, I might look at it in the first fortnight of April if no one else does it first, but I guess April is too late for you.

(In reply to Tony Mechelynck [:tonymec] from comment #7) > I guess April is too late for you. yep, it's a bit too far, but thanks for the offer.

any news here? we are about to proceed with the removal in central.

Neil: This patch basically ports the test fixes from Firefox, so the changes have already been reviewed once. I'll attach a qdiff -w version of the patch as most parts of the patch only change the indention of some code.

The "isVisited" check is obsolete as addVisits in head.js takes care of that.

Comment on attachment 737034 [details] [diff] [review] Patch >@@ -56,27 +57,28 @@ > } > > function continue_test() { >- PlacesUtils.history >- .addVisit(Services.io.newURI("http://example.tld/", null, null), >- Date.now() * 1000, null, PlacesUtils.history.TRANSITION_TYPED, false, 0); >+ {uri: Services.io.newURI("http://example.tld/", null, null), >+ visitDate: Date.now() * 1000, >+ transition: PlacesUtils.history.TRANSITION_TYPED}, >+ funcion() { This change doesn't look right...

Sorry, I've only ran parts of the places test suite by mistake, that is why I did not catch this.

Forgot to hg add the new head.js file for the chrome tests.

Comment on attachment 737110 [details] [diff] [review] Patch 3 >diff --git a/suite/common/places/tests/browser/head.js b/suite/common/places/tests/browser/head.js >--- a/suite/common/places/tests/browser/head.js >+++ b/suite/common/places/tests/browser/head.js >@@ -37,3 +37,63 @@ function waitForClearHistory(aCallback) > }, PlacesUtils.TOPIC_EXPIRATION_FINISHED, false); > PlacesUtils.bhistory.removeAllPages(); > } So, it looks as if we can use PlacesUtils here... >+ >+/** >+ * Asynchronously adds visits to a page, invoking a callback function when done. >+ * >+ * @param aPlaceInfo >+ * Can be an nsIURI, in such a case a single LINK visit will be added. >+ * Otherwise can be an object describing the visit to add, or an array >+ * of these objects: >+ * { uri: nsIURI of the page, >+ * transition: one of the TRANSITION_* from nsINavHistoryService, >+ * [optional] title: title of the page, >+ * [optional] visitDate: visit date in microseconds from the epoch >+ * [optional] referrer: nsIURI of the referrer for this visit >+ * } >+ * @param [optional] aCallback >+ * Function to be invoked on completion. >+ * @param [optional] aStack >+ * The stack frame used to report errors. >+ */ >+function addVisits(aPlaceInfo, aWindow, aCallback, aStack) { >+ let stack = aStack || Components.stack.caller; >+ let places = []; >+ if (aPlaceInfo instanceof Ci.nsIURI) { >+ places.push({ uri: aPlaceInfo }); >+ } >+ else if (Array.isArray(aPlaceInfo)) { >+ places = places.concat(aPlaceInfo); >+ } else { >+ places.push(aPlaceInfo) >+ } >+ >+ // Create mozIVisitInfo for each entry. >+ let now = Date.now(); >+ for (let i = 0; i < places.length; i++) { >+ if (!places[i].title) { >+ places[i].title = "test visit for " + places[i].uri.spec; >+ } >+ places[i].visits = [{ >+ transitionType: places[i].transition === undefined ? Ci.nsINavHistoryService.TRANSITION_LINK >+ : places[i].transition, >+ visitDate: places[i].visitDate || (now++) * 1000, >+ referrerURI: places[i].referrer >+ }]; >+ } >+ >+ aWindow.PlacesUtils.asyncHistory.updatePlaces( ... so it surprises me why you bother using aWindow. aStack also looks to be unused.

You're right, the stack var and the window var are not needed. I removed those two args/vars.

Comment on attachment 742865 [details] [diff] [review] Patch 4 Heh, so aWindow wasn't even documented ;-)

Pushed to comm-central: https://hg.mozilla.org/comm-central/rev/be2fd7f4ecf5

Comment on attachment 742865 [details] [diff] [review] Patch 4 [Approval Request Comment] Regression caused by (bug #): - User impact if declined: Test fix only Testing completed (on m-c, etc.): Works fine on comm-central (now comm-aurora) Risk to taking this patch (and alternatives if risky): No user impact, text-fix only; test fix needed because mozilla-beta does not have the old API functions used in the tests anymore String changes made by this patch: -

Comment on attachment 742865 [details] [diff] [review] Patch 4 Approving test-only fix for SM 2.19 branch (current comm-beta default branch). If you want to back-port to SM 2.18 (thinking that bug 820764 landed for mozilla21), that would be a new discussion/request.

Comment on attachment 742865 [details] [diff] [review] Patch 4 Pushed: https://hg.mozilla.org/releases/comm-beta/rev/5dfb6c9fa587 Not needed for SeaMonkey 2.18 as Bug 838872 landed on Gecko 22.

All branches should be fixed now.

(In reply to Frank Wein from comment #24) > Not needed for SeaMonkey 2.18 as Bug 838872 landed on Gecko 22. Your wording confused me! Bug 838872 didn't land until Gecko 22, so we only need to fix SeaMonkey 2.19 or later.