[webkit-reviews] review granted: [Bug 175505] Update CachedResourceLoader::requestResource() to return a WTF::Expected : [Attachment 318010] Patch

[bugzilla-daemon at webkit.org]

youenn fablet <youennf at gmail.com> has granted Chris Dumez <cdumez at apple.com>'s
request for review:
Bug 175505: Update CachedResourceLoader::requestResource() to return a
WTF::Expected
https://bugs.webkit.org/show_bug.cgi?id=175505

Attachment 318010: Patch

https://bugs.webkit.org/attachment.cgi?id=318010&action=review




--- Comment #4 from youenn fablet <youennf at gmail.com> ---
Comment on attachment 318010
  --> https://bugs.webkit.org/attachment.cgi?id=318010
Patch

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

> Source/WebCore/loader/cache/CachedResourceLoader.cpp:93
> +	   return CachedResourceHandle<T> {
static_cast<T*>(cachedResource.value().get()) };

In changed code, a downcast was used. Why using a static_cast here?

> Source/WebCore/loader/cache/CachedResourceLoader.cpp:729
> +		   CachedResourceHandle<CachedResource> resource =
createResource(type, WTFMove(request), sessionID());

Can we make createResource return a CachedResourceHandle<CachedResource>
instead?

> Source/WebCore/loader/cache/CachedResourceLoader.cpp:800
> +	       if (!shouldContinueAfterNotifyingLoadedFromMemoryCache(request,
resource.get(), error))

Would returning an optional make it better?

> Source/WebCore/loader/cache/CachedResourceLoader.cpp:839
> +		   return makeUnexpected(ResourceError { String(), 0, url,
String(), ResourceError::Type::AccessControl });

AccessControl is usually the reason of immediate failures.
Would be cool to have not null resource errors in the future.

> Source/WebCore/loader/cache/CachedResourceLoader.cpp:1258
> +	   auto resourceValue = resource.value();

auto& maybe?

> Source/WebCore/loader/cache/CachedResourceLoader.h:178
> +    bool shouldContinueAfterNotifyingLoadedFromMemoryCache(const
CachedResourceRequest&, CachedResource*, ResourceError&);

Pre-existing code issue but it would seem that we should pass a const
CachedResource* and probably even better a const CachedResource&.