Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2889023002:
[ObjC ARC] Converts ios/chrome/browser/upgrade:upgrade to ARC. (Closed)
|
Created:
3 years, 7 months ago by liaoyuke Modified:
3 years, 6 months ago Reviewers:
sdefresne CC:
chromium-reviews, marq+watch_chromium.org, ios-reviews+chrome_chromium.org, noyau+watch_chromium.org, ios-reviews_chromium.org, pkl (ping after 24h if needed) Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[ObjC ARC] Converts ios/chrome/browser/upgrade:upgrade to ARC.
Automatically generated ARCMigrate commit
Notable issues:None
BUG=624363
TEST=None
Review-Url: https://codereview.chromium.org/2889023002
Cr-Commit-Position: refs/heads/master@{#479856}
Committed: https://chromium.googlesource.com/chromium/src/+/b0d8972ae9893eb63746a3157031829173c351eb
Patch Set 1 #
Total comments: 10
Patch Set 2 : Addressed comments. #
Total comments: 8
Patch Set 3 : Addressed comments #
Messages
Total messages: 26 (15 generated)
The CQ bit was checked by liaoyuke@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
liaoyuke@chromium.org changed reviewers: + sdefresne@chromium.org
Hi Sylvain, PTAL. Thank you very much!
https://codereview.chromium.org/2889023002/diff/1/ios/chrome/browser/upgrade/... File ios/chrome/browser/upgrade/upgrade_center.mm (right): https://codereview.chromium.org/2889023002/diff/1/ios/chrome/browser/upgrade/... ios/chrome/browser/upgrade/upgrade_center.mm:167: UpgradeCenter* dismiss_delegate_; This is an Objective-C pointer. Since the original code was not using scoped_nsobject<> it was weak but is now strong which creates a reference cycle. This needs to be marked either __weak or __unsafe_unretained. https://codereview.chromium.org/2889023002/diff/1/ios/chrome/browser/upgrade/... ios/chrome/browser/upgrade/upgrade_center.mm:216: NSMutableDictionary* upgradeInfoBarDelegates_; NSMutableDictionary<NSString*, DelegateHolder*>* upgradeInfoBarDelegates_; https://codereview.chromium.org/2889023002/diff/1/ios/chrome/browser/upgrade/... ios/chrome/browser/upgrade/upgrade_center.mm:219: std::set<id<UpgradeCenterClientProtocol>> clients_; The comments says that the Objective-C objects are not retained, and the code originally did not retain them. However, by compiling with ARC, the object are now retained. This is a semantic change that will probably break things. So, this should use __weak or __unsafe_unretained: std::set<__unsafe_unretained id<UpgradeCenterClientProtocol>> clients_; I recommend __unsafe_unretained because I'm not sure the operator <() relationship will be maintained by zero-ing weak pointers and if it is not, this will break std::set<> invariants and will lead to crashes. Or better, change this to a NSHashTable as this is a class that is designed to be similar to NSSet but can have weak pointers. NSHashTable<id<UpgradeCenterClientProtocol>>* clients_; Then initialise it with [NSHashTable weakObjectsHashTable]; https://codereview.chromium.org/2889023002/diff/1/ios/chrome/browser/upgrade/... ios/chrome/browser/upgrade/upgrade_center.mm:220: #ifndef NDEBUG followup: all "#ifndef NDEBUG" should instead be "#if DCHECK_IS_ON()" so that the DCHECK are enabled if compiled with dcheck_always_enabled=true. https://codereview.chromium.org/2889023002/diff/1/ios/chrome/browser/upgrade/... ios/chrome/browser/upgrade/upgrade_center.mm:221: bool inCallback_; bool -> BOOL
The CQ bit was checked by liaoyuke@chromium.org to run a CQ dry run
PTAL. Thank you very much! https://codereview.chromium.org/2889023002/diff/1/ios/chrome/browser/upgrade/... File ios/chrome/browser/upgrade/upgrade_center.mm (right): https://codereview.chromium.org/2889023002/diff/1/ios/chrome/browser/upgrade/... ios/chrome/browser/upgrade/upgrade_center.mm:167: UpgradeCenter* dismiss_delegate_; On 2017/05/31 08:16:13, sdefresne wrote: > This is an Objective-C pointer. Since the original code was not using > scoped_nsobject<> it was weak but is now strong which creates a reference cycle. > This needs to be marked either __weak or __unsafe_unretained. Done. https://codereview.chromium.org/2889023002/diff/1/ios/chrome/browser/upgrade/... ios/chrome/browser/upgrade/upgrade_center.mm:216: NSMutableDictionary* upgradeInfoBarDelegates_; On 2017/05/31 08:16:13, sdefresne wrote: > NSMutableDictionary<NSString*, DelegateHolder*>* upgradeInfoBarDelegates_; Done. https://codereview.chromium.org/2889023002/diff/1/ios/chrome/browser/upgrade/... ios/chrome/browser/upgrade/upgrade_center.mm:219: std::set<id<UpgradeCenterClientProtocol>> clients_; On 2017/05/31 08:16:13, sdefresne wrote: > The comments says that the Objective-C objects are not retained, and the code > originally did not retain them. However, by compiling with ARC, the object are > now retained. This is a semantic change that will probably break things. > > So, this should use __weak or __unsafe_unretained: > > std::set<__unsafe_unretained id<UpgradeCenterClientProtocol>> clients_; > > I recommend __unsafe_unretained because I'm not sure the operator <() > relationship will be maintained by zero-ing weak pointers and if it is not, this > will break std::set<> invariants and will lead to crashes. > > Or better, change this to a NSHashTable as this is a class that is designed to > be similar to NSSet but can have weak pointers. > > NSHashTable<id<UpgradeCenterClientProtocol>>* clients_; > > Then initialise it with [NSHashTable weakObjectsHashTable]; Thank you for explaining! Done. https://codereview.chromium.org/2889023002/diff/1/ios/chrome/browser/upgrade/... ios/chrome/browser/upgrade/upgrade_center.mm:220: #ifndef NDEBUG On 2017/05/31 08:16:13, sdefresne wrote: > followup: all "#ifndef NDEBUG" should instead be "#if DCHECK_IS_ON()" so that > the DCHECK are enabled if compiled with dcheck_always_enabled=true. Will Create a follow up CL to clean all of them up. https://codereview.chromium.org/2889023002/diff/1/ios/chrome/browser/upgrade/... ios/chrome/browser/upgrade/upgrade_center.mm:221: bool inCallback_; On 2017/05/31 08:16:13, sdefresne wrote: > bool -> BOOL Done.
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/06/13 18:17:31, liaoyuke wrote: > PTAL. > > Thank you very much! > > https://codereview.chromium.org/2889023002/diff/1/ios/chrome/browser/upgrade/... > File ios/chrome/browser/upgrade/upgrade_center.mm (right): > > https://codereview.chromium.org/2889023002/diff/1/ios/chrome/browser/upgrade/... > ios/chrome/browser/upgrade/upgrade_center.mm:167: UpgradeCenter* > dismiss_delegate_; > On 2017/05/31 08:16:13, sdefresne wrote: > > This is an Objective-C pointer. Since the original code was not using > > scoped_nsobject<> it was weak but is now strong which creates a reference > cycle. > > This needs to be marked either __weak or __unsafe_unretained. > > Done. > > https://codereview.chromium.org/2889023002/diff/1/ios/chrome/browser/upgrade/... > ios/chrome/browser/upgrade/upgrade_center.mm:216: NSMutableDictionary* > upgradeInfoBarDelegates_; > On 2017/05/31 08:16:13, sdefresne wrote: > > NSMutableDictionary<NSString*, DelegateHolder*>* upgradeInfoBarDelegates_; > > Done. > > https://codereview.chromium.org/2889023002/diff/1/ios/chrome/browser/upgrade/... > ios/chrome/browser/upgrade/upgrade_center.mm:219: > std::set<id<UpgradeCenterClientProtocol>> clients_; > On 2017/05/31 08:16:13, sdefresne wrote: > > The comments says that the Objective-C objects are not retained, and the code > > originally did not retain them. However, by compiling with ARC, the object are > > now retained. This is a semantic change that will probably break things. > > > > So, this should use __weak or __unsafe_unretained: > > > > std::set<__unsafe_unretained id<UpgradeCenterClientProtocol>> clients_; > > > > I recommend __unsafe_unretained because I'm not sure the operator <() > > relationship will be maintained by zero-ing weak pointers and if it is not, > this > > will break std::set<> invariants and will lead to crashes. > > > > Or better, change this to a NSHashTable as this is a class that is designed to > > be similar to NSSet but can have weak pointers. > > > > NSHashTable<id<UpgradeCenterClientProtocol>>* clients_; > > > > Then initialise it with [NSHashTable weakObjectsHashTable]; > > Thank you for explaining! > > Done. > > https://codereview.chromium.org/2889023002/diff/1/ios/chrome/browser/upgrade/... > ios/chrome/browser/upgrade/upgrade_center.mm:220: #ifndef NDEBUG > On 2017/05/31 08:16:13, sdefresne wrote: > > followup: all "#ifndef NDEBUG" should instead be "#if DCHECK_IS_ON()" so that > > the DCHECK are enabled if compiled with dcheck_always_enabled=true. > > Will Create a follow up CL to clean all of them up. > > https://codereview.chromium.org/2889023002/diff/1/ios/chrome/browser/upgrade/... > ios/chrome/browser/upgrade/upgrade_center.mm:221: bool inCallback_; > On 2017/05/31 08:16:13, sdefresne wrote: > > bool -> BOOL > > Done. Did you forget to upload the patchset?
On 2017/06/15 11:28:41, sdefresne wrote: > On 2017/06/13 18:17:31, liaoyuke wrote: > > PTAL. > > > > Thank you very much! > > > > > https://codereview.chromium.org/2889023002/diff/1/ios/chrome/browser/upgrade/... > > File ios/chrome/browser/upgrade/upgrade_center.mm (right): > > > > > https://codereview.chromium.org/2889023002/diff/1/ios/chrome/browser/upgrade/... > > ios/chrome/browser/upgrade/upgrade_center.mm:167: UpgradeCenter* > > dismiss_delegate_; > > On 2017/05/31 08:16:13, sdefresne wrote: > > > This is an Objective-C pointer. Since the original code was not using > > > scoped_nsobject<> it was weak but is now strong which creates a reference > > cycle. > > > This needs to be marked either __weak or __unsafe_unretained. > > > > Done. > > > > > https://codereview.chromium.org/2889023002/diff/1/ios/chrome/browser/upgrade/... > > ios/chrome/browser/upgrade/upgrade_center.mm:216: NSMutableDictionary* > > upgradeInfoBarDelegates_; > > On 2017/05/31 08:16:13, sdefresne wrote: > > > NSMutableDictionary<NSString*, DelegateHolder*>* upgradeInfoBarDelegates_; > > > > Done. > > > > > https://codereview.chromium.org/2889023002/diff/1/ios/chrome/browser/upgrade/... > > ios/chrome/browser/upgrade/upgrade_center.mm:219: > > std::set<id<UpgradeCenterClientProtocol>> clients_; > > On 2017/05/31 08:16:13, sdefresne wrote: > > > The comments says that the Objective-C objects are not retained, and the > code > > > originally did not retain them. However, by compiling with ARC, the object > are > > > now retained. This is a semantic change that will probably break things. > > > > > > So, this should use __weak or __unsafe_unretained: > > > > > > std::set<__unsafe_unretained id<UpgradeCenterClientProtocol>> clients_; > > > > > > I recommend __unsafe_unretained because I'm not sure the operator <() > > > relationship will be maintained by zero-ing weak pointers and if it is not, > > this > > > will break std::set<> invariants and will lead to crashes. > > > > > > Or better, change this to a NSHashTable as this is a class that is designed > to > > > be similar to NSSet but can have weak pointers. > > > > > > NSHashTable<id<UpgradeCenterClientProtocol>>* clients_; > > > > > > Then initialise it with [NSHashTable weakObjectsHashTable]; > > > > Thank you for explaining! > > > > Done. > > > > > https://codereview.chromium.org/2889023002/diff/1/ios/chrome/browser/upgrade/... > > ios/chrome/browser/upgrade/upgrade_center.mm:220: #ifndef NDEBUG > > On 2017/05/31 08:16:13, sdefresne wrote: > > > followup: all "#ifndef NDEBUG" should instead be "#if DCHECK_IS_ON()" so > that > > > the DCHECK are enabled if compiled with dcheck_always_enabled=true. > > > > Will Create a follow up CL to clean all of them up. > > > > > https://codereview.chromium.org/2889023002/diff/1/ios/chrome/browser/upgrade/... > > ios/chrome/browser/upgrade/upgrade_center.mm:221: bool inCallback_; > > On 2017/05/31 08:16:13, sdefresne wrote: > > > bool -> BOOL > > > > Done. > > Did you forget to upload the patchset? Sorry for that, just uploaded, PTAL. Thank you very much!
lgtm https://codereview.chromium.org/2889023002/diff/20001/ios/chrome/browser/upgr... File ios/chrome/browser/upgrade/upgrade_center.mm (right): https://codereview.chromium.org/2889023002/diff/20001/ios/chrome/browser/upgr... ios/chrome/browser/upgrade/upgrade_center.mm:168: NSString* tab_id_; nit: __strong NSString* tab_id_; https://codereview.chromium.org/2889023002/diff/20001/ios/chrome/browser/upgr... ios/chrome/browser/upgrade/upgrade_center.mm:216: NSMutableDictionary<NSString*, DelegateHolder*>* upgradeInfoBarDelegates_; nit: __strong NSMutableDictionary<NSString*, DelegateHolder*>* upgradeInfoBarDelegates_; https://codereview.chromium.org/2889023002/diff/20001/ios/chrome/browser/upgr... ios/chrome/browser/upgrade/upgrade_center.mm:219: NSHashTable<id<UpgradeCenterClientProtocol>>* clients_; nit: __strong NSHashTable<id<UpgradeCenterClientProtocol>>* clients_; https://codereview.chromium.org/2889023002/diff/20001/ios/chrome/browser/upgr... ios/chrome/browser/upgrade/upgrade_center.mm:381: NSEnumerator* enumerator = [clients_ objectEnumerator]; NSHashTable supports NSFastEnumeration, so you can use a for( .. in ..) loop: for (id<UpgradeCenterClientProtocol> upgradeClient in clients_) [upgradeClient showUpgrade:self];
Thanks for reviewing! https://codereview.chromium.org/2889023002/diff/20001/ios/chrome/browser/upgr... File ios/chrome/browser/upgrade/upgrade_center.mm (right): https://codereview.chromium.org/2889023002/diff/20001/ios/chrome/browser/upgr... ios/chrome/browser/upgrade/upgrade_center.mm:168: NSString* tab_id_; On 2017/06/15 16:34:30, sdefresne wrote: > nit: __strong NSString* tab_id_; Done. https://codereview.chromium.org/2889023002/diff/20001/ios/chrome/browser/upgr... ios/chrome/browser/upgrade/upgrade_center.mm:216: NSMutableDictionary<NSString*, DelegateHolder*>* upgradeInfoBarDelegates_; On 2017/06/15 16:34:30, sdefresne wrote: > nit: __strong NSMutableDictionary<NSString*, DelegateHolder*>* > upgradeInfoBarDelegates_; Done. https://codereview.chromium.org/2889023002/diff/20001/ios/chrome/browser/upgr... ios/chrome/browser/upgrade/upgrade_center.mm:219: NSHashTable<id<UpgradeCenterClientProtocol>>* clients_; On 2017/06/15 16:34:30, sdefresne wrote: > nit: __strong NSHashTable<id<UpgradeCenterClientProtocol>>* clients_; Done. https://codereview.chromium.org/2889023002/diff/20001/ios/chrome/browser/upgr... ios/chrome/browser/upgrade/upgrade_center.mm:381: NSEnumerator* enumerator = [clients_ objectEnumerator]; On 2017/06/15 16:34:30, sdefresne wrote: > NSHashTable supports NSFastEnumeration, so you can use a for( .. in ..) loop: > > for (id<UpgradeCenterClientProtocol> upgradeClient in clients_) > [upgradeClient showUpgrade:self]; Done.
The CQ bit was checked by liaoyuke@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sdefresne@chromium.org Link to the patchset: https://codereview.chromium.org/2889023002/#ps40001 (title: "Addressed comments")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by liaoyuke@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1497562421899500,
"parent_rev": "4657c6480527b4b1ea6cb21d4d666ea92652f04d", "commit_rev":
"b0d8972ae9893eb63746a3157031829173c351eb"}
Message was sent while issue was closed.
Description was changed from ========== [ObjC ARC] Converts ios/chrome/browser/upgrade:upgrade to ARC. Automatically generated ARCMigrate commit Notable issues:None BUG=624363 TEST=None ========== to ========== [ObjC ARC] Converts ios/chrome/browser/upgrade:upgrade to ARC. Automatically generated ARCMigrate commit Notable issues:None BUG=624363 TEST=None Review-Url: https://codereview.chromium.org/2889023002 Cr-Commit-Position: refs/heads/master@{#479856} Committed: https://chromium.googlesource.com/chromium/src/+/b0d8972ae9893eb63746a3157031... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/b0d8972ae9893eb63746a3157031... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
