Closed Bug 1443380 Opened 2 years ago Closed 2 years ago

We don't need to mark frames modified when we recurse into InvalidateFrame

Categories

(Core :: Web Painting, enhancement, P2)

enhancement

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox60 --- wontfix
firefox61 --- fixed

People

(Reporter: mattwoodrow, Assigned: mattwoodrow)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

There are two cases where this happens:

Table invalidation - Whenever we get an invalidation on a border-collapse table, we bubble the InvalidateFrame call up to the table root so that the nsTableBorderCollapse (that paints the backgrounds/borders for the table sub parts) receives the appropriate invalidation.
We only need the initial frame in the display list modified list, since building items for that frame will descend through the table root and build the nsTableBorderCollapse.

InvalidateFrameSubtree - This calls InvalidateFrame recursively on all descendants. We only need the initial frame modified, as we build all display items within the overflow rect, which includes all descendants.

Both of these cases occur on gmail, the former causes us to rebuild the entire mail list for every change within it (it's all one table), the latter causes us to hit our modified frame limit and rebuild the whole page.
Attachment #8956312 - Flags: review?(mikokm)
Comment on attachment 8956312 [details] [diff] [review]
dont-modify-all-descendants

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

LGTM.

::: layout/tables/nsTableCellFrame.cpp
@@ +437,5 @@
>    // don't overflow the cell
>    return nsDisplayItem::GetBounds(aBuilder, aSnap);
>  }
>  
> +void nsTableCellFrame::InvalidateFrame(uint32_t aDisplayItemKey, bool aRebuildDisplayItems)

InvalidateFrame() and InvalidateFrameWithRect() functions for nsTableCellFrame, nsTableColFrame, nsTableColGroupFrame, nsTableRowFrame, and nsTableRowGroupFrame seem to be same.

If it's not too much work, it would be nice to get rid of this duplicated code, or at least consolidate it in one place.
Attachment #8956312 - Flags: review?(mikokm) → review+
Pushed by mwoodrow@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/36e0be4f9c14
Don't mark all descendant frames modified for display list building when invalidating a frame subtree since marking just the root is sufficient. r=miko
https://hg.mozilla.org/mozilla-central/rev/36e0be4f9c14
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in before you can comment on or make changes to this bug.