Bug 91844 – [WK2][GTK][EFL] Share WebKit2-GTK plugin process implementation with EFL port

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit EFL

[Mariusz Grzegorczyk]

Adding windowless plugin, and plugin process implementation

[Mariusz Grzegorczyk]

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

[WebKit Review Bot]

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.

[Gyuyoung Kim]

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

[Mariusz Grzegorczyk]

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

[Chris Dumez]

(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

[Mariusz Grzegorczyk]

Created attachment 153525 [details]
Fixed webkit style

[Gyuyoung Kim]

Comment on attachment 153525 [details]
Fixed webkit style

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

[Gyuyoung Kim]

Comment on attachment 153525 [details]
Fixed webkit style

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

[KwangYong Choi]

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)

[Gyuyoung Kim]

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.

[Chris Dumez]

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.

[Ryuan Choi]

Could you update patch for green bot ?

[Gyuyoung Kim]

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.

[Mariusz Grzegorczyk]

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

[Gyuyoung Kim]

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.

[Raphael Kubo da Costa (:rakuco)]

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.

[Simon Hausmann]

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

[Simon Hausmann]

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

[Ryuan Choi]

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.

[Chris Dumez]

Any update on this?

[Mariusz Grzegorczyk]

Created attachment 163620 [details]
Rebased, applied comments

[Ryuan Choi]

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?

[Mariusz Grzegorczyk]

Created attachment 163648 [details]
Rebased, applied comments

[Mariusz Grzegorczyk]

(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

[Simon Hausmann]

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?

[Mariusz Grzegorczyk]

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

[Simon Hausmann]

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

[Mariusz Grzegorczyk]

Created attachment 163889 [details]
Merged common files with gtk

[Grzegorz Czajkowski]

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.

[Chris Dumez]

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

[Chris Dumez]

Comment on attachment 163889 [details]
Merged common files with gtk

Can we unskip any test with this?

[Mariusz Grzegorczyk]

Created attachment 164363 [details]
Applied Grzegorz's comments

[Gyuyoung Kim]

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

[Grzegorz Czajkowski]

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

[Carlos Garcia Campos]

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.

[Kenneth Rohde Christiansen]

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

[Mariusz Grzegorczyk]

Created attachment 164865 [details]
Applied comments

[WebKit Review Bot]

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.

[Mariusz Grzegorczyk]

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.

[Mariusz Grzegorczyk]

Created attachment 164867 [details]
Fixes webkit style

[Mariusz Grzegorczyk]

(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

[Carlos Garcia Campos]

Comment on attachment 164867 [details]
Fixes webkit style

See Comment #35.

[WebKit Review Bot]

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.

[Mariusz Grzegorczyk]

Created attachment 164889 [details]
Fixes webkit style

[Mariusz Grzegorczyk]

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

[Carlos Garcia Campos]

(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

[Mariusz Grzegorczyk]

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

[Carlos Garcia Campos]

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

[Mariusz Grzegorczyk]

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

[Chris Dumez]

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

[Chris Dumez]

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

[KwangYong Choi]

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

[Simon Hausmann]

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.

[Simon Hausmann]

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

[Simon Hausmann]

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

[Grzegorz Czajkowski]

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!

[Carlos Garcia Campos]

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

[Grzegorz Czajkowski]

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

[Mariusz Grzegorczyk]

Created attachment 166229 [details]
Rebased, fixed LOG error

[Mariusz Grzegorczyk]

Created attachment 166232 [details]
Rebased, fixed LOG error

[Gyuyoung Kim]

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

[Grzegorz Czajkowski]

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.

[Mariusz Grzegorczyk]

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

[WebKit Review Bot]

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>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Raphael Kubo da Costa (:rakuco)]

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

[Mariusz Grzegorczyk]

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

[Chris Dumez]

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.

[Laszlo Gombos]

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