| Summary: | Use modern for-loops in WebCore/Modules - 2 | ||
|---|---|---|---|
| Product: | WebKit | Reporter: | Hunseop Jeong <hs85.jeong> |
| Component: | WebCore Misc. | Assignee: | Hunseop Jeong <hs85.jeong> |
| Status: | RESOLVED FIXED | ||
| Severity: | Normal | CC: | buildbot, commit-queue, darin, rniwa |
| Priority: | P2 | ||
| Version: | 528+ (Nightly build) | ||
| Hardware: | Unspecified | ||
| OS: | Unspecified | ||
| Attachments: | |||
|
Description
Hunseop Jeong
2015-06-01 19:09:30 PDT
Created attachment 254137 [details]
Patch
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 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
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 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
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 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
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 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
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 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
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 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
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 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
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 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
Created attachment 254258 [details]
Patch
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. 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. 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. Created attachment 254468 [details]
Patch
(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. Created attachment 254471 [details]
Patch
(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. Created attachment 254473 [details]
Patch
(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. Comment on attachment 254473 [details] Patch Clearing flags on attachment: 254473 Committed r185316: <http://trac.webkit.org/changeset/185316> All reviewed patches have been landed. Closing bug. |