As suggested in comment https://bugs.webkit.org/show_bug.cgi?id=71063#c2 it would be better (more readable at callsites) to have 'it->key' and 'it->value' instead of 'it->first' and 'it->second'.
Created attachment 134905 [details] Patch
Darin Adler, could you take a look at this one? The most interesting bits are in Source/WTF, in particular HashTraits.h.
Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API
Attachment 134905 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/API/JSCallbackObject..." exit_code: 1 Tools/DumpRenderTree/win/AccessibilityControllerWin.cpp:319: Use 0 instead of NULL. [readability/null] [5] Total errors found: 1 in 352 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 134905 [details] Patch Attachment 134905 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/12266568
Comment on attachment 134905 [details] Patch Attachment 134905 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/12253515
The patch looks great in general but it seems like you still have to fix some builds. And I'd differ the review of traits changes to veteran WTF reviewers :)
Created attachment 135113 [details] Patch
Created attachment 135116 [details] Patch
Comment on attachment 135116 [details] Patch Attachment 135116 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/12306757
Created attachment 135122 [details] Patch
Ideally maciej/darin would take a look at this.
Comment on attachment 135122 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=135122&action=review > Source/WTF/ChangeLog:9 > + HashMapEntry. In HashMap the ValueType serve as "contents" of the iterators. I would suggest naming this KeyValuePair instead of HashMapEntry. It seems like in principle it could be useful in other contexts where you have a key/value pair and want better names than first/second.
Created attachment 138874 [details] Patch
New rebased version of the patch. I also changed HashMapEntry to KeyValuePair, but kept it defined in HashTraits.h for now. If we decided to go with it, I can create a separate file for KeyValuePair in this or a different patch
Ping?
(In reply to comment #16) > Ping? You probably need to update the patch for ToT.
Comment on attachment 138874 [details] Patch Removing r? until I upload a new rebased version.
Created bug 92137 for the non-mechanical bits of this change. I hope that bug is more "reviewable" :-)
Created attachment 155084 [details] Patch for the EWS, not ready for review
Attachment 155084 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/API/JSCallbackObject..." exit_code: 1 Tools/DumpRenderTree/win/AccessibilityControllerWin.cpp:319: Use 0 instead of NULL. [readability/null] [5] Total errors found: 1 in 363 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 155084 [details] Patch for the EWS, not ready for review Attachment 155084 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/13372755
Comment on attachment 155084 [details] Patch for the EWS, not ready for review Attachment 155084 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/13381108
Comment on attachment 155084 [details] Patch for the EWS, not ready for review Attachment 155084 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/13380489
Comment on attachment 155084 [details] Patch for the EWS, not ready for review Attachment 155084 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/13368931
Created attachment 155275 [details] Patch for EWS
Attachment 155275 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/API/JSCallbackObject..." exit_code: 1 Tools/DumpRenderTree/win/AccessibilityControllerWin.cpp:319: Use 0 instead of NULL. [readability/null] [5] Total errors found: 1 in 371 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 155275 [details] Patch for EWS Attachment 155275 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/13392340
Comment on attachment 155275 [details] Patch for EWS Attachment 155275 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13396337
Created attachment 155317 [details] Patch
Comment on attachment 155317 [details] Patch These are the mechanical changes only now, the most important thing to review is whether this API change for HashMap iterators is something we want or not.
Maciej, ping?
Comment on attachment 155317 [details] Patch LGTM.
Comment on attachment 155317 [details] Patch Rejecting attachment 155317 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2 Last 500 characters of output: ndle.cpp patching file Tools/TestWebKitAPI/Tests/WebKit2/DOMWindowExtensionNoCache_Bundle.cpp patching file Tools/WebKitTestRunner/InjectedBundle/LayoutTestController.cpp Hunk #1 FAILED at 452. Hunk #2 FAILED at 467. 2 out of 2 hunks FAILED -- saving rejects to file Tools/WebKitTestRunner/InjectedBundle/LayoutTestController.cpp.rej Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'Eric Seidel']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue/ Full output: http://queues.webkit.org/results/13574146
Created attachment 160729 [details] For EWS
Comment on attachment 160729 [details] For EWS Rebased patch to trunk, double checking builds with EWSs.
Comment on attachment 160729 [details] For EWS Attachment 160729 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/13609407
Comment on attachment 160729 [details] For EWS Attachment 160729 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/13609416
Comment on attachment 160729 [details] For EWS Attachment 160729 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/13623096
Created attachment 160821 [details] Patch
Comment on attachment 160821 [details] Patch Rejecting attachment 160821 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2 Last 500 characters of output: commit-queue/Source/WebKit/chromium/third_party/v8-i18n --revision 117 --non-interactive --force --accept theirs-conflict --ignore-externals' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium' 48>At revision 117. ________ running '/usr/bin/python tools/clang/scripts/update.py --mac-only' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium' ________ running '/usr/bin/python gyp_webkit' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium' Updating webkit projects from gyp files... Full output: http://queues.webkit.org/results/13641065
Created attachment 160850 [details] Patch for landing
Created attachment 160852 [details] Patch for landing
Comment on attachment 160852 [details] Patch for landing Rejecting attachment 160852 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2 Last 500 characters of output: /ChangeLog Failed to merge in the changes. Patch failed at 0001 Gardening: skipping tests due to WebCore::JSEventListener::jsFunction(WebCore::ScriptExecutionContext*) crashes. When you have resolved this problem run "git rebase --continue". If you would prefer to skip this patch, instead run "git rebase --skip". To restore the original branch and stop rebasing run "git rebase --abort". rebase refs/remotes/origin/master: command returned error: 1 Died at Tools/Scripts/update-webkit line 164. Full output: http://queues.webkit.org/results/13636116
Comment on attachment 160852 [details] Patch for landing Clearing flags on attachment: 160852 Committed r126836: <http://trac.webkit.org/changeset/126836>
All reviewed patches have been landed. Closing bug.
Re-opened since this is blocked by 95163
Looks like there are also many more hidden behind #ifdefs. Please grep for ->first, ->second, ).first, ).second and check if they come from maps or pairs.
Sorry, we should have let the EWS chew on it first. My bad.
Comment on attachment 160852 [details] Patch for landing Attachment 160852 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/13651040
This patch gets outdated too quickly, in this case a patch added code to JSC using first/second while CQ was processing it. I'll land this manually tomorrow, and double check the potential cases behind development flags.
Comment on attachment 160852 [details] Patch for landing Attachment 160852 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/13646189
Comment on attachment 160852 [details] Patch for landing Attachment 160852 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/13656027
> This patch gets outdated too quickly, in this case a patch added code to JSC using first/second while CQ was processing it. > > I'll land this manually tomorrow, and double check the potential cases behind development flags. You can ping me before your land. I'll help you if the patch breaks the Mac bots again.
Committed r126914: <http://trac.webkit.org/changeset/126914>
How can this massive patch have been landed with zero justification described in the changelog and commit message?
Re-opened since this is blocked by 95239
For the record, there was a discussion in the mailing list after the rollout. http://lists.webkit.org/pipermail/webkit-dev/2012-August/022095.html
Created attachment 164222 [details] For EWS consumption
Chromium is expected to fail in this last patch since now there's code in chromium repo (outside WebKit) that uses WTF::HashMap.
Comment on attachment 164222 [details] For EWS consumption Attachment 164222 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13841908
Comment on attachment 164222 [details] For EWS consumption Attachment 164222 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/13845730
Comment on attachment 164222 [details] For EWS consumption View in context: https://bugs.webkit.org/attachment.cgi?id=164222&action=review > Source/WTF/wtf/HashTraits.h:209 > - // TODO: Rename these to key and value. See https://bugs.webkit.org/show_bug.cgi?id=82784. > - KeyTypeArg first; > - ValueTypeArg second; > + KeyTypeArg key; > + ValueTypeArg value; No luck with the fancy anonymous union stuff?
Comment on attachment 164222 [details] For EWS consumption View in context: https://bugs.webkit.org/attachment.cgi?id=164222&action=review >> Source/WTF/wtf/HashTraits.h:209 >> + ValueTypeArg value; > > No luck with the fancy anonymous union stuff? It works but requires C++11 feature called "unrestricted unions". For Apple builds it is supported only in Clang >= 3.1. Since they still use a previous version, the union solution wouldn't help. The current plan is: Benjamin will help me probably next week to land this.
Comment on attachment 164222 [details] For EWS consumption Attachment 164222 [details] did not pass cr-android-ews (chromium-android): Output: http://queues.webkit.org/results/13841918
(In reply to comment #60) > Chromium is expected to fail in this last patch since now there's code in chromium repo (outside WebKit) that uses WTF::HashMap. If you add "#define WTF_NEW_HASHMAP_ITERATORS_INTERFACE 1" to your patch, Chromium should compile after http://codereview.chromium.org/10914327/ lands. Once this lands, and that revision rolls into Chromium, I'll take care of removing the #define.
Created attachment 164965 [details] Patch
Comment on attachment 164965 [details] Patch Attachment 164965 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13955297
Comment on attachment 164965 [details] Patch Attachment 164965 [details] did not pass cr-android-ews (chromium-android): Output: http://queues.webkit.org/results/13975025
http://codereview.chromium.org/10914327/ has landed but hasn't rolled into WebKit yet, so this won't compile yet. See Source/WebKit/chromium/DEPS
(In reply to comment #70) > http://codereview.chromium.org/10914327/ has landed but hasn't rolled into WebKit yet, so this won't compile yet. See Source/WebKit/chromium/DEPS I am considering fixing this on Apple side today. Can the patch be landed as is without breaking the Google side of the Universe?
(In reply to comment #71) > (In reply to comment #70) > > http://codereview.chromium.org/10914327/ has landed but hasn't rolled into WebKit yet, so this won't compile yet. See Source/WebKit/chromium/DEPS > > I am considering fixing this on Apple side today. > Can the patch be landed as is without breaking the Google side of the Universe? The DEPS revision for Chromium has rolled past this change, so this patch should theoretically build cleanly on Chromium. Please run it by the cr-linux EWS bot first.
> The DEPS revision for Chromium has rolled past this change, so this patch should theoretically build cleanly on Chromium. > > Please run it by the cr-linux EWS bot first. Next weekend I guess.
Created attachment 167381 [details] Patch
Attachment 167381 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/API/JSCallbackObject..." exit_code: 1 Source/WebCore/svg/graphics/filters/SVGFilterBuilder.h:70: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/JavaScriptCore/dfg/DFGStructureCheckHoistingPhase.cpp:210: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3] Source/WebCore/page/SpeechInput.cpp:65: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Tools/DumpRenderTree/chromium/NotificationPresenter.cpp:92: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/inspector/InspectorMemoryAgent.cpp:238: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/inspector/InspectorMemoryAgent.cpp:249: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Total errors found: 6 in 400 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 167381 [details] Patch Attachment 167381 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/14172896
Created attachment 167488 [details] Patch
Attachment 167488 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/API/JSCallbackObject..." exit_code: 1 Source/WebCore/svg/graphics/filters/SVGFilterBuilder.h:70: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/JavaScriptCore/dfg/DFGStructureCheckHoistingPhase.cpp:210: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3] Source/WebCore/page/SpeechInput.cpp:65: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Tools/DumpRenderTree/chromium/NotificationPresenter.cpp:92: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/inspector/InspectorMemoryAgent.cpp:238: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/inspector/InspectorMemoryAgent.cpp:249: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Total errors found: 6 in 401 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 167488 [details] Patch Clearing flags on attachment: 167488 Committed r130612: <http://trac.webkit.org/changeset/130612>