[Extensions Bindings] Include request id in a custom callback response

[Extensions Bindings] Include request id in a custom callback response

Include the request id in the args curried to the custom callback set
with custom bindings. This is only used in pageCapture.saveAsMHTML, but
including it is trivial. When we move pageCapture.saveAsMHTML to
fully-native bindings, it may make sense to remove this.

BUG=653596
TEST=APIRequestHandlerTest*,
     ExtensionPageCaptureApiTest.SaveAsMHTML --native-crx-bindings=1

Review-Url: https://codereview.chromium.org/2894923003
Cr-Commit-Position: refs/heads/master@{#475666}
Committed: https://chromium.googlesource.com/chromium/src/+/5620d9efdeb210b6d27b041783f173dae3e5e37b

[Devlin, 2017-05-19 20:59:00]

Description was changed from

==========
[Extensions Bindings] Include request id in a custom callback response

Include the request id in the args curried to the custom callback set
with custom bindings. This is only used in pageCapture.saveAsMHTML, but
including it is trivial. When we move pageCapture.saveAsMHTML to
fully-native bindings, it may make sense to remove this.

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

to

==========
[Extensions Bindings] Include request id in a custom callback response

Include the request id in the args curried to the custom callback set
with custom bindings. This is only used in pageCapture.saveAsMHTML, but
including it is trivial. When we move pageCapture.saveAsMHTML to
fully-native bindings, it may make sense to remove this.

BUG=653596
TEST=APIRequestHandlerTest*,
     ExtensionPageCaptureApiTest.SaveAsMHTML --native-crx-bindings=1
==========

[Devlin, 2017-05-19 20:59:13]

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

[Devlin, 2017-05-19 20:59:59]

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

[Devlin, 2017-05-19 20:59:59]

drilling through test failures... :)

[commit-bot: I haz the power, 2017-05-19 21:00:18]

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-05-19 22:00:28]

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

[commit-bot: I haz the power, 2017-05-19 22:00:28]

Dry run: This issue passed the CQ dry run.

[jbroman, 2017-05-23 22:15:03]

https://codereview.chromium.org/2894923003/diff/1/extensions/renderer/api_req...
File extensions/renderer/api_request_handler.cc (right):

https://codereview.chromium.org/2894923003/diff/1/extensions/renderer/api_req...
extensions/renderer/api_request_handler.cc:61: int request_id =
next_request_id_++;
Should this be inside the if condition? It's not used outside it, so this seems
to increment it without doing anything else. If it should be here, comment?

[Devlin, 2017-05-24 16:08:34]

https://codereview.chromium.org/2894923003/diff/1/extensions/renderer/api_req...
File extensions/renderer/api_request_handler.cc (right):

https://codereview.chromium.org/2894923003/diff/1/extensions/renderer/api_req...
extensions/renderer/api_request_handler.cc:61: int request_id =
next_request_id_++;
On 2017/05/23 22:15:03, jbroman wrote:
> Should this be inside the if condition? It's not used outside it, so this
seems
> to increment it without doing anything else. If it should be here, comment?

The browser side uses these identifiers, even though it's not the best practice.
 I've added a comment and a TODO.

[jbroman, 2017-05-24 16:54:38]

https://codereview.chromium.org/2894923003/diff/20001/extensions/renderer/api...
File extensions/renderer/api_request_handler.cc (right):

https://codereview.chromium.org/2894923003/diff/20001/extensions/renderer/api...
extensions/renderer/api_request_handler.cc:110: int id = request->request_id;
To clarify my question, |request_id| isn't used here (or below) -- if it wasn't
populated above, it will be -1. Do we need to increment |next_request_id_| in
the case where |custom_callback.IsEmpty() && callback.IsEmpty()|?

[Devlin, 2017-05-24 17:31:40]

https://codereview.chromium.org/2894923003/diff/20001/extensions/renderer/api...
File extensions/renderer/api_request_handler.cc (right):

https://codereview.chromium.org/2894923003/diff/20001/extensions/renderer/api...
extensions/renderer/api_request_handler.cc:110: int id = request->request_id;
On 2017/05/24 16:54:38, jbroman wrote:
> To clarify my question, |request_id| isn't used here (or below) -- if it
wasn't
> populated above, it will be -1. Do we need to increment |next_request_id_| in
> the case where |custom_callback.IsEmpty() && callback.IsEmpty()|?

Oh, I see.  Good catch; this was a bug.  Fixed and added a test.

[jbroman, 2017-05-24 21:02:55]

lgtm

[lazyboy, 2017-05-30 18:05:11]

lgtm

[Devlin, 2017-05-30 19:07:45]

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

[commit-bot: I haz the power, 2017-05-30 19:08:46]

Dry run: CQ is trying da patch.

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

[Devlin, 2017-05-30 20:35:31]

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

[Devlin, 2017-05-30 20:35:34]

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

[commit-bot: I haz the power, 2017-05-30 20:36:21]

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-30 20:57:41]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1496176534222090,
"parent_rev": "e9481b6417f18f946c21e1a2049c7052caf0c8e7", "commit_rev":
"5620d9efdeb210b6d27b041783f173dae3e5e37b"}

[commit-bot: I haz the power, 2017-05-30 20:57:50]

Description was changed from

==========
[Extensions Bindings] Include request id in a custom callback response

Include the request id in the args curried to the custom callback set
with custom bindings. This is only used in pageCapture.saveAsMHTML, but
including it is trivial. When we move pageCapture.saveAsMHTML to
fully-native bindings, it may make sense to remove this.

BUG=653596
TEST=APIRequestHandlerTest*,
     ExtensionPageCaptureApiTest.SaveAsMHTML --native-crx-bindings=1
==========

to

==========
[Extensions Bindings] Include request id in a custom callback response

Include the request id in the args curried to the custom callback set
with custom bindings. This is only used in pageCapture.saveAsMHTML, but
including it is trivial. When we move pageCapture.saveAsMHTML to
fully-native bindings, it may make sense to remove this.

BUG=653596
TEST=APIRequestHandlerTest*,
     ExtensionPageCaptureApiTest.SaveAsMHTML --native-crx-bindings=1

Review-Url: https://codereview.chromium.org/2894923003
Cr-Commit-Position: refs/heads/master@{#475666}
Committed:
https://chromium.googlesource.com/chromium/src/+/5620d9efdeb210b6d27b041783f1...
==========

[commit-bot: I haz the power, 2017-05-30 20:57:51]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/5620d9efdeb210b6d27b041783f1...