Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2861183002:
Google search subdomains included for Safesearch (Closed)
DescriptionGoogle search subdomains included for Safesearch
This also fixes the Google homepage URL and the case for trailing dots.
BUG=670412
Review-Url: https://codereview.chromium.org/2861183002
Cr-Commit-Position: refs/heads/master@{#471297}
Committed: https://chromium.googlesource.com/chromium/src/+/a66d812aa78debb856c1938f439e98feabeaeada
Patch Set 1 #
Total comments: 8
Patch Set 2 : Fixed review comments #
Total comments: 4
Patch Set 3 : Fixed review comments #
Total comments: 19
Patch Set 4 : Fixed review comments #
Total comments: 2
Patch Set 5 : Nits #Patch Set 6 : Formatting #
Messages
Total messages: 35 (13 generated)
Description was changed from ========== Google search subdomains included for Safesearch This also fixes the Google homepage URL and the case for trailing dots. BUG=670412 ========== to ========== Google search subdomains included for Safesearch This also fixes the Google homepage URL and the case for trailing dots. BUG=670412 ==========
igorcov@chromium.org changed reviewers: + atwilson@chromium.org
atwilson@ Please take a look. I've included only the subdomains I know about, since we have no clear answer for that yet.
Mostly LG https://codereview.chromium.org/2861183002/diff/1/components/google/core/brow... File components/google/core/browser/google_util.cc (right): https://codereview.chromium.org/2861183002/diff/1/components/google/core/brow... components/google/core/browser/google_util.cc:44: const std::set<base::StringPiece> gGoogleSearchSubdomains{"ipv4.google.com", This is a relatively complicated static which I think is frowned upon because we can't be certain when the constructor/destructor will be invoked: https://engdoc.corp.google.com/eng/doc/devguide/cpp/styleguide.shtml?cl=head#... Use a raw array of char strings instead. https://codereview.chromium.org/2861183002/diff/1/components/google/core/brow... components/google/core/browser/google_util.cc:160: // like google.com./ Log a bug for this and add a link to the bug with this TODO. https://codereview.chromium.org/2861183002/diff/1/components/google/core/brow... components/google/core/browser/google_util.cc:244: !IsGoogleSearchSubdomainUrl(url, DISALLOW_NON_STANDARD_PORTS)) Multi-line if statements require braces. https://codereview.chromium.org/2861183002/diff/1/components/google/core/brow... components/google/core/browser/google_util.cc:257: !IsGoogleSearchSubdomainUrl(url, DISALLOW_NON_STANDARD_PORTS)) For a multi-line if statement, you need to use braces.
Thanks Drew. I've addressed the comments. https://codereview.chromium.org/2861183002/diff/1/components/google/core/brow... File components/google/core/browser/google_util.cc (right): https://codereview.chromium.org/2861183002/diff/1/components/google/core/brow... components/google/core/browser/google_util.cc:44: const std::set<base::StringPiece> gGoogleSearchSubdomains{"ipv4.google.com", On 2017/05/09 13:36:47, Andrew T Wilson (Slow) wrote: > This is a relatively complicated static which I think is frowned upon because we > can't be certain when the constructor/destructor will be invoked: > https://engdoc.corp.google.com/eng/doc/devguide/cpp/styleguide.shtml?cl=head#... > > Use a raw array of char strings instead. Done. https://codereview.chromium.org/2861183002/diff/1/components/google/core/brow... components/google/core/browser/google_util.cc:160: // like google.com./ On 2017/05/09 13:36:47, Andrew T Wilson (Slow) wrote: > Log a bug for this and add a link to the bug with this TODO. Done. https://codereview.chromium.org/2861183002/diff/1/components/google/core/brow... components/google/core/browser/google_util.cc:244: !IsGoogleSearchSubdomainUrl(url, DISALLOW_NON_STANDARD_PORTS)) On 2017/05/09 13:36:47, Andrew T Wilson (Slow) wrote: > Multi-line if statements require braces. Done. https://codereview.chromium.org/2861183002/diff/1/components/google/core/brow... components/google/core/browser/google_util.cc:257: !IsGoogleSearchSubdomainUrl(url, DISALLOW_NON_STANDARD_PORTS)) On 2017/05/09 13:36:47, Andrew T Wilson (Slow) wrote: > For a multi-line if statement, you need to use braces. Done.
igorcov@chromium.org changed reviewers: + isherman@chromium.org
isherman@chromium.org: Please review changes. Thank you.
https://codereview.chromium.org/2861183002/diff/20001/components/google/core/... File components/google/core/browser/google_util.cc (right): https://codereview.chromium.org/2861183002/diff/20001/components/google/core/... components/google/core/browser/google_util.cc:235: host = host.substr(0, host.length() - 1); This is fine, but if we do this anywhere else we might as well create a helper func: base::StringPiece StripTrailingDot(const base::StringPiece& host) https://codereview.chromium.org/2861183002/diff/20001/components/google/core/... components/google/core/browser/google_util.cc:238: gGoogleSearchSubdomains, Binary search is probably overkill for a two-item list. But if you are going to keep this, you'd better add comments where gGoogleSearchSubdomains is defined to state it's supposed to be in sorted order.
https://codereview.chromium.org/2861183002/diff/20001/components/google/core/... File components/google/core/browser/google_util.cc (right): https://codereview.chromium.org/2861183002/diff/20001/components/google/core/... components/google/core/browser/google_util.cc:235: host = host.substr(0, host.length() - 1); On 2017/05/10 14:54:17, Andrew T Wilson (Slow) wrote: > This is fine, but if we do this anywhere else we might as well create a helper > func: > > base::StringPiece StripTrailingDot(const base::StringPiece& host) > Done, thanks. https://codereview.chromium.org/2861183002/diff/20001/components/google/core/... components/google/core/browser/google_util.cc:238: gGoogleSearchSubdomains, On 2017/05/10 14:54:17, Andrew T Wilson (Slow) wrote: > Binary search is probably overkill for a two-item list. > > But if you are going to keep this, you'd better add comments where > gGoogleSearchSubdomains is defined to state it's supposed to be in sorted order. Right, forgot to put the comment. Don't know which version would be better. I've replaced it with a simple loop as it's safer now.
isherman@chromium.org changed reviewers: + pkasting@chromium.org
+Peter who knows more about the finer details of Google domain identification. In the meantime, here are some nit-picks: https://codereview.chromium.org/2861183002/diff/40001/components/google/core/... File components/google/core/browser/google_util.cc (right): https://codereview.chromium.org/2861183002/diff/40001/components/google/core/... components/google/core/browser/google_util.cc:44: const char* const gGoogleSearchSubdomains[] = {"ipv4.google.com", nit: s/g/k, since this is a constant. https://codereview.chromium.org/2861183002/diff/40001/components/google/core/... File components/google/core/browser/google_util.h (right): https://codereview.chromium.org/2861183002/diff/40001/components/google/core/... components/google/core/browser/google_util.h:107: PortPermission port_permission); Does this function need to be public, or could it be private (i.e. tucked into the anonymous namespace within the impl file)? https://codereview.chromium.org/2861183002/diff/40001/components/google/core/... File components/google/core/browser/google_util_unittest.cc (right): https://codereview.chromium.org/2861183002/diff/40001/components/google/core/... components/google/core/browser/google_util_unittest.cc:141: // the url parameter or the hash fragment. Why did you indent these lines? Is this "git cl format"'s output? I'm surprised -- I don't think these need to be indented. https://codereview.chromium.org/2861183002/diff/40001/components/google/core/... components/google/core/browser/google_util_unittest.cc:142: "%s://www.google.com/search?%s=something", Hmm, what's the diff on this line? Is the diff tool just confused, or am I? =)
LGTM https://codereview.chromium.org/2861183002/diff/40001/components/google/core/... File components/google/core/browser/google_util.cc (right): https://codereview.chromium.org/2861183002/diff/40001/components/google/core/... components/google/core/browser/google_util.cc:44: const char* const gGoogleSearchSubdomains[] = {"ipv4.google.com", On 2017/05/11 00:33:35, Ilya Sherman wrote: > nit: s/g/k, since this is a constant. Also, prefer to define this locally in the function that uses it. Could probably be constexpr as well. https://codereview.chromium.org/2861183002/diff/40001/components/google/core/... components/google/core/browser/google_util.cc:234: PortPermission port_permission) { Nit: Callers all seem to pass DISALLOW_NON_STANDARD_PORTS here, so I'd remove this second argument and hardcode that. (That goes for other functions above too as applicable.) https://codereview.chromium.org/2861183002/diff/40001/components/google/core/... components/google/core/browser/google_util.cc:243: for (const char* subdomain : gGoogleSearchSubdomains) { Nit: It might make sense to CR_DEFINE_STATIC_LOCAL a string array, instead of the char* array, so you can easily just use std::find() here. Or possibly a flat_map, which could be allocated to the right size and would have even simpler syntax to use. But maybe that's overkill. https://codereview.chromium.org/2861183002/diff/40001/components/google/core/... components/google/core/browser/google_util.cc:244: if (host == subdomain) { Nit: {} not necessary https://codereview.chromium.org/2861183002/diff/40001/components/google/core/... File components/google/core/browser/google_util_unittest.cc (right): https://codereview.chromium.org/2861183002/diff/40001/components/google/core/... components/google/core/browser/google_util_unittest.cc:175: "%s://www.google.com./#%s=something", "%s://www.google.de./#%s=something", Nit: One per line
Thank you for review. I've addressed the comments, please take a look again. https://codereview.chromium.org/2861183002/diff/40001/components/google/core/... File components/google/core/browser/google_util.cc (right): https://codereview.chromium.org/2861183002/diff/40001/components/google/core/... components/google/core/browser/google_util.cc:44: const char* const gGoogleSearchSubdomains[] = {"ipv4.google.com", On 2017/05/11 00:48:08, Peter Kasting wrote: > On 2017/05/11 00:33:35, Ilya Sherman wrote: > > nit: s/g/k, since this is a constant. > > Also, prefer to define this locally in the function that uses it. > > Could probably be constexpr as well. Included all this locally inside a static set. https://codereview.chromium.org/2861183002/diff/40001/components/google/core/... components/google/core/browser/google_util.cc:234: PortPermission port_permission) { On 2017/05/11 00:48:08, Peter Kasting wrote: > Nit: Callers all seem to pass DISALLOW_NON_STANDARD_PORTS here, so I'd remove > this second argument and hardcode that. (That goes for other functions above > too as applicable.) Done. https://codereview.chromium.org/2861183002/diff/40001/components/google/core/... components/google/core/browser/google_util.cc:243: for (const char* subdomain : gGoogleSearchSubdomains) { On 2017/05/11 00:48:08, Peter Kasting wrote: > Nit: It might make sense to CR_DEFINE_STATIC_LOCAL a string array, instead of > the char* array, so you can easily just use std::find() here. Or possibly a > flat_map, which could be allocated to the right size and would have even simpler > syntax to use. But maybe that's overkill. Done. https://codereview.chromium.org/2861183002/diff/40001/components/google/core/... File components/google/core/browser/google_util.h (right): https://codereview.chromium.org/2861183002/diff/40001/components/google/core/... components/google/core/browser/google_util.h:107: PortPermission port_permission); On 2017/05/11 00:33:35, Ilya Sherman wrote: > Does this function need to be public, or could it be private (i.e. tucked into > the anonymous namespace within the impl file)? Done. https://codereview.chromium.org/2861183002/diff/40001/components/google/core/... File components/google/core/browser/google_util_unittest.cc (right): https://codereview.chromium.org/2861183002/diff/40001/components/google/core/... components/google/core/browser/google_util_unittest.cc:141: // the url parameter or the hash fragment. On 2017/05/11 00:33:36, Ilya Sherman wrote: > Why did you indent these lines? Is this "git cl format"'s output? I'm > surprised -- I don't think these need to be indented. For this and bellow, it's the git cl format output. I've put it all back and ignored the warning. Do you know where could I file a bug for this?
LGTM https://codereview.chromium.org/2861183002/diff/40001/components/google/core/... File components/google/core/browser/google_util_unittest.cc (right): https://codereview.chromium.org/2861183002/diff/40001/components/google/core/... components/google/core/browser/google_util_unittest.cc:141: // the url parameter or the hash fragment. On 2017/05/11 11:30:05, igorcov wrote: > On 2017/05/11 00:33:36, Ilya Sherman wrote: > > Why did you indent these lines? Is this "git cl format"'s output? I'm > > surprised -- I don't think these need to be indented. > > For this and bellow, it's the git cl format output. I've put it all back and > ignored the warning. Do you know where could I file a bug for this? Honestly, the new indenting looks more style-guide-correct to me, since this is a continuation of the previous statement rather than a new scope. I would probably leave git cl format's output alone and not file a bug, but if you think it's wrong I would leave it alone and file one :) https://codereview.chromium.org/2861183002/diff/60001/components/google/core/... File components/google/core/browser/google_util.cc (right): https://codereview.chromium.org/2861183002/diff/60001/components/google/core/... components/google/core/browser/google_util.cc:181: NOTREACHED(); So, is this truly NOTREACHED today, or is it reached in the case you TODO about? If it's truly NOTREACHED, it could probably be written more simply as follows: DCHECK_NE(std::string::npos, last_dot); (I know this is separate cleanup, but since you're touching here...) If it can be reached, though, then we shouldn't be NOTREACHED/DCHECKing it; we either need to do something to handle it, or if nothing should be done for now, just remove the conditional block here.
https://codereview.chromium.org/2861183002/diff/60001/components/google/core/... File components/google/core/browser/google_util.cc (right): https://codereview.chromium.org/2861183002/diff/60001/components/google/core/... components/google/core/browser/google_util.cc:181: NOTREACHED(); On 2017/05/11 15:37:59, Peter Kasting wrote: > So, is this truly NOTREACHED today, or is it reached in the case you TODO about? > If it's truly NOTREACHED, it could probably be written more simply as follows: > > DCHECK_NE(std::string::npos, last_dot); > > (I know this is separate cleanup, but since you're touching here...) > > If it can be reached, though, then we shouldn't be NOTREACHED/DCHECKing it; we > either need to do something to handle it, or if nothing should be done for now, > just remove the conditional block here. This is truly NOTREACHED. The hostname has to have at least one dot.
The CQ bit was checked by igorcov@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pkasting@chromium.org Link to the patchset: https://codereview.chromium.org/2861183002/#ps80001 (title: "Nits")
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: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
LGTM too, thanks. https://codereview.chromium.org/2861183002/diff/40001/components/google/core/... File components/google/core/browser/google_util_unittest.cc (right): https://codereview.chromium.org/2861183002/diff/40001/components/google/core/... components/google/core/browser/google_util_unittest.cc:141: // the url parameter or the hash fragment. On 2017/05/11 15:37:58, Peter Kasting wrote: > On 2017/05/11 11:30:05, igorcov wrote: > > On 2017/05/11 00:33:36, Ilya Sherman wrote: > > > Why did you indent these lines? Is this "git cl format"'s output? I'm > > > surprised -- I don't think these need to be indented. > > > > For this and bellow, it's the git cl format output. I've put it all back and > > ignored the warning. Do you know where could I file a bug for this? > > Honestly, the new indenting looks more style-guide-correct to me, since this is > a continuation of the previous statement rather than a new scope. I would > probably leave git cl format's output alone and not file a bug, but if you think > it's wrong I would leave it alone and file one :) I agree that if this is git cl format output, there's no need to change it in this CL. However, I do think this formatting is poor, so I filed a bug: https://bugs.chromium.org/p/chromium/issues/detail?id=721496 https://codereview.chromium.org/2861183002/diff/40001/components/google/core/... components/google/core/browser/google_util_unittest.cc:175: "%s://www.google.com./#%s=something", "%s://www.google.de./#%s=something", On 2017/05/11 00:48:08, Peter Kasting wrote: > Nit: One per line This is also git cl format output, right? Ah! I wonder: does adding a trailing comma cause git cl format to "do the right thing"?
https://codereview.chromium.org/2861183002/diff/40001/components/google/core/... File components/google/core/browser/google_util_unittest.cc (right): https://codereview.chromium.org/2861183002/diff/40001/components/google/core/... components/google/core/browser/google_util_unittest.cc:141: // the url parameter or the hash fragment. On 2017/05/11 19:15:04, Ilya Sherman wrote: > On 2017/05/11 15:37:58, Peter Kasting wrote: > > On 2017/05/11 11:30:05, igorcov wrote: > > > On 2017/05/11 00:33:36, Ilya Sherman wrote: > > > > Why did you indent these lines? Is this "git cl format"'s output? I'm > > > > surprised -- I don't think these need to be indented. > > > > > > For this and bellow, it's the git cl format output. I've put it all back and > > > ignored the warning. Do you know where could I file a bug for this? > > > > Honestly, the new indenting looks more style-guide-correct to me, since this > is > > a continuation of the previous statement rather than a new scope. I would > > probably leave git cl format's output alone and not file a bug, but if you > think > > it's wrong I would leave it alone and file one :) > > I agree that if this is git cl format output, there's no need to change it in > this CL. However, I do think this formatting is poor, so I filed a bug: > https://bugs.chromium.org/p/chromium/issues/detail?id=721496 Nico says this is working as intended, so I'll need to update my mental style guide =) https://codereview.chromium.org/2861183002/diff/40001/components/google/core/... components/google/core/browser/google_util_unittest.cc:175: "%s://www.google.com./#%s=something", "%s://www.google.de./#%s=something", On 2017/05/11 19:15:04, Ilya Sherman wrote: > On 2017/05/11 00:48:08, Peter Kasting wrote: > > Nit: One per line > > This is also git cl format output, right? > > Ah! I wonder: does adding a trailing comma cause git cl format to "do the right > thing"? Per the clang-format bug, it seems like a tailing comma should indeed improve the formatting here.
Thanks Ilya, I've added the comma and we have the closed bracked where it belongs now. But, the strings haven't been put in separate lines. Will have to live with that.
The CQ bit was checked by igorcov@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from isherman@chromium.org, pkasting@chromium.org Link to the patchset: https://codereview.chromium.org/2861183002/#ps100001 (title: "Formatting")
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_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by igorcov@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 100001, "attempt_start_ts": 1494595193564950,
"parent_rev": "a3385c2e0f06b3331338de800b014f9ce0086718", "commit_rev":
"a66d812aa78debb856c1938f439e98feabeaeada"}
Message was sent while issue was closed.
Description was changed from ========== Google search subdomains included for Safesearch This also fixes the Google homepage URL and the case for trailing dots. BUG=670412 ========== to ========== Google search subdomains included for Safesearch This also fixes the Google homepage URL and the case for trailing dots. BUG=670412 Review-Url: https://codereview.chromium.org/2861183002 Cr-Commit-Position: refs/heads/master@{#471297} Committed: https://chromium.googlesource.com/chromium/src/+/a66d812aa78debb856c1938f439e... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/a66d812aa78debb856c1938f439e...
Message was sent while issue was closed.
On 2017/05/12 10:37:25, igorcov wrote: > Thanks Ilya, I've added the comma and we have the closed bracked where it > belongs now. But, the strings haven't been put in separate lines. Will have to > live with that. Is that a case where git cl format gets angry if they're on one line? If so, I would definitely file a bug for this one.
Message was sent while issue was closed.
On 2017/05/12 20:09:29, Peter Kasting wrote: > On 2017/05/12 10:37:25, igorcov wrote: > > Thanks Ilya, I've added the comma and we have the closed bracked where it > > belongs now. But, the strings haven't been put in separate lines. Will have to > > live with that. > > Is that a case where git cl format gets angry if they're on one line? If so, I > would definitely file a bug for this one. I filed https://bugs.chromium.org/p/chromium/issues/detail?id=721911 for this. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
