Bug 55719 – [Qt][WK2] Make Qt port compiling with ENABLE_PLUGIN_PROCESS=1

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

[Oleg Romashin (:romaxa)]

It is a bit problematic to compile Qt port now with PLUGIN_PROCESS enabled, bunch of file contain not properly ifdefed CoreIPC::MachPort, .pro files don't compile some certain files et.c.
Some parts need dummy implementation with notImplemented() function in "qt" folders

[Oleg Romashin (:romaxa)]

Bug 55658 - is needed to ifdef MachPort related functionality in *.messages.in files

[Oleg Romashin (:romaxa)]

Created attachment 84703 [details]
Works fine with latest webkit, need to check other platforms WIP1

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit.pri', u'Source/WebKit2/Deriv..." exit_code: 1

Source/WebKit2/PluginProcess/qt/PluginProcessQt.cpp:34:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebKit2/WebProcess/Plugins/Netscape/qt/PluginProxyQt.cpp:27:  You should add a blank line after implementation file's own header.  [build/include_order] [4]
Source/WebKit2/WebProcess/Plugins/Netscape/qt/PluginProxyQt.cpp:38:  Use 0 instead of NULL.  [readability/null] [5]
Source/WebKit2/PluginProcess/qt/PluginProcessMainQt.cpp:38:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 4 in 24 files


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

[Kenneth Rohde Christiansen]

Comment on attachment 84703 [details]
Works fine with latest webkit, need to check other platforms WIP1

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

Where are the changelogs? Did you run check-webkit-style and prepare-ChangeLog? Please consult the wikipages on how to contribute to WebKit

> Source/WebKit.pri:76
> +webkit2:INCLUDEPATH *= $$OUTPUT_DIR/include/WebKit2 $$OUTPUT_DIR/WebKit2/generated

Don't we have these generated included somewhere else?

> Source/WebKit2/PluginProcess/PluginControllerProxy.cpp:258
> +#if PLATFORM(MAC)
>  void PluginControllerProxy::setComplexTextInputEnabled(bool complexTextInputEnabled)

So we will not support input methods? Actuallu you are saying that no other platform that Mac will support this. I guess this should be #if !PLATFORM(QT) instead

> Source/WebKit2/PluginProcess/PluginProcess.cpp:133
>  
>  void PluginProcess::createWebProcessConnection()
>  {
> +#if PLATFORM(MAC)
>      // FIXME: This is platform specific!

Maybe this should be integrated into a PluginProcessMac instead?

> Source/WebKit2/Scripts/webkit2/messages.py:454
> +    headers = {
> +        '"%s"' % messages_header_filename(receiver): None,
> +        '"HandleMessage.h"': None,

This patch is very hard to review as there is no changelog explaining why you are making these changes...

[Oleg Romashin (:romaxa)]

> Where are the changelogs? Did you run check-webkit-style and prepare-ChangeLog? Please consult the wikipages on how to contribute to WebKit
> 
sorry, I'm just trying to check this patch on other platforms, so that is why it is marked as WIP

> > +webkit2:INCLUDEPATH *= $$OUTPUT_DIR/include/WebKit2 $$OUTPUT_DIR/WebKit2/generated
> 
> Don't we have these generated included somewhere else?

By some reason no, I did not get why it does not work, and found only this way to make it works, if you have any ideas what other place better for this include please share.

> > +#if PLATFORM(MAC)
> >  void PluginControllerProxy::setComplexTextInputEnabled(bool complexTextInputEnabled)
> 
> So we will not support input methods? Actuallu you are saying that no other platform that Mac will support this. I guess this should be #if !PLATFORM(QT) instead


Not sure, I can make Qt method with notImplemented  entry, probably it make more sense for future development

> > Source/WebKit2/PluginProcess/PluginProcess.cpp:133
> >  
> >  void PluginProcess::createWebProcessConnection()
> >  {
> > +#if PLATFORM(MAC)
> >      // FIXME: This is platform specific!
> 
> Maybe this should be integrated into a PluginProcessMac instead?

Not sure about this stuff yet, just got separate process create, but did not get where and how these specific things need to be initialized

> 
> > Source/WebKit2/Scripts/webkit2/messages.py:454
> > +    headers = {
> > +        '"%s"' % messages_header_filename(receiver): None,
> > +        '"HandleMessage.h"': None,
> 
> This patch is very hard to review as there is no changelog explaining why you are making these changes...

messages.py part already attached for review in separate dependent bug 55658 ....

that is about teaching IPC stub generator to use ifdefs for forward headers related to ifdef-ed methods

[Oleg Romashin (:romaxa)]

anyway thanks for WIP review

[Oleg Romashin (:romaxa)]

Created attachment 84790 [details]
WIP2 fixed style issues

[Kenneth Rohde Christiansen]

Comment on attachment 84790 [details]
WIP2 fixed style issues

Changelogs are mandatory, please use the Tools/Scripts/prepare-ChangeLog script

[Oleg Romashin (:romaxa)]

Created attachment 84796 [details]
Part1, dummy Qt implementation, Makefiles

[Oleg Romashin (:romaxa)]

Created attachment 84797 [details]
Part1, dummy Qt implementation, Makefiles

[Kenneth Rohde Christiansen]

Comment on attachment 84797 [details]
Part1, dummy Qt implementation, Makefiles

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

Seems pretty OK to me as a start.

> Source/WebKit2/PluginProcess/qt/PluginProcessMainQt.cpp:47
> +    notImplemented();
> +

Things are implemented in this method

> Source/WebKit2/PluginProcess/qt/PluginProcessMainQt.cpp:52
> +    String localization = commandLine["localization"];

I do not see what that does here. It is not used anywhere

[Oleg Romashin (:romaxa)]

Created attachment 84835 [details]
Fixed review comments

[Oleg Romashin (:romaxa)]

I hope it was ok to move r+ to updated patch...

[Kenneth Rohde Christiansen]

(In reply to comment #13)
> I hope it was ok to move r+ to updated patch...

It is not. You can manually fill out the reviewer field and just add cq?

[WebKit Commit Bot]

Comment on attachment 84835 [details]
Fixed review comments

Clearing flags on attachment: 84835

Committed r80434: <http://trac.webkit.org/changeset/80434>

[Oleg Romashin (:romaxa)]

Created attachment 84899 [details]
Part2, fixed sendComplexInput

[Build Bot]

Attachment 84899 [details] did not build on win:
Build output: http://queues.webkit.org/results/8107161

[Oleg Romashin (:romaxa)]

Created attachment 84901 [details]
Added dummy implementation for Gtk and Win port

[Oleg Romashin (:romaxa)]

Comment on attachment 84790 [details]
WIP2 fixed style issues

this is obsolete now

[Build Bot]

Attachment 84901 [details] did not build on win:
Build output: http://queues.webkit.org/results/8105317

[Oleg Romashin (:romaxa)]

Created attachment 84904 [details]
Added dummy implementation for Gtk and Win port + Fix 3 unresolved externals on win port

[Build Bot]

Attachment 84904 [details] did not build on win:
Build output: http://queues.webkit.org/results/8103423

[Oleg Romashin (:romaxa)]

Created attachment 84907 [details]
Added dummy implementation for Gtk and Win port + Fix 3 unresolved externals on win port.v4

[Oleg Romashin (:romaxa)]

Comment on attachment 84907 [details]
Added dummy implementation for Gtk and Win port + Fix 3 unresolved externals on win port.v4

Ok, this version builds fine on win port now.

[Balazs Kelemen]

Comment on attachment 84907 [details]
Added dummy implementation for Gtk and Win port + Fix 3 unresolved externals on win port.v4

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

Informal review from a non-reviewer.

Generally I thing we should first design how to make things more portable.
Btw, why do you think all platforms need the complex text stuff?
It seems to be pretty Mac specific.

> Source/WebKit2/PluginProcess/PluginProcess.cpp:135
> -    // FIXME: This is platform specific!
> -
> +#if PLATFORM(MAC)
>      // Create the listening port.
>      mach_port_t listeningPort;
>      mach_port_allocate(mach_task_self(), MACH_PORT_RIGHT_RECEIVE, &listeningPort);

This function should rather be factored to be a platform specific one that is implemented per platform...

> Source/WebKit2/PluginProcess/PluginProcess.cpp:148
> +#else
> +    ASSERT(false);
> +#endif

and than this should be removed.

> Source/WebKit2/UIProcess/Plugins/PluginProcessProxy.cpp:131
> +#else
> +        ASSERT(false);
> +#endif

Use ASSERT_NOT_REACHED(). It's an open question however how to abstract an empty message in a platform independent way.
Maybe we should solve this before actually adding even a stub implementation for plugin process.

> Source/WebKit2/UIProcess/Plugins/PluginProcessProxy.cpp:212
>      CoreIPC::ArgumentEncoder* reply = m_pendingConnectionReplies.first().second;
>      m_pendingConnectionReplies.removeFirst();
>  
> -    // FIXME: This is Mac specific.
>      reply->encode(CoreIPC::MachPort(machPort.port(), MACH_MSG_TYPE_MOVE_SEND));
> +
>      replyWebProcessProxy->connection()->sendSyncReply(reply);
>  }
> +#endif

Similar issue. It is not good having this to be implemented per platform just because of the MachPort argument.

[Oleg Romashin (:romaxa)]

(In reply to comment #25)
> (From update of attachment 84907 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=84907&action=review
> 
> Informal review from a non-reviewer.
> 
> Generally I thing we should first design how to make things more portable.
> Btw, why do you think all platforms need the complex text stuff?
> It seems to be pretty Mac specific.

Not sure about it in 55719#c4, I got suggestion about adding this for non-mac platforms (originally I just moved one entry under MAC define), I'm not mac developer, can we get some understanding is it pure mac specific stuff or is it possible to implement it for other platforms?


> Maybe we should solve this before actually adding even a stub implementation for plugin process.

Goal here is not to implement anything, but make this dummy stub compilable, with assert entries which should help us to not skip important implementations

> 
> Similar issue. It is not good having this to be implemented per platform just because of the MachPort argument.

Not sure what is the right way to fix this problem... do you have any suggestions? Wrap argument into platform-independent structure?

[Oleg Romashin (:romaxa)]

Created attachment 85008 [details]
Fixed some review comments,

moved createWebProcessConnection to platform specific implementation, fixed changed to assert_not_reached
asked kling about ComplexTextInput and he proposed to keep that Qt IME implementation.

m_connection->send(Messages::PluginProcessProxy::DidCreateWebProcessConnection(clientPort), 0); - is called only from PluginProcess::createWebProcessConnection, which is not implemented yet in Qt port.
lets keep DidCreateWebProcessConnection mac only for now, and change that when some implementation for Qt createWebProcessConnection is done.

[Oleg Romashin (:romaxa)]

Created attachment 85029 [details]
Qt Xlib includes conflict fix, Fixed include order, QTBUG-54

A bit hacky fix, against webkit style but according to 
http://bugreports.qt.nokia.com/browse/QTBUG-54
**************
Since X11 headers are not namespace safe, they have to be included after Qt headers. Generally, headers should always be included in the order from the most specialized to the most general one to prevent namespace pollution:

#include "myownheader.h"
#include <someQtHeader.h>
#include <X11/someXheader>
#include <string.h> // system header
**************

Another way to fix it is some magic with "Bool" "Status" defines, but that looks ugly.

[WebKit Review Bot]

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

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

Source/WebKit2/Shared/Plugins/NPIdentifierData.cpp:28:  Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Source/WebKit2/Shared/Plugins/NPIdentifierData.h:32:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebKit2/Shared/Plugins/NPObjectProxy.cpp:28:  Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 3 in 5 files


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

[Balazs Kelemen]

Comment on attachment 85029 [details]
Qt Xlib includes conflict fix, Fixed include order, QTBUG-54

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

> Source/WebKit2/Shared/Plugins/NPIdentifierData.h:33
> -#include <WebCore/npruntime.h>
>  #include <wtf/text/CString.h>
> +#include <WebCore/npruntime.h>
>  

We have WebCore/bridge/npruntime_internal.h to solve this. Hopefully it can solve all these "evil defines" problems.

[Oleg Romashin (:romaxa)]

Created attachment 85064 [details]
Use runtime_internal.h to solve X/Qt conflicts

Thanks Balazs, this works fine

[Andreas Kling]

Comment on attachment 85064 [details]
Use runtime_internal.h to solve X/Qt conflicts

rs=me

[Oleg Romashin (:romaxa)]

Created attachment 85403 [details]
Fixed according to recent NetscapePluginQt/Gtk -> NetscapePluginX11 changes

[WebKit Commit Bot]

Comment on attachment 85064 [details]
Use runtime_internal.h to solve X/Qt conflicts

Clearing flags on attachment: 85064

Committed r80784: <http://trac.webkit.org/changeset/80784>

[Andreas Kling]

Comment on attachment 85403 [details]
Fixed according to recent NetscapePluginQt/Gtk -> NetscapePluginX11 changes

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

> Source/WebKit2/UIProcess/API/qt/qwkpage.cpp:280
> +void
> +QWKPagePrivate::setComplexTextInputEnabled(uint64_t pluginComplexTextInputIdentifier,
> +                                           bool complexTextInputEnabled)

Coding style, this should all be on one line.

> Source/WebKit2/UIProcess/Plugins/PluginProcessProxy.cpp:140
> -        // FIXME: This is Mac specific.
> +#if PLATFORM(MAC)
>          reply->encode(CoreIPC::MachPort(0, MACH_MSG_TYPE_MOVE_SEND));
> +#else
> +        ASSERT_NOT_REACHED();
> +#endif
>          replyWebProcessProxy->connection()->sendSyncReply(reply);

Why the ASSERT_NOT_REACHED()? It reads to me like we can indeed reach this code.

> Source/WebKit2/UIProcess/Plugins/PluginProcessProxy.cpp:217
> +

Unnecessary newline.

> Source/WebKit2/WebProcess/Plugins/PluginProcessConnectionManager.cpp:74
> +    connectionIdentifier = 0;
> +    ASSERT(connectionIdentifier);

What's going on here?

[Oleg Romashin (:romaxa)]

Created attachment 85408 [details]
Fixed comments from 55719#c35

[Andreas Kling]

Comment on attachment 85408 [details]
Fixed comments from 55719#c35

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

As discussed on IRC, this is merely the first step (making it build) towards a proper implementation.

I like the intent of keeping patches incremental and readable, so I'll say r+ here.

> Source/WebKit2/UIProcess/API/qt/qwkpage.cpp:279
> +void
> +QWKPagePrivate::setComplexTextInputEnabled(uint64_t pluginComplexTextInputIdentifier, bool complexTextInputEnabled)

Coding style, the function return type should not be on a line by itself.

[Balazs Kelemen]

The patch looks good to me but we should investigate in the complex 
text concept to understand it's role. I'm still unsure about that we
really need that. I will take a look into that tomorrow.

[Oleg Romashin (:romaxa)]

Created attachment 85419 [details]
Fixed style nit from 55719#c37

[Oleg Romashin (:romaxa)]

Created attachment 85440 [details]
Build log

Not sure where is exactly problem, but I found that when we compiling UIProcess or WebProcess stuff then WebKit2 includes are in first place, but when we compiling PluginProcess stuff then WebKit includes (as well config.h) are not available in first place and WebCore stuff overriding config.h from webkit2 subfolder...
as result we are getting build error in attachment...

Fast fix is to copy PLUGIN_ARCHITECTURE stuff to webcore/config.h but that is wrong because I don't understand why in pluginProcess includes order are different comparing to WebProcess or UIProcess 
..
I tried to compare .pri/pro files and did not found what is the difference...
any ideas what is wrong here?

[Oleg Romashin (:romaxa)]

kbalazs: do you know what is needed in order to have right include order for PluginProcess?

[Balazs Kelemen]

(In reply to comment #41)
> kbalazs: do you know what is needed in order to have right include order for PluginProcess?

Currently I have not idea.

[Balazs Kelemen]

Huhh, complex text handling is complex. I'm still unsure about the purpose of the whole logic i.e
I'm unsure about it is needed for supporting exotic languages or it is just an optimization for situations when
the simple code path can be used. Actually the plugin part is seems to be really needed for supporting non-regular
text with plugins (i.e. we need to wait for the whole string to arrive before sending it to the plugin).
However I hardly think that it is even possible for X based plugins.
The complex text is sent to the plugin here (on Mac):
-> NetscapePluginMac.cpp
void NetscapePlugin::sendComplexTextInput(const String& textInput)
{
    switch (m_eventModel) {
    case NPEventModelCocoa: {
        NPCocoaEvent event = initializeEvent(NPCocoaEventTextInput);
        event.data.text.text = reinterpret_cast<NPNSString*>(static_cast<NSString*>(textInput));
        NPP_HandleEvent(&event);
        break;
    }
...

But we have to work with XKeyEvent that cannot store a unicode string.
According to that I didn't see anything in the Qt and GTK WK1 plugin code that deals with complex text.

The complex text story exists on WK1 as well at the WebKit level but it is used by only Mac and Windows.
Here is some grep output:
./mac/WebKit.order:2464:+[WebView(WebPrivate) _setAlwaysUsesComplexTextCodePath:]
./mac/WebView/WebView.mm:1019:    [self _setAlwaysUsesComplexTextCodePath:f];
./mac/WebView/WebView.mm:1022:+ (void)_setAlwaysUsesComplexTextCodePath:(BOOL)f
./mac/WebView/WebViewPrivate.h:340:+ (void)_setAlwaysUsesComplexTextCodePath:(BOOL)f;
./win/Interfaces/IWebViewPrivate.idl:156:    HRESULT setAlwaysUsesComplexTextCodePath([in] BOOL complex);
./win/Interfaces/IWebViewPrivate.idl:157:    HRESULT alwaysUsesComplexTextCodePath([out, retval] BOOL* complex);
./win/WebView.h:750:    virtual HRESULT STDMETHODCALLTYPE setAlwaysUsesComplexTextCodePath(
./win/WebView.h:753:    virtual HRESULT STDMETHODCALLTYPE alwaysUsesComplexTextCodePath(
./win/WebView.cpp:5853:HRESULT STDMETHODCALLTYPE WebView::setAlwaysUsesComplexTextCodePath(BOOL complex)
./win/WebView.cpp:5855:    WebCoreSetAlwaysUsesComplexTextCodePath(complex);
./win/WebView.cpp:5860:HRESULT STDMETHODCALLTYPE WebView::alwaysUsesComplexTextCodePath(BOOL* complex)
./win/WebView.cpp:5865:    *complex = WebCoreAlwaysUsesComplexTextCodePath();

According to special circumstances they call WebCore::Font::setCodePath (static function) to statically
anchor the path to use. I guess it is about optimizing the simple case and avoid having to decide the path
for every text run.

In summary I think we should not deal with that now but leave it as a Mac specific thing.
This way we can set up the build with a less complex patch.

[Oleg Romashin (:romaxa)]

Created attachment 85607 [details]
Same patch, but with complexTextPlugin disabled for non-Mac

Here is another patch which does not contain complexTextPlugin stuff
From my point of view it worse to add InputText API for NON-Mac plugins, and should not be hard... also it will help for som different mobile platforms with different VKB IM...

but anyway patch above with ComplexTextInput, and current patch without it.
My point here is to get ENABLE_PLUGIN_PROCES define compiling.

[Balazs Kelemen]

(In reply to comment #44)
> Created an attachment (id=85607) [details]
> Same patch, but with complexTextPlugin disabled for non-Mac
> 
> Here is another patch which does not contain complexTextPlugin stuff
> From my point of view it worse to add InputText API for NON-Mac plugins, and should not be hard... also it will help for som different mobile platforms with different VKB IM...

IMHO this can be handled in the future and not belongs strictly to
make Qt build with ENABLE(PLUGIN_PROCESS).

[Andreas Kling]

Comment on attachment 85607 [details]
Same patch, but with complexTextPlugin disabled for non-Mac

OK

[WebKit Commit Bot]

Comment on attachment 85607 [details]
Same patch, but with complexTextPlugin disabled for non-Mac

Rejecting attachment 85607 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=eseidel-cq-sf', 'ap..." exit_code: 2

Last 500 characters of output:

patching file Source/WebKit2/UIProcess/Plugins/PluginProcessProxy.h
Hunk #1 succeeded at 80 (offset 3 lines).
patching file Source/WebKit2/UIProcess/Plugins/PluginProcessProxy.messages.in
patching file Source/WebKit2/WebProcess/Plugins/PluginProcessConnectionManager.cpp
patching file Source/WebKit2/WebProcess/WebPage/WebPage.cpp
Hunk #1 succeeded at 110 (offset 1 line).

Failed to run "[u'/Projects/CommitQueue/Tools/Scripts/svn-apply', u'--reviewer', u'Andreas Kling', u'--force']" exit_code: 1

Full output: http://queues.webkit.org/results/8193339

[Oleg Romashin (:romaxa)]

Created attachment 86028 [details]
Fixed patch apply to latest trunk

[Balazs Kelemen]

(In reply to comment #48)
> Created an attachment (id=86028) [details]
> Fixed patch apply to latest trunk

May I land it?

[WebKit Commit Bot]

Comment on attachment 86028 [details]
Fixed patch apply to latest trunk

Rejecting attachment 86028 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-3', 'build'..." exit_code: 2

Last 500 characters of output:
 WebKit2 WITH CONFIGURATION Debug ===
Check dependencies

=== BUILD AGGREGATE TARGET All OF PROJECT WebKit2 WITH CONFIGURATION Debug ===
Check dependencies
** BUILD FAILED **


The following build commands failed:
WebKit2:
	CompileC /mnt/git/webkit-commit-queue/WebKitBuild/WebKit2.build/Debug/WebKit2.build/Objects-normal/x86_64/PluginProcessMac.o /mnt/git/webkit-commit-queue/Source/WebKit2/PluginProcess/mac/PluginProcessMac.mm normal x86_64 objective-c++ com.apple.compilers.gcc.4_2
(1 failure)


Full output: http://queues.webkit.org/results/8188670

[Oleg Romashin (:romaxa)]

sounds like NetscapePluginModule.h missing in PluginProcessMac.mm, but not sure... is there are some way to get more dtailed info about mac failure?

[Oleg Romashin (:romaxa)]

Created attachment 86064 [details]
Keep PluginProcess::createWebProcessConnection in PluginProcess.cpp (suggested by :andersca)

[Balazs Kelemen]

(In reply to comment #52)
> Created an attachment (id=86064) [details]
> Keep PluginProcess::createWebProcessConnection in PluginProcess.cpp (suggested by :andersca)

It's there but hard to find. Search for "PluginProcessMac.mm" from the
start of the build log. ;)

[Oleg Romashin (:romaxa)]

Created attachment 86070 [details]
Previous version, added missing WebProcessConnection.h  createWebProcessConnection platform specific method

[Balazs Kelemen]

(In reply to comment #54)
> Created an attachment (id=86070) [details]
> Previous version, added missing WebProcessConnection.h  createWebProcessConnection platform specific method

So, which one do you like to be reviewed?

[Oleg Romashin (:romaxa)]

> So, which one do you like to be reviewed?

createWebProcessConnection now contain also generic code, so I guess second one need to be reviewed.
https://bugs.webkit.org/attachment.cgi?id=86070

[Andreas Kling]

CC'ing people who care about Qt + plugins.

[Balazs Kelemen]

Created attachment 87734 [details]
patch

Updated patch. Handle some further build issues.

[Eric Seidel (no email)]

Comment on attachment 85607 [details]
Same patch, but with complexTextPlugin disabled for non-Mac

Cleared Andreas Kling's review+ from obsolete attachment 85607 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[Eric Seidel (no email)]

Comment on attachment 86028 [details]
Fixed patch apply to latest trunk

Cleared Andreas Kling's review+ from obsolete attachment 86028 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[Balazs Kelemen]

Created attachment 88903 [details]
Patch

[Balazs Kelemen]

Comment on attachment 88903 [details]
Patch

Changed my mind.
We don't need separate files for Qt and Gtk.

[Balazs Kelemen]

Comment on attachment 88903 [details]
Patch

...but the renaming should be handled in a follow up patch

[Balazs Kelemen]

ping

[Balazs Kelemen]

Comment on attachment 88903 [details]
Patch

Clearing flags on attachment: 88903

Committed r83842: <http://trac.webkit.org/changeset/83842>

[Balazs Kelemen]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

It broke the Symbian build: http://webkit.sed.hu/buildbot/builders/Qt%20Symbian%20ARMv5%20Release/builds/368

Could you fix it?

[Balazs Kelemen]

(In reply to comment #67)
> It broke the Symbian build: http://webkit.sed.hu/buildbot/builders/Qt%20Symbian%20ARMv5%20Release/builds/368
> 
> Could you fix it?

I don't think so :(
This is the error:
Compile    : Source\WebKit2\Shared\Plugins\PluginProcessCreationParameters.cpp [armv5_urel.rvct4_0]
   /bin/sh: -c: line 0: unexpected EOF while looking for matching `"'
   /bin/sh: -c: line 1: syntax error: unexpected end of filef

This make me guess that this is either a compiler error or an error with the bot.
Actually my patch should not change anything without adding the -DDEFINES+=ENABLE_PLUGIN_PROCESS flag to the build.

[jade han]

the build break on symbian builder is not related to code, but related to a raptor issue which [e-mail] will file a bug against.  By passing --no-depend-generate to sbs command, the build is passing now.

[Balazs Kelemen]

(In reply to comment #69)
> the build break on symbian builder is not related to code, but related to a raptor issue which [e-mail] will file a bug against.  By passing --no-depend-generate to sbs command, the build is passing now.

Thanks for finding this out.