Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2861873002:
Align the base of the shelf's context menu to the top edge of the shelf. (Closed)
DescriptionAlign the base of the shelf's context menu to the top edge of the shelf.
The context menu of the shelf will cover part of the icon in the shelf.
And the coordinate adjustment from |MENU_ANCHOR_BOTTOMCENTER| is not suit
for left and right shelf.
changes in this cl,
1.Adjust the position of the context menu opened by shelf icon. Make it
align to top edge of the shelf.
Context menu of touch events should be put in the middle area.
BUG=710601
Review-Url: https://codereview.chromium.org/2861873002
Cr-Commit-Position: refs/heads/master@{#471068}
Committed: https://chromium.googlesource.com/chromium/src/+/f6b2be3d685c23dd16fe7878ad8c99ab86267278
Patch Set 1 #Patch Set 2 : Fix UT. #
Total comments: 16
Patch Set 3 : Rebased and adressed comments. #
Total comments: 3
Patch Set 4 : Fixed comments. #Patch Set 5 : Align the base of the shelf's context menu to the top edge of the shelf. #
Total comments: 12
Patch Set 6 : Fixed comments. #
Total comments: 2
Patch Set 7 : nits. #
Messages
Total messages: 68 (48 generated)
The CQ bit was checked by minch@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...
Description was changed from ========== Align the base of the shelf's context menu to the top edge of the shelf. BUG=710601 ========== to ========== Align the base of the shelf's context menu to the top edge of the shelf. The context menu of the shelf will cover part of the icon in the shelf. And the coordinate adjustment from |MENU_ANCHOR_BOTTOMCENTER| is not suit for left and right shelf. changes in this cl, 1.Adjust the position of the context menu opened by shelf icon. Make it align to top edge of the shelf. Context menu of touch events should be put in the middle area. BUG=710601 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_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 minch@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_chromium_chromeos_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 minch@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.
minch@chromium.org changed reviewers: + msw@chromium.org
Hi, msw@, could you help review? Thanks. https://codereview.chromium.org/2861873002/diff/20001/ui/views/controls/menu/... File ui/views/controls/menu/menu_controller.cc (left): https://codereview.chromium.org/2861873002/diff/20001/ui/views/controls/menu/... ui/views/controls/menu/menu_controller.cc:78: const int kCenteredContextMenuYOffset = -15; Sorry, not quite sure about here. This is not needed by the shelf context menu now. But |MENU_ANCHOR_BOTTOMCENTER| is also used by |ToolkitDelegateViews|. I am not sure whether it is ok to remove this variable here directly? msw@, what's your opinion?
https://codereview.chromium.org/2861873002/diff/20001/ash/shelf/shelf_view.cc File ash/shelf/shelf_view.cc (right): https://codereview.chromium.org/2861873002/diff/20001/ash/shelf/shelf_view.cc... ash/shelf/shelf_view.cc:1604: WmWindow* shelf_window = WmWindow::Get(shelf_widget_->GetNativeWindow()); nit: add a comment to this block of code, like "Align the context menu to the edge of the shelf." https://codereview.chromium.org/2861873002/diff/20001/ash/shelf/shelf_view.cc... ash/shelf/shelf_view.cc:1608: : (wm::GetDisplayBoundsInParent(shelf_window)); You'll probably need to refactor this as per https://codereview.chromium.org/2860163005/ https://codereview.chromium.org/2861873002/diff/20001/ash/shelf/shelf_view.cc... ash/shelf/shelf_view.cc:1610: gfx::Point point_aligned_shelf; nit: consider |menu_point| or |context_menu_point|. https://codereview.chromium.org/2861873002/diff/20001/ash/shelf/shelf_view.cc... ash/shelf/shelf_view.cc:1615: shelf_bounds.bottom() - kShelfSize); Why not just use shelf_bounds.y()? https://codereview.chromium.org/2861873002/diff/20001/ash/shelf/shelf_view.cc... ash/shelf/shelf_view.cc:1618: point_aligned_shelf.SetPoint(shelf_bounds.x() + kShelfSize, point.y()); Ditto: why not just use shelf_bounds.right()? https://codereview.chromium.org/2861873002/diff/20001/ash/shelf/shelf_view.cc... ash/shelf/shelf_view.cc:1621: point_aligned_shelf.SetPoint(shelf_bounds.right() - kShelfSize, Ditto: why not just use shelf_bounds.x()? https://codereview.chromium.org/2861873002/diff/20001/ash/shelf/shelf_view.cc... ash/shelf/shelf_view.cc:1623: default: Remove the default case, so if anyone adds a new alignment they'll update this switch. https://codereview.chromium.org/2861873002/diff/20001/ash/shelf/shelf_view.cc... ash/shelf/shelf_view.cc:1700: (shelf_alignment == SHELF_ALIGNMENT_LEFT || Use !WmShelf::IsHorizontalAlignment() instead, and remove the |shelf_alignment| local. https://codereview.chromium.org/2861873002/diff/20001/ui/base/ui_base_types.h File ui/base/ui_base_types.h (right): https://codereview.chromium.org/2861873002/diff/20001/ui/base/ui_base_types.h... ui/base/ui_base_types.h:48: MENU_SOURCE_TOUCH_SIDE_SHELF = 4, We probably want to avoid this or make it not shelf-specific (ie. replace 'SHELF' with 'ANCHOR' or 'MENU'); you need to ping ui/base/OWNERS anyway. https://codereview.chromium.org/2861873002/diff/20001/ui/views/controls/menu/... File ui/views/controls/menu/menu_controller.cc (left): https://codereview.chromium.org/2861873002/diff/20001/ui/views/controls/menu/... ui/views/controls/menu/menu_controller.cc:78: const int kCenteredContextMenuYOffset = -15; On 2017/05/04 16:44:15, minch1 wrote: > Sorry, not quite sure about here. This is not needed by the shelf context menu > now. But |MENU_ANCHOR_BOTTOMCENTER| is also used by |ToolkitDelegateViews|. I am > not sure whether it is ok to remove this variable here directly? msw@, what's > your opinion? This seems wrong, it'll change the positioning of all touch menus, not just the shelf ones. Perhaps we should be using MENU_ANCHOR_BUBBLE_* for shelf menus? (ShelfView::ShowMenu is already the only user of those MENU_ANCHOR_BUBBLE_* values). Or, perhaps (1) add yet another anchor that works like BOTTOMCENTER but doesn't apply the offset, or (2) make the shelf apply the inverse of this offset when constructing the menu... https://codereview.chromium.org/2861873002/diff/20001/ui/views/controls/menu/... File ui/views/controls/menu/menu_controller.cc (right): https://codereview.chromium.org/2861873002/diff/20001/ui/views/controls/menu/... ui/views/controls/menu/menu_controller.cc:1950: // If menu does not fit above the anchor. We move it to below. nit: "Place the menu below the anchor if it does not fit above." https://codereview.chromium.org/2861873002/diff/20001/ui/views/controls/menu/... ui/views/controls/menu/menu_controller.cc:1955: y = y - (pref.height() - state_.initial_bounds.height()) / 2; nit: y -= (or += with subtraction reversed might be a little simpler?)
Oh, please also post screenshots of the before/after in the bug.
https://codereview.chromium.org/2861873002/diff/20001/ash/shelf/shelf_view.cc File ash/shelf/shelf_view.cc (right): https://codereview.chromium.org/2861873002/diff/20001/ash/shelf/shelf_view.cc... ash/shelf/shelf_view.cc:1604: WmWindow* shelf_window = WmWindow::Get(shelf_widget_->GetNativeWindow()); On 2017/05/05 17:34:18, msw wrote: > nit: add a comment to this block of code, like "Align the context menu to the > edge of the shelf." Done. https://codereview.chromium.org/2861873002/diff/20001/ash/shelf/shelf_view.cc... ash/shelf/shelf_view.cc:1610: gfx::Point point_aligned_shelf; On 2017/05/05 17:34:18, msw wrote: > nit: consider |menu_point| or |context_menu_point|. Done. https://codereview.chromium.org/2861873002/diff/20001/ash/shelf/shelf_view.cc... ash/shelf/shelf_view.cc:1615: shelf_bounds.bottom() - kShelfSize); On 2017/05/05 17:34:19, msw wrote: > Why not just use shelf_bounds.y()? This is fine for overflow mode, if it is not. The shelf_bounds actually is the whole screen, left point of the Rect is (0, 0) with the screen's width and height. So, here if use shelf_bounds.y(), it will be 0 for none overflow mode. Then, the context menu will shown on the top of the screen.
The CQ bit was checked by minch@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...
minch@chromium.org changed reviewers: + dschuyler@chromium.org
I made a note that you may want to address, but overall I'm not sure my role on reviewing this CL. I'm not an OWNER here afaik - was there something specific you wanted me to look at (or maybe I was added by mistake)? Feel free to remove me as a reviewer if I was added by mistake. https://codereview.chromium.org/2861873002/diff/40001/chrome/browser/ui/windo... File chrome/browser/ui/window_sizer/window_sizer_ash_uitest.cc (right): https://codereview.chromium.org/2861873002/diff/40001/chrome/browser/ui/windo... chrome/browser/ui/window_sizer/window_sizer_ash_uitest.cc:166: release_point.Offset(50, -153); Consider a comment about the constants 50 and -153. (e.g. // 50 is the distance to ______; -153 moves _____ to _____.) or expressing them as constexpr's with descriptive names (e.g. constexpr int kDistanceTo_____ = 50; constexpr int k______Delta = -513; ).
On 2017/05/08 20:46:58, dschuyler wrote: > I made a note that you may want to address, but overall I'm not sure my role on > reviewing this CL. I'm not an OWNER here afaik - was there something specific > you wanted me to look at (or maybe I was added by mistake)? > Feel free to remove me as a reviewer if I was added by mistake. > > https://codereview.chromium.org/2861873002/diff/40001/chrome/browser/ui/windo... > File chrome/browser/ui/window_sizer/window_sizer_ash_uitest.cc (right): > > https://codereview.chromium.org/2861873002/diff/40001/chrome/browser/ui/windo... > chrome/browser/ui/window_sizer/window_sizer_ash_uitest.cc:166: > release_point.Offset(50, -153); > Consider a comment about the constants 50 and -153. (e.g. // 50 is the distance > to ______; -153 moves _____ to _____.) > or expressing them as constexpr's with descriptive names (e.g. > constexpr int kDistanceTo_____ = 50; > constexpr int k______Delta = -513; > ). Are you the owner of ui/base/ui_base_types.h ?
On 2017/05/08 20:49:09, minch1 wrote: > On 2017/05/08 20:46:58, dschuyler wrote: > > I made a note that you may want to address, but overall I'm not sure my role > on > > reviewing this CL. I'm not an OWNER here afaik - was there something specific > > you wanted me to look at (or maybe I was added by mistake)? > > Feel free to remove me as a reviewer if I was added by mistake. > > > > > https://codereview.chromium.org/2861873002/diff/40001/chrome/browser/ui/windo... > > File chrome/browser/ui/window_sizer/window_sizer_ash_uitest.cc (right): > > > > > https://codereview.chromium.org/2861873002/diff/40001/chrome/browser/ui/windo... > > chrome/browser/ui/window_sizer/window_sizer_ash_uitest.cc:166: > > release_point.Offset(50, -153); > > Consider a comment about the constants 50 and -153. (e.g. // 50 is the > distance > > to ______; -153 moves _____ to _____.) > > or expressing them as constexpr's with descriptive names (e.g. > > constexpr int kDistanceTo_____ = 50; > > constexpr int k______Delta = -513; > > ). > > Are you the owner of ui/base/ui_base_types.h ? Ah, I see that it's likely this line in the OWNERS file being confusing: per-file template_expressions*=dschuyler@chromium.org That's trying to say that I'm owner on files that match this pattern: "template_expressions*". So I'm not an owner on ui/base/ui_base_types.h Maybe go up to the parent directory OWNERS file at https://cs.chromium.org/chromium/src/ui/OWNERS Or use the $ git cl owners command to search for owners
On 2017/05/08 20:56:26, dschuyler wrote: > On 2017/05/08 20:49:09, minch1 wrote: > > On 2017/05/08 20:46:58, dschuyler wrote: > > > I made a note that you may want to address, but overall I'm not sure my role > > on > > > reviewing this CL. I'm not an OWNER here afaik - was there something > specific > > > you wanted me to look at (or maybe I was added by mistake)? > > > Feel free to remove me as a reviewer if I was added by mistake. > > > > > > > > > https://codereview.chromium.org/2861873002/diff/40001/chrome/browser/ui/windo... > > > File chrome/browser/ui/window_sizer/window_sizer_ash_uitest.cc (right): > > > > > > > > > https://codereview.chromium.org/2861873002/diff/40001/chrome/browser/ui/windo... > > > chrome/browser/ui/window_sizer/window_sizer_ash_uitest.cc:166: > > > release_point.Offset(50, -153); > > > Consider a comment about the constants 50 and -153. (e.g. // 50 is the > > distance > > > to ______; -153 moves _____ to _____.) > > > or expressing them as constexpr's with descriptive names (e.g. > > > constexpr int kDistanceTo_____ = 50; > > > constexpr int k______Delta = -513; > > > ). > > > > Are you the owner of ui/base/ui_base_types.h ? > > Ah, I see that it's likely this line in the OWNERS file being confusing: > per-file mailto:template_expressions*=dschuyler@chromium.org > That's trying to say that I'm owner on files that match this pattern: > "template_expressions*". > So I'm not an owner on ui/base/ui_base_types.h > Maybe go up to the parent directory OWNERS file at > https://cs.chromium.org/chromium/src/ui/OWNERS > Or use the > $ git cl owners > command to search for owners Got it. Thanks.
minch@chromium.org changed reviewers: + sadrul@chromium.org - dschuyler@chromium.org
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 minch@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 checked by minch@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.
msw@, sadrul@, could you help review? Thanks. https://codereview.chromium.org/2861873002/diff/40001/ui/views/controls/menu/... File ui/views/controls/menu/menu_controller.cc (right): https://codereview.chromium.org/2861873002/diff/40001/ui/views/controls/menu/... ui/views/controls/menu/menu_controller.cc:1954: // Place the menu below if it does not fit above. Wrong position of the comment. Will update it in next PS.
https://codereview.chromium.org/2861873002/diff/40001/ui/base/ui_base_types.h File ui/base/ui_base_types.h (right): https://codereview.chromium.org/2861873002/diff/40001/ui/base/ui_base_types.h... ui/base/ui_base_types.h:49: MENU_SOURCE_TOUCH_BOTTOM_ANCHOR = 5, I don't think these belong here.
The CQ bit was checked by minch@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 checked by minch@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...
msw@, sadrul@, how about to do it like ps#5?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
This looks better to me, yes. One minor comment though: https://codereview.chromium.org/2861873002/diff/80001/ui/views/controls/menu/... File ui/views/controls/menu/menu_runner.h (right): https://codereview.chromium.org/2861873002/diff/80001/ui/views/controls/menu/... ui/views/controls/menu/menu_runner.h:90: SHELF_CONTEXT_MENU = 1 << 6, Can you call this FIXED_ANCHOR, or something like that? Essentially, it's a hint to MenuRunner so that it does not attempt to figure out the anchor-position by itself. Because views should not need to know about 'shelf' etc.
The CQ bit was checked by minch@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_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
+1 to Sadrul's comment, and some additional comments. https://codereview.chromium.org/2861873002/diff/80001/ash/shelf/shelf_view.cc File ash/shelf/shelf_view.cc (right): https://codereview.chromium.org/2861873002/diff/80001/ash/shelf/shelf_view.cc... ash/shelf/shelf_view.cc:1647: ? (owner_overflow_bubble_->bubble_view()->GetBubbleBounds()) nit: extra parens not needed here or on the next line. https://codereview.chromium.org/2861873002/diff/80001/ash/shelf/shelf_view.cc... ash/shelf/shelf_view.cc:1735: // Distinguish the touch events that triggered on the bottom shelf or Should this be clobbering the |menu_alignment| values assigned in the |if (!context_menu)| block above? If so, remove those assignments? (you should test the application menu for items with multiple windows) https://codereview.chromium.org/2861873002/diff/80001/chrome/browser/ui/windo... File chrome/browser/ui/window_sizer/window_sizer_ash_uitest.cc (right): https://codereview.chromium.org/2861873002/diff/80001/chrome/browser/ui/windo... chrome/browser/ui/window_sizer/window_sizer_ash_uitest.cc:166: // 50 is the distance from the chrome icon to leftest of the screen. nit: s/leftest/the left edge/. Also, I'm not sure if this is accurate... why would we need to offset the release point's x value by the distance between the chrome icon and the left edge of the screen, if the x value is from |chrome_icon|, which is already presumably the x-coordinate of the icon (the distance between the icon and the left edge of the screen). Perhaps this x-offset isn't needed? https://codereview.chromium.org/2861873002/diff/80001/chrome/browser/ui/windo... chrome/browser/ui/window_sizer/window_sizer_ash_uitest.cc:167: // -153 moves from the chrome icon to the "New window" in the opened context nit: // -153 moves the cursor up to the "New window" menu option. https://codereview.chromium.org/2861873002/diff/80001/ui/views/controls/menu/... File ui/views/controls/menu/menu_types.h (right): https://codereview.chromium.org/2861873002/diff/80001/ui/views/controls/menu/... ui/views/controls/menu/menu_types.h:18: MENU_ANCHOR_FIXED_BOTTOMCENTER, nit: describe these in the enum comment. https://codereview.chromium.org/2861873002/diff/80001/ui/views/controls/menu/... ui/views/controls/menu/menu_types.h:19: MENU_ANCHOR_SIDECENTER, nit: should this be |MENU_ANCHOR_FIXED_SIDECENTER|? (ie. does it have similar 'fixed' behavior to |MENU_ANCHOR_FIXED_BOTTOMCENTER|).
The CQ bit was checked by minch@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.
msw@, could you help review again? Thanks. https://codereview.chromium.org/2861873002/diff/80001/ash/shelf/shelf_view.cc File ash/shelf/shelf_view.cc (right): https://codereview.chromium.org/2861873002/diff/80001/ash/shelf/shelf_view.cc... ash/shelf/shelf_view.cc:1735: // Distinguish the touch events that triggered on the bottom shelf or On 2017/05/10 22:18:22, msw wrote: > Should this be clobbering the |menu_alignment| values assigned in the |if > (!context_menu)| block above? If so, remove those assignments? (you should test > the application menu for items with multiple windows) Oh, here will reassign the menu_alignment if |if(!context_menu)| happens, there are different alignments. I think I should put following lines in |else| block instead. https://codereview.chromium.org/2861873002/diff/80001/chrome/browser/ui/windo... File chrome/browser/ui/window_sizer/window_sizer_ash_uitest.cc (right): https://codereview.chromium.org/2861873002/diff/80001/chrome/browser/ui/windo... chrome/browser/ui/window_sizer/window_sizer_ash_uitest.cc:166: // 50 is the distance from the chrome icon to leftest of the screen. On 2017/05/10 22:18:23, msw wrote: > nit: s/leftest/the left edge/. Also, I'm not sure if this is accurate... why > would we need to offset the release point's x value by the distance between the > chrome icon and the left edge of the screen, if the x value is from > |chrome_icon|, which is already presumably the x-coordinate of the icon (the > distance between the icon and the left edge of the screen). Perhaps this > x-offset isn't needed? This is only my own understanding, I didn't check this with the owner of this line? I tried to make it as 0 locally, the UT passed locally. If Trybot results are all agree here, does that means it is ok to make it as 0 here?
lgtm with a nit https://codereview.chromium.org/2861873002/diff/80001/ash/shelf/shelf_view.cc File ash/shelf/shelf_view.cc (right): https://codereview.chromium.org/2861873002/diff/80001/ash/shelf/shelf_view.cc... ash/shelf/shelf_view.cc:1647: ? (owner_overflow_bubble_->bubble_view()->GetBubbleBounds()) On 2017/05/10 22:18:22, msw wrote: > nit: extra parens not needed here or on the next line. Just FYI, I would appreciate it if you respond to all comments, even if you just use the 'Done' and 'Acknowledged' shortcut buttons. This helps me track how comments have been addressed. https://codereview.chromium.org/2861873002/diff/80001/ash/shelf/shelf_view.cc... ash/shelf/shelf_view.cc:1735: // Distinguish the touch events that triggered on the bottom shelf or On 2017/05/11 16:34:02, minch1 wrote: > On 2017/05/10 22:18:22, msw wrote: > > Should this be clobbering the |menu_alignment| values assigned in the |if > > (!context_menu)| block above? If so, remove those assignments? (you should > test > > the application menu for items with multiple windows) > > Oh, here will reassign the menu_alignment if |if(!context_menu)| happens, there > are different alignments. I think I should put following lines in |else| block > instead. aside: it'd be nice if UX wanted both menus to look/act the same. https://codereview.chromium.org/2861873002/diff/80001/chrome/browser/ui/windo... File chrome/browser/ui/window_sizer/window_sizer_ash_uitest.cc (right): https://codereview.chromium.org/2861873002/diff/80001/chrome/browser/ui/windo... chrome/browser/ui/window_sizer/window_sizer_ash_uitest.cc:166: // 50 is the distance from the chrome icon to leftest of the screen. On 2017/05/11 16:34:03, minch1 wrote: > On 2017/05/10 22:18:23, msw wrote: > > nit: s/leftest/the left edge/. Also, I'm not sure if this is accurate... why > > would we need to offset the release point's x value by the distance between > the > > chrome icon and the left edge of the screen, if the x value is from > > |chrome_icon|, which is already presumably the x-coordinate of the icon (the > > distance between the icon and the left edge of the screen). Perhaps this > > x-offset isn't needed? > > This is only my own understanding, I didn't check this with the owner of this > line? I tried to make it as 0 locally, the UT passed locally. If Trybot results > are all agree here, does that means it is ok to make it as 0 here? Yeah, 0 sgtm; thanks for simplifying this mystifying value. https://codereview.chromium.org/2861873002/diff/100001/ui/views/controls/menu... File ui/views/controls/menu/menu_runner.h (right): https://codereview.chromium.org/2861873002/diff/100001/ui/views/controls/menu... ui/views/controls/menu/menu_runner.h:90: // figure out it. For example the context menu of shelf item. nit: s/figure out it/adjust the anchor point/
lgtm
The CQ bit was checked by minch@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 minch@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from msw@chromium.org, sadrul@chromium.org Link to the patchset: https://codereview.chromium.org/2861873002/#ps120001 (title: "nits.")
https://codereview.chromium.org/2861873002/diff/100001/ui/views/controls/menu... File ui/views/controls/menu/menu_runner.h (right): https://codereview.chromium.org/2861873002/diff/100001/ui/views/controls/menu... ui/views/controls/menu/menu_runner.h:90: // figure out it. For example the context menu of shelf item. On 2017/05/11 17:13:40, msw wrote: > nit: s/figure out it/adjust the anchor point/ Done.
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 120001, "attempt_start_ts": 1494535679939360,
"parent_rev": "e260698094e788b94a4106af1d518e7b51875102", "commit_rev":
"f6b2be3d685c23dd16fe7878ad8c99ab86267278"}
Message was sent while issue was closed.
Description was changed from ========== Align the base of the shelf's context menu to the top edge of the shelf. The context menu of the shelf will cover part of the icon in the shelf. And the coordinate adjustment from |MENU_ANCHOR_BOTTOMCENTER| is not suit for left and right shelf. changes in this cl, 1.Adjust the position of the context menu opened by shelf icon. Make it align to top edge of the shelf. Context menu of touch events should be put in the middle area. BUG=710601 ========== to ========== Align the base of the shelf's context menu to the top edge of the shelf. The context menu of the shelf will cover part of the icon in the shelf. And the coordinate adjustment from |MENU_ANCHOR_BOTTOMCENTER| is not suit for left and right shelf. changes in this cl, 1.Adjust the position of the context menu opened by shelf icon. Make it align to top edge of the shelf. Context menu of touch events should be put in the middle area. BUG=710601 Review-Url: https://codereview.chromium.org/2861873002 Cr-Commit-Position: refs/heads/master@{#471068} Committed: https://chromium.googlesource.com/chromium/src/+/f6b2be3d685c23dd16fe7878ad8c... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/chromium/src/+/f6b2be3d685c23dd16fe7878ad8c... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
