Print Preview: Hiding margins UI if an error occurs.

Print Preview: Hiding margins UI if an error occurs.

BUG=100818
TEST=Select custom margins, close the initiator tab, the margins UI should be hidden.


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

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

Added an event whenever an error occurs. I am planning to create string
constants for all custom events names we use in the print preview code in a
future cleanup CL.

[Lei Zhang, 2011-10-19 01:40:55]

LGTM, but maybe estade or arv would be a better reviewer?

[dpapad, 2011-10-19 18:11:13]

@estade
Could you take a look?

[Evan Stade, 2011-10-19 20:01:55]

http://codereview.chromium.org/8343017/diff/2003/chrome/browser/resources/pri...
File chrome/browser/resources/print_preview/preview_area.js (right):

http://codereview.chromium.org/8343017/diff/2003/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/preview_area.js:211:
cr.dispatchSimpleEvent(document, 'errorOccurred');
i'd like to see a more descriptive event name. What class of error?

[dpapad, 2011-10-19 20:06:42]

On 2011/10/19 20:01:55, Evan Stade wrote:
>
http://codereview.chromium.org/8343017/diff/2003/chrome/browser/resources/pri...
> File chrome/browser/resources/print_preview/preview_area.js (right):
> 
>
http://codereview.chromium.org/8343017/diff/2003/chrome/browser/resources/pri...
> chrome/browser/resources/print_preview/preview_area.js:211:
> cr.dispatchSimpleEvent(document, 'errorOccurred');
> i'd like to see a more descriptive event name. What class of error?

At this point there is no need for specific error events. An error event is
simply fired when an error message is shown. Are you suggesting just to rename
the event name? Also FYI i am creating an enum to keep track of all the event
names here http://codereview.chromium.org/8357009/.

[dpapad, 2011-10-19 20:11:59]

On 2011/10/19 20:06:42, dpapad wrote:
> On 2011/10/19 20:01:55, Evan Stade wrote:
> >
>
http://codereview.chromium.org/8343017/diff/2003/chrome/browser/resources/pri...
> > File chrome/browser/resources/print_preview/preview_area.js (right):
> > 
> >
>
http://codereview.chromium.org/8343017/diff/2003/chrome/browser/resources/pri...
> > chrome/browser/resources/print_preview/preview_area.js:211:
> > cr.dispatchSimpleEvent(document, 'errorOccurred');
> > i'd like to see a more descriptive event name. What class of error?
> 
> At this point there is no need for specific error events. An error event is
> simply fired when an error message is shown. Are you suggesting just to rename
> the event name? Also FYI i am creating an enum to keep track of all the event
> names here http://codereview.chromium.org/8357009/.

Another approach would be to first fire the error event and then display the
error messages in the event listeners, that would require more specific error
events (or at least passing some parameters along with each event), but I prefer
not to do it in this CL. I 'd like to keep it small in case I decide to merge
this and 3-4 other CLs related to draggable margins to M16.

[Evan Stade, 2011-10-19 22:12:26]

yes, rename it

[dpapad, 2011-10-19 22:31:59]

Addressed comments.

http://codereview.chromium.org/8343017/diff/2003/chrome/browser/resources/pri...
File chrome/browser/resources/print_preview/preview_area.js (right):

http://codereview.chromium.org/8343017/diff/2003/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/preview_area.js:211:
cr.dispatchSimpleEvent(document, 'errorOccurred');
On 2011/10/19 20:01:55, Evan Stade wrote:
> i'd like to see a more descriptive event name. What class of error?

Done.

[Evan Stade, 2011-10-19 22:34:11]

http://codereview.chromium.org/8343017/diff/2003/chrome/browser/resources/pri...
File chrome/browser/resources/print_preview/margin_settings.js (right):

http://codereview.chromium.org/8343017/diff/2003/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/margin_settings.js:301: onErrorOccurred:
function() {
private

http://codereview.chromium.org/8343017/diff/2003/chrome/browser/resources/pri...
File chrome/browser/resources/print_preview/preview_area.js (right):

http://codereview.chromium.org/8343017/diff/2003/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/preview_area.js:201: displayErrorMessage:
function(errorMessage) {
this function should be renamed and the doc should be updated as it is doing
more than simple display now.

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

http://codereview.chromium.org/8343017/diff/2003/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/print_preview.js:151:
document.addEventListener('errorOccurred', onErrorOccurred);
why does this have its own helper function? at this point I'd inline it as it's
only one line and only move to a helper if it's like 20 lines

http://codereview.chromium.org/8343017/diff/2003/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/print_preview.js:157: function
onErrorOccurred() {
likewise, move the body of cancelpendingprintrequest here

[dpapad, 2011-10-19 22:56:21]

Addressed comments.

http://codereview.chromium.org/8343017/diff/2003/chrome/browser/resources/pri...
File chrome/browser/resources/print_preview/margin_settings.js (right):

http://codereview.chromium.org/8343017/diff/2003/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/margin_settings.js:301: onErrorOccurred:
function() {
On 2011/10/19 22:34:11, Evan Stade wrote:
> private

Done.

http://codereview.chromium.org/8343017/diff/2003/chrome/browser/resources/pri...
File chrome/browser/resources/print_preview/preview_area.js (right):

http://codereview.chromium.org/8343017/diff/2003/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/preview_area.js:201: displayErrorMessage:
function(errorMessage) {
On 2011/10/19 22:34:11, Evan Stade wrote:
> this function should be renamed and the doc should be updated as it is doing
> more than simple display now.

Done. Temporarily though. I am planning to move the displayMessage logic within
listeners of error events instead of firing such an event from here.

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

http://codereview.chromium.org/8343017/diff/2003/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/print_preview.js:151:
document.addEventListener('errorOccurred', onErrorOccurred);
On 2011/10/19 22:34:11, Evan Stade wrote:
> why does this have its own helper function? at this point I'd inline it as
it's
> only one line and only move to a helper if it's like 20 lines

Done.

http://codereview.chromium.org/8343017/diff/2003/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/print_preview.js:157: function
onErrorOccurred() {
On 2011/10/19 22:34:11, Evan Stade wrote:
> likewise, move the body of cancelpendingprintrequest here

Done.

[Evan Stade, 2011-10-19 23:18:30]

lgtm

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

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dpapad@chromium.org/8343017/10004

[commit-bot: I haz the power, 2011-10-20 00:43:04]

Change committed as 106420