Bug 145541 – Use modern for-loops in WebCore/Modules - 2

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Hunseop Jeong]

Use the modern for-loops in WebCore/Modules - 2 of 2.

[Hunseop Jeong]

Created attachment 254137 [details]
Patch

[Build Bot]

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]

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 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]

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 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]

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 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]

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 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]

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 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]

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 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]

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 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]

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]

Created attachment 254258 [details]
Patch

[Hunseop Jeong]

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 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 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]

Created attachment 254468 [details]
Patch

[Hunseop Jeong]

(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]

Created attachment 254471 [details]
Patch

[Hunseop Jeong]

(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]

Created attachment 254473 [details]
Patch

[Hunseop Jeong]

(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 on attachment 254473 [details]
Patch

Clearing flags on attachment: 254473

Committed r185316: <http://trac.webkit.org/changeset/185316>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.