Bug 99521 – Remove an unneeded repaint call in RenderTableCell::setCellLogicalWidth

• Status: RESOLVED
• Resolution: FIXED
• Component: Tables

[Julien Chaffraix]

This repaint call is superseeded by other repaints in the table layout code which always take care of properly repainting.

Patch forthcoming.

[Julien Chaffraix]

Created attachment 169051 [details]
Proposed removal, includes testing as I didn't find any coverage for such repainting.

[Eric Seidel (no email)]

Our repaint coverage is very poor.

[Simon Fraser (smfr)]

I want to make it possible to test repaints without pixel testing.

[Julien Chaffraix]

(In reply to comment #2)
> Our repaint coverage is very poor.

Yeah, that's why I took some time to investigate what is covering for this repaint and added some testing to prove that cells that didn't change logical width (the middle one in the example) still gets properly repainted.

(In reply to comment #3)
> I want to make it possible to test repaints without pixel testing.

That would be sweet.

[Darin Adler]

Comment on attachment 169051 [details]
Proposed removal, includes testing as I didn't find any coverage for such repainting.

View in context: https://bugs.webkit.org/attachment.cgi?id=169051&action=review

> Source/WebCore/ChangeLog:16
> +        All in all, the existing repainting logic already handles this case properly.

The only thing I don’t like about this is that it requires global knowledge to know that this isn’t needed. It’s something you can’t tell by just reading the function itself. It’s nice to have the test coverage to mitigate this, but it would be better if there was some good convention so what each function handles and does not need to handle could be clearer.

A concrete way to put that is that I think a comment might be needed to explain why this function does *not* have to deal with repainting. Depending on what our conventions are for this sort of thing in our renderer code.

[Ahmad Saleem]

This is still present:

https://github.com/WebKit/WebKit/blob/da7f1abe0ba98769541491ccf549e553b9cd1669/Source/WebCore/rendering/RenderTableCell.cpp#L273

Is this still needed? If yes, I think this r+ patch can be applied even today or if not, I am happy to do updated PR.

Appreciate if someone can confirm. Thanks!

[Brent Fulgham]

(In reply to Ahmad Saleem from comment #6)
> This is still present:
> 
> https://github.com/WebKit/WebKit/blob/
> da7f1abe0ba98769541491ccf549e553b9cd1669/Source/WebCore/rendering/
> RenderTableCell.cpp#L273
> 
> Is this still needed? If yes, I think this r+ patch can be applied even
> today or if not, I am happy to do updated PR.
> 
> Appreciate if someone can confirm. Thanks!

Seems like a good change. Why don’t you put up a PR, and we can let modern EWS chew on it.

[Radar WebKit Bug Importer]

<rdar://problem/99781049>

[Ahmad Saleem]

Just did - https://github.com/WebKit/WebKit/pull/4226

[EWS]

Committed 254440@main (9f03d20e70c3): <https://commits.webkit.org/254440@main>

Reviewed commits have been landed. Closing PR #4226 and removing active labels.