Bug 72121 - [Qt][WK2] Set up plugin process on Unix
Summary: [Qt][WK2] Set up plugin process on Unix
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Plug-ins (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Balazs Kelemen
URL:
Keywords: Qt, QtTriaged
: 72961 (view as bug list)
Depends on: 82901
Blocks:
  Show dependency treegraph
 
Reported: 2011-11-11 05:53 PST by Balazs Kelemen
Modified: 2012-04-02 09:52 PDT (History)
12 users (show)

See Also:


Attachments
Patch (11.76 KB, text/plain)
2011-11-11 06:45 PST, Balazs Kelemen
no flags Details
patch (36.08 KB, patch)
2011-12-02 07:01 PST, Balazs Kelemen
no flags Details | Formatted Diff | Diff
Patch (53.68 KB, patch)
2011-12-06 03:17 PST, Balazs Kelemen
no flags Details | Formatted Diff | Diff
Patch (53.67 KB, patch)
2011-12-06 04:36 PST, Balazs Kelemen
no flags Details | Formatted Diff | Diff
Patch (59.04 KB, patch)
2011-12-15 06:05 PST, Balazs Kelemen
no flags Details | Formatted Diff | Diff
fixed win build (59.16 KB, patch)
2012-01-12 05:05 PST, Balazs Kelemen
no flags Details | Formatted Diff | Diff
patch (38.35 KB, patch)
2012-02-22 06:49 PST, Balazs Kelemen
no flags Details | Formatted Diff | Diff
Patch (40.71 KB, patch)
2012-02-22 16:48 PST, Balazs Kelemen
no flags Details | Formatted Diff | Diff
Patch (40.77 KB, patch)
2012-03-08 06:11 PST, Balazs Kelemen
no flags Details | Formatted Diff | Diff
Patch (40.78 KB, patch)
2012-03-08 06:49 PST, Balazs Kelemen
no flags Details | Formatted Diff | Diff
Patch (49.64 KB, patch)
2012-03-09 05:36 PST, Balazs Kelemen
no flags Details | Formatted Diff | Diff
Patch (49.85 KB, patch)
2012-03-09 08:56 PST, Balazs Kelemen
no flags Details | Formatted Diff | Diff
Patch (50.89 KB, patch)
2012-03-11 10:18 PDT, Balazs Kelemen
no flags Details | Formatted Diff | Diff
Patch (50.88 KB, patch)
2012-03-11 10:24 PDT, Balazs Kelemen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Balazs Kelemen 2011-11-11 05:53:00 PST
We already have the basic infrastructure, so let's make it alive.
Comment 1 Balazs Kelemen 2011-11-11 06:45:10 PST
Created attachment 114692 [details]
Patch
Comment 2 Simon Hausmann 2011-11-11 07:32:49 PST
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 :)
Comment 3 Balazs Kelemen 2011-11-11 07:46:15 PST
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.
Comment 4 Balazs Kelemen 2011-11-14 01:27:17 PST
(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.
Comment 5 Simon Hausmann 2011-11-14 02:47:39 PST
(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).
Comment 6 Simon Hausmann 2011-11-14 05:07:13 PST
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 :)
Comment 7 Balazs Kelemen 2011-11-14 06:18:12 PST
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.
Comment 8 Balazs Kelemen 2011-11-24 10:07:43 PST
Adding a GTK expert because we need GTK knowleadge here.
Comment 9 Balazs Kelemen 2011-11-24 10:09:58 PST
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.
Comment 10 Carlos Garcia Campos 2011-11-24 10:42:27 PST
(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.
Comment 11 Balazs Kelemen 2011-11-25 04:03:09 PST
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.
Comment 12 Balazs Kelemen 2011-11-25 10:26:57 PST
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.
Comment 13 Balazs Kelemen 2011-12-02 07:01:22 PST
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.
Comment 14 WebKit Review Bot 2011-12-02 07:03:47 PST
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.
Comment 15 Balazs Kelemen 2011-12-02 07:06:32 PST
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.
Comment 16 Balazs Kelemen 2011-12-02 07:21:42 PST
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.
Comment 17 Gustavo Noronha (kov) 2011-12-02 09:06:39 PST
Comment on attachment 117617 [details]
patch

Attachment 117617 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/10725419
Comment 18 Carlos Garcia Campos 2011-12-04 01:31:54 PST
(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
Comment 19 Balazs Kelemen 2011-12-06 03:17:02 PST
Created attachment 118016 [details]
Patch
Comment 20 Balazs Kelemen 2011-12-06 04:36:16 PST
Created attachment 118025 [details]
Patch
Comment 21 Balazs Kelemen 2011-12-06 04:37:36 PST
(In reply to comment #20)
> Created an attachment (id=118025) [details]
> Patch

Fixed the mac build (hopefully).
Comment 22 Simon Hausmann 2011-12-12 03:54:36 PST
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?
Comment 23 Balazs Kelemen 2011-12-13 10:27:03 PST
> 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.
Comment 24 Balazs Kelemen 2011-12-15 06:05:09 PST
Created attachment 119415 [details]
Patch
Comment 25 Balazs Kelemen 2011-12-15 06:17:25 PST
(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.
Comment 26 Balazs Kelemen 2012-01-12 05:05:48 PST
Created attachment 122217 [details]
fixed win build
Comment 27 Balazs Kelemen 2012-02-22 06:10:26 PST
Comment on attachment 122217 [details]
fixed win build

Need to update and split up.
Comment 28 Balazs Kelemen 2012-02-22 06:49:13 PST
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.
Comment 29 Balazs Kelemen 2012-02-22 16:48:43 PST
Created attachment 128332 [details]
Patch
Comment 30 Balazs Kelemen 2012-02-28 02:30:35 PST
*** Bug 72961 has been marked as a duplicate of this bug. ***
Comment 31 Simon Hausmann 2012-03-06 23:20:21 PST
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.
Comment 32 Balazs Kelemen 2012-03-08 06:11:56 PST
Created attachment 130813 [details]
Patch
Comment 33 Balazs Kelemen 2012-03-08 06:12:26 PST
(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.
Comment 34 WebKit Review Bot 2012-03-08 06:15:03 PST
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.
Comment 35 Balazs Kelemen 2012-03-08 06:49:41 PST
Created attachment 130819 [details]
Patch
Comment 36 Simon Hausmann 2012-03-08 11:38:41 PST
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.
Comment 37 Tor Arne Vestbø 2012-03-08 23:50:46 PST
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
Comment 38 Carlos Garcia Campos 2012-03-09 00:07:33 PST
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.
Comment 39 Balazs Kelemen 2012-03-09 01:42:22 PST
Comment on attachment 130819 [details]
Patch

That's enough for another iteration.
Comment 40 Balazs Kelemen 2012-03-09 05:36:40 PST
Created attachment 131033 [details]
Patch
Comment 41 Balazs Kelemen 2012-03-09 05:40:55 PST
(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.
Comment 42 Gustavo Noronha (kov) 2012-03-09 08:00:30 PST
Comment on attachment 131033 [details]
Patch

Attachment 131033 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11915545
Comment 43 Collabora GTK+ EWS bot 2012-03-09 08:09:57 PST
Comment on attachment 131033 [details]
Patch

Attachment 131033 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11903610
Comment 44 Balazs Kelemen 2012-03-09 08:56:16 PST
Created attachment 131043 [details]
Patch
Comment 45 Gustavo Noronha (kov) 2012-03-09 11:04:18 PST
Comment on attachment 131043 [details]
Patch

Attachment 131043 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11906682
Comment 46 Balazs Kelemen 2012-03-11 10:18:52 PDT
Created attachment 131242 [details]
Patch
Comment 47 WebKit Review Bot 2012-03-11 10:21:26 PDT
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.
Comment 48 Balazs Kelemen 2012-03-11 10:24:10 PDT
Created attachment 131243 [details]
Patch
Comment 49 Balazs Kelemen 2012-03-11 10:26:37 PDT
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.
Comment 50 Balazs Kelemen 2012-04-02 06:10:03 PDT
Landed in http://trac.webkit.org/changeset/112868