Bug 55875 – [Qt][WK2][Symbian] Shared memory implementation for Symbian

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Siddharth Mathur]

Use Symbian OS APIs to implement shared memory used by WK2.

[Siddharth Mathur]

Created attachment 89987 [details]
Native Symbian IPC implementation

[Siddharth Mathur]

I split out shared memory changes from Bug 55877 to simplify life.

[Siddharth Mathur]

Created attachment 89991 [details]
Updated patch

[Kenneth Rohde Christiansen]

Comment on attachment 89991 [details]
Updated patch

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

Shou;dn't we make this work in all cases before committing it?

> Source/WebKit2/Platform/SharedMemory.h:76
> +        mutable uint32_t m_name;

im not sure this is a good variable name.

> Source/WebKit2/Platform/qt/SharedMemorySymbian.cpp:38
> +_LIT(KFormatToString, "%d");

What does this mean?

> Source/WebKit2/Platform/qt/SharedMemorySymbian.cpp:41
> +    : m_name(0), m_size(0)

We normally put these on separate lines

> Source/WebKit2/Platform/qt/SharedMemorySymbian.cpp:58
> +    // name of the global chunk (masquarading as int32_t for ease of serialization) 

isn't masquerading spelled wrongly.

> Source/WebKit2/Platform/qt/SharedMemorySymbian.cpp:71
> +    uint32_t name;
> +     if (!decoder->decodeUInt32(name))
> +         return false;
> +       

indentation issue here

> Source/WebKit2/Platform/qt/SharedMemorySymbian.cpp:77
> +// Create a new shared memory segment

useless comment :) the method already has a descriptive name

> Source/WebKit2/Platform/qt/SharedMemorySymbian.cpp:81
> +    // On Symbian, global chunks (shared memory segments) have system-unique names, so we pick a random 
> +    // number from the kernel's random pool and use it as a string.

Are you sure these will never be reused by the kernel?

> Source/WebKit2/Platform/qt/SharedMemorySymbian.cpp:102
> +// Take a shared memory handle given usually by another process, and open it locally for read or write
> +PassRefPtr<SharedMemory> SharedMemory::create(const Handle& handle, Protection protection)

Maybe rename the method? or is it a global one being implemented here?

> Source/WebKit2/Platform/qt/SharedMemorySymbian.cpp:146
> +    // convert the name (string form) to uint32_t

Please use proper sentences (start with capital, ends with punctuation mark of some kind)

[Siddharth Mathur]

(In reply to comment #4)
> (From update of attachment 89991 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=89991&action=review
> 
> Shou;dn't we make this work in all cases before committing it?
> 

Since you as a reviewer appear to be OK with everything being together in one (working on Symbian) patch, I am removing the Shared Memory implementation from here, and including all deltas in one patch in Bug 55877. :)

[Kenneth Rohde Christiansen]

(In reply to comment #5)
> (In reply to comment #4)
> > (From update of attachment 89991 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=89991&action=review
> > 
> > Shou;dn't we make this work in all cases before committing it?
> > 
> 
> Since you as a reviewer appear to be OK with everything being together in one (working on Symbian) patch, I am removing the Shared Memory implementation from here, and including all deltas in one patch in Bug 55877. :)

I asked this because according to the comments in the patch there are open issues

[Siddharth Mathur]

Created attachment 92256 [details]
Updated patch with only SharedMemory impl for Symbian

This patch has addressed most feedback from Kenneth in this bug and from Balazs in Bug 55877. It's ready for review from my side. 

Kenneth: I did discuss the unresolved issue of when to Close() the chunk (mentioned in the Changelog and in the SharedMemory d'tor) with other Symbian and Qt gurus, and there are no clear solutions since Symbian client-server IPC is not being used for this patch. I chose to use the less pure half-duplex pipes here as they fit better with how other OSes do the WebKit2's IPC setup, process launching, ownership transfer of the endpoint and read/writes. If we decide to productize the Symbian port later, we can revisit the leaks.

[Early Warning System Bot]

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

[Siddharth Mathur]

Created attachment 92274 [details]
Updated patch

Add edited Platform.h which excludes Symbian from USE(UNIX_DOMAIN_SOCKETS) flag.

[Laszlo Gombos]

Comment on attachment 92274 [details]
Updated patch

Looks good to me as one step towards porting WebKit2 to Symbian. As other pieces of the port get implemented, the SharedMemory destructor needs to be revisited.

[WebKit Commit Bot]

Comment on attachment 92274 [details]
Updated patch

Clearing flags on attachment: 92274

Committed r86560: <http://trac.webkit.org/changeset/86560>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.