Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(5)

Issue 2827673003: Add timeout for payments shipping address editor regions data load. (Closed)

Created:
3 years, 8 months ago by MAD
Modified:
3 years, 7 months ago
Reviewers:
Mathieu, sebsg, anthonyvd
CC:
chromium-reviews, tfarina
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Add timeout for payments shipping address editor regions data load. BUG=None Review-Url: https://codereview.chromium.org/2827673003 Cr-Commit-Position: refs/heads/master@{#466742} Committed: https://chromium.googlesource.com/chromium/src/+/763ed2b4f7a7bca3230afa6dbb23f2e74a362f79

Patch Set 1 #

Patch Set 2 : Handle timeout leaks #

Total comments: 5

Patch Set 3 : minor fixes #

Patch Set 4 : With a mocked timer #

Patch Set 5 : Working version #

Patch Set 6 : Rebase + merge conflict and other fixes #

Patch Set 7 : Self CR #

Patch Set 8 : Rebase again #

Patch Set 9 : More rebase tweaks #

Total comments: 4

Patch Set 10 : Monday morning rebase #

Patch Set 11 : Fixed comments #

Total comments: 3

Patch Set 12 : Moved timeout timer into RegionDataLoader #

Patch Set 13 : Fixed compile error in components_unittests #

Unified diffs Side-by-side diffs Delta from patch set Stats (+503 lines, -405 lines) Patch
M chrome/browser/payments/chrome_payment_request_delegate.h View 1 2 3 4 5 6 7 1 chunk +1 line, -4 lines 0 comments Download
M chrome/browser/payments/chrome_payment_request_delegate.cc View 1 2 3 4 5 6 7 8 9 10 11 3 chunks +104 lines, -10 lines 0 comments Download
M chrome/browser/ui/views/payments/payment_request_browsertest_base.h View 1 2 3 4 1 chunk +2 lines, -4 lines 0 comments Download
M chrome/browser/ui/views/payments/shipping_address_editor_view_controller.h View 1 2 3 4 5 6 7 8 9 10 11 3 chunks +7 lines, -2 lines 0 comments Download
M chrome/browser/ui/views/payments/shipping_address_editor_view_controller.cc View 1 2 3 4 5 6 7 8 9 10 11 6 chunks +29 lines, -29 lines 0 comments Download
M chrome/browser/ui/views/payments/shipping_address_editor_view_controller_browsertest.cc View 1 2 3 4 5 6 7 8 9 10 11 10 chunks +139 lines, -125 lines 0 comments Download
M chrome/browser/ui/views/payments/test_chrome_payment_request_delegate.h View 1 2 3 4 5 6 7 2 chunks +4 lines, -16 lines 0 comments Download
M chrome/browser/ui/views/payments/test_chrome_payment_request_delegate.cc View 1 2 3 4 5 6 7 2 chunks +6 lines, -14 lines 0 comments Download
M components/autofill/core/browser/BUILD.gn View 1 2 3 4 5 6 7 8 9 2 chunks +3 lines, -0 lines 0 comments Download
M components/autofill/core/browser/region_combobox_model.h View 1 2 3 4 5 6 7 8 9 10 11 2 chunks +43 lines, -35 lines 0 comments Download
M components/autofill/core/browser/region_combobox_model.cc View 1 2 3 4 5 6 7 8 9 10 11 2 chunks +33 lines, -47 lines 0 comments Download
M components/autofill/core/browser/region_combobox_model_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +27 lines, -77 lines 0 comments Download
A components/autofill/core/browser/test_region_data_loader.h View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +47 lines, -0 lines 0 comments Download
A components/autofill/core/browser/test_region_data_loader.cc View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +48 lines, -0 lines 0 comments Download
M components/payments/content/payment_request_state.h View 1 2 3 4 2 chunks +2 lines, -9 lines 0 comments Download
M components/payments/content/payment_request_state.cc View 1 2 3 4 5 6 7 1 chunk +2 lines, -8 lines 0 comments Download
M components/payments/content/payment_response_helper_unittest.cc View 1 2 3 4 5 6 7 1 chunk +2 lines, -10 lines 0 comments Download
M components/payments/core/payment_request_delegate.h View 1 2 3 4 5 6 7 2 chunks +4 lines, -15 lines 0 comments Download

Messages

Total messages: 61 (36 generated)
MAD
One more step forward... :-) BYE MAD
3 years, 8 months ago (2017-04-18 18:16:01 UTC) #4
Mathieu
I'm not an owner of this code (+ leaving tomorrow), handing over to sebsg@ who ...
3 years, 8 months ago (2017-04-18 20:49:43 UTC) #8
MAD
I just uploaded a new version that handle the timeout leaks, but I still need ...
3 years, 8 months ago (2017-04-19 13:22:09 UTC) #9
MAD
https://codereview.chromium.org/2827673003/diff/40001/components/autofill/core/browser/region_combobox_model.cc File components/autofill/core/browser/region_combobox_model.cc (right): https://codereview.chromium.org/2827673003/diff/40001/components/autofill/core/browser/region_combobox_model.cc#newcode71 components/autofill/core/browser/region_combobox_model.cc:71: LOG(ERROR) << "delete this Async;"; I'll remove all those ...
3 years, 8 months ago (2017-04-19 13:29:50 UTC) #11
anthonyvd
Thanks, couple comments. I agree fixing it this way is a little weird but I'm ...
3 years, 8 months ago (2017-04-19 13:53:11 UTC) #12
MAD
Thanks... I keep testing and tweaking until convinced otherwise. I'll let you know when it's ...
3 years, 8 months ago (2017-04-19 14:23:42 UTC) #13
MAD
OK, this should now be ready to review... It ends up being a much bigger ...
3 years, 8 months ago (2017-04-21 21:06:52 UTC) #22
sebsg
A small question :) https://codereview.chromium.org/2827673003/diff/200001/chrome/browser/ui/views/payments/shipping_address_editor_view_controller.cc File chrome/browser/ui/views/payments/shipping_address_editor_view_controller.cc (right): https://codereview.chromium.org/2827673003/diff/200001/chrome/browser/ui/views/payments/shipping_address_editor_view_controller.cc#newcode382 chrome/browser/ui/views/payments/shipping_address_editor_view_controller.cc:382: OnDataChanged(/*synchronous=*/true); I'm a bit confused ...
3 years, 7 months ago (2017-04-24 13:33:44 UTC) #33
MAD
Merci! Updated comments... BYE MAD https://codereview.chromium.org/2827673003/diff/200001/chrome/browser/ui/views/payments/shipping_address_editor_view_controller.cc File chrome/browser/ui/views/payments/shipping_address_editor_view_controller.cc (right): https://codereview.chromium.org/2827673003/diff/200001/chrome/browser/ui/views/payments/shipping_address_editor_view_controller.cc#newcode382 chrome/browser/ui/views/payments/shipping_address_editor_view_controller.cc:382: OnDataChanged(/*synchronous=*/true); On 2017/04/24 13:33:44, ...
3 years, 7 months ago (2017-04-24 14:26:47 UTC) #34
sebsg
sounds good, lgtm! Thanks for looking into this
3 years, 7 months ago (2017-04-24 14:32:09 UTC) #35
MAD
Thanks Seb! Mathieu, or Anthony for OWNER approval? BYE MAD
3 years, 7 months ago (2017-04-24 15:06:11 UTC) #37
anthonyvd
Looking good! I reviewed all the views files and have a comment about the timer. ...
3 years, 7 months ago (2017-04-24 15:16:02 UTC) #38
anthonyvd
Looking good! I reviewed all the views files and have a comment about the timer.
3 years, 7 months ago (2017-04-24 15:16:03 UTC) #39
MAD
Thanks for the comment. Answered below... BYE MAD https://codereview.chromium.org/2827673003/diff/240001/chrome/browser/ui/views/payments/payment_request_dialog_view.h File chrome/browser/ui/views/payments/payment_request_dialog_view.h (right): https://codereview.chromium.org/2827673003/diff/240001/chrome/browser/ui/views/payments/payment_request_dialog_view.h#newcode159 chrome/browser/ui/views/payments/payment_request_dialog_view.h:159: base::Timer* ...
3 years, 7 months ago (2017-04-24 15:20:14 UTC) #40
MAD
On 2017/04/24 15:20:14, MAD wrote: > Thanks for the comment. > > Answered below... > ...
3 years, 7 months ago (2017-04-24 15:24:55 UTC) #41
MAD
On 2017/04/24 15:20:14, MAD wrote: > Thanks for the comment. > > Answered below... > ...
3 years, 7 months ago (2017-04-24 15:24:58 UTC) #42
anthonyvd
https://codereview.chromium.org/2827673003/diff/240001/chrome/browser/ui/views/payments/payment_request_dialog_view.h File chrome/browser/ui/views/payments/payment_request_dialog_view.h (right): https://codereview.chromium.org/2827673003/diff/240001/chrome/browser/ui/views/payments/payment_request_dialog_view.h#newcode159 chrome/browser/ui/views/payments/payment_request_dialog_view.h:159: base::Timer* test_timeout_timer_; On 2017/04/24 at 15:20:14, MAD wrote: > ...
3 years, 7 months ago (2017-04-24 15:39:58 UTC) #43
MAD
Thanks for the push, it got me to find a much better solution to this ...
3 years, 7 months ago (2017-04-24 18:33:32 UTC) #44
anthonyvd
Ohh, awesome! chrome/browser/ui/views/payments/* lgtm, thanks :)
3 years, 7 months ago (2017-04-24 18:38:46 UTC) #45
sebsg
sweet :) lgtm (look gooder to me)
3 years, 7 months ago (2017-04-24 18:41:51 UTC) #46
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2827673003/260001
3 years, 7 months ago (2017-04-24 18:48:43 UTC) #48
commit-bot: I haz the power
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_presubmit/builds/418847) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, ...
3 years, 7 months ago (2017-04-24 18:59:44 UTC) #50
Mathieu
Thanks MAD for navigating the trickiness! lgtm
3 years, 7 months ago (2017-04-24 19:02:33 UTC) #51
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2827673003/280001
3 years, 7 months ago (2017-04-24 20:16:50 UTC) #58
commit-bot: I haz the power
3 years, 7 months ago (2017-04-24 20:29:03 UTC) #61
Message was sent while issue was closed.
Committed patchset #13 (id:280001) as
https://chromium.googlesource.com/chromium/src/+/763ed2b4f7a7bca3230afa6dbb23...

Powered by Google App Engine
This is Rietveld 408576698