Commit ad39c988 authored by Lei Zhang's avatar Lei Zhang Committed by Commit Bot

Simplify EmbeddedSearch interface's FocusOmnibox method.

It currently takes an OmniboxFocusState, but only two states are ever
used. Change it to a bool to remove the possibility of the renderer
sending an invalid state.

Change-Id: Ib1bc4bb5bd3415bbaf4de96d63b81fd8cfa89907
Reviewed-on: https://chromium-review.googlesource.com/1217168Reviewed-by: default avatarSam McNally <sammc@chromium.org>
Reviewed-by: default avatarKristi Park <kristipark@chromium.org>
Commit-Queue: Lei Zhang <thestig@chromium.org>
Cr-Commit-Position: refs/heads/master@{#590154}
parent c5b55d73
...@@ -148,14 +148,14 @@ void SearchIPCRouter::OnTabDeactivated() { ...@@ -148,14 +148,14 @@ void SearchIPCRouter::OnTabDeactivated() {
is_active_tab_ = false; is_active_tab_ = false;
} }
void SearchIPCRouter::FocusOmnibox(int page_seq_no, OmniboxFocusState state) { void SearchIPCRouter::FocusOmnibox(int page_seq_no, bool focus) {
if (page_seq_no != commit_counter_) if (page_seq_no != commit_counter_)
return; return;
if (!policy_->ShouldProcessFocusOmnibox(is_active_tab_)) if (!policy_->ShouldProcessFocusOmnibox(is_active_tab_))
return; return;
delegate_->FocusOmnibox(state); delegate_->FocusOmnibox(focus);
} }
void SearchIPCRouter::DeleteMostVisitedItem(int page_seq_no, const GURL& url) { void SearchIPCRouter::DeleteMostVisitedItem(int page_seq_no, const GURL& url) {
......
...@@ -42,9 +42,8 @@ class SearchIPCRouter : public content::WebContentsObserver, ...@@ -42,9 +42,8 @@ class SearchIPCRouter : public content::WebContentsObserver,
// the page. // the page.
class Delegate { class Delegate {
public: public:
// Called when the page wants the omnibox to be focused. |state| specifies // Called when the page wants the omnibox to be focused.
// the omnibox focus state. virtual void FocusOmnibox(bool focus) = 0;
virtual void FocusOmnibox(OmniboxFocusState state) = 0;
// Called when the EmbeddedSearch wants to delete a Most Visited item. // Called when the EmbeddedSearch wants to delete a Most Visited item.
virtual void OnDeleteMostVisitedItem(const GURL& url) = 0; virtual void OnDeleteMostVisitedItem(const GURL& url) = 0;
...@@ -191,7 +190,7 @@ class SearchIPCRouter : public content::WebContentsObserver, ...@@ -191,7 +190,7 @@ class SearchIPCRouter : public content::WebContentsObserver,
void OnTabDeactivated(); void OnTabDeactivated();
// chrome::mojom::EmbeddedSearch: // chrome::mojom::EmbeddedSearch:
void FocusOmnibox(int page_id, OmniboxFocusState state) override; void FocusOmnibox(int page_id, bool focus) override;
void DeleteMostVisitedItem(int page_seq_no, const GURL& url) override; void DeleteMostVisitedItem(int page_seq_no, const GURL& url) override;
void UndoMostVisitedDeletion(int page_seq_no, const GURL& url) override; void UndoMostVisitedDeletion(int page_seq_no, const GURL& url) override;
void UndoAllMostVisitedDeletions(int page_seq_no) override; void UndoAllMostVisitedDeletions(int page_seq_no) override;
......
...@@ -56,7 +56,7 @@ class MockSearchIPCRouterDelegate : public SearchIPCRouter::Delegate { ...@@ -56,7 +56,7 @@ class MockSearchIPCRouterDelegate : public SearchIPCRouter::Delegate {
public: public:
virtual ~MockSearchIPCRouterDelegate() {} virtual ~MockSearchIPCRouterDelegate() {}
MOCK_METHOD1(FocusOmnibox, void(OmniboxFocusState state)); MOCK_METHOD1(FocusOmnibox, void(bool focus));
MOCK_METHOD1(OnDeleteMostVisitedItem, void(const GURL& url)); MOCK_METHOD1(OnDeleteMostVisitedItem, void(const GURL& url));
MOCK_METHOD1(OnUndoMostVisitedDeletion, void(const GURL& url)); MOCK_METHOD1(OnUndoMostVisitedDeletion, void(const GURL& url));
MOCK_METHOD0(OnUndoAllMostVisitedDeletions, void()); MOCK_METHOD0(OnUndoAllMostVisitedDeletions, void());
......
...@@ -244,40 +244,27 @@ void SearchTabHelper::MostVisitedItemsChanged( ...@@ -244,40 +244,27 @@ void SearchTabHelper::MostVisitedItemsChanged(
ipc_router_.SendMostVisitedItems(items, is_custom_links); ipc_router_.SendMostVisitedItems(items, is_custom_links);
} }
void SearchTabHelper::FocusOmnibox(OmniboxFocusState state) { void SearchTabHelper::FocusOmnibox(bool focus) {
OmniboxView* omnibox_view = GetOmniboxView(); OmniboxView* omnibox_view = GetOmniboxView();
if (!omnibox_view) if (!omnibox_view)
return; return;
// Do not add a default case in the switch block for the following reasons: if (focus) {
// (1) Explicitly handle the new states. If new states are added in the omnibox_view->SetFocus();
// OmniboxFocusState, the compiler will warn the developer to handle the new omnibox_view->model()->SetCaretVisibility(false);
// states. // If the user clicked on the fakebox, any text already in the omnibox
// (2) An attacker may control the renderer and sends the browser process a // should get cleared when they start typing. Selecting all the existing
// malformed IPC. This function responds to the invalid |state| values by // text is a convenient way to accomplish this. It also gives a slight
// doing nothing instead of crashing the browser process (intentional no-op). // visual cue to users who really understand selection state about what
switch (state) { // will happen if they start typing.
case OMNIBOX_FOCUS_VISIBLE: omnibox_view->SelectAll(false);
NOTREACHED(); omnibox_view->ShowVirtualKeyboardIfEnabled();
break; } else {
case OMNIBOX_FOCUS_INVISIBLE: // Remove focus only if the popup is closed. This will prevent someone
omnibox_view->SetFocus(); // from changing the omnibox value and closing the popup without user
omnibox_view->model()->SetCaretVisibility(false); // interaction.
// If the user clicked on the fakebox, any text already in the omnibox if (!omnibox_view->model()->popup_model()->IsOpen())
// should get cleared when they start typing. Selecting all the existing web_contents()->Focus();
// text is a convenient way to accomplish this. It also gives a slight
// visual cue to users who really understand selection state about what
// will happen if they start typing.
omnibox_view->SelectAll(false);
omnibox_view->ShowVirtualKeyboardIfEnabled();
break;
case OMNIBOX_FOCUS_NONE:
// Remove focus only if the popup is closed. This will prevent someone
// from changing the omnibox value and closing the popup without user
// interaction.
if (!omnibox_view->model()->popup_model()->IsOpen())
web_contents()->Focus();
break;
} }
} }
......
...@@ -96,7 +96,7 @@ class SearchTabHelper : public content::WebContentsObserver, ...@@ -96,7 +96,7 @@ class SearchTabHelper : public content::WebContentsObserver,
const content::LoadCommittedDetails& load_details) override; const content::LoadCommittedDetails& load_details) override;
// Overridden from SearchIPCRouter::Delegate: // Overridden from SearchIPCRouter::Delegate:
void FocusOmnibox(OmniboxFocusState state) override; void FocusOmnibox(bool focus) override;
void OnDeleteMostVisitedItem(const GURL& url) override; void OnDeleteMostVisitedItem(const GURL& url) override;
void OnUndoMostVisitedDeletion(const GURL& url) override; void OnUndoMostVisitedDeletion(const GURL& url) override;
void OnUndoAllMostVisitedDeletions() override; void OnUndoAllMostVisitedDeletions() override;
......
...@@ -55,7 +55,7 @@ class MockSearchIPCRouterDelegate : public SearchIPCRouter::Delegate { ...@@ -55,7 +55,7 @@ class MockSearchIPCRouterDelegate : public SearchIPCRouter::Delegate {
public: public:
virtual ~MockSearchIPCRouterDelegate() {} virtual ~MockSearchIPCRouterDelegate() {}
MOCK_METHOD1(FocusOmnibox, void(OmniboxFocusState state)); MOCK_METHOD1(FocusOmnibox, void(bool focus));
MOCK_METHOD1(OnDeleteMostVisitedItem, void(const GURL& url)); MOCK_METHOD1(OnDeleteMostVisitedItem, void(const GURL& url));
MOCK_METHOD1(OnUndoMostVisitedDeletion, void(const GURL& url)); MOCK_METHOD1(OnUndoMostVisitedDeletion, void(const GURL& url));
MOCK_METHOD0(OnUndoAllMostVisitedDeletions, void()); MOCK_METHOD0(OnUndoAllMostVisitedDeletions, void());
......
...@@ -34,7 +34,7 @@ interface EmbeddedSearchConnector { ...@@ -34,7 +34,7 @@ interface EmbeddedSearchConnector {
// See http://dev.chromium.org/embeddedsearch // See http://dev.chromium.org/embeddedsearch
interface EmbeddedSearch { interface EmbeddedSearch {
// Tells InstantExtended to set the omnibox focus state. // Tells InstantExtended to set the omnibox focus state.
FocusOmnibox(int32 page_seq_no, OmniboxFocusState state); FocusOmnibox(int32 page_seq_no, bool focus);
// Tells InstantExtended to delete a most visited item. // Tells InstantExtended to delete a most visited item.
DeleteMostVisitedItem(int32 page_seq_no, url.mojom.Url url); DeleteMostVisitedItem(int32 page_seq_no, url.mojom.Url url);
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment