CachedMetadata support for ServiceWorker script.

CachedMetadata support for ServiceWorker script.

This cl changes the followings
 - Introduce CachedMetadataHandler interface.
 - Change V8ScriptRunner to use this interface instead of Resource class while handling the metadata.
 - Introduce ServiceWorkerScriptCachedMetadataHandler.
  - In WorkerThread::initialize(), we create CachedMetadataHandler by calling workerGlobalScope()->createWorkerScriptCachedMetadataHandler() with m_cachedMetaData in WorkerThreadStartupData.
  - If the workerGlobalScope is a ServiceWorkerGlobalScope createWorkerScriptCachedMetadataHandler() returns ServiceWorkerScriptCachedMetadataHandler, otherwise returns null.
  - We pass it to WorkerScriptController. And it will be used in V8ScriptRunner.
  - When ServiceWorkerScriptCachedMetadataHandler.setCachedMetadata/clearCachedMetadata is called, we call ServiceWorkerGlobalScopeClientImpl.setCachedMetadata/clearCachedMetadata.
 - When ServiceWorkerGlobalScopeClientImpl.setCachedMetadata/clearCachedMetadata is called we call WebServiceWorkerContextClient.setCachedMetadata/clearCachedMetadata.

I will implement ServiceWorkerScriptContext.setCachedMetadata/clearCachedMetadata in chromium side.
These method will send IPC message to the browser process.

BUG=449895
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=190399

[horo, 2015-02-16 08:24:45]

horo@chromium.org changed reviewers:
+ kinuko@chromium.org

[horo, 2015-02-16 08:24:46]

kinuko@
Could you please review this?

Thank you!

[vivekg, 2015-02-16 08:31:49]

vivekg@chromium.org changed reviewers:
+ vivekg@chromium.org

[vivekg, 2015-02-16 08:31:50]

https://codereview.chromium.org/929953002/diff/1/Source/bindings/core/v8/cust...
File Source/bindings/core/v8/custom/V8InjectedScriptHostCustom.cpp (right):

https://codereview.chromium.org/929953002/diff/1/Source/bindings/core/v8/cust...
Source/bindings/core/v8/custom/V8InjectedScriptHostCustom.cpp:391:
v8::Handle<v8::Script> script = V8ScriptRunner::compileScript(expression,
String(), TextPosition(), 0, 0, 0, isolate);
nit: Now that all our bots support the C++11, using `nullptr` would be more
verbose.

[vivekg, 2015-02-16 08:36:49]

https://codereview.chromium.org/929953002/diff/1/Source/bindings/core/v8/V8Sc...
File Source/bindings/core/v8/V8ScriptRunner.h (right):

https://codereview.chromium.org/929953002/diff/1/Source/bindings/core/v8/V8Sc...
Source/bindings/core/v8/V8ScriptRunner.h:50: static v8::Local<v8::Script>
compileScript(v8::Handle<v8::String>, const String& fileName, const
TextPosition&, ScriptResource*, ScriptStreamer*, CachedMetadataHandler*,
v8::Isolate*, AccessControlStatus = SharableCrossOrigin, V8CacheOptions =
V8CacheOptionsDefault, bool isInternalScript = false);
nit: As most of the call sites of compileScript are passing 0 or the nullptr to
either of ScriptResource, ScriptStreamer or CachedMetadataHandler, would it make
sense to make these as default args (nullptr as default) with isolate arg coming
before these. WDYT?

[horo, 2015-02-16 08:57:44]

Thank you!

https://codereview.chromium.org/929953002/diff/1/Source/bindings/core/v8/V8Sc...
File Source/bindings/core/v8/V8ScriptRunner.h (right):

https://codereview.chromium.org/929953002/diff/1/Source/bindings/core/v8/V8Sc...
Source/bindings/core/v8/V8ScriptRunner.h:50: static v8::Local<v8::Script>
compileScript(v8::Handle<v8::String>, const String& fileName, const
TextPosition&, ScriptResource*, ScriptStreamer*, CachedMetadataHandler*,
v8::Isolate*, AccessControlStatus = SharableCrossOrigin, V8CacheOptions =
V8CacheOptionsDefault, bool isInternalScript = false);
On 2015/02/16 08:36:49, vivekg_ wrote:
> nit: As most of the call sites of compileScript are passing 0 or the nullptr
to
> either of ScriptResource, ScriptStreamer or CachedMetadataHandler, would it
make
> sense to make these as default args (nullptr as default) with isolate arg
coming
> before these. WDYT?

Sounds good!
Done.

https://codereview.chromium.org/929953002/diff/1/Source/bindings/core/v8/cust...
File Source/bindings/core/v8/custom/V8InjectedScriptHostCustom.cpp (right):

https://codereview.chromium.org/929953002/diff/1/Source/bindings/core/v8/cust...
Source/bindings/core/v8/custom/V8InjectedScriptHostCustom.cpp:391:
v8::Handle<v8::Script> script = V8ScriptRunner::compileScript(expression,
String(), TextPosition(), 0, 0, 0, isolate);
On 2015/02/16 08:31:49, vivekg_ wrote:
> nit: Now that all our bots support the C++11, using `nullptr` would be more
> verbose.

Done.

[vivekg, 2015-02-16 09:02:53]

https://codereview.chromium.org/929953002/diff/40001/Source/bindings/core/v8/...
File Source/bindings/core/v8/V8ScriptRunnerTest.cpp (right):

https://codereview.chromium.org/929953002/diff/40001/Source/bindings/core/v8/...
Source/bindings/core/v8/V8ScriptRunnerTest.cpp:68: isolate(), m_resource.get(),
0, 0, NotSharableCrossOrigin, cacheOptions)
nit: 0 -> nullptr :)

[vivekg, 2015-02-16 09:20:09]

bindings lgtm (non-owner) with some minor nits.

https://codereview.chromium.org/929953002/diff/40001/Source/bindings/core/v8/...
File Source/bindings/core/v8/V8ScriptRunner.h (right):

https://codereview.chromium.org/929953002/diff/40001/Source/bindings/core/v8/...
Source/bindings/core/v8/V8ScriptRunner.h:43: class CachedMetadataHandler;
nit: Usually class fwd declarations are in sorted order.

https://codereview.chromium.org/929953002/diff/40001/Source/bindings/core/v8/...
Source/bindings/core/v8/V8ScriptRunner.h:43: class CachedMetadataHandler;
nit: Usually class fwd declarations are in sorted order.

https://codereview.chromium.org/929953002/diff/40001/Source/bindings/core/v8/...
File Source/bindings/core/v8/WorkerScriptController.h (right):

https://codereview.chromium.org/929953002/diff/40001/Source/bindings/core/v8/...
Source/bindings/core/v8/WorkerScriptController.h:47: class
CachedMetadataHandler;
nit: ditto

[horo, 2015-02-16 10:23:26]

New patchsets have been uploaded after l-g-t-m from vivekg@chromium.org

[horo, 2015-02-16 10:23:27]

Thank you!!

https://codereview.chromium.org/929953002/diff/40001/Source/bindings/core/v8/...
File Source/bindings/core/v8/V8ScriptRunner.h (right):

https://codereview.chromium.org/929953002/diff/40001/Source/bindings/core/v8/...
Source/bindings/core/v8/V8ScriptRunner.h:43: class CachedMetadataHandler;
On 2015/02/16 09:20:08, vivekg_ wrote:
> nit: Usually class fwd declarations are in sorted order.

Done.

https://codereview.chromium.org/929953002/diff/40001/Source/bindings/core/v8/...
File Source/bindings/core/v8/V8ScriptRunnerTest.cpp (right):

https://codereview.chromium.org/929953002/diff/40001/Source/bindings/core/v8/...
Source/bindings/core/v8/V8ScriptRunnerTest.cpp:68: isolate(), m_resource.get(),
0, 0, NotSharableCrossOrigin, cacheOptions)
On 2015/02/16 09:02:52, vivekg_ wrote:
> nit: 0 -> nullptr :)

Done.

https://codereview.chromium.org/929953002/diff/40001/Source/bindings/core/v8/...
File Source/bindings/core/v8/WorkerScriptController.h (right):

https://codereview.chromium.org/929953002/diff/40001/Source/bindings/core/v8/...
Source/bindings/core/v8/WorkerScriptController.h:47: class
CachedMetadataHandler;
On 2015/02/16 09:20:09, vivekg_ wrote:
> nit: ditto

Done.

[horo, 2015-02-16 10:28:30]

horo@chromium.org changed reviewers:
+ vogelheim@chromium.org

[horo, 2015-02-16 10:28:31]

vogelheim@
Could you please review this?

[vogelheim, 2015-02-16 13:12:05]

Generally looks good, but I'm unhappy with the
CacheHandler-for-non-cacheable-resources thing. I try to explain in the
comments...

https://codereview.chromium.org/929953002/diff/60001/Source/bindings/core/v8/...
File Source/bindings/core/v8/V8ScriptRunner.cpp (right):

https://codereview.chromium.org/929953002/diff/60001/Source/bindings/core/v8/...
Source/bindings/core/v8/V8ScriptRunner.cpp:276: if ((resource &&
!resource->url().protocolIsInHTTPFamily()) || (!resource && !cacheHandler))
Hmm.. I find this odd. Below, all usages of resource have been replaced with
cacheHandler. Which makes perfect sense, since the point cacheHandler is to be
the abstraction for the cache. But then why are we checking for the resource
here? In fact, why is Resource* even still a parameter?


Maybe one point of confusion and/or fixing this is the ...protocolIsInHTTPFamily
check: I originally added this check because the main cache implementation will
CHECK for the protocol. The intention is that e.g. scripts from extensions
shouldn't end up in the disk cache, since they're stored locally anyhow. So
without this check, the renderer terminates with a CHECK. IMHO, the best way out
of that problem is to make the Resource never return a CachedMetadataHandler if
it can't actually cache that type of resource.

In other words: If the Resource - or whatever- gives you a MetadataCacheHandler,
it should no longer be necessary to even make this check.

https://codereview.chromium.org/929953002/diff/60001/Source/core/fetch/Resour...
File Source/core/fetch/Resource.cpp (right):

https://codereview.chromium.org/929953002/diff/60001/Source/core/fetch/Resour...
Source/core/fetch/Resource.cpp:452: return &m_cacheHandler;
This might not be correct: Not all Resources are cacheable; e.g. the default
cache impl will refuse to cache URLs belonging to extensions. In this case, this
would return a CachedMetadataHandler instance that will CHECK-fault on pretty
much every use of it.

Technically, whether that is correct depends on how you define the API :), but
it seems more sensible to only return an instance that is actually usable. You
might to something like:
  return protocolIsInHTTPFamily() ? &m_cacheHandler : nullptr;

See also the somewhat long-ish explanation at V8ScriptRunner.cpp, because this
would have a direct impact.

https://codereview.chromium.org/929953002/diff/60001/Source/core/fetch/Resour...
File Source/core/fetch/Resource.h (right):

https://codereview.chromium.org/929953002/diff/60001/Source/core/fetch/Resour...
Source/core/fetch/Resource.h:197: CachedMetadataHandler* cacheHandler();
:) Just wanted to say I like this version better than the earlier draft where
Resource inherits from CachedMetadataHandler.

[kinuko, 2015-02-16 14:18:39]

https://codereview.chromium.org/929953002/diff/60001/Source/bindings/core/v8/...
File Source/bindings/core/v8/V8ScriptRunner.cpp (right):

https://codereview.chromium.org/929953002/diff/60001/Source/bindings/core/v8/...
Source/bindings/core/v8/V8ScriptRunner.cpp:279: 
It looks it's possible to reach here when cacheHandler is null, but we don't
check if cacheHandler != nullptr in compileAndConsumeOrProduce.  Is it ok?

(Probably it's not necessary in the current possible code paths, but it's not
very obvious from the code)

https://codereview.chromium.org/929953002/diff/60001/Source/bindings/core/v8/...
File Source/bindings/core/v8/V8ScriptRunner.h (right):

https://codereview.chromium.org/929953002/diff/60001/Source/bindings/core/v8/...
Source/bindings/core/v8/V8ScriptRunner.h:39: class CachedMetadataHandler;
nit: it's not really sorted yet?

https://codereview.chromium.org/929953002/diff/60001/Source/core/fetch/Cached...
File Source/core/fetch/CachedMetadataHandler.h (right):

https://codereview.chromium.org/929953002/diff/60001/Source/core/fetch/Cached...
Source/core/fetch/CachedMetadataHandler.h:14: class CachedMetadataHandler {
Can we have a brief class comment for this?

https://codereview.chromium.org/929953002/diff/60001/Source/core/fetch/Cached...
Source/core/fetch/CachedMetadataHandler.h:24: virtual void
setCachedMetadata(unsigned, const char*, size_t, CacheType = SendToPlatform) =
0;
unsigned -> unsigned dataTypeID

otherwise the comment doesn't make sense.

https://codereview.chromium.org/929953002/diff/60001/Source/core/fetch/Resour...
File Source/core/fetch/Resource.h (right):

https://codereview.chromium.org/929953002/diff/60001/Source/core/fetch/Resour...
Source/core/fetch/Resource.h:341: class CacheHandler : public
CachedMetadataHandler {
nit: Resource class is big, does it make sense to have m_cacheHandler as a owned
ptr of CachedMetadataHandler and hide this class definition in .cpp ?

https://codereview.chromium.org/929953002/diff/60001/Source/core/workers/Work...
File Source/core/workers/WorkerGlobalScope.h (right):

https://codereview.chromium.org/929953002/diff/60001/Source/core/workers/Work...
Source/core/workers/WorkerGlobalScope.h:103: virtual void
clearCachedMetadata(const KURL&) { }
Hmm.  We modify all the WorkerThreadStartupData, WorkerGlobalScope and
WorkerThread to add cached metadata just for ServiceWorker (for now), which
feels a bit intrusive to me.

I think... we could probably introduce more specialized classes instead? 
Something like:

- Define WorkerScriptCachedMetadataClient as a supplement of WorkerClients,
which has setCachedMetadata(), clearCachedMetadata() and releaseCachedMetadata()
methods
- Implement ServiceWorkerScriptCachedMetadataClient, which holds cached
metadata, and has reference to WebServiceWorkerContextClient
- Instantiate ServiceWorkerScriptCachedMetadataClient in WebEmbeddedWorkerImpl
and provide it to worker
- In WorkerThread.cpp we create MetadataHandler with MetadataClient which can be
obtained via worker clients supplement interface

https://codereview.chromium.org/929953002/diff/60001/Source/core/workers/Work...
File Source/core/workers/WorkerThread.cpp (right):

https://codereview.chromium.org/929953002/diff/60001/Source/core/workers/Work...
Source/core/workers/WorkerThread.cpp:86: class WorkerScriptCachedMetadataHandler
: public CachedMetadataHandler {
This class can be probably in its own file, as it's not really related to
WorkerThread.

[horo, 2015-02-16 14:36:24]

New patchsets have been uploaded after l-g-t-m from vivekg@chromium.org

[horo, 2015-02-16 16:20:12]

https://codereview.chromium.org/929953002/diff/60001/Source/bindings/core/v8/...
File Source/bindings/core/v8/V8ScriptRunner.cpp (right):

https://codereview.chromium.org/929953002/diff/60001/Source/bindings/core/v8/...
Source/bindings/core/v8/V8ScriptRunner.cpp:276: if ((resource &&
!resource->url().protocolIsInHTTPFamily()) || (!resource && !cacheHandler))
On 2015/02/16 13:12:04, vogelheim wrote:
> Hmm.. I find this odd. Below, all usages of resource have been replaced with
> cacheHandler. Which makes perfect sense, since the point cacheHandler is to be
> the abstraction for the cache. But then why are we checking for the resource
> here? In fact, why is Resource* even still a parameter?
> 
> 
> Maybe one point of confusion and/or fixing this is the
...protocolIsInHTTPFamily
> check: I originally added this check because the main cache implementation
will
> CHECK for the protocol. The intention is that e.g. scripts from extensions
> shouldn't end up in the disk cache, since they're stored locally anyhow. So
> without this check, the renderer terminates with a CHECK. IMHO, the best way
out
> of that problem is to make the Resource never return a CachedMetadataHandler
if
> it can't actually cache that type of resource.
> 
> In other words: If the Resource - or whatever- gives you a
MetadataCacheHandler,
> it should no longer be necessary to even make this check.
> 

I agree.
I changed Resource.cacheHandler() to return null when the resource isn't
cacheable.

https://codereview.chromium.org/929953002/diff/60001/Source/bindings/core/v8/...
Source/bindings/core/v8/V8ScriptRunner.cpp:279: 
On 2015/02/16 14:18:38, kinuko wrote:
> It looks it's possible to reach here when cacheHandler is null, but we don't
> check if cacheHandler != nullptr in compileAndConsumeOrProduce.  Is it ok?
> 
> (Probably it's not necessary in the current possible code paths, but it's not
> very obvious from the code)

Done.

https://codereview.chromium.org/929953002/diff/60001/Source/bindings/core/v8/...
File Source/bindings/core/v8/V8ScriptRunner.h (right):

https://codereview.chromium.org/929953002/diff/60001/Source/bindings/core/v8/...
Source/bindings/core/v8/V8ScriptRunner.h:39: class CachedMetadataHandler;
On 2015/02/16 14:18:38, kinuko wrote:
> nit: it's not really sorted yet?

Done.

https://codereview.chromium.org/929953002/diff/60001/Source/core/fetch/Cached...
File Source/core/fetch/CachedMetadataHandler.h (right):

https://codereview.chromium.org/929953002/diff/60001/Source/core/fetch/Cached...
Source/core/fetch/CachedMetadataHandler.h:14: class CachedMetadataHandler {
On 2015/02/16 14:18:38, kinuko wrote:
> Can we have a brief class comment for this?

Done.

https://codereview.chromium.org/929953002/diff/60001/Source/core/fetch/Cached...
Source/core/fetch/CachedMetadataHandler.h:24: virtual void
setCachedMetadata(unsigned, const char*, size_t, CacheType = SendToPlatform) =
0;
On 2015/02/16 14:18:38, kinuko wrote:
> unsigned -> unsigned dataTypeID
> 
> otherwise the comment doesn't make sense.

Done.

https://codereview.chromium.org/929953002/diff/60001/Source/core/fetch/Resour...
File Source/core/fetch/Resource.cpp (right):

https://codereview.chromium.org/929953002/diff/60001/Source/core/fetch/Resour...
Source/core/fetch/Resource.cpp:452: return &m_cacheHandler;
On 2015/02/16 13:12:04, vogelheim wrote:
> This might not be correct: Not all Resources are cacheable; e.g. the default
> cache impl will refuse to cache URLs belonging to extensions. In this case,
this
> would return a CachedMetadataHandler instance that will CHECK-fault on pretty
> much every use of it.
> 
> Technically, whether that is correct depends on how you define the API :), but
> it seems more sensible to only return an instance that is actually usable. You
> might to something like:
>   return protocolIsInHTTPFamily() ? &m_cacheHandler : nullptr;
> 
> See also the somewhat long-ish explanation at V8ScriptRunner.cpp, because this
> would have a direct impact.

Done.

https://codereview.chromium.org/929953002/diff/60001/Source/core/fetch/Resour...
File Source/core/fetch/Resource.h (right):

https://codereview.chromium.org/929953002/diff/60001/Source/core/fetch/Resour...
Source/core/fetch/Resource.h:341: class CacheHandler : public
CachedMetadataHandler {
On 2015/02/16 14:18:38, kinuko wrote:
> nit: Resource class is big, does it make sense to have m_cacheHandler as a
owned
> ptr of CachedMetadataHandler and hide this class definition in .cpp ?

Done.

https://codereview.chromium.org/929953002/diff/60001/Source/core/workers/Work...
File Source/core/workers/WorkerGlobalScope.h (right):

https://codereview.chromium.org/929953002/diff/60001/Source/core/workers/Work...
Source/core/workers/WorkerGlobalScope.h:103: virtual void
clearCachedMetadata(const KURL&) { }
On 2015/02/16 14:18:38, kinuko wrote:
> Hmm.  We modify all the WorkerThreadStartupData, WorkerGlobalScope and
> WorkerThread to add cached metadata just for ServiceWorker (for now), which
> feels a bit intrusive to me.
> 
> I think... we could probably introduce more specialized classes instead? 
> Something like:
> 
> - Define WorkerScriptCachedMetadataClient as a supplement of WorkerClients,
> which has setCachedMetadata(), clearCachedMetadata() and
releaseCachedMetadata()
> methods
> - Implement ServiceWorkerScriptCachedMetadataClient, which holds cached
> metadata, and has reference to WebServiceWorkerContextClient
> - Instantiate ServiceWorkerScriptCachedMetadataClient in WebEmbeddedWorkerImpl
> and provide it to worker
> - In WorkerThread.cpp we create MetadataHandler with MetadataClient which can
be
> obtained via worker clients supplement interface

I want to use WorkerScriptCachedMetadataHandler in
WorkerGlobalScope::importScripts() to support v8 cache for imported scripts in
future.

In this case, I think WorkerScriptCachedMetadataClient as a supplement of
WorkerClients is not suitable.

https://codereview.chromium.org/929953002/diff/60001/Source/core/workers/Work...
File Source/core/workers/WorkerThread.cpp (right):

https://codereview.chromium.org/929953002/diff/60001/Source/core/workers/Work...
Source/core/workers/WorkerThread.cpp:86: class WorkerScriptCachedMetadataHandler
: public CachedMetadataHandler {
On 2015/02/16 14:18:38, kinuko wrote:
> This class can be probably in its own file, as it's not really related to
> WorkerThread.

Done.

[vogelheim, 2015-02-16 17:29:50]

https://codereview.chromium.org/929953002/diff/100001/Source/bindings/core/v8...
File Source/bindings/core/v8/V8ScriptRunner.cpp (right):

https://codereview.chromium.org/929953002/diff/100001/Source/bindings/core/v8...
Source/bindings/core/v8/V8ScriptRunner.cpp:202: return (v8CacheDataVersion |
kind) + StringHash::hash(cacheHandler->encoding());
fyi, there's a failing test because this assumes cacheHandler() != nullptr, and
I just convinced you that cacheHandler() should return 0 for non-cacheable
resources.

I'm not sure whether that's an artefact, or whether there's a genuine bug
somewhere. Normally, there's no reason to call cacheTag if you can't cache
anyhow, so I would have thought this shouldn't happen.

https://codereview.chromium.org/929953002/diff/100001/Source/core/fetch/Cache...
File Source/core/fetch/CachedMetadataHandler.h (right):

https://codereview.chromium.org/929953002/diff/100001/Source/core/fetch/Cache...
Source/core/fetch/CachedMetadataHandler.h:8: #include "wtf/Forward.h"
Is this still needed? I'm not seeing a wtf::* reference here.

https://codereview.chromium.org/929953002/diff/100001/Source/core/fetch/Resou...
File Source/core/fetch/Resource.cpp (right):

https://codereview.chromium.org/929953002/diff/100001/Source/core/fetch/Resou...
Source/core/fetch/Resource.cpp:182: if (url().protocolIsInHTTPFamily())
nitpick: This is exactly right, but maybe add a 1 line comment why it's there,
since it's bit non-obvious merely from reading the code.

[horo, 2015-02-16 17:54:27]

https://codereview.chromium.org/929953002/diff/100001/Source/bindings/core/v8...
File Source/bindings/core/v8/V8ScriptRunner.cpp (right):

https://codereview.chromium.org/929953002/diff/100001/Source/bindings/core/v8...
Source/bindings/core/v8/V8ScriptRunner.cpp:202: return (v8CacheDataVersion |
kind) + StringHash::hash(cacheHandler->encoding());
On 2015/02/16 17:29:49, vogelheim wrote:
> fyi, there's a failing test because this assumes cacheHandler() != nullptr,
and
> I just convinced you that cacheHandler() should return 0 for non-cacheable
> resources.
> 
> I'm not sure whether that's an artefact, or whether there's a genuine bug
> somewhere. Normally, there's no reason to call cacheTag if you can't cache
> anyhow, so I would have thought this shouldn't happen.

There was a bug in the constructor of Resource.
m_cacheHandler was not set correctly.

I uploaded the fixed version.

https://codereview.chromium.org/929953002/diff/100001/Source/core/fetch/Cache...
File Source/core/fetch/CachedMetadataHandler.h (right):

https://codereview.chromium.org/929953002/diff/100001/Source/core/fetch/Cache...
Source/core/fetch/CachedMetadataHandler.h:8: #include "wtf/Forward.h"
On 2015/02/16 17:29:49, vogelheim wrote:
> Is this still needed? I'm not seeing a wtf::* reference here.

I needed wtf/text/WTFString.h.
Done.

https://codereview.chromium.org/929953002/diff/100001/Source/core/fetch/Resou...
File Source/core/fetch/Resource.cpp (right):

https://codereview.chromium.org/929953002/diff/100001/Source/core/fetch/Resou...
Source/core/fetch/Resource.cpp:182: if (url().protocolIsInHTTPFamily())
On 2015/02/16 17:29:50, vogelheim wrote:
> nitpick: This is exactly right, but maybe add a 1 line comment why it's there,
> since it's bit non-obvious merely from reading the code.

Done.

[vogelheim, 2015-02-16 18:00:07]

lgtm

[kinuko, 2015-02-17 02:32:32]

https://codereview.chromium.org/929953002/diff/60001/Source/core/workers/Work...
File Source/core/workers/WorkerGlobalScope.h (right):

https://codereview.chromium.org/929953002/diff/60001/Source/core/workers/Work...
Source/core/workers/WorkerGlobalScope.h:103: virtual void
clearCachedMetadata(const KURL&) { }
On 2015/02/16 16:20:12, horo wrote:
> On 2015/02/16 14:18:38, kinuko wrote:
> > Hmm.  We modify all the WorkerThreadStartupData, WorkerGlobalScope and
> > WorkerThread to add cached metadata just for ServiceWorker (for now), which
> > feels a bit intrusive to me.
> > 
> > I think... we could probably introduce more specialized classes instead? 
> > Something like:
> > 
> > - Define WorkerScriptCachedMetadataClient as a supplement of WorkerClients,
> > which has setCachedMetadata(), clearCachedMetadata() and
> releaseCachedMetadata()
> > methods
> > - Implement ServiceWorkerScriptCachedMetadataClient, which holds cached
> > metadata, and has reference to WebServiceWorkerContextClient
> > - Instantiate ServiceWorkerScriptCachedMetadataClient in
WebEmbeddedWorkerImpl
> > and provide it to worker
> > - In WorkerThread.cpp we create MetadataHandler with MetadataClient which
can
> be
> > obtained via worker clients supplement interface
> 
> I want to use WorkerScriptCachedMetadataHandler in
> WorkerGlobalScope::importScripts() to support v8 cache for imported scripts in
> future.
> 
> In this case, I think WorkerScriptCachedMetadataClient as a supplement of
> WorkerClients is not suitable.

How do you plan to get the cached metadata for importScripts?

[horo, 2015-02-17 03:32:18]

https://codereview.chromium.org/929953002/diff/60001/Source/core/workers/Work...
File Source/core/workers/WorkerGlobalScope.h (right):

https://codereview.chromium.org/929953002/diff/60001/Source/core/workers/Work...
Source/core/workers/WorkerGlobalScope.h:103: virtual void
clearCachedMetadata(const KURL&) { }
On 2015/02/17 02:32:31, kinuko wrote:
> On 2015/02/16 16:20:12, horo wrote:
> > On 2015/02/16 14:18:38, kinuko wrote:
> > > Hmm.  We modify all the WorkerThreadStartupData, WorkerGlobalScope and
> > > WorkerThread to add cached metadata just for ServiceWorker (for now),
which
> > > feels a bit intrusive to me.
> > > 
> > > I think... we could probably introduce more specialized classes instead? 
> > > Something like:
> > > 
> > > - Define WorkerScriptCachedMetadataClient as a supplement of
WorkerClients,
> > > which has setCachedMetadata(), clearCachedMetadata() and
> > releaseCachedMetadata()
> > > methods
> > > - Implement ServiceWorkerScriptCachedMetadataClient, which holds cached
> > > metadata, and has reference to WebServiceWorkerContextClient
> > > - Instantiate ServiceWorkerScriptCachedMetadataClient in
> WebEmbeddedWorkerImpl
> > > and provide it to worker
> > > - In WorkerThread.cpp we create MetadataHandler with MetadataClient which
> can
> > be
> > > obtained via worker clients supplement interface
> > 
> > I want to use WorkerScriptCachedMetadataHandler in
> > WorkerGlobalScope::importScripts() to support v8 cache for imported scripts
in
> > future.
> > 
> > In this case, I think WorkerScriptCachedMetadataClient as a supplement of
> > WorkerClients is not suitable.
> 
> How do you plan to get the cached metadata for importScripts?

I will create WorkerScriptCachedMetadataHandler using scriptLoader's
cachedMetadata in the for loop of WorkerGlobalScope::importScripts() and pass it
to m_script->evaluate().

I created a WIP patch https://codereview.chromium.org/932773002

[horo, 2015-02-17 03:32:18]

https://codereview.chromium.org/929953002/diff/60001/Source/core/workers/Work...
File Source/core/workers/WorkerGlobalScope.h (right):

https://codereview.chromium.org/929953002/diff/60001/Source/core/workers/Work...
Source/core/workers/WorkerGlobalScope.h:103: virtual void
clearCachedMetadata(const KURL&) { }
On 2015/02/17 02:32:31, kinuko wrote:
> On 2015/02/16 16:20:12, horo wrote:
> > On 2015/02/16 14:18:38, kinuko wrote:
> > > Hmm.  We modify all the WorkerThreadStartupData, WorkerGlobalScope and
> > > WorkerThread to add cached metadata just for ServiceWorker (for now),
which
> > > feels a bit intrusive to me.
> > > 
> > > I think... we could probably introduce more specialized classes instead? 
> > > Something like:
> > > 
> > > - Define WorkerScriptCachedMetadataClient as a supplement of
WorkerClients,
> > > which has setCachedMetadata(), clearCachedMetadata() and
> > releaseCachedMetadata()
> > > methods
> > > - Implement ServiceWorkerScriptCachedMetadataClient, which holds cached
> > > metadata, and has reference to WebServiceWorkerContextClient
> > > - Instantiate ServiceWorkerScriptCachedMetadataClient in
> WebEmbeddedWorkerImpl
> > > and provide it to worker
> > > - In WorkerThread.cpp we create MetadataHandler with MetadataClient which
> can
> > be
> > > obtained via worker clients supplement interface
> > 
> > I want to use WorkerScriptCachedMetadataHandler in
> > WorkerGlobalScope::importScripts() to support v8 cache for imported scripts
in
> > future.
> > 
> > In this case, I think WorkerScriptCachedMetadataClient as a supplement of
> > WorkerClients is not suitable.
> 
> How do you plan to get the cached metadata for importScripts?

I will create WorkerScriptCachedMetadataHandler using scriptLoader's
cachedMetadata in the for loop of WorkerGlobalScope::importScripts() and pass it
to m_script->evaluate().

I created a WIP patch https://codereview.chromium.org/932773002

[kinuko, 2015-02-17 05:19:24]

https://codereview.chromium.org/929953002/diff/60001/Source/core/workers/Work...
File Source/core/workers/WorkerGlobalScope.h (right):

https://codereview.chromium.org/929953002/diff/60001/Source/core/workers/Work...
Source/core/workers/WorkerGlobalScope.h:103: virtual void
clearCachedMetadata(const KURL&) { }
On 2015/02/17 03:32:18, horo wrote:
> On 2015/02/17 02:32:31, kinuko wrote:
> > On 2015/02/16 16:20:12, horo wrote:
> > > On 2015/02/16 14:18:38, kinuko wrote:
> > > > Hmm.  We modify all the WorkerThreadStartupData, WorkerGlobalScope and
> > > > WorkerThread to add cached metadata just for ServiceWorker (for now),
> which
> > > > feels a bit intrusive to me.
> > > > 
> > > > I think... we could probably introduce more specialized classes instead?

> > > > Something like:
> > > > 
> > > > - Define WorkerScriptCachedMetadataClient as a supplement of
> WorkerClients,
> > > > which has setCachedMetadata(), clearCachedMetadata() and
> > > releaseCachedMetadata()
> > > > methods
> > > > - Implement ServiceWorkerScriptCachedMetadataClient, which holds cached
> > > > metadata, and has reference to WebServiceWorkerContextClient
> > > > - Instantiate ServiceWorkerScriptCachedMetadataClient in
> > WebEmbeddedWorkerImpl
> > > > and provide it to worker
> > > > - In WorkerThread.cpp we create MetadataHandler with MetadataClient
which
> > can
> > > be
> > > > obtained via worker clients supplement interface
> > > 
> > > I want to use WorkerScriptCachedMetadataHandler in
> > > WorkerGlobalScope::importScripts() to support v8 cache for imported
scripts
> in
> > > future.
> > > 
> > > In this case, I think WorkerScriptCachedMetadataClient as a supplement of
> > > WorkerClients is not suitable.
> > 
> > How do you plan to get the cached metadata for importScripts?
> 
> I will create WorkerScriptCachedMetadataHandler using scriptLoader's
> cachedMetadata in the for loop of WorkerGlobalScope::importScripts() and pass
it
> to m_script->evaluate().
> 
> I created a WIP patch https://codereview.chromium.org/932773002

Thanks. So having the client hold cached metadata is not good, I see. I still
feel it'd be a bit cleaner to have a dedicated client interface for handling
setCachedMetadata() / clearCachedMetadata() methods, but that might be an
overkill. Hmm. (I'm going to chat with you)

[horo, 2015-02-17 07:17:43]

New patchsets have been uploaded after l-g-t-m from vogelheim@chromium.org

[horo, 2015-02-17 07:35:02]

On 2015/02/17 05:19:24, kinuko wrote:
>
https://codereview.chromium.org/929953002/diff/60001/Source/core/workers/Work...
> File Source/core/workers/WorkerGlobalScope.h (right):
> 
>
https://codereview.chromium.org/929953002/diff/60001/Source/core/workers/Work...
> Source/core/workers/WorkerGlobalScope.h:103: virtual void
> clearCachedMetadata(const KURL&) { }
> On 2015/02/17 03:32:18, horo wrote:
> > On 2015/02/17 02:32:31, kinuko wrote:
> > > On 2015/02/16 16:20:12, horo wrote:
> > > > On 2015/02/16 14:18:38, kinuko wrote:
> > > > > Hmm.  We modify all the WorkerThreadStartupData, WorkerGlobalScope and
> > > > > WorkerThread to add cached metadata just for ServiceWorker (for now),
> > which
> > > > > feels a bit intrusive to me.
> > > > > 
> > > > > I think... we could probably introduce more specialized classes
instead?
> 
> > > > > Something like:
> > > > > 
> > > > > - Define WorkerScriptCachedMetadataClient as a supplement of
> > WorkerClients,
> > > > > which has setCachedMetadata(), clearCachedMetadata() and
> > > > releaseCachedMetadata()
> > > > > methods
> > > > > - Implement ServiceWorkerScriptCachedMetadataClient, which holds
cached
> > > > > metadata, and has reference to WebServiceWorkerContextClient
> > > > > - Instantiate ServiceWorkerScriptCachedMetadataClient in
> > > WebEmbeddedWorkerImpl
> > > > > and provide it to worker
> > > > > - In WorkerThread.cpp we create MetadataHandler with MetadataClient
> which
> > > can
> > > > be
> > > > > obtained via worker clients supplement interface
> > > > 
> > > > I want to use WorkerScriptCachedMetadataHandler in
> > > > WorkerGlobalScope::importScripts() to support v8 cache for imported
> scripts
> > in
> > > > future.
> > > > 
> > > > In this case, I think WorkerScriptCachedMetadataClient as a supplement
of
> > > > WorkerClients is not suitable.
> > > 
> > > How do you plan to get the cached metadata for importScripts?
> > 
> > I will create WorkerScriptCachedMetadataHandler using scriptLoader's
> > cachedMetadata in the for loop of WorkerGlobalScope::importScripts() and
pass
> it
> > to m_script->evaluate().
> > 
> > I created a WIP patch https://codereview.chromium.org/932773002
> 
> Thanks. So having the client hold cached metadata is not good, I see. I still
> feel it'd be a bit cleaner to have a dedicated client interface for handling
> setCachedMetadata() / clearCachedMetadata() methods, but that might be an
> overkill. Hmm. (I'm going to chat with you)


As we talked offline, I removed
WorkerGlobalScope::setCachedMetadata/clearCachedMetadata and added
createWorkerScriptCachedMetadataHandler.

[kinuko, 2015-02-17 07:56:46]

Thanks, lgtm

https://codereview.chromium.org/929953002/diff/160001/Source/core/fetch/Resou...
File Source/core/fetch/Resource.h (right):

https://codereview.chromium.org/929953002/diff/160001/Source/core/fetch/Resou...
Source/core/fetch/Resource.h:197: // This may returns nullptr when the resource
isn't cacheable.
nit: returns -> return

https://codereview.chromium.org/929953002/diff/160001/Source/core/workers/Wor...
File Source/core/workers/WorkerGlobalScope.h (right):

https://codereview.chromium.org/929953002/diff/160001/Source/core/workers/Wor...
Source/core/workers/WorkerGlobalScope.h:104: virtual
PassOwnPtr<CachedMetadataHandler> createWorkerScriptCachedMetadataHandler(const
KURL& /* scriptURL */, const Vector<char>* /* metaData */) { return nullptr; }
nit: I don't think we need to make the parameter names as comments here, as the
type names don't imply the meaning?

https://codereview.chromium.org/929953002/diff/160001/Source/modules/servicew...
File Source/modules/serviceworkers/ServiceWorkerGlobalScope.h (right):

https://codereview.chromium.org/929953002/diff/160001/Source/modules/servicew...
Source/modules/serviceworkers/ServiceWorkerGlobalScope.h:101: virtual
PassOwnPtr<CachedMetadataHandler> createWorkerScriptCachedMetadataHandler(const
KURL& /* scriptURL */, const Vector<char>* /* metaData */);
ditto (for commented param names)

https://codereview.chromium.org/929953002/diff/160001/Source/modules/servicew...
File Source/modules/serviceworkers/ServiceWorkerScriptCachedMetadataHandler.h
(right):

https://codereview.chromium.org/929953002/diff/160001/Source/modules/servicew...
Source/modules/serviceworkers/ServiceWorkerScriptCachedMetadataHandler.h:25:
void setCachedMetadata(unsigned /* dataTypeID */, const char*, size_t,
CacheType) override;
ditto (for commented parameter names)

[horo, 2015-02-17 08:18:24]

New patchsets have been uploaded after l-g-t-m from kinuko@chromium.org

[horo, 2015-02-17 08:20:27]

https://codereview.chromium.org/929953002/diff/160001/Source/core/fetch/Resou...
File Source/core/fetch/Resource.h (right):

https://codereview.chromium.org/929953002/diff/160001/Source/core/fetch/Resou...
Source/core/fetch/Resource.h:197: // This may returns nullptr when the resource
isn't cacheable.
On 2015/02/17 07:56:45, kinuko wrote:
> nit: returns -> return

Done.

https://codereview.chromium.org/929953002/diff/160001/Source/core/workers/Wor...
File Source/core/workers/WorkerGlobalScope.h (right):

https://codereview.chromium.org/929953002/diff/160001/Source/core/workers/Wor...
Source/core/workers/WorkerGlobalScope.h:104: virtual
PassOwnPtr<CachedMetadataHandler> createWorkerScriptCachedMetadataHandler(const
KURL& /* scriptURL */, const Vector<char>* /* metaData */) { return nullptr; }
On 2015/02/17 07:56:46, kinuko wrote:
> nit: I don't think we need to make the parameter names as comments here, as
the
> type names don't imply the meaning?

Done.

https://codereview.chromium.org/929953002/diff/160001/Source/modules/servicew...
File Source/modules/serviceworkers/ServiceWorkerGlobalScope.h (right):

https://codereview.chromium.org/929953002/diff/160001/Source/modules/servicew...
Source/modules/serviceworkers/ServiceWorkerGlobalScope.h:101: virtual
PassOwnPtr<CachedMetadataHandler> createWorkerScriptCachedMetadataHandler(const
KURL& /* scriptURL */, const Vector<char>* /* metaData */);
On 2015/02/17 07:56:46, kinuko wrote:
> ditto (for commented param names)

Done.

https://codereview.chromium.org/929953002/diff/160001/Source/modules/servicew...
File Source/modules/serviceworkers/ServiceWorkerScriptCachedMetadataHandler.h
(right):

https://codereview.chromium.org/929953002/diff/160001/Source/modules/servicew...
Source/modules/serviceworkers/ServiceWorkerScriptCachedMetadataHandler.h:25:
void setCachedMetadata(unsigned /* dataTypeID */, const char*, size_t,
CacheType) override;
On 2015/02/17 07:56:46, kinuko wrote:
> ditto (for commented parameter names)

Done.

[horo, 2015-02-17 08:21:59]

horo@chromium.org changed reviewers:
+ dcheng@chromium.org, marja@chromium.org

[horo, 2015-02-17 08:22:00]

marja@
Could you please review Source/bindings/core/v8/*?

dcheng@
Could you please review Source/web/ServiceWorkerGlobalScopeClientImpl.* and
public/web/WebServiceWorkerContextClient.h?

Thank you.

[marja, 2015-02-17 11:31:58]

lgtm modulo comments

https://codereview.chromium.org/929953002/diff/180001/Source/bindings/core/v8...
File Source/bindings/core/v8/V8ScriptRunner.cpp (right):

https://codereview.chromium.org/929953002/diff/180001/Source/bindings/core/v8...
Source/bindings/core/v8/V8ScriptRunner.cpp:235: if (!cacheHandler)
I don't think this can ever happen (or can it?), but doesn't harm to have the
code here for future-proofness...

https://codereview.chromium.org/929953002/diff/180001/Source/bindings/core/v8...
Source/bindings/core/v8/V8ScriptRunner.cpp:362: return
compileScript(v8String(isolate, source.source()), source.url(),
source.startPosition(), isolate, source.resource(), source.streamer(), nullptr,
corsStatus, cacheOptions);
Why nullptr as cacheHandler here, why not source.resource().cacheHandler() (or
nullptr if source.resource() is null)?

https://codereview.chromium.org/929953002/diff/180001/Source/bindings/core/v8...
File Source/bindings/core/v8/V8ScriptRunner.h (right):

https://codereview.chromium.org/929953002/diff/180001/Source/bindings/core/v8...
Source/bindings/core/v8/V8ScriptRunner.h:50: static v8::Local<v8::Script>
compileScript(v8::Handle<v8::String>, const String& fileName, const
TextPosition&, v8::Isolate*, ScriptResource* = nullptr, ScriptStreamer* =
nullptr, CachedMetadataHandler* = nullptr, AccessControlStatus =
SharableCrossOrigin, V8CacheOptions = V8CacheOptionsDefault, bool
isInternalScript = false);
I was confused here why we need both Resource and CachedMetaDataHandler, but
then vogelheim@ explained that *usually* cachedmetadatahandler is from the
resource, but sometimes we don't have a resource, just a cachedmetadatahandler.

Pls add a comment about that, and also an ASSERT that those are the valid cases.

https://codereview.chromium.org/929953002/diff/180001/Source/bindings/core/v8...
File Source/bindings/core/v8/V8ScriptRunnerTest.cpp (right):

https://codereview.chromium.org/929953002/diff/180001/Source/bindings/core/v8...
Source/bindings/core/v8/V8ScriptRunnerTest.cpp:91: CachedMetadataHandler*
m_cacheHandler;
Instead of adding this member, could you add a helper function which gets it
from m_resource? That's always what we're doing it, right? Or is there a reason
why it needs to be stored separately?

[horo, 2015-02-17 12:44:03]

New patchsets have been uploaded after l-g-t-m from marja@chromium.org

[marja, 2015-02-17 12:45:59]

https://codereview.chromium.org/929953002/diff/200001/Source/bindings/core/v8...
File Source/bindings/core/v8/V8ScriptRunner.cpp (right):

https://codereview.chromium.org/929953002/diff/200001/Source/bindings/core/v8...
Source/bindings/core/v8/V8ScriptRunner.cpp:369: // When streamer is set,
resource must be set.
This comment is unnecessary though :)

https://codereview.chromium.org/929953002/diff/200001/Source/bindings/core/v8...
Source/bindings/core/v8/V8ScriptRunner.cpp:370: ASSERT(!streamer || resource);
I'd add here an ASSERT which says the if there is a resource, the cachehandler
is its cachehandler.

[horo, 2015-02-17 12:53:43]

Thank you

https://codereview.chromium.org/929953002/diff/180001/Source/bindings/core/v8...
File Source/bindings/core/v8/V8ScriptRunner.cpp (right):

https://codereview.chromium.org/929953002/diff/180001/Source/bindings/core/v8...
Source/bindings/core/v8/V8ScriptRunner.cpp:235: if (!cacheHandler)
On 2015/02/17 11:31:58, marja wrote:
> I don't think this can ever happen (or can it?), but doesn't harm to have the
> code here for future-proofness...

This can happen.
When the resource is not from HTTP family, resource->cacheHandler() returns
null.

https://codereview.chromium.org/929953002/diff/180001/Source/bindings/core/v8...
Source/bindings/core/v8/V8ScriptRunner.cpp:362: return
compileScript(v8String(isolate, source.source()), source.url(),
source.startPosition(), isolate, source.resource(), source.streamer(), nullptr,
corsStatus, cacheOptions);
On 2015/02/17 11:31:58, marja wrote:
> Why nullptr as cacheHandler here, why not source.resource().cacheHandler() (or
> nullptr if source.resource() is null)?

This should be "source.resource() ? source.resource()->cacheHandler() : nullptr"
Done.

https://codereview.chromium.org/929953002/diff/180001/Source/bindings/core/v8...
File Source/bindings/core/v8/V8ScriptRunner.h (right):

https://codereview.chromium.org/929953002/diff/180001/Source/bindings/core/v8...
Source/bindings/core/v8/V8ScriptRunner.h:50: static v8::Local<v8::Script>
compileScript(v8::Handle<v8::String>, const String& fileName, const
TextPosition&, v8::Isolate*, ScriptResource* = nullptr, ScriptStreamer* =
nullptr, CachedMetadataHandler* = nullptr, AccessControlStatus =
SharableCrossOrigin, V8CacheOptions = V8CacheOptionsDefault, bool
isInternalScript = false);
On 2015/02/17 11:31:58, marja wrote:
> I was confused here why we need both Resource and CachedMetaDataHandler, but
> then vogelheim@ explained that *usually* cachedmetadatahandler is from the
> resource, but sometimes we don't have a resource, just a
cachedmetadatahandler.
> 
> Pls add a comment about that, and also an ASSERT that those are the valid
cases.

Done.

https://codereview.chromium.org/929953002/diff/180001/Source/bindings/core/v8...
File Source/bindings/core/v8/V8ScriptRunnerTest.cpp (right):

https://codereview.chromium.org/929953002/diff/180001/Source/bindings/core/v8...
Source/bindings/core/v8/V8ScriptRunnerTest.cpp:91: CachedMetadataHandler*
m_cacheHandler;
On 2015/02/17 11:31:58, marja wrote:
> Instead of adding this member, could you add a helper function which gets it
> from m_resource? That's always what we're doing it, right? Or is there a
reason
> why it needs to be stored separately?

Done.
Added cacheHandler() function.

https://codereview.chromium.org/929953002/diff/200001/Source/bindings/core/v8...
File Source/bindings/core/v8/V8ScriptRunner.cpp (right):

https://codereview.chromium.org/929953002/diff/200001/Source/bindings/core/v8...
Source/bindings/core/v8/V8ScriptRunner.cpp:369: // When streamer is set,
resource must be set.
On 2015/02/17 12:45:59, marja wrote:
> This comment is unnecessary though :)

Done.

https://codereview.chromium.org/929953002/diff/200001/Source/bindings/core/v8...
Source/bindings/core/v8/V8ScriptRunner.cpp:370: ASSERT(!streamer || resource);
On 2015/02/17 12:45:59, marja wrote:
> I'd add here an ASSERT which says the if there is a resource, the cachehandler
> is its cachehandler.

Done.

[michaeln, 2015-02-18 03:13:17]

michaeln@chromium.org changed reviewers:
+ michaeln@chromium.org

[michaeln, 2015-02-18 03:13:18]

drive-by

https://codereview.chromium.org/929953002/diff/220001/Source/bindings/core/v8...
File Source/bindings/core/v8/V8ScriptRunner.cpp (right):

https://codereview.chromium.org/929953002/diff/220001/Source/bindings/core/v8...
Source/bindings/core/v8/V8ScriptRunner.cpp:166:
cacheHandler->clearCachedMetadata();
Maybe pass in CachedMetadataHandler::CacheLocally explicitly here. I was
wondering if we could end up with two dualing metedatawriters in the backend.

https://codereview.chromium.org/929953002/diff/220001/Source/bindings/core/v8...
Source/bindings/core/v8/V8ScriptRunner.cpp:242:
cacheHandler->clearCachedMetadata();
and here

[horo, 2015-02-18 03:45:04]

https://codereview.chromium.org/929953002/diff/220001/Source/bindings/core/v8...
File Source/bindings/core/v8/V8ScriptRunner.cpp (right):

https://codereview.chromium.org/929953002/diff/220001/Source/bindings/core/v8...
Source/bindings/core/v8/V8ScriptRunner.cpp:166:
cacheHandler->clearCachedMetadata();
On 2015/02/18 03:13:18, michaeln wrote:
> Maybe pass in CachedMetadataHandler::CacheLocally explicitly here. I was
> wondering if we could end up with two dualing metedatawriters in the backend.

Done.

https://codereview.chromium.org/929953002/diff/220001/Source/bindings/core/v8...
Source/bindings/core/v8/V8ScriptRunner.cpp:242:
cacheHandler->clearCachedMetadata();
On 2015/02/18 03:13:18, michaeln wrote:
> and here

Done.

[horo, 2015-02-18 04:33:16]

horo@chromium.org changed reviewers:
- dcheng@chromium.org

[horo, 2015-02-18 04:34:37]

horo@chromium.org changed reviewers:
+ tkent@chromium.org

[horo, 2015-02-18 04:34:38]

tkent@
Could you please review Source/web/ServiceWorkerGlobalScopeClientImpl.* and
public/web/WebServiceWorkerContextClient.h?
Thank you!

[tkent, 2015-02-18 04:47:21]

lgtm

https://codereview.chromium.org/929953002/diff/240001/Source/core/fetch/Cache...
File Source/core/fetch/CachedMetadataHandler.h (right):

https://codereview.chromium.org/929953002/diff/240001/Source/core/fetch/Cache...
Source/core/fetch/CachedMetadataHandler.h:8: #include "wtf/text/WTFString.h"
nit: probably we can use wtf/Forward.h instead of WTFString.h.

https://codereview.chromium.org/929953002/diff/240001/Source/modules/servicew...
File Source/modules/serviceworkers/ServiceWorkerScriptCachedMetadataHandler.cpp
(right):

https://codereview.chromium.org/929953002/diff/240001/Source/modules/servicew...
Source/modules/serviceworkers/ServiceWorkerScriptCachedMetadataHandler.cpp:6: 
nit: We usually add no blank line between configh and header-for-this-class.h

https://codereview.chromium.org/929953002/diff/240001/Source/modules/servicew...
Source/modules/serviceworkers/ServiceWorkerScriptCachedMetadataHandler.cpp:53:
return "";
nit: Use emptyString() to avoid unnecessary String creation.

[horo, 2015-02-18 04:56:21]

New patchsets have been uploaded after l-g-t-m from tkent@chromium.org

[horo, 2015-02-18 04:56:59]

Thank you!!

https://codereview.chromium.org/929953002/diff/240001/Source/core/fetch/Cache...
File Source/core/fetch/CachedMetadataHandler.h (right):

https://codereview.chromium.org/929953002/diff/240001/Source/core/fetch/Cache...
Source/core/fetch/CachedMetadataHandler.h:8: #include "wtf/text/WTFString.h"
On 2015/02/18 04:47:20, tkent wrote:
> nit: probably we can use wtf/Forward.h instead of WTFString.h.

Done.

https://codereview.chromium.org/929953002/diff/240001/Source/modules/servicew...
File Source/modules/serviceworkers/ServiceWorkerScriptCachedMetadataHandler.cpp
(right):

https://codereview.chromium.org/929953002/diff/240001/Source/modules/servicew...
Source/modules/serviceworkers/ServiceWorkerScriptCachedMetadataHandler.cpp:6: 
On 2015/02/18 04:47:20, tkent wrote:
> nit: We usually add no blank line between configh and header-for-this-class.h

Done.

https://codereview.chromium.org/929953002/diff/240001/Source/modules/servicew...
Source/modules/serviceworkers/ServiceWorkerScriptCachedMetadataHandler.cpp:53:
return "";
On 2015/02/18 04:47:20, tkent wrote:
> nit: Use emptyString() to avoid unnecessary String creation.

Done.

[horo, 2015-02-18 04:57:05]

The CQ bit was checked by horo@chromium.org

[commit-bot: I haz the power, 2015-02-18 04:57:27]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/929953002/260001

[commit-bot: I haz the power, 2015-02-18 06:21:21]

Committed patchset #14 (id:260001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=190399