Closed Bug 1384387 Opened 7 years ago Closed 7 years ago

should call ClearInvalidationStateBits in layers-free mode

Categories

(Core :: Graphics: WebRender, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: ethlin, Assigned: ethlin)

References

Details

Attachments

(1 file)

We check nsDisplayItem::IsInvalid() to know if we need to repaint the item. We should clear the invalidation bit after each transaction for frames or the frames will keep having the NS_FRAME_NEEDS_PAINT bit.

[1] https://dxr.mozilla.org/mozilla-central/source/layout/painting/nsDisplayList.h#1757
Comment on attachment 8890184 [details]
Bug 1384387 - Clear frame's invalidation state bits in layers-free mode.

https://reviewboard.mozilla.org/r/161254/#review166804

Mostly a rubberstamp since I don't know what this code is doing but since it's a copy-paste from the non-WR codepath it seems reasonable enough.

::: layout/painting/nsDisplayList.cpp:2111
(Diff revision 1)
> +    if (widgetTransaction ||
> +        // SVG-as-an-image docs don't paint as part of the retained layer tree,
> +        // but they still need the invalidation state bits cleared in order for
> +        // invalidation for CSS/SMIL animation to work properly.
> +        (document && document->IsBeingUsedAsImage())) {
> +      frame->ClearInvalidationStateBits();

In the non-WR codepath this happens before calling EndTransaction. Is there a reason you're doing after calling EndTransaction here? If so that reason should be documented somewhere. If there's no reason then I'd prefer doing it before EndTransaction for consistency - if we ever do end up unifying the non-WR codepath with this then having the same ordering makes it one less thing to check.
Attachment #8890184 - Flags: review?(bugmail) → review+
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #2)
> Comment on attachment 8890184 [details]
> Bug 1384387 - Clear frame's invalidation state bits in layers-free mode.
> 
> https://reviewboard.mozilla.org/r/161254/#review166804
> 
> Mostly a rubberstamp since I don't know what this code is doing but since
> it's a copy-paste from the non-WR codepath it seems reasonable enough.
> 
> ::: layout/painting/nsDisplayList.cpp:2111
> (Diff revision 1)
> > +    if (widgetTransaction ||
> > +        // SVG-as-an-image docs don't paint as part of the retained layer tree,
> > +        // but they still need the invalidation state bits cleared in order for
> > +        // invalidation for CSS/SMIL animation to work properly.
> > +        (document && document->IsBeingUsedAsImage())) {
> > +      frame->ClearInvalidationStateBits();
> 
> In the non-WR codepath this happens before calling EndTransaction. Is there
> a reason you're doing after calling EndTransaction here? If so that reason
> should be documented somewhere. If there's no reason then I'd prefer doing
> it before EndTransaction for consistency - if we ever do end up unifying the
> non-WR codepath with this then having the same ordering makes it one less
> thing to check.

I have to do this after EndTransaction. For layers mode, FrameLayerBuilder checks the flags and set the invalidation regions to layers, so we can clear the bits before EndTransaction. For layers-free mode, everything happens in EndTransaction so we must clear the bits after EndTransaction. I will add comments above the changes.
Pushed by ethlin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4415d55c49f7
Clear frame's invalidation state bits in layers-free mode. r=kats
https://hg.mozilla.org/mozilla-central/rev/4415d55c49f7
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.