Should not unregister BookmarkTreeItem from lookup table on handling moved event

In BookmarkTree.handleMoved(), moved tree item (bookmark folder) is unregistered by BookmarkTree.remove(), which overrides Tree.remove() and deletes child folder node from node id to node map, treeLookup[].

This causes subsequent move operation for moved tree item doesn't update BookmarkTree UI. Because moved tree item no longer in BookmarkTree look up table.

This patch removes override method BookmarkTreeItem.remove() for keeping removed node in treeLookup[].

BUG=123535
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=175311

[yosin_UTC9, 2012-12-20 05:35:23]

Hi Eric,
Could you review this patch?
Thanks in advance.

[arv (Not doing code reviews), 2012-12-20 14:54:51]

https://codereview.chromium.org/11635038/diff/1/chrome/browser/resources/book...
File chrome/browser/resources/bookmark_manager/js/bmm/bookmark_tree.js (right):

https://codereview.chromium.org/11635038/diff/1/chrome/browser/resources/book...
chrome/browser/resources/bookmark_manager/js/bmm/bookmark_tree.js:196:
TreeItem.prototype.remove.call(oldParentItem, treeItem);
This seems pretty hacky. Is there an alternative solution we could use?

[yosin_UTC9, 2012-12-21 09:15:42]

On 2012/12/20 14:54:51, arv wrote:
>
https://codereview.chromium.org/11635038/diff/1/chrome/browser/resources/book...
> File chrome/browser/resources/bookmark_manager/js/bmm/bookmark_tree.js
(right):
> 
>
https://codereview.chromium.org/11635038/diff/1/chrome/browser/resources/book...
> chrome/browser/resources/bookmark_manager/js/bmm/bookmark_tree.js:196:
> TreeItem.prototype.remove.call(oldParentItem, treeItem);
> This seems pretty hacky. Is there an alternative solution we could use?

Option 1 - Don't override TreeItem.remove.
PROS - BookmarkTree.handleRemove() removes node from treeLookup
CONS - API change although nobody uses BookmarkTreeItem.remove otherthan
BookmarkTree.

Option 2 - Register after remove
PROS - One line change, e.g. 
  oldParentItem.remove(treeItem);
  treeLookup[treeItem.id] = treeItem; // Add
CONS - Unregister then register

Option 3 - Wrap TreeItem.remove() into BookmarkTreeItem.removeInternal()
PROS - Ugly thing in one place == removeInternal(). BookmarkTree.remove() also
calls removeInterlan() instead of TreeItem.prototype.remove.call()
CONS - Just sugar syntax(?)

Which one is the best? or another idea?

Sorry, my build environment was broken today. I couldn't make patch.

[arv (Not doing code reviews), 2012-12-21 15:02:37]

I think option 1 is the best. Lets separate the concerns of removing
the tree from treeLookup.

On Fri, Dec 21, 2012 at 4:15 AM,  <yosin@chromium.org> wrote:
> On 2012/12/20 14:54:51, arv wrote:
>
>
https://codereview.chromium.org/11635038/diff/1/chrome/browser/resources/book...
>>
>> File chrome/browser/resources/bookmark_manager/js/bmm/bookmark_tree.js
>
> (right):
>
>
>
https://codereview.chromium.org/11635038/diff/1/chrome/browser/resources/book...
>>
>> chrome/browser/resources/bookmark_manager/js/bmm/bookmark_tree.js:196:
>> TreeItem.prototype.remove.call(oldParentItem, treeItem);
>> This seems pretty hacky. Is there an alternative solution we could use?
>
>
> Option 1 - Don't override TreeItem.remove.
> PROS - BookmarkTree.handleRemove() removes node from treeLookup
> CONS - API change although nobody uses BookmarkTreeItem.remove otherthan
> BookmarkTree.
>
> Option 2 - Register after remove
> PROS - One line change, e.g.
>   oldParentItem.remove(treeItem);
>   treeLookup[treeItem.id] = treeItem; // Add
> CONS - Unregister then register
>
> Option 3 - Wrap TreeItem.remove() into BookmarkTreeItem.removeInternal()
> PROS - Ugly thing in one place == removeInternal(). BookmarkTree.remove()
> also
> calls removeInterlan() instead of TreeItem.prototype.remove.call()
> CONS - Just sugar syntax(?)
>
> Which one is the best? or another idea?
>
> Sorry, my build environment was broken today. I couldn't make patch.
>
>
>
>
> https://codereview.chromium.org/11635038/



-- 
erik

[yosin_UTC9, 2012-12-25 04:40:19]

Hi Eric,
Could you review this patch?
Thanks in advance.

This patch just removes BookmarkTreeItem.remove(), no other changes.

[arv (Not doing code reviews), 2013-01-02 14:19:14]

LGTM