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

Issue 761357 link

Starred by 1 user

Issue metadata

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



Sign in to add a comment

Record Journey Logger events when the page goes away.

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

Issue description

Payment Request instances tied to a page must record the appropriate Journey Logger events when the page/tab goes away.
 
Status: Started (was: Assigned)
Project Member

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

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

commit 81b057f734465e321da2ba8221d29605a34cba75
Author: Mohamad Ahmadi <mahmadi@chromium.org>
Date: Fri Sep 01 20:50:44 2017

[Payment Request] Records metrics when request is aborted or page goes away

Refactors the abort logic and helper functions.
Records the appropriate journey logger metrics when the request is aborted.
Records the appropriate journey logger metrics when the page/tab goes away.
Adds the remainder of the journey logger tests.

Bug:  761357 ,  602666 
Change-Id: I65a1196a530c2bded086e1edd94d915f3ff8acc8
Reviewed-on: https://chromium-review.googlesource.com/646877
Commit-Queue: mahmadi (Moe) <mahmadi@chromium.org>
Reviewed-by: Marc-Antoine Courteau <macourteau@chromium.org>
Cr-Commit-Position: refs/heads/master@{#499287}
[modify] https://crrev.com/81b057f734465e321da2ba8221d29605a34cba75/ios/chrome/browser/ui/payments/payment_request_journey_logger_egtest.mm
[modify] https://crrev.com/81b057f734465e321da2ba8221d29605a34cba75/ios/chrome/browser/ui/payments/payment_request_manager.mm

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

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

Labels: -Merge-Request-62 Hotlist-Merge-Approved Merge-Approved-62
Your change meets the bar and is auto-approved for M62. Please go ahead and merge the CL to branch 3202 manually. Please contact 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
Project Member

Comment 6 by bugdroid1@chromium.org, Sep 11 2017

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

commit 81962dd0b997caed055fa5589c52f724aff342d2
Author: Mohamad Ahmadi <mahmadi@chromium.org>
Date: Mon Sep 11 17:49:11 2017

[Payment Request] Records metrics when request is aborted or page goes away

Refactors the abort logic and helper functions.
Records the appropriate journey logger metrics when the request is aborted.
Records the appropriate journey logger metrics when the page/tab goes away.
Adds the remainder of the journey logger tests.

TBR=mahmadi@chromium.org

(cherry picked from commit 81b057f734465e321da2ba8221d29605a34cba75)

Bug:  761357 ,  602666 
Change-Id: I65a1196a530c2bded086e1edd94d915f3ff8acc8
Reviewed-on: https://chromium-review.googlesource.com/646877
Commit-Queue: mahmadi (Moe) <mahmadi@chromium.org>
Reviewed-by: Marc-Antoine Courteau <macourteau@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#499287}
Reviewed-on: https://chromium-review.googlesource.com/660400
Reviewed-by: Moe Ahmadi <mahmadi@chromium.org>
Cr-Commit-Position: refs/branch-heads/3202@{#132}
Cr-Branched-From: fa6a5d87adff761bc16afc5498c3f5944c1daa68-refs/heads/master@{#499098}
[modify] https://crrev.com/81962dd0b997caed055fa5589c52f724aff342d2/ios/chrome/browser/ui/payments/payment_request_journey_logger_egtest.mm
[modify] https://crrev.com/81962dd0b997caed055fa5589c52f724aff342d2/ios/chrome/browser/ui/payments/payment_request_manager.mm

The CL that was cherry-picked depends on https://chromium-review.googlesource.com/c/chromium/src/+/646660 that has not been cherry-picked, thus breaking the build with the following error:

../../ios/chrome/browser/ui/payments/payment_request_manager.mm:1078:23: error: no member named 'IsRendererInitiated' in 'web::NavigationContext'
          navigation->IsRendererInitiated()
          ~~~~~~~~~~  ^
1 error generated.

Will revert the cherry-picking.
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/+/0aa757d1ebc735a2533a8a77a11bd352f378a99e

commit 0aa757d1ebc735a2533a8a77a11bd352f378a99e
Author: Sylvain Defresne <sdefresne@chromium.org>
Date: Tue Sep 12 16:23:56 2017

Revert "[Payment Request] Records metrics when request is aborted or page goes away"

This reverts commit 81962dd0b997caed055fa5589c52f724aff342d2.

Reason for revert: this CL depends on https://chromium-review.googlesource.com/c/chromium/src/+/646660 that has not been cherry-picked in branch 3202 thus breaking the build with the following error:

../../ios/chrome/browser/ui/payments/payment_request_manager.mm:1078:23: error: no member named 'IsRendererInitiated' in 'web::NavigationContext'
          navigation->IsRendererInitiated()
          ~~~~~~~~~~  ^
1 error generated.

Original change's description:
> [Payment Request] Records metrics when request is aborted or page goes away
> 
> Refactors the abort logic and helper functions.
> Records the appropriate journey logger metrics when the request is aborted.
> Records the appropriate journey logger metrics when the page/tab goes away.
> Adds the remainder of the journey logger tests.
> 
> TBR=mahmadi@chromium.org
> 
> (cherry picked from commit 81b057f734465e321da2ba8221d29605a34cba75)
> 
> Bug:  761357 ,  602666 
> Change-Id: I65a1196a530c2bded086e1edd94d915f3ff8acc8
> Reviewed-on: https://chromium-review.googlesource.com/646877
> Commit-Queue: mahmadi (Moe) <mahmadi@chromium.org>
> Reviewed-by: Marc-Antoine Courteau <macourteau@chromium.org>
> Cr-Original-Commit-Position: refs/heads/master@{#499287}
> Reviewed-on: https://chromium-review.googlesource.com/660400
> Reviewed-by: Moe Ahmadi <mahmadi@chromium.org>
> Cr-Commit-Position: refs/branch-heads/3202@{#132}
> Cr-Branched-From: fa6a5d87adff761bc16afc5498c3f5944c1daa68-refs/heads/master@{#499098}

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

Change-Id: Idca82702136652d17d07b2c315243a7c925db34e
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  761357 ,  602666 
Reviewed-on: https://chromium-review.googlesource.com/663540
Reviewed-by: Sylvain Defresne <sdefresne@chromium.org>
Cr-Commit-Position: refs/branch-heads/3202@{#168}
Cr-Branched-From: fa6a5d87adff761bc16afc5498c3f5944c1daa68-refs/heads/master@{#499098}
[modify] https://crrev.com/0aa757d1ebc735a2533a8a77a11bd352f378a99e/ios/chrome/browser/ui/payments/payment_request_journey_logger_egtest.mm
[modify] https://crrev.com/0aa757d1ebc735a2533a8a77a11bd352f378a99e/ios/chrome/browser/ui/payments/payment_request_manager.mm

Labels: -Hotlist-Merge-Approved -merge-merged-3202 Merge-Request-62
Back to requesting the merge I guess, but need to merge the following two CLs (in order):

- https://chromium-review.googlesource.com/c/chromium/src/+/646660
- https://chromium-review.googlesource.com/c/chromium/src/+/646877

Project Member

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

Labels: merge-merged-3202
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/3f88833b45c3d5b832e51a4ec2759c90252d2ca7

commit 3f88833b45c3d5b832e51a4ec2759c90252d2ca7
Author: Mohamad Ahmadi <mahmadi@chromium.org>
Date: Tue Sep 12 16:43:46 2017

Adds IsRendererInitiated to NavigationContext.

This mimics content/public/browser/navigation_handle.h. IsRenderInitiated will return true if the current navigation was initiated by the renderer, and false if it was user initiated.

TBR=macourteau@chromium.org

(cherry picked from commit c6d51440206327ee177c949a4e7e1e9a8120a555)

Bug:  761357 
Change-Id: I094442e501a17a4741edca228810533e4b08253f
Reviewed-on: https://chromium-review.googlesource.com/646660
Commit-Queue: Marc-Antoine Courteau <macourteau@chromium.org>
Reviewed-by: Rohit Rao (ping after 24h) <rohitrao@chromium.org>
Reviewed-by: Kurt Horimoto <kkhorimoto@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#499188}
Reviewed-on: https://chromium-review.googlesource.com/663562
Reviewed-by: Moe Ahmadi <mahmadi@chromium.org>
Cr-Commit-Position: refs/branch-heads/3202@{#170}
Cr-Branched-From: fa6a5d87adff761bc16afc5498c3f5944c1daa68-refs/heads/master@{#499098}
[modify] https://crrev.com/3f88833b45c3d5b832e51a4ec2759c90252d2ca7/ios/chrome/browser/tabs/tab_unittest.mm
[modify] https://crrev.com/3f88833b45c3d5b832e51a4ec2759c90252d2ca7/ios/web/public/test/fakes/crw_test_web_state_observer.mm
[modify] https://crrev.com/3f88833b45c3d5b832e51a4ec2759c90252d2ca7/ios/web/public/test/fakes/fake_navigation_context.h
[modify] https://crrev.com/3f88833b45c3d5b832e51a4ec2759c90252d2ca7/ios/web/public/test/fakes/fake_navigation_context.mm
[modify] https://crrev.com/3f88833b45c3d5b832e51a4ec2759c90252d2ca7/ios/web/public/test/fakes/test_web_state_observer.mm
[modify] https://crrev.com/3f88833b45c3d5b832e51a4ec2759c90252d2ca7/ios/web/public/web_state/navigation_context.h
[modify] https://crrev.com/3f88833b45c3d5b832e51a4ec2759c90252d2ca7/ios/web/web_state/navigation_context_impl.h
[modify] https://crrev.com/3f88833b45c3d5b832e51a4ec2759c90252d2ca7/ios/web/web_state/navigation_context_impl.mm
[modify] https://crrev.com/3f88833b45c3d5b832e51a4ec2759c90252d2ca7/ios/web/web_state/navigation_context_impl_unittest.mm
[modify] https://crrev.com/3f88833b45c3d5b832e51a4ec2759c90252d2ca7/ios/web/web_state/ui/crw_web_controller.mm
[modify] https://crrev.com/3f88833b45c3d5b832e51a4ec2759c90252d2ca7/ios/web/web_state/ui/crw_wk_navigation_states_unittest.mm
[modify] https://crrev.com/3f88833b45c3d5b832e51a4ec2759c90252d2ca7/ios/web/web_state/web_state_impl_unittest.mm
[modify] https://crrev.com/3f88833b45c3d5b832e51a4ec2759c90252d2ca7/ios/web/web_state/web_state_observer_bridge_unittest.mm
[modify] https://crrev.com/3f88833b45c3d5b832e51a4ec2759c90252d2ca7/ios/web/webui/crw_web_ui_manager_unittest.mm

Project Member

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

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

commit d2d189703dde91d6431ff952d31bbfa7f3dcb577
Author: Moe Ahmadi <mahmadi@chromium.org>
Date: Tue Sep 12 17:47:07 2017

Revert "Revert "[Payment Request] Records metrics when request is aborted or page goes away""

This reverts commit 0aa757d1ebc735a2533a8a77a11bd352f378a99e.

Reason for revert: <INSERT REASONING HERE>

Original change's description:
> Revert "[Payment Request] Records metrics when request is aborted or page goes away"
> 
> This reverts commit 81962dd0b997caed055fa5589c52f724aff342d2.
> 
> Reason for revert: this CL depends on https://chromium-review.googlesource.com/c/chromium/src/+/646660 that has not been cherry-picked in branch 3202 thus breaking the build with the following error:
> 
> ../../ios/chrome/browser/ui/payments/payment_request_manager.mm:1078:23: error: no member named 'IsRendererInitiated' in 'web::NavigationContext'
>           navigation->IsRendererInitiated()
>           ~~~~~~~~~~  ^
> 1 error generated.
> 
> Original change's description:
> > [Payment Request] Records metrics when request is aborted or page goes away
> > 
> > Refactors the abort logic and helper functions.
> > Records the appropriate journey logger metrics when the request is aborted.
> > Records the appropriate journey logger metrics when the page/tab goes away.
> > Adds the remainder of the journey logger tests.
> > 
> > TBR=mahmadi@chromium.org
> > 
> > (cherry picked from commit 81b057f734465e321da2ba8221d29605a34cba75)
> > 
> > Bug:  761357 ,  602666 
> > Change-Id: I65a1196a530c2bded086e1edd94d915f3ff8acc8
> > Reviewed-on: https://chromium-review.googlesource.com/646877
> > Commit-Queue: mahmadi (Moe) <mahmadi@chromium.org>
> > Reviewed-by: Marc-Antoine Courteau <macourteau@chromium.org>
> > Cr-Original-Commit-Position: refs/heads/master@{#499287}
> > Reviewed-on: https://chromium-review.googlesource.com/660400
> > Reviewed-by: Moe Ahmadi <mahmadi@chromium.org>
> > Cr-Commit-Position: refs/branch-heads/3202@{#132}
> > Cr-Branched-From: fa6a5d87adff761bc16afc5498c3f5944c1daa68-refs/heads/master@{#499098}
> 
> TBR=macourteau@chromium.org,mahmadi@chromium.org
> 
> Change-Id: Idca82702136652d17d07b2c315243a7c925db34e
> No-Presubmit: true
> No-Tree-Checks: true
> No-Try: true
> Bug:  761357 ,  602666 
> Reviewed-on: https://chromium-review.googlesource.com/663540
> Reviewed-by: Sylvain Defresne <sdefresne@chromium.org>
> Cr-Commit-Position: refs/branch-heads/3202@{#168}
> Cr-Branched-From: fa6a5d87adff761bc16afc5498c3f5944c1daa68-refs/heads/master@{#499098}

TBR=macourteau@chromium.org,sdefresne@chromium.org,mahmadi@chromium.org

Change-Id: I481de4fd41e4b83218283ffc8b4dbf29a7332f2d
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  761357 ,  602666 
Reviewed-on: https://chromium-review.googlesource.com/663565
Reviewed-by: Moe Ahmadi <mahmadi@chromium.org>
Reviewed-by: Marc-Antoine Courteau <macourteau@chromium.org>
Cr-Commit-Position: refs/branch-heads/3202@{#174}
Cr-Branched-From: fa6a5d87adff761bc16afc5498c3f5944c1daa68-refs/heads/master@{#499098}
[modify] https://crrev.com/d2d189703dde91d6431ff952d31bbfa7f3dcb577/ios/chrome/browser/ui/payments/payment_request_journey_logger_egtest.mm
[modify] https://crrev.com/d2d189703dde91d6431ff952d31bbfa7f3dcb577/ios/chrome/browser/ui/payments/payment_request_manager.mm

mahmadi@ please provide the rationale as to why do we need to merge this please. How will happen if we don't take this? What could be impacted in the worst case scenario if we merge this.
I am seeing a lot of stuffs on payment request that needs to be merged. Was this feature code complete by Feature freeze?
Labels: -Merge-Request-62 Merge-Approved-62
Approving this as per our email conversation.
Project Member

Comment 17 by sheriffbot@chromium.org, Sep 18 2017

Cc: cma...@chromium.org sdefresne@chromium.org
This issue has been approved for a merge. Please merge the fix to any appropriate branches as soon as possible!

If all merges have been completed, please remove any remaining Merge-Approved labels from this issue.

Thanks for your time! To disable nags, add the Disable-Nags label.

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

Sign in to add a comment