Make sure the RootView is sized to the correct bounds when the opaque frame i...

Make sure the RootView is sized to the correct bounds when the opaque frame is maximized.It seems that now, when an opaque frame is maximized we also need to add the SM_CXSIZEFRAME to the top of the client rect. I don't know why, it's just is.I reworked the way Widget allows subclasses to handle sizing of the RootView... I delegate responsibility for determining the RootView's bounds to a helper virtual function which BrowserFrameWin overrides. This seems cleaner to me. I make sure this handling only occurs when the window is not maximized.http://crbug.com/25227TEST=use to a theme or switch off glass on vista or use Xp. Maximize the browser window. The tabs should be entirely visible.

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

[Peter Kasting, 2009-10-19 22:53:18]

http://codereview.chromium.org/304007/diff/6001/7004
File chrome/browser/views/frame/browser_frame_win.cc (right):

http://codereview.chromium.org/304007/diff/6001/7004#newcode209
Line 209: client_rect->top += border_thickness;
I know why this is happening; it's because in the opaque frame we need to
exactly match the screen rect, whereas in the native frame we don't (and in fact
mustn't), since we'll just extend the glass into the window top however far it
needs to go, and DwmDefWindowProc() will put the caption buttons at the right
spot.  This code only used to be used for the native frame.

To address this, I think you should remove this code block, and extend the
conditional at the top of the function to use WindowWin::OnNCCalcSize() for
maximized opaque frames too.  This has the side benefit of handling autohide
taskbars at screen top better.

http://codereview.chromium.org/304007/diff/6001/7004#newcode248
Line 248: // rect when using the opaque frame. See SizeRootViewToWindowRect.
Nit: Update function name.

http://codereview.chromium.org/304007/diff/6001/7004#newcode249
Line 249: --client_rect->bottom;
This is now the only thing we do here that we don't do in
WindowWin::OnNCCalcSize().  This prompts the following two questions:

(1) Can we safely do this for _all_ non-glass-frame WindowWins?  If so, we can
move this code there and simplify my suggestion above to be "return the initial
conditional to the way it used to be, where all opaque frames used
WindowWin::OnNCCalcSize()".

(2) Is it safe to return WVR_REDRAW in this case (the client rect differs from
the nonclient rect only by one pixel at bottom)?  Based on the comments at the
end of WindowWin::OnNCCalcSize(), it seems like it ought to be, and would make
resizing nicer.  If it is, we should return it directly from inside this
conditional arm (if we don't make all opaque frame cases use WindowWin); if it
isn't, and we move this code to WindowWin, then that function will need to
unconditionally return 0.

http://codereview.chromium.org/304007/diff/6001/7004#newcode338
Line 338: return WindowWin::GetRootViewBounds();
Nit: Since this path is simpler, seems like it'd be better to reverse the
conditional and early-return this.

http://codereview.chromium.org/304007/diff/6001/7001
File views/widget/widget_win.cc (right):

http://codereview.chromium.org/304007/diff/6001/7001#newcode827
Line 827: if (use_layered_buffer_) {
Nit: No {}

http://codereview.chromium.org/304007/diff/6001/7001#newcode833
Line 833: // screen origin in some cases.
Nit: What cases?  Anytime the monitor rect for the containing monitor is not at
0, 0?

This really worries me because we pass this rect to PaintNow(), whose code
adjusts it based on the monitor coords.  It looks like that must be wrong now.

[Ben Goodger (Google), 2009-10-20 00:46:11]

http://codereview.chromium.org/304007/diff/6001/7004
File chrome/browser/views/frame/browser_frame_win.cc (right):

http://codereview.chromium.org/304007/diff/6001/7004#newcode249
Line 249: --client_rect->bottom;
On 2009/10/19 22:53:19, Peter Kasting wrote:
> This is now the only thing we do here that we don't do in
> WindowWin::OnNCCalcSize().

Not just this decrement, which I think is safe for all WindowWins, but the
adjustment for the client area a few lines above. I think that is definitely
browser window specific.

[Ben Goodger (Google), 2009-10-20 02:46:47]

I've updated this CL a bit.
I took your suggestions to the next degree and eradicated
BrowserFrameWin::OnNCCalcSize completely. There are a couple of weird
conditions still that I have to work out but it mostly works. I am
interested to get your take on this before I go any further. I make
WindowWin have a GetClientAreaInsets virtual function that BrowserFrameWin
overrides to return specific insets then WindowWin does everything.

-Ben

On Mon, Oct 19, 2009 at 5:46 PM, <ben@chromium.org> wrote:

>
> http://codereview.chromium.org/304007/diff/6001/7004
> File chrome/browser/views/frame/browser_frame_win.cc (right):
>
> http://codereview.chromium.org/304007/diff/6001/7004#newcode249
> Line 249: --client_rect->bottom;
> On 2009/10/19 22:53:19, Peter Kasting wrote:
>
>> This is now the only thing we do here that we don't do in
>> WindowWin::OnNCCalcSize().
>>
>
> Not just this decrement, which I think is safe for all WindowWins, but
> the adjustment for the client area a few lines above. I think that is
> definitely browser window specific.
>
>
> http://codereview.chromium.org/304007
>

[Peter Kasting, 2009-10-20 18:47:04]

Concept seems OK to me.

http://codereview.chromium.org/304007/diff/5003/4003
File views/widget/widget_win.cc (right):

http://codereview.chromium.org/304007/diff/5003/4003#newcode974
Line 974: PaintNow(gfx::Rect(0, 0, size.width(), size.height()));
I'm still really anxious about passing an origin of 0, 0 to this when the old
code didn't.  Why is this safe, given how PaintNow() translates the origin?

http://codereview.chromium.org/304007/diff/5003/4004
File views/widget/widget_win.h (right):

http://codereview.chromium.org/304007/diff/5003/4004#newcode392
Line 392: // to size the RootView to the window rect rather than the client
rect.
Nit: Seems overly detailed to describe here the way in which WindowWin overrides
this... maybe this should just say what the function means, and WindowWin's
declaration should say what it's doing with the override?

http://codereview.chromium.org/304007/diff/5003/4007
File views/window/window_win.cc (right):

http://codereview.chromium.org/304007/diff/5003/4007#newcode533
Line 533: // |WidgetWin::OnNCCalcSize| to be invoked.
Nit: Normally || means "variable"... "WidgetWin::OnNCCalcSize()" might be better

http://codereview.chromium.org/304007/diff/5003/4007#newcode553
Line 553: return gfx::Insets(0, 0, 1, 0);
Shouldn't we only do this for !IsFullscreen()?

http://codereview.chromium.org/304007/diff/5003/4007#newcode730
Line 730: void AdjustRectByInsets(RECT* rect, const gfx::Insets& insets) {
Nit: If you're only calling this once, why not just roll it into OnNCCalcSize()?

http://codereview.chromium.org/304007/diff/5003/4007#newcode759
Line 759: client_rect->top += win_util::kAutoHideTaskbarThicknessPx;
This code will need to change for the native frame.

http://codereview.chromium.org/304007/diff/5003/4007#newcode775
Line 775: return mode ? WVR_REDRAW : 0;
This code will need to return 0 in more conditions... perhaps if insets.left()
or top() is nonzero?

http://codereview.chromium.org/304007/diff/5003/4008
File views/window/window_win.h (right):

http://codereview.chromium.org/304007/diff/5003/4008#newcode127
Line 127: LRESULT OnNCCalcSize(BOOL mode, LPARAM l_param); // Don't override.
Nit: Two spaces before //

[Ben Goodger (Google), 2009-10-20 19:28:21]

http://codereview.chromium.org/304007/diff/5003/4003
File views/widget/widget_win.cc (right):

http://codereview.chromium.org/304007/diff/5003/4003#newcode974
Line 974: PaintNow(gfx::Rect(0, 0, size.width(), size.height()));
On 2009/10/20 18:47:04, Peter Kasting wrote:
> I'm still really anxious about passing an origin of 0, 0 to this when the old
> code didn't.  Why is this safe, given how PaintNow() translates the origin?

The old code did - GetClientRect would have set it to 0,0.
GetWindowRect is only used if the widget is a layered window, and in that case
PaintNow ignores the rect passed to it. It's a little crazy.

[Ben Goodger (Google), 2009-10-20 20:00:00]

Possibly. I'll look into it when I get back to my home system this evening.
-Ben

On Tue, Oct 20, 2009 at 12:55 PM, <pkasting@chromium.org> wrote:

>
> http://codereview.chromium.org/304007/diff/5003/4003
> File views/widget/widget_win.cc (right):
>
> http://codereview.chromium.org/304007/diff/5003/4003#newcode974
> Line 974: PaintNow(gfx::Rect(0, 0, size.width(), size.height()));
> On 2009/10/20 19:28:21, Ben Goodger wrote:
>
>> On 2009/10/20 18:47:04, Peter Kasting wrote:
>> > I'm still really anxious about passing an origin of 0, 0 to this
>>
> when the old
>
>> > code didn't.  Why is this safe, given how PaintNow() translates the
>>
> origin?
>
>  The old code did - GetClientRect would have set it to 0,0.
>> GetWindowRect is only used if the widget is a layered window, and in
>>
> that case
>
>> PaintNow ignores the rect passed to it. It's a little crazy.
>>
>
> Ah.  I missed that when reading PaintNow().  OK.  I wonder if that
> function should be split up or something if its two halves are going to
> work so differently.
>
>
> http://codereview.chromium.org/304007
>

[Ben Goodger (Google), 2009-10-23 02:25:10]

I updated this CL.

I didn't change PaintNow since it's actually part of the public View/Widget API
and lots of people call it for different reasons. That cleanup should be done
separately.

[Ben Goodger (Google), 2009-10-23 02:25:38]

BTW the glitches I was seeing was a result of me returning WVR_REDRAW in too
many cases so your comment was right on the money.

[Peter Kasting, 2009-10-23 05:09:30]

LGTM.  So after all this, have you determined whether it's feasible to make the
whole window (or close to it)  client area for the glass frame, so we could fix
the caption button position?

http://codereview.chromium.org/304007/diff/11001/12001
File views/widget/widget_win.cc (right):

http://codereview.chromium.org/304007/diff/11001/12001#newcode963
Line 963: gfx::Size size = GetRootViewSize();
Nit: Constructor style?

http://codereview.chromium.org/304007/diff/11001/12002
File views/widget/widget_win.h (right):

http://codereview.chromium.org/304007/diff/11001/12002#newcode390
Line 390: // Returns the size that the RootView should be set to in
|LayoutRootView|.
Nit: LayoutRootView() rather than |LayoutRootView|?

http://codereview.chromium.org/304007/diff/11001/12003
File views/window/window_win.cc (right):

http://codereview.chromium.org/304007/diff/11001/12003#newcode555
Line 555: return gfx::Insets(0, 0, !IsFullscreen() ? 1 : 0, 0);
Nit: Could remove the ! and reverse the condition

http://codereview.chromium.org/304007/diff/11001/12003#newcode735
Line 735: if (insets.width() == 0 && insets.height() == 0)
Nit: I'm not in a position to check, but is there an Insets::empty() function? 
Should there be?

http://codereview.chromium.org/304007/diff/11001/12003#newcode785
Line 785: // We special case when left and top insets are 0, since these
conditions
Nit: and -> or