Streamline the layout of the BrowserView's children TabContents views....

Streamline the layout of the BrowserView's children TabContents views.
Modify SingleSplitView to calculate its children view's bounds, but do not actually resize them
and change BrowserViewLayout accordingly (BrowserViewLayout resizes all views now).
Do all reserved contents rect calculations before resizing TabContents views.

Rationale: to do all BrowserView layout related actions in the context of
BrowserViewLayout::Layout call and to minimize actual contents re-layouts.

BUG=51084
TEST=All tests should pass


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

[Ben Goodger (Google), 2011-01-06 17:05:19]

Scott can look at the SingleSplitView stuff... the integration in BrowserView
does look like an improvement though so LGTM for that.

[sky, 2011-01-06 17:55:02]

http://codereview.chromium.org/5606012/diff/11001/chrome/browser/ui/views/fra...
File chrome/browser/ui/views/frame/browser_view.cc (right):

http://codereview.chromium.org/5606012/diff/11001/chrome/browser/ui/views/fra...
chrome/browser/ui/views/frame/browser_view.cc:1819: Layout();
You should mark the children of the splitter as needing to be layed out,
otherwise there may be problem cases where things don't layout out correctly.
Also, this should invoke SchedulePaint on view.

http://codereview.chromium.org/5606012/diff/11001/views/controls/single_split...
File views/controls/single_split_view.cc (right):

http://codereview.chromium.org/5606012/diff/11001/views/controls/single_split...
views/controls/single_split_view.cc:65: if (GetChildViewCount() != 2) {
This code is fairly finicky. Please write some tests so that we have coverage.

http://codereview.chromium.org/5606012/diff/11001/views/controls/single_split...
views/controls/single_split_view.cc:67: // Otherwise don't touch these rects to
avoid unnecessary resizes.
I don't understand this comment. Shouldn't you always reset the leading/trailing
rects here (and at 77) so that callers don't get stale data?

http://codereview.chromium.org/5606012/diff/11001/views/controls/single_split...
views/controls/single_split_view.cc:84: int divide_at;
dividier_at

http://codereview.chromium.org/5606012/diff/11001/views/controls/single_split...
views/controls/single_split_view.cc:114: // Invoke super's implementation so
that the children are layed out.
If Layout was invoked because the bounds of the singlesplitview changed, then at
this point the two views have not been correctly resized.

You should refactor the code so that you have a method that will tell you where
to layout things out, perhaps:

void CalculateChildrenBounds(gfx::Rect* leading_bounds, gfx::Rect*
trailing_bounds);

Layout would call this and set the bounds. BrowserViewLayout would also call
this method if it needs the bounds. When the splitter is moved conditionally
call Layout (BrowserView would return a value indicating no Layout).

http://codereview.chromium.org/5606012/diff/11001/views/controls/single_split...
views/controls/single_split_view.cc:162:
GetChildViewAt(0)->SetBounds(leading_view_rect());
Only do this if the views are visible.

http://codereview.chromium.org/5606012/diff/11001/views/controls/single_split...
File views/controls/single_split_view.h (right):

http://codereview.chromium.org/5606012/diff/11001/views/controls/single_split...
views/controls/single_split_view.h:18: // to handle user initiated layout
changes.
The Observer should be optional, a NULL observer means layout happens
immediately.

http://codereview.chromium.org/5606012/diff/11001/views/controls/single_split...
views/controls/single_split_view.h:28: // Invoked when split handle is moved by
the user.
This should return a boolean indicating if Layout should be invoked. BrowserView
would return false.

http://codereview.chromium.org/5606012/diff/11001/views/controls/single_split...
views/controls/single_split_view.h:109: // Leading and trailing views bounds
calcualted based on the current
Newline between 108 and 109.

http://codereview.chromium.org/5606012/diff/11001/views/controls/single_split...
views/controls/single_split_view.h:109: // Leading and trailing views bounds
calcualted based on the current
calcualted - > calculated

[Aleksey Shlyapnikov, 2011-01-07 19:07:23]

http://codereview.chromium.org/5606012/diff/11001/chrome/browser/ui/views/fra...
File chrome/browser/ui/views/frame/browser_view.cc (right):

http://codereview.chromium.org/5606012/diff/11001/chrome/browser/ui/views/fra...
chrome/browser/ui/views/frame/browser_view.cc:1819: Layout();
Even if we know this view uses custom layout manager? Shouldn't it supposed to
be handling all these issues (children views will be resized anyway)? It's seems
excessive, but done.

On 2011/01/06 17:55:02, sky wrote:
> You should mark the children of the splitter as needing to be layed out,
> otherwise there may be problem cases where things don't layout out correctly.
> Also, this should invoke SchedulePaint on view.

http://codereview.chromium.org/5606012/diff/11001/views/controls/single_split...
File views/controls/single_split_view.cc (right):

http://codereview.chromium.org/5606012/diff/11001/views/controls/single_split...
views/controls/single_split_view.cc:65: if (GetChildViewCount() != 2) {
Reworked this code to reduce finickiness.

On 2011/01/06 17:55:02, sky wrote:
> This code is fairly finicky. Please write some tests so that we have coverage.

http://codereview.chromium.org/5606012/diff/11001/views/controls/single_split...
views/controls/single_split_view.cc:67: // Otherwise don't touch these rects to
avoid unnecessary resizes.
On 2011/01/06 17:55:02, sky wrote:
> I don't understand this comment. Shouldn't you always reset the
leading/trailing
> rects here (and at 77) so that callers don't get stale data?

Done.

http://codereview.chromium.org/5606012/diff/11001/views/controls/single_split...
views/controls/single_split_view.cc:84: int divide_at;
On 2011/01/06 17:55:02, sky wrote:
> dividier_at

Done.

http://codereview.chromium.org/5606012/diff/11001/views/controls/single_split...
views/controls/single_split_view.cc:114: // Invoke super's implementation so
that the children are layed out.
On 2011/01/06 17:55:02, sky wrote:
> If Layout was invoked because the bounds of the singlesplitview changed, then
at
> this point the two views have not been correctly resized.
> 
> You should refactor the code so that you have a method that will tell you
where
> to layout things out, perhaps:
> 
> void CalculateChildrenBounds(gfx::Rect* leading_bounds, gfx::Rect*
> trailing_bounds);
> 
> Layout would call this and set the bounds. BrowserViewLayout would also call
> this method if it needs the bounds. When the splitter is moved conditionally
> call Layout (BrowserView would return a value indicating no Layout).

Done.

http://codereview.chromium.org/5606012/diff/11001/views/controls/single_split...
views/controls/single_split_view.cc:162:
GetChildViewAt(0)->SetBounds(leading_view_rect());
On 2011/01/06 17:55:02, sky wrote:
> Only do this if the views are visible.

Done.

http://codereview.chromium.org/5606012/diff/11001/views/controls/single_split...
File views/controls/single_split_view.h (right):

http://codereview.chromium.org/5606012/diff/11001/views/controls/single_split...
views/controls/single_split_view.h:18: // to handle user initiated layout
changes.
On 2011/01/06 17:55:02, sky wrote:
> The Observer should be optional, a NULL observer means layout happens
> immediately.

Done.

http://codereview.chromium.org/5606012/diff/11001/views/controls/single_split...
views/controls/single_split_view.h:28: // Invoked when split handle is moved by
the user.
In the updated version SingleSplitView does layout only when observer is not
provided, otherwise it's a observer responsibility to lay out children views.

On 2011/01/06 17:55:02, sky wrote:
> This should return a boolean indicating if Layout should be invoked.
BrowserView
> would return false.

http://codereview.chromium.org/5606012/diff/11001/views/controls/single_split...
views/controls/single_split_view.h:109: // Leading and trailing views bounds
calcualted based on the current
On 2011/01/06 17:55:02, sky wrote:
> Newline between 108 and 109.

Done.

http://codereview.chromium.org/5606012/diff/11001/views/controls/single_split...
views/controls/single_split_view.h:109: // Leading and trailing views bounds
calcualted based on the current
On 2011/01/06 17:55:02, sky wrote:
> Newline between 108 and 109.

Done.

[sky, 2011-01-07 21:35:40]

http://codereview.chromium.org/5606012/diff/11001/chrome/browser/ui/views/fra...
File chrome/browser/ui/views/frame/browser_view.cc (right):

http://codereview.chromium.org/5606012/diff/11001/chrome/browser/ui/views/fra...
chrome/browser/ui/views/frame/browser_view.cc:1819: Layout();
On 2011/01/07 19:07:23, Aleksey Shlyapnikov wrote:
> Even if we know this view uses custom layout manager? Shouldn't it supposed to
> be handling all these issues (children views will be resized anyway)? It's
seems
> excessive, but done.
> 
> On 2011/01/06 17:55:02, sky wrote:
> > You should mark the children of the splitter as needing to be layed out,
> > otherwise there may be problem cases where things don't layout out
correctly.
> > Also, this should invoke SchedulePaint on view.
> 

This code invokes Layout rather high in the hierarchy. Layout ends up doing
nothing if a view isn't marked as needing layout and it's bounds haven't
changed. That could easily be the case here.

http://codereview.chromium.org/5606012/diff/24001/views/controls/single_split...
File views/controls/single_split_view.cc (right):

http://codereview.chromium.org/5606012/diff/24001/views/controls/single_split...
views/controls/single_split_view.cc:50: divider_offset_ =
NormalizeDividerOffset(divider_offset_);
I think this'll do the wrong thing on a large contraction. For example, if the
width is 1000, divider at 800 and you resize to 400 you want the divider at 200,
but I believe this makes it go to 0.

http://codereview.chromium.org/5606012/diff/24001/views/controls/single_split...
views/controls/single_split_view.cc:66: if (observer_)
I'm confused by this. Why isn't layout always doing a layout? I thought what you
cared about was having the observer control where layout happens, but this
should still do a layout.

http://codereview.chromium.org/5606012/diff/24001/views/controls/single_split...
views/controls/single_split_view.cc:131: *leading_bounds = gfx::Rect();
nit: set the bounds to empty first thing. That way you don't care about someone
forgetting to reset them later on.

http://codereview.chromium.org/5606012/diff/24001/views/controls/single_split...
File views/controls/single_split_view.h (right):

http://codereview.chromium.org/5606012/diff/24001/views/controls/single_split...
views/controls/single_split_view.h:37: SingleSplitView(View* leading, View*
trailing, Orientation orientation,
each arg on its own line.

[Aleksey Shlyapnikov, 2011-01-07 23:06:00]

http://codereview.chromium.org/5606012/diff/24001/views/controls/single_split...
File views/controls/single_split_view.cc (right):

http://codereview.chromium.org/5606012/diff/24001/views/controls/single_split...
views/controls/single_split_view.cc:50: divider_offset_ =
NormalizeDividerOffset(divider_offset_);
On 2011/01/07 21:35:40, sky wrote:
> I think this'll do the wrong thing on a large contraction. For example, if the
> width is 1000, divider at 800 and you resize to 400 you want the divider at
200,
> but I believe this makes it go to 0.

Thanks for catching this, fixed.

http://codereview.chromium.org/5606012/diff/24001/views/controls/single_split...
views/controls/single_split_view.cc:66: if (observer_)
On 2011/01/07 21:35:40, sky wrote:
> I'm confused by this. Why isn't layout always doing a layout? I thought what
you
> cared about was having the observer control where layout happens, but this
> should still do a layout.

Correct, but it's a bit more complex than that. BrowserViewLayout first resizes
the split views, inquires its children future bounds, adjusts reserved rects and
only then resizes children views. Just calling Layout there (basically, resizing
the split view and letting it to resize its children) will require another const
function to be added to SingleSplitView,
CalculateChildrenBoundsForSplitViewBounds combining DidChangeBounds and
CalculateChildrenBounds functionality, call it for both split views first,
update contents reserved rects based on returned future bounds with a bit more
calculations than we do now since split views themselves are not moved yet and
only then resize split views.
My initial idea was to explicitly set all views bounds in BrowserViewLayout, let
me try the alternative approach to and see whether it's more natural.

http://codereview.chromium.org/5606012/diff/24001/views/controls/single_split...
views/controls/single_split_view.cc:131: *leading_bounds = gfx::Rect();
On 2011/01/07 21:35:40, sky wrote:
> nit: set the bounds to empty first thing. That way you don't care about
someone
> forgetting to reset them later on.

This is a very rare case, why should we penalize the normal code path by adding
unnecessary assignments?

http://codereview.chromium.org/5606012/diff/24001/views/controls/single_split...
File views/controls/single_split_view.h (right):

http://codereview.chromium.org/5606012/diff/24001/views/controls/single_split...
views/controls/single_split_view.h:37: SingleSplitView(View* leading, View*
trailing, Orientation orientation,
On 2011/01/07 21:35:40, sky wrote:
> each arg on its own line.

Done.

[Aleksey Shlyapnikov, 2011-01-08 23:54:40]

http://codereview.chromium.org/5606012/diff/24001/views/controls/single_split...
File views/controls/single_split_view.cc (right):

http://codereview.chromium.org/5606012/diff/24001/views/controls/single_split...
views/controls/single_split_view.cc:66: if (observer_)
On 2011/01/07 23:06:00, Aleksey Shlyapnikov wrote:
> On 2011/01/07 21:35:40, sky wrote:
> > I'm confused by this. Why isn't layout always doing a layout? I thought what
> you
> > cared about was having the observer control where layout happens, but this
> > should still do a layout.
> 
> Correct, but it's a bit more complex than that. BrowserViewLayout first
resizes
> the split views, inquires its children future bounds, adjusts reserved rects
and
> only then resizes children views. Just calling Layout there (basically,
resizing
> the split view and letting it to resize its children) will require another
const
> function to be added to SingleSplitView,
> CalculateChildrenBoundsForSplitViewBounds combining DidChangeBounds and
> CalculateChildrenBounds functionality, call it for both split views first,
> update contents reserved rects based on returned future bounds with a bit more
> calculations than we do now since split views themselves are not moved yet and
> only then resize split views.
> My initial idea was to explicitly set all views bounds in BrowserViewLayout,
let
> me try the alternative approach to and see whether it's more natural.

Please give it another look, I uploaded the alternative implementation.

[sky, 2011-01-10 16:51:22]

Yes, this looks good. Could you create a unit test to cover some of the subtle
resize behavior?

Thanks,

  -Scott

[sky, 2011-01-10 16:51:57]

I'm giving the LGTM. Feel free to do the unit test in a separate patch if you
want.

  -Scott

[Aleksey Shlyapnikov, 2011-01-11 01:43:19]

Now synced to the latest and with unittest.

[sky, 2011-01-11 17:03:19]

LGTM