Closed Bug 1234725 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: Layout, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla46
Tracking Status
firefox46 --- fixed

People

(Reporter: tnikkel, Assigned: tnikkel)

References

Details

Attachments

(1 file)

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.
My patch is quite orange on try server. debugging.
Attached patch patchSplinter Review
Attachment #8701674 - Flags: review?(mstange)
Comment on attachment 8701674 [details] [diff] [review]
patch

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

Looks good. Sorry for the delay.
Attachment #8701674 - Flags: review?(mstange) → review+
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)?
(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.
Does that answer your question?
Flags: needinfo?(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.
Flags: needinfo?(mstange)
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.
https://hg.mozilla.org/mozilla-central/rev/f99c6b7ad44f
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
Product: Core → Core Graveyard
Component: Layout: Misc Code → Layout
Product: Core Graveyard → Core
You need to log in before you can comment on or make changes to this bug.