Moving common code into OmniboxView from OmniboxView*

chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm

Index: chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm
diff --git a/chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm b/chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm
index 9ed0c589b1a5109e4eadd68adae8f2f174ffda18..35bdd46269fb8f31c06d11eca4edfb5307809300 100644
--- a/chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm
+++ b/chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm
@@ -139,11 +139,8 @@ OmniboxViewMac::OmniboxViewMac(OmniboxEditController* controller,
                                Profile* profile,
                                CommandUpdater* command_updater,
                                AutocompleteTextField* field)
-    : model_(new OmniboxEditModel(this, controller, profile)),
+    : OmniboxView(profile, controller, toolbar_model, command_updater),
       popup_view_(new OmniboxPopupViewMac(this, model_.get(), field)),
-      controller_(controller),
-      toolbar_model_(toolbar_model),
-      command_updater_(command_updater),
       field_(field),
       suggest_text_length_(0),
       delete_was_pressed_(false),
@@ -178,14 +175,6 @@ OmniboxViewMac::~OmniboxViewMac() {
   [field_ setObserver:NULL];
 }
 
-OmniboxEditModel* OmniboxViewMac::model() {
-  return model_.get();
-}
-
-const OmniboxEditModel* OmniboxViewMac::model() const {
-  return model_.get();
-}
-
 void OmniboxViewMac::SaveStateToTab(WebContents* tab) {
   DCHECK(tab);
 
@@ -251,48 +240,10 @@ void OmniboxViewMac::Update(const WebContents* tab_for_state_restoring) {
   }
 }
 
-void OmniboxViewMac::OpenMatch(const AutocompleteMatch& match,
-                               WindowOpenDisposition disposition,
-                               const GURL& alternate_nav_url,
-                               size_t selected_line) {
-  // TODO(shess): Why is the caller passing an invalid url in the
-  // first place?  Make sure that case isn't being dropped on the
-  // floor.
-  if (!match.destination_url.is_valid()) {
-    return;
-  }
-
-  model_->OpenMatch(match, disposition, alternate_nav_url, selected_line);
-}
-
 string16 OmniboxViewMac::GetText() const {
   return base::SysNSStringToUTF16(GetNonSuggestTextSubstring());
 }
 
-bool OmniboxViewMac::IsEditingOrEmpty() const {
-  return model_->user_input_in_progress() || !GetTextLength();
-}
-
-int OmniboxViewMac::GetIcon() const {
-  return IsEditingOrEmpty() ?
-      AutocompleteMatch::TypeToIcon(model_->CurrentTextType()) :
-      toolbar_model_->GetIcon();
-}
-
-void OmniboxViewMac::SetUserText(const string16& text) {
-  SetUserText(text, text, true);
-}
-
-void OmniboxViewMac::SetUserText(const string16& text,
-                                 const string16& display_text,
-                                 bool update_popup) {
-  model_->SetUserText(text);
-  // TODO(shess): TODO below from gtk.
-  // TODO(deanm): something about selection / focus change here.
-  SetWindowTextAndCaretPos(display_text, display_text.length(), update_popup,
-      true);
-}
-
 NSRange OmniboxViewMac::GetSelectedRange() const {
   return [[field_ currentEditor] selectedRange];
 }
@@ -382,9 +333,7 @@ void OmniboxViewMac::SelectAll(bool reversed) {
 }
 
 void OmniboxViewMac::RevertAll() {
-  ClosePopup();
-  model_->Revert();
-  model_->OnChanged();
+  OmniboxView::RevertAll();
   [field_ clearUndoChain];
 }
 
@@ -411,10 +360,6 @@ void OmniboxViewMac::UpdatePopup() {
                             prevent_inline_autocomplete);
 }
 
-void OmniboxViewMac::ClosePopup() {
-  model_->StopAutocomplete();
-}
-
 void OmniboxViewMac::SetFocus() {
 }
 
@@ -497,11 +442,6 @@ void OmniboxViewMac::EmphasizeURLComponents() {
   }
 }
 
-void OmniboxViewMac::TextChanged() {
-  EmphasizeURLComponents();
-  model_->OnChanged();
-}
-
 void OmniboxViewMac::ApplyTextAttributes(const string16& display_text,
                                          NSMutableAttributedString* as) {
   NSUInteger as_length = [as length];
@@ -685,10 +625,6 @@ gfx::NativeView OmniboxViewMac::GetRelativeWindowForPopup() const {
   return NULL;
 }
 
-CommandUpdater* OmniboxViewMac::GetCommandUpdater() {
-  return command_updater_;
-}
-
 void OmniboxViewMac::SetInstantSuggestion(const string16& suggest_text,
                                           bool animate_to_complete) {
   NSString* text = GetNonSuggestTextSubstring();
@@ -966,6 +902,10 @@ void OmniboxViewMac::OnFrameChanged() {
   model_->OnChanged();
 }
 
+void OmniboxViewMac::ClosePopup() {
+  OmniboxView::ClosePopup();
+}
+
 bool OmniboxViewMac::OnBackspacePressed() {
   // Don't intercept if not in keyword search mode.
   if (model_->is_keyword_hint() || model_->keyword().empty()) {
@@ -1033,18 +973,18 @@ NSFont* OmniboxViewMac::GetFieldFont() {
   return rb.GetFont(ResourceBundle::BaseFont).GetNativeFont();
 }
 
+int OmniboxViewMac::GetOmniboxTextLength() const {
+  return static_cast<int>(GetTextLength());

[Peter Kasting, 2012/07/26 03:59:17]

Nit: For all these derived classes, I kind of feel like we should just have one
base-class GetTextLength() virtual, the derived classes should implement it
directly, and then call it directly -- instead of having both GetTextLength()
and GetOmniboxTextLength() in most.

If we use "int" as the return type for the base class function (as it is right
now), then most places in most derived classes don't need explicit casts -- I
think in this file, for example, maybe just the comparison in IsCaretAtEnd()
does?  (Not sure)

[dominich, 2012/07/26 22:33:24]

I originally built this however there are some problems:

Many of the users require unsigned (this is true for OSX NSUInteger and win
size_t). Changing the call-sites would have been static_cast spaghetti. I'm not
convinced this is better, but I couldn't come up with another plan.

Also, there's a subtlety in the win version where GetTextLength is actually
defined in the ATL rich text control that OmniboxViewWin inherits from. Hence
the need for the indirection and the rename to make it explicitly the Omnibox
version. Prior to this, the derived version was overriding both base classes on
win. Ugh.

[Peter Kasting, 2012/07/26 23:03:24]

My hope was that for most of those cases, if we were passing an unsigned value
to a function taking a signed argument, we didn't need an explicit cast.  But
I'm not sure.  I thought about suggesting that the base class version return
size_t -- which seems like the more correct type -- but it looked like much of
the existing usage of the size_t-returning function on Win was actually to
assign/compare to int :/.

It may still be slightly better on Mac to have the implementationn of this be
the only such function, and then cast its return as needed, but I'm not sure.
Ugh is right.  That means that if we named this GetTextLength, the Windows code
would need to explicitly qualify which base class it wanted to use.  That seems
unfortunate enough to make me think the route you've gone is better, perhaps
with a comment in the base class about avoiding this naming conflict.

+}
+
 NSUInteger OmniboxViewMac::GetTextLength() const {
   return ([field_ currentEditor] ?
           [[[field_ currentEditor] string] length] :
           [[field_ stringValue] length]) - suggest_text_length_;
 }
 
-void OmniboxViewMac::PlaceCaretAt(NSUInteger pos) {
-  DCHECK(pos <= GetTextLength());
-  SetSelectedRange(NSMakeRange(pos, pos));
-}
-
 bool OmniboxViewMac::IsCaretAtEnd() const {
   const NSRange selection = GetSelectedRange();
-  return selection.length == 0 && selection.location == GetTextLength();
+  return selection.length == 0 &&
+         selection.location == GetTextLength();
 }