Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2837903002:
Do not hide shadow underlay for max/fullscreen state even if the client disables the shadow. (Closed)
DescriptionDo not hide shadow underlay for max/fullscreen state even if the client disables the shadow.
* Make sure resets all shadow layers when the switching between deprecated shadow.
* Don't try to show the window that is already visible.
This is reland of crrev.com/2820493004 with a fix for crbug.com/713560
R=reveman@chromium.org
BUG=BUG=711514
TEST=covered by unit tests.
manual. Instal & start "Clash Royal", then F4 to toggle fullscreen.
Tested on MNC and NYC devices as well.
Review-Url: https://codereview.chromium.org/2837903002
Cr-Commit-Position: refs/heads/master@{#467485}
Committed: https://chromium.googlesource.com/chromium/src/+/27b41b0b6fb0317aa944f2bdf7e4879628aab83c
Patch Set 1 #
Total comments: 1
Patch Set 2 : Do not hide shadow underlay for max/fullscreen state even if the client disables the shadow. #
Total comments: 9
Patch Set 3 : addressed comments #
Total comments: 12
Patch Set 4 : addressed comments #Patch Set 5 : cleanup #
Messages
Total messages: 22 (13 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...
https://codereview.chromium.org/2837903002/diff/1/components/exo/shell_surfac... File components/exo/shell_surface.cc (right): https://codereview.chromium.org/2837903002/diff/1/components/exo/shell_surfac... components/exo/shell_surface.cc:631: ResetShadowWindows(); It was crashing because we were mixing overlay created for "in_surface", but then created underlay for "not in surface" mode. This CL resets these layers when switching between the mode.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
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: This issue passed the CQ dry run.
Description was changed from ========== Do not hide shadow underlay for max/fullscreen state even if the client disables the shadow. Make sure resets all shadow layers when the switching between deprecated shadow. This is reland of crrev.com/2820493004 with a fix for crbug.com/713560 R=reveman@chromium.org BUG=BUG=711514 TEST=covered by unit tests. manual. Instal & start "Clash Royal", then F4 to toggle fullscreen. Tested on MNC and NYC devices as well. ========== to ========== Do not hide shadow underlay for max/fullscreen state even if the client disables the shadow. * Make sure resets all shadow layers when the switching between deprecated shadow. * Don't try to show the window that is already visible. This is reland of crrev.com/2820493004 with a fix for crbug.com/713560 R=reveman@chromium.org BUG=BUG=711514 TEST=covered by unit tests. manual. Instal & start "Clash Royal", then F4 to toggle fullscreen. Tested on MNC and NYC devices as well. ==========
https://codereview.chromium.org/2837903002/diff/20001/components/exo/shell_su... File components/exo/shell_surface.cc (right): https://codereview.chromium.org/2837903002/diff/20001/components/exo/shell_su... components/exo/shell_surface.cc:1689: if (!shadow_underlay_->IsVisible()) Not sure why I missed the original CL, but this is additional change to avoid DCHECK failure.
https://codereview.chromium.org/2837903002/diff/20001/components/exo/shell_su... File components/exo/shell_surface.cc (right): https://codereview.chromium.org/2837903002/diff/20001/components/exo/shell_su... components/exo/shell_surface.cc:631: ResetShadowWindows(); Should these calls be deferred until commit? What if the client is waiting a significant amount of time before commit? Will it cause artifacts to do it here? https://codereview.chromium.org/2837903002/diff/20001/components/exo/shell_su... components/exo/shell_surface.cc:642: ResetShadowWindows(); ditto https://codereview.chromium.org/2837903002/diff/20001/components/exo/shell_su... components/exo/shell_surface.cc:653: ResetShadowWindows(); ditto https://codereview.chromium.org/2837903002/diff/20001/components/exo/shell_su... components/exo/shell_surface.cc:886: ResetShadowWindows(); is this needed? prior we did this to avoid leaving pointers around that point to instances that have been destroyed but now that we own them..
https://codereview.chromium.org/2837903002/diff/20001/components/exo/shell_su... File components/exo/shell_surface.cc (right): https://codereview.chromium.org/2837903002/diff/20001/components/exo/shell_su... components/exo/shell_surface.cc:631: ResetShadowWindows(); On 2017/04/25 18:04:40, reveman wrote: > Should these calls be deferred until commit? What if the client is waiting a > significant amount of time before commit? Will it cause artifacts to do it here? > Good point. Done. https://codereview.chromium.org/2837903002/diff/20001/components/exo/shell_su... components/exo/shell_surface.cc:886: ResetShadowWindows(); On 2017/04/25 18:04:40, reveman wrote: > is this needed? prior we did this to avoid leaving pointers around that point to > instances that have been destroyed but now that we own them.. This is just for the safety because they'll never be and can't be reused, but I'm fine to remove if you prefer. LMK.
lgtm with nits https://codereview.chromium.org/2837903002/diff/20001/components/exo/shell_su... File components/exo/shell_surface.cc (right): https://codereview.chromium.org/2837903002/diff/20001/components/exo/shell_su... components/exo/shell_surface.cc:886: ResetShadowWindows(); On 2017/04/26 at 16:36:11, oshima wrote: > On 2017/04/25 18:04:40, reveman wrote: > > is this needed? prior we did this to avoid leaving pointers around that point to > > instances that have been destroyed but now that we own them.. > > This is just for the safety because they'll never be and can't be reused, but I'm fine to remove if you prefer. LMK. Let's remove it if not needed as then there's also no need to add a ResetShadowWindows function https://codereview.chromium.org/2837903002/diff/40001/components/exo/shell_su... File components/exo/shell_surface.cc (right): https://codereview.chromium.org/2837903002/diff/40001/components/exo/shell_su... components/exo/shell_surface.cc:630: reset_shadow_windows_ |= shadow_underlay_in_surface_; nit: group with lines below instead of with the trace event https://codereview.chromium.org/2837903002/diff/40001/components/exo/shell_su... components/exo/shell_surface.cc:640: reset_shadow_windows_ |= shadow_underlay_in_surface_; nit: ditto https://codereview.chromium.org/2837903002/diff/40001/components/exo/shell_su... components/exo/shell_surface.cc:651: reset_shadow_windows_ |= !shadow_underlay_in_surface_; nit: ditto https://codereview.chromium.org/2837903002/diff/40001/components/exo/shell_su... components/exo/shell_surface.cc:885: ResetShadowWindows(); nit: remove https://codereview.chromium.org/2837903002/diff/40001/components/exo/shell_su... components/exo/shell_surface.cc:1746: void ShellSurface::ResetShadowWindows() { nit: remove this function as the only time we need to reset these is in ShellSurface::UpdateSurfaceBounds https://codereview.chromium.org/2837903002/diff/40001/components/exo/shell_su... File components/exo/shell_surface.h (right): https://codereview.chromium.org/2837903002/diff/40001/components/exo/shell_su... components/exo/shell_surface.h:338: void ResetShadowWindows(); nit: remove this after removing the reset call from WindowClosing https://codereview.chromium.org/2837903002/diff/40001/components/exo/shell_su... components/exo/shell_surface.h:382: bool reset_shadow_windows_ = false; nit: "bool pending_shadow_underlay_in_surface_ = true;" and move below l.380 and then "if (pending_shadow_underlay_in_surface_ != shadow_underlay_in_surface_)" to determine if we need to re-create windows. I think that's more consistent with other pending state changes and avoids unnecessary reset if changed back before commit
https://codereview.chromium.org/2837903002/diff/20001/components/exo/shell_su... File components/exo/shell_surface.cc (right): https://codereview.chromium.org/2837903002/diff/20001/components/exo/shell_su... components/exo/shell_surface.cc:886: ResetShadowWindows(); On 2017/04/26 18:00:56, reveman wrote: > On 2017/04/26 at 16:36:11, oshima wrote: > > On 2017/04/25 18:04:40, reveman wrote: > > > is this needed? prior we did this to avoid leaving pointers around that > point to > > > instances that have been destroyed but now that we own them.. > > > > This is just for the safety because they'll never be and can't be reused, but > I'm fine to remove if you prefer. LMK. > > Let's remove it if not needed as then there's also no need to add a > ResetShadowWindows function Done. https://codereview.chromium.org/2837903002/diff/40001/components/exo/shell_su... File components/exo/shell_surface.cc (right): https://codereview.chromium.org/2837903002/diff/40001/components/exo/shell_su... components/exo/shell_surface.cc:630: reset_shadow_windows_ |= shadow_underlay_in_surface_; On 2017/04/26 18:00:56, reveman wrote: > nit: group with lines below instead of with the trace event Done. https://codereview.chromium.org/2837903002/diff/40001/components/exo/shell_su... components/exo/shell_surface.cc:885: ResetShadowWindows(); On 2017/04/26 18:00:56, reveman wrote: > nit: remove Done. https://codereview.chromium.org/2837903002/diff/40001/components/exo/shell_su... components/exo/shell_surface.cc:1746: void ShellSurface::ResetShadowWindows() { On 2017/04/26 18:00:56, reveman wrote: > nit: remove this function as the only time we need to reset these is in > ShellSurface::UpdateSurfaceBounds Done. https://codereview.chromium.org/2837903002/diff/40001/components/exo/shell_su... File components/exo/shell_surface.h (right): https://codereview.chromium.org/2837903002/diff/40001/components/exo/shell_su... components/exo/shell_surface.h:338: void ResetShadowWindows(); On 2017/04/26 18:00:56, reveman wrote: > nit: remove this after removing the reset call from WindowClosing Done. https://codereview.chromium.org/2837903002/diff/40001/components/exo/shell_su... components/exo/shell_surface.h:382: bool reset_shadow_windows_ = false; On 2017/04/26 18:00:56, reveman wrote: > nit: "bool pending_shadow_underlay_in_surface_ = true;" and move below l.380 and > then "if (pending_shadow_underlay_in_surface_ != shadow_underlay_in_surface_)" > to determine if we need to re-create windows. I think that's more consistent > with other pending state changes and avoids unnecessary reset if changed back > before commit 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/2837903002/#ps80001 (title: "cleanup")
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": 80001, "attempt_start_ts": 1493243375511650,
"parent_rev": "0088ee5017f4bef6c27243a54d8998f993db11b8", "commit_rev":
"27b41b0b6fb0317aa944f2bdf7e4879628aab83c"}
Message was sent while issue was closed.
Description was changed from ========== Do not hide shadow underlay for max/fullscreen state even if the client disables the shadow. * Make sure resets all shadow layers when the switching between deprecated shadow. * Don't try to show the window that is already visible. This is reland of crrev.com/2820493004 with a fix for crbug.com/713560 R=reveman@chromium.org BUG=BUG=711514 TEST=covered by unit tests. manual. Instal & start "Clash Royal", then F4 to toggle fullscreen. Tested on MNC and NYC devices as well. ========== to ========== Do not hide shadow underlay for max/fullscreen state even if the client disables the shadow. * Make sure resets all shadow layers when the switching between deprecated shadow. * Don't try to show the window that is already visible. This is reland of crrev.com/2820493004 with a fix for crbug.com/713560 R=reveman@chromium.org BUG=BUG=711514 TEST=covered by unit tests. manual. Instal & start "Clash Royal", then F4 to toggle fullscreen. Tested on MNC and NYC devices as well. Review-Url: https://codereview.chromium.org/2837903002 Cr-Commit-Position: refs/heads/master@{#467485} Committed: https://chromium.googlesource.com/chromium/src/+/27b41b0b6fb0317aa944f2bdf7e4... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/27b41b0b6fb0317aa944f2bdf7e4... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
