[Extensions Bindings] Move request dispatch to APIRequestHandler

[Extensions Bindings] Move request dispatch to APIRequestHandler

Move the logic to dispatch the request (via a curried-in callback) to
APIRequestHandler instead of APIBinding. This has a few advantages:
- Makes a bit more sense; the request handler should handle the
  requests.
- Reduces a bit of logic in the very-busy APIBinding.
- Will make subsequent CLs (dispatching requests from custom types)
  easier.
The only reason the logic currently lives in APIBinding is because it
predates the APIRequestHandler.

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

[Devlin, 2017-02-17 01:08:07]

Description was changed from

==========
Request
churn

BUG=
==========

to

==========
[Extensions Bindings] Move request dispatch to APIRequestHandler

Move the logic to dispatch the request (via a curried-in callback) to
APIRequestHandler instead of APIBinding. This has a few advantages:
- Makes a bit more sense; the request handler should handle the
  requests.
- Reduces a bit of logic in the very-busy APIBinding.
- Will make subsequent CLs (dispatching requests from custom types)
  easier.
The only reason the logic currently lives in APIBinding is because it
predates the APIRequestHandler.

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

[Devlin, 2017-02-17 01:08:16]

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

[commit-bot: I haz the power, 2017-02-17 01:10:44]

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-02-17 03:13:59]

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

[commit-bot: I haz the power, 2017-02-17 03:14:00]

Dry run: Try jobs failed on following builders:
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)

[Devlin, 2017-02-17 16:49:39]

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

[Devlin, 2017-02-17 16:49:40]

Just moving stuff around; no functionality changes.

[jbroman, 2017-02-17 18:25:07]

lgtm

[lazyboy, 2017-02-17 19:47:14]

lgtm

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

https://codereview.chromium.org/2697363003/diff/20001/extensions/renderer/api...
extensions/renderer/api_request_handler.cc:19:
APIRequestHandler::Request::~Request() = default;
Is there any particular reason to choose "default" instead of {}?

[Devlin, 2017-02-17 21:57:42]

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

https://codereview.chromium.org/2697363003/diff/20001/extensions/renderer/api...
extensions/renderer/api_request_handler.cc:19:
APIRequestHandler::Request::~Request() = default;
On 2017/02/17 19:47:13, lazyboy wrote:
> Is there any particular reason to choose "default" instead of {}?

Good question.  I've noticed a trend towards using default for destructors, but
in looking through the style, I can't find any indication that one is preferred
over the other (and both are allowed).  Do you have a preference in either
direction?

(Or Jeremy, do you know if there's a reason to use one over the other?)

[Devlin, 2017-02-17 22:06:25]

(asked lazyboy@ offline; landing this as-is for now, but will investigate more
for a general answer)

[Devlin, 2017-02-17 22:06:29]

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

[lazyboy, 2017-02-17 22:06:55]

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

https://codereview.chromium.org/2697363003/diff/20001/extensions/renderer/api...
extensions/renderer/api_request_handler.cc:19:
APIRequestHandler::Request::~Request() = default;
On 2017/02/17 21:57:41, Devlin wrote:
> On 2017/02/17 19:47:13, lazyboy wrote:
> > Is there any particular reason to choose "default" instead of {}?
> 
> Good question.  I've noticed a trend towards using default for destructors,
but
> in looking through the style, I can't find any indication that one is
preferred
> over the other (and both are allowed).  Do you have a preference in either
> direction?

This is fine, I was just curious if this was was intentional for some reason and
I wanted to learn...
lgtm
> 
> (Or Jeremy, do you know if there's a reason to use one over the other?)

[commit-bot: I haz the power, 2017-02-17 22:08:10]

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-02-17 23:49:15]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1487369189690710,
"parent_rev": "63f826afd2936707c9c697ce774a65e783041db7", "commit_rev":
"2f81ad2e573a460938c93cdb022b40034ac7fb5f"}

[commit-bot: I haz the power, 2017-02-17 23:50:13]

Description was changed from

==========
[Extensions Bindings] Move request dispatch to APIRequestHandler

Move the logic to dispatch the request (via a curried-in callback) to
APIRequestHandler instead of APIBinding. This has a few advantages:
- Makes a bit more sense; the request handler should handle the
  requests.
- Reduces a bit of logic in the very-busy APIBinding.
- Will make subsequent CLs (dispatching requests from custom types)
  easier.
The only reason the logic currently lives in APIBinding is because it
predates the APIRequestHandler.

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

to

==========
[Extensions Bindings] Move request dispatch to APIRequestHandler

Move the logic to dispatch the request (via a curried-in callback) to
APIRequestHandler instead of APIBinding. This has a few advantages:
- Makes a bit more sense; the request handler should handle the
  requests.
- Reduces a bit of logic in the very-busy APIBinding.
- Will make subsequent CLs (dispatching requests from custom types)
  easier.
The only reason the logic currently lives in APIBinding is because it
predates the APIRequestHandler.

BUG=653596

Review-Url: https://codereview.chromium.org/2697363003
Cr-Commit-Position: refs/heads/master@{#451403}
Committed:
https://chromium.googlesource.com/chromium/src/+/2f81ad2e573a460938c93cdb022b...
==========

[commit-bot: I haz the power, 2017-02-17 23:50:15]

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

[jbroman, 2017-02-21 00:55:55]

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

https://codereview.chromium.org/2697363003/diff/20001/extensions/renderer/api...
extensions/renderer/api_request_handler.cc:19:
APIRequestHandler::Request::~Request() = default;
On 2017/02/17 at 22:06:55, lazyboy wrote:
> On 2017/02/17 21:57:41, Devlin wrote:
> > On 2017/02/17 19:47:13, lazyboy wrote:
> > > Is there any particular reason to choose "default" instead of {}?
> > 
> > Good question.  I've noticed a trend towards using default for destructors,
but
> > in looking through the style, I can't find any indication that one is
preferred
> > over the other (and both are allowed).  Do you have a preference in either
> > direction?
> 
> This is fine, I was just curious if this was was intentional for some reason
and I wanted to learn...
> lgtm
> > 
> > (Or Jeremy, do you know if there's a reason to use one over the other?)

I don't have a preference one way or another, and the style guide is silent
about it. "= default;" is more helpful for copy/move constructors and assignment
operators, but for destructors it's fine either way.