Adding condition attributes for request headers to Declarative WebRequest

Adding condition attributes for request headers to Declarative WebRequest

TEST=Install the extension "declarative_request_headers_match.crx" attached to BUG 143662. If this feature is working correctly it should cancel every navigation.
BUG=143662, 112155
TBR=yoz@chromium.org

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=162119

[vabr (Chromium), 2012-10-04 11:53:59]

Hi Dominic,

Request headers filter is here. Can you please take a look?

Thanks,
Vaclav

[battre, 2012-10-05 12:50:03]

https://codereview.chromium.org/11038031/diff/2002/chrome/browser/extensions/...
File
chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute.cc
(right):

https://codereview.chromium.org/11038031/diff/2002/chrome/browser/extensions/...
chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute.cc:498:
scoped_ptr<const HeaderMatcher>* header_matcher,
This usage of a scoped_ptr is wrong. Can you please fix this also for
WebRequestConditionAttributeResponseHeaders?

Correct:

WebRequestConditionAttributeRequestHeaders:: 
WebRequestConditionAttributeRequestHeaders(
    scoped_ptr<const HeaderMatcher>* header_matcher,
    bool positive)
    : header_matcher_(header_matcher.Pass()),
      positive_(positive) {}

https://codereview.chromium.org/11038031/diff/2002/chrome/browser/extensions/...
chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute.cc:549:
&header_matcher, name == keys::kRequestHeadersKey));
&header_matcher should be replaces with header_matcher.Pass().

https://codereview.chromium.org/11038031/diff/2002/chrome/browser/extensions/...
File
chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute_unittest.cc
(right):

https://codereview.chromium.org/11038031/diff/2002/chrome/browser/extensions/...
chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute_unittest.cc:276:
std::vector< std::vector<const std::string*> > tests;
nit: the first space after std::vector< does not belong there.

https://codereview.chromium.org/11038031/diff/2002/chrome/browser/extensions/...
chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute_unittest.cc:277:
bool result;
can you please initialize this to false by default?

https://codereview.chromium.org/11038031/diff/2002/chrome/browser/extensions/...
chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute_unittest.cc:280:
MatchAndCheck(tests, key, ON_BEFORE_SEND_HEADERS, &url_request, &result);
How about defining a variable stage and then repeating MatchAndCheck each time?
This is easier on code search and grep without sacrificing the 80 character
limit.

https://codereview.chromium.org/11038031/diff/2002/chrome/browser/extensions/...
chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute_unittest.cc:329:
// Negative filter, failing.
Can you remind me again, why this is supposed to fail?

https://codereview.chromium.org/11038031/diff/2002/chrome/common/extensions/a...
File chrome/common/extensions/api/declarative_web_request.json (right):

https://codereview.chromium.org/11038031/diff/2002/chrome/common/extensions/a...
chrome/common/extensions/api/declarative_web_request.json:100: "description":
"Matches if none of the request headers is matched by one of the
HeaderFilters.",
nit: "by one of the" -> "by any of the"

https://codereview.chromium.org/11038031/diff/2002/chrome/test/data/extension...
File chrome/test/data/extensions/api_test/webrequest/test_declarative.js
(right):

https://codereview.chromium.org/11038031/diff/2002/chrome/test/data/extension...
chrome/test/data/extensions/api_test/webrequest/test_declarative.js:535:
function() {navigateAndWait(getURLHttpWithHeaders());}
This URL returns a response with specific headers. The request headers are the
default headers. I think this test would pass if requestHeaders and
excludeRequestHeaders would do nothing.

[battre, 2012-10-05 15:45:48]

http://codereview.chromium.org/11038031/diff/2002/chrome/test/data/extensions...
File chrome/test/data/extensions/api_test/webrequest/test_declarative.js
(right):

http://codereview.chromium.org/11038031/diff/2002/chrome/test/data/extensions...
chrome/test/data/extensions/api_test/webrequest/test_declarative.js:535:
function() {navigateAndWait(getURLHttpWithHeaders());}
On 2012/10/05 12:50:03, battre wrote:
> This URL returns a response with specific headers. The request headers are the
> default headers. I think this test would pass if requestHeaders and
> excludeRequestHeaders would do nothing.

As discussed, I think that the unit tests in combination with this are
sufficient.

[vabr (Chromium), 2012-10-05 15:56:53]

Thanks, Dominic.

I addressed all your comments, please have another look.

Vaclav

https://codereview.chromium.org/11038031/diff/2002/chrome/browser/extensions/...
File
chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute.cc
(right):

https://codereview.chromium.org/11038031/diff/2002/chrome/browser/extensions/...
chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute.cc:498:
scoped_ptr<const HeaderMatcher>* header_matcher,
On 2012/10/05 12:50:03, battre wrote:
> This usage of a scoped_ptr is wrong. Can you please fix this also for
> WebRequestConditionAttributeResponseHeaders?
> 
> Correct:
> 
> WebRequestConditionAttributeRequestHeaders:: 
> WebRequestConditionAttributeRequestHeaders(
>     scoped_ptr<const HeaderMatcher>* header_matcher,
>     bool positive)
>     : header_matcher_(header_matcher.Pass()),
>       positive_(positive) {}

Done.

https://codereview.chromium.org/11038031/diff/2002/chrome/browser/extensions/...
chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute.cc:549:
&header_matcher, name == keys::kRequestHeadersKey));
On 2012/10/05 12:50:03, battre wrote:
> &header_matcher should be replaces with header_matcher.Pass().

Done.

https://codereview.chromium.org/11038031/diff/2002/chrome/browser/extensions/...
File
chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute_unittest.cc
(right):

https://codereview.chromium.org/11038031/diff/2002/chrome/browser/extensions/...
chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute_unittest.cc:276:
std::vector< std::vector<const std::string*> > tests;
On 2012/10/05 12:50:03, battre wrote:
> nit: the first space after std::vector< does not belong there.

Actually, the Google style guide allows it (search for "symmetric spacing"), and
the Chrome style guide does not change that.
I still changed it according to your suggestion, for compatibility reasons,
because you, as an OWNER, seem to prefer it.

https://codereview.chromium.org/11038031/diff/2002/chrome/browser/extensions/...
chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute_unittest.cc:277:
bool result;
On 2012/10/05 12:50:03, battre wrote:
> can you please initialize this to false by default?

Done.

https://codereview.chromium.org/11038031/diff/2002/chrome/browser/extensions/...
chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute_unittest.cc:280:
MatchAndCheck(tests, key, ON_BEFORE_SEND_HEADERS, &url_request, &result);
On 2012/10/05 12:50:03, battre wrote:
> How about defining a variable stage and then repeating MatchAndCheck each
time?
> This is easier on code search and grep without sacrificing the 80 character
> limit.

Done. Sometimes it overflows, but I think the code search improvement is worth
it.

Also did this below for MATCH_RESPONSE_HEADERS.

https://codereview.chromium.org/11038031/diff/2002/chrome/browser/extensions/...
chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute_unittest.cc:329:
// Negative filter, failing.
On 2012/10/05 12:50:03, battre wrote:
> Can you remind me again, why this is supposed to fail?

It corresponds to constructing one RequestMatcher object with a single
excludeRequestHeaders criterion. That criterion is an empty HeaderFilter, result
of which will be negated. Because a HeaderFilter applies all its (sub-)criteria
in conjunction, this results in a match, which after negation results in a
failure to match.

I added the fact about conjunction into HeaderFilter documentation.

https://codereview.chromium.org/11038031/diff/2002/chrome/common/extensions/a...
File chrome/common/extensions/api/declarative_web_request.json (right):

https://codereview.chromium.org/11038031/diff/2002/chrome/common/extensions/a...
chrome/common/extensions/api/declarative_web_request.json:100: "description":
"Matches if none of the request headers is matched by one of the
HeaderFilters.",
On 2012/10/05 12:50:03, battre wrote:
> nit: "by one of the" -> "by any of the"

Done, also for excludeResponseHeaders.

https://codereview.chromium.org/11038031/diff/2002/chrome/test/data/extension...
File chrome/test/data/extensions/api_test/webrequest/test_declarative.js
(right):

https://codereview.chromium.org/11038031/diff/2002/chrome/test/data/extension...
chrome/test/data/extensions/api_test/webrequest/test_declarative.js:535:
function() {navigateAndWait(getURLHttpWithHeaders());}
On 2012/10/05 15:45:49, battre wrote:
> On 2012/10/05 12:50:03, battre wrote:
> > This URL returns a response with specific headers. The request headers are
the
> > default headers. I think this test would pass if requestHeaders and
> > excludeRequestHeaders would do nothing.
> 
> As discussed, I think that the unit tests in combination with this are
> sufficient.

OK. I at least changed the URL to something more simple, to avoid confusion with
the headers. It does not matter what the URL is, as long as it is valid.

[battre, 2012-10-15 07:12:21]

LGTM

http://codereview.chromium.org/11038031/diff/5005/chrome/browser/extensions/a...
File
chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute.cc
(right):

http://codereview.chromium.org/11038031/diff/5005/chrome/browser/extensions/a...
chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute.cc:569:
net::HttpRequestHeaders::Iterator it(*headers);
nit, you could store a const ref in line 565 so that you don't have to
dereference it here.

http://codereview.chromium.org/11038031/diff/5005/chrome/browser/extensions/a...
chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute.cc:572:
}
nit: you may delete the {} here.

http://codereview.chromium.org/11038031/diff/5005/chrome/common/extensions/ap...
File chrome/common/extensions/api/declarative_web_request.json (right):

http://codereview.chromium.org/11038031/diff/5005/chrome/common/extensions/ap...
chrome/common/extensions/api/declarative_web_request.json:13: "description":
"Filters request headers for various criteria. Multiple criteria are evaluated
as conjunction.",
nit: "as a conjunction"?

[vabr (Chromium), 2012-10-15 11:59:10]

Thanks Dominic,

Comments addressed, sending to CQ.

TBR-ing Yoyo for OWNERS rubber stamp of
chrome/common/extensions/api/declarative_web_request.json.

Vaclav

http://codereview.chromium.org/11038031/diff/5005/chrome/browser/extensions/a...
File
chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute.cc
(right):

http://codereview.chromium.org/11038031/diff/5005/chrome/browser/extensions/a...
chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute.cc:569:
net::HttpRequestHeaders::Iterator it(*headers);
On 2012/10/15 07:12:21, battre wrote:
> nit, you could store a const ref in line 565 so that you don't have to
> dereference it here.

Done.

http://codereview.chromium.org/11038031/diff/5005/chrome/browser/extensions/a...
chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute.cc:572:
}
On 2012/10/15 07:12:21, battre wrote:
> nit: you may delete the {} here.

Done.

http://codereview.chromium.org/11038031/diff/5005/chrome/common/extensions/ap...
File chrome/common/extensions/api/declarative_web_request.json (right):

http://codereview.chromium.org/11038031/diff/5005/chrome/common/extensions/ap...
chrome/common/extensions/api/declarative_web_request.json:13: "description":
"Filters request headers for various criteria. Multiple criteria are evaluated
as conjunction.",
On 2012/10/15 07:12:21, battre wrote:
> nit: "as a conjunction"?

Done.

[commit-bot: I haz the power, 2012-10-15 11:59:22]

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

[commit-bot: I haz the power, 2012-10-15 15:10:52]

Retried try job too often for step(s) browser_tests

[commit-bot: I haz the power, 2012-10-16 07:44:40]

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

[commit-bot: I haz the power, 2012-10-16 10:48:49]

Change committed as 162119