Fix the painting problems seen in the tabstrip when switching from a theme to glass on Windows.

Fix the painting problems seen in the tabstrip when switching from a theme to glass on Windows.

This regressed with the LegacyRenderWidgetHostHWND. It looks like the reason is that the LegacyRenderWidgetHostHWND eats
paint messages which would previously cause an invalidation in the parent when bounds change, etc.

Proposed fix is to pass SWP_NOREDRAW to the SetWindowPos calls used by the LegacyRenderWidgetHostHWND class.

BUG=388690
R=sky

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

[ananta, 2014-08-05 00:24:03]

https://codereview.chromium.org/440793002/diff/1/ui/views/win/hwnd_message_ha...
File ui/views/win/hwnd_message_handler.cc (right):

https://codereview.chromium.org/440793002/diff/1/ui/views/win/hwnd_message_ha...
ui/views/win/hwnd_message_handler.cc:824:
base::TimeDelta::FromMilliseconds(500));
It now appears that the bug was never fixed. We need to merge this to M37.

[sky, 2014-08-05 01:30:56]

https://codereview.chromium.org/440793002/diff/1/ui/views/win/hwnd_message_ha...
File ui/views/win/hwnd_message_handler.cc (right):

https://codereview.chromium.org/440793002/diff/1/ui/views/win/hwnd_message_ha...
ui/views/win/hwnd_message_handler.cc:824:
base::TimeDelta::FromMilliseconds(500));
On 2014/08/05 00:24:03, ananta wrote:
> It now appears that the bug was never fixed. We need to merge this to M37.

This is a total hack. Why do we need to delay?
Also, I don't like posting to a potentially deleted window.

[ananta, 2014-08-05 15:12:08]

On 2014/08/05 01:30:56, sky wrote:
>
https://codereview.chromium.org/440793002/diff/1/ui/views/win/hwnd_message_ha...
> File ui/views/win/hwnd_message_handler.cc (right):
> 
>
https://codereview.chromium.org/440793002/diff/1/ui/views/win/hwnd_message_ha...
> ui/views/win/hwnd_message_handler.cc:824:
> base::TimeDelta::FromMilliseconds(500));
> On 2014/08/05 00:24:03, ananta wrote:
> > It now appears that the bug was never fixed. We need to merge this to M37.
> 
> This is a total hack. Why do we need to delay?
> Also, I don't like posting to a potentially deleted window.

I debugged this a bit more. As you had mentioned a while back, this has
regressed due to the legacy window.
The sequence of events is as below:-

Without the legacy window:-
1. When we hit undo on the infobar to reset the theme we get a bunch of bounds
changed notifications in RWHVA. It also forwards them to Legacy.
2. The infobar is removed triggering a layout on the BrowserView.
3. The main window gets a paint.
4. Everything is good.

With the legacy window:-
1. When we hit undo on the infobar to reset the theme we get a bunch of bounds
changed notifications in RWHVA. It also forwards them to Legacy.
2. The infobar is removed triggering a layout on the BrowserView.
3. The legacy window gets a paint. The main window does not.
4. Tabstrip does not paint.

The difference is in step 3. The layer does schedule a paint correctly in both
cases. It appears that the main window paint is required for all this to work
correctly.
We can trigger a main window paint in the legacy window if its bounds change. I
tested that and it works reliably. If this sounds reasonable I will upload a
patch with that.

[ananta, 2014-08-05 15:21:23]

On 2014/08/05 15:12:08, ananta wrote:
> On 2014/08/05 01:30:56, sky wrote:
> >
>
https://codereview.chromium.org/440793002/diff/1/ui/views/win/hwnd_message_ha...
> > File ui/views/win/hwnd_message_handler.cc (right):
> > 
> >
>
https://codereview.chromium.org/440793002/diff/1/ui/views/win/hwnd_message_ha...
> > ui/views/win/hwnd_message_handler.cc:824:
> > base::TimeDelta::FromMilliseconds(500));
> > On 2014/08/05 00:24:03, ananta wrote:
> > > It now appears that the bug was never fixed. We need to merge this to M37.
> > 
> > This is a total hack. Why do we need to delay?
> > Also, I don't like posting to a potentially deleted window.
> 
> I debugged this a bit more. As you had mentioned a while back, this has
> regressed due to the legacy window.
> The sequence of events is as below:-
> 
> Without the legacy window:-
> 1. When we hit undo on the infobar to reset the theme we get a bunch of bounds
> changed notifications in RWHVA. It also forwards them to Legacy.
> 2. The infobar is removed triggering a layout on the BrowserView.
> 3. The main window gets a paint.
> 4. Everything is good.
> 
> With the legacy window:-
> 1. When we hit undo on the infobar to reset the theme we get a bunch of bounds
> changed notifications in RWHVA. It also forwards them to Legacy.
> 2. The infobar is removed triggering a layout on the BrowserView.
> 3. The legacy window gets a paint. The main window does not.
> 4. Tabstrip does not paint.
> 
> The difference is in step 3. The layer does schedule a paint correctly in both
> cases. It appears that the main window paint is required for all this to work
> correctly.
> We can trigger a main window paint in the legacy window if its bounds change.
I
> tested that and it works reliably. If this sounds reasonable I will upload a
> patch with that.

Updated the patch as per my previous comment for your reference.

[sky, 2014-08-05 17:36:41]

On Tue, Aug 5, 2014 at 8:12 AM,  <ananta@chromium.org> wrote:
> On 2014/08/05 01:30:56, sky wrote:
>
>
https://codereview.chromium.org/440793002/diff/1/ui/views/win/hwnd_message_ha...
>>
>> File ui/views/win/hwnd_message_handler.cc (right):
>
>
>
>
https://codereview.chromium.org/440793002/diff/1/ui/views/win/hwnd_message_ha...
>>
>> ui/views/win/hwnd_message_handler.cc:824:
>> base::TimeDelta::FromMilliseconds(500));
>> On 2014/08/05 00:24:03, ananta wrote:
>> > It now appears that the bug was never fixed. We need to merge this to
>> > M37.
>
>
>> This is a total hack. Why do we need to delay?
>> Also, I don't like posting to a potentially deleted window.
>
>
> I debugged this a bit more. As you had mentioned a while back, this has
> regressed due to the legacy window.
> The sequence of events is as below:-
>
> Without the legacy window:-
> 1. When we hit undo on the infobar to reset the theme we get a bunch of
> bounds
> changed notifications in RWHVA. It also forwards them to Legacy.
> 2. The infobar is removed triggering a layout on the BrowserView.
> 3. The main window gets a paint.
> 4. Everything is good.
>
> With the legacy window:-
> 1. When we hit undo on the infobar to reset the theme we get a bunch of
> bounds
> changed notifications in RWHVA. It also forwards them to Legacy.
> 2. The infobar is removed triggering a layout on the BrowserView.

This should also trigger a SchedulePaint. I would also expect that any
time we change the theme we explicitly tell the whole window to paint.

> 3. The legacy window gets a paint. The main window does not.
> 4. Tabstrip does not paint.
>
> The difference is in step 3. The layer does schedule a paint correctly in
> both
> cases. It appears that the main window paint is required for all this to
> work
> correctly.
> We can trigger a main window paint in the legacy window if its bounds
> change. I
> tested that and it works reliably. If this sounds reasonable I will upload a
> patch with that.

We shouldn't assume every bounds change of the legacy window requires
a paint. I think that'll lead to way too much painting.

  -Scott

>
>
>
>
>
>
> https://codereview.chromium.org/440793002/

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

[ananta, 2014-08-05 17:58:28]

On 2014/08/05 17:36:41, sky wrote:
> On Tue, Aug 5, 2014 at 8:12 AM,  <mailto:ananta@chromium.org> wrote:
> > On 2014/08/05 01:30:56, sky wrote:
> >
> >
>
https://codereview.chromium.org/440793002/diff/1/ui/views/win/hwnd_message_ha...
> >>
> >> File ui/views/win/hwnd_message_handler.cc (right):
> >
> >
> >
> >
>
https://codereview.chromium.org/440793002/diff/1/ui/views/win/hwnd_message_ha...
> >>
> >> ui/views/win/hwnd_message_handler.cc:824:
> >> base::TimeDelta::FromMilliseconds(500));
> >> On 2014/08/05 00:24:03, ananta wrote:
> >> > It now appears that the bug was never fixed. We need to merge this to
> >> > M37.
> >
> >
> >> This is a total hack. Why do we need to delay?
> >> Also, I don't like posting to a potentially deleted window.
> >
> >
> > I debugged this a bit more. As you had mentioned a while back, this has
> > regressed due to the legacy window.
> > The sequence of events is as below:-
> >
> > Without the legacy window:-
> > 1. When we hit undo on the infobar to reset the theme we get a bunch of
> > bounds
> > changed notifications in RWHVA. It also forwards them to Legacy.
> > 2. The infobar is removed triggering a layout on the BrowserView.
> > 3. The main window gets a paint.
> > 4. Everything is good.
> >
> > With the legacy window:-
> > 1. When we hit undo on the infobar to reset the theme we get a bunch of
> > bounds
> > changed notifications in RWHVA. It also forwards them to Legacy.
> > 2. The infobar is removed triggering a layout on the BrowserView.
> 
> This should also trigger a SchedulePaint. I would also expect that any
> time we change the theme we explicitly tell the whole window to paint.
> 
> > 3. The legacy window gets a paint. The main window does not.
> > 4. Tabstrip does not paint.
> >
> > The difference is in step 3. The layer does schedule a paint correctly in
> > both
> > cases. It appears that the main window paint is required for all this to
> > work
> > correctly.
> > We can trigger a main window paint in the legacy window if its bounds
> > change. I
> > tested that and it works reliably. If this sounds reasonable I will upload a
> > patch with that.
> 
> We shouldn't assume every bounds change of the legacy window requires
> a paint. I think that'll lead to way too much painting.
> 
>   -Scott
> 
> >
> >
> >
> >
> >
> >
> > https://codereview.chromium.org/440793002/
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

Fair enough. Updated the patch to invalidate the parent when the bounds change
and after we receive
a theme changed notification. That is tracked via the WM_DWMCOMPOSITIONCHANGED
message which HWNDMessageHandler
sends to its children during theme changes.

[sky, 2014-08-05 21:54:08]

This code should work with and without a legacy hwnd. And I still
don't think legacyhwnd should be posting to the parent for this. When
we change the theme we should explicitly invoke a SchedulePaint on the
widget.

On Tue, Aug 5, 2014 at 10:58 AM,  <ananta@chromium.org> wrote:
> On 2014/08/05 17:36:41, sky wrote:
>
>> On Tue, Aug 5, 2014 at 8:12 AM,  <mailto:ananta@chromium.org> wrote:
>> > On 2014/08/05 01:30:56, sky wrote:
>> >
>> >
>
>
>
https://codereview.chromium.org/440793002/diff/1/ui/views/win/hwnd_message_ha...
>>
>> >>
>> >> File ui/views/win/hwnd_message_handler.cc (right):
>> >
>> >
>> >
>> >
>
>
>
https://codereview.chromium.org/440793002/diff/1/ui/views/win/hwnd_message_ha...
>>
>> >>
>> >> ui/views/win/hwnd_message_handler.cc:824:
>> >> base::TimeDelta::FromMilliseconds(500));
>> >> On 2014/08/05 00:24:03, ananta wrote:
>> >> > It now appears that the bug was never fixed. We need to merge this to
>> >> > M37.
>> >
>> >
>> >> This is a total hack. Why do we need to delay?
>> >> Also, I don't like posting to a potentially deleted window.
>> >
>> >
>> > I debugged this a bit more. As you had mentioned a while back, this has
>> > regressed due to the legacy window.
>> > The sequence of events is as below:-
>> >
>> > Without the legacy window:-
>> > 1. When we hit undo on the infobar to reset the theme we get a bunch of
>> > bounds
>> > changed notifications in RWHVA. It also forwards them to Legacy.
>> > 2. The infobar is removed triggering a layout on the BrowserView.
>> > 3. The main window gets a paint.
>> > 4. Everything is good.
>> >
>> > With the legacy window:-
>> > 1. When we hit undo on the infobar to reset the theme we get a bunch of
>> > bounds
>> > changed notifications in RWHVA. It also forwards them to Legacy.
>> > 2. The infobar is removed triggering a layout on the BrowserView.
>
>
>> This should also trigger a SchedulePaint. I would also expect that any
>> time we change the theme we explicitly tell the whole window to paint.
>
>
>> > 3. The legacy window gets a paint. The main window does not.
>> > 4. Tabstrip does not paint.
>> >
>> > The difference is in step 3. The layer does schedule a paint correctly
>> > in
>> > both
>> > cases. It appears that the main window paint is required for all this to
>> > work
>> > correctly.
>> > We can trigger a main window paint in the legacy window if its bounds
>> > change. I
>> > tested that and it works reliably. If this sounds reasonable I will
>> > upload a
>> > patch with that.
>
>
>> We shouldn't assume every bounds change of the legacy window requires
>> a paint. I think that'll lead to way too much painting.
>
>
>>    -Scott
>
>
>> >
>> >
>> >
>> >
>> >
>> >
>> > https://codereview.chromium.org/440793002/
>
>
>> To unsubscribe from this group and stop receiving emails from it, send an
>
> email
>>
>> to mailto:chromium-reviews+unsubscribe@chromium.org.
>
>
> Fair enough. Updated the patch to invalidate the parent when the bounds
> change
> and after we receive
> a theme changed notification. That is tracked via the
> WM_DWMCOMPOSITIONCHANGED
> message which HWNDMessageHandler
> sends to its children during theme changes.
>
>
> https://codereview.chromium.org/440793002/

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

[ananta, 2014-08-05 23:03:38]

On 2014/08/05 21:54:08, sky wrote:
> This code should work with and without a legacy hwnd. And I still
> don't think legacyhwnd should be posting to the parent for this. When
> we change the theme we should explicitly invoke a SchedulePaint on the
> widget.
> 
> On Tue, Aug 5, 2014 at 10:58 AM,  <mailto:ananta@chromium.org> wrote:
> > On 2014/08/05 17:36:41, sky wrote:
> >
> >> On Tue, Aug 5, 2014 at 8:12 AM,  <mailto:ananta@chromium.org> wrote:
> >> > On 2014/08/05 01:30:56, sky wrote:
> >> >
> >> >
> >
> >
> >
>
https://codereview.chromium.org/440793002/diff/1/ui/views/win/hwnd_message_ha...
> >>
> >> >>
> >> >> File ui/views/win/hwnd_message_handler.cc (right):
> >> >
> >> >
> >> >
> >> >
> >
> >
> >
>
https://codereview.chromium.org/440793002/diff/1/ui/views/win/hwnd_message_ha...
> >>
> >> >>
> >> >> ui/views/win/hwnd_message_handler.cc:824:
> >> >> base::TimeDelta::FromMilliseconds(500));
> >> >> On 2014/08/05 00:24:03, ananta wrote:
> >> >> > It now appears that the bug was never fixed. We need to merge this to
> >> >> > M37.
> >> >
> >> >
> >> >> This is a total hack. Why do we need to delay?
> >> >> Also, I don't like posting to a potentially deleted window.
> >> >
> >> >
> >> > I debugged this a bit more. As you had mentioned a while back, this has
> >> > regressed due to the legacy window.
> >> > The sequence of events is as below:-
> >> >
> >> > Without the legacy window:-
> >> > 1. When we hit undo on the infobar to reset the theme we get a bunch of
> >> > bounds
> >> > changed notifications in RWHVA. It also forwards them to Legacy.
> >> > 2. The infobar is removed triggering a layout on the BrowserView.
> >> > 3. The main window gets a paint.
> >> > 4. Everything is good.
> >> >
> >> > With the legacy window:-
> >> > 1. When we hit undo on the infobar to reset the theme we get a bunch of
> >> > bounds
> >> > changed notifications in RWHVA. It also forwards them to Legacy.
> >> > 2. The infobar is removed triggering a layout on the BrowserView.
> >
> >
> >> This should also trigger a SchedulePaint. I would also expect that any
> >> time we change the theme we explicitly tell the whole window to paint.
> >
> >
> >> > 3. The legacy window gets a paint. The main window does not.
> >> > 4. Tabstrip does not paint.
> >> >
> >> > The difference is in step 3. The layer does schedule a paint correctly
> >> > in
> >> > both
> >> > cases. It appears that the main window paint is required for all this to
> >> > work
> >> > correctly.
> >> > We can trigger a main window paint in the legacy window if its bounds
> >> > change. I
> >> > tested that and it works reliably. If this sounds reasonable I will
> >> > upload a
> >> > patch with that.
> >
> >
> >> We shouldn't assume every bounds change of the legacy window requires
> >> a paint. I think that'll lead to way too much painting.
> >
> >
> >>    -Scott
> >
> >
> >> >
> >> >
> >> >
> >> >
> >> >
> >> >
> >> > https://codereview.chromium.org/440793002/
> >
> >
> >> To unsubscribe from this group and stop receiving emails from it, send an
> >
> > email
> >>
> >> to mailto:chromium-reviews+unsubscribe@chromium.org.
> >
> >
> > Fair enough. Updated the patch to invalidate the parent when the bounds
> > change
> > and after we receive
> > a theme changed notification. That is tracked via the
> > WM_DWMCOMPOSITIONCHANGED
> > message which HWNDMessageHandler
> > sends to its children during theme changes.
> >
> >
> > https://codereview.chromium.org/440793002/
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

When the Undo infobar is clicked after the theme is applied, the following
events happen:-
1. HWNDMessageHandler::FrameTypeChanged is called which changes the frame from
opaque to glass.
   This in turn does the DWM magic, etc.
2. FrameTypeChanged returns. Now the infobar is removed which triggers  a layout
of the BrowserView.
3. The bounds of the RWHVA are changed to occupy the additional space left by
the infobar.
4. The Legacy window paints the new area leaving the tabstrip in a bad state
until the next paint.

I think a combination of the two is needed here. SchedulePaint in
HWNDMessageHandler::FrameTypeChanged and
an invalidation on the parent from the legacy window to ensure that it works
correctly with and without the
legacy window.

[ananta, 2014-08-05 23:28:24]

On 2014/08/05 23:03:38, ananta wrote:
> On 2014/08/05 21:54:08, sky wrote:
> > This code should work with and without a legacy hwnd. And I still
> > don't think legacyhwnd should be posting to the parent for this. When
> > we change the theme we should explicitly invoke a SchedulePaint on the
> > widget.
> > 
> > On Tue, Aug 5, 2014 at 10:58 AM,  <mailto:ananta@chromium.org> wrote:
> > > On 2014/08/05 17:36:41, sky wrote:
> > >
> > >> On Tue, Aug 5, 2014 at 8:12 AM,  <mailto:ananta@chromium.org> wrote:
> > >> > On 2014/08/05 01:30:56, sky wrote:
> > >> >
> > >> >
> > >
> > >
> > >
> >
>
https://codereview.chromium.org/440793002/diff/1/ui/views/win/hwnd_message_ha...
> > >>
> > >> >>
> > >> >> File ui/views/win/hwnd_message_handler.cc (right):
> > >> >
> > >> >
> > >> >
> > >> >
> > >
> > >
> > >
> >
>
https://codereview.chromium.org/440793002/diff/1/ui/views/win/hwnd_message_ha...
> > >>
> > >> >>
> > >> >> ui/views/win/hwnd_message_handler.cc:824:
> > >> >> base::TimeDelta::FromMilliseconds(500));
> > >> >> On 2014/08/05 00:24:03, ananta wrote:
> > >> >> > It now appears that the bug was never fixed. We need to merge this
to
> > >> >> > M37.
> > >> >
> > >> >
> > >> >> This is a total hack. Why do we need to delay?
> > >> >> Also, I don't like posting to a potentially deleted window.
> > >> >
> > >> >
> > >> > I debugged this a bit more. As you had mentioned a while back, this has
> > >> > regressed due to the legacy window.
> > >> > The sequence of events is as below:-
> > >> >
> > >> > Without the legacy window:-
> > >> > 1. When we hit undo on the infobar to reset the theme we get a bunch of
> > >> > bounds
> > >> > changed notifications in RWHVA. It also forwards them to Legacy.
> > >> > 2. The infobar is removed triggering a layout on the BrowserView.
> > >> > 3. The main window gets a paint.
> > >> > 4. Everything is good.
> > >> >
> > >> > With the legacy window:-
> > >> > 1. When we hit undo on the infobar to reset the theme we get a bunch of
> > >> > bounds
> > >> > changed notifications in RWHVA. It also forwards them to Legacy.
> > >> > 2. The infobar is removed triggering a layout on the BrowserView.
> > >
> > >
> > >> This should also trigger a SchedulePaint. I would also expect that any
> > >> time we change the theme we explicitly tell the whole window to paint.
> > >
> > >
> > >> > 3. The legacy window gets a paint. The main window does not.
> > >> > 4. Tabstrip does not paint.
> > >> >
> > >> > The difference is in step 3. The layer does schedule a paint correctly
> > >> > in
> > >> > both
> > >> > cases. It appears that the main window paint is required for all this
to
> > >> > work
> > >> > correctly.
> > >> > We can trigger a main window paint in the legacy window if its bounds
> > >> > change. I
> > >> > tested that and it works reliably. If this sounds reasonable I will
> > >> > upload a
> > >> > patch with that.
> > >
> > >
> > >> We shouldn't assume every bounds change of the legacy window requires
> > >> a paint. I think that'll lead to way too much painting.
> > >
> > >
> > >>    -Scott
> > >
> > >
> > >> >
> > >> >
> > >> >
> > >> >
> > >> >
> > >> >
> > >> > https://codereview.chromium.org/440793002/
> > >
> > >
> > >> To unsubscribe from this group and stop receiving emails from it, send an
> > >
> > > email
> > >>
> > >> to mailto:chromium-reviews+unsubscribe@chromium.org.
> > >
> > >
> > > Fair enough. Updated the patch to invalidate the parent when the bounds
> > > change
> > > and after we receive
> > > a theme changed notification. That is tracked via the
> > > WM_DWMCOMPOSITIONCHANGED
> > > message which HWNDMessageHandler
> > > sends to its children during theme changes.
> > >
> > >
> > > https://codereview.chromium.org/440793002/
> > 
> > To unsubscribe from this group and stop receiving emails from it, send an
> email
> > to mailto:chromium-reviews+unsubscribe@chromium.org.
> 
> When the Undo infobar is clicked after the theme is applied, the following
> events happen:-
> 1. HWNDMessageHandler::FrameTypeChanged is called which changes the frame from
> opaque to glass.
>    This in turn does the DWM magic, etc.
> 2. FrameTypeChanged returns. Now the infobar is removed which triggers  a
layout
> of the BrowserView.
> 3. The bounds of the RWHVA are changed to occupy the additional space left by
> the infobar.
> 4. The Legacy window paints the new area leaving the tabstrip in a bad state
> until the next paint.
> 
> I think a combination of the two is needed here. SchedulePaint in
> HWNDMessageHandler::FrameTypeChanged and
> an invalidation on the parent from the legacy window to ensure that it works
> correctly with and without the
> legacy window.

The parent window gets a paint without the legacy window due to the
ResetWindowRgn calls in FrameTypeChanged.
With the legacy window these calls don't seem to be generating a paint on the
parent.

[sky, 2014-08-06 00:21:58]

It seems to me the code path that is triggering the theme to change
should explicitly SChedulePaint. Is that not happening?

On Tue, Aug 5, 2014 at 4:28 PM,  <ananta@chromium.org> wrote:
> On 2014/08/05 23:03:38, ananta wrote:
>>
>> On 2014/08/05 21:54:08, sky wrote:
>> > This code should work with and without a legacy hwnd. And I still
>> > don't think legacyhwnd should be posting to the parent for this. When
>> > we change the theme we should explicitly invoke a SchedulePaint on the
>> > widget.
>> >
>> > On Tue, Aug 5, 2014 at 10:58 AM,  <mailto:ananta@chromium.org> wrote:
>> > > On 2014/08/05 17:36:41, sky wrote:
>> > >
>> > >> On Tue, Aug 5, 2014 at 8:12 AM,  <mailto:ananta@chromium.org> wrote:
>> > >> > On 2014/08/05 01:30:56, sky wrote:
>> > >> >
>> > >> >
>> > >
>> > >
>> > >
>> >
>
>
>
https://codereview.chromium.org/440793002/diff/1/ui/views/win/hwnd_message_ha...
>>
>> > >>
>> > >> >>
>> > >> >> File ui/views/win/hwnd_message_handler.cc (right):
>> > >> >
>> > >> >
>> > >> >
>> > >> >
>> > >
>> > >
>> > >
>> >
>
>
>
https://codereview.chromium.org/440793002/diff/1/ui/views/win/hwnd_message_ha...
>>
>> > >>
>> > >> >>
>> > >> >> ui/views/win/hwnd_message_handler.cc:824:
>> > >> >> base::TimeDelta::FromMilliseconds(500));
>> > >> >> On 2014/08/05 00:24:03, ananta wrote:
>> > >> >> > It now appears that the bug was never fixed. We need to merge
>> > >> >> > this
>
> to
>>
>> > >> >> > M37.
>> > >> >
>> > >> >
>> > >> >> This is a total hack. Why do we need to delay?
>> > >> >> Also, I don't like posting to a potentially deleted window.
>> > >> >
>> > >> >
>> > >> > I debugged this a bit more. As you had mentioned a while back, this
>> > >> > has
>> > >> > regressed due to the legacy window.
>> > >> > The sequence of events is as below:-
>> > >> >
>> > >> > Without the legacy window:-
>> > >> > 1. When we hit undo on the infobar to reset the theme we get a
>> > >> > bunch of
>> > >> > bounds
>> > >> > changed notifications in RWHVA. It also forwards them to Legacy.
>> > >> > 2. The infobar is removed triggering a layout on the BrowserView.
>> > >> > 3. The main window gets a paint.
>> > >> > 4. Everything is good.
>> > >> >
>> > >> > With the legacy window:-
>> > >> > 1. When we hit undo on the infobar to reset the theme we get a
>> > >> > bunch of
>> > >> > bounds
>> > >> > changed notifications in RWHVA. It also forwards them to Legacy.
>> > >> > 2. The infobar is removed triggering a layout on the BrowserView.
>> > >
>> > >
>> > >> This should also trigger a SchedulePaint. I would also expect that
>> > >> any
>> > >> time we change the theme we explicitly tell the whole window to
>> > >> paint.
>> > >
>> > >
>> > >> > 3. The legacy window gets a paint. The main window does not.
>> > >> > 4. Tabstrip does not paint.
>> > >> >
>> > >> > The difference is in step 3. The layer does schedule a paint
>> > >> > correctly
>> > >> > in
>> > >> > both
>> > >> > cases. It appears that the main window paint is required for all
>> > >> > this
>
> to
>>
>> > >> > work
>> > >> > correctly.
>> > >> > We can trigger a main window paint in the legacy window if its
>> > >> > bounds
>> > >> > change. I
>> > >> > tested that and it works reliably. If this sounds reasonable I will
>> > >> > upload a
>> > >> > patch with that.
>> > >
>> > >
>> > >> We shouldn't assume every bounds change of the legacy window requires
>> > >> a paint. I think that'll lead to way too much painting.
>> > >
>> > >
>> > >>    -Scott
>> > >
>> > >
>> > >> >
>> > >> >
>> > >> >
>> > >> >
>> > >> >
>> > >> >
>> > >> > https://codereview.chromium.org/440793002/
>> > >
>> > >
>> > >> To unsubscribe from this group and stop receiving emails from it,
>> > >> send an
>> > >
>> > > email
>> > >>
>> > >> to mailto:chromium-reviews+unsubscribe@chromium.org.
>> > >
>> > >
>> > > Fair enough. Updated the patch to invalidate the parent when the
>> > > bounds
>> > > change
>> > > and after we receive
>> > > a theme changed notification. That is tracked via the
>> > > WM_DWMCOMPOSITIONCHANGED
>> > > message which HWNDMessageHandler
>> > > sends to its children during theme changes.
>> > >
>> > >
>> > > https://codereview.chromium.org/440793002/
>> >
>> > To unsubscribe from this group and stop receiving emails from it, send
>> > an
>> email
>> > to mailto:chromium-reviews+unsubscribe@chromium.org.
>
>
>> When the Undo infobar is clicked after the theme is applied, the following
>> events happen:-
>> 1. HWNDMessageHandler::FrameTypeChanged is called which changes the frame
>> from
>> opaque to glass.
>>     This in turn does the DWM magic, etc.
>> 2. FrameTypeChanged returns. Now the infobar is removed which triggers  a
>
> layout
>>
>> of the BrowserView.
>> 3. The bounds of the RWHVA are changed to occupy the additional space left
>> by
>> the infobar.
>> 4. The Legacy window paints the new area leaving the tabstrip in a bad
>> state
>> until the next paint.
>
>
>> I think a combination of the two is needed here. SchedulePaint in
>> HWNDMessageHandler::FrameTypeChanged and
>> an invalidation on the parent from the legacy window to ensure that it
>> works
>> correctly with and without the
>> legacy window.
>
>
> The parent window gets a paint without the legacy window due to the
> ResetWindowRgn calls in FrameTypeChanged.
> With the legacy window these calls don't seem to be generating a paint on
> the
> parent.
>
>
> https://codereview.chromium.org/440793002/

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

[ananta, 2014-08-06 00:28:15]

On 2014/08/06 00:21:58, sky wrote:
> It seems to me the code path that is triggering the theme to change
> should explicitly SChedulePaint. Is that not happening?
> 
> On Tue, Aug 5, 2014 at 4:28 PM,  <mailto:ananta@chromium.org> wrote:
> > On 2014/08/05 23:03:38, ananta wrote:
> >>
> >> On 2014/08/05 21:54:08, sky wrote:
> >> > This code should work with and without a legacy hwnd. And I still
> >> > don't think legacyhwnd should be posting to the parent for this. When
> >> > we change the theme we should explicitly invoke a SchedulePaint on the
> >> > widget.
> >> >
> >> > On Tue, Aug 5, 2014 at 10:58 AM,  <mailto:ananta@chromium.org> wrote:
> >> > > On 2014/08/05 17:36:41, sky wrote:
> >> > >
> >> > >> On Tue, Aug 5, 2014 at 8:12 AM,  <mailto:ananta@chromium.org> wrote:
> >> > >> > On 2014/08/05 01:30:56, sky wrote:
> >> > >> >
> >> > >> >
> >> > >
> >> > >
> >> > >
> >> >
> >
> >
> >
>
https://codereview.chromium.org/440793002/diff/1/ui/views/win/hwnd_message_ha...
> >>
> >> > >>
> >> > >> >>
> >> > >> >> File ui/views/win/hwnd_message_handler.cc (right):
> >> > >> >
> >> > >> >
> >> > >> >
> >> > >> >
> >> > >
> >> > >
> >> > >
> >> >
> >
> >
> >
>
https://codereview.chromium.org/440793002/diff/1/ui/views/win/hwnd_message_ha...
> >>
> >> > >>
> >> > >> >>
> >> > >> >> ui/views/win/hwnd_message_handler.cc:824:
> >> > >> >> base::TimeDelta::FromMilliseconds(500));
> >> > >> >> On 2014/08/05 00:24:03, ananta wrote:
> >> > >> >> > It now appears that the bug was never fixed. We need to merge
> >> > >> >> > this
> >
> > to
> >>
> >> > >> >> > M37.
> >> > >> >
> >> > >> >
> >> > >> >> This is a total hack. Why do we need to delay?
> >> > >> >> Also, I don't like posting to a potentially deleted window.
> >> > >> >
> >> > >> >
> >> > >> > I debugged this a bit more. As you had mentioned a while back, this
> >> > >> > has
> >> > >> > regressed due to the legacy window.
> >> > >> > The sequence of events is as below:-
> >> > >> >
> >> > >> > Without the legacy window:-
> >> > >> > 1. When we hit undo on the infobar to reset the theme we get a
> >> > >> > bunch of
> >> > >> > bounds
> >> > >> > changed notifications in RWHVA. It also forwards them to Legacy.
> >> > >> > 2. The infobar is removed triggering a layout on the BrowserView.
> >> > >> > 3. The main window gets a paint.
> >> > >> > 4. Everything is good.
> >> > >> >
> >> > >> > With the legacy window:-
> >> > >> > 1. When we hit undo on the infobar to reset the theme we get a
> >> > >> > bunch of
> >> > >> > bounds
> >> > >> > changed notifications in RWHVA. It also forwards them to Legacy.
> >> > >> > 2. The infobar is removed triggering a layout on the BrowserView.
> >> > >
> >> > >
> >> > >> This should also trigger a SchedulePaint. I would also expect that
> >> > >> any
> >> > >> time we change the theme we explicitly tell the whole window to
> >> > >> paint.
> >> > >
> >> > >
> >> > >> > 3. The legacy window gets a paint. The main window does not.
> >> > >> > 4. Tabstrip does not paint.
> >> > >> >
> >> > >> > The difference is in step 3. The layer does schedule a paint
> >> > >> > correctly
> >> > >> > in
> >> > >> > both
> >> > >> > cases. It appears that the main window paint is required for all
> >> > >> > this
> >
> > to
> >>
> >> > >> > work
> >> > >> > correctly.
> >> > >> > We can trigger a main window paint in the legacy window if its
> >> > >> > bounds
> >> > >> > change. I
> >> > >> > tested that and it works reliably. If this sounds reasonable I will
> >> > >> > upload a
> >> > >> > patch with that.
> >> > >
> >> > >
> >> > >> We shouldn't assume every bounds change of the legacy window requires
> >> > >> a paint. I think that'll lead to way too much painting.
> >> > >
> >> > >
> >> > >>    -Scott
> >> > >
> >> > >
> >> > >> >
> >> > >> >
> >> > >> >
> >> > >> >
> >> > >> >
> >> > >> >
> >> > >> > https://codereview.chromium.org/440793002/
> >> > >
> >> > >
> >> > >> To unsubscribe from this group and stop receiving emails from it,
> >> > >> send an
> >> > >
> >> > > email
> >> > >>
> >> > >> to mailto:chromium-reviews+unsubscribe@chromium.org.
> >> > >
> >> > >
> >> > > Fair enough. Updated the patch to invalidate the parent when the
> >> > > bounds
> >> > > change
> >> > > and after we receive
> >> > > a theme changed notification. That is tracked via the
> >> > > WM_DWMCOMPOSITIONCHANGED
> >> > > message which HWNDMessageHandler
> >> > > sends to its children during theme changes.
> >> > >
> >> > >
> >> > > https://codereview.chromium.org/440793002/
> >> >
> >> > To unsubscribe from this group and stop receiving emails from it, send
> >> > an
> >> email
> >> > to mailto:chromium-reviews+unsubscribe@chromium.org.
> >
> >
> >> When the Undo infobar is clicked after the theme is applied, the following
> >> events happen:-
> >> 1. HWNDMessageHandler::FrameTypeChanged is called which changes the frame
> >> from
> >> opaque to glass.
> >>     This in turn does the DWM magic, etc.
> >> 2. FrameTypeChanged returns. Now the infobar is removed which triggers  a
> >
> > layout
> >>
> >> of the BrowserView.
> >> 3. The bounds of the RWHVA are changed to occupy the additional space left
> >> by
> >> the infobar.
> >> 4. The Legacy window paints the new area leaving the tabstrip in a bad
> >> state
> >> until the next paint.
> >
> >
> >> I think a combination of the two is needed here. SchedulePaint in
> >> HWNDMessageHandler::FrameTypeChanged and
> >> an invalidation on the parent from the legacy window to ensure that it
> >> works
> >> correctly with and without the
> >> legacy window.
> >
> >
> > The parent window gets a paint without the legacy window due to the
> > ResetWindowRgn calls in FrameTypeChanged.
> > With the legacy window these calls don't seem to be generating a paint on
> > the
> > parent.
> >
> >
> > https://codereview.chromium.org/440793002/
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

Sorry for not being clear. The schedulepaint call in hwndmessagehandler is not
needed. It works correctly without that. The bug occurs because of the missing
paint in hwndmessagehandler. Will revert the call to schedulepaint. The old
invalidaterect call is not needed as well

[ananta, 2014-08-06 22:42:13]

Updated thepatch to add in the SWP_NOREDRAW flag to SetWindowPos used by the
Legacy class. I also removed the InvalidateRect call from
HWNDMessageHandler::FrameTypeChanged as it is not needed. The hide window show
window calls happening above do the trick

[sky, 2014-08-07 00:00:12]

LGTM

[ananta, 2014-08-07 00:01:39]

The CQ bit was checked by ananta@chromium.org

[commit-bot: I haz the power, 2014-08-07 00:06:40]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/ananta@chromium.org/440793002/140001

[commit-bot: I haz the power, 2014-08-07 07:38:26]

Change committed as 287994