Bug 151340 – [EFL] http/tests/navigation/useragent test failed after r192459.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit EFL

[Hunseop Jeong]

https://build.webkit.org/builders/EFL%20Linux%2064-bit%20Release%20WK2/builds/25581/steps/layout-test/logs/stdio
http/tests/navigation/useragent.php [ Failure ]

16 line in http/tests/navigation/useragent.php : 
   document.write("UserAgent should match the " + userAgentTemplate + " template: " + !!userAgent.match(userAgentTemplateRegExp) + "<br>");

"!!userAgent.match(userAgentTemplateRegExp)" value is false after r192459.

I checked the useragent value at whatsmyuseragent.com using miniBrowser.
Result is "Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/WEBKIT_MAJOR_VERSION.WEBKIT_MINOR_VERSION (KHTML, like Gecko) Version/8.0 Safari/601.2.7"

[Hunseop Jeong]

Created attachment 265733 [details]
Patch

[Hunseop Jeong]

I think cause is that WEBKIT_MAJOR_VERSION and WEBKIT_MINOR_VERSION are recognized as string in #define MAKE_VERSION.
So we need one more convert process that predefined value changes to string.

[Hunseop Jeong]

Created attachment 265734 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 265734 [details]
Patch

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

> Source/WebCore/platform/efl/UserAgentEfl.cpp:61
> +#define MAKE_VERSION(major, minor) MAKE_STRING(major) "." MAKE_STRING(minor)

I'm surprised we need to have additional step to convert from number to string. LGTM.

However Darin might want to have a final look this fix before landing.

[Hunseop Jeong]

(In reply to comment #4)
> Comment on attachment 265734 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=265734&action=review
> 
> > Source/WebCore/platform/efl/UserAgentEfl.cpp:61
> > +#define MAKE_VERSION(major, minor) MAKE_STRING(major) "." MAKE_STRING(minor)
> 
> I'm surprised we need to have additional step to convert from number to
> string. LGTM.
> 
> However Darin might want to have a final look this fix before landing.

GTK already fixed it in http://trac.webkit.org/changeset/192566.
But we can't fix this issue with KaLs' patch because EFL use the WEBKIT_MAJOR_VERSION and WEBKIT_MINOR_VERSION.

[Darin Adler]

Comment on attachment 265734 [details]
Patch

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

>>> Source/WebCore/platform/efl/UserAgentEfl.cpp:61
>>> +#define MAKE_VERSION(major, minor) MAKE_STRING(major) "." MAKE_STRING(minor)
>> 
>> I'm surprised we need to have additional step to convert from number to string. LGTM.
>> 
>> However Darin might want to have a final look this fix before landing.
> 
> GTK already fixed it in http://trac.webkit.org/changeset/192566.
> But we can't fix this issue with KaLs' patch because EFL use the WEBKIT_MAJOR_VERSION and WEBKIT_MINOR_VERSION.

I am not surprised. I remember needing to do this in the past. This patch looks right to me.

> Source/WebCore/platform/gtk/UserAgentGtk.cpp:120
> -#define MAKE_VERSION(major, minor) #major "." #minor
> +#define MAKE_STRING(value) #value
> +#define MAKE_VERSION(major, minor) MAKE_STRING(major) "." MAKE_STRING(minor)
>      return MAKE_VERSION(USER_AGENT_GTK_MAJOR_VERSION, USER_AGENT_GTK_MINOR_VERSION);
> +#undef MAKE_STRING
>  #undef MAKE_VERSION

This change isn’t needed; the GTK fix is fine for them.

[Darin Adler]

Comment on attachment 265734 [details]
Patch

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

>>>> Source/WebCore/platform/efl/UserAgentEfl.cpp:61
>>>> +#define MAKE_VERSION(major, minor) MAKE_STRING(major) "." MAKE_STRING(minor)
>>> 
>>> I'm surprised we need to have additional step to convert from number to string. LGTM.
>>> 
>>> However Darin might want to have a final look this fix before landing.
>> 
>> GTK already fixed it in http://trac.webkit.org/changeset/192566.
>> But we can't fix this issue with KaLs' patch because EFL use the WEBKIT_MAJOR_VERSION and WEBKIT_MINOR_VERSION.
> 
> I am not surprised. I remember needing to do this in the past. This patch looks right to me.

I think maybe this can be done without the MAKE_VERSION macro. Might work just to do:

    return MAKE_STRING(WEBKIT_MAJOR_VERSION) "." MAKE_STRING(WEBKIT_MINOR_VERSION);

Please give it a try!

[Carlos Garcia Campos]

Comment on attachment 265734 [details]
Patch

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

>> Source/WebCore/platform/gtk/UserAgentGtk.cpp:120
>> -#define MAKE_VERSION(major, minor) #major "." #minor
>> +#define MAKE_STRING(value) #value
>> +#define MAKE_VERSION(major, minor) MAKE_STRING(major) "." MAKE_STRING(minor)
>>      return MAKE_VERSION(USER_AGENT_GTK_MAJOR_VERSION, USER_AGENT_GTK_MINOR_VERSION);
>> +#undef MAKE_STRING
>>  #undef MAKE_VERSION
> 
> This change isn’t needed; the GTK fix is fine for them.

Yes, please, don't changed this.

[Darin Adler]

Comment on attachment 265734 [details]
Patch

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

>>>>> Source/WebCore/platform/efl/UserAgentEfl.cpp:61
>>>>> +#define MAKE_VERSION(major, minor) MAKE_STRING(major) "." MAKE_STRING(minor)
>>>> 
>>>> I'm surprised we need to have additional step to convert from number to string. LGTM.
>>>> 
>>>> However Darin might want to have a final look this fix before landing.
>>> 
>>> GTK already fixed it in http://trac.webkit.org/changeset/192566.
>>> But we can't fix this issue with KaLs' patch because EFL use the WEBKIT_MAJOR_VERSION and WEBKIT_MINOR_VERSION.
>> 
>> I am not surprised. I remember needing to do this in the past. This patch looks right to me.
> 
> I think maybe this can be done without the MAKE_VERSION macro. Might work just to do:
> 
>     return MAKE_STRING(WEBKIT_MAJOR_VERSION) "." MAKE_STRING(WEBKIT_MINOR_VERSION);
> 
> Please give it a try!

I looked this up and we definitely need two levels of macros. To quote the a gcc documentation page about this: “If you want to stringify the result of expansion of a macro argument, you have to use two levels of macros.”

Too bad we got this wrong without testing it the first time. GTK didn’t need to change theirs to use strings :-(

[Gyuyoung Kim]

(In reply to comment #9)

> Too bad we got this wrong without testing it the first time. GTK didn’t need
> to change theirs to use strings :-(

Yes, I should check if original change passes the test. Sorry about that.

[Hunseop Jeong]

Created attachment 265812 [details]
Patch

[Hunseop Jeong]

(In reply to comment #9)
> Comment on attachment 265734 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=265734&action=review
> 
> >>>>> Source/WebCore/platform/efl/UserAgentEfl.cpp:61
> >>>>> +#define MAKE_VERSION(major, minor) MAKE_STRING(major) "." MAKE_STRING(minor)
> >>>> 
> >>>> I'm surprised we need to have additional step to convert from number to string. LGTM.
> >>>> 
> >>>> However Darin might want to have a final look this fix before landing.
> >>> 
> >>> GTK already fixed it in http://trac.webkit.org/changeset/192566.
> >>> But we can't fix this issue with KaLs' patch because EFL use the WEBKIT_MAJOR_VERSION and WEBKIT_MINOR_VERSION.
> >> 
> >> I am not surprised. I remember needing to do this in the past. This patch looks right to me.
> > 
> > I think maybe this can be done without the MAKE_VERSION macro. Might work just to do:
> > 
> >     return MAKE_STRING(WEBKIT_MAJOR_VERSION) "." MAKE_STRING(WEBKIT_MINOR_VERSION);
> > 
> > Please give it a try!
> 
> I looked this up and we definitely need two levels of macros. To quote the a
> gcc documentation page about this: “If you want to stringify the result of
> expansion of a macro argument, you have to use two levels of macros.”
> 
I change it to use the two levels of macro.

[WebKit Commit Bot]

Comment on attachment 265812 [details]
Patch

Clearing flags on attachment: 265812

Committed r192634: <http://trac.webkit.org/changeset/192634>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.