Chromium Code Reviews
|
|
Chromium Code Reviews|
Created:
3 years, 3 months ago by andersc Modified:
3 years, 3 months ago Reviewers:
kthelgason, Chuck CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
DescriptionFix retain cycles in RTCUIApplicationStatusObserver.
These retain cycles are theoretical since the singleton is supposed to
live for the lifetime of the application.
These measures were removed earlier when the object was turned into
a singleton in a previous CL, see
https://chromium-review.googlesource.com/c/external/webrtc/+/527442/3..4/webrtc/sdk/objc/Framework/Classes/Common/RTCUIApplicationStatusObserver.m
The weak self handling and unused dealloc method is mostly noise and
can make a casual reader think that the object will have a limited
life cycle, i.e. the code may initially look like something it is not,
which could possibly be less readable. On the other hand, for people
looking out for potential retain cycles, the code may be distracting
since it looks like it may be leaking.
BUG=b/65558647
Review-Url: https://codereview.webrtc.org/3013023002
Cr-Commit-Position: refs/heads/master@{#19811}
Committed: https://chromium.googlesource.com/external/webrtc/+/4090f380e58beb4e192691ca50643eef3aadd25d
Patch Set 1 #
Total comments: 2
Patch Set 2 : Reference weak self in initialization block. #
Created: 3 years, 3 months ago
Messages
Total messages: 21 (12 generated)
Description was changed from ========== Fix retain cycles in RTCUIApplicationStatusObserver. These retain cycles are theoretical since the singleton is supposed to live for the lifetime of the application. These measures were removed earlier when the object was turned into a singleton in a previous CL, see https://chromium-review.googlesource.com/c/external/webrtc/+/527442/3..4/webr... The weak self handling and unused dealloc method is mostly noise and can make a casual reader think that the object will have a limited life cycle, i.e. the code may initially look like something it is not, which could possibly be less readable. On the other hand, for people looking out for potential retain cycles, the code may be distracting since it looks like it may be leaking. BUG=b/65558647 ========== to ========== Fix retain cycles in RTCUIApplicationStatusObserver. These retain cycles are theoretical since the singleton is supposed to live for the lifetime of the application. These measures were removed earlier when the object was turned into a singleton in a previous CL, see https://chromium-review.googlesource.com/c/external/webrtc/+/527442/3..4/webr... The weak self handling and unused dealloc method is mostly noise and can make a casual reader think that the object will have a limited life cycle, i.e. the code may initially look like something it is not, which could possibly be less readable. On the other hand, for people looking out for potential retain cycles, the code may be distracting since it looks like it may be leaking. BUG=b/65558647 ==========
andersc@webrtc.org changed reviewers: + haysc@webrtc.org
The CQ bit was checked by andersc@webrtc.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.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.webrtc.org/3013023002/diff/1/webrtc/sdk/objc/Framework/Cla... File webrtc/sdk/objc/Framework/Classes/Common/RTCUIApplicationStatusObserver.m (right): https://codereview.webrtc.org/3013023002/diff/1/webrtc/sdk/objc/Framework/Cla... webrtc/sdk/objc/Framework/Classes/Common/RTCUIApplicationStatusObserver.m:68: _state = [UIApplication sharedApplication].applicationState; This and _initialized both have an implicit reference to self as well.
https://codereview.webrtc.org/3013023002/diff/1/webrtc/sdk/objc/Framework/Cla... File webrtc/sdk/objc/Framework/Classes/Common/RTCUIApplicationStatusObserver.m (right): https://codereview.webrtc.org/3013023002/diff/1/webrtc/sdk/objc/Framework/Cla... webrtc/sdk/objc/Framework/Classes/Common/RTCUIApplicationStatusObserver.m:68: _state = [UIApplication sharedApplication].applicationState; On 2017/09/12 13:25:51, Chuck wrote: > This and _initialized both have an implicit reference to self as well. Done.
lgtm
The CQ bit was checked by andersc@webrtc.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/21523)
andersc@webrtc.org changed reviewers: + kthelgason@webrtc.org
kthelgason - please take a look (added to get owners coverage)
On 2017/09/13 08:20:08, andersc wrote: > kthelgason - please take a look (added to get owners coverage) I don't think this change fixes any real problem, and it adds noise. However I don't feel strongly enough about it to waste anyone's time. lgtm.
The CQ bit was checked by andersc@webrtc.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1505292492314340,
"parent_rev": "47791cf0f8400984c0e90229c756e404ba7eee4f", "commit_rev":
"4090f380e58beb4e192691ca50643eef3aadd25d"}
Message was sent while issue was closed.
Description was changed from ========== Fix retain cycles in RTCUIApplicationStatusObserver. These retain cycles are theoretical since the singleton is supposed to live for the lifetime of the application. These measures were removed earlier when the object was turned into a singleton in a previous CL, see https://chromium-review.googlesource.com/c/external/webrtc/+/527442/3..4/webr... The weak self handling and unused dealloc method is mostly noise and can make a casual reader think that the object will have a limited life cycle, i.e. the code may initially look like something it is not, which could possibly be less readable. On the other hand, for people looking out for potential retain cycles, the code may be distracting since it looks like it may be leaking. BUG=b/65558647 ========== to ========== Fix retain cycles in RTCUIApplicationStatusObserver. These retain cycles are theoretical since the singleton is supposed to live for the lifetime of the application. These measures were removed earlier when the object was turned into a singleton in a previous CL, see https://chromium-review.googlesource.com/c/external/webrtc/+/527442/3..4/webr... The weak self handling and unused dealloc method is mostly noise and can make a casual reader think that the object will have a limited life cycle, i.e. the code may initially look like something it is not, which could possibly be less readable. On the other hand, for people looking out for potential retain cycles, the code may be distracting since it looks like it may be leaking. BUG=b/65558647 Review-Url: https://codereview.webrtc.org/3013023002 Cr-Commit-Position: refs/heads/master@{#19811} Committed: https://chromium.googlesource.com/external/webrtc/+/4090f380e58beb4e192691ca5... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/external/webrtc/+/4090f380e58beb4e192691ca5... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
