Unified Bookmarks and Recent Tabs signin methods.

chrome/android/java/src/org/chromium/chrome/browser/signin/SigninAndSyncView.java

Index: chrome/android/java/src/org/chromium/chrome/browser/signin/SigninAndSyncView.java
diff --git a/chrome/android/java/src/org/chromium/chrome/browser/signin/SigninAndSyncView.java b/chrome/android/java/src/org/chromium/chrome/browser/signin/SigninAndSyncView.java
new file mode 100644
index 0000000000000000000000000000000000000000..9c51b41773e7f74aad774c0ed22e6f95805751a9
--- /dev/null
+++ b/chrome/android/java/src/org/chromium/chrome/browser/signin/SigninAndSyncView.java
@@ -0,0 +1,197 @@
+// Copyright 2016 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+package org.chromium.chrome.browser.signin;
+
+import android.content.Context;
+import android.view.LayoutInflater;
+import android.view.View;
+import android.widget.Button;
+import android.widget.FrameLayout;
+import android.widget.TextView;
+
+import org.chromium.base.Log;
+import org.chromium.chrome.R;
+import org.chromium.chrome.browser.signin.SigninManager.SignInStateObserver;
+import org.chromium.chrome.browser.sync.ProfileSyncService;
+import org.chromium.sync.AndroidSyncSettings;
+import org.chromium.sync.AndroidSyncSettings.AndroidSyncSettingsObserver;
+import org.chromium.sync.signin.ChromeSigninController;
+
+/**
+ * A view that links to a signin activity if the users not signed in or to sync settings if the
+ * user is signed in but sync is disabled.
+ */
+public class SigninAndSyncView extends FrameLayout
+        implements AndroidSyncSettingsObserver, SignInStateObserver {
+    private static final String TAG = "SigninAndSyncView";
+    private final Context mContext;

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

Could you use getContext() in place of this?

[PEConn, 2016/04/25 11:21:23]

Done.

+    private final Listener mListener;
+    private final int mAccessPoint;
+    private final SigninManager mSigninManager;
+
+    private final TextView mTitle;
+    private final TextView mDescription;
+    private final Button mNegativeButton;
+    private final Button mPositiveButton;
+
+    /**
+     * A listener for the container of the SigninAndSyncView to be informed of certain user
+     * interactions.
+     */
+    public interface Listener {
+        /**
+         * The user has pressed 'no thanks' and expects the view to be removed from its parent.
+         */
+        public void onViewDismissed();
+    }
+
+    /**
+     * Constructor for use from Java.
+     */
+    public SigninAndSyncView(Context context, Listener listener, int accessPoint) {
+        // TODO(peconn): Simplify BookmarkPromoHeader
+        super(context);
+        mContext = context;
+        mListener = listener;
+        mAccessPoint = accessPoint;
+
+        assert mAccessPoint == SigninAccessPoint.BOOKMARK_MANAGER
+                || mAccessPoint == SigninAccessPoint.RECENT_TABS
+                : "SigninAndSyncView only has strings for bookmark manager and recent tabs.";
+
+        mSigninManager = SigninManager.get(mContext);
+
+        addView(LayoutInflater.from(mContext)
+                .inflate(R.layout.signin_and_sync_view, this, false));
+
+        mTitle = (TextView) findViewById(R.id.title);
+        mDescription = (TextView) findViewById(R.id.description);
+        mNegativeButton = (Button) findViewById(R.id.no_thanks);
+        mPositiveButton = (Button) findViewById(R.id.sign_in);
+
+        mTitle.setText(accessPoint == SigninAccessPoint.BOOKMARK_MANAGER
+                ? R.string.sync_your_bookmarks : R.string.sync_your_recent_tabs);
+
+        // We don't need to call update() here as it will be called in onAttachedToWindow().
+    }
+
+    private void update() {
+        setupRelevantView();

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

Inline this?

[PEConn, 2016/04/25 11:21:23]

Done.

+    }
+
+    private void setupRelevantView() {
+        mNegativeButton.setVisibility(View.VISIBLE);

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

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. 

[PEConn, 2016/04/25 11:21:23]

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.

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

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.

[PEConn, 2016/04/27 10:19:06]

Done.

+        mPositiveButton.setVisibility(View.VISIBLE);
+        mPositiveButton.setEnabled(true);
+
+        if (!isSignedIn()) {
+            setupSigninView();
+        } else {
+            setupSyncView();
+        }
+    }
+
+    private void setupSigninView() {
+        Log.d(TAG, "Creating a signin view.");
+
+        // TODO(peconn): remove IDS_CONFIRM_ACCOUNT_CHANGE_DIALOG_SIGNIN,
+        // IDS_CHOOSE_ACCOUNT_SIGN_IN, IDS_BOOKMARK_SIGN_IN_PROMO_SIGN_IN
+        mDescription.setText(mAccessPoint == SigninAccessPoint.BOOKMARK_MANAGER
+                ? R.string.bookmark_sign_in_promo_description
+                : R.string.recent_tabs_sign_in_promo_description);
+        mPositiveButton.setText(R.string.sign_in_button);
+        mNegativeButton.setText(R.string.no_thanks);
+
+        mNegativeButton.setOnClickListener(new OnClickListener() {
+            @Override
+            public void onClick(View view) {
+                mListener.onViewDismissed();
+            }
+        });
+
+        mPositiveButton.setOnClickListener(new OnClickListener() {
+            @Override
+            public void onClick(View view) {
+                AccountSigninActivity.startAccountSigninActivity(mContext, mAccessPoint);
+            }
+        });
+    }
+
+    private void setupSyncView() {
+        Log.d(TAG, "Creating a sync view.");
+        mNegativeButton.setVisibility(View.GONE);
+        mPositiveButton.setText(R.string.enable_sync_button);
+
+        if (isSyncEnabled()) {
+            assert mAccessPoint == SigninAccessPoint.RECENT_TABS
+                    : "SigninAndSyncView should not be showing if signed in from bookmarks";
+            mDescription.setText(R.string.ntp_recent_tabs_sync_promo_instructions);
+            mPositiveButton.setVisibility(View.GONE);
+        } else {
+            mDescription.setText(mAccessPoint == SigninAccessPoint.BOOKMARK_MANAGER
+                    ? R.string.bookmarks_sync_promo_enable_sync
+                    : R.string.recent_tabs_sync_promo_enable_sync);
+            mPositiveButton.setOnClickListener(new OnClickListener() {
+                @Override
+                public void onClick(View v) {
+                    mPositiveButton.setEnabled(false);
+                    enableSync();
+                }
+            });
+        }
+    }
+
+    private boolean isSignedIn() {
+        return ChromeSigninController.get(mContext).isSignedIn();
+    }
+
+    private boolean isSyncEnabled() {
+        return AndroidSyncSettings.isSyncEnabled(mContext);
+    }
+
+    public static void enableSync() {
+        ProfileSyncService syncService = ProfileSyncService.get();
+        if (syncService != null) {
+            syncService.requestStart();
+        }
+    }
+
+    @Override
+    protected void onAttachedToWindow() {
+        super.onAttachedToWindow();
+        Log.d(TAG, "Attached to window.");
+        mSigninManager.addSignInStateObserver(this);
+        AndroidSyncSettings.registerObserver(mContext, this);
+        update();
+    }
+
+    @Override
+    protected void onDetachedFromWindow() {
+        super.onDetachedFromWindow();
+        Log.d(TAG, "Detached from window.");
+        mSigninManager.removeSignInStateObserver(this);
+        AndroidSyncSettings.unregisterObserver(mContext, this);
+    }
+
+    // SigninStateObserver
+    @Override
+    public void onSignedIn() {
+        Log.d(TAG, "Notified of a sign in.");
+        update();
+    }
+
+    @Override
+    public void onSignedOut() {
+        Log.d(TAG, "Notified of a sign out.");
+        update();
+    }
+
+    // AndroidSyncStateObserver
+    @Override
+    public void androidSyncSettingsChanged() {
+        Log.d(TAG, "Notified of a sync change.");
+        update();
+    }
+}