Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
6750007:
Scrolling Tabs (Closed)
|
Created:
9 years, 9 months ago by wyck Modified:
8 years, 7 months ago CC:
chromium-reviews Visibility:
Public. |
DescriptionScrolling Tabs
Implement scrolling tabs. Selected tab stays in view.
BUG=None
TEST=None
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=80945
Patch Set 1 : self review 1 #
Total comments: 17
Patch Set 2 : addressing reviewer issues #
Total comments: 13
Patch Set 3 : MouseRelease cancel and some more comments #Patch Set 4 : TODO for transformable views #
Total comments: 10
Patch Set 5 : Using OnMouseCaptureLost instead of OnMouseReleased(cancel) #Patch Set 6 : Addressing Scott's review comments #
Messages
Total messages: 22 (0 generated)
Supercedes 6725043.
http://codereview.chromium.org/6750007/diff/2001/chrome/browser/ui/touch/tabs... File chrome/browser/ui/touch/tabs/touch_tab_strip.cc (right): http://codereview.chromium.org/6750007/diff/2001/chrome/browser/ui/touch/tabs... chrome/browser/ui/touch/tabs/touch_tab_strip.cc:9: #include "chrome/browser/ui/touch/tabs/touch_tab.h" newline between 8 and 9 http://codereview.chromium.org/6750007/diff/2001/chrome/browser/ui/touch/tabs... chrome/browser/ui/touch/tabs/touch_tab_strip.cc:143: x = std::min(std::max(0,x),width()-kTouchTabWidth); Yow, how about some spaces? http://codereview.chromium.org/6750007/diff/2001/chrome/browser/ui/touch/tabs... chrome/browser/ui/touch/tabs/touch_tab_strip.cc:151: min_scroll_offset_ = std::min(0, width() - tab_x); What about max_scroll_offset_? http://codereview.chromium.org/6750007/diff/2001/chrome/browser/ui/touch/tabs... chrome/browser/ui/touch/tabs/touch_tab_strip.cc:183: EndScroll(event.location()); If canceled do you want to revert the scroll? http://codereview.chromium.org/6750007/diff/2001/chrome/browser/ui/touch/tabs... chrome/browser/ui/touch/tabs/touch_tab_strip.cc:190: initial_tab_ = GetTabAtLocal(point); This doesn't seem to be used, is it needed? http://codereview.chromium.org/6750007/diff/2001/chrome/browser/ui/touch/tabs... chrome/browser/ui/touch/tabs/touch_tab_strip.cc:226: + std::min((scroll_offset_ - max_scroll_offset) / 4, Where does the /4 come from? http://codereview.chromium.org/6750007/diff/2001/chrome/browser/ui/touch/tabs... chrome/browser/ui/touch/tabs/touch_tab_strip.cc:244: const gfx::Point& local_point) { This is nearly identical to GetTabAt. If you're going to continue extending BaseTabStrip refactor so you don't duplicate code. http://codereview.chromium.org/6750007/diff/2001/chrome/browser/ui/touch/tabs... File chrome/browser/ui/touch/tabs/touch_tab_strip.h (right): http://codereview.chromium.org/6750007/diff/2001/chrome/browser/ui/touch/tabs... chrome/browser/ui/touch/tabs/touch_tab_strip.h:72: TouchTab* GetTabAtLocal(const gfx::Point& local_point); Newline between 71 and 72 to make it clear this isn't a views override. Also, add comments for methods and fields.
Changed as per review. http://codereview.chromium.org/6750007/diff/2001/chrome/browser/ui/touch/tabs... File chrome/browser/ui/touch/tabs/touch_tab_strip.cc (right): http://codereview.chromium.org/6750007/diff/2001/chrome/browser/ui/touch/tabs... chrome/browser/ui/touch/tabs/touch_tab_strip.cc:9: #include "chrome/browser/ui/touch/tabs/touch_tab.h" On 2011/03/25 21:05:25, sky wrote: > newline between 8 and 9 Done. http://codereview.chromium.org/6750007/diff/2001/chrome/browser/ui/touch/tabs... chrome/browser/ui/touch/tabs/touch_tab_strip.cc:151: min_scroll_offset_ = std::min(0, width() - tab_x); On 2011/03/25 21:05:25, sky wrote: > What about max_scroll_offset_? max_scroll_offset_ is technically always just zero. But I just gave it a name on line 222 to make the logic appear symmetrical with similar symbols. http://codereview.chromium.org/6750007/diff/2001/chrome/browser/ui/touch/tabs... chrome/browser/ui/touch/tabs/touch_tab_strip.cc:183: EndScroll(event.location()); On 2011/03/25 21:05:25, sky wrote: > If canceled do you want to revert the scroll? Sorry, I don't understand a "canceled" Mouse Release. How is this possible? http://codereview.chromium.org/6750007/diff/2001/chrome/browser/ui/touch/tabs... chrome/browser/ui/touch/tabs/touch_tab_strip.cc:190: initial_tab_ = GetTabAtLocal(point); On 2011/03/25 21:05:25, sky wrote: > This doesn't seem to be used, is it needed? The intent was to make sure that a very small drag less than the threshold, that crosses the boundary between two tabs, does nothing, instead of selecting one of the tabs. I have adjusted the logic on line 212. http://codereview.chromium.org/6750007/diff/2001/chrome/browser/ui/touch/tabs... chrome/browser/ui/touch/tabs/touch_tab_strip.cc:222: int max_scroll_offset = 0; Added an explanation here too. http://codereview.chromium.org/6750007/diff/2001/chrome/browser/ui/touch/tabs... chrome/browser/ui/touch/tabs/touch_tab_strip.cc:226: + std::min((scroll_offset_ - max_scroll_offset) / 4, On 2011/03/25 21:05:25, sky wrote: > Where does the /4 come from? Until I decide what the springiness of the rubber-banding effect should be, it just becomes 25% of normal displacement - as opposed to some kind of Hooke's law. It looked reasonable. I'll add a comment. http://codereview.chromium.org/6750007/diff/2001/chrome/browser/ui/touch/tabs... chrome/browser/ui/touch/tabs/touch_tab_strip.cc:244: const gfx::Point& local_point) { On 2011/03/25 21:05:25, sky wrote: > This is nearly identical to GetTabAt. If you're going to continue extending > BaseTabStrip refactor so you don't duplicate code. Done. http://codereview.chromium.org/6750007/diff/2001/chrome/browser/ui/touch/tabs... File chrome/browser/ui/touch/tabs/touch_tab_strip.h (right): http://codereview.chromium.org/6750007/diff/2001/chrome/browser/ui/touch/tabs... chrome/browser/ui/touch/tabs/touch_tab_strip.h:72: TouchTab* GetTabAtLocal(const gfx::Point& local_point); On 2011/03/25 21:05:25, sky wrote: > Newline between 71 and 72 to make it clear this isn't a views override. Also, > add comments for methods and fields. Done.
http://codereview.chromium.org/6750007/diff/2001/chrome/browser/ui/touch/tabs... File chrome/browser/ui/touch/tabs/touch_tab_strip.cc (right): http://codereview.chromium.org/6750007/diff/2001/chrome/browser/ui/touch/tabs... chrome/browser/ui/touch/tabs/touch_tab_strip.cc:183: EndScroll(event.location()); On 2011/03/28 14:21:43, wyck wrote: > On 2011/03/25 21:05:25, sky wrote: > > If canceled do you want to revert the scroll? > > Sorry, I don't understand a "canceled" Mouse Release. How is this possible? There are certain events that can cause mouse dragging to be interrupted. These are typically considered a cancel and usually treated as reverting whatever was under way.
Assorted implementation concerns. Maybe most could be fixed as TODOs. http://codereview.chromium.org/6750007/diff/5001/chrome/browser/ui/touch/tabs... File chrome/browser/ui/touch/tabs/touch_tab_strip.cc (right): http://codereview.chromium.org/6750007/diff/5001/chrome/browser/ui/touch/tabs... chrome/browser/ui/touch/tabs/touch_tab_strip.cc:169: bool TouchTabStrip::OnMousePressed(const views::MouseEvent& event) { these need unit tests. http://codereview.chromium.org/6750007/diff/5001/chrome/browser/ui/touch/tabs... chrome/browser/ui/touch/tabs/touch_tab_strip.cc:171: BeginScroll(event.location()); I thought we had discussed creating synthetic events and dispatching them from the gesture manager? perhaps you should add a todo for this? http://codereview.chromium.org/6750007/diff/5001/chrome/browser/ui/touch/tabs... chrome/browser/ui/touch/tabs/touch_tab_strip.cc:220: void TouchTabStrip::ScrollTo(int delta_x) { I would have thought that there should be a schedulepaint downstream from here? Have I missed it? And use a transformable view? Or at least take a TODO to use one? http://codereview.chromium.org/6750007/diff/5001/chrome/browser/ui/touch/tabs... File chrome/browser/ui/touch/tabs/touch_tab_strip.h (right): http://codereview.chromium.org/6750007/diff/5001/chrome/browser/ui/touch/tabs... chrome/browser/ui/touch/tabs/touch_tab_strip.h:75: void BeginScroll(const gfx::Point& point); not to make your life difficult... maybe there could be an abstract scroller mixin in some future CL? http://codereview.chromium.org/6750007/diff/5001/chrome/browser/ui/touch/tabs... chrome/browser/ui/touch/tabs/touch_tab_strip.h:105: int initial_scroll_offset_; we should be moving towards floats... all these things can be transformed. http://codereview.chromium.org/6750007/diff/5001/chrome/browser/ui/views/tabs... File chrome/browser/ui/views/tabs/base_tab_strip.cc (right): http://codereview.chromium.org/6750007/diff/5001/chrome/browser/ui/views/tabs... chrome/browser/ui/views/tabs/base_tab_strip.cc:397: remove
http://codereview.chromium.org/6750007/diff/5001/chrome/browser/ui/touch/tabs... File chrome/browser/ui/touch/tabs/touch_tab_strip.cc (right): http://codereview.chromium.org/6750007/diff/5001/chrome/browser/ui/touch/tabs... chrome/browser/ui/touch/tabs/touch_tab_strip.cc:169: bool TouchTabStrip::OnMousePressed(const views::MouseEvent& event) { On 2011/03/28 22:07:40, rjkroege wrote: > these need unit tests. Unit tests are 100% completely new territory for me. http://codereview.chromium.org/6750007/diff/5001/chrome/browser/ui/touch/tabs... chrome/browser/ui/touch/tabs/touch_tab_strip.cc:171: BeginScroll(event.location()); On 2011/03/28 22:07:40, rjkroege wrote: > I thought we had discussed creating synthetic events and dispatching them from > the gesture manager? perhaps you should add a todo for this? We did. I have added a TODO comment. http://codereview.chromium.org/6750007/diff/5001/chrome/browser/ui/touch/tabs... chrome/browser/ui/touch/tabs/touch_tab_strip.cc:220: void TouchTabStrip::ScrollTo(int delta_x) { On 2011/03/28 22:07:40, rjkroege wrote: > I would have thought that there should be a schedulepaint downstream from here? > Have I missed it? And use a transformable view? Or at least take a TODO to use > one? This function is just doing the math for setting the scroll offset. I don't know what the overhead of scheduling a paint is. http://codereview.chromium.org/6750007/diff/5001/chrome/browser/ui/touch/tabs... File chrome/browser/ui/touch/tabs/touch_tab_strip.h (right): http://codereview.chromium.org/6750007/diff/5001/chrome/browser/ui/touch/tabs... chrome/browser/ui/touch/tabs/touch_tab_strip.h:75: void BeginScroll(const gfx::Point& point); On 2011/03/28 22:07:40, rjkroege wrote: > not to make your life difficult... maybe there could be an abstract scroller > mixin in some future CL? I'm assuming you're talking about the gestures for scrolls. I shall add a todo. http://codereview.chromium.org/6750007/diff/5001/chrome/browser/ui/touch/tabs... chrome/browser/ui/touch/tabs/touch_tab_strip.h:105: int initial_scroll_offset_; On 2011/03/28 22:07:40, rjkroege wrote: > we should be moving towards floats... all these things can be transformed. Noted, but will input devices operate in the float domain? http://codereview.chromium.org/6750007/diff/5001/chrome/browser/ui/views/tabs... File chrome/browser/ui/views/tabs/base_tab_strip.cc (right): http://codereview.chromium.org/6750007/diff/5001/chrome/browser/ui/views/tabs... chrome/browser/ui/views/tabs/base_tab_strip.cc:397: On 2011/03/28 22:07:40, rjkroege wrote: > remove Done.
I also note that I'm getting bogus redraw behavior when changing focus to another app. I don't know why yet. :(
On 2011/03/30 15:55:02, wyck wrote: > I also note that I'm getting bogus redraw behavior when changing focus to > another app. I don't know why yet. :( http://codereview.chromium.org/6691065/ fixes the redraw bug.
Redraw bug is fixed. What's holding up this CL now? http://codereview.chromium.org/6750007/diff/5001/chrome/browser/ui/touch/tabs... File chrome/browser/ui/touch/tabs/touch_tab_strip.cc (right): http://codereview.chromium.org/6750007/diff/5001/chrome/browser/ui/touch/tabs... chrome/browser/ui/touch/tabs/touch_tab_strip.cc:169: bool TouchTabStrip::OnMousePressed(const views::MouseEvent& event) { On 2011/03/30 15:53:11, wyck wrote: > On 2011/03/28 22:07:40, rjkroege wrote: > > these need unit tests. > Unit tests are 100% completely new territory for me. Is this what's holding up this CL?
http://codereview.chromium.org/6750007/diff/19001/chrome/browser/ui/touch/tab... File chrome/browser/ui/touch/tabs/touch_tab.cc (right): http://codereview.chromium.org/6750007/diff/19001/chrome/browser/ui/touch/tab... chrome/browser/ui/touch/tabs/touch_tab.cc:61: // is need to directly handle the mouse movements in the TouchTab. This last sentence is incomplete. http://codereview.chromium.org/6750007/diff/19001/chrome/browser/ui/touch/tab... File chrome/browser/ui/touch/tabs/touch_tab_strip.cc (right): http://codereview.chromium.org/6750007/diff/19001/chrome/browser/ui/touch/tab... chrome/browser/ui/touch/tabs/touch_tab_strip.cc:7: #include <cmath> Can these be sorted? http://codereview.chromium.org/6750007/diff/19001/chrome/browser/ui/touch/tab... chrome/browser/ui/touch/tabs/touch_tab_strip.cc:213: // and layout This comment doesn't make sense. http://codereview.chromium.org/6750007/diff/19001/chrome/browser/ui/touch/tab... chrome/browser/ui/touch/tabs/touch_tab_strip.cc:226: if (tab && tab == initial_tab_) Don't you always want to select the tab at the location the user released the mouse? http://codereview.chromium.org/6750007/diff/19001/chrome/browser/ui/views/tab... File chrome/browser/ui/views/tabs/base_tab_strip.cc (right): http://codereview.chromium.org/6750007/diff/19001/chrome/browser/ui/views/tab... chrome/browser/ui/views/tabs/base_tab_strip.cc:384: BaseTab* BaseTabStrip::GetTabAtLocal( Position needs to match header.
Using OnMouseCaptureLost now. Also applying fix-ups as per review. http://codereview.chromium.org/6750007/diff/19001/chrome/browser/ui/touch/tab... File chrome/browser/ui/touch/tabs/touch_tab.cc (right): http://codereview.chromium.org/6750007/diff/19001/chrome/browser/ui/touch/tab... chrome/browser/ui/touch/tabs/touch_tab.cc:61: // is need to directly handle the mouse movements in the TouchTab. On 2011/04/06 15:11:50, sky wrote: > This last sentence is incomplete. Fixed http://codereview.chromium.org/6750007/diff/19001/chrome/browser/ui/touch/tab... File chrome/browser/ui/touch/tabs/touch_tab_strip.cc (right): http://codereview.chromium.org/6750007/diff/19001/chrome/browser/ui/touch/tab... chrome/browser/ui/touch/tabs/touch_tab_strip.cc:7: #include <cmath> On 2011/04/06 15:11:50, sky wrote: > Can these be sorted? Done. http://codereview.chromium.org/6750007/diff/19001/chrome/browser/ui/touch/tab... chrome/browser/ui/touch/tabs/touch_tab_strip.cc:213: // and layout On 2011/04/06 15:11:50, sky wrote: > This comment doesn't make sense. Right, sorry. That was a note to myself to move the DoLayout call in here. I've updated that. http://codereview.chromium.org/6750007/diff/19001/chrome/browser/ui/touch/tab... chrome/browser/ui/touch/tabs/touch_tab_strip.cc:226: if (tab && tab == initial_tab_) On 2011/04/06 15:11:50, sky wrote: > Don't you always want to select the tab at the location the user released the > mouse? No, I was thinking it works like a button, where if you mouse press, then move off it, then mouse release, it doesn't count as being clicked. http://codereview.chromium.org/6750007/diff/19001/chrome/browser/ui/views/tab... File chrome/browser/ui/views/tabs/base_tab_strip.cc (right): http://codereview.chromium.org/6750007/diff/19001/chrome/browser/ui/views/tab... chrome/browser/ui/views/tabs/base_tab_strip.cc:384: BaseTab* BaseTabStrip::GetTabAtLocal( On 2011/04/06 15:11:50, sky wrote: > Position needs to match header. Yuck. That's uniform, but disorganized. But I'll move it in the spirit of consistent style.
LGTM
LGTM. But I still want tests. Please.
Try job failure for 6750007-22002 on win: http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...
I need help understanding this. It looks like there was some webkit thread test on windows that failed. I don't see what this has to do with my change. - Chad On Fri, Apr 8, 2011 at 10:26 AM, <commit-bot@chromium.org> wrote: > Try job failure for 6750007-22002 on win: > http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number... > > http://codereview.chromium.org/6750007/ >
Try job failure for 6750007-22002 on mac: http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac&number...
So goes it with the tests. Some times you get failures unrelated to your patch. You can always try again, or use -r to make sure you get a specific version. That doesn't ensure you see no failures though... -Scott On Fri, Apr 8, 2011 at 7:50 AM, Chad Faragher <wyck@chromium.org> wrote: > I need help understanding this. It looks like there was some webkit > thread test on windows that failed. I don't see what this has to do > with my change. > > - Chad > > On Fri, Apr 8, 2011 at 10:26 AM, <commit-bot@chromium.org> wrote: >> Try job failure for 6750007-22002 on win: >> http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number... >> >> http://codereview.chromium.org/6750007/ >> >
Change committed as 80945
This is the first time I've used this site, so please excuse me if this is the incorrect place for these comments. I am looking forward to this feature and have 2 questions. 1. When will the Chrome that one downloads have this feature incorporated into it? 2. Will there be an easy way to configure the minimum/maximum size of the tabs? FireFox 4 removed this for no apparent reason and I'm hoping it'll be put into Chrome with the scrolling option. :) (See https://bugzilla.mozilla.org/show_bug.cgi?id=574654 to read about a bunch of people complaining about it.) IMO, it'd be a good way to get people to migrate to Chrome, myself included. :)
Probably not the proper place to comment, but I sincerely hope Chrome gets scrolling tabs. I HATE Firefox, but use it only because I have over 200 tabs at any moment, and in Chrome they are all blank. Finding tabs is impossible without a LOT of wasted time and frustration. Firefox has had this feature for years.
It's on my list of things to get to. Hopefully soon. -Scott On Fri, Jun 24, 2011 at 10:51 AM, <CharlesBailey11@gmail.com> wrote: > Probably not the proper place to comment, but I sincerely hope Chrome gets > scrolling tabs. I HATE Firefox, but use it only because I have over 200 tabs > at > any moment, and in Chrome they are all blank. Finding tabs is impossible > without > a LOT of wasted time and frustration. Firefox has had this feature for > years. > > http://codereview.chromium.org/6750007/ >
(Obligatory response: Is it soon yet? :P) I'm sure a lot of people will appreciate this feature, myself included. Just a note to nudge interest/priority, if that's possible. :) -Grant On 2011/06/24 18:15:51, sky wrote: > It's on my list of things to get to. Hopefully soon. > > -Scott > > On Fri, Jun 24, 2011 at 10:51 AM, <mailto:CharlesBailey11@gmail.com> wrote: > > Probably not the proper place to comment, but I sincerely hope Chrome gets > > scrolling tabs. I HATE Firefox, but use it only because I have over 200 tabs > > at > > any moment, and in Chrome they are all blank. Finding tabs is impossible > > without > > a LOT of wasted time and frustration. Firefox has had this feature for > > years. > > > > http://codereview.chromium.org/6750007/ > > |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
