Bug 34409 – Use WTF::getLocalTime instead of localtime_r in FTPDirectoryDocument

• Component: Page Loading
• Status: RESOLVED FIXED

[Kwang Yul Seo]

Use WTF::getLocalTime instead of localtime_r and remove platform guards.

[Kwang Yul Seo]

Created attachment 47814 [details]
Use WTF::getLocalTime

[Darin Adler]

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?

[Kwang Yul Seo]

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.

[Kwang Yul Seo]

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.

[Darin Adler]

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

[Brady Eidson]

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.

[Brady Eidson]

Darin beat me too it.  Suppose I should've reviewed the midair collision.  ;)

[Kwang Yul Seo]

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.

[Darin Adler]

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

[Kwang Yul Seo]

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.

[WebKit Review Bot]

Attachment 47956 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/230717

[Kwang Yul Seo]

Qt build failed because 34493 is not landed yet.

[Kwang Yul Seo]

Created attachment 47958 [details]
Use WTF::getLocalTime 

Submit the same patch again to re-run build bots.

[Eric Seidel (no email)]

Comment on attachment 47958 [details]
Use WTF::getLocalTime 

OK.

[WebKit Commit Bot]

Comment on attachment 47958 [details]
Use WTF::getLocalTime 

Clearing flags on attachment: 47958

Committed r54256: <http://trac.webkit.org/changeset/54256>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.