Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11569007:
Refactoring how conditions without URL attributes are handled. (Closed)
|
Created:
8 years ago by vabr (Chromium) Modified:
7 years, 11 months ago CC:
chromium-reviews, Aaron Boodman, chromium-apps-reviews_chromium.org, Jeffrey Yasskin Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionRefactoring how conditions without URL attributes are handled.
BUG=159576
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=176923
Patch Set 1 : Polished 1st version #
Total comments: 44
Patch Set 2 : Refactor of GetMatches, IsFulfilled etc. #
Total comments: 12
Patch Set 3 : Getting rid of IsFulfilledIndependentlyOfURL #
Total comments: 4
Patch Set 4 : Nits from Jeffrey addressed #
Total comments: 8
Patch Set 5 : Triggers back in ConditionSet #
Total comments: 9
Patch Set 6 : Range heuristics in IsFulfilled + renaming match_triggers_ to match_id_to_condition_ #
Total comments: 7
Patch Set 7 : Rewritten GetMatches and added the trigger ID to Set::IsFulfilled #
Total comments: 20
Patch Set 8 : GetMatches fixed, unit test added, trigger in Condition #
Total comments: 2
Patch Set 9 : Typo #
Total comments: 10
Patch Set 10 : Trigger from Condition.IsFulfilled removed #Messages
Total messages: 29 (0 generated)
+Jeffrey I heard he is working on some refactoring as well. Hi Dominic, This CL contains the refactoring you requested in https://codereview.chromium.org/11414230/#msg4. Only after speaking to you I realised I misunderstood the structure a bit. As a result I had to shift slightly from what you proposed originally. Let me recap: Structure: Each rule has one condition set (1:1 correspondence). Each condition set consists of a disjunction of conditions. Each condition consists of a conjunction of attributes. Some of these attributes can be UrlFilter (currently only 'url'). Old situation: Checking of condition sets was triggered through URL matching -- the rules registry had a map from matched UrlFilters to rules which contained conditions involving these UrlFilters. We asked the URLMatcher which UrlFilters match, and then tested the rest of attributes in each involved condition. To make this work for each condition, we had to add a dummy (hostPrefix = '') UrlFilter to conditions which did not have any. Goal (new situation): Do not trigger checking of conditions through URL matching when the conditions do not have (non-trivial) UrlFilter attributes. Implementation: The rule registry now keeps the set of all rules, which have at least one condition without a UrlFilter attribute, in their condition sets. To determine which rules should fire for a request, these steps are done: 1. NEW: Go through all rules which have some condition without UrlFilter attributes. Check whether any such condition is fulfilled (ignore the conditions which have UrlFilter attributes for now), if yes, remember that rule. 2. Do the URL matching, and follow the "rule_triggers_" to see which rules have fulfilled UrlFilter attributes. Ignore each such rule already found via 1., for all other rules, do the check of the rest of attributes in the URL-matching conditions. The difference against what I understood in your original proposal is that rules which have both types of conditions (both involving and not involving UrlFilter attributes) are considered in step 1. I found no reason to exclude them, but feel free to give me some. Anyway, PTAL. Thanks! Vaclav https://codereview.chromium.org/11569007/diff/2001/chrome/browser/extensions/... File chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.cc (left): https://codereview.chromium.org/11569007/diff/2001/chrome/browser/extensions/... chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.cc:52: CHECK(url_matcher_conditions.get()); |url_matcher_conditions| being NULL is now legal and means that there were no UrlFilter attributes in this condition. https://codereview.chromium.org/11569007/diff/2001/chrome/browser/extensions/... File chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.h (right): https://codereview.chromium.org/11569007/diff/2001/chrome/browser/extensions/... chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.h:63: CHECK(url_matcher_conditions_.get()); This check is needed because now it is possible to have |url_matcher_conditions_| NULL -- that happens exactly when there are no UrlFilter attributes in this condition. I went for CHECK instead of a graceful exit, because if the code asks for ID in a code-path where there may be no UrlFilter condition attributes, then something is wrong there. https://codereview.chromium.org/11569007/diff/2001/chrome/browser/extensions/... chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.h:144: typedef std::map<URLMatcherConditionSet::ID, WebRequestCondition*> Typedefs should be before variables, so I reshuffled this. https://codereview.chromium.org/11569007/diff/2001/chrome/browser/extensions/... File chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry.cc (right): https://codereview.chromium.org/11569007/diff/2001/chrome/browser/extensions/... chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry.cc:51: if (result.find(rule->id()) == result.end() && This check spares time, because some rules might both have a condition with and a condition without a UrlFilter attribute in their condition set. If the latter is fulfilled, then we no longer have to test the former here. https://codereview.chromium.org/11569007/diff/2001/chrome/browser/extensions/... File chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry.h (right): https://codereview.chromium.org/11569007/diff/2001/chrome/browser/extensions/... chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry.h:104: rules_with_untriggered_conditions_for_test() const { Here I'm not sure about the indenting. I've seen often in the *.cc files that if the return type + function name were too long, the name appeared on a separate line unindented. But I could not find anything like that in the style guide, so I left 4 spaces here.
Sorry for not getting to this sooner. Here are some thoughts: https://codereview.chromium.org/11569007/diff/2001/chrome/browser/extensions/... File chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.cc (right): https://codereview.chromium.org/11569007/diff/2001/chrome/browser/extensions/... chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.cc:60: bool WebRequestCondition::IsFulfilled( It would be nice to somehow DCHECK here that the URLMatcher was actually applied and matches the right URLs. https://codereview.chromium.org/11569007/diff/2001/chrome/browser/extensions/... chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.cc:78: if (url_matcher_conditions_.get()) You don't need the .get() for use inside an if() anymore. https://codereview.chromium.org/11569007/diff/2001/chrome/browser/extensions/... chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.cc:185: for (Conditions::const_iterator i = conditions_.begin(); Do you want to check all conditions here, or just the ones that don't have URL attributes? https://codereview.chromium.org/11569007/diff/2001/chrome/browser/extensions/... chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.cc:205: bool WebRequestConditionSet::IsSomeConditionWithoutUrlMatcher( s/IsSome/Has/, I think. https://codereview.chromium.org/11569007/diff/2001/chrome/browser/extensions/... chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.cc:205: bool WebRequestConditionSet::IsSomeConditionWithoutUrlMatcher( Since this doesn't use any class-private data, I'd just make it a local function in this .cc file inside an anonymous namespace. https://codereview.chromium.org/11569007/diff/2001/chrome/browser/extensions/... File chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.h (right): https://codereview.chromium.org/11569007/diff/2001/chrome/browser/extensions/... chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.h:63: CHECK(url_matcher_conditions_.get()); On 2012/12/13 12:18:06, vabr (Chromium) wrote: > This check is needed because now it is possible to have > |url_matcher_conditions_| NULL -- that happens exactly when there are no > UrlFilter attributes in this condition. I went for CHECK instead of a graceful > exit, because if the code asks for ID in a code-path where there may be no > UrlFilter condition attributes, then something is wrong there. I believe Chrome convention is to DCHECK in this case, because the result would be a NULL-pointer dereference, which isn't a security bug. https://codereview.chromium.org/11569007/diff/2001/chrome/browser/extensions/... chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.h:139: bool has_untriggered_conditions() const { "untriggered" isn't a great word for this. Perhaps "has_conditions_without_urls()"? https://codereview.chromium.org/11569007/diff/2001/chrome/browser/extensions/... File chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_unittest.cc (right): https://codereview.chromium.org/11569007/diff/2001/chrome/browser/extensions/... chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_unittest.cc:189: // Check that the empty condition reports success independently of the URL. Please check a non-empty condition that doesn't have a URL too. If IsFulfilledWithoutURLMatcher is supposed to return false for conditions _with_ url matchers, please test that too. Probably put these tests into a separate TEST() so that you don't have to modify the existing test to add it and so that failures in this part of the implementation are distinct from failures in the rest. https://codereview.chromium.org/11569007/diff/2001/chrome/browser/extensions/... File chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry.cc (left): https://codereview.chromium.org/11569007/diff/2001/chrome/browser/extensions/... chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry.cc:198: // Remove reference to actual rule. This comment should probably have read "Remove owning reference to actual rule, which deletes the rule." https://codereview.chromium.org/11569007/diff/2001/chrome/browser/extensions/... File chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry.cc (right): https://codereview.chromium.org/11569007/diff/2001/chrome/browser/extensions/... chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry.cc:29: // First check for rules with a satisfied condition set without URL Given the next change you're making, and the declarativeContent refactoring I'm working on, I think I would rearrange this function: 1) Collect a set of rules that _might_ match, given the set of URLMatcherConditionSet::IDs that MatchURL returns. That would be roughly like: std::set<const WebRequestRule*> rules_to_check = rules_with_untriggered_conditions; typedef std::set<URLMatcherConditionSet::ID> URLMatches; URLMatches url_matches = url_matcher_.MatchURL(request_data.request->url()); for (URLMatches::const_iterator url_match = url_matches.begin(); url_match != url_matches.end(); ++url_match) { RuleTriggers::const_iterator rule_trigger = rule_triggers_.find(*url_match); CHECK(rule_trigger != rule_triggers_.end()); rules_to_check.insert(rule_trigger->second); } // Now rules_to_check is all the rules that might match. 2) Check each of these rules against the request-data-and-matching-URLMatcherConditionSet::IDs: for (std::set<const WebRequestRule*>::iterator it = rules_to_check.begin(); it != rules_to_check.end(); ) { if (rule->conditions().IsFulfilled(url_matches, request_data)) ++it; else it = rules_to_check.erase(it); } return rules_to_check; Note that IsFulfilled() now takes the full set of matching IDs, and it becomes responsible for checking whether each condition's ID is in that set. Your https://codereview.chromium.org/11414230/ will add a second set<ID> argument, and my https://codereview.chromium.org/11572061/ will move all of these set<ID>s into the MatchData structs. Sound reasonable? https://codereview.chromium.org/11569007/diff/2001/chrome/browser/extensions/... chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry.cc:214: rules_with_untriggered_conditions_.erase(rule); This ought to happen before webrequest_rules_.erase() because that actually deletes the rule. Nothing will break immediately if you remove the pointer from the set afterwards, but it makes the code more brittle. https://codereview.chromium.org/11569007/diff/2001/chrome/browser/extensions/... File chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry.h (right): https://codereview.chromium.org/11569007/diff/2001/chrome/browser/extensions/... chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry.h:104: rules_with_untriggered_conditions_for_test() const { On 2012/12/13 12:18:06, vabr (Chromium) wrote: > Here I'm not sure about the indenting. I've seen often in the *.cc files that if > the return type + function name were too long, the name appeared on a separate > line unindented. But I could not find anything like that in the style guide, so > I left 4 spaces here. My editor doesn't indent the function name, so I'd tend to avoid indenting it. I'd also generally avoid defining a private getter for a private variable. Code that could call the getter can just access the variable directly.
Started my review yesterday but did not finish it completely. I hope that my comments don't replicate too much of what Jeffrey stated. (Sending the current state before a day full of meetings) Best regards, Dominic https://codereview.chromium.org/11569007/diff/2001/chrome/browser/extensions/... File chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.cc (left): https://codereview.chromium.org/11569007/diff/2001/chrome/browser/extensions/... chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.cc:52: CHECK(url_matcher_conditions.get()); On 2012/12/13 12:18:06, vabr (Chromium) wrote: > |url_matcher_conditions| being NULL is now legal and means that there were no > UrlFilter attributes in this condition. Can you please document this? https://codereview.chromium.org/11569007/diff/2001/chrome/browser/extensions/... File chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.cc (right): https://codereview.chromium.org/11569007/diff/2001/chrome/browser/extensions/... chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.cc:162: i != conditions_.end(); nit: no new line for consistency. https://codereview.chromium.org/11569007/diff/2001/chrome/browser/extensions/... chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.cc:165: continue; nit: I'd propose to invert the logic: if ((*i)->url_matcher_condition_set()) { ... } https://codereview.chromium.org/11569007/diff/2001/chrome/browser/extensions/... File chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.h (right): https://codereview.chromium.org/11569007/diff/2001/chrome/browser/extensions/... chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.h:52: bool IsFulfilled(const WebRequestRule::RequestData& request_data) const; nit: can you add a new line here, please? https://codereview.chromium.org/11569007/diff/2001/chrome/browser/extensions/... chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.h:149: static bool IsSomeConditionWithoutUrlMatcher( nit: ContainsConditionWithoutUrlMatcher? https://codereview.chromium.org/11569007/diff/2001/chrome/browser/extensions/... File chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry.cc (right): https://codereview.chromium.org/11569007/diff/2001/chrome/browser/extensions/... chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry.cc:51: if (result.find(rule->id()) == result.end() && Use ContainsKey from base/stl_util.h? https://codereview.chromium.org/11569007/diff/2001/chrome/browser/extensions/... File chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry.h (right): https://codereview.chromium.org/11569007/diff/2001/chrome/browser/extensions/... chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry.h:104: rules_with_untriggered_conditions_for_test() const { On 2012/12/13 12:18:06, vabr (Chromium) wrote: > Here I'm not sure about the indenting. I've seen often in the *.cc files that if > the return type + function name were too long, the name appeared on a separate > line unindented. But I could not find anything like that in the style guide, so > I left 4 spaces here. I have seen both styles. I think that no spaces is predominant but did not do any analysis.
https://codereview.chromium.org/11569007/diff/2001/chrome/browser/extensions/... File chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry.cc (right): https://codereview.chromium.org/11569007/diff/2001/chrome/browser/extensions/... chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry.cc:29: // First check for rules with a satisfied condition set without URL On 2012/12/18 02:24:32, Jeffrey Yasskin wrote: > Given the next change you're making, and the declarativeContent refactoring I'm > working on, I think I would rearrange this function: > > 1) Collect a set of rules that _might_ match, given the set of > URLMatcherConditionSet::IDs that MatchURL returns. That would be roughly like: > > std::set<const WebRequestRule*> rules_to_check = > rules_with_untriggered_conditions; > > typedef std::set<URLMatcherConditionSet::ID> URLMatches; > URLMatches url_matches = url_matcher_.MatchURL(request_data.request->url()); > for (URLMatches::const_iterator url_match = url_matches.begin(); > url_match != url_matches.end(); ++url_match) { > RuleTriggers::const_iterator rule_trigger = rule_triggers_.find(*url_match); > CHECK(rule_trigger != rule_triggers_.end()); > rules_to_check.insert(rule_trigger->second); > } > > // Now rules_to_check is all the rules that might match. > > > 2) Check each of these rules against the > request-data-and-matching-URLMatcherConditionSet::IDs: > > for (std::set<const WebRequestRule*>::iterator it = rules_to_check.begin(); > it != rules_to_check.end(); ) { > if (rule->conditions().IsFulfilled(url_matches, request_data)) > ++it; > else > it = rules_to_check.erase(it); > } > > return rules_to_check; > > > Note that IsFulfilled() now takes the full set of matching IDs, and it becomes > responsible for checking whether each condition's ID is in that set. Your > https://codereview.chromium.org/11414230/ will add a second set<ID> argument, > and my https://codereview.chromium.org/11572061/ will move all of these set<ID>s > into the MatchData structs. > > Sound reasonable? This is a very elegant way of dealing with this. SGTM.
Hi Dominic and Jeffrey, Thanks for your very helpful comments. I did my best to address them, please take a look. Vaclav https://codereview.chromium.org/11569007/diff/2001/chrome/browser/extensions/... File chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.cc (left): https://codereview.chromium.org/11569007/diff/2001/chrome/browser/extensions/... chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.cc:52: CHECK(url_matcher_conditions.get()); On 2012/12/18 08:20:44, battre wrote: > On 2012/12/13 12:18:06, vabr (Chromium) wrote: > > |url_matcher_conditions| being NULL is now legal and means that there were no > > UrlFilter attributes in this condition. > > Can you please document this? Done, at the declaration of |url_matcher_conditions_|. https://codereview.chromium.org/11569007/diff/2001/chrome/browser/extensions/... File chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.cc (right): https://codereview.chromium.org/11569007/diff/2001/chrome/browser/extensions/... chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.cc:60: bool WebRequestCondition::IsFulfilled( On 2012/12/18 02:24:32, Jeffrey Yasskin wrote: > It would be nice to somehow DCHECK here that the URLMatcher was actually applied > and matches the right URLs. I'm not sure that I understand. Are you suggesting to move the check DCHECK_EQ(url_match, trigger->second->url_matcher_condition_set_id()); from WebRequestConditionSet::IsFulfilled to here? EDIT: After the refactoring, there is no more that DCHECK in WebRequestConditionSet::IsFulfilled. Still, do you want me to pass the iterator to URL matches here to re-check? https://codereview.chromium.org/11569007/diff/2001/chrome/browser/extensions/... chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.cc:78: if (url_matcher_conditions_.get()) On 2012/12/18 02:24:32, Jeffrey Yasskin wrote: > You don't need the .get() for use inside an if() anymore. That's true. Unfortunately there are 4 ways to write this (https://groups.google.com/a/chromium.org/d/msg/chromium-dev/ASSnFysgpsg/hr6-w...), and the ".get()" looks like locally consistent. https://codereview.chromium.org/11569007/diff/2001/chrome/browser/extensions/... chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.cc:162: i != conditions_.end(); On 2012/12/18 08:20:44, battre wrote: > nit: no new line for consistency. Done. https://codereview.chromium.org/11569007/diff/2001/chrome/browser/extensions/... chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.cc:165: continue; On 2012/12/18 08:20:44, battre wrote: > nit: I'd propose to invert the logic: > if ((*i)->url_matcher_condition_set()) { > ... > } Done. https://codereview.chromium.org/11569007/diff/2001/chrome/browser/extensions/... chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.cc:185: for (Conditions::const_iterator i = conditions_.begin(); On 2012/12/18 02:24:32, Jeffrey Yasskin wrote: > Do you want to check all conditions here, or just the ones that don't have URL > attributes? Just the ones without URL attributes. The for-cycle goes over all conditions, but IsFulfilledIndependentlyOfURL evaluates to false quickly for each condition with a URL attribute. EDIT: After the refactoring, this function is gone. https://codereview.chromium.org/11569007/diff/2001/chrome/browser/extensions/... chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.cc:205: bool WebRequestConditionSet::IsSomeConditionWithoutUrlMatcher( On 2012/12/18 02:24:32, Jeffrey Yasskin wrote: > s/IsSome/Has/, I think. Done. https://codereview.chromium.org/11569007/diff/2001/chrome/browser/extensions/... chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.cc:205: bool WebRequestConditionSet::IsSomeConditionWithoutUrlMatcher( On 2012/12/18 02:24:32, Jeffrey Yasskin wrote: > Since this doesn't use any class-private data, I'd just make it a local function > in this .cc file inside an anonymous namespace. Done. https://codereview.chromium.org/11569007/diff/2001/chrome/browser/extensions/... File chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.h (right): https://codereview.chromium.org/11569007/diff/2001/chrome/browser/extensions/... chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.h:52: bool IsFulfilled(const WebRequestRule::RequestData& request_data) const; On 2012/12/18 08:20:44, battre wrote: > nit: can you add a new line here, please? Done. https://codereview.chromium.org/11569007/diff/2001/chrome/browser/extensions/... chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.h:63: CHECK(url_matcher_conditions_.get()); On 2012/12/18 02:24:32, Jeffrey Yasskin wrote: > On 2012/12/13 12:18:06, vabr (Chromium) wrote: > > This check is needed because now it is possible to have > > |url_matcher_conditions_| NULL -- that happens exactly when there are no > > UrlFilter attributes in this condition. I went for CHECK instead of a graceful > > exit, because if the code asks for ID in a code-path where there may be no > > UrlFilter condition attributes, then something is wrong there. > > I believe Chrome convention is to DCHECK in this case, because the result would > be a NULL-pointer dereference, which isn't a security bug. TIL, thanks! Done. https://codereview.chromium.org/11569007/diff/2001/chrome/browser/extensions/... chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.h:139: bool has_untriggered_conditions() const { On 2012/12/18 02:24:32, Jeffrey Yasskin wrote: > "untriggered" isn't a great word for this. Perhaps > "has_conditions_without_urls()"? Done. https://codereview.chromium.org/11569007/diff/2001/chrome/browser/extensions/... chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.h:149: static bool IsSomeConditionWithoutUrlMatcher( On 2012/12/18 08:20:44, battre wrote: > nit: ContainsConditionWithoutUrlMatcher? Already changed due to Jeffrey's comment (HasConditionWithoutUrlMatcher, anonymous-namespace non-member function in the *.cc file). https://codereview.chromium.org/11569007/diff/2001/chrome/browser/extensions/... File chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_unittest.cc (right): https://codereview.chromium.org/11569007/diff/2001/chrome/browser/extensions/... chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_unittest.cc:189: // Check that the empty condition reports success independently of the URL. On 2012/12/18 02:24:32, Jeffrey Yasskin wrote: > Please check a non-empty condition that doesn't have a URL too. > > If IsFulfilledWithoutURLMatcher is supposed to return false for conditions > _with_ url matchers, please test that too. > > Probably put these tests into a separate TEST() so that you don't have to modify > the existing test to add it and so that failures in this part of the > implementation are distinct from failures in the rest. Done, as WebRequestConditionTest.IsFulfilledIndependentlyOfURL. https://codereview.chromium.org/11569007/diff/2001/chrome/browser/extensions/... File chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry.cc (left): https://codereview.chromium.org/11569007/diff/2001/chrome/browser/extensions/... chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry.cc:198: // Remove reference to actual rule. On 2012/12/18 02:24:32, Jeffrey Yasskin wrote: > This comment should probably have read "Remove owning reference to actual rule, > which deletes the rule." Corrected. https://codereview.chromium.org/11569007/diff/2001/chrome/browser/extensions/... File chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry.cc (right): https://codereview.chromium.org/11569007/diff/2001/chrome/browser/extensions/... chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry.cc:29: // First check for rules with a satisfied condition set without URL I implemented Jeffrey's suggestion. My only concern was performance, so I analysed the runtime of both versions of GetMatches. I can share the analysis with you if you are interested, but it's not a solid piece of computing :). The main problem here is that the run time depends a lot on which rules we get, how much URL attributes per rule, etc. There are families of collections of rules such that the old approach is asymptotically faster, and other families of collections such that the new approach is asymptotically faster. Another source of mess were constants for asymptotic complexities of things like set insertion. To decrease the imprecision as much as possible (but it is still way beyond talking seriously) I cancelled time for similar work done in both approaches (e.g., retrieving a rule from rule triggers for each URL match). After all that I got two formulas, and when I put some random characteristics of rule sets (e.g., 1000 rules, 800 of them have each 100 conditions with a single URL attribute) in those formulas, the results seemed to be very similar, even when I scaled the characteristics (multiplying the numbers by 100, 1000, etc.). Based on that shaky evidence, I hope this is not hurting the performance. https://codereview.chromium.org/11569007/diff/2001/chrome/browser/extensions/... chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry.cc:51: if (result.find(rule->id()) == result.end() && On 2012/12/18 08:20:44, battre wrote: > Use ContainsKey from base/stl_util.h? Code changed. But thanks, base/stl_util.h is a useful place to know about. :) https://codereview.chromium.org/11569007/diff/2001/chrome/browser/extensions/... chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry.cc:214: rules_with_untriggered_conditions_.erase(rule); On 2012/12/18 02:24:32, Jeffrey Yasskin wrote: > This ought to happen before webrequest_rules_.erase() because that actually > deletes the rule. Nothing will break immediately if you remove the pointer from > the set afterwards, but it makes the code more brittle. Makes sense, thanks for explaining that to me. I changed the order and corrected the comment. https://codereview.chromium.org/11569007/diff/2001/chrome/browser/extensions/... File chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry.h (right): https://codereview.chromium.org/11569007/diff/2001/chrome/browser/extensions/... chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry.h:104: rules_with_untriggered_conditions_for_test() const { On 2012/12/18 02:24:32, Jeffrey Yasskin wrote: > On 2012/12/13 12:18:06, vabr (Chromium) wrote: > > Here I'm not sure about the indenting. I've seen often in the *.cc files that > if > > the return type + function name were too long, the name appeared on a separate > > line unindented. But I could not find anything like that in the style guide, > so > > I left 4 spaces here. > > My editor doesn't indent the function name, so I'd tend to avoid indenting it. > > I'd also generally avoid defining a private getter for a private variable. Code > that could call the getter can just access the variable directly. For the private-getter: note that this is protected, accessing private data. It is used from a deriving class' code, so it cannot be replaced by a direct access. It also restricts the access to const only, so I believe it makes sense here. https://codereview.chromium.org/11569007/diff/2001/chrome/browser/extensions/... chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry.h:104: rules_with_untriggered_conditions_for_test() const { On 2012/12/18 08:20:44, battre wrote: > On 2012/12/13 12:18:06, vabr (Chromium) wrote: > > Here I'm not sure about the indenting. I've seen often in the *.cc files that > if > > the return type + function name were too long, the name appeared on a separate > > line unindented. But I could not find anything like that in the style guide, > so > > I left 4 spaces here. > > I have seen both styles. I think that no spaces is predominant but did not do > any analysis. Thanks both for the advice on indenting. I changed to no spaces. https://codereview.chromium.org/11569007/diff/12002/chrome/browser/extensions... File chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.h (left): https://codereview.chromium.org/11569007/diff/12002/chrome/browser/extensions... chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.h:131: MatchTriggers; With refactoring the IsFulfilled() method, triggers are no longer needed inside WebRequestConditionSet, so getting rid of them. https://codereview.chromium.org/11569007/diff/12002/chrome/browser/extensions... File chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry.h (right): https://codereview.chromium.org/11569007/diff/12002/chrome/browser/extensions... chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry.h:71: std::set<const WebRequestRule*> GetMatches( If it is for some reason necessary to return (a set of) GlobalRuleID instead of a pointer to a rule, please let me know. Reverting to GlobalRuleID would add some more complexity to GetMatches, whereas changing the callsites of GetMatches did not seem to make anything ugly, so I went for returning the rule pointers.
Nice change. I think you can get rid of IsFulfilledIndependentlyOfURL now, but then LGTM. You probably should wait for Dominic's LGTM too though. https://codereview.chromium.org/11569007/diff/12002/chrome/browser/extensions... File chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.cc (right): https://codereview.chromium.org/11569007/diff/12002/chrome/browser/extensions... chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.cc:186: const URLMatcherConditionSet* set = (*i)->url_matcher_condition_set().get(); Can we put this logic inside Condition::IsFulfilled()? That is, you'd always pass a url_matches set into Condition::IsFulfilled(), it would check whatever it needs to on that set<ID> and RequestData, and IsFulfilledIndependentlyOfURL would go away. This will make my templatizing change easier since ConditionSet::IsFulfilled would go back to being a simple loop. https://codereview.chromium.org/11569007/diff/12002/chrome/browser/extensions... File chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.h (right): https://codereview.chromium.org/11569007/diff/12002/chrome/browser/extensions... chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.h:88: // The 'url' attribute of this condition. If NULL then there was no 'url' I'd say "Represents the 'url' attribute ..." https://codereview.chromium.org/11569007/diff/12002/chrome/browser/extensions... File chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_unittest.cc (right): https://codereview.chromium.org/11569007/diff/12002/chrome/browser/extensions... chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_unittest.cc:148: #define MATCHING_URL_FILTER \ It'll work to define this as a std::string, and concatenate it with '+' to the varying parts of the JSON string. https://codereview.chromium.org/11569007/diff/12002/chrome/browser/extensions... File chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry.h (right): https://codereview.chromium.org/11569007/diff/12002/chrome/browser/extensions... chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry.h:71: std::set<const WebRequestRule*> GetMatches( On 2012/12/18 18:26:42, vabr (Chromium) wrote: > If it is for some reason necessary to return (a set of) GlobalRuleID instead of > a pointer to a rule, please let me know. Reverting to GlobalRuleID would add > some more complexity to GetMatches, whereas changing the callsites of GetMatches > did not seem to make anything ugly, so I went for returning the rule pointers. The only risk here would be if WebRequestRules are destroyed before GetMatches returns them. I don't believe they can be, so returning pointers is cheaper and equally safe. https://codereview.chromium.org/11569007/diff/12002/chrome/browser/extensions... File chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry_unittest.cc (right): https://codereview.chromium.org/11569007/diff/12002/chrome/browser/extensions... chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry_unittest.cc:246: EXPECT_TRUE(matches_ids.find(std::make_pair(kExtensionId, kRuleId1)) != Use ContainsKey for this?
LGTM with 2 nits https://codereview.chromium.org/11569007/diff/26007/chrome/browser/extensions... File chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.h (right): https://codereview.chromium.org/11569007/diff/26007/chrome/browser/extensions... chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.h:66: // primary trigger for WebRequestCondition and therefore never empty. This comment is out of date. https://codereview.chromium.org/11569007/diff/26007/chrome/browser/extensions... File chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_unittest.cc (right): https://codereview.chromium.org/11569007/diff/26007/chrome/browser/extensions... chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_unittest.cc:131: // There is no "first party for cookies URL in the requests below, Missing close-" in this comment.
Wow, Jeffrey, you are fast! Do you get noticed on a patch-set upload? :) Thanks a lot, both of you. Dominic, please feel free to take a look. Vaclav https://codereview.chromium.org/11569007/diff/12002/chrome/browser/extensions... File chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.cc (right): https://codereview.chromium.org/11569007/diff/12002/chrome/browser/extensions... chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.cc:186: const URLMatcherConditionSet* set = (*i)->url_matcher_condition_set().get(); On 2012/12/18 21:56:02, Jeffrey Yasskin wrote: > Can we put this logic inside Condition::IsFulfilled()? That is, you'd always > pass a url_matches set into Condition::IsFulfilled(), it would check whatever it > needs to on that set<ID> and RequestData, and IsFulfilledIndependentlyOfURL > would go away. > > This will make my templatizing change easier since ConditionSet::IsFulfilled > would go back to being a simple loop. Great idea, thanks! Done. https://codereview.chromium.org/11569007/diff/12002/chrome/browser/extensions... File chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.h (right): https://codereview.chromium.org/11569007/diff/12002/chrome/browser/extensions... chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.h:88: // The 'url' attribute of this condition. If NULL then there was no 'url' On 2012/12/18 21:56:02, Jeffrey Yasskin wrote: > I'd say "Represents the 'url' attribute ..." Done. https://codereview.chromium.org/11569007/diff/12002/chrome/browser/extensions... File chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_unittest.cc (right): https://codereview.chromium.org/11569007/diff/12002/chrome/browser/extensions... chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_unittest.cc:148: #define MATCHING_URL_FILTER \ On 2012/12/18 21:56:02, Jeffrey Yasskin wrote: > It'll work to define this as a std::string, and concatenate it with '+' to the > varying parts of the JSON string. Agreed, only this part of the test is no longer needed after getting rid of IsFulfilledIndependentlyOfURL, so I deleted it instead. https://codereview.chromium.org/11569007/diff/12002/chrome/browser/extensions... File chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry.h (right): https://codereview.chromium.org/11569007/diff/12002/chrome/browser/extensions... chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry.h:71: std::set<const WebRequestRule*> GetMatches( On 2012/12/18 21:56:02, Jeffrey Yasskin wrote: > On 2012/12/18 18:26:42, vabr (Chromium) wrote: > > If it is for some reason necessary to return (a set of) GlobalRuleID instead > of > > a pointer to a rule, please let me know. Reverting to GlobalRuleID would add > > some more complexity to GetMatches, whereas changing the callsites of > GetMatches > > did not seem to make anything ugly, so I went for returning the rule pointers. > > The only risk here would be if WebRequestRules are destroyed before GetMatches > returns them. I don't believe they can be, so returning pointers is cheaper and > equally safe. Oh, right. Yes, I hope the only place where rules are deleted is RemoveRulesImpl, which should not be running concurrently with GetMatches. Dominic, could you please confirm this? https://codereview.chromium.org/11569007/diff/12002/chrome/browser/extensions... File chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry_unittest.cc (right): https://codereview.chromium.org/11569007/diff/12002/chrome/browser/extensions... chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry_unittest.cc:246: EXPECT_TRUE(matches_ids.find(std::make_pair(kExtensionId, kRuleId1)) != On 2012/12/18 21:56:02, Jeffrey Yasskin wrote: > Use ContainsKey for this? Done. https://codereview.chromium.org/11569007/diff/26007/chrome/browser/extensions... File chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.h (right): https://codereview.chromium.org/11569007/diff/26007/chrome/browser/extensions... chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.h:66: // primary trigger for WebRequestCondition and therefore never empty. On 2012/12/19 08:31:23, Jeffrey Yasskin wrote: > This comment is out of date. Done. https://codereview.chromium.org/11569007/diff/26007/chrome/browser/extensions... File chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_unittest.cc (right): https://codereview.chromium.org/11569007/diff/26007/chrome/browser/extensions... chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_unittest.cc:131: // There is no "first party for cookies URL in the requests below, On 2012/12/19 08:31:23, Jeffrey Yasskin wrote: > Missing close-" in this comment. Done.
https://codereview.chromium.org/11569007/diff/29002/chrome/browser/extensions... File chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.cc (right): https://codereview.chromium.org/11569007/diff/29002/chrome/browser/extensions... chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.cc:187: return true; I continued thinking about this but I am not convinced anymore that this is the right thing to do. Say you have a condition set with 10,000 conditions (cancel the request if any of these 10,000 URL filters matches). This degrades from O(1) to O(10,000) here. It is ok if we have false positives but not 9,999 of them. https://codereview.chromium.org/11569007/diff/29002/chrome/browser/extensions... File chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.h (right): https://codereview.chromium.org/11569007/diff/29002/chrome/browser/extensions... chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.h:81: // Represents the 'url' attribute of this condition. If NULL then there was no NULL, https://codereview.chromium.org/11569007/diff/29002/chrome/browser/extensions... chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.h:120: // |url_matches|, and tests the rest against |request_data.request|. I find the second sentence hard to parse. Returns whether any condition in the condition set is fulfilled based on matches in |url_matches| and the value of |request_data.request|. This function should be called for a ConditionSet if any Condition in the ConditionSet was triggered by a url match or if the ConditionSet contains a Condition without a UrlFilter.
Hi Dominic, Thanks for your comments, and especially for the explanation about triggers. I returned the triggers and tried to deploy them without adding the old versions of IsFulfilled*. Jeffrey, Please have a look at how has_conditions_without_urls_ has been replaced by conditions_without_urls_ in the ConditionSet. Is that OK with you? Thanks for your patience and taking the burden of this parallel refactoring with me. Thanks to both of you, Vaclav https://codereview.chromium.org/11569007/diff/29002/chrome/browser/extensions... File chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.cc (right): https://codereview.chromium.org/11569007/diff/29002/chrome/browser/extensions... chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.cc:160: bool HasConditionWithoutUrlMatcher( This was completely moved to the Create method. https://codereview.chromium.org/11569007/diff/29002/chrome/browser/extensions... chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.cc:187: return true; On 2012/12/19 14:38:01, battre wrote: > I continued thinking about this but I am not convinced anymore that this is the > right thing to do. > > Say you have a condition set with 10,000 conditions (cancel the request if any > of these 10,000 URL filters matches). This degrades from O(1) to O(10,000) here. > It is ok if we have false positives but not 9,999 of them. I returned the triggers. They are now used to prioritise which conditions to test in ConditionSet::IsFulfilled. https://codereview.chromium.org/11569007/diff/29002/chrome/browser/extensions... File chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.h (right): https://codereview.chromium.org/11569007/diff/29002/chrome/browser/extensions... chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.h:81: // Represents the 'url' attribute of this condition. If NULL then there was no On 2012/12/19 14:38:01, battre wrote: > NULL, Done. https://codereview.chromium.org/11569007/diff/29002/chrome/browser/extensions... chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.h:103: explicit WebRequestConditionSet(const Conditions& conditions); Was there any reason to have this public? (This was my off-line question, Dominic.) It has not been used outside of Create, so I moved it to private. Also I added 2 more arguments, and those have a well defined relation to the first one. Giving wrong arguments messes things up, so I would prefer to have the constructor private. https://codereview.chromium.org/11569007/diff/29002/chrome/browser/extensions... chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.h:120: // |url_matches|, and tests the rest against |request_data.request|. On 2012/12/19 14:38:01, battre wrote: > I find the second sentence hard to parse. > > Returns whether any condition in the condition set is fulfilled based on matches > in |url_matches| and the value of |request_data.request|. This function should > be called for a ConditionSet if any Condition in the ConditionSet was triggered > by a url match or if the ConditionSet contains a Condition without a UrlFilter. Done.
lgtm https://codereview.chromium.org/11569007/diff/33001/chrome/browser/extensions... File chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.cc (right): https://codereview.chromium.org/11569007/diff/33001/chrome/browser/extensions... chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.cc:211: MatchTriggers match_triggers; <unactionable-complaining>I wish we had a better name for this. I see where it came from, but neither part of the name really explains what it's for. It maps URL match IDs to Conditions. All it "triggers" is the full IsFulfilled() check. Maybe "conditions_matching_url", but that doesn't really capture that it's a URLMatcher ID. Anyway, no need to fix this in this change. I'm just complaining. https://codereview.chromium.org/11569007/diff/33001/chrome/browser/extensions... chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.cc:229: return conditions_without_urls_.size() > 0u; You can use !conditions_without_urls_.empty() https://codereview.chromium.org/11569007/diff/33001/chrome/browser/extensions... File chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.h (right): https://codereview.chromium.org/11569007/diff/33001/chrome/browser/extensions... chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.h:134: typedef std::map<URLMatcherConditionSet::ID, const WebRequestCondition*> For optimal memory use and performance, this ought to be a sorted vector. That would also help optimize IsFulfilled, since we could search a smaller range as we go through the url_matches set, or even skip ahead in url_matches if the smallest ID for a condition is bigger than the current match ID. No need to optimize this now, since it'll make the code a bit less readable. https://codereview.chromium.org/11569007/diff/33001/chrome/browser/extensions... chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.h:140: const std::vector<const WebRequestCondition*> conditions_without_urls); You forgot an & on conditions_without_urls. (There's an argument for taking all of these arguments by value, but it requires that I write a base::move() function, which I haven't done yet.)
Thanks, Jeffrey, for your helpful comments. I addressed all of them, and would be interested in hearing your opinion. Also, Dominic, I'll wait for your opinion and comments. Anyway, it's probably time to admit that this won't land this year. :) Have a great holidays, guys, and I'm looking forward to working with you in the next year as well! Vaclav https://codereview.chromium.org/11569007/diff/33001/chrome/browser/extensions... File chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.cc (right): https://codereview.chromium.org/11569007/diff/33001/chrome/browser/extensions... chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.cc:211: MatchTriggers match_triggers; On 2012/12/19 19:10:52, Jeffrey Yasskin wrote: > <unactionable-complaining>I wish we had a better name for this. I see where it > came from, but neither part of the name really explains what it's for. It maps > URL match IDs to Conditions. All it "triggers" is the full IsFulfilled() check. > Maybe "conditions_matching_url", but that doesn't really capture that it's a > URLMatcher ID. Anyway, no need to fix this in this change. I'm just complaining. This makes sense to me. During the programming I thought of |match_triggers_| exactly as "id to condition lookup". I believe renaming it so makes more sense, and I went for |match_id_to_condition_|. Dominic, flagging this to you in case you want to object. https://codereview.chromium.org/11569007/diff/33001/chrome/browser/extensions... chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.cc:229: return conditions_without_urls_.size() > 0u; On 2012/12/19 19:10:52, Jeffrey Yasskin wrote: > You can use !conditions_without_urls_.empty() Done. https://codereview.chromium.org/11569007/diff/33001/chrome/browser/extensions... File chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.h (right): https://codereview.chromium.org/11569007/diff/33001/chrome/browser/extensions... chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.h:134: typedef std::map<URLMatcherConditionSet::ID, const WebRequestCondition*> On 2012/12/19 19:10:52, Jeffrey Yasskin wrote: > For optimal memory use and performance, this ought to be a sorted vector. That > would also help optimize IsFulfilled, since we could search a smaller range as > we go through the url_matches set, or even skip ahead in url_matches if the > smallest ID for a condition is bigger than the current match ID. No need to > optimize this now, since it'll make the code a bit less readable. I'm not sure I understand. If we iterate over a map, it already gives us the key-value pairs sorted by keys. What do you suggest to be the type of the vector elements, and how do you want to sort them? I tried to modify IsFulfilled to only explore the range of |url_matches| corresponding to the |match_triggers_|. https://codereview.chromium.org/11569007/diff/33001/chrome/browser/extensions... chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.h:140: const std::vector<const WebRequestCondition*> conditions_without_urls); On 2012/12/19 19:10:52, Jeffrey Yasskin wrote: > You forgot an & on conditions_without_urls. > > (There's an argument for taking all of these arguments by value, but it requires > that I write a base::move() function, which I haven't done yet.) Done.
LGTM (still :) https://codereview.chromium.org/11569007/diff/33001/chrome/browser/extensions... File chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.h (right): https://codereview.chromium.org/11569007/diff/33001/chrome/browser/extensions... chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.h:134: typedef std::map<URLMatcherConditionSet::ID, const WebRequestCondition*> On 2012/12/20 08:57:13, vabr (Chromium) wrote: > On 2012/12/19 19:10:52, Jeffrey Yasskin wrote: > > For optimal memory use and performance, this ought to be a sorted vector. That > > would also help optimize IsFulfilled, since we could search a smaller range as > > we go through the url_matches set, or even skip ahead in url_matches if the > > smallest ID for a condition is bigger than the current match ID. No need to > > optimize this now, since it'll make the code a bit less readable. > > I'm not sure I understand. If we iterate over a map, it already gives us the > key-value pairs sorted by keys. What do you suggest to be the type of the vector > elements, and how do you want to sort them? The vector's elements should be std::pair<URLMatcherConditionSet::ID, const WebRequestCondition*>, and they should be sorted by the ID, just like the map. You'd look things up using std::lower_bound. The reason to pick vector<> over map<> is just that map is big (~4 pointers of overhead per value, plus N/2 internal nodes) and slow (pointer-hopping == cache misses), and for immutable mappings none of vector<>'s downsides apply. BUT, switching to vector makes the code harder to read, so we really should only do it if this code shows up on a profile. I definitely don't want to delay checking this CL in by making you optimize it. > I tried to modify IsFulfilled to only explore the range of |url_matches| > corresponding to the |match_triggers_|.
Thanks, Jeffrey, I understand now your point with the vector, and agree that we should look into it if map proves to be slow. The code change itself will be minimal -- I already use lower_bound for the map, and in the constructor we would only get a vector-snapshot of the match_id_to_condition instead of copying it. Dominic, Once you are back in the office in January, please take a look. I also left one more comment. Cheers, Vaclav
https://codereview.chromium.org/11569007/diff/32040/chrome/browser/extensions... File chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.cc (right): https://codereview.chromium.org/11569007/diff/32040/chrome/browser/extensions... chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.cc:242: const Conditions& conditions, These three const references lead to a lot of copying, and the originals are destroyed just after the constructor returns. Should I use the swap method instead? That way I would copy the container objects, but only transfer the ownership of their data. To me it sounds like a good idea, but I don't remember seeing swap in the code much -- is there some reason for why this is perhaps discouraged?
On Dec 20, 2012 11:27 AM, <vabr@chromium.org> wrote: > > > https://codereview.chromium.org/11569007/diff/32040/chrome/browser/extensions... > File > chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.cc > (right): > > https://codereview.chromium.org/11569007/diff/32040/chrome/browser/extensions... > chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.cc:242: > const Conditions& conditions, > These three const references lead to a lot of copying, and the originals > are destroyed just after the constructor returns. > > Should I use the swap method instead? That way I would copy the > container objects, but only transfer the ownership of their data. > > To me it sounds like a good idea, but I don't remember seeing swap in > the code much -- is there some reason for why this is perhaps > discouraged? It's not widely known, and the pointer method is ugly and hard to read. There's another method that's easier to read but needs a helper function. Could you check it in as it is, and I'll send you a patch to optimize it. If you can measure a difference from the patch, that'll be a good argument to add the helper function to base.
> Could you check it in as it is, and I'll send you a patch to optimize it. SGTM. I will still wait for Dominic's overall approval of this CL before checking in, though. > If you can measure a difference from the patch, that'll be a good argument > to add the helper function to base. I'll gladly give it a try. Thanks, Vaclav
Jeffrey, please take a look at the suggested interface change. Thanks, Dominic https://codereview.chromium.org/11569007/diff/32040/chrome/browser/extensions... File chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.cc (right): https://codereview.chromium.org/11569007/diff/32040/chrome/browser/extensions... chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.cc:170: url_matches.lower_bound(first_id); nit: +4 spaces as in line 182. https://codereview.chromium.org/11569007/diff/32040/chrome/browser/extensions... chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.cc:178: } I discussed this with Vaclav and I am concerned that this may become extremely slow in the worst case because |url_matches| can be very large. Our suggestion was to change the signature to the following: bool WebRequestConditionSet::IsFulfilled( URLMatcherConditionSet::ID url_match_trigger, const std::set<URLMatcherConditionSet::ID>& url_matches, const WebRequestRule::RequestData& request_data) const; We would guarantee that this function is called - once for each trigger contained in the ConditionSet, and in addition - once with |url_match_trigger| == -1 in case the ConditionSet has conditions_without_urls_. until any ConditionSet is fulfilled. https://codereview.chromium.org/11569007/diff/32040/chrome/browser/extensions... File chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry.cc (right): https://codereview.chromium.org/11569007/diff/32040/chrome/browser/extensions... chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry.cc:49: } I think this can be simplified by just iterating over rules_with_untriggered_conditions_ and url_matches and only inserting what matches. I think it may be fewer lines of code and require less ressources.
Hi Jeffrey, A bit more details about the change proposed by Dominic: Each time IsFulfilled is called, it gets the set of all URL matches, and scans through it. This means that, in the worst case, the large set of all URL matches is traversed as many times, as there are rules. True, there is a heuristic suggested by you, scanning only the relevant range of the URL matches. But that relies on URL match set ids being assigned consecutive numbers. Dominic does not want to make this a requirement of the code. Out of several alternatives we considered, the best seem to be to pass optionally the triggering URL match in the signature of IsFulfilled again. Then, if called with a valid triggering URL match, IsFulfilled will check just the condition triggered by that URL match. If called without a valid triggering URL match, it will test exactly those conditions which have no URL attributes. In both cases it will ignore the set of all URL matches (if that stays in the signature). Vaclav
https://codereview.chromium.org/11569007/diff/32040/chrome/browser/extensions... File chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.cc (right): https://codereview.chromium.org/11569007/diff/32040/chrome/browser/extensions... chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.cc:178: } On 2013/01/07 17:36:27, battre wrote: > I discussed this with Vaclav and I am concerned that this may become extremely > slow in the worst case because |url_matches| can be very large. > > Our suggestion was to change the signature to the following: > bool WebRequestConditionSet::IsFulfilled( > URLMatcherConditionSet::ID url_match_trigger, > const std::set<URLMatcherConditionSet::ID>& url_matches, > const WebRequestRule::RequestData& request_data) const; > > We would guarantee that this function is called > > - once for each trigger contained in the ConditionSet, and in addition > > - once with |url_match_trigger| == -1 in case the ConditionSet has > conditions_without_urls_. > > until any ConditionSet is fulfilled. We can optimize this scan by putting the matches into a sorted vector and using a modified binary search to find the next one. That bounds the time at O(min(|match_id_to_condition| * log(|url_matches|), log(|match_id_to_condition|)*|url_matches|, |match_id_to_condition| + |url_matches|)). In the case you're worried about, with lots of url_matches and few conditions in each rule, that adds up to O(|url_matches|*log(|rules|) + |conditions|*log(|url_matches|)), while your suggestion is O(|url_matches|*log(|conditions|)). In practice the current code will be quite fast since IDs are actually allocated sequentially. I'd like to see a benchmark (micro or macro, but automated) showing the cases you're interested in before we spend too much time worrying about optimizations here. I imagine your suggestion will work with Vaclav's first_party_url change by using the passed-in ID to narrow to a condition, and then searching the first_party_url_match_set using that condition? I think the templating change will work either way, so if you want to pass in an ID, I don't object. https://codereview.chromium.org/11569007/diff/32040/chrome/browser/extensions... File chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry.cc (right): https://codereview.chromium.org/11569007/diff/32040/chrome/browser/extensions... chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry.cc:49: } On 2013/01/07 17:36:27, battre wrote: > I think this can be simplified by just iterating over > rules_with_untriggered_conditions_ and url_matches and only inserting what > matches. I think it may be fewer lines of code and require less ressources. Yes, that's cheaper. I had guessed that it would be more lines of code since you'd want to avoid checking IsFulfilled on rules you'd already checked, but I could be wrong.
Hi Jeffrey and Dominic, Sorry it's been taking me so long. I addressed the comments, in particular: 1. Rewrote the GetMatches to 2 phases instead of 3. 2. Put back the trigger ID to ConditionSet::IsFulfilled. As for 2, I understand the options hinted by Jeffrey were either adding the trigger ID, or implementing the |match_id_to_condition| as a vector of pairs instead of a map (in light also of his previous comments here: https://codereview.chromium.org/11569007/diff/33001/chrome/browser/extensions...). The latter only on the assumption that some speed benchmarks support that. I think that sort of refactoring can still be done later, and the former option looks like the best (or, at least bearable and fast). The range-selecting heuristics, on maps or vectors, will be back once I add the first-party-URL filter, at least for one of the URL matcher ID sets. Some minor changes here also come from running cpplint. PTAL. Vaclav https://codereview.chromium.org/11569007/diff/32040/chrome/browser/extensions... File chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry.cc (right): https://codereview.chromium.org/11569007/diff/32040/chrome/browser/extensions... chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry.cc:49: } On 2013/01/07 17:36:27, battre wrote: > I think this can be simplified by just iterating over > rules_with_untriggered_conditions_ and url_matches and only inserting what > matches. I think it may be fewer lines of code and require less ressources. Done. https://codereview.chromium.org/11569007/diff/46001/chrome/browser/extensions... File chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.cc (right): https://codereview.chromium.org/11569007/diff/46001/chrome/browser/extensions... chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.cc:65: !ContainsKey(url_matches, url_matcher_conditions_->id())) Here I could check equality instead of doing a look-up in url_matches, if I passed the trigger ID from ConditionSet. Is it worth it? https://codereview.chromium.org/11569007/diff/46001/chrome/browser/extensions... chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.cc:161: if (url_matches.find(url_match_trigger) == url_matches.end()) { An alternative would be to hard-code -1 as the invalid trigger ID. It does not matter too much, because once I add the first-party URL filter, I expect having to check in this way from where the trigger ID might have come from (url_matches or first_party_url_matches). Feel free to object, though. https://codereview.chromium.org/11569007/diff/46001/chrome/browser/extensions... File chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry_unittest.cc (right): https://codereview.chromium.org/11569007/diff/46001/chrome/browser/extensions... chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry_unittest.cc:484: TEST_F(WebRequestRulesRegistryTest, GetMatchesCheckFulfilled) { I almost forgot to check IsFulfilled in the second stage of GetMatches, so I though it best to add a unit test for that here.
https://codereview.chromium.org/11569007/diff/46001/chrome/browser/extensions... File chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.cc (right): https://codereview.chromium.org/11569007/diff/46001/chrome/browser/extensions... chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.cc:65: !ContainsKey(url_matches, url_matcher_conditions_->id())) On 2013/01/10 15:12:22, vabr (Chromium) wrote: > Here I could check equality instead of doing a look-up in url_matches, if I > passed the trigger ID from ConditionSet. > Is it worth it? IMO, only if the same logic will make sense after you add the first-party URL filter. https://codereview.chromium.org/11569007/diff/46001/chrome/browser/extensions... File chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.h (right): https://codereview.chromium.org/11569007/diff/46001/chrome/browser/extensions... chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.h:120: // it returns whether some of the conditions withou URL attributes are sp: withou https://codereview.chromium.org/11569007/diff/46001/chrome/browser/extensions... chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.h:120: // it returns whether some of the conditions withou URL attributes are s/some ... are/any ... is/ (assuming it does only require one.) https://codereview.chromium.org/11569007/diff/46001/chrome/browser/extensions... File chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry.cc (right): https://codereview.chromium.org/11569007/diff/46001/chrome/browser/extensions... chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry.cc:51: if (not_fulfilled.find(rule_trigger->second) == not_fulfilled.end() && Use ContainsKey from base/stl_util.h instead of find? https://codereview.chromium.org/11569007/diff/46001/chrome/browser/extensions... chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry.cc:51: if (not_fulfilled.find(rule_trigger->second) == not_fulfilled.end() && Is this right? Say there's a rule with conditions: [ {url: "google.com"}, {contentType: "neverMatch"} ]. Then this rule will end up in rules_with_untriggered_conditions_, and since it doesn't match with no URL, it'll end up in not_fulfilled. Now it comes up on a google.com request in this triggered loop and gets skipped because it wasn't fulfilled with no URL, even though it would be fulfilled with the current URL. Could you add a test for this? (This sort of bug or confusion-on-my-part is why I pushed for the simplest implementation rather than trying to optimize it by punching information through the abstraction layers.) https://codereview.chromium.org/11569007/diff/46001/chrome/browser/extensions... File chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry_unittest.cc (right): https://codereview.chromium.org/11569007/diff/46001/chrome/browser/extensions... chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry_unittest.cc:224: DictionaryValue condition_dict; FWIW, I would probably try to initialize this using base::test::ParseJson or extensions::DictionaryBuilder, to try to make the resulting value easier to read. https://codereview.chromium.org/11569007/diff/46001/chrome/browser/extensions... chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry_unittest.cc:245: make_linked_ptr(new RulesRegistry::Rule); Just use linked_ptr<RulesRegistry::Rule> rule(new RulesRegistry::Rule); make_linked_ptr is only helpful if you didn't otherwise need to write the type.
Cool. I think we are one after addressing the comments. https://codereview.chromium.org/11569007/diff/46001/chrome/browser/extensions... File chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.cc (right): https://codereview.chromium.org/11569007/diff/46001/chrome/browser/extensions... chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.cc:222: result, match_id_to_condition, conditions_without_urls)); How about moving lines 208-222 into the constructor of WebRequestConditionSet and reverting this line to line 206? https://codereview.chromium.org/11569007/diff/46001/chrome/browser/extensions... File chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry.cc (right): https://codereview.chromium.org/11569007/diff/46001/chrome/browser/extensions... chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry.cc:51: if (not_fulfilled.find(rule_trigger->second) == not_fulfilled.end() && On 2013/01/10 22:00:57, Jeffrey Yasskin wrote: > Is this right? Say there's a rule with conditions: [ {url: "google.com"}, > {contentType: "neverMatch"} ]. Then this rule will end up in > rules_with_untriggered_conditions_, and since it doesn't match with no URL, > it'll end up in not_fulfilled. Now it comes up on a http://google.com request in this > triggered loop and gets skipped because it wasn't fulfilled with no URL, even > though it would be fulfilled with the current URL. > > Could you add a test for this? (This sort of bug or confusion-on-my-part is why > I pushed for the simplest implementation rather than trying to optimize it by > punching information through the abstraction layers.) I think the test we want here is: if (!ContainsKey(result, rule_trigger->second) && rule_trigger->second->conditions().IsFulfilled(...)) If it is fulfilled already, we don't need to test any further.
Dominic, Jeffrey, Thanks for your quick responses, and sorry for the delays on my side. I hopefully addressed everything from Jeffrey, but there is one comment from Dominic (work in Create vs. constructor) which I was not sure about. Dominic, could you please take a look again? Thanks, Vaclav https://codereview.chromium.org/11569007/diff/46001/chrome/browser/extensions... File chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.cc (right): https://codereview.chromium.org/11569007/diff/46001/chrome/browser/extensions... chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.cc:65: !ContainsKey(url_matches, url_matcher_conditions_->id())) On 2013/01/10 22:00:57, Jeffrey Yasskin wrote: > On 2013/01/10 15:12:22, vabr (Chromium) wrote: > > Here I could check equality instead of doing a look-up in url_matches, if I > > passed the trigger ID from ConditionSet. > > Is it worth it? > > IMO, only if the same logic will make sense after you add the first-party URL > filter. I believe it makes sense even after we have multiple URL attributes -- the trigger will be checked against the URL matcher sets associated with each URL attribute. If matching, we will skip full search in the corresponding URL matches for that attribute, otherwise not. So I added the trigger. https://codereview.chromium.org/11569007/diff/46001/chrome/browser/extensions... chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.cc:222: result, match_id_to_condition, conditions_without_urls)); On 2013/01/11 17:41:55, battre wrote: > How about moving lines 208-222 into the constructor of WebRequestConditionSet > and reverting this line to line 206? The reason why I put this to Create was to keep conditions_without_urls_ a constant data member. Since the constructor is private, I don't feel a danger in having assumptions about the passed arguments (i.e., that considions_without_urls are really conditions without URL attributes. I'm happy to move this to the constructor, just curious about the benefits. :) https://codereview.chromium.org/11569007/diff/46001/chrome/browser/extensions... File chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.h (right): https://codereview.chromium.org/11569007/diff/46001/chrome/browser/extensions... chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.h:120: // it returns whether some of the conditions withou URL attributes are On 2013/01/10 22:00:57, Jeffrey Yasskin wrote: > sp: withou Done. https://codereview.chromium.org/11569007/diff/46001/chrome/browser/extensions... chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.h:120: // it returns whether some of the conditions withou URL attributes are On 2013/01/10 22:00:57, Jeffrey Yasskin wrote: > s/some ... are/any ... is/ (assuming it does only require one.) Done. https://codereview.chromium.org/11569007/diff/46001/chrome/browser/extensions... File chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry.cc (right): https://codereview.chromium.org/11569007/diff/46001/chrome/browser/extensions... chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry.cc:51: if (not_fulfilled.find(rule_trigger->second) == not_fulfilled.end() && On 2013/01/11 17:41:55, battre wrote: > On 2013/01/10 22:00:57, Jeffrey Yasskin wrote: > > Is this right? Say there's a rule with conditions: [ {url: "google.com"}, > > {contentType: "neverMatch"} ]. Then this rule will end up in > > rules_with_untriggered_conditions_, and since it doesn't match with no URL, > > it'll end up in not_fulfilled. Now it comes up on a http://google.com request > in this > > triggered loop and gets skipped because it wasn't fulfilled with no URL, even > > though it would be fulfilled with the current URL. > > > > Could you add a test for this? (This sort of bug or confusion-on-my-part is > why > > I pushed for the simplest implementation rather than trying to optimize it by > > punching information through the abstraction layers.) > > I think the test we want here is: > if (!ContainsKey(result, rule_trigger->second) && > rule_trigger->second->conditions().IsFulfilled(...)) > > If it is fulfilled already, we don't need to test any further. Thanks, Jeffrey, for pointing this out, this was a stupid mistake of mine. I did as Dominic suggested, and also added the unit test. https://codereview.chromium.org/11569007/diff/46001/chrome/browser/extensions... File chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry_unittest.cc (right): https://codereview.chromium.org/11569007/diff/46001/chrome/browser/extensions... chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry_unittest.cc:224: DictionaryValue condition_dict; On 2013/01/10 22:00:57, Jeffrey Yasskin wrote: > FWIW, I would probably try to initialize this using base::test::ParseJson or > extensions::DictionaryBuilder, to try to make the resulting value easier to > read. Done. The way I did it I separated the condition string into the boilerplate and the interesting stuff (see CreateCondition). That might potentially harm readability, but personally I found it helpful, mostly for the removal of code duplication. Dominic, do you want me to convert also the other rule creating methods here to use ParseJson? The only possible drawback I see is replacing of key::... constants by hard-coded strings, but I don't see that as a maintenance problem, because the constant's name is always made dependent on its content (so changing the definition of the constant means also changing its name everywhere). https://codereview.chromium.org/11569007/diff/46001/chrome/browser/extensions... chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry_unittest.cc:245: make_linked_ptr(new RulesRegistry::Rule); On 2013/01/10 22:00:57, Jeffrey Yasskin wrote: > Just use > linked_ptr<RulesRegistry::Rule> rule(new RulesRegistry::Rule); > make_linked_ptr is only helpful if you didn't otherwise need to write the type. Oh, right, thanks for catching this. I corrected this on other places in this file as well (from where I copy-pasted that). https://codereview.chromium.org/11569007/diff/52001/chrome/browser/extensions... File chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.cc (right): https://codereview.chromium.org/11569007/diff/52001/chrome/browser/extensions... chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.cc:64: if (!(request_data.stage & applicable_request_stages_)) { This check is always very quick, so it made sense to me to put it first. https://codereview.chromium.org/11569007/diff/52001/chrome/browser/extensions... chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.cc:74: // All condition attributes must be fulfilled for a fulfilled condition. This comment looked misplaced to me, so I moved it where I felt it fits best.
https://codereview.chromium.org/11569007/diff/46001/chrome/browser/extensions... File chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.cc (right): https://codereview.chromium.org/11569007/diff/46001/chrome/browser/extensions... chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.cc:222: result, match_id_to_condition, conditions_without_urls)); On 2013/01/15 10:24:57, vabr (Chromium) wrote: > On 2013/01/11 17:41:55, battre wrote: > > How about moving lines 208-222 into the constructor of WebRequestConditionSet > > and reverting this line to line 206? > > The reason why I put this to Create was to keep conditions_without_urls_ a > constant data member. Since the constructor is private, I don't feel a danger in > having assumptions about the passed arguments (i.e., that > considions_without_urls are really conditions without URL attributes. > > I'm happy to move this to the constructor, just curious about the benefits. :) OK, that's a good argument. I felt that it would logically belong into the constructor. so that match_id_to_condition and conditions_without_urls become implementation details. But you can leave it here as well. https://codereview.chromium.org/11569007/diff/58001/chrome/browser/extensions... File chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.cc (right): https://codereview.chromium.org/11569007/diff/58001/chrome/browser/extensions... chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.cc:61: URLMatcherConditionSet::ID url_match_trigger, I would suggest to remove this line. https://codereview.chromium.org/11569007/diff/58001/chrome/browser/extensions... chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.cc:71: (url_match_trigger != url_matcher_conditions_->id())) and change this to if (url_matcher_conditions_.get() && !ContainsKey(url_matches, url_matcher_conditions_->id) Then the comment in the function header can be simplified. The reason for this is that the WebRequestCondition will only have one match trigger, right? https://codereview.chromium.org/11569007/diff/58001/chrome/browser/extensions... chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.cc:162: if (url_matches.find(url_match_trigger) == url_matches.end()) { change this to "if (url_match_trigger == -1)"? else: nit: simplify to ContainsKey? https://codereview.chromium.org/11569007/diff/58001/chrome/browser/extensions... File chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.h (right): https://codereview.chromium.org/11569007/diff/58001/chrome/browser/extensions... chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.h:124: // satisfied. I would change the Else... sentence to: "If |url_match_trigger| is -1, this function returns whether any of the conditions without URL attributes is satisfied." and change the implementation accordingly. https://codereview.chromium.org/11569007/diff/58001/chrome/browser/extensions... File chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry_unittest.cc (right): https://codereview.chromium.org/11569007/diff/58001/chrome/browser/extensions... chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry_unittest.cc:242: attributes.push_back("\"url\": { \"pathContains\": \"\" }, \n"); nit: please add a new line after this line (also below)
Hi Dominic, Thanks for a quick turnaround! Addressed all your comments. How does that look to you? Vaclav https://codereview.chromium.org/11569007/diff/58001/chrome/browser/extensions... File chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.cc (right): https://codereview.chromium.org/11569007/diff/58001/chrome/browser/extensions... chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.cc:61: URLMatcherConditionSet::ID url_match_trigger, On 2013/01/15 13:34:10, battre wrote: > I would suggest to remove this line. Done. https://codereview.chromium.org/11569007/diff/58001/chrome/browser/extensions... chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.cc:71: (url_match_trigger != url_matcher_conditions_->id())) On 2013/01/15 13:34:10, battre wrote: > and change this to > if (url_matcher_conditions_.get() && > !ContainsKey(url_matches, url_matcher_conditions_->id) > > Then the comment in the function header can be simplified. > > The reason for this is that the WebRequestCondition will only have one match > trigger, right? Done. (After an off-line chat with Dominic, we decided that the logarithmic look-up is not too much worse than the constant check, and worth having a cleaner interface.) https://codereview.chromium.org/11569007/diff/58001/chrome/browser/extensions... chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.cc:162: if (url_matches.find(url_match_trigger) == url_matches.end()) { On 2013/01/15 13:34:10, battre wrote: > change this to "if (url_match_trigger == -1)"? > > else: nit: simplify to ContainsKey? > As described here: https://codereview.chromium.org/11569007/diff/46001/chrome/browser/extensions..., the ContainsKey might be back once we add another url attribute, but I'm fine with using -1 for now. https://codereview.chromium.org/11569007/diff/58001/chrome/browser/extensions... File chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.h (right): https://codereview.chromium.org/11569007/diff/58001/chrome/browser/extensions... chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.h:124: // satisfied. On 2013/01/15 13:34:10, battre wrote: > I would change the Else... sentence to: > "If |url_match_trigger| is -1, this function returns whether any of the > conditions without URL attributes is satisfied." and change the implementation > accordingly. Done. https://codereview.chromium.org/11569007/diff/58001/chrome/browser/extensions... File chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry_unittest.cc (right): https://codereview.chromium.org/11569007/diff/58001/chrome/browser/extensions... chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry_unittest.cc:242: attributes.push_back("\"url\": { \"pathContains\": \"\" }, \n"); On 2013/01/15 13:34:10, battre wrote: > nit: please add a new line after this line (also below) Done.
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vabr@chromium.org/11569007/52002
Message was sent while issue was closed.
Change committed as 176923 |
