Closed Bug 806741 Opened 7 years ago Closed 7 years ago

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

Categories

(Firefox :: Private Browsing, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 20

People

(Reporter: ehsan, Assigned: raymondlee)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

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.
Removed this test from per-window PB builds: https://hg.mozilla.org/mozilla-central/rev/d142cca052c3
Assignee: nobody → raymond
Status: NEW → ASSIGNED
Attached file v1 (obsolete) —
Attachment #693267 - Flags: review?(ehsan)
Attached patch v2 (obsolete) — Splinter Review
Forgot to add a file in last patch
Attachment #693267 - Attachment is obsolete: true
Attachment #693267 - Flags: review?(ehsan)
Attachment #693271 - Flags: review?(ehsan)
Attachment #693271 - Flags: review?(ehsan) → review?(josh)
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.
Attachment #693271 - Flags: review?(josh) → review+
(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
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.
(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.
What's the command you're using to run the test?
(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
You need to use the mochitest-chrome target instead of mochitest-plain. You may also need to rebuild the test directory.
Attached patch v3Splinter Review
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
Attachment #693271 - Attachment is obsolete: true
It would be worth narrowing future try pushes to just execute the test harness that contains the test you're adding.
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
(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
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/87ad27a8a7de
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.