[webkit-reviews] review denied: [Bug 29084] getComputedStyle returns percentage values for left / right / top / bottom : [Attachment 188473] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Tim 'mithro' Ansell
<mithro at mithis.com>'s request for review:
Bug 29084: getComputedStyle returns percentage values for left / right / top /
bottom
https://bugs.webkit.org/show_bug.cgi?id=29084

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

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


Thanks for tackling this!

The change log for this patch is unacceptably empty. You need to explain what
you are doing and why, not just cite the name of the bug.

Also, patches that fix bugs need to include test cases that show the bug is
fixed and that would fail without the bug fix.

> Source/WebCore/css/CSSCalculationValue.cpp:188
> +//		 ASSERT_NOT_REACHED();

I assume this was included with the patch by accident, since the change log
doesn’t mention it. If not, we should discuss why you made this change. And
it’s definitely incorrect to change it by commenting out the assertion.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:582
> -static PassRefPtr<CSSValue> zoomAdjustedPixelValueForLength(const Length&
length, const RenderStyle* style)
> +static PassRefPtr<CSSValue> zoomAdjustedPixelValueForLengthOld(const Length&
length, const RenderStyle* style)

You can’t just add the word old to the end of the function without explaining
it, at least in change log. Please explain your rationale here.