Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10874029:
Adding condition attributes for response headers to Declarative WebRequest (Closed)
|
Created:
8 years, 4 months ago by vabr (Chromium) Modified:
8 years, 2 months ago Reviewers:
battre CC:
chromium-reviews, Aaron Boodman, mihaip-chromium-reviews_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdding condition attributes for response headers to Declarative WebRequest
BUG=143662, 112155
TEST=Install the extension "declarative_headers_match.crx" attached to BUG 143662. If this feature is working correctly it should cancel every navigation.
TBR=kathyw@chromium.org
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=153720
Patch Set 1 #Patch Set 2 : Added the con/dcon for TestGroup requested by Clang #Patch Set 3 : Some typos corrected #
Total comments: 64
Patch Set 4 : Dominic's comments and other clean-ups #Patch Set 5 : Minor cleanup #
Total comments: 27
Patch Set 6 : Dominic's comments (except MatchTest constructor) #Patch Set 7 : MatchTest constructor also changed to address Dominic's comment #
Total comments: 12
Patch Set 8 : #Messages
Total messages: 10 (0 generated)
Hi Dominic, Please take a look. Cheers, Vaclav
http://codereview.chromium.org/10874029/diff/7001/chrome/browser/extensions/a... File chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute.cc (right): http://codereview.chromium.org/10874029/diff/7001/chrome/browser/extensions/a... chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute.cc:299: } nit: no {}, also in 291 http://codereview.chromium.org/10874029/diff/7001/chrome/browser/extensions/a... chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute.cc:320: headers = request_data.original_response_headers; "headers =" goes into previous line. http://codereview.chromium.org/10874029/diff/7001/chrome/browser/extensions/a... chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute.cc:350: for (size_t i = 0; !success && i < tests_.size(); ++i) { How about moving this loop to become the outer loop and then introduce a function tests_[i].Matches() ? http://codereview.chromium.org/10874029/diff/7001/chrome/browser/extensions/a... chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute.cc:388: case kPrefix: { You can use StartsWithASCII and EndsWith for these tests, see string_util.h http://codereview.chromium.org/10874029/diff/7001/chrome/browser/extensions/a... chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute.cc:412: const DictionaryValue* tests, std::string* error) { How about a functional style of programming? I.e. you would return a TestGroup() instead of appending it? Same for PushOneMatchTest(). http://codereview.chromium.org/10874029/diff/7001/chrome/browser/extensions/a... chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute.cc:445: const Value* content; nit: = NULL http://codereview.chromium.org/10874029/diff/7001/chrome/browser/extensions/a... chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute.cc:446: tests->Get(*key, &content); // We already checked that |key| is there. CHECK? http://codereview.chromium.org/10874029/diff/7001/chrome/browser/extensions/a... File chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute.h (right): http://codereview.chromium.org/10874029/diff/7001/chrome/browser/extensions/a... chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute.h:36: CONDITION_CONTAINS_HEADERS We need support for request and response headers. You can handle these in different CLs, but I think the class/enum names should reflect the type. http://codereview.chromium.org/10874029/diff/7001/chrome/browser/extensions/a... chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute.h:142: class WebRequestConditionAttributeContainsHeaders I would suggest to rename this to WebRequestConditionAttributeResponseHeaders http://codereview.chromium.org/10874029/diff/7001/chrome/browser/extensions/a... chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute.h:162: enum MatchType { kPrefix, kSuffix, kEqual, kContains }; kEquals to match the parameter of the attribute http://codereview.chromium.org/10874029/diff/7001/chrome/browser/extensions/a... chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute.h:167: return data_; I'd prefer if you moved this to the constructor, even if this means one more copy operation. That way, the MatchTest becomes a pure immutable object. http://codereview.chromium.org/10874029/diff/7001/chrome/browser/extensions/a... chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute.h:170: bool Matches(const std::string& str); nit: const http://codereview.chromium.org/10874029/diff/7001/chrome/browser/extensions/a... chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute.h:173: MatchType type_; nit: DISALLOW_COPY_AND_ASSIGN due to non-trivial datatype of |data_| http://codereview.chromium.org/10874029/diff/7001/chrome/browser/extensions/a... chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute.h:179: std::vector<MatchTest> name; This should become a ScopedVector if you add the DISALLOW_COPY_AND_ASSIGNs. http://codereview.chromium.org/10874029/diff/7001/chrome/browser/extensions/a... chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute.h:181: std::vector<MatchTest> value; nit: DISALLOW_COPY_AND_ASSIGN due to non-trivial datatype of |name| and |value| http://codereview.chromium.org/10874029/diff/7001/chrome/browser/extensions/a... chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute.h:190: bool PushMatchTests(const base::DictionaryValue* tests, std::string* error); How about s/Push/Append/ to match the ListValue notation. I think that the whole "push" notation of the STL is very strange. http://codereview.chromium.org/10874029/diff/7001/chrome/browser/extensions/a... chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute.h:197: std::vector< TestGroup > tests_; nit: no spaces http://codereview.chromium.org/10874029/diff/7001/chrome/browser/extensions/a... chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute.h:197: std::vector< TestGroup > tests_; This should become a ScopedVector if you add the DISALLOW_COPY_AND_ASSIGNs. http://codereview.chromium.org/10874029/diff/7001/chrome/browser/extensions/a... File chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute_unittest.cc (right): http://codereview.chromium.org/10874029/diff/7001/chrome/browser/extensions/a... chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute_unittest.cc:195: if (length % 2 == 1) I think you can just CHECK() this. http://codereview.chromium.org/10874029/diff/7001/chrome/browser/extensions/a... chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute_unittest.cc:217: if (!entry->GetAsList(&list)) CHECK? http://codereview.chromium.org/10874029/diff/7001/chrome/browser/extensions/a... chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute_unittest.cc:239: const bool positive_test, nit: we usually don't pass atomic types as const parameters. http://codereview.chromium.org/10874029/diff/7001/chrome/browser/extensions/a... chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute_unittest.cc:240: bool* result) { return the result instead? http://codereview.chromium.org/10874029/diff/7001/chrome/browser/extensions/a... chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute_unittest.cc:244: ASSERT_TRUE(temp.get() != NULL); is the != NULL necessary? http://codereview.chromium.org/10874029/diff/7001/chrome/browser/extensions/a... chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute_unittest.cc:309: keys::kValueEqualsKey, "custom" // Test 4. how about a test for case sensitivity in the value? http://codereview.chromium.org/10874029/diff/7001/chrome/browser/extensions/a... chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute_unittest.cc:329: keys::kValueEqualsKey, "valueA" Please see my comment in headers.html.mock-http-headers http://codereview.chromium.org/10874029/diff/7001/chrome/common/extensions/ap... File chrome/common/extensions/api/declarative_web_request.json (right): http://codereview.chromium.org/10874029/diff/7001/chrome/common/extensions/ap... chrome/common/extensions/api/declarative_web_request.json:8: "description_permissions_required": ["declarative", "declarativeWebRequest"], I think this change is wrong. http://codereview.chromium.org/10874029/diff/7001/chrome/common/extensions/ap... chrome/common/extensions/api/declarative_web_request.json:16: "description" : "Matches if the header name starts with a specified string.", This does not cover the array. http://codereview.chromium.org/10874029/diff/7001/chrome/common/extensions/ap... chrome/common/extensions/api/declarative_web_request.json:109: "containsHeaders": { I would rename this to "responseHeaders" http://codereview.chromium.org/10874029/diff/7001/chrome/common/extensions/ap... chrome/common/extensions/api/declarative_web_request.json:112: "description": "Matches if some of the headers is matched by one of the HeaderFilters.", response headers http://codereview.chromium.org/10874029/diff/7001/chrome/common/extensions/ap... chrome/common/extensions/api/declarative_web_request.json:115: "doesNotContainHeaders": { I would rename this to "excludeResponseHeaders" http://codereview.chromium.org/10874029/diff/7001/chrome/common/extensions/ap... chrome/common/extensions/api/declarative_web_request.json:118: "description": "Matches if none of the headers is matched by one of the HeaderFilters.", response headers http://codereview.chromium.org/10874029/diff/7001/chrome/test/data/extensions... File chrome/test/data/extensions/api_test/webrequest/declarative/headers.html.mock-http-headers (right): http://codereview.chromium.org/10874029/diff/7001/chrome/test/data/extensions... chrome/test/data/extensions/api_test/webrequest/declarative/headers.html.mock-http-headers:5: Custom-Header-B: valueB Can you add Custom-Header-C: valueC, valueD and test that this is interpreted as a single header? See http://code.google.com/p/chromium/issues/detail?id=137396
Hi Dominic, Thanks for your comments, I found them very enlightening! :) Please take another look. Cheers, Vaclav http://codereview.chromium.org/10874029/diff/7001/chrome/browser/extensions/a... File chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute.cc (right): http://codereview.chromium.org/10874029/diff/7001/chrome/browser/extensions/a... chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute.cc:299: } On 2012/08/23 12:32:11, battre wrote: > nit: no {}, also in 291 Done, and subsequently deleted altogether. Case kDictionary never happens, I just misunderstood my own JSON scheme. http://codereview.chromium.org/10874029/diff/7001/chrome/browser/extensions/a... chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute.cc:320: headers = request_data.original_response_headers; On 2012/08/23 12:32:11, battre wrote: > "headers =" goes into previous line. Done. http://codereview.chromium.org/10874029/diff/7001/chrome/browser/extensions/a... chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute.cc:350: for (size_t i = 0; !success && i < tests_.size(); ++i) { On 2012/08/23 12:32:11, battre wrote: > How about moving this loop to become the outer loop and then introduce a > function tests_[i].Matches() ? Done. http://codereview.chromium.org/10874029/diff/7001/chrome/browser/extensions/a... chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute.cc:388: case kPrefix: { On 2012/08/23 12:32:11, battre wrote: > You can use StartsWithASCII and EndsWith for these tests, see string_util.h Done. Thanks for making me aware of those functions! http://codereview.chromium.org/10874029/diff/7001/chrome/browser/extensions/a... chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute.cc:412: const DictionaryValue* tests, std::string* error) { On 2012/08/23 12:32:11, battre wrote: > How about a functional style of programming? I.e. you would return a TestGroup() > instead of appending it? Same for PushOneMatchTest(). Done. http://codereview.chromium.org/10874029/diff/7001/chrome/browser/extensions/a... chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute.cc:445: const Value* content; On 2012/08/23 12:32:11, battre wrote: > nit: = NULL Done. http://codereview.chromium.org/10874029/diff/7001/chrome/browser/extensions/a... chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute.cc:446: tests->Get(*key, &content); // We already checked that |key| is there. On 2012/08/23 12:32:11, battre wrote: > CHECK? Done. http://codereview.chromium.org/10874029/diff/7001/chrome/browser/extensions/a... File chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute.h (right): http://codereview.chromium.org/10874029/diff/7001/chrome/browser/extensions/a... chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute.h:36: CONDITION_CONTAINS_HEADERS On 2012/08/23 12:32:11, battre wrote: > We need support for request and response headers. You can handle these in > different CLs, but I think the class/enum names should reflect the type. Done. I don't think a condition attribute for request headers will be different from this, except for how the headers are obtained. Anyway, I'll leave request headers for a second CL, and in that CL I will probably put the common functionality of the header-related condition attributes in a headers-named base class, and derive the request- and response-instances out of it. http://codereview.chromium.org/10874029/diff/7001/chrome/browser/extensions/a... chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute.h:142: class WebRequestConditionAttributeContainsHeaders On 2012/08/23 12:32:11, battre wrote: > I would suggest to rename this to WebRequestConditionAttributeResponseHeaders Done. http://codereview.chromium.org/10874029/diff/7001/chrome/browser/extensions/a... chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute.h:162: enum MatchType { kPrefix, kSuffix, kEqual, kContains }; On 2012/08/23 12:32:11, battre wrote: > kEquals to match the parameter of the attribute Done. http://codereview.chromium.org/10874029/diff/7001/chrome/browser/extensions/a... chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute.h:167: return data_; On 2012/08/23 12:32:11, battre wrote: > I'd prefer if you moved this to the constructor, even if this means one more > copy operation. That way, the MatchTest becomes a pure immutable object. Done. And retaining the number of copy operations by using a pointer to a string. http://codereview.chromium.org/10874029/diff/7001/chrome/browser/extensions/a... chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute.h:170: bool Matches(const std::string& str); On 2012/08/23 12:32:11, battre wrote: > nit: const Done. http://codereview.chromium.org/10874029/diff/7001/chrome/browser/extensions/a... chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute.h:173: MatchType type_; On 2012/08/23 12:32:11, battre wrote: > nit: DISALLOW_COPY_AND_ASSIGN due to non-trivial datatype of |data_| Done. http://codereview.chromium.org/10874029/diff/7001/chrome/browser/extensions/a... chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute.h:179: std::vector<MatchTest> name; On 2012/08/23 12:32:11, battre wrote: > This should become a ScopedVector if you add the DISALLOW_COPY_AND_ASSIGNs. Done. http://codereview.chromium.org/10874029/diff/7001/chrome/browser/extensions/a... chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute.h:181: std::vector<MatchTest> value; On 2012/08/23 12:32:11, battre wrote: > nit: DISALLOW_COPY_AND_ASSIGN due to non-trivial datatype of |name| and |value| Done. http://codereview.chromium.org/10874029/diff/7001/chrome/browser/extensions/a... chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute.h:190: bool PushMatchTests(const base::DictionaryValue* tests, std::string* error); On 2012/08/23 12:32:11, battre wrote: > How about s/Push/Append/ to match the ListValue notation. I think that the whole > "push" notation of the STL is very strange. I ended up with GetTests and GetMatchTest, because no pushing/appending is done there any more. http://codereview.chromium.org/10874029/diff/7001/chrome/browser/extensions/a... chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute.h:197: std::vector< TestGroup > tests_; On 2012/08/23 12:32:11, battre wrote: > nit: no spaces Done. http://codereview.chromium.org/10874029/diff/7001/chrome/browser/extensions/a... chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute.h:197: std::vector< TestGroup > tests_; On 2012/08/23 12:32:11, battre wrote: > This should become a ScopedVector if you add the DISALLOW_COPY_AND_ASSIGNs. Done. http://codereview.chromium.org/10874029/diff/7001/chrome/browser/extensions/a... File chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute_unittest.cc (right): http://codereview.chromium.org/10874029/diff/7001/chrome/browser/extensions/a... chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute_unittest.cc:195: if (length % 2 == 1) On 2012/08/23 12:32:11, battre wrote: > I think you can just CHECK() this. Done. http://codereview.chromium.org/10874029/diff/7001/chrome/browser/extensions/a... chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute_unittest.cc:217: if (!entry->GetAsList(&list)) On 2012/08/23 12:32:11, battre wrote: > CHECK? Done. http://codereview.chromium.org/10874029/diff/7001/chrome/browser/extensions/a... chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute_unittest.cc:239: const bool positive_test, On 2012/08/23 12:32:11, battre wrote: > nit: we usually don't pass atomic types as const parameters. Actually I got rid of the intermediate boolean an replaced it with the key (which it chose originally) directly. http://codereview.chromium.org/10874029/diff/7001/chrome/browser/extensions/a... chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute_unittest.cc:240: bool* result) { On 2012/08/23 12:32:11, battre wrote: > return the result instead? Can't do that, using ASSERT_* requires void return type. http://codereview.chromium.org/10874029/diff/7001/chrome/browser/extensions/a... chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute_unittest.cc:244: ASSERT_TRUE(temp.get() != NULL); On 2012/08/23 12:32:11, battre wrote: > is the != NULL necessary? It is not necessary :). I tend to write it because I find it more readable. I guess this is a matter of personal opinion, as neither C++ nor the style guide dictates how to use the implicit conversion pointer->boolean. You are the owner of this code, so I'm happy to strip "!= NULL" from this and other occurrences if you insist. http://codereview.chromium.org/10874029/diff/7001/chrome/browser/extensions/a... chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute_unittest.cc:309: keys::kValueEqualsKey, "custom" // Test 4. On 2012/08/23 12:32:11, battre wrote: > how about a test for case sensitivity in the value? Good idea! Done as 1.h. http://codereview.chromium.org/10874029/diff/7001/chrome/browser/extensions/a... chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute_unittest.cc:329: keys::kValueEqualsKey, "valueA" On 2012/08/23 12:32:11, battre wrote: > Please see my comment in headers.html.mock-http-headers Done. http://codereview.chromium.org/10874029/diff/7001/chrome/common/extensions/ap... File chrome/common/extensions/api/declarative_web_request.json (right): http://codereview.chromium.org/10874029/diff/7001/chrome/common/extensions/ap... chrome/common/extensions/api/declarative_web_request.json:8: "description_permissions_required": ["declarative", "declarativeWebRequest"], On 2012/08/23 12:32:11, battre wrote: > I think this change is wrong. Sorry, that was unintentional. Wrongly written substitute regexp gone wild. Reverted. http://codereview.chromium.org/10874029/diff/7001/chrome/common/extensions/ap... chrome/common/extensions/api/declarative_web_request.json:16: "description" : "Matches if the header name starts with a specified string.", On 2012/08/23 12:32:11, battre wrote: > This does not cover the array. I amended the comments, and also removed the choice of array at everything but *Contains, because elsewhere it makes little sense (e.g., prefix: "abc" && prefix: "abcde" is just prefix: "abcde"). http://codereview.chromium.org/10874029/diff/7001/chrome/common/extensions/ap... chrome/common/extensions/api/declarative_web_request.json:109: "containsHeaders": { On 2012/08/23 12:32:11, battre wrote: > I would rename this to "responseHeaders" Done. http://codereview.chromium.org/10874029/diff/7001/chrome/common/extensions/ap... chrome/common/extensions/api/declarative_web_request.json:112: "description": "Matches if some of the headers is matched by one of the HeaderFilters.", On 2012/08/23 12:32:11, battre wrote: > response headers Done. http://codereview.chromium.org/10874029/diff/7001/chrome/common/extensions/ap... chrome/common/extensions/api/declarative_web_request.json:115: "doesNotContainHeaders": { On 2012/08/23 12:32:11, battre wrote: > I would rename this to "excludeResponseHeaders" Done. http://codereview.chromium.org/10874029/diff/7001/chrome/common/extensions/ap... chrome/common/extensions/api/declarative_web_request.json:118: "description": "Matches if none of the headers is matched by one of the HeaderFilters.", On 2012/08/23 12:32:11, battre wrote: > response headers Done. http://codereview.chromium.org/10874029/diff/7001/chrome/test/data/extensions... File chrome/test/data/extensions/api_test/webrequest/declarative/headers.html.mock-http-headers (right): http://codereview.chromium.org/10874029/diff/7001/chrome/test/data/extensions... chrome/test/data/extensions/api_test/webrequest/declarative/headers.html.mock-http-headers:5: Custom-Header-B: valueB On 2012/08/23 12:32:11, battre wrote: > Can you add > Custom-Header-C: valueC, valueD > and test that this is interpreted as a single header? See > http://code.google.com/p/chromium/issues/detail?id=137396 Done.
One more note I forgot to add. http://codereview.chromium.org/10874029/diff/4002/chrome/browser/extensions/a... File chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute.cc (right): http://codereview.chromium.org/10874029/diff/4002/chrome/browser/extensions/a... chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute.cc:339: header_found |= tests_[i]->Matches(name, value); If one header occurs on multiple lines, then we test the header name as many times against the whole test group, as many lines that header occupies. I am not sure how much complexity this adds and how would it compare to caching the test results for header names. If you feel it would be better to cache the test results for headers, just please let me know, and I'll add it.
almost there, thx http://codereview.chromium.org/10874029/diff/4002/chrome/browser/extensions/a... File chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute.cc (right): http://codereview.chromium.org/10874029/diff/4002/chrome/browser/extensions/a... chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute.cc:294: scoped_ptr<const TestGroup> group(GetTests(tests, error).Pass()); Is this Pass necessary? http://codereview.chromium.org/10874029/diff/4002/chrome/browser/extensions/a... chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute.cc:303: result->tests_.swap(groups); nit: Why don't you pass a pointer to groups to the constructor and let result do the swapping? by calling ->Pass() http://codereview.chromium.org/10874029/diff/4002/chrome/browser/extensions/a... chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute.cc:333: bool header_found = false; move this to line 327 and remove success? http://codereview.chromium.org/10874029/diff/4002/chrome/browser/extensions/a... chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute.cc:357: return StartsWithASCII(str, data(), true /*case_sensitive*/); you can replace data() with data_ and remove data() entirely. http://codereview.chromium.org/10874029/diff/4002/chrome/browser/extensions/a... chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute.cc:375: for (size_t j = 0; header_success && j < name_.size(); ++j) { nit: use i as iterator? also in line 383 http://codereview.chromium.org/10874029/diff/4002/chrome/browser/extensions/a... chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute.cc:388: return value_success; you could make this function slightly shorter: for (size_t i = 0; i < name_.size(); ++j) { if (!name_[i]->Matches(name)) return false; } for (size_t i = 0; i < value_.size(); ++j) { if (!value_[i]->Matches(name)) return false; } return false; http://codereview.chromium.org/10874029/diff/4002/chrome/browser/extensions/a... chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute.cc:458: return scoped_ptr<const TestGroup>(new TestGroup(&name, &value)); FYI: we can now use make_scoped_ptr as well, i.e. return make_scoped_ptr(new TestGroup(&name, &value); http://codereview.chromium.org/10874029/diff/4002/chrome/browser/extensions/a... File chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute.h (right): http://codereview.chromium.org/10874029/diff/4002/chrome/browser/extensions/a... chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute.h:168: MatchTest(scoped_ptr<const std::string> data, MatchType type); Please pass |data| just as a const ref. The cost to copy the string once is not that big. http://codereview.chromium.org/10874029/diff/4002/chrome/browser/extensions/a... chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute.h:183: TestGroup(ScopedVector<const MatchTest>* name, Please add comment, that ownership is taken over. http://codereview.chromium.org/10874029/diff/4002/chrome/browser/extensions/a... chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute.h:200: static scoped_ptr<const TestGroup> GetTests( nit: s/Get/Create/? http://codereview.chromium.org/10874029/diff/4002/chrome/browser/extensions/a... chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute.h:206: MatchType match_type); nit: s/Get/Create/ (also in line 203) http://codereview.chromium.org/10874029/diff/4002/chrome/browser/extensions/a... File chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute_unittest.cc (right): http://codereview.chromium.org/10874029/diff/4002/chrome/browser/extensions/a... chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute_unittest.cc:230: // Returns how the response headers from |url_request| satisfy the match s/how/whether/? http://codereview.chromium.org/10874029/diff/4002/chrome/browser/extensions/a... chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute_unittest.cc:237: const std::string& key, I would change the order: key + tests make up the condition. url_request is the thing being tested.
Hi Dominic, I addressed your further comments. Please take another look. Once you are happy with that, I will ask OWNERS for approval. Thanks! Vaclav http://codereview.chromium.org/10874029/diff/4002/chrome/browser/extensions/a... File chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute.cc (right): http://codereview.chromium.org/10874029/diff/4002/chrome/browser/extensions/a... chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute.cc:294: scoped_ptr<const TestGroup> group(GetTests(tests, error).Pass()); On 2012/08/24 16:24:41, battre wrote: > Is this Pass necessary? I don't understand why, but it is not (at least with my GCC). :) I removed it. http://codereview.chromium.org/10874029/diff/4002/chrome/browser/extensions/a... chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute.cc:303: result->tests_.swap(groups); On 2012/08/24 16:24:41, battre wrote: > nit: Why don't you pass a pointer to groups to the constructor and let result do > the swapping? by calling ->Pass() Done. http://codereview.chromium.org/10874029/diff/4002/chrome/browser/extensions/a... chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute.cc:333: bool header_found = false; On 2012/08/24 16:24:41, battre wrote: > move this to line 327 and remove success? Done. http://codereview.chromium.org/10874029/diff/4002/chrome/browser/extensions/a... chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute.cc:357: return StartsWithASCII(str, data(), true /*case_sensitive*/); On 2012/08/24 16:24:41, battre wrote: > you can replace data() with data_ and remove data() entirely. Done. http://codereview.chromium.org/10874029/diff/4002/chrome/browser/extensions/a... chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute.cc:375: for (size_t j = 0; header_success && j < name_.size(); ++j) { On 2012/08/24 16:24:41, battre wrote: > nit: use i as iterator? also in line 383 Done. http://codereview.chromium.org/10874029/diff/4002/chrome/browser/extensions/a... chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute.cc:388: return value_success; On 2012/08/24 16:24:41, battre wrote: > you could make this function slightly shorter: > for (size_t i = 0; i < name_.size(); ++j) { > if (!name_[i]->Matches(name)) > return false; > } > for (size_t i = 0; i < value_.size(); ++j) { > if (!value_[i]->Matches(name)) > return false; > } > return false; Done (except for making the last return true :)). http://codereview.chromium.org/10874029/diff/4002/chrome/browser/extensions/a... chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute.cc:458: return scoped_ptr<const TestGroup>(new TestGroup(&name, &value)); On 2012/08/24 16:24:41, battre wrote: > FYI: we can now use make_scoped_ptr as well, i.e. > return make_scoped_ptr(new TestGroup(&name, &value); Thanks, I did not know that. As you only wrote "FYI" and not "change this" I left it as it is, because in this case it seems to improve readability -- it is clear what type we return without seeking one page up and reading the ugly "const WebRequestConditionAttributeResponseHeaders::TestGroup". If you let me know that you want me to change that, I will do that, though. http://codereview.chromium.org/10874029/diff/4002/chrome/browser/extensions/a... File chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute.h (right): http://codereview.chromium.org/10874029/diff/4002/chrome/browser/extensions/a... chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute.h:168: MatchTest(scoped_ptr<const std::string> data, MatchType type); On 2012/08/24 16:24:41, battre wrote: > Please pass |data| just as a const ref. The cost to copy the string once is not > that big. Done. To document this decision: pros of the scoped_ptr: * saving one copy of the string defining the test cons: * complicating the code with a separate memory management for that string * non-standard way of doing this Because this only happens once per rule registration (not rule usage), the pros are not significant enough to counter the cons. http://codereview.chromium.org/10874029/diff/4002/chrome/browser/extensions/a... chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute.h:183: TestGroup(ScopedVector<const MatchTest>* name, On 2012/08/24 16:24:41, battre wrote: > Please add comment, that ownership is taken over. Done. http://codereview.chromium.org/10874029/diff/4002/chrome/browser/extensions/a... chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute.h:200: static scoped_ptr<const TestGroup> GetTests( On 2012/08/24 16:24:41, battre wrote: > nit: s/Get/Create/? Done. http://codereview.chromium.org/10874029/diff/4002/chrome/browser/extensions/a... chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute.h:206: MatchType match_type); On 2012/08/24 16:24:41, battre wrote: > nit: s/Get/Create/ (also in line 203) Done. http://codereview.chromium.org/10874029/diff/4002/chrome/browser/extensions/a... File chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute_unittest.cc (right): http://codereview.chromium.org/10874029/diff/4002/chrome/browser/extensions/a... chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute_unittest.cc:230: // Returns how the response headers from |url_request| satisfy the match On 2012/08/24 16:24:41, battre wrote: > s/how/whether/? Done. http://codereview.chromium.org/10874029/diff/4002/chrome/browser/extensions/a... chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute_unittest.cc:237: const std::string& key, On 2012/08/24 16:24:41, battre wrote: > I would change the order: key + tests make up the condition. url_request is the > thing being tested. Done.
LGTM with nits http://codereview.chromium.org/10874029/diff/7005/chrome/browser/extensions/a... File chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute.cc (right): http://codereview.chromium.org/10874029/diff/7005/chrome/browser/extensions/a... chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute.cc:333: /* No increment here, this is done inside EnumerateHeaderLines. */) { opt: void* iter = NULL; while (!header_found && headers->EnumerateHeadersLines(&iter, &name, &value)) { ... (removes the need for the comment) http://codereview.chromium.org/10874029/diff/7005/chrome/browser/extensions/a... chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute.cc:462: new MatchTest(str, match_type)); nit: single line http://codereview.chromium.org/10874029/diff/7005/chrome/browser/extensions/a... File chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute.h (right): http://codereview.chromium.org/10874029/diff/7005/chrome/browser/extensions/a... chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute.h:165: class MatchTest { opt: StringMatchTest? http://codereview.chromium.org/10874029/diff/7005/chrome/browser/extensions/a... chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute.h:171: bool Matches(const std::string& str) const; nit: newline before private: (also below) http://codereview.chromium.org/10874029/diff/7005/chrome/browser/extensions/a... chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute.h:178: class TestGroup { opt: HeaderMatchTest? http://codereview.chromium.org/10874029/diff/7005/chrome/browser/extensions/a... chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute.h:180: // Takes ownership of both |name| and |value|. actually this should be "Takes ownership of the content of both |name| and |value|."
Thanks, Dominic. I put kathyw@ to the TBR, because the two files in chrome/common/extensions/ need her OWNERS review, but those changes are very simple and straightforward (one-sentence description of parameters in an API). Sending to the CQ now. Vaclav http://codereview.chromium.org/10874029/diff/7005/chrome/browser/extensions/a... File chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute.cc (right): http://codereview.chromium.org/10874029/diff/7005/chrome/browser/extensions/a... chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute.cc:333: /* No increment here, this is done inside EnumerateHeaderLines. */) { On 2012/08/27 17:16:25, battre wrote: > opt: > void* iter = NULL; > while (!header_found && headers->EnumerateHeadersLines(&iter, &name, &value)) { > ... > (removes the need for the comment) Done. http://codereview.chromium.org/10874029/diff/7005/chrome/browser/extensions/a... chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute.cc:462: new MatchTest(str, match_type)); On 2012/08/27 17:16:25, battre wrote: > nit: single line Does not fit any more after renaming MatchTest to StringMatchTest. http://codereview.chromium.org/10874029/diff/7005/chrome/browser/extensions/a... File chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute.h (right): http://codereview.chromium.org/10874029/diff/7005/chrome/browser/extensions/a... chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute.h:165: class MatchTest { On 2012/08/27 17:16:25, battre wrote: > opt: StringMatchTest? Done. http://codereview.chromium.org/10874029/diff/7005/chrome/browser/extensions/a... chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute.h:171: bool Matches(const std::string& str) const; On 2012/08/27 17:16:25, battre wrote: > nit: newline before private: (also below) Done. http://codereview.chromium.org/10874029/diff/7005/chrome/browser/extensions/a... chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute.h:178: class TestGroup { On 2012/08/27 17:16:25, battre wrote: > opt: HeaderMatchTest? Done. http://codereview.chromium.org/10874029/diff/7005/chrome/browser/extensions/a... chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute.h:180: // Takes ownership of both |name| and |value|. On 2012/08/27 17:16:25, battre wrote: > actually this should be "Takes ownership of the content of both |name| and > |value|." Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vabr@chromium.org/10874029/15001
Change committed as 153720 |
