[webkit-reviews] review granted: [Bug 22996] RenderTextControl unecessarily combines multi/single line text control rendering : [Attachment 26272] Updated patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Nikolas Zimmermann
<zimmermann at kde.org>'s request for review:
Bug 22996: RenderTextControl unecessarily combines multi/single line text
control rendering
https://bugs.webkit.org/show_bug.cgi?id=22996

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

------- Additional Comments from Darin Adler <darin at apple.com>
> -using namespace std;
> +using std::max;
> +using std::min;

Why that change? I like the old way better.

> +    return m_width - paddingLeft() - paddingRight() - borderLeft() -
borderRight() -
> +	      m_innerText->renderer()->paddingLeft() -
m_innerText->renderer()->paddingRight();

We normally put the operator in the continuation line, and just indent one
normal level rather than lining up, so it would be:

    return m_width - paddingLeft() - paddingRight() - borderLeft() -
borderRight()
	- m_innerText->renderer()->paddingLeft() -
m_innerText->renderer()->paddingRight();

The operator at the beginning of the line helps make it clear it's a
continuation.

> +	   ASSERT(ec == 0);

We should probably write these as ASSERT(!ec) in the future.

The history would probably work a little better if
RenderTextControlSingleLine.h/cpp and RenderTextControlMultipleLine.h/cpp
started out with "svn cp" of the RenderTextControl.h/cpp files. In fact I could
then have reviewed the patch a little more carefully if that diff was in there.


r=me