Closed Bug 806693 Opened 7 years ago Closed 7 years ago

Port browser_privatebrowsing_placestitle.js to the new per-window PB APIs

Categories

(Firefox :: Private Browsing, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 20

People

(Reporter: ehsan, Assigned: andreshm)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

http://mxr.mozilla.org/mozilla-central/source/browser/components/privatebrowsing/test/browser/global/browser_privatebrowsing_placestitle.js

In order to port this test, the file needs to be copied to the perwindow/
directory, and then instead of setting privateBrowsingEnabled, we need to open
a new private browsing window and then run the test on that window.

Note that the per-window version of this test can have both the private and
non-private window simultaneously open as opposed to the current way that the
test is structured.
Assignee: nobody → andres
Status: NEW → ASSIGNED
Attached patch Patch v1 (obsolete) — Splinter Review
The test is running fine. However, as I understand the test make sure that on private windows the history entries are not change. But, this is not true and the returned title is changed on the private window.
Attachment #681204 - Flags: review?(ehsan)
This requires the patches in bug 722850 to land as well.
Depends on: 722850
Attachment #681204 - Flags: review?(ehsan) → review+
Then probably the last condition should be:

> case 4:
>   // The second time that the page is loaded but in a private window
>   is(aPageTitle, "No Cookie",
>      "The page should be loaded with a cookie for the second time");

Since, it doesn't change on private window.
(In reply to comment #3)
> Then probably the last condition should be:
> 
> > case 4:
> >   // The second time that the page is loaded but in a private window
> >   is(aPageTitle, "No Cookie",
> >      "The page should be loaded with a cookie for the second time");
> 
> Since, it doesn't change on private window.

No, the cookie database is currently broken in per-window PB builds until bug 722850 gets fixed.
Andres, now that bug 722850 has been fixed, can you please test to see if this test works, so that we can land it?  Thanks!
I have been trying to run it, but the onTitleChanged observer, doesn't seen to be registered (or get notified) for the private window. 

> PlacesUtils.history.addObserver(historyObserver, false);

Do you know if any of the latest commits, change this to ignore PB windows?
Josh might know.
This is expected behaviour. We don't modify the places database for private windows, so no notifications occur. The original test did not attempt to observe this behaviour in PB mode either.
Attached patch Patch v2 (obsolete) — Splinter Review
Updated patch.
Attachment #681204 - Attachment is obsolete: true
Attachment #683614 - Flags: review?(ehsan)
Comment on attachment 683614 [details] [diff] [review]
Patch v2

Review of attachment 683614 [details] [diff] [review]:
-----------------------------------------------------------------

The original test goes into PB mode <http://mxr.mozilla.org/mozilla-central/source/browser/components/privatebrowsing/test/browser/global/browser_privatebrowsing_placestitle.js#45>, loads a tab, and this check here <http://mxr.mozilla.org/mozilla-central/source/browser/components/privatebrowsing/test/browser/global/browser_privatebrowsing_placestitle.js#57> makes sure that this tab load doesn't cause a places title change.  The new version of the test doesn't seem to be doing anything with the private window that it opens.  I think you should do something similar to the existing test there as well.
Attachment #683614 - Flags: review?(ehsan) → review-
Attached patch Patch v3Splinter Review
Fixed patch.
Attachment #683614 - Attachment is obsolete: true
Attachment #683698 - Flags: review?(ehsan)
Comment on attachment 683698 [details] [diff] [review]
Patch v3

Review of attachment 683698 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great, thanks!
Attachment #683698 - Flags: review?(ehsan) → review+
https://hg.mozilla.org/mozilla-central/rev/58b09385cd70
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 20
You need to log in before you can comment on or make changes to this bug.