Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2870153002:
[Payments] Record navigations that close the Payment Request as aborts. (Closed)
|
Created:
3 years, 7 months ago by sebsg Modified:
3 years, 7 months ago Reviewers:
Mathieu CC:
chromium-reviews, tfarina, jam, gogerald+paymentswatch_chromium.org, rouslan+payments_chromium.org, darin-cc_chromium.org, mahmadi+paymentswatch_chromium.org, sebsg+paymentswatch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[Payments] Record navigations that close the Payment Request as aborts.
BUG=717757
Review-Url: https://codereview.chromium.org/2870153002
Cr-Commit-Position: refs/heads/master@{#472514}
Committed: https://chromium.googlesource.com/chromium/src/+/2c8558a602d83dbb979c30e6a18d271502b951fa
Patch Set 1 #
Total comments: 18
Patch Set 2 : Rebase #Patch Set 3 : Addressed commnets #Patch Set 4 : Rebase #
Total comments: 8
Patch Set 5 : Addressed comments #
Total comments: 1
Patch Set 6 : Addressed comments #Patch Set 7 : Rebase #Patch Set 8 : Fixed Windows error #
Messages
Total messages: 52 (38 generated)
The CQ bit was checked by sebsg@chromium.org to run a CQ dry run
Patchset #1 (id:1) 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...
sebsg@chromium.org changed reviewers: + mathp@chromium.org
Hi Math, PTAL?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: 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_...)
https://codereview.chromium.org/2870153002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/payments/payment_request_can_make_payment_metrics_browsertest.cc (right): https://codereview.chromium.org/2870153002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/payments/payment_request_can_make_payment_metrics_browsertest.cc:247: NavigateTo("/payment_request_email_test.html"); can you have a test that navigates to a different origin? Also one that reloads the page, and one that closes the tab. https://codereview.chromium.org/2870153002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/payments/payment_request_can_make_payment_metrics_browsertest.cc:262: DISABLED_Called_True_NotShown) { can you enable? https://codereview.chromium.org/2870153002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/payments/payment_request_can_make_payment_metrics_browsertest.cc:286: DISABLED_Called_False_NotShown) { same https://codereview.chromium.org/2870153002/diff/20001/components/payments/con... File components/payments/content/payment_request.cc (right): https://codereview.chromium.org/2870153002/diff/20001/components/payments/con... components/payments/content/payment_request.cc:209: JourneyLogger::COMPLETION_STATUS_OTHER_ABORTED); I don't think this line should be here, because it's in contradiction with line 142, amongst others? https://codereview.chromium.org/2870153002/diff/20001/components/payments/con... File components/payments/content/payment_request.h (right): https://codereview.chromium.org/2870153002/diff/20001/components/payments/con... components/payments/content/payment_request.h:78: // Called when there is a navigation out of a page with a PaymentRequest // Called when the frame attached to this PaymentRequest is navigating away, but before the PaymentRequest is destroyed. BTW does this include reloads and tab closing? https://codereview.chromium.org/2870153002/diff/20001/components/payments/con... components/payments/content/payment_request.h:80: void NavigatedAway(bool is_user_initiated); DidStartNavigation(bool is_user_initiated) is a more descriptive name. https://codereview.chromium.org/2870153002/diff/20001/components/payments/con... File components/payments/content/payment_request_web_contents_manager.cc (right): https://codereview.chromium.org/2870153002/diff/20001/components/payments/con... components/payments/content/payment_request_web_contents_manager.cc:45: for (auto& it : payment_requests_) { PaymentRequest& ? https://codereview.chromium.org/2870153002/diff/20001/components/payments/cor... File components/payments/core/journey_logger.cc (right): https://codereview.chromium.org/2870153002/diff/20001/components/payments/cor... components/payments/core/journey_logger.cc:113: if (has_recorded_) We should enforce this better, so I agree with the DCHECK
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 #2 (id:40001) 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: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Patchset #3 (id:80001) has been deleted
The CQ bit was checked by sebsg@chromium.org to run a CQ dry run
Thanks, another look? https://codereview.chromium.org/2870153002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/payments/payment_request_can_make_payment_metrics_browsertest.cc (right): https://codereview.chromium.org/2870153002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/payments/payment_request_can_make_payment_metrics_browsertest.cc:247: NavigateTo("/payment_request_email_test.html"); On 2017/05/09 18:12:19, Mathieu wrote: > can you have a test that navigates to a different origin? > > Also one that reloads the page, and one that closes the tab. Done. https://codereview.chromium.org/2870153002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/payments/payment_request_can_make_payment_metrics_browsertest.cc:262: DISABLED_Called_True_NotShown) { On 2017/05/09 18:12:19, Mathieu wrote: > can you enable? Done. https://codereview.chromium.org/2870153002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/payments/payment_request_can_make_payment_metrics_browsertest.cc:286: DISABLED_Called_False_NotShown) { On 2017/05/09 18:12:19, Mathieu wrote: > same Done. https://codereview.chromium.org/2870153002/diff/20001/components/payments/con... File components/payments/content/payment_request.cc (right): https://codereview.chromium.org/2870153002/diff/20001/components/payments/con... components/payments/content/payment_request.cc:209: JourneyLogger::COMPLETION_STATUS_OTHER_ABORTED); On 2017/05/09 18:12:19, Mathieu wrote: > I don't think this line should be here, because it's in contradiction with line > 142, amongst others? I still feel like we should log something here. Would you be OK with me logging an abort reason here, instead of the journey stats? https://codereview.chromium.org/2870153002/diff/20001/components/payments/con... File components/payments/content/payment_request.h (right): https://codereview.chromium.org/2870153002/diff/20001/components/payments/con... components/payments/content/payment_request.h:78: // Called when there is a navigation out of a page with a PaymentRequest On 2017/05/09 18:12:19, Mathieu wrote: > // Called when the frame attached to this PaymentRequest is navigating away, but > before the PaymentRequest is destroyed. > > BTW does this include reloads and tab closing? Reload triggers it but not a tab closing. However, tabclosing already registers as a User Abort. I'll add a test. https://codereview.chromium.org/2870153002/diff/20001/components/payments/con... components/payments/content/payment_request.h:80: void NavigatedAway(bool is_user_initiated); On 2017/05/09 18:12:19, Mathieu wrote: > DidStartNavigation(bool is_user_initiated) is a more descriptive name. Done. https://codereview.chromium.org/2870153002/diff/20001/components/payments/con... File components/payments/content/payment_request_web_contents_manager.cc (right): https://codereview.chromium.org/2870153002/diff/20001/components/payments/con... components/payments/content/payment_request_web_contents_manager.cc:45: for (auto& it : payment_requests_) { On 2017/05/09 18:12:19, Mathieu wrote: > PaymentRequest& ? the elements are std::pair<PaymentRequest*, std::unique_ptr<PaymentRequest>> no? Is there a way to only iterate on the values? https://codereview.chromium.org/2870153002/diff/20001/components/payments/cor... File components/payments/core/journey_logger.cc (right): https://codereview.chromium.org/2870153002/diff/20001/components/payments/cor... components/payments/core/journey_logger.cc:113: if (has_recorded_) On 2017/05/09 18:12:19, Mathieu wrote: > We should enforce this better, so I agree with the DCHECK My fear is that we'll be missing some logs. With the if, if we make good tests we can make sure it's fine. If we don't we may be missing some error logs and not know it. We could add an abort reason instead. So we'll be missing some journey logs but if we look at the abort reasons and see a lot of Abort_reason_uknown we can investigate more. What do you think?
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_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2870153002/diff/20001/components/payments/cor... File components/payments/core/journey_logger.cc (right): https://codereview.chromium.org/2870153002/diff/20001/components/payments/cor... components/payments/core/journey_logger.cc:113: if (has_recorded_) On 2017/05/10 14:58:48, sebsg wrote: > On 2017/05/09 18:12:19, Mathieu wrote: > > We should enforce this better, so I agree with the DCHECK > > My fear is that we'll be missing some logs. With the if, if we make good tests > we can make sure it's fine. If we don't we may be missing some error logs and > not know it. > > We could add an abort reason instead. So we'll be missing some journey logs but > if we look at the abort reasons and see a lot of Abort_reason_uknown we can > investigate more. > > What do you think? In the destructor we could add a DCHECK(has_recorded_), and here have DCHECK(!has_recorded_). A silent return like this is exactly how bugs are created, no matter how good the tests... With a silent return, you are actually counting on an order of operations thing, which you can't guarantee won't change over time, and as this class is used on other platforms! https://codereview.chromium.org/2870153002/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/payments/payment_request_can_make_payment_metrics_browsertest.cc (right): https://codereview.chromium.org/2870153002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/payments/payment_request_can_make_payment_metrics_browsertest.cc:378: // Simulate that the user navigates away from the Payment Request. ... by opening a different page on the same origin https://codereview.chromium.org/2870153002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/payments/payment_request_can_make_payment_metrics_browsertest.cc:402: NavigateTo("/google.com"); this will navigate to a file called "google.com" on our embedded webserver. See how this test does a.com/b.com: https://cs.chromium.org/chromium/src/chrome/browser/payments/site_per_process... https://codereview.chromium.org/2870153002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/payments/payment_request_can_make_payment_metrics_browsertest.cc:422: // Simulate that the user navigates away from the Payment Request. by closing the tab https://codereview.chromium.org/2870153002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/payments/payment_request_can_make_payment_metrics_browsertest.cc:444: // Simulate that the user navigates away from the Payment Request. ...by reloading
Patchset #5 (id:140001) has been deleted
https://codereview.chromium.org/2870153002/diff/20001/components/payments/cor... File components/payments/core/journey_logger.cc (right): https://codereview.chromium.org/2870153002/diff/20001/components/payments/cor... components/payments/core/journey_logger.cc:113: if (has_recorded_) On 2017/05/10 17:04:20, Mathieu wrote: > On 2017/05/10 14:58:48, sebsg wrote: > > On 2017/05/09 18:12:19, Mathieu wrote: > > > We should enforce this better, so I agree with the DCHECK > > > > My fear is that we'll be missing some logs. With the if, if we make good tests > > we can make sure it's fine. If we don't we may be missing some error logs and > > not know it. > > > > We could add an abort reason instead. So we'll be missing some journey logs > but > > if we look at the abort reasons and see a lot of Abort_reason_uknown we can > > investigate more. > > > > What do you think? > > In the destructor we could add a DCHECK(has_recorded_), and here have > DCHECK(!has_recorded_). A silent return like this is exactly how bugs are > created, no matter how good the tests... With a silent return, you are actually > counting on an order of operations thing, which you can't guarantee won't change > over time, and as this class is used on other platforms! Sure, I understand that, it's a good point so I had put back the DCHECK in the other patch. Good idea for the DCHECK in the destructor. But I don't think a DCHECK is enough. The sites in the wild will do all kinds of weird things we don't think about, and if we don't log something in that case, we'll be totally blind to that. So what do you think about my proposition about logging and abort reason instead of using the journey logger (mentioned in my previous comment in this thread)? https://codereview.chromium.org/2870153002/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/payments/payment_request_can_make_payment_metrics_browsertest.cc (right): https://codereview.chromium.org/2870153002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/payments/payment_request_can_make_payment_metrics_browsertest.cc:378: // Simulate that the user navigates away from the Payment Request. On 2017/05/10 17:04:20, Mathieu wrote: > ... by opening a different page on the same origin Done. https://codereview.chromium.org/2870153002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/payments/payment_request_can_make_payment_metrics_browsertest.cc:402: NavigateTo("/google.com"); On 2017/05/10 17:04:20, Mathieu wrote: > this will navigate to a file called "google.com" on our embedded webserver. > > See how this test does http://a.com/b.com: > https://cs.chromium.org/chromium/src/chrome/browser/payments/site_per_process... Done. https://codereview.chromium.org/2870153002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/payments/payment_request_can_make_payment_metrics_browsertest.cc:422: // Simulate that the user navigates away from the Payment Request. On 2017/05/10 17:04:20, Mathieu wrote: > by closing the tab Done. https://codereview.chromium.org/2870153002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/payments/payment_request_can_make_payment_metrics_browsertest.cc:444: // Simulate that the user navigates away from the Payment Request. On 2017/05/10 17:04:20, Mathieu wrote: > ...by reloading Done.
lgtm https://codereview.chromium.org/2870153002/diff/160001/components/payments/co... File components/payments/content/payment_request.cc (right): https://codereview.chromium.org/2870153002/diff/160001/components/payments/co... components/payments/content/payment_request.cc:153: if (!has_recorded_abort_reason_) { as discussed let's move this to a private method
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: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
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 Link to the patchset: https://codereview.chromium.org/2870153002/#ps180001 (title: "Addressed comments")
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: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by sebsg@chromium.org
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: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_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: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
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 Link to the patchset: https://codereview.chromium.org/2870153002/#ps220001 (title: "Fixed Windows error")
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: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by sebsg@chromium.org
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": 220001, "attempt_start_ts": 1495042736773810,
"parent_rev": "28fe10df2390b039fe2209503421542963099dd6", "commit_rev":
"2c8558a602d83dbb979c30e6a18d271502b951fa"}
Message was sent while issue was closed.
Description was changed from ========== [Payments] Record navigations that close the Payment Request as aborts. BUG=717757 ========== to ========== [Payments] Record navigations that close the Payment Request as aborts. BUG=717757 Review-Url: https://codereview.chromium.org/2870153002 Cr-Commit-Position: refs/heads/master@{#472514} Committed: https://chromium.googlesource.com/chromium/src/+/2c8558a602d83dbb979c30e6a18d... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:220001) as https://chromium.googlesource.com/chromium/src/+/2c8558a602d83dbb979c30e6a18d... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
