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

[bugzilla-daemon at webkit.org]

Raphael Kubo da Costa <kubo 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 107665: EFL's plugin support
https://bugs.webkit.org/attachment.cgi?id=107665&action=review

------- Additional Comments from Raphael Kubo da Costa <kubo at profusion.mobi>
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().