Refactor search and zero suggest providers to use common base class.

Refactor search and zero suggest providers to use common base class.

BUG=

[Maria, 2014-01-24 03:20:10]

Hi all,

In order to implement zero-query psuggest it seems I would need to re-use a
bunch of code in search_provider.cc inside of zero_suggest_provider.cc. Since a
lot of code is shared already, I decided to move the common code up into a base
class, so it can be reused by both providers.

THIS CL IS NOT DONE. I am sending it to you to get early feedback on the
structure of the classes/API. Please ignore style issues/missing tests/etc. I
would like to get some feedback on the general direction and will do more clean
up before I ask you to do a real review.

Thanks!
Maria

[Mark P, 2014-01-24 15:53:29]

At some point, you should add msw@ as a reviewer because he's the one who
developed SearchProvider in its current form and has strong opinions on
what a refactoring should look like.  (He's been a strong advocate for
refactoring for a long time.)

--mark



On Thu, Jan 23, 2014 at 7:20 PM, <mariakhomenko@chromium.org> wrote:

> Reviewers: Peter Kasting, Mark P, H Fung,
>
> Message:
> Hi all,
>
> In order to implement zero-query psuggest it seems I would need to re-use a
> bunch of code in search_provider.cc inside of zero_suggest_provider.cc.
> Since a
> lot of code is shared already, I decided to move the common code up into a
> base
> class, so it can be reused by both providers.
>
> THIS CL IS NOT DONE. I am sending it to you to get early feedback on the
> structure of the classes/API. Please ignore style issues/missing
> tests/etc. I
> would like to get some feedback on the general direction and will do more
> clean
> up before I ask you to do a real review.
>
> Thanks!
> Maria
>
> Description:
> Refactor search and zero suggest providers to use common base class.
>
> BUG=
>
> Please review this at https://codereview.chromium.org/131433003/
>
> SVN Base: svn://svn.chromium.org/chrome/trunk/src
>
> Affected files (+1812, -1605 lines):
>   A chrome/browser/autocomplete/base_search_provider.h
>   A chrome/browser/autocomplete/base_search_provider.cc
>   M chrome/browser/autocomplete/search_provider.h
>   M chrome/browser/autocomplete/search_provider.cc
>   M chrome/browser/autocomplete/zero_suggest_provider.h
>   M chrome/browser/autocomplete/zero_suggest_provider.cc
>   M chrome/chrome_browser.gypi
>
>
>

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

[Mark P, 2014-01-24 21:19:11]

I glanced at the changes to the headers.  This looks like a move in the right
direction to reduce code copying.  I'll leave it up to msw@ to comment further
because he's the person I always look to when thinking about messing with the
structure of these files.  From my first glance, though, I have two comments:
* Are we sure we want to do this?  For instance, if ZeroSuggest turns out to
eval poorly (when will we know?), perhaps we'll instead end up wanting to rip
out ZeroSuggest entirely, which means we'd have SearchProvider split in two for
no good reason.
* I wish there was a good way for me to understand why certain functions in the
base class have a default implementation and some do not (=0).  I'd like a good
mental model for deciding what I should expect subclasses to implement and what
I shouldn't.  (Perhaps some limited code copying--i.e., duplicate
implementations--would be acceptable in order to get a clearer mental model.)

--mark

[mariakhomenko, 2014-01-24 21:28:38]

On Fri, Jan 24, 2014 at 1:19 PM, <mpearson@chromium.org> wrote:

>
> I glanced at the changes to the headers.  This looks like a move in the
> right
> direction to reduce code copying.  I'll leave it up to msw@ to comment
> further
> because he's the person I always look to when thinking about messing with
> the
> structure of these files.  From my first glance, though, I have two
> comments:
> * Are we sure we want to do this?  For instance, if ZeroSuggest turns out
> to
> eval poorly (when will we know?), perhaps we'll instead end up wanting to
> rip
> out ZeroSuggest entirely, which means we'd have SearchProvider split in
> two for
> no good reason.
>

There's a pretty strong feeling, at least on the mobile side of things,
that zero-suggest
is important and we want to do it, and it's just a matter of finding the
right algorithm to get
good results.

EtherSuggest has about 5% CTR on desktop AFAIK. We will be running
EtherSuggest and
MostVisited experiments on mobile in M33 and we think CTR might be better
there since typing
on the phone is tough. GSA has seen really good results with 0-prefix
psuggest, so this is the next thing we will be experimenting with in
Chrome. It's in fact in our 1993
OKRs to launch zero suggest once we have a good algorithm, so I think we
would need this
refactor to happen sooner or later. I think sooner is better. We've already
run into a case where
zero suggest broke after we made a code change to search_provider, but not
a corresponding change
to zero_suggest_provider in the copied code.


> * I wish there was a good way for me to understand why certain functions
> in the
> base class have a default implementation and some do not (=0).  I'd like a
> good
> mental model for deciding what I should expect subclasses to implement and
> what
> I shouldn't.  (Perhaps some limited code copying--i.e., duplicate
> implementations--would be acceptable in order to get a clearer mental
> model.)
>

The way I was generally structuring this is: if most of the functionality
is shared, I would
make a default implementation in BaseSearchProvider with some seams where
each
subclass could provide subclass-specific functionality. The pure abstract
methods are
generally those that are completely different between the two
implementations we have.


>
> --mark
>
>
> https://codereview.chromium.org/131433003/
>

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

[Peter Kasting, 2014-01-24 21:33:13]

I'm actually going to bow out in favor of msw; as Mark says, he's the most
familiar with SearchProvider's structure at this point and would have the most
detailed thoughts on refactoring.

I'm OK with a base class having a seemingly-arbitrary mixture of abstract and
concrete methods based on what the children actually want to do.  I view the
base class more as a common code repository than as a clearly-existing "object"
in a strict OO sense; it's OK if there's no real mental model for the base class
in and of itself.

[Mark P, 2014-01-27 18:02:27]

FYI, Maria's answers satisfied me with regard to both my questions, so I'm
going to step out of the way and hope msw@ can review the details.

--mark



On Fri, Jan 24, 2014 at 1:28 PM, Maria Khomenko
<mariakhomenko@google.com>wrote:

>
>
>
> On Fri, Jan 24, 2014 at 1:19 PM, <mpearson@chromium.org> wrote:
>
>>
>> I glanced at the changes to the headers.  This looks like a move in the
>> right
>> direction to reduce code copying.  I'll leave it up to msw@ to comment
>> further
>> because he's the person I always look to when thinking about messing with
>> the
>> structure of these files.  From my first glance, though, I have two
>> comments:
>> * Are we sure we want to do this?  For instance, if ZeroSuggest turns out
>> to
>> eval poorly (when will we know?), perhaps we'll instead end up wanting to
>> rip
>> out ZeroSuggest entirely, which means we'd have SearchProvider split in
>> two for
>> no good reason.
>>
>
> There's a pretty strong feeling, at least on the mobile side of things,
> that zero-suggest
> is important and we want to do it, and it's just a matter of finding the
> right algorithm to get
> good results.
>
> EtherSuggest has about 5% CTR on desktop AFAIK. We will be running
> EtherSuggest and
> MostVisited experiments on mobile in M33 and we think CTR might be better
> there since typing
> on the phone is tough. GSA has seen really good results with 0-prefix
> psuggest, so this is the next thing we will be experimenting with in
> Chrome. It's in fact in our 1993
> OKRs to launch zero suggest once we have a good algorithm, so I think we
> would need this
> refactor to happen sooner or later. I think sooner is better. We've
> already run into a case where
> zero suggest broke after we made a code change to search_provider, but not
> a corresponding change
>  to zero_suggest_provider in the copied code.
>
>
>> * I wish there was a good way for me to understand why certain functions
>> in the
>> base class have a default implementation and some do not (=0).  I'd like
>> a good
>> mental model for deciding what I should expect subclasses to implement
>> and what
>> I shouldn't.  (Perhaps some limited code copying--i.e., duplicate
>> implementations--would be acceptable in order to get a clearer mental
>> model.)
>>
>
> The way I was generally structuring this is: if most of the functionality
> is shared, I would
> make a default implementation in BaseSearchProvider with some seams where
> each
> subclass could provide subclass-specific functionality. The pure abstract
> methods are
> generally those that are completely different between the two
> implementations we have.
>
>
>>
>> --mark
>>
>>
>> https://codereview.chromium.org/131433003/
>>
>
>

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

[msw, 2014-01-29 00:06:56]

I sent some high-level comments via e-mail. Unfortunately, beyond encouragement
for reducing code duplication, my main advice is a vague desire to see this
change split into more easily review-able pieces with more specific and clear CL
descriptions. Overall, I think a shared base class which implements some common
functionality is a good direction for now.

[Mark P, 2014-03-13 17:19:21]

Given all the separate re-factoring changes that you've now landed, is this big
change now moot?  If so, can you delete/abandon it?

(trying to clean up my codereview inbox)

--mark