Refactor ChromeContextMenuPopulator population

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

Index: chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java
diff --git a/chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java b/chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java
index bf752e3ea1002fcd33d3a72c980ce8a23f4617c3..da96ebf7d58de440a670daccdbdcf7068b7c785e 100644
--- a/chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java
+++ b/chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java
@@ -10,7 +10,6 @@ import android.support.annotation.IntDef;
 import android.text.TextUtils;
 import android.view.ContextMenu;
 import android.view.MenuInflater;
-import android.view.MenuItem;
 import android.webkit.MimeTypeMap;
 
 import org.chromium.base.metrics.RecordHistogram;
@@ -24,7 +23,11 @@ import org.chromium.chrome.browser.util.UrlUtilities;
 
 import java.lang.annotation.Retention;
 import java.lang.annotation.RetentionPolicy;
+import java.util.ArrayList;
 import java.util.Arrays;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
 
 /**
  * A {@link ContextMenuPopulator} used for showing the default Chrome context menu.
@@ -202,61 +205,92 @@ public class ChromeContextMenuPopulator implements ContextMenuPopulator {
         menu.setGroupVisible(R.id.contextmenu_group_image, params.isImage());
         menu.setGroupVisible(R.id.contextmenu_group_video, params.isVideo());
 
+        boolean isGroupMessage = MailTo.isMailTo(params.getLinkUrl())

[David Trainor- moved to gerrit, 2017/02/22 05:36:33]

Looks like we call these methods below too.  Can we pull these out to final
booleans and use them in both places?  Then we could either keep the
isGropuMessage boolean or do setGroupVisible(..., isMailTo || isTelScheme).

[JJ, 2017/02/23 03:29:14]

Sorry, missed this. Pushed it inside of the url.

+                || UrlUtilities.isTelScheme(params.getLinkUrl());
+        menu.setGroupVisible(R.id.contextmenu_group_message, isGroupMessage);
+
+        Map<Integer, String> menuIdMap = new HashMap<>(menu.size());

[Ted C, 2017/02/22 06:10:54]

Take a look at 
https://developer.android.com/reference/android/util/SparseArray.html

If the APIs work, it is better to use than a HashMap of Integers to other
things.

[JJ, 2017/02/22 19:09:39]

So I did look at this before I put it up thinking if this was better to use.
After looking at what the main purpose of our hashmap, which was to remove items
I realized that it might not be the best as it'll take O(1)ish time in a hashmap
and O(log n) time for each item. So I decided against it. However! If you think
it's for the best I'll be happy to change it.

+        for (int i = 0; i < menu.size(); i++) {
+            if (menu.getItem(i).isVisible()) {

[David Trainor- moved to gerrit, 2017/02/22 05:36:33]

Can we pull out menu.getItem(i) to a local?

[JJ, 2017/02/22 19:09:39]

Done.

+                menuIdMap.put(menu.getItem(i).getItemId(), (String) menu.getItem(i).getTitle());
+            }
+        }
+        menuIdMap = getListOfOptions(params, context, menuIdMap);

[David Trainor- moved to gerrit, 2017/02/22 05:36:33]

Do we have to return menuIdMap here?  Might make it clearer that the input is
being modified if we don't return the value and make it something like
trimMenuOptions()?

[Ted C, 2017/02/22 06:10:54]

looks like dtrainor@ left comments, but I'll put this out there.  Should we
consider doing the inverse.  Basically, get rid of groups entirely from
chrome_context_menu.xml and instead have this method get the full list of valid
options  (adding to the map instead of removing).

I think medium-term, we should consider ditching chrome_context_menu entirely
and have it just be built in entirely in code here, but I think that might be
more than the intent of this change.

The gotcha is still the title.  I think for now, I'd suggest having it be a
container of ID -> title override.  All places except search by image would be
null, then in the one place you would apply an override.  I think we'll want to
find a way to encode the ordering along with title information if we move to
entirely be within this file, but again, keeping it "somewhat" shorter for now.

[JJ, 2017/02/22 19:09:39]

No we don't need to return it at all! I'm hesitating from lack of arguments (if
that was your intention) but other than that I can modify it.

[JJ, 2017/02/22 19:09:39]

I actually was thinking about this heavily when looking through the code
earlier. I wrote about it in the caveats
(go/clank-context-doc#heading=h.ydr456a07ivp) wondering if it was possible to
move to an option based problem. I went against it but was open to change about
it as shown in the comments. If need be I can add a bug for it and focus on that
later on as I'm also a little...hesitant about it myself.

+
+        // Iterate through the entire menu list, if if doesn't exist in the map, don't show it.
+        for (int i = 0; i < menu.size(); i++) {
+            boolean inMenuList = menuIdMap.containsKey(menu.getItem(i).getItemId());

[David Trainor- moved to gerrit, 2017/02/22 05:36:33]

Pull out menu.getItem(i) again imo

[JJ, 2017/02/22 19:09:39]

Done.

+            menu.getItem(i).setVisible(inMenuList);
+            if (inMenuList) {
+                menu.getItem(i).setTitle(menuIdMap.get(menu.getItem(i).getItemId()));
+            }
+        }
+    }
+
+    /**
+     * Given a set of params and context, and a list of all the items, it'll return a list of items

[David Trainor- moved to gerrit, 2017/02/22 05:36:33]

remove it'll?

[JJ, 2017/02/22 19:09:38]

Done.

+     * that should be displayed. This method never adds items, only removes or edits them from the
+     * list.
+     * @param params The parameters used for filtering out the the list of items.

[David Trainor- moved to gerrit, 2017/02/22 05:36:33]

Do we need @param context in here (even with no description)?

[JJ, 2017/02/22 19:09:38]

Question for the future: Is it alright to have a param that has no description?

+     * @param allItems A list, with all the possible options for a link. Items will be removed based

[David Trainor- moved to gerrit, 2017/02/22 05:36:33]

Can we describe what the format of the map is?

[JJ, 2017/02/22 19:09:38]

Done.

+     *                 off the parameters.
+     * @return Returns a map containing the id and string of all the items that are allowed to be
+     * on the menu.
+     */
+    private Map<Integer, String> getListOfOptions(
+            ContextMenuParams params, Context context, Map<Integer, String> allItems) {
         if (params.isAnchor() && !mDelegate.isOpenInOtherWindowSupported()) {
-            menu.findItem(R.id.contextmenu_open_in_other_window).setVisible(false);
+            allItems.remove(R.id.contextmenu_open_in_other_window);
         }
 
         if (mDelegate.isIncognito() || !mDelegate.isIncognitoSupported()) {
-            menu.findItem(R.id.contextmenu_open_in_incognito_tab).setVisible(false);
+            allItems.remove(R.id.contextmenu_open_in_incognito_tab);
         }
 
         if (params.getLinkText().trim().isEmpty() || params.isImage()) {
-            menu.findItem(R.id.contextmenu_copy_link_text).setVisible(false);
+            allItems.remove(R.id.contextmenu_copy_link_text);
         }
 
         if (params.isAnchor() && !UrlUtilities.isAcceptedScheme(params.getLinkUrl())) {
-            menu.findItem(R.id.contextmenu_open_in_other_window).setVisible(false);
-            menu.findItem(R.id.contextmenu_open_in_new_tab).setVisible(false);
-            menu.findItem(R.id.contextmenu_open_in_incognito_tab).setVisible(false);
+            allItems.remove(R.id.contextmenu_open_in_other_window);
+            allItems.remove(R.id.contextmenu_open_in_new_tab);
+            allItems.remove(R.id.contextmenu_open_in_incognito_tab);
         }
 
         if (MailTo.isMailTo(params.getLinkUrl())) {
-            menu.findItem(R.id.contextmenu_copy_link_text).setVisible(false);
-            menu.findItem(R.id.contextmenu_copy_link_address).setVisible(false);
-            menu.setGroupVisible(R.id.contextmenu_group_message, true);
+            allItems.remove(R.id.contextmenu_copy_link_text);
+            allItems.remove(R.id.contextmenu_copy_link_address);
             if (!mDelegate.supportsSendEmailMessage()) {
-                menu.findItem(R.id.contextmenu_send_message).setVisible(false);
+                allItems.remove(R.id.contextmenu_send_message);
             }
             if (TextUtils.isEmpty(MailTo.parse(params.getLinkUrl()).getTo())
                     || !mDelegate.supportsAddToContacts()) {
-                menu.findItem(R.id.contextmenu_add_to_contacts).setVisible(false);
+                allItems.remove(R.id.contextmenu_add_to_contacts);
             }
-            menu.findItem(R.id.contextmenu_call).setVisible(false);
+            allItems.remove(R.id.contextmenu_call);
         } else if (UrlUtilities.isTelScheme(params.getLinkUrl())) {
-            menu.findItem(R.id.contextmenu_copy_link_text).setVisible(false);
-            menu.findItem(R.id.contextmenu_copy_link_address).setVisible(false);
-            menu.setGroupVisible(R.id.contextmenu_group_message, true);
+            allItems.remove(R.id.contextmenu_copy_link_text);
+            allItems.remove(R.id.contextmenu_copy_link_address);
             if (!mDelegate.supportsCall()) {
-                menu.findItem(R.id.contextmenu_call).setVisible(false);
+                allItems.remove(R.id.contextmenu_call);
             }
             if (!mDelegate.supportsSendTextMessage()) {
-                menu.findItem(R.id.contextmenu_send_message).setVisible(false);
+                allItems.remove(R.id.contextmenu_send_message);
             }
             if (!mDelegate.supportsAddToContacts()) {
-                menu.findItem(R.id.contextmenu_add_to_contacts).setVisible(false);
+                allItems.remove(R.id.contextmenu_add_to_contacts);
             }
-        } else {
-            menu.setGroupVisible(R.id.contextmenu_group_message, false);
         }
 
-        menu.findItem(R.id.contextmenu_save_link_as).setVisible(
-                UrlUtilities.isDownloadableScheme(params.getLinkUrl()));
+        if (!UrlUtilities.isDownloadableScheme(params.getLinkUrl())) {
+            allItems.remove(R.id.contextmenu_save_link_as);
+        }
 
         if (params.imageWasFetchedLoFi()
                 || !DataReductionProxySettings.getInstance().wasLoFiModeActiveOnMainFrame()
                 || !DataReductionProxySettings.getInstance().canUseDataReductionProxy(
                         params.getPageUrl())) {
-            menu.findItem(R.id.contextmenu_load_images).setVisible(false);
+            allItems.remove(R.id.contextmenu_load_images);
         } else {
             // Links can have images as backgrounds that aren't recognized here as images. CSS
             // properties can also prevent an image underlying a link from being clickable.
@@ -267,26 +301,30 @@ public class ChromeContextMenuPopulator implements ContextMenuPopulator {
         }
 
         if (params.isVideo()) {
-            menu.findItem(R.id.contextmenu_save_video).setVisible(
-                    params.canSaveMedia() && UrlUtilities.isDownloadableScheme(params.getSrcUrl()));
+            boolean saveableAndDownloadable =
+                    params.canSaveMedia() && UrlUtilities.isDownloadableScheme(params.getSrcUrl());

[David Trainor- moved to gerrit, 2017/02/22 05:36:33]

pull out isDownloadableScheme(params.getSrcUrl()) and use it here and below?

[JJ, 2017/02/23 03:29:14]

Done.

+            if (!saveableAndDownloadable) {
+                allItems.remove(R.id.contextmenu_save_video);
+            }
         } else if (params.isImage() && params.imageWasFetchedLoFi()) {
             DataReductionProxyUma.previewsLoFiContextMenuAction(
                     DataReductionProxyUma.ACTION_LOFI_LOAD_IMAGE_CONTEXT_MENU_SHOWN);
             // All image context menu items other than "Load image," "Open original image in
             // new tab," and "Copy image URL" should be disabled on Lo-Fi images.
-            menu.findItem(R.id.contextmenu_save_image).setVisible(false);
-            menu.findItem(R.id.contextmenu_open_image).setVisible(false);
-            menu.findItem(R.id.contextmenu_search_by_image).setVisible(false);
-            menu.findItem(R.id.contextmenu_share_image).setVisible(false);
+            allItems.remove(R.id.contextmenu_save_image);
+            allItems.remove(R.id.contextmenu_open_image);
+            allItems.remove(R.id.contextmenu_search_by_image);
+            allItems.remove(R.id.contextmenu_share_image);
         } else if (params.isImage() && !params.imageWasFetchedLoFi()) {
-            menu.findItem(R.id.contextmenu_load_original_image).setVisible(false);
+            allItems.remove(R.id.contextmenu_load_original_image);
 
-            menu.findItem(R.id.contextmenu_save_image).setVisible(
-                    UrlUtilities.isDownloadableScheme(params.getSrcUrl()));
+            if (!UrlUtilities.isDownloadableScheme(params.getSrcUrl())) {
+                allItems.remove(R.id.contextmenu_save_image);
+            }
 
             // Avoid showing open image option for same image which is already opened.
             if (mDelegate.getPageUrl().equals(params.getSrcUrl())) {
-                menu.findItem(R.id.contextmenu_open_image).setVisible(false);
+                allItems.remove(R.id.contextmenu_open_image);
             }
             final TemplateUrlService templateUrlServiceInstance = TemplateUrlService.getInstance();
             final boolean isSearchByImageAvailable =
@@ -296,43 +334,47 @@ public class ChromeContextMenuPopulator implements ContextMenuPopulator {
                             && templateUrlServiceInstance.getDefaultSearchEngineTemplateUrl()
                                     != null;
 
-            menu.findItem(R.id.contextmenu_search_by_image).setVisible(isSearchByImageAvailable);
             if (isSearchByImageAvailable) {
-                menu.findItem(R.id.contextmenu_search_by_image).setTitle(
+                allItems.put(R.id.contextmenu_search_by_image,

[David Trainor- moved to gerrit, 2017/02/22 05:36:32]

Does this break the rule?  Seems like this adds the item even if it isn't there.
 Does this method need to be responsible for creating titles?  Looks like this
is the only thing that does this.  Can we pull this out to the parent caller
method and just pass a Set<Integer> of the ids instead of a Map<>?

[JJ, 2017/02/22 19:09:39]

So the initial idea was to set up this method as a way for us to get rid of
context_menu_xml in the future, kind of creating an id that as long as there is
a way to refer to the original value and a title it'll produce proper stats.
However you're right, this does break a rule and should be removed.

                         context.getString(R.string.contextmenu_search_web_for_image,
                                 TemplateUrlService.getInstance()
-                                        .getDefaultSearchEngineTemplateUrl().getShortName()));
+                                        .getDefaultSearchEngineTemplateUrl()
+                                        .getShortName()));
+            } else {
+                allItems.remove(R.id.contextmenu_search_by_image);
             }
         }
 
         // Hide all items that could spawn additional tabs until FRE has been completed.
         if (!FirstRunStatus.getFirstRunFlowComplete()) {
-            menu.findItem(R.id.contextmenu_open_image_in_new_tab).setVisible(false);
-            menu.findItem(R.id.contextmenu_open_in_other_window).setVisible(false);
-            menu.findItem(R.id.contextmenu_open_in_new_tab).setVisible(false);
-            menu.findItem(R.id.contextmenu_open_in_incognito_tab).setVisible(false);
-            menu.findItem(R.id.contextmenu_search_by_image).setVisible(false);
-            menu.findItem(R.id.menu_id_open_in_chrome).setVisible(false);
+            allItems.remove(R.id.contextmenu_open_image_in_new_tab);
+            allItems.remove(R.id.contextmenu_open_in_other_window);
+            allItems.remove(R.id.contextmenu_open_in_new_tab);
+            allItems.remove(R.id.contextmenu_open_in_incognito_tab);
+            allItems.remove(R.id.contextmenu_search_by_image);
+            allItems.remove(R.id.menu_id_open_in_chrome);
         }
 
         if (mMode == FULLSCREEN_TAB_MODE) {
-            removeUnsupportedItems(menu, FULLSCREEN_TAB_MODE_WHITELIST);
+            removeUnsupportedItems(allItems, FULLSCREEN_TAB_MODE_WHITELIST);
         } else if (mMode == CUSTOM_TAB_MODE) {
-            removeUnsupportedItems(menu, CUSTOM_TAB_MODE_WHITELIST);
+            removeUnsupportedItems(allItems, CUSTOM_TAB_MODE_WHITELIST);
         } else {
-            removeUnsupportedItems(menu, NORMAL_MODE_WHITELIST);
+            removeUnsupportedItems(allItems, NORMAL_MODE_WHITELIST);
         }
+
+        return allItems;
     }
 
-    private void removeUnsupportedItems(ContextMenu menu, int[] whitelist) {
+    private void removeUnsupportedItems(Map<Integer, String> itemList, int[] whitelist) {
         Arrays.sort(BASE_WHITELIST);
         Arrays.sort(whitelist);
-        for (int i = 0; i < menu.size(); i++) {
-            MenuItem item = menu.getItem(i);
-            if (Arrays.binarySearch(whitelist, item.getItemId()) < 0
-                    && Arrays.binarySearch(BASE_WHITELIST, item.getItemId()) < 0) {
-                menu.removeItem(item.getItemId());
-                i--;
+        List<Integer> keyList = new ArrayList<>(itemList.keySet());
+        for (int i = 0; i < keyList.size(); i++) {
+            int itemId = keyList.get(i);
+            if (Arrays.binarySearch(whitelist, itemId) < 0
+                    && Arrays.binarySearch(BASE_WHITELIST, itemId) < 0) {
+                itemList.remove(itemId);
             }
         }
     }