Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2952043002:
Print Preview: Create Save As PDF directory if it does not exist. (Closed)
DescriptionPrint Preview: Create Save As PDF directory if it does not exist.
If the default Save As PDF directory (which is the same as the user's
default downloads directory) does not exist, create it before opening
the select file dialog. Otherwise, the select file dialog will open in
the lowest level folder on the path that already exists.
BUG=718527
Review-Url: https://codereview.chromium.org/2952043002
Cr-Commit-Position: refs/heads/master@{#481699}
Committed: https://chromium.googlesource.com/chromium/src/+/dd822dd88fd5f1af99afb1ac3228e7bdad2ecc81
Patch Set 1 #
Total comments: 6
Patch Set 2 : Address comments #
Messages
Total messages: 21 (14 generated)
The CQ bit was checked by rbpotter@chromium.org to run a CQ dry run
Description was changed from ========== Create the default Save As PDF directory if it does not exist. If the default Save As PDF directory (which is the same as the user's default downloads directory) does not exist, create it before opening the select file dialog. Otherwise, the select file dialog will open in the lowest level folder on the path that already exists. BUG=718527 ========== to ========== Create the default Save As PDF directory if it does not exist. If the default Save As PDF directory (which is the same as the user's default downloads directory) does not exist, create it before opening the select file dialog. Otherwise, the select file dialog will open in the lowest level folder on the path that already exists. BUG=718527 ==========
rbpotter@chromium.org changed reviewers: + thestig@chromium.org
Dry run: 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
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2952043002/diff/1/chrome/browser/ui/webui/pri... File chrome/browser/ui/webui/print_preview/print_preview_handler.cc (right): https://codereview.chromium.org/2952043002/diff/1/chrome/browser/ui/webui/pri... chrome/browser/ui/webui/print_preview/print_preview_handler.cc:462: if (!base::DirectoryExists(path) && !path.empty()) Check !path.empty() first since that's cheaper and doesn't involve hitting the disk. Or check it before calling CreateDirectoryIfNeeded(), so you don't even need to thread hop. https://codereview.chromium.org/2952043002/diff/1/chrome/browser/ui/webui/pri... File chrome/browser/ui/webui/print_preview/print_preview_handler.h (right): https://codereview.chromium.org/2952043002/diff/1/chrome/browser/ui/webui/pri... chrome/browser/ui/webui/print_preview/print_preview_handler.h:111: // directory if needed. |OnDirectoryCreated| will be called when this nit: OnDirectoryCreated() is not a variable. s/this/it/ https://codereview.chromium.org/2952043002/diff/1/chrome/browser/ui/webui/pri... chrome/browser/ui/webui/print_preview/print_preview_handler.h:269: void OnDirectoryCreated(base::FilePath path); Pass by const ref.
I feel like the CL description needs to mention print preview somewhere for context.
Description was changed from ========== Create the default Save As PDF directory if it does not exist. If the default Save As PDF directory (which is the same as the user's default downloads directory) does not exist, create it before opening the select file dialog. Otherwise, the select file dialog will open in the lowest level folder on the path that already exists. BUG=718527 ========== to ========== Print Preview: Create Save As PDF directory if it does not exist. If the default Save As PDF directory (which is the same as the user's default downloads directory) does not exist, create it before opening the select file dialog. Otherwise, the select file dialog will open in the lowest level folder on the path that already exists. BUG=718527 ==========
https://codereview.chromium.org/2952043002/diff/1/chrome/browser/ui/webui/pri... File chrome/browser/ui/webui/print_preview/print_preview_handler.cc (right): https://codereview.chromium.org/2952043002/diff/1/chrome/browser/ui/webui/pri... chrome/browser/ui/webui/print_preview/print_preview_handler.cc:462: if (!base::DirectoryExists(path) && !path.empty()) On 2017/06/22 01:39:56, Lei Zhang wrote: > Check !path.empty() first since that's cheaper and doesn't involve hitting the > disk. Or check it before calling CreateDirectoryIfNeeded(), so you don't even > need to thread hop. Done. https://codereview.chromium.org/2952043002/diff/1/chrome/browser/ui/webui/pri... File chrome/browser/ui/webui/print_preview/print_preview_handler.h (right): https://codereview.chromium.org/2952043002/diff/1/chrome/browser/ui/webui/pri... chrome/browser/ui/webui/print_preview/print_preview_handler.h:111: // directory if needed. |OnDirectoryCreated| will be called when this On 2017/06/22 01:39:56, Lei Zhang wrote: > nit: OnDirectoryCreated() is not a variable. > > s/this/it/ Done. https://codereview.chromium.org/2952043002/diff/1/chrome/browser/ui/webui/pri... chrome/browser/ui/webui/print_preview/print_preview_handler.h:269: void OnDirectoryCreated(base::FilePath path); On 2017/06/22 01:39:56, Lei Zhang wrote: > Pass by const ref. Done.
The CQ bit was checked by rbpotter@chromium.org to run a CQ dry run
Dry run: 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
Dry run: This issue passed the CQ dry run.
lgtm
The CQ bit was checked by rbpotter@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1498171784994920,
"parent_rev": "668f5234e53dc61582b34a66dda5ef73615db214", "commit_rev":
"dd822dd88fd5f1af99afb1ac3228e7bdad2ecc81"}
Message was sent while issue was closed.
Description was changed from ========== Print Preview: Create Save As PDF directory if it does not exist. If the default Save As PDF directory (which is the same as the user's default downloads directory) does not exist, create it before opening the select file dialog. Otherwise, the select file dialog will open in the lowest level folder on the path that already exists. BUG=718527 ========== to ========== Print Preview: Create Save As PDF directory if it does not exist. If the default Save As PDF directory (which is the same as the user's default downloads directory) does not exist, create it before opening the select file dialog. Otherwise, the select file dialog will open in the lowest level folder on the path that already exists. BUG=718527 Review-Url: https://codereview.chromium.org/2952043002 Cr-Commit-Position: refs/heads/master@{#481699} Committed: https://chromium.googlesource.com/chromium/src/+/dd822dd88fd5f1af99afb1ac3228... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/dd822dd88fd5f1af99afb1ac3228... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
