Introduce TopChromeWebUIDialogDelegate [chromium/src : main]
Dana Fried (Gerrit)
Dana Fried added 1 comment![]( "Open in Gerrit")
Please get tests working and resolve this comment. Thanks!
Related details
- Foromo Daniel Soromou
- Tom Lukaszewicz
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. |
Tom Lukaszewicz (Gerrit)
Tom Lukaszewicz added 9 comments![]( "Open in Gerrit")
WebDialogSpec spec_;nit: Should this be `const WebDialogSpec`?
// Creates and shows a dialog with the given `contents_wrapper` and `spec`.
// Returns the created widget. The caller does not own the returned widget.
static views::Widget* Show(
gfx::NativeWindow parent,
std::unique_ptr<WebUIContentsWrapper> contents_wrapper,
const WebDialogSpec& spec);nit: Could we move this below constructor / destructor (see [declaration order](https://google.github.io/styleguide/cppguide.html#Declaration_Order))
class TopChromeWebUIDialogDelegate : public views::DialogDelegate,nit: Similar to above - we should probably drop the top-chrome prefix, or figure out a different prefix (maybe just `ChromeWebUIDialog` is good enough)
// If true, applies corner radius clipping to the WebContents to match the
// dialog's rounded corners. This prevents WebUI content from bleeding
// through rounded corners.
bool apply_corner_radius_clipping = true;QQ - is this ever not true?
namespace top_chrome_webui_dialog {nit: I think we can just drop `top_chrome` prefix since since it doesn't quite make sense for dialogs to be top-chrome
std::move(delegate), parent);optional: It may be easier to simply inline the above (current approach is file also)
gfx::NativeWindow native_window = GetWidget()->GetNativeWindow();Does it work to get the work area directly from the widget (i.e. `Widget::GetWorkAreaBoundsInScreen()` vs getting it via the NativeWindow / Display?
// Leave a small margin for aesthetics.nit: Could we put the magic constants below into a variable just above? i.e.
```
constexpr int kAestheticMargin = 64;
const int max_height = work_area.height() - kAestheticMargin;
const int max_width = work_area.width() - kAestheticMargin;
```
Also is there a UX reason for this or is this an engineering decision?
// Resize the widget to fit the new preferred size of the contents.
// The non-client view includes the window frame, so this ensures the
// entire dialog is sized correctly.
GetWidget()->SetSize(GetWidget()->non_client_view()->GetPreferredSize());QQ - Does the widget not automatically compute the preferred size of its View hierarchy in this case?
Related details
- Foromo Daniel Soromou
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. |
Foromo Daniel Soromou (Gerrit)
Foromo Daniel Soromou added 10 comments![]( "Open in Gerrit")
Please get tests working and resolve this comment. Thanks!
Done
nit: Should this be `const WebDialogSpec`?
Done
// Creates and shows a dialog with the given `contents_wrapper` and `spec`.
// Returns the created widget. The caller does not own the returned widget.
static views::Widget* Show(
gfx::NativeWindow parent,
std::unique_ptr<WebUIContentsWrapper> contents_wrapper,
const WebDialogSpec& spec);nit: Could we move this below constructor / destructor (see [declaration order](https://google.github.io/styleguide/cppguide.html#Declaration_Order))
Done
class TopChromeWebUIDialogDelegate : public views::DialogDelegate,nit: Similar to above - we should probably drop the top-chrome prefix, or figure out a different prefix (maybe just `ChromeWebUIDialog` is good enough)
Done
// If true, applies corner radius clipping to the WebContents to match the
// dialog's rounded corners. This prevents WebUI content from bleeding
// through rounded corners.
bool apply_corner_radius_clipping = true;QQ - is this ever not true?
oh yes, all our dialog have the rounded corners, what will make the differents may be the raduis, So I have updated it to allow callers to provide the radius or it will defaults to the Chrome system dialog raduis.
nit: I think we can just drop `top_chrome` prefix since since it doesn't quite make sense for dialogs to be top-chrome
Done
optional: It may be easier to simply inline the above (current approach is file also)
Done
gfx::NativeWindow native_window = GetWidget()->GetNativeWindow();Does it work to get the work area directly from the widget (i.e. `Widget::GetWorkAreaBoundsInScreen()` vs getting it via the NativeWindow / Display?
Yes It works. Updated
nit: Could we put the magic constants below into a variable just above? i.e.
```
constexpr int kAestheticMargin = 64;
const int max_height = work_area.height() - kAestheticMargin;
const int max_width = work_area.width() - kAestheticMargin;
```Also is there a UX reason for this or is this an engineering decision?
it was just an engineering decision. But I removed it since in general, dialogs are not expected to be big to occupy the whole screen.
// Resize the widget to fit the new preferred size of the contents.
// The non-client view includes the window frame, so this ensures the
// entire dialog is sized correctly.
GetWidget()->SetSize(GetWidget()->non_client_view()->GetPreferredSize());QQ - Does the widget not automatically compute the preferred size of its View hierarchy in this case?
It does compute the new preferred size internally, but setting `SetPreferredSize()` on a View only invalidates the layout hierarchy—it doesn't automatically resize the underlying physical OS window (views::Widget) on the screen. We have to explicitly call SetSize(...) on the Widget to force the native window to actually expand or shrink.
Related details
- Dana Fried
- Thomas Lukaszewicz
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. |
Thomas Lukaszewicz (Gerrit)
Thomas Lukaszewicz added 7 comments![]( "Open in Gerrit")
* **`apply_corner_radius_clipping`**: Automatically clips the WebUI content to match the rounded corners of the dialog frame. This prevents content from bleeding through rounded corners.May need to update this for corner radius changes.
top_chrome_webui_dialog::WebDialogSpec spec;nit: namespace name
views::Widget* widget = top_chrome_webui_dialog::TopChromeWebUIDialogDelegate::Show(nit: Dialog name (here and elsewhere)
// If true, the `max_size` will be clamped to the display's work area to
// ensure the dialog fits on screen.
bool clamp_to_work_area = true;QQ will this ever be false?
web_view_->SetPreferredSize(spec_.min_size);We can probably do `web_view_->EnableSizingFromWebContents(spec_.min_size, spec_.max_size);` here instead of calling `EnableAutoResize` directly on the rwhv below.
This also propagates size constraints across crashes and navigations (which the current approach won't handle)
gfx::Size bounded_size = new_size;Do we need to check `clamp_to_work_area` here?
const int max_height = work_area.height();Do we need to also consider the sizes in `spec_` when clamping size here? Or will `ResizeDueToAutoResize` only return sizes within the clamped constrains set in the constructor (in either case we should document the expected behavior in a comment somewhere)
Related details
- Dana Fried
- Foromo Daniel Soromou
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. |
Foromo Daniel Soromou (Gerrit)
Foromo Daniel Soromou added 7 comments![]( "Open in Gerrit")
* **`apply_corner_radius_clipping`**: Automatically clips the WebUI content to match the rounded corners of the dialog frame. This prevents content from bleeding through rounded corners.May need to update this for corner radius changes.
Done
top_chrome_webui_dialog::WebDialogSpec spec;Foromo Daniel Soromounit: namespace name
Done
views::Widget* widget = top_chrome_webui_dialog::TopChromeWebUIDialogDelegate::Show(nit: Dialog name (here and elsewhere)
Done
// If true, the `max_size` will be clamped to the display's work area to
// ensure the dialog fits on screen.
bool clamp_to_work_area = true;QQ will this ever be false?
No, removed, it's no longer used.
We can probably do `web_view_->EnableSizingFromWebContents(spec_.min_size, spec_.max_size);` here instead of calling `EnableAutoResize` directly on the rwhv below.
This also propagates size constraints across crashes and navigations (which the current approach won't handle)
Oh Great. Updated.
Do we need to check `clamp_to_work_area` here?
`clamp_to_work_area` has been removed. By default, it will be assumed that the dialog will be limited by the work area.
Do we need to also consider the sizes in `spec_` when clamping size here? Or will `ResizeDueToAutoResize` only return sizes within the clamped constrains set in the constructor (in either case we should document the expected behavior in a comment somewhere)
- Dana Fried
- Thomas Lukaszewicz
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. |
Thomas Lukaszewicz (Gerrit)
Thomas Lukaszewicz added 3 comments![]( "Open in Gerrit")
Thanks! Overall looks good % comments and failing test cases
if (bounded_size.height() > max_height) {I believe `bounded_size` represents the size of the web content but not necessarily the window (which includes things like borders and shadows).
We should calculate the max content size as the work_area size insetted by the frame size, which we can get using
```
gfx::Size frame_size =
GetWidget()->non_client_view()
->GetWindowBoundsForClientBounds(gfx::Rect()).size();
```
GetWidget()->SetSize(GetWidget()->non_client_view()->GetPreferredSize());nit: We should probably make this `CenterWindow` so that the dialog remains centered as its size changes.
Related details
- Dana Fried
- Foromo Daniel Soromou
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. |
Foromo Daniel Soromou (Gerrit)
Foromo Daniel Soromou added 3 comments![]( "Open in Gerrit")
I believe `bounded_size` represents the size of the web content but not necessarily the window (which includes things like borders and shadows).
We should calculate the max content size as the work_area size insetted by the frame size, which we can get using
```
gfx::Size frame_size =
GetWidget()->non_client_view()
->GetWindowBoundsForClientBounds(gfx::Rect()).size();
```
Done
GetWidget()->SetSize(GetWidget()->non_client_view()->GetPreferredSize());nit: We should probably make this `CenterWindow` so that the dialog remains centered as its size changes.
Done
// Resize the widget to fit the new preferred size of the contents.
// The non-client view includes the window frame, so this ensures the
// entire dialog is sized correctly.
GetWidget()->SetSize(GetWidget()->non_client_view()->GetPreferredSize());Foromo Daniel SoromouQQ - Does the widget not automatically compute the preferred size of its View hierarchy in this case?
It does compute the new preferred size internally, but setting `SetPreferredSize()` on a View only invalidates the layout hierarchy—it doesn't automatically resize the underlying physical OS window (views::Widget) on the screen. We have to explicitly call SetSize(...) on the Widget to force the native window to actually expand or shrink.
- Dana Fried
- Thomas Lukaszewicz
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dana Fried (Gerrit)
Dana Fried added 5 comments![]( "Open in Gerrit")
static views::Widget* Show(What is the widget ownership mode used here?
If it is CLIENT_OWNS_WIDGET (which ideally it should be as that is now standard), this should be a `unique_ptr`.
If the modal system still uses an outdated widget mode and there's no way to override it, then a bare widget pointer is fine.
// A reusable dialog delegate that hosts a TopChrome WebUI page.This does not describe what this actually does, since it only creates browser-modal dialogs. We _rarely_ use browser-modal dialogs.
views::Widget* widget = constrained_window::CreateBrowserModalDialogViews(Are you sure we always want to make the dialog modal? Can we have modality options?
Can these be anchored? Or is this very specific to one use case?
if (GetWidget() && !GetWidget()->IsVisible()) {Note that on some platforms this may be asynchronous with whether Show() has been called. It shouldn't probably cause any issues, just be aware.
web_view_->holder()->SetCornerRadii(This is a case where corising@'s work on side panels with web views might be relevant. At the very least, I would recommend tagging one of the related bugs here, as ideally we should just be able to set rounded corners on the top level layer and not have to burrow into the holder.
Related details
- Foromo Daniel Soromou
- Thomas Lukaszewicz
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. |
Dana Fried (Gerrit)
Dana Fried added 1 comment![]( "Open in Gerrit")
You appear to be leaking memory on exit (or something else that is offending the asan build).
Foromo Daniel Soromou (Gerrit)
Foromo Daniel Soromou added 6 comments![]( "Open in Gerrit")
You appear to be leaking memory on exit (or something else that is offending the asan build).
Done
What is the widget ownership mode used here?
If it is CLIENT_OWNS_WIDGET (which ideally it should be as that is now standard), this should be a `unique_ptr`.
If the modal system still uses an outdated widget mode and there's no way to override it, then a bare widget pointer is fine.
Updated to be a `CLIENT_OWNS_WIDGET`.
// A reusable dialog delegate that hosts a TopChrome WebUI page.This does not describe what this actually does, since it only creates browser-modal dialogs. We _rarely_ use browser-modal dialogs.
Updated to not be modal dialog only.
views::Widget* widget = constrained_window::CreateBrowserModalDialogViews(Are you sure we always want to make the dialog modal? Can we have modality options?
Can these be anchored? Or is this very specific to one use case?
I have update the Spec to take the modality.
Note that on some platforms this may be asynchronous with whether Show() has been called. It shouldn't probably cause any issues, just be aware.
Done
This is a case where corising@'s work on side panels with web views might be relevant. At the very least, I would recommend tagging one of the related bugs here, as ideally we should just be able to set rounded corners on the top level layer and not have to burrow into the holder.
- Dana Fried
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dana Fried (Gerrit)
Dana Fried added 1 comment![]( "Open in Gerrit")
struct WebDialogSpec {This still needs to be updated and the class renamed to indicate that it is always a browser-window-modal dialog.
I'm still not sure why you would want this to be a browser-window-modal dialog, since the uses of those are extremely limited. Can you give me an idea of what you want to use this for?
In general, most dialogs should *not* be modal.
Related details
- Foromo Daniel Soromou
- Thomas Lukaszewicz
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. |
Foromo Daniel Soromou (Gerrit)
Foromo Daniel Soromou added 1 comment![]( "Open in Gerrit")
struct WebDialogSpec {This still needs to be updated and the class renamed to indicate that it is always a browser-window-modal dialog.
I'm still not sure why you would want this to be a browser-window-modal dialog, since the uses of those are extremely limited. Can you give me an idea of what you want to use this for?
In general, most dialogs should *not* be modal.
My understand is that when the modality is `ModalType::kNone`, the dialog will be a non-modal dialog right?
Related details
- Dana Fried
- Thomas Lukaszewicz
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. |
Foromo Daniel Soromou (Gerrit)
Foromo Daniel Soromou added 1 comment![]( "Open in Gerrit")
struct WebDialogSpec {Foromo Daniel SoromouThis still needs to be updated and the class renamed to indicate that it is always a browser-window-modal dialog.
I'm still not sure why you would want this to be a browser-window-modal dialog, since the uses of those are extremely limited. Can you give me an idea of what you want to use this for?
In general, most dialogs should *not* be modal.
My understand is that when the modality is `ModalType::kNone`, the dialog will be a non-modal dialog right?
Currently we condiationally creat the Dialog base on the modality parameter:
```
if (spec.modal_type == ui::mojom::ModalType::kChild) {
CHECK(spec.parent_web_contents)
<< "kChild (tab-modal) dialogs require spec.parent_web_contents";
widget = constrained_window::CreateWebModalDialogViews(
dialog.release(), spec.parent_web_contents);
} else if (spec.modal_type == ui::mojom::ModalType::kWindow ||
spec.modal_type == ui::mojom::ModalType::kSystem) {
widget = constrained_window::CreateBrowserModalDialogViews(dialog.release(),
parent);
} else {
// Allows for non-modal unanchored dialog options.
widget = views::DialogDelegate::CreateDialogWidget(
dialog.release(), /*context=*/parent,
/*parent=*/gfx::NativeView());
}
```
Dana Fried (Gerrit)
Dana Fried voted and added 2 comments![]( "Open in Gerrit")
Votes added by Dana Fried
| Code-Review | +1 |
2 comments
// Returns the created widget. The caller does not own the returned widget.
// By default, the Widget returned is CLIENT_OWNS_WIDGET. There the caller is
// responsible to manage the lifetime.These seem to conflict.
Foromo Daniel SoromouThis still needs to be updated and the class renamed to indicate that it is always a browser-window-modal dialog.
I'm still not sure why you would want this to be a browser-window-modal dialog, since the uses of those are extremely limited. Can you give me an idea of what you want to use this for?
In general, most dialogs should *not* be modal.
Foromo Daniel SoromouMy understand is that when the modality is `ModalType::kNone`, the dialog will be a non-modal dialog right?
Currently we condiationally creat the Dialog base on the modality parameter:
```
if (spec.modal_type == ui::mojom::ModalType::kChild) {
CHECK(spec.parent_web_contents)
<< "kChild (tab-modal) dialogs require spec.parent_web_contents";
widget = constrained_window::CreateWebModalDialogViews(
dialog.release(), spec.parent_web_contents);
} else if (spec.modal_type == ui::mojom::ModalType::kWindow ||
spec.modal_type == ui::mojom::ModalType::kSystem) {
widget = constrained_window::CreateBrowserModalDialogViews(dialog.release(),
parent);
} else {
// Allows for non-modal unanchored dialog options.
widget = views::DialogDelegate::CreateDialogWidget(
dialog.release(), /*context=*/parent,
/*parent=*/gfx::NativeView());
}
```
Oh I missed that detail. Sorry. Yes, then it's fine. Good work!
Related details
- Foromo Daniel Soromou
- Thomas Lukaszewicz
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. |
Foromo Daniel Soromou (Gerrit)
Foromo Daniel Soromou added 1 comment![]( "Open in Gerrit")
// Returns the created widget. The caller does not own the returned widget.
// By default, the Widget returned is CLIENT_OWNS_WIDGET. There the caller is
// responsible to manage the lifetime.- Thomas Lukaszewicz
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Thomas Lukaszewicz (Gerrit)
Thomas Lukaszewicz added 3 comments![]( "Open in Gerrit")
Looks great! Just some last minor comments
sources = [
"chrome_webui_dialog.cc",
"chrome_webui_dialog.h",
]Could we split this into `public` for the header and `sources` for the impl?
Is it also possible to split `deps` below into `public_deps` and `deps` (i.e. transitive public dependencies from the header file and the impl dependencies)
// Optional parent WebContents for displaying as a tab-modal (kChild) dialog.If this is indeed only for tabs can we make this `tabs::TabInterface`?
Also I'm wondering if this should be a `base::WeakPtr<tabs::TabInterface>` - unless we have a high level of confidence that this would never outlive its host tab.
Related details
- Foromo Daniel Soromou
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. |
Thomas Lukaszewicz (Gerrit)
Thomas Lukaszewicz added 1 comment![]( "Open in Gerrit")
static views::UniqueWidgetPtr Show(Really quick question @kere...@chromium.org - do we prefer to use this or just a `std::unique_ptr` for CLIENT_OWNS WIDGET?
Related details
- Foromo Daniel Soromou
- Keren Zhu
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. |
Foromo Daniel Soromou (Gerrit)
Foromo Daniel Soromou added 2 comments![]( "Open in Gerrit")
sources = [
"chrome_webui_dialog.cc",
"chrome_webui_dialog.h",
]Could we split this into `public` for the header and `sources` for the impl?
Is it also possible to split `deps` below into `public_deps` and `deps` (i.e. transitive public dependencies from the header file and the impl dependencies)
Done
// Optional parent WebContents for displaying as a tab-modal (kChild) dialog.If this is indeed only for tabs can we make this `tabs::TabInterface`?
Also I'm wondering if this should be a `base::WeakPtr<tabs::TabInterface>` - unless we have a high level of confidence that this would never outlive its host tab.
- Keren Zhu
- Thomas Lukaszewicz
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. |
Thomas Lukaszewicz (Gerrit)
Thomas Lukaszewicz added 1 comment![]( "Open in Gerrit")
static views::UniqueWidgetPtr Show(Really quick question @kere...@chromium.org - do we prefer to use this or just a `std::unique_ptr` for CLIENT_OWNS WIDGET?
I think we should probably make this `unique_ptr<views::Widget>` instead of `UniqueWidgetPtr`.
It looks like the later is meant to manage other Widget ownership models whereas we are guaranteeing this is `CLIENT_OWNS_WIDGET` and we shouldn't need to close the widget when destroying it or worry about the `NativeWidget` deleting `Widget`.
Related details
- Foromo Daniel Soromou
- Keren Zhu
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. |
Foromo Daniel Soromou (Gerrit)
Foromo Daniel Soromou added 1 comment![]( "Open in Gerrit")
static views::UniqueWidgetPtr Show(Thomas LukaszewiczReally quick question @kere...@chromium.org - do we prefer to use this or just a `std::unique_ptr` for CLIENT_OWNS WIDGET?
I think we should probably make this `unique_ptr<views::Widget>` instead of `UniqueWidgetPtr`.
It looks like the later is meant to manage other Widget ownership models whereas we are guaranteeing this is `CLIENT_OWNS_WIDGET` and we shouldn't need to close the widget when destroying it or worry about the `NativeWidget` deleting `Widget`.
- Keren Zhu
- Thomas Lukaszewicz
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Thomas Lukaszewicz (Gerrit)
Thomas Lukaszewicz voted and added 2 comments![]( "Open in Gerrit")
Votes added by Thomas Lukaszewicz
| Code-Review | +1 |
2 comments
static std::unique_ptr<views::Widget> Show(nit: We should also add documentation indicating that clients should use [`Widget::MakeCloseSynchronous()`](https://source.chromium.org/chromium/chromium/src/+/main:ui/views/widget/widget.h;l=902;drc=cf6881766a4b05771620eae6f874f1c4ed422e12) to intercept close events from the created widget.
Related details
- Foromo Daniel Soromou
- Keren Zhu
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. |
Foromo Daniel Soromou (Gerrit)
Foromo Daniel Soromou added 1 comment![]( "Open in Gerrit")
nit: We should also add documentation indicating that clients should use [`Widget::MakeCloseSynchronous()`](https://source.chromium.org/chromium/chromium/src/+/main:ui/views/widget/widget.h;l=902;drc=cf6881766a4b05771620eae6f874f1c4ed422e12) to intercept close events from the created widget.
- Keren Zhu
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Foromo Daniel Soromou (Gerrit)
Foromo Daniel Soromou voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Foromo Daniel Soromou (Gerrit)
Foromo Daniel Soromou voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
- Keren Zhu
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
24 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/web_dialogs/chrome_webui_dialog_unittest.cc
Insertions: 2, Deletions: 0.
@@ -58,6 +58,8 @@
void TearDown() override {
constrained_window::SetConstrainedWindowViewsClient(nullptr);
base::RunLoop().RunUntilIdle();
+ profile_.reset();
+ base::RunLoop().RunUntilIdle();
ChromeViewsTestBase::TearDown();
}
```
```
The name of the file: chrome/browser/ui/views/web_dialogs/chrome_webui_dialog.h
Insertions: 2, Deletions: 1.
@@ -88,7 +88,8 @@
// Creates and shows a dialog with the given `contents_wrapper` and `spec`.
// Returns the created widget. By default, the Widget returned is
// CLIENT_OWNS_WIDGET. Therefore, the caller is responsible to manage the
- // lifetime.
+ // lifetime. Additionally, caller can use `Widget::MakeCloseSynchronous()` to
+ // intercept close events from the created widget.
static std::unique_ptr<views::Widget> Show(
gfx::NativeWindow parent,
std::unique_ptr<WebUIContentsWrapper> contents_wrapper,
```
Change information
Introduce TopChromeWebUIDialogDelegate
Add a reusable dialog delegate for TopChrome WebUI dialogs to reduce
boilerplate code and provide a consistent implementation for widget
sizing, lifecycle management, and flicker prevention. Currently,
developers building top-level WebUI dialogs duplicate code to handle
these concerns. This change establishes a unified approach leveraging
WebUIContentsWrapper and EnableAutoResize. Key features:
- Declarative configuration via WebDialogSpec.
- Auto-resizing within min/max bounds.
- Flicker prevention by waiting for explicit show from JS.
- Corner radius clipping to NativeViewHost.
This also adds unit tests and a README in the new
chrome/browser/ui/views/web_dialogs directory.
- M chrome/browser/ui/BUILD.gn
- A chrome/browser/ui/views/web_dialogs/BUILD.gn
- A chrome/browser/ui/views/web_dialogs/DIR_METADATA
- A chrome/browser/ui/views/web_dialogs/README.md
- A chrome/browser/ui/views/web_dialogs/chrome_webui_dialog.cc
- A chrome/browser/ui/views/web_dialogs/chrome_webui_dialog.h
- A chrome/browser/ui/views/web_dialogs/chrome_webui_dialog_unittest.cc
- M chrome/test/BUILD.gn
Code-Review: +1 by Thomas Lukaszewicz, +1 by Dana Fried
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Keren Zhu (Gerrit)
Keren Zhu added 1 comment![]( "Open in Gerrit")
base::RunLoop().RunUntilIdle();RunUntilIdle() has historically been a source of flakiness. Do we absolutely need to run the run loop here? If so, can we listen for some event or use `base::test::RunUntil`?
Sorry for leaving comment late. I was in a conference during the day time.
Related details
- Foromo Daniel Soromou
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Foromo Daniel Soromou (Gerrit)
Foromo Daniel Soromou added 1 comment![]( "Open in Gerrit")
base::RunLoop().RunUntilIdle();RunUntilIdle() has historically been a source of flakiness. Do we absolutely need to run the run loop here? If so, can we listen for some event or use `base::test::RunUntil`?
Sorry for leaving comment late. I was in a conference during the day time.
Fixed in the CL : https://crrev.com/c/7782255.
Thank you.
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |