Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1118813005:
Don't create bookmark files on OSX that start with a dot. (Closed)
|
Created:
5 years, 7 months ago by jansauer Modified:
5 years, 6 months ago Reviewers:
Scott Hess - ex-Googler CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDon't create bookmark files on OSX that start with a dot.
When creating a bookmark file by draging and droping the URL to the desktop
chromium uses the page title for the filename. In cases where the title starts
with a dot the webloc file will also start with a dot. Since files starting
with a dot are considered hidden and not shown by e.g. the Finder users won't
see the expected resulting file. To circumvent this a dot as the first
character is handled like any other character not allowed in a filename and
gets replaced by a hyphen.
BUG=138917
TEST=Paste "data:text/html,<title>. . . hey!</title>" into the Omnibox and drag
and drop the URL to the desktop.
Committed: https://crrev.com/300ff8956b18363815c4a2b3a6fd208dc95d1342
Cr-Commit-Position: refs/heads/master@{#333649}
Patch Set 1 #Patch Set 2 : Remove GOATS file. #
Total comments: 1
Patch Set 3 : Use stringByReplacingOccurrencesOfString #Patch Set 4 : Remove unused if #
Total comments: 1
Patch Set 5 : Improve code style #Patch Set 6 : Add myself to the AUTHORS file #Messages
Total messages: 28 (11 generated)
jan@jansauer.de changed reviewers: - jan@jansauer.de
shess@chromium.org changed reviewers: + shess@chromium.org
https://codereview.chromium.org/1118813005/diff/20001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell.mm (right): https://codereview.chromium.org/1118813005/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell.mm:750: This code is already terrible, maybe just follow the others and do: filteredName = [filteredName stringByReplacingOccurencesOfString:@"." withString:@"-" options:0 range:NSMakeRange(0,1)];
On 2015/05/05 19:15:48, Scott Hess wrote: > https://codereview.chromium.org/1118813005/diff/20001/chrome/browser/ui/cocoa... > File chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell.mm > (right): > > https://codereview.chromium.org/1118813005/diff/20001/chrome/browser/ui/cocoa... > chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell.mm:750: > This code is already terrible, maybe just follow the others and do: > filteredName = [filteredName stringByReplacingOccurencesOfString:@"." > withString:@"-" options:0 range:NSMakeRange(0,1)]; As you wish ... I have tried using stringByReplacingOccurrencesOfString bevor but had some problems with the syntax. I've never used Objective-C bevor.
On 2015/05/05 20:54:53, jansauer wrote: > On 2015/05/05 19:15:48, Scott Hess wrote: > > > https://codereview.chromium.org/1118813005/diff/20001/chrome/browser/ui/cocoa... > > File chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell.mm > > (right): > > > > > https://codereview.chromium.org/1118813005/diff/20001/chrome/browser/ui/cocoa... > > chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell.mm:750: > > This code is already terrible, maybe just follow the others and do: > > filteredName = [filteredName stringByReplacingOccurencesOfString:@"." > > withString:@"-" options:0 range:NSMakeRange(0,1)]; > > As you wish ... > > I have tried using stringByReplacingOccurrencesOfString bevor but had some > problems with the syntax. > I've never used Objective-C bevor. Just wanted to check whether this was planning to move forward. My earlier comment was meant to suggest an alternative which I thought fit better with the surrounding code, and AFAICT your response wasn't so much a defense of the code you used as shrugging and saying "Maybe? I don't really know!" If you're feeling attacked by the "This code is already terrible" comment, keep in mind that I was referring to the existing code. I can see your rationale for checking before replacing, I just think that the check really isn't saving you much and it makes you case different from the other cases. This form of check-then-replace might even be slower, because it has to create a new temporary object. So I think just having the contents of the if() clause without the if() is fine. If the char in (0,1) is not ".", it will return the original string, otherwise it will return an adjusted string, so no need for the test. If you feel that checking-then-replace is the way to go, then I'd suggest using a cheaper check and replace, like: if ([filteredName length] && [filteredName characterAtIndex:0]=='.') { filteredName = [filteredName stringByReplacingCharactersInRange:NSMakeRange(0,1) withString:@"_"]; } But it's conceivable that this would be slower even in the case without a leading '.', simply because of Objective-C messaging overhead.
On 2015/06/01 18:26:23, Scott Hess wrote: > On 2015/05/05 20:54:53, jansauer wrote: > > On 2015/05/05 19:15:48, Scott Hess wrote: > > > > > > https://codereview.chromium.org/1118813005/diff/20001/chrome/browser/ui/cocoa... > > > File chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell.mm > > > (right): > > > > > > > > > https://codereview.chromium.org/1118813005/diff/20001/chrome/browser/ui/cocoa... > > > chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell.mm:750: > > > This code is already terrible, maybe just follow the others and do: > > > filteredName = [filteredName stringByReplacingOccurencesOfString:@"." > > > withString:@"-" options:0 range:NSMakeRange(0,1)]; > > > > As you wish ... > > > > I have tried using stringByReplacingOccurrencesOfString bevor but had some > > problems with the syntax. > > I've never used Objective-C bevor. > > Just wanted to check whether this was planning to move forward. My earlier > comment was meant to suggest an alternative which I thought fit better with the > surrounding code, and AFAICT your response wasn't so much a defense of the code > you used as shrugging and saying "Maybe? I don't really know!" > > If you're feeling attacked by the "This code is already terrible" comment, keep > in mind that I was referring to the existing code. I can see your rationale for > checking before replacing, I just think that the check really isn't saving you > much and it makes you case different from the other cases. This form of > check-then-replace might even be slower, because it has to create a new > temporary object. So I think just having the contents of the if() clause > without the if() is fine. If the char in (0,1) is not ".", it will return the > original string, otherwise it will return an adjusted string, so no need for the > test. > > If you feel that checking-then-replace is the way to go, then I'd suggest using > a cheaper check and replace, like: > if ([filteredName length] && [filteredName characterAtIndex:0]=='.') { > filteredName = [filteredName > stringByReplacingCharactersInRange:NSMakeRange(0,1) withString:@"_"]; > } > But it's conceivable that this would be slower even in the case without a > leading '.', simply because of Objective-C messaging overhead. Sorry for the delay. I made the change to use `stringByReplacingOccurrencesOfString` right after your last comment but forgot to remove the if statement.
LGTM, but I think the style guide requires a change to the formatting of the line. I was going to bless it as-is, but then noticed that the style guide was pretty explicit about this. [In case you're wondering, the above should be interpreted as "you have my blessing to check in after formatting to make the style guide happy." If you want me to double-check again, that's fine, style guides can be pedantic.] [[Let me know if you need me to CQ things after your patch.]] https://codereview.chromium.org/1118813005/diff/60001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell.mm (right): https://codereview.chromium.org/1118813005/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell.mm:745: withString:@"-" options:0 range:NSMakeRange(0,1)]; http://google-styleguide.googlecode.com/svn/trunk/objcguide.xml?showone=Metho... Unfortunately, the combo of super-long method signature and Chromium's 80-column limit makes this one sad. I think you can line all the colons up if you put a line break between the = and the [, even with the resulting 4-space indent.
The CQ bit was checked by jan@jansauer.de
The patchset sent to the CQ was uploaded after l-g-t-m from shess@chromium.org Link to the patchset: https://codereview.chromium.org/1118813005/#ps80001 (title: "Improve code style")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1118813005/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by jan@jansauer.de
The patchset sent to the CQ was uploaded after l-g-t-m from shess@chromium.org Link to the patchset: https://codereview.chromium.org/1118813005/#ps100001 (title: "Add myself to the AUTHORS file")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1118813005/100001
In case someone else ends up here, CLA has been signed, so LGTM for AUTHORS change.
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: cast_shell_android on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_andr...)
The CQ bit was checked by jan@jansauer.de
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1118813005/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by jan@jansauer.de
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1118813005/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/300ff8956b18363815c4a2b3a6fd208dc95d1342 Cr-Commit-Position: refs/heads/master@{#333649}
Message was sent while issue was closed.
@Scott Hess: I noticed today that it's already working in Chrome Canary. Thx for your patience and all your help. |
