Bug 91844

Created attachment 153463 [details] Patch implementing windowless plugin and plugin process for webkit2 efl's port

Attachment 153463 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'Source/WebKit2/ChangeLog', u..." exit_code: 1 Source/WebKit2/UIProcess/Plugins/efl/PluginProcessProxyEfl.cpp:34: Alphabetical sorting problem. [build/include_order] [4] Source/WebKit2/WebProcess/Plugins/Netscape/x11/NetscapePluginX11.cpp:52: "PlatformContextCairo.h" already included at Source/WebKit2/WebProcess/Plugins/Netscape/x11/NetscapePluginX11.cpp:40 [build/include] [4] Source/WebKit2/WebProcess/Plugins/Netscape/x11/NetscapePluginX11.cpp:53: "RefPtrCairo.h" already included at Source/WebKit2/WebProcess/Plugins/Netscape/x11/NetscapePluginX11.cpp:41 [build/include] [4] Source/WebKit2/WebProcess/Plugins/Netscape/x11/NetscapePluginX11.cpp:54: "cairo/cairo-xlib.h" already included at Source/WebKit2/WebProcess/Plugins/Netscape/x11/NetscapePluginX11.cpp:42 [build/include] [4] Total errors found: 4 in 16 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 153463 [details] Patch implementing windowless plugin and plugin process for webkit2 efl's port Attachment 153463 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/13312188

(In reply to comment #3) > (From update of attachment 153463 [details]) > Attachment 153463 [details] did not pass efl-ews (efl): > Output: http://queues.webkit.org/results/13312188 I think that these errors are false positives. Situation looks like: #if port(X) #include A,B,C #elsif port(Y) #include A,B check-webkit-style reports duplication of includes in different excluding macros. What do you think about it?

(In reply to comment #4) > (In reply to comment #3) > > (From update of attachment 153463 [details] [details]) > > Attachment 153463 [details] [details] did not pass efl-ews (efl): > > Output: http://queues.webkit.org/results/13312188 > > I think that these errors are false positives. Situation looks like: > > #if port(X) > #include A,B,C > #elsif port(Y) > #include A,B > > check-webkit-style reports duplication of includes in different excluding macros. What do you think about it? You should probably use something like #if PORT(X) || PORT(Y) #include A #include B #endif if PORT(X) #include C #endif

Created attachment 153525 [details] Fixed webkit style

Comment on attachment 153525 [details] Fixed webkit style Attachment 153525 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/13303396

Comment on attachment 153525 [details] Fixed webkit style Attachment 153525 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/13308330

How about change PluginProcess_NAME to PluginProcess_EXECUTABLE_NAME? I will add the definition of PluginProcess/WebProcess executable name to pass the name of process to the ProcessExecutablePathEfl. CMakeList.txt:535 SET(WebProcess_EXECUTABLE_NAME WebProcess)

Comment on attachment 153525 [details] Fixed webkit style View in context: https://bugs.webkit.org/attachment.cgi?id=153525&action=review Could you check how many test cases can be covered by this patch ? > Source/WebKit2/efl/PluginMainEfl.cpp:4 > + * Copyright (C) 2012 Samsung Electronics Missing 'All rights reserved." > Tools/Scripts/webkitperl/FeatureList.pm:306 > + define => "ENABLE_NETSCAPE_PLUGIN_API", default => 1, value => \$netscapePluginAPISupport }, Can we enable this feature for all ports? I don't think yet. So, I think we have to use isEfl() instead of 1.

Comment on attachment 153525 [details] Fixed webkit style View in context: https://bugs.webkit.org/attachment.cgi?id=153525&action=review >> Tools/Scripts/webkitperl/FeatureList.pm:306 >> + define => "ENABLE_NETSCAPE_PLUGIN_API", default => 1, value => \$netscapePluginAPISupport }, > > Can we enable this feature for all ports? I don't think yet. So, I think we have to use isEfl() instead of 1. Given that the previous value was "!isEfl()", I think using "1" now is correct. The EFL port was the only one not supporting this feature.

Could you update patch for green bot ?

Comment on attachment 153525 [details] Fixed webkit style View in context: https://bugs.webkit.org/attachment.cgi?id=153525&action=review >>> Tools/Scripts/webkitperl/FeatureList.pm:306 >>> + define => "ENABLE_NETSCAPE_PLUGIN_API", default => 1, value => \$netscapePluginAPISupport }, >> >> Can we enable this feature for all ports? I don't think yet. So, I think we have to use isEfl() instead of 1. > > Given that the previous value was "!isEfl()", I think using "1" now is correct. The EFL port was the only one not supporting this feature. Yes, right. There was misunderstanding. Thanks.

Created attachment 159885 [details] Rebased, and applied KwangYong Choi's comments

Comment on attachment 159885 [details] Rebased, and applied KwangYong Choi's comments View in context: https://bugs.webkit.org/attachment.cgi?id=159885&action=review > Source/WebKit2/PlatformEfl.cmake:92 > + UIProcess/Plugins/efl/PluginProcessProxyEfl.cpp It would be good if you add a new line. Because cmake prefers to distinguish different path by a new line. > Source/WebKit2/PlatformEfl.cmake:106 > + WebProcess/Plugins/Netscape/efl/PluginProxyEfl.cpp ditto. > Source/WebKit2/PlatformEfl.cmake:236 > + "${WEBKIT2_DIR}/PluginProcess/efl" Generally, specific include path is placed above path defined by macro as below, "${WEBKIT2_DIR}/PluginProcess/efl" ${WEBKIT2_DIR} > Source/WebKit2/PluginProcess/efl/PluginControllerProxyEfl.cpp:31 > +#if ENABLE(PLUGIN_PROCESS) Please add a new line. > Source/WebKit2/PluginProcess/efl/PluginControllerProxyEfl.cpp:33 > + This is unneeded line. > Source/WebKit2/PluginProcess/efl/PluginProcessMainEfl.cpp:30 > + Don't you need to use ENABLE(PLUGIN_PROCESS) ? > Source/WebKit2/PluginProcess/efl/PluginProcessMainEfl.cpp:80 > + This is unneeded line. > Source/WebKit2/UIProcess/Plugins/efl/PluginProcessProxyEfl.cpp:61 > + If you need to use glib function, I think you need to use #if ENABLE(GLIB_SUPPORT) macro.

Comment on attachment 159885 [details] Rebased, and applied KwangYong Choi's comments View in context: https://bugs.webkit.org/attachment.cgi?id=159885&action=review Are you sure you need all those entities hold copyright over all those files? Some files being added have 3 or 4 lines of code at most. > ChangeLog:8 > + Turn on Netscape Plugin API by default. So plugins are working fine on both WK1 and WK2? >> Source/WebKit2/UIProcess/Plugins/efl/PluginProcessProxyEfl.cpp:61 >> + > > If you need to use glib function, I think you need to use #if ENABLE(GLIB_SUPPORT) macro. Alternatively, you can just use ecore.

Comment on attachment 159885 [details] Rebased, and applied KwangYong Choi's comments View in context: https://bugs.webkit.org/attachment.cgi?id=159885&action=review > Source/WebKit2/WebProcess/Plugins/Netscape/x11/NetscapePluginX11.cpp:107 > return XDefaultScreen(NetscapePlugin::x11HostDisplay()); > #elif PLATFORM(GTK) > return gdk_screen_get_number(gdk_screen_get_default()); > +#elif PLATFORM(EFL) > + return ecore_x_screen_index_get(ecore_x_default_screen_get()); > #else I wonder why we have this #ifdef mess here when all we need is to use X11 functions. It sounds like to me that the current PLATFORM(QT) code path could be generalized to work with Gtk and EFL and all that remains in #ifdefs is the function to provide the Display*.

Comment on attachment 159885 [details] Rebased, and applied KwangYong Choi's comments View in context: https://bugs.webkit.org/attachment.cgi?id=159885&action=review >> Source/WebKit2/WebProcess/Plugins/Netscape/x11/NetscapePluginX11.cpp:107 >> #else > > I wonder why we have this #ifdef mess here when all we need is to use X11 functions. It sounds like to me that the current PLATFORM(QT) code path could be generalized to work with Gtk and EFL and all that remains in #ifdefs is the function to provide the Display*. (this comment is not meant to be directed against your work/patch, but just a general observation :)

Comment on attachment 159885 [details] Rebased, and applied KwangYong Choi's comments View in context: https://bugs.webkit.org/attachment.cgi?id=159885&action=review > Source/WebKit2/PluginProcess/efl/PluginProcessMainEfl.cpp:77 > + JSC::initializeThreading(); > + WTF::initializeMainThread(); I think that ScriptController::initializeThreading looks better. > Source/WebKit2/WebProcess/Plugins/Netscape/x11/NetscapePluginX11.cpp:50 > +#if PLATFORM(GTK) || (PLATFORM(EFL) && USE(CAIRO)) I think that USE(CAIRO) looks enough but we should check win-cairo port. > Source/cmake/OptionsEfl.cmake:125 > +IF (ENABLE_WEBKIT2) > + SET(ENABLE_PLUGIN_PROCESS 1) > +ENDIF () Is it working although we disabled ENABLE_ECORE_X or can not find ecore-x ? > Tools/Scripts/webkitperl/FeatureList.pm:325 > - define => "ENABLE_NETSCAPE_PLUGIN_API", default => !isEfl(), value => \$netscapePluginAPISupport }, > + define => "ENABLE_NETSCAPE_PLUGIN_API", default => 1, value => \$netscapePluginAPISupport }, If possible, can we separate this ? It's because ENABLE_NETSCAPE_PLUGIN_API is not only for plugin process and I hope that it is enabled as soon as possible.

Any update on this?

Created attachment 163620 [details] Rebased, applied comments

Comment on attachment 163620 [details] Rebased, applied comments View in context: https://bugs.webkit.org/attachment.cgi?id=163620&action=review > Source/WebCore/PlatformEfl.cmake:16 > + "${WEBKIT_DIR}/efl/WebCoreSupport" I want not to add WebKit dependency. I will create bug to fix this issue. > Source/WebKit2/PlatformEfl.cmake:258 > + SET(PluginProcess_LIBRARIES > + ${WebKit2_LIBRARY_NAME} > + ) I need to test. but I think that we need javascript core and webcore for shared core build. > Source/WebKit2/PluginProcess/efl/PluginProcessMainEfl.cpp:76 > + if (!ecore_x_init(0)) > + return 1; I want #ifdef HAVE_ECORE_X > Source/WebKit2/UIProcess/efl/WebPageProxyEfl.cpp:86 > +void WebPageProxy::createPluginContainer(uint64_t& windowID) we can remove variable name until we implement. > Source/WebKit2/UIProcess/efl/WebPageProxyEfl.cpp:91 > +void WebPageProxy::windowedPluginGeometryDidChange(const WebCore::IntRect& frameRect, const WebCore::IntRect& clipRect, uint64_t windowID) ditto. > Source/WebKit2/WebProcess/Plugins/Netscape/x11/NetscapePluginX11.cpp:50 > +#if PLATFORM(GTK) || (PLATFORM(EFL) && USE(CAIRO)) #if USE(CAIRO) && !PLATFORM(WIN_CAIRO) > Source/WebKit2/WebProcess/Plugins/Netscape/x11/NetscapePluginX11.cpp:93 > + return static_cast<Display*>(ecore_x_display_get()); macro is needed. > Source/cmake/OptionsEfl.cmake:118 > +IF (ENABLE_WEBKIT2) > + SET(ENABLE_PLUGIN_PROCESS 1) > +ENDIF () Should we set PLUGIN_PROCESS as a default for WEBKIT2?

Created attachment 163648 [details] Rebased, applied comments

(In reply to comment #22) > (From update of attachment 163620 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=163620&action=review > > Source/cmake/OptionsEfl.cmake:118 > > +IF (ENABLE_WEBKIT2) > > + SET(ENABLE_PLUGIN_PROCESS 1) > > +ENDIF () > > Should we set PLUGIN_PROCESS as a default for WEBKIT2? Yes, because there is bug: "[GTK][WK2] Build break when trying to build webkit2 without plugin process" which I reported to gtk: https://bugs.webkit.org/show_bug.cgi?id=89451

Comment on attachment 163648 [details] Rebased, applied comments View in context: https://bugs.webkit.org/attachment.cgi?id=163648&action=review > Source/WebKit2/PluginProcess/efl/PluginProcessMainEfl.cpp:66 > +int PluginProcessMainEfl(int argc, char* argv[]) This file as well is a 99% copy of PluginProcessMainGtk.cpp, with just the gtk_init() replaced with eine_init() and ecore_x_init(). Wouldn't it be better to share one file and have an #ifdef for the toolkit init in there instead of duplicating the entire file? Then you can also get rid of PluginMainEfl.cpp. > Source/WebKit2/UIProcess/Plugins/efl/PluginProcessProxyEfl.cpp:86 > + CString binaryPath = fileSystemRepresentation(executablePathOfPluginProcess()); > + CString pluginPathCString = fileSystemRepresentation(pluginPath); > + char* argv[4]; > + argv[0] = const_cast<char*>(binaryPath.data()); > + argv[1] = const_cast<char*>("-scanPlugin"); > + argv[2] = const_cast<char*>(pluginPathCString.data()); > + argv[3] = 0; > + > + int status; > + char* stdOut; > + > + if (!g_spawn_sync(0, argv, 0, G_SPAWN_STDERR_TO_DEV_NULL, 0, 0, &stdOut, 0, &status, 0)) > + return false; > + > + if (!WIFEXITED(status) || WEXITSTATUS(status) != EXIT_SUCCESS) > + return false; > + > + const unsigned kNumLinesExpected = 3; > + String lines[kNumLinesExpected]; > + unsigned lineIndex = 0; > + const UChar* current = reinterpret_cast<const UChar*>(stdOut); > + while (lineIndex < kNumLinesExpected) { > + const UChar* start = current; > + while (*current++ != UChar('\n')) { } > + lines[lineIndex++] = String(start, current - start - 1); > + } > + > + if (stdOut) > + free(stdOut); > + > + result.name.swap(lines[0]); > + result.description.swap(lines[1]); > + result.mimeDescription.swap(lines[2]); > + > + return !result.mimeDescription.isEmpty(); This function along with the rest of the file is a 99% copy of PluginProcessProxyGtk.cpp, which btw is (C) Igalia. Is there any way that you can modify your build system to simply compile their version if you have GLIB_SUPPORT enabled and omit this file altogether when it just duplicates the code?

(In reply to comment #25) > (From update of attachment 163648 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=163648&action=review > > > Source/WebKit2/PluginProcess/efl/PluginProcessMainEfl.cpp:66 > > +int PluginProcessMainEfl(int argc, char* argv[]) > > This file as well is a 99% copy of PluginProcessMainGtk.cpp, with just the gtk_init() replaced with eine_init() and ecore_x_init(). Wouldn't it be better to share one file and have an #ifdef for the toolkit init in there instead of duplicating the entire file? Then you can also get rid of PluginMainEfl.cpp. > > > Source/WebKit2/UIProcess/Plugins/efl/PluginProcessProxyEfl.cpp:86 > > + CString binaryPath = fileSystemRepresentation(executablePathOfPluginProcess()); > > + CString pluginPathCString = fileSystemRepresentation(pluginPath); > > + char* argv[4]; > > + argv[0] = const_cast<char*>(binaryPath.data()); > > + argv[1] = const_cast<char*>("-scanPlugin"); > > + argv[2] = const_cast<char*>(pluginPathCString.data()); > > + argv[3] = 0; > > + > > + int status; > > + char* stdOut; > > + > > + if (!g_spawn_sync(0, argv, 0, G_SPAWN_STDERR_TO_DEV_NULL, 0, 0, &stdOut, 0, &status, 0)) > > + return false; > > + > > + if (!WIFEXITED(status) || WEXITSTATUS(status) != EXIT_SUCCESS) > > + return false; > > + > > + const unsigned kNumLinesExpected = 3; > > + String lines[kNumLinesExpected]; > > + unsigned lineIndex = 0; > > + const UChar* current = reinterpret_cast<const UChar*>(stdOut); > > + while (lineIndex < kNumLinesExpected) { > > + const UChar* start = current; > > + while (*current++ != UChar('\n')) { } > > + lines[lineIndex++] = String(start, current - start - 1); > > + } > > + > > + if (stdOut) > > + free(stdOut); > > + > > + result.name.swap(lines[0]); > > + result.description.swap(lines[1]); > > + result.mimeDescription.swap(lines[2]); > > + > > + return !result.mimeDescription.isEmpty(); > > This function along with the rest of the file is a 99% copy of PluginProcessProxyGtk.cpp, which btw is (C) Igalia. Is there any way that you can modify your build system to simply compile their version if you have GLIB_SUPPORT enabled and omit this file altogether when it just duplicates the code? I thought about removing glib in the future, replacing it by some ecore functions, but sharing code is also reasonable. I'm wondering about name of common directory: sometimes it is unix, and sometimes x11. Both of them seems not exactly proper because efl have/will have windows, wayland support.

(In reply to comment #26) [...] > I thought about removing glib in the future, replacing it by some ecore functions, but sharing code is also reasonable. I'm wondering about name of common directory: sometimes it is unix, and sometimes x11. Both of them seems not exactly proper because efl have/will have windows, wayland support. I'd say it depends on the code. If the code is window system specific, then it should be x11. If the code is not window system specific, then unix is probably a better name. When you build the ELF port on Windows, I think you're more likely to end up with a PluginControllerProxyWin.cpp instead of *Efl.cpp.

Created attachment 163889 [details] Merged common files with gtk

Comment on attachment 163889 [details] Merged common files with gtk View in context: https://bugs.webkit.org/attachment.cgi?id=163889&action=review > ChangeLog:3 > + [EFL][WK2] Plugin process implementation New introduced approach makes this title out of data. What about: "[WK2][GTK][EFL] Share WebKit2-GTK plugin process implementation with EFL port" ? > Source/WebKit2/ChangeLog:12 > + * PlatformEfl.cmake: Added files needed by plugin process. Please keep the same tense in the ChangeLog file. Your are using Added, Adds, Add. You can choose the most suitable for you. > Source/WebKit2/PluginProcess/unix/PluginProcessMainUnix.cpp:52 > +#if PLATFORM(GTK) This code seems duplicated for both port. Please consider common code: 1) gchar can be dropped here, char type is enough to save that message 2) Error message that is passed to g_warning and EINA_LOG_CRIT can be saved somewhere as it almost the same for both ports. If there is no any API to get program name what about using g_get_progname() in EFL with GLIB_SUPPORT macro. 3) Depend on port call g_warning(message) or EINA_LOG_CRIT(message) > Source/WebKit2/UIProcess/Plugins/unix/PluginProcessProxyUnix.cpp:64 > +#if PLATFORM(GTK) This code might be merged too. Please consider 1) gint -> int 2) GOwnPtr -> WTF:OwnPtr After that we will have shared code. > Source/WebKit2/UIProcess/Plugins/unix/PluginProcessProxyUnix.cpp:84 > +#if PLATFORM(GTK) Won't be necessary if you apply above suggestion.

(In reply to comment #29) > (From update of attachment 163889 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=163889&action=review > > > ChangeLog:3 > > + [EFL][WK2] Plugin process implementation > > New introduced approach makes this title out of data. What about: "[WK2][GTK][EFL] Share WebKit2-GTK plugin process implementation with EFL port" ? > > > Source/WebKit2/ChangeLog:12 > > + * PlatformEfl.cmake: Added files needed by plugin process. > > Please keep the same tense in the ChangeLog file. Your are using Added, Adds, Add. You can choose the most suitable for you. > > > Source/WebKit2/PluginProcess/unix/PluginProcessMainUnix.cpp:52 > > +#if PLATFORM(GTK) > > This code seems duplicated for both port. Please consider common code: > 1) gchar can be dropped here, char type is enough to save that message > 2) Error message that is passed to g_warning and EINA_LOG_CRIT can be saved somewhere as it almost the same for both ports. If there is no any API to get program name what about using g_get_progname() in EFL with GLIB_SUPPORT macro. > 3) Depend on port call g_warning(message) or EINA_LOG_CRIT(message) > > > Source/WebKit2/UIProcess/Plugins/unix/PluginProcessProxyUnix.cpp:64 > > +#if PLATFORM(GTK) > > This code might be merged too. Please consider > 1) gint -> int > 2) GOwnPtr -> WTF:OwnPtr > After that we will have shared code. > > > Source/WebKit2/UIProcess/Plugins/unix/PluginProcessProxyUnix.cpp:84 > > +#if PLATFORM(GTK) > > Won't be necessary if you apply above suggestion. I agree with Grzegorz, you should really avoid as much as possible the #if PLATFORM() in the generic *Unix file. While it does not look too ugly now that only 2 ports are using it, it will get a lot worse when other ports start to use it. We should keep this code generic (Posix + WTF). Optimally, there should be no port specific code in there.

Comment on attachment 163889 [details] Merged common files with gtk Can we unskip any test with this?

Created attachment 164363 [details] Applied Grzegorz's comments

I think This is the way to go. CC'ing Martin, could you take a look this patch ?

View in context: https://bugs.webkit.org/attachment.cgi?id=164363&action=review Thanks for the changes. LGTM! Carlos, could you express your opinion about it? > Source/WebKit2/PluginProcess/unix/PluginProcessMainUnix.cpp:50 > +static char* progname = 0; How about 'programName'?

Comment on attachment 164363 [details] Applied Grzegorz's comments I'm not sure about some of the files, the implementation is the same because it's currently unimplemented, not because the implementation is expected to be common to all ports in a unix platform. PluginProcess/unix/PluginProcessMainUnix.cpp should probably be X11 instead of Unix, but it requires a lot of #ifdefs so I'm not sure whether it would be better to keep separate files without #ifdefs. Note also that Qt port could also share code common to unix platform, like in Platform/CoreIPC/unix/ConnectionUnix.cpp, but it seems to me that most of the files shared here are not for platform specific code, but for port specific code.

Comment on attachment 164363 [details] Applied Grzegorz's comments View in context: https://bugs.webkit.org/attachment.cgi?id=164363&action=review > Source/WebKit2/PluginProcess/unix/PluginProcessMainUnix.cpp:61 > +#if PLATFORM(GTK) > + g_warning( > +#elif PLATFORM(EFL) > + EINA_LOG_CRIT( > +#endif This doesn't seem very scalable... what if Qt decided to use this code? Maybe abstract this in Platform.h? > Source/WebKit2/PluginProcess/unix/PluginProcessMainUnix.cpp:77 > + bool scanPlugin = !strcmp(argv[1], "-scanPlugin"); why not two -s? --scan-plugins > Source/WebKit2/PluginProcess/unix/PluginProcessMainUnix.cpp:87 > +#if PLATFORM(GTK) > + gtk_init(&argc, &argv); > +#elif PLATFORM(EFL) > + if (!eina_init()) > + return 1; > +#ifdef HAVE_ECORE_X > + if (!ecore_x_init(0)) > +#endif I guess you rather what a platformInitialize() method for so > Source/WebKit2/UIProcess/Plugins/unix/PluginProcessProxyUnix.cpp:66 > + int status; > + char* stdOut; > + > + if (!g_spawn_sync(0, argv, 0, G_SPAWN_STDERR_TO_DEV_NULL, 0, 0, &stdOut, 0, &status, 0)) > + return false; > + This looks so much more like a PluginProcessProxyGlib than a UNIX one

Created attachment 164865 [details] Applied comments

Attachment 164865 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'Source/WebKit2/ChangeLog', u..." exit_code: 1 Source/WebKit2/PluginProcess/unix/PluginProcessMainUnix.cpp:81: Should have a space between // and comment [whitespace/comments] [4] Total errors found: 1 in 18 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 164363 [details] Applied Grzegorz's comments View in context: https://bugs.webkit.org/attachment.cgi?id=164363&action=review >> Source/WebKit2/PluginProcess/unix/PluginProcessMainUnix.cpp:61 >> +#endif > > This doesn't seem very scalable... what if Qt decided to use this code? Maybe abstract this in Platform.h? Changed to webkit's log >> Source/WebKit2/PluginProcess/unix/PluginProcessMainUnix.cpp:77 >> + bool scanPlugin = !strcmp(argv[1], "-scanPlugin"); > > why not two -s? --scan-plugins Qt and Gtk are using "-scanPlugin" so I think it can be left as is to avoid changes on their ports. >> Source/WebKit2/PluginProcess/unix/PluginProcessMainUnix.cpp:87 >> +#endif > > I guess you rather what a platformInitialize() method for so platformInitialize is done later in "WebKit::PluginProcess::shared().initialize(socket, RunLoop::main());" line. Here gtk needs arguments from command line, and efl needs ecore x init before platformInitialize.

Created attachment 164867 [details] Fixes webkit style

(In reply to comment #34) > View in context: https://bugs.webkit.org/attachment.cgi?id=164363&action=review > > Thanks for the changes. LGTM! > Carlos, could you express your opinion about it? > > > Source/WebKit2/PluginProcess/unix/PluginProcessMainUnix.cpp:50 > > +static char* progname = 0; > > How about 'programName'? Done

Comment on attachment 164867 [details] Fixes webkit style See Comment #35.

Attachment 164867 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'Source/WebKit2/ChangeLog', u..." exit_code: 1 Source/WebKit2/PluginProcess/unix/PluginProcessMainUnix.cpp:81: Should have a space between // and comment [whitespace/comments] [4] Total errors found: 1 in 18 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 164889 [details] Fixes webkit style

(In reply to comment #35) > (From update of attachment 164363 [details]) > I'm not sure about some of the files, the implementation is the same because it's currently unimplemented, not because the implementation is expected to be common to all ports in a unix platform. PluginProcess/unix/PluginProcessMainUnix.cpp should probably be X11 instead of Unix, but it requires a lot of #ifdefs so I'm not sure whether it would be better to keep separate files without #ifdefs. Note also that Qt port could also share code common to unix platform, like in Platform/CoreIPC/unix/ConnectionUnix.cpp, but it seems to me that most of the files shared here are not for platform specific code, but for port specific code. PluginProcessMainUnix seems to be more "unix" than "x11" because X11 support adds only error handling. Sometimes it's hard to point the best solution: port, backend, operating system. What's your suggestion?

(In reply to comment #45) > (In reply to comment #35) > > (From update of attachment 164363 [details] [details]) > > I'm not sure about some of the files, the implementation is the same because it's currently unimplemented, not because the implementation is expected to be common to all ports in a unix platform. PluginProcess/unix/PluginProcessMainUnix.cpp should probably be X11 instead of Unix, but it requires a lot of #ifdefs so I'm not sure whether it would be better to keep separate files without #ifdefs. Note also that Qt port could also share code common to unix platform, like in Platform/CoreIPC/unix/ConnectionUnix.cpp, but it seems to me that most of the files shared here are not for platform specific code, but for port specific code. > > PluginProcessMainUnix seems to be more "unix" than "x11" because X11 support adds only error handling. > Sometimes it's hard to point the best solution: port, backend, operating system. > What's your suggestion? My suggestion is to keep separate files instead of a "common" file full of #ifdefs

(In reply to comment #46) > (In reply to comment #45) > > (In reply to comment #35) > > > (From update of attachment 164363 [details] [details] [details]) > > > I'm not sure about some of the files, the implementation is the same because it's currently unimplemented, not because the implementation is expected to be common to all ports in a unix platform. PluginProcess/unix/PluginProcessMainUnix.cpp should probably be X11 instead of Unix, but it requires a lot of #ifdefs so I'm not sure whether it would be better to keep separate files without #ifdefs. Note also that Qt port could also share code common to unix platform, like in Platform/CoreIPC/unix/ConnectionUnix.cpp, but it seems to me that most of the files shared here are not for platform specific code, but for port specific code. > > > > PluginProcessMainUnix seems to be more "unix" than "x11" because X11 support adds only error handling. > > Sometimes it's hard to point the best solution: port, backend, operating system. > > What's your suggestion? > > My suggestion is to keep separate files instead of a "common" file full of #ifdefs A lot of proposed changes have common source. I think PluginControllerProxyUnix.cpp, and PluginProcessUnix.cpp can be EFL and Gtk as before because of platform initialize, and destroy, but others may stay as is. Do you agree with it?

(In reply to comment #47) > (In reply to comment #46) > > (In reply to comment #45) > > > (In reply to comment #35) > > > > (From update of attachment 164363 [details] [details] [details] [details]) > > > > I'm not sure about some of the files, the implementation is the same because it's currently unimplemented, not because the implementation is expected to be common to all ports in a unix platform. PluginProcess/unix/PluginProcessMainUnix.cpp should probably be X11 instead of Unix, but it requires a lot of #ifdefs so I'm not sure whether it would be better to keep separate files without #ifdefs. Note also that Qt port could also share code common to unix platform, like in Platform/CoreIPC/unix/ConnectionUnix.cpp, but it seems to me that most of the files shared here are not for platform specific code, but for port specific code. > > > > > > PluginProcessMainUnix seems to be more "unix" than "x11" because X11 support adds only error handling. > > > Sometimes it's hard to point the best solution: port, backend, operating system. > > > What's your suggestion? > > > > My suggestion is to keep separate files instead of a "common" file full of #ifdefs > > A lot of proposed changes have common source. I think PluginControllerProxyUnix.cpp, and PluginProcessUnix.cpp can be EFL and Gtk as before because of platform initialize, and destroy, but others may stay as is. Do you agree with it? They have a common source because they are unimplemented not because they are supposed to have a common implementation for unix platform. The code is also the same as the Qt files, if we are going to share the unimplemented files, we should include the Qt files as well.

(In reply to comment #48) > (In reply to comment #47) > > (In reply to comment #46) > > > (In reply to comment #45) > > > > (In reply to comment #35) > > > > > (From update of attachment 164363 [details] [details] [details] [details] [details]) > > > > > I'm not sure about some of the files, the implementation is the same because it's currently unimplemented, not because the implementation is expected to be common to all ports in a unix platform. PluginProcess/unix/PluginProcessMainUnix.cpp should probably be X11 instead of Unix, but it requires a lot of #ifdefs so I'm not sure whether it would be better to keep separate files without #ifdefs. Note also that Qt port could also share code common to unix platform, like in Platform/CoreIPC/unix/ConnectionUnix.cpp, but it seems to me that most of the files shared here are not for platform specific code, but for port specific code. > > > > > > > > PluginProcessMainUnix seems to be more "unix" than "x11" because X11 support adds only error handling. > > > > Sometimes it's hard to point the best solution: port, backend, operating system. > > > > What's your suggestion? > > > > > > My suggestion is to keep separate files instead of a "common" file full of #ifdefs > > > > A lot of proposed changes have common source. I think PluginControllerProxyUnix.cpp, and PluginProcessUnix.cpp can be EFL and Gtk as before because of platform initialize, and destroy, but others may stay as is. Do you agree with it? > > They have a common source because they are unimplemented not because they are supposed to have a common implementation for unix platform. > The code is also the same as the Qt files, if we are going to share the unimplemented files, we should include the Qt files as well. Because of: 1. Possible differences in unsupported functions 2. No good suffix for common files 3. For Unix suffix Qt source should be merged which implies a lot of macros I think that finally previous solution will be better. Simon, do you agree with that?

This patch does not build in debug mode. Please always test debug builds. /home/chris/unencrypted/WebKit/Source/WebKit2/PluginProcess/unix/PluginProcessMainUnix.cpp: In function ‘int webkitXError(Display*, XErrorEvent*)’: /home/chris/unencrypted/WebKit/Source/WebKit2/PluginProcess/unix/PluginProcessMainUnix.cpp:57:2: error: ‘LOG_CHANNEL_PREFIXPlugins’ was not declared in this scope

Please check if tests can be unskipped with this patch. It seems at least the following tests can be unskipped: fast/replaced/no-focus-ring-embed.html fast/replaced/no-focus-ring-object.html

(In reply to comment #51) > Please check if tests can be unskipped with this patch. > > It seems at least the following tests can be unskipped: > fast/replaced/no-focus-ring-embed.html > fast/replaced/no-focus-ring-object.html There are lots of tests for plugins. At least, plugins/ and http/tests/plugins/. How about rebaseline after this patch is landed?

Comment on attachment 164889 [details] Fixes webkit style View in context: https://bugs.webkit.org/attachment.cgi?id=164889&action=review > Source/WebKit2/WebProcess/Plugins/Netscape/x11/NetscapePluginX11.cpp:106 > +#elif PLATFORM(EFL) && defined(HAVE_ECORE_X) > + return ecore_x_screen_index_get(ecore_x_default_screen_get()); I still think that this kind of stuff can easily be simplified to use Xlib functions (see PLATFORM(QT) code path), given a platform-specific way of getting the X display. But that can be done in a separate patch if you'd like.

(In reply to comment #49) > (In reply to comment #48) > > (In reply to comment #47) > > > (In reply to comment #46) > > > > (In reply to comment #45) > > > > > (In reply to comment #35) > > > > > > (From update of attachment 164363 [details] [details] [details] [details] [details] [details]) > > > > > > I'm not sure about some of the files, the implementation is the same because it's currently unimplemented, not because the implementation is expected to be common to all ports in a unix platform. PluginProcess/unix/PluginProcessMainUnix.cpp should probably be X11 instead of Unix, but it requires a lot of #ifdefs so I'm not sure whether it would be better to keep separate files without #ifdefs. Note also that Qt port could also share code common to unix platform, like in Platform/CoreIPC/unix/ConnectionUnix.cpp, but it seems to me that most of the files shared here are not for platform specific code, but for port specific code. > > > > > > > > > > PluginProcessMainUnix seems to be more "unix" than "x11" because X11 support adds only error handling. > > > > > Sometimes it's hard to point the best solution: port, backend, operating system. > > > > > What's your suggestion? > > > > > > > > My suggestion is to keep separate files instead of a "common" file full of #ifdefs > > > > > > A lot of proposed changes have common source. I think PluginControllerProxyUnix.cpp, and PluginProcessUnix.cpp can be EFL and Gtk as before because of platform initialize, and destroy, but others may stay as is. Do you agree with it? > > > > They have a common source because they are unimplemented not because they are supposed to have a common implementation for unix platform. > > The code is also the same as the Qt files, if we are going to share the unimplemented files, we should include the Qt files as well. > > Because of: > 1. Possible differences in unsupported functions > 2. No good suffix for common files > 3. For Unix suffix Qt source should be merged which implies a lot of macros > I think that finally previous solution will be better. > > Simon, do you agree with that? I agree that there is not much value in calling a file with only notImplemented() files FooUnix.cpp, but splitting it up in *Efl and *Gtk.cpp doesn't make much sense to me neither. There could be a NotImplemented.cpp file like in WebCore, but you really have to ask yourself: What is the code going to look like in the future? This is in a nutshell about implementing an X11 specific "standard", which for all means and purposes is tied to Unixy platforms. For files like Source/WebKit2/UIProcess/Plugins/unix/PluginProcessProxyUnix.cpp I really don't see a point in having the code duplicated between Efl and Gtk. (I won't r+ that, but maybe somebody else will)

(In reply to comment #54) [...] > For files like Source/WebKit2/UIProcess/Plugins/unix/PluginProcessProxyUnix.cpp I really don't see a point in having the code duplicated between Efl and Gtk. (I won't r+ that, but maybe somebody else will) (To clarify: I won't r+ a patch that duplicates perfectly shareable code, but maybe somebody else will do :)

The latest patch (with sharable code) has got r+. IMHO we should make a final decision in which form the patch will be landed. Carlos, would you like to have the separate files/implementation for: - PluginProcessUnix.cpp - PluginControllerProxyUnix.cpp for the port specific files qt/gtk/efl etc? In this case all of them will look mostly the same and have unimplemented all methods. In my opinion this patch makes sense. If some has knowledge how those unimplemented methods should look like or knows whether they are depend on the port specific API/features/files now would be a good time to speak up!

(In reply to comment #56) > The latest patch (with sharable code) has got r+. IMHO we should make a final decision in which form the patch will be landed. > > Carlos, would you like to have the separate files/implementation for: > - PluginProcessUnix.cpp > - PluginControllerProxyUnix.cpp > for the port specific files qt/gtk/efl etc? In this case all of them will look mostly the same and have unimplemented all methods. > > In my opinion this patch makes sense. If some has knowledge how those unimplemented methods should look like or knows whether they are depend on the port specific API/features/files now would be a good time to speak up! We can just share the "code" of those not implemented files, and in case a port needs to add code in the future that is not specific to unix or that would require too many #ifdefs we can split the files again.

(In reply to comment #57) > (In reply to comment #56) > > The latest patch (with sharable code) has got r+. IMHO we should make a final decision in which form the patch will be landed. > > > > Carlos, would you like to have the separate files/implementation for: > > - PluginProcessUnix.cpp > > - PluginControllerProxyUnix.cpp > > for the port specific files qt/gtk/efl etc? In this case all of them will look mostly the same and have unimplemented all methods. > > > > In my opinion this patch makes sense. If some has knowledge how those unimplemented methods should look like or knows whether they are depend on the port specific API/features/files now would be a good time to speak up! > > We can just share the "code" of those not implemented files, and in case a port needs to add code in the future that is not specific to unix or that would require too many #ifdefs we can split the files again. Thanks for your opinion Carlos. Mariusz, please fix the build on the debug build and finally land this one.

Created attachment 166229 [details] Rebased, fixed LOG error

Created attachment 166232 [details] Rebased, fixed LOG error

Comment on attachment 166232 [details] Rebased, fixed LOG error Attachment 166232 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/14056393

Comment on attachment 166232 [details] Rebased, fixed LOG error View in context: https://bugs.webkit.org/attachment.cgi?id=166232&action=review > ChangeLog:8 > + Turn on Netscape Plugin API by default. by default for WebKit2-Efl. > Source/WebKit2/PlatformEfl.cmake:12 > + PluginProcess/unix/PluginProcessMainUnix.cpp Please keep alphabetical order. > Source/WebKit2/UIProcess/Plugins/unix/PluginProcessProxyUnix.cpp:62 > + char* stdOut; Please initialize it to 0 to be sure that free() won't be called for non allocated memory.

Created attachment 166244 [details] Fixed merge error, and applied Grzegorz's comments

Comment on attachment 166244 [details] Fixed merge error, and applied Grzegorz's comments Clearing flags on attachment: 166244 Committed r129972: <http://trac.webkit.org/changeset/129972>

All reviewed patches have been landed. Closing bug.

(In reply to comment #64) > (From update of attachment 166244 [details]) > Clearing flags on attachment: 166244 > > Committed r129972: <http://trac.webkit.org/changeset/129972> Hi, This change has broken the EFL build if one passes --no-netscape-plugin-api to build-webkit. Can you take a look at this, Mariusz?

(In reply to comment #66) > (In reply to comment #64) > > (From update of attachment 166244 [details] [details]) > > Clearing flags on attachment: 166244 > > > > Committed r129972: <http://trac.webkit.org/changeset/129972> > > Hi, > > This change has broken the EFL build if one passes --no-netscape-plugin-api to build-webkit. Can you take a look at this, Mariusz? I've made initial investigation and it seems that when PLUGIN_PROCESS is on and NETSCAPE_PLUGIN_API is off it causes a lot of buildbreaks. On some ports like Gtk both macros are on and there is no way to disable them from build-webkit script. In webkit1 there was only netscape plugin api macro which means all functionality is on/off. But in webkit2 it is more confusing, e.g. createPlugin function in WebPage.cpp: #if ENABLE(PLUGIN_PROCESS) return PluginProxy::create(pluginPath); #elif ENABLE(NETSCAPE_PLUGIN_API) NetscapePlugin::setSetExceptionFunction(NPRuntimeObjectMap::setGlobalException); return NetscapePlugin::create(NetscapePluginModule::getOrCreate(pluginPath)); #else return 0; #endif Later when plugin process is initialized in PluginControllerProxy.cpp: m_plugin = NetscapePlugin::create(PluginProcess::shared().netscapePluginModule()); NetscapePlugin::create is invoked which is in macro NETSCAPE_PLUGIN_API I'm wondering if plugin process should be created at all when NETSCAPE_PLUGIN_API is off. also getting plugin path in WebPage::createPlugin seems to be useless when plugins are off. I think it will be good to make new bug on bugzilla for this combination of macros.

I get the following warning when running unit tests and 1 test is failing: 21/37 Test #21: test_webkit2_api_FrameMIMETypePNG ...................................***Failed 10.31 sec [==========] Running 1 test from 1 test case. [----------] Global test environment set-up. [----------] 1 test from WebKit2 [ RUN ] WebKit2.FrameMIMETypePNG (process:28299): GLib-WARNING **: In call to g_spawn_sync(), exit status of a child process was requested but SIGCHLD action was set to SIG_IGN and ECHILD was received by waitpid(), so exit status can't be returned. This is a bug in the program calling g_spawn_sync(); either don't request the exit status, or don't set the SIGCHLD action. 1 0x43644f 2 0x2af7189c6cb0 3 0x2af7185b4e71 WebKit::PluginProcessProxy::scanPlugin(WTF::String const&, WebKit::RawPluginMetaData&) 4 0x2af718585f78 WebKit::NetscapePluginModule::getPluginInfo(WTF::String const&, WebKit::PluginModuleInfo&) 5 0x2af7185b3883 WebKit::PluginInfoStore::getPluginInfo(WTF::String const&, WebKit::PluginModuleInfo&) 6 0x2af718495fb7 WebKit::PluginInfoStore::loadPlugin(WTF::Vector<WebKit::PluginModuleInfo, 0ul>&, WTF::String const&) 7 0x2af718495eec WebKit::PluginInfoStore::loadPluginsIfNecessary() 8 0x2af71849602d WebKit::PluginInfoStore::plugins() 9 0x2af7184718a9 WebKit::WebProcessProxy::handleGetPlugins(unsigned long, bool) 10 0x2af71847dd77 WTF::FunctionWrapper<void (WebKit::WebProcessProxy::*)(unsigned long, bool)>::operator()(WebKit::WebProcessProxy*, unsigned long, bool) 11 0x2af71847dbb0 WTF::BoundFunctionImpl<WTF::FunctionWrapper<void (WebKit::WebProcessProxy::*)(unsigned long, bool)>, void (WebKit::WebProcessProxy*, unsigned long, bool)>::operator()() 12 0x465214 WTF::Function<void ()>::operator()() const 13 0x2af71857b74e WorkQueue::performWork() 14 0x2af71857bdcc WorkQueue::workQueueThread(WorkQueue*) 15 0x2af71d68e419 16 0x4374a9 17 0x2af7189bee9a 18 0x2af719d44dbd clone LEAK: 1 WebPageProxy LEAK: 1 WebContext It looks related.

> But in webkit2 it is more confusing, e.g. createPlugin function in WebPage.cpp: > #if ENABLE(PLUGIN_PROCESS) > return PluginProxy::create(pluginPath); > #elif ENABLE(NETSCAPE_PLUGIN_API) > NetscapePlugin::setSetExceptionFunction(NPRuntimeObjectMap::setGlobalException); AFAIK the distinction is that there could be different kinds of plugins - and Netscape plugins would be one of the kind of plugins. Perhaps the build flag for build-webkit should be called "no-plugins", but the build flags in the source files seems to be properly named. > I'm wondering if plugin process should be created at all when NETSCAPE_PLUGIN_API is off. For EFL and (and probably GTK) ports I would agree. > I think it will be good to make new bug on bugzilla for this combination of macros. See bug 99757.