Bug 70592

Summary: [EFL][GTK] Share plugin's implementation between EFL and Gtk ports.
Product: WebKit Reporter: Mariusz Grzegorczyk <mariusz.g>
Component: Plug-insAssignee: Ryuan Choi <ryuan.choi>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, bunhere, cgarcia, commit-queue, gyuyoung.kim, gyuyoung.kim, japhet, leandro, mrobinson, philn, rakuco, ryuan.choi, svillar, webkit.review.bot, xan.lopez, zan
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Linux   
Bug Depends on: 44505    
Bug Blocks: 96881    
Attachments:
Description Flags
EFL windowless plugins implementation
leandro: review-
EFL windowless plugins implementation
none
EFL windowless plugins implementation
none
EFL windowless plugins implementation
none
Source of sample windowless plugin based on XPixmap
none
patch
none
updated patch
pnormand: review-, pnormand: commit-queue-
updated, rebased, applied Philippe's and Gyuyoung's comments
none
applied comments, remove ecore_x initialization from DumpRenderTree because it is done in ewk_init
none
rebased
none
Patch
none
Patch none

Description Mariusz Grzegorczyk 2011-10-21 03:04:47 PDT
Windowless plugins implementation for EFL port(based on GTK). Tested with flashplugin-nonfree package(gtk) after invoking gtk_init in EWebLauncher.
Comment 1 Mariusz Grzegorczyk 2011-10-21 05:28:11 PDT
Created attachment 111952 [details]
EFL windowless plugins implementation
Comment 2 Leandro Pereira 2011-10-21 10:57:57 PDT
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.
Comment 3 Mariusz Grzegorczyk 2011-11-15 07:09:43 PST
Created attachment 115161 [details]
EFL windowless plugins implementation

Rebased, and applied Leonardo's comments.
Comment 4 Mariusz Grzegorczyk 2011-11-15 07:12:41 PST
(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.
Comment 5 Gyuyoung Kim 2011-12-27 00:20:25 PST
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.
Comment 6 Mariusz Grzegorczyk 2012-01-02 06:17:57 PST
Created attachment 120877 [details]
EFL windowless plugins implementation
Comment 7 Mariusz Grzegorczyk 2012-02-23 08:39:45 PST
Created attachment 128482 [details]
EFL windowless plugins implementation

Rebased version.
Comment 8 Mariusz Grzegorczyk 2012-02-23 08:55:36 PST
Created attachment 128485 [details]
Source of sample windowless plugin based on XPixmap
Comment 9 Mariusz Grzegorczyk 2012-02-23 09:07:13 PST
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/.
Comment 10 Mariusz Grzegorczyk 2012-02-23 09:08:35 PST
(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.
Comment 11 Martin Robinson 2012-05-02 08:21:53 PDT
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.
Comment 12 Gyuyoung Kim 2012-10-07 20:57:28 PDT
Any update ?
Comment 13 Gyuyoung Kim 2012-11-23 22:27:43 PST
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).
Comment 14 Mariusz Grzegorczyk 2013-02-27 06:21:42 PST
Created attachment 190506 [details]
patch
Comment 15 WebKit Review Bot 2013-02-27 06:24:33 PST
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.
Comment 16 Mariusz Grzegorczyk 2013-02-27 06:43:27 PST
Created attachment 190511 [details]
updated patch
Comment 17 Philippe Normand 2013-04-12 12:45:31 PDT
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
Comment 18 Gyuyoung Kim 2013-04-13 19:03:58 PDT
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 ?
Comment 19 Mariusz Grzegorczyk 2013-04-15 09:02:17 PDT
Created attachment 198129 [details]
updated, rebased, applied Philippe's and Gyuyoung's comments
Comment 20 Gyuyoung Kim 2013-04-16 05:06:40 PDT
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().
Comment 21 Mariusz Grzegorczyk 2013-04-16 08:43:07 PDT
Created attachment 198332 [details]
applied comments, remove ecore_x initialization from DumpRenderTree because it is done in ewk_init
Comment 22 Ryuan Choi 2013-12-05 01:52:19 PST
(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.
Comment 23 Ryuan Choi 2014-01-10 01:54:22 PST
Created attachment 220823 [details]
rebased
Comment 24 Sergio Villar Senin 2014-01-10 07:00:00 PST
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
Comment 25 Ryuan Choi 2014-01-10 20:01:34 PST
(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?
Comment 26 Sergio Villar Senin 2014-01-12 23:47:17 PST
(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
Comment 27 Carlos Garcia Campos 2014-01-13 00:00:26 PST
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?
Comment 28 Ryuan Choi 2014-01-13 23:47:01 PST
Created attachment 221109 [details]
Patch
Comment 29 Ryuan Choi 2014-01-14 00:02:45 PST
Created attachment 221111 [details]
Patch
Comment 30 Ryuan Choi 2014-01-17 21:48:09 PST
(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?
Comment 31 Carlos Garcia Campos 2014-01-21 00:40:29 PST
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.
Comment 32 Gyuyoung Kim 2014-02-05 17:58:09 PST
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.
Comment 33 Ryuan Choi 2014-02-05 18:25:27 PST
Committed r163505: <http://trac.webkit.org/changeset/163505>
Comment 34 Ryuan Choi 2014-02-05 19:30:12 PST
Comment on attachment 221111 [details]
Patch

clearing flags after landed