Bug 135640 – [EFL] Add API to set application name for the user agent

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Ryuan Choi]

SSIA

[Ryuan Choi]

Created attachment 236082 [details]
Patch

[Grzegorz Czajkowski]

Comment on attachment 236082 [details]
Patch

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

> Source/WebKit2/UIProcess/API/efl/ewk_view.h:727
> + * @return @c eina_stringshare containing the current user agent, or

Does it return application name only or whole user agent string (containing the application name if it was previously set?)

> Source/WebKit2/UIProcess/API/efl/ewk_view.h:739
> + * Since browser war, most browsers contain extra compoents such as 'Mozilla/5.0' in the user agent string.

I am wondering if this sentence introduces anything interesting to the user.

> Source/WebKit2/UIProcess/API/efl/ewk_view.h:741
> + * Using this API,

Nit: As a result of the usage this API,...

> Source/WebKit2/UIProcess/API/efl/ewk_view.h:742
> + * the user agent string will consist of the common components which system provides and @a application_name.

system -> web engine ?

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:1078
> +    ASSERT_TRUE(ewk_view_user_agent_set(webView(), customUserAgent));
> +    ASSERT_STREQ(customUserAgent, ewk_view_user_agent_get(webView()));

What does it actually test? We have a separate ewk_view_user_agent test to verify such behavior. Or maybe we should check what ewk_view_application_name_for_user_agent_get(webView()) returns after setting user agent?

> Source/WebKit2/UIProcess/efl/WebPageProxyEfl.cpp:71
> +    if (!applicationNameForUserAgent)
> +        return standardUserAgentString;
> +
> +    return standardUserAgentString + ' ' + applicationNameForUserAgent;

Nit: return applicationNameForUserAgent.isEmpty() ? standardUserAgentString : standardUserAgentString + ' ' + applicationNameForUserAgent;

[Ryuan Choi]

Created attachment 236246 [details]
Patch

[Ryuan Choi]

(In reply to comment #2)
> (From update of attachment 236082 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=236082&action=review
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_view.h:727
> > + * @return @c eina_stringshare containing the current user agent, or
> 
> Does it return application name only or whole user agent string (containing the application name if it was previously set?)
> 
My mistake, fixed.

> > Source/WebKit2/UIProcess/API/efl/ewk_view.h:739
> > + * Since browser war, most browsers contain extra compoents such as 'Mozilla/5.0' in the user agent string.
> 
> I am wondering if this sentence introduces anything interesting to the user.
> 
Removed.

> > Source/WebKit2/UIProcess/API/efl/ewk_view.h:741
> > + * Using this API,
> 
> Nit: As a result of the usage this API,...
> 
I changed as other way.

> > Source/WebKit2/UIProcess/API/efl/ewk_view.h:742
> > + * the user agent string will consist of the common components which system provides and @a application_name.
> 
> system -> web engine ?
Done.

> 
> > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:1078
> > +    ASSERT_TRUE(ewk_view_user_agent_set(webView(), customUserAgent));
> > +    ASSERT_STREQ(customUserAgent, ewk_view_user_agent_get(webView()));
> 
> What does it actually test? We have a separate ewk_view_user_agent test to verify such behavior. Or maybe we should check what ewk_view_application_name_for_user_agent_get(webView()) returns after setting user agent?
I want to test custom user agent has high priority.
But you mentioned, it looks not good.
I changed.

> 
> > Source/WebKit2/UIProcess/efl/WebPageProxyEfl.cpp:71
> > +    if (!applicationNameForUserAgent)
> > +        return standardUserAgentString;
> > +
> > +    return standardUserAgentString + ' ' + applicationNameForUserAgent;
> 
> Nit: return applicationNameForUserAgent.isEmpty() ? standardUserAgentString : standardUserAgentString + ' ' + applicationNameForUserAgent;

Done

[Grzegorz Czajkowski]

Comment on attachment 236246 [details]
Patch

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

> Source/WebKit2/ChangeLog:10
> +        It's usefull for application to set only application information without knowledge

Typo: useful

> Source/WebKit2/UIProcess/API/efl/ewk_view.h:743
> + * If custom user agent it not set by ewk_view_user_agent_set(),

Typo: it -> is

> Source/WebKit2/UIProcess/API/efl/ewk_view.h:748
> + * @param o view to set the user agent

to set application name for the user agent

> Source/WebKit2/UIProcess/API/efl/ewk_view.h:749
> + * @param application_name the application_name to set or @c NULL to restore the default one

would be nice to specify what will be restored (the common user agent string without the application name?)

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:1077
> +    ASSERT_STREQ(customUserAgent, ewk_view_user_agent_get(webView()));

Can we add additional check showing that ewk_view_user_agent_get(webView()) doesn't contain an application name (if the custom user agent was set) ?

[Ryuan Choi]

Created attachment 236505 [details]
Patch

[Grzegorz Czajkowski]

Comment on attachment 236505 [details]
Patch

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

LGTM.

> Source/WebKit2/UIProcess/API/efl/ewk_view.h:743
> + * If custom user agent it not set by ewk_view_user_agent_set(),

it - is

> Source/WebKit2/UIProcess/API/efl/ewk_view.h:749
> + * @param application_name the application_name to set or @c NULL to remove application_name in the common user agent string.

in -> from

[Gyuyoung Kim]

Comment on attachment 236505 [details]
Patch

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

LGTM too.

> Source/WebKit2/UIProcess/API/efl/ewk_view.h:715
> + * @note If you just want to add your application name in ther user agent,

typo : ther -> the or their ?

> Source/WebKit2/UIProcess/API/efl/ewk_view.h:741
> + * where the origin server selects suitable content or operating parameters for the response (wikipedia).

IMHO, "(wikipedia)" looks redundant comment.

[Ryuan Choi]

Committed r172520: <http://trac.webkit.org/changeset/172520>