Let the client (browser) set blobs in WebServiceWorkerResponse.

Let the client (browser) set blobs in WebServiceWorkerResponse.

This lets ServiceWorkerCache return a response to Blink with a blob for the body.

This CL is a dependency of: https://codereview.chromium.org/622833002/

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

[jkarlin, 2014-10-03 16:45:08]

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

[jkarlin, 2014-10-03 16:45:09]

horo@chromium.org: Please review changes in all

jsbell@chromium.org: Please review changes in all

Thanks!

[jkarlin, 2014-10-03 16:46:22]

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

[jkarlin, 2014-10-03 16:46:23]

jamesr@chromium.org: Please review changes outside of modules/serviceworker/ for
OWNERS.  Thanks!

[jsbell, 2014-10-03 21:40:45]

lgtm

[horo, 2014-10-06 02:16:01]

lgtm

[jkarlin, 2014-10-06 11:16:39]

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

[jkarlin, 2014-10-06 11:17:10]

+r hochen: ptal at platform/ directories for OWNERS.  Thanks!

[jkarlin, 2014-10-06 11:18:11]

On 2014/10/06 11:17:10, jkarlin wrote:
> +r hochen: ptal at platform/ directories for OWNERS.  Thanks!

Sorry, fat fingers.  jochen@, ptal...

[jkarlin, 2014-10-07 02:17:40]

jochen or jamesr, PTAL for owners.  Many thanks!

[jamesr, 2014-10-07 04:33:13]

https://codereview.chromium.org/623753002/diff/20001/public/platform/WebServi...
File public/platform/WebServiceWorkerResponse.h (right):

https://codereview.chromium.org/623753002/diff/20001/public/platform/WebServi...
public/platform/WebServiceWorkerResponse.h:69: long long blobSize() const;
i don't know much about blobs but if this is supposed to be a number of bytes or
number of elements in an array the type must be size_t

[jkarlin, 2014-10-07 09:34:35]

https://codereview.chromium.org/623753002/diff/20001/public/platform/WebServi...
File public/platform/WebServiceWorkerResponse.h (right):

https://codereview.chromium.org/623753002/diff/20001/public/platform/WebServi...
public/platform/WebServiceWorkerResponse.h:69: long long blobSize() const;
On 2014/10/07 04:33:13, jamesr wrote:
> i don't know much about blobs but if this is supposed to be a number of bytes
or
> number of elements in an array the type must be size_t

Number of bytes.  Blobs can refer to files and they take long long sizes and
offsets.  See
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

[jamesr, 2014-10-07 19:37:34]

(1) That type is different from the one you used since it's unsigned

(2) The public API must use size_t uniformly.  If you find yourself interfacing
with some code that doesn't use size_t for a count of bytes, fix that.

not lgtm

[michaeln1, 2014-10-07 21:08:43]

> (2) The public API must use size_t uniformly.

A quick search finds many usages of the long long in the public\web and
public\platform api for a byte count. I think other parts of the WebKit api
related specifically to blob sizes consistently use 'long long'. From the point
of view on maintaining consistency with regard to the blob data type, long long
may actually be the best choice.

[jamesr, 2014-10-07 21:15:06]

Our type guidelines are here:

http://www.chromium.org/developers/coding-style

given the many security holes that can arise from getting the wrong type for
byte counts, using the correct type outweighs consistency.  Please fix any bad
places you can find.

[jsbell, 2014-10-07 21:47:31]

On 2014/10/07 21:08:43, michaeln1 wrote:
> > (2) The public API must use size_t uniformly.
> 
> A quick search finds many usages of the long long in the public\web and
> public\platform api for a byte count. I think other parts of the WebKit api
> related specifically to blob sizes consistently use 'long long'. From the
point
> of view on maintaining consistency with regard to the blob data type, long
long
> may actually be the best choice.

jkarlin also pointed out to me that it's a file size on disk, which may be >
size_t (e.g. 32 bit systems).

As michaeln says, we appear to fairly consistent within blink and in the public
API about holding the file length in a LL. Numerous points in chromium pipe
through a LL e.g. all the way from base::File GetSize() which deals in offsets
and lengths exclusively as int64. jkarlin's upstream (chromium-side) patch does
persist blob lengths as uint64s though.

But... yuck. BlobDataHandle itself takes/stores a LL for size, but has only a
ULL accessor for it. But nearly every caller (~26 of them) in Blink populates it
with LL, some explicitly negative e.g. File.cpp calls
BlobDataHandle::create(..., -1).

[jsbell, 2014-10-07 22:15:48]

On 2014/10/07 21:15:06, jamesr wrote:
> Please fix any bad places you can find.

Not disagreeing, just lamenting the current state of bogisity before deciding
how to proceed.

We can probably tackle the BlobDataHandle ctors and work backwards from there to
make sure all the callers are sanitizing inputs.

[jkarlin, 2014-10-09 13:49:52]

On 2014/10/07 22:15:48, jsbell wrote:
> On 2014/10/07 21:15:06, jamesr wrote:
> > Please fix any bad places you can find.
> 
> Not disagreeing, just lamenting the current state of bogisity before deciding
> how to proceed.
> 
> We can probably tackle the BlobDataHandle ctors and work backwards from there
to
> make sure all the callers are sanitizing inputs.

While michaeln and jsbell refactor blob I've updated this CL to use unsigned
long long (not size_t because we need at least 64 bytes on all platforms).

jamesr: PTAL.

[jkarlin, 2014-10-09 13:51:19]

Patchset #3 (id:40001) has been deleted

[jamesr, 2014-10-09 14:35:58]

On 2014/10/09 13:49:52, jkarlin wrote:
> On 2014/10/07 22:15:48, jsbell wrote:
> > On 2014/10/07 21:15:06, jamesr wrote:
> > > Please fix any bad places you can find.
> > 
> > Not disagreeing, just lamenting the current state of bogisity before
deciding
> > how to proceed.
> > 
> > We can probably tackle the BlobDataHandle ctors and work backwards from
there
> to
> > make sure all the callers are sanitizing inputs.
> 
> While michaeln and jsbell refactor blob I've updated this CL to use unsigned
> long long (not size_t because we need at least 64 bytes on all platforms).
> 

If you need 64 bits use uint64_t. Please read the document I linked earlier
about what types to use.
> jamesr: PTAL.

[jkarlin, 2014-10-09 20:38:20]

> If you need 64 bits use uint64_t. Please read the document I linked earlier
> about what types to use.
> > jamesr: PTAL.

Done.  I wasn't sure what the proper Blink type to use was (there is more
unsigned long long than uint64_t in Blink) but it looks like Chromium policy
applies.  Thanks for the correction.

[jamesr, 2014-10-09 20:44:18]

thanks, public/ lgtm

[jkarlin, 2014-10-09 22:10:26]

On 2014/10/09 20:44:18, jamesr wrote:
> thanks, public/ lgtm

Just to be clear, are you okay with landing this before the cleanup
https://code.google.com/p/chromium/issues/detail?id=422047 is finished?

[jamesr, 2014-10-09 22:35:17]

The public/ addition here, which is what I looked at, seems to be using the
right type.

[jkarlin, 2014-10-09 22:44:58]

The CQ bit was checked by jkarlin@chromium.org

[commit-bot: I haz the power, 2014-10-09 22:46:15]

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

[Ken Russell (switch to Gerrit), 2014-10-09 23:09:19]

On 2014/10/09 22:46:15, I haz the power (commit-bot) wrote:
> CQ is trying da patch. Follow status at
>  https://chromium-cq-status.appspot.com/patch-status/623753002/110001

I think your CL may have introduced browser or tab crashes and that's why the
mac_gpu_triggered_tests and mac_gpu_retina_triggered_tests bots are failing your
CL. Could you please try to reproduce those failures locally?

[commit-bot: I haz the power, 2014-10-10 00:39:40]

Committed patchset #4 (id:110001) as 183500