Print Preview: Making margin selection sticky (part 1/2).

Print Preview: Making margin selection sticky (part 1/2).

This CL makes sticky default, minimum and no margins selections. Selecting custom margins is not remembered for now (will be addressed in a follow up CL).

BUG=102446
TEST=See bug description.

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

[kmadhusu, 2011-11-01 17:17:57]

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

http://codereview.chromium.org/8428005/diff/2002/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/margin_settings.js:198:
MarginSettings.valueToIndex = {};
Is there any specific reason to create this object? 
Index and the value are the same all the time. 

MarginSettings.valueToIndex[0] = 0;
MarginSettings.valueToIndex[1] = 1;
MarginSettings.valueToIndex[2] = 2;
MarginSettings.valueToIndex[3] = 3;

http://codereview.chromium.org/8428005/diff/2002/printing/print_job_constants.h
File printing/print_job_constants.h (right):

http://codereview.chromium.org/8428005/diff/2002/printing/print_job_constants...
printing/print_job_constants.h:121: PRINTABLE_AREA_MARGINS,
Can you rename this as MINIMUM_MARGINS?

[dpapad, 2011-11-01 17:29:14]

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

http://codereview.chromium.org/8428005/diff/2002/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/margin_settings.js:198:
MarginSettings.valueToIndex = {};
On 2011/11/01 17:17:57, kmadhusu wrote:
> Is there any specific reason to create this object? 
> Index and the value are the same all the time. 
> 
> MarginSettings.valueToIndex[0] = 0;
> MarginSettings.valueToIndex[1] = 1;
> MarginSettings.valueToIndex[2] = 2;
> MarginSettings.valueToIndex[3] = 3;
> 

It happens to be the same, but this is not a constraint. I did it this way to
allow for reordering the options in the dropdown without any other changes
needed. Otherwise reordering the options will also require to change the "value"
to match the index in 2 places (margin_settings.html and print_job_constants.h).

http://codereview.chromium.org/8428005/diff/2002/printing/print_job_constants.h
File printing/print_job_constants.h (right):

http://codereview.chromium.org/8428005/diff/2002/printing/print_job_constants...
printing/print_job_constants.h:121: PRINTABLE_AREA_MARGINS,
On 2011/11/01 17:17:57, kmadhusu wrote:
> Can you rename this as MINIMUM_MARGINS?

I think that the current name is more descriptive from the suggested ine. We
display "Minimum" to the user for simplicity but that does not need to affect
internal naming. Maybe vandebo has a stronger opinion on whether to rename or
not.

[dpapad, 2011-11-01 21:57:56]

Addressed kmadhusu's comments.

[kmadhusu, 2011-11-01 22:12:25]

http://codereview.chromium.org/8428005/diff/13001/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/margin_settings.js (right):

http://codereview.chromium.org/8428005/diff/13001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/margin_settings.js:246:
marginValueTypeToIndex: function(marginTypeValue) {
marginValueTypeToIndex  => getMarginOptionIndexByValue()??

http://codereview.chromium.org/8428005/diff/13001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/margin_settings.js:246:
marginValueTypeToIndex: function(marginTypeValue) {
private function?

[vandebo (ex-Chrome), 2011-11-01 22:15:12]

LGTM

[dpapad, 2011-11-01 22:38:34]

http://codereview.chromium.org/8428005/diff/13001/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/margin_settings.js (right):

http://codereview.chromium.org/8428005/diff/13001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/margin_settings.js:246:
marginValueTypeToIndex: function(marginTypeValue) {
On 2011/11/01 22:12:25, kmadhusu wrote:
> marginValueTypeToIndex  => getMarginOptionIndexByValue()?? 

Done.

http://codereview.chromium.org/8428005/diff/13001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/margin_settings.js:246:
marginValueTypeToIndex: function(marginTypeValue) {
On 2011/11/01 22:12:25, kmadhusu wrote:
> private function?

Done.

[kmadhusu, 2011-11-01 22:41:00]

LGTM with a nit.

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

http://codereview.chromium.org/8428005/diff/16001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/margin_settings.js:253: return 0;
nit: 0 => MarginSettings.OPTION_INDEX_DEFAULT