[webkit-reviews] review denied: [Bug 16753] date set methods with no args should result in NaN (Acid3 bug) : [Attachment 19206] Patch to handle NaN, Inf and missing arguments

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has denied Michael Knaup
<michael.knaup at mac.com>'s request for review:
Bug 16753: date set methods with no args should result in NaN (Acid3 bug)
http://bugs.webkit.org/show_bug.cgi?id=16753

Attachment 19206: Patch to handle NaN, Inf and missing arguments
http://bugs.webkit.org/attachment.cgi?id=19206&action=edit

------- Additional Comments from Eric Seidel <eric at webkit.org>
Thanks for the patch!

A few comments.  One, you should familiarize yourself with the WebKit coding
style guidelines:
http://webkit.org/coding/coding-style.html

A few "violations" I see in this patch include:
if (foo)
{

(The { should be on the same line as the if)

and:

if (0 != args.size())

should be:

if (args.size()) (we avoid == 0 and != 0 when the meaning is otherwise clear
from context.  e.g. comparing pointers to NULL or using size() for isEmpty()...
in this case, there migth even be an isEmpty() which could be more clear).

Single line ifs should not have { }

Wouldn't:
ok = !(isinf(millis) || isnan(millis));
be equivalent to:
ok = isfinite(mills); ?

Also, the above if (ok) block, generally we prefer an early-return style in
webkit.  In this instance you would use:
if (!ok)
   return false;

instead of wrapping that section in an if (ok) block.

Likewise, it would be more clear (or at least more inline with webkit style) to
do:

if (!args.size()) {
   result = jsNaN();
   date->setInternalValue(result);
   return result;
} 

as an early return, instead of adding a long if block.

We'll need to of course run SunSpider with this change to make sure there is
not speed regression due to added branching in these functions.

r- for the style issues.

Ideallly your patch would include the diff for the test case, since someone
else will be landing this for you (unless you have commit-bit and I simply
don't recognize your name).

Thanks again for the patch!