Implement people search.

Implement people search.
Implement a people search provider that uses the Google+ People Search endpoint.

R=xiyuan@chromium.org
BUG=267208
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=221642

[xiyuan, 2013-09-04 23:15:41]

https://codereview.chromium.org/23874015/diff/1/chrome/browser/about_flags.cc
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/23874015/diff/1/chrome/browser/about_flags.cc...
chrome/browser/about_flags.cc:1219: kOsDesktop,
Note that app list does not exist for linux build.

https://codereview.chromium.org/23874015/diff/1/chrome/browser/ui/app_list/se...
File chrome/browser/ui/app_list/search/people/people_provider.cc (right):

https://codereview.chromium.org/23874015/diff/1/chrome/browser/ui/app_list/se...
chrome/browser/ui/app_list/search/people/people_provider.cc:44: std:: string
GetFirstValue(const ListValue& list, const char key[]) {
remove space after "::"

https://codereview.chromium.org/23874015/diff/1/chrome/browser/ui/app_list/se...
chrome/browser/ui/app_list/search/people/people_provider.cc:47: std::string();
return std::string();

https://codereview.chromium.org/23874015/diff/1/chrome/browser/ui/app_list/se...
chrome/browser/ui/app_list/search/people/people_provider.cc:51: return
std::string();;
nit: nuke one ';'

https://codereview.chromium.org/23874015/diff/1/chrome/browser/ui/app_list/se...
chrome/browser/ui/app_list/search/people/people_provider.cc:89:
StartThrottledQuery(base::Bind(&PeopleProvider::RequestAccessToken,
Is it possible to reuse the previously fetched |access_token_|?

https://codereview.chromium.org/23874015/diff/1/chrome/browser/ui/app_list/se...
chrome/browser/ui/app_list/search/people/people_provider.cc:131: void
PeopleProvider::InvalidateToken() {
This seems not used.

https://codereview.chromium.org/23874015/diff/1/chrome/browser/ui/app_list/se...
chrome/browser/ui/app_list/search/people/people_provider.cc:145: "?" +
access_token_pair + "&" + query_pair);
Use net::AppendQueryParameter to construct URL properly.

https://codereview.chromium.org/23874015/diff/1/chrome/browser/ui/app_list/se...
chrome/browser/ui/app_list/search/people/people_provider.cc:174: 
ClearResults(); to remove existing results from previous search before adding
new ones.

https://codereview.chromium.org/23874015/diff/1/chrome/browser/ui/app_list/se...
File chrome/browser/ui/app_list/search/people/people_provider.h (right):

https://codereview.chromium.org/23874015/diff/1/chrome/browser/ui/app_list/se...
chrome/browser/ui/app_list/search/people/people_provider.h:34: public
OAuth2TokenService::Consumer {
nit: alignment

https://codereview.chromium.org/23874015/diff/1/chrome/browser/ui/app_list/se...
chrome/browser/ui/app_list/search/people/people_provider.h:43: //
OAuth2TokenService::Consumer implementation.
nit: Use overrides: since it is used above.

https://codereview.chromium.org/23874015/diff/1/chrome/browser/ui/app_list/se...
chrome/browser/ui/app_list/search/people/people_provider.h:85: std::string
people_search_url_;
Store url in GURL.

https://codereview.chromium.org/23874015/diff/1/chrome/browser/ui/app_list/se...
File chrome/browser/ui/app_list/search/people/people_result.cc (right):

https://codereview.chromium.org/23874015/diff/1/chrome/browser/ui/app_list/se...
chrome/browser/ui/app_list/search/people/people_result.cc:17: 
nit: nuke one empty line

https://codereview.chromium.org/23874015/diff/1/chrome/browser/ui/app_list/se...
chrome/browser/ui/app_list/search/people/people_result.cc:20: const int
kIconSize = 32;
We probably should move this to a common place to share with WebstoreResult.

https://codereview.chromium.org/23874015/diff/1/chrome/browser/ui/app_list/se...
chrome/browser/ui/app_list/search/people/people_result.cc:39:
set_relevance(interaction_rank_);
Is |interaction_rank_| in range [0,1]? Relevance needs to be within that range
to make the Mixer happy.

https://codereview.chromium.org/23874015/diff/1/chrome/browser/ui/app_list/se...
File chrome/browser/ui/app_list/search/people/people_result.h (right):

https://codereview.chromium.org/23874015/diff/1/chrome/browser/ui/app_list/se...
chrome/browser/ui/app_list/search/people/people_result.h:15: class
AppListControllerDelegate;
Is this used?

https://codereview.chromium.org/23874015/diff/1/chrome/browser/ui/app_list/se...
chrome/browser/ui/app_list/search/people/people_result.h:19: class
InstallTracker;
Is this used?

https://codereview.chromium.org/23874015/diff/1/chrome/common/chrome_switches.cc
File chrome/common/chrome_switches.cc (right):

https://codereview.chromium.org/23874015/diff/1/chrome/common/chrome_switches...
chrome/common/chrome_switches.cc:659: const char kEnablePeopleSearch[]      =
"enable-people-search";
nit: align '=' with other lines.

[rkc, 2013-09-04 23:48:01]

https://codereview.chromium.org/23874015/diff/1/chrome/browser/about_flags.cc
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/23874015/diff/1/chrome/browser/about_flags.cc...
chrome/browser/about_flags.cc:1219: kOsDesktop,
On 2013/09/04 23:15:41, xiyuan wrote:
> Note that app list does not exist for linux build.

Done.

https://codereview.chromium.org/23874015/diff/1/chrome/browser/ui/app_list/se...
File chrome/browser/ui/app_list/search/people/people_provider.cc (right):

https://codereview.chromium.org/23874015/diff/1/chrome/browser/ui/app_list/se...
chrome/browser/ui/app_list/search/people/people_provider.cc:44: std:: string
GetFirstValue(const ListValue& list, const char key[]) {
On 2013/09/04 23:15:41, xiyuan wrote:
> remove space after "::"

Done.

https://codereview.chromium.org/23874015/diff/1/chrome/browser/ui/app_list/se...
chrome/browser/ui/app_list/search/people/people_provider.cc:47: std::string();
On 2013/09/04 23:15:41, xiyuan wrote:
> return std::string();

Whoops, thanks for catching that!
Done.

https://codereview.chromium.org/23874015/diff/1/chrome/browser/ui/app_list/se...
chrome/browser/ui/app_list/search/people/people_provider.cc:51: return
std::string();;
On 2013/09/04 23:15:41, xiyuan wrote:
> nit: nuke one ';'

Done.

https://codereview.chromium.org/23874015/diff/1/chrome/browser/ui/app_list/se...
chrome/browser/ui/app_list/search/people/people_provider.cc:89:
StartThrottledQuery(base::Bind(&PeopleProvider::RequestAccessToken,
On 2013/09/04 23:15:41, xiyuan wrote:
> Is it possible to reuse the previously fetched |access_token_|?

Possibly, but it seemed cleaner to re-request the access_token each time since
a.) the token is cached anyway, so a re-request should be really quick, and b.)
if for any reason the tokens need to change with the backend, we don't have to
worry if our saved token is still valid or not.

https://codereview.chromium.org/23874015/diff/1/chrome/browser/ui/app_list/se...
chrome/browser/ui/app_list/search/people/people_provider.cc:131: void
PeopleProvider::InvalidateToken() {
On 2013/09/04 23:15:41, xiyuan wrote:
> This seems not used.

Removed. Added a TODO to JSONRepsonseFetcher to allow us to handle http
failures. Once that change is in, I can put this back in and call it on a 401
failure.

Done.

https://codereview.chromium.org/23874015/diff/1/chrome/browser/ui/app_list/se...
chrome/browser/ui/app_list/search/people/people_provider.cc:145: "?" +
access_token_pair + "&" + query_pair);
On 2013/09/04 23:15:41, xiyuan wrote:
> Use net::AppendQueryParameter to construct URL properly.

Done.

https://codereview.chromium.org/23874015/diff/1/chrome/browser/ui/app_list/se...
chrome/browser/ui/app_list/search/people/people_provider.cc:174: 
On 2013/09/04 23:15:41, xiyuan wrote:
> ClearResults(); to remove existing results from previous search before adding
> new ones.

Done.

https://codereview.chromium.org/23874015/diff/1/chrome/browser/ui/app_list/se...
File chrome/browser/ui/app_list/search/people/people_provider.h (right):

https://codereview.chromium.org/23874015/diff/1/chrome/browser/ui/app_list/se...
chrome/browser/ui/app_list/search/people/people_provider.h:34: public
OAuth2TokenService::Consumer {
On 2013/09/04 23:15:41, xiyuan wrote:
> nit: alignment

Done.

https://codereview.chromium.org/23874015/diff/1/chrome/browser/ui/app_list/se...
chrome/browser/ui/app_list/search/people/people_provider.h:43: //
OAuth2TokenService::Consumer implementation.
On 2013/09/04 23:15:41, xiyuan wrote:
> nit: Use overrides: since it is used above. 

Done.

https://codereview.chromium.org/23874015/diff/1/chrome/browser/ui/app_list/se...
chrome/browser/ui/app_list/search/people/people_provider.h:85: std::string
people_search_url_;
On 2013/09/04 23:15:41, xiyuan wrote:
> Store url in GURL.

Done.

https://codereview.chromium.org/23874015/diff/1/chrome/browser/ui/app_list/se...
File chrome/browser/ui/app_list/search/people/people_result.cc (right):

https://codereview.chromium.org/23874015/diff/1/chrome/browser/ui/app_list/se...
chrome/browser/ui/app_list/search/people/people_result.cc:17: 
On 2013/09/04 23:15:41, xiyuan wrote:
> nit: nuke one empty line

Done.

https://codereview.chromium.org/23874015/diff/1/chrome/browser/ui/app_list/se...
chrome/browser/ui/app_list/search/people/people_result.cc:20: const int
kIconSize = 32;
On 2013/09/04 23:15:41, xiyuan wrote:
> We probably should move this to a common place to share with WebstoreResult.

As of now there didn't seem like a good place to put this, since we don't have a
common file for the result classes. I'll get this in the next set of
refactoring; we still need to move stuff from tests also into common.

https://codereview.chromium.org/23874015/diff/1/chrome/browser/ui/app_list/se...
chrome/browser/ui/app_list/search/people/people_result.cc:39:
set_relevance(interaction_rank_);
On 2013/09/04 23:15:41, xiyuan wrote:
> Is |interaction_rank_| in range [0,1]? Relevance needs to be within that range
> to make the Mixer happy.

Yes, interaction rank is in the range of 0-1.

https://codereview.chromium.org/23874015/diff/1/chrome/browser/ui/app_list/se...
File chrome/browser/ui/app_list/search/people/people_result.h (right):

https://codereview.chromium.org/23874015/diff/1/chrome/browser/ui/app_list/se...
chrome/browser/ui/app_list/search/people/people_result.h:15: class
AppListControllerDelegate;
On 2013/09/04 23:15:41, xiyuan wrote:
> Is this used?

Nope, copy/paste leftovers.
Done.

https://codereview.chromium.org/23874015/diff/1/chrome/browser/ui/app_list/se...
chrome/browser/ui/app_list/search/people/people_result.h:19: class
InstallTracker;
On 2013/09/04 23:15:41, xiyuan wrote:
> Is this used?

Done.

https://codereview.chromium.org/23874015/diff/1/chrome/common/chrome_switches.cc
File chrome/common/chrome_switches.cc (right):

https://codereview.chromium.org/23874015/diff/1/chrome/common/chrome_switches...
chrome/common/chrome_switches.cc:659: const char kEnablePeopleSearch[]      =
"enable-people-search";
On 2013/09/04 23:15:41, xiyuan wrote:
> nit: align '=' with other lines.

Done.

[tfarina, 2013-09-04 23:49:52]

https://codereview.chromium.org/23874015/diff/1/chrome/browser/ui/app_list/se...
File chrome/browser/ui/app_list/search/common/json_response_fetcher.h (right):

https://codereview.chromium.org/23874015/diff/1/chrome/browser/ui/app_list/se...
chrome/browser/ui/app_list/search/common/json_response_fetcher.h:32: class
JSONResponseFetcher : public net::URLFetcherDelegate {
this probably can be used by WalletClient
(components/autofill/content/browser/wallet/wallet_client.h)

[rkc, 2013-09-04 23:57:30]

https://codereview.chromium.org/23874015/diff/1/chrome/browser/ui/app_list/se...
File chrome/browser/ui/app_list/search/common/json_response_fetcher.h (right):

https://codereview.chromium.org/23874015/diff/1/chrome/browser/ui/app_list/se...
chrome/browser/ui/app_list/search/common/json_response_fetcher.h:32: class
JSONResponseFetcher : public net::URLFetcherDelegate {
On 2013/09/04 23:49:52, tfarina wrote:
> this probably can be used by WalletClient
> (components/autofill/content/browser/wallet/wallet_client.h)

Wallet client's processing in OnURLFetchComplete is considerably more complex, I
am not sure if it's a great idea to even try to get it to fit into this class,
let alone way out of scope of this CL.

[tfarina, 2013-09-04 23:59:04]

https://codereview.chromium.org/23874015/diff/1/chrome/browser/ui/app_list/se...
File chrome/browser/ui/app_list/search/common/json_response_fetcher.h (right):

https://codereview.chromium.org/23874015/diff/1/chrome/browser/ui/app_list/se...
chrome/browser/ui/app_list/search/common/json_response_fetcher.h:32: class
JSONResponseFetcher : public net::URLFetcherDelegate {
On 2013/09/04 23:57:30, Rahul Chaturvedi wrote:
> On 2013/09/04 23:49:52, tfarina wrote:
> > this probably can be used by WalletClient
> > (components/autofill/content/browser/wallet/wallet_client.h)
> 
> Wallet client's processing in OnURLFetchComplete is considerably more complex,
I
> am not sure if it's a great idea to even try to get it to fit into this class,
> let alone way out of scope of this CL.
sure, I was not suggesting you to try to make any change related to wallet in
this CL, since it would be pretty out of scope. ;)

[xiyuan, 2013-09-05 02:23:56]

LGTM

https://codereview.chromium.org/23874015/diff/13001/chrome/browser/ui/app_lis...
File chrome/browser/ui/app_list/search/people/people_provider.h (right):

https://codereview.chromium.org/23874015/diff/13001/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list/search/people/people_provider.h:87: bool
running_as_test_;
nit: Maybe call this |skip_request_token_for_test_|?

[rkc, 2013-09-05 21:00:58]

Adding Zel for OWNER's review of google_apis/gaia/

https://codereview.chromium.org/23874015/diff/13001/chrome/browser/ui/app_lis...
File chrome/browser/ui/app_list/search/people/people_provider.h (right):

https://codereview.chromium.org/23874015/diff/13001/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list/search/people/people_provider.h:87: bool
running_as_test_;
On 2013/09/05 02:23:56, xiyuan wrote:
> nit: Maybe call this |skip_request_token_for_test_|?

Done.

[zel, 2013-09-05 21:27:50]

https://codereview.chromium.org/23874015/diff/38001/chrome/browser/ui/app_lis...
File chrome/browser/ui/app_list/search/people/people_provider.cc (right):

https://codereview.chromium.org/23874015/diff/38001/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list/search/people/people_provider.cc:67:
oauth2_scope_.insert(GaiaConstants::kPeopleSearchOAuth2Scope);
no reason to have this as GaiaConstants, since it really has nothing to do with
GAIA - define that constant here

https://codereview.chromium.org/23874015/diff/38001/google_apis/gaia/gaia_con...
File google_apis/gaia/gaia_constants.cc (right):

https://codereview.chromium.org/23874015/diff/38001/google_apis/gaia/gaia_con...
google_apis/gaia/gaia_constants.cc:42: // OAuth2 scope for access to the Google+
People Search API.
I wouldn't keep this here at all. Move the scope const somewhere where it's
used.

[rkc, 2013-09-05 21:57:25]

https://codereview.chromium.org/23874015/diff/38001/chrome/browser/ui/app_lis...
File chrome/browser/ui/app_list/search/people/people_provider.cc (right):

https://codereview.chromium.org/23874015/diff/38001/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list/search/people/people_provider.cc:67:
oauth2_scope_.insert(GaiaConstants::kPeopleSearchOAuth2Scope);
On 2013/09/05 21:27:50, zel wrote:
> no reason to have this as GaiaConstants, since it really has nothing to do
with
> GAIA - define that constant here

Done.

https://codereview.chromium.org/23874015/diff/38001/google_apis/gaia/gaia_con...
File google_apis/gaia/gaia_constants.cc (right):

https://codereview.chromium.org/23874015/diff/38001/google_apis/gaia/gaia_con...
google_apis/gaia/gaia_constants.cc:42: // OAuth2 scope for access to the Google+
People Search API.
On 2013/09/05 21:27:50, zel wrote:
> I wouldn't keep this here at all. Move the scope const somewhere where it's
> used.

Done.

[commit-bot: I haz the power, 2013-09-05 21:58:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rkc@chromium.org/23874015/43001

[zel, 2013-09-05 21:59:22]

https://codereview.chromium.org/23874015/diff/43001/chrome/common/chrome_swit...
File chrome/common/chrome_switches.cc (right):

https://codereview.chromium.org/23874015/diff/43001/chrome/common/chrome_swit...
chrome/common/chrome_switches.cc:663: const char kEnablePeopleSearch[]          
 = "enable-people-search";
this should probably go in chromeos/chromeos_switches.h/cc

[rkc, 2013-09-05 22:00:43]

https://codereview.chromium.org/23874015/diff/43001/chrome/common/chrome_swit...
File chrome/common/chrome_switches.cc (right):

https://codereview.chromium.org/23874015/diff/43001/chrome/common/chrome_swit...
chrome/common/chrome_switches.cc:663: const char kEnablePeopleSearch[]          
 = "enable-people-search";
On 2013/09/05 21:59:22, zel wrote:
> this should probably go in chromeos/chromeos_switches.h/cc

The feature works everywhere the apps_list does, so that's ChromeOS, Mac and
Windows, which is why I put this flag here.

[commit-bot: I haz the power, 2013-09-06 08:20:10]

Change committed as 221642