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.

1. Added a new param |clear all preview data| to |PrintHostMsg_DidGetPreviewPageCount|.
2. Removed |requested_preview_page_index| from |PrintMsg_ContinuePreview|


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

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

[dpapad, 2011-08-01 17:51:40]

Still testing some cases.

http://codereview.chromium.org/7544018/diff/1/chrome/browser/resources/print_...
File chrome/browser/resources/print_preview/page_settings.js (left):

http://codereview.chromium.org/7544018/diff/1/chrome/browser/resources/print_...
chrome/browser/resources/print_preview/page_settings.js:277:
this.previouslySelectedPages_ = this.selectedPagesSet;
This seems to leave the object in an inconsistent state, why was it removed?

http://codereview.chromium.org/7544018/diff/1/chrome/browser/resources/print_...
File chrome/browser/resources/print_preview/print_preview.js (right):

http://codereview.chromium.org/7544018/diff/1/chrome/browser/resources/print_...
chrome/browser/resources/print_preview/print_preview.js:432: function
LoadSelectedPages() {
Nit: Function names start with lowercase. Also maybe a more descriptive name
would be displaySelectedPages.

http://codereview.chromium.org/7544018/diff/1/chrome/browser/resources/print_...
chrome/browser/resources/print_preview/print_preview.js:862: var pageIndex =
pageSettings.previouslySelectedPages.indexOf(pageNumber + 1);
Move this to where |pageIndex| is first used.

http://codereview.chromium.org/7544018/diff/1/chrome/browser/resources/print_...
chrome/browser/resources/print_preview/print_preview.js:867: if (pageIndex ==
-1)
Is this possible?

http://codereview.chromium.org/7544018/diff/1/chrome/browser/resources/print_...
chrome/browser/resources/print_preview/print_preview.js:905:
createPDFPlugin(-1);
Create a constant
const ALL_PAGES = -1;
and use 
createPDFPlugin(ALL_PAGES);
for more readability.

http://codereview.chromium.org/7544018/diff/1/chrome/browser/resources/print_...
chrome/browser/resources/print_preview/print_preview.js:939: * Check if only
page selection has been changed and is valid.
Document since when the page selection is being checked (since last preview
request?).

http://codereview.chromium.org/7544018/diff/1/chrome/browser/resources/print_...
chrome/browser/resources/print_preview/print_preview.js:940: * @return {boolean}
true if the nwe page selection is valid.
s/nwe/new

http://codereview.chromium.org/7544018/diff/1/chrome/browser/resources/print_...
chrome/browser/resources/print_preview/print_preview.js:942: function
isOnlyPageSettingsChanged() {
Nit (optional): Maybe haveOnlyPageSettingsChanged is more correct grammatically.

http://codereview.chromium.org/7544018/diff/1/chrome/browser/resources/print_...
chrome/browser/resources/print_preview/print_preview.js:953: * @param {number}
srcDataIndex Preview data source index.
Document the meaning of -1.

http://codereview.chromium.org/7544018/diff/1/chrome/browser/resources/print_...
chrome/browser/resources/print_preview/print_preview.js:991:
dummyPlugin.printPreviewPageCount);
Check also for newly added method resetPrintPreviewUrl.

[kmadhusu, 2011-08-02 22:53:45]

dpapad: Addressed review comments.

http://codereview.chromium.org/7544018/diff/1/chrome/browser/resources/print_...
File chrome/browser/resources/print_preview/page_settings.js (left):

http://codereview.chromium.org/7544018/diff/1/chrome/browser/resources/print_...
chrome/browser/resources/print_preview/page_settings.js:277:
this.previouslySelectedPages_ = this.selectedPagesSet;
On 2011/08/01 17:51:40, dpapad wrote:
> This seems to leave the object in an inconsistent state, why was it removed?

I have called updatePageSelection() in requestPrintPreview() to set the state of
|previouslySelectedPages_|.

http://codereview.chromium.org/7544018/diff/1/chrome/browser/resources/print_...
File chrome/browser/resources/print_preview/print_preview.js (right):

http://codereview.chromium.org/7544018/diff/1/chrome/browser/resources/print_...
chrome/browser/resources/print_preview/print_preview.js:432: function
LoadSelectedPages() {
On 2011/08/01 17:51:40, dpapad wrote:
> Nit: Function names start with lowercase.

Done

> Also maybe a more descriptive name would be displaySelectedPages.

I felt loadSelectedPages is more apt for this function, since we are asking the
plugin to load the preview urls.

http://codereview.chromium.org/7544018/diff/1/chrome/browser/resources/print_...
chrome/browser/resources/print_preview/print_preview.js:862: var pageIndex =
pageSettings.previouslySelectedPages.indexOf(pageNumber + 1);
On 2011/08/01 17:51:40, dpapad wrote:
> Move this to where |pageIndex| is first used.

Done.

http://codereview.chromium.org/7544018/diff/1/chrome/browser/resources/print_...
chrome/browser/resources/print_preview/print_preview.js:867: if (pageIndex ==
-1)
On 2011/08/01 17:51:40, dpapad wrote:
> Is this possible?

Yes it is possible now. When the user changes the layout and enter some page
ranges, we generate the draft pages for all preview pages. We only need to load
the requested selected pages.

http://codereview.chromium.org/7544018/diff/1/chrome/browser/resources/print_...
chrome/browser/resources/print_preview/print_preview.js:905:
createPDFPlugin(-1);
On 2011/08/01 17:51:40, dpapad wrote:
> Create a constant
> const ALL_PAGES = -1;
> and use 
> createPDFPlugin(ALL_PAGES);
> for more readability.

created const COMPLETE_METAFILE_DATA_INDEX

http://codereview.chromium.org/7544018/diff/1/chrome/browser/resources/print_...
chrome/browser/resources/print_preview/print_preview.js:939: * Check if only
page selection has been changed and is valid.
On 2011/08/01 17:51:40, dpapad wrote:
> Document since when the page selection is being checked (since last preview
> request?).

Done.

http://codereview.chromium.org/7544018/diff/1/chrome/browser/resources/print_...
chrome/browser/resources/print_preview/print_preview.js:940: * @return {boolean}
true if the nwe page selection is valid.
On 2011/08/01 17:51:40, dpapad wrote:
> s/nwe/new

Done.

http://codereview.chromium.org/7544018/diff/1/chrome/browser/resources/print_...
chrome/browser/resources/print_preview/print_preview.js:942: function
isOnlyPageSettingsChanged() {
On 2011/08/01 17:51:40, dpapad wrote:
> Nit (optional): Maybe haveOnlyPageSettingsChanged is more correct
grammatically.

changed to hasOnlyPageSettingsChanged

http://codereview.chromium.org/7544018/diff/1/chrome/browser/resources/print_...
chrome/browser/resources/print_preview/print_preview.js:953: * @param {number}
srcDataIndex Preview data source index.
On 2011/08/01 17:51:40, dpapad wrote:
> Document the meaning of -1.

I think the const variable defined here is descriptive.

http://codereview.chromium.org/7544018/diff/1/chrome/browser/resources/print_...
chrome/browser/resources/print_preview/print_preview.js:991:
dummyPlugin.printPreviewPageCount);
On 2011/08/01 17:51:40, dpapad wrote:
> Check also for newly added method resetPrintPreviewUrl.

Done.

[kmadhusu, 2011-08-02 22:53:59]

vandebo: Please review the c++ code.

[vandebo (ex-Chrome), 2011-08-03 20:48:20]

I didn't look at the JS

http://codereview.chromium.org/7544018/diff/8013/chrome/renderer/print_web_vi...
File chrome/renderer/print_web_view_helper.cc (right):

http://codereview.chromium.org/7544018/diff/8013/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper.cc:385: if
(!print_preview_context_.IsCompleteMetafileReady())
I think we want to return except for the one page on the transition from
complete to draft only.  As is, this can call FinalizePreviewDocument multiple
times

http://codereview.chromium.org/7544018/diff/8013/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper.cc:408: if
(is_complete_metafile_already_sent_)
Ohh, I see you handle that here... I think it would be cleaner to handle it in
the caller.  i.e. when we finish the complete metafile (when we hit that
transition point), do some extra stuff.

http://codereview.chromium.org/7544018/diff/8013/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper.cc:1012: render_page_list_ = pages;
what about if pages is empty?  In most cases it'll get handled by the following
if, but I think not all cases.  I.e. you select a subset of pages and then
select all pages again without changing the layout.

http://codereview.chromium.org/7544018/diff/8013/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper.cc:1015: if (std::find(pages.begin(),
pages.end(), i) == pages.end())
if render_page_list_ is guaranteed to be in order, you can do better than n^2

http://codereview.chromium.org/7544018/diff/8013/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper.cc:1037:
prep_frame_view_->FinishPrinting();
Is this left over from trying to do PDFs?

http://codereview.chromium.org/7544018/diff/8013/chrome/renderer/print_web_vi...
File chrome/renderer/print_web_view_helper.h (right):

http://codereview.chromium.org/7544018/diff/8013/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper.h:343: generate_draft_pages_ =
generate_draft_pages;
Put the implementation in the .cc and move this down with the rest of the
getters

http://codereview.chromium.org/7544018/diff/8013/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper.h:379: // Store the preview page index
list.
This comment is opaque, I'm not even sure what it means.

http://codereview.chromium.org/7544018/diff/8013/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper.h:388: // Specifies the current list
index.
Probably best to put this next to render_page_list

http://codereview.chromium.org/7544018/diff/8013/chrome/renderer/print_web_vi...
File chrome/renderer/print_web_view_helper_linux.cc (right):

http://codereview.chromium.org/7544018/diff/8013/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper_linux.cc:33:
is_complete_metafile_already_sent_ ? new printing::PreviewMetafile() :
It looks like metafile will leak in the first case...
scoped_ptr<print::Metafile> draft_metafile;
printing::Metafile* initial_render_metafile = print_preview_context_.metafile();
if (is_complete_metafile_already_sent_) {
  DCHECK(print_preview_context_.IsModifiable());
  draft_metafile.reset(new printing::PreviewMetafie);
  initial_render_metafile = draft_metafile.get();
}

http://codereview.chromium.org/7544018/diff/8013/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper_linux.cc:42: printing::Metafile*
page_metafile = NULL;
page_metafile = initial_render_metafile;

http://codereview.chromium.org/7544018/diff/8013/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper_linux.cc:43: if
(print_preview_context_.IsModifiable()) {
if (.... & !is_complete_metafile_already_sent) {

[dpapad, 2011-08-03 22:16:45]

http://codereview.chromium.org/7544018/diff/8013/chrome/browser/resources/pri...
File chrome/browser/resources/print_preview/print_preview.js (right):

http://codereview.chromium.org/7544018/diff/8013/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/print_preview.js:441: for (var i =0; i <
pageCount; i++)
nit: space after =

http://codereview.chromium.org/7544018/diff/8013/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/print_preview.js:442:
onDidPreviewPage(pageSet[i]-1, currentPreviewUid);
nit: spaces around binary operators.

http://codereview.chromium.org/7544018/diff/8013/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/print_preview.js:972: 
Nit: Remove blank line.

[kmadhusu, 2011-08-04 20:47:53]

dpapad, vandebo: Addressed review comments.

http://codereview.chromium.org/7544018/diff/8013/chrome/browser/resources/pri...
File chrome/browser/resources/print_preview/print_preview.js (right):

http://codereview.chromium.org/7544018/diff/8013/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/print_preview.js:441: for (var i =0; i <
pageCount; i++)
On 2011/08/03 22:16:46, dpapad wrote:
> nit: space after =

Done.

http://codereview.chromium.org/7544018/diff/8013/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/print_preview.js:442:
onDidPreviewPage(pageSet[i]-1, currentPreviewUid);
On 2011/08/03 22:16:46, dpapad wrote:
> nit: spaces around binary operators.

Done.

http://codereview.chromium.org/7544018/diff/8013/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/print_preview.js:972: 
On 2011/08/03 22:16:46, dpapad wrote:
> Nit: Remove blank line.

Done.

http://codereview.chromium.org/7544018/diff/8013/chrome/renderer/print_web_vi...
File chrome/renderer/print_web_view_helper.cc (right):

http://codereview.chromium.org/7544018/diff/8013/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper.cc:385: if
(!print_preview_context_.IsCompleteMetafileReady())
On 2011/08/03 20:48:21, vandebo wrote:
> I think we want to return except for the one page on the transition from
> complete to draft only.  As is, this can call FinalizePreviewDocument multiple
> times

As we discussed,  I renamed the function "IsCompleteMetafileReady" to
"IsLastPageOfCompleteMetafile".

http://codereview.chromium.org/7544018/diff/8013/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper.cc:408: if
(is_complete_metafile_already_sent_)
On 2011/08/03 20:48:21, vandebo wrote:
> Ohh, I see you handle that here... I think it would be cleaner to handle it in
> the caller.  i.e. when we finish the complete metafile (when we hit that
> transition point), do some extra stuff.

Done.

http://codereview.chromium.org/7544018/diff/8013/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper.cc:1012: render_page_list_ = pages;
On 2011/08/03 20:48:21, vandebo wrote:
> what about if pages is empty?  In most cases it'll get handled by the
following
> if, but I think not all cases.  I.e. you select a subset of pages and then
> select all pages again without changing the layout.

As we discussed, I added a DCHECK(generate_drate_pages_) when |pages| is empty.

http://codereview.chromium.org/7544018/diff/8013/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper.cc:1015: if (std::find(pages.begin(),
pages.end(), i) == pages.end())
On 2011/08/03 20:48:21, vandebo wrote:
> if render_page_list_ is guaranteed to be in order, you can do better than n^2

Done.

http://codereview.chromium.org/7544018/diff/8013/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper.cc:1037:
prep_frame_view_->FinishPrinting();
On 2011/08/03 20:48:21, vandebo wrote:
> Is this left over from trying to do PDFs?

No.

http://codereview.chromium.org/7544018/diff/8013/chrome/renderer/print_web_vi...
File chrome/renderer/print_web_view_helper.h (right):

http://codereview.chromium.org/7544018/diff/8013/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper.h:343: generate_draft_pages_ =
generate_draft_pages;
On 2011/08/03 20:48:21, vandebo wrote:
> Put the implementation in the .cc and move this down with the rest of the
> getters

Done.

http://codereview.chromium.org/7544018/diff/8013/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper.h:379: // Store the preview page index
list.
On 2011/08/03 20:48:21, vandebo wrote:
> This comment is opaque, I'm not even sure what it means.

Fixed.

http://codereview.chromium.org/7544018/diff/8013/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper.h:388: // Specifies the current list
index.
On 2011/08/03 20:48:21, vandebo wrote:
> Probably best to put this next to render_page_list

Done.

http://codereview.chromium.org/7544018/diff/8013/chrome/renderer/print_web_vi...
File chrome/renderer/print_web_view_helper_linux.cc (right):

http://codereview.chromium.org/7544018/diff/8013/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper_linux.cc:33:
is_complete_metafile_already_sent_ ? new printing::PreviewMetafile() :
On 2011/08/03 20:48:21, vandebo wrote:
> It looks like metafile will leak in the first case...
> scoped_ptr<print::Metafile> draft_metafile;
> printing::Metafile* initial_render_metafile =
print_preview_context_.metafile();
> if (is_complete_metafile_already_sent_) {
>   DCHECK(print_preview_context_.IsModifiable());
>   draft_metafile.reset(new printing::PreviewMetafie);
>   initial_render_metafile = draft_metafile.get();
> }

Done.

http://codereview.chromium.org/7544018/diff/8013/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper_linux.cc:42: printing::Metafile*
page_metafile = NULL;
On 2011/08/03 20:48:21, vandebo wrote:
> page_metafile = initial_render_metafile;

Done.

http://codereview.chromium.org/7544018/diff/8013/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper_linux.cc:43: if
(print_preview_context_.IsModifiable()) {
On 2011/08/03 20:48:21, vandebo wrote:
> if (.... & !is_complete_metafile_already_sent) {

Done.

[vandebo (ex-Chrome), 2011-08-04 21:45:02]

Time to do the win & mac implementations?

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

http://codereview.chromium.org/7544018/diff/21002/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:353:
print_preview_context_.set_generate_draft_pages(generate_draft_pages);
If you like, you could make set_generate_draft_pages take settings, and do the
work of extracting the value in the setter.

http://codereview.chromium.org/7544018/diff/21002/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:368: routing_id(),
nit: 4 space indent.

http://codereview.chromium.org/7544018/diff/21002/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:385: if
(!print_preview_context_.IsLastPageOfCompleteMetafile())
This is still a bit confusing/interwoven...  I suggest adding method to pp
context to tell you if you're done (like GetNextPageNumber, but const) and
removing the early return in this block and then:
if (done) {PreviewPageRenderered()}
if (LastPageOfCompleteMF()) {Finalize()}
if (page_num is invalid) {Finished(), DidFinishPrinting(OK)}

http://codereview.chromium.org/7544018/diff/21002/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:1015: // When |pages| is empty, we
always generate all draft pages.
I was hoping for a bit more here...  "And render_page_list_ will be populated by
the draft_page below." or something like that.

http://codereview.chromium.org/7544018/diff/21002/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:1018: render_page_list_ = pages;
If you like, this can be done unconditionally - either before or after the if.

http://codereview.chromium.org/7544018/diff/21002/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:1022: int index = 0;
nit: pages_index ?

http://codereview.chromium.org/7544018/diff/21002/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:1054: PreviewPagesRendered();
I think the reordering suggestion I made in OnContinuePreview above may make
this unnecessary ?

http://codereview.chromium.org/7544018/diff/21002/chrome/renderer/print_web_v...
File chrome/renderer/print_web_view_helper.h (right):

http://codereview.chromium.org/7544018/diff/21002/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.h:292: bool
is_complete_metafile_already_sent_;
nit (if you like): already isn't really necessary.

http://codereview.chromium.org/7544018/diff/21002/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.h:379: // List of page indices that needs
to be rendered.
nit: needs -> need

http://codereview.chromium.org/7544018/diff/21002/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.h:388: // Specifies the total number of
pages in complete metafile.
nit: in the complete

http://codereview.chromium.org/7544018/diff/21002/chrome/renderer/print_web_v...
File chrome/renderer/print_web_view_helper_linux.cc (right):

http://codereview.chromium.org/7544018/diff/21002/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper_linux.cc:37:
DCHECK(print_preview_context_.IsModifiable());
nit: With IsModifiable in the the if condition, this DCHECK isn't needed.

http://codereview.chromium.org/7544018/diff/21002/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper_linux.cc:50: if
(is_complete_metafile_already_sent_) {
nit: if (page_metafile)

http://codereview.chromium.org/7544018/diff/21002/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper_linux.cc:221: bool draft_mode =
is_complete_metafile_already_sent_;
nit: No need for the variable.

[kmadhusu, 2011-08-05 22:55:21]

Implemented win & mac code.

vandebo, dpapad: Addressed review comments.

Thanks.

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

http://codereview.chromium.org/7544018/diff/21002/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:353:
print_preview_context_.set_generate_draft_pages(generate_draft_pages);
On 2011/08/04 21:45:02, vandebo wrote:
> If you like, you could make set_generate_draft_pages take settings, and do the
> work of extracting the value in the setter.

I prefer the existing code.

http://codereview.chromium.org/7544018/diff/21002/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:368: routing_id(),
On 2011/08/04 21:45:02, vandebo wrote:
> nit: 4 space indent.

Done.

http://codereview.chromium.org/7544018/diff/21002/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:385: if
(!print_preview_context_.IsLastPageOfCompleteMetafile())
On 2011/08/04 21:45:02, vandebo wrote:
> This is still a bit confusing/interwoven...  I suggest adding method to pp
> context to tell you if you're done (like GetNextPageNumber, but const) and
> removing the early return in this block and then:
> if (done) {PreviewPageRenderered()}
> if (LastPageOfCompleteMF()) {Finalize()}
> if (page_num is invalid) {Finished(), DidFinishPrinting(OK)}

Done.

http://codereview.chromium.org/7544018/diff/21002/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:1015: // When |pages| is empty, we
always generate all draft pages.
On 2011/08/04 21:45:02, vandebo wrote:
> I was hoping for a bit more here...  "And render_page_list_ will be populated
by
> the draft_page below." or something like that.

Done.

http://codereview.chromium.org/7544018/diff/21002/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:1018: render_page_list_ = pages;
On 2011/08/04 21:45:02, vandebo wrote:
> If you like, this can be done unconditionally - either before or after the if.

Done.

http://codereview.chromium.org/7544018/diff/21002/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:1022: int index = 0;
On 2011/08/04 21:45:02, vandebo wrote:
> nit: pages_index ?

Done.

http://codereview.chromium.org/7544018/diff/21002/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:1054: PreviewPagesRendered();
On 2011/08/04 21:45:02, vandebo wrote:
> I think the reordering suggestion I made in OnContinuePreview above may make
> this unnecessary ?

Removed.

http://codereview.chromium.org/7544018/diff/21002/chrome/renderer/print_web_v...
File chrome/renderer/print_web_view_helper.h (right):

http://codereview.chromium.org/7544018/diff/21002/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.h:292: bool
is_complete_metafile_already_sent_;
On 2011/08/04 21:45:02, vandebo wrote:
> nit (if you like): already isn't really necessary.

Done.

http://codereview.chromium.org/7544018/diff/21002/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.h:379: // List of page indices that needs
to be rendered.
On 2011/08/04 21:45:02, vandebo wrote:
> nit: needs -> need

Done.

http://codereview.chromium.org/7544018/diff/21002/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.h:388: // Specifies the total number of
pages in complete metafile.
On 2011/08/04 21:45:02, vandebo wrote:
> nit: in the complete

Done.

http://codereview.chromium.org/7544018/diff/21002/chrome/renderer/print_web_v...
File chrome/renderer/print_web_view_helper_linux.cc (right):

http://codereview.chromium.org/7544018/diff/21002/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper_linux.cc:37:
DCHECK(print_preview_context_.IsModifiable());
On 2011/08/04 21:45:02, vandebo wrote:
> nit: With IsModifiable in the the if condition, this DCHECK isn't needed.

oops.. my bad. Fixed.

http://codereview.chromium.org/7544018/diff/21002/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper_linux.cc:50: if
(is_complete_metafile_already_sent_) {
On 2011/08/04 21:45:02, vandebo wrote:
> nit: if (page_metafile)

Done.

http://codereview.chromium.org/7544018/diff/21002/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper_linux.cc:221: bool draft_mode =
is_complete_metafile_already_sent_;
On 2011/08/04 21:45:02, vandebo wrote:
> nit: No need for the variable.

Done.

[vandebo (ex-Chrome), 2011-08-08 20:11:46]

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

http://codereview.chromium.org/7544018/diff/33001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:393: if (!FinalizePreviewDocument())
nit: FinalizeCompleteMetafile ?  Actually we've been calling it the complete
metafile, but with page selection it's not really... it's the single document
with all the selected pages.. PageSelectedCompleteDocument ?  That's kind of
long.  Maybe we call this the preview document, and the other point
(PreviewPagesRendered) as DraftAndPreviewPages ?

http://codereview.chromium.org/7544018/diff/33001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:1016: // When |pages| is empty, we
always generate all draft pages.
nit: This comment explain things fine, but it occured to me, i.e. at your
discretion, that instead this if could just populate render_page_list with a
simple for loop and then have the if on line 1022 be an else if.

http://codereview.chromium.org/7544018/diff/33001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:1094: if
((size_t)(render_page_list_index_ + 1) >= render_page_list_.size())
This can use IsFinalPageRendered()

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

http://codereview.chromium.org/7544018/diff/33001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper_browsertest.cc:45:
dict->SetBoolean(printing::kSettingGenerateDraftData, true);
Should we create another test for generating the complete metafile only (without
draft pages)?

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

http://codereview.chromium.org/7544018/diff/33001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper_mac.mm:93: if
(print_preview_context_.IsModifiable()) {
Not sure it if would be cleaner, but if you like, you could remove line 93 and
make the else on 115 be else if(modifiable && ppc_.generate_draft_pages())

http://codereview.chromium.org/7544018/diff/33001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper_mac.mm:115: } else {
Actually, does this need to check ppc_generate_draft_metafile in either case?  I
suspect either this case or the Linux case is wrong or over constrained.

http://codereview.chromium.org/7544018/diff/33001/chrome/renderer/print_web_v...
File chrome/renderer/print_web_view_helper_win.cc (right):

http://codereview.chromium.org/7544018/diff/33001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper_win.cc:133: scoped_ptr<Metafile>
draft_metafile;
You probably need a rebase.

http://codereview.chromium.org/7544018/diff/33001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper_win.cc:141: is_complete_metafile_sent_?
&draft_metafile : &metafile);
Hmm, this probably needs to be exactly the same condition as the init of
draft_metafile above.  But that is fragile. This will probably make it easier:
http://codereview.chromium.org/7491074/

http://codereview.chromium.org/7544018/diff/33001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper_win.cc:153:
draft_metafile.reset(reinterpret_cast<printing::PreviewMetafile*>(
Add a DCHECK that draf_metafile is empty?

[kmadhusu, 2011-08-09 22:09:26]

Addressed review comments.

vandebo: Yet to address one of your previous comment which is to add a new
browser test. I am working on that.

Please review the latest patch.

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

http://codereview.chromium.org/7544018/diff/33001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:393: if (!FinalizePreviewDocument())
On 2011/08/08 20:11:46, vandebo wrote:
> nit: FinalizeCompleteMetafile ?  Actually we've been calling it the complete
> metafile, but with page selection it's not really... it's the single document
> with all the selected pages.. PageSelectedCompleteDocument ?  That's kind of
> long.  Maybe we call this the preview document, and the other point
> (PreviewPagesRendered) as DraftAndPreviewPages ?

FinalizeCompleteMetafile => FinalizePrintReadyDocument
PreviewPagesRendered => AllPagesRendered

+
PrintHostMsg_PagesReadyForPreview => PrintHostMsg_MetafileReadyForPrinting

http://codereview.chromium.org/7544018/diff/33001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:1016: // When |pages| is empty, we
always generate all draft pages.
On 2011/08/08 20:11:46, vandebo wrote:
> nit: This comment explain things fine, but it occured to me, i.e. at your
> discretion, that instead this if could just populate render_page_list with a
> simple for loop and then have the if on line 1022 be an else if.

Done.

http://codereview.chromium.org/7544018/diff/33001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:1094: if
((size_t)(render_page_list_index_ + 1) >= render_page_list_.size())
On 2011/08/08 20:11:46, vandebo wrote:
> This can use IsFinalPageRendered()

Done.

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

http://codereview.chromium.org/7544018/diff/33001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper_mac.mm:93: if
(print_preview_context_.IsModifiable()) {
On 2011/08/08 20:11:46, vandebo wrote:
> Not sure it if would be cleaner, but if you like, you could remove line 93 and
> make the else on 115 be else if(modifiable && ppc_.generate_draft_pages())

Done.

http://codereview.chromium.org/7544018/diff/33001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper_mac.mm:115: } else {
On 2011/08/08 20:11:46, vandebo wrote:
> Actually, does this need to check ppc_generate_draft_metafile in either case? 
I
> suspect either this case or the Linux case is wrong or over constrained.

Yes, I need to check ppc_generate_draft_pages(), because on page selection, I
just want the complete metafile and not the draft pages.

http://codereview.chromium.org/7544018/diff/33001/chrome/renderer/print_web_v...
File chrome/renderer/print_web_view_helper_win.cc (right):

http://codereview.chromium.org/7544018/diff/33001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper_win.cc:133: scoped_ptr<Metafile>
draft_metafile;
On 2011/08/08 20:11:46, vandebo wrote:
> You probably need a rebase.

Done.

http://codereview.chromium.org/7544018/diff/33001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper_win.cc:141: is_complete_metafile_sent_?
&draft_metafile : &metafile);
On 2011/08/08 20:11:46, vandebo wrote:
> Hmm, this probably needs to be exactly the same condition as the init of
> draft_metafile above.  But that is fragile. This will probably make it easier:
> http://codereview.chromium.org/7491074/

Rebased with your changes and made code changes accordingly.

http://codereview.chromium.org/7544018/diff/33001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper_win.cc:153:
draft_metafile.reset(reinterpret_cast<printing::PreviewMetafile*>(
On 2011/08/08 20:11:46, vandebo wrote:
> Add a DCHECK that draf_metafile is empty?

Done.

[kmadhusu, 2011-08-10 00:36:49]

vandebo: Added a new PrintWebViewHelperPreviewTest.

Thanks.

[vandebo (ex-Chrome), 2011-08-10 17:21:40]

Thanks for cleaning up the names.

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

http://codereview.chromium.org/7544018/diff/46001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:1017: DCHECK(generate_draft_pages_);
nit: This DCHECK isn't required with the if-else construct.

http://codereview.chromium.org/7544018/diff/46001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:1018: // When |pages| is empty, we
always generate all draft pages.
With the if-else, this comment isn't valid any more.

http://codereview.chromium.org/7544018/diff/46001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:1118: bool
PrintWebViewHelper::PrintPreviewContext::IsLastPageOfCompleteMetafile()
Another reference to complete metafile.

http://codereview.chromium.org/7544018/diff/46001/chrome/renderer/print_web_v...
File chrome/renderer/print_web_view_helper.h (right):

http://codereview.chromium.org/7544018/diff/46001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.h:300: bool is_complete_metafile_sent_;
I thought we were going to rename "complete metafile" things?

http://codereview.chromium.org/7544018/diff/46001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.h:401: int complete_metafile_page_count_;
Another complete metafile

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

http://codereview.chromium.org/7544018/diff/46001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper_mac.mm:74: #if defined(USE_SKIA)
The if inside an if is kind of hard to follow.  How about:
#if defines(USE_SKIA)
  bool need_draft_metafile = ...
#else
  bool need_draft_metafile = ...
#endif
if (need_draft_metafile) {...
?

http://codereview.chromium.org/7544018/diff/46001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper_mac.mm:77:
print_preview_context_.generate_draft_pages()) {
Maybe add a comment here says that for CG, if we want a draft metafile, we
render to that first and then copy that into the print ready metafile because CG
doesn't allow us to do it in the other order.

http://codereview.chromium.org/7544018/diff/46001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper_mac.mm:120: } else if
(print_preview_context_.IsModifiable() &&
This if only has a body for the use skia case.  Can we put the entire thing
inside the use skia ifdef, i.e. including the if part?  It may be easier to make
it an if instead of an else if.

http://codereview.chromium.org/7544018/diff/46001/chrome/renderer/print_web_v...
File chrome/renderer/print_web_view_helper_win.cc (right):

http://codereview.chromium.org/7544018/diff/46001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper_win.cc:145: DCHECK_EQ(render_page_result,
print_preview_context_.metafile());
Change the second DCHECK arg to initial_render_metafile and remove the if.

[kmadhusu, 2011-08-10 19:01:13]

vandebo: Addressed review comments.

Thanks

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

http://codereview.chromium.org/7544018/diff/46001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:1017: DCHECK(generate_draft_pages_);
On 2011/08/10 17:21:40, vandebo wrote:
> nit: This DCHECK isn't required with the if-else construct.

Removed.

http://codereview.chromium.org/7544018/diff/46001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:1018: // When |pages| is empty, we
always generate all draft pages.
On 2011/08/10 17:21:40, vandebo wrote:
> With the if-else, this comment isn't valid any more.

Fixed the comment.

http://codereview.chromium.org/7544018/diff/46001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:1118: bool
PrintWebViewHelper::PrintPreviewContext::IsLastPageOfCompleteMetafile()
On 2011/08/10 17:21:40, vandebo wrote:
> Another reference to complete metafile.

fixed

http://codereview.chromium.org/7544018/diff/46001/chrome/renderer/print_web_v...
File chrome/renderer/print_web_view_helper.h (right):

http://codereview.chromium.org/7544018/diff/46001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.h:300: bool is_complete_metafile_sent_;
On 2011/08/10 17:21:40, vandebo wrote:
> I thought we were going to rename "complete metafile" things?

is_complete_metafile_sent => is_print_ready_metafile_sent_

http://codereview.chromium.org/7544018/diff/46001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.h:401: int complete_metafile_page_count_;
On 2011/08/10 17:21:40, vandebo wrote:
> Another complete metafile

Done.

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

http://codereview.chromium.org/7544018/diff/46001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper_mac.mm:74: #if defined(USE_SKIA)
On 2011/08/10 17:21:40, vandebo wrote:
> The if inside an if is kind of hard to follow.  How about:
> #if defines(USE_SKIA)
>   bool need_draft_metafile = ...
> #else
>   bool need_draft_metafile = ...
> #endif
> if (need_draft_metafile) {...
> ?

Done.

http://codereview.chromium.org/7544018/diff/46001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper_mac.mm:77:
print_preview_context_.generate_draft_pages()) {
On 2011/08/10 17:21:40, vandebo wrote:
> Maybe add a comment here says that for CG, if we want a draft metafile, we
> render to that first and then copy that into the print ready metafile because
CG
> doesn't allow us to do it in the other order.

Done.

http://codereview.chromium.org/7544018/diff/46001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper_mac.mm:120: } else if
(print_preview_context_.IsModifiable() &&
On 2011/08/10 17:21:40, vandebo wrote:
> This if only has a body for the use skia case.  Can we put the entire thing
> inside the use skia ifdef, i.e. including the if part?  It may be easier to
make
> it an if instead of an else if.

Changed to:

if (draft_metafile.get()) {
..
} else {
#if defined(USE_SKIA)
  if (....) {
    ...
  }
#endif
}

http://codereview.chromium.org/7544018/diff/46001/chrome/renderer/print_web_v...
File chrome/renderer/print_web_view_helper_win.cc (right):

http://codereview.chromium.org/7544018/diff/46001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper_win.cc:145: DCHECK_EQ(render_page_result,
print_preview_context_.metafile());
On 2011/08/10 17:21:40, vandebo wrote:
> Change the second DCHECK arg to initial_render_metafile and remove the if.

Done.

[vandebo (ex-Chrome), 2011-08-10 20:02:54]

C++ LGTM w/ nits.

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

http://codereview.chromium.org/7544018/diff/50001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper_mac.mm:74: bool need_draft_metafile =
print_preview_context_.IsModifiable() &&
nit: need_draft_metafile -> render_to_draft_first  (we may need a draft
metafile, where this is false).

http://codereview.chromium.org/7544018/diff/50001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper_mac.mm:79: // NOTE: If the rendering page
need to be in draft metafile and print ready
nit: NOTE isn't needed.
remove 'rendering'
need -> needs
in -> in both

http://codereview.chromium.org/7544018/diff/50001/chrome/renderer/print_web_v...
File chrome/renderer/print_web_view_helper_win.cc (right):

http://codereview.chromium.org/7544018/diff/50001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper_win.cc:142: 
nit: extra line

[dpapad, 2011-08-10 20:26:51]

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

http://codereview.chromium.org/7544018/diff/50001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/page_settings.js:277:
this.previouslySelectedPages_ = this.selectedPagesSet;
Did you find any way to keep this line here? I feel that it is fragile removing
this.

http://codereview.chromium.org/7544018/diff/50001/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/print_preview.js (right):

http://codereview.chromium.org/7544018/diff/50001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/print_preview.js:26: const
COMPLETE_PREVIEW_DATA_INDEX = -1;
Nit: Leave a blank line between these constants and the constant you added, they
are not related (see the comment "Destination list special value constants.").

[kmadhusu, 2011-08-10 21:19:39]

Fixed nits.

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

http://codereview.chromium.org/7544018/diff/50001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/page_settings.js:277:
this.previouslySelectedPages_ = this.selectedPagesSet;
On 2011/08/10 20:26:51, dpapad wrote:
> Did you find any way to keep this line here? I feel that it is fragile
removing
> this.

I tested different cases and I did not find any issues regarding this. Apart
from that, we are still updating the page selection in print_preview.js.

http://codereview.chromium.org/7544018/diff/50001/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/print_preview.js (right):

http://codereview.chromium.org/7544018/diff/50001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/print_preview.js:26: const
COMPLETE_PREVIEW_DATA_INDEX = -1;
On 2011/08/10 20:26:51, dpapad wrote:
> Nit: Leave a blank line between these constants and the constant you added,
they
> are not related (see the comment "Destination list special value constants.").

Done.

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

http://codereview.chromium.org/7544018/diff/50001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper_mac.mm:74: bool need_draft_metafile =
print_preview_context_.IsModifiable() &&
On 2011/08/10 20:02:54, vandebo wrote:
> nit: need_draft_metafile -> render_to_draft_first  (we may need a draft
> metafile, where this is false).

need_draft_metafile => render_to_draft

http://codereview.chromium.org/7544018/diff/50001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper_mac.mm:79: // NOTE: If the rendering page
need to be in draft metafile and print ready
On 2011/08/10 20:02:54, vandebo wrote:
> nit: NOTE isn't needed.
> remove 'rendering'
> need -> needs
> in -> in both

Done.

http://codereview.chromium.org/7544018/diff/50001/chrome/renderer/print_web_v...
File chrome/renderer/print_web_view_helper_win.cc (right):

http://codereview.chromium.org/7544018/diff/50001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper_win.cc:142: 
On 2011/08/10 20:02:54, vandebo wrote:
> nit: extra line

Done.

[dpapad, 2011-08-10 21:52:49]

LGTM