Fixes DesktopWindowTreeHostMus::SetSize and IsVisible

Fixes DesktopWindowTreeHostMus::SetSize and IsVisible

Both had bugs in them.

BUG=660994
TEST=covered by tests
R=msw@chromium.org

Committed: https://crrev.com/679394d5e51f81b046e9fa9a07d8b19a898db329
Cr-Commit-Position: refs/heads/master@{#431196}

[sky, 2016-11-09 23:46:57]

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

[commit-bot: I haz the power, 2016-11-09 23:47:40]

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

[msw, 2016-11-09 23:57:55]

Sorry, this stuff isn't obvious to me.

https://codereview.chromium.org/2493653002/diff/1/ui/views/mus/desktop_window...
File ui/views/mus/desktop_window_tree_host_mus.cc (right):

https://codereview.chromium.org/2493653002/diff/1/ui/views/mus/desktop_window...
ui/views/mus/desktop_window_tree_host_mus.cc:125: if (parent_ &&
!parent_->native_widget_delegate_->AsWidget()->IsVisible())
Could we simply check !parent->IsVisible() instead? Will the parent's window
visibility match the widget visibility? If not, maybe explain why not?

https://codereview.chromium.org/2493653002/diff/1/ui/views/mus/desktop_window...
ui/views/mus/desktop_window_tree_host_mus.cc:131: gfx::Rect screen_bounds =
GetWindowBoundsInScreen();
Hmm, I don't really know why it makes sense to call GetWindowBoundsInScreen here
versus GetBounds (to parallel the SetBounds call below, that also happens to be
the current impl of GetWindowBoundsInScreen with the dips todo), or some other
bounds function, like the old window->bounds(). I can rubber stamp, but maybe
it's worth adding some explanatory comment about the importance of calling one
function over the (seemingly similar) others...

[sky, 2016-11-10 00:33:47]

No need to be sorry, this isn't terribly obvious code to begin with.

https://codereview.chromium.org/2493653002/diff/1/ui/views/mus/desktop_window...
File ui/views/mus/desktop_window_tree_host_mus.cc (right):

https://codereview.chromium.org/2493653002/diff/1/ui/views/mus/desktop_window...
ui/views/mus/desktop_window_tree_host_mus.cc:125: if (parent_ &&
!parent_->native_widget_delegate_->AsWidget()->IsVisible())
On 2016/11/09 23:57:55, msw wrote:
> Could we simply check !parent->IsVisible() instead? Will the parent's window
> visibility match the widget visibility? If not, maybe explain why not?

Added comment.

https://codereview.chromium.org/2493653002/diff/1/ui/views/mus/desktop_window...
ui/views/mus/desktop_window_tree_host_mus.cc:131: gfx::Rect screen_bounds =
GetWindowBoundsInScreen();
On 2016/11/09 23:57:55, msw wrote:
> Hmm, I don't really know why it makes sense to call GetWindowBoundsInScreen
here
> versus GetBounds (to parallel the SetBounds call below, that also happens to
be
> the current impl of GetWindowBoundsInScreen with the dips todo), or some other
> bounds function, like the old window->bounds(). I can rubber stamp, but maybe
> it's worth adding some explanatory comment about the importance of calling one
> function over the (seemingly similar) others...

This should be GetBounds(). Changed.

[msw, 2016-11-10 01:33:56]

lgtm with some minor lingering confusion.

https://codereview.chromium.org/2493653002/diff/20001/ui/views/mus/desktop_wi...
File ui/views/mus/desktop_window_tree_host_mus.cc (right):

https://codereview.chromium.org/2493653002/diff/20001/ui/views/mus/desktop_wi...
ui/views/mus/desktop_window_tree_host_mus.cc:125: // Go through the Widget api
for checking visibility as it has additional
Hmm, Widget::IsVisible() just returns |native_widget_->IsVisible();|, so should
this just call parent_->desktop_native_widget_aura_->IsVisible()? or do
something like:
bool DesktopWindowTreeHostMus::IsVisible() const {
  return (!parent_ || parent->Visible()) &&
         desktop_native_widget_aura_->IsVisible() &&
         window()->IsVisible();
}

[sky, 2016-11-10 05:44:46]

https://codereview.chromium.org/2493653002/diff/20001/ui/views/mus/desktop_wi...
File ui/views/mus/desktop_window_tree_host_mus.cc (right):

https://codereview.chromium.org/2493653002/diff/20001/ui/views/mus/desktop_wi...
ui/views/mus/desktop_window_tree_host_mus.cc:125: // Go through the Widget api
for checking visibility as it has additional
On 2016/11/10 01:33:56, msw wrote:
> Hmm, Widget::IsVisible() just returns |native_widget_->IsVisible();|, so
should
> this just call parent_->desktop_native_widget_aura_->IsVisible()? or do
> something like:
> bool DesktopWindowTreeHostMus::IsVisible() const {
>   return (!parent_ || parent->Visible()) &&
>          desktop_native_widget_aura_->IsVisible() &&
>          window()->IsVisible();
> }

Good call. fixed!

[sky, 2016-11-10 05:45:06]

The CQ bit was checked by sky@chromium.org

[sky, 2016-11-10 05:45:06]

The patchset sent to the CQ was uploaded after l-g-t-m from msw@chromium.org
Link to the patchset: https://codereview.chromium.org/2493653002/#ps40001
(title: "moar")

[commit-bot: I haz the power, 2016-11-10 05:45:15]

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-10 06:39:33]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-11-10 06:42:07]

Description was changed from

==========
Fixes DesktopWindowTreeHostMus::SetSize and IsVisible

Both had bugs in them.

BUG=660994
TEST=covered by tests
R=msw@chromium.org
==========

to

==========
Fixes DesktopWindowTreeHostMus::SetSize and IsVisible

Both had bugs in them.

BUG=660994
TEST=covered by tests
R=msw@chromium.org

Committed: https://crrev.com/679394d5e51f81b046e9fa9a07d8b19a898db329
Cr-Commit-Position: refs/heads/master@{#431196}
==========

[commit-bot: I haz the power, 2016-11-10 06:42:08]

Patchset 3 (id:??) landed as
https://crrev.com/679394d5e51f81b046e9fa9a07d8b19a898db329
Cr-Commit-Position: refs/heads/master@{#431196}