[ServiceWorker] Make Stream support in ServiceWorkerURLRequestJob

[ServiceWorker] Make Stream support in ServiceWorkerURLRequestJob

In ServiceWorkerURLRequestJob::DidDispatchFetchEvent(), if response.stream_url of ServiceWorkerHostMsg_FetchEventFinished is set, ServiceWorkerURLRequestJob starts reading the body of the response from the stream instead of the blob.

But the stream may not be registered yet.
It is because ServiceWorkerHostMsg_FetchEventFinished is sent from the worker thread but StreamHostMsg_StartBuilding which triggers the stream registration is sent from the main thread of the ServiceWorker process.
So if ServiceWorkerURLRequestJob can't get the stream in DidDispatchFetchEvent(), call StreamRegistry::SetRegisterObserver() to receive the stream registration event.

When ServiceWorkerURLRequestJob successfuly gets the stream, it starts reading the stream.
The codes for reading the stream is almost same as the codes in StreamURLRequestJob.

This change depends on these changes:
  https://codereview.chromium.org/760823002
  https://codereview.chromium.org/759823003

BUG=436424
Committed: https://crrev.com/d556043ea410430fc84a555d4018265025dfc194
Cr-Commit-Position: refs/heads/master@{#308059}

[horo, 2014-11-26 07:52:59]

Patchset #1 (id:1) has been deleted

[horo, 2014-11-26 07:53:04]

Patchset #1 (id:20001) has been deleted

[horo, 2014-11-26 07:53:10]

Patchset #1 (id:40001) has been deleted

[horo, 2014-11-26 08:25:37]

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

[horo, 2014-11-26 08:25:37]

nhiroki@
Could you please review this?
Thank you.

[horo, 2014-11-26 08:26:45]

Patchset #2 (id:80001) has been deleted

[horo, 2014-11-26 08:26:54]

Patchset #1 (id:60001) has been deleted

[nhiroki, 2014-11-27 05:03:26]

Partially reviewed. I'm not really familiar with this domain, so I'd prefer you
to find one more reviewer who is an expert on net/stream.

https://codereview.chromium.org/759203002/diff/100001/content/browser/service...
File content/browser/service_worker/service_worker_url_request_job.cc (right):

https://codereview.chromium.org/759203002/diff/100001/content/browser/service...
content/browser/service_worker/service_worker_url_request_job.cc:154:
net::IOBuffer* buf, int buf_size, int *bytes_read) {
DCHECK_GE(buf_size, 0)?

https://codereview.chromium.org/759203002/diff/100001/content/browser/service...
content/browser/service_worker/service_worker_url_request_job.cc:157:
DCHECK(bytes_read);
Can you move these checks into the beginning of this function because those are
also used at line 193.

https://codereview.chromium.org/759203002/diff/100001/content/browser/service...
content/browser/service_worker/service_worker_url_request_job.cc:163: if
(to_read <= 0) {
"to_read < 0" seems a bug because...

1) |buf_size| should be >= 0, and
2) |stream_max_range_| >= |stream_total_bytes_read_|

How about having DCHECK_GE(to_read, 0) and change the condition of the
if-statement like "if (!to_read)"?

https://codereview.chromium.org/759203002/diff/100001/content/browser/service...
content/browser/service_worker/service_worker_url_request_job.cc:170: case
Stream::STREAM_HAS_DATA:
"DCHECK_GT(bytes_read, 0)" like line 276?

https://codereview.chromium.org/759203002/diff/100001/content/browser/service...
content/browser/service_worker/service_worker_url_request_job.cc:281: bytes_read
= 0;
This looks odd... at line 298 |stream_total_bytes_read_| never reaches the
actual total bytes because the last byte size is always 0.

How about using another variable for the signal?

https://codereview.chromium.org/759203002/diff/100001/content/browser/service...
content/browser/service_worker/service_worker_url_request_job.cc:295:
stream_pending_buffer_ = NULL;
nullptr?

https://codereview.chromium.org/759203002/diff/100001/content/browser/service...
File content/browser/service_worker/service_worker_url_request_job.h (right):

https://codereview.chromium.org/759203002/diff/100001/content/browser/service...
content/browser/service_worker/service_worker_url_request_job.h:98: //
StreamObserver method.
nit: method -> override? (Other places in this file use 'overrides')

https://codereview.chromium.org/759203002/diff/100001/content/browser/service...
content/browser/service_worker/service_worker_url_request_job.h:101: //
StreamRegisterObserver method.
ditto.

https://codereview.chromium.org/759203002/diff/100001/content/browser/service...
File content/browser/service_worker/service_worker_url_request_job_unittest.cc
(right):

https://codereview.chromium.org/759203002/diff/100001/content/browser/service...
content/browser/service_worker/service_worker_url_request_job_unittest.cc:305:
expected_response += kTestData;
This (and others) could cause a bunch of memory allocations... can you call
reserve() in advance?

https://codereview.chromium.org/759203002/diff/100001/content/browser/service...
content/browser/service_worker/service_worker_url_request_job_unittest.cc:313:
NULL);
NULL -> nullptr (here and somewhere else in this file)

https://codereview.chromium.org/759203002/diff/100001/content/browser/service...
content/browser/service_worker/service_worker_url_request_job_unittest.cc:458:
ASSERT_NE(nullptr, stream_context->registry()->GetStream(stream_url).get());
ASSERT_TRUE

https://codereview.chromium.org/759203002/diff/100001/content/browser/service...
content/browser/service_worker/service_worker_url_request_job_unittest.cc:460:
ASSERT_EQ(nullptr, stream_context->registry()->GetStream(stream_url).get());
ASSERT_FALSE

https://codereview.chromium.org/759203002/diff/100001/content/browser/service...
content/browser/service_worker/service_worker_url_request_job_unittest.cc:490:
// The stream is not registered to the stream registry.
nit: "should not be registered" seems to be able to clearer express your
intention than "is not registered".

https://codereview.chromium.org/759203002/diff/100001/content/browser/service...
content/browser/service_worker/service_worker_url_request_job_unittest.cc:491:
ASSERT_NE(nullptr, stream_context->registry()->GetStream(stream_url).get());
ASSERT_TRUE

[horo, 2014-11-27 06:33:54]

Patchset #2 (id:120001) has been deleted

[horo, 2014-11-27 06:51:20]

Patchset #3 (id:160001) has been deleted

[horo, 2014-11-27 06:52:51]

Thank you!

https://codereview.chromium.org/759203002/diff/100001/content/browser/service...
File content/browser/service_worker/service_worker_url_request_job.cc (right):

https://codereview.chromium.org/759203002/diff/100001/content/browser/service...
content/browser/service_worker/service_worker_url_request_job.cc:154:
net::IOBuffer* buf, int buf_size, int *bytes_read) {
On 2014/11/27 05:03:25, nhiroki wrote:
> DCHECK_GE(buf_size, 0)?

Done.

https://codereview.chromium.org/759203002/diff/100001/content/browser/service...
content/browser/service_worker/service_worker_url_request_job.cc:157:
DCHECK(bytes_read);
On 2014/11/27 05:03:25, nhiroki wrote:
> Can you move these checks into the beginning of this function because those
are
> also used at line 193.

Done.

https://codereview.chromium.org/759203002/diff/100001/content/browser/service...
content/browser/service_worker/service_worker_url_request_job.cc:163: if
(to_read <= 0) {
On 2014/11/27 05:03:25, nhiroki wrote:
> "to_read < 0" seems a bug because...
> 
> 1) |buf_size| should be >= 0, and
> 2) |stream_max_range_| >= |stream_total_bytes_read_|
> 
> How about having DCHECK_GE(to_read, 0) and change the condition of the
> if-statement like "if (!to_read)"?

We don't need stream_total_bytes_read_ nor stream_max_range_.
Removed.

https://codereview.chromium.org/759203002/diff/100001/content/browser/service...
content/browser/service_worker/service_worker_url_request_job.cc:170: case
Stream::STREAM_HAS_DATA:
On 2014/11/27 05:03:25, nhiroki wrote:
> "DCHECK_GT(bytes_read, 0)" like line 276?

Done.

https://codereview.chromium.org/759203002/diff/100001/content/browser/service...
content/browser/service_worker/service_worker_url_request_job.cc:281: bytes_read
= 0;
On 2014/11/27 05:03:25, nhiroki wrote:
> This looks odd... at line 298 |stream_total_bytes_read_| never reaches the
> actual total bytes because the last byte size is always 0.
> 
> How about using another variable for the signal?

I think checking bytes_read == 0 is enough.

https://codereview.chromium.org/759203002/diff/100001/content/browser/service...
content/browser/service_worker/service_worker_url_request_job.cc:295:
stream_pending_buffer_ = NULL;
On 2014/11/27 05:03:25, nhiroki wrote:
> nullptr?

Done.

https://codereview.chromium.org/759203002/diff/100001/content/browser/service...
File content/browser/service_worker/service_worker_url_request_job.h (right):

https://codereview.chromium.org/759203002/diff/100001/content/browser/service...
content/browser/service_worker/service_worker_url_request_job.h:98: //
StreamObserver method.
On 2014/11/27 05:03:25, nhiroki wrote:
> nit: method -> override? (Other places in this file use 'overrides')

Done.

https://codereview.chromium.org/759203002/diff/100001/content/browser/service...
content/browser/service_worker/service_worker_url_request_job.h:101: //
StreamRegisterObserver method.
On 2014/11/27 05:03:25, nhiroki wrote:
> ditto.

Done.

https://codereview.chromium.org/759203002/diff/100001/content/browser/service...
File content/browser/service_worker/service_worker_url_request_job_unittest.cc
(right):

https://codereview.chromium.org/759203002/diff/100001/content/browser/service...
content/browser/service_worker/service_worker_url_request_job_unittest.cc:305:
expected_response += kTestData;
On 2014/11/27 05:03:26, nhiroki wrote:
> This (and others) could cause a bunch of memory allocations... can you call
> reserve() in advance?

Done.

https://codereview.chromium.org/759203002/diff/100001/content/browser/service...
content/browser/service_worker/service_worker_url_request_job_unittest.cc:313:
NULL);
On 2014/11/27 05:03:26, nhiroki wrote:
> NULL -> nullptr (here and somewhere else in this file)

Done.

https://codereview.chromium.org/759203002/diff/100001/content/browser/service...
content/browser/service_worker/service_worker_url_request_job_unittest.cc:458:
ASSERT_NE(nullptr, stream_context->registry()->GetStream(stream_url).get());
On 2014/11/27 05:03:25, nhiroki wrote:
> ASSERT_TRUE

Done.

https://codereview.chromium.org/759203002/diff/100001/content/browser/service...
content/browser/service_worker/service_worker_url_request_job_unittest.cc:460:
ASSERT_EQ(nullptr, stream_context->registry()->GetStream(stream_url).get());
On 2014/11/27 05:03:26, nhiroki wrote:
> ASSERT_FALSE

Done.

https://codereview.chromium.org/759203002/diff/100001/content/browser/service...
content/browser/service_worker/service_worker_url_request_job_unittest.cc:490:
// The stream is not registered to the stream registry.
On 2014/11/27 05:03:26, nhiroki wrote:
> nit: "should not be registered" seems to be able to clearer express your
> intention than "is not registered".

Done.

https://codereview.chromium.org/759203002/diff/100001/content/browser/service...
content/browser/service_worker/service_worker_url_request_job_unittest.cc:491:
ASSERT_NE(nullptr, stream_context->registry()->GetStream(stream_url).get());
On 2014/11/27 05:03:26, nhiroki wrote:
> ASSERT_TRUE

Done.

[horo, 2014-11-28 09:28:04]

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

[horo, 2014-11-28 09:28:04]

tyoshino@
Could you please review this?

Thank you!

[horo, 2014-12-02 08:26:42]

Patchset #6 (id:240001) has been deleted

[tyoshino (SeeGerritForStatus), 2014-12-03 08:13:33]

https://codereview.chromium.org/759203002/diff/220001/content/browser/service...
File content/browser/service_worker/service_worker_url_request_job.cc (right):

https://codereview.chromium.org/759203002/diff/220001/content/browser/service...
content/browser/service_worker/service_worker_url_request_job.cc:155:
DCHECK(bytes_read);
can we assert that waiting_stream_url_ is empty here?

https://codereview.chromium.org/759203002/diff/220001/content/browser/service...
content/browser/service_worker/service_worker_url_request_job.cc:175: }
insert a blank line here

L176-L179 should be grouped with L181-186.

https://codereview.chromium.org/759203002/diff/220001/content/browser/service...
content/browser/service_worker/service_worker_url_request_job.cc:252: // Do
nothing if pending_buffer_ is empty, i.e. there's no ReadRawData()
stream_pending_buffer_

[nhiroki, 2014-12-03 08:15:08]

LGTM

https://codereview.chromium.org/759203002/diff/220001/content/browser/service...
File content/browser/service_worker/service_worker_url_request_job.cc (right):

https://codereview.chromium.org/759203002/diff/220001/content/browser/service...
content/browser/service_worker/service_worker_url_request_job.cc:86: if
(stream_.get()) {
FYI: Boolean testing of scoped_refptr has been re-enabled. You don't need to
call '.get()' anymore.
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/iXtlGO6j4Kg/UxN3p...

https://codereview.chromium.org/759203002/diff/220001/content/browser/service...
content/browser/service_worker/service_worker_url_request_job.cc:159:
DCHECK_GT(*bytes_read, 0);
You might want to add "return" here and "DCHECK(!bytes_read);" at line 161.

https://codereview.chromium.org/759203002/diff/220001/content/browser/service...
File content/browser/service_worker/service_worker_url_request_job_unittest.cc
(right):

https://codereview.chromium.org/759203002/diff/220001/content/browser/service...
content/browser/service_worker/service_worker_url_request_job_unittest.cc:510: 
nit: extra blank line

[horo, 2014-12-05 05:51:25]

Patchset #6 (id:260001) has been deleted

[horo, 2014-12-05 05:51:54]

https://codereview.chromium.org/759203002/diff/220001/content/browser/service...
File content/browser/service_worker/service_worker_url_request_job.cc (right):

https://codereview.chromium.org/759203002/diff/220001/content/browser/service...
content/browser/service_worker/service_worker_url_request_job.cc:86: if
(stream_.get()) {
On 2014/12/03 08:15:08, nhiroki wrote:
> FYI: Boolean testing of scoped_refptr has been re-enabled. You don't need to
> call '.get()' anymore.
>
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/iXtlGO6j4Kg/UxN3p...

Done.

https://codereview.chromium.org/759203002/diff/220001/content/browser/service...
content/browser/service_worker/service_worker_url_request_job.cc:155:
DCHECK(bytes_read);
On 2014/12/03 08:13:33, tyoshino wrote:
> can we assert that waiting_stream_url_ is empty here?

Done.

https://codereview.chromium.org/759203002/diff/220001/content/browser/service...
content/browser/service_worker/service_worker_url_request_job.cc:159:
DCHECK_GT(*bytes_read, 0);
On 2014/12/03 08:15:07, nhiroki wrote:
> You might want to add "return" here and "DCHECK(!bytes_read);" at line 161.

Done.

https://codereview.chromium.org/759203002/diff/220001/content/browser/service...
content/browser/service_worker/service_worker_url_request_job.cc:175: }
On 2014/12/03 08:13:33, tyoshino wrote:
> insert a blank line here
> 
> L176-L179 should be grouped with L181-186.

Done.

https://codereview.chromium.org/759203002/diff/220001/content/browser/service...
content/browser/service_worker/service_worker_url_request_job.cc:252: // Do
nothing if pending_buffer_ is empty, i.e. there's no ReadRawData()
On 2014/12/03 08:13:33, tyoshino wrote:
> stream_pending_buffer_

Done.

https://codereview.chromium.org/759203002/diff/220001/content/browser/service...
File content/browser/service_worker/service_worker_url_request_job_unittest.cc
(right):

https://codereview.chromium.org/759203002/diff/220001/content/browser/service...
content/browser/service_worker/service_worker_url_request_job_unittest.cc:510: 
On 2014/12/03 08:15:08, nhiroki wrote:
> nit: extra blank line

Done.

[tyoshino (SeeGerritForStatus), 2014-12-08 09:31:57]

CL description:
suport -> support

[tyoshino (SeeGerritForStatus), 2014-12-08 09:36:56]

almost lg

https://codereview.chromium.org/759203002/diff/280001/content/browser/service...
File content/browser/service_worker/service_worker_url_request_job.cc (right):

https://codereview.chromium.org/759203002/diff/280001/content/browser/service...
content/browser/service_worker/service_worker_url_request_job.cc:270: // Calling
NotifyReadComplete call with 0 signals completion.
s/call//

https://codereview.chromium.org/759203002/diff/280001/content/browser/service...
content/browser/service_worker/service_worker_url_request_job.cc:547: // Wait
for StreamHostMsg_StartBuilding message from the ServieWorker.
Servie -> Service

https://codereview.chromium.org/759203002/diff/280001/content/browser/service...
File content/browser/service_worker/service_worker_url_request_job.h (right):

https://codereview.chromium.org/759203002/diff/280001/content/browser/service...
content/browser/service_worker/service_worker_url_request_job.h:10: 
ref_counted.h

https://codereview.chromium.org/759203002/diff/280001/content/browser/service...
content/browser/service_worker/service_worker_url_request_job.h:24: #include
"third_party/WebKit/public/platform/WebServiceWorkerResponseType.h"
gurl.h

https://codereview.chromium.org/759203002/diff/280001/content/browser/service...
content/browser/service_worker/service_worker_url_request_job.h:183:
scoped_refptr<net::IOBuffer> stream_pending_buffer_;
declare IOBuffer

https://codereview.chromium.org/759203002/diff/280001/content/browser/service...
File content/browser/service_worker/service_worker_url_request_job_unittest.cc
(right):

https://codereview.chromium.org/759203002/diff/280001/content/browser/service...
content/browser/service_worker/service_worker_url_request_job_unittest.cc:421:
base::RunLoop().RunUntilIdle();
what's the purpose of this RunUntilIdle() call?

https://codereview.chromium.org/759203002/diff/280001/content/browser/service...
content/browser/service_worker/service_worker_url_request_job_unittest.cc:429:
stream->Finalize();
as the stream is Finalize()-ed here, this unittest is only checking that Flush()
is not broken?

[horo, 2014-12-09 03:45:04]

Patchset #7 (id:300001) has been deleted

[horo, 2014-12-09 03:47:55]

Patchset #8 (id:340001) has been deleted

[horo, 2014-12-09 03:48:03]

Patchset #7 (id:320001) has been deleted

[horo, 2014-12-09 04:23:56]

https://codereview.chromium.org/759203002/diff/280001/content/browser/service...
File content/browser/service_worker/service_worker_url_request_job.cc (right):

https://codereview.chromium.org/759203002/diff/280001/content/browser/service...
content/browser/service_worker/service_worker_url_request_job.cc:270: // Calling
NotifyReadComplete call with 0 signals completion.
On 2014/12/08 09:36:56, tyoshino wrote:
> s/call//

Done.

https://codereview.chromium.org/759203002/diff/280001/content/browser/service...
content/browser/service_worker/service_worker_url_request_job.cc:547: // Wait
for StreamHostMsg_StartBuilding message from the ServieWorker.
On 2014/12/08 09:36:56, tyoshino wrote:
> Servie -> Service

Done.

https://codereview.chromium.org/759203002/diff/280001/content/browser/service...
File content/browser/service_worker/service_worker_url_request_job.h (right):

https://codereview.chromium.org/759203002/diff/280001/content/browser/service...
content/browser/service_worker/service_worker_url_request_job.h:10: 
On 2014/12/08 09:36:56, tyoshino wrote:
> ref_counted.h

Done.

https://codereview.chromium.org/759203002/diff/280001/content/browser/service...
content/browser/service_worker/service_worker_url_request_job.h:24: #include
"third_party/WebKit/public/platform/WebServiceWorkerResponseType.h"
On 2014/12/08 09:36:56, tyoshino wrote:
> gurl.h

Done.

https://codereview.chromium.org/759203002/diff/280001/content/browser/service...
content/browser/service_worker/service_worker_url_request_job.h:183:
scoped_refptr<net::IOBuffer> stream_pending_buffer_;
On 2014/12/08 09:36:56, tyoshino wrote:
> declare IOBuffer

Done.

https://codereview.chromium.org/759203002/diff/280001/content/browser/service...
File content/browser/service_worker/service_worker_url_request_job_unittest.cc
(right):

https://codereview.chromium.org/759203002/diff/280001/content/browser/service...
content/browser/service_worker/service_worker_url_request_job_unittest.cc:421:
base::RunLoop().RunUntilIdle();
On 2014/12/08 09:36:56, tyoshino wrote:
> what's the purpose of this RunUntilIdle() call?

We don't need this.
Removed.

https://codereview.chromium.org/759203002/diff/280001/content/browser/service...
content/browser/service_worker/service_worker_url_request_job_unittest.cc:429:
stream->Finalize();
On 2014/12/08 09:36:56, tyoshino wrote:
> as the stream is Finalize()-ed here, this unittest is only checking that
Flush()
> is not broken?

Added EXPECT_EQ(expected_response, url_request_delegate_.response_data());

[tyoshino (SeeGerritForStatus), 2014-12-09 05:41:33]

lgtm!

[horo, 2014-12-09 05:49:16]

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

[horo, 2014-12-09 05:49:17]

jam@
Could you please review base/supports_user_data.h and
content/browser/resource_context_impl.*?

[jam, 2014-12-09 17:15:23]

lgtm

[horo, 2014-12-12 04:56:15]

The CQ bit was checked by horo@chromium.org

[commit-bot: I haz the power, 2014-12-12 04:56:49]

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

[commit-bot: I haz the power, 2014-12-12 06:20:20]

Committed patchset #8 (id:380001)

[commit-bot: I haz the power, 2014-12-12 06:21:08]

Patchset 8 (id:??) landed as
https://crrev.com/d556043ea410430fc84a555d4018265025dfc194
Cr-Commit-Position: refs/heads/master@{#308059}