diff options
35 files changed, 993 insertions, 388 deletions
@@ -164,3 +164,4 @@ Erik Hill <erikghill@gmail.com> Mao Yujie <maojie0924@gmail.com> Aaron Leventhal <aaronlevbugs@gmail.com> Peter Collingbourne <peter@pcc.me.uk> +Aaron Randolph <aaron.randolph@gmail.com> diff --git a/chrome/browser/autocomplete/autocomplete.cc b/chrome/browser/autocomplete/autocomplete.cc index e6404e4..9ea606f 100644 --- a/chrome/browser/autocomplete/autocomplete.cc +++ b/chrome/browser/autocomplete/autocomplete.cc @@ -5,6 +5,7 @@ #include "chrome/browser/autocomplete/autocomplete.h" #include <algorithm> +#include <set> #include "base/basictypes.h" #include "base/command_line.h" @@ -24,12 +25,16 @@ #include "chrome/browser/autocomplete/search_provider.h" #include "chrome/browser/autocomplete/shortcuts_provider.h" #include "chrome/browser/bookmarks/bookmark_model.h" +#include "chrome/browser/extensions/extension_service.h" #include "chrome/browser/external_protocol/external_protocol_handler.h" #include "chrome/browser/instant/instant_field_trial.h" #include "chrome/browser/net/url_fixer_upper.h" #include "chrome/browser/prefs/pref_service.h" #include "chrome/browser/profiles/profile.h" #include "chrome/browser/profiles/profile_io_data.h" +#include "chrome/browser/search_engines/template_url.h" +#include "chrome/browser/search_engines/template_url_service.h" +#include "chrome/browser/search_engines/template_url_service_factory.h" #include "chrome/browser/ui/webui/history_ui.h" #include "chrome/common/chrome_notification_types.h" #include "chrome/common/chrome_switches.h" @@ -683,6 +688,9 @@ void AutocompleteResult::AddMatch(const AutocompleteMatch& match) { } void AutocompleteResult::SortAndCull(const AutocompleteInput& input) { + for (ACMatches::iterator i = matches_.begin(); i != matches_.end(); ++i) + i->ComputeStrippedDestinationURL(); + // Remove duplicates. std::sort(matches_.begin(), matches_.end(), &AutocompleteMatch::DestinationSortFunc); @@ -991,6 +999,7 @@ void AutocompleteController::UpdateResult(bool is_synchronous_pass) { } UpdateKeywordDescriptions(&result_); + UpdateAssociatedKeywords(&result_); bool notify_default_match = is_synchronous_pass; if (!is_synchronous_pass) { @@ -998,19 +1007,54 @@ void AutocompleteController::UpdateResult(bool is_synchronous_pass) { last_result.default_match() != last_result.end(); const bool default_is_valid = result_.default_match() != result_.end(); // We've gotten async results. Send notification that the default match - // updated if fill_into_edit differs. We don't check the URL as that may - // change for the default match even though the fill into edit hasn't - // changed (see SearchProvider for one case of this). + // updated if fill_into_edit differs or associated_keyword differ. (The + // latter can change if we've just started Chrome and the keyword database + // finishes loading while processing this request.) We don't check the URL + // as that may change for the default match even though the fill into edit + // hasn't changed (see SearchProvider for one case of this). notify_default_match = (last_default_was_valid != default_is_valid) || (default_is_valid && - (result_.default_match()->fill_into_edit != - last_result.default_match()->fill_into_edit)); + ((result_.default_match()->fill_into_edit != + last_result.default_match()->fill_into_edit) || + (result_.default_match()->associated_keyword.get() != + last_result.default_match()->associated_keyword.get()))); } NotifyChanged(notify_default_match); } +void AutocompleteController::UpdateAssociatedKeywords( + AutocompleteResult* result) { + if (!keyword_provider_) + return; + + std::set<string16> keywords; + for (ACMatches::iterator match(result->begin()); match != result->end(); + ++match) { + if (!match->keyword.empty()) { + keywords.insert(match->keyword); + } else { + string16 keyword = match->associated_keyword.get() ? + match->associated_keyword->keyword : + keyword_provider_->GetKeywordForText(match->fill_into_edit); + + // Only add the keyword if the match does not have a duplicate keyword + // with a more relevant match. + if (!keyword.empty() && !keywords.count(keyword)) { + keywords.insert(keyword); + + if (!match->associated_keyword.get()) + match->associated_keyword.reset(new AutocompleteMatch( + keyword_provider_->CreateAutocompleteMatch(match->fill_into_edit, + keyword, input_))); + } else { + match->associated_keyword.reset(); + } + } + } +} + void AutocompleteController::UpdateKeywordDescriptions( AutocompleteResult* result) { const TemplateURL* last_template_url = NULL; diff --git a/chrome/browser/autocomplete/autocomplete.h b/chrome/browser/autocomplete/autocomplete.h index 9661404..3e27177 100644 --- a/chrome/browser/autocomplete/autocomplete.h +++ b/chrome/browser/autocomplete/autocomplete.h @@ -10,6 +10,7 @@ #include <string> #include <vector> +#include "base/gtest_prod_util.h" #include "base/logging.h" #include "base/memory/ref_counted.h" #include "base/string16.h" @@ -678,8 +679,12 @@ class AutocompleteController : public ACProviderListener { void set_search_provider(SearchProvider* provider) { search_provider_ = provider; } + void set_keyword_provider(KeywordProvider* provider) { + keyword_provider_ = provider; + } #endif SearchProvider* search_provider() const { return search_provider_; } + KeywordProvider* keyword_provider() const { return keyword_provider_; } // Getters const AutocompleteInput& input() const { return input_; } @@ -691,11 +696,20 @@ class AutocompleteController : public ACProviderListener { virtual void OnProviderUpdate(bool updated_matches); private: + friend class AutocompleteProviderTest; + FRIEND_TEST_ALL_PREFIXES(AutocompleteProviderTest, + RedundantKeywordsIgnoredInResult); + // Updates |result_| to reflect the current provider state. Resets timers and // fires notifications as necessary. |is_synchronous_pass| is true only when // Start() is calling this to get the synchronous result. void UpdateResult(bool is_synchronous_pass); + // Updates |result| to populate each match's |associated_keyword| if that + // match can show a keyword hint. |result| should be sorted by + // relevance before this is called. + void UpdateAssociatedKeywords(AutocompleteResult* result); + // For each group of contiguous matches from the same TemplateURL, show the // provider name as a description on the first match in the group. void UpdateKeywordDescriptions(AutocompleteResult* result); diff --git a/chrome/browser/autocomplete/autocomplete_edit.cc b/chrome/browser/autocomplete/autocomplete_edit.cc index e61a77e..7463906 100644 --- a/chrome/browser/autocomplete/autocomplete_edit.cc +++ b/chrome/browser/autocomplete/autocomplete_edit.cc @@ -173,7 +173,8 @@ void AutocompleteEditModel::FinalizeInstantQuery( if (skip_inline_autocomplete) { const string16 final_text = input_text + suggest_text; view_->OnBeforePossibleChange(); - view_->SetWindowTextAndCaretPos(final_text, final_text.length()); + view_->SetWindowTextAndCaretPos(final_text, final_text.length(), false, + false); view_->OnAfterPossibleChange(); } else if (popup_->IsOpen()) { SearchProvider* search_provider = @@ -406,7 +407,8 @@ void AutocompleteEditModel::Revert() { is_keyword_hint_ = false; has_temporary_text_ = false; view_->SetWindowTextAndCaretPos(permanent_text_, - has_focus_ ? permanent_text_.length() : 0); + has_focus_ ? permanent_text_.length() : 0, + false, true); NetworkActionPredictor* network_action_predictor = NetworkActionPredictorFactory::GetForProfile(profile_); if (network_action_predictor) @@ -416,6 +418,8 @@ void AutocompleteEditModel::Revert() { void AutocompleteEditModel::StartAutocomplete( bool has_selected_text, bool prevent_inline_autocomplete) const { + ClearPopupKeywordMode(); + bool keyword_is_selected = KeywordIsSelected(); popup_->SetHoveredLine(AutocompletePopupModel::kNoMatch); // We don't explicitly clear AutocompletePopupModel::manually_selected_match, @@ -618,28 +622,49 @@ void AutocompleteEditModel::OpenMatch(const AutocompleteMatch& match, bool AutocompleteEditModel::AcceptKeyword() { DCHECK(is_keyword_hint_ && !keyword_.empty()); - view_->OnBeforePossibleChange(); - view_->SetWindowTextAndCaretPos(string16(), 0); + autocomplete_controller_->Stop(false); is_keyword_hint_ = false; - view_->OnAfterPossibleChange(); - just_deleted_text_ = false; // OnAfterPossibleChange() erroneously sets this - // since the edit contents have disappeared. It - // doesn't really matter, but we clear it to be - // consistent. + + if (popup_->IsOpen()) + popup_->SetSelectedLineState(AutocompletePopupModel::KEYWORD); + else + StartAutocomplete(false, true); + + // Ensure the current selection is saved before showing keyword mode + // so that moving to another line and then reverting the text will restore + // the current state properly. + view_->OnTemporaryTextMaybeChanged( + DisplayTextFromUserText(CurrentMatch().fill_into_edit), + !has_temporary_text_); + has_temporary_text_ = true; + content::RecordAction(UserMetricsAction("AcceptedKeywordHint")); return true; } void AutocompleteEditModel::ClearKeyword(const string16& visible_text) { - view_->OnBeforePossibleChange(); + autocomplete_controller_->Stop(false); + ClearPopupKeywordMode(); + const string16 window_text(keyword_ + visible_text); - view_->SetWindowTextAndCaretPos(window_text.c_str(), keyword_.length()); - keyword_.clear(); - is_keyword_hint_ = false; - view_->OnAfterPossibleChange(); - just_deleted_text_ = true; // OnAfterPossibleChange() fails to clear this - // since the edit contents have actually grown - // longer. + + // Only reset the result if the edit text has changed since the + // keyword was accepted, or if the popup is closed. + if (just_deleted_text_ || !visible_text.empty() || !popup_->IsOpen()) { + view_->OnBeforePossibleChange(); + view_->SetWindowTextAndCaretPos(window_text.c_str(), keyword_.length(), + false, false); + keyword_.clear(); + is_keyword_hint_ = false; + view_->OnAfterPossibleChange(); + just_deleted_text_ = true; // OnAfterPossibleChange() fails to clear this + // since the edit contents have actually grown + // longer. + } else { + is_keyword_hint_ = true; + view_->SetWindowTextAndCaretPos(window_text.c_str(), keyword_.length(), + false, true); + } } const AutocompleteResult& AutocompleteEditModel::result() const { @@ -902,8 +927,9 @@ void AutocompleteEditModel::OnResultChanged(bool default_match_changed) { // can be many of these as a user types an initial series of characters, // the OS DNS cache could suffer eviction problems for minimal gain. - is_keyword_hint = popup_->GetKeywordForMatch(*match, &keyword); + is_keyword_hint = match->GetKeyword(&keyword); } + popup_->OnResultChanged(); OnPopupDataChanged(inline_autocomplete_text, NULL, keyword, is_keyword_hint); @@ -936,6 +962,12 @@ bool AutocompleteEditModel::KeywordIsSelected() const { return !is_keyword_hint_ && !keyword_.empty(); } +void AutocompleteEditModel::ClearPopupKeywordMode() const { + if (popup_->IsOpen() && + popup_->selected_line_state() == AutocompletePopupModel::KEYWORD) + popup_->SetSelectedLineState(AutocompletePopupModel::NORMAL); +} + string16 AutocompleteEditModel::DisplayTextFromUserText( const string16& text) const { return KeywordIsSelected() ? @@ -1035,7 +1067,9 @@ bool AutocompleteEditModel::ShouldAllowExactKeywordMatch( TRIM_LEADING, &keyword); // Only allow exact keyword match if |keyword| represents a keyword hint. - return keyword.length() && popup_->GetKeywordForText(keyword, &keyword); + return keyword.length() && + !autocomplete_controller_->keyword_provider()-> + GetKeywordForText(keyword).empty(); } bool AutocompleteEditModel::DoInstant(const AutocompleteMatch& match, diff --git a/chrome/browser/autocomplete/autocomplete_edit.h b/chrome/browser/autocomplete/autocomplete_edit.h index 3b046d3..95bee74 100644 --- a/chrome/browser/autocomplete/autocomplete_edit.h +++ b/chrome/browser/autocomplete/autocomplete_edit.h @@ -381,6 +381,9 @@ class AutocompleteEditModel : public AutocompleteControllerDelegate { // Returns true if a keyword is selected. bool KeywordIsSelected() const; + // Turns off keyword mode for the current match. + void ClearPopupKeywordMode() const; + // Conversion between user text and display text. User text is the text the // user has input. Display text is the text being shown in the edit. The // two are different if a keyword is selected. diff --git a/chrome/browser/autocomplete/autocomplete_edit_unittest.cc b/chrome/browser/autocomplete/autocomplete_edit_unittest.cc index 9771f94..f52bc4d 100644 --- a/chrome/browser/autocomplete/autocomplete_edit_unittest.cc +++ b/chrome/browser/autocomplete/autocomplete_edit_unittest.cc @@ -1,4 +1,4 @@ -// Copyright (c) 2011 The Chromium Authors. All rights reserved. +// Copyright (c) 2012 The Chromium Authors. All rights reserved. // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. @@ -35,7 +35,9 @@ class TestingOmniboxView : public OmniboxView { const string16& display_text, bool update_popup) OVERRIDE {} virtual void SetWindowTextAndCaretPos(const string16& text, - size_t caret_pos) OVERRIDE {} + size_t caret_pos, + bool update_popup, + bool notify_text_changed) OVERRIDE {} virtual void SetForcedQuery() OVERRIDE {} virtual bool IsSelectAll() OVERRIDE { return false; } virtual bool DeleteAtEndPressed() OVERRIDE { return false; } @@ -155,9 +157,9 @@ TEST(AutocompleteEditTest, AdjustTextForCopy) { TestingOmniboxView view; TestingAutocompleteEditController controller; TestingProfile profile; - AutocompleteEditModel model(&view, &controller, &profile); - profile.CreateAutocompleteClassifier(); profile.CreateTemplateURLService(); + profile.CreateAutocompleteClassifier(); + AutocompleteEditModel model(&view, &controller, &profile); for (size_t i = 0; i < ARRAYSIZE_UNSAFE(input); ++i) { model.UpdatePermanentText(ASCIIToUTF16(input[i].perm_text)); diff --git a/chrome/browser/autocomplete/autocomplete_match.cc b/chrome/browser/autocomplete/autocomplete_match.cc index 39ff3ba..be0b474 100644 --- a/chrome/browser/autocomplete/autocomplete_match.cc +++ b/chrome/browser/autocomplete/autocomplete_match.cc @@ -46,9 +46,61 @@ AutocompleteMatch::AutocompleteMatch(AutocompleteProvider* provider, from_previous(false) { } +AutocompleteMatch::AutocompleteMatch(const AutocompleteMatch& match) + : provider(match.provider), + relevance(match.relevance), + deletable(match.deletable), + fill_into_edit(match.fill_into_edit), + inline_autocomplete_offset(match.inline_autocomplete_offset), + destination_url(match.destination_url), + stripped_destination_url(match.stripped_destination_url), + contents(match.contents), + contents_class(match.contents_class), + description(match.description), + description_class(match.description_class), + transition(match.transition), + is_history_what_you_typed_match(match.is_history_what_you_typed_match), + type(match.type), + keyword(match.keyword), + template_url(match.template_url), + starred(match.starred), + from_previous(match.from_previous) { + if (match.associated_keyword.get()) + associated_keyword.reset(new AutocompleteMatch(*match.associated_keyword)); +} + AutocompleteMatch::~AutocompleteMatch() { } +AutocompleteMatch& AutocompleteMatch::operator=( + const AutocompleteMatch& match) { + if (this == &match) + return *this; + + provider = match.provider; + relevance = match.relevance; + deletable = match.deletable; + fill_into_edit = match.fill_into_edit; + inline_autocomplete_offset = match.inline_autocomplete_offset; + destination_url = match.destination_url; + stripped_destination_url = match.stripped_destination_url; + contents = match.contents; + contents_class = match.contents_class; + description = match.description; + description_class = match.description_class; + transition = match.transition; + is_history_what_you_typed_match = match.is_history_what_you_typed_match; + type = match.type; + associated_keyword.reset(match.associated_keyword.get() ? + new AutocompleteMatch(*match.associated_keyword) : NULL); + keyword = match.keyword; + template_url = match.template_url; + starred = match.starred; + from_previous = match.from_previous; + + return *this; +} + // static std::string AutocompleteMatch::TypeToString(Type type) { const char* strings[] = { @@ -107,15 +159,15 @@ bool AutocompleteMatch::DestinationSortFunc(const AutocompleteMatch& elem1, // Sort identical destination_urls together. Place the most relevant matches // first, so that when we call std::unique(), these are the ones that get // preserved. - return (elem1.destination_url != elem2.destination_url) ? - (elem1.destination_url < elem2.destination_url) : + return (elem1.stripped_destination_url != elem2.stripped_destination_url) ? + (elem1.stripped_destination_url < elem2.stripped_destination_url) : MoreRelevant(elem1, elem2); } // static bool AutocompleteMatch::DestinationsEqual(const AutocompleteMatch& elem1, const AutocompleteMatch& elem2) { - return elem1.destination_url == elem2.destination_url; + return elem1.stripped_destination_url == elem2.stripped_destination_url; } // static @@ -175,6 +227,28 @@ string16 AutocompleteMatch::SanitizeString(const string16& text) { return result; } +void AutocompleteMatch::ComputeStrippedDestinationURL() { + static const char prefix[] = "www."; + static const size_t prefix_len = arraysize(prefix) - 1; + + std::string host = destination_url.host(); + if (destination_url.is_valid() && host.compare(0, prefix_len, prefix) == 0) { + host = host.substr(prefix_len); + GURL::Replacements replace_host; + replace_host.SetHostStr(host); + stripped_destination_url = destination_url.ReplaceComponents(replace_host); + } else { + stripped_destination_url = destination_url; + } +} + +bool AutocompleteMatch::GetKeyword(string16* keyword) const { + const bool is_keyword_hint = associated_keyword.get() != NULL; + keyword->assign(is_keyword_hint ? associated_keyword->keyword : + this->keyword); + return is_keyword_hint; +} + #ifndef NDEBUG void AutocompleteMatch::Validate() const { ValidateClassifications(contents, contents_class); diff --git a/chrome/browser/autocomplete/autocomplete_match.h b/chrome/browser/autocomplete/autocomplete_match.h index d1229a9..aff42cf 100644 --- a/chrome/browser/autocomplete/autocomplete_match.h +++ b/chrome/browser/autocomplete/autocomplete_match.h @@ -9,6 +9,7 @@ #include <vector> #include <string> +#include "base/memory/scoped_ptr.h" #include "content/public/common/page_transition_types.h" #include "googleurl/src/gurl.h" @@ -90,9 +91,13 @@ struct AutocompleteMatch { int relevance, bool deletable, Type type); + AutocompleteMatch(const AutocompleteMatch& match); ~AutocompleteMatch(); // Converts |type| to a string representation. Used in logging and debugging. + AutocompleteMatch& operator=(const AutocompleteMatch& match); + + // Converts |type| to a string representation. Used in logging. static std::string TypeToString(Type type); // Converts |type| to a resource identifier for the appropriate icon for this @@ -104,6 +109,7 @@ struct AutocompleteMatch { const AutocompleteMatch& elem2); // Comparison functions for removing matches with duplicate destinations. + // Destinations are compared using |stripped_destination_url|. static bool DestinationSortFunc(const AutocompleteMatch& elem1, const AutocompleteMatch& elem2); static bool DestinationsEqual(const AutocompleteMatch& elem1, @@ -133,6 +139,18 @@ struct AutocompleteMatch { // or |description|. static string16 SanitizeString(const string16& text); + // Copies the destination_url with "www." stripped off to + // |stripped_destination_url|. This method is invoked internally by the + // AutocompleteController and does not normally need to be invoked. + void ComputeStrippedDestinationURL(); + + // Gets the selected keyword or keyword hint for this match. Returns + // true if |keyword| represents a keyword hint, or false if |keyword| + // represents a selected keyword. (|keyword| will always be set [though + // possibly to the empty string], and you cannot have both a selected keyword + // and a keyword hint simultaneously.) + bool GetKeyword(string16* keyword) const; + // The provider of this match, used to remember which provider the user had // selected when the input changes. This may be NULL, in which case there is // no provider (or memory of the user's selection). @@ -165,6 +183,9 @@ struct AutocompleteMatch { // It may be empty if there is no possible navigation. GURL destination_url; + // The destination URL with "www." stripped off for better dupe finding. + GURL stripped_destination_url; + // The main text displayed in the address bar dropdown. string16 contents; ACMatchClassifications contents_class; @@ -185,6 +206,22 @@ struct AutocompleteMatch { // Type of this match. Type type; + // Set with a keyword provider match if this match can show a keyword hint. + // For example, if this is a SearchProvider match for "www.amazon.com", + // |associated_keyword| could be a KeywordProvider match for "amazon.com". + scoped_ptr<AutocompleteMatch> associated_keyword; + + // For matches that correspond to valid substituting keywords ("search + // engines" that aren't the default engine, or extension keywords), this + // is the keyword. If this is set, then when displaying this match, the + // edit will use the "keyword mode" UI that shows a blue + // "Search <engine name>" chit before the user's typing. This should be + // set for any match that's an |associated_keyword| of a match in the main + // result list, as well as any other matches in the main result list that + // are direct keyword matches (e.g. if the user types in a keyword name and + // some search terms directly). + string16 keyword; + // Indicates the TemplateURL the match originated from. This is set for // keywords as well as matches for the default search provider. const TemplateURL* template_url; diff --git a/chrome/browser/autocomplete/autocomplete_popup_model.cc b/chrome/browser/autocomplete/autocomplete_popup_model.cc index d0d54f0..30198d5 100644 --- a/chrome/browser/autocomplete/autocomplete_popup_model.cc +++ b/chrome/browser/autocomplete/autocomplete_popup_model.cc @@ -1,4 +1,4 @@ -// Copyright (c) 2011 The Chromium Authors. All rights reserved. +// Copyright (c) 2012 The Chromium Authors. All rights reserved. // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. @@ -23,13 +23,16 @@ /////////////////////////////////////////////////////////////////////////////// // AutocompletePopupModel +const size_t AutocompletePopupModel::kNoMatch = -1; + AutocompletePopupModel::AutocompletePopupModel( AutocompletePopupView* popup_view, AutocompleteEditModel* edit_model) : view_(popup_view), edit_model_(edit_model), hovered_line_(kNoMatch), - selected_line_(kNoMatch) { + selected_line_(kNoMatch), + selected_line_state_(NORMAL) { edit_model->set_popup_model(this); } @@ -83,24 +86,29 @@ void AutocompletePopupModel::SetSelectedLine(size_t line, if (line == selected_line_ && !force) return; // Nothing else to do. - // We need to update |selected_line_| before calling OnPopupDataChanged(), so - // that when the edit notifies its controller that something has changed, the - // controller can get the correct updated data. + // We need to update |selected_line_state_| and |selected_line_| before + // calling InvalidateLine(), since it will check them to determine how to + // draw. We also need to update |selected_line_| before calling + // OnPopupDataChanged(), so that when the edit notifies its controller that + // something has changed, the controller can get the correct updated data. // // NOTE: We should never reach here with no selected line; the same code that // opened the popup and made it possible to get here should have also set a // selected line. CHECK(selected_line_ != kNoMatch); GURL current_destination(result.match_at(selected_line_).destination_url); - view_->InvalidateLine(selected_line_); + const size_t prev_selected_line = selected_line_; + selected_line_state_ = NORMAL; selected_line_ = line; + view_->InvalidateLine(prev_selected_line); view_->InvalidateLine(selected_line_); // Update the edit with the new data for this match. // TODO(pkasting): If |selected_line_| moves to the controller, this can be // eliminated and just become a call to the observer on the edit. string16 keyword; - const bool is_keyword_hint = GetKeywordForMatch(match, &keyword); + const bool is_keyword_hint = match.GetKeyword(&keyword); + if (reset_to_default) { string16 inline_autocomplete_text; if ((match.inline_autocomplete_offset != string16::npos) && @@ -127,59 +135,6 @@ void AutocompletePopupModel::ResetToDefaultMatch() { view_->OnDragCanceled(); } -bool AutocompletePopupModel::GetKeywordForMatch(const AutocompleteMatch& match, - string16* keyword) const { - // Assume we have no keyword until we find otherwise. - keyword->clear(); - - if (match.template_url && - TemplateURL::SupportsReplacement(match.template_url) && - match.transition == content::PAGE_TRANSITION_KEYWORD) { - // The current match is a keyword, return that as the selected keyword. - keyword->assign(match.template_url->keyword()); - return false; - } - - // See if the current match's fill_into_edit corresponds to a keyword. - return GetKeywordForText(match.fill_into_edit, keyword); -} - -bool AutocompletePopupModel::GetKeywordForText(const string16& text, - string16* keyword) const { - // Creates keyword_hint first in case |keyword| is a pointer to |text|. - const string16 keyword_hint(TemplateURLService::CleanUserInputKeyword(text)); - - // Assume we have no keyword until we find otherwise. - keyword->clear(); - - if (keyword_hint.empty()) - return false; - Profile* profile = edit_model_->profile(); - TemplateURLService* url_service = - TemplateURLServiceFactory::GetForProfile(profile); - if (!url_service) - return false; - url_service->Load(); - - // Don't provide a hint if this keyword doesn't support replacement. - const TemplateURL* const template_url = - url_service->GetTemplateURLForKeyword(keyword_hint); - if (!TemplateURL::SupportsReplacement(template_url)) - return false; - - // Don't provide a hint for inactive/disabled extension keywords. - if (template_url->IsExtensionKeyword()) { - const Extension* extension = profile->GetExtensionService()-> - GetExtensionById(template_url->GetExtensionId(), false); - if (!extension || (profile->IsOffTheRecord() && - !profile->GetExtensionService()->IsIncognitoEnabled(extension->id()))) - return false; - } - - keyword->assign(keyword_hint); - return true; -} - void AutocompletePopupModel::Move(int count) { const AutocompleteResult& result = this->result(); if (result.empty()) @@ -195,6 +150,17 @@ void AutocompletePopupModel::Move(int count) { false, false); } +void AutocompletePopupModel::SetSelectedLineState(LineState state) { + DCHECK(!result().empty()); + DCHECK_NE(kNoMatch, selected_line_); + + const AutocompleteMatch& match = result().match_at(selected_line_); + DCHECK(match.associated_keyword.get()); + + selected_line_state_ = state; + view_->InvalidateLine(selected_line_); +} + void AutocompletePopupModel::TryDeletingCurrentItem() { // We could use InfoForCurrentSelection() here, but it seems better to try // and shift-delete the actual selection, rather than any "in progress, not @@ -243,6 +209,7 @@ void AutocompletePopupModel::OnResultChanged() { // There had better not be a nonempty result set with no default match. CHECK((selected_line_ != kNoMatch) || result.empty()); manually_selected_match_.Clear(); + selected_line_state_ = NORMAL; // If we're going to trim the window size to no longer include the hovered // line, turn hover off. Practically, this shouldn't happen, but it // doesn't hurt to be defensive. diff --git a/chrome/browser/autocomplete/autocomplete_popup_model.h b/chrome/browser/autocomplete/autocomplete_popup_model.h index 173800a..59266fd 100644 --- a/chrome/browser/autocomplete/autocomplete_popup_model.h +++ b/chrome/browser/autocomplete/autocomplete_popup_model.h @@ -1,4 +1,4 @@ -// Copyright (c) 2011 The Chromium Authors. All rights reserved. +// Copyright (c) 2012 The Chromium Authors. All rights reserved. // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. @@ -15,6 +15,12 @@ class SkBitmap; class AutocompletePopupModel { public: + // See selected_line_state_ for details. + enum LineState { + NORMAL = 0, + KEYWORD + }; + AutocompletePopupModel(AutocompletePopupView* popup_view, AutocompleteEditModel* edit_model); ~AutocompletePopupModel(); @@ -45,6 +51,10 @@ class AutocompletePopupModel { return selected_line_; } + LineState selected_line_state() const { + return selected_line_state_; + } + // Call to change the selected line. This will update all state and repaint // the necessary parts of the window, as well as updating the edit with the // new temporary text. |line| will be clamped to the range of valid lines. @@ -61,19 +71,6 @@ class AutocompletePopupModel { // will change the selected line back to the default match and redraw. void ResetToDefaultMatch(); - // Gets the selected keyword or keyword hint for the given match. If the match - // is already keyword, then the keyword will be returned directly. Otherwise, - // it returns GetKeywordForText(match.fill_into_edit, keyword). - bool GetKeywordForMatch(const AutocompleteMatch& match, - string16* keyword) const; - - // Gets the selected keyword or keyword hint for the given text. Returns - // true if |keyword| represents a keyword hint, or false if |keyword| - // represents a selected keyword. (|keyword| will always be set [though - // possibly to the empty string], and you cannot have both a selected keyword - // and a keyword hint simultaneously.) - bool GetKeywordForText(const string16& text, string16* keyword) const; - // Immediately updates and opens the popup if necessary, then moves the // current selection down (|count| > 0) or up (|count| < 0), clamping to the // first or last result if necessary. If |count| == 0, the selection will be @@ -81,6 +78,11 @@ class AutocompletePopupModel { // AutocompleteEditModel. void Move(int count); + // If the selected line has both a normal match and a keyword match, this can + // be used to choose which to select. It is an error to call this when the + // selected line does not have both matches (or there is no selection). + void SetSelectedLineState(LineState state); + // Called when the user hits shift-delete. This should determine if the item // can be removed from history, and if so, remove it and update the popup. void TryDeletingCurrentItem(); @@ -100,7 +102,7 @@ class AutocompletePopupModel { // The token value for selected_line_, hover_line_ and functions dealing with // a "line number" that indicates "no line". - static const size_t kNoMatch = -1; + static const size_t kNoMatch; private: AutocompletePopupView* view_; @@ -115,6 +117,11 @@ class AutocompletePopupModel { // which should only be true when the popup is closed. size_t selected_line_; + // If the selected line has both a normal match and a keyword match, this + // determines whether the normal match (if NORMAL) or the keyword match + // (if KEYWORD) is selected. + LineState selected_line_state_; + // The match the user has manually chosen, if any. AutocompleteResult::Selection manually_selected_match_; diff --git a/chrome/browser/autocomplete/autocomplete_unittest.cc b/chrome/browser/autocomplete/autocomplete_unittest.cc index f0c7dab..8793e5b 100644 --- a/chrome/browser/autocomplete/autocomplete_unittest.cc +++ b/chrome/browser/autocomplete/autocomplete_unittest.cc @@ -1,4 +1,4 @@ -// Copyright (c) 2011 The Chromium Authors. All rights reserved. +// Copyright (c) 2012 The Chromium Authors. All rights reserved. // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. @@ -30,8 +30,8 @@ static std::ostream& operator<<(std::ostream& os, } namespace { - -const size_t num_results_per_provider = 3; +const size_t kResultsPerProvider = 3; +} // Autocomplete provider that provides known results. Note that this is // refcounted so that it can also be a task on the message loop. @@ -79,8 +79,8 @@ void TestProvider::Start(const AutocompleteInput& input, } void TestProvider::Run() { - DCHECK_GT(num_results_per_provider, 0U); - AddResults(1, num_results_per_provider); + DCHECK_GT(kResultsPerProvider, 0U); + AddResults(1, kResultsPerProvider); done_ = true; DCHECK(listener_); listener_->OnProviderUpdate(true); @@ -108,19 +108,32 @@ void TestProvider::AddResults(int start_at, int num) { class AutocompleteProviderTest : public testing::Test, public content::NotificationObserver { protected: + struct KeywordTestData { + const string16 fill_into_edit; + const string16 keyword; + const bool expected_keyword_result; + }; + + protected: void ResetControllerWithTestProviders(bool same_destinations); // Runs a query on the input "a", and makes sure both providers' input is // properly collected. void RunTest(); + void RunRedundantKeywordTest(const KeywordTestData* match_data, size_t size); + + void RunQuery(const string16 query); + void ResetControllerWithTestProvidersWithKeywordAndSearchProviders(); + void ResetControllerWithKeywordProvider(); void RunExactKeymatchTest(bool allow_exact_keyword_match); // These providers are owned by the controller once it's created. ACProviders providers_; AutocompleteResult result_; + scoped_ptr<AutocompleteController> controller_; private: // content::NotificationObserver @@ -129,7 +142,6 @@ class AutocompleteProviderTest : public testing::Test, const content::NotificationDetails& details); MessageLoopForUI message_loop_; - scoped_ptr<AutocompleteController> controller_; content::NotificationRegistrar registrar_; TestingProfile profile_; }; @@ -141,12 +153,12 @@ void AutocompleteProviderTest::ResetControllerWithTestProviders( providers_.clear(); // Construct two new providers, with either the same or different prefixes. - TestProvider* providerA = new TestProvider(num_results_per_provider, + TestProvider* providerA = new TestProvider(kResultsPerProvider, ASCIIToUTF16("http://a")); providerA->AddRef(); providers_.push_back(providerA); - TestProvider* providerB = new TestProvider(num_results_per_provider * 2, + TestProvider* providerB = new TestProvider(kResultsPerProvider * 2, same_destinations ? ASCIIToUTF16("http://a") : ASCIIToUTF16("http://b")); providerB->AddRef(); providers_.push_back(providerB); @@ -201,19 +213,84 @@ void AutocompleteProviderTest:: search_provider->AddRef(); providers_.push_back(search_provider); + controller_.reset(new AutocompleteController(providers_, &profile_)); +} + +void AutocompleteProviderTest:: + ResetControllerWithKeywordProvider() { + profile_.CreateTemplateURLService(); + + TemplateURLService* turl_model = + TemplateURLServiceFactory::GetForProfile(&profile_); + + // Create a TemplateURL for KeywordProvider. + TemplateURL* keyword_t_url = new TemplateURL(); + keyword_t_url->set_short_name(ASCIIToUTF16("foo.com")); + keyword_t_url->set_keyword(ASCIIToUTF16("foo.com")); + keyword_t_url->SetURL("http://foo.com/{searchTerms}", 0, 0); + turl_model->Add(keyword_t_url); + ASSERT_NE(0, keyword_t_url->id()); + + // Create another TemplateURL for KeywordProvider. + keyword_t_url = new TemplateURL(); + keyword_t_url->set_short_name(ASCIIToUTF16("bar.com")); + keyword_t_url->set_keyword(ASCIIToUTF16("bar.com")); + keyword_t_url->SetURL("http://bar.com/{searchTerms}", 0, 0); + turl_model->Add(keyword_t_url); + ASSERT_NE(0, keyword_t_url->id()); + + // Forget about any existing providers. The controller owns them and will + // Release() them below, when we delete it during the call to reset(). + providers_.clear(); + + // Create both a keyword and search provider, and add them in that order. + // (Order is important; see comments in RunExactKeymatchTest().) + KeywordProvider* keyword_provider = new KeywordProvider(NULL, + &profile_); + keyword_provider->AddRef(); + providers_.push_back(keyword_provider); + AutocompleteController* controller = new AutocompleteController(providers_, &profile_); + controller->set_keyword_provider(keyword_provider); controller_.reset(controller); } void AutocompleteProviderTest::RunTest() { + RunQuery(ASCIIToUTF16("a")); +} + +void AutocompleteProviderTest::RunRedundantKeywordTest( + const KeywordTestData* match_data, + size_t size) { + ACMatches matches; + for (size_t i = 0; i < size; ++i) { + AutocompleteMatch match; + match.fill_into_edit = match_data[i].fill_into_edit; + match.keyword = match_data[i].keyword; + matches.push_back(match); + } + + AutocompleteResult result; + result.AppendMatches(matches); + controller_->UpdateAssociatedKeywords(&result); + + for (size_t j = 0; j < result.size(); ++j) { + EXPECT_EQ(match_data[j].expected_keyword_result, + result.match_at(j).associated_keyword.get() != NULL); + } +} + + +void AutocompleteProviderTest::RunQuery(const string16 query) { result_.Reset(); - controller_->Start(ASCIIToUTF16("a"), string16(), true, false, true, - AutocompleteInput::ALL_MATCHES); + controller_->Start(query, string16(), true, false, true, + AutocompleteInput::ALL_MATCHES); - // The message loop will terminate when all autocomplete input has been - // collected. - MessageLoop::current()->Run(); + if (!controller_->done()) + // The message loop will terminate when all autocomplete input has been + // collected. + MessageLoop::current()->Run(); } void AutocompleteProviderTest::RunExactKeymatchTest( @@ -250,7 +327,7 @@ TEST_F(AutocompleteProviderTest, Query) { // Make sure the default match gets set to the highest relevance match. The // highest relevance matches should come from the second provider. - EXPECT_EQ(num_results_per_provider * 2, result_.size()); // two providers + EXPECT_EQ(kResultsPerProvider * 2, result_.size()); // two providers ASSERT_NE(result_.end(), result_.default_match()); EXPECT_EQ(providers_[1], result_.default_match()->provider); } @@ -261,7 +338,7 @@ TEST_F(AutocompleteProviderTest, RemoveDuplicates) { // Make sure all the first provider's results were eliminated by the second // provider's. - EXPECT_EQ(num_results_per_provider, result_.size()); + EXPECT_EQ(kResultsPerProvider, result_.size()); for (AutocompleteResult::const_iterator i(result_.begin()); i != result_.end(); ++i) EXPECT_EQ(providers_[1], i->provider); @@ -273,6 +350,48 @@ TEST_F(AutocompleteProviderTest, AllowExactKeywordMatch) { RunExactKeymatchTest(false); } +// Test that redundant associated keywords are removed. +TEST_F(AutocompleteProviderTest, RedundantKeywordsIgnoredInResult) { + ResetControllerWithKeywordProvider(); + + // Get the controller's internal members in the correct state. + RunQuery(ASCIIToUTF16("fo")); + + { + KeywordTestData duplicate_url[] = { + { ASCIIToUTF16("fo"), string16(), false }, + { ASCIIToUTF16("foo.com"), string16(), true }, + { ASCIIToUTF16("foo.com"), string16(), false } + }; + + SCOPED_TRACE("Duplicate url"); + RunRedundantKeywordTest(duplicate_url, ARRAYSIZE_UNSAFE(duplicate_url)); + } + + { + KeywordTestData keyword_match[] = { + { ASCIIToUTF16("foo.com"), ASCIIToUTF16("foo.com"), false }, + { ASCIIToUTF16("foo.com"), string16(), false } + }; + + SCOPED_TRACE("Duplicate url with keyword match"); + RunRedundantKeywordTest(keyword_match, ARRAYSIZE_UNSAFE(keyword_match)); + } + + { + KeywordTestData multiple_keyword[] = { + { ASCIIToUTF16("fo"), string16(), false }, + { ASCIIToUTF16("foo.com"), string16(), true }, + { ASCIIToUTF16("foo.com"), string16(), false }, + { ASCIIToUTF16("bar.com"), string16(), true }, + }; + + SCOPED_TRACE("Duplicate url with multiple keywords"); + RunRedundantKeywordTest(multiple_keyword, + ARRAYSIZE_UNSAFE(multiple_keyword)); + } +} + typedef testing::Test AutocompleteTest; TEST_F(AutocompleteTest, InputType) { @@ -484,5 +603,3 @@ TEST(AutocompleteInput, ParseForEmphasizeComponent) { EXPECT_EQ(input_cases[i].host.len, host.len); } } - -} // namespace diff --git a/chrome/browser/autocomplete/history_url_provider_unittest.cc b/chrome/browser/autocomplete/history_url_provider_unittest.cc index 97427dd..75abc1d 100644 --- a/chrome/browser/autocomplete/history_url_provider_unittest.cc +++ b/chrome/browser/autocomplete/history_url_provider_unittest.cc @@ -1,4 +1,4 @@ -// Copyright (c) 2011 The Chromium Authors. All rights reserved. +// Copyright (c) 2012 The Chromium Authors. All rights reserved. // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. @@ -232,6 +232,8 @@ void HistoryURLProviderTest::RunTest(const string16 text, matches_ = autocomplete_->matches(); if (sort_matches_) { + for (ACMatches::iterator i = matches_.begin(); i != matches_.end(); ++i) + i->ComputeStrippedDestinationURL(); std::sort(matches_.begin(), matches_.end(), &AutocompleteMatch::DestinationSortFunc); matches_.erase(std::unique(matches_.begin(), matches_.end(), diff --git a/chrome/browser/autocomplete/keyword_provider.cc b/chrome/browser/autocomplete/keyword_provider.cc index a88e97d..527e48a 100644 --- a/chrome/browser/autocomplete/keyword_provider.cc +++ b/chrome/browser/autocomplete/keyword_provider.cc @@ -1,4 +1,4 @@ -// Copyright (c) 2011 The Chromium Authors. All rights reserved. +// Copyright (c) 2012 The Chromium Authors. All rights reserved. // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. @@ -42,21 +42,6 @@ class KeywordProvider::ScopedEndExtensionKeywordMode { KeywordProvider* provider_; }; -// static -string16 KeywordProvider::SplitReplacementStringFromInput( - const string16& input, - bool trim_leading_whitespace) { - // The input may contain leading whitespace, strip it. - string16 trimmed_input; - TrimWhitespace(input, TRIM_LEADING, &trimmed_input); - - // And extract the replacement string. - string16 remaining_input; - SplitKeywordFromInput(trimmed_input, trim_leading_whitespace, - &remaining_input); - return remaining_input; -} - KeywordProvider::KeywordProvider(ACProviderListener* listener, Profile* profile) : AutocompleteProvider(listener, profile, "Keyword"), model_(NULL), @@ -107,6 +92,45 @@ static int global_input_uid_; } // namespace // static +string16 KeywordProvider::SplitKeywordFromInput( + const string16& input, + bool trim_leading_whitespace, + string16* remaining_input) { + // Find end of first token. The AutocompleteController has trimmed leading + // whitespace, so we need not skip over that. + const size_t first_white(input.find_first_of(kWhitespaceUTF16)); + DCHECK_NE(0U, first_white); + if (first_white == string16::npos) + return input; // Only one token provided. + + // Set |remaining_input| to everything after the first token. + DCHECK(remaining_input != NULL); + const size_t remaining_start = trim_leading_whitespace ? + input.find_first_not_of(kWhitespaceUTF16, first_white) : first_white + 1; + + if (remaining_start < input.length()) + remaining_input->assign(input.begin() + remaining_start, input.end()); + + // Return first token as keyword. + return input.substr(0, first_white); +} + +// static +string16 KeywordProvider::SplitReplacementStringFromInput( + const string16& input, + bool trim_leading_whitespace) { + // The input may contain leading whitespace, strip it. + string16 trimmed_input; + TrimWhitespace(input, TRIM_LEADING, &trimmed_input); + + // And extract the replacement string. + string16 remaining_input; + SplitKeywordFromInput(trimmed_input, trim_leading_whitespace, + &remaining_input); + return remaining_input; +} + +// static const TemplateURL* KeywordProvider::GetSubstitutingTemplateURLForInput( Profile* profile, const AutocompleteInput& input, @@ -128,6 +152,44 @@ const TemplateURL* KeywordProvider::GetSubstitutingTemplateURLForInput( return TemplateURL::SupportsReplacement(template_url) ? template_url : NULL; } +string16 KeywordProvider::GetKeywordForText( + const string16& text) const { + const string16 keyword(TemplateURLService::CleanUserInputKeyword(text)); + + if (keyword.empty()) + return keyword; + + TemplateURLService* url_service = GetTemplateURLService(); + if (!url_service) + return string16(); + + // Don't provide a keyword if it doesn't support replacement. + const TemplateURL* const template_url = + url_service->GetTemplateURLForKeyword(keyword); + if (!TemplateURL::SupportsReplacement(template_url)) + return string16(); + + // Don't provide a keyword for inactive/disabled extension keywords. + if (template_url->IsExtensionKeyword()) { + const Extension* extension = profile_->GetExtensionService()-> + GetExtensionById(template_url->GetExtensionId(), false); + if (!extension || + (profile_->IsOffTheRecord() && + !profile_->GetExtensionService()->IsIncognitoEnabled(extension->id()))) + return string16(); + } + + return keyword; +} + +AutocompleteMatch KeywordProvider::CreateAutocompleteMatch( + const string16& text, + const string16& keyword, + const AutocompleteInput& input) { + return CreateAutocompleteMatch(GetTemplateURLService(), keyword, input, + keyword.size(), SplitReplacementStringFromInput(text, true), 0); +} + void KeywordProvider::Start(const AutocompleteInput& input, bool minimal_changes) { // This object ensures we end keyword mode if we exit the function without @@ -161,14 +223,7 @@ void KeywordProvider::Start(const AutocompleteInput& input, if (!ExtractKeywordFromInput(input, &keyword, &remaining_input)) return; - // Make sure the model is loaded. This is cheap and quickly bails out if - // the model is already loaded. - TemplateURLService* model = - profile_ ? - TemplateURLServiceFactory::GetForProfile(profile_) : - model_; - DCHECK(model); - model->Load(); + TemplateURLService* model = GetTemplateURLService(); // Get the best matches for this keyword. // @@ -185,11 +240,12 @@ void KeywordProvider::Start(const AutocompleteInput& input, !remaining_input.empty(), &keyword_matches); - // Prune any extension keywords that are disallowed in incognito mode (if - // we're incognito), or disabled. for (std::vector<string16>::iterator i(keyword_matches.begin()); i != keyword_matches.end(); ) { const TemplateURL* template_url(model->GetTemplateURLForKeyword(*i)); + + // Prune any extension keywords that are disallowed in incognito mode (if + // we're incognito), or disabled. if (profile_ && input.matches_requested() == AutocompleteInput::ALL_MATCHES && template_url->IsExtensionKeyword()) { @@ -204,6 +260,14 @@ void KeywordProvider::Start(const AutocompleteInput& input, continue; } } + + // Prune any substituting keywords if there is no substitution. + if (TemplateURL::SupportsReplacement(template_url) && + remaining_input.empty() && !input.allow_exact_keyword_match()) { + i = keyword_matches.erase(i); + continue; + } + ++i; } if (keyword_matches.empty()) @@ -294,30 +358,6 @@ bool KeywordProvider::ExtractKeywordFromInput(const AutocompleteInput& input, } // static -string16 KeywordProvider::SplitKeywordFromInput( - const string16& input, - bool trim_leading_whitespace, - string16* remaining_input) { - // Find end of first token. The AutocompleteController has trimmed leading - // whitespace, so we need not skip over that. - const size_t first_white(input.find_first_of(kWhitespaceUTF16)); - DCHECK_NE(0U, first_white); - if (first_white == string16::npos) - return input; // Only one token provided. - - // Set |remaining_input| to everything after the first token. - DCHECK(remaining_input != NULL); - const size_t remaining_start = trim_leading_whitespace ? - input.find_first_not_of(kWhitespaceUTF16, first_white) : first_white + 1; - - if (remaining_start < input.length()) - remaining_input->assign(input.begin() + remaining_start, input.end()); - - // Return first token as keyword. - return input.substr(0, first_white); -} - -// static void KeywordProvider::FillInURLAndContents( Profile* profile, const string16& remaining_input, @@ -415,36 +455,37 @@ AutocompleteMatch KeywordProvider::CreateAutocompleteMatch( supports_replacement, input.prefer_keyword(), input.allow_exact_keyword_match()); } - AutocompleteMatch result(this, relevance, false, + AutocompleteMatch match(this, relevance, false, supports_replacement ? AutocompleteMatch::SEARCH_OTHER_ENGINE : AutocompleteMatch::HISTORY_KEYWORD); - result.fill_into_edit.assign(keyword); + match.fill_into_edit.assign(keyword); if (!remaining_input.empty() || !keyword_complete || supports_replacement) - result.fill_into_edit.push_back(L' '); - result.fill_into_edit.append(remaining_input); + match.fill_into_edit.push_back(L' '); + match.fill_into_edit.append(remaining_input); // If we wanted to set |result.inline_autocomplete_offset| correctly, we'd // need CleanUserInputKeyword() to return the amount of adjustment it's made // to the user's input. Because right now inexact keyword matches can't score // more highly than a "what you typed" match from one of the other providers, // we just don't bother to do this, and leave inline autocompletion off. - result.inline_autocomplete_offset = string16::npos; + match.inline_autocomplete_offset = string16::npos; // Create destination URL and popup entry content by substituting user input // into keyword templates. - FillInURLAndContents(profile_, remaining_input, element, &result); + FillInURLAndContents(profile_, remaining_input, element, &match); - if (supports_replacement) - result.template_url = element; - result.transition = content::PAGE_TRANSITION_KEYWORD; + if (supports_replacement) { + match.template_url = element; + match.keyword = keyword; + } + match.transition = content::PAGE_TRANSITION_KEYWORD; - return result; + return match; } void KeywordProvider::Observe(int type, const content::NotificationSource& source, const content::NotificationDetails& details) { - TemplateURLService* model = - profile_ ? TemplateURLServiceFactory::GetForProfile(profile_) : model_; + TemplateURLService* model = GetTemplateURLService(); const AutocompleteInput& input = extension_suggest_last_input_; switch (type) { @@ -521,6 +562,16 @@ void KeywordProvider::Observe(int type, } } +TemplateURLService* KeywordProvider::GetTemplateURLService() const { + TemplateURLService* service = profile_ ? + TemplateURLServiceFactory::GetForProfile(profile_) : model_; + // Make sure the model is loaded. This is cheap and quickly bails out if + // the model is already loaded. + DCHECK(service); + service->Load(); + return service; +} + void KeywordProvider::EnterExtensionKeywordMode( const std::string& extension_id) { DCHECK(current_keyword_extension_id_.empty()); diff --git a/chrome/browser/autocomplete/keyword_provider.h b/chrome/browser/autocomplete/keyword_provider.h index 3867345..7f395c6 100644 --- a/chrome/browser/autocomplete/keyword_provider.h +++ b/chrome/browser/autocomplete/keyword_provider.h @@ -1,4 +1,4 @@ -// Copyright (c) 2011 The Chromium Authors. All rights reserved. +// Copyright (c) 2012 The Chromium Authors. All rights reserved. // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. // @@ -55,21 +55,39 @@ class KeywordProvider : public AutocompleteProvider, // For testing. KeywordProvider(ACProviderListener* listener, TemplateURLService* model); + // Extracts the next whitespace-delimited token from input and returns it. + // Sets |remaining_input| to everything after the first token (skipping over + // the first intervening whitespace). + // If |trim_leading_whitespace| is true then leading whitespace in + // |*remaining_input| will be trimmed. + static string16 SplitKeywordFromInput(const string16& input, + bool trim_leading_whitespace, + string16* remaining_input); + // Returns the replacement string from the user input. The replacement // string is the portion of the input that does not contain the keyword. // For example, the replacement string for "b blah" is blah. // If |trim_leading_whitespace| is true then leading whitespace in // replacement string will be trimmed. - static string16 SplitReplacementStringFromInput( - const string16& input, - bool trim_leading_whitespace); + static string16 SplitReplacementStringFromInput(const string16& input, + bool trim_leading_whitespace); // Returns the matching substituting keyword for |input|, or NULL if there // is no keyword for the specified input. static const TemplateURL* GetSubstitutingTemplateURLForInput( - Profile* profile, - const AutocompleteInput& input, - string16* remaining_input); + Profile* profile, + const AutocompleteInput& input, + string16* remaining_input); + + // If |text| corresponds (in the sense of + // TemplateURLModel::CleanUserInputKeyword()) to an enabled, substituting + // keyword, returns that keyword; returns the empty string otherwise. + string16 GetKeywordForText(const string16& text) const; + + // Creates a fully marked-up AutocompleteMatch for a specific keyword. + AutocompleteMatch CreateAutocompleteMatch(const string16& text, + const string16& keyword, + const AutocompleteInput& input); // AutocompleteProvider virtual void Start(const AutocompleteInput& input, @@ -93,22 +111,12 @@ class KeywordProvider : public AutocompleteProvider, string16* keyword, string16* remaining_input); - // Extracts the next whitespace-delimited token from input and returns it. - // Sets |remaining_input| to everything after the first token (skipping over - // the first intervening whitespace). - // If |trim_leading_whitespace| is true then leading whitespace in - // |*remaining_input| will be trimmed. - static string16 SplitKeywordFromInput(const string16& input, - bool trim_leading_whitespace, - string16* remaining_input); - // Fills in the "destination_url" and "contents" fields of |match| with the // provided user input and keyword data. - static void FillInURLAndContents( - Profile* profile, - const string16& remaining_input, - const TemplateURL* element, - AutocompleteMatch* match); + static void FillInURLAndContents(Profile* profile, + const string16& remaining_input, + const TemplateURL* element, + AutocompleteMatch* match); // Determines the relevance for some input, given its type, whether the user // typed the complete keyword, and whether the user is in "prefer keyword @@ -123,13 +131,12 @@ class KeywordProvider : public AutocompleteProvider, // Creates a fully marked-up AutocompleteMatch from the user's input. // If |relevance| is negative, calculate a relevance based on heuristics. - AutocompleteMatch CreateAutocompleteMatch( - TemplateURLService* model, - const string16& keyword, - const AutocompleteInput& input, - size_t prefix_length, - const string16& remaining_input, - int relevance); + AutocompleteMatch CreateAutocompleteMatch(TemplateURLService* model, + const string16& keyword, + const AutocompleteInput& input, + size_t prefix_length, + const string16& remaining_input, + int relevance); void EnterExtensionKeywordMode(const std::string& extension_id); void MaybeEndExtensionKeywordMode(); @@ -139,6 +146,8 @@ class KeywordProvider : public AutocompleteProvider, const content::NotificationSource& source, const content::NotificationDetails& details) OVERRIDE; + TemplateURLService* GetTemplateURLService() const; + // Model for the keywords. This is only non-null when testing, otherwise the // TemplateURLService from the Profile is used. TemplateURLService* model_; diff --git a/chrome/browser/autocomplete/keyword_provider_unittest.cc b/chrome/browser/autocomplete/keyword_provider_unittest.cc index 7a02a97..6bd439a 100644 --- a/chrome/browser/autocomplete/keyword_provider_unittest.cc +++ b/chrome/browser/autocomplete/keyword_provider_unittest.cc @@ -1,4 +1,4 @@ -// Copyright (c) 2011 The Chromium Authors. All rights reserved. +// Copyright (c) 2012 The Chromium Authors. All rights reserved. // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. @@ -216,3 +216,13 @@ TEST_F(KeywordProviderTest, RemoveKeyword) { model_->Remove(model_->GetTemplateURLForKeyword(ASCIIToUTF16("aaaa"))); ASSERT_TRUE(model_->GetTemplateURLForKeyword(ASCIIToUTF16("aaaa")) == NULL); } + +TEST_F(KeywordProviderTest, GetKeywordForInput) { + EXPECT_EQ(ASCIIToUTF16("aa"), + kw_provider_->GetKeywordForText(ASCIIToUTF16("aa"))); + EXPECT_EQ(string16(), + kw_provider_->GetKeywordForText(ASCIIToUTF16("aafoo"))); + EXPECT_EQ(string16(), + kw_provider_->GetKeywordForText(ASCIIToUTF16("aa foo"))); +} + diff --git a/chrome/browser/autocomplete/search_provider.cc b/chrome/browser/autocomplete/search_provider.cc index 520820f..8f79258d 100644 --- a/chrome/browser/autocomplete/search_provider.cc +++ b/chrome/browser/autocomplete/search_provider.cc @@ -874,7 +874,9 @@ void SearchProvider::AddMatchToMap(const string16& query_string, if (is_keyword) { match.fill_into_edit.append( providers_.keyword_provider().keyword() + char16(' ')); - search_start += providers_.keyword_provider().keyword().size() + 1; + + match.keyword = providers_.keyword_provider().keyword(); + search_start += match.keyword.size() + 1; } match.fill_into_edit.append(query_string); // Not all suggestions start with the original input. diff --git a/chrome/browser/ui/cocoa/omnibox/omnibox_popup_view_mac.mm b/chrome/browser/ui/cocoa/omnibox/omnibox_popup_view_mac.mm index bc6dfc6..1a878ef 100644 --- a/chrome/browser/ui/cocoa/omnibox/omnibox_popup_view_mac.mm +++ b/chrome/browser/ui/cocoa/omnibox/omnibox_popup_view_mac.mm @@ -548,10 +548,8 @@ void OmniboxPopupViewMac::OpenURLForRow(int row, bool force_background) { // relevant match out to make sure it stays alive until the call // completes. AutocompleteMatch match = model_->result().match_at(row); - string16 keyword; - const bool is_keyword_hint = model_->GetKeywordForMatch(match, &keyword); omnibox_view_->OpenMatch(match, disposition, GURL(), row, - is_keyword_hint ? string16() : keyword); + match.keyword); } void OmniboxPopupViewMac::UserPressedOptIn(bool opt_in) { diff --git a/chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.h b/chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.h index eddcd6d..0265f84 100644 --- a/chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.h +++ b/chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.h @@ -1,4 +1,4 @@ -// Copyright (c) 2011 The Chromium Authors. All rights reserved. +// Copyright (c) 2012 The Chromium Authors. All rights reserved. // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. @@ -54,7 +54,9 @@ class OmniboxViewMac : public OmniboxView, const string16& display_text, bool update_popup) OVERRIDE; virtual void SetWindowTextAndCaretPos(const string16& text, - size_t caret_pos) OVERRIDE; + size_t caret_pos, + bool update_popup, + bool notify_text_changed) OVERRIDE; virtual void SetForcedQuery() OVERRIDE; virtual bool IsSelectAll() OVERRIDE; virtual bool DeleteAtEndPressed() OVERRIDE; @@ -164,6 +166,9 @@ class OmniboxViewMac : public OmniboxView, // though here we cannot really do the in-place operation they do. void EmphasizeURLComponents(); + // Internally invoked whenever the text changes in some way. + void TextChanged(); + // Calculates text attributes according to |display_text| and applies them // to the given |as| object. void ApplyTextAttributes(const string16& display_text, diff --git a/chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm b/chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm index 42592291..7e4cf9c 100644 --- a/chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm +++ b/chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm @@ -1,4 +1,4 @@ -// Copyright (c) 2011 The Chromium Authors. All rights reserved. +// Copyright (c) 2012 The Chromium Authors. All rights reserved. // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. @@ -329,11 +329,8 @@ void OmniboxViewMac::SetUserText(const string16& text, model_->SetUserText(text); // TODO(shess): TODO below from gtk. // TODO(deanm): something about selection / focus change here. - SetText(display_text); - if (update_popup) { - UpdatePopup(); - } - model_->OnChanged(); + SetWindowTextAndCaretPos(display_text, display_text.length(), update_popup, + true); } NSRange OmniboxViewMac::GetSelectedRange() const { @@ -364,9 +361,17 @@ void OmniboxViewMac::SetSelectedRange(const NSRange range) { } void OmniboxViewMac::SetWindowTextAndCaretPos(const string16& text, - size_t caret_pos) { + size_t caret_pos, + bool update_popup, + bool notify_text_changed) { DCHECK_LE(caret_pos, text.size()); SetTextAndSelectedRange(text, NSMakeRange(caret_pos, caret_pos)); + + if (update_popup) + UpdatePopup(); + + if (notify_text_changed) + TextChanged(); } void OmniboxViewMac::SetForcedQuery() { @@ -532,6 +537,11 @@ void OmniboxViewMac::EmphasizeURLComponents() { } } +void OmniboxViewMac::TextChanged() { + EmphasizeURLComponents(); + model_->OnChanged(); +} + void OmniboxViewMac::ApplyTextAttributes(const string16& display_text, NSMutableAttributedString* as) { NSUInteger as_length = [as length]; @@ -606,7 +616,7 @@ void OmniboxViewMac::OnTemporaryTextMaybeChanged(const string16& display_text, saved_temporary_selection_ = GetSelectedRange(); suggest_text_length_ = 0; - SetWindowTextAndCaretPos(display_text, display_text.size()); + SetWindowTextAndCaretPos(display_text, display_text.size(), false, false); model_->OnChanged(); [field_ clearUndoChain]; } @@ -698,8 +708,7 @@ bool OmniboxViewMac::OnAfterPossibleChange() { // Linux watches for something_changed && text_differs, but that // fails for us in case you copy the URL and paste the identical URL // back (we'll lose the styling). - EmphasizeURLComponents(); - model_->OnChanged(); + TextChanged(); delete_was_pressed_ = false; @@ -793,7 +802,7 @@ bool OmniboxViewMac::OnDoCommandBySelector(SEL cmd) { if (cmd == @selector(deleteForward:)) delete_was_pressed_ = true; - // Don't intercept up/down-arrow if the popup isn't open. + // Don't intercept up/down-arrow or backtab if the popup isn't open. if (popup_view_->IsOpen()) { if (cmd == @selector(moveDown:)) { model_->OnUpOrDownKeyPressed(1); @@ -804,6 +813,13 @@ bool OmniboxViewMac::OnDoCommandBySelector(SEL cmd) { model_->OnUpOrDownKeyPressed(-1); return true; } + + if (cmd == @selector(insertBacktab:) && + model_->popup_model()->selected_line_state() == + AutocompletePopupModel::KEYWORD) { + model_->ClearKeyword(GetText()); + return true; + } } if (cmd == @selector(moveRight:)) { @@ -829,26 +845,10 @@ bool OmniboxViewMac::OnDoCommandBySelector(SEL cmd) { return model_->OnEscapeKeyPressed(); } - if (cmd == @selector(insertTab:) || - cmd == @selector(insertTabIgnoringFieldEditor:)) { - if (model_->is_keyword_hint()) - return model_->AcceptKeyword(); - - if (suggest_text_length_ > 0) { - model_->CommitSuggestedText(true); - return true; - } - - if (!IsCaretAtEnd()) { - PlaceCaretAt(GetTextLength()); - // OnDidChange() will not be triggered when setting selected range in this - // method, so we need to call it explicitly. - OnDidChange(); - return true; - } - - if (model_->AcceptCurrentInstantPreview()) - return true; + if ((cmd == @selector(insertTab:) || + cmd == @selector(insertTabIgnoringFieldEditor:)) && + model_->is_keyword_hint()) { + return model_->AcceptKeyword(); } // |-noop:| is sent when the user presses Cmd+Return. Override the no-op diff --git a/chrome/browser/ui/gtk/omnibox/omnibox_popup_view_gtk.cc b/chrome/browser/ui/gtk/omnibox/omnibox_popup_view_gtk.cc index d26caf5..35f413f 100644 --- a/chrome/browser/ui/gtk/omnibox/omnibox_popup_view_gtk.cc +++ b/chrome/browser/ui/gtk/omnibox/omnibox_popup_view_gtk.cc @@ -483,10 +483,8 @@ void OmniboxPopupViewGtk::AcceptLine(size_t line, // extension, |match| and its contents. So copy the relevant match out to // make sure it stays alive until the call completes. AutocompleteMatch match = model_->result().match_at(line); - string16 keyword; - const bool is_keyword_hint = model_->GetKeywordForMatch(match, &keyword); omnibox_view_->OpenMatch(match, disposition, GURL(), line, - is_keyword_hint ? string16() : keyword); + match.keyword); } const gfx::Image* OmniboxPopupViewGtk::IconForMatch( diff --git a/chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc b/chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc index 45bdc81..8a18414 100644 --- a/chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc +++ b/chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc @@ -555,16 +555,22 @@ void OmniboxViewGtk::SetUserText(const string16& text, bool update_popup) { model_->SetUserText(text); // TODO(deanm): something about selection / focus change here. - SetWindowTextAndCaretPos(display_text, display_text.length()); - if (update_popup) - UpdatePopup(); - TextChanged(); + SetWindowTextAndCaretPos(display_text, display_text.length(), update_popup, + true); } void OmniboxViewGtk::SetWindowTextAndCaretPos(const string16& text, - size_t caret_pos) { + size_t caret_pos, + bool update_popup, + bool notify_text_changed) { CharRange range(static_cast<int>(caret_pos), static_cast<int>(caret_pos)); SetTextAndSelectedRange(text, range); + + if (update_popup) + UpdatePopup(); + + if (notify_text_changed) + TextChanged(); } void OmniboxViewGtk::SetForcedQuery() { @@ -639,7 +645,7 @@ void OmniboxViewGtk::OnTemporaryTextMaybeChanged( saved_temporary_selection_ = GetSelection(); StartUpdatingHighlightedText(); - SetWindowTextAndCaretPos(display_text, display_text.length()); + SetWindowTextAndCaretPos(display_text, display_text.length(), false, false); FinishUpdatingHighlightedText(); TextChanged(); } @@ -1077,7 +1083,8 @@ gboolean OmniboxViewGtk::HandleKeyPress(GtkWidget* widget, GdkEventKey* event) { // if IME did not handle it then "move-focus" signal will be emitted by the // default signal handler of |text_view_|. So we can intercept "move-focus" // signal of |text_view_| to know if a Tab key press event was handled by IME, - // and trigger Tab to search behavior when necessary in the signal handler. + // and trigger Tab to search or result traversal behavior when necessary in + // the signal handler. // // But for Enter key, if IME did not handle the key event, the default signal // handler will delete current selection range and insert '\n' and always @@ -1119,7 +1126,9 @@ gboolean OmniboxViewGtk::HandleKeyPress(GtkWidget* widget, GdkEventKey* event) { tab_was_pressed_ = (event->keyval == GDK_Tab || event->keyval == GDK_ISO_Left_Tab || event->keyval == GDK_KP_Tab) && - !(event->state & (GDK_CONTROL_MASK | GDK_SHIFT_MASK)); + !(event->state & GDK_CONTROL_MASK); + + shift_was_pressed_ = event->state & GDK_SHIFT_MASK; delete_was_pressed_ = event->keyval == GDK_Delete || event->keyval == GDK_KP_Delete; @@ -1740,8 +1749,18 @@ void OmniboxViewGtk::HandleViewMoveFocus(GtkWidget* widget, bool handled = false; // Trigger Tab to search behavior only when Tab key is pressed. - if (model_->is_keyword_hint()) + if (model_->is_keyword_hint() && !shift_was_pressed_) { handled = model_->AcceptKeyword(); + } else if (model_->popup_model()->IsOpen()) { + if (shift_was_pressed_ && + model_->popup_model()->selected_line_state() == + AutocompletePopupModel::KEYWORD) + model_->ClearKeyword(GetText()); + else + model_->OnUpOrDownKeyPressed(shift_was_pressed_ ? -1 : 1); + + handled = true; + } if (supports_pre_edit_ && !handled && !pre_edit_.empty()) handled = true; @@ -1749,15 +1768,6 @@ void OmniboxViewGtk::HandleViewMoveFocus(GtkWidget* widget, if (!handled && gtk_widget_get_visible(instant_view_)) handled = model_->CommitSuggestedText(true); - if (!handled) { - if (!IsCaretAtEnd()) { - OnBeforePossibleChange(); - PlaceCaretAt(GetTextLength()); - OnAfterPossibleChange(); - handled = true; - } - } - if (!handled) handled = model_->AcceptCurrentInstantPreview(); diff --git a/chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.h b/chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.h index c7dcc66..d639c11 100644 --- a/chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.h +++ b/chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.h @@ -106,7 +106,9 @@ class OmniboxViewGtk : public OmniboxView, const string16& display_text, bool update_popup) OVERRIDE; virtual void SetWindowTextAndCaretPos(const string16& text, - size_t caret_pos) OVERRIDE; + size_t caret_pos, + bool update_popup, + bool notify_text_changed) OVERRIDE; virtual void SetForcedQuery() OVERRIDE; virtual bool IsSelectAll() OVERRIDE; virtual bool DeleteAtEndPressed() OVERRIDE; @@ -461,6 +463,11 @@ class OmniboxViewGtk : public OmniboxView, // during sync dispatch of "move-focus" signal. bool tab_was_pressed_; + // Indicates if Shift key was pressed. + // Used in conjunction with the Tab key to determine if either traversal + // needs to move up the results or if the keyword needs to be cleared. + bool shift_was_pressed_; + // Indicates that user requested to paste clipboard. // The actual paste clipboard action might be performed later if the // clipboard is not empty. diff --git a/chrome/browser/ui/omnibox/omnibox_view.h b/chrome/browser/ui/omnibox/omnibox_view.h index a35f00e..234907f 100644 --- a/chrome/browser/ui/omnibox/omnibox_view.h +++ b/chrome/browser/ui/omnibox/omnibox_view.h @@ -1,4 +1,4 @@ -// Copyright (c) 2011 The Chromium Authors. All rights reserved. +// Copyright (c) 2012 The Chromium Authors. All rights reserved. // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. @@ -109,7 +109,9 @@ class OmniboxView { // Sets the window text and the caret position. virtual void SetWindowTextAndCaretPos(const string16& text, - size_t caret_pos) = 0; + size_t caret_pos, + bool update_popup, + bool notify_text_changed) = 0; // Sets the edit to forced query mode. Practically speaking, this means that // if the edit is not in forced query mode, its text is set to "?" with the diff --git a/chrome/browser/ui/omnibox/omnibox_view_browsertest.cc b/chrome/browser/ui/omnibox/omnibox_view_browsertest.cc index db2a2e1..13905f1 100644 --- a/chrome/browser/ui/omnibox/omnibox_view_browsertest.cc +++ b/chrome/browser/ui/omnibox/omnibox_view_browsertest.cc @@ -756,7 +756,7 @@ class OmniboxViewTest : public InProcessBrowserTest, // Keyword should also be accepted by typing an ideographic space. omnibox_view->OnBeforePossibleChange(); omnibox_view->SetWindowTextAndCaretPos(text + WideToUTF16(L"\x3000"), - text.length() + 1); + text.length() + 1, false, false); omnibox_view->OnAfterPossibleChange(); ASSERT_FALSE(omnibox_view->model()->is_keyword_hint()); ASSERT_EQ(text, omnibox_view->model()->keyword()); @@ -771,7 +771,7 @@ class OmniboxViewTest : public InProcessBrowserTest, // Keyword shouldn't be accepted by pressing space with a trailing // whitespace. omnibox_view->SetWindowTextAndCaretPos( - text + char16(' '), text.length() + 1); + text + char16(' '), text.length() + 1, false, false); ASSERT_NO_FATAL_FAILURE(SendKey(ui::VKEY_SPACE, 0)); ASSERT_TRUE(omnibox_view->model()->is_keyword_hint()); ASSERT_EQ(text, omnibox_view->model()->keyword()); @@ -808,7 +808,7 @@ class OmniboxViewTest : public InProcessBrowserTest, omnibox_view->OnBeforePossibleChange(); omnibox_view->model()->on_paste(); omnibox_view->SetWindowTextAndCaretPos(text + ASCIIToUTF16(" bar"), - text.length() + 4); + text.length() + 4, false, false); omnibox_view->OnAfterPossibleChange(); ASSERT_FALSE(omnibox_view->model()->is_keyword_hint()); ASSERT_TRUE(omnibox_view->model()->keyword().empty()); @@ -1022,59 +1022,137 @@ class OmniboxViewTest : public InProcessBrowserTest, ASSERT_TRUE(omnibox_view->IsSelectAll()); } - void TabMoveCursorToEndTest() { + void TabAcceptKeyword() { OmniboxView* omnibox_view = NULL; ASSERT_NO_FATAL_FAILURE(GetOmniboxView(&omnibox_view)); - omnibox_view->SetUserText(ASCIIToUTF16("Hello world")); + string16 text = ASCIIToUTF16(kSearchKeyword); + + // Trigger keyword hint mode. + ASSERT_NO_FATAL_FAILURE(SendKeySequence(kSearchKeywordKeys)); + ASSERT_TRUE(omnibox_view->model()->is_keyword_hint()); + ASSERT_EQ(text, omnibox_view->model()->keyword()); + ASSERT_EQ(text, omnibox_view->GetText()); + + // Trigger keyword mode by tab. + ASSERT_NO_FATAL_FAILURE(SendKey(ui::VKEY_TAB, 0)); + ASSERT_FALSE(omnibox_view->model()->is_keyword_hint()); + ASSERT_EQ(text, omnibox_view->model()->keyword()); + ASSERT_TRUE(omnibox_view->GetText().empty()); + + // Revert to keyword hint mode. + ASSERT_NO_FATAL_FAILURE(SendKey(ui::VKEY_BACK, 0)); + ASSERT_TRUE(omnibox_view->model()->is_keyword_hint()); + ASSERT_EQ(text, omnibox_view->model()->keyword()); + ASSERT_EQ(text, omnibox_view->GetText()); + + // The location bar should still have focus. + ASSERT_TRUE(ui_test_utils::IsViewFocused(browser(), + location_bar_focus_view_id_)); - // Move cursor to the beginning. + // Trigger keyword mode by tab. + ASSERT_NO_FATAL_FAILURE(SendKey(ui::VKEY_TAB, 0)); + ASSERT_FALSE(omnibox_view->model()->is_keyword_hint()); + ASSERT_EQ(text, omnibox_view->model()->keyword()); + ASSERT_TRUE(omnibox_view->GetText().empty()); + + // Revert to keyword hint mode with SHIFT+TAB. #if defined(OS_MACOSX) - // Home doesn't work on Mac trybot. - ASSERT_NO_FATAL_FAILURE(SendKey(ui::VKEY_A, ui::EF_CONTROL_DOWN)); + ASSERT_NO_FATAL_FAILURE(SendKey(ui::VKEY_BACKTAB, 0)); #else - ASSERT_NO_FATAL_FAILURE(SendKey(ui::VKEY_HOME, 0)); + ASSERT_NO_FATAL_FAILURE(SendKey(ui::VKEY_TAB, ui::EF_SHIFT_DOWN)); #endif + ASSERT_TRUE(omnibox_view->model()->is_keyword_hint()); + ASSERT_EQ(text, omnibox_view->model()->keyword()); + ASSERT_EQ(text, omnibox_view->GetText()); - size_t start, end; - omnibox_view->GetSelectionBounds(&start, &end); - EXPECT_EQ(0U, start); - EXPECT_EQ(0U, end); + ASSERT_TRUE(ui_test_utils::IsViewFocused(browser(), + location_bar_focus_view_id_)); + } + + void TabTraverseResultsTest() { + OmniboxView* omnibox_view = NULL; + ASSERT_NO_FATAL_FAILURE(GetOmniboxView(&omnibox_view)); + AutocompletePopupModel* popup_model = omnibox_view->model()->popup_model(); + ASSERT_TRUE(popup_model); + + // Input something to trigger results. + ASSERT_NO_FATAL_FAILURE(SendKeySequence(kDesiredTLDKeys)); + ASSERT_NO_FATAL_FAILURE(WaitForAutocompleteControllerDone()); + ASSERT_TRUE(popup_model->IsOpen()); - // Pressing tab should move cursor to the end. + size_t old_selected_line = popup_model->selected_line(); + EXPECT_EQ(0U, old_selected_line); + + // Move down the results. + for (size_t size = popup_model->result().size(); + popup_model->selected_line() < size - 1; + old_selected_line = popup_model->selected_line()) { + ASSERT_NO_FATAL_FAILURE(SendKey(ui::VKEY_TAB, 0)); + ASSERT_LT(old_selected_line, popup_model->selected_line()); + } + + // Don't move past the end. ASSERT_NO_FATAL_FAILURE(SendKey(ui::VKEY_TAB, 0)); + ASSERT_EQ(old_selected_line, popup_model->selected_line()); + ASSERT_TRUE(ui_test_utils::IsViewFocused(browser(), + location_bar_focus_view_id_)); - omnibox_view->GetSelectionBounds(&start, &end); - EXPECT_EQ(omnibox_view->GetText().size(), start); - EXPECT_EQ(omnibox_view->GetText().size(), end); + // Move back up the results. + for (; popup_model->selected_line() > 0U; + old_selected_line = popup_model->selected_line()) { + ASSERT_NO_FATAL_FAILURE(SendKey(ui::VKEY_TAB, ui::EF_SHIFT_DOWN)); + ASSERT_GT(old_selected_line, popup_model->selected_line()); + } - // The location bar should still have focus. + // Don't move past the beginning. + ASSERT_NO_FATAL_FAILURE(SendKey(ui::VKEY_TAB, ui::EF_SHIFT_DOWN)); + ASSERT_EQ(0U, popup_model->selected_line()); ASSERT_TRUE(ui_test_utils::IsViewFocused(browser(), location_bar_focus_view_id_)); - // Select all text. - omnibox_view->SelectAll(true); - EXPECT_TRUE(omnibox_view->IsSelectAll()); - omnibox_view->GetSelectionBounds(&start, &end); - EXPECT_EQ(0U, start); - EXPECT_EQ(omnibox_view->GetText().size(), end); + const TestHistoryEntry kHistoryFoo = { + "http://foo/", "Page foo", kSearchText, 1, 1, false + }; - // Pressing tab should move cursor to the end. - ASSERT_NO_FATAL_FAILURE(SendKey(ui::VKEY_TAB, 0)); + // Add a history entry so "foo" gets multiple matches. + ASSERT_NO_FATAL_FAILURE( + AddHistoryEntry(kHistoryFoo, Time::Now() - TimeDelta::FromHours(1))); - omnibox_view->GetSelectionBounds(&start, &end); - EXPECT_EQ(omnibox_view->GetText().size(), start); - EXPECT_EQ(omnibox_view->GetText().size(), end); + // Load results. + ASSERT_NO_FATAL_FAILURE(omnibox_view->SelectAll(false)); + ASSERT_NO_FATAL_FAILURE(SendKeySequence(kSearchKeywordKeys)); + ASSERT_NO_FATAL_FAILURE(WaitForAutocompleteControllerDone()); + + // Trigger keyword mode by tab. + string16 text = ASCIIToUTF16(kSearchKeyword); + ASSERT_NO_FATAL_FAILURE(SendKey(ui::VKEY_TAB, 0)); + ASSERT_FALSE(omnibox_view->model()->is_keyword_hint()); + ASSERT_EQ(text, omnibox_view->model()->keyword()); + ASSERT_TRUE(omnibox_view->GetText().empty()); // The location bar should still have focus. ASSERT_TRUE(ui_test_utils::IsViewFocused(browser(), location_bar_focus_view_id_)); - // Pressing tab when cursor is at the end should change focus. + // Pressing tab again should move to the next result and clear keyword + // mode. ASSERT_NO_FATAL_FAILURE(SendKey(ui::VKEY_TAB, 0)); + ASSERT_EQ(1U, omnibox_view->model()->popup_model()->selected_line()); + ASSERT_FALSE(omnibox_view->model()->is_keyword_hint()); + ASSERT_NE(text, omnibox_view->model()->keyword()); + + // The location bar should still have focus. + ASSERT_TRUE(ui_test_utils::IsViewFocused(browser(), + location_bar_focus_view_id_)); + + // Moving back up should not show keyword mode. + ASSERT_NO_FATAL_FAILURE(SendKey(ui::VKEY_TAB, ui::EF_SHIFT_DOWN)); + ASSERT_TRUE(omnibox_view->model()->is_keyword_hint()); + ASSERT_EQ(text, omnibox_view->model()->keyword()); - ASSERT_FALSE(ui_test_utils::IsViewFocused(browser(), - location_bar_focus_view_id_)); + ASSERT_TRUE(ui_test_utils::IsViewFocused(browser(), + location_bar_focus_view_id_)); } void PersistKeywordModeOnTabSwitch() { @@ -1195,10 +1273,17 @@ IN_PROC_BROWSER_TEST_F(OmniboxViewTest, DeleteItem) { DeleteItemTest(); } -IN_PROC_BROWSER_TEST_F(OmniboxViewTest, TabMoveCursorToEnd) { - TabMoveCursorToEndTest(); +IN_PROC_BROWSER_TEST_F(OmniboxViewTest, TabAcceptKeyword) { + TabAcceptKeyword(); } +#if !defined(OS_MACOSX) +// Mac intentionally does not support this behavior. +IN_PROC_BROWSER_TEST_F(OmniboxViewTest, TabTraverseResultsTest) { + TabTraverseResultsTest(); +} +#endif + IN_PROC_BROWSER_TEST_F(OmniboxViewTest, PersistKeywordModeOnTabSwitch) { PersistKeywordModeOnTabSwitch(); diff --git a/chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.cc b/chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.cc index 1851f5e..f7fb596 100644 --- a/chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.cc +++ b/chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.cc @@ -53,6 +53,7 @@ namespace { const SkAlpha kGlassPopupAlpha = 240; const SkAlpha kOpaquePopupAlpha = 255; + // The size delta between the font used for the edit and the result rows. Passed // to gfx::Font::DeriveFont. #if defined(OS_CHROMEOS) @@ -235,6 +236,13 @@ AutocompletePopupContentsView::AutocompletePopupContentsView( set_border(bubble_border); // The contents is owned by the LocationBarView. set_parent_owned(false); + + for (size_t i = 0; i < AutocompleteResult::kMaxMatches; ++i) { + AutocompleteResultView* result_view = + CreateResultView(this, i, result_font_, result_bold_font_); + result_view->SetVisible(false); + AddChildViewAt(result_view, static_cast<int>(i)); + } } AutocompletePopupContentsView::~AutocompletePopupContentsView() { @@ -280,7 +288,14 @@ bool AutocompletePopupContentsView::IsOpen() const { } void AutocompletePopupContentsView::InvalidateLine(size_t line) { - child_at(static_cast<int>(line))->SchedulePaint(); + AutocompleteResultView* result = static_cast<AutocompleteResultView*>( + child_at(static_cast<int>(line))); + result->Invalidate(); + + if (HasMatchAt(line) && GetMatchAtIndex(line).associated_keyword.get()) { + result->ShowKeyword(IsSelectedIndex(line) && + model_->selected_line_state() == AutocompletePopupModel::KEYWORD); + } } void AutocompletePopupContentsView::UpdatePopupAppearance() { @@ -288,6 +303,7 @@ void AutocompletePopupContentsView::UpdatePopupAppearance() { // No matches, close any existing popup. if (popup_ != NULL) { size_animation_.Stop(); + // NOTE: Do NOT use CloseNow() here, as we may be deep in a callstack // triggered by the popup receiving a message (e.g. LBUTTONUP), and // destroying the popup would cause us to read garbage when we unwind back @@ -305,19 +321,14 @@ void AutocompletePopupContentsView::UpdatePopupAppearance() { DCHECK_GT(child_rv_count, 0u); child_rv_count--; } - for (size_t i = 0; i < model_->result().size(); ++i) { - AutocompleteResultView* result_view; - if (i >= child_rv_count) { - result_view = - CreateResultView(this, i, result_font_, result_bold_font_); - AddChildViewAt(result_view, static_cast<int>(i)); - } else { - result_view = static_cast<AutocompleteResultView*>(child_at(i)); - result_view->SetVisible(true); - } - result_view->SetMatch(GetMatchAtIndex(i)); + const size_t result_size = model_->result().size(); + for (size_t i = 0; i < result_size; ++i) { + AutocompleteResultView* view = static_cast<AutocompleteResultView*>( + child_at(i)); + view->SetMatch(GetMatchAtIndex(i)); + view->SetVisible(true); } - for (size_t i = model_->result().size(); i < child_rv_count; ++i) + for (size_t i = result_size; i < child_rv_count; ++i) child_at(i)->SetVisible(false); PromoCounter* counter = profile_->GetInstantPromoCounter(); @@ -395,11 +406,11 @@ void AutocompletePopupContentsView::OnDragCanceled() { // AutocompletePopupContentsView, AutocompleteResultViewModel implementation: bool AutocompletePopupContentsView::IsSelectedIndex(size_t index) const { - return HasMatchAt(index) ? index == model_->selected_line() : false; + return index == model_->selected_line(); } bool AutocompletePopupContentsView::IsHoveredIndex(size_t index) const { - return HasMatchAt(index) ? index == model_->hovered_line() : false; + return index == model_->hovered_line(); } const SkBitmap* AutocompletePopupContentsView::GetIconIfExtensionMatch( @@ -435,7 +446,7 @@ void AutocompletePopupContentsView::Layout() { views::View* AutocompletePopupContentsView::GetEventHandlerForPoint( const gfx::Point& point) { - // If there is no opt in view, then we want all mouse events. Otherwise let + // If there is no opt in view then we want all mouse events. Otherwise, let // any descendants of the opt-in view get mouse events. if (!opt_in_view_) return this; @@ -646,10 +657,8 @@ void AutocompletePopupContentsView::OpenIndex( // extension, |match| and its contents. So copy the relevant match out to // make sure it stays alive until the call completes. AutocompleteMatch match = model_->result().match_at(index); - string16 keyword; - const bool is_keyword_hint = model_->GetKeywordForMatch(match, &keyword); omnibox_view_->OpenMatch(match, disposition, GURL(), index, - is_keyword_hint ? string16() : keyword); + match.keyword); } size_t AutocompletePopupContentsView::GetIndexForPoint( diff --git a/chrome/browser/ui/views/autocomplete/autocomplete_result_view.cc b/chrome/browser/ui/views/autocomplete/autocomplete_result_view.cc index 3009a1c..dc2ce4f 100644 --- a/chrome/browser/ui/views/autocomplete/autocomplete_result_view.cc +++ b/chrome/browser/ui/views/autocomplete/autocomplete_result_view.cc @@ -14,6 +14,7 @@ #include <algorithm> // NOLINT #include "base/i18n/bidi_line_iterator.h" +#include "chrome/browser/autocomplete/autocomplete_popup_model.h" #include "chrome/browser/ui/views/autocomplete/autocomplete_result_view_model.h" #include "chrome/browser/ui/views/location_bar/location_bar_view.h" #include "grit/generated_resources.h" @@ -112,13 +113,20 @@ AutocompleteResultView::AutocompleteResultView( normal_font_(font), bold_font_(bold_font), ellipsis_width_(font.GetStringWidth(string16(kEllipsis))), - mirroring_context_(new MirroringContext()) { + mirroring_context_(new MirroringContext()), + keyword_icon_(new views::ImageView()), + ALLOW_THIS_IN_INITIALIZER_LIST( + animation_(new ui::SlideAnimation(this))) { CHECK_GE(model_index, 0); if (default_icon_size_ == 0) { default_icon_size_ = ResourceBundle::GetSharedInstance().GetBitmapNamed( AutocompleteMatch::TypeToIcon(AutocompleteMatch::URL_WHAT_YOU_TYPED))-> width(); } + keyword_icon_->set_parent_owned(false); + keyword_icon_->EnableCanvasFlippingForRTLUI(true); + keyword_icon_->SetImage(GetKeywordIcon()); + keyword_icon_->SizeToPreferredSize(); } AutocompleteResultView::~AutocompleteResultView() { @@ -178,9 +186,38 @@ SkColor AutocompleteResultView::GetColor(ResultViewState state, void AutocompleteResultView::SetMatch(const AutocompleteMatch& match) { match_ = match; + animation_->Reset(); + + if (match.associated_keyword.get()) { + keyword_icon_->SetImage(GetKeywordIcon()); + + if (!keyword_icon_->parent()) + AddChildView(keyword_icon_.get()); + } else if (keyword_icon_->parent()) { + RemoveChildView(keyword_icon_.get()); + } + Layout(); } +void AutocompleteResultView::ShowKeyword(bool show_keyword) { + if (show_keyword) + animation_->Show(); + else + animation_->Hide(); +} + +void AutocompleteResultView::Invalidate() { + keyword_icon_->SetImage(GetKeywordIcon()); + SchedulePaint(); +} + +gfx::Size AutocompleteResultView::GetPreferredSize() { + return gfx::Size(0, std::max( + default_icon_size_ + (kMinimumIconVerticalPadding * 2), + GetTextHeight() + (kMinimumTextVerticalPadding * 2))); +} + //////////////////////////////////////////////////////////////////////////////// // AutocompleteResultView, protected: @@ -243,7 +280,7 @@ const SkBitmap* AutocompleteResultView::GetIcon() const { int icon = match_.starred ? IDR_OMNIBOX_STAR : AutocompleteMatch::TypeToIcon(match_.type); - if (model_->IsSelectedIndex(model_index_)) { + if (GetState() == SELECTED) { switch (icon) { case IDR_OMNIBOX_EXTENSION_APP: icon = IDR_OMNIBOX_EXTENSION_APP_SELECTED; @@ -268,6 +305,13 @@ const SkBitmap* AutocompleteResultView::GetIcon() const { return ResourceBundle::GetSharedInstance().GetBitmapNamed(icon); } +const SkBitmap* AutocompleteResultView::GetKeywordIcon() const { + // NOTE: If we ever begin returning icons of varying size, then callers need + // to ensure that |keyword_icon_| is resized each time its image is reset. + return ResourceBundle::GetSharedInstance().GetBitmapNamed( + (GetState() == SELECTED) ? IDR_OMNIBOX_TTS_SELECTED : IDR_OMNIBOX_TTS); +} + int AutocompleteResultView::DrawString( gfx::Canvas* canvas, const string16& text, @@ -498,14 +542,9 @@ void AutocompleteResultView::Elide(Runs* runs, int remaining_width) const { runs->clear(); } -gfx::Size AutocompleteResultView::GetPreferredSize() { - return gfx::Size(0, std::max( - default_icon_size_ + (kMinimumIconVerticalPadding * 2), - GetTextHeight() + (kMinimumTextVerticalPadding * 2))); -} - void AutocompleteResultView::Layout() { const SkBitmap* icon = GetIcon(); + icon_bounds_.SetRect(LocationBarView::kEdgeItemPadding + ((icon->width() == default_icon_size_) ? 0 : LocationBarView::kIconInternalPadding), @@ -514,9 +553,34 @@ void AutocompleteResultView::Layout() { int text_x = LocationBarView::kEdgeItemPadding + default_icon_size_ + LocationBarView::kItemPadding; int text_height = GetTextHeight(); + int text_width; + + if (match_.associated_keyword.get()) { + const int kw_collapsed_size = keyword_icon_->width() + + LocationBarView::kEdgeItemPadding; + const int max_kw_x = width() - kw_collapsed_size; + const int kw_x = animation_->CurrentValueBetween(max_kw_x, + LocationBarView::kEdgeItemPadding); + const int kw_text_x = kw_x + keyword_icon_->width() + + LocationBarView::kItemPadding; + + text_width = kw_x - text_x - LocationBarView::kItemPadding; + keyword_text_bounds_.SetRect(kw_text_x, 0, std::max( + width() - kw_text_x - LocationBarView::kEdgeItemPadding, 0), + text_height); + keyword_icon_->SetPosition(gfx::Point(kw_x, + (height() - keyword_icon_->height()) / 2)); + } else { + text_width = width() - text_x - LocationBarView::kEdgeItemPadding; + } + text_bounds_.SetRect(text_x, std::max(0, (height() - text_height) / 2), - std::max(bounds().width() - text_x - LocationBarView::kEdgeItemPadding, - 0), text_height); + std::max(text_width, 0), text_height); +} + +void AutocompleteResultView::OnBoundsChanged( + const gfx::Rect& previous_bounds) { + animation_->SetSlideDuration(width() / 4); } void AutocompleteResultView::OnPaint(gfx::Canvas* canvas) { @@ -524,12 +588,29 @@ void AutocompleteResultView::OnPaint(gfx::Canvas* canvas) { if (state != NORMAL) canvas->GetSkCanvas()->drawColor(GetColor(state, BACKGROUND)); - // Paint the icon. - canvas->DrawBitmapInt(*GetIcon(), GetMirroredXForRect(icon_bounds_), - icon_bounds_.y()); + if (!match_.associated_keyword.get() || + keyword_icon_->x() > icon_bounds_.right()) { + // Paint the icon. + canvas->DrawBitmapInt(*GetIcon(), GetMirroredXForRect(icon_bounds_), + icon_bounds_.y()); + + // Paint the text. + int x = GetMirroredXForRect(text_bounds_); + mirroring_context_->Initialize(x, text_bounds_.width()); + PaintMatch(canvas, match_, x); + } + + if (match_.associated_keyword.get()) { + // Paint the keyword text. + int x = GetMirroredXForRect(keyword_text_bounds_); + mirroring_context_->Initialize(x, keyword_text_bounds_.width()); + PaintMatch(canvas, *match_.associated_keyword.get(), x); + } +} - // Paint the text. - int x = GetMirroredXForRect(text_bounds_); - mirroring_context_->Initialize(x, text_bounds_.width()); - PaintMatch(canvas, match_, x); +void AutocompleteResultView::AnimationProgressed( + const ui::Animation* animation) { + Layout(); + SchedulePaint(); } + diff --git a/chrome/browser/ui/views/autocomplete/autocomplete_result_view.h b/chrome/browser/ui/views/autocomplete/autocomplete_result_view.h index 2c405929..7d79dfd 100644 --- a/chrome/browser/ui/views/autocomplete/autocomplete_result_view.h +++ b/chrome/browser/ui/views/autocomplete/autocomplete_result_view.h @@ -1,4 +1,4 @@ -// Copyright (c) 2011 The Chromium Authors. All rights reserved. +// Copyright (c) 2012 The Chromium Authors. All rights reserved. // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. @@ -8,16 +8,21 @@ #include "chrome/browser/autocomplete/autocomplete_match.h" #include "third_party/skia/include/core/SkColor.h" +#include "ui/base/animation/animation_delegate.h" +#include "ui/base/animation/slide_animation.h" #include "ui/gfx/font.h" #include "ui/gfx/rect.h" +#include "ui/views/controls/image_view.h" #include "ui/views/view.h" class AutocompleteResultViewModel; + namespace gfx { class Canvas; } -class AutocompleteResultView : public views::View { +class AutocompleteResultView : public views::View, + private ui::AnimationDelegate { public: enum ResultViewState { NORMAL = 0, @@ -47,6 +52,13 @@ class AutocompleteResultView : public views::View { // model has changed. void SetMatch(const AutocompleteMatch& match); + void ShowKeyword(bool show_keyword); + + void Invalidate(); + + // views::View: + virtual gfx::Size GetPreferredSize() OVERRIDE; + protected: virtual void PaintMatch(gfx::Canvas* canvas, const AutocompleteMatch& match, @@ -82,6 +94,7 @@ class AutocompleteResultView : public views::View { ResultViewState GetState() const; const SkBitmap* GetIcon() const; + const SkBitmap* GetKeywordIcon() const; // Elides |runs| to fit in |remaining_width|. The runs in |runs| should be in // logical order. @@ -100,10 +113,13 @@ class AutocompleteResultView : public views::View { void Elide(Runs* runs, int remaining_width) const; // views::View: - virtual gfx::Size GetPreferredSize() OVERRIDE; virtual void Layout() OVERRIDE; + virtual void OnBoundsChanged(const gfx::Rect& previous_bounds) OVERRIDE; virtual void OnPaint(gfx::Canvas* canvas) OVERRIDE; + // ui::AnimationDelegate: + virtual void AnimationProgressed(const ui::Animation* animation) OVERRIDE; + static int default_icon_size_; // This row's model and model index. @@ -125,6 +141,11 @@ class AutocompleteResultView : public views::View { gfx::Rect text_bounds_; gfx::Rect icon_bounds_; + gfx::Rect keyword_text_bounds_; + scoped_ptr<views::ImageView> keyword_icon_; + + scoped_ptr<ui::SlideAnimation> animation_; + DISALLOW_COPY_AND_ASSIGN(AutocompleteResultView); }; diff --git a/chrome/browser/ui/views/location_bar/location_bar_view.cc b/chrome/browser/ui/views/location_bar/location_bar_view.cc index fc64f36..0138a17 100644 --- a/chrome/browser/ui/views/location_bar/location_bar_view.cc +++ b/chrome/browser/ui/views/location_bar/location_bar_view.cc @@ -1017,10 +1017,9 @@ std::string LocationBarView::GetClassName() const { bool LocationBarView::SkipDefaultKeyEventProcessing( const views::KeyEvent& event) { #if defined(OS_WIN) - bool views_omnibox = views::Widget::IsPureViews(); if (views::FocusManager::IsTabTraversalKeyEvent(event)) { - if (HasValidSuggestText()) { - // Return true so that the edit sees the tab and commits the suggestion. + if (location_entry_->model()->popup_model()->IsOpen()) { + // Return true so that the edit sees the tab and moves the selection. return true; } if (keyword_hint_view_->visible() && !event.IsShiftDown()) { @@ -1028,12 +1027,6 @@ bool LocationBarView::SkipDefaultKeyEventProcessing( return true; } -#if !defined(USE_AURA) - // If the caret is not at the end, then tab moves the caret to the end. - if (!views_omnibox && !GetOmniboxViewWin()->IsCaretAtEnd()) - return true; -#endif - // Tab while showing instant commits instant immediately. // Return true so that focus traversal isn't attempted. The edit ends // up doing nothing in this case. @@ -1042,7 +1035,7 @@ bool LocationBarView::SkipDefaultKeyEventProcessing( } #if !defined(USE_AURA) - if (!views_omnibox) + if (!views::Widget::IsPureViews()) return GetOmniboxViewWin()->SkipDefaultKeyEventProcessing(event); #endif NOTIMPLEMENTED(); diff --git a/chrome/browser/ui/views/omnibox/omnibox_view_views.cc b/chrome/browser/ui/views/omnibox/omnibox_view_views.cc index b22f7d2f..21284d1 100644 --- a/chrome/browser/ui/views/omnibox/omnibox_view_views.cc +++ b/chrome/browser/ui/views/omnibox/omnibox_view_views.cc @@ -255,10 +255,18 @@ bool OmniboxViewViews::HandleAfterKeyEvent(const views::KeyEvent& event, handled = true; } else if (!handled && event.key_code() == ui::VKEY_TAB && - !event.IsShiftDown() && !event.IsControlDown()) { - if (model_->is_keyword_hint()) { + if (model_->is_keyword_hint() && !event.IsShiftDown()) { handled = model_->AcceptKeyword(); + } else if (model_->popup_model()->IsOpen()) { + if (event.IsShiftDown() && + model_->popup_model()->selected_line_state() == + AutocompletePopupModel::KEYWORD) { + model_->ClearKeyword(GetText()); + } else { + model_->OnUpOrDownKeyPressed(event.IsShiftDown() ? -1 : 1); + } + handled = true; } else { string16::size_type start = 0; string16::size_type end = 0; @@ -449,16 +457,22 @@ void OmniboxViewViews::SetUserText(const string16& text, const string16& display_text, bool update_popup) { model_->SetUserText(text); - SetWindowTextAndCaretPos(display_text, display_text.length()); - if (update_popup) - UpdatePopup(); - TextChanged(); + SetWindowTextAndCaretPos(display_text, display_text.length(), update_popup, + true); } void OmniboxViewViews::SetWindowTextAndCaretPos(const string16& text, - size_t caret_pos) { + size_t caret_pos, + bool update_popup, + bool notify_text_changed) { const ui::Range range(caret_pos, caret_pos); SetTextAndSelectedRange(text, range); + + if (update_popup) + UpdatePopup(); + + if (notify_text_changed) + TextChanged(); } void OmniboxViewViews::SetForcedQuery() { @@ -532,8 +546,7 @@ void OmniboxViewViews::OnTemporaryTextMaybeChanged( if (save_original_selection) textfield_->GetSelectedRange(&saved_temporary_selection_); - SetWindowTextAndCaretPos(display_text, display_text.length()); - TextChanged(); + SetWindowTextAndCaretPos(display_text, display_text.length(), false, true); } bool OmniboxViewViews::OnInlineAutocompleteTextMaybeChanged( diff --git a/chrome/browser/ui/views/omnibox/omnibox_view_views.h b/chrome/browser/ui/views/omnibox/omnibox_view_views.h index 11e6d0a..dc6f616 100644 --- a/chrome/browser/ui/views/omnibox/omnibox_view_views.h +++ b/chrome/browser/ui/views/omnibox/omnibox_view_views.h @@ -111,7 +111,9 @@ class OmniboxViewViews const string16& display_text, bool update_popup) OVERRIDE; virtual void SetWindowTextAndCaretPos(const string16& text, - size_t caret_pos) OVERRIDE; + size_t caret_pos, + bool update_popup, + bool notify_text_changed) OVERRIDE; virtual void SetForcedQuery() OVERRIDE; virtual bool IsSelectAll() OVERRIDE; virtual bool DeleteAtEndPressed() OVERRIDE; diff --git a/chrome/browser/ui/views/omnibox/omnibox_view_win.cc b/chrome/browser/ui/views/omnibox/omnibox_view_win.cc index 909bd8d..08fc235 100644 --- a/chrome/browser/ui/views/omnibox/omnibox_view_win.cc +++ b/chrome/browser/ui/views/omnibox/omnibox_view_win.cc @@ -689,16 +689,22 @@ void OmniboxViewWin::SetUserText(const string16& text, ScopedFreeze freeze(this, GetTextObjectModel()); model_->SetUserText(text); saved_selection_for_focus_change_.cpMin = -1; - SetWindowTextAndCaretPos(display_text, display_text.length()); - if (update_popup) - UpdatePopup(); - TextChanged(); + SetWindowTextAndCaretPos(display_text, display_text.length(), update_popup, + true); } void OmniboxViewWin::SetWindowTextAndCaretPos(const string16& text, - size_t caret_pos) { + size_t caret_pos, + bool update_popup, + bool notify_text_changed) { SetWindowText(text.c_str()); PlaceCaretAt(caret_pos); + + if (update_popup) + UpdatePopup(); + + if (notify_text_changed) + TextChanged(); } void OmniboxViewWin::SetForcedQuery() { @@ -738,9 +744,8 @@ void OmniboxViewWin::SelectAll(bool reversed) { void OmniboxViewWin::RevertAll() { ScopedFreeze freeze(this, GetTextObjectModel()); ClosePopup(); - model_->Revert(); saved_selection_for_focus_change_.cpMin = -1; - TextChanged(); + model_->Revert(); } void OmniboxViewWin::UpdatePopup() { @@ -835,8 +840,7 @@ void OmniboxViewWin::OnTemporaryTextMaybeChanged(const string16& display_text, // text and then arrowed to another entry with the same text, we'd still want // to move the caret. ScopedFreeze freeze(this, GetTextObjectModel()); - SetWindowTextAndCaretPos(display_text, display_text.length()); - TextChanged(); + SetWindowTextAndCaretPos(display_text, display_text.length(), false, true); } bool OmniboxViewWin::OnInlineAutocompleteTextMaybeChanged( @@ -2096,17 +2100,17 @@ bool OmniboxViewWin::OnKeyDownOnlyWritable(TCHAR key, } case VK_TAB: { - if (model_->is_keyword_hint()) { + const bool shift_pressed = GetKeyState(VK_SHIFT) < 0; + if (model_->is_keyword_hint() && !shift_pressed) { // Accept the keyword. ScopedFreeze freeze(this, GetTextObjectModel()); model_->AcceptKeyword(); - } else if (!IsCaretAtEnd()) { - ScopedFreeze freeze(this, GetTextObjectModel()); - OnBeforePossibleChange(); - PlaceCaretAt(GetTextLength()); - OnAfterPossibleChange(); + } else if (shift_pressed && + model_->popup_model()->selected_line_state() == + AutocompletePopupModel::KEYWORD) { + model_->ClearKeyword(GetText()); } else { - model_->CommitSuggestedText(true); + model_->OnUpOrDownKeyPressed(shift_pressed ? -count : count); } return true; } diff --git a/chrome/browser/ui/views/omnibox/omnibox_view_win.h b/chrome/browser/ui/views/omnibox/omnibox_view_win.h index 386179d..27ac288 100644 --- a/chrome/browser/ui/views/omnibox/omnibox_view_win.h +++ b/chrome/browser/ui/views/omnibox/omnibox_view_win.h @@ -110,7 +110,9 @@ class OmniboxViewWin const string16& display_text, bool update_popup) OVERRIDE; virtual void SetWindowTextAndCaretPos(const string16& text, - size_t caret_pos) OVERRIDE; + size_t caret_pos, + bool update_popup, + bool notify_text_changed) OVERRIDE; virtual void SetForcedQuery() OVERRIDE; virtual bool IsSelectAll() OVERRIDE; virtual bool DeleteAtEndPressed() OVERRIDE; diff --git a/chrome/chrome_tests.gypi b/chrome/chrome_tests.gypi index 7d84cc6..eda65a0 100644 --- a/chrome/chrome_tests.gypi +++ b/chrome/chrome_tests.gypi @@ -1266,7 +1266,6 @@ 'browser/about_flags_unittest.cc', 'browser/app_controller_mac_unittest.mm', 'browser/autocomplete/autocomplete_edit_unittest.cc', - 'browser/autocomplete/autocomplete_popup_model_unittest.cc', 'browser/autocomplete/autocomplete_result_unittest.cc', 'browser/autocomplete/autocomplete_unittest.cc', 'browser/autocomplete/builtin_provider_unittest.cc', diff --git a/ui/base/keycodes/keyboard_code_conversion_mac.mm b/ui/base/keycodes/keyboard_code_conversion_mac.mm index c990255..3d9e0a6 100644 --- a/ui/base/keycodes/keyboard_code_conversion_mac.mm +++ b/ui/base/keycodes/keyboard_code_conversion_mac.mm @@ -1,4 +1,4 @@ -// Copyright (c) 2011 The Chromium Authors. All rights reserved. +// Copyright (c) 2012 The Chromium Authors. All rights reserved. // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. @@ -31,6 +31,7 @@ bool operator<(const KeyCodeMap& a, const KeyCodeMap& b) { const KeyCodeMap kKeyCodesMap[] = { { VKEY_BACK /* 0x08 */, kVK_Delete, kBackspaceCharCode }, { VKEY_TAB /* 0x09 */, kVK_Tab, kTabCharCode }, + { VKEY_BACKTAB /* 0x0A */, 0x21E4, '\031' }, { VKEY_CLEAR /* 0x0C */, kVK_ANSI_KeypadClear, kClearCharCode }, { VKEY_RETURN /* 0x0D */, kVK_Return, kReturnCharCode }, { VKEY_SHIFT /* 0x10 */, kVK_Shift, 0 }, diff --git a/ui/base/keycodes/keyboard_codes_posix.h b/ui/base/keycodes/keyboard_codes_posix.h index 5534cc4..4d10edf 100644 --- a/ui/base/keycodes/keyboard_codes_posix.h +++ b/ui/base/keycodes/keyboard_codes_posix.h @@ -37,6 +37,7 @@ namespace ui { typedef enum { VKEY_BACK = 0x08, VKEY_TAB = 0x09, + VKEY_BACKTAB = 0x0A, VKEY_CLEAR = 0x0C, VKEY_RETURN = 0x0D, VKEY_SHIFT = 0x10, |