Settings People Revamp: Switch Change Picture to use iron-selector.

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

Index: chrome/browser/resources/settings/people_page/change_picture.js
diff --git a/chrome/browser/resources/settings/people_page/change_picture.js b/chrome/browser/resources/settings/people_page/change_picture.js
index 0501795342f850a909814afb4a447d4bab3550a9..d3fe5a3b054fe2231f8870f177254b70e966e821 100644
--- a/chrome/browser/resources/settings/people_page/change_picture.js
+++ b/chrome/browser/resources/settings/people_page/change_picture.js
@@ -30,28 +30,19 @@ Polymer({
 
   properties: {
     /**
-     * The currently selected profile image URL. May be a data URL.
-     * @private {string}
+     * The currently selected item. This property is bound to the iron-selector
+     * and never directly assigned.
+     * @private {Element}
      */
-    selectedImageUrl_: {
-      type: String,
+    selectedItem_: {
+      type: Element,
       notify: settings_test.changePictureOptions.notifyPropertyChangesForTest,
     },
 
     /**
-     * True only if the user has selected the camera icon. Old photos captured
-     * from the camera are represented as the 'old' image.
-     * @private {boolean}
-     */
-    cameraActive_: {
-      type: Boolean,
-      value: false,
-    },
-
-    /**
      * This differs from its default value only if the user has just captured
      * a new photo from the camera.
-     * @private {string}
+     * @private {!string}

[dpapad, 2016/01/22 02:35:32]

Nit, "!" is implied with string, number and boolean types. See
https://developers.google.com/closure/compiler/docs/js-for-compiler?hl=en,
specifically this part

"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/22 19:24:40]

Done. Oops! Thanks!

      */
     cameraImageUrl_: {
       type: String,
@@ -61,7 +52,7 @@ Polymer({
     /**
      * This differs from its default value only if the user has just captured
      * a new photo from the camera.
-     * @private {string}
+     * @private {!string}
      */
     cameraImageTitle_: {
       type: String,
@@ -70,14 +61,19 @@ Polymer({
 
     /**
      * The url of the 'old' image, which is the existing image sourced from
-     * the camera, a file, or a deprecated default image.
-     * @private {string}
+     * the camera, a file, or a deprecated default image. It defaults to an
+     * empty string instead of undefined, because Polymer bindings don't behave
+     * as expected with undefined properties.
+     * @private {!string}
      */
-    oldImageUrl_: String,
+    oldImageUrl_: {
+      type: String,
+      value: '',
+    },
 
     /**
      * The url of the profile image.
-     * @private {string}
+     * @private {!string}
      */
     profileImageUrl_: {
       type: String,
@@ -107,12 +103,20 @@ Polymer({
       }.bind(this),
 
       /**
-       * Called from C++ to provide the URL of the selected image.
+       * Called from C++ to provide the URL of the selected image. Is only
+       * called with default images.
        * @param {string} imageUrl
        */
       receiveSelectedImage: function(imageUrl) {
-        this.cameraActive_ = false;
-        this.selectedImageUrl_ = imageUrl;
+        var images = this.$.selector.items;

[dpapad, 2016/01/22 02:35:32]

Nit (optional): You can also do it as follows (I think is less work).

var index = this.$.selector.items.findIndex(
    function(image) {
      return image.dataset.type == 'default' && image.src == imageUrl;
    });
assertTrue(index != -1, .....);
this.$.selector.select(index);

[tommycli, 2016/01/22 19:24:40]

Done.

+        for (var i = 0; i < images.length; i++) {
+          if (images[i].dataset.type == 'default' &&
+              images[i].src == imageUrl) {
+            this.$.selector.select(i);
+            return;
+          }
+        }
+        assertNotReached('Default image with URL ' + imageUrl + ' not found.');
       }.bind(this),
 
       /**
@@ -123,9 +127,8 @@ Polymer({
        * @param {string} imageUrl
        */
       receiveOldImage: function(imageUrl) {
-        this.cameraActive_ = false;
         this.oldImageUrl_ = imageUrl;
-        this.selectedImageUrl_ = imageUrl;
+        this.$.selector.select(this.$.selector.indexOf(this.$.oldImage));

[dpapad, 2016/01/22 02:35:32]

iron-selector#indexOf returns a number, but iron-selector#select is documented
as expecting a string. Is the documentation not accurate or is there an implicit
conversion happening here?

[tommycli, 2016/01/22 19:24:40]

Yes... exactly. The selection key can be a string if you attach it to a custom
attribute.

However, the default is to use the index.

       }.bind(this),
 
       /**
@@ -135,10 +138,8 @@ Polymer({
        */
       receiveProfileImage: function(imageUrl, selected) {
         this.profileImageUrl_ = imageUrl;
-        if (selected) {
-          this.cameraActive_ = false;
-          this.selectedImageUrl_ = imageUrl;
-        }
+        if (selected)
+          this.$.selector.select(this.$.selector.indexOf(this.$.profileImage));
       }.bind(this),
 
       /**
@@ -161,64 +162,47 @@ Polymer({
   },
 
   /**
-   * Handler for when the user clicks the camera image.
-   * @private
-   */
-  onCameraImageTap_: function() {
-    this.cameraActive_ = true;
-  },
-
-  /**
-   * Handler for when the user clicks a new profile image.
-   * @private
+   * Handler for when the an image is activated.
    * @param {!Event} event
-   */
-  onDefaultImageTap_: function(event) {
-    var element = Polymer.dom(event).rootTarget;
-
-    var imageUrl = null;
-    if (element.nodeName == 'IMG')
-      imageUrl = element.src;
-    else if (element.dataset && element.dataset.imageUrl)
-      imageUrl = element.dataset.imageUrl;
-
-    if (imageUrl != null) {
-      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();
+  onImageActivate_: function(event) {
+    var selectedImage = event.detail.item;
+    switch (selectedImage.dataset.type) {
+      case 'camera':
+        // TODO(tommycli): Implement camera functionality.
+        break;
+      case 'profile':
+        settings.ChangePicturePrivateApi.selectProfileImage();
+        break;
+      case 'old':
+        settings.ChangePicturePrivateApi.selectOldImage();
+        break;
+      case 'default':
+        settings.ChangePicturePrivateApi.selectDefaultImage(selectedImage.src);
+        break;
+      default:
+        assertNotReached('Selected unknown image type');
+    }
   },
 
   /**
-   * Handler for when the user clicks the 'profile' image.
+   * True if there is no old image and the selection icon should be hidden.
+   * @param {!string} oldImageUrl
    * @private
-   * @param {!Event} event
    */
-  onProfileImageTap_: function(event) {
-    settings.ChangePicturePrivateApi.selectProfileImage();
-    // Button toggle state is instead controlled by the selected image URL.
-    event.preventDefault();
+  isOldImageHidden_: function(oldImageUrl) {
+    return !oldImageUrl;

[dpapad, 2016/01/22 02:35:32]

This can only return false if oldImageUrl is the empty string (null/undefined
are ruled out by the type annotation). So I suggest making the check more
explicit here to avoid implicit string to bool conversion.

return oldImageUrl.length == 0;

[tommycli, 2016/01/22 19:24:40]

Done.

   },
 
   /**
-   * Computed binding determining which profile image button is toggled on.
+   * True if the preview image is hidden and the camera controls are shown.
+   * @param {!Element} selectedItem
    * @private
-   * @param {string} imageUrl
-   * @param {string} selectedImageUrl
-   * @return {boolean}
    */
-  isActiveImage_: function(cameraActive, imageUrl, selectedImageUrl) {
-    return !cameraActive && (imageUrl == selectedImageUrl);
+  isPreviewImageHidden_: function(selectedItem) {
+    // The selected item can be undefined on initial load and between selection
+    // changes. In those cases, show the preview image.
+    return selectedItem != undefined && selectedItem.dataset.type == 'camera';
   },
 });