Print Preview: Hiding header/footer options when 'no margins' is selected

Print Preview: Hiding header/footer options when 'no margins' is selected

BUG=101457
TEST=Select "None" in the margins drop-down. The headers/footers option should disappear. Select 'Default' or 'Custom', it should reappear.


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

[Lei Zhang, 2011-10-25 17:10:04]

lgtm

[commit-bot: I haz the power, 2011-10-25 17:16:00]

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

[kmadhusu, 2011-11-24 00:34:34]

When the user selects "None", we should not only disable the header footer
option but also set "headerFooterEnabled" setting to false. 

In other words, hasHeaderFooter() function should also check the visibility of
the option along with the checked state.

[dpapad, 2011-11-29 17:27:34]

On 2011/11/24 00:34:34, kmadhusu wrote:
> When the user selects "None", we should not only disable the header footer
> option but also set "headerFooterEnabled" setting to false. 
> 
> In other words, hasHeaderFooter() function should also check the visibility of
> the option along with the checked state.

This is already done, see my other comment.

[dpapad, 2011-11-29 17:29:09]

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

http://codereview.chromium.org/8395006/diff/6001/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/header_footer_settings.js:84:
this.headerFooterCheckbox_.checked = false;
When hiding this option we also set it to not checked, therefore
hasHeaderFooterEnabled returns false.

[kmadhusu, 2011-11-29 17:48:59]

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

http://codereview.chromium.org/8395006/diff/6001/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/header_footer_settings.js:84:
this.headerFooterCheckbox_.checked = false;
On 2011/11/29 17:29:09, dpapad wrote:
> When hiding this option we also set it to not checked, therefore
> hasHeaderFooterEnabled returns false.

Consider the following scenario:

1. User previews a document.
2. Selects "Default" in Margin settings.
3. Checks the header and footer option.
4. Selects "None" in Margin settings.

Code in line 84, unchecks the previous option selected by the user. When the
user selects "Default" again, the header and footer option is unchecked.


At this moment, we just need to hide the header and footer option but not change
the setting selected by the user. We should not uncheck the checkbox, instead
hasHeaderFooter() function needs to handle this case of identifying the current
margin settings and see if the header and footer is valid for that setting.

[dpapad, 2011-11-29 19:32:07]

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

http://codereview.chromium.org/8395006/diff/6001/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/header_footer_settings.js:84:
this.headerFooterCheckbox_.checked = false;
On 2011/11/29 17:48:59, kmadhusu wrote:
> On 2011/11/29 17:29:09, dpapad wrote:
> > When hiding this option we also set it to not checked, therefore
> > hasHeaderFooterEnabled returns false.
> 
> Consider the following scenario:
> 
> 1. User previews a document.
> 2. Selects "Default" in Margin settings.
> 3. Checks the header and footer option.
> 4. Selects "None" in Margin settings.
> 
> Code in line 84, unchecks the previous option selected by the user. When the
> user selects "Default" again, the header and footer option is unchecked.
> 
> 
> At this moment, we just need to hide the header and footer option but not
change
> the setting selected by the user. We should not uncheck the checkbox, instead
> hasHeaderFooter() function needs to handle this case of identifying the
current
> margin settings and see if the header and footer is valid for that setting. 
> 

Done.

[Lei Zhang, 2011-11-29 19:45:15]

http://codereview.chromium.org/8395006/diff/16001/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/header_footer_settings.js (right):

http://codereview.chromium.org/8395006/diff/16001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/header_footer_settings.js:16:
this.headerFooterApplies_;
do you need to initialize this here?

http://codereview.chromium.org/8395006/diff/16001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/header_footer_settings.js:73:
this.setVisible_(previewModifiable);
isn't this always false here?

[dpapad, 2011-11-29 20:40:26]

Addressed Lei's comments.

http://codereview.chromium.org/8395006/diff/16001/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/header_footer_settings.js (right):

http://codereview.chromium.org/8395006/diff/16001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/header_footer_settings.js:16:
this.headerFooterApplies_;
On 2011/11/29 19:45:15, Lei Zhang wrote:
> do you need to initialize this here?

Done. This is supposed to be initialized elsewhere (in
onMarginsSelectionChanged_) before being used, so I had it like that to check if
that is true.

http://codereview.chromium.org/8395006/diff/16001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/header_footer_settings.js:73:
this.setVisible_(previewModifiable);
On 2011/11/29 19:45:15, Lei Zhang wrote:
> isn't this always false here?

Done.

[kmadhusu, 2011-11-29 20:48:16]

lgtm

[Lei Zhang, 2011-11-29 21:03:23]

On 2011/11/29 20:48:16, kmadhusu wrote:
> lgtm

lgtm++

[commit-bot: I haz the power, 2011-11-29 21:06:03]

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

[commit-bot: I haz the power, 2011-11-29 21:06:06]

Presubmit check for 8395006-17001 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for:
chrome/browser/resources/print_preview/margin_settings.js,chrome/browser/resources/print_preview/header_footer_settings.js,chrome/browser/resources/print_preview/print_preview.js

Presubmit checks took 1.4s to calculate.

[dpapad, 2011-11-30 00:09:38]

Added estade as a reviewer since he is in OWNERS.

[commit-bot: I haz the power, 2011-12-01 02:51:27]

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

[commit-bot: I haz the power, 2011-12-01 04:38:09]

Change committed as 112403