Closed Bug 1149555 Opened 9 years ago Closed 6 years ago

Update resize event firing to follow the specs

Categories

(Core :: Layout, defect)

36 Branch
x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox59 --- fixed

People

(Reporter: smaug, Assigned: smaug)

References

(Depends on 1 open bug, Blocks 2 open bugs)

Details

(Keywords: sec-want, Whiteboard: [adv-main59-])

Attachments

(6 files, 2 obsolete files)

No longer blocks: 1121882
Blocks: 1121882
Assignee: nobody → bugs
Attached patch v2 (obsolete) — Splinter Review
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
Attachment #8586368 - Attachment is obsolete: true
Attachment #8586451 - Flags: review?(dbaron)
There are still couple of test failures, looks like broken tests.
Could you summarize what it is that you're changing?
Flags: needinfo?(bugs)
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
Flags: needinfo?(bugs)
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)
Attachment #8586451 - Attachment is obsolete: true
Attachment #8586451 - Flags: review?(dbaron)
Attachment #8591394 - Flags: review?(dbaron)
Review ping.
Flags: needinfo?(dbaron)
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.
Flags: needinfo?(dbaron) → needinfo?(bugs)
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.
Flags: needinfo?(bugs)
(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?
(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.
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.
Hmm, looking at blink code, perhaps there are some more differences to spec.
But I think the spec makes sense.
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.)
Attachment #8591394 - Flags: review?(dbaron)
IE doesn't fire resize synchronously either
Assignee: bugs → nobody
OK, in that case I guess it makes sense to stop firing synchronously.
Comment on attachment 8591394 [details] [diff] [review]
v3 (more test fixes)

... so r=dbaron with comments from comment 11
Attachment #8591394 - Flags: review+
(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.
Blocks: 1161218
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.
Flags: needinfo?(bugs)
I guess I should try to re-enable that on 10.10 by pushing first to try and building 10.10 explicitly there.
https://hg.mozilla.org/mozilla-central/rev/f58f60cd772c
https://hg.mozilla.org/mozilla-central/rev/98839fa23be9
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Blocks: 1162081
https://hg.mozilla.org/integration/mozilla-inbound/rev/4ff5e8facd37

Backed out because of bug 1162081 (and don't want that to be in Aurora).
Status: RESOLVED → REOPENED
Flags: needinfo?(bugs)
Resolution: FIXED → ---
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.
(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?
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.
Target Milestone: mozilla40 → ---
(Fwiw, bug 881832 is somewhat related as well.)
Keywords: sec-want
Attached patch up-to-date (v6)Splinter Review
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.
Attachment #8918602 - Flags: feedback?(bugs)
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)})
Attachment #8918602 - Flags: feedback?(bugs) → feedback+
Attached patch up-to-date (v7)Splinter Review
(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.
Attachment #8935118 - Flags: feedback?(bugs)
(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.
Attachment #8935118 - Flags: feedback?(bugs)
Attachment #8935440 - Flags: feedback?(bugs)
Attachment #8935440 - Flags: feedback?(bugs) → feedback+
of course there shouldn't be need to update IID anymore.
Pushed by mpalmgren@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b8174ee90b58
Update resize event firing to follow the specs, dispatch right before rAF callbacks, r=dbaron
https://hg.mozilla.org/mozilla-central/rev/b8174ee90b58
Status: REOPENED → RESOLVED
Closed: 9 years ago6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Whiteboard: [adv-main59-]
You need to log in before you can comment on or make changes to this bug.