Summary: | Update CachedResourceLoader::requestResource() to return a WTF::Expected | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Chris Dumez <cdumez> | ||||||||||
Component: | WebCore Misc. | Assignee: | Chris Dumez <cdumez> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | cdumez, commit-queue, ggaren, sam, webkit-bug-importer, youennf | ||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||
Version: | WebKit Nightly Build | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Bug Depends on: | 175482 | ||||||||||||
Bug Blocks: | |||||||||||||
Attachments: |
|
Description
Chris Dumez
2017-08-11 16:55:22 PDT
Created attachment 318006 [details]
Patch
@Sam: Here is what it looks like. Created attachment 318010 [details]
Patch
Comment on attachment 318010 [details] 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&. Created attachment 318044 [details]
Patch
Created attachment 318149 [details]
Patch
Comment on attachment 318149 [details] Patch Clearing flags on attachment: 318149 Committed r220760: <http://trac.webkit.org/changeset/220760> All reviewed patches have been landed. Closing bug. |