1370833 - make less *InvalidateTableFrame* calls so we won't over painting the table.

Status: RESOLVED FIXED

(Core :: Layout, enhancement)

Description

[Ya-Chieh Wu (inactive)]

As Bug 1362412 Comment 16 mentioned, we should remove "manual invalidation" in table and count on comparison of the display items when we have a not border-collapse table.

Comment 1

[Ya-Chieh Wu (inactive)]

Attachment: table-collapse.html

this attachment could help to debug.

Comment 2

[Ya-Chieh Wu (inactive)]

Attachment: table-collapse.html

From this test case, you can see that we over paint the whole table whether we have the border-collapse style or not.

Comment 3

[Ya-Chieh Wu (inactive)]

I am going to remove the calls that call to *nsTableFrame::InvalidateTableFrame*

Comment 4

[Ya-Chieh Wu (inactive)]

Attachment: Bug 1370833 Part 1 - make less table invalidations for non-border-collapse tables.

Review commit: https://reviewboard.mozilla.org/r/147376/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/147376/

Comment 5

[Ya-Chieh Wu (inactive)]

Attachment: table-test.html

Comment 6

[Ya-Chieh Wu (inactive)]

Comment on attachment 8875968 [details]
Bug 1370833 Part 1 - make less table invalidations for non-border-collapse tables.

Hi Matt,

In test-case.html, turning on paint_flashing, you can see we paint the whole table every second even though some part of the table's contents haven't changed at all. I tried to remove those suspicious calls and confirmed the area which is being repainted has shrunk to the reasonable area. 
 
try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6372abefde2797dee46423860e697d0fff2cf236&selectedJob=105706808
How do you think?

However, I can still see some suspicious calls which call to "nsTableFrame::InvalidateTableFrame" in nsTableRowFrame.cpp, nsTableRowGroupFrame.cpp... etc but I haven't been able to make the tests which can hit those part. I think we could land what test-case.html hits first and see if there is any complains afterward. If not, we remove all of them in our codebase.



thanks!

Comment 7

[Astley Chen (inactive)]

Comment on attachment 8875968 [details]
Bug 1370833 Part 1 - make less table invalidations for non-border-collapse tables.

https://reviewboard.mozilla.org/r/147376/#review151638

::: commit-message-7efda:2
(Diff revision 1)
> +Bug 1370833 - Remove over invalidation tableFrame.
> +

Could you put more details in commit message so that people could have better understanding regarding to this changeset?

Comment 8

[Ya-Chieh Wu (inactive)]

Comment on attachment 8875968 [details]
Bug 1370833 Part 1 - make less table invalidations for non-border-collapse tables.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/147376/diff/1-2/

Comment 9

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8875968 [details]
Bug 1370833 Part 1 - make less table invalidations for non-border-collapse tables.

https://reviewboard.mozilla.org/r/147376/#review152056

Looks good!

Can you also add a reftest (or a few?) that make sure we're invalidating correctly.

You want to add 'reftest-wait' as a class on the root element, wait for the MozReftestInvalidate event, and then make the change and remove the reftest-wait class. There's lots of existing tests that use this format.
Ideally we'd have one test with border-collapse and one without.

Comment 10

[Ya-Chieh Wu (inactive)]

Attachment: Bug 1370833 Part 2 - add reftests for border-collapse and non-border-collapse table.

Review commit: https://reviewboard.mozilla.org/r/147922/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/147922/

Comment 11

[Ya-Chieh Wu (inactive)]

Comment on attachment 8875968 [details]
Bug 1370833 Part 1 - make less table invalidations for non-border-collapse tables.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/147376/diff/2-3/

Comment 12

[Ya-Chieh Wu (inactive)]

Comment on attachment 8876593 [details]
Bug 1370833 Part 2 - add reftests for border-collapse and non-border-collapse table.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/147922/diff/1-2/

Comment 13

[Ya-Chieh Wu (inactive)]

Comment on attachment 8876593 [details]
Bug 1370833 Part 2 - add reftests for border-collapse and non-border-collapse table.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/147922/diff/2-3/

Comment 14

[Ya-Chieh Wu (inactive)]

Hey Matt,

result of try looks fine:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e7ccf57a6bfaa1b0832a693143f8d1a84fd8f0f5&selectedJob=106320186

How do you think about the reftest?
thanks a lot.

Comment 15

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8876593 [details]
Bug 1370833 Part 2 - add reftests for border-collapse and non-border-collapse table.

https://reviewboard.mozilla.org/r/147922/#review153214

Comment 16

[Ya-Chieh Wu (inactive)]

Comment on attachment 8875968 [details]
Bug 1370833 Part 1 - make less table invalidations for non-border-collapse tables.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/147376/diff/3-4/

Comment 17

[Ya-Chieh Wu (inactive)]

Comment on attachment 8876593 [details]
Bug 1370833 Part 2 - add reftests for border-collapse and non-border-collapse table.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/147922/diff/3-4/

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0cd9f4c25544
Part 1 - make less table invalidations for non-border-collapse tables. r=mattwoodrow
https://hg.mozilla.org/integration/autoland/rev/2c4ea118644c
Part 2 - add reftests for border-collapse and non-border-collapse table. r=mattwoodrow

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0cd9f4c25544
https://hg.mozilla.org/mozilla-central/rev/2c4ea118644c