Make V8ValueConverter.FromV8Value behave similarly to JSON.stringify

Make V8ValueConverter.FromV8Value be more consistent with JSON.stringify: don't
serialize undefined as null, don't serialize functions as objects, omit values
from objects when they don't serialize, and insert null into arrays when they
don't serialize.

It is now possible for FromV8Value to return NULL; previously it would return
Value::CreateNullValue on failure.

This is needed for the Storage API, where we promise that the values that are
passed in are serialized as JSON, yet the value conversion doesn't work in a
way that allows it.

However, the null-stripping behavior needs to be configurable so that existing
extension APIs (which only expect null/undefined to appear for optional values)
still work.

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

[not at google - send to devlin, 2012-08-28 05:59:38]

I'm not sure about the approach here.

I kind of want this "null safe API" thing to be an annotation on the API
functions, like

{ "namespace": "storage",
  "functions": [
    { "name": "get",
      "preserve_null": true
    }
  ]
}

but that would require putting the logic in JS (since ExtensionAPI isn't safe to
use in the renderer without knowledge of the context) which requires threading
through a bool like "preserve_null" from send_request.js into StartRequest.
Maybe that's ok? Feels icky from an implementation perspective but nice from a
specification perspective. This approach seems the opposite.

WDYT?

[koz (OOO until 15th September), 2012-08-28 06:15:02]

On 2012/08/28 05:59:38, kalman wrote:
> I'm not sure about the approach here.
> 
> I kind of want this "null safe API" thing to be an annotation on the API
> functions, like
> 
> { "namespace": "storage",
>   "functions": [
>     { "name": "get",
>       "preserve_null": true
>     }
>   ]
> }
> 
> but that would require putting the logic in JS (since ExtensionAPI isn't safe
to
> use in the renderer without knowledge of the context) which requires threading
> through a bool like "preserve_null" from send_request.js into StartRequest.
> Maybe that's ok? Feels icky from an implementation perspective but nice from a
> specification perspective. This approach seems the opposite.
> 
> WDYT?

Well, the way you've done it is the nicest way of hardcoding which APIs are
null-safe :-)

The threading shouldn't be too involved, right? I'd probably prefer that method.
The kinds of errors that could arise in this area seem like they could be quite
subtle.

[not at google - send to devlin, 2012-08-28 06:49:56]

On 2012/08/28 06:15:02, koz wrote:
> On 2012/08/28 05:59:38, kalman wrote:
> > I'm not sure about the approach here.
> > 
> > I kind of want this "null safe API" thing to be an annotation on the API
> > functions, like
> > 
> > { "namespace": "storage",
> >   "functions": [
> >     { "name": "get",
> >       "preserve_null": true
> >     }
> >   ]
> > }
> > 
> > but that would require putting the logic in JS (since ExtensionAPI isn't
safe
> to
> > use in the renderer without knowledge of the context) which requires
threading
> > through a bool like "preserve_null" from send_request.js into StartRequest.
> > Maybe that's ok? Feels icky from an implementation perspective but nice from
a
> > specification perspective. This approach seems the opposite.
> > 
> > WDYT?
> 
> Well, the way you've done it is the nicest way of hardcoding which APIs are
> null-safe :-)
> 
> The threading shouldn't be too involved, right? I'd probably prefer that
method.
> The kinds of errors that could arise in this area seem like they could be
quite
> subtle.

Clarifying possible confusion that I may have caused, not sure:

I don't mean for every API call to need to specify whether to preserve nulls.
The developer API wouldn't change. I just mean for there to be an internal-only
annotation in storage.json to say that the functions support null.

Even better: express it in the json schema of the objects themselves.
Unfortunately most objects have "type:any" for their properties, which according
to

http://tools.ietf.org/html/draft-zyp-json-schema-03#section-5.1

also accepts null. The problem the null-stripping code is trying to solve is
that API implementations don't handle this correctly.

In any case, adding such an annotation would also be a PITA since you'd need to
add support for that at the V8ValueConverter layer and that aint going to
happen.

which... is how I arrived at this solution. Phew.

But I think that adding a "preserve_null_in_arguments" property to the function
schemas would be a nicer solution, so I'll do that tomorrow.

[koz (OOO until 15th September), 2012-08-28 06:58:23]

On 2012/08/28 06:49:56, kalman wrote:
> On 2012/08/28 06:15:02, koz wrote:
> > On 2012/08/28 05:59:38, kalman wrote:
> > > I'm not sure about the approach here.
> > > 
> > > I kind of want this "null safe API" thing to be an annotation on the API
> > > functions, like
> > > 
> > > { "namespace": "storage",
> > >   "functions": [
> > >     { "name": "get",
> > >       "preserve_null": true
> > >     }
> > >   ]
> > > }
> > > 
> > > but that would require putting the logic in JS (since ExtensionAPI isn't
> safe
> > to
> > > use in the renderer without knowledge of the context) which requires
> threading
> > > through a bool like "preserve_null" from send_request.js into
StartRequest.
> > > Maybe that's ok? Feels icky from an implementation perspective but nice
from
> a
> > > specification perspective. This approach seems the opposite.
> > > 
> > > WDYT?
> > 
> > Well, the way you've done it is the nicest way of hardcoding which APIs are
> > null-safe :-)
> > 
> > The threading shouldn't be too involved, right? I'd probably prefer that
> method.
> > The kinds of errors that could arise in this area seem like they could be
> quite
> > subtle.
> 
> Clarifying possible confusion that I may have caused, not sure:
> 
> I don't mean for every API call to need to specify whether to preserve nulls.
> The developer API wouldn't change. I just mean for there to be an
internal-only
> annotation in storage.json to say that the functions support null.
> 
> Even better: express it in the json schema of the objects themselves.
> Unfortunately most objects have "type:any" for their properties, which
according
> to
> 
> http://tools.ietf.org/html/draft-zyp-json-schema-03#section-5.1
> 
> also accepts null. The problem the null-stripping code is trying to solve is
> that API implementations don't handle this correctly.
> 
> In any case, adding such an annotation would also be a PITA since you'd need
to
> add support for that at the V8ValueConverter layer and that aint going to
> happen.
> 
> which... is how I arrived at this solution. Phew.
> 
> But I think that adding a "preserve_null_in_arguments" property to the
function
> schemas would be a nicer solution, so I'll do that tomorrow.

Yep, I get that it's just for internal purposes. "preserve_null_in_arguments"
SGTM.

[not at google - send to devlin, 2012-09-11 11:47:34]

Ok, done.

It ended up being a bit of a yak, since I realised that with this fix,

chrome.storage.local.set({ 'a': undefined })

would set 'a' to null. The Storage API claims to store values in a
JSON-compatible format, and this breaks that promise.

So the yak was changing V8ValueConverterImpl to behave more like JSON.stringify.

Adding aa@ for the V8ValueConverter + other extension changes, and avi@ for the
V8ValueConverter + content changes.

[not at google - send to devlin, 2012-09-11 11:50:11]

https://chromiumcodereview.appspot.com/10890002/diff/18002/content/public/ren...
File content/public/renderer/v8_value_converter.h (right):

https://chromiumcodereview.appspot.com/10890002/diff/18002/content/public/ren...
content/public/renderer/v8_value_converter.h:36: TREAT_AS_ERROR,
The existence of "treat as error" only makes if trying to maintain the behaviour
where we logged warnings when trying to convert undefined. I don't know how
useful that is.

Ideally all this configuration stuff would be deleted and OMIT would be implied.

(at least, make it the default and delete TREAT_AS_ERROR).

WDYT?

[Aaron Boodman, 2012-09-11 21:58:36]

reviewers -= aa; if koz is happy i am happy

[koz (OOO until 15th September), 2012-09-12 02:08:28]

lgtm

https://chromiumcodereview.appspot.com/10890002/diff/1/chrome/renderer/extens...
File chrome/renderer/extensions/send_request_natives.cc (right):

https://chromiumcodereview.appspot.com/10890002/diff/1/chrome/renderer/extens...
chrome/renderer/extensions/send_request_natives.cc:25: // Some APIs can handle
nulls in objects, others can't. For some that can
"For some that can" -> "For those that can"

https://chromiumcodereview.appspot.com/10890002/diff/18002/content/public/ren...
File content/public/renderer/v8_value_converter.h (right):

https://chromiumcodereview.appspot.com/10890002/diff/18002/content/public/ren...
content/public/renderer/v8_value_converter.h:36: TREAT_AS_ERROR,
On 2012/09/11 11:50:11, kalman wrote:
> The existence of "treat as error" only makes if trying to maintain the
behaviour
> where we logged warnings when trying to convert undefined. I don't know how
> useful that is.
> 
> Ideally all this configuration stuff would be deleted and OMIT would be
implied.
> 
> (at least, make it the default and delete TREAT_AS_ERROR).
> 
> WDYT?

As discussed, seeing as this logs errors to STDERR in debug builds it's probably
not doing extension developers any good so it's fine to remove it.

https://chromiumcodereview.appspot.com/10890002/diff/18002/content/public/ren...
content/public/renderer/v8_value_converter.h:70: // where unsupported types are
converted to Value(TYPE_NULL).
where unsupported types -> where elements that have unsupported types

https://chromiumcodereview.appspot.com/10890002/diff/18002/content/public/ren...
content/public/renderer/v8_value_converter.h:73: // converted, whereas if an
array throws while converting an item it will be
Perhaps better phrased as:

If an object throws while converting a property, the resulting base::Value will
omit that property altogether. If an exception is thrown while converting an
element of an array the resulting array will have a Value(TYPE_NULL) in the
corresponding position.

https://chromiumcodereview.appspot.com/10890002/diff/18002/content/renderer/v...
File content/renderer/v8_value_converter_impl_unittest.cc (right):

https://chromiumcodereview.appspot.com/10890002/diff/18002/content/renderer/v...
content/renderer/v8_value_converter_impl_unittest.cc:506:
ASSERT_FALSE(array.IsEmpty());
ASSERT_EQ(3u, array.Size()) ?

[not at google - send to devlin, 2012-09-12 08:00:53]

Ok, so it turns out the contextMenus API relies on being able to convert
functions to objects, so I added that behaviour explicitly to V8ValueConverter.
I spent a while trying to fix context_menus_custom_bindings.js to not work that
way, but it went past my yak shave patience threshhold so I worked around it.

koz, PTAL.
(and still need renderer lg)

[not at google - send to devlin, 2012-09-12 08:01:51]

Oh and in doing so I also cleaned up a bit more of V8ValueConverter (removed the
Get* methods that weren't used, put some more comments in, and removed that
set-undefined-value-behavior function as discussed).

[not at google - send to devlin, 2012-09-14 00:59:41]

replacing avi@ with more targeted reviewers for content/renderer (jamesr) and
content/public/renderer (joi).

[koz (OOO until 15th September), 2012-09-14 02:05:44]

lgtm

https://chromiumcodereview.appspot.com/10890002/diff/15008/content/public/ren...
File content/public/renderer/v8_value_converter.h (right):

https://chromiumcodereview.appspot.com/10890002/diff/15008/content/public/ren...
content/public/renderer/v8_value_converter.h:32: // seconds since Unix epoch.
Good job on adding comments.

https://chromiumcodereview.appspot.com/10890002/diff/15008/content/public/ren...
content/public/renderer/v8_value_converter.h:46: // additional properties has
been set on them.
"Otherwise they are omitted from dictionaries and arrays they are a member of."
?

[Jói, 2012-09-14 09:26:54]

LGTM for content/public.

[jamesr, 2012-09-14 22:11:51]

content/renderer lgtm

[not at google - send to devlin, 2012-09-15 06:24:24]

https://chromiumcodereview.appspot.com/10890002/diff/15008/content/public/ren...
File content/public/renderer/v8_value_converter.h (right):

https://chromiumcodereview.appspot.com/10890002/diff/15008/content/public/ren...
content/public/renderer/v8_value_converter.h:46: // additional properties has
been set on them.
On 2012/09/14 02:05:44, koz wrote:
> "Otherwise they are omitted from dictionaries and arrays they are a member
of."
> ?

Done.

[commit-bot: I haz the power, 2012-09-15 06:24:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kalman@chromium.org/10890002/10033

[commit-bot: I haz the power, 2012-09-15 07:13:54]

Sorry for I got bad news for ya.
Compile failed with a clobber build.
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-09-15 12:29:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kalman@chromium.org/10890002/10033

[commit-bot: I haz the power, 2012-09-15 13:12:02]

Sorry for I got bad news for ya.
Compile failed with a clobber build.
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-09-17 02:00:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kalman@chromium.org/10890002/12031

[commit-bot: I haz the power, 2012-09-17 04:05:26]

Change committed as 157074