Marcia has specified this as one of the pieces for the QA test plan of Private Browsing. This can, however, be written as a Mochitest, and I think we should do it. Further Litmus tests may not even be necessary for this, but I'll leave this decision to Marcia. I'll post the mochitest here.

The mochitest tries to add a visit in the places DB by a natural page load, and verifies that visited link coloring works outside of the private mode, but doesn't inside of the private mode.

Changed the test to use the keep_current_session pref instead of the private-browsing-enter notification which has been removed in the latest PB patch.

Dietrich: ping?

I found a timing issue in the previous version of this patch which made the test fail if it ran too quickly. This should fix the issue.

Comment on attachment 345955 [details] [diff] [review] Patch (v1.2) >diff --git a/toolkit/components/places/tests/Makefile.in b/toolkit/components/places/tests/Makefile.in >--- a/toolkit/components/places/tests/Makefile.in >+++ b/toolkit/components/places/tests/Makefile.in >@@ -53,24 +53,26 @@ XPCSHELL_TESTS = \ > bookmarks \ > queries \ > unit \ > $(NULL) > > # Simple MochiTests > MOCHI_TESTS = mochitest/test_bug_405924.html \ > mochitest/test_bug_411966.html \ >+ mochitest/test_bug_461710.html \ > $(NULL) > > MOCHI_CONTENT = mochitest/prompt_common.js \ > $(NULL) > fix tab indent here >+ >+ ok(true, "Starting test #" + testNum); remove >+var _PBSvc = null; >+function get_PBSvc() { >+ if (_PBSvc) >+ return _PBSvc; >+ >+ netscape.security.PrivilegeManager.enablePrivilege('UniversalXPConnect'); >+ try { >+ _PBSvc = Cc["@mozilla.org/privatebrowsing;1"]. >+ getService(Ci.nsIPrivateBrowsingService); >+ return _PBSvc; >+ } catch (e) {} >+ return null; >+} don't you want to test for failure to get the service? >+ >+ >+var ignoreLoad = false; >+function handleLoad(aEvent) { >+ netscape.security.PrivilegeManager.enablePrivilege('UniversalXPConnect'); >+ >+ ok(true, "Processing subtest #" + testNum); remove >+ >+ checkTest(); >+ >+ if (testNum < subtests.length) { >+ setTimeout(function() { >+ loadNextTest(); >+ }, 500); >+ } else { >+ ok(true, "private browsing visited coloring test finished."); remove >+ prefBranch.clearUserPref("browser.privatebrowsing.keep_current_session"); >+ SimpleTest.finish(); >+ } >+} >+ >+ >+var pb = get_PBSvc(); >+if (!pb) { // Private Browsing might not be available >+ ok(true, "Private browsing service is not available"); not a test, and why is it ok? shouldn't we fail in this case?

Comment on attachment 345992 [details] [diff] [review] Patch (v1.3) r=me thanks!

When I run the all the mochitests, I get one error in the testcase - reproduced several times. Running only the test or only tests/toolkit/components or similar doesn't cause the problem. I don't know which error it is. I guess I could log all the stdout messages and search for the error.

(In reply to comment #9) > When I run the all the mochitests, I get one error in the testcase - reproduced > several times. Running only the test or only tests/toolkit/components or > similar > doesn't cause the problem. > I don't know which error it is. I guess I could log all the stdout messages and > search for the error. This might be a timing issue I suspect, but it would be great if you can report which test is failing, and also which platform you're on, and any other details you feel might be important.

After updating I don't get the problem anymore. Something was backed out, I think.

Now I got the problem again :(

This is happening on the Tinderboxes too, just had a random failure: http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1225940222.1225943866.25049.gz

I have thought of another more reliable way to fix the random failure issue. Instead of loading each page with 500ms delay in the hope that everything happens correctly, let's split the visited and link pages to three separate files. This way, the test would only fail if what we do in IsVisited is not correct, not because of some random timing issues. With this patch, I was able to safely get rid of the timeout as well.

REOPENING because the whole purpose of this bug was to write a test which passes. :-)

Dietrich: ping?

Comment on attachment 346664 [details] [diff] [review] Followup fix r=me

Test fix checked in: http://hg.mozilla.org/mozilla-central/rev/1899ea13f9e7

Cv1, without the extraneous change :-<

Comment on attachment 369617 [details] [diff] [review] (Cv1a) Add |ok(true, ...);| [Checkin: Comment 24 & 25] what's the purpose of this?

(In reply to comment #21) > what's the purpose of this? See bug 483633 comment 3.

Comment on attachment 369617 [details] [diff] [review] (Cv1a) Add |ok(true, ...);| [Checkin: Comment 24 & 25] r=me

Comment on attachment 369617 [details] [diff] [review] (Cv1a) Add |ok(true, ...);| [Checkin: Comment 24 & 25] http://hg.mozilla.org/mozilla-central/rev/83e98dcd8439

Comment on attachment 369617 [details] [diff] [review] (Cv1a) Add |ok(true, ...);| [Checkin: Comment 24 & 25] http://hg.mozilla.org/releases/mozilla-1.9.1/rev/074a174d86dd