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:
Patch Set #67, Line 121:
int64_t display_id = display::kInvalidDisplayIdnit: Similar to the other comment and more generally for other locations in this CL - in places where we don't expect clients to need to pass a display_id would it be easier to have these data members / params be optionals rather than checking against the invalid display id?
File ui/ozone/platform/wayland/host/wayland_toplevel_window.h:
Patch Set #67, Line 261:
int64_t initial_display_id_nit: Does it make sense for this to be an optional? It seems like it is effectively an optional with `display::kInvalidDisplayId` as the sentinel if no display id is provided.
File ui/views/widget/native_widget_aura.cc:
Patch Set #48, Line 302:
window_->SetBoundsInScreen(window_bounds, dst_display);
Done. […]
Got it, thanks for the context.
File ui/views/widget/native_widget_aura.cc:
Patch Set #67, Line 287:
#if BUILDFLAG(IS_CHROMEOS)Can we get away with only setting the optional dispaly_id on chromeos platforms and avoid needing the macros? Or is there a reason we need this?
If there is a reason we need this - should we consistently buildflag the display_id param to ChromeOS platforms? (for e.g. we have ChromeOS guards around the param in browser.h but not around the param in widget.h).
File ui/views/widget/widget.h:
Patch Set #48, Line 360:
absl::optional<int64_t> display_id;
The current expected behavior of widget's initial bounds is very complicated, as it involves session […]
Got it, now is probably not the time to address this.
Adding a comment may still help indicating how the display_id is intended to be used in Widget::Init may be helpful. Something simple would work, perhaps along the lines of
```
init will use display_id as a hint and attempt to place the Widget on the
associated display if possible.
```
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 ha […]
has not 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: Mitsuru Oshima.
Mitsuru Oshima uploaded patch set #68 to this change.
Create a detached tab on the same display as original window.
* Intrduce display_id parameter in Browser::CreateParams/Widget::InitParams
* Add display parameter to
WindowParentingClient::GetDefaultParent
as a hint to prefer the specified display even if the bounds is
mostly outside of the display.
* 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.
Bug: 1455178
Test: TBD.
Change-Id: I486594789ec307bfcaf4ce81ba1c221c9bbeb93f
---
M ash/drag_drop/drag_drop_tracker.cc
M ash/root_window_controller.cc
M ash/root_window_controller_unittest.cc
M ash/shelf/drag_window_from_shelf_controller_unittest.cc
M ash/shelf/test/shelf_layout_manager_test_base.cc
M ash/test/ash_test_base.cc
M ash/test/test_window_builder.cc
M ash/wm/container_finder.cc
M ash/wm/container_finder.h
M ash/wm/desks/desks_unittests.cc
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.cc
M ash/wm/window_restore/window_restore_controller_unittest.cc
M ash/wm/workspace/workspace_layout_manager_unittest.cc
M chrome/browser/ui/ash/shelf/chrome_shelf_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/browser_view.cc
M chrome/browser/ui/views/frame/desktop_browser_frame_lacros.cc
M chrome/browser/ui/views/tabs/tab_drag_controller.cc
M chromecast/graphics/cast_window_manager_aura.cc
M chromecast/graphics/cast_window_manager_aura.h
M components/exo/client_controlled_shell_surface.cc
M components/exo/client_controlled_shell_surface.h
M components/exo/client_controlled_shell_surface_unittest.cc
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 content/browser/renderer_host/render_widget_host_view_aura.cc
M content/browser/renderer_host/render_widget_host_view_aura_unittest.cc
M content/browser/web_contents/web_contents_view_aura.cc
M content/shell/browser/shell_platform_delegate_views.cc
M extensions/shell/browser/root_window_controller.cc
M extensions/shell/browser/root_window_controller.h
M fuchsia_web/webengine/browser/frame_window_tree_host.cc
M headless/lib/browser/headless_window_parenting_client.cc
M headless/lib/browser/headless_window_parenting_client.h
M ui/aura/client/window_parenting_client.cc
M ui/aura/client/window_parenting_client.h
M ui/aura/demo/demo_main.cc
M ui/aura/test/aura_test_base.cc
M ui/aura/test/test_window_parenting_client.cc
M ui/aura/test/test_window_parenting_client.h
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_native_widget_aura.cc
M ui/views/widget/desktop_aura/desktop_native_widget_aura_unittest.cc
M ui/views/widget/desktop_aura/desktop_window_tree_host_platform.cc
M ui/views/widget/native_widget_aura.cc
M ui/views/widget/native_widget_aura.h
M ui/views/widget/widget.cc
M ui/views/widget/widget.h
M ui/wm/core/transient_window_manager_unittest.cc
M ui/wm/test/wm_test_helper.cc
M ui/wm/test/wm_test_helper.h
66 files changed, 518 insertions(+), 234 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.
Patch set 68:Commit-Queue +1
6 comments:
File components/exo/shell_surface_base.h:
Patch Set #67, Line 401:
TODO()trivial nit: Could we tag an ldap or bug?
Done
File components/exo/wayland/zaura_output_manager.h:
Patch Set #67, Line 41:
output_resource1nit: `output_resource`
Done
File components/exo/wayland/zaura_output_manager.cc:
Patch Set #48, Line 78:
wl_client_for_each_resource(client, closure, &data);
I think you're right and it shouldn't happen afaict, but I have seen deep callstacks where code ends […]
My understanding is that when each resource should be removed from the list when it is indeed removed? I'm hesitant to add check if it shouldn't happen and rather just let it fail, as it may indicate a problem.
I removed the client from the call and instead used wl_resource_get_client, then added CHECK to make sure they're valid.
File ui/views/widget/native_widget_aura.cc:
Patch Set #67, Line 287:
#if BUILDFLAG(IS_CHROMEOS)
Can we get away with only setting the optional dispaly_id on chromeos platforms and avoid needing th […]
I was going to ad macro to widget.h as well (which is done in latest patch)
File ui/views/widget/widget.h:
Patch Set #48, Line 360:
absl::optional<int64_t> display_id;
Got it, now is probably not the time to address this. […]
Done
File ui/views/widget/widget.cc:
Patch Set #48, Line 463:
if (!display_specified) {
has not been specified*
Done (changed to use should_set_initial_bounds)
To view, visit change 4606716. To unsubscribe, or for help writing mail filters, visit settings.
Mitsuru Oshima (Gerrit)
Attention is currently required from: Scott Violet, Thomas Lukaszewicz.
Mitsuru Oshima would like Scott Violet to review this change.
To view, visit change 4606716. To unsubscribe, or for help writing mail filters, visit settings.
Mitsuru Oshima (Gerrit)
Attention is currently required from: Scott Violet, Thomas Lukaszewicz.
1 comment:
Patchset:
sky@, can you first take a quick look at changes in(ui/views/widget, browser.h/cc, and tell me if this approach looks okay to you (or have alternative suggestion)?
thank you.
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, Scott Violet.
6 comments:
Patchset:
Overall everything looks good to me, just some trivial nits and the open comments on whether we should use an optional for display_id params instead of checking against the invalid display id.
Will +1 after sky@ has had a chance to take a pass.
File chrome/browser/ui/browser.h:
trivial nit: remove
File components/exo/wayland/zaura_output_manager.cc:
Patch Set #48, Line 78:
wl_client_for_each_resource(client, closure, &data);
My understanding is that when each resource should be removed from the list when it is indeed remove […]
It's more of a problem with the upstream libwayland we're using. The client destructor deletes and frees its resources one by one, but doesn't remove the resource from its list until the very end in `wl_map_release()`[1]. It's been the case that we set user_data on resources, and the destructor for these user_data objects can call back into something that iterates over the client resources (tripping over one of the freed resources).
The CHECK() in the latest patchset should be fine (since this probably won't happen even if it's technically possible).
File ui/platform_window/platform_window_init_properties.h:
Patch Set #68, Line 149:
absl::optional<int64_t> display_id;trivial nit: It might help to have a short comment here to avoid needing to scan through the display_id declaration in other param files for a quick tldr on what this does.
File ui/views/widget/native_widget_aura.cc:
Patch Set #67, Line 287:
#if BUILDFLAG(IS_CHROMEOS)
I was going to ad macro to widget. […]
Thanks, latest patchset lgtm for the macro
File ui/views/widget/widget.cc:
trivial nit: be
To view, visit change 4606716. To unsubscribe, or for help writing mail filters, visit settings.
Scott Violet (Gerrit)
Attention is currently required from: Mitsuru Oshima.
1 comment:
Patchset:
I took a quick look at the core parts. The changes largely seem fine to me. Thomas is an owner of a lot of this as well. What files do you want me to review?
To view, visit change 4606716. To unsubscribe, or for help writing mail filters, visit settings.
Scott Violet (Gerrit)
File ui/views/widget/widget.h:
Patch Set #68, Line 362:
bounds` is side the displayPlease better document what this means. I think you mean `bounds` is relative to the specified display, but you should make that clear.
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.
To view, visit change 4606716. To unsubscribe, or for help writing mail filters, visit settings.
Mitsuru Oshima (Gerrit)
Attention is currently required from: Daniel Nicoara, Dmitry Gozman, Rakina Zata Amni, Reilly Grant, Scott Violet, Sergey Ulanov.
Mitsuru Oshima would like Rakina Zata Amni, Reilly Grant, Dmitry Gozman, Daniel Nicoara and Sergey Ulanov to review this change.
To view, visit change 4606716. To unsubscribe, or for help writing mail filters, visit settings.
Mitsuru Oshima (Gerrit)
Attention is currently required from: Daniel Nicoara, Dmitry Gozman, Rakina Zata Amni, Reilly Grant, Scott Violet, Sergey Ulanov.
6 comments:
Patchset:
I took a quick look at the core parts. The changes largely seem fine to me. […]
Please review ui/views and chrome/browser/ui/views. Thank you.
File chrome/browser/ui/browser.h:
trivial nit: remove
Done
File components/exo/wayland/zaura_output_manager.cc:
Patch Set #48, Line 78:
wl_client_for_each_resource(client, closure, &data);
It's more of a problem with the upstream libwayland we're using. […]
I see, that's problematic. Thank you for clarification.
File ui/platform_window/platform_window_init_properties.h:
Patch Set #68, Line 149:
absl::optional<int64_t> display_id;
trivial nit: It might help to have a short comment here to avoid needing to scan through the display […]
Done
File ui/views/widget/widget.h:
Patch Set #68, Line 362:
bounds` is side the display
Please better document what this means. […]
Updated the comment.
File ui/views/widget/widget.cc:
trivial nit: be
Done
To view, visit change 4606716. To unsubscribe, or for help writing mail filters, visit settings.
Mitsuru Oshima (Gerrit)
Attention is currently required from: Daniel Nicoara, Dmitry Gozman, Reilly Grant, Scott Violet, Sergey Ulanov.
Mitsuru Oshima removed Rakina Zata Amni from this change.
To view, visit change 4606716. To unsubscribe, or for help writing mail filters, visit settings.
Mitsuru Oshima (Gerrit)
Attention is currently required from: Daniel Nicoara, Dmitry Gozman, Reilly Grant, Scott Violet, Sergey Ulanov.
1 comment:
Patchset:
-rakina@
reillyg@ -> extensions
dgozman@ -> content/, headless/
sergeyu@ -> fuchsia_web/
dnicoara@ -> chromecast/
To view, visit change 4606716. To unsubscribe, or for help writing mail filters, visit settings.
Mitsuru Oshima (Gerrit)
Attention is currently required from: Daniel Nicoara, Dmitry Gozman, Reilly Grant, Scott Violet, Sergey Ulanov.
Patch set 68:Commit-Queue +1
To view, visit change 4606716. To unsubscribe, or for help writing mail filters, visit settings.
Scott Violet (Gerrit)
Attention is currently required from: Daniel Nicoara, Dmitry Gozman, Mitsuru Oshima, Reilly Grant, Sergey Ulanov.
File ui/views/widget/widget.h:
Patch Set #68, Line 362:
bounds` is side the display
Updated the comment.
I don't see the update here?
To view, visit change 4606716. To unsubscribe, or for help writing mail filters, visit settings.
Daniel Nicoara (Gerrit)
Attention is currently required from: Dmitry Gozman, Mitsuru Oshima, Reilly Grant, Sergey Ulanov.
Patchset:
chromecast/* lgtm
Mitsuru Oshima (Gerrit)
Attention is currently required from: Dmitry Gozman, Mitsuru Oshima, Reilly Grant, Sergey Ulanov.
Mitsuru Oshima uploaded patch set #69 to this change.
66 files changed, 520 insertions(+), 234 deletions(-)
To view, visit change 4606716. To unsubscribe, or for help writing mail filters, visit settings.
Mitsuru Oshima (Gerrit)
Attention is currently required from: Dmitry Gozman, Reilly Grant, Scott Violet, Sergey Ulanov.
1 comment:
File ui/views/widget/widget.h:
Patch Set #68, Line 362:
bounds` is side the display
I don't see the update here?
Oops, upload wasn't completed. Uploaded.
To view, visit change 4606716. To unsubscribe, or for help writing mail filters, visit settings.
Mitsuru Oshima (Gerrit)
Attention is currently required from: Dmitry Gozman, Reilly Grant, Scott Violet, Sergey Ulanov.
Mitsuru Oshima uploaded patch set #70 to this change.
To view, visit change 4606716. To unsubscribe, or for help writing mail filters, visit settings.
Dmitry Gozman (Gerrit)
Attention is currently required from: Mitsuru Oshima, Reilly Grant, Scott Violet, Sergey Ulanov.
Patch set 70:Code-Review +1
Mitsuru Oshima (Gerrit)
Attention is currently required from: Reilly Grant, Scott Violet, Sergey Ulanov.
Patch set 70:Commit-Queue +1
Sergey Ulanov (Gerrit)
Attention is currently required from: Mitsuru Oshima, Reilly Grant, Scott Violet.
Patch set 70:Code-Review +1
1 comment:
Patchset:
//fuchsia_web LGTM
To view, visit change 4606716. To unsubscribe, or for help writing mail filters, visit settings.
Mitsuru Oshima (Gerrit)
Attention is currently required from: Finnur Thorarinsson, Reilly Grant, Scott Violet.
Mitsuru Oshima would like Finnur Thorarinsson to review this change.
To view, visit change 4606716. To unsubscribe, or for help writing mail filters, visit settings.
Mitsuru Oshima (Gerrit)
Attention is currently required from: Finnur Thorarinsson, Reilly Grant, Scott Violet.
To view, visit change 4606716. To unsubscribe, or for help writing mail filters, visit settings.
Finnur Thorarinsson (Gerrit)
Attention is currently required from: Mitsuru Oshima, Reilly Grant, Scott Violet.
Patch set 70:Code-Review +1
4 comments:
Patchset:
extensions/* LGTM. A few nits though.
File components/exo/wayland/shell_unittest.cc:
Patch Set #70, Line 375:
updaqtednit: s/updaqted/updated/
Patch Set #70, Line 410:
updaqtedditto.
File ui/views/widget/widget.h:
Patch Set #70, Line 361:
insdienit: s/insdie/inside/
To view, visit change 4606716. To unsubscribe, or for help writing mail filters, visit settings.
Mitsuru Oshima (Gerrit)
Attention is currently required from: Reilly Grant, Scott Violet.
Patch set 72:Commit-Queue +2
4 comments:
Patchset:
thank you for review!
File components/exo/wayland/shell_unittest.cc:
Patch Set #70, Line 375:
updaqtednit: s/updaqted/updated/
Done
Patch Set #70, Line 410:
updaqtedditto.
Done
File ui/views/widget/widget.h:
Patch Set #70, Line 361:
insdienit: s/insdie/inside/
Done
To view, visit change 4606716. To unsubscribe, or for help writing mail filters, visit settings.
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted this change.
70 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: ui/views/widget/widget.h
Insertions: 1, Deletions: 1.
@@ -358,7 +358,7 @@
gfx::Rect bounds;
#if BUILDFLAG(IS_CHROMEOS)
- // If specified and the `bounds` is insdie the specified display, the widget
+ // If specified and the `bounds` is inside the specified display, the widget
// will be created on this display. Otherwise, the display matching the
// `bounds` will be used.
absl::optional<int64_t> display_id;
```
```
The name of the file: components/exo/wayland/shell_unittest.cc
Insertions: 2, Deletions: 2.
@@ -372,7 +372,7 @@
// If display is not specified, new window will be placed fully inside the
// display.
// TODO(crbug.com/1291592): This logic is not consistent with
- // ash. This has to be updaqted once the bug is fixed.
+ // ash. This has to be updated once the bug is fixed.
EXPECT_EQ(gfx::Rect{kPrimarilyOnPrimary.size()},
shell_surface_base->GetWidget()->GetWindowBoundsInScreen());
}
@@ -407,7 +407,7 @@
shell_surface_base->GetWidget()->GetNativeWindow())
.id());
// TODO(crbug.com/1291592): This logic is not consistent with
- // ash. This has to be updaqted once the bug is fixed.
+ // ash. This has to be updated once the bug is fixed.
EXPECT_EQ(gfx::Rect({100, 0}, kAlmostOnPrimary.size()),
shell_surface_base->GetWidget()->GetWindowBoundsInScreen());
}
```
Create a detached tab on the same display as original window.
* Intrduce display_id parameter in Browser::CreateParams/Widget::InitParams
* Add display parameter to
WindowParentingClient::GetDefaultParent
as a hint to prefer the specified display even if the bounds is
mostly outside of the display.
* 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.
Bug: 1455178
Test: TBD.
Change-Id: I486594789ec307bfcaf4ce81ba1c221c9bbeb93f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4606716
Reviewed-by: Scott Violet <s...@chromium.org>
Reviewed-by: Thomas Lukaszewicz <tl...@chromium.org>
Reviewed-by: Daniel Nicoara <dnic...@chromium.org>
Reviewed-by: Finnur Thorarinsson <fin...@chromium.org>
Reviewed-by: Dmitry Gozman <dgo...@chromium.org>
Commit-Queue: Mitsuru Oshima <osh...@chromium.org>
Reviewed-by: Sergey Ulanov <ser...@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1198305}