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

[bugzilla-daemon at webkit.org]

Leandro Pereira <leandro at profusion.mobi> 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 105367: EFL's plugin support
https://bugs.webkit.org/attachment.cgi?id=105367&action=review

------- Additional Comments from Leandro Pereira <leandro at profusion.mobi>
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, ...));