Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
6193003:
Get rid of FromWStringHack in select_file_dialog.cc (Closed)
|
Created:
9 years, 11 months ago by tfarina Modified:
9 years, 7 months ago CC:
chromium-reviews, darin-cc_chromium.org, jam, brettw-cc_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionGet rid of FromWStringHack in select_file_dialog.cc
BUG=24672
TEST=trybots
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=71314
Patch Set 1 #Patch Set 2 : #Patch Set 3 : #
Total comments: 1
Patch Set 4 : tony review #
Total comments: 2
Patch Set 5 : os guards #
Total comments: 2
Patch Set 6 : #else #Messages
Total messages: 14 (0 generated)
Please, take a look.
I like this, but I worry that we need Value to match what JSON can hold for some reason. Tony would know.
The code to create TYPE_PATH isn't called when you run base::JSONReader::Read (it'll just read the path as a string) so I don't think this will work. In general, I don't think we should be adding more types to |Value| since the JSON reader can't read this properly. Do we know the encoding of the filename before it gets put into the json file? The proper way to do this on Linux would be to use SysWideToNativeMB to encode the filename using the filesystem encoding. A simpler approach would be to just use UTF8 since it seems like most Linux distros use UTF8.
On 2011/01/11 22:38:27, tony wrote: > The code to create TYPE_PATH isn't called when you run base::JSONReader::Read > (it'll just read the path as a string) so I don't think this will work. > > In general, I don't think we should be adding more types to |Value| since the > JSON reader can't read this properly. > > Do we know the encoding of the filename before it gets put into the json file? > The proper way to do this on Linux would be to use SysWideToNativeMB to encode > the filename using the filesystem encoding. A simpler approach would be to just > use UTF8 since it seems like most Linux distros use UTF8. What you recommend in this case? Which other way I can remove FromWStrinHack without adding FilePathValue?
http://codereview.chromium.org/6193003/diff/5001/chrome/browser/ui/views/sele... File chrome/browser/ui/views/select_file_dialog.cc (left): http://codereview.chromium.org/6193003/diff/5001/chrome/browser/ui/views/sele... chrome/browser/ui/views/select_file_dialog.cc:266: FilePath path = FilePath::FromWStringHack(UTF8ToWide(path_string)); Evan, thinking on it again, can't I just do: FilePath path(UTF8ToWide(path_string)); Since this is Windows only?
On 2011/01/12 01:30:21, tfarina wrote: > http://codereview.chromium.org/6193003/diff/5001/chrome/browser/ui/views/sele... > File chrome/browser/ui/views/select_file_dialog.cc (left): > > http://codereview.chromium.org/6193003/diff/5001/chrome/browser/ui/views/sele... > chrome/browser/ui/views/select_file_dialog.cc:266: FilePath path = > FilePath::FromWStringHack(UTF8ToWide(path_string)); > Evan, thinking on it again, can't I just do: > FilePath path(UTF8ToWide(path_string)); > > Since this is Windows only? Views is used by ChromeOS too. You can read the value as UTF8 then use an #if. For the windows case, you can convert to wide. For the linux/mac case, you can use base::SysWideToNativeMB(base::SysUTF8ToWide(...)). See chrome/browser/download/download_util.cc for an example of this. In the long run, since we don't want to keep using wstrings, we need to add SysString16ToNativeMB.
On 2011/01/12 21:29:16, tony wrote: > On 2011/01/12 01:30:21, tfarina wrote: > > > http://codereview.chromium.org/6193003/diff/5001/chrome/browser/ui/views/sele... > > File chrome/browser/ui/views/select_file_dialog.cc (left): > > > > > http://codereview.chromium.org/6193003/diff/5001/chrome/browser/ui/views/sele... > > chrome/browser/ui/views/select_file_dialog.cc:266: FilePath path = > > FilePath::FromWStringHack(UTF8ToWide(path_string)); > > Evan, thinking on it again, can't I just do: > > FilePath path(UTF8ToWide(path_string)); > > > > Since this is Windows only? > > Views is used by ChromeOS too. > Ah, I forgot it, yeah, you are right. > You can read the value as UTF8 then use an #if. For the windows case, you can > convert to wide. For the linux/mac case, you can use > base::SysWideToNativeMB(base::SysUTF8ToWide(...)). See > chrome/browser/download/download_util.cc for an example of this. > PTAL! > In the long run, since we don't want to keep using wstrings, we need to add > SysString16ToNativeMB. Yup, sounds good.
http://codereview.chromium.org/6193003/diff/14001/chrome/browser/ui/views/sel... File chrome/browser/ui/views/select_file_dialog.cc (right): http://codereview.chromium.org/6193003/diff/14001/chrome/browser/ui/views/sel... chrome/browser/ui/views/select_file_dialog.cc:268: base::SysWideToNativeMB(base::SysUTF8ToWide(path_string))); This won't work on Windows, will it?. FilePath only takes a std::wstring but SysWideToNativeMB returns std::string. That's why I suggested using an #if. You don't need to call SysWideToNativeMB on Windows.
http://codereview.chromium.org/6193003/diff/14001/chrome/browser/ui/views/sel... File chrome/browser/ui/views/select_file_dialog.cc (right): http://codereview.chromium.org/6193003/diff/14001/chrome/browser/ui/views/sel... chrome/browser/ui/views/select_file_dialog.cc:268: base::SysWideToNativeMB(base::SysUTF8ToWide(path_string))); On 2011/01/13 00:18:17, tony wrote: > This won't work on Windows, will it?. FilePath only takes a std::wstring but > SysWideToNativeMB returns std::string. That's why I suggested using an #if. > You don't need to call SysWideToNativeMB on Windows. Yeah, we sorta need a FilePath::FromWStringNotAHackJustThisOnce
On 2011/01/13 00:19:23, evanm wrote: > http://codereview.chromium.org/6193003/diff/14001/chrome/browser/ui/views/sel... > File chrome/browser/ui/views/select_file_dialog.cc (right): > > http://codereview.chromium.org/6193003/diff/14001/chrome/browser/ui/views/sel... > chrome/browser/ui/views/select_file_dialog.cc:268: > base::SysWideToNativeMB(base::SysUTF8ToWide(path_string))); > On 2011/01/13 00:18:17, tony wrote: > > This won't work on Windows, will it?. FilePath only takes a std::wstring but > > SysWideToNativeMB returns std::string. That's why I suggested using an #if. > > You don't need to call SysWideToNativeMB on Windows. > > Yeah, we sorta need a FilePath::FromWStringNotAHackJustThisOnce I'm feeling like this, this is pretty hacky. There should be a better way to do this (but it seems we don't have other way right now :( ).
Evan, Tony, added the OS guards. PTAL!
http://codereview.chromium.org/6193003/diff/21001/chrome/browser/ui/views/sel... File chrome/browser/ui/views/select_file_dialog.cc (right): http://codereview.chromium.org/6193003/diff/21001/chrome/browser/ui/views/sel... chrome/browser/ui/views/select_file_dialog.cc:269: #elif defined(OS_CHROMEOS) || (TOOLKIT_VIEWS) I think you meant defined(TOOLKIT_VIEWS) on the right side. But I'd say instead just use a plain "#else", less chance of a mistake.
http://codereview.chromium.org/6193003/diff/21001/chrome/browser/ui/views/sel... File chrome/browser/ui/views/select_file_dialog.cc (right): http://codereview.chromium.org/6193003/diff/21001/chrome/browser/ui/views/sel... chrome/browser/ui/views/select_file_dialog.cc:269: #elif defined(OS_CHROMEOS) || (TOOLKIT_VIEWS) On 2011/01/13 00:58:54, evanm wrote: > I think you meant > defined(TOOLKIT_VIEWS) > on the right side. > > But I'd say instead just use a plain "#else", less chance of a mistake. Oh, yeah, got distracted. Using #else now. PTAL!
LG |
