Closed Bug 1132566 Opened 5 years ago Closed 5 years ago

Make privatebrowsing tests run in e10s

Categories

(Firefox :: Private Browsing, defect)

defect
Not set
Points:
13

Tracking

()

RESOLVED FIXED
Firefox 39
Iteration:
39.2 - 23 Mar
Tracking Status
e10s + ---
firefox39 --- fixed

People

(Reporter: smacleod, Assigned: smacleod)

References

(Blocks 2 open bugs)

Details

Attachments

(28 files, 1 obsolete file)

39 bytes, text/x-review-board-request
ttaubert
: review+
smacleod
: review+
Details
39 bytes, text/x-review-board-request
ttaubert
: review+
smacleod
: review+
Details
39 bytes, text/x-review-board-request
ttaubert
: review+
smacleod
: review+
Details
39 bytes, text/x-review-board-request
ttaubert
: review+
smacleod
: review+
Details
39 bytes, text/x-review-board-request
ttaubert
: review+
smacleod
: review+
Details
39 bytes, text/x-review-board-request
ttaubert
: review+
smacleod
: review+
Details
39 bytes, text/x-review-board-request
ttaubert
: review+
smacleod
: review+
Details
39 bytes, text/x-review-board-request
ttaubert
: review+
smacleod
: review+
Details
39 bytes, text/x-review-board-request
ttaubert
: review+
smacleod
: review+
Details
39 bytes, text/x-review-board-request
ttaubert
: review+
smacleod
: review+
Details
39 bytes, text/x-review-board-request
ttaubert
: review+
smacleod
: review+
Details
39 bytes, text/x-review-board-request
ttaubert
: review+
smacleod
: review+
Details
39 bytes, text/x-review-board-request
ttaubert
: review+
smacleod
: review+
Details
39 bytes, text/x-review-board-request
ttaubert
: review+
smacleod
: review+
Details
39 bytes, text/x-review-board-request
ttaubert
: review+
smacleod
: review+
Details
39 bytes, text/x-review-board-request
ttaubert
: review+
smacleod
: review+
Details
39 bytes, text/x-review-board-request
ttaubert
: review+
smacleod
: review+
Details
39 bytes, text/x-review-board-request
ttaubert
: review+
smacleod
: review+
Details
39 bytes, text/x-review-board-request
ttaubert
: review+
smacleod
: review+
Details
39 bytes, text/x-review-board-request
ttaubert
: review+
smacleod
: review+
Details
39 bytes, text/x-review-board-request
ttaubert
: review+
smacleod
: review+
Details
39 bytes, text/x-review-board-request
ttaubert
: review+
smacleod
: review+
Details
39 bytes, text/x-review-board-request
ttaubert
: review+
smacleod
: review+
Details
39 bytes, text/x-review-board-request
ttaubert
: review+
smacleod
: review+
Details
39 bytes, text/x-review-board-request
ttaubert
: review+
smacleod
: review+
Details
39 bytes, text/x-review-board-request
ttaubert
: review+
smacleod
: review+
Details
39 bytes, text/x-review-board-request
ttaubert
: review+
smacleod
: review+
Details
39 bytes, text/x-review-board-request
ttaubert
: review+
smacleod
: review+
Details
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.
Flags: qe-verify-
Flags: firefox-backlog+
Hi Steven, can you provide a point value.
Iteration: --- → 38.3 - 23 Feb
Flags: needinfo?(smacleod)
Points: --- → 13
Flags: needinfo?(smacleod)
Iteration: 38.3 - 23 Feb → 39.1 - 9 Mar
/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
Attachment #8573590 - Flags: review?(wmccloskey)
Attachment #8573590 - Flags: review?(ttaubert)
Attachment #8573590 - Flags: review?(mconley)
Attachment #8573590 - Flags: review?(dtownsend)
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
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?
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.
Attachment #8573590 - Flags: review?(ttaubert) → feedback+
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.
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.
Flags: needinfo?(ttaubert)
(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 :)
Flags: needinfo?(ttaubert)
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?)
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.
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.
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*
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 on attachment 8573590 [details]
MozReview Request: bz://1132566/smacleod

r=me for the commits I reviewed with my nits fixed.
Attachment #8573590 - Flags: review?(mconley) → review+
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.
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.
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.
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.
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.
Attachment #8573590 - Flags: review?(dtownsend) → review+
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.
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.
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?
Attachment #8573590 - Flags: review?(wmccloskey) → review+
Iteration: 39.1 - 9 Mar → 39.2 - 23 Mar
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.
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?
(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.
(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?
(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.
(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
Oh, I see what you mean. I guess that's true. Never mind.
Attachment #8573590 - Flags: review?(wmccloskey)
Attachment #8573590 - Flags: review?(ttaubert)
Attachment #8573590 - Flags: review?(mconley)
Attachment #8573590 - Flags: review?(dtownsend)
Attachment #8573590 - Flags: review+
Attachment #8573590 - Flags: feedback+
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
Attachment #8573590 - Flags: review?(wmccloskey)
Attachment #8573590 - Flags: review?(mconley)
Attachment #8573590 - Flags: review?(dtownsend)
Attachment #8573590 - Flags: review+
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 on attachment 8573590 [details]
MozReview Request: bz://1132566/smacleod

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

Ship It!
Attachment #8573590 - Flags: review?(ttaubert) → review+
Blocks: 1142240
Depends on: 1142678
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
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
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 39
Attachment #8573590 - Attachment is obsolete: true
Attachment #8619437 - Flags: review+
Attachment #8619438 - Flags: review+
Attachment #8619439 - Flags: review+
Attachment #8619440 - Flags: review+
Attachment #8619441 - Flags: review+
Attachment #8619442 - Flags: review+
Attachment #8619443 - Flags: review+
Attachment #8619444 - Flags: review+
Attachment #8619445 - Flags: review+
Attachment #8619446 - Flags: review+
Attachment #8619447 - Flags: review+
Attachment #8619448 - Flags: review+
Attachment #8619449 - Flags: review+
Attachment #8619450 - Flags: review+
Attachment #8619451 - Flags: review+
Attachment #8619452 - Flags: review+
Attachment #8619453 - Flags: review+
Attachment #8619454 - Flags: review+
Attachment #8619455 - Flags: review+
Attachment #8619456 - Flags: review+
Attachment #8619457 - Flags: review+
Attachment #8619458 - Flags: review+
Attachment #8619459 - Flags: review+
Attachment #8619460 - Flags: review+
Attachment #8619461 - Flags: review+
Attachment #8619462 - Flags: review+
Attachment #8619463 - Flags: review+
Attachment #8619464 - Flags: review+
You need to log in before you can comment on or make changes to this bug.