Misc. cleanup:

Misc. cleanup:

* Shorten/simplify
* unix_hacker()-style functions should be defined inline, others not
* Eliminate unnecessary get()s
* Add more typedefs
* TabStrip has no subclasses, so don't declare anything protected
* Eliminate code that handles unselected tabs being wider than selected tabs
* No else after return
* Fix spelling error
* Definition order should match declaration order

BUG=none
TEST=none
R=sky@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=277819

[Peter Kasting, 2014-06-17 03:16:18]

Some additional notes below.

https://codereview.chromium.org/331343002/diff/1/chrome/browser/ui/views/tabs...
File chrome/browser/ui/views/tabs/tab_drag_controller.cc (right):

https://codereview.chromium.org/331343002/diff/1/chrome/browser/ui/views/tabs...
chrome/browser/ui/views/tabs/tab_drag_controller.cc:800:
GetDraggedViewTabStripBounds(dragged_view_point));
I didn't think the old code was bad, but while working on this code for other
reasons I found myself trying to discover whether |bounds| was used elsewhere,
and figured I might as well remove that question if doing so didn't add any more
code.

https://codereview.chromium.org/331343002/diff/1/chrome/browser/ui/views/tabs...
chrome/browser/ui/views/tabs/tab_drag_controller.cc:1295: index =
(dragged_bounds.right() > last_tab_right) ? tab_count : 0;
Note that this assumes attached_tabstrip_->tab_count() and
GetModel(attached_tabstrip_)->count() are equal.  That's correct, right?

https://codereview.chromium.org/331343002/diff/1/chrome/browser/ui/views/tabs...
File chrome/browser/ui/views/tabs/tab_strip.cc (left):

https://codereview.chromium.org/331343002/diff/1/chrome/browser/ui/views/tabs...
chrome/browser/ui/views/tabs/tab_strip.cc:1178: const bool stacking =
stacked_layout_ || IsStackingDraggedTabs();
Eliminating this relies on your comment over IM that you thought
|stacked_layout_| had to be true whenever IsStackingDraggedTabs() is true.

https://codereview.chromium.org/331343002/diff/1/chrome/browser/ui/views/tabs...
File chrome/browser/ui/views/tabs/tab_strip.cc (right):

https://codereview.chromium.org/331343002/diff/1/chrome/browser/ui/views/tabs...
chrome/browser/ui/views/tabs/tab_strip.cc:160: AnimationEnded(animation);
In the future some of these delegates will do more in AnimationEnded(), so I
figured standardizing on having AnimationCanceled() just call across to that was
better.

https://codereview.chromium.org/331343002/diff/1/chrome/browser/ui/views/tabs...
chrome/browser/ui/views/tabs/tab_strip.cc:736: if ((model_count > 1) &&
(model_index != model_count - 1)) {
This form read a lot more clearly to me than the old way.

https://codereview.chromium.org/331343002/diff/1/chrome/browser/ui/views/tabs...
File chrome/browser/ui/views/tabs/tab_strip.h (right):

https://codereview.chromium.org/331343002/diff/1/chrome/browser/ui/views/tabs...
chrome/browser/ui/views/tabs/tab_strip.h:376: const Tab* GetLastVisibleTab()
const;
In the future, this function will also check tab visibility, hence pulling it
out to its own helper.

[sky, 2014-06-17 15:42:42]

LGTM

https://codereview.chromium.org/331343002/diff/1/chrome/browser/ui/views/tabs...
File chrome/browser/ui/views/tabs/tab_drag_controller.cc (right):

https://codereview.chromium.org/331343002/diff/1/chrome/browser/ui/views/tabs...
chrome/browser/ui/views/tabs/tab_drag_controller.cc:1295: index =
(dragged_bounds.right() > last_tab_right) ? tab_count : 0;
On 2014/06/17 03:16:18, Peter Kasting wrote:
> Note that this assumes attached_tabstrip_->tab_count() and
> GetModel(attached_tabstrip_)->count() are equal.  That's correct, right?

Yes, they should be equal (with the exception of the observer list ordering bugs
you encountered).

https://codereview.chromium.org/331343002/diff/1/chrome/browser/ui/views/tabs...
File chrome/browser/ui/views/tabs/tab_strip.cc (right):

https://codereview.chromium.org/331343002/diff/1/chrome/browser/ui/views/tabs...
chrome/browser/ui/views/tabs/tab_strip.cc:736: if ((model_count > 1) &&
(model_index != model_count - 1)) {
On 2014/06/17 03:16:18, Peter Kasting wrote:
> This form read a lot more clearly to me than the old way.

I've noticed it's mostly just you that prefers the nested () in ifs. The other
change is arguable, but I'm ok either way here.

[Peter Kasting, 2014-06-17 18:11:02]

Landing.

https://codereview.chromium.org/331343002/diff/1/chrome/browser/ui/views/tabs...
File chrome/browser/ui/views/tabs/tab_strip.cc (right):

https://codereview.chromium.org/331343002/diff/1/chrome/browser/ui/views/tabs...
chrome/browser/ui/views/tabs/tab_strip.cc:736: if ((model_count > 1) &&
(model_index != model_count - 1)) {
On 2014/06/17 15:42:42, sky wrote:
> On 2014/06/17 03:16:18, Peter Kasting wrote:
> > This form read a lot more clearly to me than the old way.
> 
> I've noticed it's mostly just you that prefers the nested () in ifs.

I'm fine taking them out.  That wasn't actually relevant to my comment about
being more readable.  It was the other changes that helped for me.

Removed.

[Peter Kasting, 2014-06-17 18:43:12]

Committed patchset #2 manually as r277819 (presubmit successful).