Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
219613002:
Add support for matching query parameters in URLMatcher (Closed)
|
Created:
6 years, 8 months ago by kaliamoorthi Modified:
6 years, 7 months ago CC:
chromium-reviews, Andrew Wilson, vabr (Chromium), Joao da Silva Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionAdd support for matching query parameters in URLMatcher
This CL modifies the url_matcher to support matching query key=value pattern.
BUG=349997
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=263230
Patch Set 1 #
Total comments: 13
Patch Set 2 : Fixes the review comments from Dominic and Joao #
Total comments: 1
Patch Set 3 : Makes the changes backward compatible #
Total comments: 4
Patch Set 4 : Splits the implementation of queryContains and queryContainsExact #Patch Set 5 : Adds the complete query element match framework to URL matcher #
Total comments: 4
Patch Set 6 : Fixes nits from joao, and some changes for performance improvement. #
Total comments: 10
Patch Set 7 : Fixes comments from Dominic #
Total comments: 2
Patch Set 8 : Fixes nits #
Total comments: 1
Patch Set 9 : Fixes nit from Dominic #
Messages
Total messages: 28 (0 generated)
PTAL
+Jeffrey: This is not 100% compatible with the current API which is already exposed via the declarativeContent API. Do you have concerns? I think the number of extensions using that API is very, very limited at the moment. https://codereview.chromium.org/219613002/diff/1/components/url_matcher/url_m... File components/url_matcher/url_matcher.cc (right): https://codereview.chromium.org/219613002/diff/1/components/url_matcher/url_m... components/url_matcher/url_matcher.cc:253: std::string PrepareQuery(std::string query, bool prepend_query_start = true) { Please add a description of the function. Please add a comment and describe that you intentionally pass |query| via the stack instead of a const-ref. I think that prepend_query_start is not an adequate description of what the parameter does. How about prepend_beginning_of_query_component? https://codereview.chromium.org/219613002/diff/1/components/url_matcher/url_m... components/url_matcher/url_matcher.cc:326: pattern = Either the assignment should become part of the constructor, or we we may re-introduce the if statement. I think the latter may help for readability. https://codereview.chromium.org/219613002/diff/1/components/url_matcher/url_m... components/url_matcher/url_matcher.cc:328: ? prefix.substr(1, prefix.length() - 1) nit: prefix.substr(1) should be sufficient, right? https://codereview.chromium.org/219613002/diff/1/components/url_matcher/url_m... components/url_matcher/url_matcher.cc:348: (!str.empty() && str[0] == '?') ? str.substr(1, str.length() - 1) : str; Can you re-introduce the if statement (also below)? I think that is easier to read. https://codereview.chromium.org/219613002/diff/1/components/url_matcher/url_m... File components/url_matcher/url_matcher_unittest.cc (right): https://codereview.chromium.org/219613002/diff/1/components/url_matcher/url_m... components/url_matcher/url_matcher_unittest.cc:344: // Multiple key value pairs are supported (though not very useful) I think the two previous occurrences are not 100% intuitive and should be pointed out here: https://developer.chrome.com/extensions/events#type-UrlFilter
https://codereview.chromium.org/219613002/diff/1/components/url_matcher/url_m... File components/url_matcher/url_matcher.cc (right): https://codereview.chromium.org/219613002/diff/1/components/url_matcher/url_m... components/url_matcher/url_matcher.cc:253: std::string PrepareQuery(std::string query, bool prepend_query_start = true) { Default parameters are not allowed by the style guide: http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Default_Arguments
https://codereview.chromium.org/219613002/diff/1/components/url_matcher/url_m... File components/url_matcher/url_matcher.cc (right): https://codereview.chromium.org/219613002/diff/1/components/url_matcher/url_m... components/url_matcher/url_matcher.cc:253: std::string PrepareQuery(std::string query, bool prepend_query_start = true) { On 2014/04/01 09:18:47, Joao da Silva wrote: > Default parameters are not allowed by the style guide: > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Default_Arguments I thought so as well and then removed the comment. They are allowed for functions in the anonymous namespace.
PTAL https://codereview.chromium.org/219613002/diff/1/components/url_matcher/url_m... File components/url_matcher/url_matcher.cc (right): https://codereview.chromium.org/219613002/diff/1/components/url_matcher/url_m... components/url_matcher/url_matcher.cc:253: std::string PrepareQuery(std::string query, bool prepend_query_start = true) { On 2014/04/01 08:23:00, battre wrote: > Please add a description of the function. > > Please add a comment and describe that you intentionally pass |query| via the > stack instead of a const-ref. > > I think that prepend_query_start is not an adequate description of what the > parameter does. How about prepend_beginning_of_query_component? Done. https://codereview.chromium.org/219613002/diff/1/components/url_matcher/url_m... components/url_matcher/url_matcher.cc:253: std::string PrepareQuery(std::string query, bool prepend_query_start = true) { On 2014/04/01 09:18:47, Joao da Silva wrote: > Default parameters are not allowed by the style guide: > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Default_Arguments I have removed the default parameter anyway. https://codereview.chromium.org/219613002/diff/1/components/url_matcher/url_m... components/url_matcher/url_matcher.cc:326: pattern = On 2014/04/01 08:23:00, battre wrote: > Either the assignment should become part of the constructor, or we we may > re-introduce the if statement. I think the latter may help for readability. I have reintroduced the if else. https://codereview.chromium.org/219613002/diff/1/components/url_matcher/url_m... components/url_matcher/url_matcher.cc:328: ? prefix.substr(1, prefix.length() - 1) On 2014/04/01 08:23:00, battre wrote: > nit: prefix.substr(1) should be sufficient, right? That simplifies the expression. Fixed it. https://codereview.chromium.org/219613002/diff/1/components/url_matcher/url_m... components/url_matcher/url_matcher.cc:348: (!str.empty() && str[0] == '?') ? str.substr(1, str.length() - 1) : str; On 2014/04/01 08:23:00, battre wrote: > Can you re-introduce the if statement (also below)? I think that is easier to > read. Done. https://codereview.chromium.org/219613002/diff/1/components/url_matcher/url_m... File components/url_matcher/url_matcher_unittest.cc (right): https://codereview.chromium.org/219613002/diff/1/components/url_matcher/url_m... components/url_matcher/url_matcher_unittest.cc:344: // Multiple key value pairs are supported (though not very useful) On 2014/04/01 08:23:00, battre wrote: > I think the two previous occurrences are not 100% intuitive and should be > pointed out here: https://developer.chrome.com/extensions/events#type-UrlFilter Done.
LGTM but please wait for Jeffrey's opinion. https://codereview.chromium.org/219613002/diff/20001/chrome/common/extensions... File chrome/common/extensions/api/events.json (right): https://codereview.chromium.org/219613002/diff/20001/chrome/common/extensions... chrome/common/extensions/api/events.json:265: "description": "Matches if the query segment of the URL contains a specified set of key value pairs with '&' as the delimiter. Query starting with a partial key or value is not supported, i.e., the string should start with a full key optionally followed by an equal sign and a value.", s/Query starting/A queryContains filter starting/
PTAL. I have made the CL backward compatible in this revision.
On 2014/04/01 08:22:59, battre wrote: > +Jeffrey: This is not 100% compatible with the current API which is already > exposed via the declarativeContent API. Do you have concerns? I think the number > of extensions using that API is very, very limited at the moment. I'm not sure what the behavior change was. The test doesn't seem to have captured it. It's probably fine to change the declarativeContent behavior slightly, but any change (or addition) should be called out in the documentation at https://code.google.com/p/chromium/codesearch/#chromium/src/chrome/common/ext... or https://code.google.com/p/chromium/codesearch/#chromium/src/chrome/common/ext...
https://codereview.chromium.org/219613002/diff/30001/components/url_matcher/u... File components/url_matcher/url_matcher.cc (right): https://codereview.chromium.org/219613002/diff/30001/components/url_matcher/u... components/url_matcher/url_matcher.cc:158: const char kQuerySeperator = '&'; kQuerySeparator
https://codereview.chromium.org/219613002/diff/30001/components/url_matcher/u... File components/url_matcher/url_matcher.cc (right): https://codereview.chromium.org/219613002/diff/30001/components/url_matcher/u... components/url_matcher/url_matcher.cc:261: .find(string_pattern_->pattern()) != std::string::npos; I think with the new syntax (CreateQueryContainsCondition and CreateQueryContainsExactCondition) you should revert most of the changes so that QueryContains, QueryPrefix, QuerySuffix, QueryExactMatch operate as they did before your CL. WDTY?
PTAL https://codereview.chromium.org/219613002/diff/30001/components/url_matcher/u... File components/url_matcher/url_matcher.cc (right): https://codereview.chromium.org/219613002/diff/30001/components/url_matcher/u... components/url_matcher/url_matcher.cc:158: const char kQuerySeperator = '&'; On 2014/04/02 07:46:59, Joao da Silva wrote: > kQuerySeparator Done. https://codereview.chromium.org/219613002/diff/30001/components/url_matcher/u... components/url_matcher/url_matcher.cc:261: .find(string_pattern_->pattern()) != std::string::npos; On 2014/04/02 08:14:54, battre wrote: > I think with the new syntax (CreateQueryContainsCondition and > CreateQueryContainsExactCondition) you should revert most of the changes so that > QueryContains, QueryPrefix, QuerySuffix, QueryContainsExact operate as they did > before your CL. WDTY? I decorated the URL for component searches for enabling exact match. Currently query prefix, suffix and equals make use of the URL for component searches. So I have to decorate the query string there as well. The only question that remains is do I need to use QUERY_CONTAINS for the new API QueryExactMatch. If I do, then I need to modify query contains code, which is the implementation in the last version. Alternatively I can add a new QUERY_CONTAINS_EXACT just for the new API. The latest CL has such a code. If you prefer the later one, we could go with it. But, I still need to modify prefix, suffix and equals (in a backward compatible fashion) to make it work with the decoration.
The new patch includes most changes needed for fixing bug 349997 in URLmatcher. The changes are completely backward compatible, so I don't believe that existing API documents needs to be modified. PTAL.
Some nits inline. I'm not really familiar with this code to review it. https://codereview.chromium.org/219613002/diff/110001/components/url_matcher/... File components/url_matcher/url_matcher.h (right): https://codereview.chromium.org/219613002/diff/110001/components/url_matcher/... components/url_matcher/url_matcher.h:333: const std::string url_for_component_searches) const; const std::string& https://codereview.chromium.org/219613002/diff/110001/components/url_matcher/... File components/url_matcher/url_matcher_unittest.cc (right): https://codereview.chromium.org/219613002/diff/110001/components/url_matcher/... components/url_matcher/url_matcher_unittest.cc:552: } // namespace
A friendly reminder for the owners for review. Also, fixes some nits from joao. PTAL. https://codereview.chromium.org/219613002/diff/110001/components/url_matcher/... File components/url_matcher/url_matcher.h (right): https://codereview.chromium.org/219613002/diff/110001/components/url_matcher/... components/url_matcher/url_matcher.h:333: const std::string url_for_component_searches) const; On 2014/04/07 15:12:39, Joao da Silva wrote: > const std::string& Done. https://codereview.chromium.org/219613002/diff/110001/components/url_matcher/... File components/url_matcher/url_matcher_unittest.cc (right): https://codereview.chromium.org/219613002/diff/110001/components/url_matcher/... components/url_matcher/url_matcher_unittest.cc:552: } On 2014/04/07 15:12:39, Joao da Silva wrote: > // namespace Done.
https://codereview.chromium.org/219613002/diff/170001/components/url_matcher/... File components/url_matcher/url_matcher.cc (right): https://codereview.chromium.org/219613002/diff/170001/components/url_matcher/... components/url_matcher/url_matcher.cc:612: return false; The previous lines can be simplified to if (matcher_type_ != rhs.matcher_type_) return matcher_type_ < rhs.matcher_type_; https://codereview.chromium.org/219613002/diff/170001/components/url_matcher/... components/url_matcher/url_matcher.cc:769: // cycles. I am not sure that this performance speedup justifies the code duplication from not deriving URLQueryElementMatcherCondition from URLMatcherCondition. Are you sure it does? Can't the IsMatch Line in 779 start with a check that its string_pattern()->id() is in matching_patterns? https://codereview.chromium.org/219613002/diff/170001/components/url_matcher/... File components/url_matcher/url_matcher.h (right): https://codereview.chromium.org/219613002/diff/170001/components/url_matcher/... components/url_matcher/url_matcher.h:227: enum Type { MATCH_ANY, MATCH_FIRST, MATCH_LAST, MATCH_ALL }; Please add a comment that multiple query parameters with the same URL can occur in a URL. This indicates, which one(s) to consider. https://codereview.chromium.org/219613002/diff/170001/components/url_matcher/... components/url_matcher/url_matcher.h:244: // Verifies if the URL query contains the key value pair. // Returns whether the URL...
https://codereview.chromium.org/219613002/diff/170001/components/url_matcher/... File components/url_matcher/url_matcher.cc (right): https://codereview.chromium.org/219613002/diff/170001/components/url_matcher/... components/url_matcher/url_matcher.cc:769: // cycles. On 2014/04/08 10:51:21, battre wrote: > I am not sure that this performance speedup justifies the code duplication from > not deriving URLQueryElementMatcherCondition from URLMatcherCondition. Are you > sure it does? > > Can't the IsMatch Line in 779 start with a check that its string_pattern()->id() > is in matching_patterns? You can ignore this as discussed. It would make the memory management much more complicated. https://codereview.chromium.org/219613002/diff/170001/components/url_matcher/... File components/url_matcher/url_matcher_unittest.cc (right): https://codereview.chromium.org/219613002/diff/170001/components/url_matcher/... components/url_matcher/url_matcher_unittest.cc:509: std::string url_query, please pass by const ref. Also below.
PTAL https://codereview.chromium.org/219613002/diff/170001/components/url_matcher/... File components/url_matcher/url_matcher.cc (right): https://codereview.chromium.org/219613002/diff/170001/components/url_matcher/... components/url_matcher/url_matcher.cc:612: return false; On 2014/04/08 10:51:21, battre wrote: > The previous lines can be simplified to > if (matcher_type_ != rhs.matcher_type_) > return matcher_type_ < rhs.matcher_type_; Done. https://codereview.chromium.org/219613002/diff/170001/components/url_matcher/... File components/url_matcher/url_matcher.h (right): https://codereview.chromium.org/219613002/diff/170001/components/url_matcher/... components/url_matcher/url_matcher.h:227: enum Type { MATCH_ANY, MATCH_FIRST, MATCH_LAST, MATCH_ALL }; On 2014/04/08 10:51:21, battre wrote: > Please add a comment that multiple query parameters with the same URL can occur > in a URL. This indicates, which one(s) to consider. Done. https://codereview.chromium.org/219613002/diff/170001/components/url_matcher/... components/url_matcher/url_matcher.h:244: // Verifies if the URL query contains the key value pair. On 2014/04/08 10:51:21, battre wrote: > // Returns whether the URL... Done. https://codereview.chromium.org/219613002/diff/170001/components/url_matcher/... File components/url_matcher/url_matcher_unittest.cc (right): https://codereview.chromium.org/219613002/diff/170001/components/url_matcher/... components/url_matcher/url_matcher_unittest.cc:509: std::string url_query, On 2014/04/08 11:20:20, battre wrote: > please pass by const ref. Also below. Done.
The subject of this CL doesn't really parse, and the description doesn't contain any more information. Maybe "Add support for matching query parameters in URLMatcher"? Then in the description you could explain that this will allow matching prefixes of key=value parameters, or exact matches (ideally with simple examples). https://codereview.chromium.org/219613002/diff/210001/components/url_matcher/... File components/url_matcher/url_matcher.cc (right): https://codereview.chromium.org/219613002/diff/210001/components/url_matcher/... components/url_matcher/url_matcher.cc:255: const char kQuerySeparator = '&'; This constant does not match the comment at the beginning of the block. https://codereview.chromium.org/219613002/diff/210001/components/url_matcher/... components/url_matcher/url_matcher.cc:575: key_ = kQueryComponentDelimiter + key + std::string("="); Explicit string constructor isn't necessary here.
Fixes few nits
On 2014/04/09 11:01:06, kaliamoorthi wrote: > Fixes few nits PTAL
LGTM, thanks!
On 2014/04/09 11:10:08, Bernhard Bauer wrote: > LGTM, thanks! Thank you all for reviewing. Hi Dominic, Do you have more comments on this or is it good to be landed?
Yes, thank you! https://codereview.chromium.org/219613002/diff/230001/components/url_matcher/... File components/url_matcher/url_matcher.cc (right): https://codereview.chromium.org/219613002/diff/230001/components/url_matcher/... components/url_matcher/url_matcher.cc:768: // are not found, no need to verify match that is expected to take more If not all query elements are found,
On 2014/04/11 07:10:08, battre wrote: > Yes, thank you! > > https://codereview.chromium.org/219613002/diff/230001/components/url_matcher/... > File components/url_matcher/url_matcher.cc (right): > > https://codereview.chromium.org/219613002/diff/230001/components/url_matcher/... > components/url_matcher/url_matcher.cc:768: // are not found, no need to verify > match that is expected to take more > If not all query elements are found, Well, I should say "No (no more major comments), thank you!"
LGTM (to be explicit)
The CQ bit was checked by kaliamoorthi@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kaliamoorthi@chromium.org/219613002/25...
Message was sent while issue was closed.
Change committed as 263230 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
