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

chrome/browser/resources/ntp_search/most_visited_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 Thumbnail = ntp.Thumbnail;
   var ThumbnailPage = ntp.ThumbnailPage;
 
@@ skipping 38 matching lines @@
       var closeButton = cr.doc.createElement('div');
       closeButton.className = 'close-button';
       closeButton.title = loadTimeData.getString('removethumbnailtooltip');
       this.appendChild(closeButton);
     },
 
     /**
      * Update the appearance of this tile according to |data|.
      * @param {Object} data A dictionary of relevant data for the page.
      */
-    setData: function(data) {
+    set data(data) {
+      Object.getOwnPropertyDescriptor(Thumbnail.prototype, 'data').set.apply(
+          this, arguments);
+
       if (this.classList.contains('blacklisted') && data) {
         // Animate appearance of new tile.
         this.classList.add('new-tile-contents');
       }
       this.classList.remove('blacklisted');
-
-      Thumbnail.prototype.setData.apply(this, arguments);
+    },
+    get data() {
+      return this.data_;

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

nit: what does this offer over Thumbnail.data / do you need it?

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

Yes, I do need this. I might be missing something here, but if remove the getter
here and place it in the superclass (Thumbnail or even Tile), it won't work.

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

in JS, "private" members exist on all sub classes as it's actually just a proto
chain reference.

  function base(){}
  base.prototype = {/** @private */ private_: 2};

  function derived(){}
  derived.prototype = {
    __proto__: base.prototype,
    getPublic: function(){ return this.private_; },
  };

  alert(new derived().getPublic()); // alerts 2

and if you really want this it work like classical inheritance, you could just
as easily add an accessor:

  function base(){}
  base.prototype = {
    /** @private */ private_: 2,
    /** @protected */ getProtected: function() { return this.private_; },
  };

  function derived(){}
  derived.prototype = {
    __proto__: base.prototype,
    getPublic: function(){ return this.getProtected(); },
  };

or:

  function base(){}
  base.prototype = {/** @protected */ protected: 2};

  function derived(){}
  derived.prototype = {
    __proto__: base.prototype,
    getPublic: function(){ return this.protected; },
  };

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

I guess I wasn't clear enough in my previous comment. I do know how OO works in
JS, the prototype chain, the lack of real private/protected members, using
closures to emulate private members, etc.

I was talking about how getters/setters behave in the prototype chain. It seems
that if you define a setter on a subclass you must also define a getter,
otherwise if you call the getter from this subclass it will return undefined
(and the expected would be to return the superclass getter).

Check this example:
http://jsfiddle.net/qycMs/

     },
 
     /**
      * Handles a click on the tile.
      * @param {Event} e The click event.
      * @private
      */
     handleClick_: function(e) {
       if (e.target.classList.contains('close-button')) {
         this.blacklist_();
@@ skipping 149 matching lines @@
       chrome.send('mostVisitedSelected');
     }
   };
 
   return {
     MostVisitedPage: MostVisitedPage,
   };
 });
 
 document.addEventListener('ntpLoaded', ntp.MostVisitedPage.onLoaded);