Commit fec3c507 authored by Friedrich Horschig's avatar Friedrich Horschig Committed by Commit Bot

[Android, Crash] Ensure accessory provides empty actions

If the provider registered itself but never provided items, the caching
adapter would notify all observers that the last provided actions were
null and force them into an undefined state which caused a crash.

The proper way to handle this case, is to send an empty set to all
observers (see tests for an intuitive use case).

Bug: 855036
Change-Id: If3a0e9ee14ac3e24e163ebba092ab47f226e43b7
Reviewed-on: https://chromium-review.googlesource.com/1115220Reviewed-by: default avatarBernhard Bauer <bauerb@chromium.org>
Commit-Queue: Friedrich Horschig <fhorschig@chromium.org>
Cr-Commit-Position: refs/heads/master@{#570714}
parent 77318515
...@@ -43,12 +43,18 @@ class ManualFillingMediator ...@@ -43,12 +43,18 @@ class ManualFillingMediator
* If the observed provider serves a currently visible tab, the data is immediately sent on. * If the observed provider serves a currently visible tab, the data is immediately sent on.
* @param tab The {@link Tab} which the given Provider should affect immediately. * @param tab The {@link Tab} which the given Provider should affect immediately.
* @param provider The {@link Provider} to observe and whose data to cache. * @param provider The {@link Provider} to observe and whose data to cache.
* @param defaultItems The items to be notified about if the Provider hasn't provided any.
*/ */
ProviderCacheAdapter(Tab tab, Provider<T> provider) { ProviderCacheAdapter(Tab tab, Provider<T> provider, T[] defaultItems) {
mTab = tab; mTab = tab;
provider.addObserver(this); provider.addObserver(this);
mLastItems = defaultItems;
} }
/**
* Calls {@link #onItemsAvailable} with the last used items again. If there haven't been
* any calls, call it with an empty list to avoid putting observers in an undefined state.
*/
void notifyAboutCachedItems() { void notifyAboutCachedItems() {
notifyObservers(mLastItems); notifyObservers(mLastItems);
} }
...@@ -131,8 +137,8 @@ class ManualFillingMediator ...@@ -131,8 +137,8 @@ class ManualFillingMediator
} }
void registerActionProvider(Provider<KeyboardAccessoryData.Action> actionProvider) { void registerActionProvider(Provider<KeyboardAccessoryData.Action> actionProvider) {
ProviderCacheAdapter<KeyboardAccessoryData.Action> adapter = ProviderCacheAdapter<KeyboardAccessoryData.Action> adapter = new ProviderCacheAdapter<>(
new ProviderCacheAdapter<>(mActiveBrowserTab, actionProvider); mActiveBrowserTab, actionProvider, new KeyboardAccessoryData.Action[0]);
mModel.get(mActiveBrowserTab).mActionsProvider = adapter; mModel.get(mActiveBrowserTab).mActionsProvider = adapter;
getKeyboardAccessory().registerActionListProvider(adapter); getKeyboardAccessory().registerActionListProvider(adapter);
} }
......
...@@ -258,6 +258,29 @@ public class ManualFillingControllerTest { ...@@ -258,6 +258,29 @@ public class ManualFillingControllerTest {
addTab(mediator, 1111, tab); addTab(mediator, 1111, tab);
} }
@Test
public void testTreatNeverProvidedActionsAsEmptyActionList() {
ManualFillingMediator mediator = mController.getMediatorForTesting();
KeyboardAccessoryModel keyboardAccessoryModel =
mediator.getKeyboardAccessory().getMediatorForTesting().getModelForTesting();
// Open a tab.
Tab tab = addTab(mediator, 1111, null);
// Add an action provider that never provided actions.
mController.registerActionProvider(new PropertyProvider<>());
assertThat(keyboardAccessoryModel.getActionList().size(), is(0));
// Create a new tab with an action:
Tab secondTab = addTab(mediator, 1111, tab);
PropertyProvider<Action> provider = new PropertyProvider<>();
mController.registerActionProvider(provider);
provider.notifyObservers(new Action[] {new Action("Test Action", (action) -> {})});
assertThat(keyboardAccessoryModel.getActionList().size(), is(1));
switchTab(mediator, secondTab, tab);
assertThat(keyboardAccessoryModel.getActionList().size(), is(0));
}
// TODO(fhorschig): Test that updating tab1 works if tab2 is active. // 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 destroying a tab cleans the model.
// TODO(fhorschig): Test that unregistering a provider affects only one tab. // 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