Bug 47903 – [EFL] Sets default user agent

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Gyuyoung Kim]

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.

[Gyuyoung Kim]

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().

[WebKit Review Bot]

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.

[Leandro Pereira]

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.

[Gyuyoung Kim]

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.

[WebKit Review Bot]

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.

[Gyuyoung Kim]

Created attachment 71235 [details]
Patch

Fix style error. :-)

[Lucas De Marchi]

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?

[Gyuyoung Kim]

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.

[Kenneth Rohde Christiansen]

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?

[Gyuyoung Kim]

(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.

[Kenneth Rohde Christiansen]

(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.

[Gyuyoung Kim]

(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.

[Kenneth Rohde Christiansen]

(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.

[Gyuyoung Kim]

> 
> 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.

[Joone Hur]

$~/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.

[Gyuyoung Kim]

(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

[Gyuyoung Kim]

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 ?

[Gyuyoung Kim]

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)

[Gyuyoung Kim]

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 ?

[Antonio Gomes]

Rafael, Lucas, Leadro?

[Gyuyoung Kim]

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+

[Lucas De Marchi]

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.

[Gyuyoung Kim]

Created attachment 77104 [details]
Patch

Lucas, thank you for your review. I revise this patch according to your comment. Please review again.

[Lucas De Marchi]

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());"

[Gyuyoung Kim]

Created attachment 77161 [details]
Patch

There were misunderstandings. I fix them again. :)

[Antonio Gomes]

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

[Lucas De Marchi]

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.

[Lucas De Marchi]

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/

[Gyuyoung Kim]

Created attachment 77185 [details]
Patch

Thank you. I should have checked this patch. I think your reviews are right. I fix them.

[WebKit Commit Bot]

Comment on attachment 77185 [details]
Patch

Clearing flags on attachment: 77185

Committed r74467: <http://trac.webkit.org/changeset/74467>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.