Bug 91844

Summary: [WK2][GTK][EFL] Share WebKit2-GTK plugin process implementation with EFL port
Product: WebKit Reporter: Mariusz Grzegorczyk <mariusz.g>
Component: WebKit EFLAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: cdumez, cgarcia, g.czajkowski, gyuyoung.kim, hausmann, kbalazs, ky0.choi, laszlo.gombos, lucas.de.marchi, mrobinson, naginenis, rakuco, ryuan.choi, sw0524.lee, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 89719, 96513    
Bug Blocks: 96881    
Attachments:
Description Flags
Patch implementing windowless plugin and plugin process for webkit2 efl's port
gyuyoung.kim: commit-queue-
Fixed webkit style
gyuyoung.kim: commit-queue-
Rebased, and applied KwangYong Choi's comments
none
Rebased, applied comments
none
Rebased, applied comments
none
Merged common files with gtk
none
Applied Grzegorz's comments
none
Applied comments
none
Fixes webkit style
none
Fixes webkit style
hausmann: review+
Rebased, fixed LOG error
none
Rebased, fixed LOG error
gyuyoung.kim: commit-queue-
Fixed merge error, and applied Grzegorz's comments none

Description Mariusz Grzegorczyk 2012-07-20 02:44:17 PDT
Adding windowless plugin, and plugin process implementation
Comment 1 Mariusz Grzegorczyk 2012-07-20 03:18:28 PDT
Created attachment 153463 [details]
Patch implementing windowless plugin and plugin process for webkit2 efl's port
Comment 2 WebKit Review Bot 2012-07-20 03:21:39 PDT
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 3 Gyuyoung Kim 2012-07-20 03:46:14 PDT
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
Comment 4 Mariusz Grzegorczyk 2012-07-20 07:27:24 PDT
(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?
Comment 5 Chris Dumez 2012-07-20 07:31:01 PDT
(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
Comment 6 Mariusz Grzegorczyk 2012-07-20 09:33:08 PDT
Created attachment 153525 [details]
Fixed webkit style
Comment 7 Gyuyoung Kim 2012-07-20 10:57:03 PDT
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 8 Gyuyoung Kim 2012-07-20 11:14:27 PDT
Comment on attachment 153525 [details]
Fixed webkit style

Attachment 153525 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13308330
Comment 9 KwangYong Choi 2012-07-22 22:53:55 PDT
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 10 Gyuyoung Kim 2012-07-23 18:40:47 PDT
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 11 Chris Dumez 2012-07-23 23:09:42 PDT
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.
Comment 12 Ryuan Choi 2012-08-16 17:54:31 PDT
Could you update patch for green bot ?
Comment 13 Gyuyoung Kim 2012-08-16 19:00:38 PDT
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.
Comment 14 Mariusz Grzegorczyk 2012-08-22 02:10:39 PDT
Created attachment 159885 [details]
Rebased, and applied KwangYong Choi's comments
Comment 15 Gyuyoung Kim 2012-08-22 02:44:42 PDT
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 16 Raphael Kubo da Costa (:rakuco) 2012-08-22 09:13:05 PDT
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 17 Simon Hausmann 2012-08-23 00:27:49 PDT
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 18 Simon Hausmann 2012-08-23 00:28:34 PDT
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 19 Ryuan Choi 2012-08-23 16:53:32 PDT
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.
Comment 20 Chris Dumez 2012-09-07 02:59:24 PDT
Any update on this?
Comment 21 Mariusz Grzegorczyk 2012-09-12 07:08:29 PDT
Created attachment 163620 [details]
Rebased, applied comments
Comment 22 Ryuan Choi 2012-09-12 07:22:50 PDT
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?
Comment 23 Mariusz Grzegorczyk 2012-09-12 09:27:49 PDT
Created attachment 163648 [details]
Rebased, applied comments
Comment 24 Mariusz Grzegorczyk 2012-09-12 09:30:35 PDT
(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 25 Simon Hausmann 2012-09-12 11:50:39 PDT
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?
Comment 26 Mariusz Grzegorczyk 2012-09-13 01:25:40 PDT
(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.
Comment 27 Simon Hausmann 2012-09-13 01:32:23 PDT
(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.
Comment 28 Mariusz Grzegorczyk 2012-09-13 09:18:39 PDT
Created attachment 163889 [details]
Merged common files with gtk
Comment 29 Grzegorz Czajkowski 2012-09-14 05:40:30 PDT
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.
Comment 30 Chris Dumez 2012-09-14 06:14:31 PDT
(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 31 Chris Dumez 2012-09-16 04:56:17 PDT
Comment on attachment 163889 [details]
Merged common files with gtk

Can we unskip any test with this?
Comment 32 Mariusz Grzegorczyk 2012-09-17 03:24:10 PDT
Created attachment 164363 [details]
Applied Grzegorz's comments
Comment 33 Gyuyoung Kim 2012-09-17 03:34:56 PDT
I think This is the way to go. CC'ing Martin, could you take a look this patch ?
Comment 34 Grzegorz Czajkowski 2012-09-17 05:25:31 PDT
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 35 Carlos Garcia Campos 2012-09-17 05:52:25 PDT
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 36 Kenneth Rohde Christiansen 2012-09-17 06:25:37 PDT
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
Comment 37 Mariusz Grzegorczyk 2012-09-20 02:21:13 PDT
Created attachment 164865 [details]
Applied comments
Comment 38 WebKit Review Bot 2012-09-20 02:24:43 PDT
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 39 Mariusz Grzegorczyk 2012-09-20 02:26:53 PDT
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.
Comment 40 Mariusz Grzegorczyk 2012-09-20 02:28:23 PDT
Created attachment 164867 [details]
Fixes webkit style
Comment 41 Mariusz Grzegorczyk 2012-09-20 02:31:03 PDT
(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 42 Carlos Garcia Campos 2012-09-20 02:38:30 PDT
Comment on attachment 164867 [details]
Fixes webkit style

See Comment #35.
Comment 43 WebKit Review Bot 2012-09-20 03:31:32 PDT
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.
Comment 44 Mariusz Grzegorczyk 2012-09-20 04:41:50 PDT
Created attachment 164889 [details]
Fixes webkit style
Comment 45 Mariusz Grzegorczyk 2012-09-20 04:54:54 PDT
(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?
Comment 46 Carlos Garcia Campos 2012-09-20 05:11:04 PDT
(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
Comment 47 Mariusz Grzegorczyk 2012-09-20 05:53:31 PDT
(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?
Comment 48 Carlos Garcia Campos 2012-09-20 05:57:55 PDT
(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.
Comment 49 Mariusz Grzegorczyk 2012-09-20 07:38:08 PDT
(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?
Comment 50 Chris Dumez 2012-09-20 11:11:40 PDT
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
Comment 51 Chris Dumez 2012-09-20 11:19:38 PDT
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
Comment 52 KwangYong Choi 2012-09-20 19:31:43 PDT
(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 53 Simon Hausmann 2012-09-24 05:30:28 PDT
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.
Comment 54 Simon Hausmann 2012-09-24 07:37:30 PDT
(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)
Comment 55 Simon Hausmann 2012-09-24 07:38:18 PDT
(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 :)
Comment 56 Grzegorz Czajkowski 2012-09-28 01:28:39 PDT
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!
Comment 57 Carlos Garcia Campos 2012-09-28 01:40:39 PDT
(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.
Comment 58 Grzegorz Czajkowski 2012-09-28 02:03:26 PDT
(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.
Comment 59 Mariusz Grzegorczyk 2012-09-28 06:00:49 PDT
Created attachment 166229 [details]
Rebased, fixed LOG error
Comment 60 Mariusz Grzegorczyk 2012-09-28 06:07:25 PDT
Created attachment 166232 [details]
Rebased, fixed LOG error
Comment 61 Gyuyoung Kim 2012-09-28 06:46:30 PDT
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 62 Grzegorz Czajkowski 2012-09-28 06:58:26 PDT
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.
Comment 63 Mariusz Grzegorczyk 2012-09-28 07:12:37 PDT
Created attachment 166244 [details]
Fixed merge error, and applied Grzegorz's comments
Comment 64 WebKit Review Bot 2012-09-28 23:30:53 PDT
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>
Comment 65 WebKit Review Bot 2012-09-28 23:31:01 PDT
All reviewed patches have been landed.  Closing bug.
Comment 66 Raphael Kubo da Costa (:rakuco) 2012-10-02 09:05:11 PDT
(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?
Comment 67 Mariusz Grzegorczyk 2012-10-03 03:30:37 PDT
(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.
Comment 68 Chris Dumez 2012-10-03 23:44:56 PDT
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.
Comment 69 Laszlo Gombos 2012-10-18 17:10:42 PDT
> 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.