Initial implementation of ServiceWorkerGlobalScope.fetch()

Initial implementation of ServiceWorkerGlobalScope.fetch()

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

[horo, 2014-06-09 09:16:08]

yhirano@, tyoshino@
Could you please review?

I'm not so familiar with loader codes now.
So this may be completely wrong.
I would appreciate it very much if you could point out anything to me.

Thank you.

[dominicc (has gone to gerrit), 2014-06-09 20:11:00]

I would love to see the tests for this, including destroying the execution
context while fetch is happening if possible (do existing worker tests have a
test hook for that?)

https://codereview.chromium.org/318393002/diff/1/Source/modules/serviceworker...
File Source/modules/serviceworkers/FetchManager.cpp (right):

https://codereview.chromium.org/318393002/diff/1/Source/modules/serviceworker...
Source/modules/serviceworkers/FetchManager.cpp:114: if (!m_loader) {
No {s for single-statement bodies.

https://codereview.chromium.org/318393002/diff/1/Source/modules/serviceworker...
Source/modules/serviceworkers/FetchManager.cpp:140: :
m_executionContext(executionContext)
Who manages the lifetime of the FetchManager? What happens if the
ExecutionContext dies while the FetchManager is alive/loading?

https://codereview.chromium.org/318393002/diff/1/Source/modules/serviceworker...
Source/modules/serviceworkers/FetchManager.cpp:157: Loader* tmpLoader =
loader.get();
Maybe instead of loader/tmpLoader it is neater to call this ownLoader/loader?

[horo, 2014-06-10 03:34:52]

We don't have the way to terminate the service worker in LayoutTests.
So we should create browser tests which destroys the execution context while
fetch is happening.
Is my understanding correct?

https://codereview.chromium.org/318393002/diff/1/Source/modules/serviceworker...
File Source/modules/serviceworkers/FetchManager.cpp (right):

https://codereview.chromium.org/318393002/diff/1/Source/modules/serviceworker...
Source/modules/serviceworkers/FetchManager.cpp:114: if (!m_loader) {
On 2014/06/09 20:11:00, dominicc wrote:
> No {s for single-statement bodies.

Done.

https://codereview.chromium.org/318393002/diff/1/Source/modules/serviceworker...
Source/modules/serviceworkers/FetchManager.cpp:140: :
m_executionContext(executionContext)
On 2014/06/09 20:11:00, dominicc wrote:
> Who manages the lifetime of the FetchManager? What happens if the
> ExecutionContext dies while the FetchManager is alive/loading?

I added ServiceWorkerGlobalScope::stopFetch() to stop the running fetches.

https://codereview.chromium.org/318393002/diff/1/Source/modules/serviceworker...
Source/modules/serviceworkers/FetchManager.cpp:157: Loader* tmpLoader =
loader.get();
On 2014/06/09 20:11:00, dominicc wrote:
> Maybe instead of loader/tmpLoader it is neater to call this ownLoader/loader?

Done.

[yhirano, 2014-06-10 04:57:54]

https://codereview.chromium.org/318393002/diff/1/Source/modules/serviceworker...
File Source/modules/serviceworkers/FetchManager.cpp (right):

https://codereview.chromium.org/318393002/diff/1/Source/modules/serviceworker...
Source/modules/serviceworkers/FetchManager.cpp:81: m_resolver.clear();
Is this clear needed?

https://codereview.chromium.org/318393002/diff/1/Source/modules/serviceworker...
Source/modules/serviceworkers/FetchManager.cpp:115:
m_resolver->reject(DOMError::create(InvalidStateError));
DOMException?

https://codereview.chromium.org/318393002/diff/1/Source/modules/serviceworker...
Source/modules/serviceworkers/FetchManager.cpp:128:
m_resolver->reject(DOMError::create(NetworkError));
DOMException

https://codereview.chromium.org/318393002/diff/1/Source/modules/serviceworker...
Source/modules/serviceworkers/FetchManager.cpp:129: m_resolver.clear();
Is this statement needed?

https://codereview.chromium.org/318393002/diff/1/Source/modules/serviceworker...
Source/modules/serviceworkers/FetchManager.cpp:157: Loader* tmpLoader =
loader.get();
You can get a pointer to the ownptr from the result of HashSet::add.

https://codereview.chromium.org/318393002/diff/1/Source/modules/serviceworker...
File Source/modules/serviceworkers/Request.cpp (right):

https://codereview.chromium.org/318393002/diff/1/Source/modules/serviceworker...
Source/modules/serviceworkers/Request.cpp:51: // TODO(horo): Fill more info.
FIXME

[tyoshino (SeeGerritForStatus), 2014-06-10 05:18:38]

https://codereview.chromium.org/318393002/diff/40001/Source/modules/servicewo...
File Source/modules/serviceworkers/ServiceWorkerGlobalScope.cpp (right):

https://codereview.chromium.org/318393002/diff/40001/Source/modules/servicewo...
Source/modules/serviceworkers/ServiceWorkerGlobalScope.cpp:84: KURL url =
completeURL(urlstring);
if (!url.isValid())

throw TypeError

[horo, 2014-06-10 06:20:46]

https://codereview.chromium.org/318393002/diff/1/Source/modules/serviceworker...
File Source/modules/serviceworkers/FetchManager.cpp (right):

https://codereview.chromium.org/318393002/diff/1/Source/modules/serviceworker...
Source/modules/serviceworkers/FetchManager.cpp:81: m_resolver.clear();
On 2014/06/10 04:57:53, yhirano wrote:
> Is this clear needed?

Done.

https://codereview.chromium.org/318393002/diff/1/Source/modules/serviceworker...
Source/modules/serviceworkers/FetchManager.cpp:115:
m_resolver->reject(DOMError::create(InvalidStateError));
On 2014/06/10 04:57:53, yhirano wrote:
> DOMException?

Done.

https://codereview.chromium.org/318393002/diff/1/Source/modules/serviceworker...
Source/modules/serviceworkers/FetchManager.cpp:128:
m_resolver->reject(DOMError::create(NetworkError));
On 2014/06/10 04:57:54, yhirano wrote:
> DOMException

Done.

https://codereview.chromium.org/318393002/diff/1/Source/modules/serviceworker...
Source/modules/serviceworkers/FetchManager.cpp:129: m_resolver.clear();
On 2014/06/10 04:57:53, yhirano wrote:
> Is this statement needed?

Done removed.

It was intended to avoid calling m_resolver->reject more than once.

If my understanding is correct, ThreadableLoaderClient::fail() could be called
more than once.
But FetchManager::Loader::failed() is called only once.
It is because when we call notiyfyFinished() the Loader will be deleted and
m_loader->cancel() sets ThreadableLoaderClientWrapper's m_client to 0.
So ThreadableLoaderClientWrapper::fail() don't call m_client->fail() anymore.

https://codereview.chromium.org/318393002/diff/1/Source/modules/serviceworker...
Source/modules/serviceworkers/FetchManager.cpp:157: Loader* tmpLoader =
loader.get();
On 2014/06/10 04:57:53, yhirano wrote:
> You can get a pointer to the ownptr from the result of HashSet::add.

Done.

[horo, 2014-06-10 06:44:59]

https://codereview.chromium.org/318393002/diff/40001/Source/modules/servicewo...
File Source/modules/serviceworkers/ServiceWorkerGlobalScope.cpp (right):

https://codereview.chromium.org/318393002/diff/40001/Source/modules/servicewo...
Source/modules/serviceworkers/ServiceWorkerGlobalScope.cpp:84: KURL url =
completeURL(urlstring);
On 2014/06/10 05:18:38, tyoshino wrote:
> if (!url.isValid())
> 
> throw TypeError

Done.

[tyoshino (SeeGerritForStatus), 2014-06-11 04:57:39]

On 2014/06/10 03:34:52, horo wrote:
> We don't have the way to terminate the service worker in LayoutTests.
> So we should create browser tests which destroys the execution context while
> fetch is happening.
> Is my understanding correct?

Should we consider adding testRunner APIs to ease ServiceWorker layouttest-ing?

[dominicc (has gone to gerrit), 2014-06-11 06:40:16]

On Jun 10, 2014 9:57 PM, <tyoshino@chromium.org> wrote:
>
> On 2014/06/10 03:34:52, horo wrote:
>>
>> We don't have the way to terminate the service worker in LayoutTests.
>> So we should create browser tests which destroys the execution context
while
>> fetch is happening.
>> Is my understanding correct?

That would be fine.

>
> Should we consider adding testRunner APIs to ease ServiceWorker
layouttest-ing?

That would be great. That can be in a follow up change.

>
> https://codereview.chromium.org/318393002/

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[yhirano, 2014-06-11 06:51:30]

lgtm

Please use FIXME rather than TODO.

https://codereview.chromium.org/318393002/diff/1/Source/modules/serviceworker...
File Source/modules/serviceworkers/FetchManager.cpp (right):

https://codereview.chromium.org/318393002/diff/1/Source/modules/serviceworker...
Source/modules/serviceworkers/FetchManager.cpp:129: m_resolver.clear();
On 2014/06/10 06:20:46, horo wrote:
> On 2014/06/10 04:57:53, yhirano wrote:
> > Is this statement needed?
> 
> Done removed.
> 
> It was intended to avoid calling m_resolver->reject more than once.
> 
> If my understanding is correct, ThreadableLoaderClient::fail() could be called
> more than once.
> But FetchManager::Loader::failed() is called only once.
> It is because when we call notiyfyFinished() the Loader will be deleted and
> m_loader->cancel() sets ThreadableLoaderClientWrapper's m_client to 0.
> So ThreadableLoaderClientWrapper::fail() don't call m_client->fail() anymore.

It is safe to call resolve or reject more than once. From the second time, they
will be ignored.

https://codereview.chromium.org/318393002/diff/80001/Source/modules/servicewo...
File Source/modules/serviceworkers/ServiceWorkerGlobalScope.cpp (right):

https://codereview.chromium.org/318393002/diff/80001/Source/modules/servicewo...
Source/modules/serviceworkers/ServiceWorkerGlobalScope.cpp:85: if
(!url.isValid()) {
I think returning ScriptPromise::reject(V8ThrowException::createTypeError(...))
is better.

https://codereview.chromium.org/318393002/diff/100001/Source/modules/servicew...
File Source/modules/serviceworkers/FetchManager.cpp (right):

https://codereview.chromium.org/318393002/diff/100001/Source/modules/servicew...
Source/modules/serviceworkers/FetchManager.cpp:9: #include "core/dom/DOMError.h"
DOMError is now not used.

[horo, 2014-06-11 09:06:23]

https://codereview.chromium.org/318393002/diff/80001/Source/modules/servicewo...
File Source/modules/serviceworkers/ServiceWorkerGlobalScope.cpp (right):

https://codereview.chromium.org/318393002/diff/80001/Source/modules/servicewo...
Source/modules/serviceworkers/ServiceWorkerGlobalScope.cpp:85: if
(!url.isValid()) {
On 2014/06/11 06:51:30, yhirano wrote:
> I think returning
ScriptPromise::reject(V8ThrowException::createTypeError(...))
> is better.

Done.

https://codereview.chromium.org/318393002/diff/100001/Source/modules/servicew...
File Source/modules/serviceworkers/FetchManager.cpp (right):

https://codereview.chromium.org/318393002/diff/100001/Source/modules/servicew...
Source/modules/serviceworkers/FetchManager.cpp:9: #include "core/dom/DOMError.h"
On 2014/06/11 06:51:30, yhirano wrote:
> DOMError is now not used.

Done.

[yhirano, 2014-06-11 09:10:49]

https://codereview.chromium.org/318393002/diff/120001/Source/modules/servicew...
File Source/modules/serviceworkers/ServiceWorkerGlobalScope.cpp (right):

https://codereview.chromium.org/318393002/diff/120001/Source/modules/servicew...
Source/modules/serviceworkers/ServiceWorkerGlobalScope.cpp:93:
RefPtr<ScriptPromiseResolver> resolver =
ScriptPromiseResolver::create(scriptState);
[opt] You can use ScriptPromise::reject, if you prefer.

[tyoshino (SeeGerritForStatus), 2014-06-11 09:12:01]

https://codereview.chromium.org/318393002/diff/100001/Source/modules/servicew...
File Source/modules/serviceworkers/FetchManager.cpp (right):

https://codereview.chromium.org/318393002/diff/100001/Source/modules/servicew...
Source/modules/serviceworkers/FetchManager.cpp:84: failed();
I suggest that you have a flag e.g. m_failed to ensure duplicated failed() call
won't happen.

https://codereview.chromium.org/318393002/diff/100001/Source/modules/servicew...
Source/modules/serviceworkers/FetchManager.cpp:112:
m_resolver->reject(DOMException::create(InvalidStateError));
maybe this also should be TypeError. is there any spec about this?

https://codereview.chromium.org/318393002/diff/100001/Source/modules/servicew...
Source/modules/serviceworkers/FetchManager.cpp:117: m_fetchManager = 0;
add comment like "prevent notification"

https://codereview.chromium.org/318393002/diff/100001/Source/modules/servicew...
Source/modules/serviceworkers/FetchManager.cpp:126:
m_resolver->reject(DOMException::create(NetworkError));
This should be a TypeError. I also had doubts and asked Anne about this at
public-webapps.

https://codereview.chromium.org/318393002/diff/100001/Source/modules/servicew...
Source/modules/serviceworkers/FetchManager.cpp:130: void
FetchManager::Loader::notiyfyFinished()
notiyfy -> notify

https://codereview.chromium.org/318393002/diff/100001/Source/modules/servicew...
Source/modules/serviceworkers/FetchManager.cpp:153: OwnPtr<Loader>
loader(adoptPtr(new Loader(m_executionContext, this, resolver, request)));
resolver.release()

https://codereview.chromium.org/318393002/diff/100001/Source/modules/servicew...
Source/modules/serviceworkers/FetchManager.cpp:158: void
FetchManager::removeLoader(Loader* loader)
how about naming this like onLoaderFinished?

[horo, 2014-06-11 11:22:12]

https://codereview.chromium.org/318393002/diff/100001/Source/modules/servicew...
File Source/modules/serviceworkers/FetchManager.cpp (right):

https://codereview.chromium.org/318393002/diff/100001/Source/modules/servicew...
Source/modules/serviceworkers/FetchManager.cpp:84: failed();
On 2014/06/11 09:12:01, tyoshino wrote:
> I suggest that you have a flag e.g. m_failed to ensure duplicated failed()
call
> won't happen.

Done.

https://codereview.chromium.org/318393002/diff/100001/Source/modules/servicew...
Source/modules/serviceworkers/FetchManager.cpp:112:
m_resolver->reject(DOMException::create(InvalidStateError));
On 2014/06/11 09:12:01, tyoshino wrote:
> maybe this also should be TypeError. is there any spec about this?

Deleted this lines.
m_executionContext->isWorkerGlobalScope() is true, so ThreadableLoader::create()
returns WorkerThreadableLoader::create() which doesn't return null.

https://codereview.chromium.org/318393002/diff/100001/Source/modules/servicew...
Source/modules/serviceworkers/FetchManager.cpp:117: m_fetchManager = 0;
On 2014/06/11 09:12:01, tyoshino wrote:
> add comment like "prevent notification"

Done.

https://codereview.chromium.org/318393002/diff/100001/Source/modules/servicew...
Source/modules/serviceworkers/FetchManager.cpp:126:
m_resolver->reject(DOMException::create(NetworkError));
On 2014/06/11 09:12:01, tyoshino wrote:
> This should be a TypeError. I also had doubts and asked Anne about this at
> public-webapps.


Could you please let me know how to create TypeError here?
When I call "DOMError::create(TypeError)", it crashes at ASSERT in
DOMException::getErrorName().
When I call "DOMException::create(TypeError)", it crashes at ASSERT in
DOMException::create().
When I call "V8ThrowException::createTypeError("Fetch error",
m_resolver->scriptState()->isolate())". it crashes in
WebCore::reportFatalErrorInWorker.
  1  WebCore::reportFatalErrorInWorker(char const*, char const*)
  2  v8::Utils::ReportApiFailure(char const*, char const*)
  3  v8::internal::HandleScope::Extend(v8::internal::Isolate*)
  4  v8::internal::Handle<v8::internal::ExternalAsciiString>
v8::internal::Factory::New<v8::internal::ExternalAsciiString>(v8::internal::Handle<v8::internal::Map>,
v8::internal::AllocationSpace)
  5 
v8::internal::Factory::NewExternalStringFromAscii(v8::String::ExternalAsciiStringResource
const*)
  6  v8::String::NewExternal(v8::Isolate*,
v8::String::ExternalAsciiStringResource*)
  7  WebCore::makeExternalString(WTF::String const&, v8::Isolate*)
  8  WebCore::StringCache::createStringAndInsertIntoCache(WTF::StringImpl*,
v8::Isolate*)
  9  WebCore::StringCache::v8ExternalStringSlow(WTF::StringImpl*, v8::Isolate*)
  10 WebCore::StringCache::v8ExternalString(WTF::StringImpl*, v8::Isolate*)
  11 WebCore::v8String(v8::Isolate*, WTF::String const&)
  12 WebCore::V8ThrowException::createTypeError(WTF::String const&,
v8::Isolate*)

https://codereview.chromium.org/318393002/diff/100001/Source/modules/servicew...
Source/modules/serviceworkers/FetchManager.cpp:130: void
FetchManager::Loader::notiyfyFinished()
On 2014/06/11 09:12:01, tyoshino wrote:
> notiyfy -> notify

Done.

https://codereview.chromium.org/318393002/diff/100001/Source/modules/servicew...
Source/modules/serviceworkers/FetchManager.cpp:153: OwnPtr<Loader>
loader(adoptPtr(new Loader(m_executionContext, this, resolver, request)));
On 2014/06/11 09:12:01, tyoshino wrote:
> resolver.release()

Done.

https://codereview.chromium.org/318393002/diff/100001/Source/modules/servicew...
Source/modules/serviceworkers/FetchManager.cpp:158: void
FetchManager::removeLoader(Loader* loader)
On 2014/06/11 09:12:01, tyoshino wrote:
> how about naming this like onLoaderFinished?

Done.

https://codereview.chromium.org/318393002/diff/120001/Source/modules/servicew...
File Source/modules/serviceworkers/ServiceWorkerGlobalScope.cpp (right):

https://codereview.chromium.org/318393002/diff/120001/Source/modules/servicew...
Source/modules/serviceworkers/ServiceWorkerGlobalScope.cpp:93:
RefPtr<ScriptPromiseResolver> resolver =
ScriptPromiseResolver::create(scriptState);
On 2014/06/11 09:10:49, yhirano wrote:
> [opt] You can use ScriptPromise::reject, if you prefer.

Done.

[tyoshino (SeeGerritForStatus), 2014-06-12 03:27:24]

https://codereview.chromium.org/318393002/diff/100001/Source/modules/servicew...
File Source/modules/serviceworkers/FetchManager.cpp (right):

https://codereview.chromium.org/318393002/diff/100001/Source/modules/servicew...
Source/modules/serviceworkers/FetchManager.cpp:126:
m_resolver->reject(DOMException::create(NetworkError));
On 2014/06/11 11:22:11, horo wrote:
> On 2014/06/11 09:12:01, tyoshino wrote:
> > This should be a TypeError. I also had doubts and asked Anne about this at
> > public-webapps.
> 
> 
> Could you please let me know how to create TypeError here?

snip

How about this?

ScriptState* state = m_resolver->scriptState();
ScriptState::Scope scope(state);
m_resolver->reject(V8ThrowException::createTypeError("Failed",
state->isolate()));

https://codereview.chromium.org/318393002/diff/160001/LayoutTests/http/tests/...
File LayoutTests/http/tests/serviceworker/fetch-event.html (right):

https://codereview.chromium.org/318393002/diff/160001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/fetch-event.html:102:
assert_equals(frame.contentDocument.body.textContent, 'Here\'s a other html
file.\n',
a -> an

[horo, 2014-06-12 05:21:16]

https://codereview.chromium.org/318393002/diff/100001/Source/modules/servicew...
File Source/modules/serviceworkers/FetchManager.cpp (right):

https://codereview.chromium.org/318393002/diff/100001/Source/modules/servicew...
Source/modules/serviceworkers/FetchManager.cpp:126:
m_resolver->reject(DOMException::create(NetworkError));
On 2014/06/12 03:27:24, tyoshino wrote:
> On 2014/06/11 11:22:11, horo wrote:
> > On 2014/06/11 09:12:01, tyoshino wrote:
> > > This should be a TypeError. I also had doubts and asked Anne about this at
> > > public-webapps.
> > 
> > 
> > Could you please let me know how to create TypeError here?
> 
> snip
> 
> How about this?
> 
> ScriptState* state = m_resolver->scriptState();
> ScriptState::Scope scope(state);
> m_resolver->reject(V8ThrowException::createTypeError("Failed",
> state->isolate()));

Done.
Thank you.

https://codereview.chromium.org/318393002/diff/160001/LayoutTests/http/tests/...
File LayoutTests/http/tests/serviceworker/fetch-event.html (right):

https://codereview.chromium.org/318393002/diff/160001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/fetch-event.html:102:
assert_equals(frame.contentDocument.body.textContent, 'Here\'s a other html
file.\n',
On 2014/06/12 03:27:24, tyoshino wrote:
> a -> an

Done.

[tyoshino (SeeGerritForStatus), 2014-06-12 07:38:20]

lgtm

https://codereview.chromium.org/318393002/diff/200001/Source/modules/servicew...
File Source/modules/serviceworkers/FetchManager.cpp (right):

https://codereview.chromium.org/318393002/diff/200001/Source/modules/servicew...
Source/modules/serviceworkers/FetchManager.cpp:8: #include
"bindings/v8/ScriptPromiseResolverWithContext.h"
include bindings/v8/ScriptState.h

https://codereview.chromium.org/318393002/diff/200001/Source/modules/servicew...
Source/modules/serviceworkers/FetchManager.cpp:9: #include
"core/dom/DOMException.h"
remove

https://codereview.chromium.org/318393002/diff/200001/Source/modules/servicew...
Source/modules/serviceworkers/FetchManager.cpp:14: #include
"modules/serviceworkers/Response.h"
include platform/network/ResourceRequest.h

https://codereview.chromium.org/318393002/diff/200001/Source/modules/servicew...
File Source/modules/serviceworkers/ServiceWorkerGlobalScope.cpp (right):

https://codereview.chromium.org/318393002/diff/200001/Source/modules/servicew...
Source/modules/serviceworkers/ServiceWorkerGlobalScope.cpp:34: #include
"FetchPolyfill.h"
include bindings/v8/ScriptState.h, bindings/v8/ScriptPromise.h and
bindings/v8/V8ThrowException.h

https://codereview.chromium.org/318393002/diff/200001/Source/modules/servicew...
Source/modules/serviceworkers/ServiceWorkerGlobalScope.cpp:43: #include
"platform/weborigin/KURL.h"
include platform/network/ResourceRequest.h

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

https://codereview.chromium.org/318393002/diff/200001/Source/modules/servicew...
Source/modules/serviceworkers/ServiceWorkerGlobalScope.h:41: class
ScriptPromise;
add ScriptState

[horo, 2014-06-12 08:32:55]

https://codereview.chromium.org/318393002/diff/200001/Source/modules/servicew...
File Source/modules/serviceworkers/FetchManager.cpp (right):

https://codereview.chromium.org/318393002/diff/200001/Source/modules/servicew...
Source/modules/serviceworkers/FetchManager.cpp:8: #include
"bindings/v8/ScriptPromiseResolverWithContext.h"
On 2014/06/12 07:38:19, tyoshino wrote:
> include bindings/v8/ScriptState.h

Done.

https://codereview.chromium.org/318393002/diff/200001/Source/modules/servicew...
Source/modules/serviceworkers/FetchManager.cpp:9: #include
"core/dom/DOMException.h"
On 2014/06/12 07:38:19, tyoshino wrote:
> remove

Done.

https://codereview.chromium.org/318393002/diff/200001/Source/modules/servicew...
Source/modules/serviceworkers/FetchManager.cpp:14: #include
"modules/serviceworkers/Response.h"
On 2014/06/12 07:38:19, tyoshino wrote:
> include platform/network/ResourceRequest.h

Done.

https://codereview.chromium.org/318393002/diff/200001/Source/modules/servicew...
File Source/modules/serviceworkers/ServiceWorkerGlobalScope.cpp (right):

https://codereview.chromium.org/318393002/diff/200001/Source/modules/servicew...
Source/modules/serviceworkers/ServiceWorkerGlobalScope.cpp:34: #include
"FetchPolyfill.h"
On 2014/06/12 07:38:19, tyoshino wrote:
> include bindings/v8/ScriptState.h, bindings/v8/ScriptPromise.h and
> bindings/v8/V8ThrowException.h

Done.

https://codereview.chromium.org/318393002/diff/200001/Source/modules/servicew...
Source/modules/serviceworkers/ServiceWorkerGlobalScope.cpp:43: #include
"platform/weborigin/KURL.h"
On 2014/06/12 07:38:19, tyoshino wrote:
> include platform/network/ResourceRequest.h

Done.

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

https://codereview.chromium.org/318393002/diff/200001/Source/modules/servicew...
Source/modules/serviceworkers/ServiceWorkerGlobalScope.h:41: class
ScriptPromise;
On 2014/06/12 07:38:20, tyoshino wrote:
> add ScriptState

Done.

[horo, 2014-06-12 09:02:35]

dominicc@
Could you please review this again?
Thank you.

[horo, 2014-06-13 04:22:34]

On 2014/06/10 03:34:52, horo wrote:
> We don't have the way to terminate the service worker in LayoutTests.
> So we should create browser tests which destroys the execution context while
> fetch is happening.
> Is my understanding correct?

This my understanding was incorrect.
We can destroy the execution context by calling self.close().
So I added test for destroying the execution context while fetch is happening in
Patch Set 14.

[horo, 2014-06-16 05:27:25]

dominicc@
Ping?

[dominicc (has gone to gerrit), 2014-06-16 06:07:50]

LGTM

You will need falken for OWNERs I think?

https://codereview.chromium.org/318393002/diff/300001/LayoutTests/http/tests/...
File LayoutTests/http/tests/serviceworker/resources/fetch-worker.js (right):

https://codereview.chromium.org/318393002/diff/300001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/resources/fetch-worker.js:31: })
Terminate the statement with a semicolon.

https://codereview.chromium.org/318393002/diff/300001/Source/core/workers/Wor...
File Source/core/workers/WorkerThread.cpp (right):

https://codereview.chromium.org/318393002/diff/300001/Source/core/workers/Wor...
Source/core/workers/WorkerThread.cpp:209: 
Let's tighten up the whitespace here by deleting 207 and 209.

[horo, 2014-06-16 06:32:18]

According to Chromite Butler, we seems to be able to commit this cl.

https://codereview.chromium.org/318393002/diff/300001/LayoutTests/http/tests/...
File LayoutTests/http/tests/serviceworker/resources/fetch-worker.js (right):

https://codereview.chromium.org/318393002/diff/300001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/resources/fetch-worker.js:31: })
On 2014/06/16 06:07:50, dominicc wrote:
> Terminate the statement with a semicolon.

Done.

https://codereview.chromium.org/318393002/diff/300001/Source/core/workers/Wor...
File Source/core/workers/WorkerThread.cpp (right):

https://codereview.chromium.org/318393002/diff/300001/Source/core/workers/Wor...
Source/core/workers/WorkerThread.cpp:209: 
On 2014/06/16 06:07:50, dominicc wrote:
> Let's tighten up the whitespace here by deleting 207 and 209.

Done.

[horo, 2014-06-16 06:32:25]

The CQ bit was checked by horo@chromium.org

[commit-bot: I haz the power, 2014-06-16 06:32:42]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/horo@chromium.org/318393002/330001

[commit-bot: I haz the power, 2014-06-16 08:22:29]

Change committed as 176186