Document PiP: Improve standalone window parity with browser-backed [chromium/src : main]
Jiayu Chen (Gerrit)
Jiayu Chen added 1 comment![]( "Open in Gerrit")
Hi reviewers, could u help review this when available? Thx.
Related details
- Qikai Zhong
- 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 6 comments![]( "Open in Gerrit")
Overall lgtm, just some quick comments and recommendations for follow-up CLs.
"document_pip_native_widget_mac.h",nit: Could we add the header to `public`
SetBorder(views::CreateEmptyBorder(gfx::Insets::VH(3, 2)));Not for this but could we add a TODO to add these magic constants (here and below) to the layout provider?
const int alpha = GetState() == STATE_PRESSED ? 0x33 : 0x22;Could we add `kColorPipWindowForegroundHovered` and `kColorPipWindowForegroundPressed` to `chrome_color_id.h` and add appropriate mixer definitions? (this can be done as a follow up to keep this CL simpler)
if (back_to_tab_button_) {
back_to_tab_button_->SetPaintToLayer();
back_to_tab_button_->layer()->SetFillsBoundsOpaquely(false);
}
close_image_button_->SetPaintToLayer();
close_image_button_->layer()->SetFillsBoundsOpaquely(false);Layers are simple from a code perspective but can be inefficient performance wise (especially when not configured to be opaque.
Would it work to create a wrapper view with the same preferred size as the wrapped button, and toggle visibility on the child? I.e.
```
// 1. Create an empty container view to act as the placeholder in the layout
auto wrapper = std::make_unique<views::View>();
// 2. Add your actual view inside the wrapper
back_to_tab_button_ = wrapper->AddChildView(CreatePipTitleBarButton(
back_icon,
l10n_util::GetStringUTF16(
IDS_PICTURE_IN_PICTURE_BACK_TO_TAB_CONTROL_TEXT),
// Safety: The widget owns the frame view and its child buttons,
// so the callback cannot outlive the widget.
base::BindRepeating(back_to_tab_cb, base::Unretained(this)))););
// 3. Force the wrapper to always report the button's size to the parent LayoutManager
wrapper->SetPreferredSize(back_to_tab_button_->GetPreferredSize());
// Add the wrapper to your main layout
button_container_view_->AddChildView(std::move(wrapper));
```
Similarly for the close image button.
// Match browser-backed PiP title behavior: for standard web URLs, show the
// security-display origin (scheme/path omitted, subdomain preserved); for
// file URLs, keep the path but omit the file:// scheme (the File security
// chip already communicates the scheme).
std::u16string origin_text;
if (url.SchemeIsFile()) {
const url_formatter::FormatUrlTypes format_types =
url_formatter::kFormatUrlOmitDefaults |
url_formatter::kFormatUrlOmitFileScheme;
origin_text =
url_formatter::FormatUrl(url, format_types, base::UnescapeRule::NORMAL,
/*new_parsed=*/nullptr, /*prefix_end=*/nullptr,
/*offset_for_adjustment=*/nullptr);
} else {
origin_text = url_formatter::FormatUrlForSecurityDisplay(
url, url_formatter::SchemeDisplay::OMIT_HTTP_AND_HTTPS);
}
window_title_->SetText(origin_text);
origin_chip_->GetViewAccessibility().SetDescription(origin_text);
// Choose the elision direction by scheme, mirroring
// PictureInPictureBrowserFrameView. For file URLs we elide the tail, since
// the file name and/or query part of the file URL can be crafted to look
// like an origin for spoofing. For HTTPS URLs we elide the head, since in the
// HTTPS case everything besides the origin is removed for display. Extension
// and isolated-app URLs likewise elide the tail, where the tail is the more
// important part to keep visible.
gfx::ElideBehavior elide_behavior =
url.SchemeIsFile() ? gfx::ELIDE_TAIL : gfx::ELIDE_HEAD;
#if BUILDFLAG(ENABLE_EXTENSIONS)
if (url.SchemeIs(extensions::kExtensionScheme) ||
url.SchemeIs(webapps::kIsolatedAppScheme)) {
elide_behavior = gfx::ELIDE_TAIL;
}
#endif
window_title_->SetElideBehavior(elide_behavior);Since this is security sensitive can we land the changes in `UpdateOriginAndSecurity()` in a follow-up CL? (better for blame history)
Related details
- Jiayu Chen
- Qikai Zhong
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. |
Jiayu Chen (Gerrit)
Jiayu Chen voted and added 6 comments![]( "Open in Gerrit")
Votes added by Jiayu Chen
| Commit-Queue | +1 |
6 comments
nit: Could we add the header to `public`
Done
SetBorder(views::CreateEmptyBorder(gfx::Insets::VH(3, 2)));Not for this but could we add a TODO to add these magic constants (here and below) to the layout provider?
I reverted changes made to `OriginChipButton`, considering reuse `IconLabelBubbleView` would be cheaper. Plan to do it in a following CL.
const int alpha = GetState() == STATE_PRESSED ? 0x33 : 0x22;Could we add `kColorPipWindowForegroundHovered` and `kColorPipWindowForegroundPressed` to `chrome_color_id.h` and add appropriate mixer definitions? (this can be done as a follow up to keep this CL simpler)
as above.
if (back_to_tab_button_) {
back_to_tab_button_->SetPaintToLayer();
back_to_tab_button_->layer()->SetFillsBoundsOpaquely(false);
}
close_image_button_->SetPaintToLayer();
close_image_button_->layer()->SetFillsBoundsOpaquely(false);Layers are simple from a code perspective but can be inefficient performance wise (especially when not configured to be opaque.
Would it work to create a wrapper view with the same preferred size as the wrapped button, and toggle visibility on the child? I.e.
```
// 1. Create an empty container view to act as the placeholder in the layout
auto wrapper = std::make_unique<views::View>();// 2. Add your actual view inside the wrapper
back_to_tab_button_ = wrapper->AddChildView(CreatePipTitleBarButton(
back_icon,
l10n_util::GetStringUTF16(
IDS_PICTURE_IN_PICTURE_BACK_TO_TAB_CONTROL_TEXT),
// Safety: The widget owns the frame view and its child buttons,
// so the callback cannot outlive the widget.
base::BindRepeating(back_to_tab_cb, base::Unretained(this)))););// 3. Force the wrapper to always report the button's size to the parent LayoutManager
wrapper->SetPreferredSize(back_to_tab_button_->GetPreferredSize());// Add the wrapper to your main layout
button_container_view_->AddChildView(std::move(wrapper));
```Similarly for the close image button.
Thanks, this is great advice.
// Match browser-backed PiP title behavior: for standard web URLs, show theSince this is security sensitive can we land the changes in `UpdateOriginAndSecurity()` in a follow-up CL? (better for blame history)
- Qikai Zhong
- 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 1 comment![]( "Open in Gerrit")
Votes added by Thomas Lukaszewicz
| Code-Review | +1 |
1 comment
- Jiayu Chen
- Qikai Zhong
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jiayu Chen (Gerrit)
Jiayu Chen voted Commit-Queue+2![]( "Open in Gerrit")
- Qikai Zhong
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![]( "Open in Gerrit")
Unreviewed changes
12 is the latest approved patch-set.
No files were changed between the latest approved patch-set and the submitted one.
Change information
Document PiP: Improve standalone window parity with browser-backed
The standalone Document Picture-in-Picture window should visually and
behaviorally match the Browser-backed Document PiP window. This CL
closes some UI-parity gaps in the standalone path:
- Add DocumentPipNativeWidgetMac so the borderless standalone widget
gets the same macOS rounded corners, shadow, and resize affordance as
the Browser-backed PiP window while still drawing its own title bar.
- Recompute the outer window bounds after Widget::Init() so a requested
inner web-contents size is honored. This mirrors the Browser-backed
post-init bounds adjustment and avoids creating the standalone window
one top-bar height too short.
- Rework the top bar to mirror the Browser-backed location-chip layout:
a clickable security chip with hover/pressed styling, security icon,
and chip text such as "File" / "Not secure" / "Dangerous", plus a
separate draggable window-title label.
- Format the window title like the Browser-backed frame.
- Keep the back-to-tab and close buttons laid out while inactive and
fade them via layer opacity instead of toggling visibility, so the
title no longer expands into the reserved window-control area.
- Restrict the JavaScript dialog native-window title override to macOS,
where the platform accessibility API ignores the dialog description
and derives the native window name from the child RootView.
- M chrome/browser/ui/views/picture_in_picture/BUILD.gn
- M chrome/browser/ui/views/picture_in_picture/document_pip_dialog_manager_delegate.cc
- M chrome/browser/ui/views/picture_in_picture/document_pip_dialog_manager_delegate_unittest.cc
- M chrome/browser/ui/views/picture_in_picture/document_pip_frame_view.cc
- M chrome/browser/ui/views/picture_in_picture/document_pip_frame_view.h
- M chrome/browser/ui/views/picture_in_picture/document_pip_frame_view_unittest.cc
- M chrome/browser/ui/views/picture_in_picture/document_pip_host.cc
- M chrome/browser/ui/views/picture_in_picture/document_pip_host_unittest.cc
- A chrome/browser/ui/views/picture_in_picture/document_pip_native_widget_mac.h
- A chrome/browser/ui/views/picture_in_picture/document_pip_native_widget_mac.mm
Code-Review: +1 by Thomas Lukaszewicz
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |