The default bug view has changed. See this FAQ.

Convert test_openLocationLastURL.js from a xpcshell test to a mochitest

RESOLVED FIXED in Firefox 17

Status

()

Firefox
General
--
enhancement
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: sawrubh, Assigned: andreshm)

Tracking

unspecified
Firefox 17
x86_64
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [mentor=sawrubh], URL)

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

5 years ago
In the new world, we need to acccess PB flag from each window and hence it makes sense to test in a mochitest rather than a (windowless) xpcshell test. This would help avoid the "window" object fakery, which needs to be done otherwise.
(Reporter)

Updated

5 years ago
(Reporter)

Comment 1

5 years ago
CC'ing the people I know are involved.
(Reporter)

Updated

5 years ago
Severity: normal → enhancement
(Reporter)

Updated

5 years ago
Whiteboard: [mentor=sawrubh]
(Assignee)

Comment 2

5 years ago
I can take a look at this one.
Assignee: nobody → andres
(Reporter)

Comment 3

5 years ago
Great ! Andres, so this will be a straight forward synchronous browser-chrome test. You can just replace the |do_check_eq| calls with calls to |is| (Take a look at <https://developer.mozilla.org/en/Mochitest>), remove the |switchPrivateBrowsing| functions with appropriate calls to |pb.privateBrowsingEnabled = true/false|. Res t should be simple.

But still I would like to borrow the eyes of smarter people (Ehsan,Josh) to verify if what I said is correct or just junk. Anything else you guys would like to add.

Thanks.
(In reply to comment #3)
> Great ! Andres, so this will be a straight forward synchronous browser-chrome
> test. You can just replace the |do_check_eq| calls with calls to |is| (Take a
> look at <https://developer.mozilla.org/en/Mochitest>), remove the
> |switchPrivateBrowsing| functions with appropriate calls to
> |pb.privateBrowsingEnabled = true/false|. Res t should be simple.

Yeah I believe this is mostly it.  Also, make sure to take out the fake window object which is declared here: <http://mxr.mozilla.org/mozilla-central/source/browser/components/privatebrowsing/test/unit/test_openLocationLastURL.js#11>
(Assignee)

Comment 5

5 years ago
Created attachment 643111 [details] [diff] [review]
Patch v1
Attachment #643111 - Flags: review?(saurabhanandiit)
Comment on attachment 643111 [details] [diff] [review]
Patch v1

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

::: browser/components/privatebrowsing/test/browser/browser_privatebrowsing_openLocationLastURL.js
@@ +2,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +function test() {
> +  waitForExplicitFinish();

This is only needed for asynchronous tests.  You can drop this and the finish() call at the end of this function.

@@ +7,5 @@
> +
> +  const URL_1 = "mozilla.org";
> +  const URL_2 = "mozilla.com";
> +
> +  let gOpenLocationLastURL = getLocationModule();

Nit: Please drop the g prefix (which stands for "global" and just call this openLocationLastURL.

@@ +10,5 @@
> +
> +  let gOpenLocationLastURL = getLocationModule();
> +  let observerService =
> +    Cc["@mozilla.org/observer-service;1"].
> +      getService(Ci.nsIObserverService);

Nit: you can get rid of this object and use Services.obs instead.

@@ +20,5 @@
> +    observerService.notifyObservers(null, "browser:purge-session-history", "");
> +  }
> +  function switchPrivateBrowsing(aFlag) {
> +    privateBrowsingService.privateBrowsingEnabled = aFlag;
> +    window.gPrivateBrowsingUI.privateWindow = aFlag;

Remove the second line.  The first line should be enough to take care of things here.
(Reporter)

Comment 7

5 years ago
Comment on attachment 643111 [details] [diff] [review]
Patch v1

I agree with Ehsan's comment.

Nit : Can you please arrange the name of the test in alphabetical order inside the makefile, most probably here <http://mxr.mozilla.org/mozilla-central/source/browser/components/privatebrowsing/test/browser/Makefile.in#36> .

Also, I'm not qualified to handle reviews. Handing it over to Ehsan (the author of the test).
Attachment #643111 - Flags: review?(saurabhanandiit)
Attachment #643111 - Flags: review?(ehsan)
Attachment #643111 - Flags: feedback+
(Reporter)

Comment 8

5 years ago
Andres, please also make sure that the entire test suite of "browser/components/privatebrowsing/test/browser" passes. You could do something like this :

TEST_PATH=browser/components/privatebrowsing/test/browser/ make -C objdir mochitest-browser-chrome

Also a try run would be good.

Thanks again for working on this.
Comment on attachment 643111 [details] [diff] [review]
Patch v1

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

Please address the comments that I made on the patch earlier.  Thanks!

::: browser/components/privatebrowsing/test/browser/Makefile.in
@@ +53,5 @@
>  		browser_privatebrowsing_zoom.js \
>  		browser_privatebrowsing_zoomrestore.js \
>  		browser_privatebrowsing_clearplugindata.js \
>  		browser_privatebrowsing_clearplugindata.html \
> +		browser_privatebrowsing_openLocationLastURL.js \

What Saurabh said!
Attachment #643111 - Flags: review?(ehsan)
(Assignee)

Comment 10

5 years ago
Created attachment 643423 [details] [diff] [review]
Patch v2

Applied changes. 
All tests are running fine!
Attachment #643111 - Attachment is obsolete: true
Attachment #643423 - Flags: review?(ehsan)
(Assignee)

Comment 11

5 years ago
Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=16402f696e08
Comment on attachment 643423 [details] [diff] [review]
Patch v2

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

Looks great!  Please mark as checkin-needed when the try results are in.  Thanks!
Attachment #643423 - Flags: review?(ehsan) → review+
(Assignee)

Comment 13

5 years ago
Try finished but with some oranges. See: https://tbpl.mozilla.org/?tree=Try&rev=16402f696e08

Ehsan, please confirm is good to go.
(In reply to Andres Hernandez [:andreshm] from comment #13)
> Try finished but with some oranges. See:
> https://tbpl.mozilla.org/?tree=Try&rev=16402f696e08
> 
> Ehsan, please confirm is good to go.

I can't see the try results because of bug 770811.  I guess I'll just push this to inbound and assume the best!  :-)
Thanks Andres for your patch!

https://hg.mozilla.org/integration/mozilla-inbound/rev/775c7786a753
Target Milestone: --- → Firefox 17
https://hg.mozilla.org/mozilla-central/rev/775c7786a753
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.