[views] Makes DesktopWindowTreeHostX11::FrameTypeChanged use PostTask

[views] Makes DesktopWindowTreeHostX11::FrameTypeChanged use PostTask

[views] Makes DesktopWindowTreeHostX11::FrameTypeChanged use PostTask
This avoids mutation of |View::children_| during iteration inside
View::PropagateNativeThemeChanged(). This was the reason for tests
failures that caused a revert of this CL:
https://codereview.chromium.org/2583343003/ . I am hoping to re-land
that CL once this is fixed.

BUG=NONE
TEST=Existing coverage

Committed: https://crrev.com/baad79f91de6ea9fd9ecc1b94980d71f2e7b92c1
Cr-Commit-Position: refs/heads/master@{#440454}

[varkha, 2016-12-21 19:53:17]

varkha@chromium.org changed reviewers:
+ sadrul@chromium.org

[varkha, 2016-12-21 19:53:17]

sadrul@, this is what we talked about offline. PTAL. I will add some test
coverage to the views to make sure we do not have similar scenarios elsewhere.

[varkha, 2016-12-21 20:06:44]

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

[varkha, 2016-12-21 20:06:58]

Patchset #1 (id:1) has been deleted

[commit-bot: I haz the power, 2016-12-21 20:07:38]

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, 2016-12-21 20:59:22]

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

[commit-bot: I haz the power, 2016-12-21 20:59:23]

Dry run: This issue passed the CQ dry run.

[sadrul, 2016-12-22 01:12:16]

I am sad that we actually recreate the frame-view from
NonClientView::UpdateFrame(). Can you please leave a TODO there to investigate
removing that (and instead expecting the frame-view to update itself when theme
changes, like all other views)?

lgtm with the TODO and the comment below addressed.

https://codereview.chromium.org/2597733002/diff/20001/ui/views/widget/desktop...
File ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc (right):

https://codereview.chromium.org/2597733002/diff/20001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:1034:
close_widget_factory_.GetWeakPtr(),
I think you want to use weak_factory_ here. |close_widget_factory_| is checked
to see if the widget is in the process of being destroyed. Using this factory
here can introduce flakiness in widget tear-down.

[varkha, 2016-12-22 01:22:52]

Yes, will do (once we are done with the move), thanks!

https://codereview.chromium.org/2597733002/diff/20001/ui/views/widget/desktop...
File ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc (right):

https://codereview.chromium.org/2597733002/diff/20001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:1034:
close_widget_factory_.GetWeakPtr(),
On 2016/12/22 01:12:16, sadrul wrote:
> I think you want to use weak_factory_ here. |close_widget_factory_| is checked
> to see if the widget is in the process of being destroyed. Using this factory
> here can introduce flakiness in widget tear-down.

Yeah, sorry, copy-paste only works most of the time ;)

[varkha, 2016-12-22 17:49:11]

https://codereview.chromium.org/2597733002/diff/20001/ui/views/widget/desktop...
File ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc (right):

https://codereview.chromium.org/2597733002/diff/20001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:1034:
close_widget_factory_.GetWeakPtr(),
On 2016/12/22 01:22:52, varkha wrote:
> On 2016/12/22 01:12:16, sadrul wrote:
> > I think you want to use weak_factory_ here. |close_widget_factory_| is
checked
> > to see if the widget is in the process of being destroyed. Using this
factory
> > here can introduce flakiness in widget tear-down.
> 
> Yeah, sorry, copy-paste only works most of the time ;)

Done.

[varkha, 2016-12-22 17:49:28]

The CQ bit was checked by varkha@chromium.org

[varkha, 2016-12-22 17:49:28]

The patchset sent to the CQ was uploaded after l-g-t-m from sadrul@chromium.org
Link to the patchset: https://codereview.chromium.org/2597733002/#ps40001
(title: "[views] Makes DesktopWindowTreeHostX11::FrameTypeChanged use PostTask
(nits)")

[commit-bot: I haz the power, 2016-12-22 17:49:44]

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, 2016-12-22 18:29:35]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1482428968280380,
"parent_rev": "7457094152a6f85307facad14ddd13f4dda67df2", "commit_rev":
"1caeb253ebe752bcb16c47b64fe7c7ba8b27b26d"}

[commit-bot: I haz the power, 2016-12-22 18:30:05]

Description was changed from

==========
[views] Makes DesktopWindowTreeHostX11::FrameTypeChanged use PostTask

[views] Makes DesktopWindowTreeHostX11::FrameTypeChanged use PostTask
This avoids mutation of |View::children_| during iteration inside
View::PropagateNativeThemeChanged(). This was the reason for tests
failures that caused a revert of this CL:
https://codereview.chromium.org/2583343003/ . I am hoping to re-land
that CL once this is fixed.

BUG=NONE
TEST=Existing coverage
==========

to

==========
[views] Makes DesktopWindowTreeHostX11::FrameTypeChanged use PostTask

[views] Makes DesktopWindowTreeHostX11::FrameTypeChanged use PostTask
This avoids mutation of |View::children_| during iteration inside
View::PropagateNativeThemeChanged(). This was the reason for tests
failures that caused a revert of this CL:
https://codereview.chromium.org/2583343003/ . I am hoping to re-land
that CL once this is fixed.

BUG=NONE
TEST=Existing coverage

Review-Url: https://codereview.chromium.org/2597733002
==========

[commit-bot: I haz the power, 2016-12-22 18:30:06]

Committed patchset #2 (id:40001)

[commit-bot: I haz the power, 2016-12-22 18:32:45]

Description was changed from

==========
[views] Makes DesktopWindowTreeHostX11::FrameTypeChanged use PostTask

[views] Makes DesktopWindowTreeHostX11::FrameTypeChanged use PostTask
This avoids mutation of |View::children_| during iteration inside
View::PropagateNativeThemeChanged(). This was the reason for tests
failures that caused a revert of this CL:
https://codereview.chromium.org/2583343003/ . I am hoping to re-land
that CL once this is fixed.

BUG=NONE
TEST=Existing coverage

Review-Url: https://codereview.chromium.org/2597733002
==========

to

==========
[views] Makes DesktopWindowTreeHostX11::FrameTypeChanged use PostTask

[views] Makes DesktopWindowTreeHostX11::FrameTypeChanged use PostTask
This avoids mutation of |View::children_| during iteration inside
View::PropagateNativeThemeChanged(). This was the reason for tests
failures that caused a revert of this CL:
https://codereview.chromium.org/2583343003/ . I am hoping to re-land
that CL once this is fixed.

BUG=NONE
TEST=Existing coverage

Committed: https://crrev.com/baad79f91de6ea9fd9ecc1b94980d71f2e7b92c1
Cr-Commit-Position: refs/heads/master@{#440454}
==========

[commit-bot: I haz the power, 2016-12-22 18:32:46]

Patchset 2 (id:??) landed as
https://crrev.com/baad79f91de6ea9fd9ecc1b94980d71f2e7b92c1
Cr-Commit-Position: refs/heads/master@{#440454}