1082127 - [e10s] window.moveTo and window.resizeTo not implemented

Status: RESOLVED FIXED

(Core :: DOM: Content Processes, defect)

Description

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

We probably should implement this, at least for tests. It looks like we need it for Talos tresize.
http://mxr.mozilla.org/mozilla-central/source/dom/ipc/TabChild.cpp#1222

Call chain:
http://mxr.mozilla.org/mozilla-central/source/dom/base/nsGlobalWindow.cpp#6843
http://mxr.mozilla.org/mozilla-central/source/embedding/browser/nsDocShellTreeOwner.cpp

Comment 1

[Avi Halachmi (:avih)]

Same goes for window.resizeTo

I commented out the window.moveTo line in talos/startup_test/tresize-test.html, and then it failed on on next line of window.resizeTo with the same error:

[Exception... "Method not implemented"  nsresult: "0x80004001 (NS_ERROR_NOT_IMPLEMENTED)"  location: "JS frame :: http://localhost:15707/startup_test/tresize-test.html :: <TOP_LEVEL> :: line 88"  data: no]

Comment 2

[Joel Maher ( :jmaher ) (UTC -8)]

I see this is m5+, I assume there are no short term plans to fix this issue.

Comment 3

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

We're on m4 right now - so m5 is coming up.

Comment 4

[Joel Maher ( :jmaher ) (UTC -8)]

oh cool

Comment 5

[Avi Halachmi (:avih)]

This bug no longer blocks talos-tresize on e10s. See bug 1050706 comment 89 for more details.

In a nutshell, tresize should resize the window from the patent process because it's closer to the user resizing the window manually.

Comment 6

[Brian Di Palma]

I guess this bug will be addressed before e10s ships to release?
It's affecting our web app (trading application, traders like to popout components which we resize).

Comment 7

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

(In reply to Brian Di Palma from comment #6)
> I guess this bug will be addressed before e10s ships to release?
> It's affecting our web app (trading application, traders like to popout
> components which we resize).

Yes - this is a web-compat issue, and will be fixed before e10s ships to release.

Comment 8

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

Attachment: 0001-Bug-1082127-Implement-TabChild-SetDimensions-so-that.patch

Comment 9

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

Comment on attachment 8547858 [details] [diff] [review]
0001-Bug-1082127-Implement-TabChild-SetDimensions-so-that.patch

mett.

Comment 10

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8547858 [details] [diff] [review]
0001-Bug-1082127-Implement-TabChild-SetDimensions-so-that.patch

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

I'm not a widget/dom peer sorry.

Comment 11

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

Comment on attachment 8547858 [details] [diff] [review]
0001-Bug-1082127-Implement-TabChild-SetDimensions-so-that.patch

I think :smaug should be able to review this then?

I'm currently looking into tests for this, specifically why test_resize_move_windows.html wasn't being triggered for this.

Comment 12

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

So I've identified a couple of issues with this patch. It looks like (independent of what's going on here), the "size" of a window in e10s is different to that of non-e10s windows. I'll file a bug about that separately from this one.

The other issue is that we don't actually get a resize call back to TabChild from TabParent like I thought we would, but that's easily remedied by a quick call to SendUpdateDimensions in TabParent. I'm not a huge fan of the ping-ponging, but :handyman thinks it makes sense and I think it probably does too.

Comment 13

[Olli Pettay [:smaug][[e-mail]] (vacation-ish -> Aug 1)]

Comment on attachment 8547858 [details] [diff] [review]
0001-Bug-1082127-Implement-TabChild-SetDimensions-so-that.patch

So on parent side I would pass the params to the treeowner the same way as
what happens in nsGlobalWindow

  NS_ENSURE_TRUE(mFrameElement, true);
  nsCOMPtr<nsIDocShell> docShell = mFrameElement->OwnerDoc()->GetDocShell();
  NS_ENSURE_TRUE(docShell, true);
  nsCOMPtr<nsIDocShellTreeOwner> treeOwner;
  docShell->GetTreeOwner(getter_AddRefs(treeOwner));
  nsCOMPtr<nsIBaseWindow> treeOwnerAsWin = do_QueryInterface(treeOwner);
  NS_ENSURE_TRUE(treeOwnerAsWin, true);
  and then do call the methods on treeOwnerAsWin


That way we'd store the persistent attributes related to window size etc.
and would use closer the same code paths what nsGlobalWindow uses.

Comment 14

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

Attachment: 0001-Bug-1082127-Implement-TabChild-SetDimensions-so-that.patch

Mostly just took smaug's suggestion here. We can't call SetPositionAndSize directly as we need to check aFlags to ensure we only set position or size (we often get, e.g. SetDimensions with a size of 0x0).

Tests we can't enable yet because of bug 1121791 and bug 1075670.

Comment 15

[Olli Pettay [:smaug][[e-mail]] (vacation-ish -> Aug 1)]

Comment on attachment 8550418 [details] [diff] [review]
0001-Bug-1082127-Implement-TabChild-SetDimensions-so-that.patch

>+TabParent::RecvSetDimensions(const uint32_t& aFlags,
>+                             const int32_t& aX, const int32_t& aY,
>+                             const int32_t& aCx, const int32_t& aCy)
>+{
>+  nsCOMPtr<nsIWidget> widget = GetWidget();
You don't need widget for anything.

>+  NS_ENSURE_TRUE(mFrameElement, true);
>+  nsCOMPtr<nsIDocShell> docShell = mFrameElement->OwnerDoc()->GetDocShell();
>+  NS_ENSURE_TRUE(docShell, true);
>+  nsCOMPtr<nsIDocShellTreeOwner> treeOwner;
>+  docShell->GetTreeOwner(getter_AddRefs(treeOwner));
>+  nsCOMPtr<nsIBaseWindow> treeOwnerAsWin = do_QueryInterface(treeOwner);
>+  NS_ENSURE_TRUE(treeOwnerAsWin, true);
>+
>+  if (aFlags & nsIEmbeddingSiteWindow::DIM_FLAGS_POSITION) {
>+    treeOwnerAsWin->SetPosition(aX, aY);
>+  }
>+
>+  if (aFlags & nsIEmbeddingSiteWindow::DIM_FLAGS_SIZE_INNER ||
Looks like DIM_FLAGS_SIZE_INNER isn't actually used ever.
Perhaps better to just add MOZ_ASSERT that aFlags never contains it.


>+      aFlags & nsIEmbeddingSiteWindow::DIM_FLAGS_SIZE_OUTER) {
>+    treeOwnerAsWin->SetSize(aCx, aCy, true);
>+  }


Based on http://mxr.mozilla.org/mozilla-central/source/embedding/browser/nsDocShellTreeOwner.cpp#530
we might want to deal explicitly the case when both DIM_FLAGS_POSITION and DIM_FLAGS_SIZE_OUTER are set.

So perhaps something like.
  if ((aFlags & nsIEmbeddingSiteWindow::DIM_FLAGS_POSITION) &&
      (aFlags & nsIEmbeddingSiteWindow::DIM_FLAGS_SIZE_OUTER)) {
    treeOwnerAsWin->SetPositionAndSize...
    return true;
  }

  if (aFlags & nsIEmbeddingSiteWindow::DIM_FLAGS_POSITION) {
    treeOwnerAsWin->SetPosition(aX, aY);
    return true;
  }

  if (aFlags & nsIEmbeddingSiteWindow::DIM_FLAGS_SIZE_OUTER) {
    treeOwnerAsWin->SetSize(aCx, aCy, true);
    return true;
  }
MOZ_ASSERT(false, "unknown flags!");
return true;

Comment 16

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/50981656f6b7

Comment 17

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/50981656f6b7