Don't sync pref which controls the password manager to mobile.

Don't sync pref which controls the password manager to mobile.

Chrome will start sync prefs on mobile. It's not desired behaviour for the password manager pref, because it collides with the prefs reconciliation.

BUG=622620
Committed: https://crrev.com/9352a69e3b8cbf81e7a46dd9ea07a7e05ece94ee
Cr-Commit-Position: refs/heads/master@{#401592}

[melandory, 2016-06-21 19:25:56]

Patchset #2 (id:20001) has been deleted

[melandory, 2016-06-21 19:26:49]

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

[melandory, 2016-06-21 19:26:50]

Vaclav,

PTAL, thanks!

[vabr (Chromium), 2016-06-22 05:52:51]

Hi Tanja, while the code looks good (as in: it implements what your CL
description says), could you also provide the context for this change? There
should definitely be a bug associated to this CL, explaining why the preference
is now not synced on mobile.

Thanks,
Vaclav

[melandory, 2016-06-22 08:01:24]

Description was changed from

==========
Don't sync pref which controls the password manager to mobile.

BUG=NONE
==========

to

==========
Don't sync pref which controls the password manager to mobile.

Chrome will start sync prefs on mobile. It's not desired behaviour for the
password manager pref, because it collides with the prefs reconciliation. 

BUG=NONE
==========

[melandory, 2016-06-23 08:40:02]

Description was changed from

==========
Don't sync pref which controls the password manager to mobile.

Chrome will start sync prefs on mobile. It's not desired behaviour for the
password manager pref, because it collides with the prefs reconciliation. 

BUG=NONE
==========

to

==========
Don't sync pref which controls the password manager to mobile.

Chrome will start sync prefs on mobile. It's not desired behaviour for the
password manager pref, because it collides with the prefs reconciliation. 

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

[melandory, 2016-06-23 10:14:12]

On 2016/06/22 05:52:51, vabr (Chromium) wrote:
> Hi Tanja, while the code looks good (as in: it implements what your CL
> description says), could you also provide the context for this change? There
> should definitely be a bug associated to this CL, explaining why the
preference
> is now not synced on mobile.
> 
> Thanks,
> Vaclav

PTAL.

[vabr (Chromium), 2016-06-23 10:51:10]

LGTM, thank you!
Cheers,
Vaclav

https://codereview.chromium.org/2084033002/diff/40001/components/password_man...
File components/password_manager/core/browser/password_manager.cc (right):

https://codereview.chromium.org/2084033002/diff/40001/components/password_man...
components/password_manager/core/browser/password_manager.cc:140: flags =
PrefRegistry::NO_REGISTRATION_FLAGS;
Could you make the assignments on lines 140 and 142 definitions and remove line
138? As in:
#if defined(OS_IOS) || defined(OS_ANDROID)
  uint32_t flags = PrefRegistry::NO_REGISTRATION_FLAGS;
#else
  uint32_t flags = user_prefs::PrefRegistrySyncable::SYNCABLE_PREF;
#endif

It is a bit of a hazard to leave variables uninitialised, even though the
current code proceeds with the initialisation immediately after definition.

[melandory, 2016-06-23 11:59:30]

Patchset #3 (id:60001) has been deleted

[melandory, 2016-06-23 12:06:41]

https://codereview.chromium.org/2084033002/diff/40001/components/password_man...
File components/password_manager/core/browser/password_manager.cc (right):

https://codereview.chromium.org/2084033002/diff/40001/components/password_man...
components/password_manager/core/browser/password_manager.cc:140: flags =
PrefRegistry::NO_REGISTRATION_FLAGS;
On 2016/06/23 10:51:10, vabr (Chromium) wrote:
> Could you make the assignments on lines 140 and 142 definitions and remove
line
> 138? As in:
> #if defined(OS_IOS) || defined(OS_ANDROID)
>   uint32_t flags = PrefRegistry::NO_REGISTRATION_FLAGS;
> #else
>   uint32_t flags = user_prefs::PrefRegistrySyncable::SYNCABLE_PREF;
> #endif
> 
> It is a bit of a hazard to leave variables uninitialised, even though the
> current code proceeds with the initialisation immediately after definition.

Done.

[melandory, 2016-06-23 12:06:47]

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

[commit-bot: I haz the power, 2016-06-23 12:06:56]

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

[commit-bot: I haz the power, 2016-06-23 12:57:30]

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

[commit-bot: I haz the power, 2016-06-23 12:57:31]

Dry run: This issue passed the CQ dry run.

[melandory, 2016-06-23 12:59:04]

The CQ bit was checked by melandory@chromium.org

[melandory, 2016-06-23 12:59:05]

The patchset sent to the CQ was uploaded after l-g-t-m from vabr@chromium.org
Link to the patchset: https://codereview.chromium.org/2084033002/#ps80001
(title: "Proper init.")

[commit-bot: I haz the power, 2016-06-23 12:59:09]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2084033002/80001

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

Description was changed from

==========
Don't sync pref which controls the password manager to mobile.

Chrome will start sync prefs on mobile. It's not desired behaviour for the
password manager pref, because it collides with the prefs reconciliation. 

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

to

==========
Don't sync pref which controls the password manager to mobile.

Chrome will start sync prefs on mobile. It's not desired behaviour for the
password manager pref, because it collides with the prefs reconciliation. 

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

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

Committed patchset #3 (id:80001)

[commit-bot: I haz the power, 2016-06-23 13:04:16]

Description was changed from

==========
Don't sync pref which controls the password manager to mobile.

Chrome will start sync prefs on mobile. It's not desired behaviour for the
password manager pref, because it collides with the prefs reconciliation. 

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

to

==========
Don't sync pref which controls the password manager to mobile.

Chrome will start sync prefs on mobile. It's not desired behaviour for the
password manager pref, because it collides with the prefs reconciliation.

BUG=622620

Committed: https://crrev.com/9352a69e3b8cbf81e7a46dd9ea07a7e05ece94ee
Cr-Commit-Position: refs/heads/master@{#401592}
==========

[commit-bot: I haz the power, 2016-06-23 13:04:17]

Patchset 3 (id:??) landed as
https://crrev.com/9352a69e3b8cbf81e7a46dd9ea07a7e05ece94ee
Cr-Commit-Position: refs/heads/master@{#401592}