Declarative Web Request: firstPartyForCookiesUrl

Declarative Web Request: firstPartyForCookiesUrl

This adds a new URLMatcher attribute to declarative Web Request conditions.

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

[vabr (Chromium), 2012-11-30 20:08:18]

Hi Dominic,

Once you get out of the avalanche in your mailbox... :)
Please take a look at this.

Once you are fine, I'll loop in Matt for the JSON rubber stamp, and Jochen for
my use of the "first party for cookies" stuff.

[vabr (Chromium), 2012-11-30 22:55:18]

On 2012/11/30 20:08:18, vabr (Chromium) wrote:
> Hi Dominic,
> 
> Once you get out of the avalanche in your mailbox... :)
> Please take a look at this.
> 
> Once you are fine, I'll loop in Matt for the JSON rubber stamp, and Jochen for
> my use of the "first party for cookies" stuff.

Oh, the browser test WebRequestDeclarative fails on each platform for a
different reason. Fetching non-existing resources generates different error
codes based on platform, and on some of them the tests seem to have even more
problems.

I'll look into that, and will update this once the code is ready for review.
Please ignore until then.

Sorry for spamming you with this.

Vaclav

[vabr (Chromium), 2012-12-06 16:39:17]

Hi Dominic,

Please take a look at this new UrlFilter attribute for declarative Web Request
conditions.

Thanks,
Vaclav

On 2012/11/30 22:55:18, vabr (Chromium) wrote:
> On 2012/11/30 20:08:18, vabr (Chromium) wrote:
> > Hi Dominic,
> > 
> > Once you get out of the avalanche in your mailbox... :)
> > Please take a look at this.
> > 
> > Once you are fine, I'll loop in Matt for the JSON rubber stamp, and Jochen
for
> > my use of the "first party for cookies" stuff.
> 
> Oh, the browser test WebRequestDeclarative fails on each platform for a
> different reason. Fetching non-existing resources generates different error
> codes based on platform, and on some of them the tests seem to have even more
> problems.
> 
> I'll look into that, and will update this once the code is ready for review.
> Please ignore until then.
> 
> Sorry for spamming you with this.
> 
> Vaclav

[battre, 2012-12-06 19:53:26]

+Jeffrey as he is working on some refactoring as well.

Hi Vaclav,

I have added a couple of comments but some may be deprecated already. My
high-level feedback would resolve around two points:

1) I think that we should limit ourselves to a single URLMatcher. This should
speed things up, because the execution is in O(length of URL). By having tow
URLMatchers, we double the execution time.

2) I think we should do some refactoring to the rule triggers. I don't like the
artificial condition sets that are always true so much any more. This was OK in
the past where we had just one attribute that needed to be tested (the URL) but
could be empty. I propose to add support for URLMatcherConditionSets that don't
have triggers.

In WebRequestRulesRegistry we would add a
std::vector<WebRequestRule*> rules_without_triggers_;

in WebRequestConditionSet we would add a function
bool HasTriggers() const;
and a function
bool IsFulfilledWithoutTriggers() const;
which DCHECKs that match_triggers_ is empty and returns true if all conditions_
are fulfilled.

WDYT?

Best regards,
Dominic

https://codereview.chromium.org/11414230/diff/14001/chrome/browser/extensions...
File
chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.cc
(right):

https://codereview.chromium.org/11414230/diff/14001/chrome/browser/extensions...
chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.cc:81:
scoped_refptr<URLMatcherConditionSet> CreateAlwaysTrueUrlCondition(
How about
1) Renaming this to CreateAlwaysTrueConditionSet (drop Url, add Set)
2) Moving this to URLMatcherConditionFactory (as a non-static function)
?

https://codereview.chromium.org/11414230/diff/14001/chrome/browser/extensions...
chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.cc:119:
const bool name_is_url = condition_attribute_name == keys::kUrlKey;
Nit: I would prefer if you left this to have the structure
} else if (condition_attribute_name == keys::kUrlKey) {
...
} else if (condition_attribute_name == keys::kFirstPartyForCookiesUrlKey) {
...
}

I think this is easier to read. The amount of code duplication is not that big
in my opinion.

https://codereview.chromium.org/11414230/diff/14001/chrome/browser/extensions...
chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.cc:193:
match_triggers_[trigger_id] = i->get();
This is a serious performance regression as we would trigger the
WebRequetConditionSet for all URLs that don't have a first_party attribute.

The is what the logic should be:

- if exactly one condition set is non-trivial: add it as a trigger
- if both condition sets are non-trivial: add the more specific one because both
need to be fulfilled (I would use the url_matcher_condition_set as a rule of
thumb)
- if both condition sets are always true: add a single one, does not matter
which one.

https://codereview.chromium.org/11414230/diff/14001/chrome/browser/extensions...
chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.cc:207:
return trigger->second->IsFulfilled(request_data);
This is not sufficient in my opinion.  I think here we need to return whether
both are fulfilled.

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

https://codereview.chromium.org/11414230/diff/14001/chrome/browser/extensions...
chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_unittest.cc:27:
URLMatcher first_party_matcher;
I would suggest using a single URLMatcher to increase performance.

https://codereview.chromium.org/11414230/diff/14001/chrome/browser/extensions...
File
chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry.h
(right):

https://codereview.chromium.org/11414230/diff/14001/chrome/browser/extensions...
chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry.h:109:
class RuleTriggersOperator {
If it does something then the name should be verbal - but if we use it only
once, we should consider whether we are not adding more code by introducing the
operator than just doing the conversation ourselves in a loop.

https://codereview.chromium.org/11414230/diff/14001/chrome/browser/extensions...
chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry.h:119:
RuleTriggers triggers_;
Instantiating this operator is somewhat expensive as it causes a copy here.

[vabr (Chromium), 2012-12-07 15:31:22]

Hi Dominic,

Thanks for your answer and all comments!

Before I get to the other ones, I need to clarify this:

On 2012/12/06 19:53:26, battre wrote:
> 1) I think that we should limit ourselves to a single URLMatcher. This should
> speed things up, because the execution is in O(length of URL). By having tow
> URLMatchers, we double the execution time.

I'm not sure I understand where having two matchers makes things slower. As you
write, the execution time is linear in the length of the URL. Note that we now
have two URLs to test against: the request URL, and the first-party host (URL).
So either crammed into one modified URLMatcher, or put in two, the execution
time is still linear in sum of the lengths of these two URLs, no?

Another way to put this is that eventually we need to run Aho–Corasick for each
URL separately, be it inside one (modified) instance of UrlMatcher, or not.

Also, using only one URLMatcher would need significant changes inside that class
-- it would contain two groups of condition sets: the ones meant for the request
URL, and the ones meant for the first-party host. We would need to be able to
call MatchURL(some_url, which_group_of_conditions) instead of just
MatchURL(some_url).

Or did I misunderstand something?

Thanks,
Vaclav

[battre (please use the other), 2012-12-07 15:47:44]

You are absolutely right. This was a wrong thought.
Best regards,
Dominic
Am 07.12.2012 07:31 schrieb <vabr@chromium.org>:

> Hi Dominic,
>
> Thanks for your answer and all comments!
>
> Before I get to the other ones, I need to clarify this:
>
> On 2012/12/06 19:53:26, battre wrote:
>
>> 1) I think that we should limit ourselves to a single URLMatcher. This
>> should
>> speed things up, because the execution is in O(length of URL). By having
>> tow
>> URLMatchers, we double the execution time.
>>
>
> I'm not sure I understand where having two matchers makes things slower.
> As you
> write, the execution time is linear in the length of the URL. Note that we
> now
> have two URLs to test against: the request URL, and the first-party host
> (URL).
> So either crammed into one modified URLMatcher, or put in two, the
> execution
> time is still linear in sum of the lengths of these two URLs, no?
>
> Another way to put this is that eventually we need to run Aho–Corasick for
> each
> URL separately, be it inside one (modified) instance of UrlMatcher, or not.
>
> Also, using only one URLMatcher would need significant changes inside that
> class
> -- it would contain two groups of condition sets: the ones meant for the
> request
> URL, and the ones meant for the first-party host. We would need to be able
> to
> call MatchURL(some_url, which_group_of_conditions) instead of just
> MatchURL(some_url).
>
> Or did I misunderstand something?
>
> Thanks,
> Vaclav
>
>
https://codereview.chromium.**org/11414230/<https://codereview.chromium.org/1...
>

[battre, 2012-12-07 15:51:18]

No, actually, I think this was correct. Let's chat later today or on
Monday.
Am 07.12.2012 07:47 schrieb "Dominic Battre" <battre@google.com>:

> You are absolutely right. This was a wrong thought.
> Best regards,
> Dominic
> Am 07.12.2012 07:31 schrieb <vabr@chromium.org>:
>
>> Hi Dominic,
>>
>> Thanks for your answer and all comments!
>>
>> Before I get to the other ones, I need to clarify this:
>>
>> On 2012/12/06 19:53:26, battre wrote:
>>
>>> 1) I think that we should limit ourselves to a single URLMatcher. This
>>> should
>>> speed things up, because the execution is in O(length of URL). By having
>>> tow
>>> URLMatchers, we double the execution time.
>>>
>>
>> I'm not sure I understand where having two matchers makes things slower.
>> As you
>> write, the execution time is linear in the length of the URL. Note that
>> we now
>> have two URLs to test against: the request URL, and the first-party host
>> (URL).
>> So either crammed into one modified URLMatcher, or put in two, the
>> execution
>> time is still linear in sum of the lengths of these two URLs, no?
>>
>> Another way to put this is that eventually we need to run Aho–Corasick
>> for each
>> URL separately, be it inside one (modified) instance of UrlMatcher, or
>> not.
>>
>> Also, using only one URLMatcher would need significant changes inside
>> that class
>> -- it would contain two groups of condition sets: the ones meant for the
>> request
>> URL, and the ones meant for the first-party host. We would need to be
>> able to
>> call MatchURL(some_url, which_group_of_conditions) instead of just
>> MatchURL(some_url).
>>
>> Or did I misunderstand something?
>>
>> Thanks,
>> Vaclav
>>
>>
https://codereview.chromium.**org/11414230/<https://codereview.chromium.org/1...
>>
>

[vabr (Chromium), 2012-12-07 17:13:35]

:)
Feel free to ping me any time.

Vaclav

On 2012/12/07 15:51:18, battre wrote:
> No, actually, I think this was correct. Let's chat later today or on
> Monday.
> Am 07.12.2012 07:47 schrieb "Dominic Battre" <battre@google.com>:
> 
> > You are absolutely right. This was a wrong thought.
> > Best regards,
> > Dominic
> > Am 07.12.2012 07:31 schrieb <vabr@chromium.org>:

[Jeffrey Yasskin, 2012-12-18 03:09:58]

I only got through the tests and interfaces here. Some of these comments may be
irrelevant given changes from your refactoring CL.

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

https://codereview.chromium.org/11414230/diff/14001/chrome/browser/extensions...
chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.h:48:
URLMatcherConditionFactory* first_party_url_matcher_condition_factory,
Why are there two condition_factories when the other comments only refer to one
URLMatcher?

https://codereview.chromium.org/11414230/diff/14001/chrome/browser/extensions...
File
chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry.h
(right):

https://codereview.chromium.org/11414230/diff/14001/chrome/browser/extensions...
chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry.h:109:
class RuleTriggersOperator {
Generally I'd put a helper class like this into the .cc file, in an anonymous
namespace.

+1 to Dominic's question: If it's not saving code, and it doesn't have an
obvious name, don't extract it. I can imagine this having an obvious name
though: it's converting a std::map into a partial function, which is a totally
clear mathematical thing to do, obscured by trying to name it after a member
variable. If it doesn't save code in the context of this file, I'd still be
skeptical even with a clear name.

https://codereview.chromium.org/11414230/diff/14001/chrome/browser/extensions...
chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry.h:119:
RuleTriggers triggers_;
On 2012/12/06 19:53:26, battre wrote:
> Instantiating this operator is somewhat expensive as it causes a copy here.

That is, make the member into a pointer, and construct the functor from a
pointer to make the capturing clear.

https://codereview.chromium.org/11414230/diff/14001/chrome/browser/extensions...
chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry.h:123:
// the URLMatcherConditionSet::ID was returned by the |url_matcher_| and
That is, URLMatcherConditionSet::ID must be returned from both url_matcher_ and
first_party_url_matcher_ in order for you to look it that up in rule_triggers_?
Do you always test both, or only under certain conditions? If it's not always,
mention when or refer to the place that does explain when.

https://codereview.chromium.org/11414230/diff/14001/chrome/common/extensions/...
File chrome/common/extensions/api/declarative_web_request.json (right):

https://codereview.chromium.org/11414230/diff/14001/chrome/common/extensions/...
chrome/common/extensions/api/declarative_web_request.json:75: "description":
"Matches if the conditions of the UrlFilter are fulfilled for the URL
representing first party for cookies.",
How does a URL "represent first party for cookies"?

https://codereview.chromium.org/11414230/diff/14001/chrome/test/data/extensio...
File chrome/test/data/extensions/api_test/webrequest/test_declarative.js
(right):

https://codereview.chromium.org/11414230/diff/14001/chrome/test/data/extensio...
chrome/test/data/extensions/api_test/webrequest/test_declarative.js:283: expect(
Where is expect() defined? What do its parameters mean?

Which parts of this assertion were affected by the rule, and which weren't? I
don't feel like I can read the test as it currently stands and become confident
that the rules are matching as you expect. It might work better to run the test
twice, once with the firstPartyForCookiesUrl and once without, and show the
difference in behavior by modifying the "expected events" list.

https://codereview.chromium.org/11414230/diff/14001/chrome/test/data/extensio...
chrome/test/data/extensions/api_test/webrequest/test_declarative.js:292: {
label: "onBeforeSendHeaders",
If the label is the same as the event in almost all cases, that's a sign that
expect() isn't defined quite right. Perhaps make label optional, use the event
as the default, and then show both the event and label when 'label' is present.

https://codereview.chromium.org/11414230/diff/14001/chrome/test/data/extensio...
chrome/test/data/extensions/api_test/webrequest/test_declarative.js:381: // This
rule cancels everyhting, and sooner that the very first rule
sp: everyhting. s/that/than/

[vabr (Chromium), 2013-01-21 10:57:48]

Dominic, Jeffrey,

Thanks for your patience with this. Could you take another look?

Cheers,
Vaclav

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

https://codereview.chromium.org/11414230/diff/14001/chrome/browser/extensions...
chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.h:48:
URLMatcherConditionFactory* first_party_url_matcher_condition_factory,
This has now changed to multiple factories and multiple matchers.

On 2012/12/18 03:09:58, Jeffrey Yasskin wrote:
> Why are there two condition_factories when the other comments only refer to
one
> URLMatcher?

https://codereview.chromium.org/11414230/diff/14001/chrome/browser/extensions...
File
chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry.h
(right):

https://codereview.chromium.org/11414230/diff/14001/chrome/browser/extensions...
chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry.h:109:
class RuleTriggersOperator {
Thankfully I got rid of this sub-class completely.

On 2012/12/18 03:09:58, Jeffrey Yasskin wrote:
> Generally I'd put a helper class like this into the .cc file, in an anonymous
> namespace.
> 
> +1 to Dominic's question: If it's not saving code, and it doesn't have an
> obvious name, don't extract it. I can imagine this having an obvious name
> though: it's converting a std::map into a partial function, which is a totally
> clear mathematical thing to do, obscured by trying to name it after a member
> variable. If it doesn't save code in the context of this file, I'd still be
> skeptical even with a clear name.

https://codereview.chromium.org/11414230/diff/14001/chrome/browser/extensions...
chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry.h:119:
RuleTriggers triggers_;
Oh, that was a stupid mistake of mine. However, all removed by now.

On 2012/12/18 03:09:58, Jeffrey Yasskin wrote:
> On 2012/12/06 19:53:26, battre wrote:
> > Instantiating this operator is somewhat expensive as it causes a copy here.
> 
> That is, make the member into a pointer, and construct the functor from a
> pointer to make the capturing clear.

https://codereview.chromium.org/11414230/diff/14001/chrome/browser/extensions...
chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry.h:123:
// the URLMatcherConditionSet::ID was returned by the |url_matcher_| and
On 2012/12/18 03:09:58, Jeffrey Yasskin wrote:
> That is, URLMatcherConditionSet::ID must be returned from both url_matcher_
and
> first_party_url_matcher_ in order for you to look it that up in
rule_triggers_?

One matcher matching is enough. Corrected the "and" to "or", thanks for pointing
that out.

> Do you always test both, or only under certain conditions? If it's not always,
> mention when or refer to the place that does explain when.

I always put together matches from both matchers.

https://codereview.chromium.org/11414230/diff/14001/chrome/common/extensions/...
File chrome/common/extensions/api/declarative_web_request.json (right):

https://codereview.chromium.org/11414230/diff/14001/chrome/common/extensions/...
chrome/common/extensions/api/declarative_web_request.json:75: "description":
"Matches if the conditions of the UrlFilter are fulfilled for the URL
representing first party for cookies.",
On 2012/12/18 03:09:58, Jeffrey Yasskin wrote:
> How does a URL "represent first party for cookies"? 

When it is the same URL, as the one associated to the request as the first party
for cookies. It's bad if this sentence does not make sense, any suggestions for
improvement?

https://codereview.chromium.org/11414230/diff/14001/chrome/test/data/extensio...
File chrome/test/data/extensions/api_test/webrequest/test_declarative.js
(right):

https://codereview.chromium.org/11414230/diff/14001/chrome/test/data/extensio...
chrome/test/data/extensions/api_test/webrequest/test_declarative.js:283: expect(
On 2012/12/18 03:09:58, Jeffrey Yasskin wrote:
> Where is expect() defined? What do its parameters mean?
The function expect is part of the test framework:
https://cs.corp.google.com/#chrome/src/chrome/test/data/extensions/api_test/w...

> Which parts of this assertion were affected by the rule, and which weren't? I
> don't feel like I can read the test as it currently stands and become
confident
> that the rules are matching as you expect. It might work better to run the
test
> twice, once with the firstPartyForCookiesUrl and once without, and show the
> difference in behavior by modifying the "expected events" list.
I did not want to run the test twice, the tests take too long already. But I did
my best with adding comments about the important places in the event list.

https://codereview.chromium.org/11414230/diff/14001/chrome/test/data/extensio...
chrome/test/data/extensions/api_test/webrequest/test_declarative.js:292: {
label: "onBeforeSendHeaders",
I can understand your concerns, but I believe those are beyond this CL. I did
not introduce "expect", and I'm using it in a way similar to the rest of the
existing test.

Please file a crbug ticket if you want the implementation of expect improved.

On 2012/12/18 03:09:58, Jeffrey Yasskin wrote:
> If the label is the same as the event in almost all cases, that's a sign that
> expect() isn't defined quite right. Perhaps make label optional, use the event
> as the default, and then show both the event and label when 'label' is
present.

https://codereview.chromium.org/11414230/diff/14001/chrome/test/data/extensio...
chrome/test/data/extensions/api_test/webrequest/test_declarative.js:381: // This
rule cancels everyhting, and sooner that the very first rule
On 2012/12/18 03:09:58, Jeffrey Yasskin wrote:
> sp: everyhting. s/that/than/

Done.

https://codereview.chromium.org/11414230/diff/29011/chrome/browser/extensions...
File
chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry.cc
(right):

https://codereview.chromium.org/11414230/diff/29011/chrome/browser/extensions...
chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry.cc:44:
url_matches.insert(first_party.begin(), first_party.end());
This means that we first check all conditions triggered through a matching 'url'
attribute, then all conditions triggered through a matching
'firstPartyForCookiesUrl' attribute. For conditions which have both, but also
some non-URL attribute which is not satisfied, this means those are being
checked twice.

Is this a bearable performance burden, or should I try to implement some result
caching Condition::IsFulfilled? Note that the cache would need to check that
request_data did not change, which might be comparably expensive to checking one
or a couple of non-URL attributes.

[battre, 2013-01-21 17:32:08]

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

https://codereview.chromium.org/11414230/diff/29011/chrome/browser/extensions...
chrome/browser/extensions/api/declarative/declarative_rule.h:36: //   //
Arguments passed through from ConditionSet::Create.
can you please update ConditionSet->DeclarativeConditionSet

https://codereview.chromium.org/11414230/diff/29011/chrome/browser/extensions...
chrome/browser/extensions/api/declarative/declarative_rule.h:67: // array
|conditions| passed by the extension API.
Please add:
// As IsFulfilled gets passed the union of IDs of all matching
URLMatcherConditionSets coming from all URLMatcherConditionFactories, you should
make sure that the factories use strictly disjoined URLMatcherConditionSets::IDs
if you need to know which one had a match in IsFulfilled.

https://codereview.chromium.org/11414230/diff/29011/chrome/browser/extensions...
chrome/browser/extensions/api/declarative/declarative_rule.h:94: // Appends the
URLMatcherConditionSet from all conditions to |condition_sets|.
Please comment on the return value.

https://codereview.chromium.org/11414230/diff/29011/chrome/browser/extensions...
chrome/browser/extensions/api/declarative/declarative_rule.h:292: bool
positive_returned = false;
nit: rename this to "more_condition_sets_exist"?

https://codereview.chromium.org/11414230/diff/29011/chrome/browser/extensions...
chrome/browser/extensions/api/declarative/declarative_rule.h:330: index =
(*i)->GetURLMatcherConditionSets(&condition_sets, index);
should we check the invariants here?
int old_index = 0;
int index = 0;
while (index >= 0) {
  old_index = index;
  index = ...;
  CHECK(index < 0 || index = old_index + 1);
}

How about also checking that each GetURLMatcherConditionSets here returns unique
IDs?

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

https://codereview.chromium.org/11414230/diff/29011/chrome/browser/extensions...
chrome/browser/extensions/api/declarative/declarative_rule_unittest.cc:1: //
Copyright (c) 2013 The Chromium Authors. All rights reserved.
Do you want to add a unit test for the new code in declarative_rule.cc here?

https://codereview.chromium.org/11414230/diff/29011/chrome/browser/extensions...
File
chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.cc
(right):

https://codereview.chromium.org/11414230/diff/29011/chrome/browser/extensions...
chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.cc:99:
default:
nit: I would add a NOTREACHED(); here.

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

https://codereview.chromium.org/11414230/diff/29011/chrome/browser/extensions...
chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_unittest.cc:78:
result->GetURLMatcherConditionSets(&url_matcher_condition_set, 0);
check return value? also after line 81?
also check size of url_matcher_condition_set

https://codereview.chromium.org/11414230/diff/29011/chrome/browser/extensions...
chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_unittest.cc:89:
http_url);
nit: in my opinion a strange indentation

https://codereview.chromium.org/11414230/diff/29011/chrome/browser/extensions...
File
chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry.cc
(right):

https://codereview.chromium.org/11414230/diff/29011/chrome/browser/extensions...
chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry.cc:44:
url_matches.insert(first_party.begin(), first_party.end());
On 2013/01/21 10:57:48, vabr (Chromium) wrote:
> This means that we first check all conditions triggered through a matching
'url'
> attribute, then all conditions triggered through a matching
> 'firstPartyForCookiesUrl' attribute. For conditions which have both, but also
> some non-URL attribute which is not satisfied, this means those are being
> checked twice.
> 
> Is this a bearable performance burden, or should I try to implement some
result
> caching Condition::IsFulfilled? Note that the cache would need to check that
> request_data did not change, which might be comparably expensive to checking
one
> or a couple of non-URL attributes.

I think we are good here with the current approach.

https://codereview.chromium.org/11414230/diff/29011/chrome/test/data/extensio...
File chrome/test/data/extensions/api_test/webrequest/test_declarative.js
(right):

https://codereview.chromium.org/11414230/diff/29011/chrome/test/data/extensio...
chrome/test/data/extensions/api_test/webrequest/test_declarative.js:287: //  
fire later, onBeforeSendHeaders. This is necessary to avoid
It will fire later, in stage onBeforeSendHeaders.

[vabr (Chromium), 2013-01-21 18:39:29]

NOT READY FOR REVIEW YET -- NO RESPONSE REQUIRED YET

This is only a partial response, mainly a backup of changes made so far. I will
address the remaining two comments from Dominic in declarative_rule* tomorrow.

I will ping this once the CL is ready for review again.

Vaclav

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

https://codereview.chromium.org/11414230/diff/29011/chrome/browser/extensions...
chrome/browser/extensions/api/declarative/declarative_rule.h:36: //   //
Arguments passed through from ConditionSet::Create.
On 2013/01/21 17:32:08, battre wrote:
> can you please update ConditionSet->DeclarativeConditionSet

Done. To be correct, we would need to also add <ConditionT>, though.

(Personally I liked the original short version, which was easy to read, and with
the typedef from DeclarativeRule it was also quite clear to understand.)

https://codereview.chromium.org/11414230/diff/29011/chrome/browser/extensions...
chrome/browser/extensions/api/declarative/declarative_rule.h:67: // array
|conditions| passed by the extension API.
On 2013/01/21 17:32:08, battre wrote:
> Please add:
> // As IsFulfilled gets passed the union of IDs of all matching
> URLMatcherConditionSets coming from all URLMatcherConditionFactories, you
should
> make sure that the factories use strictly disjoined
URLMatcherConditionSets::IDs
> if you need to know which one had a match in IsFulfilled.

Done, with minor edits. Please check that you are fine with them.

https://codereview.chromium.org/11414230/diff/29011/chrome/browser/extensions...
chrome/browser/extensions/api/declarative/declarative_rule.h:94: // Appends the
URLMatcherConditionSet from all conditions to |condition_sets|.
On 2013/01/21 17:32:08, battre wrote:
> Please comment on the return value.

Done.

https://codereview.chromium.org/11414230/diff/29011/chrome/browser/extensions...
chrome/browser/extensions/api/declarative/declarative_rule.h:292: bool
positive_returned = false;
On 2013/01/21 17:32:08, battre wrote:
> nit: rename this to "more_condition_sets_exist"?

Done.

https://codereview.chromium.org/11414230/diff/29011/chrome/browser/extensions...
File
chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.cc
(right):

https://codereview.chromium.org/11414230/diff/29011/chrome/browser/extensions...
chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.cc:99:
default:
On 2013/01/21 17:32:08, battre wrote:
> nit: I would add a NOTREACHED(); here.

Done.

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

https://codereview.chromium.org/11414230/diff/29011/chrome/browser/extensions...
chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_unittest.cc:78:
result->GetURLMatcherConditionSets(&url_matcher_condition_set, 0);
On 2013/01/21 17:32:08, battre wrote:
> check return value? also after line 81?
> also check size of url_matcher_condition_set

Done.

https://codereview.chromium.org/11414230/diff/29011/chrome/browser/extensions...
chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_unittest.cc:89:
http_url);
On 2013/01/21 17:32:08, battre wrote:
> nit: in my opinion a strange indentation

Done. I was playing with the automatic indentation tool. :)

https://codereview.chromium.org/11414230/diff/29011/chrome/test/data/extensio...
File chrome/test/data/extensions/api_test/webrequest/test_declarative.js
(right):

https://codereview.chromium.org/11414230/diff/29011/chrome/test/data/extensio...
chrome/test/data/extensions/api_test/webrequest/test_declarative.js:287: //  
fire later, onBeforeSendHeaders. This is necessary to avoid
On 2013/01/21 17:32:08, battre wrote:
> It will fire later, in stage onBeforeSendHeaders.

Done.

[Jeffrey Yasskin, 2013-01-22 08:21:06]

On Mon, Jan 21, 2013 at 10:33 PM, Václav Brožek <vabr@google.com> wrote:
> I chose having multiple matchers and multiple factories because I found it
> less confusing than having one matcher and a set of matcher IDs of which at
> least one half is guaranteed to be irrelevant. The additional information
> about "which URL we matched against this time" would need more API changes
> than switching all URLMatcherConditionFactory* argument types to const
> std::vector<URLMatcherConditionFactory*>&. But if I missed something, please
> feel free to correct me.

I was thinking of the URLMatcher as a fast way to turn a URL into a set of
integers that's small relative to the number of rules. This lets us build an
index into the rules that tells us which rules need more inspection. Because
that's inherently a fuzzy process, I wasn't thinking of a URLMatcher as needing
to say *why* the rule is interesting, which means I don't need to care that some
of the results may be irrelevant.

Dominic's point, that we could get false positives when a request url matches a
rule's first-party-for-cookies filter, is certainly accurate, but it only
matters if the resulting extra work causes a noticeable performance change in
real rulesets.

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

https://codereview.chromium.org/11414230/diff/34002/chrome/browser/extensions...
chrome/browser/extensions/api/declarative/declarative_rule.h:46: //   // at 0
and increase by 1. The return value always gives the next higher
This seems too complicated, and you didn't manage to obey the contract (which
allows returning a value > index+1) in ConditionSet::GetURLMatcherConditionSets.
Maybe have it return true if index+1 is valid, or false if it's the highest
already?

https://codereview.chromium.org/11414230/diff/34002/chrome/browser/extensions...
chrome/browser/extensions/api/declarative/declarative_rule.h:70: // factories
use strictly disjoined URLMatcherConditionSets::IDs. This allows
How? Using a single URLMatcher would guarantee this trivially, which is one
reason I suggested doing that.

https://codereview.chromium.org/11414230/diff/34002/chrome/browser/extensions...
chrome/browser/extensions/api/declarative/declarative_rule.h:87: // whether the
corresponding condition is fulfilled wrt. |request_data|.
s/request_data/match_data/

https://codereview.chromium.org/11414230/diff/34002/chrome/browser/extensions...
chrome/browser/extensions/api/declarative/declarative_rule.h:88: // Therefore,
if |match_data| contains URL matches from more matchers, the
"more matchers" than what?

https://codereview.chromium.org/11414230/diff/34002/chrome/browser/extensions...
chrome/browser/extensions/api/declarative/declarative_rule.h:101: int
GetURLMatcherConditionSets(
It looks like every use of this function just iterates over all of the indices
and unions up all of the sets. If that's true, I think the original signature
was correct.

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

https://codereview.chromium.org/11414230/diff/34002/chrome/browser/extensions...
chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.h:24:
struct DeclarativeWebRequestData {
I had expected this change to add two fields to this type:

set<URLMatcherConditionSet::ID> url_match_ids;
set<URLMatcherConditionSet::ID> first_party_url_match_ids;

which the condition would use to tell if it matched.

https://codereview.chromium.org/11414230/diff/34002/chrome/browser/extensions...
chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.h:85:
// append them to this argument. With |index| 0 this returns the condition
Don't use magic numbers to identify kinds of URLs. At the very least, give them
enum values, but I'd prefer the new fields in DeclarativeWebRequestData that I
outlined above.

[vabr (Chromium), 2013-01-22 08:34:53]

Dominic,

Now I addressed all your comments, please take a look.

Jeffrey,

Please feel free to comment, especially about the way I pass the multiple
factories.

Thanks,
Vaclav

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

https://codereview.chromium.org/11414230/diff/29011/chrome/browser/extensions...
chrome/browser/extensions/api/declarative/declarative_rule.h:330: index =
(*i)->GetURLMatcherConditionSets(&condition_sets, index);
On 2013/01/21 17:32:08, battre wrote:
> should we check the invariants here?
> int old_index = 0;
> int index = 0;
> while (index >= 0) {
>   old_index = index;
>   index = ...;
>   CHECK(index < 0 || index = old_index + 1);
> }
> 
> How about also checking that each GetURLMatcherConditionSets here returns
unique
> IDs?

Done.

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

https://codereview.chromium.org/11414230/diff/29011/chrome/browser/extensions...
chrome/browser/extensions/api/declarative/declarative_rule_unittest.cc:1: //
Copyright (c) 2013 The Chromium Authors. All rights reserved.
On 2013/01/21 17:32:08, battre wrote:
> Do you want to add a unit test for the new code in declarative_rule.cc here?

Did you mean declarative_rule.h?
Most of the modifications there are in an already tested code, and I already
modified those tests.
The only new thing I could think of is the logic with indices in
GetURLMatcherConditionSets, so I added a test for that.

Is there anything else you had in mind?

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

https://codereview.chromium.org/11414230/diff/35004/chrome/browser/extensions...
chrome/browser/extensions/api/declarative/declarative_rule_unittest.cc:28:
struct MultipleFactoriesCondition {
I don't think that the layout of this struct conforms to the style guide, but it
is consistent with other similar structs in this file (including the missing
DISALLOW_COP_AND_ASSIGN). Let me know if you want me to change it.

[vabr (Chromium), 2013-01-22 14:54:30]

Dominic, Jeffrey,

So I finally understood the idea of Jeffrey, and tried to implement this CL with
only one matcher.

I agree with Jeffrey's reasons to have just one URL matcher, and I don't think
it brings significant performance drawbacks. To be precise, this is the
additional wasted effort when we have just one matcher:
URL matcher conditions intended for the request URL are also matched against the
first party URL, and similarly, URL matcher conditions for the first party URL
are matched against the request URL.

As far as I understood, the costly operation for the matcher is the
pre-computation for a new URL, and the cheap operation is to add URL matcher
conditions. This is why I have no concerns, because we did not add any more of
the expensive operations.

I did not perform any performance measurements. If anyone of you wants me to do
some, I'd be grateful for suggestions regarding the test scenario (e.g., how
many conditions for each URL should be used).

Jeffrey, from your last batch of comments I did not respond on those which
became obsolete, to keep it short. If you feel you miss a response from me
somewhere, please ping that comment.


PTAL,

Vaclav

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

https://codereview.chromium.org/11414230/diff/34002/chrome/browser/extensions...
chrome/browser/extensions/api/declarative/declarative_rule.h:87: // whether the
corresponding condition is fulfilled wrt. |request_data|.
On 2013/01/22 08:21:06, Jeffrey Yasskin wrote:
> s/request_data/match_data/

Done.

https://codereview.chromium.org/11414230/diff/34002/chrome/browser/extensions...
chrome/browser/extensions/api/declarative/declarative_rule.h:88: // Therefore,
if |match_data| contains URL matches from more matchers, the
On 2013/01/22 08:21:06, Jeffrey Yasskin wrote:
> "more matchers" than what?

I meant "multiple", i.e., more than 1. Changed.

EDIT: "Multiple matchers" does not make sense any more. If we decide to get rid
of |url_matches|, then that sentence will be erased. Otherwise I'll change "URL
matches from more matchers" to "matches from multiple URLs".

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

https://codereview.chromium.org/11414230/diff/34002/chrome/browser/extensions...
chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.h:24:
struct DeclarativeWebRequestData {
On 2013/01/22 08:21:06, Jeffrey Yasskin wrote:
> I had expected this change to add two fields to this type:
> 
> set<URLMatcherConditionSet::ID> url_match_ids;
> set<URLMatcherConditionSet::ID> first_party_url_match_ids;
> 
> which the condition would use to tell if it matched.

Thanks, Jeffrey, after this comment I finally understood how you suggested to
implement this with just one matcher.

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

https://codereview.chromium.org/11414230/diff/33018/chrome/browser/extensions...
chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.h:33:
std::set<URLMatcherConditionSet::ID> url_match_ids;
Adding the sets caused the need for off-lined constructors and destructor. An
alternative would be to have pointers or references to those sets, but
aggregating them in request data makes more sense from the life-time point of
view, and makes sure the sets don't get deleted earlier than the request data.

https://codereview.chromium.org/11414230/diff/33018/chrome/browser/extensions...
chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.h:76:
const std::set<URLMatcherConditionSet::ID>& /*url_matches*/,
Now that we ignore |url_matches| in favour of the two sets in |request_data|,
should we get rid of |url_matches| completely, including in the ConditionSet
template? Currently its presence only leads to generating unused copies and more
lines of code.

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

https://codereview.chromium.org/11414230/diff/33018/chrome/browser/extensions...
chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_unittest.cc:84:
std::set<URLMatcherConditionSet::ID> url_match_ids(
This would go away if we got rid of the now-unused first argument of
IsFulfilled.

https://codereview.chromium.org/11414230/diff/33018/chrome/browser/extensions...
File
chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry.cc
(right):

https://codereview.chromium.org/11414230/diff/33018/chrome/browser/extensions...
chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry.cc:44:
URLMatches all_url_matches = request_data->url_match_ids;
This would go away if we got rid of the now-unused first argument of
IsFulfilled.

[vabr (Chromium), 2013-01-23 17:08:14]

Update:
After talking to Dominic, I removed the redundant argument url_matches for
IsFulfilled.
This will have implications on other uses of the declarative condition set
templates -- Jeffrey are you fine with that?

Vaclav

[Jeffrey Yasskin, 2013-01-23 17:22:44]

On Wed, Jan 23, 2013 at 9:08 AM,  <vabr@chromium.org> wrote:
> Update:
> After talking to Dominic, I removed the redundant argument url_matches for
> IsFulfilled.
> This will have implications on other uses of the declarative condition set
> templates -- Jeffrey are you fine with that?

Yeah, that's fine. Sorry, I meant to say that yesterday.

[battre, 2013-01-23 18:56:34]

This is great. Much simpler and yet fast!

I have one final suggestion.

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

https://codereview.chromium.org/11414230/diff/27033/chrome/browser/extensions...
chrome/browser/extensions/api/declarative/declarative_rule_unittest.cc:114: bool
IsFulfilled(MatchData match_data) const {
pass by const ref?

https://codereview.chromium.org/11414230/diff/27033/chrome/browser/extensions...
File
chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry.cc
(right):

https://codereview.chromium.org/11414230/diff/27033/chrome/browser/extensions...
chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry.cc:34:
DeclarativeWebRequestData* request_data) {
What do you think this:

struct WebRequestData {
  // The network request that is currently being processed.
  net::URLRequest* request;
  // The stage (progress) of the network request.
  RequestStage stage;
  // Additional information about requests that is not
  // available in all request stages.
  const net::HttpResponseHeaders* original_response_headers;
};

struct DeclarativeWebRequestData {
  const WebRequestData* request_data;
  std::set<URLMatcherConditionSet::ID> url_match_ids;
  std::set<URLMatcherConditionSet::ID> first_party_url_match_ids;
};

std::set<const WebRequestRule*> WebRequestRulesRegistry::GetMatches(
    const WebRequestData& request_data) {
  DeclarativeWebRequestData declarative_request_data;
  declarative_request_data.request_data = &request_data;
  declarative_request_data.url_match_ids = ...
  ...

  for (...)
    if ((*it)->conditions().IsFulfilled(-1, declarative_request_data)) {
      result.insert(*it);
    }
}

This way the we can keep GetMatched and CreateDeltas purely without side-effects
and take const references as parameters.

[Jeffrey Yasskin, 2013-01-24 00:39:17]

LGTM. After considering the comments below, don't worry about waiting for
another LGTM from me unless you have particular questions.

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

https://codereview.chromium.org/11414230/diff/27033/chrome/browser/extensions...
chrome/browser/extensions/api/declarative/declarative_rule.h:59: // Factory
method that creates a WebRequestConditionSet according to the JSON
Oops, this should be "DeclarativeConditionSet".

https://codereview.chromium.org/11414230/diff/27033/chrome/browser/extensions...
chrome/browser/extensions/api/declarative/declarative_rule.h:76: // fulfilled.
If there is no such condition, then false is returned.
Probably mention |match_data| somewhere in here. Maybe as "...whether that
condition returns true from IsFulfilled(match_data)."

https://codereview.chromium.org/11414230/diff/27033/chrome/browser/extensions...
File
chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.cc
(right):

https://codereview.chromium.org/11414230/diff/27033/chrome/browser/extensions...
chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.cc:90:
url_matcher_conditions_->id())) ||
nit: I would generally split ||'ed conditions like this into two separate if()
statements, each of which returns false, since it makes each one easier to read.
That's totally a matter of taste though.

https://codereview.chromium.org/11414230/diff/27033/chrome/browser/extensions...
chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.cc:154:
if (!name_is_url) {
To avoid double-negatives (the else block happens if !!name_is_url), I'd reverse
the if and else blocks here.

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

https://codereview.chromium.org/11414230/diff/27033/chrome/browser/extensions...
chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_unittest.cc:64:
"  \"firstPartyForCookiesUrl\": { \"hostPrefix\": \"fpfc\"}, \n"
Instead of extending this test, I'd add a separate TEST() for
firstPartyForCookiesUrl. That way there's no worry that this CL changes the
behavior of the existing code, and future test failures point more precisely to
a particular attribute type.

https://codereview.chromium.org/11414230/diff/27033/chrome/browser/extensions...
chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_unittest.cc:171:
TEST(WebRequestConditionTest, CreateConditionSet) {
Consider removing this test entirely, although not necessarily in this change.
It should be covered by declarative_rule_unittest.

https://codereview.chromium.org/11414230/diff/27033/chrome/browser/extensions...
File
chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry.cc
(right):

https://codereview.chromium.org/11414230/diff/27033/chrome/browser/extensions...
chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry.cc:34:
DeclarativeWebRequestData* request_data) {
On 2013/01/23 18:56:34, battre wrote:
> What do you think this:
> 
> struct WebRequestData {
>   // The network request that is currently being processed.
>   net::URLRequest* request;
>   // The stage (progress) of the network request.
>   RequestStage stage;
>   // Additional information about requests that is not
>   // available in all request stages.
>   const net::HttpResponseHeaders* original_response_headers;
> };
> 
> struct DeclarativeWebRequestData {
>   const WebRequestData* request_data;
>   std::set<URLMatcherConditionSet::ID> url_match_ids;
>   std::set<URLMatcherConditionSet::ID> first_party_url_match_ids;
> };
> 
> std::set<const WebRequestRule*> WebRequestRulesRegistry::GetMatches(
>     const WebRequestData& request_data) {
>   DeclarativeWebRequestData declarative_request_data;
>   declarative_request_data.request_data = &request_data;
>   declarative_request_data.url_match_ids = ...
>   ...
> 
>   for (...)
>     if ((*it)->conditions().IsFulfilled(-1, declarative_request_data)) {
>       result.insert(*it);
>     }
> }
> 
> This way the we can keep GetMatched and CreateDeltas purely without
side-effects
> and take const references as parameters.

I don't have a strong opinion here, but I don't think WebRequestData vs
DeclarativeWebRequestData really captures the difference between these two
structs. I don't have better names though, so don't let me hold you up if you
want to split the struct.

https://codereview.chromium.org/11414230/diff/27033/chrome/browser/extensions...
File
chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry.h
(right):

https://codereview.chromium.org/11414230/diff/27033/chrome/browser/extensions...
chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry.h:78:
std::set<const WebRequestRule*> GetMatches(
Dominic told me to make ContentRulesRegistery::GetMatches const. Of course, no
need to do that for WebRequest in this change.

https://codereview.chromium.org/11414230/diff/27033/chrome/browser/extensions...
chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry.h:131:
// get added to |result| if one of their conditions fires. |request_data| get
s/fires/is fulfilled/?

https://codereview.chromium.org/11414230/diff/27033/chrome/common/extensions/...
File chrome/common/extensions/api/declarative_web_request.json (right):

https://codereview.chromium.org/11414230/diff/27033/chrome/common/extensions/...
chrome/common/extensions/api/declarative_web_request.json:75: "description":
"Matches if the conditions of the UrlFilter are fulfilled for the URL
representing first party for cookies.",
As a fairly naive user of this API, "representing first party for cookies"
doesn't mean much to me. Can you elaborate a bit in this description or link to
a more detailed explanation?

https://codereview.chromium.org/11414230/diff/27033/chrome/test/data/extensio...
File chrome/test/data/extensions/api_test/webrequest/test_declarative.js
(right):

https://codereview.chromium.org/11414230/diff/27033/chrome/test/data/extensio...
chrome/test/data/extensions/api_test/webrequest/test_declarative.js:280: //
Tests "firstPartyForCookiesUrl" by using it to override a cancelling rule.
Instead of doing this in an apitest, can you fake the relevant requests and test
this in a unittest? That'll run faster, and likely be clearer because you can
inspect the deltas directly instead of needing to use several rule priorities.
IMO, the API test should be limited to testing that the overall system works
together, as opposed to testing each individual piece.

https://codereview.chromium.org/11414230/diff/27033/chrome/test/data/extensio...
chrome/test/data/extensions/api_test/webrequest/test_declarative.js:292: // This
block of events is a successfull fetch of the main frame doc.
sp: successfull

[vabr (Chromium), 2013-01-24 08:31:01]

Hi Jeffrey and Dominic,

I'm just going through the comments, but would like a quick opinion on the split
of DeclarativeWebRequestData:

I thought about this, and did not dare to suggest the split, as it seemed too
complicated. But once you suggest it :), I'm for it.
I like the name of WebRequestData, but I would change DeclarativeWebRequestData
to WebRequestDataWithMatchIds. WDYT?

Vaclav

[battre, 2013-01-24 09:44:21]

On 2013/01/24 08:31:01, vabr (Chromium) wrote:
> Hi Jeffrey and Dominic,
> 
> I'm just going through the comments, but would like a quick opinion on the
split
> of DeclarativeWebRequestData:
> 
> I thought about this, and did not dare to suggest the split, as it seemed too
> complicated. But once you suggest it :), I'm for it.
> I like the name of WebRequestData, but I would change
DeclarativeWebRequestData
> to WebRequestDataWithMatchIds. WDYT?

Sounds good.

Dominic

[vabr (Chromium), 2013-01-24 18:24:08]

Dominic, Jeffrey,

I'm sorry to bother you once again, but please take a look.

Jeffrey, I'm not sure I addressed your comment about the apitest satisfactorily,
please let me know, what you think.

Vaclav

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

https://codereview.chromium.org/11414230/diff/27033/chrome/browser/extensions...
chrome/browser/extensions/api/declarative/declarative_rule.h:59: // Factory
method that creates a WebRequestConditionSet according to the JSON
On 2013/01/24 00:39:17, Jeffrey Yasskin wrote:
> Oops, this should be "DeclarativeConditionSet".

Done.

https://codereview.chromium.org/11414230/diff/27033/chrome/browser/extensions...
chrome/browser/extensions/api/declarative/declarative_rule.h:76: // fulfilled.
If there is no such condition, then false is returned.
On 2013/01/24 00:39:17, Jeffrey Yasskin wrote:
> Probably mention |match_data| somewhere in here. Maybe as "...whether that
> condition returns true from IsFulfilled(match_data)."

Done.

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

https://codereview.chromium.org/11414230/diff/27033/chrome/browser/extensions...
chrome/browser/extensions/api/declarative/declarative_rule_unittest.cc:114: bool
IsFulfilled(MatchData match_data) const {
On 2013/01/23 18:56:34, battre wrote:
> pass by const ref?

Done, thanks for catching this.

https://codereview.chromium.org/11414230/diff/27033/chrome/browser/extensions...
File
chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.cc
(right):

https://codereview.chromium.org/11414230/diff/27033/chrome/browser/extensions...
chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.cc:90:
url_matcher_conditions_->id())) ||
On 2013/01/24 00:39:17, Jeffrey Yasskin wrote:
> nit: I would generally split ||'ed conditions like this into two separate if()
> statements, each of which returns false, since it makes each one easier to
read.
> That's totally a matter of taste though.

Done. I agree, it's more readable, and since the body of the if statement is
very short, I am willing to risk code duplication. :)

https://codereview.chromium.org/11414230/diff/27033/chrome/browser/extensions...
chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.cc:154:
if (!name_is_url) {
On 2013/01/24 00:39:17, Jeffrey Yasskin wrote:
> To avoid double-negatives (the else block happens if !!name_is_url), I'd
reverse
> the if and else blocks here.

Done.

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

https://codereview.chromium.org/11414230/diff/27033/chrome/browser/extensions...
chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_unittest.cc:64:
"  \"firstPartyForCookiesUrl\": { \"hostPrefix\": \"fpfc\"}, \n"
On 2013/01/24 00:39:17, Jeffrey Yasskin wrote:
> Instead of extending this test, I'd add a separate TEST() for
> firstPartyForCookiesUrl. That way there's no worry that this CL changes the
> behavior of the existing code, and future test failures point more precisely
to
> a particular attribute type.

Done.

https://codereview.chromium.org/11414230/diff/27033/chrome/browser/extensions...
chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_unittest.cc:171:
TEST(WebRequestConditionTest, CreateConditionSet) {
On 2013/01/24 00:39:17, Jeffrey Yasskin wrote:
> Consider removing this test entirely, although not necessarily in this change.
> It should be covered by declarative_rule_unittest.

I don't think removing this test should be done in this CL, it has not much to
do with the rest.

Dominic, do you want me to remove this in a separate CL, or do you want to keep
that test?

It's true that the condition set's creation code is covered in
declarative_rule_unittest, and the condition's creation code in the first test
here.

https://codereview.chromium.org/11414230/diff/27033/chrome/browser/extensions...
File
chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry.cc
(right):

https://codereview.chromium.org/11414230/diff/27033/chrome/browser/extensions...
chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry.cc:34:
DeclarativeWebRequestData* request_data) {
On 2013/01/24 00:39:17, Jeffrey Yasskin wrote:
> On 2013/01/23 18:56:34, battre wrote:
> > What do you think this:
> > 
> > struct WebRequestData {
> >   // The network request that is currently being processed.
> >   net::URLRequest* request;
> >   // The stage (progress) of the network request.
> >   RequestStage stage;
> >   // Additional information about requests that is not
> >   // available in all request stages.
> >   const net::HttpResponseHeaders* original_response_headers;
> > };
> > 
> > struct DeclarativeWebRequestData {
> >   const WebRequestData* request_data;
> >   std::set<URLMatcherConditionSet::ID> url_match_ids;
> >   std::set<URLMatcherConditionSet::ID> first_party_url_match_ids;
> > };
> > 
> > std::set<const WebRequestRule*> WebRequestRulesRegistry::GetMatches(
> >     const WebRequestData& request_data) {
> >   DeclarativeWebRequestData declarative_request_data;
> >   declarative_request_data.request_data = &request_data;
> >   declarative_request_data.url_match_ids = ...
> >   ...
> > 
> >   for (...)
> >     if ((*it)->conditions().IsFulfilled(-1, declarative_request_data)) {
> >       result.insert(*it);
> >     }
> > }
> > 
> > This way the we can keep GetMatched and CreateDeltas purely without
> side-effects
> > and take const references as parameters.
> 
> I don't have a strong opinion here, but I don't think WebRequestData vs
> DeclarativeWebRequestData really captures the difference between these two
> structs. I don't have better names though, so don't let me hold you up if you
> want to split the struct.

I thought about this, and did not dare to suggest the split, as it seemed too
complicated. But once you suggest it :), I'm for it.
I like the name of WebRequestData, but I would change DeclarativeWebRequestData
to WebRequestDataWithMatchIds. WDYT?

https://codereview.chromium.org/11414230/diff/27033/chrome/browser/extensions...
File
chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry.h
(right):

https://codereview.chromium.org/11414230/diff/27033/chrome/browser/extensions...
chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry.h:78:
std::set<const WebRequestRule*> GetMatches(
On 2013/01/24 00:39:17, Jeffrey Yasskin wrote:
> Dominic told me to make ContentRulesRegistery::GetMatches const. Of course, no
> need to do that for WebRequest in this change.

Yay, hooray to const URLMatcher::MatchURL()! :)
Done.

https://codereview.chromium.org/11414230/diff/27033/chrome/browser/extensions...
chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry.h:131:
// get added to |result| if one of their conditions fires. |request_data| get
On 2013/01/24 00:39:17, Jeffrey Yasskin wrote:
> s/fires/is fulfilled/?

Done.

https://codereview.chromium.org/11414230/diff/27033/chrome/common/extensions/...
File chrome/common/extensions/api/declarative_web_request.json (right):

https://codereview.chromium.org/11414230/diff/27033/chrome/common/extensions/...
chrome/common/extensions/api/declarative_web_request.json:75: "description":
"Matches if the conditions of the UrlFilter are fulfilled for the URL
representing first party for cookies.",
On 2013/01/24 00:39:17, Jeffrey Yasskin wrote:
> As a fairly naive user of this API, "representing first party for cookies"
> doesn't mean much to me. Can you elaborate a bit in this description or link
to
> a more detailed explanation?

Done. How does that sound now?

https://codereview.chromium.org/11414230/diff/27033/chrome/test/data/extensio...
File chrome/test/data/extensions/api_test/webrequest/test_declarative.js
(right):

https://codereview.chromium.org/11414230/diff/27033/chrome/test/data/extensio...
chrome/test/data/extensions/api_test/webrequest/test_declarative.js:280: //
Tests "firstPartyForCookiesUrl" by using it to override a cancelling rule.
On 2013/01/24 00:39:17, Jeffrey Yasskin wrote:
> Instead of doing this in an apitest, can you fake the relevant requests and
test
> this in a unittest? That'll run faster, and likely be clearer because you can
> inspect the deltas directly instead of needing to use several rule priorities.
> IMO, the API test should be limited to testing that the overall system works
> together, as opposed to testing each individual piece.

I agree with this test being too heavy, so I had to rethink my objectives. This
is what I wanted to test:
1. That 'firstPartyForCookiesUrl' works end-to-end.
2. That it is evaluated against the firstPartyForCookies URL, as opposed to the
'url' attribute.

After talking to Dominic, I still want to test 1. with an apitest. All
Declarative WR attributes get tested end-to-end this way. If this can be
improved, we should plan for a switch of all of them to other kind of tests, but
that's beyond this CL.

However, for testing 1. I can use (and used) a much simpler apitest.

To test 2. I wrote a unittest for rules registry.

Is that acceptable to you, Jeffrey?

https://codereview.chromium.org/11414230/diff/27033/chrome/test/data/extensio...
chrome/test/data/extensions/api_test/webrequest/test_declarative.js:292: // This
block of events is a successfull fetch of the main frame doc.
On 2013/01/24 00:39:17, Jeffrey Yasskin wrote:
> sp: successfull

Done (and then removed completely :)).

[Jeffrey Yasskin, 2013-01-24 19:54:46]

lgtm

https://codereview.chromium.org/11414230/diff/27033/chrome/common/extensions/...
File chrome/common/extensions/api/declarative_web_request.json (right):

https://codereview.chromium.org/11414230/diff/27033/chrome/common/extensions/...
chrome/common/extensions/api/declarative_web_request.json:75: "description":
"Matches if the conditions of the UrlFilter are fulfilled for the URL
representing first party for cookies.",
On 2013/01/24 18:24:08, vabr (Chromium) wrote:
> On 2013/01/24 00:39:17, Jeffrey Yasskin wrote:
> > As a fairly naive user of this API, "representing first party for cookies"
> > doesn't mean much to me. Can you elaborate a bit in this description or link
> to
> > a more detailed explanation?
> 
> Done. How does that sound now?

Not great, sorry to say. Again, go ahead and submit without getting this
perfect. We can work on it later, possibly with Meggin's input.

https://codereview.chromium.org/11414230/diff/27033/chrome/test/data/extensio...
File chrome/test/data/extensions/api_test/webrequest/test_declarative.js
(right):

https://codereview.chromium.org/11414230/diff/27033/chrome/test/data/extensio...
chrome/test/data/extensions/api_test/webrequest/test_declarative.js:280: //
Tests "firstPartyForCookiesUrl" by using it to override a cancelling rule.
On 2013/01/24 18:24:08, vabr (Chromium) wrote:
> On 2013/01/24 00:39:17, Jeffrey Yasskin wrote:
> > Instead of doing this in an apitest, can you fake the relevant requests and
> test
> > this in a unittest? That'll run faster, and likely be clearer because you
can
> > inspect the deltas directly instead of needing to use several rule
priorities.
> > IMO, the API test should be limited to testing that the overall system works
> > together, as opposed to testing each individual piece.
> 
> I agree with this test being too heavy, so I had to rethink my objectives.
This
> is what I wanted to test:
> 1. That 'firstPartyForCookiesUrl' works end-to-end.
> 2. That it is evaluated against the firstPartyForCookies URL, as opposed to
the
> 'url' attribute.
> 
> After talking to Dominic, I still want to test 1. with an apitest. All
> Declarative WR attributes get tested end-to-end this way. If this can be
> improved, we should plan for a switch of all of them to other kind of tests,
but
> that's beyond this CL.
> 
> However, for testing 1. I can use (and used) a much simpler apitest.
> 
> To test 2. I wrote a unittest for rules registry.
> 
> Is that acceptable to you, Jeffrey?

Totally acceptable for this change. I think that I disagree that all
declarativeWebRequest attributes should be individually tested in an apitest,
but there's no need to change that in this CL.

https://codereview.chromium.org/11414230/diff/32018/chrome/browser/extensions...
File
chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry_unittest.cc
(right):

https://codereview.chromium.org/11414230/diff/32018/chrome/browser/extensions...
chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry_unittest.cc:530:
const char* matchingRuleIds[] = { kRuleId1, kRuleId2 };
It makes me nervous when a test changes both the input and the code that runs at
the same time. When you're looking for code (rules in this case) to be sensitive
to inputs, you need to run the same code over different inputs or the same input
through different code. Running different input through different code lets bugs
creep in.

https://codereview.chromium.org/11414230/diff/32018/chrome/browser/extensions...
chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry_unittest.cc:542:
EXPECT_EQ(1u, matches.size());
When you put an EXPECT in a loop, always be sure to include enough context to
figure out which iteration it failed in. SCOPED_TRACE
(https://code.google.com/p/googletest/wiki/V1_6_AdvancedGuide#Adding_Traces_to...)
can be enough for this.

https://codereview.chromium.org/11414230/diff/32018/chrome/browser/extensions...
chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry_unittest.cc:545:
EXPECT_EQ(expected_pair, (*matches.begin())->id());
I tend to prefer inlining the construction of the expected value. gtest prints
the source code it saw, and "expected_pair" is less informative than
"std::make_pair(kExtensionId, matchingRuleIds[i]);", which in turn would be less
informative than "std::make_pair(kExtensionId, kRuleId1);"

[vabr (Chromium), 2013-01-24 21:12:19]

Hi Yoyo,
I need a rubber-stamp (but any comments welcome) from you on
chrome/common/extensions/api/declarative_web_request.json. The rest of this CL
is already being reviewed by Dominic and Jeffrey. You may want to take a look at
the comments here:
https://codereview.chromium.org/11414230/diff/27033/chrome/common/extensions/...
(but beware that those are not on the latest patch set).

Jeffrey,
Thanks for your comments. They should be addressed by now.

Dominic, are you fine with the CL as it is now?

Thanks all,
Vaclav

https://codereview.chromium.org/11414230/diff/27033/chrome/common/extensions/...
File chrome/common/extensions/api/declarative_web_request.json (right):

https://codereview.chromium.org/11414230/diff/27033/chrome/common/extensions/...
chrome/common/extensions/api/declarative_web_request.json:75: "description":
"Matches if the conditions of the UrlFilter are fulfilled for the URL
representing first party for cookies.",
On 2013/01/24 19:54:46, Jeffrey Yasskin wrote:
> On 2013/01/24 18:24:08, vabr (Chromium) wrote:
> > On 2013/01/24 00:39:17, Jeffrey Yasskin wrote:
> > > As a fairly naive user of this API, "representing first party for cookies"
> > > doesn't mean much to me. Can you elaborate a bit in this description or
link
> > to
> > > a more detailed explanation?
> > 
> > Done. How does that sound now?
> 
> Not great, sorry to say. Again, go ahead and submit without getting this
> perfect. We can work on it later, possibly with Meggin's input.

I agree. Unless Yoyo, who I need to review this file anyway, has a good
suggestion at this time, let's postpone rewording it to another CL.

https://codereview.chromium.org/11414230/diff/32018/chrome/browser/extensions...
File
chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry_unittest.cc
(right):

https://codereview.chromium.org/11414230/diff/32018/chrome/browser/extensions...
chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry_unittest.cc:530:
const char* matchingRuleIds[] = { kRuleId1, kRuleId2 };
On 2013/01/24 19:54:46, Jeffrey Yasskin wrote:
> It makes me nervous when a test changes both the input and the code that runs
at
> the same time. When you're looking for code (rules in this case) to be
sensitive
> to inputs, you need to run the same code over different inputs or the same
input
> through different code. Running different input through different code lets
bugs
> creep in.

Clarified with Jeffrey, that both rules are used in each iteration of the for
loop. So it is only the inputs which change.

https://codereview.chromium.org/11414230/diff/32018/chrome/browser/extensions...
chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry_unittest.cc:542:
EXPECT_EQ(1u, matches.size());
On 2013/01/24 19:54:46, Jeffrey Yasskin wrote:
> When you put an EXPECT in a loop, always be sure to include enough context to
> figure out which iteration it failed in. SCOPED_TRACE
>
(https://code.google.com/p/googletest/wiki/V1_6_AdvancedGuide#Adding_Traces_to...)
> can be enough for this.

Thanks! Done.

https://codereview.chromium.org/11414230/diff/32018/chrome/browser/extensions...
chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry_unittest.cc:545:
EXPECT_EQ(expected_pair, (*matches.begin())->id());
On 2013/01/24 19:54:46, Jeffrey Yasskin wrote:
> I tend to prefer inlining the construction of the expected value. gtest prints
> the source code it saw, and "expected_pair" is less informative than
> "std::make_pair(kExtensionId, matchingRuleIds[i]);", which in turn would be
less
> informative than "std::make_pair(kExtensionId, kRuleId1);"

Done. Which rule ID it is should be now clear from the SCOPED_TRACE.

[Yoyo Zhou, 2013-01-24 21:29:42]

https://codereview.chromium.org/11414230/diff/40013/chrome/common/extensions/...
File chrome/common/extensions/api/declarative_web_request.json (right):

https://codereview.chromium.org/11414230/diff/40013/chrome/common/extensions/...
chrome/common/extensions/api/declarative_web_request.json:75: "description":
"Each URL request can be assigned a special URL, different from the request's
target URL. The special URL describes what is considered 'first party' for the
sake of third-party checks for cookies. This attribute allows to match against
the special URL.",
This should have the form "Matches..." so perhaps
"Matches if the special first-party cookies URL is contained in the list."

[vabr (Chromium), 2013-01-25 06:37:40]

Thanks, Yoyo, PTAL.

Dominic, Jeffrey, any more comments since your last review?

Vaclav

https://codereview.chromium.org/11414230/diff/40013/chrome/common/extensions/...
File chrome/common/extensions/api/declarative_web_request.json (right):

https://codereview.chromium.org/11414230/diff/40013/chrome/common/extensions/...
chrome/common/extensions/api/declarative_web_request.json:75: "description":
"Each URL request can be assigned a special URL, different from the request's
target URL. The special URL describes what is considered 'first party' for the
sake of third-party checks for cookies. This attribute allows to match against
the special URL.",
On 2013/01/24 21:29:43, Yoyo Zhou wrote:
> This should have the form "Matches..." so perhaps
> "Matches if the special first-party cookies URL is contained in the list."

Rephrased. Better now?

[battre, 2013-01-25 16:59:52]

Great!

LGTM

[Yoyo Zhou, 2013-01-25 17:12:20]

LGTM

[commit-bot: I haz the power, 2013-01-25 17:23:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vabr@chromium.org/11414230/32039

[commit-bot: I haz the power, 2013-01-25 20:35:52]

Change committed as 178882