New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 761358 link

Starred by 1 user

Issue metadata

Status: Verified
Owner:
Closed: Sep 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: iOS
Pri: 1
Type: Feature



Sign in to add a comment

Enable Payment Request on iOS by default

Project Member Reported by mahmadi@chromium.org, Sep 1 2017

Issue description

Payment Request on iOS should be enabled by default.
 
Status: Started (was: Assigned)
Project Member

Comment 2 by bugdroid1@chromium.org, Sep 8 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/1ca550a89a7bf6c1a8e2b6fc21a8d399bf51f220

commit 1ca550a89a7bf6c1a8e2b6fc21a8d399bf51f220
Author: Mohamad Ahmadi <mahmadi@chromium.org>
Date: Fri Sep 08 13:46:39 2017

[Payment Request] Enables Payment Request on iOS by default

- This CL sets PaymentRequestManager's active WebState to nullptr in BVC's
  shutdown method to make sure the active WebState is clean when BVC is
  shutting down.
- This CL adds a unit test to verify that the BVC shuts down cleanly with
  Payment Request enabled.
- This CL removes the calls to enable/disable the active WebState in the
  [PaymentRequestManager enablePaymentRequest:] and instead changes the
  behavior so that the script commands are ignored when the Payment Request
  is disabled. The reason for this change is that this method is called
  when the BVC is disabled before it shuts down. This could result in a
  second attempt to disable the active WebState when the BVC actually shuts
  down which is wrong.

Bug:  761358 , 755799,  602666 
Change-Id: I600927fd0bc00523552d2e5bb581583611c60dfb
Reviewed-on: https://chromium-review.googlesource.com/646654
Reviewed-by: Mark Cogan <marq@chromium.org>
Reviewed-by: Rouslan Solomakhin <rouslan@chromium.org>
Commit-Queue: Moe Ahmadi <mahmadi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#500580}
[modify] https://crrev.com/1ca550a89a7bf6c1a8e2b6fc21a8d399bf51f220/components/payments/core/features.cc
[modify] https://crrev.com/1ca550a89a7bf6c1a8e2b6fc21a8d399bf51f220/ios/chrome/browser/ui/BUILD.gn
[modify] https://crrev.com/1ca550a89a7bf6c1a8e2b6fc21a8d399bf51f220/ios/chrome/browser/ui/browser_view_controller.mm
[modify] https://crrev.com/1ca550a89a7bf6c1a8e2b6fc21a8d399bf51f220/ios/chrome/browser/ui/browser_view_controller_unittest.mm
[modify] https://crrev.com/1ca550a89a7bf6c1a8e2b6fc21a8d399bf51f220/ios/chrome/browser/ui/payments/payment_request_manager.mm
[modify] https://crrev.com/1ca550a89a7bf6c1a8e2b6fc21a8d399bf51f220/ios/chrome/browser/web/chrome_web_client_unittest.mm

Status: Fixed (was: Started)
Labels: Merge-Request-62
Project Member

Comment 5 by bugdroid1@chromium.org, Sep 9 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/1a37acbe061a95db57555a737e429c831b82ff2f

commit 1a37acbe061a95db57555a737e429c831b82ff2f
Author: Mike Baxley <baxley@chromium.org>
Date: Sat Sep 09 04:13:52 2017

Revert "[Payment Request] Enables Payment Request on iOS by default"

This reverts commit 1ca550a89a7bf6c1a8e2b6fc21a8d399bf51f220.

Reason for revert: This is crashing in RecentTabsTableTestCase.testClosedTabAppearsInRecentTabsPanel

It reproduces locally and in the try job that was run.

Original change's description:
> [Payment Request] Enables Payment Request on iOS by default
> 
> - This CL sets PaymentRequestManager's active WebState to nullptr in BVC's
>   shutdown method to make sure the active WebState is clean when BVC is
>   shutting down.
> - This CL adds a unit test to verify that the BVC shuts down cleanly with
>   Payment Request enabled.
> - This CL removes the calls to enable/disable the active WebState in the
>   [PaymentRequestManager enablePaymentRequest:] and instead changes the
>   behavior so that the script commands are ignored when the Payment Request
>   is disabled. The reason for this change is that this method is called
>   when the BVC is disabled before it shuts down. This could result in a
>   second attempt to disable the active WebState when the BVC actually shuts
>   down which is wrong.
> 
> Bug:  761358 , 755799,  602666 
> Change-Id: I600927fd0bc00523552d2e5bb581583611c60dfb
> Reviewed-on: https://chromium-review.googlesource.com/646654
> Reviewed-by: Mark Cogan <marq@chromium.org>
> Reviewed-by: Rouslan Solomakhin <rouslan@chromium.org>
> Commit-Queue: Moe Ahmadi <mahmadi@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#500580}

TBR=rouslan@chromium.org,marq@chromium.org,mahmadi@chromium.org

Change-Id: I7ba85ae51cf465716d32fe2ceab4fc37b835e1a7
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  761358 , 755799,  602666 
Reviewed-on: https://chromium-review.googlesource.com/658777
Reviewed-by: Mike Baxley <baxley@chromium.org>
Commit-Queue: Mike Baxley <baxley@chromium.org>
Cr-Commit-Position: refs/heads/master@{#500788}
[modify] https://crrev.com/1a37acbe061a95db57555a737e429c831b82ff2f/components/payments/core/features.cc
[modify] https://crrev.com/1a37acbe061a95db57555a737e429c831b82ff2f/ios/chrome/browser/ui/BUILD.gn
[modify] https://crrev.com/1a37acbe061a95db57555a737e429c831b82ff2f/ios/chrome/browser/ui/browser_view_controller.mm
[modify] https://crrev.com/1a37acbe061a95db57555a737e429c831b82ff2f/ios/chrome/browser/ui/browser_view_controller_unittest.mm
[modify] https://crrev.com/1a37acbe061a95db57555a737e429c831b82ff2f/ios/chrome/browser/ui/payments/payment_request_manager.mm
[modify] https://crrev.com/1a37acbe061a95db57555a737e429c831b82ff2f/ios/chrome/browser/web/chrome_web_client_unittest.mm

Project Member

Comment 6 by sheriffbot@chromium.org, Sep 9 2017

Labels: -Merge-Request-62 Merge-Review-62 Hotlist-Merge-Review
This bug requires manual review: Reverts referenced in bugdroid comments after merge request.
Please contact the milestone owner if you have questions.
Owners: amineer@(Android), cmasso@(iOS), bhthompson@(ChromeOS), abdulsyed@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Comment 7 by cma...@chromium.org, Sep 11 2017

mahmadi@ this cl was reverted. do you still plan to merge it?
Project Member

Comment 8 by bugdroid1@chromium.org, Sep 12 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/bec07eb12d64b84330e6a582ac7aa38a103a9e7d

commit bec07eb12d64b84330e6a582ac7aa38a103a9e7d
Author: Mohamad Ahmadi <mahmadi@chromium.org>
Date: Tue Sep 12 19:38:46 2017

[Payment Request] Enables Payment Request on iOS by default

- Sets PaymentRequestManager's active WebState to nullptr in BVC's shutdown
  method to make sure the active WebState is clean when BVC shuts down.
- Adds a call to update PaymentRequestManager's active WebState in
  -tabModel:didReplaceTab:withTab:atIndex: in order to clean the the
  previously active WebState and enable PaymentRequest on the new one. This
  call happens when a recent tab is opened.
- This CL adds a unit test to verify that the BVC shuts down cleanly with
  Payment Request enabled.
- This CL removes the calls to enable/disable the active WebState in the
  [PaymentRequestManager enablePaymentRequest:] and instead changes the
  behavior so that the script commands are ignored when the Payment Request
  is disabled. The reason for this change is that this method is called
  when the BVC is disabled before it shuts down. This could result in a
  second attempt to disable the active WebState when the BVC actually shuts
  down which is wrong.

TBR=rouslan@

Bug:  761358 , 755799,  602666 
Change-Id: Id984aec77194993b23085448cb802ba9fe120379
Reviewed-on: https://chromium-review.googlesource.com/660480
Commit-Queue: Moe Ahmadi <mahmadi@chromium.org>
Reviewed-by: Mark Cogan <marq@chromium.org>
Cr-Commit-Position: refs/heads/master@{#501363}
[modify] https://crrev.com/bec07eb12d64b84330e6a582ac7aa38a103a9e7d/components/payments/core/features.cc
[modify] https://crrev.com/bec07eb12d64b84330e6a582ac7aa38a103a9e7d/ios/chrome/browser/ui/BUILD.gn
[modify] https://crrev.com/bec07eb12d64b84330e6a582ac7aa38a103a9e7d/ios/chrome/browser/ui/browser_view_controller.mm
[modify] https://crrev.com/bec07eb12d64b84330e6a582ac7aa38a103a9e7d/ios/chrome/browser/ui/browser_view_controller_unittest.mm
[modify] https://crrev.com/bec07eb12d64b84330e6a582ac7aa38a103a9e7d/ios/chrome/browser/ui/payments/payment_request_manager.mm
[modify] https://crrev.com/bec07eb12d64b84330e6a582ac7aa38a103a9e7d/ios/chrome/browser/web/chrome_web_client_unittest.mm

cmasso@, yes I would like to merge the reland.

Comment 10 Deleted

Comment 11 Deleted

I thought this was already enable by default when it was considered to be in 62. lindsayw@ have you been able to test Payment request feature?

I am seeing a lot of stuffs on payment request that needs to be merged. Was this feature code complete by Feature freeze?
Tested on :

App Version: 63.0.3214.0 canary
Devices: iPhone 7, iPad Air, iPad Mini 2
iOS Versions: 10.3.3, 11.0

Payment Request feature has been enabled by default. 

Labels: -Hotlist-Merge-Review -Merge-Review-62 Merge-Approved-62
Project Member

Comment 15 by bugdroid1@chromium.org, Sep 14 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f3ce264243ab8049d8415e0080a4af616df650f1

commit f3ce264243ab8049d8415e0080a4af616df650f1
Author: Olivier Robin <olivierrobin@chromium.org>
Date: Thu Sep 14 09:12:08 2017

Check existence of navigationManagerImpl when returning currentNavItem.

If the WebController is closed, the navigationManagerImpl is deleted.
If a |currentNavItem| is called then (in a completion handler), there
is a crash.
Exemple on bots:
https://uberchromegw.corp.google.com/i/internal.bling.main/builders/iphone9-simulator-x64/builds/13829/steps/ios_chrome_integration_egtests%20%28iPhone%205s%20iOS%209.0%29%20on%20Mac

Bug:  761358 
Change-Id: Ib5c1ef59628f6c1914b4cf33203a0abe0b695b9d
Reviewed-on: https://chromium-review.googlesource.com/664718
Commit-Queue: Olivier Robin <olivierrobin@chromium.org>
Reviewed-by: Eugene But <eugenebut@chromium.org>
Reviewed-by: Rohit Rao (ping after 24h) <rohitrao@chromium.org>
Cr-Commit-Position: refs/heads/master@{#501908}
[modify] https://crrev.com/f3ce264243ab8049d8415e0080a4af616df650f1/ios/web/web_state/ui/crw_web_controller.mm

Project Member

Comment 16 by bugdroid1@chromium.org, Sep 14 2017

Labels: -merge-approved-62 merge-merged-3202
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/033b8b51499ca5b4a000f6ea7a184956c1b391a7

commit 033b8b51499ca5b4a000f6ea7a184956c1b391a7
Author: Mohamad Ahmadi <mahmadi@chromium.org>
Date: Thu Sep 14 16:17:32 2017

[Payment Request] Enables Payment Request on iOS by default

- Sets PaymentRequestManager's active WebState to nullptr in BVC's shutdown
  method to make sure the active WebState is clean when BVC shuts down.
- Adds a call to update PaymentRequestManager's active WebState in
  -tabModel:didReplaceTab:withTab:atIndex: in order to clean the the
  previously active WebState and enable PaymentRequest on the new one. This
  call happens when a recent tab is opened.
- This CL adds a unit test to verify that the BVC shuts down cleanly with
  Payment Request enabled.
- This CL removes the calls to enable/disable the active WebState in the
  [PaymentRequestManager enablePaymentRequest:] and instead changes the
  behavior so that the script commands are ignored when the Payment Request
  is disabled. The reason for this change is that this method is called
  when the BVC is disabled before it shuts down. This could result in a
  second attempt to disable the active WebState when the BVC actually shuts
  down which is wrong.

TBR=mahmadi@chromium.org, rouslan@

(cherry picked from commit bec07eb12d64b84330e6a582ac7aa38a103a9e7d)

Bug:  761358 , 755799,  602666 
Change-Id: Id984aec77194993b23085448cb802ba9fe120379
Reviewed-on: https://chromium-review.googlesource.com/660480
Commit-Queue: Moe Ahmadi <mahmadi@chromium.org>
Reviewed-by: Mark Cogan <marq@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#501363}
Reviewed-on: https://chromium-review.googlesource.com/667718
Reviewed-by: Moe Ahmadi <mahmadi@chromium.org>
Cr-Commit-Position: refs/branch-heads/3202@{#220}
Cr-Branched-From: fa6a5d87adff761bc16afc5498c3f5944c1daa68-refs/heads/master@{#499098}
[modify] https://crrev.com/033b8b51499ca5b4a000f6ea7a184956c1b391a7/components/payments/core/features.cc
[modify] https://crrev.com/033b8b51499ca5b4a000f6ea7a184956c1b391a7/ios/chrome/browser/ui/BUILD.gn
[modify] https://crrev.com/033b8b51499ca5b4a000f6ea7a184956c1b391a7/ios/chrome/browser/ui/browser_view_controller.mm
[modify] https://crrev.com/033b8b51499ca5b4a000f6ea7a184956c1b391a7/ios/chrome/browser/ui/browser_view_controller_unittest.mm
[modify] https://crrev.com/033b8b51499ca5b4a000f6ea7a184956c1b391a7/ios/chrome/browser/ui/payments/payment_request_manager.mm
[modify] https://crrev.com/033b8b51499ca5b4a000f6ea7a184956c1b391a7/ios/chrome/browser/web/chrome_web_client_unittest.mm

Project Member

Comment 17 by bugdroid1@chromium.org, Sep 14 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f07deb47d8395ccd277d3ebcc8490756f7591eae

commit f07deb47d8395ccd277d3ebcc8490756f7591eae
Author: Mohamad Ahmadi <mahmadi@chromium.org>
Date: Thu Sep 14 16:24:28 2017

Check existence of navigationManagerImpl when returning currentNavItem.

If the WebController is closed, the navigationManagerImpl is deleted.
If a |currentNavItem| is called then (in a completion handler), there
is a crash.
Exemple on bots:
https://uberchromegw.corp.google.com/i/internal.bling.main/builders/iphone9-simulator-x64/builds/13829/steps/ios_chrome_integration_egtests%20%28iPhone%205s%20iOS%209.0%29%20on%20Mac

TBR=olivierrobin@chromium.org

(cherry picked from commit f3ce264243ab8049d8415e0080a4af616df650f1)

Bug:  761358 
Change-Id: Ib5c1ef59628f6c1914b4cf33203a0abe0b695b9d
Reviewed-on: https://chromium-review.googlesource.com/664718
Commit-Queue: Olivier Robin <olivierrobin@chromium.org>
Reviewed-by: Eugene But <eugenebut@chromium.org>
Reviewed-by: Rohit Rao (ping after 24h) <rohitrao@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#501908}
Reviewed-on: https://chromium-review.googlesource.com/666998
Reviewed-by: Moe Ahmadi <mahmadi@chromium.org>
Cr-Commit-Position: refs/branch-heads/3202@{#221}
Cr-Branched-From: fa6a5d87adff761bc16afc5498c3f5944c1daa68-refs/heads/master@{#499098}
[modify] https://crrev.com/f07deb47d8395ccd277d3ebcc8490756f7591eae/ios/web/web_state/ui/crw_web_controller.mm

Project Member

Comment 18 by bugdroid1@chromium.org, Sep 15 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/9d779b84f9c23e5b8d8887c70b691e7a70695df1

commit 9d779b84f9c23e5b8d8887c70b691e7a70695df1
Author: Jean-François Geyelin <jif@chromium.org>
Date: Fri Sep 15 18:13:09 2017

Revert "[Payment Request] Enables Payment Request on iOS by default"

This reverts commit 033b8b51499ca5b4a000f6ea7a184956c1b391a7.

Reason for revert: Conflicts made some test not compile

Original change's description:
> [Payment Request] Enables Payment Request on iOS by default
> 
> - Sets PaymentRequestManager's active WebState to nullptr in BVC's shutdown
>   method to make sure the active WebState is clean when BVC shuts down.
> - Adds a call to update PaymentRequestManager's active WebState in
>   -tabModel:didReplaceTab:withTab:atIndex: in order to clean the the
>   previously active WebState and enable PaymentRequest on the new one. This
>   call happens when a recent tab is opened.
> - This CL adds a unit test to verify that the BVC shuts down cleanly with
>   Payment Request enabled.
> - This CL removes the calls to enable/disable the active WebState in the
>   [PaymentRequestManager enablePaymentRequest:] and instead changes the
>   behavior so that the script commands are ignored when the Payment Request
>   is disabled. The reason for this change is that this method is called
>   when the BVC is disabled before it shuts down. This could result in a
>   second attempt to disable the active WebState when the BVC actually shuts
>   down which is wrong.
> 
> TBR=mahmadi@chromium.org, rouslan@
> 
> (cherry picked from commit bec07eb12d64b84330e6a582ac7aa38a103a9e7d)
> 
> Bug:  761358 , 755799,  602666 
> Change-Id: Id984aec77194993b23085448cb802ba9fe120379
> Reviewed-on: https://chromium-review.googlesource.com/660480
> Commit-Queue: Moe Ahmadi <mahmadi@chromium.org>
> Reviewed-by: Mark Cogan <marq@chromium.org>
> Cr-Original-Commit-Position: refs/heads/master@{#501363}
> Reviewed-on: https://chromium-review.googlesource.com/667718
> Reviewed-by: Moe Ahmadi <mahmadi@chromium.org>
> Cr-Commit-Position: refs/branch-heads/3202@{#220}
> Cr-Branched-From: fa6a5d87adff761bc16afc5498c3f5944c1daa68-refs/heads/master@{#499098}

TBR=marq@chromium.org,mahmadi@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug:  761358 , 755799,  602666 
Change-Id: I016757f312ccbca44f8e5cee8349743aa3f19493
Reviewed-on: https://chromium-review.googlesource.com/668579
Reviewed-by: Moe Ahmadi <mahmadi@chromium.org>
Cr-Commit-Position: refs/branch-heads/3202@{#254}
Cr-Branched-From: fa6a5d87adff761bc16afc5498c3f5944c1daa68-refs/heads/master@{#499098}
[modify] https://crrev.com/9d779b84f9c23e5b8d8887c70b691e7a70695df1/components/payments/core/features.cc
[modify] https://crrev.com/9d779b84f9c23e5b8d8887c70b691e7a70695df1/ios/chrome/browser/ui/BUILD.gn
[modify] https://crrev.com/9d779b84f9c23e5b8d8887c70b691e7a70695df1/ios/chrome/browser/ui/browser_view_controller.mm
[modify] https://crrev.com/9d779b84f9c23e5b8d8887c70b691e7a70695df1/ios/chrome/browser/ui/browser_view_controller_unittest.mm
[modify] https://crrev.com/9d779b84f9c23e5b8d8887c70b691e7a70695df1/ios/chrome/browser/ui/payments/payment_request_manager.mm
[modify] https://crrev.com/9d779b84f9c23e5b8d8887c70b691e7a70695df1/ios/chrome/browser/web/chrome_web_client_unittest.mm

Project Member

Comment 19 by bugdroid1@chromium.org, Sep 18 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/86051439669d2b221dc7e52f488b7afa43e3152b

commit 86051439669d2b221dc7e52f488b7afa43e3152b
Author: Mohamad Ahmadi <mahmadi@chromium.org>
Date: Mon Sep 18 17:16:08 2017

[Payment Request] Enables Payment Request on iOS by default

- Sets PaymentRequestManager's active WebState to nullptr in BVC's shutdown
  method to make sure the active WebState is clean when BVC shuts down.
- Adds a call to update PaymentRequestManager's active WebState in
  -tabModel:didReplaceTab:withTab:atIndex: in order to clean the the
  previously active WebState and enable PaymentRequest on the new one. This
  call happens when a recent tab is opened.
- This CL adds a unit test to verify that the BVC shuts down cleanly with
  Payment Request enabled.
- This CL removes the calls to enable/disable the active WebState in the
  [PaymentRequestManager enablePaymentRequest:] and instead changes the
  behavior so that the script commands are ignored when the Payment Request
  is disabled. The reason for this change is that this method is called
  when the BVC is disabled before it shuts down. This could result in a
  second attempt to disable the active WebState when the BVC actually shuts
  down which is wrong.

TBR=mahmadi@chromium.org, rouslan@, marq@


(cherry picked from commit bec07eb12d64b84330e6a582ac7aa38a103a9e7d)

Bug:  761358 , 755799,  602666 
Change-Id: I3ad010dc08cde8aa58928f743197f7eea4a41ba2
Reviewed-on: https://chromium-review.googlesource.com/660480
Commit-Queue: Moe Ahmadi <mahmadi@chromium.org>
Reviewed-by: Mark Cogan <marq@chromium.org>
Cr-Original-Original-Commit-Position: refs/heads/master@{#501363}
Reviewed-on: https://chromium-review.googlesource.com/667718
Reviewed-by: Moe Ahmadi <mahmadi@chromium.org>
Cr-Original-Commit-Position: refs/branch-heads/3202@{#220}
Cr-Original-Branched-From: fa6a5d87adff761bc16afc5498c3f5944c1daa68-refs/heads/master@{#499098}
Reviewed-on: https://chromium-review.googlesource.com/671489
Cr-Commit-Position: refs/branch-heads/3202@{#292}
Cr-Branched-From: fa6a5d87adff761bc16afc5498c3f5944c1daa68-refs/heads/master@{#499098}
[modify] https://crrev.com/86051439669d2b221dc7e52f488b7afa43e3152b/components/payments/core/features.cc
[modify] https://crrev.com/86051439669d2b221dc7e52f488b7afa43e3152b/ios/chrome/browser/ui/BUILD.gn
[modify] https://crrev.com/86051439669d2b221dc7e52f488b7afa43e3152b/ios/chrome/browser/ui/browser_view_controller.mm
[modify] https://crrev.com/86051439669d2b221dc7e52f488b7afa43e3152b/ios/chrome/browser/ui/browser_view_controller_unittest.mm
[modify] https://crrev.com/86051439669d2b221dc7e52f488b7afa43e3152b/ios/chrome/browser/ui/payments/payment_request_manager.mm
[modify] https://crrev.com/86051439669d2b221dc7e52f488b7afa43e3152b/ios/chrome/browser/web/chrome_web_client_unittest.mm

Status: Verified (was: Fixed)
Verified in:

App Version: 62.0.3202.28 beta
Devices: iPhone 7, iPhone 7 Plus, iPad Mini 2
iOS Versions: 10.3.3, 11.0 GM

Payment Request has been enabled by default. I could navigate to all Payment Request URLs successfully.

Sign in to add a comment