800859 - Make MozAfterPaint fire after composition

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Dão Gottwald [:dao]]

From bug 715402 comment 77:

> > > > Currently MozAfterPaint fires when we've finished drawing the window
> > > > contents to the layer tree, but before we composite that to the window. But
> > > > let's fix that.
> > > 
> > > Actually, let's not, since MozAfterPaint was designed to tell us about which
> > > areas of the window have been redrawn, so it needs to fire after they've
> > > been drawn and before new dirty areas are accumulated.
> > 
> > That's not really what most MozAfterPaint consumers I've seen want. What
> > also seems worrisome to me here is that talos makes extensive use of
> > MozAfterPaint, apparently missing an important last step.
> 
> OK.
> 
> > > What you really want here is something to tell you when the window has been
> > > composited, and we have no event for that. Plus we're driving towards having
> > > that happen off the main thread, which makes this tricky.
> > 
> > When composition happens off the main thread, a MozAfterPaint listener won't
> > block it and we won't care about it as far as this bug is concerned, right?
> 
> You probably still want to delay your delayedStartup work until after the
> compositor thread has composited the window, since the two threads can still
> contend for resources.
> 
> I *suppose* we could make MozAfterPaint fire after the composite, carrying a
> list of the rectangles we repainted in the paint phase before the composite,
> even if new stuff has been invalidated between the paint and the composite.
> Maybe we should do that.

Comment 1

[(dormant account)]

p1 since it blocks a p1.

Comment 2

[Dão Gottwald [:dao]]

(In reply to Taras Glek (:taras) from comment #1)
> p1 since it blocks a p1.

Also the fact that this affects talos might be enough to make this a p1...

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Cautious try push first:
https://tbpl.mozilla.org/?tree=Try&rev=0130b70b25c0

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 1: Refactor PaintType into two independent flags

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 2: Remove nsIPresShell::DidPaint

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 3: Dispatch MozAfterPaint after we've done the COMPOSITE step for the invalidations recorded in the event

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

These patches change invariants slightly ... right after a MozAfterPaint fires, DOMWindowUtils.isMozAfterPaintPending can still return true, since we could have a pending paint event that hasn't been composited yet. I think that's OK.

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

This seems to have exposed a number of tests that expect subdocuments that aren't toplevel content documents to receive MozAfterPaint events.

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Also, dom/browser-element/BrowserElementChild.js seems to require MozAfterPaint to fire on arbitrary subdocuments.

The strange thing is that I thought we disabled that in DLBI already, restricting MozAfterPaint to toplevel documents and toplevel content documents only.

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Try to make MozAfterPaint work in all kinds of subdocuments:
https://tbpl.mozilla.org/?tree=Try&rev=8d660b0a8b2b

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

That was green!
Trying all platforms:
https://tbpl.mozilla.org/?tree=Try&rev=53f6e9745676

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 3 v2

Tightens up handling of mAllInvalidated to avoid bugs/assertions.

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 4: Enable MozAfterPaint for all subdocuments

This turns out to be super easy...

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Try results look good.

Comment 15

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 673498 [details] [diff] [review]
Part 3 v2

Review of attachment 673498 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/nsPresContext.cpp
@@ +2271,5 @@
>    }
>  
> +  if (!mFireAfterPaintEvents) {
> +    return;
> +  }

This change here is why we need part 4. Previously we were only checking for mFireAfterPaintEvent on the root pres context, and if true we would fire MozAfterPaint for all subdocuments (even though they wouldn't have any rects stored).

Comment 16

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 673499 [details] [diff] [review]
Part 4: Enable MozAfterPaint for all subdocuments

Review of attachment 673499 [details] [diff] [review]:
-----------------------------------------------------------------

Patch looks good, but are we sure that we want to do this? The extra compositing cost on BasicLayers might be an issue, not really convinced it's a problem though.

When we discussed this originally, we decided that it would be better to just send empty MozAfterPaint events to all subdocuments.

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Matt Woodrow (:mattwoodrow) from comment #15)
> This change here is why we need part 4. Previously we were only checking for
> mFireAfterPaintEvent on the root pres context, and if true we would fire
> MozAfterPaint for all subdocuments (even though they wouldn't have any rects
> stored).

True. However, part 4 also ensures we compute the correct invalidation rects for all subdocuments, which we weren't doing before.

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Matt Woodrow (:mattwoodrow) from comment #16)
> Patch looks good, but are we sure that we want to do this? The extra
> compositing cost on BasicLayers might be an issue, not really convinced it's
> a problem though.
> When we discussed this originally, we decided that it would be better to
> just send empty MozAfterPaint events to all subdocuments.

Now that you mention it, we don't want to do this. I had been thinking it would only force subdocuments to have layers when they have MozAfterPaint listeners registered on them, but of course if there's a MozAfterPaint listener registered on a window then nsPresContext::MayHavePaintEventListener will return true for all its descendants. So a single MozAfterPaint listener on a toplevel content document would layerize all its IFRAMEs, which could be bad.

Comment 19

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 4 alternative: dispatch MozAfterPaint unconditionally to all subdocuments

Comment 20

[Robert O'Callahan (:roc) (email my personal email if necessary)]

https://tbpl.mozilla.org/?tree=Try&rev=744ea1f34d52

Comment 21

[Robert O'Callahan (:roc) (email my personal email if necessary)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6e108817d0c5
https://hg.mozilla.org/integration/mozilla-inbound/rev/1bf5c18ae382
https://hg.mozilla.org/integration/mozilla-inbound/rev/74f3a93b9283
https://hg.mozilla.org/integration/mozilla-inbound/rev/bf04510e09ee

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6e108817d0c5
https://hg.mozilla.org/mozilla-central/rev/1bf5c18ae382
https://hg.mozilla.org/mozilla-central/rev/74f3a93b9283
https://hg.mozilla.org/mozilla-central/rev/bf04510e09ee