[Webkit-unassigned] [Bug 44505] [EFL] Missing plugins support for efl port

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=44505


Leandro Pereira <leandro at profusion.mobi> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #105367|review?, commit-queue?      |review-
               Flag|                            |




--- Comment #37 from Leandro Pereira <leandro at profusion.mobi>  2011-08-26 10:36:54 PST ---
(From update of attachment 105367)
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, ...));

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.