PaintedLayerData tracks both a "draw region" and a "visible region". The draw region is the union of the clipped bounds of the display items in the PaintedLayer, and the visible region is the union of the clipped bounds intersected with the "visible rect" of the display items. Ever since bug 1022612, this visible rect is just the dirty rect that the display item has been initialized with. I think that after bug 1022612 we can reduce the duplication here by ignoring the draw region and always using the visible region instead. This would make things slightly faster (fewer region operations), easier to read, and would avoid bugs like bug 1120431. There are two places that make use of the draw region: FindPaintedLayerFor and AdjustLayerDataForFixedPositioning. FindPaintedLayerFor checks the intersection of the visible rect of the to-be-added display item with both the "draw region" and the "draw above region" of the candidate layers. This comment in ProcessDisplayItems explains why: // Add the entire bounds rect to the mDrawAboveRegion. // The visible region may be excluding opaque content above the // item, and we need to ensure that that content is not placed // in a PaintedLayer below the item! data->AddDrawAboveRegion(itemDrawRect); So it's basically working around display list occlusion culling that we did before bug 1022612. Now that we're no longer doing that, we should just be able to use the visible region, right? AdjustLayerDataForFixedPositioning is more tricky, or at least confusing to me. It attempts to expand the visible region, and then intersects the enlarged region with the draw region. Is this also working around overeager occlusion culling (which we're no longer doing)? If it's still making a difference, aren't we painting bad content in the parts that we've added to the visible region, since there might be display items in those parts which we haven't added to the display list? (I also don't understand why that function sets the visible region of background-attachment:fixed items to be the displayport, and not the viewport.) Please check my reasoning. This patch passes reftests.

(In reply to Markus Stange [:mstange] from comment #0) > AdjustLayerDataForFixedPositioning is more tricky, or at least confusing to > me. It attempts to expand the visible region, and then intersects the > enlarged region with the draw region. Is this also working around overeager > occlusion culling (which we're no longer doing)? I think it's trying to work around display item visible-rects being restricted to the viewport when additional content might be visible in the displayport --- which was true, but no longer is. > If it's still making a > difference, aren't we painting bad content in the parts that we've added to > the visible region, since there might be display items in those parts which > we haven't added to the display list? Right. I think nsDisplayListBuilder::MarkOutOfFlowFrameForDisplay handles this properly now. > (I also don't understand why that function sets the visible region of > background-attachment:fixed items to be the displayport, and not the > viewport.) > > Please check my reasoning. This patch passes reftests. I think you're doing the right thing.

Comment on attachment 8566129 [details] [diff] [review] v1 Review of attachment 8566129 [details] [diff] [review]: ----------------------------------------------------------------- This is excellent!

Yay :-)