Allow serialization of ArrayBuffer params in extension/apps API methods

Allow serialization of ArrayBuffer params in extension/apps API methods

This changes the V8ValueConverter to do ArrayBuffer<->BinaryValue conversions, 
as well as supporting ArrayBufferView subclasses as request parameters (but 
not response ones, since that's unnecessary).

Also adds an experimental API for testing ArrayBuffers in request/response
parameters.

This depends on WebKit changes in https://bugs.webkit.org/show_bug.cgi?id=84899 

BUG=122675
TEST=Included browser tests should pass

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=135602

[asargent_no_longer_on_chrome, 2012-05-04 18:10:08]

Matt - once everything looks ok to you, I'll add OWNERS reviewers for the
content/ stuff.

http://codereview.chromium.org/10161038/diff/8028/content/renderer/v8_value_c...
File content/renderer/v8_value_converter_impl.cc (right):

http://codereview.chromium.org/10161038/diff/8028/content/renderer/v8_value_c...
content/renderer/v8_value_converter_impl.cc:146: memcpy(buffer.data(),
value->GetBuffer(), value->GetSize());
Note that I'm looking into ways to avoid the need to copy here, but that will be
follow-up performance optimization work and might require a bit of refactoring
(actually, I'm not even sure if it's possible to transfer ownership of memory
from data in incoming IPC's, since it might be backed by OS shared memory
regions?).

http://codereview.chromium.org/10161038/diff/8028/content/renderer/v8_value_c...
content/renderer/v8_value_converter_impl.cc:241: return
base::BinaryValue::CreateWithCopiedBuffer(data, length);
Similarly here, it would be nice to avoid needing to make a copy. One thing we
might do is add the notion of a BinaryValue with an "unowned" buffer that is
read-only. The only danger is that we would need to somehow make sure we don't
hold on to that pointer past the lifetime of this method dispatching (ie keep
from holding on to it past where we get back into V8/Webkit code which could
change or deallocate the buffer)

[Matt Perry, 2012-05-04 19:22:21]

mostly LG, just one concern about StartRequest

http://codereview.chromium.org/10161038/diff/8028/chrome/renderer/extensions/...
File chrome/renderer/extensions/send_request_natives.cc (right):

http://codereview.chromium.org/10161038/diff/8028/chrome/renderer/extensions/...
chrome/renderer/extensions/send_request_natives.cc:45:
converter->FromV8Value(args[1], v8::Context::GetCurrent()));
Won't this break the stringified case? i.e. if someone passes '1', that will get
stringified then converted to a StringValue("1"), whereas it previously would
have been stringified then parsed back to IntegerValue(1).

... I think.

http://codereview.chromium.org/10161038/diff/8028/chrome/test/data/extensions...
File chrome/test/data/extensions/api_test/idltest/binary_data/binary.js (right):

http://codereview.chromium.org/10161038/diff/8028/chrome/test/data/extensions...
chrome/test/data/extensions/api_test/idltest/binary_data/binary.js:11: var view
= new Uint8Array(buf);
view is unused

http://codereview.chromium.org/10161038/diff/8028/content/renderer/v8_value_c...
File content/renderer/v8_value_converter_impl.cc (right):

http://codereview.chromium.org/10161038/diff/8028/content/renderer/v8_value_c...
content/renderer/v8_value_converter_impl.cc:145:
WebKit::WebArrayBuffer::create(value->GetSize(), 1);
indent += 2

http://codereview.chromium.org/10161038/diff/8028/content/renderer/v8_value_c...
content/renderer/v8_value_converter_impl.cc:146: memcpy(buffer.data(),
value->GetBuffer(), value->GetSize());
On 2012/05/04 18:10:08, Antony Sargent wrote:
> Note that I'm looking into ways to avoid the need to copy here, but that will
be
> follow-up performance optimization work and might require a bit of refactoring
> (actually, I'm not even sure if it's possible to transfer ownership of memory
> from data in incoming IPC's, since it might be backed by OS shared memory
> regions?).

BinaryValues are serialized in ipc_message_utils.cc. There, they use
BinaryValue::CreateWithCopiedBuffer, so the buffer is already a copy of the IPC
stream. If there were a way to transfer ownership of the BinaryValue's buffer,
you could use that. (There isn't currently, but you could add a swap mtehod.) Of
course that assumes WebArrayBuffer supports it as well.

http://codereview.chromium.org/10161038/diff/8028/content/renderer/v8_value_c...
content/renderer/v8_value_converter_impl.cc:189: if (binary_value)
nit: use braces in compound if/else statements.

http://codereview.chromium.org/10161038/diff/8028/content/renderer/v8_value_c...
content/renderer/v8_value_converter_impl.cc:241: return
base::BinaryValue::CreateWithCopiedBuffer(data, length);
On 2012/05/04 18:10:08, Antony Sargent wrote:
> Similarly here, it would be nice to avoid needing to make a copy. One thing we
> might do is add the notion of a BinaryValue with an "unowned" buffer that is
> read-only. The only danger is that we would need to somehow make sure we don't
> hold on to that pointer past the lifetime of this method dispatching (ie keep
> from holding on to it past where we get back into V8/Webkit code which could
> change or deallocate the buffer)

Hmm... maybe introduce a WeakBinaryValue that has a weak pointer to the buffer?
But that might be overkill.

[asargent_no_longer_on_chrome, 2012-05-04 22:42:22]

PTAL

http://codereview.chromium.org/10161038/diff/8028/chrome/renderer/extensions/...
File chrome/renderer/extensions/send_request_natives.cc (right):

http://codereview.chromium.org/10161038/diff/8028/chrome/renderer/extensions/...
chrome/renderer/extensions/send_request_natives.cc:45:
converter->FromV8Value(args[1], v8::Context::GetCurrent()));
On 2012/05/04 19:22:21, Matt Perry wrote:
> Won't this break the stringified case? i.e. if someone passes '1', that will
get
> stringified then converted to a StringValue("1"), whereas it previously would
> have been stringified then parsed back to IntegerValue(1).
> 
> ... I think.

In send_request.js I made a change that should ensure we only get the raw V8
args passed to this function. 

So instead of JSON.stringify (in JS) -> JSONReader::Read (in C++), we now just
pass raw V8 -> V8ValueConverter (in C++), which hopefully should give the same
resulting base::Value object.

http://codereview.chromium.org/10161038/diff/8028/chrome/test/data/extensions...
File chrome/test/data/extensions/api_test/idltest/binary_data/binary.js (right):

http://codereview.chromium.org/10161038/diff/8028/chrome/test/data/extensions...
chrome/test/data/extensions/api_test/idltest/binary_data/binary.js:11: var view
= new Uint8Array(buf);
On 2012/05/04 19:22:21, Matt Perry wrote:
> view is unused
Removed, good catch.

http://codereview.chromium.org/10161038/diff/8028/content/renderer/v8_value_c...
File content/renderer/v8_value_converter_impl.cc (right):

http://codereview.chromium.org/10161038/diff/8028/content/renderer/v8_value_c...
content/renderer/v8_value_converter_impl.cc:145:
WebKit::WebArrayBuffer::create(value->GetSize(), 1);
On 2012/05/04 19:22:21, Matt Perry wrote:
> indent += 2

Done.

http://codereview.chromium.org/10161038/diff/8028/content/renderer/v8_value_c...
content/renderer/v8_value_converter_impl.cc:146: memcpy(buffer.data(),
value->GetBuffer(), value->GetSize());
On 2012/05/04 19:22:21, Matt Perry wrote:
> On 2012/05/04 18:10:08, Antony Sargent wrote:
> > Note that I'm looking into ways to avoid the need to copy here, but that
will
> be
> > follow-up performance optimization work and might require a bit of
refactoring
> > (actually, I'm not even sure if it's possible to transfer ownership of
memory
> > from data in incoming IPC's, since it might be backed by OS shared memory
> > regions?).
> 
> BinaryValues are serialized in ipc_message_utils.cc. There, they use
> BinaryValue::CreateWithCopiedBuffer, so the buffer is already a copy of the
IPC
> stream. If there were a way to transfer ownership of the BinaryValue's buffer,
> you could use that. (There isn't currently, but you could add a swap mtehod.)
Of
> course that assumes WebArrayBuffer supports it as well.

Yep, but I'd also probably have to do something about the "const"-ness of the
incoming parameter. Maybe add the concept of BinaryValues with read-only
ref-counted shared buffers? (I think WebKit has something like this in WTF that
they use to avoid copies of WebArrayBuffer objects).

http://codereview.chromium.org/10161038/diff/8028/content/renderer/v8_value_c...
content/renderer/v8_value_converter_impl.cc:189: if (binary_value)
On 2012/05/04 19:22:21, Matt Perry wrote:
> nit: use braces in compound if/else statements.

Done.

http://codereview.chromium.org/10161038/diff/17013/chrome/renderer/resources/...
File chrome/renderer/resources/extensions/send_request.js (right):

http://codereview.chromium.org/10161038/diff/17013/chrome/renderer/resources/...
chrome/renderer/resources/extensions/send_request.js:113: if
(opt_args.nativeFunction && !opt_args.noStringify)
FYI, this line fixes browser test failures I was getting from patch set 3
because I was forgetting to test the "noStringify" option in the case where an
API method has its own nativeFunction. It turns out the setIcon methods do this.

[Matt Perry, 2012-05-04 22:58:14]

lgtm

http://codereview.chromium.org/10161038/diff/8028/chrome/renderer/extensions/...
File chrome/renderer/extensions/send_request_natives.cc (right):

http://codereview.chromium.org/10161038/diff/8028/chrome/renderer/extensions/...
chrome/renderer/extensions/send_request_natives.cc:45:
converter->FromV8Value(args[1], v8::Context::GetCurrent()));
On 2012/05/04 22:42:22, Antony Sargent wrote:
> On 2012/05/04 19:22:21, Matt Perry wrote:
> > Won't this break the stringified case? i.e. if someone passes '1', that will
> get
> > stringified then converted to a StringValue("1"), whereas it previously
would
> > have been stringified then parsed back to IntegerValue(1).
> > 
> > ... I think.
> 
> In send_request.js I made a change that should ensure we only get the raw V8
> args passed to this function. 
> 
> So instead of JSON.stringify (in JS) -> JSONReader::Read (in C++), we now just
> pass raw V8 -> V8ValueConverter (in C++), which hopefully should give the same
> resulting base::Value object.

Sorry, I was looking at the case where opt_args.nativeFunction was defined - but
in that case we won't be calling StartRequest. Carry on!

[asargent_no_longer_on_chrome, 2012-05-04 23:05:38]

+jam for OWNERS review of V8ValueConverter changes in content/

[jam, 2012-05-06 06:38:37]

content lgtm

[commit-bot: I haz the power, 2012-05-06 19:06:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/asargent@chromium.org/10161038/17013

[commit-bot: I haz the power, 2012-05-06 20:22:53]

Change committed as 135602