[webkit-reviews] review denied: [Bug 32077] textarea grows when you type : [Attachment 50203] patch v1; preserve original height when the renderer is layout-root

[bugzilla-daemon at webkit.org]

mitz at webkit.org has denied MORITA Hajime <morrita at google.com>'s request for
review:
Bug 32077: textarea grows when you type
https://bugs.webkit.org/show_bug.cgi?id=32077

Attachment 50203: patch v1; preserve original height when the renderer is
layout-root
https://bugs.webkit.org/attachment.cgi?id=50203&action=review

------- Additional Comments from mitz at webkit.org
> +++ b/LayoutTests/ChangeLog
> @@ -1,3 +1,32 @@
> +2010-03-08  MORITA Hajime  <morrita at google.com>
> +
> +	   Reviewed by NOBODY (OOPS!).
> +
> +	   Textarea grows when you type.
> +	   https://bugs.webkit.org/show_bug.cgi?id=32077
> +
> +	   Fix to prevent box height change on re-layout with the renderer of
> +	   the box as a root. RenderBlock with pecent-specified 'height'
> +	   propety (and family) did cause different box height values between
> +	   full-layout and partial layout with the renderer as a layout-root.
> +	   This fix restores original height in such case.

The LayoutTests change log need not contain a description of the fix.

> +<script src="./resources/textarea-percentage-dimensions.js"></script>

You can omit the “./”.

> +++ b/WebCore/ChangeLog
> @@ -1,3 +1,29 @@
> +2010-03-08  MORITA Hajime  <morrita at google.com>
> +
> +	   Reviewed by NOBODY (OOPS!).
> +
> +	   Textarea grows when you type.
> +	   https://bugs.webkit.org/show_bug.cgi?id=32077
> +
> +	   Fix to prevent box height change on re-layout with the renderer of
> +	   the box as a root. RenderBlock with pecent-specified 'height'

Typo: “pecent”.

> +	   propety (and family) did cause different box height values between

Typo: “propety”. I don’t understand “and family”.

> +	   full-layout and partial layout with the renderer as a layout-root.
> +	   This fix restores original height in such case.

I think the description could be improved. The main points, as I see them, are
the specific case where percentage height calculation depends on the state of
ancestors during their layout, and the fact that by definition, the height of
the layout root must not change during layout, or else it wouldn’t be a valid
layout root.

> +    bool isLayoutRoot = node() && view()->frameView() &&
view()->frameView()->layoutRoot(true) == this;

Using a local variable for this seems redundant. Why the check for node()?

>      int oldHeight = height();
> -    calcHeight();
> +
> +    if (isLayoutRoot)
> +	   setHeight(previousHeight);
> +    else
> +	   calcHeight();
> +
>      if (oldHeight != height()) {

This is suboptimal. If you skip the height calculation, it seems like you could
skip the (oldHeight != height()) test and the previousHeight != height() test
that follows. Those could be in the else block with calcHeight().