Closed Bug 820797 Opened 7 years ago Closed 7 years ago

Remove calls to addvisit() from Seamonkey

Categories

(SeaMonkey :: Bookmarks & History, defect)

defect
Not set

Tracking

(seamonkey2.19 fixed)

RESOLVED FIXED
seamonkey2.20
Tracking Status
seamonkey2.19 --- fixed

People

(Reporter: mak, Assigned: mcsmurf)

References

Details

Attachments

(2 files, 6 obsolete files)

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.
Depends on: 820764
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.
Blocks: 834457
No longer blocks: 700250
(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.
Depends on: 838872
Attached patch Patch (obsolete) — Splinter Review
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.
Assignee: nobody → bugzilla
Status: NEW → ASSIGNED
Attachment #737034 - Flags: review?(neil)
Attached patch diff -w version of patch (obsolete) — Splinter Review
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...
Attached patch Patch 2 (obsolete) — Splinter Review
Sorry, I've only ran parts of the places test suite by mistake, that is why I did not catch this.
Attachment #737034 - Attachment is obsolete: true
Attachment #737034 - Flags: review?(neil)
Attachment #737105 - Flags: review?(neil)
Attached patch diff -w version of patch (obsolete) — Splinter Review
Attachment #737036 - Attachment is obsolete: true
Attached patch Patch 3 (obsolete) — Splinter Review
Forgot to hg add the new head.js file for the chrome tests.
Attachment #737105 - Attachment is obsolete: true
Attachment #737105 - Flags: review?(neil)
Attachment #737110 - Flags: review?(neil)
Attached patch diff -w version of patch (obsolete) — Splinter Review
Attachment #737106 - Attachment is obsolete: true
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.
Attached patch Patch 4Splinter Review
You're right, the stack var and the window var are not needed. I removed those two args/vars.
Attachment #737110 - Attachment is obsolete: true
Attachment #737110 - Flags: review?(neil)
Attachment #742865 - Flags: review?(neil)
Attachment #737111 - Attachment is obsolete: true
Comment on attachment 742865 [details] [diff] [review]
Patch 4

Heh, so aWindow wasn't even documented ;-)
Attachment #742865 - Flags: review?(neil) → review+
Pushed to comm-central: https://hg.mozilla.org/comm-central/rev/be2fd7f4ecf5
Target Milestone: --- → seamonkey2.20
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: -
Attachment #742865 - Flags: approval-comm-beta?
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.
Attachment #742865 - Flags: approval-comm-beta? → approval-comm-beta+
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.
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
(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.
You need to log in before you can comment on or make changes to this bug.