Add a card background to the app list search result groups.

Add a card background to the app list search result groups.

This CL adds a white rect with a shadow to the background of each group
of app list search results.

BUG=416756
Committed: https://crrev.com/61cdd1c91ec8a61f2e11fad32e6f2666558284e4
Cr-Commit-Position: refs/heads/master@{#304156}

[calamity, 2014-11-12 07:05:06]

calamity@chromium.org changed reviewers:
+ mgiuca@chromium.org

[calamity, 2014-11-12 07:05:06]

Screenies on bug.

[Matt Giuca, 2014-11-13 00:17:24]

Also looks good. Just a couple of clarifications requested.

https://codereview.chromium.org/722563002/diff/1/ui/app_list/views/search_res...
File ui/app_list/views/search_result_view.cc (right):

https://codereview.chromium.org/722563002/diff/1/ui/app_list/views/search_res...
ui/app_list/views/search_result_view.cc:213: if
(!switches::IsExperimentalAppListEnabled())
I'm curious about what effect this change, and the one below it, has on the
classic app list. Can you post screenshots, if there is a visible difference. Or
explain why not otherwise.

https://codereview.chromium.org/722563002/diff/1/ui/app_list/views/start_page...
File ui/app_list/views/start_page_view.cc (right):

https://codereview.chromium.org/722563002/diff/1/ui/app_list/views/start_page...
ui/app_list/views/start_page_view.cc:132:
tile_item->SetTitleBackgroundColor(kLabelBackgroundColor);
Let's clarify:

Previously, all TileItemViews had a solid background colour of #f5f5f5. (Same
colour as the background.)

Now, by default TileItemViews have a transparent background, such as is used in
your new SearchCardView (so they are properly painted onto a #ffffff
background). But on the StartPageView, the tile items explicitly have their
background set to #f5f5f5 to preserve the existing behaviour.

Correct? Sounds good, but I'm not sure why they can't just be transparent in the
first place...?

[calamity, 2014-11-13 03:55:32]

Oops, forgot to upload a patchset. Factored out the speech view's constants so
that we can have a consistent card shadow everywhere.

https://codereview.chromium.org/722563002/diff/1/ui/app_list/views/search_res...
File ui/app_list/views/search_result_view.cc (right):

https://codereview.chromium.org/722563002/diff/1/ui/app_list/views/search_res...
ui/app_list/views/search_result_view.cc:213: if
(!switches::IsExperimentalAppListEnabled())
On 2014/11/13 00:17:23, Matt Giuca wrote:
> I'm curious about what effect this change, and the one below it, has on the
> classic app list. Can you post screenshots, if there is a visible difference.
Or
> explain why not otherwise.

These have no effect on the classic app list. In the experimental app list, they
remove the separator line and remove the grey background respectively.

https://codereview.chromium.org/722563002/diff/1/ui/app_list/views/start_page...
File ui/app_list/views/start_page_view.cc (right):

https://codereview.chromium.org/722563002/diff/1/ui/app_list/views/start_page...
ui/app_list/views/start_page_view.cc:132:
tile_item->SetTitleBackgroundColor(kLabelBackgroundColor);
On 2014/11/13 00:17:23, Matt Giuca wrote:
> Let's clarify:
> 
> Previously, all TileItemViews had a solid background colour of #f5f5f5. (Same
> colour as the background.)
> 
> Now, by default TileItemViews have a transparent background, such as is used
in
> your new SearchCardView (so they are properly painted onto a #ffffff
> background). But on the StartPageView, the tile items explicitly have their
> background set to #f5f5f5 to preserve the existing behaviour.
> 
> Correct?
Yup. Kinda convoluted but I think it's correct for clients to set the background
color.
> Sounds good, but I'm not sure why they can't just be transparent in the
> first place...?

For non-CrOS, there are subpixel AA rendering issues when no background color is
retrievable up the view tree inside the layer. (when the text turns all broken)

[Matt Giuca, 2014-11-13 07:10:41]

lgtm

https://codereview.chromium.org/722563002/diff/1/ui/app_list/views/search_res...
File ui/app_list/views/search_result_view.cc (right):

https://codereview.chromium.org/722563002/diff/1/ui/app_list/views/search_res...
ui/app_list/views/search_result_view.cc:213: if
(!switches::IsExperimentalAppListEnabled())
On 2014/11/13 03:55:31, calamity wrote:
> On 2014/11/13 00:17:23, Matt Giuca wrote:
> > I'm curious about what effect this change, and the one below it, has on the
> > classic app list. Can you post screenshots, if there is a visible
difference.
> Or
> > explain why not otherwise.
> 
> These have no effect on the classic app list. In the experimental app list,
they
> remove the separator line and remove the grey background respectively.

Acknowledged.

https://codereview.chromium.org/722563002/diff/1/ui/app_list/views/start_page...
File ui/app_list/views/start_page_view.cc (right):

https://codereview.chromium.org/722563002/diff/1/ui/app_list/views/start_page...
ui/app_list/views/start_page_view.cc:132:
tile_item->SetTitleBackgroundColor(kLabelBackgroundColor);
On 2014/11/13 03:55:31, calamity wrote:
> On 2014/11/13 00:17:23, Matt Giuca wrote:
> > Let's clarify:
> > 
> > Previously, all TileItemViews had a solid background colour of #f5f5f5.
(Same
> > colour as the background.)
> > 
> > Now, by default TileItemViews have a transparent background, such as is used
> in
> > your new SearchCardView (so they are properly painted onto a #ffffff
> > background). But on the StartPageView, the tile items explicitly have their
> > background set to #f5f5f5 to preserve the existing behaviour.
> > 
> > Correct?
> Yup. Kinda convoluted but I think it's correct for clients to set the
background
> color.
> > Sounds good, but I'm not sure why they can't just be transparent in the
> > first place...?
> 
> For non-CrOS, there are subpixel AA rendering issues when no background color
is
> retrievable up the view tree inside the layer. (when the text turns all
broken)

Acknowledged.

[calamity, 2014-11-14 00:57:45]

The CQ bit was checked by calamity@chromium.org

[commit-bot: I haz the power, 2014-11-14 00:59:41]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/722563002/20001

[commit-bot: I haz the power, 2014-11-14 02:45:55]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2014-11-14 02:46:38]

Patchset 2 (id:??) landed as
https://crrev.com/61cdd1c91ec8a61f2e11fad32e6f2666558284e4
Cr-Commit-Position: refs/heads/master@{#304156}