Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1092183005:
Fix Up SingletonHwnd Observer Lifetime Issues (Closed)
|
Created:
5 years, 8 months ago by robliao Modified:
5 years, 7 months ago CC:
chromium-reviews, chromium-apps-reviews_chromium.org, darin-cc_chromium.org, jam, derat+watch_chromium.org, extensions-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix Up SingletonHwnd Observer Lifetime Issues
In preparation for handling session change notifications
(WTSRegisterSessionNotification), SingletonHwnd needs to correctly destroy its
own hwnd on destruction and all observers need to be able to handle that.
This delta refactors SingletonHwnd::Observer to be a helper class and only way
to listen for window messages from the SingletonHwnd. It also handles cleanup
for both the observer and the SingletonHwnd if either is destroyed first.
BUG=477462
Committed: https://crrev.com/305070a07d3dc2a18c01a9d3cd9e5d12e29e875d
Cr-Commit-Position: refs/heads/master@{#328017}
Patch Set 1 #
Total comments: 14
Patch Set 2 : CR Feedback #Patch Set 3 : Convert to scoped_ptr #
Total comments: 20
Patch Set 4 : CR Feedback #Patch Set 5 : Removed WndProc null guard #
Total comments: 4
Patch Set 6 : CR Feedback #Patch Set 7 : Quick Clean Up #Patch Set 8 : Rebase to 9449f96 #Patch Set 9 : Allow null SingletonHwnd Hwnd #
Total comments: 2
Messages
Total messages: 62 (25 generated)
robliao@chromium.org changed reviewers: + sky@chromium.org
https://codereview.chromium.org/1092183005/diff/1/chrome/browser/extensions/g... File chrome/browser/extensions/global_shortcut_listener_win.h (right): https://codereview.chromium.org/1092183005/diff/1/chrome/browser/extensions/g... chrome/browser/extensions/global_shortcut_listener_win.h:41: gfx::SingletonHwnd::Observer singletonHwndObserver_; singleton_hwnd_observer_ https://codereview.chromium.org/1092183005/diff/1/content/browser/time_zone_m... File content/browser/time_zone_monitor_win.cc (right): https://codereview.chromium.org/1092183005/diff/1/content/browser/time_zone_m... content/browser/time_zone_monitor_win.cc:34: gfx::SingletonHwnd::Observer singletonHwndObserver_; unix_hacker_style https://codereview.chromium.org/1092183005/diff/1/ui/gfx/win/singleton_hwnd.cc File ui/gfx/win/singleton_hwnd.cc (right): https://codereview.chromium.org/1092183005/diff/1/ui/gfx/win/singleton_hwnd.c... ui/gfx/win/singleton_hwnd.cc:20: if (wndProc_.is_null()) Is there a reason you can't add observer in the constructor and avoid having conditionally adding/removing like you are? Same goes for the wnd_proc. Is there a reason it can't be passed in the constructor? If someone wants to reset the wnd_proc they can destroy the observer. https://codereview.chromium.org/1092183005/diff/1/ui/gfx/win/singleton_hwnd.h File ui/gfx/win/singleton_hwnd.h (right): https://codereview.chromium.org/1092183005/diff/1/ui/gfx/win/singleton_hwnd.h... ui/gfx/win/singleton_hwnd.h:29: class GFX_EXPORT Observer { Put this into its own header. (From the style guide: "Prefer putting delegate classes in their own header files"). https://codereview.chromium.org/1092183005/diff/1/ui/gfx/win/singleton_hwnd.h... ui/gfx/win/singleton_hwnd.h:36: void SetWndProc(const WndProc& wndProc); wnd_proc https://codereview.chromium.org/1092183005/diff/1/ui/gfx/win/singleton_hwnd.h... ui/gfx/win/singleton_hwnd.h:44: WndProc wndProc_; wm_proc_.
Patchset #2 (id:20001) has been deleted
Patchset #2 (id:40001) has been deleted
https://codereview.chromium.org/1092183005/diff/1/chrome/browser/extensions/g... File chrome/browser/extensions/global_shortcut_listener_win.h (right): https://codereview.chromium.org/1092183005/diff/1/chrome/browser/extensions/g... chrome/browser/extensions/global_shortcut_listener_win.h:41: gfx::SingletonHwnd::Observer singletonHwndObserver_; On 2015/04/24 20:58:23, sky wrote: > singleton_hwnd_observer_ Oops, been juggling a few too many standards here. Fixed. https://codereview.chromium.org/1092183005/diff/1/content/browser/time_zone_m... File content/browser/time_zone_monitor_win.cc (right): https://codereview.chromium.org/1092183005/diff/1/content/browser/time_zone_m... content/browser/time_zone_monitor_win.cc:34: gfx::SingletonHwnd::Observer singletonHwndObserver_; On 2015/04/24 20:58:23, sky wrote: > unix_hacker_style Done. https://codereview.chromium.org/1092183005/diff/1/ui/gfx/win/singleton_hwnd.cc File ui/gfx/win/singleton_hwnd.cc (right): https://codereview.chromium.org/1092183005/diff/1/ui/gfx/win/singleton_hwnd.c... ui/gfx/win/singleton_hwnd.cc:20: if (wndProc_.is_null()) On 2015/04/24 20:58:23, sky wrote: > Is there a reason you can't add observer in the constructor and avoid having > conditionally adding/removing like you are? Same goes for the wnd_proc. Is there > a reason it can't be passed in the constructor? If someone wants to reset the > wnd_proc they can destroy the observer. A critical part of SingletonHwndObserver is that it's bound to the lifetime of the requester. This means it either needs to be a member of the requester (and it just works) or the requester needs to notify the SingletonHwndObserver of its destruction (the error prone part). I wanted this to just work, which means it needed to be either a direct member or a scoped_ptr member. All of the classes involved indicate that they will observe at some point in time, but not necessarily at the beginning. I figured if that was the case, might as well make this an inline member of the class and save ourselves a 'new' call. Since we're inlined, to support the case where we may not listen all the time, setting and clearing the WndProc seemed like a natural way to indicate that. The code to handle it is simple enough and it avoids the need to notify SingletonHwnd if the wnd_proc simply changed. https://codereview.chromium.org/1092183005/diff/1/ui/gfx/win/singleton_hwnd.h File ui/gfx/win/singleton_hwnd.h (right): https://codereview.chromium.org/1092183005/diff/1/ui/gfx/win/singleton_hwnd.h... ui/gfx/win/singleton_hwnd.h:29: class GFX_EXPORT Observer { On 2015/04/24 20:58:23, sky wrote: > Put this into its own header. (From the style guide: "Prefer putting delegate > classes in their own header files"). Done. https://codereview.chromium.org/1092183005/diff/1/ui/gfx/win/singleton_hwnd.h... ui/gfx/win/singleton_hwnd.h:36: void SetWndProc(const WndProc& wndProc); On 2015/04/24 20:58:23, sky wrote: > wnd_proc Done. https://codereview.chromium.org/1092183005/diff/1/ui/gfx/win/singleton_hwnd.h... ui/gfx/win/singleton_hwnd.h:44: WndProc wndProc_; On 2015/04/24 20:58:23, sky wrote: > wm_proc_. Done.
https://codereview.chromium.org/1092183005/diff/1/ui/gfx/win/singleton_hwnd.cc File ui/gfx/win/singleton_hwnd.cc (right): https://codereview.chromium.org/1092183005/diff/1/ui/gfx/win/singleton_hwnd.c... ui/gfx/win/singleton_hwnd.cc:20: if (wndProc_.is_null()) On 2015/04/24 21:49:03, robliao wrote: > On 2015/04/24 20:58:23, sky wrote: > > Is there a reason you can't add observer in the constructor and avoid having > > conditionally adding/removing like you are? Same goes for the wnd_proc. Is > there > > a reason it can't be passed in the constructor? If someone wants to reset the > > wnd_proc they can destroy the observer. > > A critical part of SingletonHwndObserver is that it's bound to the lifetime of > the requester. This means it either needs to be a member of the requester (and > it just works) or the requester needs to notify the SingletonHwndObserver of its > destruction (the error prone part). > > I wanted this to just work, which means it needed to be either a direct member > or a scoped_ptr member. All of the classes involved indicate that they will > observe at some point in time, but not necessarily at the beginning. I figured > if that was the case, might as well make this an inline member of the class and > save ourselves a 'new' call. > > Since we're inlined, to support the case where we may not listen all the time, > setting and clearing the WndProc seemed like a natural way to indicate that. The > code to handle it is simple enough and it avoids the need to notify > SingletonHwnd if the wnd_proc simply changed. I would prefer a scoped_ptr. It's make the observer easier to implement and the caller is already dealing with lazy setup.
Patchset #3 (id:80001) has been deleted
https://codereview.chromium.org/1092183005/diff/1/ui/gfx/win/singleton_hwnd.cc File ui/gfx/win/singleton_hwnd.cc (right): https://codereview.chromium.org/1092183005/diff/1/ui/gfx/win/singleton_hwnd.c... ui/gfx/win/singleton_hwnd.cc:20: if (wndProc_.is_null()) On 2015/04/27 15:11:28, sky wrote: > On 2015/04/24 21:49:03, robliao wrote: > > On 2015/04/24 20:58:23, sky wrote: > > > Is there a reason you can't add observer in the constructor and avoid having > > > conditionally adding/removing like you are? Same goes for the wnd_proc. Is > > there > > > a reason it can't be passed in the constructor? If someone wants to reset > the > > > wnd_proc they can destroy the observer. > > > > A critical part of SingletonHwndObserver is that it's bound to the lifetime of > > the requester. This means it either needs to be a member of the requester (and > > it just works) or the requester needs to notify the SingletonHwndObserver of > its > > destruction (the error prone part). > > > > I wanted this to just work, which means it needed to be either a direct member > > or a scoped_ptr member. All of the classes involved indicate that they will > > observe at some point in time, but not necessarily at the beginning. I figured > > if that was the case, might as well make this an inline member of the class > and > > save ourselves a 'new' call. > > > > Since we're inlined, to support the case where we may not listen all the time, > > setting and clearing the WndProc seemed like a natural way to indicate that. > The > > code to handle it is simple enough and it avoids the need to notify > > SingletonHwnd if the wnd_proc simply changed. > > I would prefer a scoped_ptr. It's make the observer easier to implement and the > caller is already dealing with lazy setup. Done.
Two more questions: . I believe you need to update BUILD.gn for the new files. . You originally wanted to do this change because you noticed it takes a while to register the HWND for a particular set of services. If you use SingletonHWND you're still going to be doing that registration on the main thread, right? https://codereview.chromium.org/1092183005/diff/100001/chrome/browser/extensi... File chrome/browser/extensions/global_shortcut_listener_win.cc (right): https://codereview.chromium.org/1092183005/diff/100001/chrome/browser/extensi... chrome/browser/extensions/global_shortcut_listener_win.cc:43: &GlobalShortcutListenerWin::OnHotkey, base::Unretained(this)))); You should document why unretained is safe here. https://codereview.chromium.org/1092183005/diff/100001/content/browser/time_z... File content/browser/time_zone_monitor_win.cc (right): https://codereview.chromium.org/1092183005/diff/100001/content/browser/time_z... content/browser/time_zone_monitor_win.cc:23: &TimeZoneMonitorWin::OnTimeChange, base::Unretained(this)))) {} In general, unretained is error prone and it's best to document why it's safe. https://codereview.chromium.org/1092183005/diff/100001/ui/gfx/font_render_par... File ui/gfx/font_render_params_win.cc (right): https://codereview.chromium.org/1092183005/diff/100001/ui/gfx/font_render_par... ui/gfx/font_render_params_win.cc:87: virtual ~CachedFontRenderParams() { This no longer needs to be virtual. https://codereview.chromium.org/1092183005/diff/100001/ui/gfx/font_render_par... ui/gfx/font_render_params_win.cc:88: // Can't remove the SingletonHwnd observer here since SingletonHwnd may have And this comment doesn't apply anymore. https://codereview.chromium.org/1092183005/diff/100001/ui/gfx/win/singleton_h... File ui/gfx/win/singleton_hwnd.cc (right): https://codereview.chromium.org/1092183005/diff/100001/ui/gfx/win/singleton_h... ui/gfx/win/singleton_hwnd.cc:43: DCHECK(!observer_list_.might_have_observers()); Instead of this define observer_list_ like base::ObserverList<..., true> observer_list_; and you'll get it for free. https://codereview.chromium.org/1092183005/diff/100001/ui/gfx/win/singleton_h... File ui/gfx/win/singleton_hwnd_observer.cc (right): https://codereview.chromium.org/1092183005/diff/100001/ui/gfx/win/singleton_h... ui/gfx/win/singleton_hwnd_observer.cc:18: ClearWndProc(); Why do you need the ClearWndProc? Won't evertyhing work fine without it? https://codereview.chromium.org/1092183005/diff/100001/ui/gfx/win/singleton_h... ui/gfx/win/singleton_hwnd_observer.cc:37: if (message == WM_NCDESTROY) { nit: no {} https://codereview.chromium.org/1092183005/diff/100001/ui/gfx/win/singleton_h... File ui/gfx/win/singleton_hwnd_observer.h (right): https://codereview.chromium.org/1092183005/diff/100001/ui/gfx/win/singleton_h... ui/gfx/win/singleton_hwnd_observer.h:1: // Copyright (c) 2015 The Chromium Authors. All rights reserved. nit: no (c) (see style guide).
On 2015/04/28 13:20:34, sky wrote: > Two more questions: > . I believe you need to update BUILD.gn for the new files. > . You originally wanted to do this change because you noticed it takes a while > to register the HWND for a particular set of services. If you use SingletonHWND > you're still going to be doing that registration on the main thread, right? > > https://codereview.chromium.org/1092183005/diff/100001/chrome/browser/extensi... > File chrome/browser/extensions/global_shortcut_listener_win.cc (right): > > https://codereview.chromium.org/1092183005/diff/100001/chrome/browser/extensi... > chrome/browser/extensions/global_shortcut_listener_win.cc:43: > &GlobalShortcutListenerWin::OnHotkey, base::Unretained(this)))); > You should document why unretained is safe here. > > https://codereview.chromium.org/1092183005/diff/100001/content/browser/time_z... > File content/browser/time_zone_monitor_win.cc (right): > > https://codereview.chromium.org/1092183005/diff/100001/content/browser/time_z... > content/browser/time_zone_monitor_win.cc:23: &TimeZoneMonitorWin::OnTimeChange, > base::Unretained(this)))) {} > In general, unretained is error prone and it's best to document why it's safe. > > https://codereview.chromium.org/1092183005/diff/100001/ui/gfx/font_render_par... > File ui/gfx/font_render_params_win.cc (right): > > https://codereview.chromium.org/1092183005/diff/100001/ui/gfx/font_render_par... > ui/gfx/font_render_params_win.cc:87: virtual ~CachedFontRenderParams() { > This no longer needs to be virtual. > > https://codereview.chromium.org/1092183005/diff/100001/ui/gfx/font_render_par... > ui/gfx/font_render_params_win.cc:88: // Can't remove the SingletonHwnd observer > here since SingletonHwnd may have > And this comment doesn't apply anymore. > > https://codereview.chromium.org/1092183005/diff/100001/ui/gfx/win/singleton_h... > File ui/gfx/win/singleton_hwnd.cc (right): > > https://codereview.chromium.org/1092183005/diff/100001/ui/gfx/win/singleton_h... > ui/gfx/win/singleton_hwnd.cc:43: DCHECK(!observer_list_.might_have_observers()); > Instead of this define observer_list_ like base::ObserverList<..., true> > observer_list_; and you'll get it for free. > > https://codereview.chromium.org/1092183005/diff/100001/ui/gfx/win/singleton_h... > File ui/gfx/win/singleton_hwnd_observer.cc (right): > > https://codereview.chromium.org/1092183005/diff/100001/ui/gfx/win/singleton_h... > ui/gfx/win/singleton_hwnd_observer.cc:18: ClearWndProc(); > Why do you need the ClearWndProc? Won't evertyhing work fine without it? > > https://codereview.chromium.org/1092183005/diff/100001/ui/gfx/win/singleton_h... > ui/gfx/win/singleton_hwnd_observer.cc:37: if (message == WM_NCDESTROY) { > nit: no {} > > https://codereview.chromium.org/1092183005/diff/100001/ui/gfx/win/singleton_h... > File ui/gfx/win/singleton_hwnd_observer.h (right): > > https://codereview.chromium.org/1092183005/diff/100001/ui/gfx/win/singleton_h... > ui/gfx/win/singleton_hwnd_observer.h:1: // Copyright (c) 2015 The Chromium > Authors. All rights reserved. > nit: no (c) (see style guide). Two more questions: . I believe you need to update BUILD.gn for the new files. Indeed. Good catch. . You originally wanted to do this change because you noticed it takes a while to register the HWND for a particular set of services. If you use SingletonHWND you're still going to be doing that registration on the main thread, right? That is correct. The main desire is to do this registration on an off thread SingletonHwnd, but there didn't seem to be much support for that idea, even though it should work. This off-thread SingletonHwnd would post notifications to the main thread, avoiding janking the main thread alltogether. This change sets up the middle ground while cleaning up the SingletonHwnd setup. Since WTS registration should be bound to the lifetime of the hwnd, and SingletonHwnd's lifetime wasn't correctly managed, this delta fixes that and allows a WTS registration with unregistration on the destruction of the HWND.
https://codereview.chromium.org/1092183005/diff/100001/chrome/browser/extensi... File chrome/browser/extensions/global_shortcut_listener_win.cc (right): https://codereview.chromium.org/1092183005/diff/100001/chrome/browser/extensi... chrome/browser/extensions/global_shortcut_listener_win.cc:43: &GlobalShortcutListenerWin::OnHotkey, base::Unretained(this)))); On 2015/04/28 13:20:34, sky wrote: > You should document why unretained is safe here. This use of base::Unretained(this) is fairly idiomatic with an understanding that the lifetime is the same as the holder. This is used in 1000+ occurrences of the codebase without a safety comment. We shouldn't get into the business of having all of these classes support refcounting just to support this callback. https://codereview.chromium.org/1092183005/diff/100001/content/browser/time_z... File content/browser/time_zone_monitor_win.cc (right): https://codereview.chromium.org/1092183005/diff/100001/content/browser/time_z... content/browser/time_zone_monitor_win.cc:23: &TimeZoneMonitorWin::OnTimeChange, base::Unretained(this)))) {} On 2015/04/28 13:20:34, sky wrote: > In general, unretained is error prone and it's best to document why it's safe. See prior comment about this. https://codereview.chromium.org/1092183005/diff/100001/ui/gfx/font_render_par... File ui/gfx/font_render_params_win.cc (right): https://codereview.chromium.org/1092183005/diff/100001/ui/gfx/font_render_par... ui/gfx/font_render_params_win.cc:87: virtual ~CachedFontRenderParams() { On 2015/04/28 13:20:34, sky wrote: > This no longer needs to be virtual. Done. https://codereview.chromium.org/1092183005/diff/100001/ui/gfx/font_render_par... ui/gfx/font_render_params_win.cc:88: // Can't remove the SingletonHwnd observer here since SingletonHwnd may have On 2015/04/28 13:20:34, sky wrote: > And this comment doesn't apply anymore. Done. https://codereview.chromium.org/1092183005/diff/100001/ui/gfx/win/singleton_h... File ui/gfx/win/singleton_hwnd.cc (right): https://codereview.chromium.org/1092183005/diff/100001/ui/gfx/win/singleton_h... ui/gfx/win/singleton_hwnd.cc:43: DCHECK(!observer_list_.might_have_observers()); On 2015/04/28 13:20:34, sky wrote: > Instead of this define observer_list_ like base::ObserverList<..., true> > observer_list_; and you'll get it for free. Neat. Done. https://codereview.chromium.org/1092183005/diff/100001/ui/gfx/win/singleton_h... File ui/gfx/win/singleton_hwnd_observer.cc (right): https://codereview.chromium.org/1092183005/diff/100001/ui/gfx/win/singleton_h... ui/gfx/win/singleton_hwnd_observer.cc:18: ClearWndProc(); On 2015/04/28 13:20:34, sky wrote: > Why do you need the ClearWndProc? Won't evertyhing work fine without it? We need to unregister from the SingletonHwnd in two cases: 1) The SingletonHwnd is tearing down. 2) The owner of this object is tearing down. This call covers case 2. https://codereview.chromium.org/1092183005/diff/100001/ui/gfx/win/singleton_h... ui/gfx/win/singleton_hwnd_observer.cc:37: if (message == WM_NCDESTROY) { On 2015/04/28 13:20:34, sky wrote: > nit: no {} Done. https://codereview.chromium.org/1092183005/diff/100001/ui/gfx/win/singleton_h... File ui/gfx/win/singleton_hwnd_observer.h (right): https://codereview.chromium.org/1092183005/diff/100001/ui/gfx/win/singleton_h... ui/gfx/win/singleton_hwnd_observer.h:1: // Copyright (c) 2015 The Chromium Authors. All rights reserved. On 2015/04/28 13:20:34, sky wrote: > nit: no (c) (see style guide). That presubmit check needs to be fixed. Done here.
https://codereview.chromium.org/1092183005/diff/100001/chrome/browser/extensi... File chrome/browser/extensions/global_shortcut_listener_win.cc (right): https://codereview.chromium.org/1092183005/diff/100001/chrome/browser/extensi... chrome/browser/extensions/global_shortcut_listener_win.cc:43: &GlobalShortcutListenerWin::OnHotkey, base::Unretained(this)))); On 2015/04/28 14:11:55, robliao wrote: > On 2015/04/28 13:20:34, sky wrote: > > You should document why unretained is safe here. > > This use of base::Unretained(this) is fairly idiomatic with an understanding > that the lifetime is the same as the holder. This is used in 1000+ occurrences > of the codebase without a safety comment. > > We shouldn't get into the business of having all of these classes support > refcounting just to support this callback. lifetime same or longer than the holder.
On Tue, Apr 28, 2015 at 7:11 AM, <robliao@chromium.org> wrote: > On 2015/04/28 13:20:34, sky wrote: >> >> Two more questions: >> . I believe you need to update BUILD.gn for the new files. >> . You originally wanted to do this change because you noticed it takes a >> while >> to register the HWND for a particular set of services. If you use > > SingletonHWND >> >> you're still going to be doing that registration on the main thread, >> right? > > > > https://codereview.chromium.org/1092183005/diff/100001/chrome/browser/extensi... >> >> File chrome/browser/extensions/global_shortcut_listener_win.cc (right): > > > > https://codereview.chromium.org/1092183005/diff/100001/chrome/browser/extensi... >> >> chrome/browser/extensions/global_shortcut_listener_win.cc:43: >> &GlobalShortcutListenerWin::OnHotkey, base::Unretained(this)))); >> You should document why unretained is safe here. > > > > https://codereview.chromium.org/1092183005/diff/100001/content/browser/time_z... >> >> File content/browser/time_zone_monitor_win.cc (right): > > > > https://codereview.chromium.org/1092183005/diff/100001/content/browser/time_z... >> >> content/browser/time_zone_monitor_win.cc:23: > > &TimeZoneMonitorWin::OnTimeChange, >> >> base::Unretained(this)))) {} >> In general, unretained is error prone and it's best to document why it's >> safe. > > > > https://codereview.chromium.org/1092183005/diff/100001/ui/gfx/font_render_par... >> >> File ui/gfx/font_render_params_win.cc (right): > > > > https://codereview.chromium.org/1092183005/diff/100001/ui/gfx/font_render_par... >> >> ui/gfx/font_render_params_win.cc:87: virtual ~CachedFontRenderParams() { >> This no longer needs to be virtual. > > > > https://codereview.chromium.org/1092183005/diff/100001/ui/gfx/font_render_par... >> >> ui/gfx/font_render_params_win.cc:88: // Can't remove the SingletonHwnd > > observer >> >> here since SingletonHwnd may have >> And this comment doesn't apply anymore. > > > > https://codereview.chromium.org/1092183005/diff/100001/ui/gfx/win/singleton_h... >> >> File ui/gfx/win/singleton_hwnd.cc (right): > > > > https://codereview.chromium.org/1092183005/diff/100001/ui/gfx/win/singleton_h... >> >> ui/gfx/win/singleton_hwnd.cc:43: > > DCHECK(!observer_list_.might_have_observers()); >> >> Instead of this define observer_list_ like base::ObserverList<..., true> >> observer_list_; and you'll get it for free. > > > > https://codereview.chromium.org/1092183005/diff/100001/ui/gfx/win/singleton_h... >> >> File ui/gfx/win/singleton_hwnd_observer.cc (right): > > > > https://codereview.chromium.org/1092183005/diff/100001/ui/gfx/win/singleton_h... >> >> ui/gfx/win/singleton_hwnd_observer.cc:18: ClearWndProc(); >> Why do you need the ClearWndProc? Won't evertyhing work fine without it? > > > > https://codereview.chromium.org/1092183005/diff/100001/ui/gfx/win/singleton_h... >> >> ui/gfx/win/singleton_hwnd_observer.cc:37: if (message == WM_NCDESTROY) { >> nit: no {} > > > > https://codereview.chromium.org/1092183005/diff/100001/ui/gfx/win/singleton_h... >> >> File ui/gfx/win/singleton_hwnd_observer.h (right): > > > > https://codereview.chromium.org/1092183005/diff/100001/ui/gfx/win/singleton_h... >> >> ui/gfx/win/singleton_hwnd_observer.h:1: // Copyright (c) 2015 The Chromium >> Authors. All rights reserved. >> nit: no (c) (see style guide). > > > Two more questions: > . I believe you need to update BUILD.gn for the new files. > Indeed. Good catch. > > . You originally wanted to do this change because you noticed it takes a > while > to register the HWND for a particular set of services. If you use > SingletonHWND > you're still going to be doing that registration on the main thread, right? > > That is correct. The main desire is to do this registration on an off thread > SingletonHwnd, but there didn't seem to be much support for that idea, even > though it should work. This off-thread SingletonHwnd would post > notifications to > the main thread, avoiding janking the main thread alltogether. I like your changes to SingletonHWND, but if you want something off thread seems like you should create your own standalone hwnd. -Scott > > This change sets up the middle ground while cleaning up the SingletonHwnd > setup. > Since WTS registration should be bound to the lifetime of the hwnd, and > SingletonHwnd's lifetime wasn't correctly managed, this delta fixes that and > allows a WTS registration with unregistration on the destruction of the > HWND. > > https://codereview.chromium.org/1092183005/ To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2015/04/28 14:12:38, robliao wrote: > https://codereview.chromium.org/1092183005/diff/100001/chrome/browser/extensi... > File chrome/browser/extensions/global_shortcut_listener_win.cc (right): > > https://codereview.chromium.org/1092183005/diff/100001/chrome/browser/extensi... > chrome/browser/extensions/global_shortcut_listener_win.cc:43: > &GlobalShortcutListenerWin::OnHotkey, base::Unretained(this)))); > On 2015/04/28 14:11:55, robliao wrote: > > On 2015/04/28 13:20:34, sky wrote: > > > You should document why unretained is safe here. > > > > This use of base::Unretained(this) is fairly idiomatic with an understanding > > that the lifetime is the same as the holder. This is used in 1000+ occurrences > > of the codebase without a safety comment. > > > > We shouldn't get into the business of having all of these classes support > > refcounting just to support this callback. > > lifetime same or longer than the holder. I'm not suggesting you remove Unretained, just document why it's safe to use here.
https://codereview.chromium.org/1092183005/diff/100001/ui/gfx/win/singleton_h... File ui/gfx/win/singleton_hwnd_observer.cc (right): https://codereview.chromium.org/1092183005/diff/100001/ui/gfx/win/singleton_h... ui/gfx/win/singleton_hwnd_observer.cc:18: ClearWndProc(); On 2015/04/28 14:11:55, robliao wrote: > On 2015/04/28 13:20:34, sky wrote: > > Why do you need the ClearWndProc? Won't evertyhing work fine without it? > > We need to unregister from the SingletonHwnd in two cases: > 1) The SingletonHwnd is tearing down. > 2) The owner of this object is tearing down. > > This call covers case 2. I see it. You're taking is_null() to know when you removed the observer. https://codereview.chromium.org/1092183005/diff/100001/ui/gfx/win/singleton_h... ui/gfx/win/singleton_hwnd_observer.cc:32: if (wnd_proc_.is_null()) Shouldn't this be a DCHECK?
On 2015/04/28 16:45:46, sky wrote: > https://codereview.chromium.org/1092183005/diff/100001/ui/gfx/win/singleton_h... > File ui/gfx/win/singleton_hwnd_observer.cc (right): > > https://codereview.chromium.org/1092183005/diff/100001/ui/gfx/win/singleton_h... > ui/gfx/win/singleton_hwnd_observer.cc:18: ClearWndProc(); > On 2015/04/28 14:11:55, robliao wrote: > > On 2015/04/28 13:20:34, sky wrote: > > > Why do you need the ClearWndProc? Won't evertyhing work fine without it? > > > > We need to unregister from the SingletonHwnd in two cases: > > 1) The SingletonHwnd is tearing down. > > 2) The owner of this object is tearing down. > > > > This call covers case 2. > > I see it. You're taking is_null() to know when you removed the observer. > > https://codereview.chromium.org/1092183005/diff/100001/ui/gfx/win/singleton_h... > ui/gfx/win/singleton_hwnd_observer.cc:32: if (wnd_proc_.is_null()) > Shouldn't this be a DCHECK? > I'm not suggesting you remove Unretained, just document why it's safe to use here. I would be more okay with a comment if this was a novel use of base::Unretained. Otherwise, the general case in the Chromium codebase is to use it for the this pointer when you need to do an instance callback and consider the lifetime of the this point with respect to the object holding the callback. The majority of cases are uncommented, and you would still have to do this even in the presence of a comment. What makes this case different from the others?
https://codereview.chromium.org/1092183005/diff/100001/ui/gfx/win/singleton_h... File ui/gfx/win/singleton_hwnd_observer.cc (right): https://codereview.chromium.org/1092183005/diff/100001/ui/gfx/win/singleton_h... ui/gfx/win/singleton_hwnd_observer.cc:32: if (wnd_proc_.is_null()) On 2015/04/28 16:45:46, sky wrote: > Shouldn't this be a DCHECK? Actually, even a DCHECK isn't necessary, as it would only trigger if we're getting ready to crash on the next line. We would only be here if the SingletonHwnd is sending us messages after unregistration. Removed the check.
I would argue they all should have a comment. On Tue, Apr 28, 2015 at 3:02 PM, <robliao@chromium.org> wrote: > On 2015/04/28 16:45:46, sky wrote: > > https://codereview.chromium.org/1092183005/diff/100001/ui/gfx/win/singleton_h... >> >> File ui/gfx/win/singleton_hwnd_observer.cc (right): > > > > https://codereview.chromium.org/1092183005/diff/100001/ui/gfx/win/singleton_h... >> >> ui/gfx/win/singleton_hwnd_observer.cc:18: ClearWndProc(); >> On 2015/04/28 14:11:55, robliao wrote: >> > On 2015/04/28 13:20:34, sky wrote: >> > > Why do you need the ClearWndProc? Won't evertyhing work fine without >> > > it? >> > >> > We need to unregister from the SingletonHwnd in two cases: >> > 1) The SingletonHwnd is tearing down. >> > 2) The owner of this object is tearing down. >> > >> > This call covers case 2. > > >> I see it. You're taking is_null() to know when you removed the observer. > > > > https://codereview.chromium.org/1092183005/diff/100001/ui/gfx/win/singleton_h... >> >> ui/gfx/win/singleton_hwnd_observer.cc:32: if (wnd_proc_.is_null()) >> Shouldn't this be a DCHECK? > > >> I'm not suggesting you remove Unretained, just document why it's safe to >> use > > here. > I would be more okay with a comment if this was a novel use of > base::Unretained. > Otherwise, the general case in the Chromium codebase is to use it for the > this > pointer when you need to do an instance callback and consider the lifetime > of > the this point with respect to the object holding the callback. The majority > of > cases are uncommented, and you would still have to do this even in the > presence > of a comment. > > What makes this case different from the others? > > > https://codereview.chromium.org/1092183005/ To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/1092183005/diff/140001/ui/gfx/font_render_par... File ui/gfx/font_render_params_win.cc (right): https://codereview.chromium.org/1092183005/diff/140001/ui/gfx/font_render_par... ui/gfx/font_render_params_win.cc:89: void OnSettingsChange(HWND hwnd, UINT message, WPARAM wparam, LPARAM lparam) { Naming these functions OnFooChange implies they are only called when 'foo' changes. Since that isn't the case I prefer calling these OnWndProc for improved clarity. https://codereview.chromium.org/1092183005/diff/140001/ui/gfx/win/singleton_h... File ui/gfx/win/singleton_hwnd_observer.cc (right): https://codereview.chromium.org/1092183005/diff/140001/ui/gfx/win/singleton_h... ui/gfx/win/singleton_hwnd_observer.cc:35: ClearWndProc(); Having any code after Run is dangerous as the callback may end up deleting 'this'. In fact a number of the callbacks do this now.
https://codereview.chromium.org/1092183005/diff/140001/ui/gfx/font_render_par... File ui/gfx/font_render_params_win.cc (right): https://codereview.chromium.org/1092183005/diff/140001/ui/gfx/font_render_par... ui/gfx/font_render_params_win.cc:89: void OnSettingsChange(HWND hwnd, UINT message, WPARAM wparam, LPARAM lparam) { On 2015/04/28 23:35:04, sky wrote: > Naming these functions OnFooChange implies they are only called when 'foo' > changes. Since that isn't the case I prefer calling these OnWndProc for improved > clarity. Done. https://codereview.chromium.org/1092183005/diff/140001/ui/gfx/win/singleton_h... File ui/gfx/win/singleton_hwnd_observer.cc (right): https://codereview.chromium.org/1092183005/diff/140001/ui/gfx/win/singleton_h... ui/gfx/win/singleton_hwnd_observer.cc:35: ClearWndProc(); On 2015/04/28 23:35:04, sky wrote: > Having any code after Run is dangerous as the callback may end up deleting > 'this'. In fact a number of the callbacks do this now. The original implementation was resilient against this. I'm okay with observers not forwarding WM_NCDESTROY.
LGTM
robliao@chromium.org changed reviewers: + sievers@chromium.org
sievers@chromium.org: Please review changes in content/browser/time_zone_monitor_win.cc Thanks!
On 2015/04/29 22:00:54, robliao wrote: > mailto:sievers@chromium.org: Please review changes in > content/browser/time_zone_monitor_win.cc > lgtm
Rebased to 9449f96
The CQ bit was checked by robliao@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org, sievers@chromium.org Link to the patchset: https://codereview.chromium.org/1092183005/#ps200001 (title: "Rebase to 9449f96")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1092183005/200001
The CQ bit was unchecked by robliao@chromium.org
The CQ bit was checked by robliao@chromium.org to run a CQ dry run
The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org, sievers@chromium.org Link to the patchset: https://codereview.chromium.org/1092183005/#ps220001 (title: "Strengthen AddObserver Check")
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1092183005/220001
The CQ bit was unchecked by robliao@chromium.org
Patchset #9 (id:220001) has been deleted
The CQ bit was checked by robliao@chromium.org to run a CQ dry run
The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org, sievers@chromium.org Link to the patchset: https://codereview.chromium.org/1092183005/#ps240001 (title: "Allow null SingletonHwnd Hwnd")
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1092183005/240001
The CQ bit was unchecked by robliao@chromium.org
Patchset #9 (id:240001) has been deleted
The CQ bit was checked by robliao@chromium.org to run a CQ dry run
The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org, sievers@chromium.org Link to the patchset: https://codereview.chromium.org/1092183005/#ps260001 (title: "Allow null SingletonHwnd Hwnd")
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1092183005/260001
The CQ bit was unchecked by robliao@chromium.org
Patchset #9 (id:260001) has been deleted
The CQ bit was checked by robliao@chromium.org to run a CQ dry run
The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org, sievers@chromium.org Link to the patchset: https://codereview.chromium.org/1092183005/#ps280001 (title: "Allow null SingletonHwnd Hwnd")
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1092183005/280001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2015/05/01 08:50:16, I haz the power (commit-bot) wrote: > Dry run: This issue passed the CQ dry run. As you can see from the green boxes from Patch Set 9 and the red boxes on Patch Set 8, a few components out there (e.g. running SingletonHwnd in the renderer) depend on _not_ initializing the SingletonHwnd, which means we'll have a null HWND running around. Consequently, the assertion that there will always be an active HWND is not true. The update allows for a null HWND just like before and adds in the proper destruction since we can no longer depend on WM_NCDESTROY with a null hwnd. If there are no objections, I'll send this to the CQ when I wake up over here, so probably around 2-4pm Friday, May 1 PDT.
Did you investigate why there is no hwnd for those tests?
On 2015/05/01 15:17:56, sky wrote: > Did you investigate why there is no hwnd for those tests? They came down to either no MessageLoop (views_unittests) or legitimately not supported to have a window (renderers for content_browsertests and more). This is why there are several tests out there that output this log message: Cannot create windows on non-UI thread!
Fair enough, LGTM
The CQ bit was checked by robliao@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1092183005/280001
Message was sent while issue was closed.
Committed patchset #9 (id:280001)
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/305070a07d3dc2a18c01a9d3cd9e5d12e29e875d Cr-Commit-Position: refs/heads/master@{#328017}
Message was sent while issue was closed.
asvitkine@chromium.org changed reviewers: + asvitkine@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/1092183005/diff/280001/ui/gfx/sys_color_chang... File ui/gfx/sys_color_change_listener.cc (right): https://codereview.chromium.org/1092183005/diff/280001/ui/gfx/sys_color_chang... ui/gfx/sys_color_change_listener.cc:71: scoped_ptr<gfx::SingletonHwndObserver> singleton_hwnd_observer_; Drive-by: Is there a reason these members are scoped_ptrs rather than direct members? Seems the latter should be sufficient.
Message was sent while issue was closed.
https://codereview.chromium.org/1092183005/diff/280001/ui/gfx/sys_color_chang... File ui/gfx/sys_color_change_listener.cc (right): https://codereview.chromium.org/1092183005/diff/280001/ui/gfx/sys_color_chang... ui/gfx/sys_color_change_listener.cc:71: scoped_ptr<gfx::SingletonHwndObserver> singleton_hwnd_observer_; On 2015/05/04 15:44:29, Alexei Svitkine wrote: > Drive-by: > > Is there a reason these members are scoped_ptrs rather than direct members? > Seems the latter should be sufficient. See https://codereview.chromium.org/1092183005/#msg7 for the discussion. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
