Autofill: Add legal message footer to save credit card bubble.

Autofill: Add legal message footer to save credit card bubble.

Footer displays a message with embedded links, using JSON-formatted
message data to be provided by server. There is currently no way to see
the footer without adding test code, as
SaveCardBubbleControllerImpl::SetLegalMessage() is not called from
anywhere yet.

Screenshot attached to bug: https://code.google.com/p/chromium/issues/detail?id=552667#c1

BUG=552667
Committed: https://crrev.com/dfa5b30be8e0809ee7f4c1c33d3b274fbc61ec80
Cr-Commit-Position: refs/heads/master@{#360866}

[bondd, 2015-11-07 02:29:37]

bondd@chromium.org changed reviewers:
+ estade@chromium.org, jdonnelly@chromium.org

[bondd, 2015-11-07 02:29:37]

bondd@chromium.org changed required reviewers:
+ estade@chromium.org

[bondd, 2015-11-07 02:29:38]

https://codereview.chromium.org/1407093007/diff/1/chrome/browser/ui/autofill/...
File chrome/browser/ui/autofill/save_card_bubble_controller_impl.cc (right):

https://codereview.chromium.org/1407093007/diff/1/chrome/browser/ui/autofill/...
chrome/browser/ui/autofill/save_card_bubble_controller_impl.cc:48: bool
SaveCardBubbleControllerImpl::SetLegalMessage(
I'll add some unit tests for SetLegalMessage() unless you think it's not worth
the trouble.

https://codereview.chromium.org/1407093007/diff/1/chrome/browser/ui/autofill/...
File chrome/browser/ui/autofill/save_card_bubble_controller_impl.h (right):

https://codereview.chromium.org/1407093007/diff/1/chrome/browser/ui/autofill/...
chrome/browser/ui/autofill/save_card_bubble_controller_impl.h:48: //    expand
correctly.
Can handle "$$" properly if desired by reviewer. Not too hard (only double a $
if previous char is not $), but it's also unlikely to be in legal text. I could
handle any or all of the caveats listed if we think they're worth the trouble.

https://codereview.chromium.org/1407093007/diff/1/chrome/browser/ui/autofill/...
chrome/browser/ui/autofill/save_card_bubble_controller_impl.h:87: struct
LegalMessageLink {
LegalMessageLink instead of Link because the former is a bit ambiguous when
skimming through code.

[Evan Stade, 2015-11-07 03:03:19]

https://codereview.chromium.org/1407093007/diff/1/chrome/browser/ui/autofill/...
File chrome/browser/ui/autofill/save_card_bubble_controller.h (right):

https://codereview.chromium.org/1407093007/diff/1/chrome/browser/ui/autofill/...
chrome/browser/ui/autofill/save_card_bubble_controller.h:31: virtual size_t
GetLegalMessageNumLinks() const = 0;
combine these into one fn that returns a const ref vector

https://codereview.chromium.org/1407093007/diff/1/chrome/browser/ui/autofill/...
File chrome/browser/ui/autofill/save_card_bubble_controller_impl.cc (right):

https://codereview.chromium.org/1407093007/diff/1/chrome/browser/ui/autofill/...
chrome/browser/ui/autofill/save_card_bubble_controller_impl.cc:48: bool
SaveCardBubbleControllerImpl::SetLegalMessage(
this function needs unit tests

https://codereview.chromium.org/1407093007/diff/1/chrome/browser/ui/autofill/...
chrome/browser/ui/autofill/save_card_bubble_controller_impl.cc:52: size_t
next_range_index = 0;
|next_range_index| is not intuitive at all and requires a thorough reading of
the code to understand; this is a good example of why I don't like added
complexity (looping) without a good reason for it

if you need to keep the proto as is, you should change this for loop body to a
single function call:

ParseAndAppendToLegalMessage();

this will disallow or at least discourage cross-iteration variables like
next_range_index which are just confusing and I don't think strictly necessary 
(e.g. if you have a local legal_message_links that you build up before appending
everything to legal_message_links_ you won't need this)

https://codereview.chromium.org/1407093007/diff/1/chrome/browser/ui/autofill/...
chrome/browser/ui/autofill/save_card_bubble_controller_impl.cc:78:
base::string16 text;
display_texts.push_back(base::string16());
if (!single_parameter->GetString("display_text", &display_texts.back()))
  return false;
// Save a line \o/

https://codereview.chromium.org/1407093007/diff/1/chrome/browser/ui/autofill/...
chrome/browser/ui/autofill/save_card_bubble_controller_impl.cc:84:
LegalMessageLink link;
ditto

https://codereview.chromium.org/1407093007/diff/1/chrome/browser/ui/autofill/...
chrome/browser/ui/autofill/save_card_bubble_controller_impl.cc:94: if
(!single_line->GetString("template", &template_icu))
this function is pretty long and hard to read, can you split into helper
functions? In particular it seems the template parsing/modification part could
be separated cleanly.

https://codereview.chromium.org/1407093007/diff/1/chrome/browser/ui/autofill/...
chrome/browser/ui/autofill/save_card_bubble_controller_impl.cc:152:
legal_message_ = message;
so legal_message_ will be empty on failure but legal_message_links_ will be
half-filled? seems inconsistent

https://codereview.chromium.org/1407093007/diff/1/chrome/browser/ui/autofill/...
File chrome/browser/ui/autofill/save_card_bubble_controller_impl.h (right):

https://codereview.chromium.org/1407093007/diff/1/chrome/browser/ui/autofill/...
chrome/browser/ui/autofill/save_card_bubble_controller_impl.h:41: //  
"template" : "This is the second line and it has no parameters"
can jstuppy or someone explain why there are multiple lines instead of a single
one with newlines?

https://codereview.chromium.org/1407093007/diff/1/chrome/browser/ui/views/aut...
File chrome/browser/ui/views/autofill/save_card_bubble_views.cc (right):

https://codereview.chromium.org/1407093007/diff/1/chrome/browser/ui/views/aut...
chrome/browser/ui/views/autofill/save_card_bubble_views.cc:184:
views::Background::CreateSolidBackground(kLightShadingColor));
this should probably actually use
color_utils::BlendTowardOppositeLuminance(GetNativeTheme()->GetSystemColor(kColorId_BubbleBackground),
7) (feel free to fix the other usage(s) of kLightShadingColor if you want to)

also the border color one should do the same, except with 10

(the only situation this is likely to matter is on Windows inverted color
schemes or a dark gtk theme with chrome on gtk theme mode)

https://codereview.chromium.org/1407093007/diff/1/chrome/browser/ui/views/aut...
chrome/browser/ui/views/autofill/save_card_bubble_views.cc:190:
views::StyledLabel::RangeStyleInfo::CreateForLink();
no need to put this here, you're copying it over and over anyway so just inline
this

[Justin Donnelly, 2015-11-09 16:04:36]

https://codereview.chromium.org/1407093007/diff/1/chrome/browser/ui/views/aut...
File chrome/browser/ui/views/autofill/save_card_bubble_views.cc (right):

https://codereview.chromium.org/1407093007/diff/1/chrome/browser/ui/views/aut...
chrome/browser/ui/views/autofill/save_card_bubble_views.cc:190:
views::StyledLabel::RangeStyleInfo::CreateForLink();
On 2015/11/07 03:03:19, Evan Stade wrote:
> no need to put this here, you're copying it over and over anyway so just
inline
> this

The default for CreateForLink in not underlined. (See screenshot at
http://crbug.com/552667.) Either you're going to need to keep this variable in
order to add font_style = gfx::Font::UNDERLINE or go back to not underlining the
"Learn More" link.

Evan, I know you were opposed to this and for reasons that make some sense. But
either way, we need to make this consistent. The password bubble doesn't use
underline, the translate bubble does and this bubble (as of this CL) uses both.

[Evan Stade, 2015-11-09 18:59:10]

https://codereview.chromium.org/1407093007/diff/1/chrome/browser/ui/views/aut...
File chrome/browser/ui/views/autofill/save_card_bubble_views.cc (right):

https://codereview.chromium.org/1407093007/diff/1/chrome/browser/ui/views/aut...
chrome/browser/ui/views/autofill/save_card_bubble_views.cc:190:
views::StyledLabel::RangeStyleInfo::CreateForLink();
On 2015/11/09 16:04:35, Justin Donnelly wrote:
> On 2015/11/07 03:03:19, Evan Stade wrote:
> > no need to put this here, you're copying it over and over anyway so just
> inline
> > this
> 
> The default for CreateForLink in not underlined. (See screenshot at
> http://crbug.com/552667.) Either you're going to need to keep this variable in
> order to add font_style = gfx::Font::UNDERLINE or go back to not underlining
the
> "Learn More" link.
> 
> Evan, I know you were opposed to this and for reasons that make some sense.
But
> either way, we need to make this consistent. The password bubble doesn't use
> underline, the translate bubble does and this bubble (as of this CL) uses
both.

uh, well it's a little different. A standalone link vs link inlined in text, so
it seems conceivable we would want different underline behaviors, although
whether this is actually true or not I don't know. (in fact, in one weird but
well-reasoned instance, we have a single link that's both underlined and not
underlined, depending on your theme)

I'm fine with leaving this variable here though if there's a need for it as
Justin points out there might be.

[bondd, 2015-11-11 01:53:37]

As Evan and I discussed, I addressed all issues other than '\n'. When we get a
resolution on that I can update SetLegalMessage() and the unit tests if
necessary.

https://codereview.chromium.org/1407093007/diff/1/chrome/browser/ui/autofill/...
File chrome/browser/ui/autofill/save_card_bubble_controller.h (right):

https://codereview.chromium.org/1407093007/diff/1/chrome/browser/ui/autofill/...
chrome/browser/ui/autofill/save_card_bubble_controller.h:31: virtual size_t
GetLegalMessageNumLinks() const = 0;
On 2015/11/07 03:03:19, Evan Stade wrote:
> combine these into one fn that returns a const ref vector

Done.

https://codereview.chromium.org/1407093007/diff/1/chrome/browser/ui/autofill/...
File chrome/browser/ui/autofill/save_card_bubble_controller_impl.cc (right):

https://codereview.chromium.org/1407093007/diff/1/chrome/browser/ui/autofill/...
chrome/browser/ui/autofill/save_card_bubble_controller_impl.cc:52: size_t
next_range_index = 0;
On 2015/11/07 03:03:19, Evan Stade wrote:
> |next_range_index| is not intuitive at all and requires a thorough reading of
> the code to understand; this is a good example of why I don't like added
> complexity (looping) without a good reason for it
> 
> if you need to keep the proto as is, you should change this for loop body to a
> single function call:
> 
> ParseAndAppendToLegalMessage();
> 
> this will disallow or at least discourage cross-iteration variables like
> next_range_index which are just confusing and I don't think strictly necessary

> (e.g. if you have a local legal_message_links that you build up before
appending
> everything to legal_message_links_ you won't need this)

Done.

https://codereview.chromium.org/1407093007/diff/1/chrome/browser/ui/autofill/...
chrome/browser/ui/autofill/save_card_bubble_controller_impl.cc:78:
base::string16 text;
On 2015/11/07 03:03:19, Evan Stade wrote:
> display_texts.push_back(base::string16());
> if (!single_parameter->GetString("display_text", &display_texts.back()))
>   return false;
> // Save a line \o/

Done.

https://codereview.chromium.org/1407093007/diff/1/chrome/browser/ui/autofill/...
chrome/browser/ui/autofill/save_card_bubble_controller_impl.cc:84:
LegalMessageLink link;
On 2015/11/07 03:03:19, Evan Stade wrote:
> ditto

Done.

https://codereview.chromium.org/1407093007/diff/1/chrome/browser/ui/autofill/...
chrome/browser/ui/autofill/save_card_bubble_controller_impl.cc:94: if
(!single_line->GetString("template", &template_icu))
On 2015/11/07 03:03:19, Evan Stade wrote:
> this function is pretty long and hard to read, can you split into helper
> functions? In particular it seems the template parsing/modification part could
> be separated cleanly.

Done. I think the new level of splitting makes sense; all of the Value reading
for a message line is in one function (less jumping back and forth between
functions to see what's happening) and each function fits on ~1 screen. Let me
know if you'd like me to split it further.

https://codereview.chromium.org/1407093007/diff/1/chrome/browser/ui/autofill/...
chrome/browser/ui/autofill/save_card_bubble_controller_impl.cc:152:
legal_message_ = message;
On 2015/11/07 03:03:19, Evan Stade wrote:
> so legal_message_ will be empty on failure but legal_message_links_ will be
> half-filled? seems inconsistent

Done. Everything is now empty on failure.

https://codereview.chromium.org/1407093007/diff/1/chrome/browser/ui/views/aut...
File chrome/browser/ui/views/autofill/save_card_bubble_views.cc (right):

https://codereview.chromium.org/1407093007/diff/1/chrome/browser/ui/views/aut...
chrome/browser/ui/views/autofill/save_card_bubble_views.cc:184:
views::Background::CreateSolidBackground(kLightShadingColor));
On 2015/11/07 03:03:19, Evan Stade wrote:
> this should probably actually use
>
color_utils::BlendTowardOppositeLuminance(GetNativeTheme()->GetSystemColor(kColorId_BubbleBackground),
> 7) (feel free to fix the other usage(s) of kLightShadingColor if you want to)
> 
> also the border color one should do the same, except with 10
> 
> (the only situation this is likely to matter is on Windows inverted color
> schemes or a dark gtk theme with chrome on gtk theme mode)

How about I change all of these at once in a followup CL? I'd prefer to replace
kLightShadingColor and kSubtleBorderColor with
kLightShadingAlpha = 7
kSubtleBorderAlpha = 10
and update all uses of them at once. I'd like to keep this CL limited to the
save card bubble.

https://codereview.chromium.org/1407093007/diff/1/chrome/browser/ui/views/aut...
chrome/browser/ui/views/autofill/save_card_bubble_views.cc:190:
views::StyledLabel::RangeStyleInfo::CreateForLink();
On 2015/11/07 03:03:19, Evan Stade wrote:
> no need to put this here, you're copying it over and over anyway so just
inline
> this

Done. And I removed the underline from the "Learn more" text.

https://codereview.chromium.org/1407093007/diff/20001/chrome/browser/ui/autof...
File chrome/browser/ui/autofill/save_card_bubble_controller_impl.cc (right):

https://codereview.chromium.org/1407093007/diff/20001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/save_card_bubble_controller_impl.cc:31: // and
|out_offsets| are undefined.
Leaves output vars undefined on failure because it makes the code
shorter/simpler, and this function is not exposed outside of this file. We know
that caller will return immediately when false is returned, and is very unlikely
to ever do anything else.

https://codereview.chromium.org/1407093007/diff/20001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/save_card_bubble_controller_impl.cc:79: // variables
are undefined.
Same reasoning about undefined vars as for ReplaceTemplatePlaceholders() above.

https://codereview.chromium.org/1407093007/diff/20001/chrome/browser/ui/autof...
File chrome/browser/ui/autofill/save_card_bubble_controller_impl.h (right):

https://codereview.chromium.org/1407093007/diff/20001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/save_card_bubble_controller_impl.h:83: const
std::vector<std::string>& GetLegalMessageLinkUrlsForTesting() const;
I'm not sure what the current best practice is for exposing this for testing. I
can think of lots of ways to do it (with various levels of refactoring), but
maybe it's fine this way. Suggestions welcome. Best suggestion being "leave it
like this". :)

https://codereview.chromium.org/1407093007/diff/20001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/save_card_bubble_controller_impl.h:116:
std::vector<std::string> legal_message_link_urls_;
Made these separate (instead of combining range and url into LegalMessageLink
like in last patch set) so that GetLegalMessageLinkRanges() can return a const
ref vector. It's a bit of extra internal complexity to make the external
interface simpler. Don't need to expose struct LegalMessageLink.

https://codereview.chromium.org/1407093007/diff/20001/chrome/chrome_tests_uni...
File chrome/chrome_tests_unit.gypi (right):

https://codereview.chromium.org/1407093007/diff/20001/chrome/chrome_tests_uni...
chrome/chrome_tests_unit.gypi:253:
'browser/ui/autofill/save_card_bubble_controller_impl_unittest.cc',
Unit tests will run on Mac, even though controller is used there yet (but will
be soon). Okay?

[Evan Stade, 2015-11-11 22:22:55]

https://codereview.chromium.org/1407093007/diff/1/chrome/browser/ui/views/aut...
File chrome/browser/ui/views/autofill/save_card_bubble_views.cc (right):

https://codereview.chromium.org/1407093007/diff/1/chrome/browser/ui/views/aut...
chrome/browser/ui/views/autofill/save_card_bubble_views.cc:184:
views::Background::CreateSolidBackground(kLightShadingColor));
On 2015/11/11 01:53:36, bondd wrote:
> On 2015/11/07 03:03:19, Evan Stade wrote:
> > this should probably actually use
> >
>
color_utils::BlendTowardOppositeLuminance(GetNativeTheme()->GetSystemColor(kColorId_BubbleBackground),
> > 7) (feel free to fix the other usage(s) of kLightShadingColor if you want
to)
> > 
> > also the border color one should do the same, except with 10
> > 
> > (the only situation this is likely to matter is on Windows inverted color
> > schemes or a dark gtk theme with chrome on gtk theme mode)
> 
> How about I change all of these at once in a followup CL? I'd prefer to
replace
> kLightShadingColor and kSubtleBorderColor with
> kLightShadingAlpha = 7
> kSubtleBorderAlpha = 10
> and update all uses of them at once. I'd like to keep this CL limited to the
> save card bubble.

sure, can you file a bug for it

https://codereview.chromium.org/1407093007/diff/20001/chrome/browser/ui/autof...
File chrome/browser/ui/autofill/save_card_bubble_controller_impl.h (right):

https://codereview.chromium.org/1407093007/diff/20001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/save_card_bubble_controller_impl.h:116:
std::vector<std::string> legal_message_link_urls_;
On 2015/11/11 01:53:36, bondd wrote:
> Made these separate (instead of combining range and url into LegalMessageLink
> like in last patch set) so that GetLegalMessageLinkRanges() can return a const
> ref vector. It's a bit of extra internal complexity to make the external
> interface simpler. Don't need to expose struct LegalMessageLink.

make the testing function protected and create a
TestSaveCardBubbleControllerImpl (if it doesn't already exist) which inherits
from this class

https://codereview.chromium.org/1407093007/diff/20001/chrome/chrome_tests_uni...
File chrome/chrome_tests_unit.gypi (right):

https://codereview.chromium.org/1407093007/diff/20001/chrome/chrome_tests_uni...
chrome/chrome_tests_unit.gypi:253:
'browser/ui/autofill/save_card_bubble_controller_impl_unittest.cc',
On 2015/11/11 01:53:37, bondd wrote:
> Unit tests will run on Mac, even though controller is used there yet (but will
> be soon). Okay?

sure

https://codereview.chromium.org/1407093007/diff/30001/chrome/browser/ui/autof...
File chrome/browser/ui/autofill/save_card_bubble_controller_impl.h (right):

https://codereview.chromium.org/1407093007/diff/30001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/save_card_bubble_controller_impl.h:79: const
base::string16& GetLegalMessage() const override;
to properly support and better reflect the structure proto, I think you need to
make this return a vector of structs.

struct LegalMessage {
  string16 text;
  vector<range> links;
  // future: bool show_checkbox;
};
const vector<LegalMessage>& GetLegalMessages() const;

this should also remove the need to append legal messages and link ranges, which
simplifies things.

[Evan Stade, 2015-11-11 23:53:54]

https://codereview.chromium.org/1407093007/diff/30001/chrome/browser/ui/autof...
File chrome/browser/ui/autofill/save_card_bubble_controller_impl_unittest.cc
(right):

https://codereview.chromium.org/1407093007/diff/30001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/save_card_bubble_controller_impl_unittest.cc:300:
expected_ranges.push_back(gfx::Range(19, 20));
struct {
  Range range;
  URL url;
} test_cases[] = {
  {{1, 2}, "..."},
  ...
}

[bondd, 2015-11-13 01:19:53]

Hi Evan,
For your consideration. :) I think this is everything we discussed.
I haven't updated the tests yet. I'll wait until we get close to an LG on this
stuff before I spend any time on the tests and trybots.
Thanks
-Dave

https://codereview.chromium.org/1407093007/diff/1/chrome/browser/ui/views/aut...
File chrome/browser/ui/views/autofill/save_card_bubble_views.cc (right):

https://codereview.chromium.org/1407093007/diff/1/chrome/browser/ui/views/aut...
chrome/browser/ui/views/autofill/save_card_bubble_views.cc:184:
views::Background::CreateSolidBackground(kLightShadingColor));
On 2015/11/11 22:22:54, Evan Stade wrote:
> On 2015/11/11 01:53:36, bondd wrote:
> > On 2015/11/07 03:03:19, Evan Stade wrote:
> > > this should probably actually use
> > >
> >
>
color_utils::BlendTowardOppositeLuminance(GetNativeTheme()->GetSystemColor(kColorId_BubbleBackground),
> > > 7) (feel free to fix the other usage(s) of kLightShadingColor if you want
> to)
> > > 
> > > also the border color one should do the same, except with 10
> > > 
> > > (the only situation this is likely to matter is on Windows inverted color
> > > schemes or a dark gtk theme with chrome on gtk theme mode)
> > 
> > How about I change all of these at once in a followup CL? I'd prefer to
> replace
> > kLightShadingColor and kSubtleBorderColor with
> > kLightShadingAlpha = 7
> > kSubtleBorderAlpha = 10
> > and update all uses of them at once. I'd like to keep this CL limited to the
> > save card bubble.
> 
> sure, can you file a bug for it

Done. https://code.google.com/p/chromium/issues/detail?id=555202

https://codereview.chromium.org/1407093007/diff/20001/chrome/browser/ui/autof...
File chrome/browser/ui/autofill/save_card_bubble_controller_impl.h (right):

https://codereview.chromium.org/1407093007/diff/20001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/save_card_bubble_controller_impl.h:116:
std::vector<std::string> legal_message_link_urls_;
On 2015/11/11 22:22:54, Evan Stade wrote:
> On 2015/11/11 01:53:36, bondd wrote:
> > Made these separate (instead of combining range and url into
LegalMessageLink
> > like in last patch set) so that GetLegalMessageLinkRanges() can return a
const
> > ref vector. It's a bit of extra internal complexity to make the external
> > interface simpler. Don't need to expose struct LegalMessageLink.
> 
> make the testing function protected and create a
> TestSaveCardBubbleControllerImpl (if it doesn't already exist) which inherits
> from this class

Thanks. If we stick with the new interface then I can get the URLs through the
public interface.

https://codereview.chromium.org/1407093007/diff/30001/chrome/browser/ui/autof...
File chrome/browser/ui/autofill/save_card_bubble_controller_impl.h (right):

https://codereview.chromium.org/1407093007/diff/30001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/save_card_bubble_controller_impl.h:79: const
base::string16& GetLegalMessage() const override;
On 2015/11/11 22:22:55, Evan Stade wrote:
> to properly support and better reflect the structure proto, I think you need
to
> make this return a vector of structs.
> 
> struct LegalMessage {
>   string16 text;
>   vector<range> links;
>   // future: bool show_checkbox;
> };
> const vector<LegalMessage>& GetLegalMessages() const;
> 
> this should also remove the need to append legal messages and link ranges,
which
> simplifies things.

Done.

https://codereview.chromium.org/1407093007/diff/50001/chrome/browser/ui/autof...
File chrome/browser/ui/autofill/save_card_bubble_controller.h (right):

https://codereview.chromium.org/1407093007/diff/50001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/save_card_bubble_controller.h:21: struct
LegalMessageLine {
I want to use this as POD, but chromium-style requires explicit out-of-line
constructor and destructor. Okay like this, or is there a better way?

[Evan Stade, 2015-11-13 01:33:07]

looks much better, thanks. Controller and view lg with nits.

https://codereview.chromium.org/1407093007/diff/50001/chrome/browser/ui/autof...
File chrome/browser/ui/autofill/save_card_bubble_controller.h (right):

https://codereview.chromium.org/1407093007/diff/50001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/save_card_bubble_controller.h:24: std::string url;
nit: GURL

https://codereview.chromium.org/1407093007/diff/50001/chrome/browser/ui/autof...
File chrome/browser/ui/autofill/save_card_bubble_controller_impl.cc (right):

https://codereview.chromium.org/1407093007/diff/50001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/save_card_bubble_controller_impl.cc:80:
autofill::SaveCardBubbleController::LegalMessageLine& out) {
outparams must always be pointer type

that said, you could just return LegalMessageLine (a return value of
LegalMessageLine() indicates failure)

https://codereview.chromium.org/1407093007/diff/50001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/save_card_bubble_controller_impl.cc:160:
std::vector<LegalMessageLine> parsed_lines;
parsed_lines? Why not just legal_message_lines_? If you think it's important to
clear it on failure, you can always call clear() before returning false.

https://codereview.chromium.org/1407093007/diff/50001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/save_card_bubble_controller_impl.cc:174:
legal_message_lines_ = parsed_lines;
if you keep this line you should use swap()

https://codereview.chromium.org/1407093007/diff/50001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/save_card_bubble_controller_impl.cc:178: void
SaveCardBubbleControllerImpl::ClearLegalMessage() {
I don't get the purpose of this fn

https://codereview.chromium.org/1407093007/diff/50001/chrome/browser/ui/views...
File chrome/browser/ui/views/autofill/save_card_bubble_views.cc (right):

https://codereview.chromium.org/1407093007/diff/50001/chrome/browser/ui/views...
chrome/browser/ui/views/autofill/save_card_bubble_views.cc:120: for (size_t i =
0; i < links.size(); ++i) {
this is why i suggested a map for range->url. But I think you might need to add
operator< for gfx::Range. I guess that's a yak too far for this CL, but it would
be nice to have that.

I do think std::find_if with a lambda would also make this cooler 8)

https://codereview.chromium.org/1407093007/diff/50001/chrome/browser/ui/views...
chrome/browser/ui/views/autofill/save_card_bubble_views.cc:143: View* view = new
View();
nit: scoped_ptr

https://codereview.chromium.org/1407093007/diff/50001/chrome/browser/ui/views...
chrome/browser/ui/views/autofill/save_card_bubble_views.cc:201: View* view = new
View();
nit: scoped_ptr

[bondd, 2015-11-13 22:30:44]

Responded to nits and fixed unit tests. I've got a couple of questions below.

https://codereview.chromium.org/1407093007/diff/50001/chrome/browser/ui/autof...
File chrome/browser/ui/autofill/save_card_bubble_controller.h (right):

https://codereview.chromium.org/1407093007/diff/50001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/save_card_bubble_controller.h:24: std::string url;
On 2015/11/13 01:33:07, Evan Stade wrote:
> nit: GURL

Done.

https://codereview.chromium.org/1407093007/diff/50001/chrome/browser/ui/autof...
File chrome/browser/ui/autofill/save_card_bubble_controller_impl.cc (right):

https://codereview.chromium.org/1407093007/diff/50001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/save_card_bubble_controller_impl.cc:80:
autofill::SaveCardBubbleController::LegalMessageLine& out) {
On 2015/11/13 01:33:07, Evan Stade wrote:
> outparams must always be pointer type
> 
> that said, you could just return LegalMessageLine (a return value of
> LegalMessageLine() indicates failure)

Done. I made it a pointer.

https://codereview.chromium.org/1407093007/diff/50001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/save_card_bubble_controller_impl.cc:160:
std::vector<LegalMessageLine> parsed_lines;
On 2015/11/13 01:33:07, Evan Stade wrote:
> parsed_lines? Why not just legal_message_lines_? If you think it's important
to
> clear it on failure, you can always call clear() before returning false.

Done.

https://codereview.chromium.org/1407093007/diff/50001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/save_card_bubble_controller_impl.cc:174:
legal_message_lines_ = parsed_lines;
On 2015/11/13 01:33:07, Evan Stade wrote:
> if you keep this line you should use swap()

Obsolete.

https://codereview.chromium.org/1407093007/diff/50001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/save_card_bubble_controller_impl.cc:178: void
SaveCardBubbleControllerImpl::ClearLegalMessage() {
On 2015/11/13 01:33:07, Evan Stade wrote:
> I don't get the purpose of this fn

SaveCardBubbleControllerImpl is per tab, and once created lives until the tab is
closed. Several bubbles can be created and dismissed during the lifetime of one
SaveCardBubbleControllerImpl, so there needs to be a way to clear the legal
message if one bubble needs the message and the next does not.

I suppose the caller could call SetLegalMessage(base::ListValue()) or similar,
but an explicit ClearLegalMessage() function seems easier to understand at a
glance.

https://codereview.chromium.org/1407093007/diff/50001/chrome/browser/ui/views...
File chrome/browser/ui/views/autofill/save_card_bubble_views.cc (right):

https://codereview.chromium.org/1407093007/diff/50001/chrome/browser/ui/views...
chrome/browser/ui/views/autofill/save_card_bubble_views.cc:120: for (size_t i =
0; i < links.size(); ++i) {
On 2015/11/13 01:33:07, Evan Stade wrote:
> this is why i suggested a map for range->url. But I think you might need to
add
> operator< for gfx::Range. I guess that's a yak too far for this CL, but it
would
> be nice to have that.
> 
> I do think std::find_if with a lambda would also make this cooler 8)

Yep, I tried a map and decided against it when I found out I needed to implement
operator<. Range end can be before start, and there is a "reverse" flag IIRC.
Not crystal clear how operator< would work. Enough people use gfx::Range that it
might drag down into a long discussion (which makes sense, just preferably not
as part of this CL). :)

https://codereview.chromium.org/1407093007/diff/50001/chrome/browser/ui/views...
chrome/browser/ui/views/autofill/save_card_bubble_views.cc:143: View* view = new
View();
On 2015/11/13 01:33:07, Evan Stade wrote:
> nit: scoped_ptr

Sorry, can you explain this out a bit further, or link to an example? I know how
scoped_ptr works, but as I understand it AddChildView() in
SaveCardBubbleViews::Init() will take ownership of this object immediately. I
feel like I'm missing something obvious. :)

https://codereview.chromium.org/1407093007/diff/110001/chrome/browser/ui/auto...
File chrome/browser/ui/autofill/save_card_bubble_controller_impl_unittest.cc
(right):

https://codereview.chromium.org/1407093007/diff/110001/chrome/browser/ui/auto...
chrome/browser/ui/autofill/save_card_bubble_controller_impl_unittest.cc:56: bool
CompareLegalMessage(
AFAIK I can't use operator== on LegalMessageLine without implementing a custom
operator== as part of the struct. Compare is only needed as part of tests, and
I'd like to keep LegalMessageLine as POD, so I added test-only compare functions
here. Is there a better way to do this?

[Justin Donnelly, 2015-11-13 22:51:04]

https://codereview.chromium.org/1407093007/diff/50001/chrome/browser/ui/views...
File chrome/browser/ui/views/autofill/save_card_bubble_views.cc (right):

https://codereview.chromium.org/1407093007/diff/50001/chrome/browser/ui/views...
chrome/browser/ui/views/autofill/save_card_bubble_views.cc:143: View* view = new
View();
On 2015/11/13 22:30:43, bondd wrote:
> On 2015/11/13 01:33:07, Evan Stade wrote:
> > nit: scoped_ptr
> 
> Sorry, can you explain this out a bit further, or link to an example? I know
how
> scoped_ptr works, but as I understand it AddChildView() in
> SaveCardBubbleViews::Init() will take ownership of this object immediately. I
> feel like I'm missing something obvious. :)

The idea is to be defensive against future changes. We can inspect it now and
say it's correct. But even assuming we're correct, if someone were to come along
later and add an early return to this method then you risk a memory leak.

I think the current school of thought is to avoid handling naked pointers at any
point, no matter how small the scope. (Though this method has plenty of scope
for someone to introduce a leak.)

This requires a few things:

- Declare this as scoped_ptr<View> view(new View()) here.
- Change the return type of this method to scoped_ptr<View>.
- Where you reach the point of the API that insists on a naked pointer
(AddChildView), call release(). In other words:
AddChildView(CreateMainContentView().release());

[bondd, 2015-11-13 23:28:23]

bondd@chromium.org changed reviewers:
+ sky@chromium.org

[bondd, 2015-11-13 23:28:23]

bondd@chromium.org changed required reviewers:
+ sky@chromium.org

[bondd, 2015-11-13 23:28:23]

+sky@ for OWNERS review:
chrome/browser/ui/views/ (except for chrome/browser/ui/views/autofill/)
ui/views/controls/

All I did here is add "StyledLabel& label" parameter to
StyledLabelListener::StyledLabelLinkClicked(), so that it can be used in
SaveCardBubbleViews::StyledLabelLinkClicked().

Thanks
-Dave

[Evan Stade, 2015-11-14 00:22:50]

https://codereview.chromium.org/1407093007/diff/50001/chrome/browser/ui/autof...
File chrome/browser/ui/autofill/save_card_bubble_controller_impl.cc (right):

https://codereview.chromium.org/1407093007/diff/50001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/save_card_bubble_controller_impl.cc:178: void
SaveCardBubbleControllerImpl::ClearLegalMessage() {
On 2015/11/13 22:30:43, bondd wrote:
> On 2015/11/13 01:33:07, Evan Stade wrote:
> > I don't get the purpose of this fn
> 
> SaveCardBubbleControllerImpl is per tab, and once created lives until the tab
is
> closed. Several bubbles can be created and dismissed during the lifetime of
one
> SaveCardBubbleControllerImpl, so there needs to be a way to clear the legal
> message if one bubble needs the message and the next does not.
> 
> I suppose the caller could call SetLegalMessage(base::ListValue()) or similar,
> but an explicit ClearLegalMessage() function seems easier to understand at a
> glance.

I would vote for reusing SetLegalMessage, but this is a discussion for another
day. Right now this is dead code, so please delete it until you need it.

https://codereview.chromium.org/1407093007/diff/110001/chrome/browser/ui/auto...
File chrome/browser/ui/autofill/save_card_bubble_controller_impl_unittest.cc
(right):

https://codereview.chromium.org/1407093007/diff/110001/chrome/browser/ui/auto...
chrome/browser/ui/autofill/save_card_bubble_controller_impl_unittest.cc:56: bool
CompareLegalMessage(
sgtm

https://codereview.chromium.org/1407093007/diff/110001/chrome/browser/ui/auto...
chrome/browser/ui/autofill/save_card_bubble_controller_impl_unittest.cc:380:
link.url = GURL("http://www.example.com/0");
you can greatly improve the legibility of this test and many others if you use
test case struct arrays. In fact, it would allow you to combine at least some of
these tests into one test.

https://codereview.chromium.org/1407093007/diff/150001/ui/views/controls/styl...
File ui/views/controls/styled_label_listener.h (right):

https://codereview.chromium.org/1407093007/diff/150001/ui/views/controls/styl...
ui/views/controls/styled_label_listener.h:21: virtual void
StyledLabelLinkClicked(StyledLabel& label,
non-const ref parameters are not allowed

[sky, 2015-11-14 15:28:37]

https://codereview.chromium.org/1407093007/diff/150001/ui/views/controls/styl...
File ui/views/controls/styled_label_listener.h (right):

https://codereview.chromium.org/1407093007/diff/150001/ui/views/controls/styl...
ui/views/controls/styled_label_listener.h:21: virtual void
StyledLabelLinkClicked(StyledLabel& label,
On 2015/11/14 00:22:50, Evan Stade wrote:
> non-const ref parameters are not allowed

More specifically make it a StyledLabel*

[bondd, 2015-11-17 00:12:04]

I'm still sorting out a few trybots, but the rest of this is ready for review.
Not too many changes since last time.

https://codereview.chromium.org/1407093007/diff/50001/chrome/browser/ui/autof...
File chrome/browser/ui/autofill/save_card_bubble_controller_impl.cc (right):

https://codereview.chromium.org/1407093007/diff/50001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/save_card_bubble_controller_impl.cc:178: void
SaveCardBubbleControllerImpl::ClearLegalMessage() {
On 2015/11/14 00:22:49, Evan Stade wrote:
> On 2015/11/13 22:30:43, bondd wrote:
> > On 2015/11/13 01:33:07, Evan Stade wrote:
> > > I don't get the purpose of this fn
> > 
> > SaveCardBubbleControllerImpl is per tab, and once created lives until the
tab
> is
> > closed. Several bubbles can be created and dismissed during the lifetime of
> one
> > SaveCardBubbleControllerImpl, so there needs to be a way to clear the legal
> > message if one bubble needs the message and the next does not.
> > 
> > I suppose the caller could call SetLegalMessage(base::ListValue()) or
similar,
> > but an explicit ClearLegalMessage() function seems easier to understand at a
> > glance.
> 
> I would vote for reusing SetLegalMessage, but this is a discussion for another
> day. Right now this is dead code, so please delete it until you need it.

Done.

https://codereview.chromium.org/1407093007/diff/50001/chrome/browser/ui/views...
File chrome/browser/ui/views/autofill/save_card_bubble_views.cc (right):

https://codereview.chromium.org/1407093007/diff/50001/chrome/browser/ui/views...
chrome/browser/ui/views/autofill/save_card_bubble_views.cc:143: View* view = new
View();
On 2015/11/13 22:51:04, Justin Donnelly wrote:
> On 2015/11/13 22:30:43, bondd wrote:
> > On 2015/11/13 01:33:07, Evan Stade wrote:
> > > nit: scoped_ptr
> > 
> > Sorry, can you explain this out a bit further, or link to an example? I know
> how
> > scoped_ptr works, but as I understand it AddChildView() in
> > SaveCardBubbleViews::Init() will take ownership of this object immediately.
I
> > feel like I'm missing something obvious. :)
> 
> The idea is to be defensive against future changes. We can inspect it now and
> say it's correct. But even assuming we're correct, if someone were to come
along
> later and add an early return to this method then you risk a memory leak.
> 
> I think the current school of thought is to avoid handling naked pointers at
any
> point, no matter how small the scope. (Though this method has plenty of scope
> for someone to introduce a leak.)
> 
> This requires a few things:
> 
> - Declare this as scoped_ptr<View> view(new View()) here.
> - Change the return type of this method to scoped_ptr<View>.
> - Where you reach the point of the API that insists on a naked pointer
> (AddChildView), call release(). In other words:
> AddChildView(CreateMainContentView().release());

Done. Thanks for the detailed explanation.

https://codereview.chromium.org/1407093007/diff/110001/chrome/browser/ui/auto...
File chrome/browser/ui/autofill/save_card_bubble_controller_impl_unittest.cc
(right):

https://codereview.chromium.org/1407093007/diff/110001/chrome/browser/ui/auto...
chrome/browser/ui/autofill/save_card_bubble_controller_impl_unittest.cc:380:
link.url = GURL("http://www.example.com/0");
On 2015/11/14 00:22:50, Evan Stade wrote:
> you can greatly improve the legibility of this test and many others if you use
> test case struct arrays. In fact, it would allow you to combine at least some
of
> these tests into one test.

Thanks. I've made this test case and most of the others shorter and clearer by
changing the way the expected output is defined. I considered using test_case
struct arrays to shorten it up more, but I like that the way it is now each test
tests one thing, and that thing is clearly indicated by the test name (e.g.
MaximumPlaceholders or MissingUrl). The testing docs I've read at Google have
always recommended keeping tests as simple and easy to read as possible. I think
the way I've got it now is simple and easy to read. This is subjective though so
your opinion may vary. :)

https://codereview.chromium.org/1407093007/diff/150001/ui/views/controls/styl...
File ui/views/controls/styled_label_listener.h (right):

https://codereview.chromium.org/1407093007/diff/150001/ui/views/controls/styl...
ui/views/controls/styled_label_listener.h:21: virtual void
StyledLabelLinkClicked(StyledLabel& label,
On 2015/11/14 15:28:37, sky wrote:
> On 2015/11/14 00:22:50, Evan Stade wrote:
> > non-const ref parameters are not allowed
> 
> More specifically make it a StyledLabel*

Done. Thanks.

[sky, 2015-11-17 14:28:18]

LGTM

[Evan Stade, 2015-11-17 19:38:19]

https://codereview.chromium.org/1407093007/diff/200001/chrome/browser/ui/auto...
File chrome/browser/ui/autofill/save_card_bubble_controller_impl.cc (right):

https://codereview.chromium.org/1407093007/diff/200001/chrome/browser/ui/auto...
chrome/browser/ui/autofill/save_card_bubble_controller_impl.cc:36:
std::vector<size_t>& out_offsets) {
no non-const refs as parameters. Outparams should be pointer types.

https://codereview.chromium.org/1407093007/diff/200001/chrome/browser/ui/auto...
chrome/browser/ui/autofill/save_card_bubble_controller_impl.cc:94: if
(!template_parameters->GetDictionary(parameter_index,
nit: curlies if the condition wraps

https://codereview.chromium.org/1407093007/diff/200001/chrome/browser/ui/auto...
chrome/browser/ui/autofill/save_card_bubble_controller_impl.cc:124: offsets))
nit: curlies if the condition wraps

https://codereview.chromium.org/1407093007/diff/200001/chrome/browser/ui/auto...
chrome/browser/ui/autofill/save_card_bubble_controller_impl.cc:175: }
nit:

if (!lines.GetDictionary(line_index, &single_line) ||
    !ParseLegalMessageLine(*single_line, &legal_message_lines_.back())) {
  legal_message_lines_.clear();
  return false;
}

https://codereview.chromium.org/1407093007/diff/200001/chrome/browser/ui/auto...
File chrome/browser/ui/autofill/save_card_bubble_controller_impl_unittest.cc
(right):

https://codereview.chromium.org/1407093007/diff/200001/chrome/browser/ui/auto...
chrome/browser/ui/autofill/save_card_bubble_controller_impl_unittest.cc:58:
const std::vector<SaveCardBubbleController::LegalMessageLine>& a,
since you write out std::vector<SaveCardBubbleController::LegalMessageLine> a
lot it might make sense to typedef it (in SaveCardBubbleController)

also in this file I'd probably say using
SaveCardBubbleController::LegalMessageLine at the top (I forget if that works,
maybe you have to typedef it instead)

https://codereview.chromium.org/1407093007/diff/200001/chrome/browser/ui/view...
File chrome/browser/ui/views/autofill/save_card_bubble_views.cc (right):

https://codereview.chromium.org/1407093007/diff/200001/chrome/browser/ui/view...
chrome/browser/ui/views/autofill/save_card_bubble_views.cc:130: // | 
learn_more_link_                            save_button_ cancel_button_  |
I'm not a huge fan of this art (sorry, I may be a philistine). For one thing,
it's not even accurate on all platforms. But more importantly it is too simple
to be of any assistance at the moment and in the future will likely go out of
date, and we'll be worse off than if we didn't have it at all.

https://codereview.chromium.org/1407093007/diff/200001/chrome/browser/ui/view...
File chrome/browser/ui/views/autofill/save_card_bubble_views.h (right):

https://codereview.chromium.org/1407093007/diff/200001/chrome/browser/ui/view...
chrome/browser/ui/views/autofill/save_card_bubble_views.h:84:
legal_message_lines_;
I don't really think you need this map, the info is already encoded in the view
hierarchy itself. You can do something like

int line_index = label->parent()->GetIndexOf(label);

[bondd, 2015-11-17 23:48:58]

https://codereview.chromium.org/1407093007/diff/200001/chrome/browser/ui/auto...
File chrome/browser/ui/autofill/save_card_bubble_controller_impl.cc (right):

https://codereview.chromium.org/1407093007/diff/200001/chrome/browser/ui/auto...
chrome/browser/ui/autofill/save_card_bubble_controller_impl.cc:36:
std::vector<size_t>& out_offsets) {
On 2015/11/17 19:38:19, Evan Stade wrote:
> no non-const refs as parameters. Outparams should be pointer types.

Done.

https://codereview.chromium.org/1407093007/diff/200001/chrome/browser/ui/auto...
chrome/browser/ui/autofill/save_card_bubble_controller_impl.cc:94: if
(!template_parameters->GetDictionary(parameter_index,
On 2015/11/17 19:38:19, Evan Stade wrote:
> nit: curlies if the condition wraps

Done.

https://codereview.chromium.org/1407093007/diff/200001/chrome/browser/ui/auto...
chrome/browser/ui/autofill/save_card_bubble_controller_impl.cc:124: offsets))
On 2015/11/17 19:38:19, Evan Stade wrote:
> nit: curlies if the condition wraps

Done.

https://codereview.chromium.org/1407093007/diff/200001/chrome/browser/ui/auto...
chrome/browser/ui/autofill/save_card_bubble_controller_impl.cc:175: }
On 2015/11/17 19:38:19, Evan Stade wrote:
> nit:
> 
> if (!lines.GetDictionary(line_index, &single_line) ||
>     !ParseLegalMessageLine(*single_line, &legal_message_lines_.back())) {
>   legal_message_lines_.clear();
>   return false;
> }

Done. Good one.

https://codereview.chromium.org/1407093007/diff/200001/chrome/browser/ui/auto...
File chrome/browser/ui/autofill/save_card_bubble_controller_impl_unittest.cc
(right):

https://codereview.chromium.org/1407093007/diff/200001/chrome/browser/ui/auto...
chrome/browser/ui/autofill/save_card_bubble_controller_impl_unittest.cc:58:
const std::vector<SaveCardBubbleController::LegalMessageLine>& a,
On 2015/11/17 19:38:19, Evan Stade wrote:
> since you write out std::vector<SaveCardBubbleController::LegalMessageLine> a
> lot it might make sense to typedef it (in SaveCardBubbleController)
> 
> also in this file I'd probably say using
> SaveCardBubbleController::LegalMessageLine at the top (I forget if that works,
> maybe you have to typedef it instead)

Done. In addition to this I could have also done
typedef SaveCardBubbleController::LegalMessageLines Lines;
but I preferred to leave it long to make it more visually distinct from |Line|.
In other words "Line" and "Lines" only differ by one character, whereas
"SaveCardBubbleController::LegalMessageLines" and "Line" are obviously different
things. Also, it's already been typedef'd down to be a bit shorter than the
std::vector<SaveCardBubbleControler::LegalMessageLine>
that was in the previous revision of this CL.

https://codereview.chromium.org/1407093007/diff/200001/chrome/browser/ui/view...
File chrome/browser/ui/views/autofill/save_card_bubble_views.cc (right):

https://codereview.chromium.org/1407093007/diff/200001/chrome/browser/ui/view...
chrome/browser/ui/views/autofill/save_card_bubble_views.cc:130: // | 
learn_more_link_                            save_button_ cancel_button_  |
On 2015/11/17 19:38:19, Evan Stade wrote:
> I'm not a huge fan of this art (sorry, I may be a philistine). For one thing,
> it's not even accurate on all platforms. But more importantly it is too simple
> to be of any assistance at the moment and in the future will likely go out of
> date, and we'll be worse off than if we didn't have it at all.

Done.

https://codereview.chromium.org/1407093007/diff/200001/chrome/browser/ui/view...
File chrome/browser/ui/views/autofill/save_card_bubble_views.h (right):

https://codereview.chromium.org/1407093007/diff/200001/chrome/browser/ui/view...
chrome/browser/ui/views/autofill/save_card_bubble_views.h:84:
legal_message_lines_;
On 2015/11/17 19:38:19, Evan Stade wrote:
> I don't really think you need this map, the info is already encoded in the
view
> hierarchy itself. You can do something like
> 
> int line_index = label->parent()->GetIndexOf(label);

Done.

[Evan Stade, 2015-11-18 00:21:14]

https://codereview.chromium.org/1407093007/diff/200001/chrome/browser/ui/auto...
File chrome/browser/ui/autofill/save_card_bubble_controller_impl_unittest.cc
(right):

https://codereview.chromium.org/1407093007/diff/200001/chrome/browser/ui/auto...
chrome/browser/ui/autofill/save_card_bubble_controller_impl_unittest.cc:58:
const std::vector<SaveCardBubbleController::LegalMessageLine>& a,
On 2015/11/17 23:48:58, bondd wrote:
> On 2015/11/17 19:38:19, Evan Stade wrote:
> > since you write out std::vector<SaveCardBubbleController::LegalMessageLine>
a
> > lot it might make sense to typedef it (in SaveCardBubbleController)
> > 
> > also in this file I'd probably say using
> > SaveCardBubbleController::LegalMessageLine at the top (I forget if that
works,
> > maybe you have to typedef it instead)
> 
> Done. In addition to this I could have also done
> typedef SaveCardBubbleController::LegalMessageLines Lines;
> but I preferred to leave it long to make it more visually distinct from
|Line|.

By this logic, we shouldn't have both LegalMessageLine and LegalMessageLines
types (but I think it's fine to).

> In other words "Line" and "Lines" only differ by one character, whereas
> "SaveCardBubbleController::LegalMessageLines" and "Line" are obviously
different
> things. Also, it's already been typedef'd down to be a bit shorter than the
> std::vector<SaveCardBubbleControler::LegalMessageLine>
> that was in the previous revision of this CL.

https://codereview.chromium.org/1407093007/diff/220001/chrome/browser/ui/auto...
File chrome/browser/ui/autofill/save_card_bubble_controller.h (right):

https://codereview.chromium.org/1407093007/diff/220001/chrome/browser/ui/auto...
chrome/browser/ui/autofill/save_card_bubble_controller.h:28: LegalMessageLine();
are you sure it's necessary to define this outside of this header file?

https://codereview.chromium.org/1407093007/diff/220001/chrome/browser/ui/auto...
File chrome/browser/ui/autofill/save_card_bubble_controller_impl_unittest.cc
(right):

https://codereview.chromium.org/1407093007/diff/220001/chrome/browser/ui/auto...
chrome/browser/ui/autofill/save_card_bubble_controller_impl_unittest.cc:15:
typedef SaveCardBubbleController::LegalMessageLine Line;
you should only use typedef to remove the class name qualifier, not to change
the type name.

https://codereview.chromium.org/1407093007/diff/220001/chrome/browser/ui/view...
File chrome/browser/ui/views/autofill/save_card_bubble_views.cc (right):

https://codereview.chromium.org/1407093007/diff/220001/chrome/browser/ui/view...
chrome/browser/ui/views/autofill/save_card_bubble_views.cc:37:
views::StyledLabel* CreateLegalMessageLineLabel(
return scoped_ptr

https://codereview.chromium.org/1407093007/diff/220001/chrome/browser/ui/view...
chrome/browser/ui/views/autofill/save_card_bubble_views.cc:41: const
std::vector<autofill::SaveCardBubbleController::LegalMessageLine::Link>&
this anonymous namespace should be inside the autofill namespace so you don't
need autofill:: qualifiers like this one

https://codereview.chromium.org/1407093007/diff/220001/chrome/browser/ui/view...
chrome/browser/ui/views/autofill/save_card_bubble_views.cc:117:
DCHECK(static_cast<size_t>(label->parent()->child_count()) ==
DCHECK_EQ

https://codereview.chromium.org/1407093007/diff/220001/chrome/browser/ui/view...
chrome/browser/ui/views/autofill/save_card_bubble_views.cc:119: int line_index =
label->parent()->GetIndexOf(label);
don't need this local variable

https://codereview.chromium.org/1407093007/diff/220001/chrome/browser/ui/view...
chrome/browser/ui/views/autofill/save_card_bubble_views.cc:128: }
nit: \n

https://codereview.chromium.org/1407093007/diff/220001/chrome/browser/ui/view...
chrome/browser/ui/views/autofill/save_card_bubble_views.cc:199: int
horizontal_inset = GetBubbleFrameView()->GetTitleInsets().left();
don't need this local var

https://codereview.chromium.org/1407093007/diff/220001/chrome/browser/ui/view...
chrome/browser/ui/views/autofill/save_card_bubble_views.cc:211:
controller_->GetLegalMessageLines()[i], this);
no need for a local var for this

[bondd, 2015-11-18 01:59:40]

https://codereview.chromium.org/1407093007/diff/200001/chrome/browser/ui/auto...
File chrome/browser/ui/autofill/save_card_bubble_controller_impl_unittest.cc
(right):

https://codereview.chromium.org/1407093007/diff/200001/chrome/browser/ui/auto...
chrome/browser/ui/autofill/save_card_bubble_controller_impl_unittest.cc:58:
const std::vector<SaveCardBubbleController::LegalMessageLine>& a,
On 2015/11/18 00:21:13, Evan Stade wrote:
> On 2015/11/17 23:48:58, bondd wrote:
> > On 2015/11/17 19:38:19, Evan Stade wrote:
> > > since you write out
std::vector<SaveCardBubbleController::LegalMessageLine>
> a
> > > lot it might make sense to typedef it (in SaveCardBubbleController)
> > > 
> > > also in this file I'd probably say using
> > > SaveCardBubbleController::LegalMessageLine at the top (I forget if that
> works,
> > > maybe you have to typedef it instead)
> > 
> > Done. In addition to this I could have also done
> > typedef SaveCardBubbleController::LegalMessageLines Lines;
> > but I preferred to leave it long to make it more visually distinct from
> |Line|.
> 
> By this logic, we shouldn't have both LegalMessageLine and LegalMessageLines
> types (but I think it's fine to).
> 
> > In other words "Line" and "Lines" only differ by one character, whereas
> > "SaveCardBubbleController::LegalMessageLines" and "Line" are obviously
> different
> > things. Also, it's already been typedef'd down to be a bit shorter than the
> > std::vector<SaveCardBubbleControler::LegalMessageLine>
> > that was in the previous revision of this CL.
> 

Good point. I added a LegalMessageLines typedef to this file.

https://codereview.chromium.org/1407093007/diff/220001/chrome/browser/ui/auto...
File chrome/browser/ui/autofill/save_card_bubble_controller.h (right):

https://codereview.chromium.org/1407093007/diff/220001/chrome/browser/ui/auto...
chrome/browser/ui/autofill/save_card_bubble_controller.h:28: LegalMessageLine();
On 2015/11/18 00:21:13, Evan Stade wrote:
> are you sure it's necessary to define this outside of this header file?

Yes. If I get rid of the constructor and destructor I get:
error: [chromium-style] Complex class/struct needs an explicit out-of-line
constructor.
error: [chromium-style] Complex class/struct needs an explicit out-of-line
destructor.

If I try to define inline implementations like this:
    LegalMessageLine() {};
    ~LegalMessageLine() {};

I get:
error: [chromium-style] Complex constructor has an inlined body.
error: [chromium-style] Complex destructor has an inline body.

https://codereview.chromium.org/1407093007/diff/220001/chrome/browser/ui/auto...
File chrome/browser/ui/autofill/save_card_bubble_controller_impl_unittest.cc
(right):

https://codereview.chromium.org/1407093007/diff/220001/chrome/browser/ui/auto...
chrome/browser/ui/autofill/save_card_bubble_controller_impl_unittest.cc:15:
typedef SaveCardBubbleController::LegalMessageLine Line;
On 2015/11/18 00:21:13, Evan Stade wrote:
> you should only use typedef to remove the class name qualifier, not to change
> the type name.

Done.

https://codereview.chromium.org/1407093007/diff/220001/chrome/browser/ui/view...
File chrome/browser/ui/views/autofill/save_card_bubble_views.cc (right):

https://codereview.chromium.org/1407093007/diff/220001/chrome/browser/ui/view...
chrome/browser/ui/views/autofill/save_card_bubble_views.cc:37:
views::StyledLabel* CreateLegalMessageLineLabel(
On 2015/11/18 00:21:13, Evan Stade wrote:
> return scoped_ptr

Done.

https://codereview.chromium.org/1407093007/diff/220001/chrome/browser/ui/view...
chrome/browser/ui/views/autofill/save_card_bubble_views.cc:41: const
std::vector<autofill::SaveCardBubbleController::LegalMessageLine::Link>&
On 2015/11/18 00:21:13, Evan Stade wrote:
> this anonymous namespace should be inside the autofill namespace so you don't
> need autofill:: qualifiers like this one

Done.

https://codereview.chromium.org/1407093007/diff/220001/chrome/browser/ui/view...
chrome/browser/ui/views/autofill/save_card_bubble_views.cc:117:
DCHECK(static_cast<size_t>(label->parent()->child_count()) ==
On 2015/11/18 00:21:13, Evan Stade wrote:
> DCHECK_EQ

Done.

https://codereview.chromium.org/1407093007/diff/220001/chrome/browser/ui/view...
chrome/browser/ui/views/autofill/save_card_bubble_views.cc:119: int line_index =
label->parent()->GetIndexOf(label);
On 2015/11/18 00:21:13, Evan Stade wrote:
> don't need this local variable

Done. Sometimes I prefer breaking a line up with a local variable like this. IMO
it makes it easier to read, and provides a bit of self-documentation. But you
are the OWNER and that's good enough for me. :)

https://codereview.chromium.org/1407093007/diff/220001/chrome/browser/ui/view...
chrome/browser/ui/views/autofill/save_card_bubble_views.cc:128: }
On 2015/11/18 00:21:13, Evan Stade wrote:
> nit: \n

Done.

https://codereview.chromium.org/1407093007/diff/220001/chrome/browser/ui/view...
chrome/browser/ui/views/autofill/save_card_bubble_views.cc:199: int
horizontal_inset = GetBubbleFrameView()->GetTitleInsets().left();
On 2015/11/18 00:21:13, Evan Stade wrote:
> don't need this local var

Done.

https://codereview.chromium.org/1407093007/diff/220001/chrome/browser/ui/view...
chrome/browser/ui/views/autofill/save_card_bubble_views.cc:211:
controller_->GetLegalMessageLines()[i], this);
On 2015/11/18 00:21:13, Evan Stade wrote:
> no need for a local var for this

Done.

https://codereview.chromium.org/1407093007/diff/240001/chrome/browser/ui/view...
File chrome/browser/ui/views/autofill/save_card_bubble_views.cc (right):

https://codereview.chromium.org/1407093007/diff/240001/chrome/browser/ui/view...
chrome/browser/ui/views/autofill/save_card_bubble_views.cc:24: 
Other autofill files seem evenly split on whether there should be a blank line
between these two:
namespace autofill {
namespace {

[Evan Stade, 2015-11-18 22:01:19]

https://codereview.chromium.org/1407093007/diff/220001/chrome/browser/ui/view...
File chrome/browser/ui/views/autofill/save_card_bubble_views.cc (right):

https://codereview.chromium.org/1407093007/diff/220001/chrome/browser/ui/view...
chrome/browser/ui/views/autofill/save_card_bubble_views.cc:119: int line_index =
label->parent()->GetIndexOf(label);
On 2015/11/18 01:59:40, bondd wrote:
> On 2015/11/18 00:21:13, Evan Stade wrote:
> > don't need this local variable
> 
> Done. Sometimes I prefer breaking a line up with a local variable like this.
IMO
> it makes it easier to read, and provides a bit of self-documentation. But you
> are the OWNER and that's good enough for me. :)

my reasoning is that saving it as a local variable sets up the false expectation
that it's used more than once

https://codereview.chromium.org/1407093007/diff/240001/chrome/browser/ui/auto...
File chrome/browser/ui/autofill/save_card_bubble_controller_impl.cc (right):

https://codereview.chromium.org/1407093007/diff/240001/chrome/browser/ui/auto...
chrome/browser/ui/autofill/save_card_bubble_controller_impl.cc:35:
base::string16& out_message,
non const ref outparams still not allowed

https://codereview.chromium.org/1407093007/diff/240001/chrome/browser/ui/auto...
chrome/browser/ui/autofill/save_card_bubble_controller_impl.cc:81:
autofill::SaveCardBubbleController::LegalMessageLine result;
this should already be in the autofill:: namespace

https://codereview.chromium.org/1407093007/diff/240001/chrome/browser/ui/auto...
chrome/browser/ui/autofill/save_card_bubble_controller_impl.cc:89: if
(line.GetList("template_parameter", &template_parameters)) {
display_texts.resize(template_parameters->GetSize());

then you don't need to push_back below. Same for result.links.

This doesn't save any lines of code but it should be marginally more efficient
and imo is more legible (easy to see that results.link.size() ==
display_texts.size())

https://codereview.chromium.org/1407093007/diff/240001/chrome/browser/ui/auto...
chrome/browser/ui/autofill/save_card_bubble_controller_impl.cc:165:
legal_message_lines_.clear();
another place you can resize once instead of pushing back many times

https://codereview.chromium.org/1407093007/diff/240001/chrome/browser/ui/view...
File chrome/browser/ui/views/autofill/save_card_bubble_views.cc (right):

https://codereview.chromium.org/1407093007/diff/240001/chrome/browser/ui/view...
chrome/browser/ui/views/autofill/save_card_bubble_views.cc:46: for (size_t i =
0; i < links.size(); ++i) {
for (const SaveCardBubbleController::LegalMessageLine::Link& link : line.links)

https://codereview.chromium.org/1407093007/diff/240001/chrome/browser/ui/view...
chrome/browser/ui/views/autofill/save_card_bubble_views.cc:121: const
std::vector<SaveCardBubbleController::LegalMessageLine::Link>& links =
I'd say this is a good candidate for auto

const auto& links = foo;
for (const SaveCardBubbleController::LegalMessageLine::Link& link : links) {

}

[bondd, 2015-11-18 23:41:42]

https://codereview.chromium.org/1407093007/diff/240001/chrome/browser/ui/auto...
File chrome/browser/ui/autofill/save_card_bubble_controller_impl.cc (right):

https://codereview.chromium.org/1407093007/diff/240001/chrome/browser/ui/auto...
chrome/browser/ui/autofill/save_card_bubble_controller_impl.cc:35:
base::string16& out_message,
On 2015/11/18 22:01:19, Evan Stade wrote:
> non const ref outparams still not allowed

Gah sorry, I can't believe I missed this one. I saw both of them, then went off
to look everywhere else for more, and came back and only fixed one. Thanks.

https://codereview.chromium.org/1407093007/diff/240001/chrome/browser/ui/auto...
chrome/browser/ui/autofill/save_card_bubble_controller_impl.cc:81:
autofill::SaveCardBubbleController::LegalMessageLine result;
On 2015/11/18 22:01:19, Evan Stade wrote:
> this should already be in the autofill:: namespace

Done.

https://codereview.chromium.org/1407093007/diff/240001/chrome/browser/ui/auto...
chrome/browser/ui/autofill/save_card_bubble_controller_impl.cc:89: if
(line.GetList("template_parameter", &template_parameters)) {
On 2015/11/18 22:01:19, Evan Stade wrote:
> display_texts.resize(template_parameters->GetSize());
> 
> then you don't need to push_back below. Same for result.links.
> 
> This doesn't save any lines of code but it should be marginally more efficient
> and imo is more legible (easy to see that results.link.size() ==
> display_texts.size())
> 

Done.

https://codereview.chromium.org/1407093007/diff/240001/chrome/browser/ui/auto...
chrome/browser/ui/autofill/save_card_bubble_controller_impl.cc:165:
legal_message_lines_.clear();
On 2015/11/18 22:01:19, Evan Stade wrote:
> another place you can resize once instead of pushing back many times

Done.

https://codereview.chromium.org/1407093007/diff/240001/chrome/browser/ui/view...
File chrome/browser/ui/views/autofill/save_card_bubble_views.cc (right):

https://codereview.chromium.org/1407093007/diff/240001/chrome/browser/ui/view...
chrome/browser/ui/views/autofill/save_card_bubble_views.cc:46: for (size_t i =
0; i < links.size(); ++i) {
On 2015/11/18 22:01:19, Evan Stade wrote:
> for (const SaveCardBubbleController::LegalMessageLine::Link& link :
line.links)

Done.

https://codereview.chromium.org/1407093007/diff/240001/chrome/browser/ui/view...
chrome/browser/ui/views/autofill/save_card_bubble_views.cc:121: const
std::vector<SaveCardBubbleController::LegalMessageLine::Link>& links =
On 2015/11/18 22:01:19, Evan Stade wrote:
> I'd say this is a good candidate for auto
> 
> const auto& links = foo;
> for (const SaveCardBubbleController::LegalMessageLine::Link& link : links) {
> 
> }

Done.

[Evan Stade, 2015-11-19 03:22:44]

lgtm

https://codereview.chromium.org/1407093007/diff/260001/chrome/browser/ui/auto...
File chrome/browser/ui/autofill/save_card_bubble_controller_impl.cc (right):

https://codereview.chromium.org/1407093007/diff/260001/chrome/browser/ui/auto...
chrome/browser/ui/autofill/save_card_bubble_controller_impl.cc:103: // Read and
store the "display_text" string.
combine these 3 conditionals

also this comment is evident enough from the code

[bondd, 2015-11-20 02:58:06]

The CQ bit was checked by bondd@chromium.org

[bondd, 2015-11-20 02:58:06]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org,
estade@chromium.org
Link to the patchset: https://codereview.chromium.org/1407093007/#ps280001
(title: "Rebase + collapse 3 if statements.")

[commit-bot: I haz the power, 2015-11-20 02:58:44]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1407093007/280001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1407093007/280001

[bondd, 2015-11-20 02:58:54]

https://codereview.chromium.org/1407093007/diff/260001/chrome/browser/ui/auto...
File chrome/browser/ui/autofill/save_card_bubble_controller_impl.cc (right):

https://codereview.chromium.org/1407093007/diff/260001/chrome/browser/ui/auto...
chrome/browser/ui/autofill/save_card_bubble_controller_impl.cc:103: // Read and
store the "display_text" string.
On 2015/11/19 03:22:43, Evan Stade wrote:
> combine these 3 conditionals
> 
> also this comment is evident enough from the code

Done.

[commit-bot: I haz the power, 2015-11-20 03:11:02]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-11-20 03:11:02]

Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  chromeos_daisy_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  chromeos_x86-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[bondd, 2015-11-20 03:58:08]

The CQ bit was checked by bondd@chromium.org

[bondd, 2015-11-20 03:58:09]

The patchset sent to the CQ was uploaded after l-g-t-m from estade@chromium.org,
sky@chromium.org
Link to the patchset: https://codereview.chromium.org/1407093007/#ps300001
(title: "Rebase.")

[commit-bot: I haz the power, 2015-11-20 03:58:38]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1407093007/300001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1407093007/300001

[commit-bot: I haz the power, 2015-11-20 05:30:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-11-20 05:30:51]

Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_TIMED_OUT, no build
URL)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_TIMED_OUT, no build URL)

[bondd, 2015-11-20 18:35:26]

The CQ bit was checked by bondd@chromium.org

[commit-bot: I haz the power, 2015-11-20 18:37:45]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1407093007/300001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1407093007/300001

[commit-bot: I haz the power, 2015-11-20 18:46:49]

Committed patchset #17 (id:300001)

[commit-bot: I haz the power, 2015-11-20 18:47:23]

Patchset 17 (id:??) landed as
https://crrev.com/dfa5b30be8e0809ee7f4c1c33d3b274fbc61ec80
Cr-Commit-Position: refs/heads/master@{#360866}