Adds class ServiceWorkerScriptCacheMap and new ServiceWorkerVersion::Listener methods related to sc…

Adds class ServiceWorkerScriptCacheMap and new ServiceWorkerVersion::Listener methods related to script caching progress. The listener methods are not yet invoked.

BUG=364318
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=270318

[michaeln, 2014-05-13 05:57:25]

hia, i'm not sure who to ask to review, this cl obviously doesn't need 3
reviewers. here's why i listed you three.

kinuko's most familar with the sw code, so i'm really interested in here
feedback on my changes about script cacheing.

jsbell shares a timezone with me, so it'd be great to co-review each others work
where possible.

gavinp is scheming/dreaming up cacheapi stuff it's be good to keep him appraised
of these scriptcache goings on

---

about the change, it pulls a map out of the version class into a seperate class
and makes that class (barely) smarter about tracking the state of the script
cache. this is working up towards landing the cl that starts storing things
(https://codereview.chromium.org/269373002/)

[michaeln, 2014-05-13 06:50:59]

https://codereview.chromium.org/280423005/diff/60001/content/browser/service_...
File content/browser/service_worker/service_worker_version.h (right):

https://codereview.chromium.org/280423005/diff/60001/content/browser/service_...
content/browser/service_worker/service_worker_version.h:95: bool success) = 0;
i'll have to track down derivatives in tests and fix them to override these too,
either that or make a scriptcache::listener class and have the
register/updatejob listen for these notifications that way.

[nhiroki, 2014-05-13 07:44:34]

Drive-by comments (looks good to me).

https://codereview.chromium.org/280423005/diff/60001/content/browser/service_...
File content/browser/service_worker/service_worker_script_cache_map.cc (right):

https://codereview.chromium.org/280423005/diff/60001/content/browser/service_...
content/browser/service_worker/service_worker_script_cache_map.cc:59: if
(is_eval_complete_ && resources_finished_ == resources_started_)
|is_eval_complete_| is always true here.

https://codereview.chromium.org/280423005/diff/60001/content/browser/service_...
File content/browser/service_worker/service_worker_script_cache_map.h (right):

https://codereview.chromium.org/280423005/diff/60001/content/browser/service_...
content/browser/service_worker/service_worker_script_cache_map.h:24: // Used
during the initial run  of a new version to build the map
nit: s/"run  of"/"run of"/   (there are 2 spaces.)

https://codereview.chromium.org/280423005/diff/60001/content/browser/service_...
content/browser/service_worker/service_worker_script_cache_map.h:36:
ServiceWorkerScriptCacheMap(ServiceWorkerVersion* owner);
nit: explicit

https://codereview.chromium.org/280423005/diff/60001/content/browser/service_...
content/browser/service_worker/service_worker_script_cache_map.h:42: // Members
used only during intial install.
nit: s/intial/initial/

https://codereview.chromium.org/280423005/diff/60001/content/browser/service_...
File content/browser/service_worker/service_worker_version.h (right):

https://codereview.chromium.org/280423005/diff/60001/content/browser/service_...
content/browser/service_worker/service_worker_version.h:113: const GURL& scope()
const { return scope_; }
scope() doesn't seem to be neccessary for this change.

https://codereview.chromium.org/280423005/diff/60001/content/browser/service_...
content/browser/service_worker/service_worker_version.h:215: // Propagates
script caching progress to listneners.
nit: s/listneners/listeners/

[kinuko, 2014-05-13 16:14:24]

I have a few questions about detailed timing / what should happen when etc, but
this basically lgtm

https://codereview.chromium.org/280423005/diff/60001/content/browser/service_...
File content/browser/service_worker/service_worker_script_cache_map.cc (right):

https://codereview.chromium.org/280423005/diff/60001/content/browser/service_...
content/browser/service_worker/service_worker_script_cache_map.cc:56: void
ServiceWorkerScriptCacheMap::NotifyEvalCompletion() {
It's a bit questionable if we should cache scripts when loading was ok but
evaluating scripts had exceptions (spec issue?)

https://codereview.chromium.org/280423005/diff/60001/content/browser/service_...
File content/browser/service_worker/service_worker_script_cache_map.h (right):

https://codereview.chromium.org/280423005/diff/60001/content/browser/service_...
content/browser/service_worker/service_worker_script_cache_map.h:34: // The
version objects owns it's script cache and provides a rawptr to it.
objects owns it's -> object owns its

https://codereview.chromium.org/280423005/diff/60001/content/browser/service_...
File content/browser/service_worker/service_worker_version.h (right):

https://codereview.chromium.org/280423005/diff/60001/content/browser/service_...
content/browser/service_worker/service_worker_version.h:95: bool success) = 0;
On 2014/05/13 06:51:00, michaeln wrote:
> i'll have to track down derivatives in tests and fix them to override these
too,
> either that or make a scriptcache::listener class and have the
> register/updatejob listen for these notifications that way.

Looks like these can be in a separate listener class, like ScriptCacheObserver,
as the source of events are a bit different and other listeners won't be
interested in these events?

https://codereview.chromium.org/280423005/diff/60001/content/browser/service_...
content/browser/service_worker/service_worker_version.h:95: bool success) = 0;
Is this where we compare scripts to see if it's the same one or not?  In update
case how the version's cache map would look like?

https://codereview.chromium.org/280423005/diff/60001/content/browser/service_...
content/browser/service_worker/service_worker_version.h:101: bool success) = 0;
Is this where we actually commit the registration with resources?

[michaeln, 2014-05-13 22:41:10]

https://codereview.chromium.org/280423005/diff/60001/content/browser/service_...
File content/browser/service_worker/service_worker_script_cache_map.cc (right):

https://codereview.chromium.org/280423005/diff/60001/content/browser/service_...
content/browser/service_worker/service_worker_script_cache_map.cc:56: void
ServiceWorkerScriptCacheMap::NotifyEvalCompletion() {
On 2014/05/13 16:14:25, kinuko wrote:
> It's a bit questionable if we should cache scripts when loading was ok but
> evaluating scripts had exceptions (spec issue?)

Spec issue is right, this is a question for the registration job to answer. Is
the registration "committed" or not in that case?

https://codereview.chromium.org/280423005/diff/60001/content/browser/service_...
content/browser/service_worker/service_worker_script_cache_map.cc:59: if
(is_eval_complete_ && resources_finished_ == resources_started_)
On 2014/05/13 07:44:34, nhiroki wrote:
> |is_eval_complete_| is always true here.

Done.

https://codereview.chromium.org/280423005/diff/60001/content/browser/service_...
File content/browser/service_worker/service_worker_script_cache_map.h (right):

https://codereview.chromium.org/280423005/diff/60001/content/browser/service_...
content/browser/service_worker/service_worker_script_cache_map.h:24: // Used
during the initial run  of a new version to build the map
On 2014/05/13 07:44:34, nhiroki wrote:
> nit: s/"run  of"/"run of"/   (there are 2 spaces.)

Done.

https://codereview.chromium.org/280423005/diff/60001/content/browser/service_...
content/browser/service_worker/service_worker_script_cache_map.h:34: // The
version objects owns it's script cache and provides a rawptr to it.
On 2014/05/13 16:14:25, kinuko wrote:
> objects owns it's -> object owns its

Done.

https://codereview.chromium.org/280423005/diff/60001/content/browser/service_...
content/browser/service_worker/service_worker_script_cache_map.h:36:
ServiceWorkerScriptCacheMap(ServiceWorkerVersion* owner);
On 2014/05/13 07:44:34, nhiroki wrote:
> nit: explicit

Done.

https://codereview.chromium.org/280423005/diff/60001/content/browser/service_...
content/browser/service_worker/service_worker_script_cache_map.h:42: // Members
used only during intial install.
On 2014/05/13 07:44:34, nhiroki wrote:
> nit: s/intial/initial/

Done.

https://codereview.chromium.org/280423005/diff/60001/content/browser/service_...
File content/browser/service_worker/service_worker_version.h (right):

https://codereview.chromium.org/280423005/diff/60001/content/browser/service_...
content/browser/service_worker/service_worker_version.h:95: bool success) = 0;
On 2014/05/13 16:14:25, kinuko wrote:
> Is this where we compare scripts to see if it's the same one or not?  In
update
> case how the version's cache map would look like?

Yes. In the first time and update cases, a versions cache map should have an
entry for the main script url. The storage class will provide a method to
compare two responses given their ids.

https://codereview.chromium.org/280423005/diff/60001/content/browser/service_...
content/browser/service_worker/service_worker_version.h:95: bool success) = 0;
On 2014/05/13 16:14:25, kinuko wrote:
> On 2014/05/13 06:51:00, michaeln wrote:
> > i'll have to track down derivatives in tests and fix them to override these
> too,
> > either that or make a scriptcache::listener class and have the
> > register/updatejob listen for these notifications that way.
> 
> Looks like these can be in a separate listener class, like
ScriptCacheObserver,
> as the source of events are a bit different and other listeners won't be
> interested in these events?

Done, ServiceWorkerScriptCacheMap::Observer.

https://codereview.chromium.org/280423005/diff/60001/content/browser/service_...
content/browser/service_worker/service_worker_version.h:101: bool success) = 0;
On 2014/05/13 16:14:25, kinuko wrote:
> Is this where we actually commit the registration with resources?

I think so. There are at least two things of interest when it comes to writing
to disk: scripts are all cached, install event has completed successfully. I
think we commit the registration after both of those have succeeded.

https://codereview.chromium.org/280423005/diff/60001/content/browser/service_...
content/browser/service_worker/service_worker_version.h:113: const GURL& scope()
const { return scope_; }
On 2014/05/13 07:44:34, nhiroki wrote:
> scope() doesn't seem to be neccessary for this change.

Done.

https://codereview.chromium.org/280423005/diff/60001/content/browser/service_...
content/browser/service_worker/service_worker_version.h:215: // Propagates
script caching progress to listneners.
On 2014/05/13 07:44:34, nhiroki wrote:
> nit: s/listneners/listeners/

removed these methods

[michaeln, 2014-05-14 00:00:39]

The CQ bit was checked by michaeln@chromium.org

[commit-bot: I haz the power, 2014-05-14 00:03:20]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/michaeln@chromium.org/280423005/80001

[commit-bot: I haz the power, 2014-05-14 03:09:40]

Change committed as 270318