Make sure to update the BookmarkIndex on a URL Change

Make sure to update the BookmarkIndex on a URL Change

TEST=components_unittests --gtest_filter=BookmarkIndexTest.ChangeURL
R=sky@chromium.org
BUG=429042
Committed: https://crrev.com/f4fde32f01dc38f613d5289d532e870a90c2229d
Cr-Commit-Position: refs/heads/master@{#302634}

[sky, 2014-11-03 20:44:46]

https://codereview.chromium.org/688473005/diff/1/components/bookmarks/browser...
File components/bookmarks/browser/bookmark_model.cc (right):

https://codereview.chromium.org/688473005/diff/1/components/bookmarks/browser...
components/bookmarks/browser/bookmark_model.cc:374:
RemoveNodeFromURLSet(mutable_node);
I think it would be less error prone if RemoveNodeFromURLSet did this. The name
would have to be updated then, maybe RemoveNodeFromInternalMaps or something.

A similar AddNodeToInternalMaps would be nice.

[danduong, 2014-11-04 01:48:06]

https://codereview.chromium.org/688473005/diff/40001/components/bookmarks/bro...
File components/bookmarks/browser/bookmark_model.cc (right):

https://codereview.chromium.org/688473005/diff/40001/components/bookmarks/bro...
components/bookmarks/browser/bookmark_model.cc:883: if (node->type() ==
BookmarkNode::URL) {
Is this right? It seems to match the previous logic closest.

https://codereview.chromium.org/688473005/diff/40001/components/bookmarks/bro...
components/bookmarks/browser/bookmark_model.cc:885: AddNodeToInternalMaps(node);
Moving this here actually makes you hold on to url_lock_ slightly longer for the
index_->Add call. Is that OK?

[sky, 2014-11-04 03:41:45]

https://codereview.chromium.org/688473005/diff/40001/components/bookmarks/bro...
File components/bookmarks/browser/bookmark_model.cc (right):

https://codereview.chromium.org/688473005/diff/40001/components/bookmarks/bro...
components/bookmarks/browser/bookmark_model.cc:883: if (node->type() ==
BookmarkNode::URL) {
On 2014/11/04 01:48:06, danduong wrote:
> Is this right? It seems to match the previous logic closest.

I don't think there is a good reason why this is after notifying observers. I
think you should move your code to 879ish. And yes, this looks right.

https://codereview.chromium.org/688473005/diff/40001/components/bookmarks/bro...
components/bookmarks/browser/bookmark_model.cc:885: AddNodeToInternalMaps(node);
On 2014/11/04 01:48:05, danduong wrote:
> Moving this here actually makes you hold on to url_lock_ slightly longer for
the
> index_->Add call. Is that OK?

I don't think it'll make much of a difference.

[danduong, 2014-11-04 03:52:48]

https://codereview.chromium.org/688473005/diff/40001/components/bookmarks/bro...
File components/bookmarks/browser/bookmark_model.cc (right):

https://codereview.chromium.org/688473005/diff/40001/components/bookmarks/bro...
components/bookmarks/browser/bookmark_model.cc:883: if (node->type() ==
BookmarkNode::URL) {
On 2014/11/04 03:41:45, sky wrote:
> On 2014/11/04 01:48:06, danduong wrote:
> > Is this right? It seems to match the previous logic closest.
> 
> I don't think there is a good reason why this is after notifying observers. I
> think you should move your code to 879ish. And yes, this looks right.

Done.

[sky, 2014-11-04 14:21:00]

https://codereview.chromium.org/688473005/diff/60001/components/bookmarks/bro...
File components/bookmarks/browser/bookmark_model.cc (right):

https://codereview.chromium.org/688473005/diff/60001/components/bookmarks/bro...
components/bookmarks/browser/bookmark_model.cc:894: index_->Add(node);
Can you DCHECK here that the lock is held? I'm pretty sure there is a function
you can call for testing it.

[danduong, 2014-11-04 14:41:42]

https://codereview.chromium.org/688473005/diff/60001/components/bookmarks/bro...
File components/bookmarks/browser/bookmark_model.cc (right):

https://codereview.chromium.org/688473005/diff/60001/components/bookmarks/bro...
components/bookmarks/browser/bookmark_model.cc:894: index_->Add(node);
On 2014/11/04 14:21:00, sky wrote:
> Can you DCHECK here that the lock is held? I'm pretty sure there is a function
> you can call for testing it.

Done.

[sky, 2014-11-04 17:43:02]

LGTM

[danduong, 2014-11-04 17:44:52]

The CQ bit was checked by danduong@chromium.org

[commit-bot: I haz the power, 2014-11-04 17:45:45]

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

[commit-bot: I haz the power, 2014-11-04 18:57:08]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2014-11-04 18:58:06]

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

[danduong, 2014-12-19 17:24:54]

Patchset #7 (id:120001) has been deleted