Bug 196289

CacheStorageEngine doesn't work because following files are unimplemented yet. Source/WebKit/NetworkProcess/cache/NetworkCacheDataCurl.cpp Source/WebKit/NetworkProcess/cache/NetworkCacheIOChannelCurl.cpp

Created attachment 366805 [details] WIP patch

Created attachment 367027 [details] WIP patch

Is it possible to use more of FileSystem and file mapping to make the network cache platform agnostic? It feels like some of your patch is going this direction so I figured I'd just ask your opinion on it.

Yeah, I have the same question in my mind.

Created attachment 367926 [details] Patch

Comment on attachment 367926 [details] Patch Attachment 367926 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: https://webkit-queues.webkit.org/results/11964268 New failing tests: imported/w3c/web-platform-tests/service-workers/service-worker/fetch-event-within-sw.https.html http/tests/cache-storage/cache-origins.https.html imported/w3c/web-platform-tests/service-workers/cache-storage/serviceworker/cache-keys-attributes-for-service-worker.https.html http/tests/cache-storage/cache-persistency.https.html

Created attachment 367983 [details] Archive of layout-test-results from ews126 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews126 Port: ios-simulator-wk2 Platform: Mac OS X 10.13.6

Comment on attachment 367926 [details] Patch Attachment 367926 [details] did not pass mac-wk2-ews (mac-wk2): Output: https://webkit-queues.webkit.org/results/11967880 New failing tests: imported/w3c/web-platform-tests/service-workers/service-worker/fetch-event-within-sw.https.html imported/w3c/web-platform-tests/service-workers/cache-storage/serviceworker/cache-keys-attributes-for-service-worker.https.html http/tests/cache-storage/cache-origins.https.html http/tests/cache-storage/cache-records-persistency.https.html http/tests/cache-storage/cache-persistency.https.html

Created attachment 368006 [details] Archive of layout-test-results from ews107 for mac-highsierra-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews107 Port: mac-highsierra-wk2 Platform: Mac OS X 10.13.6

Created attachment 369239 [details] Patch

Created attachment 369244 [details] Patch

Created attachment 369248 [details] Patch

Created attachment 369250 [details] Patch

Comment on attachment 369250 [details] Patch Attachment 369250 [details] did not pass mac-wk2-ews (mac-wk2): Output: https://webkit-queues.webkit.org/results/12120984 New failing tests: http/tests/cache-storage/cache-origins.https.html imported/w3c/web-platform-tests/service-workers/cache-storage/serviceworker/cache-match.https.html imported/w3c/web-platform-tests/service-workers/cache-storage/window/cache-match.https.html http/tests/cache-storage/cache-records-persistency.https.html http/tests/cache-storage/cache-persistency.https.html imported/w3c/web-platform-tests/service-workers/service-worker/fetch-event-within-sw.https.html imported/w3c/web-platform-tests/service-workers/cache-storage/worker/cache-match.https.html imported/w3c/web-platform-tests/service-workers/cache-storage/serviceworker/cache-keys-attributes-for-service-worker.https.html

Created attachment 369256 [details] Archive of layout-test-results from ews106 for mac-highsierra-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews106 Port: mac-highsierra-wk2 Platform: Mac OS X 10.13.6

Created attachment 369260 [details] Patch

Comment on attachment 369260 [details] Patch Attachment 369260 [details] did not pass mac-wk2-ews (mac-wk2): Output: https://webkit-queues.webkit.org/results/12121879 New failing tests: imported/w3c/web-platform-tests/service-workers/cache-storage/window/cache-match.https.html imported/w3c/web-platform-tests/service-workers/cache-storage/worker/cache-match.https.html imported/w3c/web-platform-tests/service-workers/cache-storage/serviceworker/cache-match.https.html

Created attachment 369271 [details] Archive of layout-test-results from ews105 for mac-highsierra-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews105 Port: mac-highsierra-wk2 Platform: Mac OS X 10.13.6

Created attachment 369273 [details] Patch

Comment on attachment 369273 [details] Patch Attachment 369273 [details] did not pass mac-wk2-ews (mac-wk2): Output: https://webkit-queues.webkit.org/results/12122573 New failing tests: imported/w3c/web-platform-tests/service-workers/cache-storage/window/cache-match.https.html imported/w3c/web-platform-tests/service-workers/cache-storage/worker/cache-match.https.html imported/w3c/web-platform-tests/service-workers/cache-storage/serviceworker/cache-match.https.html

Created attachment 369276 [details] Archive of layout-test-results from ews105 for mac-highsierra-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews105 Port: mac-highsierra-wk2 Platform: Mac OS X 10.13.6

Comment on attachment 369273 [details] Patch Attachment 369273 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/12122866 New failing tests: security/contentSecurityPolicy/video-with-file-url-allowed-by-media-src-star-with-AllowContentSecurityPolicySourceStarToMatchAnyProtocol-enabled.html

Created attachment 369278 [details] Archive of layout-test-results from ews212 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews212 Port: win-future Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit

Comment on attachment 369273 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=369273&action=review > Source/WTF/wtf/win/FileSystemWin.cpp:506 > +bool hardLink(const String& source, const String& destination) > +{ > + return CreateHardLink(destination.wideCharacters().data(), source.wideCharacters().data(), nullptr); > +} > + > bool hardLinkOrCopyFile(const String& source, const String& destination) > { > return !!::CopyFile(source.wideCharacters().data(), destination.wideCharacters().data(), TRUE); Why not just implement hardLinkOrCopyFile like other platforms?

Comment on attachment 369273 [details] Patch Attachment 369273 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: https://webkit-queues.webkit.org/results/12123492 New failing tests: imported/w3c/web-platform-tests/service-workers/cache-storage/window/cache-match.https.html imported/w3c/web-platform-tests/service-workers/cache-storage/worker/cache-match.https.html imported/w3c/web-platform-tests/service-workers/cache-storage/serviceworker/cache-match.https.html

Created attachment 369288 [details] Archive of layout-test-results from ews125 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews125 Port: ios-simulator-wk2 Platform: Mac OS X 10.14.4

There's some similar code to what's happening in NetworkCacheData.cpp in MemoryMappedFile within FileSystem. There isn't a Windows implementation at all but would it make sense to maybe expand on the MemoryMappedFile code and then just use that in data? This is definitely better but there's still a lot of Windows specific code happening.

Comment on attachment 369273 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=369273&action=review >> Source/WTF/wtf/win/FileSystemWin.cpp:506 >> return !!::CopyFile(source.wideCharacters().data(), destination.wideCharacters().data(), TRUE); > > Why not just implement hardLinkOrCopyFile like other platforms? They are used for different usages. hardLinkOrCopyFile is used only in SQLiteIDBTransaction::moveBlobFilesIfNecessary. (Bug 156321) https://github.com/WebKit/webkit/blob/89c28d471fae35f1788a0f857067896a10af8974/Source/WebCore/Modules/indexeddb/server/SQLiteIDBTransaction.cpp#L94 It is no problem for moveBlobFilesIfNecessary to use copying becuase it will remove the source file soon. On the other hand, BlobStorage::add should use hard linking because BlobStorage unifies files which has the identical content. I don't know why moveBlobFilesIfNecessary needs the fallback of using copy if hard linking fails. Is it possible to use only hard linking even for moveBlobFilesIfNecessary case?

(In reply to Don Olmstead from comment #28) Sounds a good idea. Filed in Bug 197684.

Created attachment 369358 [details] Patch

(In reply to Fujii Hironori from comment #29) > I don't know why moveBlobFilesIfNecessary needs the fallback of > using copy if hard linking fails. Is it possible to use only hard > linking even for moveBlobFilesIfNecessary case? hardLinkOrCopyFile uses hard linking because it's faster then falls back to copying if hard linking fails, such as if the source and destination are on different drives. It is a performance optimization to hard link.

Comment on attachment 369358 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=369358&action=review > Source/WTF/wtf/glib/FileSystemGlib.cpp:432 > +bool hardLink(const String& source, const String& destination) hardLinkOrCopyFile could call this to reduce duplicate code. > Source/WTF/wtf/posix/FileSystemPOSIX.cpp:447 > +bool hardLink(const String& source, const String& destination) ditto > Source/WebKit/NetworkProcess/cache/NetworkCacheData.h:78 > +#if !OS(WINDOWS) I think we should not have so many macros in this header. Having non-inline functions in the header would move these macros to the implementation files and make this file much tidier

Created attachment 369457 [details] Patch * Addressed review feedbacks.

Comment on attachment 369457 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=369457&action=review > Source/WTF/wtf/glib/FileSystemGlib.cpp:456 > + return !!::CopyFile(source.charactersWithNullTermination().data(), destination.charactersWithNullTermination().data(), TRUE); Why do we use charactersWithNullTermination here... > Source/WTF/wtf/win/FileSystemWin.cpp:510 > return !!::CopyFile(source.wideCharacters().data(), destination.wideCharacters().data(), TRUE); ...but wideCharacters here?

Comment on attachment 369457 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=369457&action=review >> Source/WTF/wtf/glib/FileSystemGlib.cpp:456 >> + return !!::CopyFile(source.charactersWithNullTermination().data(), destination.charactersWithNullTermination().data(), TRUE); > > Why do we use charactersWithNullTermination here... Good catch. It should be wideCharacters. We should use wideCharacters for Win32 API after ICU 59.

Created attachment 369539 [details] Patch * Addressed review feedbacks.

Comment on attachment 369539 [details] Patch Clearing flags on attachment: 369539 Committed r245186: <https://trac.webkit.org/changeset/245186>

All reviewed patches have been landed. Closing bug.

<rdar://problem/50671610>