Fix theme change paint issue on linux.

Fix theme change paint issue on linux.

We had added a recursion guard in the Widget::OnNativeThemeUpdated() function in this patch
https://codereview.chromium.org/2703933002. This was to work around a crash we were seeing
on Windows where views would be added/removed while the rootview was propagating the theme
changed notification to child views effectively making the iterators invalid.

The recursion killer check in Widget::FrameTypeChanged() causes a painting bug on Linux.

The callstack thanks to thomasanderson is here.

#2 0x7fdda6f06c83 views::Widget::FrameTypeChanged()
#3 0x55801d8ad66c BrowserView::UserChangedTheme()
#4 0x55801d8b0aef BrowserView::OnNativeThemeChanged()
#5 0x7fdda6edbdf6 views::View::PropagateNativeThemeChanged()
#6 0x7fdda6edbdc1 views::View::PropagateNativeThemeChanged()
#7 0x7fdda6edbdc1 views::View::PropagateNativeThemeChanged()
#8 0x7fdda6f09275 views::Widget::OnNativeThemeUpdated()
#9 0x7fdda6aa7493 ui::NativeTheme::NotifyObservers()
#10 0x7fdda4195366 libgtkui::GtkUi::ResetStyle()
#11 0x7fdda419532d libgtkui::OnThemeChanged()

The BrowserView::UserChangedTheme() function also has a recursion killer which is not set to
true here, which indicates that this stack is legitimate.

Fixes as below:
1. BrowserFrame now overrides the Widget::OnNativeThemeUpdated() function. After calling the base class
   we call the newly added function NativeThemeUpdated() in the BrowserView which ensures that we don't
   nuke the views while the iteration is going on in View::PropagateNativeThemeChanged()

2. In the HWNDMessageHandler::SetFullscreen method we update the fullscreen window map after we call PerformDwmTransition(). This internally at times causes a window pos changed to treat the now non fullscreen window as fullscreen leading to a DCHECK. Fixed by moving this code above.

I verified that this does not regress the previous theme fix http://crbug.com/681525 on Windows.

BUG=700135
Review-Url: https://codereview.chromium.org/2743793003
Cr-Commit-Position: refs/heads/master@{#456580}
Committed: https://chromium.googlesource.com/chromium/src/+/3d0d1cdfad3fd70d8d579dd03004b4e09d01f2f1

[ananta, 2017-03-10 01:42:36]

Description was changed from

==========
Fix theme change paint issue on linux.

We had added a recursion guard in the Widget::OnNativeThemeUpdated() function in
this patch
https://codereview.chromium.org/2703933002. This was to work around a crash we
were seeing
on Windows where views would be added/removed while the rootview was propagating
the theme
changed notification to child views effectively making the iterators invalid.

The recursion killer check in Widget::FrameTypeChanged() causes a painting bug
on Linux.

The callstack thanks to thomasanderson is here.

The BrowserView::UserChangedTheme() function also has a recursion killer which
is not set to
true here, which indicates that this stack is legitimate.

To ensure that we work correctly on all platforms, I added a call to
Widget::FrameTypeChanged() in
the views::Widget::OnNativeThemeUpdated() function after the
PropagateNativeThemeChanged() call.

I verified that this does not regress the previous theme fix
http://crbug.com/681525 on Windows.

BUG=700135
==========

to

==========
Fix theme change paint issue on linux.

We had added a recursion guard in the Widget::OnNativeThemeUpdated() function in
this patch
https://codereview.chromium.org/2703933002. This was to work around a crash we
were seeing
on Windows where views would be added/removed while the rootview was propagating
the theme
changed notification to child views effectively making the iterators invalid.

The recursion killer check in Widget::FrameTypeChanged() causes a painting bug
on Linux.

The callstack thanks to thomasanderson is here.

#2 0x7fdda6f06c83 views::Widget::FrameTypeChanged()
#3 0x55801d8ad66c BrowserView::UserChangedTheme()
#4 0x55801d8b0aef BrowserView::OnNativeThemeChanged()
#5 0x7fdda6edbdf6 views::View::PropagateNativeThemeChanged()
#6 0x7fdda6edbdc1 views::View::PropagateNativeThemeChanged()
#7 0x7fdda6edbdc1 views::View::PropagateNativeThemeChanged()
#8 0x7fdda6f09275 views::Widget::OnNativeThemeUpdated()
#9 0x7fdda6aa7493 ui::NativeTheme::NotifyObservers()
#10 0x7fdda4195366 libgtkui::GtkUi::ResetStyle()
#11 0x7fdda419532d libgtkui::OnThemeChanged()

The BrowserView::UserChangedTheme() function also has a recursion killer which
is not set to
true here, which indicates that this stack is legitimate.

To ensure that we work correctly on all platforms, I added a call to
Widget::FrameTypeChanged() in
the views::Widget::OnNativeThemeUpdated() function after the
PropagateNativeThemeChanged() call.

I verified that this does not regress the previous theme fix
http://crbug.com/681525 on Windows.

BUG=700135
==========

[ananta, 2017-03-10 01:42:54]

ananta@chromium.org changed reviewers:
+ sky@chromium.org

[ananta, 2017-03-10 01:43:28]

The CQ bit was checked by ananta@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-10 01:44:10]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Tom (Use chromium acct), 2017-03-10 02:08:02]

thomasanderson@google.com changed reviewers:
+ thomasanderson@google.com

[Tom (Use chromium acct), 2017-03-10 02:08:03]

lgtm

Thanks ananta@, this fixes the issue on Linux for me!

[ananta, 2017-03-10 04:36:54]

On 2017/03/10 02:08:03, Tom Anderson wrote:
> lgtm
> 
> Thanks ananta@, this fixes the issue on Linux for me!

Thanks Tom for testing it. However it has issues on Windows :(. sky, please hold
off on the review. We can discuss tomorrow.

[ananta, 2017-03-11 00:08:44]

Description was changed from

==========
Fix theme change paint issue on linux.

We had added a recursion guard in the Widget::OnNativeThemeUpdated() function in
this patch
https://codereview.chromium.org/2703933002. This was to work around a crash we
were seeing
on Windows where views would be added/removed while the rootview was propagating
the theme
changed notification to child views effectively making the iterators invalid.

The recursion killer check in Widget::FrameTypeChanged() causes a painting bug
on Linux.

The callstack thanks to thomasanderson is here.

#2 0x7fdda6f06c83 views::Widget::FrameTypeChanged()
#3 0x55801d8ad66c BrowserView::UserChangedTheme()
#4 0x55801d8b0aef BrowserView::OnNativeThemeChanged()
#5 0x7fdda6edbdf6 views::View::PropagateNativeThemeChanged()
#6 0x7fdda6edbdc1 views::View::PropagateNativeThemeChanged()
#7 0x7fdda6edbdc1 views::View::PropagateNativeThemeChanged()
#8 0x7fdda6f09275 views::Widget::OnNativeThemeUpdated()
#9 0x7fdda6aa7493 ui::NativeTheme::NotifyObservers()
#10 0x7fdda4195366 libgtkui::GtkUi::ResetStyle()
#11 0x7fdda419532d libgtkui::OnThemeChanged()

The BrowserView::UserChangedTheme() function also has a recursion killer which
is not set to
true here, which indicates that this stack is legitimate.

To ensure that we work correctly on all platforms, I added a call to
Widget::FrameTypeChanged() in
the views::Widget::OnNativeThemeUpdated() function after the
PropagateNativeThemeChanged() call.

I verified that this does not regress the previous theme fix
http://crbug.com/681525 on Windows.

BUG=700135
==========

to

==========
Fix theme change paint issue on linux.

We had added a recursion guard in the Widget::OnNativeThemeUpdated() function in
this patch
https://codereview.chromium.org/2703933002. This was to work around a crash we
were seeing
on Windows where views would be added/removed while the rootview was propagating
the theme
changed notification to child views effectively making the iterators invalid.

The recursion killer check in Widget::FrameTypeChanged() causes a painting bug
on Linux.

The callstack thanks to thomasanderson is here.

#2 0x7fdda6f06c83 views::Widget::FrameTypeChanged()
#3 0x55801d8ad66c BrowserView::UserChangedTheme()
#4 0x55801d8b0aef BrowserView::OnNativeThemeChanged()
#5 0x7fdda6edbdf6 views::View::PropagateNativeThemeChanged()
#6 0x7fdda6edbdc1 views::View::PropagateNativeThemeChanged()
#7 0x7fdda6edbdc1 views::View::PropagateNativeThemeChanged()
#8 0x7fdda6f09275 views::Widget::OnNativeThemeUpdated()
#9 0x7fdda6aa7493 ui::NativeTheme::NotifyObservers()
#10 0x7fdda4195366 libgtkui::GtkUi::ResetStyle()
#11 0x7fdda419532d libgtkui::OnThemeChanged()

The BrowserView::UserChangedTheme() function also has a recursion killer which
is not set to
true here, which indicates that this stack is legitimate.

Fixes as below:
1. We process the FrameTypeChanged() call in DesktopWindowTreeHostWin in a task.
Please see the newly added DelayFrameTypeChanged() function there.

2. Doing this in a background task appears to cause activation problems when we
perform a theme change on a fullscreen window when we come out 
of fullscreen. We workaround this by attempting to restore the foreground window
after a DWM transition.

3. In the HWNDMessageHandler::SetFullscreen method we update the fullscreen
window map after we call PerformDwmTransition(). This internally at times causes
a window pos changed to treat the now non fullscreen window as fullscreen
leading to a DCHECK. Fixed by moving this code above.

I verified that this does not regress the previous theme fix
http://crbug.com/681525 on Windows.

BUG=700135
==========

[ananta, 2017-03-11 00:10:09]

sky, This is ready for review now

[ananta, 2017-03-11 00:10:22]

The CQ bit was checked by ananta@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-11 00:11:02]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[sky, 2017-03-11 00:18:58]

The HWNDMessageHandler calls make me nervous and I would like to avoid them.
Additionally posting task and hoping things are better is always error prone. In
looking at this again how about the following.

Move BrowserView::OnNativeThemeChanged() override to
BrowserFrame::OnNativeThemeUpdated() override? This way BrowserView won't mutate
views during iteration.

[ananta, 2017-03-11 00:39:46]

Description was changed from

==========
Fix theme change paint issue on linux.

We had added a recursion guard in the Widget::OnNativeThemeUpdated() function in
this patch
https://codereview.chromium.org/2703933002. This was to work around a crash we
were seeing
on Windows where views would be added/removed while the rootview was propagating
the theme
changed notification to child views effectively making the iterators invalid.

The recursion killer check in Widget::FrameTypeChanged() causes a painting bug
on Linux.

The callstack thanks to thomasanderson is here.

#2 0x7fdda6f06c83 views::Widget::FrameTypeChanged()
#3 0x55801d8ad66c BrowserView::UserChangedTheme()
#4 0x55801d8b0aef BrowserView::OnNativeThemeChanged()
#5 0x7fdda6edbdf6 views::View::PropagateNativeThemeChanged()
#6 0x7fdda6edbdc1 views::View::PropagateNativeThemeChanged()
#7 0x7fdda6edbdc1 views::View::PropagateNativeThemeChanged()
#8 0x7fdda6f09275 views::Widget::OnNativeThemeUpdated()
#9 0x7fdda6aa7493 ui::NativeTheme::NotifyObservers()
#10 0x7fdda4195366 libgtkui::GtkUi::ResetStyle()
#11 0x7fdda419532d libgtkui::OnThemeChanged()

The BrowserView::UserChangedTheme() function also has a recursion killer which
is not set to
true here, which indicates that this stack is legitimate.

Fixes as below:
1. We process the FrameTypeChanged() call in DesktopWindowTreeHostWin in a task.
Please see the newly added DelayFrameTypeChanged() function there.

2. Doing this in a background task appears to cause activation problems when we
perform a theme change on a fullscreen window when we come out 
of fullscreen. We workaround this by attempting to restore the foreground window
after a DWM transition.

3. In the HWNDMessageHandler::SetFullscreen method we update the fullscreen
window map after we call PerformDwmTransition(). This internally at times causes
a window pos changed to treat the now non fullscreen window as fullscreen
leading to a DCHECK. Fixed by moving this code above.

I verified that this does not regress the previous theme fix
http://crbug.com/681525 on Windows.

BUG=700135
==========

to

==========
Fix theme change paint issue on linux.

We had added a recursion guard in the Widget::OnNativeThemeUpdated() function in
this patch
https://codereview.chromium.org/2703933002. This was to work around a crash we
were seeing
on Windows where views would be added/removed while the rootview was propagating
the theme
changed notification to child views effectively making the iterators invalid.

The recursion killer check in Widget::FrameTypeChanged() causes a painting bug
on Linux.

The callstack thanks to thomasanderson is here.

#2 0x7fdda6f06c83 views::Widget::FrameTypeChanged()
#3 0x55801d8ad66c BrowserView::UserChangedTheme()
#4 0x55801d8b0aef BrowserView::OnNativeThemeChanged()
#5 0x7fdda6edbdf6 views::View::PropagateNativeThemeChanged()
#6 0x7fdda6edbdc1 views::View::PropagateNativeThemeChanged()
#7 0x7fdda6edbdc1 views::View::PropagateNativeThemeChanged()
#8 0x7fdda6f09275 views::Widget::OnNativeThemeUpdated()
#9 0x7fdda6aa7493 ui::NativeTheme::NotifyObservers()
#10 0x7fdda4195366 libgtkui::GtkUi::ResetStyle()
#11 0x7fdda419532d libgtkui::OnThemeChanged()

The BrowserView::UserChangedTheme() function also has a recursion killer which
is not set to
true here, which indicates that this stack is legitimate.

Fixes as below:
1. BrowserFrame now overrides the Widget::OnNativeThemeUpdated() function. After
calling the base class
   we call the newly added function NativeThemeUpdated() in the BrowserView
which ensures that we don't
   nuke the views while the iteration is going on in
View::PropagateNativeThemeChanged()

2. In the HWNDMessageHandler::SetFullscreen method we update the fullscreen
window map after we call PerformDwmTransition(). This internally at times causes
a window pos changed to treat the now non fullscreen window as fullscreen
leading to a DCHECK. Fixed by moving this code above.

I verified that this does not regress the previous theme fix
http://crbug.com/681525 on Windows.

BUG=700135
==========

[ananta, 2017-03-11 00:44:22]

Patchset #6 (id:100001) has been deleted

[ananta, 2017-03-11 00:44:33]

Patchset #5 (id:80001) has been deleted

[ananta, 2017-03-11 00:46:29]

On 2017/03/11 00:18:58, sky wrote:
> The HWNDMessageHandler calls make me nervous and I would like to avoid them.
> Additionally posting task and hoping things are better is always error prone.
In
> looking at this again how about the following.
> 
> Move BrowserView::OnNativeThemeChanged() override to
> BrowserFrame::OnNativeThemeUpdated() override? This way BrowserView won't
mutate
> views during iteration.

Thanks. That works well. PTAL

[ananta, 2017-03-11 01:05:12]

The CQ bit was checked by ananta@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-11 01:05:33]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Tom (Use chromium acct), 2017-03-11 01:38:36]

latest PS still lgtm

[commit-bot: I haz the power, 2017-03-11 01:50:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-11 01:50:02]

Dry run: This issue passed the CQ dry run.

[sky, 2017-03-13 15:41:42]

https://codereview.chromium.org/2743793003/diff/120001/chrome/browser/ui/view...
File chrome/browser/ui/views/frame/browser_view.cc (right):

https://codereview.chromium.org/2743793003/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/frame/browser_view.cc:1659: void
BrowserView::NativeThemeUpdated(const ui::NativeTheme* theme) {
Please comment why NativeThemeUpdated is used rather than OnNativeThemeChanged.

https://codereview.chromium.org/2743793003/diff/120001/ui/views/win/hwnd_mess...
File ui/views/win/hwnd_message_handler.cc (right):

https://codereview.chromium.org/2743793003/diff/120001/ui/views/win/hwnd_mess...
ui/views/win/hwnd_message_handler.cc:842: // If we are out of fullscreen and
there was a pending DWM transition for the
Is this move needed? Why?

[ananta, 2017-03-13 19:48:03]

https://codereview.chromium.org/2743793003/diff/120001/chrome/browser/ui/view...
File chrome/browser/ui/views/frame/browser_view.cc (right):

https://codereview.chromium.org/2743793003/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/frame/browser_view.cc:1659: void
BrowserView::NativeThemeUpdated(const ui::NativeTheme* theme) {
On 2017/03/13 15:41:42, sky wrote:
> Please comment why NativeThemeUpdated is used rather than
OnNativeThemeChanged.

Done.

https://codereview.chromium.org/2743793003/diff/120001/ui/views/win/hwnd_mess...
File ui/views/win/hwnd_message_handler.cc (right):

https://codereview.chromium.org/2743793003/diff/120001/ui/views/win/hwnd_mess...
ui/views/win/hwnd_message_handler.cc:842: // If we are out of fullscreen and
there was a pending DWM transition for the
On 2017/03/13 15:41:42, sky wrote:
> Is this move needed? Why?

Was seeing a DCHECK at times in debug builds when the window comes out of
fullscreen and window pos changing thinks it is still a fullscreen window due to
the above map.

[ananta, 2017-03-13 19:48:34]

The CQ bit was checked by ananta@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-13 19:49:11]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-13 20:52:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-13 20:53:00]

Dry run: This issue passed the CQ dry run.

[sky, 2017-03-13 23:54:23]

Ok, LGTM

[ananta, 2017-03-14 01:22:49]

The CQ bit was checked by ananta@chromium.org

[ananta, 2017-03-14 01:22:50]

The patchset sent to the CQ was uploaded after l-g-t-m from
thomasanderson@google.com
Link to the patchset: https://codereview.chromium.org/2743793003/#ps140001
(title: "Add comments")

[commit-bot: I haz the power, 2017-03-14 01:23:36]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-14 02:22:10]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1489454569523020,
"parent_rev": "391e842f5feab036653813d0d441c1d504a971e8", "commit_rev":
"3d0d1cdfad3fd70d8d579dd03004b4e09d01f2f1"}

[commit-bot: I haz the power, 2017-03-14 02:22:55]

Description was changed from

==========
Fix theme change paint issue on linux.

We had added a recursion guard in the Widget::OnNativeThemeUpdated() function in
this patch
https://codereview.chromium.org/2703933002. This was to work around a crash we
were seeing
on Windows where views would be added/removed while the rootview was propagating
the theme
changed notification to child views effectively making the iterators invalid.

The recursion killer check in Widget::FrameTypeChanged() causes a painting bug
on Linux.

The callstack thanks to thomasanderson is here.

#2 0x7fdda6f06c83 views::Widget::FrameTypeChanged()
#3 0x55801d8ad66c BrowserView::UserChangedTheme()
#4 0x55801d8b0aef BrowserView::OnNativeThemeChanged()
#5 0x7fdda6edbdf6 views::View::PropagateNativeThemeChanged()
#6 0x7fdda6edbdc1 views::View::PropagateNativeThemeChanged()
#7 0x7fdda6edbdc1 views::View::PropagateNativeThemeChanged()
#8 0x7fdda6f09275 views::Widget::OnNativeThemeUpdated()
#9 0x7fdda6aa7493 ui::NativeTheme::NotifyObservers()
#10 0x7fdda4195366 libgtkui::GtkUi::ResetStyle()
#11 0x7fdda419532d libgtkui::OnThemeChanged()

The BrowserView::UserChangedTheme() function also has a recursion killer which
is not set to
true here, which indicates that this stack is legitimate.

Fixes as below:
1. BrowserFrame now overrides the Widget::OnNativeThemeUpdated() function. After
calling the base class
   we call the newly added function NativeThemeUpdated() in the BrowserView
which ensures that we don't
   nuke the views while the iteration is going on in
View::PropagateNativeThemeChanged()

2. In the HWNDMessageHandler::SetFullscreen method we update the fullscreen
window map after we call PerformDwmTransition(). This internally at times causes
a window pos changed to treat the now non fullscreen window as fullscreen
leading to a DCHECK. Fixed by moving this code above.

I verified that this does not regress the previous theme fix
http://crbug.com/681525 on Windows.

BUG=700135
==========

to

==========
Fix theme change paint issue on linux.

We had added a recursion guard in the Widget::OnNativeThemeUpdated() function in
this patch
https://codereview.chromium.org/2703933002. This was to work around a crash we
were seeing
on Windows where views would be added/removed while the rootview was propagating
the theme
changed notification to child views effectively making the iterators invalid.

The recursion killer check in Widget::FrameTypeChanged() causes a painting bug
on Linux.

The callstack thanks to thomasanderson is here.

#2 0x7fdda6f06c83 views::Widget::FrameTypeChanged()
#3 0x55801d8ad66c BrowserView::UserChangedTheme()
#4 0x55801d8b0aef BrowserView::OnNativeThemeChanged()
#5 0x7fdda6edbdf6 views::View::PropagateNativeThemeChanged()
#6 0x7fdda6edbdc1 views::View::PropagateNativeThemeChanged()
#7 0x7fdda6edbdc1 views::View::PropagateNativeThemeChanged()
#8 0x7fdda6f09275 views::Widget::OnNativeThemeUpdated()
#9 0x7fdda6aa7493 ui::NativeTheme::NotifyObservers()
#10 0x7fdda4195366 libgtkui::GtkUi::ResetStyle()
#11 0x7fdda419532d libgtkui::OnThemeChanged()

The BrowserView::UserChangedTheme() function also has a recursion killer which
is not set to
true here, which indicates that this stack is legitimate.

Fixes as below:
1. BrowserFrame now overrides the Widget::OnNativeThemeUpdated() function. After
calling the base class
   we call the newly added function NativeThemeUpdated() in the BrowserView
which ensures that we don't
   nuke the views while the iteration is going on in
View::PropagateNativeThemeChanged()

2. In the HWNDMessageHandler::SetFullscreen method we update the fullscreen
window map after we call PerformDwmTransition(). This internally at times causes
a window pos changed to treat the now non fullscreen window as fullscreen
leading to a DCHECK. Fixed by moving this code above.

I verified that this does not regress the previous theme fix
http://crbug.com/681525 on Windows.

BUG=700135

Review-Url: https://codereview.chromium.org/2743793003
Cr-Commit-Position: refs/heads/master@{#456580}
Committed:
https://chromium.googlesource.com/chromium/src/+/3d0d1cdfad3fd70d8d579dd03004...
==========

[commit-bot: I haz the power, 2017-03-14 02:22:59]

Committed patchset #6 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/3d0d1cdfad3fd70d8d579dd03004...