Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1136633005:
Add a new utility method to check the availability of Google Play Services. (Closed)
|
Created:
5 years, 7 months ago by Andrew Hayden (chromium.org) Modified:
5 years, 7 months ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd a new utility method to check the availability of Google Play Services.
All code needing to check Google Play Services availability should migrate to
this API and specify an explicit failure handling policy. Several policies are
provided by default, including a "silent" policy that is identical in behavior
to what happens today in most code using Google Play Services.
In addition to the "silent" policy, the method supports standardized mechanisms
from Google Play such as a modal dialog or a system notification.
BUG=
Committed: https://crrev.com/31af4784464fac0898e909bfb2d7972ae55c106b
Cr-Commit-Position: refs/heads/master@{#330746}
Patch Set 1 #
Total comments: 1
Patch Set 2 : Use verbose logging instead of debug logging #Patch Set 3 : Use verbose logging and non-static boolean field #
Total comments: 26
Patch Set 4 : Extract enum to helper class and clean up logic accordingly #
Total comments: 4
Messages
Total messages: 21 (6 generated)
andrewhayden@google.com changed reviewers: + andrewhayden@google.com
https://codereview.chromium.org/1136633005/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/externalauth/ExternalAuthUtils.java (right): https://codereview.chromium.org/1136633005/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/externalauth/ExternalAuthUtils.java:227: Log.d(TAG, "Unable to use Google Play Services: " This is potential log spam if there's a signing problem with the APK. Drop this to verbose or get rid of it.
andrewhayden@google.com changed reviewers: + dgn@chromium.org
andrewhayden@google.com changed reviewers: + bauerb@chromium.org
Bernhard and Nicolas, can you PTAL?
dgn@chromium.org changed reviewers: - bauerb@chromium.org
So there are 2 different ways of displaying an error, but only one can be shown until the next reboot? How do we choose which one is displayed if 2 parts in the code decide to display 2 different errors? The one displayed might also vary from one run to the other. I'm not sure why having the 2 options is useful. Cast (CastExtensionHandler.java) uses Google Play Services. Should the check be used there? https://codereview.chromium.org/1136633005/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/externalauth/ExternalAuthUtils.java (right): https://codereview.chromium.org/1136633005/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/externalauth/ExternalAuthUtils.java:36: private static final String TAG = "ExternalAuthUtils"; tag should look like "cr.Foo" https://codereview.chromium.org/1136633005/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/externalauth/ExternalAuthUtils.java:222: final int errorCode = GooglePlayServicesUtil.isGooglePlayServicesAvailable(context); use GoogleApiAvailability, applies everywhere GooglePlayServicesUtil is used in this patch. https://codereview.chromium.org/1136633005/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/externalauth/ExternalAuthUtils.java:227: Log.v(TAG, "Unable to use Google Play Services: " nit: string formatting? https://codereview.chromium.org/1136633005/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/externalauth/ExternalAuthUtils.java:239: -1).show(); nit: A comment to explain the -1 please? Inline could do. After a few clicks in the doc I can see that it means that we don't care about the result but still... https://codereview.chromium.org/1136633005/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/externalauth/ExternalAuthUtils.java:245: Log.w(TAG, "Unsupported error handling policy: " %s, and the toString call should not be needed. https://codereview.chromium.org/1136633005/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/externalauth/ExternalAuthUtils.java:251: if (Looper.getMainLooper().getThread() == Thread.currentThread()) { Looks like ThreadUtils.runOnUiThread() does that check for you
bauerb@chromium.org changed reviewers: + bauerb@chromium.org
https://codereview.chromium.org/1136633005/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/externalauth/ExternalAuthUtils.java (right): https://codereview.chromium.org/1136633005/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/externalauth/ExternalAuthUtils.java:150: return false; BTW, I think the internal subclass can now be merged into this class, as AFAICT the only reason it was downstream was because it dependend on Google Play Services back when we didn't have them upstream. https://codereview.chromium.org/1136633005/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/externalauth/ExternalAuthUtils.java:220: throw new IllegalArgumentException("failureHandlingPolicy must be non-null"); I'm not a big fan of checks like this. If failureHandlingPolicy is in fact null, you will crash below anyway, and the stack trace will tell you the same thing, but without contributing strings and code to the binary. https://codereview.chromium.org/1136633005/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/externalauth/ExternalAuthUtils.java:224: return true; // Hooray! Nit: Two spaces before comments. https://codereview.chromium.org/1136633005/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/externalauth/ExternalAuthUtils.java:236: case DISPLAY_DIALOG: I think the proper OO way to do this would be via polymor phism; changing behavior based on a private enum seems a bit like a code smell to me. Another indicator: two out of three variants of the FailureHandlingPolicy don't actually use the activity, but all of them have it as a member. https://codereview.chromium.org/1136633005/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/externalauth/ExternalAuthUtils.java:238: failureHandlingPolicy.mActivity, This seems to be indented weirdly. https://codereview.chromium.org/1136633005/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/externalauth/ExternalAuthUtils.java:251: if (Looper.getMainLooper().getThread() == Thread.currentThread()) { ThreadUtils.runOnUiThread() does that for you. Edit: Jinxed :)
https://codereview.chromium.org/1136633005/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/externalauth/ExternalAuthUtils.java (right): https://codereview.chromium.org/1136633005/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/externalauth/ExternalAuthUtils.java:150: return false; On 2015/05/19 17:48:01, Bernhard Bauer wrote: > BTW, I think the internal subclass can now be merged into this class, as AFAICT > the only reason it was downstream was because it dependend on Google Play > Services back when we didn't have them upstream. We are still using internal only stuff there. https://codereview.chromium.org/1136633005/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/externalauth/ExternalAuthUtils.java:251: if (Looper.getMainLooper().getThread() == Thread.currentThread()) { On 2015/05/19 17:48:01, Bernhard Bauer wrote: > ThreadUtils.runOnUiThread() does that for you. > > Edit: Jinxed :) Damn that's not fair :p
OK, new patchset up. dgn@: Yes, the cast code should use this. Not sure what the right pattern is there just yet - perhaps a silent or system-notification check to determine whether or not to show the cast icon. If the icon is shown and play services aren't available, that seems like a modal dialog use case to me. https://codereview.chromium.org/1136633005/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/externalauth/ExternalAuthUtils.java (right): https://codereview.chromium.org/1136633005/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/externalauth/ExternalAuthUtils.java:36: private static final String TAG = "ExternalAuthUtils"; On 2015/05/19 17:21:00, dgn wrote: > tag should look like "cr.Foo" Switched to Log.makeTag(...) https://codereview.chromium.org/1136633005/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/externalauth/ExternalAuthUtils.java:150: return false; On 2015/05/19 17:48:01, Bernhard Bauer wrote: > BTW, I think the internal subclass can now be merged into this class, as AFAICT > the only reason it was downstream was because it dependend on Google Play > Services back when we didn't have them upstream. See dgn@'s comment below, we still use internal-only stuff downstream so the internal subclass is still needed for downstream Chrome. https://codereview.chromium.org/1136633005/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/externalauth/ExternalAuthUtils.java:150: return false; On 2015/05/19 17:59:52, dgn wrote: > On 2015/05/19 17:48:01, Bernhard Bauer wrote: > > BTW, I think the internal subclass can now be merged into this class, as > AFAICT > > the only reason it was downstream was because it dependend on Google Play > > Services back when we didn't have them upstream. > > We are still using internal only stuff there. ^-- What he said :) https://codereview.chromium.org/1136633005/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/externalauth/ExternalAuthUtils.java:220: throw new IllegalArgumentException("failureHandlingPolicy must be non-null"); On 2015/05/19 17:48:01, Bernhard Bauer wrote: > I'm not a big fan of checks like this. If failureHandlingPolicy is in fact null, > you will crash below anyway, and the stack trace will tell you the same thing, > but without contributing strings and code to the binary. Sounds reasonable to me, I like the thought process. Done. https://codereview.chromium.org/1136633005/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/externalauth/ExternalAuthUtils.java:222: final int errorCode = GooglePlayServicesUtil.isGooglePlayServicesAvailable(context); On 2015/05/19 17:21:00, dgn wrote: > use GoogleApiAvailability, applies everywhere GooglePlayServicesUtil is used in > this patch. I think we took a look at this and found that upstream Chromium only has Google Play Services 6.x, which doesn't yet have the 7.x GoogleApiAvailability. For posterity: https://developer.android.com/reference/com/google/android/gms/common/GoogleA... https://code.google.com/p/chromium/codesearch#chromium/src/third_party/androi... https://codereview.chromium.org/1136633005/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/externalauth/ExternalAuthUtils.java:224: return true; // Hooray! On 2015/05/19 17:48:01, Bernhard Bauer wrote: > Nit: Two spaces before comments. git cl format disagreed, I think, but maybe I accidentally deleted one. I'll re-run. https://codereview.chromium.org/1136633005/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/externalauth/ExternalAuthUtils.java:227: Log.v(TAG, "Unable to use Google Play Services: " On 2015/05/19 17:21:00, dgn wrote: > nit: string formatting? Done. https://codereview.chromium.org/1136633005/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/externalauth/ExternalAuthUtils.java:236: case DISPLAY_DIALOG: On 2015/05/19 17:48:01, Bernhard Bauer wrote: > I think the proper OO way to do this would be via polymor > phism; changing behavior based on a private enum seems a bit like a code smell > to me. Another indicator: two out of three variants of the FailureHandlingPolicy > don't actually use the activity, but all of them have it as a member. I've refactored into a class with subclasses: UserRecoverableErrorHandler. It's a big bigger, but definitely cleaner. https://codereview.chromium.org/1136633005/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/externalauth/ExternalAuthUtils.java:238: failureHandlingPolicy.mActivity, On 2015/05/19 17:48:00, Bernhard Bauer wrote: > This seems to be indented weirdly. Code is gone. https://codereview.chromium.org/1136633005/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/externalauth/ExternalAuthUtils.java:239: -1).show(); On 2015/05/19 17:21:00, dgn wrote: > nit: A comment to explain the -1 please? Inline could do. After a few clicks in > the doc I can see that it means that we don't care about the result but still... Code rebuilt in UserRecoverableErrorHandler, no longer hard-coded to -1 and responsibility pushed back to the caller. https://codereview.chromium.org/1136633005/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/externalauth/ExternalAuthUtils.java:245: Log.w(TAG, "Unsupported error handling policy: " On 2015/05/19 17:21:00, dgn wrote: > %s, and the toString call should not be needed. Code deleted. https://codereview.chromium.org/1136633005/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/externalauth/ExternalAuthUtils.java:251: if (Looper.getMainLooper().getThread() == Thread.currentThread()) { On 2015/05/19 17:59:52, dgn wrote: > On 2015/05/19 17:48:01, Bernhard Bauer wrote: > > ThreadUtils.runOnUiThread() does that for you. > > > > Edit: Jinxed :) > > Damn that's not fair :p Done :)
Nice, LGTM!
The CQ bit was checked by andrewhayden@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1136633005/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/31af4784464fac0898e909bfb2d7972ae55c106b Cr-Commit-Position: refs/heads/master@{#330746}
Message was sent while issue was closed.
Looking at https://developer.android.com/google/auth/api-client.html#HandlingFailures, there are some errors that might require the user to sign in or stuff like that. Is it something we should take into account here? https://codereview.chromium.org/1136633005/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/externalauth/ExternalAuthUtils.java (right): https://codereview.chromium.org/1136633005/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/externalauth/ExternalAuthUtils.java:160: * handling policy will be applied at most one time per browser startup (i.e., a dialog or dialog can now be shown more than once https://codereview.chromium.org/1136633005/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/externalauth/ExternalAuthUtils.java:174: // The rest of the method is error-handling bits. The comment is not very useful anymore https://codereview.chromium.org/1136633005/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/externalauth/UserRecoverableErrorHandler.java (right): https://codereview.chromium.org/1136633005/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/externalauth/UserRecoverableErrorHandler.java:93: public static final class ModalDialog extends UserRecoverableErrorHandler { How about : ErrorHandler <|---- Silent <|-----UserRecoverableErrorHandler <|---- ModalDialog <|---- SystemNotification to put mActivity and mErrorCode together? https://codereview.chromium.org/1136633005/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/externalauth/UserRecoverableErrorHandler.java:122: public ModalDialog(Activity activity, int errorCode, int requestCode, Second constructor with -1 as request code and null as cancel listener for when we don't care?
Message was sent while issue was closed.
uh too late
Message was sent while issue was closed.
Sorry, I jumped the gun. I can do a subsequent CL if necessary, responses inline here. On 2015/05/20 16:33:07, dgn wrote: > Looking at > https://developer.android.com/google/auth/api-client.html#HandlingFailures, > there are some errors that might require the user to sign in or stuff like that. > Is it something we should take into account here? I think we want to retain control over what happens. The startResolutionForResult probably can take the user off to a new activity, like signing in as you say. We could add another policy, like "AutoResolve", that deals with this; but generally, in Chrome, I expect we will want to know what level of UI disruption we are causing. Currently, at worst we will show a dialog. If you want this other policy I am happy to add it in a followup CL. https://codereview.chromium.org/1136633005/diff/60001/chrome/android/java/src... > File > chrome/android/java/src/org/chromium/chrome/browser/externalauth/ExternalAuthUtils.java > (right): > > https://codereview.chromium.org/1136633005/diff/60001/chrome/android/java/src... > chrome/android/java/src/org/chromium/chrome/browser/externalauth/ExternalAuthUtils.java:160: > * handling policy will be applied at most one time per browser startup (i.e., a > dialog or > dialog can now be shown more than once I'll fix the typo in a followon. > https://codereview.chromium.org/1136633005/diff/60001/chrome/android/java/src... > chrome/android/java/src/org/chromium/chrome/browser/externalauth/ExternalAuthUtils.java:174: > // The rest of the method is error-handling bits. > The comment is not very useful anymore True. Will fix at the same time as above. > https://codereview.chromium.org/1136633005/diff/60001/chrome/android/java/src... > File > chrome/android/java/src/org/chromium/chrome/browser/externalauth/UserRecoverableErrorHandler.java > (right): > > https://codereview.chromium.org/1136633005/diff/60001/chrome/android/java/src... > chrome/android/java/src/org/chromium/chrome/browser/externalauth/UserRecoverableErrorHandler.java:93: > public static final class ModalDialog extends UserRecoverableErrorHandler { > How about : > > ErrorHandler <|---- Silent > <|-----UserRecoverableErrorHandler <|---- ModalDialog > <|---- SystemNotification > > to put mActivity and mErrorCode together? Potentially yes, but I think it might be class overkill. SystemNotification only needs a Context; ModalDialog needs an Activity. So they're not exactly 1:1 anyway (we could force an Activity instead of a Context in SystemNotification, but that seems wrong). > https://codereview.chromium.org/1136633005/diff/60001/chrome/android/java/src... > chrome/android/java/src/org/chromium/chrome/browser/externalauth/UserRecoverableErrorHandler.java:122: > public ModalDialog(Activity activity, int errorCode, int requestCode, > Second constructor with -1 as request code and null as cancel listener for when > we don't care? I think we should force the caller to make this decision. In some cases they may not care about the result; fine, they can explicitly make that clear by passing -1 and null. Others may actually want to do something different depending on the user's response, they should also be explicit. The overall theme of this review is to force callers to think about what the right behavior is for their Google Play Services dependency; in this spirit, I want the API to be explicit.
Message was sent while issue was closed.
> > https://codereview.chromium.org/1136633005/diff/60001/chrome/android/java/src... > > chrome/android/java/src/org/chromium/chrome/browser/externalauth/UserRecoverableErrorHandler.java:122: > > public ModalDialog(Activity activity, int errorCode, int requestCode, > > Second constructor with -1 as request code and null as cancel listener for when > > we don't care? > > I think we should force the caller to make this decision. In some cases they may not care about the result; fine, they can explicitly make that clear by passing -1 and null. Others may actually want to do something different depending on the user's response, they should also be explicit. The overall theme of this review is to force callers to think about what the right behavior is for their Google Play Services dependency; in this spirit, I want the API to be explicit. Exposing 2 constructors does not seem to go against that theme. Can we at least mention in the documentation that -1 is the value to give when we don't care? Otherwise you have to jump around the source and documentation to determine what you are supposed to give as input. Exposing constant would also help.
Message was sent while issue was closed.
I've uploaded https://codereview.chromium.org/1145313006 to fix the remaining issues pointed out by dgn@ (again, sorry) and to fix problems found while trying to integrate downstream code with the upstream API that landed in this CL. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
