Bug 163332

Created attachment 291357 [details] Patch

Attachment 291357 [details] did not pass style-queue: ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCache.cpp:312: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCache.h:109: Extra space before ( in function call [whitespace/parens] [4] Total errors found: 2 in 13 files If any of these errors are false positives, please file a bug against check-webkit-style.

(In reply to comment #1) > Created attachment 291357 [details] > Patch I am not exactly sure what is the best option to pass the additional cache mode information from WebCore to NetworkProcess. There seems to be two choices: 1 Pass FetchOptions::Cache information to NetworkProcess 2 Update ResourceRequestCachePolicy to add more modes: NoCache, NoStore and Reload I chose option 1 since it is the simplest. I would prefer option 2: - No need to add code to pass the new parameter - No need to check two different parameters in NetworkCache with slightly different meanings - WK1 could eventually catch up if CF network cache supports these new options. I chose option 1 since I was not sure whether there is a strong requirement to keep the resource request cache policy aligned with CFNetwork cache policy. This patch is pending bug 161310. If that is not possible to go forward with bug 161310, it might be possible to split the test file in several so that the console log issue will not hit all tests.

> I chose option 1 since I was not sure whether there is a strong requirement > to keep the resource request cache policy aligned with CFNetwork cache > policy. I think we should consider ResourceRequest/Response as pure WebCore types as opposed to wrappers for CFNetwork types. We shouldn't worry about keeping them aligned.

Created attachment 291469 [details] Patch

Created attachment 291472 [details] Patch

Comment on attachment 291472 [details] Patch Attachment 291472 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/2276449 Number of test failures exceeded the failure limit.

Created attachment 291473 [details] Archive of layout-test-results from ews114 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews114 Port: mac-yosemite Platform: Mac OS X 10.10.5

Created attachment 291475 [details] Fixing assert and builds

Comment on attachment 291475 [details] Fixing assert and builds View in context: https://bugs.webkit.org/attachment.cgi?id=291475&action=review > Source/WebCore/platform/network/ResourceRequestBase.h:44 > + UseProtocolCachePolicy, // normal load, equivalent to fetch "default" cache mode. > + ReloadIgnoringCacheData, // reload, equivalent to fetch "reload"cache mode. > + ReturnCacheDataElseLoad, // back/forward or encoding change - allow stale data, equivalent to fetch "force-cache" cache mode. > + ReturnCacheDataDontLoad, // results of a post - allow stale data and only use cache, equivalent to fetch "only-if-cached" cache mode. > + DoNotUseAnyCache, // Bypass the cache entirely, equivalent to fetch "no-store" cache mode. > + RefreshAnyCacheData, // Serve cache data only if revalidated, equivalent to fetch "no-cache" mode. In future it would be good to also improve the existing names so they would require fewer comments to understand. The old names are straight from CFNetwork. > Source/WebCore/platform/network/cf/ResourceRequestCFNet.cpp:176 > +static inline CFURLRequestCachePolicy toPlatformRequestCachePolicy(ResourceRequestCachePolicy policy) > +{ > + return (CFURLRequestCachePolicy)((policy <= ReturnCacheDataDontLoad) ? policy : ReloadIgnoringCacheData); > +} At least it this should use static_cast but it would really be more understandable with a switch. > Source/WebCore/platform/network/cocoa/ResourceRequestCocoa.mm:69 > + m_cachePolicy = (ResourceRequestCachePolicy)[m_nsRequest.get() cachePolicy]; Please use static_cast or better yet, add a conversion function. > Source/WebCore/platform/network/cocoa/ResourceRequestCocoa.mm:120 > > +static inline NSURLRequestCachePolicy toPlatformRequestCachePolicy(ResourceRequestCachePolicy policy) > +{ > + return (NSURLRequestCachePolicy)((policy <= ReturnCacheDataDontLoad) ? policy : ReloadIgnoringCacheData); > +} Same comment as above.

Created attachment 291583 [details] Patch for landing

Thanks for the review. (In reply to comment #10) > Comment on attachment 291475 [details] > Fixing assert and builds > > View in context: > https://bugs.webkit.org/attachment.cgi?id=291475&action=review > > > Source/WebCore/platform/network/ResourceRequestBase.h:44 > > + UseProtocolCachePolicy, // normal load, equivalent to fetch "default" cache mode. > > + ReloadIgnoringCacheData, // reload, equivalent to fetch "reload"cache mode. > > + ReturnCacheDataElseLoad, // back/forward or encoding change - allow stale data, equivalent to fetch "force-cache" cache mode. > > + ReturnCacheDataDontLoad, // results of a post - allow stale data and only use cache, equivalent to fetch "only-if-cached" cache mode. > > + DoNotUseAnyCache, // Bypass the cache entirely, equivalent to fetch "no-store" cache mode. > > + RefreshAnyCacheData, // Serve cache data only if revalidated, equivalent to fetch "no-cache" mode. > > In future it would be good to also improve the existing names so they would > require fewer comments to understand. The old names are straight from > CFNetwork. Agreed. > > Source/WebCore/platform/network/cf/ResourceRequestCFNet.cpp:176 > > +static inline CFURLRequestCachePolicy toPlatformRequestCachePolicy(ResourceRequestCachePolicy policy) > > +{ > > + return (CFURLRequestCachePolicy)((policy <= ReturnCacheDataDontLoad) ? policy : ReloadIgnoringCacheData); > > +} > > At least it this should use static_cast but it would really be more > understandable with a switch. Updated to use a switch > > Source/WebCore/platform/network/cocoa/ResourceRequestCocoa.mm:69 > > + m_cachePolicy = (ResourceRequestCachePolicy)[m_nsRequest.get() cachePolicy]; > > Please use static_cast or better yet, add a conversion function. Forgot this one in the landing patch... Will add it. > > Source/WebCore/platform/network/cocoa/ResourceRequestCocoa.mm:120 > > > > +static inline NSURLRequestCachePolicy toPlatformRequestCachePolicy(ResourceRequestCachePolicy policy) > > +{ > > + return (NSURLRequestCachePolicy)((policy <= ReturnCacheDataDontLoad) ? policy : ReloadIgnoringCacheData); > > +} > > Same comment as above. Updated to use a switch.

Created attachment 291586 [details] Patch for landing

Comment on attachment 291586 [details] Patch for landing Clearing flags on attachment: 291586 Committed r207330: <http://trac.webkit.org/changeset/207330>

All reviewed patches have been landed. Closing bug.

(In reply to comment #14) > Comment on attachment 291586 [details] > Patch for landing > > Clearing flags on attachment: 291586 > > Committed r207330: <http://trac.webkit.org/changeset/207330> It broke the Apple Windows build, see build.webkit.org for details.

(In reply to comment #16) > (In reply to comment #14) > > Comment on attachment 291586 [details] > > Patch for landing > > > > Clearing flags on attachment: 291586 > > > > Committed r207330: <http://trac.webkit.org/changeset/207330> > > It broke the Apple Windows build, see build.webkit.org for details. Thanks, I will check this!