Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11438009:
NTP5: Refactoring appData to use Tile's data implementation. (Closed)
|
Created:
8 years ago by pedro (no code reviews) Modified:
8 years ago CC:
chromium-reviews, dbeam+watch-ntp_chromium.org, estade+watch_chromium.org, arv (Not doing code reviews), pedrosimonetti+watch_chromium.org, David Black Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionNTP5: Refactoring appData to use Tile's data implementation and removing drag and drop behavior from Apps (which is not being used in NTP5).
This is a preparation CL for:
https://codereview.chromium.org/11445009/
BUG=164079
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=172551
Patch Set 1 #
Total comments: 16
Patch Set 2 : Addressing Dan's comments #
Total comments: 22
Patch Set 3 : Addressing Dan's comments #Patch Set 4 : Addressing Dan's comments #
Total comments: 13
Patch Set 5 : Addressing Dan's comments #Patch Set 6 : Oops #Patch Set 7 : NTP5: Refactoring appData to use Tile's data implementation #
Messages
Total messages: 21 (0 generated)
Evan and Dan, this is the first part of the animation fix for Apps.
https://chromiumcodereview.appspot.com/11438009/diff/1/chrome/browser/resourc... File chrome/browser/resources/ntp_search/apps_page.js (left): https://chromiumcodereview.appspot.com/11438009/diff/1/chrome/browser/resourc... chrome/browser/resources/ntp_search/apps_page.js:180: tileCell.tilePage.setTileRepositioningState(tileCell.index, true); does this no longer apply? https://chromiumcodereview.appspot.com/11438009/diff/1/chrome/browser/resourc... chrome/browser/resources/ntp_search/apps_page.js:214: this.appData = appData; ^ I'm confused, why did you remove this? https://chromiumcodereview.appspot.com/11438009/diff/1/chrome/browser/resourc... File chrome/browser/resources/ntp_search/apps_page.js (right): https://chromiumcodereview.appspot.com/11438009/diff/1/chrome/browser/resourc... chrome/browser/resources/ntp_search/apps_page.js:191: function App() { what's the point of an app with no data? https://chromiumcodereview.appspot.com/11438009/diff/1/chrome/browser/resourc... chrome/browser/resources/ntp_search/apps_page.js:205: initialize: function(data) { where do you remember |data| in this.data? https://chromiumcodereview.appspot.com/11438009/diff/1/chrome/browser/resourc... chrome/browser/resources/ntp_search/apps_page.js:288: * Set the URL of the icon from |data_|. This won't actually show the nit: |this.data_|, IMO https://chromiumcodereview.appspot.com/11438009/diff/1/chrome/browser/resourc... chrome/browser/resources/ntp_search/apps_page.js:487: replaceAppData: function(data) { 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? https://chromiumcodereview.appspot.com/11438009/diff/1/chrome/browser/resourc... File chrome/browser/resources/ntp_search/thumbnail_page.js (right): https://chromiumcodereview.appspot.com/11438009/diff/1/chrome/browser/resourc... chrome/browser/resources/ntp_search/thumbnail_page.js:64: }, I'm not totally sure, but you might need a `get data` as well - as long as it works for you... https://chromiumcodereview.appspot.com/11438009/diff/1/chrome/browser/resourc... File chrome/browser/resources/ntp_search/tile_page.js (right): https://chromiumcodereview.appspot.com/11438009/diff/1/chrome/browser/resourc... chrome/browser/resources/ntp_search/tile_page.js:188: this.tilePage.removeTile(this.firstChild, false); nit: you may want firstElementChild (which ignores text and comments)
https://codereview.chromium.org/11438009/diff/1/chrome/browser/resources/ntp_... File chrome/browser/resources/ntp_search/apps_page.js (left): https://codereview.chromium.org/11438009/diff/1/chrome/browser/resources/ntp_... chrome/browser/resources/ntp_search/apps_page.js:180: tileCell.tilePage.setTileRepositioningState(tileCell.index, true); On 2012/12/05 18:34:05, Dan Beam wrote: > does this no longer apply? 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. https://codereview.chromium.org/11438009/diff/1/chrome/browser/resources/ntp_... chrome/browser/resources/ntp_search/apps_page.js:214: this.appData = appData; On 2012/12/05 18:34:05, Dan Beam wrote: > ^ I'm confused, why did you remove this? 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. https://codereview.chromium.org/11438009/diff/1/chrome/browser/resources/ntp_... File chrome/browser/resources/ntp_search/apps_page.js (right): https://codereview.chromium.org/11438009/diff/1/chrome/browser/resources/ntp_... chrome/browser/resources/ntp_search/apps_page.js:191: function App() { On 2012/12/05 18:34:05, Dan Beam wrote: > what's the point of an app with no data? 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. https://codereview.chromium.org/11438009/diff/1/chrome/browser/resources/ntp_... chrome/browser/resources/ntp_search/apps_page.js:205: initialize: function(data) { On 2012/12/05 18:34:05, Dan Beam wrote: > where do you remember |data| in this.data? set data will store this value and call initialize. https://codereview.chromium.org/11438009/diff/1/chrome/browser/resources/ntp_... chrome/browser/resources/ntp_search/apps_page.js:288: * Set the URL of the icon from |data_|. This won't actually show the On 2012/12/05 18:34:05, Dan Beam wrote: > nit: |this.data_|, IMO Done. https://codereview.chromium.org/11438009/diff/1/chrome/browser/resources/ntp_... chrome/browser/resources/ntp_search/apps_page.js:487: replaceAppData: function(data) { On 2012/12/05 18:34:05, Dan Beam wrote: > 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? I don't know, I'm not the author of this code, I'm just refactoring it! :) Added assertion. https://codereview.chromium.org/11438009/diff/1/chrome/browser/resources/ntp_... File chrome/browser/resources/ntp_search/thumbnail_page.js (right): https://codereview.chromium.org/11438009/diff/1/chrome/browser/resources/ntp_... chrome/browser/resources/ntp_search/thumbnail_page.js:64: }, On 2012/12/05 18:34:05, Dan Beam wrote: > I'm not totally sure, but you might need a `get data` as well - as long as it > works for you... Thumbnail page is basically a virtual class, and will never be instantiated, that's why I did not include the getter here. The subclass (MostVisisted) has the getter though. https://codereview.chromium.org/11438009/diff/1/chrome/browser/resources/ntp_... File chrome/browser/resources/ntp_search/tile_page.js (right): https://codereview.chromium.org/11438009/diff/1/chrome/browser/resources/ntp_... chrome/browser/resources/ntp_search/tile_page.js:188: this.tilePage.removeTile(this.firstChild, false); On 2012/12/05 18:34:05, Dan Beam wrote: > nit: you may want firstElementChild (which ignores text and comments) The tile cell and the tile (its child) are being created by the code, so there's no risk of having a comment or text node in there. But I don't like using firstChild because it is not clear that we're dealing with the tile. So I've created a new getter 'tile' which returns the firstElementChild.
Friendly ping.
https://codereview.chromium.org/11438009/diff/5001/chrome/browser/resources/n... File chrome/browser/resources/ntp_search/apps_page.js (right): https://codereview.chromium.org/11438009/diff/5001/chrome/browser/resources/n... chrome/browser/resources/ntp_search/apps_page.js:199: __proto__: HTMLDivElement.prototype, 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? https://codereview.chromium.org/11438009/diff/5001/chrome/browser/resources/n... chrome/browser/resources/ntp_search/apps_page.js:203: * @param {Object} data The data object that describes the app. nit: can this be @private? https://codereview.chromium.org/11438009/diff/5001/chrome/browser/resources/n... chrome/browser/resources/ntp_search/apps_page.js:506: return this.data_; is this getter necessary? https://codereview.chromium.org/11438009/diff/5001/chrome/browser/resources/n... chrome/browser/resources/ntp_search/apps_page.js:630: var app = new App(); 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). https://codereview.chromium.org/11438009/diff/5001/chrome/browser/resources/n... chrome/browser/resources/ntp_search/apps_page.js:730: var tileContents = currentlyDraggingTile.tile; nit: why isn't this getter named `tileContents` -- it looks weird to see someTile.tile. https://codereview.chromium.org/11438009/diff/5001/chrome/browser/resources/n... chrome/browser/resources/ntp_search/apps_page.js:803: if (!(draggedTile.tile instanceof App)) same here, tile.tile https://codereview.chromium.org/11438009/diff/5001/chrome/browser/resources/n... File chrome/browser/resources/ntp_search/most_visited_page.js (right): https://codereview.chromium.org/11438009/diff/5001/chrome/browser/resources/n... chrome/browser/resources/ntp_search/most_visited_page.js:70: return this.data_; nit: what does this offer over Thumbnail.data / do you need it? https://codereview.chromium.org/11438009/diff/5001/chrome/browser/resources/n... File chrome/browser/resources/ntp_search/tile_page.js (right): https://codereview.chromium.org/11438009/diff/5001/chrome/browser/resources/n... chrome/browser/resources/ntp_search/tile_page.js:166: * @type {Tile} nit: you should assert(this.firstElementChild) and make this @type {!Tile} if you're sure it'll never be null...
I've addressed Dan's comments and removed a couple of unused variables. https://codereview.chromium.org/11438009/diff/5001/chrome/browser/resources/n... File chrome/browser/resources/ntp_search/apps_page.js (right): https://codereview.chromium.org/11438009/diff/5001/chrome/browser/resources/n... chrome/browser/resources/ntp_search/apps_page.js:199: __proto__: HTMLDivElement.prototype, On 2012/12/07 05:05:31, Dan Beam wrote: > 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? 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. https://codereview.chromium.org/11438009/diff/5001/chrome/browser/resources/n... chrome/browser/resources/ntp_search/apps_page.js:203: * @param {Object} data The data object that describes the app. On 2012/12/07 05:05:31, Dan Beam wrote: > nit: can this be @private? 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. https://codereview.chromium.org/11438009/diff/5001/chrome/browser/resources/n... chrome/browser/resources/ntp_search/apps_page.js:506: return this.data_; On 2012/12/07 05:05:31, Dan Beam wrote: > is this getter necessary? Yes, it is. It's being used outside this class. https://codereview.chromium.org/11438009/diff/5001/chrome/browser/resources/n... chrome/browser/resources/ntp_search/apps_page.js:630: var app = new App(); On 2012/12/07 05:05:31, Dan Beam wrote: > 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). 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. https://codereview.chromium.org/11438009/diff/5001/chrome/browser/resources/n... chrome/browser/resources/ntp_search/apps_page.js:730: var tileContents = currentlyDraggingTile.tile; On 2012/12/07 05:05:31, Dan Beam wrote: > nit: why isn't this getter named `tileContents` -- it looks weird to see > someTile.tile. 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. https://codereview.chromium.org/11438009/diff/5001/chrome/browser/resources/n... chrome/browser/resources/ntp_search/apps_page.js:803: if (!(draggedTile.tile instanceof App)) On 2012/12/07 05:05:31, Dan Beam wrote: > same here, tile.tile Same here too. This method and others related to drag-and-drop have been removed. https://codereview.chromium.org/11438009/diff/5001/chrome/browser/resources/n... File chrome/browser/resources/ntp_search/most_visited_page.js (right): https://codereview.chromium.org/11438009/diff/5001/chrome/browser/resources/n... chrome/browser/resources/ntp_search/most_visited_page.js:70: return this.data_; On 2012/12/07 05:05:31, Dan Beam wrote: > nit: what does this offer over Thumbnail.data / do you need it? 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. https://codereview.chromium.org/11438009/diff/5001/chrome/browser/resources/n... File chrome/browser/resources/ntp_search/tile_page.js (right): https://codereview.chromium.org/11438009/diff/5001/chrome/browser/resources/n... chrome/browser/resources/ntp_search/tile_page.js:166: * @type {Tile} On 2012/12/07 05:05:31, Dan Beam wrote: > nit: you should assert(this.firstElementChild) and make this @type {!Tile} if > you're sure it'll never be null... We cannot do that because sometimes we use |if (cell.tile)| in order to check whether the cell is filled.
https://codereview.chromium.org/11438009/diff/5001/chrome/browser/resources/n... File chrome/browser/resources/ntp_search/apps_page.js (right): https://codereview.chromium.org/11438009/diff/5001/chrome/browser/resources/n... chrome/browser/resources/ntp_search/apps_page.js:203: * @param {Object} data The data object that describes the app. On 2012/12/07 21:54:55, pedrosimonetti wrote: > On 2012/12/07 05:05:31, Dan Beam wrote: > > nit: can this be @private? > > 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. 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? https://codereview.chromium.org/11438009/diff/5001/chrome/browser/resources/n... chrome/browser/resources/ntp_search/apps_page.js:630: var app = new App(); On 2012/12/07 21:54:55, pedrosimonetti wrote: > On 2012/12/07 05:05:31, Dan Beam wrote: > > 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). > > 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); do that ^ > > 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. https://codereview.chromium.org/11438009/diff/5001/chrome/browser/resources/n... File chrome/browser/resources/ntp_search/most_visited_page.js (right): https://codereview.chromium.org/11438009/diff/5001/chrome/browser/resources/n... chrome/browser/resources/ntp_search/most_visited_page.js:70: return this.data_; On 2012/12/07 21:54:55, pedrosimonetti wrote: > On 2012/12/07 05:05:31, Dan Beam wrote: > > nit: what does this offer over Thumbnail.data / do you need it? > > 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. 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; }, };
https://codereview.chromium.org/11438009/diff/5001/chrome/browser/resources/n... File chrome/browser/resources/ntp_search/apps_page.js (right): https://codereview.chromium.org/11438009/diff/5001/chrome/browser/resources/n... chrome/browser/resources/ntp_search/apps_page.js:203: * @param {Object} data The data object that describes the app. On 2012/12/07 22:31:07, Dan Beam wrote: > On 2012/12/07 21:54:55, pedrosimonetti wrote: > > On 2012/12/07 05:05:31, Dan Beam wrote: > > > nit: can this be @private? > > > > 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. > > 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? Done. https://codereview.chromium.org/11438009/diff/5001/chrome/browser/resources/n... chrome/browser/resources/ntp_search/apps_page.js:630: var app = new App(); On 2012/12/07 22:31:07, Dan Beam wrote: > On 2012/12/07 21:54:55, pedrosimonetti wrote: > > On 2012/12/07 05:05:31, Dan Beam wrote: > > > 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). > > > > 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); > > do that ^ > > > > > 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. 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. https://codereview.chromium.org/11438009/diff/5001/chrome/browser/resources/n... File chrome/browser/resources/ntp_search/most_visited_page.js (right): https://codereview.chromium.org/11438009/diff/5001/chrome/browser/resources/n... chrome/browser/resources/ntp_search/most_visited_page.js:70: return this.data_; On 2012/12/07 22:31:07, Dan Beam wrote: > On 2012/12/07 21:54:55, pedrosimonetti wrote: > > On 2012/12/07 05:05:31, Dan Beam wrote: > > > nit: what does this offer over Thumbnail.data / do you need it? > > > > 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. > > 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; }, > }; 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/
please update the description highlighting that you're also removing d'n'd from NTP5 in this CL https://codereview.chromium.org/11438009/diff/13001/chrome/browser/resources/... File chrome/browser/resources/ntp_search/apps_page.js (right): https://codereview.chromium.org/11438009/diff/13001/chrome/browser/resources/... chrome/browser/resources/ntp_search/apps_page.js:215: * Initialize the app object. give a new description highlighting how it's different from initialize_ https://codereview.chromium.org/11438009/diff/13001/chrome/browser/resources/... chrome/browser/resources/ntp_search/apps_page.js:227: if (!this.appContents_) { 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)? https://codereview.chromium.org/11438009/diff/13001/chrome/browser/resources/... chrome/browser/resources/ntp_search/apps_page.js:512: this.formatApp_(data); what is the difference between formatApp_, replaceAppData, and `set data`? https://codereview.chromium.org/11438009/diff/13001/chrome/browser/resources/... chrome/browser/resources/ntp_search/apps_page.js:630: // TODO(pedrosimonetti): Refactor code so instantiation is new App(data). why TODO() when you could just do? https://codereview.chromium.org/11438009/diff/13001/chrome/browser/resources/... File chrome/browser/resources/ntp_search/tile_page.js (right): https://codereview.chromium.org/11438009/diff/13001/chrome/browser/resources/... chrome/browser/resources/ntp_search/tile_page.js:92: }, why don't you put `get data` here so you don't have to replicate it other places?
https://codereview.chromium.org/11438009/diff/13001/chrome/browser/resources/... File chrome/browser/resources/ntp_search/apps_page.js (right): https://codereview.chromium.org/11438009/diff/13001/chrome/browser/resources/... chrome/browser/resources/ntp_search/apps_page.js:215: * Initialize the app object. On 2012/12/11 01:49:24, Dan Beam wrote: > give a new description highlighting how it's different from initialize_ Done. https://codereview.chromium.org/11438009/diff/13001/chrome/browser/resources/... chrome/browser/resources/ntp_search/apps_page.js:227: if (!this.appContents_) { On 2012/12/11 01:49:24, Dan Beam wrote: > 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)? 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. https://codereview.chromium.org/11438009/diff/13001/chrome/browser/resources/... chrome/browser/resources/ntp_search/apps_page.js:512: this.formatApp_(data); On 2012/12/11 01:49:24, Dan Beam wrote: > what is the difference between formatApp_, replaceAppData, and `set data`? '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. https://codereview.chromium.org/11438009/diff/13001/chrome/browser/resources/... chrome/browser/resources/ntp_search/apps_page.js:630: // TODO(pedrosimonetti): Refactor code so instantiation is new App(data). On 2012/12/11 01:49:24, Dan Beam wrote: > why TODO() when you could just do? 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. https://codereview.chromium.org/11438009/diff/13001/chrome/browser/resources/... File chrome/browser/resources/ntp_search/tile_page.js (right): https://codereview.chromium.org/11438009/diff/13001/chrome/browser/resources/... chrome/browser/resources/ntp_search/tile_page.js:92: }, On 2012/12/11 01:49:24, Dan Beam wrote: > why don't you put `get data` here so you don't have to replicate it other > places? As I explained earlier, that is not possible: http://jsfiddle.net/qycMs/ Included getter here too in case a Tile subclass does not implement a new setter (and getter, once it is required).
I realized that I missed to remove one comment from apps_page.js and that setting a getter in Tile.prototype won't make any difference once the prototype of the Tile's subclass is created using Tile.subclass, so I just removed the getter I just had added. https://codereview.chromium.org/11438009/diff/13001/chrome/browser/resources/... File chrome/browser/resources/ntp_search/tile_page.js (right): https://codereview.chromium.org/11438009/diff/13001/chrome/browser/resources/... chrome/browser/resources/ntp_search/tile_page.js:92: }, On 2012/12/11 07:20:53, pedrosimonetti wrote: > On 2012/12/11 01:49:24, Dan Beam wrote: > > why don't you put `get data` here so you don't have to replicate it other > > places? > > As I explained earlier, that is not possible: http://jsfiddle.net/qycMs/ > > Included getter here too in case a Tile subclass does not implement a new setter > (and getter, once it is required). Actually, I cannot include this getter here because it won't be enumerated, therefore the Tile.subclass function won't work. Just removed the getter.
Friendly ping.
https://codereview.chromium.org/11438009/diff/13001/chrome/browser/resources/... File chrome/browser/resources/ntp_search/apps_page.js (right): https://codereview.chromium.org/11438009/diff/13001/chrome/browser/resources/... chrome/browser/resources/ntp_search/apps_page.js:227: if (!this.appContents_) { On 2012/12/11 07:20:53, pedrosimonetti wrote: > On 2012/12/11 01:49:24, Dan Beam wrote: > > 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)? > > 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. 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?
https://codereview.chromium.org/11438009/diff/13001/chrome/browser/resources/... File chrome/browser/resources/ntp_search/apps_page.js (right): https://codereview.chromium.org/11438009/diff/13001/chrome/browser/resources/... chrome/browser/resources/ntp_search/apps_page.js:227: if (!this.appContents_) { On 2012/12/11 22:47:23, Dan Beam wrote: > On 2012/12/11 07:20:53, pedrosimonetti wrote: > > On 2012/12/11 01:49:24, Dan Beam wrote: > > > 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)? > > > > 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. > > 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? 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.
lgtm
Thanks for the approval! I've filed crbug.com/165612 and will address it soon in another CL. I've also included a comment pointing to the bug in the proper place in the code.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/pedrosimonetti@chromium.org/11438009/2...
Retried try job too often on linux_chromeos for step(s) aura_unittests, base_unittests, browser_tests, cacheinvalidation_unittests, check_deps, chromeos_unittests, content_browsertests, content_unittests, crypto_unittests, dbus_unittests, device_unittests, gpu_unittests, interactive_ui_tests, ipc_tests, jingle_unittests, media_unittests, net_unittests, ppapi_unittests, printing_unittests, sandbox_linux_unittests, sql_unittests, sync_unit_tests, unit_tests
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/pedrosimonetti@chromium.org/11438009/2...
Message was sent while issue was closed.
Change committed as 172551 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
