Experiments with a new tabstrip + animator

Basics of a new TabStrip.It's very, very rough, but I wanted to check it in so I don't have to keep typing svn pset as I pass patches back and forth between machines.Behind a command line flag --enable-tabtastic2.I'm trying to split the TabContents specific stuff off of the TabStrip so it's more generic (and more easily mocked for unit testing of various layout conditions). Hence TabStrip vs. BrowserTabStrip. TabStrip may move into views/ once this process is complete.Animator is a utility that can be associated with a View that (at this point) animates that View's bounds from wherever it is now to somewhere else. The TabStrip uses this to do animations for individual Tabs that are independent of each other - a limitation of the old TabStrip is that only one animation is ever active at a time so its animations are a little jumpy compared to other products.Also, detached tab dragging shows the live contents, with all animations/video/etc.Like I said, this is really rough, but I didn't want it to grow any bigger. I will write up a design doc later.http://crbug.com/9032TEST=TBD... will finally be doing some for TabStrip layout!

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=20053

[sky, 2009-07-02 20:42:33]

The one thing I don't like about this approach is that you have a
timer per animator. Depending upon scheduling that means it's possible
for you to start a bunch of views::Animators during the same message
processing yet have them all fire at different times, resulting in
more painting/layout and visual jank. Perhaps this is why you needed
clamping?

Ideally you could group all the animations created during processing
a single message into a single timer. That way everything is updated
at once, with less janky painting and resize behavior. You might be
able to make this seamless to users of the API by way of a
MessageLoopForUI::Observer that is only installed once for all
Animators and handles grouping them into single units.

I lost steam after about an hour and didn't really make it through the tab_strip
side of things. I'll look again later when I'm reinvigorated.

http://codereview.chromium.org/42490/diff/6081/7138
File app/win_util.h (right):

http://codereview.chromium.org/42490/diff/6081/7138#newcode289
Line 289: gfx::Point GetCursorScreenPoint();
This exists in views::Screen.

http://codereview.chromium.org/42490/diff/6081/7160
File chrome/browser/browser_window.h (right):

http://codereview.chromium.org/42490/diff/6081/7160#newcode198
Line 198: // XXX
nit: description.

http://codereview.chromium.org/42490/diff/6081/7162
File chrome/browser/tabs/tab_strip_model.h (right):

http://codereview.chromium.org/42490/diff/6081/7162#newcode133
Line 133: // XXX
This seems more like a Create rather the Continue.

http://codereview.chromium.org/42490/diff/6081/7146
File chrome/browser/views/frame/browser_frame_win.h (right):

http://codereview.chromium.org/42490/diff/6081/7146#newcode105
Line 105: // True if the window is currently being moved in a detached tab drag\
nit: nuke \

http://codereview.chromium.org/42490/diff/6081/7146#newcode109
Line 109: // XXX
nit: comments

http://codereview.chromium.org/42490/diff/6081/7154
File chrome/browser/views/tabs/tab_2.h (right):

http://codereview.chromium.org/42490/diff/6081/7154#newcode30
Line 30: virtual std::wstring GetTitle(Tab2* tab) const = 0;
string16

http://codereview.chromium.org/42490/diff/6081/7156
File chrome/browser/views/tabs/tab_strip_2.h (right):

http://codereview.chromium.org/42490/diff/6081/7156#newcode25
Line 25: virtual std::wstring GetTitle(int index) const = 0;
string16

http://codereview.chromium.org/42490/diff/6081/7156#newcode90
Line 90: virtual std::wstring GetTitle(Tab2* tab) const;
string16

http://codereview.chromium.org/42490/diff/6081/7156#newcode112
Line 112: enum LayoutSource {
nit: should be first in private section.

http://codereview.chromium.org/42490/diff/6081/7144
File views/animator.cc (right):

http://codereview.chromium.org/42490/diff/6081/7144#newcode19
Line 19: animation_(NULL),
nit: no need to have animation_(NULL).

http://codereview.chromium.org/42490/diff/6081/7144#newcode46
Line 46: direction_ = direction;
Should you stop the animation in case you return out?

http://codereview.chromium.org/42490/diff/6081/7144#newcode80
Line 80: // Start the animation from the beginning if it's already running.
nit: nuke 'if it's already running'. You always restart the animation.

http://codereview.chromium.org/42490/diff/6081/7144#newcode115
Line 115: host_->GetParent()->SchedulePaint();
You need a SchedulePaint before you change the bounds too. Otherwise you won't
paint the old bounds.

http://codereview.chromium.org/42490/diff/6081/7144#newcode135
Line 135: return previous_view->bounds().right();
Do you need to do anything special for RTL?

http://codereview.chromium.org/42490/diff/6081/7145
File views/animator.h (right):

http://codereview.chromium.org/42490/diff/6081/7145#newcode32
Line 32: virtual View* GetPreviousViewForAnimator(View* host) = 0;
Rather than naming this previous, how about ClampedView. That way it ties the
enum and method name together and makes it more obvious what this is for.

http://codereview.chromium.org/42490/diff/6081/7145#newcode40
Line 40: // created, the lifetime of an animator corresponds to its host view.
I'm confused by what you mean by the 'lifetime corresponds to its host view.' I
thought you meant the view would take ownership of the animator and delete it
when the view is deleted, but I don't see that. Could you make this clearer.

http://codereview.chromium.org/42490/diff/6081/7145#newcode53
Line 53: enum BoundsChangeFlags {
nit: C++ style guide says enums should be first, before constructors.

http://codereview.chromium.org/42490/diff/6081/7145#newcode59
Line 59: ANIMATE_CLAMP = 0x10
ANIMATE_CLAMP isn't that obvious, how about a description. I suspect ANIMATE_X |
ANIMATE_Y | ANIMATE_WIDTH | ANIMATE_HEIGHT is going to be the common one. How
about ANIMATE_ALL for that.

http://codereview.chromium.org/42490/diff/6081/7145#newcode65
Line 65: void SetBounds(const gfx::Rect& bounds, int direction);
Might I suggest AnimateToBounds rather than SetBounds?

http://codereview.chromium.org/42490/diff/6081/7142
File views/widget/widget.h (right):

http://codereview.chromium.org/42490/diff/6081/7142#newcode102
Line 102: virtual void StartDragForView(View* view, const gfx::Point& point) =
0;
From the name and description I thought this would start a drag and drop
operation, but it only seems to generate a mouse pressed. Perhaps it should be
GenerateMousePressedForView?

[Ben Goodger (Google), 2009-07-03 03:42:27]

http://codereview.chromium.org/42490/diff/6081/7162
File chrome/browser/tabs/tab_strip_model.h (right):

http://codereview.chromium.org/42490/diff/6081/7162#newcode133
Line 133: // XXX
On 2009/07/02 20:42:33, sky wrote:
> This seems more like a Create rather the Continue.

It's not really a Create though it doesn't return anything. The idea is that the
TabStrip hands over to its delegate (Browser) to continue a drag session once it
can no longer perform it (i.e. once the drag has moved out of its bounds). I've
provided detailed comments in my latest update.

http://codereview.chromium.org/42490/diff/6081/7156
File chrome/browser/views/tabs/tab_strip_2.h (right):

http://codereview.chromium.org/42490/diff/6081/7156#newcode112
Line 112: enum LayoutSource {
On 2009/07/02 20:42:33, sky wrote:
> nit: should be first in private section.

That breaks the coherent block of overrides and implementations. This is more
tightly bound with the functions that follow. The style guide says "generally
should be in the following order" rather than "should be in the following order"
;-)

http://codereview.chromium.org/42490/diff/6081/7144
File views/animator.cc (right):

http://codereview.chromium.org/42490/diff/6081/7144#newcode115
Line 115: host_->GetParent()->SchedulePaint();
On 2009/07/02 20:42:33, sky wrote:
> You need a SchedulePaint before you change the bounds too. Otherwise you won't
> paint the old bounds.

This is inefficient right now - it just paints the TabStrip2, i.e. the parent
(GetParent()).

Worrying about performance in a future pass.

http://codereview.chromium.org/42490/diff/6081/7144#newcode135
Line 135: return previous_view->bounds().right();
On 2009/07/02 20:42:33, sky wrote:
> Do you need to do anything special for RTL?

Will worry about that later.

http://codereview.chromium.org/42490/diff/6081/7145
File views/animator.h (right):

http://codereview.chromium.org/42490/diff/6081/7145#newcode40
Line 40: // created, the lifetime of an animator corresponds to its host view.
On 2009/07/02 20:42:33, sky wrote:
> I'm confused by what you mean by the 'lifetime corresponds to its host view.'
I
> thought you meant the view would take ownership of the animator and delete it
> when the view is deleted, but I don't see that. Could you make this clearer.

The Tab2 uses a scoped_ptr for Animator. I changed this comment to use the term
"own" rather than "lifetime matches".

http://codereview.chromium.org/42490/diff/6081/7145#newcode65
Line 65: void SetBounds(const gfx::Rect& bounds, int direction);
On 2009/07/02 20:42:33, sky wrote:
> Might I suggest AnimateToBounds rather than SetBounds?

I was mimicking CoreAnimation here, which does this:

[[view animator] setFrame:NSRect]

which is the animated version of:

[view setFrame:NSRect]

As we add more functions to Animator, such as alpha, etc. do we prefer
AnimateToAlpha etc? I don't really care one way or another.

[sky, 2009-07-06 21:09:44]

I suggested Create instead of Continue as browser's implementation creates a new
browser.

I like AnimateToBounds better as it's clearer to me, but I'm not passionate
about it. If you like what you have better, stick with it.

http://codereview.chromium.org/42490/diff/9041/8035
File chrome/browser/views/tabs/tab_strip_2.cc (right):

http://codereview.chromium.org/42490/diff/9041/8035#newcode43
Line 43: tabs_.insert(tabs_.begin() + insertion_index, 1, tab);
nit: nuke the arg '1'.

http://codereview.chromium.org/42490/diff/9041/8035#newcode57
Line 57: LayoutImpl(LS_TAB_REMOVE);
Do you need a SchedulePaint here?

http://codereview.chromium.org/42490/diff/9041/8035#newcode69
Line 69: tabs_.insert(tabs_.begin() + GetInternalIndex(to_index), 1, tab);
nit: nuke the arg '1'.

http://codereview.chromium.org/42490/diff/9041/8035#newcode70
Line 70: LayoutImpl(LS_TAB_DRAG_REORDER);
Do you need a SchedulePaint here?

http://codereview.chromium.org/42490/diff/9041/8035#newcode91
Line 91: gfx::Rect tab_bounds = GetTabAt(i)->bounds();
Should this ignore removing tabs?

http://codereview.chromium.org/42490/diff/9041/8035#newcode100
Line 100: if (tab_right_half.Contains(point))
Missing an && here?

http://codereview.chromium.org/42490/diff/9041/8035#newcode120
Line 120: LayoutImpl(LS_TAB_DRAG_REORDER);
SchedulePaint?

http://codereview.chromium.org/42490/diff/9041/8035#newcode332
Line 332: int child_width = width() / child_count;
I love the new code over the doubles, but didn't you move to doubles so that you
distribute all the available width? I believe this code has the ability to leave
a gap on the right edge of (child_count - 1) pixels. I think you can get this to
work with ints, but it requires a little trickery to distribute the extra
pixels.

[Ben Goodger (Google), 2009-07-07 00:47:15]

I tend to expect CreateFoo functions to create a new Foo object and return it to
me for me to own. That's not quite the semantic of ContinueDragging...

I renamed SetBounds to AnimateToBounds.

http://codereview.chromium.org/42490/diff/9041/8035
File chrome/browser/views/tabs/tab_strip_2.cc (right):

http://codereview.chromium.org/42490/diff/9041/8035#newcode57
Line 57: LayoutImpl(LS_TAB_REMOVE);
On 2009/07/06 21:09:44, sky wrote:
> Do you need a SchedulePaint here?

Paint scheduling is done by the animate operations as required.

http://codereview.chromium.org/42490/diff/9041/8035#newcode70
Line 70: LayoutImpl(LS_TAB_DRAG_REORDER);
On 2009/07/06 21:09:44, sky wrote:
> Do you need a SchedulePaint here?

See above.

http://codereview.chromium.org/42490/diff/9041/8035#newcode332
Line 332: int child_width = width() / child_count;
I bet this has all the problems the old one had. My intent is to bring across
the double stuff once I get more of the mechanics sorted ;-)

[sky, 2009-07-07 16:02:57]

LGTM