Implement unimplemented functions in SharedMemoryQt.cpp. Move MappedMemory.h and MappedMemoryPool.cpp from Shared/qt to Platform/qt, change WebKit2.pro's affected lines.
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
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.