Unified Bookmarks and Recent Tabs signin methods.

Unified Bookmarks and Recent Tabs signin methods.

This involves moving the Recent Tabs signin method to showing a button that
summons a Signin Activity (previously the Signin Activity was integrated in
the Recent Tabs page) - this is the SigninAndSyncView.

This also provides a Settings link to the Bookmarks and Recent Tabs signin
flows.

Additionally, it replaces the BookmarkSigninActivity with the AccountSigninActivity.

BUG=583774, 598868, 599736
Committed: https://crrev.com/d830894230c7cde07ff92234c6a4e4424b39e10e
Cr-Commit-Position: refs/heads/master@{#390356}

[PEConn, 2016-04-20 08:28:34]

Description was changed from

==========
Unified Bookmarks and Recent Tabs signin methods.

This involves moving the Recent Tabs signin method to showing a button that
summons a Signin Activity (previously the Signin Activity was integrated in
the Recent Tabs page).

This also provides a Settings link to the Bookmarks and Recent Tabs signin
flows.

BUG=583774, 598868
==========

to

==========
Unified Bookmarks and Recent Tabs signin methods.

This involves moving the Recent Tabs signin method to showing a button that
summons a Signin Activity (previously the Signin Activity was integrated in
the Recent Tabs page) - this is the SigninAndSyncView.

This also provides a Settings link to the Bookmarks and Recent Tabs signin
flows.

Additionally, it replaces the BookmarkSigninActivity with the
AccountSigninActivity.

BUG=583774, 598868
==========

[PEConn, 2016-04-20 08:29:31]

Description was changed from

==========
Unified Bookmarks and Recent Tabs signin methods.

This involves moving the Recent Tabs signin method to showing a button that
summons a Signin Activity (previously the Signin Activity was integrated in
the Recent Tabs page) - this is the SigninAndSyncView.

This also provides a Settings link to the Bookmarks and Recent Tabs signin
flows.

Additionally, it replaces the BookmarkSigninActivity with the
AccountSigninActivity.

BUG=583774, 598868
==========

to

==========
Unified Bookmarks and Recent Tabs signin methods.

This involves moving the Recent Tabs signin method to showing a button that
summons a Signin Activity (previously the Signin Activity was integrated in
the Recent Tabs page) - this is the SigninAndSyncView.

This also provides a Settings link to the Bookmarks and Recent Tabs signin
flows.

Additionally, it replaces the BookmarkSigninActivity with the
AccountSigninActivity.

BUG=583774, 598868, 599736
==========

[PEConn, 2016-04-20 08:30:47]

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

[PEConn, 2016-04-20 08:30:48]

PTAL, this is still a work in progress (I'm going over string changes and have
plans to tidy the code up once I know there aren't any fundamental design
issues).

[Bernhard Bauer, 2016-04-20 10:59:06]

Nice!

https://codereview.chromium.org/1901623002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/RecentTabsRowAdapter.java
(right):

https://codereview.chromium.org/1901623002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/RecentTabsRowAdapter.java:597:
SigninAndSyncView.Listener listener = new SigninAndSyncView.Listener(){
Nit: Space before opening brace.

https://codereview.chromium.org/1901623002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninActivity.java
(right):

https://codereview.chromium.org/1901623002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninActivity.java:46:
assert accessPoint > 0 && accessPoint < SigninAccessPoint.MAX : "invalid access
point";
Technically, 0 is a valid enum value here (it's ACCESS_POINT_START_PAGE). I
don't think it's used on Android, but if we want to verify that (as opposed to
just verifying that it's in the general range), we should explicitly check for
the all the values that we know are used on Android.

https://codereview.chromium.org/1901623002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninActivity.java:72:
: "A access point must be provided when starting BookmarkSigninActivity";
Nit: "An access point" :)

https://codereview.chromium.org/1901623002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninActivity.java:131:
if (getAccessPoint() == SigninAccessPoint.BOOKMARK_MANAGER) {
Use a switch statement?

https://codereview.chromium.org/1901623002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/signin/SigninAndSyncView.java
(right):

https://codereview.chromium.org/1901623002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/signin/SigninAndSyncView.java:29:
private final Context mContext;
Could you use getContext() in place of this?

https://codereview.chromium.org/1901623002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/signin/SigninAndSyncView.java:81:
setupRelevantView();
Inline this?

https://codereview.chromium.org/1901623002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/signin/SigninAndSyncView.java:85:
mNegativeButton.setVisibility(View.VISIBLE);
We have a couple of things here (visibility, enabledness, text, callback) we
want to set up for each of the UI elements, depending on the state we're in.
Right now we're doing the distinction of cases by state -- would it make sense
to instead go through each of the UI elements and set it up depending on the
state? That would ensure we deal with each UI element, and do so in a single
place.

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

https://codereview.chromium.org/1901623002/diff/20001/chrome/android/java/str...
chrome/android/java/strings/android_chrome_strings.grd:997: To get your
bookmarks on all your devices, sign in to Chrome
I think this should have a period at the end.

https://codereview.chromium.org/1901623002/diff/20001/chrome/android/java/str...
chrome/android/java/strings/android_chrome_strings.grd:1000: To get your recent
tabs on all your devices, sign in to Chrome
Period here as well.

https://codereview.chromium.org/1901623002/diff/20001/chrome/android/java/str...
chrome/android/java/strings/android_chrome_strings.grd:1006: Bookmarks saved on
your other devices will appear hear.
Nit: "here"

[PEConn, 2016-04-25 11:21:24]

https://codereview.chromium.org/1901623002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/RecentTabsRowAdapter.java
(right):

https://codereview.chromium.org/1901623002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/RecentTabsRowAdapter.java:597:
SigninAndSyncView.Listener listener = new SigninAndSyncView.Listener(){
On 2016/04/20 10:59:06, Bernhard Bauer wrote:
> Nit: Space before opening brace.

Done.

https://codereview.chromium.org/1901623002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninActivity.java
(right):

https://codereview.chromium.org/1901623002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninActivity.java:46:
assert accessPoint > 0 && accessPoint < SigninAccessPoint.MAX : "invalid access
point";
On 2016/04/20 10:59:06, Bernhard Bauer wrote:
> Technically, 0 is a valid enum value here (it's ACCESS_POINT_START_PAGE). I
> don't think it's used on Android, but if we want to verify that (as opposed to
> just verifying that it's in the general range), we should explicitly check for
> the all the values that we know are used on Android.

Done.

https://codereview.chromium.org/1901623002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninActivity.java:72:
: "A access point must be provided when starting BookmarkSigninActivity";
On 2016/04/20 10:59:06, Bernhard Bauer wrote:
> Nit: "An access point" :)

Done.

https://codereview.chromium.org/1901623002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninActivity.java:131:
if (getAccessPoint() == SigninAccessPoint.BOOKMARK_MANAGER) {
On 2016/04/20 10:59:06, Bernhard Bauer wrote:
> Use a switch statement?

I'm not sure it looks that much neater. Done though, mainly for the novelty of
getting to use a switch statement.

https://codereview.chromium.org/1901623002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/signin/SigninAndSyncView.java
(right):

https://codereview.chromium.org/1901623002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/signin/SigninAndSyncView.java:29:
private final Context mContext;
On 2016/04/20 10:59:06, Bernhard Bauer wrote:
> Could you use getContext() in place of this?

Done.

https://codereview.chromium.org/1901623002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/signin/SigninAndSyncView.java:81:
setupRelevantView();
On 2016/04/20 10:59:06, Bernhard Bauer wrote:
> Inline this?

Done.

https://codereview.chromium.org/1901623002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/signin/SigninAndSyncView.java:85:
mNegativeButton.setVisibility(View.VISIBLE);
On 2016/04/20 10:59:06, Bernhard Bauer wrote:
> We have a couple of things here (visibility, enabledness, text, callback) we
> want to set up for each of the UI elements, depending on the state we're in.
> Right now we're doing the distinction of cases by state -- would it make sense
> to instead go through each of the UI elements and set it up depending on the
> state? That would ensure we deal with each UI element, and do so in a single
> place. 

These three lines of code are essentially resetting the UI to a known state
(I've added a comment).

I prefer this way of doing it so we have the code responsible for a given UI
clumped together. I think your approach would make it harder to reason about
what the UI gets set for a given state.

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

https://codereview.chromium.org/1901623002/diff/20001/chrome/android/java/str...
chrome/android/java/strings/android_chrome_strings.grd:997: To get your
bookmarks on all your devices, sign in to Chrome
On 2016/04/20 10:59:06, Bernhard Bauer wrote:
> I think this should have a period at the end.

Done.

https://codereview.chromium.org/1901623002/diff/20001/chrome/android/java/str...
chrome/android/java/strings/android_chrome_strings.grd:1000: To get your recent
tabs on all your devices, sign in to Chrome
On 2016/04/20 10:59:06, Bernhard Bauer wrote:
> Period here as well.

Done.

https://codereview.chromium.org/1901623002/diff/20001/chrome/android/java/str...
chrome/android/java/strings/android_chrome_strings.grd:1006: Bookmarks saved on
your other devices will appear hear.
On 2016/04/20 10:59:06, Bernhard Bauer wrote:
> Nit: "here"

Done.

[PEConn, 2016-04-25 16:50:40]

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

[commit-bot: I haz the power, 2016-04-25 16:51:07]

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

[commit-bot: I haz the power, 2016-04-25 17:09:05]

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

[commit-bot: I haz the power, 2016-04-25 17:09:06]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)

[PEConn, 2016-04-26 09:13:58]

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

[commit-bot: I haz the power, 2016-04-26 09:14:09]

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

[PEConn, 2016-04-26 09:36:14]

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

[commit-bot: I haz the power, 2016-04-26 09:36:33]

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

[commit-bot: I haz the power, 2016-04-26 10:26:32]

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

[commit-bot: I haz the power, 2016-04-26 10:26:33]

Dry run: This issue passed the CQ dry run.

[Bernhard Bauer, 2016-04-26 14:51:20]

https://codereview.chromium.org/1901623002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninActivity.java
(right):

https://codereview.chromium.org/1901623002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninActivity.java:131:
if (getAccessPoint() == SigninAccessPoint.BOOKMARK_MANAGER) {
On 2016/04/25 11:21:23, PEConn1 wrote:
> On 2016/04/20 10:59:06, Bernhard Bauer wrote:
> > Use a switch statement?
> 
> I'm not sure it looks that much neater. Done though, mainly for the novelty of
> getting to use a switch statement.

Whatever makes you happy, man 😉

FWIW, I do think it looks nicer, and it has the advantage of the compiler
checking that you e.g. don't accidentally duplicate values.

https://codereview.chromium.org/1901623002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/signin/SigninAndSyncView.java
(right):

https://codereview.chromium.org/1901623002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/signin/SigninAndSyncView.java:85:
mNegativeButton.setVisibility(View.VISIBLE);
On 2016/04/25 11:21:23, PEConn1 wrote:
> On 2016/04/20 10:59:06, Bernhard Bauer wrote:
> > We have a couple of things here (visibility, enabledness, text, callback) we
> > want to set up for each of the UI elements, depending on the state we're in.
> > Right now we're doing the distinction of cases by state -- would it make
sense
> > to instead go through each of the UI elements and set it up depending on the
> > state? That would ensure we deal with each UI element, and do so in a single
> > place. 
> 
> These three lines of code are essentially resetting the UI to a known state
> (I've added a comment).
> 
> I prefer this way of doing it so we have the code responsible for a given UI
> clumped together. I think your approach would make it harder to reason about
> what the UI gets set for a given state.

The thing is though, the UI is not reset completely – for example, the text
depends on the state, but is not reset here. And if you want to reason about the
UI for a given state, you'd still have to look at here, then at the method that
is called, and then in the case of setupSyncView, outside of the if-statement as
well as inside.

Basically, I'm looking here for a way to make the effect of each state on each
UI element clear, and to allow a reader to verify that everything is covered.
For example, for infobars in Chrome we use a delegate interface
(https://code.google.com/p/chromium/codesearch#chromium/src/components/infobar...)
with a method for each aspect of the UI, so the compiler enforces that every
implementation covers all of the aspects.

https://codereview.chromium.org/1901623002/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/signin/SigninAndSyncView.java
(right):

https://codereview.chromium.org/1901623002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/signin/SigninAndSyncView.java:131:
// TODO(crbug/557784): Like AccountManagementFragment, this would also
Nit: If you use crbug.com/557784, it gets linkified automatically in many
places.

[PEConn, 2016-04-27 10:19:07]

I didn't go with creating delegates - it introduced a lot of boilerplate and the
code seemed to get too spread out. Instead I created a ViewState object
requiring all the information setting up the view required and made sure that
each setup function returned it. PTAL!

https://codereview.chromium.org/1901623002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninActivity.java
(right):

https://codereview.chromium.org/1901623002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninActivity.java:131:
if (getAccessPoint() == SigninAccessPoint.BOOKMARK_MANAGER) {
On 2016/04/26 14:51:20, Bernhard Bauer wrote:
> On 2016/04/25 11:21:23, PEConn1 wrote:
> > On 2016/04/20 10:59:06, Bernhard Bauer wrote:
> > > Use a switch statement?
> > 
> > I'm not sure it looks that much neater. Done though, mainly for the novelty
of
> > getting to use a switch statement.
> 
> Whatever makes you happy, man 😉
> 
> FWIW, I do think it looks nicer, and it has the advantage of the compiler
> checking that you e.g. don't accidentally duplicate values.

Acknowledged.

https://codereview.chromium.org/1901623002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/signin/SigninAndSyncView.java
(right):

https://codereview.chromium.org/1901623002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/signin/SigninAndSyncView.java:85:
mNegativeButton.setVisibility(View.VISIBLE);
On 2016/04/26 14:51:20, Bernhard Bauer wrote:
> On 2016/04/25 11:21:23, PEConn1 wrote:
> > On 2016/04/20 10:59:06, Bernhard Bauer wrote:
> > > We have a couple of things here (visibility, enabledness, text, callback)
we
> > > want to set up for each of the UI elements, depending on the state we're
in.
> > > Right now we're doing the distinction of cases by state -- would it make
> sense
> > > to instead go through each of the UI elements and set it up depending on
the
> > > state? That would ensure we deal with each UI element, and do so in a
single
> > > place. 
> > 
> > These three lines of code are essentially resetting the UI to a known state
> > (I've added a comment).
> > 
> > I prefer this way of doing it so we have the code responsible for a given UI
> > clumped together. I think your approach would make it harder to reason about
> > what the UI gets set for a given state.
> 
> The thing is though, the UI is not reset completely – for example, the text
> depends on the state, but is not reset here. And if you want to reason about
the
> UI for a given state, you'd still have to look at here, then at the method
that
> is called, and then in the case of setupSyncView, outside of the if-statement
as
> well as inside.
> 
> Basically, I'm looking here for a way to make the effect of each state on each
> UI element clear, and to allow a reader to verify that everything is covered.
> For example, for infobars in Chrome we use a delegate interface
>
(https://code.google.com/p/chromium/codesearch#chromium/src/components/infobar...)
> with a method for each aspect of the UI, so the compiler enforces that every
> implementation covers all of the aspects.

Done.

https://codereview.chromium.org/1901623002/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/signin/SigninAndSyncView.java
(right):

https://codereview.chromium.org/1901623002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/signin/SigninAndSyncView.java:131:
// TODO(crbug/557784): Like AccountManagementFragment, this would also
On 2016/04/26 14:51:20, Bernhard Bauer wrote:
> Nit: If you use crbug.com/557784, it gets linkified automatically in many
> places.

Done.

[PEConn, 2016-04-27 10:23:50]

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

[commit-bot: I haz the power, 2016-04-27 10:24:08]

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

[commit-bot: I haz the power, 2016-04-27 11:23:34]

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

[commit-bot: I haz the power, 2016-04-27 11:23:36]

Dry run: This issue passed the CQ dry run.

[Bernhard Bauer, 2016-04-27 11:38:36]

Neat! I very much like the way the SignInAndSyncView looks now.

https://codereview.chromium.org/1901623002/diff/140001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninActivity.java
(right):

https://codereview.chromium.org/1901623002/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninActivity.java:108:
public int getAccessPoint() {
Javadoc? The implementation is rather trivial, but you could for example mention
where the possible values come from.

(Also, should we have an @IntDef for this?)

https://codereview.chromium.org/1901623002/diff/140001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/signin/SigninAndSyncView.java
(right):

https://codereview.chromium.org/1901623002/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/signin/SigninAndSyncView.java:93:
mDescription.setText(viewState.mDescriptionText);
If you wanted to, you could also invert this into a method on ViewState that
takes the description and the buttons.

https://codereview.chromium.org/1901623002/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/signin/SigninAndSyncView.java:126:
public ButtonState(int mVisibility, int mTextResource,
Can you make this constructor private? Also, having an optional parameter as the
first parameter is a bit confusing. Also also, the parameters themselves should
be named just |visibility|, |textResource| etc.

https://codereview.chromium.org/1901623002/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/signin/SigninAndSyncView.java:128:
this.mVisibility = mVisibility;
You could verify here that the button is visible if and only if the other
parameters don't have the default values (-1 and null).

https://codereview.chromium.org/1901623002/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/signin/SigninAndSyncView.java:167:
mAccessPoint == SigninAccessPoint.RECENT_TABS ? View.GONE : View.VISIBLE,
Wouldn't you want to return ButtonState.noButton() if the button is GONE?

https://codereview.chromium.org/1901623002/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/signin/SigninAndSyncView.java:227:
public static void enableSync() {
Can this be private?

[PEConn, 2016-04-27 15:39:02]

PTAL at SigninAndSyncView - I decided I could take the 'use the type system to
prevent bad  state' thing a bit further.

https://codereview.chromium.org/1901623002/diff/140001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninActivity.java
(right):

https://codereview.chromium.org/1901623002/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninActivity.java:108:
public int getAccessPoint() {
On 2016/04/27 11:38:36, Bernhard Bauer wrote:
> Javadoc? The implementation is rather trivial, but you could for example
mention
> where the possible values come from.
> 
> (Also, should we have an @IntDef for this?)

It was only used locally, so I made it private (I still created the IntDef
though).

https://codereview.chromium.org/1901623002/diff/140001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/signin/SigninAndSyncView.java
(right):

https://codereview.chromium.org/1901623002/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/signin/SigninAndSyncView.java:93:
mDescription.setText(viewState.mDescriptionText);
On 2016/04/27 11:38:36, Bernhard Bauer wrote:
> If you wanted to, you could also invert this into a method on ViewState that
> takes the description and the buttons.

Done.

https://codereview.chromium.org/1901623002/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/signin/SigninAndSyncView.java:126:
public ButtonState(int mVisibility, int mTextResource,
On 2016/04/27 11:38:36, Bernhard Bauer wrote:
> Can you make this constructor private? Also, having an optional parameter as
the
> first parameter is a bit confusing. Also also, the parameters themselves
should
> be named just |visibility|, |textResource| etc.

Acknowledged.

https://codereview.chromium.org/1901623002/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/signin/SigninAndSyncView.java:128:
this.mVisibility = mVisibility;
On 2016/04/27 11:38:36, Bernhard Bauer wrote:
> You could verify here that the button is visible if and only if the other
> parameters don't have the default values (-1 and null).

Acknowledged.

https://codereview.chromium.org/1901623002/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/signin/SigninAndSyncView.java:167:
mAccessPoint == SigninAccessPoint.RECENT_TABS ? View.GONE : View.VISIBLE,
On 2016/04/27 11:38:36, Bernhard Bauer wrote:
> Wouldn't you want to return ButtonState.noButton() if the button is GONE?

Acknowledged.

https://codereview.chromium.org/1901623002/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/signin/SigninAndSyncView.java:227:
public static void enableSync() {
On 2016/04/27 11:38:36, Bernhard Bauer wrote:
> Can this be private?

Done.

[PEConn, 2016-04-28 09:46:26]

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

[commit-bot: I haz the power, 2016-04-28 09:46:37]

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

[commit-bot: I haz the power, 2016-04-28 10:32:51]

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

[commit-bot: I haz the power, 2016-04-28 10:32:52]

Dry run: This issue passed the CQ dry run.

[Bernhard Bauer, 2016-04-28 12:24:03]

LGTM with a final nit:

https://codereview.chromium.org/1901623002/diff/180001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninActivity.java
(right):

https://codereview.chromium.org/1901623002/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninActivity.java:44:
public @interface AccessPoint {}
It would be awesome to change java_cpp_enum (the script that generates the Java
values for a C++ enum) to create an @IntDef of this form as well. I filed
https://crbug.com/607519 for it.

https://codereview.chromium.org/1901623002/diff/180001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/signin/SigninAndSyncView.java
(right):

https://codereview.chromium.org/1901623002/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/signin/SigninAndSyncView.java:128:
private abstract static class ButtonState {
This could be an interface if there is no concrete method.

[PEConn, 2016-04-28 12:33:52]

https://codereview.chromium.org/1901623002/diff/180001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninActivity.java
(right):

https://codereview.chromium.org/1901623002/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninActivity.java:44:
public @interface AccessPoint {}
On 2016/04/28 12:24:03, Bernhard Bauer wrote:
> It would be awesome to change java_cpp_enum (the script that generates the
Java
> values for a C++ enum) to create an @IntDef of this form as well. I filed
> https://crbug.com/607519 for it.

Acknowledged.

https://codereview.chromium.org/1901623002/diff/180001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/signin/SigninAndSyncView.java
(right):

https://codereview.chromium.org/1901623002/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/signin/SigninAndSyncView.java:128:
private abstract static class ButtonState {
On 2016/04/28 12:24:03, Bernhard Bauer wrote:
> This could be an interface if there is no concrete method.

Done.

[PEConn, 2016-04-28 12:34:10]

The CQ bit was checked by peconn@chromium.org

[commit-bot: I haz the power, 2016-04-28 12:34:23]

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

[commit-bot: I haz the power, 2016-04-28 12:38:55]

Description was changed from

==========
Unified Bookmarks and Recent Tabs signin methods.

This involves moving the Recent Tabs signin method to showing a button that
summons a Signin Activity (previously the Signin Activity was integrated in
the Recent Tabs page) - this is the SigninAndSyncView.

This also provides a Settings link to the Bookmarks and Recent Tabs signin
flows.

Additionally, it replaces the BookmarkSigninActivity with the
AccountSigninActivity.

BUG=583774, 598868, 599736
==========

to

==========
Unified Bookmarks and Recent Tabs signin methods.

This involves moving the Recent Tabs signin method to showing a button that
summons a Signin Activity (previously the Signin Activity was integrated in
the Recent Tabs page) - this is the SigninAndSyncView.

This also provides a Settings link to the Bookmarks and Recent Tabs signin
flows.

Additionally, it replaces the BookmarkSigninActivity with the
AccountSigninActivity.

BUG=583774, 598868, 599736
==========

[commit-bot: I haz the power, 2016-04-28 12:38:56]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2016-04-30 17:17:44]

Patchset 11 (id:??) landed as
https://crrev.com/d830894230c7cde07ff92234c6a4e4424b39e10e
Cr-Commit-Position: refs/heads/master@{#390356}

[commit-bot: I haz the power, 2016-04-30 17:55:19]

Description was changed from

==========
Unified Bookmarks and Recent Tabs signin methods.

This involves moving the Recent Tabs signin method to showing a button that
summons a Signin Activity (previously the Signin Activity was integrated in
the Recent Tabs page) - this is the SigninAndSyncView.

This also provides a Settings link to the Bookmarks and Recent Tabs signin
flows.

Additionally, it replaces the BookmarkSigninActivity with the
AccountSigninActivity.

BUG=583774, 598868, 599736
==========

to

==========
Unified Bookmarks and Recent Tabs signin methods.

This involves moving the Recent Tabs signin method to showing a button that
summons a Signin Activity (previously the Signin Activity was integrated in
the Recent Tabs page) - this is the SigninAndSyncView.

This also provides a Settings link to the Bookmarks and Recent Tabs signin
flows.

Additionally, it replaces the BookmarkSigninActivity with the
AccountSigninActivity.

BUG=583774, 598868, 599736

Committed: https://crrev.com/d830894230c7cde07ff92234c6a4e4424b39e10e
Cr-Commit-Position: refs/heads/master@{#390356}
==========