Add custom Polymer network icon element

ui/webui/resources/cr_elements/cr_network_icon/cr_network_icon.js

Index: ui/webui/resources/cr_elements/cr_network_icon/cr_network_icon.js
diff --git a/ui/webui/resources/cr_elements/cr_network_icon/cr_network_icon.js b/ui/webui/resources/cr_elements/cr_network_icon/cr_network_icon.js
new file mode 100644
index 0000000000000000000000000000000000000000..f970f00dafd3ae9f70da9551e8addeea7f9db49a
--- /dev/null
+++ b/ui/webui/resources/cr_elements/cr_network_icon/cr_network_icon.js
@@ -0,0 +1,178 @@
+// 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.
+
+

[Dan Beam, 2015/02/20 01:02:43]

why \n\n?

[stevenjb, 2015/02/20 02:22:47]

Some places we do \n\n before /**, other places we don't, I haven't been able to
figure out the pattern yet.

Removed the extra \n.

[michaelpg, 2015/02/20 10:00:17]

closure (compiler? linter?) complains if you don't have two \n's between
top-level /** */s. Not sure if it cares about the C-style comments.

[stevenjb, 2015/02/23 19:23:08]

This doesn't complain:
./tools/gyp/gyp --depth .
chrome/browser/resources/chromeos/network_ui/compiled_resources.gyp && ninja -C
out/Default

I'm with Dan, this is Chrome code, we should stick to Chrome styling. If closure
complains for some reason we should fix or disable warnings in that.

[Dan Beam, 2015/02/23 19:27:59]

the linter runs on `git cl presubmit`, but doesn't enforce the closurey checks,
I don't believe (\n\n is a closure thing, if not a "strict" mode one).

+/**
+ * @fileoverview Polymer element for rendering network icons based on ONC
+ * state properties.
+ */
+
+(function() {
+  /**
+   * @typedef {{
+   *   showDisconnected: boolean,
+   *   iconType: string,
+   *   connected: boolean,
+   *   secure: boolean,
+   *   strength: number }}
+   */
+  var IconParams;
+
+  /** @const {string} */ var RESOURCE_IMAGE_BASE =
+      'chrome://resources/cr_elements/cr_network_icon/';
+  /** @const {string} */ var RESOURCE_IMAGE_EXT = '.png';
+
+
+  /**
+   * Gets the icon type from the network type. This allows multiple types
+   * (i.e. Cellular, WiMAX) to map to the same icon type (i.e. mobile).
+   * @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';
+    }

[Dan Beam, 2015/02/20 01:02:43]

no curlies

[stevenjb, 2015/02/20 02:22:47]

Done (although some of us discussed this, and I prefer the chrome C++ rule where
curlies are always acceptable, especially for multi line if statements). Iirc,
no consensus was reached.

[Dan Beam, 2015/02/20 04:01:24]

consensus in chrome js is no curlies for consistency, however there are some
exceptions for super-closurey code (see: print preview, remoting?).  whatever
y'all do, just be consistent.  i'm only enforcing the rules i know.

[stevenjb, 2015/02/23 19:23:08]

I'll skip any jokes about "consensus in chrome" :) This particular nit varies
widely within different parts of chrome, but I agree that consistency within
Settings is good and so have changed these.

[Dan Beam, 2015/02/23 19:43:02]

fwiw, I'm shutting up about style in your new code.

it's very hard to understand what webui would look like if it were all rewritten
(what y'all are trying to do).  it's quite possible we'd change a few things to
be like closure or polymer.

ultimately you're writing code that is influenced by:

- Chrome
 - which attempts to listen to Google's JS & C++ style guides
 - which is a mostly C++ product, so C++'y rules have higher value
   (so ramp time for C++-experienced engineers is faster)

- the wider JavaScript community
 - various conflicting voices here (JSLint, JSHint, npm, other style guides)

- Closure compiler/library style
 - which most Googlers use/agree on
 - mildly contradicts typical JS-style
   (casts and other odd things, more Java-like)

- Polymer
 - which is new and pretty different as well

Chrome JS falls in the middle of all these things.  if it were more similar to
*any* of these things (e.g. Closure or web-y JS), there'd probably be an easier
road ahead of you (better tooling and community support).

my $0.02.

+    console.error('Unrecognized network type: ' + networkType);

[Dan Beam, 2015/02/20 01:02:43]

assertNotReached()

[stevenjb, 2015/02/20 02:22:47]

I don't think this should assert. The input to this is data driven, and it's
entirely possible a new network type will be introduced in which case we want to
be informed so we can implement an icon type, but the code shouldn't need to
crash.

+    return 'ethernet';
+  }
+
+
+  /**
+   * Polymer class definition for 'cr-network-icon'.
+   * @element cr-network-icon
+   */
+  Polymer('cr-network-icon', {
+    publish: {
+      /**
+       * If set, the ONC data properties will be used to display the icon.
+       *
+       * @attribute networkState
+       * @type CrOncDataElement
+       * @default null
+       */
+      networkState: null,
+
+

[Dan Beam, 2015/02/20 01:02:43]

why \n\n?

[stevenjb, 2015/02/20 02:22:47]

I copied this pattern from somewhere, maybe Polymer? It seems we are not using
it in other CLs now, so I will remove these.

[michaelpg, 2015/02/20 10:00:17]

I might've been the one to introduce this, thinking of closure's insistence on
two blank lines before /** */, but we definitely don't need it inside a function
and Polymer doesn't do it this way.

[stevenjb, 2015/02/23 19:23:08]

Again, let's fix closure if we need to. If we can't, at the point where we have
tools or tests that complain, we can create the minimal set of hard-fast rules
and fix anything that needs it.

+      /**
+       * If set, the ONC network type will be used to display the icon.
+       *
+       * @attribute networkType
+       * @type string

[Dan Beam, 2015/02/20 01:02:43]

can this be string|undefined?

[Jeremy Klein, 2015/02/20 01:23:59]

I think it'd be nice to be consistent between null and undefined here. Is there
any reason this needs to be undefined instead of null or empty string?

Polymer elements have lots of examples of default null, but no default
undefined.

[stevenjb, 2015/02/20 02:22:47]

I guess the C++ programmer in me associates 'null' with objects, not strings.

I kind of prefer null over '' because generally either networkState or
networkType will be set, but not both, but I don't feel strongly.

+       * @default undefined
+       */
+      networkType: undefined,
+
+
+      /**
+       * If true, the icon is part of a list of networks and may be displayed
+       * differently, e.g. the disconnected image will never be shown for
+       * list items.
+       *
+       * @attribute isListItem
+       * @type boolean
+       * @default false
+       */
+      isListItem: false
+    },
+

[Dan Beam, 2015/02/20 01:02:43]

assuming this automatically gets called as part of polymer, @override

[stevenjb, 2015/02/20 02:22:47]

We haven't been doing this elsewhere. Jeremy, Michael, care to weigh in? 

How should this look, like this?
    /** @override */

[Dan Beam, 2015/02/20 04:01:23]

yes

[michaelpg, 2015/02/20 10:00:17]

meh, I'm kind of indifferent, @override is useful when we're hiding a function
we've already documented, so I guess we should always include it to be
consistent.

[Jeremy Klein, 2015/02/20 18:32:26]

+1 to @override for lifecycle callbacks.

[stevenjb, 2015/02/23 19:23:08]

I like /** @override */ for all the reasons we have added it in C++. A dev
looking at this code should have a hint that 'attached' is getting called from
Polymer.

+    attached: function() {
+      var params = /** @type {IconParams} */ {
+        showDisconnected: true,
+        iconType: 'ethernet',
+        secure: false,
+        connected: false,
+        strength: 0
+      };
+      this.setIcon_(params);
+    },
+
+    /**
+     * Updates the icon based on network state when the networkState property
+     * changes.
+     * @param {CrOncDataElement} oldValue Ignored.

[Dan Beam, 2015/02/20 01:02:42]

why is oldValue sent then?

[Jeremy Klein, 2015/02/20 01:23:59]

This is just built into the polymer library as a callback when property foo
changes:

https://www.polymer-project.org/docs/polymer/polymer.html#change-watchers

I'd actually just recommend killing all the params here and using
this.networkState instead of newValue. You may be able to do that elsewhere too.

[stevenjb, 2015/02/20 02:22:47]

I don't especially like the idea of overriding fooChaned(old, new) with
fooChanged(). And using this.networkState requires knowledge that networkState
has already been assigned, which I guess should be obvious, but I still kind of
prefer this pattern.

Other perspectives?

[michaelpg, 2015/02/20 10:00:17]

Agreed.

[Jeremy Klein, 2015/02/20 18:32:26]

I'm fine with that, but keep in mind that leaving off the parameters is
extremely common among polymer elements, even more so than leaving them in. Just
check out 

https://github.com/Polymer/core-selector/blob/master/core-selector.html

or any other element with changed handlers for examples. This is also pretty
common in Google code for unused params in callbacks.

Also this.foo being updated in fooChanged is a guarantee as part of the API. As
a matter of fact, a quick code search over all of the core and paper elements
shows that there are a handful changed handlers which keep the old param, but
only 1 element in all of the core and paper elements which even has the new
value in its parameter list (and it doesn't even use it). I personally think it
makes the code much more readable to avoid unused parameters when we can.

[stevenjb, 2015/02/23 19:23:08]

After thinking about the @override question above, it seems like a good idea to
indicate that these are also called from Polymer. Does it make sense to create a
pseudo-directive, e.g. @polymer-override, or just leave it to the implementer?

[Dan Beam, 2015/02/23 19:27:59]

difference is that closure compiler actually enforces @override (which may be
good or bad depending on if polymer compiles).  we have a compiler-pass that
Does Magic in which you could recognize @polymer-override, but I don't really
see a need for a new directive, IMO (it's functionally similar).

+     * @param {CrOncDataElement} newValue The new network state used to set the
+     *     icon.
+     */
+    networkStateChanged: function(oldValue, newValue) {
+      var networkState = newValue;
+      var iconType = getIconTypeFromNetworkType(networkState.data.Type);
+      var params = /** @type {IconParams} */ {
+        showDisconnected: !this.isListItem,
+        iconType: iconType,
+        secure: iconType == 'wifi' && networkState.getWiFiSecurity() != 'None',
+        connected: networkState.data.ConnectionState == 'Connected',
+        strength: networkState.getStrength()

[Dan Beam, 2015/02/20 01:02:42]

nit: alpha, add trailing ,

[stevenjb, 2015/02/20 02:22:47]

alpha?

I assume all object definitions should include a trailing , ?

[Dan Beam, 2015/02/20 04:01:24]

betize
if possible, i'd say so, yeah

[stevenjb, 2015/02/23 19:23:08]

Done.

+      };
+      this.setIcon_(params);
+    },
+
+
+    /**
+     * Updates the icon based on network type when the networkType property
+     * changes.
+     * @param {string} oldValue Ignored.
+     * @param {string} newValue The new network type used to set the icon.
+     */
+    networkTypeChanged: function(oldValue, newValue) {
+      var networkType = newValue;

[Dan Beam, 2015/02/20 01:02:43]

opt nit: maybe find a way to explain that newValue is a networkType without
potential runtime costs?

[Jeremy Klein, 2015/02/20 01:23:59]

My suggestion above probably resolves this.

[stevenjb, 2015/02/20 02:22:47]

I'm not sure what 'is a networkType' means exactly.

[Dan Beam, 2015/02/20 04:01:23]

the fact that you wrote `var networkType = newValue;` means that you're trying
to explain that newValue "is a networkType".  networkType should probably also
be an @enum {string}.

[stevenjb, 2015/02/23 19:23:08]

OK, I wasn't familiar with @enum. I created CrOnc.Type.

+      var params = /** @type {IconParams} */ {
+        showDisconnected: true,
+        iconType: getIconTypeFromNetworkType(networkType),
+        connected: false,
+        secure: false,
+        strength: 0
+      };
+      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.
+     * @private
+     */
+    setIcon_: function(params) {
+      var iconOffset;
+      if (params.iconType == 'wifi' || params.iconType == 'mobile') {
+        if (params.showDisconnected && !params.connected) {
+          iconOffset = '0px';

[Dan Beam, 2015/02/20 01:02:42]

nit: why not use numbers? offset + '%' should work as well even when offset=0,
no?

[stevenjb, 2015/02/20 02:22:47]

You mean e.g. iconOffset = -100, then icon.style.top = iconOffset + '%' below? I
guess to me that seems a little less clear?

+        } else if (params.strength <= 25) {
+          iconOffset = '-100%';
+        } else if (params.strength <= 50) {
+          iconOffset = '-200%';
+        } else if (params.strength <= 75) {
+          iconOffset = '-300%';
+        } else {
+          iconOffset = '-400%';
+        }

[Dan Beam, 2015/02/20 01:02:43]

no curlies

[stevenjb, 2015/02/20 02:22:47]

Done.

+      }
+
+      var icon = this.$.icon;
+      if (params.iconType) {
+        icon.src = RESOURCE_IMAGE_BASE + params.iconType + RESOURCE_IMAGE_EXT;
+        if (iconOffset) {
+          icon.style.height = '500%';
+          icon.style.top = iconOffset;
+        }
+      }
+
+      var badge = this.$.badge;
+      if (params.secure) {
+        badge.style.display = '';
+        badge.src = RESOURCE_IMAGE_BASE + 'secure' + RESOURCE_IMAGE_EXT;
+      } else {
+        badge.style.display = 'none';

[Dan Beam, 2015/02/20 01:02:43]

don't do this.  use .hidden = true/false

[stevenjb, 2015/02/20 02:22:47]

Done.

+      }
+    }
+  });
+})();