Add child for parent_mus when params.parent_mus is not initialized.

Add child for parent_mus when params.parent_mus is not initialized.

BUG=662967
Committed: https://crrev.com/42aac8ce7e34d1d18932bacf417c276f2ad2cf17
Cr-Commit-Position: refs/heads/master@{#431018}

[mfomitchev, 2016-11-07 18:19:22]

mfomitchev@chromium.org changed reviewers:
+ mfomitchev@chromium.org

[mfomitchev, 2016-11-07 18:19:23]

https://codereview.chromium.org/2484813002/diff/1/ui/views/mus/native_widget_...
File ui/views/mus/native_widget_mus.cc (right):

https://codereview.chromium.org/2484813002/diff/1/ui/views/mus/native_widget_...
ui/views/mus/native_widget_mus.cc:778: if (params.parent_mus)
Can we make it so we only do AddChild once? Something like this:

ui::Window* parent_mus = params.parent_mus;

// Set-up transiency if appropriate.
if (params.parent && !params.child) {
  aura::Window* parent_root_aura = params.parent->GetRootWindow();
  ui::Window* parent_root_mus = parent_root_aura->GetProperty(kMusWindow);
  if (parent_root_mus) {
     parent_root_mus->AddTransientWindow(window_);
     if (!parent_mus)
       parent_mus = parent_root_mus;
  }
}

parent_mus->AddChild(window_);

[thanhph, 2016-11-07 19:50:56]

https://codereview.chromium.org/2484813002/diff/1/ui/views/mus/native_widget_...
File ui/views/mus/native_widget_mus.cc (right):

https://codereview.chromium.org/2484813002/diff/1/ui/views/mus/native_widget_...
ui/views/mus/native_widget_mus.cc:778: if (params.parent_mus)
On 2016/11/07 18:19:23, mfomitchev wrote:
> Can we make it so we only do AddChild once? Something like this:
> 
> ui::Window* parent_mus = params.parent_mus;
> 
> // Set-up transiency if appropriate.
> if (params.parent && !params.child) {
>   aura::Window* parent_root_aura = params.parent->GetRootWindow();
>   ui::Window* parent_root_mus = parent_root_aura->GetProperty(kMusWindow);
>   if (parent_root_mus) {
>      parent_root_mus->AddTransientWindow(window_);
>      if (!parent_mus)
>        parent_mus = parent_root_mus;
>   }
> }
> 
> parent_mus->AddChild(window_);

Thanks. I also add the if condition to prevent SEGV error. 

if (parent_mus)
    parent_mus->AddChild(window_);

Manual test work with this refactor.

[mfomitchev, 2016-11-07 20:16:47]

https://codereview.chromium.org/2484813002/diff/1/ui/views/mus/native_widget_...
File ui/views/mus/native_widget_mus.cc (right):

https://codereview.chromium.org/2484813002/diff/1/ui/views/mus/native_widget_...
ui/views/mus/native_widget_mus.cc:778: if (params.parent_mus)
On 2016/11/07 19:50:56, thanhph wrote:
> On 2016/11/07 18:19:23, mfomitchev wrote:
> > Can we make it so we only do AddChild once? Something like this:
> > 
> > ui::Window* parent_mus = params.parent_mus;
> > 
> > // Set-up transiency if appropriate.
> > if (params.parent && !params.child) {
> >   aura::Window* parent_root_aura = params.parent->GetRootWindow();
> >   ui::Window* parent_root_mus = parent_root_aura->GetProperty(kMusWindow);
> >   if (parent_root_mus) {
> >      parent_root_mus->AddTransientWindow(window_);
> >      if (!parent_mus)
> >        parent_mus = parent_root_mus;
> >   }
> > }
> > 
> > parent_mus->AddChild(window_);
> 
> Thanks. I also add the if condition to prevent SEGV error. 
> 
> if (parent_mus)
>     parent_mus->AddChild(window_);
> 
> Manual test work with this refactor.

Cool, thanks!

[thanhph, 2016-11-09 00:01:36]

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

[commit-bot: I haz the power, 2016-11-09 00:02:09]

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

[thanhph, 2016-11-09 15:37:12]

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

[thanhph, 2016-11-09 15:37:13]

sadrul@chromium.org: Please review changes in

[sadrul, 2016-11-09 17:10:20]

lgtm

[mfomitchev, 2016-11-09 17:44:20]

Some comments for the test. The main code change itself looks good.

https://codereview.chromium.org/2484813002/diff/120001/ui/views/mus/native_wi...
File ui/views/mus/native_widget_mus_unittest.cc (left):

https://codereview.chromium.org/2484813002/diff/120001/ui/views/mus/native_wi...
ui/views/mus/native_widget_mus_unittest.cc:5: #include
"ui/views/mus/native_widget_mus.h"
The unit test is for NativeWidgetMus, so this include should stay at the top.

https://codereview.chromium.org/2484813002/diff/120001/ui/views/mus/native_wi...
File ui/views/mus/native_widget_mus_unittest.cc (right):

https://codereview.chromium.org/2484813002/diff/120001/ui/views/mus/native_wi...
ui/views/mus/native_widget_mus_unittest.cc:19: #include
"ui/aura/client/aura_constants.h"
Is this used?

https://codereview.chromium.org/2484813002/diff/120001/ui/views/mus/native_wi...
ui/views/mus/native_widget_mus_unittest.cc:165: aura::Window* CreateTestWindow()
{
This isn't actually used it seems.

https://codereview.chromium.org/2484813002/diff/120001/ui/views/mus/native_wi...
ui/views/mus/native_widget_mus_unittest.cc:621: // This test is to ensure
corresponding parentship relation between widget and
This isn't quite accurate. Strictly speaking, widgets themselves don't have
parent/child relationship. There's parent/child relationship between
aura::Windows and ui::Windows, both of which are associated with Widgets. The
parentship relationship that we are ensuring here is not between Widgets, but
between ui::Windows corresponding to the Widgets. It is established based on the
parent/child relationship between aura::Windows, also associated with these
Widgets.

A description like this would be more accurate:

Ensure that when initializing a Widget with InitParams.parent set to another
widget's aura::Window, the ui::Window of the former Widget is added as a child
to the ui::Window of the latter Widget.

https://codereview.chromium.org/2484813002/diff/120001/ui/views/mus/native_wi...
ui/views/mus/native_widget_mus_unittest.cc:624: TEST_F(NativeWidgetMusTest,
WindowOfChildWidgetIsChildOfParentWindowWidget) {
Maybe just call the test something like InitNativeWidgetParentsUiWindow to make
it more parsable?

[thanhph, 2016-11-09 19:11:38]

https://codereview.chromium.org/2484813002/diff/120001/ui/views/mus/native_wi...
File ui/views/mus/native_widget_mus_unittest.cc (right):

https://codereview.chromium.org/2484813002/diff/120001/ui/views/mus/native_wi...
ui/views/mus/native_widget_mus_unittest.cc:19: #include
"ui/aura/client/aura_constants.h"
On 2016/11/09 17:44:20, mfomitchev wrote:
> Is this used?

I removed this include with the function CreateTestWindow below. Thanks!

https://codereview.chromium.org/2484813002/diff/120001/ui/views/mus/native_wi...
ui/views/mus/native_widget_mus_unittest.cc:165: aura::Window* CreateTestWindow()
{
On 2016/11/09 17:44:20, mfomitchev wrote:
> This isn't actually used it seems.

Acknowledged.

https://codereview.chromium.org/2484813002/diff/120001/ui/views/mus/native_wi...
ui/views/mus/native_widget_mus_unittest.cc:621: // This test is to ensure
corresponding parentship relation between widget and
On 2016/11/09 17:44:20, mfomitchev wrote:
> This isn't quite accurate. Strictly speaking, widgets themselves don't have
> parent/child relationship. There's parent/child relationship between
> aura::Windows and ui::Windows, both of which are associated with Widgets. The
> parentship relationship that we are ensuring here is not between Widgets, but
> between ui::Windows corresponding to the Widgets. It is established based on
the
> parent/child relationship between aura::Windows, also associated with these
> Widgets.
> 
> A description like this would be more accurate:
> 
> Ensure that when initializing a Widget with InitParams.parent set to another
> widget's aura::Window, the ui::Window of the former Widget is added as a child
> to the ui::Window of the latter Widget.

Acknowledged. Thanks for the clarification!

https://codereview.chromium.org/2484813002/diff/120001/ui/views/mus/native_wi...
ui/views/mus/native_widget_mus_unittest.cc:624: TEST_F(NativeWidgetMusTest,
WindowOfChildWidgetIsChildOfParentWindowWidget) {
On 2016/11/09 17:44:20, mfomitchev wrote:
> Maybe just call the test something like InitNativeWidgetParentsUiWindow to
make
> it more parsable?

Acknowledged.

[thanhph, 2016-11-09 19:12:09]

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

[commit-bot: I haz the power, 2016-11-09 19:12:53]

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

[mfomitchev, 2016-11-09 19:20:09]

LGTM
Pro tip: Use "Done" when you have addressed the comment. Use "Acknowledged" when
you haven't necessarily addressed the comment, but you do agree with it.


On 2016/11/09 19:11:38, thanhph wrote:
>
https://codereview.chromium.org/2484813002/diff/120001/ui/views/mus/native_wi...
> File ui/views/mus/native_widget_mus_unittest.cc (right):
> 
>
https://codereview.chromium.org/2484813002/diff/120001/ui/views/mus/native_wi...
> ui/views/mus/native_widget_mus_unittest.cc:19: #include
> "ui/aura/client/aura_constants.h"
> On 2016/11/09 17:44:20, mfomitchev wrote:
> > Is this used?
> 
> I removed this include with the function CreateTestWindow below. Thanks!
> 
>
https://codereview.chromium.org/2484813002/diff/120001/ui/views/mus/native_wi...
> ui/views/mus/native_widget_mus_unittest.cc:165: aura::Window*
CreateTestWindow()
> {
> On 2016/11/09 17:44:20, mfomitchev wrote:
> > This isn't actually used it seems.
> 
> Acknowledged.
> 
>
https://codereview.chromium.org/2484813002/diff/120001/ui/views/mus/native_wi...
> ui/views/mus/native_widget_mus_unittest.cc:621: // This test is to ensure
> corresponding parentship relation between widget and
> On 2016/11/09 17:44:20, mfomitchev wrote:
> > This isn't quite accurate. Strictly speaking, widgets themselves don't have
> > parent/child relationship. There's parent/child relationship between
> > aura::Windows and ui::Windows, both of which are associated with Widgets.
The
> > parentship relationship that we are ensuring here is not between Widgets,
but
> > between ui::Windows corresponding to the Widgets. It is established based on
> the
> > parent/child relationship between aura::Windows, also associated with these
> > Widgets.
> > 
> > A description like this would be more accurate:
> > 
> > Ensure that when initializing a Widget with InitParams.parent set to another
> > widget's aura::Window, the ui::Window of the former Widget is added as a
child
> > to the ui::Window of the latter Widget.
> 
> Acknowledged. Thanks for the clarification!
> 
>
https://codereview.chromium.org/2484813002/diff/120001/ui/views/mus/native_wi...
> ui/views/mus/native_widget_mus_unittest.cc:624: TEST_F(NativeWidgetMusTest,
> WindowOfChildWidgetIsChildOfParentWindowWidget) {
> On 2016/11/09 17:44:20, mfomitchev wrote:
> > Maybe just call the test something like InitNativeWidgetParentsUiWindow to
> make
> > it more parsable?
> 
> Acknowledged.

[thanhph, 2016-11-09 19:25:52]

On 2016/11/09 19:20:09, mfomitchev wrote:
> LGTM
> Pro tip: Use "Done" when you have addressed the comment. Use "Acknowledged"
when
> you haven't necessarily addressed the comment, but you do agree with it.

Gotcha, thanks!

> 
> 
> On 2016/11/09 19:11:38, thanhph wrote:
> >
>
https://codereview.chromium.org/2484813002/diff/120001/ui/views/mus/native_wi...
> > File ui/views/mus/native_widget_mus_unittest.cc (right):
> > 
> >
>
https://codereview.chromium.org/2484813002/diff/120001/ui/views/mus/native_wi...
> > ui/views/mus/native_widget_mus_unittest.cc:19: #include
> > "ui/aura/client/aura_constants.h"
> > On 2016/11/09 17:44:20, mfomitchev wrote:
> > > Is this used?
> > 
> > I removed this include with the function CreateTestWindow below. Thanks!
> > 
> >
>
https://codereview.chromium.org/2484813002/diff/120001/ui/views/mus/native_wi...
> > ui/views/mus/native_widget_mus_unittest.cc:165: aura::Window*
> CreateTestWindow()
> > {
> > On 2016/11/09 17:44:20, mfomitchev wrote:
> > > This isn't actually used it seems.
> > 
> > Acknowledged.
> > 
> >
>
https://codereview.chromium.org/2484813002/diff/120001/ui/views/mus/native_wi...
> > ui/views/mus/native_widget_mus_unittest.cc:621: // This test is to ensure
> > corresponding parentship relation between widget and
> > On 2016/11/09 17:44:20, mfomitchev wrote:
> > > This isn't quite accurate. Strictly speaking, widgets themselves don't
have
> > > parent/child relationship. There's parent/child relationship between
> > > aura::Windows and ui::Windows, both of which are associated with Widgets.
> The
> > > parentship relationship that we are ensuring here is not between Widgets,
> but
> > > between ui::Windows corresponding to the Widgets. It is established based
on
> > the
> > > parent/child relationship between aura::Windows, also associated with
these
> > > Widgets.
> > > 
> > > A description like this would be more accurate:
> > > 
> > > Ensure that when initializing a Widget with InitParams.parent set to
another
> > > widget's aura::Window, the ui::Window of the former Widget is added as a
> child
> > > to the ui::Window of the latter Widget.
> > 
> > Acknowledged. Thanks for the clarification!
> > 
> >
>
https://codereview.chromium.org/2484813002/diff/120001/ui/views/mus/native_wi...
> > ui/views/mus/native_widget_mus_unittest.cc:624: TEST_F(NativeWidgetMusTest,
> > WindowOfChildWidgetIsChildOfParentWindowWidget) {
> > On 2016/11/09 17:44:20, mfomitchev wrote:
> > > Maybe just call the test something like InitNativeWidgetParentsUiWindow to
> > make
> > > it more parsable?
> > 
> > Acknowledged.

[commit-bot: I haz the power, 2016-11-09 20:00:13]

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

[commit-bot: I haz the power, 2016-11-09 20:00:14]

Dry run: This issue passed the CQ dry run.

[thanhph, 2016-11-09 20:01:37]

The CQ bit was checked by thanhph@chromium.org

[thanhph, 2016-11-09 20:01:37]

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/2484813002/#ps140001
(title: "Clean up code and update test description.")

[commit-bot: I haz the power, 2016-11-09 20:02:07]

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-11-09 20:08:05]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-11-09 20:33:01]

Description was changed from

==========
Add child for parent_mus when params.parent_mus is not initialized.

BUG=662967
==========

to

==========
Add child for parent_mus when params.parent_mus is not initialized.

BUG=662967

Committed: https://crrev.com/42aac8ce7e34d1d18932bacf417c276f2ad2cf17
Cr-Commit-Position: refs/heads/master@{#431018}
==========

[commit-bot: I haz the power, 2016-11-09 20:33:02]

Patchset 8 (id:??) landed as
https://crrev.com/42aac8ce7e34d1d18932bacf417c276f2ad2cf17
Cr-Commit-Position: refs/heads/master@{#431018}