Refactor handleMessage() functionality in fullscreen

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

Index: chrome/android/java/src/org/chromium/chrome/browser/fullscreen/ChromeFullscreenManager.java
diff --git a/chrome/android/java/src/org/chromium/chrome/browser/fullscreen/ChromeFullscreenManager.java b/chrome/android/java/src/org/chromium/chrome/browser/fullscreen/ChromeFullscreenManager.java
index 974ff2037ff4793325de81dfde61a45289f14e5c..7174c52b01b29267d3b2599aa5635d8d9b988892 100644
--- a/chrome/android/java/src/org/chromium/chrome/browser/fullscreen/ChromeFullscreenManager.java
+++ b/chrome/android/java/src/org/chromium/chrome/browser/fullscreen/ChromeFullscreenManager.java
@@ -35,6 +35,7 @@ import org.chromium.chrome.browser.fullscreen.FullscreenHtmlApiHandler.Fullscree
 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;
 
@@ -148,6 +149,34 @@ public class ChromeFullscreenManager
         }
     };
 
+    // 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.
@@ -166,23 +195,7 @@ public class ChromeFullscreenManager
                 .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);
@@ -208,6 +221,13 @@ public class ChromeFullscreenManager
         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) {