PrintPreview: Preview generation should not block print button.

PrintPreview: Preview generation should not block print button.

Initial set of changes to enable the print button while the preview data is loading. This CL does not handle any print preview errors.

BUG=84127
TEST=Press Ctrl+p on a webpage. Observe the print button state in the preview tab.

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

[kmadhusu, 2011-06-06 17:46:19]

thestig: Please review c++ code.
dpapad: Please review ui code.

This is an initial step to have a working print button while a preview is
loading. Please test the code and let me know if you see any unexpected
behavior.

Thanks.

[dpapad, 2011-06-06 18:07:34]

http://codereview.chromium.org/7056070/diff/12/chrome/browser/resources/print...
File chrome/browser/resources/print_preview.js (right):

http://codereview.chromium.org/7056070/diff/12/chrome/browser/resources/print...
chrome/browser/resources/print_preview.js:52:
$('system-dialog-link').addEventListener('click', showSystemDialog);
Why move this line? If the there is no compatible plugin this line will never
get executed and Chromium users will not be able to print at all.

http://codereview.chromium.org/7056070/diff/12/chrome/browser/resources/print...
chrome/browser/resources/print_preview.js:82: function
addEventListenersToCopiesControl() {
rename to addEventListenersToCopiesControls for consistency with the previous
function.

http://codereview.chromium.org/7056070/diff/12/chrome/browser/resources/print...
chrome/browser/resources/print_preview.js:339: $('print-summary').innerHTML =
localStrings.getString('printing');
I suspect that if you select Print to PDF, the hit print and then hit cancel the
print summary will keep displaying "Printing...". If so, move this line within
the "if (getSelectedPrinterName() != PRINT_TO_PDF)" block as before.

http://codereview.chromium.org/7056070/diff/12/chrome/browser/resources/print...
chrome/browser/resources/print_preview.js:344: hasPendingPrintFileRequest =
false;
Replace if/else with
hasPendingPrintFileRequest = previewRequestCount > 0 ? true : false;

[kmadhusu, 2011-06-06 18:44:07]

http://codereview.chromium.org/7056070/diff/12/chrome/browser/resources/print...
File chrome/browser/resources/print_preview.js (right):

http://codereview.chromium.org/7056070/diff/12/chrome/browser/resources/print...
chrome/browser/resources/print_preview.js:52:
$('system-dialog-link').addEventListener('click', showSystemDialog);
On 2011/06/06 18:07:35, dpapad wrote:
> Why move this line? If the there is no compatible plugin this line will never
> get executed and Chromium users will not be able to print at all. 

Print button is now enabled. In line #49, I have associated the print button to
show the system native dialog for Chromium users.

http://codereview.chromium.org/7056070/diff/12/chrome/browser/resources/print...
chrome/browser/resources/print_preview.js:82: function
addEventListenersToCopiesControl() {
On 2011/06/06 18:07:35, dpapad wrote:
> rename to addEventListenersToCopiesControls for consistency with the previous
> function.

Done.

http://codereview.chromium.org/7056070/diff/12/chrome/browser/resources/print...
chrome/browser/resources/print_preview.js:339: $('print-summary').innerHTML =
localStrings.getString('printing');
On 2011/06/06 18:07:35, dpapad wrote:
> I suspect that if you select Print to PDF, the hit print and then hit cancel
the
> print summary will keep displaying "Printing...". If so, move this line within
> the "if (getSelectedPrinterName() != PRINT_TO_PDF)" block as before.

Done.

http://codereview.chromium.org/7056070/diff/12/chrome/browser/resources/print...
chrome/browser/resources/print_preview.js:344: hasPendingPrintFileRequest =
false;
On 2011/06/06 18:07:35, dpapad wrote:
> Replace if/else with
> hasPendingPrintFileRequest = previewRequestCount > 0 ? true : false;

Done.

[dpapad, 2011-06-06 20:47:43]

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

http://codereview.chromium.org/7056070/diff/11001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview.js:33: // Total number of print preview
requests submitted.
This variable is being decremented in updatePrintPreview(). Change the comment
to "Total number of in-flight print preview requests submitted." or something
else to this effect.

http://codereview.chromium.org/7056070/diff/11001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview.js:37: var hasPendingPrintFileRequest =
false;
This variable is calculated based on previewRequestCount. I would prefer
removing it and checking directly for previewRequestCount > 0 wherever needed.

http://codereview.chromium.org/7056070/diff/11001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview.js:49:
$('print-button').addEventListener('click', showSystemDialog);
I think for now it is better to add this listener $('system-dialog-link') as
before (until we hear back from the UI people).

http://codereview.chromium.org/7056070/diff/11001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview.js:519: 
Add "var hasPendingPrintFileRequest = previewRequestCount > 0;" here since
hasPendingPrintFileRequest is no longer global (in case you followed my earlier
comment).

[kmadhusu, 2011-06-06 21:23:55]

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

http://codereview.chromium.org/7056070/diff/11001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview.js:33: // Total number of print preview
requests submitted.
On 2011/06/06 20:47:43, dpapad wrote:
> This variable is being decremented in updatePrintPreview(). Change the comment
> to "Total number of in-flight print preview requests submitted." or something
> else to this effect.

Done.

http://codereview.chromium.org/7056070/diff/11001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview.js:37: var hasPendingPrintFileRequest =
false;
On 2011/06/06 20:47:43, dpapad wrote:
> This variable is calculated based on previewRequestCount. I would prefer
> removing it and checking directly for previewRequestCount > 0 wherever needed.

(just repeating our in-person conversation) I need this global variable. It is
modified in printFile() function. It is used to track whether a printFile
request is submitted earlier while a preview data is loading.

http://codereview.chromium.org/7056070/diff/11001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview.js:49:
$('print-button').addEventListener('click', showSystemDialog);
On 2011/06/06 20:47:43, dpapad wrote:
> I think for now it is better to add this listener $('system-dialog-link') as
> before (until we hear back from the UI people).

Done.

http://codereview.chromium.org/7056070/diff/11001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview.js:519: 
On 2011/06/06 20:47:43, dpapad wrote:
> Add "var hasPendingPrintFileRequest = previewRequestCount > 0;" here since
> hasPendingPrintFileRequest is no longer global (in case you followed my
earlier
> comment).

Please refer to my reply for your earlier comment.

[kmadhusu, 2011-06-07 01:55:38]

thestig, dpapad: I am going to refactor some of the javascript code. Please do
not review the code until further notice.

Thanks.

[kmadhusu, 2011-06-07 20:06:23]

thestig, dpapad: CL is ready for review. To my knowledge, I have addressed all
the corner cases. 

Thanks.

[dpapad, 2011-06-07 20:47:14]

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

http://codereview.chromium.org/7056070/diff/16010/chrome/browser/resources/pr...
chrome/browser/resources/print_preview.js:112: individualPages.oninput =
validateIndividualPagesText;
Shouldn't we validate the the page range field only when it loses focus or the
timeout expires? I think this will result in a lot of code being executed in
every single keystroke.

http://codereview.chromium.org/7056070/diff/16010/chrome/browser/resources/pr...
chrome/browser/resources/print_preview.js:208: $('print-button').disabled =
true;
Should we move this line within displayErrorMessage? Is there a case when we
display an error but the button should be enabled?

http://codereview.chromium.org/7056070/diff/16010/chrome/browser/resources/pr...
chrome/browser/resources/print_preview.js:393: hasPendingPrintFileRequest =
hasPendingPreviewRequest ? true : false;
hasPendingPrintFileRequest = hasPendingPreviewRequest;

http://codereview.chromium.org/7056070/diff/16010/chrome/browser/resources/pr...
chrome/browser/resources/print_preview.js:922: } else if (isInteger(part)) {
Shouldn't we check also for parseInt(part, 10) > 0 here?

[kmadhusu, 2011-06-08 00:39:14]

dpapad: Addressed review comments.

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

http://codereview.chromium.org/7056070/diff/16010/chrome/browser/resources/pr...
chrome/browser/resources/print_preview.js:112: individualPages.oninput =
validateIndividualPagesText;
On 2011/06/07 20:47:14, dpapad wrote:
> Shouldn't we validate the the page range field only when it loses focus or the
> timeout expires? I think this will result in a lot of code being executed in
> every single keystroke.

Fixed. Changed the behavior to validate on onblur event.

http://codereview.chromium.org/7056070/diff/16010/chrome/browser/resources/pr...
chrome/browser/resources/print_preview.js:208: $('print-button').disabled =
true;
On 2011/06/07 20:47:14, dpapad wrote:
> Should we move this line within displayErrorMessage? Is there a case when we
> display an error but the button should be enabled?

Fixed. (Removed line #208 and #55).

http://codereview.chromium.org/7056070/diff/16010/chrome/browser/resources/pr...
chrome/browser/resources/print_preview.js:393: hasPendingPrintFileRequest =
hasPendingPreviewRequest ? true : false;
On 2011/06/07 20:47:14, dpapad wrote:
> hasPendingPrintFileRequest = hasPendingPreviewRequest;

Just for better readability of the code, I used the tri-state format.

http://codereview.chromium.org/7056070/diff/16010/chrome/browser/resources/pr...
chrome/browser/resources/print_preview.js:922: } else if (isInteger(part)) {
On 2011/06/07 20:47:14, dpapad wrote:
> Shouldn't we check also for parseInt(part, 10) > 0 here?

Fixed.

[dpapad, 2011-06-08 01:27:50]

I did not test it yet, will do it tomorrow morning.

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

http://codereview.chromium.org/7056070/diff/21002/chrome/browser/resources/pr...
chrome/browser/resources/print_preview.js:513: * @param {boolean} showButton
Indivates whether the "Reopen the page" button
nit: s/Indivates/Indicates

http://codereview.chromium.org/7056070/diff/21002/chrome/browser/resources/pr...
chrome/browser/resources/print_preview.js:894: for (var i = 0; i < parts.length;
++i) {
This for loop is getting hard to read. Nothing to do here for now, I will remove
the validity levels once this CL lands (since we no longer need them, true/false
would be enough).

[kmadhusu, 2011-06-08 17:09:50]

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

http://codereview.chromium.org/7056070/diff/21002/chrome/browser/resources/pr...
chrome/browser/resources/print_preview.js:513: * @param {boolean} showButton
Indivates whether the "Reopen the page" button
On 2011/06/08 01:27:51, dpapad wrote:
> nit: s/Indivates/Indicates

Done.

http://codereview.chromium.org/7056070/diff/21002/chrome/browser/resources/pr...
chrome/browser/resources/print_preview.js:894: for (var i = 0; i < parts.length;
++i) {
On 2011/06/08 01:27:51, dpapad wrote:
> This for loop is getting hard to read. Nothing to do here for now, I will
remove
> the validity levels once this CL lands (since we no longer need them,
true/false
> would be enough).

we should do that.

[dpapad, 2011-06-08 17:28:51]

LGTM!

[Lei Zhang, 2011-06-09 09:54:55]

http://codereview.chromium.org/7056070/diff/22002/chrome/browser/printing/pri...
File chrome/browser/printing/print_preview_message_handler.cc (right):

http://codereview.chromium.org/7056070/diff/22002/chrome/browser/printing/pri...
chrome/browser/printing/print_preview_message_handler.cc:124: rvh->Send(new
PrintMsg_PrintingDone(rvh->routing_id(), false));
Why do you need this? If we failed to generate a print preview, then we haven't
even started printing yet.

http://codereview.chromium.org/7056070/diff/22002/chrome/browser/printing/pri...
File chrome/browser/printing/print_preview_tab_controller.h (right):

http://codereview.chromium.org/7056070/diff/22002/chrome/browser/printing/pri...
chrome/browser/printing/print_preview_tab_controller.h:64: void
EraseInitiatorTabInfo(TabContents* preview_tab);
Can we add a unit test for this? Should be easy.

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

http://codereview.chromium.org/7056070/diff/22002/chrome/browser/resources/pr...
chrome/browser/resources/print_preview.js:43: var hasCompatiblePlugin = true;
Probably safer to start with this set to false and modify line 57 accordingly.
Sorry to curb your optimism.

http://codereview.chromium.org/7056070/diff/22002/chrome/browser/resources/pr...
chrome/browser/resources/print_preview.js:92: function
handleIndividualPagesInputEvent() {
nit: can you put this next to handleAllPagesCheckbox()?

http://codereview.chromium.org/7056070/diff/22002/chrome/browser/resources/pr...
chrome/browser/resources/print_preview.js:182:
setDefaultHandlersForCopiesControls();
Does this need to be here for can it be at line 174? Can
setDefaultHandlersForPagesControls() and setDefaultHandlersForCopiesControls()
be combined since you call them both at approximately the same time?

http://codereview.chromium.org/7056070/diff/22002/chrome/browser/resources/pr...
chrome/browser/resources/print_preview.js:394: hasPendingPrintFileRequest =
hasPendingPreviewRequest ? true : false;
Uhh, isn't this the same as:
hasPendingPrintFileRequest = hasPendingPreviewRequest;

???

http://codereview.chromium.org/7056070/diff/22002/chrome/browser/ui/webui/pri...
File chrome/browser/ui/webui/print_preview_handler.cc (left):

http://codereview.chromium.org/7056070/diff/22002/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_handler.cc:437: TabContents* initiator_tab
= GetInitiatorTab();
Why did you move this code?

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

http://codereview.chromium.org/7056070/diff/22002/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_handler.cc:668: // Remove initiator tab
association with the preview tab.
You should mention why we do this here.

http://codereview.chromium.org/7056070/diff/22002/chrome/browser/ui/webui/pri...
File chrome/browser/ui/webui/print_preview_ui.h (right):

http://codereview.chromium.org/7056070/diff/22002/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_ui.h:47: void setTabHiddenForPrint() {
nit: simple setter/getters should be unix_hacker_style(). Same below.

http://www.chromium.org/developers/coding-style#TOC-Inline-functions

http://codereview.chromium.org/7056070/diff/22002/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_ui.h:47: void setTabHiddenForPrint() {
More importantly, why modify PrintPreviewUI at all? Can't you get this
information from BackgroundPrintingManager?

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

http://codereview.chromium.org/7056070/diff/22002/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:203: DidFinishPrinting(FAIL_PRINT);
Can you explain how we reach this state? It seems this means there's no PDF
plugin, but we tried to print anyway. Why would we ever do that?

[dpapad, 2011-06-09 16:33:35]

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

http://codereview.chromium.org/7056070/diff/22002/chrome/browser/resources/pr...
chrome/browser/resources/print_preview.js:76: function handleAllPagesCheckbox()
{
Could you remove this function and also change line 112 to
"allPages.onclick = updatePrintButtonState;" so that we can keep the number of
functions to a minimum?

http://codereview.chromium.org/7056070/diff/22002/chrome/browser/resources/pr...
chrome/browser/resources/print_preview.js:84: $('print-pages').checked = true;
Why is this necessary? If the print-pages checkbox is not checked this function
should not be called at all.

http://codereview.chromium.org/7056070/diff/22002/chrome/browser/resources/pr...
chrome/browser/resources/print_preview.js:610: return;
Remove return since it is not needed here.

[kmadhusu, 2011-06-09 18:11:43]

thestig, dpapad: Addressed review comments. Please review the latest patch.

Thanks.

http://codereview.chromium.org/7056070/diff/22002/chrome/browser/printing/pri...
File chrome/browser/printing/print_preview_message_handler.cc (right):

http://codereview.chromium.org/7056070/diff/22002/chrome/browser/printing/pri...
chrome/browser/printing/print_preview_message_handler.cc:124: rvh->Send(new
PrintMsg_PrintingDone(rvh->routing_id(), false));
On 2011/06/09 09:54:55, Lei Zhang wrote:
> Why do you need this? If we failed to generate a print preview, then we
haven't
> even started printing yet.

I am sending this message for 2 reasons.
(1) To display an alert dialog with an error message.  
(2) To release the acquired resources.

http://codereview.chromium.org/7056070/diff/22002/chrome/browser/printing/pri...
File chrome/browser/printing/print_preview_tab_controller.h (right):

http://codereview.chromium.org/7056070/diff/22002/chrome/browser/printing/pri...
chrome/browser/printing/print_preview_tab_controller.h:64: void
EraseInitiatorTabInfo(TabContents* preview_tab);
On 2011/06/09 09:54:55, Lei Zhang wrote:
> Can we add a unit test for this? Should be easy.

Done.

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

http://codereview.chromium.org/7056070/diff/22002/chrome/browser/resources/pr...
chrome/browser/resources/print_preview.js:43: var hasCompatiblePlugin = true;
On 2011/06/09 09:54:55, Lei Zhang wrote:
> Probably safer to start with this set to false and modify line 57 accordingly.
> Sorry to curb your optimism.

Done.

http://codereview.chromium.org/7056070/diff/22002/chrome/browser/resources/pr...
chrome/browser/resources/print_preview.js:76: function handleAllPagesCheckbox()
{
On 2011/06/09 16:33:35, dpapad wrote:
> Could you remove this function and also change line 112 to
> "allPages.onclick = updatePrintButtonState;" so that we can keep the number of
> functions to a minimum?

Done.

http://codereview.chromium.org/7056070/diff/22002/chrome/browser/resources/pr...
chrome/browser/resources/print_preview.js:84: $('print-pages').checked = true;
On 2011/06/09 16:33:35, dpapad wrote:
> Why is this necessary? If the print-pages checkbox is not checked this
function
> should not be called at all.

It is required. 

Test case: When you navigate to the text box using the tab key and enter some
data in the text box, we need to select the print-pages checkbox.

http://codereview.chromium.org/7056070/diff/22002/chrome/browser/resources/pr...
chrome/browser/resources/print_preview.js:92: function
handleIndividualPagesInputEvent() {
On 2011/06/09 09:54:55, Lei Zhang wrote:
> nit: can you put this next to handleAllPagesCheckbox()?

Done.

http://codereview.chromium.org/7056070/diff/22002/chrome/browser/resources/pr...
chrome/browser/resources/print_preview.js:182:
setDefaultHandlersForCopiesControls();
On 2011/06/09 09:54:55, Lei Zhang wrote:
> Does this need to be here for can it be at line 174? Can
> setDefaultHandlersForPagesControls() and setDefaultHandlersForCopiesControls()
> be combined since you call them both at approximately the same time?

Done.

http://codereview.chromium.org/7056070/diff/22002/chrome/browser/resources/pr...
chrome/browser/resources/print_preview.js:394: hasPendingPrintFileRequest =
hasPendingPreviewRequest ? true : false;
On 2011/06/09 09:54:55, Lei Zhang wrote:
> Uhh, isn't this the same as:
> hasPendingPrintFileRequest = hasPendingPreviewRequest;
> 
> ???

Just for better readability I used tri-state format.

http://codereview.chromium.org/7056070/diff/22002/chrome/browser/resources/pr...
chrome/browser/resources/print_preview.js:610: return;
On 2011/06/09 16:33:35, dpapad wrote:
> Remove return since it is not needed here.

Done.

http://codereview.chromium.org/7056070/diff/22002/chrome/browser/ui/webui/pri...
File chrome/browser/ui/webui/print_preview_handler.cc (left):

http://codereview.chromium.org/7056070/diff/22002/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_handler.cc:437: TabContents* initiator_tab
= GetInitiatorTab();
On 2011/06/09 09:54:55, Lei Zhang wrote:
> Why did you move this code?

oops. My bad. Reverted this change.

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

http://codereview.chromium.org/7056070/diff/22002/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_handler.cc:668: // Remove initiator tab
association with the preview tab.
On 2011/06/09 09:54:55, Lei Zhang wrote:
> You should mention why we do this here.

Done.

http://codereview.chromium.org/7056070/diff/22002/chrome/browser/ui/webui/pri...
File chrome/browser/ui/webui/print_preview_ui.h (right):

http://codereview.chromium.org/7056070/diff/22002/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_ui.h:47: void setTabHiddenForPrint() {
On 2011/06/09 09:54:55, Lei Zhang wrote:
> More importantly, why modify PrintPreviewUI at all? Can't you get this
> information from BackgroundPrintingManager?

Removed the getter/setter functions. Made code changes to get this info from
BackgroundPrintingManager.

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

http://codereview.chromium.org/7056070/diff/22002/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:203: DidFinishPrinting(FAIL_PRINT);
On 2011/06/09 09:54:55, Lei Zhang wrote:
> Can you explain how we reach this state? It seems this means there's no PDF
> plugin, but we tried to print anyway. Why would we ever do that?

While testing I came across a test case which satisfied this condition. Now I
don't remember the scenario exactly. I am reverting this changes. If required we
can handle this later.

[dpapad, 2011-06-09 18:58:41]

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

http://codereview.chromium.org/7056070/diff/18010/chrome/browser/resources/pr...
chrome/browser/resources/print_preview.js:83: function
validateIndividualPagesText() {
There are validateIndividualPagesText() and validatePageRangesField() which do
different things, but the names are confusing, indicating that they do the exact
same thing.

Could you rename validateIndividualPagesText() function to
onPageRangesFieldBlur() since it is added for the blur event only? And also
update the comments?

[kmadhusu, 2011-06-09 20:15:08]

dpapad: Renamed the function.

Thanks.

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

http://codereview.chromium.org/7056070/diff/18010/chrome/browser/resources/pr...
chrome/browser/resources/print_preview.js:83: function
validateIndividualPagesText() {
On 2011/06/09 18:58:41, dpapad wrote:
> There are validateIndividualPagesText() and validatePageRangesField() which do
> different things, but the names are confusing, indicating that they do the
exact
> same thing.
> 
> Could you rename validateIndividualPagesText() function to
> onPageRangesFieldBlur() since it is added for the blur event only? And also
> update the comments? 

Done.

http://codereview.chromium.org/7056070/diff/21012/chrome/browser/resources/pr...
chrome/browser/resources/print_preview.js:43: var hasCompatiblePlugin = false;
Removed hasCompatiblePlugin and isInitiatorTabClosed variables and replaced them
with hasError variable to handle all preview error cases (Print Preview failed,
Initiator tab closed, no plugin case).

[dpapad, 2011-06-09 20:32:37]

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

http://codereview.chromium.org/7056070/diff/21012/chrome/browser/resources/pr...
chrome/browser/resources/print_preview.js:80: $('print-pages').checked = true;
Since this line exists also in handleIndividualPagesInputEvent() the checkbox
will be checked already (if the user typed anything). If the user just focused
the textfield without typing and then unfocus we should not change the checkbox
state, right?

Imagine a use case where there in no mouse and the user wants all pages selected
and landscape.

http://codereview.chromium.org/7056070/diff/21012/chrome/browser/resources/pr...
chrome/browser/resources/print_preview.js:375: hasPendingPrintFileRequest =
hasPendingPreviewRequest ? true : false;
I agree with thestig on this, 
"hasPendingPrintFileRequest = hasPendingPreviewRequest;" is readable enough.
Moreover the ternary operator is a two-step operation (compare first then
execute the corresponding branch).

http://codereview.chromium.org/7056070/diff/21012/chrome/browser/resources/pr...
chrome/browser/resources/print_preview.js:585: 
nit: Remove blank line.

[Lei Zhang, 2011-06-09 21:03:27]

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

http://codereview.chromium.org/7056070/diff/22002/chrome/browser/resources/pr...
chrome/browser/resources/print_preview.js:394: hasPendingPrintFileRequest =
hasPendingPreviewRequest ? true : false;
On 2011/06/09 18:11:44, kmadhusu wrote:
> On 2011/06/09 09:54:55, Lei Zhang wrote:
> > Uhh, isn't this the same as:
> > hasPendingPrintFileRequest = hasPendingPreviewRequest;
> > 
> > ???
> 
> Just for better readability I used tri-state format. 

I don't understand how that enhances readability. Consider:

bool x = true;
...
bool y = x ? true : false;

vs

bool x = true;
...
bool y = x;

http://codereview.chromium.org/7056070/diff/18010/chrome/browser/printing/bac...
File chrome/browser/printing/background_printing_manager.h (right):

http://codereview.chromium.org/7056070/diff/18010/chrome/browser/printing/bac...
chrome/browser/printing/background_printing_manager.h:40: bool
hasTabContents(TabContentsWrapper* entry);
nit: hasTabContents -> HasTabContents

[kmadhusu, 2011-06-09 21:27:11]

thestig, dpapad: Addressed review comments.

Thanks.

http://codereview.chromium.org/7056070/diff/18010/chrome/browser/printing/bac...
File chrome/browser/printing/background_printing_manager.h (right):

http://codereview.chromium.org/7056070/diff/18010/chrome/browser/printing/bac...
chrome/browser/printing/background_printing_manager.h:40: bool
hasTabContents(TabContentsWrapper* entry);
On 2011/06/09 21:03:27, Lei Zhang wrote:
> nit: hasTabContents -> HasTabContents

Done.

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

http://codereview.chromium.org/7056070/diff/21012/chrome/browser/resources/pr...
chrome/browser/resources/print_preview.js:80: $('print-pages').checked = true;
On 2011/06/09 20:32:38, dpapad wrote:
> Since this line exists also in handleIndividualPagesInputEvent() the checkbox
> will be checked already (if the user typed anything). If the user just focused
> the textfield without typing and then unfocus we should not change the
checkbox
> state, right?
> 
> Imagine a use case where there in no mouse and the user wants all pages
selected
> and landscape.

(just repeating our in-person conversation): This code works as expected in the
specified test case.

http://codereview.chromium.org/7056070/diff/21012/chrome/browser/resources/pr...
chrome/browser/resources/print_preview.js:375: hasPendingPrintFileRequest =
hasPendingPreviewRequest ? true : false;
On 2011/06/09 20:32:38, dpapad wrote:
> I agree with thestig on this, 
> "hasPendingPrintFileRequest = hasPendingPreviewRequest;" is readable enough.
> Moreover the ternary operator is a two-step operation (compare first then
> execute the corresponding branch).

Done.

http://codereview.chromium.org/7056070/diff/21012/chrome/browser/resources/pr...
chrome/browser/resources/print_preview.js:585: 
On 2011/06/09 20:32:38, dpapad wrote:
> nit: Remove blank line.

Done.

[dpapad, 2011-06-09 21:41:59]

LGTM

[Lei Zhang, 2011-06-09 22:39:07]

http://codereview.chromium.org/7056070/diff/20022/chrome/browser/printing/pri...
File chrome/browser/printing/print_preview_message_handler.cc (right):

http://codereview.chromium.org/7056070/diff/20022/chrome/browser/printing/pri...
chrome/browser/printing/print_preview_message_handler.cc:126: rvh->Send(new
PrintMsg_PrintingDone(rvh->routing_id(), false));
So you said this is to tell PWVH to cleanup after itself, because
PrintMsg_PrintingDone will eventually lead to PWVH::DidFinishPrinting().

However, the only case where you can get here in the first place is if
PWVH::DidFinishPrinting() sends PrintHostMsg_PrintPreviewFailed, so it would
have already done the cleanup that you want it to do here.

http://codereview.chromium.org/7056070/diff/20022/chrome/browser/printing/pri...
chrome/browser/printing/print_preview_message_handler.cc:127: delete
print_preview_tab;
Can you delete |wrapper| instead? In a separate CL, sky was worried about us
deleting the TabContent and leaving an invalid wrapper.

[kmadhusu, 2011-06-09 23:55:48]

thestig: Addressed review comments.

Thanks.

http://codereview.chromium.org/7056070/diff/20022/chrome/browser/printing/pri...
File chrome/browser/printing/print_preview_message_handler.cc (right):

http://codereview.chromium.org/7056070/diff/20022/chrome/browser/printing/pri...
chrome/browser/printing/print_preview_message_handler.cc:126: rvh->Send(new
PrintMsg_PrintingDone(rvh->routing_id(), false));
On 2011/06/09 22:39:07, Lei Zhang wrote:
> So you said this is to tell PWVH to cleanup after itself, because
> PrintMsg_PrintingDone will eventually lead to PWVH::DidFinishPrinting().
> 
> However, the only case where you can get here in the first place is if
> PWVH::DidFinishPrinting() sends PrintHostMsg_PrintPreviewFailed, so it would
> have already done the cleanup that you want it to do here.

Removed the msg. I will handle the error case in my next CL.

http://codereview.chromium.org/7056070/diff/20022/chrome/browser/printing/pri...
chrome/browser/printing/print_preview_message_handler.cc:127: delete
print_preview_tab;
On 2011/06/09 22:39:07, Lei Zhang wrote:
> Can you delete |wrapper| instead? In a separate CL, sky was worried about us
> deleting the TabContent and leaving an invalid wrapper.

Done

[Lei Zhang, 2011-06-10 00:44:50]

LGTM