Supporting undoing bookmark deletion without creating new ID.

Supporting undoing bookmark deletion without creating new ID.

BUG=

Committed: https://crrev.com/14436d551656d35234425ec35e08345b4368710e
Cr-Commit-Position: refs/heads/master@{#353408}

[sky, 2015-10-02 15:40:29]

sky@chromium.org changed reviewers:
+ sky@chromium.org

[sky, 2015-10-02 15:40:30]

https://chromiumcodereview.appspot.com/1379983002/diff/1/components/bookmarks...
File components/bookmarks/browser/bookmark_model.cc (right):

https://chromiumcodereview.appspot.com/1379983002/diff/1/components/bookmarks...
components/bookmarks/browser/bookmark_model.cc:1: // RemvCopyright 2014 The
Chromium Authors. All rights reserved.
fix

https://chromiumcodereview.appspot.com/1379983002/diff/1/components/bookmarks...
File components/bookmarks/browser/bookmark_model.h (right):

https://chromiumcodereview.appspot.com/1379983002/diff/1/components/bookmarks...
components/bookmarks/browser/bookmark_model.h:312: void EnableUndoableRemove();
I think all the bookkeeping of undo should be kept in the undo area. To enable
BookmarkUndoService we should add an interface called BookmarkModelUndoDelegate.
At the appropriate times (remove, insert, change...) BookmarkModel will call
into the BookmarkModelUndoDelegate. 

Why do we need an interface interface? At a minimum this interface is given
ownership of removed nodes. This is unique. No doubt there will be other unique
abilities that need to be given to the BookmarkModelUndoDelegate.

[jianli, 2015-10-06 22:08:24]

https://chromiumcodereview.appspot.com/1379983002/diff/1/components/bookmarks...
File components/bookmarks/browser/bookmark_model.cc (right):

https://chromiumcodereview.appspot.com/1379983002/diff/1/components/bookmarks...
components/bookmarks/browser/bookmark_model.cc:1: // RemvCopyright 2014 The
Chromium Authors. All rights reserved.
On 2015/10/02 15:40:30, sky wrote:
> fix

Done.

https://chromiumcodereview.appspot.com/1379983002/diff/1/components/bookmarks...
File components/bookmarks/browser/bookmark_model.h (right):

https://chromiumcodereview.appspot.com/1379983002/diff/1/components/bookmarks...
components/bookmarks/browser/bookmark_model.h:312: void EnableUndoableRemove();
On 2015/10/02 15:40:30, sky wrote:
> I think all the bookkeeping of undo should be kept in the undo area. To enable
> BookmarkUndoService we should add an interface called
BookmarkModelUndoDelegate.
> At the appropriate times (remove, insert, change...) BookmarkModel will call
> into the BookmarkModelUndoDelegate. 
> 
> Why do we need an interface interface? At a minimum this interface is given
> ownership of removed nodes. This is unique. No doubt there will be other
unique
> abilities that need to be given to the BookmarkModelUndoDelegate.

Done.

[sky, 2015-10-07 16:30:15]

https://chromiumcodereview.appspot.com/1379983002/diff/40001/components/bookm...
File components/bookmarks/browser/bookmark_model.cc (right):

https://chromiumcodereview.appspot.com/1379983002/diff/40001/components/bookm...
components/bookmarks/browser/bookmark_model.cc:224: if (undo_delegate_) {
Simplify this so that there is always an UndoDelegate.

https://chromiumcodereview.appspot.com/1379983002/diff/40001/components/bookm...
components/bookmarks/browser/bookmark_model.cc:226:
undo_delegate_->OnBookmarkNodeDeleted(
Don't call out to the UndoDelegate while a lock is held (line 214). Also, I
think you should notify UndoDelegate after observers (just as you don on 871
below).

https://chromiumcodereview.appspot.com/1379983002/diff/40001/components/bookm...
File components/bookmarks/browser/bookmark_model.h (right):

https://chromiumcodereview.appspot.com/1379983002/diff/40001/components/bookm...
components/bookmarks/browser/bookmark_model.h:71: class UndoDelegate {
Move into it's own header (see chromium specific style guide for details) and
make destructor protected and virtual.

https://chromiumcodereview.appspot.com/1379983002/diff/40001/components/bookm...
components/bookmarks/browser/bookmark_model.h:80: int64_t parent_node_id,
I would rather supply the BoomarkNode as the parent. If the delegate wants to
track the id, it can.

https://chromiumcodereview.appspot.com/1379983002/diff/40001/components/bookm...
components/bookmarks/browser/bookmark_model.h:82: BookmarkNode* node) = 0;
Use scoped_ptr to clarify ownership.

https://chromiumcodereview.appspot.com/1379983002/diff/40001/components/bookm...
components/bookmarks/browser/bookmark_model.h:239: BookmarkNode*
AddNode(BookmarkNode* parent,
I don't want this to be public. Expose an interface specifically for the
UndoDelegate that BookmarkModel implements.

[jianli, 2015-10-07 23:18:09]

Patchset #4 (id:60001) has been deleted

[jianli, 2015-10-07 23:21:29]

Patchset #4 (id:80001) has been deleted

[jianli, 2015-10-07 23:26:56]

https://chromiumcodereview.appspot.com/1379983002/diff/40001/components/bookm...
File components/bookmarks/browser/bookmark_model.cc (right):

https://chromiumcodereview.appspot.com/1379983002/diff/40001/components/bookm...
components/bookmarks/browser/bookmark_model.cc:224: if (undo_delegate_) {
On 2015/10/07 16:30:15, sky wrote:
> Simplify this so that there is always an UndoDelegate.

Done.

https://chromiumcodereview.appspot.com/1379983002/diff/40001/components/bookm...
components/bookmarks/browser/bookmark_model.cc:226:
undo_delegate_->OnBookmarkNodeDeleted(
On 2015/10/07 16:30:15, sky wrote:
> Don't call out to the UndoDelegate while a lock is held (line 214). Also, I
> think you should notify UndoDelegate after observers (just as you don on 871
> below).

Done.

https://chromiumcodereview.appspot.com/1379983002/diff/40001/components/bookm...
File components/bookmarks/browser/bookmark_model.h (right):

https://chromiumcodereview.appspot.com/1379983002/diff/40001/components/bookm...
components/bookmarks/browser/bookmark_model.h:71: class UndoDelegate {
On 2015/10/07 16:30:15, sky wrote:
> Move into it's own header (see chromium specific style guide for details) and
> make destructor protected and virtual.

Done.

https://chromiumcodereview.appspot.com/1379983002/diff/40001/components/bookm...
components/bookmarks/browser/bookmark_model.h:80: int64_t parent_node_id,
On 2015/10/07 16:30:15, sky wrote:
> I would rather supply the BoomarkNode as the parent. If the delegate wants to
> track the id, it can.

Done.

https://chromiumcodereview.appspot.com/1379983002/diff/40001/components/bookm...
components/bookmarks/browser/bookmark_model.h:82: BookmarkNode* node) = 0;
On 2015/10/07 16:30:15, sky wrote:
> Use scoped_ptr to clarify ownership.

Done.

https://chromiumcodereview.appspot.com/1379983002/diff/40001/components/bookm...
components/bookmarks/browser/bookmark_model.h:239: BookmarkNode*
AddNode(BookmarkNode* parent,
On 2015/10/07 16:30:15, sky wrote:
> I don't want this to be public. Expose an interface specifically for the
> UndoDelegate that BookmarkModel implements.

Done.

[sky, 2015-10-08 15:20:01]

https://chromiumcodereview.appspot.com/1379983002/diff/100001/components/book...
File components/bookmarks/browser/bookmark_model.cc (right):

https://chromiumcodereview.appspot.com/1379983002/diff/100001/components/book...
components/bookmarks/browser/bookmark_model.cc:225:
removed_nodes.push_back(child_node);
You shouldn't need both removed_nodes and removed_node_data, just
removed_node_data and make it contain everything you need.

https://chromiumcodereview.appspot.com/1379983002/diff/100001/components/book...
components/bookmarks/browser/bookmark_model.cc:226:
removed_node_data.push_back(std::make_pair(permanent_node, j));
Please add test coverage here as permanent_node is wrong, you want child_node.

https://chromiumcodereview.appspot.com/1379983002/diff/100001/components/book...
components/bookmarks/browser/bookmark_model.cc:237: if (undo_delegate_) {
Again, always have an undo_delegate_ so that it always take ownership.
Initialize with an empty implementation.

https://chromiumcodereview.appspot.com/1379983002/diff/100001/components/book...
components/bookmarks/browser/bookmark_model.cc:740: AsMutable(parent)->Add(node,
index);
Use AddNode for the common parts.

https://chromiumcodereview.appspot.com/1379983002/diff/100001/components/book...
File components/bookmarks/browser/bookmark_undo_delegate.h (right):

https://chromiumcodereview.appspot.com/1379983002/diff/100001/components/book...
components/bookmarks/browser/bookmark_undo_delegate.h:16: class
BookmarkUndoDelegate {
Add a description.

https://chromiumcodereview.appspot.com/1379983002/diff/100001/components/book...
components/bookmarks/browser/bookmark_undo_delegate.h:18: // Called when |node|
was deleted from |parent| at position |index|. The
deleted->rmeoted

https://chromiumcodereview.appspot.com/1379983002/diff/100001/components/book...
components/bookmarks/browser/bookmark_undo_delegate.h:19: // delegate takes the
ownership of the deleted node.
The second sentence is no longer needed as node is a scoped_ptr.

https://chromiumcodereview.appspot.com/1379983002/diff/100001/components/book...
components/bookmarks/browser/bookmark_undo_delegate.h:20: virtual void
OnBookmarkNodeDeleted(BookmarkModel* model,
OnBookmarkNodeRemoved.

https://chromiumcodereview.appspot.com/1379983002/diff/100001/components/book...
components/bookmarks/browser/bookmark_undo_delegate.h:21: BookmarkUndoProvider*
provider,
Rather than supply the provider on event call how a SetProvider function?

https://chromiumcodereview.appspot.com/1379983002/diff/100001/components/book...
File components/bookmarks/browser/bookmark_undo_provider.h (right):

https://chromiumcodereview.appspot.com/1379983002/diff/100001/components/book...
components/bookmarks/browser/bookmark_undo_provider.h:25: 
nit: only one newline.

https://chromiumcodereview.appspot.com/1379983002/diff/100001/components/undo...
File components/undo/bookmark_undo_service.cc (right):

https://chromiumcodereview.appspot.com/1379983002/diff/100001/components/undo...
components/undo/bookmark_undo_service.cc:108: int64_t parent_node_id_;
const on this and the next.

https://chromiumcodereview.appspot.com/1379983002/diff/100001/components/undo...
components/undo/bookmark_undo_service.cc:330: : model_(NULL),
scoped_observer_(this) {
NULL->nullptr

[jianli, 2015-10-08 23:37:41]

https://chromiumcodereview.appspot.com/1379983002/diff/100001/components/book...
File components/bookmarks/browser/bookmark_model.cc (right):

https://chromiumcodereview.appspot.com/1379983002/diff/100001/components/book...
components/bookmarks/browser/bookmark_model.cc:225:
removed_nodes.push_back(child_node);
On 2015/10/08 15:20:00, sky wrote:
> You shouldn't need both removed_nodes and removed_node_data, just
> removed_node_data and make it contain everything you need.

Done.

https://chromiumcodereview.appspot.com/1379983002/diff/100001/components/book...
components/bookmarks/browser/bookmark_model.cc:226:
removed_node_data.push_back(std::make_pair(permanent_node, j));
On 2015/10/08 15:20:00, sky wrote:
> Please add test coverage here as permanent_node is wrong, you want child_node.

I've updated BookmarkModelTest.RemoveAllUserBookmarks to validate no permanent
nodes have been removed.

https://chromiumcodereview.appspot.com/1379983002/diff/100001/components/book...
components/bookmarks/browser/bookmark_model.cc:237: if (undo_delegate_) {
On 2015/10/08 15:20:00, sky wrote:
> Again, always have an undo_delegate_ so that it always take ownership.
> Initialize with an empty implementation.

Done.

https://chromiumcodereview.appspot.com/1379983002/diff/100001/components/book...
components/bookmarks/browser/bookmark_model.cc:740: AsMutable(parent)->Add(node,
index);
On 2015/10/08 15:20:00, sky wrote:
> Use AddNode for the common parts.

Done.

https://chromiumcodereview.appspot.com/1379983002/diff/100001/components/book...
File components/bookmarks/browser/bookmark_undo_delegate.h (right):

https://chromiumcodereview.appspot.com/1379983002/diff/100001/components/book...
components/bookmarks/browser/bookmark_undo_delegate.h:16: class
BookmarkUndoDelegate {
On 2015/10/08 15:20:01, sky wrote:
> Add a description.

Done.

https://chromiumcodereview.appspot.com/1379983002/diff/100001/components/book...
components/bookmarks/browser/bookmark_undo_delegate.h:18: // Called when |node|
was deleted from |parent| at position |index|. The
On 2015/10/08 15:20:00, sky wrote:
> deleted->rmeoted

Done.

https://chromiumcodereview.appspot.com/1379983002/diff/100001/components/book...
components/bookmarks/browser/bookmark_undo_delegate.h:19: // delegate takes the
ownership of the deleted node.
On 2015/10/08 15:20:01, sky wrote:
> The second sentence is no longer needed as node is a scoped_ptr.

Done.

https://chromiumcodereview.appspot.com/1379983002/diff/100001/components/book...
components/bookmarks/browser/bookmark_undo_delegate.h:20: virtual void
OnBookmarkNodeDeleted(BookmarkModel* model,
On 2015/10/08 15:20:00, sky wrote:
> OnBookmarkNodeRemoved.

Done.

https://chromiumcodereview.appspot.com/1379983002/diff/100001/components/book...
components/bookmarks/browser/bookmark_undo_delegate.h:21: BookmarkUndoProvider*
provider,
On 2015/10/08 15:20:00, sky wrote:
> Rather than supply the provider on event call how a SetProvider function?

Done.

https://chromiumcodereview.appspot.com/1379983002/diff/100001/components/book...
File components/bookmarks/browser/bookmark_undo_provider.h (right):

https://chromiumcodereview.appspot.com/1379983002/diff/100001/components/book...
components/bookmarks/browser/bookmark_undo_provider.h:25: 
On 2015/10/08 15:20:01, sky wrote:
> nit: only one newline.

Done.

https://chromiumcodereview.appspot.com/1379983002/diff/100001/components/undo...
File components/undo/bookmark_undo_service.cc (right):

https://chromiumcodereview.appspot.com/1379983002/diff/100001/components/undo...
components/undo/bookmark_undo_service.cc:108: int64_t parent_node_id_;
On 2015/10/08 15:20:01, sky wrote:
> const on this and the next.

Done.

https://chromiumcodereview.appspot.com/1379983002/diff/100001/components/undo...
components/undo/bookmark_undo_service.cc:330: : model_(NULL),
scoped_observer_(this) {
On 2015/10/08 15:20:01, sky wrote:
> NULL->nullptr

Done.

[sky, 2015-10-09 16:01:20]

https://chromiumcodereview.appspot.com/1379983002/diff/100001/components/book...
File components/bookmarks/browser/bookmark_model.cc (right):

https://chromiumcodereview.appspot.com/1379983002/diff/100001/components/book...
components/bookmarks/browser/bookmark_model.cc:226:
removed_node_data.push_back(std::make_pair(permanent_node, j));
On 2015/10/08 23:37:40, jianli wrote:
> On 2015/10/08 15:20:00, sky wrote:
> > Please add test coverage here as permanent_node is wrong, you want
child_node.
> 
> I've updated BookmarkModelTest.RemoveAllUserBookmarks to validate no permanent
> nodes have been removed.

Also add coverage that the undodelegate is in fact passed the correct nodes.

https://chromiumcodereview.appspot.com/1379983002/diff/120001/components/book...
File components/bookmarks/browser/bookmark_model.cc (right):

https://chromiumcodereview.appspot.com/1379983002/diff/120001/components/book...
components/bookmarks/browser/bookmark_model.cc:223:
std::vector<base::Tuple<const BookmarkNode*, int, BookmarkNode*>>
Using a tuple certainly works, but it's not particularly readable. How about a
struct?

https://chromiumcodereview.appspot.com/1379983002/diff/120001/components/book...
File components/bookmarks/browser/bookmark_model.h (right):

https://chromiumcodereview.appspot.com/1379983002/diff/120001/components/book...
components/bookmarks/browser/bookmark_model.h:314: void
set_undo_delegate(BookmarkUndoDelegate* undo_delegate);
As this isn't inlined anymore, name is SetUndoDelegate.

https://chromiumcodereview.appspot.com/1379983002/diff/120001/components/undo...
File components/undo/bookmark_undo_service.cc (right):

https://chromiumcodereview.appspot.com/1379983002/diff/120001/components/undo...
components/undo/bookmark_undo_service.cc:136: if (!parent)
This should be a DCHECK, right?

[jianli, 2015-10-09 21:51:16]

https://chromiumcodereview.appspot.com/1379983002/diff/100001/components/book...
File components/bookmarks/browser/bookmark_model.cc (right):

https://chromiumcodereview.appspot.com/1379983002/diff/100001/components/book...
components/bookmarks/browser/bookmark_model.cc:226:
removed_node_data.push_back(std::make_pair(permanent_node, j));
On 2015/10/09 16:01:19, sky wrote:
> On 2015/10/08 23:37:40, jianli wrote:
> > On 2015/10/08 15:20:00, sky wrote:
> > > Please add test coverage here as permanent_node is wrong, you want
> child_node.
> > 
> > I've updated BookmarkModelTest.RemoveAllUserBookmarks to validate no
permanent
> > nodes have been removed.
> 
> Also add coverage that the undodelegate is in fact passed the correct nodes.

Done.

https://chromiumcodereview.appspot.com/1379983002/diff/120001/components/book...
File components/bookmarks/browser/bookmark_model.cc (right):

https://chromiumcodereview.appspot.com/1379983002/diff/120001/components/book...
components/bookmarks/browser/bookmark_model.cc:223:
std::vector<base::Tuple<const BookmarkNode*, int, BookmarkNode*>>
On 2015/10/09 16:01:19, sky wrote:
> Using a tuple certainly works, but it's not particularly readable. How about a
> struct?

Done.

https://chromiumcodereview.appspot.com/1379983002/diff/120001/components/book...
File components/bookmarks/browser/bookmark_model.h (right):

https://chromiumcodereview.appspot.com/1379983002/diff/120001/components/book...
components/bookmarks/browser/bookmark_model.h:314: void
set_undo_delegate(BookmarkUndoDelegate* undo_delegate);
On 2015/10/09 16:01:19, sky wrote:
> As this isn't inlined anymore, name is SetUndoDelegate.

Done.

https://chromiumcodereview.appspot.com/1379983002/diff/120001/components/undo...
File components/undo/bookmark_undo_service.cc (right):

https://chromiumcodereview.appspot.com/1379983002/diff/120001/components/undo...
components/undo/bookmark_undo_service.cc:136: if (!parent)
On 2015/10/09 16:01:20, sky wrote:
> This should be a DCHECK, right?

Done.

[sky, 2015-10-09 22:03:12]

LGTM

[jianli, 2015-10-09 22:07:36]

The CQ bit was checked by jianli@chromium.org

[commit-bot: I haz the power, 2015-10-09 22:10:59]

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

[commit-bot: I haz the power, 2015-10-09 22:47:46]

Committed patchset #6 (id:140001)

[commit-bot: I haz the power, 2015-10-09 22:48:48]

Patchset 6 (id:??) landed as
https://crrev.com/14436d551656d35234425ec35e08345b4368710e
Cr-Commit-Position: refs/heads/master@{#353408}