Create DeclarativeConditionSet, DeclarativeActionSet, and DeclarativeRule templates

Create DeclarativeConditionSet, DeclarativeActionSet, and DeclarativeRule templates
so that code can be shared between declarativeWebRequest and future declarative APIs.

TBR=ben

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

[Jeffrey Yasskin, 2012-12-15 03:58:22]

Here's part of https://codereview.chromium.org/11547033/ split out to be more
reviewable.

[battre, 2012-12-17 11:46:06]

https://codereview.chromium.org/11572061/diff/1/chrome/browser/extensions/api...
File chrome/browser/extensions/api/declarative/declarative_rule.h (right):

https://codereview.chromium.org/11572061/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/declarative/declarative_rule.h:30:
template<typename ConditionT>
The problem I see with template parameters is that it is always pretty difficult
to see which requirements they need to fulfill (here: have a typedef for
MatchData, have a static Create() method that takes a certain set of parameters,
IsFulfilled() method, etc...). Should we document these requirements here?

I think that we should also document that conditions and actions are supposed to
be strictly immutable objects.

https://codereview.chromium.org/11572061/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/declarative/declarative_rule.h:60:
URLMatcherConditionSet::ID url_match,
This one will break with vabr's change:
He is implementing a CL to allow the decl. WebRequest API to filter on
request-urls abut also main frame-urls. PTAL here
https://codereview.chromium.org/11414230/. Can we unify this?

That CL is not finished yet. Here is how I envision it:

We'll have two URL matchers, one for the request URL and the other one for the
main frame url (actually that's the firstPartyForCookies URL).

We would introduce two MatchTrigger instances for rules with the respective URL
matchers and a list of Rules that have no URLFilters.

The biggest problem I see is that the decl. Content API has different
interpretations of URLMatchers:
- for the decl. WebRequest API it is request URL and firstPartyForCookies URL
- for the decl. Content API that you are currently writing it will be main frame
URL
- for the decl. Content API extensions (for blocking resource loads directly in
the renderers) that I will write it will be request URL and main frame URL

https://codereview.chromium.org/11572061/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/declarative/declarative_rule.h:100: const typename
ActionT::ApplyInfo& apply_info) const;
const apply_info?

https://codereview.chromium.org/11572061/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/declarative/declarative_rule.h:107: const typename
ActionT::ApplyInfo& apply_info) const;
const apply_info?

https://codereview.chromium.org/11572061/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/declarative/declarative_rule.h:169: // may be
empty in this case.
nit: there is no returned list.

https://codereview.chromium.org/11572061/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/declarative/declarative_rule.h:170: void
Apply(const typename ActionT::ApplyInfo& apply_info) const;
const?

https://codereview.chromium.org/11572061/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/declarative/declarative_rule.h:213: return
trigger->second->IsFulfilled(match_data);
should we do
return const_cast<const ConditionT*>(trigger->second)->IsFulfilled(match_data)
to enforce that conditions are immutable?

https://codereview.chromium.org/11572061/diff/1/chrome/browser/extensions/api...
File chrome/browser/extensions/api/declarative/declarative_rule_unittest.cc
(right):

https://codereview.chromium.org/11572061/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/declarative/declarative_rule_unittest.cc:103: bool
IsFulfilled(int match_data) {
nit: const?

https://codereview.chromium.org/11572061/diff/1/chrome/browser/extensions/api...
File chrome/browser/extensions/api/declarative_webrequest/webrequest_action.h
(right):

https://codereview.chromium.org/11572061/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/declarative_webrequest/webrequest_action.h:81:
struct ApplyInfo {
add comment: // Information about request and its context:?

https://codereview.chromium.org/11572061/diff/1/chrome/browser/extensions/api...
File chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.h
(right):

https://codereview.chromium.org/11572061/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.h:20:
struct DeclarativeWebRequestData {
Nit: add comment about the purpose of this data structure?

https://codereview.chromium.org/11572061/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.h:26:
const net::HttpResponseHeaders* original_response_headers)
do we need two different constructors for this? Or should we have one and state
that |original_response_headers| may be NULL if there are no HttpResponseHeaders
available at stage |stage|?

https://codereview.chromium.org/11572061/diff/1/chrome/browser/extensions/ext...
File chrome/browser/extensions/extension_browsertest.cc (right):

https://codereview.chromium.org/11572061/diff/1/chrome/browser/extensions/ext...
chrome/browser/extensions/extension_browsertest.cc:594: ).ptr());
does this belong here?

[Jeffrey Yasskin, 2012-12-18 02:44:26]

Thanks for the review. I haven't yet updated the code in response to your
comments; I wanted to get answers to a couple things, and my comments on
https://codereview.chromium.org/11569007/, first. Anything I haven't replied to
is a "will do".

https://codereview.chromium.org/11572061/diff/1/chrome/browser/extensions/api...
File chrome/browser/extensions/api/declarative/declarative_rule.h (right):

https://codereview.chromium.org/11572061/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/declarative/declarative_rule.h:60:
URLMatcherConditionSet::ID url_match,
On 2012/12/17 11:46:06, battre wrote:
> This one will break with vabr's change:
> He is implementing a CL to allow the decl. WebRequest API to filter on
> request-urls abut also main frame-urls. PTAL here
> https://codereview.chromium.org/11414230/. Can we unify this?
> 
> That CL is not finished yet. Here is how I envision it:
> 
> We'll have two URL matchers, one for the request URL and the other one for the
> main frame url (actually that's the firstPartyForCookies URL).
> 
> We would introduce two MatchTrigger instances for rules with the respective
URL
> matchers and a list of Rules that have no URLFilters.
> 
> The biggest problem I see is that the decl. Content API has different
> interpretations of URLMatchers:
> - for the decl. WebRequest API it is request URL and firstPartyForCookies URL
> - for the decl. Content API that you are currently writing it will be main
frame
> URL
> - for the decl. Content API extensions (for blocking resource loads directly
in
> the renderers) that I will write it will be request URL and main frame URL
> 

I've commented on https://codereview.chromium.org/11569007/. I suspect what we
want to do pass the matching ID sets as part of the MatchData struct. Then each
kind of rule can apply their own interpretation.

https://codereview.chromium.org/11572061/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/declarative/declarative_rule.h:100: const typename
ActionT::ApplyInfo& apply_info) const;
On 2012/12/17 11:46:06, battre wrote:
> const apply_info?

It's already a const reference. What do you mean?

https://codereview.chromium.org/11572061/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/declarative/declarative_rule.h:213: return
trigger->second->IsFulfilled(match_data);
On 2012/12/17 11:46:06, battre wrote:
> should we do
> return const_cast<const ConditionT*>(trigger->second)->IsFulfilled(match_data)
> to enforce that conditions are immutable?

We could make match_triggers_ into a std::map<URLMatcherConditionSet::ID, const
ConditionT*>. I'm not sure why it's non-const in WebRequestConditionSet.

https://codereview.chromium.org/11572061/diff/1/chrome/browser/extensions/api...
File chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.h
(right):

https://codereview.chromium.org/11572061/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.h:20:
struct DeclarativeWebRequestData {
On 2012/12/17 11:46:06, battre wrote:
> Nit: add comment about the purpose of this data structure?

This is just moved and renamed from
http://cs/#chrome/src/chrome/browser/extensions/api/declarative_webrequest/we...,
where there wasn't a comment. What comment would you like me to add here?

https://codereview.chromium.org/11572061/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.h:26:
const net::HttpResponseHeaders* original_response_headers)
On 2012/12/17 11:46:06, battre wrote:
> do we need two different constructors for this? Or should we have one and
state
> that |original_response_headers| may be NULL if there are no
HttpResponseHeaders
> available at stage |stage|?

I'd rather not change this interface in this CL, since it's just moved from
elsewhere. We can fix it up after the templatizing refactoring.

[battre, 2012-12-18 08:38:49]

https://codereview.chromium.org/11572061/diff/1/chrome/browser/extensions/api...
File chrome/browser/extensions/api/declarative/declarative_rule.h (right):

https://codereview.chromium.org/11572061/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/declarative/declarative_rule.h:100: const typename
ActionT::ApplyInfo& apply_info) const;
On 2012/12/18 02:44:26, Jeffrey Yasskin wrote:
> On 2012/12/17 11:46:06, battre wrote:
> > const apply_info?
> 
> It's already a const reference. What do you mean?

That's my point. Why is it const. This is the variable that we want to modify,
it is an output parameter.

I'd suggest to pass a "typename ActionT::ApplyInfo* apply_info"

https://codereview.chromium.org/11572061/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/declarative/declarative_rule.h:213: return
trigger->second->IsFulfilled(match_data);
On 2012/12/18 02:44:26, Jeffrey Yasskin wrote:
> On 2012/12/17 11:46:06, battre wrote:
> > should we do
> > return const_cast<const
ConditionT*>(trigger->second)->IsFulfilled(match_data)
> > to enforce that conditions are immutable?
> 
> We could make match_triggers_ into a std::map<URLMatcherConditionSet::ID,
const
> ConditionT*>. I'm not sure why it's non-const in WebRequestConditionSet.

SGTM

https://codereview.chromium.org/11572061/diff/1/chrome/browser/extensions/api...
File chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.h
(right):

https://codereview.chromium.org/11572061/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.h:20:
struct DeclarativeWebRequestData {
On 2012/12/18 02:44:26, Jeffrey Yasskin wrote:
> On 2012/12/17 11:46:06, battre wrote:
> > Nit: add comment about the purpose of this data structure?
> 
> This is just moved and renamed from
>
http://cs/#chrome/src/chrome/browser/extensions/api/declarative_webrequest/we...,
> where there wasn't a comment. What comment would you like me to add here?

How about
// Container for information about a URLRequest to determine which rules apply
to the request.
?

https://codereview.chromium.org/11572061/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.h:26:
const net::HttpResponseHeaders* original_response_headers)
On 2012/12/18 02:44:26, Jeffrey Yasskin wrote:
> On 2012/12/17 11:46:06, battre wrote:
> > do we need two different constructors for this? Or should we have one and
> state
> > that |original_response_headers| may be NULL if there are no
> HttpResponseHeaders
> > available at stage |stage|?
> 
> I'd rather not change this interface in this CL, since it's just moved from
> elsewhere. We can fix it up after the templatizing refactoring.

SGTM

[Jeffrey Yasskin, 2012-12-19 08:23:06]

This should go in after Vaclav's CL 11569007.

https://codereview.chromium.org/11572061/diff/1/chrome/browser/extensions/api...
File chrome/browser/extensions/api/declarative/declarative_rule.h (right):

https://codereview.chromium.org/11572061/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/declarative/declarative_rule.h:30:
template<typename ConditionT>
On 2012/12/17 11:46:06, battre wrote:
> The problem I see with template parameters is that it is always pretty
difficult
> to see which requirements they need to fulfill (here: have a typedef for
> MatchData, have a static Create() method that takes a certain set of
parameters,
> IsFulfilled() method, etc...). Should we document these requirements here?
> 
> I think that we should also document that conditions and actions are supposed
to
> be strictly immutable objects.

Done.

https://codereview.chromium.org/11572061/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/declarative/declarative_rule.h:100: const typename
ActionT::ApplyInfo& apply_info) const;
On 2012/12/18 08:38:49, battre wrote:
> On 2012/12/18 02:44:26, Jeffrey Yasskin wrote:
> > On 2012/12/17 11:46:06, battre wrote:
> > > const apply_info?
> > 
> > It's already a const reference. What do you mean?
> 
> That's my point. Why is it const. This is the variable that we want to modify,
> it is an output parameter.
> 
> I'd suggest to pass a "typename ActionT::ApplyInfo* apply_info"

It was const because it holds a non-const pointer to the out parameter. It's
easy enough to make the input parameters const instead, or just to rely on
Action implementations to do the right thing so they cooperate with their
RulesRegistry.

https://codereview.chromium.org/11572061/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/declarative/declarative_rule.h:107: const typename
ActionT::ApplyInfo& apply_info) const;
On 2012/12/17 11:46:06, battre wrote:
> const apply_info?

Done.

https://codereview.chromium.org/11572061/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/declarative/declarative_rule.h:122: class
DeclarativeRule {
What? No complaint that I completely forgot a comment on this class? ;) (Added
one)

https://codereview.chromium.org/11572061/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/declarative/declarative_rule.h:169: // may be
empty in this case.
On 2012/12/17 11:46:06, battre wrote:
> nit: there is no returned list.

Indeed. I've rewritten this comment to avoid discussing WebRequest details.

https://codereview.chromium.org/11572061/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/declarative/declarative_rule.h:170: void
Apply(const typename ActionT::ApplyInfo& apply_info) const;
On 2012/12/17 11:46:06, battre wrote:
> const?

Done.

https://codereview.chromium.org/11572061/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/declarative/declarative_rule.h:213: return
trigger->second->IsFulfilled(match_data);
On 2012/12/18 08:38:49, battre wrote:
> On 2012/12/18 02:44:26, Jeffrey Yasskin wrote:
> > On 2012/12/17 11:46:06, battre wrote:
> > > should we do
> > > return const_cast<const
> ConditionT*>(trigger->second)->IsFulfilled(match_data)
> > > to enforce that conditions are immutable?
> > 
> > We could make match_triggers_ into a std::map<URLMatcherConditionSet::ID,
> const
> > ConditionT*>. I'm not sure why it's non-const in WebRequestConditionSet.
> 
> SGTM

match_triggers_ is gone, but I made all the lists hold const conditions and
actions.

https://codereview.chromium.org/11572061/diff/1/chrome/browser/extensions/api...
File chrome/browser/extensions/api/declarative/declarative_rule_unittest.cc
(right):

https://codereview.chromium.org/11572061/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/declarative/declarative_rule_unittest.cc:103: bool
IsFulfilled(int match_data) {
On 2012/12/17 11:46:06, battre wrote:
> nit: const?

Done.

https://codereview.chromium.org/11572061/diff/1/chrome/browser/extensions/api...
File chrome/browser/extensions/api/declarative_webrequest/webrequest_action.h
(right):

https://codereview.chromium.org/11572061/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/declarative_webrequest/webrequest_action.h:81:
struct ApplyInfo {
On 2012/12/17 11:46:06, battre wrote:
> add comment: // Information about request and its context:?

Done.

https://codereview.chromium.org/11572061/diff/1/chrome/browser/extensions/api...
File chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.h
(right):

https://codereview.chromium.org/11572061/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.h:20:
struct DeclarativeWebRequestData {
On 2012/12/18 08:38:49, battre wrote:
> On 2012/12/18 02:44:26, Jeffrey Yasskin wrote:
> > On 2012/12/17 11:46:06, battre wrote:
> > > Nit: add comment about the purpose of this data structure?
> > 
> > This is just moved and renamed from
> >
>
http://cs/#chrome/src/chrome/browser/extensions/api/declarative_webrequest/we...,
> > where there wasn't a comment. What comment would you like me to add here?
> 
> How about
> // Container for information about a URLRequest to determine which rules apply
> to the request.
> ?

Done.

https://codereview.chromium.org/11572061/diff/1/chrome/browser/extensions/ext...
File chrome/browser/extensions/extension_browsertest.cc (right):

https://codereview.chromium.org/11572061/diff/1/chrome/browser/extensions/ext...
chrome/browser/extensions/extension_browsertest.cc:594: ).ptr());
On 2012/12/17 11:46:06, battre wrote:
> does this belong here?

No. Thanks, reverted.

https://codereview.chromium.org/11572061/diff/11001/chrome/browser/extensions...
File
chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_unittest.cc
(right):

https://codereview.chromium.org/11572061/diff/11001/chrome/browser/extensions...
chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_unittest.cc:194:
request_data.url_matches = &empty_url_matches;
These assignments into request_data are somewhat awful. I'd like to try to clean
them up after Vaclav's second CL is in so we know what the extra data looks like
and can design good constructors.

[vabr (Chromium), 2012-12-19 15:33:42]

Hi Jeffrey,
Sorry for a late response, I got a bit lost in the CLs.

My comments below are mostly nits, and I did not review unittests yet. I will
try to take a look at them soon, but feel free not to wait for me. LGTM so far.

I have one general question:
In the abstraction of the declarative condition sets, we have a couple of
specific methods regarding URLMatcher (in particular,
GetURLMatcherConditionSet). Are we confident that URL attributes will be part of
all instances of declarative conditions? Or could it happen that a different
kind of triggers will be used?

I guess URL attributes are currently the only ones which can be processed
quickly in large batches. So they are also the only ones suitable to deploy as
triggers. If there is another instance of declarative condition than the
WebRequest conditions, which does not use URL attributes, it will probably not
use any other triggers. So it can just ignore the URL related parts of the
template.

I'm OK with having GetURLMatcherConditionSet et al. in the template, just wanted
to know whether you have considered this and what are your thoughts.

Cheers,
Vaclav

https://codereview.chromium.org/11572061/diff/11001/chrome/browser/extensions...
File chrome/browser/extensions/api/declarative/declarative_rule.h (right):

https://codereview.chromium.org/11572061/diff/11001/chrome/browser/extensions...
chrome/browser/extensions/api/declarative/declarative_rule.h:115: //   // Return
the minimum priority of rules that can be evaluated after this
nit: are rules evaluated or applied? I suggest to evaluate conditions, apply
rules and run (or take) actions.

https://codereview.chromium.org/11572061/diff/11001/chrome/browser/extensions...
chrome/browser/extensions/api/declarative/declarative_rule.h:150: // this rule.
Defaults to MIN_INT.
nit: again evaluate vs. apply (see above).
Also, "this rule" does not strictly make sense, we are in an action set. I
suggest "the actions from this set ran".

https://codereview.chromium.org/11572061/diff/11001/chrome/browser/extensions...
chrome/browser/extensions/api/declarative/declarative_rule.h:219: // Returns the
minimum priority of rules that may be evaluated after
nit: evaluated or applied? (See comments above.)

https://codereview.chromium.org/11572061/diff/11001/chrome/browser/extensions...
chrome/browser/extensions/api/declarative/declarative_rule.h:220: // this rule.
Defaults to MAX_INT. Only valid if the conditions of this rule
How does this default to MAX_INT? In the implementation it just calls the action
set, which defaults to MIN_INT.

https://codereview.chromium.org/11572061/diff/11001/chrome/browser/extensions...
chrome/browser/extensions/api/declarative/declarative_rule.h:240:
template<typename ConditionT>
Please prepend // static for consistency with the rest of the file.

[Jeffrey Yasskin, 2012-12-20 01:27:35]

I think if we index the conditions by some other kind of data, we'll have to
communicate that to the ConditionSet, but we should burn that bridge when we
come to it.

https://codereview.chromium.org/11572061/diff/11001/chrome/browser/extensions...
File chrome/browser/extensions/api/declarative/declarative_rule.h (right):

https://codereview.chromium.org/11572061/diff/11001/chrome/browser/extensions...
chrome/browser/extensions/api/declarative/declarative_rule.h:115: //   // Return
the minimum priority of rules that can be evaluated after this
On 2012/12/19 15:33:42, vabr (Chromium) wrote:
> nit: are rules evaluated or applied? I suggest to evaluate conditions, apply
> rules and run (or take) actions.

I'm somewhat reluctant to go through and rename Action::Apply everywhere in this
CL. I'm not sure it's terrible to use the same verbs for some of conditions,
rules, and actions ... to "evaluate" a rule seems to mean to test/evaluate its
conditions and then apply its actions if a condition matched, while "applying" a
rule would mean to apply its actions. I'd agree that we shouldn't "apply" or
"run" a condition or "evaluate" or "test" an action.

I don't feel that strongly though, and will rename stuff if you think it's
important.

https://codereview.chromium.org/11572061/diff/11001/chrome/browser/extensions...
chrome/browser/extensions/api/declarative/declarative_rule.h:220: // this rule.
Defaults to MAX_INT. Only valid if the conditions of this rule
On 2012/12/19 15:33:42, vabr (Chromium) wrote:
> How does this default to MAX_INT? In the implementation it just calls the
action
> set, which defaults to MIN_INT.

I think that's a typo I copied from webrequest_rule.h. Fixed.

https://codereview.chromium.org/11572061/diff/11001/chrome/browser/extensions...
chrome/browser/extensions/api/declarative/declarative_rule.h:240:
template<typename ConditionT>
On 2012/12/19 15:33:42, vabr (Chromium) wrote:
> Please prepend // static for consistency with the rest of the file.

Oops, but this function's gone now.

https://codereview.chromium.org/11572061/diff/11001/chrome/browser/extensions...
File
chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_unittest.cc
(right):

https://codereview.chromium.org/11572061/diff/11001/chrome/browser/extensions...
chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_unittest.cc:194:
request_data.url_matches = &empty_url_matches;
On 2012/12/19 08:23:06, Jeffrey Yasskin wrote:
> These assignments into request_data are somewhat awful. I'd like to try to
clean
> them up after Vaclav's second CL is in so we know what the extra data looks
like
> and can design good constructors.

And I've reverted them since ConditionSet needed to be aware of url_matches
anyway. Vaclav will need to put them back for his first-party-url change, but
I'll let him figure out the readable way to do that. :)

[vabr (Chromium), 2012-12-20 10:51:29]

LGTM

I also went through the unit-tests, and found no tests lost during the moving of
code to templates.

Vaclav

https://codereview.chromium.org/11572061/diff/11001/chrome/browser/extensions...
File chrome/browser/extensions/api/declarative/declarative_rule.h (right):

https://codereview.chromium.org/11572061/diff/11001/chrome/browser/extensions...
chrome/browser/extensions/api/declarative/declarative_rule.h:115: //   // Return
the minimum priority of rules that can be evaluated after this
On 2012/12/20 01:27:35, Jeffrey Yasskin wrote:
> On 2012/12/19 15:33:42, vabr (Chromium) wrote:
> > nit: are rules evaluated or applied? I suggest to evaluate conditions, apply
> > rules and run (or take) actions.
> 
> I'm somewhat reluctant to go through and rename Action::Apply everywhere in
this
> CL. I'm not sure it's terrible to use the same verbs for some of conditions,
> rules, and actions ... to "evaluate" a rule seems to mean to test/evaluate its
> conditions and then apply its actions if a condition matched, while "applying"
a
> rule would mean to apply its actions. I'd agree that we shouldn't "apply" or
> "run" a condition or "evaluate" or "test" an action.
> 
> I don't feel that strongly though, and will rename stuff if you think it's
> important.

It's fine to leave as it is. Definitively no need to rename function names. In
this comment I was a bit confused, because priority is mainly about
(dis)allowing rule actions, not conditions. But obviously if we won't be running
the action, then we don't try to evaluate the condition as well, so it makes
roughly sense.

[Jeffrey Yasskin, 2013-01-16 00:58:01]

This is updated to match Vaclav's checkin.

[battre, 2013-01-16 15:41:10]

LGTM - only nits. In case you want to add new unit tests for my comment on
declarative_rule.h:270, feel free to ask anybody in your timezone to review.

https://codereview.chromium.org/11572061/diff/26002/chrome/browser/extensions...
File chrome/browser/extensions/api/declarative/declarative_rule.h (right):

https://codereview.chromium.org/11572061/diff/26002/chrome/browser/extensions...
chrome/browser/extensions/api/declarative/declarative_rule.h:50: //   bool
condition->IsFulfilled(
I think the "condition->" should be deleted.

https://codereview.chromium.org/11572061/diff/26002/chrome/browser/extensions...
chrome/browser/extensions/api/declarative/declarative_rule.h:55: public:
nit: too many spaces

https://codereview.chromium.org/11572061/diff/26002/chrome/browser/extensions...
chrome/browser/extensions/api/declarative/declarative_rule.h:142: //   int
GetMinimumPriority const;
nit: missing ()

https://codereview.chromium.org/11572061/diff/26002/chrome/browser/extensions...
chrome/browser/extensions/api/declarative/declarative_rule.h:181: Actions
actions_;
nit, while we are at it: Can you make this const please?

https://codereview.chromium.org/11572061/diff/26002/chrome/browser/extensions...
chrome/browser/extensions/api/declarative/declarative_rule.h:207: ActionSet*
actions,
nit: make ConditionSet and ActionSet const?

https://codereview.chromium.org/11572061/diff/26002/chrome/browser/extensions...
chrome/browser/extensions/api/declarative/declarative_rule.h:242: void
Apply(typename ActionT::ApplyInfo* apply_info) const;
no Revert? - you can delay this into a later CL.

https://codereview.chromium.org/11572061/diff/26002/chrome/browser/extensions...
chrome/browser/extensions/api/declarative/declarative_rule.h:270: if
(url_match_trigger == -1) {
This path is never exercised. Vaclav has written new unit tests for this. Do you
want to rely on the Decl. WebRequest API unit tests for testing this?

https://codereview.chromium.org/11572061/diff/26002/chrome/browser/extensions...
File chrome/browser/extensions/api/declarative/declarative_rule_unittest.cc
(right):

https://codereview.chromium.org/11572061/diff/26002/chrome/browser/extensions...
chrome/browser/extensions/api/declarative/declarative_rule_unittest.cc:38: const
base::DictionaryValue* dict;
nit: initialize with NULL?

https://codereview.chromium.org/11572061/diff/26002/chrome/browser/extensions...
chrome/browser/extensions/api/declarative/declarative_rule_unittest.cc:54: const
char* RecordingCondition::error_on_key = NULL;
I think it might be nicer to have error_on_key a const value and then
differentiating the test cases by sometimes having conditions with this key and
other conditions without the key. This way the tests don't have any permanent
side effects which might cause flakyness for sharded test execution.

https://codereview.chromium.org/11572061/diff/26002/chrome/browser/extensions...
chrome/browser/extensions/api/declarative/declarative_rule_unittest.cc:118: int
match_data) const {
nit/opt: replace int with MatchData?

https://codereview.chromium.org/11572061/diff/26002/chrome/browser/extensions...
chrome/browser/extensions/api/declarative/declarative_rule_unittest.cc:144:
TEST(DeclarativeConditionTest, FulfilConditionSet) {
sp: Fulfill

https://codereview.chromium.org/11572061/diff/26002/chrome/browser/extensions...
chrome/browser/extensions/api/declarative/declarative_rule_unittest.cc:171:
EXPECT_FALSE(result->IsFulfilled(3, url_matches, 2));
does this test anything that is not tested in line 166?

https://codereview.chromium.org/11572061/diff/26002/chrome/browser/extensions...
chrome/browser/extensions/api/declarative/declarative_rule_unittest.cc:212: int*
sum) const {
nit: replace int with ApplyInfo?

https://codereview.chromium.org/11572061/diff/26002/chrome/browser/extensions...
chrome/browser/extensions/api/declarative/declarative_rule_unittest.cc:304:
const Rule::ConditionSet::Conditions conditions =
nit: Conditions&

https://codereview.chromium.org/11572061/diff/26002/chrome/browser/extensions...
chrome/browser/extensions/api/declarative/declarative_rule_unittest.cc:324: if
(conditions->conditions().size() < 1) {
nit: replace .size() < 1 with .empty()?

https://codereview.chromium.org/11572061/diff/26002/chrome/browser/extensions...
chrome/browser/extensions/api/declarative/declarative_rule_unittest.cc:328: if
(actions->actions().size() < 1) {
dito

https://codereview.chromium.org/11572061/diff/26002/chrome/browser/extensions...
chrome/browser/extensions/api/declarative/declarative_rule_unittest.cc:329:
*error = "No actions";
this path is never exercised -> remove

https://codereview.chromium.org/11572061/diff/26002/chrome/browser/extensions...
File chrome/browser/extensions/api/declarative_webrequest/webrequest_action.cc
(right):

https://codereview.chromium.org/11572061/diff/26002/chrome/browser/extensions...
chrome/browser/extensions/api/declarative_webrequest/webrequest_action.cc:454:
extension_install_time);
nit: move to previous line?

[Jeffrey Yasskin, 2013-01-16 23:11:13]

+kalman to glance over the changes after Dominic's LGTM.
+ben for chrome/*.gypi, but I'll TBR if I don't hear back before kalman lgtms.

https://codereview.chromium.org/11572061/diff/26002/chrome/browser/extensions...
File chrome/browser/extensions/api/declarative/declarative_rule.h (right):

https://codereview.chromium.org/11572061/diff/26002/chrome/browser/extensions...
chrome/browser/extensions/api/declarative/declarative_rule.h:50: //   bool
condition->IsFulfilled(
On 2013/01/16 15:41:10, battre wrote:
> I think the "condition->" should be deleted.

Done.

https://codereview.chromium.org/11572061/diff/26002/chrome/browser/extensions...
chrome/browser/extensions/api/declarative/declarative_rule.h:55: public:
On 2013/01/16 15:41:10, battre wrote:
> nit: too many spaces

Oops.

https://codereview.chromium.org/11572061/diff/26002/chrome/browser/extensions...
chrome/browser/extensions/api/declarative/declarative_rule.h:142: //   int
GetMinimumPriority const;
On 2013/01/16 15:41:10, battre wrote:
> nit: missing ()

Done.

https://codereview.chromium.org/11572061/diff/26002/chrome/browser/extensions...
chrome/browser/extensions/api/declarative/declarative_rule.h:181: Actions
actions_;
On 2013/01/16 15:41:10, battre wrote:
> nit, while we are at it: Can you make this const please?

Done.

https://codereview.chromium.org/11572061/diff/26002/chrome/browser/extensions...
chrome/browser/extensions/api/declarative/declarative_rule.h:207: ActionSet*
actions,
On 2013/01/16 15:41:10, battre wrote:
> nit: make ConditionSet and ActionSet const?

Done.

https://codereview.chromium.org/11572061/diff/26002/chrome/browser/extensions...
chrome/browser/extensions/api/declarative/declarative_rule.h:242: void
Apply(typename ActionT::ApplyInfo* apply_info) const;
On 2013/01/16 15:41:10, battre wrote:
> no Revert? - you can delay this into a later CL.

Right. It'll get added as part of the declarativeContent CL.

https://codereview.chromium.org/11572061/diff/26002/chrome/browser/extensions...
chrome/browser/extensions/api/declarative/declarative_rule.h:270: if
(url_match_trigger == -1) {
On 2013/01/16 15:41:10, battre wrote:
> This path is never exercised. Vaclav has written new unit tests for this. Do
you
> want to rely on the Decl. WebRequest API unit tests for testing this?

Good point. I'll add a test for this.  Ideally, we should be able to remove some
webrequest tests after this test is submitted, but I'll talk to Vaclav about
which ones.

https://codereview.chromium.org/11572061/diff/26002/chrome/browser/extensions...
File chrome/browser/extensions/api/declarative/declarative_rule_unittest.cc
(right):

https://codereview.chromium.org/11572061/diff/26002/chrome/browser/extensions...
chrome/browser/extensions/api/declarative/declarative_rule_unittest.cc:38: const
base::DictionaryValue* dict;
On 2013/01/16 15:41:10, battre wrote:
> nit: initialize with NULL?

Sure, that might fix a problem with some gcc builds.

https://codereview.chromium.org/11572061/diff/26002/chrome/browser/extensions...
chrome/browser/extensions/api/declarative/declarative_rule_unittest.cc:54: const
char* RecordingCondition::error_on_key = NULL;
On 2013/01/16 15:41:10, battre wrote:
> I think it might be nicer to have error_on_key a const value and then
> differentiating the test cases by sometimes having conditions with this key
and
> other conditions without the key. This way the tests don't have any permanent
> side effects which might cause flakyness for sharded test execution.

Ok.

https://codereview.chromium.org/11572061/diff/26002/chrome/browser/extensions...
chrome/browser/extensions/api/declarative/declarative_rule_unittest.cc:118: int
match_data) const {
On 2013/01/16 15:41:10, battre wrote:
> nit/opt: replace int with MatchData?

MatchData mostly exists to communicate with the templates. In non-templated
code, avoiding the typedef tends to be more readable.

https://codereview.chromium.org/11572061/diff/26002/chrome/browser/extensions...
chrome/browser/extensions/api/declarative/declarative_rule_unittest.cc:144:
TEST(DeclarativeConditionTest, FulfilConditionSet) {
On 2013/01/16 15:41:10, battre wrote:
> sp: Fulfill

But http://en.wiktionary.org/wiki/fulfil. I guess we're better off using the US
spelling.

https://codereview.chromium.org/11572061/diff/26002/chrome/browser/extensions...
chrome/browser/extensions/api/declarative/declarative_rule_unittest.cc:171:
EXPECT_FALSE(result->IsFulfilled(3, url_matches, 2));
On 2013/01/16 15:41:10, battre wrote:
> does this test anything that is not tested in line 166?

I guess not.

https://codereview.chromium.org/11572061/diff/26002/chrome/browser/extensions...
chrome/browser/extensions/api/declarative/declarative_rule_unittest.cc:212: int*
sum) const {
On 2013/01/16 15:41:10, battre wrote:
> nit: replace int with ApplyInfo?

ApplyInfo mostly exists to communicate with the templates. In non-templated
code, avoiding the typedef tends to be more readable.

https://codereview.chromium.org/11572061/diff/26002/chrome/browser/extensions...
chrome/browser/extensions/api/declarative/declarative_rule_unittest.cc:304:
const Rule::ConditionSet::Conditions conditions =
On 2013/01/16 15:41:10, battre wrote:
> nit: Conditions&

Done.

https://codereview.chromium.org/11572061/diff/26002/chrome/browser/extensions...
chrome/browser/extensions/api/declarative/declarative_rule_unittest.cc:324: if
(conditions->conditions().size() < 1) {
On 2013/01/16 15:41:10, battre wrote:
> nit: replace .size() < 1 with .empty()?

Done.

https://codereview.chromium.org/11572061/diff/26002/chrome/browser/extensions...
chrome/browser/extensions/api/declarative/declarative_rule_unittest.cc:329:
*error = "No actions";
On 2013/01/16 15:41:10, battre wrote:
> this path is never exercised -> remove

Done.

https://codereview.chromium.org/11572061/diff/26002/chrome/browser/extensions...
File chrome/browser/extensions/api/declarative_webrequest/webrequest_action.cc
(right):

https://codereview.chromium.org/11572061/diff/26002/chrome/browser/extensions...
chrome/browser/extensions/api/declarative_webrequest/webrequest_action.cc:454:
extension_install_time);
On 2013/01/16 15:41:10, battre wrote:
> nit: move to previous line?

Done.

[not at google - send to devlin, 2013-01-16 23:15:20]

lgtm, 1 silly nit.

https://codereview.chromium.org/11572061/diff/33001/chrome/browser/extensions...
File chrome/browser/extensions/api/declarative/declarative_rule_unittest.cc
(right):

https://codereview.chromium.org/11572061/diff/33001/chrome/browser/extensions...
chrome/browser/extensions/api/declarative/declarative_rule_unittest.cc:40:
dict->HasKey("bad_key")) {
can fit on 1 line?

[commit-bot: I haz the power, 2013-01-16 23:42:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jyasskin@chromium.org/11572061/28005

[commit-bot: I haz the power, 2013-01-16 23:42:50]

Presubmit check for 11572061-28005 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit Messages **
If this change has an associated bug, add BUG=[bug number].

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome

Presubmit checks took 6.3s to calculate.

[commit-bot: I haz the power, 2013-01-16 23:47:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jyasskin@chromium.org/11572061/28005

[commit-bot: I haz the power, 2013-01-17 02:24:04]

Change committed as 177313