[Password Manager] Update Confirmation UI for saved password change for Chrome on Android.

chrome/browser/password_manager/update_password_infobar_delegate.cc

Index: chrome/browser/password_manager/update_password_infobar_delegate.cc
diff --git a/chrome/browser/password_manager/update_password_infobar_delegate.cc b/chrome/browser/password_manager/update_password_infobar_delegate.cc
new file mode 100644
index 0000000000000000000000000000000000000000..8460829afaeda08b44ba0e24e2e1b43b340d2fb3
--- /dev/null
+++ b/chrome/browser/password_manager/update_password_infobar_delegate.cc
@@ -0,0 +1,107 @@
+// Copyright 2015 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chrome/browser/password_manager/update_password_infobar_delegate.h"
+
+#include "chrome/browser/infobars/infobar_service.h"
+#include "chrome/browser/password_manager/chrome_password_manager_client.h"
+#include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/sync/profile_sync_service_factory.h"
+#include "chrome/browser/ui/android/infobars/auto_signin_first_run_infobar.h"
+#include "chrome/browser/ui/android/infobars/update_password_infobar.h"
+#include "chrome/browser/ui/passwords/manage_passwords_view_utils.h"
+#include "chrome/grit/chromium_strings.h"
+#include "chrome/grit/generated_resources.h"
+#include "components/browser_sync/browser/profile_sync_service.h"
+#include "components/infobars/core/infobar.h"
+#include "components/password_manager/core/browser/password_bubble_experiment.h"
+#include "content/public/browser/web_contents.h"
+#include "ui/base/l10n/l10n_util.h"
+
+// static
+void UpdatePasswordInfoBarDelegate::Create(
+    content::WebContents* web_contents,
+    scoped_ptr<password_manager::PasswordFormManager> form_to_save) {
+  Profile* profile =
+      Profile::FromBrowserContext(web_contents->GetBrowserContext());
+  sync_driver::SyncService* sync_service =
+      ProfileSyncServiceFactory::GetForProfile(profile);
+  bool is_smartlock_branding_enabled =
+      password_bubble_experiment::IsSmartLockBrandingEnabled(sync_service);
+  UpdatePasswordInfoBarDelegate* update_password_infobar_delegate =

[vabr (Chromium), 2016/01/13 09:44:45]

Please make this a scoped_ptr instead of waiting until line 37 to do that.

[melandory, 2016/01/13 15:11:36]

With scoped ptr it's too long in my taste. I'd better inline the creation.

[vabr (Chromium), 2016/01/13 16:04:34]

Your inlining looks good to me. Just as an alternative for the future, are you
aware of the following short way to write the scoped_ptr definition?

auto update_password_infobar_delegate = make_scoped_ptr(new ...)

Character-wise it should be slightly shorter than the naked pointer, due to
removing the redundant mention of the type name.

+      new UpdatePasswordInfoBarDelegate(web_contents, std::move(form_to_save),
+                                        is_smartlock_branding_enabled);
+  InfoBarService::FromWebContents(web_contents)
+      ->AddInfoBar(make_scoped_ptr(new UpdatePasswordInfoBar(
+          make_scoped_ptr(update_password_infobar_delegate))));
+}
+
+UpdatePasswordInfoBarDelegate::UpdatePasswordInfoBarDelegate(
+    content::WebContents* web_contents,
+    scoped_ptr<password_manager::PasswordFormManager> form_to_update,
+    bool is_smartlock_branding_enabled)
+    : PasswordManagerInfoBarDelegate(),
+      is_smartlock_branding_enabled_(is_smartlock_branding_enabled) {
+  // TODO(melandory): Add histograms.

[vabr (Chromium), 2016/01/13 09:44:45]

optional: It is better to create a bug for the TODO, with all relevant
information (here: what should be measured), and reference the bug number
instead of yourself in the TODO. That will make it easier both for other people
to do that work if needed, and also for yourself if you forget in the meantime.

[melandory, 2016/01/13 15:11:36]

Done.

+  passwords_data_.set_client(
+      ChromePasswordManagerClient::FromWebContents(web_contents));
+  passwords_data_.OnUpdatePassword(std::move(form_to_update));
+  if (is_smartlock_branding_enabled) {
+    base::string16 branding_ = l10n_util::GetStringUTF16(
+        IDS_PASSWORD_MANAGER_SMART_LOCK_FOR_PASSWORDS);
+  } else {
+    branding_ = l10n_util::GetStringUTF16(IDS_PASSWORD_MANAGER_TITLE_BRAND);
+  }
+}
+
+UpdatePasswordInfoBarDelegate::~UpdatePasswordInfoBarDelegate() {}
+
+bool UpdatePasswordInfoBarDelegate::ShowMultipleAccounts() const {
+  const password_manager::PasswordFormManager* form_manager =
+      passwords_data_.form_manager();
+  bool is_password_overriden =
+      form_manager ? form_manager->password_overridden() : false;
+  return GetCurrentForms().size() > 1 && !is_password_overriden;
+}
+
+base::string16 UpdatePasswordInfoBarDelegate::GetUsernameForOneAccountCase() {
+  password_manager::PasswordFormManager* form_manager =
+      passwords_data_.form_manager();
+  return form_manager->pending_credentials().username_value;
+}
+
+void UpdatePasswordInfoBarDelegate::UpdatePasswordInternal(
+    unsigned int form_index) {
+  if (ShowMultipleAccounts()) {
+    CHECK_GE(GetCurrentForms().size(), form_index);  // +-1 error;

[vabr (Chromium), 2016/01/13 09:44:45]

Should this be CHECK_GT? I don't think form_index should be equal to size(),
looking at line 81.

Also, the comment is not clear. Please either make it a sentence, or remove it.

[melandory, 2016/01/13 15:11:36]

My comment is actually saying that I should change it to GT before submitting :)
It's long story why it's not GT on a first place.

+    password_manager::PasswordFormManager* form_manager =
+        passwords_data_.form_manager();
+
+    form_manager->Update(*GetCurrentForms()[form_index]);
+  }
+}
+
+infobars::InfoBarDelegate::InfoBarIdentifier
+UpdatePasswordInfoBarDelegate::GetIdentifier() const {
+  return UPDATE_PASSWORD_INFOBAR_DELEGATE;
+}
+
+base::string16 UpdatePasswordInfoBarDelegate::GetButtonLabel(
+    InfoBarButton button) const {
+  return l10n_util::GetStringUTF16((button == BUTTON_OK)
+                                       ? IDS_PASSWORD_MANAGER_UPDATE_BUTTON
+                                       : IDS_PASSWORD_MANAGER_CANCEL_BUTTON);
+}
+
+bool UpdatePasswordInfoBarDelegate::Accept() {
+  password_manager::PasswordFormManager* form_manager =
+      passwords_data_.form_manager();
+  if (!ShowMultipleAccounts())

[vabr (Chromium), 2016/01/13 09:44:45]

nit: Add a comment explaining what happens if ShowMultipleAccounts is true, and
why it is OK for Accept() to be a no-op then.
(I suppose it is because of UpdatePasswordInfoBar::OnUpdateClicked, but I'm not
completely sure.)

[melandory, 2016/01/13 15:11:36]

I also change a code a bit, so it reflects the update logic more clearly  now (I
hope).

+    form_manager->Update(form_manager->pending_credentials());
+  return true;
+}
+
+bool UpdatePasswordInfoBarDelegate::Cancel() {
+  return true;
+}