Skip to content

Commit

Permalink
Fix fullscreen browser overrides when no tab is present.
Browse files Browse the repository at this point in the history
If a client requests the top controls be made visible via
the BrowserStateBrowserControlsVisibilityDelegate but no
tabs are available, the fullscreen manager should force
the controls to be visible anyway.

This fixes an issue where you only have a single incognito
tab, scroll off the top controls, then hit the close all
incognito tabs notification.  At this point, you are jumped
back to the tab switcher but the controls were not being
shown without this additional logic.

BUG=675518

Review-Url: https://codereview.chromium.org/2647943002
Cr-Original-Commit-Position: refs/heads/master@{#445157}
Review-Url: https://codereview.chromium.org/2642263005 .
Cr-Commit-Position: refs/branch-heads/2987@{#8}
Cr-Branched-From: ad51088-refs/heads/master@{#444943}
  • Loading branch information
Ted Choc committed Jan 20, 2017
1 parent 4815b3f commit 4431066
Show file tree
Hide file tree
Showing 3 changed files with 36 additions and 27 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@
import android.os.SystemClock;

import org.chromium.chrome.browser.tab.BrowserControlsVisibilityDelegate;
import org.chromium.chrome.browser.tab.Tab;

import java.lang.ref.WeakReference;
import java.util.HashSet;
Expand All @@ -30,10 +29,10 @@ public class BrowserStateBrowserControlsVisibilityDelegate

private final Set<Integer> mPersistentControlTokens = new HashSet<Integer>();
private final Handler mHandler;
private final Runnable mStateChangedCallback;

private long mCurrentShowTime;
private int mPersistentControlsCurrentToken;
private Tab mTab;

// This static inner class holds a WeakReference to the outer object, to avoid triggering the
// lint HandlerLeak warning.
Expand All @@ -60,16 +59,13 @@ public void handleMessage(Message msg) {
/**
* Constructs a BrowserControlsVisibilityDelegate designed to deal with overrides driven by
* the browser UI (as opposed to the state of the tab).
*
* @param stateChangedCallback The callback to be triggered when the fullscreen state should be
* updated based on the state of the browser visibility override.
*/
public BrowserStateBrowserControlsVisibilityDelegate() {
public BrowserStateBrowserControlsVisibilityDelegate(Runnable stateChangedCallback) {
mHandler = new VisibilityDelegateHandler(this);
}

/**
* Sets the currently visible tab for fullscreen control.
*/
protected void setTab(Tab tab) {
mTab = tab;
mStateChangedCallback = stateChangedCallback;
}

private void ensureControlsVisibleForMinDuration() {
Expand All @@ -86,16 +82,14 @@ private void ensureControlsVisibleForMinDuration() {
private int generateToken() {
int token = mPersistentControlsCurrentToken++;
mPersistentControlTokens.add(token);
if (mPersistentControlTokens.size() == 1 && mTab != null) {
mTab.updateFullscreenEnabledState();
}
if (mPersistentControlTokens.size() == 1) mStateChangedCallback.run();
return token;
}

private void releaseToken(int token) {
if (mPersistentControlTokens.remove(token)
&& mPersistentControlTokens.isEmpty() && mTab != null) {
mTab.updateFullscreenEnabledState();
&& mPersistentControlTokens.isEmpty()) {
mStateChangedCallback.run();
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,17 @@ public ChromeFullscreenManager(Activity activity, boolean isBottomControls) {
mActivity = activity;
mWindow = activity.getWindow();
mIsBottomControls = isBottomControls;
mBrowserVisibilityDelegate = new BrowserStateBrowserControlsVisibilityDelegate();
mBrowserVisibilityDelegate = new BrowserStateBrowserControlsVisibilityDelegate(
new Runnable() {
@Override
public void run() {
if (getTab() != null) {
getTab().updateFullscreenEnabledState();
} else if (!mBrowserVisibilityDelegate.isHidingBrowserControlsEnabled()) {
setPositionsForTabToNonFullscreen();
}
}
});
}

/**
Expand Down Expand Up @@ -160,6 +170,11 @@ public void tabRemoved(Tab tab) {
public void didSelectTab(Tab tab, TabSelectionType type, int lastId) {
setTab(mTabModelSelector.getCurrentTab());
}

@Override
public void didCloseTab(int tabId, boolean incognito) {
setTab(mTabModelSelector.getCurrentTab());
}
};

assert controlContainer != null;
Expand Down Expand Up @@ -198,10 +213,12 @@ public BrowserStateBrowserControlsVisibilityDelegate getBrowserVisibilityDelegat
public void setTab(Tab tab) {
Tab previousTab = getTab();
super.setTab(tab);
mBrowserVisibilityDelegate.setTab(getTab());
if (tab != null && previousTab != getTab()) {
mBrowserVisibilityDelegate.showControlsTransient();
}
if (tab == null && !mBrowserVisibilityDelegate.isHidingBrowserControlsEnabled()) {
setPositionsForTabToNonFullscreen();
}
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@
import org.robolectric.shadows.ShadowSystemClock;

import org.chromium.base.test.util.Feature;
import org.chromium.chrome.browser.tab.Tab;
import org.chromium.testing.local.LocalRobolectricTestRunner;

/**
Expand All @@ -30,16 +29,15 @@
@RunWith(LocalRobolectricTestRunner.class)
@Config(manifest = Config.NONE)
public class BrowserStateBrowserControlsVisibilityDelegateTest {
@Mock private Tab mTab;
@Mock private Runnable mCallback;

private BrowserStateBrowserControlsVisibilityDelegate mDelegate;

@Before
public void beforeTest() {
MockitoAnnotations.initMocks(this);

mDelegate = new BrowserStateBrowserControlsVisibilityDelegate();
mDelegate.setTab(mTab);
mDelegate = new BrowserStateBrowserControlsVisibilityDelegate(mCallback);
}

private void advanceTime(long amount) {
Expand All @@ -56,7 +54,7 @@ public void testTransientShow() {
ShadowLooper.runUiThreadTasksIncludingDelayedTasks();
assertTrue(mDelegate.isHidingBrowserControlsEnabled());

verify(mTab, times(2)).updateFullscreenEnabledState();
verify(mCallback, times(2)).run();
}

@Test
Expand All @@ -71,7 +69,7 @@ public void testShowPersistentTokenWithDelayedHide() {
mDelegate.hideControlsPersistent(token);
assertTrue(mDelegate.isHidingBrowserControlsEnabled());

verify(mTab, times(2)).updateFullscreenEnabledState();
verify(mCallback, times(2)).run();
}

@Test
Expand All @@ -89,7 +87,7 @@ public void testShowPersistentTokenWithImmediateHide() {
ShadowLooper.runUiThreadTasksIncludingDelayedTasks();
assertTrue(mDelegate.isHidingBrowserControlsEnabled());

verify(mTab, times(2)).updateFullscreenEnabledState();
verify(mCallback, times(2)).run();
}

@Test
Expand All @@ -108,7 +106,7 @@ public void testShowPersistentBeyondRequiredMinDurationAndShowTransient() {
mDelegate.hideControlsPersistent(token);
assertTrue(mDelegate.isHidingBrowserControlsEnabled());

verify(mTab, times(2)).updateFullscreenEnabledState();
verify(mCallback, times(2)).run();
}

@Test
Expand All @@ -131,7 +129,7 @@ public void testShowPersistentBelowRequiredMinDurationAndShowTransient() {
ShadowLooper.runUiThreadTasksIncludingDelayedTasks();
assertTrue(mDelegate.isHidingBrowserControlsEnabled());

verify(mTab, times(2)).updateFullscreenEnabledState();
verify(mCallback, times(2)).run();
}

@Test
Expand All @@ -158,6 +156,6 @@ public void testShowPersistentMultipleTimes() {
mDelegate.hideControlsPersistent(thirdToken);
assertTrue(mDelegate.isHidingBrowserControlsEnabled());

verify(mTab, times(2)).updateFullscreenEnabledState();
verify(mCallback, times(2)).run();
}
}

0 comments on commit 4431066

Please sign in to comment.