Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
9315010:
RulesRegistry for declarative APIs. (Closed)
|
Created:
8 years, 10 months ago by battre Modified:
8 years, 10 months ago CC:
chromium-reviews, mihaip+watch_chromium.org, koz (OOO until 15th September), calamity Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionRulesRegistry for declarative APIs.
This CL implements a RuleRegistry mechanism so that adding/removing/getting rules can be dispatched to individual implementations of the RulesRegistry.
BUG=112155
TEST=no
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=121369
Patch Set 1 #
Total comments: 1
Patch Set 2 : Implemented ID and priority generation #
Total comments: 64
Patch Set 3 : Addressed comments #
Total comments: 1
Patch Set 4 : Addressed remaining comments #
Total comments: 17
Patch Set 5 : Merged with ToT for trybot testing #
Total comments: 33
Patch Set 6 : Addressed comments #
Total comments: 7
Patch Set 7 : Addressed comments #Patch Set 8 : Removed a CHECK #
Total comments: 1
Patch Set 9 : Replaced string literals with constants #Patch Set 10 : Fixed comiler errors #Messages
Total messages: 26 (0 generated)
Hi. Could you please review this? BTW: Do you both want to review the following development of this API? Here is a highlevel description: The declarative_api.cc delegates all function calls to the RulesRegistryService (I want to keep declarative_api.cc very lean to make it easier to unit test the Declarative API and use fewer browser tests than in the webrequest api). The RulesRegistryService knows several implementations of the RuleRegistry interface. One of these implementations will be the WebRequestRuleRegistry. Each implementation is assigned with a key that is derived from the event name on which you call e.g. addRules(). The RulesRegistryService does the dispatching (i.e. if you call chrome.webRequest.onNetworkEvent.addRule() this addRule() call is dispatched to the WebRequestRuleRegistry). The TestRulesRegistry is currently a simple storage container for rules. I am planning to generalize this to a base class for all RuleRegistries but wanted to limit the size of this CL. I am seeing several ways how this should be refactored (e.g. the WebRequestRuleRegistry needs to live on the IO thread) but I think I will delay these into upcoming CLs. Best regards, Dominic https://chromiumcodereview.appspot.com/9315010/diff/1/chrome/browser/extensio... File chrome/browser/extensions/api/declarative/rules_registry_service.h (right): https://chromiumcodereview.appspot.com/9315010/diff/1/chrome/browser/extensio... chrome/browser/extensions/api/declarative/rules_registry_service.h:33: : public ProfileKeyedService, content::NotificationObserver { Instead of making this a ProfileKeyedService we could also put it into ExtensionService. I don't know what is is considered the preferable way, given that you asked me recently not to increase the size of ExtensionService even more.
CC'ed kalman and koz
First go. Apologies again for the review epicness. http://codereview.chromium.org/9315010/diff/4002/chrome/browser/extensions/ap... File chrome/browser/extensions/api/declarative/declarative_api.cc (right): http://codereview.chromium.org/9315010/diff/4002/chrome/browser/extensions/ap... chrome/browser/extensions/api/declarative/declarative_api.cc:17: // called. This comment isn't entirely accurate, because the API functions can be called without the presence of an event. Also I think experimental.declarative.json is descriptive enough to not need it. http://codereview.chromium.org/9315010/diff/4002/chrome/browser/extensions/ap... chrome/browser/extensions/api/declarative/declarative_api.cc:19: EXTENSION_FUNCTION_VALIDATE(args_->GetString(0, &event_name)); Note: very soon you'll be able to use json_schema_compiler to avoid all of this boilerplate. Chris' (calamity@) generator is now generating the code needed for the Declarative API on his desktop; support for it should be checked in early next week. When it is, you'll need to list the JSON file in chrome/common/extensions/api/api.gyp to get all the code to be generated. Then use it; see permissions_api.cc as a guide, but basically namespace extensions { using namespace api::experimental_declarative; bool AddRulesFunction::RunImpl() { scoped_ptr<AddRules::Params> params(AddRules::Params::Create(*args_)); EXTENSION_FUNCTION_VALIDATE(params.get()); // You now have access to fields like: params->event; // std::string params->rules; // std::vector<linked_ptr<Rule> > // Where Rule is like: Rule* rule = params->rules[0]; rule->id; // scoped_ptr<std::string>, because it's optional rule->conditions; // std::vector<linked_ptr<DictionaryValue> > rule->actions; // std::vector<linked_ptr<DictionaryValue> > rule->priority; // scoped_ptr<int>, because it's optional ... } I actually wonder if it's worth holding off on this patch until json_schema_compiler can be used. It will be massively beneficial; not only would all of this VALIDATE/looping/GetFoo stuff be unnecessary, but you'd be able to pass a vector of Rules directly into AddRules rather than a DictionaryValue. That's a massive win. Oh, and you wouldn't need that extra file for constants. So you should definitely chat to calamity@ :) http://codereview.chromium.org/9315010/diff/4002/chrome/browser/extensions/ap... chrome/browser/extensions/api/declarative/declarative_api.cc:28: DictionaryValue* rule; = NULL etc http://codereview.chromium.org/9315010/diff/4002/chrome/browser/extensions/ap... chrome/browser/extensions/api/declarative/declarative_api.cc:37: rules_registry->AddRules( EXTENSION_FUNCTION_VALIDATE is for validating that the messages from the renderer are structured correctly, as in, the same validation that the schema validation is doing on the renderer. If it fails, the renderer is killed. From what I can tell, a false return value from AddRules is due to an extension making a bad call; it shouldn't result in it being killed. That seems harsh. http://codereview.chromium.org/9315010/diff/4002/chrome/browser/extensions/ap... File chrome/browser/extensions/api/declarative/declarative_api_constants.cc (right): http://codereview.chromium.org/9315010/diff/4002/chrome/browser/extensions/ap... chrome/browser/extensions/api/declarative/declarative_api_constants.cc:7: namespace extension_declarative_api_constants { namespace extensions { namespace declarative_api_constants { ... http://codereview.chromium.org/9315010/diff/4002/chrome/browser/extensions/ap... File chrome/browser/extensions/api/declarative/rule_identifier.h (right): http://codereview.chromium.org/9315010/diff/4002/chrome/browser/extensions/ap... chrome/browser/extensions/api/declarative/rule_identifier.h:26: bool operator<(const RuleIdentifier& other) const; why do you need to overload < ? http://codereview.chromium.org/9315010/diff/4002/chrome/browser/extensions/ap... chrome/browser/extensions/api/declarative/rule_identifier.h:27: RuleIdentifier& operator=(const RuleIdentifier& other); i don't think you need to override this. http://codereview.chromium.org/9315010/diff/4002/chrome/browser/extensions/ap... File chrome/browser/extensions/api/declarative/rules_registry.h (right): http://codereview.chromium.org/9315010/diff/4002/chrome/browser/extensions/ap... chrome/browser/extensions/api/declarative/rules_registry.h:41: SetErrorCallback set_error_callback) = 0; I don't understand why errors communicated via a callback in this way. Why not pass a std::string*? Having it as a callback implies some kind of asynchronous API, which wouldn't work the way this API is being used (the API handling implementation in declarative_api.cc is synchronous). However, even simpler, I think you can return std::string from AddRules such that an empty return value implies success. Ditto RemoveRules. http://codereview.chromium.org/9315010/diff/4002/chrome/browser/extensions/ap... File chrome/browser/extensions/api/declarative/rules_registry_service.cc (right): http://codereview.chromium.org/9315010/diff/4002/chrome/browser/extensions/ap... chrome/browser/extensions/api/declarative/rules_registry_service.cc:34: rule_registries_.begin(), rule_registries_.end()); if you make RulesRegistryMap contain linked_ptrs, this wouldn't be necessary. i.e. std::map<std::string, linked_ptr<RulesRegistry> > http://codereview.chromium.org/9315010/diff/4002/chrome/browser/extensions/ap... chrome/browser/extensions/api/declarative/rules_registry_service.cc:42: // RegisterRuleRegistry("chrome.net", new FoobarRulesRegistry()); Yeah, if you make this owned by a Profile or ExtensionService, then make that responsible for registering the appropriate RulesRegistries. http://codereview.chromium.org/9315010/diff/4002/chrome/browser/extensions/ap... File chrome/browser/extensions/api/declarative/rules_registry_service.h (right): http://codereview.chromium.org/9315010/diff/4002/chrome/browser/extensions/ap... chrome/browser/extensions/api/declarative/rules_registry_service.h:36: // Use the RulesRegistryServiceFactory to retrieve an instance of this class. Why the need for RulesRegistryServiceFactory? Can this just directly belong to a Profile (or an ExtensionService, which is basically a dumping ground for extensions things that are tied to the lifetime of a Profile). Reading the documentation about ProfileFactoryServiceBlahStuff, it seems to be for Profile-lifetime objets that have initialisation interdependencies and therefore complicated initialisation procedures, so hopefully unnecessary here. http://codereview.chromium.org/9315010/diff/4002/chrome/browser/extensions/ap... chrome/browser/extensions/api/declarative/rules_registry_service.h:47: void RegisterRuleRegistry( s/Rule/Rules/ http://codereview.chromium.org/9315010/diff/4002/chrome/browser/extensions/ap... chrome/browser/extensions/api/declarative/rules_registry_service.h:53: // on which the function was called. Is it possible to move much of the logic in this class to a decorator-type RulesRegistry? And have RulesRegistryService be a wrapper around an event_name -> RulesRegistry map? Something like == rules_registry == interface RulesRegistry { // you already have this } == initializing_rules_registry == // New class which has the job of filling in missing fields during AddRules, then // delegating that call to another RulesRegistry. class InitializingRulesRulesRegistry : RulesRegistry { public: InitializingRulesRegistry(scoped_ptr<RulesRegistry> delegate) : delegate_(delegate) {} // RulesRegistry implementation. std::string AddRules(...) { CheckAndFillInOptionalRules(...); return delegate_->AddRules(...); } std::string RemoveRules(...) { return delegte_->RemoveRules(...); } private: scoped_ptr<RulesRegistry> delegate_; } == rules_registry_service == class RulesRegistryService { .. void RegisterRulesRegistry(name, delegate_registry) { rule_registries_map_[name] = new InitializingRulesRegistry(delegate_registry); } } == etc with other implementations == http://codereview.chromium.org/9315010/diff/4002/chrome/browser/extensions/ap... File chrome/browser/extensions/api/declarative/test_rules_registry.cc (right): http://codereview.chromium.org/9315010/diff/4002/chrome/browser/extensions/ap... chrome/browser/extensions/api/declarative/test_rules_registry.cc:44: for (i = rules.begin(); i != rules.end(); ++i) { I prefer to always define iterators inline, and wrap however necessary to make that work. http://codereview.chromium.org/9315010/diff/4002/chrome/browser/extensions/ap... chrome/browser/extensions/api/declarative/test_rules_registry.cc:47: CHECK(rules_.find(rule_id) == rules_.end()); Perhaps CHECK(rules_.count(rule_id)) is more concise. http://codereview.chromium.org/9315010/diff/4002/chrome/common/extensions/api... File chrome/common/extensions/api/experimental.declarative.json (right): http://codereview.chromium.org/9315010/diff/4002/chrome/common/extensions/api... chrome/common/extensions/api/experimental.declarative.json:31: } I don't think we can have these here as part of the API. Having nodoc=true hides it from documentation*, but it doesn't hide it from extensions themselves, where the namespace will still be polluted with these types and testEvent. Any reason why you can't use the webRequest stuff for tests? * (though it's a bit redundant here since the whole API is nodoc) http://codereview.chromium.org/9315010/diff/4002/chrome/test/data/extensions/... File chrome/test/data/extensions/api_test/declarative/api/background.js (right): http://codereview.chromium.org/9315010/diff/4002/chrome/test/data/extensions/... chrome/test/data/extensions/api_test/declarative/api/background.js:74: chrome.test.assertEq(outputRule1, rules[1]); perhaps more concise as chrome.test.assertEq([outputRule0, outputRule1], rules); (since you also wouldn't need the rules.length == 2 check) also, I've seen other tests define assertTrue/assertEq at the top rather than typing "chrome.test" everywhere. http://codereview.chromium.org/9315010/diff/4002/chrome/test/data/extensions/... chrome/test/data/extensions/api_test/declarative/api/background.js:91: declarative.testEvent.getRules([], callback); Why is [] get all rules, I would have though this would return no rules? Other APIs have the convention of null/undefined implying "everything"; I suggest you follow that. http://codereview.chromium.org/9315010/diff/4002/chrome/test/data/extensions/... chrome/test/data/extensions/api_test/declarative/api/background.js:135: declarative.testEvent.removeRules([], callback); See comment above about getRules. http://codereview.chromium.org/9315010/diff/4002/chrome/test/data/extensions/... chrome/test/data/extensions/api_test/declarative/api/background.js:152: declarative.testEvent.addRules([inputRule0], callback); Another important test is that the condition/action validation works. As in, it rejects calls using the wrong actions etc. In fact, the only real reason we need these API tests is so that the renderer logic is tested; and the renderer logic basically consists of that validation (plus the chrome.Event thin wrapping around the API itself). Everything else can hopefully be done with unit tests, or in the worst case, be done with an "API bindings test" (or whatever aa has been calling it), which just tests the browser-side bindings (see See http://code.google.com/codesearch#OAMlx_jo-ck/src/chrome/browser/extensions/e...). http://codereview.chromium.org/9315010/diff/4002/chrome/test/data/extensions/... File chrome/test/data/extensions/api_test/declarative/api/manifest.json (right): http://codereview.chromium.org/9315010/diff/4002/chrome/test/data/extensions/... chrome/test/data/extensions/api_test/declarative/api/manifest.json:11: "declarative" this won't do anything unless you've updated extension_permission_set to know about the declarative permission -- but no need to do that until it's out of experimental.
https://chromiumcodereview.appspot.com/9315010/diff/4002/chrome/browser/exten... File chrome/browser/extensions/api/declarative/declarative_api.cc (right): https://chromiumcodereview.appspot.com/9315010/diff/4002/chrome/browser/exten... chrome/browser/extensions/api/declarative/declarative_api.cc:84: for (ListValue::iterator i = rule_identifiers_list->begin(); If you don't end up waiting on calamity@, you could pull this out into a helper for now. https://chromiumcodereview.appspot.com/9315010/diff/4002/chrome/browser/exten... File chrome/browser/extensions/api/declarative/declarative_api_constants.cc (right): https://chromiumcodereview.appspot.com/9315010/diff/4002/chrome/browser/exten... chrome/browser/extensions/api/declarative/declarative_api_constants.cc:7: namespace extension_declarative_api_constants { On 2012/02/02 11:59:16, kalman wrote: > namespace extensions { > namespace declarative_api_constants { > > ... We don't need these separate constants files anymore (unless you need them for some specific reason?). The new way is to put the constants in an anonymous namespace in the cc file of the API implementation. https://chromiumcodereview.appspot.com/9315010/diff/4002/chrome/browser/exten... File chrome/browser/extensions/api/declarative/rule_identifier.cc (right): https://chromiumcodereview.appspot.com/9315010/diff/4002/chrome/browser/exten... chrome/browser/extensions/api/declarative/rule_identifier.cc:9: RuleIdentifier::RuleIdentifier(const std::string& extension_id, unit test? https://chromiumcodereview.appspot.com/9315010/diff/4002/chrome/browser/exten... File chrome/browser/extensions/api/declarative/rule_identifier.h (right): https://chromiumcodereview.appspot.com/9315010/diff/4002/chrome/browser/exten... chrome/browser/extensions/api/declarative/rule_identifier.h:26: bool operator<(const RuleIdentifier& other) const; On 2012/02/02 11:59:16, kalman wrote: > why do you need to overload < ? It is so that RuleIdentifier can be used with std::set. https://chromiumcodereview.appspot.com/9315010/diff/4002/chrome/browser/exten... chrome/browser/extensions/api/declarative/rule_identifier.h:27: RuleIdentifier& operator=(const RuleIdentifier& other); On 2012/02/02 11:59:16, kalman wrote: > i don't think you need to override this. Not sure about this one. Maybe some other STL container. https://chromiumcodereview.appspot.com/9315010/diff/4002/chrome/browser/exten... File chrome/browser/extensions/api/declarative/rules_registry_service.cc (right): https://chromiumcodereview.appspot.com/9315010/diff/4002/chrome/browser/exten... chrome/browser/extensions/api/declarative/rules_registry_service.cc:52: bool RulesRegistryService::IsUniqueId(const RuleIdentifier& id) const { nit: Prefer "ID" (capitalized). https://chromiumcodereview.appspot.com/9315010/diff/4002/chrome/browser/exten... File chrome/browser/extensions/api/declarative/rules_registry_service.h (right): https://chromiumcodereview.appspot.com/9315010/diff/4002/chrome/browser/exten... chrome/browser/extensions/api/declarative/rules_registry_service.h:53: // on which the function was called. On 2012/02/02 11:59:16, kalman wrote: > Is it possible to move much of the logic in this class to a decorator-type > RulesRegistry? And have RulesRegistryService be a wrapper around an event_name > -> RulesRegistry map? Feels like overdesign to use a decorator to add logic to one method (AddRules). Also there is only ever one decorator here (the one that does the rest of the parsing of AddRules) - it's not like it's a dynamic system where you many different combinations of decorations might apply at runtime. Could you just as easily move the rest of the parsing code out into declarative_api.cc and pass something fully-parsed into RulesRegistryService::AddRules()? Please remember that the higher-level patterns also have a cost! Everyone reading the code must parse the system into a structure in their head before they can reason about it. https://chromiumcodereview.appspot.com/9315010/diff/4002/chrome/common/extens... File chrome/common/extensions/api/experimental.declarative.json (right): https://chromiumcodereview.appspot.com/9315010/diff/4002/chrome/common/extens... chrome/common/extensions/api/experimental.declarative.json:31: } On 2012/02/02 11:59:16, kalman wrote: > I don't think we can have these here as part of the API. Having nodoc=true hides > it from documentation*, but it doesn't hide it from extensions themselves, where > the namespace will still be polluted with these types and testEvent. > > Any reason why you can't use the webRequest stuff for tests? > > * (though it's a bit redundant here since the whole API is nodoc) It's a bigger problem that. The whole declarative API will also be exposed, uselessly. I guess there are two paths here: 1) Hack JSON schema and SchemaGeneratedBindings some more to prevent these names from being exposed to extensions (but they still need to be exposed to other bindings files). 2) (Sorry if this playing with dead snakes a bit) Bite the bullet and do this with normal IPC. This will properly decouple the renderer and browser side of extension bindings, the way it should have always been done. Revisiting the previous thread about this (from https://chromiumcodereview.appspot.com/9315010), Dominic's reasons for not using real IPC were: a. validation of arguments b. asynchronous messaging with callbacks (addRules needs to "return" something to the callback function) c. association of messages to their extensions? d. possibly some security features? a) is not a valid argument. We will still do param validation because we will still use JSON schema for the frontend of the API. It's just that instead of the bindings calling the sendRequest() infrastructure, it will end up calling some other native function, which will call a custom IPC. The native function does not need to be careful with validation because it can assume the JSON schema already did that. b), c), and d) are somewhat legit, but I believe that code can be easily refactored out of EFD. I'm running out the door, but please see the recent thread on chrome-extensions-team w/ subject "A proposed IDL syntax" for more context. To be honest, I am not sure how much work there is in this approach. It needs a little investigation. That is why originally, I backed off proposing this because I didn't want to randomize Dominic too much. However, as we add more and more hacks to SGB, I'm continually thinking that we're pushing it in a bad direction. If it were me doing it, I would investigate the IPC route.
https://chromiumcodereview.appspot.com/9315010/diff/4002/chrome/browser/exten... File chrome/browser/extensions/api/declarative/rules_registry_service.h (right): https://chromiumcodereview.appspot.com/9315010/diff/4002/chrome/browser/exten... chrome/browser/extensions/api/declarative/rules_registry_service.h:53: // on which the function was called. On 2012/02/02 16:28:51, Aaron Boodman wrote: > Could you just as easily move the rest of the parsing code out into > declarative_api.cc and pass something fully-parsed into > RulesRegistryService::AddRules()? Sorry, I meant to remove this paragraph. I think the code is fine just as it is.
On 2012/02/02 16:28:50, Aaron Boodman wrote: > To be honest, I am not sure how much work there is in this approach. It needs a > little investigation. That is why originally, I backed off proposing this > because I didn't want to randomize Dominic too much. However, as we add more and > more hacks to SGB, I'm continually thinking that we're pushing it in a bad > direction. If it were me doing it, I would investigate the IPC route. Also, to be fair, I know this isn't the first time this pattern has been used. Several other APIs have 'nodoc' internal APIs that they call through to, which are exposed to script. This is another reason I let this change originally go through. It is just following an existing bad pattern that we eventually want to change.
Thanks for the extensive review. Best, Dominic http://codereview.chromium.org/9315010/diff/4002/chrome/browser/extensions/ap... File chrome/browser/extensions/api/declarative/declarative_api.cc (right): http://codereview.chromium.org/9315010/diff/4002/chrome/browser/extensions/ap... chrome/browser/extensions/api/declarative/declarative_api.cc:17: // called. On 2012/02/02 11:59:16, kalman wrote: > This comment isn't entirely accurate, because the API functions can be called > without the presence of an event. > > Also I think experimental.declarative.json is descriptive enough to not need it. Done. http://codereview.chromium.org/9315010/diff/4002/chrome/browser/extensions/ap... chrome/browser/extensions/api/declarative/declarative_api.cc:19: EXTENSION_FUNCTION_VALIDATE(args_->GetString(0, &event_name)); On 2012/02/02 11:59:16, kalman wrote: > Note: very soon you'll be able to use json_schema_compiler to avoid all of this > boilerplate. Chris' (calamity@) generator is now generating the code needed for > the Declarative API on his desktop; support for it should be checked in early > next week. > > When it is, you'll need to list the JSON file in > chrome/common/extensions/api/api.gyp to get all the code to be generated. Then > use it; see permissions_api.cc as a guide, but basically > > namespace extensions { > > using namespace api::experimental_declarative; > > bool AddRulesFunction::RunImpl() { > scoped_ptr<AddRules::Params> params(AddRules::Params::Create(*args_)); > EXTENSION_FUNCTION_VALIDATE(params.get()); > > // You now have access to fields like: > params->event; // std::string > params->rules; // std::vector<linked_ptr<Rule> > > > // Where Rule is like: > Rule* rule = params->rules[0]; > rule->id; // scoped_ptr<std::string>, because it's optional > rule->conditions; // std::vector<linked_ptr<DictionaryValue> > > rule->actions; // std::vector<linked_ptr<DictionaryValue> > > rule->priority; // scoped_ptr<int>, because it's optional > > ... > } > > > I actually wonder if it's worth holding off on this patch until > json_schema_compiler can be used. It will be massively beneficial; not only > would all of this VALIDATE/looping/GetFoo stuff be unnecessary, but you'd be > able to pass a vector of Rules directly into AddRules rather than a > DictionaryValue. That's a massive win. Oh, and you wouldn't need that extra > file for constants. > > So you should definitely chat to calamity@ :) sounds awesome!!! http://codereview.chromium.org/9315010/diff/4002/chrome/browser/extensions/ap... chrome/browser/extensions/api/declarative/declarative_api.cc:28: DictionaryValue* rule; On 2012/02/02 11:59:16, kalman wrote: > = NULL > > etc Done. http://codereview.chromium.org/9315010/diff/4002/chrome/browser/extensions/ap... chrome/browser/extensions/api/declarative/declarative_api.cc:37: rules_registry->AddRules( On 2012/02/02 11:59:16, kalman wrote: > EXTENSION_FUNCTION_VALIDATE is for validating that the messages from the > renderer are structured correctly, as in, the same validation that the schema > validation is doing on the renderer. If it fails, the renderer is killed. > > From what I can tell, a false return value from AddRules is due to an extension > making a bad call; it shouldn't result in it being killed. That seems harsh. Valid points. I have promoted this to graceful failures. Done. http://codereview.chromium.org/9315010/diff/4002/chrome/browser/extensions/ap... File chrome/browser/extensions/api/declarative/declarative_api_constants.cc (right): http://codereview.chromium.org/9315010/diff/4002/chrome/browser/extensions/ap... chrome/browser/extensions/api/declarative/declarative_api_constants.cc:7: namespace extension_declarative_api_constants { On 2012/02/02 11:59:16, kalman wrote: > namespace extensions { > namespace declarative_api_constants { > > ... Done. http://codereview.chromium.org/9315010/diff/4002/chrome/browser/extensions/ap... File chrome/browser/extensions/api/declarative/rule_identifier.h (right): http://codereview.chromium.org/9315010/diff/4002/chrome/browser/extensions/ap... chrome/browser/extensions/api/declarative/rule_identifier.h:26: bool operator<(const RuleIdentifier& other) const; On 2012/02/02 11:59:16, kalman wrote: > why do you need to overload < ? To store these elements in a std::set. http://codereview.chromium.org/9315010/diff/4002/chrome/browser/extensions/ap... chrome/browser/extensions/api/declarative/rule_identifier.h:27: RuleIdentifier& operator=(const RuleIdentifier& other); On 2012/02/02 11:59:16, kalman wrote: > i don't think you need to override this. I think the style guide is not exactly clear, but I read http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Copy_C... as "we want both an no implicit assignment operator" http://codereview.chromium.org/9315010/diff/4002/chrome/browser/extensions/ap... File chrome/browser/extensions/api/declarative/rules_registry.h (right): http://codereview.chromium.org/9315010/diff/4002/chrome/browser/extensions/ap... chrome/browser/extensions/api/declarative/rules_registry.h:41: SetErrorCallback set_error_callback) = 0; On 2012/02/02 11:59:16, kalman wrote: > I don't understand why errors communicated via a callback in this way. Why not > pass a std::string*? Having it as a callback implies some kind of asynchronous > API, which wouldn't work the way this API is being used (the API handling > implementation in declarative_api.cc is synchronous). > > However, even simpler, I think you can return std::string from AddRules such > that an empty return value implies success. > > Ditto RemoveRules. Done. http://codereview.chromium.org/9315010/diff/4002/chrome/browser/extensions/ap... File chrome/browser/extensions/api/declarative/rules_registry_service.cc (right): http://codereview.chromium.org/9315010/diff/4002/chrome/browser/extensions/ap... chrome/browser/extensions/api/declarative/rules_registry_service.cc:34: rule_registries_.begin(), rule_registries_.end()); On 2012/02/02 11:59:16, kalman wrote: > if you make RulesRegistryMap contain linked_ptrs, this wouldn't be necessary. > > i.e. std::map<std::string, linked_ptr<RulesRegistry> > Done. http://codereview.chromium.org/9315010/diff/4002/chrome/browser/extensions/ap... chrome/browser/extensions/api/declarative/rules_registry_service.cc:52: bool RulesRegistryService::IsUniqueId(const RuleIdentifier& id) const { On 2012/02/02 16:28:51, Aaron Boodman wrote: > nit: Prefer "ID" (capitalized). I did a check of grep -r "[ ,.>]ID[ ()]" chrome vs grep -r "[ ,.>]id[ ()]" chrome and the latter won by far. ID seems to occur only in comments. http://codereview.chromium.org/9315010/diff/4002/chrome/browser/extensions/ap... File chrome/browser/extensions/api/declarative/rules_registry_service.h (right): http://codereview.chromium.org/9315010/diff/4002/chrome/browser/extensions/ap... chrome/browser/extensions/api/declarative/rules_registry_service.h:36: // Use the RulesRegistryServiceFactory to retrieve an instance of this class. On 2012/02/02 11:59:16, kalman wrote: > Why the need for RulesRegistryServiceFactory? Can this just directly belong to > a Profile (or an ExtensionService, which is basically a dumping ground for > extensions things that are tied to the lifetime of a Profile). > > Reading the documentation about ProfileFactoryServiceBlahStuff, it seems to be > for Profile-lifetime objets that have initialisation interdependencies and > therefore complicated initialisation procedures, so hopefully unnecessary here. I raised this concern in a previous Patch Set version as well. I had the previous request not to let ExtensionService grow but I think in this case it is still the cleaner solution. I will get rid of the Factory. Done. http://codereview.chromium.org/9315010/diff/4002/chrome/browser/extensions/ap... chrome/browser/extensions/api/declarative/rules_registry_service.h:47: void RegisterRuleRegistry( On 2012/02/02 11:59:16, kalman wrote: > s/Rule/Rules/ Done. http://codereview.chromium.org/9315010/diff/4002/chrome/browser/extensions/ap... chrome/browser/extensions/api/declarative/rules_registry_service.h:53: // on which the function was called. On 2012/02/02 16:28:51, Aaron Boodman wrote: > On 2012/02/02 11:59:16, kalman wrote: > > Is it possible to move much of the logic in this class to a decorator-type > > RulesRegistry? And have RulesRegistryService be a wrapper around an > event_name > > -> RulesRegistry map? > > Feels like overdesign to use a decorator to add logic to one method (AddRules). > Also there is only ever one decorator here (the one that does the rest of the > parsing of AddRules) - it's not like it's a dynamic system where you many > different combinations of decorations might apply at runtime. > > Could you just as easily move the rest of the parsing code out into > declarative_api.cc and pass something fully-parsed into > RulesRegistryService::AddRules()? > > Please remember that the higher-level patterns also have a cost! Everyone > reading the code must parse the system into a structure in their head before > they can reason about it. At first I considered this overdesign as well. But then I recognized that is also makes the RuleIdentifier slightly cleaner because we have different scopes for each event name. I think I like this design. http://codereview.chromium.org/9315010/diff/4002/chrome/browser/extensions/ap... File chrome/browser/extensions/api/declarative/test_rules_registry.cc (right): http://codereview.chromium.org/9315010/diff/4002/chrome/browser/extensions/ap... chrome/browser/extensions/api/declarative/test_rules_registry.cc:44: for (i = rules.begin(); i != rules.end(); ++i) { On 2012/02/02 11:59:16, kalman wrote: > I prefer to always define iterators inline, and wrap however necessary to make > that work. Do you feel strongly about this? I dislike the for statements that stretch over 3 lines. http://codereview.chromium.org/9315010/diff/4002/chrome/browser/extensions/ap... chrome/browser/extensions/api/declarative/test_rules_registry.cc:47: CHECK(rules_.find(rule_id) == rules_.end()); On 2012/02/02 11:59:16, kalman wrote: > Perhaps CHECK(rules_.count(rule_id)) is more concise. But it would be wrong ;-) CHECK(!rules_.count(rule_id)) or CHECK(rules_.count(rule_id) == 0) would be correct. I like the .find() == .end() because it is pretty literal and hard to get wrong. http://codereview.chromium.org/9315010/diff/4002/chrome/common/extensions/api... File chrome/common/extensions/api/experimental.declarative.json (right): http://codereview.chromium.org/9315010/diff/4002/chrome/common/extensions/api... chrome/common/extensions/api/experimental.declarative.json:31: } On 2012/02/02 11:59:16, kalman wrote: > I don't think we can have these here as part of the API. Having nodoc=true hides > it from documentation*, but it doesn't hide it from extensions themselves, where > the namespace will still be polluted with these types and testEvent. > > Any reason why you can't use the webRequest stuff for tests? > > * (though it's a bit redundant here since the whole API is nodoc) I am considering removing the nodoc from the whole API once we have we have reached a somewhat stable state. The reason for having these TestConditions was to have clean testing. I don't think that having these objects here is a major problem. They are invisible and nobody could do anything with them. There won't be a RuleRegistry bound to "declarative.testEvent", so all all calls bounce. http://codereview.chromium.org/9315010/diff/4002/chrome/common/extensions/api... chrome/common/extensions/api/experimental.declarative.json:31: } On 2012/02/02 16:28:51, Aaron Boodman wrote: > On 2012/02/02 11:59:16, kalman wrote: > > I don't think we can have these here as part of the API. Having nodoc=true > hides > > it from documentation*, but it doesn't hide it from extensions themselves, > where > > the namespace will still be polluted with these types and testEvent. > > > > Any reason why you can't use the webRequest stuff for tests? > > > > * (though it's a bit redundant here since the whole API is nodoc) > > It's a bigger problem that. The whole declarative API will also be exposed, > uselessly. > > I guess there are two paths here: > > 1) Hack JSON schema and SchemaGeneratedBindings some more to prevent these names > from being exposed to extensions (but they still need to be exposed to other > bindings files). > > 2) (Sorry if this playing with dead snakes a bit) Bite the bullet and do this > with normal IPC. This will properly decouple the renderer and browser side of > extension bindings, the way it should have always been done. > > Revisiting the previous thread about this (from > https://chromiumcodereview.appspot.com/9315010), Dominic's reasons for not using > real IPC were: > > a. validation of arguments > b. asynchronous messaging with callbacks (addRules needs to "return" something > to the callback function) > c. association of messages to their extensions? > d. possibly some security features? > > a) is not a valid argument. We will still do param validation because we will > still use JSON schema for the frontend of the API. It's just that instead of the > bindings calling the sendRequest() infrastructure, it will end up calling some > other native function, which will call a custom IPC. The native function does > not need to be careful with validation because it can assume the JSON schema > already did that. > > b), c), and d) are somewhat legit, but I believe that code can be easily > refactored out of EFD. > > I'm running out the door, but please see the recent thread on > chrome-extensions-team w/ subject "A proposed IDL syntax" for more context. > > To be honest, I am not sure how much work there is in this approach. It needs a > little investigation. That is why originally, I backed off proposing this > because I didn't want to randomize Dominic too much. However, as we add more and > more hacks to SGB, I'm continually thinking that we're pushing it in a bad > direction. If it were me doing it, I would investigate the IPC route. I am still in favor of this approach. I don't see any advantage if we use the same JSON schema. Did you see the new CL after incorporating Ben's suggestions of using CustomBindings? It became really neat! See http://codereview.chromium.org/9192029/ About the effort of doing something ourselves: - We need a to bind the native functions in a V8Context - We need to introduce new IPC messages - We need to implement the association of requests and responses - We need to implement the association of messages to individual renderers - We need to implement security for permissions strings - We need to implement error handling This looks like a lot of unnecessary effort that is basically code duplication. Can you tell me what advantages you see in approach 2? http://codereview.chromium.org/9315010/diff/4002/chrome/test/data/extensions/... File chrome/test/data/extensions/api_test/declarative/api/background.js (right): http://codereview.chromium.org/9315010/diff/4002/chrome/test/data/extensions/... chrome/test/data/extensions/api_test/declarative/api/background.js:74: chrome.test.assertEq(outputRule1, rules[1]); On 2012/02/02 11:59:16, kalman wrote: > perhaps more concise as > > chrome.test.assertEq([outputRule0, outputRule1], rules); > > (since you also wouldn't need the rules.length == 2 check) I have changed this to chrome.test.assertEq([outputRule0, outputRule1], rules); I have not removed the rules.length == 2 here because I need to check that rules.length>0 for the "id" in rules[0] test. I'd like to keep that on top because it generates more sensible error messages that way in case something breaks. I have removed the rules.length test in other locations, though. > also, I've seen other tests define assertTrue/assertEq at the top rather than > typing "chrome.test" everywhere. http://codereview.chromium.org/9315010/diff/4002/chrome/test/data/extensions/... chrome/test/data/extensions/api_test/declarative/api/background.js:91: declarative.testEvent.getRules([], callback); On 2012/02/02 11:59:16, kalman wrote: > Why is [] get all rules, I would have though this would return no rules? > > Other APIs have the convention of null/undefined implying "everything"; I > suggest you follow that. This does not work with SchemaGeneratedBindings as of now. If I change the schema so that the both parameter of getRules are optional and pass "null, callback" or "undefined, callback", the binding layer creates an empty list in both cases. If we want to change this behavior, I suggest that we do in a separate CL. It might break a lot. http://codereview.chromium.org/9315010/diff/4002/chrome/test/data/extensions/... chrome/test/data/extensions/api_test/declarative/api/background.js:135: declarative.testEvent.removeRules([], callback); On 2012/02/02 11:59:16, kalman wrote: > See comment above about getRules. see above http://codereview.chromium.org/9315010/diff/4002/chrome/test/data/extensions/... chrome/test/data/extensions/api_test/declarative/api/background.js:152: declarative.testEvent.addRules([inputRule0], callback); On 2012/02/02 11:59:16, kalman wrote: > Another important test is that the condition/action validation works. As in, it > rejects calls using the wrong actions etc. > > In fact, the only real reason we need these API tests is so that the renderer > logic is tested; and the renderer logic basically consists of that validation > (plus the chrome.Event thin wrapping around the API itself). > > Everything else can hopefully be done with unit tests, or in the worst case, be > done with an "API bindings test" (or whatever aa has been calling it), which > just tests the browser-side bindings (see See > http://code.google.com/codesearch#OAMlx_jo-ck/src/chrome/browser/extensions/e...). Done.
Just responding to comments from last review. I'll have look at the new state of the change on Monday. https://chromiumcodereview.appspot.com/9315010/diff/4002/chrome/browser/exten... File chrome/browser/extensions/api/declarative/rule_identifier.h (right): https://chromiumcodereview.appspot.com/9315010/diff/4002/chrome/browser/exten... chrome/browser/extensions/api/declarative/rule_identifier.h:26: bool operator<(const RuleIdentifier& other) const; On 2012/02/02 19:12:36, battre wrote: > On 2012/02/02 11:59:16, kalman wrote: > > why do you need to overload < ? > > To store these elements in a std::set. I see. I'm idly wondering whether it's possible to use strings as IDs rather than these structs; like std::string id = extention_id + "/" + event_name + "/" + rule_id; but just idly, I haven't looked at the uses of this in much detail. https://chromiumcodereview.appspot.com/9315010/diff/4002/chrome/browser/exten... File chrome/browser/extensions/api/declarative/test_rules_registry.cc (right): https://chromiumcodereview.appspot.com/9315010/diff/4002/chrome/browser/exten... chrome/browser/extensions/api/declarative/test_rules_registry.cc:44: for (i = rules.begin(); i != rules.end(); ++i) { On 2012/02/02 19:12:36, battre wrote: > On 2012/02/02 11:59:16, kalman wrote: > > I prefer to always define iterators inline, and wrap however necessary to make > > that work. > > Do you feel strongly about this? I dislike the for statements that stretch over > 3 lines. I generally don't feel strongly enough about style things to worry about talking about it. However, I dislike situations where you have multiple iterators in the one function, e.g. foo() { iterator i; for (i = bar.begin(); i < bar.end(); i++) ... iterator j; for (j = bar.begin(); j < bar.end(); j++) ... } I've had those situations before. Basically, I'm just in the habit now of always limiting iterator scope to the for loops regardless of... anything. By the way, I don't think it would stretch over 3 lines: for (std::vector<DictionaryValue*>::const_iterator i = rules.begin(); i < rules.end(); ++i) { ... } } https://chromiumcodereview.appspot.com/9315010/diff/4002/chrome/common/extens... File chrome/common/extensions/api/experimental.declarative.json (right): https://chromiumcodereview.appspot.com/9315010/diff/4002/chrome/common/extens... chrome/common/extensions/api/experimental.declarative.json:31: } On 2012/02/02 19:12:36, battre wrote: > On 2012/02/02 16:28:51, Aaron Boodman wrote: > > On 2012/02/02 11:59:16, kalman wrote: > > > I don't think we can have these here as part of the API. Having nodoc=true > > hides > > > it from documentation*, but it doesn't hide it from extensions themselves, > > where > > > the namespace will still be polluted with these types and testEvent. > > > > > > Any reason why you can't use the webRequest stuff for tests? > > > > > > * (though it's a bit redundant here since the whole API is nodoc) > > > > It's a bigger problem that. The whole declarative API will also be exposed, > > uselessly. > > > > I guess there are two paths here: > > > > 1) Hack JSON schema and SchemaGeneratedBindings some more to prevent these > names > > from being exposed to extensions (but they still need to be exposed to other > > bindings files). > > > > 2) (Sorry if this playing with dead snakes a bit) Bite the bullet and do this > > with normal IPC. This will properly decouple the renderer and browser side of > > extension bindings, the way it should have always been done. > > > > Revisiting the previous thread about this (from > > https://chromiumcodereview.appspot.com/9315010), Dominic's reasons for not > using > > real IPC were: > > > > a. validation of arguments > > b. asynchronous messaging with callbacks (addRules needs to "return" something > > to the callback function) > > c. association of messages to their extensions? > > d. possibly some security features? > > > > a) is not a valid argument. We will still do param validation because we will > > still use JSON schema for the frontend of the API. It's just that instead of > the > > bindings calling the sendRequest() infrastructure, it will end up calling some > > other native function, which will call a custom IPC. The native function does > > not need to be careful with validation because it can assume the JSON schema > > already did that. > > > > b), c), and d) are somewhat legit, but I believe that code can be easily > > refactored out of EFD. > > > > I'm running out the door, but please see the recent thread on > > chrome-extensions-team w/ subject "A proposed IDL syntax" for more context. > > > > To be honest, I am not sure how much work there is in this approach. It needs > a > > little investigation. That is why originally, I backed off proposing this > > because I didn't want to randomize Dominic too much. However, as we add more > and > > more hacks to SGB, I'm continually thinking that we're pushing it in a bad > > direction. If it were me doing it, I would investigate the IPC route. > > I am still in favor of this approach. I don't see any advantage if we use the > same JSON schema. Did you see the new CL after incorporating Ben's suggestions > of using CustomBindings? It became really neat! See > http://codereview.chromium.org/9192029/ > > About the effort of doing something ourselves: > - We need a to bind the native functions in a V8Context > - We need to introduce new IPC messages > - We need to implement the association of requests and responses > - We need to implement the association of messages to individual renderers > - We need to implement security for permissions strings > - We need to implement error handling > > This looks like a lot of unnecessary effort that is basically code duplication. > > Can you tell me what advantages you see in approach 2? Weighing in here - I still don't think that these Test definitions belong here, or anywhere really. Like I said, the integration-style tests should use real data like the WebRequest bindings. Any definitions of fake data should be isolated to unit tests. If there's a problem injecting fake data with tests, hopefully we can work to factor things a bit differently for that to not be an issue, rather than exposing test infrastructure to the world. As for the more general issue of whether this API should be using the SGB infrastructure at all (as opposed to defining a schema elsewhere(?) and validating that way(?) -- the ?s are because I'm not sure what you mean by "using JSON schema at the frontend of the API")... the path of least resistance is definitely to use it. To stop "private" APIs from being exposed to normal extensions though, we could do something like (a) add a "private" property to APIs which tells bindings generators not to generate into the "chrome" object but into the "chromeHidden.privateAPIs" object or something, which only our own code would be able to access (since it's on chromeHidden). Like a more formal nodoc property. (b) something similar, but without using annotation magic, i.e. by bringing the separation up to a higher level (separate ExtensionAPI classes for example). But this sounds like more work. At a philosophical level it's fine, IMO. Oh, btw, Dominic -- why remove nodoc=true from the API? It's an API who's idiomatic use is through Event objects. Seems nicer to continue to hide the documentation. https://chromiumcodereview.appspot.com/9315010/diff/4002/chrome/test/data/ext... File chrome/test/data/extensions/api_test/declarative/api/background.js (right): https://chromiumcodereview.appspot.com/9315010/diff/4002/chrome/test/data/ext... chrome/test/data/extensions/api_test/declarative/api/background.js:74: chrome.test.assertEq(outputRule1, rules[1]); On 2012/02/02 19:12:36, battre wrote: > On 2012/02/02 11:59:16, kalman wrote: > > perhaps more concise as > > > > chrome.test.assertEq([outputRule0, outputRule1], rules); > > > > (since you also wouldn't need the rules.length == 2 check) > > I have changed this to > chrome.test.assertEq([outputRule0, outputRule1], rules); > > I have not removed the rules.length == 2 here because I need to check that > rules.length>0 for the "id" in rules[0] test. I'd like to keep that on top > because it generates more sensible error messages that way in case something > breaks. Isn't the priority=100 check redundant though, since outputRule0 has priority=100? https://chromiumcodereview.appspot.com/9315010/diff/4002/chrome/test/data/ext... chrome/test/data/extensions/api_test/declarative/api/background.js:91: declarative.testEvent.getRules([], callback); > If I change the schema so that the both parameter of getRules are optional and > pass "null, callback" or "undefined, callback", the binding layer creates an > empty list in both cases. That's surprising? The Storage API uses the tactic of passing through null/undefined in Get to get everything. See manifest: http://code.google.com/codesearch#OAMlx_jo-ck/src/chrome/common/extensions/ap... Code: http://code.google.com/codesearch#OAMlx_jo-ck/src/chrome/browser/extensions/s... Can you find where the bindings layer turns null -> [] ? That would seem like a bug to me...
http://codereview.chromium.org/9315010/diff/4002/chrome/browser/extensions/ap... File chrome/browser/extensions/api/declarative/rules_registry_service.h (right): http://codereview.chromium.org/9315010/diff/4002/chrome/browser/extensions/ap... chrome/browser/extensions/api/declarative/rules_registry_service.h:36: // Use the RulesRegistryServiceFactory to retrieve an instance of this class. On 2012/02/02 19:12:36, battre wrote: > On 2012/02/02 11:59:16, kalman wrote: > > Why the need for RulesRegistryServiceFactory? Can this just directly belong > to > > a Profile (or an ExtensionService, which is basically a dumping ground for > > extensions things that are tied to the lifetime of a Profile). > > > > Reading the documentation about ProfileFactoryServiceBlahStuff, it seems to be > > for Profile-lifetime objets that have initialisation interdependencies and > > therefore complicated initialisation procedures, so hopefully unnecessary > here. > > I raised this concern in a previous Patch Set version as well. I had the > previous request not to let ExtensionService grow but I think in this case it is > still the cleaner solution. I will get rid of the Factory. > > Done. It is OK to add members to ExtensionService right now, because there is no other home for them. But I don't want significant additional logic in there. See: crbug.com/104095 for the bigger context. http://codereview.chromium.org/9315010/diff/4002/chrome/common/extensions/api... File chrome/common/extensions/api/experimental.declarative.json (right): http://codereview.chromium.org/9315010/diff/4002/chrome/common/extensions/api... chrome/common/extensions/api/experimental.declarative.json:31: } On 2012/02/02 19:12:36, battre wrote: > On 2012/02/02 16:28:51, Aaron Boodman wrote: > > On 2012/02/02 11:59:16, kalman wrote: > > > I don't think we can have these here as part of the API. Having nodoc=true > > hides > > > it from documentation*, but it doesn't hide it from extensions themselves, > > where > > > the namespace will still be polluted with these types and testEvent. > > > > > > Any reason why you can't use the webRequest stuff for tests? > > > > > > * (though it's a bit redundant here since the whole API is nodoc) > > > > It's a bigger problem that. The whole declarative API will also be exposed, > > uselessly. > > > > I guess there are two paths here: > > > > 1) Hack JSON schema and SchemaGeneratedBindings some more to prevent these > names > > from being exposed to extensions (but they still need to be exposed to other > > bindings files). > > > > 2) (Sorry if this playing with dead snakes a bit) Bite the bullet and do this > > with normal IPC. This will properly decouple the renderer and browser side of > > extension bindings, the way it should have always been done. > > > > Revisiting the previous thread about this (from > > https://chromiumcodereview.appspot.com/9315010), Dominic's reasons for not > using > > real IPC were: > > > > a. validation of arguments > > b. asynchronous messaging with callbacks (addRules needs to "return" something > > to the callback function) > > c. association of messages to their extensions? > > d. possibly some security features? > > > > a) is not a valid argument. We will still do param validation because we will > > still use JSON schema for the frontend of the API. It's just that instead of > the > > bindings calling the sendRequest() infrastructure, it will end up calling some > > other native function, which will call a custom IPC. The native function does > > not need to be careful with validation because it can assume the JSON schema > > already did that. > > > > b), c), and d) are somewhat legit, but I believe that code can be easily > > refactored out of EFD. > > > > I'm running out the door, but please see the recent thread on > > chrome-extensions-team w/ subject "A proposed IDL syntax" for more context. > > > > To be honest, I am not sure how much work there is in this approach. It needs > a > > little investigation. That is why originally, I backed off proposing this > > because I didn't want to randomize Dominic too much. However, as we add more > and > > more hacks to SGB, I'm continually thinking that we're pushing it in a bad > > direction. If it were me doing it, I would investigate the IPC route. > > I am still in favor of this approach. I don't see any advantage if we use the > same JSON schema. Did you see the new CL after incorporating Ben's suggestions > of using CustomBindings? It became really neat! See > http://codereview.chromium.org/9192029/ Caveat: I'm willing to let this go. Not because I agree with you :), but because it's asking you to do significant infrastructure work which I don't feel is fair. Anyway, you are just following a bad pattern we have already set down. Not your fault. I do think that we should add the 'internal' flag though, so that we don't expose these APIs to extensions. We should add the internal flag to every occurrence of this pattern - which should be a subset of the things marked 'nodoc'. Ben, can you do this? But anyway... > About the effort of doing something ourselves: > - We need a to bind the native functions in a V8Context Not sure what you mean by this. We already have the native function keyword and numerous examples of using it. There is no work to do here. > - We need to introduce new IPC messages This is a pretty common occurrence in Chrome development. Nothing to be afraid of =P. > - We need to implement the association of requests and responses > - We need to implement the association of messages to individual renderers > - We need to implement security for permissions strings > - We need to implement error handling This is all pretty trivial plumbing code. The only thing I would really count as 'duplication' is the security checks. Of course those should be refactored into some shared location. > This looks like a lot of unnecessary effort that is basically code duplication. > > Can you tell me what advantages you see in approach 2? The extension API system was originally designed to expose APIs from the browser process into JS in renderers directly. All the steps between JS and ExtensionFunction implementations were generalized/generated. Over time, we realized that having the entire stack generalized was insufficient and that we frequently needed to do custom things in the renderer. Now, it has gotten to the point that we our renderer-side bindings are often completely custom. The generalized bindings system itself is now not really used and is just serving as an inefficient and difficult to use IPC system! The disadvantages of abusing SGB system this way are: 1. Non-public bindings inadvertently exposed to extensions. Exposing non-public things is just a bad engineering practice. It can lead to security issues, brittleness in the system, etc. 1a. In order to fix 1, we must add complexity to SGB by introducing 'internal' or 'private' and an implementation of that. 2. JSON is a pain to send and consume in C++. Calamity's stuff is going to help with that, but that introduces tons of complexity. Just using Chrome's built in IPC would be so much simpler and cleaner. These aren't huge problems, I just feel like it's the right way to do it. It's going to happen one way or another, because this is also what we need to do to properly support NaCL and Dart.
> > > I do think that we should add the 'internal' flag though, so that we > don't expose these APIs to extensions. We should add the internal flag > to every occurrence of this pattern - which should be a subset of the > things marked 'nodoc'. Ben, can you do this? > > Yep.
On 2012/02/03 22:38:55, kalman_google wrote: > > > > > > I do think that we should add the 'internal' flag though, so that we > > don't expose these APIs to extensions. We should add the internal flag > > to every occurrence of this pattern - which should be a subset of the > > things marked 'nodoc'. Ben, can you do this? > > > > > Yep. https://chromiumcodereview.appspot.com/9309114/ Not much point reviewing/submitting this until the API has been committed, since it's the only one at the moment which would need it, but there you go.
I have addressed all remaining issues (added a unit test, removed the test classes from experimental.declarative.json, and some other minor things). I think we are done. http://codereview.chromium.org/9315010/diff/4002/chrome/browser/extensions/ap... File chrome/browser/extensions/api/declarative/rule_identifier.cc (right): http://codereview.chromium.org/9315010/diff/4002/chrome/browser/extensions/ap... chrome/browser/extensions/api/declarative/rule_identifier.cc:9: RuleIdentifier::RuleIdentifier(const std::string& extension_id, On 2012/02/02 16:28:51, Aaron Boodman wrote: > unit test? Done. http://codereview.chromium.org/9315010/diff/4002/chrome/browser/extensions/ap... File chrome/browser/extensions/api/declarative/rules_registry_service.cc (right): http://codereview.chromium.org/9315010/diff/4002/chrome/browser/extensions/ap... chrome/browser/extensions/api/declarative/rules_registry_service.cc:42: // RegisterRuleRegistry("chrome.net", new FoobarRulesRegistry()); On 2012/02/02 11:59:16, kalman wrote: > Yeah, if you make this owned by a Profile or ExtensionService, then make that > responsible for registering the appropriate RulesRegistries. Done. http://codereview.chromium.org/9315010/diff/4002/chrome/browser/extensions/ap... File chrome/browser/extensions/api/declarative/test_rules_registry.cc (right): http://codereview.chromium.org/9315010/diff/4002/chrome/browser/extensions/ap... chrome/browser/extensions/api/declarative/test_rules_registry.cc:44: for (i = rules.begin(); i != rules.end(); ++i) { On 2012/02/03 13:06:20, kalman wrote: > On 2012/02/02 19:12:36, battre wrote: > > On 2012/02/02 11:59:16, kalman wrote: > > > I prefer to always define iterators inline, and wrap however necessary to > make > > > that work. > > > > Do you feel strongly about this? I dislike the for statements that stretch > over > > 3 lines. > > I generally don't feel strongly enough about style things to worry about talking > about it. > > However, I dislike situations where you have multiple iterators in the one > function, e.g. > > foo() { > iterator i; > for (i = bar.begin(); i < bar.end(); i++) > ... > iterator j; > for (j = bar.begin(); j < bar.end(); j++) > ... > } > > I've had those situations before. Basically, I'm just in the habit now of > always limiting iterator scope to the for loops regardless of... anything. > > By the way, I don't think it would stretch over 3 lines: > > for (std::vector<DictionaryValue*>::const_iterator i = > rules.begin(); i < rules.end(); ++i) { > ... > } > } Done. http://codereview.chromium.org/9315010/diff/4002/chrome/test/data/extensions/... File chrome/test/data/extensions/api_test/declarative/api/background.js (right): http://codereview.chromium.org/9315010/diff/4002/chrome/test/data/extensions/... chrome/test/data/extensions/api_test/declarative/api/background.js:74: chrome.test.assertEq(outputRule1, rules[1]); On 2012/02/03 13:06:20, kalman wrote: > On 2012/02/02 19:12:36, battre wrote: > > On 2012/02/02 11:59:16, kalman wrote: > > > perhaps more concise as > > > > > > chrome.test.assertEq([outputRule0, outputRule1], rules); > > > > > > (since you also wouldn't need the rules.length == 2 check) > > > > I have changed this to > > chrome.test.assertEq([outputRule0, outputRule1], rules); > > > > I have not removed the rules.length == 2 here because I need to check that > > rules.length>0 for the "id" in rules[0] test. I'd like to keep that on top > > because it generates more sensible error messages that way in case something > > breaks. > > Isn't the priority=100 check redundant though, since outputRule0 has > priority=100? I had the separate test to create simpler to read failure reports, but I am fine with removing this as well. Done. http://codereview.chromium.org/9315010/diff/4002/chrome/test/data/extensions/... chrome/test/data/extensions/api_test/declarative/api/background.js:91: declarative.testEvent.getRules([], callback); On 2012/02/03 13:06:20, kalman wrote: > > > If I change the schema so that the both parameter of getRules are optional and > > pass "null, callback" or "undefined, callback", the binding layer creates an > > empty list in both cases. > > That's surprising? The Storage API uses the tactic of passing through > null/undefined in Get to get everything. > > See manifest: > http://code.google.com/codesearch#OAMlx_jo-ck/src/chrome/common/extensions/ap... > Code: > http://code.google.com/codesearch#OAMlx_jo-ck/src/chrome/browser/extensions/s... > > Can you find where the bindings layer turns null -> [] ? That would seem like a > bug to me... I have no idea why it did not work last time. Done. http://codereview.chromium.org/9315010/diff/4002/chrome/test/data/extensions/... chrome/test/data/extensions/api_test/declarative/api/background.js:135: declarative.testEvent.removeRules([], callback); On 2012/02/02 19:12:36, battre wrote: > On 2012/02/02 11:59:16, kalman wrote: > > See comment above about getRules. > > see above Done. http://codereview.chromium.org/9315010/diff/3003/chrome/common/extensions/api... File chrome/common/extensions/api/experimental.declarative.json (right): http://codereview.chromium.org/9315010/diff/3003/chrome/common/extensions/api... chrome/common/extensions/api/experimental.declarative.json:35: "id": "TestCondition", I have removed these. Let's hope it does not bite us once we extend the functionality of the webRequest APIs.
Sorry to have so many comments again, I feel like a bit of an ass... http://codereview.chromium.org/9315010/diff/11001/chrome/browser/extensions/a... File chrome/browser/extensions/api/declarative/declarative_apitest.cc (right): http://codereview.chromium.org/9315010/diff/11001/chrome/browser/extensions/a... chrome/browser/extensions/api/declarative/declarative_apitest.cc:27: command_line->AppendSwitch(switches::kEnableExperimentalExtensionApis); other tests seem to do CommandLine::ForCurrentProcess()->AppendSwitch( switches::kEnableExperimentalExtensionApis); in tests. Perhaps that would also work in the constructor, rather than needing to override a method for this. http://codereview.chromium.org/9315010/diff/11001/chrome/browser/extensions/a... chrome/browser/extensions/api/declarative/declarative_apitest.cc:31: using namespace extensions; put at top of file http://codereview.chromium.org/9315010/diff/11001/chrome/browser/extensions/a... chrome/browser/extensions/api/declarative/declarative_apitest.cc:37: rules_registry->UnregisterAllRulesRegistries(); Why this? Is it so that the previous test's registered rules get cleared, or is to clear the already-registered rules (e.g. webRequest)? If the former, then it should be unnecessary, since it's a new browser instance for each test. And don't the rules get unloaded anyway (I can see that you're testing that below)? If the latter, then hopefully it's unnecessary because it would work without it. http://codereview.chromium.org/9315010/diff/11001/chrome/browser/extensions/a... File chrome/browser/extensions/api/declarative/rules_registry_service.h (right): http://codereview.chromium.org/9315010/diff/11001/chrome/browser/extensions/a... chrome/browser/extensions/api/declarative/rules_registry_service.h:38: void UnregisterAllRulesRegistries(); it's unfortunate to need this; see comment in the browser test about hopefully removing it. http://codereview.chromium.org/9315010/diff/11001/chrome/browser/extensions/a... chrome/browser/extensions/api/declarative/rules_registry_service.h:42: // on which the function was called. This interface could be cleaned up a lot by only having a GetRegistry(event_name) method. Rather than users of RulesRegistryService going service->AddRules("foo", ...); they would go service->GetRegistry("foo")->AddRules(...); And then you can delete AddRules/RemoveRules/GetRules/whatever else you need to add from this interface. http://codereview.chromium.org/9315010/diff/11001/chrome/browser/extensions/a... File chrome/browser/extensions/api/declarative/test_rules_registry.cc (right): http://codereview.chromium.org/9315010/diff/11001/chrome/browser/extensions/a... chrome/browser/extensions/api/declarative/test_rules_registry.cc:53: STLDeleteValues(&rules_); See comment in the API file I made about this (applies to GetRules). Either add a new method RemoveAllRules or a new version of RemoveRules which takes no rule identifiers (i.e. std::string TestRulesRegistry::RemoveRules(extension_id)). The behaviour of [] -> everything is confusing. http://codereview.chromium.org/9315010/diff/11001/chrome/browser/extensions/a... chrome/browser/extensions/api/declarative/test_rules_registry.cc:76: std::vector<std::string>::const_iterator i; same thing with the iterator here http://codereview.chromium.org/9315010/diff/10003/chrome/browser/extensions/a... File chrome/browser/extensions/api/declarative/declarative_api.cc (right): http://codereview.chromium.org/9315010/diff/10003/chrome/browser/extensions/a... chrome/browser/extensions/api/declarative/declarative_api.cc:26: DictionaryValue* rule; = NULL http://codereview.chromium.org/9315010/diff/10003/chrome/browser/extensions/a... chrome/browser/extensions/api/declarative/declarative_api.cc:44: my biased opinion based on how similar code in the storage API ended up being, but I kind of like it as follows. It clearly enumerates what the input values can be (and as a result does more error checking), doesn't instantiate unneeded objects, and has a clearly defined exit point. === std::string event_name; EXTENSION_FUNCTION_VALIDATE(args_->GetString(0, &event_name)); Value* rule_identifiers = NULL; // it's not necessary a list value so // misleading to call it as such. EXTENSION_FUNCTION_VALIDATE(args_->Get(1, &rule_identifiers)); RulesRegistry* rules_registry = profile()->GetExtensionService()->GetRulesRegistryService()-> GetRegistry(event_name); switch (rule_identifiers->GetType()) { case Value::TYPE_NULL: error_ = rules_registry->RemoveAllRules(extension_id()); break; case Value::TYPE_LIST: { std::vector<std::string> rule_identifiers_list; // Write this out, or have it as a util function, whatever. // It makes sense to have it as a util since you'd use it in AddRules too. AddAllStringValues(static_cast<ListValue*>(rules_registry), &rule_identifiers_list)); error_ = rules_registry->RemoveRules(extension_id(), rule_identifiers_list); break; } default: error_ = kUnsupportedTypeOrSomething; break; } return !error_.empty(); http://codereview.chromium.org/9315010/diff/10003/chrome/browser/extensions/a... chrome/browser/extensions/api/declarative/declarative_api.cc:52: EXTENSION_FUNCTION_VALIDATE(args_->GetList(1, &rule_identifiers_list)); This could fail if the extension does a call like chrome.declarative.addRules("hello"), or anything else in the place of "hello" which isn't a list or null. We shouldn't be killing the extension in those cases, just returning an error to that call. http://codereview.chromium.org/9315010/diff/10003/chrome/browser/extensions/a... chrome/browser/extensions/api/declarative/declarative_api.cc:65: return true; This is still a confusing API, where an empty list -> get all rules. - RemoveRules should just remove what it's told, even if that's empty. That would let you delete this logic. - Add a new method RemoveAllRules, or RemoveRules overloaded without a list argument, to remove all the rules and call that. http://codereview.chromium.org/9315010/diff/10003/chrome/browser/extensions/a... chrome/browser/extensions/api/declarative/declarative_api.cc:78: Same comment here as in RemoveRulesFunction, both in terms of code structure and for the Get(All)Rules thing. http://codereview.chromium.org/9315010/diff/10003/chrome/browser/extensions/a... chrome/browser/extensions/api/declarative/declarative_api.cc:107: typedef std::vector<DictionaryValue*> RulesList; IMO typedef unnecessary. http://codereview.chromium.org/9315010/diff/10003/chrome/browser/extensions/a... File chrome/browser/extensions/api/declarative/initializing_rules_registry.cc (right): http://codereview.chromium.org/9315010/diff/10003/chrome/browser/extensions/a... chrome/browser/extensions/api/declarative/initializing_rules_registry.cc:17: return "_" + base::IntToString(identifier) + "_"; I think c++ will automatically cast int -> std::string if you have ``return "_" + identifier + "_"''. There's also the StringPrintf option that some would recommend. BTW, this method doesn't need to be indented inside the namespace{} block; and the end of namespace{} needs a "// namespace" comment. http://codereview.chromium.org/9315010/diff/10003/chrome/browser/extensions/a... chrome/browser/extensions/api/declarative/initializing_rules_registry.cc:25: : delegate_(delegate.Pass()), I don't think you need the .Pass(). http://codereview.chromium.org/9315010/diff/10003/chrome/browser/extensions/a... chrome/browser/extensions/api/declarative/initializing_rules_registry.cc:72: ToId(last_generated_rule_identifier_id_)))) { indentation doesn't need to be this whitespacey? while (!IsUniqueId(RuleIdentifier( extension_id, ToId(last_generated_rule_identifier_id_)))) { ++last_generated_rule_identifier_id_; } http://codereview.chromium.org/9315010/diff/10003/chrome/browser/extensions/a... File chrome/browser/extensions/api/declarative/initializing_rules_registry.h (right): http://codereview.chromium.org/9315010/diff/10003/chrome/browser/extensions/a... chrome/browser/extensions/api/declarative/initializing_rules_registry.h:65: std::set<RuleIdentifier> used_rule_identifiers_; Is there any reason why this can't be std::map<std::string, std::set<std::string> > used_rule_identifiers_; a map of extension id -> used rule identifiers for that extension? we could do away with all that RuleIdentifier logic... http://codereview.chromium.org/9315010/diff/10003/chrome/browser/extensions/a... File chrome/browser/extensions/api/declarative/rules_registry.h (right): http://codereview.chromium.org/9315010/diff/10003/chrome/browser/extensions/a... chrome/browser/extensions/api/declarative/rules_registry.h:52: std::string* error) = 0; I think it's possible/simpler to return a std::string error (rather than as an out-parameter) and having (return value).empty() imply success. http://codereview.chromium.org/9315010/diff/10003/chrome/browser/extensions/e... File chrome/browser/extensions/extension_service.cc (right): http://codereview.chromium.org/9315010/diff/10003/chrome/browser/extensions/e... chrome/browser/extensions/extension_service.cc:1175: rules_registry_service_->OnExtensionUnloaded(extension->id()); Rather than this, have the RulesRegistryService listen to EXTENSION_UNLOADED events. git grep -w NOTIFICATION_EXTENSION_UNLOADED
http://codereview.chromium.org/9315010/diff/11001/chrome/browser/extensions/a... File chrome/browser/extensions/api/declarative/test_rules_registry.h (right): http://codereview.chromium.org/9315010/diff/11001/chrome/browser/extensions/a... chrome/browser/extensions/api/declarative/test_rules_registry.h:42: std::map<std::string, DictionaryValue*> rules_; also: linked_ptr<DictionaryValue> rather than manually deleting stuff.
http://codereview.chromium.org/9315010/diff/10003/chrome/browser/extensions/a... File chrome/browser/extensions/api/declarative/declarative_api.cc (right): http://codereview.chromium.org/9315010/diff/10003/chrome/browser/extensions/a... chrome/browser/extensions/api/declarative/declarative_api.cc:52: EXTENSION_FUNCTION_VALIDATE(args_->GetList(1, &rule_identifiers_list)); On 2012/02/07 03:25:10, kalman wrote: > This could fail if the extension does a call like > > chrome.declarative.addRules("hello"), or anything else in the place of "hello" > which isn't a list or null. > > We shouldn't be killing the extension in those cases, just returning an error to > that call. (this is a bit moot given my other comments anyway; but my bad, this code wouldn't get reached because the renderer should already be doing validation so killing it would be ok)
Hi Ben, The InitializingRulesRegistry got such a complexity that I want to add unittests. I am sending you the current status anyway, so that we can get another iteration in the review process. I have addressed your comments. Best regards, Dominic http://codereview.chromium.org/9315010/diff/11001/chrome/browser/extensions/a... File chrome/browser/extensions/api/declarative/declarative_apitest.cc (right): http://codereview.chromium.org/9315010/diff/11001/chrome/browser/extensions/a... chrome/browser/extensions/api/declarative/declarative_apitest.cc:27: command_line->AppendSwitch(switches::kEnableExperimentalExtensionApis); On 2012/02/07 03:25:10, kalman wrote: > other tests seem to do > > CommandLine::ForCurrentProcess()->AppendSwitch( > switches::kEnableExperimentalExtensionApis); > > in tests. Perhaps that would also work in the constructor, rather than needing > to override a method for this. There are several examples of using it this way: download_extension_apitest.cc, dns_apitest, permissions_apitest.cc serial_apitest.cc ... http://codereview.chromium.org/9315010/diff/11001/chrome/browser/extensions/a... chrome/browser/extensions/api/declarative/declarative_apitest.cc:31: using namespace extensions; On 2012/02/07 03:25:10, kalman wrote: > put at top of file There was some discussion on chrome-team that kind of discouraged this, but I think there was no clear consensus, so I have done it. Done. http://codereview.chromium.org/9315010/diff/11001/chrome/browser/extensions/a... chrome/browser/extensions/api/declarative/declarative_apitest.cc:37: rules_registry->UnregisterAllRulesRegistries(); On 2012/02/07 03:25:10, kalman wrote: > Why this? Is it so that the previous test's registered rules get cleared, or is > to clear the already-registered rules (e.g. webRequest)? > > If the former, then it should be unnecessary, since it's a new browser instance > for each test. And don't the rules get unloaded anyway (I can see that you're > testing that below)? > > If the latter, then hopefully it's unnecessary because it would work without it. The idea here is,that the webRequest implementation my represent rules internally in a different way. It could be possible that getRules() does not return exactly the same representation of rules as was passed to addRules(). This would break the apitests. I have removed this, as we can introduce it back in case we decide that we need it. Done. http://codereview.chromium.org/9315010/diff/11001/chrome/browser/extensions/a... File chrome/browser/extensions/api/declarative/rules_registry_service.h (right): http://codereview.chromium.org/9315010/diff/11001/chrome/browser/extensions/a... chrome/browser/extensions/api/declarative/rules_registry_service.h:38: void UnregisterAllRulesRegistries(); On 2012/02/07 03:25:10, kalman wrote: > it's unfortunate to need this; see comment in the browser test about hopefully > removing it. Done. http://codereview.chromium.org/9315010/diff/11001/chrome/browser/extensions/a... chrome/browser/extensions/api/declarative/rules_registry_service.h:42: // on which the function was called. On 2012/02/07 03:25:10, kalman wrote: > This interface could be cleaned up a lot by only having a > GetRegistry(event_name) method. Rather than users of RulesRegistryService going > > service->AddRules("foo", ...); > > they would go > > service->GetRegistry("foo")->AddRules(...); > > And then you can delete AddRules/RemoveRules/GetRules/whatever else you need to > add from this interface. Done. http://codereview.chromium.org/9315010/diff/11001/chrome/browser/extensions/a... File chrome/browser/extensions/api/declarative/test_rules_registry.cc (right): http://codereview.chromium.org/9315010/diff/11001/chrome/browser/extensions/a... chrome/browser/extensions/api/declarative/test_rules_registry.cc:53: STLDeleteValues(&rules_); On 2012/02/07 03:25:10, kalman wrote: > See comment in the API file I made about this (applies to GetRules). > > Either add a new method RemoveAllRules or a new version of RemoveRules which > takes no rule identifiers (i.e. std::string > TestRulesRegistry::RemoveRules(extension_id)). The behaviour of [] -> everything > is confusing. Done. http://codereview.chromium.org/9315010/diff/11001/chrome/browser/extensions/a... chrome/browser/extensions/api/declarative/test_rules_registry.cc:76: std::vector<std::string>::const_iterator i; On 2012/02/07 03:25:10, kalman wrote: > same thing with the iterator here Done. http://codereview.chromium.org/9315010/diff/11001/chrome/browser/extensions/a... File chrome/browser/extensions/api/declarative/test_rules_registry.h (right): http://codereview.chromium.org/9315010/diff/11001/chrome/browser/extensions/a... chrome/browser/extensions/api/declarative/test_rules_registry.h:42: std::map<std::string, DictionaryValue*> rules_; On 2012/02/07 03:28:10, kalman wrote: > also: linked_ptr<DictionaryValue> rather than manually deleting stuff. Done. http://codereview.chromium.org/9315010/diff/10003/chrome/browser/extensions/a... File chrome/browser/extensions/api/declarative/declarative_api.cc (right): http://codereview.chromium.org/9315010/diff/10003/chrome/browser/extensions/a... chrome/browser/extensions/api/declarative/declarative_api.cc:26: DictionaryValue* rule; On 2012/02/07 03:25:10, kalman wrote: > = NULL Done. http://codereview.chromium.org/9315010/diff/10003/chrome/browser/extensions/a... chrome/browser/extensions/api/declarative/declarative_api.cc:44: On 2012/02/07 03:25:10, kalman wrote: > my biased opinion based on how similar code in the storage API ended up being, > but I kind of like it as follows. It clearly enumerates what the input values > can be (and as a result does more error checking), doesn't instantiate unneeded > objects, and has a clearly defined exit point. > > === > > std::string event_name; > EXTENSION_FUNCTION_VALIDATE(args_->GetString(0, &event_name)); > Value* rule_identifiers = NULL; // it's not necessary a list value so > // misleading to call it as such. > EXTENSION_FUNCTION_VALIDATE(args_->Get(1, &rule_identifiers)); > > RulesRegistry* rules_registry = > profile()->GetExtensionService()->GetRulesRegistryService()-> > GetRegistry(event_name); > > switch (rule_identifiers->GetType()) { > case Value::TYPE_NULL: > error_ = rules_registry->RemoveAllRules(extension_id()); > break; > > case Value::TYPE_LIST: { > std::vector<std::string> rule_identifiers_list; > // Write this out, or have it as a util function, whatever. > // It makes sense to have it as a util since you'd use it in AddRules too. > AddAllStringValues(static_cast<ListValue*>(rules_registry), > &rule_identifiers_list)); > error_ = rules_registry->RemoveRules(extension_id(), rule_identifiers_list); > break; > } > > default: > error_ = kUnsupportedTypeOrSomething; > break; > } > > return !error_.empty(); Done. http://codereview.chromium.org/9315010/diff/10003/chrome/browser/extensions/a... chrome/browser/extensions/api/declarative/declarative_api.cc:65: return true; On 2012/02/07 03:25:10, kalman wrote: > This is still a confusing API, where an empty list -> get all rules. > - RemoveRules should just remove what it's told, even if that's empty. That > would let you delete this logic. > - Add a new method RemoveAllRules, or RemoveRules overloaded without a list > argument, to remove all the rules and call that. Done. http://codereview.chromium.org/9315010/diff/10003/chrome/browser/extensions/a... chrome/browser/extensions/api/declarative/declarative_api.cc:78: On 2012/02/07 03:25:10, kalman wrote: > Same comment here as in RemoveRulesFunction, both in terms of code structure and > for the Get(All)Rules thing. Done. http://codereview.chromium.org/9315010/diff/10003/chrome/browser/extensions/a... chrome/browser/extensions/api/declarative/declarative_api.cc:107: typedef std::vector<DictionaryValue*> RulesList; On 2012/02/07 03:25:10, kalman wrote: > IMO typedef unnecessary. Done. http://codereview.chromium.org/9315010/diff/10003/chrome/browser/extensions/a... File chrome/browser/extensions/api/declarative/initializing_rules_registry.cc (right): http://codereview.chromium.org/9315010/diff/10003/chrome/browser/extensions/a... chrome/browser/extensions/api/declarative/initializing_rules_registry.cc:17: return "_" + base::IntToString(identifier) + "_"; On 2012/02/07 03:25:10, kalman wrote: > I think c++ will automatically cast int -> std::string if you have ``return "_" > + identifier + "_"''. No. > There's also the StringPrintf option that some would recommend. I don't know which version is faster but have changed it. > BTW, this method doesn't need to be indented inside the namespace{} block; and > the end of namespace{} needs a "// namespace" comment. I think we don't want to export this function beyond the compile unit. Done. http://codereview.chromium.org/9315010/diff/10003/chrome/browser/extensions/a... chrome/browser/extensions/api/declarative/initializing_rules_registry.cc:25: : delegate_(delegate.Pass()), On 2012/02/07 03:25:10, kalman wrote: > I don't think you need the .Pass(). I do: error: 'scoped_ptr<C>::scoped_ptr(scoped_ptr<C>&) [with C = extensions::RulesRegistry]' is private http://codereview.chromium.org/9315010/diff/10003/chrome/browser/extensions/a... chrome/browser/extensions/api/declarative/initializing_rules_registry.cc:72: ToId(last_generated_rule_identifier_id_)))) { On 2012/02/07 03:25:10, kalman wrote: > indentation doesn't need to be this whitespacey? > > while (!IsUniqueId(RuleIdentifier( > extension_id, ToId(last_generated_rule_identifier_id_)))) { > ++last_generated_rule_identifier_id_; > } Hm, I am not a big fan of arbitrary line breaks. But it became much nicer anyway. http://codereview.chromium.org/9315010/diff/10003/chrome/browser/extensions/a... File chrome/browser/extensions/api/declarative/initializing_rules_registry.h (right): http://codereview.chromium.org/9315010/diff/10003/chrome/browser/extensions/a... chrome/browser/extensions/api/declarative/initializing_rules_registry.h:65: std::set<RuleIdentifier> used_rule_identifiers_; On 2012/02/07 03:25:10, kalman wrote: > Is there any reason why this can't be > > std::map<std::string, std::set<std::string> > used_rule_identifiers_; > > a map of extension id -> used rule identifiers for that extension? > > we could do away with all that RuleIdentifier logic... Good point. This was not possible before the refactoring of the InitializingRulesRegistry. Done. http://codereview.chromium.org/9315010/diff/10003/chrome/browser/extensions/a... File chrome/browser/extensions/api/declarative/rules_registry.h (right): http://codereview.chromium.org/9315010/diff/10003/chrome/browser/extensions/a... chrome/browser/extensions/api/declarative/rules_registry.h:52: std::string* error) = 0; On 2012/02/07 03:25:10, kalman wrote: > I think it's possible/simpler to return a std::string error (rather than as an > out-parameter) and having (return value).empty() imply success. We need to change the interface to be asynchronous anyway (the webrequest API implementation needs to live on the IO thread and even implementations that live on the UI thread might require some preprocessing that should not happen on the UI thread). I will do that change in a separate CL. http://codereview.chromium.org/9315010/diff/10003/chrome/browser/extensions/e... File chrome/browser/extensions/extension_service.cc (right): http://codereview.chromium.org/9315010/diff/10003/chrome/browser/extensions/e... chrome/browser/extensions/extension_service.cc:1175: rules_registry_service_->OnExtensionUnloaded(extension->id()); On 2012/02/07 03:25:10, kalman wrote: > Rather than this, have the RulesRegistryService listen to EXTENSION_UNLOADED > events. > > git grep -w NOTIFICATION_EXTENSION_UNLOADED I had that before and removed it once I transferred ownership of the rules registry service from the profile to the extension service. I think that given the direct ownership I prefer the current implementation over some broadcasting mechanism. Would you want to subscribe each RulesRegistry to NOTIFICATION_EXTENSION_UNLOADED?
Looking really good. Just those last couple of things from me. http://codereview.chromium.org/9315010/diff/11001/chrome/browser/extensions/a... File chrome/browser/extensions/api/declarative/declarative_apitest.cc (right): http://codereview.chromium.org/9315010/diff/11001/chrome/browser/extensions/a... chrome/browser/extensions/api/declarative/declarative_apitest.cc:31: using namespace extensions; On 2012/02/07 18:45:33, battre wrote: > On 2012/02/07 03:25:10, kalman wrote: > > put at top of file > > There was some discussion on chrome-team that kind of discouraged this, but I > think there was no clear consensus, so I have done it. > > Done. oh really? Well like many things it's a judgement call I guess, but IIRC that discussion the same arguments applied to why to not have "using namespace extensions" would apply here as well? Maybe not. Either case it's pretty fair to have extensions code that doesn't live in the extensions namespace to "using" it. http://codereview.chromium.org/9315010/diff/10003/chrome/browser/extensions/a... File chrome/browser/extensions/api/declarative/initializing_rules_registry.cc (right): http://codereview.chromium.org/9315010/diff/10003/chrome/browser/extensions/a... chrome/browser/extensions/api/declarative/initializing_rules_registry.cc:17: return "_" + base::IntToString(identifier) + "_"; On 2012/02/07 18:45:33, battre wrote: > On 2012/02/07 03:25:10, kalman wrote: > > I think c++ will automatically cast int -> std::string if you have ``return > "_" > > + identifier + "_"''. > > No. > > > There's also the StringPrintf option that some would recommend. > > I don't know which version is faster but have changed it. > > > BTW, this method doesn't need to be indented inside the namespace{} block; and > > the end of namespace{} needs a "// namespace" comment. > > I think we don't want to export this function beyond the compile unit. Yeah, that's not what I meant though, just unindent it. But that's what you did so cool. > > > Done. http://codereview.chromium.org/9315010/diff/10003/chrome/browser/extensions/a... chrome/browser/extensions/api/declarative/initializing_rules_registry.cc:25: : delegate_(delegate.Pass()), On 2012/02/07 18:45:33, battre wrote: > On 2012/02/07 03:25:10, kalman wrote: > > I don't think you need the .Pass(). > > I do: error: 'scoped_ptr<C>::scoped_ptr(scoped_ptr<C>&) [with C = > extensions::RulesRegistry]' is private cool, good to know. http://codereview.chromium.org/9315010/diff/10003/chrome/browser/extensions/a... File chrome/browser/extensions/api/declarative/rules_registry.h (right): http://codereview.chromium.org/9315010/diff/10003/chrome/browser/extensions/a... chrome/browser/extensions/api/declarative/rules_registry.h:52: std::string* error) = 0; On 2012/02/07 18:45:33, battre wrote: > On 2012/02/07 03:25:10, kalman wrote: > > I think it's possible/simpler to return a std::string error (rather than as an > > out-parameter) and having (return value).empty() imply success. > > We need to change the interface to be asynchronous anyway (the webrequest API > implementation needs to live on the IO thread and even implementations that live > on the UI thread might require some preprocessing that should not happen on the > UI thread). I will do that change in a separate CL. Do you mean the FILE thread? IO thread is for IPCs. Either way, the threading issue isn't going to be solved by making this API asynchronous; I think it will end up with the threading stuff a concern of either the API bindings, or another class that wraps this interface (see how the storage API works). So the interface probably won't need to be changed. Unless you're strongly opposed to this method returning the error directly, I think that would be simpler and preferable. http://codereview.chromium.org/9315010/diff/10003/chrome/browser/extensions/a... chrome/browser/extensions/api/declarative/rules_registry.h:60: std::vector<DictionaryValue*>* out) = 0; Should GetRules have the ability to report errors? http://codereview.chromium.org/9315010/diff/10003/chrome/browser/extensions/e... File chrome/browser/extensions/extension_service.cc (right): http://codereview.chromium.org/9315010/diff/10003/chrome/browser/extensions/e... chrome/browser/extensions/extension_service.cc:1175: rules_registry_service_->OnExtensionUnloaded(extension->id()); On 2012/02/07 18:45:33, battre wrote: > On 2012/02/07 03:25:10, kalman wrote: > > Rather than this, have the RulesRegistryService listen to EXTENSION_UNLOADED > > events. > > > > git grep -w NOTIFICATION_EXTENSION_UNLOADED > > I had that before and removed it once I transferred ownership of the rules > registry service from the profile to the extension service. I think that given > the direct ownership I prefer the current implementation over some broadcasting > mechanism. Would you want to subscribe each RulesRegistry to > NOTIFICATION_EXTENSION_UNLOADED? I agree that it's a shame we use a broadcasting mechanism for this sort of stuff. IMO in general nicer design would be to have some object be responsible for the installation of extensions (currently that's ExtensionService), then anything that wants to be notified of that can register listeners on ExtensionService directly. I guess one problem with listener architecture in C++ is that lifetimes are more difficult to express; you rely on ExtensionService to outlive the object which is listening. Which is presumably why they used an event bus architecture. It's also easier to write, and grep for. In any case, that's what we have. It shouldn't be a concern of ExtensionService that the RulesRegistry wants to know when extensions are uninstalled. So yeah, you'd want to subscribe RulesRegistry to NOTIFICATION_EXTENSION_UNLOADED. http://codereview.chromium.org/9315010/diff/25002/chrome/browser/extensions/a... File chrome/browser/extensions/api/declarative/rules_registry_service.cc (right): http://codereview.chromium.org/9315010/diff/25002/chrome/browser/extensions/a... chrome/browser/extensions/api/declarative/rules_registry_service.cc:28: NOTREACHED(); it seems like this NOTREACHED() check should be pushed up to the API level; only the API knows that this will never be called with an unknown event name (due to the way the events are set up). http://codereview.chromium.org/9315010/diff/25002/chrome/browser/extensions/a... File chrome/browser/extensions/api/declarative/rules_registry_service.h (right): http://codereview.chromium.org/9315010/diff/25002/chrome/browser/extensions/a... chrome/browser/extensions/api/declarative/rules_registry_service.h:33: // must have been registered before. If you push up the assertion to the API level, this comment would be "or NULL if there is no RulesRegistry registered for the event". http://codereview.chromium.org/9315010/diff/25002/chrome/browser/extensions/a... File chrome/browser/extensions/api/declarative/test_rules_registry.cc (right): http://codereview.chromium.org/9315010/diff/25002/chrome/browser/extensions/a... chrome/browser/extensions/api/declarative/test_rules_registry.cc:74: CHECK(entry != rules_.end()); not sure, similar comment here as to the NOTREACHED thing before, however in this case it would also crash the browser in Release mode. We don't want extensions to be able to crash the browser by requesting rules that don't exist. Perhaps GetRules needs the ability to report errors.
I have addressed your issues. I propose to add the unittests for initializing_rules_registry.cc in a separate CL: - I think that the integration tests provide sufficient coverage. - I would need to rewrite the unit tests completely, once calamity has landed his CL. Best regards, Dominic http://codereview.chromium.org/9315010/diff/10003/chrome/browser/extensions/a... File chrome/browser/extensions/api/declarative/rules_registry.h (right): http://codereview.chromium.org/9315010/diff/10003/chrome/browser/extensions/a... chrome/browser/extensions/api/declarative/rules_registry.h:52: std::string* error) = 0; On 2012/02/07 23:54:57, kalman wrote: > On 2012/02/07 18:45:33, battre wrote: > > On 2012/02/07 03:25:10, kalman wrote: > > > I think it's possible/simpler to return a std::string error (rather than as > an > > > out-parameter) and having (return value).empty() imply success. > > > > We need to change the interface to be asynchronous anyway (the webrequest API > > implementation needs to live on the IO thread and even implementations that > live > > on the UI thread might require some preprocessing that should not happen on > the > > UI thread). I will do that change in a separate CL. > > Do you mean the FILE thread? IO thread is for IPCs. The network stack lives on the IO thread. For the declarative webrequest API we will want to handle events in the NetworkDelegate or ExtensionWebRequestEventRouter. Both live on the IO thread. Of course we don't want to block the IO thread indefinitely either. We'll need to figure out a strategy. Maybe we will use the WorkerPool. Either way, for this to work we need to to move from the SyncExtensionFunctions in declarative_api.h to AsyncExtensionFunctions. We can then figure out the best way to handle the RulesRegistries. I can see the following protocol: - AddRulesFunction is called from extension on UI thread - Preprocessing is done on SequencedWorkerPool thread - Internal representation of webrequest rules is replaced on IO thread with the one that was generated in the previous preporcessing thread - AddRulesFunction confirms success on UI thread I can see that the Preprocessing could remain synchronous, trigger an asynchronous update to the IO thread and confirm success before the update on the IO thread has been executed. > Either way, the threading issue isn't going to be solved by making this API > asynchronous; I think it will end up with the threading stuff a concern of > either the API bindings, or another class that wraps this interface (see how the > storage API works). So the interface probably won't need to be changed. Let's see. > Unless you're strongly opposed to this method returning the error directly, I > think that would be simpler and preferable. Ok, I have changed the signatures to return a std::string. Done. http://codereview.chromium.org/9315010/diff/10003/chrome/browser/extensions/a... chrome/browser/extensions/api/declarative/rules_registry.h:60: std::vector<DictionaryValue*>* out) = 0; On 2012/02/07 23:54:57, kalman wrote: > Should GetRules have the ability to report errors? Done. http://codereview.chromium.org/9315010/diff/10003/chrome/browser/extensions/e... File chrome/browser/extensions/extension_service.cc (right): http://codereview.chromium.org/9315010/diff/10003/chrome/browser/extensions/e... chrome/browser/extensions/extension_service.cc:1175: rules_registry_service_->OnExtensionUnloaded(extension->id()); On 2012/02/07 23:54:57, kalman wrote: > On 2012/02/07 18:45:33, battre wrote: > > On 2012/02/07 03:25:10, kalman wrote: > > > Rather than this, have the RulesRegistryService listen to EXTENSION_UNLOADED > > > events. > > > > > > git grep -w NOTIFICATION_EXTENSION_UNLOADED > > > > I had that before and removed it once I transferred ownership of the rules > > registry service from the profile to the extension service. I think that given > > the direct ownership I prefer the current implementation over some > broadcasting > > mechanism. Would you want to subscribe each RulesRegistry to > > NOTIFICATION_EXTENSION_UNLOADED? > > I agree that it's a shame we use a broadcasting mechanism for this sort of > stuff. IMO in general nicer design would be to have some object be responsible > for the installation of extensions (currently that's ExtensionService), then > anything that wants to be notified of that can register listeners on > ExtensionService directly. > > I guess one problem with listener architecture in C++ is that lifetimes are more > difficult to express; you rely on ExtensionService to outlive the object which > is listening. > > Which is presumably why they used an event bus architecture. It's also easier to > write, and grep for. > > In any case, that's what we have. It shouldn't be a concern of ExtensionService > that the RulesRegistry wants to know when extensions are uninstalled. > > So yeah, you'd want to subscribe RulesRegistry to > NOTIFICATION_EXTENSION_UNLOADED. Done. http://codereview.chromium.org/9315010/diff/25002/chrome/browser/extensions/a... File chrome/browser/extensions/api/declarative/rules_registry_service.cc (right): http://codereview.chromium.org/9315010/diff/25002/chrome/browser/extensions/a... chrome/browser/extensions/api/declarative/rules_registry_service.cc:28: NOTREACHED(); On 2012/02/07 23:54:57, kalman wrote: > it seems like this NOTREACHED() check should be pushed up to the API level; only > the API knows that this will never be called with an unknown event name (due to > the way the events are set up). Done. http://codereview.chromium.org/9315010/diff/25002/chrome/browser/extensions/a... File chrome/browser/extensions/api/declarative/rules_registry_service.h (right): http://codereview.chromium.org/9315010/diff/25002/chrome/browser/extensions/a... chrome/browser/extensions/api/declarative/rules_registry_service.h:33: // must have been registered before. On 2012/02/07 23:54:57, kalman wrote: > If you push up the assertion to the API level, this comment would be "or NULL if > there is no RulesRegistry registered for the event". Done. http://codereview.chromium.org/9315010/diff/25002/chrome/browser/extensions/a... File chrome/browser/extensions/api/declarative/test_rules_registry.cc (right): http://codereview.chromium.org/9315010/diff/25002/chrome/browser/extensions/a... chrome/browser/extensions/api/declarative/test_rules_registry.cc:74: CHECK(entry != rules_.end()); On 2012/02/07 23:54:57, kalman wrote: > not sure, similar comment here as to the NOTREACHED thing before, however in > this case it would also crash the browser in Release mode. We don't want > extensions to be able to crash the browser by requesting rules that don't exist. > > Perhaps GetRules needs the ability to report errors. Done. (Note though that this is part of the testing framework - this will be changed completely for the production version.)
LGTM thanks for putting up with me :) http://codereview.chromium.org/9315010/diff/10003/chrome/browser/extensions/a... File chrome/browser/extensions/api/declarative/rules_registry.h (right): http://codereview.chromium.org/9315010/diff/10003/chrome/browser/extensions/a... chrome/browser/extensions/api/declarative/rules_registry.h:52: std::string* error) = 0; On 2012/02/08 12:49:54, battre wrote: > On 2012/02/07 23:54:57, kalman wrote: > > On 2012/02/07 18:45:33, battre wrote: > > > On 2012/02/07 03:25:10, kalman wrote: > > > > I think it's possible/simpler to return a std::string error (rather than > as > > an > > > > out-parameter) and having (return value).empty() imply success. > > > > > > We need to change the interface to be asynchronous anyway (the webrequest > API > > > implementation needs to live on the IO thread and even implementations that > > live > > > on the UI thread might require some preprocessing that should not happen on > > the > > > UI thread). I will do that change in a separate CL. > > > > Do you mean the FILE thread? IO thread is for IPCs. > > The network stack lives on the IO thread. For the declarative webrequest API we > will want to handle events in the NetworkDelegate or > ExtensionWebRequestEventRouter. Both live on the IO thread. > > Of course we don't want to block the IO thread indefinitely either. We'll need > to figure out a strategy. Maybe we will use the WorkerPool. > > Either way, for this to work we need to to move from the SyncExtensionFunctions > in declarative_api.h to AsyncExtensionFunctions. We can then figure out the best > way to handle the RulesRegistries. I can see the following protocol: > > - AddRulesFunction is called from extension on UI thread > - Preprocessing is done on SequencedWorkerPool thread > - Internal representation of webrequest rules is replaced on IO thread with the > one that was generated in the previous preporcessing thread > - AddRulesFunction confirms success on UI thread > > I can see that the Preprocessing could remain synchronous, trigger an > asynchronous update to the IO thread and confirm success before the update on > the IO thread has been executed. > > > Either way, the threading issue isn't going to be solved by making this API > > asynchronous; I think it will end up with the threading stuff a concern of > > either the API bindings, or another class that wraps this interface (see how > the > > storage API works). So the interface probably won't need to be changed. > > Let's see. > > > Unless you're strongly opposed to this method returning the error directly, I > > think that would be simpler and preferable. > > Ok, I have changed the signatures to return a std::string. > > Done. Cool, sounds good. http://codereview.chromium.org/9315010/diff/25002/chrome/browser/extensions/a... File chrome/browser/extensions/api/declarative/test_rules_registry.cc (right): http://codereview.chromium.org/9315010/diff/25002/chrome/browser/extensions/a... chrome/browser/extensions/api/declarative/test_rules_registry.cc:74: CHECK(entry != rules_.end()); On 2012/02/08 12:49:54, battre wrote: > On 2012/02/07 23:54:57, kalman wrote: > > not sure, similar comment here as to the NOTREACHED thing before, however in > > this case it would also crash the browser in Release mode. We don't want > > extensions to be able to crash the browser by requesting rules that don't > exist. > > > > Perhaps GetRules needs the ability to report errors. > > Done. (Note though that this is part of the testing framework - this will be > changed completely for the production version.) Ah, right, of course. http://codereview.chromium.org/9315010/diff/31023/chrome/browser/extensions/a... File chrome/browser/extensions/api/declarative/test_rules_registry.cc (right): http://codereview.chromium.org/9315010/diff/31023/chrome/browser/extensions/a... chrome/browser/extensions/api/declarative/test_rules_registry.cc:71: return "Rule not found."; probably should declare this as kRuleNotFound or something, at the top.
Thanks Ben for the great review. It's highly appreciated. Matt or Aaron, could you please have a (hopefully ;-)) final look? Thanks, Dominic
lgtm
I think all possible comments have been made. LGTM. On Thu, Feb 9, 2012 at 12:33 PM, <mpcomplete@chromium.org> wrote: > lgtm > > > > http://codereview.chromium.org/9315010/
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/battre@chromium.org/9315010/38001
Try job failure for 9315010-38001 (retry) (retry) on linux_rel for step "browser_tests". It's a second try, previously, step "browser_tests" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/battre@chromium.org/9315010/38001
Change committed as 121369 |
