[Extensions Bindings] Support chrome.extension.lastError

[Extensions Bindings] Support chrome.extension.lastError

For legacy reasons, we surface errors through both
chrome.runtime.lastError and chrome.extension.lastError (though the
latter is both less fancy [it doesn't check for access] and also not
available in all contexts). Allow for providing a "secondary parent"
object to the APILastError object to support this.

Add unittests and end-to-end tests for the same.

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

[Devlin, 2017-06-22 21:32:50]

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

[commit-bot: I haz the power, 2017-06-22 21:33:12]

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-06-22 22:49:00]

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

[commit-bot: I haz the power, 2017-06-22 22:49:01]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[Devlin, 2017-06-24 01:23:12]

Description was changed from

==========
extension.lastError
BUG=
==========

to

==========
[Extensions Bindings] Support chrome.extension.lastError

For legacy reasons, we surface errors through both
chrome.runtime.lastError and chrome.extension.lastError (though the
latter is both less fancy [it doesn't check for access] and also not
available in all contexts). Allow for providing a "secondary parent"
object to the APILastError object to support this.

Add unittests and end-to-end tests for the same.

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

[Devlin, 2017-06-24 01:24:16]

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

[Devlin, 2017-06-24 01:24:18]

Hey folks, mind taking a look?

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

https://codereview.chromium.org/2953133002/diff/40001/extensions/renderer/api...
extensions/renderer/api_last_error.cc:219: if (parent.IsEmpty())
Note: copy-paste from above.

[jbroman, 2017-06-25 00:12:49]

lgtm

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

https://codereview.chromium.org/2953133002/diff/40001/extensions/renderer/api...
extensions/renderer/api_last_error.cc:255: // This Set() can fail, but there's
nothing to do if it does (the exception
nit: this is SetAccessor, not Set.

https://codereview.chromium.org/2953133002/diff/40001/extensions/renderer/api...
extensions/renderer/api_last_error.cc:256: // will be caught by the TryCatch
above).
Here and below: is "the TryCatch above" the one in APILastError::SetError?

https://codereview.chromium.org/2953133002/diff/40001/extensions/renderer/api...
extensions/renderer/api_last_error.cc:277:
ignore_result(secondary_parent->CreateDataProperty(
nit: this is CreateDataProperty, not Set.

[lazyboy, 2017-06-27 17:58:44]

lgtm

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

https://codereview.chromium.org/2953133002/diff/40001/extensions/renderer/api...
extensions/renderer/api_bindings_system_unittest.h:50: virtual
v8::Local<v8::Object> GetLastErrorParent(
Update documentation.

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

https://codereview.chromium.org/2953133002/diff/40001/extensions/renderer/api...
extensions/renderer/api_last_error.cc:158: parent = get_parent_.Run(context,
&secondary_parent);
It feels a bit odd to return two parents differently from GetParent: one through
return value and the other through a pointer arg. Returning both through pointer
arg seems consistent,  up to you.

[Devlin, 2017-06-28 16:25:25]

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

[commit-bot: I haz the power, 2017-06-28 16:25:45]

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-06-28 17:50:19]

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

[commit-bot: I haz the power, 2017-06-28 17:50:20]

Dry run: This issue passed the CQ dry run.

[Devlin, 2017-06-28 17:52:01]

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

https://codereview.chromium.org/2953133002/diff/40001/extensions/renderer/api...
extensions/renderer/api_bindings_system_unittest.h:50: virtual
v8::Local<v8::Object> GetLastErrorParent(
On 2017/06/27 17:58:44, lazyboy wrote:
> Update documentation.

Done.

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

https://codereview.chromium.org/2953133002/diff/40001/extensions/renderer/api...
extensions/renderer/api_last_error.cc:158: parent = get_parent_.Run(context,
&secondary_parent);
On 2017/06/27 17:58:44, lazyboy wrote:
> It feels a bit odd to return two parents differently from GetParent: one
through
> return value and the other through a pointer arg. Returning both through
pointer
> arg seems consistent,  up to you.

I have a slight preference for this, since the primary parent should almost
never be null, whereas the secondary parent might be.  If it's alright, I'd like
to keep it as-is.

https://codereview.chromium.org/2953133002/diff/40001/extensions/renderer/api...
extensions/renderer/api_last_error.cc:255: // This Set() can fail, but there's
nothing to do if it does (the exception
On 2017/06/25 00:12:49, jbroman wrote:
> nit: this is SetAccessor, not Set.

Done.

https://codereview.chromium.org/2953133002/diff/40001/extensions/renderer/api...
extensions/renderer/api_last_error.cc:256: // will be caught by the TryCatch
above).
On 2017/06/25 00:12:49, jbroman wrote:
> Here and below: is "the TryCatch above" the one in APILastError::SetError?

Yep, clarified in the comments.

https://codereview.chromium.org/2953133002/diff/40001/extensions/renderer/api...
extensions/renderer/api_last_error.cc:277:
ignore_result(secondary_parent->CreateDataProperty(
On 2017/06/25 00:12:49, jbroman wrote:
> nit: this is CreateDataProperty, not Set.

Done.

[Devlin, 2017-06-28 17:52:07]

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

[Devlin, 2017-06-28 17:52:07]

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/2953133002/#ps60001
(title: "jbroman's, lazyboy's")

[commit-bot: I haz the power, 2017-06-28 17:52:35]

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-06-28 17:55:44]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1498672327054040,
"parent_rev": "6f2691cc7756d599ce5622cb7c9fc854afcf3c57", "commit_rev":
"63bbcde542abb20fbadae32ddf2008811a3068ed"}

[commit-bot: I haz the power, 2017-06-28 17:55:58]

Description was changed from

==========
[Extensions Bindings] Support chrome.extension.lastError

For legacy reasons, we surface errors through both
chrome.runtime.lastError and chrome.extension.lastError (though the
latter is both less fancy [it doesn't check for access] and also not
available in all contexts). Allow for providing a "secondary parent"
object to the APILastError object to support this.

Add unittests and end-to-end tests for the same.

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

to

==========
[Extensions Bindings] Support chrome.extension.lastError

For legacy reasons, we surface errors through both
chrome.runtime.lastError and chrome.extension.lastError (though the
latter is both less fancy [it doesn't check for access] and also not
available in all contexts). Allow for providing a "secondary parent"
object to the APILastError object to support this.

Add unittests and end-to-end tests for the same.

BUG=653596

Review-Url: https://codereview.chromium.org/2953133002
Cr-Commit-Position: refs/heads/master@{#483055}
Committed:
https://chromium.googlesource.com/chromium/src/+/63bbcde542abb20fbadae32ddf20...
==========

[commit-bot: I haz the power, 2017-06-28 17:56:02]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/63bbcde542abb20fbadae32ddf20...