Split SchemaGeneratedBindings up into smaller, more targetted native handlers.

Split SchemaGeneratedBindings up into smaller, more targetted native handlers.


BUG=104100
TEST=existing browser tests


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=130855

[koz (OOO until 15th September), 2012-03-29 06:41:16]

This is based off https://chromiumcodereview.appspot.com/9903010/

It wasn't written at the time that I uploaded the previous patch, so let me know
if you want me to merge them.

[benwells, 2012-03-29 23:12:30]

more drive by

https://chromiumcodereview.appspot.com/9918006/diff/1013/chrome/renderer/exte...
File chrome/renderer/extensions/send_request_natives.cc (right):

https://chromiumcodereview.appspot.com/9918006/diff/1013/chrome/renderer/exte...
chrome/renderer/extensions/send_request_natives.cc:8: #include
"chrome/renderer/extensions/extension_request_sender.h"
Do you need to split the sender out into another file now, or could it be
private to this class?

[koz (OOO until 15th September), 2012-03-30 00:08:32]

Cheers.

https://chromiumcodereview.appspot.com/9918006/diff/1013/chrome/renderer/exte...
File chrome/renderer/extensions/send_request_natives.cc (right):

https://chromiumcodereview.appspot.com/9918006/diff/1013/chrome/renderer/exte...
chrome/renderer/extensions/send_request_natives.cc:8: #include
"chrome/renderer/extensions/extension_request_sender.h"
On 2012/03/29 23:12:30, benwells wrote:
> Do you need to split the sender out into another file now, or could it be
> private to this class?

No, because ExtensionRequestSender is depended on by this class as well as
SetIconNatives. It would be possible to have SetIconNatives take a
SendRequestNatives as an argument and delegate to it, but that's kind of
stretching the responsibility of NativeHandlers. They are intended to expose
functionality to JS code, not to expose functionality to other C++ classes.

The rationale behind that line of reasoning is made clear in this case because
we have many different contexts (which each have their own instances of
ModuleSystem and all of the NativeHandlers) that all need to share the same
ExtensionRequestSender, so making ERS private to this NativeHandler would give
us the wrong lifetime semantics - each context would have its own request_id
pool, leading to ambiguity when different contexts make requests.

[not at google - send to devlin, 2012-03-30 03:23:18]

lgtm

http://codereview.chromium.org/9918006/diff/9001/chrome/renderer/extensions/a...
File chrome/renderer/extensions/api_definitions_natives.h (right):

http://codereview.chromium.org/9918006/diff/9001/chrome/renderer/extensions/a...
chrome/renderer/extensions/api_definitions_natives.h:16: class
ApiDefinitionsNatives : public ChromeV8Extension {
docs

http://codereview.chromium.org/9918006/diff/9001/chrome/renderer/extensions/a...
chrome/renderer/extensions/api_definitions_natives.h:21: v8::Handle<v8::Value>
GetExtensionAPIDefinition(const v8::Arguments& args);
DISALLOW_etc

http://codereview.chromium.org/9918006/diff/9001/chrome/renderer/extensions/e...
File chrome/renderer/extensions/extension_dispatcher.cc (right):

http://codereview.chromium.org/9918006/diff/9001/chrome/renderer/extensions/e...
chrome/renderer/extensions/extension_dispatcher.cc:72: using
extensions::SetIconNatives;
:sort

http://codereview.chromium.org/9918006/diff/9001/chrome/renderer/extensions/s...
File chrome/renderer/extensions/send_request_natives.cc (right):

http://codereview.chromium.org/9918006/diff/9001/chrome/renderer/extensions/s...
chrome/renderer/extensions/send_request_natives.cc:31: v8::Handle<v8::Value>
SendRequestNatives::StartRequestCommon(
method seems redundant now, can just include it as part of StartRequest?

http://codereview.chromium.org/9918006/diff/9001/chrome/renderer/extensions/s...
File chrome/renderer/extensions/send_request_natives.h (right):

http://codereview.chromium.org/9918006/diff/9001/chrome/renderer/extensions/s...
chrome/renderer/extensions/send_request_natives.h:18: class SendRequestNatives :
public ChromeV8Extension {
docs

http://codereview.chromium.org/9918006/diff/9001/chrome/renderer/extensions/s...
chrome/renderer/extensions/send_request_natives.h:31: ExtensionRequestSender*
request_sender_;
DISALLOW_etc

http://codereview.chromium.org/9918006/diff/9001/chrome/renderer/extensions/s...
File chrome/renderer/extensions/set_icon_natives.h (right):

http://codereview.chromium.org/9918006/diff/9001/chrome/renderer/extensions/s...
chrome/renderer/extensions/set_icon_natives.h:22: class SetIconNatives : public
ChromeV8Extension {
docs and DISALLOW_etc

http://codereview.chromium.org/9918006/diff/9001/chrome/renderer/resources/ex...
File chrome/renderer/resources/extensions/send_request.js (right):

http://codereview.chromium.org/9918006/diff/9001/chrome/renderer/resources/ex...
chrome/renderer/resources/extensions/send_request.js:8: var StartRequest =
natives.StartRequest;
i reckon a bit cleaner to just require this as "sendRequest" then use
sendRequest.GetNextRequestId / sendRequest.StartRequest directly. The fewer
things up here the better.

[benwells, 2012-03-30 04:35:07]

lgtm

[koz (OOO until 15th September), 2012-04-03 00:15:27]

Cheers.

http://codereview.chromium.org/9918006/diff/9001/chrome/renderer/extensions/a...
File chrome/renderer/extensions/api_definitions_natives.h (right):

http://codereview.chromium.org/9918006/diff/9001/chrome/renderer/extensions/a...
chrome/renderer/extensions/api_definitions_natives.h:16: class
ApiDefinitionsNatives : public ChromeV8Extension {
On 2012/03/30 03:23:18, kalman wrote:
> docs

Done.

http://codereview.chromium.org/9918006/diff/9001/chrome/renderer/extensions/a...
chrome/renderer/extensions/api_definitions_natives.h:21: v8::Handle<v8::Value>
GetExtensionAPIDefinition(const v8::Arguments& args);
On 2012/03/30 03:23:18, kalman wrote:
> DISALLOW_etc

Done.

http://codereview.chromium.org/9918006/diff/9001/chrome/renderer/extensions/e...
File chrome/renderer/extensions/extension_dispatcher.cc (right):

http://codereview.chromium.org/9918006/diff/9001/chrome/renderer/extensions/e...
chrome/renderer/extensions/extension_dispatcher.cc:72: using
extensions::SetIconNatives;
On 2012/03/30 03:23:18, kalman wrote:
> :sort

Done.

http://codereview.chromium.org/9918006/diff/9001/chrome/renderer/extensions/s...
File chrome/renderer/extensions/send_request_natives.cc (right):

http://codereview.chromium.org/9918006/diff/9001/chrome/renderer/extensions/s...
chrome/renderer/extensions/send_request_natives.cc:31: v8::Handle<v8::Value>
SendRequestNatives::StartRequestCommon(
On 2012/03/30 03:23:18, kalman wrote:
> method seems redundant now, can just include it as part of StartRequest?

Done.

http://codereview.chromium.org/9918006/diff/9001/chrome/renderer/extensions/s...
File chrome/renderer/extensions/send_request_natives.h (right):

http://codereview.chromium.org/9918006/diff/9001/chrome/renderer/extensions/s...
chrome/renderer/extensions/send_request_natives.h:18: class SendRequestNatives :
public ChromeV8Extension {
On 2012/03/30 03:23:18, kalman wrote:
> docs

Done.

http://codereview.chromium.org/9918006/diff/9001/chrome/renderer/extensions/s...
chrome/renderer/extensions/send_request_natives.h:31: ExtensionRequestSender*
request_sender_;
On 2012/03/30 03:23:18, kalman wrote:
> DISALLOW_etc

Done.

http://codereview.chromium.org/9918006/diff/9001/chrome/renderer/extensions/s...
File chrome/renderer/extensions/set_icon_natives.h (right):

http://codereview.chromium.org/9918006/diff/9001/chrome/renderer/extensions/s...
chrome/renderer/extensions/set_icon_natives.h:22: class SetIconNatives : public
ChromeV8Extension {
On 2012/03/30 03:23:18, kalman wrote:
> docs and DISALLOW_etc 

Done.

http://codereview.chromium.org/9918006/diff/9001/chrome/renderer/resources/ex...
File chrome/renderer/resources/extensions/send_request.js (right):

http://codereview.chromium.org/9918006/diff/9001/chrome/renderer/resources/ex...
chrome/renderer/resources/extensions/send_request.js:8: var StartRequest =
natives.StartRequest;
On 2012/03/30 03:23:18, kalman wrote:
> i reckon a bit cleaner to just require this as "sendRequest" then use
> sendRequest.GetNextRequestId / sendRequest.StartRequest directly. The fewer
> things up here the better.

Cool, but as sendRequest() is already the name of the function below I've left
the name as natives.

[commit-bot: I haz the power, 2012-04-04 01:01:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/koz@chromium.org/9918006/14004

[commit-bot: I haz the power, 2012-04-04 01:29:33]

Try job failure for 9918006-14004 (retry) on linux_chromeos for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2012-04-04 03:16:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/koz@chromium.org/9918006/16004

[commit-bot: I haz the power, 2012-04-04 06:27:05]

Try job failure for 9918006-16004 (retry) on win_rel for steps "browser_tests,
unit_tests".
It's a second try, previously, steps "browser_tests, unit_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2012-04-04 06:30:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/koz@chromium.org/9918006/16004

[commit-bot: I haz the power, 2012-04-04 09:48:45]

Try job failure for 9918006-16004 (retry) on win_rel for steps "browser_tests,
unit_tests".
It's a second try, previously, steps "browser_tests, unit_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2012-04-04 23:57:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/koz@chromium.org/9918006/16004

[commit-bot: I haz the power, 2012-04-05 00:18:24]

Try job failure for 9918006-16004 (retry) on linux_chromeos for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2012-04-05 05:10:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/koz@chromium.org/9918006/27001

[commit-bot: I haz the power, 2012-04-05 07:19:31]

Change committed as 130855