[Extensions Bindings] Add errors to signature parsing

[Extensions Bindings] Add errors to signature parsing

Curry up the errors from argument parsing to signature parsing in order
to indicate to scripts which argument threw the error.  Update the
APISignature unittests to test the new behavior.

BUG=653596
Review-Url: https://codereview.chromium.org/2847853002
Cr-Commit-Position: refs/heads/master@{#468486}
Committed: https://chromium.googlesource.com/chromium/src/+/29fdc161a3c313e67f09e0ef891b747005a4ccae

[Devlin, 2017-04-27 19:02:13]

rdevlin.cronin@chromium.org changed reviewers:
+ jbroman@chromium.org, lazyboy@chromium.org

[Devlin, 2017-04-27 19:02:14]

Another one for y'all.

Note: this still doesn't surface error to script (that's the next one).

[jbroman, 2017-04-27 19:37:20]

lgtm

https://codereview.chromium.org/2847853002/diff/1/extensions/renderer/api_sig...
File extensions/renderer/api_signature.cc (right):

https://codereview.chromium.org/2847853002/diff/1/extensions/renderer/api_sig...
extensions/renderer/api_signature.cc:184: *error_ =
api_errors::TooManyArguments();
Can this happen when the previous check for too many arguments succeeded? If so,
maybe a comment since it isn't immediately apparent to me.

https://codereview.chromium.org/2847853002/diff/1/extensions/renderer/argumen...
File extensions/renderer/argument_spec.cc (right):

https://codereview.chromium.org/2847853002/diff/1/extensions/renderer/argumen...
extensions/renderer/argument_spec.cc:424: // Technically, in the case of the
property being removed before access,
Hmm? Getting a nonexistent property will successfully produce undefined. This
branch is only for cases where some exception happened (either deliberated by
script, or due to another issue like an access check).

[lazyboy, 2017-04-27 20:55:03]

lgtm

https://codereview.chromium.org/2847853002/diff/1/extensions/renderer/api_sig...
File extensions/renderer/api_signature.cc (right):

https://codereview.chromium.org/2847853002/diff/1/extensions/renderer/api_sig...
extensions/renderer/api_signature.cc:195: *error_ =
api_errors::MissingRequiredArgument(spec.name().c_str());
Here and line 209, remove c_str().

[Devlin, 2017-05-01 22:22:07]

The CQ bit was checked by rdevlin.cronin@chromium.org to run a CQ dry run

[Devlin, 2017-05-01 22:22:09]

https://codereview.chromium.org/2847853002/diff/1/extensions/renderer/api_sig...
File extensions/renderer/api_signature.cc (right):

https://codereview.chromium.org/2847853002/diff/1/extensions/renderer/api_sig...
extensions/renderer/api_signature.cc:184: *error_ =
api_errors::TooManyArguments();
On 2017/04/27 19:37:20, jbroman wrote:
> Can this happen when the previous check for too many arguments succeeded? If
so,
> maybe a comment since it isn't immediately apparent to me.

Added.

    // This can potentially happen even if the check above for too many
    // arguments succeeds when optional parameters are omitted. For instance,
    // if the signature expects (optional int, function callback) and the caller
    // provides (function callback, object random), the first size check and
    // callback spec would succeed, but we wouldn't consume all the arguments.

https://codereview.chromium.org/2847853002/diff/1/extensions/renderer/api_sig...
extensions/renderer/api_signature.cc:195: *error_ =
api_errors::MissingRequiredArgument(spec.name().c_str());
On 2017/04/27 20:55:03, lazyboy wrote:
> Here and line 209, remove c_str().

MissingRequiredArgument() takes a const char* (and only needs the char* since
it's passed directly to printf).  Any reason to prefer passing the full string?

https://codereview.chromium.org/2847853002/diff/1/extensions/renderer/argumen...
File extensions/renderer/argument_spec.cc (right):

https://codereview.chromium.org/2847853002/diff/1/extensions/renderer/argumen...
extensions/renderer/argument_spec.cc:424: // Technically, in the case of the
property being removed before access,
On 2017/04/27 19:37:20, jbroman wrote:
> Hmm? Getting a nonexistent property will successfully produce undefined. This
> branch is only for cases where some exception happened (either deliberated by
> script, or due to another issue like an access check).

Ah, good point.  Comment removed.

[commit-bot: I haz the power, 2017-05-01 22:22:51]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[lazyboy, 2017-05-01 23:14:21]

https://codereview.chromium.org/2847853002/diff/1/extensions/renderer/api_sig...
File extensions/renderer/api_signature.cc (right):

https://codereview.chromium.org/2847853002/diff/1/extensions/renderer/api_sig...
extensions/renderer/api_signature.cc:195: *error_ =
api_errors::MissingRequiredArgument(spec.name().c_str());
On 2017/05/01 22:22:09, Devlin wrote:
> On 2017/04/27 20:55:03, lazyboy wrote:
> > Here and line 209, remove c_str().
> 
> MissingRequiredArgument() takes a const char* (and only needs the char* since
> it's passed directly to printf).  
I think I mistakenly read it as *error_ = some_string.c_str();

Any reason to prefer passing the full string?
Nope.

[commit-bot: I haz the power, 2017-05-01 23:25:39]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-01 23:25:40]

Dry run: This issue passed the CQ dry run.

[Devlin, 2017-05-01 23:32:15]

The CQ bit was checked by rdevlin.cronin@chromium.org

[Devlin, 2017-05-01 23:32:15]

The patchset sent to the CQ was uploaded after l-g-t-m from
lazyboy@chromium.org, jbroman@chromium.org
Link to the patchset: https://codereview.chromium.org/2847853002/#ps20001
(title: "Rebase")

[commit-bot: I haz the power, 2017-05-01 23:32:48]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-01 23:40:09]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1493681535084610,
"parent_rev": "a9983eadb02b514a528a14120090bf88794d24e8", "commit_rev":
"29fdc161a3c313e67f09e0ef891b747005a4ccae"}

[commit-bot: I haz the power, 2017-05-01 23:40:18]

Description was changed from

==========
[Extensions Bindings] Add errors to signature parsing

Curry up the errors from argument parsing to signature parsing in order
to indicate to scripts which argument threw the error.  Update the
APISignature unittests to test the new behavior.

BUG=653596
==========

to

==========
[Extensions Bindings] Add errors to signature parsing

Curry up the errors from argument parsing to signature parsing in order
to indicate to scripts which argument threw the error.  Update the
APISignature unittests to test the new behavior.

BUG=653596

Review-Url: https://codereview.chromium.org/2847853002
Cr-Commit-Position: refs/heads/master@{#468486}
Committed:
https://chromium.googlesource.com/chromium/src/+/29fdc161a3c313e67f09e0ef891b...
==========

[commit-bot: I haz the power, 2017-05-01 23:40:19]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/29fdc161a3c313e67f09e0ef891b...