Create a detached tab on the same display as original window. [chromium/src : main]

0 views
Skip to first unread message

Mitsuru Oshima (Gerrit)

unread,
Aug 15, 2023, 2:20:08 AM8/15/23
to Thomas Lukaszewicz, ozone-...@chromium.org, cambickel+watc...@google.com, jimmyxgong+wat...@chromium.org, longbowei+watc...@google.com, michaelcheco+wa...@google.com, roblia...@chromium.org, sky+...@chromium.org, zentaro+watch...@chromium.org, zhangwenyu+wat...@google.com

Attention is currently required from: Thomas Lukaszewicz.

Mitsuru Oshima would like Thomas Lukaszewicz to review this change.

View 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.

Gerrit-MessageType: newchange
Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I486594789ec307bfcaf4ce81ba1c221c9bbeb93f
Gerrit-Change-Number: 4606716
Gerrit-PatchSet: 46
Gerrit-Owner: Mitsuru Oshima <osh...@chromium.org>
Gerrit-Reviewer: Mitsuru Oshima <osh...@chromium.org>
Gerrit-Reviewer: Thomas Lukaszewicz <tl...@chromium.org>
Gerrit-CC: Daniel Cheng <dch...@chromium.org>
Gerrit-Attention: Thomas Lukaszewicz <tl...@chromium.org>

Mitsuru Oshima (Gerrit)

unread,
Aug 15, 2023, 2:20:19 AM8/15/23
to ozone-...@chromium.org, cambickel+watc...@google.com, jimmyxgong+wat...@chromium.org, longbowei+watc...@google.com, michaelcheco+wa...@google.com, roblia...@chromium.org, sky+...@chromium.org, zentaro+watch...@chromium.org, zhangwenyu+wat...@google.com, Thomas Lukaszewicz, Tricium, Chromium LUCI CQ, chromium...@chromium.org, Daniel Cheng

Attention is currently required from: Thomas Lukaszewicz.

View Change

1 comment:

  • Patchset:

    • Patch Set #46:

      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.

Gerrit-MessageType: comment
Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I486594789ec307bfcaf4ce81ba1c221c9bbeb93f
Gerrit-Change-Number: 4606716
Gerrit-PatchSet: 46
Gerrit-Owner: Mitsuru Oshima <osh...@chromium.org>
Gerrit-Reviewer: Mitsuru Oshima <osh...@chromium.org>
Gerrit-Reviewer: Thomas Lukaszewicz <tl...@chromium.org>
Gerrit-CC: Daniel Cheng <dch...@chromium.org>
Gerrit-Attention: Thomas Lukaszewicz <tl...@chromium.org>
Gerrit-Comment-Date: Tue, 15 Aug 2023 06:20:05 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No

Mitsuru Oshima (Gerrit)

unread,
Aug 15, 2023, 2:21:48 AM8/15/23
to ozone-...@chromium.org, cambickel+watc...@google.com, jimmyxgong+wat...@chromium.org, longbowei+watc...@google.com, michaelcheco+wa...@google.com, roblia...@chromium.org, sky+...@chromium.org, zentaro+watch...@chromium.org, zhangwenyu+wat...@google.com

Attention is currently required from: Thomas Lukaszewicz.

Mitsuru Oshima uploaded patch set #47 to this change.

View 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.

Gerrit-MessageType: newpatchset
Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I486594789ec307bfcaf4ce81ba1c221c9bbeb93f
Gerrit-Change-Number: 4606716
Gerrit-PatchSet: 47

Thomas Lukaszewicz (Gerrit)

unread,
Aug 15, 2023, 12:25:30 PM8/15/23
to Mitsuru Oshima, ozone-...@chromium.org, cambickel+watc...@google.com, jimmyxgong+wat...@chromium.org, longbowei+watc...@google.com, michaelcheco+wa...@google.com, roblia...@chromium.org, sky+...@chromium.org, zentaro+watch...@chromium.org, zhangwenyu+wat...@google.com, Tricium, Chromium LUCI CQ, chromium...@chromium.org, Daniel Cheng

Attention is currently required from: Mitsuru Oshima.

View Change

8 comments:

  • Patchset:

    • Patch Set #48:

      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:

  • File components/exo/wayland/zaura_output_manager.cc:

  • File ui/platform_window/platform_window_init_properties.h:

  • 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.

Gerrit-MessageType: comment
Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I486594789ec307bfcaf4ce81ba1c221c9bbeb93f
Gerrit-Change-Number: 4606716
Gerrit-PatchSet: 48
Gerrit-Owner: Mitsuru Oshima <osh...@chromium.org>
Gerrit-Reviewer: Mitsuru Oshima <osh...@chromium.org>
Gerrit-Reviewer: Thomas Lukaszewicz <tl...@chromium.org>
Gerrit-CC: Daniel Cheng <dch...@chromium.org>
Gerrit-Attention: Mitsuru Oshima <osh...@chromium.org>
Gerrit-Comment-Date: Tue, 15 Aug 2023 16:25:21 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No

Mitsuru Oshima (Gerrit)

unread,
Aug 17, 2023, 9:23:31 PM8/17/23
to ozone-...@chromium.org, cambickel+watc...@google.com, jimmyxgong+wat...@chromium.org, longbowei+watc...@google.com, michaelcheco+wa...@google.com, roblia...@chromium.org, sky+...@chromium.org, zentaro+watch...@chromium.org, zhangwenyu+wat...@google.com, Thomas Lukaszewicz, Tricium, Chromium LUCI CQ, chromium...@chromium.org, Daniel Cheng

Attention is currently required from: Thomas Lukaszewicz.

View Change

7 comments:

  • Patchset:

    • Patch Set #49:

      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:

    • 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:

    • Done

  • File components/exo/wayland/zaura_output_manager.cc:

    • 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:

    • Done

  • File ui/views/widget/native_widget_aura.cc:

    • 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:

    • 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.

Gerrit-MessageType: comment
Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I486594789ec307bfcaf4ce81ba1c221c9bbeb93f
Gerrit-Change-Number: 4606716
Gerrit-PatchSet: 49
Gerrit-Owner: Mitsuru Oshima <osh...@chromium.org>
Gerrit-Reviewer: Mitsuru Oshima <osh...@chromium.org>
Gerrit-Reviewer: Thomas Lukaszewicz <tl...@chromium.org>
Gerrit-CC: Daniel Cheng <dch...@chromium.org>
Gerrit-Attention: Thomas Lukaszewicz <tl...@chromium.org>
Gerrit-Comment-Date: Fri, 18 Aug 2023 01:23:23 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Thomas Lukaszewicz <tl...@chromium.org>

Mitsuru Oshima (Gerrit)

unread,
Aug 17, 2023, 9:27:52 PM8/17/23
to ozone-...@chromium.org, cambickel+watc...@google.com, jimmyxgong+wat...@chromium.org, longbowei+watc...@google.com, michaelcheco+wa...@google.com, roblia...@chromium.org, sky+...@chromium.org, zentaro+watch...@chromium.org, zhangwenyu+wat...@google.com, Thomas Lukaszewicz, Tricium, Chromium LUCI CQ, chromium...@chromium.org, Daniel Cheng

Attention is currently required from: Thomas Lukaszewicz.

Patch set 50:Commit-Queue +1

View Change

    To view, visit change 4606716. To unsubscribe, or for help writing mail filters, visit settings.

    Gerrit-MessageType: comment
    Gerrit-Project: chromium/src
    Gerrit-Branch: main
    Gerrit-Change-Id: I486594789ec307bfcaf4ce81ba1c221c9bbeb93f
    Gerrit-Change-Number: 4606716
    Gerrit-PatchSet: 50
    Gerrit-Owner: Mitsuru Oshima <osh...@chromium.org>
    Gerrit-Reviewer: Mitsuru Oshima <osh...@chromium.org>
    Gerrit-Reviewer: Thomas Lukaszewicz <tl...@chromium.org>
    Gerrit-CC: Daniel Cheng <dch...@chromium.org>
    Gerrit-Attention: Thomas Lukaszewicz <tl...@chromium.org>
    Gerrit-Comment-Date: Fri, 18 Aug 2023 01:27:44 +0000
    Gerrit-HasComments: No
    Gerrit-Has-Labels: Yes

    Mitsuru Oshima (Gerrit)

    unread,
    Aug 18, 2023, 10:24:43 AM8/18/23
    to ozone-...@chromium.org, cambickel+watc...@google.com, jimmyxgong+wat...@chromium.org, longbowei+watc...@google.com, michaelcheco+wa...@google.com, roblia...@chromium.org, sky+...@chromium.org, zentaro+watch...@chromium.org, zhangwenyu+wat...@google.com, Thomas Lukaszewicz, Tricium, Chromium LUCI CQ, chromium...@chromium.org, Daniel Cheng

    Attention is currently required from: Thomas Lukaszewicz.

    Patch set 51:Commit-Queue +1

    View Change

      To view, visit change 4606716. To unsubscribe, or for help writing mail filters, visit settings.

      Gerrit-MessageType: comment
      Gerrit-Project: chromium/src
      Gerrit-Branch: main
      Gerrit-Change-Id: I486594789ec307bfcaf4ce81ba1c221c9bbeb93f
      Gerrit-Change-Number: 4606716
      Gerrit-PatchSet: 51
      Gerrit-Owner: Mitsuru Oshima <osh...@chromium.org>
      Gerrit-Reviewer: Mitsuru Oshima <osh...@chromium.org>
      Gerrit-Reviewer: Thomas Lukaszewicz <tl...@chromium.org>
      Gerrit-CC: Daniel Cheng <dch...@chromium.org>
      Gerrit-Attention: Thomas Lukaszewicz <tl...@chromium.org>
      Gerrit-Comment-Date: Fri, 18 Aug 2023 14:24:31 +0000
      Gerrit-HasComments: No
      Gerrit-Has-Labels: Yes

      Mitsuru Oshima (Gerrit)

      unread,
      Aug 18, 2023, 10:18:12 PM8/18/23
      to ozone-...@chromium.org, cambickel+watc...@google.com, jimmyxgong+wat...@chromium.org, longbowei+watc...@google.com, michaelcheco+wa...@google.com, roblia...@chromium.org, sky+...@chromium.org, zentaro+watch...@chromium.org, zhangwenyu+wat...@google.com, Thomas Lukaszewicz, Tricium, Chromium LUCI CQ, chromium...@chromium.org, Daniel Cheng
      Gerrit-Comment-Date: Sat, 19 Aug 2023 02:17:59 +0000
      Gerrit-HasComments: No
      Gerrit-Has-Labels: Yes

      Mitsuru Oshima (Gerrit)

      unread,
      Aug 18, 2023, 11:15:57 PM8/18/23
      to crostin...@chromium.org, oshima...@chromium.org, yhanada+...@chromium.org, ozone-...@chromium.org, cambickel+watc...@google.com, jimmyxgong+wat...@chromium.org, longbowei+watc...@google.com, michaelcheco+wa...@google.com, roblia...@chromium.org, sky+...@chromium.org, zentaro+watch...@chromium.org, zhangwenyu+wat...@google.com, Thomas Lukaszewicz, Tricium, Chromium LUCI CQ, chromium...@chromium.org, Daniel Cheng

      Attention is currently required from: Thomas Lukaszewicz.

      Patch set 52:Auto-Submit +1

      View Change

        To view, visit change 4606716. To unsubscribe, or for help writing mail filters, visit settings.

        Gerrit-MessageType: comment
        Gerrit-Project: chromium/src
        Gerrit-Branch: main
        Gerrit-Change-Id: I486594789ec307bfcaf4ce81ba1c221c9bbeb93f
        Gerrit-Change-Number: 4606716
        Gerrit-PatchSet: 52
        Gerrit-Owner: Mitsuru Oshima <osh...@chromium.org>
        Gerrit-Reviewer: Mitsuru Oshima <osh...@chromium.org>
        Gerrit-Reviewer: Thomas Lukaszewicz <tl...@chromium.org>
        Gerrit-CC: Daniel Cheng <dch...@chromium.org>
        Gerrit-Attention: Thomas Lukaszewicz <tl...@chromium.org>
        Gerrit-Comment-Date: Sat, 19 Aug 2023 03:15:50 +0000
        Gerrit-HasComments: No
        Gerrit-Has-Labels: Yes

        Mitsuru Oshima (Gerrit)

        unread,
        Aug 19, 2023, 12:45:34 AM8/19/23
        to crostin...@chromium.org, oshima...@chromium.org, yhanada+...@chromium.org, ozone-...@chromium.org, cambickel+watc...@google.com, jimmyxgong+wat...@chromium.org, longbowei+watc...@google.com, michaelcheco+wa...@google.com, roblia...@chromium.org, sky+...@chromium.org, zentaro+watch...@chromium.org, zhangwenyu+wat...@google.com, Thomas Lukaszewicz, Tricium, Chromium LUCI CQ, chromium...@chromium.org, Daniel Cheng

        Attention is currently required from: Thomas Lukaszewicz.

        Patch set 52:-Auto-SubmitCommit-Queue +1

        View Change

          To view, visit change 4606716. To unsubscribe, or for help writing mail filters, visit settings.

          Gerrit-MessageType: comment
          Gerrit-Project: chromium/src
          Gerrit-Branch: main
          Gerrit-Change-Id: I486594789ec307bfcaf4ce81ba1c221c9bbeb93f
          Gerrit-Change-Number: 4606716
          Gerrit-PatchSet: 52
          Gerrit-Owner: Mitsuru Oshima <osh...@chromium.org>
          Gerrit-Reviewer: Mitsuru Oshima <osh...@chromium.org>
          Gerrit-Reviewer: Thomas Lukaszewicz <tl...@chromium.org>
          Gerrit-CC: Daniel Cheng <dch...@chromium.org>
          Gerrit-Attention: Thomas Lukaszewicz <tl...@chromium.org>
          Gerrit-Comment-Date: Sat, 19 Aug 2023 04:45:21 +0000
          Gerrit-HasComments: No
          Gerrit-Has-Labels: Yes

          Mitsuru Oshima (Gerrit)

          unread,
          Aug 19, 2023, 1:10:07 AM8/19/23
          to crostin...@chromium.org, oshima...@chromium.org, yhanada+...@chromium.org, ozone-...@chromium.org, cambickel+watc...@google.com, jimmyxgong+wat...@chromium.org, longbowei+watc...@google.com, michaelcheco+wa...@google.com, roblia...@chromium.org, sky+...@chromium.org, zentaro+watch...@chromium.org, zhangwenyu+wat...@google.com, Thomas Lukaszewicz, Tricium, Chromium LUCI CQ, chromium...@chromium.org, Daniel Cheng

          Attention is currently required from: Thomas Lukaszewicz.

          Patch set 53:Commit-Queue +1

          View Change

            To view, visit change 4606716. To unsubscribe, or for help writing mail filters, visit settings.

            Gerrit-MessageType: comment
            Gerrit-Project: chromium/src
            Gerrit-Branch: main
            Gerrit-Change-Id: I486594789ec307bfcaf4ce81ba1c221c9bbeb93f
            Gerrit-Change-Number: 4606716
            Gerrit-PatchSet: 53
            Gerrit-Owner: Mitsuru Oshima <osh...@chromium.org>
            Gerrit-Reviewer: Mitsuru Oshima <osh...@chromium.org>
            Gerrit-Reviewer: Thomas Lukaszewicz <tl...@chromium.org>
            Gerrit-CC: Daniel Cheng <dch...@chromium.org>
            Gerrit-Attention: Thomas Lukaszewicz <tl...@chromium.org>
            Gerrit-Comment-Date: Sat, 19 Aug 2023 05:09:55 +0000
            Gerrit-HasComments: No
            Gerrit-Has-Labels: Yes

            Mitsuru Oshima (Gerrit)

            unread,
            Aug 20, 2023, 4:49:20 AM8/20/23
            to crostin...@chromium.org, oshima...@chromium.org, yhanada+...@chromium.org, ozone-...@chromium.org, cambickel+watc...@google.com, jimmyxgong+wat...@chromium.org, longbowei+watc...@google.com, michaelcheco+wa...@google.com, roblia...@chromium.org, sky+...@chromium.org, zentaro+watch...@chromium.org, zhangwenyu+wat...@google.com, Thomas Lukaszewicz, Tricium, Chromium LUCI CQ, chromium...@chromium.org, Daniel Cheng

            Attention is currently required from: Thomas Lukaszewicz.

            Patch set 54:Commit-Queue +1

            View Change

              To view, visit change 4606716. To unsubscribe, or for help writing mail filters, visit settings.

              Gerrit-MessageType: comment
              Gerrit-Project: chromium/src
              Gerrit-Branch: main
              Gerrit-Change-Id: I486594789ec307bfcaf4ce81ba1c221c9bbeb93f
              Gerrit-Change-Number: 4606716
              Gerrit-PatchSet: 54
              Gerrit-Owner: Mitsuru Oshima <osh...@chromium.org>
              Gerrit-Reviewer: Mitsuru Oshima <osh...@chromium.org>
              Gerrit-Reviewer: Thomas Lukaszewicz <tl...@chromium.org>
              Gerrit-CC: Daniel Cheng <dch...@chromium.org>
              Gerrit-Attention: Thomas Lukaszewicz <tl...@chromium.org>
              Gerrit-Comment-Date: Sun, 20 Aug 2023 08:49:12 +0000
              Gerrit-HasComments: No
              Gerrit-Has-Labels: Yes

              Mitsuru Oshima (Gerrit)

              unread,
              Aug 21, 2023, 1:32:56 AM8/21/23
              to alexmo...@chromium.org, bici...@google.com, creis...@chromium.org, headless...@chromium.org, jbauma...@chromium.org, keithle...@chromium.org, navigation...@chromium.org, nona+...@chromium.org, shuche...@chromium.org, tranbaod...@chromium.org, yhanad...@chromium.org, crostin...@chromium.org, oshima...@chromium.org, yhanada+...@chromium.org, ozone-...@chromium.org, cambickel+watc...@google.com, jimmyxgong+wat...@chromium.org, longbowei+watc...@google.com, michaelcheco+wa...@google.com, roblia...@chromium.org, sky+...@chromium.org, zentaro+watch...@chromium.org, zhangwenyu+wat...@google.com, Daniel Andersson, Sadrul Chowdhury, James Su, Thomas Lukaszewicz, Tricium, Chromium LUCI CQ, chromium...@chromium.org, Daniel Cheng

              Attention is currently required from: Thomas Lukaszewicz.

              Patch set 56:Commit-Queue +1

              View Change

                To view, visit change 4606716. To unsubscribe, or for help writing mail filters, visit settings.

                Gerrit-MessageType: comment
                Gerrit-Project: chromium/src
                Gerrit-Branch: main
                Gerrit-Change-Id: I486594789ec307bfcaf4ce81ba1c221c9bbeb93f
                Gerrit-Change-Number: 4606716
                Gerrit-PatchSet: 56
                Gerrit-Owner: Mitsuru Oshima <osh...@chromium.org>
                Gerrit-Reviewer: Mitsuru Oshima <osh...@chromium.org>
                Gerrit-Reviewer: Thomas Lukaszewicz <tl...@chromium.org>
                Gerrit-CC: Daniel Andersson <dande...@chromium.org>
                Gerrit-CC: Daniel Cheng <dch...@chromium.org>
                Gerrit-CC: James Su <su...@chromium.org>
                Gerrit-CC: Sadrul Chowdhury <sad...@chromium.org>
                Gerrit-Attention: Thomas Lukaszewicz <tl...@chromium.org>
                Gerrit-Comment-Date: Mon, 21 Aug 2023 05:32:48 +0000
                Gerrit-HasComments: No
                Gerrit-Has-Labels: Yes

                Mitsuru Oshima (Gerrit)

                unread,
                Aug 21, 2023, 9:26:24 AM8/21/23
                to alexmo...@chromium.org, bici...@google.com, creis...@chromium.org, headless...@chromium.org, jbauma...@chromium.org, keithle...@chromium.org, navigation...@chromium.org, nona+...@chromium.org, shuche...@chromium.org, tranbaod...@chromium.org, yhanad...@chromium.org, crostin...@chromium.org, oshima...@chromium.org, yhanada+...@chromium.org, ozone-...@chromium.org, cambickel+watc...@google.com, jimmyxgong+wat...@chromium.org, longbowei+watc...@google.com, michaelcheco+wa...@google.com, roblia...@chromium.org, sky+...@chromium.org, zentaro+watch...@chromium.org, zhangwenyu+wat...@google.com, Daniel Andersson, Sadrul Chowdhury, James Su, Thomas Lukaszewicz, Tricium, Chromium LUCI CQ, chromium...@chromium.org, Daniel Cheng

                Attention is currently required from: Thomas Lukaszewicz.

                Patch set 57:Commit-Queue +1

                View Change

                  To view, visit change 4606716. To unsubscribe, or for help writing mail filters, visit settings.

                  Gerrit-MessageType: comment
                  Gerrit-Project: chromium/src
                  Gerrit-Branch: main
                  Gerrit-Change-Id: I486594789ec307bfcaf4ce81ba1c221c9bbeb93f
                  Gerrit-Change-Number: 4606716
                  Gerrit-PatchSet: 57
                  Gerrit-Owner: Mitsuru Oshima <osh...@chromium.org>
                  Gerrit-Reviewer: Mitsuru Oshima <osh...@chromium.org>
                  Gerrit-Reviewer: Thomas Lukaszewicz <tl...@chromium.org>
                  Gerrit-CC: Daniel Andersson <dande...@chromium.org>
                  Gerrit-CC: Daniel Cheng <dch...@chromium.org>
                  Gerrit-CC: James Su <su...@chromium.org>
                  Gerrit-CC: Sadrul Chowdhury <sad...@chromium.org>
                  Gerrit-Attention: Thomas Lukaszewicz <tl...@chromium.org>
                  Gerrit-Comment-Date: Mon, 21 Aug 2023 13:26:15 +0000
                  Gerrit-HasComments: No
                  Gerrit-Has-Labels: Yes

                  Mitsuru Oshima (Gerrit)

                  unread,
                  Aug 21, 2023, 9:48:51 AM8/21/23
                  to alexmo...@chromium.org, bici...@google.com, creis...@chromium.org, headless...@chromium.org, jbauma...@chromium.org, keithle...@chromium.org, navigation...@chromium.org, nona+...@chromium.org, shuche...@chromium.org, tranbaod...@chromium.org, yhanad...@chromium.org, crostin...@chromium.org, oshima...@chromium.org, yhanada+...@chromium.org, ozone-...@chromium.org, cambickel+watc...@google.com, jimmyxgong+wat...@chromium.org, longbowei+watc...@google.com, michaelcheco+wa...@google.com, roblia...@chromium.org, sky+...@chromium.org, zentaro+watch...@chromium.org, zhangwenyu+wat...@google.com, Daniel Andersson, Sadrul Chowdhury, James Su, Thomas Lukaszewicz, Tricium, Chromium LUCI CQ, chromium...@chromium.org, Daniel Cheng

                  Attention is currently required from: Thomas Lukaszewicz.

                  Patch set 58:Commit-Queue +1

                  View Change

                    To view, visit change 4606716. To unsubscribe, or for help writing mail filters, visit settings.

                    Gerrit-MessageType: comment
                    Gerrit-Project: chromium/src
                    Gerrit-Branch: main
                    Gerrit-Change-Id: I486594789ec307bfcaf4ce81ba1c221c9bbeb93f
                    Gerrit-Change-Number: 4606716
                    Gerrit-PatchSet: 58
                    Gerrit-Owner: Mitsuru Oshima <osh...@chromium.org>
                    Gerrit-Reviewer: Mitsuru Oshima <osh...@chromium.org>
                    Gerrit-Reviewer: Thomas Lukaszewicz <tl...@chromium.org>
                    Gerrit-CC: Daniel Andersson <dande...@chromium.org>
                    Gerrit-CC: Daniel Cheng <dch...@chromium.org>
                    Gerrit-CC: James Su <su...@chromium.org>
                    Gerrit-CC: Sadrul Chowdhury <sad...@chromium.org>
                    Gerrit-Attention: Thomas Lukaszewicz <tl...@chromium.org>
                    Gerrit-Comment-Date: Mon, 21 Aug 2023 13:48:41 +0000
                    Gerrit-HasComments: No
                    Gerrit-Has-Labels: Yes

                    Mitsuru Oshima (Gerrit)

                    unread,
                    Aug 21, 2023, 10:20:15 AM8/21/23
                    to alexmo...@chromium.org, bici...@google.com, creis...@chromium.org, headless...@chromium.org, jbauma...@chromium.org, keithle...@chromium.org, navigation...@chromium.org, nona+...@chromium.org, shuche...@chromium.org, tranbaod...@chromium.org, yhanad...@chromium.org, crostin...@chromium.org, oshima...@chromium.org, yhanada+...@chromium.org, ozone-...@chromium.org, cambickel+watc...@google.com, jimmyxgong+wat...@chromium.org, longbowei+watc...@google.com, michaelcheco+wa...@google.com, roblia...@chromium.org, sky+...@chromium.org, zentaro+watch...@chromium.org, zhangwenyu+wat...@google.com, Daniel Andersson, Sadrul Chowdhury, James Su, Thomas Lukaszewicz, Tricium, Chromium LUCI CQ, chromium...@chromium.org, Daniel Cheng

                    Attention is currently required from: Thomas Lukaszewicz.

                    Patch set 59:Commit-Queue +1

                    View Change

                      To view, visit change 4606716. To unsubscribe, or for help writing mail filters, visit settings.

                      Gerrit-MessageType: comment
                      Gerrit-Project: chromium/src
                      Gerrit-Branch: main
                      Gerrit-Change-Id: I486594789ec307bfcaf4ce81ba1c221c9bbeb93f
                      Gerrit-Change-Number: 4606716
                      Gerrit-PatchSet: 59
                      Gerrit-Owner: Mitsuru Oshima <osh...@chromium.org>
                      Gerrit-Reviewer: Mitsuru Oshima <osh...@chromium.org>
                      Gerrit-Reviewer: Thomas Lukaszewicz <tl...@chromium.org>
                      Gerrit-CC: Daniel Andersson <dande...@chromium.org>
                      Gerrit-CC: Daniel Cheng <dch...@chromium.org>
                      Gerrit-CC: James Su <su...@chromium.org>
                      Gerrit-CC: Sadrul Chowdhury <sad...@chromium.org>
                      Gerrit-Attention: Thomas Lukaszewicz <tl...@chromium.org>
                      Gerrit-Comment-Date: Mon, 21 Aug 2023 14:20:06 +0000
                      Gerrit-HasComments: No
                      Gerrit-Has-Labels: Yes

                      Mitsuru Oshima (Gerrit)

                      unread,
                      Aug 21, 2023, 3:28:06 PM8/21/23
                      to alexmo...@chromium.org, bici...@google.com, creis...@chromium.org, headless...@chromium.org, jbauma...@chromium.org, keithle...@chromium.org, navigation...@chromium.org, nona+...@chromium.org, shuche...@chromium.org, tranbaod...@chromium.org, yhanad...@chromium.org, crostin...@chromium.org, oshima...@chromium.org, yhanada+...@chromium.org, ozone-...@chromium.org, cambickel+watc...@google.com, jimmyxgong+wat...@chromium.org, longbowei+watc...@google.com, michaelcheco+wa...@google.com, roblia...@chromium.org, sky+...@chromium.org, zentaro+watch...@chromium.org, zhangwenyu+wat...@google.com, Daniel Andersson, Sadrul Chowdhury, James Su, Thomas Lukaszewicz, Tricium, Chromium LUCI CQ, chromium...@chromium.org, Daniel Cheng

                      Attention is currently required from: Thomas Lukaszewicz.

                      Patch set 61:Commit-Queue +1

                      View Change

                        To view, visit change 4606716. To unsubscribe, or for help writing mail filters, visit settings.

                        Gerrit-MessageType: comment
                        Gerrit-Project: chromium/src
                        Gerrit-Branch: main
                        Gerrit-Change-Id: I486594789ec307bfcaf4ce81ba1c221c9bbeb93f
                        Gerrit-Change-Number: 4606716
                        Gerrit-PatchSet: 61
                        Gerrit-Owner: Mitsuru Oshima <osh...@chromium.org>
                        Gerrit-Reviewer: Mitsuru Oshima <osh...@chromium.org>
                        Gerrit-Reviewer: Thomas Lukaszewicz <tl...@chromium.org>
                        Gerrit-CC: Daniel Andersson <dande...@chromium.org>
                        Gerrit-CC: Daniel Cheng <dch...@chromium.org>
                        Gerrit-CC: James Su <su...@chromium.org>
                        Gerrit-CC: Sadrul Chowdhury <sad...@chromium.org>
                        Gerrit-Attention: Thomas Lukaszewicz <tl...@chromium.org>
                        Gerrit-Comment-Date: Mon, 21 Aug 2023 19:27:58 +0000
                        Gerrit-HasComments: No
                        Gerrit-Has-Labels: Yes

                        Mitsuru Oshima (Gerrit)

                        unread,
                        Aug 21, 2023, 3:52:32 PM8/21/23
                        to alexmo...@chromium.org, bici...@google.com, creis...@chromium.org, headless...@chromium.org, jbauma...@chromium.org, keithle...@chromium.org, navigation...@chromium.org, nona+...@chromium.org, shuche...@chromium.org, tranbaod...@chromium.org, yhanad...@chromium.org, crostin...@chromium.org, oshima...@chromium.org, yhanada+...@chromium.org, ozone-...@chromium.org, cambickel+watc...@google.com, jimmyxgong+wat...@chromium.org, longbowei+watc...@google.com, michaelcheco+wa...@google.com, roblia...@chromium.org, sky+...@chromium.org, zentaro+watch...@chromium.org, zhangwenyu+wat...@google.com, Daniel Andersson, Sadrul Chowdhury, James Su, Thomas Lukaszewicz, Tricium, Chromium LUCI CQ, chromium...@chromium.org, Daniel Cheng

                        Attention is currently required from: Thomas Lukaszewicz.

                        Patch set 62:Commit-Queue +1

                        View Change

                          To view, visit change 4606716. To unsubscribe, or for help writing mail filters, visit settings.

                          Gerrit-MessageType: comment
                          Gerrit-Project: chromium/src
                          Gerrit-Branch: main
                          Gerrit-Change-Id: I486594789ec307bfcaf4ce81ba1c221c9bbeb93f
                          Gerrit-Change-Number: 4606716
                          Gerrit-PatchSet: 62
                          Gerrit-Owner: Mitsuru Oshima <osh...@chromium.org>
                          Gerrit-Reviewer: Mitsuru Oshima <osh...@chromium.org>
                          Gerrit-Reviewer: Thomas Lukaszewicz <tl...@chromium.org>
                          Gerrit-CC: Daniel Andersson <dande...@chromium.org>
                          Gerrit-CC: Daniel Cheng <dch...@chromium.org>
                          Gerrit-CC: James Su <su...@chromium.org>
                          Gerrit-CC: Sadrul Chowdhury <sad...@chromium.org>
                          Gerrit-Attention: Thomas Lukaszewicz <tl...@chromium.org>
                          Gerrit-Comment-Date: Mon, 21 Aug 2023 19:52:23 +0000
                          Gerrit-HasComments: No
                          Gerrit-Has-Labels: Yes

                          Mitsuru Oshima (Gerrit)

                          unread,
                          Aug 21, 2023, 8:05:22 PM8/21/23
                          to alexmo...@chromium.org, bici...@google.com, creis...@chromium.org, headless...@chromium.org, jbauma...@chromium.org, keithle...@chromium.org, navigation...@chromium.org, nona+...@chromium.org, shuche...@chromium.org, tranbaod...@chromium.org, yhanad...@chromium.org, crostin...@chromium.org, oshima...@chromium.org, yhanada+...@chromium.org, ozone-...@chromium.org, cambickel+watc...@google.com, jimmyxgong+wat...@chromium.org, longbowei+watc...@google.com, michaelcheco+wa...@google.com, roblia...@chromium.org, sky+...@chromium.org, zentaro+watch...@chromium.org, zhangwenyu+wat...@google.com, Daniel Andersson, Sadrul Chowdhury, James Su, Thomas Lukaszewicz, Tricium, Chromium LUCI CQ, chromium...@chromium.org, Daniel Cheng

                          Attention is currently required from: Thomas Lukaszewicz.

                          View Change

                          1 comment:

                          • Patchset:

                            • Patch Set #65:

                              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.

                          Gerrit-MessageType: comment
                          Gerrit-Project: chromium/src
                          Gerrit-Branch: main
                          Gerrit-Change-Id: I486594789ec307bfcaf4ce81ba1c221c9bbeb93f
                          Gerrit-Change-Number: 4606716
                          Gerrit-PatchSet: 65
                          Gerrit-Owner: Mitsuru Oshima <osh...@chromium.org>
                          Gerrit-Reviewer: Mitsuru Oshima <osh...@chromium.org>
                          Gerrit-Reviewer: Thomas Lukaszewicz <tl...@chromium.org>
                          Gerrit-CC: Daniel Andersson <dande...@chromium.org>
                          Gerrit-CC: Daniel Cheng <dch...@chromium.org>
                          Gerrit-CC: James Su <su...@chromium.org>
                          Gerrit-CC: Sadrul Chowdhury <sad...@chromium.org>
                          Gerrit-Attention: Thomas Lukaszewicz <tl...@chromium.org>
                          Gerrit-Comment-Date: Tue, 22 Aug 2023 00:05:15 +0000
                          Gerrit-HasComments: Yes
                          Gerrit-Has-Labels: No

                          Mitsuru Oshima (Gerrit)

                          unread,
                          Aug 21, 2023, 8:05:54 PM8/21/23
                          to alexmo...@chromium.org, bici...@google.com, creis...@chromium.org, headless...@chromium.org, jbauma...@chromium.org, keithle...@chromium.org, navigation...@chromium.org, nona+...@chromium.org, shuche...@chromium.org, tranbaod...@chromium.org, yhanad...@chromium.org, crostin...@chromium.org, oshima...@chromium.org, yhanada+...@chromium.org, ozone-...@chromium.org, cambickel+watc...@google.com, jimmyxgong+wat...@chromium.org, longbowei+watc...@google.com, michaelcheco+wa...@google.com, roblia...@chromium.org, sky+...@chromium.org, zentaro+watch...@chromium.org, zhangwenyu+wat...@google.com, Daniel Andersson, Sadrul Chowdhury, James Su, Thomas Lukaszewicz, Tricium, Chromium LUCI CQ, chromium...@chromium.org, Daniel Cheng

                          Attention is currently required from: Thomas Lukaszewicz.

                          View Change

                          1 comment:

                          • Patchset:

                            • Patch Set #65:

                              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.

                          Gerrit-MessageType: comment
                          Gerrit-Project: chromium/src
                          Gerrit-Branch: main
                          Gerrit-Change-Id: I486594789ec307bfcaf4ce81ba1c221c9bbeb93f
                          Gerrit-Change-Number: 4606716
                          Gerrit-PatchSet: 65
                          Gerrit-Owner: Mitsuru Oshima <osh...@chromium.org>
                          Gerrit-Reviewer: Mitsuru Oshima <osh...@chromium.org>
                          Gerrit-Reviewer: Thomas Lukaszewicz <tl...@chromium.org>
                          Gerrit-CC: Daniel Andersson <dande...@chromium.org>
                          Gerrit-CC: Daniel Cheng <dch...@chromium.org>
                          Gerrit-CC: James Su <su...@chromium.org>
                          Gerrit-CC: Sadrul Chowdhury <sad...@chromium.org>
                          Gerrit-Attention: Thomas Lukaszewicz <tl...@chromium.org>
                          Gerrit-Comment-Date: Tue, 22 Aug 2023 00:05:47 +0000
                          Gerrit-HasComments: Yes
                          Gerrit-Has-Labels: No

                          Thomas Lukaszewicz (Gerrit)

                          unread,
                          Aug 22, 2023, 8:53:45 PM8/22/23
                          to Mitsuru Oshima, alexmo...@chromium.org, bici...@google.com, creis...@chromium.org, headless...@chromium.org, jbauma...@chromium.org, keithle...@chromium.org, navigation...@chromium.org, nona+...@chromium.org, shuche...@chromium.org, tranbaod...@chromium.org, yhanad...@chromium.org, crostin...@chromium.org, oshima...@chromium.org, yhanada+...@chromium.org, ozone-...@chromium.org, cambickel+watc...@google.com, jimmyxgong+wat...@chromium.org, longbowei+watc...@google.com, michaelcheco+wa...@google.com, roblia...@chromium.org, sky+...@chromium.org, zentaro+watch...@chromium.org, zhangwenyu+wat...@google.com, Daniel Andersson, Sadrul Chowdhury, James Su, Tricium, Chromium LUCI CQ, chromium...@chromium.org, Daniel Cheng

                          Attention is currently required from: Mitsuru Oshima.

                          View Change

                          1 comment:

                          • Commit Message:

                            • Patch Set #65, Line 9: * Intrduce display_id parameter in Browser::CreateParams/Widget::InitParams

                              High 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.

                              [1] https://source.chromium.org/chromium/chromium/src/+/main:ui/views/widget/widget.h;l=388;drc=092fe7b3d8c8e236f5b3ac48c6f9047508c03cdf

                          To view, visit change 4606716. To unsubscribe, or for help writing mail filters, visit settings.

                          Gerrit-MessageType: comment
                          Gerrit-Project: chromium/src
                          Gerrit-Branch: main
                          Gerrit-Change-Id: I486594789ec307bfcaf4ce81ba1c221c9bbeb93f
                          Gerrit-Change-Number: 4606716
                          Gerrit-PatchSet: 65
                          Gerrit-Owner: Mitsuru Oshima <osh...@chromium.org>
                          Gerrit-Reviewer: Mitsuru Oshima <osh...@chromium.org>
                          Gerrit-Reviewer: Thomas Lukaszewicz <tl...@chromium.org>
                          Gerrit-CC: Daniel Andersson <dande...@chromium.org>
                          Gerrit-CC: Daniel Cheng <dch...@chromium.org>
                          Gerrit-CC: James Su <su...@chromium.org>
                          Gerrit-CC: Sadrul Chowdhury <sad...@chromium.org>
                          Gerrit-Attention: Mitsuru Oshima <osh...@chromium.org>
                          Gerrit-Comment-Date: Wed, 23 Aug 2023 00:53:32 +0000
                          Gerrit-HasComments: Yes
                          Gerrit-Has-Labels: No

                          Mitsuru Oshima (Gerrit)

                          unread,
                          Aug 22, 2023, 10:50:39 PM8/22/23
                          to alexmo...@chromium.org, bici...@google.com, creis...@chromium.org, headless...@chromium.org, jbauma...@chromium.org, keithle...@chromium.org, navigation...@chromium.org, nona+...@chromium.org, shuche...@chromium.org, tranbaod...@chromium.org, yhanad...@chromium.org, crostin...@chromium.org, oshima...@chromium.org, yhanada+...@chromium.org, ozone-...@chromium.org, cambickel+watc...@google.com, jimmyxgong+wat...@chromium.org, longbowei+watc...@google.com, michaelcheco+wa...@google.com, roblia...@chromium.org, sky+...@chromium.org, zentaro+watch...@chromium.org, zhangwenyu+wat...@google.com, Daniel Andersson, Sadrul Chowdhury, James Su, Thomas Lukaszewicz, Tricium, Chromium LUCI CQ, chromium...@chromium.org, Daniel Cheng

                          Attention is currently required from: Thomas Lukaszewicz.

                          View Change

                          1 comment:

                          • Commit Message:

                            • 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.

                          Gerrit-MessageType: comment
                          Gerrit-Project: chromium/src
                          Gerrit-Branch: main
                          Gerrit-Change-Id: I486594789ec307bfcaf4ce81ba1c221c9bbeb93f
                          Gerrit-Change-Number: 4606716
                          Gerrit-PatchSet: 65
                          Gerrit-Owner: Mitsuru Oshima <osh...@chromium.org>
                          Gerrit-Reviewer: Mitsuru Oshima <osh...@chromium.org>
                          Gerrit-Reviewer: Thomas Lukaszewicz <tl...@chromium.org>
                          Gerrit-CC: Daniel Andersson <dande...@chromium.org>
                          Gerrit-CC: Daniel Cheng <dch...@chromium.org>
                          Gerrit-CC: James Su <su...@chromium.org>
                          Gerrit-CC: Sadrul Chowdhury <sad...@chromium.org>
                          Gerrit-Attention: Thomas Lukaszewicz <tl...@chromium.org>
                          Gerrit-Comment-Date: Wed, 23 Aug 2023 02:50:27 +0000
                          Gerrit-HasComments: Yes
                          Gerrit-Has-Labels: No
                          Comment-In-Reply-To: Thomas Lukaszewicz <tl...@chromium.org>

                          Thomas Lukaszewicz (Gerrit)

                          unread,
                          Aug 23, 2023, 12:18:04 AM8/23/23
                          to Mitsuru Oshima, alexmo...@chromium.org, bici...@google.com, creis...@chromium.org, headless...@chromium.org, jbauma...@chromium.org, keithle...@chromium.org, navigation...@chromium.org, nona+...@chromium.org, shuche...@chromium.org, tranbaod...@chromium.org, yhanad...@chromium.org, crostin...@chromium.org, oshima...@chromium.org, yhanada+...@chromium.org, ozone-...@chromium.org, cambickel+watc...@google.com, jimmyxgong+wat...@chromium.org, longbowei+watc...@google.com, michaelcheco+wa...@google.com, roblia...@chromium.org, sky+...@chromium.org, zentaro+watch...@chromium.org, zhangwenyu+wat...@google.com, Daniel Andersson, Sadrul Chowdhury, James Su, Tricium, Chromium LUCI CQ, chromium...@chromium.org, Daniel Cheng

                          Attention is currently required from: Mitsuru Oshima.

                          View Change

                          4 comments:

                          • Commit Message:

                            • 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:

                            • Patch Set #65, Line 144:

                                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:

                            • Patch Set #65, Line 243:


                              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.

                          Gerrit-MessageType: comment
                          Gerrit-Project: chromium/src
                          Gerrit-Branch: main
                          Gerrit-Change-Id: I486594789ec307bfcaf4ce81ba1c221c9bbeb93f
                          Gerrit-Change-Number: 4606716
                          Gerrit-PatchSet: 65
                          Gerrit-Owner: Mitsuru Oshima <osh...@chromium.org>
                          Gerrit-Reviewer: Mitsuru Oshima <osh...@chromium.org>
                          Gerrit-Reviewer: Thomas Lukaszewicz <tl...@chromium.org>
                          Gerrit-CC: Daniel Andersson <dande...@chromium.org>
                          Gerrit-CC: Daniel Cheng <dch...@chromium.org>
                          Gerrit-CC: James Su <su...@chromium.org>
                          Gerrit-CC: Sadrul Chowdhury <sad...@chromium.org>
                          Gerrit-Attention: Mitsuru Oshima <osh...@chromium.org>
                          Gerrit-Comment-Date: Wed, 23 Aug 2023 04:17:54 +0000
                          Gerrit-HasComments: Yes
                          Gerrit-Has-Labels: No
                          Comment-In-Reply-To: Thomas Lukaszewicz <tl...@chromium.org>
                          Comment-In-Reply-To: Mitsuru Oshima <osh...@chromium.org>

                          Mitsuru Oshima (Gerrit)

                          unread,
                          Sep 7, 2023, 3:45:33 PM9/7/23
                          to alexmo...@chromium.org, bici...@google.com, creis...@chromium.org, headless...@chromium.org, jbauma...@chromium.org, keithle...@chromium.org, navigation...@chromium.org, nona+...@chromium.org, shuche...@chromium.org, tranbaod...@chromium.org, yhanad...@chromium.org, crostin...@chromium.org, oshima...@chromium.org, yhanada+...@chromium.org, ozone-...@chromium.org, cambickel+watc...@google.com, jimmyxgong+wat...@chromium.org, longbowei+watc...@google.com, michaelcheco+wa...@google.com, roblia...@chromium.org, sky+...@chromium.org, zentaro+watch...@chromium.org, zhangwenyu+wat...@google.com, Daniel Andersson, Sadrul Chowdhury, James Su, Thomas Lukaszewicz, Tricium, Chromium LUCI CQ, chromium...@chromium.org, Daniel Cheng

                          Attention is currently required from: Thomas Lukaszewicz.

                          Patch set 67:Commit-Queue +1

                          View Change

                          3 comments:

                          • File ash/wm/container_finder.h:

                            • 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:

                            • 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:

                            • 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.

                          Gerrit-MessageType: comment
                          Gerrit-Project: chromium/src
                          Gerrit-Branch: main
                          Gerrit-Change-Id: I486594789ec307bfcaf4ce81ba1c221c9bbeb93f
                          Gerrit-Change-Number: 4606716
                          Gerrit-PatchSet: 67
                          Gerrit-Owner: Mitsuru Oshima <osh...@chromium.org>
                          Gerrit-Reviewer: Mitsuru Oshima <osh...@chromium.org>
                          Gerrit-Reviewer: Thomas Lukaszewicz <tl...@chromium.org>
                          Gerrit-CC: Daniel Andersson <dande...@chromium.org>
                          Gerrit-CC: Daniel Cheng <dch...@chromium.org>
                          Gerrit-CC: James Su <su...@chromium.org>
                          Gerrit-CC: Sadrul Chowdhury <sad...@chromium.org>
                          Gerrit-Attention: Thomas Lukaszewicz <tl...@chromium.org>
                          Gerrit-Comment-Date: Thu, 07 Sep 2023 19:45:21 +0000
                          Gerrit-HasComments: Yes
                          Gerrit-Has-Labels: Yes
                          Comment-In-Reply-To: Thomas Lukaszewicz <tl...@chromium.org>

                          Thomas Lukaszewicz (Gerrit)

                          unread,
                          Sep 8, 2023, 1:07:00 PM9/8/23
                          to Mitsuru Oshima, alexmo...@chromium.org, bici...@google.com, creis...@chromium.org, headless...@chromium.org, jbauma...@chromium.org, keithle...@chromium.org, navigation...@chromium.org, nona+...@chromium.org, shuche...@chromium.org, tranbaod...@chromium.org, yhanad...@chromium.org, crostin...@chromium.org, oshima...@chromium.org, yhanada+...@chromium.org, ozone-...@chromium.org, cambickel+watc...@google.com, jimmyxgong+wat...@chromium.org, longbowei+watc...@google.com, michaelcheco+wa...@google.com, roblia...@chromium.org, sky+...@chromium.org, zentaro+watch...@chromium.org, zhangwenyu+wat...@google.com, Daniel Andersson, Sadrul Chowdhury, James Su, Tricium, Chromium LUCI CQ, chromium...@chromium.org, Daniel Cheng

                          Attention is currently required from: Mitsuru Oshima.

                          View Change

                          14 comments:

                          • Patchset:

                            • Patch Set #67:

                              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:

                            • Ah yes I remember you mentioning this at the last sync, thanks for the background. […]

                              Done

                          • File ash/wm/container_finder.h:

                            • I think the comment is wrong or obsolete. I updated the comment.

                              Thanks this is much clearer.

                          • File components/exo/shell_surface_base.h:

                          • File components/exo/shell_surface_base.cc:

                            • 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:

                          • File components/exo/wayland/zaura_output_manager.cc:

                            • 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::kInvalidDisplayId

                              nit: 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:

                            • 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:

                            • 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:

                            • 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.

                          Gerrit-MessageType: comment
                          Gerrit-Project: chromium/src
                          Gerrit-Branch: main
                          Gerrit-Change-Id: I486594789ec307bfcaf4ce81ba1c221c9bbeb93f
                          Gerrit-Change-Number: 4606716
                          Gerrit-PatchSet: 67
                          Gerrit-Owner: Mitsuru Oshima <osh...@chromium.org>
                          Gerrit-Reviewer: Mitsuru Oshima <osh...@chromium.org>
                          Gerrit-Reviewer: Thomas Lukaszewicz <tl...@chromium.org>
                          Gerrit-CC: Daniel Andersson <dande...@chromium.org>
                          Gerrit-CC: Daniel Cheng <dch...@chromium.org>
                          Gerrit-CC: James Su <su...@chromium.org>
                          Gerrit-CC: Sadrul Chowdhury <sad...@chromium.org>
                          Gerrit-Attention: Mitsuru Oshima <osh...@chromium.org>
                          Gerrit-Comment-Date: Fri, 08 Sep 2023 17:06:35 +0000

                          Mitsuru Oshima (Gerrit)

                          unread,
                          Sep 11, 2023, 4:27:37 AM9/11/23
                          to alexmo...@chromium.org, bici...@google.com, creis...@chromium.org, headless...@chromium.org, jbauma...@chromium.org, keithle...@chromium.org, navigation...@chromium.org, nona+...@chromium.org, shuche...@chromium.org, tranbaod...@chromium.org, yhanad...@chromium.org, crostin...@chromium.org, oshima...@chromium.org, yhanada+...@chromium.org, ozone-...@chromium.org, cambickel+watc...@google.com, jimmyxgong+wat...@chromium.org, longbowei+watc...@google.com, michaelcheco+wa...@google.com, roblia...@chromium.org, sky+...@chromium.org, zentaro+watch...@chromium.org, zhangwenyu+wat...@google.com

                          Attention is currently required from: Mitsuru Oshima.

                          Mitsuru Oshima uploaded patch set #68 to this change.

                          View 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.

                          Gerrit-MessageType: newpatchset
                          Gerrit-Project: chromium/src
                          Gerrit-Branch: main
                          Gerrit-Change-Id: I486594789ec307bfcaf4ce81ba1c221c9bbeb93f
                          Gerrit-Change-Number: 4606716
                          Gerrit-PatchSet: 68

                          Mitsuru Oshima (Gerrit)

                          unread,
                          Sep 11, 2023, 5:06:05 AM9/11/23