Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2848453002:
dynamic context menu resizing (Closed)
Descriptiondynamic context menu resizing
added logic to dynamically adjust menu size after tab switches
BUG=655359
Review-Url: https://codereview.chromium.org/2848453002
Cr-Commit-Position: refs/heads/master@{#468724}
Committed: https://chromium.googlesource.com/chromium/src/+/8b24c49c7e35918e4aa21ba770e0e0bc54c8dd0c
Patch Set 1 #
Total comments: 9
Patch Set 2 : comments #Patch Set 3 : comments #Messages
Total messages: 13 (5 generated)
danielpark@chromium.org changed reviewers: + danielpark@chromium.org, dfalcantara@chromium.org, jwanda@chromium.org
https://codereview.chromium.org/2848453002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuViewPager.java (left): https://codereview.chromium.org/2848453002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuViewPager.java:40: Put this line back. It' a comment sandwich otherwise. https://codereview.chromium.org/2848453002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuViewPager.java (right): https://codereview.chromium.org/2848453002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuViewPager.java:36: //getCurrentItem() does not take into account the tab layout unlike getChildCount() space after // end with a period https://google.github.io/styleguide/javaguide.html#s4.8.6-comments https://google.github.io/styleguide/cppguide.html#Punctuation,_Spelling_and_G... Why is the TabLayout a child of the ViewPager instead of the parent? AIUI a ViewPager is supposed to just flip between two views. It shouldn't have any dangly Views that aren't flipped between because you end up in situations like this. https://codereview.chromium.org/2848453002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuViewPager.java:42: MeasureSpec.makeMeasureSpec(menuHeight, MeasureSpec.UNSPECIFIED)); makeMeasureSpec(0, MeasureSpec.UNSPECIFIED). Setting this to menuHeight makes no sense because it just tells the child to measure itself without caring about height (and it's 0 to start with anyway). https://codereview.chromium.org/2848453002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuViewPager.java:53: menuHeight += tabHeight; int fullHeight = menuHeight + tabHeight?
https://codereview.chromium.org/2848453002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuViewPager.java (right): https://codereview.chromium.org/2848453002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuViewPager.java:36: //getCurrentItem() does not take into account the tab layout unlike getChildCount() On 2017/04/27 18:14:14, slow (dfalcantara) wrote: > space after // > end with a period > > https://google.github.io/styleguide/javaguide.html#s4.8.6-comments > https://google.github.io/styleguide/cppguide.html#Punctuation,_Spelling_and_G... > > Why is the TabLayout a child of the ViewPager instead of the parent? AIUI a > ViewPager is supposed to just flip between two views. It shouldn't have any > dangly Views that aren't flipped between because you end up in situations like > this. Welp, that's required because that's how ViewPager works for some weird reason. https://developer.android.com/reference/android/support/design/widget/TabLayo...
https://codereview.chromium.org/2848453002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuViewPager.java (right): https://codereview.chromium.org/2848453002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuViewPager.java:36: //getCurrentItem() does not take into account the tab layout unlike getChildCount() On 2017/04/27 18:14:14, slow (dfalcantara) wrote: > space after // > end with a period > > https://google.github.io/styleguide/javaguide.html#s4.8.6-comments > https://google.github.io/styleguide/cppguide.html#Punctuation,_Spelling_and_G... > > Why is the TabLayout a child of the ViewPager instead of the parent? AIUI a > ViewPager is supposed to just flip between two views. It shouldn't have any > dangly Views that aren't flipped between because you end up in situations like > this. Done. https://codereview.chromium.org/2848453002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuViewPager.java:42: MeasureSpec.makeMeasureSpec(menuHeight, MeasureSpec.UNSPECIFIED)); On 2017/04/27 18:14:14, slow (dfalcantara) wrote: > makeMeasureSpec(0, MeasureSpec.UNSPECIFIED). Setting this to menuHeight makes > no sense because it just tells the child to measure itself without caring about > height (and it's 0 to start with anyway). Done. https://codereview.chromium.org/2848453002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuViewPager.java:53: menuHeight += tabHeight; On 2017/04/27 18:14:14, slow (dfalcantara) wrote: > int fullHeight = menuHeight + tabHeight? Done.
lgtm
https://codereview.chromium.org/2848453002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuViewPager.java (left): https://codereview.chromium.org/2848453002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuViewPager.java:40: On 2017/04/27 18:14:14, slow (dfalcantara) wrote: > Put this line back. It' a comment sandwich otherwise. Done.
The CQ bit was checked by danielpark@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dfalcantara@chromium.org Link to the patchset: https://codereview.chromium.org/2848453002/#ps40001 (title: "comments")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1493747887386840,
"parent_rev": "bf16ab86c8929ad72134db3d09bd3681e70bd9e9", "commit_rev":
"8b24c49c7e35918e4aa21ba770e0e0bc54c8dd0c"}
Message was sent while issue was closed.
Description was changed from ========== dynamic context menu resizing added logic to dynamically adjust menu size after tab switches BUG=655359 ========== to ========== dynamic context menu resizing added logic to dynamically adjust menu size after tab switches BUG=655359 Review-Url: https://codereview.chromium.org/2848453002 Cr-Commit-Position: refs/heads/master@{#468724} Committed: https://chromium.googlesource.com/chromium/src/+/8b24c49c7e35918e4aa21ba770e0... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/8b24c49c7e35918e4aa21ba770e0... |
