[webkit-reviews] review granted: [Bug 21280] AXTables on page are probably not tables : [Attachment 23986] Patch to make AXTable detection more robust

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted chris fleizach
<cfleizach at apple.com>'s request for review:
Bug 21280: AXTables on page are probably not tables
https://bugs.webkit.org/show_bug.cgi?id=21280

Attachment 23986: Patch to make AXTable detection more robust
https://bugs.webkit.org/attachment.cgi?id=23986&action=edit

------- Additional Comments from Darin Adler <darin at apple.com>
I think I would use the word "heuristic" somewhere here to describe what we're
doing.

To me it seems overkill to look at all the cells in a table. Once you've found
borders or colors on 10 different cells, I'm not sure what the value is of
examining 100 others to determine that it's "more than half of the cells" that
have borders or colors. I think "two rows" was probably too limited, but "every
single cell" is probably too open ended. In particular we want good performance
on absurdly huge tables, and querying every cell of such tables is obviously
not helpful.

+	     // considered a bordered cell. Try the cell borders and the
style's borders

I don't see code corresponding to the style's borders.

The check for background differences isn't quite right -- it doesn't account
for transparent colors.

+    unsigned neededCellCount = (validCellCount >> 1);

It's bad style to do ">> 1" when you really mean "/ 2" and doesn't make the
code any faster. We also don't usually put parentheses around this operation.

I'm going to say r=me, but I think it's worth pondering further whether it's
sensible to iterate the entire table even for pathologically huge tables.