Cancels payment request when Settings is tapped, and opens the Autofill settings page.

Cancels payment request when Settings is tapped, and opens the Autofill settings page.

Screenshots: http://imgur.com/a/h8xyi

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

[macourteau, 2017-04-05 19:58:54]

macourteau@chromium.org changed reviewers:
+ mahmadi@google.com

[macourteau, 2017-04-05 19:58:54]

Please do a first pass :)

[Moe, 2017-04-06 04:14:05]

Description was changed from

==========
Cancels payment request when Settings is tapped, and opens the Autofill settings
page.

Screenshots: http://imgur.com/a/h8xyi

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

to

==========
Cancels payment request when Settings is tapped, and opens the Autofill settings
page.

Screenshots: http://imgur.com/a/h8xyi

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

[Moe, 2017-04-06 04:14:05]

mahmadi@chromium.org changed reviewers:
+ mahmadi@chromium.org
- mahmadi@google.com

[Moe, 2017-04-06 05:18:14]

Looks great. a few comments.

https://codereview.chromium.org/2804853002/diff/1/ios/chrome/browser/payments...
File ios/chrome/browser/payments/payment_request_manager.mm (right):

https://codereview.chromium.org/2804853002/diff/1/ios/chrome/browser/payments...
ios/chrome/browser/payments/payment_request_manager.mm:548: [self
terminateRequestWithErrorMessage:@"The payment request was canceled."
While you're at it, please refactor these messages into constants. e.g.,
kAbortMessage, kCancelMessage, etc.

https://codereview.chromium.org/2804853002/diff/1/ios/chrome/browser/payments...
ios/chrome/browser/payments/payment_request_manager.mm:549: callback:nil];
Could you do the following logic in a callback that you pass to this method?
that way we know the JS has executed? I believe you'd need a
ProceduralBlockWithBool. There are other examples in this file.

https://codereview.chromium.org/2804853002/diff/1/ios/chrome/browser/payments...
ios/chrome/browser/payments/payment_request_manager.mm:551: UIWindow*
main_window = [[UIApplication sharedApplication] keyWindow];
nit: mainWindow

https://codereview.chromium.org/2804853002/diff/1/ios/chrome/browser/payments...
ios/chrome/browser/payments/payment_request_manager.mm:553:
base::scoped_nsobject<GenericChromeCommand> command(
I believe you don't need a base::scoped_nsobject when ARC is enabled. A
GenericChromeCommand* does it.

[macourteau, 2017-04-06 14:21:22]

https://codereview.chromium.org/2804853002/diff/1/ios/chrome/browser/payments...
File ios/chrome/browser/payments/payment_request_manager.mm (right):

https://codereview.chromium.org/2804853002/diff/1/ios/chrome/browser/payments...
ios/chrome/browser/payments/payment_request_manager.mm:548: [self
terminateRequestWithErrorMessage:@"The payment request was canceled."
On 2017/04/06 05:18:13, moe wrote:
> While you're at it, please refactor these messages into constants. e.g.,
> kAbortMessage, kCancelMessage, etc.

Done.

https://codereview.chromium.org/2804853002/diff/1/ios/chrome/browser/payments...
ios/chrome/browser/payments/payment_request_manager.mm:549: callback:nil];
On 2017/04/06 05:18:13, moe wrote:
> Could you do the following logic in a callback that you pass to this method?
> that way we know the JS has executed? I believe you'd need a
> ProceduralBlockWithBool. There are other examples in this file.

Done.

https://codereview.chromium.org/2804853002/diff/1/ios/chrome/browser/payments...
ios/chrome/browser/payments/payment_request_manager.mm:551: UIWindow*
main_window = [[UIApplication sharedApplication] keyWindow];
On 2017/04/06 05:18:13, moe wrote:
> nit: mainWindow

Done.

https://codereview.chromium.org/2804853002/diff/1/ios/chrome/browser/payments...
ios/chrome/browser/payments/payment_request_manager.mm:553:
base::scoped_nsobject<GenericChromeCommand> command(
On 2017/04/06 05:18:13, moe wrote:
> I believe you don't need a base::scoped_nsobject when ARC is enabled. A
> GenericChromeCommand* does it.

Done.

[Moe, 2017-04-06 14:38:27]

On 2017/04/06 14:21:22, macourteau wrote:
>
https://codereview.chromium.org/2804853002/diff/1/ios/chrome/browser/payments...
> File ios/chrome/browser/payments/payment_request_manager.mm (right):
> 
>
https://codereview.chromium.org/2804853002/diff/1/ios/chrome/browser/payments...
> ios/chrome/browser/payments/payment_request_manager.mm:548: [self
> terminateRequestWithErrorMessage:@"The payment request was canceled."
> On 2017/04/06 05:18:13, moe wrote:
> > While you're at it, please refactor these messages into constants. e.g.,
> > kAbortMessage, kCancelMessage, etc.
> 
> Done.
> 
>
https://codereview.chromium.org/2804853002/diff/1/ios/chrome/browser/payments...
> ios/chrome/browser/payments/payment_request_manager.mm:549: callback:nil];
> On 2017/04/06 05:18:13, moe wrote:
> > Could you do the following logic in a callback that you pass to this method?
> > that way we know the JS has executed? I believe you'd need a
> > ProceduralBlockWithBool. There are other examples in this file.
> 
> Done.
> 
>
https://codereview.chromium.org/2804853002/diff/1/ios/chrome/browser/payments...
> ios/chrome/browser/payments/payment_request_manager.mm:551: UIWindow*
> main_window = [[UIApplication sharedApplication] keyWindow];
> On 2017/04/06 05:18:13, moe wrote:
> > nit: mainWindow
> 
> Done.
> 
>
https://codereview.chromium.org/2804853002/diff/1/ios/chrome/browser/payments...
> ios/chrome/browser/payments/payment_request_manager.mm:553:
> base::scoped_nsobject<GenericChromeCommand> command(
> On 2017/04/06 05:18:13, moe wrote:
> > I believe you don't need a base::scoped_nsobject when ARC is enabled. A
> > GenericChromeCommand* does it.
> 
> Done.

LGTM

[macourteau, 2017-04-06 14:43:06]

macourteau@chromium.org changed reviewers:
+ marq@chromium.org

[macourteau, 2017-04-06 14:43:07]

+marq@ for OWNERS review of remaining files. Thanks!

[macourteau, 2017-04-06 15:15:46]

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

[commit-bot: I haz the power, 2017-04-06 15:16:12]

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

[marq (ping after 24h), 2017-04-06 15:16:45]

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

https://codereview.chromium.org/2804853002/diff/20001/ios/chrome/browser/paym...
ios/chrome/browser/payments/payment_request_manager.mm:395: [strongSelf
terminateRequestWithErrorMessage:kAbortMessage
Delete lines 391-394 and just call the method on weakSelf

https://codereview.chromium.org/2804853002/diff/20001/ios/chrome/browser/paym...
ios/chrome/browser/payments/payment_request_manager.mm:417: [strongSelf
terminateRequestWithErrorMessage:kCancelMessage callback:nil];
Delete lines 413-416 and just call the method on weakSelf.

[macourteau, 2017-04-06 15:24:00]

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

https://codereview.chromium.org/2804853002/diff/20001/ios/chrome/browser/paym...
ios/chrome/browser/payments/payment_request_manager.mm:395: [strongSelf
terminateRequestWithErrorMessage:kAbortMessage
On 2017/04/06 15:16:45, marq wrote:
> Delete lines 391-394 and just call the method on weakSelf

Done.

https://codereview.chromium.org/2804853002/diff/20001/ios/chrome/browser/paym...
ios/chrome/browser/payments/payment_request_manager.mm:417: [strongSelf
terminateRequestWithErrorMessage:kCancelMessage callback:nil];
On 2017/04/06 15:16:45, marq wrote:
> Delete lines 413-416 and just call the method on weakSelf.

Done.

[Moe, 2017-04-06 15:35:43]

https://codereview.chromium.org/2804853002/diff/40001/ios/chrome/browser/paym...
File ios/chrome/browser/payments/payment_request_manager.mm (right):

https://codereview.chromium.org/2804853002/diff/40001/ios/chrome/browser/paym...
ios/chrome/browser/payments/payment_request_manager.mm:384: if (!strongSelf)
Could you please call the method on weakSelf here too? (everywhere in the file)

[macourteau, 2017-04-06 18:38:47]

https://codereview.chromium.org/2804853002/diff/40001/ios/chrome/browser/paym...
File ios/chrome/browser/payments/payment_request_manager.mm (right):

https://codereview.chromium.org/2804853002/diff/40001/ios/chrome/browser/paym...
ios/chrome/browser/payments/payment_request_manager.mm:384: if (!strongSelf)
On 2017/04/06 15:35:43, moe wrote:
> Could you please call the method on weakSelf here too? (everywhere in the
file)

Done.

[macourteau, 2017-04-06 18:39:27]

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

[commit-bot: I haz the power, 2017-04-06 18:40:12]

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-06 19:57:33]

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

[commit-bot: I haz the power, 2017-04-06 19:57:34]

Dry run: This issue passed the CQ dry run.

[marq (ping after 24h), 2017-04-07 12:25:52]

lgtm

[macourteau, 2017-04-07 12:33:45]

The CQ bit was checked by macourteau@chromium.org

[macourteau, 2017-04-07 12:33:45]

The patchset sent to the CQ was uploaded after l-g-t-m from mahmadi@chromium.org
Link to the patchset: https://codereview.chromium.org/2804853002/#ps60001
(title: "Remove more strongSelf checks.")

[commit-bot: I haz the power, 2017-04-07 12:34:04]

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-07 12:38:12]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1491568425164920,
"parent_rev": "539e731c557cbce418d3ac1ea315f6cc9bd0e21f", "commit_rev":
"69cffb5b42a7750fad55fd52c034573268e29681"}

[commit-bot: I haz the power, 2017-04-07 12:39:04]

Description was changed from

==========
Cancels payment request when Settings is tapped, and opens the Autofill settings
page.

Screenshots: http://imgur.com/a/h8xyi

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

to

==========
Cancels payment request when Settings is tapped, and opens the Autofill settings
page.

Screenshots: http://imgur.com/a/h8xyi

BUG=602666

Review-Url: https://codereview.chromium.org/2804853002
Cr-Commit-Position: refs/heads/master@{#462844}
Committed:
https://chromium.googlesource.com/chromium/src/+/69cffb5b42a7750fad55fd52c034...
==========

[commit-bot: I haz the power, 2017-04-07 12:39:06]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/69cffb5b42a7750fad55fd52c034...