[payments] Add payment app loading view [chromium/src : main]
Xuehui Chen (Gerrit)
Xuehui Chen voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Xuehui Chen (Gerrit)
Xuehui Chen voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Chromium Payments Reviews
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
gwsq (Gerrit)
Message from gwsq![]( "Open in Gerrit")
Reviewer source(s):
smcg...@chromium.org is from context(googleclient/chrome/chromium_gwsq/chrome/browser/payments/reviews.gwsq)
Related details
- Stephen McGruer
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Stephen McGruer (Gerrit)
Stephen McGruer added 10 comments![]( "Open in Gerrit")
followup cl will implement thatHyper-nit: missing '.' at end of sentence 😄
Bug: b:522844222Nit; where possible we should link CLs to public crbugs, rather than internal bugs. Is there a wider tracking bug that you could also link this change against?
// Copyright 2026 The Chromium AuthorsConsider adding a render test, like chrome/browser/ui/views/autofill/payments/bnpl_tos_view_desktop_interactive_uitest.cc
constexpr int kLoadingMessageHorizontalInsets = 24;Nit: these should be singular, not plural (no trailing 's')
: state_(state) {Do we use the state anywhere outside of the constructor? If not, we don't need to store it, right?
const SkBitmap* icon_bitmap = state_->selected_app()->icon_bitmap();Instead of passing in the State, would it be cleaner to just pass in the selected app? Or even the three data points you need (icon, id - but named as app_origin -, and top origin)?
SetHeaderColors(header_view, header_views.origin_label.get(), progress_bar,Does this call work? PaymentAppLoadingView is not attached to anything yet, so will it work to try to set theme appropriate colours on the header?
if (is_visible && GetVisible()) {Is the `GetVisible()` check here redundant? What purpose does it serve?
text_update_timer_.Stop();When does this happen, if the payment app loading view is closed/removed before the timer fires? (Either by the user or when the HTML loads?)
Related details
- Xuehui Chen
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Xuehui Chen (Gerrit)
Xuehui Chen added 3 comments![]( "Open in Gerrit")
followup cl will implement thatHyper-nit: missing '.' at end of sentence 😄
Done
if (is_visible && GetVisible()) {Is the `GetVisible()` check here redundant? What purpose does it serve?
Something I learned today, visibility change signal can come from either ancestor or the loading view itself [ui/views/view.cc](https://source.chromium.org/chromium/chromium/src/+/main:ui/views/view.cc;l=3502-3506;drc=6b0e66c1b6dafa6967898773ff9ccbc3dc2d0445). This is just a safeguard, we don't start the timer for the situation that the loading view is hidden, but the parent view has a visibility change and broadcast to all the children view. Will add a comment to the code explaining this.
text_update_timer_.Stop();When does this happen, if the payment app loading view is closed/removed before the timer fires? (Either by the user or when the HTML loads?)
User close dialog will go through a tear down flow, the timer will be destructed with the view destruction.
Webcontent finish loading within 500ms will stop this timer. I expect this happens 99% of the time.
Related details
- Stephen McGruer
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Xuehui Chen (Gerrit)
Xuehui Chen added 1 comment![]( "Open in Gerrit")
SetHeaderColors(header_view, header_views.origin_label.get(), progress_bar,Does this call work? PaymentAppLoadingView is not attached to anything yet, so will it work to try to set theme appropriate colours on the header?
It's not setting theme color, it uses a default color for the background. But good call on this, I didn't explore what exact color is being used here. The screenshot in the Test tag looks ok, but I will check it.
Related details
- Stephen McGruer
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Xuehui Chen (Gerrit)
Xuehui Chen added 7 comments![]( "Open in Gerrit")
Bug: b:522844222Nit; where possible we should link CLs to public crbugs, rather than internal bugs. Is there a wider tracking bug that you could also link this change against?
Done
// Copyright 2026 The Chromium AuthorsConsider adding a render test, like chrome/browser/ui/views/autofill/payments/bnpl_tos_view_desktop_interactive_uitest.cc
I added a basic view unit test for test coverage, will add additional browser/interactive tests after this is attached to the payment request dialog, wdyt?
constexpr int kLoadingMessageHorizontalInsets = 24;Nit: these should be singular, not plural (no trailing 's')
Done
: state_(state) {Do we use the state anywhere outside of the constructor? If not, we don't need to store it, right?
Removed in favor of passing icon and origins directly to the header constructor.
DCHECK(state_);removed.
const SkBitmap* icon_bitmap = state_->selected_app()->icon_bitmap();Instead of passing in the State, would it be cleaner to just pass in the selected app? Or even the three data points you need (icon, id - but named as app_origin -, and top origin)?
Good call, updated.
SetHeaderColors(header_view, header_views.origin_label.get(), progress_bar,Xuehui ChenDoes this call work? PaymentAppLoadingView is not attached to anything yet, so will it work to try to set theme appropriate colours on the header?
It's not setting theme color, it uses a default color for the background. But good call on this, I didn't explore what exact color is being used here. The screenshot in the Test tag looks ok, but I will check it.
the header view will take kColorDialogBackground by default, see [chrome/browser/ui/views/payments/payment_handler_header_view_util.cc](https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/ui/views/payments/payment_handler_header_view_util.cc;l=244-275) as no theme is provided, other header elements will take headers color as background color input.
Related details
- Stephen McGruer
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |