Bring up of the server side full text search.

Bring up of the server side full text search.

This class is used to perform a full text search on bookmarks, powered by the server side. The superclass will be used for other services in subsequent CLs.

BUG=None

Committed: https://crrev.com/f0e1e779e3acf7e18093550dc775e44b812a3286
Cr-Commit-Position: refs/heads/master@{#294571}

[noyau (Ping after 24h), 2014-09-04 17:48:54]

Patchset #3 (id:40001) has been deleted

[noyau (Ping after 24h), 2014-09-04 17:53:23]

noyau@chromium.org changed reviewers:
+ blundell@chromium.org, yfriedman@google.com

[noyau (Ping after 24h), 2014-09-04 21:56:27]

noyau@chromium.org changed reviewers:
+ yfriedman@chromium.org
- yfriedman@google.com

[Yaron, 2014-09-05 04:37:52]

https://codereview.chromium.org/538903003/diff/80001/components/enhanced_book...
File components/enhanced_bookmarks/bookmark_server_search_service.cc (right):

https://codereview.chromium.org/538903003/diff/80001/components/enhanced_book...
components/enhanced_bookmarks/bookmark_server_search_service.cc:76:
DCHECK(*should_notify);
this doesn't match the header. It can't actually ever be false. It's also unused

https://codereview.chromium.org/538903003/diff/80001/components/enhanced_book...
components/enhanced_bookmarks/bookmark_server_search_service.cc:103: void
BookmarkServerSearchService::BookmarkNodeAdded(BookmarkModel* model,
Why is a bookmark removal ignorable? I guess the UI handles the fact the
bookmark id doesn't correspond to an active bookmark?

https://codereview.chromium.org/538903003/diff/80001/components/enhanced_book...
File components/enhanced_bookmarks/bookmark_server_search_service.h (right):

https://codereview.chromium.org/538903003/diff/80001/components/enhanced_book...
components/enhanced_bookmarks/bookmark_server_search_service.h:56: // The search
data, a map from query to a vector of starsid.
stars.id

https://codereview.chromium.org/538903003/diff/80001/components/enhanced_book...
File components/enhanced_bookmarks/bookmark_server_service.cc (right):

https://codereview.chromium.org/538903003/diff/80001/components/enhanced_book...
components/enhanced_bookmarks/bookmark_server_service.cc:18: const std::string
kScope("https://www.googleapis.com/auth/chromesync");
use GaiaConstants::kChromeSyncOAuth2Scope instead.

https://codereview.chromium.org/538903003/diff/80001/components/enhanced_book...
File components/enhanced_bookmarks/bookmark_server_service.h (right):

https://codereview.chromium.org/538903003/diff/80001/components/enhanced_book...
components/enhanced_bookmarks/bookmark_server_service.h:4: #ifndef
COMPONENTS_ENHANCED_BOOKMARKS_BOOKMARK_SERVER_SERVICE_H_
nit: insert ws before

https://codereview.chromium.org/538903003/diff/80001/components/enhanced_book...
components/enhanced_bookmarks/bookmark_server_service.h:34: // stores the maps
necessary to translate the response from stars_id to
stars.id

https://codereview.chromium.org/538903003/diff/80001/components/enhanced_book...
File components/enhanced_bookmarks/proto/search.proto (right):

https://codereview.chromium.org/538903003/diff/80001/components/enhanced_book...
components/enhanced_bookmarks/proto/search.proto:6: option optimize_for =
LITE_RUNTIME;
I suspect this should be referenced in                                          
                                                                                
  
components/enhanced_bookmarks/proto/BUILD.gn

https://codereview.chromium.org/538903003/diff/80001/components/enhanced_book...
components/enhanced_bookmarks/proto/search.proto:10: message CorpusSearchResult
{
Should have some brief comments.

[Yaron, 2014-09-05 04:40:28]

yfriedman@chromium.org changed reviewers:
+ mcolbert@chromium.org

[Yaron, 2014-09-05 04:40:29]

+mcolbert just for high-level verification that this matches expectations from
server side.

[blundell, 2014-09-05 09:11:07]

I defer to the others for the review here.

[Mark, 2014-09-05 18:23:14]

https://codereview.chromium.org/538903003/diff/80001/components/enhanced_book...
File components/enhanced_bookmarks/bookmark_server_search_service.cc (right):

https://codereview.chromium.org/538903003/diff/80001/components/enhanced_book...
components/enhanced_bookmarks/bookmark_server_search_service.cc:63: GURL
url(kSearchUrl);
I'm assuming the bearer is being set magically somewhere in here?

https://codereview.chromium.org/538903003/diff/80001/components/enhanced_book...
components/enhanced_bookmarks/bookmark_server_search_service.cc:65: url =
net::AppendQueryParameter(url, "q", current_query_);
Make sure to add a v=[version id] here.

https://codereview.chromium.org/538903003/diff/80001/components/enhanced_book...
components/enhanced_bookmarks/bookmark_server_search_service.cc:90: if
(!clip_id.length())
You also get snippets back as to why clips matched to the search string,
including a modified title with <em> or <b> tags around the relevant search
term.  This is fairly useful because with the deep-text matching, some of these
results may appear to be random.

[Yaron, 2014-09-05 18:26:52]

https://codereview.chromium.org/538903003/diff/80001/components/enhanced_book...
File components/enhanced_bookmarks/bookmark_server_search_service.cc (right):

https://codereview.chromium.org/538903003/diff/80001/components/enhanced_book...
components/enhanced_bookmarks/bookmark_server_search_service.cc:63: GURL
url(kSearchUrl);
On 2014/09/05 18:23:14, Mark wrote:
> I'm assuming the bearer is being set magically somewhere in here?

BookmarkServerService::OnGetTokenSuccess adds it after calling here:
https://code.google.com/p/chromium/codesearch#chromium/src/components/enhance...

although it's true that access_token isn't used

[noyau (Ping after 24h), 2014-09-08 09:44:53]

Patchset #5 (id:100001) has been deleted

[noyau (Ping after 24h), 2014-09-08 09:46:49]

+tim@ for the DEPS inclusion of google_apis/gaia

https://codereview.chromium.org/538903003/diff/80001/components/enhanced_book...
File components/enhanced_bookmarks/bookmark_server_search_service.cc (right):

https://codereview.chromium.org/538903003/diff/80001/components/enhanced_book...
components/enhanced_bookmarks/bookmark_server_search_service.cc:63: GURL
url(kSearchUrl);
On 2014/09/05 18:26:52, Yaron wrote:
> On 2014/09/05 18:23:14, Mark wrote:
> > I'm assuming the bearer is being set magically somewhere in here?
> 
> BookmarkServerService::OnGetTokenSuccess adds it after calling here:
>
https://code.google.com/p/chromium/codesearch#chromium/src/components/enhance...
> 
> although it's true that access_token isn't used

Ah, good catch. This argument is a remnant of ancient times, where it was used.
It's no longer necessary to pass it on. Removed.

https://codereview.chromium.org/538903003/diff/80001/components/enhanced_book...
components/enhanced_bookmarks/bookmark_server_search_service.cc:65: url =
net::AppendQueryParameter(url, "q", current_query_);
On 2014/09/05 18:23:14, Mark wrote:
> Make sure to add a v=[version id] here.

Yup, once the code to get a revision lands. I'll probably do it in the
superclass however.

https://codereview.chromium.org/538903003/diff/80001/components/enhanced_book...
components/enhanced_bookmarks/bookmark_server_search_service.cc:76:
DCHECK(*should_notify);
On 2014/09/05 04:37:52, Yaron wrote:
> this doesn't match the header. It can't actually ever be false. It's also
unused

For this class should_notify is always true, and this asserts that invariant.

The other subclass may be called with should_notify set to false.

https://codereview.chromium.org/538903003/diff/80001/components/enhanced_book...
components/enhanced_bookmarks/bookmark_server_search_service.cc:90: if
(!clip_id.length())
On 2014/09/05 18:23:14, Mark wrote:
> You also get snippets back as to why clips matched to the search string,
> including a modified title with <em> or <b> tags around the relevant search
> term.  This is fairly useful because with the deep-text matching, some of
these
> results may appear to be random.

Acknowledged.

https://codereview.chromium.org/538903003/diff/80001/components/enhanced_book...
components/enhanced_bookmarks/bookmark_server_search_service.cc:103: void
BookmarkServerSearchService::BookmarkNodeAdded(BookmarkModel* model,
On 2014/09/05 04:37:52, Yaron wrote:
> Why is a bookmark removal ignorable? I guess the UI handles the fact the
> bookmark id doesn't correspond to an active bookmark?
This is not ignored, this is catched by the superclass and updates the map. The
method resultForQuery filters them out if needed.

https://codereview.chromium.org/538903003/diff/80001/components/enhanced_book...
File components/enhanced_bookmarks/bookmark_server_search_service.h (right):

https://codereview.chromium.org/538903003/diff/80001/components/enhanced_book...
components/enhanced_bookmarks/bookmark_server_search_service.h:56: // The search
data, a map from query to a vector of starsid.
On 2014/09/05 04:37:52, Yaron wrote:
> stars.id

Done.

https://codereview.chromium.org/538903003/diff/80001/components/enhanced_book...
File components/enhanced_bookmarks/bookmark_server_service.cc (right):

https://codereview.chromium.org/538903003/diff/80001/components/enhanced_book...
components/enhanced_bookmarks/bookmark_server_service.cc:18: const std::string
kScope("https://www.googleapis.com/auth/chromesync");
On 2014/09/05 04:37:52, Yaron wrote:
> use GaiaConstants::kChromeSyncOAuth2Scope instead.

Done.

https://codereview.chromium.org/538903003/diff/80001/components/enhanced_book...
File components/enhanced_bookmarks/bookmark_server_service.h (right):

https://codereview.chromium.org/538903003/diff/80001/components/enhanced_book...
components/enhanced_bookmarks/bookmark_server_service.h:4: #ifndef
COMPONENTS_ENHANCED_BOOKMARKS_BOOKMARK_SERVER_SERVICE_H_
On 2014/09/05 04:37:52, Yaron wrote:
> nit: insert ws before

Done.

https://codereview.chromium.org/538903003/diff/80001/components/enhanced_book...
components/enhanced_bookmarks/bookmark_server_service.h:34: // stores the maps
necessary to translate the response from stars_id to
On 2014/09/05 04:37:52, Yaron wrote:
> stars.id

Done.

https://codereview.chromium.org/538903003/diff/80001/components/enhanced_book...
File components/enhanced_bookmarks/proto/search.proto (right):

https://codereview.chromium.org/538903003/diff/80001/components/enhanced_book...
components/enhanced_bookmarks/proto/search.proto:6: option optimize_for =
LITE_RUNTIME;
On 2014/09/05 04:37:52, Yaron wrote:
> I suspect this should be referenced in                                        
 
>                                                                               
 
>   
> components/enhanced_bookmarks/proto/BUILD.gn     

Done.

https://codereview.chromium.org/538903003/diff/80001/components/enhanced_book...
components/enhanced_bookmarks/proto/search.proto:10: message CorpusSearchResult
{
On 2014/09/05 04:37:52, Yaron wrote:
> Should have some brief comments.

Done.

[Yaron, 2014-09-08 23:46:32]

lgtm

[Yaron, 2014-09-08 23:46:55]

On 2014/09/08 23:46:32, Yaron wrote:
> lgtm

(please add tracking bug for these CLs)

[noyau (Ping after 24h), 2014-09-09 08:45:51]

The CQ bit was checked by noyau@chromium.org

[noyau (Ping after 24h), 2014-09-09 08:45:56]

The CQ bit was unchecked by noyau@chromium.org

[noyau (Ping after 24h), 2014-09-09 08:46:26]

noyau@chromium.org changed reviewers:
+ tim@chromium.org

[blundell, 2014-09-09 08:57:07]

The CQ bit was checked by blundell@chromium.org

[commit-bot: I haz the power, 2014-09-09 08:57:54]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/noyau@chromium.org/538903003/140001

[commit-bot: I haz the power, 2014-09-09 09:47:50]

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

[commit-bot: I haz the power, 2014-09-09 09:47:51]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[noyau (Ping after 24h), 2014-09-09 11:02:41]

+tim@ for the DEPS additions of components/signin and google_apis/gaia

[tim (not reviewing), 2014-09-11 20:29:20]

On 2014/09/09 11:02:41, noyau wrote:
> +tim@ for the DEPS additions of components/signin and google_apis/gaia

DEPS lgtm

[noyau (Ping after 24h), 2014-09-12 09:05:25]

The CQ bit was checked by noyau@chromium.org

[commit-bot: I haz the power, 2014-09-12 09:06:24]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/538903003/160001

[commit-bot: I haz the power, 2014-09-12 10:20:29]

Committed patchset #7 (id:160001) as 67bd2d4cb7b7843b32a4f764afed882e95c5eb9e

[commit-bot: I haz the power, 2014-09-12 10:22:32]

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