Bug 158222 – Use more references in ResourceLoader related code

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

[youenn fablet]

We should use more references in ResourceLoader related code

[youenn fablet]

Created attachment 280124 [details]
Patch

[youenn fablet]

Created attachment 280126 [details]
Fixing mac build

[youenn fablet]

Created attachment 280130 [details]
Patch

[Build Bot]

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.

[Build Bot]

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

[Build Bot]

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.

[Build Bot]

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

[Build Bot]

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.

[Build Bot]

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

[Build Bot]

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.

[Build Bot]

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

[youenn fablet]

Created attachment 280141 [details]
Fixing counter issue

[youenn fablet]

Created attachment 280142 [details]
Fixing changelog

[Build Bot]

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.

[Build Bot]

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

[youenn fablet]

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

[Darin Adler]

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.

[youenn fablet]

Created attachment 280225 [details]
Fixing according comments

[youenn fablet]

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

[Alex Christensen]

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.

[youenn fablet]

(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

[Darin Adler]

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.

[youenn fablet]

Created attachment 280314 [details]
Patch for landing

[WebKit Commit Bot]

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

[youenn fablet]

Created attachment 280317 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 280317 [details]
Patch for landing

Clearing flags on attachment: 280317

Committed r201596: <http://trac.webkit.org/changeset/201596>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[youenn fablet]

Thanks for both reviews.
Except for m_frame which currently needs to be a RefPtr, I integrated all feedback.