service worker: Wait for request body blobs to finish construction

service worker: Wait for request body blobs to finish construction

Blob construction recently became async and we have to handle that.

Note to sheriff: there's a good chance the crash test added will be
flaky on debug builds. That's because of an existing crash
crbug.com/611317 which happens before this patch. In that event,
I'd rather we add the test to TestExpectations than revert the
patch.

BUG=608544
Committed: https://crrev.com/8e6cf517c56051b81e32c05dd2dddb024c93ffe2
Cr-Commit-Position: refs/heads/master@{#394241}

[falken, 2016-05-11 10:07:02]

falken@chromium.org changed reviewers:
+ dmurph@chromium.org, horo@chromium.org

[falken, 2016-05-11 10:07:03]

dmurph, horo PTAL

[dmurph, 2016-05-11 21:35:58]

https://codereview.chromium.org/1967683003/diff/1/content/browser/service_wor...
File content/browser/service_worker/service_worker_url_request_job.cc (right):

https://codereview.chromium.org/1967683003/diff/1/content/browser/service_wor...
content/browser/service_worker/service_worker_url_request_job.cc:899:
std::unique_ptr<storage::BlobDataHandle> handle =
You can check if the blob is building before calling that method, to remove
unnecessary callbacks.

handle->IsBeingBuilt(), I believe.

To keep the performance of the above counting, you could decrement the count
here if it's not building, and then at the end check if it's already 0.

You'll be guarenteed that the construction complete is called later, as long as
it IS being built.

[horo, 2016-05-12 06:13:37]

https://codereview.chromium.org/1967683003/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/request-body-blob-worker.js
(right):

https://codereview.chromium.org/1967683003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/request-body-blob-worker.js:6:
if (resp.indexOf("it's me the blob") == -1)
Nit: use single quotes

https://codereview.chromium.org/1967683003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/request-body-blob-worker.js:8:
if (resp.indexOf("and more blob!") == -1)
Nit: use single quotes

[falken, 2016-05-12 08:47:43]

Description was changed from

==========
service worker: Wait for request body blobs to finish construction

Blob construction recently became async and we have to handle that.

BUG=608544
==========

to

==========
service worker: Wait for request body blobs to finish construction

Blob construction recently became async and we have to handle that.

Note to sheriff: there's a good chance the crash test added will be flaky on
debug builds. That's because of an existing crash crbug.com/611317 which happens
before this patch. In that event, I'd rather we add the test to TestExpectations
than revert the patch.

BUG=608544
==========

[falken, 2016-05-12 08:48:01]

Description was changed from

==========
service worker: Wait for request body blobs to finish construction

Blob construction recently became async and we have to handle that.

Note to sheriff: there's a good chance the crash test added will be flaky on
debug builds. That's because of an existing crash crbug.com/611317 which happens
before this patch. In that event, I'd rather we add the test to TestExpectations
than revert the patch.

BUG=608544
==========

to

==========
service worker: Wait for request body blobs to finish construction

Blob construction recently became async and we have to handle that.

Note to sheriff: there's a good chance the crash test added will be
flaky on debug builds. That's because of an existing crash
crbug.com/611317 which happens before this patch. In that event,
I'd rather we add the test to TestExpectations than revert the
patch.

BUG=608544
==========

[falken, 2016-05-12 08:49:33]

PTAL.

I found a the crash test fails when run in isolation on the test runner, but it
doesn't happen when running all tests. I tracked the problem to
https://bugs.chromium.org/p/chromium/issues/detail?id=611317. It happens
independently of service worker and this patch, and I don't think it needs to
block this patch.

https://codereview.chromium.org/1967683003/diff/1/content/browser/service_wor...
File content/browser/service_worker/service_worker_url_request_job.cc (right):

https://codereview.chromium.org/1967683003/diff/1/content/browser/service_wor...
content/browser/service_worker/service_worker_url_request_job.cc:899:
std::unique_ptr<storage::BlobDataHandle> handle =
On 2016/05/11 21:35:58, dmurph wrote:
> You can check if the blob is building before calling that method, to remove
> unnecessary callbacks.
> 
> handle->IsBeingBuilt(), I believe.
> 
> To keep the performance of the above counting, you could decrement the count
> here if it's not building, and then at the end check if it's already 0.
> 
> You'll be guarenteed that the construction complete is called later, as long
as
> it IS being built.

Done.　I had considered that but was worried about the "Broken" case. I think the
new patch covers it.

https://codereview.chromium.org/1967683003/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/request-body-blob-worker.js
(right):

https://codereview.chromium.org/1967683003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/request-body-blob-worker.js:6:
if (resp.indexOf("it's me the blob") == -1)
On 2016/05/12 06:13:36, horo wrote:
> Nit: use single quotes

Done.

https://codereview.chromium.org/1967683003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/request-body-blob-worker.js:8:
if (resp.indexOf("and more blob!") == -1)
On 2016/05/12 06:13:36, horo wrote:
> Nit: use single quotes

Done.

[horo, 2016-05-16 02:03:53]

lgtm

[falken, 2016-05-16 04:16:37]

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

[falken, 2016-05-16 04:16:38]

Reviewers, please review:
dmurph: blob stuff
xunjieli: net/

[xunjieli, 2016-05-16 14:54:07]

xunjieli@chromium.org changed reviewers:
+ mmenke@chromium.org

[xunjieli, 2016-05-16 14:54:07]

net_log_event_type_list.h looks good to me. But I have never done review of that
particular file. +mmenke to do another pass on net_log_event_type_list.h.

[mmenke, 2016-05-16 15:30:02]

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

https://codereview.chromium.org/1967683003/diff/60001/content/browser/service...
content/browser/service_worker/service_worker_url_request_job.cc:117:
TYPE_SERVICE_WORKER_REQUEST_BODY_BLOB_CONSTRUCTION_STARTED);
Would it make sense to use BeginEvent / EndEvent with the same event name in the
constructor / destructor?  That will result in events nesting, and collapsing
the events (And adding a delta time) if there are no nesting events.

https://codereview.chromium.org/1967683003/diff/60001/net/log/net_log_event_t...
File net/log/net_log_event_type_list.h (right):

https://codereview.chromium.org/1967683003/diff/60001/net/log/net_log_event_t...
net/log/net_log_event_type_list.h:2037:
EVENT_TYPE(SERVICE_WORKER_ERROR_REQUEST_BODY_BLOB_FAILED)
The order these are in here should match the order in the CC file.

https://codereview.chromium.org/1967683003/diff/60001/net/log/net_log_event_t...
net/log/net_log_event_type_list.h:2046:
EVENT_TYPE(SERVICE_WORKER_REQUEST_BODY_BLOB_CONSTRUCTION_FINISHED)
Need to document the parameter.

[dmurph, 2016-05-16 21:08:18]

On 2016/05/16 at 15:30:02, mmenke wrote:
>
https://codereview.chromium.org/1967683003/diff/60001/content/browser/service...
> File content/browser/service_worker/service_worker_url_request_job.cc (right):
> 
>
https://codereview.chromium.org/1967683003/diff/60001/content/browser/service...
> content/browser/service_worker/service_worker_url_request_job.cc:117:
TYPE_SERVICE_WORKER_REQUEST_BODY_BLOB_CONSTRUCTION_STARTED);
> Would it make sense to use BeginEvent / EndEvent with the same event name in
the constructor / destructor?  That will result in events nesting, and
collapsing the events (And adding a delta time) if there are no nesting events.
> 
>
https://codereview.chromium.org/1967683003/diff/60001/net/log/net_log_event_t...
> File net/log/net_log_event_type_list.h (right):
> 
>
https://codereview.chromium.org/1967683003/diff/60001/net/log/net_log_event_t...
> net/log/net_log_event_type_list.h:2037:
EVENT_TYPE(SERVICE_WORKER_ERROR_REQUEST_BODY_BLOB_FAILED)
> The order these are in here should match the order in the CC file.
> 
>
https://codereview.chromium.org/1967683003/diff/60001/net/log/net_log_event_t...
> net/log/net_log_event_type_list.h:2046:
EVENT_TYPE(SERVICE_WORKER_REQUEST_BODY_BLOB_CONSTRUCTION_FINISHED)
> Need to document the parameter.

Blob interaction lgtm.

[falken, 2016-05-17 02:03:14]

mmenke: thank you. patch updated, PTAL

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

https://codereview.chromium.org/1967683003/diff/60001/content/browser/service...
content/browser/service_worker/service_worker_url_request_job.cc:117:
TYPE_SERVICE_WORKER_REQUEST_BODY_BLOB_CONSTRUCTION_STARTED);
On 2016/05/16 15:30:02, mmenke wrote:
> Would it make sense to use BeginEvent / EndEvent with the same event name in
the
> constructor / destructor?  That will result in events nesting, and collapsing
> the events (And adding a delta time) if there are no nesting events.

Done.

https://codereview.chromium.org/1967683003/diff/60001/net/log/net_log_event_t...
File net/log/net_log_event_type_list.h (right):

https://codereview.chromium.org/1967683003/diff/60001/net/log/net_log_event_t...
net/log/net_log_event_type_list.h:2037:
EVENT_TYPE(SERVICE_WORKER_ERROR_REQUEST_BODY_BLOB_FAILED)
On 2016/05/16 15:30:02, mmenke wrote:
> The order these are in here should match the order in the CC file.

Done.

https://codereview.chromium.org/1967683003/diff/60001/net/log/net_log_event_t...
net/log/net_log_event_type_list.h:2046:
EVENT_TYPE(SERVICE_WORKER_REQUEST_BODY_BLOB_CONSTRUCTION_FINISHED)
On 2016/05/16 15:30:02, mmenke wrote:
> Need to document the parameter.

Done.

[mmenke, 2016-05-17 15:33:57]

LGTM

[falken, 2016-05-17 22:05:58]

The CQ bit was checked by falken@chromium.org

[falken, 2016-05-17 22:05:58]

The patchset sent to the CQ was uploaded after l-g-t-m from horo@chromium.org,
dmurph@chromium.org
Link to the patchset: https://codereview.chromium.org/1967683003/#ps80001
(title: "review comments")

[commit-bot: I haz the power, 2016-05-17 22:07:12]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1967683003/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1967683003/80001

[commit-bot: I haz the power, 2016-05-17 22:14:56]

Description was changed from

==========
service worker: Wait for request body blobs to finish construction

Blob construction recently became async and we have to handle that.

Note to sheriff: there's a good chance the crash test added will be
flaky on debug builds. That's because of an existing crash
crbug.com/611317 which happens before this patch. In that event,
I'd rather we add the test to TestExpectations than revert the
patch.

BUG=608544
==========

to

==========
service worker: Wait for request body blobs to finish construction

Blob construction recently became async and we have to handle that.

Note to sheriff: there's a good chance the crash test added will be
flaky on debug builds. That's because of an existing crash
crbug.com/611317 which happens before this patch. In that event,
I'd rather we add the test to TestExpectations than revert the
patch.

BUG=608544
==========

[commit-bot: I haz the power, 2016-05-17 22:14:57]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-05-17 22:17:07]

Description was changed from

==========
service worker: Wait for request body blobs to finish construction

Blob construction recently became async and we have to handle that.

Note to sheriff: there's a good chance the crash test added will be
flaky on debug builds. That's because of an existing crash
crbug.com/611317 which happens before this patch. In that event,
I'd rather we add the test to TestExpectations than revert the
patch.

BUG=608544
==========

to

==========
service worker: Wait for request body blobs to finish construction

Blob construction recently became async and we have to handle that.

Note to sheriff: there's a good chance the crash test added will be
flaky on debug builds. That's because of an existing crash
crbug.com/611317 which happens before this patch. In that event,
I'd rather we add the test to TestExpectations than revert the
patch.

BUG=608544

Committed: https://crrev.com/8e6cf517c56051b81e32c05dd2dddb024c93ffe2
Cr-Commit-Position: refs/heads/master@{#394241}
==========

[commit-bot: I haz the power, 2016-05-17 22:17:12]

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