[webkit-reviews] review granted: [Bug 213884] [WK2] Add API to allow embedder to set a timezone override : [Attachment 457711] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Yury Semikhatsky
<yurys at chromium.org>'s request for review:
Bug 213884: [WK2] Add API to allow embedder to set a timezone override
https://bugs.webkit.org/show_bug.cgi?id=213884

Attachment 457711: Patch

https://bugs.webkit.org/attachment.cgi?id=457711&action=review




--- Comment #37 from Darin Adler <darin at apple.com> ---
Comment on attachment 457711
  --> https://bugs.webkit.org/attachment.cgi?id=457711
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=457711&action=review

Looks fine assuming all tests pass, but I think they’re not all passing yet

> Source/JavaScriptCore/ChangeLog:3
> +	   [WK2] Add API to allow embedder to set a timezone override

I’m not sure I love the use of "override" here as a noun meaning "an explicitly
set value that overrides the normal automatic time zone determination". I wish
there was a less peculiar term of art. But for now, I just went with it.

> Source/JavaScriptCore/runtime/DateConversion.cpp:101
> +	       String timeZoneOverride = WTF::timeZoneDisplayNameOverride();

This is not how we currently handle namespace in WTF. There should be a "using"
in the header and no WTF prefix here.

> Source/JavaScriptCore/runtime/DateConversion.cpp:102
> +	       if (!timeZoneOverride.isEmpty())

Nice style to scope the value by putting it inside the if, also allows using a
one word name:

    if (auto& override = timeZoneDisplayNameOverride(); !override.isEmpty())
	timeZoneName = override;

> Source/JavaScriptCore/runtime/DateConversion.cpp:109
> +		   const WCHAR* winTimeZoneName = t.isDST() ?
timeZoneInformation.DaylightName : timeZoneInformation.StandardName;
> +		   timeZoneName = String(winTimeZoneName);

Can we merge these into one line?

    timeZoneName = String { t.isDST() ? timeZoneInformation.DaylightName :
timeZoneInformation.StandardName };

> Source/JavaScriptCore/runtime/DateConversion.cpp:114
> +		   struct tm gtm = t;
> +		   char tzName[70];
> +		   strftime(tzName, sizeof(tzName), "%Z", &gtm);
> +		   timeZoneName = String::fromUTF8(tzName);

This code is, and always has been, incorrect for cases where the time zone
doesn’t fit into the buffer. In that case, strftime returns 0, and the contents
of the buffer are *indeterminate*. We should fix this small mistake in how we
are calling strftime by not looking at tzName if strftime returns 0.

We also could give the local variables better names, but that’s not required.

> Source/JavaScriptCore/runtime/JSDateMath.cpp:337
> +    String tz = WTF::timeZoneOverride();
> +    if (!tz.isEmpty())
> +	   return tz;

In new code, please follow WebKit coding style and use words, not
abbreviations, for variable names:

    if (auto& override = timeZoneOverride; !override.isEmpty())
	return override;

> Source/JavaScriptCore/runtime/JSDateMath.cpp:421
> +	   m_timeZoneCache = std::unique_ptr<OpaqueICUTimeZone,
OpaqueICUTimeZoneDeleter>(bitwise_cast<OpaqueICUTimeZone*>(timezone));

Frustrating that this requires direct use of bitwise_cast. We should find a
safer way to encapsulate that.

I’d also like this to be more like a one-liner the way the code just below is;
want the memory allocation and the taking ownership to be as tightly bound as
possible. If the line is too long, we can help by naming the specific
std::unique_ptr with arguments so we don’t have to write it out.

> Source/WTF/wtf/DateMath.cpp:343
> +	   int32_t offset = ucal_get(tz.cal, UCAL_ZONE_OFFSET, &status);
> +	   int32_t dstOffset = ucal_get(tz.cal, UCAL_DST_OFFSET, &status);

May need some error checking here. We are just ignoring status. That’s not
necessarily safe.

> Source/WTF/wtf/DateMath.cpp:1046
> +    // Timezone is ascii.

Not clear on the purpose of this comment.

> Source/WTF/wtf/DateMath.cpp:1051
> +    UChar* bufferStart = buffer.data();
> +    CString ctz = timeZone.utf8();
> +    if (!Unicode::convertUTF8ToUTF16(ctz.data(), ctz.data() + ctz.length(),
&bufferStart, bufferStart + timeZone.length()))
> +	   return std::nullopt;

This is a very strange way to convert a WTF::String to UTF-16. There is no
reason to first convert the UTF-8 and then back to UTF-16. A better way is:

  StringView { timeZone }.getCharactersWithUpconvert(buffer.data());

We can possibly optimize even further. Changing the argument to this function
to already be StringView will optimize still further.

> Source/WTF/wtf/DateMath.h:396
> +WTF_EXPORT_PRIVATE std::optional<Vector<UChar, 32>> isTimeZoneValid(const
String&);

I suggest changing the argument type here to StringView, since this function
takes no advantage of the fact that it’s already in a String.

This is not the correct name for a function that parses a time zone and returns
the canonical form. I would call this parseTimeZone or
computeCanonicalTimeZone, or something like that.

If the external caller only wants to check *if* the time zone is valid, I
suggest publishing only the version that returns a bool rather than returning
the parsed time zone to that caller. That can be a cover that just converts the
optional to a boolean on the way out.

> Source/WTF/wtf/DateMath.h:397
> +WTF_EXPORT_PRIVATE bool setTimeZoneOverride(const String&);

Callers are making WTF::String just to pass to this function, but that is
unnecessary. Given how it’s used, I suggest we instead have this function take
a StringView, or even a const char*. It would be easy for the callers to deal
with either of those. If we must use UTF-16, then I suggest StringView rather
than const String&, but I think const char* would work well.

> Source/WTF/wtf/DateMath.h:399
> +WTF_EXPORT_PRIVATE String& timeZoneOverride();
> +WTF_EXPORT_PRIVATE String& timeZoneDisplayNameOverride();

These should return const String&, unlesss we intend callers to use the
returned references to modify the values.

> Source/WebKit/Shared/WebProcessCreationParameters.h:259
> +    std::optional<String> timeZoneOverride;

I suggest we use String here, since String has a null value. Using
std::optional<String> is unnecessary. When the code is not generic across
multiple types, I strongly suggest not ever using optional<String>.

> Source/WebKit/UIProcess/API/glib/WebKitWebContext.cpp:400
> +    case PROP_TIME_ZONE_OVERRIDE: {
> +	   const char* timeZoneOverride = g_value_get_string(value);
> +	   if (timeZoneOverride)
> +	       webkit_web_context_set_time_zone_override(context,
timeZoneOverride);
> +	   break;
> +    }

No braces needed:

    if (auto* override = g_value_get_string(value))
	webkit_web_context_set_time_zone_override(context, override);
    break;

> Source/WebKit/WebProcess/WebProcess.cpp:505
> +    if (parameters.timeZoneOverride)
> +	   WTF::setTimeZoneOverride(*parameters.timeZoneOverride);
> +    else
> +	   WTF::setTimeZoneOverride({ });

Should not have WTF:: prefixes here. See my comments above.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/TimeZoneOverride.mm:28
> +#if PLATFORM(MAC)

This should be COCOA instead of MAC. Unless there’s some reason this doesn’t
work on iOS?