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

chrome/browser/ui/omnibox/omnibox_edit_model.cc

Index: chrome/browser/ui/omnibox/omnibox_edit_model.cc
diff --git a/chrome/browser/ui/omnibox/omnibox_edit_model.cc b/chrome/browser/ui/omnibox/omnibox_edit_model.cc
index 188611d51c8d59bf58b73013b81d2d9bb9549725..07b66aa28d5078f2a48f0651380216fccb9427ea 100644
--- a/chrome/browser/ui/omnibox/omnibox_edit_model.cc
+++ b/chrome/browser/ui/omnibox/omnibox_edit_model.cc
@@ -7,6 +7,7 @@
 #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"
@@ -1006,10 +1007,26 @@ bool OmniboxEditModel::OnAfterPossibleChange(const string16& old_text,
            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) {
@@ -1046,14 +1063,14 @@ void OmniboxEditModel::OnResultChanged(bool default_match_changed) {
   }
 
   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();
   }