The current implementation of toPlatformRequestCachePolicy is not correct for all cases.
Created attachment 331393 [details] Patch
Comment on attachment 331393 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=331393&action=review Please add a better explanation of why this change is needed, and consider whether more things are needed in the switch statement. > Source/WebCore/ChangeLog:10 > + The current implementation of toPlatformRequestCachePolicy is not correct for all cases. I don't think this is clear enough. Is the issue that the casting operation is returning an incorrect kCFURL* constant? I think you should just say that. Did this stuff break after the Fetch API changes? > Source/WebCore/platform/network/cf/ResourceRequestCFNet.cpp:202 > + return kCFURLRequestCachePolicyReloadIgnoringCache Do you need to do anything to support 'RefreshAnyCacheData'? It might be better to explicitly call out 'ReloadIgnoringCacheData, and DoNotUseAnyCache' before the 'default' just to show that they map to kCFURL*IgnoringCache.
Let's not have a default at all if possible.
(In reply to Brent Fulgham from comment #2) > Comment on attachment 331393 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=331393&action=review > > Please add a better explanation of why this change is needed, and consider > whether more things are needed in the switch statement. > > > Source/WebCore/ChangeLog:10 > > + The current implementation of toPlatformRequestCachePolicy is not correct for all cases. > > I don't think this is clear enough. Is the issue that the casting operation > is returning an incorrect kCFURL* constant? I think you should just say that. > > Did this stuff break after the Fetch API changes? > > > Source/WebCore/platform/network/cf/ResourceRequestCFNet.cpp:202 > > + return kCFURLRequestCachePolicyReloadIgnoringCache > > Do you need to do anything to support 'RefreshAnyCacheData'? It might be > better to explicitly call out 'ReloadIgnoringCacheData, and > DoNotUseAnyCache' before the 'default' just to show that they map to > kCFURL*IgnoringCache. After looking into this more closely, it turns out that toPlatformRequestCachePolicy is actually implemented correctly as is. It is just not called in all places where it should. I will update the patch. Thanks for reviewing!
Created attachment 331420 [details] Patch
Comment on attachment 331420 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=331420&action=review > Source/WebCore/platform/network/cf/ResourceRequestCFNet.cpp:150 > + // To avoid MSVC warning: We usually put ASSERT_NOT_REACHED here instead of a comment. Some linux compilers also want such a final return statement.
(In reply to Alex Christensen from comment #6) > Comment on attachment 331420 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=331420&action=review > > > Source/WebCore/platform/network/cf/ResourceRequestCFNet.cpp:150 > > + // To avoid MSVC warning: > > We usually put ASSERT_NOT_REACHED here instead of a comment. Some linux > compilers also want such a final return statement. Thanks for reviewing! I will update the patch before landing.
Created attachment 331512 [details] Patch
Comment on attachment 331512 [details] Patch Rejecting attachment 331512 [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-02', 'validate-changelog', '--check-oops', '--non-interactive', 331512, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit ChangeLog entry in Source/WebCore/ChangeLog contains OOPS!. Full output: http://webkit-queues.webkit.org/results/6107978
Created attachment 331517 [details] Patch
Comment on attachment 331517 [details] Patch Clearing flags on attachment: 331517 Committed r227077: <https://trac.webkit.org/changeset/227077>
<rdar://problem/36602111>