Print Preview: Keep the cancel button enabled whenever possible. Hook up the escape key to cancel.

Print Preview: Keep the cancel button enabled whenever possible. Hook up the escape key to cancel.

BUG=none
TEST=none

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

[arv (Not doing code reviews), 2011-10-10 19:44:26]

http://codereview.chromium.org/8223009/diff/1/chrome/browser/resources/print_...
File chrome/browser/resources/print_preview/print_header.js (right):

http://codereview.chromium.org/8223009/diff/1/chrome/browser/resources/print_...
chrome/browser/resources/print_preview/print_header.js:41:
cr.dispatchSimpleEvent(document, 'disableCancelButton');
It is a bit unclear why you are dispatching an event here. Can't you just call a
method or use a setter?

http://codereview.chromium.org/8223009/diff/1/chrome/browser/resources/print_...
chrome/browser/resources/print_preview/print_header.js:73:
window.removeEventListener('keydown');
This will not work. You need to pass in the function you added.

[dpapad, 2011-10-10 19:53:24]

http://codereview.chromium.org/8223009/diff/1/chrome/browser/resources/print_...
File chrome/browser/resources/print_preview/print_header.js (right):

http://codereview.chromium.org/8223009/diff/1/chrome/browser/resources/print_...
chrome/browser/resources/print_preview/print_header.js:73:
window.removeEventListener('keydown');
Or you could add it as window.onkeydown = ....; and do window.onkeydown = null;
here.

http://codereview.chromium.org/8223009/diff/1/chrome/browser/resources/print_...
File chrome/browser/resources/print_preview/print_preview.js (right):

http://codereview.chromium.org/8223009/diff/1/chrome/browser/resources/print_...
chrome/browser/resources/print_preview/print_preview.js:1047:
cr.dispatchSimpleEvent(document, 'disableCancelButton');
Is it necessary to disable the cancel button here, since the tab is going to
close anyway?

http://codereview.chromium.org/8223009/diff/1/chrome/browser/resources/print_...
chrome/browser/resources/print_preview/print_preview.js:1049: return
Nit: semicolon.

[Lei Zhang, 2011-10-10 20:03:14]

http://codereview.chromium.org/8223009/diff/1/chrome/browser/resources/print_...
File chrome/browser/resources/print_preview/print_header.js (right):

http://codereview.chromium.org/8223009/diff/1/chrome/browser/resources/print_...
chrome/browser/resources/print_preview/print_header.js:41:
cr.dispatchSimpleEvent(document, 'disableCancelButton');
On 2011/10/10 19:44:26, arv wrote:
> It is a bit unclear why you are dispatching an event here. Can't you just call
a
> method or use a setter?

Done.

http://codereview.chromium.org/8223009/diff/1/chrome/browser/resources/print_...
chrome/browser/resources/print_preview/print_header.js:73:
window.removeEventListener('keydown');
On 2011/10/10 19:53:24, dpapad wrote:
> Or you could add it as window.onkeydown = ....; and do window.onkeydown =
null;
> here.

Done.

http://codereview.chromium.org/8223009/diff/1/chrome/browser/resources/print_...
File chrome/browser/resources/print_preview/print_preview.js (right):

http://codereview.chromium.org/8223009/diff/1/chrome/browser/resources/print_...
chrome/browser/resources/print_preview/print_preview.js:1047:
cr.dispatchSimpleEvent(document, 'disableCancelButton');
On 2011/10/10 19:53:24, dpapad wrote:
> Is it necessary to disable the cancel button here, since the tab is going to
> close anyway?

Not sure. If the user presses escape many times quickly, is it possible for the
JS to call closePrintPreviewTab() twice? I'm just trying to play it safe.

http://codereview.chromium.org/8223009/diff/1/chrome/browser/resources/print_...
chrome/browser/resources/print_preview/print_preview.js:1049: return
On 2011/10/10 19:53:24, dpapad wrote:
> Nit: semicolon.

Done.

[dpapad, 2011-10-10 20:10:49]

http://codereview.chromium.org/8223009/diff/5001/chrome/browser/resources/pri...
File chrome/browser/resources/print_preview/print_header.js (right):

http://codereview.chromium.org/8223009/diff/5001/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/print_header.js:42:
closePrintPreviewTab();
From within PrintHeader "this" should be used and not "printHeader", which makes
PrintHeader class depend on the name of that object therefore breaks
encapsulation.

this.cancelButton_.onclick = function () {
this.disableCancelButton_();
.......;
}.bind(this);

[Lei Zhang, 2011-10-10 20:53:13]

http://codereview.chromium.org/8223009/diff/5001/chrome/browser/resources/pri...
File chrome/browser/resources/print_preview/print_header.js (right):

http://codereview.chromium.org/8223009/diff/5001/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/print_header.js:42:
closePrintPreviewTab();
On 2011/10/10 20:10:49, dpapad wrote:
> From within PrintHeader "this" should be used and not "printHeader", which
makes
> PrintHeader class depend on the name of that object therefore breaks
> encapsulation.
> 
> this.cancelButton_.onclick = function () {
> this.disableCancelButton_();
> .......;
> }.bind(this);

Done.

[dpapad, 2011-10-10 21:58:21]

LGTM with one comment.

http://codereview.chromium.org/8223009/diff/5/chrome/browser/resources/print_...
File chrome/browser/resources/print_preview/print_preview.js (right):

http://codereview.chromium.org/8223009/diff/5/chrome/browser/resources/print_...
chrome/browser/resources/print_preview/print_preview.js:180:
cr.dispatchSimpleEvent(document, 'disableCancelButton');
I believe it is better to just call printHeader.disableCancelButton here (and
make it not private), instead of sending an event. So far we are using custom
events when some settings object (copies, layout, pages etc) needs to notify
some other setting object, but we assume that within print_preview.js we can
access all settings objects directly.

http://codereview.chromium.org/8223009/diff/5/chrome/browser/resources/print_...
chrome/browser/resources/print_preview/print_preview.js:1047:
cr.dispatchSimpleEvent(document, 'disableCancelButton');
Same here.

[Lei Zhang, 2011-10-10 23:08:33]

See patch set 5.

http://codereview.chromium.org/8223009/diff/5/chrome/browser/resources/print_...
File chrome/browser/resources/print_preview/print_preview.js (right):

http://codereview.chromium.org/8223009/diff/5/chrome/browser/resources/print_...
chrome/browser/resources/print_preview/print_preview.js:180:
cr.dispatchSimpleEvent(document, 'disableCancelButton');
On 2011/10/10 21:58:22, dpapad wrote:
> I believe it is better to just call printHeader.disableCancelButton here (and
> make it not private), instead of sending an event. So far we are using custom
> events when some settings object (copies, layout, pages etc) needs to notify
> some other setting object, but we assume that within print_preview.js we can
> access all settings objects directly.

Done.

[dpapad, 2011-10-10 23:28:12]

LGTM

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

http://codereview.chromium.org/8223009/diff/2008/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/print_preview.js:1042: * @param {Event} e
The keyboard event.
@param {KeyboardEvent}.

[Lei Zhang, 2011-10-10 23:39:59]

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

http://codereview.chromium.org/8223009/diff/2008/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/print_preview.js:1042: * @param {Event} e
The keyboard event.
On 2011/10/10 23:28:12, dpapad wrote:
> @param {KeyboardEvent}.

I see "Event" used everywhere else though.

[dpapad, 2011-10-10 23:45:50]

On 2011/10/10 23:28:12, dpapad wrote:
> LGTM
> 
>
http://codereview.chromium.org/8223009/diff/2008/chrome/browser/resources/pri...
> File chrome/browser/resources/print_preview/print_preview.js (right):
> 
>
http://codereview.chromium.org/8223009/diff/2008/chrome/browser/resources/pri...
> chrome/browser/resources/print_preview/print_preview.js:1042: * @param {Event}
e
> The keyboard event.
> @param {KeyboardEvent}.

Ok. I said it because that is what appears if you do console.log(typeof(e)).

[arv (Not doing code reviews), 2011-10-10 23:46:14]

lgtm

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

http://codereview.chromium.org/8223009/diff/2008/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/print_preview.js:1049: return;
useless return

[Lei Zhang, 2011-10-10 23:49:55]

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

http://codereview.chromium.org/8223009/diff/2008/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/print_preview.js:1049: return;
On 2011/10/10 23:46:14, arv wrote:
> useless return

removed.

[commit-bot: I haz the power, 2011-10-10 23:50:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/thestig@chromium.org/8223009/1005

[commit-bot: I haz the power, 2011-10-11 05:57:51]

Change committed as 104854