Adding a gray semi transparent backdrop behind the topmost window within the default container

Adding a gray semi transparent backdrop behind the topmost window within the default container

This is part of the "always maximized" feature. Windows which cannot be maximized - or cannot be made to cover the entire screen should have a backdrop behind them which covers the desktop.

Tried various ways to implement this and this seems to be the best solution.

BUG=337567, 337563
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=255863

[Mr4D (OOO till 08-26), 2014-02-21 23:26:40]

Sky, as discussed. Here is the first 'draft' of the CL. It works from all I was
able to test. I am starting now on the unit test in parallel, but if you see
general problems - please let me know!

Thanks!

[Mr4D (OOO till 08-26), 2014-02-21 23:26:40]

Sky, as discussed. Here is the first 'draft' of the CL. It works from all I was
able to test. I am starting now on the unit test in parallel, but if you see
general problems - please let me know!

Thanks!

[sky, 2014-02-21 23:46:06]

Remind me why this is better, and less error prone, than creating a new
container?

[sky, 2014-02-21 23:48:34]

Also, it would be good if you could writeup a design doc for the feature.

[Mr4D (OOO till 08-26), 2014-02-22 02:26:51]

I created it as you have suggested yourself. Sure, I'll create a design doc.
Let's discuss on Monday!

[sky, 2014-03-06 01:03:17]

https://codereview.chromium.org/169643005/diff/80001/ash/wm/maximize_mode/max...
File ash/wm/maximize_mode/maximize_mode_window_manager.h (right):

https://codereview.chromium.org/169643005/diff/80001/ash/wm/maximize_mode/max...
ash/wm/maximize_mode/maximize_mode_window_manager.h:42: void HideBackdrops(bool
hide);
Do you really need the 'hide' variable? If you do, name it is_temporary to make
it clear. And document what 'temporary' means here. I don't see the
implementation of these either.

https://codereview.chromium.org/169643005/diff/80001/ash/wm/overview/window_o...
File ash/wm/overview/window_overview.cc (right):

https://codereview.chromium.org/169643005/diff/80001/ash/wm/overview/window_o...
ash/wm/overview/window_overview.cc:172: shell->OnOverviewModeEnded();
How about moving this to its own patch?

https://codereview.chromium.org/169643005/diff/80001/ash/wm/workspace/workspa...
File ash/wm/workspace/workspace_layout_manager.cc (right):

https://codereview.chromium.org/169643005/diff/80001/ash/wm/workspace/workspa...
ash/wm/workspace/workspace_layout_manager.cc:64: if ((NULL != backdrop_.get())
== enable)
nit: creating a local variable for NULL != backdrop_.get() is a lot more
readable.

https://codereview.chromium.org/169643005/diff/80001/ash/wm/workspace/workspa...
ash/wm/workspace/workspace_layout_manager.cc:83:
UpdateStackingAndVisibilityOfBackdrop();
How about a delegate method that is called at all these places? That way
WorskpaceLayoutManager does't have to own the backdrop_ or know anything about
it.

[Mr4D (OOO till 08-26), 2014-03-06 17:17:16]

Addressed! Please have another look!

https://codereview.chromium.org/169643005/diff/80001/ash/wm/maximize_mode/max...
File ash/wm/maximize_mode/maximize_mode_window_manager.h (right):

https://codereview.chromium.org/169643005/diff/80001/ash/wm/maximize_mode/max...
ash/wm/maximize_mode/maximize_mode_window_manager.h:42: void HideBackdrops(bool
hide);
Removed.

https://codereview.chromium.org/169643005/diff/80001/ash/wm/overview/window_o...
File ash/wm/overview/window_overview.cc (right):

https://codereview.chromium.org/169643005/diff/80001/ash/wm/overview/window_o...
ash/wm/overview/window_overview.cc:172: shell->OnOverviewModeEnded();
It is already. Please look at the latest rev of the patch from yesterday
morning. :)

https://codereview.chromium.org/169643005/diff/80001/ash/wm/workspace/workspa...
File ash/wm/workspace/workspace_layout_manager.cc (right):

https://codereview.chromium.org/169643005/diff/80001/ash/wm/workspace/workspa...
ash/wm/workspace/workspace_layout_manager.cc:64: if ((NULL != backdrop_.get())
== enable)
On 2014/03/06 01:03:18, sky wrote:
> nit: creating a local variable for NULL != backdrop_.get() is a lot more
> readable.

Done.

https://codereview.chromium.org/169643005/diff/80001/ash/wm/workspace/workspa...
ash/wm/workspace/workspace_layout_manager.cc:83:
UpdateStackingAndVisibilityOfBackdrop();
On 2014/03/06 01:03:18, sky wrote:
> How about a delegate method that is called at all these places? That way
> WorskpaceLayoutManager does't have to own the backdrop_ or know anything about
> it.

Done.

[sky, 2014-03-06 21:34:12]

https://codereview.chromium.org/169643005/diff/180001/ash/wm/maximize_mode/wo...
File ash/wm/maximize_mode/workspace_backdrop_delegate.cc (right):

https://codereview.chromium.org/169643005/diff/180001/ash/wm/maximize_mode/wo...
ash/wm/maximize_mode/workspace_backdrop_delegate.cc:30: background_ = new
views::Widget;
Is there a reason why you don't use a layer directly?

https://codereview.chromium.org/169643005/diff/180001/ash/wm/maximize_mode/wo...
ash/wm/maximize_mode/workspace_backdrop_delegate.cc:40:
contents_view->set_background(
I'm skeptical you need a widget here, but if you do can't you accomplish this
with a colored layer?

https://codereview.chromium.org/169643005/diff/180001/ash/wm/maximize_mode/wo...
ash/wm/maximize_mode/workspace_backdrop_delegate.cc:66: for
(aura::Window::Windows::const_reverse_iterator window_iter =
nit: I think this would be easier to follow if you had a function that returned
the window to stack behind and then the restacking logic.

https://codereview.chromium.org/169643005/diff/180001/ash/wm/maximize_mode/wo...
ash/wm/maximize_mode/workspace_backdrop_delegate.cc:71: window->type() ==
ui::wm::WINDOW_TYPE_NORMAL) {
If window has a transient parent in the same container should the backdrop be
behind the transient parent?

https://codereview.chromium.org/169643005/diff/180001/ash/wm/maximize_mode/wo...
ash/wm/maximize_mode/workspace_backdrop_delegate.cc:80: inside_ = true;
Use autoreset. Please rename inside_ to better match what's happening. Maybe
in_restacking_ or something.

https://codereview.chromium.org/169643005/diff/180001/ash/wm/workspace/worksp...
File ash/wm/workspace/workspace_layout_manager.h (right):

https://codereview.chromium.org/169643005/diff/180001/ash/wm/workspace/worksp...
ash/wm/workspace/workspace_layout_manager.h:40: class
WorkspaceLayoutManagerBackdrop;
You shouldn't need this.

https://codereview.chromium.org/169643005/diff/180001/ash/wm/workspace/worksp...
ash/wm/workspace/workspace_layout_manager.h:44: class MaximizeBackdropDelegate {
Move to own header and name WorkspaceLayoutManagerDelegate. Also, make methods
match where you're calling from, eg OnWindowStackingChanged. You can make all
the implementations funnel into a single function.

https://codereview.chromium.org/169643005/diff/180001/ash/wm/workspace/worksp...
ash/wm/workspace/workspace_layout_manager.h:69: void
SetMaximizeBackdropDelegate(MaximizeBackdropDelegate* delegate);
Make take scoped_ptr.

https://codereview.chromium.org/169643005/diff/180001/ash/wm/workspace/worksp...
File ash/wm/workspace/workspace_layout_manager_unittest.cc (right):

https://codereview.chromium.org/169643005/diff/180001/ash/wm/workspace/worksp...
ash/wm/workspace/workspace_layout_manager_unittest.cc:829: for (int i =
default_container()->children().size() - 1;
Does this overflow if empty?

[Mr4D (OOO till 08-26), 2014-03-07 01:00:12]

Please have another look!

https://codereview.chromium.org/169643005/diff/180001/ash/wm/maximize_mode/wo...
File ash/wm/maximize_mode/workspace_backdrop_delegate.cc (right):

https://codereview.chromium.org/169643005/diff/180001/ash/wm/maximize_mode/wo...
ash/wm/maximize_mode/workspace_backdrop_delegate.cc:30: background_ = new
views::Widget;
Yes, swallowing events.

https://codereview.chromium.org/169643005/diff/180001/ash/wm/maximize_mode/wo...
ash/wm/maximize_mode/workspace_backdrop_delegate.cc:40:
contents_view->set_background(
Done.

https://codereview.chromium.org/169643005/diff/180001/ash/wm/maximize_mode/wo...
ash/wm/maximize_mode/workspace_backdrop_delegate.cc:66: for
(aura::Window::Windows::const_reverse_iterator window_iter =
On 2014/03/06 21:34:13, sky wrote:
> nit: I think this would be easier to follow if you had a function that
returned
> the window to stack behind and then the restacking logic.

Done.

https://codereview.chromium.org/169643005/diff/180001/ash/wm/maximize_mode/wo...
ash/wm/maximize_mode/workspace_backdrop_delegate.cc:71: window->type() ==
ui::wm::WINDOW_TYPE_NORMAL) {
As long as the transient window can get Alt+Tabbed not.

https://codereview.chromium.org/169643005/diff/180001/ash/wm/maximize_mode/wo...
ash/wm/maximize_mode/workspace_backdrop_delegate.cc:80: inside_ = true;
On 2014/03/06 21:34:13, sky wrote:
> Use autoreset. Please rename inside_ to better match what's happening. Maybe
> in_restacking_ or something.

Done.

https://codereview.chromium.org/169643005/diff/180001/ash/wm/workspace/worksp...
File ash/wm/workspace/workspace_layout_manager.h (right):

https://codereview.chromium.org/169643005/diff/180001/ash/wm/workspace/worksp...
ash/wm/workspace/workspace_layout_manager.h:40: class
WorkspaceLayoutManagerBackdrop;
On 2014/03/06 21:34:13, sky wrote:
> You shouldn't need this.

Done.

https://codereview.chromium.org/169643005/diff/180001/ash/wm/workspace/worksp...
ash/wm/workspace/workspace_layout_manager.h:44: class MaximizeBackdropDelegate {
On 2014/03/06 21:34:13, sky wrote:
> Move to own header and name WorkspaceLayoutManagerDelegate. Also, make methods
> match where you're calling from, eg OnWindowStackingChanged. You can make all
> the implementations funnel into a single function.

Done.

https://codereview.chromium.org/169643005/diff/180001/ash/wm/workspace/worksp...
ash/wm/workspace/workspace_layout_manager.h:69: void
SetMaximizeBackdropDelegate(MaximizeBackdropDelegate* delegate);
On 2014/03/06 21:34:13, sky wrote:
> Make take scoped_ptr.

Done.

https://codereview.chromium.org/169643005/diff/180001/ash/wm/workspace/worksp...
File ash/wm/workspace/workspace_layout_manager_unittest.cc (right):

https://codereview.chromium.org/169643005/diff/180001/ash/wm/workspace/worksp...
ash/wm/workspace/workspace_layout_manager_unittest.cc:829: for (int i =
default_container()->children().size() - 1;
for (;false;) { do somehting; } should never execute.

[sky, 2014-03-07 15:00:10]

Did you forget to git add worskpace_layout_manager_delegate?

https://codereview.chromium.org/169643005/diff/180001/ash/wm/workspace/worksp...
File ash/wm/workspace/workspace_layout_manager_unittest.cc (right):

https://codereview.chromium.org/169643005/diff/180001/ash/wm/workspace/worksp...
ash/wm/workspace/workspace_layout_manager_unittest.cc:829: for (int i =
default_container()->children().size() - 1;
On 2014/03/07 01:00:12, Mr4D wrote:
> for (;false;) { do somehting; } should never execute.

I'm not sure which of these this code is interpreted as:

static_cast<int>(default_container()->children().size() -  1)
or
static_cast<int>(default_container()->children().size()) - 1

if the former, it'll overflow on empty. How about explicitly making it the
second so we don't have to worry.

Or go with a reverse iterator which doesn't have this problem.

https://codereview.chromium.org/169643005/diff/200001/ash/wm/maximize_mode/wo...
File ash/wm/maximize_mode/workspace_backdrop_delegate.cc (right):

https://codereview.chromium.org/169643005/diff/200001/ash/wm/maximize_mode/wo...
ash/wm/maximize_mode/workspace_backdrop_delegate.cc:32:
views::Widget::InitParams params(views::Widget::InitParams::TYPE_CONTROL);
Seems weird that you're using TYPE_CONTROL here. I think TYPE_WINDOW_FRAMELESS
would make more sense.

https://codereview.chromium.org/169643005/diff/200001/ash/wm/maximize_mode/wo...
ash/wm/maximize_mode/workspace_backdrop_delegate.cc:58: const gfx::Rect&
old_bounds,
nit: spacing

https://codereview.chromium.org/169643005/diff/200001/ash/wm/maximize_mode/wo...
ash/wm/maximize_mode/workspace_backdrop_delegate.cc:96: if
(background_->IsVisible())
nit: you shouldn't need the IsVisible() check, just hide.

https://codereview.chromium.org/169643005/diff/200001/ash/wm/maximize_mode/wo...
ash/wm/maximize_mode/workspace_backdrop_delegate.cc:121: if (window->IsVisible()
&& window->type() == ui::wm::WINDOW_TYPE_NORMAL)
How come you stopped checking for active?

https://codereview.chromium.org/169643005/diff/200001/ash/wm/maximize_mode/wo...
ash/wm/maximize_mode/workspace_backdrop_delegate.cc:131: // This needs to be
instantly.
instant.

https://codereview.chromium.org/169643005/diff/200001/ash/wm/maximize_mode/wo...
ash/wm/maximize_mode/workspace_backdrop_delegate.cc:146: background_->Show();
Since you have show on 143, is this necessary?

[Mr4D (OOO till 08-26), 2014-03-07 15:55:34]

Addressed! Please have another look!

https://codereview.chromium.org/169643005/diff/180001/ash/wm/workspace/worksp...
File ash/wm/workspace/workspace_layout_manager_unittest.cc (right):

https://codereview.chromium.org/169643005/diff/180001/ash/wm/workspace/worksp...
ash/wm/workspace/workspace_layout_manager_unittest.cc:829: for (int i =
default_container()->children().size() - 1;
size_t. Okay. Done.

https://codereview.chromium.org/169643005/diff/200001/ash/wm/maximize_mode/wo...
File ash/wm/maximize_mode/workspace_backdrop_delegate.cc (right):

https://codereview.chromium.org/169643005/diff/200001/ash/wm/maximize_mode/wo...
ash/wm/maximize_mode/workspace_backdrop_delegate.cc:32:
views::Widget::InitParams params(views::Widget::InitParams::TYPE_CONTROL);
On 2014/03/07 15:00:10, sky wrote:
> Seems weird that you're using TYPE_CONTROL here. I think TYPE_WINDOW_FRAMELESS
> would make more sense.

Done.

https://codereview.chromium.org/169643005/diff/200001/ash/wm/maximize_mode/wo...
ash/wm/maximize_mode/workspace_backdrop_delegate.cc:58: const gfx::Rect&
old_bounds,
On 2014/03/07 15:00:10, sky wrote:
> nit: spacing

Done.

https://codereview.chromium.org/169643005/diff/200001/ash/wm/maximize_mode/wo...
ash/wm/maximize_mode/workspace_backdrop_delegate.cc:96: if
(background_->IsVisible())
On 2014/03/07 15:00:10, sky wrote:
> nit: you shouldn't need the IsVisible() check, just hide.

Done.

https://codereview.chromium.org/169643005/diff/200001/ash/wm/maximize_mode/wo...
ash/wm/maximize_mode/workspace_backdrop_delegate.cc:121: if (window->IsVisible()
&& window->type() == ui::wm::WINDOW_TYPE_NORMAL)
Too many implementations of the same thing. Done.

https://codereview.chromium.org/169643005/diff/200001/ash/wm/maximize_mode/wo...
ash/wm/maximize_mode/workspace_backdrop_delegate.cc:131: // This needs to be
instantly.
On 2014/03/07 15:00:10, sky wrote:
> instant.

Done.

https://codereview.chromium.org/169643005/diff/200001/ash/wm/maximize_mode/wo...
ash/wm/maximize_mode/workspace_backdrop_delegate.cc:146: background_->Show();
On 2014/03/07 15:00:10, sky wrote:
> Since you have show on 143, is this necessary?

Done.

[sky, 2014-03-07 17:00:45]

LGTM

https://codereview.chromium.org/169643005/diff/220001/ash/wm/workspace/worksp...
File ash/wm/workspace/workspace_layout_manager_delegate.h (right):

https://codereview.chromium.org/169643005/diff/220001/ash/wm/workspace/worksp...
ash/wm/workspace/workspace_layout_manager_delegate.h:1: // Copyright (c) 2012
The Chromium Authors. All rights reserved.
2014

[Mr4D (OOO till 08-26), 2014-03-07 17:39:56]

Thanks!

https://codereview.chromium.org/169643005/diff/220001/ash/wm/workspace/worksp...
File ash/wm/workspace/workspace_layout_manager_delegate.h (right):

https://codereview.chromium.org/169643005/diff/220001/ash/wm/workspace/worksp...
ash/wm/workspace/workspace_layout_manager_delegate.h:1: // Copyright (c) 2012
The Chromium Authors. All rights reserved.
On 2014/03/07 17:00:46, sky wrote:
> 2014

Done.

[Mr4D (OOO till 08-26), 2014-03-07 17:40:03]

The CQ bit was checked by skuhne@chromium.org

[commit-bot: I haz the power, 2014-03-07 17:42:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skuhne@chromium.org/169643005/240001

[commit-bot: I haz the power, 2014-03-07 17:50:07]

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

[commit-bot: I haz the power, 2014-03-07 17:50:08]

Try jobs failed on following builders: mac_chromium_compile_dbg

[Mr4D (OOO till 08-26), 2014-03-08 04:28:25]

The CQ bit was checked by skuhne@chromium.org

[commit-bot: I haz the power, 2014-03-08 10:40:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skuhne@chromium.org/169643005/260001

[commit-bot: I haz the power, 2014-03-08 12:07:32]

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

[commit-bot: I haz the power, 2014-03-08 12:07:32]

Try jobs failed on following builders: linux_chromium_chromeos_clang_dbg

[Mr4D (OOO till 08-26), 2014-03-08 16:32:33]

The CQ bit was checked by skuhne@chromium.org

[commit-bot: I haz the power, 2014-03-08 16:32:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skuhne@chromium.org/169643005/260001

[commit-bot: I haz the power, 2014-03-08 16:56:03]

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

[commit-bot: I haz the power, 2014-03-08 16:56:03]

Try jobs failed on following builders: linux_chromium_chromeos_clang_dbg

[Mr4D (OOO till 08-26), 2014-03-08 19:09:42]

The CQ bit was checked by skuhne@chromium.org

[commit-bot: I haz the power, 2014-03-08 19:09:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skuhne@chromium.org/169643005/280001

[commit-bot: I haz the power, 2014-03-08 19:36:00]

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

[commit-bot: I haz the power, 2014-03-08 19:36:00]

Retried try job too often on win_rel for step(s) base_unittests, browser_tests,
interactive_ui_tests, net_unittests, unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[Mr4D (OOO till 08-26), 2014-03-08 20:13:14]

The CQ bit was checked by skuhne@chromium.org

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

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skuhne@chromium.org/169643005/280001

[commit-bot: I haz the power, 2014-03-08 21:33:25]

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

[commit-bot: I haz the power, 2014-03-08 21:33:26]

Retried try job too often on mac_rel for step(s) telemetry_perf_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[Mr4D (OOO till 08-26), 2014-03-09 17:43:15]

The CQ bit was checked by skuhne@chromium.org

[commit-bot: I haz the power, 2014-03-09 17:43:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skuhne@chromium.org/169643005/280001

[commit-bot: I haz the power, 2014-03-09 17:58:28]

Change committed as 255863