Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10692158:
Refactor URLBlacklistManager to use URLMatcher. (Closed)
|
Created:
8 years, 5 months ago by Bernhard Bauer Modified:
8 years, 1 month ago CC:
chromium-reviews, Aaron Boodman, cbentzel+watch_chromium.org, mihaip-chromium-reviews_chromium.org, darin-cc_chromium.org, battre Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionRefactor URLBlacklistManager to use URLMatcher.
BUG=134417
TEST=a bunch of unit tests
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=147005
Patch Set 1 #Patch Set 2 : fix #Patch Set 3 : compile fix? #Patch Set 4 : compile fix? #Patch Set 5 : fix #Patch Set 6 : fix #Patch Set 7 : . #Patch Set 8 : . #Patch Set 9 : . #Patch Set 10 : . #
Total comments: 40
Patch Set 11 : joao & dominic #Patch Set 12 : fix #Patch Set 13 : review #
Total comments: 2
Patch Set 14 : joao #
Messages
Total messages: 17 (0 generated)
Hello Joao and Mattias, could you take a look at this CL? It refactors policy::URLBlacklist to use URLMatcher and extracts the core into methods which are used by ManagedModeURLFilter. I tried sharing as much code as possible, but policy black-/whitelisting has slightly different semantics from Managed Mode whitelisting: for example, a more specific "block" filter can override a less specific "allow" one, which requires us to keep the filters around to find the most specific one. We don't need that for Managed Mode. I might split this CL up into smaller part, but I wanted to get the whole thing out first so you can give comments. Thanks!
Nice refactoring of the URLBlacklist. I'm mostly cool with all of the changes; can you split at least the url_blacklist_manager changes to another CL? I'm still making sure the url_matcher.cc changes make sense, but since the bots are happy they probably do :-) http://codereview.chromium.org/10692158/diff/12021/chrome/browser/managed_mod... File chrome/browser/managed_mode.cc (right): http://codereview.chromium.org/10692158/diff/12021/chrome/browser/managed_mod... chrome/browser/managed_mode.cc:21: #include "content/public/browser/browser_thread.h" Nit: order http://codereview.chromium.org/10692158/diff/12021/chrome/browser/managed_mod... chrome/browser/managed_mode.cc:41: // |url_filter_| unretained. What do you think of this pattern instead: - ManagedMode owns ManagedModeURLFilter. It is created from UI but lives on IO. - Every call to the filter is posted Unretained() to IO - The destructor of ManagedMode() posts DeleteSoon() to IO to delete the filter. This makes sure any task previously posted is executed before deletion. - The getter can still DCHECK it's on IO and get the filter, since as long as the ManagedMode instance is alive then so is the filter on IO. http://codereview.chromium.org/10692158/diff/12021/chrome/browser/managed_mod... chrome/browser/managed_mode.cc:50: void SetWhitelist(scoped_ptr<base::ListValue> whitelist) { Is this used? http://codereview.chromium.org/10692158/diff/12021/chrome/browser/managed_mod... chrome/browser/managed_mode.cc:319: url_filter_context_->SetActive(in_managed_mode); Is this guaranteed to be non-NULL at this stage? http://codereview.chromium.org/10692158/diff/12021/chrome/browser/managed_mod... File chrome/browser/managed_mode_url_filter.cc (right): http://codereview.chromium.org/10692158/diff/12021/chrome/browser/managed_mod... chrome/browser/managed_mode_url_filter.cc:17: using extensions::URLMatcherConditionFactory; Are these 2 used? http://codereview.chromium.org/10692158/diff/12021/chrome/browser/managed_mod... chrome/browser/managed_mode_url_filter.cc:20: using extensions::URLMatcherSchemeFilter; Are these 2 used? http://codereview.chromium.org/10692158/diff/12021/chrome/browser/managed_mod... chrome/browser/managed_mode_url_filter.cc:68: scoped_ptr<URLMatcher> url_matcher(new URLMatcher()); DCHECK(BrowserThread::GetBlockingPool()->RunsTasksOnCurrentThread()); http://codereview.chromium.org/10692158/diff/12021/chrome/browser/managed_mod... File chrome/browser/managed_mode_url_filter.h (right): http://codereview.chromium.org/10692158/diff/12021/chrome/browser/managed_mod... chrome/browser/managed_mode_url_filter.h:20: class ManagedModeURLFilter : public base::NonThreadSafe { Nit: one space before : http://codereview.chromium.org/10692158/diff/12021/chrome/browser/managed_mod... File chrome/browser/managed_mode_url_filter_unittest.cc (right): http://codereview.chromium.org/10692158/diff/12021/chrome/browser/managed_mod... chrome/browser/managed_mode_url_filter_unittest.cc:59: // Block domain and all subdomains, for any filtered scheme. Did you mean "Allow domain ..." ? http://codereview.chromium.org/10692158/diff/12021/chrome/browser/net/chrome_... File chrome/browser/net/chrome_network_delegate.cc (right): http://codereview.chromium.org/10692158/diff/12021/chrome/browser/net/chrome_... chrome/browser/net/chrome_network_delegate.cc:17: #include "chrome/browser/managed_mode.h" Needed? http://codereview.chromium.org/10692158/diff/12021/chrome/browser/net/chrome_... chrome/browser/net/chrome_network_delegate.cc:185: managed_mode_url_filter_->IsURLWhitelisted(request->url())) { Blocked when whitelisted? After looking at the impl I guess a "!" is missing here. http://codereview.chromium.org/10692158/diff/12021/chrome/browser/policy/url_... File chrome/browser/policy/url_blacklist_manager.cc (right): http://codereview.chromium.org/10692158/diff/12021/chrome/browser/policy/url_... chrome/browser/policy/url_blacklist_manager.cc:149: for (size_t i = 0; i < arraysize(kStandardSchemes); ++i) { return IsStandardScheme(url.scheme())? http://codereview.chromium.org/10692158/diff/12021/chrome/browser/policy/url_... File chrome/browser/policy/url_blacklist_manager.h (right): http://codereview.chromium.org/10692158/diff/12021/chrome/browser/policy/url_... chrome/browser/policy/url_blacklist_manager.h:41: // Allows or blocks URLs matching one of the filters, depending on |allow. Nit: |allow| http://codereview.chromium.org/10692158/diff/12021/chrome/browser/policy/url_... chrome/browser/policy/url_blacklist_manager.h:69: bool* match_subdomains, Please document the new argument.
http://codereview.chromium.org/10692158/diff/12021/chrome/common/extensions/m... File chrome/common/extensions/matcher/url_matcher.cc (right): http://codereview.chromium.org/10692158/diff/12021/chrome/common/extensions/m... chrome/common/extensions/matcher/url_matcher.cc:262: return CreateCondition(URLMatcherCondition::HOST_SUFFIX, suffix + kEndOfPath); Hmm PATH_SUFFIX? If you fix this, do you mind adding a test that would make this fail before the fix?
Great to see that the URLMatcher works well for this usecase. I have just a couple of nits. http://codereview.chromium.org/10692158/diff/12021/chrome/browser/managed_mod... File chrome/browser/managed_mode.cc (right): http://codereview.chromium.org/10692158/diff/12021/chrome/browser/managed_mod... chrome/browser/managed_mode.cc:179: const ManagedModeURLFilter* ManagedMode::GetURLFilter() { nit: // static http://codereview.chromium.org/10692158/diff/12021/chrome/browser/policy/url_... File chrome/browser/policy/url_blacklist_manager.h (right): http://codereview.chromium.org/10692158/diff/12021/chrome/browser/policy/url_... chrome/browser/policy/url_blacklist_manager.h:92: int id_; can you URLMatcherConditionSet::ID instead of int here? The URLMatcher deals with so many IDs that it is difficult to handle them if they are just denoted with int. -> URLMatcherConditionSet::ID id_; std::map<URLMatcherConditionSet::ID, FilterComponents> filters_; http://codereview.chromium.org/10692158/diff/12021/chrome/browser/policy/url_... chrome/browser/policy/url_blacklist_manager.h:93: std::map<int, FilterComponents> filters_; Isn't FilterComponents big enough to justify a linked_ptr?
Thanks for the reviews! http://codereview.chromium.org/10692158/diff/12021/chrome/browser/managed_mod... File chrome/browser/managed_mode.cc (right): http://codereview.chromium.org/10692158/diff/12021/chrome/browser/managed_mod... chrome/browser/managed_mode.cc:21: #include "content/public/browser/browser_thread.h" On 2012/07/13 12:32:11, Joao da Silva wrote: > Nit: order Done. http://codereview.chromium.org/10692158/diff/12021/chrome/browser/managed_mod... chrome/browser/managed_mode.cc:41: // |url_filter_| unretained. On 2012/07/13 12:32:11, Joao da Silva wrote: > What do you think of this pattern instead: > > - ManagedMode owns ManagedModeURLFilter. It is created from UI but lives on IO. Yup; see the comment below. > - Every call to the filter is posted Unretained() to IO > - The destructor of ManagedMode() posts DeleteSoon() to IO to delete the filter. This won't work with the current lifetime model of ManagedMode. It's a singleton, so it will be destroyed *very* late during shutdown. Specifically, at this point not only is the IO thread already shut down (which would just result in a leak), but the whole BrowserThread infrastructure (which uses singletons) is as well, which means I can't even call PostTask anymore. What I decided to do instead is to make the singleton leaky, which shouldn't cause any problems (and it doesn't make shutdown take longer). I'll still add the DeleteSoon, but it's only going to be called in tests et al. > This makes sure any task previously posted is executed before deletion. Do we really need to make sure that a posted task actually gets executed when we're shutting down? > - The getter can still DCHECK it's on IO and get the filter, since as long as > the ManagedMode instance is alive then so is the filter on IO. *** I'd still like to keep the URLFilterContext around as a bridge that can be used by ManagedMode from the UI thread. It also simplifies things if we change the lifetime of ManagedMode at some point in the future so it gets destructed earlier. Then we only need to make URLFilterContext RefCountedThreadSafe and can post tasks on it. http://codereview.chromium.org/10692158/diff/12021/chrome/browser/managed_mod... chrome/browser/managed_mode.cc:50: void SetWhitelist(scoped_ptr<base::ListValue> whitelist) { On 2012/07/13 12:32:11, Joao da Silva wrote: > Is this used? Not yet, but it will be once I have the source of the whitelist. Like I said, this is somewhat of a work in progress. http://codereview.chromium.org/10692158/diff/12021/chrome/browser/managed_mod... chrome/browser/managed_mode.cc:179: const ManagedModeURLFilter* ManagedMode::GetURLFilter() { On 2012/07/13 14:10:53, battre wrote: > nit: // static Done. http://codereview.chromium.org/10692158/diff/12021/chrome/browser/managed_mod... chrome/browser/managed_mode.cc:319: url_filter_context_->SetActive(in_managed_mode); On 2012/07/13 12:32:11, Joao da Silva wrote: > Is this guaranteed to be non-NULL at this stage? Hm, good point. I probably do need to set up everything on the UI thread first. http://codereview.chromium.org/10692158/diff/12021/chrome/browser/managed_mod... File chrome/browser/managed_mode_url_filter.cc (right): http://codereview.chromium.org/10692158/diff/12021/chrome/browser/managed_mod... chrome/browser/managed_mode_url_filter.cc:20: using extensions::URLMatcherSchemeFilter; On 2012/07/13 12:32:11, Joao da Silva wrote: > Are these 2 used? Not anymore; removed. http://codereview.chromium.org/10692158/diff/12021/chrome/browser/managed_mod... chrome/browser/managed_mode_url_filter.cc:68: scoped_ptr<URLMatcher> url_matcher(new URLMatcher()); On 2012/07/13 12:32:11, Joao da Silva wrote: > DCHECK(BrowserThread::GetBlockingPool()->RunsTasksOnCurrentThread()); Done. http://codereview.chromium.org/10692158/diff/12021/chrome/browser/managed_mod... File chrome/browser/managed_mode_url_filter.h (right): http://codereview.chromium.org/10692158/diff/12021/chrome/browser/managed_mod... chrome/browser/managed_mode_url_filter.h:20: class ManagedModeURLFilter : public base::NonThreadSafe { On 2012/07/13 12:32:11, Joao da Silva wrote: > Nit: one space before : Done. http://codereview.chromium.org/10692158/diff/12021/chrome/browser/managed_mod... File chrome/browser/managed_mode_url_filter_unittest.cc (right): http://codereview.chromium.org/10692158/diff/12021/chrome/browser/managed_mod... chrome/browser/managed_mode_url_filter_unittest.cc:59: // Block domain and all subdomains, for any filtered scheme. On 2012/07/13 12:32:11, Joao da Silva wrote: > Did you mean "Allow domain ..." ? Yep. Fixed. http://codereview.chromium.org/10692158/diff/12021/chrome/browser/net/chrome_... File chrome/browser/net/chrome_network_delegate.cc (right): http://codereview.chromium.org/10692158/diff/12021/chrome/browser/net/chrome_... chrome/browser/net/chrome_network_delegate.cc:17: #include "chrome/browser/managed_mode.h" On 2012/07/13 12:32:11, Joao da Silva wrote: > Needed? Nope; removed. http://codereview.chromium.org/10692158/diff/12021/chrome/browser/net/chrome_... chrome/browser/net/chrome_network_delegate.cc:185: managed_mode_url_filter_->IsURLWhitelisted(request->url())) { On 2012/07/13 12:32:11, Joao da Silva wrote: > Blocked when whitelisted? After looking at the impl I guess a "!" is missing > here. Indeed :-D Fixed. http://codereview.chromium.org/10692158/diff/12021/chrome/browser/policy/url_... File chrome/browser/policy/url_blacklist_manager.cc (right): http://codereview.chromium.org/10692158/diff/12021/chrome/browser/policy/url_... chrome/browser/policy/url_blacklist_manager.cc:149: for (size_t i = 0; i < arraysize(kStandardSchemes); ++i) { On 2012/07/13 12:32:11, Joao da Silva wrote: > return IsStandardScheme(url.scheme())? Done. Originally I used SchemeIs() because that doesn't have to copy the string, but that's probably premature optimization. http://codereview.chromium.org/10692158/diff/12021/chrome/browser/policy/url_... File chrome/browser/policy/url_blacklist_manager.h (right): http://codereview.chromium.org/10692158/diff/12021/chrome/browser/policy/url_... chrome/browser/policy/url_blacklist_manager.h:41: // Allows or blocks URLs matching one of the filters, depending on |allow. On 2012/07/13 12:32:11, Joao da Silva wrote: > Nit: |allow| Done. http://codereview.chromium.org/10692158/diff/12021/chrome/browser/policy/url_... chrome/browser/policy/url_blacklist_manager.h:69: bool* match_subdomains, On 2012/07/13 12:32:11, Joao da Silva wrote: > Please document the new argument. Done. http://codereview.chromium.org/10692158/diff/12021/chrome/browser/policy/url_... chrome/browser/policy/url_blacklist_manager.h:92: int id_; On 2012/07/13 14:10:53, battre wrote: > can you URLMatcherConditionSet::ID instead of int here? The URLMatcher deals > with so many IDs that it is difficult to handle them if they are just denoted > with int. > -> > URLMatcherConditionSet::ID id_; > std::map<URLMatcherConditionSet::ID, FilterComponents> filters_; Done. http://codereview.chromium.org/10692158/diff/12021/chrome/browser/policy/url_... chrome/browser/policy/url_blacklist_manager.h:93: std::map<int, FilterComponents> filters_; On 2012/07/13 14:10:53, battre wrote: > Isn't FilterComponents big enough to justify a linked_ptr? What's the advantage? I think the overhead of copying the FilterComponents into the map should be acceptable. http://codereview.chromium.org/10692158/diff/12021/chrome/common/extensions/m... File chrome/common/extensions/matcher/url_matcher.cc (right): http://codereview.chromium.org/10692158/diff/12021/chrome/common/extensions/m... chrome/common/extensions/matcher/url_matcher.cc:262: return CreateCondition(URLMatcherCondition::HOST_SUFFIX, suffix + kEndOfPath); On 2012/07/13 12:43:13, Joao da Silva wrote: > Hmm PATH_SUFFIX? It looks like using PATH_SUFFIX vs. HOST_SUFFIX doesn't make a difference here: the condition is only used when determining which parts of the URL to match against, and PATH_SUFFIX and HOST_SUFFIX are equivalent in that regard. In fact, neither one is referenced again in the file. Dominic, do you think it would make sense to reduce the number of conditions? We probably only need to distinguish the following cases: * Match host (HOST_CONTAINS) * Match path (PATH_CONTAINS) * Match query (QUERY_CONTAINS) * Match full URL (URL_{PREFIX,SUFFIX,CONTAINS,EQUALS}) * Match component-separated URL (all others) > If you fix this, do you mind adding a test that would make this fail before the > fix? I'd like to hold off on that until we have determined whether we want to change the enum.
http://codereview.chromium.org/10692158/diff/12021/chrome/common/extensions/m... File chrome/common/extensions/matcher/url_matcher.cc (right): http://codereview.chromium.org/10692158/diff/12021/chrome/common/extensions/m... chrome/common/extensions/matcher/url_matcher.cc:262: return CreateCondition(URLMatcherCondition::HOST_SUFFIX, suffix + kEndOfPath); On 2012/07/13 14:21:07, Bernhard Bauer wrote: > On 2012/07/13 12:43:13, Joao da Silva wrote: > > Hmm PATH_SUFFIX? > > It looks like using PATH_SUFFIX vs. HOST_SUFFIX doesn't make a difference here: > the condition is only used when determining which parts of the URL to match > against, and PATH_SUFFIX and HOST_SUFFIX are equivalent in that regard. In fact, > neither one is referenced again in the file. > > Dominic, do you think it would make sense to reduce the number of conditions? We > probably only need to distinguish the following cases: > > * Match host (HOST_CONTAINS) > * Match path (PATH_CONTAINS) > * Match query (QUERY_CONTAINS) > * Match full URL (URL_{PREFIX,SUFFIX,CONTAINS,EQUALS}) > * Match component-separated URL (all others) > > > If you fix this, do you mind adding a test that would make this fail before > the > > fix? > > I'd like to hold off on that until we have determined whether we want to change > the enum. I have a slight tendency towards sticking to the current naming scheme because I think that otherwise, we would need to come up with a better name for URLMatcherCondition::Criterion which I cannot at the moment.
http://codereview.chromium.org/10692158/diff/12021/chrome/common/extensions/m... File chrome/common/extensions/matcher/url_matcher.cc (right): http://codereview.chromium.org/10692158/diff/12021/chrome/common/extensions/m... chrome/common/extensions/matcher/url_matcher.cc:262: return CreateCondition(URLMatcherCondition::HOST_SUFFIX, suffix + kEndOfPath); On 2012/07/13 15:28:54, battre wrote: > On 2012/07/13 14:21:07, Bernhard Bauer wrote: > > On 2012/07/13 12:43:13, Joao da Silva wrote: > > > Hmm PATH_SUFFIX? > > > > It looks like using PATH_SUFFIX vs. HOST_SUFFIX doesn't make a difference > here: > > the condition is only used when determining which parts of the URL to match > > against, and PATH_SUFFIX and HOST_SUFFIX are equivalent in that regard. In > fact, > > neither one is referenced again in the file. > > > > Dominic, do you think it would make sense to reduce the number of conditions? > We > > probably only need to distinguish the following cases: > > > > * Match host (HOST_CONTAINS) > > * Match path (PATH_CONTAINS) > > * Match query (QUERY_CONTAINS) > > * Match full URL (URL_{PREFIX,SUFFIX,CONTAINS,EQUALS}) > > * Match component-separated URL (all others) > > > > > If you fix this, do you mind adding a test that would make this fail before > > the > > > fix? > > > > I'd like to hold off on that until we have determined whether we want to > change > > the enum. > > I have a slight tendency towards sticking to the current naming scheme because I > think that otherwise, we would need to come up with a better name for > URLMatcherCondition::Criterion which I cannot at the moment. The Criterion enum has the PATH_SUFFIX constant, but it isn't used anywhere. Should it be removed then?
http://codereview.chromium.org/10692158/diff/12021/chrome/common/extensions/m... File chrome/common/extensions/matcher/url_matcher.cc (right): http://codereview.chromium.org/10692158/diff/12021/chrome/common/extensions/m... chrome/common/extensions/matcher/url_matcher.cc:262: return CreateCondition(URLMatcherCondition::HOST_SUFFIX, suffix + kEndOfPath); On 2012/07/13 15:37:12, Joao da Silva wrote: > On 2012/07/13 15:28:54, battre wrote: > > On 2012/07/13 14:21:07, Bernhard Bauer wrote: > > > On 2012/07/13 12:43:13, Joao da Silva wrote: > > > > Hmm PATH_SUFFIX? > > > > > > It looks like using PATH_SUFFIX vs. HOST_SUFFIX doesn't make a difference > > here: > > > the condition is only used when determining which parts of the URL to match > > > against, and PATH_SUFFIX and HOST_SUFFIX are equivalent in that regard. In > > fact, > > > neither one is referenced again in the file. > > > > > > Dominic, do you think it would make sense to reduce the number of > conditions? > > We > > > probably only need to distinguish the following cases: > > > > > > * Match host (HOST_CONTAINS) > > > * Match path (PATH_CONTAINS) > > > * Match query (QUERY_CONTAINS) > > > * Match full URL (URL_{PREFIX,SUFFIX,CONTAINS,EQUALS}) > > > * Match component-separated URL (all others) > > > > > > > If you fix this, do you mind adding a test that would make this fail > before > > > the > > > > fix? > > > > > > I'd like to hold off on that until we have determined whether we want to > > change > > > the enum. > > > > I have a slight tendency towards sticking to the current naming scheme because > I > > think that otherwise, we would need to come up with a better name for > > URLMatcherCondition::Criterion which I cannot at the moment. How about: enum MatchAgainst { MATCH_AGAINST_HOST, MATCH_AGAINST_PATH, MATCH_AGAINST_QUERY, MATCH_AGAINST_URL_FULL, MATCH_AGAINST_URL_COMPONENTS, } ? > The Criterion enum has the PATH_SUFFIX constant, but it isn't used anywhere. > Should it be removed then? It is used in CreatePathSuffixCondition().
http://codereview.chromium.org/10692158/diff/12021/chrome/common/extensions/m... File chrome/common/extensions/matcher/url_matcher.cc (right): http://codereview.chromium.org/10692158/diff/12021/chrome/common/extensions/m... chrome/common/extensions/matcher/url_matcher.cc:262: return CreateCondition(URLMatcherCondition::HOST_SUFFIX, suffix + kEndOfPath); On 2012/07/13 15:37:12, Joao da Silva wrote: > On 2012/07/13 15:28:54, battre wrote: > > On 2012/07/13 14:21:07, Bernhard Bauer wrote: > > > On 2012/07/13 12:43:13, Joao da Silva wrote: > > > > Hmm PATH_SUFFIX? > > > > > > It looks like using PATH_SUFFIX vs. HOST_SUFFIX doesn't make a difference > > here: > > > the condition is only used when determining which parts of the URL to match > > > against, and PATH_SUFFIX and HOST_SUFFIX are equivalent in that regard. In > > fact, > > > neither one is referenced again in the file. > > > > > > Dominic, do you think it would make sense to reduce the number of > conditions? > > We > > > probably only need to distinguish the following cases: > > > > > > * Match host (HOST_CONTAINS) > > > * Match path (PATH_CONTAINS) > > > * Match query (QUERY_CONTAINS) > > > * Match full URL (URL_{PREFIX,SUFFIX,CONTAINS,EQUALS}) > > > * Match component-separated URL (all others) > > > > > > > If you fix this, do you mind adding a test that would make this fail > before > > > the > > > > fix? > > > > > > I'd like to hold off on that until we have determined whether we want to > > change > > > the enum. > > > > I have a slight tendency towards sticking to the current naming scheme because > I > > think that otherwise, we would need to come up with a better name for > > URLMatcherCondition::Criterion which I cannot at the moment. > > The Criterion enum has the PATH_SUFFIX constant, but it isn't used anywhere. > Should it be removed then? I think that line 262 should contain PATH_SUFFIX http://codereview.chromium.org/10692158/diff/12021/chrome/common/extensions/m... chrome/common/extensions/matcher/url_matcher.cc:262: return CreateCondition(URLMatcherCondition::HOST_SUFFIX, suffix + kEndOfPath); On 2012/07/13 16:40:39, Bernhard Bauer wrote: > On 2012/07/13 15:37:12, Joao da Silva wrote: > > On 2012/07/13 15:28:54, battre wrote: > > > On 2012/07/13 14:21:07, Bernhard Bauer wrote: > > > > On 2012/07/13 12:43:13, Joao da Silva wrote: > > > > > Hmm PATH_SUFFIX? > > > > > > > > It looks like using PATH_SUFFIX vs. HOST_SUFFIX doesn't make a difference > > > here: > > > > the condition is only used when determining which parts of the URL to > match > > > > against, and PATH_SUFFIX and HOST_SUFFIX are equivalent in that regard. In > > > fact, > > > > neither one is referenced again in the file. > > > > > > > > Dominic, do you think it would make sense to reduce the number of > > conditions? > > > We > > > > probably only need to distinguish the following cases: > > > > > > > > * Match host (HOST_CONTAINS) > > > > * Match path (PATH_CONTAINS) > > > > * Match query (QUERY_CONTAINS) > > > > * Match full URL (URL_{PREFIX,SUFFIX,CONTAINS,EQUALS}) > > > > * Match component-separated URL (all others) > > > > > > > > > If you fix this, do you mind adding a test that would make this fail > > before > > > > the > > > > > fix? > > > > > > > > I'd like to hold off on that until we have determined whether we want to > > > change > > > > the enum. > > > > > > I have a slight tendency towards sticking to the current naming scheme > because > > I > > > think that otherwise, we would need to come up with a better name for > > > URLMatcherCondition::Criterion which I cannot at the moment. > > How about: > > enum MatchAgainst { > MATCH_AGAINST_HOST, > MATCH_AGAINST_PATH, > MATCH_AGAINST_QUERY, > MATCH_AGAINST_URL_FULL, > MATCH_AGAINST_URL_COMPONENTS, > } > > ? Frankly, I am not such a big fan of this, because I think it makes the code more difficult to understand and write, while the gain is pretty limited. > > The Criterion enum has the PATH_SUFFIX constant, but it isn't used anywhere. > > Should it be removed then? > > It is used in CreatePathSuffixCondition().
On Mon, Jul 16, 2012 at 11:17 AM, <battre@chromium.org> wrote: > > http://codereview.chromium.**org/10692158/diff/12021/** > chrome/common/extensions/**matcher/url_matcher.cc<http://codereview.chromium.org/10692158/diff/12021/chrome/common/extensions/matcher/url_matcher.cc> > File chrome/common/extensions/**matcher/url_matcher.cc (right): > > http://codereview.chromium.**org/10692158/diff/12021/** > chrome/common/extensions/**matcher/url_matcher.cc#**newcode262<http://codereview.chromium.org/10692158/diff/12021/chrome/common/extensions/matcher/url_matcher.cc#newcode262> > chrome/common/extensions/**matcher/url_matcher.cc:262: return > CreateCondition(**URLMatcherCondition::HOST_**SUFFIX, suffix + > kEndOfPath); > On 2012/07/13 15:37:12, Joao da Silva wrote: > >> On 2012/07/13 15:28:54, battre wrote: >> > On 2012/07/13 14:21:07, Bernhard Bauer wrote: >> > > On 2012/07/13 12:43:13, Joao da Silva wrote: >> > > > Hmm PATH_SUFFIX? >> > > >> > > It looks like using PATH_SUFFIX vs. HOST_SUFFIX doesn't make a >> > difference > >> > here: >> > > the condition is only used when determining which parts of the URL >> > to match > >> > > against, and PATH_SUFFIX and HOST_SUFFIX are equivalent in that >> > regard. In > >> > fact, >> > > neither one is referenced again in the file. >> > > >> > > Dominic, do you think it would make sense to reduce the number of >> conditions? >> > We >> > > probably only need to distinguish the following cases: >> > > >> > > * Match host (HOST_CONTAINS) >> > > * Match path (PATH_CONTAINS) >> > > * Match query (QUERY_CONTAINS) >> > > * Match full URL (URL_{PREFIX,SUFFIX,CONTAINS,**EQUALS}) >> > > * Match component-separated URL (all others) >> > > >> > > > If you fix this, do you mind adding a test that would make this >> > fail > >> before >> > > the >> > > > fix? >> > > >> > > I'd like to hold off on that until we have determined whether we >> > want to > >> > change >> > > the enum. >> > >> > I have a slight tendency towards sticking to the current naming >> > scheme because > >> I >> > think that otherwise, we would need to come up with a better name >> > for > >> > URLMatcherCondition::Criterion which I cannot at the moment. >> > > The Criterion enum has the PATH_SUFFIX constant, but it isn't used >> > anywhere. > >> Should it be removed then? >> > > I think that line 262 should contain PATH_SUFFIX That's what I was originally suggesting. That constant is defined in the enum but isn't used anywhere currently. > > > http://codereview.chromium.**org/10692158/diff/12021/** > chrome/common/extensions/**matcher/url_matcher.cc#**newcode262<http://codereview.chromium.org/10692158/diff/12021/chrome/common/extensions/matcher/url_matcher.cc#newcode262> > chrome/common/extensions/**matcher/url_matcher.cc:262: return > CreateCondition(**URLMatcherCondition::HOST_**SUFFIX, suffix + > kEndOfPath); > On 2012/07/13 16:40:39, Bernhard Bauer wrote: > >> On 2012/07/13 15:37:12, Joao da Silva wrote: >> > On 2012/07/13 15:28:54, battre wrote: >> > > On 2012/07/13 14:21:07, Bernhard Bauer wrote: >> > > > On 2012/07/13 12:43:13, Joao da Silva wrote: >> > > > > Hmm PATH_SUFFIX? >> > > > >> > > > It looks like using PATH_SUFFIX vs. HOST_SUFFIX doesn't make a >> > difference > >> > > here: >> > > > the condition is only used when determining which parts of the >> > URL to > >> match >> > > > against, and PATH_SUFFIX and HOST_SUFFIX are equivalent in that >> > regard. In > >> > > fact, >> > > > neither one is referenced again in the file. >> > > > >> > > > Dominic, do you think it would make sense to reduce the number >> > of > >> > conditions? >> > > We >> > > > probably only need to distinguish the following cases: >> > > > >> > > > * Match host (HOST_CONTAINS) >> > > > * Match path (PATH_CONTAINS) >> > > > * Match query (QUERY_CONTAINS) >> > > > * Match full URL (URL_{PREFIX,SUFFIX,CONTAINS,**EQUALS}) >> > > > * Match component-separated URL (all others) >> > > > >> > > > > If you fix this, do you mind adding a test that would make >> > this fail > >> > before >> > > > the >> > > > > fix? >> > > > >> > > > I'd like to hold off on that until we have determined whether we >> > want to > >> > > change >> > > > the enum. >> > > >> > > I have a slight tendency towards sticking to the current naming >> > scheme > >> because >> > I >> > > think that otherwise, we would need to come up with a better name >> > for > >> > > URLMatcherCondition::Criterion which I cannot at the moment. >> > > How about: >> > > enum MatchAgainst { >> MATCH_AGAINST_HOST, >> MATCH_AGAINST_PATH, >> MATCH_AGAINST_QUERY, >> MATCH_AGAINST_URL_FULL, >> MATCH_AGAINST_URL_COMPONENTS, >> } >> > > ? >> > > Frankly, I am not such a big fan of this, because I think it makes the > code more difficult to understand and write, while the gain is pretty > limited. > > > > The Criterion enum has the PATH_SUFFIX constant, but it isn't used >> > anywhere. > >> > Should it be removed then? >> > > It is used in CreatePathSuffixCondition(). >> > CreatePathSuffixCondition() is using HOST_SUFFIX, which I think is a bug (though in practice it may not make a difference). > > http://codereview.chromium.**org/10692158/<http://codereview.chromium.org/106... >
On 2012/07/16 09:19:56, Joao da Silva wrote: > On Mon, Jul 16, 2012 at 11:17 AM, <mailto:battre@chromium.org> wrote: > > > > > http://codereview.chromium.**org/10692158/diff/12021/** > > > chrome/common/extensions/**matcher/url_matcher.cc<http://codereview.chromium.org/10692158/diff/12021/chrome/common/extensions/matcher/url_matcher.cc> > > File chrome/common/extensions/**matcher/url_matcher.cc (right): > > > > http://codereview.chromium.**org/10692158/diff/12021/** > > > chrome/common/extensions/**matcher/url_matcher.cc#**newcode262<http://codereview.chromium.org/10692158/diff/12021/chrome/common/extensions/matcher/url_matcher.cc#newcode262> > > chrome/common/extensions/**matcher/url_matcher.cc:262: return > > CreateCondition(**URLMatcherCondition::HOST_**SUFFIX, suffix + > > kEndOfPath); > > On 2012/07/13 15:37:12, Joao da Silva wrote: > > > >> On 2012/07/13 15:28:54, battre wrote: > >> > On 2012/07/13 14:21:07, Bernhard Bauer wrote: > >> > > On 2012/07/13 12:43:13, Joao da Silva wrote: > >> > > > Hmm PATH_SUFFIX? > >> > > > >> > > It looks like using PATH_SUFFIX vs. HOST_SUFFIX doesn't make a > >> > > difference > > > >> > here: > >> > > the condition is only used when determining which parts of the URL > >> > > to match > > > >> > > against, and PATH_SUFFIX and HOST_SUFFIX are equivalent in that > >> > > regard. In > > > >> > fact, > >> > > neither one is referenced again in the file. > >> > > > >> > > Dominic, do you think it would make sense to reduce the number of > >> conditions? > >> > We > >> > > probably only need to distinguish the following cases: > >> > > > >> > > * Match host (HOST_CONTAINS) > >> > > * Match path (PATH_CONTAINS) > >> > > * Match query (QUERY_CONTAINS) > >> > > * Match full URL (URL_{PREFIX,SUFFIX,CONTAINS,**EQUALS}) > >> > > * Match component-separated URL (all others) > >> > > > >> > > > If you fix this, do you mind adding a test that would make this > >> > > fail > > > >> before > >> > > the > >> > > > fix? > >> > > > >> > > I'd like to hold off on that until we have determined whether we > >> > > want to > > > >> > change > >> > > the enum. > >> > > >> > I have a slight tendency towards sticking to the current naming > >> > > scheme because > > > >> I > >> > think that otherwise, we would need to come up with a better name > >> > > for > > > >> > URLMatcherCondition::Criterion which I cannot at the moment. > >> > > > > The Criterion enum has the PATH_SUFFIX constant, but it isn't used > >> > > anywhere. > > > >> Should it be removed then? > >> > > > > I think that line 262 should contain PATH_SUFFIX > > > That's what I was originally suggesting. That constant is defined in the > enum but isn't used anywhere currently. > > > > > > > > http://codereview.chromium.**org/10692158/diff/12021/** > > > chrome/common/extensions/**matcher/url_matcher.cc#**newcode262<http://codereview.chromium.org/10692158/diff/12021/chrome/common/extensions/matcher/url_matcher.cc#newcode262> > > chrome/common/extensions/**matcher/url_matcher.cc:262: return > > CreateCondition(**URLMatcherCondition::HOST_**SUFFIX, suffix + > > kEndOfPath); > > On 2012/07/13 16:40:39, Bernhard Bauer wrote: > > > >> On 2012/07/13 15:37:12, Joao da Silva wrote: > >> > On 2012/07/13 15:28:54, battre wrote: > >> > > On 2012/07/13 14:21:07, Bernhard Bauer wrote: > >> > > > On 2012/07/13 12:43:13, Joao da Silva wrote: > >> > > > > Hmm PATH_SUFFIX? > >> > > > > >> > > > It looks like using PATH_SUFFIX vs. HOST_SUFFIX doesn't make a > >> > > difference > > > >> > > here: > >> > > > the condition is only used when determining which parts of the > >> > > URL to > > > >> match > >> > > > against, and PATH_SUFFIX and HOST_SUFFIX are equivalent in that > >> > > regard. In > > > >> > > fact, > >> > > > neither one is referenced again in the file. > >> > > > > >> > > > Dominic, do you think it would make sense to reduce the number > >> > > of > > > >> > conditions? > >> > > We > >> > > > probably only need to distinguish the following cases: > >> > > > > >> > > > * Match host (HOST_CONTAINS) > >> > > > * Match path (PATH_CONTAINS) > >> > > > * Match query (QUERY_CONTAINS) > >> > > > * Match full URL (URL_{PREFIX,SUFFIX,CONTAINS,**EQUALS}) > >> > > > * Match component-separated URL (all others) > >> > > > > >> > > > > If you fix this, do you mind adding a test that would make > >> > > this fail > > > >> > before > >> > > > the > >> > > > > fix? > >> > > > > >> > > > I'd like to hold off on that until we have determined whether we > >> > > want to > > > >> > > change > >> > > > the enum. > >> > > > >> > > I have a slight tendency towards sticking to the current naming > >> > > scheme > > > >> because > >> > I > >> > > think that otherwise, we would need to come up with a better name > >> > > for > > > >> > > URLMatcherCondition::Criterion which I cannot at the moment. > >> > > > > How about: > >> > > > > enum MatchAgainst { > >> MATCH_AGAINST_HOST, > >> MATCH_AGAINST_PATH, > >> MATCH_AGAINST_QUERY, > >> MATCH_AGAINST_URL_FULL, > >> MATCH_AGAINST_URL_COMPONENTS, > >> } > >> > > > > ? > >> > > > > Frankly, I am not such a big fan of this, because I think it makes the > > code more difficult to understand and write, while the gain is pretty > > limited. > > > > > > > The Criterion enum has the PATH_SUFFIX constant, but it isn't used > >> > > anywhere. > > > >> > Should it be removed then? > >> > > > > It is used in CreatePathSuffixCondition(). > >> > > > CreatePathSuffixCondition() is using HOST_SUFFIX, which I think is a bug > (though in practice it may not make a difference). Oops, I was looking at my checkout there, where I had already changed it to use PATH_SUFFIX ;-) I reverted everything but the URLBlacklistManaged/URLMatcher changes, and added a test that verifies the criteria created by CreateFooCondition(). Could you TAL? > > > > > > http://codereview.chromium.**org/10692158/%3Chttp://codereview.chromium.org/1...> > >
lgtm https://chromiumcodereview.appspot.com/10692158/diff/20001/chrome/browser/pol... File chrome/browser/policy/url_blacklist_manager.cc (right): https://chromiumcodereview.appspot.com/10692158/diff/20001/chrome/browser/pol... chrome/browser/policy/url_blacklist_manager.cc:262: return true; Suggestion: if (host_length != other_host_length) return host_length > other_host_length; Same for the path_length below.
https://chromiumcodereview.appspot.com/10692158/diff/20001/chrome/browser/pol... File chrome/browser/policy/url_blacklist_manager.cc (right): https://chromiumcodereview.appspot.com/10692158/diff/20001/chrome/browser/pol... chrome/browser/policy/url_blacklist_manager.cc:262: return true; On 2012/07/16 11:26:43, Joao da Silva wrote: > Suggestion: > > if (host_length != other_host_length) > return host_length > other_host_length; > > Same for the path_length below. Done.
Dominic, do the URLMatcher changes LGTY?
LGTM @ URLMatcher changes. Thanks!
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/bauerb@chromium.org/10692158/21008
Change committed as 147005 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
