Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
4883003:
Add FilePath::FinalExtension() to avoid double extensions (.tar.gz) for file selector (Closed)
|
Created:
10 years, 1 month ago by davidben Modified:
7 years ago CC:
chromium-reviews, pam+watch_chromium.org, ben+cc_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdd FilePath::FinalExtension() to avoid double extensions (.tar.gz) for file selector
Windows and OS X file selectors break badly on double-extensions. GTK and
Chrome OS handle it slightly better, but CrOS has a slight bug and GTK's
handling of file extensions is minimal, so not a whole lot changed. See
https://codereview.chromium.org/4883003/#msg14 for full analysis of
platform behavior.
There is some logic that does benefit from long extensions (renaming "foo.tar.gz"
to "foo (1).tar.gz" instead of "foo.tar (1).gz"), and some other callers store
state based on extension, so rather than changing FilePath::Extension, add a
new FilePath::FinalExtension and change SelectFileDialog callers to use it.
Also work around a problem in NSSavePanel when saving "foo.tar.gz" with
extensions hidden.
TEST=FilePath.Extension,
FilePath.Extension2
FilePath.RemoveExtension
Enabling "Ask where to save each file before downloading"; saving a tar.gz doesn't result in weird confirmation prompt on OS X, with or without "Show all filename extensions" enabled in Finder.
BUG=83084
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=239505
Patch Set 1 #Patch Set 2 : #
Total comments: 3
Patch Set 3 : FilePath::FinalExtension #Patch Set 4 : Rebase, work around new Mac problem. #
Total comments: 4
Patch Set 5 : Comment (also a rebase) #
Messages
Total messages: 30 (0 generated)
Right, I had this patch I was sitting on for ages and forgot about. Not sure who the right reviewers are, but... asanka: you commented on the bug thakis: OWNERS
On 2012/11/13 18:37:14, David Benjamin wrote: > Right, I had this patch I was sitting on for ages and forgot about. Not sure who > the right reviewers are, but... > > asanka: you commented on the bug > thakis: OWNERS I'm not a Mac expert so I'll defer to Nico. What's the resulting behavior? If I try to save foo.tar.gz, would I be prompted for foo.gz?
I think avi's been looking at this code somewhat recently, he's probably the best reviewer.
On 2012/11/13 19:38:21, asanka wrote: > What's the resulting behavior? If I try to save foo.tar.gz, would I be prompted > for foo.gz? The current behavior is that if you try to save as foo.tar.gz (and enabled prompting for download location), you get a prompt when you close the save dialog: You have used the extension “.gz” at the end of the name. The standard extension is “.tar.gz”. and if you tell it to use the "standard extension", you get foo.tar.tar.gz. Looks like NSSavePanel is extracting only the last component to compare the user-selected file extension against the expected one, and "gz" doesn't match "tar.gz". With the patch, an expected extension of "tar.gz" turns into "gz", so the prompt goes away. It won't notice if you give it foo.bogus.gz, but I don't think NSSavePanel can handle those. Though I just checked foo.tar.bogus, and it seems to append gz automatically instead of giving me the equivalent prompt before. So I'm somewhat confused about that one. Maybe that's supposed to happen with default_extension? I'm not familiar enough with the normal behavior of the Mac save dialog.
In any case, it's kinda bogus that this code is being passed an "extension" of ".tar.gz". That's not an extension; that's two. https://chromiumcodereview.appspot.com/4883003/diff/2003/ui/base/dialogs/sele... File ui/base/dialogs/select_file_dialog_mac.mm (right): https://chromiumcodereview.appspot.com/4883003/diff/2003/ui/base/dialogs/sele... ui/base/dialogs/select_file_dialog_mac.mm:42: return ext.substr(last_dot + 1); Isn't there something on FilePath that would do this? https://chromiumcodereview.appspot.com/4883003/diff/2003/ui/base/dialogs/sele... ui/base/dialogs/select_file_dialog_mac.mm:234: // NSSavePanel cannot handle extensions like tar.gz, so we It's not an NSSavePanel issue, but a UTI issue; a UTI cannot represent more than one extension.
https://chromiumcodereview.appspot.com/4883003/diff/2003/ui/base/dialogs/sele... File ui/base/dialogs/select_file_dialog_mac.mm (right): https://chromiumcodereview.appspot.com/4883003/diff/2003/ui/base/dialogs/sele... ui/base/dialogs/select_file_dialog_mac.mm:42: return ext.substr(last_dot + 1); On 2012/11/13 20:41:00, Avi wrote: > Isn't there something on FilePath that would do this? There is FilePath::Extension() and friends, but it's the thing producing the double-extensions in the first place. Maybe it shouldn't do that on OS X? That would change a lot of things though. http://code.google.com/p/chromium/source/search?q=kCommonDoubleExtensionSuffixes
+Mark Mark, this is a question about FilePath functionality, where it says that the "extension" is ".tar.gz" and the like. It looks like different parts of the code want different things; is this something we want to handle in FilePath?
I really wish FilePath didn’t have this double-extension logic to begin with. I think adding FinalExtension to FilePath is fine. It’s also a good idea to rename the thing in FilePath to make it clear that it returns more than one extension in some cases.
I'm not thrilled with what FilePath does either, but at least we should put this filename manipulation there than here.
On 2012/11/13 21:22:13, Avi wrote: > I'm not thrilled with what FilePath does either, but at least we should put this > filename manipulation there than here. Okay. There's one nuisance that SelectFileDialog never sees the FilePath. It sees a FilePath::StringType (via FileTypeInfo). The caller (someone in the downloads code) calls FilePath::Extension. Looks like Linux and Chrome OS are fine with the double-extension. Windows seems to get confused, but I haven't verified it's the same issue and not something else. Maybe it's not worth keeping it at all for the file selector? If we still want it for some platforms, a platform-specific ifdef in the downloads code is probably also objectionable. I could plumb through a SelectFileDialog::ExtensionFromFilePath or something to pick which of Extension or FinalExtension to call. Another possibility is to keep this logic in select_file_dialog_mac.mm (and I guess the Windows one too) and instead move the LastExtension function in this patch to a static function of FilePath.
:( The ideal solution would be to have FinalExtension (not thrilled with the naming btw) passed in on all platforms and verified to work. When was Extension modified to play the double-extension game? Surely that post-dates the save dialog, and I bet this would make the Windows code happier. And while the whole ".tar.gz" extension thing was introduced for Linux, was it introduced for the save dialogs or just for file naming in the downloads code? I know it's work, but it would be awesome if you could verify on all the platforms.
On 2012/11/14 17:55:48, Avi wrote: > :( > > The ideal solution would be to have FinalExtension (not thrilled with the naming > btw) passed in on all platforms and verified to work. Yeah, me neither. something like FullExtension/Extension or LongExtension/Extension would probably be better, but that'd require renaming every extension-related method. :-) (Or at least checking over all their callers to see if they should be the short one anyway.) Perhaps Extension/ShortExtension? > When was Extension > modified to play the double-extension game? Surely that post-dates the save > dialog, and I bet this would make the Windows code happier. And while the whole > ".tar.gz" extension thing was introduced for Linux, was it introduced for the > save dialogs or just for file naming in the downloads code? Looks like it was added here. Primarily to handle rename I think? +estade http://codereview.chromium.org/3018011/ > I know it's work, but it would be awesome if you could verify on all the > platforms. Sure. May take me a bit though; Windows dev environment is setting up. We'll see how much doing that in a VM will make me hate myself. :-)
On 2012/11/15 04:23:26, David Benjamin wrote: > something like FullExtension/Extension or > LongExtension/Extension would probably be better, but that'd require renaming > every extension-related method. I wouldn't mind that at all. Of course, I wouldn't be doing the work. :) Then again, having a separate CL that renames the world would be just fine. I'm the master of massive renames. > May take me a bit though You can ask for help from platform experts, especially for the more obscure ones.
On 2012/11/15 05:31:32, Avi wrote: > On 2012/11/15 04:23:26, David Benjamin wrote: > > something like FullExtension/Extension or > > LongExtension/Extension would probably be better, but that'd require renaming > > every extension-related method. > > I wouldn't mind that at all. Of course, I wouldn't be doing the work. :) > > Then again, having a separate CL that renames the world would be just fine. I'm > the master of massive renames. > > > May take me a bit though > > You can ask for help from platform experts, especially for the more obscure > ones. Well, Windows is certainly a very obscure platform. :-) Finally got that set up. I tested Windows with and without an equivalent of this patch, and that was indeed the cause of the problem. If you give it ".tar.gz", Windows thinks the provided filename of "blah.tar.gz" doesn't have a matching extension and converts to "blah.tar.gz.tar.gz". I've gotten it to append the extension several times too, so it gets pretty seriously confused. (It probably assumes the fixup routine is idempotent.) With an equivalent fix, it acts as you'd expect... the default extension hiding setting hides just the gz, so it looks a little odd, but not much can be done there. GTK handles it fine. It also, as far as I know, doesn't have any logic to correct file extensions or anything, so there's not much to handling it. I haven't built it with an equivalent patch to see what breaks if you tell it ".gz". I assume only that the filter will be less strict than it should. ChromeOS seems to also handle it mostly fine. There is one bug where it actually computes the human-readable name of the filter entry (you have to click the dropdown; defaults to "All files") wrong. If you give it ".foo.bz2", it says "BZ2 file" even though it filters to "*.foo.bz2". Source-diving, it's reusing the function to compute a filetype description for filename which assumes only the last component. (It passes in ".foo.bz2" as the filename.) That's easy enough to fix. Interestingly, if you give it ".tar.bz2", it actually writes "Bzip2 compressed tar archive" since it's got a regex for /\.tar\.bz2$/, so at some level it's "expecting" the double-extension. (It's also using the singular form which is a little weird... another artifact of using the same code to compute the type of a particular file.) See src/chrome/browser/resources/file_manager/js/file_type.js. GTK, ChromeOS, and OS X all provide a feature where (when relevant extension hiding settings are off), it highlights only the base name of the file so you don't overwrite the extension. In all three, only the last component is checked. But I don't think this is a function of the extension passed in here. Windows doesn't do this, so N/A. So, yeah. We definitely want to be passing ".gz" to Windows and OS X. Passing only single extensions to GTK I think only loses the more accurate filtering and text in the dropdown says ".gz files" or so instead of ".tar.gz files". It will work around one bug in Chrome OS, but that can also just be fixed in the Chrome OS file manager. It will also lose the more accurate filtering and the human-readable descriptions for .tar.gz and .tar.bz2. (I see no other double extensions in file_type.js.) After saving the file, viewing it in the file manager will still show the human-readable one in the Type column... I think that description is primarily for it. If we don't want to lose those in Linux/CrOS, one possibility is to add FilePath::PlatformExtension which will allow single extensions or not based on whether the platform natively supports them. Anything that passes the extension down to platform code should use that version. Though in some cases (ChromeOS), it's not really clear what that means. I think just passing single extensions everywhere is pretty sensible. Seems more complexity than is worth it, and double extensions are just weird. Arguably no one completely "supports" it, given the highlighting thing. If there aren't too many, I'll maybe skim over all the uses of Extension(), RemoveExtension(), and friends, and see how many of them actually want double-extensions. Perhaps we can just remove this logic from FilePath altogether and confine it to the "foo.tar.gz" -> "foo (1).tar.gz" logic.
Wow. That is an excellent analysis. When you make your change, it would definitely be worth adding bits of this as appropriate in commentary.
> /\.tar\.bz2$/ Er, correction: that regex is actually /\.(tar.bz2|tbz|tbz2)$/i and so incorrectly matches foo.tarXbz2. :-)
On 2012/11/17 20:42:38, David Benjamin wrote: > > /\.tar\.bz2$/ > > Er, correction: that regex is actually /\.(tar.bz2|tbz|tbz2)$/i and so > incorrectly matches foo.tarXbz2. :-) hahaha awesome.
[I'm calling this long vs short extensions below just because that's what my
notes use. Not the best name, but yeah.]
Alright, I finally got around to looking into this again. I started trying to
audit all the callers of FilePath::Extension and friends (there're a number of
file extension methods). But that'll take a while, and some of the things were
tricky, so I went with just adding a FilePath::FinalExtension method. (Still
don't really like the name.) Also noticed a bit of dead code in the process, so
I killed it.
Of the calls that I did get to looking at, most didn't care one way or another
because they can never see long extensions. Some of the more interesting ones
were:
- The file selector, of course. As noted above, OS X and Windows need short.
CrOS and GTK can deal with long but don't do much with it and CrOS has a minor
bug. I didn't check the KDE integration code.
- base/native_library_mac.mm in NativeLoadLibrary. This isn't actually an
interesting case, but library_path.Extension() can never return "dylib", only
".dylib", so I'm not sure what's the deal with that check. :-)
- file_util::GetUniquePathNumber. This wants the long one so that renaming
"foo.tar.gz" is right. It also has kind of an obnoxious API since it just
returns the int and the caller is required to InsertBeforeExtension again. :-)
chrome/browser/chromeos/drive/drive_files.cc also has a similar reimplementation
of the same logic but with Drive.
- The Drive code peels off a file extension to query for installed web apps that
handle that extension. I imagine that should be short, but I don't know what
extensions Drive lets you register.
- There's something about mounting archives in CrOS. I assume that wants the
long form since it handles tarballs and currently works with the long form.
- There's code in Chrome OS for associating default tasks for extensions.
Changing this could be annoying in that there is persistent state computed based
on potentially long extensions. I suspect it's not a big deal, but I don't know
the code.
- The Chrome OS wallpaper code is affected, sort of. No one in their right mind
would ever name a PNG or JPEG file "pretty-picture.tar.gz". But maybe someone
did dumb things with content types? I didn't look too closely at this code, but
it calls InsertBeforeExtension with a "_small" suffix and whatnot to save a
small version of the wallpaper. I didn't look closely enough to determine how
badly things might explode if "pretty-picture.tar.gz" ever got saved as a
wallpaper. (Maybe less contrived, "pretty-picture.png.gz" and the web server
treated it as a pre-gzipped image to send with gzip compression. Why you would
gzip a png, I don't know.)
- There are a few places that do path.Extension() == ".blah" which could use
path.MatchesExtension(".blah"). Although that's a case-insensitive match, so
there's some behavior change.
There are LOTS more calls, so this is certainly not a comprehensive list of all
the relevant code. Most of my examples are CrOS and Drive because I'm going
through them in alphabetical order. :-)
So yeah, I don't know what we want to do with this. Ideally, I think
FilePath::Extension() should return the short version. We have code that uses
the extension methods as convenience functions for dot-separated things.
(Drive's cache files are something.${MD5SUM} and sometimes end with an extra
.mounted or something...) And for uses like that, magic is bad. (And in general,
really...) But getting there might be a nuisance. For this update I tried to
avoid affecting other code.
(Sigh. Something this mundane really doesn't deserve to be so complicated.)
Code LGTM. The audit you mention would still be nice.
Now that I'm actually working here again, I should close the loop on some of these CLs that I kept sitting on as a student. The failure modes here seem to have gotten worse on Mavericks. Uploaded a new version of the CL: - Rebased to master - Remove .user.js from dangerous file extensions, now that it can just ignore double extensions and check for all *.js files. - Work around a NSSavePanel quirk on 10.8 I just noticed (and add FilePath::RemoveFinalExtension for it): Even after passing "gz" instead of "tar.gz" to NSSavePanel, if you have "Show all filename extensions" off in Finder, or "Hide extension" on in the panel, saving "foo.tar.gz" confuses things. It thinks you're rejecting the default extension of .gz and replacing with .tar. You get a prompt and can either save as "foo.tar" (wrong) or "foo.gz" (also wrong). Interestingly, this only happens when the extension is one that OS X recognizes somehow. "foo.bar.gz" doesn't trigger this problem I've added a workaround to remove the "Hide extension" button and never hide extensions in this case. I don't know how to detect if the penultimate extension was one that OS X recognized, so it's just checking if the length is at most 5, to mirror the double-extension logic. We could also maybe just unconditionally show the extensions and be consistent. Looking at other browsers, Firefox never sets allowed types on save panels, only open[0]. The result is that file extensions aren't hidden and the entire filename is highlighted, rather than the portion without the extension. Safari's a little more complicated. They don't have a prompt on save option, but you can right-click and "Download Linked File As...". That prompt looks like Firefox's (minus the dummy dropdown), so I think they're not setting allowed types either. BUT if you save an image or HTML file it's already loaded, it does set allowed types and shows the "Hide extension" checkbox. Tarballs tend not to load as images, so this avoids our problem, but you can still trigger if you try[1]. For instance, try to save: https://davidben.net/ball.of.tar.png (The workaround in the CL isn't quite what Safari and Firefox do in the unknown-file case, but we do get to keep the allowed file types setting this way, instead of dropping it all on the floor, if that's of any use.) I have not yet tested these changes on 10.9; only upgraded my personal machine. [0] Interestingly, they do set an accessory view with a format dropdown, but it isn't hooked up to anything in the save case. http://dxr.mozilla.org/mozilla-central/source/widget/cocoa/nsFilePicker.mm#l450 [1] Well, it's ALMOST the same as our problem. Instead of "Use .png" and "Use .tar" options, Safari gives me "Use .png" and "Use both" options, with "Use .png" as the default. Not sure what's different here.
Well, this was reviewed in the past, and I liked it then. What’s changed, or is this just a rebase now that you’re back?
On 2013/12/03 22:35:11, Mark Mentovai wrote: > Well, this was reviewed in the past, and I liked it then. What’s changed, or is > this just a rebase now that you’re back? Mostly just that I'm back and I figured landing something based on a 9 month old review would be a bit much. :-) There is one change though. Apparently the original change wasn't enough to deal with all of NSSavePanel's quirks on 10.8. select_file_dialog_mac.mm has yet another workaround in it. See comment #20.
LGTM. Good use of “penultimate.”
lgtm, feel free to ignore: https://codereview.chromium.org/4883003/diff/32002/ui/shell_dialogs/select_fi... File ui/shell_dialogs/select_file_dialog_mac.mm (right): https://codereview.chromium.org/4883003/diff/32002/ui/shell_dialogs/select_fi... ui/shell_dialogs/select_file_dialog_mac.mm:272: // is trying to override the default extension. This with s/This/This happens/ https://codereview.chromium.org/4883003/diff/32002/ui/shell_dialogs/select_fi... ui/shell_dialogs/select_file_dialog_mac.mm:278: penultimate_extension.length() <= 5U) { s/5/strlen(".foo.")/?
https://codereview.chromium.org/4883003/diff/32002/ui/shell_dialogs/select_fi... File ui/shell_dialogs/select_file_dialog_mac.mm (right): https://codereview.chromium.org/4883003/diff/32002/ui/shell_dialogs/select_fi... ui/shell_dialogs/select_file_dialog_mac.mm:272: // is trying to override the default extension. This with On 2013/12/06 21:36:36, Nico wrote: > s/This/This happens/ Done. https://codereview.chromium.org/4883003/diff/32002/ui/shell_dialogs/select_fi... ui/shell_dialogs/select_file_dialog_mac.mm:278: penultimate_extension.length() <= 5U) { On 2013/12/06 21:36:36, Nico wrote: > s/5/strlen(".foo.")/? It shouldn't include dots. It's just what the double-extension logic uses for "does the part before .gz look like a file extension". I'm not actually sure what the condition for this bug is... foo.bar.gz doesn't cause problems, while foo.tar.gz does. I suspect what we actually want is "does OS X recognize this as a file extension?". (Disclaimer: I'm ignorant of how OS X's file types actually work.) Alternatively, we can just always [dialog setExtensionHidden:NO] which is what Firefox does anyway. Possibly that's better than sometimes doing it and sometimes not. Safari does it based on whether you're downloading a file or doing Cmd-S on something displayed in the browser, possibly because they know the mimetype in the latter case, but not the former. But that breaks down if you load https://davidben.net/ball.of.tar.png into the browser and Cmd-S.
/download/ lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/davidben@chromium.org/4883003/122002
The commit queue went berserk retrying too often for a seemingly flaky test on builder android_dbg: http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/davidben@chromium.org/4883003/122002
Message was sent while issue was closed.
Change committed as 239505 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
