Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2828283002:
[ObjC ARC] Converts ios/chrome/browser/ui/print:print to ARC. (Closed)
|
Created:
3 years, 8 months ago by gambard Modified:
3 years, 8 months ago CC:
chromium-reviews, ios-reviews+chrome_chromium.org, ios-reviews_chromium.org, pkl (ping after 24h if needed), noyau+watch_chromium.org, marq+watch_chromium.org, sdefresne+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[ObjC ARC] Converts ios/chrome/browser/ui/print:print to ARC.
Automatically generated ARCMigrate commit
Notable issues:None
BUG=624363
TEST=None
Review-Url: https://codereview.chromium.org/2828283002
Cr-Commit-Position: refs/heads/master@{#466005}
Committed: https://chromium.googlesource.com/chromium/src/+/975b115ff82c6d407cd346afb4d4838f5d380eab
Patch Set 1 #
Total comments: 3
Patch Set 2 : Replace unsafe_unretained by weak #
Messages
Total messages: 20 (12 generated)
The CQ bit was checked by gambard@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...
gambard@chromium.org changed reviewers: + sdefresne@chromium.org
PTAL.
lgtm https://codereview.chromium.org/2828283002/diff/1/ios/chrome/browser/ui/print... File ios/chrome/browser/ui/print/print_controller.mm (right): https://codereview.chromium.org/2828283002/diff/1/ios/chrome/browser/ui/print... ios/chrome/browser/ui/print/print_controller.mm:103: __unsafe_unretained PrintController* owner_; Maybe change all __unsafe_unretained to __weak for more safety.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
stkhapugin@chromium.org changed reviewers: + stkhapugin@chromium.org
https://codereview.chromium.org/2828283002/diff/1/ios/chrome/browser/ui/print... File ios/chrome/browser/ui/print/print_controller.mm (right): https://codereview.chromium.org/2828283002/diff/1/ios/chrome/browser/ui/print... ios/chrome/browser/ui/print/print_controller.mm:103: __unsafe_unretained PrintController* owner_; On 2017/04/20 13:16:51, sdefresne wrote: > Maybe change all __unsafe_unretained to __weak for more safety. +1, __unsafe_unretained was probably used here to not leave an unmarked ivar, but __weak wasn't available. In general, __unsafe_unretained is only better for performance-critical code, which means we probably don't need it at all in any objective-c code in chrome ios, as most of the performance-critical code is in C++
Thanks! https://codereview.chromium.org/2828283002/diff/1/ios/chrome/browser/ui/print... File ios/chrome/browser/ui/print/print_controller.mm (right): https://codereview.chromium.org/2828283002/diff/1/ios/chrome/browser/ui/print... ios/chrome/browser/ui/print/print_controller.mm:103: __unsafe_unretained PrintController* owner_; On 2017/04/20 13:35:10, stkhapugin wrote: > On 2017/04/20 13:16:51, sdefresne wrote: > > Maybe change all __unsafe_unretained to __weak for more safety. > > +1, __unsafe_unretained was probably used here to not leave an unmarked ivar, > but __weak wasn't available. In general, __unsafe_unretained is only better for > performance-critical code, which means we probably don't need it at all in any > objective-c code in chrome ios, as most of the performance-critical code is in > C++ Done.
The CQ bit was checked by gambard@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/2828283002/#ps20001 (title: "Replace unsafe_unretained by weak")
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: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by gambard@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": 20001, "attempt_start_ts": 1492696651962710,
"parent_rev": "bd0add4325a532ca0a0d9c23eaa55f0084317b6f", "commit_rev":
"975b115ff82c6d407cd346afb4d4838f5d380eab"}
Message was sent while issue was closed.
Description was changed from ========== [ObjC ARC] Converts ios/chrome/browser/ui/print:print to ARC. Automatically generated ARCMigrate commit Notable issues:None BUG=624363 TEST=None ========== to ========== [ObjC ARC] Converts ios/chrome/browser/ui/print:print to ARC. Automatically generated ARCMigrate commit Notable issues:None BUG=624363 TEST=None Review-Url: https://codereview.chromium.org/2828283002 Cr-Commit-Position: refs/heads/master@{#466005} Committed: https://chromium.googlesource.com/chromium/src/+/975b115ff82c6d407cd346afb4d4... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/975b115ff82c6d407cd346afb4d4... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
