Add custom Polymer network icon element

ui/webui/resources/js/cr/polymer_elements/cr_network_icon/cr-network-icon.js

Index: ui/webui/resources/js/cr/polymer_elements/cr_network_icon/cr-network-icon.js
diff --git a/ui/webui/resources/js/cr/polymer_elements/cr_network_icon/cr-network-icon.js b/ui/webui/resources/js/cr/polymer_elements/cr_network_icon/cr-network-icon.js
new file mode 100644
index 0000000000000000000000000000000000000000..2769368063162c697ef0073d9dbd7c311b5c5576
--- /dev/null
+++ b/ui/webui/resources/js/cr/polymer_elements/cr_network_icon/cr-network-icon.js
@@ -0,0 +1,166 @@
+// Copyright 2015 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.
+
+/**
+ * @fileoverview Polymer element for rendering network icons based on ONC
+ *     state properties.
+ */
+
+(function() {
+  /**
+   * @typedef({

[michaelpg, 2015/02/05 06:45:52]

I think this should be @typedef {{

("the outer braces indicate that this is a type expression and the inner braces
indicate that this is a record type")

and you need the closing }}.

https://developers.google.com/closure/compiler/docs/js-for-compiler

[stevenjb, 2015/02/05 22:41:34]

Done.

+   *   showDisconnected: boolean,
+   *   iconType: string,
+   *   connected: boolean,
+   *   secure: boolean
+   */
+  var IconParams_;

[michaelpg, 2015/02/05 06:45:52]

why the underscore? Or if this isn't really a variable should it just be a part
of the @typedef statement?

[stevenjb, 2015/02/05 22:41:33]

The _ is to indicate that it's private. Added the @private also after chatting
with Jeremy.

[Dan Beam, 2015/02/05 22:52:35]

fwiw, the style guide only asks for _ after private *properties and methods*

https://google-styleguide.googlecode.com/svn/trunk/javascriptguide.xml?showon...
https://google-styleguide.googlecode.com/svn/trunk/javascriptguide.xml?showon...
(@private)

+
+  /** @const */ var kResourceImageBase =

[Jeremy Klein, 2015/02/05 05:05:30]

nit: @const {string} here and below.

[michaelpg, 2015/02/05 06:45:52]

Jeremy, source for that syntax?

From https://developers.google.com/closure/compiler/docs/js-for-compiler:

The type declaration and additional comment are optional. If you provide a type
declaration, put the declaration on a separate line.

/**
 * @const
 * @type {string}
 */
var kResourceImageBase = '...';

[Jeremy Klein, 2015/02/05 07:30:36]

This was added in a compiler release Februrary 2013:

https://github.com/google/closure-compiler/wiki/Releases#february-27-2013-v20...

FWIW Google's code has >9500 instances of "@const {<type>}". Similar syntax for
@private and @protected were added in the release before that. At the very
least, these should be typed.

[stevenjb, 2015/02/05 22:41:34]

Done.

[Tyler Breisacher (Chromium), 2015/02/23 20:50:51]

Two drive by comments from the compiler team's POV:

1. If you see any documentation that seems out of date, please let us know (or
fix it yourself -- there's an "edit this file" button if you access
developers.google.com from a corp IP address).

2. If the convention in Chrome is that vars starting with 'k[A-Z]' are
constants, you can write a custom CodingConvention whose isConstant method
reflects that. The compiler will enforce the const-ness if you set
CompilerOptions.enforceAccessControlCodingConventions to true. Eventually it
will just be true by default. Setting that up right now is probably outside the
scope of this CL so @const is probably good for now.

+      'chrome://resources/images/polymer_elements/cr_network_icon/';
+  /** @const */ var kResourceImageExt = '.png';
+
+  /**
+   * Gets the network strength from the |networkState| object if available.
+   * @param {Object} networkState The ONC network state object provided by

[Jeremy Klein, 2015/02/05 07:30:35]

nit: "!Object"

[stevenjb, 2015/02/05 22:41:34]

Done.

+   *     chrome.networkingPrivate.getState().
+   * @return {integer} The strength value, [0-100]. This is always 0 for non

[Jeremy Klein, 2015/02/05 05:05:30]

This should be "number" instead of "integer"

[stevenjb, 2015/02/05 22:41:34]

Done.

+   *     wireless networks.
+   */
+  function getStrengthFromState(networkState) {

[Jeremy Klein, 2015/02/05 05:05:30]

The signal strength here can probably be a computed property based on the
networkState so that it's automatically updated when the network state changes.
Same goes for icon type.

[Jeremy Klein, 2015/02/05 05:05:30]

I feel like these functions can just be inside the Polymer call and then we
won't need the module pattern here at all. Is there a reason you'd strongly
prefer them not to be inside the element?

+    var type = networkState['Type'];
+    var strength = 0;
+    if (type == 'WiFi') {
+      var wifi = networkState['WiFi'];
+      if (wifi)
+        strength = wifi['SignalStrength'] || 0;
+    } else if (type == 'Cellular') {
+      var cellular = networkState['Cellular'];
+      if (cellular)
+        strength = cellular['SignalStrength'] || 0;
+    } else if (type == 'WiMAX') {
+      var wimax = networkState['WiMAX'];
+      if (wimax)
+        strength = wimax['SignalStrength'] || 0;
+    }
+    return strength;

[michaelpg, 2015/02/05 06:45:52]

This is kind of a lot :-\
Do we need to explicitly check the type?

var type = networkState['Type'];
if (networkState.hasOwnProperty(type))
  return networkState[type]['SignalStrength'] || 0;
return 0;

+  };
+
+  /**
+   * Gets the icon type from the networkState object. This allows multiple
+   * types (i.e. Cellular, WiMAX) to map to the same icon type (i.e. mobile).

[michaelpg, 2015/02/05 06:45:52]

"from the |networkType| object"

[stevenjb, 2015/02/05 22:41:33]

Done.

+   * @param {string} networkType The ONC network type.
+   * @return {string} The icon type: ethernet, wifi, mobile, or vpn.
+   */
+  function getIconTypeFromNetworkType(networkType) {
+    if (networkType == 'Ethernet') {
+      return 'ethernet';
+    } else if (networkType == 'WiFi') {
+      return 'wifi';
+    } else if (networkType == 'Cellular') {
+      return 'mobile';
+    } else if (networkType == 'WiMAX') {
+      return 'mobile';
+    } else if (networkType == 'VPN') {
+      return 'vpn';
+    }
+    console.log('Unrecognized network type: ' + networkType);
+    return 'ethernet';
+  }
+
+  /**
+   * Polymer class definition for 'cr-network-icon'.
+   */
+  Polymer('cr-network-icon', {
+    publish: {
+      /**
+       * If true, the icon is part of a list of networks and will not show
+       * the disconnected image.

[michaelpg, 2015/02/05 06:45:52]

I don't understand this comment. Could the attribute just be called
showDisconnected or something?

[stevenjb, 2015/02/05 22:41:34]

The attribute is intended to describe the context, either a list item or a
non-list item. The fact that we show the disconnected icon only when not in a
list is an implementation detail. There will be other details that differ as we
add layers of complexity here (e.g. additional badges for lte/3g). Modified the
comment.

+       *
+       * @attribute listItem
+       * @type boolean
+       * @default false

[michaelpg, 2015/02/05 06:45:52]

So, @attribute and @default aren't used by closure AFAIK, but they are parsed by
the core-doc-viewer element:
https://github.com/Polymer/core-doc-viewer

But, I don't know how we'd need to change the core-doc-viewer to be compatible
with Polymer elements split into multiple files (.js, .html[, .css]).

See https://github.com/Polymer/core-icon/blob/master/core-icon.html for an
example... should the top-level description of the element be in our .js files
or our .html files?

(If we're using publish blocks (JS) rather than declaring attributes (HTML) for
0.8.x compatibility maybe it makes sense to just keep all the documentation
comments in the JS files.)

[Jeremy Klein, 2015/02/05 07:30:36]

I wonder if it might be worth trying to get the doc generator working with split
up sources. The other thing that uses these docs is the externs generator, but I
don't think we should need that if we're actually using real closure jsdocs :-).
The polymer vs closure jsdoc question is definitely one we'll need to think
about, though. I definitely think we'll want the closure jsdocs, but should we
ban all polymer docs altogether? It seems like some of them are contradicting.
For example:

@type boolean vs @type {boolean} here.

That may be one of the cases we should try to push the polymer team to change
for 0.8.

[stevenjb, 2015/02/05 22:41:33]

So, I just copy/pasted this from paper-button.html.

This is definitely the kind of discussion I wanted to start. Michael or Jeremy,
since you both understand this better than I do, could one of you start a thread
on the polymer list then follow up with what we should put here?

+       */
+      listItem: false
+    },
+
+    /**
+     * Updates the icon based on |networkState|. Should be called whenever the
+     * state changes.
+     * @param {Object} networkState The ONC network state object provided by

[Jeremy Klein, 2015/02/05 07:30:36]

Is there any stronger type we can use for the ONC objects?

[stevenjb, 2015/02/05 22:41:34]

I am going to re-factor this part and create a separate ONC network state object
which will hopefully improve this a bunch.

+     *    chrome.networkingPrivate.getState().

[michaelpg, 2015/02/05 06:45:52]

nit: 4 space indent (5 spaces total)

[stevenjb, 2015/02/05 22:41:34]

Done.

+     */
+    setNetworkState: function(networkState) {

[Jeremy Klein, 2015/02/05 05:05:30]

It seems like the network state and network type could be bound published
properties with *changed handlers.

[stevenjb, 2015/02/05 22:41:34]

I think that may be possible with the re-factor, will look into that.

+      var params = /** @type {IconParams} */ {};
+      params.showDisconnected = !this.$.listItem;

[michaelpg, 2015/02/05 06:45:52]

Does this work?

The members of this.$ should be the statically created shadow DOM nodes,
referenced by ID. The listItem attribute should just be available at
"this.listItem".

[stevenjb, 2015/02/05 22:41:33]

Done.

+      params.iconType = getIconTypeFromNetworkType(networkState['Type']);
+
+      if (params.iconType_ == 'wifi') {
+        var wifi = networkState['WiFi'];
+        if (wifi && wifi['Security'] && wifi['Security'] != 'None')
+          params.secure = true;
+      }
+      params.connected = networkState['ConnectionState'] == 'Connected';
+
+      params.strength = getStrengthFromState(networkState);
+      this.setIcon_(params);
+    },
+
+    /**
+     * Provides the ONC network type to display a disconnected icon for the
+     *    specified type.

[michaelpg, 2015/02/05 06:45:52]

4+1=5 spaces

[stevenjb, 2015/02/05 22:41:34]

Done.

+     * @param {string} type The ONC network type.
+     */
+    setNetworkType: function(networkType) {
+      var params = /** @type {IconParams} */ {};
+      params.showDisconnected = true;
+      params.iconType = getIconTypeFromNetworkType(networkType);
+      params.connected = false;
+      params.secure = false;
+      params.strenght = 0;

[michaelpg, 2015/02/05 06:45:52]

s/strenght/strength/

& strength isn't listed in the typedef

+
+      this.setIcon_(params);
+    },
+
+    /**
+     * Sets the icon and badge based on the current state and |strength|.
+     * @param {IconParams} params The set of params describing the icon.

[Jeremy Klein, 2015/02/05 07:30:35]

nit: "!IconParams" to ensure non-null.

[stevenjb, 2015/02/05 22:41:33]

Done.

+     * @private
+     */
+    setIcon_: function(params) {
+      var iconOffset;
+      if (params.iconType == 'wifi' || params.iconType == 'mobile') {
+        if (params.showDisconnected && !params.connected) {

[michaelpg, 2015/02/05 06:45:52]

nit: remove unnecessary braces for single-line if statements

[stevenjb, 2015/02/05 22:41:34]

I chrome both are valid. For single if statements with a single line we almost
always omit the brackets. For compound if statements, opinions and usage varies.
How much do we want to care about that for the Settings code?

[michaelpg, 2015/02/06 18:52:38]

I'll leave that up to dbeam@.

[James Hawkins, 2015/02/06 18:58:45]

The style rule from [1] is to leave the braces off single-link logic blocks. 
The rule came from arv, and the rationale was around minimizing code size. 
Given that we're ultimately introducing compilation (thus minification), we need
to revisit that rule (and honestly I think we should start following the JS
style guide here, which requires braces for all blocks regardless of length).

[1] http://www.chromium.org/developers/web-development-style-guide

+          iconOffset = '0px';
+        } else if (params.strength <= 25) {
+          iconOffset = '-100%';
+        } else if (params.strength <= 50) {
+          iconOffset = '-200%';
+        } else if (params.strength <= 75) {
+          iconOffset = '-300%';
+        } else {
+          iconOffset = '-400%';
+        }
+      }
+
+      var icon = this.$.icon;
+      if (params.iconType) {
+        icon.src = kResourceImageBase + params.iconType + kResourceImageExt;
+        if (iconOffset) {
+          icon.style.height = '500%';
+          icon.style.top = iconOffset;
+        }
+      }
+
+      var badge = this.$.badge;
+      if (params.secure) {
+        badge.style.display = undefined;

[michaelpg, 2015/02/05 06:45:52]

nit(?): badge.style.display = '';

[stevenjb, 2015/02/05 22:41:34]

So, undefined clearly sets the behavior to whatever the default is. Presumably
'' does the same thing, but it is not obvious to me that is necessarily the
case? This must be a common pattern, is there a "best practice" here?

[michaelpg, 2015/02/06 18:52:38]

Actually this is not a nit, it seems to be a bug:
~ badge.style.display = 'none';
~ badge.style.display
    "none"
~ badge.style.display = undefined;
~ badge.style.display
    "none"
~ getComputedStyle(badge).display
    "none"
~ badge.style.display = '';
~ badge.style.display
    ""
~ getComputedStyle(badge).display
    "block"

I'm having a hard time finding where this behavior is actually documented. I
feel like it ought to be here:
http://www.w3.org/TR/DOM-Level-2-Style/css.html#CSS-ElementCSSInlineStyle

+        badge.src = kResourceImageBase + 'secure' + kResourceImageExt;
+      } else {
+        badge.style.display = 'none';
+      }
+    }
+
+  });
+})();