Removing --disable-people-search flag

Removing "--disable-people-search" flag

It was used to disable people search, but it is obsolete.

BUG=

Committed: https://crrev.com/964242fb1e38cb05a6818bea12e34bf3e719e1e8
Cr-Commit-Position: refs/heads/master@{#311100}

[anujsharma, 2015-01-12 02:39:11]

anujk.sharma@samsung.com changed reviewers:
+ thestig@chromium.org

[anujsharma, 2015-01-12 02:39:12]

PTAL

[Lei Zhang, 2015-01-12 08:43:38]

I don't see how this command line switch is obsolete.

[anujsharma, 2015-01-12 08:47:36]

On 2015/01/12 08:43:38, Lei Zhang wrote:
> I don't see how this command line switch is obsolete.

@Lei,
There is a speadsheet shared by Mr Peter in Meta bug 343941,
https://docs.google.com/spreadsheet/ccc?key=0ArH-w4dboLrgdGRqNU5FUm1Za2VieTFH...

As per this spreadsheet, this flag was used to disable people search, but it is
obsolete now.

[Lei Zhang, 2015-01-12 08:50:49]

On 2015/01/12 08:47:36, anujsharma wrote:
> On 2015/01/12 08:43:38, Lei Zhang wrote:
> > I don't see how this command line switch is obsolete.
> 
> @Lei,
> There is a speadsheet shared by Mr Peter in Meta bug 343941,
>
https://docs.google.com/spreadsheet/ccc?key=0ArH-w4dboLrgdGRqNU5FUm1Za2VieTFH...
> 
> As per this spreadsheet, this flag was used to disable people search, but it
is
> obsolete now.

You should add BUG=418072 to the CL description so there is context.

You should ask rkc@ to review this CL, since he is the owner of that switch
according to the spreadsheet.

[anujsharma, 2015-01-12 08:53:05]

On 2015/01/12 08:50:49, Lei Zhang wrote:
> On 2015/01/12 08:47:36, anujsharma wrote:
> > On 2015/01/12 08:43:38, Lei Zhang wrote:
> > > I don't see how this command line switch is obsolete.
> > 
> > @Lei,
> > There is a speadsheet shared by Mr Peter in Meta bug 343941,
> >
>
https://docs.google.com/spreadsheet/ccc?key=0ArH-w4dboLrgdGRqNU5FUm1Za2VieTFH...
> > 
> > As per this spreadsheet, this flag was used to disable people search, but it
> is
> > obsolete now.
> 
> You should add BUG=418072 to the CL description so there is context.
> 
> You should ask rkc@ to review this CL, since he is the owner of that switch
> according to the spreadsheet.

Ok Lei Zhang,
Thanks for your suggestion, actually somehow i am unable to see that bug.
I am adding rkc@ also for review.

Thanks a lot!!

[anujsharma, 2015-01-12 08:53:31]

anujk.sharma@samsung.com changed reviewers:
+ rkc@chromium.org

[anujsharma, 2015-01-12 08:54:08]

@Rahul 
Could you please take a look over this CL.

Thanks!!

[rkc, 2015-01-12 17:55:01]

lgtm

[anujsharma, 2015-01-12 18:20:17]

On 2015/01/12 17:55:01, Rahul Chaturvedi wrote:
> lgtm

Thanks rahul for lgtm.

[Lei Zhang, 2015-01-12 18:49:24]

The CQ bit was checked by thestig@chromium.org

[Lei Zhang, 2015-01-12 18:49:26]

lgtm

[commit-bot: I haz the power, 2015-01-12 18:49:57]

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

[commit-bot: I haz the power, 2015-01-12 20:24:32]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2015-01-12 20:26:15]

Patchset 1 (id:??) landed as
https://crrev.com/964242fb1e38cb05a6818bea12e34bf3e719e1e8
Cr-Commit-Position: refs/heads/master@{#311100}

[Matt Giuca, 2015-01-15 01:43:57]

This CL inadvertently disabled people search.

1. Please be more careful with these seemingly simple CLs (both authoring and
reviewing). They can be hard to catch (I only found this because I happened to
get a merge conflict).

2. Please add reviewers from the most specific OWNERS file. In this case,
thestig@ was added because he is a top-level owner in chrome/; this really
should have been reviewed by myself or another owner in
chrome/browser/ui/app_list.

I have filed a bug:
http://crbug.com/449005
and sent out a fix:
https://codereview.chromium.org/852053003/

https://codereview.chromium.org/790793010/diff/1/chrome/browser/ui/app_list/s...
File chrome/browser/ui/app_list/search/search_controller_factory.cc (left):

https://codereview.chromium.org/790793010/diff/1/chrome/browser/ui/app_list/s...
chrome/browser/ui/app_list/search/search_controller_factory.cc:60:
controller->AddProvider(Mixer::PEOPLE_GROUP,
You are removing a NOT of the switch, therefore, the body of this if statement
should not have been deleted. (This permanently disabled people search.)

[Lei Zhang, 2015-01-15 02:21:47]

On 2015/01/15 01:43:57, Matt Giuca wrote:
> 2. Please add reviewers from the most specific OWNERS file. In this case,
> thestig@ was added because he is a top-level owner in chrome/; this really
> should have been reviewed by myself or another owner in
> chrome/browser/ui/app_list.

Sigh, rkc@ was listed as the switch's owner on the "switches to remove"
spreadsheet. So I assumed he was working on this feature.

[Matt Giuca, 2015-01-15 02:55:04]

On 2015/01/15 02:21:47, Lei Zhang wrote:
> Sigh, rkc@ was listed as the switch's owner on the "switches to remove"
> spreadsheet. So I assumed he was working on this feature.

Yeah, he was the original author (but I seem to be maintaining the search
provider code now). I'm just commenting that in general it's best to send
reviews to the most specific owners because they are more likely to have state
on it (and, I would imagine, be less busy than top-level owners).

[Lei Zhang, 2015-01-15 02:56:48]

On 2015/01/15 02:55:04, Matt Giuca wrote:
> On 2015/01/15 02:21:47, Lei Zhang wrote:
> > Sigh, rkc@ was listed as the switch's owner on the "switches to remove"
> > spreadsheet. So I assumed he was working on this feature.
> 
> Yeah, he was the original author (but I seem to be maintaining the search
> provider code now). I'm just commenting that in general it's best to send
> reviews to the most specific owners because they are more likely to have state
> on it (and, I would imagine, be less busy than top-level owners).

Yes, that's what I tried to do. I just rubber stamped. I'll check more carefully
next time.

[anujsharma, 2015-01-15 02:59:08]

On 2015/01/15 01:43:57, Matt Giuca wrote:
> This CL inadvertently disabled people search.
> 
> 1. Please be more careful with these seemingly simple CLs (both authoring and
> reviewing). They can be hard to catch (I only found this because I happened to
> get a merge conflict).
> 
> 2. Please add reviewers from the most specific OWNERS file. In this case,
> thestig@ was added because he is a top-level owner in chrome/; this really
> should have been reviewed by myself or another owner in
> chrome/browser/ui/app_list.
> 
> I have filed a bug:
> http://crbug.com/449005
> and sent out a fix:
> https://codereview.chromium.org/852053003/
> 
>
https://codereview.chromium.org/790793010/diff/1/chrome/browser/ui/app_list/s...
> File chrome/browser/ui/app_list/search/search_controller_factory.cc (left):
> 
>
https://codereview.chromium.org/790793010/diff/1/chrome/browser/ui/app_list/s...
> chrome/browser/ui/app_list/search/search_controller_factory.cc:60:
> controller->AddProvider(Mixer::PEOPLE_GROUP,
> You are removing a NOT of the switch, therefore, the body of this if statement
> should not have been deleted. (This permanently disabled people search.)

Thanks Matt for bringing this to notice.
From next time i will be more careful in making these type of changes.
Sorry for the mess!!