GTK Keyboard Support for New Autofill

chrome/browser/autofill/autofill_popup_view.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/autofill/autofill_popup_view.h"
 
 #include "base/logging.h"
 #include "chrome/browser/autofill/autofill_external_delegate.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/browser/navigation_controller.h"
 #include "content/public/browser/notification_service.h"
 #include "content/public/browser/notification_source.h"
 #include "content/public/browser/notification_types.h"
 
 AutofillPopupView::AutofillPopupView(
     content::WebContents* web_contents,
     AutofillExternalDelegate* external_delegate)
     : external_delegate_(external_delegate),
       selected_line_(-1) {
   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() {}
 
 void AutofillPopupView::Hide() {
   HideInternal();
 
   external_delegate_->ClearPreviewedForm();
 }
 
@@ skipping 24 matching lines @@
 
   selected_line_ = selected_line;
 
   if (selected_line_ != -1) {
     external_delegate_->SelectAutofillSuggestionAtIndex(
         autofill_unique_ids_[selected_line_],
         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) {
   if (type == content::NOTIFICATION_WEB_CONTENTS_HIDDEN
       || type == content::NOTIFICATION_NAV_ENTRY_COMMITTED)
     Hide();
 }