WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
Archive of layout-test-results from ews102 for mac-mavericks
(
deleted
)
2015-06-03 04:32 PDT
,
Build Bot
no flags
Details
Archive of layout-test-results from ews104 for mac-mavericks-wk2
(
deleted
)
2015-06-03 05:36 PDT
,
Build Bot
no flags
Details
Archive of layout-test-results from ews106 for mac-mavericks-wk2
(
deleted
)
2015-06-03 06:59 PDT
,
Build Bot
no flags
Details
Archive of layout-test-results from ews100 for mac-mavericks
(
deleted
)
2015-06-03 07:46 PDT
,
Build Bot
no flags
Details
Archive of layout-test-results from ews105 for mac-mavericks-wk2
(
deleted
)
2015-06-03 08:39 PDT
,
Build Bot
no flags
Details
Archive of layout-test-results from ews101 for mac-mavericks
(
deleted
)
2015-06-03 09:05 PDT
,
Build Bot
no flags
Details
Archive of layout-test-results from ews103 for mac-mavericks
(
deleted
)
2015-06-03 11:55 PDT
,
Build Bot
no flags
Details
Archive of layout-test-results from ews107 for mac-mavericks-wk2
(
deleted
)
2015-06-03 12:45 PDT
,
Build Bot
no flags
Details
Patch
(49.91 KB, patch)
2015-06-04 00:42 PDT
,
Hunseop Jeong
no flags
Details
Formatted Diff
Diff
Patch
(50.65 KB, patch)
2015-06-07 20:00 PDT
,
Hunseop Jeong
no flags
Details
Formatted Diff
Diff
Patch
(50.64 KB, patch)
2015-06-07 21:23 PDT
,
Hunseop Jeong
no flags
Details
Formatted Diff
Diff
Patch
(49.84 KB, patch)
2015-06-07 21:56 PDT
,
Hunseop Jeong
no flags
Details
Formatted Diff
Diff
Show Obsolete
(5)
View All
Add attachment
proposed patch, testcase, etc.
Hunseop Jeong
Comment 1
2015-06-02 20:48:50 PDT
Created
attachment 254137
[details]
Patch
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
Created
attachment 254258
[details]
Patch
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
Created
attachment 254468
[details]
Patch
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
Created
attachment 254471
[details]
Patch
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
Created
attachment 254473
[details]
Patch
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.
Top of Page
Format For Printing
XML
Clone This Bug