Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2455373002:
Add implicit trailing dot domain matching support to URLPattern. (Closed)
|
Created:
4 years, 1 month ago by sense (YandexTeam) Modified:
4 years, 1 month ago Reviewers:
Devlin CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd implicit trailing dot domain matching support to URLPattern.
url_matcher::URLMatcher handles both trailing dot and non trailing dot
domains as same. This CL enables same behaviour for URLPattern to be
consistent across different components.
BUG=606321
Committed: https://crrev.com/453ab88927c99dc1564b66aaa9003b870f886201
Cr-Commit-Position: refs/heads/master@{#429867}
Patch Set 1 #
Total comments: 1
Patch Set 2 : Use StringPiece to speedup MatchesHost. #
Total comments: 7
Patch Set 3 : Optimize host matching by using StringPiece directly. #Patch Set 4 : Add a comment with additional links to an external spec, cleanup test. #
Messages
Total messages: 25 (9 generated)
Description was changed from ========== Add implicit trailing dot matching support to URLPattern. url_matcher::URLMatcher handles both trailing dot and non trailing dot domains as same. This CL enables same behaviour for URLPattern to be consistent across different components. BUG=606321 ========== to ========== Add implicit trailing dot domain matching support to URLPattern. url_matcher::URLMatcher handles both trailing dot and non trailing dot domains as same. This CL enables same behaviour for URLPattern to be consistent across different components. BUG=606321 ==========
sense@yandex-team.ru changed reviewers: + rdevlin.cronin@chromium.org
Overall, looks pretty good, thanks for the patch! https://codereview.chromium.org/2455373002/diff/1/extensions/common/url_patte... File extensions/common/url_pattern.cc (right): https://codereview.chromium.org/2455373002/diff/1/extensions/common/url_patte... extensions/common/url_pattern.cc:412: const std::string test_host(CanonicalizeHostForMatching(test)); This code is called under some pretty performance-sensitive times (e.g. web request matching, etc). We need to do a pretty big overhaul to make it all faster, but I wonder if we can at least avoid a string copy here. Could we instead use a StringPiece test_host and StringPiece pattern_host that both have the trailing dot (if any) removed, and thus avoid the copies?
On 2016/10/28 19:17:27, Devlin wrote: > Overall, looks pretty good, thanks for the patch! > > https://codereview.chromium.org/2455373002/diff/1/extensions/common/url_patte... > File extensions/common/url_pattern.cc (right): > > https://codereview.chromium.org/2455373002/diff/1/extensions/common/url_patte... > extensions/common/url_pattern.cc:412: const std::string > test_host(CanonicalizeHostForMatching(test)); > This code is called under some pretty performance-sensitive times (e.g. web > request matching, etc). We need to do a pretty big overhaul to make it all > faster, but I wonder if we can at least avoid a string copy here. Could we > instead use a StringPiece test_host and StringPiece pattern_host that both have > the trailing dot (if any) removed, and thus avoid the copies? Thanks! I've simplified dot removing by using only StringPiece logic. This eliminates all unnecessary copying.
Nice! Thanks for the StringPiece change. Just a few last nits. https://codereview.chromium.org/2455373002/diff/20001/extensions/common/url_p... File extensions/common/url_pattern.cc (right): https://codereview.chromium.org/2455373002/diff/20001/extensions/common/url_p... extensions/common/url_pattern.cc:417: const std::string& test_host_str = test.host(); GURL::host() actually returns a std::string (rather than a const &), so there this should be const std::string. (The StringPiece work was important to prevent an additional copy from the host str.) https://codereview.chromium.org/2455373002/diff/20001/extensions/common/url_p... extensions/common/url_pattern.cc:444: if (test_host.find(pattern_host, simpler: if (!test_host.ends_with(pattern_host)) return false; https://codereview.chromium.org/2455373002/diff/20001/extensions/common/url_p... File extensions/common/url_pattern_unittest.cc (right): https://codereview.chromium.org/2455373002/diff/20001/extensions/common/url_p... extensions/common/url_pattern_unittest.cc:856: // Both patterns should match trailing dot and non trailing dot domains. For those interested in the future, can we add a link to the spec (or other reputable source) indicating that trailing dots are valid and are equivalent to the non-trailing dot variant? https://codereview.chromium.org/2455373002/diff/20001/extensions/common/url_p... extensions/common/url_pattern_unittest.cc:858: EXPECT_TRUE(pattern.MatchesURL(GURL(normal_domain))); No need for the GURL() ctor here - we can just pass in normal_domain directly (same for below).
https://codereview.chromium.org/2455373002/diff/20001/extensions/common/url_p... File extensions/common/url_pattern.cc (right): https://codereview.chromium.org/2455373002/diff/20001/extensions/common/url_p... extensions/common/url_pattern.cc:417: const std::string& test_host_str = test.host(); On 2016/10/31 16:13:17, Devlin wrote: > GURL::host() actually returns a std::string (rather than a const &), so there > this should be const std::string. (The StringPiece work was important to > prevent an additional copy from the host str.) Actually, even better yet, we have a GURL::host_piece() method that would work here if we change CanonicalizeHostForMatching to take a StringPiece.
On 2016/10/31 16:58:48, Devlin wrote: > https://codereview.chromium.org/2455373002/diff/20001/extensions/common/url_p... > File extensions/common/url_pattern.cc (right): > > https://codereview.chromium.org/2455373002/diff/20001/extensions/common/url_p... > extensions/common/url_pattern.cc:417: const std::string& test_host_str = > test.host(); > On 2016/10/31 16:13:17, Devlin wrote: > > GURL::host() actually returns a std::string (rather than a const &), so there > > this should be const std::string. (The StringPiece work was important to > > prevent an additional copy from the host str.) > > Actually, even better yet, we have a GURL::host_piece() method that would work > here if we change CanonicalizeHostForMatching to take a StringPiece. Thanks! That's a very good improvement, I didn't notice this *_piece() methods before.
Looks like you missed a couple of comments from the last patch set. :) https://codereview.chromium.org/2455373002/diff/20001/extensions/common/url_p... File extensions/common/url_pattern_unittest.cc (right): https://codereview.chromium.org/2455373002/diff/20001/extensions/common/url_p... extensions/common/url_pattern_unittest.cc:856: // Both patterns should match trailing dot and non trailing dot domains. On 2016/10/31 16:13:17, Devlin wrote: > For those interested in the future, can we add a link to the spec (or other > reputable source) indicating that trailing dots are valid and are equivalent to > the non-trailing dot variant? Looks like you missed this one. https://codereview.chromium.org/2455373002/diff/20001/extensions/common/url_p... extensions/common/url_pattern_unittest.cc:858: EXPECT_TRUE(pattern.MatchesURL(GURL(normal_domain))); On 2016/10/31 16:13:17, Devlin wrote: > No need for the GURL() ctor here - we can just pass in normal_domain directly > (same for below). And this one.
On 2016/11/01 14:52:14, Devlin wrote: > Looks like you missed a couple of comments from the last patch set. :) > > https://codereview.chromium.org/2455373002/diff/20001/extensions/common/url_p... > File extensions/common/url_pattern_unittest.cc (right): > > https://codereview.chromium.org/2455373002/diff/20001/extensions/common/url_p... > extensions/common/url_pattern_unittest.cc:856: // Both patterns should match > trailing dot and non trailing dot domains. > On 2016/10/31 16:13:17, Devlin wrote: > > For those interested in the future, can we add a link to the spec (or other > > reputable source) indicating that trailing dots are valid and are equivalent > to > > the non-trailing dot variant? > > Looks like you missed this one. > > https://codereview.chromium.org/2455373002/diff/20001/extensions/common/url_p... > extensions/common/url_pattern_unittest.cc:858: > EXPECT_TRUE(pattern.MatchesURL(GURL(normal_domain))); > On 2016/10/31 16:13:17, Devlin wrote: > > No need for the GURL() ctor here - we can just pass in normal_domain directly > > (same for below). > > And this one. Oh, what a shame. That was not intentional, I'm sorry :) I've added additional description with links to the RFC and an article which were mentioned in the bug.
lgtm; thanks for the great patch!
The CQ bit was checked by sense@yandex-team.ru
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by sense@yandex-team.ru
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by sense@yandex-team.ru
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Add implicit trailing dot domain matching support to URLPattern. url_matcher::URLMatcher handles both trailing dot and non trailing dot domains as same. This CL enables same behaviour for URLPattern to be consistent across different components. BUG=606321 ========== to ========== Add implicit trailing dot domain matching support to URLPattern. url_matcher::URLMatcher handles both trailing dot and non trailing dot domains as same. This CL enables same behaviour for URLPattern to be consistent across different components. BUG=606321 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Add implicit trailing dot domain matching support to URLPattern. url_matcher::URLMatcher handles both trailing dot and non trailing dot domains as same. This CL enables same behaviour for URLPattern to be consistent across different components. BUG=606321 ========== to ========== Add implicit trailing dot domain matching support to URLPattern. url_matcher::URLMatcher handles both trailing dot and non trailing dot domains as same. This CL enables same behaviour for URLPattern to be consistent across different components. BUG=606321 Committed: https://crrev.com/453ab88927c99dc1564b66aaa9003b870f886201 Cr-Commit-Position: refs/heads/master@{#429867} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/453ab88927c99dc1564b66aaa9003b870f886201 Cr-Commit-Position: refs/heads/master@{#429867} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
