587542 - need to suppress view invalidates as well as frame invalidates when painting is suppressed

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Timothy Nikkel (:tnikkel)]

Attachment: patch

An invalidate that goes through nsIFrame::Invalidate* will be dropped if painting is suppressed in the presshell. But if we move around a view during reflow (or any thing else that causes UpdateView to be called that doesn't go through the frame invalidation functions) the resulting UpdateView call does not get dropped. This leads to ugly artifacts during page load with 130078.

Comment 1

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

Should we check all view observers up to the root here? It seems to me that we should.

Also, can we move this check out of UpdateView to callers? Othewise we're going to do it for UpdateView called by nsFrame::InvalidateRoot, which means we're effectively checking twice.

Why does doing extra invalidations cause artifacts?

Comment 2

[Timothy Nikkel (:tnikkel)]

Attachment: screenshot of artifact

(In reply to comment #1)
> Should we check all view observers up to the root here? It seems to me that we
> should.

I don't think we want to do that, that would mean that the UI would freeze while loading a content page.

> Also, can we move this check out of UpdateView to callers? Othewise we're going
> to do it for UpdateView called by nsFrame::InvalidateRoot, which means we're
> effectively checking twice.

Good point. I'll fix that.

> Why does doing extra invalidations cause artifacts?

I didn't describe it clearly. What happens is that the part of the page that got these view-only invalidations will get painted and the rest of the page does not. For example, loading http://build.mozilla.org/builds/pending/ the left approx two thirds of the page get painted, leaving the right third white, and then when painting is unsuppressed the right third gets filled in. I've attached a screenshot of this in action.

Comment 3

[Timothy Nikkel (:tnikkel)]

(In reply to comment #2)
> Created attachment 466197 [details]
> screenshot of artifact
> 
> (In reply to comment #1)
> > Should we check all view observers up to the root here? It seems to me that we
> > should.
> 
> I don't think we want to do that, that would mean that the UI would freeze
> while loading a content page.

Whoops, didn't think this through. I think you're right.

Comment 4

[Timothy Nikkel (:tnikkel)]

Attachment: patch v2

The one UpdateView call in nsIFrame::InvalidateRoot is the only place where we want to avoid the double check, so I think it makes most sense to create a new function for that to use and have all the other callers use the same name.

Comment 5

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

(In reply to comment #2)
> I didn't describe it clearly. What happens is that the part of the page that
> got these view-only invalidations will get painted and the rest of the page
> does not. For example, loading http://build.mozilla.org/builds/pending/ the
> left approx two thirds of the page get painted, leaving the right third white,
> and then when painting is unsuppressed the right third gets filled in. I've
> attached a screenshot of this in action.

That seems strange. When painting is suppressed, we should only paint the background of the suppressed document due to nsDisplayListBuilder::IsBackgroundOnly.

Comment 6

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

Comment on attachment 466233 [details] [diff] [review]
patch v2

This looks fine though.

Comment 7

[Timothy Nikkel (:tnikkel)]

(In reply to comment #5)
> That seems strange. When painting is suppressed, we should only paint the
> background of the suppressed document due to
> nsDisplayListBuilder::IsBackgroundOnly.

Even if we only painted the background it would still look ugly, so we need this patch regardless.

The reason the page gets drawn and not just the background is that nothing sets nsDisplayListBuilder::IsBackgroundOnly to true when we enter the subdocument (in this case the top level content document). I have a patch that does this, but it causes a bunch of failures on try server. I'm still hoping to get that patch in but it will have to wait.

Comment 8

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

Ah I see!(In reply to comment #7)
> The reason the page gets drawn and not just the background is that nothing sets
> nsDisplayListBuilder::IsBackgroundOnly to true when we enter the subdocument
> (in this case the top level content document). I have a patch that does this,
> but it causes a bunch of failures on try server. I'm still hoping to get that
> patch in but it will have to wait.

Ah of course! Good catch.

Comment 9

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

     4.5  #define NS_IVIEWOBSERVER_IID  \
     4.6    { 0xc5dfb460, 0x50fb, 0x483e, \
     4.7 -    { 0xb4, 0x22, 0x19, 0xb7, 0x20, 0x4f, 0xe2, 0xdc } }
     4.8 +    { 0xb4, 0x22, 0x19, 0xb7, 0x20, 0x4f, 0xe2, 0xdc } } //xxx

What happened here?

Comment 10

[Timothy Nikkel (:tnikkel)]

Oops! Thanks for catching that. I put that there to remind myself to bump the IID, but I forgot. I will push a followup to do so.

Comment 11

[Timothy Nikkel (:tnikkel)]

http://hg.mozilla.org/mozilla-central/rev/3b0081e69d1f

Comment 12

[Timothy Nikkel (:tnikkel)]

Bumped the IID
http://hg.mozilla.org/mozilla-central/rev/fbe2817bee1e