[webkit-reviews] review granted: [Bug 43535] Add shared memory abstraction : [Attachment 63538] Patch

[bugzilla-daemon at webkit.org]

Adam Roben (aroben) <aroben at apple.com> has granted Anders Carlsson
<andersca at apple.com>'s request for review:
Bug 43535: Add shared memory abstraction
https://bugs.webkit.org/show_bug.cgi?id=43535

Attachment 63538: Patch
https://bugs.webkit.org/attachment.cgi?id=63538&action=review

------- Additional Comments from Adam Roben (aroben) <aroben at apple.com>
> +void SharedMemory::Handle::encode(CoreIPC::ArgumentEncoder& encoder) const
> +{
> +    ASSERT(m_port);
> +    ASSERT(m_size);
> +    
> +    encoder.encodeUInt64(m_size);
> +    encoder.encode(CoreIPC::MachPort(m_port, MACH_MSG_TYPE_COPY_SEND));
> +    const_cast<Handle*>(this)->m_port = MACH_PORT_NULL;
> +}

Making m_port mutable seems preferable.

> +bool SharedMemory::Handle::decode(CoreIPC::ArgumentDecoder& decoder, Handle&
handle)
> +{
> +    ASSERT(!handle.m_port);
> +    ASSERT(!handle.m_size);
> +
> +
> +    uint64_t size;

Extra newline here.

> +PassRefPtr<SharedMemory> SharedMemory::create(size_t size)
> +{
> +    mach_vm_address_t address;
> +    kern_return_t kr = mach_vm_allocate(mach_task_self(), &address,
round_page(size), VM_FLAGS_ANYWHERE);
> +    if (kr != KERN_SUCCESS)
> +	   return 0;
> +
> +    RefPtr<SharedMemory> sharedMemory(adoptRef(new SharedMemory));
> +    sharedMemory->m_size = size;
> +    sharedMemory->m_data = (void*)address;

Is the cast really needed? A C++ cast would be nicer if so.

> +PassRefPtr<SharedMemory> SharedMemory::create(const Handle& handle,
Protection protection)
> +{
> +    if (!handle.m_port)
> +	   return 0;
> +    
> +    // Map the memory.
> +    vm_prot_t vmProtection = machProtection(protection);
> +    mach_vm_address_t mappedAddress;
> +    kern_return_t kr = mach_vm_map(mach_task_self(), &mappedAddress,
handle.m_size, 0, VM_FLAGS_ANYWHERE, handle.m_port, 0, false, vmProtection,
vmProtection, VM_INHERIT_NONE);
> +    if (kr != KERN_SUCCESS)
> +	   return 0;
> +
> +    RefPtr<SharedMemory> sharedMemory(adoptRef(new SharedMemory));
> +    sharedMemory->m_size = handle.m_size;
> +    sharedMemory->m_data = (void*)mappedAddress;

Ditto.

> +SharedMemory::~SharedMemory()
> +{
> +    if (!m_data)
> +	   return;
> +    
> +    kern_return_t kr = mach_vm_deallocate(mach_task_self(),
(vm_address_t)m_data, round_page(m_size));

C++ cast would be better.

> +bool SharedMemory::createHandle(Handle& handle, Protection protection)
> +{
> +    ASSERT(!handle.m_port);
> +    ASSERT(!handle.m_size);
> +
> +    mach_vm_address_t address = (vm_address_t)m_data;

Here, too!

> +unsigned SharedMemory::systemPageSize()
> +{
> +    static unsigned pageSize = 0;
> +
> +    if (!pageSize) {
> +	   SYSTEM_INFO systemInfo;
> +	   ::GetSystemInfo(&systemInfo);
> +	   pageSize = systemInfo.dwPageSize;
> +    }
> +
> +    return pageSize;
> +}

Thanks!

> +++ b/WebKit2/win/WebKit2.vcproj
> @@ -1364,6 +1364,10 @@
>				RelativePath="..\Platform\WorkItem.h"
>				>
>			</File>
> +	       <File
> +		RelativePath="..\Platform\SharedMemory.h"
> +	       >
> +	       </File>

VS wants you to use tabs. Copying-and-pasting an existing <File> element is
usually the best way to go.

r=me