[Android] Add super method calls to onActivityResult overrides [chromium/src : master]
Boris Sazonov (Gerrit)
Ted, please take a look. I'll add owners for chromoting/ and media/ later if you don't have any objections.
Patch set 1:Commit-Queue +1
To view, visit change 1028690. To unsubscribe, or for help writing mail filters, visit settings.
Ted Choc (Gerrit)
1 comment:
Patch Set #1, Line 649:
super.onActivityResult(requestCode, resultCode, data);Using this as an example (since it's simple), but would you expect super.onActivityResult to be called in the case where the activity actually handled it? To me, that seems quite odd.
To me, it seems like the right behavior should be:
if (requestCode != IMPORTANT_SITES_DIALOG_CODE) {
super.onActivityResult(...);
return;
}if (resultCode != RESULT_OK) return;
...
existing code
...------------------
At a fundamental level, it seems like it would be quite dangerous for multiple levels of the activity hierarchy to listen for the same requestCode, so I wouldn't expect call super to be required or even desired (but maybe I'm being overly naive).
The problem arises for AsyncInitializationActivity though because we don't actually call onActivityResult right away.
I wonder if it would be more correct to change NativeInitializationController though to check the return value of onActivityResultWithNative and if false, call super_onActivityResult or something.
To view, visit change 1028690. To unsubscribe, or for help writing mail filters, visit settings.
Boris Sazonov (Gerrit)
Thank you for the review! Warning: super-long comment ahead.
1 comment:
Patch Set #1, Line 649:
super.onActivityResult(requestCode, resultCode, data);
Using this as an example (since it's simple), but would you expect super. […]
I agree that the approach you've proposed won't work "as is" in AsyncInitializationActivity. Unfortunately, I can't just fix this in other places and leave AsyncInitializationActivity as is, because I need functional onActivityResult in FRE fragments and FirstRunActivity subclasses AsyncInitializationActivity.
I think that unconditionally calling super in this specific case should be safe, as FragmentActivity.onActivityResult ignores all calls for request codes which have zero high bits of the requestCode [1]. FragmentActivity.startActivityForResult enforces that all activities started from AppCompatActivity will have resource codes that satisfy this condition [2]. Chrome doesn't set custom PermissionCompatDelegate, so super.onActivityResult should be no-op for all non-fragment calls to startActivityForResult.
In general, every class in the inheritance chain for activity should use unique request IDs. If these IDs are unique, calling super.onActivityResult should be safe. If there's a clash in these IDs, not verifying ID like you've proposed isn't much better than calling super all the time:
class A extends AppCompatActivity {
public static final int REQUEST_CODE = 1;@Override
protected void onActivityResult(int requestCode, int resultCode, Intent data) {
if (requestCode == REQUEST_CODE) {
// Handle result
return;
}
super.onActivityResult(requestCode, resultCode, data);
}void doSomething() {
startActivityForResult(REQUEST_CODE);
}
}class B extends A {
public static final int YET_ANOTHER_REQUEST_CODE = 1;@Override
protected void onActivityResult(int requestCode, int resultCode, Intent data) {
if (requestCode == YET_ANOTHER_REQUEST_CODE) {
// Handle result
return;
}
super.onActivityResult(requestCode, resultCode, data);
}
}In this case flows that involve doSomething won't work, because results from startActivityForResult will never get to super.onActivityResult.
A couple of related thoughts:
We already have a bunch of activities that unconditionally call super.onActivityResult [3]. I think we should bite the bullet and enforce this for all activities.We will have to resolve this sooner or later, as many fragments in Chrome call Fragment.startActivityForResult and rely onActivityResult being delivered to them (for example [4]). IUUC, framework implementation of fragments doesn't require calling super method in onActivityResult, but we'll need to call it when we migrate to support library (we'll have to do it sooner or later because of the deprecation mentioned in https://crbug.com/825789).
To view, visit change 1028690. To unsubscribe, or for help writing mail filters, visit settings.
Ted Choc (Gerrit)
Patch Set 1:
(1 comment)
Thank you for the review! Warning: super-long comment ahead.
I wonder how long of a review comment thread we can have before we break the tool :-)
1 comment:
Patch Set #1, Line 649:
super.onActivityResult(requestCode, resultCode, data);
I agree that the approach you've proposed won't work "as is" in AsyncInitializationActivity. Unfortunately, I can't just fix this in other places and leave AsyncInitializationActivity as is, because I need functional onActivityResult in FRE fragments and FirstRunActivity subclasses AsyncInitializationActivity.
I think that unconditionally calling super in this specific case should be safe, as FragmentActivity.onActivityResult ignores all calls for request codes which have zero high bits of the requestCode [1]. FragmentActivity.startActivityForResult enforces that all activities started from AppCompatActivity will have resource codes that satisfy this condition [2]. Chrome doesn't set custom PermissionCompatDelegate, so super.onActivityResult should be no-op for all non-fragment calls to startActivityForResult.
Could you fix AsyncInitializationActivity by changing NativeInitializationController to look at the return value of onActivityResultWithNative?
To me, it looks like we could do:
AsyncInitializationActivity {
...
void super_onActivityResult(int requestCode, int resultCode, Intent intent) {
super.onActivityResult(requestCode, resultCode, intent);
}...
}NativeInitializationController {
...
if (!mActivityDelegate.onActivityResultWithNative(requestCode, resultCode, data)) {
mActivityDelegate.super_onActivityResult(requestCode, resultCode, intent);
}
...
}Alternatively, we could change this entirely within AsyncInitializationActivity
{
public final void onActivityResultWithNative(int requestCode, int resultCode, Intent intent) {
if (!handleActivityResultWithNative(...)) {
super.onActivityResult(...);
}
}protected boolean handleActivityResultWithNative(...) {
}
}
In general, every class in the inheritance chain for activity should use unique request IDs. If these IDs are unique, calling super.onActivityResult should be safe. If there's a clash in these IDs, not verifying ID like you've proposed isn't much better than calling super all the time:class A extends AppCompatActivity {
public static final int REQUEST_CODE = 1;@Override
protected void onActivityResult(int requestCode, int resultCode, Intent data) {
if (requestCode == REQUEST_CODE) {
// Handle result
return;
}
super.onActivityResult(requestCode, resultCode, data);
}void doSomething() {
startActivityForResult(REQUEST_CODE);
}
}class B extends A {
public static final int YET_ANOTHER_REQUEST_CODE = 1;@Override
protected void onActivityResult(int requestCode, int resultCode, Intent data) {
if (requestCode == YET_ANOTHER_REQUEST_CODE) {
// Handle result
return;
}
super.onActivityResult(requestCode, resultCode, data);
}
}In this case flows that involve doSomething won't work, because results from startActivityForResult will never get to super.onActivityResult.
To me, this looks like a bug (a bad one). Having two activities use the same request code is going to have them do potentially conflicting and unintended behaviors.
In the extreme, it could be:
class B extends A {
public static final int SHOW_TOAST_REQUEST_CODE = 1;{
startActivityForResult(..., SHOW_TOAST_REQUEST_CODE);
}
}class A extends ... {
public static final int DELETE_ALL_YOUR_DATA_REQUEST_CODE = 1;...
}By allowing calling up the chain, it seems very, very risky if you yourself have the request code defined.
A couple of related thoughts:
We already have a bunch of activities that unconditionally call super.onActivityResult [3]. I think we should bite the bullet and enforce this for all activities.
To me, these again look like bugs. None of those are actual critical activities and I wouldn't use them as good examples of why we should adopt it...I see that we should fix them to address it.
We will have to resolve this sooner or later, as many fragments in Chrome call Fragment.startActivityForResult and rely onActivityResult being delivered to them (for example [4]). IUUC, framework implementation of fragments doesn't require calling super method in onActivityResult, but we'll need to call it when we migrate to support library (we'll have to do it sooner or later because of the deprecation mentioned in https://crbug.com/825789).
For this, I don't really understand. If the Fragment itself has onActivityResult called on it with the correct value, why would you need to call up?
I thought the problem was that if you didn't call up then the fragment responses wouldn't be propagated to the individual fragments. If you're at the point where you are handling the result, why would you need to tell the parent?
To view, visit change 1028690. To unsubscribe, or for help writing mail filters, visit settings.
Boris Sazonov (Gerrit)
Ted, please take another look.
Patch set 2:Commit-Queue +1
1 comment:
Patch Set #1, Line 649:
if (requestCode == IMPORTANT_SITES_DIALOG_CODE && resultCode == Activity.RESULT_OK) {> I agree that the approach you've proposed won't work "as is" in AsyncInitializationActivity. […]
My point about IDs was: when you have ID clash, BadStuff™ is going to happen. Unfortunately, not calling super doesn't completely protect us from this BadStuff (see my example in the previous comment).
However, I agree that your proposal is probabilistically safer, as it reduces the probability of BadStuff actually happening.I've revised the CL and now super.onActivityResult is invoked iff mWindowAndroid.onActivityResult has returned false (or there's no window).
To view, visit change 1028690. To unsubscribe, or for help writing mail filters, visit settings.
Ted Choc (Gerrit)
VIOLENT AGREEMENT!!!! Thank you Boris for this discussion. I very much appreciate you making the codebase better. We have a lot left to do, but each little bit moves us closer.
Patch set 2:Code-Review +1
Boris Sazonov (Gerrit)
Thanks a lot! That's the code review verdict I've only dreamed of!
Patch set 2:Commit-Queue +2
Commit Bot (Gerrit)
Commit Bot merged this change.
[Android] Add super method calls to onActivityResult overrides
This CL adds super.onActivityResult calls to all methods that override
onActivityResult in Activity and Fragment subclasses. This should fix
an issue when Fragment.onActivityResult isn't called when expected.
super.onActivityResult calls in activities that subclass
AppCompatActivity end up in FragmentActivity.onActivityResult that
dispatches onActivityResult calls to Fragments that have started
activities using Fragment.startActivityForResult.
Bug: 836921
Change-Id: I19550c7106ad01bf6acccb0fe0d985561c7a1f81
Reviewed-on: https://chromium-review.googlesource.com/1028690
Reviewed-by: Ted Choc <ted...@chromium.org>
Commit-Queue: Boris Sazonov <bsaz...@chromium.org>
Cr-Commit-Position: refs/heads/master@{#554421}
---
M chrome/android/java/src/org/chromium/chrome/browser/init/AsyncInitializationActivity.java
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/chrome/android/java/src/org/chromium/chrome/browser/init/AsyncInitializationActivity.java b/chrome/android/java/src/org/chromium/chrome/browser/init/AsyncInitializationActivity.java
index 5f134bb..fc3f525 100644
--- a/chrome/android/java/src/org/chromium/chrome/browser/init/AsyncInitializationActivity.java
+++ b/chrome/android/java/src/org/chromium/chrome/browser/init/AsyncInitializationActivity.java
@@ -553,11 +553,12 @@
@CallSuper
@Override
public boolean onActivityResultWithNative(int requestCode, int resultCode, Intent intent) {
- if (mWindowAndroid != null) {
- return mWindowAndroid.onActivityResult(requestCode, resultCode, intent);
- } else {
- return false;
+ if (mWindowAndroid != null
+ && mWindowAndroid.onActivityResult(requestCode, resultCode, intent)) {
+ return true;
}
+ super.onActivityResult(requestCode, resultCode, intent);
+ return false;
}
@CallSuper
To view, visit change 1028690. To unsubscribe, or for help writing mail filters, visit settings.