Refactoring the NTP.

Refactoring the NTP.


BUG=

[jeremycho_google, 2012-07-31 03:09:15]

Some initial comments - I'll take another look once the files are branched.

The high-level issue I saw was that there seems to be MostVisited-specific code
in the Thumbnail classes.

https://chromiumcodereview.appspot.com/10823052/diff/1/chrome/browser/resourc...
File chrome/browser/resources/ntp_search/mock/mock.js (right):

https://chromiumcodereview.appspot.com/10823052/diff/1/chrome/browser/resourc...
chrome/browser/resources/ntp_search/mock/mock.js:89: mockUrl =
thumbnailUrlList.splice(index, 0);
What is this for?  Shouldn't splice(index, 0) always return []?

https://chromiumcodereview.appspot.com/10823052/diff/1/chrome/browser/resourc...
chrome/browser/resources/ntp_search/mock/mock.js:162: souldCallChromeSend =
souldCallChromeSend || shouldRegisterData;
should

https://chromiumcodereview.appspot.com/10823052/diff/1/chrome/browser/resourc...
File chrome/browser/resources/ntp_search/mock/mockData.js (right):

https://chromiumcodereview.appspot.com/10823052/diff/1/chrome/browser/resourc...
chrome/browser/resources/ntp_search/mock/mockData.js:49: "url":
"http://www.youtube.com/"
Drop the 'www' for all of these to match mockedThumbnailUrls.

https://chromiumcodereview.appspot.com/10823052/diff/1/chrome/browser/resourc...
File chrome/browser/resources/ntp_search/mock/mockOld.js (right):

https://chromiumcodereview.appspot.com/10823052/diff/1/chrome/browser/resourc...
chrome/browser/resources/ntp_search/mock/mockOld.js:1: ntpMock = {};
Is this file still needed?

https://chromiumcodereview.appspot.com/10823052/diff/1/chrome/browser/resourc...
File chrome/browser/resources/ntp_search/most_visited_page.js (right):

https://chromiumcodereview.appspot.com/10823052/diff/1/chrome/browser/resourc...
chrome/browser/resources/ntp_search/most_visited_page.js:16: function
MostVisited() {
Pass in gridValues?

https://chromiumcodereview.appspot.com/10823052/diff/1/chrome/browser/resourc...
chrome/browser/resources/ntp_search/most_visited_page.js:16: function
MostVisited() {
This doesn't seem to get called.

https://chromiumcodereview.appspot.com/10823052/diff/1/chrome/browser/resourc...
chrome/browser/resources/ntp_search/most_visited_page.js:46: set data(data) {
Is it possible to avoid duplicating this from Thumbnail Page?

https://chromiumcodereview.appspot.com/10823052/diff/1/chrome/browser/resourc...
File chrome/browser/resources/ntp_search/new_tab.js (right):

https://chromiumcodereview.appspot.com/10823052/diff/1/chrome/browser/resourc...
chrome/browser/resources/ntp_search/new_tab.js:596: getThumbnailUrl:
getThumbnailUrl,
Alphabetize.

https://chromiumcodereview.appspot.com/10823052/diff/1/chrome/browser/resourc...
File chrome/browser/resources/ntp_search/page_list_view.js (right):

https://chromiumcodereview.appspot.com/10823052/diff/1/chrome/browser/resourc...
chrome/browser/resources/ntp_search/page_list_view.js:2: // Use of this source
code is governed by a BSD-style license that can be
I don't see a diff for this file.

https://chromiumcodereview.appspot.com/10823052/diff/1/chrome/browser/resourc...
File chrome/browser/resources/ntp_search/thumbnail_page.js (right):

https://chromiumcodereview.appspot.com/10823052/diff/1/chrome/browser/resourc...
chrome/browser/resources/ntp_search/thumbnail_page.js:17: * Creates a new Most
Visited object for tiling.
Rename references to Most Visited in the comments (here and elsewhere)

https://chromiumcodereview.appspot.com/10823052/diff/1/chrome/browser/resourc...
chrome/browser/resources/ntp_search/thumbnail_page.js:164: * Allow blacklisting
most visited site using the keyboard.
Is MostVisited the only type of tile that will be removable?  If so, move this
to MostVisited (as well as the other blacklist-specific logic), and delete the
isRemovable parameter?

https://chromiumcodereview.appspot.com/10823052/diff/1/chrome/browser/resourc...
chrome/browser/resources/ntp_search/thumbnail_page.js:245: var THUMBNAIL_COUNT =
8; // TODO(xci)
I still see the bug where 10 thumbnails are being shown, even if only 8 exist. 
Are you planning to address this in a later CL?

https://chromiumcodereview.appspot.com/10823052/diff/1/chrome/browser/resourc...
chrome/browser/resources/ntp_search/thumbnail_page.js:273: reinforceStyles:
true,
Please document.

https://chromiumcodereview.appspot.com/10823052/diff/1/chrome/browser/resourc...
chrome/browser/resources/ntp_search/thumbnail_page.js:284:
this.classList.add('most-visited-page');
Should this be renamed, given that other classes might extend Thumbnail besides
MostVisited?

https://chromiumcodereview.appspot.com/10823052/diff/1/chrome/browser/resourc...
File chrome/browser/resources/ntp_search/tile_page.js (right):

https://chromiumcodereview.appspot.com/10823052/diff/1/chrome/browser/resourc...
chrome/browser/resources/ntp_search/tile_page.js:607: handleCardSelection_:
function(e) {
This doesn't seem to get called when switching to Most visited, only when
switching to Apps.

[pedrosimonetti2, 2012-08-03 18:14:01]

Jeremy, I have implemented the changes you suggested. Please take another look.

http://codereview.chromium.org/10823052/diff/1/chrome/browser/resources/ntp_s...
File chrome/browser/resources/ntp_search/mock/mock.js (right):

http://codereview.chromium.org/10823052/diff/1/chrome/browser/resources/ntp_s...
chrome/browser/resources/ntp_search/mock/mock.js:89: mockUrl =
thumbnailUrlList.splice(index, 0);
Fixed.

On 2012/07/31 03:09:16, jeremycho wrote:
> What is this for?  Shouldn't splice(index, 0) always return []?

http://codereview.chromium.org/10823052/diff/1/chrome/browser/resources/ntp_s...
chrome/browser/resources/ntp_search/mock/mock.js:162: souldCallChromeSend =
souldCallChromeSend || shouldRegisterData;
On 2012/07/31 03:09:16, jeremycho wrote:
> should

Done.

http://codereview.chromium.org/10823052/diff/1/chrome/browser/resources/ntp_s...
File chrome/browser/resources/ntp_search/mock/mockData.js (right):

http://codereview.chromium.org/10823052/diff/1/chrome/browser/resources/ntp_s...
chrome/browser/resources/ntp_search/mock/mockData.js:49: "url":
"http://www.youtube.com/"
See comment on the other CL.

On 2012/07/31 03:09:16, jeremycho wrote:
> Drop the 'www' for all of these to match mockedThumbnailUrls.

http://codereview.chromium.org/10823052/diff/1/chrome/browser/resources/ntp_s...
File chrome/browser/resources/ntp_search/mock/mockOld.js (right):

http://codereview.chromium.org/10823052/diff/1/chrome/browser/resources/ntp_s...
chrome/browser/resources/ntp_search/mock/mockOld.js:1: ntpMock = {};
No. This was the old version of the
Super-Awesome-Mock-Tool-No-More-Compiling-Madness (TM). It's sad, but I deleted
this file :)

On 2012/07/31 03:09:16, jeremycho wrote:
> Is this file still needed?

http://codereview.chromium.org/10823052/diff/1/chrome/browser/resources/ntp_s...
File chrome/browser/resources/ntp_search/most_visited_page.js (right):

http://codereview.chromium.org/10823052/diff/1/chrome/browser/resources/ntp_s...
chrome/browser/resources/ntp_search/most_visited_page.js:16: function
MostVisited() {
On 2012/07/31 03:09:16, jeremycho wrote:
> Pass in gridValues?

Done. See comments on the new CL.

http://codereview.chromium.org/10823052/diff/1/chrome/browser/resources/ntp_s...
chrome/browser/resources/ntp_search/most_visited_page.js:16: function
MostVisited() {
On 2012/07/31 03:09:16, jeremycho wrote:
> This doesn't seem to get called.

Done.

http://codereview.chromium.org/10823052/diff/1/chrome/browser/resources/ntp_s...
chrome/browser/resources/ntp_search/most_visited_page.js:46: set data(data) {
I replaced the code from ThumbnailPage.prototype.set data() with an error
message because that's a virtual setter.

On 2012/07/31 03:09:16, jeremycho wrote:
> Is it possible to avoid duplicating this from Thumbnail Page?

http://codereview.chromium.org/10823052/diff/1/chrome/browser/resources/ntp_s...
File chrome/browser/resources/ntp_search/new_tab.js (right):

http://codereview.chromium.org/10823052/diff/1/chrome/browser/resources/ntp_s...
chrome/browser/resources/ntp_search/new_tab.js:596: getThumbnailUrl:
getThumbnailUrl,
On 2012/07/31 03:09:16, jeremycho wrote:
> Alphabetize.

Done.

http://codereview.chromium.org/10823052/diff/1/chrome/browser/resources/ntp_s...
File chrome/browser/resources/ntp_search/page_list_view.js (right):

http://codereview.chromium.org/10823052/diff/1/chrome/browser/resources/ntp_s...
chrome/browser/resources/ntp_search/page_list_view.js:2: // Use of this source
code is governed by a BSD-style license that can be
I think they were identical.

On 2012/07/31 03:09:16, jeremycho wrote:
> I don't see a diff for this file.

http://codereview.chromium.org/10823052/diff/1/chrome/browser/resources/ntp_s...
File chrome/browser/resources/ntp_search/thumbnail_page.js (right):

http://codereview.chromium.org/10823052/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/07/31 03:09:16, jeremycho wrote:
> Rename references to Most Visited in the comments (here and elsewhere)

Done.

http://codereview.chromium.org/10823052/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/07/31 03:09:16, jeremycho wrote:
> Is MostVisited the only type of tile that will be removable?  If so, move this
> to MostVisited (as well as the other blacklist-specific logic), and delete the
> isRemovable parameter?

Done.

http://codereview.chromium.org/10823052/diff/1/chrome/browser/resources/ntp_s...
chrome/browser/resources/ntp_search/thumbnail_page.js:245: var THUMBNAIL_COUNT =
8; // TODO(xci)
Yes, I'll address it in another CL. I took another look at it and it won't be
that simple.

On 2012/07/31 03:09:16, jeremycho wrote:
> I still see the bug where 10 thumbnails are being shown, even if only 8 exist.

> Are you planning to address this in a later CL?

http://codereview.chromium.org/10823052/diff/1/chrome/browser/resources/ntp_s...
chrome/browser/resources/ntp_search/thumbnail_page.js:273: reinforceStyles:
true,
On 2012/07/31 03:09:16, jeremycho wrote:
> Please document.

Done.

http://codereview.chromium.org/10823052/diff/1/chrome/browser/resources/ntp_s...
chrome/browser/resources/ntp_search/thumbnail_page.js:284:
this.classList.add('most-visited-page');
Yes. I renamed it to 'thumbnail-page' and overrided the
MostVisited.prototype.initialize to include the 'most-visited' class too.

On 2012/07/31 03:09:16, jeremycho wrote:
> Should this be renamed, given that other classes might extend Thumbnail
besides
> MostVisited?

http://codereview.chromium.org/10823052/diff/1/chrome/browser/resources/ntp_s...
File chrome/browser/resources/ntp_search/tile_page.js (right):

http://codereview.chromium.org/10823052/diff/1/chrome/browser/resources/ntp_s...
chrome/browser/resources/ntp_search/tile_page.js:607: handleCardSelection_:
function(e) {
Done. In the newly revised code this method is being called as expected.

On 2012/07/31 03:09:16, jeremycho wrote:
> This doesn't seem to get called when switching to Most visited, only when
> switching to Apps.