Commit 20458b5f authored by Friedrich Horschig's avatar Friedrich Horschig Committed by Commit Bot

[Android, Crash fix] Set accessory tabs independent from state

This CL fixes the most common keyboard accessory crashes for 69.
Before it, the filling coordinator removed the exact instance of a held
password sheet from the accessory.
It would crash when the password tab wasn't part of the accessory
anymore - maybe due to race conditions.

With this implementation, the filling coordinator just resets all tabs
of the accessory. As it's the owner of the accessory, this just forces
it into a sane state (which is exactly what the reset is meant to do).

Next up: Removing remove methods.

Bug: 855036, 854348
Change-Id: I73563856745bfe106f78f58c15b395ce614c107a
Reviewed-on: https://chromium-review.googlesource.com/1113308
Commit-Queue: Friedrich Horschig <fhorschig@chromium.org>
Reviewed-by: default avatarBernhard Bauer <bauerb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#570015}
parent 040f2ad0
......@@ -76,10 +76,14 @@ public class AccessorySheetCoordinator {
mMediator.addTab(tab);
}
public void removeTab(KeyboardAccessoryData.Tab tab) {
void removeTab(KeyboardAccessoryData.Tab tab) {
mMediator.removeTab(tab);
}
void setTabs(KeyboardAccessoryData.Tab[] tabs) {
mMediator.setTabs(tabs);
}
/**
* Returns a {@link KeyboardAccessoryData.Tab} object that is used to display this bottom sheet.
* @return Returns a {@link KeyboardAccessoryData.Tab}.
......
......@@ -58,6 +58,11 @@ class AccessorySheetMediator {
if (mModel.getActiveTabIndex() == NO_ACTIVE_TAB) hide();
}
void setTabs(KeyboardAccessoryData.Tab[] tabs) {
mModel.getTabList().set(tabs);
mModel.setActiveTabIndex(mModel.getTabList().size() - 1);
}
void setActiveTab(int position) {
assert position < mModel.getTabList().size()
|| position >= 0 : position + " is not a valid tab index!";
......
......@@ -135,6 +135,10 @@ public class KeyboardAccessoryCoordinator {
mMediator.removeTab(tab);
}
void setTabs(KeyboardAccessoryData.Tab[] tabs) {
mMediator.setTabs(tabs);
}
/**
* Allows any {@link KeyboardAccessoryData.Provider} to communicate with the
* {@link KeyboardAccessoryMediator} of this component.
......
......@@ -73,6 +73,10 @@ class KeyboardAccessoryMediator
mModel.removeTab(tab);
}
void setTabs(KeyboardAccessoryData.Tab[] tabs) {
mModel.getTabList().set(tabs);
}
void setSuggestions(AutofillKeyboardSuggestions suggestions) {
mModel.setAutofillSuggestions(suggestions);
}
......
......@@ -190,13 +190,13 @@ class ManualFillingMediator
private void resetAccessory(Tab browserTab) {
AccessoryState state = getOrCreateAccessoryState(browserTab);
if (state.mPasswordAccessorySheet != null) {
removeTab(state.mPasswordAccessorySheet.getTab());
setTabs(new KeyboardAccessoryData.Tab[0]);
}
}
private void removeTab(KeyboardAccessoryData.Tab tab) {
mKeyboardAccessory.removeTab(tab);
mAccessorySheet.removeTab(tab);
private void setTabs(KeyboardAccessoryData.Tab[] tabs) {
mKeyboardAccessory.setTabs(tabs);
mAccessorySheet.setTabs(tabs);
}
@VisibleForTesting
......
......@@ -245,6 +245,19 @@ public class ManualFillingControllerTest {
assertThat(accessorySheetModel.getTabList().size(), is(0));
}
@Test
public void testRecoversFromInvalidState() {
ManualFillingMediator mediator = mController.getMediatorForTesting();
// Open a tab but pretend that the states became inconsistent.
Tab tab = addTab(mediator, 1111, null);
mediator.getModelForTesting().get(tab).mPasswordAccessorySheet =
new PasswordAccessorySheetCoordinator(mMockActivity);
// Create a new tab with a passwords tab:
addTab(mediator, 1111, tab);
}
// TODO(fhorschig): Test that updating tab1 works if tab2 is active.
// TODO(fhorschig): Test that destroying a tab cleans the model.
// TODO(fhorschig): Test that unregistering a provider affects only one tab.
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment