Refactor the for-loops using the ranged-for loops and auto keyword.
Created attachment 261638 [details] Patch
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.
Created attachment 261639 [details] Patch
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.
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
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
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
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
Created attachment 261711 [details] Patch
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.
(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.
(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.
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.
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.
(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.
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.
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.)
Created attachment 261809 [details] Patch
Created attachment 261821 [details] PatchWithoutCopying
(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?
(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.
(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.
Created attachment 262139 [details] Patch
Comment on attachment 262139 [details] Patch Clearing flags on attachment: 262139 Committed r190338: <http://trac.webkit.org/changeset/190338>
All reviewed patches have been landed. Closing bug.