Bug 34409 - Use WTF::getLocalTime instead of localtime_r in FTPDirectoryDocument
: Use WTF::getLocalTime instead of localtime_r in FTPDirectoryDocument
Status: RESOLVED FIXED
: WebKit
Page Loading
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2010-01-31 22:55 PST by
Modified: 2010-02-02 14:50 PST (History)


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


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2010-01-31 22:55:17 PST
Use WTF::getLocalTime instead of localtime_r and remove platform guards.
------- Comment #1 From 2010-01-31 23:01:01 PST -------
Created an attachment (id=47814) [details]
Use WTF::getLocalTime
------- Comment #2 From 2010-02-01 08:12:02 PST -------
(From update of attachment 47814 [details])
> -#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 From 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 From 2010-02-02 10:29:32 PST -------
Created an attachment (id=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 From 2010-02-02 10:40:17 PST -------
(From update of attachment 47949 [details])
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 From 2010-02-02 10:41:33 PST -------
(From update of attachment 47949 [details])
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 From 2010-02-02 10:42:36 PST -------
Darin beat me too it.  Suppose I should've reviewed the midair collision.  ;)
------- Comment #8 From 2010-02-02 10:55:17 PST -------
Created an attachment (id=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 From 2010-02-02 11:01:44 PST -------
(From update of attachment 47951 [details])
> +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 From 2010-02-02 11:24:05 PST -------
Created an attachment (id=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 From 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 From 2010-02-02 11:31:39 PST -------
Qt build failed because 34493 is not landed yet.
------- Comment #13 From 2010-02-02 11:52:03 PST -------
Created an attachment (id=47958) [details]
Use WTF::getLocalTime 

Submit the same patch again to re-run build bots.
------- Comment #14 From 2010-02-02 14:07:35 PST -------
(From update of attachment 47958 [details])
OK.
------- Comment #15 From 2010-02-02 14:50:47 PST -------
(From update of attachment 47958 [details])
Clearing flags on attachment: 47958

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