Remove unused code in DesktopScreenPositionClient

Remove unused code in DesktopScreenPositionClient. In the cases that it is called DesktopNativeWidgetAura::ForWindow() always returns NULL

BUG=None
TEST=None

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=272904

[pkotwicz, 2014-05-17 02:48:28]

Sadrul, can you please take a look?

I am split between removing this code and checking whether the aura::Window
belongs to a DesktopNativeWidgetAura in the PositionWindowInScreenCoordinates()
block

[sadrul, 2014-05-20 17:39:17]

https://codereview.chromium.org/292583003/diff/1/ui/views/widget/desktop_aura...
File ui/views/widget/desktop_aura/desktop_screen_position_client.cc (left):

https://codereview.chromium.org/292583003/diff/1/ui/views/widget/desktop_aura...
ui/views/widget/desktop_aura/desktop_screen_position_client.cc:94: if
(desktop_native_widget &&
Is this because DesktopNativeWidgetAura::ForWindow always returns NULL here? Can
you explain why?

[pkotwicz, 2014-05-20 19:11:13]

https://codereview.chromium.org/292583003/diff/1/ui/views/widget/desktop_aura...
File ui/views/widget/desktop_aura/desktop_screen_position_client.cc (left):

https://codereview.chromium.org/292583003/diff/1/ui/views/widget/desktop_aura...
ui/views/widget/desktop_aura/desktop_screen_position_client.cc:94: if
(desktop_native_widget &&
views::Widget::is_top_level() returns true if
    !params.child &&
    params.type != InitParams::TYPE_CONTROL &&
    params.type != InitParams::TYPE_TOOLTIP;

Thus, PositionWindowInScreenCoordinates() returns false if
    params.child || params.type == InitParams::TYPE_CONTROL
There should not be any DesktopNativeWidgetAuras in the cases that
PositionWindowInScreenCoordinates() returns false

[sadrul, 2014-05-20 20:03:10]

https://codereview.chromium.org/292583003/diff/1/ui/views/widget/desktop_aura...
File ui/views/widget/desktop_aura/desktop_screen_position_client.cc (left):

https://codereview.chromium.org/292583003/diff/1/ui/views/widget/desktop_aura...
ui/views/widget/desktop_aura/desktop_screen_position_client.cc:94: if
(desktop_native_widget &&
On 2014/05/20 19:11:13, pkotwicz wrote:
> views::Widget::is_top_level() returns true if
>     !params.child &&
>     params.type != InitParams::TYPE_CONTROL &&
>     params.type != InitParams::TYPE_TOOLTIP;
> 
> Thus, PositionWindowInScreenCoordinates() returns false if
>     params.child || params.type == InitParams::TYPE_CONTROL
> There should not be any DesktopNativeWidgetAuras in the cases that
> PositionWindowInScreenCoordinates() returns false

OK. Let's add DCHECK() for this, so we know to fix this if we start creating
DNWA for these cases.

[pkotwicz, 2014-05-20 21:27:25]

https://codereview.chromium.org/292583003/diff/1/ui/views/widget/desktop_aura...
File ui/views/widget/desktop_aura/desktop_screen_position_client.cc (left):

https://codereview.chromium.org/292583003/diff/1/ui/views/widget/desktop_aura...
ui/views/widget/desktop_aura/desktop_screen_position_client.cc:94: if
(desktop_native_widget &&
There is no point in a DCHECK. One could argue that when
PositionWindowInScreenCoordinates() returns true we should also call
root->GetHost()->SetBounds().

As the code stands right now (with or without this CL), one should not call
aura::Window::SetBoundsInScreen() on an aura::Window which has an associated
DNWA.

[sadrul, 2014-05-21 19:00:05]

https://codereview.chromium.org/292583003/diff/1/ui/views/widget/desktop_aura...
File ui/views/widget/desktop_aura/desktop_screen_position_client.cc (left):

https://codereview.chromium.org/292583003/diff/1/ui/views/widget/desktop_aura...
ui/views/widget/desktop_aura/desktop_screen_position_client.cc:94: if
(desktop_native_widget &&
On 2014/05/20 21:27:25, pkotwicz wrote:
> There is no point in a DCHECK. One could argue that when
> PositionWindowInScreenCoordinates() returns true we should also call
> root->GetHost()->SetBounds().
> 
> As the code stands right now (with or without this CL), one should not call
> aura::Window::SetBoundsInScreen() on an aura::Window which has an associated
> DNWA.

Right. Do we have code to check that that call is not mistakenly being called?

[pkotwicz, 2014-05-21 19:12:32]

The CQ bit was checked by pkotwicz@chromium.org

[pkotwicz, 2014-05-21 19:12:34]

The CQ bit was unchecked by pkotwicz@chromium.org

[pkotwicz, 2014-05-21 22:18:44]

Looking at DesktopNativeWidgetTopLevelHandler, it looks like calling
Widget::SetBounds() where appropriate is probably a better solution. I need to
look at this some more. (And will get back to this CL when I have time)

[pkotwicz, 2014-05-21 22:18:44]

Looking at DesktopNativeWidgetTopLevelHandler, it looks like calling
Widget::SetBounds() where appropriate is probably a better solution. I need to
look at this some more. (And will get back to this CL when I have time)

[pkotwicz, 2014-05-25 01:18:19]

Sadrul, can you please take another look?

I have confirmed that we do not call ParentWindowWithContext() on windows
associated with toplevel widgets. I have added a DCHECK() as requested.

[sadrul, 2014-05-26 14:09:25]

lgtm

[pkotwicz, 2014-05-26 14:25:52]

The CQ bit was checked by pkotwicz@chromium.org

[commit-bot: I haz the power, 2014-05-26 14:27:00]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/pkotwicz@chromium.org/292583003/40001

[commit-bot: I haz the power, 2014-05-26 18:09:22]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  linux_chromium_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
  win_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/12234)

[commit-bot: I haz the power, 2014-05-26 18:28:20]

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

[commit-bot: I haz the power, 2014-05-26 18:28:21]

Try jobs failed on following builders:
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)

[pkotwicz, 2014-05-26 18:58:15]

The CQ bit was checked by pkotwicz@chromium.org

[commit-bot: I haz the power, 2014-05-26 18:58:46]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/pkotwicz@chromium.org/292583003/60001

[commit-bot: I haz the power, 2014-05-26 20:40:24]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)

[commit-bot: I haz the power, 2014-05-26 22:02:59]

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

[commit-bot: I haz the power, 2014-05-26 22:03:00]

Try jobs failed on following builders:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)

[pkotwicz, 2014-05-26 23:28:40]

The CQ bit was checked by pkotwicz@chromium.org

[commit-bot: I haz the power, 2014-05-26 23:29:27]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/pkotwicz@chromium.org/292583003/60001

[commit-bot: I haz the power, 2014-05-27 00:40:21]

Change committed as 272904