Do not allow sign in if Google play service is absent

Do not allow sign in if Google play service is absent

It is bad user experience to show sign in promotions to users who
eventually cannot sign in anyway, because of missing gmscore. This CL
suppresses sign in promotions by checking the availability of gmscore.

BUG=653615
Committed: https://crrev.com/7aa9383118fa65b813a5f57900cece8b66028a83
Cr-Commit-Position: refs/heads/master@{#423729}

[Ian Wen, 2016-10-06 20:54:41]

ianwen@chromium.org changed reviewers:
+ dfalcantara@chromium.org, gogerald@chromium.org

[Ian Wen, 2016-10-06 20:54:42]

gogerald@chromium.org: Please review changes in FRE/Sign in

dfalcantara@chromium.org: Please review changes in browser code

[Ian Wen, 2016-10-06 20:55:47]

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

[commit-bot: I haz the power, 2016-10-06 20:56:42]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[gone, 2016-10-06 21:25:51]

lgtm

https://chromiumcodereview.appspot.com/2399183002/diff/1/chrome/android/java/...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/MainPreferences.java
(right):

https://chromiumcodereview.appspot.com/2399183002/diff/1/chrome/android/java/...
chrome/android/java/src/org/chromium/chrome/browser/preferences/MainPreferences.java:227:
private boolean showSignIn() {
this sounds like an action.  isUserAllowedToSignIn()?

[Ian Wen, 2016-10-06 21:52:15]

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

[Ian Wen, 2016-10-06 21:52:37]

The CQ bit was unchecked by ianwen@chromium.org

[Ian Wen, 2016-10-06 21:53:46]

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

[commit-bot: I haz the power, 2016-10-06 21:54:17]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-06 22:05:50]

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

[commit-bot: I haz the power, 2016-10-06 22:05:51]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Ian Wen, 2016-10-06 22:28:48]

The CQ bit was checked by ianwen@chromium.org

[Ian Wen, 2016-10-06 22:28:48]

The patchset sent to the CQ was uploaded after l-g-t-m from
dfalcantara@chromium.org
Link to the patchset: https://codereview.chromium.org/2399183002/#ps60001
(title: "fix presubmit")

[commit-bot: I haz the power, 2016-10-06 22:29:14]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-06 23:22:19]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-10-06 23:24:32]

Description was changed from

==========
Do not allow sign in if Google play service is absent

It is bad user experience to show sign in promotions to users who
eventually cannot sign in anyway, because of missing gmscore. This CL
suppresses sign in promotions by checking the availability of gmscore.

BUG=653615
==========

to

==========
Do not allow sign in if Google play service is absent

It is bad user experience to show sign in promotions to users who
eventually cannot sign in anyway, because of missing gmscore. This CL
suppresses sign in promotions by checking the availability of gmscore.

BUG=653615

Committed: https://crrev.com/7aa9383118fa65b813a5f57900cece8b66028a83
Cr-Commit-Position: refs/heads/master@{#423729}
==========

[commit-bot: I haz the power, 2016-10-06 23:24:33]

Patchset 4 (id:??) landed as
https://crrev.com/7aa9383118fa65b813a5f57900cece8b66028a83
Cr-Commit-Position: refs/heads/master@{#423729}

[gogerald1, 2016-10-07 14:19:28]

https://codereview.chromium.org/2399183002/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/signin/SigninManager.java
(right):

https://codereview.chromium.org/2399183002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/signin/SigninManager.java:242:
new UserRecoverableErrorHandler.Silent());
It might be late to respond, but this will skip sign in screens silently no
matter it is because GMS is out of date or not available. This will affect the
sign in rate or user experience in region where GMS is available (even though
other apps might trigger GMS update). Better to talk to ewald@ to confirm it.

We usually remind user update GMS through modal error dialog or system
notification like here
https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr...
and here
https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr....

[Ian Wen, 2016-10-07 17:53:24]

On 2016/10/07 14:19:28, gogerald1 wrote:
>
https://codereview.chromium.org/2399183002/diff/60001/chrome/android/java/src...
> File
> chrome/android/java/src/org/chromium/chrome/browser/signin/SigninManager.java
> (right):
> 
>
https://codereview.chromium.org/2399183002/diff/60001/chrome/android/java/src...
>
chrome/android/java/src/org/chromium/chrome/browser/signin/SigninManager.java:242:
> new UserRecoverableErrorHandler.Silent());
> It might be late to respond, but this will skip sign in screens silently no
> matter it is because GMS is out of date or not available. This will affect the
> sign in rate or user experience in region where GMS is available (even though
> other apps might trigger GMS update). Better to talk to ewald@ to confirm it.
> 
> We usually remind user update GMS through modal error dialog or system
> notification like here
>
https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr...
> and here
>
https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr....

Okay it seems simply checking the availability does not guarantee the version
check. Will investigate more.

[ewald1, 2016-10-07 17:54:24]

On 2016/10/07 14:19:28, gogerald1 wrote:
>
https://codereview.chromium.org/2399183002/diff/60001/chrome/android/java/src...
> File
> chrome/android/java/src/org/chromium/chrome/browser/signin/SigninManager.java
> (right):
> 
>
https://codereview.chromium.org/2399183002/diff/60001/chrome/android/java/src...
>
chrome/android/java/src/org/chromium/chrome/browser/signin/SigninManager.java:242:
> new UserRecoverableErrorHandler.Silent());
> It might be late to respond, but this will skip sign in screens silently no
> matter it is because GMS is out of date or not available. This will affect the
> sign in rate or user experience in region where GMS is available (even though
> other apps might trigger GMS update). Better to talk to ewald@ to confirm it.
> 
> We usually remind user update GMS through modal error dialog or system
> notification like here
>
https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr...
> and here
>
https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr....

We should definitely only skip the sign in screen if GMS is not available in the
region/on the device. If GMS is out of date, we should show a dialogue which
links the user to the Play Store so they can update GMS.

Peter Conn (peconn@) added the code to display the GMS-out-of-date notification
in 54. In our testing, let's please be sure that still works.

[Ian Wen, 2016-10-07 17:56:46]

On 2016/10/07 17:54:24, ewald1 wrote:
> On 2016/10/07 14:19:28, gogerald1 wrote:
> >
>
https://codereview.chromium.org/2399183002/diff/60001/chrome/android/java/src...
> > File
> >
chrome/android/java/src/org/chromium/chrome/browser/signin/SigninManager.java
> > (right):
> > 
> >
>
https://codereview.chromium.org/2399183002/diff/60001/chrome/android/java/src...
> >
>
chrome/android/java/src/org/chromium/chrome/browser/signin/SigninManager.java:242:
> > new UserRecoverableErrorHandler.Silent());
> > It might be late to respond, but this will skip sign in screens silently no
> > matter it is because GMS is out of date or not available. This will affect
the
> > sign in rate or user experience in region where GMS is available (even
though
> > other apps might trigger GMS update). Better to talk to ewald@ to confirm
it.
> > 
> > We usually remind user update GMS through modal error dialog or system
> > notification like here
> >
>
https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr...
> > and here
> >
>
https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr....
> 
> We should definitely only skip the sign in screen if GMS is not available in
the
> region/on the device. If GMS is out of date, we should show a dialogue which
> links the user to the Play Store so they can update GMS.
> 
> Peter Conn (peconn@) added the code to display the GMS-out-of-date
notification
> in 54. In our testing, let's please be sure that still works.

Yes. Ideally my code will only be triggered if gmscore is missing or fake. Yet
now it is also triggered when it is not updated. I need to modify the API that I
am using.

[xing.xu, 2016-11-30 05:38:21]

Strongly agree disable signin when gms is not available or version obsolete. 

I installed latest chromium public on nexus 6p, it crashed with the following
logs:
11-30 13:14:48.361: V/cr_ExternalAuthUtils(21019): [ExternalAuthUtils.java:203]
Unable to use Google Play Services: SERVICE_VERSION_UPDATE_REQUIRED
11-30 13:14:48.365: D/AndroidRuntime(21019): Shutting down VM
11-30 13:14:48.370: E/AndroidRuntime(21019): FATAL EXCEPTION: main
11-30 13:14:48.370: E/AndroidRuntime(21019): Process: org.chromium.chrome, PID:
21019
11-30 13:14:48.370: E/AndroidRuntime(21019): java.lang.NoSuchFieldError: No
field common_google_play_services_update_text of type I in class
Lcom/google/android/gms/R$string; or its superclasses (declaration of
'com.google.android.gms.R$string' appears in base.apk)
11-30 13:14:48.370: E/AndroidRuntime(21019): 	at
com.google.android.gms.common.internal.zzg.zzi(Unknown Source)


And I workaround it like this: 

diff --git
a/chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninView.java
b/chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninView.java
index dd4c1e3..7d4367b 100644
---
a/chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninView.java
+++
b/chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninView.java
@@ -328,7 +328,7 @@ public class AccountSigninView extends FrameLayout
implements ProfileDownloader.
         mSigninChooseView.setVisibility(View.VISIBLE);
 
         setUpCancelButton();
-        updateAccounts();
+        //updateAccounts();
     }
 
     private void showConfirmSigninPage() {