Unify and Improve the Sign-In and Sync Confirmation Screens on Clank.

This improves the sign in flow, making it more obvious to users that they can change their sync settings and allowing them to 'Undo' their sign in.

In addition it allows the sign in flow to be cancelled when it is used in the Bookmarks or the Recent Tabs promo. When used in the Bookmarks promo, cancelling dismisses the sign in flow. When used in the Recent Tabs promo, cancelling removes the fragment and prevents it from showing in the future (in similar behaviour to the text button in the Bookmarks promo that brings up the sign in promo).

Also fixed small bug in ProfileDataCache where it mixes up fullName and givenName when it passes these through to it's observer in onProfileDownloaded.

BUG=557781
Committed: https://crrev.com/6d208bea61b078c391f88b111e8c9edbded1abc5
Cr-Commit-Position: refs/heads/master@{#375439}

[PEConn, 2016-01-13 11:04:37]

peconn@chromium.org changed reviewers:
+ bauerb@chromium.org, newt@chromium.org

[PEConn, 2016-01-13 11:04:37]

bauerb@chromium.org: Please review changes in 
- firstrun/AccountFirstRunFragment.java
- firstrun/AccountFirstRunView.java
- firstrun/ImageCarousel.java
- ntp/RecentTabsPromoView.java
- signin/SigninPromoScreen.java

newt@chromium.org: Please review changes in:
- res/layout/fre_choose_account.xml
- res/values/dimens.xml
- strings/android_chrome_strings.grd

[Bernhard Bauer, 2016-01-13 13:22:48]

Can you add screenshots to the CL description?

https://codereview.chromium.org/1574273002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/firstrun/AccountFirstRunView.java
(right):

https://codereview.chromium.org/1574273002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/firstrun/AccountFirstRunView.java:500:
new SpanInfo("<LINK1>", "</LINK1>", settingsSpan)));
Maybe extract these into constants?

[PEConn, 2016-01-13 13:49:20]

Screenshots for Patch Set #3 can be found in the 'v1' folder here:
https://drive.google.com/a/google.com/folderview?id=0B_ABoZqo1irYOHlNMmtQT1BO...

[PEConn, 2016-02-01 13:18:11]

Ping for newt - could you have a look at the whole bunch now?

https://codereview.chromium.org/1574273002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/firstrun/AccountFirstRunView.java
(right):

https://codereview.chromium.org/1574273002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/firstrun/AccountFirstRunView.java:500:
new SpanInfo("<LINK1>", "</LINK1>", settingsSpan)));
On 2016/01/13 13:22:48, Bernhard Bauer wrote:
> Maybe extract these into constants?

Done.

[newt (away), 2016-02-03 17:57:30]

Thanks for this change! It's mostly just minor comments.

https://codereview.chromium.org/1574273002/diff/60001/chrome/android/java/res...
File chrome/android/java/res/layout/fre_choose_account.xml (right):

https://codereview.chromium.org/1574273002/diff/60001/chrome/android/java/res...
chrome/android/java/res/layout/fre_choose_account.xml:16:
android:layout_marginBottom="57dp"
don't you need to change this to 76dp? (so it doesn't overlap the button bar)

https://codereview.chromium.org/1574273002/diff/60001/chrome/android/java/res...
chrome/android/java/res/layout/fre_choose_account.xml:71: <RelativeLayout
Use a FrameLayout here. It's simpler than RelativeLayout and is preferred unless
you're using RelativeLayout-specific features.

https://codereview.chromium.org/1574273002/diff/60001/chrome/android/java/res...
chrome/android/java/res/layout/fre_choose_account.xml:79:
android:gravity="center"
center_horizontal

https://codereview.chromium.org/1574273002/diff/60001/chrome/android/java/res...
chrome/android/java/res/layout/fre_choose_account.xml:86: <!-- We use an
invisible TextView overlapping with the visible one to keep
How does this look in landscape mode on phones? Does it feel strange to have
lots of unoccupied space at the bottom of the scroll view when the shorter
string is showing?

https://codereview.chromium.org/1574273002/diff/60001/chrome/android/java/res...
chrome/android/java/res/layout/fre_choose_account.xml:97: android:text=""
omit this

https://codereview.chromium.org/1574273002/diff/60001/chrome/android/java/res...
chrome/android/java/res/layout/fre_choose_account.xml:131: <View
set the visibility to invisible, which is slightly more efficient

https://codereview.chromium.org/1574273002/diff/60001/chrome/android/java/res...
chrome/android/java/res/layout/fre_choose_account.xml:132:
android:layout_width="match_parent"
layout_width should be 0dp. That makes for more efficient measuring as the view
is only measured once (at the end when extra space is ready to be distributed),
rather than twice.

https://codereview.chromium.org/1574273002/diff/60001/chrome/android/java/res...
chrome/android/java/res/layout/fre_choose_account.xml:133:
android:layout_height="match_parent"
might as well be 0dp

https://codereview.chromium.org/1574273002/diff/60001/chrome/android/java/res...
chrome/android/java/res/layout/fre_choose_account.xml:151:
android:id="@+id/positive_button_right_padding"
s/right/end/

In RTL mode, this will be the left padding after all.

Alternatively, you could remove this view altogether and center the "sign in"
button when needed by setting the other spacer View to GONE.

https://codereview.chromium.org/1574273002/diff/60001/chrome/android/java/res...
chrome/android/java/res/layout/fre_choose_account.xml:152:
android:layout_width="match_parent"
0dp

https://codereview.chromium.org/1574273002/diff/60001/chrome/android/java/res...
chrome/android/java/res/layout/fre_choose_account.xml:153:
android:layout_height="match_parent"
0dp

https://codereview.chromium.org/1574273002/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/firstrun/AccountFirstRunView.java
(right):

https://codereview.chromium.org/1574273002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/firstrun/AccountFirstRunView.java:66:
public void onSignIn(String accountName);
I'd call this "onAccountSelected" or "onSignInClicked". Remember, this is a
listener interface, so it's reporting events that have already happened (that's
why they're all prefixed with "on").

https://codereview.chromium.org/1574273002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/firstrun/AccountFirstRunView.java:216:
createSignInPage();
s/create/show/ ? or createAndShow

Likewise for createConfirmSignInPage()

https://codereview.chromium.org/1574273002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/firstrun/AccountFirstRunView.java:238:
// The updateAccounts function will have selected the new account.
if updateAccounts() is aware that a new account has been added, perhaps it could
just return a boolean that indicates this condition, and we could avoid what
seems like duplicate logic for checking whether a new account exists and should
be selected.

https://codereview.chromium.org/1574273002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/firstrun/AccountFirstRunView.java:333:
* wizards, it makes sense to allow the user to skip account selection.
s/wizards/First Run/

https://codereview.chromium.org/1574273002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/firstrun/AccountFirstRunView.java:491:
public void updateDrawState(TextPaint ds) {
s/ds/textPaint/

https://codereview.chromium.org/1574273002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/firstrun/AccountFirstRunView.java:509:
private void createCancelButton() {
"create" doesn't quite make sense for these method names, since these methods
don't actually create any buttons, they just change text and set listeners. How
about: "configure" or "update" or "setUp"?

https://codereview.chromium.org/1574273002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/firstrun/AccountFirstRunView.java:518:
setNegativeButtonEnabled(mCanCancel);
I'd put this line at the top of the method; it has the most significant effect.
The rest of the method should probably only be run if mCanCancel is true anyway.

https://codereview.chromium.org/1574273002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/firstrun/AccountFirstRunView.java:521:
private void createSignInButton(boolean anyAccounts) {
s/any/has/

It's best to use standard prefixes for booleans, e.g. "is"/"has"/"did","should"

https://codereview.chromium.org/1574273002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/firstrun/AccountFirstRunView.java:549:
setNegativeButtonEnabled(!isInForcedAccountMode());
likewise

https://codereview.chromium.org/1574273002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/firstrun/AccountFirstRunView.java:563:
private void setNegativeButtonEnabled(boolean enabled) {
s/Enabled/Visible/

"enabled" has a specific meaning in the context of buttons that doesn't apply
here

https://codereview.chromium.org/1574273002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/firstrun/AccountFirstRunView.java:569:
findViewById(R.id.positive_button_right_padding).setVisibility(View.VISIBLE);
s/VISIBLE/INVISIBLE/

invisible views don't take part in drawing, so are slightly more efficient.
(They still take part in measurement and layouts, though, unlike GONE views.)

https://codereview.chromium.org/1574273002/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/firstrun/ImageCarousel.java
(right):

https://codereview.chromium.org/1574273002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/firstrun/ImageCarousel.java:223:
ImageView mCheckmark;
Make this private and put it with the other member variables

https://codereview.chromium.org/1574273002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/firstrun/ImageCarousel.java:237:
return;
Early returns hidden in the middle of a method can be dangerous; it's easy for a
later developer to miss the early return and introduce bugs.

Here's another way to do this:
 - Add a member variable mIsSignedInMode
 - If isSignedIn == mIsSignInMode, then early return right at the beginning of
this method.

https://codereview.chromium.org/1574273002/diff/60001/chrome/android/java/str...
File chrome/android/java/strings/android_chrome_strings.grd (right):

https://codereview.chromium.org/1574273002/diff/60001/chrome/android/java/str...
chrome/android/java/strings/android_chrome_strings.grd:1588: Undo
Could you move "Undo" up to the top of this file with the other generic reusable
strings, and remove other occurrences of Undo from this file?

[PEConn, 2016-02-04 19:23:08]

Patchset #6 (id:100001) has been deleted

[PEConn, 2016-02-04 19:26:10]

I've added the changes to the Cancel buttons you asked for.

I also managed to pick up some changes from a different commit to do with
Bookmarks (in the dimens.xml and chrome_android_strings.grd) that I can't track
the origin for. Please ignore them!

https://codereview.chromium.org/1574273002/diff/60001/chrome/android/java/res...
File chrome/android/java/res/layout/fre_choose_account.xml (right):

https://codereview.chromium.org/1574273002/diff/60001/chrome/android/java/res...
chrome/android/java/res/layout/fre_choose_account.xml:16:
android:layout_marginBottom="57dp"
On 2016/02/03 17:57:29, newt (OOO until 2-8) wrote:
> don't you need to change this to 76dp? (so it doesn't overlap the button bar)

Done.

https://codereview.chromium.org/1574273002/diff/60001/chrome/android/java/res...
chrome/android/java/res/layout/fre_choose_account.xml:71: <RelativeLayout
On 2016/02/03 17:57:28, newt wrote:
> Use a FrameLayout here. It's simpler than RelativeLayout and is preferred
unless
> you're using RelativeLayout-specific features.

Done.

https://codereview.chromium.org/1574273002/diff/60001/chrome/android/java/res...
chrome/android/java/res/layout/fre_choose_account.xml:79:
android:gravity="center"
On 2016/02/03 17:57:28, newt wrote:
> center_horizontal

Done.

https://codereview.chromium.org/1574273002/diff/60001/chrome/android/java/res...
chrome/android/java/res/layout/fre_choose_account.xml:86: <!-- We use an
invisible TextView overlapping with the visible one to keep
On 2016/02/03 17:57:29, newt wrote:
> How does this look in landscape mode on phones? Does it feel strange to have
> lots of unoccupied space at the bottom of the scroll view when the shorter
> string is showing?

There isn't that much unused space - its essentially two lines. On a Nexus 5X,
it looks like: 
https://drive.google.com/open?id=0B_ABoZqo1irYUWdlaUo4UWlCQ0k

https://codereview.chromium.org/1574273002/diff/60001/chrome/android/java/res...
chrome/android/java/res/layout/fre_choose_account.xml:97: android:text=""
On 2016/02/03 17:57:29, newt wrote:
> omit this

Done.

https://codereview.chromium.org/1574273002/diff/60001/chrome/android/java/res...
chrome/android/java/res/layout/fre_choose_account.xml:131: <View
On 2016/02/03 17:57:29, newt wrote:
> set the visibility to invisible, which is slightly more efficient

Done.

https://codereview.chromium.org/1574273002/diff/60001/chrome/android/java/res...
chrome/android/java/res/layout/fre_choose_account.xml:132:
android:layout_width="match_parent"
On 2016/02/03 17:57:28, newt wrote:
> layout_width should be 0dp. That makes for more efficient measuring as the
view
> is only measured once (at the end when extra space is ready to be
distributed),
> rather than twice.

Done.

https://codereview.chromium.org/1574273002/diff/60001/chrome/android/java/res...
chrome/android/java/res/layout/fre_choose_account.xml:133:
android:layout_height="match_parent"
On 2016/02/03 17:57:29, newt wrote:
> might as well be 0dp

Done.

https://codereview.chromium.org/1574273002/diff/60001/chrome/android/java/res...
chrome/android/java/res/layout/fre_choose_account.xml:151:
android:id="@+id/positive_button_right_padding"
On 2016/02/03 17:57:28, newt wrote:
> s/right/end/
> 
> In RTL mode, this will be the left padding after all.
> 
> Alternatively, you could remove this view altogether and center the "sign in"
> button when needed by setting the other spacer View to GONE.

Done.

https://codereview.chromium.org/1574273002/diff/60001/chrome/android/java/res...
chrome/android/java/res/layout/fre_choose_account.xml:152:
android:layout_width="match_parent"
On 2016/02/03 17:57:29, newt wrote:
> 0dp

Done.

https://codereview.chromium.org/1574273002/diff/60001/chrome/android/java/res...
chrome/android/java/res/layout/fre_choose_account.xml:153:
android:layout_height="match_parent"
On 2016/02/03 17:57:28, newt wrote:
> 0dp

Done.

https://codereview.chromium.org/1574273002/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/firstrun/AccountFirstRunView.java
(right):

https://codereview.chromium.org/1574273002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/firstrun/AccountFirstRunView.java:66:
public void onSignIn(String accountName);
On 2016/02/03 17:57:29, newt wrote:
> I'd call this "onAccountSelected" or "onSignInClicked". Remember, this is a
> listener interface, so it's reporting events that have already happened
(that's
> why they're all prefixed with "on").

Done.

https://codereview.chromium.org/1574273002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/firstrun/AccountFirstRunView.java:216:
createSignInPage();
On 2016/02/03 17:57:29, newt wrote:
> s/create/show/ ? or createAndShow
> 
> Likewise for createConfirmSignInPage()

Done.

https://codereview.chromium.org/1574273002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/firstrun/AccountFirstRunView.java:238:
// The updateAccounts function will have selected the new account.
On 2016/02/03 17:57:29, newt wrote:
> if updateAccounts() is aware that a new account has been added, perhaps it
could
> just return a boolean that indicates this condition, and we could avoid what
> seems like duplicate logic for checking whether a new account exists and
should
> be selected.

Done.

https://codereview.chromium.org/1574273002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/firstrun/AccountFirstRunView.java:333:
* wizards, it makes sense to allow the user to skip account selection.
On 2016/02/03 17:57:29, newt (OOO until 2-8) wrote:
> s/wizards/First Run/

Done.

https://codereview.chromium.org/1574273002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/firstrun/AccountFirstRunView.java:491:
public void updateDrawState(TextPaint ds) {
On 2016/02/03 17:57:29, newt wrote:
> s/ds/textPaint/

Done.

https://codereview.chromium.org/1574273002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/firstrun/AccountFirstRunView.java:509:
private void createCancelButton() {
On 2016/02/03 17:57:29, newt wrote:
> "create" doesn't quite make sense for these method names, since these methods
> don't actually create any buttons, they just change text and set listeners.
How
> about: "configure" or "update" or "setUp"?

Done.

https://codereview.chromium.org/1574273002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/firstrun/AccountFirstRunView.java:518:
setNegativeButtonEnabled(mCanCancel);
On 2016/02/03 17:57:29, newt wrote:
> I'd put this line at the top of the method; it has the most significant
effect.
> The rest of the method should probably only be run if mCanCancel is true
anyway.

Done.

https://codereview.chromium.org/1574273002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/firstrun/AccountFirstRunView.java:521:
private void createSignInButton(boolean anyAccounts) {
On 2016/02/03 17:57:29, newt wrote:
> s/any/has/
> 
> It's best to use standard prefixes for booleans, e.g.
"is"/"has"/"did","should"

Done.

https://codereview.chromium.org/1574273002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/firstrun/AccountFirstRunView.java:549:
setNegativeButtonEnabled(!isInForcedAccountMode());
On 2016/02/03 17:57:29, newt wrote:
> likewise

Done.

https://codereview.chromium.org/1574273002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/firstrun/AccountFirstRunView.java:563:
private void setNegativeButtonEnabled(boolean enabled) {
On 2016/02/03 17:57:29, newt wrote:
> s/Enabled/Visible/
> 
> "enabled" has a specific meaning in the context of buttons that doesn't apply
> here

Done.

https://codereview.chromium.org/1574273002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/firstrun/AccountFirstRunView.java:569:
findViewById(R.id.positive_button_right_padding).setVisibility(View.VISIBLE);
On 2016/02/03 17:57:29, newt wrote:
> s/VISIBLE/INVISIBLE/
> 
> invisible views don't take part in drawing, so are slightly more efficient.
> (They still take part in measurement and layouts, though, unlike GONE views.)

Done.

https://codereview.chromium.org/1574273002/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/firstrun/ImageCarousel.java
(right):

https://codereview.chromium.org/1574273002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/firstrun/ImageCarousel.java:223:
ImageView mCheckmark;
On 2016/02/03 17:57:29, newt wrote:
> Make this private and put it with the other member variables

Done.

https://codereview.chromium.org/1574273002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/firstrun/ImageCarousel.java:237:
return;
On 2016/02/03 17:57:29, newt wrote:
> Early returns hidden in the middle of a method can be dangerous; it's easy for
a
> later developer to miss the early return and introduce bugs.
> 
> Here's another way to do this:
>  - Add a member variable mIsSignedInMode
>  - If isSignedIn == mIsSignInMode, then early return right at the beginning of
> this method.

Done.

https://codereview.chromium.org/1574273002/diff/60001/chrome/android/java/str...
File chrome/android/java/strings/android_chrome_strings.grd (right):

https://codereview.chromium.org/1574273002/diff/60001/chrome/android/java/str...
chrome/android/java/strings/android_chrome_strings.grd:1588: Undo
On 2016/02/03 17:57:29, newt wrote:
> Could you move "Undo" up to the top of this file with the other generic
reusable
> strings, and remove other occurrences of Undo from this file?

Done.

[PEConn, 2016-02-08 12:38:58]

Description was changed from

==========
Unify and Improve the Sign-In and Sync Confirmation Screens on Clank.

BUG=557781
==========

to

==========
Unify and Improve the Sign-In and Sync Confirmation Screens on Clank.

BUG=557781,585065
==========

[newt (away), 2016-02-09 23:08:12]

There seem to be a few bug fixes and other changes mixed up  with the design
changes. It'd be helpful to separate the bug fixes out so they can be more
easily and reliably reviewed.

https://codereview.chromium.org/1574273002/diff/120001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkSigninActivity.java
(right):

https://codereview.chromium.org/1574273002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkSigninActivity.java:134:
RecordUserAction.record("Stars_SignInPromoActivity_Cancel");
Where did these metrics come from? Are they documented in
tools/metrics/actions/actions.xml?

If you're adding new metrics, please mention that in the CL description and
update actions.xml.

https://codereview.chromium.org/1574273002/diff/120001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/firstrun/AccountFirstRunView.java
(right):

https://codereview.chromium.org/1574273002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/firstrun/AccountFirstRunView.java:118:
private static final String SETTINGS_LINK = "LINK1";
I wouldn't bother creating a constant for this. Simpler:

  private static final String SETTINGS_LINK_OPEN = "<LINK1>";
  private static final String SETTINGS_LINK_CLOSE = "</LINK1>";

Normally, I'd say these constants are inlinable, but since they're used in two
different places, it's reasonable to make them class constants.

https://codereview.chromium.org/1574273002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/firstrun/AccountFirstRunView.java:244:
// crbug/585065
s/crbug/crbug.com/   (that's what people usually write, and it's less
mysterious)

https://codereview.chromium.org/1574273002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/firstrun/AccountFirstRunView.java:245:
AccountTrackerService.get(getContext()).invalidateAccountSeedStatus(true);
It's probably better to do this bug fix as a separate CL. Bundling up bug fixes
in and amongst a refactoring makes it hard to follow what's going on (both for
the author, the reviewer, and future developers looking through logs) and easier
to introduce mistakes.

https://codereview.chromium.org/1574273002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/firstrun/AccountFirstRunView.java:282:
public void configureForRecentTabsPage() {
This probably needs to be renamed. But that can be done as part of the CL to
change the recent tabs page promo to be text-only.

https://codereview.chromium.org/1574273002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/firstrun/AccountFirstRunView.java:346:
* settings-type contexts, it does not make sense to allow this.
How about: "in other contexts (e.g. the user clicked "Sign in" from
Settings)..."

https://codereview.chromium.org/1574273002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/firstrun/AccountFirstRunView.java:350:
public void setCanCancel(boolean canCancel) {
Didn't we decide on the bug that there will always be a cancel button (sometimes
called "No thanks", sometimes called "Cancel")? In that case, you should be able
to remove this method, mCanCancel, and the extra view that was added to allow
for centering the positive button when the cancel button is hidden.

(Or maybe you can't do this until we make the change to the recent tabs page to
show a text-only promo. In that case, maybe add a TODO to do those cleanups.)

https://codereview.chromium.org/1574273002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/firstrun/AccountFirstRunView.java:399:
boolean accountsAdded = mSpinner.getSelectedItemPosition() != accountToSelect;
This logic seems hard to justify and could easily break in future refactorings
(or is there a simple and solid reason why the spinner's old selected item index
couldn't equal the index of the newly added account?). How about checking the
return value of getIndexOfNewElement(), or simply checking if oldAccountNames
and mAccountNames have the same elements.

https://codereview.chromium.org/1574273002/diff/120001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/firstrun/ImageCarousel.java
(right):

https://codereview.chromium.org/1574273002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/firstrun/ImageCarousel.java:378:
if (!mAccountSelected || i == getCenterPosition()) {
Why this change?

https://codereview.chromium.org/1574273002/diff/120001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/firstrun/ProfileDataCache.java
(right):

https://codereview.chromium.org/1574273002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/firstrun/ProfileDataCache.java:145:
if (mObserver != null) mObserver.onProfileDownloaded(accountId, fullName,
givenName,
It looks like you're fixing a bug along with the refactoring changes. I'd
mention the bug fix in the CL description.

https://codereview.chromium.org/1574273002/diff/120001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/RecentTabsManager.java
(right):

https://codereview.chromium.org/1574273002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ntp/RecentTabsManager.java:392:
if (PreferenceManager.getDefaultSharedPreferences(mContext).getBoolean(
Please document this change in behavior in the CL description.

https://codereview.chromium.org/1574273002/diff/120001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/RecentTabsPromoView.java
(right):

https://codereview.chromium.org/1574273002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ntp/RecentTabsPromoView.java:258:
assert mUserActionListener != null : "A UserActionListener should be provided";
Personally, I'd omit the string attached to this assert. I don't think it adds
anything.

https://codereview.chromium.org/1574273002/diff/120001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/RecentTabsRowAdapter.java
(right):

https://codereview.chromium.org/1574273002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ntp/RecentTabsRowAdapter.java:608:
RecordUserAction.record("Signin_Cancel_FromRecentTabs");
also need to add this actions.xml

https://codereview.chromium.org/1574273002/diff/120001/chrome/browser/android...
File chrome/browser/android/profiles/profile_downloader_android.cc (right):

https://codereview.chromium.org/1574273002/diff/120001/chrome/browser/android...
chrome/browser/android/profiles/profile_downloader_android.cc:196: AccountInfo
account_info =
Is this supposed to be here? If so, why / what is this fixing? (I'm not familiar
with this class)

[PEConn, 2016-02-10 10:06:12]

Description was changed from

==========
Unify and Improve the Sign-In and Sync Confirmation Screens on Clank.

BUG=557781,585065
==========

to

==========
Unify and Improve the Sign-In and Sync Confirmation Screens on Clank.

BUG=557781
==========

[PEConn, 2016-02-11 14:23:34]

Description was changed from

==========
Unify and Improve the Sign-In and Sync Confirmation Screens on Clank.

BUG=557781
==========

to

==========
Unify and Improve the Sign-In and Sync Confirmation Screens on Clank.

Also fixed small bug in ProfileDataCache where it mixes up fullName and
givenName when it passes these through to it's observer in onProfileDownloaded.

BUG=557781
==========

[PEConn, 2016-02-11 14:28:47]

Description was changed from

==========
Unify and Improve the Sign-In and Sync Confirmation Screens on Clank.

Also fixed small bug in ProfileDataCache where it mixes up fullName and
givenName when it passes these through to it's observer in onProfileDownloaded.

BUG=557781
==========

to

==========
This improves the sign in flow, making it more obvious to users that they can
change their sync settings and allowing them to 'Undo' their sign in.

In addition it allows the sign in flow to be cancelled when it is used in the
Bookmarks or the Recent Tabs promo. When used in the Bookmarks promo, cancelling
dismisses the sign in flow. When used in the Recent Tabs promo, cancelling
removes the fragment and prevents it from showing in the future (in similar
behaviour to the text button in the Bookmarks promo that brings up the sign in
promo).

Also fixed small bug in ProfileDataCache where it mixes up fullName and
givenName when it passes these through to it's observer in onProfileDownloaded.

BUG=557781
==========

[PEConn, 2016-02-11 14:42:39]

Patchset #7 (id:140001) has been deleted

[PEConn, 2016-02-11 14:42:48]

Patchset #7 (id:160001) has been deleted

[PEConn, 2016-02-11 14:44:56]

I cut down the changes in this CL, splitting out the new user profile picture
bugfix (for crbug.com/585065 ) and adding user metrics to future CLs.

I also reset upstream so hopefully the side-by-side diffs should be cleaner.

https://codereview.chromium.org/1574273002/diff/120001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkSigninActivity.java
(right):

https://codereview.chromium.org/1574273002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkSigninActivity.java:134:
RecordUserAction.record("Stars_SignInPromoActivity_Cancel");
On 2016/02/09 23:08:12, newt (slow) wrote:
> Where did these metrics come from? Are they documented in
> tools/metrics/actions/actions.xml?
> 
> If you're adding new metrics, please mention that in the CL description and
> update actions.xml.

I'm removing this to put in separate CL for crbug.com/585893.

https://codereview.chromium.org/1574273002/diff/120001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/firstrun/AccountFirstRunView.java
(right):

https://codereview.chromium.org/1574273002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/firstrun/AccountFirstRunView.java:118:
private static final String SETTINGS_LINK = "LINK1";
On 2016/02/09 23:08:12, newt (slow) wrote:
> I wouldn't bother creating a constant for this. Simpler:
> 
>   private static final String SETTINGS_LINK_OPEN = "<LINK1>";
>   private static final String SETTINGS_LINK_CLOSE = "</LINK1>";
> 
> Normally, I'd say these constants are inlinable, but since they're used in two
> different places, it's reasonable to make them class constants.

Done.

https://codereview.chromium.org/1574273002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/firstrun/AccountFirstRunView.java:244:
// crbug/585065
On 2016/02/09 23:08:12, newt (slow) wrote:
> s/crbug/crbug.com/   (that's what people usually write, and it's less
> mysterious)

Done.

https://codereview.chromium.org/1574273002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/firstrun/AccountFirstRunView.java:245:
AccountTrackerService.get(getContext()).invalidateAccountSeedStatus(true);
On 2016/02/09 23:08:12, newt (slow) wrote:
> It's probably better to do this bug fix as a separate CL. Bundling up bug
fixes
> in and amongst a refactoring makes it hard to follow what's going on (both for
> the author, the reviewer, and future developers looking through logs) and
easier
> to introduce mistakes.

I've removed this from this CL.

https://codereview.chromium.org/1574273002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/firstrun/AccountFirstRunView.java:282:
public void configureForRecentTabsPage() {
On 2016/02/09 23:08:12, newt (slow) wrote:
> This probably needs to be renamed. But that can be done as part of the CL to
> change the recent tabs page promo to be text-only.

Done.

https://codereview.chromium.org/1574273002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/firstrun/AccountFirstRunView.java:346:
* settings-type contexts, it does not make sense to allow this.
On 2016/02/09 23:08:12, newt (slow) wrote:
> How about: "in other contexts (e.g. the user clicked "Sign in" from
> Settings)..."

Done.

https://codereview.chromium.org/1574273002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/firstrun/AccountFirstRunView.java:350:
public void setCanCancel(boolean canCancel) {
On 2016/02/09 23:08:12, newt (slow) wrote:
> Didn't we decide on the bug that there will always be a cancel button
(sometimes
> called "No thanks", sometimes called "Cancel")? In that case, you should be
able
> to remove this method, mCanCancel, and the extra view that was added to allow
> for centering the positive button when the cancel button is hidden.
> 
> (Or maybe you can't do this until we make the change to the recent tabs page
to
> show a text-only promo. In that case, maybe add a TODO to do those cleanups.)

I've removed this method, however I've got to keep the extra view for centering
the positive button because the negative button isn't only used for Cancel. It
is also used for 'Undo' which may be disabled if the user is forced to accept
their account (eg. managed accounts).

https://codereview.chromium.org/1574273002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/firstrun/AccountFirstRunView.java:399:
boolean accountsAdded = mSpinner.getSelectedItemPosition() != accountToSelect;
On 2016/02/09 23:08:12, newt (slow) wrote:
> This logic seems hard to justify and could easily break in future refactorings
> (or is there a simple and solid reason why the spinner's old selected item
index
> couldn't equal the index of the newly added account?). How about checking the
> return value of getIndexOfNewElement(), or simply checking if oldAccountNames
> and mAccountNames have the same elements.

Done.

https://codereview.chromium.org/1574273002/diff/120001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/firstrun/ImageCarousel.java
(right):

https://codereview.chromium.org/1574273002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/firstrun/ImageCarousel.java:378:
if (!mAccountSelected || i == getCenterPosition()) {
On 2016/02/09 23:08:12, newt (slow) wrote:
> Why this change?

This was for crbug.com/585065 .
I'm removing it from this CL.

https://codereview.chromium.org/1574273002/diff/120001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/firstrun/ProfileDataCache.java
(right):

https://codereview.chromium.org/1574273002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/firstrun/ProfileDataCache.java:145:
if (mObserver != null) mObserver.onProfileDownloaded(accountId, fullName,
givenName,
On 2016/02/09 23:08:12, newt (slow) wrote:
> It looks like you're fixing a bug along with the refactoring changes. I'd
> mention the bug fix in the CL description.

Done.

https://codereview.chromium.org/1574273002/diff/120001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/RecentTabsManager.java
(right):

https://codereview.chromium.org/1574273002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ntp/RecentTabsManager.java:392:
if (PreferenceManager.getDefaultSharedPreferences(mContext).getBoolean(
On 2016/02/09 23:08:12, newt (slow) wrote:
> Please document this change in behavior in the CL description.

Done.

https://codereview.chromium.org/1574273002/diff/120001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/RecentTabsPromoView.java
(right):

https://codereview.chromium.org/1574273002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ntp/RecentTabsPromoView.java:258:
assert mUserActionListener != null : "A UserActionListener should be provided";
On 2016/02/09 23:08:12, newt (slow) wrote:
> Personally, I'd omit the string attached to this assert. I don't think it adds
> anything.

Done.

https://codereview.chromium.org/1574273002/diff/120001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/RecentTabsRowAdapter.java
(right):

https://codereview.chromium.org/1574273002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ntp/RecentTabsRowAdapter.java:608:
RecordUserAction.record("Signin_Cancel_FromRecentTabs");
On 2016/02/09 23:08:12, newt (slow) wrote:
> also need to add this actions.xml

Removed to put in a different CL.

https://codereview.chromium.org/1574273002/diff/120001/chrome/browser/android...
File chrome/browser/android/profiles/profile_downloader_android.cc (right):

https://codereview.chromium.org/1574273002/diff/120001/chrome/browser/android...
chrome/browser/android/profiles/profile_downloader_android.cc:196: AccountInfo
account_info =
On 2016/02/09 23:08:12, newt (slow) wrote:
> Is this supposed to be here? If so, why / what is this fixing? (I'm not
familiar
> with this class)

This catches the issue where the AccountTrackerService doesn't contain the user
account information of an added Profile and logs an error for it:
crbug.com/585065

[newt (away), 2016-02-11 16:46:06]

lgtm after two last comments. Thanks!

https://codereview.chromium.org/1574273002/diff/180001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/firstrun/AccountFirstRunView.java
(right):

https://codereview.chromium.org/1574273002/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/firstrun/AccountFirstRunView.java:379:
return oldAccountNames.size() == mAccountNames.size()
could oldAccountNames be null here?

https://codereview.chromium.org/1574273002/diff/180001/chrome/browser/android...
File chrome/browser/android/profiles/profile_downloader_android.cc (right):

https://codereview.chromium.org/1574273002/diff/180001/chrome/browser/android...
chrome/browser/android/profiles/profile_downloader_android.cc:200: LOG(ERROR) <<
"Could not get valid account information for " << email;
I believe we shouldn't log email addresses. I'd remove the email here, or check
with someone to make sure it's OK.

[PEConn, 2016-02-12 09:16:59]

The CQ bit was checked by peconn@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-02-12 09:17:15]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1574273002/200001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1574273002/200001

[commit-bot: I haz the power, 2016-02-12 09:56:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-02-12 09:56:28]

Dry run: This issue passed the CQ dry run.

[newt (away), 2016-02-12 15:53:10]

btw, I found the link about not logging PII:
https://chromium.googlesource.com/chromium/src/+/master/docs/android_logging....

[PEConn, 2016-02-12 18:41:21]

https://codereview.chromium.org/1574273002/diff/180001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/firstrun/AccountFirstRunView.java
(right):

https://codereview.chromium.org/1574273002/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/firstrun/AccountFirstRunView.java:379:
return oldAccountNames.size() == mAccountNames.size()
On 2016/02/11 16:46:05, newt (slow) wrote:
> could oldAccountNames be null here?

Done.

https://codereview.chromium.org/1574273002/diff/180001/chrome/browser/android...
File chrome/browser/android/profiles/profile_downloader_android.cc (right):

https://codereview.chromium.org/1574273002/diff/180001/chrome/browser/android...
chrome/browser/android/profiles/profile_downloader_android.cc:200: LOG(ERROR) <<
"Could not get valid account information for " << email;
On 2016/02/11 16:46:06, newt (slow) wrote:
> I believe we shouldn't log email addresses. I'd remove the email here, or
check
> with someone to make sure it's OK.

Done.

[PEConn, 2016-02-12 18:41:26]

The CQ bit was checked by peconn@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-02-12 18:44:07]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1574273002/220001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1574273002/220001

[commit-bot: I haz the power, 2016-02-12 18:49:12]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-02-12 18:49:13]

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  cast_shell_linux on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  chromeos_amd64-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  chromeos_daisy_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[PEConn, 2016-02-15 09:06:20]

The CQ bit was checked by peconn@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-02-15 09:06:55]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1574273002/220001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1574273002/220001

[commit-bot: I haz the power, 2016-02-15 09:10:18]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-02-15 09:10:20]

Dry run: Try jobs failed on following builders:
  android_chromium_gn_compile_rel on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...)
  linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[PEConn, 2016-02-15 09:42:44]

The CQ bit was checked by peconn@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-02-15 09:43:23]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1574273002/240001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1574273002/240001

[commit-bot: I haz the power, 2016-02-15 10:29:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-02-15 10:29:30]

Dry run: This issue passed the CQ dry run.

[PEConn, 2016-02-15 10:34:55]

The CQ bit was checked by peconn@chromium.org

[PEConn, 2016-02-15 10:34:56]

The patchset sent to the CQ was uploaded after l-g-t-m from newt@chromium.org
Link to the patchset: https://codereview.chromium.org/1574273002/#ps240001
(title: " ")

[commit-bot: I haz the power, 2016-02-15 10:35:41]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1574273002/240001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1574273002/240001

[commit-bot: I haz the power, 2016-02-15 10:40:46]

Description was changed from

==========
This improves the sign in flow, making it more obvious to users that they can
change their sync settings and allowing them to 'Undo' their sign in.

In addition it allows the sign in flow to be cancelled when it is used in the
Bookmarks or the Recent Tabs promo. When used in the Bookmarks promo, cancelling
dismisses the sign in flow. When used in the Recent Tabs promo, cancelling
removes the fragment and prevents it from showing in the future (in similar
behaviour to the text button in the Bookmarks promo that brings up the sign in
promo).

Also fixed small bug in ProfileDataCache where it mixes up fullName and
givenName when it passes these through to it's observer in onProfileDownloaded.

BUG=557781
==========

to

==========
This improves the sign in flow, making it more obvious to users that they can
change their sync settings and allowing them to 'Undo' their sign in.

In addition it allows the sign in flow to be cancelled when it is used in the
Bookmarks or the Recent Tabs promo. When used in the Bookmarks promo, cancelling
dismisses the sign in flow. When used in the Recent Tabs promo, cancelling
removes the fragment and prevents it from showing in the future (in similar
behaviour to the text button in the Bookmarks promo that brings up the sign in
promo).

Also fixed small bug in ProfileDataCache where it mixes up fullName and
givenName when it passes these through to it's observer in onProfileDownloaded.

BUG=557781
==========

[commit-bot: I haz the power, 2016-02-15 10:40:47]

Committed patchset #10 (id:240001)

[commit-bot: I haz the power, 2016-02-16 22:49:32]

Description was changed from

==========
This improves the sign in flow, making it more obvious to users that they can
change their sync settings and allowing them to 'Undo' their sign in.

In addition it allows the sign in flow to be cancelled when it is used in the
Bookmarks or the Recent Tabs promo. When used in the Bookmarks promo, cancelling
dismisses the sign in flow. When used in the Recent Tabs promo, cancelling
removes the fragment and prevents it from showing in the future (in similar
behaviour to the text button in the Bookmarks promo that brings up the sign in
promo).

Also fixed small bug in ProfileDataCache where it mixes up fullName and
givenName when it passes these through to it's observer in onProfileDownloaded.

BUG=557781
==========

to

==========
This improves the sign in flow, making it more obvious to users that they can
change their sync settings and allowing them to 'Undo' their sign in.

In addition it allows the sign in flow to be cancelled when it is used in the
Bookmarks or the Recent Tabs promo. When used in the Bookmarks promo, cancelling
dismisses the sign in flow. When used in the Recent Tabs promo, cancelling
removes the fragment and prevents it from showing in the future (in similar
behaviour to the text button in the Bookmarks promo that brings up the sign in
promo).

Also fixed small bug in ProfileDataCache where it mixes up fullName and
givenName when it passes these through to it's observer in onProfileDownloaded.

BUG=557781

Committed: https://crrev.com/6d208bea61b078c391f88b111e8c9edbded1abc5
Cr-Commit-Position: refs/heads/master@{#375439}
==========

[commit-bot: I haz the power, 2016-02-16 22:49:33]

Patchset 10 (id:??) landed as
https://crrev.com/6d208bea61b078c391f88b111e8c9edbded1abc5
Cr-Commit-Position: refs/heads/master@{#375439}