Custom frame UI for platform apps on Windows.

Custom frame UI for platform apps on Windows.



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

[jeremya, 2012-01-19 23:52:15]

beng- could you look at the windows API and Views hackery?
mpcomplete- i think you're the best person to look at my ExtensionView changes.
mihaip- could you look at ShellWindow and general comments?

Thanks :)

[Matt Perry, 2012-01-20 00:22:47]

https://chromiumcodereview.appspot.com/9254046/diff/1/chrome/browser/ui/views...
File chrome/browser/ui/views/extensions/extension_view.h (right):

https://chromiumcodereview.appspot.com/9254046/diff/1/chrome/browser/ui/views...
chrome/browser/ui/views/extensions/extension_view.h:32: virtual void
OnRenderViewSizeChanged() {}
this method name is confusing. it sounds like the render view has updated its
size, when really the ui has been resized and is telling the render view about
it. maybe OnViewWasResized?

[jeremya, 2012-01-20 00:59:49]

https://chromiumcodereview.appspot.com/9254046/diff/1/chrome/browser/ui/views...
File chrome/browser/ui/views/extensions/extension_view.h (right):

https://chromiumcodereview.appspot.com/9254046/diff/1/chrome/browser/ui/views...
chrome/browser/ui/views/extensions/extension_view.h:32: virtual void
OnRenderViewSizeChanged() {}
On 2012/01/20 00:22:47, Matt Perry wrote:
> this method name is confusing. it sounds like the render view has updated its
> size, when really the ui has been resized and is telling the render view about
> it. maybe OnViewWasResized?

Done.

[Matt Perry, 2012-01-20 01:04:43]

lgtm

[Ben Goodger (Google), 2012-01-26 23:03:45]

So, what I think you should be able to do is:

- create a regular views Widget (TYPE_WINDOW).
- implement a non-client frame view. this will respond to nchittest window
resizes, etc.

https://chromiumcodereview.appspot.com/9254046/diff/5001/chrome/browser/ui/ex...
File chrome/browser/ui/extensions/native_shell_frame.h (right):

https://chromiumcodereview.appspot.com/9254046/diff/5001/chrome/browser/ui/ex...
chrome/browser/ui/extensions/native_shell_frame.h:10: class NativeWidget;
outdent

https://chromiumcodereview.appspot.com/9254046/diff/5001/chrome/browser/ui/vi...
File chrome/browser/ui/views/extensions/shell_frame_win.cc (right):

https://chromiumcodereview.appspot.com/9254046/diff/5001/chrome/browser/ui/vi...
chrome/browser/ui/views/extensions/shell_frame_win.cc:28:
DwmExtendFrameIntoClientArea(GetNativeView(), &margins);
This is the kind of thing that can be done in the resize callback.

https://chromiumcodereview.appspot.com/9254046/diff/5001/chrome/browser/ui/vi...
chrome/browser/ui/views/extensions/shell_frame_win.cc:38: SWP_FRAMECHANGED);
What are you attempting to do here?

https://chromiumcodereview.appspot.com/9254046/diff/5001/chrome/browser/ui/vi...
chrome/browser/ui/views/extensions/shell_frame_win.cc:53: } else if (y >= height
- 5) {
no else after return

https://chromiumcodereview.appspot.com/9254046/diff/5001/content/browser/rend...
File content/browser/renderer_host/render_widget_host_view_win.cc (right):

https://chromiumcodereview.appspot.com/9254046/diff/5001/content/browser/rend...
content/browser/renderer_host/render_widget_host_view_win.cc:1117: if
(transparent_region_.Get()) {
no braces

https://chromiumcodereview.appspot.com/9254046/diff/5001/content/browser/rend...
content/browser/renderer_host/render_widget_host_view_win.cc:1133: return 0;
no else after return

[jeremya, 2012-01-26 23:15:56]

I don't think I can use NonClientFrameView, since I need to respond very
specifically to the WM_NCCALCSIZE message. (See
http://msdn.microsoft.com/en-us/library/windows/desktop/bb688195(v=vs.85).asp...).

I can't see any way to do that by subclassing NCFV.

[beng (no_code_reviews), 2012-01-26 23:37:41]

Is it that you think you have to call DwmExtendFrameIntoClientArea() in
WM_NCCALCSIZE or that you feel you need to SetMsgHandled(TRUE); return 0;
from WM_NCCALCSIZE?

-Ben

On Thu, Jan 26, 2012 at 3:15 PM, <jeremya@chromium.org> wrote:

> I don't think I can use NonClientFrameView, since I need to respond very
> specifically to the WM_NCCALCSIZE message. (See
> http://msdn.microsoft.com/en-**us/library/windows/desktop/**
>
bb688195(v=vs.85).aspx#**removing<http://msdn.microsoft.com/en-us/library/windows/desktop/bb688195(v=vs.85).aspx#removing>
> ).
>
> I can't see any way to do that by subclassing NCFV.
>
>
https://chromiumcodereview.**appspot.com/9254046/<https://chromiumcodereview....
>

[jeremya, 2012-01-26 23:39:48]

On 2012/01/26 23:37:41, beng wrote:
> Is it that you think you have to call DwmExtendFrameIntoClientArea() in
> WM_NCCALCSIZE or that you feel you need to SetMsgHandled(TRUE); return 0;
> from WM_NCCALCSIZE?

Need to SetMsgHandled(TRUE) and return 0 from WM_NCCALCSIZE, and bump
SetWindowPos() on WM_CREATE to tell the DWM to send a WM_NCCALCSIZE message.

[beng (no_code_reviews), 2012-01-26 23:41:18]

So, specifically:

When you create the window, you want to send a frame change that'll result
in a calcsize so you can do something.

Do you need to do this something every calcsize, or just that first time?

What does this calcsize handling do for you?

-Ben

On Thu, Jan 26, 2012 at 3:39 PM, <jeremya@chromium.org> wrote:

> On 2012/01/26 23:37:41, beng wrote:
>
>> Is it that you think you have to call DwmExtendFrameIntoClientArea() in
>> WM_NCCALCSIZE or that you feel you need to SetMsgHandled(TRUE); return 0;
>> from WM_NCCALCSIZE?
>>
>
> Need to SetMsgHandled(TRUE) and return 0 from WM_NCCALCSIZE, and bump
> SetWindowPos() on WM_CREATE to tell the DWM to send a WM_NCCALCSIZE
> message.
>
>
https://chromiumcodereview.**appspot.com/9254046/<https://chromiumcodereview....
>

[jeremya, 2012-01-26 23:46:34]

On 2012/01/26 23:41:18, beng wrote:
> So, specifically:
> 
> When you create the window, you want to send a frame change that'll result
> in a calcsize so you can do something.
> 
> Do you need to do this something every calcsize, or just that first time?
> 
> What does this calcsize handling do for you?

Every calcsize I need to return 0 if the wParam is TRUE (according to the above
MSDN link.)

Returning 0 from calcsize like this removes the caption and icon, and sets the
client area to be the size of the whole window (so we can draw over the glass).

[jeremya, 2012-01-26 23:49:19]

On 2012/01/26 23:46:34, jeremya wrote:
> On 2012/01/26 23:41:18, beng wrote:
> > So, specifically:
> > 
> > When you create the window, you want to send a frame change that'll result
> > in a calcsize so you can do something.
> > 
> > Do you need to do this something every calcsize, or just that first time?
> > 
> > What does this calcsize handling do for you?
> 
> Every calcsize I need to return 0 if the wParam is TRUE (according to the
above
> MSDN link.)
> 
> Returning 0 from calcsize like this removes the caption and icon, and sets the
> client area to be the size of the whole window (so we can draw over the
glass).

To add to that -- the reason I can't just do what GlassBrowserFrameView does is
that the ClientAreaInsets that I need to return are (0,0,0,0) -- no border. But
NCFV special cases that to mean "default border".

[beng (no_code_reviews), 2012-01-26 23:50:05]

On Thu, Jan 26, 2012 at 3:49 PM, <jeremya@chromium.org> wrote:

> Every calcsize I need to return 0 if the wParam is TRUE (according to the
>>
> above
>
>> MSDN link.)
>>
>
>  Returning 0 from calcsize like this removes the caption and icon, and
>> sets the
>> client area to be the size of the whole window (so we can draw over the
>>
> glass).
>
> To add to that -- the reason I can't just do what GlassBrowserFrameView
> does is
> that the ClientAreaInsets that I need to return are (0,0,0,0) -- no
> border. But
> NCFV special cases that to mean "default border".
>

Where does it do that?

-Ben

[jeremya, 2012-01-26 23:54:03]

My bad -- it's not NCFV that does that. It's NativeWidgetWin::OnNCCalcSize.

On Fri, Jan 27, 2012 at 10:50 AM, Ben Goodger <beng@google.com> wrote:

> On Thu, Jan 26, 2012 at 3:49 PM, <jeremya@chromium.org> wrote:
>
>> Every calcsize I need to return 0 if the wParam is TRUE (according to the
>>>
>> above
>>
>>> MSDN link.)
>>>
>>
>>  Returning 0 from calcsize like this removes the caption and icon, and
>>> sets the
>>> client area to be the size of the whole window (so we can draw over the
>>>
>> glass).
>>
>> To add to that -- the reason I can't just do what GlassBrowserFrameView
>> does is
>> that the ClientAreaInsets that I need to return are (0,0,0,0) -- no
>> border. But
>> NCFV special cases that to mean "default border".
>>
>
> Where does it do that?
>
> -Ben
>

[beng (no_code_reviews), 2012-01-26 23:59:26]

So, I think we should fix NativeWidgetWin to handle your case.

General comment is I have spent much of the past 18 months getting rid of
NativeWidgetWin subclasses. They are a pain in my assholes. It makes it
harder to tweak the widget code when there are all these subclasses
tweaking behavior in undefined ways :-) I have had some success just making
NWW more flexible.

So, NCFV has a method GetWindowBoundsForClientBounds that is supposed to do
effectively what NCCALCSIZE does. Given some bounds for the window, return
the client rect. My assertion is that if this function returns the same
rect as that supplied, in NWW::OnNCCalcSize, you could SetMsgHandled(TRUE)
and return 0. Would that work?

-Ben

On Thu, Jan 26, 2012 at 3:53 PM, Jeremy Apthorp <jeremya@chromium.org>wrote:

> My bad -- it's not NCFV that does that. It's NativeWidgetWin::OnNCCalcSize.
>
>
> On Fri, Jan 27, 2012 at 10:50 AM, Ben Goodger <beng@google.com> wrote:
>
>> On Thu, Jan 26, 2012 at 3:49 PM, <jeremya@chromium.org> wrote:
>>
>>> Every calcsize I need to return 0 if the wParam is TRUE (according to the
>>>>
>>> above
>>>
>>>> MSDN link.)
>>>>
>>>
>>>  Returning 0 from calcsize like this removes the caption and icon, and
>>>> sets the
>>>> client area to be the size of the whole window (so we can draw over the
>>>>
>>> glass).
>>>
>>> To add to that -- the reason I can't just do what GlassBrowserFrameView
>>> does is
>>> that the ClientAreaInsets that I need to return are (0,0,0,0) -- no
>>> border. But
>>> NCFV special cases that to mean "default border".
>>>
>>
>> Where does it do that?
>>
>> -Ben
>>
>
>

[jeremya, 2012-01-27 00:29:21]

On Fri, Jan 27, 2012 at 10:59 AM, Ben Goodger <beng@google.com> wrote:

> So, I think we should fix NativeWidgetWin to handle your case.
>
> General comment is I have spent much of the past 18 months getting rid of
> NativeWidgetWin subclasses. They are a pain in my assholes. It makes it
> harder to tweak the widget code when there are all these subclasses
> tweaking behavior in undefined ways :-) I have had some success just making
> NWW more flexible.
>
> So, NCFV has a method GetWindowBoundsForClientBounds that is supposed to
> do effectively what NCCALCSIZE does. Given some bounds for the window,
> return the client rect. My assertion is that if this function returns the
> same rect as that supplied, in NWW::OnNCCalcSize, you could
> SetMsgHandled(TRUE) and return 0. Would that work?
>

Ah, okay -- that makes sense. Glad to have a bit of background :)

You say GetWindowBoundsForClientBounds returns the client rect for the
window bounds -- the name of the function suggests that it does the
opposite. Am I missing something?

Also, I'm not sure how the information from GetWindowBoundsForClientBounds
would get into NWW:OnNCCalcSize. As far as I can tell, currently
GetWindowBoundsForClientBounds seems to only affect GetPreferredSize() on
the NonClientView.

Do you mean something like this in NWW::OnNCCalcSize?

NonClientFrameView* frame_view =
GetWidget()->non_client_view()->frame_view();
gfx::Rect client_area = frame_view->GetBoundsForClientView();
gfx::Rect window_area =
frame_view->GetWindowBoundsForClientArea(client_area);
if (client_area == window_area) {
  SetMsgHandled(TRUE);
  return 0;
}

[beng (no_code_reviews), 2012-01-27 01:38:58]

oops you're right yes I had it backwards. Yes the code you show makes sense
to me.

-Ben

On Thu, Jan 26, 2012 at 4:28 PM, Jeremy Apthorp <jeremya@chromium.org>wrote:

> On Fri, Jan 27, 2012 at 10:59 AM, Ben Goodger <beng@google.com> wrote:
>
>> So, I think we should fix NativeWidgetWin to handle your case.
>>
>> General comment is I have spent much of the past 18 months getting rid of
>> NativeWidgetWin subclasses. They are a pain in my assholes. It makes it
>> harder to tweak the widget code when there are all these subclasses
>> tweaking behavior in undefined ways :-) I have had some success just making
>> NWW more flexible.
>>
>> So, NCFV has a method GetWindowBoundsForClientBounds that is supposed to
>> do effectively what NCCALCSIZE does. Given some bounds for the window,
>> return the client rect. My assertion is that if this function returns the
>> same rect as that supplied, in NWW::OnNCCalcSize, you could
>> SetMsgHandled(TRUE) and return 0. Would that work?
>>
>
> Ah, okay -- that makes sense. Glad to have a bit of background :)
>
> You say GetWindowBoundsForClientBounds returns the client rect for the
> window bounds -- the name of the function suggests that it does the
> opposite. Am I missing something?
>
> Also, I'm not sure how the information from GetWindowBoundsForClientBounds
> would get into NWW:OnNCCalcSize. As far as I can tell, currently
> GetWindowBoundsForClientBounds seems to only affect GetPreferredSize() on
> the NonClientView.
>
> Do you mean something like this in NWW::OnNCCalcSize?
>
> NonClientFrameView* frame_view =
> GetWidget()->non_client_view()->frame_view();
> gfx::Rect client_area = frame_view->GetBoundsForClientView();
> gfx::Rect window_area =
> frame_view->GetWindowBoundsForClientArea(client_area);
> if (client_area == window_area) {
>   SetMsgHandled(TRUE);
>   return 0;
> }
>

[jeremya, 2012-02-01 23:46:15]

Meanwhile: from where should I call DwmExtendFrameIntoClientArea? The only
other place this is done is
http://code.google.com/codesearch#OAMlx_jo-ck/src/chrome/browser/ui/views/fra...
in
a NWW subclass.

On Fri, Jan 27, 2012 at 12:38 PM, Ben Goodger <beng@google.com> wrote:

> oops you're right yes I had it backwards. Yes the code you show makes
> sense to me.
>
> -Ben
>
>
> On Thu, Jan 26, 2012 at 4:28 PM, Jeremy Apthorp <jeremya@chromium.org>wrote:
>
>> On Fri, Jan 27, 2012 at 10:59 AM, Ben Goodger <beng@google.com> wrote:
>>
>>> So, I think we should fix NativeWidgetWin to handle your case.
>>>
>>> General comment is I have spent much of the past 18 months getting rid
>>> of NativeWidgetWin subclasses. They are a pain in my assholes. It makes it
>>> harder to tweak the widget code when there are all these subclasses
>>> tweaking behavior in undefined ways :-) I have had some success just making
>>> NWW more flexible.
>>>
>>> So, NCFV has a method GetWindowBoundsForClientBounds that is supposed to
>>> do effectively what NCCALCSIZE does. Given some bounds for the window,
>>> return the client rect. My assertion is that if this function returns the
>>> same rect as that supplied, in NWW::OnNCCalcSize, you could
>>> SetMsgHandled(TRUE) and return 0. Would that work?
>>>
>>
>> Ah, okay -- that makes sense. Glad to have a bit of background :)
>>
>> You say GetWindowBoundsForClientBounds returns the client rect for the
>> window bounds -- the name of the function suggests that it does the
>> opposite. Am I missing something?
>>
>> Also, I'm not sure how the information from
>> GetWindowBoundsForClientBounds would get into NWW:OnNCCalcSize. As far as I
>> can tell, currently GetWindowBoundsForClientBounds seems to only affect
>> GetPreferredSize() on the NonClientView.
>>
>> Do you mean something like this in NWW::OnNCCalcSize?
>>
>> NonClientFrameView* frame_view =
>> GetWidget()->non_client_view()->frame_view();
>> gfx::Rect client_area = frame_view->GetBoundsForClientView();
>> gfx::Rect window_area =
>> frame_view->GetWindowBoundsForClientArea(client_area);
>> if (client_area == window_area) {
>>   SetMsgHandled(TRUE);
>>   return 0;
>> }
>>
>
>

[jeremya, 2012-02-02 04:19:02]

Another problem: for windows to actually remove the standard frame, it
needs to be convinced to send a WM_NCCALCSIZE message. I previously
achieved this by calling SetWindowPos from WM_CREATE.

However, WM_CREATE is called before the NonClientFrameView is instantiated.
This is problematic if the signal for removing the standard frame is to
come from the NCFV, because at the time of the first WM_NCCALCSIZE (i.e,
during the handling of WM_CREATE), the NCFV is yet to be created.

Perhaps this should be a widget init parameter?

On Thu, Feb 2, 2012 at 10:45 AM, Jeremy Apthorp <jeremya@chromium.org>wrote:

> Meanwhile: from where should I call DwmExtendFrameIntoClientArea? The only
> other place this is done is
>
http://code.google.com/codesearch#OAMlx_jo-ck/src/chrome/browser/ui/views/fra...
in
> a NWW subclass.
>
>
> On Fri, Jan 27, 2012 at 12:38 PM, Ben Goodger <beng@google.com> wrote:
>
>> oops you're right yes I had it backwards. Yes the code you show makes
>> sense to me.
>>
>> -Ben
>>
>>
>> On Thu, Jan 26, 2012 at 4:28 PM, Jeremy Apthorp <jeremya@chromium.org>wrote:
>>
>>> On Fri, Jan 27, 2012 at 10:59 AM, Ben Goodger <beng@google.com> wrote:
>>>
>>>> So, I think we should fix NativeWidgetWin to handle your case.
>>>>
>>>> General comment is I have spent much of the past 18 months getting rid
>>>> of NativeWidgetWin subclasses. They are a pain in my assholes. It makes it
>>>> harder to tweak the widget code when there are all these subclasses
>>>> tweaking behavior in undefined ways :-) I have had some success just making
>>>> NWW more flexible.
>>>>
>>>> So, NCFV has a method GetWindowBoundsForClientBounds that is supposed
>>>> to do effectively what NCCALCSIZE does. Given some bounds for the window,
>>>> return the client rect. My assertion is that if this function returns the
>>>> same rect as that supplied, in NWW::OnNCCalcSize, you could
>>>> SetMsgHandled(TRUE) and return 0. Would that work?
>>>>
>>>
>>> Ah, okay -- that makes sense. Glad to have a bit of background :)
>>>
>>> You say GetWindowBoundsForClientBounds returns the client rect for the
>>> window bounds -- the name of the function suggests that it does the
>>> opposite. Am I missing something?
>>>
>>> Also, I'm not sure how the information from
>>> GetWindowBoundsForClientBounds would get into NWW:OnNCCalcSize. As far as I
>>> can tell, currently GetWindowBoundsForClientBounds seems to only affect
>>> GetPreferredSize() on the NonClientView.
>>>
>>> Do you mean something like this in NWW::OnNCCalcSize?
>>>
>>> NonClientFrameView* frame_view =
>>> GetWidget()->non_client_view()->frame_view();
>>> gfx::Rect client_area = frame_view->GetBoundsForClientView();
>>> gfx::Rect window_area =
>>> frame_view->GetWindowBoundsForClientArea(client_area);
>>> if (client_area == window_area) {
>>>   SetMsgHandled(TRUE);
>>>   return 0;
>>> }
>>>
>>
>>
>

[jeremya, 2012-02-06 00:58:32]

I'm not sure the signal for 'Should remove standard frame' should come from the
NonClientFrameView -- I think it might make more sense as an init param (see
comments on native_widget_win.cc). Thoughts?

https://chromiumcodereview.appspot.com/9254046/diff/15002/ui/views/widget/nat...
File ui/views/widget/native_widget_win.cc (right):

https://chromiumcodereview.appspot.com/9254046/diff/15002/ui/views/widget/nat...
ui/views/widget/native_widget_win.cc:817: SWP_NOACTIVATE | SWP_NOZORDER |
SWP_FRAMECHANGED);
I added this to make the call to SetInitialBounds() in Widget::Init() cause
Windows to send a WM_NCCALCSIZE message. We probably don't want to use
SWP_FRAMECHANGED in every SetBounds call -- it's only needed during the
SetInitialBounds() call -- but I'm not sure what the best way to gate it is. (An
extra argument? A second function? A flag?)

https://chromiumcodereview.appspot.com/9254046/diff/15002/ui/views/widget/nat...
ui/views/widget/native_widget_win.cc:2367: NonClientFrameView* frame_view =
GetWidget()->non_client_view()->frame_view();
I've hit some problems (hopefully none of which remain) with this function being
called before the non_client_view gets initialized. It might be better/easier to
define this 'ShouldRemoveStandardFrame' thing in the init params, since we don't
support it changing after the window is created.

https://chromiumcodereview.appspot.com/9254046/diff/15002/ui/views/widget/nat...
ui/views/widget/native_widget_win.cc:2390:
DwmExtendFrameIntoClientArea(GetNativeView(), &m);
This is appropriate for the specific case of the ShellWindow, but conceivably
other clients might want different behaviour. I think it makes sense to
hard-code it until there's a need for generalization.

[Ben Goodger (Google), 2012-02-06 19:45:52]

On 2012/02/01 23:46:15, jeremya wrote:
> Meanwhile: from where should I call DwmExtendFrameIntoClientArea? The only
> other place this is done is
>
http://code.google.com/codesearch#OAMlx_jo-ck/src/chrome/browser/ui/views/fra...
> in
> a NWW subclass.

I'm not sure how many times you need to call this method. I think we might call
it too many times right now. My suspicion was that it could be called just once.
If so, you could call it when your frameview is added to the widget:

YourFrameView::ViewHierarchyChanged() {
  if (is_add && child == this) {
     // one-off init using GetWidget()->GetNativeView()
  }
}

Similarly, you can also send framechanged from here if you need to.

[jeremya, 2012-02-07 00:35:53]

On Tue, Feb 7, 2012 at 6:45 AM, <ben@chromium.org> wrote:

> On 2012/02/01 23:46:15, jeremya wrote:
>
>> Meanwhile: from where should I call DwmExtendFrameIntoClientArea? The only
>> other place this is done is
>>
>
> http://code.google.com/**codesearch#OAMlx_jo-ck/src/**
> chrome/browser/ui/views/frame/**browser_frame_win.cc&exact_**
>
package=chromium&q=**dwmextendframeintoclientarea&**l=200<http://code.google.com/codesearch#OAMlx_jo-ck/src/chrome/browser/ui/views/frame/browser_frame_win.cc&exact_package=chromium&q=dwmextendframeintoclientarea&l=200>
>
>> in
>> a NWW subclass.
>>
>
> I'm not sure how many times you need to call this method. I think we might
> call
> it too many times right now. My suspicion was that it could be called just
> once.
> If so, you could call it when your frameview is added to the widget:
>
> YourFrameView::**ViewHierarchyChanged() {
>  if (is_add && child == this) {
>     // one-off init using GetWidget()->GetNativeView()
>  }
> }
>
> Similarly, you can also send framechanged from here if you need to.
>

Do you mean I should add a bunch of stuff behind #if defined(OS_WIN) in the
NonClientFrameView? Seems like it would be nicer to keep all the
Win-specific stuff in NWW, and have it at least possible to reuse the NCFV
across Win and CrOS.

By comparison with BrowserFrameWin, DwmExtendFrameIntoClientArea() needs to
be called from OnWindowPosChanged() and UpdateFrameAfterFrameChange().

[jeremya, 2012-02-07 02:39:19]

On Tue, Feb 7, 2012 at 11:35 AM, Jeremy Apthorp <jeremya@chromium.org>wrote:

> On Tue, Feb 7, 2012 at 6:45 AM, <ben@chromium.org> wrote:
>
>> On 2012/02/01 23:46:15, jeremya wrote:
>>
>>> Meanwhile: from where should I call DwmExtendFrameIntoClientArea? The
>>> only
>>> other place this is done is
>>>
>>
>> http://code.google.com/**codesearch#OAMlx_jo-ck/src/**
>> chrome/browser/ui/views/frame/**browser_frame_win.cc&exact_**
>>
package=chromium&q=**dwmextendframeintoclientarea&**l=200<http://code.google.com/codesearch#OAMlx_jo-ck/src/chrome/browser/ui/views/frame/browser_frame_win.cc&exact_package=chromium&q=dwmextendframeintoclientarea&l=200>
>>
>>> in
>>> a NWW subclass.
>>>
>>
>> I'm not sure how many times you need to call this method. I think we
>> might call
>> it too many times right now. My suspicion was that it could be called
>> just once.
>> If so, you could call it when your frameview is added to the widget:
>>
>> YourFrameView::**ViewHierarchyChanged() {
>>  if (is_add && child == this) {
>>     // one-off init using GetWidget()->GetNativeView()
>>  }
>> }
>>
>> Similarly, you can also send framechanged from here if you need to.
>>
>
> Do you mean I should add a bunch of stuff behind #if defined(OS_WIN) in
> the NonClientFrameView? Seems like it would be nicer to keep all the
> Win-specific stuff in NWW, and have it at least possible to reuse the NCFV
> across Win and CrOS.
>
> By comparison with BrowserFrameWin, DwmExtendFrameIntoClientArea() needs
> to be called from OnWindowPosChanged() and UpdateFrameAfterFrameChange().
>

On closer inspection: might not need to call it from OnWindowPosChanged(),
but definitely need to call it when composition changes, otherwise the DWM
shadow doesn't come back when switching out of basic mode.
OnWindowPosChanged() seems to be called when composition changes -- I just
tested and the shadow correctly came back by just calling
DwmExtendFrameIntoClientArea() from OnWindowPosChanged().

I also experimented with making remove_standard_frame explicit in a
Widget::InitParam instead of an emergent condition from NCFV. I think this
cleans the code up a bit, and means that NWW doesn't need to be careful not
to use that information before the NCFV is constructed.

[jeremya, 2012-02-07 04:25:04]

Changed to use a Widget::InitParam.

[Ben Goodger (Google), 2012-02-07 20:18:57]

https://chromiumcodereview.appspot.com/9254046/diff/22003/chrome/browser/ui/v...
File chrome/browser/ui/views/extensions/shell_window_views.cc (right):

https://chromiumcodereview.appspot.com/9254046/diff/22003/chrome/browser/ui/v...
chrome/browser/ui/views/extensions/shell_window_views.cc:27: explicit
ShellWindowFrameView();
no explicit

https://chromiumcodereview.appspot.com/9254046/diff/22003/chrome/browser/ui/v...
chrome/browser/ui/views/extensions/shell_window_views.cc:39: };
 private:
  DISALLOW_COPY_AND..

https://chromiumcodereview.appspot.com/9254046/diff/22003/chrome/browser/ui/v...
chrome/browser/ui/views/extensions/shell_window_views.cc:60: if (y <= 5) {
nit: no braces around single line if

https://chromiumcodereview.appspot.com/9254046/diff/22003/content/browser/ren...
File content/browser/renderer_host/render_widget_host_view_win.cc (right):

https://chromiumcodereview.appspot.com/9254046/diff/22003/content/browser/ren...
content/browser/renderer_host/render_widget_host_view_win.cc:1154: if
(transparent_region_.Get()) {
nit: no braces on single line if

https://chromiumcodereview.appspot.com/9254046/diff/22003/content/browser/ren...
content/browser/renderer_host/render_widget_host_view_win.cc:1168: } else {
no else after return

https://chromiumcodereview.appspot.com/9254046/diff/22003/ui/views/widget/nat...
File ui/views/widget/native_widget_win.cc (right):

https://chromiumcodereview.appspot.com/9254046/diff/22003/ui/views/widget/nat...
ui/views/widget/native_widget_win.cc:555: UpdateDWMFrame();
no braces on single line if

https://chromiumcodereview.appspot.com/9254046/diff/22003/ui/views/widget/nat...
ui/views/widget/native_widget_win.cc:2393:
DwmExtendFrameIntoClientArea(GetNativeView(), &m);
I understand the NCCALCSIZE handling... but what is this required for?

https://chromiumcodereview.appspot.com/9254046/diff/22003/ui/views/widget/wid...
File ui/views/widget/widget.h (right):

https://chromiumcodereview.appspot.com/9254046/diff/22003/ui/views/widget/wid...
ui/views/widget/widget.h:166: bool remove_standard_frame;
Please document what this does, and note that it is windows-only.

[jeremya, 2012-02-08 00:35:16]

http://codereview.chromium.org/9254046/diff/22003/chrome/browser/ui/views/ext...
File chrome/browser/ui/views/extensions/shell_window_views.cc (right):

http://codereview.chromium.org/9254046/diff/22003/chrome/browser/ui/views/ext...
chrome/browser/ui/views/extensions/shell_window_views.cc:27: explicit
ShellWindowFrameView();
On 2012/02/07 20:18:57, Ben Goodger (Google) wrote:
> no explicit

Done.

http://codereview.chromium.org/9254046/diff/22003/chrome/browser/ui/views/ext...
chrome/browser/ui/views/extensions/shell_window_views.cc:39: };
On 2012/02/07 20:18:57, Ben Goodger (Google) wrote:
>  private:
>   DISALLOW_COPY_AND..

Done.

http://codereview.chromium.org/9254046/diff/22003/chrome/browser/ui/views/ext...
chrome/browser/ui/views/extensions/shell_window_views.cc:60: if (y <= 5) {
On 2012/02/07 20:18:57, Ben Goodger (Google) wrote:
> nit: no braces around single line if

Done.

http://codereview.chromium.org/9254046/diff/22003/content/browser/renderer_ho...
File content/browser/renderer_host/render_widget_host_view_win.cc (right):

http://codereview.chromium.org/9254046/diff/22003/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_view_win.cc:1154: if
(transparent_region_.Get()) {
On 2012/02/07 20:18:57, Ben Goodger (Google) wrote:
> nit: no braces on single line if

Done.

http://codereview.chromium.org/9254046/diff/22003/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_view_win.cc:1168: } else {
On 2012/02/07 20:18:57, Ben Goodger (Google) wrote:
> no else after return

Done.

http://codereview.chromium.org/9254046/diff/22003/ui/views/widget/native_widg...
File ui/views/widget/native_widget_win.cc (right):

http://codereview.chromium.org/9254046/diff/22003/ui/views/widget/native_widg...
ui/views/widget/native_widget_win.cc:555: UpdateDWMFrame();
On 2012/02/07 20:18:57, Ben Goodger (Google) wrote:
> no braces on single line if

Done.

http://codereview.chromium.org/9254046/diff/22003/ui/views/widget/native_widg...
ui/views/widget/native_widget_win.cc:2393:
DwmExtendFrameIntoClientArea(GetNativeView(), &m);
On 2012/02/07 20:18:57, Ben Goodger (Google) wrote:
> I understand the NCCALCSIZE handling... but what is this required for?

Without this, Windows doesn't draw any glass, which means we don't get a drop
shadow.

http://codereview.chromium.org/9254046/diff/22003/ui/views/widget/widget.h
File ui/views/widget/widget.h (right):

http://codereview.chromium.org/9254046/diff/22003/ui/views/widget/widget.h#ne...
ui/views/widget/widget.h:166: bool remove_standard_frame;
On 2012/02/07 20:18:57, Ben Goodger (Google) wrote:
> Please document what this does, and note that it is windows-only.

Done.

[Ben Goodger (Google), 2012-02-09 19:21:05]

http://codereview.chromium.org/9254046/diff/28001/ui/views/widget/native_widg...
File ui/views/widget/native_widget_win.cc (right):

http://codereview.chromium.org/9254046/diff/28001/ui/views/widget/native_widg...
ui/views/widget/native_widget_win.cc:555: UpdateDWMFrame();
Rather than call this here, and in OnCreate() below, it'd be safer if you'd do
this in OnWindowPosChanged when the flags correspond to a frame changed only.

[jeremya, 2012-02-09 23:15:50]

http://codereview.chromium.org/9254046/diff/28001/ui/views/widget/native_widg...
File ui/views/widget/native_widget_win.cc (right):

http://codereview.chromium.org/9254046/diff/28001/ui/views/widget/native_widg...
ui/views/widget/native_widget_win.cc:555: UpdateDWMFrame();
On 2012/02/09 19:21:06, Ben Goodger (Google) wrote:
> Rather than call this here, and in OnCreate() below, it'd be safer if you'd do
> this in OnWindowPosChanged when the flags correspond to a frame changed only.

Done.

[Ben Goodger (Google), 2012-02-10 01:01:30]

Great. LGTM.

[commit-bot: I haz the power, 2012-02-10 02:04:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jeremya@chromium.org/9254046/33001

[commit-bot: I haz the power, 2012-02-10 03:00:46]

Try job failure for 9254046-33001 (retry) on linux_rel for step "check_deps".
It's a second try, previously, step "check_deps" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2012-02-10 04:09:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jeremya@chromium.org/9254046/37003

[commit-bot: I haz the power, 2012-02-10 05:33:24]

Change committed as 121408

[Ben Goodger (Google), 2012-02-15 02:06:47]

LGTM

[sail, 2012-02-24 04:19:09]

http://codereview.chromium.org/9254046/diff/37003/content/browser/renderer_ho...
File content/browser/renderer_host/render_widget_host_view_win.cc (right):

http://codereview.chromium.org/9254046/diff/37003/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_view_win.cc:1155:
transparent_region_.release();
quick question, why do you need to do a release here? Won't this cause a memory
leak?