Implementation of beginning of Declarative Web Request API backend

Implementation of beginning of Declarative Web Request API backend

This CL implements the beginning of the RulesRegistry for the Declarative Web Request API; it allows registering rules and checking whether their conditions are fulfilled.

BUG=112155
TEST=no

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

[battre, 2012-03-21 22:58:46]

Matt: could you please have a first pass on this.
Aaron: FYI

This is the beginning of the RulesRegistry for the Declarative WebRequest API.
It probably needs some more polish (I'd be surprised if clang did not complain),
but I think it is ready for a first pass.

The CL is pretty huge, but I don't think it makes sense to cut it into smaller
pieces. I suggest to start at webrequest_rules_registry.h, it contains a
highlevel description of what is going on.

webrequest_action.* shows how the JSON representation of rules is mapped to
concrete Actions for the declarative Web Request API:
- WebRequestAction is an abstract base class that is implemented by
WebRequestCancelAction.
- WebRequestActionCollection groups several WebRequestActions
- WebRequestActionFactory is responsible for the instantiation.

You will see the same pattern for WebRequestConditions. The only difference is
that WebRequestConditions are separated into two different parts. The matching
of url patterns is delegated to the URLMatcher. All other attributes (criteria)
of conditions are represented by WebRequestConditionAttributes.

Overall this is pretty complex, so please let me know if I can help (maybe by
adding more comments that are unclear).

Thanks,
Dominic

[Matt Perry, 2012-03-22 01:34:43]

I've only just started reviewing. I've commented on some small issues for a few
of the files I've glanced at.

I'll have to look more deeply tomorrow, but at a higher level, the
WebRequestRule/Action/Condition stuff seems quite complicated to me at first
glance. I wonder if it could be made simpler by making it less of a
class-hierarchy and more data-driven. I'll think more about this as I review
later.

https://chromiumcodereview.appspot.com/9820003/diff/1/chrome/browser/extensio...
File
chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.cc
(right):

https://chromiumcodereview.appspot.com/9820003/diff/1/chrome/browser/extensio...
chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.cc:53:
const char* kAllPathAttributes[] = {
Putting these in a global map would remove the alphabetical constraint.
Something like:
std::map<std::string, void (URLConditionFactory::*)(const std::string&)>

and have a LazyInstance or Singleton that initializes the map on first access.

https://chromiumcodereview.appspot.com/9820003/diff/1/chrome/browser/extensio...
File
chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry.cc
(right):

https://chromiumcodereview.appspot.com/9820003/diff/1/chrome/browser/extensio...
chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry.cc:25:
typedef std::set<URLMatcherConditionSet::ID> UrlMatches;
nit: URLMatches for consistency

https://chromiumcodereview.appspot.com/9820003/diff/1/chrome/browser/extensio...
File
chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry.h
(right):

https://chromiumcodereview.appspot.com/9820003/diff/1/chrome/browser/extensio...
chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry.h:21:
class WebRequestRule;
nit: just move this into the namespace block below

https://chromiumcodereview.appspot.com/9820003/diff/1/chrome/browser/extensio...
chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry.h:26:
namespace declarative_webrequest{
We shouldn't go overboard with namespaces. I think scoping all this stuff in the
extensions namespace should be good enough.

https://chromiumcodereview.appspot.com/9820003/diff/1/chrome/browser/extensio...
chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry.h:45:
// The evaluation of URL related condition attributes (http_suffix, path_prefix)
host_suffix*

https://chromiumcodereview.appspot.com/9820003/diff/1/chrome/browser/extensio...
chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry.h:55:
// example 'schema': 'http') are fulfilled.
I just started reviewing and don't have the full picture in my head, but this
seems weird. "schema" is a property of a URL - there's nothing specific to
WebRequest in there. Why not have URLMatcher/URLRequestCondition handle schemas
as well?

[battre, 2012-03-22 10:47:52]

Hi Matt,

Thanks for this review. I'll probably have little time for coding today.

On 2012/03/22 01:34:43, Matt Perry wrote:
> I've only just started reviewing. I've commented on some small issues for a
few
> of the files I've glanced at.
> 
> I'll have to look more deeply tomorrow, but at a higher level, the
> WebRequestRule/Action/Condition stuff seems quite complicated to me at first
> glance. I wonder if it could be made simpler by making it less of a
> class-hierarchy and more data-driven. I'll think more about this as I review
> later.

I have drawn this figure for you to illustrate the different layers and classes.
I think that overall the class-driven approach should work out pretty nicely,
given that individual Actions and Conditions will get state:
http://www/~battre/DeclarativeWebRequestAPI.png

Best regards,
Dominic

[Matt Perry, 2012-03-22 23:07:24]

After looking at this some more, I understand why you went with the class
hierarchies. I retract my previous comment about making things more data-driven.

https://chromiumcodereview.appspot.com/9820003/diff/1/chrome/browser/extensio...
File chrome/browser/extensions/api/declarative_webrequest/webrequest_action.cc
(right):

https://chromiumcodereview.appspot.com/9820003/diff/1/chrome/browser/extensio...
chrome/browser/extensions/api/declarative_webrequest/webrequest_action.cc:72: if
(instance_type == kInstanceCancel) {
Take a look at ExtensionFunctionRegistry for a way to do this without a long
list of if statements. It probably applies to conditions and condition
attributes as well.

https://chromiumcodereview.appspot.com/9820003/diff/1/chrome/browser/extensio...
File
chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.cc
(right):

https://chromiumcodereview.appspot.com/9820003/diff/1/chrome/browser/extensio...
chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.cc:130:
if (trigger->second->IsFulfilled(*i, request))
Is it necessary to pass in the set ID? If the condition is triggered by ID, then
it must match that ID, right? If that's the case, you can remove the first arg
and add a check:
  CHECK_EQ(*i, trigger->second->url_matcher_condition_set_id());

https://chromiumcodereview.appspot.com/9820003/diff/1/chrome/browser/extensio...
chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.cc:168:
for (base::DictionaryValue::key_iterator key = condition_dict->begin_keys();
Give DictionaryValue::Iterator a try, since you need both the key and the value.

https://chromiumcodereview.appspot.com/9820003/diff/1/chrome/browser/extensio...
chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.cc:176:
if (condition_attribute_name == instance_type) {
When is this possible?

https://chromiumcodereview.appspot.com/9820003/diff/1/chrome/browser/extensio...
File chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.h
(right):

https://chromiumcodereview.appspot.com/9820003/diff/1/chrome/browser/extensio...
chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.h:55:
std::vector<URLMatcherConditionSet>* condition_sets) const;
There's only one condition set - just provide an accessor to return a reference
to it. Callers can decide if they want to add it to a vector.

https://chromiumcodereview.appspot.com/9820003/diff/1/chrome/browser/extensio...
chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.h:67:
class WebRequestConditionCollection {
nit: name WebRequestConditionSet to mirror URLMatcherConditionSet?

ditto for WebRequestActionCollection

https://chromiumcodereview.appspot.com/9820003/diff/1/chrome/browser/extensio...
chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.h:82:
std::vector<URLMatcherConditionSet>* condition_sets) const;
I think this should collect a vector of const-references to the sets. An
URLMatcherConditionSet is a fairly heavyweight thing to be copying.

I'm noticing now that they end up getting copied into the URLMatcher anyway.
That doesn't seem right. Is there any way to have one owner of the
URLMatcherConditionSet? Other classes should just have a reference to it, or
perhaps just hold onto the ID.

https://chromiumcodereview.appspot.com/9820003/diff/1/chrome/browser/extensio...
chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.h:95:
class WebRequestConditionFactory {
since this is just a class with all static methods, I would just move the
methods onto the classes they create. e.g. WebRequestCondition::Create().

Ditto for the other factory classes you have.

https://chromiumcodereview.appspot.com/9820003/diff/1/chrome/browser/extensio...
chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.h:114:
static std::vector<std::string> GetAllPathAttributes();
this and the test are unnecessary if you take my suggestion about the method
map.

https://chromiumcodereview.appspot.com/9820003/diff/1/chrome/browser/extensio...
File
chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute.h
(right):

https://chromiumcodereview.appspot.com/9820003/diff/1/chrome/browser/extensio...
chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute.h:75:
class WebRequestConditionAttributeHasScheme
I'm still convinced this doesn't belong here. Can we move the scheme check into
URLMatcherCondition somehow? I forget why you chose not to put the scheme checks
in there.

If it's not possible, maybe there should be another layer on top of
URLMatcherConditionSet that finishes checking the URL.

https://chromiumcodereview.appspot.com/9820003/diff/1/chrome/browser/extensio...
File chrome/browser/extensions/api/declarative_webrequest/webrequest_rule.cc
(right):

https://chromiumcodereview.appspot.com/9820003/diff/1/chrome/browser/extensio...
chrome/browser/extensions/api/declarative_webrequest/webrequest_rule.cc:42:
WebRequestConditionFactory::CreateConditionCollection(
+2 indent

https://chromiumcodereview.appspot.com/9820003/diff/1/chrome/browser/extensio...
chrome/browser/extensions/api/declarative_webrequest/webrequest_rule.cc:48: //
Parse Actions
nit: superfluous comment

https://chromiumcodereview.appspot.com/9820003/diff/1/chrome/browser/extensio...
File
chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry.cc
(right):

https://chromiumcodereview.appspot.com/9820003/diff/1/chrome/browser/extensio...
chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry.cc:23:
// Then we need to check for each of these, whether the other
comment?

https://chromiumcodereview.appspot.com/9820003/diff/1/chrome/browser/extensio...
chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry.cc:30:
url_match != url_matches.end(); ++url_match) {
+1 indent (to line up with inside of parenthetical). ditto for rest of for-loops

https://chromiumcodereview.appspot.com/9820003/diff/1/chrome/browser/extensio...
chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry.cc:36:
if (rule->conditions_collection().IsFulfilled(url_matches, request)) {
It seems unnecessary to pass in all the url_matches here. Just the current
url_match should be good, because if there is another url_match for this rule,
we will soon iterate to it and retest rule->IsFulfilled with that url_match.

(I say this under the assumption that every URLMatcherConditionSet belongs to
exactly 1 rule, which is true AFAICT.)

https://chromiumcodereview.appspot.com/9820003/diff/1/chrome/browser/extensio...
chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry.cc:95:
rule_triggers_.insert(std::make_pair(j->id(), i->second.get()));
rule_triggers_[j->id()] = i->second.get(); ?

https://chromiumcodereview.appspot.com/9820003/diff/1/chrome/browser/extensio...
File
chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry.h
(right):

https://chromiumcodereview.appspot.com/9820003/diff/1/chrome/browser/extensio...
chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry.h:42:
//   'schema': 'http'
schema -> scheme

https://chromiumcodereview.appspot.com/9820003/diff/1/chrome/browser/extensio...
chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry.h:91:
std::map<WebRequestRule*, GlobalRuleId> rule_ids_;
You could avoid needing this by having RuleTriggers be a
std::map<URLMatcherConditionSet::ID, std::pair<GlobalRuleId, WebRequestRule*> >

https://chromiumcodereview.appspot.com/9820003/diff/1/chrome/browser/extensio...
File
chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry_unittest.cc
(right):

https://chromiumcodereview.appspot.com/9820003/diff/1/chrome/browser/extensio...
chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry_unittest.cc:69:
// Test success.
not sure what this comment means

[Matt Perry, 2012-03-22 23:08:08]

On 2012/03/22 10:47:52, battre wrote:
> Hi Matt,
> 
> Thanks for this review. I'll probably have little time for coding today.
> 
> On 2012/03/22 01:34:43, Matt Perry wrote:
> > I've only just started reviewing. I've commented on some small issues for a
> few
> > of the files I've glanced at.
> > 
> > I'll have to look more deeply tomorrow, but at a higher level, the
> > WebRequestRule/Action/Condition stuff seems quite complicated to me at first
> > glance. I wonder if it could be made simpler by making it less of a
> > class-hierarchy and more data-driven. I'll think more about this as I review
> > later.
> 
> I have drawn this figure for you to illustrate the different layers and
classes.
> I think that overall the class-driven approach should work out pretty nicely,
> given that individual Actions and Conditions will get state:
> http://www/%7Ebattre/DeclarativeWebRequestAPI.png
> 
> Best regards,
> Dominic

Thanks for the diagram - it helped a lot!

[battre, 2012-03-26 18:35:51]

Thanks a lot for the thorough review. Please have another look.

https://chromiumcodereview.appspot.com/9820003/diff/1/chrome/browser/extensio...
File chrome/browser/extensions/api/declarative_webrequest/webrequest_action.cc
(right):

https://chromiumcodereview.appspot.com/9820003/diff/1/chrome/browser/extensio...
chrome/browser/extensions/api/declarative_webrequest/webrequest_action.cc:72: if
(instance_type == kInstanceCancel) {
On 2012/03/22 23:07:25, Matt Perry wrote:
> Take a look at ExtensionFunctionRegistry for a way to do this without a long
> list of if statements. It probably applies to conditions and condition
> attributes as well.

Yes, I had that in mind. I will change this code once we have >1 action. ;-)

https://chromiumcodereview.appspot.com/9820003/diff/1/chrome/browser/extensio...
File
chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.cc
(right):

https://chromiumcodereview.appspot.com/9820003/diff/1/chrome/browser/extensio...
chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.cc:53:
const char* kAllPathAttributes[] = {
On 2012/03/22 01:34:43, Matt Perry wrote:
> Putting these in a global map would remove the alphabetical constraint.
> Something like:
> std::map<std::string, void (URLConditionFactory::*)(const std::string&)>
> 
> and have a LazyInstance or Singleton that initializes the map on first access.

Done.

https://chromiumcodereview.appspot.com/9820003/diff/1/chrome/browser/extensio...
chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.cc:130:
if (trigger->second->IsFulfilled(*i, request))
On 2012/03/22 23:07:25, Matt Perry wrote:
> Is it necessary to pass in the set ID? If the condition is triggered by ID,
then
> it must match that ID, right? If that's the case, you can remove the first arg
> and add a check:
>   CHECK_EQ(*i, trigger->second->url_matcher_condition_set_id());

Done.

https://chromiumcodereview.appspot.com/9820003/diff/1/chrome/browser/extensio...
chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.cc:168:
for (base::DictionaryValue::key_iterator key = condition_dict->begin_keys();
On 2012/03/22 23:07:25, Matt Perry wrote:
> Give DictionaryValue::Iterator a try, since you need both the key and the
value.

Done.

https://chromiumcodereview.appspot.com/9820003/diff/1/chrome/browser/extensio...
chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.cc:176:
if (condition_attribute_name == instance_type) {
On 2012/03/22 23:07:25, Matt Perry wrote:
> When is this possible?

This was an error, I meant to compare to kInstanceType (which I did above
already).

Done.

https://chromiumcodereview.appspot.com/9820003/diff/1/chrome/browser/extensio...
File chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.h
(right):

https://chromiumcodereview.appspot.com/9820003/diff/1/chrome/browser/extensio...
chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.h:55:
std::vector<URLMatcherConditionSet>* condition_sets) const;
On 2012/03/22 23:07:25, Matt Perry wrote:
> There's only one condition set - just provide an accessor to return a
reference
> to it. Callers can decide if they want to add it to a vector.

Done.

https://chromiumcodereview.appspot.com/9820003/diff/1/chrome/browser/extensio...
chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.h:67:
class WebRequestConditionCollection {
On 2012/03/22 23:07:25, Matt Perry wrote:
> nit: name WebRequestConditionSet to mirror URLMatcherConditionSet?
> 
> ditto for WebRequestActionCollection

In the case of URLMatcherConditionSet, you favored if class had actually Set
semantics. I picked "Collection" to have a more neutral name.

I don't want to introduce set semantic here, do you still favor renaming this
class?

https://chromiumcodereview.appspot.com/9820003/diff/1/chrome/browser/extensio...
chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.h:82:
std::vector<URLMatcherConditionSet>* condition_sets) const;
On 2012/03/22 23:07:25, Matt Perry wrote:
> I think this should collect a vector of const-references to the sets. An
> URLMatcherConditionSet is a fairly heavyweight thing to be copying.

The URLMatcherConditionSet consists of an ID (int) and a set of
URLMatchingConditions (each sonsisting of an enum and a pointer). Note that the
pattern string is not copied. I would argue that this is not sooooo heavyweight
to be copied. Besides this, they are copied only once, when they are registered
to the URLMatcher.

> I'm noticing now that they end up getting copied into the URLMatcher anyway.
> That doesn't seem right. Is there any way to have one owner of the
> URLMatcherConditionSet? Other classes should just have a reference to it, or
> perhaps just hold onto the ID.

We can set up a similar structure as we do in the URLMatcherConditionFactory but
currently we don't have a factory for URLMatcherConditionSets. They may be
created by anybody. We change the URLMatcher so that it can serve as an owner of
URLMatcherConditionSet regardless whether they are active or not.

I have added a TODO because this will be a refactoring of the URLMatcher that
justifies a separate CL.

https://chromiumcodereview.appspot.com/9820003/diff/1/chrome/browser/extensio...
chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.h:95:
class WebRequestConditionFactory {
On 2012/03/22 23:07:25, Matt Perry wrote:
> since this is just a class with all static methods, I would just move the
> methods onto the classes they create. e.g. WebRequestCondition::Create().
> 
> Ditto for the other factory classes you have.

Done.

https://chromiumcodereview.appspot.com/9820003/diff/1/chrome/browser/extensio...
chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.h:114:
static std::vector<std::string> GetAllPathAttributes();
On 2012/03/22 23:07:25, Matt Perry wrote:
> this and the test are unnecessary if you take my suggestion about the method
> map.

Done.

https://chromiumcodereview.appspot.com/9820003/diff/1/chrome/browser/extensio...
File
chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute.h
(right):

https://chromiumcodereview.appspot.com/9820003/diff/1/chrome/browser/extensio...
chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute.h:75:
class WebRequestConditionAttributeHasScheme
On 2012/03/22 23:07:25, Matt Perry wrote:
> I'm still convinced this doesn't belong here. Can we move the scheme check
into
> URLMatcherCondition somehow? I forget why you chose not to put the scheme
checks
> in there.
> 
> If it's not possible, maybe there should be another layer on top of
> URLMatcherConditionSet that finishes checking the URL.

The reason for having this here was flexibility, i.e. checking for URLs that
match "http OR https".

I have added a TODO as this can be done nicely in a separate CL.

BTW: What about the port number? This is also a difficult beast because we might
want to test for alternative ports.

https://chromiumcodereview.appspot.com/9820003/diff/1/chrome/browser/extensio...
File chrome/browser/extensions/api/declarative_webrequest/webrequest_rule.cc
(right):

https://chromiumcodereview.appspot.com/9820003/diff/1/chrome/browser/extensio...
chrome/browser/extensions/api/declarative_webrequest/webrequest_rule.cc:42:
WebRequestConditionFactory::CreateConditionCollection(
On 2012/03/22 23:07:25, Matt Perry wrote:
> +2 indent

Done.

https://chromiumcodereview.appspot.com/9820003/diff/1/chrome/browser/extensio...
chrome/browser/extensions/api/declarative_webrequest/webrequest_rule.cc:48: //
Parse Actions
On 2012/03/22 23:07:25, Matt Perry wrote:
> nit: superfluous comment

Done.

https://chromiumcodereview.appspot.com/9820003/diff/1/chrome/browser/extensio...
File
chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry.cc
(right):

https://chromiumcodereview.appspot.com/9820003/diff/1/chrome/browser/extensio...
chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry.cc:23:
// Then we need to check for each of these, whether the other
On 2012/03/22 23:07:25, Matt Perry wrote:
> comment?

Done.

https://chromiumcodereview.appspot.com/9820003/diff/1/chrome/browser/extensio...
chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry.cc:25:
typedef std::set<URLMatcherConditionSet::ID> UrlMatches;
On 2012/03/22 01:34:43, Matt Perry wrote:
> nit: URLMatches for consistency

Done.

https://chromiumcodereview.appspot.com/9820003/diff/1/chrome/browser/extensio...
chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry.cc:30:
url_match != url_matches.end(); ++url_match) {
On 2012/03/22 23:07:25, Matt Perry wrote:
> +1 indent (to line up with inside of parenthetical). ditto for rest of
for-loops

Done.

https://chromiumcodereview.appspot.com/9820003/diff/1/chrome/browser/extensio...
chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry.cc:36:
if (rule->conditions_collection().IsFulfilled(url_matches, request)) {
On 2012/03/22 23:07:25, Matt Perry wrote:
> It seems unnecessary to pass in all the url_matches here. Just the current
> url_match should be good, because if there is another url_match for this rule,
> we will soon iterate to it and retest rule->IsFulfilled with that url_match.
> 
> (I say this under the assumption that every URLMatcherConditionSet belongs to
> exactly 1 rule, which is true AFAICT.)

Good catch.

Done.

https://chromiumcodereview.appspot.com/9820003/diff/1/chrome/browser/extensio...
chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry.cc:95:
rule_triggers_.insert(std::make_pair(j->id(), i->second.get()));
On 2012/03/22 23:07:25, Matt Perry wrote:
> rule_triggers_[j->id()] = i->second.get(); ?

Possible, rule_triggers_[j->id()] first creates a empty element and then
overrides its value. Either way is fine with me, would you like me to change
this?

https://chromiumcodereview.appspot.com/9820003/diff/1/chrome/browser/extensio...
File
chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry.h
(right):

https://chromiumcodereview.appspot.com/9820003/diff/1/chrome/browser/extensio...
chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry.h:21:
class WebRequestRule;
On 2012/03/22 01:34:43, Matt Perry wrote:
> nit: just move this into the namespace block below

Done.

https://chromiumcodereview.appspot.com/9820003/diff/1/chrome/browser/extensio...
chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry.h:26:
namespace declarative_webrequest{
On 2012/03/22 01:34:43, Matt Perry wrote:
> We shouldn't go overboard with namespaces. I think scoping all this stuff in
the
> extensions namespace should be good enough.

Done.

https://chromiumcodereview.appspot.com/9820003/diff/1/chrome/browser/extensio...
chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry.h:42:
//   'schema': 'http'
On 2012/03/22 23:07:25, Matt Perry wrote:
> schema -> scheme

Done.

https://chromiumcodereview.appspot.com/9820003/diff/1/chrome/browser/extensio...
chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry.h:45:
// The evaluation of URL related condition attributes (http_suffix, path_prefix)
On 2012/03/22 01:34:43, Matt Perry wrote:
> host_suffix*

Done.

https://chromiumcodereview.appspot.com/9820003/diff/1/chrome/browser/extensio...
chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry.h:55:
// example 'schema': 'http') are fulfilled.
On 2012/03/22 01:34:43, Matt Perry wrote:
> I just started reviewing and don't have the full picture in my head, but this
> seems weird. "schema" is a property of a URL - there's nothing specific to
> WebRequest in there. Why not have URLMatcher/URLRequestCondition handle
schemas
> as well?

I have changed the example to 'has_cookie': {'name': 'Pref'}.

Regarding the scheme, I would like to keep this as a condition attribute (i.e.
something that is checked after the string matcher informs us for the simple
reason that we can allow constructs like these 'scheme': ['http', 'https']

https://chromiumcodereview.appspot.com/9820003/diff/1/chrome/browser/extensio...
chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry.h:91:
std::map<WebRequestRule*, GlobalRuleId> rule_ids_;
On 2012/03/22 23:07:25, Matt Perry wrote:
> You could avoid needing this by having RuleTriggers be a
> std::map<URLMatcherConditionSet::ID, std::pair<GlobalRuleId, WebRequestRule*>
>

Done.

https://chromiumcodereview.appspot.com/9820003/diff/1/chrome/browser/extensio...
File
chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry_unittest.cc
(right):

https://chromiumcodereview.appspot.com/9820003/diff/1/chrome/browser/extensio...
chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry_unittest.cc:69:
// Test success.
On 2012/03/22 23:07:25, Matt Perry wrote:
> not sure what this comment means

Copy and paste...

Done.

[Matt Perry, 2012-03-26 19:11:06]

more comments coming soon

https://chromiumcodereview.appspot.com/9820003/diff/1/chrome/browser/extensio...
File chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.h
(right):

https://chromiumcodereview.appspot.com/9820003/diff/1/chrome/browser/extensio...
chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.h:67:
class WebRequestConditionCollection {
On 2012/03/26 18:35:51, battre wrote:
> On 2012/03/22 23:07:25, Matt Perry wrote:
> > nit: name WebRequestConditionSet to mirror URLMatcherConditionSet?
> > 
> > ditto for WebRequestActionCollection
> 
> In the case of URLMatcherConditionSet, you favored if class had actually Set
> semantics. I picked "Collection" to have a more neutral name.
> 
> I don't want to introduce set semantic here, do you still favor renaming this
> class?

Do you expect duplicate WebRequestConditions/Actions in the collections? Or more
importantly, would a duplicate have any noticeable effect on the output? If
duplicates don't matter, I'd suggest "Set". You don't have to actually use a
std::set (I agree it adds unnecessary overhead in code and perf).

https://chromiumcodereview.appspot.com/9820003/diff/1/chrome/browser/extensio...
chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.h:82:
std::vector<URLMatcherConditionSet>* condition_sets) const;
On 2012/03/26 18:35:51, battre wrote:
> On 2012/03/22 23:07:25, Matt Perry wrote:
> > I think this should collect a vector of const-references to the sets. An
> > URLMatcherConditionSet is a fairly heavyweight thing to be copying.
> 
> The URLMatcherConditionSet consists of an ID (int) and a set of
> URLMatchingConditions (each sonsisting of an enum and a pointer). Note that
the
> pattern string is not copied. I would argue that this is not sooooo
heavyweight
> to be copied. Besides this, they are copied only once, when they are
registered
> to the URLMatcher.

True, but both copies are still stored in memory forever. It could add up if
there are a large amount of web request rules. I could be prematurely
optimizing; we can measure and revisit later.

> 
> > I'm noticing now that they end up getting copied into the URLMatcher anyway.
> > That doesn't seem right. Is there any way to have one owner of the
> > URLMatcherConditionSet? Other classes should just have a reference to it, or
> > perhaps just hold onto the ID.
> 
> We can set up a similar structure as we do in the URLMatcherConditionFactory
but
> currently we don't have a factory for URLMatcherConditionSets. They may be
> created by anybody. We change the URLMatcher so that it can serve as an owner
of
> URLMatcherConditionSet regardless whether they are active or not.
> 
> I have added a TODO because this will be a refactoring of the URLMatcher that
> justifies a separate CL.

https://chromiumcodereview.appspot.com/9820003/diff/1/chrome/browser/extensio...
File
chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute.h
(right):

https://chromiumcodereview.appspot.com/9820003/diff/1/chrome/browser/extensio...
chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute.h:75:
class WebRequestConditionAttributeHasScheme
On 2012/03/26 18:35:51, battre wrote:
> On 2012/03/22 23:07:25, Matt Perry wrote:
> > I'm still convinced this doesn't belong here. Can we move the scheme check
> into
> > URLMatcherCondition somehow? I forget why you chose not to put the scheme
> checks
> > in there.
> > 
> > If it's not possible, maybe there should be another layer on top of
> > URLMatcherConditionSet that finishes checking the URL.
> 
> The reason for having this here was flexibility, i.e. checking for URLs that
> match "http OR https".
> 
> I have added a TODO as this can be done nicely in a separate CL.
> 
> BTW: What about the port number? This is also a difficult beast because we
might
> want to test for alternative ports.

I see. The main thing bothering me here is that the layering seems wrong. I
would expect there to be a suite of URL* conditions that match against the URL,
and some WebRequest* conditions on top of that that match other metadata in the
request (like headers). The URL stuff doesn't all have to be tied to the pattern
matcher. But I suppose the way you have it is OK for now, since I don't have a
better suggestion.

https://chromiumcodereview.appspot.com/9820003/diff/1/chrome/browser/extensio...
File
chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry.cc
(right):

https://chromiumcodereview.appspot.com/9820003/diff/1/chrome/browser/extensio...
chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry.cc:95:
rule_triggers_.insert(std::make_pair(j->id(), i->second.get()));
On 2012/03/26 18:35:51, battre wrote:
> On 2012/03/22 23:07:25, Matt Perry wrote:
> > rule_triggers_[j->id()] = i->second.get(); ?
> 
> Possible, rule_triggers_[j->id()] first creates a empty element and then
> overrides its value. Either way is fine with me, would you like me to change
> this?

map[key] = value; is just more idiosyncratic. it's a style nit, so up to you.

[Matt Perry, 2012-03-26 20:21:20]

Awesome, looking good. Just a few more small nits.

https://chromiumcodereview.appspot.com/9820003/diff/13001/chrome/browser/exte...
File chrome/browser/extensions/api/declarative_webrequest/webrequest_action.h
(right):

https://chromiumcodereview.appspot.com/9820003/diff/13001/chrome/browser/exte...
chrome/browser/extensions/api/declarative_webrequest/webrequest_action.h:46:
static scoped_ptr<WebRequestAction> Create(const base::Value& json_action,
optional: I generally put factory methods right after constructor/destructor

https://chromiumcodereview.appspot.com/9820003/diff/13001/chrome/browser/exte...
File
chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.cc
(right):

https://chromiumcodereview.appspot.com/9820003/diff/13001/chrome/browser/exte...
chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.cc:96:
base::Unretained(url_matcher_condition_factory)).Run(pattern_value);
You don't need to use Bind to invoke here. The syntax for method pointer
invocation is:
 (url_matcher_condition_factory->*method)(pattern_value);

https://chromiumcodereview.appspot.com/9820003/diff/13001/chrome/browser/exte...
chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.cc:257:
if (trigger == match_triggers_.end())
Should this be an error (DCHECK)? The caller knows that url_match triggered this
collection, which should imply the collection contains a condition that matches.

https://chromiumcodereview.appspot.com/9820003/diff/13001/chrome/browser/exte...
chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.cc:260:
if (trigger->second->IsFulfilled(request))
nit: return trigger->second->IsFulfilled(request)

https://chromiumcodereview.appspot.com/9820003/diff/13001/chrome/browser/exte...
File chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.h
(right):

https://chromiumcodereview.appspot.com/9820003/diff/13001/chrome/browser/exte...
chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.h:114:
std::vector<URLMatcherConditionSet>* condition_sets) const;
I see why you chose this name, but it still confuses me a little (I expect it to
append the given sets to the collection). How about GetURLMatcherConditionSets?
The fact that it appends can be documented.

https://chromiumcodereview.appspot.com/9820003/diff/13001/chrome/browser/exte...
File
chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute.h
(right):

https://chromiumcodereview.appspot.com/9820003/diff/13001/chrome/browser/exte...
chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute.h:46:
virtual Type type() const = 0;
underscore_style applies to inline methods only. Should be GetType()

[battre, 2012-03-26 20:38:10]

Thanks.

https://chromiumcodereview.appspot.com/9820003/diff/1/chrome/browser/extensio...
File chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.h
(right):

https://chromiumcodereview.appspot.com/9820003/diff/1/chrome/browser/extensio...
chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.h:67:
class WebRequestConditionCollection {
On 2012/03/26 19:11:06, Matt Perry wrote:
> On 2012/03/26 18:35:51, battre wrote:
> > On 2012/03/22 23:07:25, Matt Perry wrote:
> > > nit: name WebRequestConditionSet to mirror URLMatcherConditionSet?
> > > 
> > > ditto for WebRequestActionCollection
> > 
> > In the case of URLMatcherConditionSet, you favored if class had actually Set
> > semantics. I picked "Collection" to have a more neutral name.
> > 
> > I don't want to introduce set semantic here, do you still favor renaming
this
> > class?
> 
> Do you expect duplicate WebRequestConditions/Actions in the collections? Or
more
> importantly, would a duplicate have any noticeable effect on the output? If
> duplicates don't matter, I'd suggest "Set". You don't have to actually use a
> std::set (I agree it adds unnecessary overhead in code and perf).

No, there should not be any duplicates. If there are, they would not have any
impact.

Done.

https://chromiumcodereview.appspot.com/9820003/diff/1/chrome/browser/extensio...
File
chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute.h
(right):

https://chromiumcodereview.appspot.com/9820003/diff/1/chrome/browser/extensio...
chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute.h:75:
class WebRequestConditionAttributeHasScheme
On 2012/03/26 19:11:06, Matt Perry wrote:
> On 2012/03/26 18:35:51, battre wrote:
> > On 2012/03/22 23:07:25, Matt Perry wrote:
> > > I'm still convinced this doesn't belong here. Can we move the scheme check
> > into
> > > URLMatcherCondition somehow? I forget why you chose not to put the scheme
> > checks
> > > in there.
> > > 
> > > If it's not possible, maybe there should be another layer on top of
> > > URLMatcherConditionSet that finishes checking the URL.
> > 
> > The reason for having this here was flexibility, i.e. checking for URLs that
> > match "http OR https".
> > 
> > I have added a TODO as this can be done nicely in a separate CL.
> > 
> > BTW: What about the port number? This is also a difficult beast because we
> might
> > want to test for alternative ports.
> 
> I see. The main thing bothering me here is that the layering seems wrong. I
> would expect there to be a suite of URL* conditions that match against the
URL,
> and some WebRequest* conditions on top of that that match other metadata in
the
> request (like headers). The URL stuff doesn't all have to be tied to the
pattern
> matcher. But I suppose the way you have it is OK for now, since I don't have a
> better suggestion.

We can improve this, but I suggest to do it in two separate CLs.

1) Make sure that there is only a single instance of each
URLMatcherConditionSet.

2) Now that we don't need to worry about the size of the URLMatcherConditionSet
so much, extend it to support filters for ports + schemes.

https://chromiumcodereview.appspot.com/9820003/diff/1/chrome/browser/extensio...
File
chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry.cc
(right):

https://chromiumcodereview.appspot.com/9820003/diff/1/chrome/browser/extensio...
chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry.cc:95:
rule_triggers_.insert(std::make_pair(j->id(), i->second.get()));
On 2012/03/26 19:11:06, Matt Perry wrote:
> On 2012/03/26 18:35:51, battre wrote:
> > On 2012/03/22 23:07:25, Matt Perry wrote:
> > > rule_triggers_[j->id()] = i->second.get(); ?
> > 
> > Possible, rule_triggers_[j->id()] first creates a empty element and then
> > overrides its value. Either way is fine with me, would you like me to change
> > this?
> 
> map[key] = value; is just more idiosyncratic. it's a style nit, so up to you.

Done.

https://chromiumcodereview.appspot.com/9820003/diff/13001/chrome/browser/exte...
File chrome/browser/extensions/api/declarative_webrequest/webrequest_action.h
(right):

https://chromiumcodereview.appspot.com/9820003/diff/13001/chrome/browser/exte...
chrome/browser/extensions/api/declarative_webrequest/webrequest_action.h:46:
static scoped_ptr<WebRequestAction> Create(const base::Value& json_action,
On 2012/03/26 20:21:20, Matt Perry wrote:
> optional: I generally put factory methods right after constructor/destructor

Done.

https://chromiumcodereview.appspot.com/9820003/diff/13001/chrome/browser/exte...
File
chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.cc
(right):

https://chromiumcodereview.appspot.com/9820003/diff/13001/chrome/browser/exte...
chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.cc:96:
base::Unretained(url_matcher_condition_factory)).Run(pattern_value);
On 2012/03/26 20:21:20, Matt Perry wrote:
> You don't need to use Bind to invoke here. The syntax for method pointer
> invocation is:
>  (url_matcher_condition_factory->*method)(pattern_value);

Done.

https://chromiumcodereview.appspot.com/9820003/diff/13001/chrome/browser/exte...
chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.cc:257:
if (trigger == match_triggers_.end())
On 2012/03/26 20:21:20, Matt Perry wrote:
> Should this be an error (DCHECK)? The caller knows that url_match triggered
this
> collection, which should imply the collection contains a condition that
matches.

Done.

https://chromiumcodereview.appspot.com/9820003/diff/13001/chrome/browser/exte...
chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.cc:260:
if (trigger->second->IsFulfilled(request))
On 2012/03/26 20:21:20, Matt Perry wrote:
> nit: return trigger->second->IsFulfilled(request)

Done.

https://chromiumcodereview.appspot.com/9820003/diff/13001/chrome/browser/exte...
File chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.h
(right):

https://chromiumcodereview.appspot.com/9820003/diff/13001/chrome/browser/exte...
chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.h:114:
std::vector<URLMatcherConditionSet>* condition_sets) const;
On 2012/03/26 20:21:20, Matt Perry wrote:
> I see why you chose this name, but it still confuses me a little (I expect it
to
> append the given sets to the collection). How about
GetURLMatcherConditionSets?
> The fact that it appends can be documented.

Done.

https://chromiumcodereview.appspot.com/9820003/diff/13001/chrome/browser/exte...
File
chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute.h
(right):

https://chromiumcodereview.appspot.com/9820003/diff/13001/chrome/browser/exte...
chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute.h:46:
virtual Type type() const = 0;
On 2012/03/26 20:21:20, Matt Perry wrote:
> underscore_style applies to inline methods only. Should be GetType()

Done.

[battre, 2012-03-27 08:16:20]

"looking good" == LGTY? Rietveld does not recognize it as the magical four
characters.

Best regards,
Dominic

[Matt Perry, 2012-03-27 18:19:37]

Sorry, LGTM. I wanted to take one last look but somehow missed your latest
patchset.

[commit-bot: I haz the power, 2012-03-27 18:34:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/battre@chromium.org/9820003/7022

[commit-bot: I haz the power, 2012-03-27 19:45:49]

Try job failure for 9820003-7022 (retry) on win for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

[commit-bot: I haz the power, 2012-03-28 13:06:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/battre@chromium.org/9820003/7022

[commit-bot: I haz the power, 2012-03-28 16:07:32]

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

[commit-bot: I haz the power, 2012-03-28 16:21:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/battre@chromium.org/9820003/7022

[commit-bot: I haz the power, 2012-03-28 21:02:10]

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

[commit-bot: I haz the power, 2012-03-28 21:03:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/battre@chromium.org/9820003/7022

[commit-bot: I haz the power, 2012-03-29 00:50:20]

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