History provider: style cleanup

History provider: style cleanup

BUG=none
TEST=it compiles

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=56939

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=56981

[Ilya Sherman, 2010-08-17 05:46:07]

Mostly cleaning up dependencies a bit, the better to be independent.

[mrossetti, 2010-08-17 15:16:12]

I'm in the process of refactoring the autocomplete history provider so this
change is in conflict with my intent. We will shortly have two autocomplete
history providers: this one (trimmed down to only do the asynchronous provision)
and a new 'quick' provider (which can provide <20ms results to autocomplete). 
After this settles down, I'm considering a possible third history provider (I'm
calling 'shortcuts') which will vend up previous choices made during a search.

So, the bottom line is that while your intent is spot-on this cleanup will occur
as a new history provider base class is introduced.

On 2010/08/17 05:46:07, Ilya Sherman wrote:
> Mostly cleaning up dependencies a bit, the better to be independent.

[Peter Kasting, 2010-08-17 16:47:32]

LGTM.  Mike, is it really difficult to sync with this?  It seems like even if
you're refactoring the history provider, we'll want this change to
history_types.*, and the additions to the history provider will either go in
that file or in one of your refactored files, meaning the diffs would be
slightly smaller.  So it seems like this is still a good change?

http://codereview.chromium.org/3177020/diff/5001/6001
File chrome/browser/autocomplete/history_url_provider.cc (right):

http://codereview.chromium.org/3177020/diff/5001/6001#newcode47
chrome/browser/autocomplete/history_url_provider.cc:47: // maybe letting the
compiler optimize a bit more freely?
I don't understand what you're proposing here.  Eliminating the default
constructor and letting the compiler auto-generate it?  It won't, when we've
written a non-default constructor.

http://codereview.chromium.org/3177020/diff/5001/6003
File chrome/browser/history/history_types.cc (right):

http://codereview.chromium.org/3177020/diff/5001/6003#newcode1
chrome/browser/history/history_types.cc:1: // Copyright (c) 2006-2008 The
Chromium Authors. All rights reserved.
Nit: 2010

[rohitrao (ping after 24h), 2010-08-17 16:58:44]

We had moved a number of these functions out of HistoryURLProvider and into
history_types.h because they were going to be shared between the
HistoryURLProvider and HistoryQuickProvider classes, at least temporarily.  I
agree that they don't really belong in history_types.h (I think in the original
review I suggested that they eventually be moved into
autocomplete/something_util.h), but for now it is convenient to have them in one
easily-shareable location.

With that background, I think moving them back into HistoryURLProvider will not
work, because we'll be using them from a different class.  I wouldn't mind
moving them into a new file in autocomplete/, if we all think they fit better
there.

On 2010/08/17 16:47:32, Peter Kasting wrote:
> LGTM.  Mike, is it really difficult to sync with this?  It seems like even if
> you're refactoring the history provider, we'll want this change to
> history_types.*, and the additions to the history provider will either go in
> that file or in one of your refactored files, meaning the diffs would be
> slightly smaller.  So it seems like this is still a good change?

[mrossetti_google.com, 2010-08-17 17:04:36]

+1 on Rohit's comments.  That was exactly the plan.

On Tue, Aug 17, 2010 at 10:58 AM, <rohitrao@chromium.org> wrote:

> We had moved a number of these functions out of HistoryURLProvider and into
> history_types.h because they were going to be shared between the
> HistoryURLProvider and HistoryQuickProvider classes, at least temporarily.
>  I
> agree that they don't really belong in history_types.h (I think in the
> original
> review I suggested that they eventually be moved into
> autocomplete/something_util.h), but for now it is convenient to have them
> in one
> easily-shareable location.
>
> With that background, I think moving them back into HistoryURLProvider will
> not
> work, because we'll be using them from a different class.  I wouldn't mind
> moving them into a new file in autocomplete/, if we all think they fit
> better
> there.
>
>
> On 2010/08/17 16:47:32, Peter Kasting wrote:
>
>> LGTM.  Mike, is it really difficult to sync with this?  It seems like even
>> if
>> you're refactoring the history provider, we'll want this change to
>> history_types.*, and the additions to the history provider will either go
>> in
>> that file or in one of your refactored files, meaning the diffs would be
>> slightly smaller.  So it seems like this is still a good change?
>>
>
>
> http://codereview.chromium.org/3177020/show
>

[Peter Kasting, 2010-08-17 17:07:55]

On 2010/08/17 16:58:44, rohitrao wrote:
> We had moved a number of these functions out of HistoryURLProvider and into
> history_types.h because they were going to be shared between the
> HistoryURLProvider and HistoryQuickProvider classes, at least temporarily.

:( They should have been moved at the point when multiple classes needed them,
and not before, because without being able to read your new classes, I can't
speculate as to the right design.  (Heck, I don't even know that splitting into
multiple history providers is going to make sense without reading your change.) 
I still support checking this patch in, especially since you're right that they
should probably never go in history_types.h.

[mrossetti_google.com, 2010-08-17 17:16:32]

Then I suggest that Ilya, armed with this new knowledge (the intent to
introduce a new provider), create a new file in autocomplete/ in which these
common pieces can live as part of this CL.

On Tue, Aug 17, 2010 at 11:07 AM, <pkasting@chromium.org> wrote:

> On 2010/08/17 16:58:44, rohitrao wrote:
>
>> We had moved a number of these functions out of HistoryURLProvider and
>> into
>> history_types.h because they were going to be shared between the
>> HistoryURLProvider and HistoryQuickProvider classes, at least temporarily.
>>
>
> :( They should have been moved at the point when multiple classes needed
> them,
> and not before, because without being able to read your new classes, I
> can't
> speculate as to the right design.  (Heck, I don't even know that splitting
> into
> multiple history providers is going to make sense without reading your
> change.)
> I still support checking this patch in, especially since you're right that
> they
> should probably never go in history_types.h.
>
>
> http://codereview.chromium.org/3177020/show
>

[Ilya Sherman, 2010-08-17 22:14:57]

http://codereview.chromium.org/3177020/diff/5001/6001
File chrome/browser/autocomplete/history_url_provider.cc (right):

http://codereview.chromium.org/3177020/diff/5001/6001#newcode47
chrome/browser/autocomplete/history_url_provider.cc:47: // maybe letting the
compiler optimize a bit more freely?
On 2010/08/17 16:47:32, Peter Kasting wrote:
> I don't understand what you're proposing here.  Eliminating the default
> constructor and letting the compiler auto-generate it?  It won't, when we've
> written a non-default constructor.

I just meant we could define the constructor as

  HistoryMatch() {}

without providing any default values for the members.

[Peter Kasting, 2010-08-17 22:17:57]

http://codereview.chromium.org/3177020/diff/5001/6001
File chrome/browser/autocomplete/history_url_provider.cc (right):

http://codereview.chromium.org/3177020/diff/5001/6001#newcode47
chrome/browser/autocomplete/history_url_provider.cc:47: // maybe letting the
compiler optimize a bit more freely?
On 2010/08/17 22:14:57, Ilya Sherman wrote:
> On 2010/08/17 16:47:32, Peter Kasting wrote:
> > I don't understand what you're proposing here.  Eliminating the default
> > constructor and letting the compiler auto-generate it?  It won't, when we've
> > written a non-default constructor.
> 
> I just meant we could define the constructor as
> 
>   HistoryMatch() {}
> 
> without providing any default values for the members.

Hmm... I don't know whether my comment suggests that we'll always overwrite
instances created by this constructor.  If not, and it merely indicates that
it's "called indirectly", then such a change wouldn't be safe.

I'd probably just leave things as they are; you're not going to get more optimal
code out of it (not that it matters) and it's a minimal footprint in terms of
code readability.

[Peter Kasting, 2010-08-20 21:55:00]

LGTM I guess, WDYT mrossetti?

[rohitrao (ping after 24h), 2010-08-20 22:18:22]

LGTM, with a few nits.

I don't know if history_provider_util.cc should be in the history namespace or
the autocomplete namespace, but that's easy enough to change later.

http://codereview.chromium.org/3177020/diff/16001/17006
File chrome/browser/history/history_types.cc (right):

http://codereview.chromium.org/3177020/diff/16001/17006#newcode1
chrome/browser/history/history_types.cc:1: // Copyright (c) 2006-2010 The
Chromium Authors. All rights reserved.
Just 2010 is enough.  No need to provide a range.

http://codereview.chromium.org/3177020/diff/16001/17008
File chrome/chrome_browser.gypi (right):

http://codereview.chromium.org/3177020/diff/16001/17008#newcode112
chrome/chrome_browser.gypi:112:
'browser/autocomplete/history_quick_provider.cc',
Spaces, not tabs.

http://codereview.chromium.org/3177020/diff/16001/17008#newcode114
chrome/chrome_browser.gypi:114: 'browser/autocomplete/history_url_provider.cc',
Spaces, not tabs.

[mrossetti, 2010-08-20 22:31:58]

LGTM, LVGTM