Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2470343003:
Use mirror layer for minimized windows in overview mode (Closed)
DescriptionUse mirror layer for minimized windows in overview mode
* This CL changes overview mode to use the mirror layer for minimized windows (normal minimized, docked minimized).
* The mirror window already excludes header, so do not adjust the top insets.
* Reposition windows when a window gets minimized/unminimized.
BUG=654067
TEST=covered by unit tests.
Committed: https://crrev.com/49d6f138d9c18c3c8843cddf43f06d4c0b484578
Cr-Commit-Position: refs/heads/master@{#432537}
Patch Set 1 #
Total comments: 9
Patch Set 2 : Use mirror layer for minimized windows in overview mode #Patch Set 3 : Use mirror layer for minimized windows in overview mode #Patch Set 4 : Use mirror layer for minimized windows in overview mode #
Total comments: 8
Patch Set 5 : Use mirror layer for minimized windows in overview mode #Patch Set 6 : Use mirror layer for minimized windows in overview mode #Messages
Total messages: 44 (31 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...
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 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.
Patchset #7 (id:120001) has been deleted
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:60001) has been deleted
Patchset #1 (id:80001) has been deleted
Patchset #1 (id:100001) has been deleted
Patchset #1 (id:140001) has been deleted
Description was changed from ========== Use mirror layer for minimized windows in overview mode BUG=654067 ========== to ========== Use mirror layer for minimized windows in overview mode * This CL changes overview mode to use the mirror layer for minimized windows (normal minimized, docked minimized). * The mirror window already excludes header, so do not adjust the top insets. * BUG=654067 TEST=covered by unit tests. ==========
Description was changed from ========== Use mirror layer for minimized windows in overview mode * This CL changes overview mode to use the mirror layer for minimized windows (normal minimized, docked minimized). * The mirror window already excludes header, so do not adjust the top insets. * BUG=654067 TEST=covered by unit tests. ========== to ========== Use mirror layer for minimized windows in overview mode * This CL changes overview mode to use the mirror layer for minimized windows (normal minimized, docked minimized). * The mirror window already excludes header, so do not adjust the top insets. * Reposition windows when a window gets minimized/unminimized. BUG=654067 TEST=covered by unit tests. ==========
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...
oshima@chromium.org changed reviewers: + varkha@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
So for the example in the comment https://bugs.chromium.org/p/chromium/issues/detail?id=654067#c3 - is the intention now to have a page stay paused? With this patch upon entering overview while the page is minimized (and paused) the overview header says "Playing" but the page's tab caption says "Paused" (and the video plays while overview is active). Alt+Tab is doing the same. Is the intention to have the video in a minimized browser window paused while in Alt+Tab (I would think playing it at least while overview / Alt+Tab focus is on that page is desirable no?
Not sure if you had the web apps running in a window in mind - this is the case when the headers get hidden, so this is not just about ARC windows. I've also noticed this bug with the docked windows (new with this CL): - Have 2 V1 apps (web links set to run in a window) - Have a short host screen so docking one window will displace and minimize the other - Dock the first one (gets docked) - Dock the second one (gets docked and minimizes the first window) - Enter overview - select the first (minimized) - the second window is displaced and loses its header (permanently). So hide / unhide the header is not symmetric somewhere. https://codereview.chromium.org/2470343003/diff/160001/ash/common/wm/overview... File ash/common/wm/overview/scoped_transform_overview_window.cc (right): https://codereview.chromium.org/2470343003/diff/160001/ash/common/wm/overview... ash/common/wm/overview/scoped_transform_overview_window.cc:282: return 0; I don't think this is right. I tried it with the web browser running in a window (menu->More tools->Add to shelf...) and I now see both the overview header and the original scaled window header when in overview. I expected to only see the fake overview header. This might be the right thing for ARC, are you able to try that and maybe restrict to just ARC, say by manipulating what this property returns in exo when a window is minimized? https://codereview.chromium.org/2470343003/diff/160001/ash/common/wm/overview... ash/common/wm/overview/scoped_transform_overview_window.cc:372: return; Ditto. https://codereview.chromium.org/2470343003/diff/160001/ash/common/wm/overview... ash/common/wm/overview/scoped_transform_overview_window.cc:472: params.name = "OverviewModeMinmized"; nit: s/OverviewModeMinmized/OverviewModeMinimized https://codereview.chromium.org/2470343003/diff/160001/ash/common/wm/overview... File ash/common/wm/overview/window_grid.cc (right): https://codereview.chromium.org/2470343003/diff/160001/ash/common/wm/overview... ash/common/wm/overview/window_grid.cc:618: // visibility may show the temporary hidden (minimized) panels. nit: temporarily nit: mismatched () in the comment. https://codereview.chromium.org/2470343003/diff/160001/ash/common/wm/overview... File ash/common/wm/overview/window_grid.h (right): https://codereview.chromium.org/2470343003/diff/160001/ash/common/wm/overview... ash/common/wm/overview/window_grid.h:50: // 0, 3, 6, 1, 4, 2, 5 Can you also please delete the 2 lines above this comment (or put a TODO(varkha) to update the comment)? There is no vertical movement anymore. https://codereview.chromium.org/2470343003/diff/160001/ash/common/wm/overview... File ash/common/wm/overview/window_selector_item.cc (right): https://codereview.chromium.org/2470343003/diff/160001/ash/common/wm/overview... ash/common/wm/overview/window_selector_item.cc:626: window_label_.reset(new views::Widget); I guess this would not work with mus. Thanks for patching this!
On 2016/11/07 23:01:38, varkha wrote: > So for the example in the comment > https://bugs.chromium.org/p/chromium/issues/detail?id=654067#c3 - is the > intention now to have a page stay paused? With this patch upon entering overview > while the page is minimized (and paused) the overview header says "Playing" but > the page's tab caption says "Paused" (and the video plays while overview is > active). Alt+Tab is doing the same. > Is the intention to have the video in a minimized browser window paused while in > Alt+Tab (I would think playing it at least while overview / Alt+Tab focus is on > that page is desirable no? I believe we shouldn't change the state in overview / alt-tab because that's not your intention to change the state. You just want to select one window form multiple choices. I talked to kuscher@ and he agreed. Of course, we can discuss if you disagree.
On 2016/11/08 00:02:57, varkha wrote: > Not sure if you had the web apps running in a window in mind - this is the case > when the headers get hidden, so this is not just about ARC windows. I'm testing chrome/webapps because ARC++ windows aren't working correctly for alt-tab (thus this too). > I've also noticed this bug with the docked windows (new with this CL): > - Have 2 V1 apps (web links set to run in a window) > - Have a short host screen so docking one window will displace and minimize the > other > - Dock the first one (gets docked) > - Dock the second one (gets docked and minimizes the first window) > - Enter overview > - select the first (minimized) > - the second window is displaced and loses its header (permanently). Thanks for finding this. New patch will have a fix. > > So hide / unhide the header is not symmetric somewhere. > > https://codereview.chromium.org/2470343003/diff/160001/ash/common/wm/overview... > File ash/common/wm/overview/scoped_transform_overview_window.cc (right): > > https://codereview.chromium.org/2470343003/diff/160001/ash/common/wm/overview... > ash/common/wm/overview/scoped_transform_overview_window.cc:282: return 0; > I don't think this is right. I tried it with the web browser running in a window > (menu->More tools->Add to shelf...) and I now see both the overview header and > the original scaled window header when in overview. I expected to only see the > fake overview header. > This might be the right thing for ARC, are you able to try that and maybe > restrict to just ARC, say by manipulating what this property returns in exo when > a window is minimized? That's what you get in alt-tab. They should be consistent, right? I can change/fix it now, or I can address it in a separate CL, but let me talk to PM first. > > https://codereview.chromium.org/2470343003/diff/160001/ash/common/wm/overview... > ash/common/wm/overview/scoped_transform_overview_window.cc:372: return; > Ditto. > > https://codereview.chromium.org/2470343003/diff/160001/ash/common/wm/overview... > ash/common/wm/overview/scoped_transform_overview_window.cc:472: params.name = > "OverviewModeMinmized"; > nit: s/OverviewModeMinmized/OverviewModeMinimized > > https://codereview.chromium.org/2470343003/diff/160001/ash/common/wm/overview... > File ash/common/wm/overview/window_grid.cc (right): > > https://codereview.chromium.org/2470343003/diff/160001/ash/common/wm/overview... > ash/common/wm/overview/window_grid.cc:618: // visibility may show the temporary > hidden (minimized) panels. > nit: temporarily > nit: mismatched () in the comment. > > https://codereview.chromium.org/2470343003/diff/160001/ash/common/wm/overview... > File ash/common/wm/overview/window_grid.h (right): > > https://codereview.chromium.org/2470343003/diff/160001/ash/common/wm/overview... > ash/common/wm/overview/window_grid.h:50: // 0, 3, 6, 1, 4, 2, 5 > Can you also please delete the 2 lines above this comment (or put a TODO(varkha) > to update the comment)? There is no vertical movement anymore. > > https://codereview.chromium.org/2470343003/diff/160001/ash/common/wm/overview... > File ash/common/wm/overview/window_selector_item.cc (right): > > https://codereview.chromium.org/2470343003/diff/160001/ash/common/wm/overview... > ash/common/wm/overview/window_selector_item.cc:626: window_label_.reset(new > views::Widget); > I guess this would not work with mus. Thanks for patching this!
https://codereview.chromium.org/2470343003/diff/160001/ash/common/wm/overview... File ash/common/wm/overview/scoped_transform_overview_window.cc (right): https://codereview.chromium.org/2470343003/diff/160001/ash/common/wm/overview... ash/common/wm/overview/scoped_transform_overview_window.cc:282: return 0; On 2016/11/08 00:02:56, varkha wrote: > I don't think this is right. I tried it with the web browser running in a window > (menu->More tools->Add to shelf...) and I now see both the overview header and > the original scaled window header when in overview. I expected to only see the > fake overview header. > This might be the right thing for ARC, are you able to try that and maybe > restrict to just ARC, say by manipulating what this property returns in exo when > a window is minimized? Updated the mirror window to do the right thing and this will fix ARC++ windows too. https://codereview.chromium.org/2470343003/diff/160001/ash/common/wm/overview... ash/common/wm/overview/scoped_transform_overview_window.cc:472: params.name = "OverviewModeMinmized"; On 2016/11/08 00:02:56, varkha wrote: > nit: s/OverviewModeMinmized/OverviewModeMinimized Done. https://codereview.chromium.org/2470343003/diff/160001/ash/common/wm/overview... File ash/common/wm/overview/window_grid.h (right): https://codereview.chromium.org/2470343003/diff/160001/ash/common/wm/overview... ash/common/wm/overview/window_grid.h:50: // 0, 3, 6, 1, 4, 2, 5 On 2016/11/08 00:02:56, varkha wrote: > Can you also please delete the 2 lines above this comment (or put a TODO(varkha) > to update the comment)? There is no vertical movement anymore. Done.
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.
ping?
lgtm. I tried it a bit and it seems to be doing the right thing. I could not test it with Android apps but I'm sure you did. https://codereview.chromium.org/2470343003/diff/220001/ash/common/wm/overview... File ash/common/wm/overview/scoped_transform_overview_window.cc (right): https://codereview.chromium.org/2470343003/diff/220001/ash/common/wm/overview... ash/common/wm/overview/scoped_transform_overview_window.cc:402: if (!minimized_.get()) nit: Same as "if (!minimized)"? https://codereview.chromium.org/2470343003/diff/220001/ash/common/wm/overview... ash/common/wm/overview/scoped_transform_overview_window.cc:490: LOG(ERROR) << "Preferred:" << preferred.ToString(); nit: remove before landing. https://codereview.chromium.org/2470343003/diff/220001/ash/common/wm/overview... File ash/common/wm/overview/scoped_transform_overview_window.h (right): https://codereview.chromium.org/2470343003/diff/220001/ash/common/wm/overview... ash/common/wm/overview/scoped_transform_overview_window.h:183: std::unique_ptr<views::Widget> minimized_; How about minimized_widget_? Longer but we often use minimized_ as a boolean elsewhere so it was slightly confusing when reading the implementation, especially since you are actually using it as "if (minimized_)".
https://codereview.chromium.org/2470343003/diff/220001/ash/common/wm/overview... File ash/common/wm/overview/scoped_transform_overview_window.cc (left): https://codereview.chromium.org/2470343003/diff/220001/ash/common/wm/overview... ash/common/wm/overview/scoped_transform_overview_window.cc:252: return window_->GetMinimizeAnimationTargetBoundsInScreen(); nit: Is WmWindow::GetMinimizeAnimationTargetBoundsInScreen still used? The point here was to start growing window animation from the icon in the shelf and right now it animates from the restored bounds - not sure if you wanted to fix that later.
https://codereview.chromium.org/2470343003/diff/220001/ash/common/wm/overview... File ash/common/wm/overview/scoped_transform_overview_window.cc (left): https://codereview.chromium.org/2470343003/diff/220001/ash/common/wm/overview... ash/common/wm/overview/scoped_transform_overview_window.cc:252: return window_->GetMinimizeAnimationTargetBoundsInScreen(); On 2016/11/15 20:17:22, varkha wrote: > nit: Is WmWindow::GetMinimizeAnimationTargetBoundsInScreen still used? The point > here was to start growing window animation from the icon in the shelf and right > now it animates from the restored bounds - not sure if you wanted to fix that > later. Yes, we're going to fix this. https://codereview.chromium.org/2470343003/diff/220001/ash/common/wm/overview... File ash/common/wm/overview/scoped_transform_overview_window.cc (right): https://codereview.chromium.org/2470343003/diff/220001/ash/common/wm/overview... ash/common/wm/overview/scoped_transform_overview_window.cc:402: if (!minimized_.get()) On 2016/11/15 20:10:21, varkha wrote: > nit: Same as "if (!minimized)"? Done. https://codereview.chromium.org/2470343003/diff/220001/ash/common/wm/overview... ash/common/wm/overview/scoped_transform_overview_window.cc:490: LOG(ERROR) << "Preferred:" << preferred.ToString(); On 2016/11/15 20:10:21, varkha wrote: > nit: remove before landing. Done. https://codereview.chromium.org/2470343003/diff/220001/ash/common/wm/overview... File ash/common/wm/overview/scoped_transform_overview_window.h (right): https://codereview.chromium.org/2470343003/diff/220001/ash/common/wm/overview... ash/common/wm/overview/scoped_transform_overview_window.h:183: std::unique_ptr<views::Widget> minimized_; On 2016/11/15 20:10:21, varkha wrote: > How about minimized_widget_? Longer but we often use minimized_ as a boolean > elsewhere so it was slightly confusing when reading the implementation, > especially since you are actually using it as "if (minimized_)". Done.
The CQ bit was checked by oshima@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from varkha@chromium.org Link to the patchset: https://codereview.chromium.org/2470343003/#ps260001 (title: "Use mirror layer for minimized windows in overview mode")
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 ========== Use mirror layer for minimized windows in overview mode * This CL changes overview mode to use the mirror layer for minimized windows (normal minimized, docked minimized). * The mirror window already excludes header, so do not adjust the top insets. * Reposition windows when a window gets minimized/unminimized. BUG=654067 TEST=covered by unit tests. ========== to ========== Use mirror layer for minimized windows in overview mode * This CL changes overview mode to use the mirror layer for minimized windows (normal minimized, docked minimized). * The mirror window already excludes header, so do not adjust the top insets. * Reposition windows when a window gets minimized/unminimized. BUG=654067 TEST=covered by unit tests. ==========
Message was sent while issue was closed.
Committed patchset #6 (id:260001)
Message was sent while issue was closed.
Description was changed from ========== Use mirror layer for minimized windows in overview mode * This CL changes overview mode to use the mirror layer for minimized windows (normal minimized, docked minimized). * The mirror window already excludes header, so do not adjust the top insets. * Reposition windows when a window gets minimized/unminimized. BUG=654067 TEST=covered by unit tests. ========== to ========== Use mirror layer for minimized windows in overview mode * This CL changes overview mode to use the mirror layer for minimized windows (normal minimized, docked minimized). * The mirror window already excludes header, so do not adjust the top insets. * Reposition windows when a window gets minimized/unminimized. BUG=654067 TEST=covered by unit tests. Committed: https://crrev.com/49d6f138d9c18c3c8843cddf43f06d4c0b484578 Cr-Commit-Position: refs/heads/master@{#432537} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/49d6f138d9c18c3c8843cddf43f06d4c0b484578 Cr-Commit-Position: refs/heads/master@{#432537} |
