[Extensions Bindings] Add lastError utilities to APIBindingJSUtil

[Extensions Bindings] Add lastError utilities to APIBindingJSUtil

Some custom bindings require the ability to query or mutate the
lastError state. Provide native bindings to perform the following
functions:
- setLastError
- clearLastError
- hasLastError
- runCallbackWithLastError (sets the last error, runs the provided
  callback, and then clears the last error).

Add tests for the same.

Note that in the JS bindings, the analogous methods also accept a
context "chrome" object. This is bad, since its only use is allowing
cross-context modification. We should see if we can eliminate it.

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

[Devlin, 2017-03-20 16:51:25]

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

[commit-bot: I haz the power, 2017-03-20 16:51:59]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Devlin, 2017-03-20 17:54:08]

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

[Devlin, 2017-03-20 17:54:09]

Last one for now, promise! :)

The testing for this one was the motivation for
https://codereview.chromium.org/2763493003/; the unittests are much cleaner by
allowing the reuse of that class.

[jbroman, 2017-03-20 20:14:50]

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

https://codereview.chromium.org/2762623003/diff/40001/extensions/renderer/api...
extensions/renderer/api_binding_js_util.cc:161:
std::vector<v8::Local<v8::Value>> callback_args) {
const vector&: gin doesn't move the arguments into the callee (maybe it ought
to, but it doesn't at present), so this currently does an unnecessary copy

https://codereview.chromium.org/2762623003/diff/40001/extensions/renderer/api...
extensions/renderer/api_binding_js_util.cc:169: if (!callback->IsNull() &&
!callback->IsUndefined())
1. What's the point of calling this will null or undefined anyhow, since it just
sets the error and then shouts (since it will obviously never be checked if that
branch is taken)?

2. Null and undefined aren't v8::Functions, so it would be a type error to get
to this point. (specifically, the gin::Converter logic before this point should
throw an exception if either of those values is provided)

https://codereview.chromium.org/2762623003/diff/40001/extensions/renderer/api...
File extensions/renderer/api_binding_js_util.h (right):

https://codereview.chromium.org/2762623003/diff/40001/extensions/renderer/api...
extensions/renderer/api_binding_js_util.h:82: std::vector<v8::Local<v8::Value>>
callback_args);
Is it advantageous to provide the arguments separately? From a JS perspective I
would just expect the caller to capture the arguments and do something like:

runCallbackWithLastError('astonishing error message', function() { callback(my,
args, here); });

This also means you have slightly less logic to write and test.

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

https://codereview.chromium.org/2762623003/diff/40001/extensions/renderer/api...
extensions/renderer/api_binding_js_util_unittest.cc:49: return "[empty]";
I find it slightly confusing that "[empty]" is returned in the case of
undefined, not an empty handle (which we'd ordinarily crash or branch on, but I
see here that seems to be how you're handling exceptions. The more usual pattern
would be something like:

v8::Isolate* isolate = context->GetIsolate();
v8::Local<v8::Value> last_error;
if (!context->Global()->Get(context, gin::StringToSymbol(isolate,
"lastError")).ToLocal(&last_error) {
  ADD_FAILURE() << "getting lastError threw an exception";
  return std::string();
}
// etc

[Devlin, 2017-03-20 23:10:40]

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

https://codereview.chromium.org/2762623003/diff/40001/extensions/renderer/api...
extensions/renderer/api_binding_js_util.cc:161:
std::vector<v8::Local<v8::Value>> callback_args) {
On 2017/03/20 20:14:49, jbroman wrote:
> const vector&: gin doesn't move the arguments into the callee (maybe it ought
> to, but it doesn't at present), so this currently does an unnecessary copy

Moot now with no |callback_args|, but this was actually because the various
RunJS() functions accept a v8::Local<v8::Value>[] - and you can't bind that  to
std::vector<v8::Local<v8::Value>>::data().  I was hoping that I could have taken
a non-const &, but gin barfed on that idea.  We could also hack our way out of
it with various casts, but given v8::Locals are pretty cheap to copy, this
seemed like the most appropriate.

https://codereview.chromium.org/2762623003/diff/40001/extensions/renderer/api...
extensions/renderer/api_binding_js_util.cc:169: if (!callback->IsNull() &&
!callback->IsUndefined())
On 2017/03/20 20:14:49, jbroman wrote:
> 1. What's the point of calling this will null or undefined anyhow, since it
just
> sets the error and then shouts (since it will obviously never be checked if
that
> branch is taken)?
> 
> 2. Null and undefined aren't v8::Functions, so it would be a type error to get
> to this point. (specifically, the gin::Converter logic before this point
should
> throw an exception if either of those values is provided)

1.  If we mimic the JS api, the function is often a curried callback from an
extension API call, which can be undefined/null if the extension is uninterested
in the result.  And in that case, immediately yelling is the right thing to do. 
But, if we change the signature to accept a bindings-supplied function, this
should never happen.

2.  D'oh.

Done.

https://codereview.chromium.org/2762623003/diff/40001/extensions/renderer/api...
File extensions/renderer/api_binding_js_util.h (right):

https://codereview.chromium.org/2762623003/diff/40001/extensions/renderer/api...
extensions/renderer/api_binding_js_util.h:82: std::vector<v8::Local<v8::Value>>
callback_args);
On 2017/03/20 20:14:49, jbroman wrote:
> Is it advantageous to provide the arguments separately? From a JS perspective
I
> would just expect the caller to capture the arguments and do something like:
> 
> runCallbackWithLastError('astonishing error message', function() {
callback(my,
> args, here); });
> 
> This also means you have slightly less logic to write and test.

Nope - this was just for parity with the JS equivalent.  The other option I
thought of was just having callers bind the arguments.  Just accepting a
v8::Function allows both, so sgtm.

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

https://codereview.chromium.org/2762623003/diff/40001/extensions/renderer/api...
extensions/renderer/api_binding_js_util_unittest.cc:49: return "[empty]";
On 2017/03/20 20:14:49, jbroman wrote:
> I find it slightly confusing that "[empty]" is returned in the case of
> undefined, not an empty handle (which we'd ordinarily crash or branch on, but
I
> see here that seems to be how you're handling exceptions. The more usual
pattern
> would be something like:
> 
> v8::Isolate* isolate = context->GetIsolate();
> v8::Local<v8::Value> last_error;
> if (!context->Global()->Get(context, gin::StringToSymbol(isolate,
> "lastError")).ToLocal(&last_error) {
>   ADD_FAILURE() << "getting lastError threw an exception";
>   return std::string();
> }
> // etc

Not so much how we're handling exceptions as how we're handling an error not
existing.  The lastError property in extension APIs (an old concept, but, well,
we're kind of stuck with it for the moment) transitively exists and doesn't
exist on its parent object (in practice, the chrome.runtime object).  So when
[parent].lastError is undefined (as here), it's not an exception; it just means
no last error was set (which is perfectly valid state for a few of these
places).  Invalid state would be if we get an empty handle, or lastError is
something unexpected - each of those are checked above and ADD_FAILURE()'d.

[jbroman, 2017-03-21 22:02:47]

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

https://codereview.chromium.org/2762623003/diff/40001/extensions/renderer/api...
extensions/renderer/api_binding_js_util_unittest.cc:49: return "[empty]";
On 2017/03/20 at 23:10:40, Devlin wrote:
> On 2017/03/20 20:14:49, jbroman wrote:
> > I find it slightly confusing that "[empty]" is returned in the case of
> > undefined, not an empty handle (which we'd ordinarily crash or branch on,
but I
> > see here that seems to be how you're handling exceptions. The more usual
pattern
> > would be something like:
> > 
> > v8::Isolate* isolate = context->GetIsolate();
> > v8::Local<v8::Value> last_error;
> > if (!context->Global()->Get(context, gin::StringToSymbol(isolate,
> > "lastError")).ToLocal(&last_error) {
> >   ADD_FAILURE() << "getting lastError threw an exception";
> >   return std::string();
> > }
> > // etc
> 
> Not so much how we're handling exceptions as how we're handling an error not
existing.  The lastError property in extension APIs (an old concept, but, well,
we're kind of stuck with it for the moment) transitively exists and doesn't
exist on its parent object (in practice, the chrome.runtime object).  So when
[parent].lastError is undefined (as here), it's not an exception; it just means
no last error was set (which is perfectly valid state for a few of these
places). 

Getting a missing property yields undefined, not an empty handle. Unless an
exception is thrown, you should _not_ get an empty handle here. (As it happens,
if you do get an empty handle here, GetPropertyFromObject has already added a
failure due to EXPECT_TRUE.)

> Invalid state would be if we get an empty handle, or lastError is something
unexpected - each of those are checked above and ADD_FAILURE()'d.

https://codereview.chromium.org/2762623003/diff/60001/extensions/renderer/api...
File extensions/renderer/api_binding_js_util.cc (right):

https://codereview.chromium.org/2762623003/diff/60001/extensions/renderer/api...
extensions/renderer/api_binding_js_util.cc:132:
request_handler_->last_error()->SetError(context, error);
nit: Since the gin::Wrappable isn't shared between contexts, this would also
suffice (this wrappable's wrapper is the holder, though it does require opening
one more v8 handle and so might be epsilon slower?):

void APIBindingJSUtil::SetLastError(v8::Isolate* isolate,
                                    const std::string& error) {
  request_handler_->last_error()->SetError(
      GetWrapper(isolate)->CreationContext(), error);
}

I don't mind either way: whatever you prefer.

[Devlin, 2017-03-21 22:51:20]

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

https://codereview.chromium.org/2762623003/diff/40001/extensions/renderer/api...
extensions/renderer/api_binding_js_util_unittest.cc:49: return "[empty]";
On 2017/03/21 22:02:46, jbroman wrote:
> Getting a missing property yields undefined, not an empty handle. Unless an
> exception is thrown, you should _not_ get an empty handle here. (As it
happens,
> if you do get an empty handle here, GetPropertyFromObject has already added a
> failure due to EXPECT_TRUE.)

Right; that's what I was saying...  I think maybe I misunderstood your original
comment.  I thought you meant you found it confusing that we were handling
exceptions by returning "[empty]" (which we're not doing - we handle exceptions
above on line 40).  Did you just mean you found the string "[empty]" strange, as
opposed to, say, "[undefined]"?

(changed to "[undefined]")

https://codereview.chromium.org/2762623003/diff/60001/extensions/renderer/api...
File extensions/renderer/api_binding_js_util.cc (right):

https://codereview.chromium.org/2762623003/diff/60001/extensions/renderer/api...
extensions/renderer/api_binding_js_util.cc:132:
request_handler_->last_error()->SetError(context, error);
On 2017/03/21 22:02:47, jbroman wrote:
> nit: Since the gin::Wrappable isn't shared between contexts, this would also
> suffice (this wrappable's wrapper is the holder, though it does require
opening
> one more v8 handle and so might be epsilon slower?):
> 
> void APIBindingJSUtil::SetLastError(v8::Isolate* isolate,
>                                     const std::string& error) {
>   request_handler_->last_error()->SetError(
>       GetWrapper(isolate)->CreationContext(), error);
> }
> 
> I don't mind either way: whatever you prefer.

Oooh, fancy.  For consistency's sake, I'll stick with this for now, but I may
make a sweeping change to use that instead.

[jbroman, 2017-03-22 18:15:46]

lgtm

[Devlin, 2017-03-23 22:11:21]

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

[commit-bot: I haz the power, 2017-03-23 22:12:07]

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-03-23 23:17:34]

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

[commit-bot: I haz the power, 2017-03-23 23:17:35]

Dry run: This issue passed the CQ dry run.

[lazyboy, 2017-03-24 01:19:52]

lgtm

[Devlin, 2017-03-24 01:33:06]

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

[Devlin, 2017-03-24 01:33:06]

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/2762623003/#ps100001
(title: "Rebase")

[commit-bot: I haz the power, 2017-03-24 01:33:31]

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-03-24 01:41:48]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1490319186300230,
"parent_rev": "d6186299e31a04599eee2e3e2c4dbc9003be40eb", "commit_rev":
"2615ab3076703e2d4e409370ba32c46ab0aa170c"}

[commit-bot: I haz the power, 2017-03-24 01:42:28]

Description was changed from

==========
[Extensions Bindings] Add lastError utilities to APIBindingJSUtil

Some custom bindings require the ability to query or mutate the
lastError state. Provide native bindings to perform the following
functions:
- setLastError
- clearLastError
- hasLastError
- runCallbackWithLastError (sets the last error, runs the provided
  callback, and then clears the last error).

Add tests for the same.

Note that in the JS bindings, the analogous methods also accept a
context "chrome" object. This is bad, since its only use is allowing
cross-context modification. We should see if we can eliminate it.

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

to

==========
[Extensions Bindings] Add lastError utilities to APIBindingJSUtil

Some custom bindings require the ability to query or mutate the
lastError state. Provide native bindings to perform the following
functions:
- setLastError
- clearLastError
- hasLastError
- runCallbackWithLastError (sets the last error, runs the provided
  callback, and then clears the last error).

Add tests for the same.

Note that in the JS bindings, the analogous methods also accept a
context "chrome" object. This is bad, since its only use is allowing
cross-context modification. We should see if we can eliminate it.

BUG=653596

Review-Url: https://codereview.chromium.org/2762623003
Cr-Commit-Position: refs/heads/master@{#459334}
Committed:
https://chromium.googlesource.com/chromium/src/+/2615ab3076703e2d4e409370ba32...
==========

[commit-bot: I haz the power, 2017-03-24 01:42:32]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/2615ab3076703e2d4e409370ba32...