Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
7740024:
Implement touch selection menu. (Closed)
|
Created:
9 years, 4 months ago by varunjain Modified:
9 years, 3 months ago CC:
chromium-reviews, dhollowa, darin-cc_chromium.org, brettw-cc_chromium.org Visibility:
Public. |
DescriptionImplement touch selection menu.
BUG=none
TEST=none
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=98473
Patch Set 1 #
Total comments: 14
Patch Set 2 : modified according to comments #Patch Set 3 : added SetBounds() on the view. #Patch Set 4 : changed SetBounds to Layout #
Messages
Total messages: 11 (0 generated)
http://codereview.chromium.org/7740024/diff/1/views/touchui/touch_selection_c... File views/touchui/touch_selection_controller_impl.cc (right): http://codereview.chromium.org/7740024/diff/1/views/touchui/touch_selection_c... views/touchui/touch_selection_controller_impl.cc:201: widget_->SetBounds(widget_bounds.AdjustToFit(monitor_bounds)); Might you end up showing the context menu over the selection handles? http://codereview.chromium.org/7740024/diff/1/views/touchui/touch_selection_c... views/touchui/touch_selection_controller_impl.cc:216: void RefreshButtonsAndLayout() { nit: this doesn't layout, so maybe just RefreshButtons? http://codereview.chromium.org/7740024/diff/1/views/touchui/touch_selection_c... views/touchui/touch_selection_controller_impl.cc:225: button->set_focusable(true); Is there a reason why we aren't using a standard menu here? http://codereview.chromium.org/7740024/diff/1/views/touchui/touch_selection_c... views/touchui/touch_selection_controller_impl.cc:243: SetBounds(0, 0, total_width, height); I don't think this is necessary. What matters is the bounds of the widget change. Once that happens, it'll update your views bounds. http://codereview.chromium.org/7740024/diff/1/views/touchui/touch_selection_c... views/touchui/touch_selection_controller_impl.cc:375: context_menu_screen_pos_.set_x((p1.x() + p2.x()) / 2); Why does this need to be cached here instead of calculating in MenuTimeFired when you need it? http://codereview.chromium.org/7740024/diff/1/views/touchui/touch_selection_c... views/touchui/touch_selection_controller_impl.cc:401: if (context_menu_timer_.IsRunning()) No need to check if running. http://codereview.chromium.org/7740024/diff/1/views/touchui/touch_selection_c... File views/touchui/touch_selection_controller_impl.h (right): http://codereview.chromium.org/7740024/diff/1/views/touchui/touch_selection_c... views/touchui/touch_selection_controller_impl.h:69: base::OneShotTimer<TouchSelectionControllerImpl> context_menu_timer_; Description?
http://codereview.chromium.org/7740024/diff/1/views/touchui/touch_selection_c... File views/touchui/touch_selection_controller_impl.cc (right): http://codereview.chromium.org/7740024/diff/1/views/touchui/touch_selection_c... views/touchui/touch_selection_controller_impl.cc:201: widget_->SetBounds(widget_bounds.AdjustToFit(monitor_bounds)); On 2011/08/26 15:45:33, sky wrote: > Might you end up showing the context menu over the selection handles? This is possible in theory. But in practice, selection can only occur on a webpage or omnibox. in both cases AdjustToFit is there to make sure that the menu does not go off-screen on the x-axis. It should never need to adjust the y position of the menu (hence never cover the selection handles) http://codereview.chromium.org/7740024/diff/1/views/touchui/touch_selection_c... views/touchui/touch_selection_controller_impl.cc:216: void RefreshButtonsAndLayout() { On 2011/08/26 15:45:33, sky wrote: > nit: this doesn't layout, so maybe just RefreshButtons? Done. http://codereview.chromium.org/7740024/diff/1/views/touchui/touch_selection_c... views/touchui/touch_selection_controller_impl.cc:243: SetBounds(0, 0, total_width, height); On 2011/08/26 15:45:33, sky wrote: > I don't think this is necessary. What matters is the bounds of the widget > change. Once that happens, it'll update your views bounds. Done. http://codereview.chromium.org/7740024/diff/1/views/touchui/touch_selection_c... views/touchui/touch_selection_controller_impl.cc:375: context_menu_screen_pos_.set_x((p1.x() + p2.x()) / 2); On 2011/08/26 15:45:33, sky wrote: > Why does this need to be cached here instead of calculating in MenuTimeFired > when you need it? It doesnt. Changed. http://codereview.chromium.org/7740024/diff/1/views/touchui/touch_selection_c... views/touchui/touch_selection_controller_impl.cc:401: if (context_menu_timer_.IsRunning()) On 2011/08/26 15:45:33, sky wrote: > No need to check if running. Done. http://codereview.chromium.org/7740024/diff/1/views/touchui/touch_selection_c... File views/touchui/touch_selection_controller_impl.h (right): http://codereview.chromium.org/7740024/diff/1/views/touchui/touch_selection_c... views/touchui/touch_selection_controller_impl.h:69: base::OneShotTimer<TouchSelectionControllerImpl> context_menu_timer_; On 2011/08/26 15:45:33, sky wrote: > Description? Done.
http://codereview.chromium.org/7740024/diff/1/views/touchui/touch_selection_c... File views/touchui/touch_selection_controller_impl.cc (right): http://codereview.chromium.org/7740024/diff/1/views/touchui/touch_selection_c... views/touchui/touch_selection_controller_impl.cc:243: SetBounds(0, 0, total_width, height); On 2011/08/26 16:31:15, varunjain wrote: > On 2011/08/26 15:45:33, sky wrote: > > I don't think this is necessary. What matters is the bounds of the widget > > change. Once that happens, it'll update your views bounds. > > Done. This is not really working. What I am seeing is that if I do not SetBounds on the view, it ends up having zero bounds while the widget has proper bounds. I am adding SetBounds on the view back.
On Fri, Aug 26, 2011 at 9:31 AM, <varunjain@chromium.org> wrote: > > http://codereview.chromium.org/7740024/diff/1/views/touchui/touch_selection_c... > File views/touchui/touch_selection_controller_impl.cc (right): > > http://codereview.chromium.org/7740024/diff/1/views/touchui/touch_selection_c... > views/touchui/touch_selection_controller_impl.cc:201: > widget_->SetBounds(widget_bounds.AdjustToFit(monitor_bounds)); > On 2011/08/26 15:45:33, sky wrote: >> >> Might you end up showing the context menu over the selection handles? > > This is possible in theory. But in practice, selection can only occur on > a webpage or omnibox. in both cases AdjustToFit is there to make sure > that the menu does not go off-screen on the x-axis. It should never need > to adjust the y position of the menu (hence never cover the selection > handles) What happens if the selection is on the page and close to the bottom of the screen. When the menu gets pushed up does it overlap the selection handles? -Scott > > http://codereview.chromium.org/7740024/diff/1/views/touchui/touch_selection_c... > views/touchui/touch_selection_controller_impl.cc:216: void > RefreshButtonsAndLayout() { > On 2011/08/26 15:45:33, sky wrote: >> >> nit: this doesn't layout, so maybe just RefreshButtons? > > Done. > > http://codereview.chromium.org/7740024/diff/1/views/touchui/touch_selection_c... > views/touchui/touch_selection_controller_impl.cc:243: SetBounds(0, 0, > total_width, height); > On 2011/08/26 15:45:33, sky wrote: >> >> I don't think this is necessary. What matters is the bounds of the > > widget >> >> change. Once that happens, it'll update your views bounds. > > Done. > > http://codereview.chromium.org/7740024/diff/1/views/touchui/touch_selection_c... > views/touchui/touch_selection_controller_impl.cc:375: > context_menu_screen_pos_.set_x((p1.x() + p2.x()) / 2); > On 2011/08/26 15:45:33, sky wrote: >> >> Why does this need to be cached here instead of calculating in > > MenuTimeFired >> >> when you need it? > > It doesnt. Changed. > > http://codereview.chromium.org/7740024/diff/1/views/touchui/touch_selection_c... > views/touchui/touch_selection_controller_impl.cc:401: if > (context_menu_timer_.IsRunning()) > On 2011/08/26 15:45:33, sky wrote: >> >> No need to check if running. > > Done. > > http://codereview.chromium.org/7740024/diff/1/views/touchui/touch_selection_c... > File views/touchui/touch_selection_controller_impl.h (right): > > http://codereview.chromium.org/7740024/diff/1/views/touchui/touch_selection_c... > views/touchui/touch_selection_controller_impl.h:69: > base::OneShotTimer<TouchSelectionControllerImpl> context_menu_timer_; > On 2011/08/26 15:45:33, sky wrote: >> >> Description? > > Done. > > http://codereview.chromium.org/7740024/ >
On Fri, Aug 26, 2011 at 9:41 AM, <varunjain@chromium.org> wrote: > > http://codereview.chromium.org/7740024/diff/1/views/touchui/touch_selection_c... > File views/touchui/touch_selection_controller_impl.cc (right): > > http://codereview.chromium.org/7740024/diff/1/views/touchui/touch_selection_c... > views/touchui/touch_selection_controller_impl.cc:243: SetBounds(0, 0, > total_width, height); > On 2011/08/26 16:31:15, varunjain wrote: >> >> On 2011/08/26 15:45:33, sky wrote: >> > I don't think this is necessary. What matters is the bounds of the > > widget >> >> > change. Once that happens, it'll update your views bounds. > >> Done. > > This is not really working. What I am seeing is that if I do not > SetBounds on the view, it ends up having zero bounds while the widget > has proper bounds. I am adding SetBounds on the view back. No where else do we explicitly set the bounds like that. If you are having to set the bounds explicitly, it indicates a bug some where that is going to bite us. It's worth digging in and figuring out why you need to do it. Does it go away if you remove View::SetVisible(visible) in TouchContextMenuView::SetVisible? -Scott
On Fri, Aug 26, 2011 at 12:54 PM, Scott Violet <sky@chromium.org> wrote: > On Fri, Aug 26, 2011 at 9:31 AM, <varunjain@chromium.org> wrote: > > > > > http://codereview.chromium.org/7740024/diff/1/views/touchui/touch_selection_c... > > File views/touchui/touch_selection_controller_impl.cc (right): > > > > > http://codereview.chromium.org/7740024/diff/1/views/touchui/touch_selection_c... > > views/touchui/touch_selection_controller_impl.cc:201: > > widget_->SetBounds(widget_bounds.AdjustToFit(monitor_bounds)); > > On 2011/08/26 15:45:33, sky wrote: > >> > >> Might you end up showing the context menu over the selection handles? > > > > This is possible in theory. But in practice, selection can only occur on > > a webpage or omnibox. in both cases AdjustToFit is there to make sure > > that the menu does not go off-screen on the x-axis. It should never need > > to adjust the y position of the menu (hence never cover the selection > > handles) > > What happens if the selection is on the page and close to the bottom > of the screen. When the menu gets pushed up does it overlap the > selection handles? > close to the bottom is actually the easy case. The menu does not increase vertically in height (the buttons are added horizontally). So the menu is guaranteed to be displayed above the handles. It may come on top of the handles if the handles are near the top of the screen which cannot be the case in the current Tabstrip UI. As I mentioned earlier, this is just a starter UI and the logic for positioning/drawing the menu will change later according to UI spec. > > -Scott > > > > > http://codereview.chromium.org/7740024/diff/1/views/touchui/touch_selection_c... > > views/touchui/touch_selection_controller_impl.cc:216: void > > RefreshButtonsAndLayout() { > > On 2011/08/26 15:45:33, sky wrote: > >> > >> nit: this doesn't layout, so maybe just RefreshButtons? > > > > Done. > > > > > http://codereview.chromium.org/7740024/diff/1/views/touchui/touch_selection_c... > > views/touchui/touch_selection_controller_impl.cc:243: SetBounds(0, 0, > > total_width, height); > > On 2011/08/26 15:45:33, sky wrote: > >> > >> I don't think this is necessary. What matters is the bounds of the > > > > widget > >> > >> change. Once that happens, it'll update your views bounds. > > > > Done. > > > > > http://codereview.chromium.org/7740024/diff/1/views/touchui/touch_selection_c... > > views/touchui/touch_selection_controller_impl.cc:375: > > context_menu_screen_pos_.set_x((p1.x() + p2.x()) / 2); > > On 2011/08/26 15:45:33, sky wrote: > >> > >> Why does this need to be cached here instead of calculating in > > > > MenuTimeFired > >> > >> when you need it? > > > > It doesnt. Changed. > > > > > http://codereview.chromium.org/7740024/diff/1/views/touchui/touch_selection_c... > > views/touchui/touch_selection_controller_impl.cc:401: if > > (context_menu_timer_.IsRunning()) > > On 2011/08/26 15:45:33, sky wrote: > >> > >> No need to check if running. > > > > Done. > > > > > http://codereview.chromium.org/7740024/diff/1/views/touchui/touch_selection_c... > > File views/touchui/touch_selection_controller_impl.h (right): > > > > > http://codereview.chromium.org/7740024/diff/1/views/touchui/touch_selection_c... > > views/touchui/touch_selection_controller_impl.h:69: > > base::OneShotTimer<TouchSelectionControllerImpl> context_menu_timer_; > > On 2011/08/26 15:45:33, sky wrote: > >> > >> Description? > > > > Done. > > > > http://codereview.chromium.org/7740024/ > > >
On Fri, Aug 26, 2011 at 12:59 PM, Scott Violet <sky@chromium.org> wrote: > On Fri, Aug 26, 2011 at 9:41 AM, <varunjain@chromium.org> wrote: > > > > > http://codereview.chromium.org/7740024/diff/1/views/touchui/touch_selection_c... > > File views/touchui/touch_selection_controller_impl.cc (right): > > > > > http://codereview.chromium.org/7740024/diff/1/views/touchui/touch_selection_c... > > views/touchui/touch_selection_controller_impl.cc:243: SetBounds(0, 0, > > total_width, height); > > On 2011/08/26 16:31:15, varunjain wrote: > >> > >> On 2011/08/26 15:45:33, sky wrote: > >> > I don't think this is necessary. What matters is the bounds of the > > > > widget > >> > >> > change. Once that happens, it'll update your views bounds. > > > >> Done. > > > > This is not really working. What I am seeing is that if I do not > > SetBounds on the view, it ends up having zero bounds while the widget > > has proper bounds. I am adding SetBounds on the view back. > > No where else do we explicitly set the bounds like that. If you are > having to set the bounds explicitly, it indicates a bug some where > that is going to bite us. It's worth digging in and figuring out why > you need to do it. Does it go away if you remove > View::SetVisible(visible) in TouchContextMenuView::SetVisible? > Investigating > > -Scott >
LGTM
Change committed as 98473 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
