Extracting header testing code from WebRequestConditionAttributeResponseHeaders.

Extracting header testing code from WebRequestConditionAttributeResponseHeaders.

In preparation for adding a new condition attribute for matching request
headers, the code independent of the headers being 'response' or 'request'
is extracted and put into a separate class TestManager to be easily reused.

BUG=143662, 112155
TEST=This change should introduce no change in behaviour and no new features. To check that it does not break the old functionality, install the extension attached to BUG 143662. If this feature is working correctly the extension should cancel every navigation.


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

[vabr (Chromium), 2012-08-31 15:34:25]

Hi Yoyo,

Do you think you could review this? I'm trying to load-balance Dominic, and you
recently touched this part of code.

Thanks,
Vaclav

[Yoyo Zhou, 2012-09-01 19:44:50]

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

http://codereview.chromium.org/10923002/diff/2001/chrome/browser/extensions/a...
chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute.cc:473:
// We never get past the "switch", but the compiler worries about no return.
I think this is usually in a default: section (as long as the compiler does not
still complain).

http://codereview.chromium.org/10923002/diff/2001/chrome/browser/extensions/a...
File
chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute.h
(right):

http://codereview.chromium.org/10923002/diff/2001/chrome/browser/extensions/a...
chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute.h:30:
class TestManager;
This name is kind of unclear. My first impression was that this was for unit
tests or some such. Can it be more descriptive, like FooTestManager? (I'm not
sure what the best foo is.) (And could Manager be more descriptive too?)

[vabr (Chromium), 2012-09-03 07:37:05]

Thanks, Yoyo, for the comments.
I addressed them, and corrected a couple of other minor things (indenting,
variable names, comments).
PTAL.

Vaclav

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

http://codereview.chromium.org/10923002/diff/2001/chrome/browser/extensions/a...
chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute.cc:473:
// We never get past the "switch", but the compiler worries about no return.
On 2012/09/01 19:44:50, Yoyo Zhou wrote:
> I think this is usually in a default: section (as long as the compiler does
not
> still complain).

Done.

http://codereview.chromium.org/10923002/diff/2001/chrome/browser/extensions/a...
File
chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute.h
(right):

http://codereview.chromium.org/10923002/diff/2001/chrome/browser/extensions/a...
chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute.h:30:
class TestManager;
On 2012/09/01 19:44:50, Yoyo Zhou wrote:
> This name is kind of unclear. My first impression was that this was for unit
> tests or some such. Can it be more descriptive, like FooTestManager? (I'm not
> sure what the best foo is.) (And could Manager be more descriptive too?)

Good point. I decided to get rid of "test" in the name as this has too many
meanings.
What about "HeaderMatcher"? It is used to match single headers against groups of
patterns. It is also independent of whether the headers are response headers or
request headers (and will be shared by code for both types), so I don't put any
restriction to one of those header types in the name.

[battre, 2012-09-03 08:46:46]

http://codereview.chromium.org/10923002/diff/11002/chrome/browser/extensions/...
File
chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute.cc
(left):

http://codereview.chromium.org/10923002/diff/11002/chrome/browser/extensions/...
chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute.cc:419:
*error = ExtensionErrorUtils::FormatErrorMessage(kInvalidValue, *key);
why did you remove this?

http://codereview.chromium.org/10923002/diff/11002/chrome/browser/extensions/...
chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute.cc:446:
*error = ExtensionErrorUtils::FormatErrorMessage(kInvalidValue, *key);
why did you remove this?

http://codereview.chromium.org/10923002/diff/11002/chrome/browser/extensions/...
File
chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute.cc
(right):

http://codereview.chromium.org/10923002/diff/11002/chrome/browser/extensions/...
chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute.cc:266:
MatchType match_type);
You can do this in a separate CL, but I think we can clean this up by removing
is_name_test. That way StringMatchTest does not need to know about the concept
of key/value paris of headers. Also the StringToLowerASCII() would be
constrained to a single class.

http://codereview.chromium.org/10923002/diff/11002/chrome/browser/extensions/...
chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute.cc:332:
new HeaderMatcher(&header_tests));
nit: single line

http://codereview.chromium.org/10923002/diff/11002/chrome/browser/extensions/...
chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute.cc:386:
return false;
I disagree with this change. This makes the compile-time check whether all enum
values are covered a runtime ceck.

http://codereview.chromium.org/10923002/diff/11002/chrome/browser/extensions/...
chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute.cc:491:
scoped_ptr<const HeaderMatcher>* header_matcher, bool positive)
nit: bool positive goes into next line (either all parameters in one line or one
per line)

http://codereview.chromium.org/10923002/diff/11002/chrome/browser/extensions/...
chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute.cc:526:
const bool positive = name == keys::kResponseHeadersKey;
nit: no need for const

[vabr (Chromium), 2012-09-03 10:12:36]

Dominic,

Thanks for your comments. Some addressed, some opposed :), please see my
answers.

Vaclav

http://codereview.chromium.org/10923002/diff/11002/chrome/browser/extensions/...
File
chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute.cc
(left):

http://codereview.chromium.org/10923002/diff/11002/chrome/browser/extensions/...
chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute.cc:419:
*error = ExtensionErrorUtils::FormatErrorMessage(kInvalidValue, *key);
On 2012/09/03 08:46:47, battre wrote:
> why did you remove this?

In all situations where creating a HeaderMatchTest (and the whole HeaderMatcher,
in fact) reported and error, kInvalidValue was reported. So I removed the
dependency of this create method on the |error| argument and report it in the
create method of the condition attribute instead. The output of the code will
not change by this.

http://codereview.chromium.org/10923002/diff/11002/chrome/browser/extensions/...
chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute.cc:446:
*error = ExtensionErrorUtils::FormatErrorMessage(kInvalidValue, *key);
On 2012/09/03 08:46:47, battre wrote:
> why did you remove this?

Please see above the explanation for HeaderMatchTest, it applies here as well.

http://codereview.chromium.org/10923002/diff/11002/chrome/browser/extensions/...
File
chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute.cc
(right):

http://codereview.chromium.org/10923002/diff/11002/chrome/browser/extensions/...
chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute.cc:266:
MatchType match_type);
On 2012/09/03 08:46:47, battre wrote:
> You can do this in a separate CL, but I think we can clean this up by removing
> is_name_test. That way StringMatchTest does not need to know about the concept
> of key/value paris of headers. Also the StringToLowerASCII() would be
> constrained to a single class.

I thought about this before, but decided against it. This way I can pass
StringValue |content| to Create, instead of std::string, and only write the
StringValue->std::string conversion code in Create once, instead of twice in its
callers.
If you think having StringToLowerASCII() constrained to a single class is of
greater benefit than non-repeating the code for StringValue->std::string
conversion, please let me know why, and I will consider that.

http://codereview.chromium.org/10923002/diff/11002/chrome/browser/extensions/...
chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute.cc:332:
new HeaderMatcher(&header_tests));
On 2012/09/03 08:46:47, battre wrote:
> nit: single line

Done.

http://codereview.chromium.org/10923002/diff/11002/chrome/browser/extensions/...
chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute.cc:386:
return false;
On 2012/09/03 08:46:47, battre wrote:
> I disagree with this change. This makes the compile-time check whether all
enum
> values are covered a runtime ceck.

You're right, reverted.
I guess I need to learn to be more assertive against review arguments involving
"usually" as the only reason. :)

http://codereview.chromium.org/10923002/diff/11002/chrome/browser/extensions/...
chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute.cc:491:
scoped_ptr<const HeaderMatcher>* header_matcher, bool positive)
On 2012/09/03 08:46:47, battre wrote:
> nit: bool positive goes into next line (either all parameters in one line or
one
> per line)

Done.

http://codereview.chromium.org/10923002/diff/11002/chrome/browser/extensions/...
chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute.cc:526:
const bool positive = name == keys::kResponseHeadersKey;
On 2012/09/03 08:46:47, battre wrote:
> nit: no need for const

I disagree. I want to document that |positive| is just an abbreviation for its
definition. I am not sure whether the compiler can optimise the code more based
on that, but the documentation purpose is good enough for me.
If you think there is a downside to declaring it const, please let me know.

[battre, 2012-09-03 12:48:08]

http://codereview.chromium.org/10923002/diff/11002/chrome/browser/extensions/...
File
chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute.cc
(left):

http://codereview.chromium.org/10923002/diff/11002/chrome/browser/extensions/...
chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute.cc:419:
*error = ExtensionErrorUtils::FormatErrorMessage(kInvalidValue, *key);
On 2012/09/03 10:12:36, vabr (Chromium) wrote:
> On 2012/09/03 08:46:47, battre wrote:
> > why did you remove this?
> 
> In all situations where creating a HeaderMatchTest (and the whole
HeaderMatcher,
> in fact) reported and error, kInvalidValue was reported. So I removed the
> dependency of this create method on the |error| argument and report it in the
> create method of the condition attribute instead. The output of the code will
> not change by this.

Thanks. Makes sense.

http://codereview.chromium.org/10923002/diff/11002/chrome/browser/extensions/...
File
chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute.cc
(right):

http://codereview.chromium.org/10923002/diff/11002/chrome/browser/extensions/...
chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute.cc:266:
MatchType match_type);
On 2012/09/03 10:12:36, vabr (Chromium) wrote:
> On 2012/09/03 08:46:47, battre wrote:
> > You can do this in a separate CL, but I think we can clean this up by
removing
> > is_name_test. That way StringMatchTest does not need to know about the
concept
> > of key/value paris of headers. Also the StringToLowerASCII() would be
> > constrained to a single class.
> 
> I thought about this before, but decided against it. This way I can pass
> StringValue |content| to Create, instead of std::string, and only write the
> StringValue->std::string conversion code in Create once, instead of twice in
its
> callers.
> If you think having StringToLowerASCII() constrained to a single class is of
> greater benefit than non-repeating the code for StringValue->std::string
> conversion, please let me know why, and I will consider that.

My thoughts were from the perspective of somebody who reads the code for the
first time:
- A StringMatchTest should match strings, not parts of headers.
- I am not very fond of the asymmetry of the StringToLowerASCII() calls. For
Matches() we expect that it is the responsibility of the caller to call it (but
only for headers) and for Create() this class takes care.

I think that moving StringToLowerASCII out of StringMatchTest would be cleaner.

http://codereview.chromium.org/10923002/diff/11002/chrome/browser/extensions/...
chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute.cc:526:
const bool positive = name == keys::kResponseHeadersKey;
On 2012/09/03 10:12:36, vabr (Chromium) wrote:
> On 2012/09/03 08:46:47, battre wrote:
> > nit: no need for const
> 
> I disagree. I want to document that |positive| is just an abbreviation for its
> definition. I am not sure whether the compiler can optimise the code more
based
> on that, but the documentation purpose is good enough for me.
> If you think there is a downside to declaring it const, please let me know.

I think the downside is the additional amount of code to read. It is pretty
uncommon in the code to use const temporary variables. I don't feel strongly
about this, though. Feel free to leave it.

[vabr (Chromium), 2012-09-03 14:21:29]

Thanks again, Dominic!

Vaclav

http://codereview.chromium.org/10923002/diff/11002/chrome/browser/extensions/...
File
chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute.cc
(right):

http://codereview.chromium.org/10923002/diff/11002/chrome/browser/extensions/...
chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute.cc:266:
MatchType match_type);
On 2012/09/03 12:48:08, battre wrote:
> On 2012/09/03 10:12:36, vabr (Chromium) wrote:
> > On 2012/09/03 08:46:47, battre wrote:
> > > You can do this in a separate CL, but I think we can clean this up by
> removing
> > > is_name_test. That way StringMatchTest does not need to know about the
> concept
> > > of key/value paris of headers. Also the StringToLowerASCII() would be
> > > constrained to a single class.
> > 
> > I thought about this before, but decided against it. This way I can pass
> > StringValue |content| to Create, instead of std::string, and only write the
> > StringValue->std::string conversion code in Create once, instead of twice in
> its
> > callers.
> > If you think having StringToLowerASCII() constrained to a single class is of
> > greater benefit than non-repeating the code for StringValue->std::string
> > conversion, please let me know why, and I will consider that.
> 
> My thoughts were from the perspective of somebody who reads the code for the
> first time:
> - A StringMatchTest should match strings, not parts of headers.
> - I am not very fond of the asymmetry of the StringToLowerASCII() calls. For
> Matches() we expect that it is the responsibility of the caller to call it
(but
> only for headers) and for Create() this class takes care.
> 
> I think that moving StringToLowerASCII out of StringMatchTest would be
cleaner.

I agree with both points. StringToLowerASCII is not used any more at all.
Surprisingly :), I moved the case-insensitive logic all to StringMatchTest,
which now has a construction flag to be case_sensitive or not. The class itself
knows nothing about headers vs. values, only case_sensitive or not. That flag is
set on construction in HeaderMatcher::HeaderMatchTest::Create, based on whether
that test is for header name or values.
I hope that this solution satisfies both the points you raised.

I also checked that the functions used inside
HeaderMatcher::StringMatchTest::Matches do not create copies of the strings they
compare -- the StartsWith/EndsWith use strcasecmp, and the std::search compares
byte per byte, so neither memory nor speed overhead should be too much.

[vabr (Chromium), 2012-09-03 15:24:14]

Thanks Dominic for one more error you found!
Corrected and enhanced the unit-tests to catch this.

PTAL,

Vaclav

[battre, 2012-09-04 09:55:21]

lgtm

[Yoyo Zhou, 2012-09-05 00:22:58]

LGTM, if that wasn't clear

[commit-bot: I haz the power, 2012-09-05 08:40:15]

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

[commit-bot: I haz the power, 2012-09-05 11:04:04]

Change committed as 154939