Separate credit card scanner from native bridge.

Separate credit card scanner from native bridge.

This patch enables using credit card scanner directly from Java, without
jumping to native first. Checking of the "AutofillScanCardholderName"
flag has moved into the Java delegate wrapper, so users of this
functionality in Java are governed by the same flag, regardless of the
implementation.

BUG=640430
Committed: https://crrev.com/8bb5bcc3e855e57293999433cab3a1cb8971597b
Cr-Commit-Position: refs/heads/master@{#415146}

[please use gerrit instead, 2016-08-24 22:59:23]

Description was changed from

==========
Separate credit card scanner from native bridge.

This patch enables using credit card scanner directly from Java, without
jumping to native first.

BUG=640430
==========

to

==========
Separate credit card scanner from native bridge.

This patch enables using credit card scanner directly from Java, without
jumping to native first. Checking of the "AutofillScanCardholderName"
flag has moved into the Java implementation of the scanner, so users of
this functionality in Java are governed by the same flag.

BUG=640430
==========

[please use gerrit instead, 2016-08-24 23:00:44]

The CQ bit was checked by rouslan@chromium.org to run a CQ dry run

[please use gerrit instead, 2016-08-24 23:00:55]

Patchset #1 (id:1) has been deleted

[commit-bot: I haz the power, 2016-08-24 23:02:48]

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, 2016-08-25 01:41:25]

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

[commit-bot: I haz the power, 2016-08-25 01:41:26]

Dry run: This issue passed the CQ dry run.

[please use gerrit instead, 2016-08-25 17:31:44]

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

[please use gerrit instead, 2016-08-25 17:31:45]

dfalcantara, ptal.

[gone, 2016-08-26 18:27:07]

lgtm, but it's a little weird that the class names don't line up.

Java:
CreditCardScanner
CreditCardScannerBridge

Native:
CreditCardScannerViewAndroid

[please use gerrit instead, 2016-08-26 22:20:09]

On 2016/08/26 18:27:07, dfalcantara wrote:
> lgtm, but it's a little weird that the class names don't line up.
> 
> Java:
> CreditCardScanner
> CreditCardScannerBridge
> 
> Native:
> CreditCardScannerViewAndroid

I'm committing this as-is for now and will think about a good naming scheme, if
that's OK with you.

[please use gerrit instead, 2016-08-26 22:21:24]

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

[please use gerrit instead, 2016-08-26 22:21:24]

msw, owners ptal
chrome/browser/ui/android/autofill/credit_card_scanner_view_android.cc

[msw, 2016-08-26 22:37:07]

https://codereview.chromium.org/2274693004/diff/20001/chrome/browser/ui/andro...
File chrome/browser/ui/android/autofill/credit_card_scanner_view_android.cc
(left):

https://codereview.chromium.org/2274693004/diff/20001/chrome/browser/ui/andro...
chrome/browser/ui/android/autofill/credit_card_scanner_view_android.cc:83: if
(base::FeatureList::IsEnabled(kAutofillScanCardholderName)) {
Where exactly is this checked now? I don't see it in your CL.

[please use gerrit instead, 2016-08-27 01:40:03]

The CQ bit was checked by rouslan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-27 01:40:34]

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, 2016-08-27 03:02:29]

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

[commit-bot: I haz the power, 2016-08-27 03:02:30]

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_...)

[please use gerrit instead, 2016-08-29 17:54:59]

Description was changed from

==========
Separate credit card scanner from native bridge.

This patch enables using credit card scanner directly from Java, without
jumping to native first. Checking of the "AutofillScanCardholderName"
flag has moved into the Java implementation of the scanner, so users of
this functionality in Java are governed by the same flag.

BUG=640430
==========

to

==========
Separate credit card scanner from native bridge.

This patch enables using credit card scanner directly from Java, without
jumping to native first. Checking of the "AutofillScanCardholderName"
flag has moved into the Java delegate wrapper, so users of this
functionality in Java are governed by the same flag, regardless of the
implementation.

BUG=640430
==========

[please use gerrit instead, 2016-08-29 17:58:31]

msw, ptal patch 2.

https://codereview.chromium.org/2274693004/diff/20001/chrome/browser/ui/andro...
File chrome/browser/ui/android/autofill/credit_card_scanner_view_android.cc
(left):

https://codereview.chromium.org/2274693004/diff/20001/chrome/browser/ui/andro...
chrome/browser/ui/android/autofill/credit_card_scanner_view_android.cc:83: if
(base::FeatureList::IsEnabled(kAutofillScanCardholderName)) {
On 2016/08/26 22:37:07, msw wrote:
> Where exactly is this checked now? I don't see it in your CL.

I was going to rely on the implementation of the scanner to check the flag, but
I realize that's hard to review. Therefore, I've created a DelegateWrapper class
inside of the CreditCardScanner.java. The wrapper checks the flag to see whether
the cardHolderName needs to be cleared, regardless of what the implementation of
the scanner does.

[msw, 2016-08-29 18:19:19]

https://codereview.chromium.org/2274693004/diff/20001/chrome/browser/ui/andro...
File chrome/browser/ui/android/autofill/credit_card_scanner_view_android.cc
(left):

https://codereview.chromium.org/2274693004/diff/20001/chrome/browser/ui/andro...
chrome/browser/ui/android/autofill/credit_card_scanner_view_android.cc:83: if
(base::FeatureList::IsEnabled(kAutofillScanCardholderName)) {
On 2016/08/29 17:58:31, rouslan wrote:
> On 2016/08/26 22:37:07, msw wrote:
> > Where exactly is this checked now? I don't see it in your CL.
> 
> I was going to rely on the implementation of the scanner to check the flag,
but
> I realize that's hard to review. Therefore, I've created a DelegateWrapper
class
> inside of the CreditCardScanner.java. The wrapper checks the flag to see
whether
> the cardHolderName needs to be cleared, regardless of what the implementation
of
> the scanner does.

Can we just DCHECK that the cardholder name is blank here if the experiment is
not enabled or something similar? Adding another layer of indirection seems very
unfortunate to me. It seems more straightforward to leave this conditional here
(and/or DCHECK) than add a delegate wrapper.

[please use gerrit instead, 2016-08-29 18:50:49]

msw, ptal patch 3.

https://codereview.chromium.org/2274693004/diff/20001/chrome/browser/ui/andro...
File chrome/browser/ui/android/autofill/credit_card_scanner_view_android.cc
(left):

https://codereview.chromium.org/2274693004/diff/20001/chrome/browser/ui/andro...
chrome/browser/ui/android/autofill/credit_card_scanner_view_android.cc:83: if
(base::FeatureList::IsEnabled(kAutofillScanCardholderName)) {
On 2016/08/29 18:19:19, msw wrote:
> On 2016/08/29 17:58:31, rouslan wrote:
> > On 2016/08/26 22:37:07, msw wrote:
> > > Where exactly is this checked now? I don't see it in your CL.
> > 
> > I was going to rely on the implementation of the scanner to check the flag,
> but
> > I realize that's hard to review. Therefore, I've created a DelegateWrapper
> class
> > inside of the CreditCardScanner.java. The wrapper checks the flag to see
> whether
> > the cardHolderName needs to be cleared, regardless of what the
implementation
> of
> > the scanner does.
> 
> Can we just DCHECK that the cardholder name is blank here if the experiment is
> not enabled or something similar? Adding another layer of indirection seems
very
> unfortunate to me. It seems more straightforward to leave this conditional
here
> (and/or DCHECK) than add a delegate wrapper.

Done.

[msw, 2016-08-29 18:54:17]

chrome/browser/ui lgtm; thanks

[please use gerrit instead, 2016-08-29 19:00:45]

The CQ bit was checked by rouslan@chromium.org

[please use gerrit instead, 2016-08-29 19:00:45]

The patchset sent to the CQ was uploaded after l-g-t-m from
dfalcantara@chromium.org
Link to the patchset: https://codereview.chromium.org/2274693004/#ps60001
(title: "DCHECK")

[commit-bot: I haz the power, 2016-08-29 19:11:24]

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

[please use gerrit instead, 2016-08-30 00:11:18]

The CQ bit was unchecked by rouslan@chromium.org

[please use gerrit instead, 2016-08-30 00:11:35]

The CQ bit was checked by rouslan@chromium.org

[commit-bot: I haz the power, 2016-08-30 00:53:17]

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, 2016-08-30 05:18:51]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2016-08-30 05:22:18]

Description was changed from

==========
Separate credit card scanner from native bridge.

This patch enables using credit card scanner directly from Java, without
jumping to native first. Checking of the "AutofillScanCardholderName"
flag has moved into the Java delegate wrapper, so users of this
functionality in Java are governed by the same flag, regardless of the
implementation.

BUG=640430
==========

to

==========
Separate credit card scanner from native bridge.

This patch enables using credit card scanner directly from Java, without
jumping to native first. Checking of the "AutofillScanCardholderName"
flag has moved into the Java delegate wrapper, so users of this
functionality in Java are governed by the same flag, regardless of the
implementation.

BUG=640430

Committed: https://crrev.com/8bb5bcc3e855e57293999433cab3a1cb8971597b
Cr-Commit-Position: refs/heads/master@{#415146}
==========

[commit-bot: I haz the power, 2016-08-30 05:22:20]

Patchset 3 (id:??) landed as
https://crrev.com/8bb5bcc3e855e57293999433cab3a1cb8971597b
Cr-Commit-Position: refs/heads/master@{#415146}