SSIA
Created attachment 236082 [details] Patch
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;
Created attachment 236246 [details] Patch
(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
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) ?
Created attachment 236505 [details] Patch
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
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.
Committed r172520: <http://trac.webkit.org/changeset/172520>