Print preview page selection should not require a rerendering of draft pages.

Print preview page selection should not require a rerendering of draft pages. 

On page selection, regenerate the complete document with the selection, but preserve the existing draft pages and simply display a subset of them. 

Added a new param |clear all preview data| to |PrintHostMsg_DidGetPreviewPageCount|. 

BUG=84383
TEST=print preview works after code changes.

Please refer to try bot results in Patch 12

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=98926

[vandebo (ex-Chrome), 2011-08-16 16:41:48]

LGTM

[Chris Guillory, 2011-08-16 20:08:53]

http://codereview.chromium.org/7647010/diff/4001/chrome/renderer/mock_printer.cc
File chrome/renderer/mock_printer.cc (right):

http://codereview.chromium.org/7647010/diff/4001/chrome/renderer/mock_printer...
chrome/renderer/mock_printer.cc:105: memset(params, 0,
sizeof(PrintMsg_PrintPages_Params));
Looking at this further I think line is your issue. You are using memset to
clear a std::vector isn't valid (especially if the vector has data). If you
remove this it looks like the problem goes away.

[kmadhusu, 2011-08-16 20:53:21]

http://codereview.chromium.org/7647010/diff/4001/chrome/renderer/mock_printer.cc
File chrome/renderer/mock_printer.cc (right):

http://codereview.chromium.org/7647010/diff/4001/chrome/renderer/mock_printer...
chrome/renderer/mock_printer.cc:105: memset(params, 0,
sizeof(PrintMsg_PrintPages_Params));
On 2011/08/16 20:08:53, Chris Guillory wrote:
> Looking at this further I think line is your issue. You are using memset to
> clear a std::vector isn't valid (especially if the vector has data). If you
> remove this it looks like the problem goes away.

hmm.. interesting.. I tested without the memset line it is fine. I think Aayush
has a fix to replace the memset. Let me update the CL once he lands his CL.
Thanks for your help Chris.

[kmadhusu, 2011-08-29 16:52:10]

vandebo: Please review the latest patch.

[vandebo (ex-Chrome), 2011-08-29 18:21:56]

LGTM with comments.

http://codereview.chromium.org/7647010/diff/31001/chrome/browser/printing/pri...
File chrome/browser/printing/print_preview_data_service.h (right):

http://codereview.chromium.org/7647010/diff/31001/chrome/browser/printing/pri...
chrome/browser/printing/print_preview_data_service.h:44: bool
IsDraftPagesAvailable(const std::string& preview_ui_addr_str,
This API is pretty fragile, you only get the right answer if you pass in the
correct count of draft pages.  Maybe it would be better to make more explicit
what information is available.  i.e. Make this method return the number of draft
pages stored.  Then the caller could compare that to the number it knows should
be there to come to the answer it needs.

http://codereview.chromium.org/7647010/diff/31001/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/page_settings.js (left):

http://codereview.chromium.org/7647010/diff/31001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/page_settings.js:277:
this.previouslySelectedPages_ = this.selectedPagesSet;
Do you want to call updatePageSelection here?

http://codereview.chromium.org/7647010/diff/31001/chrome/browser/ui/webui/pri...
File chrome/browser/ui/webui/print_preview_handler.cc (right):

http://codereview.chromium.org/7647010/diff/31001/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_handler.cc:523: bool generate_draft_data;
nit: set default (false) here?

http://codereview.chromium.org/7647010/diff/31001/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_handler.cc:531: int draft_page_count;
nit: default here?

http://codereview.chromium.org/7647010/diff/31001/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_handler.cc:532: bool preview_modifiable;
need default

http://codereview.chromium.org/7647010/diff/31001/chrome/renderer/print_web_v...
File chrome/renderer/print_web_view_helper.cc (right):

http://codereview.chromium.org/7647010/diff/31001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:673: bool generate_draft_pages;
default value?

http://codereview.chromium.org/7647010/diff/31001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:1367: print_ready_metafile_page_count_
= pages.empty() ? total_page_count_ :
Just use an if here...
count_ = selected_page_count;
if (pages.empty())
  count_ = total_page_count_;

http://codereview.chromium.org/7647010/diff/31001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:1371: // Render all pages.
Acutally, you already have the if...

http://codereview.chromium.org/7647010/diff/31001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:1441: render_page_list_index_++;
nit: Maybe init to 0 and post increment is clenaer?

http://codereview.chromium.org/7647010/diff/31001/chrome/renderer/print_web_v...
File chrome/renderer/print_web_view_helper.h (left):

http://codereview.chromium.org/7647010/diff/31001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.h:389: // The current page to render.
nit: These names seem fine.

http://codereview.chromium.org/7647010/diff/31001/chrome/renderer/print_web_v...
File chrome/renderer/print_web_view_helper_mac.mm (right):

http://codereview.chromium.org/7647010/diff/31001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper_mac.mm:80: // If the page needs to be in
both draft metafile and print ready metafile,
nit: comment before code.

http://codereview.chromium.org/7647010/diff/31001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper_mac.mm:157:
printing::MetafileSkiaWrapper::SetMetafileOnCanvas(canvasPtr, metafile);
nit: get rid of the variable and canvasPtr -> canvas.get()

[kmadhusu, 2011-08-30 17:27:00]

Fixed nits and browser test failures.

http://codereview.chromium.org/7647010/diff/31001/chrome/browser/printing/pri...
File chrome/browser/printing/print_preview_data_service.h (right):

http://codereview.chromium.org/7647010/diff/31001/chrome/browser/printing/pri...
chrome/browser/printing/print_preview_data_service.h:44: bool
IsDraftPagesAvailable(const std::string& preview_ui_addr_str,
On 2011/08/29 18:21:56, vandebo wrote:
> This API is pretty fragile, you only get the right answer if you pass in the
> correct count of draft pages.  Maybe it would be better to make more explicit
> what information is available.  i.e. Make this method return the number of
draft
> pages stored.  Then the caller could compare that to the number it knows
should
> be there to come to the answer it needs.

Done.

http://codereview.chromium.org/7647010/diff/31001/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/page_settings.js (left):

http://codereview.chromium.org/7647010/diff/31001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/page_settings.js:277:
this.previouslySelectedPages_ = this.selectedPagesSet;
On 2011/08/29 18:21:56, vandebo wrote:
> Do you want to call updatePageSelection here?

No. I do that in requestPrintPreview.

http://codereview.chromium.org/7647010/diff/31001/chrome/browser/ui/webui/pri...
File chrome/browser/ui/webui/print_preview_handler.cc (right):

http://codereview.chromium.org/7647010/diff/31001/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_handler.cc:523: bool generate_draft_data;
On 2011/08/29 18:21:56, vandebo wrote:
> nit: set default (false) here?

Default value is not required. If GetBoolean fails, "generate_draft_data" is
reset to false. As we discussed, I have modified the code to explicitly specify
the default value.

http://codereview.chromium.org/7647010/diff/31001/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_handler.cc:531: int draft_page_count;
On 2011/08/29 18:21:56, vandebo wrote:
> nit: default here?

Done.

http://codereview.chromium.org/7647010/diff/31001/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_handler.cc:532: bool preview_modifiable;
On 2011/08/29 18:21:56, vandebo wrote:
> need default

Done.

http://codereview.chromium.org/7647010/diff/31001/chrome/renderer/print_web_v...
File chrome/renderer/print_web_view_helper.cc (right):

http://codereview.chromium.org/7647010/diff/31001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:673: bool generate_draft_pages;
On 2011/08/29 18:21:56, vandebo wrote:
> default value?

Done.

http://codereview.chromium.org/7647010/diff/31001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:1367: print_ready_metafile_page_count_
= pages.empty() ? total_page_count_ :
On 2011/08/29 18:21:56, vandebo wrote:
> Just use an if here...
> count_ = selected_page_count;
> if (pages.empty())
>   count_ = total_page_count_;

Done.

http://codereview.chromium.org/7647010/diff/31001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:1371: // Render all pages.
On 2011/08/29 18:21:56, vandebo wrote:
> Acutally, you already have the if...

Fixed.

http://codereview.chromium.org/7647010/diff/31001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:1441: render_page_list_index_++;
On 2011/08/29 18:21:56, vandebo wrote:
> nit: Maybe init to 0 and post increment is clenaer?

Done.

http://codereview.chromium.org/7647010/diff/31001/chrome/renderer/print_web_v...
File chrome/renderer/print_web_view_helper.h (left):

http://codereview.chromium.org/7647010/diff/31001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.h:389: // The current page to render.
On 2011/08/29 18:21:56, vandebo wrote:
> nit: These names seem fine.

Done.

http://codereview.chromium.org/7647010/diff/31001/chrome/renderer/print_web_v...
File chrome/renderer/print_web_view_helper_mac.mm (right):

http://codereview.chromium.org/7647010/diff/31001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper_mac.mm:80: // If the page needs to be in
both draft metafile and print ready metafile,
On 2011/08/29 18:21:56, vandebo wrote:
> nit: comment before code.

This is a general information about the workflow. It is not related to the code
in line 78. So, I am leaving as it is.

http://codereview.chromium.org/7647010/diff/31001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper_mac.mm:157:
printing::MetafileSkiaWrapper::SetMetafileOnCanvas(canvasPtr, metafile);
On 2011/08/29 18:21:56, vandebo wrote:
> nit: get rid of the variable and canvasPtr -> canvas.get()

Done.

[vandebo (ex-Chrome), 2011-08-30 21:18:42]

LGTM

http://codereview.chromium.org/7647010/diff/31001/chrome/renderer/print_web_v...
File chrome/renderer/print_web_view_helper_mac.mm (right):

http://codereview.chromium.org/7647010/diff/31001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper_mac.mm:80: // If the page needs to be in
both draft metafile and print ready metafile,
On 2011/08/30 17:27:00, kmadhusu wrote:
> On 2011/08/29 18:21:56, vandebo wrote:
> > nit: comment before code.
> 
> This is a general information about the workflow. It is not related to the
code
> in line 78. So, I am leaving as it is.

But it is related - it's the reason that the boolean is set differently with or
without Skia.

[Lei Zhang, 2011-09-01 05:16:51]

http://codereview.chromium.org/7647010/diff/30008/chrome/renderer/print_web_v...
File chrome/renderer/print_web_view_helper.cc (right):

http://codereview.chromium.org/7647010/diff/30008/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:1372: for (int i = 0; i <
total_page_count_; i++)
nit: ++i here and below.

http://codereview.chromium.org/7647010/diff/30008/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:1375: int pages_index = 0;
So if I understand this correctly, if the user opens a 1000 page document, then
immediately selects page 2, we'll generate a metafile for page 2, and then go
back and generate the other 999 pages immediately, rather than on-demand?

http://codereview.chromium.org/7647010/diff/30008/chrome/renderer/print_web_v...
File chrome/renderer/print_web_view_helper_browsertest.cc (right):

http://codereview.chromium.org/7647010/diff/30008/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper_browsertest.cc:394:
TEST_F(PrintWebViewHelperPreviewTest, OnPrintPreviewForSelectedPages) {
You should add a test case using kLongPageHTML. Testing seleting page 1 on a 1
page document is not very interesting.

[kmadhusu, 2011-09-01 20:21:16]

http://codereview.chromium.org/7647010/diff/30008/chrome/renderer/print_web_v...
File chrome/renderer/print_web_view_helper.cc (right):

http://codereview.chromium.org/7647010/diff/30008/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:1372: for (int i = 0; i <
total_page_count_; i++)
On 2011/09/01 05:16:51, Lei Zhang wrote:
> nit: ++i here and below.

will submit a new CL to address this.

http://codereview.chromium.org/7647010/diff/30008/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:1375: int pages_index = 0;
On 2011/09/01 05:16:51, Lei Zhang wrote:
> So if I understand this correctly, if the user opens a 1000 page document,
then
> immediately selects page 2, we'll generate a metafile for page 2, and then go
> back and generate the other 999 pages immediately, rather than on-demand?

Yes. We generate 999 draft pages only if required. If the browser has all the
required draft pages then we will generate only the complete metafile for the
selected page range.

http://codereview.chromium.org/7647010/diff/30008/chrome/renderer/print_web_v...
File chrome/renderer/print_web_view_helper_browsertest.cc (right):

http://codereview.chromium.org/7647010/diff/30008/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper_browsertest.cc:394:
TEST_F(PrintWebViewHelperPreviewTest, OnPrintPreviewForSelectedPages) {
On 2011/09/01 05:16:51, Lei Zhang wrote:
> You should add a test case using kLongPageHTML. Testing seleting page 1 on a 1
> page document is not very interesting.

This test is to verify that we generate only the complete metafile and not the
draft metafile. I dont think it will make a difference if the initiator page is
long.