Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10829131:
Refactoring NTP5: new implementation of TilePage and MostVisitedPage (which now inherits from Thumb… (Closed)
|
Created:
8 years, 4 months ago by pedrosimonetti2 Modified:
8 years, 4 months ago CC:
chromium-reviews, arv (Not doing code reviews) Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionRefactoring NTP5: new implementation of TilePage and MostVisitedPage (which now inherits from ThumbnailPage).
BUG=142669
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=152540
Patch Set 1 #
Total comments: 88
Patch Set 2 : Implemented changes suggested by Jeremy #
Total comments: 35
Patch Set 3 : #Patch Set 4 : Implemented changes suggested by Jeremy #Patch Set 5 : Fixing error during initialization #
Total comments: 2
Patch Set 6 : Removing a trailing whitespace #
Total comments: 84
Patch Set 7 : Addressing Evan's comments #
Total comments: 75
Patch Set 8 : Addressing Evan's comments #2 #
Total comments: 48
Patch Set 9 : Addressing Evan's comments #3 #
Total comments: 18
Patch Set 10 : Addressing Evan's comments #4 #
Total comments: 26
Patch Set 11 : Addressing Evan's comments #5 #
Total comments: 24
Patch Set 12 : Addressing Evan's comments #6 #
Total comments: 8
Patch Set 13 : Addressing Evan's comments #7 #
Total comments: 14
Patch Set 14 : Addressing Evan's comments #8 #Patch Set 15 : Fixing mock.js to support renaming mock png files to jpg. #Patch Set 16 : Removing binary files. #Patch Set 17 : Renaming mockData.js to mock_data_autogen.js #Patch Set 18 : Manually fixing mock_data_autogen.js lines with more than 80 chars. #Messages
Total messages: 43 (0 generated)
Another pass. I'll take a closer look at this file, but wanted to send some comments for you to look at in the meantime. https://chromiumcodereview.appspot.com/10829131/diff/1/chrome/browser/resourc... File chrome/browser/resources/ntp_search/new_tab.js (left): https://chromiumcodereview.appspot.com/10829131/diff/1/chrome/browser/resourc... chrome/browser/resources/ntp_search/new_tab.js:90: getRequiredElement('trash'), Add a TODO for removing these non-1993 features? https://chromiumcodereview.appspot.com/10829131/diff/1/chrome/browser/resourc... File chrome/browser/resources/ntp_search/new_tab.js (right): https://chromiumcodereview.appspot.com/10829131/diff/1/chrome/browser/resourc... chrome/browser/resources/ntp_search/new_tab.js:596: getThumbnailUrl: getThumbnailUrl, alphabetize https://chromiumcodereview.appspot.com/10829131/diff/1/chrome/browser/resourc... File chrome/browser/resources/ntp_search/thumbnail_page.js (right): https://chromiumcodereview.appspot.com/10829131/diff/1/chrome/browser/resourc... chrome/browser/resources/ntp_search/thumbnail_page.js:43: get index() { Can this be deleted? It's almost identical to 'index' in TileGetters (tile_page.js). https://chromiumcodereview.appspot.com/10829131/diff/1/chrome/browser/resourc... File chrome/browser/resources/ntp_search/tile_page.js (right): https://chromiumcodereview.appspot.com/10829131/diff/1/chrome/browser/resourc... chrome/browser/resources/ntp_search/tile_page.js:92: function TileCell(tile, gridValues) { Since we're planning to remove drag functionality, is it possible to propagate these styles to Tile and get rid of this object? **** Ah it looks like this is also used for 'filler' tiles. But maybe those can be implemented as another tile subclass? https://chromiumcodereview.appspot.com/10829131/diff/1/chrome/browser/resourc... chrome/browser/resources/ntp_search/tile_page.js:105: // TODO: rename to tile-cell Delete? https://chromiumcodereview.appspot.com/10829131/diff/1/chrome/browser/resourc... chrome/browser/resources/ntp_search/tile_page.js:113: style.marginLeft = gridValues.tileHorMargin + 'px'; i88n? https://chromiumcodereview.appspot.com/10829131/diff/1/chrome/browser/resourc... chrome/browser/resources/ntp_search/tile_page.js:369: * @param {Object} gridValues Pixel values that define the size and layout Delete https://chromiumcodereview.appspot.com/10829131/diff/1/chrome/browser/resourc... chrome/browser/resources/ntp_search/tile_page.js:396: reinforceStyles: true, Is there ever a case when this would be false? https://chromiumcodereview.appspot.com/10829131/diff/1/chrome/browser/resourc... chrome/browser/resources/ntp_search/tile_page.js:399: slowFactor: 1, Unused? https://chromiumcodereview.appspot.com/10829131/diff/1/chrome/browser/resourc... chrome/browser/resources/ntp_search/tile_page.js:428: //this.tileElements_ = this.tileGrid_.getElementsByClassName('tile real'); Any reason to keep lines like these? https://chromiumcodereview.appspot.com/10829131/diff/1/chrome/browser/resourc... chrome/browser/resources/ntp_search/tile_page.js:489: this.content_.insertBefore(node, this.topMargin_.nextElementSibling); This line is throwing JS errors, since the code for creating this.topMargin_ has been deleted. What's the plan for notifications in 1993? https://chromiumcodereview.appspot.com/10829131/diff/1/chrome/browser/resourc... chrome/browser/resources/ntp_search/tile_page.js:1099: appendTile: function(tile) { Can this be factored into addTileAt? https://chromiumcodereview.appspot.com/10829131/diff/1/chrome/browser/resourc... chrome/browser/resources/ntp_search/tile_page.js:1137: if (windowWidth >= 948) { Should these numbers be made parameters? https://chromiumcodereview.appspot.com/10829131/diff/1/chrome/browser/resourc... chrome/browser/resources/ntp_search/tile_page.js:1157: renderGrid_: function(colCount, tileElements) { Please provide brief function-level comment for non-trivial functions. https://chromiumcodereview.appspot.com/10829131/diff/1/chrome/browser/resourc... chrome/browser/resources/ntp_search/tile_page.js:1404: //this.tileGridContent_.classList.remove('animate-tile') This class never gets removed, intentional? https://chromiumcodereview.appspot.com/10829131/diff/1/chrome/browser/resourc... chrome/browser/resources/ntp_search/tile_page.js:1409: onKeyDown_: function(e) { Should this be onKeyUp instead (if the user holds down the arrow button)? https://chromiumcodereview.appspot.com/10829131/diff/1/chrome/browser/resourc... chrome/browser/resources/ntp_search/tile_page.js:1462: function changeSlowFactor(el) { Unused? https://chromiumcodereview.appspot.com/10829131/diff/1/chrome/browser/resourc... chrome/browser/resources/ntp_search/tile_page.js:1467: function changeDebugMode(el) { Unused?
Another round of comments. Apologies if some of these are redundant from the other CL. https://chromiumcodereview.appspot.com/10829131/diff/1/chrome/browser/resourc... File chrome/browser/resources/ntp_search/mock/mock.js (right): https://chromiumcodereview.appspot.com/10829131/diff/1/chrome/browser/resourc... chrome/browser/resources/ntp_search/mock/mock.js:92: if (index) { if (index != -1) https://chromiumcodereview.appspot.com/10829131/diff/1/chrome/browser/resourc... chrome/browser/resources/ntp_search/mock/mock.js:94: mockUrl = thumbnailUrlList.splice(index, 0); This doesn't remove anything. splice(index, 1) instead? https://chromiumcodereview.appspot.com/10829131/diff/1/chrome/browser/resourc... chrome/browser/resources/ntp_search/mock/mock.js:187: Extra newline. https://chromiumcodereview.appspot.com/10829131/diff/1/chrome/browser/resourc... File chrome/browser/resources/ntp_search/mock/mockData.js (right): https://chromiumcodereview.appspot.com/10829131/diff/1/chrome/browser/resourc... chrome/browser/resources/ntp_search/mock/mockData.js:53: "url": "http://www.youtube.com/" Drop the www (everywhere in getRecentlyClosedTabs) to match mockedThumbnailUrls. https://chromiumcodereview.appspot.com/10829131/diff/1/chrome/browser/resourc... chrome/browser/resources/ntp_search/mock/mockData.js:194: Extra newline. https://chromiumcodereview.appspot.com/10829131/diff/1/chrome/browser/resourc... File chrome/browser/resources/ntp_search/most_visited_page.js (right): https://chromiumcodereview.appspot.com/10829131/diff/1/chrome/browser/resourc... chrome/browser/resources/ntp_search/most_visited_page.js:16: function MostVisited() { Pass in gridValues? Also, this doesn't seem to get called. https://chromiumcodereview.appspot.com/10829131/diff/1/chrome/browser/resourc... File chrome/browser/resources/ntp_search/new_tab.css (right): https://chromiumcodereview.appspot.com/10829131/diff/1/chrome/browser/resourc... chrome/browser/resources/ntp_search/new_tab.css:315: .hide-col-0 .tile-col-0, Are these column indices necessary? https://chromiumcodereview.appspot.com/10829131/diff/1/chrome/browser/resourc... File chrome/browser/resources/ntp_search/thumbnail_page.css (right): https://chromiumcodereview.appspot.com/10829131/diff/1/chrome/browser/resourc... chrome/browser/resources/ntp_search/thumbnail_page.css:5: .close-button { Since close-button is most-visited specific, rename file? https://chromiumcodereview.appspot.com/10829131/diff/1/chrome/browser/resourc... File chrome/browser/resources/ntp_search/thumbnail_page.js (right): https://chromiumcodereview.appspot.com/10829131/diff/1/chrome/browser/resourc... chrome/browser/resources/ntp_search/thumbnail_page.js:17: * Creates a new Most Visited object for tiling. most visited (please make another pass cleaning this up) https://chromiumcodereview.appspot.com/10829131/diff/1/chrome/browser/resourc... chrome/browser/resources/ntp_search/thumbnail_page.js:18: * @constructor @param https://chromiumcodereview.appspot.com/10829131/diff/1/chrome/browser/resourc... chrome/browser/resources/ntp_search/thumbnail_page.js:158: chrome.send('mostVisitedAction', This is MostVisited specific. Should handleClick_ be virtual? https://chromiumcodereview.appspot.com/10829131/diff/1/chrome/browser/resourc... chrome/browser/resources/ntp_search/thumbnail_page.js:164: * Allow blacklisting most visited site using the keyboard. Move to MostVisited? https://chromiumcodereview.appspot.com/10829131/diff/1/chrome/browser/resourc... chrome/browser/resources/ntp_search/thumbnail_page.js:285: this.classList.add('most-visited-page'); most-visited https://chromiumcodereview.appspot.com/10829131/diff/1/chrome/browser/resourc... chrome/browser/resources/ntp_search/thumbnail_page.js:352: set data(data) { As we discussed, make virtual? https://chromiumcodereview.appspot.com/10829131/diff/1/chrome/browser/resourc... File chrome/browser/resources/ntp_search/tile_page.js (right): https://chromiumcodereview.appspot.com/10829131/diff/1/chrome/browser/resourc... chrome/browser/resources/ntp_search/tile_page.js:428: //this.tileElements_ = this.tileGrid_.getElementsByClassName('tile real'); On 2012/08/02 00:19:11, jeremycho wrote: > Any reason to keep lines like these? As we discussed, these will be deleted in a future change. https://chromiumcodereview.appspot.com/10829131/diff/1/chrome/browser/resourc... chrome/browser/resources/ntp_search/tile_page.js:1094: animatingColCount: 5, // TODO initialize Delete TODO? https://chromiumcodereview.appspot.com/10829131/diff/1/chrome/browser/resourc... chrome/browser/resources/ntp_search/tile_page.js:1112: getNumOfVisibleRows: function() { getNumOfVisibleRows_ https://chromiumcodereview.appspot.com/10829131/diff/1/chrome/browser/resourc... chrome/browser/resources/ntp_search/tile_page.js:1157: renderGrid_: function(colCount, tileElements) { Never called with tileElements? https://chromiumcodereview.appspot.com/10829131/diff/1/chrome/browser/resourc... chrome/browser/resources/ntp_search/tile_page.js:1180: // Get the current tile row. I think you can drop comments like these if it's fairly obvious from the code. https://chromiumcodereview.appspot.com/10829131/diff/1/chrome/browser/resourc... chrome/browser/resources/ntp_search/tile_page.js:1191: var rowVisible = (row >= pageOffset && Make the hide-row add/remove a helper function and call it from paginate_ too? https://chromiumcodereview.appspot.com/10829131/diff/1/chrome/browser/resourc... chrome/browser/resources/ntp_search/tile_page.js:1206: Delete newline https://chromiumcodereview.appspot.com/10829131/diff/1/chrome/browser/resourc... chrome/browser/resources/ntp_search/tile_page.js:1221: if (tileCell.firstChild) { Factor out duplicate code here and in the else? https://chromiumcodereview.appspot.com/10829131/diff/1/chrome/browser/resourc... chrome/browser/resources/ntp_search/tile_page.js:1229: // TODO create filler and method to Please clarify comment. https://chromiumcodereview.appspot.com/10829131/diff/1/chrome/browser/resourc... chrome/browser/resources/ntp_search/tile_page.js:1241: tileRow.appendChild(tileCell); Move to the L1209 else? https://chromiumcodereview.appspot.com/10829131/diff/1/chrome/browser/resourc... chrome/browser/resources/ntp_search/tile_page.js:1304: setTimeout((function(animatingColCount) { Comment why this is necessary? https://chromiumcodereview.appspot.com/10829131/diff/1/chrome/browser/resourc... chrome/browser/resources/ntp_search/tile_page.js:1364: paginate: function(pageOffset, force) { paginate_
Jeremy, I have implemented the changes you suggested. Please take another look. http://codereview.chromium.org/10829131/diff/1/chrome/browser/resources/ntp_s... File chrome/browser/resources/ntp_search/mock/mock.js (right): http://codereview.chromium.org/10829131/diff/1/chrome/browser/resources/ntp_s... chrome/browser/resources/ntp_search/mock/mock.js:92: if (index) { On 2012/08/02 03:00:49, jeremycho wrote: > if (index != -1) Done. http://codereview.chromium.org/10829131/diff/1/chrome/browser/resources/ntp_s... chrome/browser/resources/ntp_search/mock/mock.js:94: mockUrl = thumbnailUrlList.splice(index, 0); You're right. The bug wasn't visible for me because the fallback for cases when there's no mock image (thumbnailUrlList.shift()) was solving the problem. The code is fixed now. On 2012/08/02 03:00:49, jeremycho wrote: > This doesn't remove anything. splice(index, 1) instead? http://codereview.chromium.org/10829131/diff/1/chrome/browser/resources/ntp_s... chrome/browser/resources/ntp_search/mock/mock.js:187: On 2012/08/02 03:00:49, jeremycho wrote: > Extra newline. Done. http://codereview.chromium.org/10829131/diff/1/chrome/browser/resources/ntp_s... File chrome/browser/resources/ntp_search/mock/mockData.js (right): http://codereview.chromium.org/10829131/diff/1/chrome/browser/resources/ntp_s... chrome/browser/resources/ntp_search/mock/mockData.js:53: "url": "http://www.youtube.com/" This file is generated by chrome.mock() function, so we are not supposed to change this file manually. We might tweak some things like adding more mock data for things we were not covering (like we did for RecentlyClosed), but the url value here is the exact one that once was recorded during a call to getRecentlyClosedTabs. I'll document more things later how to use chrome.mock(). On 2012/08/02 03:00:49, jeremycho wrote: > Drop the www (everywhere in getRecentlyClosedTabs) to match mockedThumbnailUrls. http://codereview.chromium.org/10829131/diff/1/chrome/browser/resources/ntp_s... chrome/browser/resources/ntp_search/mock/mockData.js:194: On 2012/08/02 03:00:49, jeremycho wrote: > Extra newline. Done. http://codereview.chromium.org/10829131/diff/1/chrome/browser/resources/ntp_s... File chrome/browser/resources/ntp_search/most_visited_page.js (right): http://codereview.chromium.org/10829131/diff/1/chrome/browser/resources/ntp_s... chrome/browser/resources/ntp_search/most_visited_page.js:16: function MostVisited() { Yes, we need to pass gridValues. This wasn't being called because I forgot to set the MostVisitedPage.prototype.ThumbnailClass property to the MostVisited class. It's fixed now. On 2012/08/02 03:00:49, jeremycho wrote: > Pass in gridValues? Also, this doesn't seem to get called. http://codereview.chromium.org/10829131/diff/1/chrome/browser/resources/ntp_s... File chrome/browser/resources/ntp_search/new_tab.css (right): http://codereview.chromium.org/10829131/diff/1/chrome/browser/resources/ntp_s... chrome/browser/resources/ntp_search/new_tab.css:315: .hide-col-0 .tile-col-0, Yes, they animate entire columns of Tiles at once. I included a comment clarifying the purpose of this rule. On 2012/08/02 03:00:49, jeremycho wrote: > Are these column indices necessary? http://codereview.chromium.org/10829131/diff/1/chrome/browser/resources/ntp_s... File chrome/browser/resources/ntp_search/new_tab.js (left): http://codereview.chromium.org/10829131/diff/1/chrome/browser/resources/ntp_s... chrome/browser/resources/ntp_search/new_tab.js:90: getRequiredElement('trash'), On 2012/08/02 00:19:11, jeremycho wrote: > Add a TODO for removing these non-1993 features? Done. http://codereview.chromium.org/10829131/diff/1/chrome/browser/resources/ntp_s... File chrome/browser/resources/ntp_search/new_tab.js (right): http://codereview.chromium.org/10829131/diff/1/chrome/browser/resources/ntp_s... chrome/browser/resources/ntp_search/new_tab.js:596: getThumbnailUrl: getThumbnailUrl, On 2012/08/02 00:19:11, jeremycho wrote: > alphabetize Done. http://codereview.chromium.org/10829131/diff/1/chrome/browser/resources/ntp_s... File chrome/browser/resources/ntp_search/thumbnail_page.css (right): http://codereview.chromium.org/10829131/diff/1/chrome/browser/resources/ntp_s... chrome/browser/resources/ntp_search/thumbnail_page.css:5: .close-button { I refactored the CSS and classNames in the code. close-button related rules are in most_visited_page.css. Other rules here renamed to thumbnail. On 2012/08/02 03:00:49, jeremycho wrote: > Since close-button is most-visited specific, rename file? http://codereview.chromium.org/10829131/diff/1/chrome/browser/resources/ntp_s... File chrome/browser/resources/ntp_search/thumbnail_page.js (right): http://codereview.chromium.org/10829131/diff/1/chrome/browser/resources/ntp_s... chrome/browser/resources/ntp_search/thumbnail_page.js:17: * Creates a new Most Visited object for tiling. On 2012/08/02 03:00:49, jeremycho wrote: > most visited (please make another pass cleaning this up) Done. http://codereview.chromium.org/10829131/diff/1/chrome/browser/resources/ntp_s... chrome/browser/resources/ntp_search/thumbnail_page.js:18: * @constructor On 2012/08/02 03:00:49, jeremycho wrote: > @param Done. http://codereview.chromium.org/10829131/diff/1/chrome/browser/resources/ntp_s... chrome/browser/resources/ntp_search/thumbnail_page.js:43: get index() { On 2012/08/02 00:19:11, jeremycho wrote: > Can this be deleted? It's almost identical to 'index' in TileGetters > (tile_page.js). Done. http://codereview.chromium.org/10829131/diff/1/chrome/browser/resources/ntp_s... chrome/browser/resources/ntp_search/thumbnail_page.js:158: chrome.send('mostVisitedAction', handleClick_ is not implemented in the Thumbnail class anymore. On 2012/08/02 03:00:49, jeremycho wrote: > This is MostVisited specific. Should handleClick_ be virtual? http://codereview.chromium.org/10829131/diff/1/chrome/browser/resources/ntp_s... chrome/browser/resources/ntp_search/thumbnail_page.js:164: * Allow blacklisting most visited site using the keyboard. On 2012/08/02 03:00:49, jeremycho wrote: > Move to MostVisited? Done. http://codereview.chromium.org/10829131/diff/1/chrome/browser/resources/ntp_s... chrome/browser/resources/ntp_search/thumbnail_page.js:285: this.classList.add('most-visited-page'); On 2012/08/02 03:00:49, jeremycho wrote: > most-visited I renamed it to 'thumbnail-page' and overrided the MostVisited.prototype.initialize to include the 'most-visited' class too. http://codereview.chromium.org/10829131/diff/1/chrome/browser/resources/ntp_s... chrome/browser/resources/ntp_search/thumbnail_page.js:352: set data(data) { On 2012/08/02 03:00:49, jeremycho wrote: > As we discussed, make virtual? Done. http://codereview.chromium.org/10829131/diff/1/chrome/browser/resources/ntp_s... File chrome/browser/resources/ntp_search/tile_page.js (right): http://codereview.chromium.org/10829131/diff/1/chrome/browser/resources/ntp_s... chrome/browser/resources/ntp_search/tile_page.js:92: function TileCell(tile, gridValues) { No, we need TileCell class because it will handle the positioning and animation of a Tile. I think creating a new class just for the filler is over engineering. I see the filler as a TileCell without a Tile. On 2012/08/02 00:19:11, jeremycho wrote: > Since we're planning to remove drag functionality, is it possible to propagate > these styles to Tile and get rid of this object? > > **** > > Ah it looks like this is also used for 'filler' tiles. But maybe those can be > implemented as another tile subclass? http://codereview.chromium.org/10829131/diff/1/chrome/browser/resources/ntp_s... chrome/browser/resources/ntp_search/tile_page.js:105: // TODO: rename to tile-cell On 2012/08/02 00:19:11, jeremycho wrote: > Delete? Done. http://codereview.chromium.org/10829131/diff/1/chrome/browser/resources/ntp_s... chrome/browser/resources/ntp_search/tile_page.js:113: style.marginLeft = gridValues.tileHorMargin + 'px'; What do you mean here? On 2012/08/02 00:19:11, jeremycho wrote: > i88n? http://codereview.chromium.org/10829131/diff/1/chrome/browser/resources/ntp_s... chrome/browser/resources/ntp_search/tile_page.js:369: * @param {Object} gridValues Pixel values that define the size and layout On 2012/08/02 00:19:11, jeremycho wrote: > Delete Done. http://codereview.chromium.org/10829131/diff/1/chrome/browser/resources/ntp_s... chrome/browser/resources/ntp_search/tile_page.js:396: reinforceStyles: true, Yes, if you want to set the TileCell and Tile sizes with CSS, then you can make this property false. I'll consider removing this property though. On 2012/08/02 00:19:11, jeremycho wrote: > Is there ever a case when this would be false? http://codereview.chromium.org/10829131/diff/1/chrome/browser/resources/ntp_s... chrome/browser/resources/ntp_search/tile_page.js:399: slowFactor: 1, Just for now. I need to add the debug features back when I finish to clean up the code. On 2012/08/02 00:19:11, jeremycho wrote: > Unused? http://codereview.chromium.org/10829131/diff/1/chrome/browser/resources/ntp_s... chrome/browser/resources/ntp_search/tile_page.js:428: //this.tileElements_ = this.tileGrid_.getElementsByClassName('tile real'); I kept that code commented out as a reminder to review the use of the tileElements_ property. On 2012/08/02 00:19:11, jeremycho wrote: > Any reason to keep lines like these? http://codereview.chromium.org/10829131/diff/1/chrome/browser/resources/ntp_s... chrome/browser/resources/ntp_search/tile_page.js:428: //this.tileElements_ = this.tileGrid_.getElementsByClassName('tile real'); I'll delete this code in a future CL. On 2012/08/02 03:00:49, jeremycho wrote: > On 2012/08/02 00:19:11, jeremycho wrote: > > Any reason to keep lines like these? > > As we discussed, these will be deleted in a future change. http://codereview.chromium.org/10829131/diff/1/chrome/browser/resources/ntp_s... chrome/browser/resources/ntp_search/tile_page.js:489: this.content_.insertBefore(node, this.topMargin_.nextElementSibling); Fixed. The notification is working now, even though it is a little bit buggy. I'll address this in another CL. The designers are working on a specification for the notifications. On 2012/08/02 00:19:11, jeremycho wrote: > This line is throwing JS errors, since the code for creating this.topMargin_ has > been deleted. What's the plan for notifications in 1993? http://codereview.chromium.org/10829131/diff/1/chrome/browser/resources/ntp_s... chrome/browser/resources/ntp_search/tile_page.js:1094: animatingColCount: 5, // TODO initialize No, I clarified what to do. On 2012/08/02 03:00:49, jeremycho wrote: > Delete TODO? http://codereview.chromium.org/10829131/diff/1/chrome/browser/resources/ntp_s... chrome/browser/resources/ntp_search/tile_page.js:1099: appendTile: function(tile) { The addTileAt() implementation was wrong, and I fixed it. They are slightly different and I think push is faster than splice so I would keep two different implementations like they are now. On 2012/08/02 00:19:11, jeremycho wrote: > Can this be factored into addTileAt? http://codereview.chromium.org/10829131/diff/1/chrome/browser/resources/ntp_s... chrome/browser/resources/ntp_search/tile_page.js:1112: getNumOfVisibleRows: function() { On 2012/08/02 03:00:49, jeremycho wrote: > getNumOfVisibleRows_ Done. http://codereview.chromium.org/10829131/diff/1/chrome/browser/resources/ntp_s... chrome/browser/resources/ntp_search/tile_page.js:1137: if (windowWidth >= 948) { Maybe, added a TODO. This will depend on the WebView implementation so I'm not sure what to do with this function right now. On 2012/08/02 00:19:11, jeremycho wrote: > Should these numbers be made parameters? http://codereview.chromium.org/10829131/diff/1/chrome/browser/resources/ntp_s... chrome/browser/resources/ntp_search/tile_page.js:1157: renderGrid_: function(colCount, tileElements) { Maybe I'll have to change this function soon, so I'll include more comments later. On 2012/08/02 00:19:11, jeremycho wrote: > Please provide brief function-level comment for non-trivial functions. http://codereview.chromium.org/10829131/diff/1/chrome/browser/resources/ntp_s... chrome/browser/resources/ntp_search/tile_page.js:1157: renderGrid_: function(colCount, tileElements) { It was being called with tileElements in the old prototype. Deleted this parameter now. On 2012/08/02 03:00:49, jeremycho wrote: > Never called with tileElements? http://codereview.chromium.org/10829131/diff/1/chrome/browser/resources/ntp_s... chrome/browser/resources/ntp_search/tile_page.js:1180: // Get the current tile row. On 2012/08/02 03:00:49, jeremycho wrote: > I think you can drop comments like these if it's fairly obvious from the code. Done. http://codereview.chromium.org/10829131/diff/1/chrome/browser/resources/ntp_s... chrome/browser/resources/ntp_search/tile_page.js:1191: var rowVisible = (row >= pageOffset && On 2012/08/02 03:00:49, jeremycho wrote: > Make the hide-row add/remove a helper function and call it from paginate_ too? Done. http://codereview.chromium.org/10829131/diff/1/chrome/browser/resources/ntp_s... chrome/browser/resources/ntp_search/tile_page.js:1206: On 2012/08/02 03:00:49, jeremycho wrote: > Delete newline Done. http://codereview.chromium.org/10829131/diff/1/chrome/browser/resources/ntp_s... chrome/browser/resources/ntp_search/tile_page.js:1221: if (tileCell.firstChild) { They are slightly different so I don't think we can refactor them. I need to change the tile rendering logic a little bit so I'll see if there's any room for improvement. On 2012/08/02 03:00:49, jeremycho wrote: > Factor out duplicate code here and in the else? http://codereview.chromium.org/10829131/diff/1/chrome/browser/resources/ntp_s... chrome/browser/resources/ntp_search/tile_page.js:1229: // TODO create filler and method to On 2012/08/02 03:00:49, jeremycho wrote: > Please clarify comment. Done. http://codereview.chromium.org/10829131/diff/1/chrome/browser/resources/ntp_s... chrome/browser/resources/ntp_search/tile_page.js:1241: tileRow.appendChild(tileCell); No, because when we are creating new TileCells, we tweak the cell "offline" first then afterwards we append to the document, avoid extra reflows. On 2012/08/02 03:00:49, jeremycho wrote: > Move to the L1209 else? http://codereview.chromium.org/10829131/diff/1/chrome/browser/resources/ntp_s... chrome/browser/resources/ntp_search/tile_page.js:1304: setTimeout((function(animatingColCount) { I was having a problem with this code a while ago, but I'm not entirely sure if we need this anymore. I need to fix the slowdown debug function in order to test this so I'll do it later. On 2012/08/02 03:00:49, jeremycho wrote: > Comment why this is necessary? http://codereview.chromium.org/10829131/diff/1/chrome/browser/resources/ntp_s... chrome/browser/resources/ntp_search/tile_page.js:1364: paginate: function(pageOffset, force) { On 2012/08/02 03:00:49, jeremycho wrote: > paginate_ Done. http://codereview.chromium.org/10829131/diff/1/chrome/browser/resources/ntp_s... chrome/browser/resources/ntp_search/tile_page.js:1404: //this.tileGridContent_.classList.remove('animate-tile') We need to refactor this piece of code later. On 2012/08/02 00:19:11, jeremycho wrote: > This class never gets removed, intentional? http://codereview.chromium.org/10829131/diff/1/chrome/browser/resources/ntp_s... chrome/browser/resources/ntp_search/tile_page.js:1409: onKeyDown_: function(e) { On 2012/08/02 00:19:11, jeremycho wrote: > Should this be onKeyUp instead (if the user holds down the arrow button)? Done. http://codereview.chromium.org/10829131/diff/1/chrome/browser/resources/ntp_s... chrome/browser/resources/ntp_search/tile_page.js:1462: function changeSlowFactor(el) { I'll add the debugging features later. On 2012/08/02 00:19:11, jeremycho wrote: > Unused? http://codereview.chromium.org/10829131/diff/1/chrome/browser/resources/ntp_s... chrome/browser/resources/ntp_search/tile_page.js:1467: function changeDebugMode(el) { I'll add the debugging features later. On 2012/08/02 00:19:11, jeremycho wrote: > Unused?
Thanks for addressing my comments. I'm seeing some issues with the blacklist notification: * Blacklisting a thumbnail doesn't seem to actually remove it. * If I blacklist two thumbnails, the hover titles disappear from all thumbnails. * If I blacklist a thumbnail, the notification dismissal X overlaps the first thumbnail. When I click it, a grey border appears around the most visited page. http://codereview.chromium.org/10829131/diff/12001/chrome/browser/resources/n... File chrome/browser/resources/ntp_search/mock/mock.js (right): http://codereview.chromium.org/10829131/diff/12001/chrome/browser/resources/n... chrome/browser/resources/ntp_search/mock/mock.js:28: "http---youtube.com-", Add www.youtube.com, etc. for the mock recently closed data? http://codereview.chromium.org/10829131/diff/12001/chrome/browser/resources/n... chrome/browser/resources/ntp_search/mock/mock.js:121: delete newline http://codereview.chromium.org/10829131/diff/12001/chrome/browser/resources/n... chrome/browser/resources/ntp_search/mock/mock.js:146: delete newline http://codereview.chromium.org/10829131/diff/12001/chrome/browser/resources/n... chrome/browser/resources/ntp_search/mock/mock.js:166: delete newline http://codereview.chromium.org/10829131/diff/12001/chrome/browser/resources/n... File chrome/browser/resources/ntp_search/most_visited_page.js (right): http://codereview.chromium.org/10829131/diff/12001/chrome/browser/resources/n... chrome/browser/resources/ntp_search/most_visited_page.js:13: * @constructor @param http://codereview.chromium.org/10829131/diff/12001/chrome/browser/resources/n... chrome/browser/resources/ntp_search/most_visited_page.js:151: this.createTiles_(this.data_.length); Any reason why this line was moved? http://codereview.chromium.org/10829131/diff/12001/chrome/browser/resources/n... File chrome/browser/resources/ntp_search/new_tab.html (right): http://codereview.chromium.org/10829131/diff/12001/chrome/browser/resources/n... chrome/browser/resources/ntp_search/new_tab.html:22: <!-- --> Delete? http://codereview.chromium.org/10829131/diff/12001/chrome/browser/resources/n... File chrome/browser/resources/ntp_search/new_tab.js (right): http://codereview.chromium.org/10829131/diff/12001/chrome/browser/resources/n... chrome/browser/resources/ntp_search/new_tab.js:90: // TODO(xci): remove the Trash component. and page switcher? http://codereview.chromium.org/10829131/diff/12001/chrome/browser/resources/n... File chrome/browser/resources/ntp_search/thumbnail_page.js (right): http://codereview.chromium.org/10829131/diff/12001/chrome/browser/resources/n... chrome/browser/resources/ntp_search/thumbnail_page.js:51: this.className = 'tile-content most-visited real'; Delete? It gets overridden in the next line. http://codereview.chromium.org/10829131/diff/12001/chrome/browser/resources/n... chrome/browser/resources/ntp_search/thumbnail_page.js:170: // If you want to use CSS to do that, you cant set this value to false. cant->can http://codereview.chromium.org/10829131/diff/12001/chrome/browser/resources/n... chrome/browser/resources/ntp_search/thumbnail_page.js:191: * @private @param http://codereview.chromium.org/10829131/diff/12001/chrome/browser/resources/n... File chrome/browser/resources/ntp_search/tile_page.js (right): http://codereview.chromium.org/10829131/diff/12001/chrome/browser/resources/n... chrome/browser/resources/ntp_search/tile_page.js:107: * animate with CSS to take advantage of hardware acceleration. @param http://codereview.chromium.org/10829131/diff/12001/chrome/browser/resources/n... chrome/browser/resources/ntp_search/tile_page.js:132: style.marginLeft = gridValues.tileHorMargin + 'px'; Should this be marginRight in RTL? http://codereview.chromium.org/10829131/diff/12001/chrome/browser/resources/n... chrome/browser/resources/ntp_search/tile_page.js:630: handleCardSelection_: function(e) { Now this only seems to get called when switching to Most Visited, not Apps? http://codereview.chromium.org/10829131/diff/12001/chrome/browser/resources/n... chrome/browser/resources/ntp_search/tile_page.js:1353: newline http://codereview.chromium.org/10829131/diff/12001/chrome/browser/resources/n... chrome/browser/resources/ntp_search/tile_page.js:1450: newline
We discussed offline that the notification issues I mentioned only happen with the mock data. http://codereview.chromium.org/10829131/diff/12001/chrome/browser/resources/n... File chrome/browser/resources/ntp_search/new_tab.js (right): http://codereview.chromium.org/10829131/diff/12001/chrome/browser/resources/n... chrome/browser/resources/ntp_search/new_tab.js:196: newTabView.cardSlider.currentCardValue.layout_(); On Linux Chrome, this is throwing a JS error each time I open a new tab: [27432:27432:164792166683:ERROR:CONSOLE(13797)] "Uncaught TypeError: Object #<HTMLDivElement> has no method 'layout_'", source: chrome://newtab/ (13797)
Jeremy, I have finished implementing the changes you suggested. Please ignore the Patch Set #3. http://codereview.chromium.org/10829131/diff/12001/chrome/browser/resources/n... File chrome/browser/resources/ntp_search/mock/mock.js (right): http://codereview.chromium.org/10829131/diff/12001/chrome/browser/resources/n... chrome/browser/resources/ntp_search/mock/mock.js:28: "http---youtube.com-", The logic behind mocked thumbnails is not that simple because we have to space special characters and the image file name should be identical to the values presented here. For this reason, I'm removing the 'www' from the getRecentlyClosed mock data. On 2012/08/03 20:51:24, jeremycho wrote: > Add http://www.youtube.com, etc. for the mock recently closed data? http://codereview.chromium.org/10829131/diff/12001/chrome/browser/resources/n... chrome/browser/resources/ntp_search/mock/mock.js:121: On 2012/08/03 20:51:24, jeremycho wrote: > delete newline Done. http://codereview.chromium.org/10829131/diff/12001/chrome/browser/resources/n... chrome/browser/resources/ntp_search/mock/mock.js:146: On 2012/08/03 20:51:24, jeremycho wrote: > delete newline Done. http://codereview.chromium.org/10829131/diff/12001/chrome/browser/resources/n... chrome/browser/resources/ntp_search/mock/mock.js:166: On 2012/08/03 20:51:24, jeremycho wrote: > delete newline Done. http://codereview.chromium.org/10829131/diff/12001/chrome/browser/resources/n... File chrome/browser/resources/ntp_search/most_visited_page.js (right): http://codereview.chromium.org/10829131/diff/12001/chrome/browser/resources/n... chrome/browser/resources/ntp_search/most_visited_page.js:13: * @constructor On 2012/08/03 20:51:24, jeremycho wrote: > @param Done. http://codereview.chromium.org/10829131/diff/12001/chrome/browser/resources/n... chrome/browser/resources/ntp_search/most_visited_page.js:151: this.createTiles_(this.data_.length); I forgot to update my comment but I end up fixing the problem that was showing up 8 thumbnais regardless of how many data we had. The problem was that createTiles_ was being called before the data_ object was set, so createTiles_ had no idea how many Tiles should've been drawn. So, right now we're passing the number of Tiles that should be rendered to this function. On 2012/08/03 20:51:24, jeremycho wrote: > Any reason why this line was moved? http://codereview.chromium.org/10829131/diff/12001/chrome/browser/resources/n... File chrome/browser/resources/ntp_search/new_tab.html (right): http://codereview.chromium.org/10829131/diff/12001/chrome/browser/resources/n... chrome/browser/resources/ntp_search/new_tab.html:22: <!-- --> I'm going to actually comment out the mock script because it might interfere in the behavior of the program. On 2012/08/03 20:51:24, jeremycho wrote: > Delete? http://codereview.chromium.org/10829131/diff/12001/chrome/browser/resources/n... File chrome/browser/resources/ntp_search/new_tab.js (right): http://codereview.chromium.org/10829131/diff/12001/chrome/browser/resources/n... chrome/browser/resources/ntp_search/new_tab.js:90: // TODO(xci): remove the Trash component. On 2012/08/03 20:51:24, jeremycho wrote: > and page switcher? Done. http://codereview.chromium.org/10829131/diff/12001/chrome/browser/resources/n... chrome/browser/resources/ntp_search/new_tab.js:196: newTabView.cardSlider.currentCardValue.layout_(); I think you mean ChromeOS because it works fine for me on Linux. The bottom section of ChromeOS build is buggy right now and that's the reason why the code is failing there. I suspect it has to do with the fact that two NTPs are being loaded there, the old and the new one. On 2012/08/03 21:14:27, jeremycho wrote: > On Linux Chrome, this is throwing a JS error each time I open a new tab: > [27432:27432:164792166683:ERROR:CONSOLE(13797)] "Uncaught TypeError: Object > #<HTMLDivElement> has no method 'layout_'", source: chrome://newtab/ (13797) http://codereview.chromium.org/10829131/diff/12001/chrome/browser/resources/n... File chrome/browser/resources/ntp_search/thumbnail_page.js (right): http://codereview.chromium.org/10829131/diff/12001/chrome/browser/resources/n... chrome/browser/resources/ntp_search/thumbnail_page.js:51: this.className = 'tile-content most-visited real'; Sure, I was fixing the className problem and forgot to remove that line. Thanks. On 2012/08/03 20:51:24, jeremycho wrote: > Delete? It gets overridden in the next line. http://codereview.chromium.org/10829131/diff/12001/chrome/browser/resources/n... chrome/browser/resources/ntp_search/thumbnail_page.js:170: // If you want to use CSS to do that, you cant set this value to false. On 2012/08/03 20:51:24, jeremycho wrote: > cant->can Done. http://codereview.chromium.org/10829131/diff/12001/chrome/browser/resources/n... chrome/browser/resources/ntp_search/thumbnail_page.js:191: * @private On 2012/08/03 20:51:24, jeremycho wrote: > @param Done. http://codereview.chromium.org/10829131/diff/12001/chrome/browser/resources/n... File chrome/browser/resources/ntp_search/tile_page.js (right): http://codereview.chromium.org/10829131/diff/12001/chrome/browser/resources/n... chrome/browser/resources/ntp_search/tile_page.js:107: * animate with CSS to take advantage of hardware acceleration. On 2012/08/03 20:51:24, jeremycho wrote: > @param Done. http://codereview.chromium.org/10829131/diff/12001/chrome/browser/resources/n... chrome/browser/resources/ntp_search/tile_page.js:132: style.marginLeft = gridValues.tileHorMargin + 'px'; I don't think so. The spacing should be the same. Other things might change though for RTL languages, but I would like to address them in another CL. Adding a TODO comment here. On 2012/08/03 20:51:24, jeremycho wrote: > Should this be marginRight in RTL? http://codereview.chromium.org/10829131/diff/12001/chrome/browser/resources/n... chrome/browser/resources/ntp_search/tile_page.js:630: handleCardSelection_: function(e) { That's because the Apps are still using the old TilePage implementation, so the handleCardSelection_ of the TilePage should be called when you switch to Apps. On 2012/08/03 20:51:24, jeremycho wrote: > Now this only seems to get called when switching to Most Visited, not Apps? http://codereview.chromium.org/10829131/diff/12001/chrome/browser/resources/n... chrome/browser/resources/ntp_search/tile_page.js:1353: On 2012/08/03 20:51:24, jeremycho wrote: > newline Done. http://codereview.chromium.org/10829131/diff/12001/chrome/browser/resources/n... chrome/browser/resources/ntp_search/tile_page.js:1450: On 2012/08/03 20:51:24, jeremycho wrote: > newline Done.
http://codereview.chromium.org/10829131/diff/12001/chrome/browser/resources/n... File chrome/browser/resources/ntp_search/new_tab.js (right): http://codereview.chromium.org/10829131/diff/12001/chrome/browser/resources/n... chrome/browser/resources/ntp_search/new_tab.js:196: newTabView.cardSlider.currentCardValue.layout_(); I'll take another look once this bug is fixed. Other changes look good, thanks! On 2012/08/03 22:02:39, pedrosimonetti wrote: > I think you mean ChromeOS because it works fine for me on Linux. The bottom > section of ChromeOS build is buggy right now and that's the reason why the code > is failing there. I suspect it has to do with the fact that two NTPs are being > loaded there, the old and the new one. > > On 2012/08/03 21:14:27, jeremycho wrote: > > On Linux Chrome, this is throwing a JS error each time I open a new tab: > > [27432:27432:164792166683:ERROR:CONSOLE(13797)] "Uncaught TypeError: Object > > #<HTMLDivElement> has no method 'layout_'", source: chrome://newtab/ (13797) >
Jeremy, I fixed that initialization problem. Could you please confirm if it is working for you?
Confirmed that the bug fix works. Thanks for cleaning this up. I think it's ready to send out for review, unless David has any additional comments. As we discussed there's still more clean up to be done, but I think it's best to check this in ASAP so we can start to iterate. http://codereview.chromium.org/10829131/diff/14020/chrome/browser/resources/n... File chrome/browser/resources/ntp_search/new_tab.html (right): http://codereview.chromium.org/10829131/diff/14020/chrome/browser/resources/n... chrome/browser/resources/ntp_search/new_tab.html:22: <!-- Extra blank space.
lgtm
Thanks Jeremy, I just removed the trailing whitespace you mentioned on your last review. http://codereview.chromium.org/10829131/diff/14020/chrome/browser/resources/n... File chrome/browser/resources/ntp_search/new_tab.html (right): http://codereview.chromium.org/10829131/diff/14020/chrome/browser/resources/n... chrome/browser/resources/ntp_search/new_tab.html:22: <!-- On 2012/08/03 22:49:33, jeremycho wrote: > Extra blank space. Done.
Hi Evan, As we discussed, we've been reviewing this code internally first to save you time from reviewing trivial stuff. The first round of reviews occurred on the following CL: http://codereview.chromium.org/10823052/ As I mentioned before, I'll address the deletion of code later, specially because if I do that now git will stop recognizing my changes and will think I'm creating a new file from scratch (and you won't be able to see the before and after comparison).
am I meant to review this now?
On 2012/08/07 00:42:37, Evan Stade wrote: > am I meant to review this now? Yes, that would be great. Thanks!
On 2012/08/07 00:42:37, Evan Stade wrote: > am I meant to review this now? Yes, we would like you to review this code now. Sorry if I wasn't clear in my last comment. If you have questions about the code and want some quick response feel free to send me an IM.
A lot of the code here looks WIP-y. We don't really have a process for checkpointing WIP-y code; if the reviewer relaxes his standards then some bad code may get in and never get a proper review. If the reviewer doesn't relax his standards then it may be too cumbersome for fast-changing code to keep every revision up to those standards. http://codereview.chromium.org/10829131/diff/51/chrome/browser/resources/ntp_... File chrome/browser/resources/ntp_search/most_visited_page.css (right): http://codereview.chromium.org/10829131/diff/51/chrome/browser/resources/ntp_... chrome/browser/resources/ntp_search/most_visited_page.css:39: /* TODO(xci) organize rules */ : after ) http://codereview.chromium.org/10829131/diff/51/chrome/browser/resources/ntp_... File chrome/browser/resources/ntp_search/most_visited_page.js (right): http://codereview.chromium.org/10829131/diff/51/chrome/browser/resources/ntp_... chrome/browser/resources/ntp_search/most_visited_page.js:58: // TODO(xci) logging : after ), here and elsewhere http://codereview.chromium.org/10829131/diff/51/chrome/browser/resources/ntp_... File chrome/browser/resources/ntp_search/new_tab.css (right): http://codereview.chromium.org/10829131/diff/51/chrome/browser/resources/ntp_... chrome/browser/resources/ntp_search/new_tab.css:16: bottom: 0 !important; do not use !important http://codereview.chromium.org/10829131/diff/51/chrome/browser/resources/ntp_... chrome/browser/resources/ntp_search/new_tab.css:18: top: inherit !important; do not use !important http://codereview.chromium.org/10829131/diff/51/chrome/browser/resources/ntp_... chrome/browser/resources/ntp_search/new_tab.css:37: padding: 57px 13% 0 13% !important; do not use !important http://codereview.chromium.org/10829131/diff/51/chrome/browser/resources/ntp_... chrome/browser/resources/ntp_search/new_tab.css:51: max-width: 119px !important; do not use !important http://codereview.chromium.org/10829131/diff/51/chrome/browser/resources/ntp_... chrome/browser/resources/ntp_search/new_tab.css:55: tab titles less ugly. The problem is that the items have a fixed position start each line with * http://codereview.chromium.org/10829131/diff/51/chrome/browser/resources/ntp_... chrome/browser/resources/ntp_search/new_tab.css:57: to find a better way to solve this problem. */ a 60 line hack? I would rather you fix it than commit such a complicated hack. http://codereview.chromium.org/10829131/diff/51/chrome/browser/resources/ntp_... chrome/browser/resources/ntp_search/new_tab.css:63: border: none !important; do not use !important http://codereview.chromium.org/10829131/diff/51/chrome/browser/resources/ntp_... chrome/browser/resources/ntp_search/new_tab.css:67: color: #666 !important; do not use !important http://codereview.chromium.org/10829131/diff/51/chrome/browser/resources/ntp_... chrome/browser/resources/ntp_search/new_tab.css:75: color: rgb(221, 75, 57) !important; do not use !important http://codereview.chromium.org/10829131/diff/51/chrome/browser/resources/ntp_... chrome/browser/resources/ntp_search/new_tab.css:98: border-radius: 2px !important; do not use !important (here or elsewhere) http://codereview.chromium.org/10829131/diff/51/chrome/browser/resources/ntp_... chrome/browser/resources/ntp_search/new_tab.css:120: random extra newlines http://codereview.chromium.org/10829131/diff/51/chrome/browser/resources/ntp_... chrome/browser/resources/ntp_search/new_tab.css:135: 2 more newlines than you need http://codereview.chromium.org/10829131/diff/51/chrome/browser/resources/ntp_... chrome/browser/resources/ntp_search/new_tab.css:157: font-family: Arial; wrong font again http://codereview.chromium.org/10829131/diff/51/chrome/browser/resources/ntp_... chrome/browser/resources/ntp_search/new_tab.css:158: font-size: 13px; incorrectly specified font size again http://codereview.chromium.org/10829131/diff/51/chrome/browser/resources/ntp_... chrome/browser/resources/ntp_search/new_tab.css:167: /* TODO: selected class */ TODOs must have authors. http://codereview.chromium.org/10829131/diff/51/chrome/browser/resources/ntp_... chrome/browser/resources/ntp_search/new_tab.css:174: margin-left: 13px; the margins should probably be ems as well. http://codereview.chromium.org/10829131/diff/51/chrome/browser/resources/ntp_... chrome/browser/resources/ntp_search/new_tab.css:176: min-width: 55px; /* TODO: Confirm value with Marcin */ 2 spaces before comments. http://codereview.chromium.org/10829131/diff/51/chrome/browser/resources/ntp_... chrome/browser/resources/ntp_search/new_tab.css:181: 2 more newlines than you need http://codereview.chromium.org/10829131/diff/51/chrome/browser/resources/ntp_... chrome/browser/resources/ntp_search/new_tab.css:183: TODO(pedrosimonetti): refactor comments should be clear to people who didn't write them. http://codereview.chromium.org/10829131/diff/51/chrome/browser/resources/ntp_... chrome/browser/resources/ntp_search/new_tab.css:200: width: 748px; /* TODO move */ comments should be clear to people who didn't write them. http://codereview.chromium.org/10829131/diff/51/chrome/browser/resources/ntp_... chrome/browser/resources/ntp_search/new_tab.css:227: extra newline http://codereview.chromium.org/10829131/diff/51/chrome/browser/resources/ntp_... chrome/browser/resources/ntp_search/new_tab.css:249: -webkit-transition: all 201ms ease-in-out; why 201 ms? http://codereview.chromium.org/10829131/diff/51/chrome/browser/resources/ntp_... File chrome/browser/resources/ntp_search/new_tab.html (right): http://codereview.chromium.org/10829131/diff/51/chrome/browser/resources/ntp_... chrome/browser/resources/ntp_search/new_tab.html:90: <!-- if you're just going to comment things out without removing them, please add a comment. http://codereview.chromium.org/10829131/diff/51/chrome/browser/resources/ntp_... chrome/browser/resources/ntp_search/new_tab.html:99: newline why? http://codereview.chromium.org/10829131/diff/51/chrome/browser/resources/ntp_... chrome/browser/resources/ntp_search/new_tab.html:101: newline why? http://codereview.chromium.org/10829131/diff/51/chrome/browser/resources/ntp_... chrome/browser/resources/ntp_search/new_tab.html:202: <div id="apps-promo-extras-template" class="apps-promo-extras"> this should not exist. http://codereview.chromium.org/10829131/diff/51/chrome/browser/resources/ntp_... File chrome/browser/resources/ntp_search/thumbnail_page.css (right): http://codereview.chromium.org/10829131/diff/51/chrome/browser/resources/ntp_... chrome/browser/resources/ntp_search/thumbnail_page.css:6: display: none !important; why do you need !important? Don't use it if it's possible to avoid. http://codereview.chromium.org/10829131/diff/51/chrome/browser/resources/ntp_... chrome/browser/resources/ntp_search/thumbnail_page.css:31: font: 11px Arial; this is not the correct font to use. The font should depend on the platform. See SetFontAndTextDirection. Also the size should not be hard-coded; it should be an em value which will be relative to the inherited font size. http://codereview.chromium.org/10829131/diff/51/chrome/browser/resources/ntp_... File chrome/browser/resources/ntp_search/thumbnail_page.js (right): http://codereview.chromium.org/10829131/diff/51/chrome/browser/resources/ntp_... chrome/browser/resources/ntp_search/thumbnail_page.js:159: tileHorMargin: 18, // TODO margin with CSS. There's no margin in first col TODOs must have authors and the comment should be grammatical. Also two spaces before //. http://codereview.chromium.org/10829131/diff/51/chrome/browser/resources/ntp_... chrome/browser/resources/ntp_search/thumbnail_page.js:172: // debug comments should be sentence-like. http://codereview.chromium.org/10829131/diff/51/chrome/browser/resources/ntp_... chrome/browser/resources/ntp_search/thumbnail_page.js:203: for (var i = 0; i < THUMBNAIL_COUNT; i++) { why is this still THUMBNAIL_COUNT? http://codereview.chromium.org/10829131/diff/51/chrome/browser/resources/ntp_... chrome/browser/resources/ntp_search/thumbnail_page.js:208: if (!tile) { no curlies http://codereview.chromium.org/10829131/diff/51/chrome/browser/resources/ntp_... chrome/browser/resources/ntp_search/thumbnail_page.js:250: this newline intentionally omitted http://codereview.chromium.org/10829131/diff/51/chrome/browser/resources/ntp_... chrome/browser/resources/ntp_search/thumbnail_page.js:252: throw 'Each ThumbnailPage subclass should implement the data setter.'; usually we just console.error. I guess you could throw but you don't attempt to catch it AFAICT so there's not much point. http://codereview.chromium.org/10829131/diff/51/chrome/browser/resources/ntp_... chrome/browser/resources/ntp_search/thumbnail_page.js:266: // TODO(pedrosimonetti) is it possible to keep this inside Thumbnail class? put inside the doc block. http://codereview.chromium.org/10829131/diff/51/chrome/browser/resources/ntp_... File chrome/browser/resources/ntp_search/tile_page.js (right): http://codereview.chromium.org/10829131/diff/51/chrome/browser/resources/ntp_... chrome/browser/resources/ntp_search/tile_page.js:31: if (!Subclass.hasOwnProperty(name)) { no curlies http://codereview.chromium.org/10829131/diff/51/chrome/browser/resources/ntp_... chrome/browser/resources/ntp_search/tile_page.js:152: if (this.firstChild) { no curlies http://codereview.chromium.org/10829131/diff/51/chrome/browser/resources/ntp_... chrome/browser/resources/ntp_search/tile_page.js:444: this.tileGridContent_ = this.ownerDocument.createElement('div'); the code in this file doesn't look ready for review.
http://codereview.chromium.org/10829131/diff/51/chrome/browser/resources/ntp_... File chrome/browser/resources/ntp_search/most_visited_page.css (right): http://codereview.chromium.org/10829131/diff/51/chrome/browser/resources/ntp_... chrome/browser/resources/ntp_search/most_visited_page.css:39: /* TODO(xci) organize rules */ On 2012/08/07 21:57:12, Evan Stade wrote: > : after ) Done. http://codereview.chromium.org/10829131/diff/51/chrome/browser/resources/ntp_... File chrome/browser/resources/ntp_search/most_visited_page.js (right): http://codereview.chromium.org/10829131/diff/51/chrome/browser/resources/ntp_... chrome/browser/resources/ntp_search/most_visited_page.js:58: // TODO(xci) logging On 2012/08/07 21:57:12, Evan Stade wrote: > : after ), here and elsewhere Done. http://codereview.chromium.org/10829131/diff/51/chrome/browser/resources/ntp_... File chrome/browser/resources/ntp_search/new_tab.css (right): http://codereview.chromium.org/10829131/diff/51/chrome/browser/resources/ntp_... chrome/browser/resources/ntp_search/new_tab.css:16: bottom: 0 !important; On 2012/08/07 21:57:12, Evan Stade wrote: > do not use !important Done. http://codereview.chromium.org/10829131/diff/51/chrome/browser/resources/ntp_... chrome/browser/resources/ntp_search/new_tab.css:18: top: inherit !important; On 2012/08/07 21:57:12, Evan Stade wrote: > do not use !important Done. http://codereview.chromium.org/10829131/diff/51/chrome/browser/resources/ntp_... chrome/browser/resources/ntp_search/new_tab.css:37: padding: 57px 13% 0 13% !important; On 2012/08/07 21:57:12, Evan Stade wrote: > do not use !important Done. http://codereview.chromium.org/10829131/diff/51/chrome/browser/resources/ntp_... chrome/browser/resources/ntp_search/new_tab.css:51: max-width: 119px !important; On 2012/08/07 21:57:12, Evan Stade wrote: > do not use !important Done. http://codereview.chromium.org/10829131/diff/51/chrome/browser/resources/ntp_... chrome/browser/resources/ntp_search/new_tab.css:55: tab titles less ugly. The problem is that the items have a fixed position Rule + comment removed. On 2012/08/07 21:57:12, Evan Stade wrote: > start each line with * http://codereview.chromium.org/10829131/diff/51/chrome/browser/resources/ntp_... chrome/browser/resources/ntp_search/new_tab.css:57: to find a better way to solve this problem. */ Ack. We're going to delete the rest of the code in future CLs. On 2012/08/07 21:57:12, Evan Stade wrote: > a 60 line hack? I would rather you fix it than commit such a complicated hack. http://codereview.chromium.org/10829131/diff/51/chrome/browser/resources/ntp_... chrome/browser/resources/ntp_search/new_tab.css:63: border: none !important; On 2012/08/07 21:57:12, Evan Stade wrote: > do not use !important Done. http://codereview.chromium.org/10829131/diff/51/chrome/browser/resources/ntp_... chrome/browser/resources/ntp_search/new_tab.css:67: color: #666 !important; On 2012/08/07 21:57:12, Evan Stade wrote: > do not use !important Done. http://codereview.chromium.org/10829131/diff/51/chrome/browser/resources/ntp_... chrome/browser/resources/ntp_search/new_tab.css:75: color: rgb(221, 75, 57) !important; On 2012/08/07 21:57:12, Evan Stade wrote: > do not use !important Done. http://codereview.chromium.org/10829131/diff/51/chrome/browser/resources/ntp_... chrome/browser/resources/ntp_search/new_tab.css:98: border-radius: 2px !important; On 2012/08/07 21:57:12, Evan Stade wrote: > do not use !important > > (here or elsewhere) Done. http://codereview.chromium.org/10829131/diff/51/chrome/browser/resources/ntp_... chrome/browser/resources/ntp_search/new_tab.css:120: On 2012/08/07 21:57:12, Evan Stade wrote: > random extra newlines Done. http://codereview.chromium.org/10829131/diff/51/chrome/browser/resources/ntp_... chrome/browser/resources/ntp_search/new_tab.css:135: On 2012/08/07 21:57:12, Evan Stade wrote: > 2 more newlines than you need Done. http://codereview.chromium.org/10829131/diff/51/chrome/browser/resources/ntp_... chrome/browser/resources/ntp_search/new_tab.css:157: font-family: Arial; I removed all font-family properties, but I included one in body with a TODO to confirm the value with the UX folks, AFAIK they want to use Arial in all platforms. On 2012/08/07 21:57:12, Evan Stade wrote: > wrong font again http://codereview.chromium.org/10829131/diff/51/chrome/browser/resources/ntp_... chrome/browser/resources/ntp_search/new_tab.css:158: font-size: 13px; All font-sizes converted to em. On 2012/08/07 21:57:12, Evan Stade wrote: > incorrectly specified font size again http://codereview.chromium.org/10829131/diff/51/chrome/browser/resources/ntp_... chrome/browser/resources/ntp_search/new_tab.css:167: /* TODO: selected class */ On 2012/08/07 21:57:12, Evan Stade wrote: > TODOs must have authors. Done. http://codereview.chromium.org/10829131/diff/51/chrome/browser/resources/ntp_... chrome/browser/resources/ntp_search/new_tab.css:174: margin-left: 13px; On 2012/08/07 21:57:12, Evan Stade wrote: > the margins should probably be ems as well. Done. http://codereview.chromium.org/10829131/diff/51/chrome/browser/resources/ntp_... chrome/browser/resources/ntp_search/new_tab.css:176: min-width: 55px; /* TODO: Confirm value with Marcin */ On 2012/08/07 21:57:12, Evan Stade wrote: > 2 spaces before comments. Done. http://codereview.chromium.org/10829131/diff/51/chrome/browser/resources/ntp_... chrome/browser/resources/ntp_search/new_tab.css:181: On 2012/08/07 21:57:12, Evan Stade wrote: > 2 more newlines than you need Done. http://codereview.chromium.org/10829131/diff/51/chrome/browser/resources/ntp_... chrome/browser/resources/ntp_search/new_tab.css:183: TODO(pedrosimonetti): refactor Ack. On 2012/08/07 21:57:12, Evan Stade wrote: > comments should be clear to people who didn't write them. http://codereview.chromium.org/10829131/diff/51/chrome/browser/resources/ntp_... chrome/browser/resources/ntp_search/new_tab.css:200: width: 748px; /* TODO move */ On 2012/08/07 21:57:12, Evan Stade wrote: > comments should be clear to people who didn't write them. Ack. http://codereview.chromium.org/10829131/diff/51/chrome/browser/resources/ntp_... chrome/browser/resources/ntp_search/new_tab.css:227: On 2012/08/07 21:57:12, Evan Stade wrote: > extra newline Done. http://codereview.chromium.org/10829131/diff/51/chrome/browser/resources/ntp_... chrome/browser/resources/ntp_search/new_tab.css:249: -webkit-transition: all 201ms ease-in-out; On 2012/08/07 21:57:12, Evan Stade wrote: > why 201 ms? The default value is 200ms, but I think we're going to change it to 300 or 400, so I used 201 to make it easier to search/replace to the new value. http://codereview.chromium.org/10829131/diff/51/chrome/browser/resources/ntp_... File chrome/browser/resources/ntp_search/new_tab.html (right): http://codereview.chromium.org/10829131/diff/51/chrome/browser/resources/ntp_... chrome/browser/resources/ntp_search/new_tab.html:90: <!-- On 2012/08/07 21:57:12, Evan Stade wrote: > if you're just going to comment things out without removing them, please add a > comment. Done. http://codereview.chromium.org/10829131/diff/51/chrome/browser/resources/ntp_... chrome/browser/resources/ntp_search/new_tab.html:99: On 2012/08/07 21:57:12, Evan Stade wrote: > newline why? Done. http://codereview.chromium.org/10829131/diff/51/chrome/browser/resources/ntp_... chrome/browser/resources/ntp_search/new_tab.html:101: On 2012/08/07 21:57:12, Evan Stade wrote: > newline why? Done. http://codereview.chromium.org/10829131/diff/51/chrome/browser/resources/ntp_... chrome/browser/resources/ntp_search/new_tab.html:202: <div id="apps-promo-extras-template" class="apps-promo-extras"> On 2012/08/07 21:57:12, Evan Stade wrote: > this should not exist. Done. http://codereview.chromium.org/10829131/diff/51/chrome/browser/resources/ntp_... File chrome/browser/resources/ntp_search/thumbnail_page.css (right): http://codereview.chromium.org/10829131/diff/51/chrome/browser/resources/ntp_... chrome/browser/resources/ntp_search/thumbnail_page.css:6: display: none !important; On 2012/08/07 21:57:12, Evan Stade wrote: > why do you need !important? Don't use it if it's possible to avoid. Done. http://codereview.chromium.org/10829131/diff/51/chrome/browser/resources/ntp_... chrome/browser/resources/ntp_search/thumbnail_page.css:31: font: 11px Arial; On 2012/08/07 21:57:12, Evan Stade wrote: > this is not the correct font to use. The font should depend on the platform. See > SetFontAndTextDirection. > > Also the size should not be hard-coded; it should be an em value which will be > relative to the inherited font size. Done. http://codereview.chromium.org/10829131/diff/51/chrome/browser/resources/ntp_... File chrome/browser/resources/ntp_search/thumbnail_page.js (right): http://codereview.chromium.org/10829131/diff/51/chrome/browser/resources/ntp_... chrome/browser/resources/ntp_search/thumbnail_page.js:159: tileHorMargin: 18, // TODO margin with CSS. There's no margin in first col On 2012/08/07 21:57:12, Evan Stade wrote: > TODOs must have authors and the comment should be grammatical. Also two spaces > before //. Done. http://codereview.chromium.org/10829131/diff/51/chrome/browser/resources/ntp_... chrome/browser/resources/ntp_search/thumbnail_page.js:172: // debug On 2012/08/07 21:57:12, Evan Stade wrote: > comments should be sentence-like. Done. http://codereview.chromium.org/10829131/diff/51/chrome/browser/resources/ntp_... chrome/browser/resources/ntp_search/thumbnail_page.js:203: for (var i = 0; i < THUMBNAIL_COUNT; i++) { On 2012/08/07 21:57:12, Evan Stade wrote: > why is this still THUMBNAIL_COUNT? Code has been refactored. http://codereview.chromium.org/10829131/diff/51/chrome/browser/resources/ntp_... chrome/browser/resources/ntp_search/thumbnail_page.js:208: if (!tile) { On 2012/08/07 21:57:12, Evan Stade wrote: > no curlies Done. http://codereview.chromium.org/10829131/diff/51/chrome/browser/resources/ntp_... chrome/browser/resources/ntp_search/thumbnail_page.js:250: On 2012/08/07 21:57:12, Evan Stade wrote: > this newline intentionally omitted Done. http://codereview.chromium.org/10829131/diff/51/chrome/browser/resources/ntp_... chrome/browser/resources/ntp_search/thumbnail_page.js:252: throw 'Each ThumbnailPage subclass should implement the data setter.'; On 2012/08/07 21:57:12, Evan Stade wrote: > usually we just console.error. I guess you could throw but you don't attempt to > catch it AFAICT so there's not much point. Done. http://codereview.chromium.org/10829131/diff/51/chrome/browser/resources/ntp_... chrome/browser/resources/ntp_search/thumbnail_page.js:266: // TODO(pedrosimonetti) is it possible to keep this inside Thumbnail class? Code removed. On 2012/08/07 21:57:12, Evan Stade wrote: > put inside the doc block. http://codereview.chromium.org/10829131/diff/51/chrome/browser/resources/ntp_... File chrome/browser/resources/ntp_search/tile_page.js (right): http://codereview.chromium.org/10829131/diff/51/chrome/browser/resources/ntp_... chrome/browser/resources/ntp_search/tile_page.js:31: if (!Subclass.hasOwnProperty(name)) { On 2012/08/07 21:57:12, Evan Stade wrote: > no curlies Done. http://codereview.chromium.org/10829131/diff/51/chrome/browser/resources/ntp_... chrome/browser/resources/ntp_search/tile_page.js:152: if (this.firstChild) { On 2012/08/07 21:57:12, Evan Stade wrote: > no curlies Done. http://codereview.chromium.org/10829131/diff/51/chrome/browser/resources/ntp_... chrome/browser/resources/ntp_search/tile_page.js:444: this.tileGridContent_ = this.ownerDocument.createElement('div'); On 2012/08/07 21:57:12, Evan Stade wrote: > the code in this file doesn't look ready for review. It looks much better now.
http://codereview.chromium.org/10829131/diff/51/chrome/browser/resources/ntp_... File chrome/browser/resources/ntp_search/new_tab.css (right): http://codereview.chromium.org/10829131/diff/51/chrome/browser/resources/ntp_... chrome/browser/resources/ntp_search/new_tab.css:249: -webkit-transition: all 201ms ease-in-out; On 2012/08/08 08:14:22, pedrosimonetti wrote: > On 2012/08/07 21:57:12, Evan Stade wrote: > > why 201 ms? > > The default value is 200ms, but I think we're going to change it to 300 or 400, > so I used 201 to make it easier to search/replace to the new value. animations in chrome are limited to 250ms. http://codereview.chromium.org/10829131/diff/51/chrome/browser/resources/ntp_... chrome/browser/resources/ntp_search/new_tab.css:282: background: #fff; white http://codereview.chromium.org/10829131/diff/51/chrome/browser/resources/ntp_... chrome/browser/resources/ntp_search/new_tab.css:296: .xxxxtile-cell > div > span { are these x's supposed to be here? http://codereview.chromium.org/10829131/diff/51/chrome/browser/resources/ntp_... chrome/browser/resources/ntp_search/new_tab.css:333: you still have inconsistent # of newlines before new sections http://codereview.chromium.org/10829131/diff/16002/chrome/browser/resources/n... File chrome/browser/resources/ntp_search/most_visited_page.css (right): http://codereview.chromium.org/10829131/diff/16002/chrome/browser/resources/n... chrome/browser/resources/ntp_search/most_visited_page.css:39: /* TODO(xci): organize rules */ sentence-like punctuation, i.e. Organize these rules. http://codereview.chromium.org/10829131/diff/16002/chrome/browser/resources/n... File chrome/browser/resources/ntp_search/most_visited_page.js (right): http://codereview.chromium.org/10829131/diff/16002/chrome/browser/resources/n... chrome/browser/resources/ntp_search/most_visited_page.js:29: // TODO(pedrosimonetti): Do we still need this property? I don't get it. You're adding a new property, and at the same time asking if it's still needed. http://codereview.chromium.org/10829131/diff/16002/chrome/browser/resources/n... File chrome/browser/resources/ntp_search/new_tab.css (right): http://codereview.chromium.org/10829131/diff/16002/chrome/browser/resources/n... chrome/browser/resources/ntp_search/new_tab.css:11: #xci #card-slider-frame { s/xci/ntp5 http://codereview.chromium.org/10829131/diff/16002/chrome/browser/resources/n... chrome/browser/resources/ntp_search/new_tab.css:79: /* TODO(pedrosimonetti): Confirm with designers/engineers what do we want please confirm with Glen. We've made this decision in the past and I wouldn't like the decision to change simply because a different person made it. http://codereview.chromium.org/10829131/diff/16002/chrome/browser/resources/n... chrome/browser/resources/ntp_search/new_tab.css:81: */ move this */ to end of previous line. http://codereview.chromium.org/10829131/diff/16002/chrome/browser/resources/n... chrome/browser/resources/ntp_search/new_tab.css:124: min-width: 55px; /* TODO(xci): Confirm value with Marcin. */ it's highly likely the min width needs to be locale-specific; see max-width code in ntp4. http://codereview.chromium.org/10829131/diff/16002/chrome/browser/resources/n... chrome/browser/resources/ntp_search/new_tab.css:149: text-align: left; start, not left. http://codereview.chromium.org/10829131/diff/16002/chrome/browser/resources/n... chrome/browser/resources/ntp_search/new_tab.css:163: margin-left: 0 !important; boo on !important http://codereview.chromium.org/10829131/diff/16002/chrome/browser/resources/n... chrome/browser/resources/ntp_search/new_tab.css:163: margin-left: 0 !important; -webkit-margin-start http://codereview.chromium.org/10829131/diff/16002/chrome/browser/resources/n... chrome/browser/resources/ntp_search/new_tab.css:165: ^H http://codereview.chromium.org/10829131/diff/16002/chrome/browser/resources/n... chrome/browser/resources/ntp_search/new_tab.css:172: -webkit-transition: all 201ms ease-in-out !important; I don't think all is a good idea. Specify the properties you want to transition. http://codereview.chromium.org/10829131/diff/16002/chrome/browser/resources/n... chrome/browser/resources/ntp_search/new_tab.css:182: -webkit-transition: all 201ms ease-in-out; why are these all ease-in-out? http://codereview.chromium.org/10829131/diff/16002/chrome/browser/resources/n... chrome/browser/resources/ntp_search/new_tab.css:205: margin-left: 18px; -webkit-margin-start http://codereview.chromium.org/10829131/diff/16002/chrome/browser/resources/n... chrome/browser/resources/ntp_search/new_tab.css:254: margin-right: -10px; -webkit-margin-end http://codereview.chromium.org/10829131/diff/16002/chrome/browser/resources/n... chrome/browser/resources/ntp_search/new_tab.css:255: opacity: 0 !important; no !importants http://codereview.chromium.org/10829131/diff/16002/chrome/browser/resources/n... chrome/browser/resources/ntp_search/new_tab.css:277: ^H http://codereview.chromium.org/10829131/diff/16002/chrome/browser/resources/n... File chrome/browser/resources/ntp_search/page_list_view.js (right): http://codereview.chromium.org/10829131/diff/16002/chrome/browser/resources/n... chrome/browser/resources/ntp_search/page_list_view.js:505: // TODO(xci): delete put the TODO inside the doc block, and why not actually delete the code inside? http://codereview.chromium.org/10829131/diff/16002/chrome/browser/resources/n... File chrome/browser/resources/ntp_search/tile_page.js (right): http://codereview.chromium.org/10829131/diff/16002/chrome/browser/resources/n... chrome/browser/resources/ntp_search/tile_page.js:23: * any HTMLElement), so we need to individually add methods and getters here. @param @return http://codereview.chromium.org/10829131/diff/16002/chrome/browser/resources/n... chrome/browser/resources/ntp_search/tile_page.js:28: if (!Subclass.hasOwnProperty(name)) { no curlies http://codereview.chromium.org/10829131/diff/16002/chrome/browser/resources/n... chrome/browser/resources/ntp_search/tile_page.js:68: ^H http://codereview.chromium.org/10829131/diff/16002/chrome/browser/resources/n... chrome/browser/resources/ntp_search/tile_page.js:124: if (this.firstChild) { no curlies http://codereview.chromium.org/10829131/diff/16002/chrome/browser/resources/n... chrome/browser/resources/ntp_search/tile_page.js:281: /* no commented out code or blank TODOs http://codereview.chromium.org/10829131/diff/16002/chrome/browser/resources/n... chrome/browser/resources/ntp_search/tile_page.js:296: /* ditto http://codereview.chromium.org/10829131/diff/16002/chrome/browser/resources/n... chrome/browser/resources/ntp_search/tile_page.js:407: return; why is there a return and then more code? http://codereview.chromium.org/10829131/diff/16002/chrome/browser/resources/n... chrome/browser/resources/ntp_search/tile_page.js:419: // TODO(xci) Delete. Make footer non-required. See shared/js/util.js into doc block. http://codereview.chromium.org/10829131/diff/16002/chrome/browser/resources/n... chrome/browser/resources/ntp_search/tile_page.js:467: // XCI - Extended Chrome Instant and here I thought xci was a person? TODOs must have author labels, not feature labels. http://codereview.chromium.org/10829131/diff/16002/chrome/browser/resources/n... chrome/browser/resources/ntp_search/tile_page.js:828: if (rule) { no curlies http://codereview.chromium.org/10829131/diff/16002/chrome/browser/resources/n... chrome/browser/resources/ntp_search/tile_page.js:841: { no curlies http://codereview.chromium.org/10829131/diff/16002/chrome/browser/resources/n... chrome/browser/resources/ntp_search/tile_page.js:847: function changeSlowFactor(el) { document all functions. http://codereview.chromium.org/10829131/diff/16002/chrome/browser/resources/n... chrome/browser/resources/ntp_search/tile_page.js:857: function deprecate() { this doesn't appear to be used. http://codereview.chromium.org/10829131/diff/16002/chrome/browser/resources/n... chrome/browser/resources/ntp_search/tile_page.js:863: //getCurrentlyDraggingTile2: deprecate, no commented out code
Implemented the changes suggested by Evan. http://codereview.chromium.org/10829131/diff/16002/chrome/browser/resources/n... File chrome/browser/resources/ntp_search/most_visited_page.css (right): http://codereview.chromium.org/10829131/diff/16002/chrome/browser/resources/n... chrome/browser/resources/ntp_search/most_visited_page.css:39: /* TODO(xci): organize rules */ On 2012/08/08 23:19:18, Evan Stade wrote: > sentence-like punctuation, i.e. Organize these rules. Done. http://codereview.chromium.org/10829131/diff/16002/chrome/browser/resources/n... File chrome/browser/resources/ntp_search/most_visited_page.js (right): http://codereview.chromium.org/10829131/diff/16002/chrome/browser/resources/n... chrome/browser/resources/ntp_search/most_visited_page.js:29: // TODO(pedrosimonetti): Do we still need this property? This property was in Thumbnail class, and I moved to MostVisited to make things clear when addressing your previous comments. After moving I included this note to check later if this property was still being used. Which is not, so I'm removing. On 2012/08/08 23:19:18, Evan Stade wrote: > I don't get it. You're adding a new property, and at the same time asking if > it's still needed. http://codereview.chromium.org/10829131/diff/16002/chrome/browser/resources/n... File chrome/browser/resources/ntp_search/new_tab.css (right): http://codereview.chromium.org/10829131/diff/16002/chrome/browser/resources/n... chrome/browser/resources/ntp_search/new_tab.css:11: #xci #card-slider-frame { On 2012/08/08 23:19:18, Evan Stade wrote: > s/xci/ntp5 Done. http://codereview.chromium.org/10829131/diff/16002/chrome/browser/resources/n... chrome/browser/resources/ntp_search/new_tab.css:79: /* TODO(pedrosimonetti): Confirm with designers/engineers what do we want On 2012/08/08 23:19:18, Evan Stade wrote: > please confirm with Glen. We've made this decision in the past and I wouldn't > like the decision to change simply because a different person made it. Okay, we'll confirm this, but lets submit the code this way for now while we do that. http://codereview.chromium.org/10829131/diff/16002/chrome/browser/resources/n... chrome/browser/resources/ntp_search/new_tab.css:81: */ On 2012/08/08 23:19:18, Evan Stade wrote: > move this */ to end of previous line. Done. http://codereview.chromium.org/10829131/diff/16002/chrome/browser/resources/n... chrome/browser/resources/ntp_search/new_tab.css:124: min-width: 55px; /* TODO(xci): Confirm value with Marcin. */ I don't think so. Max width I understand because some languages will have longer words, the there's no problem with min in this case. We're not cropping, we're expanding when it's too small. On 2012/08/08 23:19:18, Evan Stade wrote: > it's highly likely the min width needs to be locale-specific; see max-width code > in ntp4. http://codereview.chromium.org/10829131/diff/16002/chrome/browser/resources/n... chrome/browser/resources/ntp_search/new_tab.css:149: text-align: left; On 2012/08/08 23:19:18, Evan Stade wrote: > start, not left. Done. http://codereview.chromium.org/10829131/diff/16002/chrome/browser/resources/n... chrome/browser/resources/ntp_search/new_tab.css:163: margin-left: 0 !important; On 2012/08/08 23:19:18, Evan Stade wrote: > boo on !important Okay. I'll do this in another CL because I'll have to re-implement some pieces of the animation code to address some changes in the spec. So I'll have to review the CSS too. http://codereview.chromium.org/10829131/diff/16002/chrome/browser/resources/n... chrome/browser/resources/ntp_search/new_tab.css:163: margin-left: 0 !important; On 2012/08/08 23:19:18, Evan Stade wrote: > boo on !important Ack. http://codereview.chromium.org/10829131/diff/16002/chrome/browser/resources/n... chrome/browser/resources/ntp_search/new_tab.css:165: On 2012/08/08 23:19:18, Evan Stade wrote: > ^H Done. http://codereview.chromium.org/10829131/diff/16002/chrome/browser/resources/n... chrome/browser/resources/ntp_search/new_tab.css:172: -webkit-transition: all 201ms ease-in-out !important; On 2012/08/08 23:19:18, Evan Stade wrote: > I don't think all is a good idea. Specify the properties you want to transition. Okay. I'll do this in another CL because I'll have to re-implement some pieces of the animation code to address some changes in the spec. So I'll have to review the CSS too. http://codereview.chromium.org/10829131/diff/16002/chrome/browser/resources/n... chrome/browser/resources/ntp_search/new_tab.css:182: -webkit-transition: all 201ms ease-in-out; On 2012/08/08 23:19:18, Evan Stade wrote: > why are these all ease-in-out? If you mean "why are these all *ease-in-out*" the spec says so. If you mean "why are these *ALL 201ms ease-in-out*", I'll check if we can add individual properties in another CL. http://codereview.chromium.org/10829131/diff/16002/chrome/browser/resources/n... chrome/browser/resources/ntp_search/new_tab.css:205: margin-left: 18px; On 2012/08/08 23:19:18, Evan Stade wrote: > -webkit-margin-start Done. http://codereview.chromium.org/10829131/diff/16002/chrome/browser/resources/n... chrome/browser/resources/ntp_search/new_tab.css:254: margin-right: -10px; On 2012/08/08 23:19:18, Evan Stade wrote: > -webkit-margin-end Done. http://codereview.chromium.org/10829131/diff/16002/chrome/browser/resources/n... chrome/browser/resources/ntp_search/new_tab.css:255: opacity: 0 !important; I'll review the !important properties when I review the animation in another CL. On 2012/08/08 23:19:18, Evan Stade wrote: > no !importants http://codereview.chromium.org/10829131/diff/16002/chrome/browser/resources/n... chrome/browser/resources/ntp_search/new_tab.css:277: On 2012/08/08 23:19:18, Evan Stade wrote: > ^H Done. http://codereview.chromium.org/10829131/diff/16002/chrome/browser/resources/n... File chrome/browser/resources/ntp_search/page_list_view.js (right): http://codereview.chromium.org/10829131/diff/16002/chrome/browser/resources/n... chrome/browser/resources/ntp_search/page_list_view.js:505: // TODO(xci): delete On 2012/08/08 23:19:18, Evan Stade wrote: > put the TODO inside the doc block, and why not actually delete the code inside? Done. Because it requires changing other files we're not using right now, so we're gonna do this in another CL. http://codereview.chromium.org/10829131/diff/16002/chrome/browser/resources/n... File chrome/browser/resources/ntp_search/tile_page.js (right): http://codereview.chromium.org/10829131/diff/16002/chrome/browser/resources/n... chrome/browser/resources/ntp_search/tile_page.js:23: * any HTMLElement), so we need to individually add methods and getters here. On 2012/08/08 23:19:18, Evan Stade wrote: > @param > @return Done. http://codereview.chromium.org/10829131/diff/16002/chrome/browser/resources/n... chrome/browser/resources/ntp_search/tile_page.js:28: if (!Subclass.hasOwnProperty(name)) { On 2012/08/08 23:19:18, Evan Stade wrote: > no curlies Done. http://codereview.chromium.org/10829131/diff/16002/chrome/browser/resources/n... chrome/browser/resources/ntp_search/tile_page.js:68: On 2012/08/08 23:19:18, Evan Stade wrote: > ^H Done. http://codereview.chromium.org/10829131/diff/16002/chrome/browser/resources/n... chrome/browser/resources/ntp_search/tile_page.js:124: if (this.firstChild) { On 2012/08/08 23:19:18, Evan Stade wrote: > no curlies Done. http://codereview.chromium.org/10829131/diff/16002/chrome/browser/resources/n... chrome/browser/resources/ntp_search/tile_page.js:281: /* On 2012/08/08 23:19:18, Evan Stade wrote: > no commented out code or blank TODOs Done. http://codereview.chromium.org/10829131/diff/16002/chrome/browser/resources/n... chrome/browser/resources/ntp_search/tile_page.js:296: /* On 2012/08/08 23:19:18, Evan Stade wrote: > ditto Done. http://codereview.chromium.org/10829131/diff/16002/chrome/browser/resources/n... chrome/browser/resources/ntp_search/tile_page.js:407: return; On 2012/08/08 23:19:18, Evan Stade wrote: > why is there a return and then more code? We need to fix the notification code, so I included a comment clarifying what we have to do. http://codereview.chromium.org/10829131/diff/16002/chrome/browser/resources/n... chrome/browser/resources/ntp_search/tile_page.js:419: // TODO(xci) Delete. Make footer non-required. See shared/js/util.js On 2012/08/08 23:19:18, Evan Stade wrote: > into doc block. Done. http://codereview.chromium.org/10829131/diff/16002/chrome/browser/resources/n... chrome/browser/resources/ntp_search/tile_page.js:467: // XCI - Extended Chrome Instant On 2012/08/08 23:19:18, Evan Stade wrote: > and here I thought xci was a person? TODOs must have author labels, not feature > labels. Done. http://codereview.chromium.org/10829131/diff/16002/chrome/browser/resources/n... chrome/browser/resources/ntp_search/tile_page.js:828: if (rule) { On 2012/08/08 23:19:18, Evan Stade wrote: > no curlies Done. http://codereview.chromium.org/10829131/diff/16002/chrome/browser/resources/n... chrome/browser/resources/ntp_search/tile_page.js:841: { On 2012/08/08 23:19:18, Evan Stade wrote: > no curlies Done. http://codereview.chromium.org/10829131/diff/16002/chrome/browser/resources/n... chrome/browser/resources/ntp_search/tile_page.js:847: function changeSlowFactor(el) { On 2012/08/08 23:19:18, Evan Stade wrote: > document all functions. Done. http://codereview.chromium.org/10829131/diff/16002/chrome/browser/resources/n... chrome/browser/resources/ntp_search/tile_page.js:857: function deprecate() { Now it is being used. On 2012/08/08 23:19:18, Evan Stade wrote: > this doesn't appear to be used. http://codereview.chromium.org/10829131/diff/16002/chrome/browser/resources/n... chrome/browser/resources/ntp_search/tile_page.js:863: //getCurrentlyDraggingTile2: deprecate, On 2012/08/08 23:19:18, Evan Stade wrote: > no commented out code Done.
http://codereview.chromium.org/10829131/diff/16002/chrome/browser/resources/n... File chrome/browser/resources/ntp_search/new_tab.css (right): http://codereview.chromium.org/10829131/diff/16002/chrome/browser/resources/n... chrome/browser/resources/ntp_search/new_tab.css:182: -webkit-transition: all 201ms ease-in-out; On 2012/08/09 21:33:32, pedrosimonetti wrote: > On 2012/08/08 23:19:18, Evan Stade wrote: > > why are these all ease-in-out? > > If you mean "why are these all *ease-in-out*" the spec says so. I meant this. I don't see where the design doc says ease-in-out in it. Are you talking about a live mock? Live mocks are almost always created by an authoring tool which adds random rules like ease-in-out which were not really intended by the author. Can you explain why ease-in-out is preferable over the default? If not, you should figure it out before copying it. > > If you mean "why are these *ALL 201ms ease-in-out*", I'll check if we can add > individual properties in another CL. http://codereview.chromium.org/10829131/diff/16002/chrome/browser/resources/n... chrome/browser/resources/ntp_search/new_tab.css:191: #card-slider-frame .tile-row { the last 3 rule blocks are almost identical; share what you can. http://codereview.chromium.org/10829131/diff/16002/chrome/browser/resources/n... chrome/browser/resources/ntp_search/new_tab.css:255: opacity: 0 !important; On 2012/08/09 21:33:32, pedrosimonetti wrote: > I'll review the !important properties when I review the animation in another CL. > > On 2012/08/08 23:19:18, Evan Stade wrote: > > no !importants > there should be no importants. Don't commit code you know you are going to delete. It makes it impossible to review---how do I know where the "definitely will be deleted" code starts or ends? How do I easily follow up to make sure the code does actually get changed soon? http://codereview.chromium.org/10829131/diff/16002/chrome/browser/resources/n... File chrome/browser/resources/ntp_search/page_list_view.js (right): http://codereview.chromium.org/10829131/diff/16002/chrome/browser/resources/n... chrome/browser/resources/ntp_search/page_list_view.js:505: // TODO(xci): delete On 2012/08/09 21:33:32, pedrosimonetti wrote: > On 2012/08/08 23:19:18, Evan Stade wrote: > > put the TODO inside the doc block, and why not actually delete the code > inside? > > Done. Because it requires changing other files we're not using right now, so > we're gonna do this in another CL. I don't understand this response. Deleting this newly dead code requires changing what file? http://codereview.chromium.org/10829131/diff/7010/chrome/browser/resources/nt... File chrome/browser/resources/ntp_search/tile_page.js (right): http://codereview.chromium.org/10829131/diff/7010/chrome/browser/resources/nt... chrome/browser/resources/ntp_search/tile_page.js:135: // TODO(pedrosimonetti): Change to marginRight for RTL languages? no, webkitMarginStart http://codereview.chromium.org/10829131/diff/7010/chrome/browser/resources/nt... chrome/browser/resources/ntp_search/tile_page.js:215: tileHorMargin: 0, document all these values; do not abbreviate horizontal or vertical http://codereview.chromium.org/10829131/diff/7010/chrome/browser/resources/nt... chrome/browser/resources/ntp_search/tile_page.js:437: if (this.notification && !this.notification.hidden) { do not add dead code. The chrome repo is not a storage place for WIP code. http://codereview.chromium.org/10829131/diff/7010/chrome/browser/resources/nt... chrome/browser/resources/ntp_search/tile_page.js:474: TODO(pedrosimonetti): Make sure the following removed features are not this would be better as a note to yourself on your personal machine. http://codereview.chromium.org/10829131/diff/7010/chrome/browser/resources/nt... chrome/browser/resources/ntp_search/tile_page.js:503: rowCount: 2, underscores, documentation on all these properties. http://codereview.chromium.org/10829131/diff/7010/chrome/browser/resources/nt... chrome/browser/resources/ntp_search/tile_page.js:558: * Gets the number of columns for a given with. sp: width also, the number of columns for a given width is unclear. How about "the maximum number of columns that can fit in a given width" http://codereview.chromium.org/10829131/diff/7010/chrome/browser/resources/nt... chrome/browser/resources/ntp_search/tile_page.js:559: * @param {number} width The width. in what unit? http://codereview.chromium.org/10829131/diff/7010/chrome/browser/resources/nt... chrome/browser/resources/ntp_search/tile_page.js:584: // TODO(pedrosimonetti): Add contants? spelling. And yes, please do: it gives you a place to document what these numbers mean. http://codereview.chromium.org/10829131/diff/7010/chrome/browser/resources/nt... chrome/browser/resources/ntp_search/tile_page.js:625: var tileCell; this isn't ansi c. Declare the variables where they are used, as late as possible. http://codereview.chromium.org/10829131/diff/7010/chrome/browser/resources/nt... chrome/browser/resources/ntp_search/tile_page.js:644: var rowVisible = (row >= pageOffset && this paren not needed http://codereview.chromium.org/10829131/diff/7010/chrome/browser/resources/nt... chrome/browser/resources/ntp_search/tile_page.js:673: if (tileCell.firstChild) { this is easier to read as if (!tileCell.firstChild) // ... else if (tileElement != tileCell.firstChild) // ... http://codereview.chromium.org/10829131/diff/7010/chrome/browser/resources/nt... chrome/browser/resources/ntp_search/tile_page.js:679: } else { else if http://codereview.chromium.org/10829131/diff/7010/chrome/browser/resources/nt... chrome/browser/resources/ntp_search/tile_page.js:714: var tileRows = tileGridContent.getElementsByClassName('tile-row'); declare where used http://codereview.chromium.org/10829131/diff/7010/chrome/browser/resources/nt... chrome/browser/resources/ntp_search/tile_page.js:730: numOfVisibleRows = 2; ternary operator http://codereview.chromium.org/10829131/diff/7010/chrome/browser/resources/nt... chrome/browser/resources/ntp_search/tile_page.js:738: //tileGrid.style.height = (107 * numOfVisibleRows) + 'px'; no dead code http://codereview.chromium.org/10829131/diff/7010/chrome/browser/resources/nt... chrome/browser/resources/ntp_search/tile_page.js:743: random newline http://codereview.chromium.org/10829131/diff/7010/chrome/browser/resources/nt... chrome/browser/resources/ntp_search/tile_page.js:746: random newline http://codereview.chromium.org/10829131/diff/7010/chrome/browser/resources/nt... chrome/browser/resources/ntp_search/tile_page.js:762: random newline http://codereview.chromium.org/10829131/diff/7010/chrome/browser/resources/nt... chrome/browser/resources/ntp_search/tile_page.js:876: if (this.tileGridContent_.classList.contains('animate-tile')) { rather than if(){ if(){}}, you can use another && http://codereview.chromium.org/10829131/diff/7010/chrome/browser/resources/nt... chrome/browser/resources/ntp_search/tile_page.js:886: */ @param docs, and explain what this is doing. http://codereview.chromium.org/10829131/diff/7010/chrome/browser/resources/nt... chrome/browser/resources/ntp_search/tile_page.js:937: for (var i = 0, l = rules.length; i < l; i++) { do not abbreviate length to l http://codereview.chromium.org/10829131/diff/7010/chrome/browser/resources/nt... chrome/browser/resources/ntp_search/tile_page.js:945: * Changes the slow factor for animations. is this for testing? if so, say so. if you're going to hack a way to slow down animations, why not just add a rule like * { -webkit-transition-duration: 5s !important; } what you have is fragile and complicated in comparison.
http://codereview.chromium.org/10829131/diff/16002/chrome/browser/resources/n... File chrome/browser/resources/ntp_search/new_tab.css (right): http://codereview.chromium.org/10829131/diff/16002/chrome/browser/resources/n... chrome/browser/resources/ntp_search/new_tab.css:182: -webkit-transition: all 201ms ease-in-out; This is intended from a design point of view. All translate animations should always be eased. On 2012/08/10 02:25:19, Evan Stade wrote: > On 2012/08/09 21:33:32, pedrosimonetti wrote: > > On 2012/08/08 23:19:18, Evan Stade wrote: > > > why are these all ease-in-out? > > > > If you mean "why are these all *ease-in-out*" the spec says so. > > I meant this. I don't see where the design doc says ease-in-out in it. Are you > talking about a live mock? Live mocks are almost always created by an authoring > tool which adds random rules like ease-in-out which were not really intended by > the author. Can you explain why ease-in-out is preferable over the default? If > not, you should figure it out before copying it. > > > > > If you mean "why are these *ALL 201ms ease-in-out*", I'll check if we can add > > individual properties in another CL. >
http://codereview.chromium.org/10829131/diff/16002/chrome/browser/resources/n... File chrome/browser/resources/ntp_search/new_tab.css (right): http://codereview.chromium.org/10829131/diff/16002/chrome/browser/resources/n... chrome/browser/resources/ntp_search/new_tab.css:182: -webkit-transition: all 201ms ease-in-out; On 2012/08/10 02:25:19, Evan Stade wrote: > On 2012/08/09 21:33:32, pedrosimonetti wrote: > > On 2012/08/08 23:19:18, Evan Stade wrote: > > > why are these all ease-in-out? > > > > If you mean "why are these all *ease-in-out*" the spec says so. > > I meant this. I don't see where the design doc says ease-in-out in it. Are you > talking about a live mock? Live mocks are almost always created by an authoring > tool which adds random rules like ease-in-out which were not really intended by > the author. Can you explain why ease-in-out is preferable over the default? If > not, you should figure it out before copying it. > > > > > If you mean "why are these *ALL 201ms ease-in-out*", I'll check if we can add > > individual properties in another CL. > I was talking about the the specification (both the main specification and the resizing animation specification), not the design doc. You can find a link to both specs in the design doc though. Our main specification uses ease-in-out for most animations (sometimes linear), but the easing function is not explicitly specified in our resizing spec, and it seems that our live mock uses linear, and our live mock was created by hand by one of our designers, so I think he really meant the use of "linear". I'm adding a TODO note to confirm which easing function to use with him. http://codereview.chromium.org/10829131/diff/16002/chrome/browser/resources/n... chrome/browser/resources/ntp_search/new_tab.css:191: #card-slider-frame .tile-row { On 2012/08/10 02:25:19, Evan Stade wrote: > the last 3 rule blocks are almost identical; share what you can. Since I'll have to review the animation code, I'd rather do this later because after changing the properties from "all" to individual properties these rules will not be identical anymore. http://codereview.chromium.org/10829131/diff/16002/chrome/browser/resources/n... chrome/browser/resources/ntp_search/new_tab.css:255: opacity: 0 !important; > there should be no importants. If !important is so nasty why do you use it in the NTP4 code? I understand why using it might affect the readability of code, but I think it's not worth blocking the NTP5 development for such unimportant (pun intended) thing. Anyway, I removed all !important that are not strictly necessary. The ones that are still there are necessary to override a property that was set using JavaScript. > Don't commit code you know you are going to > delete. It makes it impossible to review---how do I know where the "definitely > will be deleted" code starts or ends? How do I easily follow up to make sure the > code does actually get changed soon? I was marking all code that we need to delete with comments like TODO(pedrosimonetti): Delete. So a simply search using grep, ack or your favorite search tool will easily show you if that code is still there. And since you're reviewing our NTP code you will know for sure when we change any part of the code. I think it would be much easier to delete each feature at once, because deleting code is not just a matter of deleting a function and its callers. We need to make sure that that logic is not necessary anymore, and we need to check in each caller what was going on, once we might want to replace some code there. http://codereview.chromium.org/10829131/diff/16002/chrome/browser/resources/n... File chrome/browser/resources/ntp_search/page_list_view.js (right): http://codereview.chromium.org/10829131/diff/16002/chrome/browser/resources/n... chrome/browser/resources/ntp_search/page_list_view.js:505: // TODO(xci): delete On 2012/08/10 02:25:19, Evan Stade wrote: > On 2012/08/09 21:33:32, pedrosimonetti wrote: > > On 2012/08/08 23:19:18, Evan Stade wrote: > > > put the TODO inside the doc block, and why not actually delete the code > > inside? > > > > Done. Because it requires changing other files we're not using right now, so > > we're gonna do this in another CL. > > I don't understand this response. Deleting this newly dead code requires > changing what file? These methods are being used by the new_tab.js file, and it uses code from apps_page.js which is not included in this CL yet because we agreed to submit the NTP5 code in smaller pieces. http://codereview.chromium.org/10829131/diff/7010/chrome/browser/resources/nt... File chrome/browser/resources/ntp_search/tile_page.js (right): http://codereview.chromium.org/10829131/diff/7010/chrome/browser/resources/nt... chrome/browser/resources/ntp_search/tile_page.js:135: // TODO(pedrosimonetti): Change to marginRight for RTL languages? On 2012/08/10 02:25:19, Evan Stade wrote: > no, webkitMarginStart Done. http://codereview.chromium.org/10829131/diff/7010/chrome/browser/resources/nt... chrome/browser/resources/ntp_search/tile_page.js:215: tileHorMargin: 0, On 2012/08/10 02:25:19, Evan Stade wrote: > document all these values; do not abbreviate horizontal or vertical Done. http://codereview.chromium.org/10829131/diff/7010/chrome/browser/resources/nt... chrome/browser/resources/ntp_search/tile_page.js:437: if (this.notification && !this.notification.hidden) { On 2012/08/10 02:25:19, Evan Stade wrote: > do not add dead code. The chrome repo is not a storage place for WIP code. Done. http://codereview.chromium.org/10829131/diff/7010/chrome/browser/resources/nt... chrome/browser/resources/ntp_search/tile_page.js:474: TODO(pedrosimonetti): Make sure the following removed features are not On 2012/08/10 02:25:19, Evan Stade wrote: > this would be better as a note to yourself on your personal machine. Done. http://codereview.chromium.org/10829131/diff/7010/chrome/browser/resources/nt... chrome/browser/resources/ntp_search/tile_page.js:503: rowCount: 2, On 2012/08/10 02:25:19, Evan Stade wrote: > underscores, documentation on all these properties. Done. http://codereview.chromium.org/10829131/diff/7010/chrome/browser/resources/nt... chrome/browser/resources/ntp_search/tile_page.js:558: * Gets the number of columns for a given with. On 2012/08/10 02:25:19, Evan Stade wrote: > sp: width > > also, the number of columns for a given width is unclear. How about "the maximum > number of columns that can fit in a given width" Done. http://codereview.chromium.org/10829131/diff/7010/chrome/browser/resources/nt... chrome/browser/resources/ntp_search/tile_page.js:559: * @param {number} width The width. On 2012/08/10 02:25:19, Evan Stade wrote: > in what unit? Done. http://codereview.chromium.org/10829131/diff/7010/chrome/browser/resources/nt... chrome/browser/resources/ntp_search/tile_page.js:584: // TODO(pedrosimonetti): Add contants? Will do in another CL. On 2012/08/10 02:25:19, Evan Stade wrote: > spelling. And yes, please do: it gives you a place to document what these > numbers mean. http://codereview.chromium.org/10829131/diff/7010/chrome/browser/resources/nt... chrome/browser/resources/ntp_search/tile_page.js:625: var tileCell; On 2012/08/10 02:25:19, Evan Stade wrote: > this isn't ansi c. Declare the variables where they are used, as late as > possible. Done. http://codereview.chromium.org/10829131/diff/7010/chrome/browser/resources/nt... chrome/browser/resources/ntp_search/tile_page.js:644: var rowVisible = (row >= pageOffset && On 2012/08/10 02:25:19, Evan Stade wrote: > this paren not needed Done. http://codereview.chromium.org/10829131/diff/7010/chrome/browser/resources/nt... chrome/browser/resources/ntp_search/tile_page.js:673: if (tileCell.firstChild) { On 2012/08/10 02:25:19, Evan Stade wrote: > this is easier to read as > > if (!tileCell.firstChild) > // ... > else if (tileElement != tileCell.firstChild) > // ... Done. http://codereview.chromium.org/10829131/diff/7010/chrome/browser/resources/nt... chrome/browser/resources/ntp_search/tile_page.js:679: } else { On 2012/08/10 02:25:19, Evan Stade wrote: > else if Done. http://codereview.chromium.org/10829131/diff/7010/chrome/browser/resources/nt... chrome/browser/resources/ntp_search/tile_page.js:714: var tileRows = tileGridContent.getElementsByClassName('tile-row'); On 2012/08/10 02:25:19, Evan Stade wrote: > declare where used Done. http://codereview.chromium.org/10829131/diff/7010/chrome/browser/resources/nt... chrome/browser/resources/ntp_search/tile_page.js:730: numOfVisibleRows = 2; On 2012/08/10 02:25:19, Evan Stade wrote: > ternary operator Done. http://codereview.chromium.org/10829131/diff/7010/chrome/browser/resources/nt... chrome/browser/resources/ntp_search/tile_page.js:738: //tileGrid.style.height = (107 * numOfVisibleRows) + 'px'; On 2012/08/10 02:25:19, Evan Stade wrote: > no dead code Done. http://codereview.chromium.org/10829131/diff/7010/chrome/browser/resources/nt... chrome/browser/resources/ntp_search/tile_page.js:743: On 2012/08/10 02:25:19, Evan Stade wrote: > random newline Done. http://codereview.chromium.org/10829131/diff/7010/chrome/browser/resources/nt... chrome/browser/resources/ntp_search/tile_page.js:746: On 2012/08/10 02:25:19, Evan Stade wrote: > random newline Done. http://codereview.chromium.org/10829131/diff/7010/chrome/browser/resources/nt... chrome/browser/resources/ntp_search/tile_page.js:762: On 2012/08/10 02:25:19, Evan Stade wrote: > random newline Done. http://codereview.chromium.org/10829131/diff/7010/chrome/browser/resources/nt... chrome/browser/resources/ntp_search/tile_page.js:876: if (this.tileGridContent_.classList.contains('animate-tile')) { On 2012/08/10 02:25:19, Evan Stade wrote: > rather than if(){ if(){}}, you can use another && I could, but other conditions will be checked inside the outermost if, so if I do what you're saying then I'll have to undo it later. http://codereview.chromium.org/10829131/diff/7010/chrome/browser/resources/nt... chrome/browser/resources/ntp_search/tile_page.js:886: */ On 2012/08/10 02:25:19, Evan Stade wrote: > @param docs, and explain what this is doing. Done. http://codereview.chromium.org/10829131/diff/7010/chrome/browser/resources/nt... chrome/browser/resources/ntp_search/tile_page.js:937: for (var i = 0, l = rules.length; i < l; i++) { On 2012/08/10 02:25:19, Evan Stade wrote: > do not abbreviate length to l Done. http://codereview.chromium.org/10829131/diff/7010/chrome/browser/resources/nt... chrome/browser/resources/ntp_search/tile_page.js:945: * Changes the slow factor for animations. On 2012/08/10 02:25:19, Evan Stade wrote: > is this for testing? if so, say so. > > if you're going to hack a way to slow down animations, why not just add a rule > like > > * { > -webkit-transition-duration: 5s !important; > } > > what you have is fragile and complicated in comparison. I can't do that because it's not guaranteed that all individual animations will have the same duration (currently they have but I haven't implemented all animations yet).
looking better http://codereview.chromium.org/10829131/diff/16002/chrome/browser/resources/n... File chrome/browser/resources/ntp_search/new_tab.css (right): http://codereview.chromium.org/10829131/diff/16002/chrome/browser/resources/n... chrome/browser/resources/ntp_search/new_tab.css:255: opacity: 0 !important; On 2012/08/10 21:28:45, pedrosimonetti wrote: > > there should be no importants. > > If !important is so nasty why do you use it in the NTP4 code? I understand why > using it might affect the readability of code, but I think it's not worth > blocking the NTP5 development for such unimportant (pun intended) thing. In general, if you are using important, (1) it leads to a readability/maintainability nightmare, (2) it's not a sustainable kludge because there's no !important !important (3) it's an indication you haven't chosen good selectors. Style guides are enforced during the review process. If you would like to change this practice, you can start a discussion on chromium-dev. ntp4 has a couple of valid uses of important, i.e. cases where the specificity of the selector is necessarily low, but we need the attribute to take precendence. The other ones probably should not exist. > > Anyway, I removed all !important that are not strictly necessary. The ones that > are still there are necessary to override a property that was set using > JavaScript. > > > Don't commit code you know you are going to > > delete. It makes it impossible to review---how do I know where the "definitely > > will be deleted" code starts or ends? How do I easily follow up to make sure > the > > code does actually get changed soon? > > I was marking all code that we need to delete with comments like > TODO(pedrosimonetti): Delete. So a simply search using grep, ack or your > favorite search tool will easily show you if that code is still there. And since > you're reviewing our NTP code you will know for sure when we change any part of > the code. I guess you are suggesting to keep a list of everything that shouldn't be there and periodically grep until all the items are scratched off my list. This doesn't seem realistic to expect of a reviewer. > > I think it would be much easier to delete each feature at once, because deleting > code is not just a matter of deleting a function and its callers. We need to > make sure that that logic is not necessary anymore, and we need to check in each > caller what was going on, once we might want to replace some code there. http://codereview.chromium.org/10829131/diff/16002/chrome/browser/resources/n... File chrome/browser/resources/ntp_search/page_list_view.js (right): http://codereview.chromium.org/10829131/diff/16002/chrome/browser/resources/n... chrome/browser/resources/ntp_search/page_list_view.js:505: // TODO(xci): delete On 2012/08/10 21:28:45, pedrosimonetti wrote: > On 2012/08/10 02:25:19, Evan Stade wrote: > > On 2012/08/09 21:33:32, pedrosimonetti wrote: > > > On 2012/08/08 23:19:18, Evan Stade wrote: > > > > put the TODO inside the doc block, and why not actually delete the code > > > inside? > > > > > > Done. Because it requires changing other files we're not using right now, so > > > we're gonna do this in another CL. > > > > I don't understand this response. Deleting this newly dead code requires > > changing what file? > > These methods are being used by the new_tab.js file, and it uses code from > apps_page.js which is not included in this CL yet because we agreed to submit > the NTP5 code in smaller pieces. I was asking why you don't delete the contents of the function, not deleting the function itself. http://codereview.chromium.org/10829131/diff/7010/chrome/browser/resources/nt... File chrome/browser/resources/ntp_search/tile_page.js (right): http://codereview.chromium.org/10829131/diff/7010/chrome/browser/resources/nt... chrome/browser/resources/ntp_search/tile_page.js:876: if (this.tileGridContent_.classList.contains('animate-tile')) { On 2012/08/10 21:28:45, pedrosimonetti wrote: > On 2012/08/10 02:25:19, Evan Stade wrote: > > rather than if(){ if(){}}, you can use another && > > I could, but other conditions will be checked inside the outermost if, so if I > do what you're saying then I'll have to undo it later. again, it's impossible to review code based on what it might look like in the future. I can't mark this code down to re-review in 3 weeks time to make sure it looks sane then. If you're checking it in, it should look sane /now/. http://codereview.chromium.org/10829131/diff/7010/chrome/browser/resources/nt... chrome/browser/resources/ntp_search/tile_page.js:945: * Changes the slow factor for animations. On 2012/08/10 21:28:45, pedrosimonetti wrote: > On 2012/08/10 02:25:19, Evan Stade wrote: > > is this for testing? if so, say so. > > > > if you're going to hack a way to slow down animations, why not just add a rule > > like > > > > * { > > -webkit-transition-duration: 5s !important; > > } > > > > what you have is fragile and complicated in comparison. > > I can't do that because it's not guaranteed that all individual animations will > have the same duration (currently they have but I haven't implemented all > animations yet). how is that relevant for testing? http://codereview.chromium.org/10829131/diff/8006/chrome/browser/resources/nt... File chrome/browser/resources/ntp_search/new_tab.css (right): http://codereview.chromium.org/10829131/diff/8006/chrome/browser/resources/nt... chrome/browser/resources/ntp_search/new_tab.css:156: .tile-cell { please give tile_page its own css file. http://codereview.chromium.org/10829131/diff/8006/chrome/browser/resources/nt... chrome/browser/resources/ntp_search/new_tab.css:245: /* Animates entire columns of Tiles at once */ final punctuation http://codereview.chromium.org/10829131/diff/8006/chrome/browser/resources/nt... File chrome/browser/resources/ntp_search/thumbnail_page.js (right): http://codereview.chromium.org/10829131/diff/8006/chrome/browser/resources/nt... chrome/browser/resources/ntp_search/thumbnail_page.js:44: * Thumbnail data object. TODO(pedrosimonetti): How is this being set? to answer your question, look below... http://codereview.chromium.org/10829131/diff/8006/chrome/browser/resources/nt... chrome/browser/resources/ntp_search/thumbnail_page.js:87: // TODO(pedrosimonetti): Do we really need id? id is used for the color stripe calculation. I would recommend keeping the color stripe at least until you make the thumbnail system more reliable. http://codereview.chromium.org/10829131/diff/8006/chrome/browser/resources/nt... chrome/browser/resources/ntp_search/thumbnail_page.js:89: this.data_ = data; this is how it's being set http://codereview.chromium.org/10829131/diff/8006/chrome/browser/resources/nt... chrome/browser/resources/ntp_search/thumbnail_page.js:129: // The horizontal margin of a Tile. this doesn't tell me much. My best guess is that it's the number of pixels between two tiles on the same row. But it could just as easily be half the distance since two tiles might combine margins to get a gap of 36px. I think you should change the name to horizontalSpacing or something to avoid confusion: that way if you decide to change the implementation the name will still make sense. http://codereview.chromium.org/10829131/diff/8006/chrome/browser/resources/nt... chrome/browser/resources/ntp_search/thumbnail_page.js:142: // TODO(pedrosimonetti): Why not always use JavaScript? why not always use CSS? It's preferable to maintain separation of presentation and logic. Almost this whole config struct could just be a block of CSS, right? http://codereview.chromium.org/10829131/diff/8006/chrome/browser/resources/nt... chrome/browser/resources/ntp_search/thumbnail_page.js:184: * Update the tiles afconfigter a change to |data_|. afconfigter? http://codereview.chromium.org/10829131/diff/8006/chrome/browser/resources/nt... File chrome/browser/resources/ntp_search/tile_page.js (right): http://codereview.chromium.org/10829131/diff/8006/chrome/browser/resources/nt... chrome/browser/resources/ntp_search/tile_page.js:54: // TODO(pedrosimonetti): Set using CSS? yes, please do. This will solve your !important problem.
http://codereview.chromium.org/10829131/diff/7010/chrome/browser/resources/nt... File chrome/browser/resources/ntp_search/tile_page.js (right): http://codereview.chromium.org/10829131/diff/7010/chrome/browser/resources/nt... chrome/browser/resources/ntp_search/tile_page.js:876: if (this.tileGridContent_.classList.contains('animate-tile')) { On 2012/08/13 22:05:45, Evan Stade wrote: > On 2012/08/10 21:28:45, pedrosimonetti wrote: > > On 2012/08/10 02:25:19, Evan Stade wrote: > > > rather than if(){ if(){}}, you can use another && > > > > I could, but other conditions will be checked inside the outermost if, so if I > > do what you're saying then I'll have to undo it later. > > again, it's impossible to review code based on what it might look like in the > future. I can't mark this code down to re-review in 3 weeks time to make sure it > looks sane then. If you're checking it in, it should look sane /now/. Ack. http://codereview.chromium.org/10829131/diff/7010/chrome/browser/resources/nt... chrome/browser/resources/ntp_search/tile_page.js:945: * Changes the slow factor for animations. On 2012/08/13 22:05:45, Evan Stade wrote: > On 2012/08/10 21:28:45, pedrosimonetti wrote: > > On 2012/08/10 02:25:19, Evan Stade wrote: > > > is this for testing? if so, say so. > > > > > > if you're going to hack a way to slow down animations, why not just add a > rule > > > like > > > > > > * { > > > -webkit-transition-duration: 5s !important; > > > } > > > > > > what you have is fragile and complicated in comparison. > > > > I can't do that because it's not guaranteed that all individual animations > will > > have the same duration (currently they have but I haven't implemented all > > animations yet). > > how is that relevant for testing? There are several minor nuances in the animation and is practically impossible to tell if all the individual properties are being animated as expected when running at normal speed (~200ms). This code was improved to mock/mock.js where the rest of debugging stuff is located, there's a new way of overriding the animations duration which is less prone to error than the approach below. http://codereview.chromium.org/10829131/diff/8006/chrome/browser/resources/nt... File chrome/browser/resources/ntp_search/new_tab.css (right): http://codereview.chromium.org/10829131/diff/8006/chrome/browser/resources/nt... chrome/browser/resources/ntp_search/new_tab.css:156: .tile-cell { On 2012/08/13 22:05:45, Evan Stade wrote: > please give tile_page its own css file. Done. http://codereview.chromium.org/10829131/diff/8006/chrome/browser/resources/nt... chrome/browser/resources/ntp_search/new_tab.css:245: /* Animates entire columns of Tiles at once */ On 2012/08/13 22:05:45, Evan Stade wrote: > final punctuation Done. http://codereview.chromium.org/10829131/diff/8006/chrome/browser/resources/nt... File chrome/browser/resources/ntp_search/thumbnail_page.js (right): http://codereview.chromium.org/10829131/diff/8006/chrome/browser/resources/nt... chrome/browser/resources/ntp_search/thumbnail_page.js:44: * Thumbnail data object. TODO(pedrosimonetti): How is this being set? On 2012/08/13 22:05:45, Evan Stade wrote: > to answer your question, look below... I guess my comment was too vague. I meant figuring out how the data object is being used across different pages/cases and see if we can minimize the duplication of code. It turns out that this is the problem Jeremy was talking to me and is blocking his work, so I fixed this problem and created a new formatThumbnail_() method. http://codereview.chromium.org/10829131/diff/8006/chrome/browser/resources/nt... chrome/browser/resources/ntp_search/thumbnail_page.js:87: // TODO(pedrosimonetti): Do we really need id? On 2012/08/13 22:05:45, Evan Stade wrote: > id is used for the color stripe calculation. I would recommend keeping the color > stripe at least until you make the thumbnail system more reliable. We have a special treatment when there's no thumbnail image available. We'll show the favicon and the domain. Also, work on improving the thumbnail system has begun. I'm removing this code for now since this is what was defined by the UX folks and approved by the UI leads. http://codereview.chromium.org/10829131/diff/8006/chrome/browser/resources/nt... chrome/browser/resources/ntp_search/thumbnail_page.js:89: this.data_ = data; On 2012/08/13 22:05:45, Evan Stade wrote: > this is how it's being set Ack. http://codereview.chromium.org/10829131/diff/8006/chrome/browser/resources/nt... chrome/browser/resources/ntp_search/thumbnail_page.js:129: // The horizontal margin of a Tile. On 2012/08/13 22:05:45, Evan Stade wrote: > this doesn't tell me much. My best guess is that it's the number of pixels > between two tiles on the same row. But it could just as easily be half the > distance since two tiles might combine margins to get a gap of 36px. > > I think you should change the name to horizontalSpacing or something to avoid > confusion: that way if you decide to change the implementation the name will > still make sense. Renamed tileHorizontalMargin to tileMarginStart which refers to the exact css property, leaving no margin (no pun intended) for multiple interpretations. http://codereview.chromium.org/10829131/diff/8006/chrome/browser/resources/nt... chrome/browser/resources/ntp_search/thumbnail_page.js:142: // TODO(pedrosimonetti): Why not always use JavaScript? On 2012/08/13 22:05:45, Evan Stade wrote: > why not always use CSS? It's preferable to maintain separation of presentation > and logic. Almost this whole config struct could just be a block of CSS, right? We could use CSS to set the size/margin of Tiles, and I did that and removed the need for the reinforceStyles property, and the need of that !important rule regarding tile margin. But we need to keep the config values because the rendering logic depends on those values. http://codereview.chromium.org/10829131/diff/8006/chrome/browser/resources/nt... chrome/browser/resources/ntp_search/thumbnail_page.js:184: * Update the tiles afconfigter a change to |data_|. On 2012/08/13 22:05:45, Evan Stade wrote: > afconfigter? Fixed typo. http://codereview.chromium.org/10829131/diff/8006/chrome/browser/resources/nt... File chrome/browser/resources/ntp_search/tile_page.js (right): http://codereview.chromium.org/10829131/diff/8006/chrome/browser/resources/nt... chrome/browser/resources/ntp_search/tile_page.js:54: // TODO(pedrosimonetti): Set using CSS? On 2012/08/13 22:05:45, Evan Stade wrote: > yes, please do. This will solve your !important problem. Done.
http://codereview.chromium.org/10829131/diff/16002/chrome/browser/resources/n... File chrome/browser/resources/ntp_search/new_tab.css (right): http://codereview.chromium.org/10829131/diff/16002/chrome/browser/resources/n... chrome/browser/resources/ntp_search/new_tab.css:182: -webkit-transition: all 201ms ease-in-out; On 2012/08/10 20:34:53, dcblack wrote: > This is intended from a design point of view. All translate animations should > always be eased. I'm not sure what you mean by "eased". The default transition function is "ease". There is also ease-in, ease-out, and ease-in-out. > > On 2012/08/10 02:25:19, Evan Stade wrote: > > On 2012/08/09 21:33:32, pedrosimonetti wrote: > > > On 2012/08/08 23:19:18, Evan Stade wrote: > > > > why are these all ease-in-out? > > > > > > If you mean "why are these all *ease-in-out*" the spec says so. > > > > I meant this. I don't see where the design doc says ease-in-out in it. Are you > > talking about a live mock? Live mocks are almost always created by an > authoring > > tool which adds random rules like ease-in-out which were not really intended > by > > the author. Can you explain why ease-in-out is preferable over the default? If > > not, you should figure it out before copying it. > > > > > > > > If you mean "why are these *ALL 201ms ease-in-out*", I'll check if we can > add > > > individual properties in another CL. > > > http://codereview.chromium.org/10829131/diff/16005/chrome/browser/resources/n... File chrome/browser/resources/ntp_search/mock/debug.css (right): http://codereview.chromium.org/10829131/diff/16005/chrome/browser/resources/n... chrome/browser/resources/ntp_search/mock/debug.css:70: END OF Work around odd capitalization http://codereview.chromium.org/10829131/diff/16005/chrome/browser/resources/n... File chrome/browser/resources/ntp_search/most_visited_page.js (right): http://codereview.chromium.org/10829131/diff/16005/chrome/browser/resources/n... chrome/browser/resources/ntp_search/most_visited_page.js:113: * Shows the undo notification when blacklisting a most visited site. technically these are supposed to have @private tags, even though it's a bit redundant with the final underscore. http://codereview.chromium.org/10829131/diff/16005/chrome/browser/resources/n... File chrome/browser/resources/ntp_search/page_list_view.js (right): http://codereview.chromium.org/10829131/diff/16005/chrome/browser/resources/n... chrome/browser/resources/ntp_search/page_list_view.js:520: if (!this.pageSwitcherStart || !this.pageSwitcherEnd) I still don't understand why you have kept the remainder of this function body. http://codereview.chromium.org/10829131/diff/16005/chrome/browser/resources/n... File chrome/browser/resources/ntp_search/tile_page.css (right): http://codereview.chromium.org/10829131/diff/16005/chrome/browser/resources/n... chrome/browser/resources/ntp_search/tile_page.css:70: .hide-col-0 .tile-col-0, could you use .tile-cell:nth-of-type(x)? http://codereview.chromium.org/10829131/diff/16005/chrome/browser/resources/n... chrome/browser/resources/ntp_search/tile_page.css:87: } \n http://codereview.chromium.org/10829131/diff/16005/chrome/browser/resources/n... chrome/browser/resources/ntp_search/tile_page.css:91: ^H http://codereview.chromium.org/10829131/diff/16005/chrome/browser/resources/n... File chrome/browser/resources/ntp_search/tile_page.js (right): http://codereview.chromium.org/10829131/diff/16005/chrome/browser/resources/n... chrome/browser/resources/ntp_search/tile_page.js:492: this.fireAddedEvent(tile, index); my eyes might deceive me but it seems like this function is adding the tile twice and firing two events. http://codereview.chromium.org/10829131/diff/16005/chrome/browser/resources/n... chrome/browser/resources/ntp_search/tile_page.js:789: for (var i = 0, l = rows.length; i < l; i++) { don't abbreviate length to l here either http://codereview.chromium.org/10829131/diff/16005/chrome/browser/resources/n... chrome/browser/resources/ntp_search/tile_page.js:798: this.tileGridContent_.style.webkitTransform = 'translate3d(0, ' + nit: keep entire transform string on one line http://codereview.chromium.org/10829131/diff/16005/chrome/browser/resources/n... chrome/browser/resources/ntp_search/tile_page.js:820: this.onTileGridAnimationEndHandler_) { this doesn't seem to have been fixed
http://codereview.chromium.org/10829131/diff/16002/chrome/browser/resources/n... File chrome/browser/resources/ntp_search/new_tab.css (right): http://codereview.chromium.org/10829131/diff/16002/chrome/browser/resources/n... chrome/browser/resources/ntp_search/new_tab.css:182: -webkit-transition: all 201ms ease-in-out; Ah, sorry for the ambiguity. We want either "ease" or "ease-in-out". We should probably play with both to see which looks best. I suspect we'll want "ease".
http://codereview.chromium.org/10829131/diff/16005/chrome/browser/resources/n... File chrome/browser/resources/ntp_search/mock/debug.css (right): http://codereview.chromium.org/10829131/diff/16005/chrome/browser/resources/n... chrome/browser/resources/ntp_search/mock/debug.css:70: END OF Work around On 2012/08/14 23:46:53, Evan Stade wrote: > odd capitalization Done. http://codereview.chromium.org/10829131/diff/16005/chrome/browser/resources/n... File chrome/browser/resources/ntp_search/most_visited_page.js (right): http://codereview.chromium.org/10829131/diff/16005/chrome/browser/resources/n... chrome/browser/resources/ntp_search/most_visited_page.js:113: * Shows the undo notification when blacklisting a most visited site. On 2012/08/14 23:46:53, Evan Stade wrote: > technically these are supposed to have @private tags, even though it's a bit > redundant with the final underscore. Done. http://codereview.chromium.org/10829131/diff/16005/chrome/browser/resources/n... File chrome/browser/resources/ntp_search/page_list_view.js (right): http://codereview.chromium.org/10829131/diff/16005/chrome/browser/resources/n... chrome/browser/resources/ntp_search/page_list_view.js:520: if (!this.pageSwitcherStart || !this.pageSwitcherEnd) On 2012/08/14 23:46:53, Evan Stade wrote: > I still don't understand why you have kept the remainder of this function body. Done. http://codereview.chromium.org/10829131/diff/16005/chrome/browser/resources/n... File chrome/browser/resources/ntp_search/tile_page.css (right): http://codereview.chromium.org/10829131/diff/16005/chrome/browser/resources/n... chrome/browser/resources/ntp_search/tile_page.css:70: .hide-col-0 .tile-col-0, On 2012/08/14 23:46:53, Evan Stade wrote: > could you use .tile-cell:nth-of-type(x)? I guess I could but I don't see what's the value of doing this now. I'm including a TODO to investigate this. http://codereview.chromium.org/10829131/diff/16005/chrome/browser/resources/n... chrome/browser/resources/ntp_search/tile_page.css:87: } On 2012/08/14 23:46:53, Evan Stade wrote: > \n Done. http://codereview.chromium.org/10829131/diff/16005/chrome/browser/resources/n... chrome/browser/resources/ntp_search/tile_page.css:91: On 2012/08/14 23:46:53, Evan Stade wrote: > ^H Done. http://codereview.chromium.org/10829131/diff/16005/chrome/browser/resources/n... File chrome/browser/resources/ntp_search/tile_page.js (right): http://codereview.chromium.org/10829131/diff/16005/chrome/browser/resources/n... chrome/browser/resources/ntp_search/tile_page.js:492: this.fireAddedEvent(tile, index); On 2012/08/14 23:46:53, Evan Stade wrote: > my eyes might deceive me but it seems like this function is adding the tile > twice and firing two events. You're right. I was supposed to call renderGrid_() instead of appendTile(). http://codereview.chromium.org/10829131/diff/16005/chrome/browser/resources/n... chrome/browser/resources/ntp_search/tile_page.js:789: for (var i = 0, l = rows.length; i < l; i++) { On 2012/08/14 23:46:53, Evan Stade wrote: > don't abbreviate length to l here either Done. http://codereview.chromium.org/10829131/diff/16005/chrome/browser/resources/n... chrome/browser/resources/ntp_search/tile_page.js:798: this.tileGridContent_.style.webkitTransform = 'translate3d(0, ' + On 2012/08/14 23:46:53, Evan Stade wrote: > nit: keep entire transform string on one line Done. http://codereview.chromium.org/10829131/diff/16005/chrome/browser/resources/n... chrome/browser/resources/ntp_search/tile_page.js:820: this.onTileGridAnimationEndHandler_) { On 2012/08/14 23:46:53, Evan Stade wrote: > this doesn't seem to have been fixed Done.
http://codereview.chromium.org/10829131/diff/16005/chrome/browser/resources/n... File chrome/browser/resources/ntp_search/tile_page.css (right): http://codereview.chromium.org/10829131/diff/16005/chrome/browser/resources/n... chrome/browser/resources/ntp_search/tile_page.css:70: .hide-col-0 .tile-col-0, On 2012/08/15 18:42:06, pedrosimonetti wrote: > On 2012/08/14 23:46:53, Evan Stade wrote: > > could you use .tile-cell:nth-of-type(x)? > > I guess I could but I don't see what's the value of doing this now. I'm > including a TODO to investigate this. the value is that you can get rid of the function which sets the .tile-col-X class name http://codereview.chromium.org/10829131/diff/16006/chrome/browser/resources/n... File chrome/browser/resources/ntp_search/tile_page.js (right): http://codereview.chromium.org/10829131/diff/16006/chrome/browser/resources/n... chrome/browser/resources/ntp_search/tile_page.js:194: tileHeight: 0, since these are now in CSS, why are they here as well? this doesn't appear to be referenced anywhere. http://codereview.chromium.org/10829131/diff/16006/chrome/browser/resources/n... chrome/browser/resources/ntp_search/tile_page.js:460: numOfVisibleRows: 0, why is this not private? http://codereview.chromium.org/10829131/diff/16006/chrome/browser/resources/n... chrome/browser/resources/ntp_search/tile_page.js:491: this.renderGrid_(); odd indent. http://codereview.chromium.org/10829131/diff/16006/chrome/browser/resources/n... chrome/browser/resources/ntp_search/tile_page.js:501: getNumOfVisibleRows_: function() { prefer getter syntax. http://codereview.chromium.org/10829131/diff/16006/chrome/browser/resources/n... chrome/browser/resources/ntp_search/tile_page.js:541: // TODO(pedrosimonetti): Add contants? this still says contants instead of constants. Why are you punting this for later again? http://codereview.chromium.org/10829131/diff/16006/chrome/browser/resources/n... chrome/browser/resources/ntp_search/tile_page.js:589: tileRow.className = 'tile-row tile-row-' + row; tile-row-X seems like another case where you could probably use nth-of-type instead. http://codereview.chromium.org/10829131/diff/16006/chrome/browser/resources/n... chrome/browser/resources/ntp_search/tile_page.js:630: } else { else if (I mentioned this before I believe) http://codereview.chromium.org/10829131/diff/16006/chrome/browser/resources/n... chrome/browser/resources/ntp_search/tile_page.js:750: var max = 10; // TODO(pedrosimonetti): Add const? two spaces between code and // comments http://codereview.chromium.org/10829131/diff/16006/chrome/browser/resources/n... chrome/browser/resources/ntp_search/tile_page.js:792: this.showTileRow_(row, true); if (x) foo(true); else foo(false); == is better as ==> foo(x); http://codereview.chromium.org/10829131/diff/16006/chrome/browser/resources/n... chrome/browser/resources/ntp_search/tile_page.js:799: 'translate3d(0, ' + (-pageOffset * 105) + 'px, 0)'; I gather that this 105 is the height of a row. You re-use it several times. Shouldn't it be read with getComputedStyle? And is it really supposed to be the same height regardless of the size of the tile? At the very least it should refer to the CSS value it copies, but since you use it many times it seems keeping it up to date in multiple places will be a headache.
http://codereview.chromium.org/10829131/diff/16005/chrome/browser/resources/n... File chrome/browser/resources/ntp_search/tile_page.css (right): http://codereview.chromium.org/10829131/diff/16005/chrome/browser/resources/n... chrome/browser/resources/ntp_search/tile_page.css:70: .hide-col-0 .tile-col-0, On 2012/08/16 00:02:31, Evan Stade wrote: > On 2012/08/15 18:42:06, pedrosimonetti wrote: > > On 2012/08/14 23:46:53, Evan Stade wrote: > > > could you use .tile-cell:nth-of-type(x)? > > > > I guess I could but I don't see what's the value of doing this now. I'm > > including a TODO to investigate this. > > the value is that you can get rid of the function which sets the .tile-col-X > class name Yeah, that's definitely a good idea. Thanks for sharing! At the same time, I think this is not the most relevant issue right now. We still have a lot of things to do, and besides Jeremy and myself, there are now 3 other engineers working on the NTP. So we need to submit this CL ASAP, and it's not worth nit picking these issues right now. We can polish up those details later, after addressing the critical pieces of code that are still missing, without blocking other engineers. http://codereview.chromium.org/10829131/diff/16006/chrome/browser/resources/n... File chrome/browser/resources/ntp_search/tile_page.js (right): http://codereview.chromium.org/10829131/diff/16006/chrome/browser/resources/n... chrome/browser/resources/ntp_search/tile_page.js:194: tileHeight: 0, On 2012/08/16 00:02:31, Evan Stade wrote: > since these are now in CSS, why are they here as well? this doesn't appear to be > referenced anywhere. Done. http://codereview.chromium.org/10829131/diff/16006/chrome/browser/resources/n... chrome/browser/resources/ntp_search/tile_page.js:460: numOfVisibleRows: 0, On 2012/08/16 00:02:31, Evan Stade wrote: > why is this not private? Done. http://codereview.chromium.org/10829131/diff/16006/chrome/browser/resources/n... chrome/browser/resources/ntp_search/tile_page.js:491: this.renderGrid_(); On 2012/08/16 00:02:31, Evan Stade wrote: > odd indent. Done. http://codereview.chromium.org/10829131/diff/16006/chrome/browser/resources/n... chrome/browser/resources/ntp_search/tile_page.js:501: getNumOfVisibleRows_: function() { On 2012/08/16 00:02:31, Evan Stade wrote: > prefer getter syntax. And I think that using functions is clearer. Added a comment to consider doing this. http://codereview.chromium.org/10829131/diff/16006/chrome/browser/resources/n... chrome/browser/resources/ntp_search/tile_page.js:541: // TODO(pedrosimonetti): Add contants? On 2012/08/16 00:02:31, Evan Stade wrote: > this still says contants instead of constants. Why are you punting this for > later again? The resizing logic will change a little bit compared to this implementation because of the way we'll be using WebView to display the bottom panel. We are calculating the size of the Bottom Section according to the size of the window. But in the new implementation, the WebView that will hold the Bottom Section has a smaller size, so we need to re-think how to implement this function. So I need to sync up with the WebView work. Also, at the same time, I think this is not the most relevant issue right now. We still have a lot of things to do, and besides Jeremy and myself, there are now 3 other engineers working on the NTP. So we need to submit this CL ASAP, and it's not worth nit picking these issues right now. We can polish up those details later, after addressing the critical pieces of code that are still missing, without blocking other engineers. http://codereview.chromium.org/10829131/diff/16006/chrome/browser/resources/n... chrome/browser/resources/ntp_search/tile_page.js:589: tileRow.className = 'tile-row tile-row-' + row; On 2012/08/16 00:02:31, Evan Stade wrote: > tile-row-X seems like another case where you could probably use nth-of-type > instead. I think we don't need to use nth-of-type() here. We need to read all rows, so we can individually hide rows that are not visible (this happens in the paginate_ function). http://codereview.chromium.org/10829131/diff/16006/chrome/browser/resources/n... chrome/browser/resources/ntp_search/tile_page.js:630: } else { On 2012/08/16 00:02:31, Evan Stade wrote: > else if (I mentioned this before I believe) Done. http://codereview.chromium.org/10829131/diff/16006/chrome/browser/resources/n... chrome/browser/resources/ntp_search/tile_page.js:750: var max = 10; // TODO(pedrosimonetti): Add const? On 2012/08/16 00:02:31, Evan Stade wrote: > two spaces between code and // comments Done. http://codereview.chromium.org/10829131/diff/16006/chrome/browser/resources/n... chrome/browser/resources/ntp_search/tile_page.js:792: this.showTileRow_(row, true); On 2012/08/16 00:02:31, Evan Stade wrote: > if (x) > foo(true); > else > foo(false); > > == is better as ==> > > foo(x); Good point. Done. http://codereview.chromium.org/10829131/diff/16006/chrome/browser/resources/n... chrome/browser/resources/ntp_search/tile_page.js:799: 'translate3d(0, ' + (-pageOffset * 105) + 'px, 0)'; On 2012/08/16 00:02:31, Evan Stade wrote: > I gather that this 105 is the height of a row. You re-use it several times. > Shouldn't it be read with getComputedStyle? And is it really supposed to be the > same height regardless of the size of the tile? At the very least it should > refer to the CSS value it copies, but since you use it many times it seems > keeping it up to date in multiple places will be a headache. Makes sense. I'm using a function now to get the size of a row. This height will be always the same, and that's why I'm using a fixed value instead of calling getComputedStyle() to get it.
http://codereview.chromium.org/10829131/diff/16005/chrome/browser/resources/n... File chrome/browser/resources/ntp_search/tile_page.css (right): http://codereview.chromium.org/10829131/diff/16005/chrome/browser/resources/n... chrome/browser/resources/ntp_search/tile_page.css:70: .hide-col-0 .tile-col-0, On 2012/08/16 00:53:19, pedrosimonetti wrote: > On 2012/08/16 00:02:31, Evan Stade wrote: > > On 2012/08/15 18:42:06, pedrosimonetti wrote: > > > On 2012/08/14 23:46:53, Evan Stade wrote: > > > > could you use .tile-cell:nth-of-type(x)? > > > > > > I guess I could but I don't see what's the value of doing this now. I'm > > > including a TODO to investigate this. > > > > the value is that you can get rid of the function which sets the .tile-col-X > > class name > > Yeah, that's definitely a good idea. Thanks for sharing! At the same time, I > think this is not the most relevant issue right now. We still have a lot of > things to do, and besides Jeremy and myself, there are now 3 other engineers > working on the NTP. So we need to submit this CL ASAP, and it's not worth nit > picking these issues right now. > > We can polish up those details later, after addressing the critical pieces of > code that are still missing, without blocking other engineers. I feel like I've repeated this a number of times, but once more: if you are putting this code through the review process then it's going to get a full review. If you don't want this code to be reviewed then you can do something else, perhaps develop it in your own private repository which you share with the developers you are blocking. It shouldn't be hard either way for other developers to continue working; I frequently make changes that depend on code which has not yet been committed, written by myself or others. Git is great for managing that workflow. Besides that, arguing over whether procrastination is ok takes longer than just fixing it to the way we both agree it should be. http://codereview.chromium.org/10829131/diff/16006/chrome/browser/resources/n... File chrome/browser/resources/ntp_search/tile_page.js (right): http://codereview.chromium.org/10829131/diff/16006/chrome/browser/resources/n... chrome/browser/resources/ntp_search/tile_page.js:501: getNumOfVisibleRows_: function() { On 2012/08/16 00:53:19, pedrosimonetti wrote: > On 2012/08/16 00:02:31, Evan Stade wrote: > > prefer getter syntax. > > And I think that using functions is clearer. Added a comment to consider doing > this. sorry, I didn't mean "I prefer". I meant that WebUI as a whole prefers it. "Use ES5 getters and setters" http://dev.chromium.org/developers/web-development-style-guide http://codereview.chromium.org/10829131/diff/16006/chrome/browser/resources/n... chrome/browser/resources/ntp_search/tile_page.js:589: tileRow.className = 'tile-row tile-row-' + row; On 2012/08/16 00:53:19, pedrosimonetti wrote: > On 2012/08/16 00:02:31, Evan Stade wrote: > > tile-row-X seems like another case where you could probably use nth-of-type > > instead. > > I think we don't need to use nth-of-type() here. We need to read all rows, so we > can individually hide rows that are not visible (this happens in the paginate_ > function). Now that I look, I see that tile-row-X classes aren't actually used anywhere, so just remove them. http://codereview.chromium.org/10829131/diff/57/chrome/browser/resources/ntp_... File chrome/browser/resources/ntp_search/tile_page.js (right): http://codereview.chromium.org/10829131/diff/57/chrome/browser/resources/ntp_... chrome/browser/resources/ntp_search/tile_page.js:507: return 105; nit: add comments that point out this is shared in the css (so if someone tries to change it it's clear there's another place it's also stored) http://codereview.chromium.org/10829131/diff/57/chrome/browser/resources/ntp_... chrome/browser/resources/ntp_search/tile_page.js:550: // TODO(pedrosimonetti): Add contants? please fix this typo. http://codereview.chromium.org/10829131/diff/57/chrome/browser/resources/ntp_... chrome/browser/resources/ntp_search/tile_page.js:691: $('page-list').style.height = (this.getRowHeight_() * numOfVisibleRows) + 'px'; 80 http://codereview.chromium.org/10829131/diff/57/chrome/browser/resources/ntp_... chrome/browser/resources/ntp_search/tile_page.js:801: var isRowVisible = i >= pageOffset && i <= (pageOffset + numOfVisibleRows - 1); 80
http://codereview.chromium.org/10829131/diff/16005/chrome/browser/resources/n... File chrome/browser/resources/ntp_search/tile_page.css (right): http://codereview.chromium.org/10829131/diff/16005/chrome/browser/resources/n... chrome/browser/resources/ntp_search/tile_page.css:70: .hide-col-0 .tile-col-0, On 2012/08/16 02:57:00, Evan Stade wrote: > On 2012/08/16 00:53:19, pedrosimonetti wrote: > > On 2012/08/16 00:02:31, Evan Stade wrote: > > > On 2012/08/15 18:42:06, pedrosimonetti wrote: > > > > On 2012/08/14 23:46:53, Evan Stade wrote: > > > > > could you use .tile-cell:nth-of-type(x)? > > > > > > > > I guess I could but I don't see what's the value of doing this now. I'm > > > > including a TODO to investigate this. > > > > > > the value is that you can get rid of the function which sets the .tile-col-X > > > class name > > > > Yeah, that's definitely a good idea. Thanks for sharing! At the same time, I > > think this is not the most relevant issue right now. We still have a lot of > > things to do, and besides Jeremy and myself, there are now 3 other engineers > > working on the NTP. So we need to submit this CL ASAP, and it's not worth nit > > picking these issues right now. > > > > We can polish up those details later, after addressing the critical pieces of > > code that are still missing, without blocking other engineers. > > I feel like I've repeated this a number of times, but once more: if you are > putting this code through the review process then it's going to get a full > review. If you don't want this code to be reviewed then you can do something > else, perhaps develop it in your own private repository which you share with the > developers you are blocking. It shouldn't be hard either way for other > developers to continue working; I frequently make changes that depend on code > which has not yet been committed, written by myself or others. Git is great for > managing that workflow. > > Besides that, arguing over whether procrastination is ok takes longer than just > fixing it to the way we both agree it should be. Of course I want this code to be reviewed. At the same time, making such change in the CSS is an optimization, and it's definitely not critical to change it now. This feature in particular (animation of columns) is working as expected, and is performing well. We can make it slightly better by doing that, and I just did what you suggested, but the most important thing now is moving forward with this CL. You have my word that I'll address all the remaining issues we discussed so far. I'm not being lazy, and I want this code to be as perfect as possible, but as I said earlier I'm worried because of others that are depending on this. http://codereview.chromium.org/10829131/diff/16006/chrome/browser/resources/n... File chrome/browser/resources/ntp_search/tile_page.js (right): http://codereview.chromium.org/10829131/diff/16006/chrome/browser/resources/n... chrome/browser/resources/ntp_search/tile_page.js:501: getNumOfVisibleRows_: function() { On 2012/08/16 02:57:00, Evan Stade wrote: > On 2012/08/16 00:53:19, pedrosimonetti wrote: > > On 2012/08/16 00:02:31, Evan Stade wrote: > > > prefer getter syntax. > > > > And I think that using functions is clearer. Added a comment to consider doing > > this. > > sorry, I didn't mean "I prefer". I meant that WebUI as a whole prefers it. "Use > ES5 getters and setters" > http://dev.chromium.org/developers/web-development-style-guide Since the property numOfVisibleRows_ is only being used internally by the TilePage itself, I think it makes more sense reading this value directly, the same we do for other internal properties like pageOffset_, rowCount_, colCount_, etc. http://codereview.chromium.org/10829131/diff/16006/chrome/browser/resources/n... chrome/browser/resources/ntp_search/tile_page.js:589: tileRow.className = 'tile-row tile-row-' + row; On 2012/08/16 02:57:00, Evan Stade wrote: > On 2012/08/16 00:53:19, pedrosimonetti wrote: > > On 2012/08/16 00:02:31, Evan Stade wrote: > > > tile-row-X seems like another case where you could probably use nth-of-type > > > instead. > > > > I think we don't need to use nth-of-type() here. We need to read all rows, so > we > > can individually hide rows that are not visible (this happens in the paginate_ > > function). > > Now that I look, I see that tile-row-X classes aren't actually used anywhere, so > just remove them. Done. http://codereview.chromium.org/10829131/diff/57/chrome/browser/resources/ntp_... File chrome/browser/resources/ntp_search/tile_page.js (right): http://codereview.chromium.org/10829131/diff/57/chrome/browser/resources/ntp_... chrome/browser/resources/ntp_search/tile_page.js:507: return 105; On 2012/08/16 02:57:00, Evan Stade wrote: > nit: add comments that point out this is shared in the css (so if someone tries > to change it it's clear there's another place it's also stored) On a second thought I think that it makes more sense putting this value inside the config object, where the size of cells and rows of the "grid" are defined. http://codereview.chromium.org/10829131/diff/57/chrome/browser/resources/ntp_... chrome/browser/resources/ntp_search/tile_page.js:550: // TODO(pedrosimonetti): Add contants? On 2012/08/16 02:57:00, Evan Stade wrote: > please fix this typo. Done. http://codereview.chromium.org/10829131/diff/57/chrome/browser/resources/ntp_... chrome/browser/resources/ntp_search/tile_page.js:691: $('page-list').style.height = (this.getRowHeight_() * numOfVisibleRows) + 'px'; On 2012/08/16 02:57:00, Evan Stade wrote: > 80 Done. http://codereview.chromium.org/10829131/diff/57/chrome/browser/resources/ntp_... chrome/browser/resources/ntp_search/tile_page.js:801: var isRowVisible = i >= pageOffset && i <= (pageOffset + numOfVisibleRows - 1); On 2012/08/16 02:57:00, Evan Stade wrote: > 80 Done.
lgtm with nits http://codereview.chromium.org/10829131/diff/16005/chrome/browser/resources/n... File chrome/browser/resources/ntp_search/tile_page.css (right): http://codereview.chromium.org/10829131/diff/16005/chrome/browser/resources/n... chrome/browser/resources/ntp_search/tile_page.css:70: .hide-col-0 .tile-col-0, On 2012/08/17 18:04:11, pedrosimonetti wrote: > On 2012/08/16 02:57:00, Evan Stade wrote: > > On 2012/08/16 00:53:19, pedrosimonetti wrote: > > > On 2012/08/16 00:02:31, Evan Stade wrote: > > > > On 2012/08/15 18:42:06, pedrosimonetti wrote: > > > > > On 2012/08/14 23:46:53, Evan Stade wrote: > > > > > > could you use .tile-cell:nth-of-type(x)? > > > > > > > > > > I guess I could but I don't see what's the value of doing this now. I'm > > > > > including a TODO to investigate this. > > > > > > > > the value is that you can get rid of the function which sets the > .tile-col-X > > > > class name > > > > > > Yeah, that's definitely a good idea. Thanks for sharing! At the same time, I > > > think this is not the most relevant issue right now. We still have a lot of > > > things to do, and besides Jeremy and myself, there are now 3 other engineers > > > working on the NTP. So we need to submit this CL ASAP, and it's not worth > nit > > > picking these issues right now. > > > > > > We can polish up those details later, after addressing the critical pieces > of > > > code that are still missing, without blocking other engineers. > > > > I feel like I've repeated this a number of times, but once more: if you are > > putting this code through the review process then it's going to get a full > > review. If you don't want this code to be reviewed then you can do something > > else, perhaps develop it in your own private repository which you share with > the > > developers you are blocking. It shouldn't be hard either way for other > > developers to continue working; I frequently make changes that depend on code > > which has not yet been committed, written by myself or others. Git is great > for > > managing that workflow. > > > > Besides that, arguing over whether procrastination is ok takes longer than > just > > fixing it to the way we both agree it should be. > > Of course I want this code to be reviewed. At the same time, making such change > in the CSS is an optimization, and it's definitely not critical to change it > now. if this code had already been committed and I were asking you to change it, I can see how you might want to postpone that. But since this is new code that you are adding, if there's a better way to do it, you should get it right the first time. > This feature in particular (animation of columns) is working as expected, > and is performing well. We can make it slightly better by doing that, and I just > did what you suggested, but the most important thing now is moving forward with > this CL. You have my word that I'll address all the remaining issues we > discussed so far. I'm not being lazy, and I want this code to be as perfect as > possible, but as I said earlier I'm worried because of others that are depending > on this. http://codereview.chromium.org/10829131/diff/16007/chrome/browser/resources/n... File chrome/browser/resources/ntp_search/most_visited_page.js (left): http://codereview.chromium.org/10829131/diff/16007/chrome/browser/resources/n... chrome/browser/resources/ntp_search/most_visited_page.js:180: text: loadTimeData.getString('undothumbnailremove'), nit: leave the extra comma http://codereview.chromium.org/10829131/diff/16007/chrome/browser/resources/n... chrome/browser/resources/ntp_search/most_visited_page.js:187: text: loadTimeData.getString('restoreThumbnailsShort'), nit: leave the extra comma http://codereview.chromium.org/10829131/diff/16007/chrome/browser/resources/n... chrome/browser/resources/ntp_search/most_visited_page.js:367: }, nit: leave the extra comma http://codereview.chromium.org/10829131/diff/16007/chrome/browser/resources/n... chrome/browser/resources/ntp_search/most_visited_page.js:466: refreshData: refreshData, nit: leave the extra comma http://codereview.chromium.org/10829131/diff/16007/chrome/browser/resources/n... File chrome/browser/resources/ntp_search/tile_page.css (right): http://codereview.chromium.org/10829131/diff/16007/chrome/browser/resources/n... chrome/browser/resources/ntp_search/tile_page.css:34: -webkit-transform: translate3d(0,0,0); spaces after commas http://codereview.chromium.org/10829131/diff/16007/chrome/browser/resources/n... File chrome/browser/resources/ntp_search/tile_page.js (right): http://codereview.chromium.org/10829131/diff/16007/chrome/browser/resources/n... chrome/browser/resources/ntp_search/tile_page.js:58: tearDown_: function() { this is dead code right? it's not actually called AFAICT, and it has no body anyway. Please remove it and add when actually needed. http://codereview.chromium.org/10829131/diff/16007/chrome/browser/resources/n... chrome/browser/resources/ntp_search/tile_page.js:153: tearDown_: function() { ditto
http://codereview.chromium.org/10829131/diff/16005/chrome/browser/resources/n... File chrome/browser/resources/ntp_search/tile_page.css (right): http://codereview.chromium.org/10829131/diff/16005/chrome/browser/resources/n... chrome/browser/resources/ntp_search/tile_page.css:70: .hide-col-0 .tile-col-0, On 2012/08/17 20:20:31, Evan Stade wrote: > On 2012/08/17 18:04:11, pedrosimonetti wrote: > > On 2012/08/16 02:57:00, Evan Stade wrote: > > > On 2012/08/16 00:53:19, pedrosimonetti wrote: > > > > On 2012/08/16 00:02:31, Evan Stade wrote: > > > > > On 2012/08/15 18:42:06, pedrosimonetti wrote: > > > > > > On 2012/08/14 23:46:53, Evan Stade wrote: > > > > > > > could you use .tile-cell:nth-of-type(x)? > > > > > > > > > > > > I guess I could but I don't see what's the value of doing this now. > I'm > > > > > > including a TODO to investigate this. > > > > > > > > > > the value is that you can get rid of the function which sets the > > .tile-col-X > > > > > class name > > > > > > > > Yeah, that's definitely a good idea. Thanks for sharing! At the same time, > I > > > > think this is not the most relevant issue right now. We still have a lot > of > > > > things to do, and besides Jeremy and myself, there are now 3 other > engineers > > > > working on the NTP. So we need to submit this CL ASAP, and it's not worth > > nit > > > > picking these issues right now. > > > > > > > > We can polish up those details later, after addressing the critical pieces > > of > > > > code that are still missing, without blocking other engineers. > > > > > > I feel like I've repeated this a number of times, but once more: if you are > > > putting this code through the review process then it's going to get a full > > > review. If you don't want this code to be reviewed then you can do something > > > else, perhaps develop it in your own private repository which you share with > > the > > > developers you are blocking. It shouldn't be hard either way for other > > > developers to continue working; I frequently make changes that depend on > code > > > which has not yet been committed, written by myself or others. Git is great > > for > > > managing that workflow. > > > > > > Besides that, arguing over whether procrastination is ok takes longer than > > just > > > fixing it to the way we both agree it should be. > > > > Of course I want this code to be reviewed. At the same time, making such > change > > in the CSS is an optimization, and it's definitely not critical to change it > > now. > > if this code had already been committed and I were asking you to change it, I > can see how you might want to postpone that. But since this is new code that you > are adding, if there's a better way to do it, you should get it right the first > time. > > > This feature in particular (animation of columns) is working as expected, > > and is performing well. We can make it slightly better by doing that, and I > just > > did what you suggested, but the most important thing now is moving forward > with > > this CL. You have my word that I'll address all the remaining issues we > > discussed so far. I'm not being lazy, and I want this code to be as perfect as > > possible, but as I said earlier I'm worried because of others that are > depending > > on this. > Okay. The changes are already in place. http://codereview.chromium.org/10829131/diff/16007/chrome/browser/resources/n... File chrome/browser/resources/ntp_search/most_visited_page.js (left): http://codereview.chromium.org/10829131/diff/16007/chrome/browser/resources/n... chrome/browser/resources/ntp_search/most_visited_page.js:180: text: loadTimeData.getString('undothumbnailremove'), On 2012/08/17 20:20:32, Evan Stade wrote: > nit: leave the extra comma Done. http://codereview.chromium.org/10829131/diff/16007/chrome/browser/resources/n... chrome/browser/resources/ntp_search/most_visited_page.js:187: text: loadTimeData.getString('restoreThumbnailsShort'), On 2012/08/17 20:20:32, Evan Stade wrote: > nit: leave the extra comma Done. http://codereview.chromium.org/10829131/diff/16007/chrome/browser/resources/n... chrome/browser/resources/ntp_search/most_visited_page.js:367: }, On 2012/08/17 20:20:32, Evan Stade wrote: > nit: leave the extra comma Done. http://codereview.chromium.org/10829131/diff/16007/chrome/browser/resources/n... chrome/browser/resources/ntp_search/most_visited_page.js:466: refreshData: refreshData, On 2012/08/17 20:20:32, Evan Stade wrote: > nit: leave the extra comma Done. http://codereview.chromium.org/10829131/diff/16007/chrome/browser/resources/n... File chrome/browser/resources/ntp_search/tile_page.css (right): http://codereview.chromium.org/10829131/diff/16007/chrome/browser/resources/n... chrome/browser/resources/ntp_search/tile_page.css:34: -webkit-transform: translate3d(0,0,0); On 2012/08/17 20:20:32, Evan Stade wrote: > spaces after commas Done. http://codereview.chromium.org/10829131/diff/16007/chrome/browser/resources/n... File chrome/browser/resources/ntp_search/tile_page.js (right): http://codereview.chromium.org/10829131/diff/16007/chrome/browser/resources/n... chrome/browser/resources/ntp_search/tile_page.js:58: tearDown_: function() { On 2012/08/17 20:20:32, Evan Stade wrote: > this is dead code right? it's not actually called AFAICT, and it has no body > anyway. Please remove it and add when actually needed. Done. http://codereview.chromium.org/10829131/diff/16007/chrome/browser/resources/n... chrome/browser/resources/ntp_search/tile_page.js:153: tearDown_: function() { On 2012/08/17 20:20:32, Evan Stade wrote: > ditto Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/pedrosimonetti@google.com/10829131/17035
Failed to request the patch to try. Please note that binary filesare still unsupported at the moment, this is being worked on. Thanks for your patience. HTTP Error 404: Not Found
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/pedrosimonetti@google.com/10829131/21001
Presubmit check for 10829131-21001 failed and returned exit status 1. Running presubmit commit checks ... ** Presubmit Warnings ** Found lines longer than 80 characters (first 5 shown). chrome/browser/resources/ntp_search/mock/mockData.js, line 20, 87 chars \ chrome/browser/resources/ntp_search/mock/mockData.js, line 22, 88 chars \ chrome/browser/resources/ntp_search/mock/mockData.js, line 59, 97 chars \ chrome/browser/resources/ntp_search/mock/mockData.js, line 71, 101 chars \ chrome/browser/resources/ntp_search/mock/mockData.js, line 101, 103 chars Presubmit checks took 3.4s to calculate. Was the presubmit check useful? Please send feedback & hate mail to maruel@chromium.org!
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/pedrosimonetti@google.com/10829131/21001
Presubmit check for 10829131-21001 failed and returned exit status 1. Running presubmit commit checks ... ** Presubmit Warnings ** Found lines longer than 80 characters (first 5 shown). chrome/browser/resources/ntp_search/mock/mockData.js, line 20, 87 chars \ chrome/browser/resources/ntp_search/mock/mockData.js, line 22, 88 chars \ chrome/browser/resources/ntp_search/mock/mockData.js, line 59, 97 chars \ chrome/browser/resources/ntp_search/mock/mockData.js, line 71, 101 chars \ chrome/browser/resources/ntp_search/mock/mockData.js, line 101, 103 chars Presubmit checks took 7.7s to calculate. Was the presubmit check useful? Please send feedback & hate mail to maruel@chromium.org!
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/pedrosimonetti@google.com/10829131/18007
Try job failure for 10829131-18007 (retry) on win_rel for step "interactive_ui_tests". It's a second try, previously, steps "interactive_ui_tests, browser_tests" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/pedrosimonetti@google.com/10829131/18007
Change committed as 152540 |
