Bug 151007 – [EFL] Add UserAgentEFl.cpp|h

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Gyuyoung Kim]

As other ports EFL port starts to have UserAgentEfl class in order to support more detailed UA.

[Gyuyoung Kim]

Created attachment 265016 [details]
Patch

[Darin Adler]

Comment on attachment 265016 [details]
Patch

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

Strange that we are actually using the prefix "Efl" instead of "EFL". I think acronyms are easier to read when they are capitalized as such.

I’m OK with this patch, but there is a lot of room for improvement. Many small things done wrong.

> Source/WebCore/platform/efl/UserAgentEfl.cpp:38
> +static const String platformForUAString()

I suggest const char* as the return type rather than String. We don’t need to allocate this string each time we call this function just to carry an ASCII literal to the caller and then get destroyed.

> Source/WebCore/platform/efl/UserAgentEfl.cpp:41
> +    return String("X11");

Should be ASCIILiteral, not String, but neither is needed if you are changing return type to const char*.

> Source/WebCore/platform/efl/UserAgentEfl.cpp:43
> +    return String("Unknown");

Should be ASCIILiteral, not String.

> Source/WebCore/platform/efl/UserAgentEfl.cpp:47
> +static const String platformVersionForUAString()

Return type should be String, not const String. All String are immutable; const String doesn’t really mean anything except that you can’t assign with the another string.

Having this return const String is like having a function that returns const int. What’s the point? You can store the result in an int or a const int whether you type it int or const int, so there’s no point of including the const there, or here.

> Source/WebCore/platform/efl/UserAgentEfl.cpp:49
> +    String osVersion;

I’d just name the local variable “version”; not sure this is really an “OS version” and even if it was don’t need to name it like that.

> Source/WebCore/platform/efl/UserAgentEfl.cpp:52
> +        osVersion = WTF::String(name.sysname) + " " + WTF::String(name.machine);

WTF:: prefix is used here, but nowhere else in the file. It should not be. The better way to write this is:

    version = makeString(name.sysname, ' ', name.machine);

It’s more efficient, doesn’t waste memory allocating two intermediate strings.

> Source/WebCore/platform/efl/UserAgentEfl.cpp:54
> +        osVersion = "Unknown";

Should be ASCIILiteral("Unknown").

> Source/WebCore/platform/efl/UserAgentEfl.cpp:62
> +static const String versionForUAString()
> +{
> +    static NeverDestroyed<const String> version(String::format("%i.%i", WEBKIT_MAJOR_VERSION, WEBKIT_MINOR_VERSION));
> +    return version;
> +}

Would like to avoid new uses of String::format; we plan to deprecate this, since it’s not type checked. Given those are compile time constants, I think this entire function can just be something like this using stringifying and token pasting instead of doing anything at runtime:

    static const char* versionForUAString()
    {
        return #WEBKIT_MAJOR_VERSION "." #WEBKIT_MINOR_VERSION;
    }

Or possibly:

    static const char* versionForUAString()
    {
        #define VERSION #WEBKIT_MAJOR_VERSION "." #WEBKIT_MINOR_VERSION
        return VERSION;
        #undef VERSION
    }

No real benefit do doing NeverDestroyed<const String> instead of NeverDestroyed<String>, but you really don’t need either here.

> Source/WebCore/platform/efl/UserAgentEfl.cpp:68
> +    String version = versionForUAString();
> +    String standardUserAgentString = "Mozilla/5.0 (" + platformForUAString() + "; " + platformVersionForUAString()
> +        + ") AppleWebKit/" + version + " (KHTML, like Gecko) Version/8.0 Safari/" + version;

Why not make the user agent string be a NeverDestroyed<String>. There’s no reason for this to recompute the string each time the function is called. It’s kind of silly to cache inside versionForUAString, which is only called here, instead of caching the string we compute here.

This code should just be:

    static NeverDestroyed<String> string = makeString("Mozilla/5.0 (", platformForUAString(), "; ", platformVersionForUAString(),
        ") AppleWebKit/", version, " (KHTML, like Gecko) Version/8.0 Safari/", version);

That will work even if the functions return const char* rather than String.

It is a bizarre strategy to repeat the EFL WebKit version twice, once as a WebKit version and a second time as a Safari version; Safari versions use a different numbering scheme so using the WebKit version for it is questionable and likely to fail at some point; if a given Safari and WebKit version seem to correspond that’s just a coincidence. You’d be better off with a hardcoded Safari version, I think. Also wish you didn’t need to make your non-Safari browser pretend to be Safari at all! Not good.

> Source/WebCore/platform/efl/UserAgentEfl.cpp:70
> +    return applicationName.isEmpty() ? standardUserAgentString : standardUserAgentString + ' ' + applicationName;

Do you leave out the application version for some good reason?

[Gyuyoung Kim]

Comment on attachment 265016 [details]
Patch

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

>> Source/WebCore/platform/efl/UserAgentEfl.cpp:47
>> +static const String platformVersionForUAString()
> 
> Return type should be String, not const String. All String are immutable; const String doesn’t really mean anything except that you can’t assign with the another string.
> 
> Having this return const String is like having a function that returns const int. What’s the point? You can store the result in an int or a const int whether you type it int or const int, so there’s no point of including the const there, or here.

Done.

>> Source/WebCore/platform/efl/UserAgentEfl.cpp:49
>> +    String osVersion;
> 
> I’d just name the local variable “version”; not sure this is really an “OS version” and even if it was don’t need to name it like that.

Done.

>> Source/WebCore/platform/efl/UserAgentEfl.cpp:52
>> +        osVersion = WTF::String(name.sysname) + " " + WTF::String(name.machine);
> 
> WTF:: prefix is used here, but nowhere else in the file. It should not be. The better way to write this is:
> 
>     version = makeString(name.sysname, ' ', name.machine);
> 
> It’s more efficient, doesn’t waste memory allocating two intermediate strings.

Done.

>> Source/WebCore/platform/efl/UserAgentEfl.cpp:54
>> +        osVersion = "Unknown";
> 
> Should be ASCIILiteral("Unknown").

Done.

>> Source/WebCore/platform/efl/UserAgentEfl.cpp:62
>> +}
> 
> Would like to avoid new uses of String::format; we plan to deprecate this, since it’s not type checked. Given those are compile time constants, I think this entire function can just be something like this using stringifying and token pasting instead of doing anything at runtime:
> 
>     static const char* versionForUAString()
>     {
>         return #WEBKIT_MAJOR_VERSION "." #WEBKIT_MINOR_VERSION;
>     }
> 
> Or possibly:
> 
>     static const char* versionForUAString()
>     {
>         #define VERSION #WEBKIT_MAJOR_VERSION "." #WEBKIT_MINOR_VERSION
>         return VERSION;
>         #undef VERSION
>     }
> 
> No real benefit do doing NeverDestroyed<const String> instead of NeverDestroyed<String>, but you really don’t need either here.

Hmm.. When I use given suggestion, there is build break on gcc 4.9.2. Any other solution ? If not, I would like to land this patch as is at the moment.

>> Source/WebCore/platform/efl/UserAgentEfl.cpp:68
>> +        + ") AppleWebKit/" + version + " (KHTML, like Gecko) Version/8.0 Safari/" + version;
> 
> Why not make the user agent string be a NeverDestroyed<String>. There’s no reason for this to recompute the string each time the function is called. It’s kind of silly to cache inside versionForUAString, which is only called here, instead of caching the string we compute here.
> 
> This code should just be:
> 
>     static NeverDestroyed<String> string = makeString("Mozilla/5.0 (", platformForUAString(), "; ", platformVersionForUAString(),
>         ") AppleWebKit/", version, " (KHTML, like Gecko) Version/8.0 Safari/", version);
> 
> That will work even if the functions return const char* rather than String.
> 
> It is a bizarre strategy to repeat the EFL WebKit version twice, once as a WebKit version and a second time as a Safari version; Safari versions use a different numbering scheme so using the WebKit version for it is questionable and likely to fail at some point; if a given Safari and WebKit version seem to correspond that’s just a coincidence. You’d be better off with a hardcoded Safari version, I think. Also wish you didn’t need to make your non-Safari browser pretend to be Safari at all! Not good.

> Why not make the user agent string be a NeverDestroyed<String>. There’s no reason for this to recompute the string each time the function is called. It’s kind of silly to cache inside versionForUAString, which is only called here, instead of caching the string we compute here. This code should just be: static NeverDestroyed<String> string = makeString("Mozilla/5.0 (", > platformForUAString(), "; ", platformVersionForUAString(), ") AppleWebKit/", version, " (KHTML, like Gecko) Version/8.0 Safari/", version);

Thanks, done.

> It is a bizarre strategy to repeat the EFL WebKit version twice, once as a WebKit version and a second time as a Safari version; Safari versions use a different numbering scheme so using the  WebKit version for it is questionable and likely to fail at some point; if a given Safari and WebKit version seem to correspond that’s just a coincidence. You’d be better off with a > hardcoded Safari version, I think. Also wish you didn’t need to make your non-Safari browser pretend to be Safari at all! Not good.

I just followed usage. However it looks your comment seems correct. I hardcorded safari version. Thank you for pointing it out !

>> Source/WebCore/platform/efl/UserAgentEfl.cpp:70
>> +    return applicationName.isEmpty() ? standardUserAgentString : standardUserAgentString + ' ' + applicationName;
> 
> Do you leave out the application version for some good reason?

This patch is just to adjust existing implementation into UserAgentFoo class. The application version is going to upload on new bug.

[Gyuyoung Kim]

Created attachment 265031 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 265031 [details]
Patch

Clearing flags on attachment: 265031

Committed r192148: <http://trac.webkit.org/changeset/192148>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

Thanks, glad you made some changes after considering my comments. On the patch as landed, I still see three minor things:

+        version = "Unknown";

This could be:

    version = ASCIILiteral("Unknown");

to be slightly more efficient.

+static const String versionForUAString()

The above type should be "const String&", not "const String".

+    String version = versionForUAString();

The type here should be const String& or auto& to avoid unnecessary reference count churn every time this function is called.

I’m also sad that the token pasting solution didn’t work. I’m sure someone can come back and get that working without String::format; I’m not sure exactly why it didn’t work. Would be good to know exactly what the compiler errors were when you tried my suggestions.

[Gyuyoung Kim]

(In reply to comment #7)
> Thanks, glad you made some changes after considering my comments. On the
> patch as landed, I still see three minor things:
> 
> +        version = "Unknown";
> 
> This could be:
> 
>     version = ASCIILiteral("Unknown");
> 
> to be slightly more efficient.
> 
> +static const String versionForUAString()
> 
> The above type should be "const String&", not "const String".
> 
> +    String version = versionForUAString();
> 
> The type here should be const String& or auto& to avoid unnecessary
> reference count churn every time this function is called.

Oops, sorry for missing those things. I fix those things in Bug 151060 together with supporting *applicationVersion* parameter. Please take a look it as well.

> I’m also sad that the token pasting solution didn’t work. I’m sure someone
> can come back and get that working without String::format; I’m not sure
> exactly why it didn’t work. Would be good to know exactly what the compiler
> errors were when you tried my suggestions.

Basically it looks gcc doesn't recognize # keyword. When I removed # keyword, there are some build errors. If I fix it, let me upstream it again !

../../Source/WebCore/platform/efl/UserAgentEfl.cpp:61:12: error: stray ‘#’ in program
     return VERSION;
            ^
../../Source/WebCore/platform/efl/UserAgentEfl.cpp:61:21: error: stray ‘#’ in program
     return VERSION;
                     ^
../../Source/WebCore/platform/efl/UserAgentEfl.cpp: In function ‘const WTF::String& WebCore::versionForUAString()’:
../../Source/WebCore/platform/efl/UserAgentEfl.cpp:61:13: error: conversion from ‘int’ to ‘const WTF::String’ is ambiguous
     return VERSION;
             ^
../../Source/WebCore/platform/efl/UserAgentEfl.cpp:61:13: note: candidates are:
In file included from ../../Source/WebCore/platform/efl/UserAgentEfl.h:29:0,
                 from ../../Source/WebCore/platform/efl/UserAgentEfl.cpp:27:
../../Source/WTF/wtf/text/WTFString.h:463:5: note: WTF::String::String(WTF::HashTableDeletedValueType) <near match>
     String(WTF::HashTableDeletedValueType) : m_impl(WTF::HashTableDeletedValue) { }
     ^
../../Source/WTF/wtf/text/WTFString.h:463:5: note:   no known conversion for argument 1 from ‘int’ to ‘WTF::HashTableDeletedValueType’
../../Source/WTF/wtf/text/WTFString.h:540:8: note: WTF::String::String(WTF::StringImpl*) <near match>
 inline String::String(StringImpl* impl)
        ^
../../Source/WTF/wtf/text/WTFString.h:540:8: note:   no known conversion for argument 1 from ‘int’ to ‘WTF::StringImpl*’
In file included from ../../Source/WebCore/platform/efl/UserAgentEfl.h:29:0,
                 from ../../Source/WebCore/platform/efl/UserAgentEfl.cpp:27:
../../Source/WTF/wtf/text/WTFString.h:112:5: note: WTF::String::String(const char*) <near match>
     WTF_EXPORT_STRING_API String(const char* characters);
     ^
../../Source/WTF/wtf/text/WTFString.h:112:5: note:   no known conversion for argument 1 from ‘int’ to ‘const char*’
../../Source/WTF/wtf/text/WTFString.h:111:5: note: WTF::String::String(const LChar*) <near match>
     WTF_EXPORT_STRING_API String(const LChar* characters);
     ^
../../Source/WTF/wtf/text/WTFString.h:111:5: note:   no known conversion for argument 1 from ‘int’ to ‘const LChar* {aka const unsigned char*}’
../../Source/WTF/wtf/text/WTFString.h:104:5: note: WTF::String::String(const UChar*) <near match>
     WTF_EXPORT_STRING_API String(const UChar*);
     ^
../../Source/WTF/wtf/text/WTFString.h:104:5: note:   no known conversion for argument 1 from ‘int’ to ‘const UChar* {aka const short unsigned int*}’
../../Source/WebCore/platform/efl/UserAgentEfl.cpp:61:17: error: expected ‘;’ before string constant
     return VERSION;
                 ^
../../Source/WebCore/platform/efl/UserAgentEfl.cpp:61:22: error: expected ‘;’ before numeric constant
     return VERSION;
                      ^
../../Source/WebCore/platform/efl/UserAgentEfl.cpp:61:23: error: statement has no effect [-Werror=unused-value]
     return VERSION;
                       ^
../../Source/WebCore/platform/efl/UserAgentEfl.cpp:65:1: error: control reaches end of non-void function [-Werror=return-type]
 }

[Darin Adler]

OK. Makes sense. Try this:

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

[Gyuyoung Kim]

(In reply to comment #9)
> OK. Makes sense. Try this:
> 
>     static const char* versionForUAString()
>     {
>         #define MAKE_VERSION(major, minor) #major "." #minor
>         return MAKE_VERSION(WEBKIT_MAJOR_VERSION, WEBKIT_MINOR_VERSION);
>         #undef MAKE_VERSION
>     }

When I adjust this code to EFL port, there was below compile error.

../../Source/WebCore/platform/efl/UserAgentEfl.cpp: In function ‘const WTF::String& WebCore::versionForUAString()’:
../../Source/WebCore/platform/efl/UserAgentEfl.cpp:61:43: error: returning reference to temporary [-Werror=return-local-addr]
         return MAKE_VERSION(WEBKIT_MAJOR_VERSION, WEBKIT_MINOR_VERSION);
                                           ^
cc1plus: all warnings being treated as errors

However this works well if we use a local variable. Is this fine for you ?

    #define MAKE_VERSION(major, minor) #major "." #minor
    const String& version = MAKE_VERSION(WEBKIT_MAJOR_VERSION, WEBKIT_MINOR_VERSION);
    return version;
    #undef MAKE_VERSION

[Gyuyoung Kim]

(In reply to comment #10)
> (In reply to comment #9)
> > OK. Makes sense. Try this:
> > 
> >     static const char* versionForUAString()
> >     {
> >         #define MAKE_VERSION(major, minor) #major "." #minor
> >         return MAKE_VERSION(WEBKIT_MAJOR_VERSION, WEBKIT_MINOR_VERSION);
> >         #undef MAKE_VERSION
> >     }
> 
> When I adjust this code to EFL port, there was below compile error.
> 
> ../../Source/WebCore/platform/efl/UserAgentEfl.cpp: In function ‘const
> WTF::String& WebCore::versionForUAString()’:
> ../../Source/WebCore/platform/efl/UserAgentEfl.cpp:61:43: error: returning
> reference to temporary [-Werror=return-local-addr]
>          return MAKE_VERSION(WEBKIT_MAJOR_VERSION, WEBKIT_MINOR_VERSION);
>                                            ^
> cc1plus: all warnings being treated as errors
> 
> However this works well if we use a local variable. Is this fine for you ?
> 
>     #define MAKE_VERSION(major, minor) #major "." #minor
>     const String& version = MAKE_VERSION(WEBKIT_MAJOR_VERSION,
> WEBKIT_MINOR_VERSION);
>     return version;
>     #undef MAKE_VERSION

I upload a patch to Bug 151250. Please leave your comment there.