Bug 1498854 - Rework dismissing of TabHistoryFragment. r=jchen
1. The patch from
bug 1476710 was nonsense and had the same effect as simply
deleting that line, because the ChildFragmentManager is only of interest if
the TabHistoryFragment loaded further Fragments itself.
2. The issue at hand is that under some circumstances the TabHistoryFragment
will be trying to dismiss itself while its responsible FragmentManager is
already busy transacting some Fragment state changes. More precisely, the
fact that the Fragment is calling popBackStack*Immediately*, which isn't
allowed if the FragmentManager is already handling some other transaction.
3. The dismiss() calls in response to the onClick() handlers are unproblematic,
because they won't trigger any FragmentManager activity through any route
other than the dismiss() call itself.
4. The dismiss() calls in onPause() *are* problematic because the Fragment-
Manager will already be busy pausing the TabHistoryFragment, so triggering a
further synchronous transaction is not allowed (and originally caused
bug 1476710).
5. If the onPause() call happened because some external entity was attempting to
remove the fragment (either BrowserApp directly, or indirectly by forwarding
a back button press to the FragmentManager), then the Fragment trying to
additionally remove itself is unnecessary.
6. If the onPause() call happens because the containing activity itself is being
paused, then the Fragment being dismissed is the desired outcome (see
bug 1093209), however the Fragment won't be able to remove itself because
a) A synchronous transaction is illegal at that point in time.
b) An async transaction would be possible, but might not complete until after
onSaveInstanceState() had subsequently already been called, which again
would be illegal because of state loss.
c) An async transaction allowing state loss would succeed in any case, but
would mean that if BrowserApp was subsequently destroyed while in back-
ground and then later recreated from the savedInstanceState, the Tab-
HistoryFragment would be recreated as well, which is undesired.
7. Therefore, the only way to dismiss the TabHistoryFragment when the containing
activity is pausing is to synchronously dismiss the Fragment from inside the
activity, *before* the onPause() call is forwarded to the FragmentManager.
8. Calling dismiss() in response to onDestroy() is unnecessary, because the
Fragment is already irrevocably being removed as soon as we hit onPause().
9. Because it doesn't make sense to have multiple TabHistoryFragments active at
the same time, we also change the logic such that any attempt to show a new
TabHistoryFragment will now replace the previous fragment.
This is also useful in view of the fact that in order to close the Fragment,
BrowserApp retrieves it by calling findFragmentByTag(), which simply returns
the first matching Fragment, i.e. wouldn't properly handle things if we ever
accidentally ended up with multiple Fragment instances active at the same
time.
Differential Revision:
https://phabricator.services.mozilla.com/D8680
--- a/mobile/android/base/java/org/mozilla/gecko/BrowserApp.java
+++ b/mobile/android/base/java/org/mozilla/gecko/BrowserApp.java
@@ -1033,16 +1033,18 @@ public class BrowserApp extends GeckoApp
for (BrowserAppDelegate delegate : delegates) {
delegate.onResume(this);
}
}
@Override
public void onPause() {
+ dismissTabHistoryFragment();
+
super.onPause();
if (mIsAbortingAppLaunch) {
return;
}
if (mHasResumed) {
// Register for Prompt:ShowTop so we can foreground this activity even if it's hidden.
getAppEventDispatcher().registerUiThreadListener(this, "Prompt:ShowTop");
@@ -3113,20 +3115,17 @@ public class BrowserApp extends GeckoApp
}
@Override
public boolean onPrepareOptionsMenu(Menu aMenu) {
if (aMenu == null)
return false;
// Hide the tab history panel when hardware menu button is pressed.
- TabHistoryFragment frag = (TabHistoryFragment) getSupportFragmentManager().findFragmentByTag(TAB_HISTORY_FRAGMENT_TAG);
- if (frag != null) {
- frag.dismiss();
- }
+ dismissTabHistoryFragment();
if (!GeckoThread.isRunning()) {
aMenu.findItem(R.id.settings).setEnabled(false);
aMenu.findItem(R.id.help).setEnabled(false);
}
Tab tab = Tabs.getInstance().getSelectedTab();
// Unlike other menu items, the bookmark star is not tinted. See {@link ThemedImageButton#setTintedDrawable}.
@@ -4135,9 +4134,16 @@ public class BrowserApp extends GeckoApp
}
@Override
public void onFinishedOnboarding(final boolean showBrowserHint) {
if (showBrowserHint && !Tabs.hasHomepage(this)) {
enterEditingMode();
}
}
+
+ private void dismissTabHistoryFragment() {
+ TabHistoryFragment frag = (TabHistoryFragment) getSupportFragmentManager().findFragmentByTag(TAB_HISTORY_FRAGMENT_TAG);
+ if (frag != null) {
+ frag.dismiss();
+ }
+ }
}
--- a/mobile/android/base/java/org/mozilla/gecko/tabs/TabHistoryFragment.java
+++ b/mobile/android/base/java/org/mozilla/gecko/tabs/TabHistoryFragment.java
@@ -100,59 +100,61 @@ public class TabHistoryFragment extends
// Since the fragment view fills the entire screen, any clicks outside of the history
// ListView will end up here.
dismiss();
}
@Override
public void onPause() {
super.onPause();
- dismiss();
+ onDismiss();
}
@Override
public void onDestroy() {
super.onDestroy();
- dismiss();
GeckoApplication.watchReference(getActivity(), this);
}
@Override
public void onSaveInstanceState(Bundle outState) {
if (backStackId >= 0) {
outState.putInt(BACK_STACK_ID, backStackId);
}
}
// Function to add this fragment to activity state with containerViewId as parent.
// This similar in functionality to DialogFragment.show() except that containerId is provided here.
public void show(final int containerViewId, final FragmentTransaction transaction, final String tag) {
dismissed = false;
- transaction.add(containerViewId, this, tag);
+ transaction.replace(containerViewId, this, tag);
transaction.addToBackStack(tag);
// Populating the tab history requires a gecko call (which can be slow) - therefore the app
// state by the time we try to show this fragment is unknown, and we could be in the
// middle of shutting down:
backStackId = transaction.commitAllowingStateLoss();
}
// Pop the fragment from backstack if it exists.
public void dismiss() {
+ if (backStackId >= 0) {
+ getFragmentManager().popBackStackImmediate(backStackId, FragmentManager.POP_BACK_STACK_INCLUSIVE);
+ backStackId = -1;
+ }
+ onDismiss();
+ }
+
+ private void onDismiss() {
if (dismissed) {
return;
}
dismissed = true;
- if (backStackId >= 0) {
- getChildFragmentManager().popBackStackImmediate(backStackId, FragmentManager.POP_BACK_STACK_INCLUSIVE);
- backStackId = -1;
- }
-
if (parent != null) {
parent.setVisibility(View.GONE);
}
}
private static class TabHistoryAdapter extends ArrayAdapter<TabHistoryPage> {
private final List<TabHistoryPage> pages;
private final Context context;