Make the minimized home card not overlap activities on Athena

Make the minimized home card not overlap activities on Athena

BUG=402509
TEST=Manual, see bug
NOTRY=true

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

[pkotwicz, 2014-08-12 01:42:56]

Oshima, PTAL

This CL depends on https://codereview.chromium.org/463733003/

[oshima, 2014-08-12 17:27:41]

Could you please update the work area and use it instead?

[pkotwicz, 2014-08-13 13:48:55]

Oshima, can you please take another look?
This CL is somewhat hacky (i.e. I am modifying TestScreen but I think that it is
good enough for now)

https://codereview.chromium.org/465803002/diff/100001/athena/screen/public/sc...
File athena/screen/public/screen_manager.h (right):

https://codereview.chromium.org/465803002/diff/100001/athena/screen/public/sc...
athena/screen/public/screen_manager.h:62: virtual gfx::Rect GetWorkArea() = 0;
I think that having ScreenManager::GetWorkArea() instead of
ScreenManager::GetDisplay() is more consistent

https://codereview.chromium.org/465803002/diff/100001/extensions/shell/browse...
File extensions/shell/browser/shell_desktop_controller.h (right):

https://codereview.chromium.org/465803002/diff/100001/extensions/shell/browse...
extensions/shell/browser/shell_desktop_controller.h:90: // Sets the screen's
work area insets.
There is no app API for setting the work area bounds so
SetDisplayWorkAreaInsets() does not really belong here. However, I could not
think of something better (without needing a lot of new code)

[oshima, 2014-08-14 00:19:03]

thanks, lg. A few nits, plus asking for a test.

https://codereview.chromium.org/465803002/diff/100001/extensions/shell/browse...
File extensions/shell/browser/shell_desktop_controller.h (right):

https://codereview.chromium.org/465803002/diff/100001/extensions/shell/browse...
extensions/shell/browser/shell_desktop_controller.h:89: #if defined(OS_CHROMEOS)
do you need this ifdef?

I believe the CHROMEOS below are for ozone. This code should work fine on ozone.

https://codereview.chromium.org/465803002/diff/100001/extensions/shell/browse...
extensions/shell/browser/shell_desktop_controller.h:90: // Sets the screen's
work area insets.
On 2014/08/13 13:48:55, pkotwicz wrote:
> There is no app API for setting the work area bounds so
> SetDisplayWorkAreaInsets() does not really belong here. However, I could not
> think of something better (without needing a lot of new code)

I was fine with something like this,

static_cast<TestScreen>(gfx::Screen::GetNativeScreen())->SetWorkAreaInsets()

but your approach is better.

https://codereview.chromium.org/465803002/diff/120001/athena/screen/public/sc...
File athena/screen/public/screen_manager.h (right):

https://codereview.chromium.org/465803002/diff/120001/athena/screen/public/sc...
athena/screen/public/screen_manager.h:54: static ScreenManager*
Create(ScreenManagerDelegate* delegate,
Nice.

I wonder if root window parameter should also be in delegate.
maybe not...

https://codereview.chromium.org/465803002/diff/120001/athena/screen/public/sc...
athena/screen/public/screen_manager.h:62: virtual gfx::Rect GetWorkArea() = 0;
a client code should use gfx::Screen::GetPrimaryDisplay().work_area()

https://codereview.chromium.org/465803002/diff/120001/athena/test/athena_test...
File athena/test/athena_test_helper.cc (right):

https://codereview.chromium.org/465803002/diff/120001/athena/test/athena_test...
athena/test/athena_test_helper.cc:91: NULL,
Can you add impl and add test for this?

[pkotwicz, 2014-08-14 15:49:39]

Oshima, can you please take another look?

I believe I addressed all of your concerns

https://codereview.chromium.org/465803002/diff/100001/extensions/shell/browse...
File extensions/shell/browser/shell_desktop_controller.h (right):

https://codereview.chromium.org/465803002/diff/100001/extensions/shell/browse...
extensions/shell/browser/shell_desktop_controller.h:89: #if defined(OS_CHROMEOS)
Removed the ifdef. Once app shell is for more than just CrOS, we will want an
ifdef because the app setting the workarea only makes sense on CrOS.

https://codereview.chromium.org/465803002/diff/120001/athena/screen/public/sc...
File athena/screen/public/screen_manager.h (right):

https://codereview.chromium.org/465803002/diff/120001/athena/screen/public/sc...
athena/screen/public/screen_manager.h:54: static ScreenManager*
Create(ScreenManagerDelegate* delegate,
I do not think that the root window parameter should also be in the delegate. It
would make AthenaTestHelper more complex

[oshima, 2014-08-14 17:16:37]

lgtm

https://codereview.chromium.org/465803002/diff/120001/athena/screen/public/sc...
File athena/screen/public/screen_manager.h (right):

https://codereview.chromium.org/465803002/diff/120001/athena/screen/public/sc...
athena/screen/public/screen_manager.h:54: static ScreenManager*
Create(ScreenManagerDelegate* delegate,
On 2014/08/14 15:49:39, pkotwicz wrote:
> I do not think that the root window parameter should also be in the delegate.
It
> would make AthenaTestHelper more complex

I was thinking from multi display support point of view, where RootWindows are
dynamic, but this is fine for now.

[pkotwicz, 2014-08-14 18:30:18]

sadrul@ can you please look at the aura/test changes?
jamescook@ can you please look at the 	extensions/shell/browser changes?

[pkotwicz, 2014-08-14 18:30:18]

sadrul@ can you please look at the aura/test changes?
jamescook@ can you please look at the 	extensions/shell/browser changes?

[James Cook, 2014-08-14 20:35:02]

extensions/shell LGTM

[sadrul, 2014-08-14 21:25:20]

LGTM

https://codereview.chromium.org/465803002/diff/160001/athena/screen/screen_ma...
File athena/screen/screen_manager_impl.cc (right):

https://codereview.chromium.org/465803002/diff/160001/athena/screen/screen_ma...
athena/screen/screen_manager_impl.cc:192: explicit
ScreenManagerImpl(ScreenManagerDelegate* delegate,
Remove explicit

https://codereview.chromium.org/465803002/diff/160001/athena/test/test_screen...
File athena/test/test_screen_manager_delegate.h (right):

https://codereview.chromium.org/465803002/diff/160001/athena/test/test_screen...
athena/test/test_screen_manager_delegate.h:27: // Now owned.
*Not

or just remove the comment

[pkotwicz, 2014-08-20 21:25:39]

The CQ bit was checked by pkotwicz@chromium.org

[commit-bot: I haz the power, 2014-08-20 21:28:28]

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

[commit-bot: I haz the power, 2014-08-20 23:15:29]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  android_clang_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
  linux_chromium_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[commit-bot: I haz the power, 2014-08-20 23:20:08]

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

[commit-bot: I haz the power, 2014-08-20 23:20:09]

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

[pkotwicz, 2014-08-21 00:02:15]

The CQ bit was checked by pkotwicz@chromium.org

[commit-bot: I haz the power, 2014-08-21 00:04:08]

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

[commit-bot: I haz the power, 2014-08-21 00:10:11]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  android_clang_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)

[commit-bot: I haz the power, 2014-08-21 00:12:58]

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

[commit-bot: I haz the power, 2014-08-21 00:13:00]

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

[pkotwicz, 2014-08-21 00:56:35]

The CQ bit was checked by pkotwicz@chromium.org

[commit-bot: I haz the power, 2014-08-21 00:57:47]

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

[commit-bot: I haz the power, 2014-08-21 01:01:49]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  android_clang_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)

[commit-bot: I haz the power, 2014-08-21 01:04:26]

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

[commit-bot: I haz the power, 2014-08-21 01:04:27]

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

[pkotwicz, 2014-08-21 14:13:35]

The CQ bit was checked by pkotwicz@chromium.org

[commit-bot: I haz the power, 2014-08-21 14:14:35]

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

[pkotwicz, 2014-08-21 15:03:45]

The CQ bit was unchecked by pkotwicz@chromium.org

[pkotwicz, 2014-08-21 15:04:00]

The CQ bit was checked by pkotwicz@chromium.org

[commit-bot: I haz the power, 2014-08-21 15:05:26]

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

[commit-bot: I haz the power, 2014-08-21 15:09:17]

Committed patchset #5 (200001) as 291072