From 394aa59248445297e01bf09acc40c2ff0fb34999 Mon Sep 17 00:00:00 2001
From: "shess@chromium.org"
<shess@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98>
Date: Mon, 11 May 2009 21:47:01 +0000
Subject: Use Chrome facilities for omnibox state save and restore on Mac.
TabContents has a facility for storing a bag of stuff across
current-tab changes. Wire up AutocompleteEditViewMac to use that
facility. Unfork some code in Browser::TabSelectedAt() so that the
new code gets used, and straighten up the Mac code along the code path
between there and AutocompleteEditViewMac.
This overall change also exposed a couple bugs/mis-features in the
AutocompleteEditViewMac code.
TEST=Text field maintains contents and selection across tab changes, even when edited.
Review URL: http://codereview.chromium.org/114017
git-svn-id: svn://svn.chromium.org/chrome/trunk/src@15790 0039d316-1c4b-4281-b951-d872f2087c98
---
.../autocomplete/autocomplete_edit_view_mac.h | 10 +-
.../autocomplete/autocomplete_edit_view_mac.mm | 131 ++++++++++++++++++---
chrome/browser/browser.cc | 4 -
chrome/browser/cocoa/browser_window_controller.mm | 3 +-
chrome/browser/cocoa/location_bar_view_mac.h | 5 +
chrome/browser/cocoa/location_bar_view_mac.mm | 6 +
chrome/browser/cocoa/toolbar_controller.h | 9 +-
chrome/browser/cocoa/toolbar_controller.mm | 25 +---
8 files changed, 142 insertions(+), 51 deletions(-)
diff --git a/chrome/browser/autocomplete/autocomplete_edit_view_mac.h b/chrome/browser/autocomplete/autocomplete_edit_view_mac.h
index 521f590..a693841 100644
--- a/chrome/browser/autocomplete/autocomplete_edit_view_mac.h
+++ b/chrome/browser/autocomplete/autocomplete_edit_view_mac.h
@@ -43,9 +43,7 @@ class AutocompleteEditViewMac : public AutocompleteEditView {
virtual const AutocompleteEditModel* model() const { return model_.get(); }
virtual void SaveStateToTab(TabContents* tab);
- virtual void Update(const TabContents* tab_for_state_restoring) {
- NOTIMPLEMENTED();
- }
+ virtual void Update(const TabContents* tab_for_state_restoring);
virtual void OpenURL(const GURL& url,
WindowOpenDisposition disposition,
@@ -55,10 +53,12 @@ class AutocompleteEditViewMac : public AutocompleteEditView {
const std::wstring& keyword);
virtual std::wstring GetText() const;
- virtual void SetUserText(const std::wstring& text) { NOTIMPLEMENTED(); }
+ virtual void SetUserText(const std::wstring& text) {
+ SetUserText(text, text, true);
+ }
virtual void SetUserText(const std::wstring& text,
const std::wstring& display_text,
- bool update_popup) { NOTIMPLEMENTED(); }
+ bool update_popup);
virtual void SetWindowTextAndCaretPos(const std::wstring& text,
size_t caret_pos);
diff --git a/chrome/browser/autocomplete/autocomplete_edit_view_mac.mm b/chrome/browser/autocomplete/autocomplete_edit_view_mac.mm
index c9ca5fc..e1a2a53 100644
--- a/chrome/browser/autocomplete/autocomplete_edit_view_mac.mm
+++ b/chrome/browser/autocomplete/autocomplete_edit_view_mac.mm
@@ -8,6 +8,41 @@
#include "chrome/browser/autocomplete/autocomplete_edit.h"
#include "chrome/browser/autocomplete/autocomplete_popup_model.h"
#include "chrome/browser/autocomplete/autocomplete_popup_view_mac.h"
+#include "chrome/browser/tab_contents/tab_contents.h"
+
+namespace {
+
+// Store's the model and view state across tab switches.
+struct AutocompleteEditViewMacState {
+ AutocompleteEditViewMacState(const AutocompleteEditModel::State model_state,
+ const bool has_focus, const NSRange& selection)
+ : model_state(model_state),
+ has_focus(has_focus),
+ selection(selection) {
+ }
+
+ const AutocompleteEditModel::State model_state;
+ const bool has_focus;
+ const NSRange selection;
+};
+
+// Returns a lazily initialized property bag accessor for saving our
+// state in a TabContents.
+PropertyAccessor<AutocompleteEditViewMacState>* GetStateAccessor() {
+ static PropertyAccessor<AutocompleteEditViewMacState> state;
+ return &state;
+}
+
+// Accessors for storing and getting the state from the tab.
+void StoreStateToTab(TabContents* tab,
+ const AutocompleteEditViewMacState& state) {
+ GetStateAccessor()->SetProperty(tab->property_bag(), state);
+}
+const AutocompleteEditViewMacState* GetStateFromTab(const TabContents* tab) {
+ return GetStateAccessor()->GetProperty(tab->property_bag());
+}
+
+} // namespace
// Thin Obj-C bridge class that is the delegate of the omnibox field.
// It intercepts various control delegate methods and vectors them to
@@ -57,19 +92,68 @@ AutocompleteEditViewMac::~AutocompleteEditViewMac() {
[field_ setDelegate:nil];
}
-// TODO(shess): This is the minimal change which seems to unblock
-// getting the minimal Omnibox code checked in without making the
-// world worse. Browser::TabSelectedAt() calls this when the tab
-// changes, but that is only wired up for Windows. I do not yet
-// understand that code well enough to go for it. Once wired up, then
-// code can be removed at:
-// [TabContentsController defocusLocationBar]
-// [TabStripController selectTabWithContents:...]
void AutocompleteEditViewMac::SaveStateToTab(TabContents* tab) {
- // TODO(shess): Actually save the state to the tab area.
+ DCHECK(tab);
+
+ NSRange range;
+ if (model_->has_focus()) {
+ range = GetSelectedRange();
+ } else {
+ // If we are not focussed, there is no selection. Manufacture
+ // something reasonable in case it starts to matter in the future.
+ range = NSMakeRange(0, [[field_ stringValue] length]);
+ }
- // Drop the popup before we change to another tab.
- ClosePopup();
+ AutocompleteEditViewMacState state(model_->GetStateForTabSwitch(),
+ model_->has_focus(), range);
+ StoreStateToTab(tab, state);
+}
+
+void AutocompleteEditViewMac::Update(
+ const TabContents* tab_for_state_restoring) {
+ // TODO(shess): It seems like if the tab is non-NULL, then this code
+ // shouldn't need to be called at all. When coded that way, I find
+ // that the field isn't always updated correctly. Figure out why
+ // this is. Maybe this method should be refactored into more
+ // specific cases.
+ const std::wstring text = toolbar_model_->GetText();
+ const bool user_visible = model_->UpdatePermanentText(text);
+
+ if (tab_for_state_restoring) {
+ RevertAll();
+
+ const AutocompleteEditViewMacState* state =
+ GetStateFromTab(tab_for_state_restoring);
+ if (state) {
+ // Should restore the user's text via SetUserText().
+ model_->RestoreState(state->model_state);
+
+ // Restore user's selection.
+ // TODO(shess): The model_ does not restore the focus state. If
+ // field_ was in focus when we switched away, I presume it
+ // should be in focus when we switch back. Figure out if model_
+ // not restoring focus is an oversight, or intentional for some
+ // subtle reason.
+ if (state->has_focus) {
+ FocusLocation();
+ DCHECK([field_ currentEditor]);
+ [[field_ currentEditor] setSelectedRange:state->selection];
+ }
+ }
+ } else if (user_visible) {
+ // Restore everything to the baseline look.
+ RevertAll();
+ // TODO(shess): Figure out how this case is used, to make sure
+ // we're getting the selection and popup right.
+
+ } else {
+ // TODO(shess): Figure out how this case is used, to make sure
+ // we're getting the selection and popup right.
+ // UpdateAndStyleText() approximates the inner part of Revertall()
+ // which under GTK is called EmphasizeURLComponents(), and is
+ // expected to change when I start feeding in the styling code.
+ UpdateAndStyleText(text, 0);
+ }
}
void AutocompleteEditViewMac::OpenURL(const GURL& url,
@@ -97,6 +181,16 @@ std::wstring AutocompleteEditViewMac::GetText() const {
return base::SysNSStringToWide([field_ stringValue]);
}
+void AutocompleteEditViewMac::SetUserText(const std::wstring& text,
+ const std::wstring& display_text,
+ bool update_popup) {
+ model_->SetUserText(text);
+ UpdateAndStyleText(display_text, display_text.size());
+ if (update_popup) {
+ UpdatePopup();
+ }
+}
+
NSRange AutocompleteEditViewMac::GetSelectedRange() const {
DCHECK([field_ currentEditor]);
return [[field_ currentEditor] selectedRange];
@@ -119,7 +213,7 @@ void AutocompleteEditViewMac::RevertAll() {
model_->Revert();
std::wstring tt = GetText();
- UpdateAndStyleText(tt, tt.size());
+ UpdateAndStyleText(tt, 0);
controller_->OnChanged();
}
@@ -156,7 +250,7 @@ void AutocompleteEditViewMac::UpdateAndStyleText(
[as addAttribute:NSForegroundColorAttributeName
value:[NSColor greenColor]
range:NSMakeRange((NSInteger)parts.host.begin,
- (NSInteger)parts.host.end())];
+ (NSInteger)parts.host.len)];
}
// TODO(shess): GTK has this as a member var, figure out why.
@@ -175,7 +269,7 @@ void AutocompleteEditViewMac::UpdateAndStyleText(
}
[as addAttribute:NSForegroundColorAttributeName value:color
range:NSMakeRange((NSInteger)parts.scheme.begin,
- (NSInteger)parts.scheme.end())];
+ (NSInteger)parts.scheme.len)];
}
// TODO(shess): Check that this updates the model's sense of focus
@@ -183,7 +277,7 @@ void AutocompleteEditViewMac::UpdateAndStyleText(
[field_ setObjectValue:as];
if (![field_ currentEditor]) {
[field_ becomeFirstResponder];
- DCHECK_EQ(field_, [[field_ window] firstResponder]);
+ DCHECK_EQ([field_ currentEditor], [[field_ window] firstResponder]);
}
NSRange selected_range = NSMakeRange(user_text_length, [as length]);
@@ -281,7 +375,12 @@ void AutocompleteEditViewMac::AcceptInput(
}
void AutocompleteEditViewMac::FocusLocation() {
- [[field_ window] makeFirstResponder:field_];
+ // -makeFirstResponder: will select the entire field_. If we're
+ // already firstResponder, it's likely that we want to retain the
+ // current selection.
+ if (![field_ currentEditor]) {
+ [[field_ window] makeFirstResponder:field_];
+ }
}
@implementation AutocompleteFieldDelegate
diff --git a/chrome/browser/browser.cc b/chrome/browser/browser.cc
index 955a3af..79130d5 100644
--- a/chrome/browser/browser.cc
+++ b/chrome/browser/browser.cc
@@ -1590,13 +1590,9 @@ void Browser::TabSelectedAt(TabContents* old_contents,
ProcessPendingUIUpdates();
if (old_contents) {
-#if defined(OS_WIN)
// Save what the user's currently typing, so it can be restored when we
// switch back to this tab.
window_->GetLocationBar()->SaveStateToContents(old_contents);
-#else
- NOTIMPLEMENTED();
-#endif
}
// Propagate the profile to the location bar.
diff --git a/chrome/browser/cocoa/browser_window_controller.mm b/chrome/browser/cocoa/browser_window_controller.mm
index 08250f1..c0150d5f 100644
--- a/chrome/browser/cocoa/browser_window_controller.mm
+++ b/chrome/browser/cocoa/browser_window_controller.mm
@@ -290,7 +290,8 @@ willPositionSheet:(NSWindow *)sheet
- (void)updateToolbarWithContents:(TabContents*)tab
shouldRestoreState:(BOOL)shouldRestore {
- [toolbarController_ updateToolbarWithContents:shouldRestore ? tab : NULL];
+ [toolbarController_ updateToolbarWithContents:tab
+ shouldRestoreState:shouldRestore];
}
- (void)setStarredState:(BOOL)isStarred {
diff --git a/chrome/browser/cocoa/location_bar_view_mac.h b/chrome/browser/cocoa/location_bar_view_mac.h
index ee9cf26..12f32fa 100644
--- a/chrome/browser/cocoa/location_bar_view_mac.h
+++ b/chrome/browser/cocoa/location_bar_view_mac.h
@@ -46,6 +46,11 @@ class LocationBarViewMac : public AutocompleteEditController,
return edit_view_.get();
}
+ // Updates the location bar. Resets the bar's permanent text and
+ // security style, and if |should_restore_state| is true, restores
+ // saved state from the tab (for tab switching).
+ void Update(const TabContents* tab, bool should_restore_state);
+
virtual void OnAutocompleteAccept(const GURL& url,
WindowOpenDisposition disposition,
PageTransition::Type transition,
diff --git a/chrome/browser/cocoa/location_bar_view_mac.mm b/chrome/browser/cocoa/location_bar_view_mac.mm
index b8c521f..138a567 100644
--- a/chrome/browser/cocoa/location_bar_view_mac.mm
+++ b/chrome/browser/cocoa/location_bar_view_mac.mm
@@ -56,6 +56,12 @@ void LocationBarViewMac::SaveStateToContents(TabContents* contents) {
edit_view_->SaveStateToTab(contents);
}
+void LocationBarViewMac::Update(const TabContents* contents,
+ bool should_restore_state) {
+ // AutocompleteEditView restores state if the tab is non-NULL.
+ edit_view_->Update(should_restore_state ? contents : NULL);
+}
+
void LocationBarViewMac::OnAutocompleteAccept(const GURL& url,
WindowOpenDisposition disposition,
PageTransition::Type transition,
diff --git a/chrome/browser/cocoa/toolbar_controller.h b/chrome/browser/cocoa/toolbar_controller.h
index 8967aee..1909800 100644
--- a/chrome/browser/cocoa/toolbar_controller.h
+++ b/chrome/browser/cocoa/toolbar_controller.h
@@ -52,9 +52,12 @@ class ToolbarView;
// Make the location bar the first responder, if possible.
- (void)focusLocationBar;
-// Called when any url bar state changes. If |tabForRestoring| is non-NULL,
-// it points to a TabContents whose state we should restore.
-- (void)updateToolbarWithContents:(TabContents*)tabForRestoring;
+// Updates the toolbar (and transitively the location bar) with the states of
+// the specified |tab|. If |shouldRestore| is true, we're switching
+// (back?) to this tab and should restore any previous location bar state
+// (such as user editing) as well.
+- (void)updateToolbarWithContents:(TabContents*)tabForRestoring
+ shouldRestoreState:(BOOL)shouldRestore;
// Sets whether or not the current page in the frontmost tab is bookmarked.
- (void)setStarredState:(BOOL)isStarred;
diff --git a/chrome/browser/cocoa/toolbar_controller.mm b/chrome/browser/cocoa/toolbar_controller.mm
index d2a986e..0860a0b 100644
--- a/chrome/browser/cocoa/toolbar_controller.mm
+++ b/chrome/browser/cocoa/toolbar_controller.mm
@@ -48,7 +48,6 @@ static NSString* const kStarredImageName = @"starred";
[self initCommandStatus:commands_];
locationBarView_.reset(new LocationBarViewMac(locationBar_, commands_,
toolbarModel_, profile_));
- [locationBar_ setStringValue:@"http://dev.chromium.org"];
}
- (void)dealloc {
@@ -98,27 +97,9 @@ static NSString* const kStarredImageName = @"starred";
[starButton_ setEnabled:commands->IsCommandEnabled(IDC_STAR) ? YES : NO];
}
-- (void)updateToolbarWithContents:(TabContents*)tab {
- // TODO(pinkerton): there's a lot of ui code in autocomplete_edit.cc
- // that we'll want to duplicate. For now, just handle setting the text.
-
- // TODO(shess): This is the start of what pinkerton refers to.
- // Unfortunately, I'm going to need to spend some time wiring things
- // up. This call should suffice to close any open autocomplete
- // pulldown. It should also be the right thing to do to save and
- // restore state, but at this time it's not clear that this is the
- // right place, and tab is not the right parameter.
- if (locationBarView_.get()) {
- locationBarView_->SaveStateToContents(NULL);
- }
-
- // TODO(pinkerton): update the security lock icon and background color
-
- // TODO(shess): Determine whether this should happen via
- // locationBarView_, instead, in which case this class can
- // potentially lose the locationBar_ reference.
- NSString* urlString = base::SysWideToNSString(toolbarModel_->GetText());
- [locationBar_ setStringValue:urlString];
+- (void)updateToolbarWithContents:(TabContents*)tab
+ shouldRestoreState:(BOOL)shouldRestore {
+ locationBarView_->Update(tab, shouldRestore ? true : false);
}
- (void)setStarredState:(BOOL)isStarred {
--
cgit v1.1