Add TitledUrlIndex for indexing arbitrary title/URL pairs

Add TitledUrlNode interface and refactor BookmarkIndex to use it

As the first step in making BookmarkIndex usable on non-bookmark items, add
a TitledUrlNode interface to represent objects like bookmarks that expose a
title and URL. BookmarkIndex now expects its items to be TitledUrlNodes and
BookmarkNode implements the TitledUrlNode interface.

BUG=630769
Committed: https://crrev.com/55324d674465179e2a4c469bff0c5b0d755dea61
Cr-Commit-Position: refs/heads/master@{#437680}

[mattreynolds, 2016-12-01 20:10:49]

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

[mattreynolds, 2016-12-01 20:10:49]

Hi Scott, PTAL.

I'll refactor BookmarkIndex in the next CL, this one just adds the new
TitledUrl* classes.

[sky, 2016-12-01 20:24:52]

https://codereview.chromium.org/2537223008/diff/1/components/bookmarks/browse...
File components/bookmarks/browser/titled_url_index.h (right):

https://codereview.chromium.org/2537223008/diff/1/components/bookmarks/browse...
components/bookmarks/browser/titled_url_index.h:29: class TitledUrlIndex {
Can you convert BookmarkIndex in place and then rename as your last step? To do
otherwise means you lose history and the review is much harder.

https://codereview.chromium.org/2537223008/diff/1/components/bookmarks/browse...
File components/bookmarks/browser/titled_url_match.h (right):

https://codereview.chromium.org/2537223008/diff/1/components/bookmarks/browse...
components/bookmarks/browser/titled_url_match.h:18: struct TitledUrlMatch {
Same comment here.

[mattreynolds, 2016-12-01 23:27:27]

Description was changed from

==========
Add TitledUrlIndex for indexing arbitrary title/URL pairs

Factor out the logic in BookmarkIndex for indexing and matching of bookmark
titles and URLs so it can be applied to non-bookmark items. In a future CL,
BookmarkIndex will be made a subclass of TitledUrlIndex.

BUG=630769
==========

to

==========
Add TitledUrlNode interface and refactor BookmarkIndex to use it

As the first step in making BookmarkIndex usable on non-bookmark items, add
a TitledUrlNode interface to represent objects like bookmarks that expose a
title and URL. BookmarkIndex now expects its items to be TitledUrlNodes and
BookmarkNode implements the TitledUrlNode interface.

BUG=630769
==========

[mattreynolds, 2016-12-01 23:33:19]

Hi Scott, please take another look

https://codereview.chromium.org/2537223008/diff/1/components/bookmarks/browse...
File components/bookmarks/browser/titled_url_index.h (right):

https://codereview.chromium.org/2537223008/diff/1/components/bookmarks/browse...
components/bookmarks/browser/titled_url_index.h:29: class TitledUrlIndex {
On 2016/12/01 20:24:51, sky wrote:
> Can you convert BookmarkIndex in place and then rename as your last step? To
do
> otherwise means you lose history and the review is much harder.

Done.

https://codereview.chromium.org/2537223008/diff/1/components/bookmarks/browse...
File components/bookmarks/browser/titled_url_match.h (right):

https://codereview.chromium.org/2537223008/diff/1/components/bookmarks/browse...
components/bookmarks/browser/titled_url_match.h:18: struct TitledUrlMatch {
On 2016/12/01 20:24:51, sky wrote:
> Same comment here.

Done.

[sky, 2016-12-02 00:21:24]

https://codereview.chromium.org/2537223008/diff/20001/components/bookmarks/br...
File components/bookmarks/browser/bookmark_client.h (right):

https://codereview.chromium.org/2537223008/diff/20001/components/bookmarks/br...
components/bookmarks/browser/bookmark_client.h:33: class BookmarkClient {
Why do you need to touch this class? I was assuming you would be changing the
index not to take a client but something that has the single function used from
this class. Also, as BookmarkNodes are TitledUrlNode you shouldn't need to
change these typedefs, right?

https://codereview.chromium.org/2537223008/diff/20001/components/bookmarks/br...
File components/bookmarks/browser/bookmark_index.h (right):

https://codereview.chromium.org/2537223008/diff/20001/components/bookmarks/br...
components/bookmarks/browser/bookmark_index.h:51: typedef std::vector<const
TitledUrlNode*> Nodes;
While you are changing this code update to using.

[mattreynolds, 2016-12-02 21:02:06]

https://codereview.chromium.org/2537223008/diff/20001/components/bookmarks/br...
File components/bookmarks/browser/bookmark_client.h (right):

https://codereview.chromium.org/2537223008/diff/20001/components/bookmarks/br...
components/bookmarks/browser/bookmark_client.h:33: class BookmarkClient {
On 2016/12/02 00:21:24, sky wrote:
> Why do you need to touch this class? I was assuming you would be changing the
> index not to take a client but something that has the single function used
from
> this class. Also, as BookmarkNodes are TitledUrlNode you shouldn't need to
> change these typedefs, right?

Even if I factor out the sorting logic, at some point we still need to get from
TitledUrlNode to whatever BookmarkClient::GetTypedCountForNode expects. I felt
the cleanest solution was to modify GetTypedCountForNode to take
set<TitledUrlNode>. I think it would also be reasonable for it to take set<GURL>
since the only BookmarkNode method it needs is the URL accessor. WDYT?

https://codereview.chromium.org/2537223008/diff/20001/components/bookmarks/br...
File components/bookmarks/browser/bookmark_index.h (right):

https://codereview.chromium.org/2537223008/diff/20001/components/bookmarks/br...
components/bookmarks/browser/bookmark_index.h:51: typedef std::vector<const
TitledUrlNode*> Nodes;
On 2016/12/02 00:21:24, sky wrote:
> While you are changing this code update to using.

Done.

https://codereview.chromium.org/2537223008/diff/40001/components/bookmarks/br...
File components/bookmarks/browser/typed_count_sorter.cc (right):

https://codereview.chromium.org/2537223008/diff/40001/components/bookmarks/br...
components/bookmarks/browser/typed_count_sorter.cc:49:
bookmark_matches.insert((BookmarkNode*)titled_url_node);
Any suggestions for getting rid of the cast?

[sky, 2016-12-02 21:30:29]

https://codereview.chromium.org/2537223008/diff/20001/components/bookmarks/br...
File components/bookmarks/browser/bookmark_client.h (right):

https://codereview.chromium.org/2537223008/diff/20001/components/bookmarks/br...
components/bookmarks/browser/bookmark_client.h:33: class BookmarkClient {
On 2016/12/02 21:02:06, mattreynolds wrote:
> On 2016/12/02 00:21:24, sky wrote:
> > Why do you need to touch this class? I was assuming you would be changing
the
> > index not to take a client but something that has the single function used
> from
> > this class. Also, as BookmarkNodes are TitledUrlNode you shouldn't need to
> > change these typedefs, right?
> 
> Even if I factor out the sorting logic, at some point we still need to get
from
> TitledUrlNode to whatever BookmarkClient::GetTypedCountForNode expects. I felt
> the cleanest solution was to modify GetTypedCountForNode to take
> set<TitledUrlNode>. I think it would also be reasonable for it to take
set<GURL>
> since the only BookmarkNode method it needs is the URL accessor. WDYT?

BookmarkClient should be able to assume it is operating on BookmarkNodes, not
TitledUrlNode. But I agree that GetTypedCountForNodes doesn't need a node, just
the url. So, I'm fine with changing it to take a set<url> I suggest you do that
patch first in hopes of more focused smaller patches.

[mattreynolds, 2016-12-06 20:54:24]

On 2016/12/02 21:30:29, sky wrote:
>
https://codereview.chromium.org/2537223008/diff/20001/components/bookmarks/br...
> File components/bookmarks/browser/bookmark_client.h (right):
> 
>
https://codereview.chromium.org/2537223008/diff/20001/components/bookmarks/br...
> components/bookmarks/browser/bookmark_client.h:33: class BookmarkClient {
> On 2016/12/02 21:02:06, mattreynolds wrote:
> > On 2016/12/02 00:21:24, sky wrote:
> > > Why do you need to touch this class? I was assuming you would be changing
> the
> > > index not to take a client but something that has the single function used
> > from
> > > this class. Also, as BookmarkNodes are TitledUrlNode you shouldn't need to
> > > change these typedefs, right?
> > 
> > Even if I factor out the sorting logic, at some point we still need to get
> from
> > TitledUrlNode to whatever BookmarkClient::GetTypedCountForNode expects. I
felt
> > the cleanest solution was to modify GetTypedCountForNode to take
> > set<TitledUrlNode>. I think it would also be reasonable for it to take
> set<GURL>
> > since the only BookmarkNode method it needs is the URL accessor. WDYT?
> 
> BookmarkClient should be able to assume it is operating on BookmarkNodes, not
> TitledUrlNode. But I agree that GetTypedCountForNodes doesn't need a node,
just
> the url. So, I'm fine with changing it to take a set<url> I suggest you do
that
> patch first in hopes of more focused smaller patches.

Once the BookmarkClient changes land I'll rebase this.

I split the remaining changes into three CLs:

* Add TitledUrlNode interface and refactor BookmarkIndex/BookmarkMatch to use it
(this CL)
* Factor out the bookmark-specific SortMatches logic into TypedCountSorter,
which implements the TitledUrlNodeSorter interface
* Rename BookmarkIndex/BookmarkMatch to TitledUrlIndex/TitledUrlMatch (just
renaming and fixing comments, no functional changes)

[sky, 2016-12-06 23:19:40]

SG send email from the patches when they are ready for  review.

On Tue, Dec 6, 2016 at 12:54 PM,  <mattreynolds@chromium.org> wrote:
> On 2016/12/02 21:30:29, sky wrote:
>>
>
https://codereview.chromium.org/2537223008/diff/20001/components/bookmarks/br...
>> File components/bookmarks/browser/bookmark_client.h (right):
>>
>>
>
https://codereview.chromium.org/2537223008/diff/20001/components/bookmarks/br...
>> components/bookmarks/browser/bookmark_client.h:33: class BookmarkClient {
>> On 2016/12/02 21:02:06, mattreynolds wrote:
>> > On 2016/12/02 00:21:24, sky wrote:
>> > > Why do you need to touch this class? I was assuming you would be
>> > > changing
>> the
>> > > index not to take a client but something that has the single function
>> > > used
>> > from
>> > > this class. Also, as BookmarkNodes are TitledUrlNode you shouldn't
>> > > need to
>> > > change these typedefs, right?
>> >
>> > Even if I factor out the sorting logic, at some point we still need to
>> > get
>> from
>> > TitledUrlNode to whatever BookmarkClient::GetTypedCountForNode expects.
>> > I
> felt
>> > the cleanest solution was to modify GetTypedCountForNode to take
>> > set<TitledUrlNode>. I think it would also be reasonable for it to take
>> set<GURL>
>> > since the only BookmarkNode method it needs is the URL accessor. WDYT?
>>
>> BookmarkClient should be able to assume it is operating on BookmarkNodes,
>> not
>> TitledUrlNode. But I agree that GetTypedCountForNodes doesn't need a node,
> just
>> the url. So, I'm fine with changing it to take a set<url> I suggest you do
> that
>> patch first in hopes of more focused smaller patches.
>
> Once the BookmarkClient changes land I'll rebase this.
>
> I split the remaining changes into three CLs:
>
> * Add TitledUrlNode interface and refactor BookmarkIndex/BookmarkMatch to
> use it
> (this CL)
> * Factor out the bookmark-specific SortMatches logic into TypedCountSorter,
> which implements the TitledUrlNodeSorter interface
> * Rename BookmarkIndex/BookmarkMatch to TitledUrlIndex/TitledUrlMatch (just
> renaming and fixing comments, no functional changes)
>
> https://codereview.chromium.org/2537223008/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[mattreynolds, 2016-12-07 19:31:36]

Hi Scott, this is ready for review now, PTAL

[sky, 2016-12-07 21:52:26]

LGTM

[mattreynolds, 2016-12-07 22:00:38]

mattreynolds@chromium.org changed reviewers:
+ mpearson@chromium.org

[mattreynolds, 2016-12-07 22:00:39]

Hi Mark, please review changes in //components/omnibox

[Mark P, 2016-12-07 22:53:33]

components/omnibox/browser/bookmark_provider.cc lgtm

--mark

[mattreynolds, 2016-12-07 23:50:08]

The CQ bit was checked by mattreynolds@chromium.org

[commit-bot: I haz the power, 2016-12-07 23:50:43]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-08 00:17:03]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-08 00:17:03]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[mattreynolds, 2016-12-08 21:09:40]

sky@, mpearson@, could you take another look? I needed to add another method to
the TitledUrlNode interface to fix the Android bookmark bridge.

https://codereview.chromium.org/2537223008/diff/100001/chrome/browser/android...
File chrome/browser/android/bookmarks/bookmark_bridge.cc (right):

https://codereview.chromium.org/2537223008/diff/100001/chrome/browser/android...
chrome/browser/android/bookmarks/bookmark_bridge.cc:613: env, j_list,
node->GetTitledUrlNodeId(), Type::URL,
I missed this one when looking for users of BookmarkMatch. We don't have any
easy way to get the BookmarkNode from the TitledUrlNode here, so I added an ID
accessor to the interface.

We can safely replace the type() call with Type::URL because BookmarkClient
already prevents us from adding any non-URL nodes to the index.

[sky, 2016-12-08 23:01:07]

https://codereview.chromium.org/2537223008/diff/100001/chrome/browser/android...
File chrome/browser/android/bookmarks/bookmark_bridge.cc (right):

https://codereview.chromium.org/2537223008/diff/100001/chrome/browser/android...
chrome/browser/android/bookmarks/bookmark_bridge.cc:613: env, j_list,
node->GetTitledUrlNodeId(), Type::URL,
On 2016/12/08 21:09:39, mattreynolds wrote:
> I missed this one when looking for users of BookmarkMatch. We don't have any
> easy way to get the BookmarkNode from the TitledUrlNode here, so I added an ID
> accessor to the interface.
> 
> We can safely replace the type() call with Type::URL because BookmarkClient
> already prevents us from adding any non-URL nodes to the index.

Is there a reason this code can't static cast to BookmarkNode? Will this code
ever be used with anything else?

https://codereview.chromium.org/2537223008/diff/100001/components/bookmarks/b...
File components/bookmarks/browser/bookmark_index.cc (left):

https://codereview.chromium.org/2537223008/diff/100001/components/bookmarks/b...
components/bookmarks/browser/bookmark_index.cc:110: if (!node->is_url())
This bothered me last time, but I'll ask now. The new code isn't quite the same.
Could we make it so BookmarkModel *only* adds nodes that are is_url() here? That
way this you wouldn't need the conditional.

[Mark P, 2016-12-08 23:12:10]

On 2016/12/08 21:09:40, mattreynolds wrote:
> sky@, mpearson@, could you take another look? I needed to add another method
to
> the TitledUrlNode interface to fix the Android bookmark bridge.
My lgtm still applies because it appears you haven't updated the only file I
reviewed (components/omnibox/browser/bookmark_provider.cc).  Please, of course,
continue to make sure that the stuff in the other files that I'm not reviewing
doesn't result in any functionality change.

--mark

> 
>
https://codereview.chromium.org/2537223008/diff/100001/chrome/browser/android...
> File chrome/browser/android/bookmarks/bookmark_bridge.cc (right):
> 
>
https://codereview.chromium.org/2537223008/diff/100001/chrome/browser/android...
> chrome/browser/android/bookmarks/bookmark_bridge.cc:613: env, j_list,
> node->GetTitledUrlNodeId(), Type::URL,
> I missed this one when looking for users of BookmarkMatch. We don't have any
> easy way to get the BookmarkNode from the TitledUrlNode here, so I added an ID
> accessor to the interface.
> 
> We can safely replace the type() call with Type::URL because BookmarkClient
> already prevents us from adding any non-URL nodes to the index.

[mattreynolds, 2016-12-09 02:01:15]

https://codereview.chromium.org/2537223008/diff/100001/chrome/browser/android...
File chrome/browser/android/bookmarks/bookmark_bridge.cc (right):

https://codereview.chromium.org/2537223008/diff/100001/chrome/browser/android...
chrome/browser/android/bookmarks/bookmark_bridge.cc:613: env, j_list,
node->GetTitledUrlNodeId(), Type::URL,
On 2016/12/08 23:01:07, sky wrote:
> On 2016/12/08 21:09:39, mattreynolds wrote:
> > I missed this one when looking for users of BookmarkMatch. We don't have any
> > easy way to get the BookmarkNode from the TitledUrlNode here, so I added an
ID
> > accessor to the interface.
> > 
> > We can safely replace the type() call with Type::URL because BookmarkClient
> > already prevents us from adding any non-URL nodes to the index.
> 
> Is there a reason this code can't static cast to BookmarkNode? Will this code
> ever be used with anything else?

It's safe, for some reason I thought such casts were discouraged.

https://codereview.chromium.org/2537223008/diff/100001/components/bookmarks/b...
File components/bookmarks/browser/bookmark_index.cc (left):

https://codereview.chromium.org/2537223008/diff/100001/components/bookmarks/b...
components/bookmarks/browser/bookmark_index.cc:110: if (!node->is_url())
On 2016/12/08 23:01:07, sky wrote:
> This bothered me last time, but I'll ask now. The new code isn't quite the
same.
> Could we make it so BookmarkModel *only* adds nodes that are is_url() here?
That
> way this you wouldn't need the conditional.

BookmarkModel::AddNodeToInternalMaps already checks is_url before it adds new
nodes to the index, so the is_valid check here is unnecessary when used with
BookmarkModel. There are cases where BookmarkModel doesn't check is_url before
adding if it had previously just removed the same node from the index.

I'll remove the is_valid checks.

[sky, 2016-12-09 02:11:28]

LGTM

[mattreynolds, 2016-12-09 18:57:03]

The CQ bit was checked by mattreynolds@chromium.org

[mattreynolds, 2016-12-09 18:57:03]

The patchset sent to the CQ was uploaded after l-g-t-m from
mpearson@chromium.org
Link to the patchset: https://codereview.chromium.org/2537223008/#ps120001
(title: "static_cast, remove is_valid checks")

[commit-bot: I haz the power, 2016-12-09 18:57:58]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-09 19:41:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-09 19:41:48]

Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)

[mattreynolds, 2016-12-09 20:09:02]

The CQ bit was checked by mattreynolds@chromium.org

[mattreynolds, 2016-12-09 20:09:02]

The patchset sent to the CQ was uploaded after l-g-t-m from
mpearson@chromium.org, sky@chromium.org
Link to the patchset: https://codereview.chromium.org/2537223008/#ps140001
(title: "const")

[commit-bot: I haz the power, 2016-12-09 20:10:14]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-09 23:07:46]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1481314142186180,
"parent_rev": "2f31e16b4d80cecedb7070812deafbe1fa8749d1", "commit_rev":
"c50122d6258dbb171c9c097b3d415069c7e66e50"}

[commit-bot: I haz the power, 2016-12-09 23:08:12]

Description was changed from

==========
Add TitledUrlNode interface and refactor BookmarkIndex to use it

As the first step in making BookmarkIndex usable on non-bookmark items, add
a TitledUrlNode interface to represent objects like bookmarks that expose a
title and URL. BookmarkIndex now expects its items to be TitledUrlNodes and
BookmarkNode implements the TitledUrlNode interface.

BUG=630769
==========

to

==========
Add TitledUrlNode interface and refactor BookmarkIndex to use it

As the first step in making BookmarkIndex usable on non-bookmark items, add
a TitledUrlNode interface to represent objects like bookmarks that expose a
title and URL. BookmarkIndex now expects its items to be TitledUrlNodes and
BookmarkNode implements the TitledUrlNode interface.

BUG=630769

Review-Url: https://codereview.chromium.org/2537223008
==========

[commit-bot: I haz the power, 2016-12-09 23:08:13]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-12-12 14:57:57]

Description was changed from

==========
Add TitledUrlNode interface and refactor BookmarkIndex to use it

As the first step in making BookmarkIndex usable on non-bookmark items, add
a TitledUrlNode interface to represent objects like bookmarks that expose a
title and URL. BookmarkIndex now expects its items to be TitledUrlNodes and
BookmarkNode implements the TitledUrlNode interface.

BUG=630769

Review-Url: https://codereview.chromium.org/2537223008
==========

to

==========
Add TitledUrlNode interface and refactor BookmarkIndex to use it

As the first step in making BookmarkIndex usable on non-bookmark items, add
a TitledUrlNode interface to represent objects like bookmarks that expose a
title and URL. BookmarkIndex now expects its items to be TitledUrlNodes and
BookmarkNode implements the TitledUrlNode interface.

BUG=630769

Committed: https://crrev.com/55324d674465179e2a4c469bff0c5b0d755dea61
Cr-Commit-Position: refs/heads/master@{#437680}
==========

[commit-bot: I haz the power, 2016-12-12 14:57:58]

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