Bug 150399 – Date creation should share a little code

• Component: New Bugs
• Status: RESOLVED FIXED

[Geoffrey Garen]

Date creation should share a little code

[Geoffrey Garen]

Created attachment 263705 [details]
Patch

[Saam Barati]

Comment on attachment 263705 [details]
Patch

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

r=me

> Source/JavaScriptCore/runtime/DateConstructor.cpp:138
> +    GregorianDateTime t;

nit: Maybe we can name this something better than "t" since you're refactoring?

[Filip Pizlo]

Comment on attachment 263705 [details]
Patch

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

r=me

> Source/JavaScriptCore/runtime/DateConstructor.cpp:115
> +static double milliseconds(ExecState* exec, const ArgList& args, WTF::TimeType timeType)

Non-zero benefit to renaming this method to avoid clashes with std::chrono.

In particular, when I first read your patch, I happened to read it in reverse and so I thought that the call to milliseconds() was a chrono thing.

>> Source/JavaScriptCore/runtime/DateConstructor.cpp:138
>> +    GregorianDateTime t;
> 
> nit: Maybe we can name this something better than "t" since you're refactoring?

This is a fun one.  I would have chosen "t" since it's the universally accepted variable for time.  I don't care either way, though.

[Geoffrey Garen]

> > Source/JavaScriptCore/runtime/DateConstructor.cpp:115
> > +static double milliseconds(ExecState* exec, const ArgList& args, WTF::TimeType timeType)
> 
> Non-zero benefit to renaming this method to avoid clashes with std::chromo.

=> millisecondsFromComponents

[Geoffrey Garen]

Committed r191393: <http://trac.webkit.org/changeset/191393>