Card unmasking without form filling

Card unmasking without form filling

This patch enables users of Autofill Manager to unmask credit cards even
when a credit card form is not being filled. This is accomplished by
refactoring the card unmasking functionality into its own class
FullCardRequest, owned by PersonalDataManager.

The FullCardRequest class is used to retrieve the full PAN and CVC of
the card. If the card is already unmasked, the user is still prompted
for the CVC, but there's no unmasking.

This is primarily useful for PaymentRequest, which provides the full PAN
and the CVC of a card to the JavaScript API.

Only one unmask request per Autofill Manager can be active at a time.
(There's one Autofill Manager per frame.)

The patch also exposes the card unmasking functionality to Java callers
through PersonalDataManager. Because there's only one instance of
PersonalDataManager in Java, only one unmask request from Java can be
active a time.

BUG=587995
Committed: https://crrev.com/f65036c8c638497a88eed1a7278b2d3022d39e4e
Cr-Commit-Position: refs/heads/master@{#389520}

[please use gerrit instead, 2016-04-18 22:33:38]

Patchset #1 (id:1) has been deleted

[please use gerrit instead, 2016-04-18 22:33:44]

Patchset #1 (id:20001) has been deleted

[please use gerrit instead, 2016-04-18 22:33:50]

Patchset #1 (id:40001) has been deleted

[please use gerrit instead, 2016-04-18 22:34:31]

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

[please use gerrit instead, 2016-04-18 22:34:32]

mathp@, ptal.

[Mathieu, 2016-04-19 13:24:25]

I saw only 2 tests mention "unmask" in autofill_manager_unittest. The code is
non trivial (just spent 20 minutes reading through all the callbacks and
delegates), surely there are more opportunities for tests, what do you think?

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

https://codereview.chromium.org/1899893002/diff/60001/components/autofill/cor...
components/autofill/core/browser/autofill_manager.cc:262:
client_->ShowUnmaskPrompt(unmask_request_.card,
can you mention why we're calling ShowUnmaskPrompt if the card is unmasked? I
understand that if called from |autofill_manager_| it will be with a masked
card, but it's not immediately obvious why other callers need this.

https://codereview.chromium.org/1899893002/diff/60001/components/autofill/cor...
components/autofill/core/browser/autofill_manager.cc:1013:
FillCreditCardForm(unmasking_query_id_, unmasking_form_, unmasking_field_,
curious, if the CVC is unused here, where is it taken from when filling the
form?

https://codereview.chromium.org/1899893002/diff/60001/components/autofill/cor...
File components/autofill/core/browser/autofill_manager.h (right):

https://codereview.chromium.org/1899893002/diff/60001/components/autofill/cor...
components/autofill/core/browser/autofill_manager.h:299: // UnmaskCallback:
Mention here or elsewhere why you are not using the cvc

[please use gerrit instead, 2016-04-22 04:03:30]

Description was changed from

==========
Card unmasking without form filling

This patch enables users of Autofill Manager to unmask credit cards even
when a credit card form is not being filled. Only one unmask request per
Autofill Manager can be active at a time. (There's one Autofill Manager
per frame.)

This is primarily useful for PaymentRequest, which provides the unmasked
card details to the JavaScript API without filling any web forms.

The patch also exposes the card unmasking functionality to Java callers
through PersonalDataManager. Because there's only one instance of
PersonalDataManager in Java, only one unmask request from Java can be
active a time.

BUG=587995
==========

to

==========
Card unmasking without form filling

This patch enables users of Autofill Manager to unmask credit cards even
when a credit card form is not being filled. This is accomplished by
refactoring the card unmasking functionality into its own class
FullCardRequest, owned by PersonalDataManager.

The FullCardRequest class is used to retrieve the full PAN and CVC of
the card. If the card is already unmasked, the user is still prompted
for the CVC, but there's no unmasking.

This is primarily useful for PaymentRequest, which provides the unmasked
card details to the JavaScript API without filling any web forms.

Only one unmask request per Autofill Manager can be active at a time.
(There's one Autofill Manager per frame.)

The patch also exposes the card unmasking functionality to Java callers
through PersonalDataManager. Because there's only one instance of
PersonalDataManager in Java, only one unmask request from Java can be
active a time.

BUG=587995
==========

[please use gerrit instead, 2016-04-22 04:04:48]

Description was changed from

==========
Card unmasking without form filling

This patch enables users of Autofill Manager to unmask credit cards even
when a credit card form is not being filled. This is accomplished by
refactoring the card unmasking functionality into its own class
FullCardRequest, owned by PersonalDataManager.

The FullCardRequest class is used to retrieve the full PAN and CVC of
the card. If the card is already unmasked, the user is still prompted
for the CVC, but there's no unmasking.

This is primarily useful for PaymentRequest, which provides the unmasked
card details to the JavaScript API without filling any web forms.

Only one unmask request per Autofill Manager can be active at a time.
(There's one Autofill Manager per frame.)

The patch also exposes the card unmasking functionality to Java callers
through PersonalDataManager. Because there's only one instance of
PersonalDataManager in Java, only one unmask request from Java can be
active a time.

BUG=587995
==========

to

==========
Card unmasking without form filling

This patch enables users of Autofill Manager to unmask credit cards even
when a credit card form is not being filled. This is accomplished by
refactoring the card unmasking functionality into its own class
FullCardRequest, owned by PersonalDataManager.

The FullCardRequest class is used to retrieve the full PAN and CVC of
the card. If the card is already unmasked, the user is still prompted
for the CVC, but there's no unmasking.

This is primarily useful for PaymentRequest, which provides the full PAN
and the CVC of a card to the JavaScript API.

Only one unmask request per Autofill Manager can be active at a time.
(There's one Autofill Manager per frame.)

The patch also exposes the card unmasking functionality to Java callers
through PersonalDataManager. Because there's only one instance of
PersonalDataManager in Java, only one unmask request from Java can be
active a time.

BUG=587995
==========

[please use gerrit instead, 2016-04-22 04:05:36]

mathp@, ptal patch 5.

[Mathieu, 2016-04-22 17:58:36]

Hi rouslan, love the latest patch, just a few questions. Thanks

https://codereview.chromium.org/1899893002/diff/140001/chrome/browser/autofil...
File chrome/browser/autofill/android/personal_data_manager_android.h (right):

https://codereview.chromium.org/1899893002/diff/140001/chrome/browser/autofil...
chrome/browser/autofill/android/personal_data_manager_android.h:106: // Gets the
card CVC and the unmasks the card (if it's masked).
*and unmasks

https://codereview.chromium.org/1899893002/diff/140001/components/autofill/co...
File components/autofill/core/browser/autofill_manager.cc (right):

https://codereview.chromium.org/1899893002/diff/140001/components/autofill/co...
components/autofill/core/browser/autofill_manager.cc:625: true /* is_credit_card
*/, base::string16());
nit: I would add /* cvc */ here and below

https://codereview.chromium.org/1899893002/diff/140001/components/autofill/co...
components/autofill/core/browser/autofill_manager.cc:830: FullCardRequest*
AutofillManager::full_card_request() {
nit: A getter with non trivial logic should probably have an Camel-case name
that is more descriptive such as GetOrCreateFullCardRequest

https://codereview.chromium.org/1899893002/diff/140001/components/autofill/co...
File components/autofill/core/browser/full_card_request.cc (right):

https://codereview.chromium.org/1899893002/diff/140001/components/autofill/co...
components/autofill/core/browser/full_card_request.cc:23:
payments_client_(url_getter, this),
could PaymentsClient be an argument to the constructor? We already have one in
AutofillManager (haven't looked in other places).

https://codereview.chromium.org/1899893002/diff/140001/components/autofill/co...
components/autofill/core/browser/full_card_request.cc:35: if (delegate_) {
Please add a comment since this is not immediately clear to the reader
(delegate_ vs delegate)

https://codereview.chromium.org/1899893002/diff/140001/components/autofill/co...
File components/autofill/core/browser/full_card_request.h (right):

https://codereview.chromium.org/1899893002/diff/140001/components/autofill/co...
components/autofill/core/browser/full_card_request.h:5: #ifndef
COMPONENTS_AUTOFILL_CORE_BROWSER_FULL_CARD_REQUEST_H_
should it be under core/browser/payments (alongside payments_client)?

[please use gerrit instead, 2016-04-22 20:50:19]

mathp@, ptal patch 6.

https://codereview.chromium.org/1899893002/diff/140001/chrome/browser/autofil...
File chrome/browser/autofill/android/personal_data_manager_android.h (right):

https://codereview.chromium.org/1899893002/diff/140001/chrome/browser/autofil...
chrome/browser/autofill/android/personal_data_manager_android.h:106: // Gets the
card CVC and the unmasks the card (if it's masked).
On 2016/04/22 17:58:35, Mathieu Perreault wrote:
> *and unmasks

Done.

https://codereview.chromium.org/1899893002/diff/140001/components/autofill/co...
File components/autofill/core/browser/autofill_manager.cc (right):

https://codereview.chromium.org/1899893002/diff/140001/components/autofill/co...
components/autofill/core/browser/autofill_manager.cc:625: true /* is_credit_card
*/, base::string16());
On 2016/04/22 17:58:35, Mathieu Perreault wrote:
> nit: I would add /* cvc */ here and below

Done.

https://codereview.chromium.org/1899893002/diff/140001/components/autofill/co...
components/autofill/core/browser/autofill_manager.cc:830: FullCardRequest*
AutofillManager::full_card_request() {
On 2016/04/22 17:58:35, Mathieu Perreault wrote:
> nit: A getter with non trivial logic should probably have an Camel-case name
> that is more descriptive such as GetOrCreateFullCardRequest

Done.

https://codereview.chromium.org/1899893002/diff/140001/components/autofill/co...
File components/autofill/core/browser/full_card_request.cc (right):

https://codereview.chromium.org/1899893002/diff/140001/components/autofill/co...
components/autofill/core/browser/full_card_request.cc:23:
payments_client_(url_getter, this),
On 2016/04/22 17:58:35, Mathieu Perreault wrote:
> could PaymentsClient be an argument to the constructor? We already have one in
> AutofillManager (haven't looked in other places).

Done.

https://codereview.chromium.org/1899893002/diff/140001/components/autofill/co...
components/autofill/core/browser/full_card_request.cc:35: if (delegate_) {
On 2016/04/22 17:58:35, Mathieu Perreault wrote:
> Please add a comment since this is not immediately clear to the reader
> (delegate_ vs delegate)

Done.

https://codereview.chromium.org/1899893002/diff/140001/components/autofill/co...
File components/autofill/core/browser/full_card_request.h (right):

https://codereview.chromium.org/1899893002/diff/140001/components/autofill/co...
components/autofill/core/browser/full_card_request.h:5: #ifndef
COMPONENTS_AUTOFILL_CORE_BROWSER_FULL_CARD_REQUEST_H_
On 2016/04/22 17:58:35, Mathieu Perreault wrote:
> should it be under core/browser/payments (alongside payments_client)?

Done.

[Mathieu, 2016-04-22 21:18:26]

lgtm, thanks a lot

[please use gerrit instead, 2016-04-22 23:02:09]

rouslan@chromium.org changed reviewers:
+ tedchoc@chromium.org

[please use gerrit instead, 2016-04-22 23:02:10]

tedchoc@, ptal PersonalDataManager.java in patch 6.

[Ted C, 2016-04-22 23:34:56]

You can see similar comments I made on a recent change about passing down the
java bits:

https://codereview.chromium.org/1757163002/

Little more complicated on this change as you need a wrapping class, but
shouldn't be too bad.

https://codereview.chromium.org/1899893002/diff/160001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalDataManager.java
(right):

https://codereview.chromium.org/1899893002/diff/160001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalDataManager.java:52:
public abstract void onFullCardDetails(CreditCard card, String cvc);
drop public and abstract in interfaces

https://codereview.chromium.org/1899893002/diff/160001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalDataManager.java:536:
if (mFullCardRequestDelegate != null) {
this looks fishy to me.  you cancel the previous delegate, but won't the new
delegate get called with the pending onFullCardDetails that comes next?

I think you should pass down the delegate.  Add
@CalledByNative("FullCardRequestDelegate") to the interface methods above. 
Create a inner class that keeps a scoped global ref to the java object and
extends the c++ FullCardRequestDelegate, calling the corresponding methods on
the interface above.  No need to track delegates and allows multiple callers w/o
chances of scary leakages.

[please use gerrit instead, 2016-04-23 01:17:43]

tedchoc@, ptal patch 8.

jdonnelly@, as the resident iOS guru, can you help me figure out why the iOS
unit tests are failing?

https://codereview.chromium.org/1899893002/diff/160001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalDataManager.java
(right):

https://codereview.chromium.org/1899893002/diff/160001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalDataManager.java:52:
public abstract void onFullCardDetails(CreditCard card, String cvc);
On 2016/04/22 23:34:56, Ted C wrote:
> drop public and abstract in interfaces

Done.

https://codereview.chromium.org/1899893002/diff/160001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalDataManager.java:536:
if (mFullCardRequestDelegate != null) {
On 2016/04/22 23:34:56, Ted C wrote:
> this looks fishy to me.  you cancel the previous delegate, but won't the new
> delegate get called with the pending onFullCardDetails that comes next?
> 
> I think you should pass down the delegate.  Add
> @CalledByNative("FullCardRequestDelegate") to the interface methods above. 
> Create a inner class that keeps a scoped global ref to the java object and
> extends the c++ FullCardRequestDelegate, calling the corresponding methods on
> the interface above.  No need to track delegates and allows multiple callers
w/o
> chances of scary leakages.

Done.

[please use gerrit instead, 2016-04-23 02:30:42]

On 2016/04/23 01:17:43, Rouslan wrote:
> jdonnelly@, as the resident iOS guru, can you help me figure out why the iOS
> unit tests are failing?

Nevermind, I've figured out that iOS single field autofill needed to be on
credit card number field in order to show the card unmasking prompt.

[Ted C, 2016-04-25 16:01:07]

lgtm

[please use gerrit instead, 2016-04-25 16:06:21]

The CQ bit was checked by rouslan@chromium.org

[please use gerrit instead, 2016-04-25 16:06:21]

The patchset sent to the CQ was uploaded after l-g-t-m from mathp@chromium.org
Link to the patchset: https://codereview.chromium.org/1899893002/#ps240001
(title: "Use the credit card number field in metrics tests, because ios
single-field form fill will not requ…")

[commit-bot: I haz the power, 2016-04-25 16:06:48]

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

[commit-bot: I haz the power, 2016-04-25 18:35:51]

Committed patchset #10 (id:240001)

[commit-bot: I haz the power, 2016-04-25 18:38:13]

Description was changed from

==========
Card unmasking without form filling

This patch enables users of Autofill Manager to unmask credit cards even
when a credit card form is not being filled. This is accomplished by
refactoring the card unmasking functionality into its own class
FullCardRequest, owned by PersonalDataManager.

The FullCardRequest class is used to retrieve the full PAN and CVC of
the card. If the card is already unmasked, the user is still prompted
for the CVC, but there's no unmasking.

This is primarily useful for PaymentRequest, which provides the full PAN
and the CVC of a card to the JavaScript API.

Only one unmask request per Autofill Manager can be active at a time.
(There's one Autofill Manager per frame.)

The patch also exposes the card unmasking functionality to Java callers
through PersonalDataManager. Because there's only one instance of
PersonalDataManager in Java, only one unmask request from Java can be
active a time.

BUG=587995
==========

to

==========
Card unmasking without form filling

This patch enables users of Autofill Manager to unmask credit cards even
when a credit card form is not being filled. This is accomplished by
refactoring the card unmasking functionality into its own class
FullCardRequest, owned by PersonalDataManager.

The FullCardRequest class is used to retrieve the full PAN and CVC of
the card. If the card is already unmasked, the user is still prompted
for the CVC, but there's no unmasking.

This is primarily useful for PaymentRequest, which provides the full PAN
and the CVC of a card to the JavaScript API.

Only one unmask request per Autofill Manager can be active at a time.
(There's one Autofill Manager per frame.)

The patch also exposes the card unmasking functionality to Java callers
through PersonalDataManager. Because there's only one instance of
PersonalDataManager in Java, only one unmask request from Java can be
active a time.

BUG=587995

Committed: https://crrev.com/f65036c8c638497a88eed1a7278b2d3022d39e4e
Cr-Commit-Position: refs/heads/master@{#389520}
==========

[commit-bot: I haz the power, 2016-04-25 18:38:14]

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