HTTP Bad: Add warning message to autofill dropdown for http sites

chrome/browser/ui/autofill/autofill_popup_controller_impl.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/autofill/autofill_popup_controller_impl.h"
 
 #include <algorithm>
 #include <utility>
 
 #include "base/command_line.h"
 #include "base/logging.h"
 #include "base/macros.h"
 #include "base/strings/utf_string_conversions.h"
 #include "build/build_config.h"
 #include "chrome/browser/ui/autofill/autofill_popup_view.h"
 #include "chrome/browser/ui/autofill/popup_constants.h"
 #include "components/autofill/core/browser/autofill_popup_delegate.h"
 #include "components/autofill/core/browser/popup_item_ids.h"
 #include "components/autofill/core/browser/suggestion.h"
+#include "components/security_state/switches.h"
+#include "components/strings/grit/components_strings.h"
 #include "content/public/browser/native_web_keyboard_event.h"
+#include "content/public/browser/web_contents.h"

[vabr (Chromium), 2016/11/04 08:55:29]

Is this still needed?

[lshang, 2016/11/07 05:34:02]

Removed.

+#include "content/public/common/origin_util.h"
+#include "ui/base/l10n/l10n_util.h"
 #include "ui/events/event.h"
 #include "ui/gfx/canvas.h"
 #include "ui/gfx/text_elider.h"
 #include "ui/gfx/text_utils.h"
 
 using base::WeakPtr;
 
 namespace autofill {
 namespace {
 
@@ skipping 357 matching lines @@
 
 int AutofillPopupControllerImpl::selected_line() const {
   return selected_line_;
 }
 
 const AutofillPopupLayoutModel& AutofillPopupControllerImpl::layout_model()
     const {
   return layout_model_;
 }
 
+bool AutofillPopupControllerImpl::IsTopLevelURLSecure() const {
+  return content::IsOriginSecure(

[Mathieu, 2016/11/05 03:26:59]

BTW in AutofillManager, we check for the secure context in a slightly different
way [1], and allow mixed passive content. Just making sure we actually want
different behavior for this warning.

[1]
https://cs.chromium.org/chromium/src/components/autofill/core/browser/autofil...

[lshang, 2016/11/07 05:34:02]

Good question. 
In my update patch, for password form, I use IsOriginSecure() as the HTTP Bad
design doc suggested
(https://docs.google.com/document/d/1xno6g6OnA7strcyzE-o_drevW8L0Mb6ZBEkjsiwa6...),
and for credit card form, since there is already a IsContextSecure() check to
show the autofill disabled message, so I added the warning message there. 

I'll ask Enamel people about these different check methods to confirm.

+      controller_common_->web_contents()->GetVisibleURL());
+}
+
 void AutofillPopupControllerImpl::SetSelectedLine(int selected_line) {
   if (selected_line_ == selected_line)
     return;
 
   if (selected_line_ != kNoSelection &&
       static_cast<size_t>(selected_line_) < suggestions_.size())
     InvalidateRow(selected_line_);
 
   if (selected_line != kNoSelection) {
     InvalidateRow(selected_line);
@@ skipping 60 matching lines @@
   if (suggestions_.empty())
     return false;
   int id = suggestions_[0].frontend_id;
   return id > 0 ||
          id == POPUP_ITEM_ID_AUTOCOMPLETE_ENTRY ||
          id == POPUP_ITEM_ID_PASSWORD_ENTRY ||
          id == POPUP_ITEM_ID_DATALIST_ENTRY ||
          id == POPUP_ITEM_ID_SCAN_CREDIT_CARD;
 }
 
 void AutofillPopupControllerImpl::SetValues(

[Mathieu, 2016/11/05 03:26:59]

This function doesn't seem like the ideal place to be adding entries to the
popup. See the code in AutofillManager that adds the existing Credit card
insecure warning here:

https://cs.chromium.org/chromium/src/components/autofill/core/browser/autofil...

[lshang, 2016/11/07 05:34:02]

Yeah, I add password entry in password_autofill_manager and credit card entry in
autofill_manager's credit card check. Please see the updated patch:-)

     const std::vector<autofill::Suggestion>& suggestions) {
   suggestions_ = suggestions;
-  elided_values_.resize(suggestions.size());
-  elided_labels_.resize(suggestions.size());
-  for (size_t i = 0; i < suggestions.size(); i++) {
-    elided_values_[i] = suggestions[i].value;
-    elided_labels_[i] = suggestions[i].label;
+
+  std::string choice =
+      base::CommandLine::ForCurrentProcess()->GetSwitchValueASCII(
+          security_state::switches::kMarkHttpAs);
+  if (!IsTopLevelURLSecure() &&

[vabr (Chromium), 2016/11/04 08:55:29]

Could you please add a test for the new functionality in
chrome/browser/ui/autofill/autofill_popup_controller_unittest.cc ?

It should test at least these things (ideally one per test):
* There is a warning as long as the warning feature is on and the current URL is
not secure.
* There no warning if the warning feature is off even if the current URL is not
secure.
* There is no warning if the warning feature is on but the current URL is
secure.
* The warning is always the first among suggestions.
* If there are, say, 3 real suggestions, and the warning is added, the total
number of suggestions shown is 4.

[lshang, 2016/11/07 05:34:02]

Done.
Tests added in autofill_manager_unittest and password_autofill_manager_unittest.

+      choice == security_state::switches::kMarkHttpWithPasswordsOrCcWithChip) {
+    autofill::Suggestion autofill_http_warning_suggestion(
+        l10n_util::GetStringUTF16(IDS_AUTOFILL_HTTP_WARNING_MESSAGE));
+    autofill_http_warning_suggestion.frontend_id =
+        POPUP_ITEM_ID_WARNING_MESSAGE;
+    suggestions_.insert(suggestions_.begin(), autofill_http_warning_suggestion);

[vabr (Chromium), 2016/11/04 08:55:29]

optional:

Currently we copy |suggestions| into |suggestions_| and then insert one more to
the front. That means that all the data from |suggestions| needs to be shifted
in |suggestions_|.

An alternative would be to delete line 490, first only push_back the warning
suggestion, and then use

std::copy(suggestions.begin(), suggestions.end(), std::inserter(suggestions_,
suggestions_.end()));


This comment is optional, because it is an unclear trade-off: your current code
may be easier to read, the proposed change will be slightly efficient. If you
considered both options and prefer the current code for its readability, then
I'm fine with your choice, because this is unlikely to be a performance hotspot.

[lshang, 2016/11/07 05:34:02]

I split the password and credit card logic into two different upper-level
classes, so that warning message is added when generating suggestions, prior to
calling into this AotufillPopupControllerImpl class. 

+  }
+
+  elided_values_.resize(suggestions_.size());
+  elided_labels_.resize(suggestions_.size());
+  for (size_t i = 0; i < suggestions_.size(); i++) {
+    elided_values_[i] = suggestions_[i].value;
+    elided_labels_[i] = suggestions_[i].label;
   }
 }
 
 void AutofillPopupControllerImpl::ShowView() {
   view_->Show();
 }
 
 void AutofillPopupControllerImpl::InvalidateRow(size_t row) {
   DCHECK(0 <= row);
   DCHECK(row < suggestions_.size());
@@ skipping 36 matching lines @@
   // Don't clear view_, because otherwise the popup will have to get regenerated
   // and this will cause flickering.
   suggestions_.clear();
   elided_values_.clear();
   elided_labels_.clear();
 
   selected_line_ = kNoSelection;
 }
 
 }  // namespace autofill