GTK Keyboard Support for New Autofill

chrome/browser/autofill/autofill_popup_view.cc

Index: chrome/browser/autofill/autofill_popup_view.cc
diff --git a/chrome/browser/autofill/autofill_popup_view.cc b/chrome/browser/autofill/autofill_popup_view.cc
index 7e3ba5a3be0d87fca605378dd1c5b1d6857fa3ad..2d3056a0c485a85116b74ef30d7d4d7d3eaa6a1a 100644
--- a/chrome/browser/autofill/autofill_popup_view.cc
+++ b/chrome/browser/autofill/autofill_popup_view.cc
@@ -20,11 +20,14 @@ AutofillPopupView::AutofillPopupView(
   registrar_.Add(this,
                  content::NOTIFICATION_WEB_CONTENTS_HIDDEN,
                  content::Source<content::WebContents>(web_contents));
-  registrar_.Add(
-      this,
-      content::NOTIFICATION_NAV_ENTRY_COMMITTED,
-      content::Source<content::NavigationController>(
-          &(web_contents->GetController())));
+
+  if (web_contents) {

[Ilya Sherman, 2012/03/15 20:10:56]

nit: How about "if (!web_contents) return;" at the top of this method instead?

[csharp, 2012/03/21 15:32:28]

Done.

+    registrar_.Add(
+        this,
+        content::NOTIFICATION_NAV_ENTRY_COMMITTED,
+        content::Source<content::NavigationController>(
+            &(web_contents->GetController())));
+  }
 }
 
 AutofillPopupView::~AutofillPopupView() {}
@@ -69,6 +72,42 @@ void AutofillPopupView::SetSelectedLine(int selected_line) {
   }
 }
 
+void AutofillPopupView::IncreaseSelectedLine() {
+  int new_selected_line = selected_line_ + 1;
+
+  if (new_selected_line == static_cast<int>(autofill_values_.size()))
+    new_selected_line = -1;

[Ilya Sherman, 2012/03/15 20:10:56]

nit: Why -1 and not 0?

[csharp, 2012/03/21 15:32:28]

Because when you go off either edge you have no selection before starting at the
other side.

[Ilya Sherman, 2012/03/21 23:35:30]

Are you matching a system UI convention, or the existing WebKit popup, or
something else?  I would not expect to have no selection prior to starting at
the other side.

[csharp, 2012/03/23 15:15:19]

This is the current WebKit Autofill method.

[Ilya Sherman, 2012/03/23 22:31:13]

Ok.  Unless this matches a system convention, or makes sense for a different
reason, I think we shouldn't have this "wrap to no selection" case.  Feel free
to add reasoning explaining why this is desirable, though, if you have something
in mind -- I'm open to being convinced otherwise :)

+
+  SetSelectedLine(new_selected_line);
+}
+
+void AutofillPopupView::DecreaseSelectedLine() {
+  int new_selected_line = selected_line_ - 1;
+
+  if (new_selected_line == -2)
+    new_selected_line = autofill_values_.size() - 1;
+
+  SetSelectedLine(new_selected_line);
+}
+
+void AutofillPopupView::ChooseLine(int line) {
+  if (line < 0 || line >= static_cast<int>(autofill_values_.size())) {

[Ilya Sherman, 2012/03/15 20:10:56]

Hmm, why is this code reachable?  (I.e., what is the user flow that would reach
this code?)

[csharp, 2012/03/21 15:32:28]

Shouldn't be, but I've added a DCHECK to just make sure that the selected line
doesn't get out of bounds at some point (there shouldn't be any way for this to
happen at the moment).

+    // Ensure that the popup is no longer visible since the user has
+    // made a selection.
+    Hide();
+    return;
+  }
+
+  external_delegate()->DidAcceptAutofillSuggestions(
+      autofill_values_[line],
+      autofill_unique_ids_[line],
+      line);
+}
+
+void AutofillPopupView::RemoveLine(int line) {
+  // TODO(csharp) add removal code.
+}
+
 void AutofillPopupView::Observe(int type,
                                 const content::NotificationSource& source,
                                 const content::NotificationDetails& details) {