Description
Oleg Romashin (:romaxa)
2011-03-03 15:27:54 PST
Bug 55658 - is needed to ifdef MachPort related functionality in *.messages.in files Created attachment 84703 [details]
Works fine with latest webkit, need to check other platforms WIP1
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.
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... > 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 anyway thanks for WIP review Created attachment 84790 [details]
WIP2 fixed style issues
Comment on attachment 84790 [details]
WIP2 fixed style issues
Changelogs are mandatory, please use the Tools/Scripts/prepare-ChangeLog script
Created attachment 84796 [details]
Part1, dummy Qt implementation, Makefiles
Created attachment 84797 [details]
Part1, dummy Qt implementation, Makefiles
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 Created attachment 84835 [details]
Fixed review comments
I hope it was ok to move r+ to updated patch... (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? Comment on attachment 84835 [details] Fixed review comments Clearing flags on attachment: 84835 Committed r80434: <http://trac.webkit.org/changeset/80434> Created attachment 84899 [details]
Part2, fixed sendComplexInput
Attachment 84899 [details] did not build on win: Build output: http://queues.webkit.org/results/8107161 Created attachment 84901 [details]
Added dummy implementation for Gtk and Win port
Comment on attachment 84790 [details]
WIP2 fixed style issues
this is obsolete now
Attachment 84901 [details] did not build on win: Build output: http://queues.webkit.org/results/8105317 Created attachment 84904 [details]
Added dummy implementation for Gtk and Win port + Fix 3 unresolved externals on win port
Attachment 84904 [details] did not build on win: Build output: http://queues.webkit.org/results/8103423 Created attachment 84907 [details]
Added dummy implementation for Gtk and Win port + Fix 3 unresolved externals on win port.v4
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.
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. (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? 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.
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. 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.
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. Created attachment 85064 [details]
Use runtime_internal.h to solve X/Qt conflicts
Thanks Balazs, this works fine
Comment on attachment 85064 [details]
Use runtime_internal.h to solve X/Qt conflicts
rs=me
Created attachment 85403 [details]
Fixed according to recent NetscapePluginQt/Gtk -> NetscapePluginX11 changes
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> 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? Created attachment 85408 [details]
Fixed comments from 55719#c35
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. 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. Created attachment 85419 [details]
Fixed style nit from 55719#c37
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?
kbalazs: do you know what is needed in order to have right include order for PluginProcess? (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. 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. 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.
(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). Comment on attachment 85607 [details]
Same patch, but with complexTextPlugin disabled for non-Mac
OK
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 Created attachment 86028 [details]
Fixed patch apply to latest trunk
(In reply to comment #48) > Created an attachment (id=86028) [details] > Fixed patch apply to latest trunk May I land it? 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 sounds like NetscapePluginModule.h missing in PluginProcessMac.mm, but not sure... is there are some way to get more dtailed info about mac failure? Created attachment 86064 [details]
Keep PluginProcess::createWebProcessConnection in PluginProcess.cpp (suggested by :andersca)
(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. ;) Created attachment 86070 [details]
Previous version, added missing WebProcessConnection.h createWebProcessConnection platform specific method
(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? > 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 CC'ing people who care about Qt + plugins. Created attachment 87734 [details]
patch
Updated patch. Handle some further build issues.
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. 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. Created attachment 88903 [details]
Patch
Comment on attachment 88903 [details]
Patch
Changed my mind.
We don't need separate files for Qt and Gtk.
Comment on attachment 88903 [details]
Patch
...but the renaming should be handled in a follow up patch
ping Comment on attachment 88903 [details] Patch Clearing flags on attachment: 88903 Committed r83842: <http://trac.webkit.org/changeset/83842> All reviewed patches have been landed. Closing bug. It broke the Symbian build: http://webkit.sed.hu/buildbot/builders/Qt%20Symbian%20ARMv5%20Release/builds/368 Could you fix it? (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. the build break on symbian builder is not related to code, but related to a raptor issue which siddharth.mathur@nokia.com will file a bug against. By passing --no-depend-generate to sbs command, the build is passing now. (In reply to comment #69) > the build break on symbian builder is not related to code, but related to a raptor issue which siddharth.mathur@nokia.com will file a bug against. By passing --no-depend-generate to sbs command, the build is passing now. Thanks for finding this out. |