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.
Created attachment 172900 [details] Patch
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.
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.
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.
Created attachment 172944 [details] Patch
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.
(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.
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 ?
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.
(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.
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. :)
(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.
Comment on attachment 172966 [details] Patch LGTM
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));
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).
(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.
Created attachment 172994 [details] Patch Changed to String::fromUTF8(path)
Comment on attachment 172994 [details] Patch LGTM.
Comment on attachment 172994 [details] Patch Clearing flags on attachment: 172994 Committed r133880: <http://trac.webkit.org/changeset/133880>
All reviewed patches have been landed. Closing bug.
Build fix landed in http://trac.webkit.org/changeset/133882
(In reply to comment #21) > Build fix landed in http://trac.webkit.org/changeset/133882 Oops, thanks.