1234725 - don't change the dirty rect to the display port when building display lists for documents

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Timothy Nikkel (:tnikkel)]

Instead do it when we first encounter the root scroll frame.

Doing this goes back to bug 635053, where we did it because fixed position items weren't getting included. However in bug 974643 we learned that this was wrong. Displayports aren't relevant to fixed pos content, displayports are only relevant to scrolled content. And we set the dirty rects of fixed pos content specially. The only other thing that should be affected is scrollbars, and we already carefully set their dirty rects too.

So we should stop doing this. The patch for bug 1214261 needs this.

Comment 1

[Timothy Nikkel (:tnikkel)]

My patch is quite orange on try server. debugging.

Comment 2

[Timothy Nikkel (:tnikkel)]

Attachment: patch

Comment 3

[Timothy Nikkel (:tnikkel)]

I think it's green, a lot of intermittents though.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6a1f315e319f
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0480a1500d63

Comment 4

[Markus Stange [:mstange]]

Comment on attachment 8701674 [details] [diff] [review]
patch

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

Looks good. Sorry for the delay.

Comment 5

[Markus Stange [:mstange]]

Comment on attachment 8701674 [details] [diff] [review]
patch

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

Looks good. Sorry for the delay.

::: layout/generic/nsGfxScrollFrame.cpp
@@ +3262,5 @@
> +      // The displayPort getter takes care of adjusting for resolution. So if
> +      // we have resolution but no displayPort then we need to adjust for
> +      // resolution here.
> +      nsIPresShell* presShell = mOuter->PresContext()->PresShell();
> +      *aDirtyRect = aDirtyRect->RemoveResolution(

Ok I'll bite. Who consumes the value that we set *aDirtyRect to? Is it only used for the visible rect of the display item created in AddCanvasBackgroundColorItem (which we call from nsSubDocumentFrame::BuildDisplayList while an AutoBuildingDisplayList with the modified dirtyRect is on the stack)?

Comment 6

[Timothy Nikkel (:tnikkel)]

(In reply to Markus Stange [:mstange] from comment #5)
> Comment on attachment 8701674 [details] [diff] [review]
> patch
> 
> Review of attachment 8701674 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good. Sorry for the delay.
> 
> ::: layout/generic/nsGfxScrollFrame.cpp
> @@ +3262,5 @@
> > +      // The displayPort getter takes care of adjusting for resolution. So if
> > +      // we have resolution but no displayPort then we need to adjust for
> > +      // resolution here.
> > +      nsIPresShell* presShell = mOuter->PresContext()->PresShell();
> > +      *aDirtyRect = aDirtyRect->RemoveResolution(
> 
> Ok I'll bite. Who consumes the value that we set *aDirtyRect to? Is it only
> used for the visible rect of the display item created in
> AddCanvasBackgroundColorItem (which we call from
> nsSubDocumentFrame::BuildDisplayList while an AutoBuildingDisplayList with
> the modified dirtyRect is on the stack)?

I'm a bit confused by your question. The variable |dirty| in nsSubDocumentFrame::BuildDisplayList is used in a few more places, the important one is the subdocRootFrame->BuildDisplayListForStackingContext() call. However DecideScrollableLayer doesn't modify |dirty| in nsSubDocumentFrame::BuildDisplayList, we pass a copy specifically so that it doesn't get modified.

DecideScrollableLayer is called in two other places: nsLayoutUtils::PaintFrame and ScrollFrameHelper::BuildDisplayList. We use a copy of the dirty rect that we never look at again in nsLayoutUtils::PaintFrame. In ScrollFrameHelper::BuildDisplayList the dirty rect that DecideScrollableLayer modifies is used as the main dirty rect (both in the ignore scroll frame case, and the normal case).

However when looking over the code again I did notice a bug: we should only be removing the resolution if we are the root scroll frame. I'll fix that.

Comment 7

[Timothy Nikkel (:tnikkel)]

Does that answer your question?

Comment 8

[Markus Stange [:mstange]]

Oops. Yes it answers my question. I thought the "*aDirtyRect = aDirtyRect->RemoveResolution..." line was in BuildDisplayList, not in DecideScrollableLayer. Now it makes sense to me.

Comment 9

[Timothy Nikkel (:tnikkel)]

I realized I should also remove this hunk

http://mxr.mozilla.org/mozilla-central/source/layout/base/nsPresShell.cpp?rev=046479939ba1#5686

like the hunk in nsSubDocumentFrame. So I incorporated that change into my patch.

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f99c6b7ad44f

Comment 11

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/f99c6b7ad44f