We should use more references in ResourceLoader related code
Created attachment 280124 [details] Patch
Created attachment 280126 [details] Fixing mac build
Created attachment 280130 [details] Patch
Comment on attachment 280130 [details] Patch Attachment 280130 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/1411707 Number of test failures exceeded the failure limit.
Created attachment 280131 [details] Archive of layout-test-results from ews102 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews102 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 280130 [details] Patch Attachment 280130 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/1411718 Number of test failures exceeded the failure limit.
Created attachment 280132 [details] Archive of layout-test-results from ews106 for mac-yosemite-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews106 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5
Comment on attachment 280130 [details] Patch Attachment 280130 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/1411708 Number of test failures exceeded the failure limit.
Created attachment 280134 [details] Archive of layout-test-results from ews124 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews124 Port: ios-simulator-wk2 Platform: Mac OS X 10.11.4
Comment on attachment 280130 [details] Patch Attachment 280130 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/1411710 Number of test failures exceeded the failure limit.
Created attachment 280135 [details] Archive of layout-test-results from ews114 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews114 Port: mac-yosemite Platform: Mac OS X 10.10.5
Created attachment 280141 [details] Fixing counter issue
Created attachment 280142 [details] Fixing changelog
Comment on attachment 280142 [details] Fixing changelog Attachment 280142 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/1412330 Number of test failures exceeded the failure limit.
Created attachment 280143 [details] Archive of layout-test-results from ews102 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews102 Port: mac-yosemite Platform: Mac OS X 10.10.5
(In reply to comment #15) > Created attachment 280143 [details] > Archive of layout-test-results from ews102 for mac-yosemite > > The attached test failures were seen while running run-webkit-tests on the > mac-ews. > Bot: ews102 Port: mac-yosemite Platform: Mac OS X 10.10.5 Mac bot errors are due to issues with wpt server (see bug 158137) and are unrelated to this patch. DEBUG:web-platform-tests:Traceback (most recent call last): File "/Volumes/Data/EWS/WebKit/LayoutTests/imported/w3c/web-platform-tests/tools/wptserve/wptserve/handlers.py", line 133, in __call__ data = self.get_data(response, path, byte_ranges) File "/Volumes/Data/EWS/WebKit/LayoutTests/imported/w3c/web-platform-tests/tools/wptserve/wptserve/handlers.py", line 183, in get_data return open(path, 'rb') IOError: [Errno 24] Too many open files: '/Volumes/Data/EWS/WebKit/LayoutTests/imported/w3c/web-platform-tests/resources/testharness.js'
Comment on attachment 280142 [details] Fixing changelog View in context: https://bugs.webkit.org/attachment.cgi?id=280142&action=review > Source/WebCore/loader/cache/CachedResource.cpp:221 > if (cachedResourceLoader.frame()->page() && cachedResourceLoader.frame()->page()->inPageCache()) { This code should use the new local variable, frame. > Source/WebCore/loader/cache/CachedResourceLoader.cpp:1015 > +void CachedResourceLoader::incrementRequestCount(const CachedResource& res) Should rename "res" to "resource" when touching this code.
Created attachment 280225 [details] Fixing according comments
Thanks for the review. > > Source/WebCore/loader/cache/CachedResource.cpp:221 > > if (cachedResourceLoader.frame()->page() && cachedResourceLoader.frame()->page()->inPageCache()) { > > This code should use the new local variable, frame. Done > > Source/WebCore/loader/cache/CachedResourceLoader.cpp:1015 > > +void CachedResourceLoader::incrementRequestCount(const CachedResource& res) > > Should rename "res" to "resource" when touching this code. Done
Comment on attachment 280225 [details] Fixing according comments View in context: https://bugs.webkit.org/attachment.cgi?id=280225&action=review > Source/WebCore/css/CSSFontFaceSource.cpp:127 > - fontSelector.beginLoadingFontSoon(m_font.get()); > + fontSelector.beginLoadingFontSoon(*m_font); I don't think m_font is guaranteed to not be null. > Source/WebKit2/WebProcess/Network/WebLoaderStrategy.cpp:298 > for (HashMap<unsigned long, RefPtr<WebResourceLoader>>::iterator i = m_webResourceLoaders.begin(); i != end; ++i) This should be made into a c++11-style forloop.
(In reply to comment #20) > Comment on attachment 280225 [details] > Fixing according comments > > View in context: > https://bugs.webkit.org/attachment.cgi?id=280225&action=review > > > Source/WebCore/css/CSSFontFaceSource.cpp:127 > > - fontSelector.beginLoadingFontSoon(m_font.get()); > > + fontSelector.beginLoadingFontSoon(*m_font); > > I don't think m_font is guaranteed to not be null. If m_font is null, status is set to Success and load() will not be called. But I agree it may be easy to break that assumption. I will add an ASSERT(m_font). > > Source/WebKit2/WebProcess/Network/WebLoaderStrategy.cpp:298 > > for (HashMap<unsigned long, RefPtr<WebResourceLoader>>::iterator i = m_webResourceLoaders.begin(); i != end; ++i) > > This should be made into a c++11-style forloop. OK
Comment on attachment 280225 [details] Fixing according comments View in context: https://bugs.webkit.org/attachment.cgi?id=280225&action=review > Source/WebCore/loader/NetscapePlugInStreamLoader.cpp:60 > RefPtr<NetscapePlugInStreamLoader> loader(adoptRef(new NetscapePlugInStreamLoader(frame, client))); Should use auto. > Source/WebCore/loader/ResourceLoader.cpp:63 > + : m_frame(&frame) m_frame should be a Ref instead a RefPtr, I think > Source/WebCore/loader/ResourceLoader.cpp:68 > , m_identifier(0) > , m_reachedTerminalState(false) > , m_notifiedLoadComplete(false) > , m_cancellationStatus(NotCancelled) Should initialize these scalars in the header instead of here. Same for m_resourceType and m_isQuickLookResource.
Created attachment 280314 [details] Patch for landing
Comment on attachment 280314 [details] Patch for landing Rejecting attachment 280314 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'build', '--no-clean', '--no-update', '--build-style=release', '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit Last 500 characters of output: /EWS/WebKit/Source/WebKit2/WebProcess/WebCoreSupport/WebPasteboardOverrides.cpp -o /Volumes/Data/EWS/WebKit/WebKitBuild/WebKit2.build/Release/WebKit.build/Objects-normal/x86_64/WebPasteboardOverrides.o ** BUILD FAILED ** The following build commands failed: CompileC /Volumes/Data/EWS/WebKit/WebKitBuild/WebKit2.build/Release/WebKit.build/Objects-normal/x86_64/WebLoaderStrategy.o WebProcess/Network/WebLoaderStrategy.cpp normal x86_64 c++ com.apple.compilers.llvm.clang.1_0.compiler (1 failure) Full output: http://webkit-queues.webkit.org/results/1421983
Created attachment 280317 [details] Patch for landing
Comment on attachment 280317 [details] Patch for landing Clearing flags on attachment: 280317 Committed r201596: <http://trac.webkit.org/changeset/201596>
All reviewed patches have been landed. Closing bug.
Thanks for both reviews. Except for m_frame which currently needs to be a RefPtr, I integrated all feedback.