Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
7647014:
Replace whitespace at beginning and end of file with hyphens, rather than silently discarding. (Closed)
|
Created:
9 years, 4 months ago by kenrb Modified:
9 years, 4 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, darin-cc_chromium.org, jshin+watch_chromium.org, brettw-cc_chromium.org, kenrb1 Visibility:
Public. |
DescriptionReplace whitespace at beginning and end of file with hyphens, rather than silently discarding.
BUG=90217
TEST=all
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=98148
Patch Set 1 #Patch Set 2 : Changed filename char replacement to percent encoding. #
Total comments: 1
Patch Set 3 : Changed to using hyphen replacement again #
Total comments: 7
Patch Set 4 : Formatting fixes #Patch Set 5 : Removed redundant function call #Patch Set 6 : Added unit test, fixed a bug #Patch Set 7 : Formatting fixup #Patch Set 8 : Retrying patch set due to Rietveld issues #
Total comments: 3
Patch Set 9 : Changes based on review comments #
Total comments: 1
Patch Set 10 : Rebased to trunk, resolved merge conflict #
Total comments: 1
Patch Set 11 : Trying again, seem to be merge issues on try server #Messages
Total messages: 26 (0 generated)
Here's a simpler idea. Forget about the escaping. On Windows, just keep track of how many characters are getting trimmed off the end (resetting the count when we hit the empty filename paths). When we hit the last step append that many - character to the end of the filename. And sorry for suggesting encoding in the first place, had I realized we were already replacing invalid path characters with - I wouldn't have proposed it. http://codereview.chromium.org/7647014/diff/2001/net/base/net_util.cc File net/base/net_util.cc (right): http://codereview.chromium.org/7647014/diff/2001/net/base/net_util.cc#newcode162 net/base/net_util.cc:162: const char16 kWhitespaceUTF16[] = { Seems like we should add missed whitespace characters to the list in string_util. And since we're going to need to include . characters in our check, we'll need to make a local string anyway.
@jschuh: Does it change anything to know that % encoding is what IE does? As we discussed, Firefox behavior is to leave the character as is which is probably wrong, but IE ends up doing byte-wise encoding on illegal characters (which is slightly different from what I implemented in the last patch). So on Windows this isn't totally strange. On Wed, Aug 17, 2011 at 10:38 AM, <jschuh@chromium.org> wrote: > Here's a simpler idea. Forget about the escaping. On Windows, just keep > track of > how many characters are getting trimmed off the end (resetting the count > when we > hit the empty filename paths). When we hit the last step append that many - > character to the end of the filename. > > And sorry for suggesting encoding in the first place, had I realized we > were > already replacing invalid path characters with - I wouldn't have proposed > it. > > > > http://codereview.chromium.**org/7647014/diff/2001/net/**base/net_util.cc<htt... > File net/base/net_util.cc (right): > > http://codereview.chromium.**org/7647014/diff/2001/net/** > base/net_util.cc#newcode162<http://codereview.chromium.org/7647014/diff/2001/net/base/net_util.cc#newcode162> > net/base/net_util.cc:162: const char16 kWhitespaceUTF16[] = { > Seems like we should add missed whitespace characters to the list in > string_util. And since we're going to need to include . characters in > our check, we'll need to make a local string anyway. > > http://codereview.chromium.**org/7647014/<http://codereview.chromium.org/7647... >
On 2011/08/17 14:49:54, kenrb_google.com wrote: > @jschuh: Does it change anything to know that % encoding is what IE does? As > we discussed, Firefox behavior is to leave the character as is which is > probably wrong, but IE ends up doing byte-wise encoding on illegal > characters (which is slightly different from what I implemented in the last > patch). So on Windows this isn't totally strange. I think it's fine to use the - character, as we've already set the precedent with the invalid characters and it's certainly easier to do it that way. > > On Wed, Aug 17, 2011 at 10:38 AM, <mailto:jschuh@chromium.org> wrote: > > > Here's a simpler idea. Forget about the escaping. On Windows, just keep > > track of > > how many characters are getting trimmed off the end (resetting the count > > when we > > hit the empty filename paths). When we hit the last step append that many - > > character to the end of the filename. > > > > And sorry for suggesting encoding in the first place, had I realized we > > were > > already replacing invalid path characters with - I wouldn't have proposed > > it. > > > > > > > > > http://codereview.chromium.**org/7647014/diff/2001/net/**base/net_util.cc%3Ch...> > > File net/base/net_util.cc (right): > > > > http://codereview.chromium.**org/7647014/diff/2001/net/** > > > base/net_util.cc#newcode162<http://codereview.chromium.org/7647014/diff/2001/net/base/net_util.cc#newcode162> > > net/base/net_util.cc:162: const char16 kWhitespaceUTF16[] = { > > Seems like we should add missed whitespace characters to the list in > > string_util. And since we're going to need to include . characters in > > our check, we'll need to make a local string anyway. > > > > > http://codereview.chromium.**org/7647014/%3Chttp://codereview.chromium.org/76...> > >
On 2011/08/17 16:32:47, Justin Schuh wrote: > I think it's fine to use the - character, as we've already set the precedent > with the invalid characters and it's certainly easier to do it that way. > > Would you mind giving it another look then?
http://codereview.chromium.org/7647014/diff/3003/net/base/net_util.cc File net/base/net_util.cc (right): http://codereview.chromium.org/7647014/diff/3003/net/base/net_util.cc#newcode163 net/base/net_util.cc:163: const typename STR::value_type Formatting is all messed up here. Make sure visual studio is configured properly, use gcl lint, and check the style guide. http://codereview.chromium.org/7647014/diff/3003/net/base/net_util.cc#newcode... net/base/net_util.cc:1428: Remove the blank line. http://codereview.chromium.org/7647014/diff/3003/net/base/net_util.cc#newcode... net/base/net_util.cc:1459: trimmed_trailing_character_count += CountTrailingChars(filename, " ."); You already have the count, so no reason to duplicate the search before trimming. http://codereview.chromium.org/7647014/diff/3003/net/base/net_util.cc#newcode... net/base/net_util.cc:1493: TrimWhitespace(path, TRIM_TRAILING, &path); Shouldn't this check include trailing . characters, in addition to the trailing utf16 whitespace? My suggestion is just figure out the length and then truncate the string as appropriate.
http://codereview.chromium.org/7647014/diff/3003/net/base/net_util.cc File net/base/net_util.cc (right): http://codereview.chromium.org/7647014/diff/3003/net/base/net_util.cc#newcode... net/base/net_util.cc:1459: trimmed_trailing_character_count += CountTrailingChars(filename, " ."); On 2011/08/17 20:02:48, Justin Schuh wrote: > You already have the count, so no reason to duplicate the search before > trimming. The filename may have changed since the last time we counted. http://codereview.chromium.org/7647014/diff/3003/net/base/net_util.cc#newcode... net/base/net_util.cc:1493: TrimWhitespace(path, TRIM_TRAILING, &path); Trailing dots should have been stripped at line 1459. This is to account for behavior in file_util::ReplaceIllegalCharactersInPath, which unfortunately strips whitespace before replacing illegal characters. We want trailing illegal characters to be replaced, not stripped, so we do it here before file_util::ReplaceIllegalCharactersInPath does. On 2011/08/17 20:02:48, Justin Schuh wrote: > Shouldn't this check include trailing . characters, in addition to the trailing > utf16 whitespace? My suggestion is just figure out the length and then truncate > the string as appropriate.
http://codereview.chromium.org/7647014/diff/3003/net/base/net_util.cc File net/base/net_util.cc (right): http://codereview.chromium.org/7647014/diff/3003/net/base/net_util.cc#newcode... net/base/net_util.cc:1459: trimmed_trailing_character_count += CountTrailingChars(filename, " ."); On 2011/08/17 20:25:51, kenrb wrote: > On 2011/08/17 20:02:48, Justin Schuh wrote: > > You already have the count, so no reason to duplicate the search before > > trimming. > > The filename may have changed since the last time we counted. To clarify, you just called your CountTrailingChars function, which means you know how many trailing chars need to be trimmed. So, there's no reason to call find_last_not_of again (which was necessary before your change).
On 2011/08/17 20:32:46, Justin Schuh wrote: > http://codereview.chromium.org/7647014/diff/3003/net/base/net_util.cc > File net/base/net_util.cc (right): > > http://codereview.chromium.org/7647014/diff/3003/net/base/net_util.cc#newcode... > net/base/net_util.cc:1459: trimmed_trailing_character_count += > CountTrailingChars(filename, " ."); > On 2011/08/17 20:25:51, kenrb wrote: > > On 2011/08/17 20:02:48, Justin Schuh wrote: > > > You already have the count, so no reason to duplicate the search before > > > trimming. > > > > The filename may have changed since the last time we counted. > > To clarify, you just called your CountTrailingChars function, which means you > know how many trailing chars need to be trimmed. So, there's no reason to call > find_last_not_of again (which was necessary before your change). Corrected
On 2011/08/17 20:59:36, kenrb wrote: > On 2011/08/17 20:32:46, Justin Schuh wrote: > > http://codereview.chromium.org/7647014/diff/3003/net/base/net_util.cc > > File net/base/net_util.cc (right): > > > > > http://codereview.chromium.org/7647014/diff/3003/net/base/net_util.cc#newcode... > > net/base/net_util.cc:1459: trimmed_trailing_character_count += > > CountTrailingChars(filename, " ."); > > On 2011/08/17 20:25:51, kenrb wrote: > > > On 2011/08/17 20:02:48, Justin Schuh wrote: > > > > You already have the count, so no reason to duplicate the search before > > > > trimming. > > > > > > The filename may have changed since the last time we counted. > > > > To clarify, you just called your CountTrailingChars function, which means you > > know how many trailing chars need to be trimmed. So, there's no reason to call > > find_last_not_of again (which was necessary before your change). > > Corrected lgtm
Eric, mind a review on the content side?
I'm running the trybots here: http://codereview.chromium.org/7631028 Eric, you probably want to hold off your review until Ken adds a test.
Eric, can you please have a look at this now? I'll also cc you on the original bug. I have added a new unit test case, and had to modify some others because it changes behavior on Windows when there are illegal trailing characters on filenames. Thanks in advance, Ken On Wed, Aug 17, 2011 at 5:19 PM, <jschuh@chromium.org> wrote: > I'm running the trybots here: http://codereview.chromium.**org/7631028<http://codereview.chromium.org/7631028> > > Eric, you probably want to hold off your review until Ken adds a test. > > > http://codereview.chromium.**org/7647014/<http://codereview.chromium.org/7647... >
I have been getting server errors from rietveld when trying to view this change (for instance downloading the patch file). It doesn't seem to have been resolved yet. I suggest re-uploading the patch in case that helps. Cheers.
On 2011/08/19 01:53:29, eroman wrote: > I have been getting server errors from rietveld when trying to view this change > (for instance downloading the patch file). > > It doesn't seem to have been resolved yet. I suggest re-uploading the patch in > case that helps. > > Cheers. I've re-uploaded now. Let me know if it still doesn't work.
Ah good it's working now! Sorry for the delay, will post comments shortly.
LGTM. Adding Asanka in case he has any comments (since I haven't worked in this file). My one concern is the tracking of trimmed_trailing_character_count is fairly subtle. I wander if we could instead of "trimming" the leading+trailing periods, just do a substitution pass. http://codereview.chromium.org/7647014/diff/10001/net/base/net_util.cc File net/base/net_util.cc (right): http://codereview.chromium.org/7647014/diff/10001/net/base/net_util.cc#newcod... net/base/net_util.cc:165: trailing_chars[]) { nit: please merge this onto previous line. http://codereview.chromium.org/7647014/diff/10001/net/base/net_util.cc#newcod... net/base/net_util.cc:167: input.find_last_not_of(trailing_chars); nit: indent continued lines by 4. http://codereview.chromium.org/7647014/diff/10001/net/base/net_util.cc#newcod... net/base/net_util.cc:1490: trimmed_trailing_character_count += path.length(); nit: I find this a bit hard to read. I suggest saving path.length into a temporary variable, sorta like: size_t path_length_before_trim = path.length(); ... trimmed_trailing_character_count += (path_length_before_trim - path.length();
On 2011/08/19 18:29:09, eroman wrote: > LGTM. > > Adding Asanka in case he has any comments (since I haven't worked in this file). > Asanka: You mentioned this morning that there will be a conflict with a different CL, but do you mind having a look at this change in the meantime? > My one concern is the tracking of trimmed_trailing_character_count is fairly > subtle. I wander if we could instead of "trimming" the leading+trailing periods, > just do a substitution pass. > If you look at the earlier patch sets, you can see that this was the first approach I tried. It ended up being convoluted because trimming happens in different ways from and through different code paths. Count-and-append ends up being a much more straightforward change.
LGTM. But you will have to change this substantially if you rebase behind http://codereview.chromium.org/7607013/, assuming that CL lands before this one. http://codereview.chromium.org/7647014/diff/15002/net/base/net_util.cc File net/base/net_util.cc (right): http://codereview.chromium.org/7647014/diff/15002/net/base/net_util.cc#newcod... net/base/net_util.cc:162: typename STR::size_type CountTrailingChars( Nit: This is only instantiated for std::string.
On 2011/08/19 19:51:19, asanka wrote: > LGTM. > > But you will have to change this substantially if you rebase behind > http://codereview.chromium.org/7607013/, assuming that CL lands before this one. > I'll wait for yours to land, then, before going any further with this one. http://codereview.chromium.org/7647014/diff/15002/net/base/net_util.cc#newcod... > net/base/net_util.cc:162: typename STR::size_type CountTrailingChars( > Nit: This is only instantiated for std::string. I don't understand this. Isn't it also instantiated for string16, alias for std::wstring?
On 2011/08/19 20:30:16, Ken Buchanan wrote: http://codereview.chromium.org/7647014/diff/15002/net/base/net_util.cc#newcod... > > net/base/net_util.cc:162: typename STR::size_type CountTrailingChars( > > Nit: This is only instantiated for std::string. > > I don't understand this. Isn't it also instantiated for string16, alias for > std::wstring? Meaning that you are only calling CountTrailingChars() on a std::string.
asanka and eroman: Can either or both of you please have another look? Some changes were required to resolve the merge conflicts. Also, can someone please run this through trybots again?
On 2011/08/22 16:05:24, Ken Buchanan wrote: > asanka and eroman: > > Can either or both of you please have another look? Some changes were required > to resolve the merge conflicts. > > Also, can someone please run this through trybots again? You don't need a re-review just for resolving merge conflicts. And nsylvain just resolved your trybot access, so you should be good to go. Just run the trybots and when it comes back clean check the commit queue box.
Still LGTM. http://codereview.chromium.org/7647014/diff/19001/net/base/net_util.cc File net/base/net_util.cc (right): http://codereview.chromium.org/7647014/diff/19001/net/base/net_util.cc#newcod... net/base/net_util.cc:1492: // e.g. Gmail might think evil.exe. is safe, so we don't want it to become What about ".evil.exe"?
On 2011/08/22 18:01:31, asanka wrote: > What about ".evil.exe"? That shouldn't evade any filters, because the extension doesn't change. Gmail (or any other service) would recognize that as an executable extension and provide appropriate restrictions.
On 2011/08/22 18:07:42, Ken Buchanan wrote: > On 2011/08/22 18:01:31, asanka wrote: > > What about ".evil.exe"? > > That shouldn't evade any filters, because the extension doesn't change. Gmail > (or any other service) would recognize that as an executable extension and > provide appropriate restrictions. Ok. SG.
Change committed as 98148 |
