Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1840513002:
Redesign sign in and sign in confirmation screens for Narnia 2.0 (Closed)
DescriptionRedesign sign in and sign in confirmation screens for Narnia 2.0
This CL implements new sign in and sign in confirmation screen. It is part of the effort to make Chrome sign in flow support N2.0. The mocks can be find in the issue. Currently we only implement for portrait mode.
BUG=595349, 574894, 598868
Committed: https://crrev.com/e8c60fdcbfdf07b55a671b818c52b950436429b1
Cr-Commit-Position: refs/heads/master@{#387359}
Patch Set 1 : #Patch Set 2 : rebase #Patch Set 3 : #
Total comments: 4
Patch Set 4 : address comments #
Total comments: 126
Patch Set 5 : address comments #Patch Set 6 : rebase #Patch Set 7 : new UI without CheckBox #
Total comments: 29
Patch Set 8 : #
Total comments: 8
Patch Set 9 : address comments #Messages
Total messages: 75 (48 generated)
Description was changed from ========== format format format format ready correct border and rule line border, rule line, confirm button top padding, head section ratio in confirmation screen backup backup layout good for confirmation good confirmation screen not scroll top scroll description 48dp backup better layout prototype backup signin screen show backup backup BUG= ========== to ========== BUG= ==========
Description was changed from ========== BUG= ========== to ========== Redesign sign in and sign in confirmation screens for Narnio 2.0 BUG= ==========
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:60001) has been deleted
Patchset #1 (id:80001) has been deleted
Patchset #3 (id:140001) has been deleted
Description was changed from ========== Redesign sign in and sign in confirmation screens for Narnio 2.0 BUG= ========== to ========== Redesign sign in and sign in confirmation screens for Narnio 2.0 BUG=595349 ==========
Description was changed from ========== Redesign sign in and sign in confirmation screens for Narnio 2.0 BUG=595349 ========== to ========== Redesign sign in and sign in confirmation screens for Narnio 2.0 This CL implements two new sign in and sign in confirmation screen. It is part of the effort to make Chrome sign in flow support N2.0. The mocks can be find here: https://folio.googleplex.com/chrome-ux-specs-and-sources/Clank%20-%20Material... BUG=595349, ==========
Description was changed from ========== Redesign sign in and sign in confirmation screens for Narnio 2.0 This CL implements two new sign in and sign in confirmation screen. It is part of the effort to make Chrome sign in flow support N2.0. The mocks can be find here: https://folio.googleplex.com/chrome-ux-specs-and-sources/Clank%20-%20Material... BUG=595349, ========== to ========== Redesign sign in and sign in confirmation screens for Narnio 2.0 This CL implements two new sign in and sign in confirmation screen. It is part of the effort to make Chrome sign in flow support N2.0. The mocks can be find in the issue. BUG=595349 ==========
Description was changed from ========== Redesign sign in and sign in confirmation screens for Narnio 2.0 This CL implements two new sign in and sign in confirmation screen. It is part of the effort to make Chrome sign in flow support N2.0. The mocks can be find in the issue. BUG=595349 ========== to ========== Redesign sign in and sign in confirmation screens for Narnio 2.0 This CL implements new sign in and sign in confirmation screen. It is part of the effort to make Chrome sign in flow support N2.0. The mocks can be find in the issue. BUG=595349 ==========
Description was changed from ========== Redesign sign in and sign in confirmation screens for Narnio 2.0 This CL implements new sign in and sign in confirmation screen. It is part of the effort to make Chrome sign in flow support N2.0. The mocks can be find in the issue. BUG=595349 ========== to ========== Redesign sign in and sign in confirmation screens for Narnio 2.0 This CL implements new sign in and sign in confirmation screen. It is part of the effort to make Chrome sign in flow support N2.0. The mocks can be find in the issue. Currently we only implement for portrait mode. BUG=595349 ==========
Description was changed from ========== Redesign sign in and sign in confirmation screens for Narnio 2.0 This CL implements new sign in and sign in confirmation screen. It is part of the effort to make Chrome sign in flow support N2.0. The mocks can be find in the issue. Currently we only implement for portrait mode. BUG=595349 ========== to ========== Redesign sign in and sign in confirmation screens for Narnio 2.0 This CL implements new sign in and sign in confirmation screen. It is part of the effort to make Chrome sign in flow support N2.0. The mocks can be find in the issue. Currently we only implement them for portrait mode. BUG=595349 ==========
Description was changed from ========== Redesign sign in and sign in confirmation screens for Narnio 2.0 This CL implements new sign in and sign in confirmation screen. It is part of the effort to make Chrome sign in flow support N2.0. The mocks can be find in the issue. Currently we only implement them for portrait mode. BUG=595349 ========== to ========== Redesign sign in and sign in confirmation screens for Narnio 2.0 This CL implements new sign in and sign in confirmation screen. It is part of the effort to make Chrome sign in flow support N2.0. The mocks can be find in the issue. Currently we only implement for portrait mode. BUG=595349 ==========
Description was changed from ========== Redesign sign in and sign in confirmation screens for Narnio 2.0 This CL implements new sign in and sign in confirmation screen. It is part of the effort to make Chrome sign in flow support N2.0. The mocks can be find in the issue. Currently we only implement for portrait mode. BUG=595349 ========== to ========== Redesign sign in and sign in confirmation screens for Narnio 2.0 This CL implements new sign in and sign in confirmation screen. It is part of the effort to make Chrome sign in flow support N2.0. The mocks can be find in the issue. Currently we only implement for portrait mode. And we have long term goal to make sign in flow unanimous from different access point. BUG=595349 ==========
Description was changed from ========== Redesign sign in and sign in confirmation screens for Narnio 2.0 This CL implements new sign in and sign in confirmation screen. It is part of the effort to make Chrome sign in flow support N2.0. The mocks can be find in the issue. Currently we only implement for portrait mode. And we have long term goal to make sign in flow unanimous from different access point. BUG=595349 ========== to ========== Redesign sign in and sign in confirmation screens for Narnio 2.0 This CL implements new sign in and sign in confirmation screen. It is part of the effort to make Chrome sign in flow support N2.0. The mocks can be find in the issue. Currently we only implement for portrait mode. BUG=595349 ==========
gogerald@chromium.org changed reviewers: + newt@chromium.org
Hi newt@, send you for review because you have reviewed most of the major changes in sign in recently. This is part of the effort to make Chrome support Narnio 2.0. We need this feature in M51. It missed the feature freezing point, but we are trying to land most of big changes before branch breaking point (this CL is one of two as I expected), could you help prioritize this review. Thanks!
Description was changed from ========== Redesign sign in and sign in confirmation screens for Narnio 2.0 This CL implements new sign in and sign in confirmation screen. It is part of the effort to make Chrome sign in flow support N2.0. The mocks can be find in the issue. Currently we only implement for portrait mode. BUG=595349 ========== to ========== Redesign sign in and sign in confirmation screens for Narnio 2.0 This CL implements new sign in and sign in confirmation screen. It is part of the effort to make Chrome sign in flow support N2.0. The mocks can be find in the issue. Currently we only implement for portrait mode. BUG=595349, 574894 ==========
bauerb@chromium.org changed reviewers: + bauerb@chromium.org
https://codereview.chromium.org/1840513002/diff/160001/chrome/android/java/re... File chrome/android/java/res/layout/account_signin_view.xml (right): https://codereview.chromium.org/1840513002/diff/160001/chrome/android/java/re... chrome/android/java/res/layout/account_signin_view.xml:150: android:drawableStart="@drawable/confirmation_logo_chrome" Nit: I would rename this to ...chrome_sync, because it refers to the Chrome Sync service here (the distinction is important in case the build does not have the Chrome branding, but rather the Chromium one). Also, "confirmation" alone isn't very descriptive, and these are in a single big directory with all the other drawables. signin_confirmation_... maybe? Or just leave it out and only use "chrome_sync_logo" or something. https://codereview.chromium.org/1840513002/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninConfirmationView.java (right): https://codereview.chromium.org/1840513002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninConfirmationView.java:67: * Disable fading out effect at the top of this ScrollView. This should be a regular comment in the body of this method, as it documents the implementation, not the public interface.
Description was changed from ========== Redesign sign in and sign in confirmation screens for Narnio 2.0 This CL implements new sign in and sign in confirmation screen. It is part of the effort to make Chrome sign in flow support N2.0. The mocks can be find in the issue. Currently we only implement for portrait mode. BUG=595349, 574894 ========== to ========== Redesign sign in and sign in confirmation screens for Narnia 2.0 This CL implements new sign in and sign in confirmation screen. It is part of the effort to make Chrome sign in flow support N2.0. The mocks can be find in the issue. Currently we only implement for portrait mode. BUG=595349, 574894 ==========
Patchset #4 (id:180001) has been deleted
https://codereview.chromium.org/1840513002/diff/160001/chrome/android/java/re... File chrome/android/java/res/layout/account_signin_view.xml (right): https://codereview.chromium.org/1840513002/diff/160001/chrome/android/java/re... chrome/android/java/res/layout/account_signin_view.xml:150: android:drawableStart="@drawable/confirmation_logo_chrome" On 2016/03/31 17:02:59, Bernhard Bauer wrote: > Nit: I would rename this to ...chrome_sync, because it refers to the Chrome Sync > service here (the distinction is important in case the build does not have the > Chrome branding, but rather the Chromium one). > > Also, "confirmation" alone isn't very descriptive, and these are in a single big > directory with all the other drawables. signin_confirmation_... maybe? Or just > leave it out and only use "chrome_sync_logo" or something. Done. https://codereview.chromium.org/1840513002/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninConfirmationView.java (right): https://codereview.chromium.org/1840513002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninConfirmationView.java:67: * Disable fading out effect at the top of this ScrollView. On 2016/03/31 17:02:59, Bernhard Bauer wrote: > This should be a regular comment in the body of this method, as it documents the > implementation, not the public interface. Done.
Chrome logos can't be checked into Chromium. Please put a Chromium icon in the chrome/android/java/res_chromium folder, and put the Chrome icon in the internal res_google_chrome directory. Change the icon name from "chrome_sync_icon" to something like "app_icon_small". Overdraw: the sign-in and confirmation screens both have significant overdraw (at least 2x everywhere). That means that we're wasting time drawing the entire screen, then drawing over it again. This typically happens when two full-window nested views both have a background color. You can see overdraw using Android Settings > Developer Options > Debug GPU Overdraw > Show Overdraw Areas. Please reduce overdraw so that main content doesn't have any overdraw (as in the other parts of the FRE). Make sure the UI is usable on the smallest of Android devices (320dp x 480dp). You can simulate a small screen on a normal device using "adb shell wm", e.g. "adb shell wm size 270dpx480dp" or "adb shell wm density 800". Once the user scrolls down to the bottom of the confirmation screen we should keep the "ok, got it" button enabled, right? Currently, the button becomes disabled when the user scrolls back up. There's inconsistent scroll handling between the sign-in screen and the confirmation screen. On the sign-in screen, only the list of accounts scrolls, while on the confirmation screen, the entire screen (including "Hi, username") scrolls up. We should be consistent here. The recent tabs page is broken. That needs to be fixed before landing this. (The easiest fix might be to fix bug 583774 - "Move to a text-based recent tabs sign in promo") Follow-up comment. This can be addressed in future CLs: For consistency with the rest of the first run flow, we should probably be using the view pager to transition between the "choose an account" screen and the "hi, username" screen (or at least simulating the sliding action using animations). Currently, clicking "sign in" causes an abrupt jump to the "hi, username" screen, which doesn't feel smooth and is inconsistent with the rest of the FRE flow. This may require some significant changes, so it's OK to do this in a separate CL. https://codereview.chromium.org/1840513002/diff/200001/chrome/android/java/An... File chrome/android/java/AndroidManifest.xml (right): https://codereview.chromium.org/1840513002/diff/200001/chrome/android/java/An... chrome/android/java/AndroidManifest.xml:303: android:configChanges="keyboardHidden|keyboard|screenSize|mcc|mnc|screenLayout|smallestScreenSize"> please leave "orientation" in this list for consistency (likewise for the other activities). It doesn't have any effect, but it makes it simpler (and harder for people to make mistakes) if every activity in Chrome has the same configChanges list. https://codereview.chromium.org/1840513002/diff/200001/chrome/android/java/re... File chrome/android/java/res/layout/account_signin_account_view.xml (right): https://codereview.chromium.org/1840513002/diff/200001/chrome/android/java/re... chrome/android/java/res/layout/account_signin_account_view.xml:7: <LinearLayout nit: use 4-space indentation, instead of 8-space indentation https://codereview.chromium.org/1840513002/diff/200001/chrome/android/java/re... chrome/android/java/res/layout/account_signin_account_view.xml:11: android:layout_gravity="start" layout_gravity determines how this view is positioned within its parent. gravity determines how the children are positioned within this view. Why / do you need this? (I don't think you do.) https://codereview.chromium.org/1840513002/diff/200001/chrome/android/java/re... chrome/android/java/res/layout/account_signin_account_view.xml:19: android:paddingTop="8dp" suggestion: use marginTop/Bottom instead of paddingTop/Bottom for consistency with marginEnd. That'll also make the width and height of the image clearer (currently to get the width of the image you need to subtract 8dp and 8dp from 56dp). https://codereview.chromium.org/1840513002/diff/200001/chrome/android/java/re... chrome/android/java/res/layout/account_signin_account_view.xml:27: android:gravity="center" for clarity, I'd use "center_vertical" instead of "center" https://codereview.chromium.org/1840513002/diff/200001/chrome/android/java/re... chrome/android/java/res/layout/account_signin_account_view.xml:31: <View This view isn't needed. Instead, just set layout_weight="1" on the account_name view (and set layout_width="0dp" on the account_name to avoid layout inefficiencies; see InefficientWeight in the list of Android lint checks: http://tools.android.com/tips/lint-checks) Currently, if the TextView takes up too much space, the checkbox won't even be shown. Moving layout_weight="1" to the TextView will fix this issue. https://codereview.chromium.org/1840513002/diff/200001/chrome/android/java/re... chrome/android/java/res/layout/account_signin_account_view.xml:37: <ImageView You probably want paddingStart="16dp" in case the account email is long (or the device is small) and runs into the checkbox. https://codereview.chromium.org/1840513002/diff/200001/chrome/android/java/re... chrome/android/java/res/layout/account_signin_account_view.xml:41: android:paddingTop="16dp" similarly, use marginTop/Bottom here https://codereview.chromium.org/1840513002/diff/200001/chrome/android/java/re... File chrome/android/java/res/layout/account_signin_choice_description_view.xml (right): https://codereview.chromium.org/1840513002/diff/200001/chrome/android/java/re... chrome/android/java/res/layout/account_signin_choice_description_view.xml:10: android:layout_gravity="start" layout_gravity positions this view within its parent. gravity positions the content within the TextView. Neither is needed here. https://codereview.chromium.org/1840513002/diff/200001/chrome/android/java/re... File chrome/android/java/res/layout/account_signin_view.xml (right): https://codereview.chromium.org/1840513002/diff/200001/chrome/android/java/re... chrome/android/java/res/layout/account_signin_view.xml:7: <org.chromium.chrome.browser.signin.AccountSigninView Since this layout is so long, it would be very helpful to have comments above each major section of the layout. https://codereview.chromium.org/1840513002/diff/200001/chrome/android/java/re... chrome/android/java/res/layout/account_signin_view.xml:14: <LinearLayout Use a FrameLayout here (and remove android:orientation) since it only holds one visible child at a time. FrameLayout is a clearer way of indicating this than a vertical LinearLayout. https://codereview.chromium.org/1840513002/diff/200001/chrome/android/java/re... chrome/android/java/res/layout/account_signin_view.xml:21: android:id="@+id/signin" maybe: "signin_choose_account_page" https://codereview.chromium.org/1840513002/diff/200001/chrome/android/java/re... chrome/android/java/res/layout/account_signin_view.xml:23: android:layout_height="wrap_content" should be match_parent https://codereview.chromium.org/1840513002/diff/200001/chrome/android/java/re... chrome/android/java/res/layout/account_signin_view.xml:27: android:id="@+id/signin_title" it's worth noting that this TextView's height is set dynamically in Java https://codereview.chromium.org/1840513002/diff/200001/chrome/android/java/re... chrome/android/java/res/layout/account_signin_view.xml:30: android:layout_gravity="start" not needed https://codereview.chromium.org/1840513002/diff/200001/chrome/android/java/re... chrome/android/java/res/layout/account_signin_view.xml:31: android:gravity="bottom" Not needed. Please avoid using gravity and layout_gravity unless they're needed. Spurious attributes just add confusion (like stray CSS properties). https://codereview.chromium.org/1840513002/diff/200001/chrome/android/java/re... chrome/android/java/res/layout/account_signin_view.xml:50: android:layout_gravity="start" not needed. layout_gravity="start" is the default. Also, since the width is match_parent, layout_gravity="start" doesn't haven't any effect anyway. https://codereview.chromium.org/1840513002/diff/200001/chrome/android/java/re... chrome/android/java/res/layout/account_signin_view.xml:51: android:paddingStart="16dp" Instead of putting paddingStart and End on the ListView, I'd put paddingStart/End on the items inside the listview. That'll make the overscroll effect look better; currently the overscroll doesn't go to the edge of the screen... it stops 16dp from either side. https://codereview.chromium.org/1840513002/diff/200001/chrome/android/java/re... chrome/android/java/res/layout/account_signin_view.xml:59: <View This view isn't needed. (And if it were needed, then you should change the parent LinearLayout's height to match_parent.) https://codereview.chromium.org/1840513002/diff/200001/chrome/android/java/re... chrome/android/java/res/layout/account_signin_view.xml:70: android:layout_height="wrap_content" should be match_parent https://codereview.chromium.org/1840513002/diff/200001/chrome/android/java/re... chrome/android/java/res/layout/account_signin_view.xml:76: <LinearLayout Use a RelativeLayout here, remove all the LinearLayout children except for signin_confirmation_head (which is needed to provide the background color), and make all the TextViews, ImageViews, etc direct children of this RelativeLayout. This file has significant nesting; anything we can do to reduce nesting will improve runtime performance and memory usage. https://codereview.chromium.org/1840513002/diff/200001/chrome/android/java/re... chrome/android/java/res/layout/account_signin_view.xml:91: android:layout_width="match_parent" Might as well be 0dp https://codereview.chromium.org/1840513002/diff/200001/chrome/android/java/re... chrome/android/java/res/layout/account_signin_view.xml:93: android:layout_weight="1" Setting layout_weight="1" here has no effect, since the parent LinearLayout's height is wrap_content. Thus, this view is useless. https://codereview.chromium.org/1840513002/diff/200001/chrome/android/java/re... chrome/android/java/res/layout/account_signin_view.xml:100: android:layout_gravity="start|bottom" layout_gravity has no effect here https://codereview.chromium.org/1840513002/diff/200001/chrome/android/java/re... chrome/android/java/res/layout/account_signin_view.xml:102: android:scaleType="fitXY" fitCenter is probably better than fitXY. fitXY will distort the image if for some reason it's not square. https://codereview.chromium.org/1840513002/diff/200001/chrome/android/java/re... chrome/android/java/res/layout/account_signin_view.xml:109: android:layout_gravity="start|bottom" again, layout_gravity isn't needed https://codereview.chromium.org/1840513002/diff/200001/chrome/android/java/re... chrome/android/java/res/layout/account_signin_view.xml:118: android:layout_gravity="start|bottom" not needed https://codereview.chromium.org/1840513002/diff/200001/chrome/android/java/re... chrome/android/java/res/layout/account_signin_view.xml:122: </LinearLayout> There should be a divider line between the header and the content. https://codereview.chromium.org/1840513002/diff/200001/chrome/android/java/re... chrome/android/java/res/layout/account_signin_view.xml:133: android:id="@+id/signin_chrome_sync" nit: remove unused IDs (such as this one). That makes it clearer when reading the XML file which views will be modified at runtime and which ones won't. https://codereview.chromium.org/1840513002/diff/200001/chrome/android/java/re... chrome/android/java/res/layout/account_signin_view.xml:145: android:layout_gravity="start" layout_gravity and gravity aren't needed. Same for all the views below. https://codereview.chromium.org/1840513002/diff/200001/chrome/android/java/re... chrome/android/java/res/layout/account_signin_view.xml:221: android:layout_width="match_parent" might as well be 0dp https://codereview.chromium.org/1840513002/diff/200001/chrome/android/java/re... chrome/android/java/res/layout/account_signin_view.xml:230: <!-- layout_height = 52dp --> this comment in no longer useful; remove it https://codereview.chromium.org/1840513002/diff/200001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/signin/AccountListAdapter.java (right): https://codereview.chromium.org/1840513002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountListAdapter.java:19: * Adapter for AccountListView. This comment could be more helpful. For example, what data does this adapter provide? In what context is it used? https://codereview.chromium.org/1840513002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountListAdapter.java:35: * @param profileData ProfileDataCache that will be used to call to retrieve user account info. remove "to call" https://codereview.chromium.org/1840513002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountListAdapter.java:37: public void setProfileDataCache(ProfileDataCache profileData) { Let's pass the ProfileDataCache in the constructor, instead of setting it separately here. Then you can remove the "mProfileData != null" checks. https://codereview.chromium.org/1840513002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountListAdapter.java:45: view = mInflater.inflate(R.layout.account_signin_account_view, null, false); Pass in "parent" instead of "null". That ensures that the correct subclass of LayoutParams is used when inflating account_signin_account_view. https://codereview.chromium.org/1840513002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountListAdapter.java:51: if (accountName.equalsIgnoreCase(mAddAccount)) { We know that the "Add account" item is always last. So, let's use the position to determine whether this is the add account item, instead of comparing the text. https://codereview.chromium.org/1840513002/diff/200001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/signin/AccountListView.java (right): https://codereview.chromium.org/1840513002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountListView.java:14: public class AccountListView extends ListView { This needs to be listed in the gni file. It currently crashes at runtime because this class can't be found. https://codereview.chromium.org/1840513002/diff/200001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninConfirmationView.java (right): https://codereview.chromium.org/1840513002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninConfirmationView.java:30: * or completely invisible to user. nit: capitalize first word of param description, and indent the second line @param isVisible If true, ... or completely... https://codereview.chromium.org/1840513002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninConfirmationView.java:32: void onPersonalizeServiceCheckBoxVisibilityChanged(boolean isVisible); We really only need to notify the observer once: the first time the checkbox becomes visible https://codereview.chromium.org/1840513002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninConfirmationView.java:49: assert MeasureSpec.getMode(widthMeasureSpec) == MeasureSpec.EXACTLY; you should have a similar assert for the height https://codereview.chromium.org/1840513002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninConfirmationView.java:91: public void setObserver(Observer observer) { This name should be clearer. Maybe "setScrolledToBottomObserver()" https://codereview.chromium.org/1840513002/diff/200001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninView.java (right): https://codereview.chromium.org/1840513002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninView.java:144: mSigninView = (View) findViewById(R.id.signin); unnecessary cast https://codereview.chromium.org/1840513002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninView.java:150: mAccountListAdapter = new AccountListAdapter(getContext().getApplicationContext()); Remove "getApplicationContext()". There's no need for it and using the activity context is generally preferable since the activity has a theme applied to it and the application context doesn't (so styles will be broken). https://codereview.chromium.org/1840513002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninView.java:152: mDescriptionText = This variable name is vague. What is it? Can you describe it better, or at least add a comment above the variable's declaration? https://codereview.chromium.org/1840513002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninView.java:159: // events. This allows the ListView to behave reasonably when nested inside a ListView. ?? Why is a ListView nested inside a ListView? What is this referring to? This doesn't seem needed, and if it is, that's a red flag. https://codereview.chromium.org/1840513002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninView.java:183: mPositiveButton = (Button) findViewById(R.id.positive_button); just cast to ButtonCompat here and change mPositiveButton's type to ButtonCompat. https://codereview.chromium.org/1840513002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninView.java:224: assert MeasureSpec.getMode(widthMeasureSpec) == MeasureSpec.EXACTLY; add a similar assert for the heigh https://codereview.chromium.org/1840513002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninView.java:319: mAccountListView.setVisibility(View.VISIBLE); not needed; the list is already/always visible, right? https://codereview.chromium.org/1840513002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninView.java:385: if (mProfileData == null) return; Can mProfileData be null here? If not, make this an assert. If so, we should still show what little information we can (e.g. the user's email address). https://codereview.chromium.org/1840513002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninView.java:472: String accountName = mAccountNames.get(mAccountListAdapter.getSelectedAccountPosition()); I think you should make a small method "getSelectedAccountName()" that returns "mAccountNames.get(mAccountListAdapter.getSelectedAccountPosition())", since you keep repeating the latter code. https://codereview.chromium.org/1840513002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninView.java:602: * Overrides AdapterView.OnItemClickListener. fix indentation, or just use "// Overrides AdapterView.OnItemClickListener" https://codereview.chromium.org/1840513002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninView.java:607: if (mAddAnotherAccount.equalsIgnoreCase(accountNameView.getText().toString())) { use the position or id, not the text, to determine if they clicked "add account" https://codereview.chromium.org/1840513002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninView.java:625: preSelectedAccount.findViewById(R.id.account_selection_mark).setVisibility(View.GONE); No no, this is the wrong way to update items in a list view. Instead, you should call notifyDataSetChanged on the adapter and the views in the list will be regenerated. https://codereview.chromium.org/1840513002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninView.java:638: private void setPositiveButtonActive() { I'd call this "setPositiveButtonEnabled()", to reuse the standard terminology. https://codereview.chromium.org/1840513002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninView.java:639: ((ButtonCompat) mPositiveButton) change the type of mPositiveButton to ButtonCompat so you can avoid these casts. https://codereview.chromium.org/1840513002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninView.java:648: private void setPositiveButtonInActive() { likewise, setPositiveButtonDisabled() https://codereview.chromium.org/1840513002/diff/200001/chrome/android/java/st... File chrome/android/java/strings/android_chrome_strings.grd (right): https://codereview.chromium.org/1840513002/diff/200001/chrome/android/java/st... chrome/android/java/strings/android_chrome_strings.grd:1676: <message name="IDS_SIGNIN_SIGNED_IN_DESCRIPTION_TITLE" desc="The title of showing explanation of what gets synced after the user signed in"> "The title of the explanation..." https://codereview.chromium.org/1840513002/diff/200001/chrome/android/java/st... chrome/android/java/strings/android_chrome_strings.grd:1685: <message name ="IDS_PERSONALIZE_GOOGLE_SERVICE" desc="Content description for the sign in confirmation to option in personalize google services"> s/name =/name=/ https://codereview.chromium.org/1840513002/diff/200001/chrome/android/java/st... chrome/android/java/strings/android_chrome_strings.grd:1685: <message name ="IDS_PERSONALIZE_GOOGLE_SERVICE" desc="Content description for the sign in confirmation to option in personalize google services"> This is not a "content description" (i.e. the text that's read aloud for accessibility users). https://codereview.chromium.org/1840513002/diff/200001/chrome/android/java/st... chrome/android/java/strings/android_chrome_strings.grd:1689: Open myaccount.google.com after signing in Put myaccount.google.com inside a placeholder tag (see other uses of <ph> in this file) to ensure that translators don't change it.
Also, be sure to run tools/resources/optimize-png-files.sh over the image files, if you haven't already.
On 2016/04/01 23:43:12, newt wrote: > Chrome logos can't be checked into Chromium. Please put a Chromium icon in the > chrome/android/java/res_chromium folder, and put the Chrome icon in the internal > res_google_chrome directory. Change the icon name from "chrome_sync_icon" to > something like "app_icon_small". I checked with ewald@ (also cc'd), and the logo here is for the Chrome Sync service, i.e. the servers run by Google that sync both Chromium and Chrome data, so the Chrome logo is appropriate. https://codereview.chromium.org/1840513002/diff/200001/chrome/android/java/st... File chrome/android/java/strings/android_chrome_strings.grd (right): https://codereview.chromium.org/1840513002/diff/200001/chrome/android/java/st... chrome/android/java/strings/android_chrome_strings.grd:1677: Chrome Sync BTW, these strings already exist in chrome/app/generated_resources.grd... where they were added to allow the engineers working on different platforms to use them (see https://codereview.chromium.org/1805633003) 😃
Patchset #5 (id:220001) has been deleted
On 2016/04/04 16:09:16, Bernhard Bauer wrote: > On 2016/04/01 23:43:12, newt wrote: > > Chrome logos can't be checked into Chromium. Please put a Chromium icon in the > > chrome/android/java/res_chromium folder, and put the Chrome icon in the > internal > > res_google_chrome directory. Change the icon name from "chrome_sync_icon" to > > something like "app_icon_small". > > I checked with ewald@ (also cc'd), and the logo here is for the Chrome Sync > service, i.e. the servers run by Google that sync both Chromium and Chrome data, > so the Chrome logo is appropriate. Ok. Are you sure that there aren't trademark issues though? Currently we don't have any copies of the Chrome logo in Chromium, and I think there are some trademark-related reasons for that.
Description was changed from ========== Redesign sign in and sign in confirmation screens for Narnia 2.0 This CL implements new sign in and sign in confirmation screen. It is part of the effort to make Chrome sign in flow support N2.0. The mocks can be find in the issue. Currently we only implement for portrait mode. BUG=595349, 574894 ========== to ========== Redesign sign in and sign in confirmation screens for Narnia 2.0 This CL implements new sign in and sign in confirmation screen. It is part of the effort to make Chrome sign in flow support N2.0. The mocks can be find in the issue. Currently we only implement for portrait mode. BUG=595349, 574894, 598868 ==========
Patchset #6 (id:260001) has been deleted
Patchset #6 (id:280001) has been deleted
Patchset #6 (id:300001) has been deleted
Patchset #7 (id:340001) has been deleted
Patchset #8 (id:380001) has been deleted
Patchset #7 (id:360001) has been deleted
Patchset #7 (id:400001) has been deleted
The CQ bit was checked by gogerald@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1840513002/420001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1840513002/420001
Patchset #7 (id:420001) has been deleted
Patchset #7 (id:440001) has been deleted
The CQ bit was checked by gogerald@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1840513002/460001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1840513002/460001
Chrome logos can't be checked into Chromium. Chrome logo for sync is allowed, refer email by @ewald. Overdraw: Create a bug (Issue 600713) to follow, Make sure the UI is usable on the smallest of Android devices (320dp x 480dp). Done. Once the user scrolls down to the bottom of the confirmation screen we should Done. There's inconsistent scroll handling between the sign-in screen and the... Work as design. The recent tabs page is broken. That needs to be fixed before landing this. (The easiest fix might be to fix bug 583774 - "Move to a text-based recent tabs sign in promo") Fix in followup CL. For consistency with the rest of the first run flow, we should probably be using the view pager to transition between the "choose an account" screen and the "hi, username" screen (or at least simulating the sliding action using animations). May not the case in future since we are re-designing all sign in screens, has a related to do in the code. Notice that sign confirmation view has been changed, we use a clickable text view instead of CheckBox, and the text of the sign in button in sign in view is "CONTINUE" when there are accounts on the device. Here are the mocks https://folio.googleplex.com/chrome-ux-specs-and-sources/Clank%20-%20Material... https://codereview.chromium.org/1840513002/diff/200001/chrome/android/java/An... File chrome/android/java/AndroidManifest.xml (right): https://codereview.chromium.org/1840513002/diff/200001/chrome/android/java/An... chrome/android/java/AndroidManifest.xml:303: android:configChanges="keyboardHidden|keyboard|screenSize|mcc|mnc|screenLayout|smallestScreenSize"> On 2016/04/01 23:43:10, newt wrote: > please leave "orientation" in this list for consistency (likewise for the other > activities). It doesn't have any effect, but it makes it simpler (and harder for > people to make mistakes) if every activity in Chrome has the same configChanges > list. Done. https://codereview.chromium.org/1840513002/diff/200001/chrome/android/java/re... File chrome/android/java/res/layout/account_signin_account_view.xml (right): https://codereview.chromium.org/1840513002/diff/200001/chrome/android/java/re... chrome/android/java/res/layout/account_signin_account_view.xml:7: <LinearLayout On 2016/04/01 23:43:10, newt wrote: > nit: use 4-space indentation, instead of 8-space indentation Done. https://codereview.chromium.org/1840513002/diff/200001/chrome/android/java/re... chrome/android/java/res/layout/account_signin_account_view.xml:11: android:layout_gravity="start" On 2016/04/01 23:43:10, newt wrote: > layout_gravity determines how this view is positioned within its parent. gravity > determines how the children are positioned within this view. Why / do you need > this? (I don't think you do.) Done. https://codereview.chromium.org/1840513002/diff/200001/chrome/android/java/re... chrome/android/java/res/layout/account_signin_account_view.xml:19: android:paddingTop="8dp" On 2016/04/01 23:43:10, newt wrote: > suggestion: use marginTop/Bottom instead of paddingTop/Bottom for consistency > with marginEnd. That'll also make the width and height of the image clearer > (currently to get the width of the image you need to subtract 8dp and 8dp from > 56dp). Done. https://codereview.chromium.org/1840513002/diff/200001/chrome/android/java/re... chrome/android/java/res/layout/account_signin_account_view.xml:27: android:gravity="center" On 2016/04/01 23:43:10, newt wrote: > for clarity, I'd use "center_vertical" instead of "center" Done. https://codereview.chromium.org/1840513002/diff/200001/chrome/android/java/re... chrome/android/java/res/layout/account_signin_account_view.xml:31: <View On 2016/04/01 23:43:10, newt wrote: > This view isn't needed. Instead, just set layout_weight="1" on the account_name > view (and set layout_width="0dp" on the account_name to avoid layout > inefficiencies; see InefficientWeight in the list of Android lint checks: > http://tools.android.com/tips/lint-checks) > > Currently, if the TextView takes up too much space, the checkbox won't even be > shown. Moving layout_weight="1" to the TextView will fix this issue. Done. https://codereview.chromium.org/1840513002/diff/200001/chrome/android/java/re... chrome/android/java/res/layout/account_signin_account_view.xml:37: <ImageView On 2016/04/01 23:43:10, newt wrote: > You probably want paddingStart="16dp" in case the account email is long (or the > device is small) and runs into the checkbox. Done. https://codereview.chromium.org/1840513002/diff/200001/chrome/android/java/re... chrome/android/java/res/layout/account_signin_account_view.xml:41: android:paddingTop="16dp" On 2016/04/01 23:43:10, newt wrote: > similarly, use marginTop/Bottom here Done. https://codereview.chromium.org/1840513002/diff/200001/chrome/android/java/re... File chrome/android/java/res/layout/account_signin_choice_description_view.xml (right): https://codereview.chromium.org/1840513002/diff/200001/chrome/android/java/re... chrome/android/java/res/layout/account_signin_choice_description_view.xml:10: android:layout_gravity="start" On 2016/04/01 23:43:10, newt wrote: > layout_gravity positions this view within its parent. gravity positions the > content within the TextView. Neither is needed here. Done. https://codereview.chromium.org/1840513002/diff/200001/chrome/android/java/re... File chrome/android/java/res/layout/account_signin_view.xml (right): https://codereview.chromium.org/1840513002/diff/200001/chrome/android/java/re... chrome/android/java/res/layout/account_signin_view.xml:7: <org.chromium.chrome.browser.signin.AccountSigninView On 2016/04/01 23:43:10, newt wrote: > Since this layout is so long, it would be very helpful to have comments above > each major section of the layout. Done. https://codereview.chromium.org/1840513002/diff/200001/chrome/android/java/re... chrome/android/java/res/layout/account_signin_view.xml:14: <LinearLayout On 2016/04/01 23:43:10, newt wrote: > Use a FrameLayout here (and remove android:orientation) since it only holds one > visible child at a time. FrameLayout is a clearer way of indicating this than a > vertical LinearLayout. Done. https://codereview.chromium.org/1840513002/diff/200001/chrome/android/java/re... chrome/android/java/res/layout/account_signin_view.xml:21: android:id="@+id/signin" On 2016/04/01 23:43:10, newt wrote: > maybe: "signin_choose_account_page" Done. https://codereview.chromium.org/1840513002/diff/200001/chrome/android/java/re... chrome/android/java/res/layout/account_signin_view.xml:23: android:layout_height="wrap_content" On 2016/04/01 23:43:10, newt wrote: > should be match_parent Done. https://codereview.chromium.org/1840513002/diff/200001/chrome/android/java/re... chrome/android/java/res/layout/account_signin_view.xml:27: android:id="@+id/signin_title" On 2016/04/01 23:43:10, newt wrote: > it's worth noting that this TextView's height is set dynamically in Java Done. https://codereview.chromium.org/1840513002/diff/200001/chrome/android/java/re... chrome/android/java/res/layout/account_signin_view.xml:30: android:layout_gravity="start" On 2016/04/01 23:43:11, newt wrote: > not needed Done. https://codereview.chromium.org/1840513002/diff/200001/chrome/android/java/re... chrome/android/java/res/layout/account_signin_view.xml:31: android:gravity="bottom" On 2016/04/01 23:43:10, newt wrote: > Not needed. Please avoid using gravity and layout_gravity unless they're needed. > Spurious attributes just add confusion (like stray CSS properties). we need this one to put the text at the bottom of the section since it is set to be 16:9 in java https://codereview.chromium.org/1840513002/diff/200001/chrome/android/java/re... chrome/android/java/res/layout/account_signin_view.xml:50: android:layout_gravity="start" On 2016/04/01 23:43:10, newt wrote: > not needed. layout_gravity="start" is the default. Also, since the width is > match_parent, layout_gravity="start" doesn't haven't any effect anyway. Done. https://codereview.chromium.org/1840513002/diff/200001/chrome/android/java/re... chrome/android/java/res/layout/account_signin_view.xml:51: android:paddingStart="16dp" On 2016/04/01 23:43:11, newt wrote: > Instead of putting paddingStart and End on the ListView, I'd put > paddingStart/End on the items inside the listview. That'll make the overscroll > effect look better; currently the overscroll doesn't go to the edge of the > screen... it stops 16dp from either side. Done. https://codereview.chromium.org/1840513002/diff/200001/chrome/android/java/re... chrome/android/java/res/layout/account_signin_view.xml:59: <View On 2016/04/01 23:43:11, newt wrote: > This view isn't needed. (And if it were needed, then you should change the > parent LinearLayout's height to match_parent.) Done. https://codereview.chromium.org/1840513002/diff/200001/chrome/android/java/re... chrome/android/java/res/layout/account_signin_view.xml:70: android:layout_height="wrap_content" On 2016/04/01 23:43:10, newt wrote: > should be match_parent Done. https://codereview.chromium.org/1840513002/diff/200001/chrome/android/java/re... chrome/android/java/res/layout/account_signin_view.xml:76: <LinearLayout On 2016/04/01 23:43:11, newt wrote: > Use a RelativeLayout here, remove all the LinearLayout children except for > signin_confirmation_head (which is needed to provide the background color), and > make all the TextViews, ImageViews, etc direct children of this RelativeLayout. > > This file has significant nesting; anything we can do to reduce nesting will > improve runtime performance and memory usage. Done. https://codereview.chromium.org/1840513002/diff/200001/chrome/android/java/re... chrome/android/java/res/layout/account_signin_view.xml:91: android:layout_width="match_parent" On 2016/04/01 23:43:10, newt wrote: > Might as well be 0dp Done. https://codereview.chromium.org/1840513002/diff/200001/chrome/android/java/re... chrome/android/java/res/layout/account_signin_view.xml:93: android:layout_weight="1" On 2016/04/01 23:43:10, newt wrote: > Setting layout_weight="1" here has no effect, since the parent LinearLayout's > height is wrap_content. Thus, this view is useless. It's useful since we will set the aspect ratio of the head to 16:9 dynamically in code. https://codereview.chromium.org/1840513002/diff/200001/chrome/android/java/re... chrome/android/java/res/layout/account_signin_view.xml:100: android:layout_gravity="start|bottom" On 2016/04/01 23:43:10, newt wrote: > layout_gravity has no effect here Done. https://codereview.chromium.org/1840513002/diff/200001/chrome/android/java/re... chrome/android/java/res/layout/account_signin_view.xml:102: android:scaleType="fitXY" On 2016/04/01 23:43:10, newt wrote: > fitCenter is probably better than fitXY. fitXY will distort the image if for > some reason it's not square. Done. https://codereview.chromium.org/1840513002/diff/200001/chrome/android/java/re... chrome/android/java/res/layout/account_signin_view.xml:109: android:layout_gravity="start|bottom" On 2016/04/01 23:43:11, newt wrote: > again, layout_gravity isn't needed Done. https://codereview.chromium.org/1840513002/diff/200001/chrome/android/java/re... chrome/android/java/res/layout/account_signin_view.xml:118: android:layout_gravity="start|bottom" On 2016/04/01 23:43:10, newt wrote: > not needed Done. https://codereview.chromium.org/1840513002/diff/200001/chrome/android/java/re... chrome/android/java/res/layout/account_signin_view.xml:122: </LinearLayout> On 2016/04/01 23:43:11, newt wrote: > There should be a divider line between the header and the content. Done. https://codereview.chromium.org/1840513002/diff/200001/chrome/android/java/re... chrome/android/java/res/layout/account_signin_view.xml:133: android:id="@+id/signin_chrome_sync" On 2016/04/01 23:43:10, newt wrote: > nit: remove unused IDs (such as this one). That makes it clearer when reading > the XML file which views will be modified at runtime and which ones won't. Done. https://codereview.chromium.org/1840513002/diff/200001/chrome/android/java/re... chrome/android/java/res/layout/account_signin_view.xml:145: android:layout_gravity="start" On 2016/04/01 23:43:11, newt wrote: > layout_gravity and gravity aren't needed. Same for all the views below. Done. https://codereview.chromium.org/1840513002/diff/200001/chrome/android/java/re... chrome/android/java/res/layout/account_signin_view.xml:221: android:layout_width="match_parent" On 2016/04/01 23:43:11, newt wrote: > might as well be 0dp Done. https://codereview.chromium.org/1840513002/diff/200001/chrome/android/java/re... chrome/android/java/res/layout/account_signin_view.xml:230: <!-- layout_height = 52dp --> On 2016/04/01 23:43:10, newt wrote: > this comment in no longer useful; remove it Done. https://codereview.chromium.org/1840513002/diff/200001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/signin/AccountListAdapter.java (right): https://codereview.chromium.org/1840513002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountListAdapter.java:19: * Adapter for AccountListView. On 2016/04/01 23:43:11, newt wrote: > This comment could be more helpful. For example, what data does this adapter > provide? In what context is it used? Done. https://codereview.chromium.org/1840513002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountListAdapter.java:35: * @param profileData ProfileDataCache that will be used to call to retrieve user account info. On 2016/04/01 23:43:11, newt wrote: > remove "to call" Done. https://codereview.chromium.org/1840513002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountListAdapter.java:37: public void setProfileDataCache(ProfileDataCache profileData) { On 2016/04/01 23:43:11, newt wrote: > Let's pass the ProfileDataCache in the constructor, instead of setting it > separately here. Then you can remove the "mProfileData != null" checks. Done. https://codereview.chromium.org/1840513002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountListAdapter.java:45: view = mInflater.inflate(R.layout.account_signin_account_view, null, false); On 2016/04/01 23:43:11, newt wrote: > Pass in "parent" instead of "null". That ensures that the correct subclass of > LayoutParams is used when inflating account_signin_account_view. Done. https://codereview.chromium.org/1840513002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountListAdapter.java:51: if (accountName.equalsIgnoreCase(mAddAccount)) { On 2016/04/01 23:43:11, newt wrote: > We know that the "Add account" item is always last. So, let's use the position > to determine whether this is the add account item, instead of comparing the > text. Done. https://codereview.chromium.org/1840513002/diff/200001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/signin/AccountListView.java (right): https://codereview.chromium.org/1840513002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountListView.java:14: public class AccountListView extends ListView { On 2016/04/01 23:43:11, newt wrote: > This needs to be listed in the gni file. It currently crashes at runtime because > this class can't be found. Done. https://codereview.chromium.org/1840513002/diff/200001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninConfirmationView.java (right): https://codereview.chromium.org/1840513002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninConfirmationView.java:30: * or completely invisible to user. On 2016/04/01 23:43:11, newt wrote: > nit: capitalize first word of param description, and indent the second line > > @param isVisible If true, ... > or completely... Done. https://codereview.chromium.org/1840513002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninConfirmationView.java:32: void onPersonalizeServiceCheckBoxVisibilityChanged(boolean isVisible); On 2016/04/01 23:43:11, newt wrote: > We really only need to notify the observer once: the first time the checkbox > becomes visible Done. https://codereview.chromium.org/1840513002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninConfirmationView.java:49: assert MeasureSpec.getMode(widthMeasureSpec) == MeasureSpec.EXACTLY; On 2016/04/01 23:43:11, newt wrote: > you should have a similar assert for the height Done. https://codereview.chromium.org/1840513002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninConfirmationView.java:91: public void setObserver(Observer observer) { On 2016/04/01 23:43:11, newt wrote: > This name should be clearer. Maybe "setScrolledToBottomObserver()" Done. https://codereview.chromium.org/1840513002/diff/200001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninView.java (right): https://codereview.chromium.org/1840513002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninView.java:144: mSigninView = (View) findViewById(R.id.signin); On 2016/04/01 23:43:11, newt wrote: > unnecessary cast Done. https://codereview.chromium.org/1840513002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninView.java:150: mAccountListAdapter = new AccountListAdapter(getContext().getApplicationContext()); On 2016/04/01 23:43:12, newt wrote: > Remove "getApplicationContext()". There's no need for it and using the activity > context is generally preferable since the activity has a theme applied to it and > the application context doesn't (so styles will be broken). Done. https://codereview.chromium.org/1840513002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninView.java:152: mDescriptionText = On 2016/04/01 23:43:11, newt wrote: > This variable name is vague. What is it? Can you describe it better, or at least > add a comment above the variable's declaration? Done. https://codereview.chromium.org/1840513002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninView.java:159: // events. This allows the ListView to behave reasonably when nested inside a ListView. On 2016/04/01 23:43:11, newt wrote: > ?? Why is a ListView nested inside a ListView? What is this referring to? This > doesn't seem needed, and if it is, that's a red flag. Currently, it has been used in recent tabs view which is a expandable list view, but we have plan to change it, so add a TODO here. https://codereview.chromium.org/1840513002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninView.java:183: mPositiveButton = (Button) findViewById(R.id.positive_button); On 2016/04/01 23:43:12, newt wrote: > just cast to ButtonCompat here and change mPositiveButton's type to > ButtonCompat. Done. https://codereview.chromium.org/1840513002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninView.java:224: assert MeasureSpec.getMode(widthMeasureSpec) == MeasureSpec.EXACTLY; On 2016/04/01 23:43:11, newt wrote: > add a similar assert for the heigh Done. https://codereview.chromium.org/1840513002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninView.java:319: mAccountListView.setVisibility(View.VISIBLE); On 2016/04/01 23:43:12, newt wrote: > not needed; the list is already/always visible, right? Done. https://codereview.chromium.org/1840513002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninView.java:385: if (mProfileData == null) return; On 2016/04/01 23:43:12, newt wrote: > Can mProfileData be null here? If not, make this an assert. If so, we should > still show what little information we can (e.g. the user's email address). Done. https://codereview.chromium.org/1840513002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninView.java:472: String accountName = mAccountNames.get(mAccountListAdapter.getSelectedAccountPosition()); On 2016/04/01 23:43:12, newt wrote: > I think you should make a small method "getSelectedAccountName()" that returns > "mAccountNames.get(mAccountListAdapter.getSelectedAccountPosition())", since you > keep repeating the latter code. Done. https://codereview.chromium.org/1840513002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninView.java:602: * Overrides AdapterView.OnItemClickListener. On 2016/04/01 23:43:12, newt wrote: > fix indentation, or just use "// Overrides AdapterView.OnItemClickListener" Done. https://codereview.chromium.org/1840513002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninView.java:607: if (mAddAnotherAccount.equalsIgnoreCase(accountNameView.getText().toString())) { On 2016/04/01 23:43:12, newt wrote: > use the position or id, not the text, to determine if they clicked "add account" Done. https://codereview.chromium.org/1840513002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninView.java:625: preSelectedAccount.findViewById(R.id.account_selection_mark).setVisibility(View.GONE); On 2016/04/01 23:43:12, newt wrote: > No no, this is the wrong way to update items in a list view. Instead, you should > call notifyDataSetChanged on the adapter and the views in the list will be > regenerated. Done, the reason to update accounts in this way is for efficiency, but considering number of accounts is small, let's keep it simple. https://codereview.chromium.org/1840513002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninView.java:638: private void setPositiveButtonActive() { On 2016/04/01 23:43:12, newt wrote: > I'd call this "setPositiveButtonEnabled()", to reuse the standard terminology. Done. https://codereview.chromium.org/1840513002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninView.java:639: ((ButtonCompat) mPositiveButton) On 2016/04/01 23:43:11, newt wrote: > change the type of mPositiveButton to ButtonCompat so you can avoid these casts. Done. https://codereview.chromium.org/1840513002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninView.java:648: private void setPositiveButtonInActive() { On 2016/04/01 23:43:11, newt wrote: > likewise, setPositiveButtonDisabled() Done. https://codereview.chromium.org/1840513002/diff/200001/chrome/android/java/st... File chrome/android/java/strings/android_chrome_strings.grd (right): https://codereview.chromium.org/1840513002/diff/200001/chrome/android/java/st... chrome/android/java/strings/android_chrome_strings.grd:1676: <message name="IDS_SIGNIN_SIGNED_IN_DESCRIPTION_TITLE" desc="The title of showing explanation of what gets synced after the user signed in"> On 2016/04/01 23:43:12, newt wrote: > "The title of the explanation..." Done. use cross platform string. https://codereview.chromium.org/1840513002/diff/200001/chrome/android/java/st... chrome/android/java/strings/android_chrome_strings.grd:1677: Chrome Sync On 2016/04/04 16:09:16, Bernhard Bauer wrote: > BTW, these strings already exist in chrome/app/generated_resources.grd... where > they were added to allow the engineers working on different platforms to use > them (see https://codereview.chromium.org/1805633003) 😃 Done.use cross platform string. https://codereview.chromium.org/1840513002/diff/200001/chrome/android/java/st... chrome/android/java/strings/android_chrome_strings.grd:1685: <message name ="IDS_PERSONALIZE_GOOGLE_SERVICE" desc="Content description for the sign in confirmation to option in personalize google services"> On 2016/04/01 23:43:12, newt wrote: > This is not a "content description" (i.e. the text that's read aloud for > accessibility users). Done. https://codereview.chromium.org/1840513002/diff/200001/chrome/android/java/st... chrome/android/java/strings/android_chrome_strings.grd:1685: <message name ="IDS_PERSONALIZE_GOOGLE_SERVICE" desc="Content description for the sign in confirmation to option in personalize google services"> On 2016/04/01 23:43:12, newt wrote: > This is not a "content description" (i.e. the text that's read aloud for > accessibility users). Done.use cross platform string. https://codereview.chromium.org/1840513002/diff/200001/chrome/android/java/st... chrome/android/java/strings/android_chrome_strings.grd:1689: Open myaccount.google.com after signing in On 2016/04/01 23:43:12, newt wrote: > Put http://myaccount.google.com inside a placeholder tag (see other uses of <ph> in > this file) to ensure that translators don't change it. > Done.use cross platform string.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1840513002/diff/200001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninView.java (right): https://codereview.chromium.org/1840513002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninView.java:159: // events. This allows the ListView to behave reasonably when nested inside a ListView. On 2016/04/12 01:21:13, gogerald1 wrote: > On 2016/04/01 23:43:11, newt wrote: > > ?? Why is a ListView nested inside a ListView? What is this referring to? This > > doesn't seem needed, and if it is, that's a red flag. > > Currently, it has been used in recent tabs view which is a expandable list view, > but we have plan to change it, so add a TODO here. Thanks for explaining. And yes, please remove this soon :) https://codereview.chromium.org/1840513002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninView.java:625: preSelectedAccount.findViewById(R.id.account_selection_mark).setVisibility(View.GONE); On 2016/04/12 01:21:13, gogerald1 wrote: > On 2016/04/01 23:43:12, newt wrote: > > No no, this is the wrong way to update items in a list view. Instead, you > should > > call notifyDataSetChanged on the adapter and the views in the list will be > > regenerated. > > Done, the reason to update accounts in this way is for efficiency, but > considering number of accounts is small, let's keep it simple. You know what they say about premature optimization... :) In this case, I suspect, the majority of time is dominated by the extra, unavoidable measure/layout/draw pass (which recurses over the entire visible view hierarchy). Asking the adapter to update itself is probably only a tiny fraction of that time. https://codereview.chromium.org/1840513002/diff/460001/chrome/android/java/re... File chrome/android/java/res/layout/account_signin_account_view.xml (right): https://codereview.chromium.org/1840513002/diff/460001/chrome/android/java/re... chrome/android/java/res/layout/account_signin_account_view.xml:16: android:id="@+id/account_image" use spaces, not tabs https://codereview.chromium.org/1840513002/diff/460001/chrome/android/java/re... chrome/android/java/res/layout/account_signin_account_view.xml:27: android:layout_height="56dp" What happens if the email address is really long? Does it wrap to a second line? We might want to set ellipsize="end" and maxLines=1 instead. Alternatively, you could set layout_height="wrap_content" and minHeight="56dp", then also set gravity="center_vertical" on the parent LinearLayout. That'll allow the UI to adjust nicely when the email wraps to two lines. (This would also allow you to remove the marginTop and marginBottom attributes from the two ImageViews, since those views will simply be centered vertically.) https://codereview.chromium.org/1840513002/diff/460001/chrome/android/java/re... File chrome/android/java/res/layout/account_signin_choice_description_view.xml (right): https://codereview.chromium.org/1840513002/diff/460001/chrome/android/java/re... chrome/android/java/res/layout/account_signin_choice_description_view.xml:10: android:paddingTop="16dp" FYI, you could write android:padding="16dp" instead to set the padding for all sides. Either way is fine. https://codereview.chromium.org/1840513002/diff/460001/chrome/android/java/re... File chrome/android/java/res/layout/account_signin_view.xml (right): https://codereview.chromium.org/1840513002/diff/460001/chrome/android/java/re... chrome/android/java/res/layout/account_signin_view.xml:19: <!-- The view allow user to choose account to sign in --> nit: fix grammar in comment (and same below) https://codereview.chromium.org/1840513002/diff/460001/chrome/android/java/re... chrome/android/java/res/layout/account_signin_view.xml:20: <LinearLayout remove trailing space https://codereview.chromium.org/1840513002/diff/460001/chrome/android/java/re... chrome/android/java/res/layout/account_signin_view.xml:66: remove trailing space https://codereview.chromium.org/1840513002/diff/460001/chrome/android/java/re... chrome/android/java/res/layout/account_signin_view.xml:82: <View Ah, I see why this was needed. You can actually remove this view and set gravity="bottom" on the parent LinearLayout. https://codereview.chromium.org/1840513002/diff/460001/chrome/android/java/re... chrome/android/java/res/layout/account_signin_view.xml:202: android:id="@+id/signin_confirmation_bottom_padding" I'd remove this view and just add 36dp of bottom padding to the parent RelativeLayout. https://codereview.chromium.org/1840513002/diff/460001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninConfirmationView.java (right): https://codereview.chromium.org/1840513002/diff/460001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninConfirmationView.java:32: void onScrolledToBottom(boolean scrolledToBottom); I'd remove the boolean parameter altogether, since we're only calling this once and scrolledToBottom will always be true when this is called. Also, mention in the comment that this is called only once. https://codereview.chromium.org/1840513002/diff/460001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninConfirmationView.java:90: private void checkAndNotifyScrolledToBottom(boolean forceNotify) { maybe call this "notifyIfScrolledToBottom()" https://codereview.chromium.org/1840513002/diff/460001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninConfirmationView.java:95: boolean isBottomVisible = true; We don't need the isBottomVisible variable anymore, since we're only notifying when at the bottom. This could simply be: if (!forceNotify && mScrolledToBottom) return; int distance = ...; int bottomPadding = ...; if (distance <= bottomPadding) { mObserver.onScrolledToBottom(); mScrolledToBottom = true; } https://codereview.chromium.org/1840513002/diff/460001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninView.java (right): https://codereview.chromium.org/1840513002/diff/460001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninView.java:150: View signinChoiceDescription = nit: unnecessary wrapping https://codereview.chromium.org/1840513002/diff/460001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninView.java:204: setPositiveButtonDisabled(); Why disable it again? Once we've scrolled to the bottom once, the button should stay enabled. https://codereview.chromium.org/1840513002/diff/460001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninView.java:370: private void updateSignedInAccountInfo() { This used to check "if (mProfileData != null)". Is it safe to remove this check?
The CQ bit was checked by gogerald@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1840513002/480001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1840513002/480001
https://codereview.chromium.org/1840513002/diff/460001/chrome/android/java/re... File chrome/android/java/res/layout/account_signin_account_view.xml (right): https://codereview.chromium.org/1840513002/diff/460001/chrome/android/java/re... chrome/android/java/res/layout/account_signin_account_view.xml:16: android:id="@+id/account_image" On 2016/04/13 18:13:30, newt (away) wrote: > use spaces, not tabs Done. Used four spaces instead of a tab, it may be changed by my editor (sublime). https://codereview.chromium.org/1840513002/diff/460001/chrome/android/java/re... chrome/android/java/res/layout/account_signin_account_view.xml:27: android:layout_height="56dp" On 2016/04/13 18:13:30, newt (away) wrote: > What happens if the email address is really long? Does it wrap to a second line? > We might want to set ellipsize="end" and maxLines=1 instead. > > Alternatively, you could set layout_height="wrap_content" and minHeight="56dp", > then also set gravity="center_vertical" on the parent LinearLayout. That'll > allow the UI to adjust nicely when the email wraps to two lines. (This would > also allow you to remove the marginTop and marginBottom attributes from the two > ImageViews, since those views will simply be centered vertically.) Done. https://codereview.chromium.org/1840513002/diff/460001/chrome/android/java/re... File chrome/android/java/res/layout/account_signin_choice_description_view.xml (right): https://codereview.chromium.org/1840513002/diff/460001/chrome/android/java/re... chrome/android/java/res/layout/account_signin_choice_description_view.xml:10: android:paddingTop="16dp" On 2016/04/13 18:13:30, newt (away) wrote: > FYI, you could write android:padding="16dp" instead to set the padding for all > sides. Either way is fine. Done. https://codereview.chromium.org/1840513002/diff/460001/chrome/android/java/re... File chrome/android/java/res/layout/account_signin_view.xml (right): https://codereview.chromium.org/1840513002/diff/460001/chrome/android/java/re... chrome/android/java/res/layout/account_signin_view.xml:19: <!-- The view allow user to choose account to sign in --> On 2016/04/13 18:13:30, newt (away) wrote: > nit: fix grammar in comment (and same below) Done. https://codereview.chromium.org/1840513002/diff/460001/chrome/android/java/re... chrome/android/java/res/layout/account_signin_view.xml:20: <LinearLayout On 2016/04/13 18:13:30, newt (away) wrote: > remove trailing space Done. https://codereview.chromium.org/1840513002/diff/460001/chrome/android/java/re... chrome/android/java/res/layout/account_signin_view.xml:66: On 2016/04/13 18:13:30, newt (away) wrote: > remove trailing space Done. https://codereview.chromium.org/1840513002/diff/460001/chrome/android/java/re... chrome/android/java/res/layout/account_signin_view.xml:82: <View On 2016/04/13 18:13:30, newt (away) wrote: > Ah, I see why this was needed. You can actually remove this view and set > gravity="bottom" on the parent LinearLayout. Done. https://codereview.chromium.org/1840513002/diff/460001/chrome/android/java/re... chrome/android/java/res/layout/account_signin_view.xml:202: android:id="@+id/signin_confirmation_bottom_padding" On 2016/04/13 18:13:30, newt (away) wrote: > I'd remove this view and just add 36dp of bottom padding to the parent > RelativeLayout. Done.pad to above text view. https://codereview.chromium.org/1840513002/diff/460001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninConfirmationView.java (right): https://codereview.chromium.org/1840513002/diff/460001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninConfirmationView.java:32: void onScrolledToBottom(boolean scrolledToBottom); On 2016/04/13 18:13:30, newt (away) wrote: > I'd remove the boolean parameter altogether, since we're only calling this once > and scrolledToBottom will always be true when this is called. Also, mention in > the comment that this is called only once. Done. https://codereview.chromium.org/1840513002/diff/460001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninConfirmationView.java:90: private void checkAndNotifyScrolledToBottom(boolean forceNotify) { On 2016/04/13 18:13:30, newt (away) wrote: > maybe call this "notifyIfScrolledToBottom()" Done. https://codereview.chromium.org/1840513002/diff/460001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninConfirmationView.java:95: boolean isBottomVisible = true; On 2016/04/13 18:13:30, newt (away) wrote: > We don't need the isBottomVisible variable anymore, since we're only notifying > when at the bottom. This could simply be: > > if (!forceNotify && mScrolledToBottom) return; > > int distance = ...; > int bottomPadding = ...; > > if (distance <= bottomPadding) { > mObserver.onScrolledToBottom(); > mScrolledToBottom = true; > } > > Done. https://codereview.chromium.org/1840513002/diff/460001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninView.java (right): https://codereview.chromium.org/1840513002/diff/460001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninView.java:150: View signinChoiceDescription = On 2016/04/13 18:13:31, newt (away) wrote: > nit: unnecessary wrapping It looks a little more readable. https://codereview.chromium.org/1840513002/diff/460001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninView.java:204: setPositiveButtonDisabled(); On 2016/04/13 18:13:31, newt (away) wrote: > Why disable it again? Once we've scrolled to the bottom once, the button should > stay enabled. Previously, the default state is scrolled to bottom, so I need change it at the beginning. Now, I no need to do this since the default state is not scrolled to bottom. https://codereview.chromium.org/1840513002/diff/460001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninView.java:370: private void updateSignedInAccountInfo() { On 2016/04/13 18:13:30, newt (away) wrote: > This used to check "if (mProfileData != null)". Is it safe to remove this check? yes, since updateProfileImages has checked it and showConfirmSigninPage is after showSigninPage.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm after one last comment :) https://codereview.chromium.org/1840513002/diff/460001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninView.java (right): https://codereview.chromium.org/1840513002/diff/460001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninView.java:150: View signinChoiceDescription = On 2016/04/13 21:33:13, gogerald1 wrote: > On 2016/04/13 18:13:31, newt (away) wrote: > > nit: unnecessary wrapping > > It looks a little more readable. Agree to disagree :) This is fine. https://codereview.chromium.org/1840513002/diff/480001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninConfirmationView.java (right): https://codereview.chromium.org/1840513002/diff/480001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninConfirmationView.java:97: mScrolledToBottom = false; this line isn't needed
https://codereview.chromium.org/1840513002/diff/480001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninConfirmationView.java (right): https://codereview.chromium.org/1840513002/diff/480001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninConfirmationView.java:97: mScrolledToBottom = false; On 2016/04/14 02:04:54, newt (away) wrote: > this line isn't needed This is needed because the user may click "undo" to go back to sign in screen, then click "continue" to confirm sign in account again (may or may not change the sign in account). In this case, we should disable "OK, got it" button if the bottom is not visible according to my discussion with Eli and Alan.
The CQ bit was checked by gogerald@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1840513002/480001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1840513002/480001
The CQ bit was unchecked by gogerald@chromium.org
The CQ bit was checked by gogerald@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1840513002/480001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1840513002/480001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
LGTM w/ nits: https://codereview.chromium.org/1840513002/diff/480001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/signin/AccountListAdapter.java (right): https://codereview.chromium.org/1840513002/diff/480001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountListAdapter.java:23: private LayoutInflater mInflater; Can you make this and |mProfileData| final? https://codereview.chromium.org/1840513002/diff/480001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninView.java (right): https://codereview.chromium.org/1840513002/diff/480001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninView.java:42: // TODO(gogerald): add lanscape mode (http://crbug.com/599139). Nit: "landscape" https://codereview.chromium.org/1840513002/diff/480001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninView.java:43: // TODO(gogerald): refactor common part into one place after redesgin all sign in screens. Nit: "redesign"
The CQ bit was checked by gogerald@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1840513002/500001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1840513002/500001
https://codereview.chromium.org/1840513002/diff/480001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/signin/AccountListAdapter.java (right): https://codereview.chromium.org/1840513002/diff/480001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountListAdapter.java:23: private LayoutInflater mInflater; On 2016/04/14 16:39:59, Bernhard Bauer wrote: > Can you make this and |mProfileData| final? Done. https://codereview.chromium.org/1840513002/diff/480001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninView.java (right): https://codereview.chromium.org/1840513002/diff/480001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninView.java:42: // TODO(gogerald): add lanscape mode (http://crbug.com/599139). On 2016/04/14 16:39:59, Bernhard Bauer wrote: > Nit: "landscape" Done. https://codereview.chromium.org/1840513002/diff/480001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninView.java:43: // TODO(gogerald): refactor common part into one place after redesgin all sign in screens. On 2016/04/14 16:39:59, Bernhard Bauer wrote: > Nit: "redesign" Done.
The CQ bit was unchecked by gogerald@chromium.org
The CQ bit was checked by gogerald@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org, newt@chromium.org Link to the patchset: https://codereview.chromium.org/1840513002/#ps500001 (title: "address comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1840513002/500001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1840513002/500001
Message was sent while issue was closed.
Description was changed from ========== Redesign sign in and sign in confirmation screens for Narnia 2.0 This CL implements new sign in and sign in confirmation screen. It is part of the effort to make Chrome sign in flow support N2.0. The mocks can be find in the issue. Currently we only implement for portrait mode. BUG=595349, 574894, 598868 ========== to ========== Redesign sign in and sign in confirmation screens for Narnia 2.0 This CL implements new sign in and sign in confirmation screen. It is part of the effort to make Chrome sign in flow support N2.0. The mocks can be find in the issue. Currently we only implement for portrait mode. BUG=595349, 574894, 598868 ==========
Message was sent while issue was closed.
Committed patchset #9 (id:500001)
Message was sent while issue was closed.
Description was changed from ========== Redesign sign in and sign in confirmation screens for Narnia 2.0 This CL implements new sign in and sign in confirmation screen. It is part of the effort to make Chrome sign in flow support N2.0. The mocks can be find in the issue. Currently we only implement for portrait mode. BUG=595349, 574894, 598868 ========== to ========== Redesign sign in and sign in confirmation screens for Narnia 2.0 This CL implements new sign in and sign in confirmation screen. It is part of the effort to make Chrome sign in flow support N2.0. The mocks can be find in the issue. Currently we only implement for portrait mode. BUG=595349, 574894, 598868 Committed: https://crrev.com/e8c60fdcbfdf07b55a671b818c52b950436429b1 Cr-Commit-Position: refs/heads/master@{#387359} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/e8c60fdcbfdf07b55a671b818c52b950436429b1 Cr-Commit-Position: refs/heads/master@{#387359} |
