Checking node object before checking its property.

Checking node object before checking its property.

Before checking any object's properties that object should be defined.

BUG=480935
Committed: https://crrev.com/bd9769126ad8c88c20d2234e838e6ef637a4b856
Cr-Commit-Position: refs/heads/master@{#327730}

[Deepak, 2015-04-24 13:17:08]

deepak.m1@samsung.com changed reviewers:
+ bauerb@chromium.org

[Deepak, 2015-04-24 13:17:09]

PTAL

[Bernhard Bauer, 2015-04-24 13:53:15]

Checking for a value solves the immediate issue, but the question is why the
value we get here is undefined? It turns out that the callback to
chrome.bookmarkManagerPrivate.getSubtree does not check
chrome.runtime.lastError. If it would (and reject the Promise, which is how
errors are handled when dealing with Promises), there would be the error "Can't
find bookmark for id.". Why that happens is what we need to find out (and this
is as far as I got).

In the mean time, there are some things we could do to surface these errors more
prominently:
* Reject the promise in getSubtreePromise if there was an error
* Change the type annotation for getAllUrls() to take an array of non-nullable
BookmarkTreeNodes (that one will not change the observable behavior, but at
least make it clearer for a developer that none of the nodes should be
undefined)

[Deepak, 2015-04-25 12:36:11]

Thanks for review.
1. Now Handling chrome.runtime.lastError in callback of
chrome.bookmarkManagerPrivate.getSubtree() 
and when we have chrome.runtime.lastError then rejecting the promise. And
printing console.error() 
in the callback function in 'then' part.
2. We are accessing 
bool BookmarksGetSubTreeFunction::RunOnReady() 
  GetBookmarkNodeFromId()
here we are trying to fetch the node that was already get deleted, via its old
id,
As no node exist with that id , so we are getting error like 
error_ = keys::kNoNodeError;
which is const char kNoNodeError[] = "Can't find bookmark for id.";
3. When bmm.loadSubtree() have chrome.runtime.lastError then it will return
undefined.
we are getting array of undefined nodes, as part of resolve() call.
As we are calling 
nodes.forEach(addNodes);
so we need to have a check in addNodes() so that properties of undefined object
are not getting
accessed.or need to put loop while passing node in addNodes();

I have done changes as suggested.
PTAL

[Deepak, 2015-04-28 02:14:37]

PTAL

[Bernhard Bauer, 2015-04-28 07:44:41]

This is going in the right direction, but I still don't understand why we are
trying to access a bookmark node that has already been deleted. The node is
obviously still in the UI.

[Deepak, 2015-04-28 08:54:42]

On 2015/04/28 07:44:41, Bernhard Bauer wrote:
> This is going in the right direction, but I still don't understand why we are
> trying to access a bookmark node that has already been deleted. The node is
> obviously still in the UI.

AFAIK this is due to asynchronous nature of Promise and way they work.
And after node get deleted, it updated the subtree with the node id of deleted
node.
and if node(folder node) with id not present , subtree for that node will not be
created.

[Bernhard Bauer, 2015-04-28 14:40:19]

On 2015/04/28 08:54:42, Deepak wrote:
> On 2015/04/28 07:44:41, Bernhard Bauer wrote:
> > This is going in the right direction, but I still don't understand why we
are
> > trying to access a bookmark node that has already been deleted. The node is
> > obviously still in the UI.
> 
> AFAIK this is due to asynchronous nature of Promise and way they work.
> And after node get deleted, it updated the subtree with the node id of deleted
> node.
> and if node(folder node) with id not present , subtree for that node will not
be
> created.

Ugh, this seems to be another one of those cases where setting JS breakpoints
changes the observable behavior :-(

I'm still of the opinion that we should not try to fetch bookmarks for
non-existing IDs, so I'll investigate a bit further to see how we can prevent
this. (But feel free to also investigate if you want)

[Deepak, 2015-04-29 08:14:04]

On 2015/04/28 14:40:19, Bernhard Bauer wrote:
> On 2015/04/28 08:54:42, Deepak wrote:
> > On 2015/04/28 07:44:41, Bernhard Bauer wrote:
> > > This is going in the right direction, but I still don't understand why we
> are
> > > trying to access a bookmark node that has already been deleted. The node
is
> > > obviously still in the UI.
> > 
> > AFAIK this is due to asynchronous nature of Promise and way they work.
> > And after node get deleted, it updated the subtree with the node id of
deleted
> > node.
> > and if node(folder node) with id not present , subtree for that node will
not
> be
> > created.
> 
> Ugh, this seems to be another one of those cases where setting JS breakpoints
> changes the observable behavior :-(
> 
> I'm still of the opinion that we should not try to fetch bookmarks for
> non-existing IDs, so I'll investigate a bit further to see how we can prevent
> this. (But feel free to also investigate if you want)

After analyzing more I observed that
1) Even when deleteBookmarks() happened then also in the getAllUrls() we are
getting all the nodes 
of  list datamodel as 
getNodesForOpen(target) returns 
bmm.list.dataModel.slice();

it have data of all nodes in list.

2) getAllUrl() get called with different value of nodes length, 
as I found that nodes.length is differnet in different calls.(It is decreasing).

[Bernhard Bauer, 2015-04-29 08:55:09]

On 2015/04/29 08:14:04, Deepak wrote:
> On 2015/04/28 14:40:19, Bernhard Bauer wrote:
> > On 2015/04/28 08:54:42, Deepak wrote:
> > > On 2015/04/28 07:44:41, Bernhard Bauer wrote:
> > > > This is going in the right direction, but I still don't understand why
we
> > are
> > > > trying to access a bookmark node that has already been deleted. The node
> is
> > > > obviously still in the UI.
> > > 
> > > AFAIK this is due to asynchronous nature of Promise and way they work.
> > > And after node get deleted, it updated the subtree with the node id of
> deleted
> > > node.
> > > and if node(folder node) with id not present , subtree for that node will
> not
> > be
> > > created.
> > 
> > Ugh, this seems to be another one of those cases where setting JS
breakpoints
> > changes the observable behavior :-(
> > 
> > I'm still of the opinion that we should not try to fetch bookmarks for
> > non-existing IDs, so I'll investigate a bit further to see how we can
prevent
> > this. (But feel free to also investigate if you want)
> 
> After analyzing more I observed that
> 1) Even when deleteBookmarks() happened then also in the getAllUrls() we are
> getting all the nodes 
> of  list datamodel as 
> getNodesForOpen(target) returns 
> bmm.list.dataModel.slice();
> 
> it have data of all nodes in list.
> 
> 2) getAllUrl() get called with different value of nodes length, 
> as I found that nodes.length is differnet in different calls.(It is
decreasing).

Yeah, the error happens with the following stack trace:
getNodesForOpen (main.js:359)
getUrlsForOpenCommands (main.js:378)
updateOpenCommand (main.js:412)
canExecuteShared (main.js:543)
canExecuteForList (main.js:643)
handleCanExecuteForList (main.js:650)
dispatchCanExecuteEvent (command.js:200)
Command.canExecuteChange (command.js:111)
updateAllCommands (main.js:715)
dispatchSimpleEvent (cr.js:249)
List.handleOnChange_ (list.js:594)
EventTarget.dispatchEvent (event_target.js:93)
ListSelectionModel.endChange (list_selection_model.js:252)
ListSelectionModel.adjustToReordering (list_selection_model.js:352)
List.handleDataModelPermuted_ (list.js:668)
EventTarget.dispatchEvent (event_target.js:93)
ArrayDataModel.dispatchPermutedEvent_ (array_data_model.js:451)
ArrayDataModel.splice (array_data_model.js:260)
BookmarkList.handleRemoved (bookmark_list.js:347)
handleRemoved (bmm.js:182)
EventImpl.dispatchToListener (extensions::event_bindings:387)
publicClass.(anonymous function) (extensions::utils:94)
EventImpl.dispatch_ (extensions::event_bindings:371)
dispatchArgs (extensions::event_bindings:243)
dispatchEvent (extensions::event_bindings:252)


(I modified main.js to enable setting breakpoints, which might have shifted line
numbers in main.js by 1.)

But basically, we are in this method:

    function getNodesForOpen(target) {
      if (target == bmm.tree) {
        if (bmm.tree.selectedItem != searchTreeItem)
          return bmm.tree.selectedFolders;
        // Fall through to use all nodes in the list.
      } else {
        var items = bmm.list.selectedItems;
        if (items.length) {
->       return items;
        }
      }
    }

So we are trying to update a command after the model has changed, but
bmm.list.selectedItems has not been updated yet, so it returns an item that
doesn't actually exist anymore. Interestingly, when I set a breakpoint and
inspect bmm.list.selectedItem, it is already empty, but |items| isn't. o_O (I
suspect that a different event still fires even though execution of the first
event handler has been stopped.)

In any case, it would be interesting to find out where exactly
bmm.list.selectedItems is updated, and whether we can make sure that happens
before the commands are updated.

[Deepak, 2015-04-29 11:01:33]

On 2015/04/29 08:55:09, Bernhard Bauer wrote:
> On 2015/04/29 08:14:04, Deepak wrote:
> > On 2015/04/28 14:40:19, Bernhard Bauer wrote:
> > > On 2015/04/28 08:54:42, Deepak wrote:
> > > > On 2015/04/28 07:44:41, Bernhard Bauer wrote:
> > > > > This is going in the right direction, but I still don't understand why
> we
> > > are
> > > > > trying to access a bookmark node that has already been deleted. The
node
> > is
> > > > > obviously still in the UI.
> > > > 
> > > > AFAIK this is due to asynchronous nature of Promise and way they work.
> > > > And after node get deleted, it updated the subtree with the node id of
> > deleted
> > > > node.
> > > > and if node(folder node) with id not present , subtree for that node
will
> > not
> > > be
> > > > created.
> > > 
> > > Ugh, this seems to be another one of those cases where setting JS
> breakpoints
> > > changes the observable behavior :-(
> > > 
> > > I'm still of the opinion that we should not try to fetch bookmarks for
> > > non-existing IDs, so I'll investigate a bit further to see how we can
> prevent
> > > this. (But feel free to also investigate if you want)
> > 
> > After analyzing more I observed that
> > 1) Even when deleteBookmarks() happened then also in the getAllUrls() we are
> > getting all the nodes 
> > of  list datamodel as 
> > getNodesForOpen(target) returns 
> > bmm.list.dataModel.slice();
> > 
> > it have data of all nodes in list.
> > 
> > 2) getAllUrl() get called with different value of nodes length, 
> > as I found that nodes.length is differnet in different calls.(It is
> decreasing).
> 
> Yeah, the error happens with the following stack trace:
> getNodesForOpen (main.js:359)
> getUrlsForOpenCommands (main.js:378)
> updateOpenCommand (main.js:412)
> canExecuteShared (main.js:543)
> canExecuteForList (main.js:643)
> handleCanExecuteForList (main.js:650)
> dispatchCanExecuteEvent (command.js:200)
> Command.canExecuteChange (command.js:111)
> updateAllCommands (main.js:715)
> dispatchSimpleEvent (cr.js:249)
> List.handleOnChange_ (list.js:594)
> EventTarget.dispatchEvent (event_target.js:93)
> ListSelectionModel.endChange (list_selection_model.js:252)
> ListSelectionModel.adjustToReordering (list_selection_model.js:352)
> List.handleDataModelPermuted_ (list.js:668)
> EventTarget.dispatchEvent (event_target.js:93)
> ArrayDataModel.dispatchPermutedEvent_ (array_data_model.js:451)
> ArrayDataModel.splice (array_data_model.js:260)
> BookmarkList.handleRemoved (bookmark_list.js:347)
> handleRemoved (bmm.js:182)
> EventImpl.dispatchToListener (extensions::event_bindings:387)
> publicClass.(anonymous function) (extensions::utils:94)
> EventImpl.dispatch_ (extensions::event_bindings:371)
> dispatchArgs (extensions::event_bindings:243)
> dispatchEvent (extensions::event_bindings:252)
> 
> 
> (I modified main.js to enable setting breakpoints, which might have shifted
line
> numbers in main.js by 1.)
> 
> But basically, we are in this method:
> 
>     function getNodesForOpen(target) {
>       if (target == bmm.tree) {
>         if (bmm.tree.selectedItem != searchTreeItem)
>           return bmm.tree.selectedFolders;
>         // Fall through to use all nodes in the list.
>       } else {
>         var items = bmm.list.selectedItems;
>         if (items.length) {
> ->       return items;
>         }
>       }
>     }
> 
> So we are trying to update a command after the model has changed, but
> bmm.list.selectedItems has not been updated yet, so it returns an item that
> doesn't actually exist anymore. Interestingly, when I set a breakpoint and
> inspect bmm.list.selectedItem, it is already empty, but |items| isn't. o_O (I
> suspect that a different event still fires even though execution of the first
> event handler has been stopped.)
> 
> In any case, it would be interesting to find out where exactly
> bmm.list.selectedItems is updated, and whether we can make sure that happens
> before the commands are updated.

1) I am little confused, in above callstack
canExecuteShared (main.js:543)
That is
 case 'open-in-new-tab-command':
in canExecuteShared().
But we have not done anything like 'open in new tab' selection from context
menu.

2) getUrlsForOpenCommands() is common for 
openBookmarks() and updateOpenCommand().

and I agree that in getNodesForOpen() , bmm.list.selectedItems.length is non
zero.
But in the last call just before we get chrome.runtime.lastError,
bmm.list.selectedItems.length is zero and 
bmm.list.dataModel.slice(); returned from getNodesForOpen().

I tried to reload the bmm.list, but that doesn't help :(

[Deepak, 2015-04-30 13:42:54]

On 2015/04/29 11:01:33, Deepak wrote:
> On 2015/04/29 08:55:09, Bernhard Bauer wrote:
> > On 2015/04/29 08:14:04, Deepak wrote:
> > > On 2015/04/28 14:40:19, Bernhard Bauer wrote:
> > > > On 2015/04/28 08:54:42, Deepak wrote:
> > > > > On 2015/04/28 07:44:41, Bernhard Bauer wrote:
> > > > > > This is going in the right direction, but I still don't understand
why
> > we
> > > > are
> > > > > > trying to access a bookmark node that has already been deleted. The
> node
> > > is
> > > > > > obviously still in the UI.
> > > > > 
> > > > > AFAIK this is due to asynchronous nature of Promise and way they work.
> > > > > And after node get deleted, it updated the subtree with the node id of
> > > deleted
> > > > > node.
> > > > > and if node(folder node) with id not present , subtree for that node
> will
> > > not
> > > > be
> > > > > created.
> > > > 
> > > > Ugh, this seems to be another one of those cases where setting JS
> > breakpoints
> > > > changes the observable behavior :-(
> > > > 
> > > > I'm still of the opinion that we should not try to fetch bookmarks for
> > > > non-existing IDs, so I'll investigate a bit further to see how we can
> > prevent
> > > > this. (But feel free to also investigate if you want)
> > > 
> > > After analyzing more I observed that
> > > 1) Even when deleteBookmarks() happened then also in the getAllUrls() we
are
> > > getting all the nodes 
> > > of  list datamodel as 
> > > getNodesForOpen(target) returns 
> > > bmm.list.dataModel.slice();
> > > 
> > > it have data of all nodes in list.
> > > 
> > > 2) getAllUrl() get called with different value of nodes length, 
> > > as I found that nodes.length is differnet in different calls.(It is
> > decreasing).
> > 
> > Yeah, the error happens with the following stack trace:
> > getNodesForOpen (main.js:359)
> > getUrlsForOpenCommands (main.js:378)
> > updateOpenCommand (main.js:412)
> > canExecuteShared (main.js:543)
> > canExecuteForList (main.js:643)
> > handleCanExecuteForList (main.js:650)
> > dispatchCanExecuteEvent (command.js:200)
> > Command.canExecuteChange (command.js:111)
> > updateAllCommands (main.js:715)
> > dispatchSimpleEvent (cr.js:249)
> > List.handleOnChange_ (list.js:594)
> > EventTarget.dispatchEvent (event_target.js:93)
> > ListSelectionModel.endChange (list_selection_model.js:252)
> > ListSelectionModel.adjustToReordering (list_selection_model.js:352)
> > List.handleDataModelPermuted_ (list.js:668)
> > EventTarget.dispatchEvent (event_target.js:93)
> > ArrayDataModel.dispatchPermutedEvent_ (array_data_model.js:451)
> > ArrayDataModel.splice (array_data_model.js:260)
> > BookmarkList.handleRemoved (bookmark_list.js:347)
> > handleRemoved (bmm.js:182)
> > EventImpl.dispatchToListener (extensions::event_bindings:387)
> > publicClass.(anonymous function) (extensions::utils:94)
> > EventImpl.dispatch_ (extensions::event_bindings:371)
> > dispatchArgs (extensions::event_bindings:243)
> > dispatchEvent (extensions::event_bindings:252)
> > 
> > 
> > (I modified main.js to enable setting breakpoints, which might have shifted
> line
> > numbers in main.js by 1.)
> > 
> > But basically, we are in this method:
> > 
> >     function getNodesForOpen(target) {
> >       if (target == bmm.tree) {
> >         if (bmm.tree.selectedItem != searchTreeItem)
> >           return bmm.tree.selectedFolders;
> >         // Fall through to use all nodes in the list.
> >       } else {
> >         var items = bmm.list.selectedItems;
> >         if (items.length) {
> > ->       return items;
> >         }
> >       }
> >     }
> > 
> > So we are trying to update a command after the model has changed, but
> > bmm.list.selectedItems has not been updated yet, so it returns an item that
> > doesn't actually exist anymore. Interestingly, when I set a breakpoint and
> > inspect bmm.list.selectedItem, it is already empty, but |items| isn't. o_O
(I
> > suspect that a different event still fires even though execution of the
first
> > event handler has been stopped.)
> > 
> > In any case, it would be interesting to find out where exactly
> > bmm.list.selectedItems is updated, and whether we can make sure that happens
> > before the commands are updated.
> 
> 1) I am little confused, in above callstack
> canExecuteShared (main.js:543)
> That is
>  case 'open-in-new-tab-command':
> in canExecuteShared().
> But we have not done anything like 'open in new tab' selection from context
> menu.
> 
> 2) getUrlsForOpenCommands() is common for 
> openBookmarks() and updateOpenCommand().
> 
> and I agree that in getNodesForOpen() , bmm.list.selectedItems.length is non
> zero.
> But in the last call just before we get chrome.runtime.lastError,
> bmm.list.selectedItems.length is zero and 
> bmm.list.dataModel.slice(); returned from getNodesForOpen().
> 
> I tried to reload the bmm.list, but that doesn't help :(

Got stuck with above observation as getAllUrls() is getting called too many
time.
Appears that due to event dispatch sequence and asynchronous nature of promise,
even when we have no selected item after delete, it is still have few selected
items(As when I have deleted 2 selected folders then it shows one selected item)
in getNodesForOpen()
Due to that nodes that are already deleted are getting accessed.

With current changes we are avoiding flood of error messages, as we handled
chrome.runtime.lastError ,
still one console log will come.

Any suggestion or direction for me for this.

[Bernhard Bauer, 2015-04-30 13:47:59]

On 2015/04/30 13:42:54, Deepak wrote:
> On 2015/04/29 11:01:33, Deepak wrote:
> > On 2015/04/29 08:55:09, Bernhard Bauer wrote:
> > > On 2015/04/29 08:14:04, Deepak wrote:
> > > > On 2015/04/28 14:40:19, Bernhard Bauer wrote:
> > > > > On 2015/04/28 08:54:42, Deepak wrote:
> > > > > > On 2015/04/28 07:44:41, Bernhard Bauer wrote:
> > > > > > > This is going in the right direction, but I still don't understand
> why
> > > we
> > > > > are
> > > > > > > trying to access a bookmark node that has already been deleted.
The
> > node
> > > > is
> > > > > > > obviously still in the UI.
> > > > > > 
> > > > > > AFAIK this is due to asynchronous nature of Promise and way they
work.
> > > > > > And after node get deleted, it updated the subtree with the node id
of
> > > > deleted
> > > > > > node.
> > > > > > and if node(folder node) with id not present , subtree for that node
> > will
> > > > not
> > > > > be
> > > > > > created.
> > > > > 
> > > > > Ugh, this seems to be another one of those cases where setting JS
> > > breakpoints
> > > > > changes the observable behavior :-(
> > > > > 
> > > > > I'm still of the opinion that we should not try to fetch bookmarks for
> > > > > non-existing IDs, so I'll investigate a bit further to see how we can
> > > prevent
> > > > > this. (But feel free to also investigate if you want)
> > > > 
> > > > After analyzing more I observed that
> > > > 1) Even when deleteBookmarks() happened then also in the getAllUrls() we
> are
> > > > getting all the nodes 
> > > > of  list datamodel as 
> > > > getNodesForOpen(target) returns 
> > > > bmm.list.dataModel.slice();
> > > > 
> > > > it have data of all nodes in list.
> > > > 
> > > > 2) getAllUrl() get called with different value of nodes length, 
> > > > as I found that nodes.length is differnet in different calls.(It is
> > > decreasing).
> > > 
> > > Yeah, the error happens with the following stack trace:
> > > getNodesForOpen (main.js:359)
> > > getUrlsForOpenCommands (main.js:378)
> > > updateOpenCommand (main.js:412)
> > > canExecuteShared (main.js:543)
> > > canExecuteForList (main.js:643)
> > > handleCanExecuteForList (main.js:650)
> > > dispatchCanExecuteEvent (command.js:200)
> > > Command.canExecuteChange (command.js:111)
> > > updateAllCommands (main.js:715)
> > > dispatchSimpleEvent (cr.js:249)
> > > List.handleOnChange_ (list.js:594)
> > > EventTarget.dispatchEvent (event_target.js:93)
> > > ListSelectionModel.endChange (list_selection_model.js:252)
> > > ListSelectionModel.adjustToReordering (list_selection_model.js:352)
> > > List.handleDataModelPermuted_ (list.js:668)
> > > EventTarget.dispatchEvent (event_target.js:93)
> > > ArrayDataModel.dispatchPermutedEvent_ (array_data_model.js:451)
> > > ArrayDataModel.splice (array_data_model.js:260)
> > > BookmarkList.handleRemoved (bookmark_list.js:347)
> > > handleRemoved (bmm.js:182)
> > > EventImpl.dispatchToListener (extensions::event_bindings:387)
> > > publicClass.(anonymous function) (extensions::utils:94)
> > > EventImpl.dispatch_ (extensions::event_bindings:371)
> > > dispatchArgs (extensions::event_bindings:243)
> > > dispatchEvent (extensions::event_bindings:252)
> > > 
> > > 
> > > (I modified main.js to enable setting breakpoints, which might have
shifted
> > line
> > > numbers in main.js by 1.)
> > > 
> > > But basically, we are in this method:
> > > 
> > >     function getNodesForOpen(target) {
> > >       if (target == bmm.tree) {
> > >         if (bmm.tree.selectedItem != searchTreeItem)
> > >           return bmm.tree.selectedFolders;
> > >         // Fall through to use all nodes in the list.
> > >       } else {
> > >         var items = bmm.list.selectedItems;
> > >         if (items.length) {
> > > ->       return items;
> > >         }
> > >       }
> > >     }
> > > 
> > > So we are trying to update a command after the model has changed, but
> > > bmm.list.selectedItems has not been updated yet, so it returns an item
that
> > > doesn't actually exist anymore. Interestingly, when I set a breakpoint and
> > > inspect bmm.list.selectedItem, it is already empty, but |items| isn't. o_O
> (I
> > > suspect that a different event still fires even though execution of the
> first
> > > event handler has been stopped.)
> > > 
> > > In any case, it would be interesting to find out where exactly
> > > bmm.list.selectedItems is updated, and whether we can make sure that
happens
> > > before the commands are updated.
> > 
> > 1) I am little confused, in above callstack
> > canExecuteShared (main.js:543)
> > That is
> >  case 'open-in-new-tab-command':
> > in canExecuteShared().
> > But we have not done anything like 'open in new tab' selection from context
> > menu.
> > 
> > 2) getUrlsForOpenCommands() is common for 
> > openBookmarks() and updateOpenCommand().
> > 
> > and I agree that in getNodesForOpen() , bmm.list.selectedItems.length is non
> > zero.
> > But in the last call just before we get chrome.runtime.lastError,
> > bmm.list.selectedItems.length is zero and 
> > bmm.list.dataModel.slice(); returned from getNodesForOpen().
> > 
> > I tried to reload the bmm.list, but that doesn't help :(
> 
> Got stuck with above observation as getAllUrls() is getting called too many
> time.
> Appears that due to event dispatch sequence and asynchronous nature of
promise,
> even when we have no selected item after delete, it is still have few selected
> items(As when I have deleted 2 selected folders then it shows one selected
item)
> in getNodesForOpen()
> Due to that nodes that are already deleted are getting accessed.
> 
> With current changes we are avoiding flood of error messages, as we handled
> chrome.runtime.lastError ,
> still one console log will come.
> 
> Any suggestion or direction for me for this.

Yeah, this is kind of icky (and I haven't had much time recently to investigate
more). I still think fetching a non-existing bookmark is a bug that should be
avoided in the first place, but I'm somewhat sympathetic to silencing the flood
of console errors. Can we at least add a comment with a TODO stating that this
is a bug that should be fixed (referencing https://crbug.com/480935)?

[Bernhard Bauer, 2015-04-30 13:51:07]

https://codereview.chromium.org/1059413005/diff/60001/chrome/browser/resource...
File chrome/browser/resources/bookmark_manager/js/bmm.js (right):

https://codereview.chromium.org/1059413005/diff/60001/chrome/browser/resource...
chrome/browser/resources/bookmark_manager/js/bmm.js:62: delete
loadingPromises[id];
BTW, we should delete the loading promise even when it is rejected. I think you
should be able to do this moving this line to a separate callback:

  loadingPromises[id].then(function() {
    delete loadingPromises[id];
  }

[Deepak, 2015-04-30 15:00:59]

I agree with you.
When we call resolve or reject then loadingPromises[id] object existed in both
cases, so we need to delete it in both cases.
Added TODO in getAllUrls().
PTAL

[Bernhard Bauer, 2015-04-30 15:08:07]

https://codereview.chromium.org/1059413005/diff/80001/chrome/browser/resource...
File chrome/browser/resources/bookmark_manager/js/bmm.js (right):

https://codereview.chromium.org/1059413005/diff/80001/chrome/browser/resource...
chrome/browser/resources/bookmark_manager/js/bmm.js:64:
loadingPromises[id].then(function() {
You can pull this out into a single callback on loadingPromises[id] after you've
assigned a value to it. The reason why this works is that adding an error
handler will cause loadingPromises[id] to be a resolved promise, even if the
promise returned from getSubtreePromise is rejected. The error handler
effectively turns the rejected promise into a resolved one (with an undefined
value). Then the callback you add with .then is guaranteed to be called.

https://codereview.chromium.org/1059413005/diff/80001/chrome/browser/resource...
File chrome/browser/resources/bookmark_manager/js/main.js (right):

https://codereview.chromium.org/1059413005/diff/80001/chrome/browser/resource...
chrome/browser/resources/bookmark_manager/js/main.js:335: // TODO(deepak.m1):
All the nodes here should be existed. When we delete
Move this up to addNodes(), because that's where the check for undefined values
is (which shouldn't be necessary). Also, s/should be existed/should exist/.

[Deepak, 2015-04-30 16:11:19]

PTAL

https://codereview.chromium.org/1059413005/diff/80001/chrome/browser/resource...
File chrome/browser/resources/bookmark_manager/js/bmm.js (right):

https://codereview.chromium.org/1059413005/diff/80001/chrome/browser/resource...
chrome/browser/resources/bookmark_manager/js/bmm.js:64:
loadingPromises[id].then(function() {
On 2015/04/30 15:08:07, Bernhard Bauer wrote:
> You can pull this out into a single callback on loadingPromises[id] after
you've
> assigned a value to it. The reason why this works is that adding an error
> handler will cause loadingPromises[id] to be a resolved promise, even if the
> promise returned from getSubtreePromise is rejected. The error handler
> effectively turns the rejected promise into a resolved one (with an undefined
> value). Then the callback you add with .then is guaranteed to be called.

Done.

https://codereview.chromium.org/1059413005/diff/80001/chrome/browser/resource...
File chrome/browser/resources/bookmark_manager/js/main.js (right):

https://codereview.chromium.org/1059413005/diff/80001/chrome/browser/resource...
chrome/browser/resources/bookmark_manager/js/main.js:335: // TODO(deepak.m1):
All the nodes here should be existed. When we delete
On 2015/04/30 15:08:07, Bernhard Bauer wrote:
> Move this up to addNodes(), because that's where the check for undefined
values
> is (which shouldn't be necessary). Also, s/should be existed/should exist/.

Done.

[Bernhard Bauer, 2015-04-30 16:19:52]

https://codereview.chromium.org/1059413005/diff/120001/chrome/browser/resourc...
File chrome/browser/resources/bookmark_manager/js/bmm.js (right):

https://codereview.chromium.org/1059413005/diff/120001/chrome/browser/resourc...
chrome/browser/resources/bookmark_manager/js/bmm.js:67: return;
The return statement is unnecessary.

[Deepak, 2015-04-30 16:22:42]

Done.
PTAL

https://codereview.chromium.org/1059413005/diff/120001/chrome/browser/resourc...
File chrome/browser/resources/bookmark_manager/js/bmm.js (right):

https://codereview.chromium.org/1059413005/diff/120001/chrome/browser/resourc...
chrome/browser/resources/bookmark_manager/js/bmm.js:67: return;
On 2015/04/30 16:19:52, Bernhard Bauer wrote:
> The return statement is unnecessary.

Done.

[Bernhard Bauer, 2015-04-30 16:23:15]

lgtm

[Deepak, 2015-04-30 16:23:36]

The CQ bit was checked by deepak.m1@samsung.com

[Deepak, 2015-04-30 16:24:01]

On 2015/04/30 16:23:15, Bernhard Bauer wrote:
> lgtm

Thanks.

[commit-bot: I haz the power, 2015-04-30 16:24:52]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1059413005/140001

[commit-bot: I haz the power, 2015-04-30 17:25:03]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2015-04-30 17:26:31]

Patchset 8 (id:??) landed as
https://crrev.com/bd9769126ad8c88c20d2234e838e6ef637a4b856
Cr-Commit-Position: refs/heads/master@{#327730}