Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1976853002:
[Remoting Android] Refactor OAuth Token Fetching Code (Closed)
Description[Remoting Android] Refactor OAuth Token Fetching Code
* OAuthTokenConsumer for each auth token consuming task. The consumer class guarantees that no same tasks can be run at the same time.
* Make connectToHost independent to mToken.
* Remove retry fetching token logic.
* Method for deleting host.
BUG=611625
Committed: https://crrev.com/a5b7e6ed12f4ebccd4e35b21a03330d635a7bda4
Cr-Commit-Position: refs/heads/master@{#393592}
Patch Set 1 #
Total comments: 17
Patch Set 2 : Reviewer's Feedback #
Messages
Total messages: 19 (9 generated)
The CQ bit was checked by yuweih@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1976853002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1976853002/1
yuweih@chromium.org changed reviewers: + lambroslambrou@chromium.org
ptal
https://codereview.chromium.org/1976853002/diff/1/remoting/android/java/src/o... File remoting/android/java/src/org/chromium/chromoting/Chromoting.java (right): https://codereview.chromium.org/1976853002/diff/1/remoting/android/java/src/o... remoting/android/java/src/org/chromium/chromoting/Chromoting.java:398: /** Called to initialize the action bar. */ Not sure how the tab comes but I'll remove it... https://codereview.chromium.org/1976853002/diff/1/remoting/android/java/src/o... remoting/android/java/src/org/chromium/chromoting/Chromoting.java:520: showAuthErrorMessage(error); Looks like I also need to do updateHostListView here... https://codereview.chromium.org/1976853002/diff/1/remoting/android/java/src/o... File remoting/android/java/src/org/chromium/chromoting/HostListManager.java (right): https://codereview.chromium.org/1976853002/diff/1/remoting/android/java/src/o... remoting/android/java/src/org/chromium/chromoting/HostListManager.java:278: case HttpURLConnection.HTTP_NO_CONTENT: // 204 This is a quick fix for the HostListManager. Turns out the server will respond to the delete operation with 204 status code. https://codereview.chromium.org/1976853002/diff/1/remoting/android/java/src/o... File remoting/android/java/src/org/chromium/chromoting/OAuthTokenConsumer.java (right): https://codereview.chromium.org/1976853002/diff/1/remoting/android/java/src/o... remoting/android/java/src/org/chromium/chromoting/OAuthTokenConsumer.java:66: * retry(). The token may become invalid after some amount of time. Oops... forgot to remove retry()
Description was changed from ========== [Remoting Android] Refactor OAuth Token Fetching Code * Make OAuthTokenConsumer for each auth token consuming task. * Make connectToHost independent to mToken. * Remove retry fetching token logic. * Method for deleting host. BUG=607216 ========== to ========== [Remoting Android] Refactor OAuth Token Fetching Code * OAuthTokenConsumer for each auth token consuming task. The consumer class guarantees that no same tasks can be run at the same time. * Make connectToHost independent to mToken. * Remove retry fetching token logic. * Method for deleting host. BUG=611625 ==========
lgtm https://codereview.chromium.org/1976853002/diff/1/remoting/android/java/src/o... File remoting/android/java/src/org/chromium/chromoting/Chromoting.java (right): https://codereview.chromium.org/1976853002/diff/1/remoting/android/java/src/o... remoting/android/java/src/org/chromium/chromoting/Chromoting.java:520: showAuthErrorMessage(error); On 2016/05/13 00:30:17, Yuwei wrote: > Looks like I also need to do updateHostListView here... I think so, to hide the host-list-loading progress indicator. https://codereview.chromium.org/1976853002/diff/1/remoting/android/java/src/o... File remoting/android/java/src/org/chromium/chromoting/HostListManager.java (right): https://codereview.chromium.org/1976853002/diff/1/remoting/android/java/src/o... remoting/android/java/src/org/chromium/chromoting/HostListManager.java:278: case HttpURLConnection.HTTP_NO_CONTENT: // 204 On 2016/05/13 00:30:17, Yuwei wrote: > This is a quick fix for the HostListManager. Turns out the server will respond > to the delete operation with 204 status code. We should probably replace this switch-statement with testing ranges of HTTP codes. 200-299 are all OK, 400-499 are AUTH_FAILED, 500-599 are SERVICE_UNAVAILABLE, everything else is UNKNOWN. We've had problems in the past where the server returned a different code than expected, so now might be a good time to make this change? Doesn't need to hold up this CL if you want to do it separately. https://codereview.chromium.org/1976853002/diff/1/remoting/android/java/src/o... File remoting/android/java/src/org/chromium/chromoting/OAuthTokenConsumer.java (right): https://codereview.chromium.org/1976853002/diff/1/remoting/android/java/src/o... remoting/android/java/src/org/chromium/chromoting/OAuthTokenConsumer.java:19: private String mLastToken; mLatestToken or mNewestToken or mCurrentToken or just mToken? In English, "last" is ambiguous - it could mean either "latest" or "previous". https://codereview.chromium.org/1976853002/diff/1/remoting/android/java/src/o... remoting/android/java/src/org/chromium/chromoting/OAuthTokenConsumer.java:39: public boolean consume(String account, final OAuthTokenFetcher.Callback callback) { If consume() is called several times with different |callback| parameters, only one of them will actually get run? I assume it's the caller's responsibility to supply the same (or equivalent) callback each time? Maybe update the JavaDoc to clarify this? The callback is used only when this method returns true. https://codereview.chromium.org/1976853002/diff/1/remoting/android/java/src/o... remoting/android/java/src/org/chromium/chromoting/OAuthTokenConsumer.java:45: OAuthTokenFetcher fetcher = new OAuthTokenFetcher(mActivity, account, mTokenScope, Maybe just do: new OAuthTokenFetcher(...).fetch(); to get rid of the temporary variable?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1976853002/diff/1/remoting/android/java/src/o... File remoting/android/java/src/org/chromium/chromoting/HostListManager.java (right): https://codereview.chromium.org/1976853002/diff/1/remoting/android/java/src/o... remoting/android/java/src/org/chromium/chromoting/HostListManager.java:278: case HttpURLConnection.HTTP_NO_CONTENT: // 204 On 2016/05/13 01:11:31, Lambros wrote: > On 2016/05/13 00:30:17, Yuwei wrote: > > This is a quick fix for the HostListManager. Turns out the server will respond > > to the delete operation with 204 status code. > > We should probably replace this switch-statement with testing ranges of HTTP > codes. > 200-299 are all OK, 400-499 are AUTH_FAILED, 500-599 are SERVICE_UNAVAILABLE, > everything else is UNKNOWN. > > We've had problems in the past where the server returned a different code than > expected, so now might be a good time to make this change? > > Doesn't need to hold up this CL if you want to do it separately. > Sure. I'll add a TODO here. BTW I think only 401 is for AUTH_FAILED. 4XX can be not found and incorrect request, etc... https://codereview.chromium.org/1976853002/diff/1/remoting/android/java/src/o... File remoting/android/java/src/org/chromium/chromoting/OAuthTokenConsumer.java (right): https://codereview.chromium.org/1976853002/diff/1/remoting/android/java/src/o... remoting/android/java/src/org/chromium/chromoting/OAuthTokenConsumer.java:39: public boolean consume(String account, final OAuthTokenFetcher.Callback callback) { On 2016/05/13 01:11:31, Lambros wrote: > If consume() is called several times with different |callback| parameters, only > one of them will actually get run? Right. Only the first callback will get run. > I assume it's the caller's responsibility to supply the same (or equivalent) > callback each time? Yes. Except that the closure can capture changing variable like hostId. > Maybe update the JavaDoc to clarify this? The callback is used only when this > method returns true. I'll update the JavaDoc to clarify all of these.
Patchset #2 (id:20001) has been deleted
https://codereview.chromium.org/1976853002/diff/1/remoting/android/java/src/o... File remoting/android/java/src/org/chromium/chromoting/Chromoting.java (right): https://codereview.chromium.org/1976853002/diff/1/remoting/android/java/src/o... remoting/android/java/src/org/chromium/chromoting/Chromoting.java:398: /** Called to initialize the action bar. */ On 2016/05/13 00:30:17, Yuwei wrote: > Not sure how the tab comes but I'll remove it... Done. https://codereview.chromium.org/1976853002/diff/1/remoting/android/java/src/o... remoting/android/java/src/org/chromium/chromoting/Chromoting.java:520: showAuthErrorMessage(error); On 2016/05/13 01:11:31, Lambros wrote: > On 2016/05/13 00:30:17, Yuwei wrote: > > Looks like I also need to do updateHostListView here... > > I think so, to hide the host-list-loading progress indicator. Done. https://codereview.chromium.org/1976853002/diff/1/remoting/android/java/src/o... File remoting/android/java/src/org/chromium/chromoting/OAuthTokenConsumer.java (right): https://codereview.chromium.org/1976853002/diff/1/remoting/android/java/src/o... remoting/android/java/src/org/chromium/chromoting/OAuthTokenConsumer.java:19: private String mLastToken; On 2016/05/13 01:11:31, Lambros wrote: > mLatestToken or mNewestToken or mCurrentToken or just mToken? In English, "last" > is ambiguous - it could mean either "latest" or "previous". Done. https://codereview.chromium.org/1976853002/diff/1/remoting/android/java/src/o... remoting/android/java/src/org/chromium/chromoting/OAuthTokenConsumer.java:39: public boolean consume(String account, final OAuthTokenFetcher.Callback callback) { On 2016/05/13 01:11:31, Lambros wrote: > If consume() is called several times with different |callback| parameters, only > one of them will actually get run? > I assume it's the caller's responsibility to supply the same (or equivalent) > callback each time? > Maybe update the JavaDoc to clarify this? The callback is used only when this > method returns true. Added more details. https://codereview.chromium.org/1976853002/diff/1/remoting/android/java/src/o... remoting/android/java/src/org/chromium/chromoting/OAuthTokenConsumer.java:45: OAuthTokenFetcher fetcher = new OAuthTokenFetcher(mActivity, account, mTokenScope, On 2016/05/13 01:11:31, Lambros wrote: > Maybe just do: > new OAuthTokenFetcher(...).fetch(); > to get rid of the temporary variable? Done. https://codereview.chromium.org/1976853002/diff/1/remoting/android/java/src/o... remoting/android/java/src/org/chromium/chromoting/OAuthTokenConsumer.java:66: * retry(). The token may become invalid after some amount of time. On 2016/05/13 00:30:17, Yuwei wrote: > Oops... forgot to remove retry() Done.
The CQ bit was checked by yuweih@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from lambroslambrou@chromium.org Link to the patchset: https://codereview.chromium.org/1976853002/#ps40001 (title: "Reviewer's Feedback")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1976853002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1976853002/40001
Message was sent while issue was closed.
Description was changed from ========== [Remoting Android] Refactor OAuth Token Fetching Code * OAuthTokenConsumer for each auth token consuming task. The consumer class guarantees that no same tasks can be run at the same time. * Make connectToHost independent to mToken. * Remove retry fetching token logic. * Method for deleting host. BUG=611625 ========== to ========== [Remoting Android] Refactor OAuth Token Fetching Code * OAuthTokenConsumer for each auth token consuming task. The consumer class guarantees that no same tasks can be run at the same time. * Make connectToHost independent to mToken. * Remove retry fetching token logic. * Method for deleting host. BUG=611625 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== [Remoting Android] Refactor OAuth Token Fetching Code * OAuthTokenConsumer for each auth token consuming task. The consumer class guarantees that no same tasks can be run at the same time. * Make connectToHost independent to mToken. * Remove retry fetching token logic. * Method for deleting host. BUG=611625 ========== to ========== [Remoting Android] Refactor OAuth Token Fetching Code * OAuthTokenConsumer for each auth token consuming task. The consumer class guarantees that no same tasks can be run at the same time. * Make connectToHost independent to mToken. * Remove retry fetching token logic. * Method for deleting host. BUG=611625 Committed: https://crrev.com/a5b7e6ed12f4ebccd4e35b21a03330d635a7bda4 Cr-Commit-Position: refs/heads/master@{#393592} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/a5b7e6ed12f4ebccd4e35b21a03330d635a7bda4 Cr-Commit-Position: refs/heads/master@{#393592} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
