AutofillSaveCardInfoBar on mobile should not appear when legal messages don't parse correctly

AutofillSaveCardInfoBar on mobile should not appear when legal messages don't parse correctly

BUG=734219
Review-Url: https://codereview.chromium.org/2945563003
Cr-Commit-Position: refs/heads/master@{#480980}
Committed: https://chromium.googlesource.com/chromium/src/+/7a5bde6f7b1b04cb4ccc0cf75e4c315130c85a44

[Jared Saul, 2017-06-16 21:48:51]

Description was changed from

==========
AutofillSaveCardInfoBar on mobile should not appear when legal messages don't
parse correctly

BUG=
==========

to

==========
AutofillSaveCardInfoBar on mobile should not appear when legal messages don't
parse correctly

BUG=734219
==========

[Jared Saul, 2017-06-16 21:56:35]

jsaul@google.com changed reviewers:
+ mathp@chromium.org, sebsg@chromium.org

[Jared Saul, 2017-06-16 21:56:35]

jsaul@google.com changed required reviewers:
+ mathp@chromium.org

[Jared Saul, 2017-06-16 21:56:36]

Hi Mathieu and Seb, PTAL.  This CL is based on this legal message parsing check
for Upstream on Desktop, which Upstream on Android was lacking:
https://cs.chromium.org/chromium/src/chrome/browser/ui/autofill/save_card_bub...

[sebsg, 2017-06-19 14:12:50]

LGTM

[Mathieu, 2017-06-20 00:36:21]

lgtm, sorry for the delay

https://codereview.chromium.org/2945563003/diff/1/chrome/browser/autofill/aut...
File
chrome/browser/autofill/autofill_save_card_infobar_delegate_mobile_unittest.cc
(right):

https://codereview.chromium.org/2945563003/diff/1/chrome/browser/autofill/aut...
chrome/browser/autofill/autofill_save_card_infobar_delegate_mobile_unittest.cc:112:
is_uploading, legal_message_string,
here you could pass "" if you apply my comment below

https://codereview.chromium.org/2945563003/diff/1/chrome/browser/autofill/aut...
chrome/browser/autofill/autofill_save_card_infobar_delegate_mobile_unittest.cc:119:
std::string& legal_message_string,
Any reason you are using a ref? For this test, std::string would be less
confusing

https://codereview.chromium.org/2945563003/diff/1/chrome/browser/autofill/aut...
chrome/browser/autofill/autofill_save_card_infobar_delegate_mobile_unittest.cc:251:
/* is_uploading= */ true, good_legal_message,
with my proposed change you can std::move(good_legal_message) here

https://codereview.chromium.org/2945563003/diff/1/components/autofill/core/br...
File
components/autofill/core/browser/autofill_save_card_infobar_delegate_mobile.cc
(right):

https://codereview.chromium.org/2945563003/diff/1/components/autofill/core/br...
components/autofill/core/browser/autofill_save_card_infobar_delegate_mobile.cc:74:
// If upload save and legal_messages_ is empty then fail, else succeed.
nit: If we are uploading to the server, verify that legal lines have been parsed
into |legal_messages_|.

https://codereview.chromium.org/2945563003/diff/1/components/autofill/core/br...
File
components/autofill/core/browser/autofill_save_card_infobar_delegate_mobile.h
(right):

https://codereview.chromium.org/2945563003/diff/1/components/autofill/core/br...
components/autofill/core/browser/autofill_save_card_infobar_delegate_mobile.h:48:
// Ensures the InfoBar is not shown if legal messages failed to parse.
maybe note that legal messages are only specified for the upload case?

[Jared Saul, 2017-06-20 20:51:27]

Changes made; thanks for the review!

https://codereview.chromium.org/2945563003/diff/1/chrome/browser/autofill/aut...
File
chrome/browser/autofill/autofill_save_card_infobar_delegate_mobile_unittest.cc
(right):

https://codereview.chromium.org/2945563003/diff/1/chrome/browser/autofill/aut...
chrome/browser/autofill/autofill_save_card_infobar_delegate_mobile_unittest.cc:112:
is_uploading, legal_message_string,
On 2017/06/20 00:36:20, Mathieu wrote:
> here you could pass "" if you apply my comment below

Done.

https://codereview.chromium.org/2945563003/diff/1/chrome/browser/autofill/aut...
chrome/browser/autofill/autofill_save_card_infobar_delegate_mobile_unittest.cc:119:
std::string& legal_message_string,
On 2017/06/20 00:36:20, Mathieu wrote:
> Any reason you are using a ref? For this test, std::string would be less
> confusing

Done, thank you.

https://codereview.chromium.org/2945563003/diff/1/chrome/browser/autofill/aut...
chrome/browser/autofill/autofill_save_card_infobar_delegate_mobile_unittest.cc:251:
/* is_uploading= */ true, good_legal_message,
On 2017/06/20 00:36:20, Mathieu wrote:
> with my proposed change you can std::move(good_legal_message) here

Done.

https://codereview.chromium.org/2945563003/diff/1/components/autofill/core/br...
File
components/autofill/core/browser/autofill_save_card_infobar_delegate_mobile.cc
(right):

https://codereview.chromium.org/2945563003/diff/1/components/autofill/core/br...
components/autofill/core/browser/autofill_save_card_infobar_delegate_mobile.cc:74:
// If upload save and legal_messages_ is empty then fail, else succeed.
On 2017/06/20 00:36:20, Mathieu wrote:
> nit: If we are uploading to the server, verify that legal lines have been
parsed
> into |legal_messages_|. 

Done, thanks!

https://codereview.chromium.org/2945563003/diff/1/components/autofill/core/br...
File
components/autofill/core/browser/autofill_save_card_infobar_delegate_mobile.h
(right):

https://codereview.chromium.org/2945563003/diff/1/components/autofill/core/br...
components/autofill/core/browser/autofill_save_card_infobar_delegate_mobile.h:48:
// Ensures the InfoBar is not shown if legal messages failed to parse.
On 2017/06/20 00:36:21, Mathieu wrote:
> maybe note that legal messages are only specified for the upload case?

Done.

[Jared Saul, 2017-06-20 20:53:41]

The CQ bit was checked by jsaul@google.com

[Jared Saul, 2017-06-20 20:53:41]

The patchset sent to the CQ was uploaded after l-g-t-m from mathp@chromium.org,
sebsg@chromium.org
Link to the patchset: https://codereview.chromium.org/2945563003/#ps20001
(title: "Code review changes")

[commit-bot: I haz the power, 2017-06-20 20:54:02]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-20 22:01:58]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1497992021350740,
"parent_rev": "d0a82df53c829fe6b9768c98aa1a86936b2cd774", "commit_rev":
"7a5bde6f7b1b04cb4ccc0cf75e4c315130c85a44"}

[commit-bot: I haz the power, 2017-06-20 22:02:10]

Description was changed from

==========
AutofillSaveCardInfoBar on mobile should not appear when legal messages don't
parse correctly

BUG=734219
==========

to

==========
AutofillSaveCardInfoBar on mobile should not appear when legal messages don't
parse correctly

BUG=734219

Review-Url: https://codereview.chromium.org/2945563003
Cr-Commit-Position: refs/heads/master@{#480980}
Committed:
https://chromium.googlesource.com/chromium/src/+/7a5bde6f7b1b04cb4ccc0cf75e4c...
==========

[commit-bot: I haz the power, 2017-06-20 22:02:12]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/7a5bde6f7b1b04cb4ccc0cf75e4c...