Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2420123002:
Remove dependency from SignInBrowsertest on tracing events watching (Closed)
|
Created:
4 years, 2 months ago by Primiano Tucci (use gerrit) Modified:
4 years, 2 months ago CC:
chromium-reviews, alemate+watch_chromium.org, achuith+watch_chromium.org, oshima+watch_chromium.org, davemoore+watch_chromium.org, oystein (OOO til 10th of July) Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRemove dependency from SignInBrowsertest on tracing events watching
LoginSigninTest.WebUIVisible is relying on tracing events watching for
the only sake of figuring out when the signin screen has been shown.
This is a hack and adds an unnecessary dependency on all the tracing
machinery to achieve what could be simply achieved with
WindowedNotificationObserver.
More importantly, this is the only dependency on events watching left
in the codebase. I am going to kill such feature entirely after this
CL to prevent further situations like this.
BUG=
TEST=LoginSigninTest.WebUIVisible
Committed: https://crrev.com/399cebdad3ff6cf4c6ac37e85db3c18233d9bdf9
Cr-Commit-Position: refs/heads/master@{#426198}
Patch Set 1 #
Total comments: 2
Patch Set 2 : use WindowedNotificationObserver #
Dependent Patchsets: Messages
Total messages: 25 (14 generated)
The CQ bit was checked by primiano@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...
Description was changed from ========== Remove dependency from SignInBrowsertest on tracing events watching LoginSigninTest.WebUIVisible is relying on tracing events watching for the only sake of figuring out when the signin screen has been shown. This is a hack and adds an unnecessary dependency on all the tracing machinery to achieve what could be simply achieved with a callback_for_testing. More importantly, this is the only dependency on events watching left in the codebase. I am going to kill such feature entirely after this CL, to prevent further situations like this. BUG= TEST=LoginSigninTest.WebUIVisible ========== to ========== Remove dependency from SignInBrowsertest on tracing events watching LoginSigninTest.WebUIVisible is relying on tracing events watching for the only sake of figuring out when the signin screen has been shown. This is a hack and adds an unnecessary dependency on all the tracing machinery to achieve what could be simply achieved with a callback_for_testing. More importantly, this is the only dependency on events watching left in the codebase. I am going to kill such feature entirely after this CL to prevent further situations like this. BUG= TEST=LoginSigninTest.WebUIVisible ==========
primiano@chromium.org changed reviewers: + stevenjb@chromium.org, xiyuan@chromium.org
Agree that we should not depends on tracing for this.
Think we can use this:
content::WindowedNotificationObserver(
chrome::NOTIFICATION_LOGIN_OR_LOCK_WEBUI_VISIBLE,
content::NotificationService::AllSources()).Wait();
instead of adding the callback for test.
Mind to give it a try?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2420123002/diff/1/chrome/browser/ui/webui/chr... File chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc (right): https://codereview.chromium.org/2420123002/diff/1/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:117: LAZY_INSTANCE_INITIALIZER; No trailing _ in file local variables. If this is just a Closure*, we don't need to use LazyInstance: base::Closure* g_ui_shown_callback_for_testing = nullptr; Then below: if (g_ui_shown_callback_for_testing) g_ui_shown_callback_for_testing->Run();
https://codereview.chromium.org/2420123002/diff/1/chrome/browser/ui/webui/chr... File chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc (right): https://codereview.chromium.org/2420123002/diff/1/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:117: LAZY_INSTANCE_INITIALIZER; On 2016/10/14 17:31:55, stevenjb wrote: > No trailing _ in file local variables. > > If this is just a Closure*, we don't need to use LazyInstance: > > base::Closure* g_ui_shown_callback_for_testing = nullptr; > > Then below: > > if (g_ui_shown_callback_for_testing) > g_ui_shown_callback_for_testing->Run(); I am not sure I follow, how do you expect the g_closure ptr to be set? Definitely It cannot be set to a pointer to some stack allocated thing. The only alternative way I see is doing g_closure = new base::Closure .... but this will make memory sanitizers unhappy because will be seen as a leak. At this point you could add a ANNOTATE_LEAKING_OBJECT_PTR, but at this point you now have more complexity than lazyinstance itself.
The CQ bit was checked by primiano@chromium.org to run a CQ dry run
anyways, WindowedNotificationObserver seems to have worked. thanks
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm Thanks.
On 2016/10/14 17:58:52, xiyuan wrote: > lgtm > > Thanks. Oh, please update the CL description to reflect the new CL.
On 2016/10/14 17:42:56, Primiano Tucci wrote: > https://codereview.chromium.org/2420123002/diff/1/chrome/browser/ui/webui/chr... > File chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc (right): > > https://codereview.chromium.org/2420123002/diff/1/chrome/browser/ui/webui/chr... > chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:117: > LAZY_INSTANCE_INITIALIZER; > On 2016/10/14 17:31:55, stevenjb wrote: > > No trailing _ in file local variables. > > > > If this is just a Closure*, we don't need to use LazyInstance: > > > > base::Closure* g_ui_shown_callback_for_testing = nullptr; > > > > Then below: > > > > if (g_ui_shown_callback_for_testing) > > g_ui_shown_callback_for_testing->Run(); > > I am not sure I follow, how do you expect the g_closure ptr to be set? > Definitely It cannot be set to a pointer to some stack allocated thing. > The only alternative way I see is doing > g_closure = new base::Closure .... > > but this will make memory sanitizers unhappy because will be seen as a leak. > At this point you could add a ANNOTATE_LEAKING_OBJECT_PTR, but at this point you > now have more complexity than lazyinstance itself. WindowedNotificationObserver is definitely a better solution, thanks xiyuan for the suggestion. FWIW, in the suggestion I made, the callback would need to be owned and destroyed (and the global set to null) in the test. It's a little more work but minimizes the overhead in non test scenarios. But again, this solution is better. LGTM
Description was changed from ========== Remove dependency from SignInBrowsertest on tracing events watching LoginSigninTest.WebUIVisible is relying on tracing events watching for the only sake of figuring out when the signin screen has been shown. This is a hack and adds an unnecessary dependency on all the tracing machinery to achieve what could be simply achieved with a callback_for_testing. More importantly, this is the only dependency on events watching left in the codebase. I am going to kill such feature entirely after this CL to prevent further situations like this. BUG= TEST=LoginSigninTest.WebUIVisible ========== to ========== Remove dependency from SignInBrowsertest on tracing events watching LoginSigninTest.WebUIVisible is relying on tracing events watching for the only sake of figuring out when the signin screen has been shown. This is a hack and adds an unnecessary dependency on all the tracing machinery to achieve what could be simply achieved with WindowedNotificationObserver. More importantly, this is the only dependency on events watching left in the codebase. I am going to kill such feature entirely after this CL to prevent further situations like this. BUG= TEST=LoginSigninTest.WebUIVisible ==========
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 primiano@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 ========== Remove dependency from SignInBrowsertest on tracing events watching LoginSigninTest.WebUIVisible is relying on tracing events watching for the only sake of figuring out when the signin screen has been shown. This is a hack and adds an unnecessary dependency on all the tracing machinery to achieve what could be simply achieved with WindowedNotificationObserver. More importantly, this is the only dependency on events watching left in the codebase. I am going to kill such feature entirely after this CL to prevent further situations like this. BUG= TEST=LoginSigninTest.WebUIVisible ========== to ========== Remove dependency from SignInBrowsertest on tracing events watching LoginSigninTest.WebUIVisible is relying on tracing events watching for the only sake of figuring out when the signin screen has been shown. This is a hack and adds an unnecessary dependency on all the tracing machinery to achieve what could be simply achieved with WindowedNotificationObserver. More importantly, this is the only dependency on events watching left in the codebase. I am going to kill such feature entirely after this CL to prevent further situations like this. BUG= TEST=LoginSigninTest.WebUIVisible ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Remove dependency from SignInBrowsertest on tracing events watching LoginSigninTest.WebUIVisible is relying on tracing events watching for the only sake of figuring out when the signin screen has been shown. This is a hack and adds an unnecessary dependency on all the tracing machinery to achieve what could be simply achieved with WindowedNotificationObserver. More importantly, this is the only dependency on events watching left in the codebase. I am going to kill such feature entirely after this CL to prevent further situations like this. BUG= TEST=LoginSigninTest.WebUIVisible ========== to ========== Remove dependency from SignInBrowsertest on tracing events watching LoginSigninTest.WebUIVisible is relying on tracing events watching for the only sake of figuring out when the signin screen has been shown. This is a hack and adds an unnecessary dependency on all the tracing machinery to achieve what could be simply achieved with WindowedNotificationObserver. More importantly, this is the only dependency on events watching left in the codebase. I am going to kill such feature entirely after this CL to prevent further situations like this. BUG= TEST=LoginSigninTest.WebUIVisible Committed: https://crrev.com/399cebdad3ff6cf4c6ac37e85db3c18233d9bdf9 Cr-Commit-Position: refs/heads/master@{#426198} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/399cebdad3ff6cf4c6ac37e85db3c18233d9bdf9 Cr-Commit-Position: refs/heads/master@{#426198} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
