[Extensions Bindings] Add argument parsing errors

[Extensions Bindings] Add argument parsing errors

When an invocation is invalid, bindings need to supply a reason why (and
currently native bindings just say "invalid invocation"). Add proper
error descriptions to reasons argument parsing can fail.

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

[Devlin, 2017-04-24 22:19:36]

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

[Devlin, 2017-04-24 22:19:37]

Folks, mind taking a look?

Note that this doesn't hook up the error message to the signature parsing;
that's the next CL (I figured this one was large enough as-is).

[Devlin, 2017-04-25 00:28:13]

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

[commit-bot: I haz the power, 2017-04-25 00:28:41]

Dry run: 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-04-25 02:43:53]

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

[commit-bot: I haz the power, 2017-04-25 02:43:53]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[jbroman, 2017-04-25 15:44:55]

https://codereview.chromium.org/2837023003/diff/40001/extensions/renderer/api...
File extensions/renderer/api_invocation_errors.cc (right):

https://codereview.chromium.org/2837023003/diff/40001/extensions/renderer/api...
extensions/renderer/api_invocation_errors.cc:38: std::string GetError(const
char* error, int placeholder) {
Hmm, my main question about this is that it's not obvious without inspecting the
source file which messages require one placeholder, which require two, etc. It
might be slightly less error-prone if these either had the format string at
their call site (where this can be seen by inspection), or this source file had
functions for each message, like:

std::string InvalidEnumValue(const char* valid_enumerators) {
  return base::StringPrintf("Value must be one of %s.", valid_enumerators);
}

https://codereview.chromium.org/2837023003/diff/40001/extensions/renderer/api...
extensions/renderer/api_invocation_errors.cc:53: return
base::StringPrintf("Error at index %u: %s", index, error.data());
You can't pass base::StringPiece to fill %s in printf, because it is not
null-terminated. (There's a comment warning about this in string_piece.h.)

You can kinda do it like this, with two caveats:
- convert the size to int, which would cause a problem if it were ever over 2G
- an embedded null will end the string early (rather than including the embedded
null in the result)

return base::StringPrintf("Error at index %u: %.*s", index, error.size(),
error.data());

Alternatively, you could use another approach, like operator+, or converting to
an std::string and then calling c_str.

[lazyboy, 2017-04-25 18:23:13]

https://codereview.chromium.org/2837023003/diff/40001/extensions/renderer/arg...
File extensions/renderer/argument_spec.cc (right):

https://codereview.chromium.org/2837023003/diff/40001/extensions/renderer/arg...
extensions/renderer/argument_spec.cc:52: // Returns true if |value| is within
the bounds specified by |minimum|,
within the bounds [|minimum|, |maximum|]

[Devlin, 2017-04-25 22:46:39]

Patchset #4 (id:60001) has been deleted

[Devlin, 2017-04-25 22:47:46]

Patchset #4 (id:80001) has been deleted

[Devlin, 2017-04-25 22:47:51]

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

[Devlin, 2017-04-25 22:47:59]

https://codereview.chromium.org/2837023003/diff/40001/extensions/renderer/api...
File extensions/renderer/api_invocation_errors.cc (right):

https://codereview.chromium.org/2837023003/diff/40001/extensions/renderer/api...
extensions/renderer/api_invocation_errors.cc:38: std::string GetError(const
char* error, int placeholder) {
On 2017/04/25 15:44:55, jbroman wrote:
> Hmm, my main question about this is that it's not obvious without inspecting
the
> source file which messages require one placeholder, which require two, etc. It
> might be slightly less error-prone if these either had the format string at
> their call site (where this can be seen by inspection), or this source file
had
> functions for each message, like:
> 
> std::string InvalidEnumValue(const char* valid_enumerators) {
>   return base::StringPrintf("Value must be one of %s.", valid_enumerators);
> }

Good idea!  I was a little worried about the excess code, but, in retrospect,
this makes them all much more readable.  Done.

https://codereview.chromium.org/2837023003/diff/40001/extensions/renderer/api...
extensions/renderer/api_invocation_errors.cc:53: return
base::StringPrintf("Error at index %u: %s", index, error.data());
On 2017/04/25 15:44:55, jbroman wrote:
> You can't pass base::StringPiece to fill %s in printf, because it is not
> null-terminated. (There's a comment warning about this in string_piece.h.)
> 
> You can kinda do it like this, with two caveats:
> - convert the size to int, which would cause a problem if it were ever over 2G
> - an embedded null will end the string early (rather than including the
embedded
> null in the result)
> 
> return base::StringPrintf("Error at index %u: %.*s", index, error.size(),
> error.data());
> 
> Alternatively, you could use another approach, like operator+, or converting
to
> an std::string and then calling c_str.

Good catch!  Opted for using std::strings for the errors, since they should be
constructed from other error calls anyway.

https://codereview.chromium.org/2837023003/diff/40001/extensions/renderer/arg...
File extensions/renderer/argument_spec.cc (right):

https://codereview.chromium.org/2837023003/diff/40001/extensions/renderer/arg...
extensions/renderer/argument_spec.cc:52: // Returns true if |value| is within
the bounds specified by |minimum|,
On 2017/04/25 18:23:12, lazyboy wrote:
> within the bounds [|minimum|, |maximum|]

Done.

[commit-bot: I haz the power, 2017-04-25 22:48:15]

Dry run: 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-04-26 00:06:40]

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

[commit-bot: I haz the power, 2017-04-26 00:06:41]

Dry run: This issue passed the CQ dry run.

[jbroman, 2017-04-26 14:34:26]

lgtm

This isn't surfaced to script yet, right? Or is it?

https://codereview.chromium.org/2837023003/diff/100001/extensions/renderer/ap...
File extensions/renderer/api_invocation_errors.cc (right):

https://codereview.chromium.org/2837023003/diff/100001/extensions/renderer/ap...
extensions/renderer/api_invocation_errors.cc:76: return
base::StringPrintf("Value did not match any choice.");
nit: no need for StringPrintf when there are no % replacements, here and below.

https://codereview.chromium.org/2837023003/diff/100001/extensions/renderer/ap...
File extensions/renderer/api_invocation_errors.h (right):

https://codereview.chromium.org/2837023003/diff/100001/extensions/renderer/ap...
extensions/renderer/api_invocation_errors.h:35: std::string TooFewArrayItems(int
minimum);
nit: You could also say how many were present (for this and string chars). Not a
big deal.

https://codereview.chromium.org/2837023003/diff/100001/extensions/renderer/ar...
File extensions/renderer/argument_spec.cc (right):

https://codereview.chromium.org/2837023003/diff/100001/extensions/renderer/ar...
extensions/renderer/argument_spec.cc:48: return "";
This isn't exhaustive (Symbol is missing, at least). Should the default be some
non-empty string? "" seems a little odd. :)

[Devlin, 2017-04-26 16:17:21]

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

[commit-bot: I haz the power, 2017-04-26 16:17:47]

Dry run: CQ is trying da patch.

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

[Devlin, 2017-04-26 16:18:26]

On 2017/04/26 14:34:26, jbroman wrote:
> This isn't surfaced to script yet, right? Or is it?

On 2017/04/24 22:19:37, Devlin wrote:
> Note that this doesn't hook up the error message to the signature parsing;
> that's the next CL (I figured this one was large enough as-is).

So nope :)

https://codereview.chromium.org/2837023003/diff/100001/extensions/renderer/ap...
File extensions/renderer/api_invocation_errors.cc (right):

https://codereview.chromium.org/2837023003/diff/100001/extensions/renderer/ap...
extensions/renderer/api_invocation_errors.cc:76: return
base::StringPrintf("Value did not match any choice.");
On 2017/04/26 14:34:26, jbroman wrote:
> nit: no need for StringPrintf when there are no % replacements, here and
below.

Whoops!  Thanks.  Find-replace fail.

https://codereview.chromium.org/2837023003/diff/100001/extensions/renderer/ap...
File extensions/renderer/api_invocation_errors.h (right):

https://codereview.chromium.org/2837023003/diff/100001/extensions/renderer/ap...
extensions/renderer/api_invocation_errors.h:35: std::string TooFewArrayItems(int
minimum);
On 2017/04/26 14:34:26, jbroman wrote:
> nit: You could also say how many were present (for this and string chars). Not
a
> big deal.

Why not.  Done.

https://codereview.chromium.org/2837023003/diff/100001/extensions/renderer/ar...
File extensions/renderer/argument_spec.cc (right):

https://codereview.chromium.org/2837023003/diff/100001/extensions/renderer/ar...
extensions/renderer/argument_spec.cc:48: return "";
On 2017/04/26 14:34:26, jbroman wrote:
> This isn't exhaustive (Symbol is missing, at least). Should the default be
some
> non-empty string? "" seems a little odd. :)

Good point.  Added "other" as a placeholder and a TODO to potentially expand
this to include more types.

[lazyboy, 2017-04-26 17:46:09]

lgtm

[commit-bot: I haz the power, 2017-04-26 17:51:00]

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

[commit-bot: I haz the power, 2017-04-26 17:51:01]

Dry run: This issue passed the CQ dry run.

[Devlin, 2017-04-26 17:55:58]

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

[Devlin, 2017-04-26 17:55:59]

The patchset sent to the CQ was uploaded after l-g-t-m from jbroman@chromium.org
Link to the patchset: https://codereview.chromium.org/2837023003/#ps120001
(title: "nits")

[commit-bot: I haz the power, 2017-04-26 17:56:57]

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-04-26 18:04:21]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1493229358720270,
"parent_rev": "bc11943ed50cef962cc059b6e24493f84c4de5fa", "commit_rev":
"d35ed4510ac666f199cc280c9eec8400cb8b9f03"}

[commit-bot: I haz the power, 2017-04-26 18:05:25]

Description was changed from

==========
[Extensions Bindings] Add argument parsing errors

When an invocation is invalid, bindings need to supply a reason why (and
currently native bindings just say "invalid invocation"). Add proper
error descriptions to reasons argument parsing can fail.

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

to

==========
[Extensions Bindings] Add argument parsing errors

When an invocation is invalid, bindings need to supply a reason why (and
currently native bindings just say "invalid invocation"). Add proper
error descriptions to reasons argument parsing can fail.

BUG=653596

Review-Url: https://codereview.chromium.org/2837023003
Cr-Commit-Position: refs/heads/master@{#467381}
Committed:
https://chromium.googlesource.com/chromium/src/+/d35ed4510ac666f199cc280c9eec...
==========

[commit-bot: I haz the power, 2017-04-26 18:05:26]

Committed patchset #5 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/d35ed4510ac666f199cc280c9eec...