Support optional arguments in SendRequestNatives::StartRequest

Support optional arguments in SendRequestNatives::StartRequest

Extensions send calls via SendRequestNatives::StartRequest. It is necessary to support undefined v8 values to express optional parameters.

This CL exposes the functionality to configure support for Undefined, Date and Regex data types in content::V8ValueConverter and uses it to enable Undefined values in SendRequestNatives::StartRequest.


BUG=127533
TEST=error messages on console are gone, see bug report


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

[battre, 2012-05-10 09:16:31]

Hi.

Please review this CL.

asargent or koz: for chrome/renderer/extensions/send_request_natives.cc
(asargent added the use of V8ValueConverter, koz wrote the rest)

avi: for trivial change in content/ that extends an interface. jam wrote the
original interface but the OWNERS file asks to refrain from asking him to review
interface changes.

Thanks,
Dominic

[asargent_no_longer_on_chrome, 2012-05-10 20:17:08]

LGTM w/ a method naming nit

https://chromiumcodereview.appspot.com/10387055/diff/1007/content/public/rend...
File content/public/renderer/v8_value_converter.h (right):

https://chromiumcodereview.appspot.com/10387055/diff/1007/content/public/rend...
content/public/renderer/v8_value_converter.h:33: virtual bool IsAllowIndefined()
const = 0;
Indefined -> Undefined

Also, "IsAllow" sounds very strange (in conversation, you'd say "is undefined
allowed?" and not "is allow undefined?"). My preference would be to name the
getter just "AllowUndefined". 

similar issue for for IsAllowDate and IsAllowRegexp.

[koz (OOO until 15th September), 2012-05-10 21:40:37]

lgtm

[battre, 2012-05-11 12:13:34]

Addressed comments

[battre, 2012-05-11 12:16:28]

https://chromiumcodereview.appspot.com/10387055/diff/1007/content/public/rend...
File content/public/renderer/v8_value_converter.h (right):

https://chromiumcodereview.appspot.com/10387055/diff/1007/content/public/rend...
content/public/renderer/v8_value_converter.h:33: virtual bool IsAllowIndefined()
const = 0;
On 2012/05/10 20:17:08, Antony Sargent wrote:
> Indefined -> Undefined
> 
> Also, "IsAllow" sounds very strange (in conversation, you'd say "is undefined
> allowed?" and not "is allow undefined?"). My preference would be to name the
> getter just "AllowUndefined". 
> 
> similar issue for for IsAllowDate and IsAllowRegexp.

Done.

[jochen (gone - plz use gerrit), 2012-05-15 16:58:55]

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

http://codereview.chromium.org/10387055/diff/6002/content/renderer/v8_value_c...
content/renderer/v8_value_converter_impl.cc:37: 
nit. remove empty line

http://codereview.chromium.org/10387055/diff/6002/content/renderer/v8_value_c...
File content/renderer/v8_value_converter_impl.h (left):

http://codereview.chromium.org/10387055/diff/6002/content/renderer/v8_value_c...
content/renderer/v8_value_converter_impl.h:25: bool allow_undefined() const {
return allow_undefined_; }
any reason you renamed the functions? The coding style does not mandate virtual
setters/getters to be named differently

[battre, 2012-05-16 07:42:33]

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

http://codereview.chromium.org/10387055/diff/6002/content/renderer/v8_value_c...
content/renderer/v8_value_converter_impl.cc:37: 
On 2012/05/15 16:58:56, jochen wrote:
> nit. remove empty line

Done.

http://codereview.chromium.org/10387055/diff/6002/content/renderer/v8_value_c...
File content/renderer/v8_value_converter_impl.h (left):

http://codereview.chromium.org/10387055/diff/6002/content/renderer/v8_value_c...
content/renderer/v8_value_converter_impl.h:25: bool allow_undefined() const {
return allow_undefined_; }
On 2012/05/15 16:58:56, jochen wrote:
> any reason you renamed the functions? The coding style does not mandate
virtual
> setters/getters to be named differently

See the thread "[chromium-dev] virtual functions -> CamelCase".

[jochen (gone - plz use gerrit), 2012-05-16 08:29:08]

http://codereview.chromium.org/10387055/diff/6002/content/renderer/v8_value_c...
File content/renderer/v8_value_converter_impl.h (left):

http://codereview.chromium.org/10387055/diff/6002/content/renderer/v8_value_c...
content/renderer/v8_value_converter_impl.h:25: bool allow_undefined() const {
return allow_undefined_; }
On 2012/05/16 07:42:33, battre wrote:
> On 2012/05/15 16:58:56, jochen wrote:
> > any reason you renamed the functions? The coding style does not mandate
> virtual
> > setters/getters to be named differently
> 
> See the thread "[chromium-dev] virtual functions -> CamelCase".

ok, but then I'd name it SetUndefinedAllowed/GetUndefinedAllowed etc..

see other interfaces in content/public,  web_contents.h has e.g.
SetClosedByUserGesture/GetClosedByUserGesture

[battre, 2012-05-16 08:50:31]

http://codereview.chromium.org/10387055/diff/6002/content/renderer/v8_value_c...
File content/renderer/v8_value_converter_impl.h (left):

http://codereview.chromium.org/10387055/diff/6002/content/renderer/v8_value_c...
content/renderer/v8_value_converter_impl.h:25: bool allow_undefined() const {
return allow_undefined_; }
On 2012/05/16 08:29:09, jochen wrote:
> On 2012/05/16 07:42:33, battre wrote:
> > On 2012/05/15 16:58:56, jochen wrote:
> > > any reason you renamed the functions? The coding style does not mandate
> > virtual
> > > setters/getters to be named differently
> > 
> > See the thread "[chromium-dev] virtual functions -> CamelCase".
> 
> ok, but then I'd name it SetUndefinedAllowed/GetUndefinedAllowed etc..
> 
> see other interfaces in content/public,  web_contents.h has e.g.
> SetClosedByUserGesture/GetClosedByUserGesture

Done.

[jochen (gone - plz use gerrit), 2012-05-16 08:52:20]

lgtm

[battre, 2012-05-16 09:23:52]

Friendly ping at Avi for OWNERS approval.

[jam, 2012-05-16 14:58:19]

lgtm with nits

http://codereview.chromium.org/10387055/diff/10003/content/renderer/v8_value_...
File content/renderer/v8_value_converter_impl.h (right):

http://codereview.chromium.org/10387055/diff/10003/content/renderer/v8_value_...
content/renderer/v8_value_converter_impl.h:23: // Use the following setters to
support additional types other than the
nit: no need for this comment now that it's in the interface

http://codereview.chromium.org/10387055/diff/10003/content/renderer/v8_value_...
content/renderer/v8_value_converter_impl.h:27: 
nit: no blank lines since these are overridden methods. also move them after the
comment on line 334

[battre, 2012-05-16 16:11:12]

thx

http://codereview.chromium.org/10387055/diff/10003/content/renderer/v8_value_...
File content/renderer/v8_value_converter_impl.h (right):

http://codereview.chromium.org/10387055/diff/10003/content/renderer/v8_value_...
content/renderer/v8_value_converter_impl.h:23: // Use the following setters to
support additional types other than the
On 2012/05/16 14:58:20, John Abd-El-Malek wrote:
> nit: no need for this comment now that it's in the interface

Done.

http://codereview.chromium.org/10387055/diff/10003/content/renderer/v8_value_...
content/renderer/v8_value_converter_impl.h:27: 
On 2012/05/16 14:58:20, John Abd-El-Malek wrote:
> nit: no blank lines since these are overridden methods. also move them after
the
> comment on line 334

Done.

[commit-bot: I haz the power, 2012-05-16 16:11:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/battre@chromium.org/10387055/1011

[commit-bot: I haz the power, 2012-05-16 17:50:55]

Change committed as 137452