Add support for vertical split into SingleSplitView.

Add support for vertical split into SingleSplitView.


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

[Ben Goodger (Google), 2009-06-23 18:04:44]

sky should review.

[sky, 2009-06-23 18:28:02]

http://codereview.chromium.org/146036/diff/1/4
File views/controls/single_split_view.cc (right):

http://codereview.chromium.org/146036/diff/1/4#newcode67
Line 67: height = std::max(height, pref.height());
for vertical split the width should the max of the preferred and the height
summed.

http://codereview.chromium.org/146036/diff/1/4#newcode140
Line 140: x - GetChildViewAt(UILayoutIsRightToLeft() ? 1 : 0)->width();
use {} when you wrap like this.

http://codereview.chromium.org/146036/diff/1/3
File views/controls/single_split_view.h (right):

http://codereview.chromium.org/146036/diff/1/3#newcode45
Line 45: // Returns true if |x| or |y| is over the divider depending on the
split
nit: nuke the part about 'depending on the split type'. It's mildly confusing
and doesn't help the explanation.

http://codereview.chromium.org/146036/diff/1/3#newcode50
Line 50: int GetSplitDimension();
This name is rather confusing. But I think it would be better if you abstracted
this to:

int primary_axis_size(int h, int v) {
  return is_horizontal_ ? h : v;
}

Then in your code rather than
  is_horizontal_ ? event.x() : event.y()

you would have
  primary_axis_size(event.x(), event.y();

If you really want to keep this method, then name it
primary_axis_size() {
  primary_axis_size(width(), height());
}

http://codereview.chromium.org/146036/diff/1/3#newcode62
Line 62: // Orientation of the split view
nit: '.' at the end of the sentence.

[pfeldman, 2009-06-23 18:46:11]

Thanks for the review.

http://codereview.chromium.org/146036/diff/1/4
File views/controls/single_split_view.cc (right):

http://codereview.chromium.org/146036/diff/1/4#newcode67
Line 67: height = std::max(height, pref.height());
On 2009/06/23 18:28:02, sky wrote:
> for vertical split the width should the max of the preferred and the height
> summed.

Done.

http://codereview.chromium.org/146036/diff/1/4#newcode140
Line 140: x - GetChildViewAt(UILayoutIsRightToLeft() ? 1 : 0)->width();
On 2009/06/23 18:28:02, sky wrote:
> use {} when you wrap like this.

Done.

http://codereview.chromium.org/146036/diff/1/3
File views/controls/single_split_view.h (right):

http://codereview.chromium.org/146036/diff/1/3#newcode50
Line 50: int GetSplitDimension();
On 2009/06/23 18:28:02, sky wrote:
> This name is rather confusing. But I think it would be better if you
abstracted
> this to:
> 
> int primary_axis_size(int h, int v) {
>   return is_horizontal_ ? h : v;
> }
> 
> Then in your code rather than
>   is_horizontal_ ? event.x() : event.y()
> 
> you would have
>   primary_axis_size(event.x(), event.y();
> 
> If you really want to keep this method, then name it
> primary_axis_size() {
>   primary_axis_size(width(), height());
> }

Done, but using CamelCase as in code style.

http://codereview.chromium.org/146036/diff/1/3#newcode62
Line 62: // Orientation of the split view
On 2009/06/23 18:28:02, sky wrote:
> nit: '.' at the end of the sentence.

Done.

[sky, 2009-06-23 19:33:19]

LGTM