Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1145313006:
Refactor and improve UserRecoverableErrorHandler for 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. |
DescriptionRefactor and improve UserRecoverableErrorHandler for Play Services.
This change refactors the way that UserRecoverableErrorHandler objects are
constructed, removing the chicken-and-the-egg problem of needing to know the
error code before it existed and allowing greater customization of interactive
responses.
BUG=490710
Committed: https://crrev.com/9edde8f3699f8ddd87190a757d67bc9c8bd7d73c
Cr-Commit-Position: refs/heads/master@{#331118}
Patch Set 1 #
Total comments: 11
Patch Set 2 : Fix incorrect comment from patchset1 #
Total comments: 4
Patch Set 3 : Address comments from dgn@ #
Total comments: 8
Patch Set 4 : Address comments from bauerb@ #Patch Set 5 : Rebase #
Total comments: 2
Patch Set 6 : bauerb@'s suggestions #
Messages
Total messages: 24 (4 generated)
andrewhayden@chromium.org changed reviewers: + bauerb@chromium.org, dgn@chromium.org
bauerb@, dgn@: PTAL. This fixes the remaining issues from the original code review (https://codereview.chromium.org/1136633005/) and I've been able to integrate with downstream code on a private branch, so this API should be usable :)
https://codereview.chromium.org/1145313006/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/externalauth/UserRecoverableErrorHandler.java (right): https://codereview.chromium.org/1145313006/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/externalauth/UserRecoverableErrorHandler.java:129: * returns null. Nope! Returns NO_RESPONSE_REQUIRED.
https://codereview.chromium.org/1145313006/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/externalauth/UserRecoverableErrorHandler.java (right): https://codereview.chromium.org/1145313006/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/externalauth/UserRecoverableErrorHandler.java:47: public void handleError(final Context context, final int errorCode) { It's not really possible to enforce that super is called, right? How about having a package visible method that will do the thread check and call another abstract method that has to be implemented by subclasses? https://codereview.chromium.org/1145313006/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/externalauth/UserRecoverableErrorHandler.java:59: * A handler that displays a System Notification. To avoid repeatedly git cl format for java is different upstream and downstream?! ew! https://codereview.chromium.org/1145313006/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/externalauth/UserRecoverableErrorHandler.java:92: public static class ModalDialog extends UserRecoverableErrorHandler { Wouldn't two constructors do the trick? ModalDialog(Activity activity) ModalDialog(Activity activity, int requestCode, OnCancelListener listener) I don't really see the benefits of this system based on subclassing. The previous implementation didn't seem to have an issue with the listener and the request code. https://codereview.chromium.org/1145313006/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/externalauth/UserRecoverableErrorHandler.java:131: protected int getRequestCode() { it's an int, null can't be returned
https://codereview.chromium.org/1145313006/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/externalauth/UserRecoverableErrorHandler.java (right): https://codereview.chromium.org/1145313006/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/externalauth/UserRecoverableErrorHandler.java:47: public void handleError(final Context context, final int errorCode) { On 2015/05/21 12:50:38, dgn wrote: > It's not really possible to enforce that super is called, right? How about > having a package visible method that will do the thread check and call another > abstract method that has to be implemented by subclasses? I can tighten this up so that the assert is always done, no matter what, sure. https://codereview.chromium.org/1145313006/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/externalauth/UserRecoverableErrorHandler.java:59: * A handler that displays a System Notification. To avoid repeatedly On 2015/05/21 12:50:38, dgn wrote: > git cl format for java is different upstream and downstream?! ew! woo :) https://codereview.chromium.org/1145313006/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/externalauth/UserRecoverableErrorHandler.java:92: public static class ModalDialog extends UserRecoverableErrorHandler { On 2015/05/21 12:50:38, dgn wrote: > Wouldn't two constructors do the trick? > ModalDialog(Activity activity) > ModalDialog(Activity activity, int requestCode, OnCancelListener listener) > > I don't really see the benefits of this system based on subclassing. The > previous implementation didn't seem to have an issue with the listener and the > request code. It's a bit nastier than that, unfortunately. You don't need a request code unless an error occurs, so if you want it in the constructor you'd have to predetermine what that code is going to be and also prepare your OnCancelListener even though an error might not actually occur. It's better to defer these to the time when an error DOES occur, i.e. you know that something bad has happened and you're going to need to recover from it. Basically it's the same chicken-and-the-egg problem we had with the error code: you don't know what to do till you've called Google Play Services, so you can't really put it into the constructor. https://codereview.chromium.org/1145313006/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/externalauth/UserRecoverableErrorHandler.java:131: protected int getRequestCode() { On 2015/05/21 12:50:38, dgn wrote: > it's an int, null can't be returned Done.
non owner lgtm % small comments https://codereview.chromium.org/1145313006/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/externalauth/UserRecoverableErrorHandler.java (right): https://codereview.chromium.org/1145313006/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/externalauth/UserRecoverableErrorHandler.java:47: public void handleError(final Context context, final int errorCode) { On 2015/05/21 12:57:29, Andrew Hayden wrote: > On 2015/05/21 12:50:38, dgn wrote: > > It's not really possible to enforce that super is called, right? How about > > having a package visible method that will do the thread check and call another > > abstract method that has to be implemented by subclasses? > > I can tighten this up so that the assert is always done, no matter what, sure. Actually, it's fine, we control how it is called so we know it's going to be on the ui thread anyway.... https://codereview.chromium.org/1145313006/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/externalauth/UserRecoverableErrorHandler.java:92: public static class ModalDialog extends UserRecoverableErrorHandler { On 2015/05/21 12:57:29, Andrew Hayden wrote: > On 2015/05/21 12:50:38, dgn wrote: > > Wouldn't two constructors do the trick? > > ModalDialog(Activity activity) > > ModalDialog(Activity activity, int requestCode, OnCancelListener listener) > > > > I don't really see the benefits of this system based on subclassing. The > > previous implementation didn't seem to have an issue with the listener and the > > request code. > > It's a bit nastier than that, unfortunately. You don't need a request code > unless an error occurs, so if you want it in the constructor you'd have to > predetermine what that code is going to be and also prepare your > OnCancelListener even though an error might not actually occur. It's better to > defer these to the time when an error DOES occur, i.e. you know that something > bad has happened and you're going to need to recover from it. > > Basically it's the same chicken-and-the-egg problem we had with the error code: > you don't know what to do till you've called Google Play Services, so you can't > really put it into the constructor. ok :( https://codereview.chromium.org/1145313006/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/externalauth/UserRecoverableErrorHandler.java (right): https://codereview.chromium.org/1145313006/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/externalauth/UserRecoverableErrorHandler.java:120: * Optionally, returns an integer request code to pass to not optional https://codereview.chromium.org/1145313006/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/externalauth/UserRecoverableErrorHandler.java:122: * If this method returns a non-null value, then upon completion of the can't be null
I've tried to tighten things up a bit, see what you think of this next patch set. https://codereview.chromium.org/1145313006/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/externalauth/UserRecoverableErrorHandler.java (right): https://codereview.chromium.org/1145313006/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/externalauth/UserRecoverableErrorHandler.java:120: * Optionally, returns an integer request code to pass to On 2015/05/21 13:32:35, dgn wrote: > not optional Done. https://codereview.chromium.org/1145313006/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/externalauth/UserRecoverableErrorHandler.java:122: * If this method returns a non-null value, then upon completion of the On 2015/05/21 13:32:35, dgn wrote: > can't be null Done.
lgtm
https://codereview.chromium.org/1145313006/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/externalauth/UserRecoverableErrorHandler.java (right): https://codereview.chromium.org/1145313006/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/externalauth/UserRecoverableErrorHandler.java:65: protected void prepareToHandle(final Context context, final int errorCode) { I assume some downstream class overrides this? Can you point me to the CL? https://codereview.chromium.org/1145313006/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/externalauth/UserRecoverableErrorHandler.java:139: * subsequent actions, and the activity which will receive the response Indent subsequent lines for @param descriptions.
andrewhayden@google.com changed reviewers: + andrewhayden@google.com
https://codereview.chromium.org/1145313006/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/externalauth/UserRecoverableErrorHandler.java (right): https://codereview.chromium.org/1145313006/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/externalauth/UserRecoverableErrorHandler.java:65: protected void prepareToHandle(final Context context, final int errorCode) { On 2015/05/21 16:06:24, Bernhard Bauer wrote: > I assume some downstream class overrides this? Can you point me to the CL? Currently no, the sole downstream class that uses this technique does not process the response. Arguably, it should, and after this lands I plan to argue that case to the owner. https://codereview.chromium.org/1145313006/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/externalauth/UserRecoverableErrorHandler.java:139: * subsequent actions, and the activity which will receive the response On 2015/05/21 16:06:24, Bernhard Bauer wrote: > Indent subsequent lines for @param descriptions. This is what git cl-format produced. I'm happy to update if you wish, but I thought the formatter would take care of it?
https://codereview.chromium.org/1145313006/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/externalauth/UserRecoverableErrorHandler.java (right): https://codereview.chromium.org/1145313006/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/externalauth/UserRecoverableErrorHandler.java:139: * subsequent actions, and the activity which will receive the response On 2015/05/21 16:15:16, andrewhayden wrote: > On 2015/05/21 16:06:24, Bernhard Bauer wrote: > > Indent subsequent lines for @param descriptions. > > This is what git cl-format produced. I'm happy to update if you wish, but I > thought the formatter would take care of it? Also, please specify what alignment you want for the indentation.
https://codereview.chromium.org/1145313006/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/externalauth/UserRecoverableErrorHandler.java (right): https://codereview.chromium.org/1145313006/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/externalauth/UserRecoverableErrorHandler.java:65: protected void prepareToHandle(final Context context, final int errorCode) { On 2015/05/21 16:15:16, andrewhayden wrote: > On 2015/05/21 16:06:24, Bernhard Bauer wrote: > > I assume some downstream class overrides this? Can you point me to the CL? > > Currently no, the sole downstream class that uses this technique does not > process the response. Arguably, it should, and after this lands I plan to argue > that case to the owner. So, there is at least a plan to override this...? In any case, can we move this method to ModalDialog? Otherwise it just looks weird to have two (!) stubs for subclasses to override. Designing for extensibility is nice and well, but when the depth of your class hierarchy is reflected in the base class, something is going wrong ;-) https://codereview.chromium.org/1145313006/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/externalauth/UserRecoverableErrorHandler.java:139: * subsequent actions, and the activity which will receive the response On 2015/05/21 16:23:11, andrewhayden wrote: > On 2015/05/21 16:15:16, andrewhayden wrote: > > On 2015/05/21 16:06:24, Bernhard Bauer wrote: > > > Indent subsequent lines for @param descriptions. > > > > This is what git cl-format produced. I'm happy to update if you wish, but I > > thought the formatter would take care of it? Huh. I have the suspicion that git-cl-format does weird things for Java code (Java support in clang-format is much less mature, as it originally was written just for languages processed by Clang, i.e. C-like ones). > Also, please specify what alignment you want for the indentation. Align with the beginning of the text block (i.e. after "activity").
https://codereview.chromium.org/1145313006/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/externalauth/UserRecoverableErrorHandler.java (right): https://codereview.chromium.org/1145313006/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/externalauth/UserRecoverableErrorHandler.java:139: * subsequent actions, and the activity which will receive the response On 2015/05/21 16:46:25, Bernhard Bauer wrote: > On 2015/05/21 16:23:11, andrewhayden wrote: > > On 2015/05/21 16:15:16, andrewhayden wrote: > > > On 2015/05/21 16:06:24, Bernhard Bauer wrote: > > > > Indent subsequent lines for @param descriptions. > > > > > > This is what git cl-format produced. I'm happy to update if you wish, but I > > > thought the formatter would take care of it? > > Huh. I have the suspicion that git-cl-format does weird things for Java code > (Java support in clang-format is much less mature, as it originally was written > just for languages processed by Clang, i.e. C-like ones). Try to file a bug with label:clang-format ?
> So, there is at least a plan to override this...? > > In any case, can we move this method to ModalDialog? Otherwise it just looks > weird to have two (!) stubs for subclasses to override. Designing for > extensibility is nice and well, but when the depth of your class hierarchy is > reflected in the base class, something is going wrong ;-) After an online discussion with bauerb@ discussing pros and cons of various approaches I've pushed the "prepare" method down into ModalDialog and made "handle" there final. So now the chain looks like: [FINAL] UserRecoverableErrorHandler.handleError(...) -> [FINAL] ModalDialog.handle(...) -> [OVERRIDABLE] ModalDialog.handle(... + more params) > Huh. I have the suspicion that git-cl-format does weird things for Java code > (Java support in clang-format is much less mature, as it originally was written > just for languages processed by Clang, i.e. C-like ones). > > > Also, please specify what alignment you want for the indentation. > > Align with the beginning of the text block (i.e. after "activity"). I got rid of all the multi-line @param comments, so this issue is no longer relevant.
bauerb@, PTAL again. dgn@, if you want to have another round of review, please feel free.
Added BUG=490710, which this is now blocking.
Nice! LGTM with one suggestion: https://codereview.chromium.org/1145313006/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/externalauth/UserRecoverableErrorHandler.java (right): https://codereview.chromium.org/1145313006/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/externalauth/UserRecoverableErrorHandler.java:215: protected void handle(final Activity activity, final Context context, final int errorCode, I think I would inline this method into the two-argument handle() one, just to make it really clear which methods to override in a subclass (and because presumably a subclass of ModalDialog wouldn't actually want to override the method that shows the dialog).
https://codereview.chromium.org/1145313006/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/externalauth/UserRecoverableErrorHandler.java (right): https://codereview.chromium.org/1145313006/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/externalauth/UserRecoverableErrorHandler.java:215: protected void handle(final Activity activity, final Context context, final int errorCode, On 2015/05/22 15:17:01, Bernhard Bauer wrote: > I think I would inline this method into the two-argument handle() one, just to > make it really clear which methods to override in a subclass (and because > presumably a subclass of ModalDialog wouldn't actually want to override the > method that shows the dialog). Sounds good, will do. I'll make the SystemNotification equivalent final as well; we should just say that if these behaviors aren't the ones you want, you can create your own subclass of UserRecoverableErrorHandler. Thanks for the suggestion.
The CQ bit was checked by andrewhayden@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from dgn@chromium.org, bauerb@chromium.org Link to the patchset: https://codereview.chromium.org/1145313006/#ps100001 (title: "bauerb@'s suggestions")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1145313006/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/9edde8f3699f8ddd87190a757d67bc9c8bd7d73c Cr-Commit-Position: refs/heads/master@{#331118} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
