Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2808513002:
[Payments] Add PaymentRequest checkout funnel UKMs. (Closed)
|
Created:
3 years, 8 months ago by sebsg Modified:
3 years, 8 months ago CC:
chromium-reviews, droger+watchlist_chromium.org, blundell+watchlist_chromium.org, sdefresne+watchlist_chromium.org, jam, gogerald+paymentswatch_chromium.org, rouslan+payments_chromium.org, darin-cc_chromium.org, agrieve+watch_chromium.org, mahmadi+paymentswatch_chromium.org, sebsg+paymentswatch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[Payments] Add PaymentRequest checkout funnel UKMs.
BUG=708999
Review-Url: https://codereview.chromium.org/2808513002
Cr-Commit-Position: refs/heads/master@{#463895}
Committed: https://chromium.googlesource.com/chromium/src/+/fa910f2825276797c554a3bfcfb647cf7be22678
Patch Set 1 #
Total comments: 30
Patch Set 2 : Addressed Mathp's comments #
Total comments: 12
Patch Set 3 : Addressed comments #
Total comments: 10
Patch Set 4 : Addressed Rouslan's comments #Patch Set 5 : Added UKM entry #
Total comments: 6
Patch Set 6 : Added comment describing where the values are defined in ukm.xml #Patch Set 7 : Rebase #Messages
Total messages: 99 (76 generated)
The CQ bit was checked by sebsg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by sebsg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by sebsg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by sebsg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Patchset #2 (id:20001) has been deleted
Patchset #2 (id:40001) has been deleted
Patchset #1 (id:1) has been deleted
The CQ bit was checked by sebsg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by sebsg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #1 (id:60001) has been deleted
Patchset #1 (id:80001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by sebsg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #1 (id:100001) has been deleted
Description was changed from ========== WIP [Payments] Add PaymentRequest checkout funnel UKMs. BUG=708999 ========== to ========== [Payments] Add PaymentRequest checkout funnel UKMs. BUG=708999 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by sebsg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #1 (id:120001) has been deleted
sebsg@chromium.org changed reviewers: + mathp@google.com
Hi Math, PTAL? :)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
mathp@chromium.org changed reviewers: + mathp@chromium.org - mathp@google.com
Thanks excited to see this https://codereview.chromium.org/2808513002/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java (right): https://codereview.chromium.org/2808513002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:383: mJourneyLogger = new JourneyLogger(mIsIncognito, mOriginForDisplay); I feel like we are tying two things that shouldn't be tied: displaying the URL, and using it for UKM. What if one changes (e.g., we stop displaying the scheme)? I would use mWebContents.getLastCommittedUrl() here https://codereview.chromium.org/2808513002/diff/140001/chrome/browser/android... File chrome/browser/android/chrome_jni_registrar.cc (right): https://codereview.chromium.org/2808513002/diff/140001/chrome/browser/android... chrome/browser/android/chrome_jni_registrar.cc:333: {"JourneyLoggerAndroid", payments::JourneyLoggerAndroid::Register}, Can you use the approach on line 228 of declaring a general ChromePayments thing instead? {"ChromePayments", payments::android::RegisterChromePayments}, The approach in https://cs.chromium.org/chromium/src/components/payments/content/android/comp... is much better if we ever need to add different implementations. https://codereview.chromium.org/2808513002/diff/140001/chrome/browser/payment... File chrome/browser/payments/android/journey_logger_android.h (right): https://codereview.chromium.org/2808513002/diff/140001/chrome/browser/payment... chrome/browser/payments/android/journey_logger_android.h:22: explicit JourneyLoggerAndroid(bool is_incognito, std::string url); no explicit https://codereview.chromium.org/2808513002/diff/140001/chrome/browser/payment... chrome/browser/payments/android/journey_logger_android.h:22: explicit JourneyLoggerAndroid(bool is_incognito, std::string url); const std::string& url? https://codereview.chromium.org/2808513002/diff/140001/components/payments/co... File components/payments/core/BUILD.gn (right): https://codereview.chromium.org/2808513002/diff/140001/components/payments/co... components/payments/core/BUILD.gn:70: "//skia", ?? https://codereview.chromium.org/2808513002/diff/140001/components/payments/co... File components/payments/core/journey_logger.cc (right): https://codereview.chromium.org/2808513002/diff/140001/components/payments/co... components/payments/core/journey_logger.cc:16: // Name constants are exposed here so they can be referenced from tests. remove https://codereview.chromium.org/2808513002/diff/140001/components/payments/co... components/payments/core/journey_logger.cc:112: current_step_ = step; Should we dcheck that step > current_step_? https://codereview.chromium.org/2808513002/diff/140001/components/payments/co... File components/payments/core/journey_logger.h (right): https://codereview.chromium.org/2808513002/diff/140001/components/payments/co... components/payments/core/journey_logger.h:62: enum CompletionStep { Completion is misleading, let's find a better term https://codereview.chromium.org/2808513002/diff/140001/components/payments/co... components/payments/core/journey_logger.h:63: STEP_INITIATED = 0, had an idea, let me know what you think. if each value was 1 << 0, 1 << 1, etc., we could accumulate checkout_step_ |= step; And this would allow us to log various other steps (not necessarily linear) easily. Let's say I extend your example and we have SELECTED_SHIPPING_OPTION = 1 << 4 SELECTED_PAYMENT_METHOD = 1 << 5 etc. we could de facto differentiate in the metrics between 00111 and 11111 or 10111, which are all success cases but with variations. https://codereview.chromium.org/2808513002/diff/140001/components/payments/co... components/payments/core/journey_logger.h:66: STEP_RECEIVED_INSTRUMENT_DETAILS = 3, I would recommend removing the STEP_ prefix from all these https://codereview.chromium.org/2808513002/diff/140001/components/payments/co... components/payments/core/journey_logger.h:77: explicit JourneyLogger(bool is_incognito, no explicit https://codereview.chromium.org/2808513002/diff/140001/components/payments/co... components/payments/core/journey_logger.h:155: void RecordUkms(CompletionStatus completion_status); RecordUrlKeyedMetrics is nice and descriptive https://codereview.chromium.org/2808513002/diff/140001/components/payments/co... components/payments/core/journey_logger.h:165: // For the UKMs. nit: I don't think we need this line https://codereview.chromium.org/2808513002/diff/140001/components/payments/co... File components/payments/core/journey_logger_unittest.cc (right): https://codereview.chromium.org/2808513002/diff/140001/components/payments/co... components/payments/core/journey_logger_unittest.cc:24: const ukm::Entry_Metric* FindMetric( talk to UKM folks about putting this inside test_ukm_service?
The CQ bit was checked by sebsg@chromium.org to run a CQ dry run
Patchset #2 (id:160001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks Math, another look? https://codereview.chromium.org/2808513002/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java (right): https://codereview.chromium.org/2808513002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:383: mJourneyLogger = new JourneyLogger(mIsIncognito, mOriginForDisplay); On 2017/04/10 01:03:01, Mathieu wrote: > I feel like we are tying two things that shouldn't be tied: displaying the URL, > and using it for UKM. What if one changes (e.g., we stop displaying the scheme)? > I would use mWebContents.getLastCommittedUrl() here Done. https://codereview.chromium.org/2808513002/diff/140001/chrome/browser/android... File chrome/browser/android/chrome_jni_registrar.cc (right): https://codereview.chromium.org/2808513002/diff/140001/chrome/browser/android... chrome/browser/android/chrome_jni_registrar.cc:333: {"JourneyLoggerAndroid", payments::JourneyLoggerAndroid::Register}, On 2017/04/10 01:03:01, Mathieu wrote: > Can you use the approach on line 228 of declaring a general ChromePayments thing > instead? > > {"ChromePayments", payments::android::RegisterChromePayments}, > > The approach in > https://cs.chromium.org/chromium/src/components/payments/content/android/comp... > is much better if we ever need to add different implementations. Done. https://codereview.chromium.org/2808513002/diff/140001/chrome/browser/payment... File chrome/browser/payments/android/journey_logger_android.h (right): https://codereview.chromium.org/2808513002/diff/140001/chrome/browser/payment... chrome/browser/payments/android/journey_logger_android.h:22: explicit JourneyLoggerAndroid(bool is_incognito, std::string url); On 2017/04/10 01:03:01, Mathieu wrote: > const std::string& url? Done. https://codereview.chromium.org/2808513002/diff/140001/chrome/browser/payment... chrome/browser/payments/android/journey_logger_android.h:22: explicit JourneyLoggerAndroid(bool is_incognito, std::string url); On 2017/04/10 01:03:01, Mathieu wrote: > no explicit Done. https://codereview.chromium.org/2808513002/diff/140001/components/payments/co... File components/payments/core/BUILD.gn (right): https://codereview.chromium.org/2808513002/diff/140001/components/payments/co... components/payments/core/BUILD.gn:70: "//skia", On 2017/04/10 01:03:01, Mathieu wrote: > ?? Done. https://codereview.chromium.org/2808513002/diff/140001/components/payments/co... File components/payments/core/journey_logger.cc (right): https://codereview.chromium.org/2808513002/diff/140001/components/payments/co... components/payments/core/journey_logger.cc:16: // Name constants are exposed here so they can be referenced from tests. On 2017/04/10 01:03:01, Mathieu wrote: > remove Done. https://codereview.chromium.org/2808513002/diff/140001/components/payments/co... components/payments/core/journey_logger.cc:112: current_step_ = step; On 2017/04/10 01:03:01, Mathieu wrote: > Should we dcheck that step > current_step_? Changed to bitfield as per you other suggestion https://codereview.chromium.org/2808513002/diff/140001/components/payments/co... File components/payments/core/journey_logger.h (right): https://codereview.chromium.org/2808513002/diff/140001/components/payments/co... components/payments/core/journey_logger.h:62: enum CompletionStep { On 2017/04/10 01:03:02, Mathieu wrote: > Completion is misleading, let's find a better term Done. https://codereview.chromium.org/2808513002/diff/140001/components/payments/co... components/payments/core/journey_logger.h:63: STEP_INITIATED = 0, On 2017/04/10 01:03:02, Mathieu wrote: > had an idea, let me know what you think. > > if each value was 1 << 0, 1 << 1, etc., we could accumulate > > checkout_step_ |= step; > > And this would allow us to log various other steps (not necessarily linear) > easily. > > Let's say I extend your example and we have > > SELECTED_SHIPPING_OPTION = 1 << 4 > SELECTED_PAYMENT_METHOD = 1 << 5 > etc. > > we could de facto differentiate in the metrics between > 00111 and 11111 or 10111, which are all success cases but with variations. Excellent idea! I was thinking that we should add more stuff, like added card or something. This is perfect! https://codereview.chromium.org/2808513002/diff/140001/components/payments/co... components/payments/core/journey_logger.h:66: STEP_RECEIVED_INSTRUMENT_DETAILS = 3, On 2017/04/10 01:03:02, Mathieu wrote: > I would recommend removing the STEP_ prefix from all these Acknowledged. https://codereview.chromium.org/2808513002/diff/140001/components/payments/co... components/payments/core/journey_logger.h:77: explicit JourneyLogger(bool is_incognito, On 2017/04/10 01:03:02, Mathieu wrote: > no explicit Done. https://codereview.chromium.org/2808513002/diff/140001/components/payments/co... components/payments/core/journey_logger.h:77: explicit JourneyLogger(bool is_incognito, On 2017/04/10 01:03:02, Mathieu wrote: > no explicit Done. https://codereview.chromium.org/2808513002/diff/140001/components/payments/co... components/payments/core/journey_logger.h:155: void RecordUkms(CompletionStatus completion_status); On 2017/04/10 01:03:02, Mathieu wrote: > RecordUrlKeyedMetrics is nice and descriptive Done. https://codereview.chromium.org/2808513002/diff/140001/components/payments/co... components/payments/core/journey_logger.h:165: // For the UKMs. On 2017/04/10 01:03:02, Mathieu wrote: > nit: I don't think we need this line Done. https://codereview.chromium.org/2808513002/diff/140001/components/payments/co... components/payments/core/journey_logger.h:165: // For the UKMs. On 2017/04/10 01:03:02, Mathieu wrote: > nit: I don't think we need this line Done. https://codereview.chromium.org/2808513002/diff/140001/components/payments/co... File components/payments/core/journey_logger_unittest.cc (right): https://codereview.chromium.org/2808513002/diff/140001/components/payments/co... components/payments/core/journey_logger_unittest.cc:24: const ukm::Entry_Metric* FindMetric( On 2017/04/10 01:03:02, Mathieu wrote: > talk to UKM folks about putting this inside test_ukm_service? Yeah I use it in every test. I'll ask them
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
https://codereview.chromium.org/2808513002/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/JourneyLogger.java (right): https://codereview.chromium.org/2808513002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/JourneyLogger.java:42: public static final int EVENT_SHOWN = 1 << 0; nit: what do you think of this sort of alignment? https://cs.chromium.org/chromium/src/base/logging.h?rcl=89669caf0861ddd3a08e3... https://codereview.chromium.org/2808513002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/JourneyLogger.java:132: public void setEventOccured(int event) { *occurred https://codereview.chromium.org/2808513002/diff/180001/components/payments/co... File components/payments/core/journey_logger.cc (right): https://codereview.chromium.org/2808513002/diff/180001/components/payments/co... components/payments/core/journey_logger.cc:75: events_(EVENT_NOT_SHOWN), I liked initiated https://codereview.chromium.org/2808513002/diff/180001/components/payments/co... File components/payments/core/journey_logger.h (right): https://codereview.chromium.org/2808513002/diff/180001/components/payments/co... components/payments/core/journey_logger.h:63: static const int EVENT_NOT_SHOWN = 0; I liked the enum! https://codereview.chromium.org/2808513002/diff/180001/components/payments/co... components/payments/core/journey_logger.h:64: static const int EVENT_SHOWN = 1 << 0; same comment about alignment: https://cs.chromium.org/chromium/src/base/logging.h?rcl=89669caf0861ddd3a08e3... https://codereview.chromium.org/2808513002/diff/180001/components/payments/co... components/payments/core/journey_logger.h:160: int events_; Add a comment talking about what it's doing and why it's plural (events get accumulated into this).
The CQ bit was checked by sebsg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by sebsg@chromium.org to run a CQ dry run
Patchset #3 (id:200001) has been deleted
Thanks! another look? https://codereview.chromium.org/2808513002/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/JourneyLogger.java (right): https://codereview.chromium.org/2808513002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/JourneyLogger.java:42: public static final int EVENT_SHOWN = 1 << 0; On 2017/04/10 15:56:18, Mathieu wrote: > nit: what do you think of this sort of alignment? > > https://cs.chromium.org/chromium/src/base/logging.h?rcl=89669caf0861ddd3a08e3... git cl format puts it back this way. WDYT? https://codereview.chromium.org/2808513002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/JourneyLogger.java:132: public void setEventOccured(int event) { On 2017/04/10 15:56:18, Mathieu wrote: > *occurred Done. https://codereview.chromium.org/2808513002/diff/180001/components/payments/co... File components/payments/core/journey_logger.cc (right): https://codereview.chromium.org/2808513002/diff/180001/components/payments/co... components/payments/core/journey_logger.cc:75: events_(EVENT_NOT_SHOWN), On 2017/04/10 15:56:18, Mathieu wrote: > I liked initiated Done. https://codereview.chromium.org/2808513002/diff/180001/components/payments/co... File components/payments/core/journey_logger.h (right): https://codereview.chromium.org/2808513002/diff/180001/components/payments/co... components/payments/core/journey_logger.h:63: static const int EVENT_NOT_SHOWN = 0; On 2017/04/10 15:56:18, Mathieu wrote: > I liked the enum! Done. https://codereview.chromium.org/2808513002/diff/180001/components/payments/co... components/payments/core/journey_logger.h:64: static const int EVENT_SHOWN = 1 << 0; On 2017/04/10 15:56:18, Mathieu wrote: > same comment about alignment: > https://cs.chromium.org/chromium/src/base/logging.h?rcl=89669caf0861ddd3a08e3... Same as above. https://codereview.chromium.org/2808513002/diff/180001/components/payments/co... components/payments/core/journey_logger.h:160: int events_; On 2017/04/10 15:56:18, Mathieu wrote: > Add a comment talking about what it's doing and why it's plural (events get > accumulated into this). Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
sebsg@chromium.org changed reviewers: + rouslan@chromium.org
Hi Rouslan, PTAL?
https://codereview.chromium.org/2808513002/diff/220001/components/payments/co... File components/payments/core/journey_logger.h (right): https://codereview.chromium.org/2808513002/diff/220001/components/payments/co... components/payments/core/journey_logger.h:162: // Accumulates the many event that have happened during the Payment Request. *events
sebsg@chromium.org changed reviewers: + holte@chromium.org
holte@chromium.org: could you please review ukm_service? thanks!
lgtm % comments and questions. https://codereview.chromium.org/2808513002/diff/220001/components/payments/co... File components/payments/core/journey_logger.h (right): https://codereview.chromium.org/2808513002/diff/220001/components/payments/co... components/payments/core/journey_logger.h:17: namespace internal { Put this inside of payments namespace please. https://codereview.chromium.org/2808513002/diff/220001/components/payments/co... components/payments/core/journey_logger.h:78: JourneyLogger(bool is_incognito, GURL url, ukm::UkmService* ukm_service); Add a comment about life time expectations for ukm_service. https://codereview.chromium.org/2808513002/diff/220001/components/payments/co... components/payments/core/journey_logger.h:78: JourneyLogger(bool is_incognito, GURL url, ukm::UkmService* ukm_service); url should be const-ref. https://codereview.chromium.org/2808513002/diff/220001/components/ukm/ukm_ser... File components/ukm/ukm_service.h (right): https://codereview.chromium.org/2808513002/diff/220001/components/ukm/ukm_ser... components/ukm/ukm_service.h:117: friend payments::JourneyLogger; Why are you reaching into the privates of ukm service?
The CQ bit was checked by sebsg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks Rouslan! https://codereview.chromium.org/2808513002/diff/220001/components/payments/co... File components/payments/core/journey_logger.h (right): https://codereview.chromium.org/2808513002/diff/220001/components/payments/co... components/payments/core/journey_logger.h:17: namespace internal { On 2017/04/10 19:20:03, ಠ_ಠ wrote: > Put this inside of payments namespace please. Done. https://codereview.chromium.org/2808513002/diff/220001/components/payments/co... components/payments/core/journey_logger.h:78: JourneyLogger(bool is_incognito, GURL url, ukm::UkmService* ukm_service); On 2017/04/10 19:20:03, ಠ_ಠ wrote: > url should be const-ref. Done. https://codereview.chromium.org/2808513002/diff/220001/components/payments/co... components/payments/core/journey_logger.h:78: JourneyLogger(bool is_incognito, GURL url, ukm::UkmService* ukm_service); On 2017/04/10 19:20:03, ಠ_ಠ wrote: > Add a comment about life time expectations for ukm_service. Added the comment on the stored pointer, seems like that's the convention looking at payment_request_state.h for example. https://codereview.chromium.org/2808513002/diff/220001/components/payments/co... components/payments/core/journey_logger.h:162: // Accumulates the many event that have happened during the Payment Request. On 2017/04/10 19:03:15, Mathieu wrote: > *events Done. https://codereview.chromium.org/2808513002/diff/220001/components/ukm/ukm_ser... File components/ukm/ukm_service.h (right): https://codereview.chromium.org/2808513002/diff/220001/components/ukm/ukm_ser... components/ukm/ukm_service.h:117: friend payments::JourneyLogger; On 2017/04/10 19:20:03, ಠ_ಠ wrote: > Why are you reaching into the privates of ukm service? Love your wording :P I need to be friends to be able to access GetEntryBuilder. All the UKM users are friends for now, this is intentional, see line 128-133 in this file for more info.
sebsg@chromium.org changed reviewers: + tedchoc@chromium.org
tedchoc@chromium.org: Could you please review chrome/browser/android/chrome_jni_registrar.cc ? Thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Please document your entry in tools/metrics/ukm/ukm.xml
The CQ bit was checked by sebsg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks, I added the entry. Another look?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/04/11 12:38:37, sebsg wrote: > Thanks, I added the entry. Another look? chrome/browser/android/chrome_jni_registrar.cc - lgtm
lgtm https://codereview.chromium.org/2808513002/diff/260001/tools/metrics/ukm/ukm.xml File tools/metrics/ukm/ukm.xml (right): https://codereview.chromium.org/2808513002/diff/260001/tools/metrics/ukm/ukm.... tools/metrics/ukm/ukm.xml:18: Whether the upload was proposed to the user or the reson why it was not. Nit: reson->reason https://codereview.chromium.org/2808513002/diff/260001/tools/metrics/ukm/ukm.... tools/metrics/ukm/ukm.xml:52: How the Payment Request ended. Reference where the values are defined. https://codereview.chromium.org/2808513002/diff/260001/tools/metrics/ukm/ukm.... tools/metrics/ukm/ukm.xml:57: The events that occurred during the Payment Request. Maybe mention that this a bitfield of events, and where the bits are defined.
Thanks sending to CQ. https://codereview.chromium.org/2808513002/diff/260001/tools/metrics/ukm/ukm.xml File tools/metrics/ukm/ukm.xml (right): https://codereview.chromium.org/2808513002/diff/260001/tools/metrics/ukm/ukm.... tools/metrics/ukm/ukm.xml:18: Whether the upload was proposed to the user or the reson why it was not. On 2017/04/11 21:13:10, Steven Holte wrote: > Nit: reson->reason Done. https://codereview.chromium.org/2808513002/diff/260001/tools/metrics/ukm/ukm.... tools/metrics/ukm/ukm.xml:52: How the Payment Request ended. On 2017/04/11 21:13:10, Steven Holte wrote: > Reference where the values are defined. Done. https://codereview.chromium.org/2808513002/diff/260001/tools/metrics/ukm/ukm.... tools/metrics/ukm/ukm.xml:57: The events that occurred during the Payment Request. On 2017/04/11 21:13:10, Steven Holte wrote: > Maybe mention that this a bitfield of events, and where the bits are defined. Done.
The CQ bit was checked by sebsg@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mathp@chromium.org, rouslan@chromium.org, tedchoc@chromium.org, holte@chromium.org Link to the patchset: https://codereview.chromium.org/2808513002/#ps280001 (title: "Added comment describing where the values are defined in ukm.xml")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
The CQ bit was checked by sebsg@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mathp@chromium.org, tedchoc@chromium.org, rouslan@chromium.org, holte@chromium.org Link to the patchset: https://codereview.chromium.org/2808513002/#ps300001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Patchset #7 (id:300001) has been deleted
The CQ bit was checked by sebsg@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mathp@chromium.org, tedchoc@chromium.org, rouslan@chromium.org, holte@chromium.org Link to the patchset: https://codereview.chromium.org/2808513002/#ps320001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 320001, "attempt_start_ts": 1491956209143600,
"parent_rev": "06147322f84cd6dc908c41224b8e0c93f1da8518", "commit_rev":
"fa910f2825276797c554a3bfcfb647cf7be22678"}
Message was sent while issue was closed.
Description was changed from ========== [Payments] Add PaymentRequest checkout funnel UKMs. BUG=708999 ========== to ========== [Payments] Add PaymentRequest checkout funnel UKMs. BUG=708999 Review-Url: https://codereview.chromium.org/2808513002 Cr-Commit-Position: refs/heads/master@{#463895} Committed: https://chromium.googlesource.com/chromium/src/+/fa910f2825276797c554a3bfcfb6... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:320001) as https://chromium.googlesource.com/chromium/src/+/fa910f2825276797c554a3bfcfb6... |
