MacViews: Fix positioning of top-level Widgets with parents

MacViews: Fix positioning of top-level Widgets with parents

In doing so, consolidate calls to `SetBounds` for MacViews.

In toolkit-views, a Widget is considered "top-level" if it it doesn't
have the "child" flag set on InitParams. All widgets (both top-level and
not) can have their lifetime controlled by a "parent" window. Parented,
but "non-child", windows are positioned with coordinates relative to the
screen, not relative to the parent window that owns them.

For Aura, the logic that does this is in a wm::ScreenPositionClient.
MacViews is currently missing the logic in
DesktopScreenPositionClient::SetBounds(..). This CL adds the same logic
to BridgedNativeWidget::SetBounds(), and moves tests from
widget/desktop_aura/desktop_screen_position_client_unittest.cc to
widget/desktop_widget_unittest.cc since they are cross-platform.

The initial NSWindow bounds also needs to consider this logic, so bounds
setting for Widgets that specify InitParams::bounds changes to use
SetBounds(). A TODO that checked for a valid (according to OSX) window
size there is resolved by adding a DCHECK to
BridgedNativeWidget::SetBounds().

- Gets the following unit tests passing on MacViews:
DesktopScreenPositionClientTest.PositionDialog
DesktopScreenPositionClientTest.PositionControlWithNonRootParent

BUG=378134
Committed: https://crrev.com/ff4479f1e616d7fc5af1355201614a5b333bcd03
Cr-Commit-Position: refs/heads/master@{#317967}

[tapted, 2015-02-18 08:21:15]

Patchset #3 (id:40001) has been deleted

[tapted, 2015-02-20 09:06:13]

Patchset #5 (id:100001) has been deleted

[tapted, 2015-02-23 02:53:20]

tapted@chromium.org changed reviewers:
+ andresantoso@chromium.org

[tapted, 2015-02-23 07:42:34]

Hi Andre, WDYT of this for fixing the positioning?

(I nearly started calling these parented non-child windows "adolescent windows"
but i'm sure that would just confuse everyone :p)

[Andre, 2015-02-23 18:27:08]

https://codereview.chromium.org/927233003/diff/120001/ui/views/cocoa/bridged_...
File ui/views/cocoa/bridged_native_widget.mm (right):

https://codereview.chromium.org/927233003/diff/120001/ui/views/cocoa/bridged_...
ui/views/cocoa/bridged_native_widget.mm:46: if (type ==
views::Widget::InitParams::TYPE_POPUP)
The aura version of this checks for ui::wm::WINDOW_TYPE_POPUP which seems to
have a different meaning from InitParams::TYPE_POPUP.
GetAuraWindowTypeForWidgetType() maps several InitParams types to
WINDOW_TYPE_POPUP:

https://code.google.com/p/chromium/codesearch#chromium/src/ui/views/widget/wi...

[tapted, 2015-02-24 00:14:32]

https://codereview.chromium.org/927233003/diff/120001/ui/views/cocoa/bridged_...
File ui/views/cocoa/bridged_native_widget.mm (right):

https://codereview.chromium.org/927233003/diff/120001/ui/views/cocoa/bridged_...
ui/views/cocoa/bridged_native_widget.mm:46: if (type ==
views::Widget::InitParams::TYPE_POPUP)
On 2015/02/23 18:27:08, Andre wrote:
> The aura version of this checks for ui::wm::WINDOW_TYPE_POPUP which seems to
> have a different meaning from InitParams::TYPE_POPUP.
> GetAuraWindowTypeForWidgetType() maps several InitParams types to
> WINDOW_TYPE_POPUP:
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/ui/views/widget/wi...
>   

ooh - good catch! Fixed.

used GetAuraWindowTypeForWidgetType(..) directly.. it introduces a dependency on
an enum defined in Aura, but not code. I think that's marginally better than
duplicating the logic somewhere, and the long-term goal is aura, so I don't
think we need to move the enum unless there's an issue. Widget::InitParams
already does a similar thing with aura::WindowLayerType.

[Andre, 2015-02-24 00:23:22]

LGTM

[tapted, 2015-02-24 00:51:58]

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

[tapted, 2015-02-24 00:51:59]

+sky for ui/views OWNERS - please take a look

note this adds ui/views/widget/desktop_widget_unittest.cc  (moved from a
desktop_aura file).

I didn't rename the test harness class because the plan would be to move other
ChromeOS-only tests in widget_unittest.cc into desktop_widget_unittest.cc rather
than using #ifdefs, but see what you think.

Thanks!

[Andre, 2015-02-24 07:01:42]

Can we add a comment for SetBounds in widget.h, documenting the
expected coordinate space of |bounds|?

[sky, 2015-02-24 16:57:14]

LGTM

[tapted, 2015-02-25 01:50:07]

On 2015/02/24 07:01:42, Andre wrote:
> Can we add a comment for SetBounds in widget.h, documenting the
> expected coordinate space of |bounds|?

I've added a comment to Mac's BridgedNativeWidget - we have all the variables so
can be explicit there. I think Widget needs to remain vague, since it just
defers the call to the NativeWidget which can do whatever. That is, it depends
on the native implementation, as well as whatever ScreenPositionClient, and
OnBeforeWidgetInit might do.

[tapted, 2015-02-25 01:58:33]

The CQ bit was checked by tapted@chromium.org

[tapted, 2015-02-25 01:58:34]

The patchset sent to the CQ was uploaded after l-g-t-m from
andresantoso@chromium.org, sky@chromium.org
Link to the patchset: https://codereview.chromium.org/927233003/#ps160001
(title: "Added comment to BridgedNativeWidget")

[commit-bot: I haz the power, 2015-02-25 01:59:38]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/927233003/160001

[commit-bot: I haz the power, 2015-02-25 03:22:53]

Committed patchset #7 (id:160001)

[commit-bot: I haz the power, 2015-02-25 03:23:42]

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