Issue 9844028: Implement rules removal for WebRequestRulesRegistry

Issue 9844028: Implement rules removal for WebRequestRulesRegistry (Closed)

Created:
8 years, 9 months ago by battre

Modified:
8 years, 9 months ago

Reviewers:
Matt Perry

CC:
chromium-reviews, Aaron Boodman, mihaip+watch_chromium.org

Base URL:
svn://svn.chromium.org/chrome/trunk/src

Visibility:
Public.

More Reviews

Description

Implement rules removal for WebRequestRulesRegistry This CL implements rules removal for the WebRequestRulesRegistry and also adds some testing functionality to ensure that we are not leaking any memory. BUG=112155 TEST=no Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=129587

Patch Set 1 #

Total comments: 4

Patch Set 2 : Addressed comments #

Created: 8 years, 9 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+177 lines, -7 lines)			Patch
M	chrome/browser/extensions/api/declarative/rules_registry_with_cache.cc	View	1	4 chunks	+4 lines, -4 lines	0 comments	Download
M	chrome/browser/extensions/api/declarative/substring_set_matcher.h	View	1	1 chunk	+3 lines, -0 lines	0 comments	Download
M	chrome/browser/extensions/api/declarative/substring_set_matcher.cc	View	1	1 chunk	+5 lines, -0 lines	0 comments	Download
M	chrome/browser/extensions/api/declarative/substring_set_matcher_unittest.cc	View	1	1 chunk	+6 lines, -0 lines	0 comments	Download
M	chrome/browser/extensions/api/declarative/url_matcher.h	View	1	2 chunks	+6 lines, -0 lines	0 comments	Download
M	chrome/browser/extensions/api/declarative/url_matcher.cc	View	1	2 chunks	+14 lines, -0 lines	0 comments	Download
M	chrome/browser/extensions/api/declarative/url_matcher_unittest.cc	View	1	2 chunks	+3 lines, -0 lines	0 comments	Download
M	chrome/browser/extensions/api/declarative_webrequest/webrequest_rule.cc	View	1	1 chunk	+1 line, -1 line	0 comments	Download
M	chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry.h	View	1	1 chunk	+3 lines, -0 lines	0 comments	Download
M	chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry.cc	View	1	1 chunk	+45 lines, -2 lines	0 comments	Download
M	chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry_unittest.cc	View	1	2 chunks	+87 lines, -0 lines	0 comments	Download

Messages

Total messages: 3 (0 generated)

Expand Messages | Collapse Messages

Matt Perry

lgtm https://chromiumcodereview.appspot.com/9844028/diff/1/chrome/browser/extensions/api/declarative/substring_set_matcher.h File chrome/browser/extensions/api/declarative/substring_set_matcher.h (right): https://chromiumcodereview.appspot.com/9844028/diff/1/chrome/browser/extensions/api/declarative/substring_set_matcher.h#newcode72 chrome/browser/extensions/api/declarative/substring_set_matcher.h:72: bool IsCompletelyEmpty() const; nit: IsEmpty should be fine... ...

8 years, 9 months ago (2012-03-28 18:59:52 UTC) #2

battre

8 years, 9 months ago (2012-03-28 19:18:42 UTC) #3

Thanks for the quick review.

Best regards,
Dominic

https://chromiumcodereview.appspot.com/9844028/diff/1/chrome/browser/extensio...
File chrome/browser/extensions/api/declarative/substring_set_matcher.h (right):

https://chromiumcodereview.appspot.com/9844028/diff/1/chrome/browser/extensio...
chrome/browser/extensions/api/declarative/substring_set_matcher.h:72: bool
IsCompletelyEmpty() const;
On 2012/03/28 18:59:52, Matt Perry wrote:
> nit: IsEmpty should be fine... I wouldn't expect it to return true if it's
only
> half empty :)

Done.

https://chromiumcodereview.appspot.com/9844028/diff/1/chrome/browser/extensio...
File
chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry.cc
(right):

https://chromiumcodereview.appspot.com/9844028/diff/1/chrome/browser/extensio...
chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry.cc:103:
WebRequestRule::GlobalRuleId rule_id = make_pair(extension_id, *i);
On 2012/03/28 18:59:52, Matt Perry wrote:
> you can just use the constructor: GlobalRuleId rule_id(extension_id, *i)

Nice, I did not know that. Thanks.

Done. (also in some other locations)

Expand Messages | Collapse Messages