[webkit-reviews] review granted: [Bug 23522] Use checked casts for render tree : [Attachment 33821] Next step: Includes toRenderTable*

[bugzilla-daemon at webkit.org]

David Levin <levin at chromium.org> has granted Darin Adler <darin at apple.com>'s
request for review:
Bug 23522: Use checked casts for render tree
https://bugs.webkit.org/show_bug.cgi?id=23522

Attachment 33821: Next step: Includes toRenderTable*
https://bugs.webkit.org/attachment.cgi?id=33821&action=review

------- Additional Comments from David Levin <levin at chromium.org>

> Index: WebCore/page/Frame.cpp
> ===================================================================
>      if (cellRenderer && cellRenderer->isTableCell()) {
> -	   RenderTableCell* cellAboveRenderer =
cellRenderer->table()->cellAbove(cellRenderer);
> +	   RenderTableCell* cellAboveRenderer =
toRenderTableCell(cellRenderer)->table()->cellAbove(toRenderTableCell(cellRende
rer));

Although it is no runtime expense, it is harder to read with two
"toRenderTableCell(cellRenderer)" in the same line.

Consider a local variable to avoid this.  For example,
	RenderTableCell* tableCell = toRenderTableCell(cellRenderer);
	RenderTableCell* cellAboveRenderer =
tableCell->table()->cellAbove(tableCell);


> Index: WebCore/page/mac/FrameMac.mm
> ===================================================================
>      if (cellRenderer && cellRenderer->isTableCell()) {
> -	   RenderTableCell* cellAboveRenderer =
cellRenderer->table()->cellAbove(cellRenderer);
> +	   RenderTableCell* cellAboveRenderer =
toRenderTableCell(cellRenderer)->table()->cellAbove(toRenderTableCell(cellRende
rer));

Same comment as before "It is harder to read with two
"toRenderTableCell(cellRenderer)" in the same line...."


> Index: WebCore/rendering/RenderTable.cpp
> ===================================================================

47	RenderTable::RenderTable(Node* node)
...
53	    , m_tableLayout(0)

This initialization may be deleted now.


> Index: WebCore/rendering/RenderTable.h
> ===================================================================
> +    virtual void paintMask(PaintInfo& paintInfo, int tx, int ty);

"paintInfo" param name is redundant.  (It existed before due to the move it
looks like a new line so it is a good time to remove this.)

> +inline RenderTable* toRenderTable(RenderObject* o)
> +inline const RenderTable* toRenderTable(const RenderObject* o)

Consider using renderObject instead of o (so that we have full words for the
variable names).


> Index: WebCore/rendering/RenderTableCell.h
> ===================================================================
> +inline RenderTableCell* toRenderTableCell(RenderObject* o)
> +inline const RenderTableCell* toRenderTableCell(const RenderObject* o)

Consider using renderObject instead of o.


> Index: WebCore/rendering/RenderTableCol.h
> ===================================================================
> +    void setSpan(int s) { m_span = s; }

Consider using span instead of s.


> +inline RenderTableCol* toRenderTableCol(RenderObject* o)
> +inline const RenderTableCol* toRenderTableCol(const RenderObject* o)

Consider using renderObject instead of o.



> Index: WebCore/rendering/RenderTableRow.h
> ===================================================================

> +inline RenderTableRow* toRenderTableRow(RenderObject* o)
> +inline const RenderTableRow* toRenderTableRow(const RenderObject* o)

Consider using renderObject instead of o.



> Index: WebCore/rendering/RenderTableSection.h
> ===================================================================
> +inline RenderTableSection* toRenderTableSection(RenderObject* o)
> +inline const RenderTableSection* toRenderTableSection(const RenderObject* o)


Consider using renderObject instead of o.