Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
415153002:
ParseHosts: Allow commas as separators on Mac OS X (Closed)
|
Created:
6 years, 5 months ago by Deprecated (see juliatuttle) Modified:
6 years, 5 months ago Reviewers:
mmenke CC:
chromium-reviews, cbentzel+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Project:
chromium Visibility:
Public. |
DescriptionParseHosts: Allow commas as separators on Mac OS X
Apparently, OS X allows commas as separators between hostnames in the hosts
file. Treat commas the same as whitespace to support this. (Hostnames and IP
addresses will never contain commas, so this shouldn't break anything.)
BUG=396309
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=285718
Patch Set 1 #Patch Set 2 : Make comma-handling OS-specific #
Total comments: 17
Patch Set 3 : Fix nits #
Messages
Total messages: 16 (0 generated)
PTAL, mmenke.
Should we support this on all platforms, or just Macs? Problem with supporting it everywhere is we may encourage people on other platforms who don't know any better to use commas, because they "work" in Chrome, but then break everything else. That's the general problem with overly permissive parsers.
On 2014/07/24 16:54:43, mmenke wrote: > Should we support this on all platforms, or just Macs? Problem with supporting > it everywhere is we may encourage people on other platforms who don't know any > better to use commas, because they "work" in Chrome, but then break everything > else. That's the general problem with overly permissive parsers. I'd rather support it everywhere for simplicity, but I can make it Mac-specific if you want.
On 2014/07/24 16:56:29, ttuttle wrote: > On 2014/07/24 16:54:43, mmenke wrote: > > Should we support this on all platforms, or just Macs? Problem with > supporting > > it everywhere is we may encourage people on other platforms who don't know any > > better to use commas, because they "work" in Chrome, but then break everything > > else. That's the general problem with overly permissive parsers. > > I'd rather support it everywhere for simplicity, but I can make it Mac-specific > if you want. I'll ask Chris if he has an opinion on it when I meet with him tomorrow.
Chris says he wants it to be OS-specific. Here's an OS-specific version.
Just minor comments. https://codereview.chromium.org/415153002/diff/20001/net/dns/dns_hosts.cc File net/dns/dns_hosts.cc (right): https://codereview.chromium.org/415153002/diff/20001/net/dns/dns_hosts.cc#new... net/dns/dns_hosts.cc:27: token_(), While you're here, don't need to use parens for no-argument constructors. https://codereview.chromium.org/415153002/diff/20001/net/dns/dns_hosts.cc#new... net/dns/dns_hosts.cc:59: // if comma_mode_ is _COMMA_IS_TOKEN, fall through: nit: Capitalize if, and I think just COMMA_IS_TOKEN is more common (Or use the full name, I'm happy, either way). https://codereview.chromium.org/415153002/diff/20001/net/dns/dns_hosts.h File net/dns/dns_hosts.h (right): https://codereview.chromium.org/415153002/diff/20001/net/dns/dns_hosts.h#newc... net/dns/dns_hosts.h:79: void NET_EXPORT_PRIVATE ParseHostsWithCommaMode( ParseHostsWithCommaModeForTests? https://codereview.chromium.org/415153002/diff/20001/net/dns/dns_hosts_unitte... File net/dns/dns_hosts_unittest.cc (right): https://codereview.chromium.org/415153002/diff/20001/net/dns/dns_hosts_unitte... net/dns/dns_hosts_unittest.cc:67: TEST(DnsHostsTest, ParseHosts_CommaIsToken) { Bit of a pain, but should probably have one that makes sure it's used as a token only on Mac - it's a trivial test, given these two, but it's often the trivial, but untested, things that end up breaking. https://codereview.chromium.org/415153002/diff/20001/net/dns/dns_hosts_unitte... net/dns/dns_hosts_unittest.cc:69: "127.0.0.1 comma1,comma2"; This fits on one line. https://codereview.chromium.org/415153002/diff/20001/net/dns/dns_hosts_unitte... net/dns/dns_hosts_unittest.cc:69: "127.0.0.1 comma1,comma2"; Also should probably make it const and use kConstNamingScheme. https://codereview.chromium.org/415153002/diff/20001/net/dns/dns_hosts_unitte... net/dns/dns_hosts_unittest.cc:75: } entries[] = { Should also probably use const naming scheme here. https://codereview.chromium.org/415153002/diff/20001/net/dns/dns_hosts_unitte... net/dns/dns_hosts_unittest.cc:86: } Suggest making this block a function.
PTAL. https://codereview.chromium.org/415153002/diff/20001/net/dns/dns_hosts.cc File net/dns/dns_hosts.cc (right): https://codereview.chromium.org/415153002/diff/20001/net/dns/dns_hosts.cc#new... net/dns/dns_hosts.cc:27: token_(), On 2014/07/24 21:32:30, mmenke wrote: > While you're here, don't need to use parens for no-argument constructors. Done. https://codereview.chromium.org/415153002/diff/20001/net/dns/dns_hosts.cc#new... net/dns/dns_hosts.cc:59: // if comma_mode_ is _COMMA_IS_TOKEN, fall through: On 2014/07/24 21:32:30, mmenke wrote: > nit: Capitalize if, and I think just COMMA_IS_TOKEN is more common (Or use the > full name, I'm happy, either way). Done. https://codereview.chromium.org/415153002/diff/20001/net/dns/dns_hosts.h File net/dns/dns_hosts.h (right): https://codereview.chromium.org/415153002/diff/20001/net/dns/dns_hosts.h#newc... net/dns/dns_hosts.h:79: void NET_EXPORT_PRIVATE ParseHostsWithCommaMode( On 2014/07/24 21:32:30, mmenke wrote: > ParseHostsWithCommaModeForTests? Done. https://codereview.chromium.org/415153002/diff/20001/net/dns/dns_hosts_unitte... File net/dns/dns_hosts_unittest.cc (right): https://codereview.chromium.org/415153002/diff/20001/net/dns/dns_hosts_unitte... net/dns/dns_hosts_unittest.cc:67: TEST(DnsHostsTest, ParseHosts_CommaIsToken) { On 2014/07/24 21:32:30, mmenke wrote: > Bit of a pain, but should probably have one that makes sure it's used as a token > only on Mac - it's a trivial test, given these two, but it's often the trivial, > but untested, things that end up breaking. So make one that's conditional on Mac and tests that the behavior of ParseHosts actually uses the right mode depending whether it's on Mac or not? https://codereview.chromium.org/415153002/diff/20001/net/dns/dns_hosts_unitte... net/dns/dns_hosts_unittest.cc:69: "127.0.0.1 comma1,comma2"; On 2014/07/24 21:32:30, mmenke wrote: > Also should probably make it const and use kConstNamingScheme. Done. https://codereview.chromium.org/415153002/diff/20001/net/dns/dns_hosts_unitte... net/dns/dns_hosts_unittest.cc:69: "127.0.0.1 comma1,comma2"; On 2014/07/24 21:32:30, mmenke wrote: > This fits on one line. Done. https://codereview.chromium.org/415153002/diff/20001/net/dns/dns_hosts_unitte... net/dns/dns_hosts_unittest.cc:75: } entries[] = { On 2014/07/24 21:32:30, mmenke wrote: > Should also probably use const naming scheme here. Done. https://codereview.chromium.org/415153002/diff/20001/net/dns/dns_hosts_unitte... net/dns/dns_hosts_unittest.cc:86: } On 2014/07/24 21:32:30, mmenke wrote: > Suggest making this block a function. Done.
https://codereview.chromium.org/415153002/diff/20001/net/dns/dns_hosts_unitte... File net/dns/dns_hosts_unittest.cc (right): https://codereview.chromium.org/415153002/diff/20001/net/dns/dns_hosts_unitte... net/dns/dns_hosts_unittest.cc:67: TEST(DnsHostsTest, ParseHosts_CommaIsToken) { On 2014/07/25 19:34:44, ttuttle wrote: > On 2014/07/24 21:32:30, mmenke wrote: > > Bit of a pain, but should probably have one that makes sure it's used as a > token > > only on Mac - it's a trivial test, given these two, but it's often the > trivial, > > but untested, things that end up breaking. > > So make one that's conditional on Mac and tests that the behavior of ParseHosts > actually uses the right mode depending whether it's on Mac or not? What I suggest is run the test everywhere, but have two different expected hosts structs, with an ifdef to select the correct one. Basically just these two tests combined (I'm fine with just merging the tests, but also fine with keeping the existing one and adding a new one).
On 2014/07/25 19:38:46, mmenke wrote: > https://codereview.chromium.org/415153002/diff/20001/net/dns/dns_hosts_unitte... > File net/dns/dns_hosts_unittest.cc (right): > > https://codereview.chromium.org/415153002/diff/20001/net/dns/dns_hosts_unitte... > net/dns/dns_hosts_unittest.cc:67: TEST(DnsHostsTest, ParseHosts_CommaIsToken) { > On 2014/07/25 19:34:44, ttuttle wrote: > > On 2014/07/24 21:32:30, mmenke wrote: > > > Bit of a pain, but should probably have one that makes sure it's used as a > > token > > > only on Mac - it's a trivial test, given these two, but it's often the > > trivial, > > > but untested, things that end up breaking. > > > > So make one that's conditional on Mac and tests that the behavior of > ParseHosts > > actually uses the right mode depending whether it's on Mac or not? > > What I suggest is run the test everywhere, but have two different expected hosts > structs, with an ifdef to select the correct one. Basically just these two > tests combined (I'm fine with just merging the tests, but also fine with keeping > the existing one and adding a new one). I added a new test with an ifdef to select the correct expected result.
On 2014/07/25 19:47:46, ttuttle wrote: > On 2014/07/25 19:38:46, mmenke wrote: > > > https://codereview.chromium.org/415153002/diff/20001/net/dns/dns_hosts_unitte... > > File net/dns/dns_hosts_unittest.cc (right): > > > > > https://codereview.chromium.org/415153002/diff/20001/net/dns/dns_hosts_unitte... > > net/dns/dns_hosts_unittest.cc:67: TEST(DnsHostsTest, ParseHosts_CommaIsToken) > { > > On 2014/07/25 19:34:44, ttuttle wrote: > > > On 2014/07/24 21:32:30, mmenke wrote: > > > > Bit of a pain, but should probably have one that makes sure it's used as a > > > token > > > > only on Mac - it's a trivial test, given these two, but it's often the > > > trivial, > > > > but untested, things that end up breaking. > > > > > > So make one that's conditional on Mac and tests that the behavior of > > ParseHosts > > > actually uses the right mode depending whether it's on Mac or not? > > > > What I suggest is run the test everywhere, but have two different expected > hosts > > structs, with an ifdef to select the correct one. Basically just these two > > tests combined (I'm fine with just merging the tests, but also fine with > keeping > > the existing one and adding a new one). > > I added a new test with an ifdef to select the correct expected result. Great, upload again and I'll sign off on it today.
It's already in the new patch set. On Fri, Jul 25, 2014 at 3:50 PM, <mmenke@chromium.org> wrote: > On 2014/07/25 19:47:46, ttuttle wrote: > >> On 2014/07/25 19:38:46, mmenke wrote: >> > >> > > https://codereview.chromium.org/415153002/diff/20001/net/ > dns/dns_hosts_unittest.cc > >> > File net/dns/dns_hosts_unittest.cc (right): >> > >> > >> > > https://codereview.chromium.org/415153002/diff/20001/net/ > dns/dns_hosts_unittest.cc#newcode67 > >> > net/dns/dns_hosts_unittest.cc:67: TEST(DnsHostsTest, >> > ParseHosts_CommaIsToken) > >> { >> > On 2014/07/25 19:34:44, ttuttle wrote: >> > > On 2014/07/24 21:32:30, mmenke wrote: >> > > > Bit of a pain, but should probably have one that makes sure it's >> used as >> > a > >> > > token >> > > > only on Mac - it's a trivial test, given these two, but it's often >> the >> > > trivial, >> > > > but untested, things that end up breaking. >> > > >> > > So make one that's conditional on Mac and tests that the behavior of >> > ParseHosts >> > > actually uses the right mode depending whether it's on Mac or not? >> > >> > What I suggest is run the test everywhere, but have two different >> expected >> hosts >> > structs, with an ifdef to select the correct one. Basically just these >> two >> > tests combined (I'm fine with just merging the tests, but also fine with >> keeping >> > the existing one and adding a new one). >> > > I added a new test with an ifdef to select the correct expected result. >> > > Great, upload again and I'll sign off on it today. > > https://codereview.chromium.org/415153002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2014/07/25 19:52:19, ttuttle wrote: > It's already in the new patch set. Sorry, misread your comment.
LGTM
The CQ bit was checked by ttuttle@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ttuttle@chromium.org/415153002/40001
Message was sent while issue was closed.
Change committed as 285718 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
