Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
628413002:
Show transient child when the transient parent is shown (Closed)
DescriptionShow transient child when the transient parent is shown.
This is controlled by set_parent_control_visibility flag, which is off by default.
BUG=410499
TEST=covered by TransientWindowManagerTest.TransientChildren
Committed: https://crrev.com/b6eaaf28e88c84e21e5d52c1f89ef54bc88661a1
Cr-Commit-Position: refs/heads/master@{#298571}
Patch Set 1 : #Patch Set 2 : fix check #
Total comments: 15
Patch Set 3 : #
Messages
Total messages: 16 (6 generated)
oshima@chromium.org changed reviewers: + sky@chromium.org
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:60001) has been deleted
https://codereview.chromium.org/628413002/diff/100001/ui/wm/core/transient_wi... File ui/wm/core/transient_window_manager.cc (right): https://codereview.chromium.org/628413002/diff/100001/ui/wm/core/transient_wi... ui/wm/core/transient_window_manager.cc:158: if (window_ != window) Why the if, at best this should be a DCHECK https://codereview.chromium.org/628413002/diff/100001/ui/wm/core/transient_wi... ui/wm/core/transient_window_manager.cc:161: for (auto* child : transient_children_) From the style guide: "auto is permitted, for local variables only. Do not use auto for file-scope or namespace-scope variables, or for class members." https://codereview.chromium.org/628413002/diff/100001/ui/wm/core/transient_wi... ui/wm/core/transient_window_manager.cc:165: void TransientWindowManager::OnWindowVisibilityChanged(Window* window, Why do you need to do something in both changed and changing? Isn't one enough? https://codereview.chromium.org/628413002/diff/100001/ui/wm/core/transient_wi... ui/wm/core/transient_window_manager.cc:167: if (window_ != window || ignore_visibility_changed_event_ || You shouldn't need the window_ != window. Add a DCHECK if you want it. https://codereview.chromium.org/628413002/diff/100001/ui/wm/core/transient_wi... File ui/wm/core/transient_window_manager.h (right): https://codereview.chromium.org/628413002/diff/100001/ui/wm/core/transient_wi... ui/wm/core/transient_window_manager.h:55: void set_parent_control_visibility(bool parent_control_visibility) { parent_controls_visibility. Also, document the default is false.
https://codereview.chromium.org/628413002/diff/100001/ui/wm/core/transient_wi... File ui/wm/core/transient_window_manager.cc (right): https://codereview.chromium.org/628413002/diff/100001/ui/wm/core/transient_wi... ui/wm/core/transient_window_manager.cc:158: if (window_ != window) On 2014/10/07 15:19:14, sky wrote: > Why the if, at best this should be a DCHECK Done. I was confused by the difference between Changing (called on the target window only) and Changing (called in hierarchy chain) https://codereview.chromium.org/628413002/diff/100001/ui/wm/core/transient_wi... ui/wm/core/transient_window_manager.cc:161: for (auto* child : transient_children_) On 2014/10/07 15:19:14, sky wrote: > From the style guide: "auto is permitted, for local variables only. Do not use > auto for file-scope or namespace-scope variables, or for class members." Where is it? I was looking at http://chromium-cpp.appspot.com/ and couldn't find that description. https://codereview.chromium.org/628413002/diff/100001/ui/wm/core/transient_wi... ui/wm/core/transient_window_manager.cc:165: void TransientWindowManager::OnWindowVisibilityChanged(Window* window, On 2014/10/07 15:19:14, sky wrote: > Why do you need to do something in both changed and changing? Isn't one enough? I tried and had problem because OnWindowVisibiltyChanged is called twice on the same target (during Down and Up traversal). Is this by design? https://codereview.chromium.org/628413002/diff/100001/ui/wm/core/transient_wi... ui/wm/core/transient_window_manager.cc:167: if (window_ != window || ignore_visibility_changed_event_ || On 2014/10/07 15:19:14, sky wrote: > You shouldn't need the window_ != window. Add a DCHECK if you want it. I needed this because OnWindowVisibilityChanged is called on hierarchy chain https://codereview.chromium.org/628413002/diff/100001/ui/wm/core/transient_wi... File ui/wm/core/transient_window_manager.h (right): https://codereview.chromium.org/628413002/diff/100001/ui/wm/core/transient_wi... ui/wm/core/transient_window_manager.h:55: void set_parent_control_visibility(bool parent_control_visibility) { On 2014/10/07 15:19:14, sky wrote: > parent_controls_visibility. Also, document the default is false. Done.
https://codereview.chromium.org/628413002/diff/100001/ui/wm/core/transient_wi... File ui/wm/core/transient_window_manager.cc (right): https://codereview.chromium.org/628413002/diff/100001/ui/wm/core/transient_wi... ui/wm/core/transient_window_manager.cc:158: if (window_ != window) On 2014/10/07 17:41:14, oshima wrote: > On 2014/10/07 15:19:14, sky wrote: > > Why the if, at best this should be a DCHECK > > Done. I was confused by the difference between Changing (called on the target > window only) and Changing (called in hierarchy chain) oops, I meant "Changed (called in hierarchy chain)"
https://codereview.chromium.org/628413002/diff/100001/ui/wm/core/transient_wi... File ui/wm/core/transient_window_manager.cc (right): https://codereview.chromium.org/628413002/diff/100001/ui/wm/core/transient_wi... ui/wm/core/transient_window_manager.cc:161: for (auto* child : transient_children_) On 2014/10/07 17:41:14, oshima wrote: > On 2014/10/07 15:19:14, sky wrote: > > From the style guide: "auto is permitted, for local variables only. Do not use > > auto for file-scope or namespace-scope variables, or for class members." > > Where is it? I was looking at > > http://chromium-cpp.appspot.com/ > > and couldn't find that description. I guess you meant the description in https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#auto I believe what this is prohibiting is something like auto global = 1; namespace { auto namespace_scope = "xyz"; } The auto in the loop creates local variable, so this should be fine.
LGTM https://codereview.chromium.org/628413002/diff/100001/ui/wm/core/transient_wi... File ui/wm/core/transient_window_manager.cc (right): https://codereview.chromium.org/628413002/diff/100001/ui/wm/core/transient_wi... ui/wm/core/transient_window_manager.cc:161: for (auto* child : transient_children_) On 2014/10/07 17:46:25, oshima wrote: > On 2014/10/07 17:41:14, oshima wrote: > > On 2014/10/07 15:19:14, sky wrote: > > > From the style guide: "auto is permitted, for local variables only. Do not > use > > > auto for file-scope or namespace-scope variables, or for class members." > > > > Where is it? I was looking at > > > > http://chromium-cpp.appspot.com/ > > > > and couldn't find that description. > > I guess you meant the description in > https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#auto > > I believe what this is prohibiting is something like > > auto global = 1; > > namespace { > auto namespace_scope = "xyz"; > } > > The auto in the loop creates local variable, so this should be fine. Yes, you are right. My mistake. https://codereview.chromium.org/628413002/diff/100001/ui/wm/core/transient_wi... ui/wm/core/transient_window_manager.cc:165: void TransientWindowManager::OnWindowVisibilityChanged(Window* window, On 2014/10/07 17:41:14, oshima wrote: > On 2014/10/07 15:19:14, sky wrote: > > Why do you need to do something in both changed and changing? Isn't one > enough? > > I tried and had problem because OnWindowVisibiltyChanged is called twice on the > same target (during Down and Up traversal). Is this by design? That seems wrong. It should only be called once. https://codereview.chromium.org/628413002/diff/100001/ui/wm/core/transient_wi... ui/wm/core/transient_window_manager.cc:167: if (window_ != window || ignore_visibility_changed_event_ || On 2014/10/07 17:41:14, oshima wrote: > On 2014/10/07 15:19:14, sky wrote: > > You shouldn't need the window_ != window. Add a DCHECK if you want it. > > I needed this because OnWindowVisibilityChanged is called on hierarchy chain Ah, ok.
On 2014/10/07 19:34:21, sky wrote: > LGTM > > https://codereview.chromium.org/628413002/diff/100001/ui/wm/core/transient_wi... > File ui/wm/core/transient_window_manager.cc (right): > > https://codereview.chromium.org/628413002/diff/100001/ui/wm/core/transient_wi... > ui/wm/core/transient_window_manager.cc:161: for (auto* child : > transient_children_) > On 2014/10/07 17:46:25, oshima wrote: > > On 2014/10/07 17:41:14, oshima wrote: > > > On 2014/10/07 15:19:14, sky wrote: > > > > From the style guide: "auto is permitted, for local variables only. Do not > > use > > > > auto for file-scope or namespace-scope variables, or for class members." > > > > > > Where is it? I was looking at > > > > > > http://chromium-cpp.appspot.com/ > > > > > > and couldn't find that description. > > > > I guess you meant the description in > > https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#auto > > > > I believe what this is prohibiting is something like > > > > auto global = 1; > > > > namespace { > > auto namespace_scope = "xyz"; > > } > > > > The auto in the loop creates local variable, so this should be fine. > > Yes, you are right. My mistake. > > https://codereview.chromium.org/628413002/diff/100001/ui/wm/core/transient_wi... > ui/wm/core/transient_window_manager.cc:165: void > TransientWindowManager::OnWindowVisibilityChanged(Window* window, > On 2014/10/07 17:41:14, oshima wrote: > > On 2014/10/07 15:19:14, sky wrote: > > > Why do you need to do something in both changed and changing? Isn't one > > enough? > > > > I tried and had problem because OnWindowVisibiltyChanged is called twice on > the > > same target (during Down and Up traversal). Is this by design? > > That seems wrong. It should only be called once. Let's discuss offline and address separately. I can consolidate them if we can fix this. > > https://codereview.chromium.org/628413002/diff/100001/ui/wm/core/transient_wi... > ui/wm/core/transient_window_manager.cc:167: if (window_ != window || > ignore_visibility_changed_event_ || > On 2014/10/07 17:41:14, oshima wrote: > > On 2014/10/07 15:19:14, sky wrote: > > > You shouldn't need the window_ != window. Add a DCHECK if you want it. > > > > I needed this because OnWindowVisibilityChanged is called on hierarchy chain > > Ah, ok.
The CQ bit was checked by oshima@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/628413002/120001
Message was sent while issue was closed.
Committed patchset #3 (id:120001) as 02a13c00350176fa47524821365331a8c585f4c8
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/b6eaaf28e88c84e21e5d52c1f89ef54bc88661a1 Cr-Commit-Position: refs/heads/master@{#298571} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
