Closed Bug 1261842 Opened 8 years ago Closed 8 years ago

Make initial browser remote sooner if we're defaulting to using remote tabs

Categories

(Firefox :: General, defect, P1)

defect

Tracking

()

RESOLVED FIXED
Firefox 50
Tracking Status
firefox50 --- fixed

People

(Reporter: mconley, Assigned: mconley)

References

(Depends on 1 open bug)

Details

Attachments

(25 files)

58 bytes, text/x-review-board-request
Felipe
: review+
Details
58 bytes, text/x-review-board-request
smaug
: review+
Details
58 bytes, text/x-review-board-request
smaug
: review+
Details
58 bytes, text/x-review-board-request
smaug
: review+
Details
58 bytes, text/x-review-board-request
smaug
: review+
Details
58 bytes, text/x-review-board-request
Felipe
: review+
Details
58 bytes, text/x-review-board-request
Felipe
: review+
Details
58 bytes, text/x-review-board-request
Felipe
: review+
Details
58 bytes, text/x-review-board-request
kmag
: review+
Details
58 bytes, text/x-review-board-request
Felipe
: review+
Details
58 bytes, text/x-review-board-request
jdm
: review+
Details
58 bytes, text/x-review-board-request
Gijs
: review+
Details
58 bytes, text/x-review-board-request
mikedeboer
: review+
Details
58 bytes, text/x-review-board-request
Gijs
: review+
Details
58 bytes, text/x-review-board-request
jdm
: review+
Details
58 bytes, text/x-review-board-request
ato
: review+
Details
58 bytes, text/x-review-board-request
jmaher
: review+
Details
58 bytes, text/x-review-board-request
gkrizsanits
: review+
Details
58 bytes, text/x-review-board-request
jdm
: review+
Details
58 bytes, text/x-review-board-request
mikedeboer
: review+
Details
58 bytes, text/x-review-board-request
baku
: review+
Details
58 bytes, text/x-review-board-request
Gijs
: review+
Details
58 bytes, text/x-review-board-request
smaug
: review+
Details
58 bytes, text/x-review-board-request
smaug
: review+
Details
58 bytes, text/x-review-board-request
automatedtester
: review+
Details
When a new browser window is opened (either just at start-up, or if the user does it by opening it from the parent, or if content opens a new window), the initial browser in the tabbrowser.xml binding is not remote.

In the case of start-up, or the user has opened the window from the parent via something like Ctrl-N, this is fine - as soon as the browser navigates to a page that is remote-friendly, the remoteness will flip automatically.

For windows that are opened from content, the new window is opened, and then ContentParent::RecvCreateWindow will call forceInitialBrowserRemote on the nsIXULBrowserWindow, which will force a remoteness flip on the initial browser before it unblocks the child (which is waiting on a sync message for an nsIDOMWindow to load content into).

This forceInitialBrowserRemote technique has some advantages, but also some disadvantages.

Advantages:
* Because the initial browser is non-remote, we can access the content docshell, which is very useful for sizing the viewport based on what has been passed through window.open.

Disadvantages:
* A number of our testing frameworks (talos, marionette are two that I know of) have to jump through some hoops in order to ensure that the windows that they create are remote.
* Forcing the initial browser to flip remoteness destroys the initially non-remote DocShell, which kicks off some async work to notify others that a window has been destroyed, and also "nukes JS compartment wrappers". This takes time, and is slowing down our getting to present the content to the user.

From my experimentation, it doesn't look like trying to update the remoteness of the browser inside the tabbrowser.xml <constructor> is soon enough - by the time the constructor is run, the initBrowser's docShell has already been created. We need to do the remoteness flip earlier, probably just as soon as the XUL DOM has been loaded.

I have confirmed that this works, and that we can avoid the original docShell destruction costs. There is, however, the problem of sizing the window; because the content docShell is no longer accessible by nsWindowWatcher, it computes size differently, meaning that that window.open calls that set a particular size are going to have their heights be incorrect. We'll need to do some work there to calculate the height correctly in this case.
So, regarding the height calculation, the foul-up occurs in nsXULWindow::SizeShellTo[1], which is called by nsWindowWatcher::OpenWindowInternal. If the initial browser is remote, what happens is that the nsIDocShellTreeItem that's passed to SizeShellTo ends up being the chrome-level DocShell for the newly opened window, and not the DocShell of the web content (which is obviously inaccessible, since it's out of process).

Here's a comparison of what occurs in [1] in the e10s and non-e10s case if I default to initing with a remote browser, and try to open a new window from content with a height of 100 on OS X:

# non-e10s case
In nsXULWindow::SizeShellTo, the height of the nsIDocShellTreeItem (the content docShell) is computed to be 950 units.

It computes a heightDelta of -850, since we're trying to open a window that's 100 units tall.

Then it gets the size of the just-opened nsXULWindow, and computes a height of 1004.

It then chooses the max of (1004 - 850) and 100. The max is (1004 - 850), which is 154. That’s the correct value to size the new window to; 54 pixels accounts for the titlebar and the toolbar.

# e10s case:
In nsXULWindow::SizeShellTo, the height of the nsIDocShellTreeItem (the chrome window docShell) is computed to be 982 units.

It computes a heightDelta of -882.

Then it gets the size of the nsXULWindow, and computes a height of 1004.

The max of (1004 - 882) and 100 is (1004 - 882), which is 122. That’s the wrong value - We’re off by 32 pixels, which is the height of the toolbar.


I think I can get around this by having nsWindowWatcher GetInterface the mPrimaryTabParent off of the nsContentTreeOwner that gets assigned to newChrome. From there, we can get nsWindowWatcher to GetOwnerElement the TabParent to get at the <xul:browser>. If it finds one, then it can detect the dimensions of the browser using ClientWidth and ClientHeight. If we don't find an nsITabParent/ownerElement, then we fall back to getting at the chrome window DocShell.

My one worry is that the content nsIDocShellTreeItem gets QI'd to an nsIDocShell and gets some things called on it before the size is set. In the e10s case, I'm not certain that we're remoting any of those settings over, but I don't think we are. With the current initting-as-non-remote state of the world, I think it means that this work just gets thrown out since we end up destroying the initial docShell.

If I were to go the route of attempting to compute the correct dimensions of the new window using the <xul:browser> dimensions, I might need to audit the rest of the actions that occur on the nsIDocShellTreeItem / nsIDocShell, to make sure unexpected stuff doesn't get set on the chrome-window docShell.

[1]: https://dxr.mozilla.org/mozilla-central/rev/9bd90088875399347b05d87c67d3709e31539dcd/xpfe/appshell/nsXULWindow.cpp#1803
Hey smaug, I know I just posted a wall of text, but I wanted your opinion on something:

Do you think I'm prepping myself for pain / failure if I attempt to modify nsWindowWatcher::OpenWindowInternal to use the mFrameElement of the initial mPrimaryTabParent for a new window to set its dimensions, since the content docShell which was originally used is no longer available? Or is there a simpler way of doing it?
Flags: needinfo?(bugs)
This is really a case where I would need to read all the relevant code to say anything useful.
Your approach is probably, fine, but keep code maintenance in mind all the time.
WW code is complicated, and getting even harder to read all the time.
Flags: needinfo?(bugs)
Initial data suggests that this will have a very nice win against non-e10s for both tpaint and ts_paint.

https://treeherder.mozilla.org/perf.html#/e10s?repo=try&revision=27e3a9012100
Depends on: 1267720
Priority: -- → P1
Depends on: 1277644
Depends on: 1278133
Depends on: 1278985
Depends on: 1278989
Blocks: 1278985
No longer depends on: 1278985
Review commit: https://reviewboard.mozilla.org/r/58792/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/58792/
Attachment #8761698 - Flags: review?(felipc)
Attachment #8761703 - Flags: review?(felipc)
Attachment #8761704 - Flags: review?(felipc)
Attachment #8761705 - Flags: review?(felipc)
Attachment #8761706 - Flags: review?(kmaglione+bmo)
Attachment #8761707 - Flags: review?(felipc)
Attachment #8761708 - Flags: review?(ehsan)
Attachment #8761709 - Flags: review?(gijskruitbosch+bugs)
Attachment #8761710 - Flags: review?(mdeboer)
Attachment #8761711 - Flags: review?(gijskruitbosch+bugs)
Attachment #8761712 - Flags: review?(ehsan)
Attachment #8761713 - Flags: review?(ato)
Attachment #8761714 - Flags: review?(jmaher)
Attachment #8761715 - Flags: review?(gkrizsanits)
Attachment #8761716 - Flags: review?(ehsan)
Attachment #8761717 - Flags: review?(mdeboer)
Attachment #8761718 - Flags: review?(amarchesini)
Attachment #8761719 - Flags: review?(gijskruitbosch+bugs)
By the time that the parent is being asked to create a new window, the name
really doesn't matter anymore.

Review commit: https://reviewboard.mozilla.org/r/58796/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/58796/
The fact that this happened to work in e10s mode is a consequence of
the fact that the initial browser started out non-remote, even in
e10s mode. Coupled with the fact that the browser only loaded
about:blank (which didn't cause a remoteness flip), this essentially
meant we were testing the non-e10s case with a non-remote browser.

Review commit: https://reviewboard.mozilla.org/r/58818/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/58818/
Before, it was assumed that the next load was the one that the Marionette client had
asked for, when this might not be the case. For example, when a new window opens,
it's possible for the initial about:blank load to be fired in content after the
parent has asked for a page to be loaded.

Review commit: https://reviewboard.mozilla.org/r/58822/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/58822/
Comment on attachment 8761714 [details]
Bug 1261842 - Make pageloader assume that initial browsers are remote in e10s mode.

https://reviewboard.mozilla.org/r/58824/#review55702

thanks for this, I like simple changes.  I assume you have verified this runs on try for all talos jobs?
Attachment #8761714 - Flags: review?(jmaher) → review+
Comment on attachment 8761710 [details]
Bug 1261842 - Update browser_394759_behavior.js to account for initial browser being remote.

https://reviewboard.mozilla.org/r/58816/#review55706

This change LGTM.
Attachment #8761710 - Flags: review?(mdeboer) → review+
Attachment #8761718 - Flags: review?(amarchesini) → review+
Comment on attachment 8761718 [details]
Bug 1261842 - Tests in browser_windowStateContainer.js should wait for new browser windows to completely load.

https://reviewboard.mozilla.org/r/58832/#review55708

::: browser/components/sessionstore/test/browser_windowStateContainer.js:71
(Diff revision 1)
>    yield BrowserTestUtils.closeWindow(win2);
>  });
>  
>  add_task(function* () {
> -  let win = window.openDialog(location, "_blank", "chrome,all,dialog=no");
> -  yield promiseWindowLoaded(win);
> +  let win = yield BrowserTestUtils.openNewBrowserWindow();
> +  yield TabStateFlusher.flush(win.gBrowser.selectedBrowser);

Why this?
Comment on attachment 8761699 [details]
Bug 1261842 - Factor out logic for creating windows for content processes from nsWindowWatcher::OpenWindowInternal.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58794/diff/1-2/
Attachment #8761699 - Flags: review?(bugs)
Attachment #8761700 - Flags: review?(bugs)
Attachment #8761701 - Flags: review?(bugs)
Attachment #8761702 - Flags: review?(bugs)
Comment on attachment 8761700 [details]
Bug 1261842 - Stop sending the window name to ContentParent when opening a new window.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58796/diff/1-2/
Comment on attachment 8761701 [details]
Bug 1261842 - ContentParent::RecvCreateWindow should always set an nsresult outparam.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58798/diff/1-2/
Comment on attachment 8761702 [details]
Bug 1261842 - Add methods to nsIDocShellTreeOwner for sizing the primary content.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58800/diff/1-2/
Comment on attachment 8761703 [details]
Bug 1261842 - browser_bug495058.js no longer needs to wait for remoteness flip on initial browser of new window.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58802/diff/1-2/
Comment on attachment 8761704 [details]
Bug 1261842 - browser_394759_behavior.js no longer needs to wait for remoteness flip on initial browser of new window.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58804/diff/1-2/
Comment on attachment 8761705 [details]
Bug 1261842 - browser_423132.js no longer needs to wait for remoteness flip on initial browser of new window.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58806/diff/1-2/
Comment on attachment 8761706 [details]
Bug 1261842 - Make sure ExtensionContent.jsm has been loaded in the parent process if Extension.jsm is initialized.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58808/diff/1-2/
Comment on attachment 8761707 [details]
Bug 1261842 - browser_async_window_flushing.js no longer needs to wait for remoteness flip on initial browser of new window.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58810/diff/1-2/
Comment on attachment 8761708 [details]
Bug 1261842 - Update browser_privatebrowsing_downloadLastDir_toggle.js to use add_task and BrowserTestUtils.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58812/diff/1-2/
Comment on attachment 8761709 [details]
Bug 1261842 - Update browser_bug495058.js to account for the initial browser being remote.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58814/diff/1-2/
Comment on attachment 8761710 [details]
Bug 1261842 - Update browser_394759_behavior.js to account for initial browser being remote.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58816/diff/1-2/
Comment on attachment 8761711 [details]
Bug 1261842 - Disable browser_contentAreaClick.js for e10s.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58818/diff/1-2/
Comment on attachment 8761712 [details]
Bug 1261842 - Fix up some private browsing tests to account for initial browsers being remote now.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58820/diff/1-2/
Comment on attachment 8761713 [details]
Bug 1261842 - Make Marionette listener ensure that the loaded document is the one that was actually requested.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58822/diff/1-2/
Comment on attachment 8761714 [details]
Bug 1261842 - Make pageloader assume that initial browsers are remote in e10s mode.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58824/diff/1-2/
Comment on attachment 8761715 [details]
Bug 1261842 - Add a test for chromeflags for new windows from content.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58826/diff/1-2/
Comment on attachment 8761716 [details]
Bug 1261842 - Add a test that ensures that proper chromeFlags and nsILoadContext properties are set for private windows.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58828/diff/1-2/
Comment on attachment 8761717 [details]
Bug 1261842 - When putting the initial tab into the restored background state, flip it to non-remote.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58830/diff/1-2/
Comment on attachment 8761718 [details]
Bug 1261842 - Tests in browser_windowStateContainer.js should wait for new browser windows to completely load.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58832/diff/1-2/
Comment on attachment 8761719 [details]
Bug 1261842 - Make browser_testOpenNewRemoteTabsFromNonRemoteBrowsers.js wait for windows to be ready before sending them places.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58834/diff/1-2/
https://reviewboard.mozilla.org/r/58832/#review55708

> Why this?

We need to make sure that the initial browser has finished being set up, and then flush messages to make sure the SessionHistoryListener has a chance to send its message (with the userContextId) up to the parent to stash into the window state.
Comment on attachment 8761703 [details]
Bug 1261842 - browser_bug495058.js no longer needs to wait for remoteness flip on initial browser of new window.

https://reviewboard.mozilla.org/r/58802/#review55720

Stealing this because I was confused by the diff on the one you asked me to review :-)
Attachment #8761703 - Flags: review+
Comment on attachment 8761709 [details]
Bug 1261842 - Update browser_bug495058.js to account for the initial browser being remote.

https://reviewboard.mozilla.org/r/58814/#review55722
Attachment #8761709 - Flags: review?(gijskruitbosch+bugs) → review+
https://reviewboard.mozilla.org/r/58814/#review55724

(oh, as noted on IRC, please file a followup to move this to the urlbar/ directory if it now passes if we do that.)
Comment on attachment 8761719 [details]
Bug 1261842 - Make browser_testOpenNewRemoteTabsFromNonRemoteBrowsers.js wait for windows to be ready before sending them places.

https://reviewboard.mozilla.org/r/58834/#review55726
Attachment #8761719 - Flags: review?(gijskruitbosch+bugs) → review+
Attachment #8761711 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 8761711 [details]
Bug 1261842 - Disable browser_contentAreaClick.js for e10s.

https://reviewboard.mozilla.org/r/58818/#review55728

Please file a followup to test this stuff for the e10s case. In particular whether we do security checks if we go through ContentClick.jsm is important, and we shouldn't be regressing that.
Thanks - filed bug 1279320 for moving the test to the urlbar directory, and bug 1279322 for adding new tests for clicking content for e10s.
Comment on attachment 8761713 [details]
Bug 1261842 - Make Marionette listener ensure that the loaded document is the one that was actually requested.

https://reviewboard.mozilla.org/r/58822/#review55774
Attachment #8761713 - Flags: review?(ato) → review+
Comment on attachment 8761706 [details]
Bug 1261842 - Make sure ExtensionContent.jsm has been loaded in the parent process if Extension.jsm is initialized.

https://reviewboard.mozilla.org/r/58808/#review55782
Attachment #8761706 - Flags: review?(kmaglione+bmo) → review+
Comment on attachment 8761708 [details]
Bug 1261842 - Update browser_privatebrowsing_downloadLastDir_toggle.js to use add_task and BrowserTestUtils.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58812/diff/2-3/
Comment on attachment 8761709 [details]
Bug 1261842 - Update browser_bug495058.js to account for the initial browser being remote.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58814/diff/2-3/
Comment on attachment 8761710 [details]
Bug 1261842 - Update browser_394759_behavior.js to account for initial browser being remote.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58816/diff/2-3/
Comment on attachment 8761711 [details]
Bug 1261842 - Disable browser_contentAreaClick.js for e10s.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58818/diff/2-3/
Comment on attachment 8761712 [details]
Bug 1261842 - Fix up some private browsing tests to account for initial browsers being remote now.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58820/diff/2-3/
Comment on attachment 8761713 [details]
Bug 1261842 - Make Marionette listener ensure that the loaded document is the one that was actually requested.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58822/diff/2-3/
Comment on attachment 8761714 [details]
Bug 1261842 - Make pageloader assume that initial browsers are remote in e10s mode.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58824/diff/2-3/
Comment on attachment 8761715 [details]
Bug 1261842 - Add a test for chromeflags for new windows from content.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58826/diff/2-3/
Comment on attachment 8761716 [details]
Bug 1261842 - Add a test that ensures that proper chromeFlags and nsILoadContext properties are set for private windows.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58828/diff/2-3/
Comment on attachment 8761717 [details]
Bug 1261842 - When putting the initial tab into the restored background state, flip it to non-remote.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58830/diff/2-3/
Comment on attachment 8761718 [details]
Bug 1261842 - Tests in browser_windowStateContainer.js should wait for new browser windows to completely load.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58832/diff/2-3/
Comment on attachment 8761719 [details]
Bug 1261842 - Make browser_testOpenNewRemoteTabsFromNonRemoteBrowsers.js wait for windows to be ready before sending them places.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58834/diff/2-3/
Attachment #8761717 - Flags: review?(mdeboer)
Comment on attachment 8761717 [details]
Bug 1261842 - When putting the initial tab into the restored background state, flip it to non-remote.

https://reviewboard.mozilla.org/r/58830/#review55848

::: browser/components/sessionstore/SessionStore.jsm:3202
(Diff revision 3)
> +    // remote, we need to flip it back to non-remote if it's going
> +    // to go into the pending background tab state. This is to make
> +    // sure that the background tab can't crash if it hasn't yet
> +    // been restored. Normally, when a window is restored, the tabs
> +    // that SessionStore inserts are non-remote - but the initial
> +    // browser is, but default, remote, so this check and flip is

nit: s/but/by/

::: browser/components/sessionstore/SessionStore.jsm:3204
(Diff revision 3)
> +    // sure that the background tab can't crash if it hasn't yet
> +    // been restored. Normally, when a window is restored, the tabs
> +    // that SessionStore inserts are non-remote - but the initial
> +    // browser is, but default, remote, so this check and flip is
> +    // mostly for that case.
> +    if (browser.isRemoteBrowser && (!restoreImmediately || forceOnDemand)) {

Why can't we put

```js
if (browser.isRemoteBrowser) {
  tabbrowser.updateBrowserRemoteness(browser, false);
}
```

inside the else-clause at http://searchfox.org/mozilla-central/source/browser/components/sessionstore/SessionStore.jsm#3307 ?
https://reviewboard.mozilla.org/r/58830/#review55848

> Why can't we put
> 
> ```js
> if (browser.isRemoteBrowser) {
>   tabbrowser.updateBrowserRemoteness(browser, false);
> }
> ```
> 
> inside the else-clause at http://searchfox.org/mozilla-central/source/browser/components/sessionstore/SessionStore.jsm#3307 ?

Because we have to flip the remoteness before we send down the session history with this message here: http://searchfox.org/mozilla-central/rev/8f19409ff1e36ab98d0f8dc1c8c0f75357bf625a/browser/components/sessionstore/SessionStore.jsm#3294.

So long as I do it before there, I can put it wherever you'd like. :) Let me know where you'd prefer it.
https://reviewboard.mozilla.org/r/58830/#review55848

> Because we have to flip the remoteness before we send down the session history with this message here: http://searchfox.org/mozilla-central/rev/8f19409ff1e36ab98d0f8dc1c8c0f75357bf625a/browser/components/sessionstore/SessionStore.jsm#3294.
> 
> So long as I do it before there, I can put it wherever you'd like. :) Let me know where you'd prefer it.

mikedeboer and I concluded that we can probably live with putting this instruction here.
Comment on attachment 8761698 [details]
Bug 1261842 - Make initial browser remote sooner if we're defaulting to using remote tabs.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58792/diff/1-2/
Attachment #8761703 - Flags: review+
Attachment #8761717 - Flags: review?(mdeboer)
Comment on attachment 8761699 [details]
Bug 1261842 - Factor out logic for creating windows for content processes from nsWindowWatcher::OpenWindowInternal.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58794/diff/2-3/
Comment on attachment 8761700 [details]
Bug 1261842 - Stop sending the window name to ContentParent when opening a new window.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58796/diff/2-3/
Comment on attachment 8761701 [details]
Bug 1261842 - ContentParent::RecvCreateWindow should always set an nsresult outparam.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58798/diff/2-3/
Comment on attachment 8761702 [details]
Bug 1261842 - Add methods to nsIDocShellTreeOwner for sizing the primary content.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58800/diff/2-3/
Comment on attachment 8761703 [details]
Bug 1261842 - browser_bug495058.js no longer needs to wait for remoteness flip on initial browser of new window.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58802/diff/2-3/
Comment on attachment 8761704 [details]
Bug 1261842 - browser_394759_behavior.js no longer needs to wait for remoteness flip on initial browser of new window.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58804/diff/2-3/
Comment on attachment 8761705 [details]
Bug 1261842 - browser_423132.js no longer needs to wait for remoteness flip on initial browser of new window.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58806/diff/2-3/
Comment on attachment 8761706 [details]
Bug 1261842 - Make sure ExtensionContent.jsm has been loaded in the parent process if Extension.jsm is initialized.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58808/diff/2-3/
Comment on attachment 8761707 [details]
Bug 1261842 - browser_async_window_flushing.js no longer needs to wait for remoteness flip on initial browser of new window.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58810/diff/2-3/
Comment on attachment 8761708 [details]
Bug 1261842 - Update browser_privatebrowsing_downloadLastDir_toggle.js to use add_task and BrowserTestUtils.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58812/diff/3-4/
Comment on attachment 8761709 [details]
Bug 1261842 - Update browser_bug495058.js to account for the initial browser being remote.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58814/diff/3-4/
Comment on attachment 8761710 [details]
Bug 1261842 - Update browser_394759_behavior.js to account for initial browser being remote.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58816/diff/3-4/
Comment on attachment 8761711 [details]
Bug 1261842 - Disable browser_contentAreaClick.js for e10s.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58818/diff/3-4/
Comment on attachment 8761712 [details]
Bug 1261842 - Fix up some private browsing tests to account for initial browsers being remote now.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58820/diff/3-4/
Comment on attachment 8761713 [details]
Bug 1261842 - Make Marionette listener ensure that the loaded document is the one that was actually requested.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58822/diff/3-4/
Comment on attachment 8761714 [details]
Bug 1261842 - Make pageloader assume that initial browsers are remote in e10s mode.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58824/diff/3-4/
Comment on attachment 8761715 [details]
Bug 1261842 - Add a test for chromeflags for new windows from content.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58826/diff/3-4/
Comment on attachment 8761716 [details]
Bug 1261842 - Add a test that ensures that proper chromeFlags and nsILoadContext properties are set for private windows.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58828/diff/3-4/
Comment on attachment 8761717 [details]
Bug 1261842 - When putting the initial tab into the restored background state, flip it to non-remote.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58830/diff/3-4/
Comment on attachment 8761718 [details]
Bug 1261842 - Tests in browser_windowStateContainer.js should wait for new browser windows to completely load.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58832/diff/3-4/
Comment on attachment 8761719 [details]
Bug 1261842 - Make browser_testOpenNewRemoteTabsFromNonRemoteBrowsers.js wait for windows to be ready before sending them places.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58834/diff/3-4/
Attachment #8761717 - Flags: review?(mdeboer) → review+
Comment on attachment 8761717 [details]
Bug 1261842 - When putting the initial tab into the restored background state, flip it to non-remote.

https://reviewboard.mozilla.org/r/58830/#review55902
Comment on attachment 8761700 [details]
Bug 1261842 - Stop sending the window name to ContentParent when opening a new window.

https://reviewboard.mozilla.org/r/58796/#review56014
Attachment #8761700 - Flags: review?(bugs) → review+
Comment on attachment 8761701 [details]
Bug 1261842 - ContentParent::RecvCreateWindow should always set an nsresult outparam.

https://reviewboard.mozilla.org/r/58798/#review56016
Attachment #8761701 - Flags: review?(bugs) → review+
Attachment #8761702 - Flags: review?(bugs) → review-
Comment on attachment 8761702 [details]
Bug 1261842 - Add methods to nsIDocShellTreeOwner for sizing the primary content.

https://reviewboard.mozilla.org/r/58800/#review56030

I think this needs some clarifications, including what sizeShellToWithLimit actually does (even though it was added in Bug 1255138).

::: docshell/base/nsIDocShellTreeOwner.idl:83
(Diff revision 3)
>  	Tells the tree owner to size its window or parent window in such a way
>  	that the shell passed along will be the size specified.
>  	*/
>  	void sizeShellTo(in nsIDocShellTreeItem shell, in long cx, in long cy);
>  
> +	void getPrimaryContentSize(out long width, out long height);

I know sizeShellTo doesn't explain which coordinate space is being used here, but please add a comment to the new methods at least.

::: embedding/components/windowwatcher/nsWindowWatcher.cpp:2362
(Diff revision 3)
>     is assumed to be called after the window has already been given
>     a default position and size; thus its current position and size are
>     accurate defaults. The new window is made visible at method end.
>  */
>  void
> -nsWindowWatcher::SizeOpenedDocShellItem(nsIDocShellTreeItem* aDocShellItem,
> +nsWindowWatcher::SizeOpenedWindow(nsIDocShellTreeOwner* aTreeOwner,

So this is now rather unclear method. The name hints about some window but param names don't tell which window. aTreeOwner? Which tree owner? The owner of aParent or something else?

::: xpfe/appshell/nsXULWindow.h:171
(Diff revision 3)
>  
>     nsCOMArray<nsIWeakReference> mTargetableShells; // targetable shells only
>  
>     nsCOMPtr<nsITabParent> mPrimaryTabParent;
> +private:
> +   NS_IMETHOD GetPrimaryTabParentSize(int32_t* aWidth, int32_t* aHeight);

Why these 3 methods are NS_IMETHOD? just nsresult as return type should be enough.

::: xpfe/appshell/nsXULWindow.cpp:1840
(Diff revision 3)
> +  NS_ENSURE_STATE(mPrimaryContentShell);
> +
> +  nsCOMPtr<nsIBaseWindow> shellWindow(do_QueryInterface(mPrimaryContentShell));
> +  NS_ENSURE_STATE(shellWindow);
> +
> +  int32_t cox, coy;

Since our coordinate handling in general is rather messy, I'd prefer to see some explanation what we're doing here. What kind of size does GetSize return and what GetPrimaryContentShellSize returns, in which coordinate space.

Also, why cox, coy, why not width, height ?

I assume you've tested this all with and without highdpi

::: xpfe/appshell/nsXULWindow.cpp:1874
(Diff revision 3)
> +  GetPrimaryTabParentSize(&shellWidth, &shellHeight);
> +
> +  double scale = 1.0;
> +  GetUnscaledDevicePixelsPerCSSPixel(&scale);
> +
> +  SizeShellToWithLimit(aWidth, aHeight,

I wish I understood what SizeShellToWithLimit does.
It was added in bug 1255138, but the documentation about it is very unclear. Doesn't tell at all what parameters mean.
And its implementation doesn't seem to deal with any shell but something else.
Comment on attachment 8761699 [details]
Bug 1261842 - Factor out logic for creating windows for content processes from nsWindowWatcher::OpenWindowInternal.

https://reviewboard.mozilla.org/r/58794/#review56042

I guess this will need couple of iterations, largely because the old code is complicated.

::: embedding/components/windowwatcher/nsPIWindowWatcher.idl:81
(Diff revision 3)
> -                                 in nsITabParent aOpeningTab,
>                                   in nsISupports aArgs,
>                                   [optional] in float aOpenerFullZoom);
>  
>    /**
> +   * Used by the Service Worker Client.openWindow API. This allows notification

I think the naming isn't quite right here. The name should somehow indicate what the method does, not only when it is supposed to be called.

It is unclear from the names how they end up behaving differently.

::: embedding/components/windowwatcher/nsWindowWatcher.cpp:490
(Diff revision 3)
>    return principalUserContextId == userContextId;
>  }
>  
> +
> +NS_IMETHODIMP
> +nsWindowWatcher::OpenWindowForTablessContent(nsITabParent** aResult)

Hmm, these two new OpenWindowFor* method implementations have plenty of common things. All the common bits should be separated to helper methods, otherwise it is pretty much guaranteed that rarely used OpenWindowForTablessContent will miss changes that the other method gets.

::: embedding/components/windowwatcher/nsWindowWatcher.cpp:520
(Diff revision 3)
> +  }
> +
> +  uint32_t contextFlags = 0;
> +  if (parentWindowOuter->IsLoadingOrRunningTimeout()) {
> +    contextFlags |=
> +            nsIWindowCreator2::PARENT_IS_LOADING_OR_RUNNING_TIMEOUT;

odd indentation here

::: embedding/components/windowwatcher/nsWindowWatcher.cpp:1801
(Diff revision 3)
> + * @param aCalledFromJS true if the window open request came from script.
> + * @return the chrome bitmask
> + */
> +// static
> +uint32_t
> +nsWindowWatcher::CalculateChromeFlagsForContent(const nsACString& aFeatures,

I'm a bit lost why we need this separate method, or why we can't reuse this kind of stuff for content and chrome?

::: embedding/components/windowwatcher/nsWindowWatcher.cpp:2081
(Diff revision 3)
>    return chromeFlags;
>  }
>  
>  // static
>  int32_t
> -nsWindowWatcher::WinHasOption(const char* aOptions, const char* aName,
> +nsWindowWatcher::WinHasOption(const nsACString& aOptions, const char* aName,

FWIW, this kind of change could have been done as a separate patch to decrease the size of this complicated patch.
Attachment #8761699 - Flags: review?(bugs) → review-
Attachment #8761715 - Flags: review?(gkrizsanits) → review+
Comment on attachment 8761715 [details]
Bug 1261842 - Add a test for chromeflags for new windows from content.

https://reviewboard.mozilla.org/r/58826/#review56698

Looks good.
Comment on attachment 8761698 [details]
Bug 1261842 - Make initial browser remote sooner if we're defaulting to using remote tabs.

https://reviewboard.mozilla.org/r/58792/#review57942

::: browser/base/content/browser.js:925
(Diff revision 2)
> +  window.QueryInterface(Ci.nsIInterfaceRequestor)
> +        .getInterface(nsIWebNavigation)
> +        .QueryInterface(Ci.nsIDocShellTreeItem).treeOwner
> +        .QueryInterface(Ci.nsIInterfaceRequestor)
> +        .getInterface(Ci.nsIXULWindow)
> +        .XULBrowserWindow = window.XULBrowserWindow;
> +  window.QueryInterface(Ci.nsIDOMChromeWindow).browserDOMWindow =
> +    new nsBrowserAccess();

why did this need to be moved here? AFAIK there's nothing related to the <browser> element here.

I'm worried some code (potentially C++) might want to use browserDOMWindow earlier.

If this really need to be here, what about having a stub nsBrowserAccess() that just prints an error message if used, which can then be replaced by this one?
Attachment #8761698 - Flags: review?(felipc) → review+
Comment on attachment 8761704 [details]
Bug 1261842 - browser_394759_behavior.js no longer needs to wait for remoteness flip on initial browser of new window.

https://reviewboard.mozilla.org/r/58804/#review57944
Attachment #8761704 - Flags: review?(felipc) → review+
Attachment #8761705 - Flags: review?(felipc) → review+
Comment on attachment 8761705 [details]
Bug 1261842 - browser_423132.js no longer needs to wait for remoteness flip on initial browser of new window.

https://reviewboard.mozilla.org/r/58806/#review57946
Attachment #8761707 - Flags: review?(felipc) → review+
Comment on attachment 8761707 [details]
Bug 1261842 - browser_async_window_flushing.js no longer needs to wait for remoteness flip on initial browser of new window.

https://reviewboard.mozilla.org/r/58810/#review57948
Attachment #8761703 - Flags: review?(felipc) → review+
Comment on attachment 8761703 [details]
Bug 1261842 - browser_bug495058.js no longer needs to wait for remoteness flip on initial browser of new window.

https://reviewboard.mozilla.org/r/58802/#review57950
https://reviewboard.mozilla.org/r/58792/#review57942

> why did this need to be moved here? AFAIK there's nothing related to the <browser> element here.
> 
> I'm worried some code (potentially C++) might want to use browserDOMWindow earlier.
> 
> If this really need to be here, what about having a stub nsBrowserAccess() that just prints an error message if used, which can then be replaced by this one?

> I'm worried some code (potentially C++) might want to use browserDOMWindow earlier.

That's actually what I'm trying to correct for here - the nsFrameLoader remoteness flipping stuff expects there to be a browserDOMWindow defined when creating the remote browser. I needed to move the definition of browserDOMWindow to before the flip might occur for the initial browser. Note that DOMContentLoaded fires *before* the load event handler, and not after.

Does that address your concern?
https://reviewboard.mozilla.org/r/58794/#review56042

> I think the naming isn't quite right here. The name should somehow indicate what the method does, not only when it is supposed to be called.
> 
> It is unclear from the names how they end up behaving differently.

I guess one fundamental difference is that OpenWindowForTablessContent doesn't have an opener nsITabParent to base the newly opened window off of - instead, OpenWindowForTablessContent uses the most recent non-private window as its parent.

Is OpenWindowWithoutParent and OpenWindowWithTabParent clearer?

> Hmm, these two new OpenWindowFor* method implementations have plenty of common things. All the common bits should be separated to helper methods, otherwise it is pretty much guaranteed that rarely used OpenWindowForTablessContent will miss changes that the other method gets.

Yep, good call. I'll see what I can extract out.

> I'm a bit lost why we need this separate method, or why we can't reuse this kind of stuff for content and chrome?

My rationale for this is because there are a number of feature strings that should only ever be used from privileged callers. For example, `dialog`, or `modal`. This seemed reasonable to do when I was trying to detangle the service worker / tab parent cases from OpenWindowInternal.

So CalculateChromeFlagsForContent has a very strict set of feature strings that it pays attention to, and callers don't have to worry about more privileged bits getting flipped. CalculateChromeFlags used to have various codepaths that should only ever flip bits if the caller wasn't privileged, and that was freaking me out, I guess. That's why I wanted to get all of that stuff out into its own method, and clean up CalculateChromeFlags (and turn it into CalculateChromeFlagsForParent) so that it runs only when the caller is privileged.

Does that make sense?

> FWIW, this kind of change could have been done as a separate patch to decrease the size of this complicated patch.

Good idea.
ni'ing felipe for comment 112, and smaug for comment 113.
Flags: needinfo?(felipc)
Flags: needinfo?(bugs)
(In reply to Mike Conley (:mconley) - (Needinfo me!) from comment #112)
> https://reviewboard.mozilla.org/r/58792/#review57942
> 
> > why did this need to be moved here? AFAIK there's nothing related to the <browser> element here.
> > 
> > I'm worried some code (potentially C++) might want to use browserDOMWindow earlier.
> > 
> > If this really need to be here, what about having a stub nsBrowserAccess() that just prints an error message if used, which can then be replaced by this one?
> 
> > I'm worried some code (potentially C++) might want to use browserDOMWindow earlier.
> 
> That's actually what I'm trying to correct for here - the nsFrameLoader
> remoteness flipping stuff expects there to be a browserDOMWindow defined
> when creating the remote browser. I needed to move the definition of
> browserDOMWindow to before the flip might occur for the initial browser.
> Note that DOMContentLoaded fires *before* the load event handler, and not
> after.
> 
> Does that address your concern?

Ah, right. I was thinking the other way around. Yep, sounds good!
Flags: needinfo?(felipc)
OpenWindowWithoutParent and OpenWindowWithTabParent sound ok, especially if there is some comment how they behave differently.

And about flags, at least factor the common bits to some common helper method. AndPerhaps we should use *Parent and *Child here, not *Parent and *Content.
Flags: needinfo?(bugs)
https://reviewboard.mozilla.org/r/58794/#review56042

> I guess one fundamental difference is that OpenWindowForTablessContent doesn't have an opener nsITabParent to base the newly opened window off of - instead, OpenWindowForTablessContent uses the most recent non-private window as its parent.
> 
> Is OpenWindowWithoutParent and OpenWindowWithTabParent clearer?

Going to go with OpenWindowWithoutParent and OpenWindowWithTabParent, and including documentation for how they behave differently, as per https://bugzilla.mozilla.org/show_bug.cgi?id=1261842#c116.

> My rationale for this is because there are a number of feature strings that should only ever be used from privileged callers. For example, `dialog`, or `modal`. This seemed reasonable to do when I was trying to detangle the service worker / tab parent cases from OpenWindowInternal.
> 
> So CalculateChromeFlagsForContent has a very strict set of feature strings that it pays attention to, and callers don't have to worry about more privileged bits getting flipped. CalculateChromeFlags used to have various codepaths that should only ever flip bits if the caller wasn't privileged, and that was freaking me out, I guess. That's why I wanted to get all of that stuff out into its own method, and clean up CalculateChromeFlags (and turn it into CalculateChromeFlagsForParent) so that it runs only when the caller is privileged.
> 
> Does that make sense?

Going to use \*Parent and \*Child here instead of \*Parent and \*Content, and will attempt to extract common bits to a helper function.
Comment on attachment 8761698 [details]
Bug 1261842 - Make initial browser remote sooner if we're defaulting to using remote tabs.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58792/diff/2-3/
Comment on attachment 8761699 [details]
Bug 1261842 - Factor out logic for creating windows for content processes from nsWindowWatcher::OpenWindowInternal.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58794/diff/3-4/
Comment on attachment 8761700 [details]
Bug 1261842 - Stop sending the window name to ContentParent when opening a new window.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58796/diff/3-4/
Comment on attachment 8761701 [details]
Bug 1261842 - ContentParent::RecvCreateWindow should always set an nsresult outparam.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58798/diff/3-4/
Comment on attachment 8761702 [details]
Bug 1261842 - Add methods to nsIDocShellTreeOwner for sizing the primary content.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58800/diff/3-4/
Comment on attachment 8761703 [details]
Bug 1261842 - browser_bug495058.js no longer needs to wait for remoteness flip on initial browser of new window.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58802/diff/3-4/
Comment on attachment 8761704 [details]
Bug 1261842 - browser_394759_behavior.js no longer needs to wait for remoteness flip on initial browser of new window.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58804/diff/3-4/
Comment on attachment 8761705 [details]
Bug 1261842 - browser_423132.js no longer needs to wait for remoteness flip on initial browser of new window.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58806/diff/3-4/
Comment on attachment 8761706 [details]
Bug 1261842 - Make sure ExtensionContent.jsm has been loaded in the parent process if Extension.jsm is initialized.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58808/diff/3-4/
Comment on attachment 8761707 [details]
Bug 1261842 - browser_async_window_flushing.js no longer needs to wait for remoteness flip on initial browser of new window.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58810/diff/3-4/
Comment on attachment 8761708 [details]
Bug 1261842 - Update browser_privatebrowsing_downloadLastDir_toggle.js to use add_task and BrowserTestUtils.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58812/diff/4-5/
Comment on attachment 8761709 [details]
Bug 1261842 - Update browser_bug495058.js to account for the initial browser being remote.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58814/diff/4-5/
Comment on attachment 8761710 [details]
Bug 1261842 - Update browser_394759_behavior.js to account for initial browser being remote.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58816/diff/4-5/
Comment on attachment 8761711 [details]
Bug 1261842 - Disable browser_contentAreaClick.js for e10s.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58818/diff/4-5/
Comment on attachment 8761712 [details]
Bug 1261842 - Fix up some private browsing tests to account for initial browsers being remote now.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58820/diff/4-5/
Comment on attachment 8761713 [details]
Bug 1261842 - Make Marionette listener ensure that the loaded document is the one that was actually requested.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58822/diff/4-5/
Comment on attachment 8761714 [details]
Bug 1261842 - Make pageloader assume that initial browsers are remote in e10s mode.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58824/diff/4-5/
Comment on attachment 8761715 [details]
Bug 1261842 - Add a test for chromeflags for new windows from content.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58826/diff/4-5/
Comment on attachment 8761716 [details]
Bug 1261842 - Add a test that ensures that proper chromeFlags and nsILoadContext properties are set for private windows.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58828/diff/4-5/
Comment on attachment 8761717 [details]
Bug 1261842 - When putting the initial tab into the restored background state, flip it to non-remote.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58830/diff/4-5/
Comment on attachment 8761718 [details]
Bug 1261842 - Tests in browser_windowStateContainer.js should wait for new browser windows to completely load.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58832/diff/4-5/
Comment on attachment 8761719 [details]
Bug 1261842 - Make browser_testOpenNewRemoteTabsFromNonRemoteBrowsers.js wait for windows to be ready before sending them places.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58834/diff/4-5/
Attachment #8761708 - Flags: review?(ehsan) → review?(josh)
Attachment #8761712 - Flags: review?(ehsan) → review?(josh)
Attachment #8761716 - Flags: review?(ehsan) → review?(josh)
https://reviewboard.mozilla.org/r/58800/#review56030

> So this is now rather unclear method. The name hints about some window but param names don't tell which window. aTreeOwner? Which tree owner? The owner of aParent or something else?

Added some documentation.

> Why these 3 methods are NS_IMETHOD? just nsresult as return type should be enough.

Yep, good point, thanks.

> Since our coordinate handling in general is rather messy, I'd prefer to see some explanation what we're doing here. What kind of size does GetSize return and what GetPrimaryContentShellSize returns, in which coordinate space.
> 
> Also, why cox, coy, why not width, height ?
> 
> I assume you've tested this all with and without highdpi

Added some documentation here. Yep, tested with high DPI. :)

> I wish I understood what SizeShellToWithLimit does.
> It was added in bug 1255138, but the documentation about it is very unclear. Doesn't tell at all what parameters mean.
> And its implementation doesn't seem to deal with any shell but something else.

I'll add some documentation.
Comment on attachment 8761698 [details]
Bug 1261842 - Make initial browser remote sooner if we're defaulting to using remote tabs.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58792/diff/3-4/
Attachment #8761702 - Flags: review?(bugs)
Comment on attachment 8768560 [details]
Bug 1261842 - Use nsACString in more places instead of raw strings inside nsWindowWatcher.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62732/diff/1-2/
Comment on attachment 8761699 [details]
Bug 1261842 - Factor out logic for creating windows for content processes from nsWindowWatcher::OpenWindowInternal.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58794/diff/4-5/
Comment on attachment 8761700 [details]
Bug 1261842 - Stop sending the window name to ContentParent when opening a new window.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58796/diff/4-5/
Comment on attachment 8761701 [details]
Bug 1261842 - ContentParent::RecvCreateWindow should always set an nsresult outparam.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58798/diff/4-5/
Comment on attachment 8761702 [details]
Bug 1261842 - Add methods to nsIDocShellTreeOwner for sizing the primary content.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58800/diff/4-5/
Comment on attachment 8761703 [details]
Bug 1261842 - browser_bug495058.js no longer needs to wait for remoteness flip on initial browser of new window.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58802/diff/4-5/
Comment on attachment 8761704 [details]
Bug 1261842 - browser_394759_behavior.js no longer needs to wait for remoteness flip on initial browser of new window.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58804/diff/4-5/
Comment on attachment 8761705 [details]
Bug 1261842 - browser_423132.js no longer needs to wait for remoteness flip on initial browser of new window.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58806/diff/4-5/
Comment on attachment 8761706 [details]
Bug 1261842 - Make sure ExtensionContent.jsm has been loaded in the parent process if Extension.jsm is initialized.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58808/diff/4-5/
Comment on attachment 8761707 [details]
Bug 1261842 - browser_async_window_flushing.js no longer needs to wait for remoteness flip on initial browser of new window.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58810/diff/4-5/
Comment on attachment 8761708 [details]
Bug 1261842 - Update browser_privatebrowsing_downloadLastDir_toggle.js to use add_task and BrowserTestUtils.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58812/diff/5-6/
Comment on attachment 8761709 [details]
Bug 1261842 - Update browser_bug495058.js to account for the initial browser being remote.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58814/diff/5-6/
Comment on attachment 8761710 [details]
Bug 1261842 - Update browser_394759_behavior.js to account for initial browser being remote.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58816/diff/5-6/
Comment on attachment 8761711 [details]
Bug 1261842 - Disable browser_contentAreaClick.js for e10s.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58818/diff/5-6/
Comment on attachment 8761712 [details]
Bug 1261842 - Fix up some private browsing tests to account for initial browsers being remote now.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58820/diff/5-6/
Comment on attachment 8761713 [details]
Bug 1261842 - Make Marionette listener ensure that the loaded document is the one that was actually requested.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58822/diff/5-6/
Comment on attachment 8761714 [details]
Bug 1261842 - Make pageloader assume that initial browsers are remote in e10s mode.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58824/diff/5-6/
Comment on attachment 8761715 [details]
Bug 1261842 - Add a test for chromeflags for new windows from content.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58826/diff/5-6/
Comment on attachment 8761716 [details]
Bug 1261842 - Add a test that ensures that proper chromeFlags and nsILoadContext properties are set for private windows.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58828/diff/5-6/
Comment on attachment 8761717 [details]
Bug 1261842 - When putting the initial tab into the restored background state, flip it to non-remote.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58830/diff/5-6/
Comment on attachment 8761718 [details]
Bug 1261842 - Tests in browser_windowStateContainer.js should wait for new browser windows to completely load.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58832/diff/5-6/
Comment on attachment 8761719 [details]
Bug 1261842 - Make browser_testOpenNewRemoteTabsFromNonRemoteBrowsers.js wait for windows to be ready before sending them places.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58834/diff/5-6/
Comment on attachment 8761708 [details]
Bug 1261842 - Update browser_privatebrowsing_downloadLastDir_toggle.js to use add_task and BrowserTestUtils.

https://reviewboard.mozilla.org/r/58812/#review59810
Attachment #8761708 - Flags: review?(josh) → review+
Attachment #8761716 - Flags: review?(josh) → review+
Comment on attachment 8761716 [details]
Bug 1261842 - Add a test that ensures that proper chromeFlags and nsILoadContext properties are set for private windows.

https://reviewboard.mozilla.org/r/58828/#review59812

::: browser/components/privatebrowsing/test/browser/browser_privatebrowsing_context_and_chromeFlags.js:29
(Diff revision 6)
> +    Assert.ok(chromeFlags & Ci.nsIWebBrowserChrome.CHROME_PRIVATE_WINDOW,
> +              "Should have the private window chrome flag");
> +  }
> +
> +  let loadContext = winDocShell.QueryInterface(Ci.nsILoadContext);
> +  Assert.ok(loadContext.usePrivateBrowsing,

We're trying to remove use of usePrivateBrowsing; let's check that the privateBrowsingId attribute of the docshell's origin attributes is nonzero instead.

::: browser/components/privatebrowsing/test/browser/browser_privatebrowsing_context_and_chromeFlags.js:34
(Diff revision 6)
> +  Assert.ok(loadContext.usePrivateBrowsing,
> +            "The parent window should be using private browsing");
> +
> +  return ContentTask.spawn(win.gBrowser.selectedBrowser, null, function*() {
> +    let loadContext = docShell.QueryInterface(Ci.nsILoadContext);
> +    Assert.ok(loadContext.usePrivateBrowsing,

Same here.
Comment on attachment 8761712 [details]
Bug 1261842 - Fix up some private browsing tests to account for initial browsers being remote now.

https://reviewboard.mozilla.org/r/58820/#review59850

::: browser/components/privatebrowsing/test/browser/browser_privatebrowsing_nonbrowser.js:5
(Diff revision 6)
> - * 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();
> -
> +/**
> + * Tests that we fire the last-pb-context-exited observer notification
> + * when the last private browsing window opens, even if a console was

when the last private browsing window closes

Also, we don't appear to be opening a console.
Attachment #8761712 - Flags: review?(josh) → review+
Attachment #8768560 - Flags: review?(bugs) → review+
Comment on attachment 8768560 [details]
Bug 1261842 - Use nsACString in more places instead of raw strings inside nsWindowWatcher.

https://reviewboard.mozilla.org/r/62732/#review60658

Those fixed, r+

::: embedding/components/windowwatcher/nsWindowWatcher.cpp:1574
(Diff revision 2)
>                                        bool aOpenedFromRemoteTab)
>  {
>    const bool inContentProcess = XRE_IsContentProcess();
>    uint32_t chromeFlags = 0;
>  
> -  if (!aFeaturesSpecified || !aFeatures) {
> +  if (aFeatures.IsEmpty()) {

aFeatures.IsVoid(), right?

::: embedding/components/windowwatcher/nsWindowWatcher.cpp:1689
(Diff revision 2)
> -    if (!PL_strcasestr(aFeatures, "close")) {
> +    if (!PL_strcasestr(aFeatures.BeginReading(), "close")) {
>        chromeFlags |= nsIWebBrowserChrome::CHROME_WINDOW_CLOSE;
>      }
>    }
>  
> -  if (aDialog && aFeaturesSpecified && !presenceFlag) {
> +  if (aDialog && !aFeatures.IsEmpty() && !presenceFlag) {

If I read the code right, you could check
!aFeatures.IsVoid() to achieve the current handling of aFeaturesSpecified.

::: embedding/components/windowwatcher/nsWindowWatcher.cpp:1791
(Diff revision 2)
>  // static
>  int32_t
> -nsWindowWatcher::WinHasOption(const char* aOptions, const char* aName,
> +nsWindowWatcher::WinHasOption(const nsACString& aOptions, const char* aName,
>                                int32_t aDefault, bool* aPresenceFlag)
>  {
> -  if (!aOptions) {
> +  if (aOptions.IsEmpty()) {

Hmm, ok, in this case IsEmpty() should be fine, since the loop wouldn't do anything with empty string anyhow. IsVoid() wouldn't be any better.
https://reviewboard.mozilla.org/r/58794/#review60668

I'll keep reviewing this. Super hard to review since functionality is partially copied from OpenWindowInternal to the new method.

::: embedding/components/windowwatcher/nsPIWindowWatcher.idl:99
(Diff revision 5)
> +   * @param aOpeningTab
> +   *        The nsITabParent that is requesting the new window be opened.
> +   * @param aFeatures
> +   *        Window features if called with window.open.
> +   * @param aCalledFromJS
> +   *        True if called via window.open.

...or similar.
openWindowWithoutParent() isn't called for window.open after all.
Comment on attachment 8761702 [details]
Bug 1261842 - Add methods to nsIDocShellTreeOwner for sizing the primary content.

https://reviewboard.mozilla.org/r/58800/#review60140

::: docshell/base/nsIDocShellTreeOwner.idl:89
(Diff revision 5)
> +	Gets the size of the primary content area in CSS pixels. This should work
> +	for both in-process and out-of-process content areas.
> +	*/
> +	void getPrimaryContentSize(out long width, out long height);
> +	/*
> +	Gets the size of the primary content area in CSS pixels. This should work

Sure "setPrimaryContentSize" does not "Gets the size"

::: embedding/components/windowwatcher/nsWindowWatcher.cpp:2306
(Diff revision 5)
>    width = NSToIntRound(width / scale);
>    height = NSToIntRound(height / scale);
>    {
> -    // scope shellWindow why not
> -    nsCOMPtr<nsIBaseWindow> shellWindow(do_QueryInterface(aDocShellItem));
> -    if (shellWindow) {
> +    int32_t contentWidth, contentHeight;
> +    aTreeOwner->GetPrimaryContentSize(&contentWidth, &contentHeight);
> +    chromeWidth = width - contentWidth;

I don't understand this stuff. Why chrome size depends on content size? Browser chrome contains the content.

::: xpfe/appshell/moz.build:42
(Diff revision 5)
>      '/dom/base',
>  ]
>  
>  FINAL_LIBRARY = 'xul'
> +
> +include('/ipc/chromium/chromium-config.mozbuild')

Why is this needed? For TabParent ?

::: xpfe/appshell/nsXULWindow.cpp:1814
(Diff revision 5)
> +  if (mPrimaryTabParent) {
> +    return GetPrimaryTabParentSize(aWidth, aHeight);
> +  } else if (mPrimaryContentShell) {
> +    return GetPrimaryContentShellSize(aWidth, aHeight);
> +  } else {
> +    return GetSize(aWidth, aHeight);

Why do we ever want to return the 'normal' size here?

::: xpfe/appshell/nsXULWindow.cpp:1863
(Diff revision 5)
> +    return SetPrimaryTabParentSize(aWidth, aHeight);
> +  } else if (mPrimaryContentShell) {
> +    return SizeShellTo(mPrimaryContentShell, aWidth, aHeight);
> +  } else if (mDocShell) {
> +    nsCOMPtr<nsIDocShellTreeItem> docShellAsItem = do_QueryInterface(mDocShell);
> +    return SizeShellTo(docShellAsItem, aWidth, aHeight);

This looks wrong. Why do we want to set the top level size to something which is meant for the primary content size.
Attachment #8761702 - Flags: review?(bugs) → review-
Attachment #8761699 - Flags: review?(bugs) → review-
Comment on attachment 8761699 [details]
Bug 1261842 - Factor out logic for creating windows for content processes from nsWindowWatcher::OpenWindowInternal.

https://reviewboard.mozilla.org/r/58794/#review60758

If you can manage to move some similar looking code from OpenWindowInternal and this new method to helper methods, I think it would make the code more maintainable.
Or if you think that isn't possible, ask review again.

But overall this is looking good. Just hard to review because of all the flag handling :/

::: embedding/components/windowwatcher/nsWindowWatcher.cpp:517
(Diff revision 5)
> +
> +  bool isPrivateBrowsingWindow =
> +    Preferences::GetBool("browser.privatebrowsing.autostart");
> +
> +  nsCOMPtr<nsPIDOMWindowOuter> parentWindowOuter;
> +  if (aOpeningTabParent) {

ok, this is almost like in non-e10s case. But I guess it is hard to move this pb flag setting to a helper.

::: embedding/components/windowwatcher/nsWindowWatcher.cpp:584
(Diff revision 5)
> +
> +  nsresult rv =
> +    windowCreator2->CreateChromeWindow2(parentChrome, chromeFlags, contextFlags,
> +                                        aOpeningTabParent, &cancel,
> +                                        getter_AddRefs(newWindowChrome));
> +

There is some very similar looking code in OpenWindowInternal. I think you could move that stuff to a helper.
Possibly starting from #ifdef MOZ_WIDGET_GONK and ending to return NS_ERROR_ABORT

::: embedding/components/windowwatcher/nsWindowWatcher.cpp:623
(Diff revision 5)
> +  SizeOpenedDocShellItem(chromeTreeItem, parentWindowOuter, false, sizeSpec,
> +                         &aOpenerFullZoom);
> +  if (PL_strcasestr(features.get(), "width=") ||
> +      PL_strcasestr(features.get(), "height=")) {
> +    chromeTreeOwner->SetPersistence(false, false, false);
> +  }

Also width=/height= and persistence handling is the same as in OpenWindowInteral. Some helper method please.

::: embedding/components/windowwatcher/nsWindowWatcher.cpp:733
(Diff revision 5)
> -      doc && nsContentUtils::IsChromeDoc(doc) && !openedFromRemoteTab;
>    }
>  
> +  bool isCallerChrome = nsContentUtils::LegacyIsCallerChromeOrNativeCode();
> +
>    // Make sure we call CalculateChromeFlags() *before* we push the

There is no CalculateChromeFlags anymore

::: embedding/components/windowwatcher/nsWindowWatcher.cpp:1783
(Diff revision 5)
> + */
> +// static
> +uint32_t
> +nsWindowWatcher::CalculateChromeFlagsForChild(const nsACString& aFeatures)
> +{
> +  if (aFeatures.IsEmpty()) {

Should this be IsVoid(), not IsEmpty()?

::: embedding/components/windowwatcher/nsWindowWatcher.cpp:1900
(Diff revision 5)
>      chromeFlags |= nsIWebBrowserChrome::CHROME_WINDOW_LOWERED;
>    } else if (WinHasOption(aFeatures, "alwaysRaised", 0, nullptr)) {
>      chromeFlags |= nsIWebBrowserChrome::CHROME_WINDOW_RAISED;
>    }
>  
>    chromeFlags |= WinHasOption(aFeatures, "macsuppressanimation", 0, nullptr) ?

Ok, this stuff and other features moving to parent/chrome only is technically a change to the API, but as we discussed, expected and good change.
https://reviewboard.mozilla.org/r/62732/#review60658

> aFeatures.IsVoid(), right?

According to my understanding of our string API, setting a string to Void causes it to truncate to length of 0[1] making it empty anyways, no?

[1]: Setting Void calls http://searchfox.org/mozilla-central/rev/92616e983b8ad6e99ec148f341e146c3c6fa312a/xpcom/glue/nsStringAPI.h#530, which eventually finds its way to http://searchfox.org/mozilla-central/rev/92616e983b8ad6e99ec148f341e146c3c6fa312a/xpcom/string/nsTSubstring.cpp#721, which Truncates the string causing it to have a length of 0, making it empty.
https://reviewboard.mozilla.org/r/62732/#review60658

> According to my understanding of our string API, setting a string to Void causes it to truncate to length of 0[1] making it empty anyways, no?
> 
> [1]: Setting Void calls http://searchfox.org/mozilla-central/rev/92616e983b8ad6e99ec148f341e146c3c6fa312a/xpcom/glue/nsStringAPI.h#530, which eventually finds its way to http://searchfox.org/mozilla-central/rev/92616e983b8ad6e99ec148f341e146c3c6fa312a/xpcom/string/nsTSubstring.cpp#721, which Truncates the string causing it to have a length of 0, making it empty.

smaug answered the above in IRC.

While it is true that IsEmpty will return true if a string is Voided, we want to have different behaviours when the string is Void and when the string is Empty. Primarily, because (from what I recall) we act differently if the user passes no string for the features, as opposed to an empty string.
https://reviewboard.mozilla.org/r/58794/#review60758

> There is some very similar looking code in OpenWindowInternal. I think you could move that stuff to a helper.
> Possibly starting from #ifdef MOZ_WIDGET_GONK and ending to return NS_ERROR_ABORT

Going to deduplicate the stuff from this patch in a follow-up patch that I'll put in this series.

> Also width=/height= and persistence handling is the same as in OpenWindowInteral. Some helper method please.

Same as above, re: follow-up patch.
https://reviewboard.mozilla.org/r/58794/#review60758

> Should this be IsVoid(), not IsEmpty()?

IsVoid! Great catch.
https://reviewboard.mozilla.org/r/58800/#review60140

> I don't understand this stuff. Why chrome size depends on content size? Browser chrome contains the content.

These values represent how much of the window is the "chrome". For example, 32 pixels for the toolbars at the top, and perhaps 2px for a window border on the bottom would mean a chromeHeight of 34px.

> Why is this needed? For TabParent ?

Correct!

> Why do we ever want to return the 'normal' size here?

As discussed in IRC, I'm making it so that nsWindowWatcher can detect if there's a primary content area, and if there is not, to manipulate the root docshell directly.

> This looks wrong. Why do we want to set the top level size to something which is meant for the primary content size.

Same as above, regarding the root docshell manipulation.
https://reviewboard.mozilla.org/r/58828/#review59812

> We're trying to remove use of usePrivateBrowsing; let's check that the privateBrowsingId attribute of the docshell's origin attributes is nonzero instead.

Talked to baku and jdm over IRC, and I don't think this is going to work. The top level docShell in newly opened windows doesn't get the private browsing ID set in its origin attributes if we've opened a private window. I think we're stuck checking the load context for now.
Comment on attachment 8761698 [details]
Bug 1261842 - Make initial browser remote sooner if we're defaulting to using remote tabs.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58792/diff/4-5/
Comment on attachment 8768560 [details]
Bug 1261842 - Use nsACString in more places instead of raw strings inside nsWindowWatcher.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62732/diff/2-3/
Comment on attachment 8761699 [details]
Bug 1261842 - Factor out logic for creating windows for content processes from nsWindowWatcher::OpenWindowInternal.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58794/diff/5-6/
Comment on attachment 8761700 [details]
Bug 1261842 - Stop sending the window name to ContentParent when opening a new window.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58796/diff/5-6/
Comment on attachment 8761701 [details]
Bug 1261842 - ContentParent::RecvCreateWindow should always set an nsresult outparam.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58798/diff/5-6/
Comment on attachment 8761702 [details]
Bug 1261842 - Add methods to nsIDocShellTreeOwner for sizing the primary content.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58800/diff/5-6/
Comment on attachment 8761703 [details]
Bug 1261842 - browser_bug495058.js no longer needs to wait for remoteness flip on initial browser of new window.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58802/diff/5-6/
Comment on attachment 8761704 [details]
Bug 1261842 - browser_394759_behavior.js no longer needs to wait for remoteness flip on initial browser of new window.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58804/diff/5-6/
Comment on attachment 8761705 [details]
Bug 1261842 - browser_423132.js no longer needs to wait for remoteness flip on initial browser of new window.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58806/diff/5-6/
Comment on attachment 8761706 [details]
Bug 1261842 - Make sure ExtensionContent.jsm has been loaded in the parent process if Extension.jsm is initialized.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58808/diff/5-6/
Comment on attachment 8761707 [details]
Bug 1261842 - browser_async_window_flushing.js no longer needs to wait for remoteness flip on initial browser of new window.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58810/diff/5-6/
Comment on attachment 8761708 [details]
Bug 1261842 - Update browser_privatebrowsing_downloadLastDir_toggle.js to use add_task and BrowserTestUtils.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58812/diff/6-7/
Comment on attachment 8761709 [details]
Bug 1261842 - Update browser_bug495058.js to account for the initial browser being remote.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58814/diff/6-7/
Comment on attachment 8761710 [details]
Bug 1261842 - Update browser_394759_behavior.js to account for initial browser being remote.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58816/diff/6-7/
Comment on attachment 8761711 [details]
Bug 1261842 - Disable browser_contentAreaClick.js for e10s.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58818/diff/6-7/
Comment on attachment 8761712 [details]
Bug 1261842 - Fix up some private browsing tests to account for initial browsers being remote now.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58820/diff/6-7/
Comment on attachment 8761713 [details]
Bug 1261842 - Make Marionette listener ensure that the loaded document is the one that was actually requested.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58822/diff/6-7/
Comment on attachment 8761714 [details]
Bug 1261842 - Make pageloader assume that initial browsers are remote in e10s mode.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58824/diff/6-7/
Comment on attachment 8761715 [details]
Bug 1261842 - Add a test for chromeflags for new windows from content.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58826/diff/6-7/
Comment on attachment 8761716 [details]
Bug 1261842 - Add a test that ensures that proper chromeFlags and nsILoadContext properties are set for private windows.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58828/diff/6-7/
Comment on attachment 8761717 [details]
Bug 1261842 - When putting the initial tab into the restored background state, flip it to non-remote.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58830/diff/6-7/
Comment on attachment 8761718 [details]
Bug 1261842 - Tests in browser_windowStateContainer.js should wait for new browser windows to completely load.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58832/diff/6-7/
Comment on attachment 8761719 [details]
Bug 1261842 - Make browser_testOpenNewRemoteTabsFromNonRemoteBrowsers.js wait for windows to be ready before sending them places.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58834/diff/6-7/
Comment on attachment 8761699 [details]
Bug 1261842 - Factor out logic for creating windows for content processes from nsWindowWatcher::OpenWindowInternal.

https://reviewboard.mozilla.org/r/58794/#review61926

Hopefully I didn't miss anything. mozreview's interdiff is rather broken.
Attachment #8761699 - Flags: review?(bugs) → review+
Comment on attachment 8761702 [details]
Bug 1261842 - Add methods to nsIDocShellTreeOwner for sizing the primary content.

https://reviewboard.mozilla.org/r/58800/#review61930
Attachment #8761702 - Flags: review?(bugs) → review+
Attachment #8771619 - Flags: review?(bugs) → review+
Comment on attachment 8771619 [details]
Bug 1261842 - Refactor nsWindowWatcher to not have so much duplication.

https://reviewboard.mozilla.org/r/64702/#review61934

thanks for doing this!

::: embedding/components/windowwatcher/nsWindowWatcher.cpp:525
(Diff revision 1)
> +    windowCreator2->CreateChromeWindow2(aParentChrome, aChromeFlags, aContextFlags,
> +                                        aOpeningTabParent, &cancel,
> +                                        getter_AddRefs(newWindowChrome));
> +
> +  if (NS_SUCCEEDED(rv) && cancel) {
> +    newWindowChrome = nullptr; // just in case

This is not really "just in case" since not all the callers check the rv immediately. So, drop the comment.
https://hg.mozilla.org/integration/mozilla-inbound/rev/a94e6a85c93170ef6b40065bb1834167547acd03
Bug 1261842 - Make initial browser remote sooner if we're defaulting to using remote tabs. r=felipe

https://hg.mozilla.org/integration/mozilla-inbound/rev/cbdc0deb92bd5bcbae0be65c7bbd5ad020f7a153
Bug 1261842 - Use nsACString in more places instead of raw strings inside nsWindowWatcher. r=smaug

https://hg.mozilla.org/integration/mozilla-inbound/rev/4cf843a05a68d55d1686d5cf347864563a623f97
Bug 1261842 - Factor out logic for creating windows for content processes from nsWindowWatcher::OpenWindowInternal. r=smaug

https://hg.mozilla.org/integration/mozilla-inbound/rev/e91913cd64b8bebe80c661935adee644f24c6055
Bug 1261842 - Stop sending the window name to ContentParent when opening a new window. r=smaug

https://hg.mozilla.org/integration/mozilla-inbound/rev/e2472a4cc209e0be8a083dd254965598d41f53c6
Bug 1261842 - ContentParent::RecvCreateWindow should always set an nsresult outparam. r=smaug

https://hg.mozilla.org/integration/mozilla-inbound/rev/9d8a382c5b1dcbaa07cb827e86652065f147f368
Bug 1261842 - Add methods to nsIDocShellTreeOwner for sizing the primary content. r=smaug

https://hg.mozilla.org/integration/mozilla-inbound/rev/8d34b61fbfb395d6816c45dcc1f45901e627c478
Bug 1261842 - browser_bug495058.js no longer needs to wait for remoteness flip on initial browser of new window. r=felipe

https://hg.mozilla.org/integration/mozilla-inbound/rev/ce46ebf231c1d679c9bc7abc28f774538b5a1362
Bug 1261842 - browser_394759_behavior.js no longer needs to wait for remoteness flip on initial browser of new window. r=felipe

https://hg.mozilla.org/integration/mozilla-inbound/rev/1240345f5624539c14543dc21888ce1f74f9059b
Bug 1261842 - browser_423132.js no longer needs to wait for remoteness flip on initial browser of new window. r=felipe

https://hg.mozilla.org/integration/mozilla-inbound/rev/855931929739ab4f44ff92f7549b4d80348b580b
Bug 1261842 - Make sure ExtensionContent.jsm has been loaded in the parent process if Extension.jsm is initialized. r=John-Galt

https://hg.mozilla.org/integration/mozilla-inbound/rev/6eaa642a93e1707ae917f9e6f61242ecef36c658
Bug 1261842 - browser_async_window_flushing.js no longer needs to wait for remoteness flip on initial browser of new window. r=felipe

https://hg.mozilla.org/integration/mozilla-inbound/rev/20eb3987a721a2c0a312868cf52dbf2f43563236
Bug 1261842 - Update browser_privatebrowsing_downloadLastDir_toggle.js to use add_task and BrowserTestUtils. r=jdm

https://hg.mozilla.org/integration/mozilla-inbound/rev/b6f7395d2bbff04ba48419d30ebecf848771e9fc
Bug 1261842 - Update browser_bug495058.js to account for the initial browser being remote. r=Gijs

https://hg.mozilla.org/integration/mozilla-inbound/rev/970cbbb3a658336a8a70cfea01ba5a6afa3ae58b
Bug 1261842 - Update browser_394759_behavior.js to account for initial browser being remote. r=mikedeboer

https://hg.mozilla.org/integration/mozilla-inbound/rev/07b259eb7121dde270818a3ca280cd658b83c795
Bug 1261842 - Disable browser_contentAreaClick.js for e10s. r=Gijs

https://hg.mozilla.org/integration/mozilla-inbound/rev/f6062f1139114f586e3ea9f6d946766f63dedb1d
Bug 1261842 - Fix up some private browsing tests to account for initial browsers being remote now. r=jdm

https://hg.mozilla.org/integration/mozilla-inbound/rev/b2e910bb1d726562548eba1148a81ec37300fb7b
Bug 1261842 - Make Marionette listener ensure that the loaded document is the one that was actually requested. r=ato

https://hg.mozilla.org/integration/mozilla-inbound/rev/0a08398b508752250588f9b46726ad90ec51637b
Bug 1261842 - Make pageloader assume that initial browsers are remote in e10s mode. r=jmaher

https://hg.mozilla.org/integration/mozilla-inbound/rev/8d708922e47ac2a5c56a8bceded6a84701536e17
Bug 1261842 - Add a test for chromeflags for new windows from content. r=gabor

https://hg.mozilla.org/integration/mozilla-inbound/rev/968e7b3b73fbc288cdda9b0ba82857065e00d956
Bug 1261842 - Add a test that ensures that proper chromeFlags and nsILoadContext properties are set for private windows. r=jdm

https://hg.mozilla.org/integration/mozilla-inbound/rev/2a1acfd17baf3a269719c70cdd5b273e8b344b41
Bug 1261842 - When putting the initial tab into the restored background state, flip it to non-remote. r=mikedeboer

https://hg.mozilla.org/integration/mozilla-inbound/rev/d6ffc9db53461550f8965e3587244b5b9d68eab6
Bug 1261842 - Tests in browser_windowStateContainer.js should wait for new browser windows to completely load. r=baku

https://hg.mozilla.org/integration/mozilla-inbound/rev/c14ccfac7b4b635cad9f728b85767deef68078be
Bug 1261842 - Make browser_testOpenNewRemoteTabsFromNonRemoteBrowsers.js wait for windows to be ready before sending them places. r=Gijs

https://hg.mozilla.org/integration/mozilla-inbound/rev/bb6c6889b802f469103df297e7c2fb417ec2961d
Bug 1261842 - Refactor nsWindowWatcher to not have so much duplication. r=smaug
Pushed by mconley@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a94e6a85c931
Make initial browser remote sooner if we're defaulting to using remote tabs. r=felipe
https://hg.mozilla.org/integration/mozilla-inbound/rev/cbdc0deb92bd
Use nsACString in more places instead of raw strings inside nsWindowWatcher. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/4cf843a05a68
Factor out logic for creating windows for content processes from nsWindowWatcher::OpenWindowInternal. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/e91913cd64b8
Stop sending the window name to ContentParent when opening a new window. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/e2472a4cc209
ContentParent::RecvCreateWindow should always set an nsresult outparam. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/9d8a382c5b1d
Add methods to nsIDocShellTreeOwner for sizing the primary content. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/8d34b61fbfb3
browser_bug495058.js no longer needs to wait for remoteness flip on initial browser of new window. r=felipe
https://hg.mozilla.org/integration/mozilla-inbound/rev/ce46ebf231c1
browser_394759_behavior.js no longer needs to wait for remoteness flip on initial browser of new window. r=felipe
https://hg.mozilla.org/integration/mozilla-inbound/rev/1240345f5624
browser_423132.js no longer needs to wait for remoteness flip on initial browser of new window. r=felipe
https://hg.mozilla.org/integration/mozilla-inbound/rev/855931929739
Make sure ExtensionContent.jsm has been loaded in the parent process if Extension.jsm is initialized. r=John-Galt
https://hg.mozilla.org/integration/mozilla-inbound/rev/6eaa642a93e1
browser_async_window_flushing.js no longer needs to wait for remoteness flip on initial browser of new window. r=felipe
https://hg.mozilla.org/integration/mozilla-inbound/rev/20eb3987a721
Update browser_privatebrowsing_downloadLastDir_toggle.js to use add_task and BrowserTestUtils. r=jdm
https://hg.mozilla.org/integration/mozilla-inbound/rev/b6f7395d2bbf
Update browser_bug495058.js to account for the initial browser being remote. r=Gijs
https://hg.mozilla.org/integration/mozilla-inbound/rev/970cbbb3a658
Update browser_394759_behavior.js to account for initial browser being remote. r=mikedeboer
https://hg.mozilla.org/integration/mozilla-inbound/rev/07b259eb7121
Disable browser_contentAreaClick.js for e10s. r=Gijs
https://hg.mozilla.org/integration/mozilla-inbound/rev/f6062f113911
Fix up some private browsing tests to account for initial browsers being remote now. r=jdm
https://hg.mozilla.org/integration/mozilla-inbound/rev/b2e910bb1d72
Make Marionette listener ensure that the loaded document is the one that was actually requested. r=ato
https://hg.mozilla.org/integration/mozilla-inbound/rev/0a08398b5087
Make pageloader assume that initial browsers are remote in e10s mode. r=jmaher
https://hg.mozilla.org/integration/mozilla-inbound/rev/8d708922e47a
Add a test for chromeflags for new windows from content. r=gabor
https://hg.mozilla.org/integration/mozilla-inbound/rev/968e7b3b73fb
Add a test that ensures that proper chromeFlags and nsILoadContext properties are set for private windows. r=jdm
https://hg.mozilla.org/integration/mozilla-inbound/rev/2a1acfd17baf
When putting the initial tab into the restored background state, flip it to non-remote. r=mikedeboer
https://hg.mozilla.org/integration/mozilla-inbound/rev/d6ffc9db5346
Tests in browser_windowStateContainer.js should wait for new browser windows to completely load. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/c14ccfac7b4b
Make browser_testOpenNewRemoteTabsFromNonRemoteBrowsers.js wait for windows to be ready before sending them places. r=Gijs
https://hg.mozilla.org/integration/mozilla-inbound/rev/bb6c6889b802
Refactor nsWindowWatcher to not have so much duplication. r=smaug
sorry had to back this out since with this push we have perma failures like https://treeherder.mozilla.org/logviewer.html#?job_id=32055750&repo=mozilla-inbound and in tier 2 like https://treeherder.mozilla.org/logviewer.html#?job_id=32052354&repo=mozilla-inbound
Flags: needinfo?(mconley)
Backout by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ad32bf1994e9
Backed out 25 changesets (bug 1261842, bug 1278985) breaking firefox-ui tests
mconley, I know you are aware of the ts_paint regression this caused, here is the full list of regressions and many improvements:
https://treeherder.mozilla.org/perf.html#/alerts?id=1927
Awesome - glad to see this is going to have the intended consequences. Once bug 1287938 is fixed, that whole list should be super green.
Flags: needinfo?(mconley)
With the initial browser defaulting to remote, there's a greater likelihood
that the DOMContentLoaded event that is handled in the "get" function will
be fired by the initial about:blank instead of the actual desired page.

get() currently works around this by ensuring that the URL of the loaded
page matches the requested one when DOMContentLoaded fires. Unfortunately,
this doesn't work for pages that redirect via their HTTP headers (and will
therefore not fire DOMContentLoaded).

This patch fixes things by adding an nsIWebProgressListener that ensures
that the requested page has started to load before paying any attention
to the DOMContentLoaded events. This handles the redirect case. We also
compare against nsIChannel.originalURI for the about: redirect case.

For neterror pages (which never open channels, and therefore are not
seen by the nsIWebProgressListener), we just check that the page that
we attempted to reach was the one that was requested.

Review commit: https://reviewboard.mozilla.org/r/65646/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/65646/
Attachment #8773000 - Flags: review?(ato)
Comment on attachment 8761698 [details]
Bug 1261842 - Make initial browser remote sooner if we're defaulting to using remote tabs.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58792/diff/5-6/
Comment on attachment 8768560 [details]
Bug 1261842 - Use nsACString in more places instead of raw strings inside nsWindowWatcher.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62732/diff/3-4/
Comment on attachment 8761699 [details]
Bug 1261842 - Factor out logic for creating windows for content processes from nsWindowWatcher::OpenWindowInternal.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58794/diff/6-7/
Comment on attachment 8761700 [details]
Bug 1261842 - Stop sending the window name to ContentParent when opening a new window.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58796/diff/6-7/
Comment on attachment 8761701 [details]
Bug 1261842 - ContentParent::RecvCreateWindow should always set an nsresult outparam.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58798/diff/6-7/
Comment on attachment 8761702 [details]
Bug 1261842 - Add methods to nsIDocShellTreeOwner for sizing the primary content.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58800/diff/6-7/
Comment on attachment 8761703 [details]
Bug 1261842 - browser_bug495058.js no longer needs to wait for remoteness flip on initial browser of new window.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58802/diff/6-7/
Comment on attachment 8761704 [details]
Bug 1261842 - browser_394759_behavior.js no longer needs to wait for remoteness flip on initial browser of new window.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58804/diff/6-7/
Comment on attachment 8761705 [details]
Bug 1261842 - browser_423132.js no longer needs to wait for remoteness flip on initial browser of new window.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58806/diff/6-7/
Comment on attachment 8761706 [details]
Bug 1261842 - Make sure ExtensionContent.jsm has been loaded in the parent process if Extension.jsm is initialized.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58808/diff/6-7/
Comment on attachment 8761707 [details]
Bug 1261842 - browser_async_window_flushing.js no longer needs to wait for remoteness flip on initial browser of new window.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58810/diff/6-7/
Comment on attachment 8761708 [details]
Bug 1261842 - Update browser_privatebrowsing_downloadLastDir_toggle.js to use add_task and BrowserTestUtils.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58812/diff/7-8/
Comment on attachment 8761709 [details]
Bug 1261842 - Update browser_bug495058.js to account for the initial browser being remote.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58814/diff/7-8/
Comment on attachment 8761710 [details]
Bug 1261842 - Update browser_394759_behavior.js to account for initial browser being remote.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58816/diff/7-8/
Comment on attachment 8761711 [details]
Bug 1261842 - Disable browser_contentAreaClick.js for e10s.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58818/diff/7-8/
Comment on attachment 8761712 [details]
Bug 1261842 - Fix up some private browsing tests to account for initial browsers being remote now.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58820/diff/7-8/
Comment on attachment 8761713 [details]
Bug 1261842 - Make Marionette listener ensure that the loaded document is the one that was actually requested.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58822/diff/7-8/
Comment on attachment 8761714 [details]
Bug 1261842 - Make pageloader assume that initial browsers are remote in e10s mode.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58824/diff/7-8/
Comment on attachment 8761715 [details]
Bug 1261842 - Add a test for chromeflags for new windows from content.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58826/diff/7-8/
Comment on attachment 8761716 [details]
Bug 1261842 - Add a test that ensures that proper chromeFlags and nsILoadContext properties are set for private windows.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58828/diff/7-8/
Comment on attachment 8761717 [details]
Bug 1261842 - When putting the initial tab into the restored background state, flip it to non-remote.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58830/diff/7-8/
Comment on attachment 8761718 [details]
Bug 1261842 - Tests in browser_windowStateContainer.js should wait for new browser windows to completely load.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58832/diff/7-8/
Comment on attachment 8761719 [details]
Bug 1261842 - Make browser_testOpenNewRemoteTabsFromNonRemoteBrowsers.js wait for windows to be ready before sending them places.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58834/diff/7-8/
Comment on attachment 8771619 [details]
Bug 1261842 - Refactor nsWindowWatcher to not have so much duplication.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64702/diff/1-2/
Attachment #8773000 - Flags: review?(ato) → review?(dburns)
Comment on attachment 8773000 [details]
Bug 1261842 - Make sure Marionette can handle the redirect and error page cases.

https://reviewboard.mozilla.org/r/65646/#review63256
Attachment #8773000 - Flags: review?(dburns) → review+
Comment on attachment 8761698 [details]
Bug 1261842 - Make initial browser remote sooner if we're defaulting to using remote tabs.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58792/diff/6-7/
Comment on attachment 8768560 [details]
Bug 1261842 - Use nsACString in more places instead of raw strings inside nsWindowWatcher.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62732/diff/4-5/
Comment on attachment 8761699 [details]
Bug 1261842 - Factor out logic for creating windows for content processes from nsWindowWatcher::OpenWindowInternal.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58794/diff/7-8/
Comment on attachment 8761700 [details]
Bug 1261842 - Stop sending the window name to ContentParent when opening a new window.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58796/diff/7-8/
Comment on attachment 8761701 [details]
Bug 1261842 - ContentParent::RecvCreateWindow should always set an nsresult outparam.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58798/diff/7-8/
Comment on attachment 8761702 [details]
Bug 1261842 - Add methods to nsIDocShellTreeOwner for sizing the primary content.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58800/diff/7-8/
Comment on attachment 8761703 [details]
Bug 1261842 - browser_bug495058.js no longer needs to wait for remoteness flip on initial browser of new window.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58802/diff/7-8/
Comment on attachment 8761704 [details]
Bug 1261842 - browser_394759_behavior.js no longer needs to wait for remoteness flip on initial browser of new window.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58804/diff/7-8/
Comment on attachment 8761705 [details]
Bug 1261842 - browser_423132.js no longer needs to wait for remoteness flip on initial browser of new window.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58806/diff/7-8/
Comment on attachment 8761706 [details]
Bug 1261842 - Make sure ExtensionContent.jsm has been loaded in the parent process if Extension.jsm is initialized.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58808/diff/7-8/
Comment on attachment 8761707 [details]
Bug 1261842 - browser_async_window_flushing.js no longer needs to wait for remoteness flip on initial browser of new window.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58810/diff/7-8/
Comment on attachment 8761708 [details]
Bug 1261842 - Update browser_privatebrowsing_downloadLastDir_toggle.js to use add_task and BrowserTestUtils.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58812/diff/8-9/
Comment on attachment 8761709 [details]
Bug 1261842 - Update browser_bug495058.js to account for the initial browser being remote.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58814/diff/8-9/
Comment on attachment 8761710 [details]
Bug 1261842 - Update browser_394759_behavior.js to account for initial browser being remote.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58816/diff/8-9/
Comment on attachment 8761711 [details]
Bug 1261842 - Disable browser_contentAreaClick.js for e10s.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58818/diff/8-9/
Comment on attachment 8761712 [details]
Bug 1261842 - Fix up some private browsing tests to account for initial browsers being remote now.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58820/diff/8-9/
Comment on attachment 8761713 [details]
Bug 1261842 - Make Marionette listener ensure that the loaded document is the one that was actually requested.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58822/diff/8-9/
Comment on attachment 8761714 [details]
Bug 1261842 - Make pageloader assume that initial browsers are remote in e10s mode.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58824/diff/8-9/
Comment on attachment 8761715 [details]
Bug 1261842 - Add a test for chromeflags for new windows from content.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58826/diff/8-9/
Comment on attachment 8761716 [details]
Bug 1261842 - Add a test that ensures that proper chromeFlags and nsILoadContext properties are set for private windows.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58828/diff/8-9/
Comment on attachment 8761717 [details]
Bug 1261842 - When putting the initial tab into the restored background state, flip it to non-remote.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58830/diff/8-9/
Comment on attachment 8761718 [details]
Bug 1261842 - Tests in browser_windowStateContainer.js should wait for new browser windows to completely load.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58832/diff/8-9/
Comment on attachment 8761719 [details]
Bug 1261842 - Make browser_testOpenNewRemoteTabsFromNonRemoteBrowsers.js wait for windows to be ready before sending them places.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58834/diff/8-9/
Comment on attachment 8771619 [details]
Bug 1261842 - Refactor nsWindowWatcher to not have so much duplication.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64702/diff/2-3/
Comment on attachment 8773000 [details]
Bug 1261842 - Make sure Marionette can handle the redirect and error page cases.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65646/diff/1-2/
Comment on attachment 8761698 [details]
Bug 1261842 - Make initial browser remote sooner if we're defaulting to using remote tabs.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58792/diff/7-8/
Comment on attachment 8768560 [details]
Bug 1261842 - Use nsACString in more places instead of raw strings inside nsWindowWatcher.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62732/diff/5-6/
Comment on attachment 8761699 [details]
Bug 1261842 - Factor out logic for creating windows for content processes from nsWindowWatcher::OpenWindowInternal.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58794/diff/8-9/
Comment on attachment 8761700 [details]
Bug 1261842 - Stop sending the window name to ContentParent when opening a new window.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58796/diff/8-9/
Comment on attachment 8761701 [details]
Bug 1261842 - ContentParent::RecvCreateWindow should always set an nsresult outparam.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58798/diff/8-9/
Comment on attachment 8761702 [details]
Bug 1261842 - Add methods to nsIDocShellTreeOwner for sizing the primary content.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58800/diff/8-9/
Comment on attachment 8761703 [details]
Bug 1261842 - browser_bug495058.js no longer needs to wait for remoteness flip on initial browser of new window.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58802/diff/8-9/
Comment on attachment 8761704 [details]
Bug 1261842 - browser_394759_behavior.js no longer needs to wait for remoteness flip on initial browser of new window.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58804/diff/8-9/
Comment on attachment 8761705 [details]
Bug 1261842 - browser_423132.js no longer needs to wait for remoteness flip on initial browser of new window.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58806/diff/8-9/
Comment on attachment 8761706 [details]
Bug 1261842 - Make sure ExtensionContent.jsm has been loaded in the parent process if Extension.jsm is initialized.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58808/diff/8-9/
Comment on attachment 8761707 [details]
Bug 1261842 - browser_async_window_flushing.js no longer needs to wait for remoteness flip on initial browser of new window.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58810/diff/8-9/
Comment on attachment 8761708 [details]
Bug 1261842 - Update browser_privatebrowsing_downloadLastDir_toggle.js to use add_task and BrowserTestUtils.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58812/diff/9-10/
Comment on attachment 8761709 [details]
Bug 1261842 - Update browser_bug495058.js to account for the initial browser being remote.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58814/diff/9-10/
Comment on attachment 8761710 [details]
Bug 1261842 - Update browser_394759_behavior.js to account for initial browser being remote.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58816/diff/9-10/
Comment on attachment 8761711 [details]
Bug 1261842 - Disable browser_contentAreaClick.js for e10s.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58818/diff/9-10/
Comment on attachment 8761712 [details]
Bug 1261842 - Fix up some private browsing tests to account for initial browsers being remote now.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58820/diff/9-10/
Comment on attachment 8761713 [details]
Bug 1261842 - Make Marionette listener ensure that the loaded document is the one that was actually requested.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58822/diff/9-10/
Comment on attachment 8761714 [details]
Bug 1261842 - Make pageloader assume that initial browsers are remote in e10s mode.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58824/diff/9-10/
Comment on attachment 8761715 [details]
Bug 1261842 - Add a test for chromeflags for new windows from content.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58826/diff/9-10/
Comment on attachment 8761716 [details]
Bug 1261842 - Add a test that ensures that proper chromeFlags and nsILoadContext properties are set for private windows.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58828/diff/9-10/
Comment on attachment 8761717 [details]
Bug 1261842 - When putting the initial tab into the restored background state, flip it to non-remote.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58830/diff/9-10/
Comment on attachment 8761718 [details]
Bug 1261842 - Tests in browser_windowStateContainer.js should wait for new browser windows to completely load.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58832/diff/9-10/
Comment on attachment 8761719 [details]
Bug 1261842 - Make browser_testOpenNewRemoteTabsFromNonRemoteBrowsers.js wait for windows to be ready before sending them places.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58834/diff/9-10/
Comment on attachment 8771619 [details]
Bug 1261842 - Refactor nsWindowWatcher to not have so much duplication.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64702/diff/3-4/
Comment on attachment 8773000 [details]
Bug 1261842 - Make sure Marionette can handle the redirect and error page cases.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65646/diff/2-3/
Pushed by mconley@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/454b3874c934
Make initial browser remote sooner if we're defaulting to using remote tabs. r=Felipe
https://hg.mozilla.org/integration/autoland/rev/755a1fc3e2f3
Use nsACString in more places instead of raw strings inside nsWindowWatcher. r=smaug
https://hg.mozilla.org/integration/autoland/rev/e3c3f9d77885
Factor out logic for creating windows for content processes from nsWindowWatcher::OpenWindowInternal. r=smaug
https://hg.mozilla.org/integration/autoland/rev/831918218f92
Stop sending the window name to ContentParent when opening a new window. r=smaug
https://hg.mozilla.org/integration/autoland/rev/2fbe723f054e
ContentParent::RecvCreateWindow should always set an nsresult outparam. r=smaug
https://hg.mozilla.org/integration/autoland/rev/b45911deab9b
Add methods to nsIDocShellTreeOwner for sizing the primary content. r=smaug
https://hg.mozilla.org/integration/autoland/rev/9d70d39a5ebb
browser_bug495058.js no longer needs to wait for remoteness flip on initial browser of new window. r=Felipe,Gijs
https://hg.mozilla.org/integration/autoland/rev/fd8faeae82a4
browser_394759_behavior.js no longer needs to wait for remoteness flip on initial browser of new window. r=Felipe
https://hg.mozilla.org/integration/autoland/rev/afbc7b46955f
browser_423132.js no longer needs to wait for remoteness flip on initial browser of new window. r=Felipe
https://hg.mozilla.org/integration/autoland/rev/48fe1d6332b3
Make sure ExtensionContent.jsm has been loaded in the parent process if Extension.jsm is initialized. r=kmag
https://hg.mozilla.org/integration/autoland/rev/715de7740db1
browser_async_window_flushing.js no longer needs to wait for remoteness flip on initial browser of new window. r=Felipe
https://hg.mozilla.org/integration/autoland/rev/e9090cf2576f
Update browser_privatebrowsing_downloadLastDir_toggle.js to use add_task and BrowserTestUtils. r=jdm
https://hg.mozilla.org/integration/autoland/rev/ee23a9c69693
Update browser_bug495058.js to account for the initial browser being remote. r=Gijs
https://hg.mozilla.org/integration/autoland/rev/f5427c4704bb
Update browser_394759_behavior.js to account for initial browser being remote. r=mikedeboer
https://hg.mozilla.org/integration/autoland/rev/bcb1a7b1d60e
Disable browser_contentAreaClick.js for e10s. r=Gijs
https://hg.mozilla.org/integration/autoland/rev/c4ede6a823d3
Fix up some private browsing tests to account for initial browsers being remote now. r=jdm
https://hg.mozilla.org/integration/autoland/rev/88b8a2529323
Make Marionette listener ensure that the loaded document is the one that was actually requested. r=ato
https://hg.mozilla.org/integration/autoland/rev/645432ac4faa
Make pageloader assume that initial browsers are remote in e10s mode. r=jmaher
https://hg.mozilla.org/integration/autoland/rev/d32102031a7f
Add a test for chromeflags for new windows from content. r=krizsa
https://hg.mozilla.org/integration/autoland/rev/ee193eb98d2f
Add a test that ensures that proper chromeFlags and nsILoadContext properties are set for private windows. r=jdm
https://hg.mozilla.org/integration/autoland/rev/402b6b26d2eb
When putting the initial tab into the restored background state, flip it to non-remote. r=mikedeboer
https://hg.mozilla.org/integration/autoland/rev/cf6ffd4bc022
Tests in browser_windowStateContainer.js should wait for new browser windows to completely load. r=baku
https://hg.mozilla.org/integration/autoland/rev/cb6168f1e841
Make browser_testOpenNewRemoteTabsFromNonRemoteBrowsers.js wait for windows to be ready before sending them places. r=Gijs
https://hg.mozilla.org/integration/autoland/rev/20c3ad980107
Refactor nsWindowWatcher to not have so much duplication. r=smaug
https://hg.mozilla.org/integration/autoland/rev/80ab1089a326
Make sure Marionette can handle the redirect and error page cases. r=automatedtester
https://hg.mozilla.org/mozilla-central/rev/454b3874c934
https://hg.mozilla.org/mozilla-central/rev/755a1fc3e2f3
https://hg.mozilla.org/mozilla-central/rev/e3c3f9d77885
https://hg.mozilla.org/mozilla-central/rev/831918218f92
https://hg.mozilla.org/mozilla-central/rev/2fbe723f054e
https://hg.mozilla.org/mozilla-central/rev/b45911deab9b
https://hg.mozilla.org/mozilla-central/rev/9d70d39a5ebb
https://hg.mozilla.org/mozilla-central/rev/fd8faeae82a4
https://hg.mozilla.org/mozilla-central/rev/afbc7b46955f
https://hg.mozilla.org/mozilla-central/rev/48fe1d6332b3
https://hg.mozilla.org/mozilla-central/rev/715de7740db1
https://hg.mozilla.org/mozilla-central/rev/e9090cf2576f
https://hg.mozilla.org/mozilla-central/rev/ee23a9c69693
https://hg.mozilla.org/mozilla-central/rev/f5427c4704bb
https://hg.mozilla.org/mozilla-central/rev/bcb1a7b1d60e
https://hg.mozilla.org/mozilla-central/rev/c4ede6a823d3
https://hg.mozilla.org/mozilla-central/rev/88b8a2529323
https://hg.mozilla.org/mozilla-central/rev/645432ac4faa
https://hg.mozilla.org/mozilla-central/rev/d32102031a7f
https://hg.mozilla.org/mozilla-central/rev/ee193eb98d2f
https://hg.mozilla.org/mozilla-central/rev/402b6b26d2eb
https://hg.mozilla.org/mozilla-central/rev/cf6ffd4bc022
https://hg.mozilla.org/mozilla-central/rev/cb6168f1e841
https://hg.mozilla.org/mozilla-central/rev/20c3ad980107
https://hg.mozilla.org/mozilla-central/rev/80ab1089a326
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
here are all the alerts for this change:
https://treeherder.mozilla.org/perf.html#/alerts?id=2019

Summary of tests that regressed:

  ts_paint linux64 opt e10s - 25.13% worse
  ts_paint windows7-32 opt e10s - 24.92% worse
  ts_paint windows8-64 opt e10s - 27.43% worse
  tsvgx summary linux64 pgo e10s - 2.22% worse

Summary of tests that improved:

  tpaint linux64 opt e10s - 3.55% better
  sessionrestore linux64 opt e10s - 5.31% better
  sessionrestore_no_auto_restore linux64 opt e10s - 4.79% better
  tp5o responsiveness linux64 opt e10s - 11.68% better
  tpaint windows7-32 opt e10s - 11.21% better
  sessionrestore_no_auto_restore windows7-32 opt e10s - 6.86% better
  tpaint windows8-64 opt e10s - 9.17% better
  sessionrestore windows8-64 opt e10s - 7.54% better
  sessionrestore_no_auto_restore windows8-64 opt e10s - 7.35% better
  sessionrestore windows7-32 opt e10s - 6.25% better
See Also: → 1289903
Blocks: 1289571
Depends on: 1290280
Depends on: 1290184
Blocks: 1290567
No longer depends on: 1290280
Depends on: 1290280
Depends on: 1291860
Assignee: nobody → mconley
Depends on: 1352413
Depends on: 1353738
Depends on: 1350197
You need to log in before you can comment on or make changes to this bug.