891763 - Browser API: mozbrowserwindowresizeto and mozbrowserwindowresizeby events

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect, P1)

Description

[Tim Guan-tin Chien [:timdream] (please needinfo)]

+++ This bug was initially created as a clone of Bug #810036 +++

Instead of bug #810036, the alternative approach suggested by Thinker is simply redirect |window.resizeTo| and |window.resizeBy| calls to mozbrowser |mozbrowserwindowresizeto|by| events, allowing Keyboard apps to explicitly set it's dimensions.

This will be a much simpler approach.

Comment 1

[Justin Lebar (not reading bugmail)]

Sure, that seems pretty simple.

Do you need me to find someone (possibly me) to do this, or do you have it under control?

Comment 2

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Justin Lebar [:jlebar] from comment #1)
> Sure, that seems pretty simple.
> 
> Do you need me to find someone (possibly me) to do this, or do you have it
> under control?

I am tempted to figure out how to fix this bug myself, in order to know more stuff about Gecko. Although I am not entirely sure if this can be done entirely in JavaScript, if it doesn't I would just have to let it go :-/

I was wondering if it's possible to overwrite CPP function like |resizeTo| here in BrowserElementChild.js

http://dxr.mozilla.org/mozilla-central/source/dom/base/nsGlobalWindow.cpp?from=resizeTo#l5926

If that can be done, I could surely try to come up with a patch.

Comment 3

[Justin Lebar (not reading bugmail)]

The thing to do is probably what we do for nsGlobalWindow::Close():

When the window closes, we dispatch a DOMWindowClose event on the window.  BrowserElementParent listens to this event, and then does its thing.

You could do a similar thing in resizeTo().

Comment 4

[Jan Jongboom [:janjongboom] (inactive)]

Looked at this but nsGlobalWindow::ResizeBy and ResizeTo never fire when using a mozbrowser component (at least in B2G desktop), so no clue how to proceed :-)

Comment 5

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Jan Jongboom [:janjongboom] from comment #4)
> Looked at this but nsGlobalWindow::ResizeBy and ResizeTo never fire when
> using a mozbrowser component (at least in B2G desktop), so no clue how to
> proceed :-)

Ah, you beat me to it! I was suspecting that we will not receive MozBeforeResize[1] event because the event is not dispatched at all because the CPP function would just return early, see 
http://dxr.mozilla.org/mozilla-central/source/dom/base/nsGlobalWindow.cpp?from=resizeTo#l5934
http://dxr.mozilla.org/mozilla-central/source/dom/base/nsGlobalWindow.cpp#l5300

I *think* |mHadOriginalOpener| will always be a falsey value. We could find out by gdb.

[1] https://developer.mozilla.org/en-US/docs/Web/Reference/Events/MozBeforeResize

Comment 6

[Justin Lebar (not reading bugmail)]

(In reply to Jan Jongboom [:janjongboom] (PTO until 7/28) from comment #4)
> Looked at this but nsGlobalWindow::ResizeBy and ResizeTo never fire when
> using a mozbrowser component

I hope those methods run, but return early, like Tim says.

You'd have to change this code so that, if the window is a mozbrowser, it does something different.  Similar to what nsGlobalWindow::Close() does.

Comment 7

[Jan Jongboom [:janjongboom] (inactive)]

No, afaik it doesn't even make it into these methods (except for phone startup one time). At least that's the case in B2G desktop.

Comment 8

[Justin Lebar (not reading bugmail)]

How did you test?

resizeTo() is a method on nsIDOMWindow.idl.  That should call directly into nsGlobalWindow::ResizeTo().

Comment 9

[Jan Jongboom [:janjongboom] (inactive)]

printf statement on the first line of the methods.

Comment 10

[Justin Lebar (not reading bugmail)]

And a dump statement right before you called the methods?  And you're sure that printf's are being reported -- you put a printf somewhere else and that worked?

Comment 11

[Jan Jongboom [:janjongboom] (inactive)]

Yeah, a printf shows up on startup of b2g desktop (initial resize), just not later on. Might be that I missed something obvious, I'm not very proficient in this part.

Comment 12

[Justin Lebar (not reading bugmail)]

The most likely scenario is that the JS which is supposed to call this method isn't running; are you positive that you're calling this method (e.g. with dump()'s before and after the call)?

Comment 13

[Ivan Tsay (:ITsay)]

Since this block the keyboard OOP, flag it to koi+. Also add keyword in whiteboard for EPM tracking and wiki site: https://wiki.mozilla.org/FirefoxOS/SprintStatus#Systems-Platform

Comment 14

[Kan-Ru Chen [:kanru] (UTC+9)]

Hmm. I really don't want to create a new Event type just to dispatch the "DOMWindowResize" event to BrowserElementChildPreload.js

Is there a better way to do this? Can't use DispatchCustomEvent because we need to add the width and height as payload.

Comment 15

[Justin Lebar (not reading bugmail)]

You could dispatch a CustomEvent with a custom payload, as we do in BrowserElementParent.cpp.  That's not a lot easier than creating a new event, but maybe it's a bit better.

Comment 16

[Gabriele Svelto [:gsvelto] (PTO)]

I was about to start investigating bug 902840 on behalf of the comm apps team and I found this bug; it seems to me that bug 902840 is a genuine duplicate of this and thus I'm going to close it. Kan-Ru, are you working on this already? Otherwise I can pick it up myself.

Comment 18

[Kan-Ru Chen [:kanru] (UTC+9)]

(In reply to Gabriele Svelto [:gsvelto] from comment #16)
> I was about to start investigating bug 902840 on behalf of the comm apps
> team and I found this bug; it seems to me that bug 902840 is a genuine
> duplicate of this and thus I'm going to close it. Kan-Ru, are you working on
> this already? Otherwise I can pick it up myself.

Yes. I have a WIP patch that could be shared soon :)

Comment 19

[Gabriele Svelto [:gsvelto] (PTO)]

(In reply to Kan-Ru Chen [:kanru] from comment #18)
> Yes. I have a WIP patch that could be shared soon :)

Thanks, that's great \o/

Comment 20

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: WIP patch

With this patch I got

[Parent 1060] ###!!! ASSERTION: Class name and proto chain interface name mismatch!: 'nsCRT::strcmp(CutPrefix(name), mData->mName) == 0', file /home/kanru/zone2/mozilla/central/dom/base/nsDOMClassInfo.cpp, line 2109

Must be something wrong when I register the new DOMClassInfo :/

Comment 21

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: Browser API: mozbrowserresize event

Methods that change the window outer size will succeed if it's an MozApp. New event "DOMWindowResize" is fired which has e.detail.width and e.detail.height property that represent the desired size.

Comment 22

[Justin Lebar (not reading bugmail)]

We need tests for this.

I'm also not sure this works right.  SetOuterSize() tries to set the size of the TreeOuterWindow(), which is the topmost window.  But if you're inside a mozbrowser, that's not the right thing.

Similarly, I don't think we want to call treeOwnerAsWin->SetSize().

>diff --git a/dom/browser-element/BrowserElementChildPreload.js b/dom/browser-element/BrowserElementChildPreload.js
>index 291daca0..d5e7f56 100644
>--- a/dom/browser-element/BrowserElementChildPreload.js
>+++ b/dom/browser-element/BrowserElementChildPreload.js

>@@ -543,20 +546,33 @@ BrowserElementChild.prototype = {
>+  _windowResizeHandler: function(e) {
>+    let win = e.target;
>+    if (win != content || e.defaultPrevented) {
>+      return;
>+    }
>+
>+    debug("resizing window " + win);
>+    sendAsyncMsg('resize', { width: e.detail.width, height: e.detail.height });
>+
>+    // Inform the window implementation that we handled this close ourselves.
>+    e.preventDefault();
>+  },

Copy-paste error with "close".

Comment 23

[Kan-Ru Chen [:kanru] (UTC+9)]

(In reply to Justin Lebar [:jlebar] from comment #22)
> We need tests for this.
> 
> I'm also not sure this works right.  SetOuterSize() tries to set the size of
> the TreeOuterWindow(), which is the topmost window.  But if you're inside a
> mozbrowser, that's not the right thing.

I'm not sure we want to trap which resize methods, maybe just resizeTo and resizeBy for now?
 
> Similarly, I don't think we want to call treeOwnerAsWin->SetSize().

Do you mean don't call treeOwnerAsWin->SetSize at all if we are inside a mozbrowser?

Comment 24

[Justin Lebar (not reading bugmail)]

> I'm not sure we want to trap which resize methods, maybe just resizeTo and resizeBy for 
> now?

Okay, that sounds fine.

> Do you mean don't call treeOwnerAsWin->SetSize at all if we are inside a mozbrowser?

Yes, I think so.  I think that's what we currently do, due to the InFrame() check.  And that's what we'd do in your patch if the embedder calls preventDefault() on the event.  The only question is, what should we do if the embedder does not call preventDefault()?

But also, I think these patches may return the size of the top-level window.  You want to return the frame's inner size, I think.

Comment 25

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: Browser API: mozbrowserresize event v2

if treeOwnerAsWin->SetSize is the size of the top-level window, why do we use that in nsGlobalWindow::ResizeTo? Do you know?

Comment 26

[Justin Lebar (not reading bugmail)]

> if treeOwnerAsWin->SetSize is the size of the top-level window, why do we use that in 
> nsGlobalWindow::ResizeTo? Do you know?

(I think) ResizeTo resizes the top-level window.  Currently you can only call it if you're not in a frame, so we don't currently have to deal with the problem of a frame calling resizeTo and resizing the whole window.

See also bug 565541, where we made resizeTo() do nothing in most cases.

>+  if (mDocShell->GetIsApp()) {

I think this should be if (IsFrame() || !CanResizeWindows()), because an app can be a top-level frame.

We should make this resize event chrome-only, because we don't want websites to see it.  (We should probably change the first DOMWindowClose event too; apparently I even made us fire that twice?!)  smaug says that the way to do this is

  event->GetInternalNSEvent()->mFlags.mOnlyChromeDispatch = true;

Comment 27

[Justin Lebar (not reading bugmail)]

Actually, I think the DOMWindowClose thing is fine; we don't actually fire it twice.  (We fire it in Close and ForceClose, but Close calls FinalClose, not ForceClose.)

Comment 28

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: Browser API: mozbrowserresize event v3

Make window.resize{To,By} works complete differently in browser-element.

When the content in a browser-element requests window.resizeTo, we send a mozbrowserresize event to its parent. The unit is CSS pixels and we assume the browser-element has no chrome thus its window.innerHeight equals to the iframe's height.

Comment 29

[Justin Lebar (not reading bugmail)]

> The unit is CSS pixels and we assume the browser-element has no chrome thus its window.innerHeight 
> equals to the iframe's height.

This isn't strictly true, because an iframe can have a border.  But I still think the code is correct.  The embedder can calculate the height to give to the iframe by doing e.detail.height + 2 * iframe.marginheight.

Comment 30

[Justin Lebar (not reading bugmail)]

>diff --git a/dom/base/nsGlobalWindow.cpp b/dom/base/nsGlobalWindow.cpp
>index 9f6b5c7..6961aebc 100644
>--- a/dom/base/nsGlobalWindow.cpp
>+++ b/dom/base/nsGlobalWindow.cpp

>@@ -4942,20 +4945,50 @@ nsGlobalWindow::DispatchCustomEvent(const char *aEventName)
> {
>   bool defaultActionEnabled = true;
>   nsContentUtils::DispatchTrustedEvent(mDoc,
>                                        GetOuterWindow(),
>                                        NS_ConvertASCIItoUTF16(aEventName),
>                                        true, true, &defaultActionEnabled);
> 
>   return defaultActionEnabled;
> }
> 
>+// NOTE: Arguments to this function should be CSS pixels, not device pixels.
>+bool
>+nsGlobalWindow::DispatchResizeEvent(const nsIntSize& aSize)
>+{
>+  nsCOMPtr<nsIDOMDocument> domDoc = do_QueryInterface(mDoc);
>+  nsCOMPtr<nsIDOMEvent> event;
>+  nsresult rv = domDoc->CreateEvent(NS_LITERAL_STRING("CustomEvent"), getter_AddRefs(event));

Nit: Wrap lines at 80 chars.

>+  NS_ENSURE_SUCCESS(rv, false);
>+  event->GetInternalNSEvent()->mFlags.mOnlyChromeDispatch = true;
>+
>+  nsCOMPtr<nsIWritableVariant> detailVariant = new nsVariant();
>+  rv = detailVariant->SetAsISupports(new nsDOMWindowResizeEventDetail(aSize));

It is dangerous and against the rules to call a XPCOM method on an object that
has refcount 0.

The reason is, if SetAsISupports(x) does

  {
    nsCOMPtr<nsISupports> foo = x;
  }

then we'll increase x's refcount to 1, and then decrement it down to 0, at
which point we'll |delete x|!  Then if SetAsISupports uses x later (which it
probably will, since it wants to store it somewhere), that will be a
use-after-free.

You'll need to stick the nsDOMWindowResizeEventDetail into an nsRefPtr before
calling SetAsISupports.

>+  NS_ENSURE_SUCCESS(rv, false);
>+
>+  nsCOMPtr<nsIDOMCustomEvent> customEvent = do_QueryInterface(event);
>+  customEvent->InitCustomEvent(NS_LITERAL_STRING("DOMWindowResize"),
>+                                                 /* bubbles = */ true,
>+                                                 /* cancelable = */ true,
>+                                                 detailVariant);

Nit: Decrease the indentation of the three lines above to be underneath the
first '('.

>+  customEvent->SetTrusted(true);
>+
>+  nsCOMPtr<EventTarget> target = do_QueryInterface(GetOuterWindow());
>+  customEvent->SetTarget(target);
>+
>+  bool defaultActionEnabled = true;
>+  target->DispatchEvent(event, &defaultActionEnabled);
>+
>+  return defaultActionEnabled;

Per comment 26, we need to set this event to chrome-only dispatch.

>+}

>@@ -5963,20 +6010,40 @@ nsGlobalWindow::ResizeTo(int32_t aWidth, int32_t aHeight)
> 
>   return NS_OK;
> }
> 
> NS_IMETHODIMP
> nsGlobalWindow::ResizeBy(int32_t aWidthDif, int32_t aHeightDif)
> {
>   FORWARD_TO_OUTER(ResizeBy, (aWidthDif, aHeightDif), NS_ERROR_NOT_INITIALIZED);
> 
>   /*
>+   * If caller is a browser-element then dispatch a resize event to
>+   * parent.
>+   */
>+  if (mDocShell->GetIsBrowserOrApp()) {
>+    CSSIntSize size;
>+    nsresult rv = GetInnerSize(size);
>+    NS_ENSURE_SUCCESS(rv, NS_OK);
>+
>+    size.width += aWidthDif;
>+    size.height += aHeightDif;
>+    CheckSecurityWidthAndHeight(&size.width, &size.height);

I'm not sure we want or need this security check, here or in Resize().  It
looks like the security check is there to make sure we don't make the top-level
window too small, but that's not an issue here.

>+    if (!DispatchResizeEvent(nsIntSize(size.width, size.height))) {
>+      // Someone chose to prevent the default action for this event, if
>+      // so, let's not resize this window after all...
>+      return NS_OK;
>+    }
>+  }
>+
>+  /*
>    * If caller is not chrome and the user has not explicitly exempted the site,
>    * prevent window.resizeBy() by exiting early
>    */
> 
>   if (!CanMoveResizeWindows() || IsFrame()) {
>     return NS_OK;
>   }
> 
>   nsCOMPtr<nsIBaseWindow> treeOwnerAsWin = GetTreeOwnerWindow();
>   NS_ENSURE_TRUE(treeOwnerAsWin, NS_ERROR_FAILURE);

One more round?

Also, we'll need tests for this.

Comment 31

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: Browser API: mozbrowserresize event v4

Address review comments and add testcase.

Comment 32

[Justin Lebar (not reading bugmail)]

Comment on attachment 795870 [details] [diff] [review]
Browser API: mozbrowserresize event v4

Looks great, with some minor nits.

>diff --git a/dom/base/nsGlobalWindow.cpp b/dom/base/nsGlobalWindow.cpp
>index 9f6b5c7..30ec09c 100644
>--- a/dom/base/nsGlobalWindow.cpp
>+++ b/dom/base/nsGlobalWindow.cpp

>+// NOTE: Arguments to this function should be CSS pixels, not device pixels.
>+bool
>+nsGlobalWindow::DispatchResizeEvent(const nsIntSize& aSize)
>+{

>+  bool defaultActionEnabled = true;
>+  target->DispatchEvent(event, &defaultActionEnabled);
>+
>+  return defaultActionEnabled;
>+}

Nit: I think you mean "defaultActionAllowed"?

>@@ -5936,20 +5971,33 @@ nsGlobalWindow::MoveBy(int32_t aXDif, int32_t aYDif)
> NS_IMETHODIMP
> nsGlobalWindow::ResizeTo(int32_t aWidth, int32_t aHeight)
> {
>   FORWARD_TO_OUTER(ResizeTo, (aWidth, aHeight), NS_ERROR_NOT_INITIALIZED);
> 
>   /*
>+   * If caller is a browser-element then dispatch a resize event to
>+   * parent.
>+   */

We call the frame which contains the browser-element the "embedder," so
s/parent/the embedder/.

>+  if (mDocShell->GetIsBrowserOrApp()) {
>+    nsIntSize size(aWidth, aHeight);
>+    if (!DispatchResizeEvent(size)) {
>+      // Someone chose to prevent the default action for this event, if
>+      // so, let's not resize this window after all...

Nit: s/Someone/The embedder/
Nit: s/if so,/, so/

>+      return NS_OK;
>+    }
>+  }

>@@ -5963,20 +6011,39 @@ nsGlobalWindow::ResizeTo(int32_t aWidth, int32_t aHeight)
> 
>   return NS_OK;
> }
> 
> NS_IMETHODIMP
> nsGlobalWindow::ResizeBy(int32_t aWidthDif, int32_t aHeightDif)
> {
>   FORWARD_TO_OUTER(ResizeBy, (aWidthDif, aHeightDif), NS_ERROR_NOT_INITIALIZED);
> 
>   /*
>+   * If caller is a browser-element then dispatch a resize event to
>+   * parent.
>+   */
>+  if (mDocShell->GetIsBrowserOrApp()) {
>+    CSSIntSize size;
>+    nsresult rv = GetInnerSize(size);
>+    NS_ENSURE_SUCCESS(rv, NS_OK);
>+
>+    size.width += aWidthDif;
>+    size.height += aHeightDif;
>+
>+    if (!DispatchResizeEvent(nsIntSize(size.width, size.height))) {
>+      // Someone chose to prevent the default action for this event, if
>+      // so, let's not resize this window after all...

Nit: s/Someone/The embedder/
Nit: s/if so,/, so/

>+      return NS_OK;
>+    }
>+  }

>diff --git a/dom/browser-element/mochitest/browserElement_BrowserWindowResize.js b/dom/browser-element/mochitest/browserElement_BrowserWindowResize.js
>new file mode 100644
>index 0000000..64153fa
>--- /dev/null
>+++ b/dom/browser-element/mochitest/browserElement_BrowserWindowResize.js

>+  function testIFrameWithSrc(src) {
>+    var iframe = document.createElement('iframe');
>+    SpecialPowers.wrap(iframe).mozbrowser = true;
>+    iframe.style = "border:none; width:400px; height:400px;";
>+    iframe.src = src;
>+    iframe.addEventListener("mozbrowserresize", function (e) {
>+      is(e.detail.width,  300, "Receive correct resize event width");
>+      is(e.detail.height, 300, "Receive correct resize event height");

Nit: s/Receive/Received/

Comment 33

[Kan-Ru Chen [:kanru] (UTC+9)]

(In reply to Justin Lebar [:jlebar] from comment #32)
> >diff --git a/dom/base/nsGlobalWindow.cpp b/dom/base/nsGlobalWindow.cpp
> >index 9f6b5c7..30ec09c 100644
> >--- a/dom/base/nsGlobalWindow.cpp
> >+++ b/dom/base/nsGlobalWindow.cpp
> 
> >+// NOTE: Arguments to this function should be CSS pixels, not device pixels.
> >+bool
> >+nsGlobalWindow::DispatchResizeEvent(const nsIntSize& aSize)
> >+{
> 
> >+  bool defaultActionEnabled = true;
> >+  target->DispatchEvent(event, &defaultActionEnabled);
> >+
> >+  return defaultActionEnabled;
> >+}
> 
> Nit: I think you mean "defaultActionAllowed"?

Yes but this name was used throughout the nsGlobalWindow.

Comment 34

[Kan-Ru Chen [:kanru] (UTC+9)]

https://hg.mozilla.org/integration/b2g-inbound/rev/157057f1396a

Comment 35

[Justin Lebar (not reading bugmail)]

> Yes but this name was used throughout the nsGlobalWindow.

Huh.  Okay then!

Comment 36

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/157057f1396a

Comment 37

[Masatoshi Kimura [:emk]]

Pleeease use WebIDL dictionary instead of sticking the random interface name on global object anymore. Even desktop Firefox will be polluted.

Comment 38

[Kan-Ru Chen [:kanru] (UTC+9)]

(In reply to Masatoshi Kimura [:emk] from comment #37)
> Pleeease use WebIDL dictionary instead of sticking the random interface name
> on global object anymore. Even desktop Firefox will be polluted.

bug 910088

Comment 39

[Justin Lebar (not reading bugmail)]

I believe there was a reason we had to use XPIDL for things like this.  But maybe that's gone now.  If so, great!

Comment 40

[Ivan Tsay (:ITsay)]

adjust keyword in white board for the status query to be more precise

Comment 41

[GaryChen [:GaryChen][:PYChen][:gchen][:陳柏宇]]

Attachment: pull request: https://github.com/mozilla-b2g/gaia/pull/12073

Hi Rudy,
   This patch tests new browser API about |window.resizeto()| and |mozbrowserresize|, it works fine but I need your feedback since I changed keyboard app and keyboard manager.

  Should we file another bug for gaia work?

Comment 42

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

Comment on attachment 802269 [details]
pull request: https://github.com/mozilla-b2g/gaia/pull/12073

Looks pretty nasty.
Good work!

Yes, please create a Gaia bug for this change.
Thank you.