[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


Raphael Kubo da Costa <kubo at profusion.mobi> changed:

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




--- Comment #45 from Raphael Kubo da Costa <kubo at profusion.mobi>  2011-09-16 11:24:15 PST ---
(From update of attachment 107665)
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().

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