Remove Finch support for PasswordBranding

Remove Finch support for PasswordBranding

PasswordBranding has launched in the form of 100% being SAVE_PROMPT_ONLY.

This CL removes the code to obtain the Finch group for the experiment, related
tests, and also the functionality only executed for the unused FULL variant of
the experiment (mainly the SmartLock warm welcome).

BUG=486739
Committed: https://crrev.com/063b99026eca67b274692d0879bfdbcd0c9b5008
Cr-Commit-Position: refs/heads/master@{#439740}

[vabr (Chromium), 2016-12-16 14:39:51]

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

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

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

[vabr (Chromium), 2016-12-16 14:45:29]

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

[commit-bot: I haz the power, 2016-12-16 14:45:39]

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, 2016-12-16 15:07:26]

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

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

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[vabr (Chromium), 2016-12-16 15:46:04]

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

[commit-bot: I haz the power, 2016-12-16 15:46:17]

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

[vabr (Chromium), 2016-12-16 15:46:39]

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

[vabr (Chromium), 2016-12-16 15:46:40]

Hi Vasilii,

Could you please review?

Thanks,
Vaclav

[commit-bot: I haz the power, 2016-12-16 16:06:54]

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

[commit-bot: I haz the power, 2016-12-16 16:06:55]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[vabr (Chromium), 2016-12-16 16:19:55]

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

[commit-bot: I haz the power, 2016-12-16 16:20:18]

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, 2016-12-16 16:44:20]

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

[commit-bot: I haz the power, 2016-12-16 16:44:21]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[vasilii, 2016-12-16 16:56:53]

https://codereview.chromium.org/2579823003/diff/60001/chrome/app/generated_re...
File chrome/app/generated_resources.grd (left):

https://codereview.chromium.org/2579823003/diff/60001/chrome/app/generated_re...
chrome/app/generated_resources.grd:12744: <message
name="IDS_MANAGE_PASSWORDS_CONFIRM_GENERATED_TEXT_INFOBAR" desc="A message that
the browser shows after saving a password it has autogenerated for the user.
This message appears in an infobar and contains a link to all the user's saved
autogenerated passwords. The link text is a separate string in the translation
console and appears here as placeholder text.">
Looks like it's Android only. You can mark is as such.

https://codereview.chromium.org/2579823003/diff/60001/chrome/browser/ui/autof...
File chrome/browser/ui/autofill/password_generation_popup_controller_impl.cc
(left):

https://codereview.chromium.org/2579823003/diff/60001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/password_generation_popup_controller_impl.cc:106:
help_text_id = IDS_PASSWORD_GENERATION_SMART_LOCK_PROMPT;
You forgot to remove it. It's in two files.

https://codereview.chromium.org/2579823003/diff/60001/chrome/browser/ui/autof...
File chrome/browser/ui/autofill/password_generation_popup_controller_impl.cc
(right):

https://codereview.chromium.org/2579823003/diff/60001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/password_generation_popup_controller_impl.cc:98:
size_t offset;
= 0; while you are here ;-)

https://codereview.chromium.org/2579823003/diff/60001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/passwords/pending_password_view_controller.mm
(right):

https://codereview.chromium.org/2579823003/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/passwords/pending_password_view_controller.mm:17:
#import
"third_party/google_toolbox_for_mac/src/AppKit/GTMUILocalizerAndLayoutTweaker.h"
used?

https://codereview.chromium.org/2579823003/diff/60001/chrome/browser/ui/passw...
File chrome/browser/ui/passwords/manage_passwords_bubble_model_unittest.cc
(right):

https://codereview.chromium.org/2579823003/diff/60001/chrome/browser/ui/passw...
chrome/browser/ui/passwords/manage_passwords_bubble_model_unittest.cc:428: enum
class SmartLockStatus { ENABLE, DISABLE };
This is not sync status, not smartlock status

https://codereview.chromium.org/2579823003/diff/60001/chrome/browser/ui/passw...
chrome/browser/ui/passwords/manage_passwords_bubble_model_unittest.cc:446:
sync_service->set_smartlock_enabled(test_case.smartlock_status ==
Can you fix TestSyncService to use better names. It's not about smart lock. It's
about "set_sync_passwords(true)"

https://codereview.chromium.org/2579823003/diff/60001/chrome/browser/ui/passw...
chrome/browser/ui/passwords/manage_passwords_bubble_model_unittest.cc:459: const
TitleTestCase kTitleTestCases[] = {
I wonder if constexpr works here.

https://codereview.chromium.org/2579823003/diff/60001/components/password_man...
File components/password_manager/core/browser/password_bubble_experiment.h
(right):

https://codereview.chromium.org/2579823003/diff/60001/components/password_man...
components/password_manager/core/browser/password_bubble_experiment.h:35: bool
ShouldShowSavePromptFirstRunExperience(
TODO: remove too

https://codereview.chromium.org/2579823003/diff/60001/components/password_man...
File
components/password_manager/core/browser/password_bubble_experiment_unittest.cc
(right):

https://codereview.chromium.org/2579823003/diff/60001/components/password_man...
components/password_manager/core/browser/password_bubble_experiment_unittest.cc:45:
bool IsUsingSecondaryPassphrase() const override {
Looks like this class can also be cleaned up.

https://codereview.chromium.org/2579823003/diff/60001/components/password_man...
components/password_manager/core/browser/password_bubble_experiment_unittest.cc:92:
void SetupFakeSyncServiceForTestCase(syncer::ModelType type,
I don't see where it's used.

https://codereview.chromium.org/2579823003/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/MainPreferences.java
(left):

https://codereview.chromium.org/2579823003/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/MainPreferences.java:136:
R.string.prefs_smart_lock_for_passwords));
Do you want to delete IDS_PREFS_SMART_LOCK_FOR_PASSWORDS in
android_chrome_strings.grd?

https://codereview.chromium.org/2579823003/diff/80001/chrome/browser/android/...
File chrome/browser/android/password_ui_view_android.cc (right):

https://codereview.chromium.org/2579823003/diff/80001/chrome/browser/android/...
chrome/browser/android/password_ui_view_android.cc:19: #include
"components/password_manager/core/browser/password_bubble_experiment.h"
seems to be unused.

https://codereview.chromium.org/2579823003/diff/80001/chrome/browser/android/...
chrome/browser/android/password_ui_view_android.cc:22: #include
"components/password_manager/core/common/experiments.h"
unused, maybe other headers too.

https://codereview.chromium.org/2579823003/diff/80001/chrome/browser/password...
File
chrome/browser/password_manager/generated_password_saved_infobar_delegate_android.cc
(right):

https://codereview.chromium.org/2579823003/diff/80001/chrome/browser/password...
chrome/browser/password_manager/generated_password_saved_infobar_delegate_android.cc:30:
size_t offset;
= 0;

https://codereview.chromium.org/2579823003/diff/80001/chrome/browser/password...
File
chrome/browser/password_manager/generated_password_saved_infobar_delegate_android.h
(right):

https://codereview.chromium.org/2579823003/diff/80001/chrome/browser/password...
chrome/browser/password_manager/generated_password_saved_infobar_delegate_android.h:21:
// Creates and shows the infobar. Implemented in the platform-specific file.
I'll pretend that I didn't notice that this file is platform-specific too :-)

https://codereview.chromium.org/2579823003/diff/80001/chrome/browser/ui/andro...
File chrome/browser/ui/android/infobars/generated_password_saved_infobar.cc
(right):

https://codereview.chromium.org/2579823003/diff/80001/chrome/browser/ui/andro...
chrome/browser/ui/android/infobars/generated_password_saved_infobar.cc:23:
base::MakeUnique<GeneratedPasswordSavedInfoBarDelegateAndroid>()));
compilation error here due to base::MakeUnique

[vabr (Chromium), 2016-12-16 17:43:39]

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

[commit-bot: I haz the power, 2016-12-16 17:44:16]

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

[vabr (Chromium), 2016-12-16 18:53:43]

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

[commit-bot: I haz the power, 2016-12-16 18:54:15]

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

[vabr (Chromium), 2016-12-16 18:56:28]

Thanks, Vasilii!

Comments addressed, please have a look.

Cheers,
Vaclav

https://codereview.chromium.org/2579823003/diff/60001/chrome/app/generated_re...
File chrome/app/generated_resources.grd (left):

https://codereview.chromium.org/2579823003/diff/60001/chrome/app/generated_re...
chrome/app/generated_resources.grd:12744: <message
name="IDS_MANAGE_PASSWORDS_CONFIRM_GENERATED_TEXT_INFOBAR" desc="A message that
the browser shows after saving a password it has autogenerated for the user.
This message appears in an infobar and contains a link to all the user's saved
autogenerated passwords. The link text is a separate string in the translation
console and appears here as placeholder text.">
On 2016/12/16 16:56:53, vasilii wrote:
> Looks like it's Android only. You can mark is as such.

Done.

https://codereview.chromium.org/2579823003/diff/60001/chrome/browser/ui/autof...
File chrome/browser/ui/autofill/password_generation_popup_controller_impl.cc
(left):

https://codereview.chromium.org/2579823003/diff/60001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/password_generation_popup_controller_impl.cc:106:
help_text_id = IDS_PASSWORD_GENERATION_SMART_LOCK_PROMPT;
On 2016/12/16 16:56:53, vasilii wrote:
> You forgot to remove it. It's in two files.

Done.

https://codereview.chromium.org/2579823003/diff/60001/chrome/browser/ui/autof...
File chrome/browser/ui/autofill/password_generation_popup_controller_impl.cc
(right):

https://codereview.chromium.org/2579823003/diff/60001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/password_generation_popup_controller_impl.cc:98:
size_t offset;
On 2016/12/16 16:56:53, vasilii wrote:
> = 0; while you are here ;-)

Done.

https://codereview.chromium.org/2579823003/diff/60001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/passwords/pending_password_view_controller.mm
(right):

https://codereview.chromium.org/2579823003/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/passwords/pending_password_view_controller.mm:17:
#import
"third_party/google_toolbox_for_mac/src/AppKit/GTMUILocalizerAndLayoutTweaker.h"
On 2016/12/16 16:56:53, vasilii wrote:
> used?

Not used -> removed.

https://codereview.chromium.org/2579823003/diff/60001/chrome/browser/ui/passw...
File chrome/browser/ui/passwords/manage_passwords_bubble_model_unittest.cc
(right):

https://codereview.chromium.org/2579823003/diff/60001/chrome/browser/ui/passw...
chrome/browser/ui/passwords/manage_passwords_bubble_model_unittest.cc:428: enum
class SmartLockStatus { ENABLE, DISABLE };
On 2016/12/16 16:56:53, vasilii wrote:
> This is not sync status, not smartlock status

Done. Changed this to a newly created TestSyncService:SyncedTypes.

https://codereview.chromium.org/2579823003/diff/60001/chrome/browser/ui/passw...
chrome/browser/ui/passwords/manage_passwords_bubble_model_unittest.cc:446:
sync_service->set_smartlock_enabled(test_case.smartlock_status ==
On 2016/12/16 16:56:53, vasilii wrote:
> Can you fix TestSyncService to use better names. It's not about smart lock.
It's
> about "set_sync_passwords(true)"

Done.

https://codereview.chromium.org/2579823003/diff/60001/chrome/browser/ui/passw...
chrome/browser/ui/passwords/manage_passwords_bubble_model_unittest.cc:459: const
TitleTestCase kTitleTestCases[] = {
On 2016/12/16 16:56:53, vasilii wrote:
> I wonder if constexpr works here.

It does. Also added that to the string constants and the other testcase array.

https://codereview.chromium.org/2579823003/diff/60001/components/password_man...
File components/password_manager/core/browser/password_bubble_experiment.h
(right):

https://codereview.chromium.org/2579823003/diff/60001/components/password_man...
components/password_manager/core/browser/password_bubble_experiment.h:35: bool
ShouldShowSavePromptFirstRunExperience(
On 2016/12/16 16:56:53, vasilii wrote:
> TODO: remove too

Done.

https://codereview.chromium.org/2579823003/diff/60001/components/password_man...
File
components/password_manager/core/browser/password_bubble_experiment_unittest.cc
(right):

https://codereview.chromium.org/2579823003/diff/60001/components/password_man...
components/password_manager/core/browser/password_bubble_experiment_unittest.cc:45:
bool IsUsingSecondaryPassphrase() const override {
On 2016/12/16 16:56:53, vasilii wrote:
> Looks like this class can also be cleaned up.

Not after I add the IsSmartLockUser test.

https://codereview.chromium.org/2579823003/diff/60001/components/password_man...
components/password_manager/core/browser/password_bubble_experiment_unittest.cc:92:
void SetupFakeSyncServiceForTestCase(syncer::ModelType type,
On 2016/12/16 16:56:53, vasilii wrote:
> I don't see where it's used.

It's not, but it should be (we are not testing IsSmartLockUser at all. Will add
that test and put this to some good use.

https://codereview.chromium.org/2579823003/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/MainPreferences.java
(left):

https://codereview.chromium.org/2579823003/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/MainPreferences.java:136:
R.string.prefs_smart_lock_for_passwords));
On 2016/12/16 16:56:53, vasilii wrote:
> Do you want to delete IDS_PREFS_SMART_LOCK_FOR_PASSWORDS in
> android_chrome_strings.grd?

I do. Thanks for pointing out what the IDS_* name is given the strange Java
notation!

https://codereview.chromium.org/2579823003/diff/80001/chrome/browser/android/...
File chrome/browser/android/password_ui_view_android.cc (right):

https://codereview.chromium.org/2579823003/diff/80001/chrome/browser/android/...
chrome/browser/android/password_ui_view_android.cc:19: #include
"components/password_manager/core/browser/password_bubble_experiment.h"
On 2016/12/16 16:56:53, vasilii wrote:
> seems to be unused.

Done.

https://codereview.chromium.org/2579823003/diff/80001/chrome/browser/android/...
chrome/browser/android/password_ui_view_android.cc:22: #include
"components/password_manager/core/common/experiments.h"
On 2016/12/16 16:56:53, vasilii wrote:
> unused, maybe other headers too.

Indeed, the affiliation_utils seemed unused as well.
Done.

https://codereview.chromium.org/2579823003/diff/80001/chrome/browser/password...
File
chrome/browser/password_manager/generated_password_saved_infobar_delegate_android.cc
(right):

https://codereview.chromium.org/2579823003/diff/80001/chrome/browser/password...
chrome/browser/password_manager/generated_password_saved_infobar_delegate_android.cc:30:
size_t offset;
On 2016/12/16 16:56:53, vasilii wrote:
> = 0;

Done.

https://codereview.chromium.org/2579823003/diff/80001/chrome/browser/password...
File
chrome/browser/password_manager/generated_password_saved_infobar_delegate_android.h
(right):

https://codereview.chromium.org/2579823003/diff/80001/chrome/browser/password...
chrome/browser/password_manager/generated_password_saved_infobar_delegate_android.h:21:
// Creates and shows the infobar. Implemented in the platform-specific file.
On 2016/12/16 16:56:53, vasilii wrote:
> I'll pretend that I didn't notice that this file is platform-specific too :-)

Yeah, this is strange. I was wondering whether to move it into the delegate's
.cc, but did not want to introduce new dependencies (it also constructs the
infobar) in this CL. Let's keep this focused on the code removal. I changed the
comment, at least.

https://codereview.chromium.org/2579823003/diff/80001/chrome/browser/ui/andro...
File chrome/browser/ui/android/infobars/generated_password_saved_infobar.cc
(right):

https://codereview.chromium.org/2579823003/diff/80001/chrome/browser/ui/andro...
chrome/browser/ui/android/infobars/generated_password_saved_infobar.cc:23:
base::MakeUnique<GeneratedPasswordSavedInfoBarDelegateAndroid>()));
On 2016/12/16 16:56:53, vasilii wrote:
> compilation error here due to base::MakeUnique

Thanks! Will revert back to WrapUnique.

[vabr (Chromium), 2016-12-16 19:01:44]

vabr@chromium.org changed reviewers:
+ dfalcantara@chromium.org, mathp@chromium.org, stevenjb@chromium.org

[vabr (Chromium), 2016-12-16 19:01:45]

Hello OWNERS, please review:

dfalcantara@:
  //chrome/android
  //chrome/browser/android
  //chrome/browser/ui/android

mathp@:
  //chrome/browser/ui/autofill/password_generation_popup_controller_impl.cc

stevenjb@:
  //chrome/browser/ui/webui/options/password_manager_handler.cc

Thank you!
Vaclav

[Mathieu, 2016-12-16 19:03:57]

c/b/ui/autofill* lgtm

[stevenjb, 2016-12-16 19:23:26]

https://codereview.chromium.org/2579823003/diff/140001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/options/password_manager_handler.cc (right):

https://codereview.chromium.org/2579823003/diff/140001/chrome/browser/ui/webu...
chrome/browser/ui/webui/options/password_manager_handler.cc:139:
IDS_PASSWORDS_EXCEPTIONS_WINDOW_TITLE);
Why not use IsSmartLockUser here?

[commit-bot: I haz the power, 2016-12-16 19:23:34]

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

[commit-bot: I haz the power, 2016-12-16 19:23:35]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[vabr (Chromium), 2016-12-16 19:47:39]

https://codereview.chromium.org/2579823003/diff/140001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/options/password_manager_handler.cc (right):

https://codereview.chromium.org/2579823003/diff/140001/chrome/browser/ui/webu...
chrome/browser/ui/webui/options/password_manager_handler.cc:139:
IDS_PASSWORDS_EXCEPTIONS_WINDOW_TITLE);
On 2016/12/16 19:23:25, stevenjb wrote:
> Why not use IsSmartLockUser here?

We had three levels of SmarLock branding: "none", "save-prompt-related-only",
and "full". We just launched (via Finch) 100%  "save-prompt-related-only".
password_bubble_experiment::IsSmartLockBrandingEnabled corresponds to "full", so
it is equivalent to |false| after the launch.

[vabr (Chromium), 2016-12-16 20:17:32]

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

[commit-bot: I haz the power, 2016-12-16 20:18:31]

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

[vabr (Chromium), 2016-12-16 20:19:59]

One more note for Vasilii:

https://codereview.chromium.org/2579823003/diff/160001/chrome/browser/passwor...
File chrome/browser/password_manager/update_password_infobar_delegate_android.cc
(right):

https://codereview.chromium.org/2579823003/diff/160001/chrome/browser/passwor...
chrome/browser/password_manager/update_password_infobar_delegate_android.cc:43:
? IDS_PASSWORD_MANAGER_SMART_LOCK
The change to IDS_PASSWORD_MANAGER_SMART_LOCK from
IDS_PASSWORD_MANAGER_SMART_LOCK_FOR_PASSWORDS is a small fix, to make Android
use the same string as the other platforms (see manage_passwords_view_utils.cc).

[commit-bot: I haz the power, 2016-12-16 22:36:54]

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

[commit-bot: I haz the power, 2016-12-16 22:36:54]

Dry run: This issue passed the CQ dry run.

[vasilii, 2016-12-19 13:31:02]

https://codereview.chromium.org/2579823003/diff/160001/chrome/browser/passwor...
File chrome/browser/password_manager/update_password_infobar_delegate_android.cc
(right):

https://codereview.chromium.org/2579823003/diff/160001/chrome/browser/passwor...
chrome/browser/password_manager/update_password_infobar_delegate_android.cc:43:
? IDS_PASSWORD_MANAGER_SMART_LOCK
On 2016/12/16 20:19:59, vabr (Chromium) wrote:
> The change to IDS_PASSWORD_MANAGER_SMART_LOCK from
> IDS_PASSWORD_MANAGER_SMART_LOCK_FOR_PASSWORDS is a small fix, to make Android
> use the same string as the other platforms (see
manage_passwords_view_utils.cc).

Acknowledged.

https://codereview.chromium.org/2579823003/diff/160001/chrome/browser/ui/pass...
File chrome/browser/ui/passwords/manage_passwords_bubble_model_unittest.cc
(right):

https://codereview.chromium.org/2579823003/diff/160001/chrome/browser/ui/pass...
chrome/browser/ui/passwords/manage_passwords_bubble_model_unittest.cc:503:
::testing::ValuesIn(kManageLinkTestStatuses));
Values(TestSyncService::SyncedTypes::ALL, TestSyncService::SyncedTypes::NONE); ?

https://codereview.chromium.org/2579823003/diff/160001/components/password_ma...
File
components/password_manager/core/browser/password_bubble_experiment_unittest.cc
(right):

https://codereview.chromium.org/2579823003/diff/160001/components/password_ma...
components/password_manager/core/browser/password_bubble_experiment_unittest.cc:58:
void set_sync_allowed(bool sync_allowed) { is_sync_allowed_ = sync_allowed; }
usused.

https://codereview.chromium.org/2579823003/diff/160001/components/password_ma...
components/password_manager/core/browser/password_bubble_experiment_unittest.cc:60:
void set_first_setup_complete(bool setup_complete) {
usused together with is_first_setup_complete_

https://codereview.chromium.org/2579823003/diff/160001/components/password_ma...
components/password_manager/core/browser/password_bubble_experiment_unittest.cc:163:
EXPECT_TRUE(IsSmartLockUser(sync_service()));
EXPECT_EQ(test_case.expected_smart_lock_user, IsSmartLockUser(sync_service()));
?

[vabr (Chromium), 2016-12-19 13:45:47]

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

[commit-bot: I haz the power, 2016-12-19 13:45:59]

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

[vabr (Chromium), 2016-12-19 13:51:03]

vasilii@ -- thanks, PTAL again!

stevenjb@ -- does my answer from the last week solve your concerns? Do you want
me to include that in the CL description?

dfalcantara@ -- friendly ping :)

mathp@ -- thanks for your fast review last Friday!

Thank you all, I appreciate your time and effort put into reviewing.
Vaclav

https://codereview.chromium.org/2579823003/diff/160001/chrome/browser/ui/pass...
File chrome/browser/ui/passwords/manage_passwords_bubble_model_unittest.cc
(right):

https://codereview.chromium.org/2579823003/diff/160001/chrome/browser/ui/pass...
chrome/browser/ui/passwords/manage_passwords_bubble_model_unittest.cc:503:
::testing::ValuesIn(kManageLinkTestStatuses));
On 2016/12/19 13:31:02, vasilii wrote:
> Values(TestSyncService::SyncedTypes::ALL, TestSyncService::SyncedTypes::NONE);
?

Done.

https://codereview.chromium.org/2579823003/diff/160001/components/password_ma...
File
components/password_manager/core/browser/password_bubble_experiment_unittest.cc
(right):

https://codereview.chromium.org/2579823003/diff/160001/components/password_ma...
components/password_manager/core/browser/password_bubble_experiment_unittest.cc:58:
void set_sync_allowed(bool sync_allowed) { is_sync_allowed_ = sync_allowed; }
On 2016/12/19 13:31:02, vasilii wrote:
> usused.

I see it used in ShouldShowChromeSignInPasswordPromo below.

https://codereview.chromium.org/2579823003/diff/160001/components/password_ma...
components/password_manager/core/browser/password_bubble_experiment_unittest.cc:60:
void set_first_setup_complete(bool setup_complete) {
On 2016/12/19 13:31:02, vasilii wrote:
> usused together with is_first_setup_complete_

I see it used in ShouldShowChromeSignInPasswordPromo below. The data member
(together with is_sync_allowed_) is part of the mocked implementation of
IsSyncAllowed, which overrides the SyncService API used from within the tested
code.

https://codereview.chromium.org/2579823003/diff/160001/components/password_ma...
components/password_manager/core/browser/password_bubble_experiment_unittest.cc:163:
EXPECT_TRUE(IsSmartLockUser(sync_service()));
On 2016/12/19 13:31:02, vasilii wrote:
> EXPECT_EQ(test_case.expected_smart_lock_user,
IsSmartLockUser(sync_service()));
> ?

Done.

[commit-bot: I haz the power, 2016-12-19 14:29:12]

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

[commit-bot: I haz the power, 2016-12-19 14:29:13]

Dry run: This issue passed the CQ dry run.

[vasilii, 2016-12-19 18:09:24]

lgtm

[stevenjb, 2016-12-19 18:26:50]

c/b/ui/webui lgtm

https://codereview.chromium.org/2579823003/diff/140001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/options/password_manager_handler.cc (right):

https://codereview.chromium.org/2579823003/diff/140001/chrome/browser/ui/webu...
chrome/browser/ui/webui/options/password_manager_handler.cc:139:
IDS_PASSWORDS_EXCEPTIONS_WINDOW_TITLE);
On 2016/12/16 19:47:39, vabr (Chromium) wrote:
> On 2016/12/16 19:23:25, stevenjb wrote:
> > Why not use IsSmartLockUser here?
> 
> We had three levels of SmarLock branding: "none", "save-prompt-related-only",
> and "full". We just launched (via Finch) 100%  "save-prompt-related-only".
> password_bubble_experiment::IsSmartLockBrandingEnabled corresponds to "full",
so
> it is equivalent to |false| after the launch.

I was confused because in PS 8 or 9 other code still used
IDS_PASSWORD_MANAGER_SMART_LOCK_FOR_PASSWORD and it was still in
generated_resources.grd. I see that that has changed.

[gone, 2016-12-19 18:32:03]

lgtm

[vabr (Chromium), 2016-12-19 18:52:39]

Thanks for the review, everyone!

And sorry for the mess in the successive patches.

Cheers,
Vaclav

[vabr (Chromium), 2016-12-19 18:52:52]

The CQ bit was checked by vabr@chromium.org

[vabr (Chromium), 2016-12-19 18:52:52]

The patchset sent to the CQ was uploaded after l-g-t-m from mathp@chromium.org
Link to the patchset: https://codereview.chromium.org/2579823003/#ps200001
(title: "More succinct")

[commit-bot: I haz the power, 2016-12-19 18:53:34]

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-12-19 19:03:04]

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

[commit-bot: I haz the power, 2016-12-19 19:03:06]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[vabr (Chromium), 2016-12-19 19:18:33]

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

[vabr (Chromium), 2016-12-19 19:18:34]

Hi stanisc@,

Please review the added dependency in 
components/password_manager/core/browser/DEPS.

Thanks!
Vaclav

[stanisc, 2016-12-19 20:58:04]

components/password_manager/core/browser/DEPS lgtm, but I am not in Sync team
anymore. You might want to include  pavely@ or zea@.

[vabr (Chromium), 2016-12-20 06:38:52]

On 2016/12/19 20:58:04, stanisc wrote:
> components/password_manager/core/browser/DEPS lgtm, but I am not in Sync team
> anymore. You might want to include  pavely@ or zea@.

Thanks for the quick approval!
I will try to remember to involve pavely@ or zea@ next time (updating
components/sync/OWNERS might help my lossy memory :)).
For now, I'm going to land given your approval and the small impact of the DEPS
change (it has effectively been there already, because the existing
components/sync/driver DEPS entry allowed to get ModelType from
components/sync/base via components/sync/driver/fake_sync_service.h).

Cheers,
Vaclav

[vabr (Chromium), 2016-12-20 06:39:02]

The CQ bit was checked by vabr@chromium.org

[commit-bot: I haz the power, 2016-12-20 06:39:16]

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-12-20 07:52:32]

CQ is committing da patch.

Bot data: {"patchset_id": 200001, "attempt_start_ts": 1482215942211150,
"parent_rev": "66a48c8fc2181dabe5d5c0006151fd65a012cf90", "commit_rev":
"6a5adcceaed45e618a1ec34af3ec3d046d0baf09"}

[commit-bot: I haz the power, 2016-12-20 07:52:56]

Description was changed from

==========
Remove Finch support for PasswordBranding

PasswordBranding has launched in the form of 100% being SAVE_PROMPT_ONLY.

This CL removes the code to obtain the Finch group for the experiment, related
tests, and also the functionality only executed for the unused FULL variant of
the experiment (mainly the SmartLock warm welcome).

BUG=486739
==========

to

==========
Remove Finch support for PasswordBranding

PasswordBranding has launched in the form of 100% being SAVE_PROMPT_ONLY.

This CL removes the code to obtain the Finch group for the experiment, related
tests, and also the functionality only executed for the unused FULL variant of
the experiment (mainly the SmartLock warm welcome).

BUG=486739

Review-Url: https://codereview.chromium.org/2579823003
==========

[commit-bot: I haz the power, 2016-12-20 07:52:59]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2016-12-20 07:55:03]

Description was changed from

==========
Remove Finch support for PasswordBranding

PasswordBranding has launched in the form of 100% being SAVE_PROMPT_ONLY.

This CL removes the code to obtain the Finch group for the experiment, related
tests, and also the functionality only executed for the unused FULL variant of
the experiment (mainly the SmartLock warm welcome).

BUG=486739

Review-Url: https://codereview.chromium.org/2579823003
==========

to

==========
Remove Finch support for PasswordBranding

PasswordBranding has launched in the form of 100% being SAVE_PROMPT_ONLY.

This CL removes the code to obtain the Finch group for the experiment, related
tests, and also the functionality only executed for the unused FULL variant of
the experiment (mainly the SmartLock warm welcome).

BUG=486739

Committed: https://crrev.com/063b99026eca67b274692d0879bfdbcd0c9b5008
Cr-Commit-Position: refs/heads/master@{#439740}
==========

[commit-bot: I haz the power, 2016-12-20 07:55:06]

Patchset 11 (id:??) landed as
https://crrev.com/063b99026eca67b274692d0879bfdbcd0c9b5008
Cr-Commit-Position: refs/heads/master@{#439740}