Bug 55877

Created attachment 89252 [details] Initial patches for native Symbian IPC and shared memory Informal review and comments welcome

Created attachment 89260 [details] more complete patch

Comment on attachment 89260 [details] more complete patch View in context: https://bugs.webkit.org/attachment.cgi?id=89260&action=review I dont really know symbian so mostly some minor comments incl. coding style issues - a kind of first round :-) > Source/WebKit2/Platform/CoreIPC/Connection.h:330 > + uint32_t messageID; why spaces before ; ? > Source/WebKit2/Platform/CoreIPC/qt/ConnectionSymbian.cpp:17 > +{ > + > + if (m_isServer) { useless newline > Source/WebKit2/Platform/CoreIPC/qt/ConnectionSymbian.cpp:35 > + m_isConnected = true; > + > +} here as well > Source/WebKit2/Platform/CoreIPC/qt/ConnectionSymbian.cpp:41 > + delete m_pipeReadNotifier; Can this somehow be an OwnPtr? > Source/WebKit2/Platform/CoreIPC/qt/ConnectionSymbian.cpp:64 > + // Read all the bytes we can. Note that multiple messages can be read in one read operation due to the stream-oriented channel WE normally end comments with punctuation mark > Source/WebKit2/Platform/CoreIPC/qt/ConnectionSymbian.cpp:69 > + if (bytesRead > 0) { > + > + __ASSERT_ALWAYS(bytesRead == readPtr.Size(), User::Invariant()); Please avoid these newlines > Source/WebKit2/Platform/CoreIPC/qt/ConnectionSymbian.cpp:79 > + while (true) { > + > + messageStart -= sizeof(uint32_t); // move back pointer to read trailer Again > Source/WebKit2/Platform/CoreIPC/qt/ConnectionSymbian.cpp:113 > + m_pipeReadNotifier->setEnabled(true); > + > +} Again :-) > Source/WebKit2/Platform/CoreIPC/qt/ConnectionSymbian.cpp:118 > + // platformInitialize() should have taken care of the connection by now Maybe add an assert then > Source/WebKit2/Platform/CoreIPC/qt/ConnectionSymbian.cpp:145 > + // a trailer marker (magic number) which makes it easier to spot mesage boundary at the receiver message* > Source/WebKit2/Platform/CoreIPC/qt/ConnectionSymbian.cpp:152 > + return (bytesWritten == writeBuffer.Size()); double space before writeBuffer > Source/WebKit2/Platform/CoreIPC/qt/QSymbianPipeNotifier.cpp:17 > + :m_pipe(pipe), m_type(type), m_bytes(bytes), QObject(parent) space after: and please use newlines for these > Source/WebKit2/Platform/CoreIPC/qt/QSymbianPipeNotifier.cpp:32 > + QEvent *postThreadChangeEvent = new QEvent(PostThreadChangeEventType); * placement > Source/WebKit2/Platform/CoreIPC/qt/QSymbianPipeNotifier.cpp:34 > + return; shouldnt it be set enabled here? you dont want to ignore the argument do you? then add a comment > Source/WebKit2/Platform/CoreIPC/qt/QSymbianPipeNotifier.cpp:47 > + if (ev->type() == QEvent::ThreadChange) { // pre-migration event > + > + if (d_ptr) { weird newline again > Source/WebKit2/Platform/CoreIPC/qt/QSymbianPipeNotifier.cpp:49 > + bool oldState = d_ptr->IsActive(); where are you using this? > Source/WebKit2/Platform/CoreIPC/qt/QSymbianPipeNotifier.cpp:57 > + QEvent *postThreadChangeEvent = new QEvent(PostThreadChangeEventType); * placement. Who frees this? > Source/WebKit2/Platform/CoreIPC/qt/QSymbianPipeNotifier.cpp:64 > + if (ev->type() == PostThreadChangeEventType) { // custom, post-migration event > + > + if (!d_ptr) { strange newline > Source/WebKit2/Platform/CoreIPC/qt/QSymbianPipeNotifier.cpp:76 > + > + return false; > + > +} newline issue agian > Source/WebKit2/Platform/CoreIPC/qt/QSymbianPipeNotifier.h:22 > + // data can be read > + PipeReadDataAvailable, > + // write pipe can take more data > + PipeWriteSpaceAvailable Better add these commetns behind > Source/WebKit2/Platform/CoreIPC/qt/QSymbianPipeNotifier.h:24 > + QSymbianPipeNotifier(RPipe&, EventType, int, QObject *parent = 0); * placement error > Source/WebKit2/Platform/CoreIPC/qt/QSymbianPipeNotifier.h:25 > + virtual ~QSymbianPipeNotifier(); As this class doesnt act as a base class (you are not defining any other method here virtual) then you do not need the destructor to be declared virtual. > Source/WebKit2/Platform/CoreIPC/qt/QSymbianPipeNotifier.h:50 > + static const QEvent::Type PostThreadChangeEventType = (QEvent::Type)(QEvent::User + 100); why not typedef? > Source/WebKit2/Platform/CoreIPC/qt/SymbianPipeActiveObject.cpp:13 > + : CActive(CActive::EPriorityStandard), m_pipe(pipe), m_type(type), m_bytes(spaceInBytes), q_ptr(qq) wrong coding style > Source/WebKit2/Platform/CoreIPC/qt/SymbianPipeActiveObject.cpp:88 > + } > + > +} weird newline > Source/WebKit2/Platform/CoreIPC/qt/SymbianPipeActiveObject.h:12 > + SymbianPipeActiveObject(RPipe, QSymbianPipeNotifier::EventType, int, QSymbianPipeNotifier *); * placement > Source/WebKit2/Platform/qt/SharedMemorySymbian.cpp:14 > + : m_name(0) , m_size(0) style > Source/WebKit2/Platform/qt/SharedMemorySymbian.cpp:54 > +{ > + > + // On Symbian, global chunks (shared memory segments) have system-unique names, so we pick a random newline > Source/WebKit2/Platform/qt/SharedMemorySymbian.cpp:117 > +{ > + > + ASSERT_ARG(handle, handle.isNull()); newline > Source/WebKit2/UIProcess/Launcher/qt/ProcessLauncherSymbian.cpp:21 > + RPipe* localPipes = new RPipe[2]; > + RPipe* remotePipes = new RPipe[2]; Can't we OwnPtr these? Or make a mini wrapper class that also makes sure to close() then when they go out of scope? > Source/WebKit2/UIProcess/Launcher/qt/ProcessLauncherSymbian.cpp:26 > + error = RPipe::Create(CoreIPC::Connection::MaxPipeCapacity, remotePipes[0], localPipes[1] ); weird space before ) > Source/WebKit2/UIProcess/Launcher/qt/ProcessLauncherSymbian.cpp:27 > + error = RPipe::Create(CoreIPC::Connection::MaxPipeCapacity, remotePipes[0], localPipes[1] ); > + error = RPipe::Create(CoreIPC::Connection::MaxPipeCapacity, localPipes[0], remotePipes[1]); why overriding error? > Source/WebKit2/UIProcess/Launcher/qt/ProcessLauncherSymbian.cpp:30 > + RProcess* workerProcess = new RProcess() ; weird space before ; > Source/WebKit2/UIProcess/Launcher/qt/ProcessLauncherSymbian.cpp:35 > + error = workerProcess->SetParameter(3, remotePipes[0]); // reader > + error = workerProcess->SetParameter(4, remotePipes[1]); // writer what about using an enum for these 3 and 4? > Source/WebKit2/UIProcess/Launcher/qt/ProcessLauncherSymbian.cpp:70 > + m_processIdentifier->Terminate(KErrNone); > + > +} remove useless newline > Source/WebKit2/UIProcess/Launcher/qt/ThreadLauncherQt.cpp:54 > + identifier = 0; // NULL pointer I dont get the comment... Isn't it an identifier not a pointer... what is it used for? Maybe you can explain it better like // Identifier is a pointer type on Symbian or 0 pointer is symbolizes invalid socket descriptor - or whatever :) > Source/WebKit2/WebProcess.pro:47 > + TARGET.CAPABILITY *= Location # geolocation well, geolocation support is on UI side due to sandboxing... the process does not need this

Thanks Kenneth for the patient look through. :) I will run the style-checker and fix all your points in the next iteration. I think I have (over)commented the Symbian-specific code, so I hope you will be able to make sense of it even without much direct experience with the OS

To simplify the review and integration complexity, I carved out the relatively simple Shared memory implementation in Bug 55875. Kenneth's comments related to SharedMemorySymbian.cpp have been addressed there.

Created attachment 90623 [details] Core IPC, process launching and shared memory for Symbian OS

Comment on attachment 90623 [details] Core IPC, process launching and shared memory for Symbian OS Ready for review. Apologies in advance for the large size. All the major components, IPC, process launching, and shared memory are required together for the Qt MiniBrowser to launch and render anything on an N8 device.

Attachment 90623 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1 Source/WebKit2/Platform/WorkQueue.h:91: The parameter name "pipe" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebKit2/Platform/WorkQueue.h:91: The parameter name "type" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebKit2/Platform/WorkQueue.h:91: The parameter name "workItem" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebKit2/Platform/qt/SharedMemorySymbian.cpp:34: Missing space after , [whitespace/comma] [3] Source/WebKit2/WebProcess/qt/WebProcessMainQt.cpp:163: Use 0 or null instead of NULL (even in *comments*). [readability/null] [4] Source/WebKit2/Platform/CoreIPC/qt/QSymbianPipeNotifier.cpp:35: Missing space after , [whitespace/comma] [3] Source/WebKit2/Platform/CoreIPC/qt/QSymbianPipeNotifier.cpp:36: Missing space after , [whitespace/comma] [3] Source/WebKit2/Platform/CoreIPC/qt/QSymbianPipeNotifier.cpp:37: Missing space after , [whitespace/comma] [3] Source/WebKit2/Platform/CoreIPC/qt/QSymbianPipeNotifier.cpp:38: Missing space after , [whitespace/comma] [3] Source/WebKit2/Platform/CoreIPC/qt/SymbianPipeActiveObject.cpp:33: Missing space after , [whitespace/comma] [3] Source/WebKit2/Platform/CoreIPC/qt/SymbianPipeActiveObject.cpp:34: Missing space after , [whitespace/comma] [3] Source/WebKit2/Platform/CoreIPC/qt/SymbianPipeActiveObject.cpp:35: Missing space after , [whitespace/comma] [3] Source/WebKit2/Platform/CoreIPC/qt/SymbianPipeActiveObject.cpp:36: Missing space after , [whitespace/comma] [3] Source/WebKit2/Platform/CoreIPC/unix/ConnectionUnix.cpp:30: You should add a blank line after implementation file's own header. [build/include_order] [4] Total errors found: 14 in 29 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 90627 [details] Style fixes

Attachment 90627 [details] did not build on qt: Build output: http://queues.webkit.org/results/8493289

Created attachment 90654 [details] fix Qt build on Linux

Attachment 90654 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1 Source/WebKit2/Platform/CoreIPC/qt/QSymbianPipeNotifier.h:23: Header file should not contain WebCore config.h. Should be: alphabetically sorted. [build/include_order] [4] Total errors found: 1 in 29 files If any of these errors are false positives, please file a bug against check-webkit-style.

Attachment 90654 [details] did not build on qt: Build output: http://queues.webkit.org/results/8487562

Created attachment 90717 [details] Qt-Linux should build correctly

Comment on attachment 90717 [details] Qt-Linux should build correctly View in context: https://bugs.webkit.org/attachment.cgi?id=90717&action=review Informal review. The most important issue I see is the need of going through both the Active Object machinery and Qt event loop with the pipe events. I recommend to avoid this if it is possible. > Source/WebKit2/Platform/SharedMemory.h:76 > +#elif OS(SYMBIAN) > + mutable uint32_t m_chunkName; Not a good name for an integer. m_chunkID? > Source/WebKit2/Platform/CoreIPC/qt/ConnectionSymbian.cpp:80 > +// Check if the data matches the trailer pattern > +static inline bool isMessageTrailer(uint8_t *data) > +{ > + return ( trailer == *(reinterpret_cast<uint32_t*>(data)) ); > +} No need for the comment. trailer is not a good name for a constant like this, it should be smg like trailerPattern or kTrailerPattern. > Source/WebKit2/Platform/CoreIPC/qt/ConnectionSymbian.cpp:97 > + // Temporary place to keep track of parsed messages. > + QVector<ParsedMessage> messsagePointers; I would use WTF::Vector with an inlineCapacity that is enough for the common cases (I guess it is between 1 and 4 inclusively). > Source/WebKit2/Platform/CoreIPC/qt/ConnectionSymbian.cpp:122 > + } // while loop No need for this comment. > Source/WebKit2/Platform/CoreIPC/qt/ConnectionSymbian.cpp:130 > + messsagePointers.clear(); No need for this. > Source/WebKit2/Platform/CoreIPC/qt/ConnectionSymbian.cpp:133 > + m_pipeReadNotifier->setEnabled(true); Maybe it would be clearer with a guard style class that handles this, like SocketNotifierResourceGuard in ConnectionUnix.cpp. > Source/WebKit2/Platform/CoreIPC/qt/ConnectionSymbian.cpp:144 > + // Start listening for read > + m_pipeReadNotifier = m_connectionQueue.registerPipeEventHandler(m_readPipe, QSymbianPipeNotifier::PipeReadDataAvailable, WorkItem::create(this, &Connection::readyReadHandler)); > + Do we really need to go through AO -> Qt event loop -> WK2 work queue abstraction? Why not we call the callback (readyReadHandler in this case) from the Active Object directly? I guess going through the Qt event loop could hurt responsiveness. > Source/WebKit2/Platform/CoreIPC/qt/SymbianPipeActiveObject.cpp:30 > +// The Symbian Active Object that registers notification requests with the kernel, similar to POSIX's select() system call > +// Note that unlike anything on Unix, Symbian Active Objects are schedulable objects that are bound to the current thread's Active Scheduler. > +// Therefore they are a lightweight version of threads from a resource utilization POV. I don't find this comment and the others in this file very useful. The code is pretty straightforward with basic knowledge about Active Objects. > Source/WebKit2/Platform/qt/WorkQueueQt.cpp:145 > + // FIXME: Need to implement a process watcher using Symbian RProcess > +#if !OS(SYMBIAN) > WorkQueue::WorkItemQt* itemQt = new WorkQueue::WorkItemQt(this, process, SIGNAL(finished(int, QProcess::ExitStatus)), workItem.leakPtr()); > itemQt->moveToThread(m_workThread); > +#endif I would add an notImplemented() to the symbian branch, i.e #if OS(SYMBIAN) notImplemented(); #else ... > Source/WebKit2/Platform/unix/SharedMemoryUnix.cpp:29 > +#if HAVE(MMAP) This should rather be "#if USE(UNIX_DOMAIN_SOCKETS)". > Tools/ChangeLog:11 > + > + Reviewed by NOBODY (OOPS!). > + > + [Qt][WK2][Symbian] Corrections to default window mode on Symbian devices > + https://bugs.webkit.org/show_bug.cgi?id=55877 > + > + * MiniBrowser/qt/BrowserWindow.cpp: > + (BrowserWindow::BrowserWindow): > + * MiniBrowser/qt/MiniBrowser.pro: > + These changes really don't belong here. Please separate them to another patch.

Comment on attachment 90717 [details] Qt-Linux should build correctly Clear review flag until I fix some of the issues mentioned

Created attachment 93812 [details] Patch

Comment on attachment 93812 [details] Patch Updated patch ready for review.

Comment on attachment 93812 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=93812&action=review Informal review v2. Looks pretty good to me in overall. I did not review the implementation in details but if it seems to be OK through testing I think it can go. However there is a few issue I see. > Source/WebKit2/Platform/CoreIPC/Connection.h:162 > +#if OS(SYMBIAN) > + static const int MaxPipeCapacity = 4096; > +#endif Move it to the cpp please. > Source/WebKit2/Platform/CoreIPC/qt/ConnectionSymbian.cpp:39 > + uint8_t* start; // base address of message > + uint32_t size; // size in bytes > + uint32_t messageID; > +}; style: double indentation > Source/WebKit2/Platform/CoreIPC/qt/ConnectionSymbian.cpp:50 > + m_readPipe = static_cast<RPipe*>(identifier)[0]; > + m_writePipe = static_cast<RPipe*>(identifier)[1]; Could be Identifier typedefed as RPipe[2]? Would be better. > Source/WebKit2/Platform/CoreIPC/qt/ConnectionSymbian.cpp:103 > + // Temporary place to keep track of parsed messages. > + Vector<ParsedMessage, 10 * sizeof(ParsedMessage)> messsagePointers; The inline capacity (second template arg) means number of elements and not size in bytes, should be simply 10. > Source/WebKit2/Platform/CoreIPC/qt/ConnectionSymbian.cpp:109 > + if (!isMessageTrailer(messageStart)) > + break; This means a connection error, isn't it? In this case the system should be notified about that via connectionDidClose. Actually, I checked the Unix version and it also missing that but the Windows implementation has this at a few places: if (error == ERROR_BROKEN_PIPE) { connectionDidClose(); return; }

> > Source/WebKit2/Platform/CoreIPC/Connection.h:162 > > +#if OS(SYMBIAN) > > + static const int MaxPipeCapacity = 4096; > > +#endif > > Move it to the cpp please. I need it in ProcessLauncherSymbian.cpp while initializing the pipe, hence the use of a header file. > > Source/WebKit2/Platform/CoreIPC/qt/ConnectionSymbian.cpp:39 > > + uint8_t* start; // base address of message > > + uint32_t size; // size in bytes > > + uint32_t messageID; > > +}; > > style: double indentation Fixed in upcoming patch. > > > Source/WebKit2/Platform/CoreIPC/qt/ConnectionSymbian.cpp:50 > > + m_readPipe = static_cast<RPipe*>(identifier)[0]; > > + m_writePipe = static_cast<RPipe*>(identifier)[1]; > > Could be Identifier typedefed as RPipe[2]? Would be better. That was my first preference too, but Identifier is used as a return value in a complete of methods, hence I punted on changing it in common code. > > Source/WebKit2/Platform/CoreIPC/qt/ConnectionSymbian.cpp:103 > > + // Temporary place to keep track of parsed messages. > > + Vector<ParsedMessage, 10 * sizeof(ParsedMessage)> messsagePointers; > > The inline capacity (second template arg) means number of elements and not size in bytes, should be simply 10. Fixed in upcoming patch. > > > Source/WebKit2/Platform/CoreIPC/qt/ConnectionSymbian.cpp:109 > > + if (!isMessageTrailer(messageStart)) > > + break; > > This means a connection error, isn't it? In this case the system should be notified about that via connectionDidClose. > Actually, I checked the Unix version and it also missing that but the Windows implementation has this at a few places: > if (error == ERROR_BROKEN_PIPE) { > connectionDidClose(); > return; > } Fixed in upcoming patch. Thanks :)

Created attachment 94254 [details] Patch

Comment on attachment 94254 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=94254&action=review > Source/WebKit2/WebProcess/qt/WebProcessMainQt.cpp:135 > +#if defined(Q_OS_SYMBIAN) Why defined(Q_OS_SYMBIAN) and below in the same file you use #if OS(SYMBIAN)? > Tools/Makefile:-1 > -MODULES = DumpRenderTree WebKitTestRunner MiniBrowser ../Source/ThirdParty/gtest/xcode TestWebKitAPI What is that file? > Tools/Makefile:22 > +INCPATH = -I"/home/simathur/testQtSDK/Symbian/SDKs/Symbian3Qt472/include/QtCore" -I"/home/simathur/testQtSDK/Symbian/SDKs/Symbian3Qt472/include/QtGui" -I"/home/simathur/testQtSDK/Symbian/SDKs/Symbian3Qt472/include" -I"/home/simathur/testQtSDK/Symbian/SDKs/Symbian3Qt472/include/QtSensors" -I"/home/simathur/testQtSDK/Symbian/SDKs/Symbian3Qt472/mkspecs/common/symbian" -I"/home/simathur/testQtSDK/Symbian/SDKs/Symbian3Qt472/epoc32/include" -I"/home/simathur/testQtSDK/Symbian/SDKs/Symbian3Qt472/epoc32/include/stdapis" -I"/home/simathur/testQtSDK/Symbian/SDKs/Symbian3Qt472/epoc32/include/stdapis/sys" -I"/home/simathur/testQtSDK/Symbian/SDKs/Symbian3Qt472/epoc32/include/mw" -I"/home/simathur/testQtSDK/Symbian/SDKs/Symbian3Qt472/epoc32/include/platform/mw" -I"/home/simathur/testQtSDK/Symbian/SDKs/Symbian3Qt472/epoc32/include/platform" -I"/home/simathur/testQtSDK/Symbian/SDKs/Symbian3Qt472/epoc32/include/platform/loc" -I"/home/simathur/testQtSDK/Symbian/SDKs/Symbian3Qt472/epoc32/include/platform/mw/loc" -I"/home/simathur/testQtSDK/Symbian/SDKs/Symbian3Qt472/epoc32/include/platform/loc/sc" -I"/home/simathur/testQtSDK/Symbian/SDKs/Symbian3Qt472/epoc32/include/platform/mw/loc/sc" -I"/QtMobility" -I"/QtSensors" -I"/home/simathur/testQtSDK/Symbian/SDKs/Symbian3Qt472/epoc32/include/stdapis/stlportv5" -I"/home/simathur/WebKit/Tools" Seems totally wrong.

Created attachment 94613 [details] Correctly generated patch Alexis: The Q_OS_SYMBIAN is for moc to process the QObjects only for Symbian targets. Moc doesn't understand WebKit style USE() and HAVE() macros.

Hmm, do we still want to run and maintain WebKit2 on Symbian?

Comment on attachment 94613 [details] Correctly generated patch Burying this work for good.