Bug 114429 – [EFL][WK2] Add ewk APIs for setting and getting user agent

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Jinwoo Song]

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.

[Jinwoo Song]

Created attachment 197563 [details]
Patch

[Chris Dumez]

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.

[Mikhail Pozdnyakov]

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 ?

[Chris Dumez]

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.

[Jinwoo Song]

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. :)

[Jinwoo Song]

Created attachment 198231 [details]
Patch

Applied comments by Mikhail and Chris. Thanks for review. :)

[Mikhail Pozdnyakov]

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?

[Jinwoo Song]

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.

[Jinwoo Song]

Created attachment 198316 [details]
Patch

Applied Mikhail's comments.

[Mikhail Pozdnyakov]

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 ?

[Jinwoo Song]

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;                                                                                  
    }
    ...
}

[Mikhail Pozdnyakov]

(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.

[Jinwoo Song]

Created attachment 198715 [details]
Patch

[Mikhail Pozdnyakov]

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.

[Jinwoo Song]

Created attachment 198720 [details]
Patch

Added a comment in setUserAgent() method.

[Mikhail Pozdnyakov]

Comment on attachment 198720 [details]
Patch

LGTM.

[Chris Dumez]

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()

[Mikhail Pozdnyakov]

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 :)

[Jinwoo Song]

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.

[Jinwoo Song]

Created attachment 198909 [details]
Patch

Applied Chris's comments.

[Andreas Kling]

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*)?

[Jinwoo Song]

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*'.

[Jinwoo Song]

Created attachment 199983 [details]
Patch

[Jinwoo Song]

(In reply to comment #23)
> Created an attachment (id=199983) [details]
> Patch

Take Andrea's comment into consideration.

[Build Bot]

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

[Build Bot]

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

[Jinwoo Song]

(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.

[Ryuan Choi]

Committed r149670: <http://trac.webkit.org/changeset/149670>

[Ryuan Choi]

*** Bug 91351 has been marked as a duplicate of this bug. ***