Bug 151250 - [EFL][GTK] Remove use of String::format() in versionForUAString()
Summary: [EFL][GTK] Remove use of String::format() in versionForUAString()
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit EFL (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Gyuyoung Kim
URL:
Keywords:
Depends on:
Blocks: 30342 151340
  Show dependency treegraph
 
Reported: 2015-11-13 00:51 PST by Gyuyoung Kim
Modified: 2019-02-03 11:11 PST (History)
2 users (show)

See Also:


Attachments
Patch (2.52 KB, patch)
2015-11-13 00:52 PST, Gyuyoung Kim
no flags Details | Formatted Diff | Diff
Patch (3.39 KB, patch)
2015-11-14 07:32 PST, Gyuyoung Kim
no flags Details | Formatted Diff | Diff
Patch (2.61 KB, patch)
2015-11-14 18:16 PST, Gyuyoung Kim
no flags Details | Formatted Diff | Diff
Patch (3.37 KB, patch)
2015-11-14 19:24 PST, Gyuyoung Kim
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Gyuyoung Kim 2015-11-13 00:51:20 PST
As String::format() will be deprecated due to security problem, reimplement versionForUAString() using a macro.
Comment 1 Gyuyoung Kim 2015-11-13 00:52:51 PST
Created attachment 265473 [details]
Patch
Comment 2 Darin Adler 2015-11-13 09:27:20 PST
Comment on attachment 265473 [details]
Patch

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

> Source/WebCore/platform/efl/UserAgentEfl.cpp:64
>  static const String& versionForUAString()
>  {
> -    static NeverDestroyed<String> version(String::format("%i.%i", WEBKIT_MAJOR_VERSION, WEBKIT_MINOR_VERSION));
> +#define MAKE_VERSION(major, minor) #major "." #minor
> +    const String& version = MAKE_VERSION(WEBKIT_MAJOR_VERSION, WEBKIT_MINOR_VERSION);
>      return version;
> +#undef MAKE_VERSION
>  }

This is wrong. It will return a reference to an already-destroyed string object.
Comment 3 Darin Adler 2015-11-13 09:29:07 PST
Comment on attachment 265473 [details]
Patch

Here’s how to do it:

    static const char* versionForUAString()
    {
#define MAKE_VERSION(major, minor) #major "." #minor
    return MAKE_VERSION(USER_AGENT_GTK_MAJOR_VERSION, USER_AGENT_GTK_MINOR_VERSION);
#undef MAKE_VERSION
    }

In the EFL case we also need to change the type from const String& to const char*.
Comment 4 Gyuyoung Kim 2015-11-14 07:32:30 PST
Created attachment 265541 [details]
Patch
Comment 5 Gyuyoung Kim 2015-11-14 07:33:51 PST
(In reply to comment #3)
> Comment on attachment 265473 [details]
> Patch
> 
> Here’s how to do it:
> 
>     static const char* versionForUAString()
>     {
> #define MAKE_VERSION(major, minor) #major "." #minor
>     return MAKE_VERSION(USER_AGENT_GTK_MAJOR_VERSION,
> USER_AGENT_GTK_MINOR_VERSION);
> #undef MAKE_VERSION
>     }
> 
> In the EFL case we also need to change the type from const String& to const
> char*.

I see. I update the patch as well as platformVersionForUAString() is modified.
Comment 6 Darin Adler 2015-11-14 16:51:16 PST
Comment on attachment 265541 [details]
Patch

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

> Source/WebCore/platform/gtk/UserAgentGtk.cpp:106
> +    return MAKE_VERSION(name.sysname, name.machine);

This won’t work. I believe this will yield the string:

"name.sysnamename.machine"

This change should not be made. Testing would have shown this is incorrect.

> Source/WebCore/platform/gtk/UserAgentGtk.cpp:109
> +    return MAKE_VERSION(cpuDescriptionForUAString(), "Mac OS X");

This won’t work. I believe this will yield the string:

"cpuDescriptionForUAString()Mac OS X"

This change should not be made. Testing would have shown this is incorrect.
Comment 7 Gyuyoung Kim 2015-11-14 18:16:54 PST
Created attachment 265548 [details]
Patch
Comment 8 Darin Adler 2015-11-14 18:46:34 PST
Comment on attachment 265548 [details]
Patch

standardUserAgent in UserAgentEfl.cpp can still be improved so it doesn’t convert the const char* returned by versionForUAString to a String for the local variable version every time it’s called
Comment 9 Gyuyoung Kim 2015-11-14 19:24:10 PST
Created attachment 265549 [details]
Patch
Comment 10 Gyuyoung Kim 2015-11-14 19:25:39 PST
(In reply to comment #8)
> Comment on attachment 265548 [details]
> Patch
> 
> standardUserAgent in UserAgentEfl.cpp can still be improved so it doesn’t
> convert the const char* returned by versionForUAString to a String for the
> local variable version every time it’s called

const char* is converted to a String using ASCIILiteral in new patch.
Comment 11 WebKit Commit Bot 2015-11-14 23:56:58 PST
Comment on attachment 265549 [details]
Patch

Clearing flags on attachment: 265549

Committed r192459: <http://trac.webkit.org/changeset/192459>
Comment 12 WebKit Commit Bot 2015-11-14 23:57:03 PST
All reviewed patches have been landed.  Closing bug.