Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1567103005:
Replace base::CallbackList with base::ObserverList in CastConfigDelegate (Closed)
DescriptionReplace base::CallbackList with base::ObserverList in CastConfigDelegate
base::CallbackList::Observer invoked a destructor inside of TrayCast. Due to wonky deinitialization order between the system tray delegate and the status area, this caused a crash.
base::ObserverList deinitialization is more easily controlled than base::CallbackList. Switch to that so we don't deinitialize the observer if it's already been removed.
Fixing the deinitialization order between the system tray delegate and the status area is the correct fix, but it will take significantly more time and needs to be thoroughly tested.
BUG=574874
Committed: https://crrev.com/6b717bb1e3bed46518e5410fbf80683114cdcb69
Cr-Commit-Position: refs/heads/master@{#369508}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Remove subscription, support at most one listener #Patch Set 3 : Replace with ObserverList instead of base::Callback #
Total comments: 12
Patch Set 4 : Fix compilation and address comments #Patch Set 5 : Add DLL export to fix windows build #
Total comments: 27
Patch Set 6 : Address feedback #Patch Set 7 : Add DISALLOW_ASSIGN to Observer #
Total comments: 3
Patch Set 8 : Add TODO for proper fix #Messages
Total messages: 51 (19 generated)
Description was changed from ========== Deinitialize the ash shelf before the system tray delegate. Currently, the system tray delegate is initialized before the shelf. BUG=574874 ========== to ========== Deinitialize the ash shelf before the system tray delegate. Currently, the system tray delegate is initialized before the shelf. BUG=574874 ==========
jdufault@chromium.org changed reviewers: + achuith@chromium.org, oshima@chromium.org
Oshima, PTAL. This should be what we discussed. achuith@ for FYI, since this is causing a crash in TrayCast. Thanks!
https://codereview.chromium.org/1567103005/diff/1/ash/shell.cc File ash/shell.cc (right): https://codereview.chromium.org/1567103005/diff/1/ash/shell.cc#newcode732 ash/shell.cc:732: // Destroy SystemTrayDelegate before destroying the status area(s). Make sure The original comment should go after HideShelf. HideShelf does seem to destroy status area though. Are you sure it's safe? Or shouldn't it just call CloseSystemBubble() ?
The CQ bit was checked by jdufault@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1567103005/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1567103005/1
https://codereview.chromium.org/1567103005/diff/1/ash/shell.cc File ash/shell.cc (right): https://codereview.chromium.org/1567103005/diff/1/ash/shell.cc#newcode732 ash/shell.cc:732: // Destroy SystemTrayDelegate before destroying the status area(s). Make sure On 2016/01/09 01:05:11, oshima wrote: > The original comment should go after HideShelf. HideShelf does seem to destroy > status area though. Are you sure it's safe? Or shouldn't it just call > CloseSystemBubble() ? Calling (*iter)->GetSystemTray()->CloseSystemBubble() doesn't cause the TrayCast dtor to run. Thinking about it more, this makes since, since TrayCast lives beyond the shelf popup - it has methods (CreateTrayView, DestroyTrayView, etc) that control the lifetime for the popup. So hiding the popup itself will not be enough. You're right though - this change is destroying the status area before the SystemTrayDelegate. Though I think this is the correct order, since the status area is initialized after the delegate. I'm a bit concerned that this will cause other things to crash, but it seems OK from a cursory overview on my device. However, this change needs to be merged into m48, which goes stable in two weeks. Any thoughts? Instead of this somewhat risky change, I can look into using weak pointers in TrayCast instead, though that seems like a band-aid fix. The comment about the deinit order was made in this[1] CL. 1: https://codereview.chromium.org/25111002
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
On 2016/01/11 20:59:34, jdufault wrote: > https://codereview.chromium.org/1567103005/diff/1/ash/shell.cc > File ash/shell.cc (right): > > https://codereview.chromium.org/1567103005/diff/1/ash/shell.cc#newcode732 > ash/shell.cc:732: // Destroy SystemTrayDelegate before destroying the status > area(s). Make sure > On 2016/01/09 01:05:11, oshima wrote: > > The original comment should go after HideShelf. HideShelf does seem to destroy > > status area though. Are you sure it's safe? Or shouldn't it just call > > CloseSystemBubble() ? > > Calling (*iter)->GetSystemTray()->CloseSystemBubble() doesn't cause the TrayCast > dtor to run. Thinking about it more, this makes since, since TrayCast lives > beyond the shelf popup - it has methods (CreateTrayView, DestroyTrayView, etc) > that control the lifetime for the popup. So hiding the popup itself will not be > enough. > > You're right though - this change is destroying the status area before the > SystemTrayDelegate. Though I think this is the correct order, since the status > area is initialized after the delegate. I'm a bit concerned that this will cause > other things to crash, but it seems OK from a cursory overview on my device. > However, this change needs to be merged into m48, which goes stable in two > weeks. > > Any thoughts? Instead of this somewhat risky change, I can look into using weak > pointers in TrayCast instead, though that seems like a band-aid fix. > > The comment about the deinit order was made in this[1] CL. > > 1: https://codereview.chromium.org/25111002 It's just moving the code. The comment and the code predates that CL. and reordering seems to be causing crash in keyboard unit tests. Usually the dependecy is "main -> delegates", not the opposite, so I agree that deleting delegates later is good in general though. Please ask bshe@/stevenjb@ how/if this can be changed.
jdufault@chromium.org changed reviewers: + bshe@chromium.org, stevenjb@chromium.org
bshe@ / stevenjb@, the deinitialization order for TrayCast (part of the status area) is causing a crash. TrayCast assumes that SystemTrayDelegate is still alive in its dtor, however, the delegate is destroyed too soon. Overall, the flow looks like this: SystemTrayDelegate() StatusArea() // ... ~SystemTrayDelegate() ~StatusArea() There is a comment in the code that says that the delegate needs to be destroyed before the status area. I'm not sure why. Any thoughts on if we can get StatusArea to deinitialize before SystemTrayDelegate? Doing so causes some of the keyboard tests to fail. More on that below. I think I can work-around the crash with a band-aid fix by using weak pointers, but I'm not a fan of that solution since it feels like a band-aid. As an additional complication, this fix will need to be merged into m48, which which go stable in two weeks or so. *If* we change the initialization order, some keyboard tests start to fail. It looks like it is caused from this[1] call. fwiw, here's the failing test stack trace: Received signal 11 SEGV_MAPERR 000000000038 #0 0x00000091743b base::debug::(anonymous namespace)::StackDumpSignalHandler() #1 0x7fbce99e2cb0 <unknown> #2 0x000000b3f7b4 views::Widget::GetWindowBoundsInScreen() #3 0x000000baa1b5 ash::ShelfLayoutManager::CalculateTargetBounds() #4 0x000000ba9d8d ash::ShelfLayoutManager::LayoutShelf() #5 0x000000bac5a9 ash::ShelfLayoutManager::OnKeyboardBoundsChanging() #6 0x000000ae5d04 keyboard::KeyboardController::NotifyKeyboardBoundsChanging() #7 0x000000ba349e ash::RootWindowController::DeactivateKeyboard() #8 0x000000ba2e50 ash::RootWindowController::CloseChildWindows() #9 0x000000b7b6d1 ash::WindowTreeHostManager::CloseChildWindows() #10 0x000000bbb2d5 ash::Shell::~Shell() 1: https://code.google.com/p/chromium/codesearch#chromium/src/ash/shell.cc&l=755
Unfortunately, SystemTrayDelegateChromeOS has become a catch-all for Chrome specific Ash code. Despite some efforts to reduce that, the class still inherits from 13 observer classes and calls into ash::Shell::GetInstance() 17 times. Auditing all of that code to ensure that it is safe to destroy the StatusArea first would be a fair bit of effort. That said, you're not wrong about the destruction order being unexpected, and I would love to see SystemTrayDelegateChromeOS cleaned up, so if you feel motivated to give it a go, I for one would support the effort. In the short term, *all* StatusArea widgets should check SystemTrayDelegate in any code accessed by the destructor. This is a good idea any time a global is accessed during destruction anyway. Furthermore (and somewhat orthogonal, but worth mentioning I think), any class that derives from views::View should test any globals that might be accessed from their destructor. Since Views are owned by the Views hierarchy, the ash code does not directly control when they are destroyed. On Mon, Jan 11, 2016 at 4:21 PM, <jdufault@chromium.org> wrote: > bshe@ / stevenjb@, the deinitialization order for TrayCast (part of the > status > area) is causing a crash. TrayCast assumes that SystemTrayDelegate is still > alive in its dtor, however, the delegate is destroyed too soon. Overall, > the > flow looks like this: > > SystemTrayDelegate() > StatusArea() > // ... > ~SystemTrayDelegate() > ~StatusArea() > > There is a comment in the code that says that the delegate needs to be > destroyed > before the status area. I'm not sure why. Any thoughts on if we can get > StatusArea to deinitialize before SystemTrayDelegate? Doing so causes some > of > the keyboard tests to fail. More on that below. > > I think I can work-around the crash with a band-aid fix by using weak > pointers, > but I'm not a fan of that solution since it feels like a band-aid. > > As an additional complication, this fix will need to be merged into m48, > which > which go stable in two weeks or so. > > *If* we change the initialization order, some keyboard tests start to > fail. It > looks like it is caused from this[1] call. fwiw, here's the failing test > stack > trace: > > Received signal 11 SEGV_MAPERR 000000000038 > #0 0x00000091743b base::debug::(anonymous > namespace)::StackDumpSignalHandler() > #1 0x7fbce99e2cb0 <unknown> > #2 0x000000b3f7b4 views::Widget::GetWindowBoundsInScreen() > #3 0x000000baa1b5 ash::ShelfLayoutManager::CalculateTargetBounds() > #4 0x000000ba9d8d ash::ShelfLayoutManager::LayoutShelf() > #5 0x000000bac5a9 ash::ShelfLayoutManager::OnKeyboardBoundsChanging() > #6 0x000000ae5d04 > keyboard::KeyboardController::NotifyKeyboardBoundsChanging() > #7 0x000000ba349e ash::RootWindowController::DeactivateKeyboard() > #8 0x000000ba2e50 ash::RootWindowController::CloseChildWindows() > #9 0x000000b7b6d1 ash::WindowTreeHostManager::CloseChildWindows() > #10 0x000000bbb2d5 ash::Shell::~Shell() > > 1: > https://code.google.com/p/chromium/codesearch#chromium/src/ash/shell.cc&l=755 > > https://codereview.chromium.org/1567103005/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Description was changed from ========== Deinitialize the ash shelf before the system tray delegate. Currently, the system tray delegate is initialized before the shelf. BUG=574874 ========== to ========== Support at most one listener for cast updates in CastConfigDelegate. Supporting multiple listeners caused a crash due to a wonky deinitialization order between system tray delegate and the status area. Fixing the deinitialization order is the correct fix, but it will take significantly more time and needs to be thoroughly tested, so for the time being we can work-around the issue by only supporting one listener. BUG=574874 ==========
oshima@/achuith@, PTAL. Given that this fix needs to be merged into m48, I've worked around the deinitialization order issue by simply supporting at most one observer. This removes the dtor call from TrayCast into CastConfigDelegateMediaRouter, which was the cause of the crash. However, the correct solution is to fix the deinitialization order. That seems like too risky of a change to me especially given stevenjb@'s comments.
This seems awkward. Why not define an Observer and use an ObserverList?
On 2016/01/12 22:19:26, stevenjb wrote: > This seems awkward. Why not define an Observer and use an ObserverList? Ah, perfect! I was looking for something like that but couldn't find it. I'll switch the code over.
Description was changed from ========== Support at most one listener for cast updates in CastConfigDelegate. Supporting multiple listeners caused a crash due to a wonky deinitialization order between system tray delegate and the status area. Fixing the deinitialization order is the correct fix, but it will take significantly more time and needs to be thoroughly tested, so for the time being we can work-around the issue by only supporting one listener. BUG=574874 ========== to ========== Replace base::CallbackList with base::ObserverList in CastConfigDelegate base::CallbackList::Observer invoked a destructor inside of TrayCast. Due to wonky deinitialization order between the system tray delegate and the status area, this caused a crash. base::ObserverList deinitialization is more easily controlled than base::CallbackList. Switch to that so we don't deinitialize the observer if it's already been removed. Fixing the deinitialization order between the system tray delegate and the status area is the correct fix, but it will take significantly more time and needs to be thoroughly tested. BUG=574874 ==========
On 2016/01/12 22:19:26, stevenjb wrote: > This seems awkward. Why not define an Observer and use an ObserverList? Done
https://codereview.chromium.org/1567103005/diff/40001/ash/cast_config_delegate.h File ash/cast_config_delegate.h (right): https://codereview.chromium.org/1567103005/diff/40001/ash/cast_config_delegat... ash/cast_config_delegate.h:80: Typically an observer class with only one or two methods should be a pure virtual with no default implementations. (I'm actually surprised this didn't generate a link error). Also, all virtual classes need an explicit destructor (which can be inlined if it is a pure virtual class). https://codereview.chromium.org/1567103005/diff/40001/ash/system/cast/tray_ca... File ash/system/cast/tray_cast.cc (right): https://codereview.chromium.org/1567103005/diff/40001/ash/system/cast/tray_ca... ash/system/cast/tray_cast.cc:47: return nullptr; nit: {} https://codereview.chromium.org/1567103005/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/api/cast_devices_private/cast_devices_private_api.h (right): https://codereview.chromium.org/1567103005/diff/40001/chrome/browser/extensio... chrome/browser/extensions/api/cast_devices_private/cast_devices_private_api.h:24: // Fetch an instance for the given context. s/Fetch/Fetches/ (while you're in here) https://codereview.chromium.org/1567103005/diff/40001/chrome/browser/extensio... chrome/browser/extensions/api/cast_devices_private/cast_devices_private_api.h:27: // Set the function that will be invoked only when a new device update is s/Set/Sets/ https://codereview.chromium.org/1567103005/diff/40001/chrome/browser/ui/ash/c... File chrome/browser/ui/ash/cast_config_delegate_chromeos.cc (right): https://codereview.chromium.org/1567103005/diff/40001/chrome/browser/ui/ash/c... chrome/browser/ui/ash/cast_config_delegate_chromeos.cc:71: return listeners->AddObserver(observer); nit: Local is unnecessary https://codereview.chromium.org/1567103005/diff/40001/chrome/browser/ui/ash/c... chrome/browser/ui/ash/cast_config_delegate_chromeos.cc:76: return listeners->RemoveObserver(observer); Local unnecessary
https://codereview.chromium.org/1567103005/diff/40001/ash/cast_config_delegate.h File ash/cast_config_delegate.h (right): https://codereview.chromium.org/1567103005/diff/40001/ash/cast_config_delegat... ash/cast_config_delegate.h:80: On 2016/01/12 23:31:49, stevenjb wrote: > Typically an observer class with only one or two methods should be a pure > virtual with no default implementations. (I'm actually surprised this didn't > generate a link error). > > Also, all virtual classes need an explicit destructor (which can be inlined if > it is a pure virtual class). Done. https://codereview.chromium.org/1567103005/diff/40001/ash/system/cast/tray_ca... File ash/system/cast/tray_cast.cc (right): https://codereview.chromium.org/1567103005/diff/40001/ash/system/cast/tray_ca... ash/system/cast/tray_cast.cc:47: return nullptr; On 2016/01/12 23:31:49, stevenjb wrote: > nit: {} Done. https://codereview.chromium.org/1567103005/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/api/cast_devices_private/cast_devices_private_api.h (right): https://codereview.chromium.org/1567103005/diff/40001/chrome/browser/extensio... chrome/browser/extensions/api/cast_devices_private/cast_devices_private_api.h:24: // Fetch an instance for the given context. On 2016/01/12 23:31:49, stevenjb wrote: > s/Fetch/Fetches/ (while you're in here) Done. https://codereview.chromium.org/1567103005/diff/40001/chrome/browser/extensio... chrome/browser/extensions/api/cast_devices_private/cast_devices_private_api.h:27: // Set the function that will be invoked only when a new device update is On 2016/01/12 23:31:50, stevenjb wrote: > s/Set/Sets/ Done. https://codereview.chromium.org/1567103005/diff/40001/chrome/browser/ui/ash/c... File chrome/browser/ui/ash/cast_config_delegate_chromeos.cc (right): https://codereview.chromium.org/1567103005/diff/40001/chrome/browser/ui/ash/c... chrome/browser/ui/ash/cast_config_delegate_chromeos.cc:71: return listeners->AddObserver(observer); On 2016/01/12 23:31:50, stevenjb wrote: > nit: Local is unnecessary Done. https://codereview.chromium.org/1567103005/diff/40001/chrome/browser/ui/ash/c... chrome/browser/ui/ash/cast_config_delegate_chromeos.cc:76: return listeners->RemoveObserver(observer); On 2016/01/12 23:31:50, stevenjb wrote: > Local unnecessary Done.
lgtm
WallpaperManagerBrowserTest.DisplayChange seems to be a real failure. https://codereview.chromium.org/1567103005/diff/80001/ash/cast_config_delegate.h File ash/cast_config_delegate.h (right): https://codereview.chromium.org/1567103005/diff/80001/ash/cast_config_delegat... ash/cast_config_delegate.h:12: #include "base/callback.h" Do you need this? https://codereview.chromium.org/1567103005/diff/80001/ash/cast_config_delegat... ash/cast_config_delegate.h:77: virtual ~Observer() {} I think this can be protected? https://codereview.chromium.org/1567103005/diff/80001/ash/cast_config_delegat... ash/cast_config_delegate.h:81: }; DISALLOW_COPY_AND_ASSIGN perhaps? https://codereview.chromium.org/1567103005/diff/80001/ash/cast_config_delegat... ash/cast_config_delegate.h:90: // Add or remove an observer. I feel like these administrative functions should be the last functions in this class since they are the least interesting for readers of this file. https://codereview.chromium.org/1567103005/diff/80001/ash/system/cast/tray_ca... File ash/system/cast/tray_cast.cc (right): https://codereview.chromium.org/1567103005/diff/80001/ash/system/cast/tray_ca... ash/system/cast/tray_cast.cc:45: if (!ash::Shell::GetInstance() || Could you please add a comment here explaining when this happens? (I believe shutdown?) https://codereview.chromium.org/1567103005/diff/80001/ash/system/cast/tray_ca... ash/system/cast/tray_cast.cc:562: Shell::GetInstance()->RemoveShellObserver(this); Do we need to check Shell::GetInstance here? https://codereview.chromium.org/1567103005/diff/80001/ash/system/cast/tray_ca... ash/system/cast/tray_cast.cc:564: if (GetCastConfigDelegate()) Using a temporary is an option here so we don't call GetCastConfigDelegate twice. Also, should we be checking added_observer_? https://codereview.chromium.org/1567103005/diff/80001/ash/system/cast/tray_ca... ash/system/cast/tray_cast.cc:598: // We add the cast listener here instead of in the ctor for two reasons: Doesn't this comment needs updating since we now have an observer? Also, the comment about the extension seems to not be entirely true in the media router case? https://codereview.chromium.org/1567103005/diff/80001/ash/system/cast/tray_ca... File ash/system/cast/tray_cast.h (right): https://codereview.chromium.org/1567103005/diff/80001/ash/system/cast/tray_ca... ash/system/cast/tray_cast.h:54: // Overridden from CastConfigDelegate::Observer. Maybe move this to after OnCastingSessionStartedOrStopped so all the overrides are together? https://codereview.chromium.org/1567103005/diff/80001/ash/test/tray_cast_test... File ash/test/tray_cast_test_api.cc (right): https://codereview.chromium.org/1567103005/diff/80001/ash/test/tray_cast_test... ash/test/tray_cast_test_api.cc:53: ash::Shell::GetInstance() Do we not need to do the checks for GetInstance and system_tray_delegate() here? https://codereview.chromium.org/1567103005/diff/80001/chrome/browser/ui/ash/c... File chrome/browser/ui/ash/cast_config_delegate_media_router.h (right): https://codereview.chromium.org/1567103005/diff/80001/chrome/browser/ui/ash/c... chrome/browser/ui/ash/cast_config_delegate_media_router.h:32: remove whitespace? Also I'd prefer Add/Remove Observer to be the last functions
On 2016/01/13 09:21:25, achuithb wrote: > WallpaperManagerBrowserTest.DisplayChange seems to be a real failure. I'm unable to reproduce the failure locally. I'm starting a new trybot run with the latest patch. https://codereview.chromium.org/1567103005/diff/80001/ash/cast_config_delegate.h File ash/cast_config_delegate.h (right): https://codereview.chromium.org/1567103005/diff/80001/ash/cast_config_delegat... ash/cast_config_delegate.h:12: #include "base/callback.h" On 2016/01/13 09:21:25, achuithb wrote: > Do you need this? Done. https://codereview.chromium.org/1567103005/diff/80001/ash/cast_config_delegat... ash/cast_config_delegate.h:77: virtual ~Observer() {} On 2016/01/13 09:21:25, achuithb wrote: > I think this can be protected? Needs to be public so the extension API can call OnDevicesUpdated. https://codereview.chromium.org/1567103005/diff/80001/ash/cast_config_delegat... ash/cast_config_delegate.h:81: }; On 2016/01/13 09:21:25, achuithb wrote: > DISALLOW_COPY_AND_ASSIGN perhaps? Doing so also requires defining a default constructor, which means that the class is mostly boilerplate. I just looked at a couple of other observers and they do not have DISALLOW_COPY_AND_ASSIGN. The other observers have a protected dtor, so I've adjusted that. https://codereview.chromium.org/1567103005/diff/80001/ash/cast_config_delegat... ash/cast_config_delegate.h:90: // Add or remove an observer. On 2016/01/13 09:21:25, achuithb wrote: > I feel like these administrative functions should be the last functions in this > class since they are the least interesting for readers of this file. Done. https://codereview.chromium.org/1567103005/diff/80001/ash/system/cast/tray_ca... File ash/system/cast/tray_cast.cc (right): https://codereview.chromium.org/1567103005/diff/80001/ash/system/cast/tray_ca... ash/system/cast/tray_cast.cc:45: if (!ash::Shell::GetInstance() || On 2016/01/13 09:21:25, achuithb wrote: > Could you please add a comment here explaining when this happens? (I believe > shutdown?) Done. https://codereview.chromium.org/1567103005/diff/80001/ash/system/cast/tray_ca... ash/system/cast/tray_cast.cc:562: Shell::GetInstance()->RemoveShellObserver(this); On 2016/01/13 09:21:25, achuithb wrote: > Do we need to check Shell::GetInstance here? Done. https://codereview.chromium.org/1567103005/diff/80001/ash/system/cast/tray_ca... ash/system/cast/tray_cast.cc:564: if (GetCastConfigDelegate()) On 2016/01/13 09:21:25, achuithb wrote: > Using a temporary is an option here so we don't call GetCastConfigDelegate > twice. > > Also, should we be checking added_observer_? Done. https://codereview.chromium.org/1567103005/diff/80001/ash/system/cast/tray_ca... ash/system/cast/tray_cast.cc:598: // We add the cast listener here instead of in the ctor for two reasons: On 2016/01/13 09:21:25, achuithb wrote: > Doesn't this comment needs updating since we now have an observer? Also, the > comment about the extension seems to not be entirely true in the media router > case? Done. https://codereview.chromium.org/1567103005/diff/80001/ash/system/cast/tray_ca... File ash/system/cast/tray_cast.h (right): https://codereview.chromium.org/1567103005/diff/80001/ash/system/cast/tray_ca... ash/system/cast/tray_cast.h:54: // Overridden from CastConfigDelegate::Observer. On 2016/01/13 09:21:25, achuithb wrote: > Maybe move this to after OnCastingSessionStartedOrStopped so all the overrides > are together? Done. https://codereview.chromium.org/1567103005/diff/80001/ash/test/tray_cast_test... File ash/test/tray_cast_test_api.cc (right): https://codereview.chromium.org/1567103005/diff/80001/ash/test/tray_cast_test... ash/test/tray_cast_test_api.cc:53: ash::Shell::GetInstance() On 2016/01/13 09:21:25, achuithb wrote: > Do we not need to do the checks for GetInstance and system_tray_delegate() here? Done, though I'm not sure we need them. This is test code, so I don't think they should ever be null. https://codereview.chromium.org/1567103005/diff/80001/chrome/browser/ui/ash/c... File chrome/browser/ui/ash/cast_config_delegate_media_router.h (right): https://codereview.chromium.org/1567103005/diff/80001/chrome/browser/ui/ash/c... chrome/browser/ui/ash/cast_config_delegate_media_router.h:32: On 2016/01/13 09:21:25, achuithb wrote: > remove whitespace? Also I'd prefer Add/Remove Observer to be the last functions Done.
https://codereview.chromium.org/1567103005/diff/80001/ash/cast_config_delegate.h File ash/cast_config_delegate.h (right): https://codereview.chromium.org/1567103005/diff/80001/ash/cast_config_delegat... ash/cast_config_delegate.h:77: virtual ~Observer() {} On 2016/01/13 19:43:26, jdufault wrote: > On 2016/01/13 09:21:25, achuithb wrote: > > I think this can be protected? > > Needs to be public so the extension API can call OnDevicesUpdated. OnDevicesUpdated should be public, but the destructor should be protected. https://codereview.chromium.org/1567103005/diff/80001/ash/cast_config_delegat... ash/cast_config_delegate.h:81: }; On 2016/01/13 19:43:26, jdufault wrote: > On 2016/01/13 09:21:25, achuithb wrote: > > DISALLOW_COPY_AND_ASSIGN perhaps? > > Doing so also requires defining a default constructor, which means that the > class is mostly boilerplate. I just looked at a couple of other observers and > they do not have DISALLOW_COPY_AND_ASSIGN. > > The other observers have a protected dtor, so I've adjusted that. It shouldn't. This is the preferred pattern: class ASH_EXPORT Observer { public: virtual void OnDevicesUpdated(const ReceiversAndActivities& devices) = 0; protected: virtual ~Observer() {} private: DISALLOW_ASSIGN(NetworkConfigurationObserver); };
https://codereview.chromium.org/1567103005/diff/80001/ash/cast_config_delegate.h File ash/cast_config_delegate.h (right): https://codereview.chromium.org/1567103005/diff/80001/ash/cast_config_delegat... ash/cast_config_delegate.h:77: virtual ~Observer() {} On 2016/01/13 20:02:18, stevenjb wrote: > On 2016/01/13 19:43:26, jdufault wrote: > > On 2016/01/13 09:21:25, achuithb wrote: > > > I think this can be protected? > > > > Needs to be public so the extension API can call OnDevicesUpdated. > > OnDevicesUpdated should be public, but the destructor should be protected. Yup, sorry I wasn't clear. https://codereview.chromium.org/1567103005/diff/80001/ash/cast_config_delegat... ash/cast_config_delegate.h:81: }; On 2016/01/13 20:02:18, stevenjb wrote: > On 2016/01/13 19:43:26, jdufault wrote: > > On 2016/01/13 09:21:25, achuithb wrote: > > > DISALLOW_COPY_AND_ASSIGN perhaps? > > > > Doing so also requires defining a default constructor, which means that the > > class is mostly boilerplate. I just looked at a couple of other observers and > > they do not have DISALLOW_COPY_AND_ASSIGN. > > > > The other observers have a protected dtor, so I've adjusted that. > > It shouldn't. This is the preferred pattern: > > class ASH_EXPORT Observer { > public: > virtual void OnDevicesUpdated(const ReceiversAndActivities& devices) = 0; > > protected: > virtual ~Observer() {} > > private: > DISALLOW_ASSIGN(NetworkConfigurationObserver); > }; +1. Thanks for clarifying, Steven
https://codereview.chromium.org/1567103005/diff/80001/ash/cast_config_delegate.h File ash/cast_config_delegate.h (right): https://codereview.chromium.org/1567103005/diff/80001/ash/cast_config_delegat... ash/cast_config_delegate.h:81: }; On 2016/01/13 21:08:33, achuithb wrote: > On 2016/01/13 20:02:18, stevenjb wrote: > > On 2016/01/13 19:43:26, jdufault wrote: > > > On 2016/01/13 09:21:25, achuithb wrote: > > > > DISALLOW_COPY_AND_ASSIGN perhaps? > > > > > > Doing so also requires defining a default constructor, which means that the > > > class is mostly boilerplate. I just looked at a couple of other observers > and > > > they do not have DISALLOW_COPY_AND_ASSIGN. > > > > > > The other observers have a protected dtor, so I've adjusted that. > > > > It shouldn't. This is the preferred pattern: > > > > class ASH_EXPORT Observer { > > public: > > virtual void OnDevicesUpdated(const ReceiversAndActivities& devices) = 0; > > > > protected: > > virtual ~Observer() {} > > > > private: > > DISALLOW_ASSIGN(NetworkConfigurationObserver); > > }; > > +1. Thanks for clarifying, Steven Done.
https://codereview.chromium.org/1567103005/diff/120001/ash/cast_config_delega... File ash/cast_config_delegate.h (right): https://codereview.chromium.org/1567103005/diff/120001/ash/cast_config_delega... ash/cast_config_delegate.h:83: DISALLOW_ASSIGN(Observer); Isn't DISALLOW_COPY_AND_ASSIGN better here?
lgtm regardless
The CQ bit was checked by achuith@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1567103005/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1567103005/120001
jdufault@chromium.org changed reviewers: + rockot@chromium.org
rockot@, PTAL. I need owner lgtm in chrome/browser/extensions/api/cast_devices_private/*, thanks! https://codereview.chromium.org/1567103005/diff/120001/ash/cast_config_delega... File ash/cast_config_delegate.h (right): https://codereview.chromium.org/1567103005/diff/120001/ash/cast_config_delega... ash/cast_config_delegate.h:83: DISALLOW_ASSIGN(Observer); On 2016/01/13 22:05:44, achuithb wrote: > Isn't DISALLOW_COPY_AND_ASSIGN better here? That causes a compilation error, with it you need to define a default ctor. tray_cast.cc:558:11: error: constructor for 'ash::TrayCast' explicitly initialize the base class 'CastConfigDelegate::Observer' which does not have a default constructor
lgtm https://codereview.chromium.org/1567103005/diff/120001/ash/cast_config_delega... File ash/cast_config_delegate.h (right): https://codereview.chromium.org/1567103005/diff/120001/ash/cast_config_delega... ash/cast_config_delegate.h:83: DISALLOW_ASSIGN(Observer); On 2016/01/13 22:12:14, jdufault wrote: > On 2016/01/13 22:05:44, achuithb wrote: > > Isn't DISALLOW_COPY_AND_ASSIGN better here? > > That causes a compilation error, with it you need to define a default ctor. > > tray_cast.cc:558:11: error: constructor for 'ash::TrayCast' > explicitly initialize the base class 'CastConfigDelegate::Observer' > which does not have a default constructor Acknowledged.
rs lgtm
The CQ bit was unchecked by jdufault@chromium.org
The CQ bit was checked by jdufault@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from stevenjb@chromium.org Link to the patchset: https://codereview.chromium.org/1567103005/#ps120001 (title: "Add DISALLOW_ASSIGN to Observer")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1567103005/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1567103005/120001
The CQ bit was unchecked by jdufault@chromium.org
The CQ bit was checked by jdufault@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1567103005/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1567103005/120001
The CQ bit was unchecked by jdufault@chromium.org
lgtm
The CQ bit was checked by jdufault@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rockot@chromium.org, achuith@chromium.org, stevenjb@chromium.org Link to the patchset: https://codereview.chromium.org/1567103005/#ps140001 (title: "Add TODO for proper fix")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1567103005/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1567103005/140001
Message was sent while issue was closed.
Description was changed from ========== Replace base::CallbackList with base::ObserverList in CastConfigDelegate base::CallbackList::Observer invoked a destructor inside of TrayCast. Due to wonky deinitialization order between the system tray delegate and the status area, this caused a crash. base::ObserverList deinitialization is more easily controlled than base::CallbackList. Switch to that so we don't deinitialize the observer if it's already been removed. Fixing the deinitialization order between the system tray delegate and the status area is the correct fix, but it will take significantly more time and needs to be thoroughly tested. BUG=574874 ========== to ========== Replace base::CallbackList with base::ObserverList in CastConfigDelegate base::CallbackList::Observer invoked a destructor inside of TrayCast. Due to wonky deinitialization order between the system tray delegate and the status area, this caused a crash. base::ObserverList deinitialization is more easily controlled than base::CallbackList. Switch to that so we don't deinitialize the observer if it's already been removed. Fixing the deinitialization order between the system tray delegate and the status area is the correct fix, but it will take significantly more time and needs to be thoroughly tested. BUG=574874 ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Replace base::CallbackList with base::ObserverList in CastConfigDelegate base::CallbackList::Observer invoked a destructor inside of TrayCast. Due to wonky deinitialization order between the system tray delegate and the status area, this caused a crash. base::ObserverList deinitialization is more easily controlled than base::CallbackList. Switch to that so we don't deinitialize the observer if it's already been removed. Fixing the deinitialization order between the system tray delegate and the status area is the correct fix, but it will take significantly more time and needs to be thoroughly tested. BUG=574874 ========== to ========== Replace base::CallbackList with base::ObserverList in CastConfigDelegate base::CallbackList::Observer invoked a destructor inside of TrayCast. Due to wonky deinitialization order between the system tray delegate and the status area, this caused a crash. base::ObserverList deinitialization is more easily controlled than base::CallbackList. Switch to that so we don't deinitialize the observer if it's already been removed. Fixing the deinitialization order between the system tray delegate and the status area is the correct fix, but it will take significantly more time and needs to be thoroughly tested. BUG=574874 Committed: https://crrev.com/6b717bb1e3bed46518e5410fbf80683114cdcb69 Cr-Commit-Position: refs/heads/master@{#369508} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/6b717bb1e3bed46518e5410fbf80683114cdcb69 Cr-Commit-Position: refs/heads/master@{#369508} |
