Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2786563003:
Add a NOT_DRAWN window in between the root_window and its children. (Closed)
DescriptionAdd a NOT_DRAWN window in between the root_window and its children.
For screen rotation animation, currently we start animation for each child
layer of root_window's layer. By adding a NOT_DRAWN window, we only need to
initiate one animation sequence for these children.
BUG=678763
R=oshima@chromium.org
TEST=Manual
Review-Url: https://codereview.chromium.org/2786563003
Cr-Commit-Position: refs/heads/master@{#464762}
Committed: https://chromium.googlesource.com/chromium/src/+/335760818a29274b57f59d23f555bf6a3b60fb3e
Patch Set 1 #
Total comments: 7
Patch Set 2 : Rename container and add depth to resize window. #
Total comments: 6
Patch Set 3 : Rebased and fixed nits. #Patch Set 4 : Fix the tests. #
Total comments: 6
Patch Set 5 : Add helper function to get WmWindow by Shell Id. #
Total comments: 6
Patch Set 6 : Fix nits in patch 5. #
Total comments: 5
Patch Set 7 : Fix the comment for ResizeWindow. #Patch Set 8 : Rebased to origin/master. #
Messages
Total messages: 58 (21 generated)
wutao@chromium.org changed reviewers: + bruthig@chromium.org, oshima@chromium.org
Hi Oshima, Could you please have a look. I am adding a NOT_DRAWN layer in between the root_window's layer and its current children for screen rotation animation optimization. Thanks, Tao
oshima@chromium.org changed reviewers: + jamescook@chromium.org
https://codereview.chromium.org/2786563003/diff/1/ash/common/wm/root_window_l... File ash/common/wm/root_window_layout_manager.cc (right): https://codereview.chromium.org/2786563003/diff/1/ash/common/wm/root_window_l... ash/common/wm/root_window_layout_manager.cc:28: ResizeWindow(child->children(), fullscreen_bounds); I don't think this is right. There may be non top level window that shouldn't be changed. Tooltip is one of such examples, but there will be more. https://codereview.chromium.org/2786563003/diff/1/ash/root_window_controller.cc File ash/root_window_controller.cc (right): https://codereview.chromium.org/2786563003/diff/1/ash/root_window_controller.... ash/root_window_controller.cc:876: kShellWindowId_DelegateRootContainer, "DelegateRootContainer", root); "delegate" is overused. maybe surrogate root? +jamescook@ who probably have a better suggestion. https://codereview.chromium.org/2786563003/diff/1/ash/rotator/screen_rotation... File ash/rotator/screen_rotation_animator.cc (right): https://codereview.chromium.org/2786563003/diff/1/ash/rotator/screen_rotation... ash/rotator/screen_rotation_animator.cc:257: GetChildLayerByName(root_window->layer(), kDelegateRootLayerName); can't you just find the window by id and get layer?
https://codereview.chromium.org/2786563003/diff/1/ash/root_window_controller.cc File ash/root_window_controller.cc (right): https://codereview.chromium.org/2786563003/diff/1/ash/root_window_controller.... ash/root_window_controller.cc:876: kShellWindowId_DelegateRootContainer, "DelegateRootContainer", root); On 2017/04/05 20:26:51, oshima wrote: > "delegate" is overused. maybe surrogate root? > > +jamescook@ who probably have a better suggestion. Why not just ScreenRotationContainer, since that's all it's used for?
wutao@chromium.org changed reviewers: - bruthig@chromium.org
Hi Oshima and James, Could you please have a look again. Thanks, Tao https://codereview.chromium.org/2786563003/diff/1/ash/common/wm/root_window_l... File ash/common/wm/root_window_layout_manager.cc (right): https://codereview.chromium.org/2786563003/diff/1/ash/common/wm/root_window_l... ash/common/wm/root_window_layout_manager.cc:28: ResizeWindow(child->children(), fullscreen_bounds); On 2017/04/05 20:26:51, oshima wrote: > I don't think this is right. There may be non top level window that shouldn't be > changed. Tooltip is one of such examples, but there will be more. I will add a third parameter |depth| and return if it is more than 3 layers. https://codereview.chromium.org/2786563003/diff/1/ash/root_window_controller.cc File ash/root_window_controller.cc (right): https://codereview.chromium.org/2786563003/diff/1/ash/root_window_controller.... ash/root_window_controller.cc:876: kShellWindowId_DelegateRootContainer, "DelegateRootContainer", root); On 2017/04/05 20:54:41, James Cook wrote: > On 2017/04/05 20:26:51, oshima wrote: > > "delegate" is overused. maybe surrogate root? > > > > +jamescook@ who probably have a better suggestion. > > Why not just ScreenRotationContainer, since that's all it's used for? Done. https://codereview.chromium.org/2786563003/diff/1/ash/rotator/screen_rotation... File ash/rotator/screen_rotation_animator.cc (right): https://codereview.chromium.org/2786563003/diff/1/ash/rotator/screen_rotation... ash/rotator/screen_rotation_animator.cc:257: GetChildLayerByName(root_window->layer(), kDelegateRootLayerName); On 2017/04/05 20:26:51, oshima wrote: > can't you just find the window by id and get layer? Done.
The new name looks good. I'll let oshima review the rest.
lgtm with nits https://codereview.chromium.org/2786563003/diff/20001/ash/common/wm/root_wind... File ash/common/wm/root_window_layout_manager.cc (right): https://codereview.chromium.org/2786563003/diff/20001/ash/common/wm/root_wind... ash/common/wm/root_window_layout_manager.cc:20: return; I think this is ok in this CL, but we should make it more explicit, probably using a property. Let's discuss offline. https://codereview.chromium.org/2786563003/diff/20001/ash/common/wm/root_wind... ash/common/wm/root_window_layout_manager.cc:33: ResizeWindow(child->children(), fullscreen_bounds, depth + 1); can you define child_depth instead of adding every time? const int child_depth = depth + 1; https://codereview.chromium.org/2786563003/diff/20001/ash/common/wm/root_wind... ash/common/wm/root_window_layout_manager.cc:54: gfx::Rect(owner_->GetBounds().size()), 1); optional: it's probably easier for us to start with 0?
Rebased to commit. https://codereview.chromium.org/2786563003/diff/20001/ash/common/wm/root_wind... File ash/common/wm/root_window_layout_manager.cc (right): https://codereview.chromium.org/2786563003/diff/20001/ash/common/wm/root_wind... ash/common/wm/root_window_layout_manager.cc:20: return; On 2017/04/06 16:34:15, oshima wrote: > I think this is ok in this CL, but we should make it more explicit, probably > using a property. Let's discuss offline. Acknowledged. https://codereview.chromium.org/2786563003/diff/20001/ash/common/wm/root_wind... ash/common/wm/root_window_layout_manager.cc:33: ResizeWindow(child->children(), fullscreen_bounds, depth + 1); On 2017/04/06 16:34:15, oshima wrote: > can you define child_depth instead of adding every time? > const int child_depth = depth + 1; Done. https://codereview.chromium.org/2786563003/diff/20001/ash/common/wm/root_wind... ash/common/wm/root_window_layout_manager.cc:54: gfx::Rect(owner_->GetBounds().size()), 1); On 2017/04/06 16:34:15, oshima wrote: > optional: it's probably easier for us to start with 0? Done.
The CQ bit was checked by wutao@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from oshima@chromium.org Link to the patchset: https://codereview.chromium.org/2786563003/#ps40001 (title: "Rebased and fixed nits.")
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
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_...)
wutao@chromium.org changed reviewers: + sadrul@chromium.org
Hi Oshima, Please verify adding this layer not break production code. I am afraid if some place have hard coded code to get root window and use the container id by order such as in the tests file. I need to reorder the container id, due to the implementation of AshFocusRules::CanActivateWindow [1], BelongsToContainerWithEqualOrGreaterId(window, kShellWindowId_SystemModalContainer) Hi sadrul@, Please look ash/devtools/ash_devtools_unittest.cc. and verify adding this layer will not break production code as far as you know. I am adding one NOT_DRAWN layer in between the root_window's layer and its current children. Some of the DEV tool test is hard coded to get the root, so I need to modify them. [1] https://cs.chromium.org/chromium/src/ash/wm/ash_focus_rules.cc?type=cs&l=69 Thanks, Tao
The CQ bit was checked by wutao@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.
On 2017/04/10 02:08:46, wutao wrote: > Hi Oshima, > > Please verify adding this layer not break production code. I am afraid if some > place have hard coded code to get root window and use the container id by order > such as in the tests file. > > I need to reorder the container id, due to the implementation of > AshFocusRules::CanActivateWindow [1], > BelongsToContainerWithEqualOrGreaterId(window, > kShellWindowId_SystemModalContainer) > > Hi sadrul@, > Please look ash/devtools/ash_devtools_unittest.cc. > and verify adding this layer will not break production code as far as you know. > > I am adding one NOT_DRAWN layer in between the root_window's layer and its > current children. Some of the DEV tool test is hard coded to get the root, so I > need to modify them. Are you inserting a *layer* or a *window*? Because if you insert a layer, the devtools test should not need to change. > > [1] https://cs.chromium.org/chromium/src/ash/wm/ash_focus_rules.cc?type=cs&l=69 > > Thanks, > Tao
On 2017/04/10 18:18:51, sadrul wrote: > On 2017/04/10 02:08:46, wutao wrote: > > Hi Oshima, > > > > Please verify adding this layer not break production code. I am afraid if some > > place have hard coded code to get root window and use the container id by > order > > such as in the tests file. > > > > I need to reorder the container id, due to the implementation of > > AshFocusRules::CanActivateWindow [1], > > BelongsToContainerWithEqualOrGreaterId(window, > > kShellWindowId_SystemModalContainer) > > > > Hi sadrul@, > > Please look ash/devtools/ash_devtools_unittest.cc. > > and verify adding this layer will not break production code as far as you > know. > > > > I am adding one NOT_DRAWN layer in between the root_window's layer and its > > current children. Some of the DEV tool test is hard coded to get the root, so > I > > need to modify them. > > Are you inserting a *layer* or a *window*? Because if you insert a layer, the > devtools test should not need to change. On a sidenote, do you know why we don't animate the root window? (because that should also take care of animating all windows)
> > Are you inserting a *layer* or a *window*? Because if you insert a layer, the > > devtools test should not need to change. > > On a sidenote, do you know why we don't animate the root window? (because that > should also take care of animating all windows) Sorry for the misleading title, I am inserting a window (WmWindow*). The reason we cannot animate the root window is that, for the animation, we need to add another layer (generated from layer copy output request) to the root, and have different animation. The new layer will serve as a mask to make the screen looks static while set display rotation, and then begin cross fade animations between this new inserted layer and other old layers.
https://codereview.chromium.org/2786563003/diff/60001/ash/root_window_control... File ash/root_window_controller.cc (right): https://codereview.chromium.org/2786563003/diff/60001/ash/root_window_control... ash/root_window_controller.cc:877: screen_rotation_container->SetChildWindowVisibilityChangesAnimated(); just noticed this. Did you need this?
On 2017/04/10 19:19:06, oshima wrote: > https://codereview.chromium.org/2786563003/diff/60001/ash/root_window_control... > File ash/root_window_controller.cc (right): > > https://codereview.chromium.org/2786563003/diff/60001/ash/root_window_control... > ash/root_window_controller.cc:877: > screen_rotation_container->SetChildWindowVisibilityChangesAnimated(); > just noticed this. Did you need this? I do not think I need this. During the animation, (in a future cl) we need to set the opacity to 0 so that we can hide the real layers while using the two copied layers to do animation. And after animation, we will set opacity back to 1. There is no visibility change for this inserted window.
can you remove if it's not necessary? https://codereview.chromium.org/2786563003/diff/60001/ash/devtools/ash_devtoo... File ash/devtools/ash_devtools_unittest.cc (right): https://codereview.chromium.org/2786563003/diff/60001/ash/devtools/ash_devtoo... ash/devtools/ash_devtools_unittest.cc:369: ->GetChildren()[0] define a utility function to get the parent window. It's probably better to use the id rather than [0].
Hi Oshima, Please review the new changes. Thank you, Tao https://codereview.chromium.org/2786563003/diff/60001/ash/devtools/ash_devtoo... File ash/devtools/ash_devtools_unittest.cc (right): https://codereview.chromium.org/2786563003/diff/60001/ash/devtools/ash_devtoo... ash/devtools/ash_devtools_unittest.cc:369: ->GetChildren()[0] On 2017/04/10 19:44:11, oshima wrote: > define a utility function to get the parent window. It's probably better to use > the id rather than [0]. Done. And changed other related parts. https://codereview.chromium.org/2786563003/diff/60001/ash/root_window_control... File ash/root_window_controller.cc (right): https://codereview.chromium.org/2786563003/diff/60001/ash/root_window_control... ash/root_window_controller.cc:877: screen_rotation_container->SetChildWindowVisibilityChangesAnimated(); On 2017/04/10 19:19:06, oshima wrote: > just noticed this. Did you need this? Removed this.
The CQ bit was checked by wutao@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.
lgtm but please wait for sadrul@'s review for dev tools. https://codereview.chromium.org/2786563003/diff/60001/ash/devtools/ash_devtoo... File ash/devtools/ash_devtools_unittest.cc (right): https://codereview.chromium.org/2786563003/diff/60001/ash/devtools/ash_devtoo... ash/devtools/ash_devtools_unittest.cc:369: ->GetChildren()[0] On 2017/04/10 23:03:54, wutao wrote: > On 2017/04/10 19:44:11, oshima wrote: > > define a utility function to get the parent window. It's probably better to > use > > the id rather than [0]. > > Done. And changed other related parts. Sorry if it wasn't clear. I was thinking of something like WmWindow* root_node_window = GetRootNodeWindow(). I'm not sure if it makes sense to use id for other windows. I'll defer to sadrul@
Hi sadrul@, Could you please look the dev tools test. Thank you, Tao
sadrul@chromium.org changed reviewers: + sky@chromium.org
+sky@ Have you tried actually inserting the *layer* for just the animation? i.e. you insert the layer between the root window and child windows before the animation, trigger the animation, then remove the layer at the end of the animation. That would be better than always having this window. https://codereview.chromium.org/2786563003/diff/80001/ash/public/cpp/shell_wi... File ash/public/cpp/shell_window_ids.h (right): https://codereview.chromium.org/2786563003/diff/80001/ash/public/cpp/shell_wi... ash/public/cpp/shell_window_ids.h:143: kShellWindowId_PowerButtonAnimationContainer, Oh ew. We really should fix this. See https://codereview.chromium.org/2700523004/#msg106 and related discussion.
Some comments on the test itself, if we still end up with the rotation window https://codereview.chromium.org/2786563003/diff/80001/ash/devtools/ash_devtoo... File ash/devtools/ash_devtools_unittest.cc (right): https://codereview.chromium.org/2786563003/diff/80001/ash/devtools/ash_devtoo... ash/devtools/ash_devtools_unittest.cc:391: GetWindowByShellId(kShellWindowId_NonLockScreenContainersContainer); These look ... weird. Can you do: WmWindow* root_window = GetPrimaryRootWindow(); WmWindow* rotation_window = root_window->GetChildren(0); WmWindow* target_window = rotation_window->GetChildren()[1]; WmWindow* child_window = rotation_window->GetChildren()[0]->GetChildren()[0]; https://codereview.chromium.org/2786563003/diff/80001/ash/devtools/ash_devtoo... ash/devtools/ash_devtools_unittest.cc:416: GetWindowByShellId(kShellWindowId_WallpaperContainer); As above, go to target_window and parent_window through root_window and rotation_window?
On 2017/04/13 17:05:15, sadrul wrote: > +sky@ > > Have you tried actually inserting the *layer* for just the animation? i.e. you > insert the layer between the root window and child windows before the animation, > trigger the animation, then remove the layer at the end of the animation. That > would be better than always having this window. Changing tree hierarchy is complicated. Just having single NOT_DRAWN is much simpler IMO, unless there is a real issue. > > https://codereview.chromium.org/2786563003/diff/80001/ash/public/cpp/shell_wi... > File ash/public/cpp/shell_window_ids.h (right): > > https://codereview.chromium.org/2786563003/diff/80001/ash/public/cpp/shell_wi... > ash/public/cpp/shell_window_ids.h:143: > kShellWindowId_PowerButtonAnimationContainer, > Oh ew. We really should fix this. See > https://codereview.chromium.org/2700523004/#msg106 and related discussion. I takled to Ahmed, and he said he'll have a CL soon.
Hi Sadrul: I changed the test to what you suggested. Please look the test again. To dynamic change the layer will be complicated and not performance efficient as Oshima mentioned. Thank you, Tao https://codereview.chromium.org/2786563003/diff/60001/ash/devtools/ash_devtoo... File ash/devtools/ash_devtools_unittest.cc (right): https://codereview.chromium.org/2786563003/diff/60001/ash/devtools/ash_devtoo... ash/devtools/ash_devtools_unittest.cc:369: ->GetChildren()[0] On 2017/04/11 21:48:08, oshima wrote: > On 2017/04/10 23:03:54, wutao wrote: > > On 2017/04/10 19:44:11, oshima wrote: > > > define a utility function to get the parent window. It's probably better to > > use > > > the id rather than [0]. > > > > Done. And changed other related parts. > > Sorry if it wasn't clear. I was thinking of something like > > WmWindow* root_node_window = GetRootNodeWindow(). > > I'm not sure if it makes sense to use id for other windows. I'll defer to > sadrul@ I changed as sadrul@ suggested. GetPrimaryRootWindow(). https://codereview.chromium.org/2786563003/diff/80001/ash/devtools/ash_devtoo... File ash/devtools/ash_devtools_unittest.cc (right): https://codereview.chromium.org/2786563003/diff/80001/ash/devtools/ash_devtoo... ash/devtools/ash_devtools_unittest.cc:391: GetWindowByShellId(kShellWindowId_NonLockScreenContainersContainer); On 2017/04/13 17:09:42, sadrul wrote: > These look ... weird. Can you do: > > WmWindow* root_window = GetPrimaryRootWindow(); > WmWindow* rotation_window = root_window->GetChildren(0); > WmWindow* target_window = rotation_window->GetChildren()[1]; > WmWindow* child_window = rotation_window->GetChildren()[0]->GetChildren()[0]; Done. https://codereview.chromium.org/2786563003/diff/80001/ash/devtools/ash_devtoo... ash/devtools/ash_devtools_unittest.cc:416: GetWindowByShellId(kShellWindowId_WallpaperContainer); On 2017/04/13 17:09:42, sadrul wrote: > As above, go to target_window and parent_window through root_window and > rotation_window? Done. https://codereview.chromium.org/2786563003/diff/80001/ash/public/cpp/shell_wi... File ash/public/cpp/shell_window_ids.h (right): https://codereview.chromium.org/2786563003/diff/80001/ash/public/cpp/shell_wi... ash/public/cpp/shell_window_ids.h:143: kShellWindowId_PowerButtonAnimationContainer, On 2017/04/13 17:05:15, sadrul wrote: > Oh ew. We really should fix this. See > https://codereview.chromium.org/2700523004/#msg106 and related discussion. Acknowledged.
On 2017/04/13 17:12:56, oshima wrote: > On 2017/04/13 17:05:15, sadrul wrote: > > +sky@ > > > > Have you tried actually inserting the *layer* for just the animation? i.e. you > > insert the layer between the root window and child windows before the > animation, > > trigger the animation, then remove the layer at the end of the animation. That > > would be better than always having this window. > > Changing tree hierarchy is complicated. Just having single NOT_DRAWN is much > simpler IMO, unless there is a real issue. Inserting a Layer between a window and its children should not be difficult/complicated. I'd like to know why it is. A Window is generally used when it is expected to affect event dispatch (either directly as an event target, or indirectly as a stacking container affecting targeting). If you only need to affect visuals, then a Layer should be sufficient. I think this CL can land with the current approach if it is blocking other work (with the following issue in root_window_layout_manager.cc resolved). But I think we should still attempt to use the temporary intermediate layer for the animation in follow up CLs. Also, if you are inserting a window, please make sure to update the CL description and title accordingly. https://codereview.chromium.org/2786563003/diff/100001/ash/wm/root_window_lay... File ash/wm/root_window_layout_manager.cc (right): https://codereview.chromium.org/2786563003/diff/100001/ash/wm/root_window_lay... ash/wm/root_window_layout_manager.cc:20: return; This doesn't make sense. Why not check to see if the Window actually is a top-level window?
On 2017/04/14 05:52:44, sadrul wrote: > On 2017/04/13 17:12:56, oshima wrote: > > On 2017/04/13 17:05:15, sadrul wrote: > > > +sky@ > > > > > > Have you tried actually inserting the *layer* for just the animation? i.e. > you > > > insert the layer between the root window and child windows before the > > animation, > > > trigger the animation, then remove the layer at the end of the animation. > That > > > would be better than always having this window. > > > > Changing tree hierarchy is complicated. Just having single NOT_DRAWN is much > > simpler IMO, unless there is a real issue. > > Inserting a Layer between a window and its children should not be > difficult/complicated. I'd like to know why it is. Sorry I wasn't clear. It's not about the code. It's about the runtime. Reparenting window triggers a lot of events that aren't really necessary for this. (it's been removed and re-added to root, hierarchy changing/changed events (up, down), One of reasons we had a lot of issues with dock windows is that it does reparent a window during docked/dragging process, for example. > A Window is generally used when it is expected to affect event dispatch (either > directly as an event target, or indirectly as a stacking container affecting > targeting). If you only need to affect visuals, then a Layer should be > sufficient. A window is also used to group windows. (honestly speaking, I think it's abused a bit though) > I think this CL can land with the current approach if it is blocking other work > (with the following issue in root_window_layout_manager.cc resolved). But I > think we should still attempt to use the temporary intermediate layer for the > animation in follow up CLs. > > Also, if you are inserting a window, please make sure to update the CL > description and title accordingly. > > https://codereview.chromium.org/2786563003/diff/100001/ash/wm/root_window_lay... > File ash/wm/root_window_layout_manager.cc (right): > > https://codereview.chromium.org/2786563003/diff/100001/ash/wm/root_window_lay... > ash/wm/root_window_layout_manager.cc:20: return; > This doesn't make sense. Why not check to see if the Window actually is a > top-level window?
https://codereview.chromium.org/2786563003/diff/100001/ash/wm/root_window_lay... File ash/wm/root_window_layout_manager.cc (right): https://codereview.chromium.org/2786563003/diff/100001/ash/wm/root_window_lay... ash/wm/root_window_layout_manager.cc:20: return; On 2017/04/14 05:52:44, sadrul wrote: > This doesn't make sense. Why not check to see if the Window actually is a > top-level window? This is equivalent to what current code does. This layer manager is responsible for specific containers, minus toplevel windows. Since non container window can also have null delegate, it may resize a window this layout manager isn't responsible for. I agree that this can be improved but that's outside of the CL's scope.
Description was changed from ========== Add one NOT_DRAWN layer in between the root_window's layer and its current children For screen rotation animation, currently we start animation for each child of root_window. By adding a NOT_DRAWN layer, we only need to initiate one animation sequence for these children. BUG=678763 ========== to ========== Add a NOT_DRAWN window in between the root_window and its children. For screen rotation animation, currently we start animation for each child layer of root_window's layer. By adding a NOT_DRAWN window, we only need to initiate one animation sequence for these children. BUG=678763 R=oshima@chromium.org TEST=Manual ==========
On 2017/04/14 05:52:44, sadrul wrote: > Also, if you are inserting a window, please make sure to update the CL > description and title accordingly. CL description and title have been updated accordingly.
Description was changed from ========== Add a NOT_DRAWN window in between the root_window and its children. For screen rotation animation, currently we start animation for each child layer of root_window's layer. By adding a NOT_DRAWN window, we only need to initiate one animation sequence for these children. BUG=678763 R=oshima@chromium.org TEST=Manual ========== to ========== Add a NOT_DRAWN window in between the root_window and its children. For screen rotation animation, currently we start animation for each child layer of root_window's layer. By adding a NOT_DRAWN window, we only need to initiate one animation sequence for these children. BUG=678763 R=oshima@chromium.org TEST=Manual ==========
On 2017/04/14 06:23:33, oshima wrote: > On 2017/04/14 05:52:44, sadrul wrote: > > On 2017/04/13 17:12:56, oshima wrote: > > > On 2017/04/13 17:05:15, sadrul wrote: > > > > +sky@ > > > > > > > > Have you tried actually inserting the *layer* for just the animation? i.e. > > you > > > > insert the layer between the root window and child windows before the > > > animation, > > > > trigger the animation, then remove the layer at the end of the animation. > > That > > > > would be better than always having this window. > > > > > > Changing tree hierarchy is complicated. Just having single NOT_DRAWN is much > > > simpler IMO, unless there is a real issue. > > > > Inserting a Layer between a window and its children should not be > > difficult/complicated. I'd like to know why it is. > > Sorry I wasn't clear. It's not about the code. It's about the runtime. > Reparenting window triggers a lot of events that aren't really necessary for > this. > (it's been removed and re-added to root, hierarchy changing/changed events (up, > down), > > One of reasons we had a lot of issues with dock windows is that it does reparent > a window during docked/dragging process, for example. > > > A Window is generally used when it is expected to affect event dispatch > (either > > directly as an event target, or indirectly as a stacking container affecting > > targeting). If you only need to affect visuals, then a Layer should be > > sufficient. > > A window is also used to group windows. (honestly speaking, I think it's > abused a bit though) Actually, if your concern is about using a window, we can use layer instead. It's not common to have a layer between window hierarchy, but that'll work. WDYT? > > > I think this CL can land with the current approach if it is blocking other > work > > (with the following issue in root_window_layout_manager.cc resolved). But I > > think we should still attempt to use the temporary intermediate layer for the > > animation in follow up CLs. > > > > Also, if you are inserting a window, please make sure to update the CL > > description and title accordingly. > > > > > https://codereview.chromium.org/2786563003/diff/100001/ash/wm/root_window_lay... > > File ash/wm/root_window_layout_manager.cc (right): > > > > > https://codereview.chromium.org/2786563003/diff/100001/ash/wm/root_window_lay... > > ash/wm/root_window_layout_manager.cc:20: return; > > This doesn't make sense. Why not check to see if the Window actually is a > > top-level window?
> > > > > Have you tried actually inserting the *layer* for just the animation? > i.e. > > > you > > > > > insert the layer between the root window and child windows before the > > > > animation, > > > > > trigger the animation, then remove the layer at the end of the > animation. > > > That > > > > > would be better than always having this window. > > > > > > > > Changing tree hierarchy is complicated. Just having single NOT_DRAWN is > much > > > > simpler IMO, unless there is a real issue. > > > > > > Inserting a Layer between a window and its children should not be > > > difficult/complicated. I'd like to know why it is. > > > > Sorry I wasn't clear. It's not about the code. It's about the runtime. > > Reparenting window triggers a lot of events that aren't really necessary for > > this. > > (it's been removed and re-added to root, hierarchy changing/changed events > (up, > > down), > > > > One of reasons we had a lot of issues with dock windows is that it does > reparent > > a window during docked/dragging process, for example. > > > > > A Window is generally used when it is expected to affect event dispatch > > (either > > > directly as an event target, or indirectly as a stacking container affecting > > > targeting). If you only need to affect visuals, then a Layer should be > > > sufficient. > > > > A window is also used to group windows. (honestly speaking, I think it's > > abused a bit though) > > Actually, if your concern is about using a window, we can use layer instead. > It's not common to have a layer between window hierarchy, but that'll work. > WDYT? Yes. I did mean a layer, and not a window.
https://codereview.chromium.org/2786563003/diff/100001/ash/wm/root_window_lay... File ash/wm/root_window_layout_manager.cc (right): https://codereview.chromium.org/2786563003/diff/100001/ash/wm/root_window_lay... ash/wm/root_window_layout_manager.cc:20: return; On 2017/04/14 06:52:15, oshima wrote: > On 2017/04/14 05:52:44, sadrul wrote: > > This doesn't make sense. Why not check to see if the Window actually is a > > top-level window? > > This is equivalent to what current code does. This layer manager is responsible > for specific containers, minus toplevel windows. Since > non container window can also have null delegate, it may > resize a window this layout manager isn't responsible for. > > I agree that this can be improved but that's outside of the CL's scope. OK. I guess checking 'window->GetTopLevelWindow() == window' doesn't work? That probably means the comment is wrong. Can that be updated?
On 2017/04/14 16:58:42, sadrul wrote: > > > > > > Have you tried actually inserting the *layer* for just the animation? > > i.e. > > > > you > > > > > > insert the layer between the root window and child windows before the > > > > > animation, > > > > > > trigger the animation, then remove the layer at the end of the > > animation. > > > > That > > > > > > would be better than always having this window. > > > > > > > > > > Changing tree hierarchy is complicated. Just having single NOT_DRAWN is > > much > > > > > simpler IMO, unless there is a real issue. > > > > > > > > Inserting a Layer between a window and its children should not be > > > > difficult/complicated. I'd like to know why it is. > > > > > > Sorry I wasn't clear. It's not about the code. It's about the runtime. > > > Reparenting window triggers a lot of events that aren't really necessary for > > > this. > > > (it's been removed and re-added to root, hierarchy changing/changed events > > (up, > > > down), > > > > > > One of reasons we had a lot of issues with dock windows is that it does > > reparent > > > a window during docked/dragging process, for example. > > > > > > > A Window is generally used when it is expected to affect event dispatch > > > (either > > > > directly as an event target, or indirectly as a stacking container > affecting > > > > targeting). If you only need to affect visuals, then a Layer should be > > > > sufficient. > > > > > > A window is also used to group windows. (honestly speaking, I think it's > > > abused a bit though) > > > > Actually, if your concern is about using a window, we can use layer instead. > > It's not common to have a layer between window hierarchy, but that'll work. > > WDYT? > > Yes. I did mean a layer, and not a window. Just to make sure we're on the same page. The layer will stay in between because we want to avoid adding/removing layer, which is heavy operation because it does things like walking through entire tree, full tree sync etc.
https://codereview.chromium.org/2786563003/diff/100001/ash/wm/root_window_lay... File ash/wm/root_window_layout_manager.cc (right): https://codereview.chromium.org/2786563003/diff/100001/ash/wm/root_window_lay... ash/wm/root_window_layout_manager.cc:20: return; On 2017/04/14 17:00:58, sadrul wrote: > On 2017/04/14 06:52:15, oshima wrote: > > On 2017/04/14 05:52:44, sadrul wrote: > > > This doesn't make sense. Why not check to see if the Window actually is a > > > top-level window? > > > > This is equivalent to what current code does. This layer manager is > responsible > > for specific containers, minus toplevel windows. Since > > non container window can also have null delegate, it may > > resize a window this layout manager isn't responsible for. > > > > I agree that this can be improved but that's outside of the CL's scope. > > OK. I guess checking 'window->GetTopLevelWindow() == window' doesn't work? That > probably means the comment is wrong. Can that be updated? You're righ. I'll talk to wutao and we'll update the comment. For layer change, we'll address it in a separate CL.
Comments updated. https://codereview.chromium.org/2786563003/diff/100001/ash/wm/root_window_lay... File ash/wm/root_window_layout_manager.cc (right): https://codereview.chromium.org/2786563003/diff/100001/ash/wm/root_window_lay... ash/wm/root_window_layout_manager.cc:20: return; On 2017/04/14 17:28:45, oshima wrote: > On 2017/04/14 17:00:58, sadrul wrote: > > On 2017/04/14 06:52:15, oshima wrote: > > > On 2017/04/14 05:52:44, sadrul wrote: > > > > This doesn't make sense. Why not check to see if the Window actually is a > > > > top-level window? > > > > > > This is equivalent to what current code does. This layer manager is > > responsible > > > for specific containers, minus toplevel windows. Since > > > non container window can also have null delegate, it may > > > resize a window this layout manager isn't responsible for. > > > > > > I agree that this can be improved but that's outside of the CL's scope. > > > > OK. I guess checking 'window->GetTopLevelWindow() == window' doesn't work? > That > > probably means the comment is wrong. Can that be updated? > > You're righ. I'll talk to wutao and we'll update the comment. > > For layer change, we'll address it in a separate CL. Comments are updated.
lgtm
The CQ bit was checked by wutao@chromium.org
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": 140001, "attempt_start_ts": 1492193999994860,
"parent_rev": "d43655436a99967bb5a12952143f29289643c9e7", "commit_rev":
"335760818a29274b57f59d23f555bf6a3b60fb3e"}
Message was sent while issue was closed.
Description was changed from ========== Add a NOT_DRAWN window in between the root_window and its children. For screen rotation animation, currently we start animation for each child layer of root_window's layer. By adding a NOT_DRAWN window, we only need to initiate one animation sequence for these children. BUG=678763 R=oshima@chromium.org TEST=Manual ========== to ========== Add a NOT_DRAWN window in between the root_window and its children. For screen rotation animation, currently we start animation for each child layer of root_window's layer. By adding a NOT_DRAWN window, we only need to initiate one animation sequence for these children. BUG=678763 R=oshima@chromium.org TEST=Manual Review-Url: https://codereview.chromium.org/2786563003 Cr-Commit-Position: refs/heads/master@{#464762} Committed: https://chromium.googlesource.com/chromium/src/+/335760818a29274b57f59d23f555... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://chromium.googlesource.com/chromium/src/+/335760818a29274b57f59d23f555... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
