Add card details and legal message to Android save credit card infobar.

chrome/browser/ui/views/autofill/save_card_bubble_views.cc

 // Copyright 2015 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/views/autofill/save_card_bubble_views.h"
 
 #include <stddef.h>
 
 #include "base/strings/utf_string_conversions.h"
 #include "build/build_config.h"
 #include "chrome/browser/ui/autofill/autofill_dialog_types.h"
 #include "chrome/browser/ui/autofill/save_card_bubble_controller.h"
+#include "components/autofill/core/browser/legal_message_line.h"
 #include "grit/components_strings.h"
 #include "ui/base/l10n/l10n_util.h"
 #include "ui/views/bubble/bubble_frame_view.h"
 #include "ui/views/controls/button/blue_button.h"
 #include "ui/views/controls/button/label_button.h"
 #include "ui/views/controls/label.h"
 #include "ui/views/controls/link.h"
 #include "ui/views/controls/styled_label.h"
 #include "ui/views/layout/box_layout.h"
 #include "ui/views/layout/grid_layout.h"
@@ skipping 11 matching lines @@
 // TODO(bondd): BubbleManager will eventually move this logic somewhere else,
 // and then kIsOkButtonOnLeftSide can be removed from here and
 // dialog_client_view.cc.
 #if defined(OS_WIN) || defined(OS_CHROMEOS)
 const bool kIsOkButtonOnLeftSide = true;
 #else
 const bool kIsOkButtonOnLeftSide = false;
 #endif
 
 scoped_ptr<views::StyledLabel> CreateLegalMessageLineLabel(
-    const SaveCardBubbleController::LegalMessageLine& line,
+    const LegalMessageLine& line,
     views::StyledLabelListener* listener) {
   scoped_ptr<views::StyledLabel> label(
-      new views::StyledLabel(line.text, listener));
-  for (const SaveCardBubbleController::LegalMessageLine::Link& link :
-       line.links) {
+      new views::StyledLabel(line.text(), listener));
+  for (const LegalMessageLine::Link& link : line.links()) {
     label->AddStyleRange(link.range,
                          views::StyledLabel::RangeStyleInfo::CreateForLink());
   }
   return label;
 }
 
 }  // namespace
 
 SaveCardBubbleViews::SaveCardBubbleViews(views::View* anchor_view,
                                          content::WebContents* web_contents,
@@ skipping 55 matching lines @@
                                                  const gfx::Range& range,
                                                  int event_flags) {
   // Index of |label| within its parent's view hierarchy is the same as the
   // legal message line index. DCHECK this assumption to guard against future
   // layout changes.
   DCHECK_EQ(static_cast<size_t>(label->parent()->child_count()),
             controller_->GetLegalMessageLines().size());
 
   const auto& links =
       controller_->GetLegalMessageLines()[label->parent()->GetIndexOf(label)]
-          .links;
-  for (const SaveCardBubbleController::LegalMessageLine::Link& link : links) {
+          ->links();
+  for (const LegalMessageLine::Link& link : links) {
     if (link.range == range) {
       controller_->OnLegalMessageLinkClicked(link.url);
       return;
     }
   }
 
   // |range| was not found.
   NOTREACHED();
 }
 
@@ skipping 85 matching lines @@
   view->SetLayoutManager(
       new views::BoxLayout(views::BoxLayout::kVertical,
                            GetBubbleFrameView()->GetTitleInsets().left(),
                            views::kRelatedControlVerticalSpacing, 0));
   view->SetBorder(
       views::Border::CreateSolidSidedBorder(1, 0, 0, 0, kSubtleBorderColor));
   view->set_background(
       views::Background::CreateSolidBackground(kLightShadingColor));
 
   // Add a StyledLabel for each line of the legal message.
-  for (const SaveCardBubbleController::LegalMessageLine& line :
-       controller_->GetLegalMessageLines()) {
-    view->AddChildView(CreateLegalMessageLineLabel(line, this).release());
-  }
+  for (const auto& line : controller_->GetLegalMessageLines())

[Evan Stade, 2016/01/13 04:09:19]

remove auto

[please use gerrit instead, 2016/01/13 21:22:09]

Done.

[Peter Kasting, 2016/01/13 22:10:25]

Seems like we debated this already?:
https://codereview.chromium.org/1540423004/diff/440001/chrome/browser/ui/view...

[Evan Stade, 2016/01/13 22:52:40]

Yes, we did, and I abandoned the debate because I don't feel like it's a good
use of time to argue over style issues in individual codereview issues. Since
Autofill is code I own and continue to be the primary reviewer for, I feel
strongly about this subject in this file.

FWIW, I agree with the examples provided by the google style guide[1]. One is an
iterator type (type specification is over half the length of the line) and one
is a repeated type (not informative). Since auto is not here being used to hide
a repetitive or even very long type specification, I don't think the style guide
explicitly suggests its use. If you think there is a good case for auto here,
I'd ask that you take it up with the style guide owners so they can make their
advice more specific.

And FWIW, using auto here is inconsistent with non-use of auto on L126.

[1] https://google.github.io/styleguide/cppguide.html#auto

[Peter Kasting, 2016/01/13 23:25:31]

The previous debate was about this same line in this same file.  I'm confused by
you saying that you were willing to abandon that debate but not this one, when
they're the same debate.
But it is used to hide a repetitive and longer type:

  for (const scoped_ptr<LegalMessageLine>& line :
           controller_->GetLegalMessageLines())

Here the type specification is over half the line (just like the first example
you cited) and causes a line break, reducing readability of the statement.  It
uses a template, just like the text in the style guide refers to: "...especially
when they involve templates or namespaces." Also, the key type information,
"<LegalMessageLine>", is also repeated in both the function name being called
and partially in the name of the variable, as well as in the name of the call
made in the body of the loop.  The initialization is also taking place based
only on information given in this immediate context; this is in contrast to the
text and example given in the styleguide for the "con" scenario: "...especially
when a variable's initialization depends on things that were declared far away.
In an expression like: 'auto i = x.Lookup(key);' it may not be obvious what i's
type is, if x was declared hundreds of lines earlier."

So I'm confused why you don't feel this is similar to the style guide examples
when it has both the properties you summarize the examples as having, and when
it explicitly falls into a case described in the "Pros" section and avoids the
one described in the "Cons" section.

Finally, as a matter of principle, examples in the style guide are illustrative,
not normative.  They are not intended to limit style guide applicability to only
situations exactly like the examples.  The normative text is the ruling text
itself, which simply reads:

"auto is permitted, for local variables only. Do not use auto for file-scope or
namespace-scope variables, or for class members. Never initialize an auto-typed
variable with a braced initializer list."
I don't see how the style guide could be more specific about this.  It seems
very clearly to fit the situation being described.  If you still disagree, I
would be willing to briefly query some fellow C++ Readability reviewers for you
to see what they think, but it doesn't seem like it ought to be necessary.
Yes, that should ideally use auto as well.

[please use gerrit instead, 2016/01/14 02:39:40]

Now that LegalMessageLines is a plain old std::vector<LegalMessageLine>, we can
replace "auto" with "LegalMessageLine" and keep the single line. I think this is
a good compromise, so let's go with it.

[Peter Kasting, 2016/01/14 04:19:24]

Yeah, in that situation I think we're good without auto.

However, it's worth saying what I was coming here to post before I saw this new
patch of yours: I'm reversing myself and think the old code should avoid auto as
well.

To be clear, I still think auto here is more readable, and compliant with the
style guide, so I'm glad we had the discussion, because I hope Evan's outlook
changes at some point :).  But the readability difference is not enormous, and
the style guide clearly _allows_ both behaviors.  In that situation, it's
ultimately the OWNERS' call what to do.  I'm a c/b/ui/ OWNER, but Evan is
specifically a c/b/ui/autofill/ OWNER, and his desires should have governed if I
didn't have overwhelming reason to reverse.

Sorry, Evan, for needing a bit of time away from this thread to recognize that I
should have conceded sooner.

+    view->AddChildView(CreateLegalMessageLineLabel(*line, this).release());
 
   return view;
 }
 
 void SaveCardBubbleViews::Init() {
   SetLayoutManager(new views::BoxLayout(views::BoxLayout::kVertical, 0, 0, 0));
   AddChildView(CreateMainContentView().release());
   if (!controller_->GetLegalMessageLines().empty())
     AddChildView(CreateFootnoteView().release());
 
   set_margins(gfx::Insets(1, 0, 1, 0));
 }
 
 }  // namespace autofill