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);
+    el.initialize_();
 
     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.
+     * @private
      */
-    initialize: function(appData) {
-      this.className = 'app focusable';
-
+    initialize_: function(data) {
       Tile.prototype.initialize.apply(this, arguments);
 
-      this.appData = appData;
-      assert(this.appData_.id, 'Got an app without an ID');
-      this.id = this.appData_.id;
+      this.classList.add('app');
+      this.classList.add('focusable');
+    },
+
+    /**
+     * Initialize the app object.

[Dan Beam, 2012/12/11 01:49:24]

give a new description highlighting how it's different from initialize_

[pedro (no code reviews), 2012/12/11 07:20:53]

Done.

+     * @param {Object} data The data object that describes the app.
+     * @private
+     */
+    formatApp_: function(data) {
+      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_);
+      if (!this.appContents_) {

[Dan Beam, 2012/12/11 01:49:24]

what happens if you want to re-use apps - does this logic need to change (or
does this change break the ability to do that)?

[pedro (no code reviews), 2012/12/11 07:20:53]

This change does not break the ability to do that. The purpose of this method is
to 'paint' the app with the contents of |data|. It might be called more than
once, so it should create elements just once, otherwise the titles and icons
will duplicate every time formatApp_ is called.

[Dan Beam, 2012/12/11 22:47:23]

this.useSmallIcon_ is based on this.data, so if you change this.data on an
already 'painted' App you'll have a stale this.appContents_, correct?

[pedro (no code reviews), 2012/12/11 23:23:15]

No, that's not entirely correct. If an already 'painted' App changes, let's say
it is updated, and the availability of the Apps's icons has changed (it has now
a big icon which it didn't have before), then the new icon won't be displayed
properly which is a bug and I'll file a bug and address it in another CL. But
the appContents will remain valid, and the new image will be redrawn (it just
won't use the bigger image). But please note that all App icons now have a fixed
size, so no matter if it is a 64px or a 128px, it will be presented as a 70x70
pixel image.

+        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() {
@@ skipping 20 matching lines @@
      * 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);
+
+      this.formatApp_(data);

[Dan Beam, 2012/12/11 01:49:24]

what is the difference between formatApp_, replaceAppData, and `set data`?

[pedro (no code reviews), 2012/12/11 07:20:53]

'set data' is the setter of, well, data. When that happens, the app needs to be
formatted/painted by formatApp. replaceAppData is the legacy code.

     },
-    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);
-    },
   });
 
   /**
    * Creates a new AppsPage object.
    * @constructor
    * @extends {TilePage}
    */
   function AppsPage() {
     var el = new TilePage();
     el.__proto__ = AppsPage.prototype;
@@ skipping 36 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);
+      // TODO(pedrosimonetti): Refactor code so instantiation is new App(data).

[Dan Beam, 2012/12/11 01:49:24]

why TODO() when you could just do?

[pedro (no code reviews), 2012/12/11 07:20:53]

Because not having to do this would've made this CL to be submitted faster, not
blocking other CLs. Meanwhile I would've been able to address this issue and
possibly polish up a few more things, cleaning up code, etc.

I just did it.

+      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 51 matching lines @@
       TilePage.prototype.onScroll.apply(this, arguments);
 
       for (var i = 0; i < this.tiles_.length; i++) {
         var app = this.tiles_[i];
         assert(app instanceof App);
         if (app.currentBubbleShowing_)
           app.currentBubbleShowing_.resizeAndReposition();
         }
     },
 
-    /** @override */
-    doDragOver: function(e) {
-      // Only animatedly re-arrange if the user is currently dragging an app.
-      var tile = ntp.getCurrentlyDraggingTile();
-      if (tile && tile.querySelector('.app')) {
-        TilePage.prototype.doDragOver.call(this, e);
-      } else {
-        e.preventDefault();
-        this.setDropEffect(e.dataTransfer);
-      }
-    },
-
-    /** @override */
-    shouldAcceptDrag: function(e) {
-      if (ntp.getCurrentlyDraggingTile())
-        return true;
-      if (!e.dataTransfer || !e.dataTransfer.types)
-        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;
-        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);
-            this.fireAddedEvent(currentlyDraggingTile, index, true);
-          }
-        } else if (currentlyDraggingTile.querySelector('.most-visited')) {
-          this.generateAppForLink(tileContents.data);
-          sourceId = DRAG_SOURCE.MOST_VISITED_PANE;
-        }
-      } else {
-        this.addOutsideData_(dataTransfer);
-        sourceId = DRAG_SOURCE.OUTSIDE_NTP;
-      }
-
-      assert(sourceId != -1);
-      chrome.send('metricsHandler:recordInHistogram',
-          ['NewTabPage.AppsPageDragSource', sourceId, DRAG_SOURCE_LIMIT]);
-    },
-
-    /**
-     * Adds drag data that has been dropped from a source that is not a tile.
-     * @param {Object} dataTransfer The data transfer object that holds drop
-     *     data.
-     * @private
-     */
-    addOutsideData_: function(dataTransfer) {
-      var url = dataTransfer.getData('url');
-      assert(url);
-
-      // If the dataTransfer has html data, use that html's text contents as the
-      // title of the new link.
-      var html = dataTransfer.getData('text/html');
-      var title;
-      if (html) {
-        // It's important that we don't attach this node to the document
-        // because it might contain scripts.
-        var node = this.ownerDocument.createElement('div');
-        node.innerHTML = html;
-        title = node.textContent;
-      }
-
-      // Make sure title is >=1 and <=45 characters for Chrome app limits.
-      if (!title)
-        title = url;
-      if (title.length > 45)
-        title = title.substring(0, 45);
-      var data = {url: url, title: title};
-
-      // Synthesize an app.
-      this.generateAppForLink(data);
-    },
-
     /**
      * Creates a new crx-less app manifest and installs it.
      * @param {Object} data The data object describing the link. Must have |url|
      *     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))
-        return;
-
-      chrome.send('setPageIndex', [draggedTile.firstChild.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]);
-    },
-
-    /** @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,
   };
 });