Factor out logic to determine whether the default search engine or its base URL has changed

Factor out logic to determine whether the default search engine or its base URL has changed

For the implementation of crbug.com/674389 we need to be able to detect when
the default search engine changes, or when base URL of the default search
engine changes. It turns out that we basically track the same changes for
instant search. This CL factors out that common logic into a reusable class
which can listen for those changes.

The existing GoogleURLTracker class only tracks base URL changes when Google
is the default search engine, but not base URL changes that occur as a result
of the default search engine itself changing. This class notifies the listener
of both types of changes.

BUG=674389

[raymes, 2016-12-15 04:04:28]

[email protected] changed reviewers:
+ [email protected]

[raymes, 2016-12-15 04:04:29]

Hey there, PTAL! I will add a test but I wanted to make sure the approach was ok
first. Let me know if this is the right location for this class as well. It's
other client will be in chrome/browser/geolocation. 

Thanks :)

[Marc Treib, 2016-12-15 10:58:06]

Nice! Generally looks good; a few comments below.
I agree this is a useful thing to have, and I'm a big fan of moving this
functionality out of InstantService.
That said, I think chrome/browser/search_engines/ might be a better home for it,
since most of the stuff it uses lives there (TemplateURL, SearchTermsData).
WDYT?

https://codereview.chromium.org/2581553003/diff/1/chrome/browser/search/insta...
File chrome/browser/search/instant_service.cc (right):

https://codereview.chromium.org/2581553003/diff/1/chrome/browser/search/insta...
chrome/browser/search/instant_service.cc:68:
base::Bind(&InstantService::OnSearchEngineBaseURLChanged,
Passing out a callback this early seems dangerous - if the tracker decides to
immediately call it, things will break, since this class isn't initialized yet.
Maybe we should at least put it into the body of the constructor?

https://codereview.chromium.org/2581553003/diff/1/chrome/browser/search/insta...
chrome/browser/search/instant_service.cc:444:
SearchEngineBaseURLTracker::ChangeReason::GOOGLE_BASE_URL_CHANGED);
nit: Add a
bool google_base_url_changed = ...
outside of the loop.

https://codereview.chromium.org/2581553003/diff/1/chrome/browser/search/insta...
File chrome/browser/search/instant_service.h (right):

https://codereview.chromium.org/2581553003/diff/1/chrome/browser/search/insta...
chrome/browser/search/instant_service.h:120:
SearchEngineBaseURLTracker::ChangeReason change_reason);
nit: Move this after the "override" methods now

https://codereview.chromium.org/2581553003/diff/1/chrome/browser/search/searc...
File chrome/browser/search/search_engine_base_url_tracker.cc (right):

https://codereview.chromium.org/2581553003/diff/1/chrome/browser/search/searc...
chrome/browser/search/search_engine_base_url_tracker.cc:16: : profile_(profile),
Looks like the profile is only needed to get the TemplateURLService and the
UIThreadSearchTermsData. For testing, it might be nicer to inject those two
directly. It'd also make the dependencies clear.

https://codereview.chromium.org/2581553003/diff/1/chrome/browser/search/searc...
File chrome/browser/search/search_engine_base_url_tracker.h (right):

https://codereview.chromium.org/2581553003/diff/1/chrome/browser/search/searc...
chrome/browser/search/search_engine_base_url_tracker.h:28:
base::Callback<void(ChangeReason)> base_url_changed_callback);
Pass by const ref.
Also, maybe add an alias like
using SearchEngineChangedCallback = base::Callback<void(ChangeReason)>;
?

https://codereview.chromium.org/2581553003/diff/1/chrome/browser/search/searc...
chrome/browser/search/search_engine_base_url_tracker.h:32: // Returns true if a
base URL of the current search engine is Google.
s/a base URL/the base URL/
?

[raymes, 2016-12-19 05:44:37]

[email protected] changed reviewers:
+ [email protected]

[raymes, 2016-12-19 05:44:38]

+pkasting, ptal :)

https://codereview.chromium.org/2581553003/diff/1/chrome/browser/search/insta...
File chrome/browser/search/instant_service.cc (right):

https://codereview.chromium.org/2581553003/diff/1/chrome/browser/search/insta...
chrome/browser/search/instant_service.cc:68:
base::Bind(&InstantService::OnSearchEngineBaseURLChanged,
On 2016/12/15 10:58:06, Marc Treib (OOO until Jan 12) wrote:
> Passing out a callback this early seems dangerous - if the tracker decides to
> immediately call it, things will break, since this class isn't initialized
yet.
> Maybe we should at least put it into the body of the constructor?

Done. I moved it to effectively the same place as it was before.

https://codereview.chromium.org/2581553003/diff/1/chrome/browser/search/insta...
chrome/browser/search/instant_service.cc:444:
SearchEngineBaseURLTracker::ChangeReason::GOOGLE_BASE_URL_CHANGED);
On 2016/12/15 10:58:06, Marc Treib (OOO until Jan 12) wrote:
> nit: Add a
> bool google_base_url_changed = ...
> outside of the loop.

Done.

https://codereview.chromium.org/2581553003/diff/1/chrome/browser/search/insta...
File chrome/browser/search/instant_service.h (right):

https://codereview.chromium.org/2581553003/diff/1/chrome/browser/search/insta...
chrome/browser/search/instant_service.h:120:
SearchEngineBaseURLTracker::ChangeReason change_reason);
On 2016/12/15 10:58:06, Marc Treib (OOO until Jan 12) wrote:
> nit: Move this after the "override" methods now

Done.

https://codereview.chromium.org/2581553003/diff/1/chrome/browser/search/searc...
File chrome/browser/search/search_engine_base_url_tracker.cc (right):

https://codereview.chromium.org/2581553003/diff/1/chrome/browser/search/searc...
chrome/browser/search/search_engine_base_url_tracker.cc:16: : profile_(profile),
On 2016/12/15 10:58:06, Marc Treib (OOO until Jan 12) wrote:
> Looks like the profile is only needed to get the TemplateURLService and the
> UIThreadSearchTermsData. For testing, it might be nicer to inject those two
> directly. It'd also make the dependencies clear.

Done.

https://codereview.chromium.org/2581553003/diff/1/chrome/browser/search/searc...
File chrome/browser/search/search_engine_base_url_tracker.h (right):

https://codereview.chromium.org/2581553003/diff/1/chrome/browser/search/searc...
chrome/browser/search/search_engine_base_url_tracker.h:28:
base::Callback<void(ChangeReason)> base_url_changed_callback);
On 2016/12/15 10:58:06, Marc Treib (OOO until Jan 12) wrote:
> Pass by const ref.
> Also, maybe add an alias like
> using SearchEngineChangedCallback = base::Callback<void(ChangeReason)>;
> ?

Done.

https://codereview.chromium.org/2581553003/diff/1/chrome/browser/search/searc...
chrome/browser/search/search_engine_base_url_tracker.h:32: // Returns true if a
base URL of the current search engine is Google.
On 2016/12/15 10:58:06, Marc Treib (OOO until Jan 12) wrote:
> s/a base URL/the base URL/
> ?

Done.

[Peter Kasting, 2016-12-19 06:17:13]

The bug, design doc, and CL don't mention the existing GoogleURLTracker class. 
Can you update one or more of these to describe how this relates to that code
and point me to that?

[Peter Kasting, 2016-12-19 06:19:10]

On 2016/12/19 06:17:13, Peter Kasting wrote:
> The bug, design doc, and CL don't mention the existing GoogleURLTracker class.

> Can you update one or more of these to describe how this relates to that code
> and point me to that?

(Clarity: my primary interest is in minimizing overlapping partly
functionality/purpose, so I'm looking for whether these should be combined or in
some way modified to work more closely together)

[raymes, 2016-12-19 06:22:09]

Description was changed from

==========
Factor out logic to determine whether the default search engine or its base URL
has changed

For the implementation of crbug.com/674389 we need to be able to detect when
the default search engine changes, or when base URL of the default search
engine changes. It turns out that we basically track the same changes for
instant search. This CL factors out that common logic into a reusable class
which can listen for those changes.

BUG=674389
==========

to

==========
Factor out logic to determine whether the default search engine or its base URL
has changed

For the implementation of crbug.com/674389 we need to be able to detect when
the default search engine changes, or when base URL of the default search
engine changes. It turns out that we basically track the same changes for
instant search. This CL factors out that common logic into a reusable class
which can listen for those changes.

The existing GoogleURLTracker class only tracks base URL changes when Google
is the default search engine, but not base URL changes that occur as a result
of the default search engine itself changing. This class notifies the listener
of both types of changes.

BUG=674389
==========

[raymes, 2016-12-19 06:24:14]

I updated the CL description. Basically the GoogleURLTracker class only notifies
of Google URL changes, i.e. if Google was the default search engine it would be
enough. But this class also triggers updates if the default search engine itself
changes, e.g. to Yahoo.

[Peter Kasting, 2016-12-19 07:02:15]

On 2016/12/19 06:24:14, raymes wrote:
> I updated the CL description. Basically the GoogleURLTracker class only
notifies
> of Google URL changes, i.e. if Google was the default search engine it would
be
> enough. But this class also triggers updates if the default search engine
itself
> changes, e.g. to Yahoo.

OK.

My suspicion is that one of the following approaches would be better than adding
this class:
* Make TemplateURLService fire finer-grained callbacks than just "something
changed".  A number of clients already want more detail than this.  Provide
enough distinction so clients that would use your new service can just override
the callbacks they care about.
* Make this class track only cases where the engine changes, not cases where the
Google base URL changes.  Have clients register with both services if they care
about both things.

I'm inclined towards the first, since TemplateURLService has the relevant data
here and this will be useful for other existing code too.  Have you considered
either of these options already?

[raymes, 2016-12-19 22:38:32]

On 2016/12/19 07:02:15, Peter Kasting wrote:
> On 2016/12/19 06:24:14, raymes wrote:
> > I updated the CL description. Basically the GoogleURLTracker class only
> notifies
> > of Google URL changes, i.e. if Google was the default search engine it would
> be
> > enough. But this class also triggers updates if the default search engine
> itself
> > changes, e.g. to Yahoo.
> 
> OK.
> 
> My suspicion is that one of the following approaches would be better than
adding
> this class:
> * Make TemplateURLService fire finer-grained callbacks than just "something
> changed".  A number of clients already want more detail than this.  Provide
> enough distinction so clients that would use your new service can just
override
> the callbacks they care about.
> * Make this class track only cases where the engine changes, not cases where
the
> Google base URL changes.  Have clients register with both services if they
care
> about both things.
> 
> I'm inclined towards the first, since TemplateURLService has the relevant data
> here and this will be useful for other existing code too.  Have you considered
> either of these options already?

Hmm, I'd love to be able to get into the guts of TemplateURLService but I'm not
sure that I have the time. It has 8 observer clients and 13 calls to
NotifyObservers.

The second option sounds reasonable but it feels like it would be a tiny class
and perhaps not even worth pulling it out. I'd also be fine to implement the new
thing we need in a similar way to instant_service.cc and not trying to share
code at all if that is a reasonable approach for now. A refactor to improve
TemplateURLServiceObserver could then be scoped out at some later point. WDYT?

[Peter Kasting, 2016-12-19 22:45:29]

On 2016/12/19 22:38:32, raymes wrote:
> On 2016/12/19 07:02:15, Peter Kasting wrote:
> > I'm inclined towards the first, since TemplateURLService has the relevant
data
> > here and this will be useful for other existing code too.  Have you
considered
> > either of these options already?
> 
> Hmm, I'd love to be able to get into the guts of TemplateURLService but I'm
not
> sure that I have the time. It has 8 observer clients and 13 calls to
> NotifyObservers.

Correct, this would not be a completely trivial refactor.  It should be doable
by someone who doesn't understand TemplateURLService, but it will take a bit of
time/thought/care.

> The second option sounds reasonable but it feels like it would be a tiny class
> and perhaps not even worth pulling it out. I'd also be fine to implement the
new
> thing we need in a similar way to instant_service.cc and not trying to share
> code at all if that is a reasonable approach for now. A refactor to improve
> TemplateURLServiceObserver could then be scoped out at some later point. WDYT?

My gut feeling is that I'd rather factor out a class than copy code if the
amount of the copied code is more than a few lines.  It looks like you ended up
saving about 35 LOC from instant_service.cc, which seems like maybe worth
pulling out if that's also about how much space it would take to do this in your
new class?

Either one of these routes feels like a shorter-term solution than reworking
TemplateURLService.  How speculative is your work?  Is this implementation
probably going to stick around long-term?  If so, is there a way that we can get
the time to do the refactor budgeted somewhere so that, while it won't block
your initial implementation, it does get done later and we don't just say "yeah
that'd be nice"?

[raymes, 2016-12-19 22:58:49]

On 2016/12/19 22:45:29, Peter Kasting wrote:
> On 2016/12/19 22:38:32, raymes wrote:
> > On 2016/12/19 07:02:15, Peter Kasting wrote:
> > > I'm inclined towards the first, since TemplateURLService has the relevant
> data
> > > here and this will be useful for other existing code too.  Have you
> considered
> > > either of these options already?
> > 
> > Hmm, I'd love to be able to get into the guts of TemplateURLService but I'm
> not
> > sure that I have the time. It has 8 observer clients and 13 calls to
> > NotifyObservers.
> 
> Correct, this would not be a completely trivial refactor.  It should be doable
> by someone who doesn't understand TemplateURLService, but it will take a bit
of
> time/thought/care.
> 
> > The second option sounds reasonable but it feels like it would be a tiny
class
> > and perhaps not even worth pulling it out. I'd also be fine to implement the
> new
> > thing we need in a similar way to instant_service.cc and not trying to share
> > code at all if that is a reasonable approach for now. A refactor to improve
> > TemplateURLServiceObserver could then be scoped out at some later point.
WDYT?
> 
> My gut feeling is that I'd rather factor out a class than copy code if the
> amount of the copied code is more than a few lines.  It looks like you ended
up
> saving about 35 LOC from instant_service.cc, which seems like maybe worth
> pulling out if that's also about how much space it would take to do this in
your
> new class?

If we're only pulling out the case where the search engine changes, it wouldn't
save as much in instant_service.cc. That's why I was thinking it might not be
worth it.

> 
> Either one of these routes feels like a shorter-term solution than reworking
> TemplateURLService.  How speculative is your work?  Is this implementation
> probably going to stick around long-term?  If so, is there a way that we can
get
> the time to do the refactor budgeted somewhere so that, while it won't block
> your initial implementation, it does get done later and we don't just say
"yeah
> that'd be nice"?

This implementation will probably stick around. 

> If so, is there a way that we can get
> the time to do the refactor budgeted somewhere so that, while it won't block
> your initial implementation, it does get done later and we don't just say
"yeah
> that'd be nice"?

Good question. Unfortunately I really don't have any time to go about that. I
work a lot on the permissions code and we have our own set of code health
problems that I'm trying to help with. Maybe you (or someone else who knows the
search code a bit better) is in a better place to ask for some resources if this
is something that is high value/priority?

[Peter Kasting, 2016-12-19 23:25:26]

After some thought, my suggestion is to go ahead and copy what you need to into
your new class without refactoring, but try to make clear by comments that this
is similar to the instant service code.  File a bug against me for
TemplateURLService refactoring with the idea of more granular callbacks I posted
above, making clear the specific callbacks you'd need for your particular use
case (I can decide what others would also be needed), and, if necessary, how
someone doing this refactoring should simplify these two classes once these bits
are added.

Sound OK?

[raymes, 2016-12-19 23:29:40]

That sounds great, thanks Peter :)

On Tue, 20 Dec 2016 at 10:25 <[email protected]> wrote:

> After some thought, my suggestion is to go ahead and copy what you need to
> into
> your new class without refactoring, but try to make clear by comments that
> this
> is similar to the instant service code. File a bug against me for
> TemplateURLService refactoring with the idea of more granular callbacks I
> posted
> above, making clear the specific callbacks you'd need for your particular
> use
> case (I can decide what others would also be needed), and, if necessary,
> how
> someone doing this refactoring should simplify these two classes once
> these bits
> are added.
>
> Sound OK?
>
>
>
> https://codereview.chromium.org/2581553003/
>

-- 
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 [email protected].

[Peter Kasting, 2017-02-11 01:52:43]

Can this be closed?

[Marc Treib, 2017-02-13 09:21:00]

On 2017/02/11 01:52:43, Peter Kasting wrote:
> Can this be closed?

I still like factoring the URL tracker out of InstantService (i.e. PS 1), even
if it's not used anywhere else. raymes, do you want to do that? Alternatively,
any objections to me stealing this? ;)

[raymes, 2017-02-13 23:24:52]

On 2017/02/13 09:21:00, Marc Treib wrote:
> On 2017/02/11 01:52:43, Peter Kasting wrote:
> > Can this be closed?
> 
> I still like factoring the URL tracker out of InstantService (i.e. PS 1), even
> if it's not used anywhere else. raymes, do you want to do that? Alternatively,
> any objections to me stealing this? ;)

treib: We ended up pursuing a different path so I'm not really interested but
please feel free to take whatever is useful :)