[ntp4] Bookmarks page implementation, first-pass.

[ntp4] Bookmarks page implementation, first-pass.

This is an initial implementation of the bookmarks page for NTP4. This is a work in progress with the following known issues: 
* Editing is not supported (add, edit, delete, re-order). 
* The parent hierarchy currently doesn't scroll if it gets too long. 
* Changes to the bookmarks model is not currently observed. 
* The page navigation buttons on the page edges currently overlap the titles bar. 
* The vertical scrollbar currently renders over the titles bar. 
* Drag & drop is not currently supported.
* Changes will be required for full Theme compatibility.

BUG=92921
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=96867

[csilv, 2011-08-12 00:10:09]

+estade for review

[Evan Stade, 2011-08-12 21:13:30]

http://codereview.chromium.org/7610014/diff/7016/chrome/browser/resources/ntp...
File chrome/browser/resources/ntp4/bookmarks_page.js (right):

http://codereview.chromium.org/7610014/diff/7016/chrome/browser/resources/ntp...
chrome/browser/resources/ntp4/bookmarks_page.js:9: var tileID = 0;
comment this

http://codereview.chromium.org/7610014/diff/7016/chrome/browser/resources/ntp...
chrome/browser/resources/ntp4/bookmarks_page.js:32: '<a class="button
bookmark-fills-parent">' +
perhaps you could put this html in new_tab.html as a hidden template node and
then clone it. I don't have a particular preference for this approach but some
others apparently do.

(I should update most visited too)

http://codereview.chromium.org/7610014/diff/7016/chrome/browser/resources/ntp...
chrome/browser/resources/ntp4/bookmarks_page.js:41: this.setAttribute('id',
'tile' + id);
I think you can just do this.id =

(again, regardless of what most visited does)

however this is going to conflict with most visited IDs (which are also id +
number)

http://codereview.chromium.org/7610014/diff/7016/chrome/browser/resources/ntp...
chrome/browser/resources/ntp4/bookmarks_page.js:56:
this.setStripeColor(this.data.faviconDominantColor);
add TODOs for the parts of the code that don't actually work yet (makes it
easier to review)

http://codereview.chromium.org/7610014/diff/7016/chrome/browser/resources/ntp...
chrome/browser/resources/ntp4/bookmarks_page.js:147: this.innerText =
localStrings.getString('bookmarksPage');
ternary operator

http://codereview.chromium.org/7610014/diff/7016/chrome/browser/resources/ntp...
File chrome/browser/resources/ntp4/new_tab.js (right):

http://codereview.chromium.org/7610014/diff/7016/chrome/browser/resources/ntp...
chrome/browser/resources/ntp4/new_tab.js:193: chrome.send('getBookmarksData',
['0']);
you should make it pass no arguments for the root folder instead of assuming
knowledge of the root id being 0.

Also, you can pass numbers rather than strings as parameters (on the other side
you have to GetDouble and cast to int)

http://codereview.chromium.org/7610014/diff/7016/chrome/browser/resources/ntp...
chrome/browser/resources/ntp4/new_tab.js:714: setBookmarksData:
setBookmarksData,
can you alphabetize this list

[arv (Not doing code reviews), 2011-08-13 01:02:07]

Nice.

My main concern is that we will have to duplicate almost all of the bookmarks
extension API here. This was one of the reasons why we ended up doing the
bookmark manager as a component extension instead of a DOMUI.

http://codereview.chromium.org/7610014/diff/7016/chrome/browser/resources/ntp...
File chrome/browser/resources/ntp4/bookmarks_page.js (right):

http://codereview.chromium.org/7610014/diff/7016/chrome/browser/resources/ntp...
chrome/browser/resources/ntp4/bookmarks_page.js:9: var tileID = 0;
tileId for consistency

http://codereview.chromium.org/7610014/diff/7016/chrome/browser/resources/ntp...
chrome/browser/resources/ntp4/bookmarks_page.js:56:
this.setStripeColor(this.data.faviconDominantColor);
Use a setter whenever you see a setFoo.

http://codereview.chromium.org/7610014/diff/7016/chrome/browser/resources/ntp...
chrome/browser/resources/ntp4/bookmarks_page.js:62:
this.setStripeColor('#919191');
The icon for the bookmark bar folder is different on different platforms...
should the stripe take that into account?

http://codereview.chromium.org/7610014/diff/7016/chrome/browser/resources/ntp...
chrome/browser/resources/ntp4/bookmarks_page.js:100:
chrome.send('getBookmarksData', [ this.data.id ]);
no ws after [ nor before ]

http://codereview.chromium.org/7610014/diff/7016/chrome/browser/resources/ntp...
chrome/browser/resources/ntp4/bookmarks_page.js:118: this.data_ = data;
No need for a getter or setter her since there are no side effects.

http://codereview.chromium.org/7610014/diff/7016/chrome/browser/resources/ntp...
chrome/browser/resources/ntp4/bookmarks_page.js:147: this.innerText =
localStrings.getString('bookmarksPage');
Use textContent instead of innerText

http://codereview.chromium.org/7610014/diff/7016/chrome/browser/resources/ntp...
chrome/browser/resources/ntp4/bookmarks_page.js:151:
this.addEventListener('click', this.handleClick_.bind(this));
no need for this bind since you are adding the event listener to 'this' already.

http://codereview.chromium.org/7610014/diff/7016/chrome/browser/resources/ntp...
chrome/browser/resources/ntp4/bookmarks_page.js:160:
chrome.send('getBookmarksData', [ this.folderId_ ]);
ws

http://codereview.chromium.org/7610014/diff/7016/chrome/browser/resources/ntp...
chrome/browser/resources/ntp4/bookmarks_page.js:200:
titleWrapper.setAttribute('id', 'bookmarks_title_wrapper');
Prefer DOM bindings over setAttribute

titleWrapper.id = 'bookmarks_title_wrapper';

http://codereview.chromium.org/7610014/diff/7016/chrome/browser/resources/ntp...
chrome/browser/resources/ntp4/bookmarks_page.js:219: var title =
wrapper.getElementsByClassName('section-title')[0];
querySelector('.section-title')

http://codereview.chromium.org/7610014/diff/7016/chrome/browser/resources/ntp...
chrome/browser/resources/ntp4/bookmarks_page.js:226: var separator =
document.createElement('div');
maybe use an hr here?

http://codereview.chromium.org/7610014/diff/7016/chrome/browser/resources/ntp...
chrome/browser/resources/ntp4/bookmarks_page.js:270: var height =
parseInt(wrapperStyle.height);
always use two args to parseInt due to its strange behavior where it prefers to
parse things as octal.

http://codereview.chromium.org/7610014/diff/7016/chrome/browser/resources/ntp...
File chrome/browser/resources/ntp4/new_tab.js (right):

http://codereview.chromium.org/7610014/diff/7016/chrome/browser/resources/ntp...
chrome/browser/resources/ntp4/new_tab.js:193: chrome.send('getBookmarksData',
['0']);
On 2011/08/12 21:13:30, Evan Stade wrote:
> you should make it pass no arguments for the root folder instead of assuming
> knowledge of the root id being 0.
> 
> Also, you can pass numbers rather than strings as parameters (on the other
side
> you have to GetDouble and cast to int)

Bookmark IDs are int64 so this needs to be done as strings.

http://codereview.chromium.org/7610014/diff/7016/chrome/browser/resources/ntp...
File chrome/browser/resources/ntp4/tile_page.js (right):

http://codereview.chromium.org/7610014/diff/7016/chrome/browser/resources/ntp...
chrome/browser/resources/ntp4/tile_page.js:506: while
(this.tileElements_.length) {
why not use?

this.tileGrid_.innerHTML = '';

[csilv, 2011-08-13 01:09:20]

http://codereview.chromium.org/7610014/diff/7016/chrome/browser/resources/ntp...
File chrome/browser/resources/ntp4/bookmarks_page.js (right):

http://codereview.chromium.org/7610014/diff/7016/chrome/browser/resources/ntp...
chrome/browser/resources/ntp4/bookmarks_page.js:9: var tileID = 0;
On 2011/08/12 21:13:30, Evan Stade wrote:
> comment this

Done.

http://codereview.chromium.org/7610014/diff/7016/chrome/browser/resources/ntp...
chrome/browser/resources/ntp4/bookmarks_page.js:32: '<a class="button
bookmark-fills-parent">' +
On 2011/08/12 21:13:30, Evan Stade wrote:
> perhaps you could put this html in new_tab.html as a hidden template node and
> then clone it. I don't have a particular preference for this approach but some
> others apparently do.
> 
> (I should update most visited too)

Oh, that's a great idea!  Done.  Also did the same for the bookmarks page titles
header below.

http://codereview.chromium.org/7610014/diff/7016/chrome/browser/resources/ntp...
chrome/browser/resources/ntp4/bookmarks_page.js:41: this.setAttribute('id',
'tile' + id);
On 2011/08/12 21:13:30, Evan Stade wrote:
> I think you can just do this.id =
> 
> (again, regardless of what most visited does)
> 
> however this is going to conflict with most visited IDs (which are also id +
> number)

Done.  I also made updates so that the dominant color can be fetched for both
Most Visited and Bookmarks independently.  So the conflicting IDs are now
resolved.

http://codereview.chromium.org/7610014/diff/7016/chrome/browser/resources/ntp...
chrome/browser/resources/ntp4/bookmarks_page.js:56:
this.setStripeColor(this.data.faviconDominantColor);
On 2011/08/12 21:13:30, Evan Stade wrote:
> add TODOs for the parts of the code that don't actually work yet (makes it
> easier to review)

Done.  This is now implemented, all other missing parts should be commented with
TODOs.

http://codereview.chromium.org/7610014/diff/7016/chrome/browser/resources/ntp...
chrome/browser/resources/ntp4/bookmarks_page.js:147: this.innerText =
localStrings.getString('bookmarksPage');
On 2011/08/12 21:13:30, Evan Stade wrote:
> ternary operator

Done.

http://codereview.chromium.org/7610014/diff/7016/chrome/browser/resources/ntp...
File chrome/browser/resources/ntp4/new_tab.js (right):

http://codereview.chromium.org/7610014/diff/7016/chrome/browser/resources/ntp...
chrome/browser/resources/ntp4/new_tab.js:193: chrome.send('getBookmarksData',
['0']);
On 2011/08/12 21:13:30, Evan Stade wrote:
> you should make it pass no arguments for the root folder instead of assuming
> knowledge of the root id being 0.
> 
> Also, you can pass numbers rather than strings as parameters (on the other
side
> you have to GetDouble and cast to int)

Done.

http://codereview.chromium.org/7610014/diff/7016/chrome/browser/resources/ntp...
chrome/browser/resources/ntp4/new_tab.js:714: setBookmarksData:
setBookmarksData,
On 2011/08/12 21:13:30, Evan Stade wrote:
> can you alphabetize this list

Done.

[csilv, 2011-08-13 01:48:17]

http://codereview.chromium.org/7610014/diff/7016/chrome/browser/resources/ntp...
File chrome/browser/resources/ntp4/bookmarks_page.js (right):

http://codereview.chromium.org/7610014/diff/7016/chrome/browser/resources/ntp...
chrome/browser/resources/ntp4/bookmarks_page.js:9: var tileID = 0;
On 2011/08/13 01:02:07, arv wrote:
> tileId for consistency

Done.

http://codereview.chromium.org/7610014/diff/7016/chrome/browser/resources/ntp...
chrome/browser/resources/ntp4/bookmarks_page.js:56:
this.setStripeColor(this.data.faviconDominantColor);
On 2011/08/13 01:02:07, arv wrote:
> Use a setter whenever you see a setFoo.

Done.

http://codereview.chromium.org/7610014/diff/7016/chrome/browser/resources/ntp...
chrome/browser/resources/ntp4/bookmarks_page.js:62:
this.setStripeColor('#919191');
Perhaps, right now it's just using a neutral color for folders.  I've added a
TODO here so we can figure that out.

http://codereview.chromium.org/7610014/diff/7016/chrome/browser/resources/ntp...
chrome/browser/resources/ntp4/bookmarks_page.js:100:
chrome.send('getBookmarksData', [ this.data.id ]);
On 2011/08/13 01:02:07, arv wrote:
> no ws after [ nor before ]

Done.

http://codereview.chromium.org/7610014/diff/7016/chrome/browser/resources/ntp...
chrome/browser/resources/ntp4/bookmarks_page.js:118: this.data_ = data;
On 2011/08/13 01:02:07, arv wrote:
> No need for a getter or setter her since there are no side effects.

Done.

http://codereview.chromium.org/7610014/diff/7016/chrome/browser/resources/ntp...
chrome/browser/resources/ntp4/bookmarks_page.js:147: this.innerText =
localStrings.getString('bookmarksPage');
On 2011/08/13 01:02:07, arv wrote:
> Use textContent instead of innerText

Done.

http://codereview.chromium.org/7610014/diff/7016/chrome/browser/resources/ntp...
chrome/browser/resources/ntp4/bookmarks_page.js:151:
this.addEventListener('click', this.handleClick_.bind(this));
On 2011/08/13 01:02:07, arv wrote:
> no need for this bind since you are adding the event listener to 'this'
already.

Done.

http://codereview.chromium.org/7610014/diff/7016/chrome/browser/resources/ntp...
chrome/browser/resources/ntp4/bookmarks_page.js:160:
chrome.send('getBookmarksData', [ this.folderId_ ]);
On 2011/08/13 01:02:07, arv wrote:
> ws

Done.

http://codereview.chromium.org/7610014/diff/7016/chrome/browser/resources/ntp...
chrome/browser/resources/ntp4/bookmarks_page.js:200:
titleWrapper.setAttribute('id', 'bookmarks_title_wrapper');
On 2011/08/13 01:02:07, arv wrote:
> Prefer DOM bindings over setAttribute
> 
> titleWrapper.id = 'bookmarks_title_wrapper';

Done.

http://codereview.chromium.org/7610014/diff/7016/chrome/browser/resources/ntp...
chrome/browser/resources/ntp4/bookmarks_page.js:219: var title =
wrapper.getElementsByClassName('section-title')[0];
On 2011/08/13 01:02:07, arv wrote:
> querySelector('.section-title')

Done.

http://codereview.chromium.org/7610014/diff/7016/chrome/browser/resources/ntp...
chrome/browser/resources/ntp4/bookmarks_page.js:226: var separator =
document.createElement('div');
On 2011/08/13 01:02:07, arv wrote:
> maybe use an hr here?

Done.

http://codereview.chromium.org/7610014/diff/7016/chrome/browser/resources/ntp...
chrome/browser/resources/ntp4/bookmarks_page.js:270: var height =
parseInt(wrapperStyle.height);
On 2011/08/13 01:02:07, arv wrote:
> always use two args to parseInt due to its strange behavior where it prefers
to
> parse things as octal.

Done.

http://codereview.chromium.org/7610014/diff/7016/chrome/browser/resources/ntp...
File chrome/browser/resources/ntp4/tile_page.js (right):

http://codereview.chromium.org/7610014/diff/7016/chrome/browser/resources/ntp...
chrome/browser/resources/ntp4/tile_page.js:506: while
(this.tileElements_.length) {
On 2011/08/13 01:02:07, arv wrote:
> why not use?
> 
> this.tileGrid_.innerHTML = '';

I've changed this since this is going to be more efficient.  This assumes that
we'll only put tile elements into the grid, Evan, any concerns about this?

[csilv, 2011-08-13 01:49:34]

evan, arv,
Replied to all review comments, ptal at your convenience.  Initiating trybot
runs as well.

[csilv, 2011-08-15 17:26:57]

On 2011/08/13 01:02:07, arv wrote:
> My main concern is that we will have to duplicate almost all of the bookmarks
> extension API here. This was one of the reasons why we ended up doing the
> bookmark manager as a component extension instead of a DOMUI.

Agreed, this is a downside.  I did make use of one of the bookmark extension
helper class, but ultimately there's going to be some code duplication.  I think
we should consider transforming NTP into an extension to gain the benefit of
code re-use, but I don't think we can do this for this revision.

[arv (Not doing code reviews), 2011-08-15 17:46:30]

I'm still curious how you plan to tackle the issue where we will need to
duplicate a lot of the extensions bookmark APIs?

http://codereview.chromium.org/7610014/diff/17003/chrome/browser/resources/nt...
File chrome/browser/resources/ntp4/bookmarks_page.css (right):

http://codereview.chromium.org/7610014/diff/17003/chrome/browser/resources/nt...
chrome/browser/resources/ntp4/bookmarks_page.css:99: #bookmarks_title_wrapper {
Use dash-separated-ids

http://codereview.chromium.org/7610014/diff/17003/chrome/browser/resources/nt...
File chrome/browser/resources/ntp4/most_visited_page.js (right):

http://codereview.chromium.org/7610014/diff/17003/chrome/browser/resources/nt...
chrome/browser/resources/ntp4/most_visited_page.js:87: this.id =
'most_visited_tile_' + id;
most-visited-tile

http://codereview.chromium.org/7610014/diff/17003/chrome/browser/ui/webui/ntp...
File chrome/browser/ui/webui/ntp/bookmarks_handler.cc (right):

http://codereview.chromium.org/7610014/diff/17003/chrome/browser/ui/webui/ntp...
chrome/browser/ui/webui/ntp/bookmarks_handler.cc:74:
prefs->RegisterInt64Pref(prefs::kNTPShownBookmarksFolder, 0,
Add a comment that 0 is the ID of the bookmark root?

Maybe the default should be the bookmark bar?

http://codereview.chromium.org/7610014/diff/17003/chrome/browser/ui/webui/ntp...
File chrome/browser/ui/webui/ntp/new_tab_ui.cc (right):

http://codereview.chromium.org/7610014/diff/17003/chrome/browser/ui/webui/ntp...
chrome/browser/ui/webui/ntp/new_tab_ui.cc:349: if (Ntp4Enabled())
Ntp -> NTP in C++ code

[arv (Not doing code reviews), 2011-08-15 17:48:08]

I see that you responded to the code duplication question.

LGTM once the naming nits are fixed.

[csilv, 2011-08-15 18:24:28]

http://codereview.chromium.org/7610014/diff/17003/chrome/browser/resources/nt...
File chrome/browser/resources/ntp4/bookmarks_page.css (right):

http://codereview.chromium.org/7610014/diff/17003/chrome/browser/resources/nt...
chrome/browser/resources/ntp4/bookmarks_page.css:99: #bookmarks_title_wrapper {
On 2011/08/15 17:46:30, arv wrote:
> Use dash-separated-ids

Done.

http://codereview.chromium.org/7610014/diff/17003/chrome/browser/resources/nt...
File chrome/browser/resources/ntp4/most_visited_page.js (right):

http://codereview.chromium.org/7610014/diff/17003/chrome/browser/resources/nt...
chrome/browser/resources/ntp4/most_visited_page.js:87: this.id =
'most_visited_tile_' + id;
On 2011/08/15 17:46:30, arv wrote:
> most-visited-tile

Done.

http://codereview.chromium.org/7610014/diff/17003/chrome/browser/ui/webui/ntp...
File chrome/browser/ui/webui/ntp/bookmarks_handler.cc (right):

http://codereview.chromium.org/7610014/diff/17003/chrome/browser/ui/webui/ntp...
chrome/browser/ui/webui/ntp/bookmarks_handler.cc:74:
prefs->RegisterInt64Pref(prefs::kNTPShownBookmarksFolder, 0,
On 2011/08/15 17:46:30, arv wrote:
> Add a comment that 0 is the ID of the bookmark root?
> 
> Maybe the default should be the bookmark bar?

Added a comment.  Bookmarks bar may be the logical default.  I'll bring that up
in our next meeting.

http://codereview.chromium.org/7610014/diff/17003/chrome/browser/ui/webui/ntp...
File chrome/browser/ui/webui/ntp/new_tab_ui.cc (right):

http://codereview.chromium.org/7610014/diff/17003/chrome/browser/ui/webui/ntp...
chrome/browser/ui/webui/ntp/new_tab_ui.cc:349: if (Ntp4Enabled())
On 2011/08/15 17:46:30, arv wrote:
> Ntp -> NTP in C++ code 

Done.

[Evan Stade, 2011-08-15 22:15:17]

lgtm

http://codereview.chromium.org/7610014/diff/12018/chrome/browser/resources/nt...
File chrome/browser/resources/ntp4/bookmarks_page.js (right):

http://codereview.chromium.org/7610014/diff/12018/chrome/browser/resources/nt...
chrome/browser/resources/ntp4/bookmarks_page.js:178: var titleWrapper =
$('bookmarks-title-wrapper-template').cloneNode(true);
if the title div is unique (seems it is), then instead of cloning you can
reparent

http://codereview.chromium.org/7610014/diff/12018/chrome/browser/resources/nt...
chrome/browser/resources/ntp4/bookmarks_page.js:190: var title =
wrapper.querySelector('.section-title')
;

http://codereview.chromium.org/7610014/diff/12018/chrome/browser/resources/nt...
chrome/browser/resources/ntp4/bookmarks_page.js:195:
title.appendChild(titleCrumb);
combine L194-195

http://codereview.chromium.org/7610014/diff/12018/chrome/browser/resources/nt...
chrome/browser/resources/ntp4/bookmarks_page.js:248: ctx.lineTo(0, height);
prefer to use image mask + themed bg color as discussed offline. doesn't have to
block this patch landing though.

http://codereview.chromium.org/7610014/diff/12018/chrome/browser/resources/nt...
chrome/browser/resources/ntp4/bookmarks_page.js:254: * from a request made when
the tile was created.
document params

http://codereview.chromium.org/7610014/diff/12018/chrome/browser/resources/nt...
File chrome/browser/resources/ntp4/most_visited_page.js (right):

http://codereview.chromium.org/7610014/diff/12018/chrome/browser/resources/nt...
chrome/browser/resources/ntp4/most_visited_page.js:99: else
curlies

[Evan Stade, 2011-08-15 22:23:17]

re: NTP as extension. We also want this so that extensions can modify the NTP
(not just replace it). However I think it will be a large undertaking to make
this switch and to harden our code against security threats.

[csilv, 2011-08-15 22:50:13]

http://codereview.chromium.org/7610014/diff/12018/chrome/browser/resources/nt...
File chrome/browser/resources/ntp4/bookmarks_page.js (right):

http://codereview.chromium.org/7610014/diff/12018/chrome/browser/resources/nt...
chrome/browser/resources/ntp4/bookmarks_page.js:178: var titleWrapper =
$('bookmarks-title-wrapper-template').cloneNode(true);
On 2011/08/15 22:15:17, Evan Stade wrote:
> if the title div is unique (seems it is), then instead of cloning you can
> reparent

Done.

http://codereview.chromium.org/7610014/diff/12018/chrome/browser/resources/nt...
chrome/browser/resources/ntp4/bookmarks_page.js:190: var title =
wrapper.querySelector('.section-title')
On 2011/08/15 22:15:17, Evan Stade wrote:
> ;

Done.

http://codereview.chromium.org/7610014/diff/12018/chrome/browser/resources/nt...
chrome/browser/resources/ntp4/bookmarks_page.js:195:
title.appendChild(titleCrumb);
On 2011/08/15 22:15:17, Evan Stade wrote:
> combine L194-195

Done.

http://codereview.chromium.org/7610014/diff/12018/chrome/browser/resources/nt...
chrome/browser/resources/ntp4/bookmarks_page.js:254: * from a request made when
the tile was created.
On 2011/08/15 22:15:17, Evan Stade wrote:
> document params

Done.

http://codereview.chromium.org/7610014/diff/12018/chrome/browser/resources/nt...
File chrome/browser/resources/ntp4/most_visited_page.js (right):

http://codereview.chromium.org/7610014/diff/12018/chrome/browser/resources/nt...
chrome/browser/resources/ntp4/most_visited_page.js:99: else
On 2011/08/15 22:15:17, Evan Stade wrote:
> curlies

Done.

[Sheridan Rawlins, 2011-08-16 05:50:27]

Drive by review - mostly style nits, but a few questions/concerns.

http://codereview.chromium.org/7610014/diff/12042/chrome/browser/resources/nt...
File chrome/browser/resources/ntp4/bookmarks_page.js (right):

http://codereview.chromium.org/7610014/diff/12042/chrome/browser/resources/nt...
chrome/browser/resources/ntp4/bookmarks_page.js:12: * a unique ID.
nit: @type {number}

http://codereview.chromium.org/7610014/diff/12042/chrome/browser/resources/nt...
chrome/browser/resources/ntp4/bookmarks_page.js:34: initialize: function() {
nit: JSDoc (@inheritDoc?)

http://codereview.chromium.org/7610014/diff/12042/chrome/browser/resources/nt...
chrome/browser/resources/ntp4/bookmarks_page.js:76: *     animate.
Sorry, what does "animate" mean?

http://codereview.chromium.org/7610014/diff/12042/chrome/browser/resources/nt...
chrome/browser/resources/ntp4/bookmarks_page.js:125: initialize: function(data)
{
nit: JSDoc (@inheritDoc ?)

http://codereview.chromium.org/7610014/diff/12042/chrome/browser/resources/nt...
chrome/browser/resources/ntp4/bookmarks_page.js:131:
this.addEventListener('click', this.handleClick_);
Will this work, or does it need to be "bound"?  It looks like the method relies
on |this| - will |this| be set properly without .bind() here?

http://codereview.chromium.org/7610014/diff/12042/chrome/browser/resources/nt...
chrome/browser/resources/ntp4/bookmarks_page.js:190: for (var i = items.length -
1; i > 0; i--) {
nit: --i

http://codereview.chromium.org/7610014/diff/12042/chrome/browser/resources/nt...
chrome/browser/resources/ntp4/bookmarks_page.js:210: for (var i = 0; i <
items.length; i++)
nit: ++i

[Evan Stade, 2011-08-16 06:05:38]

http://codereview.chromium.org/7610014/diff/12042/chrome/browser/resources/nt...
File chrome/browser/resources/ntp4/bookmarks_page.js (right):

http://codereview.chromium.org/7610014/diff/12042/chrome/browser/resources/nt...
chrome/browser/resources/ntp4/bookmarks_page.js:190: for (var i = items.length -
1; i > 0; i--) {
On 2011/08/16 05:50:28, Sheridan Rawlins wrote:
> nit: --i

why? I don't see any mention of post vs pre increment on either the internal js
style guide or the external chrome web development guide but the google C++
style guide says "For simple scalar (non-object) values there is no reason to
prefer one form and we allow either"

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml

[Sheridan Rawlins, 2011-08-16 06:39:13]

http://codereview.chromium.org/7610014/diff/12042/chrome/browser/resources/nt...
File chrome/browser/resources/ntp4/bookmarks_page.js (right):

http://codereview.chromium.org/7610014/diff/12042/chrome/browser/resources/nt...
chrome/browser/resources/ntp4/bookmarks_page.js:190: for (var i = items.length -
1; i > 0; i--) {
I would have thought that getting in the habit of preinc/dec was preferred. 
Thanks for the correction and reference.

On 2011/08/16 06:05:38, Evan Stade wrote:
> On 2011/08/16 05:50:28, Sheridan Rawlins wrote:
> > nit: --i
> 
> why? I don't see any mention of post vs pre increment on either the internal
js
> style guide or the external chrome web development guide but the google C++
> style guide says "For simple scalar (non-object) values there is no reason to
> prefer one form and we allow either"
> 
> http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml

http://codereview.chromium.org/7610014/diff/12042/chrome/browser/resources/nt...
chrome/browser/resources/ntp4/bookmarks_page.js:210: for (var i = 0; i <
items.length; i++)
On 2011/08/16 05:50:28, Sheridan Rawlins wrote:
> nit: ++i

Please ignore

[arv (Not doing code reviews), 2011-08-16 19:36:55]

http://codereview.chromium.org/7610014/diff/12042/chrome/browser/resources/nt...
File chrome/browser/resources/ntp4/bookmarks_page.css (right):

http://codereview.chromium.org/7610014/diff/12042/chrome/browser/resources/nt...
chrome/browser/resources/ntp4/bookmarks_page.css:122: margin-right: 0;
-webkit-margin-end?

http://codereview.chromium.org/7610014/diff/12042/chrome/browser/resources/nt...
chrome/browser/resources/ntp4/bookmarks_page.css:123: padding-right: 10px;
-webkit-padding-end?

http://codereview.chromium.org/7610014/diff/12042/chrome/browser/resources/nt...
chrome/browser/resources/ntp4/bookmarks_page.css:128: left: 0;
rtl?

http://codereview.chromium.org/7610014/diff/12042/chrome/browser/resources/nt...
File chrome/browser/resources/ntp4/bookmarks_page.js (right):

http://codereview.chromium.org/7610014/diff/12042/chrome/browser/resources/nt...
chrome/browser/resources/ntp4/bookmarks_page.js:34: initialize: function() {
On 2011/08/16 05:50:28, Sheridan Rawlins wrote:
> nit: JSDoc (@inheritDoc?)

HTMLDivElement does not have initialize so this should not be @inheritDoc.

http://codereview.chromium.org/7610014/diff/12042/chrome/browser/resources/nt...
chrome/browser/resources/ntp4/bookmarks_page.js:131:
this.addEventListener('click', this.handleClick_);
On 2011/08/16 05:50:28, Sheridan Rawlins wrote:
> Will this work, or does it need to be "bound"?  It looks like the method
relies
> on |this| - will |this| be set properly without .bind() here?

It will work since addEventListener is called on this and the callbacks will use
that as the receiver.

http://codereview.chromium.org/7610014/diff/12042/chrome/browser/resources/nt...
chrome/browser/resources/ntp4/bookmarks_page.js:190: for (var i = items.length -
1; i > 0; i--) {
On 2011/08/16 06:39:13, Sheridan Rawlins wrote:
> I would have thought that getting in the habit of preinc/dec was preferred. 
> Thanks for the correction and reference.
> 
> On 2011/08/16 06:05:38, Evan Stade wrote:
> > On 2011/08/16 05:50:28, Sheridan Rawlins wrote:
> > > nit: --i
> > 
> > why? I don't see any mention of post vs pre increment on either the internal
> js
> > style guide or the external chrome web development guide but the google C++
> > style guide says "For simple scalar (non-object) values there is no reason
to
> > prefer one form and we allow either"
> > 
> > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml
> 

All the Google JS uses post inc/dec for consistency.