Skip to content

Commit

Permalink
Remove TabModelSelector's dependency on ChromeFullscreenManager.
Browse files Browse the repository at this point in the history
ChromeFullscreenManager will now register and listen
for updates internally and avoids the prior cyclical
dependency.

This also fixes a bug introduced in:
https://codereview.chromium.org/2414913004

In that, we were not clearing the reference to Tab when
reparenting and then there were two fullscreen managers
listening for updates.

BUG=656939

Review-Url: https://chromiumcodereview.appspot.com/2440643002
Cr-Commit-Position: refs/heads/master@{#426935}
  • Loading branch information
tedchoc authored and Commit bot committed Oct 22, 2016
1 parent a02af5e commit 820a95b
Show file tree
Hide file tree
Showing 18 changed files with 382 additions and 197 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -322,6 +322,7 @@ public void onTouchExplorationStateChanged(boolean enabled) {
mWindowAndroid.setKeyboardAccessoryView((ViewGroup) findViewById(R.id.keyboard_accessory));
initializeToolbar();
initializeTabModels();
if (!isFinishing()) mFullscreenManager = createFullscreenManager();
}

@Override
Expand Down Expand Up @@ -446,11 +447,6 @@ public void onMenuVisibilityChanged(boolean isVisible) {
protected final void initializeTabModels() {
if (mTabModelsInitialized) return;

// TODO(tedchoc): FullscreenManager is required to create the TabModelSelector, but that
// does not seem like the right ordering. This should happen after
// initializing the tab models.
mFullscreenManager = createFullscreenManager();

mTabModelSelector = createTabModelSelector();
if (mTabModelSelector == null) {
assert isFinishing();
Expand Down Expand Up @@ -1267,11 +1263,10 @@ public void run() {
* @return The {@link TabModelSelector}, possibly null.
*/
public TabModelSelector getTabModelSelector() {
// TODO(tedchoc): Enable once CCT early tab creation is fixed.
//if (!mTabModelsInitialized) {
// throw new IllegalStateException(
// "Attempting to access TabModelSelector before initialization");
//}
if (!mTabModelsInitialized) {
throw new IllegalStateException(
"Attempting to access TabModelSelector before initialization");
}
return mTabModelSelector;
}

Expand All @@ -1286,11 +1281,10 @@ public InsetObserverView getInsetObserverView() {

@Override
public TabCreatorManager.TabCreator getTabCreator(boolean incognito) {
// TODO(tedchoc): Enable once CCT early tab creation is fixed.
//if (!mTabModelsInitialized) {
// throw new IllegalStateException(
// "Attempting to access TabCreator before initialization");
//}
if (!mTabModelsInitialized) {
throw new IllegalStateException(
"Attempting to access TabCreator before initialization");
}
return incognito ? mIncognitoTabCreator : mRegularTabCreator;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -991,8 +991,7 @@ protected TabModelSelector createTabModelSelector() {
int index = savedInstanceState != null ? savedInstanceState.getInt(WINDOW_INDEX, 0) : 0;

mTabModelSelectorImpl = (TabModelSelectorImpl)
TabWindowManager.getInstance().requestSelector(this, this, getFullscreenManager(),
index);
TabWindowManager.getInstance().requestSelector(this, this, index);
if (mTabModelSelectorImpl == null) {
Toast.makeText(this, getString(R.string.unsupported_number_of_windows),
Toast.LENGTH_LONG).show();
Expand Down Expand Up @@ -1658,6 +1657,6 @@ public VrShellDelegate getVrShellDelegate() {
protected ChromeFullscreenManager createFullscreenManager() {
return new ChromeFullscreenManager(this,
(ToolbarControlContainer) findViewById(R.id.control_container),
getControlContainerHeightResource(), true);
getTabModelSelector(), getControlContainerHeightResource(), true);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -265,12 +265,7 @@ public void preInflationStartup() {
CustomTabsConnection.getInstance(getApplication()).getPrerenderedUrl(mSession));
if (getSavedInstanceState() == null
&& CustomTabsConnection.hasWarmUpBeenFinished(getApplication())) {
// TODO(tedchoc): Tab has dependencies on the tab model, so this should be called
// before creating the tab. That is not currently possible due to
// an ordering problem with creating the Fullscreen manager. If this
// CCT were to be displaying a native page, it would likely crash, but
// that doesn't happen in practice.
// initializeTabModels();
initializeTabModels();
mMainTab = createMainTab();
loadUrlInTab(mMainTab, new LoadUrlParams(getUrlToLoad()),
IntentHandler.getTimestampFromIntent(getIntent()));
Expand Down Expand Up @@ -318,8 +313,7 @@ protected TabModelSelector createTabModelSelector() {
TabPersistencePolicy persistencePolicy = new CustomTabTabPersistencePolicy(
getTaskId(), getSavedInstanceState() != null);

return new TabModelSelectorImpl(
this, this, getFullscreenManager(), persistencePolicy, false);
return new TabModelSelectorImpl(this, this, persistencePolicy, false);
}

@Override
Expand Down Expand Up @@ -900,6 +894,6 @@ private String getUrlToLoad() {
protected ChromeFullscreenManager createFullscreenManager() {
return new ChromeFullscreenManager(this,
(ToolbarControlContainer) findViewById(R.id.control_container),
getControlContainerHeightResource(), true);
getTabModelSelector(), getControlContainerHeightResource(), true);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,9 @@
import org.chromium.base.library_loader.LibraryLoader;
import org.chromium.chrome.browser.fullscreen.FullscreenHtmlApiHandler.FullscreenHtmlApiDelegate;
import org.chromium.chrome.browser.tab.Tab;
import org.chromium.chrome.browser.tabmodel.TabModel.TabSelectionType;
import org.chromium.chrome.browser.tabmodel.TabModelSelector;
import org.chromium.chrome.browser.tabmodel.TabModelSelectorTabModelObserver;
import org.chromium.chrome.browser.widget.ControlContainer;
import org.chromium.content.browser.ContentVideoView;
import org.chromium.content.browser.ContentViewCore;
Expand Down Expand Up @@ -61,6 +64,9 @@ public class ChromeFullscreenManager
private final Handler mHandler;
private final int mControlContainerHeight;

private final TabModelSelector mTabModelSelector;
private final TabModelSelectorTabModelObserver mTabModelObserver;

private final ControlContainer mControlContainer;

private long mMinShowNotificationMs = MINIMUM_SHOW_DURATION_MS;
Expand Down Expand Up @@ -181,19 +187,44 @@ public void handleMessage(Message msg) {
* Creates an instance of the fullscreen mode manager.
* @param activity The activity that supports fullscreen.
* @param controlContainer Container holding the controls (Toolbar).
* @param modelSelector The tab model selector that will be monitored for tab changes.
* @param resControlContainerHeight The dimension resource ID for the control container height.
* @param supportsBrowserOverride Whether we want to disable the token system used by the
browser.
*/
public ChromeFullscreenManager(Activity activity, ControlContainer controlContainer,
int resControlContainerHeight, boolean supportsBrowserOverride) {
TabModelSelector modelSelector, int resControlContainerHeight,
boolean supportsBrowserOverride) {
super(activity.getWindow());

mActivity = activity;
ApplicationStatus.registerStateListenerForActivity(this, activity);
((BaseChromiumApplication) activity.getApplication())
.registerWindowFocusChangedListener(this);

mTabModelSelector = modelSelector;
mTabModelObserver = new TabModelSelectorTabModelObserver(mTabModelSelector) {
@Override
public void tabClosureCommitted(Tab tab) {
setTab(mTabModelSelector.getCurrentTab());
}

@Override
public void allTabsClosureCommitted() {
setTab(mTabModelSelector.getCurrentTab());
}

@Override
public void tabRemoved(Tab tab) {
setTab(mTabModelSelector.getCurrentTab());
}

@Override
public void didSelectTab(Tab tab, TabSelectionType type, int lastId) {
setTab(mTabModelSelector.getCurrentTab());
}
};

mWindow = activity.getWindow();
mHandler = new FullscreenHandler(this);
assert controlContainer != null;
Expand All @@ -220,6 +251,8 @@ public void onActivityStateChange(Activity activity, int newState) {
ApplicationStatus.unregisterActivityStateListener(this);
((BaseChromiumApplication) mWindow.getContext().getApplicationContext())
.unregisterWindowFocusChangedListener(this);

mTabModelObserver.destroy();
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@
import org.chromium.base.VisibleForTesting;
import org.chromium.chrome.browser.compositor.layouts.OverviewModeBehavior;
import org.chromium.chrome.browser.compositor.layouts.content.TabContentManager;
import org.chromium.chrome.browser.fullscreen.FullscreenManager;
import org.chromium.chrome.browser.ntp.NativePageFactory;
import org.chromium.chrome.browser.tab.Tab;
import org.chromium.chrome.browser.tabmodel.TabModel.TabLaunchType;
Expand All @@ -30,8 +29,6 @@ public class TabModelSelectorImpl extends TabModelSelectorBase implements TabMod

private final TabCreatorManager mTabCreatorManager;

private FullscreenManager mFullscreenManager;

/** Flag set to false when the asynchronous loading of tabs is finished. */
private final AtomicBoolean mSessionRestoreInProgress =
new AtomicBoolean(true);
Expand Down Expand Up @@ -60,16 +57,13 @@ public class TabModelSelectorImpl extends TabModelSelectorBase implements TabMod
*
* @param activity An {@link Activity} instance.
* @param tabCreatorManager A {@link TabCreatorManager} instance.
* @param fullscreenManager A {@link FullscreenManager} instance.
* @param persistencePolicy A {@link TabPersistencePolicy} instance.
* @param supportUndo Whether a tab closure can be undone.
*/
public TabModelSelectorImpl(Activity activity, TabCreatorManager tabCreatorManager,
FullscreenManager fullscreenManager, TabPersistencePolicy persistencePolicy,
boolean supportUndo) {
TabPersistencePolicy persistencePolicy, boolean supportUndo) {
super();
mTabCreatorManager = tabCreatorManager;
mFullscreenManager = fullscreenManager;
mUma = new TabModelSelectorUma(activity);
final TabPersistentStoreObserver persistentStoreObserver =
new TabPersistentStoreObserver() {
Expand Down Expand Up @@ -351,7 +345,6 @@ public void requestToShowTab(Tab tab, TabSelectionType type) {
cacheTabBitmap(mVisibleTab);
}
mVisibleTab.hide();
if (mFullscreenManager != null) mFullscreenManager.setTab(null);
mTabSaver.addTabToSaveQueue(mVisibleTab);
}
mVisibleTab = null;
Expand All @@ -369,7 +362,6 @@ public void requestToShowTab(Tab tab, TabSelectionType type) {
tab.loadIfNeeded();
return;
}
if (mFullscreenManager != null) mFullscreenManager.setTab(tab);
mVisibleTab = tab;

// Don't execute the tab display part if Chrome has just been sent to background. This
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,87 @@
// Copyright 2016 The Chromium Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

package org.chromium.chrome.browser.tabmodel;

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

import java.util.List;

/**
* Observer for all {@link TabModel}s owned by a {@link TabModelSelector}.
*
* <p>
* This can safely be constructed before native libraries have been initialized as this will
* register to observe the underlying TabModels as they are created lazily.
*/
public class TabModelSelectorTabModelObserver extends EmptyTabModelObserver {
private final TabModelSelector mTabModelSelector;

private TabModelSelectorObserver mSelectorObserver;

/**
* Constructs an observer that should be notified of changes for all tab models owned
* by a specified {@link TabModelSelector}.
*
* <p>
* {@link #destroy()} must be called to unregister this observer.
*
* @param selector The selector that owns the Tab Models that should notify this observer.
*/
public TabModelSelectorTabModelObserver(TabModelSelector selector) {
mTabModelSelector = selector;

List<TabModel> tabModels = selector.getModels();
if (tabModels.isEmpty()) {
mSelectorObserver = new EmptyTabModelSelectorObserver() {
@Override
public void onNewTabCreated(Tab tab) {
assert false : "onChange should have happened and unregistered this listener.";
}

@Override
public void onChange() {
mTabModelSelector.removeObserver(this);
mSelectorObserver = null;
registerModelObservers();
}
};
mTabModelSelector.addObserver(mSelectorObserver);
} else {
registerModelObservers();
}
}

private void registerModelObservers() {
List<TabModel> tabModels = mTabModelSelector.getModels();
for (int i = 0; i < tabModels.size(); i++) {
TabModel tabModel = tabModels.get(i);
tabModel.addObserver(this);
}

onRegistrationComplete();
}

/**
* Notifies that the registration of the observers has been completed.
*/
protected void onRegistrationComplete() {
}

/**
* Destroys the observer and removes itself as a listener for Tab updates.
*/
public void destroy() {
if (mSelectorObserver != null) {
mTabModelSelector.removeObserver(mSelectorObserver);
mSelectorObserver = null;
}

List<TabModel> tabModels = mTabModelSelector.getModels();
for (int i = 0; i < tabModels.size(); i++) {
TabModel tabModel = tabModels.get(i);
tabModel.removeObserver(this);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,7 @@
public class TabModelSelectorTabObserver extends EmptyTabObserver {

private final TabModelSelector mTabModelSelector;
private final TabModelObserver mTabModelObserver;

private TabModelSelectorObserver mSelectorObserver;
private final TabModelSelectorTabModelObserver mTabModelObserver;

/**
* Constructs an observer that should be notified of tabs changes for all tabs owned
Expand All @@ -33,7 +31,7 @@ public class TabModelSelectorTabObserver extends EmptyTabObserver {
public TabModelSelectorTabObserver(TabModelSelector selector) {
mTabModelSelector = selector;

mTabModelObserver = new EmptyTabModelObserver() {
mTabModelObserver = new TabModelSelectorTabModelObserver(selector) {
@Override
public void didAddTab(Tab tab, TabLaunchType type) {
// This observer is automatically removed by tab when it is destroyed.
Expand All @@ -44,50 +42,27 @@ public void didAddTab(Tab tab, TabLaunchType type) {
public void tabRemoved(Tab tab) {
tab.removeObserver(TabModelSelectorTabObserver.this);
}
};

List<TabModel> tabModels = selector.getModels();
if (tabModels.isEmpty()) {
mSelectorObserver = new EmptyTabModelSelectorObserver() {
@Override
public void onNewTabCreated(Tab tab) {
assert false : "onChange should have happened and unregistered this listener.";
}

@Override
public void onChange() {
mTabModelSelector.removeObserver(this);
mSelectorObserver = null;
registerModelObservers();
@Override
protected void onRegistrationComplete() {
List<TabModel> tabModels = mTabModelSelector.getModels();
for (int i = 0; i < tabModels.size(); i++) {
TabModel tabModel = tabModels.get(i);
TabList comprehensiveTabList = tabModel.getComprehensiveModel();
for (int j = 0; j < comprehensiveTabList.getCount(); j++) {
comprehensiveTabList.getTabAt(j).addObserver(
TabModelSelectorTabObserver.this);
}
}
};
mTabModelSelector.addObserver(mSelectorObserver);
} else {
registerModelObservers();
}
}

private void registerModelObservers() {
List<TabModel> tabModels = mTabModelSelector.getModels();
for (int i = 0; i < tabModels.size(); i++) {
TabModel tabModel = tabModels.get(i);
tabModel.addObserver(mTabModelObserver);

TabList comprehensiveTabList = tabModel.getComprehensiveModel();
for (int j = 0; j < comprehensiveTabList.getCount(); j++) {
comprehensiveTabList.getTabAt(j).addObserver(this);
}
}
};
}

/**
* Destroys the observer and removes itself as a listener for Tab updates.
*/
public void destroy() {
if (mSelectorObserver != null) {
mTabModelSelector.removeObserver(mSelectorObserver);
mSelectorObserver = null;
}
mTabModelObserver.destroy();

List<TabModel> tabModels = mTabModelSelector.getModels();
for (int i = 0; i < tabModels.size(); i++) {
Expand Down
Loading

0 comments on commit 820a95b

Please sign in to comment.