Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2747453002:
Split context menu display and population/handling (Closed)
DescriptionSplit context menu display and population/handling
This change had a main goal of deleteing chrome_context_menu.xml and
instead usings a combination of ids.xml and ContextMenuItems.java in
order to create a context menu. This is done mainly in preperation to
simply create the ContextMenu file. The tests were covered in a previous
cl (https://codereview.chromium.org/2710833002/).
BUG=613357
Review-Url: https://codereview.chromium.org/2747453002
Cr-Commit-Position: refs/heads/master@{#458248}
Committed: https://chromium.googlesource.com/chromium/src/+/fdd4f1fc2b5e991c9e0484e42bbee1f04102f9d3
Patch Set 1 #
Total comments: 37
Patch Set 2 : git rebase #
Total comments: 15
Patch Set 3 : Updated based of tedchoc comments #
Total comments: 14
Patch Set 4 : Fixed base off tedchoc's comments #
Total comments: 13
Patch Set 5 : I temproarily forgot how ContextMenu works #Patch Set 6 : Updated based off comment made by me (jwanda) Also Mondaaaaaaaaaays #Patch Set 7 : This is a very clear sign I need to go home now. #
Total comments: 5
Patch Set 8 : Let's uh...pretend Monday didn't happen #Patch Set 9 : Fixed based of Tedchoc's comments and a little bit of code smell #
Total comments: 4
Patch Set 10 : Fixed based off more comments #
Total comments: 6
Patch Set 11 : Fixed based off tedchoc's comments #
Total comments: 30
Patch Set 12 : Updated based off tedchoc@ and dtrainor@ comments #Patch Set 13 : Fixed based off dtrainor's comment #Patch Set 14 : Fixed based off tedchoc's comments #
Total comments: 1
Patch Set 15 : Minor change #Patch Set 16 : One space too many #Patch Set 17 : I'm genuinely confused on why some code did not upload #Messages
Total messages: 54 (19 generated)
jwanda@chromium.org changed reviewers: + dtrainor@chromium.org, tedchoc@chromium.org, twellington@chromium.org
Hello! This is the CL I request you all look at first! It is a massive refactor of how we want to handle ContextMenuUi. It's still a massive CL so I understand if it takes a while (473 insertions and 168 deletions is a little bit of pain) but hopefully clever titles and informative docs will let you understand everything. Can't wait to hear about ways to improve this! https://codereview.chromium.org/2747453002/diff/1/chrome/android/java/res/val... File chrome/android/java/res/values/ids.xml (right): https://codereview.chromium.org/2747453002/diff/1/chrome/android/java/res/val... chrome/android/java/res/values/ids.xml:73: <item type="id" name="contextmenu_open_in_new_chrome_tab" /> Any objections to me moving this from contextmenu.* -> context_menu.*? I didn't change it yet but over time this has been slightly bugging me. https://codereview.chromium.org/2747453002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java (right): https://codereview.chromium.org/2747453002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java:37: public class ChromeContextMenuPopulator implements ContextMenuPopulator { So I know Ted and I have had discussions about this file (please correct me if I'm wrong about any of the following). We were debating about moving from what it is now to a Model/View/Controller implementation. Or! We can change the name of this class (and all interface above) to signify that it does more than just population. Maybe ChromeContextMenuHandler? Regardless, I suggest to do this in a separate change after this. https://codereview.chromium.org/2747453002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuHelper.java (right): https://codereview.chromium.org/2747453002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuHelper.java:35: private Activity mActivity; 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.
The CQ bit was unchecked by jwanda@chromium.org
The CQ bit was checked by jwanda@chromium.org
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...
Minor text fixes. https://codereview.chromium.org/2747453002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuItems.java (right): https://codereview.chromium.org/2747453002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuItems.java:52: public int iconId; Forgot to remove this on my first pass. Apologies, already deleted and will be uploaded on next batch.
https://codereview.chromium.org/2747453002/diff/1/chrome/android/java/res/val... File chrome/android/java/res/values/ids.xml (right): https://codereview.chromium.org/2747453002/diff/1/chrome/android/java/res/val... chrome/android/java/res/values/ids.xml:73: <item type="id" name="contextmenu_open_in_new_chrome_tab" /> On 2017/03/10 18:01:16, JJ wrote: > Any objections to me moving this from contextmenu.* -> context_menu.*? I didn't > change it yet but over time this has been slightly bugging me. Nope...go for it https://codereview.chromium.org/2747453002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java (right): https://codereview.chromium.org/2747453002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java:37: public class ChromeContextMenuPopulator implements ContextMenuPopulator { On 2017/03/10 18:01:16, JJ wrote: > So I know Ted and I have had discussions about this file (please correct me if > I'm wrong about any of the following). We were debating about moving from what > it is now to a Model/View/Controller implementation. Or! We can change the name > of this class (and all interface above) to signify that it does more than just > population. Maybe ChromeContextMenuHandler? > Regardless, I suggest to do this in a separate change after this. separate change SGTM https://codereview.chromium.org/2747453002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuHelper.java (right): https://codereview.chromium.org/2747453002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuHelper.java:78: mActivity = windowAndroid.getActivity().get(); if the activity is null, we should return as well https://codereview.chromium.org/2747453002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuHelper.java:80: view.setOnCreateContextMenuListener(this); 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. https://codereview.chromium.org/2747453002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuHelper.java:139: new SimpleContextMenuUi(menu, new ContextMenuUi.OnMenuClickedListener() { 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 https://codereview.chromium.org/2747453002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuHelper.java:147: menuUi.displayMenu(mActivity, mCurrentContextMenuParams, items); 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. https://codereview.chromium.org/2747453002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuItems.java (right): https://codereview.chromium.org/2747453002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuItems.java:53: public int stringId; should these be final? https://codereview.chromium.org/2747453002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuItems.java:74: String value; You can skip this variable altogether. Just return in the if/else blocks directly, then you don't need the final else either and the last line can be return context.getString(stringId); https://codereview.chromium.org/2747453002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuPopulator.java (right): https://codereview.chromium.org/2747453002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuPopulator.java:28: * @return A list matched by 3 groups: Link, Image, and Video. Each "group" will contain items I wouldn't list out the groups here. We could add others in the future (or remove them). You can mention the items are grouped by their logical relationship. But stating the first in the pair is a resource ID is good. https://codereview.chromium.org/2747453002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuPopulator.java:29: * related to said group as well as a integer that is a string resource for the group. Image tiny nit, but align follow up lines to be aligned with the "A list ..." https://codereview.chromium.org/2747453002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/SimpleContextMenuUi.java (right): https://codereview.chromium.org/2747453002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/SimpleContextMenuUi.java:20: public class SimpleContextMenuUi implements ContextMenuUi { as mentioned offline, I think PlatformContextMenuUi or FrameworkContextMenuUi is better. https://codereview.chromium.org/2747453002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/SimpleContextMenuUi.java:34: if (params.isFile()) return; yeah, since this is in two places, moving it to the shared point definitely seems right to me. https://codereview.chromium.org/2747453002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/SimpleContextMenuUi.java:36: List<ContextMenuItems> items = new ArrayList<>(); I wouldn't create this intermediate list. Just have a nested for loop that adds things to the menu https://codereview.chromium.org/2747453002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/SimpleContextMenuUi.java:37: for (Pair<Integer, List<ContextMenuItems>> group : itemGroups) { same thing about using the for (int i = 0; i <...) for Lists instead of the foreach iterators https://codereview.chromium.org/2747453002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/SimpleContextMenuUi.java:44: for (int i = 0; i < mMenu.size(); i++) { can this just be part of the loop above? https://codereview.chromium.org/2747453002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/SimpleContextMenuUi.java:46: if (menuItem.isVisible()) { they're all going to be visible at this point right? https://codereview.chromium.org/2747453002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java (right): https://codereview.chromium.org/2747453002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java:61: LINK, these might all fit on a single line maybe? https://codereview.chromium.org/2747453002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java:102: public static final List<ContextMenuItems> CUSTOM_TAB_GROUP = Collections.unmodifiableList( I would put a comment above these that these determine the display order as we discussed offline (and to break them up between the sets above and these lists). Also, do these need to be public? https://codereview.chromium.org/2747453002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java:232: public static String createHeaderText(ContextMenuParams params) { non-private methods should have javadoc as a general rule https://codereview.chromium.org/2747453002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java:246: if (params.isFile()) return new ArrayList<>(); Should we just early return in showContextMenu then this method wouldn't be called. https://codereview.chromium.org/2747453002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java:270: R.string.contextmenu_link_title, filterMenuItems(menuOptions, LINK))); if filterMenuItems were to hide all of them, then I don't think we should add a group at all. I wonder if we could do this with a few fewer data structures and loops. First we have a method like: private void populateItemGroup( @ContextMenuGroup int contextMenuType, int titleResId, List<Pair<Integer, List<ContextMenuItems>>> itemGroups, Set<ContextMenuItems> supportedOptions, Set<ContextMenuItems> disabledOptions) { List<ContextMenuItems> items ...; switch (contextMenuType) { case LINK: addValidItems(items, LINK_GROUP, supportedOptions, disabledOptions); addValidItems(items, MESSAGE_GROUP, supportedOptions, disabledOptions); break; } if (items.isEmpty()) return; // Add item group now. } private void addValidItems( List<ContextMenuItems> validItems, List<ContextMenuItems> allItems, Set<ContextMenuItems> supportedOptions, Set<ContextMenuItems> disabledOptions) { for (int i = 0; i < allItems.size(); i++) { ContextMenuItem item = allItems.get(i); if (supportedOptions.contains(item) && !disabledOptions.contains(item)) { validItems.add(item); } } } I think that should work and cut down on a bunch of the intermediate objects and some of the iterations. https://codereview.chromium.org/2747453002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java:299: List<ContextMenuItems> whichGroup = new ArrayList<>(); why do you need whichGroup? https://codereview.chromium.org/2747453002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java:320: for (Iterator<ContextMenuItems> iterator = itemGroup.iterator(); iterator.hasNext();) { BTW...we try to avoid the Iterator loop types where possible (to avoid object creation). Here, I would have done: for (int i = itemGroup.size() - 1; i >= 0; i--)
Changes uploaded and ready to go! Tell me whatever else needs work! Or if I missed something. Comments were in two different patches so I'm a little worried something went over my head. https://codereview.chromium.org/2747453002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuHelper.java (right): https://codereview.chromium.org/2747453002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuHelper.java:78: mActivity = windowAndroid.getActivity().get(); On 2017/03/11 00:31:14, Ted C wrote: > if the activity is null, we should return as well Done. https://codereview.chromium.org/2747453002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuHelper.java:80: view.setOnCreateContextMenuListener(this); On 2017/03/11 00:31:14, Ted C wrote: > 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. 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. https://codereview.chromium.org/2747453002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuHelper.java:139: new SimpleContextMenuUi(menu, new ContextMenuUi.OnMenuClickedListener() { On 2017/03/11 00:31:14, Ted C wrote: > 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 Is that what the youngins are into these days? I need to get hip with these trends. https://codereview.chromium.org/2747453002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuHelper.java:147: menuUi.displayMenu(mActivity, mCurrentContextMenuParams, items); On 2017/03/11 00:31:14, Ted C wrote: > 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. Done. https://codereview.chromium.org/2747453002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuItems.java (right): https://codereview.chromium.org/2747453002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuItems.java:53: public int stringId; On 2017/03/11 00:31:14, Ted C wrote: > should these be final? Done. https://codereview.chromium.org/2747453002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuItems.java:74: String value; On 2017/03/11 00:31:14, Ted C wrote: > You can skip this variable altogether. Just return in the if/else blocks > directly, then you don't need the final else either and the last line can be > > return context.getString(stringId); Slight debate on it being if/else if block or two if statements. Put it as an if/else if since in my view it separates them as one or the other. But if you feel that it should be two if statements, no tantrum will be thrown. https://codereview.chromium.org/2747453002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuPopulator.java (right): https://codereview.chromium.org/2747453002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuPopulator.java:28: * @return A list matched by 3 groups: Link, Image, and Video. Each "group" will contain items On 2017/03/11 00:31:14, Ted C wrote: > I wouldn't list out the groups here. We could add others in the future (or > remove them). > > You can mention the items are grouped by their logical relationship. But > stating the first in the pair is a resource ID is good. Done. https://codereview.chromium.org/2747453002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuPopulator.java:29: * related to said group as well as a integer that is a string resource for the group. Image On 2017/03/11 00:31:14, Ted C wrote: > tiny nit, but align follow up lines to be aligned with the "A list ..." Done. https://codereview.chromium.org/2747453002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/SimpleContextMenuUi.java (right): https://codereview.chromium.org/2747453002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/SimpleContextMenuUi.java:20: public class SimpleContextMenuUi implements ContextMenuUi { On 2017/03/11 00:31:14, Ted C wrote: > as mentioned offline, I think PlatformContextMenuUi or FrameworkContextMenuUi is > better. Done. https://codereview.chromium.org/2747453002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/SimpleContextMenuUi.java:36: List<ContextMenuItems> items = new ArrayList<>(); On 2017/03/11 00:31:14, Ted C wrote: > I wouldn't create this intermediate list. Just have a nested for loop that adds > things to the menu nested loops it is! https://codereview.chromium.org/2747453002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/SimpleContextMenuUi.java:37: for (Pair<Integer, List<ContextMenuItems>> group : itemGroups) { On 2017/03/11 00:31:14, Ted C wrote: > same thing about using the for (int i = 0; i <...) for Lists instead of the > foreach iterators Done. https://codereview.chromium.org/2747453002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/SimpleContextMenuUi.java:44: for (int i = 0; i < mMenu.size(); i++) { On 2017/03/11 00:31:14, Ted C wrote: > can this just be part of the loop above? Done. https://codereview.chromium.org/2747453002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/SimpleContextMenuUi.java:46: if (menuItem.isVisible()) { On 2017/03/11 00:31:14, Ted C wrote: > they're all going to be visible at this point right? Yup https://codereview.chromium.org/2747453002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java (right): https://codereview.chromium.org/2747453002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java:61: LINK, On 2017/03/11 00:31:15, Ted C wrote: > these might all fit on a single line maybe? Done. https://codereview.chromium.org/2747453002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java:102: public static final List<ContextMenuItems> CUSTOM_TAB_GROUP = Collections.unmodifiableList( On 2017/03/11 00:31:14, Ted C wrote: > I would put a comment above these that these determine the display order as we > discussed offline (and to break them up between the sets above and these lists). > > Also, do these need to be public? Done. https://codereview.chromium.org/2747453002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java:232: public static String createHeaderText(ContextMenuParams params) { On 2017/03/11 00:31:15, Ted C wrote: > non-private methods should have javadoc as a general rule Done. https://codereview.chromium.org/2747453002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java:246: if (params.isFile()) return new ArrayList<>(); On 2017/03/11 00:31:15, Ted C wrote: > Should we just early return in showContextMenu then this method wouldn't be > called. 100% right. https://codereview.chromium.org/2747453002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java:270: R.string.contextmenu_link_title, filterMenuItems(menuOptions, LINK))); On 2017/03/11 00:31:15, Ted C wrote: > if filterMenuItems were to hide all of them, then I don't think we should add a > group at all. > > I wonder if we could do this with a few fewer data structures and loops. > > First we have a method like: > > private void populateItemGroup( > @ContextMenuGroup int contextMenuType, int titleResId, > List<Pair<Integer, List<ContextMenuItems>>> itemGroups, > Set<ContextMenuItems> supportedOptions, > Set<ContextMenuItems> disabledOptions) { > List<ContextMenuItems> items ...; > switch (contextMenuType) { > case LINK: > addValidItems(items, LINK_GROUP, supportedOptions, disabledOptions); > addValidItems(items, MESSAGE_GROUP, supportedOptions, > disabledOptions); > break; > } > if (items.isEmpty()) return; > > // Add item group now. > } > > private void addValidItems( > List<ContextMenuItems> validItems, List<ContextMenuItems> allItems, > Set<ContextMenuItems> supportedOptions, Set<ContextMenuItems> > disabledOptions) { > for (int i = 0; i < allItems.size(); i++) { > ContextMenuItem item = allItems.get(i); > if (supportedOptions.contains(item) && !disabledOptions.contains(item)) > { > validItems.add(item); > } > } > } > > I think that should work and cut down on a bunch of the intermediate objects and > some of the iterations. Oh wow that was clever. Thanks! I added a special case for the Custom Tab Group and Other Group as well. https://codereview.chromium.org/2747453002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java:299: List<ContextMenuItems> whichGroup = new ArrayList<>(); On 2017/03/11 00:31:15, Ted C wrote: > why do you need whichGroup? Oh whoops, that was completely unintentional. From a very confusing bug. https://codereview.chromium.org/2747453002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java:320: for (Iterator<ContextMenuItems> iterator = itemGroup.iterator(); iterator.hasNext();) { On 2017/03/11 00:31:15, Ted C wrote: > BTW...we try to avoid the Iterator loop types where possible (to avoid object > creation). > > Here, I would have done: > for (int i = itemGroup.size() - 1; i >= 0; i--) Done.
https://codereview.chromium.org/2747453002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuHelper.java (right): https://codereview.chromium.org/2747453002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuHelper.java:80: view.setOnCreateContextMenuListener(this); On 2017/03/13 20:24:28, JJ wrote: > On 2017/03/11 00:31:14, Ted C wrote: > > 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. > > 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. Seems ok to do in a follow up. https://codereview.chromium.org/2747453002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuHelper.java:151: public boolean onMenuItemClick(MenuItem item) { I think you can delete this now https://codereview.chromium.org/2747453002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java (right): https://codereview.chromium.org/2747453002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java:280: if (!groupedItems.isEmpty()) { Looking at the old code, I don't see where the isEmpty check is coming from. In the previous code, would we have shown these options even if all other options were disabled? I guess we wouldn't have a valid tab title in this case, but seems like we could use the first of the values from above. if (groupedItems.isEmpty()) { int titleResId = R.string.contextmenu_video_title; if (params.isAnchor()) titleResId = R.string.contextmenu_link_title; else if (params.isImage()) titleResId = R.string.contextmenu_image_title; groupedItems.add(new Pair<>(titleResId, new ArrayList<ContextMenuItems>())); } But maybe I'm looking over something from the old code that wouldn't have shown this in which case you can skip this. Also, we might also want to just duplicate some of the logic of populateItemGroup to make it less special casing. like for OTHER, this could be: addValidItems(groupedItems.get(0).second, OTHER_GROUP, supportedOptions, disabledOptions); And maybe for CUSTOM_TAB_MODE, we could create a new list and insert it to the top using addAll like you do below. I like the idea that populateItemGroup only has GROUP definitions for the actual UI tabs we'd show in the new context menu design. https://codereview.chromium.org/2747453002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java:315: if (items.isEmpty()) { I would put this all on one line (which is allowed in java if the statement and condition fit on one line) https://codereview.chromium.org/2747453002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java:325: itemGroups.get(0).second.addAll(0, items); would you need to return here to avoid adding to a new group below? https://codereview.chromium.org/2747453002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuHelper.java (right): https://codereview.chromium.org/2747453002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuHelper.java:137: if (mCurrentContextMenuParams.isFile()) { I think this should go in showContextMenu above (also same thing about being able to fit on one line) https://codereview.chromium.org/2747453002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuItems.java (right): https://codereview.chromium.org/2747453002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuItems.java:15: * A in class representation of chrome_context_menu.xml. We should update this now that the xml file is going away (List of all context menu items available in Chrome). https://codereview.chromium.org/2747453002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuItems.java:17: public enum ContextMenuItems { nit, should be ContextMenuItem as they each represent just one https://codereview.chromium.org/2747453002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/PlatformContextMenuUi.java (right): https://codereview.chromium.org/2747453002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/PlatformContextMenuUi.java:39: menuItem.setOnMenuItemClickListener(new MenuItem.OnMenuItemClickListener() { I would create just one MenuItem.OnMenuItemClickListener outside the loop and share it with all of them
Fixed errors, some made me realize that submitting CLs before noon is a bad idea. https://codereview.chromium.org/2747453002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuHelper.java (right): https://codereview.chromium.org/2747453002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuHelper.java:80: view.setOnCreateContextMenuListener(this); On 2017/03/13 21:55:19, Ted C wrote: > On 2017/03/13 20:24:28, JJ wrote: > > On 2017/03/11 00:31:14, Ted C wrote: > > > 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. > > > > 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. > > Seems ok to do in a follow up. Thanks thanks. https://codereview.chromium.org/2747453002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuHelper.java:151: public boolean onMenuItemClick(MenuItem item) { On 2017/03/13 21:55:19, Ted C wrote: > I think you can delete this now Done. https://codereview.chromium.org/2747453002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/SimpleContextMenuUi.java (right): https://codereview.chromium.org/2747453002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/SimpleContextMenuUi.java:34: if (params.isFile()) return; On 2017/03/11 00:31:14, Ted C wrote: > yeah, since this is in two places, moving it to the shared point definitely > seems right to me. Done. https://codereview.chromium.org/2747453002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java (right): https://codereview.chromium.org/2747453002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java:280: if (!groupedItems.isEmpty()) { On 2017/03/13 21:55:19, Ted C wrote: > Looking at the old code, I don't see where the isEmpty check is coming from. In > the previous code, would we have shown these options even if all other options > were disabled? > > I guess we wouldn't have a valid tab title in this case, but seems like we could > use the first of the values from above. > > if (groupedItems.isEmpty()) { > int titleResId = R.string.contextmenu_video_title; > if (params.isAnchor()) titleResId = R.string.contextmenu_link_title; > else if (params.isImage()) titleResId = R.string.contextmenu_image_title; > groupedItems.add(new Pair<>(titleResId, new ArrayList<ContextMenuItems>())); > } > > But maybe I'm looking over something from the old code that wouldn't have shown > this in which case you can skip this. > > Also, we might also want to just duplicate some of the logic of > populateItemGroup to make it less special casing. > > like for OTHER, this could be: > addValidItems(groupedItems.get(0).second, OTHER_GROUP, supportedOptions, > disabledOptions); > > And maybe for CUSTOM_TAB_MODE, we could create a new list and insert it to the > top using addAll like you do below. > > I like the idea that populateItemGroup only has GROUP definitions for the actual > UI tabs we'd show in the new context menu design. > You're right, we would have. When I rewrote this code I was thinking more of "When is it ever possible to get a moment where no other items are shown except for just custom tab. My conclusion was "literally never" but! You're likely right that this could happen. Switched it so this will change. To the rest of your comment. The logic is now duplicated. I tried for a while to think of a clever way to use this code for both populate the regular item group and the other but realized that it couldn't be done thanks to the itemGroups line at the bottom. Which could be pushed to a separate method but at that point then we'd start adding lines more than deleting them and quite possibly making it more confusing instead of less. https://codereview.chromium.org/2747453002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java:315: if (items.isEmpty()) { On 2017/03/13 21:55:19, Ted C wrote: > I would put this all on one line (which is allowed in java if the statement and > condition fit on one line) I tried! git cl upload yelled at me for trying. git cl format pushed it back to what it was. Though it turned out because I added brackets instead of removing brackets for a one line. https://codereview.chromium.org/2747453002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java:325: itemGroups.get(0).second.addAll(0, items); On 2017/03/13 21:55:19, Ted C wrote: > would you need to return here to avoid adding to a new group below? Yup! Sorry thought I fixed this before it was sent. https://codereview.chromium.org/2747453002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuHelper.java (right): https://codereview.chromium.org/2747453002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuHelper.java:137: if (mCurrentContextMenuParams.isFile()) { On 2017/03/13 21:55:19, Ted C wrote: > I think this should go in showContextMenu above (also same thing about being > able to fit on one line) Thought a little harder about whether or not I want it to be on the same line as literally all the other possible return values. Decided against it since it felt like the other if statement was more of "If Chrome Exists in the way we want it to" more than "If the Context Menu is alright". Feel free to correct me though. https://codereview.chromium.org/2747453002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuItems.java (right): https://codereview.chromium.org/2747453002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuItems.java:15: * A in class representation of chrome_context_menu.xml. On 2017/03/13 21:55:19, Ted C wrote: > We should update this now that the xml file is going away (List of all context > menu items available in Chrome). Done. https://codereview.chromium.org/2747453002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuItems.java:17: public enum ContextMenuItems { On 2017/03/13 21:55:19, Ted C wrote: > nit, should be ContextMenuItem as they each represent just one Noted. https://codereview.chromium.org/2747453002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/PlatformContextMenuUi.java (right): https://codereview.chromium.org/2747453002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/PlatformContextMenuUi.java:39: menuItem.setOnMenuItemClickListener(new MenuItem.OnMenuItemClickListener() { On 2017/03/13 21:55:19, Ted C wrote: > I would create just one MenuItem.OnMenuItemClickListener outside the loop and > share it with all of them Done!
https://codereview.chromium.org/2747453002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java (right): https://codereview.chromium.org/2747453002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java:278: // Special cases that happen when we're in custom tab mode and need to add the items This comment might need to be updated. https://codereview.chromium.org/2747453002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java:280: if (groupedItems.isEmpty()) { Sigh...my previous comment might have been overzealous as before. Now, we could result in adding a group with no menu options. We could not create the group but do something like: List<ContextMenuItem> firstGroupItems = groupedItems.isEmpty() ? new ArrayList<>() : groupedItems.get(0).second; ... add stuff and things ... if (!firstGroupItems.isEmpty() && groupedItems.isEmpty()) { ... the stuff you're doing in this if ... } Or you can just delete the group at the end if groupedItems.get(0).second.isEmpty(); https://codereview.chromium.org/2747453002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java:317: case OTHER: Do we need OTHER or CUSTOM anymore? https://codereview.chromium.org/2747453002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java:343: itemGroups.get(0).second.addAll(0, validItems); Should the OTHER go to the end of the list while CUSTOM go to the top? I thought your previous comment was that one went to the front and one went to the back. If OTHER does go to the end, I think you should use addValidItems for it directly. But I think addValidItemsToFront would be a better name for this, and we should pass the same set of params as above (you don't need the groups here since you're only modifying the first). https://codereview.chromium.org/2747453002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/PlatformContextMenuUi.java (right): https://codereview.chromium.org/2747453002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/PlatformContextMenuUi.java:36: MenuItem.OnMenuItemClickListener menuListener = new MenuItem.OnMenuItemClickListener() { This should be able to go fully outside any of the loops
Mondays are not a good day to submit code properly. Apologies to all. https://codereview.chromium.org/2747453002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java (right): https://codereview.chromium.org/2747453002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java:278: // Special cases that happen when we're in custom tab mode and need to add the items On 2017/03/14 00:02:53, Ted C wrote: > This comment might need to be updated. Done. https://codereview.chromium.org/2747453002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java:280: if (groupedItems.isEmpty()) { On 2017/03/14 00:02:53, Ted C wrote: > Sigh...my previous comment might have been overzealous as before. Now, we could > result in adding a group with no menu options. > > We could not create the group but do something like: > > List<ContextMenuItem> firstGroupItems = groupedItems.isEmpty() > ? new ArrayList<>() : groupedItems.get(0).second; > ... add stuff and things ... > if (!firstGroupItems.isEmpty() && groupedItems.isEmpty()) { > ... the stuff you're doing in this if ... > } > > Or you can just delete the group at the end if > groupedItems.get(0).second.isEmpty(); Done. https://codereview.chromium.org/2747453002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java:317: case OTHER: On 2017/03/14 00:02:53, Ted C wrote: > Do we need OTHER or CUSTOM anymore? Yup! git cl upload literally does not let you upload without it. https://codereview.chromium.org/2747453002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java:343: itemGroups.get(0).second.addAll(0, validItems); On 2017/03/14 00:02:53, Ted C wrote: > Should the OTHER go to the end of the list while CUSTOM go to the top? I > thought your previous comment was that one went to the front and one went to the > back. > > If OTHER does go to the end, I think you should use addValidItems for it > directly. But I think addValidItemsToFront would be a better name for this, and > we should pass the same set of params as above (you don't need the groups here > since you're only modifying the first). Fixed. Though it rendered "handleSpecialCases" useless since they both have to add the items in different way. Adding a method for the first two lines would equal the same amount of lines without really increasing a lot of readability. https://codereview.chromium.org/2747453002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/PlatformContextMenuUi.java (right): https://codereview.chromium.org/2747453002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/PlatformContextMenuUi.java:36: MenuItem.OnMenuItemClickListener menuListener = new MenuItem.OnMenuItemClickListener() { On 2017/03/14 00:02:53, Ted C wrote: > This should be able to go fully outside any of the loops Done.
Mondays are not a good day to submit code properly. Apologies to all. https://codereview.chromium.org/2747453002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java (right): https://codereview.chromium.org/2747453002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java:278: // Special cases that happen when we're in custom tab mode and need to add the items On 2017/03/14 00:02:53, Ted C wrote: > This comment might need to be updated. Done. https://codereview.chromium.org/2747453002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java:280: if (groupedItems.isEmpty()) { On 2017/03/14 00:02:53, Ted C wrote: > Sigh...my previous comment might have been overzealous as before. Now, we could > result in adding a group with no menu options. > > We could not create the group but do something like: > > List<ContextMenuItem> firstGroupItems = groupedItems.isEmpty() > ? new ArrayList<>() : groupedItems.get(0).second; > ... add stuff and things ... > if (!firstGroupItems.isEmpty() && groupedItems.isEmpty()) { > ... the stuff you're doing in this if ... > } > > Or you can just delete the group at the end if > groupedItems.get(0).second.isEmpty(); Done. https://codereview.chromium.org/2747453002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java:317: case OTHER: On 2017/03/14 00:02:53, Ted C wrote: > Do we need OTHER or CUSTOM anymore? Yup! git cl upload literally does not let you upload without it. https://codereview.chromium.org/2747453002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java:343: itemGroups.get(0).second.addAll(0, validItems); On 2017/03/14 00:02:53, Ted C wrote: > Should the OTHER go to the end of the list while CUSTOM go to the top? I > thought your previous comment was that one went to the front and one went to the > back. > > If OTHER does go to the end, I think you should use addValidItems for it > directly. But I think addValidItemsToFront would be a better name for this, and > we should pass the same set of params as above (you don't need the groups here > since you're only modifying the first). Fixed. Though it rendered "handleSpecialCases" useless since they both have to add the items in different way. Adding a method for the first two lines would equal the same amount of lines without really increasing a lot of readability. https://codereview.chromium.org/2747453002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/PlatformContextMenuUi.java (right): https://codereview.chromium.org/2747453002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/PlatformContextMenuUi.java:36: MenuItem.OnMenuItemClickListener menuListener = new MenuItem.OnMenuItemClickListener() { On 2017/03/14 00:02:53, Ted C wrote: > This should be able to go fully outside any of the loops Done.
Quick mistake fix. https://codereview.chromium.org/2747453002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java (right): https://codereview.chromium.org/2747453002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java:280: if (groupedItems.isEmpty()) { On 2017/03/14 00:31:09, JJ wrote: > On 2017/03/14 00:02:53, Ted C wrote: > > Sigh...my previous comment might have been overzealous as before. Now, we > could > > result in adding a group with no menu options. > > > > We could not create the group but do something like: > > > > List<ContextMenuItem> firstGroupItems = groupedItems.isEmpty() > > ? new ArrayList<>() : groupedItems.get(0).second; > > ... add stuff and things ... > > if (!firstGroupItems.isEmpty() && groupedItems.isEmpty()) { > > ... the stuff you're doing in this if ... > > } > > > > Or you can just delete the group at the end if > > groupedItems.get(0).second.isEmpty(); > > Done. UnDone. As the code above amounts to if (false) .... Since if grouped Items is empty itll produce a new list that is empty. Thus evaluating to false If it contains an item then it'll also evaluate to false since groupedItems is not empty. Change incoming.
Fixed the small issue on the if statement. Add clarifications. https://codereview.chromium.org/2747453002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java (right): https://codereview.chromium.org/2747453002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java:280: if (groupedItems.isEmpty()) { On 2017/03/14 00:44:20, JJ wrote: > On 2017/03/14 00:31:09, JJ wrote: > > On 2017/03/14 00:02:53, Ted C wrote: > > > Sigh...my previous comment might have been overzealous as before. Now, we > > could > > > result in adding a group with no menu options. > > > > > > We could not create the group but do something like: > > > > > > List<ContextMenuItem> firstGroupItems = groupedItems.isEmpty() > > > ? new ArrayList<>() : groupedItems.get(0).second; > > > ... add stuff and things ... > > > if (!firstGroupItems.isEmpty() && groupedItems.isEmpty()) { > > > ... the stuff you're doing in this if ... > > > } > > > > > > Or you can just delete the group at the end if > > > groupedItems.get(0).second.isEmpty(); > > > > Done. > > UnDone. As the code above amounts to > if (false) .... > > Since if grouped Items is empty itll produce a new list that is empty. Thus > evaluating to false > If it contains an item then it'll also evaluate to false since groupedItems is > not empty. Change incoming. Change in!
https://codereview.chromium.org/2747453002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java (right): https://codereview.chromium.org/2747453002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java:317: case OTHER: On 2017/03/14 00:31:09, JJ wrote: > On 2017/03/14 00:02:53, Ted C wrote: > > Do we need OTHER or CUSTOM anymore? > > Yup! git cl upload literally does not let you upload without it. I meant can we remove OTHER and CUSTOM altogether. They don't represent UIs, so I think we should just remove the constants entirely. https://codereview.chromium.org/2747453002/diff/80002/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java (right): https://codereview.chromium.org/2747453002/diff/80002/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java:292: titleResId, new ArrayList<ContextMenuItem>())); you need to use firstGroup here I think https://codereview.chromium.org/2747453002/diff/80002/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java:298: groupedItems.get(groupedItems.size() - 1).second.addAll(validItems); sigh...this logic is sadly confusing at this point. This keeps the ordering and logic the same as before so that is good. But in the tabular UI, I think we need to rethink this. I think both of these sets of items should be in the LINK group. Granted, I don't know if we should create the LINK group if it isn't there. If there is just one group, I think we should add them to those regardless of them being LINKs. But yeah, this logic keeps it consistent but kinda weird.
Updated based off Ted's comments. Some luckily were in a patch that was uploaded earlier. The rest are in an upcoming patch. Also added did create a special case that limits the code smell cause. https://codereview.chromium.org/2747453002/diff/80002/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java (right): https://codereview.chromium.org/2747453002/diff/80002/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java:292: titleResId, new ArrayList<ContextMenuItem>())); On 2017/03/14 05:23:58, Ted C wrote: > you need to use firstGroup here I think Sorry check the most recent version it has the change here. https://codereview.chromium.org/2747453002/diff/80002/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java:298: groupedItems.get(groupedItems.size() - 1).second.addAll(validItems); On 2017/03/14 05:23:58, Ted C wrote: > sigh...this logic is sadly confusing at this point. This keeps the ordering and > logic the same as before so that is good. But in the tabular UI, I think we > need to rethink this. > > I think both of these sets of items should be in the LINK group. Granted, I > don't know if we should create the LINK group if it isn't there. If there is > just one group, I think we should add them to those regardless of them being > LINKs. > > But yeah, this logic keeps it consistent but kinda weird. So I agree with the first change and the most recent patch kind of supports it. (Well it more said "Link is default" instead of "Video is default"). I thought about the logic for a while (this bugs me a lot more, it has code smell and feels fragile for handling a special case). I do want to point out though that creating a group with the title "link" in the custom context menu means that it would be dual page for every item in a custom tab. Which might confuse the user as now there is no way to discern images from image-links. For the logic point I thought long and hard about it and I'm debating all 3 options. It led me down to 3 conclusions 1. Keep it as is - It preserves the logic and gets to exactly what we want it to do. However it leads to code smell. 2. Leave the "Custom" or "Other" groups and let the UI handle the ordering - This has the benefit of keeping the code here clean and neat. However it could confuse the greater intention of keeping groups as visible groups that the user could see. 3. Forget ordering, add the rest to the bottom - It gets rid of code duplication, lets us handle the special case in a consistent manner. However this might annoy developers who use custom tab. I tried to think higher up than this (i.e. do we even need groups?) but it all led back to the same baseline of "we need groups' just how to split and populate them. This might need a discussion offline? It's starting to get a little unwieldy.
https://codereview.chromium.org/2747453002/diff/80002/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java (right): https://codereview.chromium.org/2747453002/diff/80002/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java:298: groupedItems.get(groupedItems.size() - 1).second.addAll(validItems); On 2017/03/14 17:39:17, JJ wrote: > On 2017/03/14 05:23:58, Ted C wrote: > > sigh...this logic is sadly confusing at this point. This keeps the ordering > and > > logic the same as before so that is good. But in the tabular UI, I think we > > need to rethink this. > > > > I think both of these sets of items should be in the LINK group. Granted, I > > don't know if we should create the LINK group if it isn't there. If there is > > just one group, I think we should add them to those regardless of them being > > LINKs. > > > > But yeah, this logic keeps it consistent but kinda weird. > > So I agree with the first change and the most recent patch kind of supports it. > (Well it more said "Link is default" instead of "Video is default"). > > I thought about the logic for a while (this bugs me a lot more, it has code > smell and feels fragile for handling a special case). > > I do want to point out though that creating a group with the title "link" in the > custom context menu means that it would be dual page for every item in a custom > tab. Which might confuse the user as now there is no way to discern images from > image-links. > > For the logic point I thought long and hard about it and I'm debating all 3 > options. > > It led me down to 3 conclusions > 1. Keep it as is - It preserves the logic and gets to exactly what we want it to > do. However it leads to code smell. > 2. Leave the "Custom" or "Other" groups and let the UI handle the ordering - > This has the benefit of keeping the code here clean and neat. However it could > confuse the greater intention of keeping groups as visible groups that the user > could see. > 3. Forget ordering, add the rest to the bottom - It gets rid of code > duplication, lets us handle the special case in a consistent manner. However > this might annoy developers who use custom tab. > > I tried to think higher up than this (i.e. do we even need groups?) but it all > led back to the same baseline of "we need groups' just how to split and populate > them. > > This might need a discussion offline? It's starting to get a little unwieldy. > I think we should keep the logic as is. My comment was more that we should rethink it in the future with the new UI. I think these items make sense to go in the LINK category if it exists. If it doesn't exist, I think it should go into whatever group is there (to prevent creating a new tab). If no group exists, we would create one. But that logic would change the existing behavior, so I think we shouldn't do that now at all. https://codereview.chromium.org/2747453002/diff/150001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java (right): https://codereview.chromium.org/2747453002/diff/150001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java:289: addValidItemsToAList(OTHER_GROUP, supportedOptions, disabledOptions, to keep the ui exactly the same as before, shouldn't this use addValidItems? This would add it to the front of that group and this always appeared at the end. https://codereview.chromium.org/2747453002/diff/150001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java:305: private void addValidItemsToAList(List<ContextMenuItem> allItems, I would still recommend calling this addValidItemsToFront and having the input params the same (in ordering and name) as addValidItems. The only difference I see is that it adds to the front at this point. Also, I'd put it right next to the other addValidItems method.
More reviews! https://codereview.chromium.org/2747453002/diff/150001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java (right): https://codereview.chromium.org/2747453002/diff/150001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java:289: addValidItemsToAList(OTHER_GROUP, supportedOptions, disabledOptions, On 2017/03/14 18:10:58, Ted C wrote: > to keep the ui exactly the same as before, shouldn't this use addValidItems? > > This would add it to the front of that group and this always appeared at the > end. Done. https://codereview.chromium.org/2747453002/diff/150001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java:305: private void addValidItemsToAList(List<ContextMenuItem> allItems, On 2017/03/14 18:10:58, Ted C wrote: > I would still recommend calling this addValidItemsToFront and having the input > params the same (in ordering and name) as addValidItems. > > The only difference I see is that it adds to the front at this point. > > Also, I'd put it right next to the other addValidItems method. Done.
https://codereview.chromium.org/2747453002/diff/170001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java (right): https://codereview.chromium.org/2747453002/diff/170001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java:289: List<ContextMenuItem> validItems = new ArrayList<>(); Why do you need this list? If there are no valid items to be added, then the list doesn't change if you passed it in. And if there are valid items, they would be added correctly. I think you should just be calling: addValidItems(groupedItems.get(groupedItems.size() - 1).second, OTHER_GROUP, supportedOptions, disabledOptions); https://codereview.chromium.org/2747453002/diff/170001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java:296: validItems = new ArrayList<>(); same comment here where this list shouldn't be needed. You should be able to just pass groupedItems.get(0).second here and it does the right thing internally. https://codereview.chromium.org/2747453002/diff/170001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java:349: validItems.add(item); I assume this needs to be add(0, item); right?
Fixed stuff! https://codereview.chromium.org/2747453002/diff/170001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java (right): https://codereview.chromium.org/2747453002/diff/170001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java:289: List<ContextMenuItem> validItems = new ArrayList<>(); On 2017/03/14 22:15:00, Ted C wrote: > Why do you need this list? If there are no valid items to be added, then the > list doesn't change if you passed it in. And if there are valid items, they > would be added correctly. > > I think you should just be calling: > > addValidItems(groupedItems.get(groupedItems.size() - 1).second, > OTHER_GROUP, supportedOptions, disabledOptions); Done. https://codereview.chromium.org/2747453002/diff/170001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java:296: validItems = new ArrayList<>(); On 2017/03/14 22:15:00, Ted C wrote: > same comment here where this list shouldn't be needed. You should be able to > just pass groupedItems.get(0).second here and it does the right thing > internally. Done. https://codereview.chromium.org/2747453002/diff/170001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java:349: validItems.add(item); On 2017/03/14 22:15:00, Ted C wrote: > I assume this needs to be add(0, item); right? The problem with working on two branches is that you think you did a change on one of them and you did it on another. Thanks!
lgtm
more nits. Take or leave the collections api suggestions. They feel a bit nicer to me but it's fine either way. https://codereview.chromium.org/2747453002/diff/190001/chrome/android/java/re... File chrome/android/java/res/values/ids.xml (right): https://codereview.chromium.org/2747453002/diff/190001/chrome/android/java/re... chrome/android/java/res/values/ids.xml:76: <!-- Link Group --> New line before each comment group? https://codereview.chromium.org/2747453002/diff/190001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java (right): https://codereview.chromium.org/2747453002/diff/190001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java:256: for (ContextMenuItem item : supportedOptions) { Maybe supportedOptions.removeAll(getDisabledOptions(params))? https://codereview.chromium.org/2747453002/diff/190001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java:277: if (groupedItems.isEmpty()) { This last chunk of logic is a bit confusing. Can you comment a bit about this in the code? I think we're doing: 1. Adding an empty group if we have no groups already added. 2. Adding the "Open in Chrome" menu item as the last element in whatever the last group is (assuming it's a valid item). 3. Adding some custom tabs items at the front of the first group if possible. 4. Removing the first group if it's empty. https://codereview.chromium.org/2747453002/diff/190001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java:297: if (groupedItems.get(0).second.isEmpty()) { If it's empty, do we have any groups period? Is that okay? https://codereview.chromium.org/2747453002/diff/190001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java:328: for (int i = 0; i < allItems.size(); i++) { I'm fine with this, but you could probably do something like the following I think? List<ContextMenuItem> items(allItems); items.retainAll(supportedOptions); items.removeAll(disabledOptions); validItems.add(items); https://codereview.chromium.org/2747453002/diff/190001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java:342: validItems.add(0, item); What if validItems already contains item? Also could just do something like the above (or turn the above into a helper and use it here): List<ContextMenuItem> items(allItems); items.retainAll(supportedOptions); items.removeAll(disabledOptions); validItems.addAll(0, items); https://codereview.chromium.org/2747453002/diff/190001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuItem.java (right): https://codereview.chromium.org/2747453002/diff/190001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuItem.java:23: OPEN_IN_BROWSER_ID(0, R.id.context_menu_open_in_browser_id), is @StringRes ok with 0? I guess we don't crash with the annotations, just checking. https://codereview.chromium.org/2747453002/diff/190001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuItem.java:24: // Link Group New line before each comment? https://codereview.chromium.org/2747453002/diff/190001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuItem.java:52: public final int stringId; @StringRes here too? That is a cool annotation :). https://codereview.chromium.org/2747453002/diff/190001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuItem.java:53: public final int menuId; @IdRes or @MenuRes? :) https://codereview.chromium.org/2747453002/diff/190001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuItem.java:61: ContextMenuItem(@StringRes int stringId, int menuId) { Same here https://codereview.chromium.org/2747453002/diff/190001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/PlatformContextMenuUi.java (right): https://codereview.chromium.org/2747453002/diff/190001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/PlatformContextMenuUi.java:32: setHeaderText(activity, mMenu, ChromeContextMenuPopulator.createHeaderText(params)); This feels a bit roundabout (we're pushing from a populator but then we're pulling from it here). I don't feel strongly either way though.
https://codereview.chromium.org/2747453002/diff/190001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java (right): https://codereview.chromium.org/2747453002/diff/190001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java:328: for (int i = 0; i < allItems.size(); i++) { On 2017/03/15 06:06:40, David Trainor-ping if over 24h wrote: > I'm fine with this, but you could probably do something like the following I > think? > > List<ContextMenuItem> items(allItems); > items.retainAll(supportedOptions); > items.removeAll(disabledOptions); > > validItems.add(items); This seems fine, but you're creating another list object and iterating over it twice instead of once. It is slightly more readable, but not enough that I 100% agree with it. But I also don't feel strongly enough to say no either. I'll defer to the two of you here.
On 2017/03/15 15:47:13, Ted C wrote: > https://codereview.chromium.org/2747453002/diff/190001/chrome/android/java/sr... > File > chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java > (right): > > https://codereview.chromium.org/2747453002/diff/190001/chrome/android/java/sr... > chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java:328: > for (int i = 0; i < allItems.size(); i++) { > On 2017/03/15 06:06:40, David Trainor-ping if over 24h wrote: > > I'm fine with this, but you could probably do something like the following I > > think? > > > > List<ContextMenuItem> items(allItems); > > items.retainAll(supportedOptions); > > items.removeAll(disabledOptions); > > > > validItems.add(items); > > This seems fine, but you're creating another list object and iterating over > it twice instead of once. It is slightly more readable, but not enough that > I 100% agree with it. But I also don't feel strongly enough to say no either. > > I'll defer to the two of you here. Yeah I agree it's not the most optimal way to do it, but the allItems list looked relatively small. Alternatively you could just remove all the disallowed from the allowed items before you call any of these methods and cache that list up front? Actually, it looks like we already do that with the disabled options... so you only have to do the following I think? Fine either way though. List<ContextMenuItem> items(allItems) items.retainAll(supportedOptions)
https://codereview.chromium.org/2747453002/diff/190001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java (right): https://codereview.chromium.org/2747453002/diff/190001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java:256: for (ContextMenuItem item : supportedOptions) { On 2017/03/15 06:06:40, David Trainor-ping if over 24h wrote: > Maybe supportedOptions.removeAll(getDisabledOptions(params))? Looks like menuOptions isn't used at all, maybe this block should just be removed.
More comments! A few questions to dtrainor and tedchoc about things. https://codereview.chromium.org/2747453002/diff/190001/chrome/android/java/re... File chrome/android/java/res/values/ids.xml (right): https://codereview.chromium.org/2747453002/diff/190001/chrome/android/java/re... chrome/android/java/res/values/ids.xml:76: <!-- Link Group --> On 2017/03/15 06:06:40, David Trainor-ping if over 24h wrote: > New line before each comment group? Done. https://codereview.chromium.org/2747453002/diff/190001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java (right): https://codereview.chromium.org/2747453002/diff/190001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java:256: for (ContextMenuItem item : supportedOptions) { On 2017/03/15 16:07:45, Ted C wrote: > On 2017/03/15 06:06:40, David Trainor-ping if over 24h wrote: > > Maybe supportedOptions.removeAll(getDisabledOptions(params))? > > Looks like menuOptions isn't used at all, maybe this block should just be > removed. Done. https://codereview.chromium.org/2747453002/diff/190001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java:277: if (groupedItems.isEmpty()) { On 2017/03/15 06:06:40, David Trainor-ping if over 24h wrote: > This last chunk of logic is a bit confusing. Can you comment a bit about this > in the code? I think we're doing: > > 1. Adding an empty group if we have no groups already added. > 2. Adding the "Open in Chrome" menu item as the last element in whatever the > last group is (assuming it's a valid item). > 3. Adding some custom tabs items at the front of the first group if possible. > 4. Removing the first group if it's empty. Yeah, it's a confusing portion of code. Commented on it and added reasons to this piece of code. https://codereview.chromium.org/2747453002/diff/190001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java:297: if (groupedItems.get(0).second.isEmpty()) { On 2017/03/15 06:06:40, David Trainor-ping if over 24h wrote: > If it's empty, do we have any groups period? Is that okay? Yeah since at this line of code there has to be at least one group, and the code before the special cases specifies that a group is only created if there is at least one item within it. https://codereview.chromium.org/2747453002/diff/190001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java:328: for (int i = 0; i < allItems.size(); i++) { On 2017/03/15 15:47:13, Ted C wrote: > On 2017/03/15 06:06:40, David Trainor-ping if over 24h wrote: > > I'm fine with this, but you could probably do something like the following I > > think? > > > > List<ContextMenuItem> items(allItems); > > items.retainAll(supportedOptions); > > items.removeAll(disabledOptions); > > > > validItems.add(items); > > This seems fine, but you're creating another list object and iterating over > it twice instead of once. It is slightly more readable, but not enough that > I 100% agree with it. But I also don't feel strongly enough to say no either. > > I'll defer to the two of you here. Most of the comments so far from Ted has been among the theme of less iterations so I'll leave side with Ted https://codereview.chromium.org/2747453002/diff/190001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java:342: validItems.add(0, item); On 2017/03/15 06:06:40, David Trainor-ping if over 24h wrote: > What if validItems already contains item? > > Also could just do something like the above (or turn the above into a helper and > use it here): > > List<ContextMenuItem> items(allItems); > items.retainAll(supportedOptions); > items.removeAll(disabledOptions); > > validItems.addAll(0, items); Gonna defer to Ted's about item instantiating here. However since each group is only called once this could never happen. But do you feel like we should consider that possible use case and change it so? https://codereview.chromium.org/2747453002/diff/190001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuItem.java (right): https://codereview.chromium.org/2747453002/diff/190001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuItem.java:23: OPEN_IN_BROWSER_ID(0, R.id.context_menu_open_in_browser_id), On 2017/03/15 06:06:40, David Trainor-ping if over 24h wrote: > is @StringRes ok with 0? I guess we don't crash with the annotations, just > checking. Eeeeeh, to be on the safe side I added something to getString. https://codereview.chromium.org/2747453002/diff/190001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuItem.java:24: // Link Group On 2017/03/15 06:06:40, David Trainor-ping if over 24h wrote: > New line before each comment? Done. https://codereview.chromium.org/2747453002/diff/190001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuItem.java:52: public final int stringId; On 2017/03/15 06:06:40, David Trainor-ping if over 24h wrote: > @StringRes here too? That is a cool annotation :). Done. https://codereview.chromium.org/2747453002/diff/190001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuItem.java:53: public final int menuId; On 2017/03/15 06:06:40, David Trainor-ping if over 24h wrote: > @IdRes or @MenuRes? :) Done. https://codereview.chromium.org/2747453002/diff/190001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuItem.java:61: ContextMenuItem(@StringRes int stringId, int menuId) { On 2017/03/15 06:06:40, David Trainor-ping if over 24h wrote: > Same here Done. https://codereview.chromium.org/2747453002/diff/190001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/PlatformContextMenuUi.java (right): https://codereview.chromium.org/2747453002/diff/190001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/PlatformContextMenuUi.java:32: setHeaderText(activity, mMenu, ChromeContextMenuPopulator.createHeaderText(params)); On 2017/03/15 06:06:40, David Trainor-ping if over 24h wrote: > This feels a bit roundabout (we're pushing from a populator but then we're > pulling from it here). I don't feel strongly either way though. Had similar thoughts when making this I was thinking of creating an abstract class but then realize that might be a little too much for a single method. So I decided against it.
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: Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
Super friendly ping: https://lh3.googleusercontent.com/-FkRI7blcSdk/WMsit0TTPaI/AAAAAAAACyc/MyEEl8...
still lgtm
lgtm https://codereview.chromium.org/2747453002/diff/190001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java (right): https://codereview.chromium.org/2747453002/diff/190001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java:342: validItems.add(0, item); On 2017/03/15 17:29:53, JJ wrote: > On 2017/03/15 06:06:40, David Trainor-ping if over 24h wrote: > > What if validItems already contains item? > > > > Also could just do something like the above (or turn the above into a helper > and > > use it here): > > > > List<ContextMenuItem> items(allItems); > > items.retainAll(supportedOptions); > > items.removeAll(disabledOptions); > > > > validItems.addAll(0, items); > > Gonna defer to Ted's about item instantiating here. However since each group is > only called once this could never happen. But do you feel like we should > consider that possible use case and change it so? Eh I'd just add && !validItems.contains(item) to be safe.
Thanks for your comments! Uploading! https://codereview.chromium.org/2747453002/diff/190001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java (right): https://codereview.chromium.org/2747453002/diff/190001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java:342: validItems.add(0, item); On 2017/03/17 21:00:09, David Trainor-ping if over 24h wrote: > On 2017/03/15 17:29:53, JJ wrote: > > On 2017/03/15 06:06:40, David Trainor-ping if over 24h wrote: > > > What if validItems already contains item? > > > > > > Also could just do something like the above (or turn the above into a helper > > and > > > use it here): > > > > > > List<ContextMenuItem> items(allItems); > > > items.retainAll(supportedOptions); > > > items.removeAll(disabledOptions); > > > > > > validItems.addAll(0, items); > > > > Gonna defer to Ted's about item instantiating here. However since each group > is > > only called once this could never happen. But do you feel like we should > > consider that possible use case and change it so? > > Eh I'd just add && !validItems.contains(item) to be safe. Done.
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...
yay for contradicting opinions! https://codereview.chromium.org/2747453002/diff/190001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java (right): https://codereview.chromium.org/2747453002/diff/190001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java:342: validItems.add(0, item); On 2017/03/17 21:03:14, JJ wrote: > On 2017/03/17 21:00:09, David Trainor-ping if over 24h wrote: > > On 2017/03/15 17:29:53, JJ wrote: > > > On 2017/03/15 06:06:40, David Trainor-ping if over 24h wrote: > > > > What if validItems already contains item? > > > > > > > > Also could just do something like the above (or turn the above into a > helper > > > and > > > > use it here): > > > > > > > > List<ContextMenuItem> items(allItems); > > > > items.retainAll(supportedOptions); > > > > items.removeAll(disabledOptions); > > > > > > > > validItems.addAll(0, items); > > > > > > Gonna defer to Ted's about item instantiating here. However since each group > > is > > > only called once this could never happen. But do you feel like we should > > > consider that possible use case and change it so? > > > > Eh I'd just add && !validItems.contains(item) to be safe. > > Done. validItems is a list. .contains requires iterating it for each addition. Right now all the groups are mutually exclusive, so this seems rather wasteful to me. Can we not just do assert !validItems.contains(item) for now? This seems like it would be covering up a coding error by adding it in at this point.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The back and forth! It's too much! Thanks for all your help though. https://codereview.chromium.org/2747453002/diff/190001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java (right): https://codereview.chromium.org/2747453002/diff/190001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java:342: validItems.add(0, item); On 2017/03/17 21:59:18, Ted C wrote: > On 2017/03/17 21:03:14, JJ wrote: > > On 2017/03/17 21:00:09, David Trainor-ping if over 24h wrote: > > > On 2017/03/15 17:29:53, JJ wrote: > > > > On 2017/03/15 06:06:40, David Trainor-ping if over 24h wrote: > > > > > What if validItems already contains item? > > > > > > > > > > Also could just do something like the above (or turn the above into a > > helper > > > > and > > > > > use it here): > > > > > > > > > > List<ContextMenuItem> items(allItems); > > > > > items.retainAll(supportedOptions); > > > > > items.removeAll(disabledOptions); > > > > > > > > > > validItems.addAll(0, items); > > > > > > > > Gonna defer to Ted's about item instantiating here. However since each > group > > > is > > > > only called once this could never happen. But do you feel like we should > > > > consider that possible use case and change it so? > > > > > > Eh I'd just add && !validItems.contains(item) to be safe. > > > > Done. > > validItems is a list. .contains requires iterating it for each addition. Right > now all the groups are mutually exclusive, so this seems rather wasteful to me. > > Can we not just do assert !validItems.contains(item) for now? This seems like > it would be covering up a coding error by adding it in at this point. Done?!
submit away...dtrainor@ and I can battle royale offline if he disagrees (in an entirely work friendly, pc, not actual battle or even particularly royale sort of way) https://codereview.chromium.org/2747453002/diff/250001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java (right): https://codereview.chromium.org/2747453002/diff/250001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java:331: assert !validItems.contains(item); you have one too many spaces after assert (below as well)
On 2017/03/17 23:35:39, Ted C wrote: > submit away...dtrainor@ and I can battle royale offline if he disagrees (in an > entirely work friendly, pc, not actual battle or even particularly royale sort > of way) > > https://codereview.chromium.org/2747453002/diff/250001/chrome/android/java/sr... > File > chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java > (right): > > https://codereview.chromium.org/2747453002/diff/250001/chrome/android/java/sr... > chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java:331: > assert !validItems.contains(item); > you have one too many spaces after assert (below as well) :P
Code will be submitted shortly! Sorry there was a bug (https://bugs.chromium.org/p/chromium/issues/detail?id=703290) that made me hesitant on submitting. At first it looked linked to the context menu but now it looks like it's linked to both a long press and a tap so it seems not to be only the context menu. Please take a look at the next cl (https://codereview.chromium.org/2751333006/) when you have the chance.
The CQ bit was checked by jwanda@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tedchoc@chromium.org, dtrainor@chromium.org Link to the patchset: https://codereview.chromium.org/2747453002/#ps290001 (title: "One space too many")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by jwanda@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tedchoc@chromium.org, dtrainor@chromium.org Link to the patchset: https://codereview.chromium.org/2747453002/#ps310001 (title: "I'm genuinely confused on why some code did not upload")
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": 310001, "attempt_start_ts": 1490046427633230,
"parent_rev": "cc6b59309149c3bff4a8c7eb57252d7323fab0dd", "commit_rev":
"fdd4f1fc2b5e991c9e0484e42bbee1f04102f9d3"}
Message was sent while issue was closed.
Description was changed from ========== Split context menu display and population/handling This change had a main goal of deleteing chrome_context_menu.xml and instead usings a combination of ids.xml and ContextMenuItems.java in order to create a context menu. This is done mainly in preperation to simply create the ContextMenu file. The tests were covered in a previous cl (https://codereview.chromium.org/2710833002/). BUG=613357 ========== to ========== Split context menu display and population/handling This change had a main goal of deleteing chrome_context_menu.xml and instead usings a combination of ids.xml and ContextMenuItems.java in order to create a context menu. This is done mainly in preperation to simply create the ContextMenu file. The tests were covered in a previous cl (https://codereview.chromium.org/2710833002/). BUG=613357 Review-Url: https://codereview.chromium.org/2747453002 Cr-Commit-Position: refs/heads/master@{#458248} Committed: https://chromium.googlesource.com/chromium/src/+/fdd4f1fc2b5e991c9e0484e42bbe... ==========
Message was sent while issue was closed.
Committed patchset #17 (id:310001) as https://chromium.googlesource.com/chromium/src/+/fdd4f1fc2b5e991c9e0484e42bbe... |
