Last Comment Bug 769435 - Convert test_openLocationLastURL.js from a xpcshell test to a mochitest
: Convert test_openLocationLastURL.js from a xpcshell test to a mochitest
Status: RESOLVED FIXED
[mentor=sawrubh]
:
Product: Firefox
Classification: Client Software
Component: General (show other bugs)
: unspecified
: x86_64 Linux
: -- enhancement (vote)
: Firefox 17
Assigned To: Andres Hernandez [:andreshm]
:
:
Mentors:
http://mxr.mozilla.org/mozilla-centra...
Depends on:
Blocks: 722988
  Show dependency treegraph
 
Reported: 2012-06-28 13:34 PDT by Saurabh Anand [:sawrubh]
Modified: 2012-07-20 06:47 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v1 (7.40 KB, patch)
2012-07-17 13:21 PDT, Andres Hernandez [:andreshm]
saurabhanandiit: feedback+
Details | Diff | Splinter Review
Patch v2 (8.70 KB, patch)
2012-07-18 09:53 PDT, Andres Hernandez [:andreshm]
ehsan: review+
Details | Diff | Splinter Review

Description Saurabh Anand [:sawrubh] 2012-06-28 13:34:31 PDT
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] 2012-06-28 13:37:09 PDT
CC'ing the people I know are involved.
Comment 2 Andres Hernandez [:andreshm] 2012-07-16 15:52:29 PDT
I can take a look at this one.
Comment 3 Saurabh Anand [:sawrubh] 2012-07-17 01:25:57 PDT
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 :Ehsan Akhgari 2012-07-17 08:04:22 PDT
(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] 2012-07-17 13:21:46 PDT
Created attachment 643111 [details] [diff] [review]
Patch v1
Comment 6 :Ehsan Akhgari 2012-07-17 19:10:32 PDT
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] 2012-07-17 22:13:53 PDT
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] 2012-07-17 22:21:20 PDT
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 :Ehsan Akhgari 2012-07-18 08:41:21 PDT
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] 2012-07-18 09:53:01 PDT
Created attachment 643423 [details] [diff] [review]
Patch v2

Applied changes. 
All tests are running fine!
Comment 11 Andres Hernandez [:andreshm] 2012-07-18 10:02:09 PDT
Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=16402f696e08
Comment 12 :Ehsan Akhgari 2012-07-18 10:04:12 PDT
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] 2012-07-19 08:07:08 PDT
Try finished but with some oranges. See: https://tbpl.mozilla.org/?tree=Try&rev=16402f696e08

Ehsan, please confirm is good to go.
Comment 14 :Ehsan Akhgari 2012-07-19 08:12:34 PDT
(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 :Ehsan Akhgari 2012-07-19 08:14:02 PDT
Thanks Andres for your patch!

https://hg.mozilla.org/integration/mozilla-inbound/rev/775c7786a753
Comment 16 Ed Morley [:emorley] 2012-07-20 06:47:08 PDT
https://hg.mozilla.org/mozilla-central/rev/775c7786a753

Note You need to log in before you can comment on or make changes to this bug.