Bug 47345 – [Qt] Implement SharedMemory for WebKit2

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

[Zoltan Horvath]

Implement unimplemented functions in SharedMemoryQt.cpp.
Move MappedMemory.h and MappedMemoryPool.cpp from Shared/qt to Platform/qt, change WebKit2.pro's affected lines.

[Zoltan Horvath]

Created attachment 70077 [details]
proposed patch

[WebKit Review Bot]

Attachment 70077 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKit2/Shared/qt/UpdateChunk.cpp:37:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Zoltan Horvath]

Created attachment 70086 [details]
updated proposed patch

I removed unnecessary headers.
I'll open a new bug to remove global functions and make UpdateChunk to use SharedMemory.

[Balazs Kelemen]

Comment on attachment 70086 [details]
updated proposed patch

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

> WebKit2/Platform/qt/SharedMemoryQt.cpp:170
> +
> +    for (unsigned n = 0; n < pool->size(); ++n) {
> +        MappedMemory mm = pool->at(n);
> +        *current = mm;

There is no need to allocating a MappedMemory for storing the result. You should do it like 'current = &pool->at(n)'

> WebKit2/Platform/qt/SharedMemoryQt.cpp:173
> +
> +    for (unsigned n = 0; n < pool->size(); ++n) {
> +        MappedMemory mm = pool->at(n);
> +        *current = mm;
> +        if (current->data == p) 
> +            return current;
> +    }

You have almost the same for in mapFile and mapMemory. All of these should be merged to a static that returns
with a pointer and you can do what you want to do with the pointer at the caller site.

> WebKit2/Platform/qt/SharedMemoryQt.cpp:192
> +
> +    QString file = searchForMappedMemory(reinterpret_cast<uchar*>(m_data))->file->fileName();

Here you should check for 0 pointer or asserting that the result is not zero. I think it should be never be zero here.

[Zoltan Horvath]

Created attachment 70100 [details]
updated proposed patch

(In reply to comment #4)
> There is no need to allocating a MappedMemory for storing the result. You should do it like 'current = &pool->at(n)'

It's true. Done.

> You have almost the same for in mapFile and mapMemory. All of these should be merged to a static that returns
> with a pointer and you can do what you want to do with the pointer at the caller site.

Unfortunately, we can't merge them, these needs to do different things.
 
> Here you should check for 0 pointer or asserting that the result is not zero. I think it should be never be zero here.

I added a null check.

[WebKit Review Bot]

Attachment 70100 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKit2/Shared/qt/UpdateChunk.cpp:35:  Alphabetical sorting problem.  [build/include_order] [4]
WebKit2/Platform/qt/SharedMemoryQt.cpp:35:  Alphabetical sorting problem.  [build/include_order] [4]
WebKit2/Platform/qt/SharedMemoryQt.cpp:182:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 3 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Zoltan Horvath]

Created attachment 70107 [details]
proposed patch

Style issues solved.

[Balazs Kelemen]

Comment on attachment 70107 [details]
proposed patch

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

> WebKit2/Platform/qt/SharedMemoryQt.cpp:198
> +
> +    QString file;   
> +
> +    if (mm)
> +        file = mm->file->fileName();
> +
> +    if (file.isEmpty())
> +        return false;

Why not just returning early if mm is zero? You can fix that before landing I think.

[Kenneth Rohde Christiansen]

I really think you should discuss this patch with Antti.

[Zoltan Horvath]

Okay, then I'm waiting for Antti. 
Btw, I think the correct way of WebKit developing is to sending patches into bugzilla. Fortunately, my actual changes wouldn't cause much rebasing trouble for Antti.

[Zoltan Horvath]

Created attachment 70856 [details]
updated patch

Updated to latest.
Ping for r!

[Zoltan Horvath]

Created attachment 70869 [details]
svn patch

[Early Warning System Bot]

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

[Zoltan Horvath]

Created attachment 70871 [details]
svn patch

This contains the svn mv infos.

[Kenneth Rohde Christiansen]

Antti, could you comment on this?

[Antti Koivisto]

Comment on attachment 70871 [details]
svn patch

We discussed this on irc. The direction looks good but I would prefer a patch with more completed refactoring.
 
- MappedMemory.h has two classes and it is not clear which interfaces are public
- MappedMemory.h vs. impl in MappedMemoryPool.cpp
- Would it reduce confusion to move MappedMemory code to SharedMemoryQt.cpp and make UpdateChunk use SharedMemory?
- as is, mapFile/mapMemory should be declared in header, not in .cpp
- mapFile/mapMemory should probably be in class scope
- MappedMemoryPool::cleanUp() (and possibly the whole class) might be unnecessary since we unlink the shared memory files right after mapping.
- Nokia copyrights should be maintained when copying code to new files

[Zoltan Horvath]

Created attachment 71722 [details]
proposed patch

(In reply to comment #16)
> (From update of attachment 70871 [details])
> We discussed this on irc. The direction looks good but I would prefer a patch with more completed refactoring.
> 
> - MappedMemory.h has two classes and it is not clear which interfaces are public

I renamed MappedMemory.h to MappedMemoryPool.h.

> - MappedMemory.h vs. impl in MappedMemoryPool.cpp

I put MappedMemory implementation into MappedMemoryPool.

> - Would it reduce confusion to move MappedMemory code to SharedMemoryQt.cpp and make UpdateChunk use SharedMemory?

I didn't do this. In this mod, UpdateChunk uses MappedMemoryPool.

> - as is, mapFile/mapMemory should be declared in header, not in .cpp
> - mapFile/mapMemory should probably be in class scope
> - MappedMemoryPool::cleanUp() (and possibly the whole class) might be unnecessary since we unlink the shared memory files right after mapping.

I put mapFile/mapMemory into MappedMemoryPool and removed unnecessary methods.

> - Nokia copyrights should be maintained when copying code to new files

Done.

[WebKit Review Bot]

Attachment 71722 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKit2/Platform/qt/MappedMemoryPool.cpp:28:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 1 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Kenneth Rohde Christiansen]

Comment on attachment 71722 [details]
proposed patch

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

> WebKit2/Platform/qt/MappedMemoryPool.h:36
> +struct MappedMemory {

Here

> WebKit2/Platform/qt/MappedMemoryPool.h:40
> +    struct MappedMemory {

Why is this defined twice?

[Kenneth Rohde Christiansen]

(In reply to comment #19)
> (From update of attachment 71722 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=71722&action=review
> 
> > WebKit2/Platform/qt/MappedMemoryPool.h:36
> > +struct MappedMemory {
> 
> Here
> 
> > WebKit2/Platform/qt/MappedMemoryPool.h:40
> > +    struct MappedMemory {
> 
> Why is this defined twice?

Hmm, it is like the patch is showing the same file twise!

[Zoltan Horvath]

(In reply to comment #20)

> Hmm, it is like the patch is showing the same file twise!

It caused by the svn move and after the changes, didn't it?

[Kenneth Rohde Christiansen]

Comment on attachment 71722 [details]
proposed patch

Maybe :-) just be careful when landing!

[Zoltan Horvath]

Thanks for the review!
Patch landed successfully.
Committed revision 70450.
http://trac.webkit.org/changeset/70450

[Zoltan Horvath]

The guys rolled out the patch in r70457.
It contained 1 wrong line in SharedMemoryQt.cpp. (line 128) 
I removed the wrong line and reland the patch in r70520.