We need ewk APIs to set and get the user agent string to test the mobile site. Actually, there is a bug 91351 for these APIs but it seems to be obsoleted. (6 months old!), so I created a new bug for this. Also, I got okay from Heyrim to take over this bug.
Created attachment 197563 [details] Patch
Comment on attachment 197563 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=197563&action=review > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:618 > + if (WKStringIsEqual(newUserAgent.get(), currentUserAgent.get())) Why this check? This does not seem useful, especially if you don't return false in this case. > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:633 > + return WKEinaSharedString(userAgent.get()).leakString(); The caller needs to call eina_stringshare_del() on the returned string and this is not obvious nor documented. > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:997 > + ASSERT_STREQ(0, ewk_view_user_agent_get(webView())); leak > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:1001 > + ASSERT_STREQ(customUserAgent, ewk_view_user_agent_get(webView())); ditto > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:1004 > + ASSERT_STREQ(0, ewk_view_user_agent_get(webView())); ditto.
Comment on attachment 197563 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=197563&action=review > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:633 > + return WKEinaSharedString(userAgent.get()).leakString(); and here this is added to eina string share, which is not pointed out in doc... Could you please take a look how other 'const char*' properties implemented and take after? (for instance ewk_view_custom_encoding_get) > Source/WebKit2/UIProcess/API/efl/ewk_view.h:862 > +EAPI void ewk_view_user_agent_set(Evas_Object *o, const char *user_agent); EAPI Eina_Bool ?
Comment on attachment 197563 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=197563&action=review >> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:633 >> + return WKEinaSharedString(userAgent.get()).leakString(); > > The caller needs to call eina_stringshare_del() on the returned string and this is not obvious nor documented. It seems more consistent to keep the user agent as a member so that the caller does not need to call eina_stringshare_del(). You should also avoid leakString() unless you have a very good reason to use it. > Source/WebKit2/UIProcess/API/efl/ewk_view.h:862 > +EAPI void ewk_view_user_agent_set(Evas_Object *o, const char *user_agent); Most of our setters return an Eina_Bool. > Source/WebKit2/UIProcess/API/efl/ewk_view.h:872 > +EAPI const char *ewk_view_user_agent_get(const Evas_Object *o); How about ewk_view_custom_user_agent_get() ? This does not return the user agent, this returns the *custom* one if one was set.
Comment on attachment 197563 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=197563&action=review >> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:618 >> + if (WKStringIsEqual(newUserAgent.get(), currentUserAgent.get())) > > Why this check? This does not seem useful, especially if you don't return false in this case. I'll remove this line. >>>> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:633 >>>> + return WKEinaSharedString(userAgent.get()).leakString(); >>> >>> The caller needs to call eina_stringshare_del() on the returned string and this is not obvious nor documented. >> >> and here this is added to eina string share, which is not pointed out in doc... >> >> Could you please take a look how other 'const char*' properties implemented and take after? (for instance ewk_view_custom_encoding_get) > > It seems more consistent to keep the user agent as a member so that the caller does not need to call eina_stringshare_del(). You should also avoid leakString() unless you have a very good reason to use it. I'll make it consistent to other APIs with referring ewk_view_custom_encoding_get(). >>> Source/WebKit2/UIProcess/API/efl/ewk_view.h:862 >>> +EAPI void ewk_view_user_agent_set(Evas_Object *o, const char *user_agent); >> >> EAPI Eina_Bool ? > > Most of our setters return an Eina_Bool. Okay, I'll fix it. >> Source/WebKit2/UIProcess/API/efl/ewk_view.h:872 >> +EAPI const char *ewk_view_user_agent_get(const Evas_Object *o); > > How about ewk_view_custom_user_agent_get() ? This does not return the user agent, this returns the *custom* one if one was set. I'd better to use the WKPageCopyUserAgent() than WKPageCopyCustomUserAgent() because it's the right purpose of this API. Then current name may be more reasonable than before. :)
Created attachment 198231 [details] Patch Applied comments by Mikhail and Chris. Thanks for review. :)
Comment on attachment 198231 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=198231&action=review > Source/WebKit2/UIProcess/API/efl/EwkView.cpp:682 > + return m_userAgent; m_userAgent = WKEinaSharedString(AdoptWK, WKPageCopyUserAgent(wkPage())); return m_userAgent; should suffice, but other way probably is better: const char* EwkView::userAgent() const { return m_userAgent; } // could be even defined in header void EwkView::setUserAgent(const char* userAgent) { if (m_userAgent != userAgent) { m_userAgent = userAgent; WKRetainPtr<WKStringRef> wkUserAgent = adoptWK(WKStringCreateWithUTF8CString(theme)); WKPageSetCustomUserAgent(wkPage(), wkUserAgent.get()); } } and please check whether m_userAgent will require being initialized in the EwkView ctor. > Source/WebKit2/UIProcess/API/efl/EwkView.h:142 > void setCustomTextEncodingName(const String& encoding); sorry setCustomTextEncodingName wasn't the best example, maybe you could fix it either in some of your next patch?
Comment on attachment 198231 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=198231&action=review >> Source/WebKit2/UIProcess/API/efl/EwkView.cpp:682 >> + return m_userAgent; > > m_userAgent = WKEinaSharedString(AdoptWK, WKPageCopyUserAgent(wkPage())); > > return m_userAgent; > > should suffice, but other way probably is better: > > const char* EwkView::userAgent() const > { > return m_userAgent; > } // could be even defined in header > > void EwkView::setUserAgent(const char* userAgent) > { > if (m_userAgent != userAgent) { > m_userAgent = userAgent; > WKRetainPtr<WKStringRef> wkUserAgent = adoptWK(WKStringCreateWithUTF8CString(theme)); > WKPageSetCustomUserAgent(wkPage(), wkUserAgent.get()); > } > } > > and please check whether m_userAgent will require being initialized in the EwkView ctor. I'll choose the better one in my thought, too. Yes, m_userAgent needs to be initialized with the standardUserAgent() in WebPageProxy. In additionally, as the user agent is reset to default one when userAgent is 0, m_userAgent needs to be assigned with WKPageCopyUserAgent(). m_userAgent = WKPageCopyUserAgent(wkPage()); >> Source/WebKit2/UIProcess/API/efl/EwkView.h:142 >> void setCustomTextEncodingName(const String& encoding); > > sorry setCustomTextEncodingName wasn't the best example, maybe you could fix it either in some of your next patch? Sure, I'll fix this one after this patch.
Created attachment 198316 [details] Patch Applied Mikhail's comments.
Comment on attachment 198316 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=198316&action=review > Source/WebKit2/UIProcess/API/efl/EwkView.cpp:258 > + , m_userAgent(WKPageCopyUserAgent(wkPage())) adopt tag is missing > Source/WebKit2/UIProcess/API/efl/EwkView.cpp:680 > + m_userAgent = WKPageCopyUserAgent(wkPage()); it's leaking. why not m_userAgent = userAgent ?
Comment on attachment 198316 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=198316&action=review >> Source/WebKit2/UIProcess/API/efl/EwkView.cpp:258 >> + , m_userAgent(WKPageCopyUserAgent(wkPage())) > > adopt tag is missing My bad, I'll fix it. >> Source/WebKit2/UIProcess/API/efl/EwkView.cpp:680 >> + m_userAgent = WKPageCopyUserAgent(wkPage()); > > it's leaking. why not m_userAgent = userAgent ? When 'userAgent' is 0, user agent is set as a standard user agent by WKPageSetCusomUserAgent(). So IMO, m_userAgent needs to be updated from WKPageCopyUserAgent(). WebPageProxy.cpp void WebPageProxy::setCustomUserAgent(const String& customUserAgent) { ... if (m_customUserAgent.isEmpty()) { setUserAgent(standardUserAgent(m_applicationNameForUserAgent)); return; } ... }
(In reply to comment #11) > (From update of attachment 198316 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=198316&action=review > > >> Source/WebKit2/UIProcess/API/efl/EwkView.cpp:680 > >> + m_userAgent = WKPageCopyUserAgent(wkPage()); > > > > it's leaking. why not m_userAgent = userAgent ? > > When 'userAgent' is 0, user agent is set as a standard user agent by WKPageSetCusomUserAgent(). > So IMO, m_userAgent needs to be updated from WKPageCopyUserAgent(). > > WebPageProxy.cpp > void WebPageProxy::setCustomUserAgent(const String& customUserAgent) > { > ... > if (m_customUserAgent.isEmpty()) { > setUserAgent(standardUserAgent(m_applicationNameForUserAgent)); > return; > } > ... > } Ok.
Created attachment 198715 [details] Patch
Comment on attachment 198715 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=198715&action=review LGTM > Source/WebKit2/UIProcess/API/efl/EwkView.cpp:680 > + m_userAgent = WKEinaSharedString(AdoptWK, WKPageCopyUserAgent(wkPage())); think it deserves a comment, with an explanation that you had given.
Created attachment 198720 [details] Patch Added a comment in setUserAgent() method.
Comment on attachment 198720 [details] Patch LGTM.
Comment on attachment 198720 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=198720&action=review > Source/WebKit2/UIProcess/API/efl/EwkView.cpp:681 > + // When 'userAgent' is 0, user agent is set as a standard user agent by WKPageSetCusomUserAgent() Cusom -> Custom > Source/WebKit2/UIProcess/API/efl/EwkView.cpp:682 > + // so m_userAgent needs to be updated from WKPageCopyUserAgent(). from -> using > Source/WebKit2/UIProcess/API/efl/EwkView.h:277 > + mutable WKEinaSharedString m_userAgent; It does not look like it needs to be mutable. > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:1003 > + ewk_view_user_agent_set(webView(), 0); Missing ASSERT_TRUE()
Comment on attachment 198720 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=198720&action=review >> Source/WebKit2/UIProcess/API/efl/EwkView.h:277 >> + mutable WKEinaSharedString m_userAgent; > > It does not look like it needs to be mutable. indeed, I didn't pay attention it :/ thanks for noticing Chris :)
Comment on attachment 198720 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=198720&action=review >>> Source/WebKit2/UIProcess/API/efl/EwkView.h:277 >>> + mutable WKEinaSharedString m_userAgent; >> >> It does not look like it needs to be mutable. > > indeed, I didn't pay attention it :/ thanks for noticing Chris :) Indeed! I'll fix it.
Created attachment 198909 [details] Patch Applied Chris's comments.
Comment on attachment 198909 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=198909&action=review > Source/WebKit2/UIProcess/API/efl/EwkView.cpp:691 > + if (m_userAgent != userAgent) { WebKit style typically prefers early returns, i.e: if (m_userAgent == userAgent) return; This looks a bit dubious to me though, since you are comparing pointers and not character strings. I guess the eina stringshare is analogous to WTF::AtomicString? If that's the case, and the method is expected to receive only stringshared parameters, shouldn't it be EwkView::setUserAgent(Eina_Stringshare*)?
Comment on attachment 198909 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=198909&action=review >> Source/WebKit2/UIProcess/API/efl/EwkView.cpp:691 >> + if (m_userAgent != userAgent) { > > WebKit style typically prefers early returns, i.e: > > if (m_userAgent == userAgent) > return; > > This looks a bit dubious to me though, since you are comparing pointers and not character strings. > I guess the eina stringshare is analogous to WTF::AtomicString? > If that's the case, and the method is expected to receive only stringshared parameters, shouldn't it be EwkView::setUserAgent(Eina_Stringshare*)? Okay, I'll change to make early return. Operator overloading of '==' is defined in WKEinaStringShare. (m_userAgent is WKEinaStringShare) bool WKEinaSharedString::operator==(const char* str) const { return (!str || !m_string) ? (str == m_string) : !strcmp(m_string, str); } eina stringshare is interchangeable with 'const char*'.
Created attachment 199983 [details] Patch
(In reply to comment #23) > Created an attachment (id=199983) [details] > Patch Take Andrea's comment into consideration.
Comment on attachment 199983 [details] Patch Attachment 199983 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/244078 New failing tests: media/video-zoom.html
Created attachment 200007 [details] Archive of layout-test-results from webkit-ews-04 for mac-mountainlion The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: webkit-ews-04 Port: mac-mountainlion Platform: Mac OS X 10.8.2
(In reply to comment #26) > Created an attachment (id=200007) [details] > Archive of layout-test-results from webkit-ews-04 for mac-mountainlion > > The attached test failures were seen while running run-webkit-tests on the mac-ews. > Bot: webkit-ews-04 Port: mac-mountainlion Platform: Mac OS X 10.8.2 This patch has no relation to the failing test case.
Committed r149670: <http://trac.webkit.org/changeset/149670>
*** Bug 91351 has been marked as a duplicate of this bug. ***