Get rid of the 1-pixel client inset hack.

Get rid of the 1-pixel client inset hack.

This patch attempts to fix various graphical glitches and corruption
once and for all.  The root of all these issues is the 1-pixel bottom
client inset hack, so this patch aims to replace that hack with a
(hopefully) less onerous hack.

Prior to this patch, there was a comment indicating that before
changing the frame type we must hide the window (and then show the window
after the frame type is changed) or else the client area will paint black.
However, no code was present to actually do this.  The original comment also
indicated that this was not necessary for chrome theme changes, but rather
only WM_DWMCOMPOSITIONCHANGED messages, but testing showed that this is
not the case, and it is in fact required for both cases.

Additionally, after changing the frame type, it is required to call
SetWindowPos() with SWP_FRAMECHANGED in order to notify windows of the
change.

BUG=312535, 311405
TEST=
  Classic -> Aero Test  (Win7 only)
    0. Shutdown Chrome
    1. Set a Windows 7 Basic Theme
    2. Start Chrome
    3. Change to Windows 7 Aero Theme
    4. Observe correct rendering of browser frame
  Aero -> Classic Test  (Win7 only)
    0. Shutdown Chrome
    1. Set a Windows 7 Aero Theme
    2. Start Chrome
    3. Change to Windows 7 Basic Theme
    4. Observe correct rendering of browser frame
  Chrome Theme Test  (Win7 w/ Aero Theme, Win8, Win8.1)
    1. Hotdog -> Settings -> Reset to Default Theme
    2. Close Chrome
    3. Start Chrome
    4. Install Brushed Theme
      https://chrome.google.com/webstore/detail/brushed/bfjgbcjfpbbfepcccpaffkjofcmglifg?hl=en
    5. Close Chrome
    6. Start Chrome
    7. Hotdog -> Settings -> Reset to Default Theme
    8. Observe correct rendering of browser frame

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=234386

[zturner, 2013-11-07 01:06:07]

Adding both sky and ben, since ben was the original author of the 1-pixel inset
hack.

[Ben Goodger (Google), 2013-11-07 02:46:16]

It's not just installing/resetting theme.. did you try doing this across
maximized/restore transitions, also resizing the window?

-Ben


On Wed, Nov 6, 2013 at 5:06 PM, <zturner@chromium.org> wrote:

> Reviewers: sky, Ben Goodger (Google),
>
> Message:
> Adding both sky and ben, since ben was the original author of the 1-pixel
> inset
> hack.
>
> Description:
> Get rid of the 1-pixel client inset hack.
>
> This patch attempts to fix various graphical glitches and corruption
> once and for all.  The root of all these issues is the 1-pixel bottom
> client inset hack, so this patch aims to replace that hack with a
> (hopefully) less onerous hack.
>
> Prior to this patch, there was a comment indicating that before
> changing the frame type we must hide the window (and then show the window
> after the frame type is changed) or else the client area will paint black.
> However, no code was present to actually do this.  The original comment
> also
> indicated that this was not necessary for chrome theme changes, but rather
> only WM_DWMCOMPOSITIONCHANGED messages, but testing showed that this is
> not the case, and it is in fact required for both cases.
>
> Additionally, after changing the frame type, it is required to call
> SetWindowPos() with SWP_FRAMECHANGED in order to notify windows of the
> change.
>
> After this patch, there is 1 outstanding issue.  On ananta's Win7 physical
> machine, black client area is still observed when switching from Classic ->
> Aero.  We feel this is acceptable given the extremely low frequency of
> users changing from classic -> aero, combined with the fact that not all
> machines will even exhibit this problem (it does not happen, for example,
> on my Win7 VM).  We were unable to determine the exact conditions as
> --disable-gpu did not fix the problem on ananta's machine.
>
> BUG=312535,311405
>
> TEST=
>   Classic -> Aero Test  (Win7 only)
>     0. Shutdown Chrome
>     1. Set a Windows 7 Basic Theme
>     2. Start Chrome
>     3. Change to Windows 7 Aero Theme
>     4. Observe correct rendering of browser frame
>   Aero -> Classic Test  (Win7 only)
>     0. Shutdown Chrome
>     1. Set a Windows 7 Aero Theme
>     2. Start Chrome
>     3. Change to Windows 7 Basic Theme
>     4. Observe correct rendering of browser frame
>   Chrome Theme Test  (Win7 w/ Aero Theme, Win8, Win8.1)
>     1. Hotdog -> Settings -> Reset to Default Theme
>     2. Close Chrome
>     3. Start Chrome
>     4. Install Brushed Theme
>
> https://chrome.google.com/webstore/detail/brushed/
> bfjgbcjfpbbfepcccpaffkjofcmglifg?hl=en
>     5. Close Chrome
>     6. Start Chrome
>     7. Hotdog -> Settings -> Reset to Default Theme
>     8. Observe correct rendering of browser frame
>
> Please review this at https://codereview.chromium.org/59043012/
>
> SVN Base: svn://svn.chromium.org/chrome/trunk/src
>
> Affected files (+37, -76 lines):
>   M ui/views/win/hwnd_message_handler.cc
>
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[zturner, 2013-11-07 04:03:14]

On 2013/11/07 02:46:16, Ben Goodger (Google) wrote:
> It's not just installing/resetting theme.. did you try doing this across
> maximized/restore transitions, also resizing the window?
> 
> -Ben
> 
> 
> On Wed, Nov 6, 2013 at 5:06 PM, <mailto:zturner@chromium.org> wrote:
> 
> > Reviewers: sky, Ben Goodger (Google),
> >
> > Message:
> > Adding both sky and ben, since ben was the original author of the 1-pixel
> > inset
> > hack.
> >
> > Description:
> > Get rid of the 1-pixel client inset hack.
> >
> > This patch attempts to fix various graphical glitches and corruption
> > once and for all.  The root of all these issues is the 1-pixel bottom
> > client inset hack, so this patch aims to replace that hack with a
> > (hopefully) less onerous hack.
> >
> > Prior to this patch, there was a comment indicating that before
> > changing the frame type we must hide the window (and then show the window
> > after the frame type is changed) or else the client area will paint black.
> > However, no code was present to actually do this.  The original comment
> > also
> > indicated that this was not necessary for chrome theme changes, but rather
> > only WM_DWMCOMPOSITIONCHANGED messages, but testing showed that this is
> > not the case, and it is in fact required for both cases.
> >
> > Additionally, after changing the frame type, it is required to call
> > SetWindowPos() with SWP_FRAMECHANGED in order to notify windows of the
> > change.
> >
> > After this patch, there is 1 outstanding issue.  On ananta's Win7 physical
> > machine, black client area is still observed when switching from Classic ->
> > Aero.  We feel this is acceptable given the extremely low frequency of
> > users changing from classic -> aero, combined with the fact that not all
> > machines will even exhibit this problem (it does not happen, for example,
> > on my Win7 VM).  We were unable to determine the exact conditions as
> > --disable-gpu did not fix the problem on ananta's machine.
> >
> > BUG=312535,311405
> >
> > TEST=
> >   Classic -> Aero Test  (Win7 only)
> >     0. Shutdown Chrome
> >     1. Set a Windows 7 Basic Theme
> >     2. Start Chrome
> >     3. Change to Windows 7 Aero Theme
> >     4. Observe correct rendering of browser frame
> >   Aero -> Classic Test  (Win7 only)
> >     0. Shutdown Chrome
> >     1. Set a Windows 7 Aero Theme
> >     2. Start Chrome
> >     3. Change to Windows 7 Basic Theme
> >     4. Observe correct rendering of browser frame
> >   Chrome Theme Test  (Win7 w/ Aero Theme, Win8, Win8.1)
> >     1. Hotdog -> Settings -> Reset to Default Theme
> >     2. Close Chrome
> >     3. Start Chrome
> >     4. Install Brushed Theme
> >
> > https://chrome.google.com/webstore/detail/brushed/
> > bfjgbcjfpbbfepcccpaffkjofcmglifg?hl=en
> >     5. Close Chrome
> >     6. Start Chrome
> >     7. Hotdog -> Settings -> Reset to Default Theme
> >     8. Observe correct rendering of browser frame
> >
> > Please review this at https://codereview.chromium.org/59043012/
> >
> > SVN Base: svn://svn.chromium.org/chrome/trunk/src
> >
> > Affected files (+37, -76 lines):
> >   M ui/views/win/hwnd_message_handler.cc
> >
> >
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

Yes, minimize / maximize / restore / snap / resize all seem to work.  There are
a 
few issues with the patch as is, but I have a fix for those in progress (mostly 
just code that I forgot to take out of this patch before uploading that messes
up 
focus / activation).

[zturner, 2013-11-07 04:03:58]

On 2013/11/07 04:03:14, zturner wrote:
> On 2013/11/07 02:46:16, Ben Goodger (Google) wrote:
> > It's not just installing/resetting theme.. did you try doing this across
> > maximized/restore transitions, also resizing the window?
> > 
> > -Ben
> > 
> > 
> > On Wed, Nov 6, 2013 at 5:06 PM, <mailto:zturner@chromium.org> wrote:
> > 
> > > Reviewers: sky, Ben Goodger (Google),
> > >
> > > Message:
> > > Adding both sky and ben, since ben was the original author of the 1-pixel
> > > inset
> > > hack.
> > >
> > > Description:
> > > Get rid of the 1-pixel client inset hack.
> > >
> > > This patch attempts to fix various graphical glitches and corruption
> > > once and for all.  The root of all these issues is the 1-pixel bottom
> > > client inset hack, so this patch aims to replace that hack with a
> > > (hopefully) less onerous hack.
> > >
> > > Prior to this patch, there was a comment indicating that before
> > > changing the frame type we must hide the window (and then show the window
> > > after the frame type is changed) or else the client area will paint black.
> > > However, no code was present to actually do this.  The original comment
> > > also
> > > indicated that this was not necessary for chrome theme changes, but rather
> > > only WM_DWMCOMPOSITIONCHANGED messages, but testing showed that this is
> > > not the case, and it is in fact required for both cases.
> > >
> > > Additionally, after changing the frame type, it is required to call
> > > SetWindowPos() with SWP_FRAMECHANGED in order to notify windows of the
> > > change.
> > >
> > > After this patch, there is 1 outstanding issue.  On ananta's Win7 physical
> > > machine, black client area is still observed when switching from Classic
->
> > > Aero.  We feel this is acceptable given the extremely low frequency of
> > > users changing from classic -> aero, combined with the fact that not all
> > > machines will even exhibit this problem (it does not happen, for example,
> > > on my Win7 VM).  We were unable to determine the exact conditions as
> > > --disable-gpu did not fix the problem on ananta's machine.
> > >
> > > BUG=312535,311405
> > >
> > > TEST=
> > >   Classic -> Aero Test  (Win7 only)
> > >     0. Shutdown Chrome
> > >     1. Set a Windows 7 Basic Theme
> > >     2. Start Chrome
> > >     3. Change to Windows 7 Aero Theme
> > >     4. Observe correct rendering of browser frame
> > >   Aero -> Classic Test  (Win7 only)
> > >     0. Shutdown Chrome
> > >     1. Set a Windows 7 Aero Theme
> > >     2. Start Chrome
> > >     3. Change to Windows 7 Basic Theme
> > >     4. Observe correct rendering of browser frame
> > >   Chrome Theme Test  (Win7 w/ Aero Theme, Win8, Win8.1)
> > >     1. Hotdog -> Settings -> Reset to Default Theme
> > >     2. Close Chrome
> > >     3. Start Chrome
> > >     4. Install Brushed Theme
> > >
> > > https://chrome.google.com/webstore/detail/brushed/
> > > bfjgbcjfpbbfepcccpaffkjofcmglifg?hl=en
> > >     5. Close Chrome
> > >     6. Start Chrome
> > >     7. Hotdog -> Settings -> Reset to Default Theme
> > >     8. Observe correct rendering of browser frame
> > >
> > > Please review this at https://codereview.chromium.org/59043012/
> > >
> > > SVN Base: svn://svn.chromium.org/chrome/trunk/src
> > >
> > > Affected files (+37, -76 lines):
> > >   M ui/views/win/hwnd_message_handler.cc
> > >
> > >
> > >
> > 
> > To unsubscribe from this group and stop receiving emails from it, send an
> email
> > to mailto:chromium-reviews+unsubscribe@chromium.org.
> 
> Yes, minimize / maximize / restore / snap / resize all seem to work.  There
are
> a 
> few issues with the patch as is, but I have a fix for those in progress
(mostly 
> just code that I forgot to take out of this patch before uploading that messes
> up 
> focus / activation).

After my next upload tomorrow, feel free to come by and play around with it on
my
machine and see if you can break anything.  I know we've tried to fix this many
times before, so I'd definitely like some careful eyes to look at this patch.

[zturner, 2013-11-07 21:23:15]

+gab, msw to cc so they can test patch.

This fixes an accidental introduction of some focus / activation bugs, and
additionally adds a call to UpdateWindow() after showing and hiding.  initial
reports suggests that this UpdateWindow() may fix the remaining outstanding
issue (Classic -> Glass Windows theme), which if so will mean no known issues
remain.

Feel free to download patch and test thoroughly.

[zturner, 2013-11-08 00:49:45]

After further testing the classic -> glass windows theme issue persists, but
only with a gpu enabled, so it is likely to be a bug in the gpu process or the
browser process is not correctly updating the gpu with the window state, so we
can track this issue separately.

[gab, 2013-11-08 15:28:31]

I tested this CL and the flow which I think was causing the black strips some
time ago still works fine with this CL, i.e.:

1) RDP from Chromebook
2) Put Chromebook to sleep without closing RDP session (sometimes I'd close it
but this time I didn't... not sure it makes a difference)
3) Open local session on physical machine (Windows flashes a bit; I think
because it switches back to Glass)

All tabstrips looked fine on my local build of Chrome with this CL after these
steps.

Cheers,
Gab

[jbauman, 2013-11-08 21:38:25]

https://codereview.chromium.org/59043012/diff/170001/ui/views/win/hwnd_messag...
File ui/views/win/hwnd_message_handler.cc (right):

https://codereview.chromium.org/59043012/diff/170001/ui/views/win/hwnd_messag...
ui/views/win/hwnd_message_handler.cc:796: // will still be black after switching
from Win7 classic theme -> win7 aero
Is FrameTypeChanged being correctly called on the root_window host and native
widget? This can happen if the compositor thinks everything's opaque, but it's
actually transparent.

[zturner, 2013-11-09 00:16:42]

On 2013/11/08 21:38:25, jbauman wrote:
>
https://codereview.chromium.org/59043012/diff/170001/ui/views/win/hwnd_messag...
> File ui/views/win/hwnd_message_handler.cc (right):
> 
>
https://codereview.chromium.org/59043012/diff/170001/ui/views/win/hwnd_messag...
> ui/views/win/hwnd_message_handler.cc:796: // will still be black after
switching
> from Win7 classic theme -> win7 aero
> Is FrameTypeChanged being correctly called on the root_window host and native
> widget? This can happen if the compositor thinks everything's opaque, but it's
> actually transparent.

It looks like we are calling FrameTypeChanged on a theme change, but maybe
there's something else going on.  Could be Angle related or GPU related.

The fact that it works correctly when the browser starts in glass mode (i.e.
glass -> change to classic -> change to glass), makes it feel like an angle bug
to me, but I'm not sure.

RDP works according to gab, so since the use case of switching from classic ->
glass is rare, and since it can be fixed by opening a new window, I feel like
it's ok to open a new bug for that one after this CL lands, and then have you or
another GPU person look into it and see if you can figure out if the GPU process
is doing anything differently between the two cases.

[jbauman, 2013-11-09 00:22:23]

On 2013/11/09 00:16:42, zturner wrote:
> On 2013/11/08 21:38:25, jbauman wrote:
> >
>
https://codereview.chromium.org/59043012/diff/170001/ui/views/win/hwnd_messag...
> > File ui/views/win/hwnd_message_handler.cc (right):
> > 
> >
>
https://codereview.chromium.org/59043012/diff/170001/ui/views/win/hwnd_messag...
> > ui/views/win/hwnd_message_handler.cc:796: // will still be black after
> switching
> > from Win7 classic theme -> win7 aero
> > Is FrameTypeChanged being correctly called on the root_window host and
native
> > widget? This can happen if the compositor thinks everything's opaque, but
it's
> > actually transparent.
> 
> It looks like we are calling FrameTypeChanged on a theme change, but maybe
> there's something else going on.  Could be Angle related or GPU related.
> 
> The fact that it works correctly when the browser starts in glass mode (i.e.
> glass -> change to classic -> change to glass), makes it feel like an angle
bug
> to me, but I'm not sure.

When chrome starts in glass mode, then layers (and the output) are created
transparent. If it starts in classic then they're created opaque, which is why
you'd see that bug when using the GPU. ANGLE couldn't care less.
> 
> RDP works according to gab, so since the use case of switching from classic ->
> glass is rare, and since it can be fixed by opening a new window, I feel like
> it's ok to open a new bug for that one after this CL lands, and then have you
or
> another GPU person look into it and see if you can figure out if the GPU
process
> is doing anything differently between the two cases.

[zturner, 2013-11-09 02:46:28]

Thanks to jbauman, figured out how to fix the black bar on classic -> glass. 
Latest patch should resolve all outstanding issues.

[zturner, 2013-11-11 19:55:29]

On 2013/11/09 02:46:28, zturner wrote:
> Thanks to jbauman, figured out how to fix the black bar on classic -> glass. 
> Latest patch should resolve all outstanding issues.

ping

[Ben Goodger (Google), 2013-11-11 20:30:07]

This is really cool.

LGTM.

https://codereview.chromium.org/59043012/diff/270001/ui/views/win/hwnd_messag...
File ui/views/win/hwnd_message_handler.cc (right):

https://codereview.chromium.org/59043012/diff/270001/ui/views/win/hwnd_messag...
ui/views/win/hwnd_message_handler.cc:1093: *insets = gfx::Insets(0, 0, 0, 0);
nit: gfx::Insets() should be sufficient.

[jbauman, 2013-11-11 22:02:50]

lgtm

[commit-bot: I haz the power, 2013-11-12 01:11:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/zturner@chromium.org/59043012/370001

[Peter Kasting, 2013-11-12 01:47:02]

Drive-by.  No need to get an LGTM from me.

I have been seeing lots of insanity on e.g. installing new video drivers while
Chrome windows are open.  Hope this fixes that!

https://codereview.chromium.org/59043012/diff/370001/ui/views/win/hwnd_messag...
File ui/views/win/hwnd_message_handler.cc (right):

https://codereview.chromium.org/59043012/diff/370001/ui/views/win/hwnd_messag...
ui/views/win/hwnd_message_handler.cc:788: SWP_NOMOVE | SWP_NOSIZE |
SWP_FRAMECHANGED);
Nit: All lines of args should begin at the same horizontal location  (3 places)

https://codereview.chromium.org/59043012/diff/370001/ui/views/win/hwnd_messag...
ui/views/win/hwnd_message_handler.cc:790: // The non-client view needs to update
too.
Does this explicitly need to happen before some of the code below?  If so note
what restrictions apply so no one ever reintroduces subtle bugs with later
refactorings.

https://codereview.chromium.org/59043012/diff/370001/ui/views/win/hwnd_messag...
ui/views/win/hwnd_message_handler.cc:801: SWP_NOMOVE | SWP_NOSIZE |
SWP_SHOWWINDOW);
How are these calls different than what you could do with a pair of ShowWindow()
calls with SW_HIDE and SW_SHOW?

https://codereview.chromium.org/59043012/diff/370001/ui/views/win/hwnd_messag...
ui/views/win/hwnd_message_handler.cc:1093: *insets = gfx::Insets(0, 0, 0, 0);
Nit: Just use gfx::Insets()

[zturner, 2013-11-12 01:54:24]

On 2013/11/12 01:47:02, Peter Kasting wrote:
> Drive-by.  No need to get an LGTM from me.
> 
> I have been seeing lots of insanity on e.g. installing new video drivers while
> Chrome windows are open.  Hope this fixes that!
> 
>
https://codereview.chromium.org/59043012/diff/370001/ui/views/win/hwnd_messag...
> File ui/views/win/hwnd_message_handler.cc (right):
> 
>
https://codereview.chromium.org/59043012/diff/370001/ui/views/win/hwnd_messag...
> ui/views/win/hwnd_message_handler.cc:788: SWP_NOMOVE | SWP_NOSIZE |
> SWP_FRAMECHANGED);
> Nit: All lines of args should begin at the same horizontal location  (3
places)
> 
>
https://codereview.chromium.org/59043012/diff/370001/ui/views/win/hwnd_messag...
> ui/views/win/hwnd_message_handler.cc:790: // The non-client view needs to
update
> too.
> Does this explicitly need to happen before some of the code below?  If so note
> what restrictions apply so no one ever reintroduces subtle bugs with later
> refactorings.
> 
>
https://codereview.chromium.org/59043012/diff/370001/ui/views/win/hwnd_messag...
> ui/views/win/hwnd_message_handler.cc:801: SWP_NOMOVE | SWP_NOSIZE |
> SWP_SHOWWINDOW);
> How are these calls different than what you could do with a pair of
ShowWindow()
> calls with SW_HIDE and SW_SHOW?
> 
>
https://codereview.chromium.org/59043012/diff/370001/ui/views/win/hwnd_messag...
> ui/views/win/hwnd_message_handler.cc:1093: *insets = gfx::Insets(0, 0, 0, 0);
> Nit: Just use gfx::Insets()


This stuff is in the CQ already so I'll fix these in a follow-up.  To answer
your question briefly,
I used SetWindowPos() instead of ShowWindow() because some tracing indicated
that ShowWindow()
/ HideWindow() end up sending a few more window messages than SetWindowPos, and
that extra work 
wasn't necessary to resolve the graphical glitches.  

In a followup CL, I will probably instead try to figure out why the Hide / Show
is even necessary 
in the first place and remove the calls entirely.  Since the Hide / Show is only
necessary for resetting
your custom chrome theme now (I tested changing classic -> glass without the
Hide / Show and it worked)
we can eliminate any funky Windows behavior from the equation since we're
rendering the entire window
ourselves, and hopefully it should be more tractable to track that one down
independently.

[commit-bot: I haz the power, 2013-11-12 02:54:57]

Change committed as 234386

[scottmg, 2013-11-12 03:57:16]

On 2013/11/12 02:54:57, I haz the power (commit-bot) wrote:
> Change committed as 234386

WOOHOO! Really hope this one sticks.