Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2359633003:
Makes ScreenDimmer deal with the shell going away (Closed)
|
Created:
4 years, 3 months ago by sky Modified:
4 years, 3 months ago Reviewers:
James Cook CC:
chromium-reviews, kalyank, sadrul Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMakes ScreenDimmer deal with the shell going away
Chrome may end up using ScreenDimmer in such a way that the ScreenDimmer is destroyed after the shell. This change makes ~ScreenDimmer deal with the shell having been destroyed so that it doesn't crash.
BUG=648723
TEST=covered by test
R=jamescook@chromium.org
Committed: https://crrev.com/2ce4166d48b553822dda8ab5cafa0042c36729c5
Cr-Commit-Position: refs/heads/master@{#419909}
Patch Set 1 #Patch Set 2 : comment #
Total comments: 4
Patch Set 3 : comment #
Messages
Total messages: 22 (13 generated)
Description was changed from ========== shell# Enter a description of the change. Makes ScreenDimmer deal with the shell going away Chrome may end up using ScreenDimmer in such a way that it is destroyed after the shell. This change makes ~ScreenDimmer deal with the shell having been destroyed so that it doesn't crash. BUG=648723 TEST=covered by test R=oshima@chromium.org ========== to ========== Makes ScreenDimmer deal with the shell going away Chrome may end up using ScreenDimmer in such a way that it is destroyed after the shell. This change makes ~ScreenDimmer deal with the shell having been destroyed so that it doesn't crash. BUG=648723 TEST=covered by test R=jamescook@chromium.org ==========
sky@chromium.org changed reviewers: + jamescook@chromium.org - oshima@chromium.org
The CQ bit was checked by sky@chromium.org to run a CQ dry run
Description was changed from ========== Makes ScreenDimmer deal with the shell going away Chrome may end up using ScreenDimmer in such a way that it is destroyed after the shell. This change makes ~ScreenDimmer deal with the shell having been destroyed so that it doesn't crash. BUG=648723 TEST=covered by test R=jamescook@chromium.org ========== to ========== Makes ScreenDimmer deal with the shell going away Chrome may end up using ScreenDimmer in such a way that the ScreenDimmer is destroyed after the shell. This change makes ~ScreenDimmer deal with the shell having been destroyed so that it doesn't crash. BUG=648723 TEST=covered by test R=jamescook@chromium.org ==========
oshima->jamescook as Oshima is OOO.
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/2359633003/diff/20001/ash/common/wm/screen_di... File ash/common/wm/screen_dimmer.cc (right): https://codereview.chromium.org/2359633003/diff/20001/ash/common/wm/screen_di... ash/common/wm/screen_dimmer.cc:36: if (WmShell::HasInstance()) Does this mean we'll never clean up this observer in classic ash? https://codereview.chromium.org/2359633003/diff/20001/ash/wm/screen_dimmer_un... File ash/wm/screen_dimmer_unittest.cc (right): https://codereview.chromium.org/2359633003/diff/20001/ash/wm/screen_dimmer_un... ash/wm/screen_dimmer_unittest.cc:132: } I would add a comment above this line pointing out the deletion.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2359633003/diff/20001/ash/common/wm/screen_di... File ash/common/wm/screen_dimmer.cc (right): https://codereview.chromium.org/2359633003/diff/20001/ash/common/wm/screen_di... ash/common/wm/screen_dimmer.cc:36: if (WmShell::HasInstance()) On 2016/09/20 23:26:18, James Cook wrote: > Does this mean we'll never clean up this observer in classic ash? That is correct. The ObserverList does not enforce it is empty on destruction.
https://codereview.chromium.org/2359633003/diff/20001/ash/wm/screen_dimmer_un... File ash/wm/screen_dimmer_unittest.cc (right): https://codereview.chromium.org/2359633003/diff/20001/ash/wm/screen_dimmer_un... ash/wm/screen_dimmer_unittest.cc:132: } On 2016/09/20 23:26:18, James Cook wrote: > I would add a comment above this line pointing out the deletion. Done.
The CQ bit was checked by sky@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm
The CQ bit was unchecked by sky@chromium.org
The CQ bit was checked by sky@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 ========== Makes ScreenDimmer deal with the shell going away Chrome may end up using ScreenDimmer in such a way that the ScreenDimmer is destroyed after the shell. This change makes ~ScreenDimmer deal with the shell having been destroyed so that it doesn't crash. BUG=648723 TEST=covered by test R=jamescook@chromium.org ========== to ========== Makes ScreenDimmer deal with the shell going away Chrome may end up using ScreenDimmer in such a way that the ScreenDimmer is destroyed after the shell. This change makes ~ScreenDimmer deal with the shell having been destroyed so that it doesn't crash. BUG=648723 TEST=covered by test R=jamescook@chromium.org ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Makes ScreenDimmer deal with the shell going away Chrome may end up using ScreenDimmer in such a way that the ScreenDimmer is destroyed after the shell. This change makes ~ScreenDimmer deal with the shell having been destroyed so that it doesn't crash. BUG=648723 TEST=covered by test R=jamescook@chromium.org ========== to ========== Makes ScreenDimmer deal with the shell going away Chrome may end up using ScreenDimmer in such a way that the ScreenDimmer is destroyed after the shell. This change makes ~ScreenDimmer deal with the shell having been destroyed so that it doesn't crash. BUG=648723 TEST=covered by test R=jamescook@chromium.org Committed: https://crrev.com/2ce4166d48b553822dda8ab5cafa0042c36729c5 Cr-Commit-Position: refs/heads/master@{#419909} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/2ce4166d48b553822dda8ab5cafa0042c36729c5 Cr-Commit-Position: refs/heads/master@{#419909} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
