Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2710833002:
Refactor ChromeContextMenuPopulator population (Closed)
DescriptionRefactor ChromeContextMenuPopulator population
In preparation for the redesign of Chrome's Context Menu this cl
refactors the population into a seperate method. As a result of this
refactor, several tests are now added to confirm that the context menu
shows what is expected in a context (which surprisingly wasn't there
beforehand).
BUG=655359
Review-Url: https://codereview.chromium.org/2710833002
Cr-Commit-Position: refs/heads/master@{#454077}
Committed: https://chromium.googlesource.com/chromium/src/+/4de4918b64f6fc0bdd4022d452bd693a785d6074
Patch Set 1 #
Total comments: 25
Patch Set 2 : Fixed based off tedchoc #Patch Set 3 : Two comments I missed #
Total comments: 8
Patch Set 4 : Fixed based off rebase + tedchoc comments #
Total comments: 1
Patch Set 5 : Added clarifying comment #
Total comments: 12
Patch Set 6 : Fiexed based off tedchoc comments #
Total comments: 5
Patch Set 7 : Fixed based off Ted's comments #
Messages
Total messages: 31 (15 generated)
jwanda@chromium.org changed reviewers: + dtrainor@chromium.org, tedchoc@chromium.org
Hello dtrainor@chromium.org and tedchoc@chromium.org, you both seem to have had the most recent reviews within these two files. If you feel like you cannot get to this at a reasonable time or feel like there's someone better to send it to, please feel free to reassign to anyone you feel that this should go to. Thanks!
Just one bit of info on a method I made. https://codereview.chromium.org/2710833002/diff/1/chrome/android/javatests/sr... File chrome/android/javatests/src/org/chromium/chrome/browser/contextmenu/ContextMenuTest.java (right): https://codereview.chromium.org/2710833002/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/contextmenu/ContextMenuTest.java:418: private boolean confirmMenuList(ContextMenu menu, List<Integer> expectedItems) { Preemptive comment! For a while I thought of putting in an assert within this method. Thinking that an assertEquals would give me some useful data if it didn't work out. However, I found out pretty quickly that since it's a list of ids as integers that it actually gives you no useful information as seeing [211343, 2300342, 32230] doesn't help much. As a result I left it out and just left it as return true or false.
https://codereview.chromium.org/2710833002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java (right): https://codereview.chromium.org/2710833002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java:208: boolean isGroupMessage = MailTo.isMailTo(params.getLinkUrl()) 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). https://codereview.chromium.org/2710833002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java:214: if (menu.getItem(i).isVisible()) { Can we pull out menu.getItem(i) to a local? https://codereview.chromium.org/2710833002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java:218: menuIdMap = getListOfOptions(params, context, menuIdMap); 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()? https://codereview.chromium.org/2710833002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java:222: boolean inMenuList = menuIdMap.containsKey(menu.getItem(i).getItemId()); Pull out menu.getItem(i) again imo https://codereview.chromium.org/2710833002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java:231: * Given a set of params and context, and a list of all the items, it'll return a list of items remove it'll? https://codereview.chromium.org/2710833002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java:234: * @param params The parameters used for filtering out the the list of items. Do we need @param context in here (even with no description)? https://codereview.chromium.org/2710833002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java:235: * @param allItems A list, with all the possible options for a link. Items will be removed based Can we describe what the format of the map is? https://codereview.chromium.org/2710833002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java:305: params.canSaveMedia() && UrlUtilities.isDownloadableScheme(params.getSrcUrl()); pull out isDownloadableScheme(params.getSrcUrl()) and use it here and below? https://codereview.chromium.org/2710833002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java:338: allItems.put(R.id.contextmenu_search_by_image, 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<>? https://codereview.chromium.org/2710833002/diff/1/chrome/android/javatests/sr... File chrome/android/javatests/src/org/chromium/chrome/browser/contextmenu/ContextMenuTest.java (right): https://codereview.chromium.org/2710833002/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/contextmenu/ContextMenuTest.java:418: private boolean confirmMenuList(ContextMenu menu, List<Integer> expectedItems) { On 2017/02/22 00:57:43, JJ wrote: > Preemptive comment! For a while I thought of putting in an assert within this > method. Thinking that an assertEquals would give me some useful data if it > didn't work out. However, I found out pretty quickly that since it's a list of > ids as integers that it actually gives you no useful information as seeing > [211343, 2300342, 32230] doesn't help much. As a result I left it out and just > left it as return true or false. I see your point. IMO the (debatable) useful piece of information you'd lose is whether or not line 426 or line 430 failed. To that end, if we only expect something like O(<20) menu items the sort algorithm will be really quick.
https://codereview.chromium.org/2710833002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java (right): https://codereview.chromium.org/2710833002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java:212: Map<Integer, String> menuIdMap = new HashMap<>(menu.size()); 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. https://codereview.chromium.org/2710833002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java:218: menuIdMap = getListOfOptions(params, context, menuIdMap); 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.
So I did some stuff based on your changes. I did ask for some follow up on SparseArrays, the test case, and caveats on how we change the context menu. Please take a look when you can. https://codereview.chromium.org/2710833002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java (right): https://codereview.chromium.org/2710833002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java:212: Map<Integer, String> menuIdMap = new HashMap<>(menu.size()); On 2017/02/22 06:10:54, Ted C wrote: > 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. 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. https://codereview.chromium.org/2710833002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java:214: if (menu.getItem(i).isVisible()) { On 2017/02/22 05:36:33, David Trainor wrote: > Can we pull out menu.getItem(i) to a local? Done. https://codereview.chromium.org/2710833002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java:218: menuIdMap = getListOfOptions(params, context, menuIdMap); On 2017/02/22 05:36:33, David Trainor wrote: > 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()? 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. https://codereview.chromium.org/2710833002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java:218: menuIdMap = getListOfOptions(params, context, menuIdMap); On 2017/02/22 06:10:54, Ted C wrote: > 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. 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. https://codereview.chromium.org/2710833002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java:222: boolean inMenuList = menuIdMap.containsKey(menu.getItem(i).getItemId()); On 2017/02/22 05:36:33, David Trainor wrote: > Pull out menu.getItem(i) again imo Done. https://codereview.chromium.org/2710833002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java:231: * Given a set of params and context, and a list of all the items, it'll return a list of items On 2017/02/22 05:36:33, David Trainor wrote: > remove it'll? Done. https://codereview.chromium.org/2710833002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java:234: * @param params The parameters used for filtering out the the list of items. On 2017/02/22 05:36:33, David Trainor wrote: > Do we need @param context in here (even with no description)? Question for the future: Is it alright to have a param that has no description? https://codereview.chromium.org/2710833002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java:235: * @param allItems A list, with all the possible options for a link. Items will be removed based On 2017/02/22 05:36:33, David Trainor wrote: > Can we describe what the format of the map is? Done. https://codereview.chromium.org/2710833002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java:338: allItems.put(R.id.contextmenu_search_by_image, On 2017/02/22 05:36:32, David Trainor wrote: > 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<>? 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. https://codereview.chromium.org/2710833002/diff/1/chrome/android/javatests/sr... File chrome/android/javatests/src/org/chromium/chrome/browser/contextmenu/ContextMenuTest.java (right): https://codereview.chromium.org/2710833002/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/contextmenu/ContextMenuTest.java:418: private boolean confirmMenuList(ContextMenu menu, List<Integer> expectedItems) { On 2017/02/22 05:36:33, David Trainor wrote: > On 2017/02/22 00:57:43, JJ wrote: > > Preemptive comment! For a while I thought of putting in an assert within this > > method. Thinking that an assertEquals would give me some useful data if it > > didn't work out. However, I found out pretty quickly that since it's a list of > > ids as integers that it actually gives you no useful information as seeing > > [211343, 2300342, 32230] doesn't help much. As a result I left it out and just > > left it as return true or false. > > I see your point. IMO the (debatable) useful piece of information you'd lose is > whether or not line 426 or line 430 failed. To that end, if we only expect > something like O(<20) menu items the sort algorithm will be really quick. Interesting! I didn't think of it with line numbers. I'm a little confused by this comment though, as I feel like it's advocating for both a change to add the assert back but also that since it's very quick to remove the if line all together? Unless that's exactly what you're thinking? Adding a single assertEquals at 430 and remove the if statement?
The CQ bit was checked by jwanda@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I forgot my bike so I have to wait for the bus so here I am! I forgot to address two comments, my bad. https://codereview.chromium.org/2710833002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java (right): https://codereview.chromium.org/2710833002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java:208: boolean isGroupMessage = MailTo.isMailTo(params.getLinkUrl()) On 2017/02/22 05:36:33, David Trainor wrote: > 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). Sorry, missed this. Pushed it inside of the url. https://codereview.chromium.org/2710833002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java:305: params.canSaveMedia() && UrlUtilities.isDownloadableScheme(params.getSrcUrl()); On 2017/02/22 05:36:33, David Trainor wrote: > pull out isDownloadableScheme(params.getSrcUrl()) and use it here and below? Done.
lgtm lgtm % tedchoc
https://codereview.chromium.org/2710833002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java (right): https://codereview.chromium.org/2710833002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java:209: || UrlUtilities.isTelScheme(params.getLinkUrl())); 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 https://codereview.chromium.org/2710833002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java:244: private void trimMenuOptions(ContextMenuParams params, Set<Integer> allItems) { 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. https://codereview.chromium.org/2710833002/diff/40001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/contextmenu/ContextMenuTest.java (right): https://codereview.chromium.org/2710833002/diff/40001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/contextmenu/ContextMenuTest.java:426: if (actualItems.size() != expectedItems.size()) return false; if the sizes are the same, couldn't all the numbers still be different? I wouldn't optimize this anyway as it makes the test harder to read. https://codereview.chromium.org/2710833002/diff/40001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/contextmenu/ContextMenuTest.java:430: return actualItems.equals(expectedItems); You might also be able to use this: https://developer.android.com/reference/android/test/MoreAsserts.html#assertC..., java.lang.Object...) Instead of passing the list, you pass a Integer... vararg and that should allow you to pass that to that MoreAsserts method w/o the sorting.
One of the comments took a little longer than expected but it should be all fixed. Please take a look as that fix adds a lot of extra detail that, while it does work, might get complicated. https://codereview.chromium.org/2710833002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java (right): https://codereview.chromium.org/2710833002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java:209: || UrlUtilities.isTelScheme(params.getLinkUrl())); On 2017/02/28 06:10:13, Ted C wrote: > 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 Yup! Sorry about that, it was yelling at me each time I tried to submit and this must've yelled at me. https://codereview.chromium.org/2710833002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java:244: private void trimMenuOptions(ContextMenuParams params, Set<Integer> allItems) { On 2017/02/28 06:10:13, Ted C wrote: > 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. 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. https://codereview.chromium.org/2710833002/diff/40001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/contextmenu/ContextMenuTest.java (right): https://codereview.chromium.org/2710833002/diff/40001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/contextmenu/ContextMenuTest.java:426: if (actualItems.size() != expectedItems.size()) return false; On 2017/02/28 06:10:13, Ted C wrote: > if the sizes are the same, couldn't all the numbers still be different? I > wouldn't optimize this anyway as it makes the test harder to read. Done. https://codereview.chromium.org/2710833002/diff/40001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/contextmenu/ContextMenuTest.java:430: return actualItems.equals(expectedItems); On 2017/02/28 06:10:13, Ted C wrote: > You might also be able to use this: > https://developer.android.com/reference/android/test/MoreAsserts.html#assertC..., > java.lang.Object...) > > Instead of passing the list, you pass a Integer... vararg and that should allow > you to pass that to that MoreAsserts method w/o the sorting. Done. https://codereview.chromium.org/2710833002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java (right): https://codereview.chromium.org/2710833002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java:56: private static final Integer[] BASE_WHITELIST = { cl format would not letup about changing this from a line to shrinking it as much as possible. I originally ignored it but then realized that the custom Tab actually does it so I let it through.
https://codereview.chromium.org/2710833002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java (right): https://codereview.chromium.org/2710833002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java:56: private static final Integer[] BASE_WHITELIST = { tangent...do we actually need these to be arrays anymore? I wonder if we should just do https://cs.chromium.org/chromium/src/base/android/java/src/org/chromium/base/... and convert these to sets. https://codereview.chromium.org/2710833002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java:219: trimInvisibleItems(supportedOptions, menu); Hmm...I'm a bit confused why we need this. It seems like the for loop below should hit all items and only set them to visible if they're in the enable list and not the disable list. Ohh...I think I get it now. The group visibility is most likely a subset of the whitelist set, so the whitelist is enabling things that aren't in the initial group I suspect. So longer term, we'll need group whitelist, a mode whitelist, and the disabled set. It would need to be a combo of the three. But if we were to add the group whitelist, we are really getting too close to not needing the context menu and that is bigger than a breadbox and hence your comment. https://codereview.chromium.org/2710833002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java:224: MenuItem item = menu.getItem(i); sorry for the stream of consciousness comments here, but could you just do: if (!item.isVisible()) continue; here and avoid the trimInvisibleItems altogether? I think that accomplishes the same. https://codereview.chromium.org/2710833002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java:246: if (!menu.findItem(itemId).isVisible()) { I think you should reverse the ordering of your conditional. findItem does an internal for loop to find the item anyway, so it should be more efficient to do (or something like this): for (int i = 0; i < menu.size(); i++) { MenuItem item = menu.getItem(i); if (!item.isVisible()) supportedItems.remove(item.getId()); } https://codereview.chromium.org/2710833002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java:366: private void removeUnsupportedItems(Set<Integer> itemList, int[] whitelist) { this doesn't seem to be needed anymore https://codereview.chromium.org/2710833002/diff/80001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/contextmenu/ContextMenuTest.java (right): https://codereview.chromium.org/2710833002/diff/80001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/contextmenu/ContextMenuTest.java:419: private void assertMenuItemsAreEqual(ContextMenu menu, List<Integer> expectedItems) { Does a vararg as the second param not work? It seems like it should get rid of the need for the list creation and the toArray below?
Fixed based off the comments. A running theme is that there are few methods that were meant to be deleted that were in a separate branch (whoops). Also take note of the @RetryOnFailure and the explanation. It's made me realized that having a test run overnight might be for the best next time. https://codereview.chromium.org/2710833002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java (right): https://codereview.chromium.org/2710833002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java:56: private static final Integer[] BASE_WHITELIST = { On 2017/03/01 06:30:49, Ted C wrote: > tangent...do we actually need these to be arrays anymore? > > I wonder if we should just do > https://cs.chromium.org/chromium/src/base/android/java/src/org/chromium/base/... > and convert these to sets. I see no reason not to! https://codereview.chromium.org/2710833002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java:219: trimInvisibleItems(supportedOptions, menu); On 2017/03/01 06:30:49, Ted C wrote: > Hmm...I'm a bit confused why we need this. It seems like the for loop below > should hit all items and only set them to visible if they're in the enable list > and not the disable list. > > Ohh...I think I get it now. The group visibility is most likely a subset of the > whitelist set, so the whitelist is enabling things that aren't in the initial > group I suspect. > > So longer term, we'll need group whitelist, a mode whitelist, and the disabled > set. It would need to be a combo of the three. But if we were to add the group > whitelist, we are really getting too close to not needing the context menu and > that is bigger than a breadbox and hence your comment. Exactly! As we get to the next cl which has much less of a dependency of the context menu. https://codereview.chromium.org/2710833002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java:224: MenuItem item = menu.getItem(i); On 2017/03/01 06:30:49, Ted C wrote: > sorry for the stream of consciousness comments here, but could you just do: > > if (!item.isVisible()) continue; > > here and avoid the trimInvisibleItems altogether? I think that accomplishes the > same. Huh, you're right. https://codereview.chromium.org/2710833002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java:246: if (!menu.findItem(itemId).isVisible()) { On 2017/03/01 06:30:49, Ted C wrote: > I think you should reverse the ordering of your conditional. findItem does an > internal for loop to find the item anyway, so it should be more efficient to do > (or something like this): > > for (int i = 0; i < menu.size(); i++) { > MenuItem item = menu.getItem(i); > if (!item.isVisible()) supportedItems.remove(item.getId()); > } Acknowledged. https://codereview.chromium.org/2710833002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java:366: private void removeUnsupportedItems(Set<Integer> itemList, int[] whitelist) { On 2017/03/01 06:30:49, Ted C wrote: > this doesn't seem to be needed anymore Whoops, I was certain I deleted it. Turns out it was on the wrong branch. https://codereview.chromium.org/2710833002/diff/80001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/contextmenu/ContextMenuTest.java (right): https://codereview.chromium.org/2710833002/diff/80001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/contextmenu/ContextMenuTest.java:419: private void assertMenuItemsAreEqual(ContextMenu menu, List<Integer> expectedItems) { On 2017/03/01 06:30:49, Ted C wrote: > Does a vararg as the second param not work? It seems like it should get rid of > the need for the list creation and the toArray below? Super sorry. Again, implemented in a different branch by accident. https://codereview.chromium.org/2710833002/diff/100001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/contextmenu/ContextMenuTest.java (right): https://codereview.chromium.org/2710833002/diff/100001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/contextmenu/ContextMenuTest.java:357: @RetryOnFailure By happenstance this test failed in my most recent run. It fails because of setUp() so I added RetryOnFailure. I promise you, I've run this test about 150 times and this is the first time an error happened but it seems like just enough of an error to add this. Especially considering all other tests have it as well.
lgtm w/ a couple small last things https://codereview.chromium.org/2710833002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java (right): https://codereview.chromium.org/2710833002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java:54: private static final HashSet<Integer> BASE_WHITELIST = CollectionUtil.newHashSet( In general, try to use the generic collections interface unless you need something to be a particular type (i.e. just Set<Integer> here) https://codereview.chromium.org/2710833002/diff/100001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/contextmenu/ContextMenuTest.java (right): https://codereview.chromium.org/2710833002/diff/100001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/contextmenu/ContextMenuTest.java:357: @RetryOnFailure On 2017/03/01 17:43:11, JJ wrote: > By happenstance this test failed in my most recent run. It fails because of > setUp() so I added RetryOnFailure. I promise you, I've run this test about 150 > times and this is the first time an error happened but it seems like just enough > of an error to add this. Especially considering all other tests have it as well. Looks like this whole class has them, so I guess there is some systemic thing there. Can you file a new bug with the exception you saw so we can track it? Or maybe look to see if one already exists?
The CQ bit was checked by jwanda@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Fixed all things. Pointed to the most recent bug. Everything should be in order! https://codereview.chromium.org/2710833002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java (right): https://codereview.chromium.org/2710833002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java:54: private static final HashSet<Integer> BASE_WHITELIST = CollectionUtil.newHashSet( On 2017/03/01 17:57:40, Ted C wrote: > In general, try to use the generic collections interface unless you need > something to be a particular type (i.e. just Set<Integer> here) Done. https://codereview.chromium.org/2710833002/diff/100001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/contextmenu/ContextMenuTest.java (right): https://codereview.chromium.org/2710833002/diff/100001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/contextmenu/ContextMenuTest.java:357: @RetryOnFailure On 2017/03/01 17:57:40, Ted C wrote: > On 2017/03/01 17:43:11, JJ wrote: > > By happenstance this test failed in my most recent run. It fails because of > > setUp() so I added RetryOnFailure. I promise you, I've run this test about 150 > > times and this is the first time an error happened but it seems like just > enough > > of an error to add this. Especially considering all other tests have it as > well. > > Looks like this whole class has them, so I guess there is some systemic thing > there. > > Can you file a new bug with the exception you saw so we can track it? Or maybe > look to see if one already exists? This seems to be the closest one: https://bugs.chromium.org/p/chromium/issues/detail?id=606939 sadly I didn't save the results of the test so I can't tell you any more details sadly.
The CQ bit was checked by jwanda@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by jwanda@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dtrainor@chromium.org, tedchoc@chromium.org Link to the patchset: https://codereview.chromium.org/2710833002/#ps120001 (title: "Fixed based off Ted's comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 120001, "attempt_start_ts": 1488407935691900,
"parent_rev": "d8a18c1f9600706b040641eae27e33ddc837f0f7", "commit_rev":
"4de4918b64f6fc0bdd4022d452bd693a785d6074"}
Message was sent while issue was closed.
Description was changed from ========== Refactor ChromeContextMenuPopulator population In preparation for the redesign of Chrome's Context Menu this cl refactors the population into a seperate method. As a result of this refactor, several tests are now added to confirm that the context menu shows what is expected in a context (which surprisingly wasn't there beforehand). BUG=655359 ========== to ========== Refactor ChromeContextMenuPopulator population In preparation for the redesign of Chrome's Context Menu this cl refactors the population into a seperate method. As a result of this refactor, several tests are now added to confirm that the context menu shows what is expected in a context (which surprisingly wasn't there beforehand). BUG=655359 Review-Url: https://codereview.chromium.org/2710833002 Cr-Commit-Position: refs/heads/master@{#454077} Committed: https://chromium.googlesource.com/chromium/src/+/4de4918b64f6fc0bdd4022d452bd... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/chromium/src/+/4de4918b64f6fc0bdd4022d452bd... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
