+++ 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.

Sure, that seems pretty simple. Do you need me to find someone (possibly me) to do this, or do you have it under control?

(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.

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().

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 :-)

(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

(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.

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.

How did you test? resizeTo() is a method on nsIDOMWindow.idl. That should call directly into nsGlobalWindow::ResizeTo().

printf statement on the first line of the methods.

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?

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.

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)?

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

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.

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.

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.

(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 :)

(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/

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 :/

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.

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".

(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?

> 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.

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

> 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;

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.)

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.

> 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.

>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.

Address review comments and add testcase.

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/

(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.

> Yes but this name was used throughout the nsGlobalWindow. Huh. Okay then!

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

(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

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

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

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 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.