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);
-      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;
   }
 
   App.prototype = Tile.subclass({
     __proto__: HTMLDivElement.prototype,

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

nit: why isn't this simply Tile.prototype or Thumbnail.prototype (I don't know
the difference)?  Also, why does `Tile.subclass` need to exist?

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

Thumbnail is a subclass of Tile. App is also a subclass of Tile.

NTP4 uses this "peculiar" way of subclassing native HTML elements (using
createElement and __proto__ madness), which kinda breaks the prototype chain,
that is, there is no way to do the pure prototype-based inheritance like Apps
--> Tile --> SomeHTMLElement, if you consider that different Tile
implementations might wanna use different HTML elements.

When refactoring the TilePage code from NTP4 a while ago, I realized that the
TilePage was assuming a few things from the "tile's content" object, but those
things were not reinforced by the code (it was up to the subclass to reimplement
those or else it would fail). To avoid reimplementing the same thing (few
getters and setters) over and over again, and in order to be less prone to
errors, I introduced the TileCell and Tile classes.

That's why Tile.subclass exists.

 
     /**
      * 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_ ?
           $('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 |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';
       }
 
       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) {
+      assert(data);
+      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_;

[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;
     },
 
     /**
      * 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();

[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_();
     },
 
     /**
      * 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 78 matching lines @@
         return false;
       return Array.prototype.indexOf.call(e.dataTransfer.types,
                                           'text/uri-list') != -1;
     },
 
     /** @override */
     addDragData: function(dataTransfer, index) {
       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;
           sourceId = samePageDrag ? DRAG_SOURCE.SAME_APPS_PANE :
                                     DRAG_SOURCE.OTHER_APPS_PANE;
           this.tileGrid_.insertBefore(currentlyDraggingTile,
                                       this.tileElements_[index]);
           this.tileMoved(currentlyDraggingTile);
           if (!samePageDrag) {
             originalPage.fireRemovedEvent(currentlyDraggingTile, index, true);
@@ skipping 52 matching lines @@
      *     and |title| members.
      */
     generateAppForLink: function(data) {
       assert(data.url != undefined);
       assert(data.title != undefined);
       chrome.send('generateAppForLink', [data.url, data.title, 0]);
     },
 
     /** @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++) {
         var tileContents = this.tiles_[i];
         if (tileContents instanceof App)
           appIds.push(tileContents.appId);
       }
 
-      chrome.send('reorderApps', [draggedTile.firstChild.appId, appIds]);
+      chrome.send('reorderApps', [draggedTile.tile.appId, appIds]);
     },
 
     /** @override */
     setDropEffect: function(dataTransfer) {
       var tile = ntp.getCurrentlyDraggingTile();
       if (tile && tile.querySelector('.app'))
         ntp.setCurrentDropEffect(dataTransfer, 'move');
       else
         ntp.setCurrentDropEffect(dataTransfer, 'copy');
     },
   };
 
   /**
    * Launches the specified app using the APP_LAUNCH_NTP_APP_RE_ENABLE
    * 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,
   };
 });