Print Preview: Fixing printer list when no printer is installed.

Print Preview: Fixing printer list when no printer is installed.

Also
1) Adding minor animation after user clicks the print button.
2) Fixing vertical alignment of "Destinations" label.

BUG=83653, 83643, 83414, 83264
TEST=See bug description.


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

[Lei Zhang, 2011-05-23 21:46:19]

LGTM

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

http://codereview.chromium.org/7064006/diff/2001/chrome/browser/resources/pri...
chrome/browser/resources/print_preview.js:343:
addDestinationListOption('','',false, true);
nit: can you put a space after the comma here and below?

[dpapad, 2011-05-23 21:56:54]

Added one more single line fix in this CL. Please reLGTM.

[Lei Zhang, 2011-05-23 22:10:01]

LGTM

[dpapad, 2011-05-23 22:17:36]

I think the alignment should be fixed now, I will also ask Marcin before marking
this as fixed.

[dpapad, 2011-05-24 00:13:24]

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

http://codereview.chromium.org/7064006/diff/1005/chrome/browser/resources/pri...
chrome/browser/resources/print_preview.js:328: $('print-summary').innerHTML =
'Printing...';
I will localize this once I get the ok for the animation.

[dpapad, 2011-05-24 00:23:57]

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

http://codereview.chromium.org/7064006/diff/11/chrome/browser/resources/print...
chrome/browser/resources/print_preview.js:329:
cancelButton.classList.add('loading');
I will remove all the event listeners here to be safe, when I will rebase
http://codereview.chromium.org/7051040/.

[Lei Zhang, 2011-05-24 00:58:28]

LGTM with nits:

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

http://codereview.chromium.org/7064006/diff/11/chrome/browser/resources/print...
chrome/browser/resources/print_preview.js:325: var printButton =
$('print-button');
nit: you don't need |printButton| and |cancelButton| since they are only used
once, or are you are going to use them immediately after in the next CL?

http://codereview.chromium.org/7064006/diff/11/chrome/browser/resources/print...
chrome/browser/resources/print_preview.js:328: $('print-summary').innerHTML =
localStrings.getString('printing');
nit: can you put this one line down?

http://codereview.chromium.org/7064006/diff/11/chrome/browser/resources/print...
chrome/browser/resources/print_preview.js:331: if (getSelectedPrinterName() !=
PRINT_TO_PDF)
nit: { } around multi line if blocks.

http://codereview.chromium.org/7064006/diff/11/chrome/browser/resources/print...
chrome/browser/resources/print_preview.js:362: addDestinationListOption('','',
false, true);
nit: still need a space after the first comma.

[dpapad, 2011-05-24 01:10:05]

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

http://codereview.chromium.org/7064006/diff/11/chrome/browser/resources/print...
chrome/browser/resources/print_preview.js:325: var printButton =
$('print-button');
On 2011/05/24 00:58:29, Lei Zhang wrote:
> nit: you don't need |printButton| and |cancelButton| since they are only used
> once, or are you are going to use them immediately after in the next CL?

Done.

http://codereview.chromium.org/7064006/diff/11/chrome/browser/resources/print...
chrome/browser/resources/print_preview.js:328: $('print-summary').innerHTML =
localStrings.getString('printing');
On 2011/05/24 00:58:29, Lei Zhang wrote:
> nit: can you put this one line down?

Done.

http://codereview.chromium.org/7064006/diff/11/chrome/browser/resources/print...
chrome/browser/resources/print_preview.js:331: if (getSelectedPrinterName() !=
PRINT_TO_PDF)
On 2011/05/24 00:58:29, Lei Zhang wrote:
> nit: { } around multi line if blocks.

Done.

http://codereview.chromium.org/7064006/diff/11/chrome/browser/resources/print...
chrome/browser/resources/print_preview.js:362: addDestinationListOption('','',
false, true);
On 2011/05/24 00:58:29, Lei Zhang wrote:
> nit: still need a space after the first comma.

Done.

[commit-bot: I haz the power, 2011-05-24 02:41:29]

Change committed as 86391