Skip to content

Commit

Permalink
Empty check bookmark refresh.
Browse files Browse the repository at this point in the history
There was a gap between using the back nav button to exit the bookmark manager, and when the mediator was actually destroyed. When the back nav was used, the internal state stack was empty.

During this time before destroy was called, the mediator was still observing the BookmarkModel, and a synced change could cause the mediator to try to refresh the UI. However this refresh didn't verify
the state of the back stack, and would blow up trying to pop off an empty stack.

This change simply makes sure the stack isn't empty when refreshing, otherwise the refresh is a no-op. In the case that we're in this inconsistent mid-destroy state, a no-op is fine for the refresh call.

(cherry picked from commit 3eeade1)

Bug: 1450539
Change-Id: I6ebcf1604992b6c1dbe8c274f7753c279c1efe7c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4598006
Reviewed-by: Matthew Jones <mdjones@chromium.org>
Commit-Queue: Sky Malice <skym@chromium.org>
Cr-Original-Commit-Position: refs/heads/main@{#1155024}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4608669
Auto-Submit: Sky Malice <skym@chromium.org>
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Commit-Queue: Krishna Govind <govind@chromium.org>
Reviewed-by: Krishna Govind <govind@chromium.org>
Owners-Override: Krishna Govind <govind@chromium.org>
Cr-Commit-Position: refs/branch-heads/5790@{#676}
Cr-Branched-From: 1d71a33-refs/heads/main@{#1148114}
  • Loading branch information
Sky Malice authored and Chromium LUCI CQ committed Jun 13, 2023
1 parent 007072d commit d98d8cd
Show file tree
Hide file tree
Showing 2 changed files with 13 additions and 1 deletion.
Expand Up @@ -891,7 +891,9 @@ private void updateAllLocations() {

/** Refresh the list of bookmarks within the currently visible folder. */
private void refresh() {
notifyUi(mStateStack.peek());
if (!mStateStack.isEmpty()) {
notifyUi(mStateStack.peek());
}
}

/**
Expand Down
Expand Up @@ -463,6 +463,16 @@ public void onBackPressed_MultipleStateStack() {
assertTrue(mMediator.onBackPressed());
}

@Test
public void onBackPressed_AndThenModelEvent() {
initAndLoadBookmarkModel();
assertFalse(mMediator.onBackPressed());

verify(mBookmarkModel).addObserver(mBookmarkModelObserverArgumentCaptor.capture());
mBookmarkModelObserverArgumentCaptor.getValue().bookmarkModelChanged();
// This test is verifying the observer event doesn't crash.
}

@Test
public void testMoveDownUp() {
finishLoading();
Expand Down

0 comments on commit d98d8cd

Please sign in to comment.