Skip to content

Commit

Permalink
Revert "Use the magnifying glass icon instead of the Google icon for …
Browse files Browse the repository at this point in the history
…the search box when the default search"

This reverts commit 8665027.

Reason for revert: The search_box_view_unittest.cc broke ASan for your last two tests. You apparently leak the images, maybe you just forgot to clear "SearchBoxView* view_"?
Can you use unique_pointer for your test raw pointer members?
Link to the first break:
https://build.chromium.org/p/chromium.memory/builders/Linux%20Chromium%20OS%20ASan%20LSan%20Tests%20%281%29/builds/22297


Original change's description:
> Use the magnifying glass icon instead of the Google icon for the search box when the default search
> provider is not Google.
> 
> Bug:736038
> TEST:unit tested, device tested with --enable-features=EnableFullcreenAppList
> 
> Change-Id: Ib305fbbdfea06b7f21fad2aa717af91e0f20aec0
> Reviewed-on: https://chromium-review.googlesource.com/557505
> Commit-Queue: Maajid <maajid@chromium.org>
> Reviewed-by: Yury Khmel <khmel@chromium.org>
> Reviewed-by: Qiang(Joe) Xu <warx@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#486215}

TBR=khmel@chromium.org,warx@chromium.org,newcomer@chromium.org,maajid@chromium.org

Change-Id: Ic2f9d9d0291bd676a01a23ef904d575b11300d51
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 736038
Reviewed-on: https://chromium-review.googlesource.com/569739
Reviewed-by: Friedrich Horschig <fhorschig@chromium.org>
Commit-Queue: Friedrich Horschig <fhorschig@chromium.org>
Cr-Commit-Position: refs/heads/master@{#486305}
  • Loading branch information
FHorschig authored and Commit Bot committed Jul 13, 2017
1 parent 6048b89 commit cb12b2a
Show file tree
Hide file tree
Showing 11 changed files with 30 additions and 175 deletions.
1 change: 0 additions & 1 deletion ui/app_list/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -290,7 +290,6 @@ test("app_list_unittests") {
]
deps += [
"//ui/accessibility",
"//ui/app_list/vector_icons",
"//ui/views",
"//ui/views:test_support",
]
Expand Down
8 changes: 0 additions & 8 deletions ui/app_list/app_list_constants.cc
Original file line number Diff line number Diff line change
Expand Up @@ -160,14 +160,6 @@ const char kSearchResultDistanceFromOrigin[] =
// The height of tiles in search result.
const int kSearchTileHeight = 92;

// The size of the search icon in the search box.
const int kSearchIconSize = 24;

// Default color used when wallpaper customized color is not available for
// searchbox, #000 at 87% opacity.
const SkColor kDefaultSearchboxColor =
SkColorSetARGBMacro(0xDE, 0x00, 0x00, 0x00);

gfx::ShadowValue GetShadowForZHeight(int z_height) {
if (z_height <= 0)
return gfx::ShadowValue();
Expand Down
3 changes: 0 additions & 3 deletions ui/app_list/app_list_constants.h
Original file line number Diff line number Diff line change
Expand Up @@ -110,9 +110,6 @@ APP_LIST_EXPORT extern const char kSearchResultDistanceFromOrigin[];

APP_LIST_EXPORT extern const int kSearchTileHeight;

APP_LIST_EXPORT extern const int kSearchIconSize;
APP_LIST_EXPORT extern const SkColor kDefaultSearchboxColor;

// Returns the shadow values for a view at |z_height|.
APP_LIST_EXPORT gfx::ShadowValue GetShadowForZHeight(int z_height);

Expand Down
4 changes: 0 additions & 4 deletions ui/app_list/test/app_list_test_view_delegate.cc
Original file line number Diff line number Diff line change
Expand Up @@ -89,9 +89,5 @@ void AppListTestViewDelegate::ReplaceTestModel(int item_count) {
model_->PopulateApps(item_count);
}

void AppListTestViewDelegate::SetSearchEngineIsGoogle(bool is_google) {
model_->SetSearchEngineIsGoogle(is_google);
}

} // namespace test
} // namespace app_list
3 changes: 0 additions & 3 deletions ui/app_list/test/app_list_test_view_delegate.h
Original file line number Diff line number Diff line change
Expand Up @@ -47,9 +47,6 @@ class AppListTestViewDelegate : public AppListViewDelegate {
// value to 0.
int GetStopSpeechRecognitionCountAndReset();

// Sets whether the search engine is Google or not.
void SetSearchEngineIsGoogle(bool is_google);

// AppListViewDelegate overrides:
AppListModel* GetModel() override;
SpeechUIModel* GetSpeechUI() override;
Expand Down
2 changes: 0 additions & 2 deletions ui/app_list/vector_icons/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,6 @@ aggregate_vector_icons("app_list_vector_icons") {
"ic_google_black.icon",
"ic_mic_black.1x.icon",
"ic_mic_black.icon",
"ic_search_engine_not_google.1x.icon",
"ic_search_engine_not_google.icon",
]
}

Expand Down
26 changes: 0 additions & 26 deletions ui/app_list/vector_icons/ic_search_engine_not_google.1x.icon

This file was deleted.

25 changes: 0 additions & 25 deletions ui/app_list/vector_icons/ic_search_engine_not_google.icon

This file was deleted.

56 changes: 29 additions & 27 deletions ui/app_list/views/search_box_view.cc
Original file line number Diff line number Diff line change
Expand Up @@ -58,8 +58,14 @@ constexpr SkColor kHintTextColor = SkColorSetARGBMacro(0xFF, 0xA0, 0xA0, 0xA0);

constexpr int kBackgroundBorderCornerRadius = 2;
constexpr int kBackgroundBorderCornerRadiusFullscreen = 24;
constexpr int kGoogleIconSize = 24;
constexpr int kMicIconSize = 24;

// Default color used when wallpaper customized color is not available for
// searchbox, #000 at 87% opacity.
constexpr SkColor kDefaultSearchboxColor =
SkColorSetARGBMacro(0xDE, 0x00, 0x00, 0x00);

constexpr int kLightVibrantBlendAlpha = 0xB3;

// Color of placeholder text in zero query state.
Expand Down Expand Up @@ -146,7 +152,7 @@ SearchBoxView::SearchBoxView(SearchBoxViewDelegate* delegate,
view_delegate_(view_delegate),
model_(nullptr),
content_container_(new views::View),
search_icon_(nullptr),
google_icon_(nullptr),
back_button_(nullptr),
speech_button_(nullptr),
search_box_(new views::Textfield),
Expand Down Expand Up @@ -189,10 +195,11 @@ SearchBoxView::SearchBoxView(SearchBoxViewDelegate* delegate,
content_container_->AddChildView(back_button_);

if (is_fullscreen_app_list_enabled_) {
search_icon_ = new views::ImageView();
UpdateSearchIcon(view_delegate_->GetModel()->search_engine_is_google(),
kDefaultSearchboxColor);
content_container_->AddChildView(search_icon_);
google_icon_ = new views::ImageView();
google_icon_->SetImage(gfx::CreateVectorIcon(
kIcGoogleBlackIcon, kGoogleIconSize, kDefaultSearchboxColor));
content_container_->AddChildView(google_icon_);

search_box_->set_placeholder_text_color(kDefaultSearchboxColor);
search_box_->set_placeholder_text_draw_flags(
gfx::Canvas::TEXT_ALIGN_CENTER);
Expand Down Expand Up @@ -220,8 +227,6 @@ void SearchBoxView::ModelChanged() {

model_ = view_delegate_->GetModel();
DCHECK(model_);
if (is_fullscreen_app_list_enabled_)
UpdateSearchIcon(model_->search_engine_is_google(), kDefaultSearchboxColor);
model_->search_box()->AddObserver(this);
SpeechRecognitionButtonPropChanged();
HintTextChanged();
Expand Down Expand Up @@ -351,7 +356,7 @@ void SearchBoxView::ShowBackOrGoogleIcon(bool show_back_button) {
if (!is_fullscreen_app_list_enabled_)
return;

search_icon_->SetVisible(!show_back_button);
google_icon_->SetVisible(!show_back_button);
back_button_->SetVisible(show_back_button);
content_container_->Layout();
}
Expand Down Expand Up @@ -595,24 +600,29 @@ void SearchBoxView::WallpaperProminentColorsChanged() {
prominent_colors.size());
const SkColor dark_muted =
prominent_colors[static_cast<int>(ColorProfileType::DARK_MUTED)];
const SkColor search_box_color =
SK_ColorTRANSPARENT == dark_muted ? kDefaultSearchboxColor : dark_muted;
UpdateSearchIcon(model_->search_engine_is_google(), search_box_color);
const bool dark_muted_available = SK_ColorTRANSPARENT != dark_muted;
google_icon_->SetImage(gfx::CreateVectorIcon(
kIcGoogleBlackIcon, kGoogleIconSize,
dark_muted_available ? dark_muted : kDefaultSearchboxColor));
speech_button_->SetImage(
views::Button::STATE_NORMAL,
gfx::CreateVectorIcon(kIcMicBlackIcon, kMicIconSize, search_box_color));
search_box_->set_placeholder_text_color(search_box_color);
gfx::CreateVectorIcon(
kIcMicBlackIcon, kMicIconSize,
dark_muted_available ? dark_muted : kDefaultSearchboxColor));
search_box_->set_placeholder_text_color(
dark_muted_available ? dark_muted : kDefaultSearchboxColor);

const SkColor light_vibrant =
prominent_colors[static_cast<int>(ColorProfileType::LIGHT_VIBRANT)];
const SkColor light_vibrant_mixed = color_utils::AlphaBlend(
SK_ColorWHITE, light_vibrant, kLightVibrantBlendAlpha);
const SkColor background_color = SK_ColorTRANSPARENT == light_vibrant
? kSearchBoxBackgroundDefault
: light_vibrant_mixed;
content_container_->SetBackground(
base::MakeUnique<SearchBoxBackground>(background_color));
search_box_->SetBackgroundColor(background_color);
const bool light_vibrant_available = SK_ColorTRANSPARENT != light_vibrant;
content_container_->SetBackground(base::MakeUnique<SearchBoxBackground>(
light_vibrant_available ? light_vibrant_mixed
: kSearchBoxBackgroundDefault));
search_box_->SetBackgroundColor(light_vibrant_available
? light_vibrant_mixed
: kSearchBoxBackgroundDefault);

SchedulePaint();
}
Expand All @@ -623,12 +633,4 @@ void SearchBoxView::OnSpeechRecognitionStateChanged(
SchedulePaint();
}

void SearchBoxView::UpdateSearchIcon(bool is_google,
const SkColor& search_box_color) {
const gfx::VectorIcon& icon =
is_google ? kIcGoogleBlackIcon : kIcSearchEngineNotGoogleIcon;
search_icon_->SetImage(
gfx::CreateVectorIcon(icon, kSearchIconSize, search_box_color));
}

} // namespace app_list
8 changes: 1 addition & 7 deletions ui/app_list/views/search_box_view.h
Original file line number Diff line number Diff line change
Expand Up @@ -107,19 +107,13 @@ class APP_LIST_EXPORT SearchBoxView : public views::View,
void OnGestureEvent(ui::GestureEvent* event) override;
void OnMouseEvent(ui::MouseEvent* event) override;

// Used only in the tests to get the current search icon.
views::ImageView* get_search_icon_for_test() { return search_icon_; }

private:
// Updates model text and selection model with current Textfield info.
void UpdateModel();

// Fires query change notification.
void NotifyQueryChanged();

// Updates the search icon.
void UpdateSearchIcon(bool is_google, const SkColor& search_box_color);

// Overridden from views::TextfieldController:
void ContentsChanged(views::Textfield* sender,
const base::string16& new_contents) override;
Expand Down Expand Up @@ -149,7 +143,7 @@ class APP_LIST_EXPORT SearchBoxView : public views::View,
AppListModel* model_; // Owned by the profile-keyed service.

views::View* content_container_; // Owned by views hierarchy.
views::ImageView* search_icon_; // Owned by views hierarchy.
views::ImageView* google_icon_; // Owned by views hierarchy.
SearchBoxImageButton* back_button_; // Owned by views hierarchy.
SearchBoxImageButton* speech_button_; // Owned by views hierarchy.
views::Textfield* search_box_; // Owned by views hierarchy.
Expand Down
69 changes: 0 additions & 69 deletions ui/app_list/views/search_box_view_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -9,16 +9,8 @@

#include "base/macros.h"
#include "base/strings/utf_string_conversions.h"
#include "base/test/scoped_feature_list.h"
#include "ui/app_list/app_list_constants.h"
#include "ui/app_list/app_list_features.h"
#include "ui/app_list/test/app_list_test_view_delegate.h"
#include "ui/app_list/vector_icons/vector_icons.h"
#include "ui/app_list/views/search_box_view_delegate.h"
#include "ui/gfx/image/image_skia.h"
#include "ui/gfx/image/image_unittest_util.h"
#include "ui/gfx/paint_vector_icon.h"
#include "ui/views/controls/image_view.h"
#include "ui/views/controls/textfield/textfield.h"
#include "ui/views/test/widget_test.h"

Expand Down Expand Up @@ -135,43 +127,6 @@ class SearchBoxViewTest : public views::test::WidgetTest,
DISALLOW_COPY_AND_ASSIGN(SearchBoxViewTest);
};

class SearchBoxViewFullScreenTest : public views::test::WidgetTest {
public:
SearchBoxViewFullScreenTest() {}
~SearchBoxViewFullScreenTest() override {}

// Overridden from testing::Test:
void SetUp() override {
views::test::WidgetTest::SetUp();
scoped_feature_list_.InitAndEnableFeature(
app_list::features::kEnableFullscreenAppList);
widget_ = CreateTopLevelPlatformWidget();
widget_->SetBounds(gfx::Rect(0, 0, 300, 200));
view_ = new SearchBoxView(nullptr, &view_delegate_);
}

void TearDown() override {
widget_->CloseNow();
views::test::WidgetTest::TearDown();
}

protected:
SearchBoxView* view() { return view_; }

void SetSearchEngineIsGoogle(bool is_google) {
view_delegate_.SetSearchEngineIsGoogle(is_google);
}

private:
base::test::ScopedFeatureList scoped_feature_list_;

AppListTestViewDelegate view_delegate_;
views::Widget* widget_;
SearchBoxView* view_;

DISALLOW_COPY_AND_ASSIGN(SearchBoxViewFullScreenTest);
};

TEST_F(SearchBoxViewTest, Basic) {
KeyPress(ui::VKEY_A);
EXPECT_EQ("a", GetLastQueryAndReset());
Expand Down Expand Up @@ -209,29 +164,5 @@ TEST_F(SearchBoxViewTest, CancelAutoLaunch) {
EXPECT_EQ(base::TimeDelta(), GetAutoLaunchTimeout());
}

TEST_F(SearchBoxViewFullScreenTest, SearchEngineGoogle) {
SetSearchEngineIsGoogle(true);
gfx::ImageSkia expected_icon = gfx::CreateVectorIcon(
kIcGoogleBlackIcon, kSearchIconSize, kDefaultSearchboxColor);
view()->ModelChanged();

gfx::ImageSkia actual_icon = view()->get_search_icon_for_test()->GetImage();

EXPECT_TRUE(gfx::test::AreBitmapsEqual(*expected_icon.bitmap(),
*actual_icon.bitmap()));
}

TEST_F(SearchBoxViewFullScreenTest, SearchEngineNotGoogle) {
SetSearchEngineIsGoogle(false);
gfx::ImageSkia expected_icon = gfx::CreateVectorIcon(
kIcSearchEngineNotGoogleIcon, kSearchIconSize, kDefaultSearchboxColor);
view()->ModelChanged();

gfx::ImageSkia actual_icon = view()->get_search_icon_for_test()->GetImage();

EXPECT_TRUE(gfx::test::AreBitmapsEqual(*expected_icon.bitmap(),
*actual_icon.bitmap()));
}

} // namespace test
} // namespace app_list

0 comments on commit cb12b2a

Please sign in to comment.