Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1158693006:
Create a mechanism define declarative rules via the extension manifest. (Closed)
|
Created:
5 years, 6 months ago by danduong Modified:
5 years, 6 months ago CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionCreate a mechanism define declarative rules via the extension manifest.
BUG=490339
Committed: https://crrev.com/a79e151dfd2c6e8c402bd0b6c296f8ea8e97b338
Cr-Commit-Position: refs/heads/master@{#334190}
Patch Set 1 #Patch Set 2 : #Patch Set 3 : add test #
Total comments: 22
Patch Set 4 : Cleanup #Patch Set 5 : update manifest api #Patch Set 6 : Prevent removal of rules set in manifest. #
Total comments: 41
Patch Set 7 : #
Total comments: 13
Patch Set 8 : add manifest_rules_ #Patch Set 9 : add declarative_content_apitest #Patch Set 10 : use extension instead of extension_id #Patch Set 11 : some code cleanup #Patch Set 12 : #
Total comments: 40
Patch Set 13 : addressing comments #
Total comments: 1
Patch Set 14 : add a manifest handler #
Total comments: 15
Patch Set 15 : #
Total comments: 7
Patch Set 16 : #Patch Set 17 : #Patch Set 18 : add docs #Patch Set 19 : don't crash #Patch Set 20 : rebase #Patch Set 21 : add thread safety to extensionregistry notifications #Messages
Total messages: 84 (15 generated)
danduong@chromium.org changed reviewers: + kalman@chromium.org, wittman@chromium.org
This needs tests and a bug.
On 2015/06/01 23:22:26, kalman wrote: > This needs tests and a bug. Bug is https://crbug.com/490339. Added.
test added
https://codereview.chromium.org/1158693006/diff/40001/chrome/common/extension... File chrome/common/extensions/api/_manifest_features.json (right): https://codereview.chromium.org/1158693006/diff/40001/chrome/common/extension... chrome/common/extensions/api/_manifest_features.json:99: "declarativeContent": { Use declarative_content to be consistent e.g. browserAction API uses the browser_action manifest key. Though actually I think it should be "declarative_content_rules". https://codereview.chromium.org/1158693006/diff/40001/extensions/browser/api/... File extensions/browser/api/declarative/rules_registry.cc (right): https://codereview.chromium.org/1158693006/diff/40001/extensions/browser/api/... extensions/browser/api/declarative/rules_registry.cc:226: void RulesRegistry::OnExtensionLoaded(const std::string& extension_id) { Could you update OnExtensionUnloaded/Uninstalled/Loaded to pass in an Extension rather than its ID? It's trivial (call sites just grab the ID out of the Extension object they already have access to) and it would save needing to do the extra map lookup + the extensionregistry dependency. https://codereview.chromium.org/1158693006/diff/40001/extensions/browser/api/... extensions/browser/api/declarative/rules_registry.cc:231: // If no rules have been registered yet, check if there are any rules Why only if there are no rules registered yet? It seems a perfectly reasonable use case to have a set of rules in the manifest, with a dynamic set to add later. https://codereview.chromium.org/1158693006/diff/40001/extensions/browser/api/... File extensions/browser/api/declarative/rules_registry_unittest.cc (right): https://codereview.chromium.org/1158693006/diff/40001/extensions/browser/api/... extensions/browser/api/declarative/rules_registry_unittest.cc:33: RulesRegistryTest() {} Empty constructor isn't necessary, but this needs a destructor. However, you can also do "using RulesRegistryTest = ExtensionsTest" to have the same namespacing advantages, I think. https://codereview.chromium.org/1158693006/diff/40001/extensions/browser/api/... extensions/browser/api/declarative/rules_registry_unittest.cc:187: (base::DictionaryValue*)ParseJson( Use C++ style static_cast<>, not C style cast. However in this case you can just used ParseDictionary from api_test_utils. I also thought there was a version somewhere which let you use single quotes, in the //extensions directory perhaps, maybe you can find it. https://codereview.chromium.org/1158693006/diff/40001/extensions/browser/api/... extensions/browser/api/declarative/rules_registry_unittest.cc:196: " \"instanceType\": \"declarativeContent.ShowPageAction\"" This is a bit of an awkward API. Better to try to make it match the public-facing API rather than the internal API that we use. For example, I'd expect this to be: "declarative_content_rules": [{ "conditions": [ "declarativeContent.PageStateMatcher": { "css": ["video"] } ], "actions": ["declarativeContent.ShowPageAction"] }] https://codereview.chromium.org/1158693006/diff/40001/extensions/browser/api/... extensions/browser/api/declarative/rules_registry_unittest.cc:218: content::BrowserThread::UI, NULL, key); nullptr not NULL - though I think that if you remove the BrowserContext dependency from RulesRegistry then you could use the simplified TestRulesRegistry constructor anyway. https://codereview.chromium.org/1158693006/diff/40001/extensions/browser/api/... extensions/browser/api/declarative/rules_registry_unittest.cc:225: ASSERT_EQ(1u, get_rules.size()); Could you assert that this rule matches the one constructed?
https://codereview.chromium.org/1158693006/diff/40001/extensions/browser/api/... File extensions/browser/api/declarative/rules_registry_unittest.cc (right): https://codereview.chromium.org/1158693006/diff/40001/extensions/browser/api/... extensions/browser/api/declarative/rules_registry_unittest.cc:181: TEST_F(RulesRegistryTest, ExtensionWithRulesInManifest) { Can you add tests for handling and reporting of errors in the rules specified in the manifest as well? https://codereview.chromium.org/1158693006/diff/40001/extensions/browser/api/... extensions/browser/api/declarative/rules_registry_unittest.cc:216: scoped_refptr<RulesRegistry> registry = new TestRulesRegistry( I'm not sure why this stored in a scoped_refptr in the other tests. I think this can be allocated on the stack, or stored in a plain scoped_ptr if you want to ensure it's only accessed via the RulesRegistry interface.
https://codereview.chromium.org/1158693006/diff/40001/extensions/browser/api/... File extensions/browser/api/declarative/rules_registry_unittest.cc (right): https://codereview.chromium.org/1158693006/diff/40001/extensions/browser/api/... extensions/browser/api/declarative/rules_registry_unittest.cc:216: scoped_refptr<RulesRegistry> registry = new TestRulesRegistry( On 2015/06/02 23:11:59, Mike Wittman wrote: > I'm not sure why this stored in a scoped_refptr in the other tests. I think this > can be allocated on the stack, or stored in a plain scoped_ptr if you want to > ensure it's only accessed via the RulesRegistry interface. TestRulesRegistry --> RulesRegistry --> RefCountedThreadSafe :-(
https://codereview.chromium.org/1158693006/diff/40001/extensions/browser/api/... File extensions/browser/api/declarative/rules_registry_unittest.cc (right): https://codereview.chromium.org/1158693006/diff/40001/extensions/browser/api/... extensions/browser/api/declarative/rules_registry_unittest.cc:216: scoped_refptr<RulesRegistry> registry = new TestRulesRegistry( On 2015/06/02 23:14:40, kalman wrote: > On 2015/06/02 23:11:59, Mike Wittman wrote: > > I'm not sure why this stored in a scoped_refptr in the other tests. I think > this > > can be allocated on the stack, or stored in a plain scoped_ptr if you want to > > ensure it's only accessed via the RulesRegistry interface. > > TestRulesRegistry --> RulesRegistry --> RefCountedThreadSafe > > :-( Ah, should have known.
Addressed a few of the comments. I'm still missing some tests for bad rule definitions and reformatting the API as I have some questions in-line. https://codereview.chromium.org/1158693006/diff/40001/extensions/browser/api/... File extensions/browser/api/declarative/rules_registry_unittest.cc (right): https://codereview.chromium.org/1158693006/diff/40001/extensions/browser/api/... extensions/browser/api/declarative/rules_registry_unittest.cc:33: RulesRegistryTest() {} On 2015/06/02 22:44:50, kalman wrote: > Empty constructor isn't necessary, but this needs a destructor. > > However, you can also do "using RulesRegistryTest = ExtensionsTest" to have the > same namespacing advantages, I think. Done. https://codereview.chromium.org/1158693006/diff/40001/extensions/browser/api/... extensions/browser/api/declarative/rules_registry_unittest.cc:187: (base::DictionaryValue*)ParseJson( On 2015/06/02 22:44:50, kalman wrote: > Use C++ style static_cast<>, not C style cast. > > However in this case you can just used ParseDictionary from api_test_utils. I > also thought there was a version somewhere which let you use single quotes, in > the //extensions directory perhaps, maybe you can find it. Done. https://codereview.chromium.org/1158693006/diff/40001/extensions/browser/api/... extensions/browser/api/declarative/rules_registry_unittest.cc:196: " \"instanceType\": \"declarativeContent.ShowPageAction\"" On 2015/06/02 22:44:50, kalman wrote: > This is a bit of an awkward API. Better to try to make it match the > public-facing API rather than the internal API that we use. > > For example, I'd expect this to be: > > "declarative_content_rules": [{ > "conditions": [ > "declarativeContent.PageStateMatcher": { > "css": ["video"] > } > ], > "actions": ["declarativeContent.ShowPageAction"] > }] Where should we specify onPageChanged? How should we support declarativeWebRequest as well? https://codereview.chromium.org/1158693006/diff/40001/extensions/browser/api/... extensions/browser/api/declarative/rules_registry_unittest.cc:218: content::BrowserThread::UI, NULL, key); On 2015/06/02 22:44:50, kalman wrote: > nullptr not NULL - though I think that if you remove the BrowserContext > dependency from RulesRegistry then you could use the simplified > TestRulesRegistry constructor anyway. Done. https://codereview.chromium.org/1158693006/diff/40001/extensions/browser/api/... extensions/browser/api/declarative/rules_registry_unittest.cc:225: ASSERT_EQ(1u, get_rules.size()); On 2015/06/02 22:44:50, kalman wrote: > Could you assert that this rule matches the one constructed? Done.
https://codereview.chromium.org/1158693006/diff/40001/extensions/browser/api/... File extensions/browser/api/declarative/rules_registry_unittest.cc (right): https://codereview.chromium.org/1158693006/diff/40001/extensions/browser/api/... extensions/browser/api/declarative/rules_registry_unittest.cc:196: " \"instanceType\": \"declarativeContent.ShowPageAction\"" On 2015/06/03 00:18:06, danduong wrote: > On 2015/06/02 22:44:50, kalman wrote: > > This is a bit of an awkward API. Better to try to make it match the > > public-facing API rather than the internal API that we use. > > > > For example, I'd expect this to be: > > > > "declarative_content_rules": [{ > > "conditions": [ > > "declarativeContent.PageStateMatcher": { > > "css": ["video"] > > } > > ], > > "actions": ["declarativeContent.ShowPageAction"] > > }] > > Where should we specify onPageChanged? How should we support > declarativeWebRequest as well? Oh right, I suppose the event declaration syntax should specify that. I'll try again: "declarative_events": [{ "name": "declarativeContent.OnPageChanged", "conditions": [ "declarativeContent.PageStateMatcher": { "css": ["video"] } ], "actions": ["declarativeContent.ShowPageAction"] }]
https://codereview.chromium.org/1158693006/diff/40001/extensions/browser/api/... File extensions/browser/api/declarative/rules_registry_unittest.cc (right): https://codereview.chromium.org/1158693006/diff/40001/extensions/browser/api/... extensions/browser/api/declarative/rules_registry_unittest.cc:196: " \"instanceType\": \"declarativeContent.ShowPageAction\"" On 2015/06/03 16:21:44, kalman wrote: > On 2015/06/03 00:18:06, danduong wrote: > > On 2015/06/02 22:44:50, kalman wrote: > > > This is a bit of an awkward API. Better to try to make it match the > > > public-facing API rather than the internal API that we use. > > > > > > For example, I'd expect this to be: > > > > > > "declarative_content_rules": [{ > > > "conditions": [ > > > "declarativeContent.PageStateMatcher": { > > > "css": ["video"] > > > } > > > ], > > > "actions": ["declarativeContent.ShowPageAction"] > > > }] > > > > Where should we specify onPageChanged? How should we support > > declarativeWebRequest as well? > > Oh right, I suppose the event declaration syntax should specify that. I'll try > again: > > "declarative_events": [{ > "name": "declarativeContent.OnPageChanged", > "conditions": [ > "declarativeContent.PageStateMatcher": { > "css": ["video"] > } > ], > "actions": ["declarativeContent.ShowPageAction"] > }] I don't think that's valid JSON. Do you mean: "conditions": [ { "declarativeContent.PageStateMatcher": { "css": ["video"] } } ], If this is the case, the object nesting seems a little awkward. It is saying that an individual condition could have a PageStateMatcher and potentially another kind of matcher, but not 2 page state matchers. instanceType flattens this out, but we can switch to instance_type if we prefer underscores in manifest. Do we expect this transformation universally? e.g. does pageUrl: { hostEquals: 'www.google.com', schemes: ['https'] }, become page_url: { 'host_equals': 'www.google.com', 'schemes: ['https']},
On 2015/06/03 16:45:53, danduong wrote: > https://codereview.chromium.org/1158693006/diff/40001/extensions/browser/api/... > File extensions/browser/api/declarative/rules_registry_unittest.cc (right): > > https://codereview.chromium.org/1158693006/diff/40001/extensions/browser/api/... > extensions/browser/api/declarative/rules_registry_unittest.cc:196: " > \"instanceType\": \"declarativeContent.ShowPageAction\"" > On 2015/06/03 16:21:44, kalman wrote: > > On 2015/06/03 00:18:06, danduong wrote: > > > On 2015/06/02 22:44:50, kalman wrote: > > > > This is a bit of an awkward API. Better to try to make it match the > > > > public-facing API rather than the internal API that we use. > > > > > > > > For example, I'd expect this to be: > > > > > > > > "declarative_content_rules": [{ > > > > "conditions": [ > > > > "declarativeContent.PageStateMatcher": { > > > > "css": ["video"] > > > > } > > > > ], > > > > "actions": ["declarativeContent.ShowPageAction"] > > > > }] > > > > > > Where should we specify onPageChanged? How should we support > > > declarativeWebRequest as well? > > > > Oh right, I suppose the event declaration syntax should specify that. I'll try > > again: > > > > "declarative_events": [{ > > "name": "declarativeContent.OnPageChanged", > > "conditions": [ > > "declarativeContent.PageStateMatcher": { > > "css": ["video"] > > } > > ], > > "actions": ["declarativeContent.ShowPageAction"] > > }] > > I don't think that's valid JSON. Do you mean: > > "conditions": [ > { > "declarativeContent.PageStateMatcher": { > "css": ["video"] > } > } > ], > > If this is the case, the object nesting seems a little awkward. It is saying > that an individual condition could have a PageStateMatcher and potentially > another kind of matcher, but not 2 page state matchers. > > instanceType flattens this out, but we can switch to instance_type if we prefer > underscores in manifest. Do we expect this transformation universally? > > e.g. does > pageUrl: { hostEquals: 'www.google.com', schemes: ['https'] }, > become > page_url: { 'host_equals': 'www.google.com', 'schemes: ['https']}, Mm yeah ok. Well my invalid JSON aside I just want it to match how it looks in the API (or at least, a nice-looking API), which isn't necessarily how it's stored in prefs. I'm thinking in particular the "instanceType" keyword doesn't look great. So, we could start with literally transcribing the API call. For example, taking: var declarativeContent = chrome.declarativeContent; declarativeContent.onPageChanged.addRules([{ conditions: [ new declarativeContent.PageStateMatcher({ css: ['video'] } ], actions: [ new declarativeContent.ShowPageAction() ] }, { conditions: [ new declarativeContent.IsBookmarked() ], actions: [ new declarativeContent.SetIcon({ ... }) ] }]); which transposes most literally as: 'declarative_content_rules': [{ 'declarativeContent.onPageChange': { 'conditions': [{ 'declarativeContent.PageStateMatcher': { 'css': ['video'] } }, 'actions': [{ 'declarativeContent.ShowPageAction': {} }] }, { 'declarativeContent.onPageChange': { 'conditions': [{ 'declarativeContent.IsBookmarked': {} }, 'actions': [{ 'declarativeContent.SetIcon': {...} }] }] which does look pretty awkward with the nesting and needing the empty arguments {}. I suppose you could flatten that to: 'declarative_content_rules': [{ 'event': 'declarativeContent.onPageChange', 'conditions': [{ 'declarativeContent.PageStateMatcher': { 'css': ['video'] } }, 'actions': [{ 'declarativeContent.ShowPageAction': {} }] }, { 'event': 'declarativeContent.onPageChange', 'conditions': [{ 'declarativeContent.IsBookmarked': {} }, 'actions': [{ 'declarativeContent.SetIcon': {...} }] }] which is still awkward, so you flatten even further to: 'declarative_content_rules': [{ 'event': 'declarativeContent.onPageChange', 'conditions': [{ 'type': 'declarativeContent.PageStateMatcher', 'css': ['video'] }], 'actions': [{ 'type': 'declarativeContent.ShowPageAction' }] }, { 'event': 'declarativeContent.onPageChange', 'conditions': [{ 'type': 'declarativeContent.IsBookmarked' }], 'actions': [{ 'type': 'declarativeContent.SetIcon', ... }] }] which seems pretty good. Is that more or less what you're suggesting, modulo using 'instanceType'?
On 2015/06/03 18:14:21, kalman wrote: > On 2015/06/03 16:45:53, danduong wrote: > > > https://codereview.chromium.org/1158693006/diff/40001/extensions/browser/api/... > > File extensions/browser/api/declarative/rules_registry_unittest.cc (right): > > > > > https://codereview.chromium.org/1158693006/diff/40001/extensions/browser/api/... > > extensions/browser/api/declarative/rules_registry_unittest.cc:196: " > > > \"instanceType\": \"declarativeContent.ShowPageAction\"" > > On 2015/06/03 16:21:44, kalman wrote: > > > On 2015/06/03 00:18:06, danduong wrote: > > > > On 2015/06/02 22:44:50, kalman wrote: > > > > > This is a bit of an awkward API. Better to try to make it match the > > > > > public-facing API rather than the internal API that we use. > > > > > > > > > > For example, I'd expect this to be: > > > > > > > > > > "declarative_content_rules": [{ > > > > > "conditions": [ > > > > > "declarativeContent.PageStateMatcher": { > > > > > "css": ["video"] > > > > > } > > > > > ], > > > > > "actions": ["declarativeContent.ShowPageAction"] > > > > > }] > > > > > > > > Where should we specify onPageChanged? How should we support > > > > declarativeWebRequest as well? > > > > > > Oh right, I suppose the event declaration syntax should specify that. I'll > try > > > again: > > > > > > "declarative_events": [{ > > > "name": "declarativeContent.OnPageChanged", > > > "conditions": [ > > > "declarativeContent.PageStateMatcher": { > > > "css": ["video"] > > > } > > > ], > > > "actions": ["declarativeContent.ShowPageAction"] > > > }] > > > > I don't think that's valid JSON. Do you mean: > > > > "conditions": [ > > { > > "declarativeContent.PageStateMatcher": { > > "css": ["video"] > > } > > } > > ], > > > > If this is the case, the object nesting seems a little awkward. It is saying > > that an individual condition could have a PageStateMatcher and potentially > > another kind of matcher, but not 2 page state matchers. > > > > instanceType flattens this out, but we can switch to instance_type if we > prefer > > underscores in manifest. Do we expect this transformation universally? > > > > e.g. does > > pageUrl: { hostEquals: 'www.google.com', schemes: ['https'] }, > > become > > page_url: { 'host_equals': 'www.google.com', 'schemes: ['https']}, > > Mm yeah ok. Well my invalid JSON aside I just want it to match how it looks in > the API (or at least, a nice-looking API), which isn't necessarily how it's > stored in prefs. I'm thinking in particular the "instanceType" keyword doesn't > look great. > > So, we could start with literally transcribing the API call. For example, > taking: > > var declarativeContent = chrome.declarativeContent; > declarativeContent.onPageChanged.addRules([{ > conditions: [ > new declarativeContent.PageStateMatcher({ > css: ['video'] > } > ], > actions: [ > new declarativeContent.ShowPageAction() > ] > }, { > conditions: [ > new declarativeContent.IsBookmarked() > ], > actions: [ > new declarativeContent.SetIcon({ ... }) > ] > }]); > > which transposes most literally as: > > 'declarative_content_rules': [{ > 'declarativeContent.onPageChange': { > 'conditions': [{ > 'declarativeContent.PageStateMatcher': { > 'css': ['video'] > } > }, > 'actions': [{ > 'declarativeContent.ShowPageAction': {} > }] > }, { > 'declarativeContent.onPageChange': { > 'conditions': [{ > 'declarativeContent.IsBookmarked': {} > }, > 'actions': [{ > 'declarativeContent.SetIcon': {...} > }] > }] > > which does look pretty awkward with the nesting and needing the empty arguments > {}. I suppose you could flatten that to: > > 'declarative_content_rules': [{ > 'event': 'declarativeContent.onPageChange', > 'conditions': [{ > 'declarativeContent.PageStateMatcher': { > 'css': ['video'] > } > }, > 'actions': [{ > 'declarativeContent.ShowPageAction': {} > }] > }, { > 'event': 'declarativeContent.onPageChange', > 'conditions': [{ > 'declarativeContent.IsBookmarked': {} > }, > 'actions': [{ > 'declarativeContent.SetIcon': {...} > }] > }] > > which is still awkward, so you flatten even further to: > > 'declarative_content_rules': [{ > 'event': 'declarativeContent.onPageChange', > 'conditions': [{ > 'type': 'declarativeContent.PageStateMatcher', > 'css': ['video'] > }], > 'actions': [{ > 'type': 'declarativeContent.ShowPageAction' > }] > }, { > 'event': 'declarativeContent.onPageChange', > 'conditions': [{ > 'type': 'declarativeContent.IsBookmarked' > }], > 'actions': [{ > 'type': 'declarativeContent.SetIcon', > ... > }] > }] > > which seems pretty good. Is that more or less what you're suggesting, modulo > using 'instanceType'? That looks good to me. With regards to the rules, does this look right to you? 'conditions': [{ 'type': 'declarativeContent.PageStateMatcher', 'pageUrl': { 'hostEquals': 'www.google.com', 'schemes': ['https'] }, 'css': ["input[type='password']"] }]
Updated the manifest api. - Still need some more test coverage. - And make the manifest rules not removable.
OK, PTAL. I added a mechanism to prevent the programmatic removal of event rules and some testing around this area.
https://codereview.chromium.org/1158693006/diff/100001/chrome/common/extensio... File chrome/common/extensions/api/_manifest_features.json (right): https://codereview.chromium.org/1158693006/diff/100001/chrome/common/extensio... chrome/common/extensions/api/_manifest_features.json:100: "channel": "stable", Note to readers (no action needed): I don't think it's necessary to start this in dev channel, it doesn't grant capabilities. https://codereview.chromium.org/1158693006/diff/100001/chrome/common/extensio... chrome/common/extensions/api/_manifest_features.json:101: "extension_types": "all" "all" is scary, but the right thing in this case. Could you add a comment here: // "event_rules" does not grant any capabilities, it's just // an optimisation for any API which uses events, so it's safe // to expose to all extension types. "extension_types": "all" https://codereview.chromium.org/1158693006/diff/100001/extensions/browser/api... File extensions/browser/api/declarative/rules_registry.cc (right): https://codereview.chromium.org/1158693006/diff/100001/extensions/browser/api... extensions/browser/api/declarative/rules_registry.cc:82: std::vector<linked_ptr<extensions::RulesRegistry::Rule>> RulesFromManifest( - This file is already in the extensions namespace, so no need for extensions::. - You can return a ScopedVector<RulesRegistry::Rule> from here, because you can .Pass() them. https://codereview.chromium.org/1158693006/diff/100001/extensions/browser/api... extensions/browser/api/declarative/rules_registry.cc:91: for (size_t i = 0; i < list->GetSize(); ++i) { You should look at defining this structure in extensions_manifest_types.json, which will generate much of this parsing for you. See how OptionsUI is implemented, for example. https://codereview.chromium.org/1158693006/diff/100001/extensions/browser/api... extensions/browser/api/declarative/rules_registry.cc:212: return "List contains non-removable rules"; I'd prefer to define this as a constant. https://codereview.chromium.org/1158693006/diff/100001/extensions/browser/api... extensions/browser/api/declarative/rules_registry.cc:299: void RulesRegistry::OnExtensionLoaded(const std::string& extension_id) { Repeat previous comment: Could you update OnExtensionUnloaded/Uninstalled/Loaded to pass in an Extension rather than its ID? It's trivial (call sites just grab the ID out of the Extension object they already have access to) and it would save needing to do the extra map lookup + the extensionregistry dependency. https://codereview.chromium.org/1158693006/diff/100001/extensions/browser/api... extensions/browser/api/declarative/rules_registry.cc:303: if (rules.empty()) { Repeat previous comment: Why only if there are no rules registered yet? It seems a perfectly reasonable use case to have a set of rules in the manifest, with a dynamic set to add later. https://codereview.chromium.org/1158693006/diff/100001/extensions/browser/api... extensions/browser/api/declarative/rules_registry.cc:308: ->GetExtensionById(extension_id, ExtensionRegistry::EVERYTHING); Btw once https://codereview.chromium.org/1130353010 goes in you'll be able to write "GetInstalledExtension" instead of "... EVERYTHING". https://codereview.chromium.org/1158693006/diff/100001/extensions/browser/api... File extensions/browser/api/declarative/rules_registry_unittest.cc (right): https://codereview.chromium.org/1158693006/diff/100001/extensions/browser/api... extensions/browser/api/declarative/rules_registry_unittest.cc:226: const base::DictionaryValue* action; it's nice to initialise pointers to null. https://codereview.chromium.org/1158693006/diff/100001/extensions/browser/api... extensions/browser/api/declarative/rules_registry_unittest.cc:227: ASSERT_EQ(true, get_rules[0]->actions[0]->GetAsDictionary(&action)); ASSERT_TRUE https://codereview.chromium.org/1158693006/diff/100001/extensions/browser/api... extensions/browser/api/declarative/rules_registry_unittest.cc:253: ASSERT_EQ("input[type='password']", css_value); You can have a much easier time here defining the expected structure like scoped_ptr<base::Dictionary> expected_first_rule = ParseDictionary( "{\"actions\": ...}, {\"conditions\": ...})"); scoped_ptr<base::Dictionary> expected_second_rule = ...; then pulling out each individually and using Value::Equals or whatever. https://codereview.chromium.org/1158693006/diff/100001/extensions/browser/api... extensions/browser/api/declarative/rules_registry_unittest.cc:319: Test somewhere around here what happens if you try to add rules programmatically with some rules declared; it looks to me like that's what you're indirectly testing (?) but it also looks like you've written code to prevent that. https://codereview.chromium.org/1158693006/diff/100001/extensions/common/api/... File extensions/common/api/events.json (right): https://codereview.chromium.org/1158693006/diff/100001/extensions/common/api/... extensions/common/api/events.json:23: "removable": { This shouldn't be part of the public API, instead, annotate it in the internal data structure you use.
Can you also add at least one API test to declarative_content_apitest.cc to test the full implementation? https://codereview.chromium.org/1158693006/diff/100001/extensions/browser/api... File extensions/browser/api/declarative/rules_registry.cc (right): https://codereview.chromium.org/1158693006/diff/100001/extensions/browser/api... extensions/browser/api/declarative/rules_registry.cc:311: AddRules(extension_id, RulesFromManifest(value, event_name_)); this needs to handle the error which may be returned, and propagate it to the extension's error log https://codereview.chromium.org/1158693006/diff/100001/extensions/browser/api... File extensions/browser/api/declarative/rules_registry_unittest.cc (right): https://codereview.chromium.org/1158693006/diff/100001/extensions/browser/api... extensions/browser/api/declarative/rules_registry_unittest.cc:177: TEST_F(RulesRegistryTest, TwoRulesInManifest) { Can you add a comment before each test describing what it's testing? https://codereview.chromium.org/1158693006/diff/100001/extensions/browser/api... extensions/browser/api/declarative/rules_registry_unittest.cc:235: ASSERT_EQ("declarativeContent.PageStateMatcher", instance_type); Checks that can fail without causing the test to crash should use EXPECT rather than ASSERT.
Still in progress. Please respond to some of my comments. https://codereview.chromium.org/1158693006/diff/100001/chrome/common/extensio... File chrome/common/extensions/api/_manifest_features.json (right): https://codereview.chromium.org/1158693006/diff/100001/chrome/common/extensio... chrome/common/extensions/api/_manifest_features.json:101: "extension_types": "all" On 2015/06/04 17:58:48, kalman wrote: > "all" is scary, but the right thing in this case. Could you add a comment here: > > // "event_rules" does not grant any capabilities, it's just > // an optimisation for any API which uses events, so it's safe > // to expose to all extension types. > "extension_types": "all" Done. https://codereview.chromium.org/1158693006/diff/100001/extensions/browser/api... File extensions/browser/api/declarative/rules_registry.cc (right): https://codereview.chromium.org/1158693006/diff/100001/extensions/browser/api... extensions/browser/api/declarative/rules_registry.cc:82: std::vector<linked_ptr<extensions::RulesRegistry::Rule>> RulesFromManifest( On 2015/06/04 17:58:49, kalman wrote: > - This file is already in the extensions namespace, so no need for extensions::. > - You can return a ScopedVector<RulesRegistry::Rule> from here, because you can > .Pass() them. - All these helper functions are in anonymous namespace. Should I move them into extensions? - The receiving function AddRules is expecting a vector<linked_ptr<Registry::Rule>> how would I reconcile that? https://codereview.chromium.org/1158693006/diff/100001/extensions/browser/api... extensions/browser/api/declarative/rules_registry.cc:212: return "List contains non-removable rules"; On 2015/06/04 17:58:48, kalman wrote: > I'd prefer to define this as a constant. Done. https://codereview.chromium.org/1158693006/diff/100001/extensions/browser/api... extensions/browser/api/declarative/rules_registry.cc:303: if (rules.empty()) { On 2015/06/04 17:58:48, kalman wrote: > Repeat previous comment: > > Why only if there are no rules registered yet? It seems a perfectly reasonable > use case to have a set of rules in the manifest, with a dynamic set to add > later. Isn't that use case still met, since the complete set of rules is persisted to storage? On first load after install, the manifest rules will be added and persisted. The reason why I put it here vs. an OnInstall is it makes adding manifest extensions a little more robust just in-case we miss an onInstall event for whatever reason. https://codereview.chromium.org/1158693006/diff/100001/extensions/browser/api... extensions/browser/api/declarative/rules_registry.cc:308: ->GetExtensionById(extension_id, ExtensionRegistry::EVERYTHING); On 2015/06/04 17:58:49, kalman wrote: > Btw once https://codereview.chromium.org/1130353010 goes in you'll be able to > write "GetInstalledExtension" instead of "... EVERYTHING". Acknowledged. https://codereview.chromium.org/1158693006/diff/100001/extensions/browser/api... extensions/browser/api/declarative/rules_registry.cc:311: AddRules(extension_id, RulesFromManifest(value, event_name_)); On 2015/06/04 18:40:13, Mike Wittman wrote: > this needs to handle the error which may be returned, and propagate it to the > extension's error log Done. https://codereview.chromium.org/1158693006/diff/100001/extensions/browser/api... File extensions/browser/api/declarative/rules_registry_unittest.cc (right): https://codereview.chromium.org/1158693006/diff/100001/extensions/browser/api... extensions/browser/api/declarative/rules_registry_unittest.cc:226: const base::DictionaryValue* action; On 2015/06/04 17:58:49, kalman wrote: > it's nice to initialise pointers to null. Done. https://codereview.chromium.org/1158693006/diff/100001/extensions/browser/api... extensions/browser/api/declarative/rules_registry_unittest.cc:227: ASSERT_EQ(true, get_rules[0]->actions[0]->GetAsDictionary(&action)); On 2015/06/04 17:58:49, kalman wrote: > ASSERT_TRUE Done. https://codereview.chromium.org/1158693006/diff/100001/extensions/browser/api... extensions/browser/api/declarative/rules_registry_unittest.cc:319: On 2015/06/04 17:58:49, kalman wrote: > Test somewhere around here what happens if you try to add rules programmatically > with some rules declared; it looks to me like that's what you're indirectly > testing (?) but it also looks like you've written code to prevent that. I believe that is tested unless you mean something else. 297: removes a rule that was added programmatically. 305: tries to remove a rule that was added in the manifest and expectedly failes. https://codereview.chromium.org/1158693006/diff/100001/extensions/common/api/... File extensions/common/api/events.json (right): https://codereview.chromium.org/1158693006/diff/100001/extensions/common/api/... extensions/common/api/events.json:23: "removable": { On 2015/06/04 17:58:49, kalman wrote: > This shouldn't be part of the public API, instead, annotate it in the internal > data structure you use. Which internal data-structure. Part of the reason why it is here is because how the serialization to storage works.
https://codereview.chromium.org/1158693006/diff/100001/extensions/browser/api... File extensions/browser/api/declarative/rules_registry.cc (right): https://codereview.chromium.org/1158693006/diff/100001/extensions/browser/api... extensions/browser/api/declarative/rules_registry.cc:82: std::vector<linked_ptr<extensions::RulesRegistry::Rule>> RulesFromManifest( On 2015/06/04 19:10:59, danduong wrote: > On 2015/06/04 17:58:49, kalman wrote: > > - This file is already in the extensions namespace, so no need for > extensions::. > > - You can return a ScopedVector<RulesRegistry::Rule> from here, because you > can > > .Pass() them. > > - All these helper functions are in anonymous namespace. Should I move them > into extensions? > - The receiving function AddRules is expecting a > vector<linked_ptr<Registry::Rule>> how would I reconcile that? I see. Move the "namespace extensions {" declaration to the top of the file. https://codereview.chromium.org/1158693006/diff/100001/extensions/browser/api... extensions/browser/api/declarative/rules_registry.cc:303: if (rules.empty()) { On 2015/06/04 19:10:59, danduong wrote: > On 2015/06/04 17:58:48, kalman wrote: > > Repeat previous comment: > > > > Why only if there are no rules registered yet? It seems a perfectly reasonable > > use case to have a set of rules in the manifest, with a dynamic set to add > > later. > > Isn't that use case still met, since the complete set of rules is persisted to > storage? On first load after install, the manifest rules will be added and > persisted. The reason why I put it here vs. an OnInstall is it makes adding > manifest extensions a little more robust just in-case we miss an onInstall event > for whatever reason. I think this won't work for update, though. There might already be rules and when the extension updates and adds a new rule, it won't be updated. Basically I don't think you can implement this by writing the rules to disk, plus it's wasteful to be duplicating them. https://codereview.chromium.org/1158693006/diff/100001/extensions/common/api/... File extensions/common/api/events.json (right): https://codereview.chromium.org/1158693006/diff/100001/extensions/common/api/... extensions/common/api/events.json:23: "removable": { On 2015/06/04 19:10:59, danduong wrote: > On 2015/06/04 17:58:49, kalman wrote: > > This shouldn't be part of the public API, instead, annotate it in the internal > > data structure you use. > > Which internal data-structure. Part of the reason why it is here is because how > the serialization to storage works. However rules are implemented in C++. you're changing the public API here.
https://codereview.chromium.org/1158693006/diff/100001/chrome/common/extensio... File chrome/common/extensions/api/_manifest_features.json (right): https://codereview.chromium.org/1158693006/diff/100001/chrome/common/extensio... chrome/common/extensions/api/_manifest_features.json:100: "channel": "stable", On 2015/06/04 17:58:48, kalman wrote: > Note to readers (no action needed): I don't think it's necessary to start this > in dev channel, it doesn't grant capabilities. Acknowledged. https://codereview.chromium.org/1158693006/diff/100001/extensions/browser/api... File extensions/browser/api/declarative/rules_registry.cc (right): https://codereview.chromium.org/1158693006/diff/100001/extensions/browser/api... extensions/browser/api/declarative/rules_registry.cc:303: if (rules.empty()) { On 2015/06/04 19:23:53, kalman wrote: > On 2015/06/04 19:10:59, danduong wrote: > > On 2015/06/04 17:58:48, kalman wrote: > > > Repeat previous comment: > > > > > > Why only if there are no rules registered yet? It seems a perfectly > reasonable > > > use case to have a set of rules in the manifest, with a dynamic set to add > > > later. > > > > Isn't that use case still met, since the complete set of rules is persisted to > > storage? On first load after install, the manifest rules will be added and > > persisted. The reason why I put it here vs. an OnInstall is it makes adding > > manifest extensions a little more robust just in-case we miss an onInstall > event > > for whatever reason. > > I think this won't work for update, though. There might already be rules and > when the extension updates and adds a new rule, it won't be updated. > > Basically I don't think you can implement this by writing the rules to disk, > plus it's wasteful to be duplicating them. Update would work still right? Uninstall will remove all rules and start from scratch. After the extension is loaded, programatic rules are added back. I'm concerned about the additional complexity it adds to have some rules persisted and others not. https://codereview.chromium.org/1158693006/diff/100001/extensions/browser/api... File extensions/browser/api/declarative/rules_registry_unittest.cc (right): https://codereview.chromium.org/1158693006/diff/100001/extensions/browser/api... extensions/browser/api/declarative/rules_registry_unittest.cc:177: TEST_F(RulesRegistryTest, TwoRulesInManifest) { On 2015/06/04 18:40:13, Mike Wittman wrote: > Can you add a comment before each test describing what it's testing? Done. https://codereview.chromium.org/1158693006/diff/100001/extensions/browser/api... extensions/browser/api/declarative/rules_registry_unittest.cc:235: ASSERT_EQ("declarativeContent.PageStateMatcher", instance_type); On 2015/06/04 18:40:14, Mike Wittman wrote: > Checks that can fail without causing the test to crash should use EXPECT rather > than ASSERT. Done. https://codereview.chromium.org/1158693006/diff/100001/extensions/browser/api... extensions/browser/api/declarative/rules_registry_unittest.cc:253: ASSERT_EQ("input[type='password']", css_value); On 2015/06/04 17:58:49, kalman wrote: > You can have a much easier time here defining the expected structure like > > scoped_ptr<base::Dictionary> expected_first_rule = ParseDictionary( > "{\"actions\": ...}, > {\"conditions\": ...})"); > scoped_ptr<base::Dictionary> expected_second_rule = ...; > > then pulling out each individually and using Value::Equals or whatever. Done.
https://codereview.chromium.org/1158693006/diff/100001/extensions/browser/api... File extensions/browser/api/declarative/rules_registry.cc (right): https://codereview.chromium.org/1158693006/diff/100001/extensions/browser/api... extensions/browser/api/declarative/rules_registry.cc:303: if (rules.empty()) { On 2015/06/04 19:56:07, danduong wrote: > On 2015/06/04 19:23:53, kalman wrote: > > On 2015/06/04 19:10:59, danduong wrote: > > > On 2015/06/04 17:58:48, kalman wrote: > > > > Repeat previous comment: > > > > > > > > Why only if there are no rules registered yet? It seems a perfectly > > reasonable > > > > use case to have a set of rules in the manifest, with a dynamic set to add > > > > later. > > > > > > Isn't that use case still met, since the complete set of rules is persisted > to > > > storage? On first load after install, the manifest rules will be added and > > > persisted. The reason why I put it here vs. an OnInstall is it makes adding > > > manifest extensions a little more robust just in-case we miss an onInstall > > event > > > for whatever reason. > > > > I think this won't work for update, though. There might already be rules and > > when the extension updates and adds a new rule, it won't be updated. > > > > Basically I don't think you can implement this by writing the rules to disk, > > plus it's wasteful to be duplicating them. > > Update would work still right? Uninstall will remove all rules and start from > scratch. After the extension is loaded, programatic rules are added back. I'm > concerned about the additional complexity it adds to have some rules persisted > and others not. Maybe. Either way this code looks wrong. I would hope the only changes necessary would be to have 2 sets of rules, one you read from disk, the other from the manifest.
On 2015/06/04 20:02:21, kalman wrote: > https://codereview.chromium.org/1158693006/diff/100001/extensions/browser/api... > File extensions/browser/api/declarative/rules_registry.cc (right): > > https://codereview.chromium.org/1158693006/diff/100001/extensions/browser/api... > extensions/browser/api/declarative/rules_registry.cc:303: if (rules.empty()) { > On 2015/06/04 19:56:07, danduong wrote: > > On 2015/06/04 19:23:53, kalman wrote: > > > On 2015/06/04 19:10:59, danduong wrote: > > > > On 2015/06/04 17:58:48, kalman wrote: > > > > > Repeat previous comment: > > > > > > > > > > Why only if there are no rules registered yet? It seems a perfectly > > > reasonable > > > > > use case to have a set of rules in the manifest, with a dynamic set to > add > > > > > later. > > > > > > > > Isn't that use case still met, since the complete set of rules is > persisted > > to > > > > storage? On first load after install, the manifest rules will be added and > > > > persisted. The reason why I put it here vs. an OnInstall is it makes > adding > > > > manifest extensions a little more robust just in-case we miss an onInstall > > > event > > > > for whatever reason. > > > > > > I think this won't work for update, though. There might already be rules and > > > when the extension updates and adds a new rule, it won't be updated. > > > > > > Basically I don't think you can implement this by writing the rules to disk, > > > plus it's wasteful to be duplicating them. > > > > Update would work still right? Uninstall will remove all rules and start from > > scratch. After the extension is loaded, programatic rules are added back. I'm > > concerned about the additional complexity it adds to have some rules persisted > > and others not. > > Maybe. Either way this code looks wrong. I would hope the only changes necessary > would be to have 2 sets of rules, one you read from disk, the other from the > manifest. by rules I mean rules, not registries - like have |rules_| and |manifest_rules_|.
kalman@ let me know if this is what you were thinking about not persisting manifest based rules. https://codereview.chromium.org/1158693006/diff/100001/extensions/browser/api... File extensions/browser/api/declarative/rules_registry.cc (right): https://codereview.chromium.org/1158693006/diff/100001/extensions/browser/api... extensions/browser/api/declarative/rules_registry.cc:82: std::vector<linked_ptr<extensions::RulesRegistry::Rule>> RulesFromManifest( On 2015/06/04 19:23:52, kalman wrote: > On 2015/06/04 19:10:59, danduong wrote: > > On 2015/06/04 17:58:49, kalman wrote: > > > - This file is already in the extensions namespace, so no need for > > extensions::. > > > - You can return a ScopedVector<RulesRegistry::Rule> from here, because you > > can > > > .Pass() them. > > > > - All these helper functions are in anonymous namespace. Should I move them > > into extensions? > > - The receiving function AddRules is expecting a > > vector<linked_ptr<Registry::Rule>> how would I reconcile that? > > I see. Move the "namespace extensions {" declaration to the top of the file. Done. https://codereview.chromium.org/1158693006/diff/100001/extensions/browser/api... extensions/browser/api/declarative/rules_registry.cc:303: if (rules.empty()) { On 2015/06/04 20:02:21, kalman wrote: > On 2015/06/04 19:56:07, danduong wrote: > > On 2015/06/04 19:23:53, kalman wrote: > > > On 2015/06/04 19:10:59, danduong wrote: > > > > On 2015/06/04 17:58:48, kalman wrote: > > > > > Repeat previous comment: > > > > > > > > > > Why only if there are no rules registered yet? It seems a perfectly > > > reasonable > > > > > use case to have a set of rules in the manifest, with a dynamic set to > add > > > > > later. > > > > > > > > Isn't that use case still met, since the complete set of rules is > persisted > > to > > > > storage? On first load after install, the manifest rules will be added and > > > > persisted. The reason why I put it here vs. an OnInstall is it makes > adding > > > > manifest extensions a little more robust just in-case we miss an onInstall > > > event > > > > for whatever reason. > > > > > > I think this won't work for update, though. There might already be rules and > > > when the extension updates and adds a new rule, it won't be updated. > > > > > > Basically I don't think you can implement this by writing the rules to disk, > > > plus it's wasteful to be duplicating them. > > > > Update would work still right? Uninstall will remove all rules and start from > > scratch. After the extension is loaded, programatic rules are added back. I'm > > concerned about the additional complexity it adds to have some rules persisted > > and others not. > > Maybe. Either way this code looks wrong. I would hope the only changes necessary > would be to have 2 sets of rules, one you read from disk, the other from the > manifest. Done. https://codereview.chromium.org/1158693006/diff/100001/extensions/common/api/... File extensions/common/api/events.json (right): https://codereview.chromium.org/1158693006/diff/100001/extensions/common/api/... extensions/common/api/events.json:23: "removable": { On 2015/06/04 19:23:53, kalman wrote: > On 2015/06/04 19:10:59, danduong wrote: > > On 2015/06/04 17:58:49, kalman wrote: > > > This shouldn't be part of the public API, instead, annotate it in the > internal > > > data structure you use. > > > > Which internal data-structure. Part of the reason why it is here is because > how > > the serialization to storage works. > > However rules are implemented in C++. you're changing the public API here. Done.
not really, you're still persisting the list of rules https://codereview.chromium.org/1158693006/diff/120001/extensions/browser/api... File extensions/browser/api/declarative/rules_registry.cc (right): https://codereview.chromium.org/1158693006/diff/120001/extensions/browser/api... extensions/browser/api/declarative/rules_registry.cc:278: i != rules_.end(); ++i) { here and in GetRules, augment the list you return with the manifest rules https://codereview.chromium.org/1158693006/diff/120001/extensions/browser/api... File extensions/browser/api/declarative/rules_registry.h (right): https://codereview.chromium.org/1158693006/diff/120001/extensions/browser/api... extensions/browser/api/declarative/rules_registry.h:80: bool from_manifest = false); default parameters are against style guide
https://codereview.chromium.org/1158693006/diff/120001/extensions/browser/api... File extensions/browser/api/declarative/rules_registry.cc (right): https://codereview.chromium.org/1158693006/diff/120001/extensions/browser/api... extensions/browser/api/declarative/rules_registry.cc:278: i != rules_.end(); ++i) { On 2015/06/04 22:00:50, kalman wrote: > here and in GetRules, augment the list you return with the manifest rules where manifest rules are a different set of rules. don't put them through AddRules.
https://codereview.chromium.org/1158693006/diff/120001/extensions/browser/api... File extensions/browser/api/declarative/rules_registry.cc (right): https://codereview.chromium.org/1158693006/diff/120001/extensions/browser/api... extensions/browser/api/declarative/rules_registry.cc:278: i != rules_.end(); ++i) { On 2015/06/04 22:00:50, kalman wrote: > here and in GetRules, augment the list you return with the manifest rules They should already be in there. The manifest rules are still a part of |rules_|
https://codereview.chromium.org/1158693006/diff/120001/extensions/browser/api... File extensions/browser/api/declarative/rules_registry.cc (right): https://codereview.chromium.org/1158693006/diff/120001/extensions/browser/api... extensions/browser/api/declarative/rules_registry.cc:278: i != rules_.end(); ++i) { On 2015/06/04 22:02:33, danduong wrote: > On 2015/06/04 22:00:50, kalman wrote: > > here and in GetRules, augment the list you return with the manifest rules > > They should already be in there. The manifest rules are still a part of |rules_| yes. don't put them in rules.
https://codereview.chromium.org/1158693006/diff/100001/extensions/browser/api... File extensions/browser/api/declarative/rules_registry_unittest.cc (right): https://codereview.chromium.org/1158693006/diff/100001/extensions/browser/api... extensions/browser/api/declarative/rules_registry_unittest.cc:177: TEST_F(RulesRegistryTest, TwoRulesInManifest) { On 2015/06/04 19:56:07, danduong wrote: > On 2015/06/04 18:40:13, Mike Wittman wrote: > > Can you add a comment before each test describing what it's testing? > > Done. convention is to put the comments above the TEST declaration https://codereview.chromium.org/1158693006/diff/120001/extensions/browser/api... File extensions/browser/api/declarative/rules_registry_unittest.cc (right): https://codereview.chromium.org/1158693006/diff/120001/extensions/browser/api... extensions/browser/api/declarative/rules_registry_unittest.cc:296: ASSERT_EQ(3u, get_rules.size()); ASSERTs in this test can all be EXPECTs if you're not looking at the rules https://codereview.chromium.org/1158693006/diff/120001/extensions/browser/api... extensions/browser/api/declarative/rules_registry_unittest.cc:323: // definition in the manifest. ... and the expected behavior is ??? https://codereview.chromium.org/1158693006/diff/120001/extensions/browser/api... extensions/browser/api/declarative/rules_registry_unittest.cc:419: ASSERT_EQ(0u, get_rules.size()); EXPECT_EQ
Still remaining: - Defining a structure in extensions_manifest_types.json - API Test in declarative_content_apitest.cc - OnExtensionUnloaded/Uninstalled/Loaded to pass in an Extension https://codereview.chromium.org/1158693006/diff/40001/chrome/common/extension... File chrome/common/extensions/api/_manifest_features.json (right): https://codereview.chromium.org/1158693006/diff/40001/chrome/common/extension... chrome/common/extensions/api/_manifest_features.json:99: "declarativeContent": { On 2015/06/02 22:44:50, kalman wrote: > Use declarative_content to be consistent e.g. browserAction API uses the > browser_action manifest key. Though actually I think it should be > "declarative_content_rules". Done. Using event_rules. https://codereview.chromium.org/1158693006/diff/40001/extensions/browser/api/... File extensions/browser/api/declarative/rules_registry.cc (right): https://codereview.chromium.org/1158693006/diff/40001/extensions/browser/api/... extensions/browser/api/declarative/rules_registry.cc:231: // If no rules have been registered yet, check if there are any rules On 2015/06/02 22:44:50, kalman wrote: > Why only if there are no rules registered yet? It seems a perfectly reasonable > use case to have a set of rules in the manifest, with a dynamic set to add > later. Done. https://codereview.chromium.org/1158693006/diff/120001/extensions/browser/api... File extensions/browser/api/declarative/rules_registry.cc (right): https://codereview.chromium.org/1158693006/diff/120001/extensions/browser/api... extensions/browser/api/declarative/rules_registry.cc:278: i != rules_.end(); ++i) { On 2015/06/04 22:09:28, kalman wrote: > On 2015/06/04 22:02:33, danduong wrote: > > On 2015/06/04 22:00:50, kalman wrote: > > > here and in GetRules, augment the list you return with the manifest rules > > > > They should already be in there. The manifest rules are still a part of > |rules_| > > yes. don't put them in rules. Done. https://codereview.chromium.org/1158693006/diff/120001/extensions/browser/api... File extensions/browser/api/declarative/rules_registry.h (right): https://codereview.chromium.org/1158693006/diff/120001/extensions/browser/api... extensions/browser/api/declarative/rules_registry.h:80: bool from_manifest = false); On 2015/06/04 22:00:51, kalman wrote: > default parameters are against style guide Done. https://codereview.chromium.org/1158693006/diff/120001/extensions/browser/api... File extensions/browser/api/declarative/rules_registry_unittest.cc (right): https://codereview.chromium.org/1158693006/diff/120001/extensions/browser/api... extensions/browser/api/declarative/rules_registry_unittest.cc:296: ASSERT_EQ(3u, get_rules.size()); On 2015/06/04 22:15:15, Mike Wittman wrote: > ASSERTs in this test can all be EXPECTs if you're not looking at the rules Done. https://codereview.chromium.org/1158693006/diff/120001/extensions/browser/api... extensions/browser/api/declarative/rules_registry_unittest.cc:323: // definition in the manifest. On 2015/06/04 22:15:15, Mike Wittman wrote: > ... and the expected behavior is ??? Done. https://codereview.chromium.org/1158693006/diff/120001/extensions/browser/api... extensions/browser/api/declarative/rules_registry_unittest.cc:419: ASSERT_EQ(0u, get_rules.size()); On 2015/06/04 22:15:15, Mike Wittman wrote: > EXPECT_EQ Done.
On 2015/06/04 18:40:14, Mike Wittman wrote: > Can you also add at least one API test to declarative_content_apitest.cc to test > the full implementation? Added 1 api test. > > https://codereview.chromium.org/1158693006/diff/100001/extensions/browser/api... > File extensions/browser/api/declarative/rules_registry.cc (right): > > https://codereview.chromium.org/1158693006/diff/100001/extensions/browser/api... > extensions/browser/api/declarative/rules_registry.cc:311: AddRules(extension_id, > RulesFromManifest(value, event_name_)); > this needs to handle the error which may be returned, and propagate it to the > extension's error log > > https://codereview.chromium.org/1158693006/diff/100001/extensions/browser/api... > File extensions/browser/api/declarative/rules_registry_unittest.cc (right): > > https://codereview.chromium.org/1158693006/diff/100001/extensions/browser/api... > extensions/browser/api/declarative/rules_registry_unittest.cc:177: > TEST_F(RulesRegistryTest, TwoRulesInManifest) { > Can you add a comment before each test describing what it's testing? > > https://codereview.chromium.org/1158693006/diff/100001/extensions/browser/api... > extensions/browser/api/declarative/rules_registry_unittest.cc:235: > ASSERT_EQ("declarativeContent.PageStateMatcher", instance_type); > Checks that can fail without causing the test to crash should use EXPECT rather > than ASSERT.
PTAL again. Appreciate the reviews! https://codereview.chromium.org/1158693006/diff/40001/extensions/browser/api/... File extensions/browser/api/declarative/rules_registry.cc (right): https://codereview.chromium.org/1158693006/diff/40001/extensions/browser/api/... extensions/browser/api/declarative/rules_registry.cc:226: void RulesRegistry::OnExtensionLoaded(const std::string& extension_id) { On 2015/06/02 22:44:50, kalman wrote: > Could you update OnExtensionUnloaded/Uninstalled/Loaded to pass in an Extension > rather than its ID? It's trivial (call sites just grab the ID out of the > Extension object they already have access to) and it would save needing to do > the extra map lookup + the extensionregistry dependency. Done. https://codereview.chromium.org/1158693006/diff/100001/extensions/browser/api... File extensions/browser/api/declarative/rules_registry.cc (right): https://codereview.chromium.org/1158693006/diff/100001/extensions/browser/api... extensions/browser/api/declarative/rules_registry.cc:91: for (size_t i = 0; i < list->GetSize(); ++i) { On 2015/06/04 17:58:49, kalman wrote: > You should look at defining this structure in extensions_manifest_types.json, > which will generate much of this parsing for you. See how OptionsUI is > implemented, for example. I looked into it and I ran into some problems with the code generator since event_rules is type:array. The individual rule object is already defined in events.json. So I refactored the code a little bit to make better use of this. https://codereview.chromium.org/1158693006/diff/100001/extensions/browser/api... File extensions/browser/api/declarative/rules_registry_unittest.cc (right): https://codereview.chromium.org/1158693006/diff/100001/extensions/browser/api... extensions/browser/api/declarative/rules_registry_unittest.cc:177: TEST_F(RulesRegistryTest, TwoRulesInManifest) { On 2015/06/04 22:15:15, Mike Wittman wrote: > On 2015/06/04 19:56:07, danduong wrote: > > On 2015/06/04 18:40:13, Mike Wittman wrote: > > > Can you add a comment before each test describing what it's testing? > > > > Done. > > convention is to put the comments above the TEST declaration Done.
https://codereview.chromium.org/1158693006/diff/220001/extensions/browser/api... File extensions/browser/api/declarative/rules_registry.cc (right): https://codereview.chromium.org/1158693006/diff/220001/extensions/browser/api... extensions/browser/api/declarative/rules_registry.cc:33: const char kErrorNonRemovableRules[] = "List contains non-removable rules"; Be more literal: const char kErrorCannotRemoveManifestRules[] = "Rules declared in the 'event_rules' manifest field cannot be removed"; https://codereview.chromium.org/1158693006/diff/220001/extensions/browser/api... extensions/browser/api/declarative/rules_registry.cc:64: bool ConvertManifestRule(linked_ptr<RulesRegistry::Rule> rule) { avoid copy and use a const linked_ptr& https://codereview.chromium.org/1158693006/diff/220001/extensions/browser/api... extensions/browser/api/declarative/rules_registry.cc:65: // Swaps key 'type' (used in manifest) for 'instanceType'. Could you move this comment up to a function-level comment? All the existing functions should have comments too, oh well. Describe what the differences are, explain that it's specifically the 'conditions' and 'actions' that need converting, etc. https://codereview.chromium.org/1158693006/diff/220001/extensions/browser/api... extensions/browser/api/declarative/rules_registry.cc:67: for (linked_ptr<base::Value> value : list) { avoid copy and use a (const?) linked_ptr& https://codereview.chromium.org/1158693006/diff/220001/extensions/browser/api... extensions/browser/api/declarative/rules_registry.cc:88: if (!value || !value->GetAsList(&list)) The (!value) here here shouldn't be necessary, it doesn't make sense for a caller to pass in a null |value|. I don't know why the existing code does it either :-\ https://codereview.chromium.org/1158693006/diff/220001/extensions/browser/api... extensions/browser/api/declarative/rules_registry.cc:89: return rules; Another problem is that this silently fails if an extension passes anything other than a list into here. Regarding your comment: "I looked into it and I ran into some problems with the code generator since event_rules is type:array." That is a problem that I recall somebody else having, but if "type": { "id": "RuleList", "type": "array", "items": {"$ref": "events.Rule"} } or whatever doesn't work, then you could work around that by doing "type": { "id": "RuleList", "type": "object", "properties": [{ "rules": { "type": "array", "items": {"$ref": "events.Rule"} } }] } i.e. nest the list inside an object. https://codereview.chromium.org/1158693006/diff/220001/extensions/browser/api... extensions/browser/api/declarative/rules_registry.cc:99: continue; It would help to document this stuff. I only know what's going on because I reviewed the proposal. I.e. what is "event", what is "type", and so forth. https://codereview.chromium.org/1158693006/diff/220001/extensions/browser/api... extensions/browser/api/declarative/rules_registry.cc:105: rules.push_back(rule); Apologies for this nit, but btw, these methods that return bools are designed to be chained together like if (RulesRegistry::Rule::Populate(*dict, rule.get()) && ConvertManifestRule(rule)) { rules.push_back(rule); } and likewise above, if (!dict->GetString("event", &event) || event_name != event)) { continue; } https://codereview.chromium.org/1158693006/diff/220001/extensions/browser/api... extensions/browser/api/declarative/rules_registry.cc:142: bool from_manifest) { It's more direct to pass around the list of rules to add to, rather than a flag to which one to use. https://codereview.chromium.org/1158693006/diff/220001/extensions/browser/api... extensions/browser/api/declarative/rules_registry.cc:187: return AddRulesNoFill(extension_id, rules, from_manifest); This can only ever be false, so remove the |from_manifest| flag from the public interface, and pass in false here. Or rather, pass in |&rules_|. https://codereview.chromium.org/1158693006/diff/220001/extensions/browser/api... extensions/browser/api/declarative/rules_registry.cc:242: } Btw the way I usually see this written is along the lines of for (auto it = dictionary.begin(); it != dictionary.end();) { if ((*it->first).first == extension_id) dictionary.erase(it++) else ++it; } but you could also using std::remove_if? Not worth making significant changes to existing code, I suppose. https://codereview.chromium.org/1158693006/diff/220001/extensions/browser/api... extensions/browser/api/declarative/rules_registry.cc:267: } Simpler: RulesDictionary::iterator entry = rules_.find(lookup_key); if (entry == rules_.end()) entry = manifest_rules_.find(lookup_key); if (entry != rules_.end()) out->push_back(entry->second); https://codereview.chromium.org/1158693006/diff/220001/extensions/browser/api... extensions/browser/api/declarative/rules_registry.cc:276: i != dictionary.end(); ++i) { (btw, consider changing all of these sorts of loops to C++11 range-based loops as you go, cleaner) https://codereview.chromium.org/1158693006/diff/220001/extensions/browser/api... extensions/browser/api/declarative/rules_registry.cc:366: } This is pretty wasteful. 1. GetAllRules gets |rules_| and |manifest_rules_|. 2. This code removes |manifest_rules_|. 3. Write out. there is no point getting |manifest_rules_| so factor code accordingly. https://codereview.chromium.org/1158693006/diff/220001/extensions/browser/api... File extensions/browser/api/declarative/rules_registry.h (right): https://codereview.chromium.org/1158693006/diff/220001/extensions/browser/api... extensions/browser/api/declarative/rules_registry.h:81: bool from_manifest); This from_manifest flag shouldn't be part of the public interface. The manifest rule loading is done in the OnExtensionLoaded event, which is purely internal to this class. It would be wrong for anybody, other than this class, to pass anything other than false in here. https://codereview.chromium.org/1158693006/diff/220001/extensions/browser/api... extensions/browser/api/declarative/rules_registry.h:117: void OnExtensionLoaded(const Extension* extension); Thanks for doing this. https://codereview.chromium.org/1158693006/diff/220001/extensions/browser/api... File extensions/browser/api/declarative/rules_registry_unittest.cc (right): https://codereview.chromium.org/1158693006/diff/220001/extensions/browser/api... extensions/browser/api/declarative/rules_registry_unittest.cc:213: " }]" It would be worth also setting an ID and tags on one of these to make sure those get propagated, too. https://codereview.chromium.org/1158693006/diff/220001/extensions/browser/api... extensions/browser/api/declarative/rules_registry_unittest.cc:317: // Remove all rules. Why does removing all rules also remove the ones from the manifest? It seems like you shouldn't be able to remove them.
https://codereview.chromium.org/1158693006/diff/220001/extensions/browser/api... File extensions/browser/api/declarative/rules_registry.cc (right): https://codereview.chromium.org/1158693006/diff/220001/extensions/browser/api... extensions/browser/api/declarative/rules_registry.cc:33: const char kErrorNonRemovableRules[] = "List contains non-removable rules"; On 2015/06/05 16:16:14, kalman wrote: > Be more literal: > > const char kErrorCannotRemoveManifestRules[] = > "Rules declared in the 'event_rules' manifest field cannot be removed"; Done. https://codereview.chromium.org/1158693006/diff/220001/extensions/browser/api... extensions/browser/api/declarative/rules_registry.cc:64: bool ConvertManifestRule(linked_ptr<RulesRegistry::Rule> rule) { On 2015/06/05 16:16:14, kalman wrote: > avoid copy and use a const linked_ptr& Done. https://codereview.chromium.org/1158693006/diff/220001/extensions/browser/api... extensions/browser/api/declarative/rules_registry.cc:65: // Swaps key 'type' (used in manifest) for 'instanceType'. On 2015/06/05 16:16:14, kalman wrote: > Could you move this comment up to a function-level comment? All the existing > functions should have comments too, oh well. Describe what the differences are, > explain that it's specifically the 'conditions' and 'actions' that need > converting, etc. Done. https://codereview.chromium.org/1158693006/diff/220001/extensions/browser/api... extensions/browser/api/declarative/rules_registry.cc:67: for (linked_ptr<base::Value> value : list) { On 2015/06/05 16:16:14, kalman wrote: > avoid copy and use a (const?) linked_ptr& Done. https://codereview.chromium.org/1158693006/diff/220001/extensions/browser/api... extensions/browser/api/declarative/rules_registry.cc:88: if (!value || !value->GetAsList(&list)) On 2015/06/05 16:16:14, kalman wrote: > The (!value) here here shouldn't be necessary, it doesn't make sense for a > caller to pass in a null |value|. I don't know why the existing code does it > either :-\ Done. https://codereview.chromium.org/1158693006/diff/220001/extensions/browser/api... extensions/browser/api/declarative/rules_registry.cc:89: return rules; On 2015/06/05 16:16:13, kalman wrote: > Another problem is that this silently fails if an extension passes anything > other than a list into here. Regarding your comment: > > "I looked into it and I ran into some problems with the code generator since > event_rules is type:array." > > That is a problem that I recall somebody else having, but if > > "type": { > "id": "RuleList", > "type": "array", > "items": {"$ref": "events.Rule"} > } > > or whatever doesn't work, then you could work around that by doing > > "type": { > "id": "RuleList", > "type": "object", > "properties": [{ > "rules": { > "type": "array", > "items": {"$ref": "events.Rule"} > } > }] > } > > i.e. nest the list inside an object. Should we just log error in these cases? https://codereview.chromium.org/1158693006/diff/220001/extensions/browser/api... extensions/browser/api/declarative/rules_registry.cc:99: continue; On 2015/06/05 16:16:14, kalman wrote: > It would help to document this stuff. I only know what's going on because I > reviewed the proposal. I.e. what is "event", what is "type", and so forth. Done. https://codereview.chromium.org/1158693006/diff/220001/extensions/browser/api... extensions/browser/api/declarative/rules_registry.cc:105: rules.push_back(rule); On 2015/06/05 16:16:14, kalman wrote: > Apologies for this nit, but btw, these methods that return bools are designed to > be chained together like > > if (RulesRegistry::Rule::Populate(*dict, rule.get()) && > ConvertManifestRule(rule)) { > rules.push_back(rule); > } > > and likewise above, > > if (!dict->GetString("event", &event) || event_name != event)) { > continue; > } Done. https://codereview.chromium.org/1158693006/diff/220001/extensions/browser/api... extensions/browser/api/declarative/rules_registry.cc:142: bool from_manifest) { On 2015/06/05 16:16:14, kalman wrote: > It's more direct to pass around the list of rules to add to, rather than a flag > to which one to use. Done. https://codereview.chromium.org/1158693006/diff/220001/extensions/browser/api... extensions/browser/api/declarative/rules_registry.cc:187: return AddRulesNoFill(extension_id, rules, from_manifest); On 2015/06/05 16:16:14, kalman wrote: > This can only ever be false, so remove the |from_manifest| flag from the public > interface, and pass in false here. Or rather, pass in |&rules_|. Done. https://codereview.chromium.org/1158693006/diff/220001/extensions/browser/api... extensions/browser/api/declarative/rules_registry.cc:242: } On 2015/06/05 16:16:14, kalman wrote: > Btw the way I usually see this written is along the lines of > > for (auto it = dictionary.begin(); it != dictionary.end();) { > if ((*it->first).first == extension_id) > dictionary.erase(it++) > else > ++it; > } > > but you could also using std::remove_if? Not worth making significant changes to > existing code, I suppose. Done. https://codereview.chromium.org/1158693006/diff/220001/extensions/browser/api... extensions/browser/api/declarative/rules_registry.cc:267: } On 2015/06/05 16:16:14, kalman wrote: > Simpler: > > RulesDictionary::iterator entry = rules_.find(lookup_key); > if (entry == rules_.end()) > entry = manifest_rules_.find(lookup_key); > if (entry != rules_.end()) > out->push_back(entry->second); Done. https://codereview.chromium.org/1158693006/diff/220001/extensions/browser/api... extensions/browser/api/declarative/rules_registry.cc:276: i != dictionary.end(); ++i) { On 2015/06/05 16:16:13, kalman wrote: > (btw, consider changing all of these sorts of loops to C++11 range-based loops > as you go, cleaner) Acknowledged. https://codereview.chromium.org/1158693006/diff/220001/extensions/browser/api... extensions/browser/api/declarative/rules_registry.cc:366: } On 2015/06/05 16:16:13, kalman wrote: > This is pretty wasteful. > 1. GetAllRules gets |rules_| and |manifest_rules_|. > 2. This code removes |manifest_rules_|. > 3. Write out. > > there is no point getting |manifest_rules_| so factor code accordingly. Done. https://codereview.chromium.org/1158693006/diff/220001/extensions/browser/api... File extensions/browser/api/declarative/rules_registry.h (right): https://codereview.chromium.org/1158693006/diff/220001/extensions/browser/api... extensions/browser/api/declarative/rules_registry.h:81: bool from_manifest); On 2015/06/05 16:16:14, kalman wrote: > This from_manifest flag shouldn't be part of the public interface. The manifest > rule loading is done in the OnExtensionLoaded event, which is purely internal to > this class. It would be wrong for anybody, other than this class, to pass > anything other than false in here. Done. https://codereview.chromium.org/1158693006/diff/220001/extensions/browser/api... extensions/browser/api/declarative/rules_registry.h:117: void OnExtensionLoaded(const Extension* extension); On 2015/06/05 16:16:14, kalman wrote: > Thanks for doing this. Acknowledged. https://codereview.chromium.org/1158693006/diff/220001/extensions/browser/api... File extensions/browser/api/declarative/rules_registry_unittest.cc (right): https://codereview.chromium.org/1158693006/diff/220001/extensions/browser/api... extensions/browser/api/declarative/rules_registry_unittest.cc:213: " }]" On 2015/06/05 16:16:14, kalman wrote: > It would be worth also setting an ID and tags on one of these to make sure those > get propagated, too. Done. https://codereview.chromium.org/1158693006/diff/220001/extensions/browser/api... extensions/browser/api/declarative/rules_registry_unittest.cc:317: // Remove all rules. On 2015/06/05 16:16:14, kalman wrote: > Why does removing all rules also remove the ones from the manifest? It seems > like you shouldn't be able to remove them. Good catch. Some reason I thought this was internal-only for uninstall. I made it so that it only removes that during uninstall.
https://codereview.chromium.org/1158693006/diff/240001/extensions/browser/api... File extensions/browser/api/declarative/rules_registry.cc (right): https://codereview.chromium.org/1158693006/diff/240001/extensions/browser/api... extensions/browser/api/declarative/rules_registry.cc:92: // The following is an example of how an event programmatic rule definition You'll need to add something like this to the manifest API documentation in chrome/common/extensions/docs as well.
Gotta runs sorry, will finish first thing next week, but responding to one of your comments. https://codereview.chromium.org/1158693006/diff/220001/extensions/browser/api... File extensions/browser/api/declarative/rules_registry.cc (right): https://codereview.chromium.org/1158693006/diff/220001/extensions/browser/api... extensions/browser/api/declarative/rules_registry.cc:89: return rules; On 2015/06/05 21:35:06, danduong wrote: > On 2015/06/05 16:16:13, kalman wrote: > > Another problem is that this silently fails if an extension passes anything > > other than a list into here. Regarding your comment: > > > > "I looked into it and I ran into some problems with the code generator since > > event_rules is type:array." > > > > That is a problem that I recall somebody else having, but if > > > > "type": { > > "id": "RuleList", > > "type": "array", > > "items": {"$ref": "events.Rule"} > > } > > > > or whatever doesn't work, then you could work around that by doing > > > > "type": { > > "id": "RuleList", > > "type": "object", > > "properties": [{ > > "rules": { > > "type": "array", > > "items": {"$ref": "events.Rule"} > > } > > }] > > } > > > > i.e. nest the list inside an object. > > Should we just log error in these cases? Ideally we'd be parsing these rules when the extension is installed. That way you can actually report warnings on installation. To do that you'd set up a manifest handler (ManifestHandler). I was debating asking you to do this. It's idiomatic, whereas doing the parsing ad-hoc like you've done is uncommon, but it seemed easier enough to do it like you've done (given you'd written it that way). To show a warning you could use the ErrorConsole: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext... but preferable solution is really to use a manifest handler.
https://codereview.chromium.org/1158693006/diff/220001/extensions/browser/api... File extensions/browser/api/declarative/rules_registry.cc (right): https://codereview.chromium.org/1158693006/diff/220001/extensions/browser/api... extensions/browser/api/declarative/rules_registry.cc:89: return rules; On 2015/06/05 23:29:42, kalman wrote: > On 2015/06/05 21:35:06, danduong wrote: > > On 2015/06/05 16:16:13, kalman wrote: > > > Another problem is that this silently fails if an extension passes anything > > > other than a list into here. Regarding your comment: > > > > > > "I looked into it and I ran into some problems with the code generator since > > > event_rules is type:array." > > > > > > That is a problem that I recall somebody else having, but if > > > > > > "type": { > > > "id": "RuleList", > > > "type": "array", > > > "items": {"$ref": "events.Rule"} > > > } > > > > > > or whatever doesn't work, then you could work around that by doing > > > > > > "type": { > > > "id": "RuleList", > > > "type": "object", > > > "properties": [{ > > > "rules": { > > > "type": "array", > > > "items": {"$ref": "events.Rule"} > > > } > > > }] > > > } > > > > > > i.e. nest the list inside an object. > > > > Should we just log error in these cases? > > Ideally we'd be parsing these rules when the extension is installed. That way > you can actually report warnings on installation. To do that you'd set up a > manifest handler (ManifestHandler). > > I was debating asking you to do this. It's idiomatic, whereas doing the parsing > ad-hoc like you've done is uncommon, but it seemed easier enough to do it like > you've done (given you'd written it that way). > > To show a warning you could use the ErrorConsole: > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext... > > but preferable solution is really to use a manifest handler. added a manifest handler.
Thanks for adding the ManifestHandler, much better separation + more idiomatic. https://codereview.chromium.org/1158693006/diff/220001/extensions/browser/api... File extensions/browser/api/declarative/rules_registry.cc (right): https://codereview.chromium.org/1158693006/diff/220001/extensions/browser/api... extensions/browser/api/declarative/rules_registry.cc:142: bool from_manifest) { On 2015/06/05 21:35:06, danduong wrote: > On 2015/06/05 16:16:14, kalman wrote: > > It's more direct to pass around the list of rules to add to, rather than a > flag > > to which one to use. > > Done. except, pass as a pointer not a reference. https://codereview.chromium.org/1158693006/diff/260001/extensions/browser/api... File extensions/browser/api/declarative/rules_registry.cc (right): https://codereview.chromium.org/1158693006/diff/260001/extensions/browser/api... extensions/browser/api/declarative/rules_registry.cc:188: bool uninstall) { I'm not sure what "uninstall" means in this context - I initially thought that it meant to uninstall the rules which is why its call sites doesn't make sense. But, (a) better to be literal so call this "remove_manifest_rules", and (b) mention it in the header file comment. https://codereview.chromium.org/1158693006/diff/260001/extensions/browser/api... File extensions/browser/api/declarative/rules_registry.h (right): https://codereview.chromium.org/1158693006/diff/260001/extensions/browser/api... extensions/browser/api/declarative/rules_registry.h:185: RulesDictionary& out); Non-const references as arguments are not allowed by the Google style guide. Use "RulesDictionary*" everywhere. https://codereview.chromium.org/1158693006/diff/260001/extensions/browser/api... File extensions/browser/api/declarative/rules_registry_unittest.cc (right): https://codereview.chromium.org/1158693006/diff/260001/extensions/browser/api... extensions/browser/api/declarative/rules_registry_unittest.cc:325: EXPECT_EQ(1u, get_rules.size()); For this whole test, is it actually possible to test that you removed the *right* rule rather than just *any* rule? I.e after saying "EXPECT_EQ(3u, get_rules.size())" also then check against the IDs or something? (you may want to create a utility to get a linked_ptr to a Rule with an ID instead of just using "make_linked_ptr(new RulesRegistry::Rule))". https://codereview.chromium.org/1158693006/diff/260001/extensions/common/api/... File extensions/common/api/declarative/declarative_manifest_data.cc (right): https://codereview.chromium.org/1158693006/diff/260001/extensions/common/api/... extensions/common/api/declarative/declarative_manifest_data.cc:8: #include "extensions/common/api/extensions_manifest_types.h" What are you using from this? https://codereview.chromium.org/1158693006/diff/260001/extensions/common/api/... extensions/common/api/declarative/declarative_manifest_data.cc:17: std::string ValueTypeToString(base::Value::Type type) { You might as well pass the whole Value into here, rather than its type. https://codereview.chromium.org/1158693006/diff/260001/extensions/common/api/... extensions/common/api/declarative/declarative_manifest_data.cc:27: return std::string(strings[type]); you could also make it return a const char* if you prefer. https://codereview.chromium.org/1158693006/diff/260001/extensions/common/api/... extensions/common/api/declarative/declarative_manifest_data.cc:122: There is a lot of code here for generating errors... is it tested? You could clean it up a lot by writing a quick ErrorBuilder class. It would let you (a) remove all of the UTF8ToUTF16 calls, and (b) let you remove all of the semicolon handling: class ErrorBuilder { public: ErrorBuilder() {} const base::string16& error() { return error_; } // Appends a literal string |error|. void Append(const char* error) { if (error_.length()) error_.append(UF8ToUTF16("; ")); error_.append(UTF8ToUTF16(error)); } // Appends a string |error| with the first %s replaced by |sub|. void Append(const char* error, const char* sub) { Append(base::StringPrintf(error, sub)); } private: DISALLOW_COPY_AND_ASSIGN(ErrorBuilder); base::string16 error_; } here, assuming it's passed in as a pointer, use like: error_builder->Append("'event_rules' expected list, got ", ValueTypeToString(value)); ... (later) error_builder->Append("expected dictionary"); Would generally make the business logic easier to review as well. https://codereview.chromium.org/1158693006/diff/260001/extensions/common/api/... File extensions/common/api/declarative/declarative_manifest_data.h (right): https://codereview.chromium.org/1158693006/diff/260001/extensions/common/api/... extensions/common/api/declarative/declarative_manifest_data.h:23: explicit DeclarativeManifestData(); explicit not needed on a zero-argument constructor. in fact an explicit constructor isn't really needed (though actually - perhaps it is - you should put DISALLOW_COPY_AND_ASSIGN on this class).
https://codereview.chromium.org/1158693006/diff/220001/extensions/browser/api... File extensions/browser/api/declarative/rules_registry.cc (right): https://codereview.chromium.org/1158693006/diff/220001/extensions/browser/api... extensions/browser/api/declarative/rules_registry.cc:142: bool from_manifest) { On 2015/06/08 21:44:58, kalman wrote: > On 2015/06/05 21:35:06, danduong wrote: > > On 2015/06/05 16:16:14, kalman wrote: > > > It's more direct to pass around the list of rules to add to, rather than a > > flag > > > to which one to use. > > > > Done. > > except, pass as a pointer not a reference. Done. https://codereview.chromium.org/1158693006/diff/260001/extensions/browser/api... File extensions/browser/api/declarative/rules_registry.cc (right): https://codereview.chromium.org/1158693006/diff/260001/extensions/browser/api... extensions/browser/api/declarative/rules_registry.cc:188: bool uninstall) { On 2015/06/08 21:44:58, kalman wrote: > I'm not sure what "uninstall" means in this context - I initially thought that > it meant to uninstall the rules which is why its call sites doesn't make sense. > > But, (a) better to be literal so call this "remove_manifest_rules", and (b) > mention it in the header file comment. Done. https://codereview.chromium.org/1158693006/diff/260001/extensions/browser/api... File extensions/browser/api/declarative/rules_registry.h (right): https://codereview.chromium.org/1158693006/diff/260001/extensions/browser/api... extensions/browser/api/declarative/rules_registry.h:185: RulesDictionary& out); On 2015/06/08 21:44:58, kalman wrote: > Non-const references as arguments are not allowed by the Google style guide. Use > "RulesDictionary*" everywhere. Done. https://codereview.chromium.org/1158693006/diff/260001/extensions/browser/api... File extensions/browser/api/declarative/rules_registry_unittest.cc (right): https://codereview.chromium.org/1158693006/diff/260001/extensions/browser/api... extensions/browser/api/declarative/rules_registry_unittest.cc:325: EXPECT_EQ(1u, get_rules.size()); On 2015/06/08 21:44:58, kalman wrote: > For this whole test, is it actually possible to test that you removed the > *right* rule rather than just *any* rule? I.e after saying "EXPECT_EQ(3u, > get_rules.size())" also then check against the IDs or something? (you may want > to create a utility to get a linked_ptr to a Rule with an ID instead of just > using "make_linked_ptr(new RulesRegistry::Rule))". Done. https://codereview.chromium.org/1158693006/diff/260001/extensions/common/api/... File extensions/common/api/declarative/declarative_manifest_data.cc (right): https://codereview.chromium.org/1158693006/diff/260001/extensions/common/api/... extensions/common/api/declarative/declarative_manifest_data.cc:8: #include "extensions/common/api/extensions_manifest_types.h" On 2015/06/08 21:44:58, kalman wrote: > What are you using from this? Done. https://codereview.chromium.org/1158693006/diff/260001/extensions/common/api/... extensions/common/api/declarative/declarative_manifest_data.cc:17: std::string ValueTypeToString(base::Value::Type type) { On 2015/06/08 21:44:58, kalman wrote: > You might as well pass the whole Value into here, rather than its type. Done. https://codereview.chromium.org/1158693006/diff/260001/extensions/common/api/... extensions/common/api/declarative/declarative_manifest_data.cc:122: On 2015/06/08 21:44:58, kalman wrote: > There is a lot of code here for generating errors... is it tested? You could > clean it up a lot by writing a quick ErrorBuilder class. It would let you (a) > remove all of the UTF8ToUTF16 calls, and (b) let you remove all of the semicolon > handling: > > > class ErrorBuilder { > public: > ErrorBuilder() {} > > const base::string16& error() { return error_; } > > // Appends a literal string |error|. > void Append(const char* error) { > if (error_.length()) > error_.append(UF8ToUTF16("; ")); > error_.append(UTF8ToUTF16(error)); > } > > // Appends a string |error| with the first %s replaced by |sub|. > void Append(const char* error, const char* sub) { > Append(base::StringPrintf(error, sub)); > } > > > private: > DISALLOW_COPY_AND_ASSIGN(ErrorBuilder); > > base::string16 error_; > } > > > here, assuming it's passed in as a pointer, use like: > > error_builder->Append("'event_rules' expected list, got ", > ValueTypeToString(value)); > ... (later) > error_builder->Append("expected dictionary"); > > > Would generally make the business logic easier to review as well. Done. https://codereview.chromium.org/1158693006/diff/260001/extensions/common/api/... File extensions/common/api/declarative/declarative_manifest_data.h (right): https://codereview.chromium.org/1158693006/diff/260001/extensions/common/api/... extensions/common/api/declarative/declarative_manifest_data.h:23: explicit DeclarativeManifestData(); On 2015/06/08 21:44:58, kalman wrote: > explicit not needed on a zero-argument constructor. in fact an explicit > constructor isn't really needed (though actually - perhaps it is - you should > put DISALLOW_COPY_AND_ASSIGN on this class). Done.
lgtm https://codereview.chromium.org/1158693006/diff/280001/extensions/browser/api... File extensions/browser/api/declarative/rules_registry_unittest.cc (right): https://codereview.chromium.org/1158693006/diff/280001/extensions/browser/api... extensions/browser/api/declarative/rules_registry_unittest.cc:313: ASSERT_EQ("rule_2", *(get_rules[2]->id)); These 2 can be EXPECT not ASSERT, as can the other places you're just comparing IDs. https://codereview.chromium.org/1158693006/diff/280001/extensions/common/api/... File extensions/common/api/declarative/declarative_manifest_data.cc (right): https://codereview.chromium.org/1158693006/diff/280001/extensions/common/api/... extensions/common/api/declarative/declarative_manifest_data.cc:34: ErrorBuilder(base::string16* error) : error_(error) {} explicit https://codereview.chromium.org/1158693006/diff/280001/extensions/common/api/... extensions/common/api/declarative/declarative_manifest_data.cc:71: error_builder->Append("'type' is required"); this will also fail if "type" is set but isn't a string, so you might want to say "'type' is required, and must be a string". https://codereview.chromium.org/1158693006/diff/280001/extensions/common/api/... extensions/common/api/declarative/declarative_manifest_data.cc:164: error_builder.Append("rule failed to populate"); I think that, now, the Populate and the ConvertManifestRule should be different steps, because "rule failed to populate" may not actually be true, in the case that it was actually ConvertManifestRule which failed (in which case the error will have already been populated).
Also added some docs. https://codereview.chromium.org/1158693006/diff/280001/extensions/browser/api... File extensions/browser/api/declarative/rules_registry_unittest.cc (right): https://codereview.chromium.org/1158693006/diff/280001/extensions/browser/api... extensions/browser/api/declarative/rules_registry_unittest.cc:313: ASSERT_EQ("rule_2", *(get_rules[2]->id)); On 2015/06/09 20:55:40, kalman wrote: > These 2 can be EXPECT not ASSERT, as can the other places you're just comparing > IDs. Done. https://codereview.chromium.org/1158693006/diff/280001/extensions/common/api/... File extensions/common/api/declarative/declarative_manifest_data.cc (right): https://codereview.chromium.org/1158693006/diff/280001/extensions/common/api/... extensions/common/api/declarative/declarative_manifest_data.cc:34: ErrorBuilder(base::string16* error) : error_(error) {} On 2015/06/09 20:55:41, kalman wrote: > explicit Done. https://codereview.chromium.org/1158693006/diff/280001/extensions/common/api/... extensions/common/api/declarative/declarative_manifest_data.cc:71: error_builder->Append("'type' is required"); On 2015/06/09 20:55:40, kalman wrote: > this will also fail if "type" is set but isn't a string, so you might want to > say "'type' is required, and must be a string". Done.
lgtm
The CQ bit was checked by danduong@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kalman@chromium.org Link to the patchset: https://codereview.chromium.org/1158693006/#ps340001 (title: "add docs")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1158693006/340001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by danduong@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from wittman@chromium.org, kalman@chromium.org Link to the patchset: https://codereview.chromium.org/1158693006/#ps360001 (title: "don't crash")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1158693006/360001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by danduong@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1158693006/360001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by danduong@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1158693006/360001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by danduong@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1158693006/360001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Have you tried running those tests locally?
On 2015/06/11 16:37:56, kalman wrote: > Have you tried running those tests locally? I've run the failing linux test locally and they passed.
On 2015/06/11 16:53:23, danduong wrote: > On 2015/06/11 16:37:56, kalman wrote: > > Have you tried running those tests locally? > > I've run the failing linux test locally and they passed. Ah! I see what's happening now. It's the change for OnExtensionUninstalled/Loaded/Unloaded(const std::string& extension_id) -> OnExtensionUninstalled/Loaded/Unloaded(Extension* extension_id) This introduces a race condition. We can move it to scoped_refptr<Extension>. Is the access to the id thread safe to a constructed object?
Yep it's thread safe On Jun 11, 2015 6:28 PM, <danduong@chromium.org> wrote: > On 2015/06/11 16:53:23, danduong wrote: > >> On 2015/06/11 16:37:56, kalman wrote: >> > Have you tried running those tests locally? >> > > I've run the failing linux test locally and they passed. >> > > Ah! I see what's happening now. It's the change for > OnExtensionUninstalled/Loaded/Unloaded(const std::string& extension_id) -> > OnExtensionUninstalled/Loaded/Unloaded(Extension* extension_id) > > This introduces a race condition. > > We can move it to scoped_refptr<Extension>. Is the access to the id thread > safe > to a constructed object? > > https://codereview.chromium.org/1158693006/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2015/06/12 03:50:58, kalman wrote: > Yep it's thread safe > On Jun 11, 2015 6:28 PM, <mailto:danduong@chromium.org> wrote: > > > On 2015/06/11 16:53:23, danduong wrote: > > > >> On 2015/06/11 16:37:56, kalman wrote: > >> > Have you tried running those tests locally? > >> > > > > I've run the failing linux test locally and they passed. > >> > > > > Ah! I see what's happening now. It's the change for > > OnExtensionUninstalled/Loaded/Unloaded(const std::string& extension_id) -> > > OnExtensionUninstalled/Loaded/Unloaded(Extension* extension_id) > > > > This introduces a race condition. > > > > We can move it to scoped_refptr<Extension>. Is the access to the id thread > > safe > > to a constructed object? > > > > https://codereview.chromium.org/1158693006/ > > > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. Looks like I'll still run into a problem here. It's a const Extension* so scoped_refptr won't work. I may need to revert back to querying the extensionregistry for the extension_id.
Actually can you point out exactly where this race condition is? Changing the function signature to an Extension shouldn't really change anything.
On 2015/06/12 14:55:06, kalman wrote: > Actually can you point out exactly where this race condition is? Changing the > function signature to an Extension shouldn't really change anything. RulesRegistryService::OnExtensionUninstalled() posts a tasks to RulesRegistry::OnExtensionUninstalled on it's owner_thread(). I believe the extension can be deleted before the PostTask.
On 2015/06/12 15:14:33, danduong wrote:
> On 2015/06/12 14:55:06, kalman wrote:
> > Actually can you point out exactly where this race condition is? Changing
the
> > function signature to an Extension shouldn't really change anything.
>
> RulesRegistryService::OnExtensionUninstalled() posts a tasks to
> RulesRegistry::OnExtensionUninstalled on it's owner_thread().
>
> I believe the extension can be deleted before the PostTask.
In that case have an intermediate method which holds onto a scoped_refptr. No
need to pull that knowledge into the interface method:
RulesRegistryService::OnExtensionUninstalled(const Extension* extension) {
auto task = base::Bind(&RulesRegistry::OnExtensionUninstalled,
rules_registry);
rules_registry->owner_thread()->PostTask(
FROM_HERE,
base::Bind(&CallWithExtensionSafe,
make_scoped_refptr(extension), task));
}
void CallWithExtensionSafe(scoped_refptr<const Extension> extension,
const base::Callback<void(const Extension*)> task) {
task.Run(extension.get());
}
that sort of thing, modulo whatever is actually correct.
On 2015/06/12 15:14:33, danduong wrote: > On 2015/06/12 14:55:06, kalman wrote: > > Actually can you point out exactly where this race condition is? Changing the > > function signature to an Extension shouldn't really change anything. > > RulesRegistryService::OnExtensionUninstalled() posts a tasks to > RulesRegistry::OnExtensionUninstalled on it's owner_thread(). > > I believe the extension can be deleted before the PostTask. Now if I changed the signature back to a string, that string will be copied on the PostTask so it would be valid. I still run into a little bit of a problem because I still need to access the ExtensionRegistry to get the extension when the extension is loaded. This doesn't seem completely thread safe, but it seems like we're already doing that kind of thing in the ChromeContentsRulesRegistry. We probably don't see an issue because it's really unlikely that an extension is getting uninstalled while it is loading, but it still seems like an existing issue.
On 2015/06/12 15:19:41, danduong wrote: > On 2015/06/12 15:14:33, danduong wrote: > > On 2015/06/12 14:55:06, kalman wrote: > > > Actually can you point out exactly where this race condition is? Changing > the > > > function signature to an Extension shouldn't really change anything. > > > > RulesRegistryService::OnExtensionUninstalled() posts a tasks to > > RulesRegistry::OnExtensionUninstalled on it's owner_thread(). > > > > I believe the extension can be deleted before the PostTask. > > Now if I changed the signature back to a string, that string will be copied on > the PostTask so it would be valid. > > I still run into a little bit of a problem because I still need to access the > ExtensionRegistry to get the extension when the extension is loaded. This > doesn't seem completely thread safe, but it seems like we're already doing that > kind of thing in the ChromeContentsRulesRegistry. We probably don't see an issue > because it's really unlikely that an extension is getting uninstalled while it > is loading, but it still seems like an existing issue. Does make_scoped_refptr work on const Extension*?
Yes On Jun 12, 2015 8:21 AM, <danduong@chromium.org> wrote: > On 2015/06/12 15:19:41, danduong wrote: > >> On 2015/06/12 15:14:33, danduong wrote: >> > On 2015/06/12 14:55:06, kalman wrote: >> > > Actually can you point out exactly where this race condition is? >> Changing >> the >> > > function signature to an Extension shouldn't really change anything. >> > >> > RulesRegistryService::OnExtensionUninstalled() posts a tasks to >> > RulesRegistry::OnExtensionUninstalled on it's owner_thread(). >> > >> > I believe the extension can be deleted before the PostTask. >> > > Now if I changed the signature back to a string, that string will be >> copied on >> the PostTask so it would be valid. >> > > I still run into a little bit of a problem because I still need to access >> the >> ExtensionRegistry to get the extension when the extension is loaded. This >> doesn't seem completely thread safe, but it seems like we're already doing >> > that > >> kind of thing in the ChromeContentsRulesRegistry. We probably don't see an >> > issue > >> because it's really unlikely that an extension is getting uninstalled >> while it >> is loading, but it still seems like an existing issue. >> > > Does make_scoped_refptr work on const Extension*? > > https://codereview.chromium.org/1158693006/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
That only seems to work if I const_cast is that what you're suggesting?
Mm ok, extension pointers are only ever passed around as const pointers. Apologies if I didn't pick that up. On Jun 12, 2015 8:40 AM, <danduong@chromium.org> wrote: > That only seems to work if I const_cast is that what you're suggesting? > > https://codereview.chromium.org/1158693006/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
The CQ bit was checked by danduong@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from wittman@chromium.org, kalman@chromium.org Link to the patchset: https://codereview.chromium.org/1158693006/#ps400001 (title: "add thread safety to extensionregistry notifications")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1158693006/400001
On 2015/06/12 15:14:33, danduong wrote: > On 2015/06/12 14:55:06, kalman wrote: > > Actually can you point out exactly where this race condition is? Changing the > > function signature to an Extension shouldn't really change anything. > > RulesRegistryService::OnExtensionUninstalled() posts a tasks to > RulesRegistry::OnExtensionUninstalled on it's owner_thread(). Which RulesRegistry subclass is this? The ChromeContentRulesRegistry at least should be running on the UI thread and should get a direct call. > I believe the extension can be deleted before the PostTask.
On 2015/06/12 16:32:14, Mike Wittman wrote: > On 2015/06/12 15:14:33, danduong wrote: > > On 2015/06/12 14:55:06, kalman wrote: > > > Actually can you point out exactly where this race condition is? Changing > the > > > function signature to an Extension shouldn't really change anything. > > > > RulesRegistryService::OnExtensionUninstalled() posts a tasks to > > RulesRegistry::OnExtensionUninstalled on it's owner_thread(). > > Which RulesRegistry subclass is this? The ChromeContentRulesRegistry at least > should be running on the UI thread and should get a direct call. > > > I believe the extension can be deleted before the PostTask. It was the WebRequest Registry that was crashing. I believe that lives on IO thread.
Message was sent while issue was closed.
Committed patchset #21 (id:400001)
Message was sent while issue was closed.
Patchset 21 (id:??) landed as https://crrev.com/a79e151dfd2c6e8c402bd0b6c296f8ea8e97b338 Cr-Commit-Position: refs/heads/master@{#334190} |
