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

chrome/browser/resources/ntp_search/apps_page.js

Index: chrome/browser/resources/ntp_search/apps_page.js
diff --git a/chrome/browser/resources/ntp_search/apps_page.js b/chrome/browser/resources/ntp_search/apps_page.js
index c24d6b05fce339b53fd605664e9b5af6c147ecca..4370c48b94deb9c74a4b36209fd8c7acfe42f841 100644
--- a/chrome/browser/resources/ntp_search/apps_page.js
+++ b/chrome/browser/resources/ntp_search/apps_page.js
@@ -120,19 +120,19 @@ cr.define('ntp', function() {
     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;
@@ -155,15 +155,15 @@ cr.define('ntp', function() {
           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;
     },
@@ -172,29 +172,25 @@ cr.define('ntp', function() {
       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;
   }
@@ -204,19 +200,18 @@ cr.define('ntp', function() {
 
     /**
      * 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_ ?
@@ -232,22 +227,22 @@ cr.define('ntp', function() {
       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);
 
@@ -266,6 +261,8 @@ cr.define('ntp', function() {
       this.addEventListener('mousedown', this.onMousedown_, true);
       this.addEventListener('keydown', this.onKeydown_);
       this.addEventListener('keyup', this.onKeyup_);
+
+      this.isInitialized_ = true;
     },
 
     /**
@@ -288,15 +285,15 @@ cr.define('ntp', function() {
     },
 
     /**
-     * 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';
       }
 
@@ -359,7 +356,7 @@ cr.define('ntp', function() {
           // 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();
@@ -395,8 +392,8 @@ cr.define('ntp', function() {
      * @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');
 
@@ -484,11 +481,11 @@ cr.define('ntp', function() {
     },
 
     /**
-     * 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();
     },
@@ -497,15 +494,19 @@ cr.define('ntp', function() {
      * 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;
     },
 
     /**
@@ -526,14 +527,14 @@ cr.define('ntp', function() {
      * @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();
@@ -544,8 +545,8 @@ cr.define('ntp', function() {
      * 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);
     },
   });
 
@@ -603,29 +604,30 @@ cr.define('ntp', function() {
 
     /**
      * 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_();
     },
@@ -834,7 +836,7 @@ cr.define('ntp', function() {
   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);
   }