Added 1 liner to fix shelf background hiding on side shelf alignment.

Fixed shelf background hiding on side shelf alignment.
Also added a test!

BUG=735495
Review-Url: https://codereview.chromium.org/2946363002
Cr-Commit-Position: refs/heads/master@{#481995}
Committed: https://chromium.googlesource.com/chromium/src/+/650e3fb5b4d1452c658b2698d7c498eff6eed277

[newcomer, 2017-06-21 23:15:10]

Description was changed from

==========
Added 1 liner to fix shelf background hiding on side shelf alignment.

BUG=735495
==========

to

==========
Added 1 liner to fix shelf background hiding on side shelf alignment.

BUG=735495
==========

[newcomer, 2017-06-21 23:15:10]

newcomer@chromium.org changed reviewers:
+ jamescook@chromium.org, vadimt@chromium.org

[newcomer, 2017-06-21 23:29:30]

Patchset #1 (id:1) has been deleted

[newcomer, 2017-06-21 23:31:04]

Patchset #1 (id:20001) has been deleted

[newcomer, 2017-06-21 23:31:29]

Description was changed from

==========
Added 1 liner to fix shelf background hiding on side shelf alignment.

BUG=735495
==========

to

==========
Fixed shelf background hiding on side shelf alignment.

BUG=735495
==========

[newcomer, 2017-06-21 23:31:40]

Description was changed from

==========
Fixed shelf background hiding on side shelf alignment.

BUG=735495
==========

to

==========
Fixed shelf background hiding on side shelf alignment.
Also added a test!

BUG=735495
==========

[newcomer, 2017-06-21 23:32:16]

PTAL! Short cl.

-Alex

[newcomer, 2017-06-21 23:32:20]

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

[commit-bot: I haz the power, 2017-06-21 23:32:35]

Dry run: CQ is trying da patch.

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

[vadimt, 2017-06-21 23:45:33]

lgtm

https://codereview.chromium.org/2946363002/diff/40001/ash/app_list/app_list_p...
File ash/app_list/app_list_presenter_delegate_unittest.cc (right):

https://codereview.chromium.org/2946363002/diff/40001/ash/app_list/app_list_p...
ash/app_list/app_list_presenter_delegate_unittest.cc:251: // Set the alignment
to the side and show the app list. The background should
nit: empty line before

[James Cook, 2017-06-21 23:57:33]

https://codereview.chromium.org/2946363002/diff/40001/ash/app_list/app_list_p...
File ash/app_list/app_list_presenter_delegate_unittest.cc (right):

https://codereview.chromium.org/2946363002/diff/40001/ash/app_list/app_list_p...
ash/app_list/app_list_presenter_delegate_unittest.cc:241: Shelf* shelf =
nit: You can just do GetPrimaryShelf().  It's a method on AshTestBase.

https://codereview.chromium.org/2946363002/diff/40001/ash/app_list/app_list_p...
ash/app_list/app_list_presenter_delegate_unittest.cc:245: // Show the app list,
the shelf background should be hidden.
super-nit: "hidden" sounds like shelf autohide. I would say "transparent".

https://codereview.chromium.org/2946363002/diff/40001/ash/app_list/app_list_p...
ash/app_list/app_list_presenter_delegate_unittest.cc:258: }  // namespace ash
nit: blank line above

https://codereview.chromium.org/2946363002/diff/40001/ash/shelf/shelf_layout_...
File ash/shelf/shelf_layout_manager.cc (right):

https://codereview.chromium.org/2946363002/diff/40001/ash/shelf/shelf_layout_...
ash/shelf/shelf_layout_manager.cc:485:
app_list::features::IsFullscreenAppListEnabled())
IsFullscreenAppListEnabled() runs a non-trivial amount of code. I think you
should either:

* Cache the result here in a local variable, or
* Cache the value in a const member variable for this class (since it won't
change at runtime and this object is torn down between tests)

[newcomer, 2017-06-22 00:16:09]

Thanks for the quick feedback!

Responded to comments!

-Alex

https://codereview.chromium.org/2946363002/diff/40001/ash/app_list/app_list_p...
File ash/app_list/app_list_presenter_delegate_unittest.cc (right):

https://codereview.chromium.org/2946363002/diff/40001/ash/app_list/app_list_p...
ash/app_list/app_list_presenter_delegate_unittest.cc:241: Shelf* shelf =
On 2017/06/21 23:57:32, James Cook wrote:
> nit: You can just do GetPrimaryShelf().  It's a method on AshTestBase.

Wow Awesome. Thanks!

https://codereview.chromium.org/2946363002/diff/40001/ash/app_list/app_list_p...
ash/app_list/app_list_presenter_delegate_unittest.cc:245: // Show the app list,
the shelf background should be hidden.
On 2017/06/21 23:57:32, James Cook wrote:
> super-nit: "hidden" sounds like shelf autohide. I would say "transparent".

Done.

https://codereview.chromium.org/2946363002/diff/40001/ash/app_list/app_list_p...
ash/app_list/app_list_presenter_delegate_unittest.cc:251: // Set the alignment
to the side and show the app list. The background should
On 2017/06/21 23:45:32, vadimt wrote:
> nit: empty line before

Done.

https://codereview.chromium.org/2946363002/diff/40001/ash/app_list/app_list_p...
ash/app_list/app_list_presenter_delegate_unittest.cc:258: }  // namespace ash
On 2017/06/21 23:57:32, James Cook wrote:
> nit: blank line above

Done.

https://codereview.chromium.org/2946363002/diff/40001/ash/shelf/shelf_layout_...
File ash/shelf/shelf_layout_manager.cc (right):

https://codereview.chromium.org/2946363002/diff/40001/ash/shelf/shelf_layout_...
ash/shelf/shelf_layout_manager.cc:485:
app_list::features::IsFullscreenAppListEnabled())
On 2017/06/21 23:57:33, James Cook wrote:
> IsFullscreenAppListEnabled() runs a non-trivial amount of code. I think you
> should either:
> 
> * Cache the result here in a local variable, or
> * Cache the value in a const member variable for this class (since it won't
> change at runtime and this object is torn down between tests)

I picked option 2 because that's what I've done elsewhere!

Done.

[James Cook, 2017-06-22 00:32:47]

LGTM with nit.

Thanks for adding test coverage!

https://codereview.chromium.org/2946363002/diff/60001/ash/shelf/shelf_layout_...
File ash/shelf/shelf_layout_manager.h (right):

https://codereview.chromium.org/2946363002/diff/60001/ash/shelf/shelf_layout_...
ash/shelf/shelf_layout_manager.h:332: bool is_fullscreen_app_list_enabled_;
nit: const

[newcomer, 2017-06-22 17:08:17]

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

[commit-bot: I haz the power, 2017-06-22 17:08:38]

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-06-22 18:33:45]

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

[commit-bot: I haz the power, 2017-06-22 18:33:46]

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_...)

[newcomer, 2017-06-23 18:17:34]

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

[commit-bot: I haz the power, 2017-06-23 18:17:50]

Dry run: CQ is trying da patch.

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

[newcomer, 2017-06-23 18:55:10]

Patchset #4 (id:100001) has been deleted

[commit-bot: I haz the power, 2017-06-23 18:57:16]

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_...)

[newcomer, 2017-06-23 18:57:59]

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

[commit-bot: I haz the power, 2017-06-23 18:58:17]

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-06-23 19:47:52]

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

[commit-bot: I haz the power, 2017-06-23 19:47:55]

Dry run: This issue passed the CQ dry run.

[newcomer, 2017-06-23 19:50:40]

The CQ bit was checked by newcomer@chromium.org

[newcomer, 2017-06-23 19:50:41]

The patchset sent to the CQ was uploaded after l-g-t-m from vadimt@chromium.org,
jamescook@chromium.org
Link to the patchset: https://codereview.chromium.org/2946363002/#ps120001
(title: "Added 1 liner to fix shelf background hiding on side shelf alignment.")

[commit-bot: I haz the power, 2017-06-23 19:51:03]

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-06-23 19:55:00]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1498247440692720,
"parent_rev": "fd86d1f58b8f648498cb2ba93be84aff80e6ab1d", "commit_rev":
"650e3fb5b4d1452c658b2698d7c498eff6eed277"}

[commit-bot: I haz the power, 2017-06-23 19:55:09]

Description was changed from

==========
Fixed shelf background hiding on side shelf alignment.
Also added a test!

BUG=735495
==========

to

==========
Fixed shelf background hiding on side shelf alignment.
Also added a test!

BUG=735495

Review-Url: https://codereview.chromium.org/2946363002
Cr-Commit-Position: refs/heads/master@{#481995}
Committed:
https://chromium.googlesource.com/chromium/src/+/650e3fb5b4d1452c658b2698d7c4...
==========

[commit-bot: I haz the power, 2017-06-23 19:55:10]

Committed patchset #4 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/650e3fb5b4d1452c658b2698d7c4...

[newcomer, 2017-07-11 19:54:12]

https://codereview.chromium.org/2946363002/diff/60001/ash/shelf/shelf_layout_...
File ash/shelf/shelf_layout_manager.h (right):

https://codereview.chromium.org/2946363002/diff/60001/ash/shelf/shelf_layout_...
ash/shelf/shelf_layout_manager.h:332: bool is_fullscreen_app_list_enabled_;
On 2017/06/22 00:32:46, James Cook wrote:
> nit: const

Done.