Print Preview: Making the UI not block when preview is generated

Print Preview: Making the UI not block when preview is generated

Silently rerendering the preview when the user changes the settings.

BUG=82961, 83442, 83660
TEST=Load a multi-page doc in the print preview tab. Change the settings.
The displayed doc shuld reflect the latest changes.


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

[dpapad, 2011-05-23 18:00:58]

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

http://codereview.chromium.org/7051040/diff/1002/chrome/browser/resources/pri...
chrome/browser/resources/print_preview.js:953: this.isLandscape = '';
I think that more state variables (defined at the top of this file) could be
eventually moved in this class.

[James Hawkins, 2011-05-23 20:49:45]

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

http://codereview.chromium.org/7051040/diff/6001/chrome/browser/resources/pri...
chrome/browser/resources/print_preview.js:471: return;
You have returns on three of these cases, but not the third case. Inconsistent.

http://codereview.chromium.org/7051040/diff/6001/chrome/browser/resources/pri...
chrome/browser/resources/print_preview.js:935: function SettingsState() {
s/SettingsState/PrintSettings/

SettingsState is too ambiguous.

[dpapad, 2011-05-23 21:05:50]

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

http://codereview.chromium.org/7051040/diff/6001/chrome/browser/resources/pri...
chrome/browser/resources/print_preview.js:471: return;
On 2011/05/23 20:49:45, James Hawkins wrote:
> You have returns on three of these cases, but not the third case.
Inconsistent.

This is intentional though. In case the page range field has changed while the
preview was generated and the current value is not valid we dont want to return,
we want to show the document and just display the "invalid page range" message.

http://codereview.chromium.org/7051040/diff/6001/chrome/browser/resources/pri...
chrome/browser/resources/print_preview.js:935: function SettingsState() {
On 2011/05/23 20:49:45, James Hawkins wrote:
> s/SettingsState/PrintSettings/
> 
> SettingsState is too ambiguous.

Done.

[James Hawkins, 2011-05-23 21:12:30]

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

http://codereview.chromium.org/7051040/diff/6001/chrome/browser/resources/pri...
chrome/browser/resources/print_preview.js:471: return;
On 2011/05/23 21:05:50, dpapad wrote:
> On 2011/05/23 20:49:45, James Hawkins wrote:
> > You have returns on three of these cases, but not the third case.
> Inconsistent.
> 
> This is intentional though. In case the page range field has changed while the
> preview was generated and the current value is not valid we dont want to
return,
> we want to show the document and just display the "invalid page range"
message.

Ah sorry, thought the code below was in a separate method. Consider refactoring
this; it is a bit unreadable.

[dpapad, 2011-05-23 21:17:05]

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

http://codereview.chromium.org/7051040/diff/7003/chrome/browser/resources/pri...
chrome/browser/resources/print_preview.js:496: pageRangesFieldChanged();
I think it is more readable now.

[James Hawkins, 2011-05-23 21:24:06]

LGTM

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

http://codereview.chromium.org/7051040/diff/7003/chrome/browser/resources/pri...
chrome/browser/resources/print_preview.js:59: var individualPages =
$('individual-pages');
Move this var to where it's first used.

http://codereview.chromium.org/7051040/diff/7003/chrome/browser/resources/pri...
chrome/browser/resources/print_preview.js:88: var individualPages =
$('individual-pages');
Same here.

[Lei Zhang, 2011-05-23 22:04:44]

LGTM

[dpapad, 2011-05-23 23:04:07]

Fixed some issues (updating print button state, validating page ranges while
preview is loading).

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

http://codereview.chromium.org/7051040/diff/7003/chrome/browser/resources/pri...
chrome/browser/resources/print_preview.js:59: var individualPages =
$('individual-pages');
On 2011/05/23 21:24:06, James Hawkins wrote:
> Move this var to where it's first used.

Done.

http://codereview.chromium.org/7051040/diff/7003/chrome/browser/resources/pri...
chrome/browser/resources/print_preview.js:88: var individualPages =
$('individual-pages');
On 2011/05/23 21:24:06, James Hawkins wrote:
> Same here.

Done.

[dpapad, 2011-05-24 17:16:19]

There have been a couple of minor fixes since last LGTMs.

[dpapad, 2011-05-24 18:04:57]

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

http://codereview.chromium.org/7051040/diff/6014/chrome/browser/resources/pri...
chrome/browser/resources/print_preview.js:609: // span to 0x0.
I left it as a span. Changing it into a div makes it appear on the next line
(and requires a display: inline-block; rule to fix it).

[James Hawkins, 2011-05-24 18:09:08]

LGTM

[commit-bot: I haz the power, 2011-05-24 19:49:43]

Change committed as 86470