1149555 - Update resize event firing to follow the specs

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

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

https://html.spec.whatwg.org/multipage/webappapis.html#run-the-resize-steps
http://dev.w3.org/csswg/cssom-view/#resizing-viewports

Comment 1

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

Attachment: v1

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1320e8c3748b

Comment 2

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

Attachment: v2

I'm still thinking what kind of tests to write for this.
test_reentrant_flush.html change of course does test one part of this.


https://treeherder.mozilla.org/#/jobs?repo=try&revision=6615aac6bcb3

Comment 3

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

There are still couple of test failures, looks like broken tests.

Comment 4

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

Trying to fix the tests
https://treeherder.mozilla.org/#/jobs?repo=try&revision=acd45759f7a9

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Could you summarize what it is that you're changing?

Comment 6

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

Per HTML spec resize should be fired when refreshdriver ticks, before animation frame callbacks.
So, I remove the task to dispatch resize event, and also remove dispatching pending resize event when layout is flushed.
This should give us a bit better compatibility with other browsers, see bug 1121882.
Though, blink will too get minor tweaks to follow the spec better
https://code.google.com/p/chromium/issues/detail?id=472247

Comment 7

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

Yet some more fixes, https://treeherder.mozilla.org/#/jobs?repo=try&revision=7e68a2b39783

(looks like layout/reftests/font-face has lots of racy tests, but not fixing them all, only making one test behave like the others)

Comment 8

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

Attachment: v3 (more test fixes)

Comment 10

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

Review ping.

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8591394 [details] [diff] [review]
v3 (more test fixes)

This patch should have a useful commit message.  (In general, I like
to review commit messages, both in order to make sure that they're
good commit messages, and because it's useful to know what the patch
is intending to do before I review it.)

I'd suggest the following, based on comment 6:

> Bug 1149555 - Move firing of resize events from its own event on the event loop to being part of the refresh driver tick.  r=dbaron
> 
> This also removes the firing of pending resize events during layout
> flushes.
> 
> This matches requirements in the HTML specification, and should improve
> compatibility with other browsers.


I'm a little worried about the performance effects of removing the
firing of pending resize events during layout flushes.  There's a
potential performance cost in that if the resize gets fired later, we
might end up having to do extra work to handle the changes made by the
resize event that would otherwise have been merged with the work of
processing the flush.  On the flip side, there's a potential performance
benefit of doing less work if there are multiple resizes, although that
seems a little less likely.

Given that I don't think there's a specification that describes what
should happen during layout flushes, I'm hesitant to remove the firing
in response to a specification.  Does removing it match the behavior of
other browsers?


browser/base/content/test/general/browser_windowopen_reflows.js:

The other entries in this file have a comment explaining them; yours
should too.

browser/base/content/test/social/browser_social_chatwindow_resize.js:

Could you explain the changes in this test?

nsPresShell.h:

>+  virtual void FireResizeEvent();

add 'override'.

nsPresShell.cpp:

>+  rd->RemoveResizeEventFlushObserver(this);

Seems like this may as well be conditional on mResizeEventPending.

nsRefreshDriver.cpp:

>   for (uint32_t i = 0; i < ArrayLength(mObservers); ++i) {
>+
>+    // Resize events should be fired before layout flushes or
>+    // calling animation frame callbacks.
>+    if (i == 0 && mPresContext && mPresContext->GetPresShell()) {

Please just put this before the loop and skip the i==0 check.

>+      for (uint32_t j = observers.Length();
>+           j && mPresContext && mPresContext->GetPresShell(); --j) {
>+        // Make sure to not process observers which might have been removed
>+        // during previous iterations.
>+        nsCOMPtr<nsIPresShell> shell = observers[j - 1];

This is awfully strange.  How about just:

  for (uint32_t j = observers.Length(); j-- != 0; ) {
    if (!mPresContext || !mPresContext->GetPresShell()) {
      break;
    }
    nsCOMPtr<nsIPresShell> shell = observers[j];
  }

and then put the comment right before the Contains() check?

>+        // Make sure to not process observers which might have been removed
>+        // during previous iterations.
>+        if (!mResizeEventFlushObservers.Contains(shell)) {
>+          continue;
>+        }

This being O(N^2) seems less than great.

Instead of Contains(), could you use LastIndexOf() != nsTArray<...>::NoIndex,
or, even better, add an RContains() method to nsTArray?

nsRefreshDriver.h:

>+    bool appended = mResizeEventFlushObservers.AppendElement(aShell) != nullptr;
>+    EnsureTimerStarted();
>+    return appended;

nsAutoTArray is infallible.  So please drop |bool appended|, change
the return type of AddResizeEventFlushObserver to void, and fix the
caller to act as though it always succeeds.

layout/reftests/font-face/ex-unit-1-dynamic.html:

Would it also be sufficient to make run() read
document.getElementsByTagName("iframe")[0].contentDocument.body.offsetWidth
as the first thing it does (before calling arm())?



So looks good with those changes except that I'd like to hear back about
whether removing the sending of resize events on flush makes us more or less
compatible with other browsers.

Comment 12

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

The O(n^2) is what other refreshdriver stuff is doing too, unfortunately.
I guess we should fix them all.

"Given that I don't think there's a specification that describes what
should happen during layout flushes, I'm hesitant to remove the firing
in response to a specification.  Does removing it match the behavior of
other browsers?"
I don't quite understand the sentence "I'm hesitant to remove the firing.."
As explained in Bug 1121882, our current behavior doesn't match the spec nor other browsers.

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to Olli Pettay [:smaug] from comment #12)
> The O(n^2) is what other refreshdriver stuff is doing too, unfortunately.
> I guess we should fix them all.
> 
> "Given that I don't think there's a specification that describes what
> should happen during layout flushes, I'm hesitant to remove the firing
> in response to a specification.  Does removing it match the behavior of
> other browsers?"
> I don't quite understand the sentence "I'm hesitant to remove the firing.."
> As explained in Bug 1121882, our current behavior doesn't match the spec nor
> other browsers.

I disagreed with your claim that it doesn't match the spec; there's no spec for what happens on layout flush that I'm aware of -- and certainly not a complete one -- so the absence of a spec saying to do something during a layout flush doesn't mean that it shouldn't happen.

What does IE do here?  Does it match Gecko or match WebKit+Blink?

Comment 14

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to Olli Pettay [:smaug] from comment #12)
> The O(n^2) is what other refreshdriver stuff is doing too, unfortunately.
> I guess we should fix them all.

OK, probably a separate patch for that, then.

Comment 15

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

The spec says when the resize event fires. What else should it say? The spec is pretty close to blink and webkit, though there is https://code.google.com/p/chromium/issues/detail?id=472247.

Comment 16

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

Hmm, looking at blink code, perhaps there are some more differences to spec.
But I think the spec makes sense.

Comment 17

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

The spec says that the resize event fires during the refresh cycle.  It doesn't say that that's the only time it fires.  (And given that I don't think there's a spec for what I think is the other time it fires, I think this is a reasonable argument.)

Comment 18

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

IE doesn't fire resize synchronously either

Comment 19

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

OK, in that case I guess it makes sense to stop firing synchronously.

Comment 20

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8591394 [details] [diff] [review]
v3 (more test fixes)

... so r=dbaron with comments from comment 11

Comment 21

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

(In reply to David Baron [:dbaron] ⏰UTC+2 from comment #11) 
> browser/base/content/test/social/browser_social_chatwindow_resize.js:
> 
> Could you explain the changes in this test?
We may now merge several resizes to just one event.

Comment 22

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

Attachment: v4

https://treeherder.mozilla.org/#/jobs?repo=try&revision=aec1bd0d28ac

Comment 23

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

Attachment: up-to-date (v5)

Comment 24

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f58f60cd772c

Comment 25

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/98839fa23be9

Comment 26

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

This made a test unexpectedly pass on OSX 10.10, so smaug said to just skip that test there:
https://hg.mozilla.org/integration/mozilla-inbound/rev/98839fa23be9


https://treeherder.mozilla.org/logviewer.html#?job_id=9578284&repo=mozilla-inbound


Not sure if you want to file a followup to reenable the test or not.

Comment 27

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

I guess I should try to re-enable that on 10.10 by pushing first to try and building 10.10 explicitly there.

Comment 28

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f58f60cd772c
https://hg.mozilla.org/mozilla-central/rev/98839fa23be9

Comment 29

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/4ff5e8facd37

Backed out because of bug 1162081 (and don't want that to be in Aurora).

Comment 30

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/4ff5e8facd37

Comment 31

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

as a note, this appears to be the root cause of bug 1161915.  I did a bunch of retriggers:
https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&filter-searchStr=Windows%20XP%2032-bit%20mozilla-inbound%20pgo%20test%20reftest&startdate=2015-05-04&enddate=2015-05-05

Since this is backed out, there isn't much to do in the other bug.

Comment 32

[Avi Halachmi (:avih)]

(In reply to Joel Maher (:jmaher) from comment #31)
> as a note, this appears to be the root cause of bug 1161915.

That's a non-performance issue, likely due to the new behavior. The test should either be modified to accommodate the new behavior, or, it might be related to the later first paint. See below.

> I did a bunch of retriggers:
> https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&filter-
> searchStr=Windows%20XP%2032-bit%20mozilla-
> inbound%20pgo%20test%20reftest&startdate=2015-05-04&enddate=2015-05-05
> 
> Since this is backed out, there isn't much to do in the other bug.

Bug 1162081 shows 7-15% "paint" regressions on few platforms. This test measures how long it takes till the first paint on a new window.

Personally I don't think it's horrible, but if there's some low hanging fruit by firing the first resize event earlier somehow, IMO it's worth a go.

dbaron, smaug, is the delayed first paint an inherent derivative of the new behavior? or do you think it could be possible to get the first event out earlier?

Comment 33

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

I don't know why first paint would happen later. Hmm, well, actually, if we do something heavy in the first resize handling (which might in theory be fired right before the first paint), then the first paint would be delayed.

Comment 34

[Mats Palmgren (inactive)]

(Fwiw, bug 881832 is somewhat related as well.)

Comment 35

[Mats Palmgren (inactive)]

Attachment: up-to-date (v6)

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a6d58a5c98e89df15d76addd6f319503fb961ae3

Olli, you might want to glance over this since there's been some changes
in the surrounding code since the last time.

Comment 36

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

Comment on attachment 8918602 [details] [diff] [review]
up-to-date (v6)

>+  // Resize events should be fired before layout flushes or
>+  // calling animation frame callbacks.
>+  AutoTArray<nsIPresShell*, 16> observers;
>+  observers.AppendElements(mResizeEventFlushObservers);
>+  for (uint32_t i = observers.Length(); i; --i) {
>+    if (!mPresContext || !mPresContext->GetPresShell()) {
>+      break;
>+    }
>+    // Make sure to not process observers which might have been removed
>+    // during previous iterations.
>+    nsIPresShell* shell = observers[i - 1];
>+    if (!mResizeEventFlushObservers.Contains(shell)) {
>+      continue;
>+    }
>+    mResizeEventFlushObservers.RemoveElement(shell);
>+    shell->FireResizeEvent();
>+  }
>+
I think this is too early in the method. It should be where it is in the v5 patch.

>+++ b/layout/reftests/font-face/ex-unit-1-dynamic.html
>@@ -10,11 +10,11 @@ body { font-family: Ahhhem; font-size: 5
> <script type="application/ecmascript">
> function run() {
>   document.getElementsByTagName("iframe")[0].contentWindow.arm();
>   document.getElementsByTagName("style")[0].sheet.insertRule(
>     '@font-face { font-family: "Ahhhem"; src: url(../fonts/Ahem.ttf); }',
>     0);
> }
> </script>
>-<body onload="setTimeout(run, 0)">
>+<body onload="setTimeout(run, 20)">

I can't now recall why I wrote it this way. Should it possibly be requestAnimationFrame(function() { setTimeout(run, 0)})

Comment 37

[Mats Palmgren (inactive)]

Attachment: up-to-date (v7)

(In reply to Olli Pettay [:smaug] (ni?s and f?s processed after r?s) from comment #36)
> I think this is too early in the method. It should be where it is in the v5
> patch.

Hmm, it's in the exact same spot as in v5 AFAICT.
Am I misreading it somehow?

> >+++ b/layout/reftests/font-face/ex-unit-1-dynamic.html
> >-<body onload="setTimeout(run, 0)">
> >+<body onload="setTimeout(run, 20)">
> 
> I can't now recall why I wrote it this way. Should it possibly be
> requestAnimationFrame(function() { setTimeout(run, 0)})

I'll file a follow-up bug on it.

This time around there's a permafail on the test:
devtools/client/shared/test/browser_graphs-07e.js
https://treeherder.mozilla.org/#/jobs?repo=try&revision=bd103a4807d3f74db31f372ec514bb7e8f906d66
It looks like a bug in the test, so I just disabled it -
I'll file a follow-up bug on that one too.

Seriously, I think we should just land this now to stop this endless
ping-pong of bit-rotted patches and new bogus tests failing...
I really don't think there's anything wrong with the patch.

Comment 39

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

(In reply to Mats Palmgren (:mats) from comment #37)
> Created attachment 8935118 [details] [diff] [review]
> up-to-date (v7)
> 
> (In reply to Olli Pettay [:smaug] (ni?s and f?s processed after r?s) from
> comment #36)
> > I think this is too early in the method. It should be where it is in the v5
> > patch.
> 
> Hmm, it's in the exact same spot as in v5 AFAICT.
> Am I misreading it somehow?
It is not at the same place, because more code has been added there.
https://searchfox.org/mozilla-central/rev/f5f1c3f294f89cfd242c3af9eb2c40d19d5e04e7/layout/base/nsRefreshDriver.cpp#1818-1832
Especially mEarlyRunners should run before mResizeEventFlushObservers.


> Seriously, I think we should just land this now to stop this endless
> ping-pong of bit-rotted patches and new bogus tests failing...
> I really don't think there's anything wrong with the patch.
Yeah, only that mEarlyRunners case need to be fixed.

Comment 40

[Mats Palmgren (inactive)]

Attachment: 8935118: up-to-date (v8)

Comment 41

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

of course there shouldn't be need to update IID anymore.

Comment 42

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b8174ee90b58
Update resize event firing to follow the specs, dispatch right before rAF callbacks, r=dbaron

Comment 43

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/b8174ee90b58