Move KeywordProvider to components/omnibox

Move KeywordProvider to components/omnibox

BUG=371538
TBR=pkasting@chromium.org for include fix in chrome/browser/ui/omnibox/omnibox_edit_model.cc

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=291360

[Mark P, 2014-08-18 22:01:18]

Personally, I think this only makes sense if you think you can move all the
providers to the component.  Having them split, some in the component and some
not, would be confusing.  Do you think all providers can be extracted from
chrome/... code and moved this point?  If not, them I'm not sure this is worth
starting now.

--mark

[hashimoto, 2014-08-19 00:24:52]

On 2014/08/18 22:01:18, Mark P wrote:
> Personally, I think this only makes sense if you think you can move all the
> providers to the component.  Having them split, some in the component and some
> not, would be confusing.  Do you think all providers can be extracted from
> chrome/... code and moved this point?  If not, them I'm not sure this is worth
> starting now.
> 
> --mark

In long term, all providers will eventually be moved to components/ for iOS.
For short term, I want to just move providers needed by athena (SearchProvider
and history related providers).
I want to move KeywordProvider as well because SearchProvider depends on
KeywordProvider and KeywordProvider has no chrome/ dependencies.

[Mark P, 2014-08-19 19:27:22]

On Mon, Aug 18, 2014 at 5:24 PM, <hashimoto@chromium.org> wrote:

> On 2014/08/18 22:01:18, Mark P wrote:
>
>> Personally, I think this only makes sense if you think you can move all
>> the
>> providers to the component.  Having them split, some in the component and
>> some
>> not, would be confusing.  Do you think all providers can be extracted from
>> chrome/... code and moved this point?  If not, them I'm not sure this is
>> worth
>> starting now.
>>
>
>  --mark
>>
>
> In long term, all providers will eventually be moved to components/ for
> iOS.
> For short term, I want to just move providers needed by athena
> (SearchProvider
> and history related providers).
>

Is this currently possible?  The history providers use a ton of
chrome/history/... stuff.

Can we hold off on moving this until we're confident that we can move what
you believe is necessary?  I worry we'll move some things then you'll find
out we can't move the rest that you need, and we'll be stuck in an awkward
middle ground.

--mark


> I want to move KeywordProvider as well because SearchProvider depends on
> KeywordProvider and KeywordProvider has no chrome/ dependencies.
>
> https://codereview.chromium.org/469623004/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[hashimoto, 2014-08-20 01:42:17]

On 2014/08/19 19:27:22, Mark P wrote:
> On Mon, Aug 18, 2014 at 5:24 PM, <mailto:hashimoto@chromium.org> wrote:
> 
> > On 2014/08/18 22:01:18, Mark P wrote:
> >
> >> Personally, I think this only makes sense if you think you can move all
> >> the
> >> providers to the component.  Having them split, some in the component and
> >> some
> >> not, would be confusing.  Do you think all providers can be extracted from
> >> chrome/... code and moved this point?  If not, them I'm not sure this is
> >> worth
> >> starting now.
> >>
> >
> >  --mark
> >>
> >
> > In long term, all providers will eventually be moved to components/ for
> > iOS.
> > For short term, I want to just move providers needed by athena
> > (SearchProvider
> > and history related providers).
> >
> 
> Is this currently possible?  The history providers use a ton of
> chrome/history/... stuff.
> 
> Can we hold off on moving this until we're confident that we can move what
> you believe is necessary?  I worry we'll move some things then you'll find
> out we can't move the rest that you need, and we'll be stuck in an awkward
> middle ground.
Classes in chrome/browser/history are being moved to components/history
(crbug.com/370850) and
Athena needs to use SearchProvider ASAP.

Why do you think it's awkward to have some providers in components/omnibox?
AutocompleteProviders used only by Chrome living in chrome/browser/autocomplete
while ones used by multiple embedders living in components/omnibox should make
sense.
> 
> --mark
> 
> 
> > I want to move KeywordProvider as well because SearchProvider depends on
> > KeywordProvider and KeywordProvider has no chrome/ dependencies.
> >
> > https://codereview.chromium.org/469623004/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[Mark P, 2014-08-20 17:51:01]

On 2014/08/20 01:42:17, hashimoto wrote:
> On 2014/08/19 19:27:22, Mark P wrote:
> > On Mon, Aug 18, 2014 at 5:24 PM, <mailto:hashimoto@chromium.org> wrote:
> > 
> > > On 2014/08/18 22:01:18, Mark P wrote:
> > >
> > >> Personally, I think this only makes sense if you think you can move all
> > >> the
> > >> providers to the component.  Having them split, some in the component and
> > >> some
> > >> not, would be confusing.  Do you think all providers can be extracted
from
> > >> chrome/... code and moved this point?  If not, them I'm not sure this is
> > >> worth
> > >> starting now.
> > >>
> > >
> > >  --mark
> > >>
> > >
> > > In long term, all providers will eventually be moved to components/ for
> > > iOS.
> > > For short term, I want to just move providers needed by athena
> > > (SearchProvider
> > > and history related providers).
> > >
> > 
> > Is this currently possible?  The history providers use a ton of
> > chrome/history/... stuff.
> > 
> > Can we hold off on moving this until we're confident that we can move what
> > you believe is necessary?  I worry we'll move some things then you'll find
> > out we can't move the rest that you need, and we'll be stuck in an awkward
> > middle ground.
> Classes in chrome/browser/history are being moved to components/history
> (crbug.com/370850) and
> Athena needs to use SearchProvider ASAP.
> 
> Why do you think it's awkward to have some providers in components/omnibox?

To some extent, the scoring of each provider is related to the others.  For
instance,
SearchProvider doesn't score anything above 1399 because it knows that 1400+
is "reserved" for high-quality URL prefix matches, which should beat everything
else.
Having providers live in separate directories implies that these relationships
don't
exist or at least are not important.  This would be false and misleading.  It'll
signify
that developers can change something in one place without worrying much about
its effects elsewhere, because that code elsewhere seems distant (not in the
same
top-level directory structure).

Given how much you're pushing, I'll go along.  I'm just worried that we'll
exacerbate
the trouble we had with code that was half-moved from calling itself
autocomplete
to calling itself omnibox to now being half-split over chrome/ and components/.
If this componentiziation does not work out, promise me you'll put things back
in
a sane state, okay?  :-)

--mark


> AutocompleteProviders used only by Chrome living in
chrome/browser/autocomplete
> while ones used by multiple embedders living in components/omnibox should make
> sense.
> > 
> > --mark
> > 
> > 
> > > I want to move KeywordProvider as well because SearchProvider depends on
> > > KeywordProvider and KeywordProvider has no chrome/ dependencies.
> > >
> > > https://codereview.chromium.org/469623004/
> > >
> > 
> > To unsubscribe from this group and stop receiving emails from it, send an
> email
> > to mailto:chromium-reviews+unsubscribe@chromium.org.

[Mark P, 2014-08-20 17:54:11]

BTW, why do you need keyword provider?  It only suggests keywords (getting
people to enter custom search mode) and provides extension suggestions.  I
didn't think your needs required either of those.

--mark

[Mark P, 2014-08-20 17:59:17]

https://codereview.chromium.org/469623004/diff/1/chrome/browser/autocomplete/...
File chrome/browser/autocomplete/keyword_provider_unittest.cc (right):

https://codereview.chromium.org/469623004/diff/1/chrome/browser/autocomplete/...
chrome/browser/autocomplete/keyword_provider_unittest.cc:1: // Copyright (c)
2012 The Chromium Authors. All rights reserved.
You're leaving the unit tests behind in chrome/ ?

[hashimoto, 2014-08-21 09:12:13]

On 2014/08/20 17:51:01, Mark P wrote:
> On 2014/08/20 01:42:17, hashimoto wrote:
> > On 2014/08/19 19:27:22, Mark P wrote:
> > > On Mon, Aug 18, 2014 at 5:24 PM, <mailto:hashimoto@chromium.org> wrote:
> > > 
> > > > On 2014/08/18 22:01:18, Mark P wrote:
> > > >
> > > >> Personally, I think this only makes sense if you think you can move all
> > > >> the
> > > >> providers to the component.  Having them split, some in the component
and
> > > >> some
> > > >> not, would be confusing.  Do you think all providers can be extracted
> from
> > > >> chrome/... code and moved this point?  If not, them I'm not sure this
is
> > > >> worth
> > > >> starting now.
> > > >>
> > > >
> > > >  --mark
> > > >>
> > > >
> > > > In long term, all providers will eventually be moved to components/ for
> > > > iOS.
> > > > For short term, I want to just move providers needed by athena
> > > > (SearchProvider
> > > > and history related providers).
> > > >
> > > 
> > > Is this currently possible?  The history providers use a ton of
> > > chrome/history/... stuff.
> > > 
> > > Can we hold off on moving this until we're confident that we can move what
> > > you believe is necessary?  I worry we'll move some things then you'll find
> > > out we can't move the rest that you need, and we'll be stuck in an awkward
> > > middle ground.
> > Classes in chrome/browser/history are being moved to components/history
> > (crbug.com/370850) and
> > Athena needs to use SearchProvider ASAP.
> > 
> > Why do you think it's awkward to have some providers in components/omnibox?
> 
> To some extent, the scoring of each provider is related to the others.  For
> instance,
> SearchProvider doesn't score anything above 1399 because it knows that 1400+
> is "reserved" for high-quality URL prefix matches, which should beat
everything
> else.
> Having providers live in separate directories implies that these relationships
> don't
> exist or at least are not important.  This would be false and misleading. 
It'll
> signify
> that developers can change something in one place without worrying much about
> its effects elsewhere, because that code elsewhere seems distant (not in the
> same
> top-level directory structure).
Even if they are put in the same directory, I think careless developers may try
to break this system.
The only thing which can prevent it from happening is reviews by the owners of
these directories (pkasting and you).
(The table in autocomplete_provider.h which describes the scoring system might
be helpful too.)
> 
> Given how much you're pushing, I'll go along.  I'm just worried that we'll
> exacerbate
> the trouble we had with code that was half-moved from calling itself
> autocomplete
> to calling itself omnibox to now being half-split over chrome/ and
components/.
> If this componentiziation does not work out, promise me you'll put things back
> in
> a sane state, okay?  :-)
I'm afraid things cannot be put back later because once SearchProvider gets
moved to components,
it'll be used to suggest queries by athena::UrlSearchProvider who cannot depend
on code under chrome/.
> 
> --mark
> 
> 
> > AutocompleteProviders used only by Chrome living in
> chrome/browser/autocomplete
> > while ones used by multiple embedders living in components/omnibox should
make
> > sense.
> > > 
> > > --mark
> > > 
> > > 
> > > > I want to move KeywordProvider as well because SearchProvider depends on
> > > > KeywordProvider and KeywordProvider has no chrome/ dependencies.
> > > >
> > > > https://codereview.chromium.org/469623004/
> > > >
> > > 
> > > To unsubscribe from this group and stop receiving emails from it, send an
> > email
> > > to mailto:chromium-reviews+unsubscribe@chromium.org.

[hashimoto, 2014-08-21 09:12:49]

On 2014/08/20 17:54:11, Mark P wrote:
> BTW, why do you need keyword provider?  It only suggests keywords (getting
> people to enter custom search mode) and provides extension suggestions.  I
> didn't think your needs required either of those.
> 
> --mark

KeywordProvider is needed because it's used by SearchProvider.

[hashimoto, 2014-08-21 09:24:43]

https://codereview.chromium.org/469623004/diff/1/chrome/browser/autocomplete/...
File chrome/browser/autocomplete/keyword_provider_unittest.cc (right):

https://codereview.chromium.org/469623004/diff/1/chrome/browser/autocomplete/...
chrome/browser/autocomplete/keyword_provider_unittest.cc:1: // Copyright (c)
2012 The Chromium Authors. All rights reserved.
On 2014/08/20 17:59:17, Mark P wrote:
> You're leaving the unit tests behind in chrome/ ?

Done.

[Mark P, 2014-08-21 20:26:10]

lgtm

[hashimoto, 2014-08-22 03:18:07]

The CQ bit was checked by hashimoto@chromium.org

[commit-bot: I haz the power, 2014-08-22 03:19:23]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/hashimoto@chromium.org/469623004/40001

[commit-bot: I haz the power, 2014-08-22 04:14:19]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[commit-bot: I haz the power, 2014-08-22 04:18:16]

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

[commit-bot: I haz the power, 2014-08-22 04:18:17]

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

[hashimoto, 2014-08-22 05:08:34]

TBRing pkasting@chromium.org for include fix in
chrome/browser/ui/omnibox/omnibox_edit_model.cc

[hashimoto, 2014-08-22 05:08:41]

The CQ bit was checked by hashimoto@chromium.org

[commit-bot: I haz the power, 2014-08-22 05:09:28]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/hashimoto@chromium.org/469623004/40001

[commit-bot: I haz the power, 2014-08-22 06:02:02]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)

[commit-bot: I haz the power, 2014-08-22 07:44:56]

Committed patchset #3 (40001) as 291360