Bug 101530 – [EFL][WK2] Add ewk_context_additional_plugin_path_set API

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit EFL

[KwangYong Choi]

Add ewk_context_additional_plugin_path_set() to set additional plugin directory.

Currently, get additional plugin directories API is missing. Test can be modified after adding it.

[KwangYong Choi]

Created attachment 172900 [details]
Patch

[Jinwoo Song]

Comment on attachment 172900 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=172900&action=review

rebase the patch as the ewk_context has been changed.

> Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:225
> +    WKContextSetAdditionalPluginsDirectory(m_context.get(), WKStringCreateWithUTF8CString(path));

You may use WebContext::setAdditionalPluginsDirectory() instead of WKContextSetAdditionalPluginsDirectory(), as ewk_context uses WebContext now.

[Chris Dumez]

Comment on attachment 172900 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=172900&action=review

> Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:222
> +bool EwkContext::setAdditionalPluginPath(const char* path)

Should take a const String& in argument.

>> Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:225
>> +    WKContextSetAdditionalPluginsDirectory(m_context.get(), WKStringCreateWithUTF8CString(path));
> 
> You may use WebContext::setAdditionalPluginsDirectory() instead of WKContextSetAdditionalPluginsDirectory(), as ewk_context uses WebContext now.

Yes, please use C++ API instead of C API.

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_context.cpp:218
> +    /* FIXME: Get additional plugin path and compare with the path. */

I'm not sure this comment should be there. There does not seem to be an API to retrieve additional plugin paths at the moment.

[KwangYong Choi]

Comment on attachment 172900 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=172900&action=review

>> Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:222
>> +bool EwkContext::setAdditionalPluginPath(const char* path)
> 
> Should take a const String& in argument.

OK.

>>> Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:225
>>> +    WKContextSetAdditionalPluginsDirectory(m_context.get(), WKStringCreateWithUTF8CString(path));
>> 
>> You may use WebContext::setAdditionalPluginsDirectory() instead of WKContextSetAdditionalPluginsDirectory(), as ewk_context uses WebContext now.
> 
> Yes, please use C++ API instead of C API.

OK.

>> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_context.cpp:218
>> +    /* FIXME: Get additional plugin path and compare with the path. */
> 
> I'm not sure this comment should be there. There does not seem to be an API to retrieve additional plugin paths at the moment.

As you wrote, currently, there is no API to retrieve additional plugin directories. And these TRUE/FALSE tests are not enough for this API, I think. So, I added some comment here.

[KwangYong Choi]

Created attachment 172944 [details]
Patch

[Mikhail Pozdnyakov]

Comment on attachment 172944 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=172944&action=review

> Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:204
> +bool EwkContext::setAdditionalPluginPath(const String& path)

does this method make sense at all if NETSCAPE_PLUGIN_API is not enabled?

> Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:208
> +    return true;

m_webContext->setAdditionalPluginsDirectory does not return bool and we should not do it too.

[Chris Dumez]

(In reply to comment #6)
> (From update of attachment 172944 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=172944&action=review
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:204
> > +bool EwkContext::setAdditionalPluginPath(const String& path)
> 
> does this method make sense at all if NETSCAPE_PLUGIN_API is not enabled?
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:208
> > +    return true;
> 
> m_webContext->setAdditionalPluginsDirectory does not return bool and we should not do it too.

I agree with both comments.

[Gyuyoung Kim]

Comment on attachment 172944 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=172944&action=review

> Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:324
> +Eina_Bool ewk_context_additional_plugin_path_set(Ewk_Context* ewkContext, const char* path)

It seems to me ewk_settings.cpp is more proper place for this function. Can't this API be moved to there ?

[Chris Dumez]

Comment on attachment 172944 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=172944&action=review

>> Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:324
>> +Eina_Bool ewk_context_additional_plugin_path_set(Ewk_Context* ewkContext, const char* path)
> 
> It seems to me ewk_settings.cpp is more proper place for this function. Can't this API be moved to there ?

Why? It is a WebContext API not a WebPreferences one.

[Gyuyoung Kim]

(In reply to comment #9)
> (From update of attachment 172944 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=172944&action=review
> 
> >> Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:324
> >> +Eina_Bool ewk_context_additional_plugin_path_set(Ewk_Context* ewkContext, const char* path)
> > 
> > It seems to me ewk_settings.cpp is more proper place for this function. Can't this API be moved to there ?
> 
> Why? It is a WebContext API not a WebPreferences one.

Yes, I know it well. But, I just check this because this functionality just sets plugin path. It looks this is close to global setting, not related to context. Of course, this functionality can only be supported by context for now.

[KwangYong Choi]

Created attachment 172966 [details]
Patch

Applied Mikhail mentioned.

I'm not sure this API can be moved to settings. I think, it can be handled by another bug if it's possible. :)

[Gyuyoung Kim]

(In reply to comment #11)
> Created an attachment (id=172966) [details]
> Patch
> 
> Applied Mikhail mentioned.
> 
> I'm not sure this API can be moved to settings. I think, it can be handled by another bug if it's possible. :)

Yes, I think so. I just would check if this can be moved.

[Mikhail Pozdnyakov]

Comment on attachment 172966 [details]
Patch

LGTM

[Gyuyoung Kim]

Comment on attachment 172966 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=172966&action=review

> Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:326
> +    impl->setAdditionalPluginPath(String(path));

I think you need to use filenameToString as gtk port.

context->priv->context->setAdditionalPluginsDirectory(WebCore::filenameToString(directory));

[Chris Dumez]

Comment on attachment 172966 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=172966&action=review

>> Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:326
>> +    impl->setAdditionalPluginPath(String(path));
> 
> I think you need to use filenameToString as gtk port.
> 
> context->priv->context->setAdditionalPluginsDirectory(WebCore::filenameToString(directory));

Only GTK port has filenameToString().

In any case, using String(path) is seriously wrong as the path will be processed as ASCII. It should be String::fromUTF8(path).

[Gyuyoung Kim]

(In reply to comment #15)
> (From update of attachment 172966 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=172966&action=review
> 
> >> Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:326
> >> +    impl->setAdditionalPluginPath(String(path));
> > 
> > I think you need to use filenameToString as gtk port.
> > 
> > context->priv->context->setAdditionalPluginsDirectory(WebCore::filenameToString(directory));
> 
> Only GTK port has filenameToString().
> 
> In any case, using String(path) is seriously wrong as the path will be processed as ASCII. It should be String::fromUTF8(path).

If gtk port only has it, we should use String::fromUTF8() instead.

[KwangYong Choi]

Created attachment 172994 [details]
Patch

Changed to String::fromUTF8(path)

[Chris Dumez]

Comment on attachment 172994 [details]
Patch

LGTM.

[WebKit Review Bot]

Comment on attachment 172994 [details]
Patch

Clearing flags on attachment: 172994

Committed r133880: <http://trac.webkit.org/changeset/133880>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Raphael Kubo da Costa (:rakuco)]

Build fix landed in http://trac.webkit.org/changeset/133882

[Gyuyoung Kim]

(In reply to comment #21)
> Build fix landed in http://trac.webkit.org/changeset/133882

Oops, thanks.