Refactor handleMessage() functionality in fullscreen

chrome/android/java/src/org/chromium/chrome/browser/fullscreen/ChromeFullscreenManager.java

 // Copyright 2014 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.fullscreen;
 
 import android.animation.Animator;
 import android.animation.AnimatorListenerAdapter;
 import android.animation.ObjectAnimator;
 import android.app.Activity;
@@ skipping 17 matching lines @@
 import org.chromium.base.ApplicationStatus.ActivityStateListener;
 import org.chromium.base.BaseChromiumApplication;
 import org.chromium.base.BaseChromiumApplication.WindowFocusChangedListener;
 import org.chromium.base.TraceEvent;
 import org.chromium.base.VisibleForTesting;
 import org.chromium.chrome.browser.Tab;
 import org.chromium.chrome.browser.fullscreen.FullscreenHtmlApiHandler.FullscreenHtmlApiDelegate;
 import org.chromium.chrome.browser.tabmodel.TabModelSelector;
 import org.chromium.content.browser.ContentViewCore;
 
+import java.lang.ref.WeakReference;
 import java.util.ArrayList;
 import java.util.HashSet;
 
 /**
  * A class that manages control and content views to create the fullscreen mode.
  */
 public class ChromeFullscreenManager
         extends FullscreenManager implements ActivityStateListener, WindowFocusChangedListener {
     // Minimum showtime of the toolbar (in ms).
     private static final long MINIMUM_SHOW_DURATION_MS = 3000;
@@ skipping 93 matching lines @@
             if (mControlContainer.getVisibility() == visibility) return;
             // requestLayout is required to trigger a new gatherTransparentRegion(), which
             // only occurs together with a layout and let's SurfaceFlinger trim overlays.
             // This may be almost equivalent to using View.GONE, but we still use View.INVISIBLE
             // since drawing caches etc. won't be destroyed, and the layout may be less expensive.
             mControlContainer.setVisibility(visibility);
             mControlContainer.requestLayout();
         }
     };
 
+    // This static inner class holds a WeakReference to the outer object, to avoid triggering the
+    // lint HandlerLeak warning.
+    private static class FullscreenHandler extends Handler {
+        private final WeakReference<ChromeFullscreenManager> mChromeFullscreenManager;
+
+        public FullscreenHandler(ChromeFullscreenManager chromeFullscreenManager) {
+            mChromeFullscreenManager = new WeakReference<ChromeFullscreenManager>(
+                    chromeFullscreenManager);
+        }
+
+        @Override
+        public void handleMessage(Message msg) {
+            if (msg == null) return;
+            ChromeFullscreenManager chromeFullscreenManager = mChromeFullscreenManager.get();

[nyquist, 2015/01/10 01:27:05]

I wonder, are there any cases whatsoever where this could return null?
For example if a message is posted to the handler, and then the last real
reference to the ChromeFullscreenManager was removed before the message was
handled here.

Or was the thinking behind this CL that ChromeFullscreenManager owns this
handler, so it will not be an issue as the handler would be GCed before the CFM?

Same goes for the other class, which uses a similar pattern.

[wajahat, 2015/01/12 08:06:46]

The reasoning behind this CL was CFM would hold the handler, but on the safer
side i do agree we should validate for null and also for the other class
FSApiHandler.

+            switch (msg.what) {
+                case MSG_ID_CONTROLS_REQUEST_LAYOUT:
+                    chromeFullscreenManager.getControlContainer().requestLayout();
+                    break;
+                case MSG_ID_HIDE_CONTROLS:
+                    chromeFullscreenManager.update(false);
+                    break;
+                default:
+                    assert false : "Unexpected message for ID: " + msg.what;
+                    break;
+            }
+        }
+    }
+
     /**
      * Creates an instance of the fullscreen mode manager.
      * @param activity The activity that supports fullscreen.
      * @param controlContainer Container holding the controls (Toolbar).
      * @param enabled Whether fullscreen is globally enabled.
      * @param modelSelector The model selector providing access to the current tab.
      */
     public ChromeFullscreenManager(Activity activity, View controlContainer, boolean enabled,
             boolean persistentFullscreenSupported, TabModelSelector modelSelector,
             int resControlContainerHeight) {
         super(activity.getWindow(), modelSelector, enabled, persistentFullscreenSupported);
 
         mActivity = activity;
         ApplicationStatus.registerStateListenerForActivity(this, activity);
         ((BaseChromiumApplication) activity.getApplication())
                 .registerWindowFocusChangedListener(this);
 
         mWindow = activity.getWindow();
-        mHandler = new Handler() {
-            @Override
-            public void handleMessage(Message msg) {
-                if (msg == null) return;
-                switch (msg.what) {
-                    case MSG_ID_CONTROLS_REQUEST_LAYOUT:
-                        mControlContainer.requestLayout();
-                        break;
-                    case MSG_ID_HIDE_CONTROLS:
-                        update(false);
-                        break;
-                    default:
-                        assert false : "Unexpected message for ID: " + msg.what;
-                        break;
-                }
-            }
-        };
+        mHandler = new FullscreenHandler(this);
         setControlContainer(controlContainer);
         Resources resources = mWindow.getContext().getResources();
         mControlContainerHeight = resources.getDimensionPixelSize(resControlContainerHeight);
         mRendererContentOffset = mControlContainerHeight;
         mEnabled = enabled;
         updateControlOffset();
     }
 
     /**
      * @return Whether or not fullscreen is enabled.
      */
     public boolean isEnabled() {
         return mEnabled;
     }
 
     /**
      * Set the control container that is being hidden and shown when manipulating the fullscreen
      * state.
      * @param controlContainer The container at the top of the screen that contains the controls.
      */
     public void setControlContainer(View controlContainer) {
         assert controlContainer != null;
         mControlContainer = controlContainer;
     }
 
+    /**
+     * Gets the container at the top of the screen that contains the controls.
+     */
+    public View getControlContainer() {

[Ted C, 2015/01/09 19:53:36]

this doesn't need to be exposed (and shouldn't be public if it needed to be).

Just do:
chromeFullscreenManager.mControlContainer.requestLayout(); above.

[wajahat, 2015/01/12 08:06:46]

Done.

+        return mControlContainer;
+    }
+
     @Override
     public void onActivityStateChange(Activity activity, int newState) {
         if (newState == ActivityState.STOPPED) {
             // Exit fullscreen in onStop to ensure the system UI flags are set correctly when
             // showing again (on JB MR2+ builds, the omnibox would be covered by the
             // notification bar when this was done in onStart()).
             setPersistentFullscreenMode(false);
         } else if (newState == ActivityState.STARTED) {
             showControlsTransient();
         } else if (newState == ActivityState.DESTROYED) {
@@ skipping 491 matching lines @@
         mControlAnimation.start();
         mCurrentAnimationIsShowing = show;
     }
 
     @Override
     public void onContentViewScrollingStateChanged(boolean scrolling) {
         mContentViewScrolling = scrolling;
         if (!scrolling) updateVisuals();
     }
 }