Resizing windows from JS is not supported in e10s mode

RESOLVED FIXED in Firefox 48

Status

()

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: Ehsan, Assigned: gkrizsanits)

Tracking

(Blocks: 1 bug)

unspecified
mozilla49
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(e10sm9+, firefox48 fixed, firefox49 fixed)

Details

(Whiteboard: btpp-active)

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

3 years ago
Doing things such as setting window.innerWidth, window.innerHeight, or calling window.sizeToContent() doesn't work in e10s mode, since TabChild::SizeBrowserTo() is unimplemented.

This is the reason why test_resize_move_windows.html and test_sizetocontent_clamp.html are currently disabled in e10s mode.
Assignee: nobody → mrbkap
tracking-e10s: ? → m9+
baku's going to take this.
Assignee: mrbkap → amarchesini
Depends on: 1257105
:baku thanks for taking this. Is there an ETA yet?
Flags: needinfo?(amarchesini)
Created attachment 8736300 [details] [diff] [review]
WIP
Flags: needinfo?(amarchesini)
(Assignee)

Comment 4

3 years ago
(In reply to Erin Lancaster [:elan] from comment #2)
> :baku thanks for taking this. Is there an ETA yet?

I talked to Andrea, he is working on it and he thinks he is half way there, I'm going to look into this and see if I can help him out to speed things up if it's possible.
(Assignee)

Comment 5

3 years ago
(In reply to :Ehsan Akhgari (busy) from comment #0)
> calling window.sizeToContent() doesn't work in e10s mode, since
> TabChild::SizeBrowserTo() is unimplemented.

SizeBrowserTo does not seem to be implemented in non-e10s mode, SizeShellTo seems to do the heavy lifting instead, so that should trigger the message we send to the parent to do the work (or if that does not work then its caller)
(Reporter)

Comment 6

3 years ago
I was basing my assessment on the stdout output.  :-)
(Assignee)

Updated

3 years ago
Attachment #8736300 - Attachment is patch: true
Whiteboard: btpp-active
(Assignee)

Comment 7

3 years ago
I talked to Andrea and he does not mind if I'm stealing this from him as I'm working on it already.
Assignee: amarchesini → gkrizsanits
(Assignee)

Updated

3 years ago
Depends on: 1258925
No longer depends on: 1257105
(Assignee)

Comment 8

3 years ago
I think I fixed all the APIs but CanMoveResizeWindows is tricky with e10s, so let's tackle that problem first (it's tracked under 1258925)
Just wanted to mention that in the e10s-tests spreadsheets there are two tests that are disabled for e10s and are tracked by this bug:

dom/tests/mochitest/bugs/mochitest.ini:
  test_resize_move_windows.html
  test_sizetocontent_clamp.html
(Assignee)

Comment 12

3 years ago
This test drives me crazy... crossing fingers that I fixed everything this time: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c4fcb2884f8d
(Assignee)

Comment 13

3 years ago
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #12)
> This test drives me crazy... crossing fingers that I fixed everything this
> time: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c4fcb2884f8d

OK, apart from the unrelated bustage this looks quite good. The timeout on linux is because of the incorrect fix in the test for linux. It's a known issue already without e10s and with e10s the random effect that this test comes with is a bit different on linux. I'm going to turn off the test for linux and file a followup, but that should not block e10s.
(Assignee)

Comment 14

3 years ago
Created attachment 8744247 [details] [diff] [review]
window size and position manipulation from JS for e10s. v1

Mike, is this something you feel comfortable reviewing? I'm going to add some comments about what this patch does and why in a minute.
Attachment #8744247 - Flags: feedback?(mconley)
(Assignee)

Comment 15

3 years ago
Comment on attachment 8744247 [details] [diff] [review]
window size and position manipulation from JS for e10s. v1

Review of attachment 8744247 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/ipc/TabChild.cpp
@@ +1021,5 @@
> +  // change we want the parent to ignore its value. For that we will use a special
> +  // value: INT32_MIN;
> +  int32_t x, y, cx, cy;
> +  GetDimensions(aFlags, &x, &y, &cx, &cy);
> +  SetOrReset(x, aX);

I'm not proud of this hack. I use INT32_MIN for dimensions that did not change. Various APIs are calling this for changing only some of the dimensions and use the old value for the rest. But with e10s that old value might not be updated yet, so we cannot use them. Problem is that the parent process is in control for the actual resizing of the window. So we send a resize request to the parent and it does the resize and update us about the change. Sending a second request before we get the update could reset some values unintentionally.

We could send different messages, or a flag that tells which values are to be ignored. Neither of those approaches looked better in practice :(

::: dom/tests/mochitest/bugs/test_resize_move_windows.html
@@ -238,5 @@
>      isnot(aWindow.outerWidth, oWidth, "Window outerWidth should have changed");
>      isnot(aWindow.outerHeight, oHeight, "Window outerHeight should have changed");
> -
> -    aWindow.outerWidth = oWidth;
> -    aWindow.outerHeight = oHeight;

This test is sneaky. Usually we set some of the dimensions and then spin the event loop until we can detect the changes. But here we change the dimensions after the test and not waiting for the changes, which can mess up the upcoming test (storing not updated current dimensions and calling GetNewHeight with not updated dimensions). It worth nothing that because of the weird math in GetNewHeight like functions it can happen that we end up with a small enough window that sizeContentTo changes only one of the dimensions and that causes the test to time out.

The other evil twist is that for linux we let through some tests when one of the dimensions changes only, which can confuse everything in the test and only a lucky coincidence that the test do not fail because of it (most of the time).

::: dom/tests/mochitest/bugs/test_sizetocontent_clamp.html
@@ -52,4 @@
>        ok(w.innerHeight + epsilon >= 100 && w.innerHeight - epsilon <= 100,
> -         "innerHeight should be around 100");
> -
> -      // It's not clear why 2 events are coming...

We used to get one resize event before the one that actually matters. I think it's from the creation of the window. And with e10s now we get 2. I don't consider this a huge problem based on the comment.

::: embedding/browser/nsDocShellTreeOwner.cpp
@@ +593,5 @@
>  nsDocShellTreeOwner::SetPositionDesktopPix(int32_t aX, int32_t aY)
>  {
>    if (mWebBrowser) {
> +    nsresult rv = mWebBrowser->SetPositionDesktopPix(aX, aY);
> +    NS_ENSURE_SUCCESS(rv, rv);

I definitely need to call the SetPosition in the e10s case here (see below) to update the puppet widget that will update the parent process about the changes. This does not seem to be breaking anything in the non-e10s case, but I can totally imagine that we had a reason to return here and this will cause some trouble. I need some confirmation about this from someone with deep docshell knowledge. I can ofc check if we're in a child process to mitigate the risk.

::: xpfe/appshell/nsXULWindow.cpp
@@ +2180,5 @@
>    mXULBrowserWindow = aXULBrowserWindow;
>    return NS_OK;
>  }
>  
> +void nsXULWindow::SizeShellToWithLimit(int32_t aCX, int32_t aCY,

For sizeShellTo we use the incoming aShellItem to limit the resize. But the window we resize is in the parent process, the aShellItem in the child, so when I send the request messages I include the size of the aShellItem, which is if I got it right _should_ be valid always. Fingers crossed... I really need someone to confirm this.
(Assignee)

Comment 16

3 years ago
Baku, asked me to needinfo him so he can take a look at the patch when I'm done.
Flags: needinfo?(amarchesini)
Attachment #8744247 - Flags: feedback?(mconley) → feedback+
(Assignee)

Updated

3 years ago
Attachment #8744247 - Flags: feedback+ → feedback?(mconley)
Comment on attachment 8744247 [details] [diff] [review]
window size and position manipulation from JS for e10s. v1

Review of attachment 8744247 [details] [diff] [review]:
-----------------------------------------------------------------

This is actually semi-related to work I'm doing in bug 1261842, where when the content process opens a new window, the parent needs to size it correctly based on the height and width window features that window.open was called with, but with bug 1261842, we lose access to the temporary initial browser docShell which made resizing it properly so easy.

So, give me some time to study this. Window opening and sizing is crazy stuff. I'll hopefully have some feedback for you on Monday.
Flags: needinfo?(amarchesini)
Comment on attachment 8744247 [details] [diff] [review]
window size and position manipulation from JS for e10s. v1

Review of attachment 8744247 [details] [diff] [review]:
-----------------------------------------------------------------

I really just have a question here, mostly about the scheme you're using to handle dimensions that haven't changed. I apologize if my questions seem obvious.

::: dom/ipc/TabChild.cpp
@@ +1012,5 @@
>  {
> +  // The parent is in charge of the dimension changes. If JS code wants to
> +  // change the dimensions (moveTo, screenX, etc.) we send a message to the
> +  // parent about the new requested dimension, the parent does the resize/move
> +  // then send a message to the child to update itself. For APIs like screenX

I'm confused... window.screenX is read-only. Or is this some API I'm not familiar with that sets dimensions somehow?

@@ +1021,5 @@
> +  // change we want the parent to ignore its value. For that we will use a special
> +  // value: INT32_MIN;
> +  int32_t x, y, cx, cy;
> +  GetDimensions(aFlags, &x, &y, &cx, &cy);
> +  SetOrReset(x, aX);

Can we not just cache the child's notions of the dimensions? Like, suppose we have a window that's 500x500.

Child sends a message to resize to 250x250, and its internal cache of dimensions updates immediately. Parent starts to resize window.

Meanwhile, child resizes window to 150x250. Since internally, the child knows that only one dimension changed, it sends just the one dimension to change.

Or am I totally misunderstanding what's going on here?

::: xpfe/appshell/nsXULWindow.cpp
@@ +2180,5 @@
>    mXULBrowserWindow = aXULBrowserWindow;
>    return NS_OK;
>  }
>  
> +void nsXULWindow::SizeShellToWithLimit(int32_t aCX, int32_t aCY,

From my understanding, I think you're right, regarding the child knowing its own size already.
(Assignee)

Comment 19

3 years ago
(In reply to Mike Conley (:mconley) - Needinfo me! from comment #18)
> Comment on attachment 8744247 [details] [diff] [review]
> I'm confused... window.screenX is read-only. Or is this some API I'm not
> familiar with that sets dimensions somehow?

I'd wish. It's a tricky area... see: http://hg.mozilla.org/mozilla-central/annotate/cfc7ebe59293/dom/webidl/Window.webidl#l210

I have not done much research on the story, just wanted to keep the already existing behavior and pass the test to be honest.

> 
> @@ +1021,5 @@
> > +  // change we want the parent to ignore its value. For that we will use a special
> > +  // value: INT32_MIN;
> > +  int32_t x, y, cx, cy;
> > +  GetDimensions(aFlags, &x, &y, &cx, &cy);
> > +  SetOrReset(x, aX);
> 
> Can we not just cache the child's notions of the dimensions? Like, suppose
> we have a window that's 500x500.
> 
> Child sends a message to resize to 250x250, and its internal cache of
> dimensions updates immediately. Parent starts to resize window.
> 
> Meanwhile, child resizes window to 150x250. Since internally, the child
> knows that only one dimension changed, it sends just the one dimension to
> change.
> 
> Or am I totally misunderstanding what's going on here?
> 

I think you're understanding it perfectly. That cache is PuppetWidget.mBounds and exists already, and that's exactly the logic I'm relying on.
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #15)
> Comment on attachment 8744247 [details] [diff] [review]
> window size and position manipulation from JS for e10s. v1
> 
> Review of attachment 8744247 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/ipc/TabChild.cpp
> @@ +1021,5 @@
> > +  // change we want the parent to ignore its value. For that we will use a special
> > +  // value: INT32_MIN;
> > +  int32_t x, y, cx, cy;
> > +  GetDimensions(aFlags, &x, &y, &cx, &cy);
> > +  SetOrReset(x, aX);
> 
> I'm not proud of this hack. I use INT32_MIN for dimensions that did not
> change. Various APIs are calling this for changing only some of the
> dimensions and use the old value for the rest. But with e10s that old value
> might not be updated yet, so we cannot use them. Problem is that the parent
> process is in control for the actual resizing of the window. So we send a
> resize request to the parent and it does the resize and update us about the
> change. Sending a second request before we get the update could reset some
> values unintentionally.
> 
> We could send different messages, or a flag that tells which values are to
> be ignored. Neither of those approaches looked better in practice :(
> 

I guess this is the part I'm raising my eyebrow at the most.

Here's the problem as I understand it:

Child sends a message to resize a window with some dimensions.

Parent receives the message and starts doing the resize.

Child, meanwhile, sends a second message to resize the window again - perhaps just changing the width. However, since it hasn't been informed by the parent that the resize went through, it's notion on _height_ is still the old value, so when the resize message goes up to the parent, it sends that old height value.

Parent receives the second message with the height at the old value, and now we have a window that's incorrectly sized, height-wise, because the height is the original value it was at before we had started.

If the above is the scenario we're attempting to solve, how come we're waiting for the parent to update us with new dimensions? Can't we just assume synchronously in the child that the resize went through until told otherwise?
Flags: needinfo?(gkrizsanits)
(Assignee)

Comment 21

3 years ago
(In reply to Mike Conley (:mconley) - Needinfo me! from comment #20)
> (In reply to Gabor Krizsanits [:krizsa :gabor] from comment #15)
> I guess this is the part I'm raising my eyebrow at the most.

No surprise there.

> If the above is the scenario we're attempting to solve, how come we're
> waiting for the parent to update us with new dimensions? Can't we just
> assume synchronously in the child that the resize went through until told
> otherwise?

Yes that's the scenario. If I get it right you would update the cached dimensions (PuppetWidget::mBounds) right away. Right now if you do:

[1] w.innerHeight = 100;
[2] w.innerHeight -> returns the old height until we spin the event loop and the resize actually happens and we also get a resize event (OK I'm not sure if innerHeigh is really always async but some of these APIs are)

if we update mBounds right away w.innerHeight at [2] will return 100 right away even though there was no resize event fired yet nor was the actual window resized yet.

then the parent gets the request and decides that instead of 100 the new size should be 150 and sends the message about the new dimensions, and then we have to change the value innerHeight again (so in the original case innerHeight never returned 100).

Honestly? I don't know if this behavior change will break a lot more things than this test, but would rather not risk it. We could keep around a second cache, but would not bother with that either, it sounds like more complexity.

Does that answer your questions / make sense?
Flags: needinfo?(gkrizsanits)
Comment on attachment 8744247 [details] [diff] [review]
window size and position manipulation from JS for e10s. v1

Review of attachment 8744247 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/ipc/PBrowser.ipdl
@@ +156,5 @@
>       * the next or previous focusable element or document.
>       */
>      async MoveFocus(bool forward, bool forDocumentNavigation);
>  
> +    async SizeShellTo(int32_t aWidth, int32_t aHeight,

Worth documenting.

::: dom/ipc/TabChild.cpp
@@ +1021,5 @@
> +  // change we want the parent to ignore its value. For that we will use a special
> +  // value: INT32_MIN;
> +  int32_t x, y, cx, cy;
> +  GetDimensions(aFlags, &x, &y, &cx, &cy);
> +  SetOrReset(x, aX);

I think it's more common to return values for things like this rather than altering the value of one of the arguments.

Tell me what you think of this - how about we extend the flags being passed back and forth here to encode which values the child is attempting to change? That way, the parent knows which ones it can ignore without a magic value.

::: dom/ipc/TabParent.cpp
@@ +852,5 @@
> +  treeOwnerAsWin->GetSize(&cx, &cy);
> +  SetOrIgnore(x, aX);
> +  SetOrIgnore(y, aY);
> +  SetOrIgnore(cx, aCx);
> +  SetOrIgnore(cy, aCy);

As before, perhaps return a value instead of altering the argument.

::: embedding/browser/nsDocShellTreeOwner.cpp
@@ +458,5 @@
>    if (aShellItem == mWebBrowser->mDocShell) {
> +    nsCOMPtr<nsITabChild> tabChild = do_QueryInterface(webBrowserChrome);
> +    if (tabChild) {
> +      // The XUL window to resize is in the parent process, but there we
> +      // won't be able to get aShellItem to do the hack in nsXULWindow::SizeShellTo,

It occurs to me (and this is similar to the stuff I'm struggling with in bug 1261842, which made me think of it) that the parent can infer the size of the shell by measuring the size of the remote browser element, can it not?

Like, the TabParent has a frameelement, which (I believe) we can resolve to a browser, and from that, I think we can get its dimensions. Would that not suffice?

@@ +466,5 @@
> +
> +      int32_t width = 0;
> +      int32_t height = 0;
> +      shellAsWin->GetSize(&width, &height);
> +      tabChild->RemoteSizeShellTo(aCX, aCY, width, height);

Probably should return the result of RemoteSizeShellTo.

@@ +593,5 @@
>  nsDocShellTreeOwner::SetPositionDesktopPix(int32_t aX, int32_t aY)
>  {
>    if (mWebBrowser) {
> +    nsresult rv = mWebBrowser->SetPositionDesktopPix(aX, aY);
> +    NS_ENSURE_SUCCESS(rv, rv);

That change is actually quite recent (https://hg.mozilla.org/mozilla-central/rev/3bff8885288e). Worth asking jkew.

::: xpfe/appshell/nsIXULWindow.idl
@@ +17,5 @@
>  interface nsIDocShellTreeItem;
>  interface nsIXULBrowserWindow;
>  interface nsITabParent;
>  
> +[scriptable, uuid(0d2760ac-636e-4b80-a8ec-11385bc47f0e)]

We don't need to bump these anymore, thankfully.

@@ +134,5 @@
>     * docshell could cause problems.
>     */
>    [noscript] void applyChromeFlags();
> +
> +  [noscript, notxpcom] void sizeShellToWithLimit(in int32_t aCx, in int32_t aCy,

Needs documentation
Attachment #8744247 - Flags: feedback?(mconley) → feedback+
(Assignee)

Comment 23

3 years ago
(In reply to Mike Conley (:mconley) - Needinfo me! from comment #22)
> Tell me what you think of this - how about we extend the flags being passed
> back and forth here to encode which values the child is attempting to
> change? That way, the parent knows which ones it can ignore without a magic
> value.

I've been thinking about that. For some reason I assumed that nsIEmbeddingSiteWindow where these flags are defined is used for other classes and didn't want to add tab child specific flags. I don't know where did that come from, I guess I was just tired. After double checking it I think we should do this.

> ::: embedding/browser/nsDocShellTreeOwner.cpp
> @@ +458,5 @@
> >    if (aShellItem == mWebBrowser->mDocShell) {
> > +    nsCOMPtr<nsITabChild> tabChild = do_QueryInterface(webBrowserChrome);
> > +    if (tabChild) {
> > +      // The XUL window to resize is in the parent process, but there we
> > +      // won't be able to get aShellItem to do the hack in nsXULWindow::SizeShellTo,
> 
> It occurs to me (and this is similar to the stuff I'm struggling with in bug
> 1261842, which made me think of it) that the parent can infer the size of
> the shell by measuring the size of the remote browser element, can it not?
> 
> Like, the TabParent has a frameelement, which (I believe) we can resolve to
> a browser, and from that, I think we can get its dimensions. Would that not
> suffice?


I think that there is(/can be) an extra border there, but I'm really not sure. I can give it a try. I'm a little bit concerned about some random side effect for cases we don't test. But that would be nicer indeed.

> 
> @@ +593,5 @@
> >  nsDocShellTreeOwner::SetPositionDesktopPix(int32_t aX, int32_t aY)
> >  {
> >    if (mWebBrowser) {
> > +    nsresult rv = mWebBrowser->SetPositionDesktopPix(aX, aY);
> > +    NS_ENSURE_SUCCESS(rv, rv);
> 
> That change is actually quite recent
> (https://hg.mozilla.org/mozilla-central/rev/3bff8885288e). Worth asking jkew.

Good idea.

> > +[scriptable, uuid(0d2760ac-636e-4b80-a8ec-11385bc47f0e)]
> 
> We don't need to bump these anymore, thankfully.

I was planning to uplift this in which case I thought we still have to. But it looks like from 47 it's OK to leave it.

I will address the rest of the comments, thanks!
(Assignee)

Comment 24

3 years ago
(In reply to Mike Conley (:mconley) - Needinfo me! from comment #22)
> @@ +593,5 @@
> >  nsDocShellTreeOwner::SetPositionDesktopPix(int32_t aX, int32_t aY)
> >  {
> >    if (mWebBrowser) {
> > +    nsresult rv = mWebBrowser->SetPositionDesktopPix(aX, aY);
> > +    NS_ENSURE_SUCCESS(rv, rv);
> 
> That change is actually quite recent
> (https://hg.mozilla.org/mozilla-central/rev/3bff8885288e). Worth asking jkew.
> 

Hey, for e10s if we return here then we won't have a chance to notify the puppet widget which would notify the parent about the changes. This did not cause trouble before because the test was turned off. What do you think about this change? Tests seem to pass with it on non-e10s too, did you have any reason to return here early, or do you think this change is acceptable?
Flags: needinfo?(jkew)
(Assignee)

Comment 25

3 years ago
A bit more code context to the change in the previous comment to save you a few clicks:

 NS_IMETHODIMP
 nsDocShellTreeOwner::SetPositionDesktopPix(int32_t aX, int32_t aY)
 {
   if (mWebBrowser) {
-    return mWebBrowser->SetPositionDesktopPix(aX, aY);
+    nsresult rv = mWebBrowser->SetPositionDesktopPix(aX, aY);
+    NS_ENSURE_SUCCESS(rv, rv);
   }
 
   double scale = 1.0;
   GetDevicePixelsPerDesktopPixel(&scale);
   return SetPosition(NSToIntRound(aX * scale), NSToIntRound(aY * scale));
 }
(Assignee)

Comment 26

3 years ago
(In reply to Mike Conley (:mconley) - Needinfo me! from comment #22)
> ::: embedding/browser/nsDocShellTreeOwner.cpp
> @@ +458,5 @@
> >    if (aShellItem == mWebBrowser->mDocShell) {
> > +    nsCOMPtr<nsITabChild> tabChild = do_QueryInterface(webBrowserChrome);
> > +    if (tabChild) {
> > +      // The XUL window to resize is in the parent process, but there we
> > +      // won't be able to get aShellItem to do the hack in nsXULWindow::SizeShellTo,
> 
> It occurs to me (and this is similar to the stuff I'm struggling with in bug
> 1261842, which made me think of it) that the parent can infer the size of
> the shell by measuring the size of the remote browser element, can it not?
> 
> Like, the TabParent has a frameelement, which (I believe) we can resolve to
> a browser, and from that, I think we can get its dimensions. Would that not
> suffice?

Actually I realized that I might not get this. That frame element is a RemoteBrowser
which does not seem to implement any useful interface for dimension requests: mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/remote-browser.xml#14
We could extend let's say nsIRemoteBrowser to either cache the dimensions or request them with sync messages... but I don't see how that would be a lot better. We can also get the elements docshell/tree owner, but those are dead ends, they don't know about the dimensions we need here. So I think I will just stick to the original version, unless I misunderstood you, or missing something. What do you think?
Flags: needinfo?(mconley)
(Assignee)

Comment 27

3 years ago
I talked about this with Mike on IRC, and his idea is to get the dimensions from RemoteBrowser as a plain element. This is probably a good idea but does not change a lot about the patch and I would like to land it so let's try to get back to this later and see if it helps as a follow up.
Flags: needinfo?(mconley)
(Assignee)

Comment 28

3 years ago
Created attachment 8747770 [details] [diff] [review]
interdiff since last version

I guess it helps if you have an interdiff since the last patch.
(Assignee)

Comment 29

3 years ago
Created attachment 8747771 [details] [diff] [review]
window size and position manipulation from JS for e10s. v2
Attachment #8736300 - Attachment is obsolete: true
Attachment #8744247 - Attachment is obsolete: true
Attachment #8747771 - Flags: review?(mconley)
Comment on attachment 8747771 [details] [diff] [review]
window size and position manipulation from JS for e10s. v2

Review of attachment 8747771 [details] [diff] [review]:
-----------------------------------------------------------------

Great work on this, gabor.

::: dom/ipc/PBrowser.ipdl
@@ +158,5 @@
>       */
>      async MoveFocus(bool forward, bool forDocumentNavigation);
>  
> +    /**
> +     * SizeShelTo request propagation to parent.

typo - "SizeShelTo" -> "SizeShellTo"

@@ +160,5 @@
>  
> +    /**
> +     * SizeShelTo request propagation to parent.
> +     *
> +     * aFlag            Can indicate if one of the dimensions should be ignored.

Maybe make reference to nsIEmbeddingSiteWindow::DIM_FLAGS_IGNORE_*

::: dom/ipc/TabChild.cpp
@@ +1019,5 @@
> +  // then send a message to the child to update itself. For APIs like screenX
> +  // this function is called with the current value for the non-changed values.
> +  // In a series of calls like window.screenX = 10; window.screenY = 10; for
> +  // the second call, since screenX is not yet updated we might accidentally
> +  // reset back screeX to it's old value. To avoid this if a parameter did not

typo: "screeX" -> "screenX"

::: embedding/browser/nsDocShellTreeOwner.cpp
@@ +592,5 @@
>  nsDocShellTreeOwner::SetPositionDesktopPix(int32_t aX, int32_t aY)
>  {
>    if (mWebBrowser) {
> +    nsresult rv = mWebBrowser->SetPositionDesktopPix(aX, aY);
> +    NS_ENSURE_SUCCESS(rv, rv);

I guess jkew never responded about this? Might be worth making sure this doesn't somehow re-open bug 1252347.

::: xpfe/appshell/nsIXULWindow.idl
@@ +137,5 @@
> +
> +
> +  /**
> +   * Sometimes the child's nsDocShellTreeOwner needs to propogate a SizeShellTo call to the parent. But the
> +   * shellItem argument of the call will not be available on the parent side, so we pass its dimenstions here.

typo "dimenstions" -> "dimensions"
Attachment #8747771 - Flags: review?(mconley) → review+
(In reply to Mike Conley (:mconley) - Needinfo me! from comment #30)
> ::: embedding/browser/nsDocShellTreeOwner.cpp
> @@ +592,5 @@
> >  nsDocShellTreeOwner::SetPositionDesktopPix(int32_t aX, int32_t aY)
> >  {
> >    if (mWebBrowser) {
> > +    nsresult rv = mWebBrowser->SetPositionDesktopPix(aX, aY);
> > +    NS_ENSURE_SUCCESS(rv, rv);
> 
> I guess jkew never responded about this? Might be worth making sure this
> doesn't somehow re-open bug 1252347.

Ah, sorry - I was away over the bank holiday weekend, and this dropped off my immediate-mode radar....

I'd worry that the patch here, which calls mWebBrowser->SetPositionDesktopPix and then _also_ calls SetPosition, may end up mis-positioning windows in some multi-monitor, mixed-DPI situations.

The reason the current code prefers to delegate to mWebBrowser->SetPositionDesktopPix is that the "desktop pixel" coordinates are globally unambiguous, and can be used to place a window anywhere across a multi-screen desktop. SetPosition, on the other hand, takes coordinates that are tied to a specific monitor, and may not work correctly if the "target" of the positioning is a different screen than where the window currently lives (which will determine the device-to-desktop conversion scale factor).

Maybe calling mWebBrowser->SetPositionDesktopPix first will mean that GetDevicePixelsPerDesktopPixel then gets the correct scale for the "destination" position, and so we'll also get the SetPosition call right.... I'm not sure.

I think it'd be important to test this with the scenarios in bug 1252347 (it shouldn't reintroduce that specific type of failure, AFAICS, but also good to check whether the pop-up gets placed properly); and also re-test the issues from bug 1247335. These things should be tested on both Windows 8.1/10 and OS X with multi-screen, mixed-DPI setups, because it's in those type of environments that problems may arise if a stale scale factor gets used.
Flags: needinfo?(jkew)
(Assignee)

Comment 32

3 years ago
(In reply to Jonathan Kew (:jfkthame) from comment #31)
> (In reply to Mike Conley (:mconley) - Needinfo me! from comment #30)
> > ::: embedding/browser/nsDocShellTreeOwner.cpp
> > @@ +592,5 @@
> > >  nsDocShellTreeOwner::SetPositionDesktopPix(int32_t aX, int32_t aY)
> > >  {
> > >    if (mWebBrowser) {
> > > +    nsresult rv = mWebBrowser->SetPositionDesktopPix(aX, aY);
> > > +    NS_ENSURE_SUCCESS(rv, rv);
> > 
> > I guess jkew never responded about this? Might be worth making sure this
> > doesn't somehow re-open bug 1252347.
> 
> Ah, sorry - I was away over the bank holiday weekend, and this dropped off
> my immediate-mode radar....
> 
> I'd worry that the patch here, which calls
> mWebBrowser->SetPositionDesktopPix and then _also_ calls SetPosition, may
> end up mis-positioning windows in some multi-monitor, mixed-DPI situations.
> 
> The reason the current code prefers to delegate to
> mWebBrowser->SetPositionDesktopPix is that the "desktop pixel" coordinates
> are globally unambiguous, and can be used to place a window anywhere across
> a multi-screen desktop. SetPosition, on the other hand, takes coordinates
> that are tied to a specific monitor, and may not work correctly if the
> "target" of the positioning is a different screen than where the window
> currently lives (which will determine the device-to-desktop conversion scale
> factor).
> 
> Maybe calling mWebBrowser->SetPositionDesktopPix first will mean that
> GetDevicePixelsPerDesktopPixel then gets the correct scale for the
> "destination" position, and so we'll also get the SetPosition call right....
> I'm not sure.
> 
> I think it'd be important to test this with the scenarios in bug 1252347 (it
> shouldn't reintroduce that specific type of failure, AFAICS, but also good
> to check whether the pop-up gets placed properly); and also re-test the
> issues from bug 1247335. These things should be tested on both Windows
> 8.1/10 and OS X with multi-screen, mixed-DPI setups, because it's in those
> type of environments that problems may arise if a stale scale factor gets
> used.

I tested this on OSX with the mixed screen setup and everything worked as before
my patches both with and without e10s on, so I'll go ahead and land this patch
and will check the binaries on windows after, and file a follow-up if needed.

Comment 34

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/ada37e72623d
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox49: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
(Assignee)

Comment 35

3 years ago
Comment on attachment 8747771 [details] [diff] [review]
window size and position manipulation from JS for e10s. v2

Approval Request Comment
[Feature/regressing bug #]: These features never worked with e10s.
[User impact if declined]: Some window size and position manipulation related JS APIs will not work with e10s. This is a M9 e10s bug which means it's a release blocker.
[Describe test coverage new/current, TreeHerder]: The patch is on mc for a while and the test are up (pre patch the tests were turned off for e10s)
[Risks and why]: It's a bit heavy patch but we're moving from something that does not work at all to something that is work and tested, so if any issue might pop up that is likely still better than the pre-patch case. There is one more risk worth to be mentioned in a mixed screen setup, see comment 32 for more details.
[String/UUID change made/needed]: No string changed, and UUID should not matter any more.
Attachment #8747771 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 36

3 years ago
This is an m9 and the aurora request is pending for a week now, so I'm setting needinfo in case this is under the radar.
Flags: needinfo?(rkothari)
Comment on attachment 8747771 [details] [diff] [review]
window size and position manipulation from JS for e10s. v2

e10s related release blocker, Aurora48+
Flags: needinfo?(rkothari)
Attachment #8747771 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
status-firefox48: --- → affected

Comment 38

3 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/a0f48f447f41
status-firefox48: affected → fixed
You need to log in before you can comment on or make changes to this bug.