Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2815453002:
[Android] Show Browser Actions dialog in Chrome (Closed)
Description[Android] Show Browser Actions dialog in Chrome
Display Browser Actions dialog by refactoring Chrome context menu code. Add
ContextMenuItemGetter interface to provide the capability to custom menu
title and icon. Also add BrowserActionsContextMenuPopulator to match the
menu id with custom behavior.
BUG=710078
Review-Url: https://codereview.chromium.org/2815453002
Cr-Commit-Position: refs/heads/master@{#473124}
Committed: https://chromium.googlesource.com/chromium/src/+/fc5ed1f9afe1fee37119450781651e2fbae29f6c
Patch Set 1 #Patch Set 2 : Update default menu list based on new UI mocks. #
Total comments: 6
Patch Set 3 : Update based on Ted's comments. #
Total comments: 28
Patch Set 4 : Update based on Ted's comments. #Patch Set 5 : git rebase. #Patch Set 6 : Rebase to latest changes. #Patch Set 7 : Another rebase. #
Total comments: 22
Patch Set 8 : Update based on Yusuf's comments. #
Total comments: 16
Patch Set 9 : Update based on Maria's comments. #
Total comments: 32
Patch Set 10 : Update based on Ted's comments. #
Total comments: 9
Patch Set 11 : Update based on Ted's new comments. #
Total comments: 4
Patch Set 12 : Update based on Ted's comments. #
Total comments: 4
Patch Set 13 : Update based on Ted's comments. #Patch Set 14 : Rebase #Patch Set 15 : Fix tests fail. #Dependent Patchsets: Messages
Total messages: 49 (18 generated)
ltian@chromium.org changed reviewers: + tedchoc@chromium.org, yusufo@chromium.org
Can you take a look whether these changes makes sense to you? Thanks!
https://codereview.chromium.org/2815453002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java (right): https://codereview.chromium.org/2815453002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java:59: private static final Set<? extends ContextMenuItemGetter> BASE_WHITELIST = do you need the "? extends " here or can you just use the interface? https://codereview.chromium.org/2815453002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuItemGetter.java (right): https://codereview.chromium.org/2815453002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuItemGetter.java:14: public interface ContextMenuItemGetter { This should just be called ContextMenuItem https://codereview.chromium.org/2815453002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuItemSelector.java (right): https://codereview.chromium.org/2815453002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuItemSelector.java:18: public abstract class ContextMenuItemSelector { Really, I think this is what the Populator interface should be limited too. In general, I don't think the browser action code should use a populator in it's current form at all. The populator is still very specific to the chrome context menu and I think it doesn't do anything for us. If there are useful helper functions, I think we should just pull out ContextMenuPopulatorUtils or something where we can share the behavior. For now, I think browser actions can do it much more simply than that code since it only supports a single path (not 4 modes).
https://codereview.chromium.org/2815453002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java (right): https://codereview.chromium.org/2815453002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java:59: private static final Set<? extends ContextMenuItemGetter> BASE_WHITELIST = On 2017/04/14 20:16:07, Ted C wrote: > do you need the "? extends " here or can you just use the interface? If just using the interface, it will report an error "incompatible types: List<ChromeContextMenuItem> cannot be converted to List<ContextMenuItem>" and I search it only, it seems for Java Collections, we need to add the "? extends" to solve this problem. https://codereview.chromium.org/2815453002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuItemGetter.java (right): https://codereview.chromium.org/2815453002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuItemGetter.java:14: public interface ContextMenuItemGetter { On 2017/04/14 20:16:07, Ted C wrote: > This should just be called ContextMenuItem Done. https://codereview.chromium.org/2815453002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuItemSelector.java (right): https://codereview.chromium.org/2815453002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuItemSelector.java:18: public abstract class ContextMenuItemSelector { On 2017/04/14 20:16:07, Ted C wrote: > Really, I think this is what the Populator interface should be limited too. > > In general, I don't think the browser action code should use a populator in > it's current form at all. The populator is still very specific to the > chrome context menu and I think it doesn't do anything for us. > > If there are useful helper functions, I think we should just pull out > ContextMenuPopulatorUtils or something where we can share the behavior. > > For now, I think browser actions can do it much more simply than that > code since it only supports a single path (not 4 modes). The latest change removes this class and reverts back the implementation of ChromeContextMenuPopulator. It also removes the populator class of Browser Actions and simplifies the population process and moves all into the helper class of Browser Actions.
srahim@chromium.org changed reviewers: + srahim@chromium.org
https://codereview.chromium.org/2815453002/diff/30001/chrome/android/java/str... File chrome/android/java/strings/android_chrome_strings.grd (right): https://codereview.chromium.org/2815453002/diff/30001/chrome/android/java/str... chrome/android/java/strings/android_chrome_strings.grd:1611: Open in incognito tab UI review deck shows this as "Open in incognito Chrome tab", was it deliberately changed?
tedchoc@chromium.org changed reviewers: + mariakhomenko@chromium.org
+mariakhomenko for the referrer comment https://codereview.chromium.org/2815453002/diff/30001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionActivity.java (right): https://codereview.chromium.org/2815453002/diff/30001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionActivity.java:46: LinearLayout layout = new LinearLayout(this); why do you need this? Does getWindow().getDecorView() not work? Is setContentView required by Activity (we always do it, but I don't know what happens if you don't). If we do need this, why does this need to be a LinearLayout? Would just a vanilla View work? https://codereview.chromium.org/2815453002/diff/30001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionActivity.java:54: public void openContextMenu(LinearLayout layout) { Unless you need some specific behavior of LinearLayout, this should be passed in as either a View or ViewGroup as needed. https://codereview.chromium.org/2815453002/diff/30001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionActivity.java:70: Referrer referrer = new Referrer(mUri.toString(), Referrer.REFERRER_POLICY_DEFAULT); The referrer isn't normally the URL that you long clicked on but the URL of the current page. I think this should either be null or use the logic in IntentHandler.getReferrerUrlIncludingExtraHeaders. @mariakhomenko, does this sound reasonable? https://codereview.chromium.org/2815453002/diff/30001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionActivity.java:137: public void setOnCloseContextMenuListener(OnCloseContextMenuListener listener) { I don't think the helper should be setting this on the activity. I think we should hold onto the helper reference and call onContextMenuClosed. https://codereview.chromium.org/2815453002/diff/30001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuHelper.java (right): https://codereview.chromium.org/2815453002/diff/30001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuHelper.java:42: private final BrowserActionActivity mActivity; We should find a way to make this just a vanilla activity reference here. https://codereview.chromium.org/2815453002/diff/30001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuHelper.java:51: private static final List<? extends ContextMenuItem> BROWSERACTIONS_LINK_GROUP = ordering nit: statics above local finals above local non-finals https://codereview.chromium.org/2815453002/diff/30001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuHelper.java:64: private static final List<BrowserActionsCustomContextMenuItem> CUSTOM_ITEM_GROUP = this doesn't look like it should be static. https://codereview.chromium.org/2815453002/diff/30001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuHelper.java:68: private static final Map<Integer, PendingIntent> CUSTOM_TIME_ACTION_MAP = new HashMap<>(); Should we use a https://developer.android.com/reference/android/util/SparseArray.html here? Also, this doesn't look like it should be static either. https://codereview.chromium.org/2815453002/diff/30001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuHelper.java:166: }, 50); why does this need to be delayed? Why 50ms? https://codereview.chromium.org/2815453002/diff/30001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuHelper.java:179: clearCustomItems(); Should this be mOnMenuClosed.run() ? https://codereview.chromium.org/2815453002/diff/30001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsCustomContextMenuItem.java (right): https://codereview.chromium.org/2815453002/diff/30001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsCustomContextMenuItem.java:25: BrowserActionsCustomContextMenuItem(@IdRes int id, BrowserActionItem item) { javadoc for this https://codereview.chromium.org/2815453002/diff/30001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsCustomContextMenuItem.java:43: return new BitmapDrawable(context.getResources(), mIcon); I "think" we might want to cache this drawable. Calling get two times might end up causing 2x the memory consumed.
https://codereview.chromium.org/2815453002/diff/30001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionActivity.java (right): https://codereview.chromium.org/2815453002/diff/30001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionActivity.java:70: Referrer referrer = new Referrer(mUri.toString(), Referrer.REFERRER_POLICY_DEFAULT); On 2017/04/19 19:58:41, Ted C wrote: > The referrer isn't normally the URL that you long clicked on but the URL of the > current page. I think this should either be null or use the logic in > IntentHandler.getReferrerUrlIncludingExtraHeaders. > > @mariakhomenko, does this sound reasonable? I think this should be an android-app:// URL of the application initiating the context menu. You can construct the android-app:// URL using the package name for the application. Seems like that's something you should be able to know here? (mCreatorPackageName?)
https://codereview.chromium.org/2815453002/diff/30001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionActivity.java (right): https://codereview.chromium.org/2815453002/diff/30001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionActivity.java:46: LinearLayout layout = new LinearLayout(this); On 2017/04/19 19:58:41, Ted C wrote: > why do you need this? > > Does getWindow().getDecorView() not work? Is setContentView required by > Activity (we always do it, but I don't know what happens if you don't). > > If we do need this, why does this need to be a LinearLayout? Would just a > vanilla View work? Just try that if we don't have setContentView, the old version (using Android Context Menu API one) context menu is not displayed. And also pass getWindow().getDecorView() to openContextMenu does not work even we call setContentView. But creating a simple View is definitely enough. https://codereview.chromium.org/2815453002/diff/30001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionActivity.java:54: public void openContextMenu(LinearLayout layout) { On 2017/04/19 19:58:41, Ted C wrote: > Unless you need some specific behavior of LinearLayout, this should be passed in > as either a View or ViewGroup as needed. Thanks for pointing out, I think passing in a View is a better option. https://codereview.chromium.org/2815453002/diff/30001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionActivity.java:70: Referrer referrer = new Referrer(mUri.toString(), Referrer.REFERRER_POLICY_DEFAULT); On 2017/04/19 21:16:01, Maria wrote: > On 2017/04/19 19:58:41, Ted C wrote: > > The referrer isn't normally the URL that you long clicked on but the URL of > the > > current page. I think this should either be null or use the logic in > > IntentHandler.getReferrerUrlIncludingExtraHeaders. > > > > @mariakhomenko, does this sound reasonable? > > I think this should be an android-app:// URL of the application initiating the > context menu. > > You can construct the android-app:// URL using the package name for the > application. Seems like that's something you should be able to know here? > (mCreatorPackageName?) Done. https://codereview.chromium.org/2815453002/diff/30001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionActivity.java:137: public void setOnCloseContextMenuListener(OnCloseContextMenuListener listener) { On 2017/04/19 19:58:41, Ted C wrote: > I don't think the helper should be setting this on the activity. I think we > should hold onto the helper reference and call onContextMenuClosed. Done. https://codereview.chromium.org/2815453002/diff/30001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuHelper.java (right): https://codereview.chromium.org/2815453002/diff/30001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuHelper.java:42: private final BrowserActionActivity mActivity; On 2017/04/19 19:58:41, Ted C wrote: > We should find a way to make this just a vanilla activity reference here. Done. https://codereview.chromium.org/2815453002/diff/30001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuHelper.java:51: private static final List<? extends ContextMenuItem> BROWSERACTIONS_LINK_GROUP = On 2017/04/19 19:58:41, Ted C wrote: > ordering nit: statics above local finals above local non-finals Done. https://codereview.chromium.org/2815453002/diff/30001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuHelper.java:64: private static final List<BrowserActionsCustomContextMenuItem> CUSTOM_ITEM_GROUP = On 2017/04/19 19:58:41, Ted C wrote: > this doesn't look like it should be static. Done. https://codereview.chromium.org/2815453002/diff/30001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuHelper.java:68: private static final Map<Integer, PendingIntent> CUSTOM_TIME_ACTION_MAP = new HashMap<>(); On 2017/04/19 19:58:41, Ted C wrote: > Should we use a > https://developer.android.com/reference/android/util/SparseArray.html here? > > Also, this doesn't look like it should be static either. Done. https://codereview.chromium.org/2815453002/diff/30001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuHelper.java:166: }, 50); On 2017/04/19 19:58:41, Ted C wrote: > why does this need to be delayed? Why 50ms? The only reason I have this Handler is because if not, Chrome will crash with error "Failed looking up window. IllegalArgmentException: Requested window null does not exist." And find a post online shows this could solve the problem. 50ms is just a number I find in Chrome that is set for delay for Handler. https://codereview.chromium.org/2815453002/diff/30001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuHelper.java:179: clearCustomItems(); On 2017/04/19 19:58:41, Ted C wrote: > Should this be mOnMenuClosed.run() ? Done. https://codereview.chromium.org/2815453002/diff/30001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsCustomContextMenuItem.java (right): https://codereview.chromium.org/2815453002/diff/30001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsCustomContextMenuItem.java:25: BrowserActionsCustomContextMenuItem(@IdRes int id, BrowserActionItem item) { On 2017/04/19 19:58:41, Ted C wrote: > javadoc for this Done. https://codereview.chromium.org/2815453002/diff/30001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsCustomContextMenuItem.java:43: return new BitmapDrawable(context.getResources(), mIcon); On 2017/04/19 19:58:41, Ted C wrote: > I "think" we might want to cache this drawable. Calling get two times might end > up causing 2x the memory consumed. Sorry I am not sure how should we cache this drawable. Should we store it in locally as a bitmap? https://codereview.chromium.org/2815453002/diff/30001/chrome/android/java/str... File chrome/android/java/strings/android_chrome_strings.grd (right): https://codereview.chromium.org/2815453002/diff/30001/chrome/android/java/str... chrome/android/java/strings/android_chrome_strings.grd:1611: Open in incognito tab On 2017/04/18 20:29:38, srahim wrote: > UI review deck shows this as "Open in incognito Chrome tab", was it deliberately > changed? This recent mock (https://folio.googleplex.com/chrome-ux/mocks/293-web-view-read-only-mode/0323...) shows it is "Open in incognito tab". And I think the reason we remove "Chrome" from it is because we need to limit the character for translation to 30 (otherwise string will not be all shown in small screen devices).
https://codereview.chromium.org/2815453002/diff/30001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsCustomContextMenuItem.java (right): https://codereview.chromium.org/2815453002/diff/30001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsCustomContextMenuItem.java:43: return new BitmapDrawable(context.getResources(), mIcon); On 2017/04/21 05:10:54, ltian wrote: > On 2017/04/19 19:58:41, Ted C wrote: > > I "think" we might want to cache this drawable. Calling get two times might > end > > up causing 2x the memory consumed. > > Sorry I am not sure how should we cache this drawable. Should we store it in > locally as a bitmap? Looks like the BitmapDrawable stores the reference for the drawable, so calling it twice should not consume 2x the memory.
Since there is a lot of new logic we are introducing, I feel like this is a good point to add a test harness and add tests. https://codereview.chromium.org/2815453002/diff/110001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionActivity.java (right): https://codereview.chromium.org/2815453002/diff/110001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionActivity.java:105: IntentHandler.ANDROID_APP_REFERRER_SCHEME + "://" + mCreatorPackageName, See IntentHandler.constructValidReferrerForAuthority and usages for this. https://codereview.chromium.org/2815453002/diff/110001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionActivity.java:159: mHelper.onContextMenuClosed(); one line https://codereview.chromium.org/2815453002/diff/110001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuHelper.java (right): https://codereview.chromium.org/2815453002/diff/110001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuHelper.java:41: Collections.unmodifiableList(CollectionUtil.newArrayList( Arrays.asList also does the same thing I think https://codereview.chromium.org/2815453002/diff/110001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuHelper.java:42: ChromeContextMenuItem.BROWSERACTIONS_OPEN_IN_BACKGROUND, BROWSER_ACTIONS here and elsewhere I think. https://codereview.chromium.org/2815453002/diff/110001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuHelper.java:55: // Map the custom items' ids with their PendingIntent actions. Map that links ... https://codereview.chromium.org/2815453002/diff/110001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuHelper.java:72: mActivity = activity; I would save this as a BrowserActionActivity and get it so as param if that is what you need eventually. https://codereview.chromium.org/2815453002/diff/110001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuHelper.java:148: javadoc https://codereview.chromium.org/2815453002/diff/110001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuHelper.java:160: mOnMenuShown.run(); doesnt this already run when the menu is dismissed? If no why are we passing it below? https://codereview.chromium.org/2815453002/diff/110001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuHelper.java:162: } why delayed? why 50? https://codereview.chromium.org/2815453002/diff/110001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuHelper.java:176: mOnMenuClosed.run(); doesnt this already run when the menu is dismissed? If no why are we passing it above? https://codereview.chromium.org/2815453002/diff/110001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuItemDelegate.java (right): https://codereview.chromium.org/2815453002/diff/110001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuItemDelegate.java:25: private final Context mContext; if all you need is a Context and using Application is OK, use ContextUtils.getApplicationContext. If not and you need Activity, pass as activity.
https://codereview.chromium.org/2815453002/diff/110001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionActivity.java (right): https://codereview.chromium.org/2815453002/diff/110001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionActivity.java:105: IntentHandler.ANDROID_APP_REFERRER_SCHEME + "://" + mCreatorPackageName, On 2017/05/09 23:39:36, Yusuf wrote: > See IntentHandler.constructValidReferrerForAuthority and usages for this. Done. https://codereview.chromium.org/2815453002/diff/110001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionActivity.java:159: mHelper.onContextMenuClosed(); On 2017/05/09 23:39:36, Yusuf wrote: > one line Done. https://codereview.chromium.org/2815453002/diff/110001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuHelper.java (right): https://codereview.chromium.org/2815453002/diff/110001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuHelper.java:41: Collections.unmodifiableList(CollectionUtil.newArrayList( On 2017/05/09 23:39:37, Yusuf (OOO May 10-17) wrote: > Arrays.asList also does the same thing I think Done. https://codereview.chromium.org/2815453002/diff/110001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuHelper.java:42: ChromeContextMenuItem.BROWSERACTIONS_OPEN_IN_BACKGROUND, On 2017/05/09 23:39:36, Yusuf wrote: > BROWSER_ACTIONS here and elsewhere I think. Done. https://codereview.chromium.org/2815453002/diff/110001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuHelper.java:55: // Map the custom items' ids with their PendingIntent actions. On 2017/05/09 23:39:37, Yusuf wrote: > Map that links ... Done. https://codereview.chromium.org/2815453002/diff/110001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuHelper.java:72: mActivity = activity; On 2017/05/09 23:39:37, Yusuf wrote: > I would save this as a BrowserActionActivity and get it so as param if that is > what you need eventually. Done. https://codereview.chromium.org/2815453002/diff/110001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuHelper.java:148: On 2017/05/09 23:39:36, Yusuf wrote: > javadoc Done. https://codereview.chromium.org/2815453002/diff/110001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuHelper.java:160: mOnMenuShown.run(); On 2017/05/09 23:39:36, Yusuf wrote: > doesnt this already run when the menu is dismissed? If no why are we passing it > below? For new UI, this is not needed. But for old UI, the Android Context Menu, it need to be explicitly called. https://codereview.chromium.org/2815453002/diff/110001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuHelper.java:162: } On 2017/05/09 23:39:36, Yusuf wrote: > why delayed? why 50? The Handler is needed because if not, Chrome will crash with error "Failed looking up window. IllegalArgmentException: Requested window null does not exist." And I find a post online shows this could solve the problem. 50ms is just a number I find somewhere in Chrome that is used for the delay of a Handler. Should I add some comments here to explain these? https://codereview.chromium.org/2815453002/diff/110001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuHelper.java:176: mOnMenuClosed.run(); On 2017/05/09 23:39:37, Yusuf wrote: > doesnt this already run when the menu is dismissed? If no why are we passing it > above? If we use the new UI it is not necessary. It is only needed for old UI which uses Android Context Menu library. https://codereview.chromium.org/2815453002/diff/110001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuItemDelegate.java (right): https://codereview.chromium.org/2815453002/diff/110001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuItemDelegate.java:25: private final Context mContext; On 2017/05/09 23:39:37, Yusuf (OOO May 10-17) wrote: > if all you need is a Context and using Application is OK, use > ContextUtils.getApplicationContext. If not and you need Activity, pass as > activity. I think it needs both Context and Activity. Context is used to start new Activity from Intent and send Notification and Activity is used to open share dialog.
https://codereview.chromium.org/2815453002/diff/130001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionActivity.java (right): https://codereview.chromium.org/2815453002/diff/130001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionActivity.java:99: * imageWasFetchedLoFi} and canSavemedia are set false. unmatched closing curly brace. Also instead of this whole comment, I would suggest annotating the parameters inline, e.g. ContextMenuParams(..., false /* canSaveMedia */, refererrer, false /* imageWasFetchedLoFi */) That'll make the use more clear. https://codereview.chromium.org/2815453002/diff/130001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionActivity.java:159: super.finish(); should this be just finish()? Why super specifically? https://codereview.chromium.org/2815453002/diff/130001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuHelper.java (right): https://codereview.chromium.org/2815453002/diff/130001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuHelper.java:65: private boolean mIsNewUIEnable = true; this value never changes, is that something to be implemented or can unreachable code be removed? Also what is this newUI? finally, naming nit, should be: isNewUiEnabled https://codereview.chromium.org/2815453002/diff/130001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuHelper.java:80: mActivity.finish(); is this different from super.finish() call in the onContextMenuClosed() callback? https://codereview.chromium.org/2815453002/diff/130001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuHelper.java:100: * Build items for Browser Actions context menu. nit: Build -> Builds https://codereview.chromium.org/2815453002/diff/130001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuHelper.java:113: * Functions to add List of {@link BrowserActionItem} to the populator English nit: Populates custom actions map. https://codereview.chromium.org/2815453002/diff/130001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuItem.java (right): https://codereview.chromium.org/2815453002/diff/130001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuItem.java:105: this.mMenuId = menuId; nit: no longer need this. for these assignments https://codereview.chromium.org/2815453002/diff/130001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuParams.java (right): https://codereview.chromium.org/2815453002/diff/130001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuParams.java:140: Referrer referrer, boolean canSavemedia) { while you are here, can you rename canSavemedia to canSaveMedia please?
https://codereview.chromium.org/2815453002/diff/130001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionActivity.java (right): https://codereview.chromium.org/2815453002/diff/130001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionActivity.java:99: * imageWasFetchedLoFi} and canSavemedia are set false. On 2017/05/10 22:11:49, Maria wrote: > unmatched closing curly brace. > > Also instead of this whole comment, I would suggest annotating the parameters > inline, e.g. > > ContextMenuParams(..., false /* canSaveMedia */, refererrer, false /* > imageWasFetchedLoFi */) > > That'll make the use more clear. Done. https://codereview.chromium.org/2815453002/diff/130001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionActivity.java:159: super.finish(); On 2017/05/10 22:11:49, Maria wrote: > should this be just finish()? Why super specifically? Yes, it is. Thanks for pointing out. I think this finish() is even not necessary because mOnMenuClosed in BrowserActionContextMenu always call this. https://codereview.chromium.org/2815453002/diff/130001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuHelper.java (right): https://codereview.chromium.org/2815453002/diff/130001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuHelper.java:65: private boolean mIsNewUIEnable = true; On 2017/05/10 22:11:50, Maria wrote: > this value never changes, is that something to be implemented or can unreachable > code be removed? > > Also what is this newUI? > > finally, naming nit, should be: isNewUiEnabled Recently we have a new version of Context Menu Ui and that Ui is controlled through ChromeFeatureList (https://codesearch.chromium.org/chromium/src/chrome/android/java/src/org/chro...). Because ChromeFeatureList relies on native code, but when this Browser Action dialog is shown, the native libraries have not been loaded, we have this flag to control which version to use. Now this is mainly a simple way to test the code works ok on both Uis. Later we will either figure out a way to control for both this and Chrome context menu or directly use the new Ui. https://codereview.chromium.org/2815453002/diff/130001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuHelper.java:80: mActivity.finish(); On 2017/05/10 22:11:50, Maria wrote: > is this different from super.finish() call in the onContextMenuClosed() > callback? Yes, they are the same. Thank you for pointing out. I think the one in onContextMenuClosed() is not needed. https://codereview.chromium.org/2815453002/diff/130001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuHelper.java:100: * Build items for Browser Actions context menu. On 2017/05/10 22:11:50, Maria wrote: > nit: Build -> Builds Done. https://codereview.chromium.org/2815453002/diff/130001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuHelper.java:113: * Functions to add List of {@link BrowserActionItem} to the populator On 2017/05/10 22:11:49, Maria wrote: > English nit: Populates custom actions map. Done. https://codereview.chromium.org/2815453002/diff/130001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuItem.java (right): https://codereview.chromium.org/2815453002/diff/130001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuItem.java:105: this.mMenuId = menuId; On 2017/05/10 22:11:50, Maria wrote: > nit: no longer need this. for these assignments Done. https://codereview.chromium.org/2815453002/diff/130001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuParams.java (right): https://codereview.chromium.org/2815453002/diff/130001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuParams.java:140: Referrer referrer, boolean canSavemedia) { On 2017/05/10 22:11:50, Maria wrote: > while you are here, can you rename canSavemedia to canSaveMedia please? Done.
lgtm
https://codereview.chromium.org/2815453002/diff/150001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionActivity.java (right): https://codereview.chromium.org/2815453002/diff/150001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionActivity.java:100: private ContextMenuParams getContextMenuParams() { s/get/create https://codereview.chromium.org/2815453002/diff/150001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuHelper.java (right): https://codereview.chromium.org/2815453002/diff/150001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuHelper.java:45: private static final List<Integer> BROWSER_ACTIONS_ID_GROUP = [CUSTOM_]BROWSER_ACTIONS_ID_GROUP https://codereview.chromium.org/2815453002/diff/150001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuHelper.java:58: private Callback<Integer> mCallback; s/mCallback/mItemSelectedCallback https://codereview.chromium.org/2815453002/diff/150001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuHelper.java:63: private List<Pair<Integer, List<ContextMenuItem>>> mItems; looks like all this and above can be private https://codereview.chromium.org/2815453002/diff/150001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuHelper.java:65: private boolean mIsNewUiEnabled = true; this should be a static https://codereview.chromium.org/2815453002/diff/150001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuHelper.java:102: private void buildContextMenuItems(List<BrowserActionItem> customItems) { maybe this should just return a List<Pair<Integer, List<ContextMenuItem>>> instead of having to access mItems declared right above. https://codereview.chromium.org/2815453002/diff/150001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuHelper.java:118: mCustomItemGroup.add(new BrowserActionsCustomContextMenuItem( I don't think you need mCustomItemGroup at all. You could pass in the list of ContextMenuItem(s) and have it add it directly to that. https://codereview.chromium.org/2815453002/diff/150001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuHelper.java:124: private void clearCustomItems() { why do you need to do this? https://codereview.chromium.org/2815453002/diff/150001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuHelper.java:129: private boolean onItemSelected(ContextMenuParams params, int itemId) { no need to pass in params, you already have access to it with mCurrentContextMenuParams It seems odd to use mDelegate and mCustomItemActionMap, but pass in mCurrentContextMenuParams. https://codereview.chromium.org/2815453002/diff/150001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuHelper.java:156: new Handler().postDelayed(new Runnable() { maybe add a comment for why this is needed https://codereview.chromium.org/2815453002/diff/150001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuItemDelegate.java (right): https://codereview.chromium.org/2815453002/diff/150001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuItemDelegate.java:28: private final Activity mActivity; Activity isn't used https://codereview.chromium.org/2815453002/diff/150001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java (right): https://codereview.chromium.org/2815453002/diff/150001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java:315: public void populateItemGroup(@ContextMenuGroup int contextMenuType, @StringRes int titleResId, can all of these public's be reverted back to private? https://codereview.chromium.org/2815453002/diff/150001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuItem.java (right): https://codereview.chromium.org/2815453002/diff/150001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuItem.java:17: * @return. remove https://codereview.chromium.org/2815453002/diff/150001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuItem.java:34: Drawable getDrawableAndDescription(Context context); we should rename this as it now only returns the Drawable (not changed with your patch, but since you're here) https://codereview.chromium.org/2815453002/diff/150001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuListAdapter.java (right): https://codereview.chromium.org/2815453002/diff/150001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuListAdapter.java:32: public interface DirectShareDelegate { no longer needed?
https://codereview.chromium.org/2815453002/diff/150001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionActivity.java (right): https://codereview.chromium.org/2815453002/diff/150001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionActivity.java:100: private ContextMenuParams getContextMenuParams() { On 2017/05/12 23:35:25, Ted C wrote: > s/get/create Done. https://codereview.chromium.org/2815453002/diff/150001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuHelper.java (right): https://codereview.chromium.org/2815453002/diff/150001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuHelper.java:45: private static final List<Integer> BROWSER_ACTIONS_ID_GROUP = On 2017/05/12 23:35:26, Ted C wrote: > [CUSTOM_]BROWSER_ACTIONS_ID_GROUP Done. https://codereview.chromium.org/2815453002/diff/150001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuHelper.java:58: private Callback<Integer> mCallback; On 2017/05/12 23:35:26, Ted C wrote: > s/mCallback/mItemSelectedCallback Done. https://codereview.chromium.org/2815453002/diff/150001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuHelper.java:63: private List<Pair<Integer, List<ContextMenuItem>>> mItems; On 2017/05/12 23:35:25, Ted C wrote: > looks like all this and above can be private Do you mean final? I think final is ok for all these above. Will change all of them to final. https://codereview.chromium.org/2815453002/diff/150001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuHelper.java:65: private boolean mIsNewUiEnabled = true; On 2017/05/12 23:35:26, Ted C wrote: > this should be a static Done. https://codereview.chromium.org/2815453002/diff/150001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuHelper.java:102: private void buildContextMenuItems(List<BrowserActionItem> customItems) { On 2017/05/12 23:35:25, Ted C wrote: > maybe this should just return a List<Pair<Integer, List<ContextMenuItem>>> > instead of having to access mItems declared right above. Done. https://codereview.chromium.org/2815453002/diff/150001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuHelper.java:118: mCustomItemGroup.add(new BrowserActionsCustomContextMenuItem( On 2017/05/12 23:35:26, Ted C wrote: > I don't think you need mCustomItemGroup at all. You could pass in the list of > ContextMenuItem(s) and have it add it directly to that. Done. https://codereview.chromium.org/2815453002/diff/150001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuHelper.java:124: private void clearCustomItems() { On 2017/05/12 23:35:26, Ted C wrote: > why do you need to do this? Hmm, I remember I have this because I see the custom items keeps appending every time the dialog is re-shown. But somehow recently I cannot reproduce the problem... From the code, this is not necessary so I will remove it. https://codereview.chromium.org/2815453002/diff/150001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuHelper.java:129: private boolean onItemSelected(ContextMenuParams params, int itemId) { On 2017/05/12 23:35:26, Ted C wrote: > no need to pass in params, you already have access to it with > mCurrentContextMenuParams > > It seems odd to use mDelegate and mCustomItemActionMap, but pass in > mCurrentContextMenuParams. Done. https://codereview.chromium.org/2815453002/diff/150001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuHelper.java:156: new Handler().postDelayed(new Runnable() { On 2017/05/12 23:35:26, Ted C wrote: > maybe add a comment for why this is needed Done. Change the delay to 100 because there is still a chance (pretty low) Chrome crashes because of requested window is null. https://codereview.chromium.org/2815453002/diff/150001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuItemDelegate.java (right): https://codereview.chromium.org/2815453002/diff/150001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuItemDelegate.java:28: private final Activity mActivity; On 2017/05/12 23:35:26, Ted C wrote: > Activity isn't used It will be used for next CL when showing the toast and opening tab in the background. So I just leave it there for now. https://codereview.chromium.org/2815453002/diff/150001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java (right): https://codereview.chromium.org/2815453002/diff/150001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java:315: public void populateItemGroup(@ContextMenuGroup int contextMenuType, @StringRes int titleResId, On 2017/05/12 23:35:26, Ted C wrote: > can all of these public's be reverted back to private? For current implementation, they should be private. Thanks for pointing out! https://codereview.chromium.org/2815453002/diff/150001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuItem.java (right): https://codereview.chromium.org/2815453002/diff/150001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuItem.java:17: * @return. On 2017/05/12 23:35:26, Ted C wrote: > remove Done. https://codereview.chromium.org/2815453002/diff/150001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuItem.java:34: Drawable getDrawableAndDescription(Context context); On 2017/05/12 23:35:26, Ted C wrote: > we should rename this as it now only returns the Drawable (not changed with your > patch, but since you're here) Done. https://codereview.chromium.org/2815453002/diff/150001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuListAdapter.java (right): https://codereview.chromium.org/2815453002/diff/150001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuListAdapter.java:32: public interface DirectShareDelegate { On 2017/05/12 23:35:26, Ted C wrote: > no longer needed? Oh, yes, sorry forget removing it.
https://codereview.chromium.org/2815453002/diff/150001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuItemDelegate.java (right): https://codereview.chromium.org/2815453002/diff/150001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuItemDelegate.java:28: private final Activity mActivity; On 2017/05/15 21:47:04, ltian wrote: > On 2017/05/12 23:35:26, Ted C wrote: > > Activity isn't used > > It will be used for next CL when showing the toast and opening tab in the > background. So I just leave it there for now. Toast doesn't need an activity. A Context is fine. Please remove. In general, try not to add things to changes that aren't used. Even if you plan on adding it in a subsequent change. Changes should be entirely self contained. https://codereview.chromium.org/2815453002/diff/170001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuHelper.java (right): https://codereview.chromium.org/2815453002/diff/170001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuHelper.java:104: if (items.isEmpty()) return menuItems; Can this ever happen? Is this for in the future if we support different groups or something where only the custom options would be there? https://codereview.chromium.org/2815453002/diff/170001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuHelper.java:146: public void displayBrowserActionsMenu(final View view) { If mItems is empty, should we call mOnMenuClosed (probably posted to ensure it keeps the api consistent)? https://codereview.chromium.org/2815453002/diff/170001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuHelper.java:152: // Having a delay to display the context menu to avoid Chrome crashed from the s/crashed/crashing remove extra space after crashed. https://codereview.chromium.org/2815453002/diff/170001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuHelper.java:154: new Handler().postDelayed(new Runnable() { When this crashes, does mView.getWindowToken() == null? If so, can you add an addOnAttachStateChangeListener to see when that gets called to avoid the delay?
https://codereview.chromium.org/2815453002/diff/170001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuHelper.java (right): https://codereview.chromium.org/2815453002/diff/170001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuHelper.java:104: if (items.isEmpty()) return menuItems; On 2017/05/17 15:40:38, Ted C wrote: > Can this ever happen? Is this for in the future if we support different groups > or something where only the custom options would be there? Yes, this line should not happen for current code. I follow JJ's implementation for Chrome context menu and it might happen in the future if we support different menus for different types of urls. I might just remove this check since it does not happen now. https://codereview.chromium.org/2815453002/diff/170001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuHelper.java:146: public void displayBrowserActionsMenu(final View view) { On 2017/05/17 15:40:38, Ted C wrote: > If mItems is empty, should we call mOnMenuClosed (probably posted to ensure it > keeps the api consistent)? Yea, this also reminds me that the new Chrome context menu code might also need this because it is definitely possible for Chrome context menu to be empty. (I remember I see the context menu for a video is empty and screen is dim but no dialog shown). https://codereview.chromium.org/2815453002/diff/170001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuHelper.java:152: // Having a delay to display the context menu to avoid Chrome crashed from the On 2017/05/17 15:40:38, Ted C wrote: > s/crashed/crashing > > remove extra space after crashed. Done. https://codereview.chromium.org/2815453002/diff/170001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuHelper.java:154: new Handler().postDelayed(new Runnable() { On 2017/05/17 15:40:38, Ted C wrote: > When this crashes, does mView.getWindowToken() == null? > > If so, can you add an addOnAttachStateChangeListener to see when that gets > called to avoid the delay? That works, that is definitely a better solution. Thanks!
https://codereview.chromium.org/2815453002/diff/150001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuItemDelegate.java (right): https://codereview.chromium.org/2815453002/diff/150001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuItemDelegate.java:28: private final Activity mActivity; On 2017/05/17 15:40:38, Ted C wrote: > On 2017/05/15 21:47:04, ltian wrote: > > On 2017/05/12 23:35:26, Ted C wrote: > > > Activity isn't used > > > > It will be used for next CL when showing the toast and opening tab in the > > background. So I just leave it there for now. > > Toast doesn't need an activity. A Context is fine. Please remove. > > In general, try not to add things to changes that aren't used. Even if you plan > on adding it in a subsequent change. Changes should be entirely self contained. Still need to remove this https://codereview.chromium.org/2815453002/diff/170001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuHelper.java (right): https://codereview.chromium.org/2815453002/diff/170001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuHelper.java:104: if (items.isEmpty()) return menuItems; On 2017/05/17 19:21:47, ltian wrote: > On 2017/05/17 15:40:38, Ted C wrote: > > Can this ever happen? Is this for in the future if we support different > groups > > or something where only the custom options would be there? > > Yes, this line should not happen for current code. I follow JJ's implementation > for Chrome context menu and it might happen in the future if we support > different menus for different types of urls. I might just remove this check > since it does not happen now. per offline discussion, let's remove this line for now as it would require the additional mItems changes below and that complicates the code unnecessarily for now. https://codereview.chromium.org/2815453002/diff/190001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuHelper.java (right): https://codereview.chromium.org/2815453002/diff/190001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuHelper.java:153: view.setOnCreateContextMenuListener(BrowserActionsContextMenuHelper.this); I think we should add a assert view.getWindowToken() == null here. Otherwise if the view is already attached at this point, it would never show the context menu (since you wouldn't receive a onViewAttachedToWindow) https://codereview.chromium.org/2815453002/diff/190001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuHelper.java:174: view.removeOnAttachStateChangeListener(this); I think we should remove this unconditionally
Patchset #12 (id:210001) has been deleted
https://codereview.chromium.org/2815453002/diff/190001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuHelper.java (right): https://codereview.chromium.org/2815453002/diff/190001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuHelper.java:153: view.setOnCreateContextMenuListener(BrowserActionsContextMenuHelper.this); On 2017/05/17 19:31:53, Ted C wrote: > I think we should add a assert view.getWindowToken() == null here. Otherwise if > the view is already attached at this point, it would never show the context menu > (since you wouldn't receive a onViewAttachedToWindow) Done. https://codereview.chromium.org/2815453002/diff/190001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuHelper.java:174: view.removeOnAttachStateChangeListener(this); On 2017/05/17 19:31:53, Ted C wrote: > I think we should remove this unconditionally Done.
https://codereview.chromium.org/2815453002/diff/230001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuHelper.java (right): https://codereview.chromium.org/2815453002/diff/230001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuHelper.java:105: if (items.isEmpty()) return menuItems; remove https://codereview.chromium.org/2815453002/diff/230001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuItemDelegate.java (right): https://codereview.chromium.org/2815453002/diff/230001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuItemDelegate.java:28: private final Activity mActivity; remove
https://codereview.chromium.org/2815453002/diff/230001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuHelper.java (right): https://codereview.chromium.org/2815453002/diff/230001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuHelper.java:105: if (items.isEmpty()) return menuItems; On 2017/05/18 14:10:43, Ted C wrote: > remove Done. https://codereview.chromium.org/2815453002/diff/230001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuItemDelegate.java (right): https://codereview.chromium.org/2815453002/diff/230001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsContextMenuItemDelegate.java:28: private final Activity mActivity; On 2017/05/18 14:10:43, Ted C wrote: > remove Oh, I remember I remove this but looks like somehow I stash the change. Sorry for that.
lgtm
The CQ bit was checked by ltian@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mariakhomenko@chromium.org Link to the patchset: https://codereview.chromium.org/2815453002/#ps250001 (title: "Update based on Ted's comments.")
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
Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by ltian@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tedchoc@chromium.org, mariakhomenko@chromium.org Link to the patchset: https://codereview.chromium.org/2815453002/#ps270001 (title: "Rebase")
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
Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by ltian@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 ltian@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tedchoc@chromium.org, mariakhomenko@chromium.org Link to the patchset: https://codereview.chromium.org/2815453002/#ps290001 (title: "Fix tests fail.")
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": 290001, "attempt_start_ts": 1495178052568470,
"parent_rev": "d9f77a30d1fec84cb5dd1d7c225f089d9b735412", "commit_rev":
"fc5ed1f9afe1fee37119450781651e2fbae29f6c"}
Message was sent while issue was closed.
Description was changed from ========== [Android] Show Browser Actions dialog in Chrome Display Browser Actions dialog by refactoring Chrome context menu code. Add ContextMenuItemGetter interface to provide the capability to custom menu title and icon. Also add BrowserActionsContextMenuPopulator to match the menu id with custom behavior. BUG=710078 ========== to ========== [Android] Show Browser Actions dialog in Chrome Display Browser Actions dialog by refactoring Chrome context menu code. Add ContextMenuItemGetter interface to provide the capability to custom menu title and icon. Also add BrowserActionsContextMenuPopulator to match the menu id with custom behavior. BUG=710078 Review-Url: https://codereview.chromium.org/2815453002 Cr-Commit-Position: refs/heads/master@{#473124} Committed: https://chromium.googlesource.com/chromium/src/+/fc5ed1f9afe1fee3711945078165... ==========
Message was sent while issue was closed.
Committed patchset #15 (id:290001) as https://chromium.googlesource.com/chromium/src/+/fc5ed1f9afe1fee3711945078165... |
