Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1929443002:
Add selection of downloaded file using Nautilus (Closed)
|
Created:
4 years, 7 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. |
DescriptionAdd selection of downloaded file using Nautilus
If nautilus is default directory browser and its version is
>= 3.0.2 and if a user clicked "show in folder" near the
downloaded file in "Downloads", then the folder, where the
file is located, is opened with Nautilus and the file is
highlighted. Otherwise old method invoking xdg-open is used
and the folder with the file is opened without highlighting
the file.
the code review for this started in
https://codereview.chromium.org/1867533002/
BUG=352988
Committed: https://crrev.com/4e6a58a6ab3798ad62bb3db33bb9fafcbd3a64df
Cr-Commit-Position: refs/heads/master@{#391027}
Patch Set 1 #
Total comments: 7
Patch Set 2 : #Patch Set 3 : removed extra check in if #
Messages
Total messages: 22 (10 generated)
Description was changed from ========== 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. BUG= ========== 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 ==========
maksim.sisov@intel.com changed reviewers: + estade@chromium.org, thakis@chromium.org, thestig@chromium.org
PTAL
Patchset #2 (id:20001) has been deleted
Can you write a better CL description? (Search for "how to write a git commit message" for reference) I also don't think it's necessary to reference all the Nautilus bugs in the CL description. And for the record, the code review for this started in https://codereview.chromium.org/1867533002/
https://codereview.chromium.org/1929443002/diff/1/chrome/browser/platform_uti... File chrome/browser/platform_util_linux.cc (right): https://codereview.chromium.org/1929443002/diff/1/chrome/browser/platform_uti... chrome/browser/platform_util_linux.cc:81: // "nautilus", skip the whole string and add 1 to get the position of Please update the comment. https://codereview.chromium.org/1929443002/diff/1/chrome/browser/platform_uti... chrome/browser/platform_util_linux.cc:86: output.length() > version_position) { This always evaluates to true if the comparison on the previous line evaluates to true. https://codereview.chromium.org/1929443002/diff/1/chrome/browser/platform_uti... chrome/browser/platform_util_linux.cc:109: if (!success || file_browser != kNautilusKey) { No need for braces with single line if statements + bodies.
Description was changed from ========== 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 ========== to ========== If nautilus is default directory browser and its version is >= 3.0.2 and if a user clicked "show in folder" near the downloaded file in "Downloads", then the folder, where the file is located, is opened with Nautilus and the file is highlighted. Otherwise old method invoking xdg-open is used and the folder with the file is opened without highlighting the file. the code review for this started in https://codereview.chromium.org/1867533002/ BUG=352988 ==========
https://codereview.chromium.org/1929443002/diff/1/chrome/browser/platform_uti... File chrome/browser/platform_util_linux.cc (right): https://codereview.chromium.org/1929443002/diff/1/chrome/browser/platform_uti... chrome/browser/platform_util_linux.cc:81: // "nautilus", skip the whole string and add 1 to get the position of On 2016/04/28 18:20:52, Lei Zhang wrote: > Please update the comment. Done. https://codereview.chromium.org/1929443002/diff/1/chrome/browser/platform_uti... chrome/browser/platform_util_linux.cc:86: output.length() > version_position) { On 2016/04/28 18:20:52, Lei Zhang wrote: > This always evaluates to true if the comparison on the previous line evaluates > to true. Imagine if output is something like "GNOME nautilus " (with extra space in the end), then substr will fail. Of course, the chances it happens are small but they still exist. https://codereview.chromium.org/1929443002/diff/1/chrome/browser/platform_uti... chrome/browser/platform_util_linux.cc:109: if (!success || file_browser != kNautilusKey) { On 2016/04/28 18:20:52, Lei Zhang wrote: > No need for braces with single line if statements + bodies. Done.
The CQ bit was checked by thestig@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1929443002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1929443002/40001
https://codereview.chromium.org/1929443002/diff/1/chrome/browser/platform_uti... File chrome/browser/platform_util_linux.cc (right): https://codereview.chromium.org/1929443002/diff/1/chrome/browser/platform_uti... chrome/browser/platform_util_linux.cc:86: output.length() > version_position) { On 2016/04/29 06:00:14, maksims wrote: > On 2016/04/28 18:20:52, Lei Zhang wrote: > > This always evaluates to true if the comparison on the previous line evaluates > > to true. > > Imagine if output is something like "GNOME nautilus " (with extra space in the > end), then substr will fail. Of course, the chances it happens are small but > they still exist. In this case, we get to the end of the string and the "fail" mode for std::string::substr() is to return an empty string. Then we trim an empty string and return it. That's fine. In previous iterations of this CL, you went beyond the end of the string, and the failure there is more catastrophic. To quote http://www.cplusplus.com/reference/string/string/substr/ If this is equal to the string length, the function returns an empty string. If this is greater than the string length, it throws out_of_range.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
ptal
Description was changed from ========== If nautilus is default directory browser and its version is >= 3.0.2 and if a user clicked "show in folder" near the downloaded file in "Downloads", then the folder, where the file is located, is opened with Nautilus and the file is highlighted. Otherwise old method invoking xdg-open is used and the folder with the file is opened without highlighting the file. the code review for this started in https://codereview.chromium.org/1867533002/ BUG=352988 ========== to ========== Add selection of downloaded file using Nautilus If nautilus is default directory browser and its version is >= 3.0.2 and if a user clicked "show in folder" near the downloaded file in "Downloads", then the folder, where the file is located, is opened with Nautilus and the file is highlighted. Otherwise old method invoking xdg-open is used and the folder with the file is opened without highlighting the file. the code review for this started in https://codereview.chromium.org/1867533002/ BUG=352988 ==========
The CQ bit was checked by thestig@chromium.org
lgtm I made a minor edit to the CL description.
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1929443002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1929443002/60001
Message was sent while issue was closed.
Description was changed from ========== Add selection of downloaded file using Nautilus If nautilus is default directory browser and its version is >= 3.0.2 and if a user clicked "show in folder" near the downloaded file in "Downloads", then the folder, where the file is located, is opened with Nautilus and the file is highlighted. Otherwise old method invoking xdg-open is used and the folder with the file is opened without highlighting the file. the code review for this started in https://codereview.chromium.org/1867533002/ BUG=352988 ========== to ========== Add selection of downloaded file using Nautilus If nautilus is default directory browser and its version is >= 3.0.2 and if a user clicked "show in folder" near the downloaded file in "Downloads", then the folder, where the file is located, is opened with Nautilus and the file is highlighted. Otherwise old method invoking xdg-open is used and the folder with the file is opened without highlighting the file. the code review for this started in https://codereview.chromium.org/1867533002/ BUG=352988 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Add selection of downloaded file using Nautilus If nautilus is default directory browser and its version is >= 3.0.2 and if a user clicked "show in folder" near the downloaded file in "Downloads", then the folder, where the file is located, is opened with Nautilus and the file is highlighted. Otherwise old method invoking xdg-open is used and the folder with the file is opened without highlighting the file. the code review for this started in https://codereview.chromium.org/1867533002/ BUG=352988 ========== to ========== Add selection of downloaded file using Nautilus If nautilus is default directory browser and its version is >= 3.0.2 and if a user clicked "show in folder" near the downloaded file in "Downloads", then the folder, where the file is located, is opened with Nautilus and the file is highlighted. Otherwise old method invoking xdg-open is used and the folder with the file is opened without highlighting the file. the code review for this started in https://codereview.chromium.org/1867533002/ BUG=352988 Committed: https://crrev.com/4e6a58a6ab3798ad62bb3db33bb9fafcbd3a64df Cr-Commit-Position: refs/heads/master@{#391027} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/4e6a58a6ab3798ad62bb3db33bb9fafcbd3a64df Cr-Commit-Position: refs/heads/master@{#391027} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
