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..62de3ced9b27a15c4d13f2a455ba18930fd93da3 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);
-      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() {
     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.

[Dan Beam, 2012/12/07 05:05:31]

nit: can this be @private?

[pedro (no code reviews), 2012/12/07 21:54:55]

All other classes have a public initialize method. I've separated this method
into initialize and formatApps_, which is the private one. This is similar to
what Thumbnail does.

[Dan Beam, 2012/12/07 22:31:07]

but these public initialize methods are never used outside of their respective
classes/files, though, right?  why would an app ever initialize a most visited,
for instance?

[pedro (no code reviews), 2012/12/10 19:44:43]

Done.

      */
-    initialize: function(appData) {
+    initialize: function(data) {
       this.className = 'app focusable';
 
       Tile.prototype.initialize.apply(this, arguments);
 
-      this.appData = appData;
-      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 |this.data_|. This won't actually show the
      * 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,12 @@ 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) {
+      assert(data);
+      this.data_ = data;
       this.setIcon();
       this.loadIcon();
     },
@@ -497,15 +495,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_;

[Dan Beam, 2012/12/07 05:05:31]

is this getter necessary?

[pedro (no code reviews), 2012/12/07 21:54:55]

Yes, it is. It's being used outside this class.

     },
 
     get appId() {
-      return this.appData_.id;
+      return this.data_.id;
     },
 
     /**
@@ -526,14 +528,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 +546,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 +605,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();

[Dan Beam, 2012/12/07 05:05:31]

I'm still confused as to why you wouldn't want to allow an App to take optional
data in it's ctor and say essentially:

  if (opt_data)
    this.initialize(data);

so you're allowed to have both options, as it'd seem natural to have an app (and
kind of unnatural to make an app *without* data).

[pedro (no code reviews), 2012/12/07 21:54:55]

This was part of the new design/refactoring. The constructor of a Tile is being
used to create just the outer HTML element, but the Tile is actually "rendered"
(filled with contents) when the data property is changed. By separating
initialize and formatApps_, we can simply call formatApps_ again when the data
changes, without having to de-construct and re-construct the object.

I could include a check inside the initialization, so if there's data the
formatApps_ would called immediately, then we'd simply write:

var app = new App(data);

The problem is that Tile's constructor requires another parameter, which is a
configuration object, but this object is not necessary anymore since one of last
big TilePage refactoring. I can revisit this in another CL.

[Dan Beam, 2012/12/07 22:31:07]

do that ^

[pedro (no code reviews), 2012/12/10 19:44:43]

Added a TODO. Since this is not exactly related to the animation bug I'm trying
to fix, so if you're okay with it, I'd like to do this in another CL.

+      app.data = data;
       this.addTileAt(app, index);
       this.renderGrid_();
     },
@@ -724,7 +727,7 @@ cr.define('ntp', function() {
       var sourceId = -1;
       var currentlyDraggingTile = ntp.getCurrentlyDraggingTile();
       if (currentlyDraggingTile) {
-        var tileContents = currentlyDraggingTile.firstChild;
+        var tileContents = currentlyDraggingTile.tile;

[Dan Beam, 2012/12/07 05:05:31]

nit: why isn't this getter named `tileContents` -- it looks weird to see
someTile.tile.

[pedro (no code reviews), 2012/12/07 21:54:55]

Because it is indeed a tile. The variable which is misnamed is
currentlyDraggingTile, which was supposed to be currentlyDraggingCell.

But anyway, we've removed all drag and drop behavior, so this code is actually
not necessary anymore.

         if (tileContents.classList.contains('app')) {
           var originalPage = currentlyDraggingTile.tilePage;
           var samePageDrag = originalPage == this;
@@ -797,10 +800,10 @@ cr.define('ntp', function() {
 
     /** @override */
     tileMoved: function(draggedTile) {
-      if (!(draggedTile.firstChild instanceof App))
+      if (!(draggedTile.tile instanceof App))

[Dan Beam, 2012/12/07 05:05:31]

same here, tile.tile

[pedro (no code reviews), 2012/12/07 21:54:55]

Same here too. This method and others related to drag-and-drop have been
removed.

         return;
 
-      chrome.send('setPageIndex', [draggedTile.firstChild.appId, 0]);
+      chrome.send('setPageIndex', [draggedTile.tile.appId, 0]);
 
       var appIds = [];
       for (var i = 0; i < this.tiles_.length; i++) {
@@ -809,7 +812,7 @@ cr.define('ntp', function() {
           appIds.push(tileContents.appId);
       }
 
-      chrome.send('reorderApps', [draggedTile.firstChild.appId, appIds]);
+      chrome.send('reorderApps', [draggedTile.tile.appId, appIds]);
     },
 
     /** @override */
@@ -834,7 +837,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);
   }