Settings People Revamp: Implement parts of ChromeOS Change Picture

chrome/browser/resources/settings/people_page/change_picture.js

+// 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
+ * 'settings-change-picture' is the settings subpage containing controls to
+ * edit a ChromeOS user's picture.
+ *
+ * @group Chrome Settings Elements
+ * @element settings-change-picture
+ */
+Polymer({
+  is: 'settings-change-picture',
+
+  behaviors: [
+    I18nBehavior,
+  ],
+
+  properties: {
+    /**
+     * The currently selected profile image URL. May be a data URL.
+     * @private {string}
+     */
+    selectedImageUrl_: String,
+
+    /**
+     * The url of the 'old' image, which is the existing image sourced from
+     * the camera, a file, or a deprecated default image.
+     * @private {string}
+     */
+    oldImageUrl_: String,
+
+    /**
+     * The url of the profile image.
+     * @private {string}
+     */
+    profileImageUrl_: {
+      type: String,
+      value: settings.ChangePicturePrivateApi.ButtonImages.PROFILE_PICTURE,
+    },
+
+    /**
+     * The default user image images. Populated by ChangePicturePrivateApi.

[dpapad, 2016/01/11 19:25:50]

Maybe change to "The default user images"?

[tommycli, 2016/01/13 18:29:11]

Done.

+     * @private {!Array<!settings.DefaultImage>}
+     */
+    defaultImages_: {
+      type: Array,
+      value: function() { return []; },
+    },
+  },
+
+  /** @override */
+  created: function() {
+    settings.ChangePicturePrivateApi.initialize(
+        this.handleDefaultImages_.bind(this),
+        this.handleSelectedImageUrl_.bind(this),
+        this.handleOldImageUrl_.bind(this),
+        this.handleProfileImageUrl_.bind(this));
+  },
+
+  /**
+   * Handler for when the default images are pushed from
+   * ChangePicturePrivateApi.
+   * @private
+   * @param {!Array<!settings.DefaultImage>} images
+   */
+  handleDefaultImages_: function(images) {
+    this.defaultImages_ = images;
+  },
+
+  /**
+   * Handler for when the selected image URL is pushed from
+   * ChangePicturePrivateApi.
+   * @private
+   * @param {string} imageUrl
+   */
+  handleSelectedImageUrl_: function(imageUrl) {
+    this.selectedImageUrl_ = imageUrl;
+  },
+
+  /**
+   * Handler for when there is an 'old' image is pushed from
+   * ChangePicturePrivateApi. When an 'old' image is pushed, it implies that
+   * it's the selected one.
+   * @private
+   * @param {string} imageUrl
+   */
+  handleOldImageUrl_: function(imageUrl) {
+    this.oldImageUrl_ = imageUrl;
+    this.selectedImageUrl_ = imageUrl;
+  },
+
+  /**
+   * Handler for when there the profile image is pushed from
+   * ChangePicturePrivateApi.
+   * @private
+   * @param {string} imageUrl
+   * @param {boolean} selected
+   */
+  handleProfileImageUrl_: function(imageUrl, selected) {
+    this.profileImageUrl_ = imageUrl;
+    if (selected)
+      this.selectedImageUrl_ = imageUrl;
+  },
+
+  /**
+   * Handler for when the user clicks a new profile image.
+   * @private
+   * @param {!Event} event
+   */
+  onDefaultImageTap_: function(event) {
+    var element = Polymer.dom(event).rootTarget;
+
+    var imageUrl;

[dpapad, 2016/01/11 19:25:50]

Nit: Avoid implicit boolean conversions when possible.

var imageUrl = null;
...
...

if (imageUrl != null) {
 ...
}

[tommycli, 2016/01/13 18:29:11]

Done.

+    if (element.nodeName == 'IMG')

[dpapad, 2016/01/11 19:25:50]

Should we change this to
if (element.nodeName.toUpperCase() == 'IMG')

to make it a bit more robust?

[michaelpg, 2016/01/11 20:38:05]

element.nodeName is always element.tagName for Elements, which is always
uppercase.

and if the element isn't an Element, you have other problems.

+      imageUrl = element.src;
+    else if (element.dataset && element.dataset.imageUrl)
+      imageUrl = element.dataset.imageUrl;
+
+    if (imageUrl) {
+      settings.ChangePicturePrivateApi.selectDefaultImage(imageUrl);
+      // Button toggle state is instead controlled by the selected image URL.
+      event.preventDefault();
+    }
+  },
+
+  /**
+   * Handler for when the user clicks the 'old' image.
+   * @private
+   * @param {!Event} event
+   */
+  onOldImageTap_: function(event) {
+    settings.ChangePicturePrivateApi.selectOldImage();
+    // Button toggle state is instead controlled by the selected image URL.
+    event.preventDefault();
+  },
+
+  /**
+   * Handler for when the user clicks the 'profile' image.
+   * @private
+   * @param {!Event} event
+   */
+  onProfileImageTap_: function(event) {
+    settings.ChangePicturePrivateApi.selectProfileImage();
+    // Button toggle state is instead controlled by the selected image URL.
+    event.preventDefault();
+  },
+
+  /**
+   * Computed binding determining which profile image button is toggled on.
+   * @private
+   * @param {!string} imageUrl

[dpapad, 2016/01/11 19:25:50]

No need to put "!" before "string". It is implied, see
https://developers.google.com/closure/compiler/docs/js-for-compiler?hl=en#non...,
specifically this sentence

"Functions and all value types (boolean, number, and string) are non-nullable by
default whether or not they are declared with the Non-nullable operator."

[tommycli, 2016/01/13 18:29:11]

Done.

+   * @param {!string} selectedImageUrl
+   * @return {boolean}
+   */
+  isActiveImage_: function(imageUrl, selectedImageUrl) {
+    return imageUrl == selectedImageUrl;
+  },
+});