ServiceWorkerCache: Support response body in blob form.

ServiceWorkerCache: Support response body in blob form.

Downstream of Blink CL: https://codereview.chromium.org/623753002

BUG=419417
Committed: https://crrev.com/ab4f77a47853afc2df5debbb0c7ebc50ba0203a9
Cr-Commit-Position: refs/heads/master@{#299110}

[jkarlin, 2014-10-03 17:19:26]

jkarlin@chromium.org changed reviewers:
+ michaeln@chromium.org, tsepez@chromium.org

[jkarlin, 2014-10-03 17:19:27]

michaeln@chromium.org: Please review changes in everything

tsepez@chromium.org: Please review changes in service_worker_messages.h


Thanks!

[jkarlin, 2014-10-03 17:21:59]

Note that this passes some trivial layout tests that I put together showing that
cache.match and cache.put do return the appropriate response bodies.  I didn't
include them in this CL as https://codereview.chromium.org/425413002/ should
cover it.

[jkarlin, 2014-10-03 17:23:26]

On 2014/10/03 17:21:59, jkarlin wrote:
> Note that this passes some trivial layout tests that I put together showing
that
> cache.match and cache.put do return the appropriate response bodies.  I didn't
> include them in this CL as https://codereview.chromium.org/425413002/ should
> cover it.

Er, layout tests wouldn't be in a browser-side CL anyway I suppose :)  

It'd also be good to have service_worker_cache_listener_unittests in the near
future though that should likely be in its own CL as it'll take a little
reworking of the listener class.

[Tom Sepez, 2014-10-03 17:27:22]

My only concern is where does |blob_size| come from?  Is it trustworthy (e.g.
set by browser and only used in SWs)?  What do we do with in other parts of the
code (e.g. memcpy based upon it would be bad if it is not trustworthy)?

[Tom Sepez, 2014-10-03 17:45:21]

https://codereview.chromium.org/622833002/diff/20001/content/renderer/service...
File content/renderer/service_worker/service_worker_cache_storage_dispatcher.cc
(right):

https://codereview.chromium.org/622833002/diff/20001/content/renderer/service...
content/renderer/service_worker/service_worker_cache_storage_dispatcher.cc:556:
for (ServiceWorkerHeaderMap::const_iterator i = response.headers.begin(),
nit: this might be a place for one of those newfangled loops:

  for (const auto& i : response.headers) { 
    web_response->setHeader(base::ASCIIToUTF16(i.first),
                            base::ASCIIToUTF16(i.second));
  }

[jkarlin, 2014-10-03 18:09:12]

On 2014/10/03 17:27:22, Tom Sepez wrote:
> My only concern is where does |blob_size| come from?  Is it trustworthy (e.g.
> set by browser and only used in SWs)?  What do we do with in other parts of
the
> code (e.g. memcpy based upon it would be bad if it is not trustworthy)?

Good question.  The blob size is trustworthy.  It originates in Response.cpp
where the length of the input (from javascript) string/array/blob are computed.

[Tom Sepez, 2014-10-03 18:11:38]

> Good question.  The blob size is trustworthy.  It originates in Response.cpp
> where the length of the input (from javascript) string/array/blob are
computed.
LGTM. Thanks for the explanation.

[jkarlin, 2014-10-03 18:25:50]

https://codereview.chromium.org/622833002/diff/20001/content/renderer/service...
File content/renderer/service_worker/service_worker_cache_storage_dispatcher.cc
(right):

https://codereview.chromium.org/622833002/diff/20001/content/renderer/service...
content/renderer/service_worker/service_worker_cache_storage_dispatcher.cc:556:
for (ServiceWorkerHeaderMap::const_iterator i = response.headers.begin(),
On 2014/10/03 17:45:21, Tom Sepez wrote:
> nit: this might be a place for one of those newfangled loops:
> 
>   for (const auto& i : response.headers) { 
>     web_response->setHeader(base::ASCIIToUTF16(i.first),
>                             base::ASCIIToUTF16(i.second));
>   }

Done. That felt good :)

[jsbell, 2014-10-03 21:35:42]

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

[jsbell, 2014-10-03 21:35:43]

https://codereview.chromium.org/622833002/diff/40001/content/browser/service_...
File content/browser/service_worker/service_worker_cache_listener.h (right):

https://codereview.chromium.org/622833002/diff/40001/content/browser/service_...
content/browser/service_worker/service_worker_cache_listener.h:68: typedef
std::map<std::string, std::list<storage::BlobDataHandle> >
Nit: You can use >> now!

[jsbell, 2014-10-03 21:41:32]

lgtm but I'm not an OWNER

[jkarlin, 2014-10-03 21:48:58]

https://codereview.chromium.org/622833002/diff/40001/content/browser/service_...
File content/browser/service_worker/service_worker_cache_listener.h (right):

https://codereview.chromium.org/622833002/diff/40001/content/browser/service_...
content/browser/service_worker/service_worker_cache_listener.h:68: typedef
std::map<std::string, std::list<storage::BlobDataHandle> >
On 2014/10/03 21:35:42, jsbell wrote:
> Nit: You can use >> now!

Done. Oh frabjous day!

[jkarlin, 2014-10-06 00:09:25]

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

[jkarlin, 2014-10-06 00:09:43]

+r horo: Please review all.  Thanks!

[horo, 2014-10-06 02:53:51]

lgtm

[michaeln, 2014-10-06 21:01:33]

lgtm

https://codereview.chromium.org/622833002/diff/80001/content/browser/service_...
File content/browser/service_worker/service_worker_cache_listener.cc (right):

https://codereview.chromium.org/622833002/diff/80001/content/browser/service_...
content/browser/service_worker/service_worker_cache_listener.cc:292: void
ServiceWorkerCacheListener::OnBlobDataHandled(const base::string16& uuid) {
std::string?

https://codereview.chromium.org/622833002/diff/80001/content/common/service_w...
File content/common/service_worker/service_worker_messages.h (right):

https://codereview.chromium.org/622833002/diff/80001/content/common/service_w...
content/common/service_worker/service_worker_messages.h:239: base::string16 /*
uuid */)
std::string?

https://codereview.chromium.org/622833002/diff/80001/content/renderer/service...
File content/renderer/service_worker/service_worker_cache_storage_dispatcher.cc
(right):

https://codereview.chromium.org/622833002/diff/80001/content/renderer/service...
content/renderer/service_worker/service_worker_cache_storage_dispatcher.cc:539:
script_context_->GetRoutingID(), base::ASCIIToUTF16(uuid)));
can this be std::string?

https://codereview.chromium.org/622833002/diff/80001/content/renderer/service...
File content/renderer/service_worker/service_worker_cache_storage_dispatcher.h
(right):

https://codereview.chromium.org/622833002/diff/80001/content/renderer/service...
content/renderer/service_worker/service_worker_cache_storage_dispatcher.h:113:
void dispatchBlobDataHandled(const std::string& uuid);
i think the only callsite for this method is in PopulateWebResponseFromResponse,
if thats right can this method also be private or maybe even integrated directly
into PopulateWebResponseFromResponse

[jkarlin, 2014-10-07 12:48:17]

https://codereview.chromium.org/622833002/diff/80001/content/browser/service_...
File content/browser/service_worker/service_worker_cache_listener.cc (right):

https://codereview.chromium.org/622833002/diff/80001/content/browser/service_...
content/browser/service_worker/service_worker_cache_listener.cc:292: void
ServiceWorkerCacheListener::OnBlobDataHandled(const base::string16& uuid) {
On 2014/10/06 21:01:33, michaeln wrote:
> std::string?

I agree that we should be sending strings instead of string16s over IPC but that
isn't the convention in service_worker_messages.h.  I went with the convention.

https://codereview.chromium.org/622833002/diff/80001/content/common/service_w...
File content/common/service_worker/service_worker_messages.h (right):

https://codereview.chromium.org/622833002/diff/80001/content/common/service_w...
content/common/service_worker/service_worker_messages.h:239: base::string16 /*
uuid */)
On 2014/10/06 21:01:33, michaeln wrote:
> std::string?

I agree that we should be sending strings instead of string16s over IPC but that
isn't the convention in service_worker_messages.h.  I went with the convention.

https://codereview.chromium.org/622833002/diff/80001/content/renderer/service...
File content/renderer/service_worker/service_worker_cache_storage_dispatcher.cc
(right):

https://codereview.chromium.org/622833002/diff/80001/content/renderer/service...
content/renderer/service_worker/service_worker_cache_storage_dispatcher.cc:539:
script_context_->GetRoutingID(), base::ASCIIToUTF16(uuid)));
On 2014/10/06 21:01:33, michaeln wrote:
> can this be std::string?

I agree that we should be sending strings instead of string16s over IPC but that
isn't the convention in service_worker_messages.h.  I went with the convention.

https://codereview.chromium.org/622833002/diff/80001/content/renderer/service...
File content/renderer/service_worker/service_worker_cache_storage_dispatcher.h
(right):

https://codereview.chromium.org/622833002/diff/80001/content/renderer/service...
content/renderer/service_worker/service_worker_cache_storage_dispatcher.h:113:
void dispatchBlobDataHandled(const std::string& uuid);
On 2014/10/06 21:01:33, michaeln wrote:
> i think the only callsite for this method is in
PopulateWebResponseFromResponse,
> if thats right can this method also be private or maybe even integrated
directly
> into PopulateWebResponseFromResponse

Done.

[michaeln, 2014-10-07 21:29:57]

https://codereview.chromium.org/622833002/diff/80001/content/renderer/service...
File content/renderer/service_worker/service_worker_cache_storage_dispatcher.cc
(right):

https://codereview.chromium.org/622833002/diff/80001/content/renderer/service...
content/renderer/service_worker/service_worker_cache_storage_dispatcher.cc:539:
script_context_->GetRoutingID(), base::ASCIIToUTF16(uuid)));
On 2014/10/07 12:48:17, jkarlin wrote:
> On 2014/10/06 21:01:33, michaeln wrote:
> > can this be std::string?
> 
> I agree that we should be sending strings instead of string16s over IPC but
that
> isn't the convention in service_worker_messages.h.  I went with the
convention.

nit: oh well, cargo cult wins?

In this case i think you're converting to/from on both sides of the wire which
is kind of silly. For other blob related messages std::string is pretty
consistently used.

[jkarlin, 2014-10-07 21:59:58]

On 2014/10/07 21:29:57, michaeln wrote:
>
https://codereview.chromium.org/622833002/diff/80001/content/renderer/service...
> File
content/renderer/service_worker/service_worker_cache_storage_dispatcher.cc
> (right):
> 
>
https://codereview.chromium.org/622833002/diff/80001/content/renderer/service...
>
content/renderer/service_worker/service_worker_cache_storage_dispatcher.cc:539:
> script_context_->GetRoutingID(), base::ASCIIToUTF16(uuid)));
> On 2014/10/07 12:48:17, jkarlin wrote:
> > On 2014/10/06 21:01:33, michaeln wrote:
> > > can this be std::string?
> > 
> > I agree that we should be sending strings instead of string16s over IPC but
> that
> > isn't the convention in service_worker_messages.h.  I went with the
> convention.
> 
> nit: oh well, cargo cult wins?
> 
> In this case i think you're converting to/from on both sides of the wire which
> is kind of silly. For other blob related messages std::string is pretty
> consistently used.

Excellent point.  Certainly enough to make me convert to std::string :)  Done.

[jkarlin, 2014-10-10 13:46:57]

The CQ bit was checked by jkarlin@chromium.org

[commit-bot: I haz the power, 2014-10-10 13:48:03]

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

[commit-bot: I haz the power, 2014-10-10 14:08:20]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2014-10-10 14:09:16]

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