Show password sync status in CBD

Show password sync status in CBD

This adds "(synced)" to the password counter text if there is a stored password and password sync is activated.
I also removed all "browsing_data::" usages from browsing_data_utils.cc because this class is inside the browsing_data namespace.

BUG=681523
Review-Url: https://codereview.chromium.org/2798243004
Cr-Commit-Position: refs/heads/master@{#465566}
Committed: https://chromium.googlesource.com/chromium/src/+/83d87e7a1a0cd89eaa29e8806c8f959106eda34b

[dullweber, 2017-04-06 15:39:41]

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

[commit-bot: I haz the power, 2017-04-06 15:40:14]

Dry run: 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, 2017-04-06 15:49:01]

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

[commit-bot: I haz the power, 2017-04-06 15:49:01]

Dry run: Try jobs failed on following builders:
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[dullweber, 2017-04-07 07:56:02]

Description was changed from

==========
Show password sync status in CBD

BUG=681523
==========

to

==========
Show password sync status in CBD

This adds "(synced)" to the password counter text if there is a stored password
and password sync is activated.
I also removed all "browsing_data::" usages from browsing_data_utils.cc because
this class is inside the browsing_data namespace.

BUG=681523
==========

[dullweber, 2017-04-07 08:03:45]

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

[commit-bot: I haz the power, 2017-04-07 08:03:58]

Dry run: 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, 2017-04-07 08:07:36]

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

[commit-bot: I haz the power, 2017-04-07 08:07:37]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[dullweber, 2017-04-07 08:10:55]

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

[commit-bot: I haz the power, 2017-04-07 08:11:17]

Dry run: 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, 2017-04-07 08:20:57]

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

[commit-bot: I haz the power, 2017-04-07 08:20:58]

Dry run: Try jobs failed on following builders:
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[dullweber, 2017-04-07 11:36:32]

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

[commit-bot: I haz the power, 2017-04-07 11:36:49]

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

[dullweber, 2017-04-07 11:57:27]

dullweber@chromium.org changed reviewers:
+ msramek@chromium.org

[dullweber, 2017-04-07 11:57:28]

Hi,
I extended the PasswordsCounter to show "(synced)" when necessary. 

This change will affect also affect the CBD dialog on Desktop and IOS but I
guess that is ok?

There should probably also be a browsertest for the PasswordCounter with
different sync states but I'm not sure if I should move the
HistoryCounterTest.RestartOnSyncChange test to
chrome/browser/sync/test/integration/ first and add a password test there too?

[msramek, 2017-04-07 12:17:41]

Please do add a test similar to the one about history sync. We can ask the Sync
team if they'd like to move it to their codebase in parallel / later.

I'm OK with changing the strings in the live instances on CBD (Desktop, iOS),
but note that we're between the string freeze and branchpoint. Changing them now
would mean they go to M59 branch untranslated. Please do not land this until the
end of next week.

Some comments below:

https://codereview.chromium.org/2798243004/diff/60001/components/browsing_dat...
File components/browsing_data/core/counters/passwords_counter.cc (right):

https://codereview.chromium.org/2798243004/diff/60001/components/browsing_dat...
components/browsing_data/core/counters/passwords_counter.cc:13: bool
IsSyncEnabled(const syncer::SyncService* sync_service) {
nit: one line padding between the method and the namespace on both sides

nit: IsPasswordsSyncEnabled() to avoid confusion

https://codereview.chromium.org/2798243004/diff/60001/components/browsing_dat...
components/browsing_data/core/counters/passwords_counter.cc:28: if
(store_.get())
Hmm... either we're sure that the store must exist, or we're not. So either we
DCHECK everywhere, or we guard it everywhere, but not both.

https://codereview.chromium.org/2798243004/diff/60001/components/browsing_dat...
components/browsing_data/core/counters/passwords_counter.cc:30: if
(sync_service_)
Ditto here. AFAIK SyncService doesn't have to exist, so the DCHECK on line 36 is
not correct.

https://codereview.chromium.org/2798243004/diff/60001/components/browsing_dat...
components/browsing_data/core/counters/passwords_counter.cc:81: void
PasswordsCounter::OnStateChanged(syncer::SyncService* sync) {
style: Ordering. You have a few PasswordsCounter methods, then PasswordResult,
then PasswordsCounter again.

https://codereview.chromium.org/2798243004/diff/60001/components/browsing_dat...
File components/browsing_data_strings.grdp (right):

https://codereview.chromium.org/2798243004/diff/60001/components/browsing_dat...
components/browsing_data_strings.grdp:34: <message
name="IDS_DEL_PASSWORDS_COUNTER_SYNCED" desc="A counter showing how many synced
passwords the user has.">
To be precise - how many passwords the user has, and that they are synced.

I think your description could lead to a translation saying "5 synced
passwords", and the user could ask - are there also some unsynced passwords that
are not mentioned?

[msramek, 2017-04-07 12:18:28]

msramek@chromium.org changed reviewers:
+ maxwalker@chromium.org

[msramek, 2017-04-07 12:18:28]

+maxwalker@ to please sanity check the string changes in
browsing_data_strings.grdp (will apply to CBD on all platforms)

[commit-bot: I haz the power, 2017-04-07 12:52:26]

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

[commit-bot: I haz the power, 2017-04-07 12:52:27]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[maxwalker, 2017-04-07 15:04:17]

LGTM

[dullweber, 2017-04-11 15:12:49]

I added a sync test and fixed the other issues

https://codereview.chromium.org/2798243004/diff/60001/components/browsing_dat...
File components/browsing_data/core/counters/passwords_counter.cc (right):

https://codereview.chromium.org/2798243004/diff/60001/components/browsing_dat...
components/browsing_data/core/counters/passwords_counter.cc:13: bool
IsSyncEnabled(const syncer::SyncService* sync_service) {
On 2017/04/07 12:17:40, msramek wrote:
> nit: one line padding between the method and the namespace on both sides
> 
> nit: IsPasswordsSyncEnabled() to avoid confusion

Done.

https://codereview.chromium.org/2798243004/diff/60001/components/browsing_dat...
components/browsing_data/core/counters/passwords_counter.cc:28: if
(store_.get())
On 2017/04/07 12:17:40, msramek wrote:
> Hmm... either we're sure that the store must exist, or we're not. So either we
> DCHECK everywhere, or we guard it everywhere, but not both.

It is now expected to always be passed to the counter. Thanks

https://codereview.chromium.org/2798243004/diff/60001/components/browsing_dat...
components/browsing_data/core/counters/passwords_counter.cc:30: if
(sync_service_)
On 2017/04/07 12:17:41, msramek wrote:
> Ditto here. AFAIK SyncService doesn't have to exist, so the DCHECK on line 36
is
> not correct.

Done.

https://codereview.chromium.org/2798243004/diff/60001/components/browsing_dat...
components/browsing_data/core/counters/passwords_counter.cc:81: void
PasswordsCounter::OnStateChanged(syncer::SyncService* sync) {
On 2017/04/07 12:17:40, msramek wrote:
> style: Ordering. You have a few PasswordsCounter methods, then PasswordResult,
> then PasswordsCounter again.

Done.

https://codereview.chromium.org/2798243004/diff/60001/components/browsing_dat...
File components/browsing_data_strings.grdp (right):

https://codereview.chromium.org/2798243004/diff/60001/components/browsing_dat...
components/browsing_data_strings.grdp:34: <message
name="IDS_DEL_PASSWORDS_COUNTER_SYNCED" desc="A counter showing how many synced
passwords the user has.">
On 2017/04/07 12:17:41, msramek wrote:
> To be precise - how many passwords the user has, and that they are synced.
> 
> I think your description could lead to a translation saying "5 synced
> passwords", and the user could ask - are there also some unsynced passwords
that
> are not mentioned?

Yes, that is more clear. Thanks

[dullweber, 2017-04-11 15:13:18]

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

[commit-bot: I haz the power, 2017-04-11 15:13:48]

Dry run: 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, 2017-04-11 16:16:19]

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

[commit-bot: I haz the power, 2017-04-11 16:16:20]

Dry run: This issue passed the CQ dry run.

[dullweber, 2017-04-19 09:05:10]

On 2017/04/07 12:17:41, msramek wrote:
> Please do add a test similar to the one about history sync. We can ask the
Sync
> team if they'd like to move it to their codebase in parallel / later.
> 
> I'm OK with changing the strings in the live instances on CBD (Desktop, iOS),
> but note that we're between the string freeze and branchpoint. Changing them
now
> would mean they go to M59 branch untranslated. Please do not land this until
the
> end of next week.
> 
> Some comments below:
> 
>
https://codereview.chromium.org/2798243004/diff/60001/components/browsing_dat...
> File components/browsing_data/core/counters/passwords_counter.cc (right):
> 
>
https://codereview.chromium.org/2798243004/diff/60001/components/browsing_dat...
> components/browsing_data/core/counters/passwords_counter.cc:13: bool
> IsSyncEnabled(const syncer::SyncService* sync_service) {
> nit: one line padding between the method and the namespace on both sides
> 
> nit: IsPasswordsSyncEnabled() to avoid confusion
> 
>
https://codereview.chromium.org/2798243004/diff/60001/components/browsing_dat...
> components/browsing_data/core/counters/passwords_counter.cc:28: if
> (store_.get())
> Hmm... either we're sure that the store must exist, or we're not. So either we
> DCHECK everywhere, or we guard it everywhere, but not both.
> 
>
https://codereview.chromium.org/2798243004/diff/60001/components/browsing_dat...
> components/browsing_data/core/counters/passwords_counter.cc:30: if
> (sync_service_)
> Ditto here. AFAIK SyncService doesn't have to exist, so the DCHECK on line 36
is
> not correct.
> 
>
https://codereview.chromium.org/2798243004/diff/60001/components/browsing_dat...
> components/browsing_data/core/counters/passwords_counter.cc:81: void
> PasswordsCounter::OnStateChanged(syncer::SyncService* sync) {
> style: Ordering. You have a few PasswordsCounter methods, then PasswordResult,
> then PasswordsCounter again.
> 
>
https://codereview.chromium.org/2798243004/diff/60001/components/browsing_dat...
> File components/browsing_data_strings.grdp (right):
> 
>
https://codereview.chromium.org/2798243004/diff/60001/components/browsing_dat...
> components/browsing_data_strings.grdp:34: <message
> name="IDS_DEL_PASSWORDS_COUNTER_SYNCED" desc="A counter showing how many
synced
> passwords the user has.">
> To be precise - how many passwords the user has, and that they are synced.
> 
> I think your description could lead to a translation saying "5 synced
> passwords", and the user could ask - are there also some unsynced passwords
that
> are not mentioned?

The branch for M59 is done and I added a sync test. Can this be submitted now?

[msramek, 2017-04-19 09:18:54]

Yep! LGTM.

[dullweber, 2017-04-19 09:28:05]

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

[dullweber, 2017-04-19 09:28:06]

droger@chromium.org: Please review a small change in 
ios/chrome/browser/browsing_data/ios_browsing_data_counter_factory.cc
I'm passing a SyncService to the passwords counter in order to determine if
synced passwords will be affected by deleting passwords in Clear Browsing Data.

[droger, 2017-04-19 11:40:47]

ios/ lgtm

[dullweber, 2017-04-19 11:44:03]

The CQ bit was checked by dullweber@chromium.org

[dullweber, 2017-04-19 11:44:03]

The patchset sent to the CQ was uploaded after l-g-t-m from
maxwalker@chromium.org
Link to the patchset: https://codereview.chromium.org/2798243004/#ps80001
(title: "add sync test")

[commit-bot: I haz the power, 2017-04-19 11:44:19]

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, 2017-04-19 12:49:28]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1492602243412160,
"parent_rev": "0211d69efdea283f421b7c402dca9d308d830207", "commit_rev":
"83d87e7a1a0cd89eaa29e8806c8f959106eda34b"}

[commit-bot: I haz the power, 2017-04-19 12:49:43]

Description was changed from

==========
Show password sync status in CBD

This adds "(synced)" to the password counter text if there is a stored password
and password sync is activated.
I also removed all "browsing_data::" usages from browsing_data_utils.cc because
this class is inside the browsing_data namespace.

BUG=681523
==========

to

==========
Show password sync status in CBD

This adds "(synced)" to the password counter text if there is a stored password
and password sync is activated.
I also removed all "browsing_data::" usages from browsing_data_utils.cc because
this class is inside the browsing_data namespace.

BUG=681523

Review-Url: https://codereview.chromium.org/2798243004
Cr-Commit-Position: refs/heads/master@{#465566}
Committed:
https://chromium.googlesource.com/chromium/src/+/83d87e7a1a0cd89eaa29e8806c8f...
==========

[commit-bot: I haz the power, 2017-04-19 12:49:44]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/83d87e7a1a0cd89eaa29e8806c8f...