Bug 55875 - [Qt][WK2][Symbian] Shared memory implementation for Symbian
Summary: [Qt][WK2][Symbian] Shared memory implementation for Symbian
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: S60 Hardware S60 3rd edition
: P2 Normal
Assignee: Siddharth Mathur
URL:
Keywords: Qt, QtTriaged
Depends on:
Blocks: 50251 55877
  Show dependency treegraph
 
Reported: 2011-03-07 06:32 PST by Siddharth Mathur
Modified: 2011-05-16 05:41 PDT (History)
8 users (show)

See Also:


Attachments
Native Symbian IPC implementation (7.08 KB, patch)
2011-04-17 21:24 PDT, Siddharth Mathur
no flags Details | Formatted Diff | Diff
Updated patch (8.58 KB, patch)
2011-04-17 21:45 PDT, Siddharth Mathur
no flags Details | Formatted Diff | Diff
Updated patch with only SharedMemory impl for Symbian (8.10 KB, patch)
2011-05-04 09:24 PDT, Siddharth Mathur
no flags Details | Formatted Diff | Diff
Updated patch (9.16 KB, patch)
2011-05-04 10:33 PDT, Siddharth Mathur
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Siddharth Mathur 2011-03-07 06:32:47 PST
Use Symbian OS APIs to implement shared memory used by WK2.
Comment 1 Siddharth Mathur 2011-04-17 21:24:56 PDT
Created attachment 89987 [details]
Native Symbian IPC implementation
Comment 2 Siddharth Mathur 2011-04-17 21:26:21 PDT
I split out shared memory changes from Bug 55877 to simplify life.
Comment 3 Siddharth Mathur 2011-04-17 21:45:51 PDT
Created attachment 89991 [details]
Updated patch
Comment 4 Kenneth Rohde Christiansen 2011-04-19 01:01:22 PDT
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)
Comment 5 Siddharth Mathur 2011-04-21 15:34:32 PDT
(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. :)
Comment 6 Kenneth Rohde Christiansen 2011-04-22 06:35:59 PDT
(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
Comment 7 Siddharth Mathur 2011-05-04 09:24:40 PDT
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.
Comment 8 Early Warning System Bot 2011-05-04 09:36:13 PDT
Attachment 92256 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8556566
Comment 9 Siddharth Mathur 2011-05-04 10:33:47 PDT
Created attachment 92274 [details]
Updated patch

Add edited Platform.h which excludes Symbian from USE(UNIX_DOMAIN_SOCKETS) flag.
Comment 10 Laszlo Gombos 2011-05-16 03:10:10 PDT
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.
Comment 11 WebKit Commit Bot 2011-05-16 05:41:17 PDT
Comment on attachment 92274 [details]
Updated patch

Clearing flags on attachment: 92274

Committed r86560: <http://trac.webkit.org/changeset/86560>
Comment 12 WebKit Commit Bot 2011-05-16 05:41:21 PDT
All reviewed patches have been landed.  Closing bug.