Redesign sign in and sign in confirmation screens for Narnia 2.0

chrome/android/java/res/layout/account_signin_view.xml

Index: chrome/android/java/res/layout/account_signin_view.xml
diff --git a/chrome/android/java/res/layout/account_signin_view.xml b/chrome/android/java/res/layout/account_signin_view.xml
index 41288f0a43c4d672fbf5eb9711bc0d94a442d92e..3f24a83086b3ed801ca6879147d21484cbb498e5 100644
--- a/chrome/android/java/res/layout/account_signin_view.xml
+++ b/chrome/android/java/res/layout/account_signin_view.xml
@@ -8,97 +8,234 @@
     xmlns:android="http://schemas.android.com/apk/res/android"
     xmlns:chrome="http://schemas.android.com/apk/res-auto"
     android:layout_width="match_parent"
-    android:layout_height="match_parent">
+    android:layout_height="match_parent"
+    android:background="@color/signin_body_background">
 
-    <ScrollView
+    <LinearLayout

[newt (away), 2016/04/01 23:43:10]

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.

[gogerald1, 2016/04/12 01:21:11]

Done.

         android:layout_width="match_parent"
         android:layout_height="match_parent"
-        android:layout_marginBottom="72dp"
-        android:fillViewport="true">
+        android:layout_marginBottom="52dp"
+        android:orientation="vertical">
 
         <LinearLayout
-            android:id="@+id/fre_main_layout"
+            android:id="@+id/signin"

[newt (away), 2016/04/01 23:43:10]

maybe: "signin_choose_account_page"

[gogerald1, 2016/04/12 01:21:11]

Done.

             android:layout_width="match_parent"
             android:layout_height="wrap_content"

[newt (away), 2016/04/01 23:43:10]

should be match_parent

[gogerald1, 2016/04/12 01:21:11]

Done.

-            android:orientation="vertical"
-            android:gravity="center_horizontal">
+            android:orientation="vertical">
 
             <TextView
-                android:id="@+id/title"
-                android:text="@string/signin_set_up_chrome"
-                style="@style/FreTitle"/>
-            
-            <!-- The orientation of this view is changed dynamically to give a nicer layout when in
-            landscape mode on devices with small screens. -->
-            <LinearLayout
-                android:id="@+id/fre_image_and_content"
+                android:id="@+id/signin_title"

[newt (away), 2016/04/01 23:43:10]

it's worth noting that this TextView's height is set dynamically in Java

[gogerald1, 2016/04/12 01:21:11]

Done.

+                android:layout_width="match_parent"
+                android:layout_height="wrap_content"
+                android:layout_gravity="start"

[newt (away), 2016/04/01 23:43:11]

not needed

[gogerald1, 2016/04/12 01:21:12]

Done.

+                android:gravity="bottom"

[newt (away), 2016/04/01 23:43:10]

Not needed. Please avoid using gravity and layout_gravity unless they're needed.
Spurious attributes just add confusion (like stray CSS properties).

[gogerald1, 2016/04/12 01:21:12]

we need this one to put the text at the bottom of the section since it is set to
be 16:9 in java

+                android:paddingStart="16dp"
+                android:paddingEnd="16dp" 
+                android:paddingBottom="16dp"
+                android:background="@color/signin_head_background"
+                android:textColor="@color/default_text_color"
+                android:textSize="@dimen/fre_title_text_size"
+                android:text="@string/sign_in_to_chrome"/>
+
+            <View 
+                android:layout_width="match_parent"
+                android:layout_height="1dp"
+                android:background="@color/signin_border_line_color"
+                android:alpha="0.08"/>
+
+            <org.chromium.chrome.browser.signin.AccountListView 
+                android:id="@+id/signin_account_list"
                 android:layout_width="match_parent"
                 android:layout_height="wrap_content"
-                android:orientation="vertical"
-                android:gravity="center_horizontal"
-                android:layout_marginTop="@dimen/fre_vertical_spacing">
+                android:layout_gravity="start"

[newt (away), 2016/04/01 23:43:10]

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.

[gogerald1, 2016/04/12 01:21:12]

Done.

+                android:paddingStart="16dp"

[newt (away), 2016/04/01 23:43:11]

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.

[gogerald1, 2016/04/12 01:21:11]

Done.

+                android:paddingEnd="16dp"
+                android:divider="@null"
+                android:dividerHeight="0dp"
+                android:scrollbars="none"
+                android:requiresFadingEdge="vertical"
+                android:fadingEdgeLength="48dp"/>
+
+            <View

[newt (away), 2016/04/01 23:43:11]

This view isn't needed. (And if it were needed, then you should change the
parent LinearLayout's height to match_parent.)

[gogerald1, 2016/04/12 01:21:11]

Done.

+                android:layout_width="match_parent"
+                android:layout_height="0dp"
+                android:layout_weight="1"
+                android:visibility="invisible"/>
+
+        </LinearLayout>
 
-                <org.chromium.chrome.browser.firstrun.ImageCarousel
-                    android:id="@+id/image"
-                    android:layout_width="@dimen/signin_image_carousel_width"
-                    android:layout_height="@dimen/fre_image_height" />
+        <org.chromium.chrome.browser.signin.AccountSigninConfirmationView
+            android:id="@+id/signin_confirmation_layout"
+            android:layout_width="match_parent"
+            android:layout_height="wrap_content"

[newt (away), 2016/04/01 23:43:10]

should be match_parent

[gogerald1, 2016/04/12 01:21:11]

Done.

+            android:scrollbars="none"
+            android:requiresFadingEdge="vertical"
+            android:fadingEdgeLength="48dp"
+            android:visibility="gone">
+
+            <LinearLayout

[newt (away), 2016/04/01 23:43:11]

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.

[gogerald1, 2016/04/12 01:21:12]

Done.

+                android:layout_width="match_parent"
+                android:layout_height="wrap_content"
+                android:orientation="vertical">
 
                 <LinearLayout
-                    android:id="@+id/fre_content_wrapper"
-                    android:layout_width="0dp"
-                    android:layout_height="0dp"
-                    android:layout_weight="1"
-                    android:layout_marginTop="@dimen/fre_vertical_spacing"
-                    android:layout_marginEnd="@dimen/fre_content_margin"
-                    android:layout_marginStart="@dimen/fre_content_margin"
-                    android:orientation="vertical" >
-
-                    <Spinner
-                        style="@style/Widget.AppCompat.Spinner.Underlined"
-                        android:id="@+id/google_accounts_spinner"
-                        android:layout_width="wrap_content"
+                    android:id="@+id/signin_confirmation_head"
+                    android:layout_width="match_parent"
+                    android:layout_height="wrap_content"
+                    android:paddingStart="16dp"
+                    android:paddingEnd="16dp"
+                    android:background="@color/signin_head_background"
+                    android:orientation="vertical">
+
+                    <View
+                        android:layout_width="match_parent"

[newt (away), 2016/04/01 23:43:10]

Might as well be 0dp

[gogerald1, 2016/04/12 01:21:12]

Done.

+                        android:layout_height="0dp"
+                        android:layout_weight="1"

[newt (away), 2016/04/01 23:43:10]

Setting layout_weight="1" here has no effect, since the parent LinearLayout's
height is wrap_content. Thus, this view is useless.

[gogerald1, 2016/04/12 01:21:12]

It's useful since we will set the aspect ratio of the head to 16:9 dynamically
in code.

+                        android:visibility="invisible"/>
+
+                    <ImageView
+                        android:id="@+id/signin_account_image"
+                        android:layout_width="64dp"
+                        android:layout_height="64dp"
+                        android:layout_gravity="start|bottom"

[newt (away), 2016/04/01 23:43:10]

layout_gravity has no effect here

[gogerald1, 2016/04/12 01:21:11]

Done.

+                        android:layout_marginBottom="24dp"
+                        android:scaleType="fitXY"

[newt (away), 2016/04/01 23:43:10]

fitCenter is probably better than fitXY. fitXY will distort the image if for
some reason it's not square.

[gogerald1, 2016/04/12 01:21:11]

Done.

+                        android:contentDescription="@null"/>
+
+                    <TextView
+                        android:id="@+id/signin_account_name"
+                        android:layout_width="match_parent"
                         android:layout_height="wrap_content"
-                        android:layout_marginBottom="@dimen/fre_vertical_spacing"
-                        android:layout_gravity="center_horizontal"
-                        android:popupBackground="#ffffff"
-                        android:textColor="@color/fre_text_color" />
+                        android:layout_gravity="start|bottom"

[newt (away), 2016/04/01 23:43:11]

again, layout_gravity isn't needed

[gogerald1, 2016/04/12 01:21:11]

Done.

+                        android:layout_marginBottom="8dp"
+                        android:textColor="@color/default_text_color"
+                        android:textSize="@dimen/fre_title_text_size"/>
 
                     <TextView
-                        android:id="@+id/confirm_account_email"
-                        android:layout_width="wrap_content"
+                        android:id="@+id/signin_account_email"
+                        android:layout_width="match_parent"
                         android:layout_height="wrap_content"
-                        android:layout_gravity="center"
+                        android:layout_gravity="start|bottom"

[newt (away), 2016/04/01 23:43:10]

not needed

[gogerald1, 2016/04/12 01:21:11]

Done.

                         android:layout_marginBottom="16dp"
-                        android:paddingTop="12dp"
-                        android:paddingBottom="8dp"
-                        android:textColor="@color/fre_text_color"
-                        android:visibility="gone" />
-
-                    <org.chromium.ui.widget.TextViewWithClickableSpans
-                        android:id="@+id/description"
-                        android:layout_width="wrap_content"
+                        android:textColor="@color/descriptive_text_color"
+                        android:textSize="@dimen/fre_normal_text_size"/>
+                </LinearLayout>

[newt (away), 2016/04/01 23:43:11]

There should be a divider line between the header and the content.

[gogerald1, 2016/04/12 01:21:11]

Done.

+
+                <LinearLayout
+                    android:id="@+id/signin_confirmation_content"
+                    android:layout_width="match_parent"
+                    android:layout_height="wrap_content"
+                    android:paddingStart="16dp"
+                    android:paddingEnd="16dp"
+                    android:orientation="vertical">
+                    
+                    <LinearLayout
+                        android:id="@+id/signin_chrome_sync"

[newt (away), 2016/04/01 23:43:10]

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.

[gogerald1, 2016/04/12 01:21:12]

Done.

+                        android:layout_width="match_parent"
                         android:layout_height="wrap_content"
-                        android:gravity="center"
-                        android:lineSpacingMultiplier="1.4"
-                        android:text="@string/signin_account_choice_description"
-                        android:textColor="@color/signin_light_text_color"
-                        android:textSize="@dimen/fre_normal_text_size" />
+                        android:layout_marginTop="16dp"
+                        android:layout_marginBottom="16dp"
+                        android:orientation="vertical">
+
+                        <TextView
+                            android:id="@+id/signin_chrome_sync_title"
+                            android:layout_width="match_parent"
+                            android:layout_height="wrap_content"
+                            android:layout_marginBottom="8dp"
+                            android:layout_gravity="start"

[newt (away), 2016/04/01 23:43:11]

layout_gravity and gravity aren't needed. Same for all the views below.

[gogerald1, 2016/04/12 01:21:12]

Done.

+                            android:gravity="start"
+                            android:text="@string/signin_signed_in_description_title"
+                            android:textColor="@color/default_text_color"
+                            android:textSize="@dimen/fre_normal_text_size"
+                            android:drawableStart="@drawable/chrome_sync_logo"
+                            android:drawablePadding="16dp"/>
+
+                        <org.chromium.ui.widget.TextViewWithClickableSpans
+                            android:id="@+id/signin_chrome_sync_description"
+                            android:layout_width="match_parent"
+                            android:layout_height="wrap_content"
+                            android:layout_marginStart="32dp"
+                            android:layout_gravity="start"
+                            android:gravity="start"
+                            android:lineSpacingMultiplier="1.4"
+                            android:text="@string/signin_signed_in_description"
+                            android:textColor="@color/descriptive_text_color"
+                            android:textSize="@dimen/fre_normal_text_size" />
                     </LinearLayout>
-                    
+
+                    <View 
+                        android:layout_width="match_parent"
+                        android:layout_height="1dp"
+                        android:layout_marginStart="32dp"
+                        android:background="@color/signin_border_line_color"
+                        android:alpha="0.08"/>
+
+                    <LinearLayout
+                        android:id="@+id/signin_personalize_service"
+                        android:layout_width="match_parent"
+                        android:layout_height="wrap_content"
+                        android:layout_marginTop="16dp"
+                        android:orientation="vertical">
+
+                        <TextView
+                            android:id="@+id/signin_personalize_service_title"
+                            android:layout_width="match_parent"
+                            android:layout_height="wrap_content"
+                            android:layout_marginBottom="8dp"
+                            android:layout_gravity="start"
+                            android:gravity="start"
+                            android:text="@string/personalize_google_service"
+                            android:textColor="@color/default_text_color"
+                            android:textSize="@dimen/fre_normal_text_size"
+                            android:drawableStart="@drawable/personalize_google_service_logo"
+                            android:drawablePadding="16dp"/>
+
+                        <TextView
+                            android:id="@+id/signin_personalize_service_description"
+                            android:layout_width="match_parent"
+                            android:layout_height="wrap_content"
+                            android:layout_marginStart="32dp"
+                            android:layout_marginBottom="16dp"
+                            android:layout_gravity="start"
+                            android:gravity="start"
+                            android:lineSpacingMultiplier="1.4"
+                            android:text="@string/signin_account_choice_description"
+                            android:textColor="@color/descriptive_text_color"
+                            android:textSize="@dimen/fre_normal_text_size"/>
+
+                        <CheckBox
+                            android:id="@+id/signin_personalize_service_checkbox"
+                            android:layout_width="wrap_content"
+                            android:layout_height="wrap_content"
+                            android:layout_marginStart="27dp"
+                            android:layout_gravity="start"
+                            android:paddingStart="11dp"
+                            android:gravity="start"
+                            android:lineSpacingMultiplier="1.4"
+                            android:text="@string/open_myaccount_after_sign_in"
+                            android:textColor="@color/descriptive_text_color"
+                            android:textSize="@dimen/fre_normal_text_size" />
+
+                        <View
+                            android:id="@+id/signin_confirmation_bottom_padding"
+                            android:layout_width="match_parent"

[newt (away), 2016/04/01 23:43:11]

might as well be 0dp

[gogerald1, 2016/04/12 01:21:11]

Done.

+                            android:layout_height="36dp"
+                            android:visibility="invisible"/>
+                    </LinearLayout>
+                </LinearLayout>
             </LinearLayout>
-        </LinearLayout>
-    </ScrollView>
+        </org.chromium.chrome.browser.signin.AccountSigninConfirmationView>
+    </LinearLayout>
 
-    <!-- layout_height = 72dp = fre_button_bar_height + paddingTop -->
+    <!-- layout_height = 52dp -->

[newt (away), 2016/04/01 23:43:10]

this comment in no longer useful; remove it

[gogerald1, 2016/04/12 01:21:12]

Done.

     <LinearLayout
         android:id="@+id/button_bar"
         android:layout_width="match_parent"
-        android:layout_height="72dp"
+        android:layout_height="52dp"
         android:layout_gravity="bottom"
-        android:orientation="horizontal"
-        android:paddingTop="16dp"
         android:paddingStart="16dp"
-        android:paddingEnd="16dp" >
+        android:paddingEnd="16dp"
+        android:orientation="horizontal">
 
         <!--suppress ButtonStyle -->
         <Button
@@ -109,8 +246,7 @@
             android:text="@string/fre_skip_text"
             android:textAllCaps="true"
             android:textColor="@color/light_active_color"
-            android:textSize="14sp"
-            android:layout_marginBottom="24dp" />
+            android:textSize="14sp"/>
 
         <View
             android:layout_width="0dp"
@@ -127,8 +263,7 @@
             android:textAllCaps="true"
             android:textColor="@android:color/white"
             android:textSize="14sp"
-            chrome:buttonColor="@color/light_active_color"
-            android:layout_marginBottom="24dp" />
+            chrome:buttonColor="@color/light_active_color"/>
 
         <View
             android:id="@+id/positive_button_end_padding"