[webkit-reviews] review denied: [Bug 32541] Percentage height should be ignored in some cases : [Attachment 44927] Proposed patch (rev.3)

[bugzilla-daemon at webkit.org]

Beth Dakin <bdakin at apple.com> has denied TAMURA, Kent <tkent at chromium.org>'s
request for review:
Bug 32541: Percentage height should be ignored in some cases
https://bugs.webkit.org/show_bug.cgi?id=32541

Attachment 44927: Proposed patch (rev.3)
https://bugs.webkit.org/attachment.cgi?id=44927&action=review

------- Additional Comments from Beth Dakin <bdakin at apple.com>
> +	       if (cb->isTableCell() && cb->style()->height().isAuto()
> +		   // Check the table for the cell too.. This is needed to pass

> +		   // LayoutTests/tables/mozilla/bug/bug32205-2.html.
> +		   && cb->containingBlock()->style()->height().isAuto()) {

We don't usually put comments in the middle of the condition of an if-statement
like you have done here, and I don't think it's a style we want to encourage.
This comment should be moved out. I also think there needs to be a better
justification for checking the containing block's containing block than that it
is needed to make a test pass. Are you sure this isn't just a hack for that
test?

> +		   // Reset to Auto.
> +		   style()->setHeight(Length());

Generally, we avoid changing the RenderStyle's values like this. Instead, when
this case is detected while calculating the height of the renderer, the height
of the renderer should be adjusted accordingly, but you should not have to
change the RenderStyle.

> +		       RenderTheme::defaultTheme()->adjustSize(style());

You should also avoid calling into the RenderTheme. Again, you want to change
the render's height value, not the RenderStyle's. If you work on fixing the
renderer's calculated height value, calling into the theme should not be
necessary.

> +void RenderTheme::adjustSize(RenderStyle* style)
>
I don't understand how this new function is related to this bug or why it is
necessary. But it certainly won't be necessary when you focus on calculating
the correct height for the renderer rather than changing the RenderStyle.

Also, I we try to avoid such class-specific code (in this case, it's
table-specific code) in RenderBox. If you find that calcHeight() is still the
right place to patch when you work on the calculation of the renderer's height,
then you should consider creating a version of calcHeight in one of the
RenderTable* files to do the table-specific parts of the calculation.