Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2360783004:
Get FragmentManager directly from AccountSigninView.Delegate. (Closed)
DescriptionGet FragmentManager directly from AccountSigninView.Delegate.
Luckily both implementers of AccountSigninView.Delegate already implement
getFragmentManager() methods, so this change really is minimal.
BUG=646978
Committed: https://crrev.com/482cb50371df51638505bf32bebb46fc2cb8c03e
Cr-Commit-Position: refs/heads/master@{#421162}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Updated comments. #Messages
Total messages: 24 (12 generated)
peconn@chromium.org changed reviewers: + gogerald@chromium.org, mvanouwerkerk@chromium.org
This is a potential bug for a crasher on Dev and Beta, PTAL!
The CQ bit was checked by peconn@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I am not sure this solves the problem, could you somehow justify it (in theory)? According to the doc (https://developer.android.com/reference/android/app/Fragment.html#getFragment...), getFragmentManager will be non-null slightly before getActivity(). Here are ideas I found in stackoverflow for your reference http://stackoverflow.com/questions/11631408/android-fragment-getactivity-some....
On 2016/09/22 14:44:30, gogerald1 wrote: > I am not sure this solves the problem, could you somehow justify it (in theory)? > > According to the doc > (https://developer.android.com/reference/android/app/Fragment.html#getFragment...), > getFragmentManager will be non-null slightly before getActivity(). > > Here are ideas I found in stackoverflow for your reference > http://stackoverflow.com/questions/11631408/android-fragment-getactivity-some.... I still can't track down why the bug is happening (I don't think getActivity() should ever be null while showConfirmSigninPagePreviousAccountCheck() is called). The bug wasn't occurring in the previous beta (53.0.2785.113) and here are all the changes to 'chrome/android/java/src/org/chromium/chrome/browser/signin' between that version and the version the bug started (54.0.2840.25): https://chromium.googlesource.com/chromium/src/+log/53.0.2785.113..54.0.2840.... The only change that looked suspicious was https://codereview.chromium.org/2154733002/, and this change undoes the part of that change that touches this code.
lgtm with nits https://codereview.chromium.org/2360783004/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninView.java (right): https://codereview.chromium.org/2360783004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninView.java:85: * Provides an Activity for the View to checked GMSCore version. Not sure what this sentence was meant to be exactly, it doesn't seem to flow. https://codereview.chromium.org/2360783004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninView.java:92: * implementers is a Fragment where getActivity() may return null. The interface is not supposed to know anything about specific implementers. Could you rephrase this doc a bit to describe the problem it addresses without saying as much about the implementers?
https://codereview.chromium.org/2360783004/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninView.java (right): https://codereview.chromium.org/2360783004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninView.java:85: * Provides an Activity for the View to checked GMSCore version. On 2016/09/22 15:10:57, Michael van Ouwerkerk wrote: > Not sure what this sentence was meant to be exactly, it doesn't seem to flow. Done. https://codereview.chromium.org/2360783004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountSigninView.java:92: * implementers is a Fragment where getActivity() may return null. On 2016/09/22 15:10:57, Michael van Ouwerkerk wrote: > The interface is not supposed to know anything about specific implementers. > Could you rephrase this doc a bit to describe the problem it addresses without > saying as much about the implementers? Done.
gogerald@, what do you recommend doing with this? Do you have any other ideas for the cause of this bug?
Hi Peter, sorry for late response. I could not reproduce this problem either, but from the Android doc, that's possible. I guess this issue only happens in first run sign in, so it might be better to overwrite AccountSigninView.Delegate.getActivity in AccountFirstRunFragment since getPageDelegate() returns the FirstRunActivity in AccountFirstRunFragment. I think this is better because it is reliable. From the doc getFragmentManager could also be null which might just make this issue become even more difficult to reproduce but not resolve it. WDYT?
On 2016/09/26 15:49:21, gogerald1 wrote: > Hi Peter, sorry for late response. I could not reproduce this problem either, > but from the Android doc, that's possible. I guess this issue only happens in > first run sign in, so it might be better to overwrite > AccountSigninView.Delegate.getActivity in AccountFirstRunFragment since > getPageDelegate() returns the FirstRunActivity in AccountFirstRunFragment. > I think this is better because it is reliable. From the doc getFragmentManager > could also be null which might just make this issue become even more difficult > to reproduce but not resolve it. WDYT? Unfortunately Fragment.getActivity() is final, so you can't overwrite AccountFirstRunFragment.getActivity(). I was wondering about getFragmentManager returning null, but everything seems to have worked previously.
We might could rename the interface a little bit, like getAssociateActivity() since both places have to overwrite it. Not sure why it has worked previously, might be difficult to reproduce with not long enough exposure or timing is correct. Anyway, I still lightly prefer reliable version, but it depends on you if it has worked previously, the code lgtm.
The CQ bit was checked by peconn@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by peconn@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mvanouwerkerk@chromium.org Link to the patchset: https://codereview.chromium.org/2360783004/#ps20001 (title: "Updated comments.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Get FragmentManager directly from AccountSigninView.Delegate. Luckily both implementers of AccountSigninView.Delegate already implement getFragmentManager() methods, so this change really is minimal. BUG=646978 ========== to ========== Get FragmentManager directly from AccountSigninView.Delegate. Luckily both implementers of AccountSigninView.Delegate already implement getFragmentManager() methods, so this change really is minimal. BUG=646978 Committed: https://crrev.com/482cb50371df51638505bf32bebb46fc2cb8c03e Cr-Commit-Position: refs/heads/master@{#421162} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/482cb50371df51638505bf32bebb46fc2cb8c03e Cr-Commit-Position: refs/heads/master@{#421162} |
