diff options
-rw-r--r-- | chrome/browser/autocomplete/autocomplete.cc | 21 | ||||
-rw-r--r-- | chrome/browser/autocomplete/autocomplete.h | 10 | ||||
-rw-r--r-- | chrome/browser/autocomplete/autocomplete_edit.cc | 80 | ||||
-rw-r--r-- | chrome/browser/autocomplete/autocomplete_edit.h | 3 | ||||
-rw-r--r-- | chrome/browser/autocomplete/autocomplete_edit_view_browsertest.cc | 37 | ||||
-rw-r--r-- | chrome/browser/autocomplete/autocomplete_popup_model.cc | 43 | ||||
-rw-r--r-- | chrome/browser/autocomplete/autocomplete_popup_model.h | 15 | ||||
-rw-r--r-- | chrome/browser/autocomplete/autocomplete_unittest.cc | 2 | ||||
-rw-r--r-- | chrome/common/notification_type.h | 13 |
9 files changed, 111 insertions, 113 deletions
diff --git a/chrome/browser/autocomplete/autocomplete.cc b/chrome/browser/autocomplete/autocomplete.cc index bd8cf16..5f8f681 100644 --- a/chrome/browser/autocomplete/autocomplete.cc +++ b/chrome/browser/autocomplete/autocomplete.cc @@ -872,13 +872,9 @@ void AutocompleteController::Stop(bool clear_result) { done_ = true; if (clear_result && !result_.empty()) { result_.Reset(); - NotificationService::current()->Notify( - NotificationType::AUTOCOMPLETE_CONTROLLER_RESULT_UPDATED, - Source<AutocompleteController>(this), - Details<const AutocompleteResult>(&result_)); - // NOTE: We don't notify AUTOCOMPLETE_CONTROLLER_DEFAULT_MATCH_UPDATED since - // we're trying to only clear the popup, not touch the edit... this is all - // a mess and should be cleaned up :( + // NOTE: We pass in false since we're trying to only clear the popup, not + // touch the edit... this is all a mess and should be cleaned up :( + NotifyChanged(false); } } @@ -951,16 +947,7 @@ void AutocompleteController::NotifyChanged(bool notify_default_match) { NotificationService::current()->Notify( NotificationType::AUTOCOMPLETE_CONTROLLER_RESULT_UPDATED, Source<AutocompleteController>(this), - Details<const AutocompleteResult>(&result_)); - if (notify_default_match) { - // This notification must be sent after the other so the popup has time to - // update its state before the edit calls into it. - // TODO(pkasting): Eliminate this ordering requirement. - NotificationService::current()->Notify( - NotificationType::AUTOCOMPLETE_CONTROLLER_DEFAULT_MATCH_UPDATED, - Source<AutocompleteController>(this), - Details<const AutocompleteResult>(&result_)); - } + Details<bool>(¬ify_default_match)); } void AutocompleteController::CheckIfDone() { diff --git a/chrome/browser/autocomplete/autocomplete.h b/chrome/browser/autocomplete/autocomplete.h index ea6382c..148209f 100644 --- a/chrome/browser/autocomplete/autocomplete.h +++ b/chrome/browser/autocomplete/autocomplete.h @@ -615,12 +615,10 @@ class AutocompleteController : public ACProviderListener { // return matches which are synchronously available, which should mean that // all providers will be done immediately. // - // The controller will fire AUTOCOMPLETE_CONTROLLER_DEFAULT_MATCH_UPDATED from - // inside this call, and unless the query is stopped, will fire at least one - // (and perhaps more) AUTOCOMPLETE_CONTROLLER_RESULT_UPDATED later as more - // matches come in (even if the query completes synchronously). Listeners - // should use the result set provided in the accompanying Details object to - // update themselves. + // The controller will fire AUTOCOMPLETE_CONTROLLER_RESULT_UPDATED from + // inside this call at least once. If matches are available later on that + // result in changing the result set AUTOCOMPLETE_CONTROLLER_RESULT_UPDATED + // is sent again. void Start(const string16& text, const string16& desired_tld, bool prevent_inline_autocomplete, diff --git a/chrome/browser/autocomplete/autocomplete_edit.cc b/chrome/browser/autocomplete/autocomplete_edit.cc index d8681d7..5f3aeef 100644 --- a/chrome/browser/autocomplete/autocomplete_edit.cc +++ b/chrome/browser/autocomplete/autocomplete_edit.cc @@ -15,6 +15,7 @@ #include "chrome/browser/autocomplete/autocomplete_edit_view.h" #include "chrome/browser/autocomplete/autocomplete_match.h" #include "chrome/browser/autocomplete/autocomplete_popup_model.h" +#include "chrome/browser/autocomplete/autocomplete_popup_view.h" #include "chrome/browser/autocomplete/keyword_provider.h" #include "chrome/browser/browser_list.h" #include "chrome/browser/command_updater.h" @@ -82,7 +83,7 @@ AutocompleteEditModel::~AutocompleteEditModel() { void AutocompleteEditModel::SetPopupModel(AutocompletePopupModel* popup_model) { popup_ = popup_model; registrar_.Add(this, - NotificationType::AUTOCOMPLETE_CONTROLLER_DEFAULT_MATCH_UPDATED, + NotificationType::AUTOCOMPLETE_CONTROLLER_RESULT_UPDATED, Source<AutocompleteController>(popup_->autocomplete_controller())); } @@ -147,8 +148,6 @@ bool AutocompleteEditModel::UpdatePermanentText( void AutocompleteEditModel::SetUserText(const string16& text) { SetInputInProgress(true); - keyword_.clear(); - is_keyword_hint_ = false; InternalSetUserText(text); paste_state_ = NONE; has_temporary_text_ = false; @@ -688,14 +687,6 @@ void AutocompleteEditModel::PopupBoundsChangedTo(const gfx::Rect& bounds) { controller_->OnPopupBoundsChanged(bounds); } -void AutocompleteEditModel::OnPopupClosed() { - // Accepts the temporary text as the user text, because it makes little - // sense to have temporary text when the popup is closed. - InternalSetUserText(UserTextFromDisplayText(view_->GetText())); - has_temporary_text_ = false; - PopupBoundsChangedTo(gfx::Rect()); -} - // Return true if the suggestion type warrants a TCP/IP preconnection. // i.e., it is now highly likely that the user will select the related domain. static bool IsPreconnectable(AutocompleteMatch::Type type) { @@ -718,36 +709,53 @@ static bool IsPreconnectable(AutocompleteMatch::Type type) { void AutocompleteEditModel::Observe(NotificationType type, const NotificationSource& source, const NotificationDetails& details) { - DCHECK_EQ(NotificationType::AUTOCOMPLETE_CONTROLLER_DEFAULT_MATCH_UPDATED, + DCHECK_EQ(NotificationType::AUTOCOMPLETE_CONTROLLER_RESULT_UPDATED, type.value); - string16 inline_autocomplete_text; - string16 keyword; - bool is_keyword_hint = false; - const AutocompleteResult* result = - Details<const AutocompleteResult>(details).ptr(); - const AutocompleteResult::const_iterator match(result->default_match()); - if (match != result->end()) { - if ((match->inline_autocomplete_offset != string16::npos) && - (match->inline_autocomplete_offset < match->fill_into_edit.length())) { - inline_autocomplete_text = - match->fill_into_edit.substr(match->inline_autocomplete_offset); - } - - if (!match->destination_url.SchemeIs(chrome::kExtensionScheme)) { - // Warm up DNS Prefetch cache, or preconnect to a search service. - chrome_browser_net::AnticipateOmniboxUrl(match->destination_url, - IsPreconnectable(match->type)); + const bool was_open = popup_->view()->IsOpen(); + if (*(Details<bool>(details).ptr())) { + string16 inline_autocomplete_text; + string16 keyword; + bool is_keyword_hint = false; + const AutocompleteResult& result = + popup_->autocomplete_controller()->result(); + const AutocompleteResult::const_iterator match(result.default_match()); + if (match != result.end()) { + if ((match->inline_autocomplete_offset != string16::npos) && + (match->inline_autocomplete_offset < + match->fill_into_edit.length())) { + inline_autocomplete_text = + match->fill_into_edit.substr(match->inline_autocomplete_offset); + } + + if (!match->destination_url.SchemeIs(chrome::kExtensionScheme)) { + // Warm up DNS Prefetch cache, or preconnect to a search service. + chrome_browser_net::AnticipateOmniboxUrl(match->destination_url, + IsPreconnectable(match->type)); + } + + // We could prefetch the alternate nav URL, if any, but because there + // 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); } - - // We could prefetch the alternate nav URL, if any, but because there - // 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); + popup_->OnResultChanged(); + OnPopupDataChanged(inline_autocomplete_text, NULL, keyword, + is_keyword_hint); + } else { + popup_->OnResultChanged(); } - OnPopupDataChanged(inline_autocomplete_text, NULL, keyword, is_keyword_hint); + if (popup_->view()->IsOpen()) { + PopupBoundsChangedTo(popup_->view()->GetTargetBounds()); + } else if (was_open) { + // Accepts the temporary text as the user text, because it makes little + // sense to have temporary text when the popup is closed. + InternalSetUserText(UserTextFromDisplayText(view_->GetText())); + has_temporary_text_ = false; + PopupBoundsChangedTo(gfx::Rect()); + } } void AutocompleteEditModel::InternalSetUserText(const string16& text) { diff --git a/chrome/browser/autocomplete/autocomplete_edit.h b/chrome/browser/autocomplete/autocomplete_edit.h index b407b11..8a94be4 100644 --- a/chrome/browser/autocomplete/autocomplete_edit.h +++ b/chrome/browser/autocomplete/autocomplete_edit.h @@ -324,9 +324,6 @@ class AutocompleteEditModel : public NotificationObserver { // Invoked when the popup is going to change its bounds to |bounds|. void PopupBoundsChangedTo(const gfx::Rect& bounds); - // Called when the popup is closed. - void OnPopupClosed(); - private: enum PasteState { NONE, // Most recent edit was not a paste. diff --git a/chrome/browser/autocomplete/autocomplete_edit_view_browsertest.cc b/chrome/browser/autocomplete/autocomplete_edit_view_browsertest.cc index 0e9c8c2..0cb8fdf 100644 --- a/chrome/browser/autocomplete/autocomplete_edit_view_browsertest.cc +++ b/chrome/browser/autocomplete/autocomplete_edit_view_browsertest.cc @@ -994,6 +994,33 @@ class AutocompleteEditViewTest : public InProcessBrowserTest, ASSERT_FALSE(ui_test_utils::IsViewFocused(browser(), VIEW_ID_LOCATION_BAR)); } + + void PersistKeywordModeOnTabSwitch() { + AutocompleteEditView* edit_view = NULL; + ASSERT_NO_FATAL_FAILURE(GetAutocompleteEditView(&edit_view)); + + // Trigger keyword hint mode. + ASSERT_NO_FATAL_FAILURE(SendKeySequence(kSearchKeywordKeys)); + ASSERT_TRUE(edit_view->model()->is_keyword_hint()); + ASSERT_EQ(kSearchKeyword, UTF16ToUTF8(edit_view->model()->keyword())); + + // Trigger keyword mode. + ASSERT_NO_FATAL_FAILURE(SendKey(ui::VKEY_TAB, false, false, false)); + ASSERT_FALSE(edit_view->model()->is_keyword_hint()); + ASSERT_EQ(kSearchKeyword, UTF16ToUTF8(edit_view->model()->keyword())); + + // Input something as search text. + ASSERT_NO_FATAL_FAILURE(SendKeySequence(kSearchTextKeys)); + + // Create a new tab. + browser()->NewTab(); + + // Switch back to the first tab. + browser()->SelectTabContentsAt(0, true); + + // Make sure we're still in keyword mode. + ASSERT_EQ(kSearchKeyword, UTF16ToUTF8(edit_view->model()->keyword())); + } }; // Test if ctrl-* accelerators are workable in omnibox. @@ -1059,6 +1086,11 @@ IN_PROC_BROWSER_TEST_F(AutocompleteEditViewTest, TabMoveCursorToEnd) { TabMoveCursorToEndTest(); } +IN_PROC_BROWSER_TEST_F(AutocompleteEditViewTest, + PersistKeywordModeOnTabSwitch) { + PersistKeywordModeOnTabSwitch(); +} + #if defined(OS_LINUX) // TODO(oshima): enable these tests for views-implmentation when // these featuers are supported. @@ -1251,4 +1283,9 @@ IN_PROC_BROWSER_TEST_F(AutocompleteEditViewViewsTest, TabMoveCursorToEndTest(); } +IN_PROC_BROWSER_TEST_F(AutocompleteEditViewViewsTest, + PersistKeywordModeOnTabSwitch) { + PersistKeywordModeOnTabSwitch(); +} + #endif diff --git a/chrome/browser/autocomplete/autocomplete_popup_model.cc b/chrome/browser/autocomplete/autocomplete_popup_model.cc index 9e2d43b..6809d0c 100644 --- a/chrome/browser/autocomplete/autocomplete_popup_model.cc +++ b/chrome/browser/autocomplete/autocomplete_popup_model.cc @@ -18,8 +18,6 @@ #include "chrome/browser/profiles/profile.h" #include "chrome/browser/search_engines/template_url.h" #include "chrome/browser/search_engines/template_url_model.h" -#include "chrome/common/notification_details.h" -#include "chrome/common/notification_source.h" #include "ui/gfx/rect.h" /////////////////////////////////////////////////////////////////////////////// @@ -35,8 +33,6 @@ AutocompletePopupModel::AutocompletePopupModel( profile_(profile), hovered_line_(kNoMatch), selected_line_(kNoMatch) { - registrar_.Add(this, NotificationType::AUTOCOMPLETE_CONTROLLER_RESULT_UPDATED, - Source<AutocompleteController>(controller_.get())); } AutocompletePopupModel::~AutocompletePopupModel() { @@ -287,38 +283,27 @@ void AutocompletePopupModel::TryDeletingCurrentItem() { } } -void AutocompletePopupModel::Observe(NotificationType type, - const NotificationSource& source, - const NotificationDetails& details) { - DCHECK_EQ(NotificationType::AUTOCOMPLETE_CONTROLLER_RESULT_UPDATED, - type.value); +const SkBitmap* AutocompletePopupModel::GetSpecialIconForMatch( + const AutocompleteMatch& match) const { + if (!match.template_url || !match.template_url->IsExtensionKeyword()) + return NULL; - const AutocompleteResult* result = - Details<const AutocompleteResult>(details).ptr(); - selected_line_ = result->default_match() == result->end() ? - kNoMatch : static_cast<size_t>(result->default_match() - result->begin()); + return &profile_->GetExtensionService()->GetOmniboxPopupIcon( + match.template_url->GetExtensionId()); +} + +void AutocompletePopupModel::OnResultChanged() { + const AutocompleteResult& result = controller_->result(); + selected_line_ = result.default_match() == result.end() ? + kNoMatch : static_cast<size_t>(result.default_match() - result.begin()); // There had better not be a nonempty result set with no default match. - CHECK((selected_line_ != kNoMatch) || result->empty()); + CHECK((selected_line_ != kNoMatch) || result.empty()); manually_selected_match_.Clear(); // 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. - if ((hovered_line_ != kNoMatch) && (result->size() <= hovered_line_)) + if ((hovered_line_ != kNoMatch) && (result.size() <= hovered_line_)) SetHoveredLine(kNoMatch); - const bool was_open = view_->IsOpen(); view_->UpdatePopupAppearance(); - if (view_->IsOpen()) - edit_model_->PopupBoundsChangedTo(view_->GetTargetBounds()); - else if (was_open) - edit_model_->OnPopupClosed(); -} - -const SkBitmap* AutocompletePopupModel::GetSpecialIconForMatch( - const AutocompleteMatch& match) const { - if (!match.template_url || !match.template_url->IsExtensionKeyword()) - return NULL; - - return &profile_->GetExtensionService()->GetOmniboxPopupIcon( - match.template_url->GetExtensionId()); } diff --git a/chrome/browser/autocomplete/autocomplete_popup_model.h b/chrome/browser/autocomplete/autocomplete_popup_model.h index 37687a5..6402ff2 100644 --- a/chrome/browser/autocomplete/autocomplete_popup_model.h +++ b/chrome/browser/autocomplete/autocomplete_popup_model.h @@ -8,8 +8,6 @@ #include "base/scoped_ptr.h" #include "chrome/browser/autocomplete/autocomplete.h" -#include "chrome/common/notification_observer.h" -#include "chrome/common/notification_registrar.h" class AutocompleteEditModel; class AutocompleteEditView; @@ -18,7 +16,7 @@ class SkBitmap; class AutocompletePopupView; -class AutocompletePopupModel : public NotificationObserver { +class AutocompletePopupModel { public: AutocompletePopupModel(AutocompletePopupView* popup_view, AutocompleteEditModel* edit_model, @@ -128,23 +126,20 @@ class AutocompletePopupModel : public NotificationObserver { Profile* profile() const { return profile_; } + // Invoked from the edit model any time the result set of the controller + // changes. + void OnResultChanged(); + // 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; private: - // NotificationObserver - virtual void Observe(NotificationType type, - const NotificationSource& source, - const NotificationDetails& details); - AutocompletePopupView* view_; AutocompleteEditModel* edit_model_; scoped_ptr<AutocompleteController> controller_; - NotificationRegistrar registrar_; - // Profile for current tab. Profile* profile_; diff --git a/chrome/browser/autocomplete/autocomplete_unittest.cc b/chrome/browser/autocomplete/autocomplete_unittest.cc index 332fedf..93ce9da 100644 --- a/chrome/browser/autocomplete/autocomplete_unittest.cc +++ b/chrome/browser/autocomplete/autocomplete_unittest.cc @@ -226,7 +226,7 @@ void AutocompleteProviderTest::Observe(NotificationType type, const NotificationSource& source, const NotificationDetails& details) { if (controller_->done()) { - result_.CopyFrom(*(Details<const AutocompleteResult>(details).ptr())); + result_.CopyFrom(controller_->result()); MessageLoop::current()->Quit(); } } diff --git a/chrome/common/notification_type.h b/chrome/common/notification_type.h index 174138c..0c390939 100644 --- a/chrome/common/notification_type.h +++ b/chrome/common/notification_type.h @@ -784,19 +784,10 @@ class NotificationType { // Autocomplete ------------------------------------------------------------ - // Sent by the autocomplete controller at least once per query, each time - // new matches are available, subject to rate-limiting/coalescing to reduce - // the number of updates. The details hold the AutocompleteResult that - // observers should use if they want to see the updated matches. + // Sent by the autocomplete controller each time the result set updates. + // The details is a boolean indicating if the default match has changed. AUTOCOMPLETE_CONTROLLER_RESULT_UPDATED, - // Sent by the autocomplete controller immediately after synchronous matches - // become available, and thereafter at the same time that - // AUTOCOMPLETE_CONTROLLER_RESULT_UPDATED is sent. The details hold the - // AutocompleteResult that observers should use if they want to see the - // up-to-date matches. - AUTOCOMPLETE_CONTROLLER_DEFAULT_MATCH_UPDATED, - // This is sent when an item of the Omnibox popup is selected. The source // is the profile. OMNIBOX_OPENED_URL, |