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.

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?

(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.

(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.

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.

(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 on attachment 466233 [details] [diff] [review] patch v2 This looks fine though.

(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.

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.

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?

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.

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