WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
72121
[Qt][WK2] Set up plugin process on Unix
https://bugs.webkit.org/show_bug.cgi?id=72121
Summary
[Qt][WK2] Set up plugin process on Unix
Balazs Kelemen
Reported
2011-11-11 05:53:00 PST
We already have the basic infrastructure, so let's make it alive.
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
Show Obsolete
(13)
View All
Add attachment
proposed patch, testcase, etc.
Balazs Kelemen
Comment 1
2011-11-11 06:45:10 PST
Created
attachment 114692
[details]
Patch
Simon Hausmann
Comment 2
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 :)
Balazs Kelemen
Comment 3
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.
Balazs Kelemen
Comment 4
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.
Simon Hausmann
Comment 5
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).
Simon Hausmann
Comment 6
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 :)
Balazs Kelemen
Comment 7
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.
Balazs Kelemen
Comment 8
2011-11-24 10:07:43 PST
Adding a GTK expert because we need GTK knowleadge here.
Balazs Kelemen
Comment 9
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.
Carlos Garcia Campos
Comment 10
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.
Balazs Kelemen
Comment 11
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.
Balazs Kelemen
Comment 12
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.
Balazs Kelemen
Comment 13
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.
WebKit Review Bot
Comment 14
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.
Balazs Kelemen
Comment 15
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.
Balazs Kelemen
Comment 16
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.
Gustavo Noronha (kov)
Comment 17
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
Carlos Garcia Campos
Comment 18
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
Balazs Kelemen
Comment 19
2011-12-06 03:17:02 PST
Created
attachment 118016
[details]
Patch
Balazs Kelemen
Comment 20
2011-12-06 04:36:16 PST
Created
attachment 118025
[details]
Patch
Balazs Kelemen
Comment 21
2011-12-06 04:37:36 PST
(In reply to
comment #20
)
> Created an attachment (id=118025) [details] > Patch
Fixed the mac build (hopefully).
Simon Hausmann
Comment 22
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?
Balazs Kelemen
Comment 23
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.
Balazs Kelemen
Comment 24
2011-12-15 06:05:09 PST
Created
attachment 119415
[details]
Patch
Balazs Kelemen
Comment 25
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.
Balazs Kelemen
Comment 26
2012-01-12 05:05:48 PST
Created
attachment 122217
[details]
fixed win build
Balazs Kelemen
Comment 27
2012-02-22 06:10:26 PST
Comment on
attachment 122217
[details]
fixed win build Need to update and split up.
Balazs Kelemen
Comment 28
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.
Balazs Kelemen
Comment 29
2012-02-22 16:48:43 PST
Created
attachment 128332
[details]
Patch
Balazs Kelemen
Comment 30
2012-02-28 02:30:35 PST
***
Bug 72961
has been marked as a duplicate of this bug. ***
Simon Hausmann
Comment 31
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.
Balazs Kelemen
Comment 32
2012-03-08 06:11:56 PST
Created
attachment 130813
[details]
Patch
Balazs Kelemen
Comment 33
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.
WebKit Review Bot
Comment 34
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.
Balazs Kelemen
Comment 35
2012-03-08 06:49:41 PST
Created
attachment 130819
[details]
Patch
Simon Hausmann
Comment 36
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.
Tor Arne Vestbø
Comment 37
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
Carlos Garcia Campos
Comment 38
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.
Balazs Kelemen
Comment 39
2012-03-09 01:42:22 PST
Comment on
attachment 130819
[details]
Patch That's enough for another iteration.
Balazs Kelemen
Comment 40
2012-03-09 05:36:40 PST
Created
attachment 131033
[details]
Patch
Balazs Kelemen
Comment 41
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.
Gustavo Noronha (kov)
Comment 42
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
Collabora GTK+ EWS bot
Comment 43
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
Balazs Kelemen
Comment 44
2012-03-09 08:56:16 PST
Created
attachment 131043
[details]
Patch
Gustavo Noronha (kov)
Comment 45
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
Balazs Kelemen
Comment 46
2012-03-11 10:18:52 PDT
Created
attachment 131242
[details]
Patch
WebKit Review Bot
Comment 47
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.
Balazs Kelemen
Comment 48
2012-03-11 10:24:10 PDT
Created
attachment 131243
[details]
Patch
Balazs Kelemen
Comment 49
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.
Balazs Kelemen
Comment 50
2012-04-02 06:10:03 PDT
Landed in
http://trac.webkit.org/changeset/112868
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug