Bug 70592

Created attachment 111952 [details] EFL windowless plugins implementation

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.

Created attachment 115161 [details] EFL windowless plugins implementation Rebased, and applied Leonardo's comments.

(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 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.

Created attachment 120877 [details] EFL windowless plugins implementation

Created attachment 128482 [details] EFL windowless plugins implementation Rebased version.

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

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/.

(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.

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.

Any update ?

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).

Created attachment 190506 [details] patch

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.

Created attachment 190511 [details] updated patch

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 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 ?

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

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().

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

(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.

Created attachment 220823 [details] rebased

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

(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?

(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

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?

Created attachment 221109 [details] Patch

Created attachment 221111 [details] Patch

(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 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 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.

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

Comment on attachment 221111 [details] Patch clearing flags after landed