769435 - Convert test_openLocationLastURL.js from a xpcshell test to a mochitest

Status: RESOLVED FIXED

(Firefox :: General, enhancement)

Description

[Saurabh Anand [:sawrubh]]

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.

Comment 1

[Saurabh Anand [:sawrubh]]

CC'ing the people I know are involved.

Comment 2

[Andres Hernandez [:andreshm]]

I can take a look at this one.

Comment 3

[Saurabh Anand [:sawrubh]]

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.

Comment 4

[(no longer active)]

(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>

Comment 5

[Andres Hernandez [:andreshm]]

Attachment: Patch v1

Comment 6

[(no longer active)]

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.

Comment 7

[Saurabh Anand [:sawrubh]]

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).

Comment 8

[Saurabh Anand [:sawrubh]]

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 9

[(no longer active)]

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!

Comment 10

[Andres Hernandez [:andreshm]]

Attachment: Patch v2

Applied changes. 
All tests are running fine!

Comment 11

[Andres Hernandez [:andreshm]]

Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=16402f696e08

Comment 12

[(no longer active)]

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!

Comment 13

[Andres Hernandez [:andreshm]]

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

Ehsan, please confirm is good to go.

Comment 14

[(no longer active)]

(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!  :-)

Comment 15

[(no longer active)]

Thanks Andres for your patch!

https://hg.mozilla.org/integration/mozilla-inbound/rev/775c7786a753

Comment 16

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/775c7786a753