Implement the Instant extended API startMargin, endMargin, and rtl properties and the onmarginchang…

chrome/browser/ui/omnibox/omnibox_edit_model.cc

 // 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/omnibox/omnibox_edit_model.h"
 
 #include <string>
 
 #include "base/format_macros.h"
+#include "base/i18n/rtl.h"
 #include "base/metrics/histogram.h"
 #include "base/string_util.h"
 #include "base/stringprintf.h"
 #include "base/utf_string_conversions.h"
 #include "chrome/app/chrome_command_ids.h"
 #include "chrome/browser/autocomplete/autocomplete_classifier.h"
 #include "chrome/browser/autocomplete/autocomplete_classifier_factory.h"
 #include "chrome/browser/autocomplete/autocomplete_input.h"
 #include "chrome/browser/autocomplete/autocomplete_log.h"
 #include "chrome/browser/autocomplete/autocomplete_provider.h"
@@ skipping 979 matching lines @@
   // determine what keyword, if any, is applicable.
   //
   // If MaybeAcceptKeywordBySpace() accepts the keyword and returns true, that
   // will have updated our state already, so in that case we don't also return
   // true from this function.
   return !(text_differs && allow_keyword_ui_change && !just_deleted_text &&
            no_selection && (selection_start == user_text_.length()) &&
            MaybeAcceptKeywordBySpace(user_text_));
 }
 
-void OmniboxEditModel::PopupBoundsChangedTo(const gfx::Rect& bounds) {
+void OmniboxEditModel::OnPopupBoundsChanged(const gfx::Rect& bounds) {
   InstantController* instant = controller_->GetInstant();
   if (instant)
-    instant->SetOmniboxBounds(bounds);
+    instant->SetPopupBounds(bounds);
+}
+
+void OmniboxEditModel::OnOmniboxBoundsChanged(const gfx::Rect& bounds) {
+  InstantController* instant = controller_->GetInstant();
+  if (instant) {
+    int start;
+    int end;
+    if (base::i18n::IsRTL()) {

[dhollowa, 2012/11/16 17:29:03]

A couple questions.  (1) Isn't this supposed to be the other way round?  (2) To
compute |end|, which is a width, I'd assume you'd need the containing bounds to
determine it correctly.

This naming is the source of the confusion I think.  We should change the
implementation and the spec to be something like |SetMarginSize(first_margin,
last_margin)| or something.

[samarth, 2012/11/16 22:08:26]

I'm also confused why this isn't the other way around.

Re (2), David: are the bounds reported by GetBoundsInScreen() not absolute?

[dhollowa, 2012/11/16 22:14:28]

Not sure, but consider: b = { x = 10, y = 20, w = 50, h = 60 },
then x is the "left margin" assuming 0 is the left edge of the container.  But
what is the right edge of the container.  I'd assume it is greater than x+w, and
that (x+w) - right_edge is the "right margin" we're looking for.  Or am I being
dense?

[melevin, 2012/11/16 22:17:14]

Oops, yes it is.
We can't change the naming because it is defined in the Searchbox API as
"startMargin" and "endMargin". However, I looked into using the container's
dimensions to calculate the width from the end edge and changed the code to use
that instead. So now we are actually passing around the start and end margin
size and the naming should make sense.

[melevin, 2012/11/16 22:34:43]

They are relative to the browser window, so x=0 y=0 refers to the top-left
corner of the browser window regardless of RTL status.

[samarth, 2012/11/19 21:42:44]

Ok, I get it now. I was misreading the API: I thought we were sending the start
and end coordinates relative to the start edge.

We can actually change the API naming and semantics since it's not published yet
(only exists at go/searchboxapi3). Let's quickly chat with David (Black) about
what makes the most sense here.

+      start = bounds.x();
+      end = bounds.x() + bounds.width();
+    } else {
+      start = bounds.x() + bounds.width();
+      end = bounds.x();
+    }
+    instant->SetMarginSize(start, end);
+  }
 }
 
 void OmniboxEditModel::OnResultChanged(bool default_match_changed) {
   const bool was_open = popup_->IsOpen();
   if (default_match_changed) {
     string16 inline_autocomplete_text;
     string16 keyword;
     bool is_keyword_hint = false;
     const AutocompleteResult& result = this->result();
     const AutocompleteResult::const_iterator match(result.default_match());
@@ skipping 16 matching lines @@
     }
 
     popup_->OnResultChanged();
     OnPopupDataChanged(inline_autocomplete_text, NULL, keyword,
                        is_keyword_hint);
   } else {
     popup_->OnResultChanged();
   }
 
   if (popup_->IsOpen()) {
-    PopupBoundsChangedTo(popup_->view()->GetTargetBounds());
+    OnPopupBoundsChanged(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;
     is_temporary_text_set_by_instant_ = false;
-    PopupBoundsChangedTo(gfx::Rect());
+    OnPopupBoundsChanged(gfx::Rect());
     NotifySearchTabHelper();
   }
 
   if (InstantController* instant = controller_->GetInstant())
     instant->HandleAutocompleteResults(*autocomplete_controller_->providers());
 }
 
 bool OmniboxEditModel::query_in_progress() const {
   return !autocomplete_controller_->done();
 }
@@ skipping 221 matching lines @@
 }
 
 void OmniboxEditModel::ClassifyStringForPasteAndGo(
     const string16& text,
     AutocompleteMatch* match,
     GURL* alternate_nav_url) const {
   DCHECK(match);
   AutocompleteClassifierFactory::GetForProfile(profile_)->Classify(text,
       string16(), false, false, match, alternate_nav_url);
 }