Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
7831041:
Fix print preview workflow to reflect settings of selected printer. (Closed)
|
Created:
9 years, 3 months ago by arthurhsu Modified:
9 years, 3 months ago CC:
chromium-reviews, arv (Not doing code reviews), darin-cc_chromium.org, brettw-cc_chromium.org Visibility:
Public. |
DescriptionFix print preview workflow to reflect settings of selected printer.
BUG=95110
TEST=none
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=102242
Patch Set 1 #
Total comments: 13
Patch Set 2 : Update per code review #
Total comments: 22
Patch Set 3 : Update per code review #
Total comments: 18
Patch Set 4 : Update per code review #
Total comments: 6
Patch Set 5 : Update per code review #
Total comments: 7
Patch Set 6 : Update per code review #
Total comments: 4
Patch Set 7 : Update per code review #
Total comments: 2
Patch Set 8 : Update per code review #
Total comments: 12
Patch Set 9 : Update per code review #Messages
Total messages: 33 (0 generated)
+ thestig
http://codereview.chromium.org/7831041/diff/1/chrome/browser/printing/printin... File chrome/browser/printing/printing_message_filter.cc (right): http://codereview.chromium.org/7831041/diff/1/chrome/browser/printing/printin... chrome/browser/printing/printing_message_filter.cc:292: printer_query = new printing::PrinterQuery(); just pass an empty |printer_query| and check printer_query.get() in OnUpdatePrintSettingsReply(). http://codereview.chromium.org/7831041/diff/1/chrome/renderer/print_web_view_... File chrome/renderer/print_web_view_helper.cc (left): http://codereview.chromium.org/7831041/diff/1/chrome/renderer/print_web_view_... chrome/renderer/print_web_view_helper.cc:633: if (!InitPrintSettings(print_preview_context_.frame(), I don't understand why you are not calling InitPrintSettings(). http://codereview.chromium.org/7831041/diff/1/printing/print_settings_initial... File printing/print_settings_initializer_win.cc (right): http://codereview.chromium.org/7831041/diff/1/printing/print_settings_initial... printing/print_settings_initializer_win.cc:26: print_settings->ranges = const_cast<PageRanges&>(new_ranges); Why is this needed?
http://codereview.chromium.org/7831041/diff/1/chrome/app/generated_resources.grd File chrome/app/generated_resources.grd (right): http://codereview.chromium.org/7831041/diff/1/chrome/app/generated_resources.... chrome/app/generated_resources.grd:5981: The selected printer is not available or not installed correctly. Check your printer or try selecting another printer. Don't worry about this change. I got it covered in a separate cleanup CL.
http://codereview.chromium.org/7831041/diff/1/chrome/app/generated_resources.grd File chrome/app/generated_resources.grd (right): http://codereview.chromium.org/7831041/diff/1/chrome/app/generated_resources.... chrome/app/generated_resources.grd:5981: The selected printer is not available or not installed correctly. Check your printer or try selecting another printer. On 2011/09/03 00:50:17, Lei Zhang wrote: > Don't worry about this change. I got it covered in a separate cleanup CL. Still keep it. The CL said try select. Should be try selecting. http://codereview.chromium.org/7831041/diff/1/chrome/browser/printing/printin... File chrome/browser/printing/printing_message_filter.cc (right): http://codereview.chromium.org/7831041/diff/1/chrome/browser/printing/printin... chrome/browser/printing/printing_message_filter.cc:292: printer_query = new printing::PrinterQuery(); On 2011/09/02 18:44:19, Lei Zhang wrote: > just pass an empty |printer_query| and check printer_query.get() in > OnUpdatePrintSettingsReply(). It will be used in line 299 and can't be NULL. http://codereview.chromium.org/7831041/diff/1/chrome/renderer/print_web_view_... File chrome/renderer/print_web_view_helper.cc (left): http://codereview.chromium.org/7831041/diff/1/chrome/renderer/print_web_view_... chrome/renderer/print_web_view_helper.cc:633: if (!InitPrintSettings(print_preview_context_.frame(), On 2011/09/02 18:44:19, Lei Zhang wrote: > I don't understand why you are not calling InitPrintSettings(). It's done so based on Kausalya's suggestion. For print preview, we don't need InitPrintSettings(), we just need to get the selected printer's settings. http://codereview.chromium.org/7831041/diff/1/printing/print_settings_initial... File printing/print_settings_initializer_win.cc (right): http://codereview.chromium.org/7831041/diff/1/printing/print_settings_initial... printing/print_settings_initializer_win.cc:26: print_settings->ranges = const_cast<PageRanges&>(new_ranges); On 2011/09/02 18:44:19, Lei Zhang wrote: > Why is this needed? VC10 Debug build will trigger run time error because VC thinks the iterator is of the wrong type. Workaround it is.
http://codereview.chromium.org/7831041/diff/1/chrome/renderer/print_web_view_... File chrome/renderer/print_web_view_helper.cc (left): http://codereview.chromium.org/7831041/diff/1/chrome/renderer/print_web_view_... chrome/renderer/print_web_view_helper.cc:633: if (!InitPrintSettings(print_preview_context_.frame(), On 2011/09/06 22:02:27, arthurhsu wrote: > On 2011/09/02 18:44:19, Lei Zhang wrote: > > I don't understand why you are not calling InitPrintSettings(). > > It's done so based on Kausalya's suggestion. For print preview, we don't need > InitPrintSettings(), we just need to get the selected printer's settings. thestig@: Two main reasons for this change: (1) InitPrintSettings() is initializing the PrintSettings info with default printer default settings. Whenever the user changes the printer, we want to initialize the PrintSettings with selected printer default settings. (2) InitPrintSettings() is not actually required for the print preview workflow. We had that native printing workflow because we need to calculate the page count before displaying the system dialog to the user. With respect to the preview workflow, we always call InitPrintSettings() followed by UpdatePrintSettings(). Do you remember any reasons why we had InitPrintSettings() before UpdatePrintSettings()?
On 2011/09/06 22:47:11, kmadhusu wrote: > http://codereview.chromium.org/7831041/diff/1/chrome/renderer/print_web_view_... > File chrome/renderer/print_web_view_helper.cc (left): > > http://codereview.chromium.org/7831041/diff/1/chrome/renderer/print_web_view_... > chrome/renderer/print_web_view_helper.cc:633: if > (!InitPrintSettings(print_preview_context_.frame(), > On 2011/09/06 22:02:27, arthurhsu wrote: > > On 2011/09/02 18:44:19, Lei Zhang wrote: > > > I don't understand why you are not calling InitPrintSettings(). > > > > It's done so based on Kausalya's suggestion. For print preview, we don't need > > InitPrintSettings(), we just need to get the selected printer's settings. > > thestig@: > > Two main reasons for this change: > > (1) InitPrintSettings() is initializing the PrintSettings info with default > printer default settings. Whenever the user changes the printer, we want to > initialize the PrintSettings with selected printer default settings. > > (2) InitPrintSettings() is not actually required for the print preview workflow. > We had that native printing workflow because we need to calculate the page count > before displaying the system dialog to the user. With respect to the preview > workflow, we always call InitPrintSettings() followed by UpdatePrintSettings(). > > Do you remember any reasons why we had InitPrintSettings() before > UpdatePrintSettings()? ping
http://codereview.chromium.org/7831041/diff/1/chrome/renderer/print_web_view_... File chrome/renderer/print_web_view_helper.cc (right): http://codereview.chromium.org/7831041/diff/1/chrome/renderer/print_web_view_... chrome/renderer/print_web_view_helper.cc:1079: if (print_pages_params_ == NULL) { Can you please explain why do you need this check here? It looks like you will be initializing print_pages_params_ only once.
http://codereview.chromium.org/7831041/diff/1/chrome/renderer/print_web_view_... File chrome/renderer/print_web_view_helper.cc (right): http://codereview.chromium.org/7831041/diff/1/chrome/renderer/print_web_view_... chrome/renderer/print_web_view_helper.cc:1079: if (print_pages_params_ == NULL) { On 2011/09/12 17:20:33, kmadhusu wrote: > Can you please explain why do you need this check here? It looks like you will > be initializing print_pages_params_ only once. The check is to avoid crashing in line 1085 when params is dereferenced. I think the print_pages_params_ shall only be created once. No?
http://codereview.chromium.org/7831041/diff/1/chrome/renderer/print_web_view_... File chrome/renderer/print_web_view_helper.cc (right): http://codereview.chromium.org/7831041/diff/1/chrome/renderer/print_web_view_... chrome/renderer/print_web_view_helper.cc:1079: if (print_pages_params_ == NULL) { On 2011/09/12 17:25:13, arthurhsu wrote: > On 2011/09/12 17:20:33, kmadhusu wrote: > > Can you please explain why do you need this check here? It looks like you will > > be initializing print_pages_params_ only once. > > The check is to avoid crashing in line 1085 when params is dereferenced. I > think the print_pages_params_ shall only be created once. No? (repeating our in-person conversation): We will call this function whenever a setting change (layout, color, page range, printer change, etc.,). Therefore for every preview request, we will reset this print_pages_params_ with new settings info.
http://codereview.chromium.org/7831041/diff/1/chrome/renderer/print_web_view_... File chrome/renderer/print_web_view_helper.cc (right): http://codereview.chromium.org/7831041/diff/1/chrome/renderer/print_web_view_... chrome/renderer/print_web_view_helper.cc:1079: if (print_pages_params_ == NULL) { On 2011/09/12 17:36:52, kmadhusu wrote: > On 2011/09/12 17:25:13, arthurhsu wrote: > > On 2011/09/12 17:20:33, kmadhusu wrote: > > > Can you please explain why do you need this check here? It looks like you > will > > > be initializing print_pages_params_ only once. > > > > The check is to avoid crashing in line 1085 when params is dereferenced. I > > think the print_pages_params_ shall only be created once. No? > > (repeating our in-person conversation): We will call this function whenever a > setting change (layout, color, page range, printer change, etc.,). Therefore for > every preview request, we will reset this print_pages_params_ with new settings > info. Done.
ping
http://codereview.chromium.org/7831041/diff/12002/chrome/browser/printing/pri... File chrome/browser/printing/printing_message_filter.cc (right): http://codereview.chromium.org/7831041/diff/12002/chrome/browser/printing/pri... chrome/browser/printing/printing_message_filter.cc:291: if (!printer_query.get()) { nit: {} is not required. http://codereview.chromium.org/7831041/diff/12002/chrome/browser/printing/pri... chrome/browser/printing/printing_message_filter.cc:309: failure = true; The changes in this function is not required because when the printer_query->last_status != OK, we are resetting the params values which in turn set the dpi() value to zero. http://codereview.chromium.org/7831041/diff/12002/chrome/renderer/print_web_v... File chrome/renderer/print_web_view_helper.cc (left): http://codereview.chromium.org/7831041/diff/12002/chrome/renderer/print_web_v... chrome/renderer/print_web_view_helper.cc:1069: if (PrintMsg_Print_Params_IsEmpty(settings.params)) { You still need this code. This function is used by native printing workflow. http://codereview.chromium.org/7831041/diff/12002/chrome/renderer/print_web_v... File chrome/renderer/print_web_view_helper.cc (right): http://codereview.chromium.org/7831041/diff/12002/chrome/renderer/print_web_v... chrome/renderer/print_web_view_helper.cc:637: return; Do you want to clear print_pages_params_ ?? http://codereview.chromium.org/7831041/diff/12002/chrome/renderer/print_web_v... chrome/renderer/print_web_view_helper.cc:1095: Send(new PrintHostMsg_DidGetDocumentCookie( You need to send this message only during the preview workflow. During the native printing or preview printing, this message is already sent by InitPrintSettingsAndPrepareFrame() function.
http://codereview.chromium.org/7831041/diff/12002/chrome/browser/printing/pri... File chrome/browser/printing/printing_message_filter.cc (right): http://codereview.chromium.org/7831041/diff/12002/chrome/browser/printing/pri... chrome/browser/printing/printing_message_filter.cc:306: bool failure = false; I don't think you need |failure|. Whenever |failure| is set to true, cookie and dpi will be set to 0. http://codereview.chromium.org/7831041/diff/12002/chrome/renderer/print_web_v... File chrome/renderer/print_web_view_helper.cc (left): http://codereview.chromium.org/7831041/diff/12002/chrome/renderer/print_web_v... chrome/renderer/print_web_view_helper.cc:633: if (!InitPrintSettings(print_preview_context_.frame(), Since you are removing this, nobody will call InitPrintSettings() with |print_preview| set to true. Can you cleanup InitPrintSettings()? I noticed InitPrintSettings() also doesn't use it's |node| argument. http://codereview.chromium.org/7831041/diff/12002/chrome/renderer/print_web_v... chrome/renderer/print_web_view_helper.cc:1069: if (PrintMsg_Print_Params_IsEmpty(settings.params)) { I don't think you can remove this for the native dialog print workflow, where we never call UpdatePrintSettings(). Thus the code in your CL at line 1100 doesn't make sense. http://codereview.chromium.org/7831041/diff/12002/chrome/renderer/print_web_v... File chrome/renderer/print_web_view_helper.cc (right): http://codereview.chromium.org/7831041/diff/12002/chrome/renderer/print_web_v... chrome/renderer/print_web_view_helper.cc:1094: print_pages_params_.reset(new PrintMsg_PrintPages_Params(settings)); This basically resets |print_pages_params_| to all 0 values. Why do this? http://codereview.chromium.org/7831041/diff/12002/chrome/renderer/print_web_v... chrome/renderer/print_web_view_helper.cc:1096: routing_id(), print_pages_params_->params.document_cookie)); This will always send a value of 0 for the cookie.
http://codereview.chromium.org/7831041/diff/12002/chrome/renderer/print_web_v... File chrome/renderer/print_web_view_helper.cc (right): http://codereview.chromium.org/7831041/diff/12002/chrome/renderer/print_web_v... chrome/renderer/print_web_view_helper.cc:573: if (!InitPrintSettingsAndPrepareFrame(pdf_frame, &pdf_element, &prepare)) { What will happen if the user has a invalid default printer and has chosen a valid printer for the current print job? I think we will fail here.
http://codereview.chromium.org/7831041/diff/12002/chrome/browser/printing/pri... File chrome/browser/printing/printing_message_filter.cc (right): http://codereview.chromium.org/7831041/diff/12002/chrome/browser/printing/pri... chrome/browser/printing/printing_message_filter.cc:291: if (!printer_query.get()) { On 2011/09/13 20:01:33, kmadhusu wrote: > nit: {} is not required. Done. http://codereview.chromium.org/7831041/diff/12002/chrome/browser/printing/pri... chrome/browser/printing/printing_message_filter.cc:306: bool failure = false; On 2011/09/13 20:09:53, Lei Zhang wrote: > I don't think you need |failure|. Whenever |failure| is set to true, cookie and > dpi will be set to 0. Done. http://codereview.chromium.org/7831041/diff/12002/chrome/browser/printing/pri... chrome/browser/printing/printing_message_filter.cc:309: failure = true; On 2011/09/13 20:01:33, kmadhusu wrote: > The changes in this function is not required because when the > printer_query->last_status != OK, we are resetting the params values which in > turn set the dpi() value to zero. Done. http://codereview.chromium.org/7831041/diff/12002/chrome/renderer/print_web_v... File chrome/renderer/print_web_view_helper.cc (left): http://codereview.chromium.org/7831041/diff/12002/chrome/renderer/print_web_v... chrome/renderer/print_web_view_helper.cc:633: if (!InitPrintSettings(print_preview_context_.frame(), On 2011/09/13 20:09:53, Lei Zhang wrote: > Since you are removing this, nobody will call InitPrintSettings() with > |print_preview| set to true. Can you cleanup InitPrintSettings()? I noticed > InitPrintSettings() also doesn't use it's |node| argument. Done. http://codereview.chromium.org/7831041/diff/12002/chrome/renderer/print_web_v... chrome/renderer/print_web_view_helper.cc:1069: if (PrintMsg_Print_Params_IsEmpty(settings.params)) { On 2011/09/13 20:01:33, kmadhusu wrote: > You still need this code. This function is used by native printing workflow. Done. http://codereview.chromium.org/7831041/diff/12002/chrome/renderer/print_web_v... chrome/renderer/print_web_view_helper.cc:1069: if (PrintMsg_Print_Params_IsEmpty(settings.params)) { On 2011/09/13 20:09:53, Lei Zhang wrote: > I don't think you can remove this for the native dialog print workflow, where we > never call UpdatePrintSettings(). Thus the code in your CL at line 1100 doesn't > make sense. Done. http://codereview.chromium.org/7831041/diff/12002/chrome/renderer/print_web_v... File chrome/renderer/print_web_view_helper.cc (right): http://codereview.chromium.org/7831041/diff/12002/chrome/renderer/print_web_v... chrome/renderer/print_web_view_helper.cc:573: if (!InitPrintSettingsAndPrepareFrame(pdf_frame, &pdf_element, &prepare)) { On 2011/09/13 20:15:34, kmadhusu wrote: > What will happen if the user has a invalid default printer and has chosen a > valid printer for the current print job? I think we will fail here. Done. http://codereview.chromium.org/7831041/diff/12002/chrome/renderer/print_web_v... chrome/renderer/print_web_view_helper.cc:637: return; On 2011/09/13 20:01:33, kmadhusu wrote: > Do you want to clear print_pages_params_ ?? Done. http://codereview.chromium.org/7831041/diff/12002/chrome/renderer/print_web_v... chrome/renderer/print_web_view_helper.cc:1094: print_pages_params_.reset(new PrintMsg_PrintPages_Params(settings)); On 2011/09/13 20:09:53, Lei Zhang wrote: > This basically resets |print_pages_params_| to all 0 values. Why do this? Done. http://codereview.chromium.org/7831041/diff/12002/chrome/renderer/print_web_v... chrome/renderer/print_web_view_helper.cc:1095: Send(new PrintHostMsg_DidGetDocumentCookie( On 2011/09/13 20:01:33, kmadhusu wrote: > You need to send this message only during the preview workflow. > > During the native printing or preview printing, this message is already sent by > InitPrintSettingsAndPrepareFrame() function. Done. http://codereview.chromium.org/7831041/diff/12002/chrome/renderer/print_web_v... chrome/renderer/print_web_view_helper.cc:1096: routing_id(), print_pages_params_->params.document_cookie)); On 2011/09/13 20:09:53, Lei Zhang wrote: > This will always send a value of 0 for the cookie. Done.
http://codereview.chromium.org/7831041/diff/18001/chrome/browser/printing/pri... File chrome/browser/printing/printing_message_filter.cc (right): http://codereview.chromium.org/7831041/diff/18001/chrome/browser/printing/pri... chrome/browser/printing/printing_message_filter.cc:223: nit: Please remove the blank line. http://codereview.chromium.org/7831041/diff/18001/chrome/browser/printing/pri... chrome/browser/printing/printing_message_filter.cc:314: nit: Please remove the blank line. http://codereview.chromium.org/7831041/diff/18001/chrome/browser/printing/pri... chrome/browser/printing/printing_message_filter.cc:317: nit: Please remove the blank line. http://codereview.chromium.org/7831041/diff/18001/chrome/renderer/print_web_v... File chrome/renderer/print_web_view_helper.cc (right): http://codereview.chromium.org/7831041/diff/18001/chrome/renderer/print_web_v... chrome/renderer/print_web_view_helper.cc:572: print_pages_params_.reset(new PrintMsg_PrintPages_Params()); lines 571 & 572 are not required. Any specific reason for adding this code here? http://codereview.chromium.org/7831041/diff/18001/chrome/renderer/print_web_v... chrome/renderer/print_web_view_helper.cc:583: LOG(ERROR) << "Failed to initialize print page settings"; You need to call DidFinishPrinting(FAIL_PRINT); http://codereview.chromium.org/7831041/diff/18001/chrome/renderer/print_web_v... chrome/renderer/print_web_view_helper.cc:640: print_pages_params_.reset(new PrintMsg_PrintPages_Params()); Just release the previous settings. print_pages_params_.reset(); http://codereview.chromium.org/7831041/diff/18001/chrome/renderer/print_web_v... chrome/renderer/print_web_view_helper.cc:805: !PrepareFrame(frame, node, &prepare)) nit: {} required http://codereview.chromium.org/7831041/diff/18001/chrome/renderer/print_web_v... chrome/renderer/print_web_view_helper.cc:805: !PrepareFrame(frame, node, &prepare)) When PrepareFrame() fails, you need to call DidFinishPrinting(FAIL_PRINT) http://codereview.chromium.org/7831041/diff/18001/chrome/renderer/print_web_v... chrome/renderer/print_web_view_helper.cc:1109: print_pages_params_->params.document_cookie, job_settings, &settings)); As we discussed, you can remove print_pages_params_->params.document_cookie. You can also remove lines 1102-1107.
http://codereview.chromium.org/7831041/diff/18001/chrome/browser/printing/pri... File chrome/browser/printing/printing_message_filter.cc (right): http://codereview.chromium.org/7831041/diff/18001/chrome/browser/printing/pri... chrome/browser/printing/printing_message_filter.cc:223: On 2011/09/14 01:37:25, kmadhusu wrote: > nit: Please remove the blank line. Done. http://codereview.chromium.org/7831041/diff/18001/chrome/browser/printing/pri... chrome/browser/printing/printing_message_filter.cc:314: On 2011/09/14 01:37:25, kmadhusu wrote: > nit: Please remove the blank line. Done. http://codereview.chromium.org/7831041/diff/18001/chrome/browser/printing/pri... chrome/browser/printing/printing_message_filter.cc:317: On 2011/09/14 01:37:25, kmadhusu wrote: > nit: Please remove the blank line. Done. http://codereview.chromium.org/7831041/diff/18001/chrome/renderer/print_web_v... File chrome/renderer/print_web_view_helper.cc (right): http://codereview.chromium.org/7831041/diff/18001/chrome/renderer/print_web_v... chrome/renderer/print_web_view_helper.cc:572: print_pages_params_.reset(new PrintMsg_PrintPages_Params()); On 2011/09/14 01:37:25, kmadhusu wrote: > lines 571 & 572 are not required. Any specific reason for adding this code here? Done. http://codereview.chromium.org/7831041/diff/18001/chrome/renderer/print_web_v... chrome/renderer/print_web_view_helper.cc:583: LOG(ERROR) << "Failed to initialize print page settings"; On 2011/09/14 01:37:25, kmadhusu wrote: > You need to call DidFinishPrinting(FAIL_PRINT); Done. http://codereview.chromium.org/7831041/diff/18001/chrome/renderer/print_web_v... chrome/renderer/print_web_view_helper.cc:640: print_pages_params_.reset(new PrintMsg_PrintPages_Params()); On 2011/09/14 01:37:25, kmadhusu wrote: > Just release the previous settings. > > print_pages_params_.reset(); This line is actually wrong. It shall issue failure. http://codereview.chromium.org/7831041/diff/18001/chrome/renderer/print_web_v... chrome/renderer/print_web_view_helper.cc:805: !PrepareFrame(frame, node, &prepare)) On 2011/09/14 01:37:25, kmadhusu wrote: > nit: {} required Done. http://codereview.chromium.org/7831041/diff/18001/chrome/renderer/print_web_v... chrome/renderer/print_web_view_helper.cc:805: !PrepareFrame(frame, node, &prepare)) On 2011/09/14 01:37:25, kmadhusu wrote: > When PrepareFrame() fails, you need to call DidFinishPrinting(FAIL_PRINT) Done. http://codereview.chromium.org/7831041/diff/18001/chrome/renderer/print_web_v... chrome/renderer/print_web_view_helper.cc:1109: print_pages_params_->params.document_cookie, job_settings, &settings)); On 2011/09/14 01:37:25, kmadhusu wrote: > As we discussed, you can remove print_pages_params_->params.document_cookie. > > You can also remove lines 1102-1107. Done.
http://codereview.chromium.org/7831041/diff/21001/chrome/browser/printing/pri... File chrome/browser/printing/printing_message_filter.cc (right): http://codereview.chromium.org/7831041/diff/21001/chrome/browser/printing/pri... chrome/browser/printing/printing_message_filter.cc:314: print_job_manager_->QueuePrinterQuery(printer_query.get()); Do you need to queue the printer query if we are going to always create a new one in UpdatePrintSettings? http://codereview.chromium.org/7831041/diff/21001/chrome/renderer/print_web_v... File chrome/renderer/print_web_view_helper.cc (right): http://codereview.chromium.org/7831041/diff/21001/chrome/renderer/print_web_v... chrome/renderer/print_web_view_helper.cc:635: if (print_preview_context_.last_error() != PREVIEW_ERROR_BAD_SETTING) { How about? print_preview_context_.last_error() == PREVIEW_ERROR_INVALID_PRINTER_SETTINGS
http://codereview.chromium.org/7831041/diff/21001/chrome/browser/printing/pri... File chrome/browser/printing/printing_message_filter.cc (left): http://codereview.chromium.org/7831041/diff/21001/chrome/browser/printing/pri... chrome/browser/printing/printing_message_filter.cc:290: print_job_manager_->PopPrinterQuery(document_cookie, &printer_query); Validate the document_cookie and if it is valid we want to pop the printer query object from the queue.
http://codereview.chromium.org/7831041/diff/21001/chrome/browser/printing/pri... File chrome/browser/printing/printing_message_filter.cc (left): http://codereview.chromium.org/7831041/diff/21001/chrome/browser/printing/pri... chrome/browser/printing/printing_message_filter.cc:290: print_job_manager_->PopPrinterQuery(document_cookie, &printer_query); On 2011/09/16 20:39:11, kmadhusu wrote: > Validate the document_cookie and if it is valid we want to pop the printer query > object from the queue. Done. http://codereview.chromium.org/7831041/diff/21001/chrome/browser/printing/pri... File chrome/browser/printing/printing_message_filter.cc (right): http://codereview.chromium.org/7831041/diff/21001/chrome/browser/printing/pri... chrome/browser/printing/printing_message_filter.cc:314: print_job_manager_->QueuePrinterQuery(printer_query.get()); On 2011/09/16 20:31:14, kmadhusu wrote: > Do you need to queue the printer query if we are going to always create a new > one in UpdatePrintSettings? Done. http://codereview.chromium.org/7831041/diff/21001/chrome/renderer/print_web_v... File chrome/renderer/print_web_view_helper.cc (right): http://codereview.chromium.org/7831041/diff/21001/chrome/renderer/print_web_v... chrome/renderer/print_web_view_helper.cc:635: if (print_preview_context_.last_error() != PREVIEW_ERROR_BAD_SETTING) { On 2011/09/16 20:31:14, kmadhusu wrote: > How about? > print_preview_context_.last_error() == PREVIEW_ERROR_INVALID_PRINTER_SETTINGS There are three possible values, BAD_SETTING, INVALID_PRINTER_SETTING, and UPDATING_PRINT_SETTINGS. The later two shall trigger the error message.
http://codereview.chromium.org/7831041/diff/28001/chrome/browser/printing/pri... File chrome/browser/printing/printing_message_filter.cc (right): http://codereview.chromium.org/7831041/diff/28001/chrome/browser/printing/pri... chrome/browser/printing/printing_message_filter.cc:297: if (!printer_query.get()) { nit: {} is not required http://codereview.chromium.org/7831041/diff/28001/chrome/renderer/print_web_v... File chrome/renderer/print_web_view_helper.cc (right): http://codereview.chromium.org/7831041/diff/28001/chrome/renderer/print_web_v... chrome/renderer/print_web_view_helper.cc:1105: print_pages_params_.reset(new PrintMsg_PrintPages_Params()); Why do you want to reset print_pages_params_ here? By doing this, you are going to reset document_cookie to 0 and always stop the worker whose document_cookie is 0. You can do the following in DidFinishPrinting() for the error scenarios. "int cookie = print_pages_params_.get() ? print_pages_params_->params.document_cookie : 0;" http://codereview.chromium.org/7831041/diff/28001/chrome/renderer/print_web_v... File chrome/renderer/print_web_view_helper.h (right): http://codereview.chromium.org/7831041/diff/28001/chrome/renderer/print_web_v... chrome/renderer/print_web_view_helper.h:334: PREVIEW_ERROR_INVALID_PRINTER_SETTINGS, Please don't forget to add add this in the uma xml file. (You might have known this already).
http://codereview.chromium.org/7831041/diff/28001/chrome/browser/printing/pri... File chrome/browser/printing/printing_message_filter.cc (right): http://codereview.chromium.org/7831041/diff/28001/chrome/browser/printing/pri... chrome/browser/printing/printing_message_filter.cc:297: if (!printer_query.get()) { On 2011/09/19 19:09:41, kmadhusu wrote: > nit: {} is not required Done. http://codereview.chromium.org/7831041/diff/28001/chrome/renderer/print_web_v... File chrome/renderer/print_web_view_helper.cc (right): http://codereview.chromium.org/7831041/diff/28001/chrome/renderer/print_web_v... chrome/renderer/print_web_view_helper.cc:1105: print_pages_params_.reset(new PrintMsg_PrintPages_Params()); On 2011/09/19 19:09:41, kmadhusu wrote: > Why do you want to reset print_pages_params_ here? > By doing this, you are going to reset document_cookie to 0 and always stop the > worker whose document_cookie is 0. > > You can do the following in DidFinishPrinting() for the error scenarios. > > "int cookie = print_pages_params_.get() ? > print_pages_params_->params.document_cookie : 0;" Done. http://codereview.chromium.org/7831041/diff/28001/chrome/renderer/print_web_v... File chrome/renderer/print_web_view_helper.h (right): http://codereview.chromium.org/7831041/diff/28001/chrome/renderer/print_web_v... chrome/renderer/print_web_view_helper.h:334: PREVIEW_ERROR_INVALID_PRINTER_SETTINGS, On 2011/09/19 19:09:41, kmadhusu wrote: > Please don't forget to add add this in the uma xml file. (You might have known > this already). Will do in a separate CL.
http://codereview.chromium.org/7831041/diff/34001/chrome/renderer/print_web_v... File chrome/renderer/print_web_view_helper.cc (right): http://codereview.chromium.org/7831041/diff/34001/chrome/renderer/print_web_v... chrome/renderer/print_web_view_helper.cc:1104: if (is_preview && job_settings.empty()) { In the preview printing workflow, we still want to handle job_settings.empty() case. http://codereview.chromium.org/7831041/diff/34001/chrome/renderer/print_web_v... chrome/renderer/print_web_view_helper.cc:1119: print_pages_params_.reset(new PrintMsg_PrintPages_Params(settings)); Do you still need the code in line 1119?
http://codereview.chromium.org/7831041/diff/34001/chrome/renderer/print_web_v... File chrome/renderer/print_web_view_helper.cc (right): http://codereview.chromium.org/7831041/diff/34001/chrome/renderer/print_web_v... chrome/renderer/print_web_view_helper.cc:1104: if (is_preview && job_settings.empty()) { On 2011/09/20 00:42:41, kmadhusu wrote: > In the preview printing workflow, we still want to handle job_settings.empty() > case. Yes, but it shall send back BAD_SETTINGS instead of INVALID_PRINTER_SETTINGS. http://codereview.chromium.org/7831041/diff/34001/chrome/renderer/print_web_v... chrome/renderer/print_web_view_helper.cc:1119: print_pages_params_.reset(new PrintMsg_PrintPages_Params(settings)); On 2011/09/20 00:42:41, kmadhusu wrote: > Do you still need the code in line 1119? Removed misleading comments. Basically the page params shall be updated with new settings returned.
LGTM with a comment. http://codereview.chromium.org/7831041/diff/38001/chrome/renderer/print_web_v... File chrome/renderer/print_web_view_helper.cc (right): http://codereview.chromium.org/7831041/diff/38001/chrome/renderer/print_web_v... chrome/renderer/print_web_view_helper.cc:1103: if (is_preview && job_settings.empty()) { As we discussed split this if condition.
Also update test cases per our discussions. http://codereview.chromium.org/7831041/diff/38001/chrome/renderer/print_web_v... File chrome/renderer/print_web_view_helper.cc (right): http://codereview.chromium.org/7831041/diff/38001/chrome/renderer/print_web_v... chrome/renderer/print_web_view_helper.cc:1103: if (is_preview && job_settings.empty()) { On 2011/09/20 20:55:08, kmadhusu wrote: > As we discussed split this if condition. Done.
Since the print preview tests all interactive with a mock render thread and mock
printer, you never exercise the real browser code.
The Linux printing code was written with the existing workflow in mind, and
breaks with your changes. You'll need to fix the Linux printing code. I also
recommend testing on Mac just to make sure nothing breaks there.
Index: chrome/browser/printing/print_dialog_gtk.cc
===================================================================
--- chrome/browser/printing/print_dialog_gtk.cc (revision 101780)
+++ chrome/browser/printing/print_dialog_gtk.cc (working copy)
@@ -175,6 +175,9 @@
return false;
}
+ if (!gtk_settings_)
+ gtk_settings_ = gtk_print_settings_new();
+
if (!print_to_pdf) {
scoped_ptr<GtkPrinterList> printer_list(new GtkPrinterList);
printer_ = printer_list->GetPrinterWithName(device_name.c_str());
@@ -182,7 +185,13 @@
g_object_ref(printer_);
gtk_print_settings_set_printer(gtk_settings_,
gtk_printer_get_name(printer_));
+ if (!page_setup_) {
+ page_setup_ = gtk_printer_get_default_page_size(printer_);
+ }
}
+ if (!page_setup_)
+ page_setup_ = gtk_page_setup_new();
+
gtk_print_settings_set_n_copies(gtk_settings_, copies);
gtk_print_settings_set_collate(gtk_settings_, collate);
Index: printing/printing_context_cairo.cc
===================================================================
--- printing/printing_context_cairo.cc (revision 102060)
+++ printing/printing_context_cairo.cc (working copy)
@@ -155,6 +155,11 @@
#else
DCHECK(!in_print_job_);
+ if (!print_dialog_) {
+ print_dialog_ = create_dialog_func_(this);
+ print_dialog_->AddRefToDialog();
+ }
+
if (!print_dialog_->UpdateSettings(job_settings, ranges))
return OnError();
http://codereview.chromium.org/7831041/diff/28001/chrome/browser/printing/pri...
File chrome/browser/printing/printing_message_filter.cc (right):
http://codereview.chromium.org/7831041/diff/28001/chrome/browser/printing/pri...
chrome/browser/printing/printing_message_filter.cc:297: if
(!printer_query.get()) {
On 2011/09/19 19:09:41, kmadhusu wrote:
> nit: {} is not required
I would have preferred it for consistency with the other PopPrinterQuery()
statements in the file. The style guide says:
"In general, curly braces are not required for single-line statements, but they
are allowed if you like them; conditional or loop statements with complex
conditions or statements may be more readable with curly braces. Some projects
require that an if must always always have an accompanying brace."
s/interactive/interacts/
http://codereview.chromium.org/7831041/diff/41001/chrome/renderer/mock_printe... File chrome/renderer/mock_printer.cc (left): http://codereview.chromium.org/7831041/diff/41001/chrome/renderer/mock_printe... chrome/renderer/mock_printer.cc:126: EXPECT_EQ(document_cookie_, cookie); Since UpdateSettings() is effectively the new InitPrintSettings() for print preview, you should create a new |document_cookie_| if |cookie| is invalid. http://codereview.chromium.org/7831041/diff/41001/chrome/renderer/print_web_v... File chrome/renderer/print_web_view_helper.cc (right): http://codereview.chromium.org/7831041/diff/41001/chrome/renderer/print_web_v... chrome/renderer/print_web_view_helper.cc:1109: // Send the cookie so that UpdatePrintSettings can reuse PrintQuery when nit: s/PrintQuery/PrinterQuery/ http://codereview.chromium.org/7831041/diff/41001/chrome/renderer/print_web_v... chrome/renderer/print_web_view_helper.cc:1112: print_pages_params_->params.document_cookie : 0; nit: indentation http://codereview.chromium.org/7831041/diff/41001/chrome/renderer/print_web_v... chrome/renderer/print_web_view_helper.cc:1119: if (!is_preview) { nit: I would do: if (is_preview) { ... } else { ... } http://codereview.chromium.org/7831041/diff/41001/chrome/renderer/print_web_v... chrome/renderer/print_web_view_helper.cc:1123: DCHECK(frame); GetPrintFrame() can fail. Thus I don't know if this DCHECK will always hold. http://codereview.chromium.org/7831041/diff/41001/chrome/renderer/print_web_v... File chrome/renderer/print_web_view_helper.h (right): http://codereview.chromium.org/7831041/diff/41001/chrome/renderer/print_web_v... chrome/renderer/print_web_view_helper.h:174: // Initialize print page settings with default settings. You may want to add a comment to say InitPrintSettings() and InitPrintSettingsAndPrepareFrame() are only used for the native printing work flow.
Linux part fixed with Lei's code, thanks. Tested Mac runs fine locally. http://codereview.chromium.org/7831041/diff/41001/chrome/renderer/mock_printe... File chrome/renderer/mock_printer.cc (left): http://codereview.chromium.org/7831041/diff/41001/chrome/renderer/mock_printe... chrome/renderer/mock_printer.cc:126: EXPECT_EQ(document_cookie_, cookie); On 2011/09/21 03:56:04, Lei Zhang wrote: > Since UpdateSettings() is effectively the new InitPrintSettings() for print > preview, you should create a new |document_cookie_| if |cookie| is invalid. Done. http://codereview.chromium.org/7831041/diff/41001/chrome/renderer/print_web_v... File chrome/renderer/print_web_view_helper.cc (right): http://codereview.chromium.org/7831041/diff/41001/chrome/renderer/print_web_v... chrome/renderer/print_web_view_helper.cc:1109: // Send the cookie so that UpdatePrintSettings can reuse PrintQuery when On 2011/09/21 03:56:04, Lei Zhang wrote: > nit: s/PrintQuery/PrinterQuery/ Done. http://codereview.chromium.org/7831041/diff/41001/chrome/renderer/print_web_v... chrome/renderer/print_web_view_helper.cc:1112: print_pages_params_->params.document_cookie : 0; On 2011/09/21 03:56:04, Lei Zhang wrote: > nit: indentation Done. http://codereview.chromium.org/7831041/diff/41001/chrome/renderer/print_web_v... chrome/renderer/print_web_view_helper.cc:1119: if (!is_preview) { On 2011/09/21 03:56:04, Lei Zhang wrote: > nit: I would do: > > if (is_preview) { > ... > } else { > ... > } Done. http://codereview.chromium.org/7831041/diff/41001/chrome/renderer/print_web_v... chrome/renderer/print_web_view_helper.cc:1123: DCHECK(frame); On 2011/09/21 03:56:04, Lei Zhang wrote: > GetPrintFrame() can fail. Thus I don't know if this DCHECK will always hold. Done. http://codereview.chromium.org/7831041/diff/41001/chrome/renderer/print_web_v... File chrome/renderer/print_web_view_helper.h (right): http://codereview.chromium.org/7831041/diff/41001/chrome/renderer/print_web_v... chrome/renderer/print_web_view_helper.h:174: // Initialize print page settings with default settings. On 2011/09/21 03:56:04, Lei Zhang wrote: > You may want to add a comment to say InitPrintSettings() and > InitPrintSettingsAndPrepareFrame() are only used for the native printing work > flow. Done.
LGTM
Change committed as 102242 |
