Don't sync default_search_provider.synced_guid to mobile.

Don't sync default_search_provider.synced_guid to mobile.

Chrome will start sync prefs on mobile. It's not desired behaviour for
the default_search_provider.synced_guid pref.

BUG=622620
TBR=stevet@chromium.org

Committed: https://crrev.com/c1105ec72a099911751513d4a38a6c9ae3840158
Cr-Commit-Position: refs/heads/master@{#403439}

[melandory, 2016-06-23 13:05:48]

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

[commit-bot: I haz the power, 2016-06-23 13:05:54]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2091763002/1

[melandory, 2016-06-23 13:06:13]

melandory@chromium.org changed reviewers:
+ stevet@chromium.org

[melandory, 2016-06-23 13:06:13]

PTAL, thanks!

[commit-bot: I haz the power, 2016-06-23 13:50:48]

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

[commit-bot: I haz the power, 2016-06-23 13:50:49]

Dry run: This issue passed the CQ dry run.

[melandory, 2016-07-01 10:54:29]

On 2016/06/23 13:50:49, commit-bot: I haz the power wrote:
> Dry run: This issue passed the CQ dry run.

Friendly ping.

[melandory, 2016-07-01 10:55:44]

Description was changed from

==========
Don't sync default_search_provider.synced_guid to mobile.

Chrome will start sync prefs on mobile. It's not desired behaviour for
the default_search_provider.synced_guid pref.

BUG=622620
==========

to

==========
Don't sync default_search_provider.synced_guid to mobile.

Chrome will start sync prefs on mobile. It's not desired behaviour for
the default_search_provider.synced_guid pref.

BUG=622620
==========

[melandory, 2016-07-01 10:55:44]

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

[battre, 2016-07-01 11:23:33]

battre@chromium.org changed reviewers:
+ battre@chromium.org

[battre, 2016-07-01 11:23:34]

I think this is a sufficiently small CL that I'll take the blame if Steve or
Peter disagree. Let's get this landed before the branch point.

LGTM

[battre, 2016-07-01 11:24:17]

Description was changed from

==========
Don't sync default_search_provider.synced_guid to mobile.

Chrome will start sync prefs on mobile. It's not desired behaviour for
the default_search_provider.synced_guid pref.

BUG=622620
==========

to

==========
Don't sync default_search_provider.synced_guid to mobile.

Chrome will start sync prefs on mobile. It's not desired behaviour for
the default_search_provider.synced_guid pref.

BUG=622620
TBR=stevet@chromium.org
==========

[battre, 2016-07-01 11:24:28]

The CQ bit was checked by battre@chromium.org

[commit-bot: I haz the power, 2016-07-01 11:24:46]

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-07-01 12:12:15]

Description was changed from

==========
Don't sync default_search_provider.synced_guid to mobile.

Chrome will start sync prefs on mobile. It's not desired behaviour for
the default_search_provider.synced_guid pref.

BUG=622620
TBR=stevet@chromium.org
==========

to

==========
Don't sync default_search_provider.synced_guid to mobile.

Chrome will start sync prefs on mobile. It's not desired behaviour for
the default_search_provider.synced_guid pref.

BUG=622620
TBR=stevet@chromium.org
==========

[commit-bot: I haz the power, 2016-07-01 12:12:16]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-07-01 12:13:33]

Description was changed from

==========
Don't sync default_search_provider.synced_guid to mobile.

Chrome will start sync prefs on mobile. It's not desired behaviour for
the default_search_provider.synced_guid pref.

BUG=622620
TBR=stevet@chromium.org
==========

to

==========
Don't sync default_search_provider.synced_guid to mobile.

Chrome will start sync prefs on mobile. It's not desired behaviour for
the default_search_provider.synced_guid pref.

BUG=622620
TBR=stevet@chromium.org

Committed: https://crrev.com/c1105ec72a099911751513d4a38a6c9ae3840158
Cr-Commit-Position: refs/heads/master@{#403439}
==========

[commit-bot: I haz the power, 2016-07-01 12:13:34]

Patchset 1 (id:??) landed as
https://crrev.com/c1105ec72a099911751513d4a38a6c9ae3840158
Cr-Commit-Position: refs/heads/master@{#403439}

[Peter Kasting, 2016-07-01 20:23:57]

Sorry, I didn't see this review request.  In the future please harass me if you
don't hear anything within 2 days.

I don't understand what problem this is solving.  The CL and the bug both
basically just say "we don't want to do this" without motivating why.  What
problem does syncing this cause?

It worries me that we will be choosing not to sync something that specifically
has "synced" in its name.  Perhaps that's not the right name for this, if we
really don't want to sync it on some platforms.

I would have implemented this slightly differently as well, but let's resolve
that after the larger questions above.

I don't think this needs to be rolled back until I understand what it's doing,
but in the future, if a patch is this small, I wouldn't push to land it before
branching; we can always cherry-pick it shortly after the branch point if it's
safe.

[battre, 2016-07-05 09:46:10]

On 2016/07/01 20:23:57, Peter Kasting wrote:
> Sorry, I didn't see this review request.  In the future please harass me if
you
> don't hear anything within 2 days.
> 
> I don't understand what problem this is solving.  The CL and the bug both
> basically just say "we don't want to do this" without motivating why.  What
> problem does syncing this cause?
> 
> It worries me that we will be choosing not to sync something that specifically
> has "synced" in its name.  Perhaps that's not the right name for this, if we
> really don't want to sync it on some platforms.
> 
> I would have implemented this slightly differently as well, but let's resolve
> that after the larger questions above.
> 
> I don't think this needs to be rolled back until I understand what it's doing,
> but in the future, if a patch is this small, I wouldn't push to land it before
> branching; we can always cherry-pick it shortly after the branch point if it's
> safe.

Hi.

The reason behind not syncing this is that the default search engine (along with
startup tabs and home button URL) is a very frequent target of unwanted software
that hijacks your Windows settings. My feeling is that the cost of syncing this
to mobile phones is larger than the benefit. Hijacked settings are a top user
complaint for the past months.

Best regards,
Dominic

[Peter Kasting, 2016-07-06 20:35:50]

On 2016/07/05 09:46:10, battre wrote:
> On 2016/07/01 20:23:57, Peter Kasting wrote:
> > Sorry, I didn't see this review request.  In the future please harass me if
> you
> > don't hear anything within 2 days.
> > 
> > I don't understand what problem this is solving.  The CL and the bug both
> > basically just say "we don't want to do this" without motivating why.  What
> > problem does syncing this cause?
> > 
> > It worries me that we will be choosing not to sync something that
specifically
> > has "synced" in its name.  Perhaps that's not the right name for this, if we
> > really don't want to sync it on some platforms.
> > 
> > I would have implemented this slightly differently as well, but let's
resolve
> > that after the larger questions above.
> > 
> > I don't think this needs to be rolled back until I understand what it's
doing,
> > but in the future, if a patch is this small, I wouldn't push to land it
before
> > branching; we can always cherry-pick it shortly after the branch point if
it's
> > safe.
> 
> Hi.
> 
> The reason behind not syncing this is that the default search engine (along
with
> startup tabs and home button URL) is a very frequent target of unwanted
software
> that hijacks your Windows settings. My feeling is that the cost of syncing
this
> to mobile phones is larger than the benefit. Hijacked settings are a top user
> complaint for the past months.

But this is also one of the key settings that users would actually want synced. 
And since Android doesn't support custom search engines to my knowledge, how can
syncing this cause problems there?

Furthermore, if we're so under attack by malware that we don't believe it's safe
to sync the DSP, then it's just as unsafe (if not moreso, as just mentioned) to
sync it to other desktop profiles.  It makes no sense at all to do a
mobile-specific restriction here.  We should turn off syncing this globally. 
(Which, IMO, would be disastrous.)

Finally, if we did somehow want to do this, then as mentioned, we really need to
rename variables, clean up and add comments, etc.

Given all of my concerns above, I think this patch is the wrong way to do
whatever we're trying to do, and I think it should be rolled back while this is
discussed more deeply.  Not actually rolling back yet so you have a chance to
respond first.