Bug 72121 – [Qt][WK2] Set up plugin process on Unix

• Status: RESOLVED
• Resolution: FIXED
• Component: Plug-ins

[Balazs Kelemen]

We already have the basic infrastructure, so let's make it alive.

[Balazs Kelemen]

Created attachment 114692 [details]
Patch

[Simon Hausmann]

Comment on attachment 114692 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=114692&action=review

r=me, but please remove the MeeGoTouch bit before landing :)

> Source/WebKit2/PluginProcess/qt/PluginProcessMainQt.cpp:52
> +#if USE(MEEGOTOUCH)
> +    new MComponentData(argc, argv);

MeegoTouch? No thanks :)

[Balazs Kelemen]

Comment on attachment 114692 [details]
Patch

Wait a bit, another approach need to be considered: reusing the gtk process since we depend on gtk anyway.

[Balazs Kelemen]

(In reply to comment #3)
> (From update of attachment 114692 [details])
> Wait a bit, another approach need to be considered: reusing the gtk process since we depend on gtk anyway.

After considering this possibility I think reusing the entry point is not a big deal so we shouldn't bother with that. So I'm going to land the patch if you are not against it.

[Simon Hausmann]

(In reply to comment #4)
> (In reply to comment #3)
> > (From update of attachment 114692 [details] [details])
> > Wait a bit, another approach need to be considered: reusing the gtk process since we depend on gtk anyway.
> 
> After considering this possibility I think reusing the entry point is not a big deal so we shouldn't bother with that. So I'm going to land the patch if you are not against it.

It's not about the entry point, it's about the plugin process being a Gtk+2 application (that doesn't link against Qt or Gtk3).

[Simon Hausmann]

For the time being I landed http://trac.webkit.org/changeset/100132 to disable plugins for x11 as long as they're in-process. This was causing crashes for too many developers, turning it into a PITA :)

[Balazs Kelemen]

Comment on attachment 114692 [details]
Patch

At least it needs to be updated and also it's not clear whether it is the desired design.

[Balazs Kelemen]

Adding a GTK expert because we need GTK knowleadge here.

[Balazs Kelemen]

I take a closer look into the GTK PluginProcess. (Context: I'm investigating of reusing the GTK2 based plugin process for the Qt port. The rationale is that plugins depend on GTK2 anyway and also that the architecture of Qt5 makes implementing plugin support a bit hard.) I was wondering about how much it is using WebCore and JSCore. It links to the WebCore, the Gtk2 version of JSCore (I'm not sure why GTK has this, maybe because of other reasons as well), and another static lib named WebCoreGtk2, which contains the GTK2 platform parts of WebCore. However, what gprof showed me is that it actually does not call into WebCore too much. Actually the profile created for just launching a flash movie and closing the app does not show me any calls into WebCore, just a few to WTF functions. In summary it seems to be not impossible to just find the dependencies manually and build all of them. I am also thinking about whether we could have a shared project file. In the case of Qt it would mean additional build time dependencies but maybe that's acceptable. I'm curious about your opinion on the topic.

[Carlos Garcia Campos]

(In reply to comment #9)
> I take a closer look into the GTK PluginProcess. (Context: I'm investigating of reusing the GTK2 based plugin process for the Qt port. The rationale is that plugins depend on GTK2 anyway and also that the architecture of Qt5 makes implementing plugin support a bit hard.) I was wondering about how much it is using WebCore and JSCore. It links to the WebCore, the Gtk2 version of JSCore (I'm not sure why GTK has this, maybe because of other reasons as well), and another static lib named WebCoreGtk2, which contains the GTK2 platform parts of WebCore. However, what gprof showed me is that it actually does not call into WebCore too much.

Maybe it has changed recently, but when I enabled plugin process for GTK+ port, the webkit2 code used by the plugin process depended on WebCore, that's why we had to split WebCore into two static libs, one containing only the files that actually use gtk to be able to build it twice: WebCoreGtk2 (linking to gtk2 and used only by plugin process) and WebCoreGtk (linking to gtk3 and used by web and UI processes).

> Actually the profile created for just launching a flash movie and closing the app does not show me any calls into WebCore, just a few to WTF functions. In summary it seems to be not impossible to just find the dependencies manually and build all of them. 

That's what I did, and the files listed in Programs_WebKitPluginProcess_SOURCES (in Source/WebKit2/GNUmakefile.am) are all the files I needed to build the plugin process.

> I am also thinking about whether we could have a shared project file. In the case of Qt it would mean additional build time dependencies but maybe that's acceptable. I'm curious about your opinion on the topic.

yes, it's more build time for us too, but I think it's worth it.

[Balazs Kelemen]

I have realized that I did the profile wrong. I only added "-g -pg" for the plugin process but not to WebCore. I'm going to do it again.

[Balazs Kelemen]

I have investigated a bit more. Unfortunately with no success. Actually the profiles does not show very much dependency but I guess that is because of their statistical nature. When I tried to throw away the WebCore static lib and just build the dependencies along with the binary than a lot of other undefined functions appeared. Even just GraphicsContext has a huge dependency tree. By just examining the binary (nm) there are tons of WebCore symbols. If I'm not wrong those would be thrown away if they would not be referenced. The fact is that the plugin process binary is 25M (release!). After all I don't think it's suitable for Qt to depend on a GTK WebCore just for plugins. Actually the real solution would be a self-contained plugin process but that is a totally different design than what we have now.

[Balazs Kelemen]

Created attachment 117617 [details]
patch

Another WIP patch (however I think it's good enough to be reviewed now).
I have to solve an issue with the Gtk build.

[WebKit Review Bot]

Attachment 117617 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'Source/QtWebKit.pro', u'Sour..." exit_code: 1

Source/WebKit2/PluginProcess/qt/PluginProcessMainQt.cpp:43:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebKit2/UIProcess/Plugins/unix/PluginProcessProxyUnix.cpp:37:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebKit2/UIProcess/Plugins/unix/PluginProcessProxyUnix.cpp:39:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebKit2/UIProcess/Plugins/unix/PluginProcessProxyUnix.cpp:119:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Source/WebKit2/Shared/Plugins/Netscape/x11/NetscapePluginModuleX11.cpp:185:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Source/WebKit2/qt/PluginMainQt.cpp:28:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebKit2/qt/PluginMainQt.cpp:31:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 7 in 18 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Balazs Kelemen]

The approach of the new patch was inspired by Simon's comment in #72961:
"Like I said in the previous comment, I'd prefer a non-IPC approach with a disk-cache. See also bug #43179 for an alternate approach of using an sqlite database as cache."

The patch does actually 2 think:
 - set up the plugin process for qt
 - move away all plugin loading to the plugin process

Without the second we cannot be safe from plugin crashes, that's why I did these in one step.

[Balazs Kelemen]

Comment on attachment 117617 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=117617&action=review

Self review :) - to help you reviewing the patch.

> Source/WebKit2/Shared/qt/ShareableBitmapQt.cpp:74
> +    if (scaleFactor == 1.0) {
> +        paint(context, dstPoint, srcRect);
> +        return;
> +    }
> +
> +    context.platformContext()->drawImage(dstPoint, createQImage(), QRect(srcRect));

That's wrong, I did not consider the scale factor. Will fix.

> Source/WebKit2/UIProcess/Plugins/unix/PluginProcessProxyUnix.cpp:71
> +#elif PLATFORM(GKT) && defined(LIBEXECDIR)
> +    // LIBEXECDIR is not defined when we build the static binary of the plugin process.
> +    const gchar* execDirectory = g_getenv("WEBKIT_EXEC_PATH");
> +    return CString(g_build_filename(execDirectory ? execDirectory : LIBEXECDIR, "WebKitPluginProcess");
> +#else

Acually that's not true, LIBEXECDIR is not neither defined when building the lib. I get this code from ProcessLauncherGtk.cpp. I couldn't figure out where this thing is coming from (even preprocessing ProcessLauncherGtk.cpp did not help), it's seems like it's some Gtk magic.

> Source/WebKit2/qt/PluginMainQt.cpp:3
> - * Copyright (C) 2011 Igalia S.L.
> + * Copyright (C) 2011 Apple Inc. All rights reserved.
> + * Copyright (C) 2011 University of Szeged. All rights reserved.

Don't worry about copirights, this is actually a new file but git diff messed it up with the one I removed.

[Gustavo Noronha (kov)]

Comment on attachment 117617 [details]
patch

Attachment 117617 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/10725419

[Carlos Garcia Campos]

(In reply to comment #16)
> > Source/WebKit2/UIProcess/Plugins/unix/PluginProcessProxyUnix.cpp:71
> > +#elif PLATFORM(GKT) && defined(LIBEXECDIR)
> > +    // LIBEXECDIR is not defined when we build the static binary of the plugin process.
> > +    const gchar* execDirectory = g_getenv("WEBKIT_EXEC_PATH");
> > +    return CString(g_build_filename(execDirectory ? execDirectory : LIBEXECDIR, "WebKitPluginProcess");
> > +#else
> 
> Acually that's not true, LIBEXECDIR is not neither defined when building the lib. I get this code from ProcessLauncherGtk.cpp. I couldn't figure out where this thing is coming from (even preprocessing ProcessLauncherGtk.cpp did not help), it's seems like it's some Gtk magic.

The "magic" has nothing to do with GTK, it's just a compiler command line parameter -DLIBEXECDIR=/foo/bar

[Balazs Kelemen]

Created attachment 118016 [details]
Patch

[Balazs Kelemen]

Created attachment 118025 [details]
Patch

[Balazs Kelemen]

(In reply to comment #20)
> Created an attachment (id=118025) [details]
> Patch

Fixed the mac build (hopefully).

[Simon Hausmann]

Comment on attachment 118025 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=118025&action=review

> Source/WebCore/platform/qt/FileSystemQt.cpp:223
> +    QByteArray local8Bit = static_cast<QString>(path).toLocal8Bit();

This should be written as

    QByteArray local8Bit = QFile::encodeName(static_Cast<QString>(path));

> Source/WebKit2/PluginProcess.pro:22
> +QT += network
> +macx: QT += xml

I think you can leave these two lines out. Let's add them it really turns out that we need them :)

> Source/WebKit2/PluginProcess.pro:27
> +contains(QT_CONFIG, opengl) {
> +    QT += opengl
> +    DEFINES += QT_CONFIGURED_WITH_OPENGL
> +}

This seems unnecessary as well for the plugin process.

> Source/WebKit2/Shared/Plugins/Netscape/x11/NetscapePluginModuleX11.cpp:235
> +    String baseName = pluginFileName + String::format(".%u", StringHash::hash(pluginPath)) + String(".bin");

One thing I don't like about the approach of having one meta-data file per plugin is that over time we end up accumulating garbage. If a plugin gets removed from the system, we never remove the .bin file.

I think it would be better to re-use the plugin meta-data cache code from WebCore/plugins/PluginDatabase.cpp, which collects all meta-data together in one file.

> Source/WebKit2/Shared/Plugins/Netscape/x11/NetscapePluginModuleX11.cpp:358
> +    QString dataLocation = QStandardPaths::writableLocation(QStandardPaths::DataLocation);
> +    if (dataLocation.isEmpty())
> +        dataLocation = pathByAppendingComponent(QDir::homePath(), QCoreApplication::applicationName());
> +    dataLocation = pathByAppendingComponent(dataLocation, ".QtWebKit");
> +    cacheDirectory = dataLocation;

This is me nitpicking: So we call pathByAppendingComponent, which takes a String and returns one. But we pass QStrings and have that also as return value.

I suggest to rewrite this block to either use String instead of QString more consistently or using Qt APIs all the way.

> Source/WebKit2/Shared/qt/ShareableBitmapQt.cpp:69
> +    if (scaleFactor == 1.0) {

I suggest to use qFuzzyCompare for comparing the float against an absolute value.

> Source/WebKit2/UIProcess/Plugins/unix/PluginProcessProxyUnix.cpp:45
> +void PluginProcessProxy::platformInitializePluginProcess(PluginProcessCreationParameters& parameters)

Unused parameter?

> Source/WebKit2/UIProcess/Plugins/unix/PluginProcessProxyUnix.cpp:54
> +    CString pluginPathASCII = pluginPath.ascii();
> +    CString filePathASCII = filePath.ascii();

Shuldn't this use fileSystemRepresentation() ?

> Source/WebKit2/WebProcess/Plugins/Netscape/NetscapePlugin.h:347
> +

Stray newline?

> Source/WebKit2/qt/PluginMainQt.cpp:39
> +    QLibrary gtkLibrary(QLatin1String("libgtk-x11-2.0"), 0);

Before you can use QLibrary you have to instantiate a QCoreApplication (at least) ;(

Perhaps the gtk initialization needs to be done at a later point then?

[Balazs Kelemen]

> One thing I don't like about the approach of having one meta-data file per plugin is that over time we end up accumulating garbage. If a plugin gets removed from the system, we never remove the .bin file.

This is not impossible to remove those files if there is no plugin associated wit h them. There are some minor advantages of having one file per plugin:
 - we do almost exactly the same as the Mac port
 - we can compare the modification date of the file and the plugin
 - fit well with how PluginInfoStore works currently

> I think it would be better to re-use the plugin meta-data cache code from WebCore/plugins/PluginDatabase.cpp, which collects all meta-data together in one file.
> 

Theoretically a lot of things could be reused from the wk1 plugin code but it has been chosen to reimplement the plugin system for wk2 so it's not trivial to reuse just one piece. 

Currently I am investigating in merging the two (and using just one cache file), I'm just a bit dubious about whether it's really worth to do.

[Balazs Kelemen]

Created attachment 119415 [details]
Patch

[Balazs Kelemen]

(In reply to comment #22)
> (From update of attachment 118025 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=118025&action=review
> 
> > Source/WebCore/platform/qt/FileSystemQt.cpp:223
> > +    QByteArray local8Bit = static_cast<QString>(path).toLocal8Bit();
> 
> This should be written as
> 
>     QByteArray local8Bit = QFile::encodeName(static_Cast<QString>(path));

Fixed.

> 
> > Source/WebKit2/PluginProcess.pro:22
> > +QT += network
> > +macx: QT += xml
> 
> I think you can leave these two lines out. Let's add them it really turns out that we need them :)
> 
> > Source/WebKit2/PluginProcess.pro:27
> > +contains(QT_CONFIG, opengl) {
> > +    QT += opengl
> > +    DEFINES += QT_CONFIGURED_WITH_OPENGL
> > +}
> 
> This seems unnecessary as well for the plugin process.

Fixed.

> 
> > Source/WebKit2/Shared/Plugins/Netscape/x11/NetscapePluginModuleX11.cpp:235
> > +    String baseName = pluginFileName + String::format(".%u", StringHash::hash(pluginPath)) + String(".bin");
> 
> One thing I don't like about the approach of having one meta-data file per plugin is that over time we end up accumulating garbage. If a plugin gets removed from the system, we never remove the .bin file.
> 
> I think it would be better to re-use the plugin meta-data cache code from WebCore/plugins/PluginDatabase.cpp, which collects all meta-data together in one file.

I did not change on that. I started to implement it with only one file but I did not like that. At the plugin process side we need to append the content and the UI process need to read the file again and again. Also it's pretty confusing how to behave on IO error - which can happen in both processes - or data corruption. With one file per plugin we never write the files at the UI side and we don't need to regenerate everything if the data for a particular plugin is out-of-date or has been corrupted. Also, I don't think we should merge it with the WebCore implementation. It would make thinks overcomplicated as the WK2 plugin system is diverging too much from WebCore.

> > Source/WebKit2/Shared/Plugins/Netscape/x11/NetscapePluginModuleX11.cpp:358
> > +    QString dataLocation = QStandardPaths::writableLocation(QStandardPaths::DataLocation);
> > +    if (dataLocation.isEmpty())
> > +        dataLocation = pathByAppendingComponent(QDir::homePath(), QCoreApplication::applicationName());
> > +    dataLocation = pathByAppendingComponent(dataLocation, ".QtWebKit");
> > +    cacheDirectory = dataLocation;
> 
> This is me nitpicking: So we call pathByAppendingComponent, which takes a String and returns one. But we pass QStrings and have that also as return value.

Fixed.

> 
> I suggest to rewrite this block to either use String instead of QString more consistently or using Qt APIs all the way.
> 
> > Source/WebKit2/Shared/qt/ShareableBitmapQt.cpp:69
> > +    if (scaleFactor == 1.0) {
> 
> I suggest to use qFuzzyCompare for comparing the float against an absolute value.

Fixed.

> 
> > Source/WebKit2/UIProcess/Plugins/unix/PluginProcessProxyUnix.cpp:45
> > +void PluginProcessProxy::platformInitializePluginProcess(PluginProcessCreationParameters& parameters)
> 
> Unused parameter?

Fixed.

> 
> > Source/WebKit2/UIProcess/Plugins/unix/PluginProcessProxyUnix.cpp:54
> > +    CString pluginPathASCII = pluginPath.ascii();
> > +    CString filePathASCII = filePath.ascii();
> 
> Shuldn't this use fileSystemRepresentation() ?

Fixed.

> 
> > Source/WebKit2/WebProcess/Plugins/Netscape/NetscapePlugin.h:347
> > +
> 
> Stray newline?

Fixed.

> 
> > Source/WebKit2/qt/PluginMainQt.cpp:39
> > +    QLibrary gtkLibrary(QLatin1String("libgtk-x11-2.0"), 0);
> 
> Before you can use QLibrary you have to instantiate a QCoreApplication (at least) ;(
> 
> Perhaps the gtk initialization needs to be done at a later point then?

Fixed.

[Balazs Kelemen]

Created attachment 122217 [details]
fixed win build

[Balazs Kelemen]

Comment on attachment 122217 [details]
fixed win build

Need to update and split up.

[Balazs Kelemen]

Created attachment 128204 [details]
patch

Only the process setup part, no caching of meta data. Plugin scanning has been implemented by reading stdout of plugin process via QProcess. It's still a WIP version since I did not test on Gtk because I have some build issues.

[Balazs Kelemen]

Created attachment 128332 [details]
Patch

[Balazs Kelemen]

*** Bug 72961 has been marked as a duplicate of this bug. ***

[Simon Hausmann]

Comment on attachment 128332 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=128332&action=review

A couple of nitpicks, but otherwise this looks good to me. Let's do one last round :)

> Source/WebKit2/PluginProcess.pro:7
> +load(features)

Is this needed?

> Source/WebKit2/PluginProcess/gtk/PluginProcessMainGtk.cpp:61
> +    fprintf(stderr, "PP running\nargs=%s,%s,%s\n", argv[0], argv[1], argv[2]);

If arc2 == 2, which seems like a valid condition, the accessing argv[2] here seems like an out-of-bounds array access.

> Source/WebKit2/PluginProcess/qt/PluginProcessMainQt.cpp:97
> +        // QProcess does not allow to read the output of the process after
> +        // it has been terminated. Let's go to sleep and rely on the UI
> +        // process to kill us.
> +        sleep(10);

I don't understand how this is needed. The documentation of the QProcess finished signal explains that the buffer should still be intact. Is there perhaps an fflush(stdout); or so missing here?

> Source/WebKit2/Shared/Plugins/Netscape/x11/NetscapePluginModuleX11.cpp:153
> +    return adoptPtr(truncated);

I think this should be adoptArrayPtr.

> Source/WebKit2/Shared/Plugins/Netscape/x11/NetscapePluginModuleX11.cpp:172
> +    OwnPtr<char> name(truncateNewLines(metaData.name.ascii().data()));

Wait, truncateNewLines takes a String() as argument, but we're passing a const char * as parameter, so this is implicitly constructing a String. Shouldn't truncateNewLines take a const char* instead then?

> Source/WebKit2/Shared/qt/ShareableBitmapQt.cpp:80
> +    if (qFuzzyCompare(scaleFactor, 1)) {
> +        paint(context, dstPoint, srcRect);
> +        return;
> +    }

Hm, my memory is weak here, but doesn't this belong into a separate patch? :)

> Source/WebKit2/UIProcess/Plugins/qt/PluginProcessProxyQt.cpp:63
> +    QByteArray output = process->readAll();

Perhaps this could be simplified by using the patter that you can also find in the QProcess auto-tests:

process.start();
process.waitForStarted();
if (process.waitForFinished()) {
    // success, read all stdout
} else
    // Fail, probably crashed.

[Balazs Kelemen]

Created attachment 130813 [details]
Patch

[Balazs Kelemen]

(In reply to comment #31)
> (From update of attachment 128332 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=128332&action=review
> 
> A couple of nitpicks, but otherwise this looks good to me. Let's do one last round :)
> 
> > Source/WebKit2/PluginProcess.pro:7
> > +load(features)
> 
> Is this needed?

No, removed.

> 
> > Source/WebKit2/PluginProcess/gtk/PluginProcessMainGtk.cpp:61
> > +    fprintf(stderr, "PP running\nargs=%s,%s,%s\n", argv[0], argv[1], argv[2]);
> 
> If arc2 == 2, which seems like a valid condition, the accessing argv[2] here seems like an out-of-bounds array access.

Well, it just left over from debugging. Removed.

> > Source/WebKit2/PluginProcess/qt/PluginProcessMainQt.cpp:97
> > +        // QProcess does not allow to read the output of the process after
> > +        // it has been terminated. Let's go to sleep and rely on the UI
> > +        // process to kill us.
> > +        sleep(10);
> 
> I don't understand how this is needed. The documentation of the QProcess finished signal explains that the buffer should still be intact. Is there perhaps an fflush(stdout); or so missing here?

Right, by using waitForFinished() I could remove this. Fixed.

> 
> > Source/WebKit2/Shared/Plugins/Netscape/x11/NetscapePluginModuleX11.cpp:153
> > +    return adoptPtr(truncated);
> 
> I think this should be adoptArrayPtr.
> 
> > Source/WebKit2/Shared/Plugins/Netscape/x11/NetscapePluginModuleX11.cpp:172
> > +    OwnPtr<char> name(truncateNewLines(metaData.name.ascii().data()));
> 
> Wait, truncateNewLines takes a String() as argument, but we're passing a const char * as parameter, so this is implicitly constructing a String. Shouldn't truncateNewLines take a const char* instead then?

Both are true. However I realized that I was totally unaware of the fact that the meta data is UTF8 (since http://trac.webkit.org/changeset/108281). So I had to rework this in order to pass the unicode data without conversion.

> 
> > Source/WebKit2/Shared/qt/ShareableBitmapQt.cpp:80
> > +    if (qFuzzyCompare(scaleFactor, 1)) {
> > +        paint(context, dstPoint, srcRect);
> > +        return;
> > +    }
> 
> Hm, my memory is weak here, but doesn't this belong into a separate patch? :)

Without this you won't see anything from the plugin, so I think it's better to add this here so we can make sure that this stuff is actually working :)

> 
> > Source/WebKit2/UIProcess/Plugins/qt/PluginProcessProxyQt.cpp:63
> > +    QByteArray output = process->readAll();
> 
> Perhaps this could be simplified by using the patter that you can also find in the QProcess auto-tests:
> 
> process.start();
> process.waitForStarted();
> if (process.waitForFinished()) {
>     // success, read all stdout
> } else
>     // Fail, probably crashed.

Done.

[WebKit Review Bot]

Attachment 130813 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'Source/QtWebKit.pro', u'Sour..." exit_code: 1
Source/WebKit2/Shared/Plugins/Netscape/x11/NetscapePluginModuleX11.cpp:161:  Missing space before ( in while(  [whitespace/parens] [5]
Source/WebKit2/Shared/Plugins/Netscape/x11/NetscapePluginModuleX11.cpp:193:  Missing space before ( in while(  [whitespace/parens] [5]
Total errors found: 2 in 21 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Balazs Kelemen]

Created attachment 130819 [details]
Patch

[Simon Hausmann]

Comment on attachment 130819 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=130819&action=review

r=me with comments. A few things that should be fixed before landing. The QProcess one could be done in a separate patch or you could fix up this one and upload a new version of this patch. Whichever you prefer :)

> Source/WebKit2/PluginProcess.pro:17
> +CONFIG += qtwebkit

Note that when the fix for bug #80590 lands, this has to be written as QT += webkit instead.

> Source/WebKit2/PluginProcess/qt/PluginProcessMainQt.cpp:75
> +    QApplication app(argc, argv);

In the future we should change this to be QGuiApplication. At the latest as part of the fix for bug #79458.

> Source/WebKit2/Shared/Plugins/Netscape/x11/NetscapePluginModuleX11.cpp:146
> +    unsigned newLenght = 0;

newLenght -> newLength :)

> Source/WebKit2/Shared/Plugins/Netscape/x11/NetscapePluginModuleX11.cpp:163
> +static inline void putCharToStdOut(char c)
> +{
> +    int result;
> +    while ((result = fputc(c, stdout)) == EOF && errno == EINTR) { }
> +    ASSERT(result != EOF);
> +}

This function seems unused. Looks like an earlier left-over that could be removed before landing.

> Source/WebKit2/UIProcess/Plugins/qt/PluginProcessProxyQt.cpp:64
> +    QProcess* process = new QProcess;
> +    QObject::connect(process, SIGNAL(finished(int)), process, SLOT(deleteLater()), Qt::QueuedConnection);
> +    process->setReadChannel(QProcess::StandardOutput);
> +    process->start(commandLine);
> +
> +    if (!process->waitForFinished()
> +        || process->exitStatus() != QProcess::NormalExit
> +        || process->exitCode() != EXIT_SUCCESS) {
> +        process->kill();
> +        return false;
> +    }
> +

Since you now deal with the process synchronously, I think you can safely allocate the process object on the stack and get rid of the finished -> deleteLater connection.

[Tor Arne Vestbø]

Comment on attachment 130819 [details]
Patch

This patch needs a slight tweak after the Qt module patch was landed. See https://bugs.webkit.org/show_bug.cgi?id=80590#c14

[Carlos Garcia Campos]

Comment on attachment 130819 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=130819&action=review

> Source/WebKit2/GNUmakefile.am:1480
> +	Source/WebKit2/UIProcess/Launcher/ProcessLauncher.cpp \
> +	Source/WebKit2/UIProcess/Launcher/gtk/ProcessLauncherGtk.cpp \
> +	Source/WebKit2/UIProcess/Plugins/gtk/PluginProcessProxyGtk.cpp \

It's a bit weird that the plugin process needs sources of the UI process. It needs the Launcher but it doesn't use it to launch a process, just to get the executable path of the plugin process. Maybe we could move ProcessType stuff to Shared with the method to get the path of the process there, and Launcher would use it from Shared too.

> Source/WebKit2/UIProcess/Plugins/gtk/PluginProcessProxyGtk.cpp:61
> +    if (!g_spawn_sync(0, argv, 0, G_SPAWN_STDERR_TO_DEV_NULL, 0, 0, &stdOut, 0, &status, &error.outPtr()))
> +        return false;

Your are not handling errors, you should either show a warning message if it failed or just pass NULL instead of error.

[Balazs Kelemen]

Comment on attachment 130819 [details]
Patch

That's enough for another iteration.

[Balazs Kelemen]

Created attachment 131033 [details]
Patch

[Balazs Kelemen]

(In reply to comment #38)
> (From update of attachment 130819 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=130819&action=review
> 
> > Source/WebKit2/GNUmakefile.am:1480
> > +	Source/WebKit2/UIProcess/Launcher/ProcessLauncher.cpp \
> > +	Source/WebKit2/UIProcess/Launcher/gtk/ProcessLauncherGtk.cpp \
> > +	Source/WebKit2/UIProcess/Plugins/gtk/PluginProcessProxyGtk.cpp \

Fixed. The PluginProcessProxy still need to be added for linking.
I could move the scanPlugin function into NetscapePluginModuleX11.cpp but it would not change on the fact that we need a function to link in that we do not actually use in the plugin process. I think it's somewhat nicer to go through the PluginProcessProxy, this is what he is for.

Fixed the all other comments.

[Gustavo Noronha (kov)]

Comment on attachment 131033 [details]
Patch

Attachment 131033 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11915545

[Collabora GTK+ EWS bot]

Comment on attachment 131033 [details]
Patch

Attachment 131033 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11903610

[Balazs Kelemen]

Created attachment 131043 [details]
Patch

[Gustavo Noronha (kov)]

Comment on attachment 131043 [details]
Patch

Attachment 131043 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11906682

[Balazs Kelemen]

Created attachment 131242 [details]
Patch

[WebKit Review Bot]

Attachment 131242 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'Source/QtWebKit.pro', u'Sour..." exit_code: 1
Source/WebKit2/Shared/gtk/ProcessExecutablePathGtk.cpp:28:  You should add a blank line after implementation file's own header.  [build/include_order] [4]
Source/WebKit2/Shared/gtk/ProcessExecutablePathGtk.cpp:30:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebKit2/UIProcess/Plugins/gtk/PluginProcessProxyGtk.cpp:38:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 3 in 24 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Balazs Kelemen]

Created attachment 131243 [details]
Patch

[Balazs Kelemen]

Comment on attachment 131243 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=131243&action=review

> Tools/MiniBrowser/gtk/GNUmakefile.am:35
> -	$(GTK_LIBS)
> +	$(GTK_LIBS) \
> +	$(LIBSOUP_LIBS)

This had to be added unless MiniBrowser doesn't link. Honestly I don't understand where does this dependency came from. In fact GtkLauncher also has this so maybe it's ok.

[Balazs Kelemen]

Landed in http://trac.webkit.org/changeset/112868