Create a detached tab on the same display as original window. [chromium/src : main]
Mitsuru Oshima (Gerrit)
Attention is currently required from: Thomas Lukaszewicz.
Mitsuru Oshima would like Thomas Lukaszewicz to review this change.
Create a detached tab on the same display as original window.
* Intrduce display_id parameter in Browser::CreateParams/Widget::InitParams
* Update WindowParentingController to honor the display (root window)
Fallback if the window will be off screen.
* Do not use saved bounds for detached tab.
Bug: 1455178
Test: TBD.
Change-Id: I486594789ec307bfcaf4ce81ba1c221c9bbeb93f
---
M ash/root_window_controller.cc
M ash/wm/container_finder.cc
M ash/wm/container_finder.h
M ash/wm/window_finder_unittest.cc
M ash/wm/window_parenting_controller.cc
M ash/wm/window_parenting_controller.h
M ash/wm/window_restore/window_restore_controller_unittest.cc
M chrome/browser/ui/browser.h
M chrome/browser/ui/views/frame/browser_frame_ash.cc
M chrome/browser/ui/views/frame/desktop_browser_frame_lacros.cc
M chrome/browser/ui/views/tabs/tab_drag_controller.cc
M components/exo/client_controlled_shell_surface.cc
M components/exo/client_controlled_shell_surface.h
M components/exo/shell_surface_base.cc
M components/exo/shell_surface_base.h
M components/exo/wayland/shell_unittest.cc
M components/exo/wayland/wayland_display_observer.h
M components/exo/wayland/zaura_output_manager.cc
M components/exo/wayland/zaura_output_manager.h
M components/exo/wayland/zaura_shell.cc
M components/exo/wayland/zaura_shell.h
M tools/chromeos/run_cros.sh
M ui/ozone/platform/wayland/host/shell_toplevel_wrapper.h
M ui/ozone/platform/wayland/host/wayland_toplevel_window.cc
M ui/ozone/platform/wayland/host/wayland_toplevel_window.h
M ui/ozone/platform/wayland/host/xdg_toplevel_wrapper_impl.cc
M ui/ozone/platform/wayland/host/xdg_toplevel_wrapper_impl.h
M ui/platform_window/platform_window_init_properties.h
M ui/views/widget/desktop_aura/desktop_window_tree_host_platform.cc
M ui/views/widget/native_widget_aura.cc
M ui/views/widget/widget.cc
M ui/views/widget/widget.h
32 files changed, 257 insertions(+), 70 deletions(-)
To view, visit change 4606716. To unsubscribe, or for help writing mail filters, visit settings.
Mitsuru Oshima (Gerrit)
Attention is currently required from: Thomas Lukaszewicz.
1 comment:
Patchset:
tluk@ can you take a first look?
I'm working on the test and please ignore changes in shell_unittest.cc
To view, visit change 4606716. To unsubscribe, or for help writing mail filters, visit settings.
Mitsuru Oshima (Gerrit)
Attention is currently required from: Thomas Lukaszewicz.
Mitsuru Oshima uploaded patch set #47 to this change.
Create a detached tab on the same display as original window.
* Intrduce display_id parameter in Browser::CreateParams/Widget::InitParams
* Update WindowParentingController to honor the display (root window)
Fallback if the window will be off screen.
* Do not use saved bounds for detached tab.
* Support the output parameter in set_window_bounds request.
I changed lacros size not to send the output unless the target
display id is specified to keep the same behavior as ToT.
To view, visit change 4606716. To unsubscribe, or for help writing mail filters, visit settings.
Thomas Lukaszewicz (Gerrit)
Attention is currently required from: Mitsuru Oshima.
8 comments:
Patchset:
Overall lgtm - just some requests for inline comments & questions regarding bounds
File components/exo/shell_surface_base.cc:
Patch Set #48, Line 1885:
return geometry_;Just checking - previously this was assumed to be screen coordinates and this is no longer the case right (seems like now this will be in display coordinates)?
File components/exo/wayland/zaura_output_manager.h:
nit: `output_resource`
File components/exo/wayland/zaura_output_manager.cc:
Patch Set #48, Line 78:
wl_client_for_each_resource(client, closure, &data);Just a heads up - if GetDisplayIdForOutput() is called during client destruction then we will likely end up with a crash (see [1]). It may not be likely but could happen. I had a bug to push the boolean tracking client destruction into some higher level object that could be generally accessible by ozone but there hasn't been a pressing need yet.
It might be fine to tag this code with crbug.com/1433187 that tracks the work to be done. Alternatively I can quickly push this check into something we can check from here.
File ui/platform_window/platform_window_init_properties.h:
Patch Set #48, Line 149:
absl::optional<int64_t> display_id;nit: Could we similarly add a quick comment here?
File ui/views/widget/native_widget_aura.cc:
Patch Set #48, Line 302:
window_->SetBoundsInScreen(window_bounds, dst_display);Should we CHECK that params.parent is null? Just to make sure bounds is being set correctly in the init params (see below)
```
// Specifies the initial bounds of the Widget. Default is empty, which means
// the NativeWidget may specify a default size. If the parent is specified,
// |bounds| is in the parent's coordinate system. If the parent is not
// specified, it's in screen's global coordinate system.
gfx::Rect bounds;
```
File ui/views/widget/widget.h:
Patch Set #48, Line 360:
absl::optional<int64_t> display_id;nit: Could we add a comment indicating that this is the desired display on which to create the widget.
Could we also indicate what the expected behavior might be if this is not set (i.e. does the widget get placed on the currently active display or perhaps there some other heuristic)?
Also is it expected that this be defined only if the `Widget::InitParams::parent` is null? If not we may want to update how window_bounds is used in native_widget_aura.cc and convert to screen coordinates if parent is defined before calling `window_->SetBoundsInScreen()`
File ui/views/widget/widget.cc:
Patch Set #48, Line 463:
if (!display_specified) {nit: Can we add a comment indicating why it is not necessary to set initial bounds if the display has been specified?
To view, visit change 4606716. To unsubscribe, or for help writing mail filters, visit settings.
Mitsuru Oshima (Gerrit)
Attention is currently required from: Thomas Lukaszewicz.
7 comments:
Patchset:
I need to revisit the approach on ash-chrome side. settings WIP and I'll ping you when it's ready.
File components/exo/shell_surface_base.cc:
Patch Set #48, Line 1885:
return geometry_;
Just checking - previously this was assumed to be screen coordinates and this is no longer the case […]
geometry_ is surface local, but for historical reasons, geomery_ for ARC (ClientControlledShellSurface) is screen coordinates. I know it's confusing and
we have a plan to clean this up, but this is what it is for now.
File components/exo/wayland/zaura_output_manager.h:
nit: `output_resource`
Done
File components/exo/wayland/zaura_output_manager.cc:
Patch Set #48, Line 78:
wl_client_for_each_resource(client, closure, &data);
Just a heads up - if GetDisplayIdForOutput() is called during client destruction then we will likely […]
Ack & added comment in h. For this particular scenario tho, I think the server won't receive (process) events after wl_client is being destroyed?
File ui/platform_window/platform_window_init_properties.h:
Patch Set #48, Line 149:
absl::optional<int64_t> display_id;nit: Could we similarly add a quick comment here?
Done
File ui/views/widget/native_widget_aura.cc:
Patch Set #48, Line 302:
window_->SetBoundsInScreen(window_bounds, dst_display);
Should we CHECK that params. […]
Done.
By the way, bounds settings is one of issues in widget impl. The bounds is updated multiple times in multiple places, and this is currently not exactly accurate.
File ui/views/widget/widget.h:
Patch Set #48, Line 360:
absl::optional<int64_t> display_id;
nit: Could we add a comment indicating that this is the desired display on which to create the widge […]
The current expected behavior of widget's initial bounds is very complicated, as it involves session restore, restore (uses saved state which is different from restore, and also most likely it has some platform specific behavior.
I agree that it will be nice if we can clarify this in a consistent way, but I think it'll be quite challenging to keep this info correct and up to date for all platforms. (as it may change later)
To view, visit change 4606716. To unsubscribe, or for help writing mail filters, visit settings.
Mitsuru Oshima (Gerrit)
Attention is currently required from: Thomas Lukaszewicz.
Patch set 50:Commit-Queue +1
Mitsuru Oshima (Gerrit)
Attention is currently required from: Thomas Lukaszewicz.
Patch set 51:Commit-Queue +1
Mitsuru Oshima (Gerrit)
Mitsuru Oshima (Gerrit)
Attention is currently required from: Thomas Lukaszewicz.
Patch set 52:Auto-Submit +1
Mitsuru Oshima (Gerrit)
Attention is currently required from: Thomas Lukaszewicz.
Patch set 52:-Auto-SubmitCommit-Queue +1
Mitsuru Oshima (Gerrit)
Attention is currently required from: Thomas Lukaszewicz.
Patch set 53:Commit-Queue +1
Mitsuru Oshima (Gerrit)
Attention is currently required from: Thomas Lukaszewicz.
Patch set 54:Commit-Queue +1
Mitsuru Oshima (Gerrit)
Attention is currently required from: Thomas Lukaszewicz.
Patch set 56:Commit-Queue +1
Mitsuru Oshima (Gerrit)
Attention is currently required from: Thomas Lukaszewicz.
Patch set 57:Commit-Queue +1
Mitsuru Oshima (Gerrit)
Attention is currently required from: Thomas Lukaszewicz.
Patch set 58:Commit-Queue +1
Mitsuru Oshima (Gerrit)
Attention is currently required from: Thomas Lukaszewicz.
Patch set 59:Commit-Queue +1
Mitsuru Oshima (Gerrit)
Attention is currently required from: Thomas Lukaszewicz.
Patch set 61:Commit-Queue +1
Mitsuru Oshima (Gerrit)
Attention is currently required from: Thomas Lukaszewicz.
Patch set 62:Commit-Queue +1
Mitsuru Oshima (Gerrit)
Attention is currently required from: Thomas Lukaszewicz.
1 comment:
Patchset:
Hi, can you take another look? I first tried to use context as an indicator
(which was originally added to distinguish win-ash and desktop mode), but it was
too difficult.
Instead, I added a display id parameter to WindowParentingClient API.
To view, visit change 4606716. To unsubscribe, or for help writing mail filters, visit settings.
Mitsuru Oshima (Gerrit)
Patchset:
Just take a quick look and let me know what you think. I'll add comments and a few more tests.
To view, visit change 4606716. To unsubscribe, or for help writing mail filters, visit settings.
Thomas Lukaszewicz (Gerrit)
Attention is currently required from: Mitsuru Oshima.
1 comment:
Commit Message:
Patch Set #65, Line 9:
* Intrduce display_id parameter in Browser::CreateParams/Widget::InitParamsHigh level question.
I understand the approach at a high level however I was wondering whether we need to actually set the display_id its own param in Widget::InitParams. Is it possible instead to get the appropriate display by making sure the `Widget::InitParams::context`[1] is set correctly to match the display that we would like the newly created window to open into?
It seems this can be set to the appropriate root window for a given desktop / display. This would also mean that we may not need the display_id for wm helper functions that take a context.
To view, visit change 4606716. To unsubscribe, or for help writing mail filters, visit settings.
Mitsuru Oshima (Gerrit)
Patch Set #65, Line 9:
* Intrduce display_id parameter in Browser::CreateParams/Widget::InitParams
High level question. […]
That's what I tried last week, but I eventually had to give up because
1) it changes the meaning of the context, and requires quite a lot of changes
2) it makes it difficult to write cross platform code because there is no 'root window for display' in desktop chrome.A bit of background. This was added to distinguish win-ash and NON win-ash environment, which could co-exist, not to tell the root window for a display.
To view, visit change 4606716. To unsubscribe, or for help writing mail filters, visit settings.
Thomas Lukaszewicz (Gerrit)
Patch Set #65, Line 9:
* Intrduce display_id parameter in Browser::CreateParams/Widget::InitParams
That's what I tried last week, but I eventually had to give up because […]
Ah yes I remember you mentioning this at the last sync, thanks for the background.
I can see why we went with the current approach. Overall this seems fine, just some quick comments on some of the APIs that came to mind when passing over the CL.
File ash/wm/container_finder.h:
Patch Set #65, Line 29:
aura::Window* root_window,nit: It's a little hard to understand how this is intended to work. The comment mentions this is generally used when moving from one root to another. Is `root_window` in this case the original root or the new root window (don't need to add a comment in the current pass, just trying to understand the API expectations).
File components/exo/wayland/zaura_shell.h:
void SetWindowBounds(int32_t x,
int32_t y,
int32_t width,
int32_t height,
int64_t display_id);nit: This feels a bit off since I wouldn't expect the `display_id` to be passed in with the other parameters into `SetWindowBounds()`. For e.g. it's not clear what the effect of passing `display_id` into this will have.
Does it make sense to instead add a separate `AuraTopLevel::SetDisplayId()`? I may also be misunderstanding this API.
File ui/views/widget/native_widget_aura.h:
void SetBoundsInternal(const gfx::Rect& bounds,
absl::optional<int64_t> display_id);nit: It is also somewhat unclear what this API needs with the `display_id` when setting bounds (you can deduce it from the impl but it's hard to figure out at a glance), perhaps a comment / explanation may help when we clean this up.
To view, visit change 4606716. To unsubscribe, or for help writing mail filters, visit settings.
Mitsuru Oshima (Gerrit)
Attention is currently required from: Thomas Lukaszewicz.
Patch set 67:Commit-Queue +1
3 comments:
File ash/wm/container_finder.h:
Patch Set #65, Line 29:
aura::Window* root_window,
nit: It's a little hard to understand how this is intended to work. […]
I think the comment is wrong or obsolete. I updated the comment.
File components/exo/wayland/zaura_shell.h:
void SetWindowBounds(int32_t x,
int32_t y,
int32_t width,
int32_t height,
int64_t display_id);
nit: This feels a bit off since I wouldn't expect the `display_id` to be passed in with the other pa […]
This is done for two reasons.
1) I want this level of API to be more explicit. Specifying the screen bounds w/o target display makes less sense on CrOS.
2) Updating it should be done atomically but we need to skip updating when a window is being dragged. Please see the SetWindowBounds impl.
File ui/views/widget/native_widget_aura.h:
void SetBoundsInternal(const gfx::Rect& bounds,
absl::optional<int64_t> display_id);
nit: It is also somewhat unclear what this API needs with the `display_id` when setting bounds (you […]
Done
To view, visit change 4606716. To unsubscribe, or for help writing mail filters, visit settings.
Thomas Lukaszewicz (Gerrit)
Attention is currently required from: Mitsuru Oshima.
14 comments:
Patchset:
Overall still lgtm. Only two questions I have relate to use of buildflags and optional params / members vs checking for the invalid display id.
Commit Message:
Patch Set #65, Line 9:
* Intrduce display_id parameter in Browser::CreateParams/Widget::InitParams
Ah yes I remember you mentioning this at the last sync, thanks for the background. […]
Done
File ash/wm/container_finder.h:
Patch Set #65, Line 29:
aura::Window* root_window,
I think the comment is wrong or obsolete. I updated the comment.
Thanks this is much clearer.
File components/exo/shell_surface_base.h:
Patch Set #67, Line 401:
TODO()trivial nit: Could we tag an ldap or bug?
File components/exo/shell_surface_base.cc:
Patch Set #48, Line 1885:
return geometry_;
geometry_ is surface local, but for historical reasons, geomery_ for ARC (ClientControlledShellSurfa […]
This is good to know thanks.
File components/exo/wayland/zaura_output_manager.h:
Patch Set #67, Line 41:
output_resource1nit: `output_resource`
File components/exo/wayland/zaura_output_manager.cc:
Patch Set #48, Line 78:
wl_client_for_each_resource(client, closure, &data);
Ack & added comment in h. […]
I think you're right and it shouldn't happen afaict, but I have seen deep callstacks where code ends up calling back into wayland/ozone code during destruction to query for ozone-related state (including display state) and this has caused crashes in the past.
Now that we have `IsClientDestroyed()` in server_util[1] we can check that here and early return if this evaluates true (probably up near where output_resource is checked).
File components/exo/wayland/zaura_shell.h:
void SetWindowBounds(int32_t x,
int32_t y,
int32_t width,
int32_t height,
int64_t display_id);
This is done for two reasons. […]
sgmt - thanks for the explanation.
File ui/ozone/platform/wayland/host/shell_toplevel_wrapper.h: