Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2444523003:
Have net-export's save dialog default to last used save directory (Closed)
DescriptionHave net-export's save dialog default to last used save directory
When "Start logging to disk" is clicked in chrome://net-export, the pop-up save dialog always defaulted to the home directory. Now, it defaults to the last save directory that was selected.
BUG=625296
Committed: https://crrev.com/27ade38287fc58ee03339d9c57c6d3151736b94d
Cr-Commit-Position: refs/heads/master@{#433034}
Patch Set 1 #
Total comments: 2
Patch Set 2 : First save dialog defaults to user download path instead of home dir #
Total comments: 2
Patch Set 3 : Added DCHECK_CURRENTLY_ON for functions that access last_save_dir #
Depends on Patchset: Messages
Total messages: 37 (12 generated)
wangyix@chromium.org changed reviewers: + eroman@chromium.org, mmenke@chromium.org, wangyix@chromium.org
https://codereview.chromium.org/2444523003/diff/1/chrome/browser/ui/webui/net... File chrome/browser/ui/webui/net_export_ui.cc (right): https://codereview.chromium.org/2444523003/diff/1/chrome/browser/ui/webui/net... chrome/browser/ui/webui/net_export_ui.cc:206: ShowSelectFileDialog(initial_path); This will show the home directory every time Chrome is restarted. What's the behavior if we just use an empty path? On Windows, I suspect it will show us the path wherever the last file dialog was open at, which is preserved across browser restarts, which would probably be superior behavior. Not sure about other platforms, though.
https://codereview.chromium.org/2444523003/diff/1/chrome/browser/ui/webui/net... File chrome/browser/ui/webui/net_export_ui.cc (right): https://codereview.chromium.org/2444523003/diff/1/chrome/browser/ui/webui/net... chrome/browser/ui/webui/net_export_ui.cc:206: ShowSelectFileDialog(initial_path); On 2016/10/24 17:04:55, mmenke wrote: > This will show the home directory every time Chrome is restarted. > > What's the behavior if we just use an empty path? On Windows, I suspect it will > show us the path wherever the last file dialog was open at, which is preserved > across browser restarts, which would probably be superior behavior. Not sure > about other platforms, though. I'm working on Goobuntu, and the behavior is the following: When the dialog is first opened after starting chrome, it defaults to listing "recently used" folders. After the first save, it defaults to the last saved location, as desired. However, the last saved location is not preserved across browser restarts.
Can you run a test for Windows too? It may be that simply passing empty path may be better in that case. The different with using a singleton is if a dialog were opened for another purpose by Chrome, it would lose the log position. But doesn't seem worthwhile just for that case.
On 2016/10/25 20:18:33, eroman wrote: > Can you run a test for Windows too? > > It may be that simply passing empty path may be better in that case. > > The different with using a singleton is if a dialog were opened for another > purpose by Chrome, it would lose the log position. But doesn't seem worthwhile > just for that case. We could also go with an empty path the first time, and then populate a singleton and use it for subsequent dialogs, if we really wanted.
Functionality wise, I think the spirit of BUG=625296 is that when repeatedly creating NetLog dumps, the destination should be somewhat sticky (to facilitate either overwriting an existing log, or generating a series of logs). * Persistence across restarts would be nice, but not critical. So I wouldn't worry about saving this to a setting. * Having the "last used" path mucked up by web content, or other features like "save page as" may or may not be better/worse, depends on the use-case. So my overall take is, if starting with an empty path gives a consistent behavior across platforms (windows, mac, linux, chromeos) then we should prefer that out of simplicity, per Matt's recommendation. If it does not, then the current singleton approach looks good. For comparison, the DevTools code similarly saves the last path to a singleton -- not sure what their design constraints were.
Also, the same comment as on other CLs regarding the description. (The information conveyed in this description is good! But the style can be more in line with that of other Chromium change descriptions)
Description was changed from ========== chrome://net-export save dialog now defaults to last used saved directory. BUG=625296 ========== to ========== Have net-export's save dialog default to last used save directory When "Start logging to disk" is clicked in chrome://net-export, the pop-up save dialog always defaulted to the home directory. Now, it defaults to the last save directory that was selected. BUG=625296 ==========
So I finally tried this on Windows, and here's what I found: If we use empty path for the save dialog, the first time using net-export, it will default to the user's home directory. On subsequent uses of net-export, it will default the last used location, and this DOES persist across browser restarts.
Another thing to consider is that if we use the empty path approach, the save dialog can't suggest a filename (e.g. "chrome-net-export-log.json")
On 2016/11/11 23:54:47, wangyix wrote: > Another thing to consider is that if we use the empty path approach, the save > dialog can't suggest a filename (e.g. "chrome-net-export-log.json") That's unfortunate. That's not a limitation of the Window's select file dialog, I believe, so it's a restriction imposed on us by Chrome's wrapper around it. I guess we're stuck either grabbing the path we're using for downloads, always giving a full path, or hooking this up as a download (Which can get ugly, if we can't just use a file URL - not sure if we allow file:// access to where we save the log on Android).
On 2016/11/14 18:46:08, mmenke wrote: > On 2016/11/11 23:54:47, wangyix wrote: > > Another thing to consider is that if we use the empty path approach, the save > > dialog can't suggest a filename (e.g. "chrome-net-export-log.json") > > That's unfortunate. That's not a limitation of the Window's select file dialog, > I believe, so it's a restriction imposed on us by Chrome's wrapper around it. I > guess we're stuck either grabbing the path we're using for downloads, always > giving a full path, or hooking this up as a download (Which can get ugly, if we > can't just use a file URL - not sure if we allow file:// access to where we save > the log on Android). Oh, we show this on log start, not log end, so a file URL wouldn't work, so making this look like a download would just be way too ugly.
I would say use the singleton approach, but with the modification that the first time it is opened use an empty path rather than home directory. This preserves most of the goodness -- ability to specify log name, stick within browsing session, and also mostly sticky across restarts. My second preferred approach would be to pass empty path, forego an initial filename, but auto-fill in at least the extension (.json, or if we get fancy .netlog)
On 2016/11/16 19:56:55, eroman (slow) wrote: > I would say use the singleton approach, but with the modification that the first > time it is opened use an empty path rather than home directory. > > This preserves most of the goodness -- ability to specify log name, stick within > browsing session, and also mostly sticky across restarts. > > My second preferred approach would be to pass empty path, forego an initial > filename, but auto-fill in at least the extension (.json, or if we get fancy > .netlog) The first approach is a bit weird to me: a filename would not be suggested on the first save dialog, but subsequent save dialogs would have a filename suggested. The second approach has different behavior on Linux vs Windows. Out of these two, I lean slightly towards the second approach. What about using the singleton approach and use the default download path for the first save dialog?
Yeah download path seems fine for initial choice.
On 2016/11/16 21:16:42, eroman (slow) wrote: > Yeah download path seems fine for initial choice. The CL has been updated: the first save dialog now defaults to the user's download path instead of home dir.
LGTM, just one minor comment. https://codereview.chromium.org/2444523003/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/net_export_ui.cc (right): https://codereview.chromium.org/2444523003/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/net_export_ui.cc:47: last_save_dir = LAZY_INSTANCE_INITIALIZER; Comment that this may only be accessed on the UI thread? And also add DCHECK_CURRENTLY_ON(content::BrowserThread::UI); at the start of both methods that access it? Unsafe to read/write to it on multiple threads.
https://codereview.chromium.org/2444523003/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/net_export_ui.cc (right): https://codereview.chromium.org/2444523003/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/net_export_ui.cc:47: last_save_dir = LAZY_INSTANCE_INITIALIZER; On 2016/11/17 18:50:50, mmenke wrote: > Comment that this may only be accessed on the UI thread? And also add > DCHECK_CURRENTLY_ON(content::BrowserThread::UI); at the start of both methods > that access it? > > Unsafe to read/write to it on multiple threads. Done.
The CQ bit was checked by wangyix@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from mmenke@chromium.org Link to the patchset: https://codereview.chromium.org/2444523003/#ps40001 (title: "Added DCHECK_CURRENTLY_ON for functions that access last_save_dir")
The CQ bit was unchecked by wangyix@google.com
The CQ bit was unchecked by wangyix@google.com
wangyix@google.com changed reviewers: + tommycli@chromium.org
Adding tommycli for OWNER approval.
also lgtm
rs lgtm
The CQ bit was checked by wangyix@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by wangyix@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Have net-export's save dialog default to last used save directory When "Start logging to disk" is clicked in chrome://net-export, the pop-up save dialog always defaulted to the home directory. Now, it defaults to the last save directory that was selected. BUG=625296 ========== to ========== Have net-export's save dialog default to last used save directory When "Start logging to disk" is clicked in chrome://net-export, the pop-up save dialog always defaulted to the home directory. Now, it defaults to the last save directory that was selected. BUG=625296 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Have net-export's save dialog default to last used save directory When "Start logging to disk" is clicked in chrome://net-export, the pop-up save dialog always defaulted to the home directory. Now, it defaults to the last save directory that was selected. BUG=625296 ========== to ========== Have net-export's save dialog default to last used save directory When "Start logging to disk" is clicked in chrome://net-export, the pop-up save dialog always defaulted to the home directory. Now, it defaults to the last save directory that was selected. BUG=625296 Committed: https://crrev.com/27ade38287fc58ee03339d9c57c6d3151736b94d Cr-Commit-Position: refs/heads/master@{#433034} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/27ade38287fc58ee03339d9c57c6d3151736b94d Cr-Commit-Position: refs/heads/master@{#433034} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
