📰 Add a context menu to remove status items

chrome/android/java/src/org/chromium/chrome/browser/ntp/ContextMenuHandler.java

Index: chrome/android/java/src/org/chromium/chrome/browser/ntp/ContextMenuHandler.java
diff --git a/chrome/android/java/src/org/chromium/chrome/browser/ntp/ContextMenuHandler.java b/chrome/android/java/src/org/chromium/chrome/browser/ntp/ContextMenuHandler.java
index 3752485e7c26e672950bd4aaba524f66d6669256..f02ed66387a542c6cc5915516b2d9f5c5518b433 100644
--- a/chrome/android/java/src/org/chromium/chrome/browser/ntp/ContextMenuHandler.java
+++ b/chrome/android/java/src/org/chromium/chrome/browser/ntp/ContextMenuHandler.java
@@ -27,12 +27,12 @@
     @IntDef({ID_OPEN_IN_NEW_WINDOW, ID_OPEN_IN_NEW_TAB, ID_OPEN_IN_INCOGNITO_TAB, ID_REMOVE,
             ID_SAVE_FOR_OFFLINE})
     @Retention(RetentionPolicy.SOURCE)
-    @interface ContextMenuItemId {}
-    static final int ID_OPEN_IN_NEW_WINDOW = 0;
-    static final int ID_OPEN_IN_NEW_TAB = 1;
-    static final int ID_OPEN_IN_INCOGNITO_TAB = 2;
-    static final int ID_REMOVE = 3;
-    static final int ID_SAVE_FOR_OFFLINE = 4;
+    public @interface ContextMenuItemId {}
+    public static final int ID_OPEN_IN_NEW_WINDOW = 0;
+    public static final int ID_OPEN_IN_NEW_TAB = 1;
+    public static final int ID_OPEN_IN_INCOGNITO_TAB = 2;
+    public static final int ID_REMOVE = 3;
+    public static final int ID_SAVE_FOR_OFFLINE = 4;
 
     private final NewTabPageManager mManager;
     private final Delegate mDelegate;
@@ -58,7 +58,6 @@
 
     public ContextMenuHandler(NewTabPageManager newTabPageManager, TouchDisableableView outerView,
             Delegate delegate) {
-        assert outerView instanceof View;
         mManager = newTabPageManager;
         mOuterView = outerView;
         mDelegate = delegate;
@@ -82,7 +81,7 @@ public boolean showItem(@ContextMenuItemId int itemId) {
         }
     }
 
-    public void onCreateContextMenu(ContextMenu menu) {
+    public void onCreateContextMenu(ContextMenu menu, final View associatedView) {
         if (showItem(ID_OPEN_IN_NEW_WINDOW)) {
             addContextMenuItem(
                     menu, ID_OPEN_IN_NEW_WINDOW, R.string.contextmenu_open_in_other_window);
@@ -111,11 +110,14 @@ public void onCreateContextMenu(ContextMenu menu) {
         // https://crbug.com/636296)
         mOuterView.setTouchEnabled(false);
 
+        mManager.getContextMenuLifetimeManager().onContextMenuCreated(menu, associatedView);

[Bernhard Bauer, 2016/11/11 13:30:08]

Wait, so the manager owns the LifetimeManager, which is defined by this class
and has a private method that only we are able to call? That seems like very
tight coupling. Could we move this into its own class? And maybe we could move
the contextMenuCloseCallback methods to that class as well, making it a
ContextMenuManager or some'n?

[dgn, 2016/11/14 18:34:08]

Done.

+
         mManager.addContextMenuCloseCallback(new Callback<Menu>() {
             @Override
             public void onResult(Menu result) {
                 mOuterView.setTouchEnabled(true);
                 mManager.removeContextMenuCloseCallback(this);
+                mManager.getContextMenuLifetimeManager().onMenuClosed();
             }
         });
     }
@@ -157,4 +159,27 @@ public boolean onMenuItemClick(MenuItem item) {
     private void addContextMenuItem(ContextMenu menu, @ContextMenuItemId int id, int resourceId) {
         menu.add(Menu.NONE, id, Menu.NONE, resourceId).setOnMenuItemClickListener(this);
     }
+
+    /** Helps closing open context menus when the view the associated view is modified. */
+    public static class LifetimeManager {
+        private ContextMenu mContextMenu;
+        private View mAssociatedView;
+
+        public void onContextMenuCreated(ContextMenu c, View v) {
+            mContextMenu = c;
+            mAssociatedView = v;
+        }
+
+        public void onViewModified(View v) {
+            if (mAssociatedView == null || mAssociatedView != v) return;

[Bernhard Bauer, 2016/11/11 13:30:08]

|mAssociatedView| would be null if either that is passed at construction time,
or the menu is closed, right? In the former case, that will make the whole class
a no-op, so the client could just skip creating it, and in the latter case the
next line will catch it anyway.

[dgn, 2016/11/14 18:34:08]

Simplified this and removed the redundant checks.

+            if (mContextMenu == null) return;
+            mContextMenu.close();
+            mContextMenu = null;
+        }
+
+        private void onMenuClosed() {
+            mContextMenu = null;
+            mAssociatedView = null;
+        }
+    }
 }