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

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.
 
 cr.define('settings_test', function() {
   var changePictureOptions = settings_test.changePictureOptions || {
     /**
      * True if property changes should fire events for testing purposes.
      * @type {boolean}
      */
@@ skipping 12 matching lines @@
  */
 Polymer({
   is: 'settings-change-picture',
 
   behaviors: [
     I18nBehavior,
   ],
 
   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,
       value: settings.ChangePicturePrivateApi.ButtonImages.TAKE_PHOTO,
     },
 
     /**
      * 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,
       value: function() { return loadTimeData.getString('takePhoto'); },
     },
 
     /**
      * 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,
       value: settings.ChangePicturePrivateApi.ButtonImages.PROFILE_PICTURE,
     },
 
     /**
      * The default user images. Populated by ChangePicturePrivateApi.
      * @private {!Array<!settings.DefaultImage>}
      */
     defaultImages_: {
       type: Array,
       value: function() { return []; },
     },
   },
 
   /** @override */
   attached: function() {
     // This is the interface called by the C++ handler.
     var nativeInterface = {
       /**
        * Called from C++ to provide the default set of images.
        * @param {!Array<!settings.DefaultImage>} images
        */
       receiveDefaultImages: function(images) {
         this.defaultImages_ = images;
       }.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),
 
       /**
        * Called from C++ to provide the URL of the 'old' image. The 'old'
        * image is any selected non-profile and non-default image. It can be
        * from the camera, a file, or a deprecated default image. When this
        * method is called, it's implied that the old image is selected.
        * @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),
 
       /**
        * Called from C++ to provide the URL of the profile image.
        * @param {string} imageUrl
        * @param {boolean} selected
        */
       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),
 
       /**
        * Called from the C++ to notify page about camera presence.
        * @param {boolean} cameraPresent
        */
       receiveCameraPresence: function(cameraPresent) {
         // TODO(tommycli): Implement camera functionality.
       }.bind(this),
     };
 
     cr.define('settings', function() {
       var ChangePicturePage = nativeInterface;
       return {
         ChangePicturePage: ChangePicturePage,
       };
     });
 
     settings.ChangePicturePrivateApi.initialize();
   },
 
   /**
-   * Handler for when the user clicks the camera image.
+   * Handler for when the an image is activated.
+   * @param {!Event} event
    * @private
    */
-  onCameraImageTap_: function() {
-    this.cameraActive_ = true;
-  },
-
-  /**
-   * 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 = 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();
+  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 'old' image.
+   * True if there is no old image and the selection icon should be hidden.
+   * @param {!string} oldImageUrl
    * @private
-   * @param {!Event} event
    */
-  onOldImageTap_: function(event) {
-    settings.ChangePicturePrivateApi.selectOldImage();
-    // 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.

   },
 
   /**
-   * Handler for when the user clicks the 'profile' image.
+   * True if the preview image is hidden and the camera controls are shown.
+   * @param {!Element} selectedItem
    * @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
-   * @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';
   },
 });