Extract ExtensionFunctionRegistry from ExtensionFunctionDispatcher.

Refactor extension_function_dispatcher to extract ExtensionFunctionRegistry. This allows us to generate an additional code block that takes an ExtensionFunctionRegistry and asks it to register generated API functions.

Then switch DnsResolve over to get registered this way.

Along the way, notice that DNSResolve is capitalized using an untenable style. Fix that.

BUG=none (essential plumbing work)
TEST=same as before

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

[miket_OOO, 2012-03-14 00:00:25]

http://codereview.chromium.org/9666059/diff/14016/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_factory_registry.cc (left):

http://codereview.chromium.org/9666059/diff/14016/chrome/browser/extensions/e...
chrome/browser/extensions/extension_factory_registry.cc:22: #include
"chrome/browser/extensions/api/dns/dns_api.h"
This is one line of manually written code that was removed.

http://codereview.chromium.org/9666059/diff/14016/chrome/browser/extensions/e...
chrome/browser/extensions/extension_factory_registry.cc:521:
RegisterFunction<extensions::DNSResolveFunction>();
This is the other line.

[asargent_no_longer_on_chrome, 2012-03-14 22:46:07]

LGTM

http://codereview.chromium.org/9666059/diff/17001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_function_dispatcher.cc (left):

http://codereview.chromium.org/9666059/diff/17001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_function_dispatcher.cc:115: class
FactoryRegistry {
nit: can you change the first line of the description of the CL to something
like "make FactoryRegistry available outside of ExtensionFunctionDispatcher"?
That's a little more descriptive of what's actually happening here, and then the
part about the removal of a line of code can be in the rest of the description.

http://codereview.chromium.org/9666059/diff/17001/tools/json_schema_compiler/...
File tools/json_schema_compiler/compiler.py (right):

http://codereview.chromium.org/9666059/diff/17001/tools/json_schema_compiler/...
tools/json_schema_compiler/compiler.py:124: .Generate().Render())
nit: the indentation looks a little weird here. Even if it is technically
correct, could we split this up into a couple of lines with temp variables to
hold intermediate results? Something like:

generator = h_bundle_generator.HBundleGenerator(api_model,
                                                type_generator)
h_bundle_code = generator.Generate().Render()

http://codereview.chromium.org/9666059/diff/16002/tools/json_schema_compiler/...
File tools/json_schema_compiler/h_bundle_generator.py (right):

http://codereview.chromium.org/9666059/diff/16002/tools/json_schema_compiler/...
tools/json_schema_compiler/h_bundle_generator.py:77: c.Eblock("};")
Seeing this code (Sblock/Append/Eblock, etc.) made me wish we had some sort of
templating solution, even just a simple one that used placeholders and had no
loop or other fancy stuff supported. This is ok for now, but perhaps we can
think about a future enhancement to the entire compiler.

[miket_OOO, 2012-03-14 22:55:16]

http://codereview.chromium.org/9666059/diff/17001/tools/json_schema_compiler/...
File tools/json_schema_compiler/compiler.py (right):

http://codereview.chromium.org/9666059/diff/17001/tools/json_schema_compiler/...
tools/json_schema_compiler/compiler.py:124: .Generate().Render())
On 2012/03/14 22:46:07, Antony Sargent wrote:
> nit: the indentation looks a little weird here. Even if it is technically
> correct, could we split this up into a couple of lines with temp variables to
> hold intermediate results? Something like:
> 
> generator = h_bundle_generator.HBundleGenerator(api_model,
>                                                 type_generator)
> h_bundle_code = generator.Generate().Render()

Done.

http://codereview.chromium.org/9666059/diff/16002/tools/json_schema_compiler/...
File tools/json_schema_compiler/h_bundle_generator.py (right):

http://codereview.chromium.org/9666059/diff/16002/tools/json_schema_compiler/...
tools/json_schema_compiler/h_bundle_generator.py:77: c.Eblock("};")
On 2012/03/14 22:46:07, Antony Sargent wrote:
> Seeing this code (Sblock/Append/Eblock, etc.) made me wish we had some sort of
> templating solution, even just a simple one that used placeholders and had no
> loop or other fancy stuff supported. This is ok for now, but perhaps we can
> think about a future enhancement to the entire compiler. 

It's possible the right way is to refactor this code into smaller code
generators subroutines, so the readability from indentation still occurs, but
it's in the host language rather than the target language.

[commit-bot: I haz the power, 2012-03-14 23:07:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/miket@chromium.org/9666059/10020

[commit-bot: I haz the power, 2012-03-15 01:47:22]

Try job failure for 9666059-10020 (retry) on mac_rel for step "browser_tests".
It's a second try, previously, step "browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[Aaron Boodman, 2012-03-15 02:27:47]

http://codereview.chromium.org/9666059/diff/10020/chrome/browser/extensions/a...
File chrome/browser/extensions/api/dns/dns_api.h (right):

http://codereview.chromium.org/9666059/diff/10020/chrome/browser/extensions/a...
chrome/browser/extensions/api/dns/dns_api.h:22: class DnsResolveFunction :
public AsyncExtensionFunction {
No!

$ cd chrome
$ git grep Url | wc -l
5628
$ git grep URL | wc -l
41165
$ git grep Dns | wc -l
264
$ git grep DNS | wc -l
882

http://codereview.chromium.org/9666059/diff/10020/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_factory_registry.h (right):

http://codereview.chromium.org/9666059/diff/10020/chrome/browser/extensions/e...
chrome/browser/extensions/extension_factory_registry.h:5: #ifndef
CHROME_BROWSER_EXTENSIONS_EXTENSION_FACTORY_REGISTRY_H_
Sadly, it is actually an ExtensionFunction Factory Registry.

The "factory" bit is not really important though. So we could just call it
ExtensionFunctionRegistry.

http://codereview.chromium.org/9666059/diff/10020/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_function_dispatcher.h (right):

http://codereview.chromium.org/9666059/diff/10020/chrome/browser/extensions/e...
chrome/browser/extensions/extension_function_dispatcher.h:13: #include
"chrome/browser/extensions/extension_factory_registry.h"
You could forward declare the factory to avoid this include.

[miket_OOO, 2012-03-15 16:01:25]

> chrome/browser/extensions/api/dns/dns_api.h:22: class DnsResolveFunction :
> public AsyncExtensionFunction {
> No!

Antony, Mihai, and I had extensive discussion about this in person. Two points:

1.
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Type_N...

2. We are generating these function names from IDL, and the IDL uses a
JavaScript-like lowercase scheme: dns.resolve, somewhere.doSomething, etc. So if
we want to preserve this inconsistent capitalization for DNS, we'd presumably
have to change to dNS.resolve, which looks ridiculous (and might become
d_n_s.resolve in other parts of the code, which looks even more ridiculous), or
hardcode certain acronyms because they're word-like (URL, DNS) but not
capitalized, which feels like we're elevating inconsistent style over substance
-- and violating the C++ style guide to boot.

[commit-bot: I haz the power, 2012-03-15 19:25:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/miket@chromium.org/9666059/15034

[commit-bot: I haz the power, 2012-03-15 22:44:33]

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

[Aaron Boodman, 2012-03-16 07:53:46]

lgtm

[commit-bot: I haz the power, 2012-03-16 13:08:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/miket@chromium.org/9666059/15034

[commit-bot: I haz the power, 2012-03-16 14:33:29]

Change committed as 127159