Remove people search

Remove people search

BUG=601233
Committed: https://crrev.com/1333573b1f5535417caa9e4d41ba2872cf14e9cd
Cr-Commit-Position: refs/heads/master@{#391276}

[xiaoyinh(OOO Sep 11-29), 2016-04-29 22:33:02]

xiaoyinh@chromium.org changed reviewers:
+ achuith@chromium.org, glevin@chromium.org, jennyz@chromium.org,
rkc@chromium.org, stevenjb@chromium.org, xiyuan@chromium.org

[rkc, 2016-04-29 22:46:14]

I presume removing hangoutsPrivate will be in a followup CL?

https://codereview.chromium.org/1925733005/diff/20001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/search/people/people_provider.cc (left):

https://codereview.chromium.org/1925733005/diff/20001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/search/people/people_provider.cc:22: #include
"chrome/browser/ui/app_list/search/people/person.h"
Delete person.*?

https://codereview.chromium.org/1925733005/diff/20001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/search/people/people_result.cc (left):

https://codereview.chromium.org/1925733005/diff/20001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/search/people/people_result.cc:134:
*bundle.GetImageSkiaNamed(IDR_PEOPLE_SEARCH_ACTION_CHAT),
Delete resources and remove resource entries?

https://codereview.chromium.org/1925733005/diff/20001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/search/people/people_result.cc:172:
content::RecordAction(base::UserMetricsAction("PeopleSearch_OpenChat"));
Is there a procedure to delete UserMetricsActions like we have for UMA enums
(marking with DELETED_)?

[stevenjb, 2016-04-29 23:08:13]

owner lgtm with person.cc/h removed.

[jennyz, 2016-04-29 23:45:30]

On 2016/04/29 23:08:13, stevenjb wrote:
> owner lgtm with person.cc/h removed.
ditto stevenjb's comment.

[jennyz, 2016-04-29 23:45:34]

On 2016/04/29 23:08:13, stevenjb wrote:
> owner lgtm with person.cc/h removed.
ditto stevenjb's comment.

[achuithb, 2016-05-02 18:54:18]

Do you need my review? Seems like you have a lot of reviewers for this CL

[xiaoyinh(OOO Sep 11-29), 2016-05-02 19:15:48]

On 2016/05/02 18:54:18, achuithb wrote:
> Do you need my review? Seems like you have a lot of reviewers for this CL
Hi,

I must find your name in chrome/OWNERS file and also include some more specific
OWNERS under this directory.
Sorry about the confusion.

[xiaoyinh(OOO Sep 11-29), 2016-05-02 19:41:54]

xiaoyinh@chromium.org changed reviewers:
+ oshima@chromium.org

[xiaoyinh(OOO Sep 11-29), 2016-05-02 19:50:44]

On 2016/04/29 22:46:14, Rahul Chaturvedi wrote:
> I presume removing hangoutsPrivate will be in a followup CL?

Could you fill me in how this is impacted by people search feature? I could do
it in a separate CL.

> 
>
https://codereview.chromium.org/1925733005/diff/20001/chrome/browser/ui/app_l...
> File chrome/browser/ui/app_list/search/people/people_provider.cc (left):
> 
>
https://codereview.chromium.org/1925733005/diff/20001/chrome/browser/ui/app_l...
> chrome/browser/ui/app_list/search/people/people_provider.cc:22: #include
> "chrome/browser/ui/app_list/search/people/person.h"
> Delete person.*?
> 
Done.

>
https://codereview.chromium.org/1925733005/diff/20001/chrome/browser/ui/app_l...
> File chrome/browser/ui/app_list/search/people/people_result.cc (left):
> 
>
https://codereview.chromium.org/1925733005/diff/20001/chrome/browser/ui/app_l...
> chrome/browser/ui/app_list/search/people/people_result.cc:134:
> *bundle.GetImageSkiaNamed(IDR_PEOPLE_SEARCH_ACTION_CHAT),
> Delete resources and remove resource entries?
> 
Done.

>
https://codereview.chromium.org/1925733005/diff/20001/chrome/browser/ui/app_l...
> chrome/browser/ui/app_list/search/people/people_result.cc:172:
> content::RecordAction(base::UserMetricsAction("PeopleSearch_OpenChat"));
> Is there a procedure to delete UserMetricsActions like we have for UMA enums
> (marking with DELETED_)?

Could you provide some instructions? I could do it in a separate CL.

[stevenjb, 2016-05-02 20:35:55]

On 2016/05/02 19:50:44, xiaoyinh wrote:
> On 2016/04/29 22:46:14, Rahul Chaturvedi wrote:
> > I presume removing hangoutsPrivate will be in a followup CL?
> 
> Could you fill me in how this is impacted by people search feature? I could do
> it in a separate CL.
> 
> > 
> >
>
https://codereview.chromium.org/1925733005/diff/20001/chrome/browser/ui/app_l...
> > File chrome/browser/ui/app_list/search/people/people_provider.cc (left):
> > 
> >
>
https://codereview.chromium.org/1925733005/diff/20001/chrome/browser/ui/app_l...
> > chrome/browser/ui/app_list/search/people/people_provider.cc:22: #include
> > "chrome/browser/ui/app_list/search/people/person.h"
> > Delete person.*?
> > 
> Done.
> 
> >
>
https://codereview.chromium.org/1925733005/diff/20001/chrome/browser/ui/app_l...
> > File chrome/browser/ui/app_list/search/people/people_result.cc (left):
> > 
> >
>
https://codereview.chromium.org/1925733005/diff/20001/chrome/browser/ui/app_l...
> > chrome/browser/ui/app_list/search/people/people_result.cc:134:
> > *bundle.GetImageSkiaNamed(IDR_PEOPLE_SEARCH_ACTION_CHAT),
> > Delete resources and remove resource entries?
> > 
> Done.
> 
> >
>
https://codereview.chromium.org/1925733005/diff/20001/chrome/browser/ui/app_l...
> > chrome/browser/ui/app_list/search/people/people_result.cc:172:
> > content::RecordAction(base::UserMetricsAction("PeopleSearch_OpenChat"));
> > Is there a procedure to delete UserMetricsActions like we have for UMA enums
> > (marking with DELETED_)?
> 
> Could you provide some instructions? I could do it in a separate CL.

At the top of actions.xml:

"If a user action is not being used any more, put an <obsolete> tag under
the <action> tag with necessary explanation. Don't delete things in this file."

We should do that for PeopleSearch_OpenChat and PeopleSearch_SendEmail.

[stevenjb, 2016-05-02 20:37:49]

https://codereview.chromium.org/1925733005/diff/40001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/search/people/people_result.cc (left):

https://codereview.chromium.org/1925733005/diff/40001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/search/people/people_result.cc:88:
RecordHistogram(SEARCH_PEOPLE_SEARCH_RESULT);
This enum value can probably also be removed.

[stevenjb, 2016-05-02 20:39:59]

On 2016/05/02 19:50:44, xiaoyinh wrote:
> On 2016/04/29 22:46:14, Rahul Chaturvedi wrote:
> > I presume removing hangoutsPrivate will be in a followup CL?
> 
> Could you fill me in how this is impacted by people search feature? I could do
> it in a separate CL.
> 
> > 
> >
>
https://codereview.chromium.org/1925733005/diff/20001/chrome/browser/ui/app_l...
> > File chrome/browser/ui/app_list/search/people/people_provider.cc (left):
> > 
> >
>
https://codereview.chromium.org/1925733005/diff/20001/chrome/browser/ui/app_l...
> > chrome/browser/ui/app_list/search/people/people_provider.cc:22: #include
> > "chrome/browser/ui/app_list/search/people/person.h"
> > Delete person.*?
> > 
> Done.
> 
> >
>
https://codereview.chromium.org/1925733005/diff/20001/chrome/browser/ui/app_l...
> > File chrome/browser/ui/app_list/search/people/people_result.cc (left):
> > 
> >
>
https://codereview.chromium.org/1925733005/diff/20001/chrome/browser/ui/app_l...
> > chrome/browser/ui/app_list/search/people/people_result.cc:134:
> > *bundle.GetImageSkiaNamed(IDR_PEOPLE_SEARCH_ACTION_CHAT),
> > Delete resources and remove resource entries?
> > 
> Done.
> 
> >
>
https://codereview.chromium.org/1925733005/diff/20001/chrome/browser/ui/app_l...
> > chrome/browser/ui/app_list/search/people/people_result.cc:172:
> > content::RecordAction(base::UserMetricsAction("PeopleSearch_OpenChat"));
> > Is there a procedure to delete UserMetricsActions like we have for UMA enums
> > (marking with DELETED_)?
> 
> Could you provide some instructions? I could do it in a separate CL.

This is the only file that references hangouts_private.h so we should (in a
separate CL) eliminate hangouts_private.idl and any references to
hangoutsPrivate.

[rkc, 2016-05-02 20:53:54]

lgtm - please do a followup CL to remove hangoutsPrivate.

[xiaoyinh(OOO Sep 11-29), 2016-05-02 21:09:59]

On 2016/05/02 20:37:49, stevenjb wrote:
>
https://codereview.chromium.org/1925733005/diff/40001/chrome/browser/ui/app_l...
> File chrome/browser/ui/app_list/search/people/people_result.cc (left):
> 
>
https://codereview.chromium.org/1925733005/diff/40001/chrome/browser/ui/app_l...
> chrome/browser/ui/app_list/search/people/people_result.cc:88:
> RecordHistogram(SEARCH_PEOPLE_SEARCH_RESULT);
> This enum value can probably also be removed.

Based on the comment in search_util.h

// The type of the chrome search result. This is used for logging so do not
// change the order of this enum.

enum SearchResultType {
  // A result that forwards an omnibox search result.
  OMNIBOX_SEARCH_RESULT,
  // An app result.
  APP_SEARCH_RESULT,
  // A search result from the webstore.
  WEBSTORE_SEARCH_RESULT,
  // A result that opens a webstore search.
  SEARCH_WEBSTORE_SEARCH_RESULT,
  // A result that opens a people search.
   SEARCH_PEOPLE_SEARCH_RESULT,           
  // A result that opens a suggestion.
  SUGGESTIONS_SEARCH_RESULT,
  // A result that is provided by the custom launcher search provider.
  LAUNCHER_SEARCH_PROVIDER_RESULT,
  // Boundary is always last.
  SEARCH_RESULT_TYPE_BOUNDARY

};

If I delete SEARCH_PEOPLE_SEARCH_RESULT, will it cause any backwards
compatibility issues?

[stevenjb, 2016-05-02 21:14:38]

Yes, sorry, I should have been more clear:

s/SEARCH_PEOPLE_SEARCH_RESULT/SEARCH_PEOPLE_SEARCH_RESULT_DEPRECATED/



On Mon, May 2, 2016 at 3:09 PM, <xiaoyinh@chromium.org> wrote:

> On 2016/05/02 20:37:49, stevenjb wrote:
> >
>
>
https://codereview.chromium.org/1925733005/diff/40001/chrome/browser/ui/app_l...
> > File chrome/browser/ui/app_list/search/people/people_result.cc (left):
> >
> >
>
>
https://codereview.chromium.org/1925733005/diff/40001/chrome/browser/ui/app_l...
> > chrome/browser/ui/app_list/search/people/people_result.cc:88:
> > RecordHistogram(SEARCH_PEOPLE_SEARCH_RESULT);
> > This enum value can probably also be removed.
>
> Based on the comment in search_util.h
>
> // The type of the chrome search result. This is used for logging so do not
> // change the order of this enum.
>
> enum SearchResultType {
> // A result that forwards an omnibox search result.
> OMNIBOX_SEARCH_RESULT,
> // An app result.
> APP_SEARCH_RESULT,
> // A search result from the webstore.
> WEBSTORE_SEARCH_RESULT,
> // A result that opens a webstore search.
> SEARCH_WEBSTORE_SEARCH_RESULT,
> // A result that opens a people search.
> SEARCH_PEOPLE_SEARCH_RESULT,
> // A result that opens a suggestion.
> SUGGESTIONS_SEARCH_RESULT,
> // A result that is provided by the custom launcher search provider.
> LAUNCHER_SEARCH_PROVIDER_RESULT,
> // Boundary is always last.
> SEARCH_RESULT_TYPE_BOUNDARY
>
> };
>
> If I delete SEARCH_PEOPLE_SEARCH_RESULT, will it cause any backwards
> compatibility issues?
>
> https://codereview.chromium.org/1925733005/
>

-- 
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.

[xiyuan, 2016-05-02 21:26:01]

https://codereview.chromium.org/1925733005/diff/40001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/search/people/people_result.cc (left):

https://codereview.chromium.org/1925733005/diff/40001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/search/people/people_result.cc:172:
content::RecordAction(base::UserMetricsAction("PeopleSearch_OpenChat"));
And update this in tools/metrics/actions/actions.xml. Think marking them with
<obsolete> should be good enough.

https://code.google.com/p/chromium/codesearch#chromium/src/tools/metrics/acti...

[xiaoyinh(OOO Sep 11-29), 2016-05-02 21:52:38]

xiaoyinh@chromium.org changed reviewers:
+ isherman@chromium.org

[xiaoyinh(OOO Sep 11-29), 2016-05-02 21:52:39]

Latest Patch has incorporated all the above comments.

TODO: remove Hangouts Private in a separate CL.

[xiyuan, 2016-05-02 21:54:29]

lgtm

[stevenjb, 2016-05-02 21:55:22]

New patch lgtm, thanks.

[xiaoyinh(OOO Sep 11-29), 2016-05-02 22:48:44]

The CQ bit was checked by xiaoyinh@chromium.org

[xiaoyinh(OOO Sep 11-29), 2016-05-02 22:48:44]

The patchset sent to the CQ was uploaded after l-g-t-m from rkc@chromium.org
Link to the patchset: https://codereview.chromium.org/1925733005/#ps60001
(title: "Deprecate people search actions and enum")

[commit-bot: I haz the power, 2016-05-02 22:50:08]

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

[commit-bot: I haz the power, 2016-05-02 23:03:30]

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

[commit-bot: I haz the power, 2016-05-02 23:03:31]

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

[oshima, 2016-05-02 23:42:02]

c/a/theme lgtm

[Ilya Sherman, 2016-05-03 01:38:33]

actions.xml lgtm

[xiaoyinh(OOO Sep 11-29), 2016-05-03 16:59:45]

The CQ bit was checked by xiaoyinh@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-05-03 17:00:24]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1925733005/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1925733005/60001

[commit-bot: I haz the power, 2016-05-03 17:08:53]

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

[commit-bot: I haz the power, 2016-05-03 17:08:54]

Dry run: This issue passed the CQ dry run.

[xiaoyinh(OOO Sep 11-29), 2016-05-03 17:11:03]

The CQ bit was checked by xiaoyinh@chromium.org

[commit-bot: I haz the power, 2016-05-03 17:11:29]

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

[commit-bot: I haz the power, 2016-05-03 17:17:32]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-05-03 17:19:50]

Description was changed from

==========
Remove people search

BUG=601233
==========

to

==========
Remove people search

BUG=601233

Committed: https://crrev.com/1333573b1f5535417caa9e4d41ba2872cf14e9cd
Cr-Commit-Position: refs/heads/master@{#391276}
==========

[commit-bot: I haz the power, 2016-05-03 17:19:51]

Patchset 4 (id:??) landed as
https://crrev.com/1333573b1f5535417caa9e4d41ba2872cf14e9cd
Cr-Commit-Position: refs/heads/master@{#391276}