I noticed, looking at the new table display item generation code from bug 929484, that it appends display items for table borders whether or not there are actually borders to draw. This causes creation of more display items than needed, and isn't the way we normally generate border display items. It may well make the various display item optimizations less effective. It seems like the code that generates these display items in nsTableFrame::DisplayGenericTablePart should only generate them when there are actually borders. (The code in nsTableCellFrame::ProcessBorders does the right thing for table cell borders.) For the non-border-collapse case this should be easy, since it requires only checking the border on the table. For the border-collapse case, it's harder since it requires testing for border on every table part. However, I think such testing would also be useful for other things. A few days ago I saw a bug with discussion of making the special table invalidation code that was needed because we couldn't depend on DLBI conditional on not being border-collapse. (I can't find the bug right now, though!) If we had a boolean for whether the table actually *had* borders, we could make it conditional on being border-collapse and having borders.

Sorry for late reply, thanks for the suggestion. I'll do this.

MozReview-Commit-ID: H5PYj8ErRto

Are you planning to address the border-collapse part in this bug, or should there be a separate bug for that?

The codes that generating border are only consider table frame and not other table parts (see [1]). And we also create border-collapse display item in [2]. Do we need address border-collapse here? [1]: https://searchfox.org/mozilla-central/source/layout/tables/nsTableFrame.cpp#1489-1498 [2]: https://searchfox.org/mozilla-central/source/layout/tables/nsTableFrame.cpp#1492-1494

See comment 0 about the value. The border-collapse case is one where we have to disable a bunch of optimization, so I'd prefer not to do so for a border-collapse table that doesn't actually have borders.

Ah, I understood. I'll fix the border-collapse part in this bug as well.

Markus, I slightly modified this patch. Can you review it again? MozReview-Commit-ID: H5PYj8ErRto

MozReview-Commit-ID: 29g6HqAn0ej

Looks like some tests failed, pending review request.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3b7391a1b5f82364a201efc9226ea75299d76598&selectedJob=104305799 latest try looks good. Re-request review again.

MozReview-Commit-ID: H5PYj8ErRto

MozReview-Commit-ID: 29g6HqAn0ej

Comment on attachment 8874185 [details] [diff] [review] Create nsDisplayBorder when there are borders to draw. Review of attachment 8874185 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/tables/nsTableFrame.cpp @@ +1501,1 @@ > if (aFrame->IsVisibleForPainting(aBuilder)) { Doesn't this condition already check the visibility? I think you can go back to the patch I r+ed previously and just create a separate patch to remove the dead function FrameHasBorderOrBackground.

Comment on attachment 8874186 [details] [diff] [review] Create nsDisplayTableBorderCollapse when there is collapse border to draw. Review of attachment 8874186 [details] [diff] [review]: ----------------------------------------------------------------- Iterating over everything seems a bit expensive but it's probably what we need to do. It would be nice if we could reuse some work that we're already doing in order to compute the result here, but I don't know the code well enough to assess whether that's possible. ::: layout/tables/nsTableFrame.cpp @@ +1424,5 @@ > return false; > } > > +void nsTableFrame::CalcHasBCBorders() > +{ Can you add an early check here that handles the case where IsBorderCollape() is false? I know that at the moment, this function is only called if IsBorderCollape() is true, but new callers without such a check might be added in the future. @@ +1425,5 @@ > } > > +void nsTableFrame::CalcHasBCBorders() > +{ > + if (FrameHasBorder(this)) { I see now, so this is where you need the visibility check. You can put the FrameHasBorder function from the previous patch into this patch :)

Use previous r+ed patch. MozReview-Commit-ID: H5PYj8ErRto

Addressed comments. MozReview-Commit-ID: 29g6HqAn0ej

(In reply to Markus Stange [:mstange] from comment #15) > Comment on attachment 8874186 [details] [diff] [review] > Create nsDisplayTableBorderCollapse when there is collapse border to draw. > > Review of attachment 8874186 [details] [diff] [review]: > ----------------------------------------------------------------- > > Iterating over everything seems a bit expensive but it's probably what we > need to do. It would be nice if we could reuse some work that we're already > doing in order to compute the result here, but I don't know the code well > enough to assess whether that's possible. > I hope we can reuse this function in some frame invalidation check. Or we can remove this function once border collapses are rendered by each cell.

Latest try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8e9ef02841a83b6b0b4b6294f8e932d01c3341e1

Try looks ok.

Pushed by mtseng@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/370809b43a59 Create nsDisplayBorder when there are borders to draw. r=mstange https://hg.mozilla.org/integration/mozilla-inbound/rev/4ab3d69a2cf0 Create nsDisplayTableBorderCollapse when there is collapse border to draw. r=mstange

https://hg.mozilla.org/mozilla-central/rev/370809b43a59 https://hg.mozilla.org/mozilla-central/rev/4ab3d69a2cf0