Print Preview: Make preview generation event driven to eliminate synchronous messages.

Print Preview: Make preview generation event driven to eliminate synchronous messages.

BUG=89069
TEST=Print preview still works.

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

[kmadhusu, 2011-07-15 01:22:46]

http://codereview.chromium.org/7365003/diff/9001/chrome/browser/printing/prin...
File chrome/browser/printing/print_preview_message_handler.cc (right):

http://codereview.chromium.org/7365003/diff/9001/chrome/browser/printing/prin...
chrome/browser/printing/print_preview_message_handler.cc:82: if
(!print_preview_tab) {
Can you make this if condition (!print_preview_tab ||
!print_preview_tab->web_ui())?

http://codereview.chromium.org/7365003/diff/9001/chrome/common/print_messages.h
File chrome/common/print_messages.h (right):

http://codereview.chromium.org/7365003/diff/9001/chrome/common/print_messages...
chrome/common/print_messages.h:253: int  /* document cookie */)
(repeating our conversation): Send |document_cookie| along with
PrintHostMsg_DidGetPreviewPageCount and remove it from here.

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

http://codereview.chromium.org/7365003/diff/9001/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper.cc:201: OnContinuePreview)
nit: This argument will fit in the previous line.

http://codereview.chromium.org/7365003/diff/9001/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper.cc:203: OnCancelPreview)
same here

http://codereview.chromium.org/7365003/diff/9001/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper.cc:205: OnAbortPreview)
same here

http://codereview.chromium.org/7365003/diff/9001/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper.cc:358: DCHECK_GT(buf_size, 0u);
http://codereview.chromium.org/7365003/diff/9001/chrome/renderer/print_web_vi...

Ref line: 194 

It says, the metafile file size should be greater than 128u on Win.

http://codereview.chromium.org/7365003/diff/9001/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper.cc:409: if (GetPrintFrame(&frame))
Handle the else case.

http://codereview.chromium.org/7365003/diff/9001/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper.cc:683: WebKit::WebNode* node) {
Add DCHECK(frame) here.

http://codereview.chromium.org/7365003/diff/9001/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper.cc:834: bool too_frequent = false;
Declare this variable after the comments

http://codereview.chromium.org/7365003/diff/9001/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper.cc:935: DCHECK(IsInitialized());
If (!IsInitialized())
  return false;

is better than DCHECK()

http://codereview.chromium.org/7365003/diff/9001/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper.cc:946: frame(),
nit: frame() can fit in the previous line.

http://codereview.chromium.org/7365003/diff/9001/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper.cc:954: if (pages.empty()) {
How about:

actual_page_count = pages.empty()? page_count_ : pages.size();

if (actual_page_count_) {
line 956
} else {
  lines 959 - 964
}

http://codereview.chromium.org/7365003/diff/9001/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper.cc:960: for (size_t i = 0; i <
pages.size(); ++i) {
"i < actual_page_count_"

http://codereview.chromium.org/7365003/diff/9001/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper.cc:987:
prep_frame_view_->FinishPrinting();
Please retain the previous comment.

http://codereview.chromium.org/7365003/diff/9001/chrome/renderer/print_web_vi...

Ref line: 178

http://codereview.chromium.org/7365003/diff/9001/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper.cc:988: metafile_->FinishDocument();
if (!metafile->FinishDocument)
  NOTREACHED();

http://codereview.chromium.org/7365003/diff/9001/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper.cc:998: document_render_time_;
nit: Fix indentation.

http://codereview.chromium.org/7365003/diff/9001/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper.cc:1035: bool
PrintWebViewHelper::PrintPreviewContext::IsInitialized() const {
I was expecting "return state_ == INITIALIZED;" as the function definition. 

The function name is misleading. Either change the name or the definition.

http://codereview.chromium.org/7365003/diff/9001/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper.cc:1039: bool
PrintWebViewHelper::PrintPreviewContext::IsBusy() const {
This function needs a better name. 

state PRINTING == Busy
state INITIALIZED != BUSY ( it is a ready state).

http://codereview.chromium.org/7365003/diff/9001/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper.cc:1047: if (mime == "application/pdf")
Instead of lines 1047-1049, use 

return mime != "application/pdf";

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

http://codereview.chromium.org/7365003/diff/9001/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper.h:115: // Initialize the print preview
document.
nit: Please add blank line after lines 114, 119, 121.

http://codereview.chromium.org/7365003/diff/9001/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper.h:120: // Renders a print preview page.
Document |page_number|.

http://codereview.chromium.org/7365003/diff/9001/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper.h:125: // Cancel the preview to put it
into the 'DONE' state.
Define "it".

http://codereview.chromium.org/7365003/diff/9001/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper.h:128: // Abort the preview to put it into
the 'UNINITIALIZED' state.
Define "it".

http://codereview.chromium.org/7365003/diff/9001/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper.h:284: class PrintPreviewContext {
Document this class.

http://codereview.chromium.org/7365003/diff/9001/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper.h:286: PrintPreviewContext();
destructor??

http://codereview.chromium.org/7365003/diff/9001/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper.h:298: const std::vector<int>& pages);
document |pages|.

http://codereview.chromium.org/7365003/diff/9001/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper.h:317: bool GetNextPage(int* page_number);
How about GetNextPageNumber? Rather than returning a bool value, Can you return
the actual page number?

int page_number = print_preview_context_.GetNextPageNumber();

http://codereview.chromium.org/7365003/diff/9001/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper.h:334: PRINTING,       // Printing.
Since we are not actually printing, can you change PRINTING => RENDERING ?

http://codereview.chromium.org/7365003/diff/9001/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper.h:338: void Reset();
Document the purpose of this function.

http://codereview.chromium.org/7365003/diff/9001/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper.h:340: // Specifies what to print.
In PrintPreviewContext class:
s/print/print preview/g

http://codereview.chromium.org/7365003/diff/9001/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper.h:349: int page_count_;
To make it clear:
page_count_ => total_page_count_

http://codereview.chromium.org/7365003/diff/9001/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper.h:355: int current_page_;
current_page => current_page_number ?

http://codereview.chromium.org/7365003/diff/9001/chrome/renderer/print_web_vi...
File chrome/renderer/print_web_view_helper_win.cc (right):

http://codereview.chromium.org/7365003/diff/9001/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper_win.cc:131: scoped_ptr<Metafile>
metafile(print_preview_context_.metafile());
Please add a comment on why you are doing this.

[Lei Zhang, 2011-07-15 01:48:02]

http://codereview.chromium.org/7365003/diff/9001/chrome/browser/printing/prin...
File chrome/browser/printing/print_preview_message_handler.cc (right):

http://codereview.chromium.org/7365003/diff/9001/chrome/browser/printing/prin...
chrome/browser/printing/print_preview_message_handler.cc:82: if
(!print_preview_tab) {
On 2011/07/15 01:22:46, kmadhusu wrote:
> Can you make this if condition (!print_preview_tab ||
> !print_preview_tab->web_ui())?

I don't think we need it. I've never seen a crash on line 91.

http://codereview.chromium.org/7365003/diff/9001/chrome/common/print_messages.h
File chrome/common/print_messages.h (right):

http://codereview.chromium.org/7365003/diff/9001/chrome/common/print_messages...
chrome/common/print_messages.h:253: int  /* document cookie */)
On 2011/07/15 01:22:46, kmadhusu wrote:
> (repeating our conversation): Send |document_cookie| along with
> PrintHostMsg_DidGetPreviewPageCount and remove it from here.

Done.

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

http://codereview.chromium.org/7365003/diff/9001/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper.cc:201: OnContinuePreview)
On 2011/07/15 01:22:46, kmadhusu wrote:
> nit: This argument will fit in the previous line. 

Done.

http://codereview.chromium.org/7365003/diff/9001/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper.cc:203: OnCancelPreview)
On 2011/07/15 01:22:46, kmadhusu wrote:
> same here

Done.

http://codereview.chromium.org/7365003/diff/9001/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper.cc:205: OnAbortPreview)
On 2011/07/15 01:22:46, kmadhusu wrote:
> same here

Done.

http://codereview.chromium.org/7365003/diff/9001/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper.cc:358: DCHECK_GT(buf_size, 0u);
On 2011/07/15 01:22:46, kmadhusu wrote:
>
http://codereview.chromium.org/7365003/diff/9001/chrome/renderer/print_web_vi...
> 
> Ref line: 194 
> 
> It says, the metafile file size should be greater than 128u on Win. 
> 

That was a copy and paste from the Windows EMF printing code. Why does our PDF
file have to be > 128 bytes on Windows but not on Linux/Mac?

http://codereview.chromium.org/7365003/diff/9001/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper.cc:409: if (GetPrintFrame(&frame))
On 2011/07/15 01:22:46, kmadhusu wrote:
> Handle the else case.

The else case is to do nothing. I changed this to reflect that.

http://codereview.chromium.org/7365003/diff/9001/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper.cc:683: WebKit::WebNode* node) {
On 2011/07/15 01:22:46, kmadhusu wrote:
> Add DCHECK(frame) here.

Done.

http://codereview.chromium.org/7365003/diff/9001/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper.cc:834: bool too_frequent = false;
On 2011/07/15 01:22:46, kmadhusu wrote:
> Declare this variable after the comments

I don't think it helps with readability.

http://codereview.chromium.org/7365003/diff/9001/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper.cc:935: DCHECK(IsInitialized());
On 2011/07/15 01:22:46, kmadhusu wrote:
> If (!IsInitialized())
>   return false;
> 
> is better than DCHECK()

The DCHECK should never fail. If it fails in a release build, then just crash.
That way we will actually see the problem and we can figure out how to deal with
it. (One way to deal with it is to do what you suggested)

http://codereview.chromium.org/7365003/diff/9001/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper.cc:946: frame(),
On 2011/07/15 01:22:46, kmadhusu wrote:
> nit: frame() can fit in the previous line.

Done.

http://codereview.chromium.org/7365003/diff/9001/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper.cc:954: if (pages.empty()) {
On 2011/07/15 01:22:46, kmadhusu wrote:
> How about:
> 
> actual_page_count = pages.empty()? page_count_ : pages.size();
> 
> if (actual_page_count_) {
> line 956
> } else {
>   lines 959 - 964
> }

Your logic is wrong.

http://codereview.chromium.org/7365003/diff/9001/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper.cc:960: for (size_t i = 0; i <
pages.size(); ++i) {
On 2011/07/15 01:22:46, kmadhusu wrote:
> "i < actual_page_count_"

Done.

http://codereview.chromium.org/7365003/diff/9001/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper.cc:987:
prep_frame_view_->FinishPrinting();
On 2011/07/15 01:22:46, kmadhusu wrote:
> Please retain the previous comment.
> 
>
http://codereview.chromium.org/7365003/diff/9001/chrome/renderer/print_web_vi...
> 
> Ref line: 178

We have the comment on Windows only, but not Linux and Mac. I think we are fine
without it.

http://codereview.chromium.org/7365003/diff/9001/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper.cc:988: metafile_->FinishDocument();
On 2011/07/15 01:22:46, kmadhusu wrote:
> if (!metafile->FinishDocument)
>   NOTREACHED();

Again, we don't have it on Mac/Linux.

http://codereview.chromium.org/7365003/diff/9001/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper.cc:998: document_render_time_;
On 2011/07/15 01:22:46, kmadhusu wrote:
> nit: Fix indentation.

Done.

http://codereview.chromium.org/7365003/diff/9001/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper.cc:1035: bool
PrintWebViewHelper::PrintPreviewContext::IsInitialized() const {
On 2011/07/15 01:22:46, kmadhusu wrote:
> I was expecting "return state_ == INITIALIZED;" as the function definition. 
> 
> The function name is misleading. Either change the name or the definition. 

Changed to "IsReadyToRender()"

http://codereview.chromium.org/7365003/diff/9001/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper.cc:1039: bool
PrintWebViewHelper::PrintPreviewContext::IsBusy() const {
On 2011/07/15 01:22:46, kmadhusu wrote:
> This function needs a better name. 
> 
> state PRINTING == Busy
> state INITIALIZED != BUSY ( it is a ready state).

I can't think of a better name. "Busy" is relative and here INITIALIZED is
"busy".

http://codereview.chromium.org/7365003/diff/9001/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper.cc:1047: if (mime == "application/pdf")
On 2011/07/15 01:22:46, kmadhusu wrote:
> Instead of lines 1047-1049, use 
> 
> return mime != "application/pdf";

Done.

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

http://codereview.chromium.org/7365003/diff/9001/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper.h:115: // Initialize the print preview
document.
On 2011/07/15 01:22:46, kmadhusu wrote:
> nit: Please add blank line after lines 114, 119, 121.

This is a very subtle way of showing OnPrintPreview() and
CreatePreviewDocument() are related.

http://codereview.chromium.org/7365003/diff/9001/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper.h:120: // Renders a print preview page.
On 2011/07/15 01:22:46, kmadhusu wrote:
> Document |page_number|. 

Done.

http://codereview.chromium.org/7365003/diff/9001/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper.h:125: // Cancel the preview to put it
into the 'DONE' state.
On 2011/07/15 01:22:46, kmadhusu wrote:
> Define "it".

Done.

http://codereview.chromium.org/7365003/diff/9001/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper.h:128: // Abort the preview to put it into
the 'UNINITIALIZED' state.
On 2011/07/15 01:22:46, kmadhusu wrote:
> Define "it".

Done.

http://codereview.chromium.org/7365003/diff/9001/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper.h:284: class PrintPreviewContext {
On 2011/07/15 01:22:46, kmadhusu wrote:
> Document this class.

Done.

http://codereview.chromium.org/7365003/diff/9001/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper.h:286: PrintPreviewContext();
On 2011/07/15 01:22:46, kmadhusu wrote:
> destructor??

Got nothing to destroy.

http://codereview.chromium.org/7365003/diff/9001/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper.h:298: const std::vector<int>& pages);
On 2011/07/15 01:22:46, kmadhusu wrote:
> document |pages|.

Done.

http://codereview.chromium.org/7365003/diff/9001/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper.h:317: bool GetNextPage(int* page_number);
On 2011/07/15 01:22:46, kmadhusu wrote:
> How about GetNextPageNumber? Rather than returning a bool value, Can you
return
> the actual page number?
> 
> int page_number = print_preview_context_.GetNextPageNumber();

Done.

http://codereview.chromium.org/7365003/diff/9001/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper.h:334: PRINTING,       // Printing.
On 2011/07/15 01:22:46, kmadhusu wrote:
> Since we are not actually printing, can you change PRINTING => RENDERING ?

Done.

http://codereview.chromium.org/7365003/diff/9001/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper.h:338: void Reset();
On 2011/07/15 01:22:46, kmadhusu wrote:
> Document the purpose of this function.

Done.

http://codereview.chromium.org/7365003/diff/9001/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper.h:340: // Specifies what to print.
On 2011/07/15 01:22:46, kmadhusu wrote:
> In PrintPreviewContext class:
> s/print/print preview/g 

Done.

http://codereview.chromium.org/7365003/diff/9001/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper.h:349: int page_count_;
On 2011/07/15 01:22:46, kmadhusu wrote:
> To make it clear:
> page_count_ => total_page_count_

Done.

http://codereview.chromium.org/7365003/diff/9001/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper.h:355: int current_page_;
On 2011/07/15 01:22:46, kmadhusu wrote:
> current_page => current_page_number ?

Done.

http://codereview.chromium.org/7365003/diff/9001/chrome/renderer/print_web_vi...
File chrome/renderer/print_web_view_helper_win.cc (right):

http://codereview.chromium.org/7365003/diff/9001/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper_win.cc:131: scoped_ptr<Metafile>
metafile(print_preview_context_.metafile());
On 2011/07/15 01:22:46, kmadhusu wrote:
> Please add a comment on why you are doing this.

Done.

[kmadhusu, 2011-07-15 19:25:46]

(repeating our conversation): When I tested this CL, I saw two different
initiator tab crashes. The corresponding console debug messages are as follows:

Crash info - 1:
chrome --type=renderer --lang=en-US
--force-fieldtest=ConnCountImpact/conn_count_6/ConnnectBackupJobs/ConnectBackupJobsEnabled/DnsImpact/default_enabled_prefetch/DnsParallelism/parallel_default/GlobalSdch/global_enable_sdch/IdleSktToImpact/idle_timeout_10/Prefetch/ContentPrefetchPrerender1/ProxyConnectionImpact/proxy_connections_32/SpdyImpact/npn_with_spdy/WarmSocketImpact/warmest_socket/
--disable-client-side-phishing-detection --enable-print-preview
--channel=22984.0x7f9164dc5a00.942889956: ./base/memory/scoped_ptr.h:96: C*
scoped_ptr<C>::operator->() const [with C = PrepareFrameAndViewForPrint]:
Assertion `ptr_ != __null' failed.

Crash Info - 2:
ASSERTION FAILED: !needsLayout()
third_party/WebKit/Source/WebCore/page/FrameView.cpp(2473) : virtual void
WebCore::FrameView::paintContents(WebCore::GraphicsContext*, const
WebCore::IntRect&)

http://codereview.chromium.org/7365003/diff/9001/chrome/browser/printing/prin...
File chrome/browser/printing/print_preview_message_handler.cc (right):

http://codereview.chromium.org/7365003/diff/9001/chrome/browser/printing/prin...
chrome/browser/printing/print_preview_message_handler.cc:82: if
(!print_preview_tab) {
On 2011/07/15 01:48:02, Lei Zhang wrote:
> On 2011/07/15 01:22:46, kmadhusu wrote:
> > Can you make this if condition (!print_preview_tab ||
> > !print_preview_tab->web_ui())?
> 
> I don't think we need it. I've never seen a crash on line 91.

As we discussed, it can happen. So, modify the if condition.

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

http://codereview.chromium.org/7365003/diff/9001/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper.cc:954: if (pages.empty()) {
On 2011/07/15 01:48:02, Lei Zhang wrote:
> On 2011/07/15 01:22:46, kmadhusu wrote:
> > How about:
> > 
> > actual_page_count = pages.empty()? page_count_ : pages.size();
> > 
> > if (actual_page_count_) {
> > line 956
> > } else {
> >   lines 959 - 964
> > }
> 
> Your logic is wrong.

oops.. I meant if (actual_page_count_ == page_count_)

http://codereview.chromium.org/7365003/diff/9001/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper.cc:988: metafile_->FinishDocument();
On 2011/07/15 01:48:02, Lei Zhang wrote:
> On 2011/07/15 01:22:46, kmadhusu wrote:
> > if (!metafile->FinishDocument)
> >   NOTREACHED();
> 
> Again, we don't have it on Mac/Linux.

At least on linux, we should have handled this case.
PdfMetafileSkia::FinishDocument can return true/false. So its better to handle
it now.

http://codereview.chromium.org/7365003/diff/14002/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:1070: return *print_params_;
How about ClearPrintPreviewContext()? We are not changing any state in this
function definition.

http://codereview.chromium.org/7365003/diff/19002/chrome/common/print_messages.h
File chrome/common/print_messages.h (right):

http://codereview.chromium.org/7365003/diff/19002/chrome/common/print_message...
chrome/common/print_messages.h:177: IPC_MESSAGE_ROUTED0(PrintMsg_CancelPreview)
This msg is not used. Please remove this.

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

http://codereview.chromium.org/7365003/diff/19002/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.h:353: // Number of pages to print.
// Actual number of pages that needs to be rendered ??

http://codereview.chromium.org/7365003/diff/19002/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.h:356: // The current page to print.
s/print/render

[Lei Zhang, 2011-07-15 20:23:35]

I don't see how the scoped_ptr error would have happened. We only reset /
dereference the scoped_ptr in a few states, and there's DCHECKs to make sure we
are in the right state when we do so. If you can get a stacktrace it might help
in figuring out why it happened.

http://codereview.chromium.org/7365003/diff/9001/chrome/browser/printing/prin...
File chrome/browser/printing/print_preview_message_handler.cc (right):

http://codereview.chromium.org/7365003/diff/9001/chrome/browser/printing/prin...
chrome/browser/printing/print_preview_message_handler.cc:82: if
(!print_preview_tab) {
On 2011/07/15 19:25:46, kmadhusu wrote:
> On 2011/07/15 01:48:02, Lei Zhang wrote:
> > On 2011/07/15 01:22:46, kmadhusu wrote:
> > > Can you make this if condition (!print_preview_tab ||
> > > !print_preview_tab->web_ui())?
> > 
> > I don't think we need it. I've never seen a crash on line 91.
> 
> As we discussed, it can happen. So, modify the if condition.

Done.

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

http://codereview.chromium.org/7365003/diff/14002/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:1070: void
PrintWebViewHelper::PrintPreviewContext::ResetRenderingState() {
On 2011/07/15 19:25:47, kmadhusu wrote:
> How about ClearPrintPreviewContext()? We are not changing any state in this
> function definition. 

Changed to ClearContext().

http://codereview.chromium.org/7365003/diff/19002/chrome/common/print_messages.h
File chrome/common/print_messages.h (right):

http://codereview.chromium.org/7365003/diff/19002/chrome/common/print_message...
chrome/common/print_messages.h:177: IPC_MESSAGE_ROUTED0(PrintMsg_CancelPreview)
On 2011/07/15 19:25:47, kmadhusu wrote:
> This msg is not used. Please remove this.

Done.

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

http://codereview.chromium.org/7365003/diff/19002/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.h:353: // Number of pages to print.
On 2011/07/15 19:25:47, kmadhusu wrote:
> // Actual number of pages that needs to be rendered ??

Done.

http://codereview.chromium.org/7365003/diff/19002/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.h:356: // The current page to print.
On 2011/07/15 19:25:47, kmadhusu wrote:
> s/print/render

Done.

[kmadhusu, 2011-07-15 20:47:05]

LGTM

I don't have a specific test case to repro the crashes. I will try to get the
stack trace next time.

[commit-bot: I haz the power, 2011-07-18 02:47:29]

Change committed as 92828