Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1490193003:
[Password Manager] Update Confirmation UI for saved password change for Chrome on Android. (Closed)
|
Created:
5 years ago by melandory Modified:
4 years, 10 months ago CC:
sabineb, chromium-reviews, gcasto+watchlist_chromium.org, vabr+watchlistpasswordmanager_chromium.org, mkwst+watchlist-passwords_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Password Manager] Update Confirmation UI for for Chrome on Android.
This CL contains implementation of the Update password infobar:
1.For confirmation from the user that he/she changed password in case when the user has only one credentials for current site. In case saved credentials have a username we present the user with that username in the infobar message and ask is the user what to update password which is corresponding to this username. In case there is no username saved, we're asking user if saved password for this site should be updated or not.
2.For selecting by user which credentials should be updated after change password form submission in case when the user has more than one credentials and we have no clue which credentials is correct. It contains a selector with a list of all credentials for current site.
Mocks: https://folio.googleplex.com/chrome-ux/mocks/321-password-manager/password%20update/mobile#%2Fpassword%20update%20-%20mobile.png%3Fbg=dkgray&c=show
BUG=564674
Committed: https://crrev.com/f5bf9e8f3672962246c4efa694f0496cf87bbb1d
Cr-Commit-Position: refs/heads/master@{#377260}
Patch Set 1 #Patch Set 2 : Rebased on top of master. #Patch Set 3 : #Patch Set 4 : #
Total comments: 21
Patch Set 5 : #
Total comments: 4
Patch Set 6 : #Patch Set 7 : #
Total comments: 26
Patch Set 8 : #
Total comments: 42
Patch Set 9 : #
Total comments: 22
Patch Set 10 : #
Total comments: 28
Patch Set 11 : #Patch Set 12 : #Messages
Total messages: 72 (35 generated)
Description was changed from ========== [Password Manager] Update password forms support for Chrome for Android. BUG= ========== to ========== [Password Manager] Update password forms support for Chrome for Android. BUG=564674 ==========
The CQ bit was checked by melandory@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1490193003/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1490193003/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
Patchset #4 (id:60001) has been deleted
Patchset #3 (id:40001) has been deleted
Patchset #3 (id:80001) has been deleted
Patchset #3 (id:100001) has been deleted
The CQ bit was checked by melandory@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1490193003/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1490193003/140001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
Patchset #4 (id:140001) has been deleted
The CQ bit was checked by melandory@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1490193003/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1490193003/160001
melandory@chromium.org changed reviewers: + vabr@chromium.org
Hi Vaclav, PTAL at files in password_manager/
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
chrome/browser/password_manager/* LGTM with comments. https://codereview.chromium.org/1490193003/diff/160001/chrome/browser/passwor... File chrome/browser/password_manager/update_password_infobar_delegate.cc (right): https://codereview.chromium.org/1490193003/diff/160001/chrome/browser/passwor... chrome/browser/password_manager/update_password_infobar_delegate.cc:32: UpdatePasswordInfoBarDelegate* update_password_infobar_delegate = Please make this a scoped_ptr instead of waiting until line 37 to do that. https://codereview.chromium.org/1490193003/diff/160001/chrome/browser/passwor... chrome/browser/password_manager/update_password_infobar_delegate.cc:46: // TODO(melandory): Add histograms. 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. https://codereview.chromium.org/1490193003/diff/160001/chrome/browser/passwor... chrome/browser/password_manager/update_password_infobar_delegate.cc:77: CHECK_GE(GetCurrentForms().size(), form_index); // +-1 error; 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. https://codereview.chromium.org/1490193003/diff/160001/chrome/browser/passwor... chrome/browser/password_manager/update_password_infobar_delegate.cc:100: if (!ShowMultipleAccounts()) 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.) https://codereview.chromium.org/1490193003/diff/160001/chrome/browser/passwor... File chrome/browser/password_manager/update_password_infobar_delegate.h (right): https://codereview.chromium.org/1490193003/diff/160001/chrome/browser/passwor... chrome/browser/password_manager/update_password_infobar_delegate.h:1: // Copyright 2015 The Chromium Authors. All rights reserved. 2015 -> 2016 https://codereview.chromium.org/1490193003/diff/160001/chrome/browser/passwor... chrome/browser/password_manager/update_password_infobar_delegate.h:51: // Updates the form with index |from_index| in case of multiple credentials or typo: from_index -> form_index https://codereview.chromium.org/1490193003/diff/160001/chrome/browser/passwor... chrome/browser/password_manager/update_password_infobar_delegate.h:53: void UpdatePasswordInternal(unsigned int form_index); |form_index| comes from jint, which is signed. The style guide also says: "Do not use unsigned types to mean "this value should never be < 0". For that, use assertions or run-time checks (as appropriate)." Therefore I suggest using just int here, and add the non-negativeness to the check inside UpdatePasswordInternal. https://codereview.chromium.org/1490193003/diff/160001/chrome/browser/passwor... chrome/browser/password_manager/update_password_infobar_delegate.h:71: DISALLOW_COPY_AND_ASSIGN(UpdatePasswordInfoBarDelegate); #include "base/macros.h" for this macro. https://codereview.chromium.org/1490193003/diff/160001/chrome/browser/ui/andr... File chrome/browser/ui/android/infobars/update_password_infobar.cc (right): https://codereview.chromium.org/1490193003/diff/160001/chrome/browser/ui/andr... chrome/browser/ui/android/infobars/update_password_infobar.cc:1: // Copyright 2015 The Chromium Authors. All rights reserved. 2016 https://codereview.chromium.org/1490193003/diff/160001/chrome/browser/ui/andr... File chrome/browser/ui/android/infobars/update_password_infobar.h (right): https://codereview.chromium.org/1490193003/diff/160001/chrome/browser/ui/andr... chrome/browser/ui/android/infobars/update_password_infobar.h:1: // Copyright 2015 The Chromium Authors. All rights reserved. 2016
Also one more comment: the CL description will need more care and detail. In particular, right now the parsing is ambiguous: Are you updating the support, or supporting the updating? :) It would be also nice to elaborate more in the body of the description, what this added support for updating passwords means. Cheers, Vaclav
The CQ bit was checked by melandory@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1490193003/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1490193003/180001
Description was changed from ========== [Password Manager] Update password forms support for Chrome for Android. BUG=564674 ========== to ========== [Password Manager] Update Confirmation UI for for Chrome on Android. This CL contains implementation of the Update password infobar: 1.For confirmation from the user that he/she changed password in case when the user has only one credentials for current site. In case saved credentials have a username we present the user with that username in the infobar message and ask is the user what to update password which is corresponding to this username. In case there is no username saved, we're asking user if saved password for this site should be updated or not. 2.For selecting by user which credentials should be updated after change password form submission in case when the user has more than one credentials and we have no clue which credentials is correct. It contains a selector with a list of all credentials for current site. Mocks: https://folio.googleplex.com/chrome-ux/mocks/321-password-manager/password%20... BUG=564674 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_chromium_gn_compile_dbg on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...)
Patchset #5 (id:180001) has been deleted
The CQ bit was checked by melandory@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1490193003/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1490193003/200001
https://codereview.chromium.org/1490193003/diff/160001/chrome/browser/passwor... File chrome/browser/password_manager/update_password_infobar_delegate.cc (right): https://codereview.chromium.org/1490193003/diff/160001/chrome/browser/passwor... chrome/browser/password_manager/update_password_infobar_delegate.cc:32: UpdatePasswordInfoBarDelegate* update_password_infobar_delegate = On 2016/01/13 09:44:45, vabr (Chromium) wrote: > Please make this a scoped_ptr instead of waiting until line 37 to do that. With scoped ptr it's too long in my taste. I'd better inline the creation. https://codereview.chromium.org/1490193003/diff/160001/chrome/browser/passwor... chrome/browser/password_manager/update_password_infobar_delegate.cc:46: // TODO(melandory): Add histograms. On 2016/01/13 09:44:45, vabr (Chromium) wrote: > 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. Done. https://codereview.chromium.org/1490193003/diff/160001/chrome/browser/passwor... chrome/browser/password_manager/update_password_infobar_delegate.cc:77: CHECK_GE(GetCurrentForms().size(), form_index); // +-1 error; On 2016/01/13 09:44:45, vabr (Chromium) wrote: > 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. 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. https://codereview.chromium.org/1490193003/diff/160001/chrome/browser/passwor... chrome/browser/password_manager/update_password_infobar_delegate.cc:100: if (!ShowMultipleAccounts()) On 2016/01/13 09:44:45, vabr (Chromium) wrote: > 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.) I also change a code a bit, so it reflects the update logic more clearly now (I hope). https://codereview.chromium.org/1490193003/diff/160001/chrome/browser/passwor... File chrome/browser/password_manager/update_password_infobar_delegate.h (right): https://codereview.chromium.org/1490193003/diff/160001/chrome/browser/passwor... chrome/browser/password_manager/update_password_infobar_delegate.h:1: // Copyright 2015 The Chromium Authors. All rights reserved. On 2016/01/13 09:44:46, vabr (Chromium) wrote: > 2015 -> 2016 Done. https://codereview.chromium.org/1490193003/diff/160001/chrome/browser/passwor... chrome/browser/password_manager/update_password_infobar_delegate.h:51: // Updates the form with index |from_index| in case of multiple credentials or On 2016/01/13 09:44:46, vabr (Chromium) wrote: > typo: from_index -> form_index Done. https://codereview.chromium.org/1490193003/diff/160001/chrome/browser/passwor... chrome/browser/password_manager/update_password_infobar_delegate.h:53: void UpdatePasswordInternal(unsigned int form_index); On 2016/01/13 09:44:46, vabr (Chromium) wrote: > |form_index| comes from jint, which is signed. The style guide also says: "Do > not use unsigned types to mean "this value should never be < 0". For that, use > assertions or run-time checks (as appropriate)." > > Therefore I suggest using just int here, and add the non-negativeness to the > check inside UpdatePasswordInternal. Done. https://codereview.chromium.org/1490193003/diff/160001/chrome/browser/passwor... chrome/browser/password_manager/update_password_infobar_delegate.h:71: DISALLOW_COPY_AND_ASSIGN(UpdatePasswordInfoBarDelegate); On 2016/01/13 09:44:46, vabr (Chromium) wrote: > #include "base/macros.h" for this macro. Done. https://codereview.chromium.org/1490193003/diff/160001/chrome/browser/ui/andr... File chrome/browser/ui/android/infobars/update_password_infobar.cc (right): https://codereview.chromium.org/1490193003/diff/160001/chrome/browser/ui/andr... chrome/browser/ui/android/infobars/update_password_infobar.cc:1: // Copyright 2015 The Chromium Authors. All rights reserved. On 2016/01/13 09:44:46, vabr (Chromium) wrote: > 2016 Done. https://codereview.chromium.org/1490193003/diff/160001/chrome/browser/ui/andr... File chrome/browser/ui/android/infobars/update_password_infobar.h (right): https://codereview.chromium.org/1490193003/diff/160001/chrome/browser/ui/andr... chrome/browser/ui/android/infobars/update_password_infobar.h:1: // Copyright 2015 The Chromium Authors. All rights reserved. On 2016/01/13 09:44:46, vabr (Chromium) wrote: > 2016 Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Still LGTM, some minor comments. Cheers, Vaclav https://codereview.chromium.org/1490193003/diff/160001/chrome/browser/passwor... File chrome/browser/password_manager/update_password_infobar_delegate.cc (right): https://codereview.chromium.org/1490193003/diff/160001/chrome/browser/passwor... chrome/browser/password_manager/update_password_infobar_delegate.cc:32: UpdatePasswordInfoBarDelegate* update_password_infobar_delegate = On 2016/01/13 15:11:36, melandory wrote: > On 2016/01/13 09:44:45, vabr (Chromium) wrote: > > Please make this a scoped_ptr instead of waiting until line 37 to do that. > > With scoped ptr it's too long in my taste. I'd better inline the creation. 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. https://codereview.chromium.org/1490193003/diff/200001/chrome/browser/passwor... File chrome/browser/password_manager/update_password_infobar_delegate.cc (right): https://codereview.chromium.org/1490193003/diff/200001/chrome/browser/passwor... chrome/browser/password_manager/update_password_infobar_delegate.cc:77: CHECK_GT(GetCurrentForms().size(), static_cast<unsigned int>(form_index)); Please do an explicit CHECK_LE(0, form_index); to help the readability. https://codereview.chromium.org/1490193003/diff/200001/chrome/browser/passwor... chrome/browser/password_manager/update_password_infobar_delegate.cc:77: CHECK_GT(GetCurrentForms().size(), static_cast<unsigned int>(form_index)); Instead of static_cast, please do a checked_cast (from base/numerics/safe_conversions.h). https://codereview.chromium.org/1490193003/diff/200001/chrome/browser/passwor... chrome/browser/password_manager/update_password_infobar_delegate.cc:98: UpdatePasswordInternal( Please create a separate method, e.g. UpdatePasswordWithPending, instead of using a special argument value. (If you kept the -1, then at least document it. But I strongly advise against it, it is rather unexpected.) https://codereview.chromium.org/1490193003/diff/200001/chrome/browser/passwor... File chrome/browser/password_manager/update_password_infobar_delegate.h (right): https://codereview.chromium.org/1490193003/diff/200001/chrome/browser/passwor... chrome/browser/password_manager/update_password_infobar_delegate.h:54: void UpdatePasswordInternal(int form_index); optional nit: What about renaming to, e.g., UpdatePasswordWithCurrentForm? The "Internal" sounds to go against this method being public.
The CQ bit was checked by melandory@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1490193003/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1490193003/240001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
melandory@chromium.org changed reviewers: + dfalcantara@chromium.org
dfalcantara@chromium.org: Please review changes in chrome/android/java/src/org/chromium/chrome/browser/infobar/UpdatePasswordInfoBar.java chrome/android/java/strings/android_chrome_strings.grd chrome/browser/android/chrome_jni_registrar.cc chrome/browser/ui/android/infobars/update_password_infobar.h chrome/browser/ui/android/infobars/update_password_infobar.cc
https://chromiumcodereview.appspot.com/1490193003/diff/240001/chrome/android/... File chrome/android/java/src/org/chromium/chrome/browser/infobar/UpdatePasswordInfoBar.java (right): https://chromiumcodereview.appspot.com/1490193003/diff/240001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/infobar/UpdatePasswordInfoBar.java:26: private long mNativePtr; private final long mNativePtr; final everywhere else it makes sense https://chromiumcodereview.appspot.com/1490193003/diff/240001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/infobar/UpdatePasswordInfoBar.java:78: SpannableString username = new SpannableString(usernameToDisplay + downPointingArrow); nit: this isn't really just the username, since it has the down arrow https://chromiumcodereview.appspot.com/1490193003/diff/240001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/infobar/UpdatePasswordInfoBar.java:84: }, 0, username.length(), Spannable.SPAN_EXCLUSIVE_EXCLUSIVE); Shouldn't this be SPAN_INCLUSIVE_EXCLUSIVE? https://chromiumcodereview.appspot.com/1490193003/diff/240001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/infobar/UpdatePasswordInfoBar.java:96: if (isPrimaryButton) nativeOnUpdateClicked(mNativePtr, mSelectedUsername); This would use the infobar/delegate framework more correctly if you: 1) Remove this override completely. 2) Add this to UpdatePasswordInfoBar.java: @CalledByNative private String getSelectedUsername() { return mSelectedUsername; } 3) Have UpdatePasswordInfoBar store a base::android::ScopedJavaGlobalRef<jobject> java_infobar_; (like how AppBannerInfoBarAndroid does it). 4) Make UpdatePasswordInfoBarDelegate::Accept() ask its infobar() to retrieve the String from the Java infobar using Java_UpdatePasswordInfoBar_getSelectedUsername(). https://chromiumcodereview.appspot.com/1490193003/diff/240001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/infobar/UpdatePasswordInfoBar.java:105: createUsernameMessageText(item.getItemId()), TextView.BufferType.SPANNABLE); Why isn't this updating mSelectedUsername? https://chromiumcodereview.appspot.com/1490193003/diff/240001/chrome/android/... File chrome/android/java/strings/android_chrome_strings.grd (right): https://chromiumcodereview.appspot.com/1490193003/diff/240001/chrome/android/... chrome/android/java/strings/android_chrome_strings.grd:829: nit: remove. https://chromiumcodereview.appspot.com/1490193003/diff/240001/chrome/android/... chrome/android/java/strings/android_chrome_strings.grd:2388: <!-- Password Manager strings --> Use the existing IDS_UPDATE_PASSWORD; redefining the exact same string in another file just bloats the APK. https://chromiumcodereview.appspot.com/1490193003/diff/240001/chrome/android/... chrome/android/java/strings/android_chrome_strings.grd:2392: <message name="IDS_UPDATE_PASSWORD_FOR_ACCOUNT" desc="The title of the update password infobar."> Will this string ever be used on non-Android? If so, move it to generated_resources.grd. https://chromiumcodereview.appspot.com/1490193003/diff/240001/chrome/browser/... File chrome/browser/password_manager/chrome_password_manager_client.cc (right): https://chromiumcodereview.appspot.com/1490193003/diff/240001/chrome/browser/... chrome/browser/password_manager/chrome_password_manager_client.cc:227: LOG(ERROR) << "PromptUserToSaveOrUpdatePassword"; Remove all these debugging logs. https://chromiumcodereview.appspot.com/1490193003/diff/240001/chrome/browser/... chrome/browser/password_manager/chrome_password_manager_client.cc:249: #if BUILDFLAG(ANDROID_JAVA_UI) This branch won't do anything at all if ANDROID_JAVA_UI isn't set, but OS_MACOSX is. Can you enter this branch if you're on MACOSX? Did you want the #ifdef to include the condition, as well? https://chromiumcodereview.appspot.com/1490193003/diff/240001/chrome/browser/... File chrome/browser/password_manager/update_password_infobar_delegate.h (right): https://chromiumcodereview.appspot.com/1490193003/diff/240001/chrome/browser/... chrome/browser/password_manager/update_password_infobar_delegate.h:53: // multiple nit: Indentation is wonky. https://chromiumcodereview.appspot.com/1490193003/diff/240001/chrome/browser/... File chrome/browser/ui/android/infobars/update_password_infobar.cc (right): https://chromiumcodereview.appspot.com/1490193003/diff/240001/chrome/browser/... chrome/browser/ui/android/infobars/update_password_infobar.cc:64: RemoveSelf(); I don't think this is right. onButtonClicked() from Java pipes down to UpdatePasswordInfoBarDelegate::Accept(), which returns true and tells the InfoBar to close itself. You're then closing it again here. You should be letting the InfoBarDelegate handle logic, and have it ask its infobar() for details instead of the other way around.
The CQ bit was checked by melandory@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1490193003/280001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1490193003/280001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
Patchset #8 (id:260001) has been deleted
Patchset #8 (id:280001) has been deleted
The CQ bit was checked by melandory@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1490193003/300001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1490193003/300001
The CQ bit was checked by melandory@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1490193003/320001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1490193003/320001
Patchset #8 (id:300001) has been deleted
https://codereview.chromium.org/1490193003/diff/240001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/infobar/UpdatePasswordInfoBar.java (right): https://codereview.chromium.org/1490193003/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/UpdatePasswordInfoBar.java:26: private long mNativePtr; On 2016/01/26 19:59:22, dfalcantara wrote: > private final long mNativePtr; > > final everywhere else it makes sense Done. https://codereview.chromium.org/1490193003/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/UpdatePasswordInfoBar.java:78: SpannableString username = new SpannableString(usernameToDisplay + downPointingArrow); On 2016/01/26 19:59:22, dfalcantara wrote: > nit: this isn't really just the username, since it has the down arrow Done. https://codereview.chromium.org/1490193003/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/UpdatePasswordInfoBar.java:84: }, 0, username.length(), Spannable.SPAN_EXCLUSIVE_EXCLUSIVE); On 2016/01/26 19:59:22, dfalcantara wrote: > Shouldn't this be SPAN_INCLUSIVE_EXCLUSIVE? Yep, thanks https://codereview.chromium.org/1490193003/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/UpdatePasswordInfoBar.java:96: if (isPrimaryButton) nativeOnUpdateClicked(mNativePtr, mSelectedUsername); On 2016/01/26 19:59:22, dfalcantara wrote: > This would use the infobar/delegate framework more correctly if you: > > 1) Remove this override completely. > > 2) Add this to UpdatePasswordInfoBar.java: > @CalledByNative > private String getSelectedUsername() { > return mSelectedUsername; > } > > 3) Have UpdatePasswordInfoBar store a > base::android::ScopedJavaGlobalRef<jobject> java_infobar_; (like how > AppBannerInfoBarAndroid does it). > > 4) Make UpdatePasswordInfoBarDelegate::Accept() ask its infobar() to retrieve > the String from the Java infobar using > Java_UpdatePasswordInfoBar_getSelectedUsername(). Done. https://codereview.chromium.org/1490193003/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/UpdatePasswordInfoBar.java:105: createUsernameMessageText(item.getItemId()), TextView.BufferType.SPANNABLE); On 2016/01/26 19:59:22, dfalcantara wrote: > Why isn't this updating mSelectedUsername? Yes, it is. This is the idea. If menu item is selected we update mSelectedUsername as current choice. https://codereview.chromium.org/1490193003/diff/240001/chrome/android/java/st... File chrome/android/java/strings/android_chrome_strings.grd (right): https://codereview.chromium.org/1490193003/diff/240001/chrome/android/java/st... chrome/android/java/strings/android_chrome_strings.grd:829: On 2016/01/26 19:59:22, dfalcantara wrote: > nit: remove. Done. https://codereview.chromium.org/1490193003/diff/240001/chrome/android/java/st... chrome/android/java/strings/android_chrome_strings.grd:2388: <!-- Password Manager strings --> On 2016/01/26 19:59:22, dfalcantara wrote: > Use the existing IDS_UPDATE_PASSWORD; redefining the exact same string in > another file just bloats the APK. My problem here is that if I pass IDS_UPDATE_PASSWORD, then I have to pass the range for the Smart Lock link. And creating a title will have more conditions. Is there a clean way to use $1 placeholder in Java? https://codereview.chromium.org/1490193003/diff/240001/chrome/android/java/st... chrome/android/java/strings/android_chrome_strings.grd:2392: <message name="IDS_UPDATE_PASSWORD_FOR_ACCOUNT" desc="The title of the update password infobar."> On 2016/01/26 19:59:22, dfalcantara wrote: > Will this string ever be used on non-Android? If so, move it to > generated_resources.grd. Nope, due to difference in the UI, it will never be used on desktop platforms. https://codereview.chromium.org/1490193003/diff/240001/chrome/browser/passwor... File chrome/browser/password_manager/chrome_password_manager_client.cc (right): https://codereview.chromium.org/1490193003/diff/240001/chrome/browser/passwor... chrome/browser/password_manager/chrome_password_manager_client.cc:227: LOG(ERROR) << "PromptUserToSaveOrUpdatePassword"; On 2016/01/26 19:59:22, dfalcantara wrote: > Remove all these debugging logs. Done. https://codereview.chromium.org/1490193003/diff/240001/chrome/browser/passwor... chrome/browser/password_manager/chrome_password_manager_client.cc:249: #if BUILDFLAG(ANDROID_JAVA_UI) On 2016/01/26 19:59:22, dfalcantara wrote: > This branch won't do anything at all if ANDROID_JAVA_UI isn't set, but OS_MACOSX > is. Can you enter this branch if you're on MACOSX? Did you want the #ifdef to > include the condition, as well? Done. https://codereview.chromium.org/1490193003/diff/240001/chrome/browser/passwor... File chrome/browser/password_manager/update_password_infobar_delegate.h (right): https://codereview.chromium.org/1490193003/diff/240001/chrome/browser/passwor... chrome/browser/password_manager/update_password_infobar_delegate.h:53: // multiple On 2016/01/26 19:59:22, dfalcantara wrote: > nit: Indentation is wonky. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Good to go. lgtm https://codereview.chromium.org/1490193003/diff/240001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/infobar/UpdatePasswordInfoBar.java (right): https://codereview.chromium.org/1490193003/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/UpdatePasswordInfoBar.java:105: createUsernameMessageText(item.getItemId()), TextView.BufferType.SPANNABLE); On 2016/01/29 15:56:14, melandory wrote: > On 2016/01/26 19:59:22, dfalcantara wrote: > > Why isn't this updating mSelectedUsername? > > Yes, it is. This is the idea. If menu item is selected we update > mSelectedUsername as current choice. Blarg, I see it now. I mistook the body of createUesrnameMessageText as the constructor's body and thought it wouldn't be set again. Mea culpa. https://codereview.chromium.org/1490193003/diff/240001/chrome/android/java/st... File chrome/android/java/strings/android_chrome_strings.grd (right): https://codereview.chromium.org/1490193003/diff/240001/chrome/android/java/st... chrome/android/java/strings/android_chrome_strings.grd:2388: <!-- Password Manager strings --> On 2016/01/29 15:56:14, melandory wrote: > On 2016/01/26 19:59:22, dfalcantara wrote: > > Use the existing IDS_UPDATE_PASSWORD; redefining the exact same string in > > another file just bloats the APK. > > My problem here is that if I pass IDS_UPDATE_PASSWORD, then I have to pass the > range for the Smart Lock link. And creating a title will have more conditions. > Is there a clean way to use $1 placeholder in Java? Huh. TIL Android uses a different placeholder. Looked exactly the same because my eyes glazed over the ^. Did a git grep and couldn't find any obvious use cases where the templates were converted. I'll ping newt@ about this and ask for a follow up CL if necessary. https://codereview.chromium.org/1490193003/diff/240001/chrome/android/java/st... chrome/android/java/strings/android_chrome_strings.grd:2392: <message name="IDS_UPDATE_PASSWORD_FOR_ACCOUNT" desc="The title of the update password infobar."> On 2016/01/29 15:56:14, melandory wrote: > On 2016/01/26 19:59:22, dfalcantara wrote: > > Will this string ever be used on non-Android? If so, move it to > > generated_resources.grd. > > Nope, due to difference in the UI, it will never be used on desktop platforms. Alright. If it ends up on iOS we can swap it over then. https://codereview.chromium.org/1490193003/diff/320001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/infobar/UpdatePasswordInfoBar.java (right): https://codereview.chromium.org/1490193003/diff/320001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/UpdatePasswordInfoBar.java:31: private final SpannableString mBrandingSpan; nit: swap the ordering of private finals so they're grouped above the privates
melandory@chromium.org changed reviewers: + pkasting@chromium.org
pkasting@chromium.org: Please review changes in components/infobars/core/infobar_delegate.h
https://codereview.chromium.org/1490193003/diff/320001/chrome/browser/passwor... File chrome/browser/password_manager/update_password_infobar_delegate.cc (right): https://codereview.chromium.org/1490193003/diff/320001/chrome/browser/passwor... chrome/browser/password_manager/update_password_infobar_delegate.cc:51: base::string16 branding_ = l10n_util::GetStringUTF16( This aliases the member. I don't think you intended this. https://codereview.chromium.org/1490193003/diff/320001/chrome/browser/passwor... chrome/browser/password_manager/update_password_infobar_delegate.cc:55: } Nit: Shorter: branding_ = l10n_util::GetStringUTF16( is_smartlock_branding_enabled ? IDS_PASSWORD_MANAGER_SMART_LOCK_FOR_PASSWORDS : IDS_PASSWORD_MANAGER_TITLE_BRAND); https://codereview.chromium.org/1490193003/diff/320001/chrome/browser/passwor... chrome/browser/password_manager/update_password_infobar_delegate.cc:65: return GetCurrentForms().size() > 1 && !is_password_overriden; Nit: Shorter: return GetCurrentForms().size() > 1 && (!form_manager || !form_manager->password_overridden()); https://codereview.chromium.org/1490193003/diff/320001/chrome/browser/passwor... chrome/browser/password_manager/update_password_infobar_delegate.cc:104: void UpdatePasswordInfoBarDelegate::UpdatePasswordWithCurrentForm( Nit: Function definition order should match declaration order. https://codereview.chromium.org/1490193003/diff/320001/chrome/browser/passwor... chrome/browser/password_manager/update_password_infobar_delegate.cc:106: CHECK_LT(base::checked_cast<unsigned int>(form_index), Why is this a CHECK and not a DCHECK? We should normally be using the latter unless there's a security issue mandating the former or you're in the midst of tracking down a bug as to why this isn't working. Also, you shouldn't be casting to unsigned int, you should be casting to size_t. I would also avoid checked_cast here and just do: DCHECK_GE(form_index, 0); DCHECK_LT(static_cast<size_t>(form_index), GetCurrentForms().size()); https://codereview.chromium.org/1490193003/diff/320001/chrome/browser/passwor... File chrome/browser/password_manager/update_password_infobar_delegate.h (right): https://codereview.chromium.org/1490193003/diff/320001/chrome/browser/passwor... chrome/browser/password_manager/update_password_infobar_delegate.h:34: Nit: Blank line unnecessary https://codereview.chromium.org/1490193003/diff/320001/chrome/browser/passwor... chrome/browser/password_manager/update_password_infobar_delegate.h:41: // credentials save for the web site. Nit: Maybe "Returns true if an infobar should be shown when there are multiple matching credentials, e.g. when a user uses a password change form on a site with several saved credentials." Although that assumes I've understood the intent of this function correctly. Maybe it's not actually "given that there are multiple accounts, return whether to show the infobar" and more "given that we're showing an infobar, return whether it will talk about multiple accounts". https://codereview.chromium.org/1490193003/diff/320001/chrome/browser/passwor... chrome/browser/password_manager/update_password_infobar_delegate.h:44: const std::vector<const autofill::PasswordForm*>& GetCurrentForms() const { Do not inline CamelCase functions. This should be defined out-of-line. https://codereview.chromium.org/1490193003/diff/320001/chrome/browser/passwor... chrome/browser/password_manager/update_password_infobar_delegate.h:48: // Returns the username of the saved credentials in case when there is only Nit: in -> in the https://codereview.chromium.org/1490193003/diff/320001/chrome/browser/passwor... chrome/browser/password_manager/update_password_infobar_delegate.h:56: bool Cancel() override; Nit: Do these really need to be public? Make them private if possible. https://codereview.chromium.org/1490193003/diff/320001/chrome/browser/passwor... chrome/browser/password_manager/update_password_infobar_delegate.h:65: // multiple choice available. This comment makes no sense to me. I don't think we should have this helper anyway; it's very short and called only once, and its contents would be better just inlined into the lone caller. In fact, I would probably inline the two methods below as well, since they share common prologue code (get the form manager) and epilogue (call Update() on it) and thus that could be common-factored out of the inlined implementations. https://codereview.chromium.org/1490193003/diff/320001/chrome/browser/passwor... chrome/browser/password_manager/update_password_infobar_delegate.h:71: // Updates the form with index |form_index| in case of multiple credentials. Nit: in -> in the https://codereview.chromium.org/1490193003/diff/320001/chrome/browser/ui/andr... File chrome/browser/ui/android/infobars/update_password_infobar.cc (right): https://codereview.chromium.org/1490193003/diff/320001/chrome/browser/ui/andr... chrome/browser/ui/android/infobars/update_password_infobar.cc:22: UpdatePasswordInfoBar::CreateRenderInfoBar(JNIEnv* env) { Nit: Function definition order should match declaration order https://codereview.chromium.org/1490193003/diff/320001/chrome/browser/ui/andr... chrome/browser/ui/android/infobars/update_password_infobar.cc:36: for (auto password_form : update_password_delegate->GetCurrentForms()) { Nit: No {} https://codereview.chromium.org/1490193003/diff/320001/chrome/browser/ui/andr... chrome/browser/ui/android/infobars/update_password_infobar.cc:44: base::android::ScopedJavaLocalRef<jobjectArray> java_usernames = Nit: No need to qualify these https://codereview.chromium.org/1490193003/diff/320001/chrome/browser/ui/andr... chrome/browser/ui/android/infobars/update_password_infobar.cc:71: scoped_ptr<infobars::InfoBar> CreateUpdatePasswordInfoBar( This function is not declared or referenced anywhere. I'd remove it. https://codereview.chromium.org/1490193003/diff/320001/chrome/browser/ui/andr... File chrome/browser/ui/android/infobars/update_password_infobar.h (right): https://codereview.chromium.org/1490193003/diff/320001/chrome/browser/ui/andr... chrome/browser/ui/android/infobars/update_password_infobar.h:19: Nit: Unnecessary blank line https://codereview.chromium.org/1490193003/diff/320001/chrome/browser/ui/andr... chrome/browser/ui/android/infobars/update_password_infobar.h:24: static bool Register(JNIEnv* env); Nit: Prefer to place static functions above non-static non-constructor/destructors https://codereview.chromium.org/1490193003/diff/320001/chrome/browser/ui/andr... chrome/browser/ui/android/infobars/update_password_infobar.h:32: Nit: Unnecessary blank line
https://codereview.chromium.org/1490193003/diff/320001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/infobar/UpdatePasswordInfoBar.java (right): https://codereview.chromium.org/1490193003/diff/320001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/UpdatePasswordInfoBar.java:31: private final SpannableString mBrandingSpan; On 2016/01/29 18:31:24, dfalcantara wrote: > nit: swap the ordering of private finals so they're grouped above the privates Done. https://codereview.chromium.org/1490193003/diff/320001/chrome/browser/passwor... File chrome/browser/password_manager/update_password_infobar_delegate.cc (right): https://codereview.chromium.org/1490193003/diff/320001/chrome/browser/passwor... chrome/browser/password_manager/update_password_infobar_delegate.cc:51: base::string16 branding_ = l10n_util::GetStringUTF16( On 2016/02/03 00:56:27, Peter Kasting wrote: > This aliases the member. I don't think you intended this. Done. https://codereview.chromium.org/1490193003/diff/320001/chrome/browser/passwor... chrome/browser/password_manager/update_password_infobar_delegate.cc:55: } On 2016/02/03 00:56:26, Peter Kasting wrote: > Nit: Shorter: > > branding_ = l10n_util::GetStringUTF16( > is_smartlock_branding_enabled > ? IDS_PASSWORD_MANAGER_SMART_LOCK_FOR_PASSWORDS > : IDS_PASSWORD_MANAGER_TITLE_BRAND); Done. https://codereview.chromium.org/1490193003/diff/320001/chrome/browser/passwor... chrome/browser/password_manager/update_password_infobar_delegate.cc:65: return GetCurrentForms().size() > 1 && !is_password_overriden; On 2016/02/03 00:56:26, Peter Kasting wrote: > Nit: Shorter: > > return GetCurrentForms().size() > 1 && > (!form_manager || !form_manager->password_overridden()); I actually prefer current version, I think it's easier to read. https://codereview.chromium.org/1490193003/diff/320001/chrome/browser/passwor... chrome/browser/password_manager/update_password_infobar_delegate.cc:104: void UpdatePasswordInfoBarDelegate::UpdatePasswordWithCurrentForm( On 2016/02/03 00:56:27, Peter Kasting wrote: > Nit: Function definition order should match declaration order. Done. https://codereview.chromium.org/1490193003/diff/320001/chrome/browser/passwor... chrome/browser/password_manager/update_password_infobar_delegate.cc:106: CHECK_LT(base::checked_cast<unsigned int>(form_index), On 2016/02/03 00:56:27, Peter Kasting wrote: > Why is this a CHECK and not a DCHECK? We should normally be using the latter > unless there's a security issue mandating the former or you're in the midst of > tracking down a bug as to why this isn't working. > > Also, you shouldn't be casting to unsigned int, you should be casting to size_t. > > I would also avoid checked_cast here and just do: > > DCHECK_GE(form_index, 0); > DCHECK_LT(static_cast<size_t>(form_index), GetCurrentForms().size()); Done. https://codereview.chromium.org/1490193003/diff/320001/chrome/browser/passwor... File chrome/browser/password_manager/update_password_infobar_delegate.h (right): https://codereview.chromium.org/1490193003/diff/320001/chrome/browser/passwor... chrome/browser/password_manager/update_password_infobar_delegate.h:34: On 2016/02/03 00:56:27, Peter Kasting wrote: > Nit: Blank line unnecessary Done. https://codereview.chromium.org/1490193003/diff/320001/chrome/browser/passwor... chrome/browser/password_manager/update_password_infobar_delegate.h:41: // credentials save for the web site. On 2016/02/03 00:56:27, Peter Kasting wrote: > Nit: Maybe "Returns true if an infobar should be shown when there are multiple > matching credentials, e.g. when a user uses a password change form on a site > with several saved credentials." > > Although that assumes I've understood the intent of this function correctly. > Maybe it's not actually "given that there are multiple accounts, return whether > to show the infobar" and more "given that we're showing an infobar, return > whether it will talk about multiple accounts". We show infobar even in case of one credentials. This helper functions tell us that we should show special case of update infobar -> update infobar for case, when user has multiple credentials stored https://codereview.chromium.org/1490193003/diff/320001/chrome/browser/passwor... chrome/browser/password_manager/update_password_infobar_delegate.h:44: const std::vector<const autofill::PasswordForm*>& GetCurrentForms() const { On 2016/02/03 00:56:27, Peter Kasting wrote: > Do not inline CamelCase functions. This should be defined out-of-line. Done. https://codereview.chromium.org/1490193003/diff/320001/chrome/browser/passwor... chrome/browser/password_manager/update_password_infobar_delegate.h:48: // Returns the username of the saved credentials in case when there is only On 2016/02/03 00:56:27, Peter Kasting wrote: > Nit: in -> in the Done. https://codereview.chromium.org/1490193003/diff/320001/chrome/browser/passwor... chrome/browser/password_manager/update_password_infobar_delegate.h:56: bool Cancel() override; On 2016/02/03 00:56:27, Peter Kasting wrote: > Nit: Do these really need to be public? Make them private if possible. Done. https://codereview.chromium.org/1490193003/diff/320001/chrome/browser/passwor... chrome/browser/password_manager/update_password_infobar_delegate.h:65: // multiple choice available. On 2016/02/03 00:56:27, Peter Kasting wrote: > This comment makes no sense to me. > > I don't think we should have this helper anyway; it's very short and called only > once, and its contents would be better just inlined into the lone caller. > > In fact, I would probably inline the two methods below as well, since they share > common prologue code (get the form manager) and epilogue (call Update() on it) > and thus that could be common-factored out of the inlined implementations. Done. https://codereview.chromium.org/1490193003/diff/320001/chrome/browser/passwor... chrome/browser/password_manager/update_password_infobar_delegate.h:71: // Updates the form with index |form_index| in case of multiple credentials. On 2016/02/03 00:56:27, Peter Kasting wrote: > Nit: in -> in the Done. https://codereview.chromium.org/1490193003/diff/320001/chrome/browser/ui/andr... File chrome/browser/ui/android/infobars/update_password_infobar.cc (right): https://codereview.chromium.org/1490193003/diff/320001/chrome/browser/ui/andr... chrome/browser/ui/android/infobars/update_password_infobar.cc:22: UpdatePasswordInfoBar::CreateRenderInfoBar(JNIEnv* env) { On 2016/02/03 00:56:27, Peter Kasting wrote: > Nit: Function definition order should match declaration order Done. https://codereview.chromium.org/1490193003/diff/320001/chrome/browser/ui/andr... chrome/browser/ui/android/infobars/update_password_infobar.cc:36: for (auto password_form : update_password_delegate->GetCurrentForms()) { On 2016/02/03 00:56:27, Peter Kasting wrote: > Nit: No {} Done. https://codereview.chromium.org/1490193003/diff/320001/chrome/browser/ui/andr... chrome/browser/ui/android/infobars/update_password_infobar.cc:44: base::android::ScopedJavaLocalRef<jobjectArray> java_usernames = On 2016/02/03 00:56:27, Peter Kasting wrote: > Nit: No need to qualify these Done. https://codereview.chromium.org/1490193003/diff/320001/chrome/browser/ui/andr... chrome/browser/ui/android/infobars/update_password_infobar.cc:71: scoped_ptr<infobars::InfoBar> CreateUpdatePasswordInfoBar( On 2016/02/03 00:56:27, Peter Kasting wrote: > This function is not declared or referenced anywhere. I'd remove it. Done. https://codereview.chromium.org/1490193003/diff/320001/chrome/browser/ui/andr... File chrome/browser/ui/android/infobars/update_password_infobar.h (right): https://codereview.chromium.org/1490193003/diff/320001/chrome/browser/ui/andr... chrome/browser/ui/android/infobars/update_password_infobar.h:19: On 2016/02/03 00:56:27, Peter Kasting wrote: > Nit: Unnecessary blank line Done. https://codereview.chromium.org/1490193003/diff/320001/chrome/browser/ui/andr... chrome/browser/ui/android/infobars/update_password_infobar.h:24: static bool Register(JNIEnv* env); On 2016/02/03 00:56:27, Peter Kasting wrote: > Nit: Prefer to place static functions above non-static > non-constructor/destructors Done. https://codereview.chromium.org/1490193003/diff/320001/chrome/browser/ui/andr... chrome/browser/ui/android/infobars/update_password_infobar.h:32: On 2016/02/03 00:56:27, Peter Kasting wrote: > Nit: Unnecessary blank line Done.
https://codereview.chromium.org/1490193003/diff/320001/chrome/browser/passwor... File chrome/browser/password_manager/update_password_infobar_delegate.cc (right): https://codereview.chromium.org/1490193003/diff/320001/chrome/browser/passwor... chrome/browser/password_manager/update_password_infobar_delegate.cc:65: return GetCurrentForms().size() > 1 && !is_password_overriden; On 2016/02/12 19:19:36, melandory wrote: > On 2016/02/03 00:56:26, Peter Kasting wrote: > > Nit: Shorter: > > > > return GetCurrentForms().size() > 1 && > > (!form_manager || !form_manager->password_overridden()); > > I actually prefer current version, I think it's easier to read. At least change the line above to: const bool is_password_overriden = form_manager && form_manager->password_overridden(); ...as the ?: form is needlessly complex. https://codereview.chromium.org/1490193003/diff/320001/chrome/browser/passwor... File chrome/browser/password_manager/update_password_infobar_delegate.h (right): https://codereview.chromium.org/1490193003/diff/320001/chrome/browser/passwor... chrome/browser/password_manager/update_password_infobar_delegate.h:41: // credentials save for the web site. On 2016/02/12 19:19:37, melandory wrote: > On 2016/02/03 00:56:27, Peter Kasting wrote: > > Nit: Maybe "Returns true if an infobar should be shown when there are multiple > > matching credentials, e.g. when a user uses a password change form on a site > > with several saved credentials." > > > > Although that assumes I've understood the intent of this function correctly. > > Maybe it's not actually "given that there are multiple accounts, return > whether > > to show the infobar" and more "given that we're showing an infobar, return > > whether it will talk about multiple accounts". > > We show infobar even in case of one credentials. This helper functions tell us > that we should show special case of update infobar -> update infobar for case, > when user has multiple credentials stored This comment doesn't communicate that clearly at all. Maybe "Returns whether the infobar should show the credentials from multiple accounts [where?], e.g. when the user is trying to change the password on a site with multiple saved credentials." https://codereview.chromium.org/1490193003/diff/340001/chrome/android/java/st... File chrome/android/java/strings/android_chrome_strings.grd (right): https://codereview.chromium.org/1490193003/diff/340001/chrome/android/java/st... chrome/android/java/strings/android_chrome_strings.grd:2339: <message name="IDS_UPDATE_PASSWORD" desc="The title of the update password infobar."> Neither of these strings is a "title", they're messages shown on the infobar. A better description would be something like "A message shown to users to allow updating a saved password for a site, shown in an infobar that appears after the user uses a different password to sign in to the site." The description for the item below should be similar but distinguish why we need to show the user the relevant username. https://codereview.chromium.org/1490193003/diff/340001/chrome/android/java/st... chrome/android/java/strings/android_chrome_strings.grd:2343: Do you want <ph name="PASSWORD_MANAGER_BRAND">^1<ex>Google Chrome</ex></ph> to update your password of <ph name="USERNAME">^2<ex>don.john.lemon@example.com</ex></ph> for this site? This sentence doesn't read very grammatically. Did a tech writer write this? I would recommend replacing "your password of" with "the password for". https://codereview.chromium.org/1490193003/diff/340001/chrome/app/generated_r... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/1490193003/diff/340001/chrome/app/generated_r... chrome/app/generated_resources.grd:7286: + <message name="IDS_PASSWORD_MANAGER_CANCEL_BUTTON" desc="Text for the 'Manage Passwords' bubble's 'Not now' option"> Clarify for the translators what this does, how long the decision (to say "no") stays in effect for, etc. https://codereview.chromium.org/1490193003/diff/340001/chrome/app/generated_r... chrome/app/generated_resources.grd:7287: + Nope If this is intended to be colloquial, you should indicate that to the translators in the description. https://codereview.chromium.org/1490193003/diff/340001/chrome/app/generated_r... chrome/app/generated_resources.grd:7328: + <message name="IDS_PASSWORD_MANAGER_UPDATE_BUTTON" desc="Update button text for password manager"> Again, this description tells me little to nothing about what this actually means/does. https://codereview.chromium.org/1490193003/diff/340001/chrome/browser/passwor... File chrome/browser/password_manager/update_password_infobar_delegate.h (right): https://codereview.chromium.org/1490193003/diff/340001/chrome/browser/passwor... chrome/browser/password_manager/update_password_infobar_delegate.h:24: // which one should be updated. Nit: Grammar. How about: An infobar delegate which asks the user if the password should be updated for a set of saved credentials for a site. If several such sets are present, the user can choose which one to update. PasswordManager displays this infobar when the user signs into the site with a new password for a known username or fills in a password change form. https://codereview.chromium.org/1490193003/diff/340001/chrome/browser/passwor... chrome/browser/password_manager/update_password_infobar_delegate.h:47: base::string16 GetUsernameForOneAccountCase(); Nit: One -> Single https://codereview.chromium.org/1490193003/diff/340001/chrome/browser/ui/andr... File chrome/browser/ui/android/infobars/update_password_infobar.h (right): https://codereview.chromium.org/1490193003/diff/340001/chrome/browser/ui/andr... chrome/browser/ui/android/infobars/update_password_infobar.h:10: #include "chrome/browser/password_manager/update_password_infobar_delegate.h" Nit: I think you can forward-declare UpdatePasswordInfoBarDelegate instead of #including this in this header. https://codereview.chromium.org/1490193003/diff/340001/chrome/browser/ui/andr... chrome/browser/ui/android/infobars/update_password_infobar.h:14: // for the site. Nit: How about: The infobar to be used with UpdatePasswordInfoBarDelegate. https://codereview.chromium.org/1490193003/diff/340001/chrome/browser/ui/andr... chrome/browser/ui/android/infobars/update_password_infobar.h:17: static bool Register(JNIEnv* env); Nit: Google style guide says static methods go below constructor/destructor. https://codereview.chromium.org/1490193003/diff/340001/chrome/browser/ui/andr... chrome/browser/ui/android/infobars/update_password_infobar.h:23: int GetIdOfSelectedUsername(); Nit: const?
https://codereview.chromium.org/1490193003/diff/340001/chrome/android/java/st... File chrome/android/java/strings/android_chrome_strings.grd (right): https://codereview.chromium.org/1490193003/diff/340001/chrome/android/java/st... chrome/android/java/strings/android_chrome_strings.grd:2339: <message name="IDS_UPDATE_PASSWORD" desc="The title of the update password infobar."> On 2016/02/13 01:44:52, Peter Kasting wrote: > Neither of these strings is a "title", they're messages shown on the infobar. A > better description would be something like "A message shown to users to allow > updating a saved password for a site, shown in an infobar that appears after the > user uses a different password to sign in to the site." > > The description for the item below should be similar but distinguish why we need > to show the user the relevant username. Done. https://codereview.chromium.org/1490193003/diff/340001/chrome/android/java/st... chrome/android/java/strings/android_chrome_strings.grd:2343: Do you want <ph name="PASSWORD_MANAGER_BRAND">^1<ex>Google Chrome</ex></ph> to update your password of <ph name="USERNAME">^2<ex>don.john.lemon@example.com</ex></ph> for this site? On 2016/02/13 01:44:52, Peter Kasting wrote: > This sentence doesn't read very grammatically. Did a tech writer write this? I > would recommend replacing "your password of" with "the password for". No, it wasn't. It was string form mocks. https://codereview.chromium.org/1490193003/diff/340001/chrome/app/generated_r... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/1490193003/diff/340001/chrome/app/generated_r... chrome/app/generated_resources.grd:7286: + <message name="IDS_PASSWORD_MANAGER_CANCEL_BUTTON" desc="Text for the 'Manage Passwords' bubble's 'Not now' option"> On 2016/02/13 01:44:52, Peter Kasting wrote: > Clarify for the translators what this does, how long the decision (to say "no") > stays in effect for, etc. Done. https://codereview.chromium.org/1490193003/diff/340001/chrome/app/generated_r... chrome/app/generated_resources.grd:7287: + Nope On 2016/02/13 01:44:52, Peter Kasting wrote: > If this is intended to be colloquial, you should indicate that to the > translators in the description. Checked with our PM. It shouldn't be marked as colloquial. https://codereview.chromium.org/1490193003/diff/340001/chrome/app/generated_r... chrome/app/generated_resources.grd:7328: + <message name="IDS_PASSWORD_MANAGER_UPDATE_BUTTON" desc="Update button text for password manager"> On 2016/02/13 01:44:52, Peter Kasting wrote: > Again, this description tells me little to nothing about what this actually > means/does. Done. https://codereview.chromium.org/1490193003/diff/340001/chrome/browser/passwor... File chrome/browser/password_manager/update_password_infobar_delegate.h (right): https://codereview.chromium.org/1490193003/diff/340001/chrome/browser/passwor... chrome/browser/password_manager/update_password_infobar_delegate.h:24: // which one should be updated. On 2016/02/13 01:44:52, Peter Kasting wrote: > Nit: Grammar. How about: > > An infobar delegate which asks the user if the password should be updated for a > set of saved credentials for a site. If several such sets are present, the user > can choose which one to update. PasswordManager displays this infobar when the > user signs into the site with a new password for a known username or fills in a > password change form. Done. https://codereview.chromium.org/1490193003/diff/340001/chrome/browser/passwor... chrome/browser/password_manager/update_password_infobar_delegate.h:47: base::string16 GetUsernameForOneAccountCase(); On 2016/02/13 01:44:52, Peter Kasting wrote: > Nit: One -> Single Done. https://codereview.chromium.org/1490193003/diff/340001/chrome/browser/ui/andr... File chrome/browser/ui/android/infobars/update_password_infobar.h (right): https://codereview.chromium.org/1490193003/diff/340001/chrome/browser/ui/andr... chrome/browser/ui/android/infobars/update_password_infobar.h:10: #include "chrome/browser/password_manager/update_password_infobar_delegate.h" On 2016/02/13 01:44:52, Peter Kasting wrote: > Nit: I think you can forward-declare UpdatePasswordInfoBarDelegate instead of > #including this in this header. Done. https://codereview.chromium.org/1490193003/diff/340001/chrome/browser/ui/andr... chrome/browser/ui/android/infobars/update_password_infobar.h:14: // for the site. On 2016/02/13 01:44:52, Peter Kasting wrote: > Nit: How about: > > The infobar to be used with UpdatePasswordInfoBarDelegate. Done. https://codereview.chromium.org/1490193003/diff/340001/chrome/browser/ui/andr... chrome/browser/ui/android/infobars/update_password_infobar.h:17: static bool Register(JNIEnv* env); On 2016/02/13 01:44:52, Peter Kasting wrote: > Nit: Google style guide says static methods go below constructor/destructor. Done. https://codereview.chromium.org/1490193003/diff/340001/chrome/browser/ui/andr... chrome/browser/ui/android/infobars/update_password_infobar.h:23: int GetIdOfSelectedUsername(); On 2016/02/13 01:44:52, Peter Kasting wrote: > Nit: const? Done.
https://codereview.chromium.org/1490193003/diff/360001/chrome/android/java/st... File chrome/android/java/strings/android_chrome_strings.grd (right): https://codereview.chromium.org/1490193003/diff/360001/chrome/android/java/st... chrome/android/java/strings/android_chrome_strings.grd:2367: <message name="IDS_UPDATE_PASSWORD" desc="A message shown to users to allow updating a saved password for a site, shown in an infobar that appears after the user uses a different password to sign in to the site or uses change password form."> Nit: change password -> a password change (2 places) https://codereview.chromium.org/1490193003/diff/360001/chrome/app/generated_r... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/1490193003/diff/360001/chrome/app/generated_r... chrome/app/generated_resources.grd:7294: + <message name="IDS_PASSWORD_MANAGER_CANCEL_BUTTON" desc="Text for the 'Manage Passwords' bubble's and Update infobar's 'Not now' option. This button allows user to dismiss the shown UI which asks to perform some action (update, delete) on saved credentials without performing this action."> Nit: Awkward; how about "Label for the 'dismiss' button in the Manage Password bubble/Update Password infobar. These UIs ask the user if they wish to perform some action with saved passwords, e.g. updating or deleting them; the button dismisses the UI without taking the suggested action." https://codereview.chromium.org/1490193003/diff/360001/chrome/app/generated_r... chrome/app/generated_resources.grd:7295: + Nope So, if this is not supposed to be colloquial in the translations, why is it colloquial here? https://codereview.chromium.org/1490193003/diff/360001/chrome/app/generated_r... chrome/app/generated_resources.grd:7336: + <message name="IDS_PASSWORD_MANAGER_UPDATE_BUTTON" desc="Update button text for password manager. This buttons is used for the Update Password infobar, when user agrees to update early saved credentials with new password."> Nit: I can't really parse this. How about "Label for the 'update' button in the Update Password infobar. This infobar asks if the user wishes to update the saved password for a site to a new password the user has just entered; the button applies the suggested update." https://codereview.chromium.org/1490193003/diff/360001/chrome/browser/passwor... File chrome/browser/password_manager/update_password_infobar_delegate.cc (right): https://codereview.chromium.org/1490193003/diff/360001/chrome/browser/passwor... chrome/browser/password_manager/update_password_infobar_delegate.cc:31: password_bubble_experiment::IsSmartLockBrandingEnabled(sync_service); Nit: Could save a couple lines by just inlining: const bool is_smartlock_branding_enabled = password_bubble_experiment::IsSmartLockBrandingEnabled( ProfileSyncServiceFactory::GetForProfile( Profile::FromBrowserContext(web_contents->GetBrowserContext()))); This also makes the temp const. https://codereview.chromium.org/1490193003/diff/360001/chrome/browser/passwor... chrome/browser/password_manager/update_password_infobar_delegate.cc:45: form_manager ? form_manager->password_overridden() : false; Simpler: const bool is_password_overriden = form_manager && form_manager->password_overridden(); But even simpler would be to inline below: return (GetCurrentForms().size() > 1) && (!form_manager || !form_manager->password_overridden()); https://codereview.chromium.org/1490193003/diff/360001/chrome/browser/passwor... chrome/browser/password_manager/update_password_infobar_delegate.cc:58: return form_manager->pending_credentials().username_value; Nit: Just inline: return passwords_data_.form_manager()->pending_credentials().username_value; At that point, this function can be renamed get_username_for_single_account() and inlined into the header. https://codereview.chromium.org/1490193003/diff/360001/chrome/browser/passwor... chrome/browser/password_manager/update_password_infobar_delegate.cc:65: : PasswordManagerInfoBarDelegate(), Nit: Explicit call to parent null constructor not necessary https://codereview.chromium.org/1490193003/diff/360001/chrome/browser/passwor... chrome/browser/password_manager/update_password_infobar_delegate.cc:67: // TODO(crbug.com/577129): Add histograms. Nit: TODO form is to list a username in the parens, not a bug number. You can give a bug number after the colon. https://codereview.chromium.org/1490193003/diff/360001/chrome/browser/passwor... chrome/browser/password_manager/update_password_infobar_delegate.cc:71: branding_ = l10n_util::GetStringUTF16( Why do we need a member for this? Why not change branding() to return the result of this call, rename it GetBranding() or GetBrandName() or something and move it to the .cc file, and remove |branding_|? https://codereview.chromium.org/1490193003/diff/360001/chrome/browser/passwor... File chrome/browser/password_manager/update_password_infobar_delegate.h (right): https://codereview.chromium.org/1490193003/diff/360001/chrome/browser/passwor... chrome/browser/password_manager/update_password_infobar_delegate.h:21: // a Fix wrapping, please https://codereview.chromium.org/1490193003/diff/360001/chrome/browser/passwor... chrome/browser/password_manager/update_password_infobar_delegate.h:44: // credentials save for the web site. Nit: Grammar; how about "Returns whether the user has multiple saved credentials, of which the infobar affects just one. In this case the infobar should clarify which credential is being affected." https://codereview.chromium.org/1490193003/diff/360001/chrome/browser/passwor... chrome/browser/password_manager/update_password_infobar_delegate.h:47: const std::vector<const autofill::PasswordForm*>& GetCurrentForms() const; Nit: This return type is super ugly. Consider adding a type alias inside ManagePasswordsState. https://codereview.chromium.org/1490193003/diff/360001/chrome/browser/passwor... chrome/browser/password_manager/update_password_infobar_delegate.h:65: ManagePasswordsState passwords_data_; Nit: Name this |passwords_state_| for parallel with the type name.
https://codereview.chromium.org/1490193003/diff/360001/chrome/android/java/st... File chrome/android/java/strings/android_chrome_strings.grd (right): https://codereview.chromium.org/1490193003/diff/360001/chrome/android/java/st... chrome/android/java/strings/android_chrome_strings.grd:2367: <message name="IDS_UPDATE_PASSWORD" desc="A message shown to users to allow updating a saved password for a site, shown in an infobar that appears after the user uses a different password to sign in to the site or uses change password form."> On 2016/02/22 22:38:12, Peter Kasting wrote: > Nit: change password -> a password change (2 places) Done. https://codereview.chromium.org/1490193003/diff/360001/chrome/app/generated_r... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/1490193003/diff/360001/chrome/app/generated_r... chrome/app/generated_resources.grd:7294: + <message name="IDS_PASSWORD_MANAGER_CANCEL_BUTTON" desc="Text for the 'Manage Passwords' bubble's and Update infobar's 'Not now' option. This button allows user to dismiss the shown UI which asks to perform some action (update, delete) on saved credentials without performing this action."> On 2016/02/22 22:38:12, Peter Kasting wrote: > Nit: Awkward; how about "Label for the 'dismiss' button in the Manage Password > bubble/Update Password infobar. These UIs ask the user if they wish to perform > some action with saved passwords, e.g. updating or deleting them; the button > dismisses the UI without taking the suggested action." Done. https://codereview.chromium.org/1490193003/diff/360001/chrome/app/generated_r... chrome/app/generated_resources.grd:7295: + Nope On 2016/02/22 22:38:12, Peter Kasting wrote: > So, if this is not supposed to be colloquial in the translations, why is it > colloquial here? +sabineb for this discussion https://codereview.chromium.org/1490193003/diff/360001/chrome/app/generated_r... chrome/app/generated_resources.grd:7336: + <message name="IDS_PASSWORD_MANAGER_UPDATE_BUTTON" desc="Update button text for password manager. This buttons is used for the Update Password infobar, when user agrees to update early saved credentials with new password."> On 2016/02/22 22:38:12, Peter Kasting wrote: > Nit: I can't really parse this. How about "Label for the 'update' button in the > Update Password infobar. This infobar asks if the user wishes to update the > saved password for a site to a new password the user has just entered; the > button applies the suggested update." Done. https://codereview.chromium.org/1490193003/diff/360001/chrome/browser/passwor... File chrome/browser/password_manager/update_password_infobar_delegate.cc (right): https://codereview.chromium.org/1490193003/diff/360001/chrome/browser/passwor... chrome/browser/password_manager/update_password_infobar_delegate.cc:31: password_bubble_experiment::IsSmartLockBrandingEnabled(sync_service); On 2016/02/22 22:38:13, Peter Kasting wrote: > Nit: Could save a couple lines by just inlining: > > const bool is_smartlock_branding_enabled = > password_bubble_experiment::IsSmartLockBrandingEnabled( > ProfileSyncServiceFactory::GetForProfile( > Profile::FromBrowserContext(web_contents->GetBrowserContext()))); > > This also makes the temp const. Done. https://codereview.chromium.org/1490193003/diff/360001/chrome/browser/passwor... chrome/browser/password_manager/update_password_infobar_delegate.cc:45: form_manager ? form_manager->password_overridden() : false; On 2016/02/22 22:38:13, Peter Kasting wrote: > Simpler: > > const bool is_password_overriden = > form_manager && form_manager->password_overridden(); > > But even simpler would be to inline below: > > return (GetCurrentForms().size() > 1) && > (!form_manager || !form_manager->password_overridden()); Done. https://codereview.chromium.org/1490193003/diff/360001/chrome/browser/passwor... chrome/browser/password_manager/update_password_infobar_delegate.cc:58: return form_manager->pending_credentials().username_value; On 2016/02/22 22:38:13, Peter Kasting wrote: > Nit: Just inline: > > return passwords_data_.form_manager()->pending_credentials().username_value; > > At that point, this function can be renamed get_username_for_single_account() > and inlined into the header. Done. https://codereview.chromium.org/1490193003/diff/360001/chrome/browser/passwor... chrome/browser/password_manager/update_password_infobar_delegate.cc:65: : PasswordManagerInfoBarDelegate(), On 2016/02/22 22:38:13, Peter Kasting wrote: > Nit: Explicit call to parent null constructor not necessary Done. https://codereview.chromium.org/1490193003/diff/360001/chrome/browser/passwor... chrome/browser/password_manager/update_password_infobar_delegate.cc:67: // TODO(crbug.com/577129): Add histograms. On 2016/02/22 22:38:13, Peter Kasting wrote: > Nit: TODO form is to list a username in the parens, not a bug number. You can > give a bug number after the colon. Done. https://codereview.chromium.org/1490193003/diff/360001/chrome/browser/passwor... chrome/browser/password_manager/update_password_infobar_delegate.cc:71: branding_ = l10n_util::GetStringUTF16( On 2016/02/22 22:38:12, Peter Kasting wrote: > Why do we need a member for this? Why not change branding() to return the > result of this call, rename it GetBranding() or GetBrandName() or something and > move it to the .cc file, and remove |branding_|? Done. https://codereview.chromium.org/1490193003/diff/360001/chrome/browser/passwor... File chrome/browser/password_manager/update_password_infobar_delegate.h (right): https://codereview.chromium.org/1490193003/diff/360001/chrome/browser/passwor... chrome/browser/password_manager/update_password_infobar_delegate.h:21: // a On 2016/02/22 22:38:13, Peter Kasting wrote: > Fix wrapping, please Sorry, cl format sometimes does this. https://codereview.chromium.org/1490193003/diff/360001/chrome/browser/passwor... chrome/browser/password_manager/update_password_infobar_delegate.h:44: // credentials save for the web site. On 2016/02/22 22:38:13, Peter Kasting wrote: > Nit: Grammar; how about "Returns whether the user has multiple saved > credentials, of which the infobar affects just one. In this case the infobar > should clarify which credential is being affected." Done. https://codereview.chromium.org/1490193003/diff/360001/chrome/browser/passwor... chrome/browser/password_manager/update_password_infobar_delegate.h:47: const std::vector<const autofill::PasswordForm*>& GetCurrentForms() const; On 2016/02/22 22:38:13, Peter Kasting wrote: > Nit: This return type is super ugly. Consider adding a type alias inside > ManagePasswordsState. Will address in separate CL. https://codereview.chromium.org/1490193003/diff/360001/chrome/browser/passwor... chrome/browser/password_manager/update_password_infobar_delegate.h:65: ManagePasswordsState passwords_data_; On 2016/02/22 22:38:13, Peter Kasting wrote: > Nit: Name this |passwords_state_| for parallel with the type name. Done.
LGTM other than that question about colloquial language
The CQ bit was checked by melandory@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from vabr@chromium.org, dfalcantara@chromium.org Link to the patchset: https://codereview.chromium.org/1490193003/#ps400001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1490193003/400001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1490193003/400001
Message was sent while issue was closed.
Description was changed from ========== [Password Manager] Update Confirmation UI for for Chrome on Android. This CL contains implementation of the Update password infobar: 1.For confirmation from the user that he/she changed password in case when the user has only one credentials for current site. In case saved credentials have a username we present the user with that username in the infobar message and ask is the user what to update password which is corresponding to this username. In case there is no username saved, we're asking user if saved password for this site should be updated or not. 2.For selecting by user which credentials should be updated after change password form submission in case when the user has more than one credentials and we have no clue which credentials is correct. It contains a selector with a list of all credentials for current site. Mocks: https://folio.googleplex.com/chrome-ux/mocks/321-password-manager/password%20... BUG=564674 ========== to ========== [Password Manager] Update Confirmation UI for for Chrome on Android. This CL contains implementation of the Update password infobar: 1.For confirmation from the user that he/she changed password in case when the user has only one credentials for current site. In case saved credentials have a username we present the user with that username in the infobar message and ask is the user what to update password which is corresponding to this username. In case there is no username saved, we're asking user if saved password for this site should be updated or not. 2.For selecting by user which credentials should be updated after change password form submission in case when the user has more than one credentials and we have no clue which credentials is correct. It contains a selector with a list of all credentials for current site. Mocks: https://folio.googleplex.com/chrome-ux/mocks/321-password-manager/password%20... BUG=564674 ==========
Message was sent while issue was closed.
Committed patchset #12 (id:400001)
Message was sent while issue was closed.
Description was changed from ========== [Password Manager] Update Confirmation UI for for Chrome on Android. This CL contains implementation of the Update password infobar: 1.For confirmation from the user that he/she changed password in case when the user has only one credentials for current site. In case saved credentials have a username we present the user with that username in the infobar message and ask is the user what to update password which is corresponding to this username. In case there is no username saved, we're asking user if saved password for this site should be updated or not. 2.For selecting by user which credentials should be updated after change password form submission in case when the user has more than one credentials and we have no clue which credentials is correct. It contains a selector with a list of all credentials for current site. Mocks: https://folio.googleplex.com/chrome-ux/mocks/321-password-manager/password%20... BUG=564674 ========== to ========== [Password Manager] Update Confirmation UI for for Chrome on Android. This CL contains implementation of the Update password infobar: 1.For confirmation from the user that he/she changed password in case when the user has only one credentials for current site. In case saved credentials have a username we present the user with that username in the infobar message and ask is the user what to update password which is corresponding to this username. In case there is no username saved, we're asking user if saved password for this site should be updated or not. 2.For selecting by user which credentials should be updated after change password form submission in case when the user has more than one credentials and we have no clue which credentials is correct. It contains a selector with a list of all credentials for current site. Mocks: https://folio.googleplex.com/chrome-ux/mocks/321-password-manager/password%20... BUG=564674 Committed: https://crrev.com/f5bf9e8f3672962246c4efa694f0496cf87bbb1d Cr-Commit-Position: refs/heads/master@{#377260} ==========
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/f5bf9e8f3672962246c4efa694f0496cf87bbb1d Cr-Commit-Position: refs/heads/master@{#377260} |
