Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
861103002:
Credentials chooser UI for Android. (Closed)
|
Created:
5 years, 11 months ago by melandory Modified:
5 years, 10 months ago CC:
chromium-reviews, gcasto+watchlist_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. |
DescriptionCredentials chooser.
Infobar which allows user to choose Credentials.
Infobar is triggered via Credentials Manager API.
R=vabr@chromium.org,brettw@chromium.org,newt@chromium.org
BUG=454815
Committed: https://crrev.com/ca57a51bbb794575650b9e1b071b68aeae3d5e87
Cr-Commit-Position: refs/heads/master@{#317368}
Patch Set 1 : Work in progres. Proper view for infobar content. #Patch Set 2 : Work in progress. #Patch Set 3 : Work in progress. Click with log print. #Patch Set 4 : Work in progress. Pass credentials to c++ code (using OnCredentialClick) in infobar. #Patch Set 5 : Work in progress. Make credentials in credential picker scrollable. #Patch Set 6 : Work in progress. Perferct height of credential picker. #Patch Set 7 : Work in progress. Adding Menu button (links in menu are inactive yet). #Patch Set 8 : Changes to InfoBar (proposed by dfalcantara ) which aloows ScrollView to be same width as Infobar. #Patch Set 9 : Work in progress. Removing icon as was requested by sabineb. #Patch Set 10 : Work in progress. Pass credential back to cpp in proper way. #Patch Set 11 : Work in progress. Infobar title from mocks. #Patch Set 12 : Work in progress. Code with infoBar changes. #Patch Set 13 : Merge AccountChooserInfobar and AccountChooserInfobarDelegate #Patch Set 14 : Rebaised on top of master. #
Total comments: 26
Patch Set 15 : Rebased on top of master #
Total comments: 41
Patch Set 16 : #
Total comments: 26
Patch Set 17 : Review, please #
Total comments: 33
Patch Set 18 : #
Total comments: 40
Patch Set 19 : #
Total comments: 5
Patch Set 20 : Please review. #
Total comments: 20
Patch Set 21 : Please review. #
Total comments: 4
Patch Set 22 : #Patch Set 23 : #Patch Set 24 : Fixing tests. #
Total comments: 1
Patch Set 25 : Comment. #Messages
Total messages: 87 (43 generated)
Patchset #1 (id:1) has been deleted
rouslan@chromium.org changed reviewers: + rouslan@chromium.org
https://codereview.chromium.org/861103002/diff/120001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/infobar/AccountChooserInfoBar.java (right): https://codereview.chromium.org/861103002/diff/120001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/AccountChooserInfoBar.java:40: private long mNativeInfoBar; Should stay in delegate. Only delegate should communicate with C++. Keep a reference to delegate here.
Hi Rouslan, I don't know if you meant to send this for review already or not. It might help if you make it explicit next time you send an e-mail. :) I looked at chrome/browser/password_manager/* and left some comments. Cheers, Vaclav https://codereview.chromium.org/861103002/diff/120001/chrome/browser/password... File chrome/browser/password_manager/account_chooser_infobar_delegate_android.cc (right): https://codereview.chromium.org/861103002/diff/120001/chrome/browser/password... chrome/browser/password_manager/account_chooser_infobar_delegate_android.cc:1: // Copyright 2014 The Chromium Authors. All rights reserved. 2015 https://codereview.chromium.org/861103002/diff/120001/chrome/browser/password... chrome/browser/password_manager/account_chooser_infobar_delegate_android.cc:35: LOG(ERROR) << "Local form"; Consider using VLOG(1) instead. It allows filtering the log output by source file. Also elsewhere in this CL. https://codereview.chromium.org/861103002/diff/120001/chrome/browser/password... chrome/browser/password_manager/account_chooser_infobar_delegate_android.cc:35: LOG(ERROR) << "Local form"; The logs here look like private logging, not something you want to commit. If you do want to commit them, please make the messages more precise. https://codereview.chromium.org/861103002/diff/120001/chrome/browser/password... File chrome/browser/password_manager/account_chooser_infobar_delegate_android.h (right): https://codereview.chromium.org/861103002/diff/120001/chrome/browser/password... chrome/browser/password_manager/account_chooser_infobar_delegate_android.h:1: // Copyright 2014 The Chromium Authors. All rights reserved. nit: 2015 https://codereview.chromium.org/861103002/diff/120001/chrome/browser/password... chrome/browser/password_manager/account_chooser_infobar_delegate_android.h:29: ~AccountChooserInfoBarDelegateAndroid() = default; Should this also bear the "override" keyword, or is it implied? Before C++11 I would write this as ~AccountChooserInfoBarDelegateAndroid() override; // in header AccountChooserInfoBarDelegateAndroid::~AccountChooserInfoBarDelegateAndroid() {} // in cc but I'm not sure how this works with = default. https://codereview.chromium.org/861103002/diff/120001/chrome/browser/password... chrome/browser/password_manager/account_chooser_infobar_delegate_android.h:34: ScopedVector<autofill::PasswordForm>& local_credentials_forms() const { This should not be "const", because it allows modification of a member of a member of the delegate. Despite it compiles, this sounds logically wrong. https://codereview.chromium.org/861103002/diff/120001/chrome/browser/password... chrome/browser/password_manager/account_chooser_infobar_delegate_android.h:60: // bool RegisterAccountChooserInfoBarDelegate(JNIEnv* env); Looks like this should be removed?
Sorry, I did not mean to trigger a review. My comment was an attempt to resolve Tanja's issue with JNI.
Sorry, everyone! My bad, I should have looked at who the author of the CL is, instead of assuming the author always posts first. :) Just ignore me, I'll wait until Tanja asks me for review. Have a good day, all of you, Vaclav
melandory@chromium.org changed reviewers: - miguelg@chromium.org
melandory@chromium.org changed reviewers: - rouslan@chromium.org, vabr@chromium.org
melandory@chromium.org changed reviewers: + ben@chromium.org, newt@chromium.org, vabr@chromium.org
Hi all, review this CL please. Files are distributed among reviewers in a followings way (but fill free to review everything ;)): vabr@ chrome/browser/ui/passwords/manage_passwords_ui_controller.cc chrome/browser/password_manager/account_chooser_infobar_delegate_android.h chrome/browser/password_manager/account_chooser_infobar_delegate_android.cc chrome/browser/password_manager/chrome_password_manager_client.cc newt@ chrome/android/java/res/layout/credential_picker_buttonbar.xml chrome/android/java/res/layout/credential_picker_infobar_item.xml chrome/android/java/res/layout/credential_picker_infobar_items.xml chrome/android/java/res/menu/credential_picker_infobar_more_menu_popup.xml chrome/android/java/src/org/chromium/chrome/browser/infobar/AccountChooserInfoBar.java chrome/android/java/strings/android_chrome_strings.grd chrome/browser/android/chrome_jni_registrar.cc ben@ chrome/browser/ui/android/infobars/account_chooser_infobar.h chrome/browser/ui/android/infobars/account_chooser_infobar.cc chrome/browser/ui/tab_helpers.cc chrome/chrome_browser.gypi chrome/chrome_browser_ui.gypi components/infobars/core/infobar_delegate.h components/infobars/core/infobar_delegate.cc https://codereview.chromium.org/861103002/diff/120001/chrome/browser/password... File chrome/browser/password_manager/account_chooser_infobar_delegate_android.h (right): https://codereview.chromium.org/861103002/diff/120001/chrome/browser/password... chrome/browser/password_manager/account_chooser_infobar_delegate_android.h:29: ~AccountChooserInfoBarDelegateAndroid() = default; On 2015/01/29 17:38:09, vabr (Chromium) wrote: > Should this also bear the "override" keyword, or is it implied? I'm not sure that I understood your point, sorry, so I'll just write some random thoughts here. "override" should be added here if it should be added to dtors in general (but I can't find this requirement in style guide). For dtors I see only one point to use "override": if one forgot to make dtor in base class virtual and then usage of override in derived class will make compiler unhappy. =default just allows me not to write default dtor which looks cleaner to me.
https://codereview.chromium.org/861103002/diff/120001/chrome/browser/password... File chrome/browser/password_manager/account_chooser_infobar_delegate_android.h (right): https://codereview.chromium.org/861103002/diff/120001/chrome/browser/password... chrome/browser/password_manager/account_chooser_infobar_delegate_android.h:29: ~AccountChooserInfoBarDelegateAndroid() = default; On 2015/01/29 17:38:09, vabr (Chromium) wrote: > Should this also bear the "override" keyword, or is it implied? > Before C++11 I would write this as > ~AccountChooserInfoBarDelegateAndroid() override; // in header > AccountChooserInfoBarDelegateAndroid::~AccountChooserInfoBarDelegateAndroid() {} > // in cc > > but I'm not sure how this works with = default. Oh, somehow manage to publish this comment without updating it: I've already found info about override and dtors in style guide http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Inheritance
Patchset #19 (id:380001) has been deleted
Patchset #18 (id:360001) has been deleted
Hi melandory@, This CL does not seem to change chrome/browser/password_manager/chrome_password_manager_client.cc, so I did not review that. I had a look at chrome/browser/ui/passwords/manage_passwords_ui_controller.cc chrome/browser/password_manager/account_chooser_infobar_delegate_android.h chrome/browser/password_manager/account_chooser_infobar_delegate_android.cc and left some comments. Cheers, Vaclav https://codereview.chromium.org/861103002/diff/420001/chrome/browser/password... File chrome/browser/password_manager/account_chooser_infobar_delegate_android.cc (right): https://codereview.chromium.org/861103002/diff/420001/chrome/browser/password... chrome/browser/password_manager/account_chooser_infobar_delegate_android.cc:1: // Copyright 2014 The Chromium Authors. All rights reserved. 2015 https://codereview.chromium.org/861103002/diff/420001/chrome/browser/password... chrome/browser/password_manager/account_chooser_infobar_delegate_android.cc:7: #include <cstddef> Please go through the #includes and check which of them are unused. https://codereview.chromium.org/861103002/diff/420001/chrome/browser/password... chrome/browser/password_manager/account_chooser_infobar_delegate_android.cc:24: autofill::PasswordForm* form = new autofill::PasswordForm(); Please use scoped_vector to hold the allocated form. https://codereview.chromium.org/861103002/diff/420001/chrome/browser/password... chrome/browser/password_manager/account_chooser_infobar_delegate_android.cc:28: form = ui_controller_->local_credentials_forms()[credential_index]; Is this a memory leak? (Also line 34.) https://codereview.chromium.org/861103002/diff/420001/chrome/browser/password... chrome/browser/password_manager/account_chooser_infobar_delegate_android.cc:38: ui_controller_->ChooseCredential(*form, credential_type); Is this a memory leak? https://codereview.chromium.org/861103002/diff/420001/chrome/browser/password... File chrome/browser/password_manager/account_chooser_infobar_delegate_android.h (right): https://codereview.chromium.org/861103002/diff/420001/chrome/browser/password... chrome/browser/password_manager/account_chooser_infobar_delegate_android.h:9: #include "base/strings/string16.h" Seems unused. https://codereview.chromium.org/861103002/diff/420001/chrome/browser/password... chrome/browser/password_manager/account_chooser_infobar_delegate_android.h:12: #include "ui/gfx/range/range.h" Also seems unused. https://codereview.chromium.org/861103002/diff/420001/chrome/browser/password... chrome/browser/password_manager/account_chooser_infobar_delegate_android.h:31: ScopedVector<autofill::PasswordForm>& local_credentials_forms() { Please #include base/memory/scoped_vector.h and at least forward declare namespace autofill { struct PasswordForm; } // namespace autofill https://codereview.chromium.org/861103002/diff/420001/chrome/browser/password... chrome/browser/password_manager/account_chooser_infobar_delegate_android.h:31: ScopedVector<autofill::PasswordForm>& local_credentials_forms() { Please make this a const method returning a const reference. The only place where you use it you don't modify the vector. So declaring const will make the code clearer for the human reader, as well as more open to optimisations for the compiler. https://codereview.chromium.org/861103002/diff/420001/chrome/browser/password... chrome/browser/password_manager/account_chooser_infobar_delegate_android.h:43: virtual Type GetInfoBarType() const override; Please drop "virtual". See: "For clarity, use exactly one of override, final, or virtual when declaring an override." in http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Inheritance https://codereview.chromium.org/861103002/diff/420001/chrome/browser/password... chrome/browser/password_manager/account_chooser_infobar_delegate_android.h:47: // TODO(melandory): It's look like that controller part can be separated from Grammar + shortening: It should be possible to separate the controller part from ManagePasswordsUIController, because it does some desktop UI work. Having said that, I'm still not sure I understand the comment. :) I have troubles finding out what "the controller part" is (especially if you mean "the controller part of a controller"), and the "because it does some desktop UI" reason is also cryptic to me. https://codereview.chromium.org/861103002/diff/420001/chrome/browser/password... chrome/browser/password_manager/account_chooser_infobar_delegate_android.h:50: ManagePasswordsUIController* ui_controller_; Please note whether this is a weak pointer, or whether the delegate owns the controller. https://codereview.chromium.org/861103002/diff/420001/chrome/browser/ui/passw... File chrome/browser/ui/passwords/manage_passwords_ui_controller.cc (right): https://codereview.chromium.org/861103002/diff/420001/chrome/browser/ui/passw... chrome/browser/ui/passwords/manage_passwords_ui_controller.cc:138: bubble_shown_ = true; This seems ad-hoc. It makes the comment at the declaration of bubble_shown_ no longer true. There already is a !ANDROID #ifdef in UpdateBubbleAndIconVisibility(), should this code be moved to an #else branch of that one?
melandory@chromium.org changed reviewers: - ben@chromium.org
melandory@chromium.org changed reviewers: + brettw@chromium.org
This way I probably have more chances for review: +brettw@ -ben@ chrome/browser/ui/android/infobars/account_chooser_infobar.h chrome/browser/ui/android/infobars/account_chooser_infobar.cc chrome/browser/ui/tab_helpers.cc chrome/chrome_browser.gypi chrome/chrome_browser_ui.gypi components/infobars/core/infobar_delegate.h components/infobars/core/infobar_delegate.cc https://codereview.chromium.org/861103002/diff/420001/chrome/browser/password... File chrome/browser/password_manager/account_chooser_infobar_delegate_android.cc (right): https://codereview.chromium.org/861103002/diff/420001/chrome/browser/password... chrome/browser/password_manager/account_chooser_infobar_delegate_android.cc:1: // Copyright 2014 The Chromium Authors. All rights reserved. On 2015/02/04 08:43:07, vabr (Chromium) wrote: > 2015 Done. https://codereview.chromium.org/861103002/diff/420001/chrome/browser/password... chrome/browser/password_manager/account_chooser_infobar_delegate_android.cc:7: #include <cstddef> On 2015/02/04 08:43:07, vabr (Chromium) wrote: > Please go through the #includes and check which of them are unused. Done. https://codereview.chromium.org/861103002/diff/420001/chrome/browser/password... chrome/browser/password_manager/account_chooser_infobar_delegate_android.cc:24: autofill::PasswordForm* form = new autofill::PasswordForm(); On 2015/02/04 08:43:07, vabr (Chromium) wrote: > Please use scoped_vector to hold the allocated form. Actually, I do not need to allocate form. Done. https://codereview.chromium.org/861103002/diff/420001/chrome/browser/password... chrome/browser/password_manager/account_chooser_infobar_delegate_android.cc:28: form = ui_controller_->local_credentials_forms()[credential_index]; On 2015/02/04 08:43:07, vabr (Chromium) wrote: > Is this a memory leak? (Also line 34.) Done. https://codereview.chromium.org/861103002/diff/420001/chrome/browser/password... chrome/browser/password_manager/account_chooser_infobar_delegate_android.cc:38: ui_controller_->ChooseCredential(*form, credential_type); On 2015/02/04 08:43:07, vabr (Chromium) wrote: > Is this a memory leak? Done. https://codereview.chromium.org/861103002/diff/420001/chrome/browser/password... File chrome/browser/password_manager/account_chooser_infobar_delegate_android.h (right): https://codereview.chromium.org/861103002/diff/420001/chrome/browser/password... chrome/browser/password_manager/account_chooser_infobar_delegate_android.h:9: #include "base/strings/string16.h" On 2015/02/04 08:43:07, vabr (Chromium) wrote: > Seems unused. Done. https://codereview.chromium.org/861103002/diff/420001/chrome/browser/password... chrome/browser/password_manager/account_chooser_infobar_delegate_android.h:12: #include "ui/gfx/range/range.h" On 2015/02/04 08:43:07, vabr (Chromium) wrote: > Also seems unused. Done. https://codereview.chromium.org/861103002/diff/420001/chrome/browser/password... chrome/browser/password_manager/account_chooser_infobar_delegate_android.h:31: ScopedVector<autofill::PasswordForm>& local_credentials_forms() { On 2015/02/04 08:43:07, vabr (Chromium) wrote: > Please make this a const method returning a const reference. The only place > where you use it you don't modify the vector. So declaring const will make the > code clearer for the human reader, as well as more open to optimisations for the > compiler. Done. https://codereview.chromium.org/861103002/diff/420001/chrome/browser/password... chrome/browser/password_manager/account_chooser_infobar_delegate_android.h:31: ScopedVector<autofill::PasswordForm>& local_credentials_forms() { On 2015/02/04 08:43:07, vabr (Chromium) wrote: > Please #include base/memory/scoped_vector.h and at least forward declare > namespace autofill { > struct PasswordForm; > } // namespace autofill Done. https://codereview.chromium.org/861103002/diff/420001/chrome/browser/password... chrome/browser/password_manager/account_chooser_infobar_delegate_android.h:43: virtual Type GetInfoBarType() const override; On 2015/02/04 08:43:07, vabr (Chromium) wrote: > Please drop "virtual". > > See: "For clarity, use exactly one of override, final, or virtual when declaring > an override." in > http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Inheritance Done. https://codereview.chromium.org/861103002/diff/420001/chrome/browser/password... chrome/browser/password_manager/account_chooser_infobar_delegate_android.h:47: // TODO(melandory): It's look like that controller part can be separated from On 2015/02/04 08:43:08, vabr (Chromium) wrote: > Grammar + shortening: > It should be possible to separate the controller part from > ManagePasswordsUIController, because it does some desktop UI work. > > Having said that, I'm still not sure I understand the comment. :) I have > troubles finding out what "the controller part" is (especially if you mean "the > controller part of a controller"), and the "because it does some desktop UI" > reason is also cryptic to me. Done. https://codereview.chromium.org/861103002/diff/420001/chrome/browser/password... chrome/browser/password_manager/account_chooser_infobar_delegate_android.h:50: ManagePasswordsUIController* ui_controller_; On 2015/02/04 08:43:07, vabr (Chromium) wrote: > Please note whether this is a weak pointer, or whether the delegate owns the > controller. Done. https://codereview.chromium.org/861103002/diff/420001/chrome/browser/ui/passw... File chrome/browser/ui/passwords/manage_passwords_ui_controller.cc (right): https://codereview.chromium.org/861103002/diff/420001/chrome/browser/ui/passw... chrome/browser/ui/passwords/manage_passwords_ui_controller.cc:138: bubble_shown_ = true; On 2015/02/04 08:43:08, vabr (Chromium) wrote: > This seems ad-hoc. It makes the comment at the declaration of bubble_shown_ no > longer true. There already is a !ANDROID #ifdef in > UpdateBubbleAndIconVisibility(), should this code be moved to an #else branch of > that one? Done.
https://codereview.chromium.org/861103002/diff/460001/chrome/android/java/res... File chrome/android/java/res/layout/credential_picker_buttonbar.xml (right): https://codereview.chromium.org/861103002/diff/460001/chrome/android/java/res... chrome/android/java/res/layout/credential_picker_buttonbar.xml:1: <?xml version="1.0" encoding="utf-8"?> Hopefully, you can get rid of this file and use the standard infobar buttons. Why did you need to create this in the first place? https://codereview.chromium.org/861103002/diff/460001/chrome/android/java/res... File chrome/android/java/res/layout/credential_picker_infobar_item.xml (right): https://codereview.chromium.org/861103002/diff/460001/chrome/android/java/res... chrome/android/java/res/layout/credential_picker_infobar_item.xml:8: android:layout_width="fill_parent" use match_parent instead of fill_parent. They're synonyms, but we use match_parent for consistency. https://codereview.chromium.org/861103002/diff/460001/chrome/android/java/res... chrome/android/java/res/layout/credential_picker_infobar_item.xml:11: android:background="?attr/selectableItemBackground" don't need selectableItemBackground once you start using a ListView (as I mentioned in AccountChooseInfobar.java) https://codereview.chromium.org/861103002/diff/460001/chrome/android/java/res... chrome/android/java/res/layout/credential_picker_infobar_item.xml:14: android:id="@+id/credentials_app_image" why "app_image"? Isn't this a user profile image? How about calling this "profile_image"? The IDs below could also be improved. https://codereview.chromium.org/861103002/diff/460001/chrome/android/java/res... chrome/android/java/res/layout/credential_picker_infobar_item.xml:22: android:layout_width="wrap_content" use layout_width="0dp" and layout_weight="1". That'll make this view expand horizontally to take up the available space. https://codereview.chromium.org/861103002/diff/460001/chrome/android/java/res... chrome/android/java/res/layout/credential_picker_infobar_item.xml:26: android:id="@+id/credentials_app_name" attributes should be in this order: - id - layout_width - layout_height - other layout_* attributes - other attributes, typically alphabetized https://codereview.chromium.org/861103002/diff/460001/chrome/android/java/res... chrome/android/java/res/layout/credential_picker_infobar_item.xml:27: android:textAppearance="?android:attr/textAppearanceMedium" textAppearanceMedium and textAppearanceSmall have different colors and sizes on Android L devices compared to pre-L devices. You should probably specify the font color and size explicitly. https://codereview.chromium.org/861103002/diff/460001/chrome/android/java/res... File chrome/android/java/res/layout/credential_picker_infobar_items.xml (right): https://codereview.chromium.org/861103002/diff/460001/chrome/android/java/res... chrome/android/java/res/layout/credential_picker_infobar_items.xml:1: <?xml version="1.0" encoding="utf-8"?> You should be able to get rid of this layout once you start using a ListView and the default infobar buttons. https://codereview.chromium.org/861103002/diff/460001/chrome/android/java/res... File chrome/android/java/res/menu/credential_picker_infobar_more_menu_popup.xml (right): https://codereview.chromium.org/861103002/diff/460001/chrome/android/java/res... chrome/android/java/res/menu/credential_picker_infobar_more_menu_popup.xml:7: xmlns:chrome="http://schemas.android.com/apk/res-auto"> remove xmlns:chrome https://codereview.chromium.org/861103002/diff/460001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/infobar/AccountChooserInfoBar.java (right): https://codereview.chromium.org/861103002/diff/460001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/AccountChooserInfoBar.java:30: private enum CredentialType { enums are discouraged on Android for performance reasons. In this case, I'd just use private static final ints. https://codereview.chromium.org/861103002/diff/460001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/AccountChooserInfoBar.java:54: static InfoBar show(long nativeInfoBar, int enumeratedIconId, String[] usernames) { can you make this private? https://codereview.chromium.org/861103002/diff/460001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/AccountChooserInfoBar.java:63: * @param titleText Message to display in the infobar. titleText and moreButtonText aren't parameters https://codereview.chromium.org/861103002/diff/460001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/AccountChooserInfoBar.java:105: ViewGroup scroll = (ViewGroup) frame.findViewById(R.id.credential_scroll); You should use a ListView instead of a ScrollView containing a LinearLayout. ListView is designed for this type of situation. https://codereview.chromium.org/861103002/diff/460001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/AccountChooserInfoBar.java:120: /** Represents row in account chooser infobar, usual row has avatar, username and full name. nit: newline before "Represents" https://codereview.chromium.org/861103002/diff/460001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/AccountChooserInfoBar.java:130: username_view.setText(username); camelCase for Java variable names https://codereview.chromium.org/861103002/diff/460001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/AccountChooserInfoBar.java:152: private void setupCustomButtonsRow(ViewGroup frame) { I can add support to InfoBarLayout for this button. What exactly do you need that InfoBarLayout doesn't provide currently? https://codereview.chromium.org/861103002/diff/460001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/AccountChooserInfoBar.java:177: private void onChooseCredentials( I'd remove this method and just call nativeOnCredentialClicked() directly in paddCredentialsToNative(). https://codereview.chromium.org/861103002/diff/460001/chrome/android/java/str... File chrome/android/java/strings/android_chrome_strings.grd (right): https://codereview.chromium.org/861103002/diff/460001/chrome/android/java/str... chrome/android/java/strings/android_chrome_strings.grd:485: Settings There's already a string for "Settings" in this file, which you should use https://codereview.chromium.org/861103002/diff/460001/chrome/android/java/str... chrome/android/java/strings/android_chrome_strings.grd:488: Learn more There's actually already a "Learn more" string in android_ui_strings.grd, which you can use.
https://codereview.chromium.org/861103002/diff/460001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/infobar/AccountChooserInfoBar.java (right): https://codereview.chromium.org/861103002/diff/460001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/AccountChooserInfoBar.java:105: ViewGroup scroll = (ViewGroup) frame.findViewById(R.id.credential_scroll); On 2015/02/04 22:17:52, newt wrote: > You should use a ListView instead of a ScrollView containing a LinearLayout. > ListView is designed for this type of situation. You are right that I do not need it here, but I left ScrollView because I will need it in follow up CL: view should be scrollable when we have more than 2 credentials to show. Should I change it or may I leave it? https://codereview.chromium.org/861103002/diff/460001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/AccountChooserInfoBar.java:152: private void setupCustomButtonsRow(ViewGroup frame) { On 2015/02/04 22:17:52, newt wrote: > I can add support to InfoBarLayout for this button. What exactly do you need > that InfoBarLayout doesn't provide currently? 1) Infobar forces specific style of buttons (something like main button is blue and so on), in my case I will potentially have 2 buttons, both white. 2) I'm not completely understand logic behind InfoBarLayout::onClick(View view). If I'll decide to go with it then I need to create another handler (similar to mInfoBarView.onCloseButtonClicked();) and pass View there, because I need it to show popupMenu. It doesn't look very clean to me. Maybe you can point me to different approach?
https://codereview.chromium.org/861103002/diff/460001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/infobar/AccountChooserInfoBar.java (right): https://codereview.chromium.org/861103002/diff/460001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/AccountChooserInfoBar.java:105: ViewGroup scroll = (ViewGroup) frame.findViewById(R.id.credential_scroll); On 2015/02/04 22:36:11, melandory wrote: > On 2015/02/04 22:17:52, newt wrote: > > You should use a ListView instead of a ScrollView containing a LinearLayout. > > ListView is designed for this type of situation. > > You are right that I do not need it here, but I left ScrollView because I will > need it in follow up CL: view should be scrollable when we have more than 2 > credentials to show. Should I change it or may I leave it? ListView handles scrolling. You should use ListView here. https://codereview.chromium.org/861103002/diff/460001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/AccountChooserInfoBar.java:152: private void setupCustomButtonsRow(ViewGroup frame) { On 2015/02/04 22:36:11, melandory wrote: > On 2015/02/04 22:17:52, newt wrote: > > I can add support to InfoBarLayout for this button. What exactly do you need > > that InfoBarLayout doesn't provide currently? > > 1) Infobar forces specific style of buttons (something like main button is blue > and so on), in my case I will potentially have 2 buttons, both white. InfoBarLayout can be modified to support different button colors. I'll add that to my current work on InfoBarLayout. > 2) I'm not completely understand logic behind InfoBarLayout::onClick(View view). > If I'll decide to go with it then I need to create another handler (similar to > mInfoBarView.onCloseButtonClicked();) and pass View there, because I need it to > show popupMenu. It doesn't look very clean to me. Maybe you can point me to > different approach? When a button is clicked, InfoBarView.onButtonClicked() is pressed. This class extends InfoBarView, so all you need to do to detect button clicks is to override onButtonClicked() in this class.
https://codereview.chromium.org/861103002/diff/460001/chrome/android/java/res... File chrome/android/java/res/layout/credential_picker_infobar_item.xml (right): https://codereview.chromium.org/861103002/diff/460001/chrome/android/java/res... chrome/android/java/res/layout/credential_picker_infobar_item.xml:8: android:layout_width="fill_parent" On 2015/02/04 22:17:51, newt wrote: > use match_parent instead of fill_parent. They're synonyms, but we use > match_parent for consistency. Done. https://codereview.chromium.org/861103002/diff/460001/chrome/android/java/res... chrome/android/java/res/layout/credential_picker_infobar_item.xml:14: android:id="@+id/credentials_app_image" On 2015/02/04 22:17:51, newt wrote: > why "app_image"? Isn't this a user profile image? How about calling this > "profile_image"? The IDs below could also be improved. Done. https://codereview.chromium.org/861103002/diff/460001/chrome/android/java/res... chrome/android/java/res/layout/credential_picker_infobar_item.xml:26: android:id="@+id/credentials_app_name" On 2015/02/04 22:17:51, newt wrote: > attributes should be in this order: > - id > - layout_width > - layout_height > - other layout_* attributes > - other attributes, typically alphabetized Done. https://codereview.chromium.org/861103002/diff/460001/chrome/android/java/res... chrome/android/java/res/layout/credential_picker_infobar_item.xml:27: android:textAppearance="?android:attr/textAppearanceMedium" On 2015/02/04 22:17:51, newt wrote: > textAppearanceMedium and textAppearanceSmall have different colors and sizes on > Android L devices compared to pre-L devices. You should probably specify the > font color and size explicitly. I'll take this into account. I've chosen same font settings as Indentity team who is implementing this infobar for Android. So I prefer to discuss this with them and have similar appearance. https://codereview.chromium.org/861103002/diff/460001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/infobar/AccountChooserInfoBar.java (right): https://codereview.chromium.org/861103002/diff/460001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/AccountChooserInfoBar.java:54: static InfoBar show(long nativeInfoBar, int enumeratedIconId, String[] usernames) { On 2015/02/04 22:17:52, newt wrote: > can you make this private? Done. https://codereview.chromium.org/861103002/diff/460001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/AccountChooserInfoBar.java:63: * @param titleText Message to display in the infobar. On 2015/02/04 22:17:52, newt wrote: > titleText and moreButtonText aren't parameters Done. https://codereview.chromium.org/861103002/diff/460001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/AccountChooserInfoBar.java:120: /** Represents row in account chooser infobar, usual row has avatar, username and full name. On 2015/02/04 22:17:52, newt wrote: > nit: newline before "Represents" Done. https://codereview.chromium.org/861103002/diff/460001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/AccountChooserInfoBar.java:130: username_view.setText(username); On 2015/02/04 22:17:51, newt wrote: > camelCase for Java variable names Done. https://codereview.chromium.org/861103002/diff/460001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/AccountChooserInfoBar.java:152: private void setupCustomButtonsRow(ViewGroup frame) { On 2015/02/04 22:44:40, newt wrote: > On 2015/02/04 22:36:11, melandory wrote: > > On 2015/02/04 22:17:52, newt wrote: > > > I can add support to InfoBarLayout for this button. What exactly do you need > > > that InfoBarLayout doesn't provide currently? > > > > 1) Infobar forces specific style of buttons (something like main button is > blue > > and so on), in my case I will potentially have 2 buttons, both white. > > InfoBarLayout can be modified to support different button colors. I'll add that > to my current work on InfoBarLayout. > > > 2) I'm not completely understand logic behind InfoBarLayout::onClick(View > view). > > If I'll decide to go with it then I need to create another handler (similar to > > mInfoBarView.onCloseButtonClicked();) and pass View there, because I need it > to > > show popupMenu. It doesn't look very clean to me. Maybe you can point me to > > different approach? > > When a button is clicked, InfoBarView.onButtonClicked() is pressed. This class > extends InfoBarView, so all you need to do to detect button clicks is to > override onButtonClicked() in this class. I'm still a bit confused (I haven't formulated my thoughts clearly before, sorry). InfobarView has public void onButtonClicked(boolean isPrimaryButton); which is called from InfoBarLayout::onClick(View view), because InfoBarLayout implements View.OnClickListener and called first whem button is clicked. So I see two problems: * I need View in order to show pop up when my button is clicked (see AccountChooserInfoBar::showMorePopup(View v). So I either need to change InfobarView ::onButtonClicked signature to onButtonClicked(boolean isPrimaryButton, View v) or introduce new method in InfoBarView onButtonWhichNeedsViewClicked(boolean isPrimaryButton, View v). * In case I'll introduce new method it should be called only when my special buttton is clicke, so in order to detect my button in InfoBarLayout::onClick I'll need to introduce new type of button R.id.button_which_needs_view, same as R.id.button_tertiary, R.id.button_tertiary, R.id.infobar_close_button And since we have this discussion here, just few general questions. Why not to allow infobars to have more control over Buttons?
https://codereview.chromium.org/861103002/diff/460001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/infobar/AccountChooserInfoBar.java (right): https://codereview.chromium.org/861103002/diff/460001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/AccountChooserInfoBar.java:152: private void setupCustomButtonsRow(ViewGroup frame) { On 2015/02/05 14:45:36, melandory wrote: > On 2015/02/04 22:44:40, newt wrote: > > On 2015/02/04 22:36:11, melandory wrote: > > > On 2015/02/04 22:17:52, newt wrote: > > > > I can add support to InfoBarLayout for this button. What exactly do you > need > > > > that InfoBarLayout doesn't provide currently? > > > > > > 1) Infobar forces specific style of buttons (something like main button is > > blue > > > and so on), in my case I will potentially have 2 buttons, both white. > > > > InfoBarLayout can be modified to support different button colors. I'll add > that > > to my current work on InfoBarLayout. > > > > > 2) I'm not completely understand logic behind InfoBarLayout::onClick(View > > view). > > > If I'll decide to go with it then I need to create another handler (similar > to > > > mInfoBarView.onCloseButtonClicked();) and pass View there, because I need it > > to > > > show popupMenu. It doesn't look very clean to me. Maybe you can point me to > > > different approach? > > > > When a button is clicked, InfoBarView.onButtonClicked() is pressed. This class > > extends InfoBarView, so all you need to do to detect button clicks is to > > override onButtonClicked() in this class. > > I'm still a bit confused (I haven't formulated my thoughts clearly before, > sorry). > > InfobarView has > public void onButtonClicked(boolean isPrimaryButton); > > which is called from InfoBarLayout::onClick(View view), because InfoBarLayout > implements View.OnClickListener and called first whem button is clicked. > > So I see two problems: > > * I need View in order to show pop up when my button is clicked (see > AccountChooserInfoBar::showMorePopup(View v). So I either need to change > InfobarView ::onButtonClicked signature to onButtonClicked(boolean > isPrimaryButton, View v) or introduce new method in InfoBarView > onButtonWhichNeedsViewClicked(boolean isPrimaryButton, View v). > > * In case I'll introduce new method it should be called only when my special > buttton is clicke, so in order to detect my button in InfoBarLayout::onClick > I'll need to introduce new type of button R.id.button_which_needs_view, same as > R.id.button_tertiary, R.id.button_tertiary, R.id.infobar_close_button > > > And since we have this discussion here, just few general questions. Why not to > allow infobars to have more control over Buttons? Ah, I didn't realize you need access to the View to show the pop-up. I think what I'll do is add support for custom views in place of buttons in InfoBarLayout. https://codereview.chromium.org/861103002/diff/480001/chrome/android/java/res... File chrome/android/java/res/layout/credential_picker_buttonbar.xml (right): https://codereview.chromium.org/861103002/diff/480001/chrome/android/java/res... chrome/android/java/res/layout/credential_picker_buttonbar.xml:13: <Button You might be able to use a Spinner here, instead of a Button that shows a pop-up when pressed. The material Spinner looks just like the mocks for this button.
melandory@chromium.org changed reviewers: - brettw@chromium.org
Apparently, I misunderstood OWNERS file, in order to fix it I'm adding avi@ for chrome/browser/ui/tab_helpers.cc and pkasting@ for chrome/browser/ui/android/infobars/account_chooser_infobar.h chrome/browser/ui/android/infobars/account_chooser_infobar.cc chrome/chrome_browser.gypi chrome/chrome_browser_ui.gypi components/infobars/core/infobar_delegate.h components/infobars/core/infobar_delegate.cc On 2015/02/05 18:08:42, newt wrote: > https://codereview.chromium.org/861103002/diff/460001/chrome/android/java/src... > File > chrome/android/java/src/org/chromium/chrome/browser/infobar/AccountChooserInfoBar.java > (right): > > https://codereview.chromium.org/861103002/diff/460001/chrome/android/java/src... > chrome/android/java/src/org/chromium/chrome/browser/infobar/AccountChooserInfoBar.java:152: > private void setupCustomButtonsRow(ViewGroup frame) { > On 2015/02/05 14:45:36, melandory wrote: > > On 2015/02/04 22:44:40, newt wrote: > > > On 2015/02/04 22:36:11, melandory wrote: > > > > On 2015/02/04 22:17:52, newt wrote: > > > > > I can add support to InfoBarLayout for this button. What exactly do you > > need > > > > > that InfoBarLayout doesn't provide currently? > > > > > > > > 1) Infobar forces specific style of buttons (something like main button is > > > blue > > > > and so on), in my case I will potentially have 2 buttons, both white. > > > > > > InfoBarLayout can be modified to support different button colors. I'll add > > that > > > to my current work on InfoBarLayout. > > > > > > > 2) I'm not completely understand logic behind InfoBarLayout::onClick(View > > > view). > > > > If I'll decide to go with it then I need to create another handler > (similar > > to > > > > mInfoBarView.onCloseButtonClicked();) and pass View there, because I need > it > > > to > > > > show popupMenu. It doesn't look very clean to me. Maybe you can point me > to > > > > different approach? > > > > > > When a button is clicked, InfoBarView.onButtonClicked() is pressed. This > class > > > extends InfoBarView, so all you need to do to detect button clicks is to > > > override onButtonClicked() in this class. > > > > I'm still a bit confused (I haven't formulated my thoughts clearly before, > > sorry). > > > > InfobarView has > > public void onButtonClicked(boolean isPrimaryButton); > > > > which is called from InfoBarLayout::onClick(View view), because InfoBarLayout > > implements View.OnClickListener and called first whem button is clicked. > > > > So I see two problems: > > > > * I need View in order to show pop up when my button is clicked (see > > AccountChooserInfoBar::showMorePopup(View v). So I either need to change > > InfobarView ::onButtonClicked signature to onButtonClicked(boolean > > isPrimaryButton, View v) or introduce new method in InfoBarView > > onButtonWhichNeedsViewClicked(boolean isPrimaryButton, View v). > > > > * In case I'll introduce new method it should be called only when my special > > buttton is clicke, so in order to detect my button in InfoBarLayout::onClick > > I'll need to introduce new type of button R.id.button_which_needs_view, same > as > > R.id.button_tertiary, R.id.button_tertiary, R.id.infobar_close_button > > > > > > And since we have this discussion here, just few general questions. Why not to > > allow infobars to have more control over Buttons? > > Ah, I didn't realize you need access to the View to show the pop-up. I think > what I'll do is add support for custom views in place of buttons in > InfoBarLayout. > > https://codereview.chromium.org/861103002/diff/480001/chrome/android/java/res... > File chrome/android/java/res/layout/credential_picker_buttonbar.xml (right): > > https://codereview.chromium.org/861103002/diff/480001/chrome/android/java/res... > chrome/android/java/res/layout/credential_picker_buttonbar.xml:13: <Button > You might be able to use a Spinner here, instead of a Button that shows a pop-up > when pressed. The material Spinner looks just like the mocks for this button.
melandory@chromium.org changed reviewers: + avi@chromium.org, pkasting@chromium.org
Apparently, I misunderstood OWNERS file, in order to fix it I'm adding avi@ for chrome/browser/ui/tab_helpers.cc and pkasting@ for chrome/browser/ui/android/infobars/account_chooser_infobar.h chrome/browser/ui/android/infobars/account_chooser_infobar.cc chrome/chrome_browser.gypi chrome/chrome_browser_ui.gypi components/infobars/core/infobar_delegate.h components/infobars/core/infobar_delegate.cc https://codereview.chromium.org/861103002/diff/460001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/infobar/AccountChooserInfoBar.java (right): https://codereview.chromium.org/861103002/diff/460001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/AccountChooserInfoBar.java:30: private enum CredentialType { On 2015/02/04 22:17:52, newt wrote: > enums are discouraged on Android for performance reasons. In this case, I'd just > use private static final ints. https://codereview.chromium.org/872893004/
Patchset #18 (id:400001) has been deleted
Patchset #17 (id:340001) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #21 (id:520001) has been deleted
Patchset #20 (id:500001) has been deleted
https://codereview.chromium.org/861103002/diff/540001/chrome/browser/password... File chrome/browser/password_manager/account_chooser_infobar_delegate_android.cc (right): https://codereview.chromium.org/861103002/diff/540001/chrome/browser/password... chrome/browser/password_manager/account_chooser_infobar_delegate_android.cc:7: #include <cstddef> You don't need this. https://codereview.chromium.org/861103002/diff/540001/chrome/browser/password... chrome/browser/password_manager/account_chooser_infobar_delegate_android.cc:35: ui_controller_->ChooseCredential(*password_form, credential_type); Nit: Simpler: if ((credential_type != CredentialType::CREDENTIAL_TYPE_LOCAL) && (credential_type != CredentialType::CREDENTIAL_TYPE_FEDERATED)) return; auto& credentials_forms = (credential_type == CredentialType::CREDENTIAL_TYPE_LOCAL) ? ui_controller_->local_credentials_forms() : ui_controller_->federated_credentials_forms(); if (credential_index < credentials_forms.size()) { ui_controller_->ChooseCredential(*credentials_forms[credential_index], credential_type); } You can change the initial conditional to a DCHECK if you know the caller will only ever pass one of those two values. https://codereview.chromium.org/861103002/diff/540001/chrome/browser/password... File chrome/browser/password_manager/account_chooser_infobar_delegate_android.h (right): https://codereview.chromium.org/861103002/diff/540001/chrome/browser/password... chrome/browser/password_manager/account_chooser_infobar_delegate_android.h:9: #include "base/memory/scoped_vector.h" Nit: This #include isn't needed, the type in question comes from manage_passwords_ui_controller.h. https://codereview.chromium.org/861103002/diff/540001/chrome/browser/password... chrome/browser/password_manager/account_chooser_infobar_delegate_android.h:25: // Android-only infobar to notify that the generated password was saved. This comment doesn't sound correct. It sounds like you copy-and-pasted it from another class. https://codereview.chromium.org/861103002/diff/540001/chrome/browser/password... chrome/browser/password_manager/account_chooser_infobar_delegate_android.h:28: // Creates and shows the infobar. Implemented in the platform-specific file. Nit: Please follow other infobar delegates' comment style (see next comment for why I write "infobar service"): Creates an account chooser infobar and delegate and adds the infobar to |infobar_service|. You should place the note about where this is implemented -- including the precise filename -- in the .cc file where you would have otherwise defined this. That said, why does this have to be defined in a different file? Why can't the .cc file here #include account_chooser_infobar.h and just define the method in question? https://codereview.chromium.org/861103002/diff/540001/chrome/browser/password... chrome/browser/password_manager/account_chooser_infobar_delegate_android.h:29: static void Create(content::WebContents* web_contents, Pass an InfoBarService* instead of a WebContents*. https://codereview.chromium.org/861103002/diff/540001/chrome/browser/password... chrome/browser/password_manager/account_chooser_infobar_delegate_android.h:38: void choose_credential(unsigned int credential_index, "unsigned int" is banned by the Google style guide. You should be using int or size_t (probably the latter based on the .cc file). This may mean you need to fix the caller side. Nit: Use CamelCase names for non-inlined functions. https://codereview.chromium.org/861103002/diff/540001/chrome/browser/password... chrome/browser/password_manager/account_chooser_infobar_delegate_android.h:45: // InfoBarDelegate implementation: Nit: Prefix "InfoBarDelegate" with "infobars::" and remove "implementation" https://codereview.chromium.org/861103002/diff/540001/chrome/browser/password... chrome/browser/password_manager/account_chooser_infobar_delegate_android.h:52: // for example TranslateUIDelegate does it. Nit: This comment sounds like it belongs in ManagePasswordUIController somewhere instead of here. https://codereview.chromium.org/861103002/diff/540001/chrome/browser/password... chrome/browser/password_manager/account_chooser_infobar_delegate_android.h:53: // AccountChooserInfoBarDelegateAndroid doesn't own this pointer. So who does own it? "Owned by ____" would be shorter and clearer. https://codereview.chromium.org/861103002/diff/540001/chrome/browser/ui/andro... File chrome/browser/ui/android/infobars/account_chooser_infobar.h (right): https://codereview.chromium.org/861103002/diff/540001/chrome/browser/ui/andro... chrome/browser/ui/android/infobars/account_chooser_infobar.h:14: #include "base/strings/string16.h" This #include is unused. https://codereview.chromium.org/861103002/diff/540001/chrome/browser/ui/andro... chrome/browser/ui/android/infobars/account_chooser_infobar.h:15: #include "chrome/browser/password_manager/account_chooser_infobar_delegate_android.h" I believe you can eliminate this and simply forward-declare AccountChooserInfoBarDelegateAndroid. https://codereview.chromium.org/861103002/diff/540001/components/infobars/cor... File components/infobars/core/infobar_delegate.h (right): https://codereview.chromium.org/861103002/diff/540001/components/infobars/cor... components/infobars/core/infobar_delegate.h:109: AsAccountChooserInfoBarDelegateAndroid(); You don't need this method. You only use it in AccountChooserInfoBar::GetDelegate(), where you already know the delegate is this type. Just static_cast the delegate there instead of adding this method. (These methods should only be needed when some code needs to do RTTI.)
Patchset #21 (id:560001) has been deleted
https://codereview.chromium.org/861103002/diff/460001/chrome/android/java/res... File chrome/android/java/res/layout/credential_picker_buttonbar.xml (right): https://codereview.chromium.org/861103002/diff/460001/chrome/android/java/res... chrome/android/java/res/layout/credential_picker_buttonbar.xml:1: <?xml version="1.0" encoding="utf-8"?> On 2015/02/04 22:17:51, newt wrote: > Hopefully, you can get rid of this file and use the standard infobar buttons. > Why did you need to create this in the first place? As discussed in comments to AccountChooserInfoBar.java custom views in place of buttons support should be added to InfoBar. https://codereview.chromium.org/861103002/diff/460001/chrome/android/java/res... File chrome/android/java/res/layout/credential_picker_infobar_item.xml (right): https://codereview.chromium.org/861103002/diff/460001/chrome/android/java/res... chrome/android/java/res/layout/credential_picker_infobar_item.xml:22: android:layout_width="wrap_content" On 2015/02/04 22:17:51, newt wrote: > use layout_width="0dp" and layout_weight="1". That'll make this view expand > horizontally to take up the available space. Done. https://codereview.chromium.org/861103002/diff/460001/chrome/android/java/res... File chrome/android/java/res/menu/credential_picker_infobar_more_menu_popup.xml (right): https://codereview.chromium.org/861103002/diff/460001/chrome/android/java/res... chrome/android/java/res/menu/credential_picker_infobar_more_menu_popup.xml:7: xmlns:chrome="http://schemas.android.com/apk/res-auto"> On 2015/02/04 22:17:51, newt wrote: > remove xmlns:chrome Done. https://codereview.chromium.org/861103002/diff/460001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/infobar/AccountChooserInfoBar.java (right): https://codereview.chromium.org/861103002/diff/460001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/AccountChooserInfoBar.java:105: ViewGroup scroll = (ViewGroup) frame.findViewById(R.id.credential_scroll); On 2015/02/04 22:44:40, newt wrote: > On 2015/02/04 22:36:11, melandory wrote: > > On 2015/02/04 22:17:52, newt wrote: > > > You should use a ListView instead of a ScrollView containing a LinearLayout. > > > ListView is designed for this type of situation. > > > > You are right that I do not need it here, but I left ScrollView because I will > > need it in follow up CL: view should be scrollable when we have more than 2 > > credentials to show. Should I change it or may I leave it? > > ListView handles scrolling. You should use ListView here. Still it feels like separate CL, since I'll have to add quite some code in order to use ListView (extend ArrayAdapter for example). So for now I've got rid of Scroll View completely and added TODO. https://codereview.chromium.org/861103002/diff/460001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/AccountChooserInfoBar.java:177: private void onChooseCredentials( On 2015/02/04 22:17:52, newt wrote: > I'd remove this method and just call nativeOnCredentialClicked() directly in > paddCredentialsToNative(). Done. https://codereview.chromium.org/861103002/diff/460001/chrome/android/java/str... File chrome/android/java/strings/android_chrome_strings.grd (right): https://codereview.chromium.org/861103002/diff/460001/chrome/android/java/str... chrome/android/java/strings/android_chrome_strings.grd:485: Settings On 2015/02/04 22:17:52, newt wrote: > There's already a string for "Settings" in this file, which you should use Done. https://codereview.chromium.org/861103002/diff/460001/chrome/android/java/str... chrome/android/java/strings/android_chrome_strings.grd:488: Learn more On 2015/02/04 22:17:52, newt wrote: > There's actually already a "Learn more" string in android_ui_strings.grd, which > you can use. I have found only More there: https://code.google.com/p/chromium/codesearch#chromium/src/ui/android/java/st... https://codereview.chromium.org/861103002/diff/480001/chrome/android/java/res... File chrome/android/java/res/layout/credential_picker_buttonbar.xml (right): https://codereview.chromium.org/861103002/diff/480001/chrome/android/java/res... chrome/android/java/res/layout/credential_picker_buttonbar.xml:13: <Button On 2015/02/05 18:08:42, newt wrote: > You might be able to use a Spinner here, instead of a Button that shows a pop-up > when pressed. The material Spinner looks just like the mocks for this button. I like this idea. I'll ask UX (because as I understood choices in spinner will appear differently) and add TODO to code which I'll address in separate CL. https://codereview.chromium.org/861103002/diff/540001/chrome/browser/password... File chrome/browser/password_manager/account_chooser_infobar_delegate_android.cc (right): https://codereview.chromium.org/861103002/diff/540001/chrome/browser/password... chrome/browser/password_manager/account_chooser_infobar_delegate_android.cc:7: #include <cstddef> On 2015/02/16 20:37:07, Peter Kasting wrote: > You don't need this. Done. https://codereview.chromium.org/861103002/diff/540001/chrome/browser/password... chrome/browser/password_manager/account_chooser_infobar_delegate_android.cc:35: ui_controller_->ChooseCredential(*password_form, credential_type); On 2015/02/16 20:37:07, Peter Kasting wrote: > Nit: Simpler: > > if ((credential_type != CredentialType::CREDENTIAL_TYPE_LOCAL) && > (credential_type != CredentialType::CREDENTIAL_TYPE_FEDERATED)) > return; > auto& credentials_forms = > (credential_type == CredentialType::CREDENTIAL_TYPE_LOCAL) ? > ui_controller_->local_credentials_forms() : > ui_controller_->federated_credentials_forms(); > if (credential_index < credentials_forms.size()) { > ui_controller_->ChooseCredential(*credentials_forms[credential_index], > credential_type); > } > > You can change the initial conditional to a DCHECK if you know the caller will > only ever pass one of those two values. Great! I didn't like my code, but nothing more elegant haven't come to me. https://codereview.chromium.org/861103002/diff/540001/chrome/browser/password... File chrome/browser/password_manager/account_chooser_infobar_delegate_android.h (right): https://codereview.chromium.org/861103002/diff/540001/chrome/browser/password... chrome/browser/password_manager/account_chooser_infobar_delegate_android.h:9: #include "base/memory/scoped_vector.h" On 2015/02/16 20:37:07, Peter Kasting wrote: > Nit: This #include isn't needed, the type in question comes from > manage_passwords_ui_controller.h. Done. https://codereview.chromium.org/861103002/diff/540001/chrome/browser/password... chrome/browser/password_manager/account_chooser_infobar_delegate_android.h:25: // Android-only infobar to notify that the generated password was saved. On 2015/02/16 20:37:07, Peter Kasting wrote: > This comment doesn't sound correct. It sounds like you copy-and-pasted it from > another class. Done. https://codereview.chromium.org/861103002/diff/540001/chrome/browser/password... chrome/browser/password_manager/account_chooser_infobar_delegate_android.h:28: // Creates and shows the infobar. Implemented in the platform-specific file. On 2015/02/16 20:37:07, Peter Kasting wrote: > Nit: Please follow other infobar delegates' comment style (see next comment for > why I write "infobar service"): > > Creates an account chooser infobar and delegate and adds the infobar to > |infobar_service|. > > You should place the note about where this is implemented -- including the > precise filename -- in the .cc file where you would have otherwise defined this. > That said, why does this have to be defined in a different file? Why can't the > .cc file here #include account_chooser_infobar.h and just define the method in > question? Done. https://codereview.chromium.org/861103002/diff/540001/chrome/browser/password... chrome/browser/password_manager/account_chooser_infobar_delegate_android.h:29: static void Create(content::WebContents* web_contents, On 2015/02/16 20:37:07, Peter Kasting wrote: > Pass an InfoBarService* instead of a WebContents*. Done. https://codereview.chromium.org/861103002/diff/540001/chrome/browser/password... chrome/browser/password_manager/account_chooser_infobar_delegate_android.h:38: void choose_credential(unsigned int credential_index, On 2015/02/16 20:37:07, Peter Kasting wrote: > "unsigned int" is banned by the Google style guide. You should be using int or > size_t (probably the latter based on the .cc file). This may mean you need to > fix the caller side. > > Nit: Use CamelCase names for non-inlined functions. Done. https://codereview.chromium.org/861103002/diff/540001/chrome/browser/password... chrome/browser/password_manager/account_chooser_infobar_delegate_android.h:45: // InfoBarDelegate implementation: On 2015/02/16 20:37:07, Peter Kasting wrote: > Nit: Prefix "InfoBarDelegate" with "infobars::" and remove "implementation" Done. https://codereview.chromium.org/861103002/diff/540001/chrome/browser/password... chrome/browser/password_manager/account_chooser_infobar_delegate_android.h:52: // for example TranslateUIDelegate does it. On 2015/02/16 20:37:07, Peter Kasting wrote: > Nit: This comment sounds like it belongs in ManagePasswordUIController somewhere > instead of here. Removed it for now. https://codereview.chromium.org/861103002/diff/540001/chrome/browser/password... chrome/browser/password_manager/account_chooser_infobar_delegate_android.h:53: // AccountChooserInfoBarDelegateAndroid doesn't own this pointer. On 2015/02/16 20:37:07, Peter Kasting wrote: > So who does own it? "Owned by ____" would be shorter and clearer. Done. https://codereview.chromium.org/861103002/diff/540001/chrome/browser/ui/andro... File chrome/browser/ui/android/infobars/account_chooser_infobar.h (right): https://codereview.chromium.org/861103002/diff/540001/chrome/browser/ui/andro... chrome/browser/ui/android/infobars/account_chooser_infobar.h:14: #include "base/strings/string16.h" On 2015/02/16 20:37:07, Peter Kasting wrote: > This #include is unused. Done. https://codereview.chromium.org/861103002/diff/540001/chrome/browser/ui/andro... chrome/browser/ui/android/infobars/account_chooser_infobar.h:15: #include "chrome/browser/password_manager/account_chooser_infobar_delegate_android.h" On 2015/02/16 20:37:07, Peter Kasting wrote: > I believe you can eliminate this and simply forward-declare > AccountChooserInfoBarDelegateAndroid. Done. https://codereview.chromium.org/861103002/diff/540001/components/infobars/cor... File components/infobars/core/infobar_delegate.h (right): https://codereview.chromium.org/861103002/diff/540001/components/infobars/cor... components/infobars/core/infobar_delegate.h:109: AsAccountChooserInfoBarDelegateAndroid(); On 2015/02/16 20:37:07, Peter Kasting wrote: > You don't need this method. > > You only use it in AccountChooserInfoBar::GetDelegate(), where you already know > the delegate is this type. Just static_cast the delegate there instead of > adding this method. > > (These methods should only be needed when some code needs to do RTTI.) Done.
Patchset #5 (id:120001) has been deleted
Patchset #16 (id:440001) has been deleted
Patchset #17 (id:480001) has been deleted
Patchset #13 (id:300001) has been deleted
newt@, can you have a look again. Thanks!
Patchset #20 (id:640001) has been deleted
https://codereview.chromium.org/861103002/diff/620001/chrome/android/java/res... File chrome/android/java/res/layout/account_chooser_infobar_buttonbar.xml (right): https://codereview.chromium.org/861103002/diff/620001/chrome/android/java/res... chrome/android/java/res/layout/account_chooser_infobar_buttonbar.xml:1: <?xml version="1.0" encoding="utf-8"?> InfoBarLayout should now have everything you need to use standard infobar buttons and get rid of this file. Something like this should work: public void createContent(InfoBarLayout layout) { layout.setButtons("No thanks", null); View moreButton = ButtonCompat.createBorderlessButton(context); moreButton.setOnClickListener(...); layout.setCustomViewInButtonRow(moreButton); } https://codereview.chromium.org/861103002/diff/620001/chrome/android/java/res... File chrome/android/java/res/layout/account_chooser_infobar_item.xml (right): https://codereview.chromium.org/861103002/diff/620001/chrome/android/java/res... chrome/android/java/res/layout/account_chooser_infobar_item.xml:33: android:textAppearance="?android:attr/textAppearanceMedium"/> textAppearanceMedium and textAppearanceSmall are quite different on Android L devices compared to earlier devices. It's safer to declare the size and color explicitly rather than using a textAppearance attribute https://codereview.chromium.org/861103002/diff/620001/chrome/android/java/res... chrome/android/java/res/layout/account_chooser_infobar_item.xml:39: android:layout_width="wrap_content" Attributes should be in this order: - id - layout_width - layout_height - other layout_* attributes - other attributes, generally alphabetized https://codereview.chromium.org/861103002/diff/620001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/infobar/AccountChooserInfoBar.java (right): https://codereview.chromium.org/861103002/diff/620001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/AccountChooserInfoBar.java:108: // TODO(melandory): use ListView to represent credentials and make them scrollable. Please go ahead and turn this into a ListView as part of this CL. It's not much extra code (mostly just moving things around), and I'd rather not land a large chunk of code that's not production quality with a TODO to rewrite it before shipping. Might as well get it right the first time. (Alternatively, you could leave this class partially implemented with a TODO to finish implementing it. That would be fine too) https://codereview.chromium.org/861103002/diff/620001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/AccountChooserInfoBar.java:142: final Integer currentCredentialIndex = credentialIndex; Use int instead of Integer throughout this file. You should never use Integer except when you have no other option (e.g. when storing ints in a generic container) https://codereview.chromium.org/861103002/diff/620001/chrome/android/java/str... File chrome/android/java/strings/android_chrome_strings.grd (right): https://codereview.chromium.org/861103002/diff/620001/chrome/android/java/str... chrome/android/java/strings/android_chrome_strings.grd:548: Learn more There are already strings you can use for "Learn more" and "No thanks" (IDS_LEARN_MORE and IDS_NO_THANKS)
https://codereview.chromium.org/861103002/diff/620001/chrome/browser/password... File chrome/browser/password_manager/account_chooser_infobar_delegate_android.h (right): https://codereview.chromium.org/861103002/diff/620001/chrome/browser/password... chrome/browser/password_manager/account_chooser_infobar_delegate_android.h:22: // Android-only infobar to allow user to choose credentials for login. Nit: Technically this is an infobar delegate, not an infobar. https://codereview.chromium.org/861103002/diff/620001/chrome/browser/password... chrome/browser/password_manager/account_chooser_infobar_delegate_android.h:43: // infobars::InfoBarDelegate Nit: Need trailing colon https://codereview.chromium.org/861103002/diff/620001/chrome/browser/ui/andro... File chrome/browser/ui/android/infobars/account_chooser_infobar.cc (right): https://codereview.chromium.org/861103002/diff/620001/chrome/browser/ui/andro... chrome/browser/ui/android/infobars/account_chooser_infobar.cc:20: AccountChooserInfoBar::CreateRenderInfoBar(JNIEnv* env) { Function definition order must match declaration order https://codereview.chromium.org/861103002/diff/620001/chrome/browser/ui/andro... chrome/browser/ui/android/infobars/account_chooser_infobar.cc:25: for (auto password_form : infobar_delegate->local_credentials_forms()) { Nit: No {} (you don't put it on one-line conditionals, so don't do it here) https://codereview.chromium.org/861103002/diff/620001/chrome/browser/ui/andro... chrome/browser/ui/android/infobars/account_chooser_infobar.cc:38: return; Nit: For clarity, add: "// We're closing; don't call anything, it might access the owner." https://codereview.chromium.org/861103002/diff/620001/chrome/browser/ui/andro... chrome/browser/ui/android/infobars/account_chooser_infobar.cc:50: void AccountChooserInfoBar::ChooseCredential(int credential_item, Why do we have this helper for just one caller? Why not inline into OnCredentialClicked()? https://codereview.chromium.org/861103002/diff/620001/chrome/browser/ui/andro... chrome/browser/ui/android/infobars/account_chooser_infobar.cc:55: credential_item, (password_manager::CredentialType)credential_type); No C-style casts, please https://codereview.chromium.org/861103002/diff/620001/chrome/browser/ui/andro... File chrome/browser/ui/android/infobars/account_chooser_infobar.h (right): https://codereview.chromium.org/861103002/diff/620001/chrome/browser/ui/andro... chrome/browser/ui/android/infobars/account_chooser_infobar.h:31: // InfoBarAndroid implementation: Nit: Remove " implementation" https://codereview.chromium.org/861103002/diff/620001/chrome/browser/ui/passw... File chrome/browser/ui/passwords/manage_passwords_ui_controller.cc (right): https://codereview.chromium.org/861103002/diff/620001/chrome/browser/ui/passw... chrome/browser/ui/passwords/manage_passwords_ui_controller.cc:137: return true; If you're going to return true here, you need to put the entire remainder of the function in the #else, to avoid potential unreachable code warnings. Does it make sense to set |bubble_shown_| to true instead? Then you could omit both the last two lines here. https://codereview.chromium.org/861103002/diff/620001/chrome/browser/ui/tab_h... File chrome/browser/ui/tab_helpers.cc (right): https://codereview.chromium.org/861103002/diff/620001/chrome/browser/ui/tab_h... chrome/browser/ui/tab_helpers.cc:168: ManagePasswordsUIController::CreateForWebContents(web_contents); This doesn't seem right -- didn't you mean to create this for all platforms, instead of moving it from non-Android to Android-only?
https://codereview.chromium.org/861103002/diff/620001/chrome/android/java/res... File chrome/android/java/res/layout/account_chooser_infobar_buttonbar.xml (right): https://codereview.chromium.org/861103002/diff/620001/chrome/android/java/res... chrome/android/java/res/layout/account_chooser_infobar_buttonbar.xml:1: <?xml version="1.0" encoding="utf-8"?> On 2015/02/17 21:49:07, newt wrote: > InfoBarLayout should now have everything you need to use standard infobar > buttons and get rid of this file. > > Something like this should work: > > public void createContent(InfoBarLayout layout) { > layout.setButtons("No thanks", null); > View moreButton = ButtonCompat.createBorderlessButton(context); > moreButton.setOnClickListener(...); > layout.setCustomViewInButtonRow(moreButton); > } Done. https://codereview.chromium.org/861103002/diff/620001/chrome/android/java/res... File chrome/android/java/res/layout/account_chooser_infobar_item.xml (right): https://codereview.chromium.org/861103002/diff/620001/chrome/android/java/res... chrome/android/java/res/layout/account_chooser_infobar_item.xml:33: android:textAppearance="?android:attr/textAppearanceMedium"/> On 2015/02/17 21:49:07, newt wrote: > textAppearanceMedium and textAppearanceSmall are quite different on Android L > devices compared to earlier devices. It's safer to declare the size and color > explicitly rather than using a textAppearance attribute I've chosen same font settings as native Android implementation has, because I think having same appearance is more important than having consistent appearance across Android versions. https://codereview.chromium.org/861103002/diff/620001/chrome/android/java/res... chrome/android/java/res/layout/account_chooser_infobar_item.xml:39: android:layout_width="wrap_content" On 2015/02/17 21:49:07, newt wrote: > Attributes should be in this order: > - id > - layout_width > - layout_height > - other layout_* attributes > - other attributes, generally alphabetized Done. https://codereview.chromium.org/861103002/diff/620001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/infobar/AccountChooserInfoBar.java (right): https://codereview.chromium.org/861103002/diff/620001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/AccountChooserInfoBar.java:108: // TODO(melandory): use ListView to represent credentials and make them scrollable. On 2015/02/17 21:49:08, newt wrote: > Please go ahead and turn this into a ListView as part of this CL. It's not much > extra code (mostly just moving things around), and I'd rather not land a large > chunk of code that's not production quality with a TODO to rewrite it before > shipping. Might as well get it right the first time. (Alternatively, you could > leave this class partially implemented with a TODO to finish implementing it. > That would be fine too) Implemented this using ListView. But why do you think that having just LinearLayout and not having scroll at all for now is very bad idea (having in mind that there was nothing about this area should be scrollable in mocks)? https://codereview.chromium.org/861103002/diff/620001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/AccountChooserInfoBar.java:142: final Integer currentCredentialIndex = credentialIndex; On 2015/02/17 21:49:08, newt wrote: > Use int instead of Integer throughout this file. You should never use Integer > except when you have no other option (e.g. when storing ints in a generic > container) Done. https://codereview.chromium.org/861103002/diff/620001/chrome/android/java/str... File chrome/android/java/strings/android_chrome_strings.grd (right): https://codereview.chromium.org/861103002/diff/620001/chrome/android/java/str... chrome/android/java/strings/android_chrome_strings.grd:548: Learn more On 2015/02/17 21:49:08, newt wrote: > There are already strings you can use for "Learn more" and "No thanks" > (IDS_LEARN_MORE and IDS_NO_THANKS) Done. https://codereview.chromium.org/861103002/diff/620001/chrome/browser/password... File chrome/browser/password_manager/account_chooser_infobar_delegate_android.h (right): https://codereview.chromium.org/861103002/diff/620001/chrome/browser/password... chrome/browser/password_manager/account_chooser_infobar_delegate_android.h:22: // Android-only infobar to allow user to choose credentials for login. On 2015/02/18 00:23:05, Peter Kasting wrote: > Nit: Technically this is an infobar delegate, not an infobar. Done. https://codereview.chromium.org/861103002/diff/620001/chrome/browser/password... chrome/browser/password_manager/account_chooser_infobar_delegate_android.h:43: // infobars::InfoBarDelegate On 2015/02/18 00:23:05, Peter Kasting wrote: > Nit: Need trailing colon Done. https://codereview.chromium.org/861103002/diff/620001/chrome/browser/ui/andro... File chrome/browser/ui/android/infobars/account_chooser_infobar.cc (right): https://codereview.chromium.org/861103002/diff/620001/chrome/browser/ui/andro... chrome/browser/ui/android/infobars/account_chooser_infobar.cc:20: AccountChooserInfoBar::CreateRenderInfoBar(JNIEnv* env) { On 2015/02/18 00:23:05, Peter Kasting wrote: > Function definition order must match declaration order Done. https://codereview.chromium.org/861103002/diff/620001/chrome/browser/ui/andro... chrome/browser/ui/android/infobars/account_chooser_infobar.cc:25: for (auto password_form : infobar_delegate->local_credentials_forms()) { On 2015/02/18 00:23:05, Peter Kasting wrote: > Nit: No {} (you don't put it on one-line conditionals, so don't do it here) Done. https://codereview.chromium.org/861103002/diff/620001/chrome/browser/ui/andro... chrome/browser/ui/android/infobars/account_chooser_infobar.cc:38: return; On 2015/02/18 00:23:05, Peter Kasting wrote: > Nit: For clarity, add: "// We're closing; don't call anything, it might access > the owner." Done. https://codereview.chromium.org/861103002/diff/620001/chrome/browser/ui/andro... chrome/browser/ui/android/infobars/account_chooser_infobar.cc:50: void AccountChooserInfoBar::ChooseCredential(int credential_item, On 2015/02/18 00:23:05, Peter Kasting wrote: > Why do we have this helper for just one caller? Why not inline into > OnCredentialClicked()? Done. https://codereview.chromium.org/861103002/diff/620001/chrome/browser/ui/andro... chrome/browser/ui/android/infobars/account_chooser_infobar.cc:55: credential_item, (password_manager::CredentialType)credential_type); On 2015/02/18 00:23:05, Peter Kasting wrote: > No C-style casts, please Done. https://codereview.chromium.org/861103002/diff/620001/chrome/browser/ui/andro... File chrome/browser/ui/android/infobars/account_chooser_infobar.h (right): https://codereview.chromium.org/861103002/diff/620001/chrome/browser/ui/andro... chrome/browser/ui/android/infobars/account_chooser_infobar.h:31: // InfoBarAndroid implementation: On 2015/02/18 00:23:05, Peter Kasting wrote: > Nit: Remove " implementation" Done. https://codereview.chromium.org/861103002/diff/620001/chrome/browser/ui/passw... File chrome/browser/ui/passwords/manage_passwords_ui_controller.cc (right): https://codereview.chromium.org/861103002/diff/620001/chrome/browser/ui/passw... chrome/browser/ui/passwords/manage_passwords_ui_controller.cc:137: return true; On 2015/02/18 00:23:05, Peter Kasting wrote: > If you're going to return true here, you need to put the entire remainder of the > function in the #else, to avoid potential unreachable code warnings. > > Does it make sense to set |bubble_shown_| to true instead? Then you could omit > both the last two lines here. Done. https://codereview.chromium.org/861103002/diff/620001/chrome/browser/ui/tab_h... File chrome/browser/ui/tab_helpers.cc (right): https://codereview.chromium.org/861103002/diff/620001/chrome/browser/ui/tab_h... chrome/browser/ui/tab_helpers.cc:168: ManagePasswordsUIController::CreateForWebContents(web_contents); On 2015/02/18 00:23:05, Peter Kasting wrote: > This doesn't seem right -- didn't you mean to create this for all platforms, > instead of moving it from non-Android to Android-only? You're right. Fixed.
LGTM https://codereview.chromium.org/861103002/diff/680001/chrome/android/java/str... File chrome/android/java/strings/android_chrome_strings.grd (right): https://codereview.chromium.org/861103002/diff/680001/chrome/android/java/str... chrome/android/java/strings/android_chrome_strings.grd:551: More Please make sure we don't have a generic version of this string elsewhere. I would have expected someone else to need this already. If we have a non-generic version elsewhere it may be worth collapsing all such cases into one generic instance. https://codereview.chromium.org/861103002/diff/680001/chrome/browser/password... File chrome/browser/password_manager/account_chooser_infobar_delegate_android.cc (right): https://codereview.chromium.org/861103002/diff/680001/chrome/browser/password... chrome/browser/password_manager/account_chooser_infobar_delegate_android.cc:32: credential_type == CredentialType::CREDENTIAL_TYPE_FEDERATED); How about: if (credential_type == CredentialType::CREDENTIAL_TYPE_EMPTY) { ui_controller_->ChooseCredential(autofill::PasswordForm(), credential_type); return; } DCHECK((credential_type == CredentialType::CREDENTIAL_TYPE_LOCAL) || (credential_type == CredentialType::CREDENTIAL_TYPE_FEDERATED)); ... This is basically the same length but is less indented and hopefully slightly easier to parse. https://codereview.chromium.org/861103002/diff/680001/chrome/browser/password... File chrome/browser/password_manager/account_chooser_infobar_delegate_android.h (right): https://codereview.chromium.org/861103002/diff/680001/chrome/browser/password... chrome/browser/password_manager/account_chooser_infobar_delegate_android.h:30: ~AccountChooserInfoBarDelegateAndroid() override = default; Nit: I would define this in the .cc file rather than defaulting it here, because AFAICT, defaulting it here will cause the destructor to be inline, and we generally don't want such destructors to be inline. (I believe you could actually use "= default" in the .cc file if you wanted, but by that point it's probably just as good to simply use {}.) https://codereview.chromium.org/861103002/diff/680001/chrome/browser/ui/andro... File chrome/browser/ui/android/infobars/account_chooser_infobar.cc (right): https://codereview.chromium.org/861103002/diff/680001/chrome/browser/ui/andro... chrome/browser/ui/android/infobars/account_chooser_infobar.cc:16: void NotifyDelegateAboutChoosenCredential( Nit: "Choosen" isn't a word. Why not just call this ChooseCredential()? https://codereview.chromium.org/861103002/diff/680001/chrome/browser/ui/andro... chrome/browser/ui/android/infobars/account_chooser_infobar.cc:21: static_cast<AccountChooserInfoBarDelegateAndroid*>(delegate); Nit: You do this twice in this file. If you define a private helper: AccountChooserInfoBarDelegateAndroid* AccountChooserInfoBar::GetDelegate() { return static_cast<AccountChooserInfoBarDelegateAndroid*>(delegate()); } ...then you can avoid this cast entirely by passing GetDelegate() directly from the callers, and you can avoid the temp in CreateRenderInfoBar() by simply doing "GetDelegate()->local_credentials_forms()" in the for loop directly. https://codereview.chromium.org/861103002/diff/680001/chrome/browser/ui/andro... chrome/browser/ui/android/infobars/account_chooser_infobar.cc:60: // the owner. Nit: This comment can all fit on one line, and should go on the EOL of the return statement (since otherwise it unconditionally says we're closing when that's actually conditional). https://codereview.chromium.org/861103002/diff/680001/chrome/browser/ui/andro... File chrome/browser/ui/android/infobars/account_chooser_infobar.h (right): https://codereview.chromium.org/861103002/diff/680001/chrome/browser/ui/andro... chrome/browser/ui/android/infobars/account_chooser_infobar.h:23: ~AccountChooserInfoBar() override = default; Nit: Same comment
melandory@chromium.org changed reviewers: - avi@chromium.org, vabr@chromium.org
melandory@chromium.org changed reviewers: + mkwst@chromium.org
mkwst@chromium.org: Please review changes in chrome/browser/password_manager/account_chooser_infobar_delegate_android.h chrome/browser/password_manager/account_chooser_infobar_delegate_android.cc
https://codereview.chromium.org/861103002/diff/620001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/infobar/AccountChooserInfoBar.java (right): https://codereview.chromium.org/861103002/diff/620001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/AccountChooserInfoBar.java:108: // TODO(melandory): use ListView to represent credentials and make them scrollable. On 2015/02/18 21:15:34, melandory wrote: > On 2015/02/17 21:49:08, newt wrote: > > Please go ahead and turn this into a ListView as part of this CL. It's not > much > > extra code (mostly just moving things around), and I'd rather not land a large > > chunk of code that's not production quality with a TODO to rewrite it before > > shipping. Might as well get it right the first time. (Alternatively, you could > > leave this class partially implemented with a TODO to finish implementing it. > > That would be fine too) > Implemented this using ListView. > > But why do you think that having just LinearLayout and not having scroll at all > for now is very bad idea (having in mind that there was nothing about this area > should be scrollable in mocks)? This has to be scrollable because some people have more than 2 accounts. https://codereview.chromium.org/861103002/diff/680001/chrome/android/java/res... File chrome/android/java/res/layout/account_chooser_infobar_items.xml (right): https://codereview.chromium.org/861103002/diff/680001/chrome/android/java/res... chrome/android/java/res/layout/account_chooser_infobar_items.xml:1: <?xml version="1.0" encoding="utf-8"?> I'd name this file something like "account_chooser_infobar_list.xml" https://codereview.chromium.org/861103002/diff/680001/chrome/android/java/res... chrome/android/java/res/layout/account_chooser_infobar_items.xml:5: <FrameLayout You don't need this FrameLayout. The ListView itself can be the root. https://codereview.chromium.org/861103002/diff/680001/chrome/android/java/res... chrome/android/java/res/layout/account_chooser_infobar_items.xml:10: android:id="@+id/account_chooser_layout" I'd change this ID to "account_list" (or you may not even need the ID at all once you make the ListView the root of this file) https://codereview.chromium.org/861103002/diff/680001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/infobar/AccountChooserInfoBar.java (right): https://codereview.chromium.org/861103002/diff/680001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/AccountChooserInfoBar.java:75: /** Called when the user selects a contextual menu item */ Don't add javadoc when overriding a method, unless the javadoc comment is different (in a helpful way) than the method you're overriding. In this case, I'd just remove the javadoc comment. https://codereview.chromium.org/861103002/diff/680001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/AccountChooserInfoBar.java:88: * Called when the close button is clicked. Notifies the native infobar, which closes the Same deal: onCloseButtonClicked() is already documented in the parent class. No need to duplicate that here. You could keep "Notifies the native infobar, which closes the infobar", but move it inside the method as an implementation comment. https://codereview.chromium.org/861103002/diff/680001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/AccountChooserInfoBar.java:102: * Used to specify button layout and custom content. Makes infobar display list of Again, no javadoc needed. https://codereview.chromium.org/861103002/diff/680001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/AccountChooserInfoBar.java:120: if (adapter.getCount() >= 2) { I'd expect this to be "> 2" not ">= 2". Why did you use >= ? https://codereview.chromium.org/861103002/diff/680001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/AccountChooserInfoBar.java:127: layout.setCustomContent(accountsView); To make the ListView expand to the full width of the infobar, you can add this code to the end of InfoBarLayout.updateCustomGroupForRow(): if (row == ROW_OTHER && mCustomGroup.views.length == 1) { mCustomGroup.gravity = Gravity.FILL_HORIZONTAL; } This doesn't break our other infobars and it seems like a reasonable default for the custom view to expand to the full infobar width. Of course, you should check how this looks in landscape mode on a tablet.... you may find that you don't want the ListView to be full-width anymore. https://codereview.chromium.org/861103002/diff/680001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/AccountChooserInfoBar.java:132: class ViewHolder { Personally, I wouldn't bother with a ViewHolder class. It's an optimization that makes almost no difference in this case since the list is typically so short. In fact, it's probably a net slowdown rather than speedup... and it obfuscates the code somewhat. https://codereview.chromium.org/861103002/diff/680001/ui/android/java/res/val... File ui/android/java/res/values/dimens.xml (right): https://codereview.chromium.org/861103002/diff/680001/ui/android/java/res/val... ui/android/java/res/values/dimens.xml:25: <dimen name="account_chooser_infobar_item_height">100dp</dimen> put this constant in src/chrome, not src/ui.
https://codereview.chromium.org/861103002/diff/680001/chrome/android/java/res... File chrome/android/java/res/layout/account_chooser_infobar_items.xml (right): https://codereview.chromium.org/861103002/diff/680001/chrome/android/java/res... chrome/android/java/res/layout/account_chooser_infobar_items.xml:5: <FrameLayout On 2015/02/18 22:26:02, newt wrote: > You don't need this FrameLayout. The ListView itself can be the root. This way I can't control height of ListView, because listView.getLayoutParams(); returns null. https://codereview.chromium.org/861103002/diff/680001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/infobar/AccountChooserInfoBar.java (right): https://codereview.chromium.org/861103002/diff/680001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/AccountChooserInfoBar.java:120: if (adapter.getCount() >= 2) { On 2015/02/18 22:26:03, newt wrote: > I'd expect this to be "> 2" not ">= 2". Why did you use >= ? List view shows only one element by default. Behavior I want to achieve: show 2.5 elements when more than 2 elements are available and show everything otherwise (1 and 2 elements).
New patchsets have been uploaded after l-g-t-m from pkasting@chromium.org
https://codereview.chromium.org/861103002/diff/680001/chrome/android/java/res... File chrome/android/java/res/layout/account_chooser_infobar_items.xml (right): https://codereview.chromium.org/861103002/diff/680001/chrome/android/java/res... chrome/android/java/res/layout/account_chooser_infobar_items.xml:5: <FrameLayout On 2015/02/18 22:31:57, melandory wrote: > On 2015/02/18 22:26:02, newt wrote: > > You don't need this FrameLayout. The ListView itself can be the root. > > This way I can't control height of ListView, because listView.getLayoutParams(); > returns null. In fact, you can remove the FrameLayout. Then, in AccountChooserInfoBar.java, you can just create LayoutParams from scratch (see my comment in AccountChooserInfoBar) https://codereview.chromium.org/861103002/diff/680001/chrome/android/java/res... chrome/android/java/res/layout/account_chooser_infobar_items.xml:14: android:orientation="vertical"/> orientation doesn't apply to ListView https://codereview.chromium.org/861103002/diff/680001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/infobar/AccountChooserInfoBar.java (right): https://codereview.chromium.org/861103002/diff/680001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/AccountChooserInfoBar.java:120: if (adapter.getCount() >= 2) { On 2015/02/18 22:31:57, melandory wrote: > On 2015/02/18 22:26:03, newt wrote: > > I'd expect this to be "> 2" not ">= 2". Why did you use >= ? > > List view shows only one element by default. Behavior I want to achieve: show > 2.5 elements when more than 2 elements are available and show everything > otherwise (1 and 2 elements). Ok. Then how about this: float numVisibleItems = adapter.getCount() > 2 ? 2.5f : adapter.getCount(); int listViewHeight = (int) (numVisibleItems * getContext().getResources().getDimension(R.dimen.account_chooser_infobar_item_height)); listView.setLayoutParams(new LayoutParams(LayoutParams.MATCH_PARENT, listViewHeight));
LGTM % nits. https://codereview.chromium.org/861103002/diff/700001/chrome/browser/password... File chrome/browser/password_manager/account_chooser_infobar_delegate_android.cc (right): https://codereview.chromium.org/861103002/diff/700001/chrome/browser/password... chrome/browser/password_manager/account_chooser_infobar_delegate_android.cc:32: using namespace password_manager; Nit: I don't think we generally do this sort of scoped `using`. And really, why not just put the infobar itself into the `password_manager` namespace? https://codereview.chromium.org/861103002/diff/700001/chrome/browser/password... chrome/browser/password_manager/account_chooser_infobar_delegate_android.cc:32: using namespace password_manager; Nit: I don't think we generally do this sort of scoped `using`. And really, why not just put the infobar itself into the `password_manager` namespace? https://codereview.chromium.org/861103002/diff/700001/chrome/browser/password... chrome/browser/password_manager/account_chooser_infobar_delegate_android.cc:40: (credential_type == CredentialType::CREDENTIAL_TYPE_LOCAL) Nit: You don't need the () here.
https://codereview.chromium.org/861103002/diff/700001/chrome/browser/password... File chrome/browser/password_manager/account_chooser_infobar_delegate_android.cc (right): https://codereview.chromium.org/861103002/diff/700001/chrome/browser/password... chrome/browser/password_manager/account_chooser_infobar_delegate_android.cc:32: using namespace password_manager; On 2015/02/19 10:06:17, Mike West wrote: > Nit: I don't think we generally do this sort of scoped `using`. I'm OK with it if it makes the code below noticeably more readable. Otherwise it could go away. Don't care much. (I'm always happier with function-scoped using than file-scoped using.) Putting the infobar in the namespace might make sense. Or getting rid of the password_manager namespace entirely... our namespace policy across Chromium seems rather inconsistent, I usually end up just avoiding namespaces. https://codereview.chromium.org/861103002/diff/700001/chrome/browser/password... chrome/browser/password_manager/account_chooser_infobar_delegate_android.cc:40: (credential_type == CredentialType::CREDENTIAL_TYPE_LOCAL) On 2015/02/19 10:06:17, Mike West wrote: > Nit: You don't need the () here. No, but I think it makes things a little more readable. Up to you guys though.
New patchsets have been uploaded after l-g-t-m from mkwst@chromium.org
https://codereview.chromium.org/861103002/diff/680001/chrome/android/java/res... File chrome/android/java/res/layout/account_chooser_infobar_items.xml (right): https://codereview.chromium.org/861103002/diff/680001/chrome/android/java/res... chrome/android/java/res/layout/account_chooser_infobar_items.xml:1: <?xml version="1.0" encoding="utf-8"?> On 2015/02/18 22:26:02, newt wrote: > I'd name this file something like "account_chooser_infobar_list.xml" Done. https://codereview.chromium.org/861103002/diff/680001/chrome/android/java/res... chrome/android/java/res/layout/account_chooser_infobar_items.xml:10: android:id="@+id/account_chooser_layout" On 2015/02/18 22:26:02, newt wrote: > I'd change this ID to "account_list" (or you may not even need the ID at all > once you make the ListView the root of this file) Done. https://codereview.chromium.org/861103002/diff/680001/chrome/android/java/res... chrome/android/java/res/layout/account_chooser_infobar_items.xml:14: android:orientation="vertical"/> On 2015/02/19 00:00:48, newt wrote: > orientation doesn't apply to ListView Done. https://codereview.chromium.org/861103002/diff/680001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/infobar/AccountChooserInfoBar.java (right): https://codereview.chromium.org/861103002/diff/680001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/AccountChooserInfoBar.java:75: /** Called when the user selects a contextual menu item */ On 2015/02/18 22:26:03, newt wrote: > Don't add javadoc when overriding a method, unless the javadoc comment is > different (in a helpful way) than the method you're overriding. In this case, > I'd just remove the javadoc comment. Done. https://codereview.chromium.org/861103002/diff/680001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/AccountChooserInfoBar.java:88: * Called when the close button is clicked. Notifies the native infobar, which closes the On 2015/02/18 22:26:03, newt wrote: > Same deal: onCloseButtonClicked() is already documented in the parent class. No > need to duplicate that here. You could keep "Notifies the native infobar, which > closes the infobar", but move it inside the method as an implementation comment. Done. https://codereview.chromium.org/861103002/diff/680001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/AccountChooserInfoBar.java:102: * Used to specify button layout and custom content. Makes infobar display list of On 2015/02/18 22:26:03, newt wrote: > Again, no javadoc needed. Done. https://codereview.chromium.org/861103002/diff/680001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/AccountChooserInfoBar.java:120: if (adapter.getCount() >= 2) { On 2015/02/19 00:00:48, newt wrote: > On 2015/02/18 22:31:57, melandory wrote: > > On 2015/02/18 22:26:03, newt wrote: > > > I'd expect this to be "> 2" not ">= 2". Why did you use >= ? > > > > List view shows only one element by default. Behavior I want to achieve: show > > 2.5 elements when more than 2 elements are available and show everything > > otherwise (1 and 2 elements). > > Ok. Then how about this: > > float numVisibleItems = adapter.getCount() > 2 ? 2.5f : adapter.getCount(); > int listViewHeight = (int) (numVisibleItems * > getContext().getResources().getDimension(R.dimen.account_chooser_infobar_item_height)); > listView.setLayoutParams(new LayoutParams(LayoutParams.MATCH_PARENT, > listViewHeight)); Correct me if I'm wrong, but if I remove FrameLayout then LayoutParams I should create should be from InfoBar.LayoutsParams. And there is two issues: 1) InfoBar.LayoutParams are private 2) InfoBar.LayoutParams doesn't have ctor which takes (int width, int height) as arguments: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/android/jav... It just forces (LayoutParams.WRAP_CONTENT, LayoutParams.WRAP_CONTENT) https://codereview.chromium.org/861103002/diff/680001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/AccountChooserInfoBar.java:127: layout.setCustomContent(accountsView); On 2015/02/18 22:26:03, newt wrote: > To make the ListView expand to the full width of the infobar, you can add this > code to the end of InfoBarLayout.updateCustomGroupForRow(): > > if (row == ROW_OTHER && mCustomGroup.views.length == 1) { > mCustomGroup.gravity = Gravity.FILL_HORIZONTAL; > } > > This doesn't break our other infobars and it seems like a reasonable default for > the custom view to expand to the full infobar width. Of course, you should check > how this looks in landscape mode on a tablet.... you may find that you don't > want the ListView to be full-width anymore. Done. https://codereview.chromium.org/861103002/diff/680001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/AccountChooserInfoBar.java:132: class ViewHolder { On 2015/02/18 22:26:03, newt wrote: > Personally, I wouldn't bother with a ViewHolder class. It's an optimization that > makes almost no difference in this case since the list is typically so short. In > fact, it's probably a net slowdown rather than speedup... and it obfuscates the > code somewhat. Done. https://codereview.chromium.org/861103002/diff/680001/chrome/android/java/str... File chrome/android/java/strings/android_chrome_strings.grd (right): https://codereview.chromium.org/861103002/diff/680001/chrome/android/java/str... chrome/android/java/strings/android_chrome_strings.grd:551: More On 2015/02/18 21:44:17, Peter Kasting wrote: > Please make sure we don't have a generic version of this string elsewhere. I > would have expected someone else to need this already. If we have a non-generic > version elsewhere it may be worth collapsing all such cases into one generic > instance. I haven't found generic one, so I've created it. https://codereview.chromium.org/861103002/diff/680001/chrome/browser/password... File chrome/browser/password_manager/account_chooser_infobar_delegate_android.cc (right): https://codereview.chromium.org/861103002/diff/680001/chrome/browser/password... chrome/browser/password_manager/account_chooser_infobar_delegate_android.cc:32: credential_type == CredentialType::CREDENTIAL_TYPE_FEDERATED); On 2015/02/18 21:44:17, Peter Kasting wrote: > How about: > > if (credential_type == CredentialType::CREDENTIAL_TYPE_EMPTY) { > ui_controller_->ChooseCredential(autofill::PasswordForm(), credential_type); > return; > } > > DCHECK((credential_type == CredentialType::CREDENTIAL_TYPE_LOCAL) || > (credential_type == CredentialType::CREDENTIAL_TYPE_FEDERATED)); > ... > > This is basically the same length but is less indented and hopefully slightly > easier to parse. Done. https://codereview.chromium.org/861103002/diff/680001/chrome/browser/password... File chrome/browser/password_manager/account_chooser_infobar_delegate_android.h (right): https://codereview.chromium.org/861103002/diff/680001/chrome/browser/password... chrome/browser/password_manager/account_chooser_infobar_delegate_android.h:30: ~AccountChooserInfoBarDelegateAndroid() override = default; On 2015/02/18 21:44:17, Peter Kasting wrote: > Nit: I would define this in the .cc file rather than defaulting it here, because > AFAICT, defaulting it here will cause the destructor to be inline, and we > generally don't want such destructors to be inline. > > (I believe you could actually use "= default" in the .cc file if you wanted, but > by that point it's probably just as good to simply use {}.) Done. https://codereview.chromium.org/861103002/diff/680001/chrome/browser/ui/andro... File chrome/browser/ui/android/infobars/account_chooser_infobar.cc (right): https://codereview.chromium.org/861103002/diff/680001/chrome/browser/ui/andro... chrome/browser/ui/android/infobars/account_chooser_infobar.cc:16: void NotifyDelegateAboutChoosenCredential( On 2015/02/18 21:44:17, Peter Kasting wrote: > Nit: "Choosen" isn't a word. Why not just call this ChooseCredential()? I'll remove this function, because with GetDelegate it lost its purpose. https://codereview.chromium.org/861103002/diff/680001/chrome/browser/ui/andro... chrome/browser/ui/android/infobars/account_chooser_infobar.cc:21: static_cast<AccountChooserInfoBarDelegateAndroid*>(delegate); On 2015/02/18 21:44:17, Peter Kasting wrote: > Nit: You do this twice in this file. If you define a private helper: > > AccountChooserInfoBarDelegateAndroid* AccountChooserInfoBar::GetDelegate() { > return static_cast<AccountChooserInfoBarDelegateAndroid*>(delegate()); > } > > ...then you can avoid this cast entirely by passing GetDelegate() directly from > the callers, and you can avoid the temp in CreateRenderInfoBar() by simply doing > "GetDelegate()->local_credentials_forms()" in the for loop directly. Done. https://codereview.chromium.org/861103002/diff/680001/chrome/browser/ui/andro... chrome/browser/ui/android/infobars/account_chooser_infobar.cc:60: // the owner. On 2015/02/18 21:44:17, Peter Kasting wrote: > Nit: This comment can all fit on one line, and should go on the EOL of the > return statement (since otherwise it unconditionally says we're closing when > that's actually conditional). Done. https://codereview.chromium.org/861103002/diff/680001/chrome/browser/ui/andro... File chrome/browser/ui/android/infobars/account_chooser_infobar.h (right): https://codereview.chromium.org/861103002/diff/680001/chrome/browser/ui/andro... chrome/browser/ui/android/infobars/account_chooser_infobar.h:23: ~AccountChooserInfoBar() override = default; On 2015/02/18 21:44:17, Peter Kasting wrote: > Nit: Same comment Done. https://codereview.chromium.org/861103002/diff/680001/ui/android/java/res/val... File ui/android/java/res/values/dimens.xml (right): https://codereview.chromium.org/861103002/diff/680001/ui/android/java/res/val... ui/android/java/res/values/dimens.xml:25: <dimen name="account_chooser_infobar_item_height">100dp</dimen> On 2015/02/18 22:26:03, newt wrote: > put this constant in src/chrome, not src/ui. Done.
https://codereview.chromium.org/861103002/diff/680001/chrome/android/java/res... File chrome/android/java/res/layout/account_chooser_infobar_items.xml (right): https://codereview.chromium.org/861103002/diff/680001/chrome/android/java/res... chrome/android/java/res/layout/account_chooser_infobar_items.xml:5: <FrameLayout On 2015/02/19 00:00:48, newt wrote: > On 2015/02/18 22:31:57, melandory wrote: > > On 2015/02/18 22:26:02, newt wrote: > > > You don't need this FrameLayout. The ListView itself can be the root. > > > > This way I can't control height of ListView, because > listView.getLayoutParams(); > > returns null. > > In fact, you can remove the FrameLayout. Then, in AccountChooserInfoBar.java, > you can just create LayoutParams from scratch (see my comment in > AccountChooserInfoBar) As far as I understood it won't work if I remove FrameLayout because then I need to work with InfoBarLayout.LayoutParams (for details see comment in AccountChooserInfoBar)
https://codereview.chromium.org/861103002/diff/780001/chrome/android/java/res... File chrome/android/java/res/layout/account_chooser_infobar_item.xml (right): https://codereview.chromium.org/861103002/diff/780001/chrome/android/java/res... chrome/android/java/res/layout/account_chooser_infobar_item.xml:14: android:layout_gravity="center_vertical" nit: put layout_width and layout_height before layout_gravity https://codereview.chromium.org/861103002/diff/780001/chrome/android/java/res... File chrome/android/java/res/layout/account_chooser_infobar_list.xml (right): https://codereview.chromium.org/861103002/diff/780001/chrome/android/java/res... chrome/android/java/res/layout/account_chooser_infobar_list.xml:7: android:layout_width="wrap_content" width should be match_parent https://codereview.chromium.org/861103002/diff/780001/chrome/android/java/res... chrome/android/java/res/layout/account_chooser_infobar_list.xml:9: <ListView nit: fix indentation. It should be like the indentation in account_chooser_infobar_item.xml https://codereview.chromium.org/861103002/diff/780001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/infobar/AccountChooserInfoBar.java (right): https://codereview.chromium.org/861103002/diff/780001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/AccountChooserInfoBar.java:30: * An infobar which allows user to choose credentials. Can you improve this comment a bit. Which credentials? Why is the user choosing them? https://codereview.chromium.org/861103002/diff/780001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/AccountChooserInfoBar.java:107: ViewGroup accountsView = nit: fix indentation https://codereview.chromium.org/861103002/diff/780001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/AccountChooserInfoBar.java:126: LinearLayout row = You should still reuse convertView if it's non-null, like you were doing before: if (convertView == null) { convertView = LayoutInflater.from(getContext()).inflate(R.layout.account_chooser_infobar_item, parent, false); } Also, be sure to pass parent instead of null to inflate(). That'll ensure the layout is inflated with the correct type of LayoutParams. https://codereview.chromium.org/861103002/diff/780001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/AccountChooserInfoBar.java:153: * For the Account Chooser infobar different, than InfobarLayout has, appearance of button Please clarify this comment. I'm really not sure what it's saying. https://codereview.chromium.org/861103002/diff/780001/chrome/android/java/str... File chrome/android/java/strings/android_chrome_strings.grd (right): https://codereview.chromium.org/861103002/diff/780001/chrome/android/java/str... chrome/android/java/strings/android_chrome_strings.grd:118: <message name="IDS_MORE" desc="Generic label for a more button.ore [CHAR-LIMIT=20]"> "Generic label for a button to show more items or options." Also, remove "ore". https://codereview.chromium.org/861103002/diff/780001/chrome/android/java/str... chrome/android/java/strings/android_chrome_strings.grd:560: <!-- Account chooser infobar strings. --> Move this string down with the other infobar strings (It's currently in the middle of the settings strings. I know, the structure of this file is hard to see)
https://codereview.chromium.org/861103002/diff/780001/chrome/android/java/res... File chrome/android/java/res/layout/account_chooser_infobar_list.xml (right): https://codereview.chromium.org/861103002/diff/780001/chrome/android/java/res... chrome/android/java/res/layout/account_chooser_infobar_list.xml:5: <FrameLayout It's OK to keep this FrameLayout. We could fix InfoBarLayout to respect the custom view's layout params, but for now, this is fine.
https://codereview.chromium.org/861103002/diff/780001/chrome/android/java/res... File chrome/android/java/res/layout/account_chooser_infobar_item.xml (right): https://codereview.chromium.org/861103002/diff/780001/chrome/android/java/res... chrome/android/java/res/layout/account_chooser_infobar_item.xml:14: android:layout_gravity="center_vertical" On 2015/02/19 21:48:02, newt wrote: > nit: put layout_width and layout_height before layout_gravity Done. https://codereview.chromium.org/861103002/diff/780001/chrome/android/java/res... File chrome/android/java/res/layout/account_chooser_infobar_list.xml (right): https://codereview.chromium.org/861103002/diff/780001/chrome/android/java/res... chrome/android/java/res/layout/account_chooser_infobar_list.xml:5: <FrameLayout On 2015/02/19 21:49:27, newt wrote: > It's OK to keep this FrameLayout. We could fix InfoBarLayout to respect the > custom view's layout params, but for now, this is fine. Acknowledged. https://codereview.chromium.org/861103002/diff/780001/chrome/android/java/res... chrome/android/java/res/layout/account_chooser_infobar_list.xml:7: android:layout_width="wrap_content" On 2015/02/19 21:48:02, newt wrote: > width should be match_parent Done. https://codereview.chromium.org/861103002/diff/780001/chrome/android/java/res... chrome/android/java/res/layout/account_chooser_infobar_list.xml:9: <ListView On 2015/02/19 21:48:02, newt wrote: > nit: fix indentation. It should be like the indentation in > account_chooser_infobar_item.xml Done. https://codereview.chromium.org/861103002/diff/780001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/infobar/AccountChooserInfoBar.java (right): https://codereview.chromium.org/861103002/diff/780001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/AccountChooserInfoBar.java:30: * An infobar which allows user to choose credentials. On 2015/02/19 21:48:02, newt wrote: > Can you improve this comment a bit. Which credentials? Why is the user choosing > them? Done. https://codereview.chromium.org/861103002/diff/780001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/AccountChooserInfoBar.java:107: ViewGroup accountsView = On 2015/02/19 21:48:02, newt wrote: > nit: fix indentation what is issue with indentation? clang-format think it's fine. https://codereview.chromium.org/861103002/diff/780001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/AccountChooserInfoBar.java:126: LinearLayout row = On 2015/02/19 21:48:02, newt wrote: > You should still reuse convertView if it's non-null, like you were doing before: > > if (convertView == null) { > convertView = > LayoutInflater.from(getContext()).inflate(R.layout.account_chooser_infobar_item, > parent, false); > } > > Also, be sure to pass parent instead of null to inflate(). That'll ensure the > layout is inflated with the correct type of LayoutParams. Done. https://codereview.chromium.org/861103002/diff/780001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/AccountChooserInfoBar.java:153: * For the Account Chooser infobar different, than InfobarLayout has, appearance of button On 2015/02/19 21:48:02, newt wrote: > Please clarify this comment. I'm really not sure what it's saying. Yeah, comment is a bit outdated. But it remineds me: "No thanks" button is blue now, but according to mocks it shouldn't be blue. Should I in future create a custom button and add it in a same way as "More" button or at some point it will be possible to control appearance of infobar buttons? https://codereview.chromium.org/861103002/diff/780001/chrome/android/java/str... File chrome/android/java/strings/android_chrome_strings.grd (right): https://codereview.chromium.org/861103002/diff/780001/chrome/android/java/str... chrome/android/java/strings/android_chrome_strings.grd:118: <message name="IDS_MORE" desc="Generic label for a more button.ore [CHAR-LIMIT=20]"> On 2015/02/19 21:48:02, newt wrote: > "Generic label for a button to show more items or options." Also, remove "ore". Done. https://codereview.chromium.org/861103002/diff/780001/chrome/android/java/str... chrome/android/java/strings/android_chrome_strings.grd:560: <!-- Account chooser infobar strings. --> On 2015/02/19 21:48:02, newt wrote: > Move this string down with the other infobar strings (It's currently in the > middle of the settings strings. I know, the structure of this file is hard to > see) Done.
lgtm after nits https://codereview.chromium.org/861103002/diff/840001/chrome/android/java/res... File chrome/android/java/res/layout/account_chooser_infobar_list.xml (right): https://codereview.chromium.org/861103002/diff/840001/chrome/android/java/res... chrome/android/java/res/layout/account_chooser_infobar_list.xml:13: android:layout_height="match_parent" I'd leave this as wrap_content. It doesn't really matter since we explicitly set the height later, but it displays the intention better. https://codereview.chromium.org/861103002/diff/840001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/infobar/AccountChooserInfoBar.java (right): https://codereview.chromium.org/861103002/diff/840001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/AccountChooserInfoBar.java:107: ViewGroup accountsView = It looks awkward and could be easier to read. Like this: ViewGroup accountsView = (ViewGroup) LayoutInflater.from(getContext()).inflate( R.layout.account_chooser_infobar_list, null, false Make the same change below in getView(). (FWIW, clang-format on Java code is somewhat experimental.)
New patchsets have been uploaded after l-g-t-m from newt@chromium.org
https://codereview.chromium.org/861103002/diff/840001/chrome/android/java/res... File chrome/android/java/res/layout/account_chooser_infobar_list.xml (right): https://codereview.chromium.org/861103002/diff/840001/chrome/android/java/res... chrome/android/java/res/layout/account_chooser_infobar_list.xml:13: android:layout_height="match_parent" On 2015/02/19 22:56:00, newt wrote: > I'd leave this as wrap_content. It doesn't really matter since we explicitly set > the height later, but it displays the intention better. Done. https://codereview.chromium.org/861103002/diff/840001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/infobar/AccountChooserInfoBar.java (right): https://codereview.chromium.org/861103002/diff/840001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/AccountChooserInfoBar.java:107: ViewGroup accountsView = On 2015/02/19 22:56:00, newt wrote: > It looks awkward and could be easier to read. Like this: > > ViewGroup accountsView = (ViewGroup) > LayoutInflater.from(getContext()).inflate( > R.layout.account_chooser_infobar_list, null, false > > Make the same change below in getView(). > > (FWIW, clang-format on Java code is somewhat experimental.) Done.
Patchset #32 (id:900001) has been deleted
Patchset #31 (id:880001) has been deleted
Patchset #23 (id:720001) has been deleted
Patchset #27 (id:820001) has been deleted
Patchset #28 (id:860001) has been deleted
Patchset #17 (id:580001) has been deleted
Patchset #17 (id:600001) has been deleted
Patchset #24 (id:800001) has been deleted
Patchset #18 (id:660001) has been deleted
Patchset #20 (id:740001) has been deleted
Patchset #20 (id:760001) has been deleted
The CQ bit was checked by melandory@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/861103002/940001
melandory@chromium.org changed reviewers: + markusheintz@chromium.org
On 2015/02/20 14:01:10, I haz the power (commit-bot) wrote: > CQ is trying da patch. Follow status at > https://chromium-cq-status.appspot.com/patch-status/861103002/940001 Adding markusheintz@chromium.org to review last changes in manage_passwords_ui_controller
On 2015/02/20 14:01:10, I haz the power (commit-bot) wrote: > CQ is trying da patch. Follow status at > https://chromium-cq-status.appspot.com/patch-status/861103002/940001 Adding markusheintz@chromium.org to review last changes in manage_passwords_ui_controller
The changes from Patchset #24 LGTM. Would be interesting to learn why the test are crashing with out this changes. https://codereview.chromium.org/861103002/diff/960001/chrome/browser/ui/passw... File chrome/browser/ui/passwords/manage_passwords_ui_controller.h (right): https://codereview.chromium.org/861103002/diff/960001/chrome/browser/ui/passw... chrome/browser/ui/passwords/manage_passwords_ui_controller.h:185: virtual void UpdateAndroidAccountChooserInfoBarVisibility(); Please add a comment for this method.
New patchsets have been uploaded after l-g-t-m from markusheintz@chromium.org
Patchset #25 (id:980001) has been deleted
The CQ bit was checked by melandory@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/861103002/1000001
Message was sent while issue was closed.
Committed patchset #25 (id:1000001)
Message was sent while issue was closed.
Patchset 25 (id:??) landed as https://crrev.com/ca57a51bbb794575650b9e1b071b68aeae3d5e87 Cr-Commit-Position: refs/heads/master@{#317368} |
