Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2237983002:
Allow updating arc widget bounds in maximize/fullscreen state. (Closed)
DescriptionAllow updating arc widget bounds in maximize/fullscreen state.
ARC window can be pillar/letterboxed in maximized state, and chrome needs to adjust its window to those sizes.
BUG=634572
R=reveman@chromium.org
TEST=WindowStateTest.AllowSetBoundsInMaximized
Manually tested on minnie
Committed: https://crrev.com/e68d9c3c4f704de541d5f796aec4c0bd82e7ee7b
Cr-Commit-Position: refs/heads/master@{#412006}
Patch Set 1 #Patch Set 2 : . #
Total comments: 7
Patch Set 3 : addressed comments #Patch Set 4 : remoevd debug code #
Total comments: 11
Patch Set 5 : . #Patch Set 6 : Allow updating arc widget bounds in maximize/fullscreen state. #Patch Set 7 : . #
Total comments: 2
Patch Set 8 #Patch Set 9 #Patch Set 10 : Allow updating arc widget bounds in maximize/fullscreen state. #
Total comments: 6
Patch Set 11 : addressed comments #Patch Set 12 : Use black background #
Messages
Total messages: 44 (28 generated)
The CQ bit was checked by oshima@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...
have you verified that normal xdg clients like wayland-terminal still work properly? https://codereview.chromium.org/2237983002/diff/20001/ash/common/wm/window_st... File ash/common/wm/window_state.h (right): https://codereview.chromium.org/2237983002/diff/20001/ash/common/wm/window_st... ash/common/wm/window_state.h:295: allow_set_bounds_in_maximized_ = true; "allow_set_bounds_in_maximized_ = value;" ? https://codereview.chromium.org/2237983002/diff/20001/ash/common/wm/window_st... ash/common/wm/window_state.h:387: bool allow_set_bounds_in_maximized_ = false; it's a bit confusing that this is false by default. so normal chrome app window bounds can't be updated? https://codereview.chromium.org/2237983002/diff/20001/ash/wm/window_state_uni... File ash/wm/window_state_unittest.cc (right): https://codereview.chromium.org/2237983002/diff/20001/ash/wm/window_state_uni... ash/wm/window_state_unittest.cc:388: window_state->set_allow_set_bounds_in_maximized(true); can you test that restoring it to false after this also works? https://codereview.chromium.org/2237983002/diff/20001/components/exo/shell_su... File components/exo/shell_surface.cc (right): https://codereview.chromium.org/2237983002/diff/20001/components/exo/shell_su... components/exo/shell_surface.cc:1157: if (IsResizing()) I guess it's fine to remove this thanks to the pending configure requests we'll have for normal xdg-shell clients. correct?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_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 oshima@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...
https://codereview.chromium.org/2237983002/diff/20001/ash/common/wm/window_st... File ash/common/wm/window_state.h (right): https://codereview.chromium.org/2237983002/diff/20001/ash/common/wm/window_st... ash/common/wm/window_state.h:295: allow_set_bounds_in_maximized_ = true; On 2016/08/11 19:07:25, reveman wrote: > "allow_set_bounds_in_maximized_ = value;" ? Oops, thanks for the catch. Done and added test case for this. https://codereview.chromium.org/2237983002/diff/20001/ash/common/wm/window_st... ash/common/wm/window_state.h:387: bool allow_set_bounds_in_maximized_ = false; On 2016/08/11 19:07:25, reveman wrote: > it's a bit confusing that this is false by default. so normal chrome app window > bounds can't be updated? Maximized state means it's always maximized to its maximum size. Setting bounds results in updating restore bounds in normal case and will be used when restored. If an app wants to change the bounds, then it should restore first. On second thought, I think we should do this only for arc apps because this behavior is very specific to arc apps (landscape/portrait). I changed to set this only when application id is set.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
please hold on. looks like test failures are real.
Removed debug code I had (my bad). PTAL. I tested with wayland-terminal with this flag just in case, and worked fine. https://codereview.chromium.org/2237983002/diff/20001/components/exo/shell_su... File components/exo/shell_surface.cc (right): https://codereview.chromium.org/2237983002/diff/20001/components/exo/shell_su... components/exo/shell_surface.cc:1157: if (IsResizing()) On 2016/08/11 19:07:25, reveman wrote: > I guess it's fine to remove this thanks to the pending configure requests we'll > have for normal xdg-shell clients. correct? Done.
Description was changed from ========== Allow updating arc widget bounds in maximize/fullscreen state. BUG=634572 R=reveman@chromium.org TEST=WindowStateTest.AllowSetBoundsInMaximized Manually tested on minnie ========== to ========== Allow updating arc widget bounds in maximize/fullscreen state. ARC window can be pillar/letterboxed in maximized state, and chrome needs to adjust its window to those sizes. BUG=634572 R=reveman@chromium.org TEST=WindowStateTest.AllowSetBoundsInMaximized Manually tested on minnie ==========
The CQ bit was checked by oshima@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Exceeded global retry quota
https://codereview.chromium.org/2237983002/diff/60001/ash/common/wm/window_st... File ash/common/wm/window_state.h (right): https://codereview.chromium.org/2237983002/diff/60001/ash/common/wm/window_st... ash/common/wm/window_state.h:294: void set_allow_set_bounds_in_maximized(bool value) { nit: ..when_maximized or in_maximized_mode/state https://codereview.chromium.org/2237983002/diff/60001/components/exo/shell_su... File components/exo/shell_surface.cc (left): https://codereview.chromium.org/2237983002/diff/60001/components/exo/shell_su... components/exo/shell_surface.cc:1156: if (widget_->IsMaximized() || widget_->IsFullscreen() || IsResizing()) I'm pretty sure we need the IsResizing part for interactive resize of wl_shell client surfaces. That type of interactive resize works by chrome changing the size of the widget and that generating configure events that the client can ignore if it doesn't like the size or provide contents of a new size if it likes to. For this to work, client contents cannot control the widget size while a interactive resize is taking place. Actually, I think we need the other checks too for wl_shell clients as they are not being sent configure events so removing those will create a race condition that can result in these clients not producing contents correctly for maximized or fullscreen mode. I don't fully understand why this if-statement needs to be removed. Can you explain more why this is needed? https://codereview.chromium.org/2237983002/diff/60001/components/exo/shell_su... File components/exo/shell_surface.cc (right): https://codereview.chromium.org/2237983002/diff/60001/components/exo/shell_su... components/exo/shell_surface.cc:936: // Allow remote shell clients to update the bounds in maixmized/fullscreen nit: maximized https://codereview.chromium.org/2237983002/diff/60001/components/exo/shell_su... components/exo/shell_surface.cc:938: if (!application_id_.empty()) This is not a proper test for arc apps. application_id is used by xdg-shell clients too. initial_bounds_ and activatable_ being set is unique to arc apps today. initial_bounds_ will be set for xdg-shell popups but they are never activatable_. nit: please avoid referring to "remote shell" in the code as that interface will change name in the near future. maybe we can say "absolute positioned" shell surface in this case?
https://codereview.chromium.org/2237983002/diff/60001/ash/common/wm/window_st... File ash/common/wm/window_state.h (right): https://codereview.chromium.org/2237983002/diff/60001/ash/common/wm/window_st... ash/common/wm/window_state.h:294: void set_allow_set_bounds_in_maximized(bool value) { On 2016/08/12 16:17:31, reveman wrote: > nit: ..when_maximized or in_maximized_mode/state MAXIMIZED is used as a name of state ui code, and I'm following the same pattern. https://cs.chromium.org/chromium/src/ui/base/ui_base_types.h?rcl=0&l=20 May I keep this? (and "when" sounds like transiton, maximized_mode conflicts with maximize_mode) https://codereview.chromium.org/2237983002/diff/60001/components/exo/shell_su... File components/exo/shell_surface.cc (left): https://codereview.chromium.org/2237983002/diff/60001/components/exo/shell_su... components/exo/shell_surface.cc:1156: if (widget_->IsMaximized() || widget_->IsFullscreen() || IsResizing()) On 2016/08/12 16:17:32, reveman wrote: > I'm pretty sure we need the IsResizing part for interactive resize of wl_shell > client surfaces. That type of interactive resize works by chrome changing the > size of the widget and that generating configure events that the client can > ignore if it doesn't like the size or provide contents of a new size if it likes > to. For this to work, client contents cannot control the widget size while a > interactive resize is taking place. > I thought you wanted me to remove it. (and I looks working when I tested, both arc and wayland terminal) > Actually, I think we need the other checks too for wl_shell clients as they are > not being sent configure events so removing those will create a race condition > that can result in these clients not producing contents correctly for maximized > or fullscreen mode. > > I don't fully understand why this if-statement needs to be removed. Can you > explain more why this is needed? For arc++ apps, android controls the actual size of maximized/fullscreen, so his is causing the inconsistency between the window size that chromeos have, and what android uses, which leads to blank areas around the window in overview mode. https://codereview.chromium.org/2237983002/diff/60001/components/exo/shell_su... File components/exo/shell_surface.cc (right): https://codereview.chromium.org/2237983002/diff/60001/components/exo/shell_su... components/exo/shell_surface.cc:936: // Allow remote shell clients to update the bounds in maixmized/fullscreen On 2016/08/12 16:17:32, reveman wrote: > nit: maximized Done. https://codereview.chromium.org/2237983002/diff/60001/components/exo/shell_su... components/exo/shell_surface.cc:938: if (!application_id_.empty()) On 2016/08/12 16:17:32, reveman wrote: > This is not a proper test for arc apps. application_id is used by xdg-shell > clients too. initial_bounds_ and activatable_ being set is unique to arc apps > today. initial_bounds_ will be set for xdg-shell popups but they are never > activatable_. > > nit: please avoid referring to "remote shell" in the code as that interface will > change name in the near future. maybe we can say "absolute positioned" shell > surface in this case? Done.
Patchset #5 (id:80001) has been deleted
https://codereview.chromium.org/2237983002/diff/60001/ash/common/wm/window_st... File ash/common/wm/window_state.h (right): https://codereview.chromium.org/2237983002/diff/60001/ash/common/wm/window_st... ash/common/wm/window_state.h:294: void set_allow_set_bounds_in_maximized(bool value) { On 2016/08/12 at 23:03:33, oshima wrote: > On 2016/08/12 16:17:31, reveman wrote: > > nit: ..when_maximized or in_maximized_mode/state > > MAXIMIZED is used as a name of state ui code, and I'm following the same pattern. > > https://cs.chromium.org/chromium/src/ui/base/ui_base_types.h?rcl=0&l=20 > > May I keep this? (and "when" sounds like transiton, maximized_mode conflicts with maximize_mode) Sure https://codereview.chromium.org/2237983002/diff/60001/components/exo/shell_su... File components/exo/shell_surface.cc (left): https://codereview.chromium.org/2237983002/diff/60001/components/exo/shell_su... components/exo/shell_surface.cc:1156: if (widget_->IsMaximized() || widget_->IsFullscreen() || IsResizing()) On 2016/08/12 at 23:03:33, oshima wrote: > On 2016/08/12 16:17:32, reveman wrote: > > I'm pretty sure we need the IsResizing part for interactive resize of wl_shell > > client surfaces. That type of interactive resize works by chrome changing the > > size of the widget and that generating configure events that the client can > > ignore if it doesn't like the size or provide contents of a new size if it likes > > to. For this to work, client contents cannot control the widget size while a > > interactive resize is taking place. > > > > I thought you wanted me to remove it. (and I looks working when I tested, both arc and wayland terminal) > > > > Actually, I think we need the other checks too for wl_shell clients as they are > > not being sent configure events so removing those will create a race condition > > that can result in these clients not producing contents correctly for maximized > > or fullscreen mode. > > > > I don't fully understand why this if-statement needs to be removed. Can you > > explain more why this is needed? > > For arc++ apps, android controls the actual size of maximized/fullscreen, so his is causing the inconsistency between the window size that chromeos have, and what android uses, which leads to blank areas around the window in overview mode. Sorry for the confusion. In my initial comment I was trying to understand how this does not break wl_shell and xdg_shell clients. Removing IsResizing definitely does so we need to keep that for sure. Here's the sequence of events that can be a problem for wl-shell clients: 1. client produces a new frame in non-maximized mode and starts transmitting it to chrome 2. chrome maximizes the window before processing the new frame from the client 2.1. chrome changes the size of the widget to the ideal maximized size 2.2. chrome sends configure event 1 to the client with the new maximized state and suggested size 3. chrome receives the new (non-maximized) frame from the client and updates the widget size as a result of this CL (window is now maximized but widget size is same as before) 4. chrome moves focus to a different window 4.1. the move of focus results in chrome sending configure event 2 to the client with the current size of the widget and new active state 5. client receives configure event 1 and produces a frame for the ideal maximized size 6. client receives configure event 2 and produces a frame for the non-maximized size even though the window is still maximized (BAD) this might be limited to wl-shell clients and not xdg-shell clients as the serials in configure events should help in that case. anyhow, to remove these checks as this CL does we'd need to keep track of the ideal maximized size so we can send that instead of the widget size in configure events when a window is maximized. I think the easiest solution is to just make these checks depend on initial_bounds_ not being set. that makes sense to me as when initial_bounds_ is set we shouldn't really be changing widget bounds at all and the bounds should always be controlled by the contents produced by the client. Change that and this CL looks good. https://codereview.chromium.org/2237983002/diff/140001/components/exo/shell_s... File components/exo/shell_surface.cc (right): https://codereview.chromium.org/2237983002/diff/140001/components/exo/shell_s... components/exo/shell_surface.cc:1163: if (((widget_->IsMaximized() || widget_->IsFullscreen()) && please split this into multiple early outs. something like: if (!initial_bounds_.IsEmpty() && window_state->IsMaximizedOrFullscreenOrPinned()) return; if (IsResizing()) return; you can use allow_set_bounds_in_maximized instead of !initial_bounds_.IsEmpty() if you like. either is fine as non-arc windows with that should never be maximized or made fullscren.
https://codereview.chromium.org/2237983002/diff/60001/components/exo/shell_su... File components/exo/shell_surface.cc (left): https://codereview.chromium.org/2237983002/diff/60001/components/exo/shell_su... components/exo/shell_surface.cc:1156: if (widget_->IsMaximized() || widget_->IsFullscreen() || IsResizing()) On 2016/08/12 23:44:40, reveman wrote: > On 2016/08/12 at 23:03:33, oshima wrote: > > On 2016/08/12 16:17:32, reveman wrote: > > > I'm pretty sure we need the IsResizing part for interactive resize of > wl_shell > > > client surfaces. That type of interactive resize works by chrome changing > the > > > size of the widget and that generating configure events that the client can > > > ignore if it doesn't like the size or provide contents of a new size if it > likes > > > to. For this to work, client contents cannot control the widget size while a > > > interactive resize is taking place. > > > > > > > I thought you wanted me to remove it. (and I looks working when I tested, both > arc and wayland terminal) > > > > > > > Actually, I think we need the other checks too for wl_shell clients as they > are > > > not being sent configure events so removing those will create a race > condition > > > that can result in these clients not producing contents correctly for > maximized > > > or fullscreen mode. > > > > > > I don't fully understand why this if-statement needs to be removed. Can you > > > explain more why this is needed? > > > > For arc++ apps, android controls the actual size of maximized/fullscreen, so > his is causing the inconsistency between the window size that chromeos have, and > what android uses, which leads to blank areas around the window in overview > mode. > > Sorry for the confusion. In my initial comment I was trying to understand how > this does not break wl_shell and xdg_shell clients. Removing IsResizing > definitely does so we need to keep that for sure. > > Here's the sequence of events that can be a problem for wl-shell clients: > > 1. client produces a new frame in non-maximized mode and starts transmitting it > to chrome > 2. chrome maximizes the window before processing the new frame from the client > 2.1. chrome changes the size of the widget to the ideal maximized size > 2.2. chrome sends configure event 1 to the client with the new maximized state > and suggested size > 3. chrome receives the new (non-maximized) frame from the client and updates the > widget size as a result of this CL (window is now maximized but widget size is > same as before) > 4. chrome moves focus to a different window > 4.1. the move of focus results in chrome sending configure event 2 to the > client with the current size of the widget and new active state > 5. client receives configure event 1 and produces a frame for the ideal > maximized size > 6. client receives configure event 2 and produces a frame for the non-maximized > size even though the window is still maximized (BAD) Note that it will only happens when this flag is set. That is, SetBounds will not update the widget bounds in maximized/fullscreen state unless this flag is set. I agree that checking would be safer. > this might be limited to wl-shell clients and not xdg-shell clients as the > serials in configure events should help in that case. > > anyhow, to remove these checks as this CL does we'd need to keep track of the > ideal maximized size so we can send that instead of the widget size in configure > events when a window is maximized. > > I think the easiest solution is to just make these checks depend on > initial_bounds_ not being set. that makes sense to me as when initial_bounds_ is > set we shouldn't really be changing widget bounds at all and the bounds should > always be controlled by the contents produced by the client. Change that and > this CL looks good. https://codereview.chromium.org/2237983002/diff/140001/components/exo/shell_s... File components/exo/shell_surface.cc (right): https://codereview.chromium.org/2237983002/diff/140001/components/exo/shell_s... components/exo/shell_surface.cc:1163: if (((widget_->IsMaximized() || widget_->IsFullscreen()) && On 2016/08/12 23:44:40, reveman wrote: > please split this into multiple early outs. something like: > > if (!initial_bounds_.IsEmpty() && > window_state->IsMaximizedOrFullscreenOrPinned()) > return; > > if (IsResizing()) > return; > > > you can use allow_set_bounds_in_maximized instead of !initial_bounds_.IsEmpty() > if you like. either is fine as non-arc windows with that should never be > maximized or made fullscren. splitting done. I still think it's better to use allow_set_bound_in_maximzied() because that's exact condition that bounds have to be updated in these state. or I can change the line 939 to if (!initial_bounds.IsEmtpy()) window_state->set_allow_set_bounds_in_maximized(true); because popup will never be maximized nor fullscreen. WDYT?
Patchset #8 (id:160001) has been deleted
Patchset #9 (id:200001) has been deleted
The CQ bit was checked by oshima@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...
On 2016/08/13 at 00:45:07, oshima wrote: > https://codereview.chromium.org/2237983002/diff/60001/components/exo/shell_su... > File components/exo/shell_surface.cc (left): > > https://codereview.chromium.org/2237983002/diff/60001/components/exo/shell_su... > components/exo/shell_surface.cc:1156: if (widget_->IsMaximized() || widget_->IsFullscreen() || IsResizing()) > On 2016/08/12 23:44:40, reveman wrote: > > On 2016/08/12 at 23:03:33, oshima wrote: > > > On 2016/08/12 16:17:32, reveman wrote: > > > > I'm pretty sure we need the IsResizing part for interactive resize of > > wl_shell > > > > client surfaces. That type of interactive resize works by chrome changing > > the > > > > size of the widget and that generating configure events that the client can > > > > ignore if it doesn't like the size or provide contents of a new size if it > > likes > > > > to. For this to work, client contents cannot control the widget size while a > > > > interactive resize is taking place. > > > > > > > > > > I thought you wanted me to remove it. (and I looks working when I tested, both > > arc and wayland terminal) > > > > > > > > > > Actually, I think we need the other checks too for wl_shell clients as they > > are > > > > not being sent configure events so removing those will create a race > > condition > > > > that can result in these clients not producing contents correctly for > > maximized > > > > or fullscreen mode. > > > > > > > > I don't fully understand why this if-statement needs to be removed. Can you > > > > explain more why this is needed? > > > > > > For arc++ apps, android controls the actual size of maximized/fullscreen, so > > his is causing the inconsistency between the window size that chromeos have, and > > what android uses, which leads to blank areas around the window in overview > > mode. > > > > Sorry for the confusion. In my initial comment I was trying to understand how > > this does not break wl_shell and xdg_shell clients. Removing IsResizing > > definitely does so we need to keep that for sure. > > > > Here's the sequence of events that can be a problem for wl-shell clients: > > > > 1. client produces a new frame in non-maximized mode and starts transmitting it > > to chrome > > 2. chrome maximizes the window before processing the new frame from the client > > 2.1. chrome changes the size of the widget to the ideal maximized size > > 2.2. chrome sends configure event 1 to the client with the new maximized state > > and suggested size > > 3. chrome receives the new (non-maximized) frame from the client and updates the > > widget size as a result of this CL (window is now maximized but widget size is > > same as before) > > 4. chrome moves focus to a different window > > 4.1. the move of focus results in chrome sending configure event 2 to the > > client with the current size of the widget and new active state > > 5. client receives configure event 1 and produces a frame for the ideal > > maximized size > > 6. client receives configure event 2 and produces a frame for the non-maximized > > size even though the window is still maximized (BAD) > > Note that it will only happens when this flag is set. That is, SetBounds will not update the > widget bounds in maximized/fullscreen state unless this flag is set. > > I agree that checking would be safer. > > > this might be limited to wl-shell clients and not xdg-shell clients as the > > serials in configure events should help in that case. > > > > anyhow, to remove these checks as this CL does we'd need to keep track of the > > ideal maximized size so we can send that instead of the widget size in configure > > events when a window is maximized. > > > > I think the easiest solution is to just make these checks depend on > > initial_bounds_ not being set. that makes sense to me as when initial_bounds_ is > > set we shouldn't really be changing widget bounds at all and the bounds should > > always be controlled by the contents produced by the client. Change that and > > this CL looks good. > > https://codereview.chromium.org/2237983002/diff/140001/components/exo/shell_s... > File components/exo/shell_surface.cc (right): > > https://codereview.chromium.org/2237983002/diff/140001/components/exo/shell_s... > components/exo/shell_surface.cc:1163: if (((widget_->IsMaximized() || widget_->IsFullscreen()) && > On 2016/08/12 23:44:40, reveman wrote: > > please split this into multiple early outs. something like: > > > > if (!initial_bounds_.IsEmpty() && > > window_state->IsMaximizedOrFullscreenOrPinned()) > > return; > > > > if (IsResizing()) > > return; > > > > > > you can use allow_set_bounds_in_maximized instead of !initial_bounds_.IsEmpty() > > if you like. either is fine as non-arc windows with that should never be > > maximized or made fullscren. > > splitting done. I still think it's better to use allow_set_bound_in_maximzied() because that's exact condition that bounds have to be updated in these state. > > or I can change the line 939 to > > if (!initial_bounds.IsEmtpy()) > window_state->set_allow_set_bounds_in_maximized(true); > > because popup will never be maximized nor fullscreen. > > WDYT? yes, changing line 939 to that sgtm.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_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 oshima@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...
On 2016/08/13 01:18:02, reveman wrote: > On 2016/08/13 at 00:45:07, oshima wrote: > > > https://codereview.chromium.org/2237983002/diff/60001/components/exo/shell_su... > > File components/exo/shell_surface.cc (left): > > > > > https://codereview.chromium.org/2237983002/diff/60001/components/exo/shell_su... > > components/exo/shell_surface.cc:1156: if (widget_->IsMaximized() || > widget_->IsFullscreen() || IsResizing()) > > On 2016/08/12 23:44:40, reveman wrote: > > > On 2016/08/12 at 23:03:33, oshima wrote: > > > > On 2016/08/12 16:17:32, reveman wrote: > > > > > I'm pretty sure we need the IsResizing part for interactive resize of > > > wl_shell > > > > > client surfaces. That type of interactive resize works by chrome > changing > > > the > > > > > size of the widget and that generating configure events that the client > can > > > > > ignore if it doesn't like the size or provide contents of a new size if > it > > > likes > > > > > to. For this to work, client contents cannot control the widget size > while a > > > > > interactive resize is taking place. > > > > > > > > > > > > > I thought you wanted me to remove it. (and I looks working when I tested, > both > > > arc and wayland terminal) > > > > > > > > > > > > > Actually, I think we need the other checks too for wl_shell clients as > they > > > are > > > > > not being sent configure events so removing those will create a race > > > condition > > > > > that can result in these clients not producing contents correctly for > > > maximized > > > > > or fullscreen mode. > > > > > > > > > > I don't fully understand why this if-statement needs to be removed. Can > you > > > > > explain more why this is needed? > > > > > > > > For arc++ apps, android controls the actual size of maximized/fullscreen, > so > > > his is causing the inconsistency between the window size that chromeos have, > and > > > what android uses, which leads to blank areas around the window in overview > > > mode. > > > > > > Sorry for the confusion. In my initial comment I was trying to understand > how > > > this does not break wl_shell and xdg_shell clients. Removing IsResizing > > > definitely does so we need to keep that for sure. > > > > > > Here's the sequence of events that can be a problem for wl-shell clients: > > > > > > 1. client produces a new frame in non-maximized mode and starts transmitting > it > > > to chrome > > > 2. chrome maximizes the window before processing the new frame from the > client > > > 2.1. chrome changes the size of the widget to the ideal maximized size > > > 2.2. chrome sends configure event 1 to the client with the new maximized > state > > > and suggested size > > > 3. chrome receives the new (non-maximized) frame from the client and updates > the > > > widget size as a result of this CL (window is now maximized but widget size > is > > > same as before) > > > 4. chrome moves focus to a different window > > > 4.1. the move of focus results in chrome sending configure event 2 to the > > > client with the current size of the widget and new active state > > > 5. client receives configure event 1 and produces a frame for the ideal > > > maximized size > > > 6. client receives configure event 2 and produces a frame for the > non-maximized > > > size even though the window is still maximized (BAD) > > > > Note that it will only happens when this flag is set. That is, SetBounds will > not update the > > widget bounds in maximized/fullscreen state unless this flag is set. > > > > I agree that checking would be safer. > > > > > this might be limited to wl-shell clients and not xdg-shell clients as the > > > serials in configure events should help in that case. > > > > > > anyhow, to remove these checks as this CL does we'd need to keep track of > the > > > ideal maximized size so we can send that instead of the widget size in > configure > > > events when a window is maximized. > > > > > > I think the easiest solution is to just make these checks depend on > > > initial_bounds_ not being set. that makes sense to me as when > initial_bounds_ is > > > set we shouldn't really be changing widget bounds at all and the bounds > should > > > always be controlled by the contents produced by the client. Change that and > > > this CL looks good. > > > > > https://codereview.chromium.org/2237983002/diff/140001/components/exo/shell_s... > > File components/exo/shell_surface.cc (right): > > > > > https://codereview.chromium.org/2237983002/diff/140001/components/exo/shell_s... > > components/exo/shell_surface.cc:1163: if (((widget_->IsMaximized() || > widget_->IsFullscreen()) && > > On 2016/08/12 23:44:40, reveman wrote: > > > please split this into multiple early outs. something like: > > > > > > if (!initial_bounds_.IsEmpty() && > > > window_state->IsMaximizedOrFullscreenOrPinned()) > > > return; > > > > > > if (IsResizing()) > > > return; > > > > > > > > > you can use allow_set_bounds_in_maximized instead of > !initial_bounds_.IsEmpty() > > > if you like. either is fine as non-arc windows with that should never be > > > maximized or made fullscren. > > > > splitting done. I still think it's better to use > allow_set_bound_in_maximzied() because that's exact condition that bounds have > to be updated in these state. > > > > or I can change the line 939 to > > > > if (!initial_bounds.IsEmtpy()) > > window_state->set_allow_set_bounds_in_maximized(true); > > > > because popup will never be maximized nor fullscreen. > > > > WDYT? > > yes, changing line 939 to that sgtm. Done
lgtm with nits https://codereview.chromium.org/2237983002/diff/230001/components/exo/shell_s... File components/exo/shell_surface.cc (right): https://codereview.chromium.org/2237983002/diff/230001/components/exo/shell_s... components/exo/shell_surface.cc:1164: // isn't controlled by a clientt. nit: client https://codereview.chromium.org/2237983002/diff/230001/components/exo/shell_s... components/exo/shell_surface.cc:1165: // 2) When a window is being dragged. nit: move this comment below to the IsResizing early out https://codereview.chromium.org/2237983002/diff/230001/components/exo/shell_s... components/exo/shell_surface.cc:1175: } nit: no need for {} here
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2237983002/diff/230001/components/exo/shell_s... File components/exo/shell_surface.cc (right): https://codereview.chromium.org/2237983002/diff/230001/components/exo/shell_s... components/exo/shell_surface.cc:1164: // isn't controlled by a clientt. On 2016/08/15 16:19:37, reveman wrote: > nit: client Done. https://codereview.chromium.org/2237983002/diff/230001/components/exo/shell_s... components/exo/shell_surface.cc:1165: // 2) When a window is being dragged. On 2016/08/15 16:19:37, reveman wrote: > nit: move this comment below to the IsResizing early out Done. https://codereview.chromium.org/2237983002/diff/230001/components/exo/shell_s... components/exo/shell_surface.cc:1175: } On 2016/08/15 16:19:37, reveman wrote: > nit: no need for {} here Done.
The CQ bit was checked by oshima@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from reveman@chromium.org Link to the patchset: https://codereview.chromium.org/2237983002/#ps250001 (title: "addressed comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Allow updating arc widget bounds in maximize/fullscreen state. ARC window can be pillar/letterboxed in maximized state, and chrome needs to adjust its window to those sizes. BUG=634572 R=reveman@chromium.org TEST=WindowStateTest.AllowSetBoundsInMaximized Manually tested on minnie ========== to ========== Allow updating arc widget bounds in maximize/fullscreen state. ARC window can be pillar/letterboxed in maximized state, and chrome needs to adjust its window to those sizes. BUG=634572 R=reveman@chromium.org TEST=WindowStateTest.AllowSetBoundsInMaximized Manually tested on minnie ==========
Message was sent while issue was closed.
Committed patchset #11 (id:250001)
Message was sent while issue was closed.
Description was changed from ========== Allow updating arc widget bounds in maximize/fullscreen state. ARC window can be pillar/letterboxed in maximized state, and chrome needs to adjust its window to those sizes. BUG=634572 R=reveman@chromium.org TEST=WindowStateTest.AllowSetBoundsInMaximized Manually tested on minnie ========== to ========== Allow updating arc widget bounds in maximize/fullscreen state. ARC window can be pillar/letterboxed in maximized state, and chrome needs to adjust its window to those sizes. BUG=634572 R=reveman@chromium.org TEST=WindowStateTest.AllowSetBoundsInMaximized Manually tested on minnie Committed: https://crrev.com/e68d9c3c4f704de541d5f796aec4c0bd82e7ee7b Cr-Commit-Position: refs/heads/master@{#412006} ==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/e68d9c3c4f704de541d5f796aec4c0bd82e7ee7b Cr-Commit-Position: refs/heads/master@{#412006} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
