Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1566523002:
[Signin] Prepare to remove use of deprecated AMH method in VariationsSerssion. (Closed)
|
Created:
4 years, 11 months ago by maxbogue Modified:
4 years, 11 months ago CC:
chromium-reviews, tim+watch_chromium.org, zea+watch_chromium.org, maxbogue+watch_chromium.org, pvalenzuela+watch_chromium.org, plaree+watch_chromium.org, asvitkine+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Signin] Prepare to remove use of deprecated AMH method in VariationsSerssion.
AccountManagerHelper's synchronous methods have all been deprecated.
This change prepares to convert a downstream synchronous call to
asynchronous. It is part 1/3 in a series.
This change also corrects an erroneous comment in AMH.
BUG=517697
Committed: https://crrev.com/aabe6bf89a063b163a3e5724bd52d5ad95c3c7db
Cr-Commit-Position: refs/heads/master@{#368126}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Use synchronous method for now. #Patch Set 3 : Do the transition to async properly. #
Total comments: 4
Patch Set 4 : Assert restrictMode is not null. #
Total comments: 2
Patch Set 5 : Add comment. #
Messages
Total messages: 21 (7 generated)
Description was changed from ========== [Signin] Prepare to remove use of deprecated AMH method. AccountManagerHelper's synchronous methods have all been deprecated. This change prepares to convert a downstream synchronous call to asynchronous. It is part 1/3 in a series. This change also corrects an erroneous comment in AMH. BUG=517697 ========== to ========== [Signin] Prepare to remove use of deprecated AMH method in VariationsSerssion. AccountManagerHelper's synchronous methods have all been deprecated. This change prepares to convert a downstream synchronous call to asynchronous. It is part 1/3 in a series. This change also corrects an erroneous comment in AMH. BUG=517697 ==========
maxbogue@chromium.org changed reviewers: + nyquist@chromium.org
Hey Tommy, PTAL.
https://codereview.chromium.org/1566523002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/metrics/VariationsSession.java (right): https://codereview.chromium.org/1566523002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/metrics/VariationsSession.java:44: callback.onResult(""); Wouldn't this break the current functionality until the downstream patch lands? Could you instead make this call the other getRestrictMode(Context) for now, until the downstream patch lands? This method will be directly overridden downstream anyway, so whatever's here for now should probably just continue to work.
https://codereview.chromium.org/1566523002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/metrics/VariationsSession.java (right): https://codereview.chromium.org/1566523002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/metrics/VariationsSession.java:44: callback.onResult(""); On 2016/01/05 23:41:30, nyquist wrote: > Wouldn't this break the current functionality until the downstream patch lands? > > Could you instead make this call the other getRestrictMode(Context) for now, > until the downstream patch lands? This method will be directly overridden > downstream anyway, so whatever's here for now should probably just continue to > work. Done.
https://codereview.chromium.org/1566523002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/metrics/VariationsSession.java (right): https://codereview.chromium.org/1566523002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/metrics/VariationsSession.java:44: callback.onResult(""); On 2016/01/06 00:02:43, maxbogue wrote: > On 2016/01/05 23:41:30, nyquist wrote: > > Wouldn't this break the current functionality until the downstream patch > lands? > > > > Could you instead make this call the other getRestrictMode(Context) for now, > > until the downstream patch lands? This method will be directly overridden > > downstream anyway, so whatever's here for now should probably just continue to > > work. > > Done. I think you misunderstood me. I was thinking about ensuring that at all times does the old functionality keep working. I was thinking something like this: protected void getRestrictMode(Context context, Callback<String> callback) { callback.onResult(getRestrictMode(context)); } Otherwise, like it is now, when the downstream patch lands, "" will be returned until the last upstream patch lands.
https://codereview.chromium.org/1566523002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/metrics/VariationsSession.java (right): https://codereview.chromium.org/1566523002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/metrics/VariationsSession.java:44: callback.onResult(""); On 2016/01/06 21:43:59, nyquist wrote: > On 2016/01/06 00:02:43, maxbogue wrote: > > On 2016/01/05 23:41:30, nyquist wrote: > > > Wouldn't this break the current functionality until the downstream patch > > lands? > > > > > > Could you instead make this call the other getRestrictMode(Context) for now, > > > until the downstream patch lands? This method will be directly overridden > > > downstream anyway, so whatever's here for now should probably just continue > to > > > work. > > > > Done. > > I think you misunderstood me. I was thinking about ensuring that at all times > does the old functionality keep working. > > I was thinking something like this: > > protected void getRestrictMode(Context context, Callback<String> callback) { > callback.onResult(getRestrictMode(context)); > } > > Otherwise, like it is now, when the downstream patch lands, "" will be returned > until the last upstream patch lands. Right, fixed now, sorry. I understood what you wanted, I just did it wrong :S
asvitkine@chromium.org changed reviewers: + asvitkine@chromium.org
https://codereview.chromium.org/1566523002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/metrics/VariationsSession.java (right): https://codereview.chromium.org/1566523002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/metrics/VariationsSession.java:29: public void onResult(String restrictMode) { Do we know how long the asynchronous API is supposed to take? https://codereview.chromium.org/1566523002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/metrics/VariationsSession.java:35: nativeStartVariationsSession(mRestrictMode); The new logic is pretty complicated and makes me wonder about what if onResult() is called with null and whether this would result in future calls to never call nativeStartVariationsSession(). Perhaps we can add a separate boolean instead of checking mRestrictMode for null?
https://codereview.chromium.org/1566523002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/metrics/VariationsSession.java (right): https://codereview.chromium.org/1566523002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/metrics/VariationsSession.java:29: public void onResult(String restrictMode) { On 2016/01/06 22:26:15, Alexei Svitkine wrote: > Do we know how long the asynchronous API is supposed to take? Before Android M the 95th percentile is at 3 ms (Signin.AndroidGetAccountsTime_AccountManager). Android M and after the 95th percentile is at 24 ms (Signin.AndroidGetAccountsTime_GoogleAuthUtil). See these two histograms for details: https://uma.googleplex.com/p/chrome/histograms/?dayCount=28&endDate=01-05-201... In addition to this time for the specific call, there's of course the time we use for the callback itself (to schedule the callback to be invoked, etc), but that's usually very quick unless Chrome is busy (which I guess might in fact be the case during startup).
https://codereview.chromium.org/1566523002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/metrics/VariationsSession.java (right): https://codereview.chromium.org/1566523002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/metrics/VariationsSession.java:35: nativeStartVariationsSession(mRestrictMode); On 2016/01/06 22:26:15, Alexei Svitkine wrote: > The new logic is pretty complicated and makes me wonder about what if onResult() > is called with null and whether this would result in future calls to never call > nativeStartVariationsSession(). > > Perhaps we can add a separate boolean instead of checking mRestrictMode for > null? True, if onResult was called with null, that would in fact break things. Instead of adding another boolean though, I'm just going to add an assert that it's not null, since there's no reason it should ever be called with null.
lgtm, but please get approval from asvitkine as well, as this is his area of expertise :-) asvitkine: The downstream part of this change is here: https://chrome-internal-review.googlesource.com/#/c/243666/
lgtm https://codereview.chromium.org/1566523002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/metrics/VariationsSession.java (right): https://codereview.chromium.org/1566523002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/metrics/VariationsSession.java:35: } else if (mRestrictMode != null) { Nit: Add a comment about the null check. Something like: // If |mRestrictMode| is null, it means the asynchronous initialization is still // in progress and nativeStartVariationsSession will be called once that // completes.
https://codereview.chromium.org/1566523002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/metrics/VariationsSession.java (right): https://codereview.chromium.org/1566523002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/metrics/VariationsSession.java:35: } else if (mRestrictMode != null) { On 2016/01/07 16:24:19, Alexei Svitkine wrote: > Nit: Add a comment about the null check. > > Something like: > > // If |mRestrictMode| is null, it means the asynchronous initialization is still > // in progress and nativeStartVariationsSession will be called once that > // completes. Done.
The CQ bit was checked by maxbogue@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nyquist@chromium.org, asvitkine@chromium.org Link to the patchset: https://codereview.chromium.org/1566523002/#ps80001 (title: "Add comment.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1566523002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1566523002/80001
Message was sent while issue was closed.
Description was changed from ========== [Signin] Prepare to remove use of deprecated AMH method in VariationsSerssion. AccountManagerHelper's synchronous methods have all been deprecated. This change prepares to convert a downstream synchronous call to asynchronous. It is part 1/3 in a series. This change also corrects an erroneous comment in AMH. BUG=517697 ========== to ========== [Signin] Prepare to remove use of deprecated AMH method in VariationsSerssion. AccountManagerHelper's synchronous methods have all been deprecated. This change prepares to convert a downstream synchronous call to asynchronous. It is part 1/3 in a series. This change also corrects an erroneous comment in AMH. BUG=517697 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== [Signin] Prepare to remove use of deprecated AMH method in VariationsSerssion. AccountManagerHelper's synchronous methods have all been deprecated. This change prepares to convert a downstream synchronous call to asynchronous. It is part 1/3 in a series. This change also corrects an erroneous comment in AMH. BUG=517697 ========== to ========== [Signin] Prepare to remove use of deprecated AMH method in VariationsSerssion. AccountManagerHelper's synchronous methods have all been deprecated. This change prepares to convert a downstream synchronous call to asynchronous. It is part 1/3 in a series. This change also corrects an erroneous comment in AMH. BUG=517697 Committed: https://crrev.com/aabe6bf89a063b163a3e5724bd52d5ad95c3c7db Cr-Commit-Position: refs/heads/master@{#368126} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/aabe6bf89a063b163a3e5724bd52d5ad95c3c7db Cr-Commit-Position: refs/heads/master@{#368126} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
