Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2261473002:
exo: WMHelperMus: Add focus and activation support. (Closed)
Descriptionexo: WMHelperMus: Add focus and activation support.
BUG=637914
Committed: https://crrev.com/e399c0a42bc0a2be463af70863e264a51ed90bbb
Cr-Commit-Position: refs/heads/master@{#413621}
Patch Set 1 #Patch Set 2 : Update #Patch Set 3 : Update #Patch Set 4 : Rebase #
Total comments: 20
Patch Set 5 : Fix review issues. #
Total comments: 2
Patch Set 6 : Fix a review issue #
Messages
Total messages: 35 (22 generated)
Description was changed from ========== WIP Implement WMHelperMus WIP WIP WIP Run exo in mus+ash BUG= patch from issue 2250863003 at patchset 1 (http://crrev.com/2250863003#ps1) ========== to ========== exo: WMHelperMus: Add focus and activation support. BUG=637914 ==========
penghuang@chromium.org changed reviewers: + reveman@chromium.org, sadrul@chromium.org
Patchset #4 (id:60001) has been deleted
Hi Sadrul & David, PTAL. Thanks.
penghuang@chromium.org changed reviewers: - sadrul@chromium.org
Just rebased the CL. The changes in ui/views/mus is not necessary now, so Sadrul doesn't need review this CL anymore.
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/2261473002/diff/80001/components/exo/wm_helpe... File components/exo/wm_helper_mus.cc (right): https://codereview.chromium.org/2261473002/diff/80001/components/exo/wm_helpe... components/exo/wm_helper_mus.cc:18: if (!window) nit: I prefer if this check was moved to the caller as I find it surprising that it's valid behavior to call this with a null input https://codereview.chromium.org/2261473002/diff/80001/components/exo/wm_helpe... components/exo/wm_helper_mus.cc:22: auto* widget = views::NativeWidgetMus::GetWidgetForWindow(window); nit: I prefer to avoid "auto" unless iterator types or similar where it's clear what the type is and writing the full name provides little value and typically just makes the code harder to read. https://codereview.chromium.org/2261473002/diff/80001/components/exo/wm_helpe... components/exo/wm_helper_mus.cc:33: : active_window_(GetActiveWindow()), focused_window_(GetFocusedWindow()) { hm, can we avoid calling virtual functions in the ctor? while the behavior might be well defined it's not always what the person reading the code expects. https://codereview.chromium.org/2261473002/diff/80001/components/exo/wm_helpe... components/exo/wm_helper_mus.cc:38: auto* connection = views::WindowManagerConnection::Get(); nit: I prefer not using auto here https://codereview.chromium.org/2261473002/diff/80001/components/exo/wm_helpe... components/exo/wm_helper_mus.cc:39: if (connection) do we need this check? https://codereview.chromium.org/2261473002/diff/80001/components/exo/wm_helpe... components/exo/wm_helper_mus.cc:63: auto* active_window = GetActiveWindow(); nit: I prefer not using auto here https://codereview.chromium.org/2261473002/diff/80001/components/exo/wm_helpe... components/exo/wm_helper_mus.cc:66: auto* focus_client = aura::client::GetFocusClient(active_window); nit: I prefer not using auto here https://codereview.chromium.org/2261473002/diff/80001/components/exo/wm_helpe... components/exo/wm_helper_mus.cc:105: OnWindowFocused(nullptr, focused_window_); why is this called twice with null as temporarily focus instead of just one time with old and new focus window? https://codereview.chromium.org/2261473002/diff/80001/components/exo/wm_helpe... components/exo/wm_helper_mus.cc:111: auto* focus_client = aura::client::GetFocusClient(active_window_); nit: I prefer not using auto here https://codereview.chromium.org/2261473002/diff/80001/components/exo/wm_helpe... components/exo/wm_helper_mus.cc:119: auto* focus_client = aura::client::GetFocusClient(active_window_); nit: I prefer not using auto here
https://codereview.chromium.org/2261473002/diff/80001/components/exo/wm_helpe... File components/exo/wm_helper_mus.cc (right): https://codereview.chromium.org/2261473002/diff/80001/components/exo/wm_helpe... components/exo/wm_helper_mus.cc:18: if (!window) On 2016/08/22 19:28:38, reveman wrote: > nit: I prefer if this check was moved to the caller as I find it surprising that > it's valid behavior to call this with a null input Done. https://codereview.chromium.org/2261473002/diff/80001/components/exo/wm_helpe... components/exo/wm_helper_mus.cc:22: auto* widget = views::NativeWidgetMus::GetWidgetForWindow(window); On 2016/08/22 19:28:38, reveman wrote: > nit: I prefer to avoid "auto" unless iterator types or similar where it's clear > what the type is and writing the full name provides little value and typically > just makes the code harder to read. Done. https://codereview.chromium.org/2261473002/diff/80001/components/exo/wm_helpe... components/exo/wm_helper_mus.cc:33: : active_window_(GetActiveWindow()), focused_window_(GetFocusedWindow()) { On 2016/08/22 19:28:38, reveman wrote: > hm, can we avoid calling virtual functions in the ctor? while the behavior might > be well defined it's not always what the person reading the code expects. Done. https://codereview.chromium.org/2261473002/diff/80001/components/exo/wm_helpe... components/exo/wm_helper_mus.cc:38: auto* connection = views::WindowManagerConnection::Get(); On 2016/08/22 19:28:38, reveman wrote: > nit: I prefer not using auto here Done. https://codereview.chromium.org/2261473002/diff/80001/components/exo/wm_helpe... components/exo/wm_helper_mus.cc:39: if (connection) On 2016/08/22 19:28:38, reveman wrote: > do we need this check? Removed it. Done https://codereview.chromium.org/2261473002/diff/80001/components/exo/wm_helpe... components/exo/wm_helper_mus.cc:63: auto* active_window = GetActiveWindow(); On 2016/08/22 19:28:38, reveman wrote: > nit: I prefer not using auto here Done. https://codereview.chromium.org/2261473002/diff/80001/components/exo/wm_helpe... components/exo/wm_helper_mus.cc:66: auto* focus_client = aura::client::GetFocusClient(active_window); On 2016/08/22 19:28:38, reveman wrote: > nit: I prefer not using auto here Done. https://codereview.chromium.org/2261473002/diff/80001/components/exo/wm_helpe... components/exo/wm_helper_mus.cc:105: OnWindowFocused(nullptr, focused_window_); On 2016/08/22 19:28:38, reveman wrote: > why is this called twice with null as temporarily focus instead of just one time > with old and new focus window? Done. https://codereview.chromium.org/2261473002/diff/80001/components/exo/wm_helpe... components/exo/wm_helper_mus.cc:111: auto* focus_client = aura::client::GetFocusClient(active_window_); On 2016/08/22 19:28:38, reveman wrote: > nit: I prefer not using auto here Done. https://codereview.chromium.org/2261473002/diff/80001/components/exo/wm_helpe... components/exo/wm_helper_mus.cc:119: auto* focus_client = aura::client::GetFocusClient(active_window_); On 2016/08/22 19:28:38, reveman wrote: > nit: I prefer not using auto here Done.
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...
lgtm https://codereview.chromium.org/2261473002/diff/100001/components/exo/wm_help... File components/exo/wm_helper_mus.cc (right): https://codereview.chromium.org/2261473002/diff/100001/components/exo/wm_help... components/exo/wm_helper_mus.cc:25: } nit: blank line after this to be consistent with having a blank line after "namespace {"
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
penghuang@chromium.org changed reviewers: + sky@chromium.org
Hi Scott, PTAL. Thanks.
The CQ bit was checked by penghuang@chromium.org to run a CQ dry run
https://codereview.chromium.org/2261473002/diff/100001/components/exo/wm_help... File components/exo/wm_helper_mus.cc (right): https://codereview.chromium.org/2261473002/diff/100001/components/exo/wm_help... components/exo/wm_helper_mus.cc:25: } On 2016/08/22 21:38:33, reveman wrote: > nit: blank line after this to be consistent with having a blank line after > "namespace {" Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
DEP LGTM
The CQ bit was unchecked by penghuang@chromium.org
The CQ bit was checked by penghuang@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from reveman@chromium.org Link to the patchset: https://codereview.chromium.org/2261473002/#ps120001 (title: "Fix a review issue")
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: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_clobber_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 penghuang@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.
Description was changed from ========== exo: WMHelperMus: Add focus and activation support. BUG=637914 ========== to ========== exo: WMHelperMus: Add focus and activation support. BUG=637914 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== exo: WMHelperMus: Add focus and activation support. BUG=637914 ========== to ========== exo: WMHelperMus: Add focus and activation support. BUG=637914 Committed: https://crrev.com/e399c0a42bc0a2be463af70863e264a51ed90bbb Cr-Commit-Position: refs/heads/master@{#413621} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/e399c0a42bc0a2be463af70863e264a51ed90bbb Cr-Commit-Position: refs/heads/master@{#413621} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
