1369627 - [e10s] resizeBy not working as expected immediately after popup opens

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[szmydadam]

Attachment: Current and expected result

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/58.0.3029.110 Safari/537.36

Steps to reproduce:

1. Open a popup i.e.: `var popup = window.open('', '', 'width=200,height=200');`
2. Right after that, run resizeBy method on that popup: `popup.resizeBy(50,0);`

Fiddle: https://jsfiddle.net/peo3x4aq/
Fiddle with working workaround (timeout): https://jsfiddle.net/peo3x4aq/1/


Actual results:

Popup gets shrink to very small size.


Expected results:

Popup should just be resized horizontally by 50px

Comment 1

[Loic]

It's an issue only with e10s enabled. FF40 has the bug, therefore the issue is probably here since the introduction of multiprocess.

Comment 2

[Hsin-Yi Tsai (she/her) [:hsinyi]]

I noticed Boris' footprints on ResizeBy* code. Perhaps he knows where our code goes wrong in e10s case.

Comment 3

[Boris Zbarsky [:bzbarsky]]

No idea offhand; I'd have to step through this.  Presumably the "get the current size" codepath doesn't get the right numbers in e10s mode or something, so we end up doing more like a resizeTo.

Michael, want to take a look?  You've been in this code a lot recently... ;)

Comment 4

[Nika Layzell [:nika] (ni? for response)]

I'll look into it after I get the patches for bug 1365032 up (Which I hope will be soon). I'd rather not race my other major changes to that code :).

Comment 5

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 1: Add a test for the behavior of window.resizeBy right after opening a new window

If you're wondering why the WidgetEvent change is present in this patch, I honestly don't know either. Basically when I made the changes to the ipdl files, it randomly caused a warning which was made an error due to -WError start firing on that line of code. I had to add the explicit base class initialization to disable that warning.

No idea why that happened.

MozReview-Commit-ID: GLqs4YU0lbX

Comment 6

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 2: Send DimensionInfo down to the content process synchronously when creating a new window

MozReview-Commit-ID: GlzJ491RLUE

Comment 7

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 3: Add an ugly workaround for non-e10s windows

This is really gross. I don't know why but in non-e10s windows (which wasn't
affected by my patch) we don't seem to fire off a resize event if you resize
immediately after opening the window.

This changes the code to spin the event loop once before actually firing the
resize event. I would be really curious to know why the event wasn't being
fired.

MozReview-Commit-ID: 9l8tAWSo55P

Comment 8

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

I guess in non-e10s there might not be layout yet.

Comment 9

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

Comment on attachment 8875847 [details] [diff] [review]
Part 3: Add an ugly workaround for non-e10s windows

I thought this bug was about calling resizeBy immediately after opening, so how is this test testing that anymore?

Comment 10

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 1: Add a test for the behavior of window.resizeBy right after opening a new window

I think I found a way to avoid needing to delay the call to resizeBy - this seems to be green on try.

MozReview-Commit-ID: GLqs4YU0lbX

Comment 11

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 1: Add a test for the behavior of window.resizeBy right after opening a new window

Disabled the test on android, as testing resizeby on android doesn't really make sense.

MozReview-Commit-ID: GLqs4YU0lbX

Comment 12

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

Comment on attachment 8877304 [details] [diff] [review]
Part 1: Add a test for the behavior of window.resizeBy right after opening a new window

>+        // We resized synchronously. If this happened, we sometimes won't fire
>+        // an onresize event, so we resolve immediately.
Nit, there isn't such even as onresize. onresize is event handler name. Event is resize.

>+        if (popup.innerWidth == 550) {
>+          resolve(popup);
>+        } else {
>+          let onresize = () => {
hopefully this doesn't get mixed with onresize on global scope.
But renaming to popupresize might not be too bad idea.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f351fbc39a79
Part 1: Add a test for the behavior of window.resizeBy right after opening a new window, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/8dd7bd8a399a
Part 2: Send DimensionInfo down to the content process synchronously when creating a new window, r=smaug

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/f351fbc39a79
https://hg.mozilla.org/mozilla-central/rev/8dd7bd8a399a