Bug 34409 - Use WTF::getLocalTime instead of localtime_r in FTPDirectoryDocument
: Use WTF::getLocalTime instead of localtime_r in FTPDirectoryDocument
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: Page Loading
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To: Nobody
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2010-01-31 22:55 PST by Kwang Yul Seo
Modified: 2010-02-02 14:50 PST (History)
4 users (show)

See Also:


Attachments
Use WTF::getLocalTime (2.03 KB, patch)
2010-01-31 23:01 PST, Kwang Yul Seo
no flags Details | Formatted Diff | Diff
Use WTF::getLocalTime (4.19 KB, patch)
2010-02-02 10:29 PST, Kwang Yul Seo
darin: review+
Details | Formatted Diff | Diff
Use WTF::getLocalTime (4.22 KB, patch)
2010-02-02 10:55 PST, Kwang Yul Seo
darin: review+
Details | Formatted Diff | Diff
Use WTF::getLocalTime (4.17 KB, patch)
2010-02-02 11:24 PST, Kwang Yul Seo
no flags Details | Formatted Diff | Diff
Use WTF::getLocalTime (4.17 KB, patch)
2010-02-02 11:52 PST, Kwang Yul Seo
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kwang Yul Seo 2010-01-31 22:55:17 PST
Use WTF::getLocalTime instead of localtime_r and remove platform guards.
Comment 1 Kwang Yul Seo 2010-01-31 23:01:01 PST
Created attachment 47814 [details]
Use WTF::getLocalTime
Comment 2 Darin Adler 2010-02-01 08:12:02 PST
Comment on attachment 47814 [details]
Use WTF::getLocalTime

> -#define localtime_r(x, y) localTimeQt(x, y)
> -#elif OS(WINDOWS) && !defined(localtime_r)
> -#if defined(_MSC_VER) && (_MSC_VER >= 1400) 
> -#define localtime_r(x, y) localtime_s((y), (x))
> -#else /* !_MSC_VER */ 
> -#define localtime_r(x,y) (localtime(x)?(*(y)=*localtime(x),(y)):0)
> -#endif
> +#define getLocalTime(x, y) localTimeQt(x, y)
>  #endif

What is this needed for? Doesn't the WTF getLocalTime work on Qt? If not, then why not put the ifdefs in WTF's getCurrentTime function?
Comment 3 Kwang Yul Seo 2010-02-02 10:04:39 PST
From the code comment, it seems localTimeQt was needed because MinGW lacks localtime_r.  As WTF::getLocalTime already covers COMPILER(MINGW), I guess it is safe to remove the entire platform guards and can replace localtime_r with WTF::getLocalTime.

However, the helper function gmtimeQt(const QDateTime &input) is shared by FTPDirectoryParser.cpp to implement gmTimeQt(const time_t *const timep, struct tm *result), so I couldn't remove it.
Comment 4 Kwang Yul Seo 2010-02-02 10:29:32 PST
Created attachment 47949 [details]
Use WTF::getLocalTime

Move gmtimeQt(const QDateTime &input) to FTPDirectoryParser.cpp because it is shared. Remove platform guards for localtime_r and use WTF::getLocalTime unconditionally.
Comment 5 Darin Adler 2010-02-02 10:40:17 PST
Comment on attachment 47949 [details]
Use WTF::getLocalTime

Looks good.

> -    localtime_r(&now_t, &now);
> +    WTF::getLocalTime(&now_t, &now);

The correct way to do this with WTF is to add:

    using WTF::getLocalTime;

to the bottom of the CurrentTime.h header. And not include the "WTF::" prefix here.

I won’t hold up this patch just for that reason, but I hope you are willing to tackle this rather than leaving it for someone else.

>  #if PLATFORM(QT) && defined(Q_WS_WIN32)
> -// Defined in FTPDirectoryDocument.cpp.
> -struct tm gmtimeQt(const QDateTime &input);
> +
> +// Replacement for localtime_r() which is not available on MinGW.
> +// We use this on all of Qt's platforms for portability.

The comment is now incorrect with the function in its new location. This is only used for Win32 in this file.

It's incorrect to call this a "replacement for localtime_r" here; it's a replacement for gmtime_r.

This is not the normal portability approach for WebKit. We typically put these platform dependencies in WTF or the platform directory, not directly in the affected file. The WebKit approach to this would be to create a replacement for gmtime_r in a WTF header named something like TimeExtras.h or DateExtras.h rather than having it here in this source file.

> +struct tm gmtimeQt(const QDateTime &input)

Style comment on code that was moved: The "&" character should be next to the type, not the argument name. Not sure why the style bot let you get away with it.

> +    const QDate date(input.date());

> +    const QTime time(input.time());

We normally don't use const like this on local variables in WebKit code. There are lots of local variables that could be marked const, and we decided to mark none of them rather than trying to mark all of them.

r=me despite these comments
Comment 6 Brady Eidson 2010-02-02 10:41:33 PST
Comment on attachment 47949 [details]
Use WTF::getLocalTime

I don't know the full structure of this code off-hand nor do I understand the specific reasons why QT is so special here.  It appears on the surface that the FTPDocument or FTPParser code is utterly the wrong place for any time-related methods.  It seems like such fundamental platform code that the other platforms use WTF:: functions for should be made to live in WTF.

That said, the damage was already done and I'm not going to make you change it here.  Please consider spending a little time to consolidate it to a different place.
Comment 7 Brady Eidson 2010-02-02 10:42:36 PST
Darin beat me too it.  Suppose I should've reviewed the midair collision.  ;)
Comment 8 Kwang Yul Seo 2010-02-02 10:55:17 PST
Created attachment 47951 [details]
Use WTF::getLocalTime 

Fix styles and comments as Darin suggested. I will file another bug to move gmtime_r to WTF.
Comment 9 Darin Adler 2010-02-02 11:01:44 PST
Comment on attachment 47951 [details]
Use WTF::getLocalTime 

> +using WTF::getLocalTime;

Thanks for changing this, but it's still not quite right. This needs to go at the bottom of CurrentTime.h with the other "using" that is already there. But as I said last time, I will not insist it goes there in this patch. It might be more pleasant to have this patch not touch JavaScriptCore.

>  #if PLATFORM(QT) && defined(Q_WS_WIN32)
> -// Defined in FTPDirectoryDocument.cpp.
> -struct tm gmtimeQt(const QDateTime &input);
> +
> +// Replacement for gmtime_r() which is not available on MinGW.
> +// We use this on all of Qt's platforms for portability.

The code above says "use this only on Win 32", but the comment says "on all of Qt's platforms". The comment should match the #if.

r=me because I think it's OK to land even with these mistakes, but I cleared commit-queue because I think we'll probably go another round
Comment 10 Kwang Yul Seo 2010-02-02 11:24:05 PST
Created attachment 47956 [details]
Use WTF::getLocalTime 

Fix comments as Darin suggested. Remove using WTF::getLocalTime and WTF namespace as a patch for https://bugs.webkit.org/show_bug.cgi?id=34493 is submitted separately.
Comment 11 WebKit Review Bot 2010-02-02 11:29:30 PST
Attachment 47956 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/230717
Comment 12 Kwang Yul Seo 2010-02-02 11:31:39 PST
Qt build failed because 34493 is not landed yet.
Comment 13 Kwang Yul Seo 2010-02-02 11:52:03 PST
Created attachment 47958 [details]
Use WTF::getLocalTime 

Submit the same patch again to re-run build bots.
Comment 14 Eric Seidel 2010-02-02 14:07:35 PST
Comment on attachment 47958 [details]
Use WTF::getLocalTime 

OK.
Comment 15 WebKit Commit Bot 2010-02-02 14:50:47 PST
Comment on attachment 47958 [details]
Use WTF::getLocalTime 

Clearing flags on attachment: 47958

Committed r54256: <http://trac.webkit.org/changeset/54256>
Comment 16 WebKit Commit Bot 2010-02-02 14:50:54 PST
All reviewed patches have been landed.  Closing bug.