[Payment Request] Displays appropriate footer if first transaction completed

[Payment Request] Displays appropriate footer if first transation completed

TBR=gab@

BUG=602666
Review-Url: https://codereview.chromium.org/2970013003
Cr-Commit-Position: refs/heads/master@{#484297}
Committed: https://chromium.googlesource.com/chromium/src/+/e3b1c5a95d41bb37d5d3cace150c45a7ab2c4916

[Moe, 2017-07-04 19:07:52]

mahmadi@chromium.org changed reviewers:
+ eugenebut@chromium.org, macourteau@chromium.org, mathp@chromium.org

[Moe, 2017-07-04 19:07:53]

Hi MAC, mathp, eugenebut, Please take a look at this CL.
macourteau@ for c/b/payments and ios/c/b/ui/payments 
mathp@ for components/autofill/core/browser/
eugenebut@ for ios/b/b/prefs/browser_prefs.mm

[Mathieu, 2017-07-04 20:28:51]

https://codereview.chromium.org/2970013003/diff/1/components/autofill/core/br...
File components/autofill/core/browser/DEPS (right):

https://codereview.chromium.org/2970013003/diff/1/components/autofill/core/br...
components/autofill/core/browser/DEPS:7: "+components/payments",
is it possible to not have autofill depend on components/payments? It's usually
the other way around, and I wouldn't want to create cycles. Does that make
sense?

Perhaps you can have a test pref service that lives over in components/payments/

[Moe, 2017-07-04 21:26:52]

Description was changed from

==========
[Payment Request] Displays appropriate footer if first transation completed

BUG=602666
==========

to

==========
[Payment Request] Displays appropriate footer if first transation completed

TBR=gab@

BUG=602666
==========

[Moe, 2017-07-04 21:27:27]

mahmadi@chromium.org changed reviewers:
+ gab@chromium.org

[Moe, 2017-07-04 21:27:28]

PTAL?
TBR gab@ for added dependency to components/prefs

https://codereview.chromium.org/2970013003/diff/1/components/autofill/core/br...
File components/autofill/core/browser/DEPS (right):

https://codereview.chromium.org/2970013003/diff/1/components/autofill/core/br...
components/autofill/core/browser/DEPS:7: "+components/payments",
On 2017/07/04 at 20:28:50, Mathieu wrote:
> is it possible to not have autofill depend on components/payments? It's
usually the other way around, and I wouldn't want to create cycles. Does that
make sense?
> 
> Perhaps you can have a test pref service that lives over in
components/payments/

Done.

[Mathieu, 2017-07-05 13:26:29]

lgtm

https://codereview.chromium.org/2970013003/diff/20001/components/payments/cor...
File components/payments/core/BUILD.gn (right):

https://codereview.chromium.org/2970013003/diff/20001/components/payments/cor...
components/payments/core/BUILD.gn:64: "payments_test_util.cc",
nit: payment_test_utils

[Moe, 2017-07-05 13:59:23]

https://codereview.chromium.org/2970013003/diff/20001/components/payments/cor...
File components/payments/core/BUILD.gn (right):

https://codereview.chromium.org/2970013003/diff/20001/components/payments/cor...
components/payments/core/BUILD.gn:64: "payments_test_util.cc",
On 2017/07/05 at 13:26:28, Mathieu wrote:
> nit: payment_test_utils

Hmm :? I've received the opposite feedback in the past, which is to prefer
_util.h over _utils.h

[macourteau, 2017-07-05 14:10:37]

ios/c/b/* lgtm (with optional nits)

https://codereview.chromium.org/2970013003/diff/20001/ios/chrome/browser/paym...
File ios/chrome/browser/payments/test_payment_request.h (right):

https://codereview.chromium.org/2970013003/diff/20001/ios/chrome/browser/paym...
ios/chrome/browser/payments/test_payment_request.h:43:
region_data_loader_(nullptr),
optional nit: I find it clearer when trivial default values are set where these
instance variables are declared:

  autofill::RegionDataLoader* region_data_loader_ = nullptr;
  // ...

https://codereview.chromium.org/2970013003/diff/20001/ios/chrome/browser/ui/p...
File ios/chrome/browser/ui/payments/payment_request_mediator.mm (right):

https://codereview.chromium.org/2970013003/diff/20001/ios/chrome/browser/ui/p...
ios/chrome/browser/ui/payments/payment_request_mediator.mm:258: bool
firstTransactionCompleted =
optional nit (personal preference): const bool

[Mathieu, 2017-07-05 14:23:00]

https://codereview.chromium.org/2970013003/diff/20001/components/payments/cor...
File components/payments/core/BUILD.gn (right):

https://codereview.chromium.org/2970013003/diff/20001/components/payments/cor...
components/payments/core/BUILD.gn:64: "payments_test_util.cc",
On 2017/07/05 13:59:23, Moe wrote:
> On 2017/07/05 at 13:26:28, Mathieu wrote:
> > nit: payment_test_utils
> 
> Hmm :? I've received the opposite feedback in the past, which is to prefer
> _util.h over _utils.h

From code search test_util does seem a little more prevalent, you're right
(perhaps a 60/40 split between the two). I was saying this because we have
autofill_test_utils and now having this as test_util feels weird. We should pick
one

[Eugene But (OOO till 7-30), 2017-07-05 14:47:31]

lgtm

https://codereview.chromium.org/2970013003/diff/20001/components/payments/cor...
File components/payments/core/BUILD.gn (right):

https://codereview.chromium.org/2970013003/diff/20001/components/payments/cor...
components/payments/core/BUILD.gn:64: "payments_test_util.cc",
On 2017/07/05 14:23:00, Mathieu wrote:
> On 2017/07/05 13:59:23, Moe wrote:
> > On 2017/07/05 at 13:26:28, Mathieu wrote:
> > > nit: payment_test_utils
> > 
> > Hmm :? I've received the opposite feedback in the past, which is to prefer
> > _util.h over _utils.h
> 
> From code search test_util does seem a little more prevalent, you're right
> (perhaps a 60/40 split between the two). I was saying this because we have
> autofill_test_utils and now having this as test_util feels weird. We should
pick
> one
FWIW: _util is more prevalent than _utils

https://codereview.chromium.org/2970013003/diff/20001/ios/chrome/browser/paym...
File ios/chrome/browser/payments/payment_request.mm (right):

https://codereview.chromium.org/2970013003/diff/20001/ios/chrome/browser/paym...
ios/chrome/browser/payments/payment_request.mm:158: const SigninManager*
signinManager =
s/signinManager/signin_manager

[Moe, 2017-07-05 15:59:21]

Patchset #3 (id:40001) has been deleted

[Moe, 2017-07-05 16:01:45]

The CQ bit was checked by mahmadi@chromium.org

[Moe, 2017-07-05 16:01:47]

Thanks everyone!

https://codereview.chromium.org/2970013003/diff/20001/components/payments/cor...
File components/payments/core/BUILD.gn (right):

https://codereview.chromium.org/2970013003/diff/20001/components/payments/cor...
components/payments/core/BUILD.gn:64: "payments_test_util.cc",
On 2017/07/05 at 14:47:31, Eugene But wrote:
> On 2017/07/05 14:23:00, Mathieu wrote:
> > On 2017/07/05 13:59:23, Moe wrote:
> > > On 2017/07/05 at 13:26:28, Mathieu wrote:
> > > > nit: payment_test_utils
> > > 
> > > Hmm :? I've received the opposite feedback in the past, which is to prefer
> > > _util.h over _utils.h
> > 
> > From code search test_util does seem a little more prevalent, you're right
> > (perhaps a 60/40 split between the two). I was saying this because we have
> > autofill_test_utils and now having this as test_util feels weird. We should
pick
> > one
> FWIW: _util is more prevalent than _utils

It makes sense to move toward what's more prevalent. Let's change the
autofill_test_utils to autofill_test_util in a separate CL.

https://codereview.chromium.org/2970013003/diff/20001/ios/chrome/browser/paym...
File ios/chrome/browser/payments/payment_request.mm (right):

https://codereview.chromium.org/2970013003/diff/20001/ios/chrome/browser/paym...
ios/chrome/browser/payments/payment_request.mm:158: const SigninManager*
signinManager =
On 2017/07/05 at 14:47:31, Eugene But wrote:
> s/signinManager/signin_manager

Done.

https://codereview.chromium.org/2970013003/diff/20001/ios/chrome/browser/paym...
File ios/chrome/browser/payments/test_payment_request.h (right):

https://codereview.chromium.org/2970013003/diff/20001/ios/chrome/browser/paym...
ios/chrome/browser/payments/test_payment_request.h:43:
region_data_loader_(nullptr),
On 2017/07/05 at 14:10:36, macourteau wrote:
> optional nit: I find it clearer when trivial default values are set where
these instance variables are declared:
> 
>   autofill::RegionDataLoader* region_data_loader_ = nullptr;
>   // ...

Ack. However I think it's clearer if all member variable initialization, whether
initialization to a constructor argument or to a default trivial or non-trivial
value is done in one place, which is the constructor initializer list and/or its
body.

https://codereview.chromium.org/2970013003/diff/20001/ios/chrome/browser/ui/p...
File ios/chrome/browser/ui/payments/payment_request_mediator.mm (right):

https://codereview.chromium.org/2970013003/diff/20001/ios/chrome/browser/ui/p...
ios/chrome/browser/ui/payments/payment_request_mediator.mm:258: bool
firstTransactionCompleted =
On 2017/07/05 at 14:10:36, macourteau wrote:
> optional nit (personal preference): const bool

Done.

[Moe, 2017-07-05 16:01:48]

The patchset sent to the CQ was uploaded after l-g-t-m from mathp@chromium.org,
eugenebut@chromium.org, macourteau@chromium.org
Link to the patchset: https://codereview.chromium.org/2970013003/#ps60001
(title: "Addressed comments + rebase")

[commit-bot: I haz the power, 2017-07-05 16:02:00]

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-07-05 17:16:20]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1499270505879490,
"parent_rev": "90af2340e24a4250bba3c73d55c48244172ea449", "commit_rev":
"e3b1c5a95d41bb37d5d3cace150c45a7ab2c4916"}

[commit-bot: I haz the power, 2017-07-05 17:16:37]

Description was changed from

==========
[Payment Request] Displays appropriate footer if first transation completed

TBR=gab@

BUG=602666
==========

to

==========
[Payment Request] Displays appropriate footer if first transation completed

TBR=gab@

BUG=602666

Review-Url: https://codereview.chromium.org/2970013003
Cr-Commit-Position: refs/heads/master@{#484297}
Committed:
https://chromium.googlesource.com/chromium/src/+/e3b1c5a95d41bb37d5d3cace150c...
==========

[commit-bot: I haz the power, 2017-07-05 17:16:38]

Committed patchset #3 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/e3b1c5a95d41bb37d5d3cace150c...

[gab, 2017-07-05 23:46:25]

On 2017/07/04 21:27:28, Moe wrote:
> PTAL?
> TBR gab@ for added dependency to components/prefs

r-s lgtm for DEPS to prefs/