Refactor backdrop

Refactor backdrop that is currently used in the maximized mode.

The maximized mode creates a backdrop window so that a user will not see the content of windows behind the top window,
in case it doesn't cover the entire window. (can happen if the maximize size is specified for example)
This CL generalizes the backdrop code used in maximize mode as to create the backdrop in the following scenarios:

1) Has a aura::client::kHasBackdrop property = true.
2) BackdropDelegate::HasBackdrop(aura::Window* window) returns true.
3) Active ARC window when the spoken feedback is enabled.


* Added delegate to check if the window should have a backdrop.
  Maximized mode always puts a backdrop.

* Added kHasBackdrop property for a window that needs a backdrop even in clamshell.

* Move the accessibility feature implemented in exo's backbround. This
  is useful and should be there even for non-arc/exo case.

BUG=721646
TEST=coverted by unit tests

Review-Url: https://codereview.chromium.org/2884623002
Cr-Commit-Position: refs/heads/master@{#472401}
Committed: https://chromium.googlesource.com/chromium/src/+/04936c54ed2396ae54cd824e24f11151e0e11948

[oshima, 2017-05-15 09:08:39]

Patchset #4 (id:60001) has been deleted

[oshima, 2017-05-15 09:08:43]

Patchset #2 (id:20001) has been deleted

[oshima, 2017-05-15 09:08:50]

Patchset #2 (id:40001) has been deleted

[oshima, 2017-05-15 09:24:14]

Patchset #2 (id:80001) has been deleted

[oshima, 2017-05-15 09:27:56]

Description was changed from

==========
Generalize Backdrop

BUG=721646
TEST=coverted by unit tests
==========

to

==========
Refactor backdrop

* Generalize the backdrop code used in maximize mode.
  - Added delegate to check if the window should have a backdrop.
  - maximized mode will always have one
* Added kHasBackdrop property for a window that needs a backdrop even in
clamshell.

* Move the accessibility feature implemented in exo's backbround. This
  is useful and should be there even for non-arc/exo case.


BUG=721646
TEST=coverted by unit tests
==========

[oshima, 2017-05-15 09:27:56]

oshima@chromium.org changed reviewers:
+ jamescook@chromium.org, reveman@chromium.org, sky@chromium.org

[oshima, 2017-05-15 09:28:02]

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

[commit-bot: I haz the power, 2017-05-15 09:28:25]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[oshima, 2017-05-15 09:28:37]

oshima@chromium.org changed reviewers:
- sky@chromium.org

[oshima, 2017-05-15 09:29:36]

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

[oshima, 2017-05-15 09:29:37]

jamescook@ -> ash
reveman@ -> exo
sky@ -> ui/aura

skuhne@ FYI as you're busy for io

[commit-bot: I haz the power, 2017-05-15 09:34:52]

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

[commit-bot: I haz the power, 2017-05-15 09:34:53]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[reveman, 2017-05-15 13:46:23]

components/exo lgtm with nits

https://codereview.chromium.org/2884623002/diff/100001/components/exo/shell_s...
File components/exo/shell_surface.cc (right):

https://codereview.chromium.org/2884623002/diff/100001/components/exo/shell_s...
components/exo/shell_surface.cc:1540: bool enable_backdrop =
nit: describes a state rather than an action so backdrop_enabled, has_backdrop
or should_have_backdrop would be preferred

https://codereview.chromium.org/2884623002/diff/100001/components/exo/shell_s...
components/exo/shell_surface.cc:1543: if
(window->GetProperty(aura::client::kHasBackdrop) != enable_backdrop)
is this check necessary? is SetProprty not a no-op already in this case?

https://codereview.chromium.org/2884623002/diff/100001/components/exo/shell_s...
File components/exo/shell_surface_unittest.cc (right):

https://codereview.chromium.org/2884623002/diff/100001/components/exo/shell_s...
components/exo/shell_surface_unittest.cc:842: // N underlay if there is no
shadow.
nit: s/N/An/?

[sky, 2017-05-15 17:38:29]

LGTM with the following addressed

https://codereview.chromium.org/2884623002/diff/100001/ui/aura/client/aura_co...
File ui/aura/client/aura_constants.h (right):

https://codereview.chromium.org/2884623002/diff/100001/ui/aura/client/aura_co...
ui/aura/client/aura_constants.h:118: // A bool property key to specify if the
window should have a backdrop.
'Backdrop' isn't a readily obvious term. Please document what a backdrop means.
Also, keep sorted.

[James Cook, 2017-05-15 21:57:40]

Can you add to CL description what "backdrop" means?

https://codereview.chromium.org/2884623002/diff/100001/ash/wm/maximize_mode/m...
File ash/wm/maximize_mode/maximize_mode_backdrop_delegate_impl.h (right):

https://codereview.chromium.org/2884623002/diff/100001/ash/wm/maximize_mode/m...
ash/wm/maximize_mode/maximize_mode_backdrop_delegate_impl.h:12: class ASH_EXPORT
MaximizeModeBackdropDelegateImpl : public BackdropDelegate {
Class docs please. (Like, why is this a public class rather than something local
to MaximizeModeWindowManager? Tests, right?)

optional: Also, could MaximizeModeWindowManager just implement BackdropDelegate?
Or do you think that makes the lifetimes of the delegate unclear?

https://codereview.chromium.org/2884623002/diff/100001/ash/wm/maximize_mode/m...
ash/wm/maximize_mode/maximize_mode_backdrop_delegate_impl.h:14:
MaximizeModeBackdropDelegateImpl() {}
nit: = default (and consider moving to .cc)

https://codereview.chromium.org/2884623002/diff/100001/ash/wm/workspace/backd...
File ash/wm/workspace/backdrop_controller.cc (right):

https://codereview.chromium.org/2884623002/diff/100001/ash/wm/workspace/backd...
ash/wm/workspace/backdrop_controller.cc:28: BackdropEventHandler() {}
nit: = default, and next line

https://codereview.chromium.org/2884623002/diff/100001/ash/wm/workspace/backd...
ash/wm/workspace/backdrop_controller.cc:64: : container_(container),
in_restacking_(false) {
nit: DCHECK container

https://codereview.chromium.org/2884623002/diff/100001/ash/wm/workspace/backd...
ash/wm/workspace/backdrop_controller.cc:127: // Since the backdrop needs to be
immediately behind the window and the
nit: blank line above

https://codereview.chromium.org/2884623002/diff/100001/ash/wm/workspace/backd...
ash/wm/workspace/backdrop_controller.cc:177: }
nit: no {

https://codereview.chromium.org/2884623002/diff/100001/ash/wm/workspace/backd...
ash/wm/workspace/backdrop_controller.cc:205: (delegate_ ?
delegate_->HasBackdrop(window) : false))) {
This conditional is really long. Can you break it into parts (utility function,
or local boolean variables) please?

https://codereview.chromium.org/2884623002/diff/100001/ash/wm/workspace/backd...
ash/wm/workspace/backdrop_controller.cc:218: if (backdrop_) {
nit: if (!backdrop_) return;

https://codereview.chromium.org/2884623002/diff/100001/ash/wm/workspace/backd...
File ash/wm/workspace/backdrop_controller.h (right):

https://codereview.chromium.org/2884623002/diff/100001/ash/wm/workspace/backd...
ash/wm/workspace/backdrop_controller.h:40: // entire container.
Please expand this comment to say why the backdrop exists. I had to ask Dominik,
who said:

"i believe it refers to the black background that exo inserts under maximized
arc apps that are unresizable.
in some cases, portrait apps are sized to fill the screen vertically and
positioned in the center of the workspace. the remaining space is filled with a
black backdrop.
it's also used as a target window for accessibility input events"

so maybe something like that

https://codereview.chromium.org/2884623002/diff/100001/ash/wm/workspace/backd...
ash/wm/workspace/backdrop_controller.h:43: explicit BackdropController(WmWindow*
container);
Is it possible to take aura::Window* here?

https://codereview.chromium.org/2884623002/diff/100001/ash/wm/workspace/backd...
ash/wm/workspace/backdrop_controller.h:46: void
OnWindowAddedToLayout(aura::Window* child);
Thanks for switching from WmWindow to aura::Window!

https://codereview.chromium.org/2884623002/diff/100001/ash/wm/workspace/backd...
ash/wm/workspace/backdrop_controller.h:55: // Update the visibility of, and
Restack the backdrop relatively to
nit: Restack -> restack, relatively -> relative

https://codereview.chromium.org/2884623002/diff/100001/ash/wm/workspace/backd...
ash/wm/workspace/backdrop_controller.h:77: // Hide the overlay.
overlay -> backdrop? and above?

https://codereview.chromium.org/2884623002/diff/100001/ash/wm/workspace/backd...
ash/wm/workspace/backdrop_controller.h:99: bool in_restacking_;
nit: initialize here, like the others

https://codereview.chromium.org/2884623002/diff/100001/ash/wm/workspace/backd...
File ash/wm/workspace/backdrop_delegate.h (right):

https://codereview.chromium.org/2884623002/diff/100001/ash/wm/workspace/backd...
ash/wm/workspace/backdrop_delegate.h:9: #include "ash/wm/wm_types.h"
still needed?

https://codereview.chromium.org/2884623002/diff/100001/ash/wm/workspace/backd...
ash/wm/workspace/backdrop_delegate.h:23: };
Hooray, smaller delegate!

https://codereview.chromium.org/2884623002/diff/100001/ash/wm/workspace/works...
File ash/wm/workspace/workspace_layout_manager_unittest.cc (right):

https://codereview.chromium.org/2884623002/diff/100001/ash/wm/workspace/works...
ash/wm/workspace/workspace_layout_manager_unittest.cc:1020: // THe background
widget will be created when there is a window.
nit: THe -> The

https://codereview.chromium.org/2884623002/diff/100001/ash/wm/workspace/works...
ash/wm/workspace/workspace_layout_manager_unittest.cc:1022: ASSERT_EQ(0U,
default_container()->children().size());
super-nit: 0u

https://codereview.chromium.org/2884623002/diff/100001/ash/wm/workspace/works...
ash/wm/workspace/workspace_layout_manager_unittest.cc:1288:
TEST_F(WorkspaceLayoutManagerKeyboardTest, BackdropTest) {
Can you either move the backdrop tests to someplace related to maximize mode
(maximize_mode_window_manager_unittest.cc?) or backdrop (make a new
backdrop_controller_unittest.cc?) or WorkspaceController?

Or if that's too much work, at least document in the class comments for
BackdropController where the tests live.

https://codereview.chromium.org/2884623002/diff/100001/ash/wm/workspace/works...
ash/wm/workspace/workspace_layout_manager_unittest.cc:1309:
EXPECT_TRUE(test_helper.GetBackdropWindow());
EXPECT_TRUE(backdrop)

https://codereview.chromium.org/2884623002/diff/100001/ash/wm/workspace/works...
ash/wm/workspace/workspace_layout_manager_unittest.cc:1339: // Claring the
property in the middle will not change the backdrop position.
Clearing

https://codereview.chromium.org/2884623002/diff/100001/ash/wm/workspace/works...
ash/wm/workspace/workspace_layout_manager_unittest.cc:1349: // Claring the
property on tpo will move the backdrop to bottom.
ditto, and tpo -> top

https://codereview.chromium.org/2884623002/diff/100001/ash/wm/workspace/works...
ash/wm/workspace/workspace_layout_manager_unittest.cc:1359: // Toggle overview
super nit: end with .

https://codereview.chromium.org/2884623002/diff/100001/ash/wm/workspace/works...
ash/wm/workspace/workspace_layout_manager_unittest.cc:1376:
wc->SetBackdropDelegate(base::MakeUnique<MaximizeModeBackdropDelegateImpl>());
nit: Comment what you're testing here.

https://codereview.chromium.org/2884623002/diff/100001/ash/wm/workspace/works...
ash/wm/workspace/workspace_layout_manager_unittest.cc:1385: // Toggle overview
with the delegate
super nit: end with .

https://codereview.chromium.org/2884623002/diff/100001/ash/wm/workspace/works...
ash/wm/workspace/workspace_layout_manager_unittest.cc:1472: }
Thanks for adding the test coverage!

[oshima, 2017-05-16 05:42:17]

Patchset #3 (id:120001) has been deleted

[oshima, 2017-05-16 05:47:43]

Description was changed from

==========
Refactor backdrop

* Generalize the backdrop code used in maximize mode.
  - Added delegate to check if the window should have a backdrop.
  - maximized mode will always have one
* Added kHasBackdrop property for a window that needs a backdrop even in
clamshell.

* Move the accessibility feature implemented in exo's backbround. This
  is useful and should be there even for non-arc/exo case.


BUG=721646
TEST=coverted by unit tests
==========

to

==========
Refactor backdrop that is currently used in the maximized mode.

The maximized mode creates a backdrop window so that a user will not see the
content of windows behind the top window, in case it doesn't cover the entire
window. (can happen if the maximize size is specified)

* Generalize the backdrop code used in maximize mode.
  - Added delegate to check if the window should have a backdrop.
  - maximized mode will always have one
* Added kHasBackdrop property for a window that needs a backdrop even in
clamshell.

* Move the accessibility feature implemented in exo's backbround. This
  is useful and should be there even for non-arc/exo case.


BUG=721646
TEST=coverted by unit tests
==========

[oshima, 2017-05-16 05:48:45]

Description was changed from

==========
Refactor backdrop that is currently used in the maximized mode.

The maximized mode creates a backdrop window so that a user will not see the
content of windows behind the top window, in case it doesn't cover the entire
window. (can happen if the maximize size is specified)

* Generalize the backdrop code used in maximize mode.
  - Added delegate to check if the window should have a backdrop.
  - maximized mode will always have one
* Added kHasBackdrop property for a window that needs a backdrop even in
clamshell.

* Move the accessibility feature implemented in exo's backbround. This
  is useful and should be there even for non-arc/exo case.


BUG=721646
TEST=coverted by unit tests
==========

to

==========
Refactor backdrop that is currently used in the maximized mode.

The maximized mode creates a backdrop window so that a user will not see the
content of windows behind the top window,
in case it doesn't cover the entire window. (can happen if the maximize size is
specified)
This CL generalizes the backdrop code used in maximize mode.

* Added delegate to check if the window should have a backdrop.
  Maximized mode always puts a backdrop.

* Added kHasBackdrop property for a window that needs a backdrop even in
clamshell.

* Move the accessibility feature implemented in exo's backbround. This
  is useful and should be there even for non-arc/exo case.

BUG=721646
TEST=coverted by unit tests
==========

[oshima, 2017-05-16 05:49:26]

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

[commit-bot: I haz the power, 2017-05-16 05:49:44]

Dry run: 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, 2017-05-16 06:12:51]

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

[commit-bot: I haz the power, 2017-05-16 06:12:53]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[oshima, 2017-05-16 08:22:10]

There is one scenario that was missing in the original CL.
ARC window should have the backdrop when SpokenFeedback is
enabled and its active window. I update the backdrop_controller.cc and added a
new test. PTAL.

https://codereview.chromium.org/2884623002/diff/100001/ash/wm/maximize_mode/m...
File ash/wm/maximize_mode/maximize_mode_backdrop_delegate_impl.h (right):

https://codereview.chromium.org/2884623002/diff/100001/ash/wm/maximize_mode/m...
ash/wm/maximize_mode/maximize_mode_backdrop_delegate_impl.h:12: class ASH_EXPORT
MaximizeModeBackdropDelegateImpl : public BackdropDelegate {
On 2017/05/15 21:57:38, James Cook wrote:
> Class docs please. (Like, why is this a public class rather than something
local
> to MaximizeModeWindowManager? Tests, right?)

By public class, you mean the ctor is public? I can create a factory method and
hide
this class for test, so I'm not sure if mentioning the test is really useful.

I can move the impl to MMWM, then just enter the maximized mode in the test.
WDYT?

> optional: Also, could MaximizeModeWindowManager just implement
BackdropDelegate?
> Or do you think that makes the lifetimes of the delegate unclear?

The delegate is owned by the controller, and want to keep it that way especially
because it doesn't depend on any state of the MMWM.

https://codereview.chromium.org/2884623002/diff/100001/ash/wm/maximize_mode/m...
ash/wm/maximize_mode/maximize_mode_backdrop_delegate_impl.h:14:
MaximizeModeBackdropDelegateImpl() {}
On 2017/05/15 21:57:38, James Cook wrote:
> nit: = default (and consider moving to .cc)

Done.

Is "= default" now recommended? Just double checking
because there were folks didn't like it because it's actually longer.

https://codereview.chromium.org/2884623002/diff/100001/ash/wm/workspace/backd...
File ash/wm/workspace/backdrop_controller.cc (right):

https://codereview.chromium.org/2884623002/diff/100001/ash/wm/workspace/backd...
ash/wm/workspace/backdrop_controller.cc:28: BackdropEventHandler() {}
On 2017/05/15 21:57:39, James Cook wrote:
> nit: = default, and next line

Done.

https://codereview.chromium.org/2884623002/diff/100001/ash/wm/workspace/backd...
ash/wm/workspace/backdrop_controller.cc:64: : container_(container),
in_restacking_(false) {
On 2017/05/15 21:57:38, James Cook wrote:
> nit: DCHECK container

Done.

https://codereview.chromium.org/2884623002/diff/100001/ash/wm/workspace/backd...
ash/wm/workspace/backdrop_controller.cc:127: // Since the backdrop needs to be
immediately behind the window and the
On 2017/05/15 21:57:38, James Cook wrote:
> nit: blank line above

Done.

https://codereview.chromium.org/2884623002/diff/100001/ash/wm/workspace/backd...
ash/wm/workspace/backdrop_controller.cc:177: }
On 2017/05/15 21:57:38, James Cook wrote:
> nit: no {

Done.

https://codereview.chromium.org/2884623002/diff/100001/ash/wm/workspace/backd...
ash/wm/workspace/backdrop_controller.cc:205: (delegate_ ?
delegate_->HasBackdrop(window) : false))) {
On 2017/05/15 21:57:38, James Cook wrote:
> This conditional is really long. Can you break it into parts (utility
function,
> or local boolean variables) please?

Done.

https://codereview.chromium.org/2884623002/diff/100001/ash/wm/workspace/backd...
ash/wm/workspace/backdrop_controller.cc:218: if (backdrop_) {
On 2017/05/15 21:57:38, James Cook wrote:
> nit: if (!backdrop_) return;

Done.

https://codereview.chromium.org/2884623002/diff/100001/ash/wm/workspace/backd...
File ash/wm/workspace/backdrop_controller.h (right):

https://codereview.chromium.org/2884623002/diff/100001/ash/wm/workspace/backd...
ash/wm/workspace/backdrop_controller.h:40: // entire container.
On 2017/05/15 21:57:39, James Cook wrote:
> Please expand this comment to say why the backdrop exists. I had to ask
Dominik,
> who said:
> 
> "i believe it refers to the black background that exo inserts under maximized
> arc apps that are unresizable.
> in some cases, portrait apps are sized to fill the screen vertically and
> positioned in the center of the workspace. the remaining space is filled with
a
> black backdrop.
> it's also used as a target window for accessibility input events"
> 
> so maybe something like that

I updated to be more generic

https://codereview.chromium.org/2884623002/diff/100001/ash/wm/workspace/backd...
ash/wm/workspace/backdrop_controller.h:55: // Update the visibility of, and
Restack the backdrop relatively to
On 2017/05/15 21:57:39, James Cook wrote:
> nit: Restack -> restack, relatively -> relative

Done.

https://codereview.chromium.org/2884623002/diff/100001/ash/wm/workspace/backd...
ash/wm/workspace/backdrop_controller.h:77: // Hide the overlay.
On 2017/05/15 21:57:39, James Cook wrote:
> overlay -> backdrop? and above?

Done.

https://codereview.chromium.org/2884623002/diff/100001/ash/wm/workspace/backd...
ash/wm/workspace/backdrop_controller.h:99: bool in_restacking_;
On 2017/05/15 21:57:39, James Cook wrote:
> nit: initialize here, like the others

Done.

https://codereview.chromium.org/2884623002/diff/100001/ash/wm/workspace/backd...
File ash/wm/workspace/backdrop_delegate.h (right):

https://codereview.chromium.org/2884623002/diff/100001/ash/wm/workspace/backd...
ash/wm/workspace/backdrop_delegate.h:9: #include "ash/wm/wm_types.h"
On 2017/05/15 21:57:39, James Cook wrote:
> still needed?

Done.

https://codereview.chromium.org/2884623002/diff/100001/ash/wm/workspace/backd...
ash/wm/workspace/backdrop_delegate.h:23: };
On 2017/05/15 21:57:39, James Cook wrote:
> Hooray, smaller delegate!

I was actually wondering if I should just make it a callback. LMK if you prefer
that way.

https://codereview.chromium.org/2884623002/diff/100001/ash/wm/workspace/works...
File ash/wm/workspace/workspace_layout_manager_unittest.cc (right):

https://codereview.chromium.org/2884623002/diff/100001/ash/wm/workspace/works...
ash/wm/workspace/workspace_layout_manager_unittest.cc:1020: // THe background
widget will be created when there is a window.
On 2017/05/15 21:57:39, James Cook wrote:
> nit: THe -> The

Done.

https://codereview.chromium.org/2884623002/diff/100001/ash/wm/workspace/works...
ash/wm/workspace/workspace_layout_manager_unittest.cc:1022: ASSERT_EQ(0U,
default_container()->children().size());
On 2017/05/15 21:57:39, James Cook wrote:
> super-nit: 0u

Done.

https://codereview.chromium.org/2884623002/diff/100001/ash/wm/workspace/works...
ash/wm/workspace/workspace_layout_manager_unittest.cc:1288:
TEST_F(WorkspaceLayoutManagerKeyboardTest, BackdropTest) {
On 2017/05/15 21:57:39, James Cook wrote:
> Can you either move the backdrop tests to someplace related to maximize mode
> (maximize_mode_window_manager_unittest.cc?) or backdrop (make a new
> backdrop_controller_unittest.cc?) or WorkspaceController?
> 
> Or if that's too much work, at least document in the class comments for
> BackdropController where the tests live.

This test already have tests for backdrop, and I'm adding new tests that uses
property key.
I should have grouped them together. moved.

https://codereview.chromium.org/2884623002/diff/100001/ash/wm/workspace/works...
ash/wm/workspace/workspace_layout_manager_unittest.cc:1309:
EXPECT_TRUE(test_helper.GetBackdropWindow());
On 2017/05/15 21:57:39, James Cook wrote:
> EXPECT_TRUE(backdrop)

Done.

https://codereview.chromium.org/2884623002/diff/100001/ash/wm/workspace/works...
ash/wm/workspace/workspace_layout_manager_unittest.cc:1339: // Claring the
property in the middle will not change the backdrop position.
On 2017/05/15 21:57:39, James Cook wrote:
> Clearing

Done.

https://codereview.chromium.org/2884623002/diff/100001/ash/wm/workspace/works...
ash/wm/workspace/workspace_layout_manager_unittest.cc:1349: // Claring the
property on tpo will move the backdrop to bottom.
On 2017/05/15 21:57:40, James Cook wrote:
> ditto, and tpo -> top

Done.

https://codereview.chromium.org/2884623002/diff/100001/ash/wm/workspace/works...
ash/wm/workspace/workspace_layout_manager_unittest.cc:1359: // Toggle overview
On 2017/05/15 21:57:39, James Cook wrote:
> super nit: end with .

Done.

https://codereview.chromium.org/2884623002/diff/100001/ash/wm/workspace/works...
ash/wm/workspace/workspace_layout_manager_unittest.cc:1376:
wc->SetBackdropDelegate(base::MakeUnique<MaximizeModeBackdropDelegateImpl>());
On 2017/05/15 21:57:39, James Cook wrote:
> nit: Comment what you're testing here.

Done.

https://codereview.chromium.org/2884623002/diff/100001/ash/wm/workspace/works...
ash/wm/workspace/workspace_layout_manager_unittest.cc:1385: // Toggle overview
with the delegate
On 2017/05/15 21:57:39, James Cook wrote:
> super nit: end with .

Done.

https://codereview.chromium.org/2884623002/diff/100001/components/exo/shell_s...
File components/exo/shell_surface.cc (right):

https://codereview.chromium.org/2884623002/diff/100001/components/exo/shell_s...
components/exo/shell_surface.cc:1540: bool enable_backdrop =
On 2017/05/15 13:46:22, reveman wrote:
> nit: describes a state rather than an action so backdrop_enabled, has_backdrop
> or should_have_backdrop would be preferred

Done.

https://codereview.chromium.org/2884623002/diff/100001/components/exo/shell_s...
components/exo/shell_surface.cc:1543: if
(window->GetProperty(aura::client::kHasBackdrop) != enable_backdrop)
On 2017/05/15 13:46:22, reveman wrote:
> is this check necessary? is SetProprty not a no-op already in this case?

This is currently necessary. We probably can add it to Traits though.

https://codereview.chromium.org/2884623002/diff/100001/components/exo/shell_s...
File components/exo/shell_surface_unittest.cc (right):

https://codereview.chromium.org/2884623002/diff/100001/components/exo/shell_s...
components/exo/shell_surface_unittest.cc:842: // N underlay if there is no
shadow.
On 2017/05/15 13:46:22, reveman wrote:
> nit: s/N/An/?
Fixed: No underlay.

https://codereview.chromium.org/2884623002/diff/100001/ui/aura/client/aura_co...
File ui/aura/client/aura_constants.h (right):

https://codereview.chromium.org/2884623002/diff/100001/ui/aura/client/aura_co...
ui/aura/client/aura_constants.h:118: // A bool property key to specify if the
window should have a backdrop.
On 2017/05/15 17:38:28, sky wrote:
> 'Backdrop' isn't a readily obvious term. Please document what a backdrop
means.
> Also, keep sorted.

Done.

[oshima, 2017-05-16 08:33:39]

Patchset #4 (id:160001) has been deleted

[oshima, 2017-05-16 08:33:48]

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

[commit-bot: I haz the power, 2017-05-16 08:34:05]

Dry run: 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, 2017-05-16 09:10:18]

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

[commit-bot: I haz the power, 2017-05-16 09:10:19]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[oshima, 2017-05-16 12:17:13]

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

[commit-bot: I haz the power, 2017-05-16 12:17:31]

Dry run: 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, 2017-05-16 13:37:36]

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

[commit-bot: I haz the power, 2017-05-16 13:37:37]

Dry run: This issue passed the CQ dry run.

[James Cook, 2017-05-16 15:12:15]

LGTM with nits

https://codereview.chromium.org/2884623002/diff/100001/ash/wm/maximize_mode/m...
File ash/wm/maximize_mode/maximize_mode_backdrop_delegate_impl.h (right):

https://codereview.chromium.org/2884623002/diff/100001/ash/wm/maximize_mode/m...
ash/wm/maximize_mode/maximize_mode_backdrop_delegate_impl.h:12: class ASH_EXPORT
MaximizeModeBackdropDelegateImpl : public BackdropDelegate {
On 2017/05/16 08:22:08, oshima wrote:
> On 2017/05/15 21:57:38, James Cook wrote:
> > Class docs please. (Like, why is this a public class rather than something
> local
> > to MaximizeModeWindowManager? Tests, right?)
> 
> By public class, you mean the ctor is public? I can create a factory method
and
> hide
> this class for test, so I'm not sure if mentioning the test is really useful.
> 
> I can move the impl to MMWM, then just enter the maximized mode in the test.
> WDYT?
> 
> > optional: Also, could MaximizeModeWindowManager just implement
> BackdropDelegate?
> > Or do you think that makes the lifetimes of the delegate unclear?
> 
> The delegate is owned by the controller, and want to keep it that way
especially
> because it doesn't depend on any state of the MMWM.

Yes, I meant have this class be in the .cc file for MMWM.

Keeping it owned by the controller sgtm.

https://codereview.chromium.org/2884623002/diff/100001/ash/wm/maximize_mode/m...
ash/wm/maximize_mode/maximize_mode_backdrop_delegate_impl.h:14:
MaximizeModeBackdropDelegateImpl() {}
On 2017/05/16 08:22:08, oshima wrote:
> On 2017/05/15 21:57:38, James Cook wrote:
> > nit: = default (and consider moving to .cc)
> 
> Done.
> 
> Is "= default" now recommended? Just double checking
> because there were folks didn't like it because it's actually longer.
> 

I don't think the style guide has an opinion. I prefer it.

https://codereview.chromium.org/2884623002/diff/100001/ash/wm/workspace/backd...
File ash/wm/workspace/backdrop_controller.h (right):

https://codereview.chromium.org/2884623002/diff/100001/ash/wm/workspace/backd...
ash/wm/workspace/backdrop_controller.h:40: // entire container.
On 2017/05/16 08:22:09, oshima wrote:
> On 2017/05/15 21:57:39, James Cook wrote:
> > Please expand this comment to say why the backdrop exists. I had to ask
> Dominik,
> > who said:
> > 
> > "i believe it refers to the black background that exo inserts under
maximized
> > arc apps that are unresizable.
> > in some cases, portrait apps are sized to fill the screen vertically and
> > positioned in the center of the workspace. the remaining space is filled
with
> a
> > black backdrop.
> > it's also used as a target window for accessibility input events"
> > 
> > so maybe something like that
> 
> I updated to be more generic

Can you copy the text you added to the CL description here. It would help me
know what the feature does. This part:

The maximized mode creates a backdrop window so that a user will not see the
content of windows behind the top window,
in case it doesn't cover the entire window. (can happen if the maximize size is
specified)

and it might be nice to say something like "it's common for ARC apps and
packaged Chrome apps not to be resizable" since I think that's the motivation
for this feature.

https://codereview.chromium.org/2884623002/diff/100001/ash/wm/workspace/backd...
ash/wm/workspace/backdrop_controller.h:43: explicit BackdropController(WmWindow*
container);
On 2017/05/15 21:57:39, James Cook wrote:
> Is it possible to take aura::Window* here?

Again, can you take aura::Window here instead of WmWindow?

https://codereview.chromium.org/2884623002/diff/100001/ash/wm/workspace/backd...
File ash/wm/workspace/backdrop_delegate.h (right):

https://codereview.chromium.org/2884623002/diff/100001/ash/wm/workspace/backd...
ash/wm/workspace/backdrop_delegate.h:23: };
On 2017/05/16 08:22:09, oshima wrote:
> On 2017/05/15 21:57:39, James Cook wrote:
> > Hooray, smaller delegate!
> 
> I was actually wondering if I should just make it a callback. LMK if you
prefer
> that way.

Either way is fine with me.

https://codereview.chromium.org/2884623002/diff/200001/ash/wm/workspace/backd...
File ash/wm/workspace/backdrop_controller.cc (right):

https://codereview.chromium.org/2884623002/diff/200001/ash/wm/workspace/backd...
ash/wm/workspace/backdrop_controller.cc:202:
static_cast<int>(ash::AppType::ARC_APP)) {
nit: ash:: not needed

https://codereview.chromium.org/2884623002/diff/200001/ash/wm/workspace/backd...
ash/wm/workspace/backdrop_controller.cc:223: return
(window->GetAllPropertyKeys().count(aura::client::kHasBackdrop) &&
optional: This might be easier to read as:

if (window->...)
  return true;

if (delegate_)
  return delegate_->HasBackdrop()

return false;

https://codereview.chromium.org/2884623002/diff/200001/ash/wm/workspace/backd...
File ash/wm/workspace/backdrop_controller.h (right):

https://codereview.chromium.org/2884623002/diff/200001/ash/wm/workspace/backd...
ash/wm/workspace/backdrop_controller.h:42: // 1) Has a
aura::client::kHasBackdrpo property = true.
nit: kHasBackdrop

https://codereview.chromium.org/2884623002/diff/200001/ash/wm/workspace/works...
File ash/wm/workspace/workspace_layout_manager_unittest.cc (right):

https://codereview.chromium.org/2884623002/diff/200001/ash/wm/workspace/works...
ash/wm/workspace/workspace_layout_manager_unittest.cc:1186: // Clearning the
property in the middle will not change the backdrop position.
Clearing

https://codereview.chromium.org/2884623002/diff/200001/components/exo/shell_s...
File components/exo/shell_surface.cc (right):

https://codereview.chromium.org/2884623002/diff/200001/components/exo/shell_s...
components/exo/shell_surface.cc:1536: // the bounds of the window in
fullscreen/maximize mode ( thus the
nit: no space after (

[oshima, 2017-05-17 07:37:17]

Description was changed from

==========
Refactor backdrop that is currently used in the maximized mode.

The maximized mode creates a backdrop window so that a user will not see the
content of windows behind the top window,
in case it doesn't cover the entire window. (can happen if the maximize size is
specified)
This CL generalizes the backdrop code used in maximize mode.

* Added delegate to check if the window should have a backdrop.
  Maximized mode always puts a backdrop.

* Added kHasBackdrop property for a window that needs a backdrop even in
clamshell.

* Move the accessibility feature implemented in exo's backbround. This
  is useful and should be there even for non-arc/exo case.

BUG=721646
TEST=coverted by unit tests
==========

to

==========
Refactor backdrop that is currently used in the maximized mode.

The maximized mode creates a backdrop window so that a user will not see the
content of windows behind the top window,
in case it doesn't cover the entire window. (can happen if the maximize size is
specified)
This CL generalizes the backdrop code used in maximize mode as to create the
backdrop in the following scenarios:

1) Has a aura::client::kHasBackdrop property = true.
2) BackdropDelegate::HasBackdrop(aura::Window* window) returns true.
3) Active ARC window when the spoken feedback is enabled.


* Added delegate to check if the window should have a backdrop.
  Maximized mode always puts a backdrop.

* Added kHasBackdrop property for a window that needs a backdrop even in
clamshell.

* Move the accessibility feature implemented in exo's backbround. This
  is useful and should be there even for non-arc/exo case.

BUG=721646
TEST=coverted by unit tests
==========

[oshima, 2017-05-17 07:44:34]

https://codereview.chromium.org/2884623002/diff/100001/ash/wm/maximize_mode/m...
File ash/wm/maximize_mode/maximize_mode_backdrop_delegate_impl.h (right):

https://codereview.chromium.org/2884623002/diff/100001/ash/wm/maximize_mode/m...
ash/wm/maximize_mode/maximize_mode_backdrop_delegate_impl.h:12: class ASH_EXPORT
MaximizeModeBackdropDelegateImpl : public BackdropDelegate {
On 2017/05/16 15:12:14, James Cook wrote:
> On 2017/05/16 08:22:08, oshima wrote:
> > On 2017/05/15 21:57:38, James Cook wrote:
> > > Class docs please. (Like, why is this a public class rather than something
> > local
> > > to MaximizeModeWindowManager? Tests, right?)
> > 
> > By public class, you mean the ctor is public? I can create a factory method
> and
> > hide
> > this class for test, so I'm not sure if mentioning the test is really
useful.
> > 
> > I can move the impl to MMWM, then just enter the maximized mode in the test.
> > WDYT?
> > 
> > > optional: Also, could MaximizeModeWindowManager just implement
> > BackdropDelegate?
> > > Or do you think that makes the lifetimes of the delegate unclear?
> > 
> > The delegate is owned by the controller, and want to keep it that way
> especially
> > because it doesn't depend on any state of the MMWM.
> 
> Yes, I meant have this class be in the .cc file for MMWM.
> 
> Keeping it owned by the controller sgtm.

Looks like the some of existing tests assume it's NOT in maximize mode, so i
just added comment.

https://codereview.chromium.org/2884623002/diff/100001/ash/wm/workspace/backd...
File ash/wm/workspace/backdrop_controller.h (right):

https://codereview.chromium.org/2884623002/diff/100001/ash/wm/workspace/backd...
ash/wm/workspace/backdrop_controller.h:40: // entire container.
On 2017/05/16 15:12:14, James Cook wrote:
> On 2017/05/16 08:22:09, oshima wrote:
> > On 2017/05/15 21:57:39, James Cook wrote:
> > > Please expand this comment to say why the backdrop exists. I had to ask
> > Dominik,
> > > who said:
> > > 
> > > "i believe it refers to the black background that exo inserts under
> maximized
> > > arc apps that are unresizable.
> > > in some cases, portrait apps are sized to fill the screen vertically and
> > > positioned in the center of the workspace. the remaining space is filled
> with
> > a
> > > black backdrop.
> > > it's also used as a target window for accessibility input events"
> > > 
> > > so maybe something like that
> > 
> > I updated to be more generic
> 
> Can you copy the text you added to the CL description here. It would help me
> know what the feature does. This part:
> 
> The maximized mode creates a backdrop window so that a user will not see the
> content of windows behind the top window,
> in case it doesn't cover the entire window. (can happen if the maximize size
is
> specified)
> 
> and it might be nice to say something like "it's common for ARC apps and
> packaged Chrome apps not to be resizable" since I think that's the motivation
> for this feature.

I added the comment, although I don't want to put too much implementation
specific information because:
a) the term "resizeable" is very subtle concept in android. b) It can and will
become stale.

https://codereview.chromium.org/2884623002/diff/100001/ash/wm/workspace/backd...
ash/wm/workspace/backdrop_controller.h:43: explicit BackdropController(WmWindow*
container);
On 2017/05/16 15:12:14, James Cook wrote:
> On 2017/05/15 21:57:39, James Cook wrote:
> > Is it possible to take aura::Window* here?
> 
> Again, can you take aura::Window here instead of WmWindow?

Done.

https://codereview.chromium.org/2884623002/diff/200001/ash/wm/workspace/backd...
File ash/wm/workspace/backdrop_controller.cc (right):

https://codereview.chromium.org/2884623002/diff/200001/ash/wm/workspace/backd...
ash/wm/workspace/backdrop_controller.cc:202:
static_cast<int>(ash::AppType::ARC_APP)) {
On 2017/05/16 15:12:14, James Cook wrote:
> nit: ash:: not needed

Done.

https://codereview.chromium.org/2884623002/diff/200001/ash/wm/workspace/backd...
File ash/wm/workspace/backdrop_controller.h (right):

https://codereview.chromium.org/2884623002/diff/200001/ash/wm/workspace/backd...
ash/wm/workspace/backdrop_controller.h:42: // 1) Has a
aura::client::kHasBackdrpo property = true.
On 2017/05/16 15:12:14, James Cook wrote:
> nit: kHasBackdrop

Done.

https://codereview.chromium.org/2884623002/diff/200001/ash/wm/workspace/works...
File ash/wm/workspace/workspace_layout_manager_unittest.cc (right):

https://codereview.chromium.org/2884623002/diff/200001/ash/wm/workspace/works...
ash/wm/workspace/workspace_layout_manager_unittest.cc:1186: // Clearning the
property in the middle will not change the backdrop position.
On 2017/05/16 15:12:15, James Cook wrote:
> Clearing

Done.

https://codereview.chromium.org/2884623002/diff/200001/components/exo/shell_s...
File components/exo/shell_surface.cc (right):

https://codereview.chromium.org/2884623002/diff/200001/components/exo/shell_s...
components/exo/shell_surface.cc:1536: // the bounds of the window in
fullscreen/maximize mode ( thus the
On 2017/05/16 15:12:15, James Cook wrote:
> nit: no space after (

Done.

[oshima, 2017-05-17 07:45:16]

Description was changed from

==========
Refactor backdrop that is currently used in the maximized mode.

The maximized mode creates a backdrop window so that a user will not see the
content of windows behind the top window,
in case it doesn't cover the entire window. (can happen if the maximize size is
specified)
This CL generalizes the backdrop code used in maximize mode as to create the
backdrop in the following scenarios:

1) Has a aura::client::kHasBackdrop property = true.
2) BackdropDelegate::HasBackdrop(aura::Window* window) returns true.
3) Active ARC window when the spoken feedback is enabled.


* Added delegate to check if the window should have a backdrop.
  Maximized mode always puts a backdrop.

* Added kHasBackdrop property for a window that needs a backdrop even in
clamshell.

* Move the accessibility feature implemented in exo's backbround. This
  is useful and should be there even for non-arc/exo case.

BUG=721646
TEST=coverted by unit tests
==========

to

==========
Refactor backdrop that is currently used in the maximized mode.

The maximized mode creates a backdrop window so that a user will not see the
content of windows behind the top window,
in case it doesn't cover the entire window. (can happen if the maximize size is
specified for example)
This CL generalizes the backdrop code used in maximize mode as to create the
backdrop in the following scenarios:

1) Has a aura::client::kHasBackdrop property = true.
2) BackdropDelegate::HasBackdrop(aura::Window* window) returns true.
3) Active ARC window when the spoken feedback is enabled.


* Added delegate to check if the window should have a backdrop.
  Maximized mode always puts a backdrop.

* Added kHasBackdrop property for a window that needs a backdrop even in
clamshell.

* Move the accessibility feature implemented in exo's backbround. This
  is useful and should be there even for non-arc/exo case.

BUG=721646
TEST=coverted by unit tests
==========

[oshima, 2017-05-17 07:57:34]

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

[commit-bot: I haz the power, 2017-05-17 07:58:22]

Dry run: 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, 2017-05-17 09:33:10]

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

[commit-bot: I haz the power, 2017-05-17 09:33:13]

Dry run: This issue passed the CQ dry run.

[oshima, 2017-05-17 09:37:20]

The CQ bit was checked by oshima@chromium.org

[oshima, 2017-05-17 09:37:20]

The patchset sent to the CQ was uploaded after l-g-t-m from
reveman@chromium.org, sky@chromium.org, jamescook@chromium.org
Link to the patchset: https://codereview.chromium.org/2884623002/#ps220001
(title: ".")

[commit-bot: I haz the power, 2017-05-17 09:37:55]

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, 2017-05-17 09:44:37]

CQ is committing da patch.

Bot data: {"patchset_id": 220001, "attempt_start_ts": 1495013840376260,
"parent_rev": "4c356ae265cc3276e7a3fb89c07bcbb13d8bf5c0", "commit_rev":
"04936c54ed2396ae54cd824e24f11151e0e11948"}

[commit-bot: I haz the power, 2017-05-17 09:44:49]

Description was changed from

==========
Refactor backdrop that is currently used in the maximized mode.

The maximized mode creates a backdrop window so that a user will not see the
content of windows behind the top window,
in case it doesn't cover the entire window. (can happen if the maximize size is
specified for example)
This CL generalizes the backdrop code used in maximize mode as to create the
backdrop in the following scenarios:

1) Has a aura::client::kHasBackdrop property = true.
2) BackdropDelegate::HasBackdrop(aura::Window* window) returns true.
3) Active ARC window when the spoken feedback is enabled.


* Added delegate to check if the window should have a backdrop.
  Maximized mode always puts a backdrop.

* Added kHasBackdrop property for a window that needs a backdrop even in
clamshell.

* Move the accessibility feature implemented in exo's backbround. This
  is useful and should be there even for non-arc/exo case.

BUG=721646
TEST=coverted by unit tests
==========

to

==========
Refactor backdrop that is currently used in the maximized mode.

The maximized mode creates a backdrop window so that a user will not see the
content of windows behind the top window,
in case it doesn't cover the entire window. (can happen if the maximize size is
specified for example)
This CL generalizes the backdrop code used in maximize mode as to create the
backdrop in the following scenarios:

1) Has a aura::client::kHasBackdrop property = true.
2) BackdropDelegate::HasBackdrop(aura::Window* window) returns true.
3) Active ARC window when the spoken feedback is enabled.


* Added delegate to check if the window should have a backdrop.
  Maximized mode always puts a backdrop.

* Added kHasBackdrop property for a window that needs a backdrop even in
clamshell.

* Move the accessibility feature implemented in exo's backbround. This
  is useful and should be there even for non-arc/exo case.

BUG=721646
TEST=coverted by unit tests

Review-Url: https://codereview.chromium.org/2884623002
Cr-Commit-Position: refs/heads/master@{#472401}
Committed:
https://chromium.googlesource.com/chromium/src/+/04936c54ed2396ae54cd824e24f1...
==========

[commit-bot: I haz the power, 2017-05-17 09:44:51]

Committed patchset #6 (id:220001) as
https://chromium.googlesource.com/chromium/src/+/04936c54ed2396ae54cd824e24f1...

[kolos1, 2017-05-17 13:15:59]

A revert of this CL (patchset #6 id:220001) has been created in
https://codereview.chromium.org/2887103002/ by kolos@chromium.org.

The reason for reverting is: FindIt detected this CL as culprit.
https://findit-for-me.appspot.com/waterfall/build-failure?url=https://build.c....