[webkit-reviews] review granted: [Bug 131796] AX: Malformed tables exposing incorrect col and colSpans : [Attachment 229555] patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted chris fleizach
<cfleizach at apple.com>'s request for review:
Bug 131796: AX: Malformed tables exposing incorrect col and colSpans
https://bugs.webkit.org/show_bug.cgi?id=131796

Attachment 229555: patch
https://bugs.webkit.org/attachment.cgi?id=229555&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=229555&action=review


> Source/WebCore/accessibility/AccessibilityTableCell.cpp:251
>      RenderTableCell* renderCell = toRenderTableCell(m_renderer);

I’d suggest using a reference rather than a pointer for this local variable.
And I would name it cell. I don’t think a “render” prefix adds anything.

> Source/WebCore/accessibility/AccessibilityTableCell.cpp:256
> +    RenderTable* table = renderCell->table();
> +    if (!table)
> +	   return;

Can this ever be null? In practice I don’t think it can, so the code above
doesn’t matter. I was going to criticize sometimes returning col/colSpan
directly from the element, but I’m not sure it matters.

> Source/WebCore/accessibility/AccessibilityTableCell.cpp:258
> +    columnRange.first = table->colToEffCol(columnRange.first);

I think it’s strange to use columnRange.first here instead of
renderCell->col(). Makes the code harder to read for me.