Bug 44505

Created attachment 65249 [details] Patch adding plugins support to efl port

Attachment 65249 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1 WebCore/plugins/efl/X11EmbedContainer.cpp:236: Should have a space between // and comment [whitespace/comments] [4] WebCore/plugins/PluginView.h:360: More than one command on the same line in if [whitespace/parens] [4] Total errors found: 2 in 12 files If any of these errors are false positives, please file a bug against check-webkit-style.

Attachment 65249 [details] did not build on win: Build output: http://queues.webkit.org/results/3806069

Created attachment 65400 [details] Patch adding plugins support to efl port

(In reply to comment #4) > Created an attachment (id=65400) [details] > Patch adding plugins support to efl port This looks like the implementation from the old EFL port. If it really is, it has been already reviewed negatively; please refer to bug #35762 for details.

Comment on attachment 65400 [details] Patch adding plugins support to efl port r- due to above comments

Any news on this?

Created attachment 81212 [details] Efl plugin's part support

Attachment 81212 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1 Source/WebCore/plugins/efl/X11Window.cpp:29: The parameter name "object" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/plugins/efl/X11Window.h:56: The parameter name "evas" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/plugins/efl/X11Window.h:60: The parameter name "id" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/plugins/efl/X11Window.h:72: The parameter name "object" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/plugins/efl/X11Window.h:74: The parameter name "event" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/plugins/efl/X11Window.h:76: The parameter name "rect" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/plugins/efl/X11Window.h:81: The parameter name "rect" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/plugins/efl/X11EmbedContainer.h:36: The parameter name "evas" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/plugins/efl/X11EmbedContainer.h:36: The parameter name "view" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/plugins/efl/X11EmbedContainer.h:53: The parameter name "event" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/plugins/efl/X11EmbedContainer.h:55: The parameter name "rect" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 11 in 14 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 81214 [details] EFL's plugin support

Created attachment 81215 [details] EFL's plugin support

Fixed comments described in bug #35762. This is base version of efl's port plugin. It's good starting point, from which further implementations can be added.

Comment on attachment 81215 [details] EFL's plugin support View in context: https://bugs.webkit.org/attachment.cgi?id=81215&action=review At least, your patch should pass in efl ews. > Source/JavaScriptCore/ChangeLog:5 > + Support for efl's plugin part There is no Bug Title and Bug url. Please add them. > Source/WebCore/ChangeLog:5 > + Added missing efl plugin's part Same. > Source/WebCore/plugins/efl/PluginPackageEfl.cpp:7 > + * Copyright (C) 2009-2010 Samsung Electronics Please update date for samsung

Created attachment 85669 [details] EFL's plugin support

Comment on attachment 85669 [details] EFL's plugin support This patch is huge!

(In reply to comment #15) > (From update of attachment 85669 [details]) > This patch is huge! Hello Mariusz, As mentioned in adam comment, this patch is a little huge. Please separate this patch into more smaller patches. I think you need to make new bug for each patch. We should land this patch. Because we need this patch.

Please make patches on latest WebKit.

(In reply to comment #17) > Please make patches on latest WebKit. Please keep in mind comment #5 before updating this patch and splitting it up to ease review.

Created attachment 88807 [details] EFL's plugin support

New patch was uploaded. This is shorter version with only windowless mode template.

Comment on attachment 88807 [details] EFL's plugin support View in context: https://bugs.webkit.org/attachment.cgi?id=88807&action=review > Source/WebCore/CMakeListsEfl.txt:78 > + Please remove unnecessary blank line > Source/WebCore/CMakeListsEfl.txt:97 > + Please remove unnecessary blank line > Source/WebKit/efl/ewk/ewk_frame.cpp:2061 > +#endif // #if ENABLE(NETSCAPE_PLUGIN_API) It seems we don't need to add this comment to here. Just #endif.

Demarchi, I'd like to listen your comment for this patch.

Comment on attachment 88807 [details] EFL's plugin support View in context: https://bugs.webkit.org/attachment.cgi?id=88807&action=review > Source/WebCore/ChangeLog:8 > + No new tests. (OOPS!) You'll need to replace this line with something else before this patch can be landed. Ideally you'd say why no new tests are needed. > Source/WebCore/ChangeLog:43 > + * CMakeListsEfl.txt: > + * plugins/PluginView.h: > + * plugins/efl/PluginDataEfl.cpp: Added. > + (WebCore::PluginData::initPlugins): > + (WebCore::PluginData::refresh): > + * plugins/efl/PluginPackageEfl.cpp: Added. > + (WebCore::PluginPackage::fetchInfo): > + (WebCore::PluginPackage::NPVersion): > + (WebCore::PluginPackage::load): > + * plugins/efl/PluginViewEfl.cpp: Added. > + (WebCore::PluginView::dispatchNPEvent): > + (WebCore::PluginView::halt): > + (WebCore::PluginView::handleFocusInEvent): > + (WebCore::PluginView::handleFocusOutEvent): > + (WebCore::PluginView::restart): > + (WebCore::PluginView::handleKeyboardEvent): > + (WebCore::PluginView::handleMouseEvent): > + (WebCore::PluginView::updatePluginWidget): > + (WebCore::PluginView::setFocus): > + (WebCore::PluginView::show): > + (WebCore::PluginView::hide): > + (WebCore::PluginView::paint): > + (WebCore::PluginView::setParent): > + (WebCore::PluginView::setNPWindowRect): > + (WebCore::PluginView::setNPWindowIfNeeded): > + (WebCore::PluginView::setParentVisible): > + (WebCore::PluginView::handlePostReadFile): > + (WebCore::PluginView::platformGetValueStatic): > + (WebCore::PluginView::platformGetValue): > + (WebCore::PluginView::invalidateRect): > + (WebCore::PluginView::invalidateRegion): > + (WebCore::PluginView::forceRedraw): > + (WebCore::PluginView::platformStart): > + (WebCore::PluginView::platformDestroy): prepare-ChangeLog creates this boilerplate for you so that you can fill it in with details explaining why you made the changes you did. It's not meant to be left there blank. > Source/WebCore/plugins/efl/PluginDataEfl.cpp:23 > +/* > + Copyright (C) 2006, 2007 Apple Inc. All rights reserved. > + Copyright (C) 2008 Trolltech ASA > + Copyright (C) 2008 Collabora Ltd. All rights reserved. > + Copyright (C) 2008 INdT - Instituto Nokia de Tecnologia > + Copyright (C) 2009-2010 ProFUSION embedded systems > + Copyright (C) 2009-2011 Samsung Electronics > + > + This library is free software; you can redistribute it and/or > + modify it under the terms of the GNU Library General Public > + License as published by the Free Software Foundation; either > + version 2 of the License, or (at your option) any later version. > + > + This library is distributed in the hope that it will be useful, > + but WITHOUT ANY WARRANTY; without even the implied warranty of > + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > + Library General Public License for more details. > + > + You should have received a copy of the GNU Library General Public License > + along with this library; see the file COPYING.LIB. If not, write to > + the Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor, > + Boston, MA 02110-1301, USA. > +*/ Why this license instead of the standard BSD-like one? > Source/WebCore/plugins/efl/PluginDataEfl.cpp:36 > + PluginDatabase* db = PluginDatabase::installedPlugins(); > + const Vector<PluginPackage*> &plugins = db->plugins(); I don't think the db local variable is needed. & should go next to the type, not the variable name. I'm surprised the style bot doesn't flag this. > Source/WebCore/plugins/efl/PluginDataEfl.cpp:38 > + for (unsigned int i = 0; i < plugins.size(); ++i) { We just say "unsigned", not "unsigned int". But for iterating over a Vector the correct type is size_t. > Source/WebCore/plugins/efl/PluginDataEfl.cpp:42 > + PluginInfo info; > + PluginPackage* package = plugins[i]; > + > + info.name = package->name(); Please move the info declaration below the package declaration. No need to split up the use of info like this. > Source/WebCore/plugins/efl/PluginDataEfl.cpp:56 > + Vector<String> extensions = package->mimeToExtensions().get(mime.type); > + info.mimes.append(mime); > + } You don't seem to be using the extensions Vector at all. > Source/WebCore/plugins/efl/PluginDataEfl.cpp:65 > + PluginDatabase* db = PluginDatabase::installedPlugins(); > + db->refresh(); No need for the local. > Source/WebCore/plugins/efl/PluginPackageEfl.cpp:58 > + getValue = (NPP_GetValueProcPtr)dlsym(m_module, "NP_GetValue"); We prefer C++ casts to C-style casts. > Source/WebCore/plugins/efl/PluginPackageEfl.cpp:71 > + NPError err = getValue(0, NPPVpluginNameString, (void*) &buffer); Same comment here about casting. > Source/WebCore/plugins/efl/PluginPackageEfl.cpp:92 > + if (mime.size() > 0) { it's better to use !mime.isEmpty(). I'd reverse this if and turn it into an early-continue. > Source/WebCore/plugins/efl/PluginViewEfl.cpp:62 > +#include "Document.h" > +#include "DocumentLoader.h" > +#include "Element.h" > +#include "Frame.h" > +#include "FrameLoadRequest.h" > +#include "FrameLoader.h" > +#include "FrameTree.h" > +#include "FrameView.h" > +#include "GraphicsContext.h" > +#include "HTMLNames.h" > +#include "HTMLPlugInElement.h" > +#include "HostWindow.h" > +#include "Image.h" > +#include "IntRect.h" > +#include "JSDOMBinding.h" > +#include "KeyboardEvent.h" > +#include "MouseEvent.h" > +#include "NotImplemented.h" > +#include "Page.h" > +#include "PlatformMouseEvent.h" > +#include "PlatformWheelEvent.h" > +#include "PluginDebug.h" > +#include "PluginPackage.h" > +#include "RenderLayer.h" > +#include "ScriptController.h" > +#include "Settings.h" > +#include "npruntime_impl.h" > +#include "runtime/JSLock.h" > +#include "runtime/JSValue.h" > +#include "runtime_root.h" I'm surprised that all of these are needed, given how many of the functions in this file aren't really implemented. > Source/WebCore/plugins/efl/PluginViewEfl.cpp:75 > +using JSC::ExecState; > +using JSC::Interpreter; > +using JSC::JSLock; > +using JSC::JSObject; > +using JSC::UString; Are these really needed? > Source/WebCore/plugins/efl/PluginViewEfl.cpp:79 > +using namespace std; > +using namespace WTF; > +using namespace WebCore; You shouldn't need the WTF or WebCore directives. > Source/WebCore/plugins/efl/PluginViewEfl.cpp:84 > +using namespace HTMLNames; > +#if ENABLE(NETSCAPE_PLUGIN_API) Please add a blank line in here. > Source/WebCore/plugins/efl/PluginViewEfl.cpp:173 > + m_windowRect = IntRect(frameView->contentsToScreen(IntRect(frameRect().location(), frameRect().size()))); IntRect(frameRect().location(), frameRect().size()) should be equivalent to frameRect(). Why is the outer IntRect constructor needed? > Source/WebCore/plugins/efl/PluginViewEfl.cpp:184 > +void PluginView::setFocus(bool) > +{ > + m_element->document()->setFocusedNode(m_element); > + > + Widget::setFocus(true); > +} Why are you ignoring the bool argument? > Source/WebCore/plugins/efl/PluginViewEfl.cpp:225 > + else { > + if (!platformPluginWidget()) > + return; > + } You can use "else if" instead of nesting. But you don't even need to check the return value of platformPluginWidget, since your early return is right at the try end of the function. > Source/WebCore/plugins/efl/PluginViewEfl.cpp:252 > + if (filename.startsWith("file:///")) > + filename = filename.substring(8); Is stripping off the leading / of the path really correct? > Source/WebCore/plugins/efl/PluginViewEfl.cpp:309 > + *(void **)value = (Display*) ecore_x_display_get(); Again, C++ casts are preferred. > Source/WebKit/efl/ewk/ewk_frame.cpp:2040 > WTF::PassRefPtr<WebCore::Widget> ewk_frame_plugin_create(Evas_Object* o, const WebCore::IntSize& pluginSize, WebCore::HTMLPlugInElement* element, const WebCore::KURL& url, const WTF::Vector<WTF::String>& paramNames, const WTF::Vector<WTF::String>& paramValues, const WTF::String& mimeType, bool loadManually) All the WTF::s shouldn't be necessary. WTF headers have using directives that make them unnecessary. You should replace the WebCore:: qualifiers with a "using namespace WebCore;" near the top of the file. > Source/WebKit/efl/ewk/ewk_frame.cpp:2053 > + WTF::RefPtr<WebCore::PluginView> pluginView = WebCore::PluginView::create Same comment here about qualifiers.

(In reply to comment #23) > (From update of attachment 88807 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=88807&action=review > > > Source/WebCore/ChangeLog:8 > > + No new tests. (OOPS!) > > You'll need to replace this line with something else before this patch can be landed. Ideally you'd say why no new tests are needed. > > > Source/WebCore/ChangeLog:43 > > + * CMakeListsEfl.txt: > > + * plugins/PluginView.h: > > + * plugins/efl/PluginDataEfl.cpp: Added. > > + (WebCore::PluginData::initPlugins): > > + (WebCore::PluginData::refresh): > > + * plugins/efl/PluginPackageEfl.cpp: Added. > > + (WebCore::PluginPackage::fetchInfo): > > + (WebCore::PluginPackage::NPVersion): > > + (WebCore::PluginPackage::load): > > + * plugins/efl/PluginViewEfl.cpp: Added. > > + (WebCore::PluginView::dispatchNPEvent): > > + (WebCore::PluginView::halt): > > + (WebCore::PluginView::handleFocusInEvent): > > + (WebCore::PluginView::handleFocusOutEvent): > > + (WebCore::PluginView::restart): > > + (WebCore::PluginView::handleKeyboardEvent): > > + (WebCore::PluginView::handleMouseEvent): > > + (WebCore::PluginView::updatePluginWidget): > > + (WebCore::PluginView::setFocus): > > + (WebCore::PluginView::show): > > + (WebCore::PluginView::hide): > > + (WebCore::PluginView::paint): > > + (WebCore::PluginView::setParent): > > + (WebCore::PluginView::setNPWindowRect): > > + (WebCore::PluginView::setNPWindowIfNeeded): > > + (WebCore::PluginView::setParentVisible): > > + (WebCore::PluginView::handlePostReadFile): > > + (WebCore::PluginView::platformGetValueStatic): > > + (WebCore::PluginView::platformGetValue): > > + (WebCore::PluginView::invalidateRect): > > + (WebCore::PluginView::invalidateRegion): > > + (WebCore::PluginView::forceRedraw): > > + (WebCore::PluginView::platformStart): > > + (WebCore::PluginView::platformDestroy): > > prepare-ChangeLog creates this boilerplate for you so that you can fill it in with details explaining why you made the changes you did. It's not meant to be left there blank. > > > Source/WebCore/plugins/efl/PluginDataEfl.cpp:23 > > +/* > > + Copyright (C) 2006, 2007 Apple Inc. All rights reserved. > > + Copyright (C) 2008 Trolltech ASA > > + Copyright (C) 2008 Collabora Ltd. All rights reserved. > > + Copyright (C) 2008 INdT - Instituto Nokia de Tecnologia > > + Copyright (C) 2009-2010 ProFUSION embedded systems > > + Copyright (C) 2009-2011 Samsung Electronics > > + > > + This library is free software; you can redistribute it and/or > > + modify it under the terms of the GNU Library General Public > > + License as published by the Free Software Foundation; either > > + version 2 of the License, or (at your option) any later version. > > + > > + This library is distributed in the hope that it will be useful, > > + but WITHOUT ANY WARRANTY; without even the implied warranty of > > + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > > + Library General Public License for more details. > > + > > + You should have received a copy of the GNU Library General Public License > > + along with this library; see the file COPYING.LIB. If not, write to > > + the Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor, > > + Boston, MA 02110-1301, USA. > > +*/ > > Why this license instead of the standard BSD-like one? Look at the gtk port. Same license there. > > > Source/WebCore/plugins/efl/PluginDataEfl.cpp:36 > > + PluginDatabase* db = PluginDatabase::installedPlugins(); > > + const Vector<PluginPackage*> &plugins = db->plugins(); > > I don't think the db local variable is needed. > > & should go next to the type, not the variable name. I'm surprised the style bot doesn't flag this. > > > Source/WebCore/plugins/efl/PluginDataEfl.cpp:38 > > + for (unsigned int i = 0; i < plugins.size(); ++i) { > > We just say "unsigned", not "unsigned int". But for iterating over a Vector the correct type is size_t. > > > Source/WebCore/plugins/efl/PluginDataEfl.cpp:42 > > + PluginInfo info; > > + PluginPackage* package = plugins[i]; > > + > > + info.name = package->name(); > > Please move the info declaration below the package declaration. No need to split up the use of info like this. > > > Source/WebCore/plugins/efl/PluginDataEfl.cpp:56 > > + Vector<String> extensions = package->mimeToExtensions().get(mime.type); > > + info.mimes.append(mime); > > + } > > You don't seem to be using the extensions Vector at all. > > > Source/WebCore/plugins/efl/PluginDataEfl.cpp:65 > > + PluginDatabase* db = PluginDatabase::installedPlugins(); > > + db->refresh(); > > No need for the local. > > > Source/WebCore/plugins/efl/PluginPackageEfl.cpp:58 > > + getValue = (NPP_GetValueProcPtr)dlsym(m_module, "NP_GetValue"); > > We prefer C++ casts to C-style casts. > > > Source/WebCore/plugins/efl/PluginPackageEfl.cpp:71 > > + NPError err = getValue(0, NPPVpluginNameString, (void*) &buffer); > > Same comment here about casting. > > > Source/WebCore/plugins/efl/PluginPackageEfl.cpp:92 > > + if (mime.size() > 0) { > > it's better to use !mime.isEmpty(). > > I'd reverse this if and turn it into an early-continue. > > > Source/WebCore/plugins/efl/PluginViewEfl.cpp:62 > > +#include "Document.h" > > +#include "DocumentLoader.h" > > +#include "Element.h" > > +#include "Frame.h" > > +#include "FrameLoadRequest.h" > > +#include "FrameLoader.h" > > +#include "FrameTree.h" > > +#include "FrameView.h" > > +#include "GraphicsContext.h" > > +#include "HTMLNames.h" > > +#include "HTMLPlugInElement.h" > > +#include "HostWindow.h" > > +#include "Image.h" > > +#include "IntRect.h" > > +#include "JSDOMBinding.h" > > +#include "KeyboardEvent.h" > > +#include "MouseEvent.h" > > +#include "NotImplemented.h" > > +#include "Page.h" > > +#include "PlatformMouseEvent.h" > > +#include "PlatformWheelEvent.h" > > +#include "PluginDebug.h" > > +#include "PluginPackage.h" > > +#include "RenderLayer.h" > > +#include "ScriptController.h" > > +#include "Settings.h" > > +#include "npruntime_impl.h" > > +#include "runtime/JSLock.h" > > +#include "runtime/JSValue.h" > > +#include "runtime_root.h" > > I'm surprised that all of these are needed, given how many of the functions in this file aren't really implemented. > > > Source/WebCore/plugins/efl/PluginViewEfl.cpp:75 > > +using JSC::ExecState; > > +using JSC::Interpreter; > > +using JSC::JSLock; > > +using JSC::JSObject; > > +using JSC::UString; > > Are these really needed? > > > Source/WebCore/plugins/efl/PluginViewEfl.cpp:79 > > +using namespace std; > > +using namespace WTF; > > +using namespace WebCore; > > You shouldn't need the WTF or WebCore directives. > > > Source/WebCore/plugins/efl/PluginViewEfl.cpp:84 > > +using namespace HTMLNames; > > +#if ENABLE(NETSCAPE_PLUGIN_API) > > Please add a blank line in here. > > > Source/WebCore/plugins/efl/PluginViewEfl.cpp:173 > > + m_windowRect = IntRect(frameView->contentsToScreen(IntRect(frameRect().location(), frameRect().size()))); > > IntRect(frameRect().location(), frameRect().size()) should be equivalent to frameRect(). > > Why is the outer IntRect constructor needed? > > > Source/WebCore/plugins/efl/PluginViewEfl.cpp:184 > > +void PluginView::setFocus(bool) > > +{ > > + m_element->document()->setFocusedNode(m_element); > > + > > + Widget::setFocus(true); > > +} > > Why are you ignoring the bool argument? > > > Source/WebCore/plugins/efl/PluginViewEfl.cpp:225 > > + else { > > + if (!platformPluginWidget()) > > + return; > > + } > > You can use "else if" instead of nesting. But you don't even need to check the return value of platformPluginWidget, since your early return is right at the try end of the function. > > > Source/WebCore/plugins/efl/PluginViewEfl.cpp:252 > > + if (filename.startsWith("file:///")) > > + filename = filename.substring(8); > > Is stripping off the leading / of the path really correct? > > > Source/WebCore/plugins/efl/PluginViewEfl.cpp:309 > > + *(void **)value = (Display*) ecore_x_display_get(); > > Again, C++ casts are preferred. > > > Source/WebKit/efl/ewk/ewk_frame.cpp:2040 > > WTF::PassRefPtr<WebCore::Widget> ewk_frame_plugin_create(Evas_Object* o, const WebCore::IntSize& pluginSize, WebCore::HTMLPlugInElement* element, const WebCore::KURL& url, const WTF::Vector<WTF::String>& paramNames, const WTF::Vector<WTF::String>& paramValues, const WTF::String& mimeType, bool loadManually) > > All the WTF::s shouldn't be necessary. WTF headers have using directives that make them unnecessary. > > You should replace the WebCore:: qualifiers with a "using namespace WebCore;" near the top of the file. In whole ewk_frame.cpp it is common practice to use WTF::, and WebCore:: qualifiers. > > > Source/WebKit/efl/ewk/ewk_frame.cpp:2053 > > + WTF::RefPtr<WebCore::PluginView> pluginView = WebCore::PluginView::create > > Same comment here about qualifiers.

Created attachment 91654 [details] EFL's plugin support

Created attachment 96734 [details] EFL's plugin support Version synchronized with newest webkit's code.

Comment on attachment 96734 [details] EFL's plugin support View in context: https://bugs.webkit.org/attachment.cgi?id=96734&action=review > Source/JavaScriptCore/ChangeLog:7 > + Add patch summary > Source/WebCore/plugins/efl/PluginViewEfl.cpp:125 > + return; Should we use "return" here ? > Source/WebCore/plugins/efl/PluginViewEfl.cpp:283 > +#if ENABLE(NETSCAPE_PLUGIN_API) Why do you use same NETSCAPE_PLUGIN_API here ? 239 line already use this macro. > Source/WebKit/efl/ChangeLog:7 > + ChangeLog doesnt have summary

Created attachment 96740 [details] EFL's plugin support

Comment on attachment 96740 [details] EFL's plugin support View in context: https://bugs.webkit.org/attachment.cgi?id=96740&action=review Looks fine. Some nits ... > Source/WebCore/plugins/efl/PluginViewEfl.cpp:101 > + IntRect p = static_cast<FrameView*>(parent())->contentsToScreen(IntRect(0, 0, event->offsetX(), event->offsetY())); Use a more descriptive name than p > Source/WebCore/plugins/efl/PluginViewEfl.cpp:112 > + xEvent.xbutton.button = event->button() + 1; // DOM MouseEvent counts from 0, and XButtonEvent from 1 add . at the end. > Source/WebCore/plugins/efl/PluginViewEfl.cpp:190 > + if (parent) > + init(); > + else if (!platformPluginWidget()) > + return; do you need a return here? or a else-if then return?

Created attachment 97117 [details] EFL's plugin support

Comment on attachment 97117 [details] EFL's plugin support View in context: https://bugs.webkit.org/attachment.cgi?id=97117&action=review > Source/WebKit/efl/ewk/ewk_frame.cpp:2058 > + if (pluginView->status() == WebCore::PluginStatusLoadedSuccessfully) > + return pluginView; > +#else > return 0; > +#endif // #if ENABLE(NETSCAPE_PLUGIN_API) > } If plugin hasn't been loaded successfully, this function will return some invalid value. Change that #else to an #endif and it should prevent that.

Created attachment 102263 [details] EFL's plugin support

(In reply to comment #32) > Created an attachment (id=102263) [details] > EFL's plugin support Could you rebase patch or make newer?

Created attachment 105018 [details] EFL's plugin support Rebased.

Comment on attachment 105018 [details] EFL's plugin support View in context: https://bugs.webkit.org/attachment.cgi?id=105018&action=review It looks this patch is a little huge. Can you split this patch into smaller ones ? > Source/JavaScriptCore/ChangeLog:7 > + There is no description. > Source/WebCore/CMakeListsEfl.txt:13 > + "${WEBCORE_DIR}/plugins/efl" Please adhere alphabetical order. > Source/WebCore/CMakeListsEfl.txt:84 > + plugins/PluginView.cpp Please add an empty line to here. > Source/WebCore/ChangeLog:8 > + Basic functionality of plugins for efl port Missing "." at the end of line.

Created attachment 105367 [details] EFL's plugin support Fixed and rebased. This patch is already splitted.

Comment on attachment 105367 [details] EFL's plugin support View in context: https://bugs.webkit.org/attachment.cgi?id=105367&action=review Informal r- for the reasons below: > Source/WebCore/plugins/efl/PluginDataEfl.cpp:7 > + Copyright (C) 2006, 2007 Apple Inc. All rights reserved. > + Copyright (C) 2008 Trolltech ASA > + Copyright (C) 2008 Collabora Ltd. All rights reserved. > + Copyright (C) 2008 INdT - Instituto Nokia de Tecnologia > + Copyright (C) 2009-2010 ProFUSION embedded systems > + Copyright (C) 2009-2011 Samsung Electronics Do we really need so many copyrights for such a simple/small file? > Source/WebCore/plugins/efl/PluginDataEfl.cpp:12 > + This library is free software; you can redistribute it and/or > + modify it under the terms of the GNU Library General Public > + License as published by the Free Software Foundation; either > + version 2 of the License, or (at your option) any later version. Other files from this directory uses a different license. Is this intentional? > Source/WebCore/plugins/efl/PluginPackageEfl.cpp:121 > + if (m_isLoaded) { > + m_loadCount++; > + return true; > + } Small nit: you could ditch m_isLoaded and just use m_loadCount instead. > Source/WebCore/plugins/efl/PluginViewEfl.cpp:106 > + if (event->type() == eventNames().mousemoveEvent > + || event->type() == eventNames().mouseoutEvent > + || event->type() == eventNames().mouseoverEvent) { Minor nit: wouldn't a switch() statement be better here? > Source/WebCore/plugins/efl/PluginViewEfl.cpp:139 > + IntRect oldWindowRect = m_windowRect; > + IntRect oldClipRect = m_clipRect; What are these used for? > Source/WebCore/plugins/efl/PluginViewEfl.cpp:174 > +void PluginView::paint(GraphicsContext* context, const IntRect& rect) > +{ > + if (!m_isStarted) { > + paintMissingPluginIcon(context, rect); > + return; > + } if (!m_fnord) paintFooBar(); > Source/WebCore/plugins/efl/PluginViewEfl.cpp:188 > + return; Useless return statement. > Source/WebCore/plugins/efl/PluginViewEfl.cpp:224 > + if (bytesRead <= 0) > + return NPERR_FILE_NOT_FOUND; NPERR_FILE_NOT_FOUND should be returned if the file is missing or is invalid; in this case, isn't NPERR_NO_DATA more suitable? > Source/WebCore/plugins/efl/PluginViewEfl.cpp:236 > + *static_cast<uint32_t*>(value) = 0; > + *result = NPERR_NO_ERROR; Are value and result guaranteed to be non-null? > Source/WebCore/plugins/efl/PluginViewEfl.cpp:267 > + *static_cast<void **>(value) = static_cast<Display*>(ecore_x_display_get()); > + *result = NPERR_NO_ERROR; Are value and result guaranteed to be non-null? Calling ecore_x_display_get() will most likely crash WebKit if it is being run on a framebuffer or Ecore_Evas_Buffer. Since the engine can be chosen in runtime, please take care of it. (FWIW, DumpRenderTree uses Ecore_Evas_Buffer but plugin tests wouldn't run because of this function call.) > Source/WebCore/plugins/efl/PluginViewEfl.cpp:287 > + void** v = (void**)value; > + *v = windowScriptObject; Minor nit: variable 'v' can be removed if you do something along the lines of (perhaps with C++-style cast?): *(void **)value = (void*)windowScriptObject; > Source/WebCore/plugins/efl/PluginViewEfl.cpp:309 > + void** v = (void**)value; > + *v = pluginScriptObject; Same as above. > Source/WebCore/plugins/efl/PluginViewEfl.cpp:328 > + case NPNVToolkit: > + if (m_plugin->quirks().contains(PluginQuirkRequiresGtkToolKit)) { > + *((uint32_t *)value) = 2; > + *result = NPERR_NO_ERROR; What '2' means here? > Source/WebCore/plugins/efl/PluginViewEfl.cpp:334 > + return false; > + // fall through > + default: > + return false; Remove either the comment (which should be a proper sentence anyway) or the return statement. > Source/WebCore/plugins/efl/PluginViewEfl.cpp:354 > + IntRect r(rect->left, rect->top, rect->right - rect->left, rect->bottom - rect->top); > + > + invalidateRect(r); Minor nit: invalidateRect(IntRect(rect->left, ...));

Comment on attachment 105367 [details] EFL's plugin support View in context: https://bugs.webkit.org/attachment.cgi?id=105367&action=review >> Source/WebCore/plugins/efl/PluginPackageEfl.cpp:121 >> + } > > Small nit: you could ditch m_isLoaded and just use m_loadCount instead. There can be situation that m_isLoaded is set, but m_loadCount is not incremented when module is opened, but symbols(NP_Initialize, and NP_Shutdown) can't be taken. Same implementation in Gtk. >> Source/WebCore/plugins/efl/PluginViewEfl.cpp:106 >> + || event->type() == eventNames().mouseoverEvent) { > > Minor nit: wouldn't a switch() statement be better here? event->type() is type of AtomicString, so can't use switch here >> Source/WebCore/plugins/efl/PluginViewEfl.cpp:236 >> + *result = NPERR_NO_ERROR; > > Are value and result guaranteed to be non-null? result is guaranteed, but value is not checked in any port. It looks that in this matter webkit trusts plugins. Also type of value is taken from type of NPNVariable which is possibly dangerous(casting from void* to different types of returned value)

Comment on attachment 105367 [details] EFL's plugin support View in context: https://bugs.webkit.org/attachment.cgi?id=105367&action=review >> Source/WebCore/plugins/efl/PluginDataEfl.cpp:12 >> + version 2 of the License, or (at your option) any later version. > > Other files from this directory uses a different license. Is this intentional? Code was moved from GTK version, so type of license is kept. Also copyrights were moved with adding new ones. >>> Source/WebCore/plugins/efl/PluginPackageEfl.cpp:121 >>> + } >> >> Small nit: you could ditch m_isLoaded and just use m_loadCount instead. > > There can be situation that m_isLoaded is set, but m_loadCount is not incremented when module is opened, but symbols(NP_Initialize, and NP_Shutdown) can't be taken. Same implementation in Gtk. There can be situation that m_isLoaded is set, but m_loadCount is not incremented when module is opened, but symbols(NP_Initialize, and NP_Shutdown) can't be taken. Same implementation in Gtk. >>> Source/WebCore/plugins/efl/PluginViewEfl.cpp:106 >>> + || event->type() == eventNames().mouseoverEvent) { >> >> Minor nit: wouldn't a switch() statement be better here? > > event->type() is type of AtomicString, so can't use switch here event->type() is type of AtomicString, so can't use switch here >> Source/WebCore/plugins/efl/PluginViewEfl.cpp:224 >> + return NPERR_FILE_NOT_FOUND; > > NPERR_FILE_NOT_FOUND should be returned if the file is missing or is invalid; in this case, isn't NPERR_NO_DATA more suitable? From PluginDebug.cpp it looks ok: this error means that file is missing, or is INVALID, but no data is for streams. "File missing or invalid.", /* NPERR_FILE_NOT_FOUND */ "Stream contains no data.", /* NPERR_NO_DATA */ >>> Source/WebCore/plugins/efl/PluginViewEfl.cpp:236 >>> + *result = NPERR_NO_ERROR; >> >> Are value and result guaranteed to be non-null? > > result is guaranteed, but value is not checked in any port. It looks that in this matter webkit trusts plugins. Also type of value is taken from type of NPNVariable which is possibly dangerous(casting from void* to different types of returned value) result is guaranteed, but value is not checked in any port. It looks that in this matter webkit trusts plugins. Also type of value is taken from type of NPNVariable which is possibly dangerous(casting from void* to different types of returned value)

I tried to build with this patch and couldn't get it to work. Tested with Youtube, pages with Windows Media videos, and Java applets. According to the debugger, the shared objects for each tested plugin is actually opened but no reaction was observed. Also, from a broad inspection, this implementation doesn't support windowless mode, which is required for better interoperability with other Evas citizens. I'll be happy to get plugin support added to EFL port if and only if windowless mode is supported.

Because of comment of GyuYoung I have separated patch to smaller parts. This is the first of them, and it is only windowless template(nothing is shown). It is starting point to windowless, and windowed working versions.

Do you need to see running plugin on the screen to positively review this patch? Should I add second patch with windowless implementation to this bug?

(In reply to comment #42) > Do you need to see running plugin on the screen to positively review this > patch? Should I add second patch with windowless implementation to this bug? Please open another bug report with the windowless implementation. I'll informally review both.

Created attachment 107665 [details] EFL's plugin support Rebased.

Comment on attachment 107665 [details] EFL's plugin support View in context: https://bugs.webkit.org/attachment.cgi?id=107665&action=review > Source/WebCore/CMakeListsEfl.txt:169 > + platform/network/soup/ProxyServerSoup.cpp Is this file and the curl equivalent related to this change? I don't see proxy code being used in this patch. > Source/WebCore/ChangeLog:14 > + * plugins/efl/PluginDataEfl.cpp: Added. If files such as this one were copied from other files, the ChangeLog entry should have automatically said it was copied from foo/bar.cpp (see other examples in ChangeLog itself). > Source/WebCore/ChangeLog:15 > + (WebCore::PluginData::initPlugins): It'd be good to add some description to the entries, even it is just "add boilerplate code" or something like that (again, ChangeLog itself can provide some examples). > Source/WebCore/plugins/efl/PluginPackageEfl.cpp:129 > + m_isLoaded = true; Is m_isLoaded really supposed to stay true if the dlsym() calls below fail? > Source/WebCore/plugins/efl/PluginViewEfl.cpp:52 > +#if ENABLE(NETSCAPE_PLUGIN_API) This file is built only if NETSCAPE_PLUGIN_API is enabled, so are these checks needed? > Source/WebCore/plugins/efl/PluginViewEfl.cpp:92 > + IntRect rect = static_cast<FrameView*>(parent())->contentsToScreen(IntRect(0, 0, event->offsetX(), event->offsetY())); Is the cast needed? constentsToScreen() comes from ScrollView. > Source/WebCore/plugins/efl/PluginViewEfl.cpp:93 > + int eventX = rect.width(); can be const. > Source/WebCore/plugins/efl/PluginViewEfl.cpp:94 > + int eventY = rect.height(); can be const. > Source/WebCore/plugins/efl/PluginViewEfl.cpp:127 > + FrameView* frameView = static_cast<FrameView*>(parent()); The only FrameView method being called is contentsToScreen, which comes from ScrollView. Why is the downcast needed? > Source/WebCore/plugins/efl/PluginViewEfl.cpp:129 > + IntRect oldWindowRect = m_windowRect; This variable does not seem to be used. > Source/WebCore/plugins/efl/PluginViewEfl.cpp:130 > + IntRect oldClipRect = m_clipRect; Neither does this one. > Source/WebCore/plugins/efl/PluginViewEfl.cpp:164 > + return; Useless return statement. > Source/WebCore/plugins/efl/PluginViewEfl.cpp:179 > + return; Useless return statement. > Source/WebCore/plugins/efl/PluginViewEfl.cpp:188 > + if (isParentVisible() == visible) Is this check really necessary? > Source/WebCore/plugins/efl/PluginViewEfl.cpp:199 > + if (filename.startsWith("file:///")) Adam's question in comment #23 has not been answered yet: is stripping off the leading / of the path really correct? > Source/WebCore/plugins/efl/PluginViewEfl.cpp:210 > + int bytesRead = fread(buffer.data(), 1, 0, fileHandle); bytesRead can be const. I didn't understand the point of this call. You seem to be reading 0 bytes. > Source/WebCore/plugins/efl/PluginViewEfl.cpp:257 > + *static_cast<void **>(value) = static_cast<Display*>(ecore_x_display_get()); The space before ** is wrong. > Source/WebCore/plugins/efl/PluginViewEfl.cpp:277 > + void** v = (void**)value; "v" is not a good variable name. > Source/WebCore/plugins/efl/PluginViewEfl.cpp:307 > + void* w = reinterpret_cast<void*>(value); "w" is not a good variable name. > Source/WebCore/plugins/efl/PluginViewEfl.cpp:310 > + Ecore_Evas* ee = ecore_evas_ecore_evas_get(evas); "ee" is not a good variable name. > Source/WebCore/plugins/efl/PluginViewEfl.cpp:311 > + *((XID *)w) = (Window) ecore_evas_window_get(ee); C-style casts. > Source/WebCore/plugins/efl/PluginViewEfl.cpp:323 > + // fall through Unnecessary comment. > Source/WebCore/plugins/efl/PluginViewEfl.cpp:343 > + IntRect r(rect->left, rect->top, rect->right - rect->left, rect->bottom - rect->top); Just construct the IntRect when calling invalidateRect().

Comment on attachment 107665 [details] EFL's plugin support View in context: https://bugs.webkit.org/attachment.cgi?id=107665&action=review >> Source/WebCore/CMakeListsEfl.txt:169 >> + platform/network/soup/ProxyServerSoup.cpp > > Is this file and the curl equivalent related to this change? I don't see proxy code being used in this patch. When ENABLE_NETSCAPE_PLUGIN_API is on in PluginView.cpp in function PluginView::getValueForURL(), proxyServersForURL() is called. By default webkit is build with macro off and causes build break for efl port. >> Source/WebCore/plugins/efl/PluginPackageEfl.cpp:129 >> + m_isLoaded = true; > > Is m_isLoaded really supposed to stay true if the dlsym() calls below fail? If one of the dlsym() calls below fail program goes to abort label, which calls unloadWithoutShutdown() which sets m_isLoaded to false. >> Source/WebCore/plugins/efl/PluginViewEfl.cpp:52 >> +#if ENABLE(NETSCAPE_PLUGIN_API) > > This file is built only if NETSCAPE_PLUGIN_API is enabled, so are these checks needed? Yes, it can be ommited

Created attachment 107993 [details] EFL's plugin support

Comment on attachment 107993 [details] EFL's plugin support View in context: https://bugs.webkit.org/attachment.cgi?id=107993&action=review Informal r- for the reason below. > Source/WebCore/plugins/efl/PluginViewEfl.cpp:181 > + if (filename.startsWith("file:///")) > +#if defined(XP_UNIX) > + filename = filename.substring(7); > +#else > + filename = filename.substring(8); > +#endif If you're on Unix, a "file:///bin/bash" URI will have the "file://" stripped, yielding "/bin/bash". This is correct. However, I don't understand why the first '/' is stripped in a non-Unix build, since only "file://" is part of the URI schema. For example: "file://c:/autoexec.bat" will be transformed to ":/autoexec.bat", which is clearly wrong.

Comment on attachment 107993 [details] EFL's plugin support View in context: https://bugs.webkit.org/attachment.cgi?id=107993&action=review > Source/WebCore/CMakeListsEfl.txt:7 > + "${DERIVED_SOURCES_DIR}" I'd rather if this patch just added plugins/efl to the list. > Source/WebCore/plugins/efl/PluginViewEfl.cpp:187 > + FILE* fileHandle = fopen(filename.utf8().data(), "r"); Besides getFileSize, you could also use openFile, readFromFile and the other calls from FileSystem.h. > Source/WebCore/plugins/efl/PluginViewEfl.cpp:340 > +{ Missing notImplemented().

(In reply to comment #48) > (From update of attachment 107993 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=107993&action=review > > Informal r- for the reason below. > > > Source/WebCore/plugins/efl/PluginViewEfl.cpp:181 > > + if (filename.startsWith("file:///")) > > +#if defined(XP_UNIX) > > + filename = filename.substring(7); > > +#else > > + filename = filename.substring(8); > > +#endif > > If you're on Unix, a "file:///bin/bash" URI will have the "file://" stripped, yielding "/bin/bash". This is correct. However, I don't understand why the first '/' is stripped in a non-Unix build, since only "file://" is part of the URI schema. For example: "file://c:/autoexec.bat" will be transformed to ":/autoexec.bat", which is clearly wrong. At first the if statement says that this is only in case that filename starts with "file:///", so there is no problem with example: "file://c:/autoexec.bat". Second: as I know proper uri schema for local file is "file:///", for other combinations like: "file://something" it will try to resolve name "something" f.e. file://localhost/.... For windows proper schema for local file will be f.e. "file:///C:/autoexec.bat". Same stripping is made in Mac, Qt and Win ports, however I think it should be fixed at least in Qt.

(In reply to comment #49) > (From update of attachment 107993 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=107993&action=review > > > Source/WebCore/CMakeListsEfl.txt:7 > > + "${DERIVED_SOURCES_DIR}" > > I'd rather if this patch just added plugins/efl to the list. > > > Source/WebCore/plugins/efl/PluginViewEfl.cpp:187 > > + FILE* fileHandle = fopen(filename.utf8().data(), "r"); > > Besides getFileSize, you could also use openFile, readFromFile and the other calls from FileSystem.h. Thought about this, but left as is because none of the ports is using FileSystem methods for some reason, and FILE methods are used in Mac, and Qt > > > Source/WebCore/plugins/efl/PluginViewEfl.cpp:340 > > +{ > > Missing notImplemented().

(In reply to comment #50) > At first the if statement says that this is only in case that filename starts > with "file:///", so there is no problem with example: "file://c:/autoexec.bat". You're right. I'm changing the r- to r? again.

Comment on attachment 107993 [details] EFL's plugin support Informal r+. Setting cq- so that this doesn't get committed before other parts of this patch gets properly reviewed.

This patch works with windowless implementation. For details see: https://bugs.webkit.org/show_bug.cgi?id=70592

Created attachment 114951 [details] EFL's plugin support Rebased version.

Comment on attachment 114951 [details] EFL's plugin support View in context: https://bugs.webkit.org/attachment.cgi?id=114951&action=review > Source/WebCore/plugins/efl/PluginViewEfl.cpp:209 > + Unneeded empty line. > Source/WebCore/plugins/efl/PluginViewEfl.cpp:214 > + ditto. > Source/WebCore/plugins/efl/PluginViewEfl.cpp:219 > + ditto. > Source/WebCore/plugins/efl/PluginViewEfl.cpp:224 > + ditto. > Source/WebCore/plugins/efl/PluginViewEfl.cpp:240 > + ditto. > Source/WebCore/plugins/efl/PluginViewEfl.cpp:244 > + ditto. > Source/WebCore/plugins/efl/PluginViewEfl.cpp:262 > + ditto. > Source/WebCore/plugins/efl/PluginViewEfl.cpp:278 > + *static_cast<void**>(value) = pluginScriptObject; Is *static_cast correct usage ? I can't find similar usage in WebKit. > Source/WebCore/plugins/efl/PluginViewEfl.cpp:283 > + ditto. > Source/WebCore/plugins/efl/PluginViewEfl.cpp:292 > + ditto.

Comment on attachment 114951 [details] EFL's plugin support View in context: https://bugs.webkit.org/attachment.cgi?id=114951&action=review >> Source/WebCore/plugins/efl/PluginViewEfl.cpp:209 > > Unneeded empty line. Same style applied in PluginViewGtk and PluginView. >> Source/WebCore/plugins/efl/PluginViewEfl.cpp:278 >> + *static_cast<void**>(value) = pluginScriptObject; > > Is *static_cast correct usage ? I can't find similar usage in WebKit. It's better to use static_cast in C++ than C-style cast. For similar usage see f.e. PluginViewMac: *static_cast<NPBool*>(value) = false;

Any news for it?

Added people connected with gtk plugin's port since implementation is based on it.

Comment on attachment 114951 [details] EFL's plugin support View in context: https://bugs.webkit.org/attachment.cgi?id=114951&action=review > Source/WebKit/efl/ewk/ewk_frame.cpp:1639 > + return pluginView; It's slightly more efficient to use pluginView.release() here.

Comment on attachment 114951 [details] EFL's plugin support Marking cq+ since the author is not a comitter. The extra ref probably shouldn't hold up this patch.

Comment on attachment 114951 [details] EFL's plugin support Rejecting attachment 114951 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2 Last 500 characters of output: hromium/testing/gmock --revision 374 --non-interactive --force --accept theirs-conflict --ignore-externals returned non-zero exit status 1 in /mnt/git/webkit-commit-queue/Source/WebKit/chromium Error: 'depot_tools/gclient sync --force --reset --delete_unversioned_trees' failed 3 tries and returned 256 at Tools/Scripts/update-webkit-chromium line 106. Re-trying 'depot_tools/gclient sync --force --reset --delete_unversioned_trees' No such file or directory at Tools/Scripts/update-webkit line 112. Full output: http://queues.webkit.org/results/10986247

Mariusz, this patch get a r+ at last. :-) Please rebase this patch based on latest WebKit. And, add "Reviewed by Anders Carlsson" instead of "Reviewed by NOBODY (OOPS!)." in ChangeLog. Then, please only request cq?.

Created attachment 120300 [details] EFL's plugin support

Rebased.

Created attachment 120301 [details] EFL's plugin support

Patch for landing.

Comment on attachment 120301 [details] EFL's plugin support Rejecting attachment 120301 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1 Anders Calsson found in /mnt/git/webkit-commit-queue/Source/WebKit/efl/ChangeLog does not appear to be a valid reviewer according to committers.py. ERROR: /mnt/git/webkit-commit-queue/Source/WebKit/efl/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive). Full output: http://queues.webkit.org/results/10981493

Comment on attachment 120301 [details] EFL's plugin support View in context: https://bugs.webkit.org/attachment.cgi?id=120301&action=review > Source/WebKit/efl/ChangeLog:6 > + Reviewed by Anders Calsson. %s/Calsson/Carlsson/g

Created attachment 120306 [details] EFL's plugin support

Comment on attachment 120306 [details] EFL's plugin support Clearing flags on attachment: 120306 Committed r103544: <http://trac.webkit.org/changeset/103544>

All reviewed patches have been landed. Closing bug.