Fix the print preview regression bug caused by r98926.

Fix the print preview regression bug caused by r98926.

If we are previewing a PDF file, always invoke "PrepareFrameAndPrint"
FinishPrinting() before calling metafile FinishDocument() function.

If we are previewing a normal html page, we can invoke FinishDocument()
as soon as the print ready document is ready and then we can call
FinishPrinting() after rendering all the draft pages.

BUG=94998
TEST=Preview PDFs results in valid preview data.


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

[Lei Zhang, 2011-09-01 05:26:07]

LGTM

Are these rules about the Finish*() ordering documented anywhere? It seems this
is fragile and easy to mess up, and we don't have automated tests to detect this
error.

[kmadhusu, 2011-09-01 18:40:20]

vandebo: As we discussed I have refactored the code to make it less fragile.

thestig: PTAL at patch 2.

Thanks.

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

http://codereview.chromium.org/7822014/diff/4001/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper.cc:705: while
(!print_preview_context_.IsFinalPageRendered()) {
GetNextPageNumber() function needs to be called when print_preview_context_ is
in "RENDERING" state. Now that we can change the print_preview_context_ to a
"DONE" state within the loop. I have changed the loop condition here.

[vandebo (ex-Chrome), 2011-09-01 18:53:56]

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

http://codereview.chromium.org/7822014/diff/4001/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper.cc:714: // AllPagesRendered() will be
called only once i.e., after rendering the
I wouldn't include the second part of this comment, but make the first part more
focused....
// We must call PrepareFrameAndViewForPrint::FinishPrinting() (by way of
print_preview_context_.AllPagesRendered()) before calling
FinalizePrintReadyDocument() when printing a PDF because the plugin code does
not generate output until we call FinishPrinting.  We do not generate draft
pages for PDFs, so IsFinalPageRendered and IsLastPageOfPrintReadyMetafile will
be true in the same iteration of the loop.

http://codereview.chromium.org/7822014/diff/4001/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper.cc:725: if
(print_preview_context_.IsLastPageOfPrintReadyMetafile()) {
To help enforce the comment, you can add a DCHECK here...
DCHECK(IsModifiable() || IsFinalPageRendered()) ?

http://codereview.chromium.org/7822014/diff/4001/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper.cc:1486: return
(size_t)current_page_index_ == pages_to_render_.size();
static_cast<size_t>

[kmadhusu, 2011-09-01 20:07:14]

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

http://codereview.chromium.org/7822014/diff/4001/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper.cc:714: // AllPagesRendered() will be
called only once i.e., after rendering the
On 2011/09/01 18:53:56, vandebo wrote:
> I wouldn't include the second part of this comment, but make the first part
more
> focused....
> // We must call PrepareFrameAndViewForPrint::FinishPrinting() (by way of
> print_preview_context_.AllPagesRendered()) before calling
> FinalizePrintReadyDocument() when printing a PDF because the plugin code does
> not generate output until we call FinishPrinting.  We do not generate draft
> pages for PDFs, so IsFinalPageRendered and IsLastPageOfPrintReadyMetafile will
> be true in the same iteration of the loop.

Done..

http://codereview.chromium.org/7822014/diff/4001/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper.cc:725: if
(print_preview_context_.IsLastPageOfPrintReadyMetafile()) {
On 2011/09/01 18:53:56, vandebo wrote:
> To help enforce the comment, you can add a DCHECK here...
> DCHECK(IsModifiable() || IsFinalPageRendered()) ?

Done.

http://codereview.chromium.org/7822014/diff/4001/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper.cc:1486: return
(size_t)current_page_index_ == pages_to_render_.size();
On 2011/09/01 18:53:56, vandebo wrote:
> static_cast<size_t>

Done.

[vandebo (ex-Chrome), 2011-09-01 20:25:03]

C++ LGTM

[kmadhusu, 2011-09-01 20:25:18]

dpapad: Can you please review js code?

[dpapad, 2011-09-01 21:20:53]

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

http://codereview.chromium.org/7822014/diff/4004/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/print_preview.js:924: if
(pageSettings.requestPrintPreviewIfNeeded())
Repeating our in person discussion, it seems better to remove these 2 checks and
add a single check in onDidGetPreviewPageCount().

[kmadhusu, 2011-09-01 21:38:34]

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

http://codereview.chromium.org/7822014/diff/4004/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/print_preview.js:924: if
(pageSettings.requestPrintPreviewIfNeeded())
On 2011/09/01 21:20:53, dpapad wrote:
> Repeating our in person discussion, it seems better to remove these 2 checks
and
> add a single check in onDidGetPreviewPageCount().

Done.

[dpapad, 2011-09-01 21:45:05]

LGTM for js.

[commit-bot: I haz the power, 2011-09-02 09:04:41]

Change committed as 99344