[ntp4] Observe and process bookmark change notifications from the bookmarks data model.

[ntp4] Observe and process bookmark change notifications from the bookmarks data model.

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

[csilv, 2011-08-23 17:07:09]

+estade for review

[Evan Stade, 2011-08-23 21:50:50]

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

http://codereview.chromium.org/7713026/diff/6002/chrome/browser/resources/ntp...
chrome/browser/resources/ntp4/bookmarks_page.js:219: bookmarkImportBegan:
function() {
why is this here? why can't you just handle it in C++?

http://codereview.chromium.org/7713026/diff/6002/chrome/browser/resources/ntp...
chrome/browser/resources/ntp4/bookmarks_page.js:229: bookmarkNodeAdded:
function(id, bookmark) {
document each function

http://codereview.chromium.org/7713026/diff/6002/chrome/browser/resources/ntp...
chrome/browser/resources/ntp4/bookmarks_page.js:231: if (this.id ==
bookmark.parentId) {
no curlies

http://codereview.chromium.org/7713026/diff/6002/chrome/browser/resources/ntp...
chrome/browser/resources/ntp4/bookmarks_page.js:236: bookmarkNodeChanged:
function(id, changeInfo) {
wouldn't a change to a folder name anywhere in the ancestry of |this| need to be
reflected in the crumbs

http://codereview.chromium.org/7713026/diff/6002/chrome/browser/resources/ntp...
chrome/browser/resources/ntp4/bookmarks_page.js:277: this.id ==
moveInfo.oldParentId)
curlies

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

http://codereview.chromium.org/7713026/diff/6002/chrome/browser/ui/webui/ntp/...
chrome/browser/ui/webui/ntp/bookmarks_handler.cc:121: // Ignored.
explain why or add a TODO

[Evan Stade, 2011-08-23 21:51:10]

also, I think the description should say "observe and process..."

[csilv, 2011-08-24 23:15:11]

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

http://codereview.chromium.org/7713026/diff/6002/chrome/browser/resources/ntp...
chrome/browser/resources/ntp4/bookmarks_page.js:219: bookmarkImportBegan:
function() {
On 2011/08/23 21:50:50, Evan Stade wrote:
> why is this here? why can't you just handle it in C++?

We could handle this in the C++ handler, but I think it's better to try to
pattern the bookmarks api so that if/when we migrate to the API the logic here
will already be correct.

http://codereview.chromium.org/7713026/diff/6002/chrome/browser/resources/ntp...
chrome/browser/resources/ntp4/bookmarks_page.js:229: bookmarkNodeAdded:
function(id, bookmark) {
On 2011/08/23 21:50:50, Evan Stade wrote:
> document each function

Done.

http://codereview.chromium.org/7713026/diff/6002/chrome/browser/resources/ntp...
chrome/browser/resources/ntp4/bookmarks_page.js:231: if (this.id ==
bookmark.parentId) {
On 2011/08/23 21:50:50, Evan Stade wrote:
> no curlies

Done.

http://codereview.chromium.org/7713026/diff/6002/chrome/browser/resources/ntp...
chrome/browser/resources/ntp4/bookmarks_page.js:236: bookmarkNodeChanged:
function(id, changeInfo) {
On 2011/08/23 21:50:50, Evan Stade wrote:
> wouldn't a change to a folder name anywhere in the ancestry of |this| need to
be
> reflected in the crumbs

Yes indeed.  Fixed here and also in move/remove.

http://codereview.chromium.org/7713026/diff/6002/chrome/browser/resources/ntp...
chrome/browser/resources/ntp4/bookmarks_page.js:277: this.id ==
moveInfo.oldParentId)
On 2011/08/23 21:50:50, Evan Stade wrote:
> curlies

Done.

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

http://codereview.chromium.org/7713026/diff/6002/chrome/browser/ui/webui/ntp/...
chrome/browser/ui/webui/ntp/bookmarks_handler.cc:121: // Ignored.
On 2011/08/23 21:50:50, Evan Stade wrote:
> explain why or add a TODO

Done.

[Evan Stade, 2011-08-25 01:57:57]

lgtm aside from the loaded thing

[csilv, 2011-08-25 18:13:33]

On 2011/08/25 01:57:57, Evan Stade wrote:
> lgtm aside from the loaded thing

I decided to use the Loaded handler after giving it some more thought.  If you
could review this change I'd appreciate it.

[Evan Stade, 2011-08-26 23:54:37]

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

http://codereview.chromium.org/7713026/diff/11003/chrome/browser/ui/webui/ntp...
chrome/browser/ui/webui/ntp/bookmarks_handler.cc:147: getBookmarksDataIsPending_
= true;
I don't really think this is necessary. Loaded only happens once.

[csilv, 2011-08-29 17:54:03]

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

http://codereview.chromium.org/7713026/diff/11003/chrome/browser/ui/webui/ntp...
chrome/browser/ui/webui/ntp/bookmarks_handler.cc:147: getBookmarksDataIsPending_
= true;
On 2011/08/26 23:54:37, Evan Stade wrote:
> I don't really think this is necessary. Loaded only happens once.

Yes, it only happens once.  But the problem is that the bookmarks loaded event
could be received before the NTP4 DOM has loaded.  If we don't check for this
condition, we attempt to send the bookmarks to the NTP4 page early either
causing it to be missed or processed incorrectly.

[Evan Stade, 2011-08-29 18:17:03]

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

http://codereview.chromium.org/7713026/diff/11003/chrome/browser/ui/webui/ntp...
chrome/browser/ui/webui/ntp/bookmarks_handler.cc:147: getBookmarksDataIsPending_
= true;
I'm not saying the model->IsLoaded() check is unnecessary, it's the
getBookmarksDataIsPending_ thing that's extraneous.

[csilv, 2011-08-29 19:18:48]

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

http://codereview.chromium.org/7713026/diff/11003/chrome/browser/ui/webui/ntp...
chrome/browser/ui/webui/ntp/bookmarks_handler.cc:147: getBookmarksDataIsPending_
= true;
On 2011/08/29 18:17:03, Evan Stade wrote:
> I'm not saying the model->IsLoaded() check is unnecessary, it's the
> getBookmarksDataIsPending_ thing that's extraneous.

It's important to have.  Without that flag, the BookmarksHandler::Loaded may be
called before the DOM is loaded.  That would cause us to send the bookmarks list
to the NTP4 prematurely, possibly resulting in the contents of the bookmarks
page getting lose or mishandled.

[Evan Stade, 2011-08-29 20:09:45]

oh I get it. LGTM