Bug 149397 – Use modern for-loops in WebCore/loader.

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Hunseop Jeong]

Refactor the for-loops using the ranged-for loops and auto keyword.

[Hunseop Jeong]

Created attachment 261638 [details]
Patch

[WebKit Commit Bot]

Attachment 261638 [details] did not pass style-queue:


ERROR: Source/WebCore/loader/icon/IconDatabase.cpp:193:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/WebCore/loader/icon/IconDatabase.cpp:777:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/WebCore/loader/icon/IconDatabase.cpp:815:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/WebCore/loader/icon/IconDatabase.cpp:816:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/WebCore/loader/archive/cf/LegacyWebArchive.cpp:250:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 5 in 19 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Hunseop Jeong]

Created attachment 261639 [details]
Patch

[WebKit Commit Bot]

Attachment 261639 [details] did not pass style-queue:


ERROR: Source/WebCore/loader/icon/IconDatabase.cpp:193:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/WebCore/loader/icon/IconDatabase.cpp:777:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/WebCore/loader/icon/IconDatabase.cpp:815:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/WebCore/loader/icon/IconDatabase.cpp:816:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/WebCore/loader/archive/cf/LegacyWebArchive.cpp:250:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 5 in 19 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 261639 [details]
Patch

Attachment 261639 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/192555

New failing tests:
fast/events/before-unload-remove-and-add-subframe.html
fast/events/before-unload-adopt-subframe-to-outside.html

[Build Bot]

Created attachment 261642 [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 261639 [details]
Patch

Attachment 261639 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/192591

New failing tests:
fast/events/before-unload-remove-and-add-subframe.html
fast/events/before-unload-adopt-subframe-to-outside.html

[Build Bot]

Created attachment 261643 [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 261711 [details]
Patch

[WebKit Commit Bot]

Attachment 261711 [details] did not pass style-queue:


ERROR: Source/WebCore/loader/icon/IconDatabase.cpp:193:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/WebCore/loader/icon/IconDatabase.cpp:777:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/WebCore/loader/icon/IconDatabase.cpp:815:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/WebCore/loader/icon/IconDatabase.cpp:816:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/WebCore/loader/archive/cf/LegacyWebArchive.cpp:250:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 5 in 19 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Hunseop Jeong]

(In reply to comment #10)
> Attachment 261711 [details] did not pass style-queue:
> 
> 
> ERROR: Source/WebCore/loader/icon/IconDatabase.cpp:193:  Multi line control
> clauses should use braces.  [whitespace/braces] [4]
> ERROR: Source/WebCore/loader/icon/IconDatabase.cpp:777:  Multi line control
> clauses should use braces.  [whitespace/braces] [4]
> ERROR: Source/WebCore/loader/icon/IconDatabase.cpp:815:  Multi line control
> clauses should use braces.  [whitespace/braces] [4]
> ERROR: Source/WebCore/loader/icon/IconDatabase.cpp:816:  Multi line control
> clauses should use braces.  [whitespace/braces] [4]
> ERROR: Source/WebCore/loader/archive/cf/LegacyWebArchive.cpp:250:  Multi
> line control clauses should use braces.  [whitespace/braces] [4]
> Total errors found: 5 in 19 files
> 
> 
> If any of these errors are false positives, please file a bug against
> check-webkit-style.


It looks false alarm. These modified codes don't need the braces.

[Chris Dumez]

(In reply to comment #11)
> (In reply to comment #10)
> > Attachment 261711 [details] did not pass style-queue:
> > 
> > 
> > ERROR: Source/WebCore/loader/icon/IconDatabase.cpp:193:  Multi line control
> > clauses should use braces.  [whitespace/braces] [4]
> > ERROR: Source/WebCore/loader/icon/IconDatabase.cpp:777:  Multi line control
> > clauses should use braces.  [whitespace/braces] [4]
> > ERROR: Source/WebCore/loader/icon/IconDatabase.cpp:815:  Multi line control
> > clauses should use braces.  [whitespace/braces] [4]
> > ERROR: Source/WebCore/loader/icon/IconDatabase.cpp:816:  Multi line control
> > clauses should use braces.  [whitespace/braces] [4]
> > ERROR: Source/WebCore/loader/archive/cf/LegacyWebArchive.cpp:250:  Multi
> > line control clauses should use braces.  [whitespace/braces] [4]
> > Total errors found: 5 in 19 files
> > 
> > 
> > If any of these errors are false positives, please file a bug against
> > check-webkit-style.
> 
> 
> It looks false alarm. These modified codes don't need the braces.

I don't think they are. This usually means you have extra spaces on the next line. You should remove those extra spaces.

[Chris Dumez]

Comment on attachment 261711 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=261711&action=review

> Source/WebCore/loader/icon/IconDatabase.cpp:195
>              

For example, you probably have white spaces on this line.

[Darin Adler]

Comment on attachment 261711 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=261711&action=review

> Source/WebCore/loader/DocumentLoader.cpp:1146
> +        RefPtr<ResourceLoader> loader = entry.key;

No need for the reference count churn here. Could just be:

    auto& loader = entry.key;

> Source/WebCore/loader/FrameLoader.cpp:1860
> +        for (auto& response : m_documentLoader->responses()) {

I think that in both the old and new code this is risky. What if sendRemainingDelegateMessages calls something that ends up calling back through to us and deletes the object currently in m_documentLoader or adds responses to the vector? We probably need to make a copy of the response vector and iterate that. Not sure how to test this.

> Source/WebCore/loader/appcache/ApplicationCache.cpp:156
> +    for (auto& whiteURL : m_onlineWhitelist) {

I think whitelistURL would be better than whiteURL.

> Source/WebCore/loader/appcache/ApplicationCacheGroup.cpp:734
> +    for (auto& loader : m_pendingMasterResourceLoaders)
> +        associateDocumentLoaderWithCache(loader, m_cacheBeingUpdated.get());
> +

Why the new extra blank line here? Please don’t include that.

> Source/WebCore/loader/appcache/ApplicationCacheHost.cpp:308
> +    for (auto& deferred : m_deferredEvents)

I think I’d call this “event” rather than “deferred”.

> Source/WebCore/loader/appcache/ApplicationCacheStorage.cpp:753
> +        for (auto& url : onlineWhitelist) {

I think whitelistURL is probably the best name for this, better than just url.

> Source/WebCore/loader/archive/cf/LegacyWebArchive.cpp:138
> +        RetainPtr<CFDictionaryRef> subresource = createPropertyListRepresentation(resource.get(), Subresource);
>          if (subresource)

I’d suggest putting the RetainPtr inside the if.

> Source/WebCore/loader/archive/cf/LegacyWebArchive.cpp:149
> +        RetainPtr<CFDictionaryRef> subframeArchive = createPropertyListRepresentation(subframe.get());
>          if (subframeArchive)

I’d suggest putting the RetainPtr inside the if.

> Source/WebCore/loader/cache/CachedResourceLoader.cpp:1093
> +    for (auto& res : m_preloads) {

Would be nice to rename this resource instead of "res".

> Source/WebCore/loader/icon/IconDatabase.cpp:817
> +    for (auto& loader : m_loadersPendingDecision)
> +        if (loader->refCount() > 1)
> +            loader->iconLoadDecisionAvailable();

Should have braces for WebKit coding style.

[Hunseop Jeong]

(In reply to comment #13)
> Comment on attachment 261711 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=261711&action=review
> 
> > Source/WebCore/loader/icon/IconDatabase.cpp:195
> >              
> 
> For example, you probably have white spaces on this line.

Oops,, I didn't see the white spaces.. I will remove the white spaces.

[Hunseop Jeong]

Comment on attachment 261711 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=261711&action=review

>> Source/WebCore/loader/FrameLoader.cpp:1860
>> +        for (auto& response : m_documentLoader->responses()) {
> 
> I think that in both the old and new code this is risky. What if sendRemainingDelegateMessages calls something that ends up calling back through to us and deletes the object currently in m_documentLoader or adds responses to the vector? We probably need to make a copy of the response vector and iterate that. Not sure how to test this.

You mean, We need to make a copy of the response vector to escape the risk.
Like as : 

        Vector<ResourceResponse> responses;
        responses.reserveCapacity(m_documentLoader->responses().size());
        for (auto& response : m_documentLoader->responses())
            responses.append(response);

        for (auto& response : responses) {
            ...

>> Source/WebCore/loader/appcache/ApplicationCache.cpp:156
>> +    for (auto& whiteURL : m_onlineWhitelist) {
> 
> I think whitelistURL would be better than whiteURL.

Okay, Naming is very hard work.

[Darin Adler]

Comment on attachment 261711 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=261711&action=review

>>> Source/WebCore/loader/FrameLoader.cpp:1860
>>> +        for (auto& response : m_documentLoader->responses()) {
>> 
>> I think that in both the old and new code this is risky. What if sendRemainingDelegateMessages calls something that ends up calling back through to us and deletes the object currently in m_documentLoader or adds responses to the vector? We probably need to make a copy of the response vector and iterate that. Not sure how to test this.
> 
> You mean, We need to make a copy of the response vector to escape the risk.
> Like as : 
> 
>         Vector<ResourceResponse> responses;
>         responses.reserveCapacity(m_documentLoader->responses().size());
>         for (auto& response : m_documentLoader->responses())
>             responses.append(response);
> 
>         for (auto& response : responses) {
>             ...

Yes, copying the response vector is one thing that might help. It also might have a performance cost or create other problems, so I think we need to look into testing this, not just writing code that looks right.

If we do want to copy it, it’s actually much easier to copy than the code above. It’s this one-liner:

    auto responsesCopy = m_documentLoader->responses();

No need for a loop with reserveCapacity and append. (And if we did write the loop, it should be with reserveInitialCapacity and uncheckedAppend instead.)

[Hunseop Jeong]

Created attachment 261809 [details]
Patch

[Hunseop Jeong]

Created attachment 261821 [details]
PatchWithoutCopying

[Hunseop Jeong]

(In reply to comment #17)
> Comment on attachment 261711 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=261711&action=review
> 
> >>> Source/WebCore/loader/FrameLoader.cpp:1860
> >>> +        for (auto& response : m_documentLoader->responses()) {
> >> 
> >> I think that in both the old and new code this is risky. What if sendRemainingDelegateMessages calls something that ends up calling back through to us and deletes the object currently in m_documentLoader or adds responses to the vector? We probably need to make a copy of the response vector and iterate that. Not sure how to test this.
> > 
> > You mean, We need to make a copy of the response vector to escape the risk.
> > Like as : 
> > 
> >         Vector<ResourceResponse> responses;
> >         responses.reserveCapacity(m_documentLoader->responses().size());
> >         for (auto& response : m_documentLoader->responses())
> >             responses.append(response);
> > 
> >         for (auto& response : responses) {
> >             ...
> 
> Yes, copying the response vector is one thing that might help. It also might
> have a performance cost or create other problems, so I think we need to look
> into testing this, not just writing code that looks right.
> 
> If we do want to copy it, it’s actually much easier to copy than the code
> above. It’s this one-liner:
> 
>     auto responsesCopy = m_documentLoader->responses();
> 
> No need for a loop with reserveCapacity and append. (And if we did write the
> loop, it should be with reserveInitialCapacity and uncheckedAppend instead.)
Other comments are done.

How to look into testing this?
Is it covered by existing layoutTest or PerformanceTest?

If we need the deep testing, could you upload this patch without the copying code?

[Darin Adler]

(In reply to comment #20)
> How to look into testing this?

That’s an excellent question. I don’t know exactly how to test it, other than the suggestions I made above about sendRemainingDelegateMessages. We somehow have to make a test case where those delegate messages trigger a change in the frame loader, probably by loading a new page in the frame, but with a load that is as synchronous as possible (perhaps loading about:blank).

> Is it covered by existing layoutTest or PerformanceTest?

Definitely not, because if there is a bug here, it already existed in the old code.

> If we need the deep testing, could you upload this patch without the copying
> code?

Sure, we can do refactoring without fixing this, but someone should really be following up to explore whether this is a bug.

[Hunseop Jeong]

(In reply to comment #21)
> (In reply to comment #20)
> > How to look into testing this?
> 
> That’s an excellent question. I don’t know exactly how to test it, other
> than the suggestions I made above about sendRemainingDelegateMessages. We
> somehow have to make a test case where those delegate messages trigger a
> change in the frame loader, probably by loading a new page in the frame, but
> with a load that is as synchronous as possible (perhaps loading about:blank).
> 
> > Is it covered by existing layoutTest or PerformanceTest?
> 
> Definitely not, because if there is a bug here, it already existed in the
> old code.
> 
> > If we need the deep testing, could you upload this patch without the copying
> > code?
> 
> Sure, we can do refactoring without fixing this, but someone should really
> be following up to explore whether this is a bug.
Thanks. I also try to explore whether this is a bug.

[Hunseop Jeong]

Created attachment 262139 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 262139 [details]
Patch

Clearing flags on attachment: 262139

Committed r190338: <http://trac.webkit.org/changeset/190338>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.