|
|
Chromium Code Reviews|
Created:
9 years, 8 months ago by dpapad Modified:
9 years, 7 months ago CC:
chromium-reviews, arv (Not doing code reviews) Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionPrepopulating "Print To PDF" select file dialog with a suggested filename and path.
The suggested name is the title of the initiator tab. Also, the first time, the select file dialog opens on the "Documents" folder (or the equivalent for the current platform). In subsequent "Print To Pdf" sessions the last used folder is remembered.
BUG=NONE
TEST=In the print preview tab select "Print to PDF". The select file dialog box should be pre-populated. Also the second time you save to pdf, the suggested folder should be the one used right before.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=81815
Patch Set 1 #
Total comments: 9
Patch Set 2 : Addressing comments. #Patch Set 3 : Rebasing #
Total comments: 2
Patch Set 4 : Getting print job title in a different way. #
Total comments: 5
Patch Set 5 : Addressing comments. #Patch Set 6 : Remembering last used path. #
Total comments: 8
Patch Set 7 : Addressing comments #
Total comments: 12
Patch Set 8 : Addressing comments. #
Total comments: 9
Patch Set 9 : Removing unused included, adding comments. #Patch Set 10 : Rebasing #
Messages
Total messages: 22 (0 generated)
http://codereview.chromium.org/6759044/diff/1/chrome/browser/resources/print_... File chrome/browser/resources/print_preview.js (right): http://codereview.chromium.org/6759044/diff/1/chrome/browser/resources/print_... chrome/browser/resources/print_preview.js:209: 'printJobTitle': printJobTitle}); I actually want to get rid of printJobTitle altogether. Notice all the other settings are things that the user can change via controls whereas the job title is a property of the page that the browser already knows about. When/if http://codereview.chromium.org/6783005/ lands, we can get the title via the PrintViewManager on the browser side instead. http://codereview.chromium.org/6759044/diff/1/chrome/browser/ui/webui/print_p... File chrome/browser/ui/webui/print_preview_handler.cc (right): http://codereview.chromium.org/6759044/diff/1/chrome/browser/ui/webui/print_p... chrome/browser/ui/webui/print_preview_handler.cc:201: bool print_to_pdf; Can you initialize this to false? Missed it in the last CL. http://codereview.chromium.org/6759044/diff/1/chrome/browser/ui/webui/print_p... chrome/browser/ui/webui/print_preview_handler.cc:207: settings->GetString(printing::kPrintJobTitle, &print_job_title); Check the return value and handle both the failure case and the case where print_job_title is ''. I don't want to save to /home/foo/Documents/.pdf. http://codereview.chromium.org/6759044/diff/1/chrome/browser/ui/webui/print_p... chrome/browser/ui/webui/print_preview_handler.cc:208: file_util::ReplaceIllegalCharactersInPath(&print_job_title, '_'); This won't work on Windows. You need to write this as: #if defined(OS_WIN) string16 print_job_title_win = UTF8ToUTF16(print_job_title); file_util::ReplaceIllegalCharactersInPath(&print_job_title_win, '_'); FilePath default_filename(print_job_title_win); #else file_util::ReplaceIllegalCharactersInPath(&print_job_title, '_'); FilePath default_filename(print_job_title); #endif // defined(OS_WIN) default_filename = default_filename.ReplaceExtension(FILE_PATH_LITERAL("pdf")); FilePath default_path; PathService::Get(chrome::DIR_USER_DOCUMENTS, &default_path); SelectFile(default_path.Append(default_filename)); // whew! http://codereview.chromium.org/6759044/diff/1/chrome/browser/ui/webui/print_p... File chrome/browser/ui/webui/print_preview_handler.h (right): http://codereview.chromium.org/6759044/diff/1/chrome/browser/ui/webui/print_p... chrome/browser/ui/webui/print_preview_handler.h:35: void SelectFile(const FilePath& default_filename); nit: default_path.
http://codereview.chromium.org/6759044/diff/1/chrome/browser/ui/webui/print_p... File chrome/browser/ui/webui/print_preview_handler.cc (right): http://codereview.chromium.org/6759044/diff/1/chrome/browser/ui/webui/print_p... chrome/browser/ui/webui/print_preview_handler.cc:201: bool print_to_pdf; On 2011/04/01 01:05:35, Lei Zhang wrote: > Can you initialize this to false? Missed it in the last CL. Done. http://codereview.chromium.org/6759044/diff/1/chrome/browser/ui/webui/print_p... chrome/browser/ui/webui/print_preview_handler.cc:207: settings->GetString(printing::kPrintJobTitle, &print_job_title); On 2011/04/01 01:05:35, Lei Zhang wrote: > Check the return value and handle both the failure case and the case where > print_job_title is ''. I don't want to save to /home/foo/Documents/.pdf. Done. http://codereview.chromium.org/6759044/diff/1/chrome/browser/ui/webui/print_p... chrome/browser/ui/webui/print_preview_handler.cc:208: file_util::ReplaceIllegalCharactersInPath(&print_job_title, '_'); On 2011/04/01 01:05:35, Lei Zhang wrote: > This won't work on Windows. You need to write this as: > > #if defined(OS_WIN) > string16 print_job_title_win = UTF8ToUTF16(print_job_title); > file_util::ReplaceIllegalCharactersInPath(&print_job_title_win, '_'); > FilePath default_filename(print_job_title_win); > #else > file_util::ReplaceIllegalCharactersInPath(&print_job_title, '_'); > FilePath default_filename(print_job_title); > #endif // defined(OS_WIN) > > default_filename = > default_filename.ReplaceExtension(FILE_PATH_LITERAL("pdf")); > FilePath default_path; > PathService::Get(chrome::DIR_USER_DOCUMENTS, &default_path); > SelectFile(default_path.Append(default_filename)); > > // whew! Done, without the ifdefs (thanks to overloading ;> ). http://codereview.chromium.org/6759044/diff/1/chrome/browser/ui/webui/print_p... File chrome/browser/ui/webui/print_preview_handler.h (right): http://codereview.chromium.org/6759044/diff/1/chrome/browser/ui/webui/print_p... chrome/browser/ui/webui/print_preview_handler.h:35: void SelectFile(const FilePath& default_filename); On 2011/04/01 01:05:35, Lei Zhang wrote: > nit: default_path. Done.
Ping
On 2011/04/05 20:44:36, dpapad wrote: > Ping Still waiting on http://codereview.chromium.org/6783005/
http://codereview.chromium.org/6759044/diff/7001/chrome/browser/ui/webui/prin... File chrome/browser/ui/webui/print_preview_handler.cc (right): http://codereview.chromium.org/6759044/diff/7001/chrome/browser/ui/webui/prin... chrome/browser/ui/webui/print_preview_handler.cc:215: if (!ret || print_job_title.length() == 0) If the "print_job_title" is empty, I think its better to set the default print document title (IDS_DEFAULT_PRINT_DOCUMENT_TITLE) and proceed to display the Save As file dialog box.
http://codereview.chromium.org/6759044/diff/7001/chrome/browser/ui/webui/prin... File chrome/browser/ui/webui/print_preview_handler.cc (right): http://codereview.chromium.org/6759044/diff/7001/chrome/browser/ui/webui/prin... chrome/browser/ui/webui/print_preview_handler.cc:215: if (!ret || print_job_title.length() == 0) On 2011/04/05 20:57:48, kmadhusu wrote: > If the "print_job_title" is empty, I think its better to set the default print > document title (IDS_DEFAULT_PRINT_DOCUMENT_TITLE) and proceed to display the > Save As file dialog box. Done. http://codereview.chromium.org/6759044/diff/14001/chrome/browser/printing/pri... File chrome/browser/printing/print_preview_message_handler.cc (right): http://codereview.chromium.org/6759044/diff/14001/chrome/browser/printing/pri... chrome/browser/printing/print_preview_message_handler.cc:85: wrapper->print_view_manager()->RenderSourceName()); Changed the way the title is retrieved before passed to the js file.
http://codereview.chromium.org/6759044/diff/14001/chrome/browser/ui/webui/pri... File chrome/browser/ui/webui/print_preview_handler.cc (right): http://codereview.chromium.org/6759044/diff/14001/chrome/browser/ui/webui/pri... chrome/browser/ui/webui/print_preview_handler.cc:217: if (!ret || print_job_title.length() == 0) If "ret" value is false, we need to return. Please split this if condition into two different if statements. http://codereview.chromium.org/6759044/diff/14001/chrome/browser/ui/webui/pri... chrome/browser/ui/webui/print_preview_handler.cc:218: print_job_title = l10n_util::GetStringUTF8(IDS_PRINT_PREVIEW_TITLE); We don't want to display the default file name as "Print Preview.pdf". We want it to be "Untitled Document.pdf". Please use 'IDS_DEFAULT_PRINT_DOCUMENT_TITLE'.
http://codereview.chromium.org/6759044/diff/14001/chrome/browser/ui/webui/pri... File chrome/browser/ui/webui/print_preview_handler.cc (right): http://codereview.chromium.org/6759044/diff/14001/chrome/browser/ui/webui/pri... chrome/browser/ui/webui/print_preview_handler.cc:217: if (!ret || print_job_title.length() == 0) On 2011/04/11 17:34:26, kmadhusu wrote: > If "ret" value is false, we need to return. Please split this if condition into > two different if statements. > Why do we want to prevent the user from printing, just because the job title could not be retrieved? This is why we have a default document name. Also at this point print_job_title could not be null, as explained in the comments of the Get method in chromium/src/base/values.h:249. If it fails then the second argument is not affected at all, therefore it will be the empty string and the if statement will be executed. http://codereview.chromium.org/6759044/diff/14001/chrome/browser/ui/webui/pri... chrome/browser/ui/webui/print_preview_handler.cc:218: print_job_title = l10n_util::GetStringUTF8(IDS_PRINT_PREVIEW_TITLE); On 2011/04/11 17:34:26, kmadhusu wrote: > We don't want to display the default file name as "Print Preview.pdf". We want > it to be "Untitled Document.pdf". Please use 'IDS_DEFAULT_PRINT_DOCUMENT_TITLE'. Done.
On 2011/04/12 00:20:09, dpapad wrote: > http://codereview.chromium.org/6759044/diff/14001/chrome/browser/ui/webui/pri... > File chrome/browser/ui/webui/print_preview_handler.cc (right): > > http://codereview.chromium.org/6759044/diff/14001/chrome/browser/ui/webui/pri... > chrome/browser/ui/webui/print_preview_handler.cc:217: if (!ret || > print_job_title.length() == 0) > On 2011/04/11 17:34:26, kmadhusu wrote: > > If "ret" value is false, we need to return. Please split this if condition > into > > two different if statements. > > > > Why do we want to prevent the user from printing, just because the job title > could not be retrieved? This is why we have a default document name. > > Also at this point print_job_title could not be null, as explained in the > comments of the Get method in chromium/src/base/values.h:249. If it fails then > the second argument is not affected at all, therefore it will be the empty > string and the if statement will be executed. > It can remain as a single if statement. Thanks for the explanation.
This last patch causes Chromium to remember the last saved path when opening the select file dialog instead of always defaulting to the Documents folder. The last saved path is forgotten when Chromium is closed (since nothing is written to disk).
On 2011/04/13 22:04:20, dpapad wrote: > This last patch causes Chromium to remember the last saved path when opening the > select file dialog instead of always defaulting to the Documents folder. The > last saved path is forgotten when Chromium is closed (since nothing is written > to disk). I don't think this is necessary. In the future, when you select "print to PDF" and print, the tab will close and you'll lose the saved state.
http://codereview.chromium.org/6759044/diff/22001/chrome/browser/resources/pr... File chrome/browser/resources/print_preview.js (right): http://codereview.chromium.org/6759044/diff/22001/chrome/browser/resources/pr... chrome/browser/resources/print_preview.js:219: 'printJobTitle': printJobTitle}); just get rid of |printJobTitle| here. http://codereview.chromium.org/6759044/diff/22001/chrome/browser/ui/webui/pri... File chrome/browser/ui/webui/print_preview_handler.cc (right): http://codereview.chromium.org/6759044/diff/22001/chrome/browser/ui/webui/pri... chrome/browser/ui/webui/print_preview_handler.cc:157: PathService::Get(chrome::DIR_USER_DOCUMENTS, last_saved_path_); BTW, you can't do this - file access on the UI thread. http://codereview.chromium.org/6759044/diff/22001/chrome/browser/ui/webui/pri... chrome/browser/ui/webui/print_preview_handler.cc:221: FilePath::StringType print_job_title; I think you can instead just do the following: TabContentsWrapper* wrapper = TabContentsWrapper::GetCurrentWrapperForContents(web_ui_->tab_contents()); string16 print_job_title = wrapper->print_view_manager()->RenderSourceName(); std::string print_job_title_utf8 = UTF16ToUTF8(print_job_title); You'll need a #ifdef for Windows vs Posix though. http://codereview.chromium.org/6759044/diff/22001/printing/print_job_constants.h File printing/print_job_constants.h (right): http://codereview.chromium.org/6759044/diff/22001/printing/print_job_constant... printing/print_job_constants.h:16: extern const char kPrintJobTitle[]; and get rid of it here.
On 2011/04/13 23:53:20, Lei Zhang wrote: > On 2011/04/13 22:04:20, dpapad wrote: > > This last patch causes Chromium to remember the last saved path when opening > the > > select file dialog instead of always defaulting to the Documents folder. The > > last saved path is forgotten when Chromium is closed (since nothing is written > > to disk). > > I don't think this is necessary. In the future, when you select "print to PDF" > and print, the tab will close and you'll lose the saved state. It is a static variable so I the saved state will not be lost I think. So if you print to pdf once and the print preview another webpage and print to pdf it should work.
http://codereview.chromium.org/6759044/diff/22001/chrome/browser/resources/pr... File chrome/browser/resources/print_preview.js (right): http://codereview.chromium.org/6759044/diff/22001/chrome/browser/resources/pr... chrome/browser/resources/print_preview.js:219: 'printJobTitle': printJobTitle}); On 2011/04/14 00:15:45, Lei Zhang wrote: > just get rid of |printJobTitle| here. Done. http://codereview.chromium.org/6759044/diff/22001/chrome/browser/ui/webui/pri... File chrome/browser/ui/webui/print_preview_handler.cc (right): http://codereview.chromium.org/6759044/diff/22001/chrome/browser/ui/webui/pri... chrome/browser/ui/webui/print_preview_handler.cc:157: PathService::Get(chrome::DIR_USER_DOCUMENTS, last_saved_path_); On 2011/04/14 00:15:45, Lei Zhang wrote: > BTW, you can't do this - file access on the UI thread. Added a scoped exception for the moment. Alternatively, a synchronous renderer-to-browser Message could be dispatched if it is considerer a better technique. http://codereview.chromium.org/6759044/diff/22001/chrome/browser/ui/webui/pri... chrome/browser/ui/webui/print_preview_handler.cc:221: FilePath::StringType print_job_title; On 2011/04/14 00:15:45, Lei Zhang wrote: > I think you can instead just do the following: > > TabContentsWrapper* wrapper = > TabContentsWrapper::GetCurrentWrapperForContents(web_ui_->tab_contents()); > > string16 print_job_title = > wrapper->print_view_manager()->RenderSourceName(); > > std::string print_job_title_utf8 = UTF16ToUTF8(print_job_title); > > You'll need a #ifdef for Windows vs Posix though. Done. http://codereview.chromium.org/6759044/diff/22001/printing/print_job_constants.h File printing/print_job_constants.h (right): http://codereview.chromium.org/6759044/diff/22001/printing/print_job_constant... printing/print_job_constants.h:16: extern const char kPrintJobTitle[]; On 2011/04/14 00:15:45, Lei Zhang wrote: > and get rid of it here. Done. http://codereview.chromium.org/6759044/diff/28001/chrome/browser/ui/webui/pri... File chrome/browser/ui/webui/print_preview_handler.cc (right): http://codereview.chromium.org/6759044/diff/28001/chrome/browser/ui/webui/pri... chrome/browser/ui/webui/print_preview_handler.cc:237: #else Should this be #elif defined(OS_POSIX)?
http://codereview.chromium.org/6759044/diff/28001/chrome/browser/ui/webui/pri... File chrome/browser/ui/webui/print_preview_handler.cc (right): http://codereview.chromium.org/6759044/diff/28001/chrome/browser/ui/webui/pri... chrome/browser/ui/webui/print_preview_handler.cc:159: last_saved_path_ = new FilePath(); I think you're going to leak the new FilePath if you do it this way. |last_saved_path_| probably should be a FilePath and not a FilePath*. http://codereview.chromium.org/6759044/diff/28001/chrome/browser/ui/webui/pri... chrome/browser/ui/webui/print_preview_handler.cc:161: PathService::Get(chrome::DIR_USER_DOCUMENTS, last_saved_path_); You can do this in later in SelectFile(). http://codereview.chromium.org/6759044/diff/28001/chrome/browser/ui/webui/pri... chrome/browser/ui/webui/print_preview_handler.cc:230: ->RenderSourceName(); nit: this looks kind of weird style-wise. http://codereview.chromium.org/6759044/diff/28001/chrome/browser/ui/webui/pri... chrome/browser/ui/webui/print_preview_handler.cc:232: if (print_job_title_temp.length() == 0) You don't need this. PrintViewManager::RenderSourceName() should have done that for you. http://codereview.chromium.org/6759044/diff/28001/chrome/browser/ui/webui/pri... chrome/browser/ui/webui/print_preview_handler.cc:237: #else On 2011/04/14 02:33:35, dpapad wrote: > Should this be #elif defined(OS_POSIX)? Sure.
http://codereview.chromium.org/6759044/diff/28001/chrome/browser/ui/webui/pri... File chrome/browser/ui/webui/print_preview_handler.cc (right): http://codereview.chromium.org/6759044/diff/28001/chrome/browser/ui/webui/pri... chrome/browser/ui/webui/print_preview_handler.cc:159: last_saved_path_ = new FilePath(); On 2011/04/14 21:00:37, Lei Zhang wrote: > I think you're going to leak the new FilePath if you do it this way. > |last_saved_path_| probably should be a FilePath and not a FilePath*. I did this because that is what the style guide suggests for static variables. More specifically it states: "If you need a static or global variable of a class type, consider initializing a pointer (which will never be freed)..." http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Static_and_Glo... http://codereview.chromium.org/6759044/diff/28001/chrome/browser/ui/webui/pri... chrome/browser/ui/webui/print_preview_handler.cc:161: PathService::Get(chrome::DIR_USER_DOCUMENTS, last_saved_path_); On 2011/04/14 21:00:37, Lei Zhang wrote: > You can do this in later in SelectFile(). Done. http://codereview.chromium.org/6759044/diff/28001/chrome/browser/ui/webui/pri... chrome/browser/ui/webui/print_preview_handler.cc:230: ->RenderSourceName(); On 2011/04/14 21:00:37, Lei Zhang wrote: > nit: this looks kind of weird style-wise. Done. http://codereview.chromium.org/6759044/diff/28001/chrome/browser/ui/webui/pri... chrome/browser/ui/webui/print_preview_handler.cc:232: if (print_job_title_temp.length() == 0) On 2011/04/14 21:00:37, Lei Zhang wrote: > You don't need this. PrintViewManager::RenderSourceName() should have done that > for you. Done. http://codereview.chromium.org/6759044/diff/28001/chrome/browser/ui/webui/pri... chrome/browser/ui/webui/print_preview_handler.cc:237: #else On 2011/04/14 21:00:37, Lei Zhang wrote: > On 2011/04/14 02:33:35, dpapad wrote: > > Should this be #elif defined(OS_POSIX)? > > Sure. Done.
http://codereview.chromium.org/6759044/diff/28001/chrome/browser/ui/webui/pri... File chrome/browser/ui/webui/print_preview_handler.cc (right): http://codereview.chromium.org/6759044/diff/28001/chrome/browser/ui/webui/pri... chrome/browser/ui/webui/print_preview_handler.cc:159: last_saved_path_ = new FilePath(); On 2011/04/14 21:25:32, dpapad wrote: > On 2011/04/14 21:00:37, Lei Zhang wrote: > > I think you're going to leak the new FilePath if you do it this way. > > |last_saved_path_| probably should be a FilePath and not a FilePath*. > > I did this because that is what the style guide suggests for static variables. > > More specifically it states: > "If you need a static or global variable of a class type, consider initializing > a pointer (which will never be freed)..." > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Static_and_Glo... Oh right. Have you considered using a LazyInstance instead?
On 2011/04/14 21:29:56, Lei Zhang wrote: > http://codereview.chromium.org/6759044/diff/28001/chrome/browser/ui/webui/pri... > File chrome/browser/ui/webui/print_preview_handler.cc (right): > > http://codereview.chromium.org/6759044/diff/28001/chrome/browser/ui/webui/pri... > chrome/browser/ui/webui/print_preview_handler.cc:159: last_saved_path_ = new > FilePath(); > On 2011/04/14 21:25:32, dpapad wrote: > > On 2011/04/14 21:00:37, Lei Zhang wrote: > > > I think you're going to leak the new FilePath if you do it this way. > > > |last_saved_path_| probably should be a FilePath and not a FilePath*. > > > > I did this because that is what the style guide suggests for static variables. > > > > More specifically it states: > > "If you need a static or global variable of a class type, consider > initializing > > a pointer (which will never be freed)..." > > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Static_and_Glo... > > Oh right. Have you considered using a LazyInstance instead? I have not looked into it, but I thought that LazyInstance should be used when there is a race condition. In our case only one select file dialog can be displayed at any time (since it is modal) so there is no danger while initializing the static variable. Pls correct me If I am missing something.
http://codereview.chromium.org/6759044/diff/23009/chrome/browser/ui/webui/pri... File chrome/browser/ui/webui/print_preview_handler.cc (right): http://codereview.chromium.org/6759044/diff/23009/chrome/browser/ui/webui/pri... chrome/browser/ui/webui/print_preview_handler.cc:26: #include "grit/generated_resources.h" no longer needed http://codereview.chromium.org/6759044/diff/23009/chrome/browser/ui/webui/pri... chrome/browser/ui/webui/print_preview_handler.cc:31: #include "ui/base/l10n/l10n_util.h" no longer needed http://codereview.chromium.org/6759044/diff/23009/chrome/browser/ui/webui/pri... chrome/browser/ui/webui/print_preview_handler.cc:151: FilePath* PrintPreviewHandler::last_saved_path_ = NULL; add // static comment http://codereview.chromium.org/6759044/diff/23009/chrome/browser/ui/webui/pri... chrome/browser/ui/webui/print_preview_handler.cc:224: string16 print_job_title_temp = how about s/temp/utf16/ http://codereview.chromium.org/6759044/diff/23009/chrome/browser/ui/webui/pri... chrome/browser/ui/webui/print_preview_handler.cc:280: base::ThreadRestrictions::ScopedAllowIO allow_io; You should add a comment to explain why this is ok to do here.
http://codereview.chromium.org/6759044/diff/23009/chrome/browser/ui/webui/pri... File chrome/browser/ui/webui/print_preview_handler.cc (right): http://codereview.chromium.org/6759044/diff/23009/chrome/browser/ui/webui/pri... chrome/browser/ui/webui/print_preview_handler.cc:31: #include "ui/base/l10n/l10n_util.h" On 2011/04/14 22:53:43, Lei Zhang wrote: > no longer needed Thanks for catching these. http://codereview.chromium.org/6759044/diff/23009/chrome/browser/ui/webui/pri... chrome/browser/ui/webui/print_preview_handler.cc:151: FilePath* PrintPreviewHandler::last_saved_path_ = NULL; On 2011/04/14 22:53:43, Lei Zhang wrote: > add // static comment Done. http://codereview.chromium.org/6759044/diff/23009/chrome/browser/ui/webui/pri... chrome/browser/ui/webui/print_preview_handler.cc:224: string16 print_job_title_temp = On 2011/04/14 22:53:43, Lei Zhang wrote: > how about s/temp/utf16/ Done. http://codereview.chromium.org/6759044/diff/23009/chrome/browser/ui/webui/pri... chrome/browser/ui/webui/print_preview_handler.cc:280: base::ThreadRestrictions::ScopedAllowIO allow_io; On 2011/04/14 22:53:43, Lei Zhang wrote: > You should add a comment to explain why this is ok to do here. Done.
LGTM |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
