Bug 181686 – [Win] Use switch when converting from ResourceRequestCachePolicy to platform cache policy.

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

[Per Arne Vollan]

The current implementation of toPlatformRequestCachePolicy is not correct for all cases.

[Per Arne Vollan]

Created attachment 331393 [details]
Patch

[Brent Fulgham]

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.

[Alex Christensen]

Let's not have a default at all if possible.

[Per Arne Vollan]

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

[Per Arne Vollan]

Created attachment 331420 [details]
Patch

[Alex Christensen]

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.

[Per Arne Vollan]

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

[Per Arne Vollan]

Created attachment 331512 [details]
Patch

[WebKit Commit Bot]

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

[Per Arne Vollan]

Created attachment 331517 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 331517 [details]
Patch

Clearing flags on attachment: 331517

Committed r227077: <https://trac.webkit.org/changeset/227077>

[Radar WebKit Bug Importer]

<rdar://problem/36602111>