[webkit-reviews] review granted: [Bug 170720] [JSC] Date.parse should accept wider range of representation : [Attachment 306985] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Yusuke Suzuki
<utatane.tea at gmail.com>'s request for review:
Bug 170720: [JSC] Date.parse should accept wider range of representation
https://bugs.webkit.org/show_bug.cgi?id=170720

Attachment 306985: Patch

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




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

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

> Source/WTF/wtf/DateMath.cpp:897
> +    std::optional<int> year = std::nullopt;

No need for "= std::nullopt". That’s what optionals do when not specified.

> Source/WTF/wtf/DateMath.cpp:1132
> +	   // 1. According to the V8, this default value is compatible to the
old KJS. While current V8 uses 2001,
> +	   //	 SpiderMonkey does not have such a fallback path. So this
reason is not big deal.

I am a bit confused by this comment. V8 is the name of a JavaScript engine, not
a person, so I don’t know what "according to the V8" means; maybe someone on
the V8 team did some research about the behavior of older versions of WebKit? I
don’t know what "compatible to the old KJS" means or why it’s relevant to our
decision of what behavior is best. “The old KJS” seems quite unimportant; very
few websites were tailored to assume the behavior of the KJS engine since it
wasn’t used by all that many people. So maybe it’s simply a peculiar way to
talk about older versions of WebKit and JavaScriptCore. The term “current V8”
is not good since the comment is likely to last a long time and this may no
longer be true in the future, so we should probably say this in a different
way. Sometimes I use the phrase "as of this writing". Here’s a comment I might
write, but I don’t know if it’s accurate:

    // 1. Year 2000 was used in older versions of JavaScriptCore, thus some
older content
    //	  tested only with WebKit might rely on this behavior (according to
research done by the V8 team).
    //	  (As of April 2017, V8 is using the year 2001 and Spider Monkey is not
doing this kind of fallback.)

But when written this way it becomes clear that it’s a little strange to rely
on research done by the V8 team to know what the behavior of old versions of
WebKit is. Can’t we just check this rather than relying on what they say?

Also, it would be nice to be precise rather than just "older versions of
JavaScriptCore".