cros: add google/mic icons to searchbox

cros: add google/mic icons to searchbox

Changes:
(1) add ui/app_list/vector_icons to store new assets and apply them
based on the state that wallpaper color customization is not available.
(2) remove back button in searchbox for new launcher

TODO:
current searchbox background color, icon and hint text color are all
defined assuming that wallpaper color customization is not available.
Modify them when wallpaper color info is available.

R=xiyuan@chromium.org
BUG=731891
TEST=device test, with --enable-features=EnableFullscreenAppList flag,
UI is in a correct WIP state. Without flag, it falls to current
behavior.

Review-Url: https://codereview.chromium.org/2941573002
Cr-Commit-Position: refs/heads/master@{#479196}
Committed: https://chromium.googlesource.com/chromium/src/+/a7ab578dc05583c914b5a1c395df4f6163dbac6e

[Qiang(Joe) Xu, 2017-06-13 18:42:16]

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

[commit-bot: I haz the power, 2017-06-13 18:42:54]

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-13 18:46:16]

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

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

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Qiang(Joe) Xu, 2017-06-13 18:57:02]

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

[commit-bot: I haz the power, 2017-06-13 18:57:31]

Dry run: CQ is trying da patch.

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

[Qiang(Joe) Xu, 2017-06-13 19:01:39]

xiyuan PTAL thanks

[xiyuan, 2017-06-13 20:10:41]

https://codereview.chromium.org/2941573002/diff/20001/ui/app_list/views/searc...
File ui/app_list/views/search_box_view.cc (right):

https://codereview.chromium.org/2941573002/diff/20001/ui/app_list/views/searc...
ui/app_list/views/search_box_view.cc:64: SkColorSetA(SK_ColorTRANSPARENT, 0xDE);
nit: SK_ColorTRANSPARENT -> SK_ColorBLACK. Techniquely, this is black as the
comment says, rather than transparency.

https://codereview.chromium.org/2941573002/diff/20001/ui/app_list/views/searc...
ui/app_list/views/search_box_view.cc:64: SkColorSetA(SK_ColorTRANSPARENT, 0xDE);
Prefer to use SkColorSetARGBMacro instead of SkColorSetA because I am not sure
what compiler would do to call an inline function to initialize a global var.

https://codereview.chromium.org/2941573002/diff/20001/ui/app_list/views/searc...
ui/app_list/views/search_box_view.cc:66: bool IsFullscreenAppListEnabled() {
Get rid of this. Alex has a CL (https://codereview.chromium.org/2934513004/)
that adds a bool member in SearchBoxView for this purpose. Please rebase and use
that.

https://codereview.chromium.org/2941573002/diff/20001/ui/app_list/views/searc...
ui/app_list/views/search_box_view.cc:150: google_icon_(new views::ImageView),
This leaks for the old behavior since |google_icon_| is not added to views
hierarchy in that case.

https://codereview.chromium.org/2941573002/diff/20001/ui/app_list/views/searc...
ui/app_list/views/search_box_view.cc:452: kDefaultSearchboxColor));
So we will show the mic icon regardless of the speech UI state?

https://codereview.chromium.org/2941573002/diff/20001/ui/app_list/views/searc...
ui/app_list/views/search_box_view.cc:460:
speech_button_->SetTooltipText(speech_button_prop->on_tooltip);
It is a bit strange that we change the image optionally but always change the
tooltip. Can you double check?

[commit-bot: I haz the power, 2017-06-13 20:39:27]

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

[commit-bot: I haz the power, 2017-06-13 20:39:28]

Dry run: This issue passed the CQ dry run.

[Qiang(Joe) Xu, 2017-06-13 20:48:57]

Patchset #4 (id:60001) has been deleted

[Qiang(Joe) Xu, 2017-06-13 20:50:53]

comments addressed, ptal

https://codereview.chromium.org/2941573002/diff/20001/ui/app_list/views/searc...
File ui/app_list/views/search_box_view.cc (right):

https://codereview.chromium.org/2941573002/diff/20001/ui/app_list/views/searc...
ui/app_list/views/search_box_view.cc:64: SkColorSetA(SK_ColorTRANSPARENT, 0xDE);
On 2017/06/13 20:10:41, xiyuan wrote:
> Prefer to use SkColorSetARGBMacro instead of SkColorSetA because I am not sure
> what compiler would do to call an inline function to initialize a global var.

Done.

https://codereview.chromium.org/2941573002/diff/20001/ui/app_list/views/searc...
ui/app_list/views/search_box_view.cc:66: bool IsFullscreenAppListEnabled() {
On 2017/06/13 20:10:41, xiyuan wrote:
> Get rid of this. Alex has a CL (https://codereview.chromium.org/2934513004/)
> that adds a bool member in SearchBoxView for this purpose. Please rebase and
use
> that.

Done.

https://codereview.chromium.org/2941573002/diff/20001/ui/app_list/views/searc...
ui/app_list/views/search_box_view.cc:150: google_icon_(new views::ImageView),
On 2017/06/13 20:10:41, xiyuan wrote:
> This leaks for the old behavior since |google_icon_| is not added to views
> hierarchy in that case.

Done.

https://codereview.chromium.org/2941573002/diff/20001/ui/app_list/views/searc...
ui/app_list/views/search_box_view.cc:452: kDefaultSearchboxColor));
On 2017/06/13 20:10:41, xiyuan wrote:
> So we will show the mic icon regardless of the speech UI state?

Specification doesn't mention this. I think we can show it, as the OK google UI
will be shown on top of searchbox right now.

https://codereview.chromium.org/2941573002/diff/20001/ui/app_list/views/searc...
ui/app_list/views/search_box_view.cc:460:
speech_button_->SetTooltipText(speech_button_prop->on_tooltip);
On 2017/06/13 20:10:41, xiyuan wrote:
> It is a bit strange that we change the image optionally but always change the
> tooltip. Can you double check?

The tooltip is changed based on whether OK google is waiting on hotword voice.
And the old behavior has two mic icons based on whether OK google is enabled.

New behavior is using one icon, no matter OK google is enabled or not. 

We really cannot see the tooltip when hotword is waiting on voice as the current
UI has "ok google" card which hides the searchbox.

We probably should remove on_tooltip, even in current behavior. Let me confirm
with PM and open a bug for this.

[xiyuan, 2017-06-13 21:09:27]

lgtm

https://codereview.chromium.org/2941573002/diff/20001/ui/app_list/views/searc...
File ui/app_list/views/search_box_view.cc (right):

https://codereview.chromium.org/2941573002/diff/20001/ui/app_list/views/searc...
ui/app_list/views/search_box_view.cc:452: kDefaultSearchboxColor));
On 2017/06/13 20:50:53, Qiang(Joe) Xu wrote:
> On 2017/06/13 20:10:41, xiyuan wrote:
> > So we will show the mic icon regardless of the speech UI state?
> 
> Specification doesn't mention this. I think we can show it, as the OK google
UI
> will be shown on top of searchbox right now.

Acknowledged.

https://codereview.chromium.org/2941573002/diff/20001/ui/app_list/views/searc...
ui/app_list/views/search_box_view.cc:460:
speech_button_->SetTooltipText(speech_button_prop->on_tooltip);
On 2017/06/13 20:50:53, Qiang(Joe) Xu wrote:
> On 2017/06/13 20:10:41, xiyuan wrote:
> > It is a bit strange that we change the image optionally but always change
the
> > tooltip. Can you double check?
> 
> The tooltip is changed based on whether OK google is waiting on hotword voice.
> And the old behavior has two mic icons based on whether OK google is enabled.
> 
> New behavior is using one icon, no matter OK google is enabled or not. 
> 
> We really cannot see the tooltip when hotword is waiting on voice as the
current
> UI has "ok google" card which hides the searchbox.
> 
> We probably should remove on_tooltip, even in current behavior. Let me confirm
> with PM and open a bug for this.

Thanks for the clarification.

https://codereview.chromium.org/2941573002/diff/80001/ui/app_list/views/searc...
File ui/app_list/views/search_box_view.cc (right):

https://codereview.chromium.org/2941573002/diff/80001/ui/app_list/views/searc...
ui/app_list/views/search_box_view.cc:47: const int kPadding = 16;
nit: Can you update those const -> constexpr since you are here?

[Qiang(Joe) Xu, 2017-06-13 21:31:22]

https://codereview.chromium.org/2941573002/diff/80001/ui/app_list/views/searc...
File ui/app_list/views/search_box_view.cc (right):

https://codereview.chromium.org/2941573002/diff/80001/ui/app_list/views/searc...
ui/app_list/views/search_box_view.cc:47: const int kPadding = 16;
On 2017/06/13 21:09:27, xiyuan wrote:
> nit: Can you update those const -> constexpr since you are here?

Done.

[Qiang(Joe) Xu, 2017-06-13 21:31:27]

The CQ bit was checked by warx@chromium.org

[Qiang(Joe) Xu, 2017-06-13 21:31:28]

The patchset sent to the CQ was uploaded after l-g-t-m from xiyuan@chromium.org
Link to the patchset: https://codereview.chromium.org/2941573002/#ps100001
(title: "constexpr")

[commit-bot: I haz the power, 2017-06-13 21:32:08]

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-13 23:10:41]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1497389487891800,
"parent_rev": "060c739be2660ac6903b8af1d1585d7eac6bfe25", "commit_rev":
"a7ab578dc05583c914b5a1c395df4f6163dbac6e"}

[commit-bot: I haz the power, 2017-06-13 23:10:51]

Description was changed from

==========
cros: add google/mic icons to searchbox

Changes:
(1) add ui/app_list/vector_icons to store new assets and apply them
based on the state that wallpaper color customization is not available.
(2) remove back button in searchbox for new launcher

TODO:
current searchbox background color, icon and hint text color are all
defined assuming that wallpaper color customization is not available.
Modify them when wallpaper color info is available.

R=xiyuan@chromium.org
BUG=731891
TEST=device test, with --enable-features=EnableFullscreenAppList flag,
UI is in a correct WIP state. Without flag, it falls to current
behavior.
==========

to

==========
cros: add google/mic icons to searchbox

Changes:
(1) add ui/app_list/vector_icons to store new assets and apply them
based on the state that wallpaper color customization is not available.
(2) remove back button in searchbox for new launcher

TODO:
current searchbox background color, icon and hint text color are all
defined assuming that wallpaper color customization is not available.
Modify them when wallpaper color info is available.

R=xiyuan@chromium.org
BUG=731891
TEST=device test, with --enable-features=EnableFullscreenAppList flag,
UI is in a correct WIP state. Without flag, it falls to current
behavior.

Review-Url: https://codereview.chromium.org/2941573002
Cr-Commit-Position: refs/heads/master@{#479196}
Committed:
https://chromium.googlesource.com/chromium/src/+/a7ab578dc05583c914b5a1c395df...
==========

[commit-bot: I haz the power, 2017-06-13 23:10:54]

Committed patchset #5 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/a7ab578dc05583c914b5a1c395df...