ServiceWorker: Use mojo's data pipe for respondWith(stream)

ServiceWorker: Use mojo's data pipe for respondWith(stream)

Currently respondWith(fetch(...)) is using Stream provided by BlobRegistry.
BlobRegistry is living on the main thread and any respondWith using the
BlobRegistry like 'respondWith(fetch(...))' or
'respondWith(new Response('body'))' on the service worker thread contain a
thread hop implicitly. This patch is to use mojo's data pipe instead of Stream
to avoid the thread hop.

Proof-of-Concept patch: https://crrev.com/2684533002

BUG=690795
Review-Url: https://codereview.chromium.org/2703343002
Cr-Commit-Position: refs/heads/master@{#465909}
Committed: https://chromium.googlesource.com/chromium/src/+/1ac68cf90fa046d3e9c1418aee7cd14b8ec38210

[shimazu, 2017-02-21 07:36:40]

Description was changed from

==========
ServiceWorker: Use mojo's data pipe for respondWith(stream)

Currently respondWith(stream) is using Stream provided by BlobRegistry.
BlobRegistry is living on the main thread and any respondWith on the service
worker thread contain a thread hop implicitly. This patch is to use mojo's data
pipe instead of Stream to avoid the thread hop.

BUG=690795
==========

to

==========
ServiceWorker: Use mojo's data pipe for respondWith(stream)

Currently respondWith(stream) is using Stream provided by BlobRegistry.
BlobRegistry is living on the main thread and any respondWith on the service
worker thread contain a thread hop implicitly. This patch is to use mojo's data
pipe instead of Stream to avoid the thread hop.

Proof-of-Concept patch: https://crrev.com/2684533002

BUG=690795
==========

[shimazu, 2017-02-21 07:37:00]

Description was changed from

==========
ServiceWorker: Use mojo's data pipe for respondWith(stream)

Currently respondWith(stream) is using Stream provided by BlobRegistry.
BlobRegistry is living on the main thread and any respondWith on the service
worker thread contain a thread hop implicitly. This patch is to use mojo's data
pipe instead of Stream to avoid the thread hop.

Proof-of-Concept patch: https://crrev.com/2684533002

BUG=690795
==========

to

==========
ServiceWorker: Use mojo's data pipe for respondWith(streamResponse)

Currently respondWith(stream) is using Stream provided by BlobRegistry.
BlobRegistry is living on the main thread and any respondWith on the service
worker thread contain a thread hop implicitly. This patch is to use mojo's data
pipe instead of Stream to avoid the thread hop.

Proof-of-Concept patch: https://crrev.com/2684533002

BUG=690795
==========

[shimazu, 2017-02-21 07:37:23]

Description was changed from

==========
ServiceWorker: Use mojo's data pipe for respondWith(streamResponse)

Currently respondWith(stream) is using Stream provided by BlobRegistry.
BlobRegistry is living on the main thread and any respondWith on the service
worker thread contain a thread hop implicitly. This patch is to use mojo's data
pipe instead of Stream to avoid the thread hop.

Proof-of-Concept patch: https://crrev.com/2684533002

BUG=690795
==========

to

==========
ServiceWorker: Use mojo's data pipe for respondWith(stream)

Currently respondWith(stream) is using Stream provided by BlobRegistry.
BlobRegistry is living on the main thread and any respondWith on the service
worker thread contain a thread hop implicitly. This patch is to use mojo's data
pipe instead of Stream to avoid the thread hop.

Proof-of-Concept patch: https://crrev.com/2684533002

BUG=690795
==========

[shimazu, 2017-02-21 07:52:21]

The CQ bit was checked by shimazu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-21 07:52:36]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-21 08:04:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-21 08:04:44]

Dry run: Try jobs failed on following builders:
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[shimazu, 2017-02-21 09:37:24]

The CQ bit was checked by shimazu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-21 09:37:52]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-21 10:05:45]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-21 10:05:46]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[shimazu, 2017-02-22 11:11:17]

The CQ bit was checked by shimazu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-22 11:11:59]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-22 13:05:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-22 13:05:35]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[shimazu, 2017-03-31 02:06:25]

The CQ bit was checked by shimazu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-31 02:06:57]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-31 02:10:39]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-31 02:10:40]

Dry run: Try jobs failed on following builders:
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[shimazu, 2017-04-05 10:28:26]

The CQ bit was checked by shimazu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-05 10:28:38]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-05 10:31:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-05 10:31:05]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[shimazu, 2017-04-06 02:42:12]

The CQ bit was checked by shimazu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-06 02:42:29]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[shimazu, 2017-04-06 03:22:15]

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

[shimazu, 2017-04-06 03:22:16]

ptal, thanks!

https://codereview.chromium.org/2703343002/diff/140001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/serviceworkers/FetchRespondWithObserver.cpp
(right):

https://codereview.chromium.org/2703343002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/serviceworkers/FetchRespondWithObserver.cpp:233:
// MojoCreateDataPipeOptions options;
Do you have insight for these parameters?
I think the default might be better if there is no requirement.

[commit-bot: I haz the power, 2017-04-06 04:33:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-06 04:33:42]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[horo, 2017-04-06 05:22:09]

https://codereview.chromium.org/2703343002/diff/140001/content/browser/BUILD.gn
File content/browser/BUILD.gn (right):

https://codereview.chromium.org/2703343002/diff/140001/content/browser/BUILD....
content/browser/BUILD.gn:1343: "service_worker/service_worker_stream_reader.cc",
You can delete service_worker_stream_reader.*.

https://codereview.chromium.org/2703343002/diff/140001/content/browser/servic...
File content/browser/service_worker/service_worker_data_pipe_reader.cc (right):

https://codereview.chromium.org/2703343002/diff/140001/content/browser/servic...
content/browser/service_worker/service_worker_data_pipe_reader.cc:49: void
ServiceWorkerDataPipeReader::OnHandleGotReadable(MojoResult) {
DCHECK_EQ(State::STREAMING, producer_state_)?

https://codereview.chromium.org/2703343002/diff/140001/content/browser/servic...
content/browser/service_worker/service_worker_data_pipe_reader.cc:78: case
MOJO_RESULT_RESOURCE_EXHAUSTED:
mojo::ReadDataRaw doesn't return MOJO_RESULT_RESOURCE_EXHAUSTED.

https://chromium.googlesource.com/chromium/src/+/dd3c9a967d856c4dc8b41fc1738c...

https://codereview.chromium.org/2703343002/diff/140001/content/browser/servic...
content/browser/service_worker/service_worker_data_pipe_reader.cc:85: }
ASSERT_NOT_REACHED();

https://codereview.chromium.org/2703343002/diff/140001/content/browser/servic...
content/browser/service_worker/service_worker_data_pipe_reader.cc:122: case
MOJO_RESULT_RESOURCE_EXHAUSTED:
mojo::ReadDataRaw doesn't return MOJO_RESULT_RESOURCE_EXHAUSTED.

https://codereview.chromium.org/2703343002/diff/140001/content/browser/servic...
content/browser/service_worker/service_worker_data_pipe_reader.cc:125: return
net::ERR_FAILED;
ASSERT_NOT_REACHED();?

https://codereview.chromium.org/2703343002/diff/140001/content/browser/servic...
File content/browser/service_worker/service_worker_data_pipe_reader.h (right):

https://codereview.chromium.org/2703343002/diff/140001/content/browser/servic...
content/browser/service_worker/service_worker_data_pipe_reader.h:23: class
ServiceWorkerDataPipeReader
Can you have unittests for this class?

https://codereview.chromium.org/2703343002/diff/140001/content/browser/servic...
content/browser/service_worker/service_worker_data_pipe_reader.h:32: void
OnHandleGotReadable(MojoResult);
private?

https://codereview.chromium.org/2703343002/diff/140001/content/browser/servic...
content/browser/service_worker/service_worker_data_pipe_reader.h:33: int
ReadRawData(net::IOBuffer* buf, int buf_size);
Please add comments about these methods.

https://codereview.chromium.org/2703343002/diff/140001/content/browser/servic...
content/browser/service_worker/service_worker_data_pipe_reader.h:43: void
MaybeAbort();
Delete

https://codereview.chromium.org/2703343002/diff/140001/content/browser/servic...
File content/browser/service_worker/service_worker_fetch_dispatcher.cc (right):

https://codereview.chromium.org/2703343002/diff/140001/content/browser/servic...
content/browser/service_worker/service_worker_fetch_dispatcher.cc:301: if
(!version_->FinishRequest(request_id, true, dispatch_event_time))
true /* was_handled */

https://codereview.chromium.org/2703343002/diff/140001/content/browser/servic...
content/browser/service_worker/service_worker_fetch_dispatcher.cc:301: if
(!version_->FinishRequest(request_id, true, dispatch_event_time))
You can use HandleResponse()

https://codereview.chromium.org/2703343002/diff/140001/content/browser/servic...
content/browser/service_worker/service_worker_fetch_dispatcher.cc:345: if
(!version_->FinishRequest(fetch_event_id_, false, dispatch_event_time))
s/false/fetch_result == SERVICE_WORKER_FETCH_EVENT_RESULT_RESPONSE/

https://codereview.chromium.org/2703343002/diff/140001/content/browser/servic...
File content/browser/service_worker/service_worker_url_request_job_unittest.cc
(right):

https://codereview.chromium.org/2703343002/diff/140001/content/browser/servic...
content/browser/service_worker/service_worker_url_request_job_unittest.cc:735:
base::RunLoop().RunUntilIdle();
Why we don't need to call OnCompleted()?

And could you please add tests for OnAborted()?

https://codereview.chromium.org/2703343002/diff/140001/content/common/service...
File content/common/service_worker/service_worker_event_dispatcher.mojom
(right):

https://codereview.chromium.org/2703343002/diff/140001/content/common/service...
content/common/service_worker/service_worker_event_dispatcher.mojom:56: // the
IPC's ordering issue.
Could you please add TODO comment and a link to crbug?

https://codereview.chromium.org/2703343002/diff/140001/content/renderer/servi...
File content/renderer/service_worker/service_worker_context_client.cc (right):

https://codereview.chromium.org/2703343002/diff/140001/content/renderer/servi...
content/renderer/service_worker/service_worker_context_client.cc:907: //
creation or is mojofied.
we should have crbug ID.

https://codereview.chromium.org/2703343002/diff/140001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/serviceworkers/FetchRespondWithObserver.cpp
(right):

https://codereview.chromium.org/2703343002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/serviceworkers/FetchRespondWithObserver.cpp:222:
if (buffer) {
Why don't you handle null buffer case?

https://codereview.chromium.org/2703343002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/serviceworkers/FetchRespondWithObserver.cpp:233:
// MojoCreateDataPipeOptions options;
On 2017/04/06 03:22:16, shimazu wrote:
> Do you have insight for these parameters?
> I think the default might be better if there is no requirement.

SGTM

[horo, 2017-04-06 05:52:40]

Some layout tests are failing.
Please check the try-bot result.

[shimazu, 2017-04-06 06:27:33]

The CQ bit was checked by shimazu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-06 06:33:04]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-06 08:45:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-06 08:45:01]

Dry run: This issue passed the CQ dry run.

[shimazu, 2017-04-06 10:13:26]

The CQ bit was checked by shimazu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-06 10:13:42]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-06 10:43:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-06 10:43:07]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[shimazu, 2017-04-07 00:29:58]

The CQ bit was checked by shimazu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-07 00:30:43]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-07 00:59:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-07 00:59:07]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[shimazu, 2017-04-07 01:32:45]

Patchset #11 (id:200001) has been deleted

[shimazu, 2017-04-07 08:49:50]

The CQ bit was checked by shimazu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-07 08:50:05]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[shimazu, 2017-04-07 08:52:29]

Uploaded ps#11. PTAnL:)

https://codereview.chromium.org/2703343002/diff/140001/content/browser/BUILD.gn
File content/browser/BUILD.gn (right):

https://codereview.chromium.org/2703343002/diff/140001/content/browser/BUILD....
content/browser/BUILD.gn:1343: "service_worker/service_worker_stream_reader.cc",
On 2017/04/06 05:22:08, horo wrote:
> You can delete service_worker_stream_reader.*.

Done.

https://codereview.chromium.org/2703343002/diff/140001/content/browser/servic...
File content/browser/service_worker/service_worker_data_pipe_reader.cc (right):

https://codereview.chromium.org/2703343002/diff/140001/content/browser/servic...
content/browser/service_worker/service_worker_data_pipe_reader.cc:49: void
ServiceWorkerDataPipeReader::OnHandleGotReadable(MojoResult) {
On 2017/04/06 05:22:08, horo wrote:
> DCHECK_EQ(State::STREAMING, producer_state_)?

Done.

https://codereview.chromium.org/2703343002/diff/140001/content/browser/servic...
content/browser/service_worker/service_worker_data_pipe_reader.cc:78: case
MOJO_RESULT_RESOURCE_EXHAUSTED:
On 2017/04/06 05:22:08, horo wrote:
> mojo::ReadDataRaw doesn't return MOJO_RESULT_RESOURCE_EXHAUSTED.
> 
>
https://chromium.googlesource.com/chromium/src/+/dd3c9a967d856c4dc8b41fc1738c...

Done.

https://codereview.chromium.org/2703343002/diff/140001/content/browser/servic...
content/browser/service_worker/service_worker_data_pipe_reader.cc:85: }
On 2017/04/06 05:22:08, horo wrote:
> ASSERT_NOT_REACHED();

Done.

https://codereview.chromium.org/2703343002/diff/140001/content/browser/servic...
content/browser/service_worker/service_worker_data_pipe_reader.cc:122: case
MOJO_RESULT_RESOURCE_EXHAUSTED:
On 2017/04/06 05:22:08, horo wrote:
> mojo::ReadDataRaw doesn't return MOJO_RESULT_RESOURCE_EXHAUSTED.

Done.

https://codereview.chromium.org/2703343002/diff/140001/content/browser/servic...
content/browser/service_worker/service_worker_data_pipe_reader.cc:125: return
net::ERR_FAILED;
On 2017/04/06 05:22:08, horo wrote:
> ASSERT_NOT_REACHED();?

Done.

https://codereview.chromium.org/2703343002/diff/140001/content/browser/servic...
File content/browser/service_worker/service_worker_data_pipe_reader.h (right):

https://codereview.chromium.org/2703343002/diff/140001/content/browser/servic...
content/browser/service_worker/service_worker_data_pipe_reader.h:23: class
ServiceWorkerDataPipeReader
On 2017/04/06 05:22:09, horo wrote:
> Can you have unittests for this class?

Done.

https://codereview.chromium.org/2703343002/diff/140001/content/browser/servic...
content/browser/service_worker/service_worker_data_pipe_reader.h:32: void
OnHandleGotReadable(MojoResult);
On 2017/04/06 05:22:09, horo wrote:
> private?

Done.

https://codereview.chromium.org/2703343002/diff/140001/content/browser/servic...
content/browser/service_worker/service_worker_data_pipe_reader.h:33: int
ReadRawData(net::IOBuffer* buf, int buf_size);
On 2017/04/06 05:22:09, horo wrote:
> Please add comments about these methods.

Done.

https://codereview.chromium.org/2703343002/diff/140001/content/browser/servic...
content/browser/service_worker/service_worker_data_pipe_reader.h:43: void
MaybeAbort();
On 2017/04/06 05:22:09, horo wrote:
> Delete

Done.

https://codereview.chromium.org/2703343002/diff/140001/content/browser/servic...
File content/browser/service_worker/service_worker_fetch_dispatcher.cc (right):

https://codereview.chromium.org/2703343002/diff/140001/content/browser/servic...
content/browser/service_worker/service_worker_fetch_dispatcher.cc:301: if
(!version_->FinishRequest(request_id, true, dispatch_event_time))
On 2017/04/06 05:22:09, horo wrote:
> You can use HandleResponse()

Done.

https://codereview.chromium.org/2703343002/diff/140001/content/browser/servic...
content/browser/service_worker/service_worker_fetch_dispatcher.cc:345: if
(!version_->FinishRequest(fetch_event_id_, false, dispatch_event_time))
On 2017/04/06 05:22:09, horo wrote:
> s/false/fetch_result == SERVICE_WORKER_FETCH_EVENT_RESULT_RESPONSE/

Done.

https://codereview.chromium.org/2703343002/diff/140001/content/browser/servic...
File content/browser/service_worker/service_worker_url_request_job_unittest.cc
(right):

https://codereview.chromium.org/2703343002/diff/140001/content/browser/servic...
content/browser/service_worker/service_worker_url_request_job_unittest.cc:735:
base::RunLoop().RunUntilIdle();
On 2017/04/06 05:22:09, horo wrote:
> Why we don't need to call OnCompleted()?
> 
> And could you please add tests for OnAborted()?

Oops, I forgot to add it.
Added OnCompleted and added the tests.

https://codereview.chromium.org/2703343002/diff/140001/content/common/service...
File content/common/service_worker/service_worker_event_dispatcher.mojom
(right):

https://codereview.chromium.org/2703343002/diff/140001/content/common/service...
content/common/service_worker/service_worker_event_dispatcher.mojom:56: // the
IPC's ordering issue.
On 2017/04/06 05:22:09, horo wrote:
> Could you please add TODO comment and a link to crbug?

Done.

https://codereview.chromium.org/2703343002/diff/140001/content/renderer/servi...
File content/renderer/service_worker/service_worker_context_client.cc (right):

https://codereview.chromium.org/2703343002/diff/140001/content/renderer/servi...
content/renderer/service_worker/service_worker_context_client.cc:907: //
creation or is mojofied.
On 2017/04/06 05:22:09, horo wrote:
> we should have crbug ID.

Done.

https://codereview.chromium.org/2703343002/diff/140001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/serviceworkers/FetchRespondWithObserver.cpp
(right):

https://codereview.chromium.org/2703343002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/serviceworkers/FetchRespondWithObserver.cpp:222:
if (buffer) {
On 2017/04/06 05:22:09, horo wrote:
> Why don't you handle null buffer case?

Ohh, thanks. I missed that..
Updated.

https://codereview.chromium.org/2703343002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/serviceworkers/FetchRespondWithObserver.cpp:233:
// MojoCreateDataPipeOptions options;
On 2017/04/06 05:22:09, horo wrote:
> On 2017/04/06 03:22:16, shimazu wrote:
> > Do you have insight for these parameters?
> > I think the default might be better if there is no requirement.
> 
> SGTM

Done.

[commit-bot: I haz the power, 2017-04-07 10:24:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-07 10:25:00]

Dry run: This issue passed the CQ dry run.

[shimazu, 2017-04-10 00:43:39]

The CQ bit was checked by shimazu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-10 00:43:50]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[shimazu, 2017-04-10 00:45:39]

Uploaded ps#11 for additional tests. PTAnL:)

[commit-bot: I haz the power, 2017-04-10 00:46:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-10 00:46:22]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[falken, 2017-04-10 01:21:56]

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

[falken, 2017-04-10 01:21:57]

Drive-by: I just looked at the mojom files mostly.

nit in CL description: "any respondWith on the service
worker thread contain a thread hop implicitly"

respondWith(cache.match(..)) and respondWith(preloadResponse) don't do a thread
hop, right? The common time you respond with a stream is respondWith(fetch(...))

https://codereview.chromium.org/2703343002/diff/240001/content/common/service...
File content/common/service_worker/service_worker_event_dispatcher.mojom
(right):

https://codereview.chromium.org/2703343002/diff/240001/content/common/service...
content/common/service_worker/service_worker_event_dispatcher.mojom:54: //
Browser-side interface which is passed through DispatchFetchEvent.
In addition to "browser-side interface", the comment could explain what the
interface is for. "The renderer uses this interface to respond to a fetch
event."

https://codereview.chromium.org/2703343002/diff/240001/content/common/service...
content/common/service_worker/service_worker_event_dispatcher.mojom:56: // the
IPC's ordering issue.
It's a little hard to tell in which cases this interface is used vs in which
cases the legacy IPC is used. Suggest:
"This interface is only used for responses where the body is a stream. The
legacy IPC is used instead when..."

"The IPC's ordering issue" is a bit vague. 

Are the only response types blob and stream? What happens in
respondWith(preloadResponse)?

https://codereview.chromium.org/2703343002/diff/240001/third_party/WebKit/pub...
File
third_party/WebKit/public/platform/modules/serviceworker/service_worker_stream_handle.mojom
(right):

https://codereview.chromium.org/2703343002/diff/240001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/serviceworker/service_worker_stream_handle.mojom:7:
// Notifies the end of streaming respondWith.
"The renderer uses this interface to tell the browser when stream response
passed to respondWith() ended"

Or the other way around?

[shimazu, 2017-04-10 02:37:07]

The CQ bit was checked by shimazu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-10 02:37:14]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-10 04:28:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-10 04:28:39]

Dry run: This issue passed the CQ dry run.

[shimazu, 2017-04-10 05:36:22]

Description was changed from

==========
ServiceWorker: Use mojo's data pipe for respondWith(stream)

Currently respondWith(stream) is using Stream provided by BlobRegistry.
BlobRegistry is living on the main thread and any respondWith on the service
worker thread contain a thread hop implicitly. This patch is to use mojo's data
pipe instead of Stream to avoid the thread hop.

Proof-of-Concept patch: https://crrev.com/2684533002

BUG=690795
==========

to

==========
ServiceWorker: Use mojo's data pipe for respondWith(stream)

Currently respondWith(fetch(...)) is using Stream provided by BlobRegistry.
BlobRegistry is living on the main thread and any respondWith using the
BlobRegistry like 'respondWith(fetch(...))' or
'respondWith(new Response('body'))' on the service worker thread contain a
thread hop implicitly. This patch is to use mojo's data pipe instead of Stream
to avoid the thread hop.

Proof-of-Concept patch: https://crrev.com/2684533002

BUG=690795
==========

[horo, 2017-04-10 09:24:38]

https://codereview.chromium.org/2703343002/diff/140001/content/browser/BUILD.gn
File content/browser/BUILD.gn (right):

https://codereview.chromium.org/2703343002/diff/140001/content/browser/BUILD....
content/browser/BUILD.gn:1343: "service_worker/service_worker_stream_reader.cc",
On 2017/04/07 08:52:29, shimazu wrote:
> On 2017/04/06 05:22:08, horo wrote:
> > You can delete service_worker_stream_reader.*.
> 
> Done.

Please delete these files.

https://codereview.chromium.org/2703343002/diff/280001/content/browser/servic...
File content/browser/service_worker/service_worker_data_pipe_reader.cc (right):

https://codereview.chromium.org/2703343002/diff/280001/content/browser/servic...
content/browser/service_worker/service_worker_data_pipe_reader.cc:32: if
(streaming_version_) {
You don't need this if check.
streaming_version_ must not be null.

https://codereview.chromium.org/2703343002/diff/280001/content/browser/servic...
content/browser/service_worker/service_worker_data_pipe_reader.cc:59: //
COMPLETE/ABORT has been already notified.
OnComplete/OnAbort has already been called.

https://codereview.chromium.org/2703343002/diff/280001/content/browser/servic...
content/browser/service_worker/service_worker_data_pipe_reader.cc:105: //
COMPLETE/ABORT has been already notified.
OnComplete/OnAbort has already been called.

https://codereview.chromium.org/2703343002/diff/280001/content/browser/servic...
content/browser/service_worker/service_worker_data_pipe_reader.cc:184: return 0;
return net::OK;

https://codereview.chromium.org/2703343002/diff/280001/content/browser/servic...
File content/browser/service_worker/service_worker_data_pipe_reader.h (right):

https://codereview.chromium.org/2703343002/diff/280001/content/browser/servic...
content/browser/service_worker/service_worker_data_pipe_reader.h:36: // zero
when all data have received, and net::ERR_* when failure or pending
zero (net::OK)

https://codereview.chromium.org/2703343002/diff/280001/content/browser/servic...
content/browser/service_worker/service_worker_data_pipe_reader.h:69: };
DISALLOW_COPY_AND_ASSIGN

https://codereview.chromium.org/2703343002/diff/280001/content/browser/servic...
File content/browser/service_worker/service_worker_data_pipe_reader_unittest.cc
(right):

https://codereview.chromium.org/2703343002/diff/280001/content/browser/servic...
content/browser/service_worker/service_worker_data_pipe_reader_unittest.cc:6:
#include "base/run_loop.h"
one blank line.

https://codereview.chromium.org/2703343002/diff/280001/content/browser/servic...
content/browser/service_worker/service_worker_data_pipe_reader_unittest.cc:65:
};
DISALLOW_COPY_AND_ASSIGN

https://codereview.chromium.org/2703343002/diff/280001/content/browser/servic...
content/browser/service_worker/service_worker_data_pipe_reader_unittest.cc:122:
// ServiceWorkerContextCore* context() const { return helper_->context(); }
delete

https://codereview.chromium.org/2703343002/diff/280001/content/browser/servic...
content/browser/service_worker/service_worker_data_pipe_reader_unittest.cc:123:
// ServiceWorkerStorage* storage() const { return context()->storage(); }
delete

https://codereview.chromium.org/2703343002/diff/280001/content/browser/servic...
content/browser/service_worker/service_worker_data_pipe_reader_unittest.cc:134:
TEST_F(ServiceWorkerDataPipeReaderTest, SyncRead) {
Can you have 0 byte body case?

https://codereview.chromium.org/2703343002/diff/280001/content/renderer/servi...
File content/renderer/service_worker/service_worker_context_client.cc (right):

https://codereview.chromium.org/2703343002/diff/280001/content/renderer/servi...
content/renderer/service_worker/service_worker_context_client.cc:708: // Aborts
all the pending events callbacks.
Is it OK not to abort all fetch_response_callbacks?

https://codereview.chromium.org/2703343002/diff/280001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/serviceworkers/FetchRespondWithObserver.cpp
(right):

https://codereview.chromium.org/2703343002/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/serviceworkers/FetchRespondWithObserver.cpp:252:
->RespondToFetchEventWithResponseStream(event_id_, web_response,
It is safe to call StartLoading() before
RespondToFetchEventWithResponseStream(), because
FetchDataLoaderAsDataPipe::Start() starts loading asynchronously.

But I think calling RespondToFetchEventWithResponseStream() before
StartLoading() looks more clear.
WDYT?

https://codereview.chromium.org/2703343002/diff/280001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerGlobalScopeClient.h
(right):

https://codereview.chromium.org/2703343002/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerGlobalScopeClient.h:102:
virtual void RespondToFetchEvent(int fetchEventID,
Keep the New Blink Style.


Please check other places.

[shimazu, 2017-04-11 01:21:20]

The CQ bit was checked by shimazu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-11 01:22:08]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[shimazu, 2017-04-11 01:32:58]

Thanks for your comments!
Uploaded ps#15. PTAnL.

https://codereview.chromium.org/2703343002/diff/140001/content/browser/BUILD.gn
File content/browser/BUILD.gn (right):

https://codereview.chromium.org/2703343002/diff/140001/content/browser/BUILD....
content/browser/BUILD.gn:1343: "service_worker/service_worker_stream_reader.cc",
On 2017/04/10 09:24:37, horo wrote:
> On 2017/04/07 08:52:29, shimazu wrote:
> > On 2017/04/06 05:22:08, horo wrote:
> > > You can delete service_worker_stream_reader.*.
> > 
> > Done.
> 
> Please delete these files.

Done.

https://codereview.chromium.org/2703343002/diff/240001/content/common/service...
File content/common/service_worker/service_worker_event_dispatcher.mojom
(right):

https://codereview.chromium.org/2703343002/diff/240001/content/common/service...
content/common/service_worker/service_worker_event_dispatcher.mojom:54: //
Browser-side interface which is passed through DispatchFetchEvent.
On 2017/04/10 01:21:57, falken wrote:
> In addition to "browser-side interface", the comment could explain what the
> interface is for. "The renderer uses this interface to respond to a fetch
> event."

Done.

https://codereview.chromium.org/2703343002/diff/240001/content/common/service...
content/common/service_worker/service_worker_event_dispatcher.mojom:56: // the
IPC's ordering issue.
On 2017/04/10 01:21:57, falken wrote:
> It's a little hard to tell in which cases this interface is used vs in which
> cases the legacy IPC is used. Suggest:
> "This interface is only used for responses where the body is a stream. The
> legacy IPC is used instead when..."
> 
> "The IPC's ordering issue" is a bit vague. 
> 
> Are the only response types blob and stream? What happens in
> respondWith(preloadResponse)?

Done.

https://codereview.chromium.org/2703343002/diff/240001/third_party/WebKit/pub...
File
third_party/WebKit/public/platform/modules/serviceworker/service_worker_stream_handle.mojom
(right):

https://codereview.chromium.org/2703343002/diff/240001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/serviceworker/service_worker_stream_handle.mojom:7:
// Notifies the end of streaming respondWith.
On 2017/04/10 01:21:57, falken wrote:
> "The renderer uses this interface to tell the browser when stream response
> passed to respondWith() ended"
> 
> Or the other way around?

It sounds good. Thanks!

https://codereview.chromium.org/2703343002/diff/280001/content/browser/servic...
File content/browser/service_worker/service_worker_data_pipe_reader.cc (right):

https://codereview.chromium.org/2703343002/diff/280001/content/browser/servic...
content/browser/service_worker/service_worker_data_pipe_reader.cc:32: if
(streaming_version_) {
On 2017/04/10 09:24:37, horo wrote:
> You don't need this if check.
> streaming_version_ must not be null.

Done.

https://codereview.chromium.org/2703343002/diff/280001/content/browser/servic...
content/browser/service_worker/service_worker_data_pipe_reader.cc:59: //
COMPLETE/ABORT has been already notified.
On 2017/04/10 09:24:37, horo wrote:
> OnComplete/OnAbort has already been called.

Done.

https://codereview.chromium.org/2703343002/diff/280001/content/browser/servic...
content/browser/service_worker/service_worker_data_pipe_reader.cc:105: //
COMPLETE/ABORT has been already notified.
On 2017/04/10 09:24:37, horo wrote:
> OnComplete/OnAbort has already been called.

Done.

https://codereview.chromium.org/2703343002/diff/280001/content/browser/servic...
content/browser/service_worker/service_worker_data_pipe_reader.cc:184: return 0;
On 2017/04/10 09:24:37, horo wrote:
> return net::OK;

Done.

https://codereview.chromium.org/2703343002/diff/280001/content/browser/servic...
File content/browser/service_worker/service_worker_data_pipe_reader.h (right):

https://codereview.chromium.org/2703343002/diff/280001/content/browser/servic...
content/browser/service_worker/service_worker_data_pipe_reader.h:36: // zero
when all data have received, and net::ERR_* when failure or pending
On 2017/04/10 09:24:37, horo wrote:
> zero (net::OK)

Done.

https://codereview.chromium.org/2703343002/diff/280001/content/browser/servic...
content/browser/service_worker/service_worker_data_pipe_reader.h:69: };
On 2017/04/10 09:24:37, horo wrote:
> DISALLOW_COPY_AND_ASSIGN

Done.

https://codereview.chromium.org/2703343002/diff/280001/content/browser/servic...
File content/browser/service_worker/service_worker_data_pipe_reader_unittest.cc
(right):

https://codereview.chromium.org/2703343002/diff/280001/content/browser/servic...
content/browser/service_worker/service_worker_data_pipe_reader_unittest.cc:6:
#include "base/run_loop.h"
On 2017/04/10 09:24:37, horo wrote:
> one blank line.

Done.

https://codereview.chromium.org/2703343002/diff/280001/content/browser/servic...
content/browser/service_worker/service_worker_data_pipe_reader_unittest.cc:122:
// ServiceWorkerContextCore* context() const { return helper_->context(); }
On 2017/04/10 09:24:37, horo wrote:
> delete

Done.

https://codereview.chromium.org/2703343002/diff/280001/content/browser/servic...
content/browser/service_worker/service_worker_data_pipe_reader_unittest.cc:123:
// ServiceWorkerStorage* storage() const { return context()->storage(); }
On 2017/04/10 09:24:37, horo wrote:
> delete

Done.

https://codereview.chromium.org/2703343002/diff/280001/content/browser/servic...
content/browser/service_worker/service_worker_data_pipe_reader_unittest.cc:134:
TEST_F(ServiceWorkerDataPipeReaderTest, SyncRead) {
On 2017/04/10 09:24:37, horo wrote:
> Can you have 0 byte body case?

Done.

https://codereview.chromium.org/2703343002/diff/280001/content/renderer/servi...
File content/renderer/service_worker/service_worker_context_client.cc (right):

https://codereview.chromium.org/2703343002/diff/280001/content/renderer/servi...
content/renderer/service_worker/service_worker_context_client.cc:708: // Aborts
all the pending events callbacks.
On 2017/04/10 09:24:38, horo wrote:
> Is it OK not to abort all fetch_response_callbacks?

Good point. There is no behavior change in my understanding, but we should know
clearly what will happen if it's aborted.
Currently no fetch response seems triggered when it's aborted, but it might have
to be handled a kind of error like a network error.
WDYT?

https://codereview.chromium.org/2703343002/diff/280001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/serviceworkers/FetchRespondWithObserver.cpp
(right):

https://codereview.chromium.org/2703343002/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/serviceworkers/FetchRespondWithObserver.cpp:252:
->RespondToFetchEventWithResponseStream(event_id_, web_response,
On 2017/04/10 09:24:38, horo wrote:
> It is safe to call StartLoading() before
> RespondToFetchEventWithResponseStream(), because
> FetchDataLoaderAsDataPipe::Start() starts loading asynchronously.
> 
> But I think calling RespondToFetchEventWithResponseStream() before
> StartLoading() looks more clear.
> WDYT?

It makes sense. Changed.

https://codereview.chromium.org/2703343002/diff/280001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerGlobalScopeClient.h
(right):

https://codereview.chromium.org/2703343002/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerGlobalScopeClient.h:102:
virtual void RespondToFetchEvent(int fetchEventID,
On 2017/04/10 09:24:38, horo wrote:
> Keep the New Blink Style.
> 
> 
> Please check other places.

Done.

[commit-bot: I haz the power, 2017-04-11 03:16:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-11 03:16:49]

Dry run: This issue passed the CQ dry run.

[horo, 2017-04-11 05:41:53]

https://codereview.chromium.org/2703343002/diff/280001/content/renderer/servi...
File content/renderer/service_worker/service_worker_context_client.cc (right):

https://codereview.chromium.org/2703343002/diff/280001/content/renderer/servi...
content/renderer/service_worker/service_worker_context_client.cc:708: // Aborts
all the pending events callbacks.
On 2017/04/11 01:32:58, shimazu wrote:
> On 2017/04/10 09:24:38, horo wrote:
> > Is it OK not to abort all fetch_response_callbacks?
> 
> Good point. There is no behavior change in my understanding, but we should
know
> clearly what will happen if it's aborted.
> Currently no fetch response seems triggered when it's aborted, but it might
have
> to be handled a kind of error like a network error.
> WDYT?

Even if we don't call all fetch_response_callback,
ServiceWorkerVersion::OnStoppedInternal() aborts all pending request.
So it is OK for the pending requests in fetch_response_callbacks.

https://chromium.googlesource.com/chromium/src/+/25f662809b16e810def434f02226...

But after the worker started streaming, the current code can't handle the
abortion correctly.
I think if you change FetchDataLoaderAsDataPipe::OnStateChange() to call
DidFetchDataLoadedDataPipe() before Cancel(),
you can treat the MOJO_HANDLE_SIGNAL_PEER_CLOSED signal before OnCompleted() as
an error in ServiceWorkerDataPipeReader.

https://codereview.chromium.org/2703343002/diff/300001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/serviceworkers/FetchRespondWithObserver.cpp
(right):

https://codereview.chromium.org/2703343002/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/serviceworkers/FetchRespondWithObserver.cpp:246:
WebServiceWorkerStreamHandle* stream_handle_rawptr = stream_handle.get();
You don't need this.

[shimazu, 2017-04-12 03:44:50]

The CQ bit was checked by shimazu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-12 03:45:08]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[shimazu, 2017-04-12 03:45:57]

The CQ bit was unchecked by shimazu@chromium.org

[shimazu, 2017-04-12 03:48:27]

The CQ bit was checked by shimazu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-12 03:48:47]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-12 03:51:23]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-12 03:51:24]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[shimazu, 2017-04-12 05:49:31]

The CQ bit was checked by shimazu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-12 05:49:48]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[shimazu, 2017-04-12 05:53:35]

Thanks for your comment!
What is really wrong was that the destruction of FetchLoaderClient wasn't
notified to StreamHandleListener in ServiceWorkerContextClient which actually
has the stream_callback. I've fixed that bug in ps#16.

I also added the test for checking if reading the response 's body fails
successfully. (and I've realized that, when the worker is stopped, SWVersion
cancels the pending request so the test passes without the change in ps#16.)

PTAnL.

https://codereview.chromium.org/2703343002/diff/300001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/serviceworkers/FetchRespondWithObserver.cpp
(right):

https://codereview.chromium.org/2703343002/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/serviceworkers/FetchRespondWithObserver.cpp:246:
WebServiceWorkerStreamHandle* stream_handle_rawptr = stream_handle.get();
On 2017/04/11 05:41:53, horo wrote:
> You don't need this.

Thanks, you're right. Done.

[horo, 2017-04-12 06:21:59]

lgtm with a nit.

Thank you!

https://codereview.chromium.org/2703343002/diff/380001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/http/tests/serviceworker/chromium/stop-worker-during-respond-with.html
(right):

https://codereview.chromium.org/2703343002/diff/380001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/serviceworker/chromium/stop-worker-during-respond-with.html:9:
var SCOPE = 'resources/';
nit:
I prefer narrow scope to avoid interactions between tests.

Please use "var SCOPE = 'resources/slow-fetch-and-stop-worker-iframe.html';" and
"with_iframe(SCOPE, ...)".

[commit-bot: I haz the power, 2017-04-12 06:25:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-12 06:25:29]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[shimazu, 2017-04-12 07:22:00]

shimazu@chromium.org changed reviewers:
+ haraken@chromium.org, jsbell@chromium.org, kinuko@chromium.org,
tsepez@chromium.org

[shimazu, 2017-04-12 07:22:01]

kinuko@chromium.org: Please review changes in content/browser/BUILD.gn and DEPS,
and could you roughly scan?

haraken@chromium.org: Please review changes in WebKit/Source/platform and
WebKit/Source/web.

jsbell@chromium.org: Please review changes in
content/{browser,renderer}/cache_storage.

tsepez@chromium.org: Please review changes in IPC related things (.mojom,
.tyepmap and messages.h).

[falken, 2017-04-12 08:00:17]

https://codereview.chromium.org/2703343002/diff/400001/content/common/service...
File content/common/service_worker/service_worker_event_dispatcher.mojom
(right):

https://codereview.chromium.org/2703343002/diff/400001/content/common/service...
content/common/service_worker/service_worker_event_dispatcher.mojom:56: // only
used for responses where the body is a stream or doesn't exist.
nit: Add "when the body is a blob, the legacy IPC blahblabhlah is used"

https://codereview.chromium.org/2703343002/diff/400001/content/common/service...
content/common/service_worker/service_worker_event_dispatcher.mojom:59:
interface ServiceWorkerFetchResponseCallback {
// Responds to the request with |response|. The body is empty.

https://codereview.chromium.org/2703343002/diff/400001/content/common/service...
content/common/service_worker/service_worker_event_dispatcher.mojom:61:
mojo.common.mojom.Time dispatch_event_time);
// Responds to the request with |response|. The body is returned as a stream.

https://codereview.chromium.org/2703343002/diff/400001/content/common/service...
content/common/service_worker/service_worker_event_dispatcher.mojom:63:
blink.mojom.ServiceWorkerStreamHandle stream_handle,
Is it easier to understand if "stream_handle" is renamed "body" or maybe
"body_as_stream"?

https://codereview.chromium.org/2703343002/diff/400001/content/common/service...
content/common/service_worker/service_worker_event_dispatcher.mojom:64:
mojo.common.mojom.Time dispatch_event_time);
// Provides no response to the request. The browser will fall back to the
network.

https://codereview.chromium.org/2703343002/diff/400001/content/common/service...
content/common/service_worker/service_worker_event_dispatcher.mojom:93: //
browser process.
Can you add a comment explaining why this takes both
ServiceWorkerFetchResponseCallback in addition to the mojo callback?

IIRC this has to do this the waitUntil/respondWith duality.

Maybe: The callback is called once the event finishes, which means the event
handler ran and all outstanding respondWith() and waitUntil() promises have
settled. |response_callback| is called once the promise to respondWith()
settles, or when the event handler ran without calling respondWith().

https://codereview.chromium.org/2703343002/diff/400001/content/common/service...
File content/common/service_worker/service_worker_types.h (right):

https://codereview.chromium.org/2703343002/diff/400001/content/common/service...
content/common/service_worker/service_worker_types.h:179:
std::unique_ptr<ServiceWorkerHeaderMap> headers,
Can you put a comment/todo here about the blobuuid and blobsize is only used for
responses where the body is a blob. For other types of responses, the body is
provided separately in the mojo IPC via ServiceWorkerFetchResponseCallback.

https://codereview.chromium.org/2703343002/diff/400001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/fetch/FetchDataLoader.cpp (right):

https://codereview.chromium.org/2703343002/diff/400001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/FetchDataLoader.cpp:349:
mojo::SimpleWatcher datapipe_watcher_;
nit: since the CamelCase version would be DataPipe, the snake_case versions
should be data_pipe, not datapipe.

https://codereview.chromium.org/2703343002/diff/400001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/serviceworkers/FetchRespondWithObserver.cpp
(right):

https://codereview.chromium.org/2703343002/diff/400001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/serviceworkers/FetchRespondWithObserver.cpp:238:
} else {
else isn't needed after the if() { return }

https://codereview.chromium.org/2703343002/diff/400001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerGlobalScopeClient.h
(right):

https://codereview.chromium.org/2703343002/diff/400001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerGlobalScopeClient.h:101:
// provided by the service worker in the fetch events, so fallback to native.
This comment is redundant with the interface. A better name should also obviate
this comment.

https://codereview.chromium.org/2703343002/diff/400001/third_party/WebKit/pub...
File
third_party/WebKit/public/platform/modules/serviceworker/service_worker_stream_handle.mojom
(right):

https://codereview.chromium.org/2703343002/diff/400001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/serviceworker/service_worker_stream_handle.mojom:7:
// The renderer uses this interface to tell the browser when stream response
nit: "when the"

https://codereview.chromium.org/2703343002/diff/400001/third_party/WebKit/pub...
File
third_party/WebKit/public/web/modules/serviceworker/WebServiceWorkerContextClient.h
(right):

https://codereview.chromium.org/2703343002/diff/400001/third_party/WebKit/pub...
third_party/WebKit/public/web/modules/serviceworker/WebServiceWorkerContextClient.h:171:
// dispatchFetchEvent respectively.
RespondToFetchEvent and ResponseToFetchEventWithResponse are tricky names.

suggest....

// Used when respondWith() is not called. Tells the browser to fall back to
native fetch.
RespondToFetchEventWithNoResponse()
// Responds to the fetch event with |response|.
RespondToFetchEvent()
// Responds to the fetch event with |response|, where body is
|web_stream_handle|.
RespondToFetchEventWithStreamBody()

I think |body_as_stream| or |body| is easier to understand than
|web_stream_handle|.

[Tom Sepez, 2017-04-12 16:15:39]

mojom lgtm

[kinuko, 2017-04-13 02:22:28]

https://codereview.chromium.org/2703343002/diff/400001/content/browser/servic...
File content/browser/service_worker/service_worker_data_pipe_reader.h (right):

https://codereview.chromium.org/2703343002/diff/400001/content/browser/servic...
content/browser/service_worker/service_worker_data_pipe_reader.h:44: enum class
State { STREAMING, COMPLETED, ABORTED };
state() getter is private, does this enum need to be public?

https://codereview.chromium.org/2703343002/diff/400001/third_party/WebKit/pub...
File
third_party/WebKit/public/platform/modules/serviceworker/WebServiceWorkerStreamHandle.h
(right):

https://codereview.chromium.org/2703343002/diff/400001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/serviceworker/WebServiceWorkerStreamHandle.h:15:
class BLINK_PLATFORM_EXPORT WebServiceWorkerStreamHandle {
I'm probably missing something but do we need this wrapper? Would it not be
possible to directly use ServiceWorkerStreamHandlePtr?

[haraken, 2017-04-13 05:03:44]

https://codereview.chromium.org/2703343002/diff/400001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/serviceworkers/FetchRespondWithObserver.cpp
(right):

https://codereview.chromium.org/2703343002/diff/400001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/serviceworkers/FetchRespondWithObserver.cpp:116:
class FetchLoaderClient final

Would it be possible to remove this wrapper class (e.g., by making
WebServiceWorkerStreamHandle inherit from FetchDataLoader::Client)?

https://codereview.chromium.org/2703343002/diff/400001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/serviceworkers/FetchRespondWithObserver.cpp:123:
FetchLoaderClient(std::unique_ptr<WebServiceWorkerStreamHandle> handle)

Add explicit.

https://codereview.chromium.org/2703343002/diff/400001/third_party/WebKit/pub...
File
third_party/WebKit/public/platform/modules/serviceworker/WebServiceWorkerStreamHandle.h
(right):

https://codereview.chromium.org/2703343002/diff/400001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/serviceworker/WebServiceWorkerStreamHandle.h:15:
class BLINK_PLATFORM_EXPORT WebServiceWorkerStreamHandle {
On 2017/04/13 02:22:27, kinuko wrote:
> I'm probably missing something but do we need this wrapper? Would it not be
> possible to directly use ServiceWorkerStreamHandlePtr?

Yeah, also I think it's worth thinking about removing FetchLoaderClient.
FetchLoaderClient is just redirecting methods to WebServiceWorkerStreamHandle.

https://codereview.chromium.org/2703343002/diff/400001/third_party/WebKit/pub...
File
third_party/WebKit/public/platform/modules/serviceworker/service_worker_stream_handle.mojom
(right):

https://codereview.chromium.org/2703343002/diff/400001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/serviceworker/service_worker_stream_handle.mojom:7:
// The renderer uses this interface to tell the browser when stream response
On 2017/04/12 08:00:16, falken wrote:
> nit: "when the"

...to notify the browser that the stream response...

[haraken, 2017-04-13 05:05:14]

https://codereview.chromium.org/2703343002/diff/400001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/serviceworkers/FetchRespondWithObserver.cpp
(right):

https://codereview.chromium.org/2703343002/diff/400001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/serviceworkers/FetchRespondWithObserver.cpp:116:
class FetchLoaderClient final
On 2017/04/13 05:03:44, haraken wrote:
> 
> Would it be possible to remove this wrapper class (e.g., by making
> WebServiceWorkerStreamHandle inherit from FetchDataLoader::Client)?

Correction: It would be nicer if we can remove both FetchLoaderClient and
WebServiceWorkerStreamHandle. As kinuko-san said, it's interesting to consider
using ServiceWorkerStreamHandlePtr directly.

[shimazu, 2017-04-17 07:35:29]

The CQ bit was checked by shimazu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-17 07:35:46]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-17 09:23:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-17 09:23:48]

Dry run: This issue passed the CQ dry run.

[shimazu, 2017-04-18 01:50:05]

Thanks for your comments!
Uploaded ps#22. PTAnL.

https://codereview.chromium.org/2703343002/diff/380001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/http/tests/serviceworker/chromium/stop-worker-during-respond-with.html
(right):

https://codereview.chromium.org/2703343002/diff/380001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/serviceworker/chromium/stop-worker-during-respond-with.html:9:
var SCOPE = 'resources/';
On 2017/04/12 06:21:58, horo wrote:
> nit:
> I prefer narrow scope to avoid interactions between tests.
> 
> Please use "var SCOPE = 'resources/slow-fetch-and-stop-worker-iframe.html';"
and
> "with_iframe(SCOPE, ...)".

Done.

https://codereview.chromium.org/2703343002/diff/400001/content/browser/servic...
File content/browser/service_worker/service_worker_data_pipe_reader.h (right):

https://codereview.chromium.org/2703343002/diff/400001/content/browser/servic...
content/browser/service_worker/service_worker_data_pipe_reader.h:44: enum class
State { STREAMING, COMPLETED, ABORTED };
On 2017/04/13 02:22:27, kinuko wrote:
> state() getter is private, does this enum need to be public?

Oops, it seems become public at some point by mistake. Done.

https://codereview.chromium.org/2703343002/diff/400001/content/common/service...
File content/common/service_worker/service_worker_event_dispatcher.mojom
(right):

https://codereview.chromium.org/2703343002/diff/400001/content/common/service...
content/common/service_worker/service_worker_event_dispatcher.mojom:56: // only
used for responses where the body is a stream or doesn't exist.
On 2017/04/12 08:00:16, falken wrote:
> nit: Add "when the body is a blob, the legacy IPC blahblabhlah is used"

Done.

https://codereview.chromium.org/2703343002/diff/400001/content/common/service...
content/common/service_worker/service_worker_event_dispatcher.mojom:59:
interface ServiceWorkerFetchResponseCallback {
On 2017/04/12 08:00:16, falken wrote:
> // Responds to the request with |response|. The body is empty.

Done.

https://codereview.chromium.org/2703343002/diff/400001/content/common/service...
content/common/service_worker/service_worker_event_dispatcher.mojom:61:
mojo.common.mojom.Time dispatch_event_time);
On 2017/04/12 08:00:16, falken wrote:
> // Responds to the request with |response|. The body is returned as a stream.

Done.

https://codereview.chromium.org/2703343002/diff/400001/content/common/service...
content/common/service_worker/service_worker_event_dispatcher.mojom:63:
blink.mojom.ServiceWorkerStreamHandle stream_handle,
On 2017/04/12 08:00:16, falken wrote:
> Is it easier to understand if "stream_handle" is renamed "body" or maybe
> "body_as_stream"?

Done.

https://codereview.chromium.org/2703343002/diff/400001/content/common/service...
content/common/service_worker/service_worker_event_dispatcher.mojom:64:
mojo.common.mojom.Time dispatch_event_time);
On 2017/04/12 08:00:16, falken wrote:
> // Provides no response to the request. The browser will fall back to the
> network.

Done.

https://codereview.chromium.org/2703343002/diff/400001/content/common/service...
content/common/service_worker/service_worker_event_dispatcher.mojom:93: //
browser process.
On 2017/04/12 08:00:16, falken wrote:
> Can you add a comment explaining why this takes both
> ServiceWorkerFetchResponseCallback in addition to the mojo callback?
> 
> IIRC this has to do this the waitUntil/respondWith duality.
> 
> Maybe: The callback is called once the event finishes, which means the event
> handler ran and all outstanding respondWith() and waitUntil() promises have
> settled. |response_callback| is called once the promise to respondWith()
> settles, or when the event handler ran without calling respondWith().

Done.

https://codereview.chromium.org/2703343002/diff/400001/content/common/service...
File content/common/service_worker/service_worker_types.h (right):

https://codereview.chromium.org/2703343002/diff/400001/content/common/service...
content/common/service_worker/service_worker_types.h:179:
std::unique_ptr<ServiceWorkerHeaderMap> headers,
On 2017/04/12 08:00:16, falken wrote:
> Can you put a comment/todo here about the blobuuid and blobsize is only used
for
> responses where the body is a blob. For other types of responses, the body is
> provided separately in the mojo IPC via ServiceWorkerFetchResponseCallback.

Done.

https://codereview.chromium.org/2703343002/diff/400001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/fetch/FetchDataLoader.cpp (right):

https://codereview.chromium.org/2703343002/diff/400001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/FetchDataLoader.cpp:349:
mojo::SimpleWatcher datapipe_watcher_;
On 2017/04/12 08:00:16, falken wrote:
> nit: since the CamelCase version would be DataPipe, the snake_case versions
> should be data_pipe, not datapipe.

Done.

https://codereview.chromium.org/2703343002/diff/400001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/serviceworkers/FetchRespondWithObserver.cpp
(right):

https://codereview.chromium.org/2703343002/diff/400001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/serviceworkers/FetchRespondWithObserver.cpp:116:
class FetchLoaderClient final
On 2017/04/13 05:05:14, haraken wrote:
> On 2017/04/13 05:03:44, haraken wrote:
> > 
> > Would it be possible to remove this wrapper class (e.g., by making
> > WebServiceWorkerStreamHandle inherit from FetchDataLoader::Client)?
> 
> Correction: It would be nicer if we can remove both FetchLoaderClient and
> WebServiceWorkerStreamHandle. As kinuko-san said, it's interesting to consider
> using ServiceWorkerStreamHandlePtr directly.

IIUC, WebServiceWorkerStreamHandle cannot inherit FetchDataLoader::Client
because WebSWStreamHandle is in Source/platform/exported and
FetchDataLoder::Client is in Source/modules.
Is it possible to remove this class?
// Sorry if I missed something!

https://codereview.chromium.org/2703343002/diff/400001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/serviceworkers/FetchRespondWithObserver.cpp:123:
FetchLoaderClient(std::unique_ptr<WebServiceWorkerStreamHandle> handle)
On 2017/04/13 05:03:44, haraken wrote:
> 
> Add explicit.

Done.

https://codereview.chromium.org/2703343002/diff/400001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/serviceworkers/FetchRespondWithObserver.cpp:238:
} else {
On 2017/04/12 08:00:16, falken wrote:
> else isn't needed after the if() { return }

Done.

https://codereview.chromium.org/2703343002/diff/400001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerGlobalScopeClient.h
(right):

https://codereview.chromium.org/2703343002/diff/400001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerGlobalScopeClient.h:101:
// provided by the service worker in the fetch events, so fallback to native.
On 2017/04/12 08:00:16, falken wrote:
> This comment is redundant with the interface. A better name should also
obviate
> this comment.

Done.

https://codereview.chromium.org/2703343002/diff/400001/third_party/WebKit/pub...
File
third_party/WebKit/public/platform/modules/serviceworker/WebServiceWorkerStreamHandle.h
(right):

https://codereview.chromium.org/2703343002/diff/400001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/serviceworker/WebServiceWorkerStreamHandle.h:15:
class BLINK_PLATFORM_EXPORT WebServiceWorkerStreamHandle {
On 2017/04/13 05:03:44, haraken wrote:
> On 2017/04/13 02:22:27, kinuko wrote:
> > I'm probably missing something but do we need this wrapper? Would it not be
> > possible to directly use ServiceWorkerStreamHandlePtr?
> 
> Yeah, also I think it's worth thinking about removing FetchLoaderClient.
> FetchLoaderClient is just redirecting methods to WebServiceWorkerStreamHandle.

I totally agree that it's better if we can remove this wrapper.
However, passing the mojo object (blink.mojom.ServiceWorkerStreamHandle in this
case) from RespondWithObserver (in blink) to ServiceWorkerContextClient (in
renderer) when respondWith is fulfilled is awkward because we have two separated
mojo headers (.mojom.h and .mojom-blink.h) and we cannot move the interfaces
beyond the border between the renderer and the blink. 
That's why I put all mojo interfaces/structs in renderer.

https://codereview.chromium.org/2703343002/diff/400001/third_party/WebKit/pub...
File
third_party/WebKit/public/platform/modules/serviceworker/service_worker_stream_handle.mojom
(right):

https://codereview.chromium.org/2703343002/diff/400001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/serviceworker/service_worker_stream_handle.mojom:7:
// The renderer uses this interface to tell the browser when stream response
On 2017/04/13 05:03:44, haraken wrote:
> On 2017/04/12 08:00:16, falken wrote:
> > nit: "when the"
> 
> ...to notify the browser that the stream response...

Done.

https://codereview.chromium.org/2703343002/diff/400001/third_party/WebKit/pub...
File
third_party/WebKit/public/web/modules/serviceworker/WebServiceWorkerContextClient.h
(right):

https://codereview.chromium.org/2703343002/diff/400001/third_party/WebKit/pub...
third_party/WebKit/public/web/modules/serviceworker/WebServiceWorkerContextClient.h:171:
// dispatchFetchEvent respectively.
On 2017/04/12 08:00:16, falken wrote:
> RespondToFetchEvent and ResponseToFetchEventWithResponse are tricky names.
> 
> suggest....
> 
> // Used when respondWith() is not called. Tells the browser to fall back to
> native fetch.
> RespondToFetchEventWithNoResponse()
> // Responds to the fetch event with |response|.
> RespondToFetchEvent()
> // Responds to the fetch event with |response|, where body is
> |web_stream_handle|.
> RespondToFetchEventWithStreamBody()
> 
> I think |body_as_stream| or |body| is easier to understand than
> |web_stream_handle|.
> 

Done.

[falken, 2017-04-18 05:05:34]

https://codereview.chromium.org/2703343002/diff/440001/content/browser/servic...
File content/browser/service_worker/service_worker_data_pipe_reader.cc (right):

https://codereview.chromium.org/2703343002/diff/440001/content/browser/servic...
content/browser/service_worker/service_worker_data_pipe_reader.cc:119: // called
yet.
Where does this async completion happen? In OnHandleGotSignal? Or
OnComplete/OnAborted get called automatically by someone else?

https://codereview.chromium.org/2703343002/diff/440001/content/browser/servic...
File content/browser/service_worker/service_worker_data_pipe_reader.h (right):

https://codereview.chromium.org/2703343002/diff/440001/content/browser/servic...
content/browser/service_worker/service_worker_data_pipe_reader.h:23: 
nit: add a class-level comment like ServiceWorkerStreamReader had. what does the
class do, who owns it.

https://codereview.chromium.org/2703343002/diff/440001/content/browser/servic...
content/browser/service_worker/service_worker_data_pipe_reader.h:32: void
Start();
Can you document it calls owner_->OnResponseStarted.

https://codereview.chromium.org/2703343002/diff/440001/content/browser/servic...
content/browser/service_worker/service_worker_data_pipe_reader.h:34: // Copies
the data provided from the service worker to |buf| up to |bus_size|
nit: buf_size

https://codereview.chromium.org/2703343002/diff/440001/content/browser/servic...
content/browser/service_worker/service_worker_data_pipe_reader.h:37: // io.
It's kind of vague to group failure and pending IO together. Should this comment
just say "Same as URLRequestJob::ReadRawData" like ServiceWorkerStreamReader
did? What happens when ERR_IO_PENDING is returned, how does the caller know when
the operation completes?

https://codereview.chromium.org/2703343002/diff/440001/content/browser/servic...
content/browser/service_worker/service_worker_data_pipe_reader.h:68: State
producer_state_;
nit: initialize both |buffer_size_| and |producer_state_| in the same place,
either in the constructor or here.

https://codereview.chromium.org/2703343002/diff/440001/content/browser/servic...
File content/browser/service_worker/service_worker_data_pipe_reader_unittest.cc
(right):

https://codereview.chromium.org/2703343002/diff/440001/content/browser/servic...
content/browser/service_worker/service_worker_data_pipe_reader_unittest.cc:134:
public testing::WithParamInterface<std::tuple<bool, bool>> {
bool /* should_close_connection_first */, bool /* has_body */

https://codereview.chromium.org/2703343002/diff/440001/content/browser/servic...
content/browser/service_worker/service_worker_data_pipe_reader_unittest.cc:140:
bool is_closing_connection_first() const { return std::get<0>(GetParam()); }
should_close_connection_first?

https://codereview.chromium.org/2703343002/diff/440001/content/browser/servic...
content/browser/service_worker/service_worker_data_pipe_reader_unittest.cc:141:
bool is_body_exist() const { return std::get<1>(GetParam()); }
has_body

https://codereview.chromium.org/2703343002/diff/440001/content/browser/servic...
File content/browser/service_worker/service_worker_fetch_dispatcher.cc (right):

https://codereview.chromium.org/2703343002/diff/440001/content/browser/servic...
content/browser/service_worker/service_worker_fetch_dispatcher.cc:340:
base::WeakPtr<ServiceWorkerFetchDispatcher> dispatcher(fetch_dispatcher_);
I don't understand what this is doing. Making a WeakPtr to fetch_dispatcher_
doesn't protect it, no?

https://codereview.chromium.org/2703343002/diff/440001/content/browser/servic...
File content/browser/service_worker/service_worker_fetch_dispatcher.h (right):

https://codereview.chromium.org/2703343002/diff/440001/content/browser/servic...
content/browser/service_worker/service_worker_fetch_dispatcher.h:81:
blink::mojom::ServiceWorkerStreamHandlePtr stream_handle);
body_as_stream?

https://codereview.chromium.org/2703343002/diff/440001/content/browser/servic...
content/browser/service_worker/service_worker_fetch_dispatcher.h:85:
blink::mojom::ServiceWorkerStreamHandlePtr stream_handle);
ditto

https://codereview.chromium.org/2703343002/diff/440001/content/browser/servic...
File content/browser/service_worker/service_worker_url_request_job.cc (right):

https://codereview.chromium.org/2703343002/diff/440001/content/browser/servic...
content/browser/service_worker/service_worker_url_request_job.cc:633: // Process
stream using mojo's data pipe.
"Mojo's date pipe" or "mojo::DataPipe"

https://codereview.chromium.org/2703343002/diff/440001/content/browser/servic...
File content/browser/service_worker/service_worker_url_request_job.h (right):

https://codereview.chromium.org/2703343002/diff/440001/content/browser/servic...
content/browser/service_worker/service_worker_url_request_job.h:191:
blink::mojom::ServiceWorkerStreamHandlePtr stream_handle,
body_as_stream?

https://codereview.chromium.org/2703343002/diff/440001/content/common/service...
File content/common/service_worker/service_worker_messages.h (right):

https://codereview.chromium.org/2703343002/diff/440001/content/common/service...
content/common/service_worker/service_worker_messages.h:248: // to keep the IPC
ordering. For stream, no body and fallback, mojo IPC is used.
The second sentence is difficult to parse :)

"Mojo IPC is used when the response body is a stream or is empty, and for the
fallback-to-network response."

https://codereview.chromium.org/2703343002/diff/440001/content/common/service...
File content/common/service_worker/service_worker_types.h (right):

https://codereview.chromium.org/2703343002/diff/440001/content/common/service...
content/common/service_worker/service_worker_types.h:198: // types of responses,
the body is provided separately in the mojo IPC via
Mojo IPC

https://codereview.chromium.org/2703343002/diff/440001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/http/tests/serviceworker/chromium/resources/slow-fetch-and-stop-worker-iframe.html
(right):

https://codereview.chromium.org/2703343002/diff/440001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/serviceworker/chromium/resources/slow-fetch-and-stop-worker-iframe.html:12:
e.origin);
I don't think you need the e.origin thing

https://codereview.chromium.org/2703343002/diff/440001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/http/tests/serviceworker/chromium/stop-worker-during-respond-with.html
(right):

https://codereview.chromium.org/2703343002/diff/440001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/serviceworker/chromium/stop-worker-during-respond-with.html:2:
<title>Service Worker: Stopping the worker during responding to a fetch</title>
add comment: This is in chromium/ because it relies on the internals API to
terminate a service worker.

https://codereview.chromium.org/2703343002/diff/440001/third_party/WebKit/pub...
File
third_party/WebKit/public/platform/modules/serviceworker/WebServiceWorkerStreamHandle.h
(right):

https://codereview.chromium.org/2703343002/diff/440001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/serviceworker/WebServiceWorkerStreamHandle.h:15:
class BLINK_PLATFORM_EXPORT WebServiceWorkerStreamHandle {
nit document this class and the public Listener interface?

[haraken, 2017-04-18 11:41:19]

https://codereview.chromium.org/2703343002/diff/400001/third_party/WebKit/pub...
File
third_party/WebKit/public/platform/modules/serviceworker/WebServiceWorkerStreamHandle.h
(right):

https://codereview.chromium.org/2703343002/diff/400001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/serviceworker/WebServiceWorkerStreamHandle.h:15:
class BLINK_PLATFORM_EXPORT WebServiceWorkerStreamHandle {
On 2017/04/18 01:50:05, shimazu wrote:
> On 2017/04/13 05:03:44, haraken wrote:
> > On 2017/04/13 02:22:27, kinuko wrote:
> > > I'm probably missing something but do we need this wrapper? Would it not
be
> > > possible to directly use ServiceWorkerStreamHandlePtr?
> > 
> > Yeah, also I think it's worth thinking about removing FetchLoaderClient.
> > FetchLoaderClient is just redirecting methods to
WebServiceWorkerStreamHandle.
> 
> I totally agree that it's better if we can remove this wrapper.
> However, passing the mojo object (blink.mojom.ServiceWorkerStreamHandle in
this
> case) from RespondWithObserver (in blink) to ServiceWorkerContextClient (in
> renderer) when respondWith is fulfilled is awkward because we have two
separated
> mojo headers (.mojom.h and .mojom-blink.h) and we cannot move the interfaces
> beyond the border between the renderer and the blink. 
> That's why I put all mojo interfaces/structs in renderer.

I'm not sure if I understand the background but would it be hard to move
ServiceWorkerContextClient to the blink side? Given that the overall goal is to
move more SW code from content/renderer/ to Blink, it seems like that adding
mojo interfaces/structs to content/renderer/ is going in the opposite way.

[shimazu, 2017-04-19 05:49:47]

Thanks for your review.
Uploaded ps#23 for adding/refining comments. PTAnL.

https://codereview.chromium.org/2703343002/diff/400001/third_party/WebKit/pub...
File
third_party/WebKit/public/platform/modules/serviceworker/WebServiceWorkerStreamHandle.h
(right):

https://codereview.chromium.org/2703343002/diff/400001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/serviceworker/WebServiceWorkerStreamHandle.h:15:
class BLINK_PLATFORM_EXPORT WebServiceWorkerStreamHandle {
On 2017/04/18 11:41:18, haraken wrote:
> On 2017/04/18 01:50:05, shimazu wrote:
> > On 2017/04/13 05:03:44, haraken wrote:
> > > On 2017/04/13 02:22:27, kinuko wrote:
> > > > I'm probably missing something but do we need this wrapper? Would it not
> be
> > > > possible to directly use ServiceWorkerStreamHandlePtr?
> > > 
> > > Yeah, also I think it's worth thinking about removing FetchLoaderClient.
> > > FetchLoaderClient is just redirecting methods to
> WebServiceWorkerStreamHandle.
> > 
> > I totally agree that it's better if we can remove this wrapper.
> > However, passing the mojo object (blink.mojom.ServiceWorkerStreamHandle in
> this
> > case) from RespondWithObserver (in blink) to ServiceWorkerContextClient (in
> > renderer) when respondWith is fulfilled is awkward because we have two
> separated
> > mojo headers (.mojom.h and .mojom-blink.h) and we cannot move the interfaces
> > beyond the border between the renderer and the blink. 
> > That's why I put all mojo interfaces/structs in renderer.
> 
> I'm not sure if I understand the background but would it be hard to move
> ServiceWorkerContextClient to the blink side? Given that the overall goal is
to
> move more SW code from content/renderer/ to Blink, it seems like that adding
> mojo interfaces/structs to content/renderer/ is going in the opposite way.
> 


Yes, I don't think it's easy because we should move the mojo-related thing at
once (due to the boundary issue). This patch is just for avoiding the thread
hop.
Additionally, our focus is now slipping to how to re-architect the service
worker aligning with the servicification. I'm considering it would be good to
move the code into blink after the design of servicified architecture is
depicted.
WDYT?

https://codereview.chromium.org/2703343002/diff/440001/content/browser/servic...
File content/browser/service_worker/service_worker_data_pipe_reader.cc (right):

https://codereview.chromium.org/2703343002/diff/440001/content/browser/servic...
content/browser/service_worker/service_worker_data_pipe_reader.cc:119: // called
yet.
On 2017/04/18 05:05:33, falken (away until May 8) wrote:
> Where does this async completion happen? In OnHandleGotSignal? Or
> OnComplete/OnAborted get called automatically by someone else?

This state means |stream_| has already been disconnected. If |binding_| is also
disconnected, OnAborted is called because it's set as the error handler in the
constructor. Otherwise OnAborted/OnCompleted should be called from the renderer.

https://codereview.chromium.org/2703343002/diff/440001/content/browser/servic...
File content/browser/service_worker/service_worker_data_pipe_reader.h (right):

https://codereview.chromium.org/2703343002/diff/440001/content/browser/servic...
content/browser/service_worker/service_worker_data_pipe_reader.h:23: 
On 2017/04/18 05:05:33, falken (away until May 8) wrote:
> nit: add a class-level comment like ServiceWorkerStreamReader had. what does
the
> class do, who owns it.

Done.

https://codereview.chromium.org/2703343002/diff/440001/content/browser/servic...
content/browser/service_worker/service_worker_data_pipe_reader.h:32: void
Start();
On 2017/04/18 05:05:33, falken (away until May 8) wrote:
> Can you document it calls owner_->OnResponseStarted.

Done.

https://codereview.chromium.org/2703343002/diff/440001/content/browser/servic...
content/browser/service_worker/service_worker_data_pipe_reader.h:34: // Copies
the data provided from the service worker to |buf| up to |bus_size|
On 2017/04/18 05:05:33, falken (away until May 8) wrote:
> nit: buf_size

Done.

https://codereview.chromium.org/2703343002/diff/440001/content/browser/servic...
content/browser/service_worker/service_worker_data_pipe_reader.h:37: // io.
On 2017/04/18 05:05:33, falken (away until May 8) wrote:
> It's kind of vague to group failure and pending IO together. Should this
comment
> just say "Same as URLRequestJob::ReadRawData" like ServiceWorkerStreamReader
> did? What happens when ERR_IO_PENDING is returned, how does the caller know
when
> the operation completes?

Done.

https://codereview.chromium.org/2703343002/diff/440001/content/browser/servic...
content/browser/service_worker/service_worker_data_pipe_reader.h:68: State
producer_state_;
On 2017/04/18 05:05:33, falken (away until May 8) wrote:
> nit: initialize both |buffer_size_| and |producer_state_| in the same place,
> either in the constructor or here.

Done.

https://codereview.chromium.org/2703343002/diff/440001/content/browser/servic...
File content/browser/service_worker/service_worker_data_pipe_reader_unittest.cc
(right):

https://codereview.chromium.org/2703343002/diff/440001/content/browser/servic...
content/browser/service_worker/service_worker_data_pipe_reader_unittest.cc:134:
public testing::WithParamInterface<std::tuple<bool, bool>> {
On 2017/04/18 05:05:33, falken (away until May 8) wrote:
> bool /* should_close_connection_first */, bool /* has_body */

Done.

https://codereview.chromium.org/2703343002/diff/440001/content/browser/servic...
content/browser/service_worker/service_worker_data_pipe_reader_unittest.cc:140:
bool is_closing_connection_first() const { return std::get<0>(GetParam()); }
On 2017/04/18 05:05:33, falken (away until May 8) wrote:
> should_close_connection_first?

Done.

https://codereview.chromium.org/2703343002/diff/440001/content/browser/servic...
content/browser/service_worker/service_worker_data_pipe_reader_unittest.cc:141:
bool is_body_exist() const { return std::get<1>(GetParam()); }
On 2017/04/18 05:05:33, falken (away until May 8) wrote:
> has_body

Done.

https://codereview.chromium.org/2703343002/diff/440001/content/browser/servic...
File content/browser/service_worker/service_worker_fetch_dispatcher.cc (right):

https://codereview.chromium.org/2703343002/diff/440001/content/browser/servic...
content/browser/service_worker/service_worker_fetch_dispatcher.cc:340:
base::WeakPtr<ServiceWorkerFetchDispatcher> dispatcher(fetch_dispatcher_);
On 2017/04/18 05:05:33, falken (away until May 8) wrote:
> I don't understand what this is doing. Making a WeakPtr to fetch_dispatcher_
> doesn't protect it, no?

This protects the weakptr itself. base::Closure finish_task =
base::Bind(&SWFetchDispatcher::DidFinish, fetch_dispatcher, ...); and calling it
at l.351 would be the same.

https://codereview.chromium.org/2703343002/diff/440001/content/browser/servic...
File content/browser/service_worker/service_worker_fetch_dispatcher.h (right):

https://codereview.chromium.org/2703343002/diff/440001/content/browser/servic...
content/browser/service_worker/service_worker_fetch_dispatcher.h:81:
blink::mojom::ServiceWorkerStreamHandlePtr stream_handle);
On 2017/04/18 05:05:33, falken (away until May 8) wrote:
> body_as_stream?

Done.

https://codereview.chromium.org/2703343002/diff/440001/content/browser/servic...
content/browser/service_worker/service_worker_fetch_dispatcher.h:85:
blink::mojom::ServiceWorkerStreamHandlePtr stream_handle);
On 2017/04/18 05:05:33, falken (away until May 8) wrote:
> ditto

Done.

https://codereview.chromium.org/2703343002/diff/440001/content/browser/servic...
File content/browser/service_worker/service_worker_url_request_job.cc (right):

https://codereview.chromium.org/2703343002/diff/440001/content/browser/servic...
content/browser/service_worker/service_worker_url_request_job.cc:633: // Process
stream using mojo's data pipe.
On 2017/04/18 05:05:33, falken (away until May 8) wrote:
> "Mojo's date pipe" or "mojo::DataPipe"

Done.

https://codereview.chromium.org/2703343002/diff/440001/content/browser/servic...
File content/browser/service_worker/service_worker_url_request_job.h (right):

https://codereview.chromium.org/2703343002/diff/440001/content/browser/servic...
content/browser/service_worker/service_worker_url_request_job.h:191:
blink::mojom::ServiceWorkerStreamHandlePtr stream_handle,
On 2017/04/18 05:05:33, falken (away until May 8) wrote:
> body_as_stream?

Done.

https://codereview.chromium.org/2703343002/diff/440001/content/common/service...
File content/common/service_worker/service_worker_messages.h (right):

https://codereview.chromium.org/2703343002/diff/440001/content/common/service...
content/common/service_worker/service_worker_messages.h:248: // to keep the IPC
ordering. For stream, no body and fallback, mojo IPC is used.
On 2017/04/18 05:05:33, falken (away until May 8) wrote:
> The second sentence is difficult to parse :)
> 
> "Mojo IPC is used when the response body is a stream or is empty, and for the
> fallback-to-network response."

Done.

https://codereview.chromium.org/2703343002/diff/440001/content/common/service...
File content/common/service_worker/service_worker_types.h (right):

https://codereview.chromium.org/2703343002/diff/440001/content/common/service...
content/common/service_worker/service_worker_types.h:198: // types of responses,
the body is provided separately in the mojo IPC via
On 2017/04/18 05:05:34, falken (away until May 8) wrote:
> Mojo IPC

Done.

https://codereview.chromium.org/2703343002/diff/440001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/http/tests/serviceworker/chromium/resources/slow-fetch-and-stop-worker-iframe.html
(right):

https://codereview.chromium.org/2703343002/diff/440001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/serviceworker/chromium/resources/slow-fetch-and-stop-worker-iframe.html:12:
e.origin);
On 2017/04/18 05:05:34, falken (away until May 8) wrote:
> I don't think you need the e.origin thing

IIUC, MessagePort.postMessage doesn't need the origin, but this code needs it
because posting a message to e.source (WindowProxy).

https://codereview.chromium.org/2703343002/diff/440001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/http/tests/serviceworker/chromium/stop-worker-during-respond-with.html
(right):

https://codereview.chromium.org/2703343002/diff/440001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/serviceworker/chromium/stop-worker-during-respond-with.html:2:
<title>Service Worker: Stopping the worker during responding to a fetch</title>
On 2017/04/18 05:05:34, falken (away until May 8) wrote:
> add comment: This is in chromium/ because it relies on the internals API to
> terminate a service worker.

Done.

https://codereview.chromium.org/2703343002/diff/440001/third_party/WebKit/pub...
File
third_party/WebKit/public/platform/modules/serviceworker/WebServiceWorkerStreamHandle.h
(right):

https://codereview.chromium.org/2703343002/diff/440001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/serviceworker/WebServiceWorkerStreamHandle.h:15:
class BLINK_PLATFORM_EXPORT WebServiceWorkerStreamHandle {
On 2017/04/18 05:05:34, falken (away until May 8) wrote:
> nit document this class and the public Listener interface?

Done.

[falken, 2017-04-19 06:56:53]

I think this lgtm.

https://codereview.chromium.org/2703343002/diff/460001/content/browser/servic...
File content/browser/service_worker/service_worker_data_pipe_reader.cc (right):

https://codereview.chromium.org/2703343002/diff/460001/content/browser/servic...
content/browser/service_worker/service_worker_data_pipe_reader.cc:51: void
ServiceWorkerDataPipeReader::OnHandleGotSignal(MojoResult) {
It's a little surprising MojoResult is not used. Is this intentional?

https://codereview.chromium.org/2703343002/diff/460001/content/browser/servic...
content/browser/service_worker/service_worker_data_pipe_reader.cc:53: this,
"OnHandleGotReadable");
OnHandleGotSignal?

https://codereview.chromium.org/2703343002/diff/460001/content/browser/servic...
File content/browser/service_worker/service_worker_data_pipe_reader.h (right):

https://codereview.chromium.org/2703343002/diff/460001/content/browser/servic...
content/browser/service_worker/service_worker_data_pipe_reader.h:36: // the
response starts.
IIUC, this synchronously calls OnResponseStarted no matter what. So it should be
"Calls owner_->OnResponseStarted()"?

https://codereview.chromium.org/2703343002/diff/460001/content/browser/servic...
File content/browser/service_worker/service_worker_data_pipe_reader_unittest.cc
(right):

https://codereview.chromium.org/2703343002/diff/460001/content/browser/servic...
content/browser/service_worker/service_worker_data_pipe_reader_unittest.cc:319:
ASSERT_EQ(0UL, mock_url_request_job()->results().size());
EXPECT_EQ?

https://codereview.chromium.org/2703343002/diff/460001/content/browser/servic...
content/browser/service_worker/service_worker_data_pipe_reader_unittest.cc:393:
ASSERT_EQ(0UL, mock_url_request_job()->results().size());
EXPECT_EQ?

https://codereview.chromium.org/2703343002/diff/460001/content/browser/servic...
File content/browser/service_worker/service_worker_fetch_dispatcher.cc (right):

https://codereview.chromium.org/2703343002/diff/460001/content/browser/servic...
content/browser/service_worker/service_worker_fetch_dispatcher.cc:339: //
Protects |fetch_dispatcher_|.
// Copy |fetch_dispatcher_| and |fetch_event_id| for use after |this|  is
destroyed.

https://codereview.chromium.org/2703343002/diff/460001/content/browser/servic...
File content/browser/service_worker/service_worker_url_request_job_unittest.cc
(right):

https://codereview.chromium.org/2703343002/diff/460001/content/browser/servic...
content/browser/service_worker/service_worker_url_request_job_unittest.cc:741: }
nit: Why is this happening 512 times? It's just to be sure that every
WriteDataRaw will result in an error after Cancel() was called?

https://codereview.chromium.org/2703343002/diff/460001/content/browser/servic...
content/browser/service_worker/service_worker_url_request_job_unittest.cc:853:
EXPECT_EQ(net::ERR_CONNECTION_RESET, request_->status().ToNetError());
I think lines 851-852 are redundant with 853.

https://codereview.chromium.org/2703343002/diff/460001/content/renderer/servi...
File content/renderer/service_worker/service_worker_context_client.h (right):

https://codereview.chromium.org/2703343002/diff/460001/content/renderer/servi...
content/renderer/service_worker/service_worker_context_client.h:78: base::Time
/* dispatch_event_time */)>;
mistaken rebase?

https://codereview.chromium.org/2703343002/diff/460001/third_party/WebKit/pub...
File
third_party/WebKit/public/web/modules/serviceworker/WebServiceWorkerContextClient.h
(right):

https://codereview.chromium.org/2703343002/diff/460001/third_party/WebKit/pub...
third_party/WebKit/public/web/modules/serviceworker/WebServiceWorkerContextClient.h:171:
// DispatchFetchEvent respectively.
maybe add an empty line to make clear that "Used when..." applies to
RespondToFetchEventWithResponse only, whereas the paragraph above applies to all
the RespondToFetch* functions.

[shimazu, 2017-04-19 08:26:44]

The CQ bit was checked by shimazu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-19 08:27:11]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[shimazu, 2017-04-19 08:28:42]

The CQ bit was unchecked by shimazu@chromium.org

[shimazu, 2017-04-19 08:29:01]

Patchset #24 (id:480001) has been deleted

[shimazu, 2017-04-19 08:31:12]

The CQ bit was checked by shimazu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-19 08:31:27]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[shimazu, 2017-04-19 08:32:16]

https://codereview.chromium.org/2703343002/diff/460001/content/browser/servic...
File content/browser/service_worker/service_worker_data_pipe_reader.cc (right):

https://codereview.chromium.org/2703343002/diff/460001/content/browser/servic...
content/browser/service_worker/service_worker_data_pipe_reader.cc:51: void
ServiceWorkerDataPipeReader::OnHandleGotSignal(MojoResult) {
On 2017/04/19 06:56:52, falken (away until May 8) wrote:
> It's a little surprising MojoResult is not used. Is this intentional?

Yes, it is. MojoResult indicates the state of the wather, and this method works
well even if the watcher won't be triggered anymore or the watched handle has
been closed.
This is a link to the comment about what type of result will be returned:
https://cs.chromium.org/chromium/src/mojo/public/cpp/system/simple_watcher.h?...

https://codereview.chromium.org/2703343002/diff/460001/content/browser/servic...
content/browser/service_worker/service_worker_data_pipe_reader.cc:53: this,
"OnHandleGotReadable");
On 2017/04/19 06:56:52, falken (away until May 8) wrote:
> OnHandleGotSignal?

Oops, thanks. Done.

https://codereview.chromium.org/2703343002/diff/460001/content/browser/servic...
File content/browser/service_worker/service_worker_data_pipe_reader.h (right):

https://codereview.chromium.org/2703343002/diff/460001/content/browser/servic...
content/browser/service_worker/service_worker_data_pipe_reader.h:36: // the
response starts.
On 2017/04/19 06:56:52, falken (away until May 8) wrote:
> IIUC, this synchronously calls OnResponseStarted no matter what. So it should
be
> "Calls owner_->OnResponseStarted()"?

Done.

https://codereview.chromium.org/2703343002/diff/460001/content/browser/servic...
File content/browser/service_worker/service_worker_data_pipe_reader_unittest.cc
(right):

https://codereview.chromium.org/2703343002/diff/460001/content/browser/servic...
content/browser/service_worker/service_worker_data_pipe_reader_unittest.cc:319:
ASSERT_EQ(0UL, mock_url_request_job()->results().size());
On 2017/04/19 06:56:52, falken (away until May 8) wrote:
> EXPECT_EQ?

Done.

https://codereview.chromium.org/2703343002/diff/460001/content/browser/servic...
content/browser/service_worker/service_worker_data_pipe_reader_unittest.cc:393:
ASSERT_EQ(0UL, mock_url_request_job()->results().size());
On 2017/04/19 06:56:52, falken (away until May 8) wrote:
> EXPECT_EQ?

Done.

https://codereview.chromium.org/2703343002/diff/460001/content/browser/servic...
File content/browser/service_worker/service_worker_fetch_dispatcher.cc (right):

https://codereview.chromium.org/2703343002/diff/460001/content/browser/servic...
content/browser/service_worker/service_worker_fetch_dispatcher.cc:339: //
Protects |fetch_dispatcher_|.
On 2017/04/19 06:56:52, falken (away until May 8) wrote:
> // Copy |fetch_dispatcher_| and |fetch_event_id| for use after |this|  is
> destroyed.

Done.

https://codereview.chromium.org/2703343002/diff/460001/content/browser/servic...
File content/browser/service_worker/service_worker_url_request_job_unittest.cc
(right):

https://codereview.chromium.org/2703343002/diff/460001/content/browser/servic...
content/browser/service_worker/service_worker_url_request_job_unittest.cc:741: }
On 2017/04/19 06:56:52, falken (away until May 8) wrote:
> nit: Why is this happening 512 times? It's just to be sure that every
> WriteDataRaw will result in an error after Cancel() was called?

Yes, but I don't have a strong opinion here. This is just copied from
StreamResponseAndCancel test. 
We can test only once because it seems like a test for Mojo's data pipe. 
Updated.

https://codereview.chromium.org/2703343002/diff/460001/content/browser/servic...
content/browser/service_worker/service_worker_url_request_job_unittest.cc:853:
EXPECT_EQ(net::ERR_CONNECTION_RESET, request_->status().ToNetError());
On 2017/04/19 06:56:52, falken (away until May 8) wrote:
> I think lines 851-852 are redundant with 853.

Done.

https://codereview.chromium.org/2703343002/diff/460001/content/renderer/servi...
File content/renderer/service_worker/service_worker_context_client.h (right):

https://codereview.chromium.org/2703343002/diff/460001/content/renderer/servi...
content/renderer/service_worker/service_worker_context_client.h:78: base::Time
/* dispatch_event_time */)>;
On 2017/04/19 06:56:52, falken (away until May 8) wrote:
> mistaken rebase?

Done.

https://codereview.chromium.org/2703343002/diff/460001/third_party/WebKit/pub...
File
third_party/WebKit/public/web/modules/serviceworker/WebServiceWorkerContextClient.h
(right):

https://codereview.chromium.org/2703343002/diff/460001/third_party/WebKit/pub...
third_party/WebKit/public/web/modules/serviceworker/WebServiceWorkerContextClient.h:171:
// DispatchFetchEvent respectively.
On 2017/04/19 06:56:52, falken (away until May 8) wrote:
> maybe add an empty line to make clear that "Used when..." applies to
> RespondToFetchEventWithResponse only, whereas the paragraph above applies to
all
> the RespondToFetch* functions.

Done.

[haraken, 2017-04-19 09:14:30]

https://codereview.chromium.org/2703343002/diff/400001/third_party/WebKit/pub...
File
third_party/WebKit/public/platform/modules/serviceworker/WebServiceWorkerStreamHandle.h
(right):

https://codereview.chromium.org/2703343002/diff/400001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/serviceworker/WebServiceWorkerStreamHandle.h:15:
class BLINK_PLATFORM_EXPORT WebServiceWorkerStreamHandle {
On 2017/04/19 05:49:46, shimazu wrote:
> On 2017/04/18 11:41:18, haraken wrote:
> > On 2017/04/18 01:50:05, shimazu wrote:
> > > On 2017/04/13 05:03:44, haraken wrote:
> > > > On 2017/04/13 02:22:27, kinuko wrote:
> > > > > I'm probably missing something but do we need this wrapper? Would it
not
> > be
> > > > > possible to directly use ServiceWorkerStreamHandlePtr?
> > > > 
> > > > Yeah, also I think it's worth thinking about removing FetchLoaderClient.
> > > > FetchLoaderClient is just redirecting methods to
> > WebServiceWorkerStreamHandle.
> > > 
> > > I totally agree that it's better if we can remove this wrapper.
> > > However, passing the mojo object (blink.mojom.ServiceWorkerStreamHandle in
> > this
> > > case) from RespondWithObserver (in blink) to ServiceWorkerContextClient
(in
> > > renderer) when respondWith is fulfilled is awkward because we have two
> > separated
> > > mojo headers (.mojom.h and .mojom-blink.h) and we cannot move the
interfaces
> > > beyond the border between the renderer and the blink. 
> > > That's why I put all mojo interfaces/structs in renderer.
> > 
> > I'm not sure if I understand the background but would it be hard to move
> > ServiceWorkerContextClient to the blink side? Given that the overall goal is
> to
> > move more SW code from content/renderer/ to Blink, it seems like that adding
> > mojo interfaces/structs to content/renderer/ is going in the opposite way.
> > 
> 
> 
> Yes, I don't think it's easy because we should move the mojo-related thing at
> once (due to the boundary issue). This patch is just for avoiding the thread
> hop.
> Additionally, our focus is now slipping to how to re-architect the service
> worker aligning with the servicification. I'm considering it would be good to
> move the code into blink after the design of servicified architecture is
> depicted.
> WDYT?

If it's hard, I'm okay with proceeding with this CL as is.

What are you planning as the end state of the servicification of SW? I was
assuming that almost everything in content/renderer/ is moved to Blink which
talks directly with the SW service in the browser process.

[kinuko, 2017-04-19 09:17:47]

lgtm % some more nits & one question.

https://codereview.chromium.org/2703343002/diff/460001/content/browser/servic...
File content/browser/service_worker/service_worker_data_pipe_reader.cc (right):

https://codereview.chromium.org/2703343002/diff/460001/content/browser/servic...
content/browser/service_worker/service_worker_data_pipe_reader.cc:89: case
MOJO_RESULT_BUSY:
Why is this handled as an error, is that because this shouldn't happen in the
current code?

https://codereview.chromium.org/2703343002/diff/460001/content/browser/servic...
File content/browser/service_worker/service_worker_data_pipe_reader.h (right):

https://codereview.chromium.org/2703343002/diff/460001/content/browser/servic...
content/browser/service_worker/service_worker_data_pipe_reader.h:48: enum class
State { STREAMING, COMPLETED, ABORTED };
nit: kStreaming, kCompleted, kAborted ? (context:
https://codereview.chromium.org/2820553002)

https://codereview.chromium.org/2703343002/diff/460001/content/browser/servic...
File content/browser/service_worker/service_worker_data_pipe_reader_unittest.cc
(right):

https://codereview.chromium.org/2703343002/diff/460001/content/browser/servic...
content/browser/service_worker/service_worker_data_pipe_reader_unittest.cc:28:
MockServiceWorkerURLRequestJob(ServiceWorkerURLRequestJob::Delegate* delegate)
nit: explicit

[kinuko, 2017-04-19 09:24:41]

On 2017/04/19 09:14:30, haraken wrote:
>
https://codereview.chromium.org/2703343002/diff/400001/third_party/WebKit/pub...
> File
>
third_party/WebKit/public/platform/modules/serviceworker/WebServiceWorkerStreamHandle.h
> (right):
> 
>
https://codereview.chromium.org/2703343002/diff/400001/third_party/WebKit/pub...
>
third_party/WebKit/public/platform/modules/serviceworker/WebServiceWorkerStreamHandle.h:15:
> class BLINK_PLATFORM_EXPORT WebServiceWorkerStreamHandle {
> On 2017/04/19 05:49:46, shimazu wrote:
> > On 2017/04/18 11:41:18, haraken wrote:
> > > On 2017/04/18 01:50:05, shimazu wrote:
> > > > On 2017/04/13 05:03:44, haraken wrote:
> > > > > On 2017/04/13 02:22:27, kinuko wrote:
> > > > > > I'm probably missing something but do we need this wrapper? Would it
> not
> > > be
> > > > > > possible to directly use ServiceWorkerStreamHandlePtr?
> > > > > 
> > > > > Yeah, also I think it's worth thinking about removing
FetchLoaderClient.
> > > > > FetchLoaderClient is just redirecting methods to
> > > WebServiceWorkerStreamHandle.
> > > > 
> > > > I totally agree that it's better if we can remove this wrapper.
> > > > However, passing the mojo object (blink.mojom.ServiceWorkerStreamHandle
in
> > > this
> > > > case) from RespondWithObserver (in blink) to ServiceWorkerContextClient
> (in
> > > > renderer) when respondWith is fulfilled is awkward because we have two
> > > separated
> > > > mojo headers (.mojom.h and .mojom-blink.h) and we cannot move the
> interfaces
> > > > beyond the border between the renderer and the blink. 
> > > > That's why I put all mojo interfaces/structs in renderer.
> > > 
> > > I'm not sure if I understand the background but would it be hard to move
> > > ServiceWorkerContextClient to the blink side? Given that the overall goal
is
> > to
> > > move more SW code from content/renderer/ to Blink, it seems like that
adding
> > > mojo interfaces/structs to content/renderer/ is going in the opposite way.
> > > 
> > 
> > 
> > Yes, I don't think it's easy because we should move the mojo-related thing
at
> > once (due to the boundary issue). This patch is just for avoiding the thread
> > hop.
> > Additionally, our focus is now slipping to how to re-architect the service
> > worker aligning with the servicification. I'm considering it would be good
to
> > move the code into blink after the design of servicified architecture is
> > depicted.
> > WDYT?
> 
> If it's hard, I'm okay with proceeding with this CL as is.
> 
> What are you planning as the end state of the servicification of SW? I was
> assuming that almost everything in content/renderer/ is moved to Blink which
> talks directly with the SW service in the browser process.

Fyi, we're massively discussing this, and it will likely be huge revise rather
than incremental move. And yes, we should be moving away from content/renderer.

[commit-bot: I haz the power, 2017-04-19 10:30:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-19 10:31:01]

Dry run: This issue passed the CQ dry run.

[haraken, 2017-04-19 11:29:09]

LGTM

I'd add a TODO to remove WebServiceWorkerStreamHandle by moving the SW client to
Blink. The point of the Blink mojofication is to avoid such a public API
boundary by letting Blink directly talk with the browser process using Mojo.

https://codereview.chromium.org/2703343002/diff/500001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/fetch/FetchDataLoader.cpp (right):

https://codereview.chromium.org/2703343002/diff/500001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/FetchDataLoader.cpp:248:
"FetchDataLoaderAsDataPipe::FetchDataLoaderAsDataPipe");

Is this trace event useful?

https://codereview.chromium.org/2703343002/diff/500001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/serviceworkers/FetchRespondWithObserver.cpp
(right):

https://codereview.chromium.org/2703343002/diff/500001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/serviceworkers/FetchRespondWithObserver.cpp:116:
class FetchLoaderClient final

Add a class-level comment. e.g., why this indirection is needed.

https://codereview.chromium.org/2703343002/diff/500001/third_party/WebKit/pub...
File
third_party/WebKit/public/platform/modules/serviceworker/WebServiceWorkerStreamHandle.h
(right):

https://codereview.chromium.org/2703343002/diff/500001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/serviceworker/WebServiceWorkerStreamHandle.h:31:
WebServiceWorkerStreamHandle(mojo::ScopedDataPipeConsumerHandle stream)

Add explicit.

[haraken, 2017-04-19 11:29:10]

LGTM

I'd add a TODO to remove WebServiceWorkerStreamHandle by moving the SW client to
Blink. The point of the Blink mojofication is to avoid such a public API
boundary by letting Blink directly talk with the browser process using Mojo.

https://codereview.chromium.org/2703343002/diff/500001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/fetch/FetchDataLoader.cpp (right):

https://codereview.chromium.org/2703343002/diff/500001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/FetchDataLoader.cpp:248:
"FetchDataLoaderAsDataPipe::FetchDataLoaderAsDataPipe");

Is this trace event useful?

https://codereview.chromium.org/2703343002/diff/500001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/serviceworkers/FetchRespondWithObserver.cpp
(right):

https://codereview.chromium.org/2703343002/diff/500001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/serviceworkers/FetchRespondWithObserver.cpp:116:
class FetchLoaderClient final

Add a class-level comment. e.g., why this indirection is needed.

https://codereview.chromium.org/2703343002/diff/500001/third_party/WebKit/pub...
File
third_party/WebKit/public/platform/modules/serviceworker/WebServiceWorkerStreamHandle.h
(right):

https://codereview.chromium.org/2703343002/diff/500001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/serviceworker/WebServiceWorkerStreamHandle.h:31:
WebServiceWorkerStreamHandle(mojo::ScopedDataPipeConsumerHandle stream)

Add explicit.

[shimazu, 2017-04-20 04:20:42]

Thanks for comments. updated.

https://codereview.chromium.org/2703343002/diff/460001/content/browser/servic...
File content/browser/service_worker/service_worker_data_pipe_reader.cc (right):

https://codereview.chromium.org/2703343002/diff/460001/content/browser/servic...
content/browser/service_worker/service_worker_data_pipe_reader.cc:89: case
MOJO_RESULT_BUSY:
On 2017/04/19 09:17:47, kinuko wrote:
> Why is this handled as an error, is that because this shouldn't happen in the
> current code?

Yes, this won't happen. NOTREACHED might be preferable here. Updated.

https://codereview.chromium.org/2703343002/diff/460001/content/browser/servic...
File content/browser/service_worker/service_worker_data_pipe_reader.h (right):

https://codereview.chromium.org/2703343002/diff/460001/content/browser/servic...
content/browser/service_worker/service_worker_data_pipe_reader.h:48: enum class
State { STREAMING, COMPLETED, ABORTED };
On 2017/04/19 09:17:47, kinuko wrote:
> nit: kStreaming, kCompleted, kAborted ? (context:
> https://codereview.chromium.org/2820553002)

Done.

https://codereview.chromium.org/2703343002/diff/460001/content/browser/servic...
File content/browser/service_worker/service_worker_data_pipe_reader_unittest.cc
(right):

https://codereview.chromium.org/2703343002/diff/460001/content/browser/servic...
content/browser/service_worker/service_worker_data_pipe_reader_unittest.cc:28:
MockServiceWorkerURLRequestJob(ServiceWorkerURLRequestJob::Delegate* delegate)
On 2017/04/19 09:17:47, kinuko wrote:
> nit: explicit

Done.

https://codereview.chromium.org/2703343002/diff/500001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/fetch/FetchDataLoader.cpp (right):

https://codereview.chromium.org/2703343002/diff/500001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/FetchDataLoader.cpp:248:
"FetchDataLoaderAsDataPipe::FetchDataLoaderAsDataPipe");
On 2017/04/19 11:29:10, haraken wrote:
> 
> Is this trace event useful?

Thanks! It's for debug. Removed.

https://codereview.chromium.org/2703343002/diff/500001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/serviceworkers/FetchRespondWithObserver.cpp
(right):

https://codereview.chromium.org/2703343002/diff/500001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/serviceworkers/FetchRespondWithObserver.cpp:116:
class FetchLoaderClient final
On 2017/04/19 11:29:10, haraken wrote:
> 
> Add a class-level comment. e.g., why this indirection is needed.

Done.

[shimazu, 2017-04-20 04:21:33]

The CQ bit was checked by shimazu@chromium.org

[shimazu, 2017-04-20 04:21:34]

The patchset sent to the CQ was uploaded after l-g-t-m from horo@chromium.org,
tsepez@chromium.org, falken@chromium.org, kinuko@chromium.org,
haraken@chromium.org
Link to the patchset: https://codereview.chromium.org/2703343002/#ps520001
(title: "Addressed comments from kinuko and haraken")

[commit-bot: I haz the power, 2017-04-20 04:22:04]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-20 06:03:53]

CQ is committing da patch.

Bot data: {"patchset_id": 520001, "attempt_start_ts": 1492662093378370,
"parent_rev": "f9f882493c6285711f95b88648974e1641168c28", "commit_rev":
"1ac68cf90fa046d3e9c1418aee7cd14b8ec38210"}

[commit-bot: I haz the power, 2017-04-20 06:04:01]

Description was changed from

==========
ServiceWorker: Use mojo's data pipe for respondWith(stream)

Currently respondWith(fetch(...)) is using Stream provided by BlobRegistry.
BlobRegistry is living on the main thread and any respondWith using the
BlobRegistry like 'respondWith(fetch(...))' or
'respondWith(new Response('body'))' on the service worker thread contain a
thread hop implicitly. This patch is to use mojo's data pipe instead of Stream
to avoid the thread hop.

Proof-of-Concept patch: https://crrev.com/2684533002

BUG=690795
==========

to

==========
ServiceWorker: Use mojo's data pipe for respondWith(stream)

Currently respondWith(fetch(...)) is using Stream provided by BlobRegistry.
BlobRegistry is living on the main thread and any respondWith using the
BlobRegistry like 'respondWith(fetch(...))' or
'respondWith(new Response('body'))' on the service worker thread contain a
thread hop implicitly. This patch is to use mojo's data pipe instead of Stream
to avoid the thread hop.

Proof-of-Concept patch: https://crrev.com/2684533002

BUG=690795

Review-Url: https://codereview.chromium.org/2703343002
Cr-Commit-Position: refs/heads/master@{#465909}
Committed:
https://chromium.googlesource.com/chromium/src/+/1ac68cf90fa046d3e9c1418aee7c...
==========

[commit-bot: I haz the power, 2017-04-20 06:04:04]

Committed patchset #25 (id:520001) as
https://chromium.googlesource.com/chromium/src/+/1ac68cf90fa046d3e9c1418aee7c...