Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2096153002:
GetStatusAreaWidget from ash/test in focus_cycler_unittest (Closed)
|
Created:
4 years, 6 months ago by Qiang(Joe) Xu Modified:
4 years, 6 months ago Reviewers:
James Cook CC:
chromium-reviews, kalyank, sadrul Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionGetStatusAreaWidget from ash/test in focus_cycler_unittest
BUG=623212
TEST=focus_cycler_unittest passes
When CL from https://codereview.chromium.org/2087703002/ applied, asan test also passes.
AddressSanitizer: heap-use-after-free is fixed.
Committed: https://crrev.com/7f39741962076e661a559cab8d9bdacdcb02badc
Cr-Commit-Position: refs/heads/master@{#402024}
Patch Set 1 #
Total comments: 10
Patch Set 2 : incorporate comments #
Total comments: 3
Patch Set 3 : delete unused includes #Messages
Total messages: 19 (9 generated)
Description was changed from ========== GetStatusAreaWidget from ash/test in focus_cycler_unittest BUG=623212 ========== to ========== GetStatusAreaWidget from ash/test in focus_cycler_unittest BUG=623212 ==========
warx@chromium.org changed reviewers: + jamescook@chromium.org
Description was changed from ========== GetStatusAreaWidget from ash/test in focus_cycler_unittest BUG=623212 ========== to ========== GetStatusAreaWidget from ash/test in focus_cycler_unittest BUG=623212 TEST=focus_cycler_unittest passes When CL from https://codereview.chromium.org/2087703002/ applied, asan test also passes. AddressSanitizer: heap-use-after-free is fixed. ==========
James, could you PTAL? tks!
The CQ bit was checked by warx@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/2096153002/diff/1/ash/focus_cycler_unittest.cc File ash/focus_cycler_unittest.cc (right): https://codereview.chromium.org/2096153002/diff/1/ash/focus_cycler_unittest.c... ash/focus_cycler_unittest.cc:62: SystemTray* GetSystemTray() { Use AshTestBase::GetPrimarySystemTray() instead of adding this method. https://codereview.chromium.org/2096153002/diff/1/ash/focus_cycler_unittest.c... ash/focus_cycler_unittest.cc:84: bool SetUpTrayFocusCycle() { Keep this protected. https://codereview.chromium.org/2096153002/diff/1/ash/focus_cycler_unittest.c... ash/focus_cycler_unittest.cc:89: if (!GetSystemTray()->GetWidget()) GetWidget() should always return a non-null pointer. If that's important to the test, you could ASSERT_TRUE() here. https://codereview.chromium.org/2096153002/diff/1/ash/focus_cycler_unittest.c... ash/focus_cycler_unittest.cc:91: focus_cycler_->AddWidget(GetSystemTray()->GetWidget()); nit: cache GetSystemTray()->GetWidget() in a local variable https://codereview.chromium.org/2096153002/diff/1/ash/focus_cycler_unittest.c... ash/focus_cycler_unittest.cc:101: shelf_widget()->SetFocusCycler(nullptr); Thanks for fixing these. https://codereview.chromium.org/2096153002/diff/1/ash/focus_cycler_unittest.c... ash/focus_cycler_unittest.cc:138: ASSERT_TRUE(SetUpTrayFocusCycle()); with the assert above you won't need all these other asserts
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Updated. Thanks! https://codereview.chromium.org/2096153002/diff/1/ash/focus_cycler_unittest.cc File ash/focus_cycler_unittest.cc (right): https://codereview.chromium.org/2096153002/diff/1/ash/focus_cycler_unittest.c... ash/focus_cycler_unittest.cc:62: SystemTray* GetSystemTray() { On 2016/06/24 22:31:42, James Cook wrote: > Use AshTestBase::GetPrimarySystemTray() instead of adding this method. Done. https://codereview.chromium.org/2096153002/diff/1/ash/focus_cycler_unittest.c... ash/focus_cycler_unittest.cc:84: bool SetUpTrayFocusCycle() { On 2016/06/24 22:31:42, James Cook wrote: > Keep this protected. Done. https://codereview.chromium.org/2096153002/diff/1/ash/focus_cycler_unittest.c... ash/focus_cycler_unittest.cc:89: if (!GetSystemTray()->GetWidget()) On 2016/06/24 22:31:42, James Cook wrote: > GetWidget() should always return a non-null pointer. If that's important to the > test, you could ASSERT_TRUE() here. Done. https://codereview.chromium.org/2096153002/diff/1/ash/focus_cycler_unittest.c... ash/focus_cycler_unittest.cc:91: focus_cycler_->AddWidget(GetSystemTray()->GetWidget()); On 2016/06/24 22:31:42, James Cook wrote: > nit: cache GetSystemTray()->GetWidget() in a local variable Done.
LGTM with nit Thanks for investigating and fixing this. https://codereview.chromium.org/2096153002/diff/20001/ash/focus_cycler_unitte... File ash/focus_cycler_unittest.cc (right): https://codereview.chromium.org/2096153002/diff/20001/ash/focus_cycler_unitte... ash/focus_cycler_unittest.cc:9: #include "ash/common/shell_window_ids.h" nit: do you need this include? https://codereview.chromium.org/2096153002/diff/20001/ash/focus_cycler_unitte... ash/focus_cycler_unittest.cc:10: #include "ash/common/wm_root_window_controller.h" nit: or this one?
https://codereview.chromium.org/2096153002/diff/20001/ash/focus_cycler_unitte... File ash/focus_cycler_unittest.cc (right): https://codereview.chromium.org/2096153002/diff/20001/ash/focus_cycler_unitte... ash/focus_cycler_unittest.cc:9: #include "ash/common/shell_window_ids.h" On 2016/06/24 23:04:14, James Cook wrote: > nit: do you need this include? Both are not needed. And "ash/common/wm_window.h" is also not used. Thanks for catching : )
The CQ bit was checked by warx@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jamescook@chromium.org Link to the patchset: https://codereview.chromium.org/2096153002/#ps40001 (title: "delete unused includes")
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 ========== GetStatusAreaWidget from ash/test in focus_cycler_unittest BUG=623212 TEST=focus_cycler_unittest passes When CL from https://codereview.chromium.org/2087703002/ applied, asan test also passes. AddressSanitizer: heap-use-after-free is fixed. ========== to ========== GetStatusAreaWidget from ash/test in focus_cycler_unittest BUG=623212 TEST=focus_cycler_unittest passes When CL from https://codereview.chromium.org/2087703002/ applied, asan test also passes. AddressSanitizer: heap-use-after-free is fixed. ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== GetStatusAreaWidget from ash/test in focus_cycler_unittest BUG=623212 TEST=focus_cycler_unittest passes When CL from https://codereview.chromium.org/2087703002/ applied, asan test also passes. AddressSanitizer: heap-use-after-free is fixed. ========== to ========== GetStatusAreaWidget from ash/test in focus_cycler_unittest BUG=623212 TEST=focus_cycler_unittest passes When CL from https://codereview.chromium.org/2087703002/ applied, asan test also passes. AddressSanitizer: heap-use-after-free is fixed. Committed: https://crrev.com/7f39741962076e661a559cab8d9bdacdcb02badc Cr-Commit-Position: refs/heads/master@{#402024} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/7f39741962076e661a559cab8d9bdacdcb02badc Cr-Commit-Position: refs/heads/master@{#402024} |
