Stores server card as a full server card when upload to server succeeds.

Stores server card as a full server card when upload to server succeeds.

BUG=714290
Review-Url: https://codereview.chromium.org/2829853008
Cr-Commit-Position: refs/heads/master@{#466792}
Committed: https://chromium.googlesource.com/chromium/src/+/94ecc0403ad6cdd384b574c07aed90afc8f2d1af

[csashi, 2017-04-21 22:14:22]

The CQ bit was checked by csashi@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-04-21 22:15:14]

Dry run: CQ is trying da patch.

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

[csashi, 2017-04-21 22:19:31]

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

[csashi, 2017-04-21 22:19:31]

Hi Mathieu,
Please review changes in components/autofill (Please reassign if needed to load
balance).

Hi Rouslan,
Please review changes in chrome/browser/ui/views/payments/ - It is a boilerplate
change.

Hi Jared,
Please review the parsing of credit-card server id server response in
components/autofill/core/browser/payments/payments_client.cc.

Thanks!
-sashi.

[commit-bot: I haz the power, 2017-04-21 22:37:23]

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

[commit-bot: I haz the power, 2017-04-21 22:37:24]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[csashi, 2017-04-21 23:19:59]

The CQ bit was checked by csashi@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-04-21 23:20:50]

Dry run: CQ is trying da patch.

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

[Jared Saul, 2017-04-21 23:29:10]

lgtm for credit_card_id

https://codereview.chromium.org/2829853008/diff/20001/components/autofill/cor...
File components/autofill/core/browser/webdata/autofill_table.h (right):

https://codereview.chromium.org/2829853008/diff/20001/components/autofill/cor...
components/autofill/core/browser/webdata/autofill_table.h:172: //               
      available on saving card to serevr, it will be stored
s/serevr/server
(nit: s/on/upon)

https://codereview.chromium.org/2829853008/diff/20001/components/autofill/cor...
components/autofill/core/browser/webdata/autofill_table.h:362: // Adds to the
masked_credit_cards and unmasked_credit_cards table.
nit: s/table/tables

[csashi, 2017-04-21 23:39:50]

https://codereview.chromium.org/2829853008/diff/20001/components/autofill/cor...
File components/autofill/core/browser/webdata/autofill_table.h (right):

https://codereview.chromium.org/2829853008/diff/20001/components/autofill/cor...
components/autofill/core/browser/webdata/autofill_table.h:172: //               
      available on saving card to serevr, it will be stored
On 2017/04/21 23:29:10, Jared Saul wrote:
> s/serevr/server
> (nit: s/on/upon)

Done.

https://codereview.chromium.org/2829853008/diff/20001/components/autofill/cor...
components/autofill/core/browser/webdata/autofill_table.h:362: // Adds to the
masked_credit_cards and unmasked_credit_cards table.
On 2017/04/21 23:29:10, Jared Saul wrote:
> nit: s/table/tables

Done.

[csashi, 2017-04-21 23:59:27]

The CQ bit was checked by csashi@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-04-21 23:59:53]

Dry run: 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-04-22 00:25:26]

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

[commit-bot: I haz the power, 2017-04-22 00:25:28]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[csashi, 2017-04-22 03:43:08]

The CQ bit was checked by csashi@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-04-22 03:43:22]

Dry run: CQ is trying da patch.

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

[csashi, 2017-04-22 04:03:44]

The CQ bit was checked by csashi@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-04-22 04:03:58]

Dry run: 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-04-22 04:51:11]

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

[commit-bot: I haz the power, 2017-04-22 04:51:11]

Dry run: This issue passed the CQ dry run.

[please use gerrit instead, 2017-04-24 13:57:55]

chrome/browser/ui/views/payments/ lgtm

[Mathieu, 2017-04-24 18:11:54]

mathp@chromium.org changed reviewers:
+ sebsg@chromium.org

[Mathieu, 2017-04-24 18:11:55]

lgtm but I'll have sebsg@ have a look because he touched at all this code
before.

https://codereview.chromium.org/2829853008/diff/80001/components/autofill/cor...
File components/autofill/core/browser/autofill_manager.cc (right):

https://codereview.chromium.org/2829853008/diff/80001/components/autofill/cor...
components/autofill/core/browser/autofill_manager.cc:1089: // TODO(jdonnelly):
Log duration.
we should either transform this into a TODO(crbug.com/xxx) or delete it,
depending on whether we still care about this metric.

https://codereview.chromium.org/2829853008/diff/80001/components/autofill/cor...
File components/autofill/core/browser/autofill_manager_unittest.cc (right):

https://codereview.chromium.org/2829853008/diff/80001/components/autofill/cor...
components/autofill/core/browser/autofill_manager_unittest.cc:136: class
TestPersonalDataManager : public PersonalDataManager {
I wonder if we could subclass
components/autofill/core/browser/test_personal_data_manager.h and save a bunch
of lines here?

https://codereview.chromium.org/2829853008/diff/80001/components/autofill/cor...
File components/autofill/core/browser/webdata/autofill_table.cc (right):

https://codereview.chromium.org/2829853008/diff/80001/components/autofill/cor...
components/autofill/core/browser/webdata/autofill_table.cc:1366:
AddMaskedCreditCards({masked});
was it a practice before to have a copy of the card in both masked and unmasked
tables? It appears so, but I can't figure out why and I don't have strong
knowledge of this code.

[csashi, 2017-04-24 18:30:07]

https://codereview.chromium.org/2829853008/diff/80001/components/autofill/cor...
File components/autofill/core/browser/autofill_manager.cc (right):

https://codereview.chromium.org/2829853008/diff/80001/components/autofill/cor...
components/autofill/core/browser/autofill_manager.cc:1089: // TODO(jdonnelly):
Log duration.
On 2017/04/24 18:11:55, Mathieu wrote:
> we should either transform this into a TODO(crbug.com/xxx) or delete it,
> depending on whether we still care about this metric.

Done.

https://codereview.chromium.org/2829853008/diff/80001/components/autofill/cor...
File components/autofill/core/browser/autofill_manager_unittest.cc (right):

https://codereview.chromium.org/2829853008/diff/80001/components/autofill/cor...
components/autofill/core/browser/autofill_manager_unittest.cc:136: class
TestPersonalDataManager : public PersonalDataManager {
On 2017/04/24 18:11:55, Mathieu wrote:
> I wonder if we could subclass
> components/autofill/core/browser/test_personal_data_manager.h and save a bunch
> of lines here?

Looks like the savings would be minimal and we would need to override some
important functions like GetCreditCards.

https://codereview.chromium.org/2829853008/diff/80001/components/autofill/cor...
File components/autofill/core/browser/webdata/autofill_table.cc (right):

https://codereview.chromium.org/2829853008/diff/80001/components/autofill/cor...
components/autofill/core/browser/webdata/autofill_table.cc:1366:
AddMaskedCreditCards({masked});
On 2017/04/24 18:11:55, Mathieu wrote:
> was it a practice before to have a copy of the card in both masked and
unmasked
> tables? It appears so, but I can't figure out why and I don't have strong
> knowledge of this code.

From what I understand, yes.
If card is masked it has an entry only in masked table.
If card is unmasked it has an entry (joined by id) in both masked and unmasked
tables.

Please feel free to add other reviewers who may have more familiarity. Thanks!

[sebsg, 2017-04-24 19:18:23]

lgtm % 2 comments

https://codereview.chromium.org/2829853008/diff/80001/components/autofill/cor...
File components/autofill/core/browser/webdata/autofill_table.cc (right):

https://codereview.chromium.org/2829853008/diff/80001/components/autofill/cor...
components/autofill/core/browser/webdata/autofill_table.cc:1366:
AddMaskedCreditCards({masked});
On 2017/04/24 18:30:06, csashi wrote:
> On 2017/04/24 18:11:55, Mathieu wrote:
> > was it a practice before to have a copy of the card in both masked and
> unmasked
> > tables? It appears so, but I can't figure out why and I don't have strong
> > knowledge of this code.
> 
> From what I understand, yes.
> If card is masked it has an entry only in masked table.
> If card is unmasked it has an entry (joined by id) in both masked and unmasked
> tables.
> 
> Please feel free to add other reviewers who may have more familiarity. Thanks!

That's exacly it Sashi.

https://codereview.chromium.org/2829853008/diff/100001/components/autofill/co...
File components/autofill/core/browser/personal_data_manager.h (right):

https://codereview.chromium.org/2829853008/diff/100001/components/autofill/co...
components/autofill/core/browser/personal_data_manager.h:156: virtual void
AddServerCreditCard(const CreditCard& credit_card);
Maybe name it AddFullServerCreditCard? This would make it super clear, WDYT?

https://codereview.chromium.org/2829853008/diff/100001/components/autofill/co...
File components/autofill/core/browser/webdata/autofill_table.h (right):

https://codereview.chromium.org/2829853008/diff/100001/components/autofill/co...
components/autofill/core/browser/webdata/autofill_table.h:540: // Adds to
|masked_credit_cards| and updates |server_card_metadata|.
Can you add a comment that these should be called as part of a transaction?

Thanks!

[csashi, 2017-04-24 20:09:02]

https://codereview.chromium.org/2829853008/diff/80001/components/autofill/cor...
File components/autofill/core/browser/webdata/autofill_table.cc (right):

https://codereview.chromium.org/2829853008/diff/80001/components/autofill/cor...
components/autofill/core/browser/webdata/autofill_table.cc:1366:
AddMaskedCreditCards({masked});
On 2017/04/24 19:18:23, sebsg wrote:
> On 2017/04/24 18:30:06, csashi wrote:
> > On 2017/04/24 18:11:55, Mathieu wrote:
> > > was it a practice before to have a copy of the card in both masked and
> > unmasked
> > > tables? It appears so, but I can't figure out why and I don't have strong
> > > knowledge of this code.
> > 
> > From what I understand, yes.
> > If card is masked it has an entry only in masked table.
> > If card is unmasked it has an entry (joined by id) in both masked and
unmasked
> > tables.
> > 
> > Please feel free to add other reviewers who may have more familiarity.
Thanks!
> 
> That's exacly it Sashi.

Acknowledged.

https://codereview.chromium.org/2829853008/diff/100001/components/autofill/co...
File components/autofill/core/browser/personal_data_manager.h (right):

https://codereview.chromium.org/2829853008/diff/100001/components/autofill/co...
components/autofill/core/browser/personal_data_manager.h:156: virtual void
AddServerCreditCard(const CreditCard& credit_card);
On 2017/04/24 19:18:23, sebsg wrote:
> Maybe name it AddFullServerCreditCard? This would make it super clear, WDYT?

Done.

https://codereview.chromium.org/2829853008/diff/100001/components/autofill/co...
File components/autofill/core/browser/webdata/autofill_table.h (right):

https://codereview.chromium.org/2829853008/diff/100001/components/autofill/co...
components/autofill/core/browser/webdata/autofill_table.h:540: // Adds to
|masked_credit_cards| and updates |server_card_metadata|.
On 2017/04/24 19:18:23, sebsg wrote:
> Can you add a comment that these should be called as part of a transaction?
> 
> Thanks!

Done. I also add a debug CHECK. Turns out that we were not starting a
transaction explicitly in UnmaskServerCreditCard - I changed that to start an
explicit transaction and commit it.

[csashi, 2017-04-24 20:09:15]

The CQ bit was checked by csashi@google.com

[csashi, 2017-04-24 20:09:16]

The patchset sent to the CQ was uploaded after l-g-t-m from jsaul@google.com,
mathp@chromium.org, rouslan@chromium.org, sebsg@chromium.org
Link to the patchset: https://codereview.chromium.org/2829853008/#ps120001
(title: "Renames AddServerCreditCard to AddFullServerCreditCard. Adds DCHECK to
verify that we are in a tran…")

[commit-bot: I haz the power, 2017-04-24 20:10:18]

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-04-24 20:16:15]

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

[commit-bot: I haz the power, 2017-04-24 20:16:17]

Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[csashi, 2017-04-24 21:22:08]

The CQ bit was checked by csashi@google.com

[csashi, 2017-04-24 21:22:09]

The patchset sent to the CQ was uploaded after l-g-t-m from mathp@chromium.org,
rouslan@chromium.org, jsaul@google.com, sebsg@chromium.org
Link to the patchset: https://codereview.chromium.org/2829853008/#ps140001
(title: "Renames AddServerCreditCard to AddFullServerCreditCard. Adds DCHECK to
verify that we are in a tran…")

[commit-bot: I haz the power, 2017-04-24 21:22:36]

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-04-24 22:21:00]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1493068928773370,
"parent_rev": "27f4fa45a207b4bd813a5c1bc5875e4696080a4c", "commit_rev":
"94ecc0403ad6cdd384b574c07aed90afc8f2d1af"}

[commit-bot: I haz the power, 2017-04-24 22:21:11]

Description was changed from

==========
Stores server card as a full server card when upload to server succeeds.

BUG=714290
==========

to

==========
Stores server card as a full server card when upload to server succeeds.

BUG=714290

Review-Url: https://codereview.chromium.org/2829853008
Cr-Commit-Position: refs/heads/master@{#466792}
Committed:
https://chromium.googlesource.com/chromium/src/+/94ecc0403ad6cdd384b574c07aed...
==========

[commit-bot: I haz the power, 2017-04-24 22:21:21]

Committed patchset #8 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/94ecc0403ad6cdd384b574c07aed...