RESOLVED FIXED 145541
Use modern for-loops in WebCore/Modules - 2
https://bugs.webkit.org/show_bug.cgi?id=145541
Summary Use modern for-loops in WebCore/Modules - 2
Hunseop Jeong
Reported 2015-06-01 19:09:30 PDT
Use the modern for-loops in WebCore/Modules - 2 of 2.
Attachments
Patch (49.71 KB, patch)
2015-06-02 20:48 PDT, Hunseop Jeong
no flags
Archive of layout-test-results from ews102 for mac-mavericks (deleted)
2015-06-03 04:32 PDT, Build Bot
no flags
Archive of layout-test-results from ews104 for mac-mavericks-wk2 (deleted)
2015-06-03 05:36 PDT, Build Bot
no flags
Archive of layout-test-results from ews106 for mac-mavericks-wk2 (deleted)
2015-06-03 06:59 PDT, Build Bot
no flags
Archive of layout-test-results from ews100 for mac-mavericks (deleted)
2015-06-03 07:46 PDT, Build Bot
no flags
Archive of layout-test-results from ews105 for mac-mavericks-wk2 (deleted)
2015-06-03 08:39 PDT, Build Bot
no flags
Archive of layout-test-results from ews101 for mac-mavericks (deleted)
2015-06-03 09:05 PDT, Build Bot
no flags
Archive of layout-test-results from ews103 for mac-mavericks (deleted)
2015-06-03 11:55 PDT, Build Bot
no flags
Archive of layout-test-results from ews107 for mac-mavericks-wk2 (deleted)
2015-06-03 12:45 PDT, Build Bot
no flags
Patch (49.91 KB, patch)
2015-06-04 00:42 PDT, Hunseop Jeong
no flags
Patch (50.65 KB, patch)
2015-06-07 20:00 PDT, Hunseop Jeong
no flags
Patch (50.64 KB, patch)
2015-06-07 21:23 PDT, Hunseop Jeong
no flags
Patch (49.84 KB, patch)
2015-06-07 21:56 PDT, Hunseop Jeong
no flags
Hunseop Jeong
Comment 1 2015-06-02 20:48:50 PDT
Build Bot
Comment 2 2015-06-03 04:32:20 PDT
Comment on attachment 254137 [details] Patch Attachment 254137 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/5037866975494144 New failing tests: webaudio/audioparam-setValueAtTime.html webaudio/oscillator-square.html webaudio/audioparam-linearRampToValueAtTime.html webaudio/oscillator-custom.html webaudio/audioparam-exponentialRampToValueAtTime.html webaudio/audiobuffersource-playbackrate.html webaudio/audioparam-setTargetAtTime.html webaudio/audioparam-setValueCurveAtTime.html webaudio/oscillator-sawtooth.html webaudio/oscillator-triangle.html webaudio/oscillator-sine.html
Build Bot
Comment 3 2015-06-03 04:32:23 PDT
Created attachment 254167 [details] Archive of layout-test-results from ews102 for mac-mavericks The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews102 Port: mac-mavericks Platform: Mac OS X 10.9.5
Build Bot
Comment 4 2015-06-03 05:36:00 PDT
Comment on attachment 254137 [details] Patch Attachment 254137 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/6013651332890624 New failing tests: webaudio/audioparam-setValueAtTime.html webaudio/oscillator-square.html webaudio/audioparam-linearRampToValueAtTime.html webaudio/oscillator-custom.html webaudio/audioparam-exponentialRampToValueAtTime.html webaudio/audiobuffersource-playbackrate.html webaudio/audioparam-setTargetAtTime.html webaudio/audioparam-setValueCurveAtTime.html webaudio/oscillator-sawtooth.html webaudio/oscillator-triangle.html webaudio/oscillator-sine.html
Build Bot
Comment 5 2015-06-03 05:36:02 PDT
Created attachment 254170 [details] Archive of layout-test-results from ews104 for mac-mavericks-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews104 Port: mac-mavericks-wk2 Platform: Mac OS X 10.9.5
Build Bot
Comment 6 2015-06-03 06:59:04 PDT
Comment on attachment 254137 [details] Patch Attachment 254137 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/5916993093894144 New failing tests: webaudio/audioparam-setValueAtTime.html webaudio/oscillator-square.html webaudio/audioparam-linearRampToValueAtTime.html webaudio/oscillator-custom.html webaudio/audioparam-exponentialRampToValueAtTime.html webaudio/audiobuffersource-playbackrate.html webaudio/audioparam-setTargetAtTime.html webaudio/audioparam-setValueCurveAtTime.html webaudio/oscillator-sawtooth.html webaudio/oscillator-triangle.html webaudio/oscillator-sine.html
Build Bot
Comment 7 2015-06-03 06:59:08 PDT
Created attachment 254172 [details] Archive of layout-test-results from ews106 for mac-mavericks-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews106 Port: mac-mavericks-wk2 Platform: Mac OS X 10.9.5
Build Bot
Comment 8 2015-06-03 07:46:06 PDT
Comment on attachment 254137 [details] Patch Attachment 254137 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/6728427306483712 New failing tests: webaudio/audioparam-setValueAtTime.html webaudio/oscillator-square.html webaudio/audioparam-linearRampToValueAtTime.html webaudio/oscillator-custom.html webaudio/audioparam-exponentialRampToValueAtTime.html webaudio/audiobuffersource-playbackrate.html webaudio/audioparam-setTargetAtTime.html webaudio/audioparam-setValueCurveAtTime.html webaudio/oscillator-sawtooth.html webaudio/oscillator-triangle.html webaudio/oscillator-sine.html
Build Bot
Comment 9 2015-06-03 07:46:09 PDT
Created attachment 254176 [details] Archive of layout-test-results from ews100 for mac-mavericks The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews100 Port: mac-mavericks Platform: Mac OS X 10.9.5
Build Bot
Comment 10 2015-06-03 08:39:06 PDT
Comment on attachment 254137 [details] Patch Attachment 254137 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/4772107284119552 New failing tests: webaudio/audioparam-setValueAtTime.html webaudio/oscillator-square.html webaudio/audioparam-linearRampToValueAtTime.html webaudio/oscillator-custom.html webaudio/audioparam-exponentialRampToValueAtTime.html webaudio/audiobuffersource-playbackrate.html webaudio/audioparam-setTargetAtTime.html webaudio/audioparam-setValueCurveAtTime.html webaudio/oscillator-sawtooth.html webaudio/oscillator-triangle.html webaudio/oscillator-sine.html
Build Bot
Comment 11 2015-06-03 08:39:09 PDT
Created attachment 254177 [details] Archive of layout-test-results from ews105 for mac-mavericks-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews105 Port: mac-mavericks-wk2 Platform: Mac OS X 10.9.5
Build Bot
Comment 12 2015-06-03 09:05:12 PDT
Comment on attachment 254137 [details] Patch Attachment 254137 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/6570532497522688 New failing tests: webaudio/audioparam-setValueAtTime.html webaudio/oscillator-square.html webaudio/audioparam-linearRampToValueAtTime.html webaudio/oscillator-custom.html webaudio/audioparam-exponentialRampToValueAtTime.html webaudio/audiobuffersource-playbackrate.html webaudio/audioparam-setTargetAtTime.html webaudio/audioparam-setValueCurveAtTime.html webaudio/oscillator-sawtooth.html webaudio/oscillator-triangle.html webaudio/oscillator-sine.html
Build Bot
Comment 13 2015-06-03 09:05:14 PDT
Created attachment 254179 [details] Archive of layout-test-results from ews101 for mac-mavericks The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews101 Port: mac-mavericks Platform: Mac OS X 10.9.5
Build Bot
Comment 14 2015-06-03 11:55:41 PDT
Comment on attachment 254137 [details] Patch Attachment 254137 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/5967004766830592 New failing tests: webaudio/audioparam-setValueAtTime.html webaudio/oscillator-square.html webaudio/audioparam-linearRampToValueAtTime.html webaudio/oscillator-custom.html webaudio/audioparam-exponentialRampToValueAtTime.html webaudio/audiobuffersource-playbackrate.html webaudio/audioparam-setTargetAtTime.html webaudio/audioparam-setValueCurveAtTime.html webaudio/oscillator-sine.html webaudio/oscillator-triangle.html webaudio/oscillator-sawtooth.html
Build Bot
Comment 15 2015-06-03 11:55:44 PDT
Created attachment 254191 [details] Archive of layout-test-results from ews103 for mac-mavericks The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews103 Port: mac-mavericks Platform: Mac OS X 10.9.5
Build Bot
Comment 16 2015-06-03 12:45:13 PDT
Comment on attachment 254137 [details] Patch Attachment 254137 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/4948009817210880 New failing tests: webaudio/audioparam-setValueAtTime.html webaudio/oscillator-square.html webaudio/audioparam-linearRampToValueAtTime.html webaudio/oscillator-custom.html webaudio/audioparam-exponentialRampToValueAtTime.html webaudio/audiobuffersource-playbackrate.html webaudio/audioparam-setTargetAtTime.html webaudio/audioparam-setValueCurveAtTime.html webaudio/oscillator-sawtooth.html webaudio/oscillator-triangle.html webaudio/oscillator-sine.html
Build Bot
Comment 17 2015-06-03 12:45:16 PDT
Created attachment 254199 [details] Archive of layout-test-results from ews107 for mac-mavericks-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews107 Port: mac-mavericks-wk2 Platform: Mac OS X 10.9.5
Hunseop Jeong
Comment 18 2015-06-04 00:42:37 PDT
Hunseop Jeong
Comment 19 2015-06-04 00:50:05 PDT
Comment on attachment 254137 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=254137&action=review > Source/WebCore/Modules/webaudio/AudioParamTimeline.cpp:95 > unsigned i = 0; > float insertTime = event.time(); > - for (i = 0; i < m_events.size(); ++i) { > + for (auto& paramEvent : m_events) { > // Overwrite same event type and time. > - if (m_events[i].time() == insertTime && m_events[i].type() == event.type()) { > - m_events[i] = event; > + if (paramEvent.time() == insertTime && paramEvent.type() == event.type()) { > + paramEvent = event; > return; > } > > - if (m_events[i].time() > insertTime) > + if (paramEvent.time() > insertTime) > break; I forgot to increase the value of 'i'. It makes the test failures.
Hunseop Jeong
Comment 20 2015-06-04 01:14:21 PDT
Comment on attachment 254258 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=254258&action=review > Source/WebCore/Modules/webaudio/AudioParamTimeline.cpp:97 > unsigned i = 0; > float insertTime = event.time(); > - for (i = 0; i < m_events.size(); ++i) { > + for (auto& paramEvent : m_events) { > // Overwrite same event type and time. > - if (m_events[i].time() == insertTime && m_events[i].type() == event.type()) { > - m_events[i] = event; > + if (paramEvent.time() == insertTime && paramEvent.type() == event.type()) { > + paramEvent = event; > return; > } > > - if (m_events[i].time() > insertTime) > + if (paramEvent.time() > insertTime) > break; > + > + ++i; I added the 'i' and increased it in new patch. All tests passed.
Darin Adler
Comment 21 2015-06-05 14:42:20 PDT
Comment on attachment 254258 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=254258&action=review I have a few comments, but none are about the new for loops themselves. > Source/WebCore/ChangeLog:8 > + No new tests, no behavior chnages. typo Also, “no behavior change” is not completely a justification for no test coverage; in this case we definitely will tolerate it. > Source/WebCore/Modules/mediastream/RTCDataChannel.cpp:326 > events.clear(); This line of code is not needed. > Source/WebCore/Modules/webaudio/WaveShaperProcessor.cpp:69 > + WaveShaperDSPKernel* kernel = static_cast<WaveShaperDSPKernel*>(audioDSPKernel.get()); I think we should use a reference and * rather than a pointer and get(). > Source/WebCore/Modules/webdatabase/DatabaseTracker.cpp:723 > + for (auto& fileName : listDirectory(originPath, String("*.db"))) { This should either just be "*.db" with no String() or ASCIILiteral("*.db"). > Source/WebCore/Modules/webdatabase/DatabaseTracker.cpp:1226 > + RefPtr<SecurityOrigin> origin = openDatabase.key; Strange that this is a RefPtr; no need to ref it. Could just do: auto& origin = openDatabase.key; Or: auto& origin = *openDatabase.key; In the second case we’d have to make a few changes below. > Source/WebCore/Modules/webdatabase/DatabaseTracker.cpp:1232 > + for (auto& databaseName : *databaseNameMap) { I am not sure that the entries in the database name maps are themselves database names. > Source/WebCore/Modules/webdatabase/DatabaseTracker.cpp:1356 > + DatabaseContext* context = database->databaseContext(); > context->setPaused(paused); No need for a local variable here.
Hunseop Jeong
Comment 22 2015-06-07 20:00:35 PDT
Hunseop Jeong
Comment 23 2015-06-07 21:20:41 PDT
(In reply to comment #21) > Comment on attachment 254258 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=254258&action=review > > I have a few comments, but none are about the new for loops themselves. > > > Source/WebCore/ChangeLog:8 > > + No new tests, no behavior chnages. > > typo Changed. > > Also, “no behavior change” is not completely a justification for no test > coverage; in this case we definitely will tolerate it. > > > Source/WebCore/Modules/mediastream/RTCDataChannel.cpp:326 > > events.clear(); > > This line of code is not needed. I removed it. > > > Source/WebCore/Modules/webaudio/WaveShaperProcessor.cpp:69 > > + WaveShaperDSPKernel* kernel = static_cast<WaveShaperDSPKernel*>(audioDSPKernel.get()); > > I think we should use a reference and * rather than a pointer and get(). I changed it to "WaveShaperDSPKernel& kernel = static_cast<WaveShaperDSPKernel&>(*audioDSPKernel);" > > > Source/WebCore/Modules/webdatabase/DatabaseTracker.cpp:723 > > + for (auto& fileName : listDirectory(originPath, String("*.db"))) { > > This should either just be "*.db" with no String() or ASCIILiteral("*.db"). Used ASCIILiteral("*.db"). > > > Source/WebCore/Modules/webdatabase/DatabaseTracker.cpp:1226 > > + RefPtr<SecurityOrigin> origin = openDatabase.key; > > Strange that this is a RefPtr; no need to ref it. Could just do: > > auto& origin = openDatabase.key; > > Or: > > auto& origin = *openDatabase.key; > > In the second case we’d have to make a few changes below. > Used the first case. > > Source/WebCore/Modules/webdatabase/DatabaseTracker.cpp:1232 > > + for (auto& databaseName : *databaseNameMap) { > > I am not sure that the entries in the database name maps are themselves > database names. > typedef HashMap<String, DatabaseSet*> DatabaseNameMap; DatabaseNameMap consists of the 'String' and the 'DatabaseSet'. Maybe 'String' is the name of databases and 'DatabaseSet' is set of the databases. I changed the 'databaseName' to 'databases'. It looks like a good name rather than 'databasName'. How about your idea? > > Source/WebCore/Modules/webdatabase/DatabaseTracker.cpp:1356 > > + DatabaseContext* context = database->databaseContext(); > > context->setPaused(paused); > > No need for a local variable here. I removed the local variable.
Hunseop Jeong
Comment 24 2015-06-07 21:23:07 PDT
Hunseop Jeong
Comment 25 2015-06-07 21:27:16 PDT
(In reply to comment #23) > (In reply to comment #21) > > Comment on attachment 254258 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=254258&action=review > > > > I have a few comments, but none are about the new for loops themselves. > > > > > Source/WebCore/ChangeLog:8 > > > + No new tests, no behavior chnages. > > > > typo > Changed. > > > > Also, “no behavior change” is not completely a justification for no test > > coverage; in this case we definitely will tolerate it. > > > > > Source/WebCore/Modules/mediastream/RTCDataChannel.cpp:326 > > > events.clear(); > > > > This line of code is not needed. > I removed it. > > > > > Source/WebCore/Modules/webaudio/WaveShaperProcessor.cpp:69 > > > + WaveShaperDSPKernel* kernel = static_cast<WaveShaperDSPKernel*>(audioDSPKernel.get()); > > > > I think we should use a reference and * rather than a pointer and get(). > I changed it to "WaveShaperDSPKernel& kernel = > static_cast<WaveShaperDSPKernel&>(*audioDSPKernel);" > > > > > Source/WebCore/Modules/webdatabase/DatabaseTracker.cpp:723 > > > + for (auto& fileName : listDirectory(originPath, String("*.db"))) { > > > > This should either just be "*.db" with no String() or ASCIILiteral("*.db"). > Used ASCIILiteral("*.db"). > > > > > Source/WebCore/Modules/webdatabase/DatabaseTracker.cpp:1226 > > > + RefPtr<SecurityOrigin> origin = openDatabase.key; > > > > Strange that this is a RefPtr; no need to ref it. Could just do: > > > > auto& origin = openDatabase.key; > > > > Or: > > > > auto& origin = *openDatabase.key; > > > > In the second case we’d have to make a few changes below. > > > Used the first case. > > > Source/WebCore/Modules/webdatabase/DatabaseTracker.cpp:1232 > > > + for (auto& databaseName : *databaseNameMap) { > > > > I am not sure that the entries in the database name maps are themselves > > database names. > > > typedef HashMap<String, DatabaseSet*> DatabaseNameMap; > > DatabaseNameMap consists of the 'String' and the 'DatabaseSet'. > Maybe 'String' is the name of databases and 'DatabaseSet' is set of the > databases. I changed the 'databaseName' to 'databases'. > It looks like a good name rather than 'databasName'. > How about your idea? Oops,, I misunderstood your comment,, I will change it correctly!! > > > > Source/WebCore/Modules/webdatabase/DatabaseTracker.cpp:1356 > > > + DatabaseContext* context = database->databaseContext(); > > > context->setPaused(paused); > > > > No need for a local variable here. > I removed the local variable.
Hunseop Jeong
Comment 26 2015-06-07 21:56:33 PDT
Hunseop Jeong
Comment 27 2015-06-08 00:03:17 PDT
(In reply to comment #25) > (In reply to comment #23) > > (In reply to comment #21) > > > Comment on attachment 254258 [details] > > > Patch > > > > > > View in context: > > > https://bugs.webkit.org/attachment.cgi?id=254258&action=review > > > > > > I have a few comments, but none are about the new for loops themselves. > > > > > > > Source/WebCore/ChangeLog:8 > > > > + No new tests, no behavior chnages. > > > > > > typo > > Changed. > > > > > > Also, “no behavior change” is not completely a justification for no test > > > coverage; in this case we definitely will tolerate it. > > > > > > > Source/WebCore/Modules/mediastream/RTCDataChannel.cpp:326 > > > > events.clear(); > > > > > > This line of code is not needed. > > I removed it. > > > > > > > Source/WebCore/Modules/webaudio/WaveShaperProcessor.cpp:69 > > > > + WaveShaperDSPKernel* kernel = static_cast<WaveShaperDSPKernel*>(audioDSPKernel.get()); > > > > > > I think we should use a reference and * rather than a pointer and get(). > > I changed it to "WaveShaperDSPKernel& kernel = > > static_cast<WaveShaperDSPKernel&>(*audioDSPKernel);" > > > > > > > Source/WebCore/Modules/webdatabase/DatabaseTracker.cpp:723 > > > > + for (auto& fileName : listDirectory(originPath, String("*.db"))) { > > > > > > This should either just be "*.db" with no String() or ASCIILiteral("*.db"). > > Used ASCIILiteral("*.db"). > > > > > > > Source/WebCore/Modules/webdatabase/DatabaseTracker.cpp:1226 > > > > + RefPtr<SecurityOrigin> origin = openDatabase.key; > > > > > > Strange that this is a RefPtr; no need to ref it. Could just do: > > > > > > auto& origin = openDatabase.key; > > > > > > Or: > > > > > > auto& origin = *openDatabase.key; > > > > > > In the second case we’d have to make a few changes below. > > > > > Used the first case. > > > > Source/WebCore/Modules/webdatabase/DatabaseTracker.cpp:1232 > > > > + for (auto& databaseName : *databaseNameMap) { > > > > > > I am not sure that the entries in the database name maps are themselves > > > database names. > > > > > typedef HashMap<String, DatabaseSet*> DatabaseNameMap; > > > > DatabaseNameMap consists of the 'String' and the 'DatabaseSet'. > > Maybe 'String' is the name of databases and 'DatabaseSet' is set of the > > databases. I changed the 'databaseName' to 'databases'. > > It looks like a good name rather than 'databasName'. > > How about your idea? > Oops,, I misunderstood your comment,, I will change it correctly!! Done!! Could you check again? > > > > > > Source/WebCore/Modules/webdatabase/DatabaseTracker.cpp:1356 > > > > + DatabaseContext* context = database->databaseContext(); > > > > context->setPaused(paused); > > > > > > No need for a local variable here. > > I removed the local variable.
WebKit Commit Bot
Comment 28 2015-06-08 07:37:17 PDT
Comment on attachment 254473 [details] Patch Clearing flags on attachment: 254473 Committed r185316: <http://trac.webkit.org/changeset/185316>
WebKit Commit Bot
Comment 29 2015-06-08 07:37:22 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.