[webkit-reviews] review granted: [Bug 34409] Use WTF::getLocalTime instead of localtime_r in FTPDirectoryDocument : [Attachment 47949] Use WTF::getLocalTime

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Kwang Yul Seo
<kwangyul.seo at gmail.com>'s request for review:
Bug 34409: Use WTF::getLocalTime instead of localtime_r in FTPDirectoryDocument
https://bugs.webkit.org/show_bug.cgi?id=34409

Attachment 47949: Use WTF::getLocalTime
https://bugs.webkit.org/attachment.cgi?id=47949&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
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