Skip to content

Commit

Permalink
Register and unregister CaptionManager listener in attach/detach
Browse files Browse the repository at this point in the history
Main goal is make sure the listener is not left registered indefinitely
in Android WebView, which does not guarantee ContentViewCore.destroy to
be called.

This is probably a good change for chrome as well. Tabs in the
background should not care about caption changes. And now it no longer
registers a listener per tab, which may be a super high number.

BUG=457850, 469803

Review URL: https://codereview.chromium.org/1093353002

Cr-Commit-Position: refs/heads/master@{#326127}
  • Loading branch information
boliu authored and Commit bot committed Apr 21, 2015
1 parent cfe1892 commit ea6c309
Show file tree
Hide file tree
Showing 4 changed files with 32 additions and 11 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -989,7 +989,6 @@ public void destroy() {
if (mNativeContentViewCore != 0) {
nativeOnJavaContentViewCoreDestroyed(mNativeContentViewCore);
}
mSystemCaptioningBridge.destroy();
mWebContentsObserver.destroy();
mWebContentsObserver = null;
setSmartClipDataListener(null);
Expand Down Expand Up @@ -1506,6 +1505,7 @@ public void onAttachedToWindow() {
restoreSelectionPopupsIfNecessary();
ScreenOrientationListener.getInstance().addObserver(this, mContext);
GamepadList.onAttachedToWindow(mContext);
mSystemCaptioningBridge.registerBridge();
}

/**
Expand All @@ -1528,6 +1528,7 @@ public void onDetachedFromWindow() {
// locking and app switching.
setTextHandlesTemporarilyHidden(true);
hidePopupsAndPreserveSelection();
mSystemCaptioningBridge.unregisterBridge();
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,10 +12,18 @@ public class EmptyCaptioningBridge implements SystemCaptioningBridge {
/**
* A no-op implementation of the syncToDelegate function.
*/
@Override
public void syncToDelegate() {}

/**
* A no-op implementation of the destroy function.
* No-op implementation of registerBridge.
*/
public void destroy() {}
@Override
public void registerBridge() {}

/**
* A no-op implementation of the unregisterBridge function.
*/
@Override
public void unregisterBridge() {}
}
Original file line number Diff line number Diff line change
Expand Up @@ -63,13 +63,12 @@ public KitKatCaptioningBridge(ContentViewCore contenViewCore) {
mCaptioningManager = (CaptioningManager) contenViewCore.getContext()
.getApplicationContext()
.getSystemService(Context.CAPTIONING_SERVICE);
mCaptioningManager.addCaptioningChangeListener(mCaptioningChangeListener);
syncToDelegate();
}

/**
* Force-sync the current closed caption settings to the delegate
*/
@Override
public void syncToDelegate() {
mCaptioningChangeDelegate.onEnabledChanged(mCaptioningManager.isEnabled());
mCaptioningChangeDelegate.onFontScaleChanged(mCaptioningManager.getFontScale());
Expand All @@ -79,10 +78,19 @@ public void syncToDelegate() {
}

/**
* De-register this bridge from the system captioning manager. This bridge
* should not be used again after this is called.
* Register this bridge for event changes with the system CaptioningManager.
*/
@Override
public void registerBridge() {
mCaptioningManager.addCaptioningChangeListener(mCaptioningChangeListener);
syncToDelegate();
}

/**
* De-register this bridge from the system captioning manager.
*/
public void destroy() {
@Override
public void unregisterBridge() {
mCaptioningManager.removeCaptioningChangeListener(mCaptioningChangeListener);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,12 @@ public interface SystemCaptioningBridge {
public void syncToDelegate();

/**
* Removes any external listeners that were added. This implementation doesn't do anything.
* After destroy is called, this object should not be used again.
* Register this bridge for event changes with the system CaptioningManager.
*/
public void destroy();
public void registerBridge();

/**
* Unregister this bridge from system CaptionManager. Must be called to avoid leaks.
*/
public void unregisterBridge();
}

0 comments on commit ea6c309

Please sign in to comment.