Currently we always use it as a singleton. We might want to use it as backend for cache web API and should make it work as a non-singleton instance too.
Created attachment 317158 [details] patch
Created attachment 317167 [details] patch
Created attachment 317168 [details] patch
Comment on attachment 317168 [details] patch Nice! This also has the nice side effect that if we want to do targeted unit testing of it, it's much more straight forward. One thing that might make things a little nicer, though could be left for later, is to make the NetworkProcess object, which is a singleton, own the NetworkCache instance, any make accessing the singleton be via NetworkProcess::singleton()->diskCache() (or whatever). It would also make the calls to NetworkCache::Cache::open() that don't use their return values much less mysterious.
Comment on attachment 317168 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=317168&action=review r=me when it all builds > Source/WebKit/NetworkProcess/cocoa/NetworkProcessCocoa.mm:121 > + NetworkCache::Cache::open(m_diskCacheDirectory, cacheOptions); This feels too mysterious. While I think you should store the Cache ref here, maybe for now just a comment explaining what's happening. > Source/WebKit/NetworkProcess/soup/NetworkProcessSoup.cpp:124 > + NetworkCache::Cache::open(m_diskCacheDirectory, cacheOptions); This feels too mysterious. While I think you should store the Cache ref here, maybe for now just a comment explaining what's happening.
> One thing that might make things a little nicer, though could be left for > later, is to make the NetworkProcess object, which is a singleton, own the > NetworkCache instance, any make accessing the singleton be via > NetworkProcess::singleton()->diskCache() (or whatever). It would also make > the calls to NetworkCache::Cache::open() that don't use their return values > much less mysterious. Good point. I'll update the patch.
Created attachment 317233 [details] patch
Created attachment 317236 [details] patch
Comment on attachment 317236 [details] patch Rejecting attachment 317236 [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-01', 'validate-changelog', '--check-oops', '--non-interactive', 317236, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit Last 500 characters of output: a/EWS/WebKit/Tools/Scripts/webkitpy/tool/commands/stepsequence.py", line 73, in run_and_handle_errors self._run(tool, options, state) File "/Volumes/Data/EWS/WebKit/Tools/Scripts/webkitpy/tool/commands/stepsequence.py", line 67, in _run step(tool, options).run(state) File "/Volumes/Data/EWS/WebKit/Tools/Scripts/webkitpy/tool/steps/validatereviewer.py", line 54, in run if changelog_entry.has_valid_reviewer(): AttributeError: 'NoneType' object has no attribute 'has_valid_reviewer' Full output: http://webkit-queues.webkit.org/results/4252602
> AttributeError: 'NoneType' object has no attribute 'has_valid_reviewer' Huh?
Created attachment 317238 [details] patch
Comment on attachment 317238 [details] patch Clearing flags on attachment: 317238 Committed r220267: <http://trac.webkit.org/changeset/220267>
All reviewed patches have been landed. Closing bug.
<rdar://problem/33723415>
(In reply to Sam Weinig from comment #4) > > One thing that might make things a little nicer, though could be left for > later, is to make the NetworkProcess object, which is a singleton, own the > NetworkCache instance, any make accessing the singleton be via > NetworkProcess::singleton()->diskCache() (or whatever). Actually we need to do better than that (and now we can) because it's faulty to only have a singleton network cache. This is because the WebKit API now supports multiple persistent data stores, yet all NetworkResourceLoaders with persistent data stores share the same NetworkCache. We need to have a map of sessionIDs to NetworkCaches (each pointing to a unique location on disk)
That should be pretty easy to implement now though it is not clear to me why someone would want split caches within a single client. Some sort of weaker private browsing mode? Things like cache shrinking policy in presence of multiple caches would need to be resolved. Another approach for that would be to keep a single cache instance but include a data store identifier with the cache key. This would allow data deduplication and existing shrinking policy to function to work without any extra effort. Or maybe we want to have multiple Cache instances that share the same Storage backend. We are not far architecturally from supporting that either.
(In reply to Antti Koivisto from comment #16) > That should be pretty easy to implement now though it is not clear to me why > someone would want split caches within a single client. Some sort of weaker > private browsing mode? Things like cache shrinking policy in presence of > multiple caches would need to be resolved. > > Another approach for that would be to keep a single cache instance but > include a data store identifier with the cache key. This would allow data > deduplication and existing shrinking policy to function to work without any > extra effort. > > Or maybe we want to have multiple Cache instances that share the same > Storage backend. We are not far architecturally from supporting that either. I'm not sure which one of these approaches is better, but we should analyze and pick one to move forward with. Upcoming OSes have clients with multiple sessions in the same Networking Process. Ping me on IRC for details.