[webkit-reviews] review granted: [Bug 89071] JavaScript: Invalid date parse for ISO 8601 strings when no timezone given : [Attachment 386760] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Ross Kirsling
<ross.kirsling at sony.com>'s request for review:
Bug 89071: JavaScript: Invalid date parse for ISO 8601 strings when no timezone
given
https://bugs.webkit.org/show_bug.cgi?id=89071

Attachment 386760: Patch

https://bugs.webkit.org/attachment.cgi?id=386760&action=review




--- Comment #9 from Darin Adler <darin at apple.com> ---
Comment on attachment 386760
  --> https://bugs.webkit.org/attachment.cgi?id=386760
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=386760&action=review

Not a huge fan of "out arguments" but we should respect the style of this
old-ish code, I suppose.

> Source/JavaScriptCore/runtime/JSDateMath.cpp:218
> +    int offset = isLocalTime ? localTimeOffset(vm, ms,
WTF::LocalTime).offset : 0;
> +    return ms - offset;

I don't think it’s great to use the trinary operator here. How about this?

    if (isLocalTime)
	ms -= localTimeOffset(vm, ms, WTF::LocalTime).offset;

    return ms;

Also, maybe instead of "ms" we could name the local variable "value", or
"date", or "time". I think a word is better than an SI unit for a variable
name.

> Source/WTF/wtf/DateMath.cpp:575
> +    isLocalTime = false;

Why not do this at the very top of the function instead of down here?

> Source/WTF/wtf/DateMath.cpp:653
> +	   isLocalTime = false;

Why not do this at the start of the function rather than here in this else?