Bug 175139 – Network cache should be usable as non-singleton

• Status: RESOLVED
• Resolution: FIXED
• Component: Page Loading

[Antti Koivisto]

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.

[Antti Koivisto]

Created attachment 317158 [details]
patch

[Antti Koivisto]

Created attachment 317167 [details]
patch

[Antti Koivisto]

Created attachment 317168 [details]
patch

[Sam Weinig]

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.

[Sam Weinig]

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.

[Antti Koivisto]

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

[Antti Koivisto]

Created attachment 317233 [details]
patch

[Antti Koivisto]

Created attachment 317236 [details]
patch

[WebKit Commit Bot]

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

[Antti Koivisto]

> AttributeError: 'NoneType' object has no attribute 'has_valid_reviewer'

Huh?

[Antti Koivisto]

Created attachment 317238 [details]
patch

[WebKit Commit Bot]

Comment on attachment 317238 [details]
patch

Clearing flags on attachment: 317238

Committed r220267: <http://trac.webkit.org/changeset/220267>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/33723415>

[Brady Eidson]

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

[Antti Koivisto]

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.

[Brady Eidson]

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