Refactor print preview web ui

chrome/browser/resources/print_preview/data/margins.js

+// Copyright (c) 2012 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+cr.define('print_preview', function() {
+  'use strict';
+
+  /**
+   * Creates a Margins object that holds four margin values in points.
+   *
+   * @param {number} top The top margin in pts.
+   * @param {number} right The right margin in pts.
+   * @param {number} bottom The bottom margin in pts.
+   * @param {number} left The left margin in pts.
+   * @constructor
+   */
+  function Margins(top, right, bottom, left) {
+    /**
+     * Top margin in points.
+     * @type {number}
+     * @private
+     */
+    this.top_ = top;
+
+    /**
+     * Right margin in points.
+     * @type {number}
+     * @private
+     */
+    this.right_ = right;
+
+    /**
+     * Bottom margin in points.
+     * @type {number}
+     * @private
+     */
+    this.bottom_ = bottom;
+
+    /**
+     * Left margin in points.
+     * @type {number}
+     * @private
+     */
+    this.left_ = left;
+  };
+
+  /**
+   * Enumeration of margin types.
+   * @enum {string}
+   */
+  Margins.Type = {
+    DEFAULT: 'default',

[dpapad, 2012/04/24 01:24:56]

Why changing these to strings?

[Robert Toscano, 2012/04/24 22:29:56]

Good question. The only reason why numbers were used before was because they
matched values in the native (C++) layer. In an effort to isolate the native
layer from the javascript layer, I tried to move all dependencies on
native-related constants into nativelayer.js. So the Margins class should not
assign any semantic meaning to these enum items. That said, I chose strings
because easy to use in boolean conditionals. Compare:

marginType = null;
if (marginType) // false

marginType = 'foo';
if (marginType) // true

marginType = 3;
if (marginType) // true

marginType = 0;
if (marginType) // false, but in this case we want it to be true since 0 would
be a valid value for a margin type.

[dpapad, 2012/04/24 22:50:19]

Alternatively you could add one more enum constant UNINITIALIZED = -1, and make
sure that whenever you declare a marginsType var you give it that value, then
you can replace
if (marginsType)
with
if (marginsType != Margins.Type.UNINITIALIZED)

I know is not as convenient but it would eliminate the need for a map, given
that you follow the enum order in the C++ code (and mention this in a comment
for future devs).

[Robert Toscano, 2012/04/28 01:41:37]

Ok, I'll go with what you suggest except for the UNINITIALIZED enum value. Seems
easier just to check for null instead.

[dpapad, 2012/04/30 16:19:26]

Ok, just make sure to initialize variables to null (or handle null and undefined
properly), specifically

var hello;
hello == null // true
hello === null // false
hello == undefined // true
hello === undefined // true

hello = null;
hello == null // true
hello === null // true
hello == undefined // true
hello === undefined // false

+    NO_MARGINS: 'no-margins',
+    MINIMUM: 'minimum',
+    CUSTOM: 'custom'
+  },
+
+  Margins.prototype = {
+    /** @return {number} Top margin in points. */
+    get top() {
+      return this.top_;
+    },
+
+    /** @return {number} Right margin in points. */
+    get right() {
+      return this.right_;
+    },
+
+    /** @return {number} Bottom margin in points. */
+    get bottom() {
+      return this.bottom_;
+    },
+
+    /** @return {number} Left margin in points. */
+    get left() {
+      return this.left_;
+    },
+
+    /**
+     * @param {print_preview.Margins} other The other margins object to compare
+     *     against.
+     * @return {boolean} Whether this margins object is equal to another.
+     */
+    equals: function(other) {
+      return other != null &&
+          this.top_ == other.top_ &&
+          this.right_ == other.right_ &&
+          this.bottom_ == other.bottom_ &&
+          this.left_ == other.left_;
+    }
+  };
+
+  // Export
+  return {
+    Margins: Margins
+  };
+});