806741 - Port test_bug_461710.html to the new per-tab PB APIs

Status: RESOLVED FIXED

(Firefox :: Private Browsing, defect)

Description

[(no longer active)]

http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/tests/mochitest/test_bug_461710.html

In order to port this test, the file needs to be copied to the same directory (perhaps with "_perwindowpb" appended to its file name), 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 original test should only be added to the list of test files in Makefile.in ifndef MOZ_PER_WINDOW_PRIVATE_BROWSING, and the new test file should be added to the list with the reverse condition.

Comment 1

[(no longer active)]

Removed this test from per-window PB builds: https://hg.mozilla.org/mozilla-central/rev/d142cca052c3

Comment 2

[Raymond Lee [:raymondlee]]

Attachment: v1

Comment 3

[Raymond Lee [:raymondlee]]

Attachment: v2

Forgot to add a file in last patch

Comment 4

[Josh Matthews [:jdm]]

Comment on attachment 693271 [details] [diff] [review]
v2

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

::: toolkit/components/places/tests/mochitest/test_bug_461710.html
@@ +15,5 @@
>  <script class="testbody" type="text/javascript">
>  
>  /** Test for Bug 461710 **/
>  
> +netscape.security.PrivilegeManager.enablePrivilege('UniversalXPConnect');

We want to avoid adding more of these. Use SpecialPowers when necessary, please. You can usually just use SpecialPowers.wrap(obj).foo when obj.foo requires chrome privileges.

Comment 5

[Raymond Lee [:raymondlee]]

(In reply to Josh Matthews [:jdm] from comment #4)
> Comment on attachment 693271 [details] [diff] [review]
> v2
> 
> Review of attachment 693271 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/components/places/tests/mochitest/test_bug_461710.html
> @@ +15,5 @@
> >  <script class="testbody" type="text/javascript">
> >  
> >  /** Test for Bug 461710 **/
> >  
> > +netscape.security.PrivilegeManager.enablePrivilege('UniversalXPConnect');
> 
> We want to avoid adding more of these. Use SpecialPowers when necessary,
> please. You can usually just use SpecialPowers.wrap(obj).foo when obj.foo
> requires chrome privileges.

@Josh: I have tried to remove the netscape.security.PrivilegeManager.enablePrivilege('UniversalXPConnect'), and then added SpecialPowers.wrap to the following code which requires chrome privileges. 

+var mainWindow = SpecialPowers.wrap(window)
+                    .QueryInterface(Ci.nsIInterfaceRequestor)
+                    .getInterface(Ci.nsIWebNavigation)
+                    .QueryInterface(Ci.nsIDocShellTreeItem)
+                    .rootTreeItem
+                    .QueryInterface(Ci.nsIInterfaceRequestor)
+                    .getInterface(Ci.nsIDOMWindow);

However, I am getting an exception "TypeError: utils.getVisitedDependentComputedStyle" when running the test, and I have tried different ways to resolve this but without luck.

Any suggestions how I can fix that? Thanks

Comment 6

[Josh Matthews [:jdm]]

Actually, I suspect the problem can be solved by ignoring my SpecialPowers advice earlier and making this a mochitest-chrome test instead, so it should run with chrome privileges. It may just be a matter of using MOCHITEST_CHROME_FILES in the makefile instead.

Comment 7

[Raymond Lee [:raymondlee]]

(In reply to Josh Matthews [:jdm] from comment #6)
> Actually, I suspect the problem can be solved by ignoring my SpecialPowers
> advice earlier and making this a mochitest-chrome test instead, so it should
> run with chrome privileges. It may just be a matter of using
> MOCHITEST_CHROME_FILES in the makefile instead.

Unfortunately, it didn't work.  I got Error: Permission denied for window.QueryInterface(Ci.nsIInterfaceRequestor)...  Wrapping it with SpecialPowers.wrap() gave me error as before.

Comment 8

[Josh Matthews [:jdm]]

What's the command you're using to run the test?

Comment 9

[Raymond Lee [:raymondlee]]

(In reply to Josh Matthews [:jdm] from comment #8)
> What's the command you're using to run the test?

I use the following, is it the right thing to do?
TEST_PATH=toolkit/components/places/tests/ make -C obj-ff-dbg mochitest-plain

Comment 10

[Josh Matthews [:jdm]]

You need to use the mochitest-chrome target instead of mochitest-plain. You may also need to rebuild the test directory.

Comment 11

[Raymond Lee [:raymondlee]]

Attachment: v3

I have checked to use run this test as mochitest-chrome

Pushed to try and looking good so far
https://tbpl.mozilla.org/?tree=Try&rev=e0ec03e5a18e

Comment 12

[Josh Matthews [:jdm]]

It would be worth narrowing future try pushes to just execute the test harness that contains the test you're adding.

Comment 13

[Ed Morley [:emorley]]

Please can you also cancel previous try runs when you push another iteration, to save resources (particularly helps our most resource constrained platforms, since they are the most likely still not yet started).

https://groups.google.com/d/msg/mozilla.dev.platform/iengKcyD504/DPJeGxOpGw0J

Comment 14

[Raymond Lee [:raymondlee]]

(In reply to Ed Morley [UTC+0; email:edmorley@moco] from comment #13)
> Please can you also cancel previous try runs when you push another
> iteration, to save resources (particularly helps our most resource
> constrained platforms, since they are the most likely still not yet started).
> 
> https://groups.google.com/d/msg/mozilla.dev.platform/iengKcyD504/DPJeGxOpGw0J

Sure

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/87ad27a8a7de

Comment 16

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/87ad27a8a7de