Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2476353002:
Fix bug where removed (but not deleted) windows are not reflected in the tree properly (Closed)
|
Created:
4 years, 1 month ago by Sarmad Hashmi Modified:
4 years, 1 month ago Reviewers:
sadrul CC:
chromium-reviews, kalyank, sadrul, pfeldman, devtools-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix bug where removed (but not deleted) windows are not reflected in the tree properly
When there are windows which are removed as children of some parent, and
they still exist, OnWindowDestroying is not called because the child window
was never destroyed. However, OnWindowTreeChanging is called, if
the target has an old_parent, aka the parent it is being removed from.
OnWindowTreeChanged is called if the target has a new_parent, aka the
parent it is being added to.
This CL removes nodes on OnWindowTreeChanging and adds them back in
OnWindowTreeChanged if necessary. Since we only want them to trigger once,
we check to see in either callback if the window that the callback is
being called on is the old_parent or the new_parent.
BUG=648701
Committed: https://crrev.com/dbef379ab2b2c8a2127d20839f77d355e9f5742e
Cr-Commit-Position: refs/heads/master@{#431482}
Patch Set 1 #Patch Set 2 : Fix bug where removed (but not deleted) windows are not reflected in the tree properly #Patch Set 3 : Add a test for window being actually removed and inserted #Patch Set 4 : rebase #
Total comments: 6
Patch Set 5 : sadruls comments #Patch Set 6 : sadruls comments #Patch Set 7 : Remove old_parent parameter #
Total comments: 2
Depends on Patchset: Dependent Patchsets: Messages
Total messages: 35 (25 generated)
The CQ bit was checked by mhashmi@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 checked by mhashmi@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...
mhashmi@chromium.org changed reviewers: + sadrul@chromium.org
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 mhashmi@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/2476353002/diff/60001/ash/common/devtools/ash... File ash/common/devtools/ash_devtools_dom_agent.cc (right): https://codereview.chromium.org/2476353002/diff/60001/ash/common/devtools/ash... ash/common/devtools/ash_devtools_dom_agent.cc:113: if (window == params.old_parent) Should you check that params.new_parent is nullptr? https://codereview.chromium.org/2476353002/diff/60001/ash/common/devtools/ash... ash/common/devtools/ash_devtools_dom_agent.cc:171: WmWindow* old_parent) { Can you add a DCHECK that old_parent is non-null here as a documentation/readability hint? https://codereview.chromium.org/2476353002/diff/60001/ash/common/devtools/ash... ash/common/devtools/ash_devtools_dom_agent.cc:178: if (window != window->GetRootWindow()) { Is |window| ever the root-window here, i.e. are there cases when this condition is not met? Because a root-window wouldn't have a parent, i.e. old_parent will be null, and this function should not be called in those cases. Which means |window| should never be the root window. If that is the case, this should probably be a DCHECK() instead.
The CQ bit was checked by mhashmi@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/2476353002/diff/60001/ash/common/devtools/ash... File ash/common/devtools/ash_devtools_dom_agent.cc (right): https://codereview.chromium.org/2476353002/diff/60001/ash/common/devtools/ash... ash/common/devtools/ash_devtools_dom_agent.cc:113: if (window == params.old_parent) On 2016/11/09 16:09:26, sadrul wrote: > Should you check that params.new_parent is nullptr? Updated comment. This handles the removing when reorganizing from params.old_parent to params.new_parent. So, params.new_parent can be null or not, either way we simply only remove the node here. https://codereview.chromium.org/2476353002/diff/60001/ash/common/devtools/ash... ash/common/devtools/ash_devtools_dom_agent.cc:171: WmWindow* old_parent) { On 2016/11/09 16:09:26, sadrul wrote: > Can you add a DCHECK that old_parent is non-null here as a > documentation/readability hint? Done. https://codereview.chromium.org/2476353002/diff/60001/ash/common/devtools/ash... ash/common/devtools/ash_devtools_dom_agent.cc:178: if (window != window->GetRootWindow()) { On 2016/11/09 16:09:26, sadrul wrote: > Is |window| ever the root-window here, i.e. are there cases when this condition > is not met? Because a root-window wouldn't have a parent, i.e. old_parent will > be null, and this function should not be called in those cases. Which means > |window| should never be the root window. If that is the case, this should > probably be a DCHECK() instead. Ah, thanks for catching this. Done. I'm going to deal with RootWindow add/remove in future CLs using OnRootWindowAdded in ShellObserver (and OnWindowDestroying).
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_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 mhashmi@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.
The CQ bit was checked by mhashmi@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/2476353002/diff/120001/ash/common/devtools/as... File ash/common/devtools/ash_devtools_unittest.cc (right): https://codereview.chromium.org/2476353002/diff/120001/ash/common/devtools/as... ash/common/devtools/ash_devtools_unittest.cc:271: child_window->GetParent()->RemoveChild(child_window); Why is this necessary?
https://codereview.chromium.org/2476353002/diff/120001/ash/common/devtools/as... File ash/common/devtools/ash_devtools_unittest.cc (right): https://codereview.chromium.org/2476353002/diff/120001/ash/common/devtools/as... ash/common/devtools/ash_devtools_unittest.cc:271: child_window->GetParent()->RemoveChild(child_window); On 2016/11/10 20:25:24, sadrul wrote: > Why is this necessary? We know AddChild removes |window| from its old parent. This test simply confirms that a |window| with no parent (i.e: a removed but not destroyed child) that is added as a child to another window at a later time still triggers remove/insert calls.
lgtm
The CQ bit was checked by mhashmi@chromium.org
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.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Fix bug where removed (but not deleted) windows are not reflected in the tree properly When there are windows which are removed as children of some parent, and they still exist, OnWindowDestroying is not called because the child window was never destroyed. However, OnWindowTreeChanging is called, if the target has an old_parent, aka the parent it is being removed from. OnWindowTreeChanged is called if the target has a new_parent, aka the parent it is being added to. This CL removes nodes on OnWindowTreeChanging and adds them back in OnWindowTreeChanged if necessary. Since we only want them to trigger once, we check to see in either callback if the window that the callback is being called on is the old_parent or the new_parent. BUG=648701 ========== to ========== Fix bug where removed (but not deleted) windows are not reflected in the tree properly When there are windows which are removed as children of some parent, and they still exist, OnWindowDestroying is not called because the child window was never destroyed. However, OnWindowTreeChanging is called, if the target has an old_parent, aka the parent it is being removed from. OnWindowTreeChanged is called if the target has a new_parent, aka the parent it is being added to. This CL removes nodes on OnWindowTreeChanging and adds them back in OnWindowTreeChanged if necessary. Since we only want them to trigger once, we check to see in either callback if the window that the callback is being called on is the old_parent or the new_parent. BUG=648701 Committed: https://crrev.com/dbef379ab2b2c8a2127d20839f77d355e9f5742e Cr-Commit-Position: refs/heads/master@{#431482} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/dbef379ab2b2c8a2127d20839f77d355e9f5742e Cr-Commit-Position: refs/heads/master@{#431482}
Message was sent while issue was closed.
A revert of this CL (patchset #7 id:120001) has been created in https://codereview.chromium.org/2498593002/ by hiroshige@chromium.org. The reason for reverting is: Suspected to cause SystemTrayTest.NullDefaultViewIsNotRecorded failure. BUG=664428 . |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
