Bug 47345

Summary: [Qt] Implement SharedMemory for WebKit2
Product: WebKit Reporter: Zoltan Horvath <zoltan>
Component: WebKit2Assignee: Zoltan Horvath <zoltan>
Status: RESOLVED FIXED    
Severity: Normal CC: christian.webkit, kenneth, koivisto, webkit-ews, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Linux   
Bug Depends on: 48238    
Bug Blocks:    
Attachments:
Description Flags
proposed patch
none
updated proposed patch
none
updated proposed patch
none
proposed patch
none
updated patch
none
svn patch
none
svn patch
koivisto: review-
proposed patch none

Description Zoltan Horvath 2010-10-07 06:52:30 PDT
Implement unimplemented functions in SharedMemoryQt.cpp.
Move MappedMemory.h and MappedMemoryPool.cpp from Shared/qt to Platform/qt, change WebKit2.pro's affected lines.
Comment 1 Zoltan Horvath 2010-10-07 06:54:48 PDT
Created attachment 70077 [details]
proposed patch
Comment 2 WebKit Review Bot 2010-10-07 06:55:30 PDT
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.
Comment 3 Zoltan Horvath 2010-10-07 07:19:32 PDT
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.
Comment 4 Balazs Kelemen 2010-10-07 08:08:22 PDT
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.
Comment 5 Zoltan Horvath 2010-10-07 09:53:50 PDT
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.
Comment 6 WebKit Review Bot 2010-10-07 09:55:16 PDT
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.
Comment 7 Zoltan Horvath 2010-10-07 10:13:43 PDT
Created attachment 70107 [details]
proposed patch

Style issues solved.
Comment 8 Balazs Kelemen 2010-10-07 10:22:09 PDT
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.
Comment 9 Kenneth Rohde Christiansen 2010-10-08 05:49:19 PDT
I really think you should discuss this patch with Antti.
Comment 10 Zoltan Horvath 2010-10-08 06:06:39 PDT
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.
Comment 11 Zoltan Horvath 2010-10-15 05:23:44 PDT
Created attachment 70856 [details]
updated patch

Updated to latest.
Ping for r!
Comment 12 Zoltan Horvath 2010-10-15 08:37:35 PDT
Created attachment 70869 [details]
svn patch
Comment 13 Early Warning System Bot 2010-10-15 08:46:53 PDT
Attachment 70869 [details] did not build on qt:
Build output: http://queues.webkit.org/results/4423050
Comment 14 Zoltan Horvath 2010-10-15 08:51:07 PDT
Created attachment 70871 [details]
svn patch

This contains the svn mv infos.
Comment 15 Kenneth Rohde Christiansen 2010-10-15 12:33:45 PDT
Antti, could you comment on this?
Comment 16 Antti Koivisto 2010-10-19 06:16:31 PDT
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
Comment 17 Zoltan Horvath 2010-10-25 02:30:42 PDT
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.
Comment 18 WebKit Review Bot 2010-10-25 02:34:56 PDT
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.
Comment 19 Kenneth Rohde Christiansen 2010-10-25 03:23:54 PDT
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?
Comment 20 Kenneth Rohde Christiansen 2010-10-25 03:24:38 PDT
(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!
Comment 21 Zoltan Horvath 2010-10-25 05:28:31 PDT
(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?
Comment 22 Kenneth Rohde Christiansen 2010-10-25 05:30:35 PDT
Comment on attachment 71722 [details]
proposed patch

Maybe :-) just be careful when landing!
Comment 23 Zoltan Horvath 2010-10-25 06:57:02 PDT
Thanks for the review!
Patch landed successfully.
Committed revision 70450.
http://trac.webkit.org/changeset/70450
Comment 24 Zoltan Horvath 2010-10-26 08:03:08 PDT
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.