Moving common code into OmniboxView from OmniboxView*

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

 // 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.
 
 #include "chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.h"
 
 #include <Carbon/Carbon.h>  // kVK_Return
 
 #include "base/property_bag.h"
 #include "base/string_util.h"
@@ skipping 121 matching lines @@
 NSImage* OmniboxViewMac::ImageForResource(int resource_id) {
   ResourceBundle& rb = ResourceBundle::GetSharedInstance();
   return rb.GetNativeImageNamed(resource_id);
 }
 
 OmniboxViewMac::OmniboxViewMac(OmniboxEditController* controller,
                                ToolbarModel* toolbar_model,
                                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),
       delete_at_end_pressed_(false),
       line_height_(0) {
   DCHECK(controller);
   DCHECK(toolbar_model);
   DCHECK(profile);
   DCHECK(command_updater);
   DCHECK(field);
@@ skipping 14 matching lines @@
   // Destroy popup view before this object in case it tries to call us
   // back in the destructor.  Likewise for destroying the model before
   // this object.
   popup_view_.reset();
   model_.reset();
 
   // Disconnect from |field_|, it outlives this object.
   [field_ setObserver:NULL];
 }
 
-OmniboxEditModel* OmniboxViewMac::model() {
-  return model_.get();
-}
-
-const OmniboxEditModel* OmniboxViewMac::model() const {
-  return model_.get();
-}
-
 void OmniboxViewMac::SaveStateToTab(WebContents* tab) {
   DCHECK(tab);
 
   const bool hasFocus = [field_ currentEditor] ? true : false;
 
   NSRange range;
   if (hasFocus) {
     range = GetSelectedRange();
   } else {
     // If we are not focussed, there is no selection.  Manufacture
@@ skipping 45 matching lines @@
     // TODO(shess): This corresponds to _win and _gtk, except those
     // guard it with a test for whether the security level changed.
     // But AFAICT, that can only change if the text changed, and that
     // code compares the toolbar_model_ security level with the local
     // security level.  Dig in and figure out why this isn't a no-op
     // that should go away.
     EmphasizeURLComponents();
   }
 }
 
-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];
 }
 
 NSRange OmniboxViewMac::GetMarkedRange() const {
   DCHECK([field_ currentEditor]);
   return [(NSTextView*)[field_ currentEditor] markedRange];
 }
 
 void OmniboxViewMac::SetSelectedRange(const NSRange range) {
@@ skipping 69 matching lines @@
   // TODO(shess): Figure out what |reversed| implies.  The gtk version
   // has it imply inverting the selection front to back, but I don't
   // even know if that makes sense for Mac.
 
   // TODO(shess): Verify that we should be stealing focus at this
   // point.
   SetSelectedRange(NSMakeRange(0, GetTextLength()));
 }
 
 void OmniboxViewMac::RevertAll() {
-  ClosePopup();
-  model_->Revert();
-  model_->OnChanged();
+  OmniboxView::RevertAll();
   [field_ clearUndoChain];
 }
 
 void OmniboxViewMac::UpdatePopup() {
   model_->SetInputInProgress(true);
   if (!model_->has_focus())
     return;
 
   // Comment copied from OmniboxViewWin::UpdatePopup():
   // Don't inline autocomplete when:
   //   * The user is deleting text
   //   * The caret/selection isn't at the end of the text
   //   * The user has just pasted in something that replaced all the text
   //   * The user is trying to compose something in an IME
   bool prevent_inline_autocomplete = IsImeComposing();
   NSTextView* editor = (NSTextView*)[field_ currentEditor];
   if (editor) {
     if (NSMaxRange([editor selectedRange]) <
         [[editor textStorage] length] - suggest_text_length_)
       prevent_inline_autocomplete = true;
   }
 
   model_->StartAutocomplete([editor selectedRange].length != 0,
                             prevent_inline_autocomplete);
 }
 
-void OmniboxViewMac::ClosePopup() {
-  model_->StopAutocomplete();
-}
-
 void OmniboxViewMac::SetFocus() {
 }
 
 void OmniboxViewMac::SetText(const string16& display_text) {
   // If we are setting the text directly, there cannot be any suggest text.
   suggest_text_length_ = 0;
   SetTextInternal(display_text);
 }
 
 void OmniboxViewMac::SetTextInternal(const string16& display_text) {
@@ skipping 62 matching lines @@
     [storage setAttributes:[NSDictionary dictionary]
                      range:NSMakeRange(0, [storage length])];
     ApplyTextAttributes(GetText(), storage);
 
     [storage endEditing];
   } else {
     SetText(GetText());
   }
 }
 
-void OmniboxViewMac::TextChanged() {
-  EmphasizeURLComponents();
-  model_->OnChanged();
-}
-
 void OmniboxViewMac::ApplyTextAttributes(const string16& display_text,
                                          NSMutableAttributedString* as) {
   NSUInteger as_length = [as length];
   NSRange as_entire_string = NSMakeRange(0, as_length);
 
   [as addAttribute:NSFontAttributeName value:GetFieldFont()
              range:as_entire_string];
 
   // A kinda hacky way to add breaking at periods. This is what Safari does.
   // This works for IDNs too, despite the "en_US".
@@ skipping 163 matching lines @@
 gfx::NativeView OmniboxViewMac::GetNativeView() const {
   return field_;
 }
 
 gfx::NativeView OmniboxViewMac::GetRelativeWindowForPopup() const {
   // Not used on mac.
   NOTREACHED();
   return NULL;
 }
 
-CommandUpdater* OmniboxViewMac::GetCommandUpdater() {
-  return command_updater_;
-}
-
 void OmniboxViewMac::SetInstantSuggestion(const string16& suggest_text,
                                           bool animate_to_complete) {
   NSString* text = GetNonSuggestTextSubstring();
   bool needs_update = (suggest_text_length_ > 0);
 
   // Append the new suggest text.
   suggest_text_length_ = suggest_text.length();
   if (suggest_text_length_ > 0) {
     text = [text stringByAppendingString:base::SysUTF16ToNSString(
                suggest_text)];
@@ skipping 257 matching lines @@
   // should be really cheap, but in this case we could probably make
   // things even cheaper by refactoring between the popup-placement
   // code and the matrix-population code.
   popup_view_->UpdatePopupAppearance();
   model_->PopupBoundsChangedTo(popup_view_->GetTargetBounds());
 
   // Give controller a chance to rearrange decorations.
   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()) {
     return false;
   }
 
   // Don't intercept if there is a selection, or the cursor isn't at
   // the leftmost position.
   const NSRange selection = GetSelectedRange();
   if (selection.length > 0 || selection.location > 0) {
@@ skipping 47 matching lines @@
     DCHECK_EQ([field_ currentEditor], [[field_ window] firstResponder]);
   }
 }
 
 // static
 NSFont* OmniboxViewMac::GetFieldFont() {
   ResourceBundle& rb = ResourceBundle::GetSharedInstance();
   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();
 }