Bug 45984

Created attachment 67929 [details] proposed patch

Attachment 67929 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1 WebKit2/Shared/qt/MappedMemory.h:32: Alphabetical sorting problem. [build/include_order] [4] WebKit2/Shared/qt/MappedMemory.cpp:27: 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: 2 in 7 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 67929 [details] proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=67929&action=prettypatch This seems to include code written by a.o. Antti. Why isn't he credited? > WebKit2/Shared/qt/UpdateChunk.cpp:4 > + * Copyright (C) 2010 Andras Becsi (abecsi@inf.u-szeged.hu), University of Szeged Are we now going to get copyrights statements for everyone working at WebKit at University of Szeged?

Created attachment 67931 [details] proposed patch

Attachment 67931 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1 WebKit2/Shared/qt/MappedMemory.cpp:27: 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 7 files If any of these errors are false positives, please file a bug against check-webkit-style.

(In reply to comment #3) > (From update of attachment 67929 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=67929&action=prettypatch > > This seems to include code written by a.o. Antti. Why isn't he credited? MappedMemory.h does include code written by Antti, I suppose, so I left the copyright text unchanged, but MappedMemory.cpp shouldn't. If I'm wrong, please point me to the snippet which includes code from Antti. > > > WebKit2/Shared/qt/UpdateChunk.cpp:4 > > + * Copyright (C) 2010 Andras Becsi (abecsi@inf.u-szeged.hu), University of Szeged > > Are we now going to get copyrights statements for everyone working at WebKit at University of Szeged? I think this should be a decision of each individual developer. I usually use this template, if it is not appropriate I'll change it.

> > > WebKit2/Shared/qt/UpdateChunk.cpp:4 > > > + * Copyright (C) 2010 Andras Becsi (abecsi@inf.u-szeged.hu), University of Szeged > > > > Are we now going to get copyrights statements for everyone working at WebKit at University of Szeged? > > I think this should be a decision of each individual developer. I usually use this template, if it is not appropriate I'll change it. Personally, I would keep Szeged Copyright and add a "Contributor(s)" field with people names. But in the end, it is up to you, I think. You get to see that is reasonable.

> Personally, I would keep Szeged Copyright and add a "Contributor(s)" field with people names. > > But in the end, it is up to you, I think. You get to see that is reasonable. We kind of have ChangeLogs and git blame for these kinds of things.

Created attachment 68004 [details] proposed patch Changed, to get the focus back on the code.

Attachment 68004 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1 WebKit2/Shared/qt/MappedMemory.cpp:27: 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 7 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 68004 [details] proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=68004&action=prettypatch > WebKit2/ChangeLog:4 > + Would be nice with a comment as "Based on code by Antti Koivisto" or what ever.

(In reply to comment #11) > (From update of attachment 68004 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=68004&action=prettypatch > > > WebKit2/ChangeLog:4 > > + > > Would be nice with a comment as "Based on code by Antti Koivisto" or what ever. I do not fully understand why you think this is needed. I only relocated the MappedMemory struct code (which was previously in UpdateChunk.cpp, and checked in by Antti) to MappedMemory.h. That is why I left the copyright as it was in UpdateChunk.cpp. This is only needed because I store MappedMemory in my MappedMemoryPool class which is the implementation I mention in the ChangeLog. I do not see the "based on" relation here, but maybe I am wrong.

Created attachment 68005 [details] proposed patch Update ChangeLog text to avoid confusion.

Attachment 68005 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1 WebKit2/Shared/qt/MappedMemory.cpp:27: 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 7 files If any of these errors are false positives, please file a bug against check-webkit-style.

(In reply to comment #12) > (In reply to comment #11) > > (From update of attachment 68004 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=68004&action=prettypatch > > > > > WebKit2/ChangeLog:4 > > > + > > > > Would be nice with a comment as "Based on code by Antti Koivisto" or what ever. > > I do not fully understand why you think this is needed. > I only relocated the MappedMemory struct code (which was previously in UpdateChunk.cpp, and checked in by Antti) to MappedMemory.h. That is why I left the copyright as it was in UpdateChunk.cpp. This is only needed because I store MappedMemory in my MappedMemoryPool class which is the implementation I mention in the ChangeLog. I do not see the "based on" relation here, but maybe I am wrong. Ah OK, that wasn't obvious from the diff - I should probably have looked better.

Comment on attachment 68005 [details] proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=68005&action=prettypatch > WebKit2/ChangeLog:14 > + * Shared/qt/MappedMemory.cpp: Added. It seems that you didn't do it correctly, as this was a moved/renamed file. Not a newly added one. You need to use git mv or the svn equivalent. r- because of this.

(In reply to comment #16) > (From update of attachment 68005 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=68005&action=prettypatch > > > WebKit2/ChangeLog:14 > > + * Shared/qt/MappedMemory.cpp: Added. > > It seems that you didn't do it correctly, as this was a moved/renamed file. Not a newly added one. > > You need to use git mv or the svn equivalent. r- because of this. No, it was not. MappedMemory.cpp is a new file, which implements the MappedMemoryPool class. The moved code snippet is the MappedMemory struct which is in MappedMemory.h and was previously in UpdateChunk.cpp (and forward declared in UpdateChunk.h), there is no moved file in my patch. But I should probably rename MappedMemory.cpp to MappedMemoryPool.cpp, since this is what it implements, and this might be the root of confusion.

Created attachment 68006 [details] proposed patch Renamed MappedMemory.cpp to MappedMemoryPool.cpp and made the ChangeLog more informative.

Attachment 68006 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1 WebKit2/Shared/qt/MappedMemoryPool.cpp:27: 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 7 files If any of these errors are false positives, please file a bug against check-webkit-style.

(In reply to comment #18) > Created an attachment (id=68006) [details] > proposed patch > > Renamed MappedMemory.cpp to MappedMemoryPool.cpp and made the ChangeLog more informative. Generally looks good to me. The main problem I see is calling cleanUp in RunLoop::stop. > WebKit2/Platform/qt/RunLoopQt.cpp:69 > + WebKit::MappedMemoryPool::instance()->cleanUp(); This does not belongs to here. For assuring that cleanUp will be called on exit I would use the QCoreApplication::aboutToQuit signal. MappedMemoryPool could be a QObject with the cleanUp slot and you could connect that to the signal in the ctor. Could you take a chance on that? > WebKit2/Shared/qt/MappedMemory.h:47 > + Vector<MappedMemory>* pool(); I think wrapper methods for accessing the pool instead of public access would be somewhat more encapsulated. > WebKit2/Shared/qt/MappedMemoryPool.cpp:47 > + MappedMemory* chunk = &m_pool.at(i); I would use a reference here. (Sorry for the irregular way of review but I have still no bug edit permission currently.)

Created attachment 68099 [details] proposed patch v2 Uploading new patch which takes Balazs's comment into consideration. Thanks Balazs for the reasonable suggestions.

Attachment 68099 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1 WebKit2/Shared/qt/MappedMemoryPool.cpp:27: 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] WebKit2/Shared/qt/MappedMemoryPool.cpp:33: Missing space after , [whitespace/comma] [3] Total errors found: 2 in 6 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 68109 [details] proposed patch v2 Corrected the weird style issue.

Attachment 68109 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1 WebKit2/Shared/qt/MappedMemoryPool.cpp:27: 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 6 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 68109 [details] proposed patch v2 View in context: https://bugs.webkit.org/attachment.cgi?id=68109&action=review > WebKit2/Shared/qt/MappedMemoryPool.cpp:29 > +#include "MappedMemory.h" > + > +#include "StdLibExtras.h" #include "config.h" should be the first include, please add it before landing. Otherwise LGTM, r=me.

Comment on attachment 68109 [details] proposed patch v2 We shouldn't include config.h in WebKit2 code, I realized it is a bug in check-webkit-style script: https://bugs.webkit.org/show_bug.cgi?id=46032 Style is correct, so cq+

Comment on attachment 68109 [details] proposed patch v2 Clearing flags on attachment: 68109 Committed r67945: <http://trac.webkit.org/changeset/67945>

All reviewed patches have been landed. Closing bug.

Comment on attachment 68109 [details] proposed patch v2 Rejecting patch 68109 from commit-queue. Failed to run "[u'/Users/abarth/git/webkit-queue/WebKitTools/Scripts/svn-apply', u'--force']" exit_code: 1 Last 500 characters of output: #1 FAILED at 1. Hunk #2 FAILED at 32. Hunk #3 FAILED at 55. Hunk #4 FAILED at 81. 4 out of 4 hunks FAILED -- saving rejects to file WebKit2/Shared/qt/UpdateChunk.cpp.rej patching file WebKit2/Shared/qt/UpdateChunk.h Hunk #1 FAILED at 27. Hunk #2 FAILED at 38. 2 out of 2 hunks FAILED -- saving rejects to file WebKit2/Shared/qt/UpdateChunk.h.rej patching file WebKit2/WebKit2.pro Hunk #1 FAILED at 162. Hunk #2 FAILED at 294. 2 out of 2 hunks FAILED -- saving rejects to file WebKit2/WebKit2.pro.rej Full output: http://queues.webkit.org/results/4020089

Eric, have you got any idea what happened? CQ landed the patch and after that it can't apply again.

Comment on attachment 68109 [details] proposed patch v2 Clearing flags. Something strange happened with the commit-bot. The change was already landed by the commit-queue before.

If the app crashes the map files will still be left hanging around forever, so this doesn't really solve the problem. Maybe we could have a directory namespace for our map files and just wipe it out completely?

(In reply to comment #32) > If the app crashes the map files will still be left hanging around forever, so this doesn't really solve the problem. Maybe we could have a directory namespace for our map files and just wipe it out completely? How would you wipe out the dir if the app crashes. As I see it you still would have to implement a crash handler, or something similar. On crash the files are left in /tmp/ now which is a tmpfs or is whiped out on reboot on Linux (at least by distributions I now).

It would probably be sufficient to wipe out the leftover files next time the app is used. That stops unlimited accumulation of files. Kimmo suggested it might be a good idea to actually unlink the files right away after they have been mmapd in the client. Then there would be no need for explicit cleanup on exit.

(In reply to comment #34) > It would probably be sufficient to wipe out the leftover files next time the app is used. That stops unlimited accumulation of files. > > Kimmo suggested it might be a good idea to actually unlink the files right away after they have been mmapd in the client. Then there would be no need for explicit cleanup on exit. If we clean up leftover files at startup we need a way to find files, which are currently unused, there could be files of another QtWebProcess which are currently used. I would rather prefer a concept Kimmo suggested then we do not need complex magic like this.

The commit-queue recently moved from one machine to a cluster, and we had locking problems for a bit (wehre multiple machines would attempt to land the same patch). Hopefully those are all solved.

Hey guys, stop flooding a closed bug, go on with this stuff in https://bugs.webkit.org/show_bug.cgi?id=46252 ;) . This had been created for continuing the debate.