Settings People Revamp: Implement Change People preview pane

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 21 matching lines @@
     /**
      * The currently selected profile image URL. May be a data URL.
      * @private {string}
      */
     selectedImageUrl_: {
       type: String,
       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.
+     * @type {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.
+     * @type {string}
+     */
+    cameraImageUrl: {

[dpapad, 2016/01/21 18:11:40]

Does this URL need to be public (all other URLs in this file are private)?

[tommycli, 2016/01/21 18:47:05]

Done. Actually I think not.

The eventual goal is to have an embedded element. I think this can still be
private.

<camera-image camera-image-url="{{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.
+     * @type {string}

[dpapad, 2016/01/21 18:11:40]

@private or remove the underscore from the name.

[tommycli, 2016/01/21 18:47:05]

Done.

+     */
+    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}
      */
     oldImageUrl_: String,
 
     /**
      * The url of the profile image.
      * @private {string}
      */
@@ skipping 22 matching lines @@
        */
       receiveDefaultImages: function(images) {
         this.defaultImages_ = images;
       }.bind(this),
 
       /**
        * Called from C++ to provide the URL of the selected image.
        * @param {string} imageUrl
        */
       receiveSelectedImage: function(imageUrl) {
+        this.cameraActive = false;

[dpapad, 2016/01/21 18:11:40]

Would it make sense to set the value of this boolean in an observer of the
selectedImageUrl_ property instead of repeating it three times?

[tommycli, 2016/01/21 18:47:05]

Hmm.. I'm not sure yet.

Basically I'm still not sure how to reconcile selectedImageUrl_ with
cameraActive_ yet. The two seem a little redundant. I think this will become
clear in the next patchset, so would it be alright if I left it as-is for now,
but came to a decision next patch?

[dpapad, 2016/01/21 18:55:47]

SG.

         this.selectedImageUrl_ = imageUrl;
       }.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;
       }.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)
+        if (selected) {
+          this.cameraActive = false;
           this.selectedImageUrl_ = imageUrl;
+        }
       }.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.
+   * @private
+   * @param {!Event} event

[dpapad, 2016/01/21 18:55:47]

We are not using the event parameter. Can it be removed?

[tommycli, 2016/01/21 19:08:03]

Done.

+   */
+  onCameraImageTap_: function(event) {
+    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;
@@ skipping 29 matching lines @@
     event.preventDefault();
   },
 
   /**
    * Computed binding determining which profile image button is toggled on.
    * @private
    * @param {string} imageUrl
    * @param {string} selectedImageUrl
    * @return {boolean}
    */
-  isActiveImage_: function(imageUrl, selectedImageUrl) {
-    return imageUrl == selectedImageUrl;
+  isActiveImage_: function(cameraActive, imageUrl, selectedImageUrl) {
+    return !cameraActive && (imageUrl == selectedImageUrl);
   },
 });