Handle resize corner layout entirely in the platform BrowserWindow*/BrowserView* code...

Handle resize corner layout entirely in the BrowserView (views) or BrowserWindow* (Mac)
and extend RenderViewHost with a concept of reserved contents rect, a place to show extra stuff,
such as Sidebar's mini tab UI.
sidebar UI implementation warranted this change (mini tabs UI and resize corner area for sidebar
contents).

TabContentsDelegate::GetRootWindowResizerRect() is no more, reserved contents area is now cached
in RenderWidgetHostView and updated upon view resize.

Views: BrowserView is responsible for the actual layout and reserved contents area update.

Mac: TabContentsController now manages all TabContents views in the main browser window to solve
two problems, first to prevent contents flickering during tab change not only for the page,
but for the sidebar and devtools contents too and, second, to monitor contents view frame changes
and update reserved contents area accordingly.


BUG=51084
TEST=All tests should pass, this is a refactoring CL.


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

[sky, 2010-09-30 22:50:21]

http://codereview.chromium.org/3547008/diff/1/15
File chrome/browser/renderer_host/render_view_host_delegate.h (right):

http://codereview.chromium.org/3547008/diff/1/15#newcode813
chrome/browser/renderer_host/render_view_host_delegate.h:813: virtual gfx::Rect
GetReservedContentsRect() const;
This (and GetRootWindowResizerRect) should be on View. Please move them.

http://codereview.chromium.org/3547008/diff/1/16
File chrome/browser/renderer_host/render_widget_host.cc (right):

http://codereview.chromium.org/3547008/diff/1/16#newcode261
chrome/browser/renderer_host/render_widget_host.cc:261: const gfx::Rect
view_bounds = view_->GetViewBounds();
informally we don't use const like this. You should do what generally happens in
the file, and in this file const isn't used like this.

http://codereview.chromium.org/3547008/diff/1/16#newcode265
chrome/browser/renderer_host/render_widget_host.cc:265:
GetReservedContentsRect().Union(GetRootWindowResizerRect());
Shouldn't the reserved contents rect include the root window resizer rect?

http://codereview.chromium.org/3547008/diff/1/16#newcode293
chrome/browser/renderer_host/render_widget_host.cc:293: current_reserved_rect_ =
reserved_rect;
Why is it ok to set the reserved_rect immediately here? Also, I don't think you
should set the current_size_.

http://codereview.chromium.org/3547008/diff/1/17
File chrome/browser/renderer_host/render_widget_host.h (right):

http://codereview.chromium.org/3547008/diff/1/17#newcode604
chrome/browser/renderer_host/render_widget_host.h:604: // The reserved area we
last sent to the renderer. |current_reserved_rect_|
newline between 603 and 604.

http://codereview.chromium.org/3547008/diff/1/30
File chrome/browser/views/frame/browser_view.h (left):

http://codereview.chromium.org/3547008/diff/1/30#oldcode281
chrome/browser/views/frame/browser_view.h:281: virtual gfx::Rect
GetRootWindowResizerRect() const;
If you're going to nuke this, could you nuke GetRootWindowResizerRect from every
where?

[Aleksey Shlyapnikov, 2010-10-01 17:28:33]

http://codereview.chromium.org/3547008/diff/1/15
File chrome/browser/renderer_host/render_view_host_delegate.h (right):

http://codereview.chromium.org/3547008/diff/1/15#newcode813
chrome/browser/renderer_host/render_view_host_delegate.h:813: virtual gfx::Rect
GetReservedContentsRect() const;
On 2010/09/30 22:50:21, sky wrote:
> This (and GetRootWindowResizerRect) should be on View. Please move them.

I'm not sure I understand your request. Are you suggesting to directly call
delegate_->GetViewDelegate()->GetRootWindowResizerRect() in RenderViewHost
instead of forwarding this call to delegate_? But only GetRootWindowResizerRect
call goes to view, GetReservedContentsRect is forwarded to TabContents's
delegate, since it's the only entity aware of the reserved area for this
particular renderer.

http://codereview.chromium.org/3547008/diff/1/16
File chrome/browser/renderer_host/render_widget_host.cc (right):

http://codereview.chromium.org/3547008/diff/1/16#newcode261
chrome/browser/renderer_host/render_widget_host.cc:261: const gfx::Rect
view_bounds = view_->GetViewBounds();
On 2010/09/30 22:50:21, sky wrote:
> informally we don't use const like this. You should do what generally happens
in
> the file, and in this file const isn't used like this.

Done.

http://codereview.chromium.org/3547008/diff/1/16#newcode265
chrome/browser/renderer_host/render_widget_host.cc:265:
GetReservedContentsRect().Union(GetRootWindowResizerRect());
On 2010/09/30 22:50:21, sky wrote:
> Shouldn't the reserved contents rect include the root window resizer rect?

That was my initial intent, but in this case every TabContentsDelegate instance
will have to request resize rect from view in order to return the correct
reserved contents rect, even if it does not care about reserved rect (e.g.
MatchPreview). These rects should be merged somewhere anyway, this seemed to be
the most generic place.

http://codereview.chromium.org/3547008/diff/1/16#newcode293
chrome/browser/renderer_host/render_widget_host.cc:293: current_reserved_rect_ =
reserved_rect;
On 2010/09/30 22:50:21, sky wrote:
> Why is it ok to set the reserved_rect immediately here? Also, I don't think
you
> should set the current_size_.

We hit this branch only when new_size is empty/not changed and ViewMsg_Resize
was sent successfully. In this case we're not going to receive an ACK, so these
values will be stuck in in_flight* members and since we have two variables
changing now, we can easily hit DCHECK in RenderWidget::OnResize, line 238, next
time we send an update.

http://codereview.chromium.org/3547008/diff/1/17
File chrome/browser/renderer_host/render_widget_host.h (right):

http://codereview.chromium.org/3547008/diff/1/17#newcode604
chrome/browser/renderer_host/render_widget_host.h:604: // The reserved area we
last sent to the renderer. |current_reserved_rect_|
On 2010/09/30 22:50:21, sky wrote:
> newline between 603 and 604.

Done.

http://codereview.chromium.org/3547008/diff/1/30
File chrome/browser/views/frame/browser_view.h (left):

http://codereview.chromium.org/3547008/diff/1/30#oldcode281
chrome/browser/views/frame/browser_view.h:281: virtual gfx::Rect
GetRootWindowResizerRect() const;
On 2010/09/30 22:50:21, sky wrote:
> If you're going to nuke this, could you nuke GetRootWindowResizerRect from
every
> where?

Unfortunately, no. Windows version needs GetRootWindowResizerRect to update
mouse pointer shape and to alter non-client area clicks.

[sky, 2010-10-01 18:21:12]

http://codereview.chromium.org/3547008/diff/1/15
File chrome/browser/renderer_host/render_view_host_delegate.h (right):

http://codereview.chromium.org/3547008/diff/1/15#newcode813
chrome/browser/renderer_host/render_view_host_delegate.h:813: virtual gfx::Rect
GetReservedContentsRect() const;
On 2010/10/01 17:28:34, Aleksey Shlyapnikov wrote:
> On 2010/09/30 22:50:21, sky wrote:
> > This (and GetRootWindowResizerRect) should be on View. Please move them.
> 
> I'm not sure I understand your request. Are you suggesting to directly call
> delegate_->GetViewDelegate()->GetRootWindowResizerRect() in RenderViewHost
> instead of forwarding this call to delegate_? But only
GetRootWindowResizerRect
> call goes to view, GetReservedContentsRect is forwarded to TabContents's
> delegate, since it's the only entity aware of the reserved area for this
> particular renderer. 

You can still forward it to the TabContentsDelegate if you need to.
TabContentsView (which is the one implement RVHD::View) has a handled to the
TabCOntents and can forward to the TabContentsDelegate if it needs to.

http://codereview.chromium.org/3547008/diff/1/16
File chrome/browser/renderer_host/render_widget_host.cc (right):

http://codereview.chromium.org/3547008/diff/1/16#newcode293
chrome/browser/renderer_host/render_widget_host.cc:293: current_reserved_rect_ =
reserved_rect;
On 2010/10/01 17:28:34, Aleksey Shlyapnikov wrote:
> On 2010/09/30 22:50:21, sky wrote:
> > Why is it ok to set the reserved_rect immediately here? Also, I don't think
> you
> > should set the current_size_.
> 
> We hit this branch only when new_size is empty/not changed and ViewMsg_Resize
> was sent successfully.

Doesn't that mean the size didn't change and this has no effect? I must be
missing something.

> In this case we're not going to receive an ACK, so these
> values will be stuck in in_flight* members and since we have two variables
> changing now, we can easily hit DCHECK in RenderWidget::OnResize, line 238,
next
> time we send an update.

[Aleksey Shlyapnikov, 2010-10-01 22:32:22]

http://codereview.chromium.org/3547008/diff/1/15
File chrome/browser/renderer_host/render_view_host_delegate.h (right):

http://codereview.chromium.org/3547008/diff/1/15#newcode813
chrome/browser/renderer_host/render_view_host_delegate.h:813: virtual gfx::Rect
GetReservedContentsRect() const;
On 2010/10/01 18:21:12, sky wrote:
> On 2010/10/01 17:28:34, Aleksey Shlyapnikov wrote:
> > On 2010/09/30 22:50:21, sky wrote:
> > > This (and GetRootWindowResizerRect) should be on View. Please move them.
> > 
> > I'm not sure I understand your request. Are you suggesting to directly call
> > delegate_->GetViewDelegate()->GetRootWindowResizerRect() in RenderViewHost
> > instead of forwarding this call to delegate_? But only
> GetRootWindowResizerRect
> > call goes to view, GetReservedContentsRect is forwarded to TabContents's
> > delegate, since it's the only entity aware of the reserved area for this
> > particular renderer. 
> 
> You can still forward it to the TabContentsDelegate if you need to.
> TabContentsView (which is the one implement RVHD::View) has a handled to the
> TabCOntents and can forward to the TabContentsDelegate if it needs to.

Done, although changes are a bit more widespread now.

http://codereview.chromium.org/3547008/diff/1/16
File chrome/browser/renderer_host/render_widget_host.cc (right):

http://codereview.chromium.org/3547008/diff/1/16#newcode293
chrome/browser/renderer_host/render_widget_host.cc:293: current_reserved_rect_ =
reserved_rect;
On 2010/10/01 18:21:12, sky wrote:
> On 2010/10/01 17:28:34, Aleksey Shlyapnikov wrote:
> > On 2010/09/30 22:50:21, sky wrote:
> > > Why is it ok to set the reserved_rect immediately here? Also, I don't
think
> > you
> > > should set the current_size_.
> > 
> > We hit this branch only when new_size is empty/not changed and
ViewMsg_Resize
> > was sent successfully.
> 
> Doesn't that mean the size didn't change and this has no effect? I must be
> missing something.
> 
> > In this case we're not going to receive an ACK, so these
> > values will be stuck in in_flight* members and since we have two variables
> > changing now, we can easily hit DCHECK in RenderWidget::OnResize, line 238,
> next
> > time we send an update.
> 

Yes, but that is true for current_size_ only. As of now, changing reserved rect
only does not generate any side effects, except assigning a member variable in
RenderWidget. I'm planning to improve it (I have to do it anyway, we need to
communicate reserved rect changes to WebKit), but not in this CL. Before my
change it did not really matter whether we receive an ACK for empty size or not
(it's empty, after all, user see nothing on the screen), now it does matter
because reserved rect can be updated independently from size and we need to
communicate this change to RenderWidget somehow. We can add a new message to
deliver changes in reserved rect only, but I think the existing message is
enough, it reports changes in renderable region, it's non-rectangular, but still
one region.

[Aleksey Shlyapnikov, 2010-10-02 01:26:40]

Hey Rohit, would you please have a look at the Mac specific changes in this CL?
Thanks.

[rohitrao (ping after 24h), 2010-10-04 14:36:39]

http://codereview.chromium.org/3547008/diff/54001/55003
File chrome/browser/browser_window.h (right):

http://codereview.chromium.org/3547008/diff/54001/55003#newcode170
chrome/browser/browser_window.h:170: // Returns the rect in contents coordinates
where contents should not be
I don't understand this comment.  Can you give me an example of an area of the
window that is reserved and should not be drawn in?

http://codereview.chromium.org/3547008/diff/54001/55034
File chrome/browser/tab_contents/tab_contents_view_mac.mm (right):

http://codereview.chromium.org/3547008/diff/54001/55034#newcode383
chrome/browser/tab_contents/tab_contents_view_mac.mm:383: // TODO(alekseys):
This is a workaround to compensate for RenderWidget's
I don't understand this comment, or why we need a fallback case.  Are we
painting before the TabContentsView is ever added to the window?

Can you file a bug for fixing this and add a crbug.com link to this comment?

[sky, 2010-10-04 15:45:11]

http://codereview.chromium.org/3547008/diff/54001/55018
File chrome/browser/renderer_host/render_widget_host.cc (right):

http://codereview.chromium.org/3547008/diff/54001/55018#newcode280
chrome/browser/renderer_host/render_widget_host.cc:280: if (!new_size.IsEmpty()
&& new_size != current_size_)
resize_ack_pending = (the contents of the if).

http://codereview.chromium.org/3547008/diff/54001/55039
File chrome/browser/views/tab_contents/tab_contents_view_win.cc (right):

http://codereview.chromium.org/3547008/diff/54001/55039#newcode311
chrome/browser/views/tab_contents/tab_contents_view_win.cc:311: // int
scrollbar_thickness = gfx::scrollbar_size();
Please put a TODO in here.

http://codereview.chromium.org/3547008/diff/54001/55039#newcode330
chrome/browser/views/tab_contents/tab_contents_view_win.cc:330: const WidgetWin*
root_widget = GetRootWidget(GetNativeView());
Can you just use GetWindow() instead of this?

http://codereview.chromium.org/3547008/diff/54001/55039#newcode350
chrome/browser/views/tab_contents/tab_contents_view_win.cc:350:
GetLowerRightSubrect(root_widget_bounds, resize_corner_size);
content_resize_rect and window_resize_rect are in different coordinates. Don't
you need to convert them to the same coordinate system?

[Aleksey Shlyapnikov, 2010-10-05 00:03:30]

http://codereview.chromium.org/3547008/diff/54001/55003
File chrome/browser/browser_window.h (right):

http://codereview.chromium.org/3547008/diff/54001/55003#newcode170
chrome/browser/browser_window.h:170: // Returns the rect in contents coordinates
where contents should not be
On 2010/10/04 14:36:39, rohitrao wrote:
> I don't understand this comment.  Can you give me an example of an area of the
> window that is reserved and should not be drawn in?

Check
https://docs.google.com/a/google.com/Doc?docid=0AV4Qg3xyZ8RQZGZtbWIydDJfNWc0e....

http://codereview.chromium.org/3547008/diff/54001/55018
File chrome/browser/renderer_host/render_widget_host.cc (right):

http://codereview.chromium.org/3547008/diff/54001/55018#newcode280
chrome/browser/renderer_host/render_widget_host.cc:280: if (!new_size.IsEmpty()
&& new_size != current_size_)
On 2010/10/04 15:45:11, sky wrote:
> resize_ack_pending = (the contents of the if).

Done.

http://codereview.chromium.org/3547008/diff/54001/55034
File chrome/browser/tab_contents/tab_contents_view_mac.mm (right):

http://codereview.chromium.org/3547008/diff/54001/55034#newcode383
chrome/browser/tab_contents/tab_contents_view_mac.mm:383: // TODO(alekseys):
This is a workaround to compensate for RenderWidget's
On 2010/10/04 14:36:39, rohitrao wrote:
> I don't understand this comment, or why we need a fallback case.  Are we
> painting before the TabContentsView is ever added to the window?
> 
> Can you file a bug for fixing this and add a http://crbug.com link to this
comment?

Yes, contents view frame is set before it gets added into view hierarchy,
there's no [self window] yet.

http://codereview.chromium.org/3547008/diff/54001/55039
File chrome/browser/views/tab_contents/tab_contents_view_win.cc (right):

http://codereview.chromium.org/3547008/diff/54001/55039#newcode311
chrome/browser/views/tab_contents/tab_contents_view_win.cc:311: // int
scrollbar_thickness = gfx::scrollbar_size();
On 2010/10/04 15:45:11, sky wrote:
> Please put a TODO in here.

Done.

http://codereview.chromium.org/3547008/diff/54001/55039#newcode330
chrome/browser/views/tab_contents/tab_contents_view_win.cc:330: const WidgetWin*
root_widget = GetRootWidget(GetNativeView());
On 2010/10/04 15:45:11, sky wrote:
> Can you just use GetWindow() instead of this?

I need to access WidgetWin anyway, there's no function in Window to get its
bounds without border.

http://codereview.chromium.org/3547008/diff/54001/55039#newcode350
chrome/browser/views/tab_contents/tab_contents_view_win.cc:350:
GetLowerRightSubrect(root_widget_bounds, resize_corner_size);
On 2010/10/04 15:45:11, sky wrote:
> content_resize_rect and window_resize_rect are in different coordinates. Don't
> you need to convert them to the same coordinate system?

Comment on GetBounds() says that it returns rect in screen coordinates, so both
rects are in the same coordinate system.

[Aleksey Shlyapnikov, 2010-10-05 01:40:46]

@pinkerton,could you please check TabContentsController changes?

[pink (ping after 24hrs), 2010-10-05 13:20:39]

TabContentsController looks ok to me, i guess.

[sky, 2010-10-05 15:29:34]

http://codereview.chromium.org/3547008/diff/54001/55039
File chrome/browser/views/tab_contents/tab_contents_view_win.cc (right):

http://codereview.chromium.org/3547008/diff/54001/55039#newcode330
chrome/browser/views/tab_contents/tab_contents_view_win.cc:330: const WidgetWin*
root_widget = GetRootWidget(GetNativeView());
On 2010/10/05 00:03:30, Aleksey Shlyapnikov wrote:
> On 2010/10/04 15:45:11, sky wrote:
> > Can you just use GetWindow() instead of this?
> 
> I need to access WidgetWin anyway, there's no function in Window to get its
> bounds without border. 
> 

Window is a WidgetWin on windows.

[Aleksey Shlyapnikov, 2010-10-07 23:34:35]

Scott, I changed the implementation according to our discussion earlier this
week. While I'm working on Mac part, could you please check views version,
whether it is more in line with Chrome concepts this time? Thanks.

[sky, 2010-10-08 16:45:52]

On 2010/10/07 23:34:35, Aleksey Shlyapnikov wrote:
> Scott, I changed the implementation according to our discussion earlier this
> week. While I'm working on Mac part, could you please check views version,
> whether it is more in line with Chrome concepts this time? Thanks.

It was my understanding of the conversation that Ben was asking you to expose a
method in TabContentsView or RenderWidgetHostView for explicitly setting/getting
the bounds, and not to reach up like this patch does. BrowserViewLayout would
invoke the appropriate setter.

I've added Ben as a reviewer though, so he can correct me if I'm wrong.

  -Scott

[Aleksey Shlyapnikov, 2010-10-08 16:57:50]

I explored that approach too, the problem with it is that it has to be synced
with resize and view resize does not always happen during layout. My
understanding of the same conversation was that the BrowserView is the only
entity knowing about resize corner layout and RenderWidget is the one
communicating reserved rect to WebKit, all other components should be agnostic
of it and merely pass requests through layers.

[Ben Goodger (Google), 2010-10-08 17:06:50]

What do you mean "synced with resize"? and "resize does not always happen during
layout?"

[Aleksey Shlyapnikov, 2010-10-08 17:28:22]

If we store just the size of the reserved rect in RVH (or wherever else), we'll
have to also take LTR/RTL into account before reporting rectangle to WebKit,
which seems to be beyond RVH responsibilities. So, we have to store a rect, but
in this case we have to calculate it at the same time we resize the view,
because if this rect does not cover the lower right corner of the rendering
area, WebKit ignores it (at least, scrollbar does not respect it anymore). To
the second statement, sorry, I meant to say "layout does not always happen
during resize", for example, when split handle is moved. It can be fixed in
split view, indeed, but considering all other points, I decided that enquiring
the reserved area when it's actually needed is cleaner and more robust.

[Ben Goodger (Google), 2010-10-08 17:42:15]

But the BrowserView knows LTR/RTLness too. It should take LTR/RTLness into
account when setting the position of the rect.

[Aleksey Shlyapnikov, 2010-10-08 18:22:23]

That's correct and it does, but as I mentioned earlier there's still an issue
with keeping this position in sync with the current view size (in case of the
rect directly set and stored in RenderWidget, we have to calculate the new view
size, calculate reserved rect, set reserved rect and then set view size, so we
need to make sure we know about view resize before it actually happens to do not
send two ViewMsg_Resize notifications).

[Ben Goodger (Google), 2010-10-08 19:52:42]

The RenderWidgetHostView is always sized by its container. If you need to
store the rect in the RenderWidgetHostView as a member variable, please do.

The logic for calculating the placement of it should live in the
BrowserView.

-Ben

On Fri, Oct 8, 2010 at 11:22 AM, <alekseys@chromium.org> wrote:

> That's correct and it does, but as I mentioned earlier there's still an
> issue
> with keeping this position in sync with the current view size (in case of
> the
> rect directly set and stored in RenderWidget, we have to calculate the new
> view
> size, calculate reserved rect, set reserved rect and then set view size, so
> we
> need to make sure we know about view resize before it actually happens to
> do not
> send two ViewMsg_Resize notifications).
>
>
> http://codereview.chromium.org/3547008/show
>

[Aleksey Shlyapnikov, 2010-10-08 20:58:57]

BrowserView does not originate and is not notified about all view resizes (Mac
view has a similar problem), so there will be more callbacks somewhere, and,
most importantly, my impression from that discussion was that view classes are
supposed to be agnostic of the resize corner concept, but ok, I can do it in
RenderWidgetHostView too.

[Aleksey Shlyapnikov, 2010-10-11 00:25:05]

Please have another look, now reserved rect is cached in RenderWidgetHostView,
TabContentsContainer notifies BrowserView about size changes, BrowserView
updates RenderWidgetHostView state and resize corner is gone from everywhere
else. Mac implementation is under way.

[sky, 2010-10-13 15:41:35]

Sorry for not responding, I thought Ben was going to take over the review.

http://codereview.chromium.org/3547008/diff/101001/102023
File chrome/browser/views/tab_contents/tab_contents_container.h (right):

http://codereview.chromium.org/3547008/diff/101001/102023#newcode22
chrome/browser/views/tab_contents/tab_contents_container.h:22: class
ReservedAreaDelegate {
I must be missing something. Ben is asking for this view not to reach up,
instead BrowserViweLayout should invoke something like:

SetResizeRect(const gfx::Rect& rect);

on the TabContentsView/RWHV. Why can't that work?

[Ben Goodger (Google), 2010-10-13 17:14:29]

I have been busy with yell and other things over the past couple of days. I
will make time to comment on this this afternoon.

-Ben

On Wed, Oct 13, 2010 at 8:41 AM, <sky@chromium.org> wrote:

> Sorry for not responding, I thought Ben was going to take over the review.
>
>
> http://codereview.chromium.org/3547008/diff/101001/102023
> File chrome/browser/views/tab_contents/tab_contents_container.h (right):
>
> http://codereview.chromium.org/3547008/diff/101001/102023#newcode22
> chrome/browser/views/tab_contents/tab_contents_container.h:22: class
> ReservedAreaDelegate {
> I must be missing something. Ben is asking for this view not to reach
> up, instead BrowserViweLayout should invoke something like:
>
> SetResizeRect(const gfx::Rect& rect);
>
> on the TabContentsView/RWHV. Why can't that work?
>
>
> http://codereview.chromium.org/3547008/show
>

[Aleksey Shlyapnikov, 2010-10-13 18:15:47]

BrowserView should know somehow the moment RenderWidgetHostView size (or view
itself) has changed, layout is not called in all cases we need to take care of,
this is the way to notify BrowserView that reserved rect needs to be updated.

On 2010/10/13 15:41:35, sky wrote:
> Sorry for not responding, I thought Ben was going to take over the review.
> 
> http://codereview.chromium.org/3547008/diff/101001/102023
> File chrome/browser/views/tab_contents/tab_contents_container.h (right):
> 
> http://codereview.chromium.org/3547008/diff/101001/102023#newcode22
> chrome/browser/views/tab_contents/tab_contents_container.h:22: class
> ReservedAreaDelegate {
> I must be missing something. Ben is asking for this view not to reach up,
> instead BrowserViweLayout should invoke something like:
> 
> SetResizeRect(const gfx::Rect& rect);
> 
> on the TabContentsView/RWHV. Why can't that work?

[Ben Goodger (Google), 2010-10-13 20:29:41]

http://codereview.chromium.org/3547008/diff/101001/102011
File chrome/browser/renderer_host/render_widget_host.h (right):

http://codereview.chromium.org/3547008/diff/101001/102011#newcode594
chrome/browser/renderer_host/render_widget_host.h:594: gfx::Rect
current_reserved_rect_;
Note that content will be drawn here anyway, such as background images etc. The
resizer is overlaid on top. I assume this is true for sidebar mini tabs too.

http://codereview.chromium.org/3547008/diff/101001/102013
File chrome/browser/renderer_host/render_widget_host_view.h (right):

http://codereview.chromium.org/3547008/diff/101001/102013#newcode280
chrome/browser/renderer_host/render_widget_host_view.h:280: // rendered to draw
the resize corner, sidebar mini tabs etc.
See my comment in the other file. Contents will be drawn, but they can expect to
be overlaid.

http://codereview.chromium.org/3547008/diff/101001/102022
File chrome/browser/views/tab_contents/tab_contents_container.cc (right):

http://codereview.chromium.org/3547008/diff/101001/102022#newcode39
chrome/browser/views/tab_contents/tab_contents_container.cc:39:
reserved_area_delegate_->UpdateReservedContentsRect(this);
I believe that in both this case and the RenderViewHostChanged method below you
can just freshen the resizer rect from the current state. Store the resizer rect
bounds in a member variable when it is set by the setter method I describe in my
comment if you have to.

http://codereview.chromium.org/3547008/diff/101001/102023
File chrome/browser/views/tab_contents/tab_contents_container.h (right):

http://codereview.chromium.org/3547008/diff/101001/102023#newcode22
chrome/browser/views/tab_contents/tab_contents_container.h:22: class
ReservedAreaDelegate {
Indeed. There should be the setter sky describes and it should be called at the
end of BrowserView's Layout, once everything has been sized. BrowserView can
then have some function LayoutResizeCorner that calculates the bounds of the
resize rect for all the various involved parties (such as this
TabContentsContainer, the sidebar thingy etc). and calls the setters on each.

The setter then calls into the attached TabContents and gets its RVH to display
the resize corner.

[Aleksey Shlyapnikov, 2010-10-13 21:23:00]

http://codereview.chromium.org/3547008/diff/101001/102023
File chrome/browser/views/tab_contents/tab_contents_container.h (right):

http://codereview.chromium.org/3547008/diff/101001/102023#newcode22
chrome/browser/views/tab_contents/tab_contents_container.h:22: class
ReservedAreaDelegate {
But if we update reserved area after everything has been sized, we'll have to
send two messages to update RenderWidget state instead of just one. Also,
scrollbars will visibly jerk, once laid out with the old reserved area and then
with the updated one (moving scrollbar controls away from the resize corner area
to keep them functional is the most visible effect of this WebKit feature).
I probably terribly miss something obvious, but your proposal ignores this
double update problem (not mentioning the fact that WebKit does not relayout the
page if we update reserved area only, without resizing it, but this is a
separate problem I need to look at).


On 2010/10/13 20:29:41, Ben Goodger wrote:
> Indeed. There should be the setter sky describes and it should be called at
the
> end of BrowserView's Layout, once everything has been sized. BrowserView can
> then have some function LayoutResizeCorner that calculates the bounds of the
> resize rect for all the various involved parties (such as this
> TabContentsContainer, the sidebar thingy etc). and calls the setters on each.
> 
> The setter then calls into the attached TabContents and gets its RVH to
display
> the resize corner.

[Ben Goodger (Google), 2010-10-13 21:31:18]

http://codereview.chromium.org/3547008/diff/101001/102023
File chrome/browser/views/tab_contents/tab_contents_container.h (right):

http://codereview.chromium.org/3547008/diff/101001/102023#newcode22
chrome/browser/views/tab_contents/tab_contents_container.h:22: class
ReservedAreaDelegate {
How about this:

1. In BrowserView::Layout, figure out the size of everything.
2. Figure out the size of the resizer rect in the relevant subviews, update the
subviews with this information.
3. Atomically size everything at the end.

[Aleksey Shlyapnikov, 2010-10-13 21:53:13]

I thought about this approach, but
1) Resizing split view components does not trigger layout;
2) Layout resizes split view, it changes the size of the controlled views by
itself, so we'll need a expose a function to calculate a size of embedded views
for the given size of the split view;
3) How is it simpler than what I did? What is fundamentally wrong with this
delegate? 

On 2010/10/13 21:31:18, Ben Goodger wrote:
> http://codereview.chromium.org/3547008/diff/101001/102023
> File chrome/browser/views/tab_contents/tab_contents_container.h (right):
> 
> http://codereview.chromium.org/3547008/diff/101001/102023#newcode22
> chrome/browser/views/tab_contents/tab_contents_container.h:22: class
> ReservedAreaDelegate {
> How about this:
> 
> 1. In BrowserView::Layout, figure out the size of everything.
> 2. Figure out the size of the resizer rect in the relevant subviews, update
the
> subviews with this information.
> 3. Atomically size everything at the end.

[Aleksey Shlyapnikov, 2010-10-14 19:55:00]

http://codereview.chromium.org/3547008/diff/101001/102011
File chrome/browser/renderer_host/render_widget_host.h (right):

http://codereview.chromium.org/3547008/diff/101001/102011#newcode594
chrome/browser/renderer_host/render_widget_host.h:594: gfx::Rect
current_reserved_rect_;
Yes, I realize that (with the addition of the scrollbar "evasion"). I just
thought the use of "should not", rather than "must not", makes it an advisory
restriction.

On 2010/10/13 20:29:41, Ben Goodger wrote:
> Note that content will be drawn here anyway, such as background images etc.
The
> resizer is overlaid on top. I assume this is true for sidebar mini tabs too.

http://codereview.chromium.org/3547008/diff/101001/102022
File chrome/browser/views/tab_contents/tab_contents_container.cc (right):

http://codereview.chromium.org/3547008/diff/101001/102022#newcode39
chrome/browser/views/tab_contents/tab_contents_container.cc:39:
reserved_area_delegate_->UpdateReservedContentsRect(this);
On 2010/10/13 20:29:41, Ben Goodger wrote:
> I believe that in both this case and the RenderViewHostChanged method below
you
> can just freshen the resizer rect from the current state. Store the resizer
rect
> bounds in a member variable when it is set by the setter method I describe in
my
> comment if you have to.

Done.

[Aleksey Shlyapnikov, 2010-10-19 01:42:43]

Not to be a nagger, but ping?

[Ben Goodger (Google), 2010-10-19 19:09:13]

I am trying to reconcile my proposal with the way SingleSplitView works.

-Ben

On Mon, Oct 18, 2010 at 6:42 PM, <alekseys@chromium.org> wrote:

> Not to be a nagger, but ping?
>
>
> http://codereview.chromium.org/3547008/show
>

[Aleksey Shlyapnikov, 2010-10-22 17:31:36]

Any chance to get it moving, please?

[Aleksey Shlyapnikov, 2010-10-27 00:35:28]

Ben, would you happen to have time this week to look at this issue? Thanks.

[Ben Goodger (Google), 2010-10-27 01:06:43]

I have been OOO, I will look Weds or Thurs.

-Ben

On Tue, Oct 26, 2010 at 5:35 PM, <alekseys@chromium.org> wrote:

> Ben, would you happen to have time this week to look at this issue? Thanks.
>
>
> http://codereview.chromium.org/3547008/show
>

[Ben Goodger (Google), 2010-11-01 15:01:44]

Looking at this again now.

-Ben

On Tue, Oct 26, 2010 at 6:06 PM, Ben Goodger (Google) <ben@chromium.org>wrote:

> I have been OOO, I will look Weds or Thurs.
>
> -Ben
>
>
> On Tue, Oct 26, 2010 at 5:35 PM, <alekseys@chromium.org> wrote:
>
>> Ben, would you happen to have time this week to look at this issue?
>> Thanks.
>>
>>
>> http://codereview.chromium.org/3547008/show
>>
>
>

[Ben Goodger (Google), 2010-11-01 16:56:27]

After much thought I have not been able to come up with anything that's less
gross than this.

My one remaining question is - what is your use case for the sidebar code now?

[Ben Goodger (Google), 2010-11-02 17:42:24]

After some discussion, LGTM.

On Mon, Nov 1, 2010 at 9:56 AM, <ben@chromium.org> wrote:

> After much thought I have not been able to come up with anything that's
> less
> gross than this.
>
> My one remaining question is - what is your use case for the sidebar code
> now?
>
>
> http://codereview.chromium.org/3547008/show
>

[Aleksey Shlyapnikov, 2010-11-03 00:49:48]

Rohit, Mac specific part of this CL was completely reworked since your last
comment, would you please give it another look? Thanks.

[rohitrao (ping after 24h), 2010-11-04 23:13:48]

How many different views need to have reserved content areas?  I dislike that we
have to hook this delegate into so many different controllers, and I didn't
understand why the DevToolsController and SidebarController need the delegate
callback at all.  I also dislike how SidebarController now needs to know about
TabContentsController.

Is there a paragraph somewhere explaining what is supposed to happen when the
window is resized?  It looks like we have to do a bit of extra work on Mac
because we don't do layout in quite the same way.

http://codereview.chromium.org/3547008/diff/172001/173012
File chrome/browser/cocoa/tab_contents_controller.h (right):

http://codereview.chromium.org/3547008/diff/172001/173012#newcode34
chrome/browser/cocoa/tab_contents_controller.h:34: @interface
TabContentsViewController : NSViewController {
Why did you rename this class?  We generally don't put "View" into our
controller names.

[Aleksey Shlyapnikov, 2010-11-05 00:45:11]

At the moment, there're three views, the page itself, sidebar and devTools.
TabContentsViewController handles flicker-less contents switch and since we have
to switch contents in all three mentioned views as smooth as possible, using the
same mechanism in all of them makes sense.
Here's a rationale behind this design. The only entity aware of the resize
corner and sidebar mini tabs positions is BrowserWindow+BrowserWindowController,
that's why all calls are forwarded to it to decide on the reserved contents area
for the particular view. We also have to update the reserved area before the
contents view is actually resized, so we can piggyback reserved area update with
the coming ViewMsg_Resize event and avoid sending this event twice (second one
would be sent to communicate the updated reserved area to WebKit), that's why
TabContentsViewController need a delegate to notify about coming frame size
changes.
SidebarController does not know about TabContentsController, it owns
TabContentsViewController instance.

So to summarize:
1) All controllers use TabContentsViewController instance to do flicker-less
contents switches and
2) the same instance is used to callback a delegate before view frame size is
changed to do not send extra ViewMsg_Resize event

Regarding of  what is supposed to happen when the window is resized, I'll add
what I've just explained as a comment to
BrowserViewController::tabContentsViewFrameWillChange, provided this design is
viable.

http://codereview.chromium.org/3547008/diff/172001/173012
File chrome/browser/cocoa/tab_contents_controller.h (right):

http://codereview.chromium.org/3547008/diff/172001/173012#newcode34
chrome/browser/cocoa/tab_contents_controller.h:34: @interface
TabContentsViewController : NSViewController {
On 2010/11/04 23:13:48, rohitrao wrote:
> Why did you rename this class?  We generally don't put "View" into our
> controller names.

There's a class named TabContentsController and it adds a functionality specific
for the page TabContents (as opposed to sidebar and devTools TabContents). I
thought about renaming it, but have not came up with the better name.

[pink (ping after 24hrs), 2010-11-12 17:34:25]

Are there mocks somewhere for these minitabs or the sidebar? I must admit I have
no context for why we're making these changes.

http://codereview.chromium.org/3547008/diff/172001/chrome/browser/cocoa/dev_t...
File chrome/browser/cocoa/dev_tools_controller.h (right):

http://codereview.chromium.org/3547008/diff/172001/chrome/browser/cocoa/dev_t...
chrome/browser/cocoa/dev_tools_controller.h:11: #import
"chrome/browser/cocoa/tab_contents_controller.h"
alphabetize

http://codereview.chromium.org/3547008/diff/172001/chrome/browser/cocoa/sideb...
File chrome/browser/cocoa/sidebar_controller.h (right):

http://codereview.chromium.org/3547008/diff/172001/chrome/browser/cocoa/sideb...
chrome/browser/cocoa/sidebar_controller.h:11: #import
"chrome/browser/cocoa/tab_contents_controller.h"
alphabetize

[rohitrao (ping after 24h), 2010-11-12 18:45:18]

Ok, I can't work out all of the kinks when using notifications, so we're back to
this.  I think the basic setup here is fine, but we jump through a lot of hoops
to avoid flicker.  I'd like to find a simpler way to manage tab switches that
doesn't involve so much work.

http://codereview.chromium.org/3547008/diff/172001/chrome/browser/cocoa/tab_c...
File chrome/browser/cocoa/tab_contents_controller.h (right):

http://codereview.chromium.org/3547008/diff/172001/chrome/browser/cocoa/tab_c...
chrome/browser/cocoa/tab_contents_controller.h:34: @interface
TabContentsViewController : NSViewController {
Do you think there's value in splitting off a base class for the split views? 
Why not just reuse TabContentsController as-is?

If you want to keep the base class, let's come up with a better name for it.

http://codereview.chromium.org/3547008/diff/172001/chrome/browser/cocoa/tab_c...
File chrome/browser/cocoa/tab_contents_controller.mm (right):

http://codereview.chromium.org/3547008/diff/172001/chrome/browser/cocoa/tab_c...
chrome/browser/cocoa/tab_contents_controller.mm:178: if (oldHost && newHost &&
oldHost->view() && newHost->view()) {
You're going to have this same basic code copy/pasted in three different view
classes.  Can we put this logic in one place?  Maybe in TabContentsView or
RenderWidgetHostView?

http://codereview.chromium.org/3547008/diff/172001/chrome/browser/cocoa/tab_c...
chrome/browser/cocoa/tab_contents_controller.mm:183: frameRect:[[self view]
frame]];
How important is this fallback case?  When does it get triggered?

http://codereview.chromium.org/3547008/diff/172001/chrome/browser/views/tab_c...
File chrome/browser/views/tab_contents/tab_contents_container.h (right):

http://codereview.chromium.org/3547008/diff/172001/chrome/browser/views/tab_c...
chrome/browser/views/tab_contents/tab_contents_container.h:25: // Returns the
rect in tab contents view coordinates where contents should
This comment no longer applies.  From the method name, it sounds like the
delegate sets the reserved rect directly, rather than returning the new rect.

[Aleksey Shlyapnikov, 2010-11-12 18:45:20]

There's a mock screenshot at the top of
http://www.chromium.org/developers/design-documents/extensions/sidebar-extens....

On 2010/11/12 17:34:25, pink wrote:
> Are there mocks somewhere for these minitabs or the sidebar? I must admit I
have
> no context for why we're making these changes.
> 
>
http://codereview.chromium.org/3547008/diff/172001/chrome/browser/cocoa/dev_t...
> File chrome/browser/cocoa/dev_tools_controller.h (right):
> 
>
http://codereview.chromium.org/3547008/diff/172001/chrome/browser/cocoa/dev_t...
> chrome/browser/cocoa/dev_tools_controller.h:11: #import
> "chrome/browser/cocoa/tab_contents_controller.h"
> alphabetize
> 
>
http://codereview.chromium.org/3547008/diff/172001/chrome/browser/cocoa/sideb...
> File chrome/browser/cocoa/sidebar_controller.h (right):
> 
>
http://codereview.chromium.org/3547008/diff/172001/chrome/browser/cocoa/sideb...
> chrome/browser/cocoa/sidebar_controller.h:11: #import
> "chrome/browser/cocoa/tab_contents_controller.h"
> alphabetize

[Aleksey Shlyapnikov, 2010-11-13 01:37:11]

http://codereview.chromium.org/3547008/diff/172001/chrome/browser/cocoa/dev_t...
File chrome/browser/cocoa/dev_tools_controller.h (right):

http://codereview.chromium.org/3547008/diff/172001/chrome/browser/cocoa/dev_t...
chrome/browser/cocoa/dev_tools_controller.h:11: #import
"chrome/browser/cocoa/tab_contents_controller.h"
On 2010/11/12 17:34:26, pink wrote:
> alphabetize

Done.

http://codereview.chromium.org/3547008/diff/172001/chrome/browser/cocoa/sideb...
File chrome/browser/cocoa/sidebar_controller.h (right):

http://codereview.chromium.org/3547008/diff/172001/chrome/browser/cocoa/sideb...
chrome/browser/cocoa/sidebar_controller.h:11: #import
"chrome/browser/cocoa/tab_contents_controller.h"
On 2010/11/12 17:34:26, pink wrote:
> alphabetize

Done.

http://codereview.chromium.org/3547008/diff/172001/chrome/browser/cocoa/tab_c...
File chrome/browser/cocoa/tab_contents_controller.h (right):

http://codereview.chromium.org/3547008/diff/172001/chrome/browser/cocoa/tab_c...
chrome/browser/cocoa/tab_contents_controller.h:34: @interface
TabContentsViewController : NSViewController {
There's some logic in TabContentsController which does not apply to Sidebar and
DevTools contents, but since we're anticipating changes in view layout anyway,
it does not make much sense to separate it now. Done.

On 2010/11/12 18:45:19, rohitrao wrote:
> Do you think there's value in splitting off a base class for the split views? 
> Why not just reuse TabContentsController as-is?
> 
> If you want to keep the base class, let's come up with a better name for it.

http://codereview.chromium.org/3547008/diff/172001/chrome/browser/cocoa/tab_c...
File chrome/browser/cocoa/tab_contents_controller.mm (right):

http://codereview.chromium.org/3547008/diff/172001/chrome/browser/cocoa/tab_c...
chrome/browser/cocoa/tab_contents_controller.mm:178: if (oldHost && newHost &&
oldHost->view() && newHost->view()) {
I tried to move this part into RenderViewHostManager::CommitPending (the most
logical place, I presume), completely remove this handler here and rely on other
resize, but it did not work right away in case of docking in/dragging out tabs.
We can add another notification, indeed, but since we have to subscribe to some
notification anyway, why not this one? Also, the idea was to keep the reserved
area related code as limited to BrowserView* as possible, this class is involved
only because this is seemingly the only way to catch the exact moment our view
is resized and adding more classes into the loop now does not seem to be
necessary (maybe after we redesign a way views are laid out, there'll be less
corner cases to catch and then we'll get back and generalize it).

On 2010/11/12 18:45:19, rohitrao wrote:
> You're going to have this same basic code copy/pasted in three different view
> classes.  Can we put this logic in one place?  Maybe in TabContentsView or
> RenderWidgetHostView?

http://codereview.chromium.org/3547008/diff/172001/chrome/browser/cocoa/tab_c...
chrome/browser/cocoa/tab_contents_controller.mm:183: frameRect:[[self view]
frame]];
One or both of the hosts can be NULL when we're transitioning from/to "Aw,
Snap", oldHost is NULL when you drag/dock a tab from one window to another, and
comment for RENDER_VIEW_HOST_CHANGED event states that oldHost can be NULL first
RVH is set, so I decided to cover all cases here.

On 2010/11/12 18:45:19, rohitrao wrote:
> How important is this fallback case?  When does it get triggered?

http://codereview.chromium.org/3547008/diff/172001/chrome/browser/views/tab_c...
File chrome/browser/views/tab_contents/tab_contents_container.h (right):

http://codereview.chromium.org/3547008/diff/172001/chrome/browser/views/tab_c...
chrome/browser/views/tab_contents/tab_contents_container.h:25: // Returns the
rect in tab contents view coordinates where contents should
On 2010/11/12 18:45:19, rohitrao wrote:
> This comment no longer applies.  From the method name, it sounds like the
> delegate sets the reserved rect directly, rather than returning the new rect.

Done.

[rohitrao (ping after 24h), 2010-11-15 23:46:57]

Looks ok to me, with a few comments.

I had a few other comments here, but codereview ate them all in a 500 error. 
The gist of it is that I think the view hierarchy is now more complicated than I
would like, so please do circle back on this and try to clean things up as I
mentioned in my email.

http://codereview.chromium.org/3547008/diff/227001/chrome/browser/cocoa/dev_t...
File chrome/browser/cocoa/dev_tools_controller.mm (right):

http://codereview.chromium.org/3547008/diff/227001/chrome/browser/cocoa/dev_t...
chrome/browser/cocoa/dev_tools_controller.mm:154:
shouldAdjustSizeOfSubview:(NSView *)subview {
Is this new?  Is it now required because of some other change in this CL, or was
our behavior always broken?

http://codereview.chromium.org/3547008/diff/227001/chrome/browser/cocoa/tab_c...
File chrome/browser/cocoa/tab_contents_controller.mm (right):

http://codereview.chromium.org/3547008/diff/227001/chrome/browser/cocoa/tab_c...
chrome/browser/cocoa/tab_contents_controller.mm:65: void
TabContentsNotificationBridge::ObserveNotifications(
Maybe call this method "RegisterForTabContents"?  Or ChangeTabContents.

http://codereview.chromium.org/3547008/diff/227001/chrome/browser/cocoa/tab_c...
chrome/browser/cocoa/tab_contents_controller.mm:78: @interface
TabContentsControllerView : NSView {
This isn't a great name, especially since we already have so many
similarly-named classes.  I would be ok with naming the view after its
functionality. ResizeNotificationView, or somesuch.  Ok, that's terrible too.

http://codereview.chromium.org/3547008/diff/227001/chrome/browser/cocoa/tab_c...
chrome/browser/cocoa/tab_contents_controller.mm:109:
tabContentsBridge_.reset(new TabContentsNotificationBridge(self));
I really dislike listening for this notification here.  Please continue to look
for other places to put it.

[Aleksey Shlyapnikov, 2010-11-16 02:07:37]

http://codereview.chromium.org/3547008/diff/227001/chrome/browser/cocoa/dev_t...
File chrome/browser/cocoa/dev_tools_controller.mm (right):

http://codereview.chromium.org/3547008/diff/227001/chrome/browser/cocoa/dev_t...
chrome/browser/cocoa/dev_tools_controller.mm:154:
shouldAdjustSizeOfSubview:(NSView *)subview {
It was broken since we switched to one devtools container per browser window
(see CL description for the repro case) and it used to be required for the
earlier versions of this CL, but looks like now it works without this change.
Should I fix it in a separate CL then?

On 2010/11/15 23:46:58, rohitrao wrote:
> Is this new?  Is it now required because of some other change in this CL, or
was
> our behavior always broken?

http://codereview.chromium.org/3547008/diff/227001/chrome/browser/cocoa/tab_c...
File chrome/browser/cocoa/tab_contents_controller.mm (right):

http://codereview.chromium.org/3547008/diff/227001/chrome/browser/cocoa/tab_c...
chrome/browser/cocoa/tab_contents_controller.mm:65: void
TabContentsNotificationBridge::ObserveNotifications(
On 2010/11/15 23:46:58, rohitrao wrote:
> Maybe call this method "RegisterForTabContents"?  Or ChangeTabContents.

Done.

http://codereview.chromium.org/3547008/diff/227001/chrome/browser/cocoa/tab_c...
chrome/browser/cocoa/tab_contents_controller.mm:78: @interface
TabContentsControllerView : NSView {
Cannot think of anything better than this. Done.

On 2010/11/15 23:46:58, rohitrao wrote:
> This isn't a great name, especially since we already have so many
> similarly-named classes.  I would be ok with naming the view after its
> functionality. ResizeNotificationView, or somesuch.  Ok, that's terrible too.

[rohitrao (ping after 24h), 2010-11-16 02:14:47]

Please run these latest changes through the trybots once and then go ahead and
submit.  Thanks for your patience =)

http://codereview.chromium.org/3547008/diff/227001/chrome/browser/cocoa/dev_t...
File chrome/browser/cocoa/dev_tools_controller.mm (right):

http://codereview.chromium.org/3547008/diff/227001/chrome/browser/cocoa/dev_t...
chrome/browser/cocoa/dev_tools_controller.mm:154:
shouldAdjustSizeOfSubview:(NSView *)subview {
Nah, this is fine.

On 2010/11/16 02:07:37, Aleksey Shlyapnikov wrote:
> It was broken since we switched to one devtools container per browser window
> (see CL description for the repro case) and it used to be required for the
> earlier versions of this CL, but looks like now it works without this change.
> Should I fix it in a separate CL then?
> 
> On 2010/11/15 23:46:58, rohitrao wrote:
> > Is this new?  Is it now required because of some other change in this CL, or
> was
> > our behavior always broken?
>