Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2868403003:
added scale animation for context menu (Closed)
Descriptionadded scale animation for context menu
BUG=655359, 730330
Review-Url: https://codereview.chromium.org/2868403003
Cr-Commit-Position: refs/heads/master@{#479100}
Committed: https://chromium.googlesource.com/chromium/src/+/facb458c362b51cbe79dc41b6a2680c8b76ea4c5
Patch Set 1 #Patch Set 2 : updates #Patch Set 3 : added animations to context menu #Patch Set 4 : put back deleted new lines #Patch Set 5 : changes to exit animation destination & duration #Patch Set 6 : switching context menu tab bug fix #
Total comments: 24
Patch Set 7 : bug fix #Patch Set 8 : bug fix #Patch Set 9 : made contextmenuwidth a local variable, renamed mContextMenuLocationOnScreen --> mContextMenuY... #Patch Set 10 : periods #
Total comments: 27
Patch Set 11 : y #
Total comments: 22
Patch Set 12 : comments #Patch Set 13 : refactoring #
Total comments: 20
Patch Set 14 : refactor #Patch Set 15 : comments #
Total comments: 13
Patch Set 16 : comments #Patch Set 17 : creating a new contextmenudialog constructor #
Total comments: 8
Patch Set 18 : comments #
Total comments: 29
Patch Set 19 : comments #Patch Set 20 : more renaming #Patch Set 21 : fixing resizing when rotating issue #Patch Set 22 : refactor #
Total comments: 1
Patch Set 23 : fixing issues brought because infrequent syncing" #
Total comments: 2
Patch Set 24 : refactor #Patch Set 25 : fixed broken tests #Patch Set 26 : fixing more tests #Patch Set 27 : fixing broken tests #Messages
Total messages: 61 (31 generated)
danielpark@chromium.org changed reviewers: + danielpark@chromium.org, ltian@chromium.org
danielpark@chromium.org changed reviewers: + dfalcantara@chromium.org, twellington@chromium.org - ltian@chromium.org
Will you please upload some videos of this animation on tablets and phones? https://codereview.chromium.org/2868403003/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuParams.java (right): https://codereview.chromium.org/2868403003/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuParams.java:140: public int getX() { Public methods need JavaDocs. It would be good to note whether this is dp or px and what the coordinates represent (e.g. coordinates are relative to rendered content). https://codereview.chromium.org/2868403003/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUi.java (right): https://codereview.chromium.org/2868403003/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUi.java:104: mPagerView.setVisibility(View.INVISIBLE); nit: I would move this visibility change and the layoutChangeListener before the call to mDialog.show() if possible. https://codereview.chromium.org/2868403003/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUi.java:110: if (mContextMenuHeight == -1) { Rather than relying on mContextMenuHeight == -1, let's remove this listener after the first layout pass. https://codereview.chromium.org/2868403003/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUi.java:126: @SuppressWarnings("deprecation") What method is deprecated? https://codereview.chromium.org/2868403003/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUi.java:153: mContextMenuHeight = -1; Each time the context menu is displayed, a new Dialog is created, correct? Rather than relying on mContextMenuHeight == -1 we should be able to remove the layout change listener after the first layout pass. https://codereview.chromium.org/2868403003/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUi.java:162: (int) Math.min(Resources.getSystem().getDisplayMetrics().widthPixels * 0.75, 1000); 0.75 should be defined as private static final MAX_WIDTH_PERCENT or something like that. Rather than hard-coding 1,000 pixels here, we should define a max width in dp for the context menu in dimens.xml and use Resources#getDimensionPixelSize() to retrieve the value in pixels. https://codereview.chromium.org/2868403003/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUi.java:190: .setBackgroundResource(R.drawable.white_with_rounded_corners); Let's set the background in tabular_context_menu.xml rather than programatically. https://codereview.chromium.org/2868403003/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUi.java:361: * @param startX The starting x-coordinate of the translation animation nit: add a period to the end of all param descriptions and return statements in JavaDocs. https://codereview.chromium.org/2868403003/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUi.java:373: float offset = mContentViewCore.getRenderCoordinates().getContentOffsetYPix(); nit: Add a comment about why this is necessary. If the device has android controls on the side (e.g. Nexus 6P in landscape), do we need to offset the X coordinate as well? https://codereview.chromium.org/2868403003/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUi.java:395: public void setContentViewCore(ContentViewCore contentViewCore) { nit: JavaDoc for this public method https://codereview.chromium.org/2868403003/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUi.java:406: Animation.ABSOLUTE, mContextMenuSourceX, Animation.ABSOLUTE, mContextMenuSourceY); On dismiss, is the context menu disappearing back into the touch point? I thought we had discussed it shrinking in the middle of the screen? https://codereview.chromium.org/2868403003/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUi.java:408: long duration = (long) (comingIn ? 250 : (250 * 0.6)); These int's should be defined as "private static final" variables.
https://codereview.chromium.org/2868403003/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuParams.java (right): https://codereview.chromium.org/2868403003/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuParams.java:140: public int getX() { On 2017/05/19 17:19:30, Theresa wrote: > Public methods need JavaDocs. It would be good to note whether this is dp or px > and what the coordinates represent (e.g. coordinates are relative to rendered > content). Done. https://codereview.chromium.org/2868403003/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUi.java (right): https://codereview.chromium.org/2868403003/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUi.java:104: mPagerView.setVisibility(View.INVISIBLE); On 2017/05/19 17:19:30, Theresa wrote: > nit: I would move this visibility change and the layoutChangeListener before the > call to mDialog.show() if possible. Done. https://codereview.chromium.org/2868403003/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUi.java:110: if (mContextMenuHeight == -1) { On 2017/05/19 17:19:30, Theresa wrote: > Rather than relying on mContextMenuHeight == -1, let's remove this listener > after the first layout pass. Done. https://codereview.chromium.org/2868403003/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUi.java:126: @SuppressWarnings("deprecation") On 2017/05/19 17:19:30, Theresa wrote: > What method is deprecated? Done. https://codereview.chromium.org/2868403003/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUi.java:153: mContextMenuHeight = -1; On 2017/05/19 17:19:30, Theresa wrote: > Each time the context menu is displayed, a new Dialog is created, correct? > > Rather than relying on mContextMenuHeight == -1 we should be able to remove the > layout change listener after the first layout pass. Done. https://codereview.chromium.org/2868403003/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUi.java:162: (int) Math.min(Resources.getSystem().getDisplayMetrics().widthPixels * 0.75, 1000); On 2017/05/19 17:19:30, Theresa wrote: > 0.75 should be defined as private static final MAX_WIDTH_PERCENT or something > like that. > > Rather than hard-coding 1,000 pixels here, we should define a max width in dp > for the context menu in dimens.xml and use Resources#getDimensionPixelSize() to > retrieve the value in pixels. Done. https://codereview.chromium.org/2868403003/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUi.java:190: .setBackgroundResource(R.drawable.white_with_rounded_corners); On 2017/05/19 17:19:30, Theresa wrote: > Let's set the background in tabular_context_menu.xml rather than > programatically. Done. https://codereview.chromium.org/2868403003/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUi.java:361: * @param startX The starting x-coordinate of the translation animation On 2017/05/19 17:19:30, Theresa wrote: > nit: add a period to the end of all param descriptions and return statements in > JavaDocs. Done. https://codereview.chromium.org/2868403003/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUi.java:373: float offset = mContentViewCore.getRenderCoordinates().getContentOffsetYPix(); On 2017/05/19 17:19:30, Theresa wrote: > nit: Add a comment about why this is necessary. > > If the device has android controls on the side (e.g. Nexus 6P in landscape), do > we need to offset the X coordinate as well? Done. https://codereview.chromium.org/2868403003/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUi.java:395: public void setContentViewCore(ContentViewCore contentViewCore) { On 2017/05/19 17:19:30, Theresa wrote: > nit: JavaDoc for this public method Done. https://codereview.chromium.org/2868403003/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUi.java:406: Animation.ABSOLUTE, mContextMenuSourceX, Animation.ABSOLUTE, mContextMenuSourceY); On 2017/05/19 17:19:30, Theresa wrote: > On dismiss, is the context menu disappearing back into the touch point? I > thought we had discussed it shrinking in the middle of the screen? The updated design asks for it to go back to the touch point to finish "telling the story" https://codereview.chromium.org/2868403003/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUi.java:408: long duration = (long) (comingIn ? 250 : (250 * 0.6)); On 2017/05/19 17:19:30, Theresa wrote: > These int's should be defined as "private static final" variables. Done.
https://codereview.chromium.org/2868403003/diff/170001/chrome/android/java/re... File chrome/android/java/res/values/dimens.xml (right): https://codereview.chromium.org/2868403003/diff/170001/chrome/android/java/re... chrome/android/java/res/values/dimens.xml:474: <dimen name="context_menu_max_width_in_dp">500dp</dimen> the "_in_dp" isn't really necessary. https://codereview.chromium.org/2868403003/diff/170001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuHelper.java (right): https://codereview.chromium.org/2868403003/diff/170001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuHelper.java:126: ContentViewCore.fromWebContents(contentViewCore.getWebContents())); ContentViewCore is going away, per the warning at the top of ContentViewCore.java (I should have caught this on the last pass, sorry), so new uses should not be added. Instead of passing ContentViewCore to TabularContextMenuUi, can we pass the RenderCoordinates? Also, as an aside, if we were going to pass in the ContentViewCore, you could have passed "contentViewCore" directory rather than using to get a WebContents than creating another CVC from WebContents. https://codereview.chromium.org/2868403003/diff/170001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuParams.java (right): https://codereview.chromium.org/2868403003/diff/170001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuParams.java:141: * @return The x-coordinate of the touch in dp relative to the content window; nit: of the touch that triggered the context menu... https://codereview.chromium.org/2868403003/diff/170001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUi.java (right): https://codereview.chromium.org/2868403003/diff/170001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUi.java:69: private static final double MAX_PROPORTION = 0.75; nit: MAX_WIDTH_PROPORTION https://codereview.chromium.org/2868403003/diff/170001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUi.java:116: return; nit: Add a comment about why we're returning early in this case https://codereview.chromium.org/2868403003/diff/170001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUi.java:140: dialog.findViewById(android.R.id.content).setOnClickListener(new OnClickListener() { The dialog can also be dismissed by a tap on the back button or a tap out-side of the content view. What happens in that case? It feels a bit odd to set an on click listener on the content view. I think that if you create a new ContextMenuDialog class that extends AlwaysDismissedDialog, you can override the dismiss() method to run the animation before calling super.dismiss(). https://codereview.chromium.org/2868403003/diff/170001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUi.java:201: View view = LayoutInflater.from(activity).inflate(R.layout.tabular_context_menu, null); Can tabular_context_menu be updated to include the full container view so that it doesn't need to be constructed programatically? Instaed of inflating here, you would inflate above, get the pager view using findViewById() and change this method name to initializePagerView() or something like that. https://codereview.chromium.org/2868403003/diff/170001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUi.java:358: * @param touchPointX The x-coordinate of the pixel where the touch was registered. nit: I think this would read better as "The x-coordinate of the touch that triggered the context menu in pixels." https://codereview.chromium.org/2868403003/diff/170001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUi.java:360: * @param activity Activity to get the offsets. Elaborate on what "offsets" means. https://codereview.chromium.org/2868403003/diff/170001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUi.java:372: mPagerView.getLocationOnScreen(locationOnScreen); Why is getLocationOnScreen() called again here? https://codereview.chromium.org/2868403003/diff/170001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUi.java:382: * @param activity activity needs a description https://codereview.chromium.org/2868403003/diff/170001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUi.java:384: * pivots set relative to the view because 0, 0 is relative to the view, not the device nit: because 0,0.... belongs in an in-line comment below rather than as part of the JavaDoc. It's an implementation detail that the caller of this method shouldn't need to know about. https://codereview.chromium.org/2868403003/diff/170001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUi.java:387: protected Animation initAnimations(final float touchPointX, final float touchPointY, nit: can this method be private?
https://codereview.chromium.org/2868403003/diff/170001/chrome/android/java/re... File chrome/android/java/res/values/dimens.xml (right): https://codereview.chromium.org/2868403003/diff/170001/chrome/android/java/re... chrome/android/java/res/values/dimens.xml:474: <dimen name="context_menu_max_width_in_dp">500dp</dimen> On 2017/05/23 01:58:07, Theresa wrote: > the "_in_dp" isn't really necessary. Done. https://codereview.chromium.org/2868403003/diff/170001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuHelper.java (right): https://codereview.chromium.org/2868403003/diff/170001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuHelper.java:126: ContentViewCore.fromWebContents(contentViewCore.getWebContents())); On 2017/05/23 01:58:07, Theresa wrote: > ContentViewCore is going away, per the warning at the top of > ContentViewCore.java (I should have caught this on the last pass, sorry), so new > uses should not be added. > > Instead of passing ContentViewCore to TabularContextMenuUi, can we pass the > RenderCoordinates? > > Also, as an aside, if we were going to pass in the ContentViewCore, you could > have passed "contentViewCore" directory rather than using to get a WebContents > than creating another CVC from WebContents. Done. https://codereview.chromium.org/2868403003/diff/170001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuParams.java (right): https://codereview.chromium.org/2868403003/diff/170001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuParams.java:141: * @return The x-coordinate of the touch in dp relative to the content window; On 2017/05/23 01:58:07, Theresa wrote: > nit: of the touch that triggered the context menu... Done. https://codereview.chromium.org/2868403003/diff/170001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUi.java (right): https://codereview.chromium.org/2868403003/diff/170001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUi.java:69: private static final double MAX_PROPORTION = 0.75; On 2017/05/23 01:58:08, Theresa wrote: > nit: MAX_WIDTH_PROPORTION Done. https://codereview.chromium.org/2868403003/diff/170001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUi.java:116: return; On 2017/05/23 01:58:08, Theresa wrote: > nit: Add a comment about why we're returning early in this case Done. https://codereview.chromium.org/2868403003/diff/170001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUi.java:140: dialog.findViewById(android.R.id.content).setOnClickListener(new OnClickListener() { On 2017/05/23 01:58:08, Theresa wrote: > The dialog can also be dismissed by a tap on the back button or a tap out-side > of the content view. What happens in that case? > > It feels a bit odd to set an on click listener on the content view. I think that > if you create a new ContextMenuDialog class that extends AlwaysDismissedDialog, > you can override the dismiss() method to run the animation before calling > super.dismiss(). Done. https://codereview.chromium.org/2868403003/diff/170001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUi.java:201: View view = LayoutInflater.from(activity).inflate(R.layout.tabular_context_menu, null); On 2017/05/23 01:58:08, Theresa wrote: > Can tabular_context_menu be updated to include the full container view so that > it doesn't need to be constructed programatically? Instaed of inflating here, > you would inflate above, get the pager view using findViewById() and change this > method name to initializePagerView() or something like that. Done. https://codereview.chromium.org/2868403003/diff/170001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUi.java:358: * @param touchPointX The x-coordinate of the pixel where the touch was registered. On 2017/05/23 01:58:07, Theresa wrote: > nit: I think this would read better as "The x-coordinate of the touch that > triggered the context menu in pixels." Done. https://codereview.chromium.org/2868403003/diff/170001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUi.java:360: * @param activity Activity to get the offsets. On 2017/05/23 01:58:08, Theresa wrote: > Elaborate on what "offsets" means. Done. https://codereview.chromium.org/2868403003/diff/170001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUi.java:372: mPagerView.getLocationOnScreen(locationOnScreen); On 2017/05/23 01:58:08, Theresa wrote: > Why is getLocationOnScreen() called again here? it was for debugging purposes that i forgot to take out. https://codereview.chromium.org/2868403003/diff/170001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUi.java:372: mPagerView.getLocationOnScreen(locationOnScreen); On 2017/05/23 01:58:08, Theresa wrote: > Why is getLocationOnScreen() called again here? Done. https://codereview.chromium.org/2868403003/diff/170001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUi.java:382: * @param activity On 2017/05/23 01:58:08, Theresa wrote: > activity needs a description Done. https://codereview.chromium.org/2868403003/diff/170001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUi.java:384: * pivots set relative to the view because 0, 0 is relative to the view, not the device On 2017/05/23 01:58:08, Theresa wrote: > nit: because 0,0.... belongs in an in-line comment below rather than as part of > the JavaDoc. It's an implementation detail that the caller of this method > shouldn't need to know about. Done. https://codereview.chromium.org/2868403003/diff/170001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUi.java:387: protected Animation initAnimations(final float touchPointX, final float touchPointY, On 2017/05/23 01:58:07, Theresa wrote: > nit: can this method be private? Done.
https://codereview.chromium.org/2868403003/diff/190001/chrome/android/java/re... File chrome/android/java/res/layout/tabular_context_menu.xml (right): https://codereview.chromium.org/2868403003/diff/190001/chrome/android/java/re... chrome/android/java/res/layout/tabular_context_menu.xml:24: app:tabTextColor="#80000000" nit: While not a change introduced in this CL, all color should be defined in colors.xml so that we can more easily track them. Also, #80000000 isn't in our recently formed color spec. #8A000000 (54% black), however, is, so we should use that instead. https://codereview.chromium.org/2868403003/diff/190001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuHelper.java (right): https://codereview.chromium.org/2868403003/diff/190001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuHelper.java:124: ((TabularContextMenuUi) menuUi) Instead of casting here, let's change line 118 to: final TabularContextMenuUi menuUi = ... https://codereview.chromium.org/2868403003/diff/190001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuParams.java (right): https://codereview.chromium.org/2868403003/diff/190001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuParams.java:144: public int getX() { nit: getTriggeringTouchX() or something else more descriptive. Whatever you pick, have mX and mY match. https://codereview.chromium.org/2868403003/diff/190001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUi.java (right): https://codereview.chromium.org/2868403003/diff/190001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUi.java:143: FrameLayout fullContainer = new FrameLayout(activity); Remove this and the line below since the FrameLayout is now defined in XML. https://codereview.chromium.org/2868403003/diff/190001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUi.java:155: dialog.addContentView(view.findViewById(R.id.context_menu_frame_layout), view should already be the FrameLayout associated with context_menu_frame_layout. From the LayoutInflater#inflate() documentation: Returns the "root View of the inflated hierarchy. If root was supplied, this is the root View; otherwise it is the root of the inflated XML file." https://codereview.chromium.org/2868403003/diff/190001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUi.java:167: * tab. Did git cl format do this? Ideally this line would be aligned with "The list..." on the line above. https://codereview.chromium.org/2868403003/diff/190001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUi.java:173: List<Pair<Integer, List<ContextMenuItem>>> itemGroups, View view) { Pass TabularContextMenuViewPager (and cast in the call to initPagerView) here rather than casting it below. https://codereview.chromium.org/2868403003/diff/190001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUi.java:331: * window. nit: indent this to align with "Activity to..." https://codereview.chromium.org/2868403003/diff/190001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUi.java:407: private Animation getScaleAnimationIn() { Ideally ContextMenuDialog would own both animations (out and in). Can this be moved to the new ContextMenuDialog class? https://codereview.chromium.org/2868403003/diff/190001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/widget/ContextMenuDialog.java (right): https://codereview.chromium.org/2868403003/diff/190001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/widget/ContextMenuDialog.java:25: // TabularContextMenuUi.java Comments like this spread across files are likely to be missed if someone is changing ANIMATION_IN_DURATION in the other file sometime in the future. https://codereview.chromium.org/2868403003/diff/190001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/widget/ContextMenuDialog.java:42: Animation exitAnimation = getAnimationOut(); nit: rename method to getExitAnimation() for more consistency between variables and method names. Similar, getScaleAnimationIn() and initializeAnimationAndRevealView() and initAnimations() should be named consistently with respect to each other and this file.
https://codereview.chromium.org/2868403003/diff/190001/chrome/android/java/re... File chrome/android/java/res/layout/tabular_context_menu.xml (right): https://codereview.chromium.org/2868403003/diff/190001/chrome/android/java/re... chrome/android/java/res/layout/tabular_context_menu.xml:24: app:tabTextColor="#80000000" On 2017/05/24 17:53:33, Theresa wrote: > nit: While not a change introduced in this CL, all color should be defined in > colors.xml so that we can more easily track them. Also, > #80000000 isn't in our recently formed color spec. #8A000000 (54% black), > however, is, so we should use that instead. Done. https://codereview.chromium.org/2868403003/diff/190001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuHelper.java (right): https://codereview.chromium.org/2868403003/diff/190001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuHelper.java:124: ((TabularContextMenuUi) menuUi) On 2017/05/24 17:53:33, Theresa wrote: > Instead of casting here, let's change line 118 to: > > final TabularContextMenuUi menuUi = ... Done. https://codereview.chromium.org/2868403003/diff/190001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuParams.java (right): https://codereview.chromium.org/2868403003/diff/190001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuParams.java:144: public int getX() { On 2017/05/24 17:53:33, Theresa (OOO) wrote: > nit: getTriggeringTouchX() or something else more descriptive. Whatever you > pick, have mX and mY match. Done. https://codereview.chromium.org/2868403003/diff/190001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUi.java (right): https://codereview.chromium.org/2868403003/diff/190001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUi.java:143: FrameLayout fullContainer = new FrameLayout(activity); On 2017/05/24 17:53:33, Theresa wrote: > Remove this and the line below since the FrameLayout is now defined in XML. Done. https://codereview.chromium.org/2868403003/diff/190001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUi.java:155: dialog.addContentView(view.findViewById(R.id.context_menu_frame_layout), On 2017/05/24 17:53:34, Theresa wrote: > view should already be the FrameLayout associated with > context_menu_frame_layout. > > From the LayoutInflater#inflate() documentation: > Returns the "root View of the inflated hierarchy. If root was supplied, this is > the root View; otherwise it is the root of the inflated XML file." Done. https://codereview.chromium.org/2868403003/diff/190001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUi.java:167: * tab. On 2017/05/24 17:53:34, Theresa wrote: > Did git cl format do this? Ideally this line would be aligned with "The list..." > on the line above. Done. https://codereview.chromium.org/2868403003/diff/190001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUi.java:173: List<Pair<Integer, List<ContextMenuItem>>> itemGroups, View view) { On 2017/05/24 17:53:33, Theresa wrote: > Pass TabularContextMenuViewPager (and cast in the call to initPagerView) here > rather than casting it below. Done. https://codereview.chromium.org/2868403003/diff/190001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUi.java:331: * window. On 2017/05/24 17:53:33, Theresa wrote: > nit: indent this to align with "Activity to..." Done. https://codereview.chromium.org/2868403003/diff/190001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUi.java:407: private Animation getScaleAnimationIn() { On 2017/05/24 17:53:33, Theresa wrote: > Ideally ContextMenuDialog would own both animations (out and in). Can this be > moved to the new ContextMenuDialog class? Done. https://codereview.chromium.org/2868403003/diff/190001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/widget/ContextMenuDialog.java (right): https://codereview.chromium.org/2868403003/diff/190001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/widget/ContextMenuDialog.java:25: // TabularContextMenuUi.java On 2017/05/24 17:53:34, Theresa wrote: > Comments like this spread across files are likely to be missed if someone is > changing ANIMATION_IN_DURATION in the other file sometime in the future. Done. https://codereview.chromium.org/2868403003/diff/190001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/widget/ContextMenuDialog.java:42: Animation exitAnimation = getAnimationOut(); On 2017/05/24 17:53:34, Theresa wrote: > nit: rename method to getExitAnimation() for more consistency between variables > and method names. Similar, getScaleAnimationIn() and > initializeAnimationAndRevealView() and initAnimations() should be named > consistently with respect to each other and this file. Done.
https://codereview.chromium.org/2868403003/diff/230001/chrome/android/java/re... File chrome/android/java/res/layout/tabular_context_menu.xml (right): https://codereview.chromium.org/2868403003/diff/230001/chrome/android/java/re... chrome/android/java/res/layout/tabular_context_menu.xml:29: app:tabMaxWidth="2000dp" /> nit: add a comment explaining why a max width is needed here. https://codereview.chromium.org/2868403003/diff/230001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuParams.java (right): https://codereview.chromium.org/2868403003/diff/230001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuParams.java:150: * content window; 0 corresponds to the left edge. nit: s/content window/render view to match native documentation? https://codereview.chromium.org/2868403003/diff/230001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUi.java (right): https://codereview.chromium.org/2868403003/diff/230001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUi.java:61: private static final double MAX_WIDTH_PROPORTION = 0.75; nit: private static final variables should go above regular private variables https://codereview.chromium.org/2868403003/diff/230001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUi.java:145: dialog.addContentView( setContentView() instead of addContentView() since there should only ever be one content view attached to the dialog? Also, does this work without the constructed layout params if you set android:layout_with and layout_height on the root view in the inflated XML? https://codereview.chromium.org/2868403003/diff/230001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUi.java:343: * window. nit: align "window" with "Activity" https://codereview.chromium.org/2868403003/diff/230001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUi.java:346: private Animation initPivotValuesAndGetEnterScaleAnimation(final float touchPointX, nit: this is probably more verbose than needed. I would just call it getEnterScaleAnimation() https://codereview.chromium.org/2868403003/diff/230001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUi.java:369: I was hoping all of this could move to ContextMenuDialog as well. In a perfect world, this class would just call dialog#show() and ContextMenuDialog would handle creating the scale animation and starting it. If that's not possible (because of the layout listeners needed), then we could move this method to ContextMenuDialog and change the layout listener to: public void onLayoutChange(View v, int left, int top, int right, int bottom, int oldLeft, int oldTop, int oldRight, int oldBottom) { .... mContextMenuDialog.startEnterAnimation() mPagerView.removeOnLayoutChangeListener(this); } https://codereview.chromium.org/2868403003/diff/230001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUi.java:390: * Gives this class access to the content view core to allow access to the total size of the This comment needs to be updated https://codereview.chromium.org/2868403003/diff/230001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/widget/ContextMenuDialog.java (right): https://codereview.chromium.org/2868403003/diff/230001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/widget/ContextMenuDialog.java:16: * AlwaysDismissedDialog that ensures that the dialog animates when it's being dismissed. nit: The class comment and class name don't really match. I think the comment should talk about the context menu. https://codereview.chromium.org/2868403003/diff/230001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/widget/ContextMenuDialog.java:32: public void setExitAnimationParameters(View pagerView, float contextMenuDestinationX, nit: JavaDocs for this. Also, does it matter that this is the pagerView or should we call it contentView or something else more generic?
https://codereview.chromium.org/2868403003/diff/230001/chrome/android/java/re... File chrome/android/java/res/layout/tabular_context_menu.xml (right): https://codereview.chromium.org/2868403003/diff/230001/chrome/android/java/re... chrome/android/java/res/layout/tabular_context_menu.xml:29: app:tabMaxWidth="2000dp" /> On 2017/05/24 20:16:32, Theresa (OOO) wrote: > nit: add a comment explaining why a max width is needed here. Done. https://codereview.chromium.org/2868403003/diff/230001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuParams.java (right): https://codereview.chromium.org/2868403003/diff/230001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuParams.java:150: * content window; 0 corresponds to the left edge. On 2017/05/24 20:16:33, Theresa (OOO) wrote: > nit: s/content window/render view to match native documentation? Done. https://codereview.chromium.org/2868403003/diff/230001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUi.java (right): https://codereview.chromium.org/2868403003/diff/230001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUi.java:61: private static final double MAX_WIDTH_PROPORTION = 0.75; On 2017/05/24 20:16:33, Theresa (OOO) wrote: > nit: private static final variables should go above regular private variables Done. https://codereview.chromium.org/2868403003/diff/230001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUi.java:145: dialog.addContentView( On 2017/05/24 20:16:33, Theresa (OOO) wrote: > setContentView() instead of addContentView() since there should only ever be one > content view attached to the dialog? > > Also, does this work without the constructed layout params if you set > android:layout_with and layout_height on the root view in the inflated XML? Done. https://codereview.chromium.org/2868403003/diff/230001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUi.java:343: * window. On 2017/05/24 20:16:33, Theresa (OOO) wrote: > nit: align "window" with "Activity" Done. https://codereview.chromium.org/2868403003/diff/230001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUi.java:346: private Animation initPivotValuesAndGetEnterScaleAnimation(final float touchPointX, On 2017/05/24 20:16:33, Theresa (OOO) wrote: > nit: this is probably more verbose than needed. I would just call it > getEnterScaleAnimation() Done. https://codereview.chromium.org/2868403003/diff/230001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUi.java:369: On 2017/05/24 20:16:33, Theresa (OOO) wrote: > I was hoping all of this could move to ContextMenuDialog as well. In a perfect > world, this class would just call dialog#show() and ContextMenuDialog would > handle creating the scale animation and starting it. > > > If that's not possible (because of the layout listeners needed), then we could > move this method to ContextMenuDialog and change the layout listener to: > > public void onLayoutChange(View v, int left, int top, int right, int bottom, > int oldLeft, int oldTop, int oldRight, int oldBottom) { > > .... > mContextMenuDialog.startEnterAnimation() > mPagerView.removeOnLayoutChangeListener(this); > } Done. https://codereview.chromium.org/2868403003/diff/230001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUi.java:390: * Gives this class access to the content view core to allow access to the total size of the On 2017/05/24 20:16:33, Theresa (OOO) wrote: > This comment needs to be updated Done. https://codereview.chromium.org/2868403003/diff/230001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/widget/ContextMenuDialog.java (right): https://codereview.chromium.org/2868403003/diff/230001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/widget/ContextMenuDialog.java:16: * AlwaysDismissedDialog that ensures that the dialog animates when it's being dismissed. On 2017/05/24 20:16:33, Theresa (OOO) wrote: > nit: The class comment and class name don't really match. I think the comment > should talk about the context menu. Done. https://codereview.chromium.org/2868403003/diff/230001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/widget/ContextMenuDialog.java:32: public void setExitAnimationParameters(View pagerView, float contextMenuDestinationX, On 2017/05/24 20:16:33, Theresa (OOO) wrote: > nit: JavaDocs for this. Also, does it matter that this is the pagerView or > should we call it contentView or something else more generic? Done.
This is getting pretty close! https://codereview.chromium.org/2868403003/diff/270001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUi.java (right): https://codereview.chromium.org/2868403003/diff/270001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUi.java:83: dialogWindow.setLayout(LayoutParams.MATCH_PARENT, LayoutParams.MATCH_PARENT); Can contextMenuDialog be responsible for this as well, since it's part of the requirements for running the desired animation? https://codereview.chromium.org/2868403003/diff/270001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUi.java:89: mPagerView.setVisibility(View.INVISIBLE); nit: let's set the view to invisible in ContextMenuDialog's method. https://codereview.chromium.org/2868403003/diff/270001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/widget/ContextMenuDialog.java (right): https://codereview.chromium.org/2868403003/diff/270001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/widget/ContextMenuDialog.java:48: public void setupEnterAnimationAndShow(final View contentView, final float touchPointX, How about an init() methods that takes all of these params or an updated constructor that takes all of the parameters (constructor is my preference if we know all of these values at construction time). If we use a separate init() method then we probably don't need to pass activity again since it's passed in the constructor (unless it's changed). Then we could override show(), which feels more in-line with our approach of overriding dismiss() below. https://codereview.chromium.org/2868403003/diff/270001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/widget/ContextMenuDialog.java:103: private Animation getStartAnimation(float touchPointX, float touchPointY, I would combine this with startEnterAnimation(). The split between the two methods feels a bit arbitrary. https://codereview.chromium.org/2868403003/diff/270001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/widget/ContextMenuDialog.java:148: this.dismiss(); Just to confirm onTouchEvent() doesn't get called if TabularContextMenu#onItemClick() is called when an item is clicked? https://codereview.chromium.org/2868403003/diff/270001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/widget/ContextMenuDialog.java:155: * or not. nit: "Whether animation is for showing the context menu as opposed to hiding it."? show/hide is more common in our code base than enter/exit for displaying UI.
https://codereview.chromium.org/2868403003/diff/270001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUi.java (right): https://codereview.chromium.org/2868403003/diff/270001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUi.java:83: dialogWindow.setLayout(LayoutParams.MATCH_PARENT, LayoutParams.MATCH_PARENT); On 2017/05/24 22:50:50, Theresa (OOO) wrote: > Can contextMenuDialog be responsible for this as well, since it's part of the > requirements for running the desired animation? Done. https://codereview.chromium.org/2868403003/diff/270001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUi.java:89: mPagerView.setVisibility(View.INVISIBLE); On 2017/05/24 22:50:50, Theresa (OOO) wrote: > nit: let's set the view to invisible in ContextMenuDialog's method. Done. https://codereview.chromium.org/2868403003/diff/270001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/widget/ContextMenuDialog.java (right): https://codereview.chromium.org/2868403003/diff/270001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/widget/ContextMenuDialog.java:48: public void setupEnterAnimationAndShow(final View contentView, final float touchPointX, On 2017/05/24 22:50:50, Theresa (OOO) wrote: > How about an init() methods that takes all of these params or an updated > constructor that takes all of the parameters (constructor is my preference if we > know all of these values at construction time). If we use a separate init() > method then we probably don't need to pass activity again since it's passed in > the constructor (unless it's changed). > > Then we could override show(), which feels more in-line with our approach of > overriding dismiss() below. Done. https://codereview.chromium.org/2868403003/diff/270001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/widget/ContextMenuDialog.java:103: private Animation getStartAnimation(float touchPointX, float touchPointY, On 2017/05/24 22:50:50, Theresa (OOO) wrote: > I would combine this with startEnterAnimation(). The split between the two > methods feels a bit arbitrary. Done. https://codereview.chromium.org/2868403003/diff/270001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/widget/ContextMenuDialog.java:148: this.dismiss(); On 2017/05/24 22:50:50, Theresa (OOO) wrote: > Just to confirm onTouchEvent() doesn't get called if > TabularContextMenu#onItemClick() is called when an item is clicked? from testing, it does get called when an item is clicked. the exit animation and the item event happen simultaneously https://codereview.chromium.org/2868403003/diff/270001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/widget/ContextMenuDialog.java:148: this.dismiss(); On 2017/05/24 22:50:50, Theresa (OOO) wrote: > Just to confirm onTouchEvent() doesn't get called if > TabularContextMenu#onItemClick() is called when an item is clicked? Done. https://codereview.chromium.org/2868403003/diff/270001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/widget/ContextMenuDialog.java:155: * or not. On 2017/05/24 22:50:50, Theresa (OOO) wrote: > nit: "Whether animation is for showing the context menu as opposed to hiding > it."? > > show/hide is more common in our code base than enter/exit for displaying UI. Done.
lgtm % a few comments https://codereview.chromium.org/2868403003/diff/310001/chrome/android/java/re... File chrome/android/java/res/layout/tabular_context_menu.xml (right): https://codereview.chromium.org/2868403003/diff/310001/chrome/android/java/re... chrome/android/java/res/layout/tabular_context_menu.xml:10: <org.chromium.chrome.browser.contextmenu.TabularContextMenuViewPager nit: add a blank line before this line and above the comment on line 19. https://codereview.chromium.org/2868403003/diff/310001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUi.java (right): https://codereview.chromium.org/2868403003/diff/310001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUi.java:126: * @param viewPager The viewpager to initialize. nit: s/viewpager/{@link TabularContextMenuViewPager} https://codereview.chromium.org/2868403003/diff/310001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/widget/ContextMenuDialog.java (right): https://codereview.chromium.org/2868403003/diff/310001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/widget/ContextMenuDialog.java:26: * animation is played upon calling dismiss(). nit: ... upon calling {@link #show()} and {@link #dismiss()}. https://codereview.chromium.org/2868403003/diff/310001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/widget/ContextMenuDialog.java:134: return true; This method should return true if the event was handled and false otherwise, so this sould probably only return true if the MotionEvent is ACTION_DOWN.
https://codereview.chromium.org/2868403003/diff/310001/chrome/android/java/re... File chrome/android/java/res/layout/tabular_context_menu.xml (right): https://codereview.chromium.org/2868403003/diff/310001/chrome/android/java/re... chrome/android/java/res/layout/tabular_context_menu.xml:10: <org.chromium.chrome.browser.contextmenu.TabularContextMenuViewPager On 2017/05/30 16:16:07, Theresa wrote: > nit: add a blank line before this line and above the comment on line 19. Done. https://codereview.chromium.org/2868403003/diff/310001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUi.java (right): https://codereview.chromium.org/2868403003/diff/310001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUi.java:126: * @param viewPager The viewpager to initialize. On 2017/05/30 16:16:07, Theresa wrote: > nit: s/viewpager/{@link TabularContextMenuViewPager} Done. https://codereview.chromium.org/2868403003/diff/310001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/widget/ContextMenuDialog.java (right): https://codereview.chromium.org/2868403003/diff/310001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/widget/ContextMenuDialog.java:26: * animation is played upon calling dismiss(). On 2017/05/30 16:16:07, Theresa wrote: > nit: ... upon calling {@link #show()} and {@link #dismiss()}. Done. https://codereview.chromium.org/2868403003/diff/310001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/widget/ContextMenuDialog.java:134: return true; On 2017/05/30 16:16:07, Theresa wrote: > This method should return true if the event was handled and false otherwise, so > this sould probably only return true if the MotionEvent is ACTION_DOWN. Done.
The CQ bit was checked by danielpark@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from twellington@chromium.org Link to the patchset: https://codereview.chromium.org/2868403003/#ps330001 (title: "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 danielpark@chromium.org
danielpark@chromium.org changed reviewers: + tedchoc@chromium.org
+tedchoc@ for OWNERS
most style and naming nits...overall looks good https://codereview.chromium.org/2868403003/diff/330001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuParams.java (right): https://codereview.chromium.org/2868403003/diff/330001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuParams.java:144: public int getTriggeringTouchX() { I would put a Dp suffix here and below https://codereview.chromium.org/2868403003/diff/330001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUi.java (right): https://codereview.chromium.org/2868403003/diff/330001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUi.java:92: * @param touchPointY The x-coordinate of the touch that triggered the context menu in pixels. I would just put a suffix of Px and then you can drop "in pixels" in the comment https://codereview.chromium.org/2868403003/diff/330001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUi.java:105: (int) Math.min(resources.getDisplayMetrics().widthPixels * MAX_WIDTH_PROPORTION, Does widthPixels change if you rotate the phone? If you show the dialog in landscape and rotate, does this need to be updated? For the narrowest of phones, I'm a bit worried that 75% might be too limiting. I wonder if we should instead have a min padding from the edge (I was thinking something like 10-15 dp). Although if this is what UX decided on, then that is ok. We should just try on our smallest phone to make sure that doesn't hurt anything (or try multi-window in landscape on a phone) https://codereview.chromium.org/2868403003/diff/330001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/widget/ContextMenuDialog.java (right): https://codereview.chromium.org/2868403003/diff/330001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/widget/ContextMenuDialog.java:29: private static final int ENTER_ANIMATION_DURATION = 250; suffix of _MS to indicate milliseconds nit, I think we also normally define these as "long"s as that is usually what animations take. https://codereview.chromium.org/2868403003/diff/330001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/widget/ContextMenuDialog.java:36: private float mContextMenuSourceX; same comment about the suffixes of Px or Dp https://codereview.chromium.org/2868403003/diff/330001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/widget/ContextMenuDialog.java:43: public ContextMenuDialog(Activity ownerActivity, int theme, float touchPointX, javadoc https://codereview.chromium.org/2868403003/diff/330001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/widget/ContextMenuDialog.java:50: mRenderCoordinates = renderCoordinates; looks like many if not all of these could also be final above? https://codereview.chromium.org/2868403003/diff/330001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/widget/ContextMenuDialog.java:55: Window dialogWindow = this.getWindow(); no need for this. https://codereview.chromium.org/2868403003/diff/330001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/widget/ContextMenuDialog.java:61: remove blank line https://codereview.chromium.org/2868403003/diff/330001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/widget/ContextMenuDialog.java:67: if (group.getChildAt(i).getHeight() == 0 I think you can use getMeasuredHeight() == 0 here as that is set earlier than getHeight() https://codereview.chromium.org/2868403003/diff/330001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/widget/ContextMenuDialog.java:91: mContentView.getLocationOnScreen(currentLocationOnScreen); you should try this on a phone on a sight with theme colors (facebook.com for example). Then rotate in all directions and make sure your starting point is correct. Sometimes this will include the android nav bars and not, so it would be good to ensure this is correct for you always. https://codereview.chromium.org/2868403003/diff/330001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/widget/ContextMenuDialog.java:114: remove blank line https://codereview.chromium.org/2868403003/diff/330001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/widget/ContextMenuDialog.java:132: this.dismiss(); can drop "this."
https://codereview.chromium.org/2868403003/diff/330001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuParams.java (right): https://codereview.chromium.org/2868403003/diff/330001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuParams.java:144: public int getTriggeringTouchX() { On 2017/05/30 23:04:04, Ted C wrote: > I would put a Dp suffix here and below Done. https://codereview.chromium.org/2868403003/diff/330001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUi.java (right): https://codereview.chromium.org/2868403003/diff/330001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUi.java:92: * @param touchPointY The x-coordinate of the touch that triggered the context menu in pixels. On 2017/05/30 23:04:04, Ted C wrote: > I would just put a suffix of Px and then you can drop "in pixels" in the comment Done. https://codereview.chromium.org/2868403003/diff/330001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUi.java:105: (int) Math.min(resources.getDisplayMetrics().widthPixels * MAX_WIDTH_PROPORTION, On 2017/05/30 23:04:04, Ted C wrote: > Does widthPixels change if you rotate the phone? If you show the dialog in > landscape and rotate, does this need to be updated? > > For the narrowest of phones, I'm a bit worried that 75% might be too limiting. > I wonder if we should instead have a min padding from the edge (I was thinking > something like 10-15 dp). Although if this is what UX decided on, then that is > ok. We should just try on our smallest phone to make sure that doesn't hurt > anything (or try multi-window in landscape on a phone) Acknowledged. https://codereview.chromium.org/2868403003/diff/330001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUi.java:105: (int) Math.min(resources.getDisplayMetrics().widthPixels * MAX_WIDTH_PROPORTION, On 2017/05/30 23:04:04, Ted C wrote: > Does widthPixels change if you rotate the phone? If you show the dialog in > landscape and rotate, does this need to be updated? > > For the narrowest of phones, I'm a bit worried that 75% might be too limiting. > I wonder if we should instead have a min padding from the edge (I was thinking > something like 10-15 dp). Although if this is what UX decided on, then that is > ok. We should just try on our smallest phone to make sure that doesn't hurt > anything (or try multi-window in landscape on a phone) There's an issue where rotating the screen from landscape to portrait will cause the context menu to be wider than the phone's width. I'll fix that & change it to the minimum padding in a future cl. https://codereview.chromium.org/2868403003/diff/330001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/widget/ContextMenuDialog.java (right): https://codereview.chromium.org/2868403003/diff/330001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/widget/ContextMenuDialog.java:29: private static final int ENTER_ANIMATION_DURATION = 250; On 2017/05/30 23:04:04, Ted C wrote: > suffix of _MS to indicate milliseconds > > nit, I think we also normally define these as "long"s as that is usually what > animations take. Done. https://codereview.chromium.org/2868403003/diff/330001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/widget/ContextMenuDialog.java:36: private float mContextMenuSourceX; On 2017/05/30 23:04:04, Ted C wrote: > same comment about the suffixes of Px or Dp Done. https://codereview.chromium.org/2868403003/diff/330001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/widget/ContextMenuDialog.java:43: public ContextMenuDialog(Activity ownerActivity, int theme, float touchPointX, On 2017/05/30 23:04:04, Ted C wrote: > javadoc Done. https://codereview.chromium.org/2868403003/diff/330001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/widget/ContextMenuDialog.java:50: mRenderCoordinates = renderCoordinates; On 2017/05/30 23:04:04, Ted C wrote: > looks like many if not all of these could also be final above? Done. https://codereview.chromium.org/2868403003/diff/330001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/widget/ContextMenuDialog.java:55: Window dialogWindow = this.getWindow(); On 2017/05/30 23:04:04, Ted C wrote: > no need for this. Done. https://codereview.chromium.org/2868403003/diff/330001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/widget/ContextMenuDialog.java:61: On 2017/05/30 23:04:04, Ted C wrote: > remove blank line Done. https://codereview.chromium.org/2868403003/diff/330001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/widget/ContextMenuDialog.java:67: if (group.getChildAt(i).getHeight() == 0 On 2017/05/30 23:04:04, Ted C wrote: > I think you can use getMeasuredHeight() == 0 here as that is set earlier than > getHeight() Done. https://codereview.chromium.org/2868403003/diff/330001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/widget/ContextMenuDialog.java:91: mContentView.getLocationOnScreen(currentLocationOnScreen); On 2017/05/30 23:04:04, Ted C wrote: > you should try this on a phone on a sight with theme colors (http://facebook.com for > example). Then rotate in all directions and make sure your starting point is > correct. Sometimes this will include the android nav bars and not, so it would > be good to ensure this is correct for you always. Done. https://codereview.chromium.org/2868403003/diff/330001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/widget/ContextMenuDialog.java:91: mContentView.getLocationOnScreen(currentLocationOnScreen); On 2017/05/30 23:04:04, Ted C wrote: > you should try this on a phone on a sight with theme colors (http://facebook.com for > example). Then rotate in all directions and make sure your starting point is > correct. Sometimes this will include the android nav bars and not, so it would > be good to ensure this is correct for you always. the starting points are all correct. https://codereview.chromium.org/2868403003/diff/330001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/widget/ContextMenuDialog.java:114: On 2017/05/30 23:04:05, Ted C wrote: > remove blank line Done. https://codereview.chromium.org/2868403003/diff/330001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/widget/ContextMenuDialog.java:132: this.dismiss(); On 2017/05/30 23:04:04, Ted C wrote: > can drop "this." Done.
lgtm w/ min-padding change https://codereview.chromium.org/2868403003/diff/330001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUi.java (right): https://codereview.chromium.org/2868403003/diff/330001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUi.java:105: (int) Math.min(resources.getDisplayMetrics().widthPixels * MAX_WIDTH_PROPORTION, On 2017/05/31 18:19:39, Daniel Park wrote: > On 2017/05/30 23:04:04, Ted C wrote: > > Does widthPixels change if you rotate the phone? If you show the dialog in > > landscape and rotate, does this need to be updated? > > > > For the narrowest of phones, I'm a bit worried that 75% might be too limiting. > > > I wonder if we should instead have a min padding from the edge (I was thinking > > something like 10-15 dp). Although if this is what UX decided on, then that > is > > ok. We should just try on our smallest phone to make sure that doesn't hurt > > anything (or try multi-window in landscape on a phone) > > There's an issue where rotating the screen from landscape to portrait will cause > the context menu to be wider than the phone's width. > I'll fix that & change it to the minimum padding in a future cl. As discussed offline, that is ok in this context as the UI is behind a flag, but normally it would need to be fixed here. Let's file a bug and add a TODO here: // TODO(injae): Fix sizing on orientation changes (crbug.com/XXXXX). As for the min sizing, I think we should just go ahead and change that in this CL. Instead of doing MAX_WIDTH_PROPORTION, let's define a new dimen in xml (I'd say 20dp) called context_menu_min_side_padding and do widthPixels - 2 * min padding. I think if it isn't wide enough, we should take up more than 75% of the screen. That is likely a small enough change to warrant doing it here instead of filing a separate bug.
Description was changed from ========== added scale animation for context menu BUG=655359 ========== to ========== added scale animation for context menu BUG=655359, 730330 ==========
lgtm w/ nit (we discussed moving the width logic to a followup CL, so we can land this as is) https://codereview.chromium.org/2868403003/diff/410001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUi.java (right): https://codereview.chromium.org/2868403003/diff/410001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUi.java:42: private static final double MAX_WIDTH_PROPORTION = 0.75; remove as this isn't used in this file anymore.
https://codereview.chromium.org/2868403003/diff/430001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionActivity.java (right): https://codereview.chromium.org/2868403003/diff/430001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionActivity.java:104: referrer, false /* canSaveMedia */, 0 /* touchPointXDp */, 0 /* touchPointYDp */); Currently the only call to touchPointDp are from the context menu, so setting both of these to 0 shouldn't cause any issues. https://codereview.chromium.org/2868403003/diff/430001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuParams.java (right): https://codereview.chromium.org/2868403003/diff/430001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuParams.java:157: public ContextMenuParams(int mediaType, String pageUrl, String linkUrl, String linkText, this isn't a new change since this is already marked public in code search.
The CQ bit was checked by danielpark@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_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
The CQ bit was checked by danielpark@chromium.org to run a CQ dry run
danielpark@chromium.org changed reviewers: + ltian@chromium.org - dfalcantara@chromium.org, tedchoc@chromium.org, twellington@chromium.org
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_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 danielpark@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: 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 danielpark@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from twellington@chromium.org, tedchoc@chromium.org Link to the patchset: https://codereview.chromium.org/2868403003/#ps490001 (title: "fixing more tests")
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: 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 danielpark@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from twellington@chromium.org, tedchoc@chromium.org Link to the patchset: https://codereview.chromium.org/2868403003/#ps510001 (title: "fixing broken tests")
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...) android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) android_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...) android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by danielpark@chromium.org
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": 510001, "attempt_start_ts": 1497375413892460,
"parent_rev": "115c560f5886afd3c512d054c988439893974f24", "commit_rev":
"536ecafa38b40b0760a47b5644629b0564552a46"}
CQ is committing da patch.
Bot data: {"patchset_id": 510001, "attempt_start_ts": 1497375413892460,
"parent_rev": "4abd1664c3858b0bccf0520871dd2bb86fcad0a2", "commit_rev":
"9b262e1c8d20ffd01f060e2cca19af73f8a8e759"}
CQ is committing da patch.
Bot data: {"patchset_id": 510001, "attempt_start_ts": 1497375413892460,
"parent_rev": "9391176848bb7c6e5822a05f73592ed7c3ff8fbb", "commit_rev":
"facb458c362b51cbe79dc41b6a2680c8b76ea4c5"}
Message was sent while issue was closed.
Description was changed from ========== added scale animation for context menu BUG=655359, 730330 ========== to ========== added scale animation for context menu BUG=655359, 730330 Review-Url: https://codereview.chromium.org/2868403003 Cr-Commit-Position: refs/heads/master@{#479100} Committed: https://chromium.googlesource.com/chromium/src/+/facb458c362b51cbe79dc41b6a26... ==========
Message was sent while issue was closed.
Committed patchset #27 (id:510001) as https://chromium.googlesource.com/chromium/src/+/facb458c362b51cbe79dc41b6a26... |
