Bug 181686 - [Win] Use switch when converting from ResourceRequestCachePolicy to platform cache policy.
Summary: [Win] Use switch when converting from ResourceRequestCachePolicy to platform ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Per Arne Vollan
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-01-16 09:20 PST by Per Arne Vollan
Modified: 2018-01-17 17:10 PST (History)
4 users (show)

See Also:


Attachments
Patch (1.90 KB, patch)
2018-01-16 09:23 PST, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (4.08 KB, patch)
2018-01-16 13:06 PST, Per Arne Vollan
achristensen: review+
Details | Formatted Diff | Diff
Patch (4.14 KB, patch)
2018-01-17 10:40 PST, Per Arne Vollan
commit-queue: commit-queue-
Details | Formatted Diff | Diff
Patch (4.14 KB, patch)
2018-01-17 10:48 PST, Per Arne Vollan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Per Arne Vollan 2018-01-16 09:20:14 PST
The current implementation of toPlatformRequestCachePolicy is not correct for all cases.
Comment 1 Per Arne Vollan 2018-01-16 09:23:08 PST
Created attachment 331393 [details]
Patch
Comment 2 Brent Fulgham 2018-01-16 09:54:08 PST
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.
Comment 3 Alex Christensen 2018-01-16 10:25:43 PST
Let's not have a default at all if possible.
Comment 4 Per Arne Vollan 2018-01-16 12:59:00 PST
(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!
Comment 5 Per Arne Vollan 2018-01-16 13:06:31 PST
Created attachment 331420 [details]
Patch
Comment 6 Alex Christensen 2018-01-17 10:10:48 PST
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.
Comment 7 Per Arne Vollan 2018-01-17 10:14:45 PST
(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.
Comment 8 Per Arne Vollan 2018-01-17 10:40:18 PST
Created attachment 331512 [details]
Patch
Comment 9 WebKit Commit Bot 2018-01-17 10:45:27 PST
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
Comment 10 Per Arne Vollan 2018-01-17 10:48:17 PST
Created attachment 331517 [details]
Patch
Comment 11 WebKit Commit Bot 2018-01-17 11:35:25 PST
Comment on attachment 331517 [details]
Patch

Clearing flags on attachment: 331517

Committed r227077: <https://trac.webkit.org/changeset/227077>
Comment 12 Radar WebKit Bug Importer 2018-01-17 17:10:30 PST
<rdar://problem/36602111>