[webkit-reviews] review denied: [Bug 44505] [EFL] Missing plugins support for efl port : [Attachment 88807] EFL's plugin support

[bugzilla-daemon at webkit.org]

Adam Roben (:aroben) <aroben at apple.com> has denied Mariusz Grzegorczyk
<mariusz.g at samsung.com>'s request for review:
Bug 44505: [EFL] Missing plugins support for efl port
https://bugs.webkit.org/show_bug.cgi?id=44505

Attachment 88807: EFL's plugin support
https://bugs.webkit.org/attachment.cgi?id=88807&action=review

------- Additional Comments from Adam Roben (:aroben) <aroben at apple.com>
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.