Delete from Sync the artificial search engines created by the omnibox extensions.

Delete from Sync the artificial search engines created by the omnibox extensions.

They are not synced for almost two years. However, the old entries may still reside in the server data. Now the client will remove them.

BUG=411197
Committed: https://crrev.com/20991198e3ad681d109e78a9b3407c0b17248671
Cr-Commit-Position: refs/heads/master@{#423489}

[vasilii, 2016-09-22 11:37:45]

The CQ bit was checked by vasilii@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-22 11:38:05]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[vasilii, 2016-09-22 11:38:42]

vasilii@chromium.org changed reviewers:
+ pkasting@chromium.org

[vasilii, 2016-09-22 11:38:42]

Hi Peter,

please review.

[commit-bot: I haz the power, 2016-09-22 12:43:25]

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

[commit-bot: I haz the power, 2016-09-22 12:43:26]

Dry run: This issue passed the CQ dry run.

[Peter Kasting, 2016-09-22 19:01:44]

https://codereview.chromium.org/2354413004/diff/1/components/search_engines/t...
File components/search_engines/template_url_service.cc (right):

https://codereview.chromium.org/2354413004/diff/1/components/search_engines/t...
components/search_engines/template_url_service.cc:1349: //
RestoreExtensionInfoIfNecessary().
Just to make sure I understand: is RestoreExtensionInfoIfNecessary() still
needed today because, since these are still on the server, they are still
getting pulled down and the type/associated extension info incorrectly set (to
not OMNIBOX_API_EXTENSION), so if we were to just delete that function,
everybody's local data would get screwed up?

Does template_url_service_sync_unittest.cc still need a full implementation of
this method?  I'm wondering if that or any associated test code can/should go
away.

[vasilii, 2016-09-28 09:59:31]

The CQ bit was checked by vasilii@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-28 09:59:36]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[vasilii, 2016-09-28 10:00:11]

https://codereview.chromium.org/2354413004/diff/1/components/search_engines/t...
File components/search_engines/template_url_service.cc (right):

https://codereview.chromium.org/2354413004/diff/1/components/search_engines/t...
components/search_engines/template_url_service.cc:1349: //
RestoreExtensionInfoIfNecessary().
On 2016/09/22 19:01:44, Peter Kasting wrote:
> Just to make sure I understand: is RestoreExtensionInfoIfNecessary() still
> needed today because, since these are still on the server, they are still
> getting pulled down and the type/associated extension info incorrectly set (to
> not OMNIBOX_API_EXTENSION), so if we were to just delete that function,
> everybody's local data would get screwed up?
> 
> Does template_url_service_sync_unittest.cc still need a full implementation of
> this method?  I'm wondering if that or any associated test code can/should go
> away.

Until few days ago there was no |type_| in the TemplateURL. It was deduced from
the format of the URL. If we drop RestoreExtensionInfoIfNecessary() then those
entries will become just normal search engines.
I cleaned the test.

[commit-bot: I haz the power, 2016-09-28 11:30:35]

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

[commit-bot: I haz the power, 2016-09-28 11:30:36]

Dry run: This issue passed the CQ dry run.

[Peter Kasting, 2016-09-28 19:34:34]

https://codereview.chromium.org/2354413004/diff/1/components/search_engines/t...
File components/search_engines/template_url_service.cc (right):

https://codereview.chromium.org/2354413004/diff/1/components/search_engines/t...
components/search_engines/template_url_service.cc:1349: //
RestoreExtensionInfoIfNecessary().
On 2016/09/28 10:00:11, vasilii wrote:
> On 2016/09/22 19:01:44, Peter Kasting wrote:
> > Just to make sure I understand: is RestoreExtensionInfoIfNecessary() still
> > needed today because, since these are still on the server, they are still
> > getting pulled down and the type/associated extension info incorrectly set
(to
> > not OMNIBOX_API_EXTENSION), so if we were to just delete that function,
> > everybody's local data would get screwed up?
> > 
> > Does template_url_service_sync_unittest.cc still need a full implementation
of
> > this method?  I'm wondering if that or any associated test code can/should
go
> > away.
> 
> Until few days ago there was no |type_| in the TemplateURL. It was deduced
from
> the format of the URL. If we drop RestoreExtensionInfoIfNecessary() then those
> entries will become just normal search engines.

What I'm thinking is, we don't actually need RestoreExtensionInfoIfNecessary();
we just need to detect the things it would detect.  I.e. we can change the code
here to something like:

  if (GURL(data.url()).SchemeIs(extensions::kExtensionScheme)) {
    ... delete entry and return null

This would avoid calling set_type() on the underlying TemplateURL, which is the
thing we're trying to get rid of.

I was trying to falsify this by looking for a way in which doing this leaves the
local data store corrupted in a way the current code does not, but since |turl|
is a local object, I think we're safe there.

[vasilii, 2016-09-29 11:48:06]

https://codereview.chromium.org/2354413004/diff/1/components/search_engines/t...
File components/search_engines/template_url_service.cc (right):

https://codereview.chromium.org/2354413004/diff/1/components/search_engines/t...
components/search_engines/template_url_service.cc:1349: //
RestoreExtensionInfoIfNecessary().
On 2016/09/28 19:34:34, Peter Kasting (busy Sep 29) wrote:
> On 2016/09/28 10:00:11, vasilii wrote:
> > On 2016/09/22 19:01:44, Peter Kasting wrote:
> > > Just to make sure I understand: is RestoreExtensionInfoIfNecessary() still
> > > needed today because, since these are still on the server, they are still
> > > getting pulled down and the type/associated extension info incorrectly set
> (to
> > > not OMNIBOX_API_EXTENSION), so if we were to just delete that function,
> > > everybody's local data would get screwed up?
> > > 
> > > Does template_url_service_sync_unittest.cc still need a full
implementation
> of
> > > this method?  I'm wondering if that or any associated test code can/should
> go
> > > away.
> > 
> > Until few days ago there was no |type_| in the TemplateURL. It was deduced
> from
> > the format of the URL. If we drop RestoreExtensionInfoIfNecessary() then
those
> > entries will become just normal search engines.
> 
> What I'm thinking is, we don't actually need
RestoreExtensionInfoIfNecessary();
> we just need to detect the things it would detect.  I.e. we can change the
code
> here to something like:
> 
>   if (GURL(data.url()).SchemeIs(extensions::kExtensionScheme)) {
>     ... delete entry and return null
> 
> This would avoid calling set_type() on the underlying TemplateURL, which is
the
> thing we're trying to get rid of.
> 
> I was trying to falsify this by looking for a way in which doing this leaves
the
> local data store corrupted in a way the current code does not, but since
|turl|
> is a local object, I think we're safe there.

We could but it's a layering violation. The component shouldn't know about the
extensions.

[Peter Kasting, 2016-09-30 23:52:24]

On 2016/09/29 11:48:06, vasilii wrote:
>
https://codereview.chromium.org/2354413004/diff/1/components/search_engines/t...
> File components/search_engines/template_url_service.cc (right):
> 
>
https://codereview.chromium.org/2354413004/diff/1/components/search_engines/t...
> components/search_engines/template_url_service.cc:1349: //
> RestoreExtensionInfoIfNecessary().
> On 2016/09/28 19:34:34, Peter Kasting (busy Sep 29) wrote:
> > On 2016/09/28 10:00:11, vasilii wrote:
> > > On 2016/09/22 19:01:44, Peter Kasting wrote:
> > > > Just to make sure I understand: is RestoreExtensionInfoIfNecessary()
still
> > > > needed today because, since these are still on the server, they are
still
> > > > getting pulled down and the type/associated extension info incorrectly
set
> > (to
> > > > not OMNIBOX_API_EXTENSION), so if we were to just delete that function,
> > > > everybody's local data would get screwed up?
> > > > 
> > > > Does template_url_service_sync_unittest.cc still need a full
> implementation
> > of
> > > > this method?  I'm wondering if that or any associated test code
can/should
> > go
> > > > away.
> > > 
> > > Until few days ago there was no |type_| in the TemplateURL. It was deduced
> > from
> > > the format of the URL. If we drop RestoreExtensionInfoIfNecessary() then
> those
> > > entries will become just normal search engines.
> > 
> > What I'm thinking is, we don't actually need
> RestoreExtensionInfoIfNecessary();
> > we just need to detect the things it would detect.  I.e. we can change the
> code
> > here to something like:
> > 
> >   if (GURL(data.url()).SchemeIs(extensions::kExtensionScheme)) {
> >     ... delete entry and return null
> > 
> > This would avoid calling set_type() on the underlying TemplateURL, which is
> the
> > thing we're trying to get rid of.
> > 
> > I was trying to falsify this by looking for a way in which doing this leaves
> the
> > local data store corrupted in a way the current code does not, but since
> |turl|
> > is a local object, I think we're safe there.
> 
> We could but it's a layering violation. The component shouldn't know about the
> extensions.

Sure, that's fine.  But can we change the callee to just something like:

  bool IsOmniboxExtensionURL(const GURL& url);  // Name it whatever you want

And have it just implement this conditional?  That still gets rid of the type
setter, which was my goal.

Basically, I'm trying to get as close to ripping this functionality out now as
we reasonably can :)

[vasilii, 2016-10-04 14:57:29]

The CQ bit was checked by vasilii@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-04 14:57:42]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[vasilii, 2016-10-04 14:57:57]

Done!

[commit-bot: I haz the power, 2016-10-04 15:46:01]

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

[commit-bot: I haz the power, 2016-10-04 15:46:04]

Dry run: This issue passed the CQ dry run.

[Peter Kasting, 2016-10-05 00:15:21]

LGTM

https://codereview.chromium.org/2354413004/diff/40001/chrome/browser/search_e...
File chrome/browser/search_engines/template_url_service_sync_unittest.cc
(right):

https://codereview.chromium.org/2354413004/diff/40001/chrome/browser/search_e...
chrome/browser/search_engines/template_url_service_sync_unittest.cc:167: 
Nit: No blank line

https://codereview.chromium.org/2354413004/diff/40001/components/search_engin...
File components/search_engines/template_url.h (right):

https://codereview.chromium.org/2354413004/diff/40001/components/search_engin...
components/search_engines/template_url.h:757: Type type_;
Nit: Can now be const?

https://codereview.chromium.org/2354413004/diff/40001/components/search_engin...
File components/search_engines/template_url_service_client.h (right):

https://codereview.chromium.org/2354413004/diff/40001/components/search_engin...
components/search_engines/template_url_service_client.h:40: // Returns true iff
|url| has "chrome-extension" scheme.
Nit: Technically this description is a layering violation; it is how the
Chrome-side implementation works, but on the component side, we shouldn't
theoretically know about schemes like this.

Maybe instead say something like: "Given the main search |url| for a
TemplateURL, returns whether the TemplateURL is from an omnibox extension."

https://codereview.chromium.org/2354413004/diff/40001/ios/chrome/browser/sear...
File ios/chrome/browser/search_engines/template_url_service_client_impl.cc
(right):

https://codereview.chromium.org/2354413004/diff/40001/ios/chrome/browser/sear...
ios/chrome/browser/search_engines/template_url_service_client_impl.cc:73: // iOS
does not supports extension.
Nit: "iOS does not support extensions."

[vasilii, 2016-10-05 12:03:48]

https://codereview.chromium.org/2354413004/diff/40001/chrome/browser/search_e...
File chrome/browser/search_engines/template_url_service_sync_unittest.cc
(right):

https://codereview.chromium.org/2354413004/diff/40001/chrome/browser/search_e...
chrome/browser/search_engines/template_url_service_sync_unittest.cc:167: 
On 2016/10/05 00:15:20, Peter Kasting wrote:
> Nit: No blank line

Done.

https://codereview.chromium.org/2354413004/diff/40001/components/search_engin...
File components/search_engines/template_url.h (right):

https://codereview.chromium.org/2354413004/diff/40001/components/search_engin...
components/search_engines/template_url.h:757: Type type_;
On 2016/10/05 00:15:20, Peter Kasting wrote:
> Nit: Can now be const?

Done.

https://codereview.chromium.org/2354413004/diff/40001/components/search_engin...
File components/search_engines/template_url_service_client.h (right):

https://codereview.chromium.org/2354413004/diff/40001/components/search_engin...
components/search_engines/template_url_service_client.h:40: // Returns true iff
|url| has "chrome-extension" scheme.
On 2016/10/05 00:15:21, Peter Kasting wrote:
> Nit: Technically this description is a layering violation; it is how the
> Chrome-side implementation works, but on the component side, we shouldn't
> theoretically know about schemes like this.
> 
> Maybe instead say something like: "Given the main search |url| for a
> TemplateURL, returns whether the TemplateURL is from an omnibox extension."

Done.

https://codereview.chromium.org/2354413004/diff/40001/ios/chrome/browser/sear...
File ios/chrome/browser/search_engines/template_url_service_client_impl.cc
(right):

https://codereview.chromium.org/2354413004/diff/40001/ios/chrome/browser/sear...
ios/chrome/browser/search_engines/template_url_service_client_impl.cc:73: // iOS
does not supports extension.
On 2016/10/05 00:15:21, Peter Kasting wrote:
> Nit: "iOS does not support extensions."

Done.

[vasilii, 2016-10-05 12:06:51]

The CQ bit was checked by vasilii@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-05 12:07:06]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[vasilii, 2016-10-05 12:07:41]

vasilii@chromium.org changed reviewers:
+ sdefresne@chromium.org

[vasilii, 2016-10-05 12:07:42]

Hi Sylvain,

please review template_url_service_client_impl.*

[commit-bot: I haz the power, 2016-10-05 12:43:59]

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

[commit-bot: I haz the power, 2016-10-05 12:44:00]

Dry run: This issue passed the CQ dry run.

[vasilii, 2016-10-06 09:22:12]

vasilii@chromium.org changed reviewers:
+ droger@chromium.org

[vasilii, 2016-10-06 09:22:13]

Hi David,

please review template_url_service_client_impl.*

[droger, 2016-10-06 09:28:32]

ios lgtm

[vasilii, 2016-10-06 09:33:13]

The CQ bit was checked by vasilii@chromium.org

[vasilii, 2016-10-06 09:33:13]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/2354413004/#ps60001
(title: "comments")

[commit-bot: I haz the power, 2016-10-06 09:33:23]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-06 09:39:11]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-10-06 09:40:49]

Description was changed from

==========
Delete from Sync the artificial search engines created by the omnibox
extensions.

They are not synced for almost two years. However, the old entries may still
reside in the server data. Now the client will remove them.

BUG=411197
==========

to

==========
Delete from Sync the artificial search engines created by the omnibox
extensions.

They are not synced for almost two years. However, the old entries may still
reside in the server data. Now the client will remove them.

BUG=411197

Committed: https://crrev.com/20991198e3ad681d109e78a9b3407c0b17248671
Cr-Commit-Position: refs/heads/master@{#423489}
==========

[commit-bot: I haz the power, 2016-10-06 09:40:50]

Patchset 4 (id:??) landed as
https://crrev.com/20991198e3ad681d109e78a9b3407c0b17248671
Cr-Commit-Position: refs/heads/master@{#423489}

[sdefresne, 2016-10-06 15:38:57]

lgtm