If application doesn't set user agent, WebKit EFL doesn't set user agent by default. In my opinion, even though application doesn't set user agent, we need to set user agent by default. Because, some websites give different pages according to browser's user agent.
Created attachment 71169 [details] Patch I make this patch referencing by gtk port. This patch makes a default user agent below, "Mozilla/5.0 (Unknown; U; Linux i686; ) AppleWebKit/534.7+ (KHTML, like Gecko) Version/5.0 Safari/534.7+" However, Efl port doesn't implement defaultLanguage() of Language.h yet. So, there is no language value in above user agent. We need to implement the defaultLanguage().
Attachment 71169 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1 WebKit/efl/ewk/ewk_version.h:32: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] WebKit/efl/ewk/ewk_version.h:33: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] WebKit/efl/ewk/ewk_version.h:34: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] WebKit/efl/ewk/ewk_version.h:35: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] WebKit/efl/ewk/ewk_settings.cpp:45: Alphabetical sorting problem. [build/include_order] [4] WebKit/efl/ewk/ewk_settings.cpp:78: One line control clauses should not use braces. [whitespace/braces] [4] WebKit/efl/ewk/ewk_settings.cpp:79: An else should appear on the same line as the preceding } [whitespace/newline] [4] WebKit/efl/ewk/ewk_settings.cpp:307: Extra space between return and eina_stringshare_add [whitespace/declaration] [3] Total errors found: 8 in 5 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 71169 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=71169&action=review Patch looks fine otherwise. Fix the style issues and other comments and you get my informal r+ :) > WebKit/efl/ewk/ewk_settings.cpp:80 > + uaOSVersion = eina_stringshare_add("Unknown"); Please follow the coding style here: if (condition) { statement1; statement2; } else statement3; > WebKit/efl/ewk/ewk_version.h:37 > +#endif These version macros should be set in cmakeconfig.h, so there's only one place to set the EWebKit version.
Created attachment 71233 [details] Patch Hello Leandro, Thank you for your review. I define version macros in cmakeconfig.h.cmake. So, I remove unnecessary ewk_version.h file.
Attachment 71233 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1 WebKit/efl/ewk/ewk_settings.cpp:37: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 1 in 6 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 71235 [details] Patch Fix style error. :-)
Comment on attachment 71235 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=71235&action=review > ChangeLog:8 > + If applcation(e.g EWebLauncher) doesn't set user agent, WebKit EFL doesn't If application (e.g EWebLauncher) > WebKit/efl/ChangeLog:8 > + If applcation(e.g EWebLauncher) doesn't set user agent, WebKit EFL doesn't same typo as above > WebKit/efl/ewk/ewk_settings.cpp:47 > +#endif why the #ifdef? See above... there are already some headers that are specific to OS(UNIX) and the current implementation does not work without them. > WebKit/efl/ewk/ewk_settings.cpp:80 > +#endif Antognolli, what do you think about those #ifdefs?
Created attachment 71528 [details] Patch Lucas, thank you for your comment. :-) I fix the description in Changelog. I think you right. ewk_settings.cpp already uses unix header files without #ifdef. So, I remove the preprocessor macro.
Comment on attachment 71528 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=71528&action=review > cmakeconfig.h.cmake:9 > +#define WEBKIT_USER_AGENT_MAJOR_VERSION (534) > +#define WEBKIT_USER_AGENT_MINOR_VERSION (7) Hmm, why not get these from webcore?
(In reply to comment #9) > (From update of attachment 71528 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=71528&action=review > > > cmakeconfig.h.cmake:9 > > +#define WEBKIT_USER_AGENT_MAJOR_VERSION (534) > > +#define WEBKIT_USER_AGENT_MINOR_VERSION (7) > > Hmm, why not get these from webcore? Are there funtions to know user agent version in WebCore? I don't find the functions yet.
(In reply to comment #10) > (In reply to comment #9) > > (From update of attachment 71528 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=71528&action=review > > > > > cmakeconfig.h.cmake:9 > > > +#define WEBKIT_USER_AGENT_MAJOR_VERSION (534) > > > +#define WEBKIT_USER_AGENT_MINOR_VERSION (7) > > > > Hmm, why not get these from webcore? > > Are there funtions to know user agent version in WebCore? I don't find the functions yet. I believe so, but you might have to "generate" them at build time. Take a look at what Qt does.
(In reply to comment #11) > (In reply to comment #10) > > (In reply to comment #9) > > > (From update of attachment 71528 [details] [details] [details]) > > > View in context: https://bugs.webkit.org/attachment.cgi?id=71528&action=review > > > > > > > cmakeconfig.h.cmake:9 > > > > +#define WEBKIT_USER_AGENT_MAJOR_VERSION (534) > > > > +#define WEBKIT_USER_AGENT_MINOR_VERSION (7) > > > > > > Hmm, why not get these from webcore? > > > > Are there funtions to know user agent version in WebCore? I don't find the functions yet. > > I believe so, but you might have to "generate" them at build time. Take a look at what Qt does. Sorry for my late reply. I don't find that QT port handles the version macro for user agent in WebCore yet. I found a function to make user agent in QT port. QString QWebPage::userAgentForUrl(const QUrl&) const { ... 3627 // webkit/qt version 3628 secondPartTemp += QString::fromLatin1("AppleWebKit/"); 3629 secondPartTemp += qWebKitVersion(); 3630 secondPartTemp += QString::fromLatin1(" (KHTML, like Gecko) "); 3631 ... } In WebKit/qt/Api, QString qWebKitVersion() { return QString("%1.%2").arg(WEBKIT_MAJOR_VERSION).arg(WEBKIT_MINOR_VERSION); } I couldn't find the definitions of "WEBKIT_MAJOR_VERSION" and "WEBKIT_MINOR_VERSION" macros except for configure.ac. Hmm, I am not sure if qt port uses configure.ac. And,in WebKit/qt/qt_webkit_version.pri, Qt Port has some version macros as below, 1 QT_WEBKIT_VERSION = 4.9.0 2 QT_WEBKIT_MAJOR_VERSION = 4 3 QT_WEBKIT_MINOR_VERSION = 9 4 QT_WEBKIT_PATCH_VERSION = 0 I don't know if these macros are being used by user agent in qt port. In my opinion, cmakeconfig.h file is not bad place to put the USER_AGENT macros.
(In reply to comment #12) > (In reply to comment #11) > > (In reply to comment #10) > > > (In reply to comment #9) > > > > (From update of attachment 71528 [details] [details] [details] [details]) > > > > View in context: https://bugs.webkit.org/attachment.cgi?id=71528&action=review > > > > > > > > > cmakeconfig.h.cmake:9 > > > > > +#define WEBKIT_USER_AGENT_MAJOR_VERSION (534) > > > > > +#define WEBKIT_USER_AGENT_MINOR_VERSION (7) > > > > > > > > Hmm, why not get these from webcore? > > > > > > Are there funtions to know user agent version in WebCore? I don't find the functions yet. > > > > I believe so, but you might have to "generate" them at build time. Take a look at what Qt does. > > Sorry for my late reply. > > I don't find that QT port handles the version macro for user agent in WebCore yet. > > I found a function to make user agent in QT port. > > QString QWebPage::userAgentForUrl(const QUrl&) const > { > ... > 3627 // webkit/qt version > 3628 secondPartTemp += QString::fromLatin1("AppleWebKit/"); > 3629 secondPartTemp += qWebKitVersion(); > 3630 secondPartTemp += QString::fromLatin1(" (KHTML, like Gecko) "); > 3631 > ... > } > > In WebKit/qt/Api, > QString qWebKitVersion() > { > return QString("%1.%2").arg(WEBKIT_MAJOR_VERSION).arg(WEBKIT_MINOR_VERSION); > } > > I couldn't find the definitions of "WEBKIT_MAJOR_VERSION" and "WEBKIT_MINOR_VERSION" macros except for configure.ac. Hmm, I am not sure if qt port uses configure.ac. > > And,in WebKit/qt/qt_webkit_version.pri, Qt Port has some version macros as below, > > 1 QT_WEBKIT_VERSION = 4.9.0 > 2 QT_WEBKIT_MAJOR_VERSION = 4 > 3 QT_WEBKIT_MINOR_VERSION = 9 > 4 QT_WEBKIT_PATCH_VERSION = These are not the ones being used :-) They must be from somewhere else.
> > These are not the ones being used :-) They must be from somewhere else. Do you mean the macros(QT_WEBKIT_VERSION, QT_WEBKIT_MAJOR_VERSION, QT_WEBKIT_MINOR_VERSION and so on) aren't used ? To my understanding, QtWebKit uses WEBKIT_MAJOR_VERSION and WEBKIT_MINOR_VERSION. QString QWebPage::userAgentForUrl(const QUrl&) const { ... secondPartTemp += QString::fromLatin1("AppleWebKit/"); secondPartTemp += qWebKitVersion(); secondPartTemp += QString::fromLatin1(" (KHTML, like Gecko) "); ... } QString qWebKitVersion() { return QString("%1.%2").arg(WEBKIT_MAJOR_VERSION).arg(WEBKIT_MINOR_VERSION); } I can't find the macros definitions in QtWebKit. But, the macros are defined in configure.ac. Do you know where the macros are defined in Qt Port area ? I need this patch for HTML5 Video. Because, youtube give different pages according to user agent.
$~/git/WebKit/WebKit/gtk$ grep -rn WEBKIT_MINOR_VERSION * docs/GNUmakefile.in:211:WEBKIT_MINOR_VERSION = @WEBKIT_MINOR_VERSION@ docs/webkitgtk-sections.txt:633:WEBKIT_MINOR_VERSION webkit/webkitversion.h.in:29:#define WEBKIT_MINOR_VERSION (@WEBKIT_MINOR_VERSION@) webkit/webkitversion.h.in:37: (WEBKIT_MAJOR_VERSION == (major) && WEBKIT_MINOR_VERSION > (minor)) || \ webkit/webkitversion.h.in:38: (WEBKIT_MAJOR_VERSION == (major) && WEBKIT_MINOR_VERSION == (minor) && \ webkit/webkitversion.cpp:48: return WEBKIT_MINOR_VERSION; You can get the WebKit version during building it.
(In reply to comment #15) > $~/git/WebKit/WebKit/gtk$ grep -rn WEBKIT_MINOR_VERSION * > docs/GNUmakefile.in:211:WEBKIT_MINOR_VERSION = @WEBKIT_MINOR_VERSION@ > docs/webkitgtk-sections.txt:633:WEBKIT_MINOR_VERSION > webkit/webkitversion.h.in:29:#define WEBKIT_MINOR_VERSION (@WEBKIT_MINOR_VERSION@) > webkit/webkitversion.h.in:37: (WEBKIT_MAJOR_VERSION == (major) && WEBKIT_MINOR_VERSION > (minor)) || \ > webkit/webkitversion.h.in:38: (WEBKIT_MAJOR_VERSION == (major) && WEBKIT_MINOR_VERSION == (minor) && \ > webkit/webkitversion.cpp:48: return WEBKIT_MINOR_VERSION; > > You can get the WebKit version during building it. Yes, the version macros are defined in configure.ac file. - configure.ac 3 m4_define([webkit_major_version], [1]) 4 m4_define([webkit_minor_version], [3]) 5 m4_define([webkit_micro_version], [5]) 6 7 # This is the version we'll be using as part of our User-Agent string 8 # e.g., AppleWebKit/$(webkit_user_agent_version) ... 9 # 10 # Sourced from WebCore/Configurations/Version.xcconfig 11 m4_define([webkit_user_agent_major_version], [534]) 12 m4_define([webkit_user_agent_minor_version], [7]) ... 23 WEBKIT_MAJOR_VERSION=webkit_major_version 24 WEBKIT_MINOR_VERSION=webkit_minor_version 25 WEBKIT_MICRO_VERSION=webkit_micro_version 26 WEBKIT_USER_AGENT_MAJOR_VERSION=webkit_user_agent_major_version 27 WEBKIT_USER_AGENT_MINOR_VERSION=webkit_user_agent_minor_version But, as you know, WebKit EFL cannot use the configure.ac file. So, I define the version macros in cmakeconfig.h
Created attachment 72481 [details] Patch I define user agent version macros in cmake/OptionEfl.cmake as PROJECT_VERSION_XXX. So, the WEBKIT_USER_AGENT_MAJOR_VERSION / WEBKIT_USER_AGENT_MINOR_VERSION macro's vaules are also set from cmake/OptionsEfl.cmake, and they are set during the build time. 8 #define WEBKIT_USER_AGENT_MAJOR_VERSION @USER_AGENT_VERSION_MAJOR@ 9 #define WEBKIT_USER_AGENT_MINOR_VERSION @USER_AGENT_VERSION_MINOR@ Kenneth, How do you think about this approach ?
Kenneth and Antonio, I don't follow this bug recently because I was busy a little. Please review my latest patch again. I want to land this patch. I define macros for user agent in cmake/OptionsEfl.cmake. SET(USER_AGENT_VERSION_MAJOR 534) SET(USER_AGENT_VERSION_MINOR 7)
Created attachment 76978 [details] Patch Hello Tonikitoo and Antonio, I have not focused on this patch so far because of too many my company works. I make this patch again based on latest webkit. I'd like to close this bug. Could you please review this patch ?
Rafael, Lucas, Leadro?
This patch makes an user agent as below, Mozilla/5.0 (Unknown; U; Linux i686; en-US.utf8) AppleWebKit/534.7+ (KHTML, like Gecko) Version/5.0 Safari/534.7+
Comment on attachment 76978 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=76978&action=review Sorry for the delay in answering this. Please, fix the issues mentioned below. Thanks > WebKit/efl/ewk/ewk_settings.cpp:77 > +static const char* _ewk_settings_webkit_platform() > +{ > + const char* uaPlatform; > +#if PLATFORM(X11) > + uaPlatform = eina_stringshare_add("X11"); > +#else > + uaPlatform = eina_stringshare_add("Unknown"); > +#endif > + return uaPlatform; > +} > + > +static const char* _ewk_settings_webkit_os_version() > +{ > + const char* uaOSVersion; > + struct utsname name; > + > + if (uname(&name) != -1) > + uaOSVersion = eina_stringshare_add(makeString(name.sysname, ' ', name.machine).utf8().data()); > + else > + uaOSVersion = eina_stringshare_add("Unknown"); > + > + return uaOSVersion; > +} You are leaking uaOSVersion and uaPlatform. These functions can be simplified by making them return WTF::String instead of char*. > WebKit/efl/ewk/ewk_settings.cpp:298 > +/** > +* @internal > +* Gets default user agent string. > +* > +* @return user agent string. > +*/ Please, mention it's stringshared as in other functions that return char*: see ewk_view_setting_encoding_custom_get() for an example. > cmake/OptionsEfl.cmake:7 > +SET(USER_AGENT_VERSION_MAJOR 534) > +SET(USER_AGENT_VERSION_MINOR 7) You might want to use the same as Mac, which is 534.16 (see WebCore/Configurations/Version.xcconfig). Maybe you could add a comment saying to keep this in sync with that file. > cmakeconfig.h.cmake:6 > +#define WEBKITEFL_MAJOR_VERSION @PROJECT_VERSION_MAJOR@ > +#define WEBKITEFL_MINOR_VERSION @PROJECT_VERSION_MINOR@ > +#define WEBKITEFL_MICRO_VERSION @PROJECT_VERSION_PATCH@ What's this?? It's not used anywhere else and it seems wrong to me.
Created attachment 77104 [details] Patch Lucas, thank you for your review. I revise this patch according to your comment. Please review again.
Comment on attachment 77104 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=77104&action=review See remaining issues. Otherwise LGTM. > WebKit/efl/ewk/ewk_settings.cpp:298 > +/** > +* @internal > +* Gets default user agent string. > +* > +* @return user agent string. > +*/ Doc about being stringshare still missing. Please, see previous comment. > WebKit/efl/ewk/ewk_settings.cpp:307 > + > + eina_stringshare_replace(&user_agent, static_ua.utf8().data()); > + return user_agent; Why are you using eina_stringshare_replace() now? It was good with a "return eina_stringshare_add(static_ua.utf8().data());"
Created attachment 77161 [details] Patch There were misunderstandings. I fix them again. :)
Comment on attachment 77161 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=77161&action=review Looks good. > WebKit/efl/ewk/ewk_settings.cpp:55 > +WTF::String _ewk_settings_webkit_platform() do you need the _get suffix here? Like: _ewk_settings_webkit_platform_get() > WebKit/efl/ewk/ewk_settings.cpp:66 > +WTF::String _ewk_settings_webkit_os_version() Ditto? > WebKit/efl/ewk/ewk_settings.cpp:295 > +* Gets default user agent string. Gets THE default
Comment on attachment 77161 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=77161&action=review >> WebKit/efl/ewk/ewk_settings.cpp:55 >> +WTF::String _ewk_settings_webkit_platform() > > do you need the _get suffix here? > > Like: _ewk_settings_webkit_platform_get() yeah, it's good to have a _get suffix.
Comment on attachment 77161 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=77161&action=review >>> WebKit/efl/ewk/ewk_settings.cpp:55 >>> +WTF::String _ewk_settings_webkit_platform() >> >> do you need the _get suffix here? >> >> Like: _ewk_settings_webkit_platform_get() > > yeah, it's good to have a _get suffix. Make this function static. >> WebKit/efl/ewk/ewk_settings.cpp:66 >> +WTF::String _ewk_settings_webkit_os_version() > > Ditto? Make this function static. > WebKit/efl/ewk/ewk_settings.cpp:297 > +* @return A pointer to an eina_stringsahre containing the user agent string. typo: s/eina_stringsahre/eina_stringshare/
Created attachment 77185 [details] Patch Thank you. I should have checked this patch. I think your reviews are right. I fix them.
Comment on attachment 77185 [details] Patch Clearing flags on attachment: 77185 Committed r74467: <http://trac.webkit.org/changeset/74467>
All reviewed patches have been landed. Closing bug.