Lazy load chrome.* APIs

Set up V8 bindings for extension/app APIs when they're first used, not on
context creation. This should gives us a significant reduction in extension/app
startup time and slightly better memory usage.

It also gives us better error messages, the chance to complete the 
implementation of API features, and eventually the ability to expose select
extension APIs (e.g. extension.sendMessage) to web pages.

Resubmitting: changes made to resubmit this patch reviewed in: https://codereview.chromium.org/12378077/

BUG=163678, 120070, 55316, 177163
TBR=ben@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=186643

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=187143

[not at google - send to devlin, 2012-12-13 22:26:40]

Removing CC'ed people from the review while we talk about it.

https://codereview.chromium.org/11571014/diff/1/chrome/common/extensions/api/...
File chrome/common/extensions/api/extension_api.cc (right):

https://codereview.chromium.org/11571014/diff/1/chrome/common/extensions/api/...
chrome/common/extensions/api/extension_api.cc:697: // Don't do this. Instead
reference the unloaded schemas and load as needed.
Yup. Loading all the schemas somewhat defeats loading things lazily.

https://codereview.chromium.org/11571014/diff/1/chrome/common/extensions/api/...
File chrome/common/extensions/api/extension_api.h (right):

https://codereview.chromium.org/11571014/diff/1/chrome/common/extensions/api/...
chrome/common/extensions/api/extension_api.h:94:
scoped_ptr<std::set<std::string> > GetAllAPINames();
just return a std::set<std::string>, the GetAPIsForContext code is silly (and I
can say that because I wrote it before I knew better), would be greatly
appreciated if you could change that to return just a std::set<std::string> too.

https://codereview.chromium.org/11571014/diff/1/chrome/renderer/extensions/di...
File chrome/renderer/extensions/dispatcher.cc (right):

https://codereview.chromium.org/11571014/diff/1/chrome/renderer/extensions/di...
chrome/renderer/extensions/dispatcher.cc:583: // TODO: Make this lazy.
Yep - the idea is to pull the SetLazyField out of the if{} above and execute it
for all native handlers.

https://codereview.chromium.org/11571014/diff/1/chrome/renderer/extensions/di...
chrome/renderer/extensions/dispatcher.cc:823: // TODO: Move this back down to
next case.
Why is it up here?

https://codereview.chromium.org/11571014/diff/1/chrome/renderer/extensions/mo...
File chrome/renderer/extensions/module_system.cc (right):

https://codereview.chromium.org/11571014/diff/1/chrome/renderer/extensions/mo...
chrome/renderer/extensions/module_system.cc:245: v8::Local<v8::Value> value =
module->Get(field);
Something to add a TODO about - it seems like we could save a bit here by
actually setting the property on the lazy object, thus never calling into this
LazyFieldGetter again for that property. The laziness here comes from
RequireForJs not requiring something twice, but we can do a little bit better.

https://codereview.chromium.org/11571014/diff/1/chrome/renderer/extensions/mo...
chrome/renderer/extensions/module_system.cc:251: return
handle_scope.Close(module_system->CallModuleMethod(value, 1, argv));
Yeah, all this stuff - can it go in schema_binding_generator?

https://codereview.chromium.org/11571014/diff/1/chrome/renderer/extensions/mo...
File chrome/renderer/extensions/module_system.h (right):

https://codereview.chromium.org/11571014/diff/1/chrome/renderer/extensions/mo...
chrome/renderer/extensions/module_system.h:104: //
require(|module_name|)[|module_field|](|arg|).
This is pretty cool, though see comment in schema_generated_native_handler - I
was hoping that the act of requiring a module like 'alarms' would eventually
call into NewInstance there, which could do the function calling itself (set up
arguments etc).

https://codereview.chromium.org/11571014/diff/1/chrome/renderer/extensions/sc...
File chrome/renderer/extensions/schema_generated_native_handler.cc (right):

https://codereview.chromium.org/11571014/diff/1/chrome/renderer/extensions/sc...
chrome/renderer/extensions/schema_generated_native_handler.cc:22:
object_template_->Set(bind_to_.c_str(), schema);
My thought was to have this call out to schema_binding_generator and return the
populated object, rather than doing it as part of the module system.

https://codereview.chromium.org/11571014/diff/1/chrome/renderer/resources/ext...
File chrome/renderer/resources/extensions/schema_binding_generator.js (right):

https://codereview.chromium.org/11571014/diff/1/chrome/renderer/resources/ext...
chrome/renderer/resources/extensions/schema_binding_generator.js:12: return
'ERROR MESSAGE';
oops

https://codereview.chromium.org/11571014/diff/1/chrome/renderer/resources/ext...
File chrome/renderer/resources/extensions/schema_generated_bindings.js (right):

https://codereview.chromium.org/11571014/diff/1/chrome/renderer/resources/ext...
chrome/renderer/resources/extensions/schema_generated_bindings.js:8: //
TODO(cduvall): MASSIVE REFACTOR
nice

[cduvall, 2012-12-14 00:25:18]

https://codereview.chromium.org/11571014/diff/1/chrome/common/extensions/api/...
File chrome/common/extensions/api/extension_api.h (right):

https://codereview.chromium.org/11571014/diff/1/chrome/common/extensions/api/...
chrome/common/extensions/api/extension_api.h:94:
scoped_ptr<std::set<std::string> > GetAllAPINames();
On 2012/12/13 22:26:40, kalman wrote:
> just return a std::set<std::string>, the GetAPIsForContext code is silly (and
I
> can say that because I wrote it before I knew better), would be greatly
> appreciated if you could change that to return just a std::set<std::string>
too.

Done.

https://codereview.chromium.org/11571014/diff/1/chrome/renderer/extensions/di...
File chrome/renderer/extensions/dispatcher.cc (right):

https://codereview.chromium.org/11571014/diff/1/chrome/renderer/extensions/di...
chrome/renderer/extensions/dispatcher.cc:823: // TODO: Move this back down to
next case.
On 2012/12/13 22:26:40, kalman wrote:
> Why is it up here?

Just so I could make sure everything was working. :)

I can just open up a browser to any webpage and check what chrome.whatever is.

[not at google - send to devlin, 2012-12-14 00:44:00]

https://codereview.chromium.org/11571014/diff/7001/chrome/renderer/extensions...
File chrome/renderer/extensions/chrome_v8_context.cc (right):

https://codereview.chromium.org/11571014/diff/7001/chrome/renderer/extensions...
chrome/renderer/extensions/chrome_v8_context.cc:154: return
*(available_extension_apis_.get());
use .reset(new ...)

https://codereview.chromium.org/11571014/diff/7001/chrome/renderer/extensions...
File chrome/renderer/extensions/dispatcher.cc (right):

https://codereview.chromium.org/11571014/diff/7001/chrome/renderer/extensions...
chrome/renderer/extensions/dispatcher.cc:559: // TODO: Change this to
ExtensionAPI::GetAPIsForContext()?
what you have is correct, the comment and the commented-out code can be deleted.

https://codereview.chromium.org/11571014/diff/7001/chrome/renderer/extensions...
chrome/renderer/extensions/dispatcher.cc:582: api_name);
I think the file roles and their names are getting mixed up here. What I had in
mind doesn't involve needing this special version of SetLazyField.

https://codereview.chromium.org/11571014/diff/7001/chrome/renderer/extensions...
File chrome/renderer/extensions/schema_generated_native_handler.cc (right):

https://codereview.chromium.org/11571014/diff/7001/chrome/renderer/extensions...
chrome/renderer/extensions/schema_generated_native_handler.cc:36: return
object_template_->NewInstance();
I think you can just construct a new object, no need to assign into the parent's
object_template_.

In fact the only thing we're using is NativeHandler's NewInstance method.
Perhaps this should be a pure virtual instance, and what NativeHandler is
currently should be like ObjectBackedNativeHandler or something.

https://codereview.chromium.org/11571014/diff/7001/chrome/renderer/resources/...
File chrome/renderer/resources/extensions/schema_binding_generator.js (right):

https://codereview.chromium.org/11571014/diff/7001/chrome/renderer/resources/...
chrome/renderer/resources/extensions/schema_binding_generator.js:8: return
this[arg];
I would expect this function to look like:

exports.generate = function generate(schema) {
  var binding = {};
  schema.functions.forEach(function(fn) {
    binding[fn.name] = function() {
      ...
    };
  });
  schema.events.forEach(function(event) {
    ...
  });
  ...
  return binding;
}

https://codereview.chromium.org/11571014/diff/7001/chrome/renderer/resources/...
chrome/renderer/resources/extensions/schema_binding_generator.js:9: }.bind({});
why the bind({})?

oh, for the cache. Yeah I don't think caching is necessary, let callers do that.

https://codereview.chromium.org/11571014/diff/7001/chrome/renderer/resources/...
File chrome/renderer/resources/extensions/schema_generated_bindings.js (right):

https://codereview.chromium.org/11571014/diff/7001/chrome/renderer/resources/...
chrome/renderer/resources/extensions/schema_generated_bindings.js:10: return
json;
This is kind what I think is meant to be in schema_binding_generator

[cduvall, 2012-12-14 02:04:50]

Still a work in progress. The C++ is almost there but I still need to fix up
extension_api.cc.

JS refactor coming soon!

https://codereview.chromium.org/11571014/diff/7001/chrome/renderer/extensions...
File chrome/renderer/extensions/chrome_v8_context.cc (right):

https://codereview.chromium.org/11571014/diff/7001/chrome/renderer/extensions...
chrome/renderer/extensions/chrome_v8_context.cc:154: return
*(available_extension_apis_.get());
On 2012/12/14 00:44:00, kalman wrote:
> use .reset(new ...)

Done.

https://codereview.chromium.org/11571014/diff/7001/chrome/renderer/extensions...
File chrome/renderer/extensions/dispatcher.cc (right):

https://codereview.chromium.org/11571014/diff/7001/chrome/renderer/extensions...
chrome/renderer/extensions/dispatcher.cc:559: // TODO: Change this to
ExtensionAPI::GetAPIsForContext()?
On 2012/12/14 00:44:00, kalman wrote:
> what you have is correct, the comment and the commented-out code can be
deleted.

Done.

https://codereview.chromium.org/11571014/diff/7001/chrome/renderer/extensions...
chrome/renderer/extensions/dispatcher.cc:582: api_name);
On 2012/12/14 00:44:00, kalman wrote:
> I think the file roles and their names are getting mixed up here. What I had
in
> mind doesn't involve needing this special version of SetLazyField.

Done.

https://codereview.chromium.org/11571014/diff/7001/chrome/renderer/extensions...
File chrome/renderer/extensions/schema_generated_native_handler.cc (right):

https://codereview.chromium.org/11571014/diff/7001/chrome/renderer/extensions...
chrome/renderer/extensions/schema_generated_native_handler.cc:36: return
object_template_->NewInstance();
On 2012/12/14 00:44:00, kalman wrote:
> I think you can just construct a new object, no need to assign into the
parent's
> object_template_.
> 
> In fact the only thing we're using is NativeHandler's NewInstance method.
> Perhaps this should be a pure virtual instance, and what NativeHandler is
> currently should be like ObjectBackedNativeHandler or something.

I agree, I'll do it once I get some other stuff figured out.

https://codereview.chromium.org/11571014/diff/7001/chrome/renderer/resources/...
File chrome/renderer/resources/extensions/schema_binding_generator.js (right):

https://codereview.chromium.org/11571014/diff/7001/chrome/renderer/resources/...
chrome/renderer/resources/extensions/schema_binding_generator.js:8: return
this[arg];
On 2012/12/14 00:44:00, kalman wrote:
> I would expect this function to look like:
> 
> exports.generate = function generate(schema) {
>   var binding = {};
>   schema.functions.forEach(function(fn) {
>     binding[fn.name] = function() {
>       ...
>     };
>   });
>   schema.events.forEach(function(event) {
>     ...
>   });
>   ...
>   return binding;
> }

Refactor coming soon!

https://codereview.chromium.org/11571014/diff/7001/chrome/renderer/resources/...
chrome/renderer/resources/extensions/schema_binding_generator.js:9: }.bind({});
On 2012/12/14 00:44:00, kalman wrote:
> why the bind({})?
> 
> oh, for the cache. Yeah I don't think caching is necessary, let callers do
that.

Done.

https://codereview.chromium.org/11571014/diff/7001/chrome/renderer/resources/...
File chrome/renderer/resources/extensions/schema_generated_bindings.js (right):

https://codereview.chromium.org/11571014/diff/7001/chrome/renderer/resources/...
chrome/renderer/resources/extensions/schema_generated_bindings.js:10: return
json;
On 2012/12/14 00:44:00, kalman wrote:
> This is kind what I think is meant to be in schema_binding_generator

Done.

[not at google - send to devlin, 2013-01-24 21:10:12]

https://codereview.chromium.org/11571014/diff/18001/chrome/renderer/extension...
File chrome/renderer/extensions/chrome_v8_context.cc (right):

https://codereview.chromium.org/11571014/diff/18001/chrome/renderer/extension...
chrome/renderer/extensions/chrome_v8_context.cc:146:
available_extension_apis_.reset(new std::set<std::string>(
available_extension_apis_.swap(GetAPIsForContext...)

https://codereview.chromium.org/11571014/diff/18001/chrome/renderer/extension...
File chrome/renderer/extensions/dispatcher.cc (right):

https://codereview.chromium.org/11571014/diff/18001/chrome/renderer/extension...
chrome/renderer/extensions/dispatcher.cc:595: 
extra \n

https://codereview.chromium.org/11571014/diff/18001/chrome/renderer/extension...
chrome/renderer/extensions/dispatcher.cc:626: // TODO(cduvall): Handle apitest.
Would it help to re-implement apitest.js as a custom binding of chrome.test?
Maybe we can get somebody to help with that. It would also address a
long-standing TODO.

https://codereview.chromium.org/11571014/diff/18001/chrome/renderer/extension...
chrome/renderer/extensions/dispatcher.cc:888: CHECK(extension);
IIRC there were some cases where there actually wasn't an extension here, but I
can't remember them... perhaps look back at the revision history of
dispatcher.cc.

https://codereview.chromium.org/11571014/diff/18001/chrome/renderer/extension...
File chrome/renderer/extensions/dispatcher.h (right):

https://codereview.chromium.org/11571014/diff/18001/chrome/renderer/extension...
chrome/renderer/extensions/dispatcher.h:217: void
SetUpChainedField(v8::Handle<v8::Object> object,
comment

https://codereview.chromium.org/11571014/diff/18001/chrome/renderer/extension...
File chrome/renderer/extensions/module_system.h (right):

https://codereview.chromium.org/11571014/diff/18001/chrome/renderer/extension...
chrome/renderer/extensions/module_system.h:85: v8::Handle<v8::Value>
CallModuleMethod(v8::Local<v8::Value> value,
would prefer this to be a v8::Function but if it adds too much annoyance then
fine the way it is.

https://codereview.chromium.org/11571014/diff/18001/chrome/renderer/extension...
chrome/renderer/extensions/module_system.h:94: bool HasNativeHandler(const
std::string& name);
docs on all these methods

https://codereview.chromium.org/11571014/diff/18001/chrome/renderer/extension...
chrome/renderer/extensions/module_system.h:121: v8::Handle<v8::Object>
parameters);
why are these static?

https://codereview.chromium.org/11571014/diff/18001/chrome/renderer/resources...
File chrome/renderer/resources/extensions/json_schema.js (right):

https://codereview.chromium.org/11571014/diff/18001/chrome/renderer/resources...
chrome/renderer/resources/extensions/json_schema.js:41: var chromeHidden =
requireNative('chrome_hidden').GetChromeHidden();
At some point it would be nice to make this file not depend on chromeHidden. I
should file bugs on all these things.

https://codereview.chromium.org/11571014/diff/18001/chrome/renderer/resources...
File chrome/renderer/resources/extensions/schema_binding_generator.js (right):

https://codereview.chromium.org/11571014/diff/18001/chrome/renderer/resources...
chrome/renderer/resources/extensions/schema_binding_generator.js:18: var DCHECK
= requireNative('logging').DCHECK;
DCHECK is badass, why do you want to take it out?

https://codereview.chromium.org/11571014/diff/18001/chrome/renderer/resources...
chrome/renderer/resources/extensions/schema_binding_generator.js:27:
chromeHidden.internalAPIs = internalAPIs;
With the work you're doing it seems possible to implement this internal API
stuff in a much less hacky way. I can call it hacky because I wrote it.

https://codereview.chromium.org/11571014/diff/18001/chrome/renderer/resources...
chrome/renderer/resources/extensions/schema_binding_generator.js:369: return
mod;
Yeah - so we could do internal APIs like so:

Rather than like

chromeHidden.internalAPIs.webRequestInternal.addEventListener(...)

have

var webRequestInternal = (new
require('schema_bindings_generator.js').Bindings)('webRequestInternal');
webRequestInternal.addEventListener(...);

or whatever it is.

Of course, that wouldn't actually work, I think you'd end up generating some
redundant bindings. I'm not 100% sure the permissions checking would let you do
that either.

But it's something we should aim for. This file shouldn't have side effects.

https://codereview.chromium.org/11571014/diff/18001/chrome/renderer/resources...
chrome/renderer/resources/extensions/schema_binding_generator.js:389:
this.registerCustomType = function(typeName, customTypeFactory) {
This isn't the idiomatic way to put class methods on objects in JS (but neither
was the way it was done originally, my bad).

It should be

function Bindings(apiName) {
  ...
}

Bindings.prototype =  {
  registerCustomType: function(typeName, customTypeFactory) {
  },

  registerCustomEvent: function(event) {
  },

  ...
};

https://codereview.chromium.org/11571014/diff/18001/chrome/renderer/resources...
File chrome/renderer/resources/extensions/storage_custom_bindings.js (right):

https://codereview.chromium.org/11571014/diff/18001/chrome/renderer/resources...
chrome/renderer/resources/extensions/storage_custom_bindings.js:8: var bindings
= new Bindings('storage');
for all instances of this pattern: having a bindings and Bindings variable is
confusing, let's just always do

var bindings = new (require('schema_binding_generator').Bindings)('storage');

https://codereview.chromium.org/11571014/diff/18001/chrome/renderer/resources...
File chrome/renderer/resources/extensions/utils.js (right):

https://codereview.chromium.org/11571014/diff/18001/chrome/renderer/resources...
chrome/renderer/resources/extensions/utils.js:5: var chrome =
requireNative('chrome').GetChrome();
Just curious: is there a particular reason why you're doing this rather than
getting it from window.chrome (for all cases where you've done this)?

https://codereview.chromium.org/11571014/diff/18001/chrome/renderer/resources...
chrome/renderer/resources/extensions/utils.js:32: return chrome[parts.slice(0,
parts.length - 1).join(".")];
So this forces the object to be loaded? What if |ref| has no dots in it (e.g. a
ref into the current namespace)?

[cduvall, 2013-01-24 22:15:15]

Mostly added TODOs and fixed some of the quick things.

https://codereview.chromium.org/11571014/diff/18001/chrome/renderer/extension...
File chrome/renderer/extensions/chrome_v8_context.cc (right):

https://codereview.chromium.org/11571014/diff/18001/chrome/renderer/extension...
chrome/renderer/extensions/chrome_v8_context.cc:146:
available_extension_apis_.reset(new std::set<std::string>(
On 2013/01/24 21:10:12, kalman wrote:
> available_extension_apis_.swap(GetAPIsForContext...)

GetAPIsForContext doesn't return a scoped_ptr, so I think this should still be
reset, right?

https://codereview.chromium.org/11571014/diff/18001/chrome/renderer/extension...
File chrome/renderer/extensions/dispatcher.cc (right):

https://codereview.chromium.org/11571014/diff/18001/chrome/renderer/extension...
chrome/renderer/extensions/dispatcher.cc:595: 
On 2013/01/24 21:10:12, kalman wrote:
> extra \n

Done.

https://codereview.chromium.org/11571014/diff/18001/chrome/renderer/extension...
chrome/renderer/extensions/dispatcher.cc:626: // TODO(cduvall): Handle apitest.
On 2013/01/24 21:10:12, kalman wrote:
> Would it help to re-implement apitest.js as a custom binding of chrome.test?
> Maybe we can get somebody to help with that. It would also address a
> long-standing TODO.

That sounds like a good idea. I don't really like it how it is right now.

https://codereview.chromium.org/11571014/diff/18001/chrome/renderer/extension...
chrome/renderer/extensions/dispatcher.cc:888: CHECK(extension);
On 2013/01/24 21:10:12, kalman wrote:
> IIRC there were some cases where there actually wasn't an extension here, but
I
> can't remember them... perhaps look back at the revision history of
> dispatcher.cc.

Done.

https://codereview.chromium.org/11571014/diff/18001/chrome/renderer/extension...
File chrome/renderer/extensions/dispatcher.h (right):

https://codereview.chromium.org/11571014/diff/18001/chrome/renderer/extension...
chrome/renderer/extensions/dispatcher.h:217: void
SetUpChainedField(v8::Handle<v8::Object> object,
On 2013/01/24 21:10:12, kalman wrote:
> comment

Done.

https://codereview.chromium.org/11571014/diff/18001/chrome/renderer/extension...
File chrome/renderer/extensions/module_system.h (right):

https://codereview.chromium.org/11571014/diff/18001/chrome/renderer/extension...
chrome/renderer/extensions/module_system.h:85: v8::Handle<v8::Value>
CallModuleMethod(v8::Local<v8::Value> value,
On 2013/01/24 21:10:12, kalman wrote:
> would prefer this to be a v8::Function but if it adds too much annoyance then
> fine the way it is.

Added TODO.

https://codereview.chromium.org/11571014/diff/18001/chrome/renderer/extension...
chrome/renderer/extensions/module_system.h:94: bool HasNativeHandler(const
std::string& name);
On 2013/01/24 21:10:12, kalman wrote:
> docs on all these methods

Done.

https://codereview.chromium.org/11571014/diff/18001/chrome/renderer/extension...
chrome/renderer/extensions/module_system.h:121: v8::Handle<v8::Object>
parameters);
On 2013/01/24 21:10:12, kalman wrote:
> why are these static?

Because... who knows. They aren't anymore.

https://codereview.chromium.org/11571014/diff/18001/chrome/renderer/resources...
File chrome/renderer/resources/extensions/json_schema.js (right):

https://codereview.chromium.org/11571014/diff/18001/chrome/renderer/resources...
chrome/renderer/resources/extensions/json_schema.js:41: var chromeHidden =
requireNative('chrome_hidden').GetChromeHidden();
On 2013/01/24 21:10:12, kalman wrote:
> At some point it would be nice to make this file not depend on chromeHidden. I
> should file bugs on all these things.

Put in TODO.

https://codereview.chromium.org/11571014/diff/18001/chrome/renderer/resources...
File chrome/renderer/resources/extensions/schema_binding_generator.js (right):

https://codereview.chromium.org/11571014/diff/18001/chrome/renderer/resources...
chrome/renderer/resources/extensions/schema_binding_generator.js:18: var DCHECK
= requireNative('logging').DCHECK;
On 2013/01/24 21:10:12, kalman wrote:
> DCHECK is badass, why do you want to take it out?

I just put in in for debugging.

https://codereview.chromium.org/11571014/diff/18001/chrome/renderer/resources...
chrome/renderer/resources/extensions/schema_binding_generator.js:27:
chromeHidden.internalAPIs = internalAPIs;
On 2013/01/24 21:10:12, kalman wrote:
> With the work you're doing it seems possible to implement this internal API
> stuff in a much less hacky way. I can call it hacky because I wrote it.

Put in TODO on me.

https://codereview.chromium.org/11571014/diff/18001/chrome/renderer/resources...
chrome/renderer/resources/extensions/schema_binding_generator.js:369: return
mod;
On 2013/01/24 21:10:12, kalman wrote:
> Yeah - so we could do internal APIs like so:
> 
> Rather than like
> 
> chromeHidden.internalAPIs.webRequestInternal.addEventListener(...)
> 
> have
> 
> var webRequestInternal = (new
> require('schema_bindings_generator.js').Bindings)('webRequestInternal');
> webRequestInternal.addEventListener(...);
> 
> or whatever it is.
> 
> Of course, that wouldn't actually work, I think you'd end up generating some
> redundant bindings. I'm not 100% sure the permissions checking would let you
do
> that either.
> 
> But it's something we should aim for. This file shouldn't have side effects.

Yeah, I'll probably need to refactor a little to get rid of internalAPIs. TODO
for now.

https://codereview.chromium.org/11571014/diff/18001/chrome/renderer/resources...
chrome/renderer/resources/extensions/schema_binding_generator.js:389:
this.registerCustomType = function(typeName, customTypeFactory) {
On 2013/01/24 21:10:12, kalman wrote:
> This isn't the idiomatic way to put class methods on objects in JS (but
neither
> was the way it was done originally, my bad).
> 
> It should be
> 
> function Bindings(apiName) {
>   ...
> }
> 
> Bindings.prototype =  {
>   registerCustomType: function(typeName, customTypeFactory) {
>   },
> 
>   registerCustomEvent: function(event) {
>   },
> 
>   ...
> };

Done.

https://codereview.chromium.org/11571014/diff/18001/chrome/renderer/resources...
File chrome/renderer/resources/extensions/storage_custom_bindings.js (right):

https://codereview.chromium.org/11571014/diff/18001/chrome/renderer/resources...
chrome/renderer/resources/extensions/storage_custom_bindings.js:8: var bindings
= new Bindings('storage');
On 2013/01/24 21:10:12, kalman wrote:
> for all instances of this pattern: having a bindings and Bindings variable is
> confusing, let's just always do
> 
> var bindings = new (require('schema_binding_generator').Bindings)('storage');

Done.

https://codereview.chromium.org/11571014/diff/18001/chrome/renderer/resources...
File chrome/renderer/resources/extensions/utils.js (right):

https://codereview.chromium.org/11571014/diff/18001/chrome/renderer/resources...
chrome/renderer/resources/extensions/utils.js:5: var chrome =
requireNative('chrome').GetChrome();
On 2013/01/24 21:10:12, kalman wrote:
> Just curious: is there a particular reason why you're doing this rather than
> getting it from window.chrome (for all cases where you've done this)?

I think since they are being loaded differently they no longer have access to
window.chrome.

https://codereview.chromium.org/11571014/diff/18001/chrome/renderer/resources...
chrome/renderer/resources/extensions/utils.js:32: return chrome[parts.slice(0,
parts.length - 1).join(".")];
On 2013/01/24 21:10:12, kalman wrote:
> So this forces the object to be loaded? What if |ref| has no dots in it (e.g.
a
> ref into the current namespace)?

This will just do nothing if its called for a ref in the current namespace
(because it's already loaded). It will just access chrome['']. This is bad if
the result is used, so I'm adding a TODO for now.

[not at google - send to devlin, 2013-01-25 00:44:31]

Ok - this should be pleeeenty of stuff to go on with for now.

https://codereview.chromium.org/11571014/diff/18001/chrome/renderer/extension...
File chrome/renderer/extensions/chrome_v8_context.cc (right):

https://codereview.chromium.org/11571014/diff/18001/chrome/renderer/extension...
chrome/renderer/extensions/chrome_v8_context.cc:146:
available_extension_apis_.reset(new std::set<std::string>(
On 2013/01/24 22:15:15, cduvall wrote:
> On 2013/01/24 21:10:12, kalman wrote:
> > available_extension_apis_.swap(GetAPIsForContext...)
> 
> GetAPIsForContext doesn't return a scoped_ptr, so I think this should still be
> reset, right?

Ah, I mean for available_extension_apis to be just a set not a scoped_ptr<set>,
probably with a separate bool to say whether it's initialized (since it seems
conceivable that it initialises to empty).

https://codereview.chromium.org/11571014/diff/31001/chrome/renderer/extension...
File chrome/renderer/extensions/dispatcher.cc (right):

https://codereview.chromium.org/11571014/diff/31001/chrome/renderer/extension...
chrome/renderer/extensions/dispatcher.cc:654: }
I feel like this should look more like

const std::string& api_name = *it;

std::vector<std::string> split;
base::SplitString(api_name, '.', &split);

v8::Handle<v8::Object> bind_object = GetOrCreateChrome(v8_context);
for (size_t i = 0; i < split.size() - 1; ++i)
  bind_object = GetOrCreateObject(bind_object, split[i]);

if (lazy_bindings_map_.find(api_name) != lazy_bindings_map_.end()) {
  InstallBindings(module_system, v8_context, api_name);
} else if (source_map_.Contains(api_name)) {
  // There is a custom binding for |api_name|. When an API is needed run that;
  // custom bindings will be responsible for generating all bindings.
  module_system->SetLazyField(bind_object,
                              split[split.size() - 1],
                              api_name,
                              "bindings");
} else {
  // There is no custom binding to generate bindings, so use
  // SchemaGeneratingNativeHandler to generate it for us.
  module_system->RegisterNativeHandler(
      api_name,
      scoped_ptr<NativeHandler>(new SchemaGeneratingNativeHandler(
          module_system,
          v8_schema_registry(),
          api_name,
          "bindings"));
  module_system->SetNativeLazyField(bind_object,
                                    split[split.size() - 1],
                                    api_name,
                                    "bindings");
}

Writing GetOrCreateObject is much easier than writing SetUpChainedField, and
besides I find SetUpChainedField a bit too abstract/indirect.

https://codereview.chromium.org/11571014/diff/31001/chrome/renderer/extension...
chrome/renderer/extensions/dispatcher.cc:657: "event_bindings",
would it work to make schema_binding_generator require() Event if it wants to
use it? I.e. if it finds itself wanting to generate an event. This wouldn't need
to be lazy then, since those bindings are lazy.

https://codereview.chromium.org/11571014/diff/31001/chrome/renderer/extension...
chrome/renderer/extensions/dispatcher.cc:659:
module_system->SetLazyField(GetOrCreateChrome(v8_context),
Maybe we should fix that test thing before submitting this change.

https://codereview.chromium.org/11571014/diff/31001/chrome/renderer/extension...
File chrome/renderer/extensions/module_system.h (right):

https://codereview.chromium.org/11571014/diff/31001/chrome/renderer/extension...
chrome/renderer/extensions/module_system.h:77: v8::Handle<v8::Value>
RequireForJsInner(v8::Handle<v8::String> module_name);
hm these two methods (RequireForJs/RequireForJsInner) should be private.

https://codereview.chromium.org/11571014/diff/31001/chrome/renderer/extension...
chrome/renderer/extensions/module_system.h:119: static v8::Handle<v8::Value>
BaseLazyFieldGetter(
Should be private.

Also the "base" confused me, it sounds like a modifier like ((base lazy field)
getter) which doesn't mean anything.

Perhaps LazyFieldGetterInner or something would be better.

https://codereview.chromium.org/11571014/diff/31001/chrome/renderer/extension...
chrome/renderer/extensions/module_system.h:126: v8::Handle<v8::Object>
GetNativeModule(v8::Handle<v8::Object> parameters);
should all be private

I also don't think these are the same as require? They're specifically getters
designed to collaborate with lazy fields (more specifically
BaseLazyFieldGetter).

Looking at the implementation, they just seem to pull out kModuleName from
parameters. In that case, could BaseLazyFieldGetter pull out the module name
from parameters itself, then pass just the name to its get_module callback? That
would let you use Require or GetNativeFromString (TBH I'd call those
RequireNative/RequireNativeFromString but I don't want to go renaming koz's
stuff arbitrarily) without needing these extra slightly-confusing methods at
all.

https://codereview.chromium.org/11571014/diff/31001/chrome/renderer/extension...
File chrome/renderer/extensions/native_handler.h (right):

https://codereview.chromium.org/11571014/diff/31001/chrome/renderer/extension...
chrome/renderer/extensions/native_handler.h:18: public:
this needs a virtual destructor.

https://codereview.chromium.org/11571014/diff/31001/chrome/renderer/extension...
File chrome/renderer/extensions/schema_generated_native_handler.cc (right):

https://codereview.chromium.org/11571014/diff/31001/chrome/renderer/extension...
chrome/renderer/extensions/schema_generated_native_handler.cc:24:
v8::Handle<v8::Value> module =
schema_binding_generator

https://codereview.chromium.org/11571014/diff/31001/chrome/renderer/extension...
chrome/renderer/extensions/schema_generated_native_handler.cc:26:
v8::Local<v8::Value> func =
generate

https://codereview.chromium.org/11571014/diff/31001/chrome/renderer/extension...
chrome/renderer/extensions/schema_generated_native_handler.cc:32: };
Ok now I see why you need the "generate" function on schema_binding_generator as
well as on Binding.

I think we should do this slightly differently. Make Binding take the schema in
its constructor rather than the string, then use that here (it's slightly more
verbose, but not that bad).

Then give Binding a method like Create which takes the string.

I also think the custom bindings will look a bit nicer that way, like

var binding = require('schema_binding_generator').Binding.Create('foo');

which makes me think that perhaps the file should just be called "binding" not
"schema_binding_generator", and this class actually called
BindingGeneratingNativeHandler.

https://codereview.chromium.org/11571014/diff/31001/chrome/renderer/extension...
File chrome/renderer/extensions/schema_generated_native_handler.h (right):

https://codereview.chromium.org/11571014/diff/31001/chrome/renderer/extension...
chrome/renderer/extensions/schema_generated_native_handler.h:18: class
SchemaGeneratedNativeHandler : public NativeHandler {
comment?

SchemaGenerat*ing*NativeHandler perhaps?

https://codereview.chromium.org/11571014/diff/31001/chrome/renderer/resources...
File chrome/renderer/resources/extensions/schema_binding_generator.js (right):

https://codereview.chromium.org/11571014/diff/31001/chrome/renderer/resources...
chrome/renderer/resources/extensions/schema_binding_generator.js:73: var
apiFunctions = new APIFunctions();
ok, so this shouldn't be global anymore, it should be per binding. That should
give you the chance to delete this silly APIFunctions vs NamespacedAPIFunctions
stuff, I hope.

Basically this file should be just

// Should automatically namespace things by virtue of being private to
// Bindings, therefore NamespaceAPIFunction should be deleted.
function APIFunctions() {
}

APIFunctions.prototype = {
};

function Binding(schema) {
  this._schema = schema;
  this._apiFunctions = new APIFunctions();
  this._customTypes = {};
}

Binding.Create = function(name) {
  return new Binding(schemaRegistry.get(name));
};

Binding.prototype = {
  registerCustomType: function() {
    this._customTypes_ ...
  },

  registerCustomEvent: function(event) {
    this._customEvent ...
  },

  registerCustomHook: function(hook) {
    this._hooks ...
  },

  generate: function() {
    ... ALL THE GOOP
  }
};

exports.Binding = Binding;

https://codereview.chromium.org/11571014/diff/31001/chrome/renderer/resources...
chrome/renderer/resources/extensions/schema_binding_generator.js:165: 
We could and should be way cool here. Firstly: this is now the only place that
permissions get checked, so we want a native handler for ExtensionAPI. I'll
pretend it exists.

// at top
var extensionApi = requireNative('ExtensionAPI');

// ... and how, here in the code:

// Change IsAvailable to GetAvailability in ExtensionAPI.
// The custom binding should know how to plug in the Extension and the context.
var availability = extensionApi.GetAvailability(schema.namespace)
if (!availability.is_available) {
  throw new Error('"chrome.' + schema.namespace + "" is not available: " +
                  availability.message);
}

The cool part is then that you need to catch that exception in modulesystem and
teach it to throw it when it gets required.

[cduvall, 2013-02-12 02:13:47]

I addressed most of the comments from the previous patch. Still TODO:

- apitest is still being loaded the old way
- chromeHidden is still being used in some files
- 2 browser_tests failing
(DevToolsSanityTest.TestNoScriptDuplicatesOnPanelSwitch,
ExperimentalPlatformAppBrowserTest.WindowsApiSetIcon)
- Some TODO(cduvall)'s sprinkled around

https://codereview.chromium.org/11571014/diff/31001/chrome/renderer/extension...
File chrome/renderer/extensions/dispatcher.cc (right):

https://codereview.chromium.org/11571014/diff/31001/chrome/renderer/extension...
chrome/renderer/extensions/dispatcher.cc:654: }
On 2013/01/25 00:44:31, kalman wrote:
> I feel like this should look more like
> 
> const std::string& api_name = *it;
> 
> std::vector<std::string> split;
> base::SplitString(api_name, '.', &split);
> 
> v8::Handle<v8::Object> bind_object = GetOrCreateChrome(v8_context);
> for (size_t i = 0; i < split.size() - 1; ++i)
>   bind_object = GetOrCreateObject(bind_object, split[i]);
> 
> if (lazy_bindings_map_.find(api_name) != lazy_bindings_map_.end()) {
>   InstallBindings(module_system, v8_context, api_name);
> } else if (source_map_.Contains(api_name)) {
>   // There is a custom binding for |api_name|. When an API is needed run that;
>   // custom bindings will be responsible for generating all bindings.
>   module_system->SetLazyField(bind_object,
>                               split[split.size() - 1],
>                               api_name,
>                               "bindings");
> } else {
>   // There is no custom binding to generate bindings, so use
>   // SchemaGeneratingNativeHandler to generate it for us.
>   module_system->RegisterNativeHandler(
>       api_name,
>       scoped_ptr<NativeHandler>(new SchemaGeneratingNativeHandler(
>           module_system,
>           v8_schema_registry(),
>           api_name,
>           "bindings"));
>   module_system->SetNativeLazyField(bind_object,
>                                     split[split.size() - 1],
>                                     api_name,
>                                     "bindings");
> }
> 
> Writing GetOrCreateObject is much easier than writing SetUpChainedField, and
> besides I find SetUpChainedField a bit too abstract/indirect.

Done.

https://codereview.chromium.org/11571014/diff/31001/chrome/renderer/extension...
chrome/renderer/extensions/dispatcher.cc:657: "event_bindings",
On 2013/01/25 00:44:31, kalman wrote:
> would it work to make schema_binding_generator require() Event if it wants to
> use it? I.e. if it finds itself wanting to generate an event. This wouldn't
need
> to be lazy then, since those bindings are lazy.

Done.

https://codereview.chromium.org/11571014/diff/31001/chrome/renderer/extension...
File chrome/renderer/extensions/module_system.h (right):

https://codereview.chromium.org/11571014/diff/31001/chrome/renderer/extension...
chrome/renderer/extensions/module_system.h:77: v8::Handle<v8::Value>
RequireForJsInner(v8::Handle<v8::String> module_name);
On 2013/01/25 00:44:31, kalman wrote:
> hm these two methods (RequireForJs/RequireForJsInner) should be private.

Done.

https://codereview.chromium.org/11571014/diff/31001/chrome/renderer/extension...
chrome/renderer/extensions/module_system.h:119: static v8::Handle<v8::Value>
BaseLazyFieldGetter(
On 2013/01/25 00:44:31, kalman wrote:
> Should be private.
> 
> Also the "base" confused me, it sounds like a modifier like ((base lazy field)
> getter) which doesn't mean anything.
> 
> Perhaps LazyFieldGetterInner or something would be better.

Done.

https://codereview.chromium.org/11571014/diff/31001/chrome/renderer/extension...
chrome/renderer/extensions/module_system.h:126: v8::Handle<v8::Object>
GetNativeModule(v8::Handle<v8::Object> parameters);
On 2013/01/25 00:44:31, kalman wrote:
> should all be private
> 
> I also don't think these are the same as require? They're specifically getters
> designed to collaborate with lazy fields (more specifically
> BaseLazyFieldGetter).
> 
> Looking at the implementation, they just seem to pull out kModuleName from
> parameters. In that case, could BaseLazyFieldGetter pull out the module name
> from parameters itself, then pass just the name to its get_module callback?
That
> would let you use Require or GetNativeFromString (TBH I'd call those
> RequireNative/RequireNativeFromString but I don't want to go renaming koz's
> stuff arbitrarily) without needing these extra slightly-confusing methods at
> all.

Done.

https://codereview.chromium.org/11571014/diff/31001/chrome/renderer/extension...
File chrome/renderer/extensions/native_handler.h (right):

https://codereview.chromium.org/11571014/diff/31001/chrome/renderer/extension...
chrome/renderer/extensions/native_handler.h:18: public:
On 2013/01/25 00:44:31, kalman wrote:
> this needs a virtual destructor.

Done.

https://codereview.chromium.org/11571014/diff/31001/chrome/renderer/extension...
File chrome/renderer/extensions/schema_generated_native_handler.cc (right):

https://codereview.chromium.org/11571014/diff/31001/chrome/renderer/extension...
chrome/renderer/extensions/schema_generated_native_handler.cc:24:
v8::Handle<v8::Value> module =
On 2013/01/25 00:44:31, kalman wrote:
> schema_binding_generator

Done.

https://codereview.chromium.org/11571014/diff/31001/chrome/renderer/extension...
chrome/renderer/extensions/schema_generated_native_handler.cc:26:
v8::Local<v8::Value> func =
On 2013/01/25 00:44:31, kalman wrote:
> generate

Done.

https://codereview.chromium.org/11571014/diff/31001/chrome/renderer/extension...
chrome/renderer/extensions/schema_generated_native_handler.cc:32: };
On 2013/01/25 00:44:31, kalman wrote:
> Ok now I see why you need the "generate" function on schema_binding_generator
as
> well as on Binding.
> 
> I think we should do this slightly differently. Make Binding take the schema
in
> its constructor rather than the string, then use that here (it's slightly more
> verbose, but not that bad).
> 
> Then give Binding a method like Create which takes the string.
> 
> I also think the custom bindings will look a bit nicer that way, like
> 
> var binding = require('schema_binding_generator').Binding.Create('foo');
> 
> which makes me think that perhaps the file should just be called "binding" not
> "schema_binding_generator", and this class actually called
> BindingGeneratingNativeHandler.

Done.

https://codereview.chromium.org/11571014/diff/31001/chrome/renderer/extension...
File chrome/renderer/extensions/schema_generated_native_handler.h (right):

https://codereview.chromium.org/11571014/diff/31001/chrome/renderer/extension...
chrome/renderer/extensions/schema_generated_native_handler.h:18: class
SchemaGeneratedNativeHandler : public NativeHandler {
On 2013/01/25 00:44:31, kalman wrote:
> comment?
> 
> SchemaGenerat*ing*NativeHandler perhaps?

Done.

https://codereview.chromium.org/11571014/diff/31001/chrome/renderer/resources...
File chrome/renderer/resources/extensions/schema_binding_generator.js (right):

https://codereview.chromium.org/11571014/diff/31001/chrome/renderer/resources...
chrome/renderer/resources/extensions/schema_binding_generator.js:73: var
apiFunctions = new APIFunctions();
On 2013/01/25 00:44:31, kalman wrote:
> ok, so this shouldn't be global anymore, it should be per binding. That should
> give you the chance to delete this silly APIFunctions vs
NamespacedAPIFunctions
> stuff, I hope.
> 
> Basically this file should be just
> 
> // Should automatically namespace things by virtue of being private to
> // Bindings, therefore NamespaceAPIFunction should be deleted.
> function APIFunctions() {
> }
> 
> APIFunctions.prototype = {
> };
> 
> function Binding(schema) {
>   this._schema = schema;
>   this._apiFunctions = new APIFunctions();
>   this._customTypes = {};
> }
> 
> Binding.Create = function(name) {
>   return new Binding(schemaRegistry.get(name));
> };
> 
> Binding.prototype = {
>   registerCustomType: function() {
>     this._customTypes_ ...
>   },
> 
>   registerCustomEvent: function(event) {
>     this._customEvent ...
>   },
> 
>   registerCustomHook: function(hook) {
>     this._hooks ...
>   },
> 
>   generate: function() {
>     ... ALL THE GOOP
>   }
> };
> 
> exports.Binding = Binding;

Done.

[not at google - send to devlin, 2013-02-13 01:45:49]

https://codereview.chromium.org/11571014/diff/42011/chrome/browser/autocomple...
File chrome/browser/autocomplete/autocomplete_match.cc (right):

https://codereview.chromium.org/11571014/diff/42011/chrome/browser/autocomple...
chrome/browser/autocomplete/autocomplete_match.cc:345: // NOTE: This logic is
mirrored by |sanitizeString()| in bindings.js.
it's actually in omnibox_custom_bindings.js.

https://codereview.chromium.org/11571014/diff/42011/chrome/common/extensions/...
File chrome/common/extensions/api/extension_api.cc (right):

https://codereview.chromium.org/11571014/diff/42011/chrome/common/extensions/...
chrome/common/extensions/api/extension_api.cc:638: // chrome.devtools is
available everywhere.
... but even for extensions that don't have the devtools permission?

I still can't parse the comment, sorry :(

https://codereview.chromium.org/11571014/diff/42011/chrome/renderer/extension...
File chrome/renderer/extensions/binding_generating_native_handler.cc (right):

https://codereview.chromium.org/11571014/diff/42011/chrome/renderer/extension...
chrome/renderer/extensions/binding_generating_native_handler.cc:18:
schema_registry_(schema_registry),
schema_registry unused?

https://codereview.chromium.org/11571014/diff/42011/chrome/renderer/extension...
chrome/renderer/extensions/binding_generating_native_handler.cc:39:
module_system_->LogExceptionToConsole(try_catch);
Doesn't seem like it should be module system's responsibility to provide this,
just call the ExtensionHelper class directly? Or pull it out into a separate
class, but using ExtensionHelper is simpler...

https://codereview.chromium.org/11571014/diff/42011/chrome/renderer/extension...
chrome/renderer/extensions/binding_generating_native_handler.cc:44:
object->Set(v8::String::New(bind_to_.c_str()), compiled_schema);
I'll need to get koz@ to look at this, I think this needs to be a v8::Local with
a handlescope so that it doesn't leak? I can't remember how v8 refcounting works
in the APIs.

https://codereview.chromium.org/11571014/diff/42011/chrome/renderer/extension...
File chrome/renderer/extensions/binding_generating_native_handler.h (right):

https://codereview.chromium.org/11571014/diff/42011/chrome/renderer/extension...
chrome/renderer/extensions/binding_generating_native_handler.h:19: // the schema
into the |generate| function of schema_binding_generator.js.
note to self: is this |generate| function comment still accurate?

https://codereview.chromium.org/11571014/diff/42011/chrome/renderer/extension...
chrome/renderer/extensions/binding_generating_native_handler.h:22:
BindingGeneratingNativeHandler(ModuleSystem* module_system,
Comment api_name and bind_to?

https://codereview.chromium.org/11571014/diff/42011/chrome/renderer/extension...
chrome/renderer/extensions/binding_generating_native_handler.h:24: const
std::string& api_name,
+2 space

https://codereview.chromium.org/11571014/diff/42011/chrome/renderer/extension...
File chrome/renderer/extensions/dispatcher.cc (right):

https://codereview.chromium.org/11571014/diff/42011/chrome/renderer/extension...
chrome/renderer/extensions/dispatcher.cc:139: return v8::Undefined();
CHECK?

https://codereview.chromium.org/11571014/diff/42011/chrome/renderer/extension...
chrome/renderer/extensions/dispatcher.cc:151: }
Heh - ok so let's add a GetAvailability method to ChromeV8Context, which returns
a Feature::Availability. Until everything is converted to Features we'll just
need to make up some generic error message (not as generic as "SORRY", but
something). Then when we convert to using a Feature for checking API
availability it will start working.

https://codereview.chromium.org/11571014/diff/42011/chrome/renderer/extension...
chrome/renderer/extensions/dispatcher.cc:639: if (!existing->IsUndefined())
koz@ can't remember which of the many existence checks you need here,
existing.IsEmpty()? existing->IsNull()?

Or is this check because lazy fields are set to undefined? Is there a stronger
check that would work here, like, module_system->HasLazyThingy(object, key) (the
implementation of which could use something like this check, or something
smarter).

I guess it's just a bit subtle for me.

https://codereview.chromium.org/11571014/diff/42011/chrome/renderer/extension...
chrome/renderer/extensions/dispatcher.cc:951: // TODO(cduvall): Hack for loading
JSON into chromeHidden (event.js).
what JSON?

https://codereview.chromium.org/11571014/diff/42011/chrome/renderer/extension...
File chrome/renderer/extensions/module_system.cc (right):

https://codereview.chromium.org/11571014/diff/42011/chrome/renderer/extension...
chrome/renderer/extensions/module_system.cc:17: using
content::ConsoleMessageLevel;
not used

https://codereview.chromium.org/11571014/diff/42011/chrome/renderer/extension...
chrome/renderer/extensions/module_system.cc:124:
CreateExceptionString(try_catch));
Doesn't seem like this belongs in ModuleSystem (at least not as a public
method), just do this in BindingGeneratingNativeHandler directly?

Also can then go back to just injecting the v8::Context rather than the
ChromeV8Context.

https://codereview.chromium.org/11571014/diff/42011/chrome/renderer/extension...
chrome/renderer/extensions/module_system.cc:170: // TODO(cduvall): Figure out
how to handle this and throw it for later.
I think rethrow is fine. We should always throw it though (i.e. don't cache the
undefined value) - but leaving it the way it is is fine.

https://codereview.chromium.org/11571014/diff/42011/chrome/renderer/extension...
chrome/renderer/extensions/module_system.cc:283: return
handle_scope.Close(v8::Handle<v8::Value>());
rethrow should be fine

https://codereview.chromium.org/11571014/diff/42011/chrome/renderer/extension...
chrome/renderer/extensions/module_system.cc:296: object->Set(property,
new_field);
This shouldn't be necessary since Require/RequireNative does this already?
Right?

https://codereview.chromium.org/11571014/diff/42011/chrome/renderer/extension...
chrome/renderer/extensions/module_system.cc:321:
object->SetAccessor(v8::String::New(field.c_str()),
As mentioned in IRC, if you use SetNamedPropertyAccessor then you can make the
existence checks also call Require() then devtools should hopefully work fine.
Also, it's just good behaviour - extensions should be able to check access to an
API via ('foo' in chrome) or chrome.hasOwnProperty('foo').

https://codereview.chromium.org/11571014/diff/42011/chrome/renderer/extension...
chrome/renderer/extensions/module_system.cc:370: v8::Handle<v8::Value>
ModuleSystem::GetNativeFromString(
Yeah this is confusing, can you rename it to
RequireNative/RequireNativeFromString. Sorry...

https://codereview.chromium.org/11571014/diff/42011/chrome/renderer/extension...
File chrome/renderer/extensions/module_system.h (right):

https://codereview.chromium.org/11571014/diff/42011/chrome/renderer/extension...
chrome/renderer/extensions/module_system.h:73: void LogExceptionToConsole(const
v8::TryCatch& try_catch);
I think dumping to console also puts to warning. Let's just change DumpException
to do that. Also can be private (see comment in other file...)

https://codereview.chromium.org/11571014/diff/42011/chrome/renderer/resources...
File chrome/renderer/resources/extensions/apitest.js (right):

https://codereview.chromium.org/11571014/diff/42011/chrome/renderer/resources...
chrome/renderer/resources/extensions/apitest.js:15: chrome_test =
api.compiledApi;
should be chromeTest, but I'd just call it test.

https://codereview.chromium.org/11571014/diff/42011/chrome/renderer/resources...
File chrome/renderer/resources/extensions/app_custom_bindings.js (right):

https://codereview.chromium.org/11571014/diff/42011/chrome/renderer/resources...
chrome/renderer/resources/extensions/app_custom_bindings.js:73: function
defineCheckAvailabilityGetter(name, apiName, target) {
defineLazyGetter?

Comment why it's necessary?

https://codereview.chromium.org/11571014/diff/42011/chrome/renderer/resources...
chrome/renderer/resources/extensions/app_custom_bindings.js:80: return;
might as well say return undefined;

https://codereview.chromium.org/11571014/diff/42011/chrome/renderer/resources...
File chrome/renderer/resources/extensions/app_runtime_custom_bindings.js
(right):

https://codereview.chromium.org/11571014/diff/42011/chrome/renderer/resources...
chrome/renderer/resources/extensions/app_runtime_custom_bindings.js:18:
chrome.Event;
?

https://codereview.chromium.org/11571014/diff/42011/chrome/renderer/resources...
File chrome/renderer/resources/extensions/binding.js (right):

https://codereview.chromium.org/11571014/diff/42011/chrome/renderer/resources...
chrome/renderer/resources/extensions/binding.js:171: hook({
There's actually not really any reason to have this runHooks stuff anymore, that
was only needed because all generation happens at once.

Now that it happens per-binding, a much simpler model would be something like:

rather than

var binding = Binding.create('foo');
binding.registerCustomHook(function(api) {
  api.apiFunctions.setHandleRequest('blah', function() {
    ...
  });
});
exports.binding = binding.generate();

have

var binding = Binding.create('foo');
binding.setHandleRequest('blah', function() {
  ...
});
exports.binding = binding.generate();

However, this is a much more invasive change - I reckon we should just land what
you have and do it afterwards.

But yeah: add epic TODO(kalman/cduvall) here :)

https://codereview.chromium.org/11571014/diff/42011/chrome/renderer/resources...
chrome/renderer/resources/extensions/binding.js:179: generate: function() {
comment

https://codereview.chromium.org/11571014/diff/42011/chrome/renderer/resources...
chrome/renderer/resources/extensions/binding.js:183: return;
(see below comment: we do anyway).

Hm, this is another cleanup we could do later, but I think that GetAvailability
should also know how to handle this (in fact, Features will).

TODO(kalman/cduvall): Make GetAvailability handle this, then delete the
supporting code.

https://codereview.chromium.org/11571014/diff/42011/chrome/renderer/resources...
chrome/renderer/resources/extensions/binding.js:189: }
I can see that this exception throwing stuff is causing a few niggles. Could you
just log the error here, then return undefined? Have the module system deal with
that?

I might be missing something of course - but it seems a lot simpler.

[koz (OOO until 15th September), 2013-02-14 06:21:13]

https://codereview.chromium.org/11571014/diff/42011/chrome/renderer/extension...
File chrome/renderer/extensions/binding_generating_native_handler.cc (right):

https://codereview.chromium.org/11571014/diff/42011/chrome/renderer/extension...
chrome/renderer/extensions/binding_generating_native_handler.cc:44:
object->Set(v8::String::New(bind_to_.c_str()), compiled_schema);
On 2013/02/13 01:45:49, kalman wrote:
> I'll need to get koz@ to look at this, I think this needs to be a v8::Local
with
> a handlescope so that it doesn't leak? I can't remember how v8 refcounting
works
> in the APIs.

You don't need to declare it a v8::Local, but it would be prudent to surround
this function with a

v8::HandleScope scope;
...
return scope.Close(object);

When a v8::HandleScope goes out of scope it will mark as garbage any new v8
objects declared while it was alive (unless a later v8::HandleScope is
introduced).

For this function I suspect there's probably a higher v8::HandleScope that will
be capturing all these variables, but better safe than sorry I guess.

https://codereview.chromium.org/11571014/diff/42011/chrome/renderer/extension...
File chrome/renderer/extensions/dispatcher.cc (right):

https://codereview.chromium.org/11571014/diff/42011/chrome/renderer/extension...
chrome/renderer/extensions/dispatcher.cc:639: if (!existing->IsUndefined())
On 2013/02/13 01:45:49, kalman wrote:
> koz@ can't remember which of the many existence checks you need here,
> existing.IsEmpty()? existing->IsNull()?
> 
> Or is this check because lazy fields are set to undefined? Is there a stronger
> check that would work here, like, module_system->HasLazyThingy(object, key)
(the
> implementation of which could use something like this check, or something
> smarter).
> 
> I guess it's just a bit subtle for me.

Just existing.IsEmpty() to make sure the handle is valid, otherwise
dereferencing it will fail. V8 casts return empty handles if they fail.

[cduvall, 2013-02-15 00:40:27]

Should be done except for the pending WebKit/devtools problem.

https://codereview.chromium.org/11571014/diff/42011/chrome/browser/autocomple...
File chrome/browser/autocomplete/autocomplete_match.cc (right):

https://codereview.chromium.org/11571014/diff/42011/chrome/browser/autocomple...
chrome/browser/autocomplete/autocomplete_match.cc:345: // NOTE: This logic is
mirrored by |sanitizeString()| in bindings.js.
On 2013/02/13 01:45:49, kalman wrote:
> it's actually in omnibox_custom_bindings.js.

Done.

https://codereview.chromium.org/11571014/diff/42011/chrome/common/extensions/...
File chrome/common/extensions/api/extension_api.cc (right):

https://codereview.chromium.org/11571014/diff/42011/chrome/common/extensions/...
chrome/common/extensions/api/extension_api.cc:638: // chrome.devtools is
available everywhere.
On 2013/02/13 01:45:49, kalman wrote:
> ... but even for extensions that don't have the devtools permission?
> 
> I still can't parse the comment, sorry :(

Took this out, because it works with a change in WebKit.

https://codereview.chromium.org/11571014/diff/42011/chrome/renderer/extension...
File chrome/renderer/extensions/binding_generating_native_handler.cc (right):

https://codereview.chromium.org/11571014/diff/42011/chrome/renderer/extension...
chrome/renderer/extensions/binding_generating_native_handler.cc:18:
schema_registry_(schema_registry),
On 2013/02/13 01:45:49, kalman wrote:
> schema_registry unused?

Done.

https://codereview.chromium.org/11571014/diff/42011/chrome/renderer/extension...
chrome/renderer/extensions/binding_generating_native_handler.cc:39:
module_system_->LogExceptionToConsole(try_catch);
On 2013/02/13 01:45:49, kalman wrote:
> Doesn't seem like it should be module system's responsibility to provide this,
> just call the ExtensionHelper class directly? Or pull it out into a separate
> class, but using ExtensionHelper is simpler...

Now the exception is logged in binding.js.

https://codereview.chromium.org/11571014/diff/42011/chrome/renderer/extension...
chrome/renderer/extensions/binding_generating_native_handler.cc:44:
object->Set(v8::String::New(bind_to_.c_str()), compiled_schema);
On 2013/02/14 06:21:13, koz wrote:
> On 2013/02/13 01:45:49, kalman wrote:
> > I'll need to get koz@ to look at this, I think this needs to be a v8::Local
> with
> > a handlescope so that it doesn't leak? I can't remember how v8 refcounting
> works
> > in the APIs.
> 
> You don't need to declare it a v8::Local, but it would be prudent to surround
> this function with a
> 
> v8::HandleScope scope;
> ...
> return scope.Close(object);
> 
> When a v8::HandleScope goes out of scope it will mark as garbage any new v8
> objects declared while it was alive (unless a later v8::HandleScope is
> introduced).
> 
> For this function I suspect there's probably a higher v8::HandleScope that
will
> be capturing all these variables, but better safe than sorry I guess.

Done.

https://codereview.chromium.org/11571014/diff/42011/chrome/renderer/extension...
File chrome/renderer/extensions/binding_generating_native_handler.h (right):

https://codereview.chromium.org/11571014/diff/42011/chrome/renderer/extension...
chrome/renderer/extensions/binding_generating_native_handler.h:19: // the schema
into the |generate| function of schema_binding_generator.js.
On 2013/02/13 01:45:49, kalman wrote:
> note to self: is this |generate| function comment still accurate?

Done.

https://codereview.chromium.org/11571014/diff/42011/chrome/renderer/extension...
chrome/renderer/extensions/binding_generating_native_handler.h:22:
BindingGeneratingNativeHandler(ModuleSystem* module_system,
On 2013/02/13 01:45:49, kalman wrote:
> Comment api_name and bind_to?

Done.

https://codereview.chromium.org/11571014/diff/42011/chrome/renderer/extension...
chrome/renderer/extensions/binding_generating_native_handler.h:24: const
std::string& api_name,
On 2013/02/13 01:45:49, kalman wrote:
> +2 space

Done.

https://codereview.chromium.org/11571014/diff/42011/chrome/renderer/extension...
File chrome/renderer/extensions/dispatcher.cc (right):

https://codereview.chromium.org/11571014/diff/42011/chrome/renderer/extension...
chrome/renderer/extensions/dispatcher.cc:139: return v8::Undefined();
On 2013/02/13 01:45:49, kalman wrote:
> CHECK?

Done.

https://codereview.chromium.org/11571014/diff/42011/chrome/renderer/extension...
chrome/renderer/extensions/dispatcher.cc:151: }
On 2013/02/13 01:45:49, kalman wrote:
> Heh - ok so let's add a GetAvailability method to ChromeV8Context, which
returns
> a Feature::Availability. Until everything is converted to Features we'll just
> need to make up some generic error message (not as generic as "SORRY", but
> something). Then when we convert to using a Feature for checking API
> availability it will start working.

Done.

https://codereview.chromium.org/11571014/diff/42011/chrome/renderer/extension...
chrome/renderer/extensions/dispatcher.cc:639: if (!existing->IsUndefined())
On 2013/02/14 06:21:13, koz wrote:
> On 2013/02/13 01:45:49, kalman wrote:
> > koz@ can't remember which of the many existence checks you need here,
> > existing.IsEmpty()? existing->IsNull()?
> > 
> > Or is this check because lazy fields are set to undefined? Is there a
stronger
> > check that would work here, like, module_system->HasLazyThingy(object, key)
> (the
> > implementation of which could use something like this check, or something
> > smarter).
> > 
> > I guess it's just a bit subtle for me.
> 
> Just existing.IsEmpty() to make sure the handle is valid, otherwise
> dereferencing it will fail. V8 casts return empty handles if they fail.

Done.

https://codereview.chromium.org/11571014/diff/42011/chrome/renderer/extension...
chrome/renderer/extensions/dispatcher.cc:951: // TODO(cduvall): Hack for loading
JSON into chromeHidden (event.js).
On 2013/02/13 01:45:49, kalman wrote:
> what JSON?

Clarified the comment.

https://codereview.chromium.org/11571014/diff/42011/chrome/renderer/extension...
File chrome/renderer/extensions/module_system.cc (right):

https://codereview.chromium.org/11571014/diff/42011/chrome/renderer/extension...
chrome/renderer/extensions/module_system.cc:17: using
content::ConsoleMessageLevel;
On 2013/02/13 01:45:49, kalman wrote:
> not used

Done.

https://codereview.chromium.org/11571014/diff/42011/chrome/renderer/extension...
chrome/renderer/extensions/module_system.cc:124:
CreateExceptionString(try_catch));
On 2013/02/13 01:45:49, kalman wrote:
> Doesn't seem like this belongs in ModuleSystem (at least not as a public
> method), just do this in BindingGeneratingNativeHandler directly?
> 
> Also can then go back to just injecting the v8::Context rather than the
> ChromeV8Context.

Done.

https://codereview.chromium.org/11571014/diff/42011/chrome/renderer/extension...
chrome/renderer/extensions/module_system.cc:170: // TODO(cduvall): Figure out
how to handle this and throw it for later.
On 2013/02/13 01:45:49, kalman wrote:
> I think rethrow is fine. We should always throw it though (i.e. don't cache
the
> undefined value) - but leaving it the way it is is fine.

Reverted to how this used to be because logging is now handled in the JS.

https://codereview.chromium.org/11571014/diff/42011/chrome/renderer/extension...
chrome/renderer/extensions/module_system.cc:283: return
handle_scope.Close(v8::Handle<v8::Value>());
On 2013/02/13 01:45:49, kalman wrote:
> rethrow should be fine

Changed it to HandleException like the rest of the file.

https://codereview.chromium.org/11571014/diff/42011/chrome/renderer/extension...
chrome/renderer/extensions/module_system.cc:296: object->Set(property,
new_field);
On 2013/02/13 01:45:49, kalman wrote:
> This shouldn't be necessary since Require/RequireNative does this already?
> Right?

Done.

https://codereview.chromium.org/11571014/diff/42011/chrome/renderer/extension...
chrome/renderer/extensions/module_system.cc:321:
object->SetAccessor(v8::String::New(field.c_str()),
On 2013/02/13 01:45:49, kalman wrote:
> As mentioned in IRC, if you use SetNamedPropertyAccessor then you can make the
> existence checks also call Require() then devtools should hopefully work fine.
> Also, it's just good behaviour - extensions should be able to check access to
an
> API via ('foo' in chrome) or chrome.hasOwnProperty('foo').

As you said in your email, looks like SetNamedPropertyAccessor isn't the
solution. Still looking for something that will work with ('foo' in chrome) and
chrome.hasOwnProperty('foo')

https://codereview.chromium.org/11571014/diff/42011/chrome/renderer/extension...
chrome/renderer/extensions/module_system.cc:370: v8::Handle<v8::Value>
ModuleSystem::GetNativeFromString(
On 2013/02/13 01:45:49, kalman wrote:
> Yeah this is confusing, can you rename it to
> RequireNative/RequireNativeFromString. Sorry...

Done.

https://codereview.chromium.org/11571014/diff/42011/chrome/renderer/extension...
File chrome/renderer/extensions/module_system.h (right):

https://codereview.chromium.org/11571014/diff/42011/chrome/renderer/extension...
chrome/renderer/extensions/module_system.h:73: void LogExceptionToConsole(const
v8::TryCatch& try_catch);
On 2013/02/13 01:45:49, kalman wrote:
> I think dumping to console also puts to warning. Let's just change
DumpException
> to do that. Also can be private (see comment in other file...)

Switched this back to how it was since the logging is done in binding.js now.

https://codereview.chromium.org/11571014/diff/42011/chrome/renderer/resources...
File chrome/renderer/resources/extensions/apitest.js (right):

https://codereview.chromium.org/11571014/diff/42011/chrome/renderer/resources...
chrome/renderer/resources/extensions/apitest.js:15: chrome_test =
api.compiledApi;
On 2013/02/13 01:45:49, kalman wrote:
> should be chromeTest, but I'd just call it test.

Called it chromeTest because test conflicted in some places.

https://codereview.chromium.org/11571014/diff/42011/chrome/renderer/resources...
File chrome/renderer/resources/extensions/app_custom_bindings.js (right):

https://codereview.chromium.org/11571014/diff/42011/chrome/renderer/resources...
chrome/renderer/resources/extensions/app_custom_bindings.js:73: function
defineCheckAvailabilityGetter(name, apiName, target) {
On 2013/02/13 01:45:49, kalman wrote:
> defineLazyGetter?
> 
> Comment why it's necessary?

This actually doesn't need the lazy getter anymore.

https://codereview.chromium.org/11571014/diff/42011/chrome/renderer/resources...
chrome/renderer/resources/extensions/app_custom_bindings.js:80: return;
On 2013/02/13 01:45:49, kalman wrote:
> might as well say return undefined;

Done.

https://codereview.chromium.org/11571014/diff/42011/chrome/renderer/resources...
File chrome/renderer/resources/extensions/app_runtime_custom_bindings.js
(right):

https://codereview.chromium.org/11571014/diff/42011/chrome/renderer/resources...
chrome/renderer/resources/extensions/app_runtime_custom_bindings.js:18:
chrome.Event;
On 2013/02/13 01:45:49, kalman wrote:
> ?

Deleted.

https://codereview.chromium.org/11571014/diff/42011/chrome/renderer/resources...
File chrome/renderer/resources/extensions/binding.js (right):

https://codereview.chromium.org/11571014/diff/42011/chrome/renderer/resources...
chrome/renderer/resources/extensions/binding.js:171: hook({
On 2013/02/13 01:45:49, kalman wrote:
> There's actually not really any reason to have this runHooks stuff anymore,
that
> was only needed because all generation happens at once.
> 
> Now that it happens per-binding, a much simpler model would be something like:
> 
> rather than
> 
> var binding = Binding.create('foo');
> binding.registerCustomHook(function(api) {
>   api.apiFunctions.setHandleRequest('blah', function() {
>     ...
>   });
> });
> exports.binding = binding.generate();
> 
> have
> 
> var binding = Binding.create('foo');
> binding.setHandleRequest('blah', function() {
>   ...
> });
> exports.binding = binding.generate();
> 
> However, this is a much more invasive change - I reckon we should just land
what
> you have and do it afterwards.
> 
> But yeah: add epic TODO(kalman/cduvall) here :)

Done.

https://codereview.chromium.org/11571014/diff/42011/chrome/renderer/resources...
chrome/renderer/resources/extensions/binding.js:179: generate: function() {
On 2013/02/13 01:45:49, kalman wrote:
> comment

Done.

https://codereview.chromium.org/11571014/diff/42011/chrome/renderer/resources...
chrome/renderer/resources/extensions/binding.js:183: return;
On 2013/02/13 01:45:49, kalman wrote:
> (see below comment: we do anyway).
> 
> Hm, this is another cleanup we could do later, but I think that
GetAvailability
> should also know how to handle this (in fact, Features will).
> 
> TODO(kalman/cduvall): Make GetAvailability handle this, then delete the
> supporting code.

Done.

https://codereview.chromium.org/11571014/diff/42011/chrome/renderer/resources...
chrome/renderer/resources/extensions/binding.js:189: }
On 2013/02/13 01:45:49, kalman wrote:
> I can see that this exception throwing stuff is causing a few niggles. Could
you
> just log the error here, then return undefined? Have the module system deal
with
> that?
> 
> I might be missing something of course - but it seems a lot simpler.

Done.

[not at google - send to devlin, 2013-02-15 22:26:17]

This basically lg to me now, though there are a couple of things to point out
and one worrying thing to fix:
- the "you cannot access the API" message appears to be showing with a stack.
The stack is in our internal code, so it's just misleading - it would be nice to
not show the stack.
- I said something about showing an error message every time an unsuppoted API
is accessed, but this isn't how require() works, so never mind. What you have is
fine.

The worrying thing: I run ExtensionApiTest.ActiveTab and, despite it passing, I
get this:

#
# Fatal error in ../../v8/src/contexts.h, line 215
# CHECK(context->IsContext()) failed
#

==== C stack trace ===============================

 1: ??
 2: ??
 3: ??
 4: v8::Context::Global()
 5: ??
 6: ??
 7: ??
 8: ??
 9: ??
10: ??
11: ??
12: ??
13: ??
14: ??
15: ??
16: ??
17: ??
18: MessageLoop::RunTask(base::PendingTask const&)
19: MessageLoop::DeferOrRunPendingTask(base::PendingTask const&)
20: MessageLoop::DoWork()
21: ??
22: MessageLoop::RunInternal()
23: MessageLoop::RunHandler()
24: base::RunLoop::Run()
25: MessageLoop::Run()
26: ??
27: ??
28: ??
29: ??
30: content::ContentMain(int, char const**, content::ContentMainDelegate*)
31: ??
32: ??
33: ??
34: __libc_start_main
35: ??

Now, it possibly has something to do with one of the times we access Global:

Global
app_window_custom_bindings.cc:    v8::Local<v8::Object> global =
context->Global();
app_window_custom_bindings.cc:  v8::Local<v8::Value> window =
frame->mainWorldScriptContext()->Global();
chrome_v8_context.cc:  v8::Local<v8::Object> global = context->Global();
chrome_v8_context.cc:  v8::Local<v8::Object> global = v8_context_->Global();
dispatcher.cc:  v8::Handle<v8::Object> global(context->Global());
dispatcher.cc:    v8::Handle<v8::Object> global(v8_context->Global());
extension_custom_bindings.cc:      v8::Local<v8::Value> window =
context->Global();
miscellaneous_bindings.cc:   
args->callback->Call(args->callback->CreationContext()->Global(), 0, NULL);
module_system.cc:  v8::Handle<v8::Object> global(context_->Global());
module_system.cc:  context_->Global()->DeleteHiddenValue(
module_system.cc:  v8::Handle<v8::Object>
global(v8::Context::GetCurrent()->Global());
module_system.cc:  v8::Handle<v8::Object> global(context_->Global());
module_system.cc:  v8::Handle<v8::Object> global(context_->Global());
module_system.cc:  v8::Handle<v8::Object>
global(parameters->CreationContext()->Global());

but I'm not so sure. Unfortunately the stack trace is useless - it probably goes
into v8 and into jit code or something - and I also can't repro it outside of
tests :\ this one could be tricky. I've run out of time to look into it today.

Lastly, koz should lg this in addition to me.

And yeah, lg when that error gets fixed.

https://codereview.chromium.org/11571014/diff/55001/chrome/common/extensions/...
File chrome/common/extensions/api/extension_api.cc (right):

https://codereview.chromium.org/11571014/diff/55001/chrome/common/extensions/...
chrome/common/extensions/api/extension_api.cc:358:
IDR_EXTENSION_API_JSON_DEVTOOLS));
Good news! https://codereview.chromium.org/12281003/ fixes the devtools test
failures without touching webkit.

And if that patch gets delayed, removing this reference to the devtools API will
also fix it.

https://codereview.chromium.org/11571014/diff/55001/chrome/common/extensions/...
File chrome/common/extensions/extension.cc (right):

https://codereview.chromium.org/11571014/diff/55001/chrome/common/extensions/...
chrome/common/extensions/extension.cc:1438:
api_permissions.insert(APIPermission::kDevtools);
Yeah never mind, we can revert this.

https://codereview.chromium.org/11571014/diff/55001/chrome/common/extensions/...
File chrome/common/extensions/permissions/permission_set.cc (left):

https://codereview.chromium.org/11571014/diff/55001/chrome/common/extensions/...
chrome/common/extensions/permissions/permission_set.cc:43: "devtools",
ditto

https://codereview.chromium.org/11571014/diff/55001/chrome/renderer/extension...
File chrome/renderer/extensions/chrome_v8_context.cc (right):

https://codereview.chromium.org/11571014/diff/55001/chrome/renderer/extension...
chrome/renderer/extensions/chrome_v8_context.cc:26: const char
kUnavailableMessage[] = "API is unavailable.";
Ok, let's make this nice and informative.

"you do not have permission to access this API. Ensure that the required
permission or manifest property is included in your manifest.json."

https://codereview.chromium.org/11571014/diff/55001/chrome/renderer/extension...
chrome/renderer/extensions/chrome_v8_context.cc:163: if
(available_apis.find(api_name) != available_apis.end())
Call ExtensionAPI::IsAvailable?

https://codereview.chromium.org/11571014/diff/55001/chrome/renderer/extension...
File chrome/renderer/extensions/dispatcher.cc (right):

https://codereview.chromium.org/11571014/diff/55001/chrome/renderer/extension...
chrome/renderer/extensions/dispatcher.cc:360: return v8::Undefined();
I just tested, and console.error works from the only place this is called - so I
don't think we need to add it.

https://codereview.chromium.org/11571014/diff/55001/chrome/renderer/extension...
chrome/renderer/extensions/dispatcher.cc:962:
module_system->Require("event_bindings");
ah, good point.

I just wrote https://codereview.chromium.org/12287011/ so hopefully that'll go
in before this, then you can require json rather than the whole of event.

Also I think this should go up in the extension-only switch case, webpages
shouldn't need to execute it.

https://codereview.chromium.org/11571014/diff/55001/chrome/renderer/extension...
File chrome/renderer/extensions/module_system.cc (right):

https://codereview.chromium.org/11571014/diff/55001/chrome/renderer/extension...
chrome/renderer/extensions/module_system.cc:98: << error_message;
just use +, no stringstream. It's efficient enough.

https://codereview.chromium.org/11571014/diff/55001/chrome/renderer/extension...
File chrome/renderer/extensions/module_system_unittest.cc (right):

https://codereview.chromium.org/11571014/diff/55001/chrome/renderer/extension...
chrome/renderer/extensions/module_system_unittest.cc:9: using
extensions::ModuleSystem;
TODO: put this file in extensions namespace.

https://codereview.chromium.org/11571014/diff/55001/chrome/renderer/extension...
File chrome/renderer/extensions/native_handler.h (right):

https://codereview.chromium.org/11571014/diff/55001/chrome/renderer/extension...
chrome/renderer/extensions/native_handler.h:20: virtual v8::Handle<v8::Object>
NewInstance() = 0;
comment.

https://codereview.chromium.org/11571014/diff/55001/chrome/renderer/resources...
File chrome/renderer/resources/extensions/apitest.js (right):

https://codereview.chromium.org/11571014/diff/55001/chrome/renderer/resources...
chrome/renderer/resources/extensions/apitest.js:15: chromeTest =
api.compiledApi;
var chromeTest

https://codereview.chromium.org/11571014/diff/55001/chrome/renderer/resources...
chrome/renderer/resources/extensions/apitest.js:16: apiFunctions =
api.apiFunctions;
var apiFunctions

https://codereview.chromium.org/11571014/diff/55001/chrome/renderer/resources...
File chrome/renderer/resources/extensions/app_custom_bindings.js (right):

https://codereview.chromium.org/11571014/diff/55001/chrome/renderer/resources...
chrome/renderer/resources/extensions/app_custom_bindings.js:81:
logging.Error('chrome.app is not available: ' + availability.message);
Unfortunately we can't do this, it ends up showing up for all platform apps
because they (a) don't have access to app and (b) always end up running this
binding :\ so yeah, no warning.

https://codereview.chromium.org/11571014/diff/55001/chrome/renderer/resources...
File chrome/renderer/resources/extensions/binding.js (right):

https://codereview.chromium.org/11571014/diff/55001/chrome/renderer/resources...
chrome/renderer/resources/extensions/binding.js:195: availability.message);
Yeah, so, console.error should work fine.

https://codereview.chromium.org/11571014/diff/55001/chrome/renderer/resources...
File chrome/renderer/resources/extensions/event.js (right):

https://codereview.chromium.org/11571014/diff/55001/chrome/renderer/resources...
chrome/renderer/resources/extensions/event.js:6: require('json_schema');
TODO: json_schema shouldn't put things on chromeHidden.

[cduvall, 2013-02-19 23:58:49]

https://codereview.chromium.org/11571014/diff/55001/chrome/common/extensions/...
File chrome/common/extensions/api/extension_api.cc (right):

https://codereview.chromium.org/11571014/diff/55001/chrome/common/extensions/...
chrome/common/extensions/api/extension_api.cc:358:
IDR_EXTENSION_API_JSON_DEVTOOLS));
On 2013/02/15 22:26:17, kalman wrote:
> Good news! https://codereview.chromium.org/12281003/ fixes the devtools test
> failures without touching webkit.
> 
> And if that patch gets delayed, removing this reference to the devtools API
will
> also fix it.

Yay!

https://codereview.chromium.org/11571014/diff/55001/chrome/common/extensions/...
File chrome/common/extensions/extension.cc (right):

https://codereview.chromium.org/11571014/diff/55001/chrome/common/extensions/...
chrome/common/extensions/extension.cc:1438:
api_permissions.insert(APIPermission::kDevtools);
On 2013/02/15 22:26:17, kalman wrote:
> Yeah never mind, we can revert this.

Done.

https://codereview.chromium.org/11571014/diff/55001/chrome/common/extensions/...
File chrome/common/extensions/permissions/permission_set.cc (left):

https://codereview.chromium.org/11571014/diff/55001/chrome/common/extensions/...
chrome/common/extensions/permissions/permission_set.cc:43: "devtools",
On 2013/02/15 22:26:17, kalman wrote:
> ditto

Done.

https://codereview.chromium.org/11571014/diff/55001/chrome/renderer/extension...
File chrome/renderer/extensions/chrome_v8_context.cc (right):

https://codereview.chromium.org/11571014/diff/55001/chrome/renderer/extension...
chrome/renderer/extensions/chrome_v8_context.cc:26: const char
kUnavailableMessage[] = "API is unavailable.";
On 2013/02/15 22:26:17, kalman wrote:
> Ok, let's make this nice and informative.
> 
> "you do not have permission to access this API. Ensure that the required
> permission or manifest property is included in your manifest.json."

Done.

https://codereview.chromium.org/11571014/diff/55001/chrome/renderer/extension...
chrome/renderer/extensions/chrome_v8_context.cc:163: if
(available_apis.find(api_name) != available_apis.end())
On 2013/02/15 22:26:17, kalman wrote:
> Call ExtensionAPI::IsAvailable?

IsAvailable() only works for Features. I added a TODO to switch to that once
Features are ready for all the APIs.

https://codereview.chromium.org/11571014/diff/55001/chrome/renderer/extension...
File chrome/renderer/extensions/dispatcher.cc (right):

https://codereview.chromium.org/11571014/diff/55001/chrome/renderer/extension...
chrome/renderer/extensions/dispatcher.cc:360: return v8::Undefined();
On 2013/02/15 22:26:17, kalman wrote:
> I just tested, and console.error works from the only place this is called - so
I
> don't think we need to add it.

Hmm I don't know why I thought it didn't, I thought I even tested it. But yeah,
console.error works.

https://codereview.chromium.org/11571014/diff/55001/chrome/renderer/extension...
chrome/renderer/extensions/dispatcher.cc:962:
module_system->Require("event_bindings");
On 2013/02/15 22:26:17, kalman wrote:
> ah, good point.
> 
> I just wrote https://codereview.chromium.org/12287011/ so hopefully that'll go
> in before this, then you can require json rather than the whole of event.
> 
> Also I think this should go up in the extension-only switch case, webpages
> shouldn't need to execute it.

Done. I'll change it to include json once your change goes in.

https://codereview.chromium.org/11571014/diff/55001/chrome/renderer/extension...
File chrome/renderer/extensions/module_system.cc (right):

https://codereview.chromium.org/11571014/diff/55001/chrome/renderer/extension...
chrome/renderer/extensions/module_system.cc:98: << error_message;
On 2013/02/15 22:26:17, kalman wrote:
> just use +, no stringstream. It's efficient enough.

I was using a stringstream because GetLineNumber() returns an int that has to be
converted to a string. I switched to StringPrintf because I saw this in some
other places.

https://codereview.chromium.org/11571014/diff/55001/chrome/renderer/extension...
File chrome/renderer/extensions/module_system_unittest.cc (right):

https://codereview.chromium.org/11571014/diff/55001/chrome/renderer/extension...
chrome/renderer/extensions/module_system_unittest.cc:9: using
extensions::ModuleSystem;
On 2013/02/15 22:26:17, kalman wrote:
> TODO: put this file in extensions namespace.

Done.

https://codereview.chromium.org/11571014/diff/55001/chrome/renderer/extension...
File chrome/renderer/extensions/native_handler.h (right):

https://codereview.chromium.org/11571014/diff/55001/chrome/renderer/extension...
chrome/renderer/extensions/native_handler.h:20: virtual v8::Handle<v8::Object>
NewInstance() = 0;
On 2013/02/15 22:26:17, kalman wrote:
> comment.

Done.

https://codereview.chromium.org/11571014/diff/55001/chrome/renderer/resources...
File chrome/renderer/resources/extensions/apitest.js (right):

https://codereview.chromium.org/11571014/diff/55001/chrome/renderer/resources...
chrome/renderer/resources/extensions/apitest.js:15: chromeTest =
api.compiledApi;
On 2013/02/15 22:26:17, kalman wrote:
> var chromeTest

Done.

https://codereview.chromium.org/11571014/diff/55001/chrome/renderer/resources...
chrome/renderer/resources/extensions/apitest.js:16: apiFunctions =
api.apiFunctions;
On 2013/02/15 22:26:17, kalman wrote:
> var apiFunctions

Done.

https://codereview.chromium.org/11571014/diff/55001/chrome/renderer/resources...
File chrome/renderer/resources/extensions/app_custom_bindings.js (right):

https://codereview.chromium.org/11571014/diff/55001/chrome/renderer/resources...
chrome/renderer/resources/extensions/app_custom_bindings.js:81:
logging.Error('chrome.app is not available: ' + availability.message);
On 2013/02/15 22:26:17, kalman wrote:
> Unfortunately we can't do this, it ends up showing up for all platform apps
> because they (a) don't have access to app and (b) always end up running this
> binding :\ so yeah, no warning.

Done.

https://codereview.chromium.org/11571014/diff/55001/chrome/renderer/resources...
File chrome/renderer/resources/extensions/binding.js (right):

https://codereview.chromium.org/11571014/diff/55001/chrome/renderer/resources...
chrome/renderer/resources/extensions/binding.js:195: availability.message);
On 2013/02/15 22:26:17, kalman wrote:
> Yeah, so, console.error should work fine.

Done.

https://codereview.chromium.org/11571014/diff/55001/chrome/renderer/resources...
File chrome/renderer/resources/extensions/event.js (right):

https://codereview.chromium.org/11571014/diff/55001/chrome/renderer/resources...
chrome/renderer/resources/extensions/event.js:6: require('json_schema');
On 2013/02/15 22:26:17, kalman wrote:
> TODO: json_schema shouldn't put things on chromeHidden.

Done.

[not at google - send to devlin, 2013-02-20 00:36:38]

lgtm

This is a pretty epic and far-reaching change so I want to get Matt to sanity
check it too.

https://codereview.chromium.org/11571014/diff/55001/chrome/renderer/extension...
File chrome/renderer/extensions/chrome_v8_context.cc (right):

https://codereview.chromium.org/11571014/diff/55001/chrome/renderer/extension...
chrome/renderer/extensions/chrome_v8_context.cc:26: const char
kUnavailableMessage[] = "API is unavailable.";
On 2013/02/19 23:58:49, cduvall wrote:
> On 2013/02/15 22:26:17, kalman wrote:
> > Ok, let's make this nice and informative.
> > 
> > "you do not have permission to access this API. Ensure that the required
> > permission or manifest property is included in your manifest.json."
> 
> Done.

Nice, could you make "this API" be %s and use StringPrintf?

https://codereview.chromium.org/11571014/diff/68001/chrome/renderer/extension...
File chrome/renderer/extensions/chrome_v8_context.h (right):

https://codereview.chromium.org/11571014/diff/68001/chrome/renderer/extension...
chrome/renderer/extensions/chrome_v8_context.h:39: void Invalidate();
comment

https://codereview.chromium.org/11571014/diff/68001/chrome/renderer/extension...
chrome/renderer/extensions/chrome_v8_context.h:53: web_frame_ = NULL;
let's merge this into Invalidate().

https://codereview.chromium.org/11571014/diff/68001/chrome/renderer/extension...
File chrome/renderer/extensions/module_system.cc (right):

https://codereview.chromium.org/11571014/diff/68001/chrome/renderer/extension...
chrome/renderer/extensions/module_system.cc:53: is_valid_ = false;
I can't get over the weird placement of this line :) sorry. Put at end of
function?

https://codereview.chromium.org/11571014/diff/68001/chrome/renderer/extension...
chrome/renderer/extensions/module_system.cc:104: return
base::StringPrintf("[%s(%d)]%s",
This is supposed to say like

[foo(10)]Cannot access property foo of undefined

right?

I think something like

foo:10: Cannot access property foo of undefined

is more standard? Am I mistaken here?

https://codereview.chromium.org/11571014/diff/68001/chrome/renderer/extension...
chrome/renderer/extensions/module_system.cc:141: if (!is_valid_)
This shouldn't be possible, though, of course, if we make this a CHECK I'll
probably start getting crash reports for it.

But make it a CHECK anyway.

https://codereview.chromium.org/11571014/diff/68001/chrome/renderer/extension...
File chrome/renderer/extensions/module_system.h (right):

https://codereview.chromium.org/11571014/diff/68001/chrome/renderer/extension...
chrome/renderer/extensions/module_system.h:133: // Invalidate this module system
so it cannot be used any more.
This is needed because it's possible for this to outlive |context_|. Invalidate
must be called before this happens.

[not at google - send to devlin, 2013-02-20 00:49:29]

https://codereview.chromium.org/11571014/diff/55001/chrome/renderer/extension...
File chrome/renderer/extensions/chrome_v8_context.cc (right):

https://codereview.chromium.org/11571014/diff/55001/chrome/renderer/extension...
chrome/renderer/extensions/chrome_v8_context.cc:26: const char
kUnavailableMessage[] = "API is unavailable.";
On 2013/02/20 00:36:38, kalman wrote:
> On 2013/02/19 23:58:49, cduvall wrote:
> > On 2013/02/15 22:26:17, kalman wrote:
> > > Ok, let's make this nice and informative.
> > > 
> > > "you do not have permission to access this API. Ensure that the required
> > > permission or manifest property is included in your manifest.json."
> > 
> > Done.
> 
> Nice, could you make "this API" be %s and use StringPrintf?

Never mind, being dumb. Ignore.

[cduvall, 2013-02-20 00:57:53]

https://codereview.chromium.org/11571014/diff/68001/chrome/renderer/extension...
File chrome/renderer/extensions/chrome_v8_context.h (right):

https://codereview.chromium.org/11571014/diff/68001/chrome/renderer/extension...
chrome/renderer/extensions/chrome_v8_context.h:39: void Invalidate();
On 2013/02/20 00:36:38, kalman wrote:
> comment

Done.

https://codereview.chromium.org/11571014/diff/68001/chrome/renderer/extension...
chrome/renderer/extensions/chrome_v8_context.h:53: web_frame_ = NULL;
On 2013/02/20 00:36:38, kalman wrote:
> let's merge this into Invalidate().

Done.

https://codereview.chromium.org/11571014/diff/68001/chrome/renderer/extension...
File chrome/renderer/extensions/module_system.cc (right):

https://codereview.chromium.org/11571014/diff/68001/chrome/renderer/extension...
chrome/renderer/extensions/module_system.cc:53: is_valid_ = false;
On 2013/02/20 00:36:38, kalman wrote:
> I can't get over the weird placement of this line :) sorry. Put at end of
> function?

Done.

https://codereview.chromium.org/11571014/diff/68001/chrome/renderer/extension...
chrome/renderer/extensions/module_system.cc:104: return
base::StringPrintf("[%s(%d)]%s",
On 2013/02/20 00:36:38, kalman wrote:
> This is supposed to say like
> 
> [foo(10)]Cannot access property foo of undefined
> 
> right?
> 
> I think something like
> 
> foo:10: Cannot access property foo of undefined
> 
> is more standard? Am I mistaken here?

I just had it like this because thats how it was before. I think your way is
more standard though.

https://codereview.chromium.org/11571014/diff/68001/chrome/renderer/extension...
chrome/renderer/extensions/module_system.cc:141: if (!is_valid_)
On 2013/02/20 00:36:38, kalman wrote:
> This shouldn't be possible, though, of course, if we make this a CHECK I'll
> probably start getting crash reports for it.
> 
> But make it a CHECK anyway.

Done.

https://codereview.chromium.org/11571014/diff/68001/chrome/renderer/extension...
File chrome/renderer/extensions/module_system.h (right):

https://codereview.chromium.org/11571014/diff/68001/chrome/renderer/extension...
chrome/renderer/extensions/module_system.h:133: // Invalidate this module system
so it cannot be used any more.
On 2013/02/20 00:36:38, kalman wrote:
> This is needed because it's possible for this to outlive |context_|.
Invalidate
> must be called before this happens.

Done.

[Matt Perry, 2013-02-20 02:40:45]

I'll take a look, but don't let me stop you from landing this. If I find
anything, you can fix it in a followup CL.

https://codereview.chromium.org/11571014/diff/77009/chrome/renderer/resources...
File chrome/renderer/resources/extensions/binding.js (right):

https://codereview.chromium.org/11571014/diff/77009/chrome/renderer/resources...
chrome/renderer/resources/extensions/binding.js:23: this._apiFunctions = {};
style is apiFunctions_ for private methods/vars

[cduvall, 2013-02-20 17:47:03]

https://codereview.chromium.org/11571014/diff/77009/chrome/renderer/resources...
File chrome/renderer/resources/extensions/binding.js (right):

https://codereview.chromium.org/11571014/diff/77009/chrome/renderer/resources...
chrome/renderer/resources/extensions/binding.js:23: this._apiFunctions = {};
On 2013/02/20 02:40:46, Matt Perry wrote:
> style is apiFunctions_ for private methods/vars

Done.

[commit-bot: I haz the power, 2013-02-20 17:47:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/cduvall@chromium.org/11571014/84001

[commit-bot: I haz the power, 2013-02-20 17:48:29]

Presubmit check for 11571014-84001 failed and returned exit status 1.

INFO:root:Found 73 file(s).

Running presubmit commit checks ...
Running /b/commit-queue/workdir/chromium/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/common/extensions/PRESUBMIT.py

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome
    chrome/browser/autocomplete
    chrome/test

Presubmit checks took 6.3s to calculate.

[commit-bot: I haz the power, 2013-02-20 17:50:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/cduvall@chromium.org/11571014/84001

[commit-bot: I haz the power, 2013-02-20 18:45:16]

Failed to trigger a try job on mac_rel
HTTP Error 400: Bad Request

[commit-bot: I haz the power, 2013-02-20 18:45:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/cduvall@chromium.org/11571014/88004

[commit-bot: I haz the power, 2013-02-20 21:29:57]

Retried try job too often on mac_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[Matt Perry, 2013-02-21 01:07:57]

https://codereview.chromium.org/11571014/diff/88004/chrome/renderer/resources...
File chrome/renderer/resources/extensions/binding.js (right):

https://codereview.chromium.org/11571014/diff/88004/chrome/renderer/resources...
chrome/renderer/resources/extensions/binding.js:122: this._schema = schema;
you missed these ones.

[Matt Perry, 2013-02-21 01:57:43]

LGTM! Great stuff. 2 more minor comments.

https://codereview.chromium.org/11571014/diff/88004/chrome/renderer/extension...
File chrome/renderer/extensions/dispatcher.h (right):

https://codereview.chromium.org/11571014/diff/88004/chrome/renderer/extension...
chrome/renderer/extensions/dispatcher.h:217: typedef void
(ModuleSystem::*LazyFieldSetter)(v8::Handle<v8::Object>,
Is this used anywhere? And if so, why does it belong here?

https://codereview.chromium.org/11571014/diff/88004/chrome/renderer/resources...
File chrome/renderer/resources/extensions/apitest.js (right):

https://codereview.chromium.org/11571014/diff/88004/chrome/renderer/resources...
chrome/renderer/resources/extensions/apitest.js:5: // extension_apitest.js
Let's rename this file to test_custom_bindings.js

[miu, 2013-02-26 00:34:11]

FYI--I added bug 177163 to the BUG= line.

[cduvall, 2013-02-26 00:36:56]

https://codereview.chromium.org/11571014/diff/88004/chrome/renderer/extension...
File chrome/renderer/extensions/dispatcher.h (right):

https://codereview.chromium.org/11571014/diff/88004/chrome/renderer/extension...
chrome/renderer/extensions/dispatcher.h:217: typedef void
(ModuleSystem::*LazyFieldSetter)(v8::Handle<v8::Object>,
On 2013/02/21 01:57:43, Matt Perry wrote:
> Is this used anywhere? And if so, why does it belong here?

Removed.

https://codereview.chromium.org/11571014/diff/88004/chrome/renderer/resources...
File chrome/renderer/resources/extensions/apitest.js (right):

https://codereview.chromium.org/11571014/diff/88004/chrome/renderer/resources...
chrome/renderer/resources/extensions/apitest.js:5: // extension_apitest.js
On 2013/02/21 01:57:43, Matt Perry wrote:
> Let's rename this file to test_custom_bindings.js

Done.

https://codereview.chromium.org/11571014/diff/88004/chrome/renderer/resources...
File chrome/renderer/resources/extensions/binding.js (right):

https://codereview.chromium.org/11571014/diff/88004/chrome/renderer/resources...
chrome/renderer/resources/extensions/binding.js:122: this._schema = schema;
On 2013/02/21 01:07:57, Matt Perry wrote:
> you missed these ones.

Oops, got them now.

[commit-bot: I haz the power, 2013-02-26 00:38:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/cduvall@chromium.org/11571014/91118

[commit-bot: I haz the power, 2013-02-26 01:10:43]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_chromeos_clang.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-02-26 01:12:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/cduvall@chromium.org/11571014/85036

[commit-bot: I haz the power, 2013-02-26 03:22:14]

Failed to trigger a try job on linux_rel
HTTP Error 400: Bad Request

[commit-bot: I haz the power, 2013-02-26 03:22:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/cduvall@chromium.org/11571014/95071

[commit-bot: I haz the power, 2013-02-26 04:54:13]

Retried try job too often on mac_rel for step(s) interactive_ui_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2013-02-26 21:53:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/cduvall@chromium.org/11571014/85050

[commit-bot: I haz the power, 2013-02-27 00:31:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/cduvall@chromium.org/11571014/95078

[commit-bot: I haz the power, 2013-02-27 02:38:04]

Change committed as 184837

[not at google - send to devlin, 2013-03-01 00:36:05]

lgtm

https://codereview.chromium.org/11571014/diff/116001/chrome/renderer/extensio...
File chrome/renderer/extensions/dispatcher.cc (right):

https://codereview.chromium.org/11571014/diff/116001/chrome/renderer/extensio...
chrome/renderer/extensions/dispatcher.cc:636: if (!value->IsUndefined() &&
value->IsObject())
I think that IsObject implies !undefined so the first check can be removed.

[cduvall, 2013-03-01 00:43:52]

https://codereview.chromium.org/11571014/diff/116001/chrome/renderer/extensio...
File chrome/renderer/extensions/dispatcher.cc (right):

https://codereview.chromium.org/11571014/diff/116001/chrome/renderer/extensio...
chrome/renderer/extensions/dispatcher.cc:636: if (!value->IsUndefined() &&
value->IsObject())
On 2013/03/01 00:36:06, kalman wrote:
> I think that IsObject implies !undefined so the first check can be removed.

Done.

[commit-bot: I haz the power, 2013-03-01 00:48:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/cduvall@chromium.org/11571014/113003

[commit-bot: I haz the power, 2013-03-01 03:09:49]

Failed to apply patch for chrome/renderer/extensions/dispatcher.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/renderer/extensions/dispatcher.cc
  Hunk #1 FAILED at 8.
  Hunk #2 succeeded at 24 (offset 1 line).
  Hunk #3 succeeded at 40 (offset 1 line).
  Hunk #4 succeeded at 91 (offset 1 line).
  Hunk #5 succeeded at 213 (offset 1 line).
  Hunk #6 succeeded at 226 (offset 1 line).
  Hunk #7 succeeded at 315 (offset 1 line).
  Hunk #8 succeeded at 346 (offset 1 line).
  Hunk #9 succeeded at 377 (offset 1 line).
  Hunk #10 succeeded at 397 (offset 1 line).
  Hunk #11 succeeded at 621 (offset 1 line).
  Hunk #12 succeeded at 755 (offset 1 line).
  Hunk #13 succeeded at 824 (offset 1 line).
  Hunk #14 succeeded at 901 (offset 1 line).
  Hunk #15 succeeded at 939 (offset 1 line).
  Hunk #16 succeeded at 963 (offset 1 line).
  Hunk #17 succeeded at 1004 (offset 1 line).
  Hunk #18 succeeded at 1242 (offset 1 line).
  1 out of 18 hunks FAILED -- saving rejects to file
chrome/renderer/extensions/dispatcher.cc.rej

Patch:       chrome/renderer/extensions/dispatcher.cc
Index: chrome/renderer/extensions/dispatcher.cc
diff --git a/chrome/renderer/extensions/dispatcher.cc
b/chrome/renderer/extensions/dispatcher.cc
index
930cb5d568e4bfc8812c9d484c4820e8c8efeaf5..198ad6695a41896c9225621ff717ca200d5ff862
100644
--- a/chrome/renderer/extensions/dispatcher.cc
+++ b/chrome/renderer/extensions/dispatcher.cc
@@ -8,12 +8,14 @@
 #include "base/command_line.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/string_piece.h"
+#include "base/string_split.h"
 #include "chrome/common/child_process_logging.h"
 #include "chrome/common/chrome_switches.h"
 #include "chrome/common/chrome_version_info.h"
 #include "chrome/common/extensions/api/extension_api.h"
 #include "chrome/common/extensions/extension.h"
 #include "chrome/common/extensions/extension_messages.h"
+#include "chrome/common/extensions/features/feature.h"
 #include "chrome/common/extensions/manifest.h"
 #include "chrome/common/extensions/permissions/permission_set.h"
 #include "chrome/common/url_constants.h"
@@ -23,6 +25,7 @@
 #include "chrome/renderer/extensions/app_bindings.h"
 #include "chrome/renderer/extensions/app_runtime_custom_bindings.h"
 #include "chrome/renderer/extensions/app_window_custom_bindings.h"
+#include "chrome/renderer/extensions/binding_generating_native_handler.h"
 #include "chrome/renderer/extensions/chrome_v8_context.h"
 #include "chrome/renderer/extensions/chrome_v8_extension.h"
 #include "chrome/renderer/extensions/content_watcher.h"
@@ -38,7 +41,7 @@
 #include "chrome/renderer/extensions/media_galleries_custom_bindings.h"
 #include "chrome/renderer/extensions/miscellaneous_bindings.h"
 #include "chrome/renderer/extensions/module_system.h"
-#include "chrome/renderer/extensions/native_handler.h"
+#include "chrome/renderer/extensions/object_backed_native_handler.h"
 #include "chrome/renderer/extensions/page_actions_custom_bindings.h"
 #include "chrome/renderer/extensions/page_capture_custom_bindings.h"
 #include "chrome/renderer/extensions/request_sender.h"
@@ -89,24 +92,97 @@ static const char kEventDispatchFunction[] =
"Event.dispatchEvent";
 static const char kOnSuspendEvent[] = "runtime.onSuspend";
 static const char kOnSuspendCanceledEvent[] = "runtime.onSuspendCanceled";
 
-class ChromeHiddenNativeHandler : public NativeHandler {
+static v8::Handle<v8::Object> GetOrCreateChrome(
+    v8::Handle<v8::Context> context) {
+  v8::Handle<v8::String> chrome_string(v8::String::New("chrome"));
+  v8::Handle<v8::Object> global(context->Global());
+  v8::Handle<v8::Value> chrome(global->Get(chrome_string));
+  if (chrome.IsEmpty() || chrome->IsUndefined()) {
+    v8::Handle<v8::Object> chrome_object(v8::Object::New());
+    global->Set(chrome_string, chrome_object);
+    return chrome_object;
+  }
+  CHECK(chrome->IsObject());
+  return chrome->ToObject();
+}
+
+class SchemaRegistryNativeHandler : public ObjectBackedNativeHandler {
  public:
-  explicit ChromeHiddenNativeHandler(v8::Isolate* isolate)
-      : NativeHandler(isolate) {
+  SchemaRegistryNativeHandler(V8SchemaRegistry* registry,
+                              v8::Handle<v8::Context> context)
+      : ObjectBackedNativeHandler(context),
+        registry_(registry) {
+    RouteFunction("GetSchema",
+        base::Bind(&SchemaRegistryNativeHandler::GetSchema,
+                   base::Unretained(this)));
+  }
+
+ private:
+  v8::Handle<v8::Value> GetSchema(const v8::Arguments& args) {
+    return registry_->GetSchema(*v8::String::AsciiValue(args[0]));
+  }
+
+  V8SchemaRegistry* registry_;
+};
+
+class V8ContextNativeHandler : public ObjectBackedNativeHandler {
+ public:
+  explicit V8ContextNativeHandler(ChromeV8Context* context)
+      : ObjectBackedNativeHandler(context->v8_context()),
+        context_(context) {
+    RouteFunction("GetAvailability",
+        base::Bind(&V8ContextNativeHandler::GetAvailability,
+                   base::Unretained(this)));
+  }
+
+ private:
+  v8::Handle<v8::Value> GetAvailability(const v8::Arguments& args) {
+    CHECK_EQ(args.Length(), 1);
+    std::string api_name = *v8::String::AsciiValue(args[0]->ToString());
+    Feature::Availability availability = context_->GetAvailability(api_name);
+
+    v8::Handle<v8::Object> ret = v8::Object::New();
+    ret->Set(v8::String::New("is_available"),
+             v8::Boolean::New(availability.is_available()));
+    ret->Set(v8::String::New("message"),
+             v8::String::New(availability.message().c_str()));
+    return ret;
+  }
+
+  ChromeV8Context* context_;
+};
+
+class ChromeHiddenNativeHandler : public ObjectBackedNativeHandler {
+ public:
+  explicit ChromeHiddenNativeHandler(v8::Handle<v8::Context> context)
+      : ObjectBackedNativeHandler(context) {
     RouteFunction("GetChromeHidden",
         base::Bind(&ChromeHiddenNativeHandler::GetChromeHidden,
                    base::Unretained(this)));
   }
 
   v8::Handle<v8::Value> GetChromeHidden(const v8::Arguments& args) {
-    return ChromeV8Context::GetOrCreateChromeHidden(v8::Context::GetCurrent());
+    return ChromeV8Context::GetOrCreateChromeHidden(v8_context());
   }
 };
 
-class PrintNativeHandler : public NativeHandler {
+class ChromeNativeHandler : public ObjectBackedNativeHandler {
  public:
-  explicit PrintNativeHandler(v8::Isolate* isolate)
-      : NativeHandler(isolate) {
+  explicit ChromeNativeHandler(v8::Handle<v8::Context> context)
+      : ObjectBackedNativeHandler(context) {
+    RouteFunction("GetChrome",
+        base::Bind(&ChromeNativeHandler::GetChrome, base::Unretained(this)));
+  }
+
+  v8::Handle<v8::Value> GetChrome(const v8::Arguments& args) {
+    return GetOrCreateChrome(v8_context());
+  }
+};
+
+class PrintNativeHandler : public ObjectBackedNativeHandler {
+ public:
+  explicit PrintNativeHandler(v8::Handle<v8::Context> context)
+      : ObjectBackedNativeHandler(context) {
     RouteFunction("Print",
         base::Bind(&PrintNativeHandler::Print,
                    base::Unretained(this)));
@@ -138,7 +214,8 @@ class LazyBackgroundPageNativeHandler : public
ChromeV8Extension {
   }
 
   v8::Handle<v8::Value> IncrementKeepaliveCount(const v8::Arguments& args) {
-    ChromeV8Context* context = dispatcher()->v8_context_set().GetCurrent();
+    ChromeV8Context* context =
+        dispatcher()->v8_context_set().GetByV8Context(v8_context());
     if (!context)
       return v8::Undefined();
     RenderView* render_view = context->GetRenderView();
@@ -150,7 +227,8 @@ class LazyBackgroundPageNativeHandler : public
ChromeV8Extension {
   }
 
   v8::Handle<v8::Value> DecrementKeepaliveCount(const v8::Arguments& args) {
-    ChromeV8Context* context = dispatcher()->v8_context_set().GetCurrent();
+    ChromeV8Context* context =
+        dispatcher()->v8_context_set().GetByV8Context(v8_context());
     if (!context)
       return v8::Undefined();
     RenderView* render_view = context->GetRenderView();
@@ -238,13 +316,12 @@ class ProcessInfoNativeHandler : public ChromeV8Extension
{
   bool send_request_disabled_;
 };
 
-class LoggingNativeHandler : public NativeHandler {
+class LoggingNativeHandler : public ObjectBackedNativeHandler {
  public:
-  explicit LoggingNativeHandler(v8::Isolate* isolate)
-      : NativeHandler(isolate) {
+  explicit LoggingNativeHandler(v8::Handle<v8::Context> context)
+      : ObjectBackedNativeHandler(context) {
     RouteFunction("DCHECK",
-        base::Bind(&LoggingNativeHandler::Dcheck,
-                   base::Unretained(this)));
+        base::Bind(&LoggingNativeHandler::Dcheck, base::Unretained(this)));
   }
 
   v8::Handle<v8::Value> Dcheck(const v8::Arguments& args) {
@@ -270,7 +347,6 @@ class LoggingNativeHandler : public NativeHandler {
       }
     }
     DCHECK(check_value) << error_message;
-    LOG(WARNING) << error_message;
     return v8::Undefined();
   }
 
@@ -302,20 +378,6 @@ void InstallWebstoreBindings(ModuleSystem* module_system,
                               "chromeHiddenWebstore");
 }
 
-static v8::Handle<v8::Object> GetOrCreateChrome(
-    v8::Handle<v8::Context> context) {
-  v8::Handle<v8::String> chrome_string(v8::String::New("chrome"));
-  v8::Handle<v8::Object> global(context->Global());
-  v8::Handle<v8::Value> chrome(global->Get(chrome_string));
-  if (chrome.IsEmpty() || chrome->IsUndefined()) {
-    v8::Handle<v8::Object> chrome_object(v8::Object::New());
-    global->Set(chrome_string, chrome_object);
-    return chrome_object;
-  }
-  CHECK(chrome->IsObject());
-  return chrome->ToObject();
-}
-
 }  // namespace
 
 Dispatcher::Dispatcher()
@@ -336,7 +398,7 @@ Dispatcher::Dispatcher()
   }
 
   user_script_slave_.reset(new UserScriptSlave(&extensions_));
-  request_sender_.reset(new RequestSender(this, &v8_context_set_));
+  request_sender_.reset(new RequestSender(this));
   PopulateSourceMap();
   PopulateLazyBindingsMap();
 }
@@ -560,58 +622,128 @@ bool Dispatcher::AllowScriptExtensio…
(message too large)

[commit-bot: I haz the power, 2013-03-01 16:59:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/cduvall@chromium.org/11571014/110019

[commit-bot: I haz the power, 2013-03-01 18:19:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/cduvall@chromium.org/11571014/112035

[commit-bot: I haz the power, 2013-03-01 19:03:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/cduvall@chromium.org/11571014/118146

[commit-bot: I haz the power, 2013-03-02 07:22:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/cduvall@chromium.org/11571014/111154

[commit-bot: I haz the power, 2013-03-02 16:01:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/cduvall@chromium.org/11571014/111154

[commit-bot: I haz the power, 2013-03-02 16:58:44]

Change committed as 185709

[commit-bot: I haz the power, 2013-03-05 22:25:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/cduvall@chromium.org/11571014/130001

[commit-bot: I haz the power, 2013-03-06 00:24:32]

Retried try job too often on mac_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2013-03-06 20:44:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/cduvall@chromium.org/11571014/163005

[commit-bot: I haz the power, 2013-03-06 20:45:04]

Failed to apply patch for chrome/renderer/extensions/dispatcher.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/renderer/extensions/dispatcher.cc
  Hunk #14 FAILED at 624.
  1 out of 22 hunks FAILED -- saving rejects to file
chrome/renderer/extensions/dispatcher.cc.rej

Patch:       chrome/renderer/extensions/dispatcher.cc
Index: chrome/renderer/extensions/dispatcher.cc
diff --git a/chrome/renderer/extensions/dispatcher.cc
b/chrome/renderer/extensions/dispatcher.cc
index
770a771d0302bda1058e14bcc1e5f32e067be67d..af039af1b28cd81555f577c6fa46a9af8d45e00c
100644
--- a/chrome/renderer/extensions/dispatcher.cc
+++ b/chrome/renderer/extensions/dispatcher.cc
@@ -8,6 +8,7 @@
 #include "base/command_line.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/string_piece.h"
+#include "base/string_split.h"
 #include "chrome/common/child_process_logging.h"
 #include "chrome/common/chrome_switches.h"
 #include "chrome/common/chrome_version_info.h"
@@ -15,6 +16,7 @@
 #include "chrome/common/extensions/background_info.h"
 #include "chrome/common/extensions/extension.h"
 #include "chrome/common/extensions/extension_messages.h"
+#include "chrome/common/extensions/features/feature.h"
 #include "chrome/common/extensions/manifest.h"
 #include "chrome/common/extensions/permissions/permission_set.h"
 #include "chrome/common/url_constants.h"
@@ -24,6 +26,7 @@
 #include "chrome/renderer/extensions/app_bindings.h"
 #include "chrome/renderer/extensions/app_runtime_custom_bindings.h"
 #include "chrome/renderer/extensions/app_window_custom_bindings.h"
+#include "chrome/renderer/extensions/binding_generating_native_handler.h"
 #include "chrome/renderer/extensions/chrome_v8_context.h"
 #include "chrome/renderer/extensions/chrome_v8_extension.h"
 #include "chrome/renderer/extensions/content_watcher.h"
@@ -39,7 +42,7 @@
 #include "chrome/renderer/extensions/media_galleries_custom_bindings.h"
 #include "chrome/renderer/extensions/miscellaneous_bindings.h"
 #include "chrome/renderer/extensions/module_system.h"
-#include "chrome/renderer/extensions/native_handler.h"
+#include "chrome/renderer/extensions/object_backed_native_handler.h"
 #include "chrome/renderer/extensions/page_actions_custom_bindings.h"
 #include "chrome/renderer/extensions/page_capture_custom_bindings.h"
 #include "chrome/renderer/extensions/request_sender.h"
@@ -90,24 +93,97 @@ static const char kEventDispatchFunction[] =
"Event.dispatchEvent";
 static const char kOnSuspendEvent[] = "runtime.onSuspend";
 static const char kOnSuspendCanceledEvent[] = "runtime.onSuspendCanceled";
 
-class ChromeHiddenNativeHandler : public NativeHandler {
+static v8::Handle<v8::Object> GetOrCreateChrome(
+    v8::Handle<v8::Context> context) {
+  v8::Handle<v8::String> chrome_string(v8::String::New("chrome"));
+  v8::Handle<v8::Object> global(context->Global());
+  v8::Handle<v8::Value> chrome(global->Get(chrome_string));
+  if (chrome.IsEmpty() || chrome->IsUndefined()) {
+    v8::Handle<v8::Object> chrome_object(v8::Object::New());
+    global->Set(chrome_string, chrome_object);
+    return chrome_object;
+  }
+  CHECK(chrome->IsObject());
+  return chrome->ToObject();
+}
+
+class SchemaRegistryNativeHandler : public ObjectBackedNativeHandler {
+ public:
+  SchemaRegistryNativeHandler(V8SchemaRegistry* registry,
+                              v8::Handle<v8::Context> context)
+      : ObjectBackedNativeHandler(context),
+        registry_(registry) {
+    RouteFunction("GetSchema",
+        base::Bind(&SchemaRegistryNativeHandler::GetSchema,
+                   base::Unretained(this)));
+  }
+
+ private:
+  v8::Handle<v8::Value> GetSchema(const v8::Arguments& args) {
+    return registry_->GetSchema(*v8::String::AsciiValue(args[0]));
+  }
+
+  V8SchemaRegistry* registry_;
+};
+
+class V8ContextNativeHandler : public ObjectBackedNativeHandler {
  public:
-  explicit ChromeHiddenNativeHandler(v8::Isolate* isolate)
-      : NativeHandler(isolate) {
+  explicit V8ContextNativeHandler(ChromeV8Context* context)
+      : ObjectBackedNativeHandler(context->v8_context()),
+        context_(context) {
+    RouteFunction("GetAvailability",
+        base::Bind(&V8ContextNativeHandler::GetAvailability,
+                   base::Unretained(this)));
+  }
+
+ private:
+  v8::Handle<v8::Value> GetAvailability(const v8::Arguments& args) {
+    CHECK_EQ(args.Length(), 1);
+    std::string api_name = *v8::String::AsciiValue(args[0]->ToString());
+    Feature::Availability availability = context_->GetAvailability(api_name);
+
+    v8::Handle<v8::Object> ret = v8::Object::New();
+    ret->Set(v8::String::New("is_available"),
+             v8::Boolean::New(availability.is_available()));
+    ret->Set(v8::String::New("message"),
+             v8::String::New(availability.message().c_str()));
+    return ret;
+  }
+
+  ChromeV8Context* context_;
+};
+
+class ChromeHiddenNativeHandler : public ObjectBackedNativeHandler {
+ public:
+  explicit ChromeHiddenNativeHandler(v8::Handle<v8::Context> context)
+      : ObjectBackedNativeHandler(context) {
     RouteFunction("GetChromeHidden",
         base::Bind(&ChromeHiddenNativeHandler::GetChromeHidden,
                    base::Unretained(this)));
   }
 
   v8::Handle<v8::Value> GetChromeHidden(const v8::Arguments& args) {
-    return ChromeV8Context::GetOrCreateChromeHidden(v8::Context::GetCurrent());
+    return ChromeV8Context::GetOrCreateChromeHidden(v8_context());
+  }
+};
+
+class ChromeNativeHandler : public ObjectBackedNativeHandler {
+ public:
+  explicit ChromeNativeHandler(v8::Handle<v8::Context> context)
+      : ObjectBackedNativeHandler(context) {
+    RouteFunction("GetChrome",
+        base::Bind(&ChromeNativeHandler::GetChrome, base::Unretained(this)));
+  }
+
+  v8::Handle<v8::Value> GetChrome(const v8::Arguments& args) {
+    return GetOrCreateChrome(v8_context());
   }
 };
 
-class PrintNativeHandler : public NativeHandler {
+class PrintNativeHandler : public ObjectBackedNativeHandler {
  public:
-  explicit PrintNativeHandler(v8::Isolate* isolate)
-      : NativeHandler(isolate) {
+  explicit PrintNativeHandler(v8::Handle<v8::Context> context)
+      : ObjectBackedNativeHandler(context) {
     RouteFunction("Print",
         base::Bind(&PrintNativeHandler::Print,
                    base::Unretained(this)));
@@ -128,8 +204,9 @@ class PrintNativeHandler : public NativeHandler {
 
 class LazyBackgroundPageNativeHandler : public ChromeV8Extension {
  public:
-  explicit LazyBackgroundPageNativeHandler(Dispatcher* dispatcher)
-      : ChromeV8Extension(dispatcher) {
+  LazyBackgroundPageNativeHandler(Dispatcher* dispatcher,
+                                  v8::Handle<v8::Context> context)
+      : ChromeV8Extension(dispatcher, context) {
     RouteFunction("IncrementKeepaliveCount",
         base::Bind(&LazyBackgroundPageNativeHandler::IncrementKeepaliveCount,
                    base::Unretained(this)));
@@ -139,7 +216,8 @@ class LazyBackgroundPageNativeHandler : public
ChromeV8Extension {
   }
 
   v8::Handle<v8::Value> IncrementKeepaliveCount(const v8::Arguments& args) {
-    ChromeV8Context* context = dispatcher()->v8_context_set().GetCurrent();
+    ChromeV8Context* context =
+        dispatcher()->v8_context_set().GetByV8Context(v8_context());
     if (!context)
       return v8::Undefined();
     RenderView* render_view = context->GetRenderView();
@@ -151,7 +229,8 @@ class LazyBackgroundPageNativeHandler : public
ChromeV8Extension {
   }
 
   v8::Handle<v8::Value> DecrementKeepaliveCount(const v8::Arguments& args) {
-    ChromeV8Context* context = dispatcher()->v8_context_set().GetCurrent();
+    ChromeV8Context* context =
+        dispatcher()->v8_context_set().GetByV8Context(v8_context());
     if (!context)
       return v8::Undefined();
     RenderView* render_view = context->GetRenderView();
@@ -176,14 +255,14 @@ class LazyBackgroundPageNativeHandler : public
ChromeV8Extension {
 
 class ProcessInfoNativeHandler : public ChromeV8Extension {
  public:
-  explicit ProcessInfoNativeHandler(
-      Dispatcher* dispatcher,
-      const std::string& extension_id,
-      const std::string& context_type,
-      bool is_incognito_context,
-      int manifest_version,
-      bool send_request_disabled)
-      : ChromeV8Extension(dispatcher),
+  ProcessInfoNativeHandler(Dispatcher* dispatcher,
+                           v8::Handle<v8::Context> context,
+                           const std::string& extension_id,
+                           const std::string& context_type,
+                           bool is_incognito_context,
+                           int manifest_version,
+                           bool send_request_disabled)
+      : ChromeV8Extension(dispatcher, context),
         extension_id_(extension_id),
         context_type_(context_type),
         is_incognito_context_(is_incognito_context),
@@ -239,13 +318,12 @@ class ProcessInfoNativeHandler : public ChromeV8Extension
{
   bool send_request_disabled_;
 };
 
-class LoggingNativeHandler : public NativeHandler {
+class LoggingNativeHandler : public ObjectBackedNativeHandler {
  public:
-  explicit LoggingNativeHandler(v8::Isolate* isolate)
-      : NativeHandler(isolate) {
+  explicit LoggingNativeHandler(v8::Handle<v8::Context> context)
+      : ObjectBackedNativeHandler(context) {
     RouteFunction("DCHECK",
-        base::Bind(&LoggingNativeHandler::Dcheck,
-                   base::Unretained(this)));
+        base::Bind(&LoggingNativeHandler::Dcheck, base::Unretained(this)));
   }
 
   v8::Handle<v8::Value> Dcheck(const v8::Arguments& args) {
@@ -271,7 +349,6 @@ class LoggingNativeHandler : public NativeHandler {
       }
     }
     DCHECK(check_value) << error_message;
-    LOG(WARNING) << error_message;
     return v8::Undefined();
   }
 
@@ -303,20 +380,6 @@ void InstallWebstoreBindings(ModuleSystem* module_system,
                               "chromeHiddenWebstore");
 }
 
-static v8::Handle<v8::Object> GetOr…
(message too large)

[commit-bot: I haz the power, 2013-03-06 21:39:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/cduvall@chromium.org/11571014/154002

[commit-bot: I haz the power, 2013-03-07 00:09:45]

Retried try job too often on mac_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2013-03-07 01:20:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/cduvall@chromium.org/11571014/169001

[commit-bot: I haz the power, 2013-03-07 06:32:00]

Change committed as 186643

[tonyg, 2013-03-07 18:41:05]

On 2013/03/07 06:32:00, I haz the power (commit-bot) wrote:
> Change committed as 186643

This caused a 10% sunspider benchmark regression. Reverted in
https://code.google.com/p/chromium/issues/detail?id=180937

[commit-bot: I haz the power, 2013-03-09 04:33:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/cduvall@chromium.org/11571014/191001

[commit-bot: I haz the power, 2013-03-09 04:59:45]

Sorry for I got bad news for ya.
Compile failed with a clobber build on android_dbg.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-03-09 06:51:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/cduvall@chromium.org/11571014/206001

[commit-bot: I haz the power, 2013-03-09 14:26:27]

Change committed as 187143