Print Preview: Adding UI for margin settings.

Print Preview: Adding UI for margin settings.

BUG=69337
TEST=Open print preview. Margin options should be available.


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

[dpapad, 2011-10-04 00:16:16]

Still working on adding some documentation in preview_area.js, did not want to
delay the review for that.

[Evan Stade, 2011-10-04 02:49:17]

what is this "group name" concept? Is that a holdover from drawing lines for
every page?

Also, you should finalize comments before uploading for review as the comments
are very useful in the review, plus I am reviewing the comments themselves in
addition to the code.

http://codereview.chromium.org/7891016/diff/65004/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/margin_line.js (right):

http://codereview.chromium.org/7891016/diff/65004/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/margin_line.js:11:
line.setAttribute('class', MarginLine.CSS_CLASS_MARGIN_LINE);
line.className = ...

http://codereview.chromium.org/7891016/diff/65004/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/margin_line.js:12:
line.setAttribute('margin-group', groupName);
why not

line.marginGroup = groupName;

http://codereview.chromium.org/7891016/diff/65004/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/margin_line.js:14: line.visibleLine =
document.createElement('div');
docs

http://codereview.chromium.org/7891016/diff/65004/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/margin_line.js:24:
MarginLine.VISIBLE_LINE_WIDTH = 0;
this doesn't make sense. How is it 0? isn't it 1?

http://codereview.chromium.org/7891016/diff/65004/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/margin_line.js:34:
MarginLine.drawLineUsingPercent = function (line, rectangle) {
no space after function (here and elsewhere)

http://codereview.chromium.org/7891016/diff/65004/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/margin_line.js:35: line.style.left = 100
* rectangle.x.toFixed(6) + '%';
is 6 decimal places the limit css accepts or something?

http://codereview.chromium.org/7891016/diff/65004/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/margin_line.js:50:
MarginLine.drawLineUsingPixels = function (line, rectangle, width, height) {
drawLineUsingPixels is an odd name. If I understand correctly this is used for
the draggable area. Maybe call it something along those lines? drawDragControl
or something.

 Also the description is a bit off, it doesn't actually do any drawing (e.g. by
using a canvas). It just sets CSS styles to size an HTMLElement.

http://codereview.chromium.org/7891016/diff/65004/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/margin_line.js:57: 
extra line

http://codereview.chromium.org/7891016/diff/65004/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/margin_line.js:67: * margin line (not
visible) and the inner line (visible).
outer and inner are not very descriptive, 'drag control' and 'dotted line' are
better (if that's what these are)

http://codereview.chromium.org/7891016/diff/65004/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/margin_line.js:79: * Calucaltes the
coordinates and size of |this|.
sp

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

http://codereview.chromium.org/7891016/diff/65004/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/margin_settings.js:26: return this.top ==
rhs.top &&
use ===

http://codereview.chromium.org/7891016/diff/65004/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/margin_settings.js:100: 
extra line

http://codereview.chromium.org/7891016/diff/65004/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/margin_settings.js:137:
MarginSettings.MARGINS_VALUE_CUSTOM = 2;
order these by value, not alphabet

[dpapad, 2011-10-04 20:41:20]

Addressed comments.
groupName is still needed to associate textboxes and lines with the
corresponding margin, it does not need to be an html attribute though, just a
member var is enough.

http://codereview.chromium.org/7891016/diff/65004/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/margin_line.js (right):

http://codereview.chromium.org/7891016/diff/65004/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/margin_line.js:11:
line.setAttribute('class', MarginLine.CSS_CLASS_MARGIN_LINE);
On 2011/10/04 02:49:18, Evan Stade wrote:
> line.className = ...

Done, here and elsewhere.

http://codereview.chromium.org/7891016/diff/65004/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/margin_line.js:12:
line.setAttribute('margin-group', groupName);
On 2011/10/04 02:49:18, Evan Stade wrote:
> why not
> 
> line.marginGroup = groupName;

Done, here and elsewhere.

http://codereview.chromium.org/7891016/diff/65004/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/margin_line.js:14: line.visibleLine =
document.createElement('div');
On 2011/10/04 02:49:18, Evan Stade wrote:
> docs

Done.

http://codereview.chromium.org/7891016/diff/65004/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/margin_line.js:24:
MarginLine.VISIBLE_LINE_WIDTH = 0;
On 2011/10/04 02:49:18, Evan Stade wrote:
> this doesn't make sense. How is it 0? isn't it 1?

Removed it. Indeed it does not make sense since it is not the actual visible
width. In order to make it look like a line a zero width or height div element
is used, therefore only the border is visible.

http://codereview.chromium.org/7891016/diff/65004/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/margin_line.js:34:
MarginLine.drawLineUsingPercent = function (line, rectangle) {
On 2011/10/04 02:49:18, Evan Stade wrote:
> no space after function (here and elsewhere)

Done.

http://codereview.chromium.org/7891016/diff/65004/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/margin_line.js:35: line.style.left = 100
* rectangle.x.toFixed(6) + '%';
On 2011/10/04 02:49:18, Evan Stade wrote:
> is 6 decimal places the limit css accepts or something?
Done. It accepts more digits (even though it would not make any difference in
pixels unless the screen is huge).

http://codereview.chromium.org/7891016/diff/65004/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/margin_line.js:50:
MarginLine.drawLineUsingPixels = function (line, rectangle, width, height) {
On 2011/10/04 02:49:18, Evan Stade wrote:
> drawLineUsingPixels is an odd name. If I understand correctly this is used for
> the draggable area. Maybe call it something along those lines? drawDragControl
> or something.
> 
>  Also the description is a bit off, it doesn't actually do any drawing (e.g.
by
> using a canvas). It just sets CSS styles to size an HTMLElement.

Actually these should not be class (as of static in C++) methods anymore. I made
them regular methods and renamed to more descriptive names. I jkept the term
"draw" in the names since changing the CSS style causes the divs to be redrawn
and to be consistent with the rest of the code.

http://codereview.chromium.org/7891016/diff/65004/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/margin_line.js:57: 
On 2011/10/04 02:49:18, Evan Stade wrote:
> extra line

Done.

http://codereview.chromium.org/7891016/diff/65004/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/margin_line.js:67: * margin line (not
visible) and the inner line (visible).
On 2011/10/04 02:49:18, Evan Stade wrote:
> outer and inner are not very descriptive, 'drag control' and 'dotted line' are
> better (if that's what these are)

Done.

http://codereview.chromium.org/7891016/diff/65004/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/margin_line.js:79: * Calucaltes the
coordinates and size of |this|.
On 2011/10/04 02:49:18, Evan Stade wrote:
> sp

Done.

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

http://codereview.chromium.org/7891016/diff/65004/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/margin_settings.js:26: return this.top ==
rhs.top &&
On 2011/10/04 02:49:18, Evan Stade wrote:
> use ===

Done.

http://codereview.chromium.org/7891016/diff/65004/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/margin_settings.js:100: 
On 2011/10/04 02:49:18, Evan Stade wrote:
> extra line

Done.

http://codereview.chromium.org/7891016/diff/65004/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/margin_settings.js:137:
MarginSettings.MARGINS_VALUE_CUSTOM = 2;
On 2011/10/04 02:49:18, Evan Stade wrote:
> order these by value, not alphabet

Done.

[Evan Stade, 2011-10-05 03:05:23]

http://codereview.chromium.org/7891016/diff/73028/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/margin_utils.js (right):

http://codereview.chromium.org/7891016/diff/73028/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/margin_utils.js:45: // TODO(dpapad):
convert |value| to number before comparing.
I don't understand this comment, if value is not a number then how can you
compare it

http://codereview.chromium.org/7891016/diff/73028/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/margin_utils.js:53: function Point(x, y)
{
what is the point of point? why not just do var point = { x: x, y: y};

http://codereview.chromium.org/7891016/diff/73028/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/margins_ui.js (right):

http://codereview.chromium.org/7891016/diff/73028/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/margins_ui.js:115:
this.classList.remove('hidden');
use hidden attribute?

http://codereview.chromium.org/7891016/diff/73028/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/preview_area.js (right):

http://codereview.chromium.org/7891016/diff/73028/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/preview_area.js:9: * Creates a
PreviewArea object.
explain what PreviewArea is

http://codereview.chromium.org/7891016/diff/73028/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/print_preview.css (right):

http://codereview.chromium.org/7891016/diff/73028/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/print_preview.css:727: color: #ffffff;
black

http://codereview.chromium.org/7891016/diff/73028/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/print_preview.css:752: border-color:
#4080FA;
i prefer rgb() style (easier to read)

http://codereview.chromium.org/7891016/diff/73028/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/print_preview.js (right):

http://codereview.chromium.org/7891016/diff/73028/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/print_preview.js:74: var previewArea;
docs

http://codereview.chromium.org/7891016/diff/73028/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/print_preview.js:320: &&
marginSettings.areMarginSettingsValid() &&
operator on end of line

[dpapad, 2011-10-05 16:39:49]

Addressed comments.

http://codereview.chromium.org/7891016/diff/73028/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/margin_utils.js (right):

http://codereview.chromium.org/7891016/diff/73028/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/margin_utils.js:45: // TODO(dpapad):
convert |value| to number before comparing.
On 2011/10/05 03:05:23, Evan Stade wrote:
> I don't understand this comment, if value is not a number then how can you
> compare it

It is already of type number. The comment is obsolete, removed it.

http://codereview.chromium.org/7891016/diff/73028/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/margin_utils.js:53: function Point(x, y)
{
On 2011/10/05 03:05:23, Evan Stade wrote:
> what is the point of point? why not just do var point = { x: x, y: y};

Having a class instead of creating an object on the fly makes it easier when
documenting functions/methods (currently used in 2 places besides this file).
Also thre might be a need to add some behavior to the Point class in the future
(when dragging will be added, for example offset(dx, dy)). Let's leave it as is,
unless you strongly believe that this class is unnecessary.

http://codereview.chromium.org/7891016/diff/73028/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/margins_ui.js (right):

http://codereview.chromium.org/7891016/diff/73028/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/margins_ui.js:115:
this.classList.remove('hidden');
On 2011/10/05 03:05:23, Evan Stade wrote:
> use hidden attribute?

Done.

http://codereview.chromium.org/7891016/diff/73028/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/preview_area.js (right):

http://codereview.chromium.org/7891016/diff/73028/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/preview_area.js:9: * Creates a
PreviewArea object.
On 2011/10/05 03:05:23, Evan Stade wrote:
> explain what PreviewArea is

Done.

http://codereview.chromium.org/7891016/diff/73028/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/print_preview.css (right):

http://codereview.chromium.org/7891016/diff/73028/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/print_preview.css:727: color: #ffffff;
On 2011/10/05 03:05:23, Evan Stade wrote:
> black

See next comment.

http://codereview.chromium.org/7891016/diff/73028/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/print_preview.css:752: border-color:
#4080FA;
On 2011/10/05 03:05:23, Evan Stade wrote:
> i prefer rgb() style (easier to read)

According to the style guide, hex values should be used,
http://www.chromium.org/developers/web-development-style-guide.

http://codereview.chromium.org/7891016/diff/73028/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/print_preview.js (right):

http://codereview.chromium.org/7891016/diff/73028/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/print_preview.js:74: var previewArea;
On 2011/10/05 03:05:23, Evan Stade wrote:
> docs

Done. Also updated the doc of the remaining objects to include their type.

http://codereview.chromium.org/7891016/diff/73028/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/print_preview.js:320: &&
marginSettings.areMarginSettingsValid() &&
On 2011/10/05 03:05:23, Evan Stade wrote:
> operator on end of line

Done.

[Evan Stade, 2011-10-05 22:42:14]

just nits

http://codereview.chromium.org/7891016/diff/73028/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/print_preview.css (right):

http://codereview.chromium.org/7891016/diff/73028/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/print_preview.css:752: border-color:
#4080FA;
On 2011/10/05 16:39:49, dpapad wrote:
> On 2011/10/05 03:05:23, Evan Stade wrote:
> > i prefer rgb() style (easier to read)
> 
> According to the style guide, hex values should be used,
> http://www.chromium.org/developers/web-development-style-guide.

I've started a thread with James and Erik. We'll see where that goes.

http://codereview.chromium.org/7891016/diff/80001/chrome/app/generated_resour...
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/7891016/diff/80001/chrome/app/generated_resour...
chrome/app/generated_resources.grd:6076: Invalid margin values
remove as agreed

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

http://codereview.chromium.org/7891016/diff/80001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/margin_settings.js:289: if
(this.marginsUI_ == null) {
!this.marginsUI

http://codereview.chromium.org/7891016/diff/80001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/margin_settings.js:361: * Listener
executing when the default margins option is selected.
nit: "Listener executing" sounds mildly awkward. Maybe "Function that executes"

http://codereview.chromium.org/7891016/diff/80001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/margin_settings.js:425: * @private
I think private comes after return

http://codereview.chromium.org/7891016/diff/80001/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/margin_textbox.js (right):

http://codereview.chromium.org/7891016/diff/80001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/margin_textbox.js:105:
this.classList.add('invalid');
I think lining up the return vals in a ternary statement is an OK reason to
break the 4 space continuation indent rule (akin to lining up args in a function
call)

http://codereview.chromium.org/7891016/diff/80001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/margin_textbox.js:184: }
newline

http://codereview.chromium.org/7891016/diff/80001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/margin_textbox.js:209: }
newline

http://codereview.chromium.org/7891016/diff/80001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/margin_textbox.js:226: }
no curlies

http://codereview.chromium.org/7891016/diff/80001/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/margin_utils.js (right):

http://codereview.chromium.org/7891016/diff/80001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/margin_utils.js:21: // TODO(dpapad):
Return -1 instead of null to indicate an invalid value.
is this todo still valid? what's wrong with what it's doing now?

http://codereview.chromium.org/7891016/diff/80001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/margin_utils.js:49: * Creates a Rect
object. This object describes a rectangle in a 2D plane.
here is where you would mention that the units can be percentages of a page (or
always are percentages?)

http://codereview.chromium.org/7891016/diff/80001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/margin_utils.js:60: get x() {
remove the getters you don't need (all but origin)

http://codereview.chromium.org/7891016/diff/80001/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/preview_area.js (right):

http://codereview.chromium.org/7891016/diff/80001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/preview_area.js:41: * The height of the
plugin area excluding any visible scrollbars in pixels.
nit: 'The height of the plugin area in pixels, excluding any visible
scrollbars.'

(applies elsewhere too)

http://codereview.chromium.org/7891016/diff/80001/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/print_preview.js (right):

http://codereview.chromium.org/7891016/diff/80001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/print_preview.js:1002: var
pluginInterface = [ dummyPlugin.onload,
I don't see anything wrong with the old format, why did you change it?

[dpapad, 2011-10-06 00:05:39]

Addressed comments

http://codereview.chromium.org/7891016/diff/80001/chrome/app/generated_resour...
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/7891016/diff/80001/chrome/app/generated_resour...
chrome/app/generated_resources.grd:6076: Invalid margin values
On 2011/10/05 22:42:14, Evan Stade wrote:
> remove as agreed

Done.

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

http://codereview.chromium.org/7891016/diff/80001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/margin_settings.js:289: if
(this.marginsUI_ == null) {
On 2011/10/05 22:42:14, Evan Stade wrote:
> !this.marginsUI

Done.

http://codereview.chromium.org/7891016/diff/80001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/margin_settings.js:361: * Listener
executing when the default margins option is selected.
On 2011/10/05 22:42:14, Evan Stade wrote:
> nit: "Listener executing" sounds mildly awkward. Maybe "Function that
executes"

Done.

http://codereview.chromium.org/7891016/diff/80001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/margin_settings.js:425: * @private
On 2011/10/05 22:42:14, Evan Stade wrote:
> I think private comes after return

Done, for files already in this CL.

http://codereview.chromium.org/7891016/diff/80001/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/margin_textbox.js (right):

http://codereview.chromium.org/7891016/diff/80001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/margin_textbox.js:105:
this.classList.add('invalid');
On 2011/10/05 22:42:14, Evan Stade wrote:
> I think lining up the return vals in a ternary statement is an OK reason to
> break the 4 space continuation indent rule (akin to lining up args in a
function
> call)

Done.

http://codereview.chromium.org/7891016/diff/80001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/margin_textbox.js:184: }
On 2011/10/05 22:42:14, Evan Stade wrote:
> newline

Done.

http://codereview.chromium.org/7891016/diff/80001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/margin_textbox.js:209: }
On 2011/10/05 22:42:14, Evan Stade wrote:
> newline

Done.

http://codereview.chromium.org/7891016/diff/80001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/margin_textbox.js:226: }
On 2011/10/05 22:42:14, Evan Stade wrote:
> no curlies

Done.

http://codereview.chromium.org/7891016/diff/80001/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/margin_utils.js (right):

http://codereview.chromium.org/7891016/diff/80001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/margin_utils.js:21: // TODO(dpapad):
Return -1 instead of null to indicate an invalid value.
On 2011/10/05 22:42:14, Evan Stade wrote:
> is this todo still valid? what's wrong with what it's doing now?

This TODO is not necessary, i just though it is better to return -1 which is
also invalid and still of type number.

http://codereview.chromium.org/7891016/diff/80001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/margin_utils.js:49: * Creates a Rect
object. This object describes a rectangle in a 2D plane.
On 2011/10/05 22:42:14, Evan Stade wrote:
> here is where you would mention that the units can be percentages of a page
(or
> always are percentages?)

Done.

http://codereview.chromium.org/7891016/diff/80001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/margin_utils.js:60: get x() {
On 2011/10/05 22:42:14, Evan Stade wrote:
> remove the getters you don't need (all but origin)

Done. Removed origin too since it is not used in the code.

http://codereview.chromium.org/7891016/diff/80001/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/preview_area.js (right):

http://codereview.chromium.org/7891016/diff/80001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/preview_area.js:41: * The height of the
plugin area excluding any visible scrollbars in pixels.
On 2011/10/05 22:42:14, Evan Stade wrote:
> nit: 'The height of the plugin area in pixels, excluding any visible
> scrollbars.'
> 
> (applies elsewhere too)

Done.

http://codereview.chromium.org/7891016/diff/80001/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/print_preview.js (right):

http://codereview.chromium.org/7891016/diff/80001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/print_preview.js:1002: var
pluginInterface = [ dummyPlugin.onload,
On 2011/10/05 22:42:14, Evan Stade wrote:
> I don't see anything wrong with the old format, why did you change it?

Because this way is much easier to find which function is missing from the
plugin (by inserting a console.log in the for loop). This happens sometimes when
renaming functions of the plugin and it is tedious to spot which one.

[Evan Stade, 2011-10-07 03:29:58]

http://codereview.chromium.org/7891016/diff/80001/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/print_preview.js (right):

http://codereview.chromium.org/7891016/diff/80001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/print_preview.js:1002: var
pluginInterface = [ dummyPlugin.onload,
On 2011/10/06 00:05:39, dpapad wrote:
> On 2011/10/05 22:42:14, Evan Stade wrote:
> > I don't see anything wrong with the old format, why did you change it?
> 
> Because this way is much easier to find which function is missing from the
> plugin (by inserting a console.log in the for loop). This happens sometimes
when
> renaming functions of the plugin and it is tedious to spot which one.

I suggest you leave the console.log there.

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

http://codereview.chromium.org/7891016/diff/96003/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/margin_settings.js:255: for (var i = 0; i
< pairs.length; i++) {
return pairs.every(function(pair) {
  return pair.box_.isValid;
});

http://codereview.chromium.org/7891016/diff/96003/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/margin_settings.js:281: * 1) The input is
compeletely valid (it can be parsed in its entirety).
indent the list

http://codereview.chromium.org/7891016/diff/96003/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/margin_settings.js:417: * @private
.

http://codereview.chromium.org/7891016/diff/96003/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/margin_settings.js:435: * @private
.

http://codereview.chromium.org/7891016/diff/96003/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/margin_textbox.js (right):

http://codereview.chromium.org/7891016/diff/96003/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/margin_textbox.js:174: x =
this.marginsRectangle_.x +
this block will get a lot more readable if you save  this.marginsRectangle_ in a
local var. You might consider doing the same for
MarginTextBox.total{Width,Height}() as well.

http://codereview.chromium.org/7891016/diff/96003/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/margin_textbox.js:186:
(MarginTextbox.totalWidth() / totalWidth);
indent

http://codereview.chromium.org/7891016/diff/96003/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/margin_utils.js (right):

http://codereview.chromium.org/7891016/diff/96003/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/margin_utils.js:23: return null;
why didn't you make this -1?

http://codereview.chromium.org/7891016/diff/96003/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/margin_utils.js:42: if (value ==  null)
I don't think this will work, since 0 == null

http://codereview.chromium.org/7891016/diff/96003/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/margins_ui.js (right):

http://codereview.chromium.org/7891016/diff/96003/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/margins_ui.js:107: for (var i = 0; i <
uiPairs.length; i++)
consider:

uiPairs.forEach(function(pair) { pair.draw(); });

http://codereview.chromium.org/7891016/diff/96003/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/margins_ui.js:134: this.style.clip =
"rect(" + top + "px," + right +"px, " + bottom +
space after +

also, I think bottom should be on the same line as "px, "

http://codereview.chromium.org/7891016/diff/96003/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/margins_ui.js:137: 
remove this newline

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

Addressed comments.

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

http://codereview.chromium.org/7891016/diff/96003/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/margin_settings.js:255: for (var i = 0; i
< pairs.length; i++) {
On 2011/10/07 03:29:58, Evan Stade wrote:
> return pairs.every(function(pair) {
>   return pair.box_.isValid;
> });

Done.

http://codereview.chromium.org/7891016/diff/96003/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/margin_settings.js:281: * 1) The input is
compeletely valid (it can be parsed in its entirety).
On 2011/10/07 03:29:58, Evan Stade wrote:
> indent the list

Done.

http://codereview.chromium.org/7891016/diff/96003/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/margin_settings.js:417: * @private
On 2011/10/07 03:29:58, Evan Stade wrote:
> .

?

http://codereview.chromium.org/7891016/diff/96003/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/margin_settings.js:435: * @private
On 2011/10/07 03:29:58, Evan Stade wrote:
> .

?

http://codereview.chromium.org/7891016/diff/96003/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/margin_textbox.js (right):

http://codereview.chromium.org/7891016/diff/96003/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/margin_textbox.js:174: x =
this.marginsRectangle_.x +
On 2011/10/07 03:29:58, Evan Stade wrote:
> this block will get a lot more readable if you save  this.marginsRectangle_ in
a
> local var. You might consider doing the same for
> MarginTextBox.total{Width,Height}() as well.

Done. I created getters for middleX, middleY, bottom and right in class Rect
which helped to improve readability here.

http://codereview.chromium.org/7891016/diff/96003/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/margin_textbox.js:186:
(MarginTextbox.totalWidth() / totalWidth);
On 2011/10/07 03:29:58, Evan Stade wrote:
> indent

Done.

http://codereview.chromium.org/7891016/diff/96003/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/margin_utils.js (right):

http://codereview.chromium.org/7891016/diff/96003/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/margin_utils.js:23: return null;
On 2011/10/07 03:29:58, Evan Stade wrote:
> why didn't you make this -1?

Done.

http://codereview.chromium.org/7891016/diff/96003/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/margin_utils.js:42: if (value ==  null)
On 2011/10/07 03:29:58, Evan Stade wrote:
> I don't think this will work, since 0 == null

(0 == null) returns false when I tried in the console.

http://codereview.chromium.org/7891016/diff/96003/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/margins_ui.js (right):

http://codereview.chromium.org/7891016/diff/96003/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/margins_ui.js:107: for (var i = 0; i <
uiPairs.length; i++)
On 2011/10/07 03:29:58, Evan Stade wrote:
> consider:
> 
> uiPairs.forEach(function(pair) { pair.draw(); });

Done.

http://codereview.chromium.org/7891016/diff/96003/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/margins_ui.js:134: this.style.clip =
"rect(" + top + "px," + right +"px, " + bottom +
On 2011/10/07 03:29:58, Evan Stade wrote:
> space after +
> 
> also, I think bottom should be on the same line as "px, "

Done. Also removed "px" for top and left since they are 0, therefore px is not
needed.

http://codereview.chromium.org/7891016/diff/96003/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/margins_ui.js:137: 
On 2011/10/07 03:29:58, Evan Stade wrote:
> remove this newline

Done.

[Evan Stade, 2011-10-07 19:53:09]

lgtm

http://codereview.chromium.org/7891016/diff/96003/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/margins_ui.js (right):

http://codereview.chromium.org/7891016/diff/96003/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/margins_ui.js:134: this.style.clip =
"rect(" + top + "px," + right +"px, " + bottom +
On 2011/10/07 16:49:20, dpapad wrote:
> On 2011/10/07 03:29:58, Evan Stade wrote:
> > space after +
> > 
> > also, I think bottom should be on the same line as "px, "
> 
> Done. Also removed "px" for top and left since they are 0, therefore px is not
> needed.

right, good point, you should get rid of top and left

http://codereview.chromium.org/7891016/diff/98002/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/margins_ui.js (right):

http://codereview.chromium.org/7891016/diff/98002/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/margins_ui.js:131: this.style.clip =
"rect(" + top + ", " + right + "px, "
I'd just put 0 straight in there instead of storing it in top/left

[commit-bot: I haz the power, 2011-10-12 06:06:54]

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

[commit-bot: I haz the power, 2011-10-12 08:57:29]

Change committed as 105044