Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2679403003:
exo: ShellSurface: Remove the CustomWindowStateDelegate (Closed)
Descriptionexo: ShellSurface: Remove the CustomWindowStateDelegate
With CL [1], the ash will restore previous show state after
exiting fullscreen. So we don't need the CustomWindowStateDelegate
to restore the previous show state anymore.
[1] https://codereview.chromium.org/2622053004
BUG=670496
Review-Url: https://codereview.chromium.org/2679403003
Cr-Commit-Position: refs/heads/master@{#449748}
Committed: https://chromium.googlesource.com/chromium/src/+/7d1ee017dbdb0d8e3890d40891dbb52ac53ed84f
Patch Set 1 #
Total comments: 3
Patch Set 2 : Update #
Total comments: 2
Patch Set 3 : Fix review issue #Messages
Total messages: 23 (14 generated)
penghuang@chromium.org changed reviewers: + oshima@chromium.org
Hi Oshima, PTAL. Thanks.
The CQ bit was checked by penghuang@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.
https://codereview.chromium.org/2679403003/diff/1/components/exo/shell_surfac... File components/exo/shell_surface.cc (left): https://codereview.chromium.org/2679403003/diff/1/components/exo/shell_surfac... components/exo/shell_surface.cc:149: class CustomWindowStateDelegate : public ash::wm::WindowStateDelegate, I believe we still need this to go fullscreen using F4 key and set the immersive flag.
Mu apologies, I missed the last change. lgtm with a nit. https://codereview.chromium.org/2679403003/diff/1/components/exo/shell_surfac... File components/exo/shell_surface.cc (right): https://codereview.chromium.org/2679403003/diff/1/components/exo/shell_surfac... components/exo/shell_surface.cc:798: window->SetProperty(aura::client::kImmersiveFullscreenKey, true); Please add comment that we assume the exo windows is always immersive (unlike browser window which can become both).
https://codereview.chromium.org/2679403003/diff/1/components/exo/shell_surfac... File components/exo/shell_surface.cc (right): https://codereview.chromium.org/2679403003/diff/1/components/exo/shell_surfac... components/exo/shell_surface.cc:798: window->SetProperty(aura::client::kImmersiveFullscreenKey, true); On 2017/02/09 19:38:59, oshima wrote: > Please add comment that we assume the exo windows is always immersive (unlike > browser window which can become both). Done.
The CQ bit was checked by penghuang@chromium.org to run a CQ dry run
penghuang@chromium.org changed reviewers: + reveman@chromium.org
Hi David, PTAL. Thanks.
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.
LGTM % nit https://codereview.chromium.org/2679403003/diff/20001/components/exo/shell_su... File components/exo/shell_surface.cc (right): https://codereview.chromium.org/2679403003/diff/20001/components/exo/shell_su... components/exo/shell_surface.cc:798: // We assume all exo windows use immersive fullscreen mode. Nit: Remove 'assume' and reference to exo. I.e. "ShellSurfaces always use immersive mode."
https://codereview.chromium.org/2679403003/diff/20001/components/exo/shell_su... File components/exo/shell_surface.cc (right): https://codereview.chromium.org/2679403003/diff/20001/components/exo/shell_su... components/exo/shell_surface.cc:798: // We assume all exo windows use immersive fullscreen mode. On 2017/02/10 17:35:58, reveman wrote: > Nit: Remove 'assume' and reference to exo. I.e. "ShellSurfaces always use > immersive mode." Done.
The CQ bit was checked by penghuang@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from oshima@chromium.org, reveman@chromium.org Link to the patchset: https://codereview.chromium.org/2679403003/#ps40001 (title: "Fix review issue")
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": 1486759303002700,
"parent_rev": "0ae853677c3c509f63606dda265476175c0b11e7", "commit_rev":
"7d1ee017dbdb0d8e3890d40891dbb52ac53ed84f"}
Message was sent while issue was closed.
Description was changed from ========== exo: ShellSurface: Remove the CustomWindowStateDelegate With CL [1], the ash will restore previous show state after exiting fullscreen. So we don't need the CustomWindowStateDelegate to restore the previous show state anymore. [1] https://codereview.chromium.org/2622053004 BUG=670496 ========== to ========== exo: ShellSurface: Remove the CustomWindowStateDelegate With CL [1], the ash will restore previous show state after exiting fullscreen. So we don't need the CustomWindowStateDelegate to restore the previous show state anymore. [1] https://codereview.chromium.org/2622053004 BUG=670496 Review-Url: https://codereview.chromium.org/2679403003 Cr-Commit-Position: refs/heads/master@{#449748} Committed: https://chromium.googlesource.com/chromium/src/+/7d1ee017dbdb0d8e3890d40891db... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/7d1ee017dbdb0d8e3890d40891db... |
