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"
Created attachment 265733 [details] Patch
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.
Created attachment 265734 [details] Patch
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.
(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.
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.
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!
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.
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 :-(
(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.
Created attachment 265812 [details] Patch
(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.
Comment on attachment 265812 [details] Patch Clearing flags on attachment: 265812 Committed r192634: <http://trac.webkit.org/changeset/192634>
All reviewed patches have been landed. Closing bug.