Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1867533002:
(TOBEDELETED)Select downloaded file in the folder. (Closed)
|
Created:
4 years, 8 months ago by maksims (do not use this acc) Modified:
4 years, 7 months ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdded checks: if nautilus is default directory browser and its version is >= 3.0.2, then open file with nautilus. Otherwise old method is used.
Nautilus bug report: https://bugzilla.gnome.org/show_bug.cgi?id=632427
Nautilus fix: https://git.gnome.org/browse/nautilus/commit/?id=a96761350ad422ec084c12f649562f9a2a83963d
Nautilus change log: http://ftp.gnome.org/pub/GNOME/sources/nautilus/3.0/nautilus-3.0.2.changes
BUG=352988
Patch Set 1 #
Total comments: 13
Patch Set 2 : #
Total comments: 27
Patch Set 3 : Lel Zhang comments #
Total comments: 7
Patch Set 4 : minor fixes according to review #
Total comments: 10
Patch Set 5 : #
Total comments: 1
Patch Set 6 : whitespace trimming #
Total comments: 4
Patch Set 7 : review fixes #
Total comments: 3
Messages
Total messages: 49 (14 generated)
Description was changed from ========== Select downloaded file in the folder. BUG= ========== to ========== Adding possibility to check if nautilus is the default directory browser. If yes, use it in order to be able to highlight the downloaded file in opened folder. If nautilus is not installed, use old method. I have not checked other directory browsers that support selection yet. But let's count this patch as a first improvement. BUG=352988 ==========
maksim.sisov@intel.com changed reviewers: + thakis@chromium.org
please take a look
maksim.sisov@intel.com changed reviewers: + estade@chromium.org, mikhail.pozdnyakov@intel.com, thestig@chromium.org
The CQ bit was checked by maksim.sisov@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1867533002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1867533002/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: No L-G-T-M from a valid reviewer yet. CQ run can only be started by full committers or once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
The CQ bit was unchecked by maksim.sisov@intel.com
please take a look
https://codereview.chromium.org/1867533002/diff/1/chrome/browser/platform_uti... File chrome/browser/platform_util.cc (left): https://codereview.chromium.org/1867533002/diff/1/chrome/browser/platform_uti... chrome/browser/platform_util.cc:31: if (base::DirectoryExists(path) != (type == OPEN_FOLDER)) { why this change? https://codereview.chromium.org/1867533002/diff/1/chrome/browser/platform_uti... File chrome/browser/platform_util_linux.cc (right): https://codereview.chromium.org/1867533002/diff/1/chrome/browser/platform_uti... chrome/browser/platform_util_linux.cc:51: void XDGMimeGetDefaultDirectoryBrowser(std::string* cmdline_output) { return std::string? https://codereview.chromium.org/1867533002/diff/1/chrome/browser/platform_uti... chrome/browser/platform_util_linux.cc:78: DLOG(ERROR) << "PRE"; is this for debug? https://codereview.chromium.org/1867533002/diff/1/chrome/browser/platform_uti... chrome/browser/platform_util_linux.cc:91: if(output.find("nautilus") != std::string::npos) { what happens if you just do this regardless of what the file browser is? https://codereview.chromium.org/1867533002/diff/1/chrome/browser/platform_uti... chrome/browser/platform_util_linux.cc:95: else formatting is off https://codereview.chromium.org/1867533002/diff/1/chrome/browser/platform_uti... chrome/browser/platform_util_linux.cc:111: OpenItem(profile, full_path, OPEN_FOLDER, OpenOperationCallback()); indent is off
On 2016/04/07 16:58:59, Evan Stade wrote: > https://codereview.chromium.org/1867533002/diff/1/chrome/browser/platform_uti... > File chrome/browser/platform_util.cc (left): > > https://codereview.chromium.org/1867533002/diff/1/chrome/browser/platform_uti... > chrome/browser/platform_util.cc:31: if (base::DirectoryExists(path) != (type == > OPEN_FOLDER)) { > why this change? > Because if the file is directory, S_ISDIR returns non-zero, right? Thus, it is True. Then type == OPEN_FOLDER is true as well, because we are trying to open a folder. TRUE != TRUE. Right? If I leave this line there, only files can be opened, but there is no action when "open in folder" button is pressed. Only downloaded files can be opened otherwise. > https://codereview.chromium.org/1867533002/diff/1/chrome/browser/platform_uti... > File chrome/browser/platform_util_linux.cc (right): > > https://codereview.chromium.org/1867533002/diff/1/chrome/browser/platform_uti... > chrome/browser/platform_util_linux.cc:51: void > XDGMimeGetDefaultDirectoryBrowser(std::string* cmdline_output) { > return std::string? > May be better to return. > https://codereview.chromium.org/1867533002/diff/1/chrome/browser/platform_uti... > chrome/browser/platform_util_linux.cc:78: DLOG(ERROR) << "PRE"; > is this for debug? > Ops, sorry. > https://codereview.chromium.org/1867533002/diff/1/chrome/browser/platform_uti... > chrome/browser/platform_util_linux.cc:91: if(output.find("nautilus") != > std::string::npos) { > what happens if you just do this regardless of what the file browser is? > Do you mean opening a file in folder with nautilus regardless of what was found? > https://codereview.chromium.org/1867533002/diff/1/chrome/browser/platform_uti... > chrome/browser/platform_util_linux.cc:95: else > formatting is off > Ops. > https://codereview.chromium.org/1867533002/diff/1/chrome/browser/platform_uti... > chrome/browser/platform_util_linux.cc:111: OpenItem(profile, full_path, > OPEN_FOLDER, OpenOperationCallback()); > indent is off Ops.
On 2016/04/11 05:55:17, maksims wrote: > On 2016/04/07 16:58:59, Evan Stade wrote: > > > https://codereview.chromium.org/1867533002/diff/1/chrome/browser/platform_uti... > > chrome/browser/platform_util.cc:31: if (base::DirectoryExists(path) != (type > == > > OPEN_FOLDER)) { > > why this change? > > > > Because if the file is directory, S_ISDIR returns non-zero, right? Thus, it is > True. Then type == OPEN_FOLDER is true as well, because we are trying to open a > folder. > TRUE != TRUE. Right? If I leave this line there, only files can be opened, but > there is no action when "open in folder" button is pressed. Only downloaded > files can be opened otherwise. I'm sorry, I don't understand this paragraph. Can you try again? Also, please respond to code review comments with more code review comments, not inline as you have done. This helps keeps track of that various conversations. > https://codereview.chromium.org/1867533002/diff/1/chrome/browser/platform_uti... > > chrome/browser/platform_util_linux.cc:91: if(output.find("nautilus") != > > std::string::npos) { > > what happens if you just do this regardless of what the file browser is? > > > Do you mean opening a file in folder with nautilus regardless of what was found? No, I meant OpenFileInDefaultDirectoryBrowser(output, ...);
maksim.sisov@intel.com changed reviewers: - mikhail.pozdnyakov@intel.com
please take a look https://codereview.chromium.org/1867533002/diff/1/chrome/browser/platform_uti... File chrome/browser/platform_util.cc (left): https://codereview.chromium.org/1867533002/diff/1/chrome/browser/platform_uti... chrome/browser/platform_util.cc:31: if (base::DirectoryExists(path) != (type == OPEN_FOLDER)) { On 2016/04/07 16:58:58, Evan Stade wrote: > why this change? Because OpenItem() function used to send path.DirName(), but now it sends full path with the file name inside. OpenItem(profile, full_path.DirName(), OPEN_FOLDER,OpenOperationCallback()); -> OpenItem(profile, full_path, OPEN_FOLDER,OpenOperationCallback()); If there is (base::DirectoryExists(path) != (type == OPEN_FOLDER), then OPEN_FAILED_INVALID_TYPE is called. https://codereview.chromium.org/1867533002/diff/1/chrome/browser/platform_uti... File chrome/browser/platform_util_linux.cc (right): https://codereview.chromium.org/1867533002/diff/1/chrome/browser/platform_uti... chrome/browser/platform_util_linux.cc:78: DLOG(ERROR) << "PRE"; On 2016/04/07 16:58:59, Evan Stade wrote: > is this for debug? Yes https://codereview.chromium.org/1867533002/diff/1/chrome/browser/platform_uti... chrome/browser/platform_util_linux.cc:91: if(output.find("nautilus") != std::string::npos) { On 2016/04/07 16:58:59, Evan Stade wrote: > what happens if you just do this regardless of what the file browser is? if there is no nautilus, nothing happens.
https://codereview.chromium.org/1867533002/diff/1/chrome/browser/platform_uti... File chrome/browser/platform_util.cc (left): https://codereview.chromium.org/1867533002/diff/1/chrome/browser/platform_uti... chrome/browser/platform_util.cc:31: if (base::DirectoryExists(path) != (type == OPEN_FOLDER)) { On 2016/04/14 08:30:33, maksims wrote: > On 2016/04/07 16:58:58, Evan Stade wrote: > > why this change? > > Because OpenItem() function used to send path.DirName(), but now it sends full > path with the file name inside. > > OpenItem(profile, full_path.DirName(), OPEN_FOLDER,OpenOperationCallback()); -> > OpenItem(profile, full_path, OPEN_FOLDER,OpenOperationCallback()); > > If there is (base::DirectoryExists(path) != (type == OPEN_FOLDER), then > OPEN_FAILED_INVALID_TYPE is called. Sorry, but VerifyAndOpenItemOnBlockingThread(), basically OpenItem()'s implementation, is used in many places. You can't just change it to suit your own needs without considering the other callers. Likely ShowItemInFolder() can't just simply reply on OpenItem() anymore. I'd suggest ShowItemInFolder() first try to figure out if the file manager is Nautilus. If it is not, then do the current behavior and ShowItemInFolder() for the folder. For Nautilus, do something else. https://codereview.chromium.org/1867533002/diff/1/chrome/browser/platform_uti... File chrome/browser/platform_util_linux.cc (right): https://codereview.chromium.org/1867533002/diff/1/chrome/browser/platform_uti... chrome/browser/platform_util_linux.cc:78: DLOG(ERROR) << "PRE"; On 2016/04/14 08:30:33, maksims wrote: > On 2016/04/07 16:58:59, Evan Stade wrote: > > is this for debug? > > Yes We'd prefer if you removed it. https://codereview.chromium.org/1867533002/diff/1/chrome/browser/platform_uti... chrome/browser/platform_util_linux.cc:95: else On 2016/04/07 16:58:58, Evan Stade wrote: > formatting is off And you need curly braces.
https://codereview.chromium.org/1867533002/diff/1/chrome/browser/platform_uti... File chrome/browser/platform_util_linux.cc (right): https://codereview.chromium.org/1867533002/diff/1/chrome/browser/platform_uti... chrome/browser/platform_util_linux.cc:91: if(output.find("nautilus") != std::string::npos) { And what happens if nautilus is an older version that doesn't support selecting the file?
On 2016/04/15 08:23:13, Lei Zhang wrote: > https://codereview.chromium.org/1867533002/diff/1/chrome/browser/platform_uti... > File chrome/browser/platform_util_linux.cc (right): > > https://codereview.chromium.org/1867533002/diff/1/chrome/browser/platform_uti... > chrome/browser/platform_util_linux.cc:91: if(output.find("nautilus") != > std::string::npos) { > And what happens if nautilus is an older version that doesn't support selecting > the file? Ok, I got it. Will do another way. Haven't thought about that.
Description was changed from ========== Adding possibility to check if nautilus is the default directory browser. If yes, use it in order to be able to highlight the downloaded file in opened folder. If nautilus is not installed, use old method. I have not checked other directory browsers that support selection yet. But let's count this patch as a first improvement. BUG=352988 ========== to ========== Added checks: if nautilus is default directory browser and its version is >= 3.0.2, then open file with nautilus. Nautilus bug report: https://bugzilla.gnome.org/show_bug.cgi?id=632427 Nautilus fix: https://git.gnome.org/browse/nautilus/commit/?id=a96761350ad422ec084c12f64956... Nautilus change log: http://ftp.gnome.org/pub/GNOME/sources/nautilus/3.0/nautilus-3.0.2.changes BUG=352988 ==========
Description was changed from ========== Added checks: if nautilus is default directory browser and its version is >= 3.0.2, then open file with nautilus. Nautilus bug report: https://bugzilla.gnome.org/show_bug.cgi?id=632427 Nautilus fix: https://git.gnome.org/browse/nautilus/commit/?id=a96761350ad422ec084c12f64956... Nautilus change log: http://ftp.gnome.org/pub/GNOME/sources/nautilus/3.0/nautilus-3.0.2.changes BUG=352988 ========== to ========== Added checks: if nautilus is default directory browser and its version is >= 3.0.2, then open file with nautilus. Otherwise old method is used. Nautilus bug report: https://bugzilla.gnome.org/show_bug.cgi?id=632427 Nautilus fix: https://git.gnome.org/browse/nautilus/commit/?id=a96761350ad422ec084c12f64956... Nautilus change log: http://ftp.gnome.org/pub/GNOME/sources/nautilus/3.0/nautilus-3.0.2.changes BUG=352988 ==========
Patchset #2 (id:20001) has been deleted
Hope it's better now. Comments, suggestions, feedbacks are welcome!
https://codereview.chromium.org/1867533002/diff/40001/chrome/browser/platform... File chrome/browser/platform_util.h (right): https://codereview.chromium.org/1867533002/diff/40001/chrome/browser/platform... chrome/browser/platform_util.h:40: enum OpenItemType { OPEN_FILE, OPEN_FOLDER, SHOW_ITEM_IN_FOLDER }; You may want to make SHOW_ITEM_IN_FOLDER Linux-only. As is, it's not used on other platforms, but compilers may notice switch statements that handle an OpenItemType are forgetting to handle this enum value, and complain. https://codereview.chromium.org/1867533002/diff/40001/chrome/browser/platform... File chrome/browser/platform_util_linux.cc (right): https://codereview.chromium.org/1867533002/diff/40001/chrome/browser/platform... chrome/browser/platform_util_linux.cc:65: const std::string& path){ nit: space after ) https://codereview.chromium.org/1867533002/diff/40001/chrome/browser/platform... chrome/browser/platform_util_linux.cc:73: base::CommandLine nautilus_cl(base::FilePath("/usr/bin/nautilus")); Don't hard code /usr/bin/nautilus. It's also weird that you check /usr/bin/nautilus but you don't launch /usr/bin/nautilus. https://codereview.chromium.org/1867533002/diff/40001/chrome/browser/platform... chrome/browser/platform_util_linux.cc:78: std::char_traits<char>::length(kNautilusKey) + 1; Why not just strlen()? https://codereview.chromium.org/1867533002/diff/40001/chrome/browser/platform... chrome/browser/platform_util_linux.cc:82: found_version[found_version.length()-1] == '\n') { foo - 1 https://codereview.chromium.org/1867533002/diff/40001/chrome/browser/platform... chrome/browser/platform_util_linux.cc:91: const base::Version supported_version(kSupportedNautilusVersion); You can declare variables closer to where they are used. https://codereview.chromium.org/1867533002/diff/40001/chrome/browser/platform... chrome/browser/platform_util_linux.cc:94: base::CommandLine xdg_mime(base::FilePath("/usr/bin/xdg-mime")); Don't hard code /usr/bin/xdg-mime. Use whatever the user has in their PATH. https://codereview.chromium.org/1867533002/diff/40001/chrome/browser/platform... chrome/browser/platform_util_linux.cc:100: if (file_browser.find(kNautilusKey) != std::string::npos) { xdg-mime query default inode/directory is expected to return "nautilus.desktop" right? Why not just check for that? What if the user has some other software that just happens to have nautilus in the name? https://codereview.chromium.org/1867533002/diff/40001/chrome/browser/platform... chrome/browser/platform_util_linux.cc:112: if ((*file_browser).find(kNautilusKey) != std::string::npos) In general (*foo).bar() can be written as foo->bar(). https://codereview.chromium.org/1867533002/diff/40001/chrome/browser/platform... chrome/browser/platform_util_linux.cc:114: else Once again, when there are multi-line statements, the if and else blocks need braces around them. https://codereview.chromium.org/1867533002/diff/40001/chrome/browser/platform... chrome/browser/platform_util_linux.cc:148: std::string* output = new std::string; No need, just: base::PostTaskAndReplyWithResult( content::BrowserThread::GetBlockingPool(), FROM_HERE, base::Bind(Foo), base::Bind(...)); Where Foo() simply returns a std::string.
Patchset #3 (id:60001) has been deleted
PTAL https://codereview.chromium.org/1867533002/diff/40001/chrome/browser/platform... File chrome/browser/platform_util.h (right): https://codereview.chromium.org/1867533002/diff/40001/chrome/browser/platform... chrome/browser/platform_util.h:40: enum OpenItemType { OPEN_FILE, OPEN_FOLDER, SHOW_ITEM_IN_FOLDER }; On 2016/04/19 21:51:25, Lei Zhang wrote: > You may want to make SHOW_ITEM_IN_FOLDER Linux-only. As is, it's not used on > other platforms, but compilers may notice switch statements that handle an > OpenItemType are forgetting to handle this enum value, and complain. Done. https://codereview.chromium.org/1867533002/diff/40001/chrome/browser/platform... File chrome/browser/platform_util_linux.cc (right): https://codereview.chromium.org/1867533002/diff/40001/chrome/browser/platform... chrome/browser/platform_util_linux.cc:65: const std::string& path){ On 2016/04/19 21:51:25, Lei Zhang wrote: > nit: space after ) Done. https://codereview.chromium.org/1867533002/diff/40001/chrome/browser/platform... chrome/browser/platform_util_linux.cc:73: base::CommandLine nautilus_cl(base::FilePath("/usr/bin/nautilus")); On 2016/04/19 21:51:25, Lei Zhang wrote: > Don't hard code /usr/bin/nautilus. It's also weird that you check > /usr/bin/nautilus but you don't launch /usr/bin/nautilus. Done. https://codereview.chromium.org/1867533002/diff/40001/chrome/browser/platform... chrome/browser/platform_util_linux.cc:78: std::char_traits<char>::length(kNautilusKey) + 1; On 2016/04/19 21:51:25, Lei Zhang wrote: > Why not just strlen()? Done. Different people like different ways of doing that. Which method is better? https://codereview.chromium.org/1867533002/diff/40001/chrome/browser/platform... chrome/browser/platform_util_linux.cc:82: found_version[found_version.length()-1] == '\n') { On 2016/04/19 21:51:25, Lei Zhang wrote: > foo - 1 Spaces? https://codereview.chromium.org/1867533002/diff/40001/chrome/browser/platform... chrome/browser/platform_util_linux.cc:91: const base::Version supported_version(kSupportedNautilusVersion); On 2016/04/19 21:51:25, Lei Zhang wrote: > You can declare variables closer to where they are used. Done. https://codereview.chromium.org/1867533002/diff/40001/chrome/browser/platform... chrome/browser/platform_util_linux.cc:94: base::CommandLine xdg_mime(base::FilePath("/usr/bin/xdg-mime")); On 2016/04/19 21:51:25, Lei Zhang wrote: > Don't hard code /usr/bin/xdg-mime. Use whatever the user has in their PATH. Done. https://codereview.chromium.org/1867533002/diff/40001/chrome/browser/platform... chrome/browser/platform_util_linux.cc:100: if (file_browser.find(kNautilusKey) != std::string::npos) { On 2016/04/19 21:51:25, Lei Zhang wrote: > xdg-mime query default inode/directory is expected to return "nautilus.desktop" > right? Why not just check for that? What if the user has some other software > that just happens to have nautilus in the name? Done. Right my fault. https://codereview.chromium.org/1867533002/diff/40001/chrome/browser/platform... chrome/browser/platform_util_linux.cc:112: if ((*file_browser).find(kNautilusKey) != std::string::npos) On 2016/04/19 21:51:25, Lei Zhang wrote: > In general (*foo).bar() can be written as foo->bar(). Done. Depends how you like it. https://codereview.chromium.org/1867533002/diff/40001/chrome/browser/platform... chrome/browser/platform_util_linux.cc:114: else On 2016/04/19 21:51:25, Lei Zhang wrote: > Once again, when there are multi-line statements, the if and else blocks need > braces around them. Done. https://codereview.chromium.org/1867533002/diff/40001/chrome/browser/platform... chrome/browser/platform_util_linux.cc:148: std::string* output = new std::string; On 2016/04/19 21:51:25, Lei Zhang wrote: > No need, just: > > base::PostTaskAndReplyWithResult( > content::BrowserThread::GetBlockingPool(), > FROM_HERE, > base::Bind(Foo), > base::Bind(...)); > > Where Foo() simply returns a std::string. Done.
The CQ bit was checked by maksim.sisov@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1867533002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1867533002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: No L-G-T-M from a valid reviewer yet. CQ run can only be started by full committers or once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
https://codereview.chromium.org/1867533002/diff/40001/chrome/browser/platform... File chrome/browser/platform_util.h (right): https://codereview.chromium.org/1867533002/diff/40001/chrome/browser/platform... chrome/browser/platform_util.h:40: enum OpenItemType { OPEN_FILE, OPEN_FOLDER, SHOW_ITEM_IN_FOLDER }; On 2016/04/20 12:02:45, maksims wrote: > On 2016/04/19 21:51:25, Lei Zhang wrote: > > You may want to make SHOW_ITEM_IN_FOLDER Linux-only. As is, it's not used on > > other platforms, but compilers may notice switch statements that handle an > > OpenItemType are forgetting to handle this enum value, and complain. > > Done. Consider if the enum had many different values. Would you repeat them like you did here? How about: enum OpenItemType { OPEN_FILE, OPEN_FOLDER, #if defined(OS_LINUX) SHOW_ITEM_IN_FOLDER, #endif }; https://codereview.chromium.org/1867533002/diff/40001/chrome/browser/platform... File chrome/browser/platform_util_linux.cc (right): https://codereview.chromium.org/1867533002/diff/40001/chrome/browser/platform... chrome/browser/platform_util_linux.cc:78: std::char_traits<char>::length(kNautilusKey) + 1; On 2016/04/20 12:02:45, maksims wrote: > On 2016/04/19 21:51:25, Lei Zhang wrote: > > Why not just strlen()? > > Done. Different people like different ways of doing that. Which method is > better? IMO the simpler version, unless there's some advantage otherwise. So why not just strlen(), no std:: ? https://codereview.chromium.org/1867533002/diff/40001/chrome/browser/platform... chrome/browser/platform_util_linux.cc:82: found_version[found_version.length()-1] == '\n') { On 2016/04/20 12:02:45, maksims wrote: > On 2016/04/19 21:51:25, Lei Zhang wrote: > > foo - 1 > > Spaces? Yes, you forgot the next line as well. Alternatively, use back() and pop_back(). https://codereview.chromium.org/1867533002/diff/40001/chrome/browser/platform... chrome/browser/platform_util_linux.cc:91: const base::Version supported_version(kSupportedNautilusVersion); On 2016/04/20 12:02:45, maksims wrote: > On 2016/04/19 21:51:25, Lei Zhang wrote: > > You can declare variables closer to where they are used. > > Done. I didn't mean move |kSupportedNautilusVersion|. I mean move |supported_version| down to line ~102. https://codereview.chromium.org/1867533002/diff/40001/chrome/browser/platform... chrome/browser/platform_util_linux.cc:100: if (file_browser.find(kNautilusKey) != std::string::npos) { On 2016/04/20 12:02:45, maksims wrote: > On 2016/04/19 21:51:25, Lei Zhang wrote: > > xdg-mime query default inode/directory is expected to return > "nautilus.desktop" > > right? Why not just check for that? What if the user has some other software > > that just happens to have nautilus in the name? > > Done. Right my fault. Simplify to: file_browser == kNautilusKey ? If you use find(), you'll still match on foo-nautilus.desktop. https://codereview.chromium.org/1867533002/diff/80001/chrome/browser/platform... File chrome/browser/platform_util_linux.cc (right): https://codereview.chromium.org/1867533002/diff/80001/chrome/browser/platform... chrome/browser/platform_util_linux.cc:67: XDGUtil(kNautilusCmd, working_directory, path); XDGUtil() should be renamed since it's not just launching XDG utils anymore. https://codereview.chromium.org/1867533002/diff/80001/chrome/browser/platform... chrome/browser/platform_util_linux.cc:75: nautilus_cl.AppendArg("--version"); nit: indentation https://codereview.chromium.org/1867533002/diff/80001/chrome/browser/platform... chrome/browser/platform_util_linux.cc:78: const std::size_t version_position = output.find(kNautilusCmd) + Shouldn't you check the output.find() result before adding to it? https://codereview.chromium.org/1867533002/diff/80001/chrome/browser/platform... chrome/browser/platform_util_linux.cc:83: (found_version[found_version.length() - 1]) == '\n') { Why do you need the parenthesis around foo[bar] ? https://codereview.chromium.org/1867533002/diff/80001/chrome/browser/platform... chrome/browser/platform_util_linux.cc:91: std::string CheckNautilusIsDefault() { Why not just return a bool? https://codereview.chromium.org/1867533002/diff/80001/chrome/browser/platform... chrome/browser/platform_util_linux.cc:96: xdg_mime.AppendArg("query"); Also indentation. If you are not familiar with Chromium's coding style, consider just running: git cl format, and let the computer do it. https://codereview.chromium.org/1867533002/diff/80001/chrome/browser/platform... chrome/browser/platform_util_linux.cc:118: OpenItem(profile, full_path.DirName(), OPEN_FOLDER, The TODO from estade still applies, for non-Nautilus users.
PTAL
BTW, have you looked into adding support to xdg-open? The equivalent on KDE with Dolphin is dolphin --select /path/to/file. https://codereview.chromium.org/1867533002/diff/100001/chrome/browser/platfor... File chrome/browser/platform_util_linux.cc (right): https://codereview.chromium.org/1867533002/diff/100001/chrome/browser/platfor... chrome/browser/platform_util_linux.cc:28: void CMDUtil(const std::string& util, How about RunCommand() and change |util| to |command| ? https://codereview.chromium.org/1867533002/diff/100001/chrome/browser/platfor... chrome/browser/platform_util_linux.cc:78: const std::size_t nautilus_position = output.find(kNautilusCmd); You may want to add a comment to explain what you are trying to. Are you assuming nautilus --version returns something like "GNOME nautilus 3.10.1", so you looks for "nautilus", skip past that and then 1 more to get to the 3? https://codereview.chromium.org/1867533002/diff/100001/chrome/browser/platfor... chrome/browser/platform_util_linux.cc:85: if (!found_version.empty() && Maybe what you really want here is TrimWhitespaceASCII() ? https://codereview.chromium.org/1867533002/diff/100001/chrome/browser/platfor... chrome/browser/platform_util_linux.cc:102: if (base::GetAppOutputAndError(xdg_mime, &file_browser)) { You can write this block in fewer lines. Incorporating the comments below: if (!base::GetAppOutputAndError(xdg_mime, &file_browser) || file_browser != kNautilusKey) { return false; } const base::Version supported_version(kSupportedNautilusVersion); DCHECK(supported_version.IsValid()); const base::Version current_version(GetNautilusVersion()); return current_version.IsValid() && current_version >= supported_version; https://codereview.chromium.org/1867533002/diff/100001/chrome/browser/platfor... chrome/browser/platform_util_linux.cc:103: if (file_browser.find(kNautilusKey) != std::string::npos) { file_browser == kNautilusKey, as previously suggested? https://codereview.chromium.org/1867533002/diff/100001/chrome/browser/platfor... chrome/browser/platform_util_linux.cc:107: if ((current_version.IsValid() && supported_version.IsValid()) && Why do you need the extra parenthesis? Isn't (cond1 && cond2) && cond3 the same as cond1 && cond2 && cond3 ? https://codereview.chromium.org/1867533002/diff/100001/chrome/browser/platfor... chrome/browser/platform_util_linux.cc:107: if ((current_version.IsValid() && supported_version.IsValid()) && I would just DCHECK(supported_version.IsValid()) above. https://codereview.chromium.org/1867533002/diff/100001/chrome/browser/platfor... chrome/browser/platform_util_linux.cc:118: bool nautilus_file_browser) { use_nautilus_file_browser? nautilus_is_default_file_browser?
ptal
On 2016/04/21 06:16:51, Lei Zhang wrote: > BTW, have you looked into adding support to xdg-open? > > The equivalent on KDE with Dolphin is dolphin --select /path/to/file. > No, I have not though yet. At first, I wanted to add nautilus support and then extend the support. And what do you mean about adding support to xdg-open? Do you mean this one - https://code.google.com/p/chromium/codesearch#chromium/src/third_party/xdg-ut... ?
Patchset #5 (id:120001) has been deleted
On 2016/04/21 07:07:47, maksims wrote: > On 2016/04/21 06:16:51, Lei Zhang wrote: > > BTW, have you looked into adding support to xdg-open? > > > > The equivalent on KDE with Dolphin is dolphin --select /path/to/file. > > > > No, I have not though yet. At first, I wanted to add nautilus support and then > extend the support. > > And what do you mean about adding support to xdg-open? Do you mean this one - > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/xdg-ut... > ? xdg-open is part of xdg-utils, with upstream located at https://www.freedesktop.org/wiki/Software/xdg-utils/ - I'm saying add support there. That's what estade's TODO refers to.
On 2016/04/21 07:39:47, Lei Zhang wrote: > On 2016/04/21 07:07:47, maksims wrote: > > On 2016/04/21 06:16:51, Lei Zhang wrote: > > > BTW, have you looked into adding support to xdg-open? > > > > > > The equivalent on KDE with Dolphin is dolphin --select /path/to/file. > > > > > > > No, I have not though yet. At first, I wanted to add nautilus support and then > > extend the support. > > > > And what do you mean about adding support to xdg-open? Do you mean this one - > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/xdg-ut... > > ? > > xdg-open is part of xdg-utils, with upstream located at > https://www.freedesktop.org/wiki/Software/xdg-utils/ - I'm saying add support > there. That's what estade's TODO refers to. Aha, that's what you're talking about. No, I have not considered this. Sorry. First, support for nautilus then think further.
ptal https://codereview.chromium.org/1867533002/diff/100001/chrome/browser/platfor... File chrome/browser/platform_util_linux.cc (right): https://codereview.chromium.org/1867533002/diff/100001/chrome/browser/platfor... chrome/browser/platform_util_linux.cc:103: if (file_browser.find(kNautilusKey) != std::string::npos) { On 2016/04/21 06:16:51, Lei Zhang wrote: > file_browser == kNautilusKey, as previously suggested? file_browser string gets trailing whitespace in GetAppOutputAndError as "bonus" :D. Thus, no, only find();
https://codereview.chromium.org/1867533002/diff/100001/chrome/browser/platfor... File chrome/browser/platform_util_linux.cc (right): https://codereview.chromium.org/1867533002/diff/100001/chrome/browser/platfor... chrome/browser/platform_util_linux.cc:103: if (file_browser.find(kNautilusKey) != std::string::npos) { On 2016/04/21 08:33:49, maksims wrote: > On 2016/04/21 06:16:51, Lei Zhang wrote: > > file_browser == kNautilusKey, as previously suggested? > > file_browser string gets trailing whitespace in GetAppOutputAndError as "bonus" > :D. Thus, no, only find(); Trim the whitespace? https://codereview.chromium.org/1867533002/diff/140001/chrome/browser/platfor... File chrome/browser/platform_util_linux.cc (right): https://codereview.chromium.org/1867533002/diff/140001/chrome/browser/platfor... chrome/browser/platform_util_linux.cc:89: if (!found_version.empty() && If you are going to trim, there's no need to check anything else.
ptal
https://codereview.chromium.org/1867533002/diff/160001/chrome/browser/platfor... File chrome/browser/platform_util_linux.cc (right): https://codereview.chromium.org/1867533002/diff/160001/chrome/browser/platfor... chrome/browser/platform_util_linux.cc:83: const std::size_t nautilus_position = output.find(kNautilusCmd); Just size_t, same for line 85. http://www.cplusplus.com/reference/string/string/find/ https://codereview.chromium.org/1867533002/diff/160001/chrome/browser/platfor... chrome/browser/platform_util_linux.cc:86: nautilus_position + strlen(kNautilusCmd) + 1; If |output| just happens to be "GNOME nautilus", output.substr() on the next line goes out of bounds. https://codereview.chromium.org/1867533002/diff/160001/chrome/browser/platfor... chrome/browser/platform_util_linux.cc:118: current_version >= supported_version; Actually fits on the previous line. When I suggested this, I was not in a text editor and was not sure how close this was to 80 columns. https://codereview.chromium.org/1867533002/diff/160001/chrome/browser/platfor... chrome/browser/platform_util_linux.cc:164: base::PostTaskAndReplyWithResult(content::BrowserThread::GetBlockingPool(), This means we would ultimately end up doing one more trip back and forth between threads end up at: (UI thread -> blocking pool -> UI -> blocking pool -> UI). - I thought about doing this only once and caching the result, but then users could potentially change their preferences any time. - For users that are not using Nautilus, this ends up being slightly slower, but given all the work xdg-open does interally anyway, this probably won't be much of a slow down. I can't think of anything better, so let's go with this.
ptal
https://codereview.chromium.org/1867533002/diff/180001/chrome/browser/platfor... File chrome/browser/platform_util_linux.cc (right): https://codereview.chromium.org/1867533002/diff/180001/chrome/browser/platfor... chrome/browser/platform_util_linux.cc:83: size_t nautilus_position = output.find(kNautilusCmd); Why not just search for "nautilus " ?
https://codereview.chromium.org/1867533002/diff/180001/chrome/browser/platfor... File chrome/browser/platform_util_linux.cc (right): https://codereview.chromium.org/1867533002/diff/180001/chrome/browser/platfor... chrome/browser/platform_util_linux.cc:83: size_t nautilus_position = output.find(kNautilusCmd); On 2016/04/25 17:40:19, Lei Zhang wrote: > Why not just search for "nautilus " ? const char kNautilusCmd[] = "nautilus"; IMHO, more elegant.
https://codereview.chromium.org/1867533002/diff/180001/chrome/browser/platfor... File chrome/browser/platform_util_linux.cc (right): https://codereview.chromium.org/1867533002/diff/180001/chrome/browser/platfor... chrome/browser/platform_util_linux.cc:83: size_t nautilus_position = output.find(kNautilusCmd); On 2016/04/26 06:20:19, maksims wrote: > On 2016/04/25 17:40:19, Lei Zhang wrote: > > Why not just search for "nautilus " ? > > const char kNautilusCmd[] = "nautilus"; > > IMHO, more elegant. The version string you are searching for isn't actually the command you are running. It just happens both are approximately the same. If running "nautilus --version" returned "Nautilus x.y.z", you wouldn't be reusing |kNautilusCmd| here. If you search for "nautilus " then you can skip the |version_position| check because you know for sure it is safe to seek that far into the string.
On 2016/04/26 16:14:12, Lei Zhang wrote: > https://codereview.chromium.org/1867533002/diff/180001/chrome/browser/platfor... > File chrome/browser/platform_util_linux.cc (right): > > https://codereview.chromium.org/1867533002/diff/180001/chrome/browser/platfor... > chrome/browser/platform_util_linux.cc:83: size_t nautilus_position = > output.find(kNautilusCmd); > On 2016/04/26 06:20:19, maksims wrote: > > On 2016/04/25 17:40:19, Lei Zhang wrote: > > > Why not just search for "nautilus " ? > > > > const char kNautilusCmd[] = "nautilus"; > > > > IMHO, more elegant. > > The version string you are searching for isn't actually the command you are > running. It just happens both are approximately the same. If running "nautilus > --version" returned "Nautilus x.y.z", you wouldn't be reusing |kNautilusCmd| > here. > > If you search for "nautilus " then you can skip the |version_position| check > because you know for sure it is safe to seek that far into the string. I am sorry, my OS got corrupter and I had to reinstall everything. Couldn't upload to the same cl. Thus, please, comment on this one - https://codereview.chromium.org/1929443002/
On 2016/04/27 07:52:09, maksims wrote: > I am sorry, my OS got corrupter and I had to reinstall everything. Couldn't > upload to the same cl. > Thus, please, comment on this one - https://codereview.chromium.org/1929443002/ You can still reuse the same CL: git cl issue 1867533002
On 2016/04/27 19:15:23, Lei Zhang wrote: > On 2016/04/27 07:52:09, maksims wrote: > > I am sorry, my OS got corrupter and I had to reinstall everything. Couldn't > > upload to the same cl. > > Thus, please, comment on this one - > https://codereview.chromium.org/1929443002/ > > You can still reuse the same CL: git cl issue 1867533002 nope. git rev-parse error.
On 2016/04/28 04:39:39, maksims wrote: > On 2016/04/27 19:15:23, Lei Zhang wrote: > > On 2016/04/27 07:52:09, maksims wrote: > > > I am sorry, my OS got corrupter and I had to reinstall everything. Couldn't > > > upload to the same cl. > > > Thus, please, comment on this one - > > https://codereview.chromium.org/1929443002/ > > > > You can still reuse the same CL: git cl issue 1867533002 > > nope. git rev-parse error. Ok, I don't understand what your error is, and I'm not going to worry about it. I'll close this code review for you then. |
