91351 – [EFL][WK2] Add ewk_view_user_agent_set / ewk_view_user_agent_get API.

Hyerim Bae

Reported 2012-07-15 18:22:26 PDT

Add ewk_view_user_agent_set / ewk_view_user_agent_get API for setting user agent string to WK2.

Hyerim Bae

Comment 1 2012-07-15 23:56:31 PDT

Created attachment 152486 [details] Add ewk_view_user_agent_get / set APIs. Add ewk_view_user_agent_get / set APIs.

Gyuyoung Kim

Comment 2 2012-07-16 00:12:59 PDT

Comment on attachment 152486 [details] Add ewk_view_user_agent_get / set APIs. Attachment 152486 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/13238858

Hyerim Bae

Comment 3 2012-07-16 00:24:29 PDT

I'll reupload this patch later.

Hyerim Bae

Comment 4 2012-07-16 00:49:34 PDT

Created attachment 152493 [details] Patch

Gyuyoung Kim

Comment 5 2012-07-16 00:55:50 PDT

Comment on attachment 152493 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=152493&action=review > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:970 > + OwnArrayPtr<char> buffer = adoptArrayPtr(new char[length]); Should you use this ? Is below more simple ? toImpl(userAgentString.get())->string().utf8().data(); > Source/WebKit2/UIProcess/API/efl/ewk_view.h:403 > +EAPI Eina_Bool ewk_view_user_agent_set(Evas_Object* o, const char* user_agent); Move '*' to variable side for public APIs. > Source/WebKit2/UIProcess/API/efl/ewk_view.h:412 > +EAPI const char* ewk_view_user_agent_get(const Evas_Object* o); ditto.

Hyerim Bae

Comment 6 2012-07-16 01:32:40 PDT

Created attachment 152497 [details] Patch

Gyuyoung Kim

Comment 7 2012-07-16 01:51:45 PDT

Comment on attachment 152497 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=152497&action=review > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:952 > + eina_stringshare_replace(&priv->userAgent, userAgent); I think you need to set user agent when stringshare_replace is succeeded. Please see also WK1 implementation. http://trac.webkit.org/browser/trunk/Source/WebKit/efl/ewk/ewk_view.cpp#L2044 > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:966 > + WKRetainPtr<WKStringRef> userAgentString(AdoptWK, WKPageCopyUserAgent(toAPI(priv->pageClient->page()))); By the way, is there default user agent ? IIRC, there is no default user agent if we don't set user agent. If this is true, we don't need to have this logic to get user agent. > Source/WebKit2/UIProcess/API/efl/ewk_view.h:410 > +* @return @c user agent string If my comment is correct, I think you need to update this comment as well.

Hyerim Bae

Comment 8 2012-07-16 02:27:33 PDT

Comment on attachment 152497 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=152497&action=review >> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:966 >> + WKRetainPtr<WKStringRef> userAgentString(AdoptWK, WKPageCopyUserAgent(toAPI(priv->pageClient->page()))); > > By the way, is there default user agent ? IIRC, there is no default user agent if we don't set user agent. If this is true, we don't need to have this logic to get user agent. There is a default user agent if don't set it.

Hyerim Bae

Comment 9 2012-07-16 02:33:04 PDT

Created attachment 152500 [details] Patch

Gyuyoung Kim

Comment 10 2012-07-16 02:37:52 PDT

Comment on attachment 152500 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=152500&action=review > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:947 > + EINA_SAFETY_ON_NULL_RETURN_VAL(userAgent, false); nit : Though this is not formal rule, generally, EINA_SAFETY_ON_NULL_RETURN_VAL has placed to just below EWK_VIEW_PRIV_GET_OR_RETURN() in ewk_view.cpp so far.

Hyerim Bae

Comment 11 2012-07-16 02:43:37 PDT

Created attachment 152502 [details] Patch

Gyuyoung Kim

Comment 12 2012-07-16 02:50:13 PDT

Comment on attachment 152502 [details] Patch Almost looks good to me. But, what is default user agent ? If I'm not sure whether we can return WebKit default user agent when user is not set user agent. If we can return default user agent, I think this code needs to be adjusted into WK1 as well.

Hyerim Bae

Comment 13 2012-07-16 03:04:27 PDT

(In reply to comment #12) > (From update of attachment 152502 [details]) > Almost looks good to me. But, what is default user agent ? If I'm not sure whether we can return WebKit default user agent when user is not set user agent. If we can return default user agent, I think this code needs to be adjusted into WK1 as well. The default user agent is like below. in WebKit2/UIProcess/efl/WebPageProxyEfl.cpp String WebPageProxy::standardUserAgent(const String& applicationNameForUserAgent) { ... return makeString("Mozilla/5.0 (", platform, "; ", osVersion, ") AppleWebKit/", version) + makeString(" (KHTML, like Gecko) Version/5.0 Safari/", version);

Ryuan Choi

Comment 14 2012-07-16 05:36:35 PDT

Comment on attachment 152502 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=152502&action=review > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:969 > + if (!priv->userAgent) { > + WKRetainPtr<WKStringRef> userAgentString(AdoptWK, WKPageCopyUserAgent(toAPI(priv->pageClient->page()))); > + eina_stringshare_replace(&priv->userAgent, toImpl(userAgentString.get())->string().utf8().data()); > + } > + > + return priv->userAgent; Don't you need to return default user agent if priv->userAgent is 0 ? In addition, I want a way to reset user-agent to default after changed user-agent.

Hyerim Bae

Comment 15 2012-07-16 17:20:12 PDT

Comment on attachment 152502 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=152502&action=review > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:970 > +} Don't you need to return default user agent if priv->userAgent is 0 ? > If user doesn't set the user agent, the default user agent is used. So if user get the user agent at that time without setting his own user agent, the default user agent should be returned which is actually used. In addition, I want a way to reset user-agent to default after changed user-agent. > Is there any other port to support this feature? It may be handled by user.

Chris Dumez

Comment 16 2012-07-16 22:49:07 PDT

Comment on attachment 152502 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=152502&action=review > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:966 > + eina_stringshare_replace(&priv->userAgent, toImpl(userAgentString.get())->string().utf8().data()); eina_stringshare_add() is sufficient. > Source/WebKit2/UIProcess/API/efl/ewk_view.h:397 > +* Queries to set the user agent string. "Queries to set" does not make much sense. "Sets the user agent string" maybe? > Source/WebKit2/UIProcess/API/efl/ewk_view.h:400 > +* Missing @param for user_agent. Please document what happens if user_agent is NULL. > Source/WebKit2/UIProcess/API/efl/ewk_view.h:410 > +* @return @c user agent string It should be documented that the user agent is stringshared. See ewk_intent.h documentation for examples.

Hyerim Bae

Comment 17 2012-07-17 00:30:28 PDT

Created attachment 152707 [details] Patch

Chris Dumez

Comment 18 2012-07-17 00:48:51 PDT

Comment on attachment 152707 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=152707&action=review > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:972 > + EINA_SAFETY_ON_NULL_RETURN_VAL(userAgent, false); I prefer if we accept a NULL userAgent. WKPageSetCustomUserAgent() accepts a NULL user agent so I think we should allow it as well. We should simply document in the header that this resets to the default user agent. > Source/WebKit2/UIProcess/API/efl/ewk_view.h:423 > +/** Extra space before comment? > Source/WebKit2/UIProcess/API/efl/ewk_view.h:427 > +* @param user_agent user agent, may be @c EINA_FALSE if user_agent is 0. I think you mean "may return" not "may be ". Also please use "@c NULL" instead of 0. Finally, I would prefer if passing a NULL user agents resets the user agent string to the default one. > Source/WebKit2/UIProcess/API/efl/ewk_view.h:433 > +/** Extra space before comment?

Gyuyoung Kim

Comment 19 2012-07-17 01:04:02 PDT

Comment on attachment 152707 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=152707&action=review >> Source/WebKit2/UIProcess/API/efl/ewk_view.h:423 >> +/** > > Extra space before comment? What space do you mean? >> Source/WebKit2/UIProcess/API/efl/ewk_view.h:433 >> +/** > > Extra space before comment? ditto ?

Hyerim Bae

Comment 20 2012-07-17 01:16:47 PDT

Created attachment 152712 [details] Patch

Hyerim Bae

Comment 21 2012-07-17 01:19:21 PDT

Created attachment 152713 [details] Patch

Gyuyoung Kim

Comment 22 2012-07-17 01:23:38 PDT

Comment on attachment 152713 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=152713&action=review > Source/WebKit2/UIProcess/API/efl/ewk_view.h:424 > +* Sets the user agent string I can understand what space Christope mean now. Please add a space before '*'.

Hyerim Bae

Comment 23 2012-07-17 01:27:59 PDT

Created attachment 152716 [details] Patch

Gyuyoung Kim

Comment 24 2012-07-17 01:30:59 PDT

Comment on attachment 152713 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=152713&action=review > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:977 > + } else { Should we distinguish this ? I think we just set userAgent to WKPageSetCustomUserAgent. If user wanna set NULL as user agent, it just set NULL to parameter. So, we don't need to split this condition. In addition, I don't see other ports has similar logic yet.

Hyerim Bae

Comment 25 2012-07-17 01:42:49 PDT

Comment on attachment 152713 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=152713&action=review >> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:977 >> + } else { > > Should we distinguish this ? I think we just set userAgent to WKPageSetCustomUserAgent. If user wanna set NULL as user agent, it just set NULL to parameter. So, we don't need to split this condition. > > In addition, I don't see other ports has similar logic yet. If userAgent parameter is null, the default user agent may be set by WKPageSetCustomUserAgent. Then, priv->userAgent is also set to the default user agent. That's because the const char * is managed separately in ewk_view like title, uri.

Gyuyoung Kim

Comment 26 2012-07-17 02:02:12 PDT

Comment on attachment 152713 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=152713&action=review >>> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:977 >>> + } else { >> >> Should we distinguish this ? I think we just set userAgent to WKPageSetCustomUserAgent. If user wanna set NULL as user agent, it just set NULL to parameter. So, we don't need to split this condition. >> >> In addition, I don't see other ports has similar logic yet. > > If userAgent parameter is null, the default user agent may be set by WKPageSetCustomUserAgent. > Then, priv->userAgent is also set to the default user agent. > That's because the const char * is managed separately in ewk_view like title, uri. Do you think we should not support null as user agent value? I think this is our policy. We can decide whether we return default user agent when user agent parameter is NULL. But, though user wanna set user agent with NULL value, I don't know why we should return default user agent. It seems to me other ports allow to set NULL to user agent. Is there any reason to return default user agent when user agent parameter is NULL ?

Hyerim Bae

Comment 27 2012-07-17 03:10:44 PDT

Comment on attachment 152713 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=152713&action=review >>>> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:977 >>>> + } else { >>> >>> Should we distinguish this ? I think we just set userAgent to WKPageSetCustomUserAgent. If user wanna set NULL as user agent, it just set NULL to parameter. So, we don't need to split this condition. >>> >>> In addition, I don't see other ports has similar logic yet. >> >> If userAgent parameter is null, the default user agent may be set by WKPageSetCustomUserAgent. >> Then, priv->userAgent is also set to the default user agent. >> That's because the const char * is managed separately in ewk_view like title, uri. > > Do you think we should not support null as user agent value? I think this is our policy. We can decide whether we return default user agent when user agent parameter is NULL. But, though user wanna set user agent with NULL value, I don't know why we should return default user agent. It seems to me other ports allow to set NULL to user agent. Is there any reason to return default user agent when user agent parameter is NULL ? Do you think below scenario have no problem? If so, it doesn't matter. 1. If user sets the user agent as null, then the user agent will be null if user get it by ewk_view_user_agent_get. 2. With this condition, when user access the whatsmyuseragent.com, he will see the default user agent. but he can't the default user agent by ewk_view_user_agent_get at that time. The user agents doesn't match. How do you think?

Hyerim Bae

Comment 28 2012-07-17 22:45:05 PDT

Created attachment 152929 [details] Patch

Ryuan Choi

Comment 29 2012-07-24 18:47:45 PDT

Comment on attachment 152929 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=152929&action=review > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:984 > + WKPageSetCustomUserAgent(toAPI(priv->pageClient->page()), 0); > + } else { > + if (eina_stringshare_replace(&priv->userAgent, userAgent)) { > + WKRetainPtr<WKStringRef> userAgentString(AdoptWK, WKStringCreateWithUTF8CString(userAgent)); > + WKPageSetCustomUserAgent(toAPI(priv->pageClient->page()), userAgentString.get()); > + } > + } Personally, I preferred early return without else.

Hyerim Bae

Comment 30 2012-08-08 17:39:59 PDT

Created attachment 157348 [details] Patch

Gyuyoung Kim

Comment 31 2012-08-08 18:08:52 PDT

Comment on attachment 157348 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=157348&action=review > Source/WebKit2/UIProcess/API/efl/ewk_view.h:610 > + * @return the user agent, this pointer is To be consisted with existing nice API document, I think it is better to use below comment instead of above. http://trac.webkit.org/browser/trunk/Source/WebKit2/UIProcess/API/efl/ewk_view.h#L311

Ryuan Choi

Comment 32 2012-08-08 18:19:11 PDT

Comment on attachment 157348 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=157348&action=review >> Source/WebKit2/UIProcess/API/efl/ewk_view.h:610 >> + * @return the user agent, this pointer is > > To be consisted with existing nice API document, I think it is better to use below comment instead of above. > > http://trac.webkit.org/browser/trunk/Source/WebKit2/UIProcess/API/efl/ewk_view.h#L311 And I think that we can make test cases for this. Could you make them?

Hyerim Bae

Comment 33 2012-08-09 17:15:08 PDT

Created attachment 157589 [details] Patch

Hyerim Bae

Comment 34 2012-08-22 03:48:56 PDT

Created attachment 159894 [details] Patch

Kenneth Rohde Christiansen

Comment 35 2012-08-22 03:57:04 PDT

Comment on attachment 159894 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=159894&action=review > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:1396 > +Eina_Bool ewk_view_user_agent_set(Evas_Object* ewkView, const char* userAgent) > +{ Qt WebKit1 had the more powerful api of userAgentForUrl(...) where you could return a different UA for certain URLs. Maybe such a similar api (not callback based, as that would block) could be useful > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:201 > +TEST_F(EWK2UnitTestBase, ewk_view_user_agent) > +{ > + const char* defaultUserAgent = ewk_view_user_agent_get(webView()); > + > + ASSERT_TRUE(ewk_view_user_agent_set(webView(), "Foo")); > + ASSERT_STREQ(ewk_view_user_agent_get(webView()), "Foo"); > + // Set the default user agent. > + ASSERT_TRUE(ewk_view_user_agent_set(webView(), 0)); > + ASSERT_STREQ(ewk_view_user_agent_get(webView()), defaultUserAgent); > +} Such an api is a bit useless if you don't have accessors to actually get the webkit versions. Do you? same with platform etc. Qt had something like qWebKitMinorVersion() etc

Mikhail Pozdnyakov

Comment 36 2012-08-22 03:58:49 PDT

Comment on attachment 159894 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=159894&action=review > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:1403 > + WKPageSetCustomUserAgent(toAPI(priv->pageClient->page()), userAgent ? userAgentString.get() : 0); Won't userAgentString.get() return 0 if userAgent is 0? > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:1417 > + if (priv->userAgent.isNull()) priv->userAgent = WKEinaSharedString(AdoptWK, WKPageCopyUserAgent(toAPI(priv->pageClient->page()))); > Source/WebKit2/UIProcess/API/efl/ewk_view.h:610 > + * @return user agent on success or @c NULL on failure Please mention that returned string is shared.

Simon Hausmann

Comment 37 2012-08-22 06:25:57 PDT

(In reply to comment #35) > (From update of attachment 159894 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=159894&action=review > > > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:1396 > > +Eina_Bool ewk_view_user_agent_set(Evas_Object* ewkView, const char* userAgent) > > +{ > > Qt WebKit1 had the more powerful api of userAgentForUrl(...) where you could return a different UA for certain URLs. > > Maybe such a similar api (not callback based, as that would block) could be useful I would advise against such a userAgentForUrl() API and I think it was a mistake to have it in WK1, because it has direct performance implications. Every single resource request will have to do a synchronous callback into this API and it will usually also involve converting from KURL to whatever url type is used on the API level.

Kenneth Rohde Christiansen

Comment 38 2012-08-22 07:19:03 PDT

(In reply to comment #37) > (In reply to comment #35) > > (From update of attachment 159894 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=159894&action=review > > > > > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:1396 > > > +Eina_Bool ewk_view_user_agent_set(Evas_Object* ewkView, const char* userAgent) > > > +{ > > > > Qt WebKit1 had the more powerful api of userAgentForUrl(...) where you could return a different UA for certain URLs. > > > > Maybe such a similar api (not callback based, as that would block) could be useful > > I would advise against such a userAgentForUrl() API and I think it was a mistake to have it in WK1, because it has direct performance implications. Every single resource request will have to do a synchronous callback into this API and it will usually also involve converting from KURL to whatever url type is used on the API level. If it should be done it could be like a hashmap that you add to like "addUserAgentForOrigin(...)" then there would also be less conversion and it would not need to talk to the UI side. Anyway :)

Hyerim Bae

Comment 39 2012-08-23 23:12:29 PDT

(In reply to comment #37) > (In reply to comment #35) > > (From update of attachment 159894 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=159894&action=review > > > > > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:1396 > > > +Eina_Bool ewk_view_user_agent_set(Evas_Object* ewkView, const char* userAgent) > > > +{ > > > > Qt WebKit1 had the more powerful api of userAgentForUrl(...) where you could return a different UA for certain URLs. > > > > Maybe such a similar api (not callback based, as that would block) could be useful > > I would advise against such a userAgentForUrl() API and I think it was a mistake to have it in WK1, because it has direct performance implications. Every single resource request will have to do a synchronous callback into this API and it will usually also involve converting from KURL to whatever url type is used on the API level. How do you think about kenneth's comment below ?

Simon Hausmann

Comment 40 2012-08-23 23:20:50 PDT

(In reply to comment #38) > (In reply to comment #37) > > (In reply to comment #35) > > > (From update of attachment 159894 [details] [details] [details]) > > > View in context: https://bugs.webkit.org/attachment.cgi?id=159894&action=review > > > > > > > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:1396 > > > > +Eina_Bool ewk_view_user_agent_set(Evas_Object* ewkView, const char* userAgent) > > > > +{ > > > > > > Qt WebKit1 had the more powerful api of userAgentForUrl(...) where you could return a different UA for certain URLs. > > > > > > Maybe such a similar api (not callback based, as that would block) could be useful > > > > I would advise against such a userAgentForUrl() API and I think it was a mistake to have it in WK1, because it has direct performance implications. Every single resource request will have to do a synchronous callback into this API and it will usually also involve converting from KURL to whatever url type is used on the API level. > > If it should be done it could be like a hashmap that you add to like "addUserAgentForOrigin(...)" then there would also be less conversion and it would not need to talk to the UI side. Anyway :) That's true. For simplicity I would start with a simple setUserAgent(string) API and add a setUserAgentForOrigin(origin, string) only when the use-case actually arises.

Gyuyoung Kim

Comment 41 2012-09-17 02:08:44 PDT

Comment on attachment 159894 [details] Patch Cleared review? from attachment 159894 [details] so that this bug does not appear in http://webkit.org/pending-review. If you would like this patch reviewed, please attach it to this bug or a new bug.

Ryuan Choi

Comment 42 2014-01-20 23:59:03 PST

It was fixed at other bug. *** This bug has been marked as a duplicate of bug 114429 ***

Attachments
Add ewk_view_user_agent_get / set APIs. (3.52 KB, patch) 2012-07-15 23:56 PDT, Hyerim Bae	no flags	Details Formatted Diff Diff
Patch (4.30 KB, patch) 2012-07-16 00:49 PDT, Hyerim Bae	no flags	Details Formatted Diff Diff
Patch (3.90 KB, patch) 2012-07-16 01:32 PDT, Hyerim Bae	no flags	Details Formatted Diff Diff
Patch (3.92 KB, patch) 2012-07-16 02:33 PDT, Hyerim Bae	no flags	Details Formatted Diff Diff
Patch (3.92 KB, patch) 2012-07-16 02:43 PDT, Hyerim Bae	no flags	Details Formatted Diff Diff
Patch (4.47 KB, patch) 2012-07-17 00:30 PDT, Hyerim Bae	no flags	Details Formatted Diff Diff
Patch (4.78 KB, patch) 2012-07-17 01:16 PDT, Hyerim Bae	no flags	Details Formatted Diff Diff
Patch (4.78 KB, patch) 2012-07-17 01:19 PDT, Hyerim Bae	no flags	Details Formatted Diff Diff
Patch (4.79 KB, patch) 2012-07-17 01:27 PDT, Hyerim Bae	no flags	Details Formatted Diff Diff
Patch (4.70 KB, patch) 2012-07-17 22:45 PDT, Hyerim Bae	no flags	Details Formatted Diff Diff
Patch (4.87 KB, patch) 2012-08-08 17:39 PDT, Hyerim Bae	no flags	Details Formatted Diff Diff
Patch (4.82 KB, patch) 2012-08-09 17:15 PDT, Hyerim Bae	no flags	Details Formatted Diff Diff
Patch (4.86 KB, patch) 2012-08-22 03:48 PDT, Hyerim Bae	no flags	Details Formatted Diff Diff
Show Obsolete (12) View All Add attachment proposed patch, testcase, etc.