Closed Bug 528306 Opened 10 years ago Closed 10 years ago

[FIX]Hook up restyle processing to refresh driver

Categories

(Core :: Layout, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

(Depends on 1 open bug)

Details

Attachments

(3 files, 3 obsolete files)

Let's do it.
Attached patch Fix (obsolete) — Splinter Review
Attachment #412049 - Flags: review?(dbaron)
One possible issue there is removing every time we have no restyles to process.  Maybe we should remove on WillRefresh() instead, and readd after restyle processing if there's anything left there.  Otherwise it seems like we might thrash the removes/adds (including stopping/starting the timer) if something keeps posting and flushing over and over.

That's probably a better setup.  Will do that.
Ah, now I remember why I didn't do that.  It's because animations will add to the restyle table during their WillRefresh calls, and those can easily happen after the frame constructor's.  So we'll end up with the frame constructor observing the refresh driver though there are no pending restyles.

Will think about this after dinner.
Attachment #412049 - Attachment is obsolete: true
Attachment #412049 - Flags: review?(dbaron)
Attached patch Ready for review (obsolete) — Splinter Review
Attachment #412142 - Flags: review?(dbaron)
Attachment #412142 - Attachment is obsolete: true
Attachment #413413 - Flags: review?(dbaron)
Attachment #412142 - Flags: review?(dbaron)
Comment on attachment 413413 [details] [diff] [review]
Fix shutdown asserts from the refresh driver

I'm planning to change refresh driver a bit, so need to rev this, and would like to see bug 531585 land first anyway.
Attachment #413413 - Flags: review?(dbaron) → review-
Depends on: 531585
Attachment #413413 - Attachment is obsolete: true
Attachment #419593 - Flags: review?(dbaron)
Comment on attachment 419593 [details] [diff] [review]
Part 1: change when the refresh driver stops its timer

r=dbaron, but maybe add a comment in Notify explaining why you want to wait until you get to a notification and still don't have observers?
Attachment #419593 - Flags: review?(dbaron) → review+
Comment on attachment 419594 [details] [diff] [review]
Part 2: refcount the frame constructor

r=dbaron
Attachment #419594 - Flags: review?(dbaron) → review+
Comment on attachment 419595 [details] [diff] [review]
Part 3: Run restyles off the refresh driver

r=dbaron, but maybe add a comment to the WillRefresh implementation explaining that the actual work gets done when the refresh driver flushes style
Attachment #419595 - Flags: review?(dbaron) → review+
Made those two comment changes, and pushed:
  http://hg.mozilla.org/mozilla-central/rev/88dd8acfbc05
  http://hg.mozilla.org/mozilla-central/rev/6ef8b28b0d9b
  http://hg.mozilla.org/mozilla-central/rev/2e580c431f4e
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Pushed http://hg.mozilla.org/mozilla-central/rev/228adabcc071 trying to fix orange in an invalidation test.  Bug 537416 covers that orange.
Looks like this gave a Tdhml win of a few % (0.09 - 3.23 depending on OS and such).
Depends on: 537507
Depends on: 538082
Depends on: 675582
You need to log in before you can comment on or make changes to this bug.