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..f39ba5460fb354cab3202fa9d37178c7b73bfefc 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
@@ -25,6 +25,9 @@ import org.chromium.chrome.browser.util.UrlUtilities;
 import java.lang.annotation.Retention;
 import java.lang.annotation.RetentionPolicy;
 import java.util.Arrays;
+import java.util.HashSet;
+import java.util.Iterator;
+import java.util.Set;
 
 /**
  * A {@link ContextMenuPopulator} used for showing the default Chrome context menu.
@@ -202,61 +205,95 @@ public class ChromeContextMenuPopulator implements ContextMenuPopulator {
         menu.setGroupVisible(R.id.contextmenu_group_image, params.isImage());
         menu.setGroupVisible(R.id.contextmenu_group_video, params.isVideo());
 
+        menu.setGroupVisible(R.id.contextmenu_group_message, MailTo.isMailTo(params.getLinkUrl())
+                        || UrlUtilities.isTelScheme(params.getLinkUrl()));

[Ted C, 2017/02/28 06:10:13]

this looks to be indented too much...is this what git cl format/clang-format
tried to do?

Should be 8 from the start of the line

[JJ, 2017/03/01 01:41:04]

Yup! Sorry about that, it was yelling at me each time I tried to submit and this
must've yelled at me.

+
+        Set<Integer> menuIdSet = new HashSet<>(menu.size());
+        for (int i = 0; i < menu.size(); i++) {
+            MenuItem item = menu.getItem(i);
+            if (item.isVisible()) {
+                menuIdSet.add(item.getItemId());
+            }
+        }
+        trimMenuOptions(params, menuIdSet);
+
+        // 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++) {
+            MenuItem item = menu.getItem(i);
+            item.setVisible(menuIdSet.contains(item.getItemId()));
+        }
+
+        // Special case for searching by image element.
+        if (menuIdSet.contains(R.id.contextmenu_search_by_image)) {
+            menu.findItem(R.id.contextmenu_search_by_image)
+                    .setTitle(context.getString(R.string.contextmenu_search_web_for_image,
+                            TemplateUrlService.getInstance()
+                                    .getDefaultSearchEngineTemplateUrl()
+                                    .getShortName()));
+        }
+    }
+
+    /**
+     * Given a set of params and context, and a list of all the items. 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.
+     * @param allItems A set, with all the possible options for a link represented as ids Items will
+     *                 be removed based off the parameters.
+     */
+    private void trimMenuOptions(ContextMenuParams params, Set<Integer> allItems) {

[Ted C, 2017/02/28 06:10:13]

Instead of this getting the full list of items, what does this look like if it
returned a set of the disabled items (i.e. adding to the set instead of
removing).

Set<Integer> supportedOptions = new HashSet<>();
Collections.addAll(supportedOptions, BASE_WHITELIST);
Collections.addAll(supportedOptions, <mode specific whitelist>);

Set<Integer> disabledOptions = <this method call>;

for (int i = 0; i < menu.size(); i++) {
    item.setVisible(supportedOptions.contains(itemId) &&
!disabledOptions.contains(itemId));
}

It's not cutting down on a ton of iterations or anything, but I think having
methods have less access to the larger state is better.  In this case, there is
no enforcement that it is actually trimming as you could add as well, so making
it easier to enforce contracts is preferable to me.

[JJ, 2017/03/01 01:41:04]

Alright, take a look at what I did. While it does make one item have access to a
method, a second had to be made since adding all to the baselist doesn't
properly work.

Also the only way to access the groups is to access the entire menu and if we do
that then the point of this rather moot. So there's a second method to address
this that trims the basic options. I realize this is a little bit of a problem
and I do address this in a future cl (where we partially get rid of
chrome_context_menu.xml) so bare with me partially.

If you feel this adds more confusion feel free to tell me and we can revert.

         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.
@@ -266,73 +303,70 @@ public class ChromeContextMenuPopulator implements ContextMenuPopulator {
                     DataReductionProxyUma.ACTION_LOFI_LOAD_IMAGES_CONTEXT_MENU_SHOWN);
         }
 
+        boolean isSrcDownloadableScheme = UrlUtilities.isDownloadableScheme(params.getSrcUrl());
         if (params.isVideo()) {
-            menu.findItem(R.id.contextmenu_save_video).setVisible(
-                    params.canSaveMedia() && UrlUtilities.isDownloadableScheme(params.getSrcUrl()));
+            boolean saveableAndDownloadable = params.canSaveMedia() && isSrcDownloadableScheme;
+            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 (!isSrcDownloadableScheme) {
+                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 =
-                    UrlUtilities.isDownloadableScheme(params.getSrcUrl())
-                            && templateUrlServiceInstance.isLoaded()
-                            && templateUrlServiceInstance.isSearchByImageAvailable()
-                            && templateUrlServiceInstance.getDefaultSearchEngineTemplateUrl()
-                                    != null;
-
-            menu.findItem(R.id.contextmenu_search_by_image).setVisible(isSearchByImageAvailable);
-            if (isSearchByImageAvailable) {
-                menu.findItem(R.id.contextmenu_search_by_image).setTitle(
-                        context.getString(R.string.contextmenu_search_web_for_image,
-                                TemplateUrlService.getInstance()
-                                        .getDefaultSearchEngineTemplateUrl().getShortName()));
+            final boolean isSearchByImageAvailable = isSrcDownloadableScheme
+                    && templateUrlServiceInstance.isLoaded()
+                    && templateUrlServiceInstance.isSearchByImageAvailable()
+                    && templateUrlServiceInstance.getDefaultSearchEngineTemplateUrl() != null;
+
+            if (!isSearchByImageAvailable) {
+                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);
         }
     }
 
-    private void removeUnsupportedItems(ContextMenu menu, int[] whitelist) {
+    private void removeUnsupportedItems(Set<Integer> 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--;
+        for (Iterator<Integer> idIterator = itemList.iterator(); idIterator.hasNext();) {
+            int itemId = idIterator.next();
+            if (Arrays.binarySearch(whitelist, itemId) < 0
+                    && Arrays.binarySearch(BASE_WHITELIST, itemId) < 0) {
+                idIterator.remove();
             }
         }
     }