1121791 - [e10s] window size is calculated differently in e10s vs non-e10s

Status: RESOLVED FIXED

(Core :: DOM: Content Processes, defect)

Description

[George Wright (:gw280) (needinfo me!)]

Attachment: e10s vs non-e10s window sizes (200x100 specified)

Using the testcase at http://www.w3schools.com/jsref/tryit.asp?filename=tryjsref_win_moveto I see a difference in the size of the window created by the line:

    myWindow=window.open("", "myWindow", "width=200, height=100");

See the attached screenshot for the discrepancy (e10s on the left, non-e10s on the right). This is blocking bug 1082127.

Comment 1

[George Wright (:gw280) (needinfo me!)]

Attachment: e10s vs non-e10s window sizes (200x100 specified)

Moved the ruler to make it more visually obvious what the issue is.

Comment 2

[George Wright (:gw280) (needinfo me!)]

Attachment: 0001-Bug-1121791-Accommodate-chrome-sizes-when-sizing-e10.patch

So I think I'm getting closer to working out what's going on here; basically, we're doing a chrome size calculation [0], and the two values are returned the same and so the chrome width/height is calculated as 0. More bizarrely, the two values being returned are always for the entire window, including the chrome, even when I've explicitly checked what I believe to be the content DocShell's size against the XUL window's size. The only way I can find to get the "true" content size is to query the TabParent directly.

So that's what this patch does, but there's a further complication. The shell window that we query in [0] is for the parent window, not the popup, so when we try to calculate what the chrome padding should be, we actually end up with the value for the parent window that spawned the popup (which has a tab bar in this case, so the chromeHeight comes back as ~71 instead of ~28 like the popup's should be). Further, I don't understand what's going on at [1] where we seem to just ignore the values of the chrome size if both sizeChromeWidth and sizeChromeHeight are false.

I also looked into using the SizeConstraint on the frame as that seems to be set to the size of the chrome + 1x1, but I see no easy way to query that constraint within nsWindowWatcher, and I think that querying the relevant TabParent for its size and then doing the same subtraction trick is the correct solution here, based on its similarity to the existing code.

:smaug - do you have any further pointers on where to go with this? I feel like I'm trapped in a rat's nest of code and could do with some guidance here!

[0] - http://mxr.mozilla.org/mozilla-central/source/embedding/components/windowwatcher/nsWindowWatcher.cpp#1937
[1] - http://mxr.mozilla.org/mozilla-central/source/embedding/components/windowwatcher/nsWindowWatcher.cpp#2064

Comment 3

[George Wright (:gw280) (needinfo me!)]

(Just to be clear, this patch doesn't currently work)

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

(In reply to George Wright (:gw280) from comment #2)
> Created attachment 8557251 [details] [diff] [review]
> 0001-Bug-1121791-Accommodate-chrome-sizes-when-sizing-e10.patch
> 
> So I think I'm getting closer to working out what's going on here;
> basically, we're doing a chrome size calculation [0], and the two values are
> returned the same and so the chrome width/height is calculated as 0. More
> bizarrely, the two values being returned are always for the entire window,
> including the chrome, even when I've explicitly checked what I believe to be
> the content DocShell's size against the XUL window's size.
Not sure what this means. There is no content docshell in the parent.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8557251 [details] [diff] [review]
0001-Bug-1121791-Accommodate-chrome-sizes-when-sizing-e10.patch

You need to update uuid of nsITabParent

> 
>   void setIsDocShellActive(in bool aIsActive);
>+
>+  void getSize(out int32_t cx, out int32_t cy);
I'd call the params w and h

>+TabParent::GetSize(int32_t* aCx, int32_t* aCy)
>+{
>+  if (*aCx) {
>+    *aCx = mDimensions.width;
>+  }
>+  if (*aCy) {
>+    *aCy = mDimensions.height;
>+  }
>+  return NS_OK;
>+}

... especially given what is done here.


>+
> nsWindowWatcher::SizeOpenedDocShellItem(nsIDocShellTreeItem *aDocShellItem,
>                                         nsIDOMWindow *aParent,
>+                                        nsITabParent *aTabParent,
>                                         bool aIsCallerChrome,
>                                         const SizeSpec & aSizeSpec)
> {
>@@ -1927,7 +1928,12 @@ nsWindowWatcher::SizeOpenedDocShellItem(nsIDocShellTreeItem *aDocShellItem,
>   top = NSToIntRound(top / scale);
>   width = NSToIntRound(width / scale);
>   height = NSToIntRound(height / scale);
>-  { // scope shellWindow why not
>+  if (aTabParent) {
>+    int32_t cox, coy;
>+    aTabParent->GetSize(&cox, &coy);
>+    chromeWidth = width - cox;
>+    chromeHeight = height - coy;
>+  } else { // scope shellWindow why not
Yes, this is something I was expecting us to miss, not dealing TabParent here



We should get some tests here.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

(In reply to George Wright (:gw280) from comment #2)
> So that's what this patch does, but there's a further complication. The
> shell window that we query in [0] is for the parent window, not the popup,
> so when we try to calculate what the chrome padding should be, we actually
> end up with the value for the parent window that spawned the popup (which
> has a tab bar in this case, so the chromeHeight comes back as ~71 instead of
> ~28 like the popup's should be).
Hmm, so how does this work in non-e10s?

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Perhaps not super useful answers, but one needs to "just" go through the code to see what is happening.

Comment 8

[George Wright (:gw280) (needinfo me!)]

I've been hammering through this with :mconley today and we got far enough to realise that his patch to bug 1047603 will actually fix this issue, so I think we can mark this as a dependency of that.

Comment 9

[George Wright (:gw280) (needinfo me!)]

Confirmed that this is now fixed in nightly.