Add ewk_view_user_agent_set / ewk_view_user_agent_get API for setting user agent string to WK2.
Created attachment 152486 [details] Add ewk_view_user_agent_get / set APIs. Add ewk_view_user_agent_get / set APIs.
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
I'll reupload this patch later.
Created attachment 152493 [details] Patch
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.
Created attachment 152497 [details] Patch
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.
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.
Created attachment 152500 [details] Patch
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.
Created attachment 152502 [details] Patch
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.
(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);
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.
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.
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.
Created attachment 152707 [details] Patch
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?
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 ?
Created attachment 152712 [details] Patch
Created attachment 152713 [details] Patch
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 '*'.
Created attachment 152716 [details] Patch
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.
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.
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 ?
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?
Created attachment 152929 [details] Patch
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.
Created attachment 157348 [details] Patch
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
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?
Created attachment 157589 [details] Patch
Created attachment 159894 [details] Patch
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
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.
(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.
(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 :)
(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 ?
(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.
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.
It was fixed at other bug. *** This bug has been marked as a duplicate of bug 114429 ***