[EFL] Missing plugins support for efl port

[Mariusz Grzegorczyk]

Plugin support in efl port is missing, and function that creates plugins returns NULL.

[Mariusz Grzegorczyk]

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

[WebKit Review Bot]

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.

[WebKit Review Bot]

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

[Mariusz Grzegorczyk]

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

[Leandro Pereira]

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

[Kenneth Rohde Christiansen]

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

r- due to above comments

[Gyuyoung Kim]

Any news on this?

[Mariusz Grzegorczyk]

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

[WebKit Review Bot]

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.

[Mariusz Grzegorczyk]

Created attachment 81214 [details]
EFL's plugin support

[Mariusz Grzegorczyk]

Created attachment 81215 [details]
EFL's plugin support

[Mariusz Grzegorczyk]

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.

[Gyuyoung Kim]

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

[Mariusz Grzegorczyk]

Created attachment 85669 [details]
EFL's plugin support

[Adam Barth]

Comment on attachment 85669 [details]
EFL's plugin support

This patch is huge!

[Gyuyoung Kim]

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

[Gyuyoung Kim]

Please make patches on latest WebKit.

[Leandro Pereira]

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

[Mariusz Grzegorczyk]

Created attachment 88807 [details]
EFL's plugin support

[Mariusz Grzegorczyk]

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

[Gyuyoung Kim]

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.

[Gyuyoung Kim]

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

[Adam Roben (:aroben)]

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.

[Mariusz Grzegorczyk]

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

[Mariusz Grzegorczyk]

Created attachment 91654 [details]
EFL's plugin support

[Mariusz Grzegorczyk]

Created attachment 96734 [details]
EFL's plugin support

Version synchronized with newest webkit's code.

[Gyuyoung Kim]

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

[Mariusz Grzegorczyk]

Created attachment 96740 [details]
EFL's plugin support

[Antonio Gomes]

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?

[Mariusz Grzegorczyk]

Created attachment 97117 [details]
EFL's plugin support

[Leandro Pereira]

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.

[Mariusz Grzegorczyk]

Created attachment 102263 [details]
EFL's plugin support

[Ryuan Choi]

(In reply to comment #32)
> Created an attachment (id=102263) [details]
> EFL's plugin support

Could you rebase patch or make newer?

[Mariusz Grzegorczyk]

Created attachment 105018 [details]
EFL's plugin support

Rebased.

[Gyuyoung Kim]

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.

[Mariusz Grzegorczyk]

Created attachment 105367 [details]
EFL's plugin support

Fixed and rebased. This patch is already splitted.

[Leandro Pereira]

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, ...));

[Mariusz Grzegorczyk]

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)

[Mariusz Grzegorczyk]

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)

[Leandro Pereira]

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.

[Mariusz Grzegorczyk]

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.

[Mariusz Grzegorczyk]

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?

[Leandro Pereira]

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

[Mariusz Grzegorczyk]

Created attachment 107665 [details]
EFL's plugin support

Rebased.

[Raphael Kubo da Costa (:rakuco)]

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

[Mariusz Grzegorczyk]

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

[Mariusz Grzegorczyk]

Created attachment 107993 [details]
EFL's plugin support

[Leandro Pereira]

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.

[Raphael Kubo da Costa (:rakuco)]

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

[Mariusz Grzegorczyk]

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

[Mariusz Grzegorczyk]

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

[Leandro Pereira]

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

[Leandro Pereira]

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.

[Mariusz Grzegorczyk]

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

[Mariusz Grzegorczyk]

Created attachment 114951 [details]
EFL's plugin support

Rebased version.

[Gyuyoung Kim]

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.

[Mariusz Grzegorczyk]

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;

[Mariusz Grzegorczyk]

Any news for it?

[Mariusz Grzegorczyk]

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

[Anders Carlsson]

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.

[Eric Seidel (no email)]

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.

[WebKit Review Bot]

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

[Gyuyoung Kim]

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

[Mariusz Grzegorczyk]

Created attachment 120300 [details]
EFL's plugin support

[Mariusz Grzegorczyk]

Rebased.

[Mariusz Grzegorczyk]

Created attachment 120301 [details]
EFL's plugin support

[Mariusz Grzegorczyk]

Patch for landing.

[WebKit Review Bot]

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

[Gyuyoung Kim]

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

[Mariusz Grzegorczyk]

Created attachment 120306 [details]
EFL's plugin support

[WebKit Review Bot]

Comment on attachment 120306 [details]
EFL's plugin support

Clearing flags on attachment: 120306

Committed r103544: <http://trac.webkit.org/changeset/103544>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.