Bug 240327 – [GTK][WPE] Do not return pointer to disposed timezone string

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Yury Semikhatsky]

[GTK][WPE] Do not return pointer to disposed timezone string

[Yury Semikhatsky]

Created attachment 459196 [details]
Patch

[Yury Semikhatsky]

Created attachment 459201 [details]
Patch

[Michael Catanzaro]

Comment on attachment 459201 [details]
Patch

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

> Source/WebKit/UIProcess/API/glib/WebKitWebContext.cpp:396
> +        if (const auto* override = g_value_get_string(value); isTimeZoneValid(override))

IMO it's more readable if you use two ifs rather than a semicolon.

> Source/WebKit/UIProcess/API/glib/WebKitWebContext.cpp:-1883
> -/**
> - * webkit_web_context_set_time_zone_override:
> - * @context: a #WebKitWebContext
> - * @time_zone_override: value to set
> - *
> - * Set the #WebKitWebContext:time-zone-override property. Refer to the IANA database for valid
> - * specifiers, https://en.wikipedia.org/wiki/List_of_tz_database_time_zones
> - *
> - * Since: 2.38
> - */
> -void webkit_web_context_set_time_zone_override(WebKitWebContext* context, const gchar* timeZoneOverride)

Hm, why are you removing this?

Since it's declared in public headers, you need to remove it from the headers too.

> Source/WebKit/UIProcess/API/glib/WebKitWebContext.cpp:-1888
> -    context->priv->timeZoneOverride = String::fromUTF8(timeZoneOverride);

If you don't want to remove the setter, I think you can just do:

context->priv->timeZoneOverride = timeZoneOverride

?

[Yury Semikhatsky]

Comment on attachment 459201 [details]
Patch

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

>> Source/WebKit/UIProcess/API/glib/WebKitWebContext.cpp:396
>> +        if (const auto* override = g_value_get_string(value); isTimeZoneValid(override))
> 
> IMO it's more readable if you use two ifs rather than a semicolon.

Changed. I personally prefer single statement per line but in another review[1] I was suggested reduce the scope of variables to the if block when possible.


[1] https://bugs.webkit.org/show_bug.cgi?id=213884#c37

>> Source/WebKit/UIProcess/API/glib/WebKitWebContext.cpp:-1883
>> -void webkit_web_context_set_time_zone_override(WebKitWebContext* context, const gchar* timeZoneOverride)
> 
> Hm, why are you removing this?
> 
> Since it's declared in public headers, you need to remove it from the headers too.

Removed it from the header as well. There is no point in this method as calling it after context has been created will not update WebProcessPool configuration (and that is intentional as we don't want to have mixed time zones for pages in one pool).

[Yury Semikhatsky]

Created attachment 459236 [details]
Patch

[Michael Catanzaro]

(In reply to Yury Semikhatsky from comment #4)
> Removed it from the header as well. There is no point in this method as
> calling it after context has been created will not update WebProcessPool
> configuration (and that is intentional as we don't want to have mixed time
> zones for pages in one pool).

Ah yes, I see the property is construct only, so that's a good clue that there should definitely not be a setter function.

Another concern: removing it without breaking any tests reveals that there was no API test for it, which would have caught that it could not work.

[EWS]

Committed r294109 (250492@main): <https://commits.webkit.org/250492@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 459236 [details].