Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2840903002:
mash: Fix create fullscreen widget (Closed)
Descriptionmash: Fix create fullscreen widget
Set kShowStateKey property from init params show_state so that
mus window is created properly.
BUG=714804
TEST=DesktopWindowTreeHostMusTest.CreateFullscreenWidget
Review-Url: https://codereview.chromium.org/2840903002
Cr-Commit-Position: refs/heads/master@{#467572}
Committed: https://chromium.googlesource.com/chromium/src/+/c1ad8f0a9e80c7b864f432fb4cdbb6f987de53fb
Patch Set 1 #
Total comments: 2
Patch Set 2 : fix init show_state not respected in mus #Patch Set 3 : const #
Messages
Total messages: 22 (12 generated)
xiyuan@chromium.org changed reviewers: + jamescook@chromium.org
jamescook@chromium.org changed reviewers: + sky@chromium.org
+sky, a question about Widget::InitParams and mus show state. https://codereview.chromium.org/2840903002/diff/1/chrome/browser/chromeos/log... File chrome/browser/chromeos/login/ui/lock_window.cc (right): https://codereview.chromium.org/2840903002/diff/1/chrome/browser/chromeos/log... chrome/browser/chromeos/login/ui/lock_window.cc:32: params.mus_properties[WindowManager::kShowState_Property] = It seems bad that setting Views::Widget::InitParams::show_state = ui::SHOW_STATE_FULLSCREEN is not sufficient to make this work. I wonder if this should be fixed lower-down, like in views.
https://codereview.chromium.org/2840903002/diff/1/chrome/browser/chromeos/log... File chrome/browser/chromeos/login/ui/lock_window.cc (right): https://codereview.chromium.org/2840903002/diff/1/chrome/browser/chromeos/log... chrome/browser/chromeos/login/ui/lock_window.cc:32: params.mus_properties[WindowManager::kShowState_Property] = On 2017/04/25 19:42:28, James Cook wrote: > It seems bad that setting Views::Widget::InitParams::show_state = > ui::SHOW_STATE_FULLSCREEN is not sufficient to make this work. > > I wonder if this should be fixed lower-down, like in views. Setting the SHOW_STATE_FULLSCREEN on the init params should mean we end up in DesktopWindowTreeHostMus::ShowWindowWithState and the fullscreenstate is then set on the window. Setting the kShowStateKey should make it so ash sees the change as well. Is that not working? Is the issue that the window needs to be fullscreen before parented in ash?
On 2017/04/25 22:35:43, sky wrote: > Setting the SHOW_STATE_FULLSCREEN on the init params should mean we end up in > DesktopWindowTreeHostMus::ShowWindowWithState and the fullscreenstate is then > set on the window. Setting the kShowStateKey should make it so ash sees the > change as well. Is that not working? > Is the issue that the window needs to be fullscreen before parented in ash? The broken part is that DesktopWindowTreeHostMus::ShowWindowWithState never sees the show_state of init params for frameless window. The fullscreen state seems never worked. LockWindow appears fullscreen because its bounds was respected in cash. Let me see how to plumb the init show_state for frameless window through.
On 2017/04/26 20:24:41, xiyuan wrote: > On 2017/04/25 22:35:43, sky wrote: > > Setting the SHOW_STATE_FULLSCREEN on the init params should mean we end up in > > DesktopWindowTreeHostMus::ShowWindowWithState and the fullscreenstate is then > > set on the window. Setting the kShowStateKey should make it so ash sees the > > change as well. Is that not working? > > Is the issue that the window needs to be fullscreen before parented in ash? > > The broken part is that DesktopWindowTreeHostMus::ShowWindowWithState never sees > the show_state of init params for frameless window. The fullscreen state seems > never worked. LockWindow appears fullscreen because its bounds was respected in > cash. > > Let me see how to plumb the init show_state for frameless window through. Take back the "never worked" claim. NativeWidgetAura::InitNativeWidget sets show state from init params [1]. But this did not happen in mus code path. Adding similar code to DesktopWindowTreeHostMus::Init seems fixed the problem. [1]: https://cs.chromium.org/chromium/src/ui/views/widget/native_widget_aura.cc?rc...
Description was changed from ========== mash: Fix LockWindow fullscreen Use mus_properties[kShowState_Property] to set fullscreen state for mus window. Otherwise, shelf area is un-occupied. BUG=714804 ========== to ========== mash: Fix create fullscreen widget Set kShowStateKey property from init params show_state so that mus window is created with properly. BUG=714804 TEST=DesktopWindowTreeHostMusTest.CreateFullscreenWidget ==========
LGTM but sky should look too.
On 2017/04/26 22:51:21, James Cook wrote: > LGTM but sky should look too. hoho... You read my mind and reviewed before I even asked. :) sky@, WDYT?
The CQ bit was checked by xiyuan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Nice! LGTM
Description was changed from ========== mash: Fix create fullscreen widget Set kShowStateKey property from init params show_state so that mus window is created with properly. BUG=714804 TEST=DesktopWindowTreeHostMusTest.CreateFullscreenWidget ========== to ========== mash: Fix create fullscreen widget Set kShowStateKey property from init params show_state so that mus window is created properly. BUG=714804 TEST=DesktopWindowTreeHostMusTest.CreateFullscreenWidget ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by xiyuan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jamescook@chromium.org Link to the patchset: https://codereview.chromium.org/2840903002/#ps40001 (title: "const")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1493262896064970,
"parent_rev": "2236c8e01131fe9f2d02056a04d7494188d83a23", "commit_rev":
"c1ad8f0a9e80c7b864f432fb4cdbb6f987de53fb"}
Message was sent while issue was closed.
Description was changed from ========== mash: Fix create fullscreen widget Set kShowStateKey property from init params show_state so that mus window is created properly. BUG=714804 TEST=DesktopWindowTreeHostMusTest.CreateFullscreenWidget ========== to ========== mash: Fix create fullscreen widget Set kShowStateKey property from init params show_state so that mus window is created properly. BUG=714804 TEST=DesktopWindowTreeHostMusTest.CreateFullscreenWidget Review-Url: https://codereview.chromium.org/2840903002 Cr-Commit-Position: refs/heads/master@{#467572} Committed: https://chromium.googlesource.com/chromium/src/+/c1ad8f0a9e80c7b864f432fb4cdb... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/c1ad8f0a9e80c7b864f432fb4cdb... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
