1132566 - Make privatebrowsing tests run in e10s

Status: RESOLVED FIXED

(Firefox :: Private Browsing, defect)

Description

[Steven MacLeod [:smacleod]]

All tests under browser/components/privatebrowsing/test/browser/ are currently disabled in e10s. We need to fix them up to work in e10s so that privatebrowsing has coverage again.

Comment 1

[Marco Mucci [:MarcoM]]

Hi Steven, can you provide a point value.

Comment 2

[Steven MacLeod [:smacleod]]

Attachment: MozReview Request: bz://1132566/smacleod

/r/4815 - Bug 1132566 - Stop defaulting to skip privatebrowsing mochitest-browser tests.
/r/4817 - Bug 1132566 - Make test e10s compatible: browser_privatebrowsing_aboutHomeButtonAfterWindowClose.js
/r/4819 - Bug 1132566 - Make test e10s compatible: browser_privatebrowsing_aboutSessionRestore.js
/r/4821 - Bug 1132566 - Enable test in e10s: browser_privatebrowsing_DownloadLastDirWithCPS.js
/r/4823 - Bug 1132566 - Enable test in e10s: browser_privatebrowsing_cache.js
/r/4825 - Bug 1132566 - Enable test in e10s: browser_privatebrowsing_certexceptionsui.js
/r/4827 - Bug 1132566 - Make test e10s compatible: browser_privatebrowsing_concurrent.js
/r/4829 - Bug 1132566 - Cleanup browser_privatebrowsing_concurrent.js
/r/4831 - Bug 1132566 - Make test e10s compatible: browser_privatebrowsing_cookieacceptdialog.js
/r/4833 - Bug 1132566 - Make test e10s compatible: browser_privatebrowsing_crh.js
/r/4835 - Bug 1132566 - Enable test in e10s: browser_privatebrowsing_downloadLastDir.js
/r/4837 - Bug 1132566 - Enable test in e10s: browser_privatebrowsing_downloadLastDir_c.js
/r/4839 - Bug 1132566 - Enable test in e10s: browser_privatebrowsing_downloadLastDir_toggle.js
/r/4841 - Bug 1132566 - Make test e10s compatible: browser_privatebrowsing_geoprompt.js
/r/4843 - Bug 1132566 - Enable test in e10s: browser_privatebrowsing_lastpbcontextexited.js
/r/4845 - Bug 1132566 - Make test e10s compatible: browser_privatebrowsing_localStorage.js
/r/4847 - Bug 1132566 - Make test e10s compatible: browser_privatebrowsing_localStorage_before_after.js
/r/4849 - Bug 1132566 - Enable test in e10s: browser_privatebrowsing_noSessionRestoreMenuOption.js
/r/4851 - Bug 1132566 - Enable test in e10s: browser_privatebrowsing_nonbrowser.js
/r/4853 - Bug 1132566 - Enable test in e10s: browser_privatebrowsing_opendir.js
/r/4855 - Bug 1132566 - Make test e10s compatible: browser_privatebrowsing_placesTitleNoUpdate.js
/r/4857 - Bug 1132566 - Make test e10s compatible: browser_privatebrowsing_placestitle.js
/r/4859 - Bug 1132566 - Make test e10s compatible: browser_privatebrowsing_popupblocker.js
/r/4861 - Bug 1132566 - Make test e10s compatible: browser_privatebrowsing_protocolhandler.js
/r/4863 - Bug 1132566 - Enable test in e10s: browser_privatebrowsing_sidebar.js
/r/4865 - Bug 1132566 - Enable test in e10s: browser_privatebrowsing_theming.js
/r/4867 - Bug 1132566 - Enable test in e10s: browser_privatebrowsing_ui.js
/r/4869 - Bug 1132566 - Enable test in e10s: browser_privatebrowsing_urlbarfocus.js

Pull down these commits:

hg pull review -r 771533ad1ec60ec869def99206a65a56559d812c

Comment 3

[Steven MacLeod [:smacleod]]

https://reviewboard.mozilla.org/r/4813/#review3911

There are a lot of commits here so I tried to split up the reviewing load a bit between the four of you. Bill, I've given you all the commits which just enable a test that wasn't failing for me, as well as a few actual fixes (which is why you have the most commits to review).

Feel free to tell me to reassign commits etc if you think there is someone better.

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=68e20223a540

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

https://reviewboard.mozilla.org/r/4855/#review3927

::: browser/components/privatebrowsing/test/browser/browser_privatebrowsing_placesTitleNoUpdate.js
(Diff revision 1)
> -function test() {
> +add_task(function test() {

Nit: add_task(function* test() {

::: browser/components/privatebrowsing/test/browser/browser_privatebrowsing_placesTitleNoUpdate.js
(Diff revision 1)
> -  let windowsToClose = [];
> +  deferredAfterUpdateVisit.promise = new Promise((resolve, reject) => {

Should probably use PromiseUtils.defer().

::: browser/components/privatebrowsing/test/browser/browser_privatebrowsing_placesTitleNoUpdate.js
(Diff revision 1)
> -      aWin.gBrowser.selectedBrowser.removeEventListener("load", onLoad, true);
> +  BrowserTestUtils.closeWindow(privateWin);

I can't find where BrowserTestUtils.closeWindow() is defined but that should probably return a Promise, just like promiseWindowClosed(). It needs to call win.close() and wait for the domwindowclosed notification - like we do in the sessionstore test suite.

::: browser/components/privatebrowsing/test/browser/browser_privatebrowsing_placesTitleNoUpdate.js
(Diff revision 1)
> +          deferredAfterUpdateVisit.resolve();

Could we move the whole historyObserver code to a separate function maybe? That just uses the |new Promise(...)| API and returns a Promise?

Comment 5

[Tim Taubert [:ttaubert] (inactive)]

https://reviewboard.mozilla.org/r/4857/#review3925

::: browser/components/privatebrowsing/test/browser/browser_privatebrowsing_placestitle.js
(Diff revision 1)
> -function test() {
> +add_task(function test() {

Nit: add_task(function* test() {

::: browser/components/privatebrowsing/test/browser/browser_privatebrowsing_placestitle.js
(Diff revision 1)
> +  let PromiseUtils = Cu.import("resource://gre/modules/PromiseUtils.jsm", {}).PromiseUtils;

Nit: let {PromiseUtils} = Cu.import("resource://gre/modules/PromiseUtils.jsm", {});

OTOH, maybe just put that into head.js?

::: browser/components/privatebrowsing/test/browser/browser_privatebrowsing_placestitle.js
(Diff revision 1)
>    let cm = Cc["@mozilla.org/cookiemanager;1"].getService(Ci.nsICookieManager);

Use Services.cookies?

::: browser/components/privatebrowsing/test/browser/browser_privatebrowsing_placestitle.js
(Diff revision 1)
> -          is(aPageTitle, "No Cookie",
> +          deferredFirst.resolve(aPageTitle);

It seems like this whole test can be simplified if we would move the historyObserver code to a separate function instead of reusing the same observer for multiple steps.

We could just do something, yield waitForPageTitleChanged(), check that everything worked and then do the next step, yield again, etc.

::: browser/components/privatebrowsing/test/browser/browser_privatebrowsing_placestitle.js
(Diff revision 1)
> +  BrowserTestUtils.closeWindow(win2);

Same remark that this should be async.

::: browser/components/privatebrowsing/test/browser/browser_privatebrowsing_placestitle.js
(Diff revision 1)
> -  let selectedWin = null;
> +  function waitForCleanup() {

The name here is a little misleading, we're not really just waiting for the cleanup to happen. We're kicking it off.

Comment 6

[Steven MacLeod [:smacleod]]

https://reviewboard.mozilla.org/r/4855/#review3937

> I can't find where BrowserTestUtils.closeWindow() is defined but that should probably return a Promise, just like promiseWindowClosed(). It needs to call win.close() and wait for the domwindowclosed notification - like we do in the sessionstore test suite.

Ah yes I probably should have split `BrowserTestUtils.closeWindow()` out into its own commit to make that more obvious. Currently it is part of the first commit to use it, https://reviewboard.mozilla.org/r/4831/

It does return a promise, waiting for domwindowclosed.

Comment 7

[Steven MacLeod [:smacleod]]

https://reviewboard.mozilla.org/r/4857/#review3939

> It seems like this whole test can be simplified if we would move the historyObserver code to a separate function instead of reusing the same observer for multiple steps.
> 
> We could just do something, yield waitForPageTitleChanged(), check that everything worked and then do the next step, yield again, etc.

How do you suggest dealing with the `default` case that makes sure it doesn't fire for the private window? I'd rather avoid having two listeners, same with timeouts.

Comment 8

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Steven MacLeod [:smacleod] from comment #7)
> How do you suggest dealing with the `default` case that makes sure it
> doesn't fire for the private window? I'd rather avoid having two listeners,
> same with timeouts.

Hmm :/ Overlooked to case where we check that it doesn't fire. Seems fine to keep it as is then I guess. Didn't see those as r+ requirements, was thinking out loud so please do what you think is reasonable :)

Comment 9

[Mike Conley (:mconley) (:⚙️)]

https://reviewboard.mozilla.org/r/4817/#review3941

::: browser/components/privatebrowsing/test/browser/browser_privatebrowsing_aboutHomeButtonAfterWindowClose.js
(Diff revision 1)
> +add_task(function test_no_sessionrestore_button() {

function*

::: browser/components/privatebrowsing/test/browser/browser_privatebrowsing_aboutHomeButtonAfterWindowClose.js
(Diff revision 1)
>  // This test checks that the Session Restore about:home button
>  // is disabled in private mode

Took me a minute or two to figure out what the "Session Restore about:home" button was. Maybe let's update this comment to refer to the "Restore Previous Session" button.

::: browser/components/privatebrowsing/test/browser/browser_privatebrowsing_aboutHomeButtonAfterWindowClose.js
(Diff revision 1)
> +  (yield BrowserTestUtils.openNewBrowserWindow({private: true})).close();

Can you quickly explain why we're opening and closing this first window? (I assume it's to theoretically create a session?)

Comment 10

[Mike Conley (:mconley) (:⚙️)]

https://reviewboard.mozilla.org/r/4819/#review3943

This is so much more readable. This is the best.

::: browser/components/privatebrowsing/test/browser/browser_privatebrowsing_aboutSessionRestore.js
(Diff revision 1)
> +add_task(function testNoSessionRestoreButton() {

function*

::: browser/components/privatebrowsing/test/browser/browser_privatebrowsing_aboutSessionRestore.js
(Diff revision 1)
> +  (yield BrowserTestUtils.openNewBrowserWindow({private: true})).close();

Quickly mention why we're opening and closing this window - much like the last test, I imagine.

Comment 11

[Mike Conley (:mconley) (:⚙️)]

https://reviewboard.mozilla.org/r/4841/#review3947

::: browser/components/privatebrowsing/test/browser/browser_privatebrowsing_geoprompt.js
(Diff revision 1)
> -        function runTest() {
> +      // Wait until the notification is available.
> +      while (!notification){
> +        yield new Promise(resolve => { executeSoon(resolve); });
> -          let notification = aWindow.PopupNotifications.getNotification("geolocation");
> +        let notification = aWindow.PopupNotifications.getNotification("geolocation");
> -          if (!notification) {
> -            // Wait until the notification is available
> -            executeSoon(runTest);
> -            return;
> -          }
> +      }

This sort of thing - waiting for an eventual popup - is something I see often, and can/should probably be put into BrowserTestUtils.

I normally see it solved with something like "waitForCondition", and it polls via setInterval for a set number of times before timing out.

I suggest writing something like that for BrowserTestUtils that we can use like this:

```
yield BrowserTestUtils.waitForCondition(() => {
  let notification = aWindow.PopupNotifications.getNotification("geolocation");
  return !!notification;
});
```

And have it have some default timeout of like... 5 seconds or something. Maybe that could be overridable with an optional arg to waitForCondition.

I'm not saying you have to do that here and now, but maybe let's get a bug on file to get it added to BrowserTestUtils?

::: browser/components/privatebrowsing/test/browser/browser_privatebrowsing_geoprompt.js
(Diff revision 1)
> -  testOnWindow({private: false}, function(win) {
> +

Nit - remove extra newline.

Comment 12

[Mike Conley (:mconley) (:⚙️)]

https://reviewboard.mozilla.org/r/4841/#review3951

::: browser/components/privatebrowsing/test/browser/browser_privatebrowsing_geoprompt.js
(Diff revision 1)
> -function test() {
> +add_task(function test() {

function*

Comment 13

[Mike Conley (:mconley) (:⚙️)]

https://reviewboard.mozilla.org/r/4859/#review3955

::: browser/components/privatebrowsing/test/browser/browser_privatebrowsing_popupblocker.js
(Diff revision 1)
> -function test() {
> +add_task(function test() {

function*

::: browser/components/privatebrowsing/test/browser/browser_privatebrowsing_popupblocker.js
(Diff revision 1)
> -          testOnWindow({}, function(win) {
> +  gPrefService.setBoolPref("dom.disable_open_during_load", oldPopupPolicy);

Maybe move this into a registerCleanupFunction so that we don't contaminate other tests if we throw or something before we get here.

Comment 14

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8573590 [details]
MozReview Request: bz://1132566/smacleod

r=me for the commits I reviewed with my nits fixed.

Comment 15

[Dave Townsend [:mossop]]

https://reviewboard.mozilla.org/r/4827/#review4001

::: browser/components/privatebrowsing/test/browser/browser_privatebrowsing_concurrent.js
(Diff revision 1)
> +  });

You should be able to use private_tab.contentTitle here and thoughout the rest of the test.

Comment 16

[Dave Townsend [:mossop]]

https://reviewboard.mozilla.org/r/4829/#review4003

::: browser/components/privatebrowsing/test/browser/browser_privatebrowsing_concurrent.js
(Diff revision 1)
> -  });
> +    });

Again I'd expect browser.contentTitle to work here.

Comment 17

[Dave Townsend [:mossop]]

https://reviewboard.mozilla.org/r/4829/#review4005

Comment 18

[Dave Townsend [:mossop]]

https://reviewboard.mozilla.org/r/4833/#review4007

::: browser/components/privatebrowsing/test/browser/browser_privatebrowsing_crh.js
(Diff revision 1)
> +  let testURI = "http://mochi.test:8888/";

Doesn't seem like these needed to move. Would keep the blame cleaner.

Comment 19

[Dave Townsend [:mossop]]

https://reviewboard.mozilla.org/r/4833/#review4009

::: browser/components/privatebrowsing/test/browser/browser_privatebrowsing_crh.js
(Diff revision 1)
> -  waitForExplicitFinish();
> +  let windowsToClose = [];

This variable is never used now.

Comment 20

[Dave Townsend [:mossop]]

https://reviewboard.mozilla.org/r/4861/#review4011

::: browser/components/privatebrowsing/test/browser/browser_privatebrowsing_protocolhandler.js
(Diff revision 1)
>    function doTest(aIsPrivateMode, aWindow, aCallback) {

Use Task.async here and you can get rid of the callback and generally make this saner.

Comment 21

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

https://reviewboard.mozilla.org/r/4847/#review4061

::: browser/components/privatebrowsing/test/browser/browser_privatebrowsing_localStorage_before_after.js
(Diff revision 1)
> -    testURI = prefix + 'browser_privatebrowsing_localStorage_before_after_page.html';
> -    doTest(true, aWin, function() {
> -      // then test when not on private mode
> +  privateBrowser.loadURI(
> +    prefix + 'browser_privatebrowsing_localStorage_before_after_page.html');
> +  yield BrowserTestUtils.browserLoaded(privateBrowser);

There's a subtle problem here that we have with all our e10s tests: loadURI is sent to the child immediately and the page may already have loaded by the time you call browserLoader. That can't happen in non-e10s.

As long as you're changing these tests, it would be nice to fix this. Also be aware that you can't just switch the order of loadURI and browserLoaded since then browserLoaded might just fire for an about:blank load.

Maybe we could have a function that loads a URI and waits for it to finish loading? That would be nice.

Comment 22

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

https://reviewboard.mozilla.org/r/4845/#review4057

::: browser/components/privatebrowsing/test/browser/browser_privatebrowsing_localStorage.js
(Diff revision 1)
> -  requestLongerTimeout(2);

Why take this line out? I suspect we need it.

::: browser/components/privatebrowsing/test/browser/browser_privatebrowsing_localStorage.js
(Diff revision 1)
> -  testOnWindow({private: true}, function(win) {
> +  let tab2 = win.gBrowser.selectedTab = win.gBrowser.addTab();

This test doesn't make a lot of sense to me. Why does it open tab2 and never use it? It might be a good idea to ask Ehsan about this.

Comment 23

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

https://reviewboard.mozilla.org/r/4831/#review4055

::: browser/components/privatebrowsing/test/browser/browser_privatebrowsing_cookieacceptdialog.js
(Diff revision 1)
> +        Services.ww.unregisterNotification(observer);

I don't think you want this. It will unregister the observer twice.

::: browser/components/privatebrowsing/test/browser/browser_privatebrowsing_cookieacceptdialog.js
(Diff revision 1)
> -  function checkRememberOption(expectedDisabled, aWindow, callback) {
> +  function domWindowOpened() {

Could this go in BrowserTestUtils as well?

Comment 24

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

https://reviewboard.mozilla.org/r/4831/#review4063

Ship It!

Comment 25

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8573590 [details]
MozReview Request: bz://1132566/smacleod

https://reviewboard.mozilla.org/r/4813/#review4065

Ship It!

Comment 26

[Steven MacLeod [:smacleod]]

https://reviewboard.mozilla.org/r/4845/#review4155

> This test doesn't make a lot of sense to me. Why does it open tab2 and never use it? It might be a good idea to ask Ehsan about this.

Talked to jdm about this, he suspected it could have just been copied from another test. I'll nuke it.

Comment 27

[Steven MacLeod [:smacleod]]

https://reviewboard.mozilla.org/r/4847/#review4167

> There's a subtle problem here that we have with all our e10s tests: loadURI is sent to the child immediately and the page may already have loaded by the time you call browserLoader. That can't happen in non-e10s.
> 
> As long as you're changing these tests, it would be nice to fix this. Also be aware that you can't just switch the order of loadURI and browserLoaded since then browserLoaded might just fire for an about:blank load.
> 
> Maybe we could have a function that loads a URI and waits for it to finish loading? That would be nice.

I've switched this test to add new tabs with the pages loaded there, which I think should avoid the problem of the initial page loading blank.

Since browserLoaded waits for a message coming from the child process (which listens to the load in the frame script and then sends an async message) I don't see how the load could possibly beat my call to browserLoaded - the test js should be synchronous until it adds its message listener. Am I missing something?

Comment 28

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

(In reply to Steven MacLeod [:smacleod] from comment #27)
> the
> test js should be synchronous until it adds its message listener.

This is the part that's incorrect. loadURI immediately sends a message to the child process asking it to do a load. The child process runs independently of the parent, so it can finish the load and send the message manager message before the parent has a chance to do addMessageListener.

Comment 29

[Dave Townsend [:mossop]]

(In reply to Bill McCloskey (:billm) from comment #28)
> (In reply to Steven MacLeod [:smacleod] from comment #27)
> > the
> > test js should be synchronous until it adds its message listener.
> 
> This is the part that's incorrect. loadURI immediately sends a message to
> the child process asking it to do a load. The child process runs
> independently of the parent, so it can finish the load and send the message
> manager message before the parent has a chance to do addMessageListener.

It can send the message, but surely it can't arrive until JS drops back to the event loop, or are IPC messages processed and delivered in a different thread?

Comment 30

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

(In reply to Dave Townsend [:mossop] from comment #29)
> It can send the message, but surely it can't arrive until JS drops back to
> the event loop, or are IPC messages processed and delivered in a different
> thread?

Well, the message arrives in the child, which probably isn't running any JS at all at this time. The test is running in the parent.

Comment 31

[Dave Townsend [:mossop]]

(In reply to Bill McCloskey (:billm) from comment #30)
> (In reply to Dave Townsend [:mossop] from comment #29)
> > It can send the message, but surely it can't arrive until JS drops back to
> > the event loop, or are IPC messages processed and delivered in a different
> > thread?
> 
> Well, the message arrives in the child, which probably isn't running any JS
> at all at this time. The test is running in the parent.

I'm talking about the message from the child to the parent to tell it the load has completed, the one dispatched here: http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/BrowserTestUtils/content/content-utils.js#13

Comment 32

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Oh, I see what you mean. I guess that's true. Never mind.

Comment 33

[Steven MacLeod [:smacleod]]

Comment on attachment 8573590 [details]
MozReview Request: bz://1132566/smacleod

/r/4815 - Bug 1132566 - Stop defaulting to skip privatebrowsing mochitest-browser tests; r=billm
/r/4817 - Bug 1132566 - Make test e10s compatible: browser_privatebrowsing_aboutHomeButtonAfterWindowClose.js; r=mconley
/r/4819 - Bug 1132566 - Make test e10s compatible: browser_privatebrowsing_aboutSessionRestore.js; r=mconley
/r/4821 - Bug 1132566 - Enable test in e10s: browser_privatebrowsing_DownloadLastDirWithCPS.js; r=billm
/r/4823 - Bug 1132566 - Enable test in e10s: browser_privatebrowsing_cache.js; r=billm
/r/4825 - Bug 1132566 - Enable test in e10s: browser_privatebrowsing_certexceptionsui.js; r=billm
/r/4827 - Bug 1132566 - Make test e10s compatible: browser_privatebrowsing_concurrent.js; r=mossop
/r/4829 - Bug 1132566 - Cleanup browser_privatebrowsing_concurrent.js; r=mossop
/r/4831 - Bug 1132566 - Make test e10s compatible: browser_privatebrowsing_cookieacceptdialog.js; r=billm
/r/4833 - Bug 1132566 - Make test e10s compatible: browser_privatebrowsing_crh.js; r=mossop
/r/4835 - Bug 1132566 - Enable test in e10s: browser_privatebrowsing_downloadLastDir.js; r=billm
/r/4837 - Bug 1132566 - Enable test in e10s: browser_privatebrowsing_downloadLastDir_c.js; r=billm
/r/4839 - Bug 1132566 - Enable test in e10s: browser_privatebrowsing_downloadLastDir_toggle.js; r=billm
/r/4841 - Bug 1132566 - Make test e10s compatible: browser_privatebrowsing_geoprompt.js; r=mconley
/r/4843 - Bug 1132566 - Enable test in e10s: browser_privatebrowsing_lastpbcontextexited.js; r=billm
/r/4845 - Bug 1132566 - Make test e10s compatible: browser_privatebrowsing_localStorage.js; r=billm
/r/4847 - Bug 1132566 - Make test e10s compatible: browser_privatebrowsing_localStorage_before_after.js; r=billm
/r/4849 - Bug 1132566 - Enable test in e10s: browser_privatebrowsing_noSessionRestoreMenuOption.js; r=billm
/r/4851 - Bug 1132566 - Enable test in e10s: browser_privatebrowsing_nonbrowser.js; r=billm
/r/4853 - Bug 1132566 - Enable test in e10s: browser_privatebrowsing_opendir.js; r=billm
/r/4855 - Bug 1132566 - Make test e10s compatible: browser_privatebrowsing_placesTitleNoUpdate.js; r=ttaubert
/r/4857 - Bug 1132566 - Make test e10s compatible: browser_privatebrowsing_placestitle.js; r=ttaubert
/r/4859 - Bug 1132566 - Make test e10s compatible: browser_privatebrowsing_popupblocker.js; r=mconley
/r/4861 - Bug 1132566 - Make test e10s compatible: browser_privatebrowsing_protocolhandler.js; r=mossop
/r/4863 - Bug 1132566 - Enable test in e10s: browser_privatebrowsing_sidebar.js; r=billm
/r/4865 - Bug 1132566 - Enable test in e10s: browser_privatebrowsing_theming.js; r=billm
/r/4867 - Bug 1132566 - Enable test in e10s: browser_privatebrowsing_ui.js; r=billm
/r/4869 - Bug 1132566 - Enable test in e10s: browser_privatebrowsing_urlbarfocus.js; r=billm

Pull down these commits:

hg pull review -r 7441009605d99b05dd701c01066c84c68b5f4cf2

Comment 34

[Tim Taubert [:ttaubert] (inactive)]

https://reviewboard.mozilla.org/r/4857/#review4207

Ship It!

Comment 35

[Tim Taubert [:ttaubert] (inactive)]

https://reviewboard.mozilla.org/r/4855/#review4209

::: browser/components/privatebrowsing/test/browser/browser_privatebrowsing_placesTitleNoUpdate.js
(Diff revision 2)
>  // Test to make sure that the visited page titles do not get updated inside the

Nit: "use strict"; please

::: browser/components/privatebrowsing/test/browser/browser_privatebrowsing_placesTitleNoUpdate.js
(Diff revision 2)
> -      handleError: function () do_throw("Unexpected error in adding visit."),
> +    handleError: function () do_throw("Unexpected error in adding visit."),

That do_throw() looks copied from an xpcshell test. Maybe ok(false, x)?

Comment 36

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8573590 [details]
MozReview Request: bz://1132566/smacleod

https://reviewboard.mozilla.org/r/4813/#review4211

Ship It!

Comment 37

[Steven MacLeod [:smacleod]]

https://hg.mozilla.org/integration/fx-team/pushloghtml?changeset=02ea96f256cf

https://hg.mozilla.org/integration/fx-team/rev/bb14b5ecea67
https://hg.mozilla.org/integration/fx-team/rev/8c349714246d
https://hg.mozilla.org/integration/fx-team/rev/b6d448535004
https://hg.mozilla.org/integration/fx-team/rev/f92f9ce02337
https://hg.mozilla.org/integration/fx-team/rev/6f72ff7afafa
https://hg.mozilla.org/integration/fx-team/rev/1a47b7ee9377
https://hg.mozilla.org/integration/fx-team/rev/c84bc3a8edf0
https://hg.mozilla.org/integration/fx-team/rev/034add5853cd
https://hg.mozilla.org/integration/fx-team/rev/3dfae5c35fab
https://hg.mozilla.org/integration/fx-team/rev/7713401cf0c4
https://hg.mozilla.org/integration/fx-team/rev/7a8c47a8aae6
https://hg.mozilla.org/integration/fx-team/rev/9b5ad01e8eef
https://hg.mozilla.org/integration/fx-team/rev/6e8d12dd69f2
https://hg.mozilla.org/integration/fx-team/rev/bc7ca394c5cf
https://hg.mozilla.org/integration/fx-team/rev/bd4ad6d4d52a
https://hg.mozilla.org/integration/fx-team/rev/36478f270e88
https://hg.mozilla.org/integration/fx-team/rev/f20eb0510848
https://hg.mozilla.org/integration/fx-team/rev/c55d5c59edb2
https://hg.mozilla.org/integration/fx-team/rev/24e396499557
https://hg.mozilla.org/integration/fx-team/rev/64e1b463e5b2
https://hg.mozilla.org/integration/fx-team/rev/85840d8c3242
https://hg.mozilla.org/integration/fx-team/rev/9b7f81054f06
https://hg.mozilla.org/integration/fx-team/rev/dd371585e028
https://hg.mozilla.org/integration/fx-team/rev/9da87b37243e
https://hg.mozilla.org/integration/fx-team/rev/64b04ba6e424
https://hg.mozilla.org/integration/fx-team/rev/7326f9105f4b
https://hg.mozilla.org/integration/fx-team/rev/f31357fcb5e9
https://hg.mozilla.org/integration/fx-team/rev/02ea96f256cf

Comment 38

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/bb14b5ecea67
https://hg.mozilla.org/mozilla-central/rev/8c349714246d
https://hg.mozilla.org/mozilla-central/rev/b6d448535004
https://hg.mozilla.org/mozilla-central/rev/f92f9ce02337
https://hg.mozilla.org/mozilla-central/rev/6f72ff7afafa
https://hg.mozilla.org/mozilla-central/rev/1a47b7ee9377
https://hg.mozilla.org/mozilla-central/rev/c84bc3a8edf0
https://hg.mozilla.org/mozilla-central/rev/034add5853cd
https://hg.mozilla.org/mozilla-central/rev/3dfae5c35fab
https://hg.mozilla.org/mozilla-central/rev/7713401cf0c4
https://hg.mozilla.org/mozilla-central/rev/7a8c47a8aae6
https://hg.mozilla.org/mozilla-central/rev/9b5ad01e8eef
https://hg.mozilla.org/mozilla-central/rev/6e8d12dd69f2
https://hg.mozilla.org/mozilla-central/rev/bc7ca394c5cf
https://hg.mozilla.org/mozilla-central/rev/bd4ad6d4d52a
https://hg.mozilla.org/mozilla-central/rev/36478f270e88
https://hg.mozilla.org/mozilla-central/rev/f20eb0510848
https://hg.mozilla.org/mozilla-central/rev/c55d5c59edb2
https://hg.mozilla.org/mozilla-central/rev/24e396499557
https://hg.mozilla.org/mozilla-central/rev/64e1b463e5b2
https://hg.mozilla.org/mozilla-central/rev/85840d8c3242
https://hg.mozilla.org/mozilla-central/rev/9b7f81054f06
https://hg.mozilla.org/mozilla-central/rev/dd371585e028
https://hg.mozilla.org/mozilla-central/rev/9da87b37243e
https://hg.mozilla.org/mozilla-central/rev/64b04ba6e424
https://hg.mozilla.org/mozilla-central/rev/7326f9105f4b
https://hg.mozilla.org/mozilla-central/rev/f31357fcb5e9
https://hg.mozilla.org/mozilla-central/rev/02ea96f256cf

Comment 40

[Steven MacLeod [:smacleod]]

Attachment: MozReview Request: Bug 1132566 - Enable test in e10s: browser_privatebrowsing_theming.js; r=billm

Comment 41

[Steven MacLeod [:smacleod]]

Attachment: MozReview Request: Bug 1132566 - Enable test in e10s: browser_privatebrowsing_ui.js; r=billm

Comment 42

[Steven MacLeod [:smacleod]]

Attachment: MozReview Request: Bug 1132566 - Enable test in e10s: browser_privatebrowsing_urlbarfocus.js; r=billm

Comment 43

[Steven MacLeod [:smacleod]]

Attachment: MozReview Request: Bug 1132566 - Stop defaulting to skip privatebrowsing mochitest-browser tests; r=billm

Comment 44

[Steven MacLeod [:smacleod]]

Attachment: MozReview Request: Bug 1132566 - Make test e10s compatible: browser_privatebrowsing_aboutHomeButtonAfterWindowClose.js; r=mconley

Comment 45

[Steven MacLeod [:smacleod]]

Attachment: MozReview Request: Bug 1132566 - Make test e10s compatible: browser_privatebrowsing_aboutSessionRestore.js; r=mconley

Comment 46

[Steven MacLeod [:smacleod]]

Attachment: MozReview Request: Bug 1132566 - Enable test in e10s: browser_privatebrowsing_DownloadLastDirWithCPS.js; r=billm

Comment 47

[Steven MacLeod [:smacleod]]

Attachment: MozReview Request: Bug 1132566 - Enable test in e10s: browser_privatebrowsing_cache.js; r=billm

Comment 48

[Steven MacLeod [:smacleod]]

Attachment: MozReview Request: Bug 1132566 - Enable test in e10s: browser_privatebrowsing_certexceptionsui.js; r=billm

Comment 49

[Steven MacLeod [:smacleod]]

Attachment: MozReview Request: Bug 1132566 - Make test e10s compatible: browser_privatebrowsing_concurrent.js; r=mossop

Comment 50

[Steven MacLeod [:smacleod]]

Attachment: MozReview Request: Bug 1132566 - Cleanup browser_privatebrowsing_concurrent.js; r=mossop

Comment 51

[Steven MacLeod [:smacleod]]

Attachment: MozReview Request: Bug 1132566 - Make test e10s compatible: browser_privatebrowsing_cookieacceptdialog.js; r=billm

Comment 52

[Steven MacLeod [:smacleod]]

Attachment: MozReview Request: Bug 1132566 - Make test e10s compatible: browser_privatebrowsing_crh.js; r=mossop

Comment 53

[Steven MacLeod [:smacleod]]

Attachment: MozReview Request: Bug 1132566 - Enable test in e10s: browser_privatebrowsing_downloadLastDir.js; r=billm

Comment 54

[Steven MacLeod [:smacleod]]

Attachment: MozReview Request: Bug 1132566 - Enable test in e10s: browser_privatebrowsing_downloadLastDir_c.js; r=billm

Comment 55

[Steven MacLeod [:smacleod]]

Attachment: MozReview Request: Bug 1132566 - Enable test in e10s: browser_privatebrowsing_downloadLastDir_toggle.js; r=billm

Comment 56

[Steven MacLeod [:smacleod]]

Attachment: MozReview Request: Bug 1132566 - Make test e10s compatible: browser_privatebrowsing_geoprompt.js; r=mconley

Comment 57

[Steven MacLeod [:smacleod]]

Attachment: MozReview Request: Bug 1132566 - Enable test in e10s: browser_privatebrowsing_lastpbcontextexited.js; r=billm

Comment 58

[Steven MacLeod [:smacleod]]

Attachment: MozReview Request: Bug 1132566 - Make test e10s compatible: browser_privatebrowsing_localStorage.js; r=billm

Comment 59

[Steven MacLeod [:smacleod]]

Attachment: MozReview Request: Bug 1132566 - Make test e10s compatible: browser_privatebrowsing_localStorage_before_after.js; r=billm

Comment 60

[Steven MacLeod [:smacleod]]

Attachment: MozReview Request: Bug 1132566 - Enable test in e10s: browser_privatebrowsing_noSessionRestoreMenuOption.js; r=billm

Comment 61

[Steven MacLeod [:smacleod]]

Attachment: MozReview Request: Bug 1132566 - Enable test in e10s: browser_privatebrowsing_nonbrowser.js; r=billm

Comment 62

[Steven MacLeod [:smacleod]]

Attachment: MozReview Request: Bug 1132566 - Enable test in e10s: browser_privatebrowsing_opendir.js; r=billm

Comment 63

[Steven MacLeod [:smacleod]]

Attachment: MozReview Request: Bug 1132566 - Make test e10s compatible: browser_privatebrowsing_placesTitleNoUpdate.js; r=ttaubert

Comment 64

[Steven MacLeod [:smacleod]]

Attachment: MozReview Request: Bug 1132566 - Make test e10s compatible: browser_privatebrowsing_placestitle.js; r=ttaubert

Comment 65

[Steven MacLeod [:smacleod]]

Attachment: MozReview Request: Bug 1132566 - Make test e10s compatible: browser_privatebrowsing_popupblocker.js; r=mconley

Comment 66

[Steven MacLeod [:smacleod]]

Attachment: MozReview Request: Bug 1132566 - Make test e10s compatible: browser_privatebrowsing_protocolhandler.js; r=mossop

Comment 67

[Steven MacLeod [:smacleod]]

Attachment: MozReview Request: Bug 1132566 - Enable test in e10s: browser_privatebrowsing_sidebar.js; r=billm