Bug 150399 - Date creation should share a little code
Summary: Date creation should share a little code
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Geoffrey Garen
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-10-21 11:21 PDT by Geoffrey Garen
Modified: 2015-10-21 11:42 PDT (History)
0 users

See Also:

Attachments
Patch (7.97 KB, patch)
2015-10-21 11:25 PDT, Geoffrey Garen 		fpizlo: review+
Details | Formatted Diff | Diff
View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Geoffrey Garen 2015-10-21 11:21:25 PDT 
Date creation should share a little code
Comment 1 Geoffrey Garen 2015-10-21 11:25:40 PDT 
Created attachment 263705 [details]
Patch
Comment 2 Saam Barati 2015-10-21 11:33:00 PDT 
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?
Comment 3 Filip Pizlo 2015-10-21 11:36:24 PDT 
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.
Comment 4 Geoffrey Garen 2015-10-21 11:41:13 PDT 
> > 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
Comment 5 Geoffrey Garen 2015-10-21 11:42:45 PDT 
Committed r191393: <http://trac.webkit.org/changeset/191393>