[Payments] Build payment app loading view UI [2/N] [chromium/src : main]
Stephen McGruer (Gerrit)
Stephen McGruer added 11 comments![]( "Open in Gerrit")
"payment_app_loading_view_interactive_uitest.cc",I think this should be part of a interactive_ui_tests test target, not a browser_tests one, right?
void AddedToWidget() override;Could we just override OnThemeChanged instead? https://source.chromium.org/chromium/chromium/src/+/main:ui/views/view.h;l=2038;drc=54dd37fb9545f90b93fed90434ebf30cadacf65e
It is apparently called when the widget is attached (worth testing to confirm), and should also be called if the dark/light mode changes.
#include "third_party/skia/include/core/SkColor.h"Doesn't seem to be referenced, unused include?
#include "base/timer/timer.h"You don't seem to set any timers, unused include?
#include <optional>You don't seem to use optional in the header, unused include?
#include "base/strings/utf_string_conversions.h"Is this used? I don't see an obvious usage, maybe unused include?
#include "chrome/browser/ui/views/payments/payment_request_views_util.h"Is this used? I don't see an obvious usage, maybe unused include?
#include "ui/color/color_provider.h"Is this used? Seems unused.
void PaymentAppLoadingView::AddedToWidget() {Nit; call your parent class' AddedToWidget() first.
widget_ = nullptr;According to jetski, you need to either Close() the widget here, or call SetOwnershipOfNewWidget on the delegate when you create it with CLIENT_OWNS_WIDGET. Otherwise (apparently) the modal dialog will be left open at the end of the test which isn't very clean.
(Note; purely AI, I don't have the knowledge to know if its right!)
loading_view_overlay_ = AddChildView(std::make_unique<PaymentAppLoadingView>(I missed this before, but do you need to focus on the newly added view? Otherwise isn't focus still on whatever is underneath it?
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 11 comments![]( "Open in Gerrit")
"payment_app_loading_view_interactive_uitest.cc",I think this should be part of a interactive_ui_tests test target, not a browser_tests one, right?
Added a new source set for interactive_ui_tests, let me know your thoughts.
Could we just override OnThemeChanged instead? https://source.chromium.org/chromium/chromium/src/+/main:ui/views/view.h;l=2038;drc=54dd37fb9545f90b93fed90434ebf30cadacf65e
It is apparently called when the widget is attached (worth testing to confirm), and should also be called if the dark/light mode changes.
Yea, that worked, However, payment handler dialog itself doesn't work with theme change for now, b/535575392 for tracking the fix.
Doesn't seem to be referenced, unused include?
Done
You don't seem to set any timers, unused include?
Done
You don't seem to use optional in the header, unused include?
Done
Is this used? I don't see an obvious usage, maybe unused include?
Done
#include "chrome/browser/ui/views/payments/payment_request_views_util.h"Is this used? I don't see an obvious usage, maybe unused include?
Done
Is this used? Seems unused.
Done
Nit; call your parent class' AddedToWidget() first.
Switched to OnThemeChanged.
According to jetski, you need to either Close() the widget here, or call SetOwnershipOfNewWidget on the delegate when you create it with CLIENT_OWNS_WIDGET. Otherwise (apparently) the modal dialog will be left open at the end of the test which isn't very clean.
(Note; purely AI, I don't have the knowledge to know if its right!)
I've updated the test to use CLIENT_OWNS_WIDGET and cleaned up TearDownOnMainThread(). Also cleaned up unused pointer.
Here is my understanding:
Previous approach is a workaround to just release widget pointer in TearDownOnMainThread, but the widget itself is still unmanaged(not deleted).
Now after set ownership to CLIENT_OWNS_WIDGET, widget pointer takes full ownership of the widget, calling pointer reset can clean and destroy the modal dialog.
loading_view_overlay_ = AddChildView(std::make_unique<PaymentAppLoadingView>(I missed this before, but do you need to focus on the newly added view? Otherwise isn't focus still on whatever is underneath it?
Yea, that's on my radar, so focus is needed and the hidden of view_stack as well to make sure the focus and keyboard navigation work properly. This will be in the following cls. I have added the note in the description.
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. |
Stephen McGruer (Gerrit)
Stephen McGruer voted and added 1 comment![]( "Open in Gerrit")
Votes added by Stephen McGruer
| Code-Review | +1 |
1 comment
"payment_app_loading_view_interactive_uitest.cc",Xuehui ChenI think this should be part of a interactive_ui_tests test target, not a browser_tests one, right?
Added a new source set for interactive_ui_tests, let me know your thoughts.
- Xuehui Chen
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Darwin Yang (Gerrit)
Darwin Yang voted and added 1 comment![]( "Open in Gerrit")
Votes added by Darwin Yang
| Code-Review | +1 |
1 comment
constexpr int kLoadingMessageVerticalInset = 120;
constexpr int kLoadingMessageHorizontalInset = 24;Is it possible to use standard distance values here?
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 1 comment![]( "Open in Gerrit")
constexpr int kLoadingMessageVerticalInset = 120;
constexpr int kLoadingMessageHorizontalInset = 24;Is it possible to use standard distance values here?
- Darwin Yang
- Stephen McGruer
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Darwin Yang (Gerrit)
Darwin Yang voted Code-Review+1![]( "Open in Gerrit")
- Stephen McGruer
- Xuehui Chen
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 voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Xuehui Chen
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+2![]( "Open in Gerrit")
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 added 1 comment![]( "Open in Gerrit")
widget_ = nullptr;Xuehui ChenAccording to jetski, you need to either Close() the widget here, or call SetOwnershipOfNewWidget on the delegate when you create it with CLIENT_OWNS_WIDGET. Otherwise (apparently) the modal dialog will be left open at the end of the test which isn't very clean.
(Note; purely AI, I don't have the knowledge to know if its right!)
I've updated the test to use CLIENT_OWNS_WIDGET and cleaned up TearDownOnMainThread(). Also cleaned up unused pointer.
Here is my understanding:
Previous approach is a workaround to just release widget pointer in TearDownOnMainThread, but the widget itself is still unmanaged(not deleted).Now after set ownership to CLIENT_OWNS_WIDGET, widget pointer takes full ownership of the widget, calling pointer reset can clean and destroy the modal dialog.
The asan test captured another leak from dialog delegate. Apparently the widget doesn't own the dialog delegate so it's not freed during test tear down, I have patched a change to own dialog delegate pointer and reset it during tear down, waiting for the try job to finish.
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")
widget_ = nullptr;Xuehui ChenAccording to jetski, you need to either Close() the widget here, or call SetOwnershipOfNewWidget on the delegate when you create it with CLIENT_OWNS_WIDGET. Otherwise (apparently) the modal dialog will be left open at the end of the test which isn't very clean.
(Note; purely AI, I don't have the knowledge to know if its right!)
Xuehui ChenI've updated the test to use CLIENT_OWNS_WIDGET and cleaned up TearDownOnMainThread(). Also cleaned up unused pointer.
Here is my understanding:
Previous approach is a workaround to just release widget pointer in TearDownOnMainThread, but the widget itself is still unmanaged(not deleted).Now after set ownership to CLIENT_OWNS_WIDGET, widget pointer takes full ownership of the widget, calling pointer reset can clean and destroy the modal dialog.
The asan test captured another leak from dialog delegate. Apparently the widget doesn't own the dialog delegate so it's not freed during test tear down, I have patched a change to own dialog delegate pointer and reset it during tear down, waiting for the try job to finish.
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+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
9 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: chrome/browser/ui/views/payments/payment_app_loading_view_interactive_uitest.cc
Insertions: 8, Deletions: 6.
@@ -42,23 +42,23 @@
views::DialogClientView::kTopViewId) {
return Steps(
Do([this]() {
- auto delegate = std::make_unique<views::DialogDelegate>();
- delegate->SetOwnershipOfNewWidget(
+ delegate_ = std::make_unique<views::DialogDelegate>();
+ delegate_->SetOwnershipOfNewWidget(
views::Widget::InitParams::CLIENT_OWNS_WIDGET);
- delegate->SetModalType(ui::mojom::ModalType::kChild);
- delegate->SetButtons(
+ delegate_->SetModalType(ui::mojom::ModalType::kChild);
+ delegate_->SetButtons(
static_cast<int>(ui::mojom::DialogButton::kNone));
auto loading_view = std::make_unique<PaymentAppLoadingView>(
&icon_, GURL("https://app.com"), GURL("https://merchant.com"),
close_callback_.Get());
- loading_view_ = delegate->SetContentsView(std::move(loading_view));
+ loading_view_ = delegate_->SetContentsView(std::move(loading_view));
tabs::TabInterface* tab_interface =
tabs::TabInterface::GetFromContents(web_contents());
widget_ = tab_interface->GetTabFeatures()
->tab_dialog_manager()
->CreateAndShowDialog(
- delegate.release(),
+ delegate_.get(),
std::make_unique<tabs::TabDialogManager::Params>());
}),
InAnyContext(WaitForShow(element_specifier)));
@@ -67,6 +67,7 @@
void TearDownOnMainThread() override {
loading_view_ = nullptr;
widget_.reset();
+ delegate_.reset();
InteractiveBrowserTest::TearDownOnMainThread();
}
@@ -76,6 +77,7 @@
SkBitmap icon_;
std::unique_ptr<views::Widget> widget_;
+ std::unique_ptr<views::DialogDelegate> delegate_;
raw_ptr<PaymentAppLoadingView> loading_view_ = nullptr;
base::MockRepeatingCallback<void(const ui::Event&)> close_callback_;
};
```
Change information
[payments] Build payment app loading view UI [2/N]
Implements the visual components, localized strings, and UI styling for
PaymentAppLoadingView, which is shown while a web payment application is
loading in Web Payments.
Key changes:
- Build UI elements including the header, origin labels, progress bar,
close button, and loading text.
- Configure UI styling according to UX mocks.
- Set header color after the widget is added to align color handling
with PaymentHandlerWebFlowView default behavior.
- Set rounded corner radius on PaymentRequestDialogView loading overlay.
- Add interactive UI tests to cover dialog display and close button
interaction.
Mock: https://screenshot.googleplex.com/9eTUJbXuhqgBFRp
Implementation: https://screenshot.googleplex.com/A7z6Mh2hhVBGWBp
The following cls will implement the delayed loading message, resize
window and view focus transition.
- M chrome/browser/ui/views/payments/BUILD.gn
- M chrome/browser/ui/views/payments/payment_app_loading_view.cc
- M chrome/browser/ui/views/payments/payment_app_loading_view.h
- A chrome/browser/ui/views/payments/payment_app_loading_view_interactive_uitest.cc
- M chrome/browser/ui/views/payments/payment_request_dialog_view.cc
- M chrome/test/BUILD.gn
- M components/payments/core/sizes.h
- M components/payments_strings.grdp
- A components/payments_strings_grdp/IDS_PAYMENT_APP_LOADING_MESSAGE.png.sha1
Code-Review: +1 by Darwin Yang, +1 by Stephen McGruer
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |