Bug 55877 – [Qt][WK2][Symbian] CoreIPC implementation for Symbian OS

• Component: WebKit2
• Status: RESOLVED WONTFIX

[Siddharth Mathur]

Implement reliable and performant IPC mechanisms with Symbian OS APIs similar to what ConnectionQt.cpp and friends do for Linux.

[Siddharth Mathur]

Created attachment 89252 [details]
Initial patches for native Symbian IPC and shared memory

Informal review and comments welcome

[Siddharth Mathur]

Created attachment 89260 [details]
more complete patch

[Kenneth Rohde Christiansen]

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

[Siddharth Mathur]

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

[Siddharth Mathur]

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.

[Siddharth Mathur]

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

[Siddharth Mathur]

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.

[WebKit Review Bot]

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.

[Siddharth Mathur]

Created attachment 90627 [details]
Style fixes

[Early Warning System Bot]

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

[Siddharth Mathur]

Created attachment 90654 [details]
fix Qt build on Linux

[WebKit Review Bot]

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.

[Early Warning System Bot]

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

[Siddharth Mathur]

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

[Balazs Kelemen]

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.

[Siddharth Mathur]

Comment on attachment 90717 [details]
Qt-Linux should build correctly

Clear review flag until I fix some of the issues mentioned

[Siddharth Mathur]

Created attachment 93812 [details]
Patch

[Siddharth Mathur]

Comment on attachment 93812 [details]
Patch

Updated patch ready for review.

[Balazs Kelemen]

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;
}

[Siddharth Mathur]

 
> > 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 :)

[Siddharth Mathur]

Created attachment 94254 [details]
Patch

[Alexis Menard (darktears)]

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.

[Siddharth Mathur]

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.

[Simon Hausmann]

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

[Siddharth Mathur]

Comment on attachment 94613 [details]
Correctly generated patch 

Burying this work for good.