820797 - Remove calls to addvisit() from Seamonkey

Status: RESOLVED FIXED

(SeaMonkey :: Bookmarks & History, defect)

Description

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

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.

Comment 1

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

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.

Comment 2

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

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

Comment 3

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

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

Comment 4

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

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

Comment 5

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

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

Comment 6

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

directly blocking the meta bug than the deprecation bug.

Comment 7

[Tony Mechelynck [:tonymec]]

(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.

Comment 8

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(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.

Comment 9

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

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

Comment 10

[Frank Wein [:mcsmurf]]

Attachment: Patch

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.

Comment 11

[Frank Wein [:mcsmurf]]

Attachment: diff -w version of patch

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

Comment 12

[[e-mail]]

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...

Comment 13

[Frank Wein [:mcsmurf]]

Attachment: Patch 2

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

Comment 14

[Frank Wein [:mcsmurf]]

Attachment: diff -w version of patch

Comment 15

[Frank Wein [:mcsmurf]]

Attachment: Patch 3

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

Comment 16

[Frank Wein [:mcsmurf]]

Attachment: diff -w version of patch

Comment 17

[[e-mail]]

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.

Comment 18

[Frank Wein [:mcsmurf]]

Attachment: Patch 4

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

Comment 19

[Frank Wein [:mcsmurf]]

Attachment: diff -w version of patch

Comment 20

[[e-mail]]

Comment on attachment 742865 [details] [diff] [review]
Patch 4

Heh, so aWindow wasn't even documented ;-)

Comment 21

[Frank Wein [:mcsmurf]]

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

Comment 22

[Frank Wein [:mcsmurf]]

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 23

[Jens Hatlak (:InvisibleSmiley)]

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 24

[Frank Wein [:mcsmurf]]

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.

Comment 25

[Frank Wein [:mcsmurf]]

All branches should be fixed now.

Comment 26

[[e-mail]]

(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.