Bug 70592 – [EFL][GTK] Share plugin's implementation between EFL and Gtk ports.

• Component: Plug-ins
• Status: RESOLVED FIXED

[Mariusz Grzegorczyk]

Windowless plugins implementation for EFL port(based on GTK). Tested with flashplugin-nonfree package(gtk) after invoking gtk_init in EWebLauncher.

[Mariusz Grzegorczyk]

Created attachment 111952 [details]
EFL windowless plugins implementation

[Leandro Pereira]

Comment on attachment 111952 [details]
EFL windowless plugins implementation

View in context: https://bugs.webkit.org/attachment.cgi?id=111952&action=review

> Source/WebCore/plugins/efl/PluginViewEfl.cpp:51
> +#include <PlatformContextCairo.h>
> +#include <X11/Xlib.h>
> +#include <cairo/cairo-xlib.h>

Be aware that even though Ecore_X is present during compilation, it might not be during runtime. For instance, DumpRenderTree won't be able to run plugin tests because it uses Ecore_Evas_Buffer (which performs all operations in memory, without a connection to a X server). It would be nice if either the plugin subsystem were disabled if it's not running under X, or provide an implementation independent of the graphical backend.

> Source/WebCore/plugins/efl/PluginViewEfl.cpp:60
> +    // sanity check

Unneeded comment.

> Source/WebCore/plugins/efl/PluginViewEfl.cpp:216
> +    const bool syncX = m_pluginDisplay && m_pluginDisplay != display;

Please precede ``syncX'' by ``should'': shouldSyncWithX.

> Source/WebCore/plugins/efl/PluginViewEfl.cpp:233
> +        RefPtr<cairo_t> cr = adoptRef(cairo_create(drawableSurface.get()));

``cr'' doesn't look like a good variable name.

> Source/WebCore/plugins/efl/PluginViewEfl.cpp:261
> +    exposeEvent.width = exposedRect.x() + exposedRect.width(); // flash bug? it thinks width is the right in transparent mode
> +    exposeEvent.height = exposedRect.y() + exposedRect.height(); // flash bug? it thinks height is the bottom in transparent mode

Please use proper sentences in comments.

[Mariusz Grzegorczyk]

Created attachment 115161 [details]
EFL windowless plugins implementation

Rebased, and applied Leonardo's comments.

[Mariusz Grzegorczyk]

(In reply to comment #2)
> (From update of attachment 111952 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=111952&action=review
> 
> > Source/WebCore/plugins/efl/PluginViewEfl.cpp:51
> > +#include <PlatformContextCairo.h>
> > +#include <X11/Xlib.h>
> > +#include <cairo/cairo-xlib.h>
> 
> Be aware that even though Ecore_X is present during compilation, it might not be during runtime. For instance, DumpRenderTree won't be able to run plugin tests because it uses Ecore_Evas_Buffer (which performs all operations in memory, without a connection to a X server). It would be nice if either the plugin subsystem were disabled if it's not running under X, or provide an implementation independent of the graphical backend.

I've added ecore_x_init to DumpRendererTree. Webkit library is linked with ecore_x for efl port.

[Gyuyoung Kim]

Comment on attachment 115161 [details]
EFL windowless plugins implementation

View in context: https://bugs.webkit.org/attachment.cgi?id=115161&action=review

You need to rebase this patch again.

> Source/WebCore/ChangeLog:3
> +        Bug 70592 - [EFL] Windowless plugins implementation

Bug 70592 is unneeded.

> Source/WebCore/plugins/efl/PluginViewEfl.cpp:522
> +    // TODO: optimize

Do not use TODO. According to webkit conding style guide line, we should use "FIXME". It looks "optimize" is too insufficient description.

[Mariusz Grzegorczyk]

Created attachment 120877 [details]
EFL windowless plugins implementation

[Mariusz Grzegorczyk]

Created attachment 128482 [details]
EFL windowless plugins implementation

Rebased version.

[Mariusz Grzegorczyk]

Created attachment 128485 [details]
Source of sample windowless plugin based on XPixmap

[Mariusz Grzegorczyk]

I've tested newest patch with created sample plugin: just drawing rectangle in place of plugin. Source is attached as TestPlugin.tar. After compiling TestPlugin.cpp output library should be moved to one of the plugins location e.g.: /usr/lib/mozilla/plugins/.

[Mariusz Grzegorczyk]

(In reply to comment #9)
> I've tested newest patch with created sample plugin: just drawing rectangle in place of plugin. Source is attached as TestPlugin.tar. After compiling TestPlugin.cpp output library should be moved to one of the plugins location e.g.: /usr/lib/mozilla/plugins/.
Plugin has "application/plugtest" mime type.

[Martin Robinson]

I think it's really time to consider adding a shared X11 plugin implementation for WebKit1, rather than adding yet another X11 plugin implementation with a tiny bit of glue code not shared in common. In WebKit2 we are able to share the implementation.

[Gyuyoung Kim]

Any update ?

[Gyuyoung Kim]

Comment on attachment 128482 [details]
EFL windowless plugins implementation



Cleared review? from attachment 128482 [details] so that this bug does not appear in http://webkit.org/pending-review.  If you would like this patch reviewed, please attach it to a new bug (or re-open this bug before marking it for review again).

[Mariusz Grzegorczyk]

Created attachment 190506 [details]
patch

[WebKit Review Bot]

Attachment 190506 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/GNUmakefile.am', u'Source/WebCore/GNUmakefile.list.am', u'Source/WebCore/PlatformEfl.cmake', u'Source/WebCore/plugins/efl/PluginViewEfl.cpp', u'Source/WebCore/plugins/gtk/PluginViewGtk.cpp', u'Source/WebCore/plugins/x11/PluginViewX11.cpp', u'Tools/ChangeLog', u'Tools/DumpRenderTree/efl/CMakeLists.txt', u'Tools/DumpRenderTree/efl/DumpRenderTree.cpp']" exit_code: 1
Source/WebCore/plugins/x11/PluginViewX11.cpp:257:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Source/WebCore/plugins/x11/PluginViewX11.cpp:283:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Source/WebCore/plugins/x11/PluginViewX11.cpp:860:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 3 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Mariusz Grzegorczyk]

Created attachment 190511 [details]
updated patch

[Philippe Normand]

Comment on attachment 190511 [details]
updated patch

View in context: https://bugs.webkit.org/attachment.cgi?id=190511&action=review

> Source/WebCore/plugins/x11/PluginViewX11.cpp:78
> +#else

#elif PLATFORM(EFL)

> Source/WebCore/plugins/x11/PluginViewX11.cpp:145
> +#else

Ditto

> Source/WebCore/plugins/x11/PluginViewX11.cpp:680
> +#if PLATFORM(GTK)
> +        *static_cast<uint32_t*>(value) = 2;
> +#else
> +        *static_cast<uint32_t*>(value) = 0;
> +#endif

I don't understand this, I guess it's inherited from the previous PluginView implementation but a comment would be good to have here. Also, mind turning the #else to and #elif ?

> Source/WebCore/plugins/x11/PluginViewX11.cpp:687
> +#else

#elif PLATFORM(EFL)

> Source/WebCore/plugins/x11/PluginViewX11.cpp:994
> +#else

Ditto

[Gyuyoung Kim]

Comment on attachment 190511 [details]
updated patch

View in context: https://bugs.webkit.org/attachment.cgi?id=190511&action=review

> Tools/DumpRenderTree/efl/DumpRenderTree.cpp:385
> +        ewk_shutdown();

Shouldn't ewk_shutdown() is called first ?

[Mariusz Grzegorczyk]

Created attachment 198129 [details]
updated, rebased, applied Philippe's and Gyuyoung's comments

[Gyuyoung Kim]

Comment on attachment 198129 [details]
updated, rebased, applied Philippe's and Gyuyoung's comments

View in context: https://bugs.webkit.org/attachment.cgi?id=198129&action=review

> Tools/DumpRenderTree/efl/DumpRenderTree.cpp:374
> +    if (!ecore_x_init(0)) {

Missing #ifdef HAVE_ECORE_X. BTW, is it better to move ecore_x_init(0) to ecore initializations ? I mean it would be good if you place this before edje_init().

[Mariusz Grzegorczyk]

Created attachment 198332 [details]
applied comments, remove ecore_x initialization from DumpRenderTree because it is done in ewk_init

[Ryuan Choi]

(In reply to comment #21)
> Created an attachment (id=198332) [details]
> applied comments, remove ecore_x initialization from DumpRenderTree because it is done in ewk_init

Is this still valid?

At least, we need to rebase this.

[Ryuan Choi]

Created attachment 220823 [details]
rebased

[Sergio Villar Senin]

Comment on attachment 220823 [details]
rebased

View in context: https://bugs.webkit.org/attachment.cgi?id=220823&action=review

I added some nits.

Said that, I am not entirely sure that this change really pays off. I thought the refactoring could be a good idea, but the outcome doesn't look better than two separate files because in exchange of moving the implementation to a single file, we get a file full of #ifdefs which is difficult to read is several parts.

> Source/WebCore/plugins/x11/PluginViewX11.cpp:89
> +#define Status int // ditto

I know they come from the removed files, but what do we need these two defines for? Looks like they're totally unused

> Source/WebCore/plugins/x11/PluginViewX11.cpp:106
> +    // sanity check

This comment looks useless

> Source/WebCore/plugins/x11/PluginViewX11.cpp:141
> +    // later.

Put this inside the GTK ifdef as it is not valid for EFL

> Source/WebCore/plugins/x11/PluginViewX11.cpp:236
> +        m_windowRect.height()));

You can write this in a single line

> Source/WebCore/plugins/x11/PluginViewX11.cpp:309
> +    xEvent.type = (event->type() == eventNames().keydownEvent) ? 2 : 3; // KeyPress/Release get unset somewhere

That comment looks suspicious. Time to research a bit why we are using those magic numbers?

> Source/WebCore/plugins/x11/PluginViewX11.cpp:478
> +    event.type = 9; // FocusIn gets unset somewhere

Again another magic number

> Source/WebCore/plugins/x11/PluginViewX11.cpp:494
> +    event.type = 10; // FocusOut gets unset somewhere

Ditto.

> Source/WebCore/plugins/x11/PluginViewX11.cpp:604
> +    // Don't do anything if the allocation has not changed.

This comment doesn't add up, better to remove it.

> Source/WebCore/plugins/x11/PluginViewX11.cpp:610
> +    // Don't do anything if both the old and the new allocations are outside the frame.

Ditto. The code is self-explanatory

[Ryuan Choi]

(In reply to comment #24)
> (From update of attachment 220823 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=220823&action=review
> 
> I added some nits.
> 
> Said that, I am not entirely sure that this change really pays off. I thought the refactoring could be a good idea, but the outcome doesn't look better than two separate files because in exchange of moving the implementation to a single file, we get a file full of #ifdefs which is difficult to read is several parts.
> 

First, thank you for your quick feedback.
I had similar concerns while rebasing this patch.

I can refactor more but I think that we still have some differences.
So, how about just resolving this invalid and fixing only EFL side?

[Sergio Villar Senin]

(In reply to comment #25)
> (In reply to comment #24)
> > (From update of attachment 220823 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=220823&action=review
> > 
> > I added some nits.
> > 
> > Said that, I am not entirely sure that this change really pays off. I thought the refactoring could be a good idea, but the outcome doesn't look better than two separate files because in exchange of moving the implementation to a single file, we get a file full of #ifdefs which is difficult to read is several parts.
> > 
> 
> First, thank you for your quick feedback.
> I had similar concerns while rebasing this patch.
> 
> I can refactor more but I think that we still have some differences.
> So, how about just resolving this invalid and fixing only EFL side?

Yeah I think the best thing you could do is to fill the gaps in EFL

[Carlos Garcia Campos]

I'm not sure I like this idea of sharing the implementation to add a "common" file with a lot of #ifdefs. Would it be possible to add a PluginViewX11 file with the implementation of the methods that are actually common, and leave PluginViewGtk and PluginViewEfl with the platform specific methods?

[Ryuan Choi]

Created attachment 221109 [details]
Patch

[Ryuan Choi]

Created attachment 221111 [details]
Patch

[Ryuan Choi]

(In reply to comment #29)
> Created an attachment (id=221111) [details]
> Patch

I tried to separate the common logic from the GTK's code like KaL said.

Could you let me know what should I do for the next step?
Should I cook it more?

[Carlos Garcia Campos]

Comment on attachment 221111 [details]
Patch

Thanks for the updated patch, the GTK+ part looks good to me, it simply moves the code but someone from EFL should review that part.

[Gyuyoung Kim]

Comment on attachment 221111 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=221111&action=review

I believe this is a way to go between efl and gtk. r+ed by me on behalf of gtk reviewer. Please fix trivial nits before landing.

> Source/WebCore/plugins/efl/PluginViewEfl.cpp:309
> +    // TODO remove in favor of null events, like mac port?

TODO -> FIXME: ?

> Source/WebCore/plugins/x11/PluginViewX11.cpp:135
> +    // TODO: optimize

ditto.

[Ryuan Choi]

Committed r163505: <http://trac.webkit.org/changeset/163505>

[Ryuan Choi]

Comment on attachment 221111 [details]
Patch

clearing flags after landed