Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2887413002:
[ObjC ARC] Converts ios/chrome/app:main to ARC. (Closed)
|
Created:
3 years, 7 months ago by liaoyuke Modified:
3 years, 5 months ago 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/app:main to ARC.
Automatically generated ARCMigrate commit
Notable issues:None
BUG=624363
TEST=None
Patch Set 1 #
Total comments: 3
Messages
Total messages: 20 (9 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: + rohitrao@chromium.org
Hey Rohit, PTAL. Thank you very much!
rohitrao@chromium.org changed reviewers: + sdefresne@chromium.org, stkhapugin@chromium.org
https://codereview.chromium.org/2887413002/diff/1/ios/chrome/app/chrome_exe_m... File ios/chrome/app/chrome_exe_main.mm (left): https://codereview.chromium.org/2887413002/diff/1/ios/chrome/app/chrome_exe_m... ios/chrome/app/chrome_exe_main.mm:53: // calls to live forever. We added this autorelease pool for a specific reason, so I don't think we should remove it. Does it work to add two @autoreleasepool blocks to replace the old pools? +sdefresne and +stk to weigh in on this as well, since it's nontrivial.
https://codereview.chromium.org/2887413002/diff/1/ios/chrome/app/chrome_exe_m... File ios/chrome/app/chrome_exe_main.mm (left): https://codereview.chromium.org/2887413002/diff/1/ios/chrome/app/chrome_exe_m... ios/chrome/app/chrome_exe_main.mm:53: // calls to live forever. On 2017/05/18 20:50:34, rohitrao (ping after 24h) wrote: > We added this autorelease pool for a specific reason, so I don't think we should > remove it. Does it work to add two @autoreleasepool blocks to replace the old > pools? > > +sdefresne and +stk to weigh in on this as well, since it's nontrivial. This should be replaced with @autoreleasepool in a separate CL, since @autoreleasepool macro should build fine without ARC. Please, submit a separate parent CL that puts @autoreleasepool {} around lines 31-54 and 55-64. If this CL lands cleanly, this CL will be trivial.
ping
Hey Stepan, PTAL. Thank you very much! https://codereview.chromium.org/2887413002/diff/1/ios/chrome/app/chrome_exe_m... File ios/chrome/app/chrome_exe_main.mm (left): https://codereview.chromium.org/2887413002/diff/1/ios/chrome/app/chrome_exe_m... ios/chrome/app/chrome_exe_main.mm:53: // calls to live forever. On 2017/05/30 14:04:06, stkhapugin wrote: > On 2017/05/18 20:50:34, rohitrao (ping after 24h) wrote: > > We added this autorelease pool for a specific reason, so I don't think we > should > > remove it. Does it work to add two @autoreleasepool blocks to replace the old > > pools? > > > > +sdefresne and +stk to weigh in on this as well, since it's nontrivial. > > This should be replaced with @autoreleasepool in a separate CL, since > @autoreleasepool macro should build fine without ARC. Please, submit a separate > parent CL that puts @autoreleasepool {} around lines 31-54 and 55-64. > > If this CL lands cleanly, this CL will be trivial. Have uploaded this parent CL: https://chromium-review.googlesource.com/c/534874/
lgtm Please don't land this until the parent has been rolled downstream and the bots have stayed green.
The CQ bit was checked by stkhapugin@chromium.org
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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Sylvain/Rohit, PTAL for OWNERS
Please rebase. There should not be any changes in chrome_exe_main.mm except for the addition of the ARC guard.
On 2017/06/26 10:32:49, sdefresne wrote: > Please rebase. There should not be any changes in chrome_exe_main.mm except for > the addition of the ARC guard. Sorry that I forgot to update this bug, but this was already done by Stepan in a separate CL: https://codereview.chromium.org/2959473002/, so I'm closing this one.
Description was changed from ========== [ObjC ARC] Converts ios/chrome/app:main to ARC. Automatically generated ARCMigrate commit Notable issues:None BUG=624363 TEST=None ========== to ========== [ObjC ARC] Converts ios/chrome/app:main to ARC. Automatically generated ARCMigrate commit Notable issues:None BUG=624363 TEST=None ========== |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
