NTP5: Refactoring appData to use Tile's data implementation.

chrome/browser/resources/ntp_search/apps_page.js

 // Copyright (c) 2012 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('ntp', function() {
   'use strict';
 
   var Tile = ntp.Tile;
   var TilePage = ntp.TilePage;
   var APP_LAUNCH = ntp.APP_LAUNCH;
@@ skipping 102 matching lines @@
       }
     },
 
     /**
      * Does all the necessary setup to show the menu for the given app.
      * @param {App} app The App object that will be showing a context menu.
      */
     setupForApp: function(app) {
       this.app_ = app;
 
-      this.launch_.textContent = app.appData.title;
+      this.launch_.textContent = app.data.title;
 
       this.forAllLaunchTypes_(function(launchTypeButton, id) {
         launchTypeButton.disabled = false;
-        launchTypeButton.checked = app.appData.launch_type == id;
+        launchTypeButton.checked = app.data.launch_type == id;
       });
 
-      this.options_.disabled = !app.appData.optionsUrl || !app.appData.enabled;
-      this.details_.disabled = !app.appData.detailsUrl;
-      this.uninstall_.disabled = !app.appData.mayDisable;
+      this.options_.disabled = !app.data.optionsUrl || !app.data.enabled;
+      this.details_.disabled = !app.data.detailsUrl;
+      this.uninstall_.disabled = !app.data.mayDisable;
 
       this.disableNotifications_.hidden = true;
-      var notificationsDisabled = app.appData.notifications_disabled;
+      var notificationsDisabled = app.data.notifications_disabled;
       if (typeof notificationsDisabled != 'undefined') {
         this.disableNotifications_.hidden = false;
         this.disableNotifications_.checked = notificationsDisabled;
       }
     },
 
     /**
      * Handlers for menu item activation.
      * @param {Event} e The activation event.
      * @private
      */
     onLaunch_: function(e) {
       chrome.send('launchApp', [this.app_.appId, APP_LAUNCH.NTP_APPS_MENU]);
     },
     onLaunchTypeChanged_: function(e) {
       var pressed = e.currentTarget;
       var app = this.app_;
       this.forAllLaunchTypes_(function(launchTypeButton, id) {
         if (launchTypeButton == pressed) {
           chrome.send('setLaunchType', [app.appId, id]);
           // Manually update the launch type. We will only get
           // appsPrefChangeCallback calls after changes to other NTP instances.
-          app.appData.launch_type = id;
+          app.data.launch_type = id;
         }
       });
     },
     onShowOptions_: function(e) {
-      window.location = this.app_.appData.optionsUrl;
+      window.location = this.app_.data.optionsUrl;
     },
     onShowDetails_: function(e) {
-      var url = this.app_.appData.detailsUrl;
+      var url = this.app_.data.detailsUrl;
       url = appendParam(url, 'utm_source', 'chrome-ntp-launcher');
       window.location = url;
     },
     onDisableNotifications_: function(e) {
       var app = this.app_;
       app.removeBubble();
       // Toggle the current disable setting.
       var newSetting = !this.disableNotifications_.checked;
-      app.appData.notifications_disabled = newSetting;
-      chrome.send('setNotificationsDisabled', [app.appData.id, newSetting]);
+      app.data.notifications_disabled = newSetting;
+      chrome.send('setNotificationsDisabled', [app.data.id, newSetting]);
     },
     onUninstall_: function(e) {
-      var tileCell = this.app_.tileCell;
-      tileCell.tilePage.setTileRepositioningState(tileCell.index, true);

[Dan Beam, 2012/12/05 18:34:05]

does this no longer apply?

[pedro (no code reviews), 2012/12/05 19:41:34]

No, this was misplaced. This will be called before the user confirmation to
remove the App. The correct place to put this code is inside the delete method.
This is being addressed in the next CL.

-      chrome.send('uninstallApp', [this.app_.appData.id]);
+      chrome.send('uninstallApp', [this.app_.data.id]);
     },
     onCreateShortcut_: function(e) {
-      chrome.send('createAppShortcut', [this.app_.appData.id]);
+      chrome.send('createAppShortcut', [this.app_.data.id]);
     },
   };
 
   /**
    * Creates a new App object.
-   * @param {Object} appData The data object that describes the app.
    * @constructor
    * @extends {HTMLDivElement}
    */
-  function App(appData) {
+  function App() {

[Dan Beam, 2012/12/05 18:34:05]

what's the point of an app with no data?

[pedro (no code reviews), 2012/12/05 19:41:34]

I'm reusing the new Tile/TilePage logic, and the TilePage creates the tile grid
by instantiating empty Tiles (in this case Apps), which will be later filled
with data.

     var el = cr.doc.createElement('div');
     el.__proto__ = App.prototype;
-    el.initialize(appData);
 
     return el;
   }
 
   App.prototype = Tile.subclass({
     __proto__: HTMLDivElement.prototype,
 
     /**
      * Initialize the app object.
-     * @param {Object} appData The data object that describes the app.
+     * @param {Object} data The data object that describes the app.
      */
-    initialize: function(appData) {
+    initialize: function(data) {

[Dan Beam, 2012/12/05 18:34:05]

where do you remember |data| in this.data?

[pedro (no code reviews), 2012/12/05 19:41:34]

set data will store this value and call initialize.

       this.className = 'app focusable';
 
       Tile.prototype.initialize.apply(this, arguments);
 
-      this.appData = appData;

[Dan Beam, 2012/12/05 18:34:05]

^ I'm confused, why did you remove this?

[pedro (no code reviews), 2012/12/05 19:41:34]

The initialize is now being called when the App data is assigned. So, if I keep
this here, it'll end up in a infinite loop.

-      assert(this.appData_.id, 'Got an app without an ID');
-      this.id = this.appData_.id;
+      assert(this.data_.id, 'Got an app without an ID');
+      this.id = this.data_.id;
       this.setAttribute('role', 'menuitem');
 
-      if (!this.appData_.icon_big_exists && this.appData_.icon_small_exists)
+      if (!this.data_.icon_big_exists && this.data_.icon_small_exists)
         this.useSmallIcon_ = true;
 
       this.appContents_ = this.useSmallIcon_ ?
           $('app-small-icon-template').cloneNode(true) :
           $('app-large-icon-template').cloneNode(true);
       this.appContents_.id = '';
       this.appendChild(this.appContents_);
 
       this.appImgContainer_ = this.querySelector('.app-img-container');
       this.appImg_ = this.appImgContainer_.querySelector('img');
       this.setIcon();
 
       if (this.useSmallIcon_) {
         this.imgDiv_ = this.querySelector('.app-icon-div');
         this.addLaunchClickTarget_(this.imgDiv_);
-        this.imgDiv_.title = this.appData_.title;
+        this.imgDiv_.title = this.data_.title;
         chrome.send('getAppIconDominantColor', [this.id]);
       } else {
         this.addLaunchClickTarget_(this.appImgContainer_);
-        this.appImgContainer_.title = this.appData_.title;
+        this.appImgContainer_.title = this.data_.title;
       }
 
       var appSpan = this.appContents_.querySelector('.title');
-      appSpan.textContent = appSpan.title = this.appData_.title;
+      appSpan.textContent = appSpan.title = this.data_.title;
       this.addLaunchClickTarget_(appSpan);
 
-      var notification = this.appData_.notification;
+      var notification = this.data_.notification;
       var hasNotification = typeof notification != 'undefined' &&
                             typeof notification['title'] != 'undefined' &&
                             typeof notification['body'] != 'undefined' &&
-                            !this.appData_.notifications_disabled;
+                            !this.data_.notifications_disabled;
       if (hasNotification)
         this.setupNotification_(notification);
 
       this.addEventListener('keydown', cr.ui.contextMenuHandler);
       this.addEventListener('keyup', cr.ui.contextMenuHandler);
 
       // This hack is here so that appContents.contextMenu will be the same as
       // this.contextMenu.
       var self = this;
       this.appContents_.__defineGetter__('contextMenu', function() {
         return self.contextMenu;
       });
       this.appContents_.addEventListener('contextmenu',
                                          cr.ui.contextMenuHandler);
 
       this.addEventListener('mousedown', this.onMousedown_, true);
       this.addEventListener('keydown', this.onKeydown_);
       this.addEventListener('keyup', this.onKeyup_);
+
+      this.isInitialized_ = true;
     },
 
     /**
      * Sets the color of the favicon dominant color bar.
      * @param {string} color The css-parsable value for the color.
      */
     set stripeColor(color) {
       this.querySelector('.color-stripe').style.backgroundColor = color;
     },
 
     /**
      * Removes the app tile from the page. Should be called after the app has
      * been uninstalled.
      */
     remove: function(opt_animate) {
       // Unset the ID immediately, because the app is already gone. But leave
       // the tile on the page as it animates out.
       this.id = '';
       this.tileCell.doRemove(opt_animate);
     },
 
     /**
-     * Set the URL of the icon from |appData_|. This won't actually show the
+     * Set the URL of the icon from |data_|. This won't actually show the

[Dan Beam, 2012/12/05 18:34:05]

nit: |this.data_|, IMO

[pedro (no code reviews), 2012/12/05 19:41:34]

Done.

      * icon until loadIcon() is called (for performance reasons; we don't want
      * to load icons until we have to).
      */
     setIcon: function() {
-      var src = this.useSmallIcon_ ? this.appData_.icon_small :
-                                     this.appData_.icon_big;
-      if (!this.appData_.enabled ||
-          (!this.appData_.offlineEnabled && !navigator.onLine)) {
+      var src = this.useSmallIcon_ ? this.data_.icon_small :
+                                     this.data_.icon_big;
+      if (!this.data_.enabled ||
+          (!this.data_.offlineEnabled && !navigator.onLine)) {
         src += '?grayscale=true';
       }
 
       this.appImgSrc_ = src;
       this.classList.add('icon-loading');
     },
 
     /**
      * Shows the icon for the app. That is, it causes chrome to load the app
      * icon resource.
@@ skipping 42 matching lines @@
      * @param {Object} notification The notification to show in the bubble.
      * @private
      */
     setupNotification_: function(notification) {
       if (notification) {
         var infoBubble;
         if (!this.currentBubbleShowing_) {
           // Create a new bubble.
           infoBubble = new cr.ui.ExpandableBubble;
           infoBubble.anchorNode = this;
-          infoBubble.appId = this.appData_.id;
+          infoBubble.appId = this.data_.id;
           infoBubble.handleCloseEvent = function() {
             chrome.send('closeNotification', [this.appId]);
             infoBubble.hide();
           };
         } else {
           // Reuse the old bubble instead of popping up a new bubble over
           // the old one.
           infoBubble = this.currentBubbleShowing_;
           infoBubble.collapseBubble_();
         }
@@ skipping 15 matching lines @@
         this.currentBubbleShowing_ = null;
       }
     },
 
     /**
      * Invoked when an app is clicked.
      * @param {Event} e The click event.
      * @private
      */
     onClick_: function(e) {
-      var url = !this.appData_.is_webstore ? '' :
-          appendParam(this.appData_.url,
+      var url = !this.data_.is_webstore ? '' :
+          appendParam(this.data_.url,
                       'utm_source',
                       'chrome-ntp-icon');
 
       chrome.send('launchApp',
                   [this.appId, APP_LAUNCH.NTP_APPS_MAXIMIZED, url,
                    e.button, e.altKey, e.ctrlKey, e.metaKey, e.shiftKey]);
 
       // Don't allow the click to trigger a link or anything
       e.preventDefault();
     },
@@ skipping 67 matching lines @@
       } else {
         this.appContents_.classList.remove('suppress-active');
       }
 
       // This class is here so we don't show the focus state for apps that
       // gain keyboard focus via mouse clicking.
       this.classList.add('click-focus');
     },
 
     /**
-     * Change the appData and update the appearance of the app.
-     * @param {Object} appData The new data object that describes the app.
+     * Change the data and update the appearance of the app.
+     * @param {Object} data The new data object that describes the app.
      */
-    replaceAppData: function(appData) {
-      this.appData_ = appData;
+    replaceAppData: function(data) {

[Dan Beam, 2012/12/05 18:34:05]

what's the point of this when `set data` exists?  also, if this is
"replaceAppData", should there be an assert(this.data_); before anything else to
verify you're actually replacing?

[pedro (no code reviews), 2012/12/05 19:41:34]

I don't know, I'm not the author of this code, I'm just refactoring it! :)

Added assertion.

+      this.data_ = data;
       this.setIcon();
       this.loadIcon();
     },
 
     /**
      * The data and preferences for this app.
      * @type {Object}
      */
-    set appData(data) {
-      this.appData_ = data;
+    set data(data) {
+      Object.getOwnPropertyDescriptor(Tile.prototype, 'data').set.apply(this,
+          arguments);
+
+      if (!this.isInitialized_)
+        this.initialize(data);
     },
-    get appData() {
-      return this.appData_;
+    get data() {
+      return this.data_;
     },
 
     get appId() {
-      return this.appData_.id;
+      return this.data_.id;
     },
 
     /**
      * Returns a pointer to the context menu for this app. All apps share the
      * singleton AppContextMenu. This function is called by the
      * ContextMenuHandler in response to the 'contextmenu' event.
      * @type {cr.ui.Menu}
      */
     get contextMenu() {
       var menu = AppContextMenu.getInstance();
       menu.setupForApp(this);
       return menu.menu;
     },
 
     /**
      * Returns whether this element can be 'removed' from chrome (i.e. whether
      * the user can drag it onto the trash and expect something to happen).
      * @return {boolean} True if the app can be uninstalled.
      */
     canBeRemoved: function() {
-      return this.appData_.mayDisable;
+      return this.data_.mayDisable;
     },
 
     /**
      * Uninstalls the app after it's been dropped on the trash.
      */
     removeFromChrome: function() {
-      chrome.send('uninstallApp', [this.appData_.id, true]);
+      chrome.send('uninstallApp', [this.data_.id, true]);
       this.tile.tilePage.removeTile(this.tile, true);
       if (this.currentBubbleShowing_)
         this.currentBubbleShowing_.hide();
     },
 
     /**
      * Called when a drag is starting on the tile. Updates dataTransfer with
      * data for this tile.
      */
     setDragData: function(dataTransfer) {
-      dataTransfer.setData('Text', this.appData_.title);
-      dataTransfer.setData('URL', this.appData_.url);
+      dataTransfer.setData('Text', this.data_.title);
+      dataTransfer.setData('URL', this.data_.url);
     },
   });
 
   /**
    * Creates a new AppsPage object.
    * @constructor
    * @extends {TilePage}
    */
   function AppsPage() {
     var el = new TilePage();
@@ skipping 37 matching lines @@
       // view.
       this.addEventListener('carddeselected', this.onCardDeselected_);
       this.addEventListener('cardSlider:card_change_ended',
                             this.onCardChangeEnded_);
 
       this.addEventListener('tilePage:tile_added', this.onTileAdded_);
     },
 
     /**
      * Highlight a newly installed app as it's added to the NTP.
-     * @param {Object} appData The data object that describes the app.
+     * @param {Object} data The data object that describes the app.
      */
-    insertAndHighlightApp: function(appData) {
+    insertAndHighlightApp: function(data) {
       ntp.getCardSlider().selectCardByValue(this);
-      this.insertApp(appData, true);
+      this.insertApp(data, true);
     },
 
     /**
      * Inserts an App into the TilePage, preserving the alphabetical order.
-     * @param {Object} appData The data that describes the app.
+     * @param {Object} data The data that describes the app.
      * @param {boolean} animate Whether to animate the insertion.
      */
-    insertApp: function(appData, animate) {
+    insertApp: function(data, animate) {
       var index = this.tiles_.length;
       for (var i = 0; i < this.tiles_.length; i++) {
-        if (appData.title.toLocaleLowerCase() <
-            this.tiles_[i].appData.title.toLocaleLowerCase()) {
+        if (data.title.toLocaleLowerCase() <
+            this.tiles_[i].data.title.toLocaleLowerCase()) {
           index = i;
           break;
         }
       }
 
-      var app = new App(appData);
+      var app = new App();
+      app.data = data;
       this.addTileAt(app, index);
       this.renderGrid_();
     },
 
     /**
      * Handler for 'cardselected' event, fired when |this| is selected. The
      * first time this is called, we load all the app icons.
      * @private
      */
     onCardSelected_: function(e) {
@@ skipping 188 matching lines @@
    * histogram. This should only be invoked from the AppLauncherHandler.
    * @param {String} appID The ID of the app.
    */
   function launchAppAfterEnable(appId) {
     chrome.send('launchApp', [appId, APP_LAUNCH.NTP_APP_RE_ENABLE]);
   }
 
   function appNotificationChanged(id, notification) {
     var app = $(id);
     // The app might have been uninstalled, or notifications might be disabled.
-    if (app && !app.appData.notifications_disabled)
+    if (app && !app.data.notifications_disabled)
       app.setupNotification_(notification);
   }
 
   return {
     appNotificationChanged: appNotificationChanged,
     AppsPage: AppsPage,
     launchAppAfterEnable: launchAppAfterEnable,
   };
 });