Add solid black background for arc immersive fullscreen.

Add solid black background for arc immersive fullscreen.

BUG=637854
Committed: https://crrev.com/c81e0cce848adda65b4e78b1a6a51e6670296e5f
Cr-Commit-Position: refs/heads/master@{#412865}

[oshima, 2016-08-16 01:07:04]

Description was changed from

==========
Add solid black background for arc immersive fullscreen.

BUG=637854
~
==========

to

==========
Add solid black background for arc immersive fullscreen.

BUG=637854
==========

[oshima, 2016-08-16 01:07:12]

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

[oshima, 2016-08-16 01:07:27]

Patchset #1 (id:1) has been deleted

[commit-bot: I haz the power, 2016-08-16 01:07: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, 2016-08-16 02:04:19]

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

[commit-bot: I haz the power, 2016-08-16 02:04:20]

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

[oshima, 2016-08-16 20:15:10]

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

[oshima, 2016-08-16 20:15:16]

Patchset #1 (id:20001) has been deleted

[commit-bot: I haz the power, 2016-08-16 20:15:34]

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, 2016-08-16 20:24:33]

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

[commit-bot: I haz the power, 2016-08-16 20:24:34]

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

[oshima, 2016-08-16 21:42:34]

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

[commit-bot: I haz the power, 2016-08-16 21:42:57]

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, 2016-08-16 22:12:41]

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

[commit-bot: I haz the power, 2016-08-16 22:12:41]

Dry run: This issue passed the CQ dry run.

[oshima, 2016-08-17 01:31:44]

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

[oshima, 2016-08-17 01:31:45]

I'm using the existing shadow underlay for now as I'm not 100% sure if this will
stay. (we may change the decision later)

Let me know if you want me to add dedicated layer.

[reveman, 2016-08-17 13:50:36]

https://codereview.chromium.org/2251543002/diff/40001/components/exo/shell_su...
File components/exo/shell_surface.cc (right):

https://codereview.chromium.org/2251543002/diff/40001/components/exo/shell_su...
components/exo/shell_surface.cc:1217: if (widget_->IsFullscreen() &&
why do we need to do this here instead of just adding this bounds logic to the
existing code for showing the shadow underlay below?

https://codereview.chromium.org/2251543002/diff/40001/components/exo/shell_su...
components/exo/shell_surface.cc:1226: shadow_underlay_->Hide();
why do we need to unconditionally hide the underlay just because the window is
transformed? shouldn't the existing shadow underlay logic be used in this case?

[oshima, 2016-08-17 15:15:37]

Patchset #2 (id:60001) has been deleted

[oshima, 2016-08-17 15:15:50]

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

[oshima, 2016-08-17 15:16:13]

https://codereview.chromium.org/2251543002/diff/40001/components/exo/shell_su...
File components/exo/shell_surface.cc (right):

https://codereview.chromium.org/2251543002/diff/40001/components/exo/shell_su...
components/exo/shell_surface.cc:1217: if (widget_->IsFullscreen() &&
On 2016/08/17 13:50:36, reveman wrote:
> why do we need to do this here instead of just adding this bounds logic to the
> existing code for showing the shadow underlay below? 

We want to show this for all immersive fullscreen even if no shadow is
requested, so I put here to be safe side. There may not be such app, so I can
move it if you prefer. let me know.

https://codereview.chromium.org/2251543002/diff/40001/components/exo/shell_su...
components/exo/shell_surface.cc:1226: shadow_underlay_->Hide();
On 2016/08/17 13:50:36, reveman wrote:
> why do we need to unconditionally hide the underlay just because the window is
> transformed? shouldn't the existing shadow underlay logic be used in this
case? 

Yes, you're right. Removed.

[commit-bot: I haz the power, 2016-08-17 15:16:15]

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, 2016-08-17 15:49:46]

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

[commit-bot: I haz the power, 2016-08-17 15:49:47]

Dry run: This issue passed the CQ dry run.

[reveman, 2016-08-17 15:54:50]

https://codereview.chromium.org/2251543002/diff/40001/components/exo/shell_su...
File components/exo/shell_surface.cc (right):

https://codereview.chromium.org/2251543002/diff/40001/components/exo/shell_su...
components/exo/shell_surface.cc:1217: if (widget_->IsFullscreen() &&
On 2016/08/17 at 15:16:12, oshima wrote:
> On 2016/08/17 13:50:36, reveman wrote:
> > why do we need to do this here instead of just adding this bounds logic to
the
> > existing code for showing the shadow underlay below? 
> 
> We want to show this for all immersive fullscreen even if no shadow is
requested, so I put here to be safe side. There may not be such app, so I can
move it if you prefer. let me know.

Yes, Id prefer if you moved it and limited this to surfaces that have a shadow
set.

[oshima, 2016-08-18 05:08:39]

Patchset #2 (id:80001) has been deleted

[oshima, 2016-08-18 05:08:44]

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

[commit-bot: I haz the power, 2016-08-18 05:08:54]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[oshima, 2016-08-18 05:10:52]

Patchset #3 (id:120001) has been deleted

[oshima, 2016-08-18 05:13:39]

Patchset #3 (id:140001) has been deleted

[oshima, 2016-08-18 05:13:44]

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

[commit-bot: I haz the power, 2016-08-18 05:14:02]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[oshima, 2016-08-18 05:14:54]

On 2016/08/17 15:54:50, reveman wrote:
>
https://codereview.chromium.org/2251543002/diff/40001/components/exo/shell_su...
> File components/exo/shell_surface.cc (right):
> 
>
https://codereview.chromium.org/2251543002/diff/40001/components/exo/shell_su...
> components/exo/shell_surface.cc:1217: if (widget_->IsFullscreen() &&
> On 2016/08/17 at 15:16:12, oshima wrote:
> > On 2016/08/17 13:50:36, reveman wrote:
> > > why do we need to do this here instead of just adding this bounds logic to
> the
> > > existing code for showing the shadow underlay below? 
> > 
> > We want to show this for all immersive fullscreen even if no shadow is
> requested, so I put here to be safe side. There may not be such app, so I can
> move it if you prefer. let me know.
> 
> Yes, Id prefer if you moved it and limited this to surfaces that have a shadow
> set.

Done.

[commit-bot: I haz the power, 2016-08-18 06:09:27]

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

[commit-bot: I haz the power, 2016-08-18 06:09:28]

Dry run: This issue passed the CQ dry run.

[reveman, 2016-08-18 16:01:21]

lgtm with nit

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

https://codereview.chromium.org/2251543002/diff/160001/components/exo/shell_s...
components/exo/shell_surface.cc:1245: ash::wm::WindowState* window_state =
ash::wm::GetWindowState(window);
nit: no need for this temporary variable as window_state is only used in one
place below

[oshima, 2016-08-18 16:24:01]

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

https://codereview.chromium.org/2251543002/diff/160001/components/exo/shell_s...
components/exo/shell_surface.cc:1245: ash::wm::WindowState* window_state =
ash::wm::GetWindowState(window);
On 2016/08/18 16:01:21, reveman wrote:
> nit: no need for this temporary variable as window_state is only used in one
> place below

Done.

[oshima, 2016-08-18 16:24:16]

Patchset #4 (id:180001) has been deleted

[oshima, 2016-08-18 16:25:06]

The CQ bit was checked by oshima@chromium.org

[oshima, 2016-08-18 16:25:06]

The patchset sent to the CQ was uploaded after l-g-t-m from reveman@chromium.org
Link to the patchset: https://codereview.chromium.org/2251543002/#ps200001
(title: "addressed comment")

[commit-bot: I haz the power, 2016-08-18 16:25:36]

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, 2016-08-18 17:13:33]

Description was changed from

==========
Add solid black background for arc immersive fullscreen.

BUG=637854
==========

to

==========
Add solid black background for arc immersive fullscreen.

BUG=637854
==========

[commit-bot: I haz the power, 2016-08-18 17:13:34]

Committed patchset #4 (id:200001)

[commit-bot: I haz the power, 2016-08-18 17:15:20]

Description was changed from

==========
Add solid black background for arc immersive fullscreen.

BUG=637854
==========

to

==========
Add solid black background for arc immersive fullscreen.

BUG=637854

Committed: https://crrev.com/c81e0cce848adda65b4e78b1a6a51e6670296e5f
Cr-Commit-Position: refs/heads/master@{#412865}
==========

[commit-bot: I haz the power, 2016-08-18 17:15:22]

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