New issue
Advanced search Search tips

Issue 764009 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: May 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 2
Type: Bug

Blocked on:
issue 682402
issue 844453

Blocking:
issue 768908



Sign in to add a comment

mash: Make WindowSizerAsh place new windows correctly

Project Member Reported by jamescook@chromium.org, Sep 11 2017

Issue description

WindowSizer::GetBrowserBoundsAsh() is being skipped on mash, due to a left-over early exit from the Windows Ash days.

This results in new windows not being positioned correctly. For example, hitting Ctrl-N repeatedly should alternate new windows on the left side and right side of the screen. Under mash they are all on top of each other.

+cc some people who touched the file long ago

 

Comment 1 by msw@chromium.org, Sep 26 2017

Blocking: 768908
Components: Internals>MUS
Labels: -Pri-2 Pri-1
Owner: jamescook@chromium.org
Status: Started (was: Untriaged)
This seems to be causing some browser_tests --mash failures, so I'm going to take a look now.

-BrowserTestTabbedOrApp/BrowserTestParam.*
-TabRestoreTest.*

Project Member

Comment 3 by bugdroid1@chromium.org, Dec 7 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a39bd3e52cb4d803b29b91caa0194391e2ac72fe

commit a39bd3e52cb4d803b29b91caa0194391e2ac72fe
Author: James Cook <jamescook@chromium.org>
Date: Thu Dec 07 01:49:46 2017

cros: Clean up ash::WindowPositioner and GetPopupPosition

* Convert GetPopupPosition to take a gfx::Size(), because it does not
  use the position of the passed Rect.
* Convert member variables that always match constant values to just
  use constants
* Convert non-mutating functions to static

I noticed these while looking at WindowPositioner for mustash.

TBR=msw@chromium.org

Bug:  764009 
Test: ash_unittests, chrome unit_tests WindowSizer*
Change-Id: Ie9c16a0a41fc46d8b6ca8918e1cfa757f80e8b70
Reviewed-on: https://chromium-review.googlesource.com/811684
Commit-Queue: James Cook <jamescook@chromium.org>
Reviewed-by: James Cook <jamescook@chromium.org>
Reviewed-by: Mitsuru Oshima <oshima@chromium.org>
Cr-Commit-Position: refs/heads/master@{#522297}
[modify] https://crrev.com/a39bd3e52cb4d803b29b91caa0194391e2ac72fe/ash/wm/window_positioner.cc
[modify] https://crrev.com/a39bd3e52cb4d803b29b91caa0194391e2ac72fe/ash/wm/window_positioner.h
[modify] https://crrev.com/a39bd3e52cb4d803b29b91caa0194391e2ac72fe/chrome/browser/ui/ash/window_positioner_unittest.cc
[modify] https://crrev.com/a39bd3e52cb4d803b29b91caa0194391e2ac72fe/chrome/browser/ui/window_sizer/window_sizer_ash.cc

Project Member

Comment 4 by bugdroid1@chromium.org, Dec 8 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/cfbb5308a23ae0e05e7e7e47ddcac58501f584c1

commit cfbb5308a23ae0e05e7e7e47ddcac58501f584c1
Author: James Cook <jamescook@chromium.org>
Date: Fri Dec 08 00:21:35 2017

cros: Move ash::WindowPositioner::GetDefaultWindowBounds into chrome

For mustash code in chrome can't call directly into ash. This function
is only used for positioning browser windows and has no ash
dependencies, so it should live in chrome.

Bug:  764009 
Test: ash_unittests, chrome unit_tests WindowSizer*
Change-Id: Ia9c56b64327e9baea2773acd6ea275dfa76b37c0
Reviewed-on: https://chromium-review.googlesource.com/815676
Reviewed-by: Scott Violet <sky@chromium.org>
Commit-Queue: James Cook <jamescook@chromium.org>
Cr-Commit-Position: refs/heads/master@{#522637}
[modify] https://crrev.com/cfbb5308a23ae0e05e7e7e47ddcac58501f584c1/ash/wm/window_positioner.cc
[modify] https://crrev.com/cfbb5308a23ae0e05e7e7e47ddcac58501f584c1/ash/wm/window_positioner.h
[modify] https://crrev.com/cfbb5308a23ae0e05e7e7e47ddcac58501f584c1/chrome/browser/ui/window_sizer/window_sizer.cc
[modify] https://crrev.com/cfbb5308a23ae0e05e7e7e47ddcac58501f584c1/chrome/browser/ui/window_sizer/window_sizer.h
[modify] https://crrev.com/cfbb5308a23ae0e05e7e7e47ddcac58501f584c1/chrome/browser/ui/window_sizer/window_sizer_ash.cc
[modify] https://crrev.com/cfbb5308a23ae0e05e7e7e47ddcac58501f584c1/chrome/browser/ui/window_sizer/window_sizer_ash_unittest.cc

Cc: jamescook@chromium.org sky@chromium.org
Owner: ----
Status: Available (was: Started)
Putting this on hold.

I can't figure out how to untangle the chrome and ash code. Chrome needs initial bounds and show state synchronously during browser widget initialization. However, that depends on code in ash that needs the entire window hierarchy, ash::WindowState, MruWindowTracker, etc. I've tried to defer parts of window initialization, but I can't get the behavior to match. Either we need to use sync IPC to call this code, or we need a mostly-rewritten implementation of this code for mash. The latter is hard because there's no complete specification of window positioning behavior -- I think we would need to use git blame on every block of code to go back in time and figure out why each behavior was added.

My best suggestion for a path forward is to reimplement some of this behavior in ash::CreateAndParentTopLevelWindow(), see draft CL https://chromium-review.googlesource.com/#/c/chromium/src/+/818300 

Doc with notes on approaches: https://docs.google.com/document/d/18qf9zMJWpcSejl7M4r60TwPkGP0WO5BX-VaAULq8wpU/edit

Components: -Internals>MUS Internals>Services>WindowService
Owner: est...@chromium.org
Status: Started (was: Available)
Project Member

Comment 8 by bugdroid1@chromium.org, Mar 6 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/4c2f73bf763b9dc52982a84ce703ba546afc3dda

commit 4c2f73bf763b9dc52982a84ce703ba546afc3dda
Author: Evan Stade <estade@chromium.org>
Date: Tue Mar 06 18:24:56 2018

Code clean ups in WindowPositioner/WindowSizer.

These are readability improvements I came across while trying to grok
WindowSizer. There should be no behavioral changes here.
- make WindowSizer ctors private
- remove dead SetMaximizeFirstWindow function
- replace else { if with else if

Bug:  764009 
Change-Id: I2594e31fee48a5f25cd00840d2aef3db1980b301
Reviewed-on: https://chromium-review.googlesource.com/950476
Commit-Queue: Evan Stade <estade@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Cr-Commit-Position: refs/heads/master@{#541152}
[modify] https://crrev.com/4c2f73bf763b9dc52982a84ce703ba546afc3dda/ash/wm/window_positioner.cc
[modify] https://crrev.com/4c2f73bf763b9dc52982a84ce703ba546afc3dda/ash/wm/window_positioner.h
[modify] https://crrev.com/4c2f73bf763b9dc52982a84ce703ba546afc3dda/chrome/browser/ui/window_sizer/window_sizer.cc
[modify] https://crrev.com/4c2f73bf763b9dc52982a84ce703ba546afc3dda/chrome/browser/ui/window_sizer/window_sizer.h
[modify] https://crrev.com/4c2f73bf763b9dc52982a84ce703ba546afc3dda/chrome/browser/ui/window_sizer/window_sizer_ash.cc
[modify] https://crrev.com/4c2f73bf763b9dc52982a84ce703ba546afc3dda/chrome/browser/ui/window_sizer/window_sizer_ash_unittest.cc
[modify] https://crrev.com/4c2f73bf763b9dc52982a84ce703ba546afc3dda/chrome/browser/ui/window_sizer/window_sizer_common_unittest.cc
[modify] https://crrev.com/4c2f73bf763b9dc52982a84ce703ba546afc3dda/chrome/browser/ui/window_sizer/window_sizer_common_unittest.h
[modify] https://crrev.com/4c2f73bf763b9dc52982a84ce703ba546afc3dda/chrome/browser/ui/window_sizer/window_sizer_unittest.cc

Comment 9 by est...@chromium.org, Mar 13 2018

 Issue 756098  has been merged into this issue.
Project Member

Comment 10 by bugdroid1@chromium.org, Mar 13 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/cbac7cb773cff9e2cc7dba9c78e723cc0668edf6

commit cbac7cb773cff9e2cc7dba9c78e723cc0668edf6
Author: Evan Stade <estade@chromium.org>
Date: Tue Mar 13 18:19:38 2018

Add command line flag for disabling some Ash-specific logic for
arranging new Browser windows.

This also reinstates some logic for Mash that doesn't depend on code
in ash/. With this flag passed, or when running in Mash,
ash::WindowPositioner won't be used for placing new Windows. Some
other Chrome OS specific code, like maximizing the first run window
according to policy, is retained.

Bug:  764009 
Change-Id: I7563b0b0648e2470ec4a601805d2b351b6d64bb0
Reviewed-on: https://chromium-review.googlesource.com/958030
Commit-Queue: Evan Stade <estade@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: James Cook <jamescook@chromium.org>
Cr-Commit-Position: refs/heads/master@{#542858}
[modify] https://crrev.com/cbac7cb773cff9e2cc7dba9c78e723cc0668edf6/chrome/browser/about_flags.cc
[modify] https://crrev.com/cbac7cb773cff9e2cc7dba9c78e723cc0668edf6/chrome/browser/flag_descriptions.cc
[modify] https://crrev.com/cbac7cb773cff9e2cc7dba9c78e723cc0668edf6/chrome/browser/flag_descriptions.h
[modify] https://crrev.com/cbac7cb773cff9e2cc7dba9c78e723cc0668edf6/chrome/browser/ui/window_sizer/window_sizer_ash.cc
[modify] https://crrev.com/cbac7cb773cff9e2cc7dba9c78e723cc0668edf6/chrome/common/chrome_switches.cc
[modify] https://crrev.com/cbac7cb773cff9e2cc7dba9c78e723cc0668edf6/chrome/common/chrome_switches.h
[modify] https://crrev.com/cbac7cb773cff9e2cc7dba9c78e723cc0668edf6/tools/metrics/histograms/enums.xml

Project Member

Comment 11 by bugdroid1@chromium.org, Apr 2 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/0c7e0b9bb17f324c6565154ec852344884463ae0

commit 0c7e0b9bb17f324c6565154ec852344884463ae0
Author: Evan Stade <estade@chromium.org>
Date: Mon Apr 02 23:43:30 2018

Ash: Remove smart window positioning logic for popups

Also move the first run/force-maximizing logic into Chrome/, i.e.
WindowSizerAsh. Removing it from ash::WindowPositioner seems OK because
the logic should only really apply to browser windows.

UX contact: jennschen
PM contact: kuscher

Bug:  764009 
Change-Id: Ie0642d1a50c4047082de025a36f59fadc1a3fc1d
Reviewed-on: https://chromium-review.googlesource.com/985015
Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: James Cook <jamescook@chromium.org>
Commit-Queue: Evan Stade <estade@chromium.org>
Cr-Commit-Position: refs/heads/master@{#547567}
[modify] https://crrev.com/0c7e0b9bb17f324c6565154ec852344884463ae0/ash/shell/shell_delegate_impl.cc
[modify] https://crrev.com/0c7e0b9bb17f324c6565154ec852344884463ae0/ash/shell/shell_delegate_impl.h
[modify] https://crrev.com/0c7e0b9bb17f324c6565154ec852344884463ae0/ash/shell_delegate.h
[modify] https://crrev.com/0c7e0b9bb17f324c6565154ec852344884463ae0/ash/shell_delegate_mus.cc
[modify] https://crrev.com/0c7e0b9bb17f324c6565154ec852344884463ae0/ash/shell_delegate_mus.h
[modify] https://crrev.com/0c7e0b9bb17f324c6565154ec852344884463ae0/ash/test_shell_delegate.cc
[modify] https://crrev.com/0c7e0b9bb17f324c6565154ec852344884463ae0/ash/test_shell_delegate.h
[modify] https://crrev.com/0c7e0b9bb17f324c6565154ec852344884463ae0/ash/wm/window_positioner.cc
[modify] https://crrev.com/0c7e0b9bb17f324c6565154ec852344884463ae0/ash/wm/window_positioner.h
[modify] https://crrev.com/0c7e0b9bb17f324c6565154ec852344884463ae0/ash/wm/window_positioner_unittest.cc
[modify] https://crrev.com/0c7e0b9bb17f324c6565154ec852344884463ae0/chrome/browser/about_flags.cc
[modify] https://crrev.com/0c7e0b9bb17f324c6565154ec852344884463ae0/chrome/browser/chromeos/policy/force_maximize_on_first_run_chromeos_browsertest.cc
[modify] https://crrev.com/0c7e0b9bb17f324c6565154ec852344884463ae0/chrome/browser/flag_descriptions.cc
[modify] https://crrev.com/0c7e0b9bb17f324c6565154ec852344884463ae0/chrome/browser/flag_descriptions.h
[modify] https://crrev.com/0c7e0b9bb17f324c6565154ec852344884463ae0/chrome/browser/ui/ash/chrome_shell_delegate.cc
[modify] https://crrev.com/0c7e0b9bb17f324c6565154ec852344884463ae0/chrome/browser/ui/ash/chrome_shell_delegate.h
[delete] https://crrev.com/6de618859d1c700738ce94cfcce017bac664b171/chrome/browser/ui/ash/window_positioner_unittest.cc
[modify] https://crrev.com/0c7e0b9bb17f324c6565154ec852344884463ae0/chrome/browser/ui/window_sizer/window_sizer.cc
[modify] https://crrev.com/0c7e0b9bb17f324c6565154ec852344884463ae0/chrome/browser/ui/window_sizer/window_sizer.h
[modify] https://crrev.com/0c7e0b9bb17f324c6565154ec852344884463ae0/chrome/browser/ui/window_sizer/window_sizer_ash.cc
[modify] https://crrev.com/0c7e0b9bb17f324c6565154ec852344884463ae0/chrome/browser/ui/window_sizer/window_sizer_ash_unittest.cc
[modify] https://crrev.com/0c7e0b9bb17f324c6565154ec852344884463ae0/chrome/browser/ui/window_sizer/window_sizer_common_unittest.cc
[modify] https://crrev.com/0c7e0b9bb17f324c6565154ec852344884463ae0/chrome/common/chrome_switches.cc
[modify] https://crrev.com/0c7e0b9bb17f324c6565154ec852344884463ae0/chrome/common/chrome_switches.h
[modify] https://crrev.com/0c7e0b9bb17f324c6565154ec852344884463ae0/chrome/test/BUILD.gn

Blockedon: 682402
Components: -Internals>Services>WindowService Internals>Services>Ash
Labels: -Proj-Mustash-Mash
Labels: -Pri-1 Pri-2
this function still relies on ash:

display::Display WindowSizer::DefaultTargetDisplayProvider::GetTargetDisplay(
    const display::Screen* screen,
    const gfx::Rect& bounds) const {
#if defined(OS_CHROMEOS)
  // Use the target display on ash.
  if (ash_util::ShouldOpenAshOnStartup()) {
    aura::Window* target = ash::Shell::GetRootWindowForNewWindows();
    return screen->GetDisplayNearestWindow(target);
  }
#endif
  // Find the size of the work area of the monitor that intersects the bounds
  // of the anchor window.
  return screen->GetDisplayMatching(bounds);
}

Options:

1. just remove the OS_CHROMEOS block. This causes new browser windows to show on the wrong display: they'll appear on the display where a browser window was last closed instead of the display for new windows. For example, on startup, a browser window may be created on the non-primary display. Also, ctrl+n or opening a first window via the shelf will place the window on the display where a browser window was last closed instead of the active/"for new windows" display.

2. try to remove calls to this function and let ash place new windows on the correct display. This breaks several places where we adjust initial bounds to fit within the bounds of the target display

3. make the concept of "display for new windows" accessible from Chrome. Add it to Screen? Add a Chrome-accessible version of Shell that mirrors miscellaneous details like this into the Chrome process?

Comment 15 by sky@chromium.org, May 11 2018

(2) would be really nice! But as you say, it's rather hard to do.
We've typically gone with (3) for things of this sort.
Is that because chrome needs to know the bounds of the "display for new windows"?

At first I was hoping we could do something hacky like let chrome under oop-ash set a mus window property display id of -999 or something, as a clue to let ash put the window on the "display for new windows".

(1) seems bad and likely to break things in a user-visible way.

I don't like a chrome-accessible version of Shell. Mirroring the display-id-for-new-windows into Screen (or some similar class) seems better to me.

Anyone else have thoughts?

2) would be nice, but 3) isn't bad either.

Adding display::Screen::GetDisplayForNewWindow() seems to be good short
term solution (assuming it's much simpler) at least.
I think display::GetDisplayForNewWindow() could be implemented this way:

ash::Shell (or an OnWindowActivated observer) ->
//services/ui/ws2/window_service.cc ->
ScreenProviderMus ->
(mojo)
ScreenMus

It could follow SetFrameDecorationValues. Maybe it should have its own mojo method to avoid sending the whole display list every time a window activates.

I'm going to try implementing GetDisplayForNewWindow() today with the approach in #18 (via Screen). I think msw will need it for the shortcut viewer. Filed  issue 844453 .

Ping me on hangouts if you have already started this and I'll stop. :-)

Cc: msw@chromium.org
I looked at (3) and adding GetDisplayForNewWindows() to display::Screen. It felt wrong - Screen is cross-platform and we only need/use this on ash. It's also a bit more complex than I thought it would be.

I'm going to look at (3) with a Chrome-accessible mirror of this state.
Blockedon: 844453

Comment 22 by msw@chromium.org, May 18 2018

Why doesn't ash just place new windows on the display for new windows by default?
If a client doesn't specify kDisplayId_InitProperty, shouldn't it just be placed on the display for new windows (which is known to ash)? Clients probably shouldn't need to provide that info themselves.
re #22: see option 2 in #14

Comment 24 by msw@chromium.org, May 22 2018

Thanks for pointing that out, it seems like the right idea, and James suggested moving some of the initial bounds setting to ash as well (maybe the window asks to be centered, or ash has that behavior by default, or something smarter when multiple windows are opened?)
 Issue 844453  has been merged into this issue.
Cc: -jamescook@chromium.org est...@chromium.org
Owner: jamescook@chromium.org
I'm taking a look at WindowSizer::DefaultTargetDisplayProvider::GetTargetDisplay mentioned in comment 14.

Cc: -varkha@chromium.org
Project Member

Comment 28 by bugdroid1@chromium.org, May 25 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/40f7d75ba9538c1323b69da9d348c92d5fce4136

commit 40f7d75ba9538c1323b69da9d348c92d5fce4136
Author: James Cook <jamescook@chromium.org>
Date: Fri May 25 18:21:58 2018

chromeos: Update browser window placement for out-of-process ash

WindowSizerAsh needs synchronous access to the target display for each
new browser window.

* Create ash::ShellState and move root_window_for_new_windows_ there
* Introduce mojom::ShellState interface in ash
* Create a ShellStateClient in chrome to cache the target display id
  for new windows

The interface is not implemented by ash::Shell because ash::Shell
is already massive (1500 lines). Also, shell.h is included in 900
places in the code base and I don't want to add mojo generated headers
to shell.h.

Windows still don't appear on the secondary display with
--enable-features=Mash, but chrome is getting the correct display
id and setting the window bounds properly. I think ash needs to be
fixed to support top-level bounds on the secondary display, but that
will need to be a follow-up CL.

Bug:  764009 ,  768908 
Test: ash_unittests, unit_tests for WindowSizer
Change-Id: Ib77834f8ed5dc4e65f02ff83ab81acbf3331db61
Reviewed-on: https://chromium-review.googlesource.com/1067591
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: Evan Stade <estade@chromium.org>
Commit-Queue: James Cook <jamescook@chromium.org>
Cr-Commit-Position: refs/heads/master@{#561950}
[modify] https://crrev.com/40f7d75ba9538c1323b69da9d348c92d5fce4136/ash/BUILD.gn
[modify] https://crrev.com/40f7d75ba9538c1323b69da9d348c92d5fce4136/ash/manifest.json
[modify] https://crrev.com/40f7d75ba9538c1323b69da9d348c92d5fce4136/ash/mojo_interface_factory.cc
[modify] https://crrev.com/40f7d75ba9538c1323b69da9d348c92d5fce4136/ash/public/interfaces/BUILD.gn
[add] https://crrev.com/40f7d75ba9538c1323b69da9d348c92d5fce4136/ash/public/interfaces/shell_state.mojom
[modify] https://crrev.com/40f7d75ba9538c1323b69da9d348c92d5fce4136/ash/root_window_controller.cc
[modify] https://crrev.com/40f7d75ba9538c1323b69da9d348c92d5fce4136/ash/scoped_root_window_for_new_windows.cc
[modify] https://crrev.com/40f7d75ba9538c1323b69da9d348c92d5fce4136/ash/scoped_root_window_for_new_windows.h
[modify] https://crrev.com/40f7d75ba9538c1323b69da9d348c92d5fce4136/ash/shelf/shelf_view.cc
[modify] https://crrev.com/40f7d75ba9538c1323b69da9d348c92d5fce4136/ash/shell.cc
[modify] https://crrev.com/40f7d75ba9538c1323b69da9d348c92d5fce4136/ash/shell.h
[add] https://crrev.com/40f7d75ba9538c1323b69da9d348c92d5fce4136/ash/shell_state.cc
[add] https://crrev.com/40f7d75ba9538c1323b69da9d348c92d5fce4136/ash/shell_state.h
[add] https://crrev.com/40f7d75ba9538c1323b69da9d348c92d5fce4136/ash/shell_state_unittest.cc
[modify] https://crrev.com/40f7d75ba9538c1323b69da9d348c92d5fce4136/ash/wm/window_positioning_utils.cc
[modify] https://crrev.com/40f7d75ba9538c1323b69da9d348c92d5fce4136/chrome/browser/ui/BUILD.gn
[modify] https://crrev.com/40f7d75ba9538c1323b69da9d348c92d5fce4136/chrome/browser/ui/ash/DEPS
[modify] https://crrev.com/40f7d75ba9538c1323b69da9d348c92d5fce4136/chrome/browser/ui/ash/chrome_browser_main_extra_parts_ash.cc
[modify] https://crrev.com/40f7d75ba9538c1323b69da9d348c92d5fce4136/chrome/browser/ui/ash/chrome_browser_main_extra_parts_ash.h
[modify] https://crrev.com/40f7d75ba9538c1323b69da9d348c92d5fce4136/chrome/browser/ui/ash/launcher/DEPS
[modify] https://crrev.com/40f7d75ba9538c1323b69da9d348c92d5fce4136/chrome/browser/ui/ash/launcher/extension_launcher_context_menu.cc
[add] https://crrev.com/40f7d75ba9538c1323b69da9d348c92d5fce4136/chrome/browser/ui/ash/shell_state_client.cc
[add] https://crrev.com/40f7d75ba9538c1323b69da9d348c92d5fce4136/chrome/browser/ui/ash/shell_state_client.h
[add] https://crrev.com/40f7d75ba9538c1323b69da9d348c92d5fce4136/chrome/browser/ui/ash/shell_state_client_unittest.cc
[modify] https://crrev.com/40f7d75ba9538c1323b69da9d348c92d5fce4136/chrome/browser/ui/window_sizer/window_sizer.cc
[modify] https://crrev.com/40f7d75ba9538c1323b69da9d348c92d5fce4136/chrome/browser/ui/window_sizer/window_sizer.h
[modify] https://crrev.com/40f7d75ba9538c1323b69da9d348c92d5fce4136/chrome/browser/ui/window_sizer/window_sizer_ash_unittest.cc
[modify] https://crrev.com/40f7d75ba9538c1323b69da9d348c92d5fce4136/chrome/browser/ui/window_sizer/window_sizer_common_unittest.cc
[modify] https://crrev.com/40f7d75ba9538c1323b69da9d348c92d5fce4136/chrome/test/BUILD.gn

Project Member

Comment 29 by bugdroid1@chromium.org, May 30 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a863721005aa16a55213880db40aebb5baf7bf1e

commit a863721005aa16a55213880db40aebb5baf7bf1e
Author: James Cook <jamescook@chromium.org>
Date: Wed May 30 02:47:10 2018

chromeos: Set initial browser widget bounds under mash

This makes new windows appear correctly on secondary displays.

Bug:  764009 
Test: browser_tests, manually open browser windows
Change-Id: I92838f73cc1f52d5e7c28b914d82fb80a795a523
Reviewed-on: https://chromium-review.googlesource.com/1077549
Reviewed-by: Michael Wasserman <msw@chromium.org>
Commit-Queue: James Cook <jamescook@chromium.org>
Cr-Commit-Position: refs/heads/master@{#562706}
[modify] https://crrev.com/a863721005aa16a55213880db40aebb5baf7bf1e/chrome/browser/ui/views/frame/browser_frame_ash_browsertest.cc
[modify] https://crrev.com/a863721005aa16a55213880db40aebb5baf7bf1e/chrome/browser/ui/views/frame/browser_frame_mus.cc
[modify] https://crrev.com/a863721005aa16a55213880db40aebb5baf7bf1e/chrome/browser/ui/views/frame/browser_frame_mus.h

Status: Fixed (was: Started)
Done. There's still a bug where the contents area isn't always the same size as the widget bounds/frame. We might make some changes to which BrowserFrame class we use in mash, though, so I'm going to hold off on investigating (and there's a separate bug for it,  issue 831369 ).

Thanks to estade for doing all the heavy lifting here!

Project Member

Comment 31 by bugdroid1@chromium.org, Aug 7

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/05b3a3f7cada9f1fe67ef2ff4f73f013525975f3

commit 05b3a3f7cada9f1fe67ef2ff4f73f013525975f3
Author: James Cook <jamescook@chromium.org>
Date: Tue Aug 07 00:38:55 2018

Add Screen::GetDisplayForNewWindows to support remote mojo apps

Chrome OS supports the concept of a "root window for new windows",
which allows new windows to be automatically placed on the correct
display. This knowledge lives in ash, currently inside the browser
process.

Remote processes that use views::Widget (e.g. the shortcut viewer app)
sometimes need to know what window they are going to open on,
for example to do initial bounds computations.

Change Screen to provide set and get for the display for new windows.
Use mojo to send the information to other processes via ScreenProvider
(on the ash side) and ScreenMus (on the remote side). Send the display
id as part of display updates so that ScreenMus gets it on startup,
and also so updates are atomic with other display changes.

Once this lands we'll be able to port browser windows to use this
mechanism and delete the ash::ShellState / chrome ShellStateClient
code I built as a one-off for browser windows under mash.

Bug:  867559 ,  764009 ,  826569 
Test: added unit tests
Change-Id: I6b5beb2610243f30072c3d73180b3ce21a310ca6
Reviewed-on: https://chromium-review.googlesource.com/1162980
Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: Tom Sepez <tsepez@chromium.org>
Reviewed-by: Michael Wasserman <msw@chromium.org>
Commit-Queue: James Cook <jamescook@chromium.org>
Cr-Commit-Position: refs/heads/master@{#581055}
[modify] https://crrev.com/05b3a3f7cada9f1fe67ef2ff4f73f013525975f3/ash/display/screen_ash.cc
[modify] https://crrev.com/05b3a3f7cada9f1fe67ef2ff4f73f013525975f3/ash/display/screen_ash.h
[modify] https://crrev.com/05b3a3f7cada9f1fe67ef2ff4f73f013525975f3/ash/display/screen_ash_unittest.cc
[modify] https://crrev.com/05b3a3f7cada9f1fe67ef2ff4f73f013525975f3/ash/shell.cc
[modify] https://crrev.com/05b3a3f7cada9f1fe67ef2ff4f73f013525975f3/ash/shell_state.cc
[modify] https://crrev.com/05b3a3f7cada9f1fe67ef2ff4f73f013525975f3/ash/shell_state.h
[modify] https://crrev.com/05b3a3f7cada9f1fe67ef2ff4f73f013525975f3/ash/ws/window_service_owner.cc
[modify] https://crrev.com/05b3a3f7cada9f1fe67ef2ff4f73f013525975f3/services/ui/public/interfaces/screen_provider_observer.mojom
[modify] https://crrev.com/05b3a3f7cada9f1fe67ef2ff4f73f013525975f3/services/ui/ws2/screen_provider.cc
[modify] https://crrev.com/05b3a3f7cada9f1fe67ef2ff4f73f013525975f3/services/ui/ws2/screen_provider.h
[modify] https://crrev.com/05b3a3f7cada9f1fe67ef2ff4f73f013525975f3/services/ui/ws2/screen_provider_unittest.cc
[modify] https://crrev.com/05b3a3f7cada9f1fe67ef2ff4f73f013525975f3/services/ui/ws2/test_screen_provider_observer.cc
[modify] https://crrev.com/05b3a3f7cada9f1fe67ef2ff4f73f013525975f3/services/ui/ws2/test_screen_provider_observer.h
[modify] https://crrev.com/05b3a3f7cada9f1fe67ef2ff4f73f013525975f3/services/ui/ws2/window_service.cc
[modify] https://crrev.com/05b3a3f7cada9f1fe67ef2ff4f73f013525975f3/services/ui/ws2/window_service.h
[modify] https://crrev.com/05b3a3f7cada9f1fe67ef2ff4f73f013525975f3/ui/aura/mus/window_tree_client.cc
[modify] https://crrev.com/05b3a3f7cada9f1fe67ef2ff4f73f013525975f3/ui/aura/mus/window_tree_client.h
[modify] https://crrev.com/05b3a3f7cada9f1fe67ef2ff4f73f013525975f3/ui/aura/mus/window_tree_client_delegate.h
[modify] https://crrev.com/05b3a3f7cada9f1fe67ef2ff4f73f013525975f3/ui/display/screen.cc
[modify] https://crrev.com/05b3a3f7cada9f1fe67ef2ff4f73f013525975f3/ui/display/screen.h
[modify] https://crrev.com/05b3a3f7cada9f1fe67ef2ff4f73f013525975f3/ui/display/screen_unittest.cc
[modify] https://crrev.com/05b3a3f7cada9f1fe67ef2ff4f73f013525975f3/ui/views/mus/mus_client.cc
[modify] https://crrev.com/05b3a3f7cada9f1fe67ef2ff4f73f013525975f3/ui/views/mus/mus_client.h
[modify] https://crrev.com/05b3a3f7cada9f1fe67ef2ff4f73f013525975f3/ui/views/mus/screen_mus.cc
[modify] https://crrev.com/05b3a3f7cada9f1fe67ef2ff4f73f013525975f3/ui/views/mus/screen_mus.h
[modify] https://crrev.com/05b3a3f7cada9f1fe67ef2ff4f73f013525975f3/ui/views/mus/screen_mus_unittest.cc

Project Member

Comment 32 by bugdroid1@chromium.org, Aug 10

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/7363ebc06d042af169ab640f33a3d4b985546000

commit 7363ebc06d042af169ab640f33a3d4b985546000
Author: James Cook <jamescook@chromium.org>
Date: Fri Aug 10 21:17:24 2018

chromeos: Use Screen::GetDisplayForNewWindows in chrome/browser

display::Screen (and ScreenMus) now has the display for new windows,
so we can use that when making new browser windows under both mash
and classic ash. Likewise, we can use display::Screen (as ScreenAsh)
for new windows inside of ash.

This lets me remove ShellStateClient, a different mechanism that
accomplishes the same goal in a less general way.

Leave ash::ShellState for now. I will clean that up in a separate CL.

TBR=tsepez@chromium.org

Bug:  764009 
Test: existing browser_tests for browser frame/size
Change-Id: I5c255dc2bf8fdd48f519f148d29aca3d37e7ae73
Reviewed-on: https://chromium-review.googlesource.com/1168197
Reviewed-by: James Cook <jamescook@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Commit-Queue: James Cook <jamescook@chromium.org>
Cr-Commit-Position: refs/heads/master@{#582336}
[modify] https://crrev.com/7363ebc06d042af169ab640f33a3d4b985546000/ash/display/screen_ash.cc
[modify] https://crrev.com/7363ebc06d042af169ab640f33a3d4b985546000/ash/display/screen_ash.h
[modify] https://crrev.com/7363ebc06d042af169ab640f33a3d4b985546000/ash/mojo_interface_factory.cc
[modify] https://crrev.com/7363ebc06d042af169ab640f33a3d4b985546000/ash/public/interfaces/BUILD.gn
[delete] https://crrev.com/07f5eb56f751c2ab177a66471742d3f10ee41bf8/ash/public/interfaces/shell_state.mojom
[modify] https://crrev.com/7363ebc06d042af169ab640f33a3d4b985546000/ash/shell_state.cc
[modify] https://crrev.com/7363ebc06d042af169ab640f33a3d4b985546000/ash/shell_state.h
[modify] https://crrev.com/7363ebc06d042af169ab640f33a3d4b985546000/ash/shell_state_unittest.cc
[modify] https://crrev.com/7363ebc06d042af169ab640f33a3d4b985546000/chrome/browser/ui/BUILD.gn
[modify] https://crrev.com/7363ebc06d042af169ab640f33a3d4b985546000/chrome/browser/ui/ash/chrome_browser_main_extra_parts_ash.cc
[modify] https://crrev.com/7363ebc06d042af169ab640f33a3d4b985546000/chrome/browser/ui/ash/chrome_browser_main_extra_parts_ash.h
[modify] https://crrev.com/7363ebc06d042af169ab640f33a3d4b985546000/chrome/browser/ui/ash/launcher/crostini_app_display.cc
[modify] https://crrev.com/7363ebc06d042af169ab640f33a3d4b985546000/chrome/browser/ui/ash/launcher/extension_launcher_context_menu.cc
[delete] https://crrev.com/07f5eb56f751c2ab177a66471742d3f10ee41bf8/chrome/browser/ui/ash/shell_state_client.cc
[delete] https://crrev.com/07f5eb56f751c2ab177a66471742d3f10ee41bf8/chrome/browser/ui/ash/shell_state_client.h
[delete] https://crrev.com/07f5eb56f751c2ab177a66471742d3f10ee41bf8/chrome/browser/ui/ash/shell_state_client_unittest.cc
[modify] https://crrev.com/7363ebc06d042af169ab640f33a3d4b985546000/chrome/browser/ui/window_sizer/window_sizer.cc
[modify] https://crrev.com/7363ebc06d042af169ab640f33a3d4b985546000/chrome/browser/ui/window_sizer/window_sizer_ash_unittest.cc
[modify] https://crrev.com/7363ebc06d042af169ab640f33a3d4b985546000/chrome/test/BUILD.gn
[modify] https://crrev.com/7363ebc06d042af169ab640f33a3d4b985546000/ui/display/screen.cc
[modify] https://crrev.com/7363ebc06d042af169ab640f33a3d4b985546000/ui/display/screen.h
[modify] https://crrev.com/7363ebc06d042af169ab640f33a3d4b985546000/ui/views/mus/screen_mus.cc
[modify] https://crrev.com/7363ebc06d042af169ab640f33a3d4b985546000/ui/views/mus/screen_mus.h

Sign in to add a comment