[Password Manager] Fix IsEnabledSmartLockBranding

[Password Manager] Fix IsEnabledSmartLockBranding

Currently, IsEnabledSmartLockBranding returns true if Finch agrees and the user syncs (anything). It should only return true if Finch agrees and the user syncs passwords without a custom passphrase.

BUG=400674, 486739
Committed: https://crrev.com/29b5a69f88b3268ba6b56d4f8761bdf0a4d173fb
Cr-Commit-Position: refs/heads/master@{#329649}

[vabr (Chromium), 2015-05-13 13:38:47]

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

[vabr (Chromium), 2015-05-13 13:38:47]

Hi Balázs,

Could you please have a look?

We spoke about not checking the sync until sync backends are running in the
right mode -- this is actually verified by the existing call to SyncActive().

There are no tests added for IsEnabledSmartLockBranding, because that's
apparently Vasilii's TODO (see
chrome/browser/ui/passwords/password_bubble_experiment_unittest.cc).

Also, sorry, cannot run tryjobs until
https://codereview.chromium.org/1136403002/ lands.

Cheers,
Vaclav

[engedy, 2015-05-13 14:41:20]

LGTM % a few minor things. Thanks!

https://codereview.chromium.org/1134343002/diff/1/chrome/browser/password_man...
File chrome/browser/password_manager/password_manager_util.h (right):

https://codereview.chromium.org/1134343002/diff/1/chrome/browser/password_man...
chrome/browser/password_manager/password_manager_util.h:40: // Reports whether
and how passwords are currently synced.
nit: We should clarify in the comments that |sync_service| can be null.

https://codereview.chromium.org/1134343002/diff/1/chrome/browser/ui/passwords...
File chrome/browser/ui/passwords/password_bubble_experiment.cc (right):

https://codereview.chromium.org/1134343002/diff/1/chrome/browser/ui/passwords...
chrome/browser/ui/passwords/password_bubble_experiment.cc:10: #include
"chrome/browser/sync/profile_sync_service.h"
nit: I think this include is not needed. The factory should already forward
declare the product type.

https://codereview.chromium.org/1134343002/diff/1/chrome/browser/ui/passwords...
chrome/browser/ui/passwords/password_bubble_experiment.cc:30: return
(password_manager_util::GetPasswordSyncState(sync_service) &&
nit: Are these enclossing parens needed for clang-format to avoid uglification
of the code? If not, we could drop them.

[vabr (Chromium), 2015-05-13 14:55:06]

Thanks!
Vaclav

https://codereview.chromium.org/1134343002/diff/1/chrome/browser/password_man...
File chrome/browser/password_manager/password_manager_util.h (right):

https://codereview.chromium.org/1134343002/diff/1/chrome/browser/password_man...
chrome/browser/password_manager/password_manager_util.h:40: // Reports whether
and how passwords are currently synced.
On 2015/05/13 14:41:20, engedy wrote:
> nit: We should clarify in the comments that |sync_service| can be null.

Done.

https://codereview.chromium.org/1134343002/diff/1/chrome/browser/ui/passwords...
File chrome/browser/ui/passwords/password_bubble_experiment.cc (right):

https://codereview.chromium.org/1134343002/diff/1/chrome/browser/ui/passwords...
chrome/browser/ui/passwords/password_bubble_experiment.cc:10: #include
"chrome/browser/sync/profile_sync_service.h"
On 2015/05/13 14:41:20, engedy wrote:
> nit: I think this include is not needed. The factory should already forward
> declare the product type.

Done. But not in the client -- the factory only forward-declares the
ProfileSyncService, and the client still needs to actually know the definition.

https://codereview.chromium.org/1134343002/diff/1/chrome/browser/ui/passwords...
chrome/browser/ui/passwords/password_bubble_experiment.cc:30: return
(password_manager_util::GetPasswordSyncState(sync_service) &&
On 2015/05/13 14:41:20, engedy wrote:
> nit: Are these enclossing parens needed for clang-format to avoid uglification
> of the code? If not, we could drop them.

Done.

[engedy, 2015-05-13 14:57:33]

https://codereview.chromium.org/1134343002/diff/1/chrome/browser/ui/passwords...
File chrome/browser/ui/passwords/password_bubble_experiment.cc (right):

https://codereview.chromium.org/1134343002/diff/1/chrome/browser/ui/passwords...
chrome/browser/ui/passwords/password_bubble_experiment.cc:10: #include
"chrome/browser/sync/profile_sync_service.h"
On 2015/05/13 14:55:06, vabr (Chromium) wrote:
> On 2015/05/13 14:41:20, engedy wrote:
> > nit: I think this include is not needed. The factory should already forward
> > declare the product type.
> 
> Done. But not in the client -- the factory only forward-declares the
> ProfileSyncService, and the client still needs to actually know the
definition.

Yeah, I only meant removing the include here.

[vabr (Chromium), 2015-05-13 15:39:23]

The CQ bit was checked by vabr@chromium.org

[vabr (Chromium), 2015-05-13 15:39:23]

The patchset sent to the CQ was uploaded after l-g-t-m from engedy@chromium.org
Link to the patchset: https://codereview.chromium.org/1134343002/#ps20001
(title: "Comments addressed")

[commit-bot: I haz the power, 2015-05-13 15:40:35]

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

[commit-bot: I haz the power, 2015-05-13 16:14:19]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-05-13 16:16:07]

Patchset 2 (id:??) landed as
https://crrev.com/29b5a69f88b3268ba6b56d4f8761bdf0a4d173fb
Cr-Commit-Position: refs/heads/master@{#329649}