1367747 - should only append display items for table borders when there are borders to draw

Status: RESOLVED FIXED

(Core :: Layout: Tables, enhancement)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 1

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

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

Comment 2

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: Create nsDisplayBorder when there are borders to draw.

MozReview-Commit-ID: H5PYj8ErRto

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 4

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

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

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 6

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

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

Comment 7

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: Create nsDisplayBorder when there are borders to draw.

Markus, I slightly modified this patch. Can you review it again?

MozReview-Commit-ID: H5PYj8ErRto

Comment 8

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: Create nsDisplayTableBorderCollapse when there is collapse border to draw.

MozReview-Commit-ID: 29g6HqAn0ej

Comment 9

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a9d25e5cf19ce3a226a2bcc72d756aef9cfc1e14

Comment 10

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Looks like some tests failed, pending review request.

Comment 11

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3b7391a1b5f82364a201efc9226ea75299d76598&selectedJob=104305799

latest try looks good. Re-request review again.

Comment 12

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: Create nsDisplayBorder when there are borders to draw.

MozReview-Commit-ID: H5PYj8ErRto

Comment 13

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: Create nsDisplayTableBorderCollapse when there is collapse border to draw.

MozReview-Commit-ID: 29g6HqAn0ej

Comment 14

[Markus Stange [:mstange]]

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 15

[Markus Stange [:mstange]]

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 :)

Comment 16

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: Create nsDisplayBorder when there are borders to draw. (carry r+: mstange)

Use previous r+ed patch.

MozReview-Commit-ID: H5PYj8ErRto

Comment 17

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: Create nsDisplayTableBorderCollapse when there is collapse border to draw. (carry r+: mstange)

Addressed comments.

MozReview-Commit-ID: 29g6HqAn0ej

Comment 18

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

(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.

Comment 19

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

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

Comment 20

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Try looks ok.

Comment 21

[Pulsebot]

Pushed by [e-mail]:
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

Comment 22

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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