Split context menu display and population/handling

chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuHelper.java

Index: chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuHelper.java
diff --git a/chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuHelper.java b/chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuHelper.java
index 529b678e4d6d6f3afb955caabd95fd7a5392c1c5..69f59b97366d93e1678190f8d985856c8c9dc92e 100644
--- a/chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuHelper.java
+++ b/chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuHelper.java
@@ -5,6 +5,7 @@
 package org.chromium.chrome.browser.contextmenu;
 
 import android.app.Activity;
+import android.util.Pair;
 import android.view.ContextMenu;
 import android.view.ContextMenu.ContextMenuInfo;
 import android.view.MenuItem;
@@ -21,6 +22,8 @@ import org.chromium.content_public.browser.WebContents;
 import org.chromium.ui.base.WindowAndroid;
 import org.chromium.ui.base.WindowAndroid.OnCloseContextMenuListener;
 
+import java.util.List;
+
 /**
  * A helper class that handles generating context menus for {@link ContentViewCore}s.
  */
@@ -29,6 +32,7 @@ public class ContextMenuHelper implements OnCreateContextMenuListener, OnMenuIte
 
     private ContextMenuPopulator mPopulator;
     private ContextMenuParams mCurrentContextMenuParams;
+    private Activity mActivity;

[JJ, 2017/03/10 18:01:16]

If you're wondering why we get activity instead of context, it's because in a
future cl regards to sharing we need the activity to share properly and get the
share icon.

 
     private ContextMenuHelper(long nativeContextMenuHelper) {
         mNativeContextMenuHelper = nativeContextMenuHelper;
@@ -71,6 +75,7 @@ public class ContextMenuHelper implements OnCreateContextMenuListener, OnMenuIte
         }
 
         mCurrentContextMenuParams = params;
+        mActivity = windowAndroid.getActivity().get();

[Ted C, 2017/03/11 00:31:14]

if the activity is null, we should return as well

[JJ, 2017/03/13 20:24:28]

Done.

 
         view.setOnCreateContextMenuListener(this);

[Ted C, 2017/03/11 00:31:14]

Can this move into the ui code?  This is specific to the platform ContextMenu
flow, so we should ideally isolate it to that ui only.

You could have displayMenu return a boolean and we could call it right here. 
Then the following logic would remain the same.

Then we could call the populator before this and if there are no groups then we
also return.  I think a bunch of the logic would be simplified.

[JJ, 2017/03/13 20:24:28]

In a previous iteration of this (before it was uploaded) there was a separate
method to do this. And it almost worked! But I hit a massive wall with how
Android likes to hand us a Context Menu and what we do with that info. It meant
that in doing this we would have to rewrite a decently sized portion of code.
From the majority of the tests to work with the contextual search (though
luckily this doesn't rely on the menu as much as the tests do). While I do agree
that the end result should put this in the UI (cause it makes sense there) I'd
suggest to do it closer to getting rid of the ContextMenu, in a seperate CL
(cause this one is getting super large as it is) where our tests will hopefully
allow us to completely separate how the two functions are setup.

[Ted C, 2017/03/13 21:55:19]

Seems ok to do in a follow up.

[JJ, 2017/03/13 23:39:32]

Thanks thanks.

         if (view.showContextMenu()) {
@@ -128,11 +133,18 @@ public class ContextMenuHelper implements OnCreateContextMenuListener, OnMenuIte
     @Override
     public void onCreateContextMenu(ContextMenu menu, View v, ContextMenuInfo menuInfo) {
         assert mPopulator != null;
-        mPopulator.buildContextMenu(menu, v.getContext(), mCurrentContextMenuParams);
-
-        for (int i = 0; i < menu.size(); i++) {
-            menu.getItem(i).setOnMenuItemClickListener(this);
-        }
+        List<Pair<Integer, List<ContextMenuItems>>> items =
+                mPopulator.buildContextMenu(menu, v.getContext(), mCurrentContextMenuParams);
+        ContextMenuUi menuUi =
+                new SimpleContextMenuUi(menu, new ContextMenuUi.OnMenuClickedListener() {

[Ted C, 2017/03/11 00:31:14]

Take a look at Callback.java in base instead of adding an interface that has one
method with one param.  It's all the rage :-P

[JJ, 2017/03/13 20:24:28]

Is that what the youngins are into these days? I need to get hip with these
trends.

+                    @Override
+                    public void onItemClicked(int menuId) {
+                        mPopulator.onItemSelected(
+                                ContextMenuHelper.this, mCurrentContextMenuParams, menuId);
+                    }
+                });
+
+        menuUi.displayMenu(mActivity, mCurrentContextMenuParams, items);

[Ted C, 2017/03/11 00:31:14]

Also if both UIs are going to need the click listener, I would add it as a param
here to make even more code shared.

[JJ, 2017/03/13 20:24:29]

Done.

     }
 
     @Override