Summary: | [Qt] Implement SharedMemory for WebKit2 | ||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Zoltan Horvath <zoltan> | ||||||||||||||||||
Component: | WebKit2 | Assignee: | 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
Zoltan Horvath
2010-10-07 06:52:30 PDT
Created attachment 70077 [details]
proposed patch
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.
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 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. 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. 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.
Created attachment 70107 [details]
proposed patch
Style issues solved.
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. I really think you should discuss this patch with Antti. 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. Created attachment 70856 [details]
updated patch
Updated to latest.
Ping for r!
Created attachment 70869 [details]
svn patch
Attachment 70869 [details] did not build on qt: Build output: http://queues.webkit.org/results/4423050 Created attachment 70871 [details]
svn patch
This contains the svn mv infos.
Antti, could you comment on this? 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
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. 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 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? (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! (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 on attachment 71722 [details]
proposed patch
Maybe :-) just be careful when landing!
Thanks for the review! Patch landed successfully. Committed revision 70450. http://trac.webkit.org/changeset/70450 |