|
|
Description[ObjC ARC] Converts components/reading_list/ios:ios to ARC.
Automatically generated ARCMigrate commit
Notable issues:None
BUG=624363
TEST=None
Review-Url: https://codereview.chromium.org/2888163004
Cr-Commit-Position: refs/heads/master@{#476763}
Committed: https://chromium.googlesource.com/chromium/src/+/f3fd628d5db7f01f9f11bf1ba00130ee03a2f2f8
Patch Set 1 #
Total comments: 5
Patch Set 2 : Adding todo for weak pointer reactoring. #
Total comments: 1
Patch Set 3 : Reworded todo. #
Messages
Total messages: 26 (14 generated)
The CQ bit was checked by lindsayw@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: This issue passed the CQ dry run.
lindsayw@chromium.org changed reviewers: + stkhapugin@chromium.org
Thanks for the review!
lindsayw@chromium.org changed reviewers: + noyau@chromium.org
noyau@chromium.org: Please review as owner
https://codereview.chromium.org/2888163004/diff/1/components/reading_list/ios... File components/reading_list/ios/reading_list_model_bridge_observer.mm (right): https://codereview.chromium.org/2888163004/diff/1/components/reading_list/ios... components/reading_list/ios/reading_list_model_bridge_observer.mm:17: : observer_(observer), model_(model) { This now has the side effect of retaining the model, as the header only has a comment specifying weak: With ARC this is now strong by default. In the header model_ needs to be marked as unsafe_unretained at a minimum.
https://codereview.chromium.org/2888163004/diff/1/components/reading_list/ios... File components/reading_list/ios/reading_list_model_bridge_observer.mm (right): https://codereview.chromium.org/2888163004/diff/1/components/reading_list/ios... components/reading_list/ios/reading_list_model_bridge_observer.mm:17: : observer_(observer), model_(model) { On 2017/05/29 11:57:59, noyau (Ping after 24h) wrote: > This now has the side effect of retaining the model, as the header only has a > comment specifying weak: With ARC this is now strong by default. > > In the header model_ needs to be marked as unsafe_unretained at a minimum. In reading_list_model_bridge_observer.h, replace: ReadingListModel* model_; // weak with: __unsafe_unretained ReadingListModel* model_; Ideally, try to replace it with: __weak ReadingListModel* model_; and see if it builds. If it does, also replace: __unsafe_unretained id<ReadingListModelBridgeObserver> observer_; with: __weak id<ReadingListModelBridgeObserver> observer_;
https://codereview.chromium.org/2888163004/diff/1/components/reading_list/ios... File components/reading_list/ios/reading_list_model_bridge_observer.mm (right): https://codereview.chromium.org/2888163004/diff/1/components/reading_list/ios... components/reading_list/ios/reading_list_model_bridge_observer.mm:17: : observer_(observer), model_(model) { On 2017/05/30 13:57:51, stkhapugin wrote: > On 2017/05/29 11:57:59, noyau (Ping after 24h) wrote: > > This now has the side effect of retaining the model, as the header only has a > > comment specifying weak: With ARC this is now strong by default. > > > > In the header model_ needs to be marked as unsafe_unretained at a minimum. > > In reading_list_model_bridge_observer.h, replace: > ReadingListModel* model_; // weak > with: > __unsafe_unretained ReadingListModel* model_; > > Ideally, try to replace it with: > __weak ReadingListModel* model_; > and see if it builds. If it does, also replace: > __unsafe_unretained id<ReadingListModelBridgeObserver> observer_; > with: > __weak id<ReadingListModelBridgeObserver> observer_; It won't build with __weak ReadingListModel* model_; so I put __unsafe_unretained ReadingListModel* model_; and that also doesn't build successfully. The error for the latter is: ../../components/reading_list/ios/reading_list_model_bridge_observer.h:78:3: error: 'objc_ownership' only applies to Objective-C object or block pointer types; type here is 'ReadingListModel *' [-Werror,-Wignored-attributes]
https://codereview.chromium.org/2888163004/diff/1/components/reading_list/ios... File components/reading_list/ios/reading_list_model_bridge_observer.mm (right): https://codereview.chromium.org/2888163004/diff/1/components/reading_list/ios... components/reading_list/ios/reading_list_model_bridge_observer.mm:17: : observer_(observer), model_(model) { On 2017/06/01 16:49:39, lindsayw wrote: > On 2017/05/30 13:57:51, stkhapugin wrote: > > On 2017/05/29 11:57:59, noyau (Ping after 24h) wrote: > > > This now has the side effect of retaining the model, as the header only has > a > > > comment specifying weak: With ARC this is now strong by default. > > > > > > In the header model_ needs to be marked as unsafe_unretained at a minimum. > > > > In reading_list_model_bridge_observer.h, replace: > > ReadingListModel* model_; // weak > > with: > > __unsafe_unretained ReadingListModel* model_; > > > > Ideally, try to replace it with: > > __weak ReadingListModel* model_; > > and see if it builds. If it does, also replace: > > __unsafe_unretained id<ReadingListModelBridgeObserver> observer_; > > with: > > __weak id<ReadingListModelBridgeObserver> observer_; > > It won't build with __weak ReadingListModel* model_; so I put > __unsafe_unretained ReadingListModel* model_; and that also doesn't build > successfully. The error for the latter is: > ../../components/reading_list/ios/reading_list_model_bridge_observer.h:78:3: > error: 'objc_ownership' only applies to Objective-C object or block pointer > types; type here is 'ReadingListModel *' [-Werror,-Wignored-attributes] > > Oh, disregard that then, turns out ReadingListModel is not an objc class, but a C++ one. No need to change anything.
Sorry, I thought this was an objc object. C++ object should not be naked pointers, so I assumed wrong. Can it be turned into something less dangerous? On Thu, 1 Jun 2017 at 18:58, <stkhapugin@chromium.org> wrote: > > > https://codereview.chromium.org/2888163004/diff/1/components/reading_list/ios... > File components/reading_list/ios/reading_list_model_bridge_observer.mm > (right): > > > https://codereview.chromium.org/2888163004/diff/1/components/reading_list/ios... > components/reading_list/ios/reading_list_model_bridge_observer.mm:17: : > observer_(observer), model_(model) { > On 2017/06/01 16:49:39, lindsayw wrote: > > On 2017/05/30 13:57:51, stkhapugin wrote: > > > On 2017/05/29 11:57:59, noyau (Ping after 24h) wrote: > > > > This now has the side effect of retaining the model, as the header > only has > > a > > > > comment specifying weak: With ARC this is now strong by default. > > > > > > > > In the header model_ needs to be marked as unsafe_unretained at a > minimum. > > > > > > In reading_list_model_bridge_observer.h, replace: > > > ReadingListModel* model_; // weak > > > with: > > > __unsafe_unretained ReadingListModel* model_; > > > > > > Ideally, try to replace it with: > > > __weak ReadingListModel* model_; > > > and see if it builds. If it does, also replace: > > > __unsafe_unretained id<ReadingListModelBridgeObserver> > observer_; > > > with: > > > __weak id<ReadingListModelBridgeObserver> observer_; > > > > It won't build with __weak ReadingListModel* model_; so I put > > __unsafe_unretained ReadingListModel* model_; and that also doesn't > build > > successfully. The error for the latter is: > > > > ../../components/reading_list/ios/reading_list_model_bridge_observer.h:78:3: > > error: 'objc_ownership' only applies to Objective-C object or block > pointer > > types; type here is 'ReadingListModel *' > [-Werror,-Wignored-attributes] > > > > > > Oh, disregard that then, turns out ReadingListModel is not an objc > class, but a C++ one. No need to change anything. > > https://codereview.chromium.org/2888163004/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/2888163004/diff/1/components/reading_list/ios... File components/reading_list/ios/reading_list_model_bridge_observer.mm (right): https://codereview.chromium.org/2888163004/diff/1/components/reading_list/ios... components/reading_list/ios/reading_list_model_bridge_observer.mm:17: : observer_(observer), model_(model) { On 2017/06/01 16:58:43, stkhapugin wrote: > On 2017/06/01 16:49:39, lindsayw wrote: > > On 2017/05/30 13:57:51, stkhapugin wrote: > > > On 2017/05/29 11:57:59, noyau (Ping after 24h) wrote: > > > > This now has the side effect of retaining the model, as the header only > has > > a > > > > comment specifying weak: With ARC this is now strong by default. > > > > > > > > In the header model_ needs to be marked as unsafe_unretained at a minimum. > > > > > > In reading_list_model_bridge_observer.h, replace: > > > ReadingListModel* model_; // weak > > > with: > > > __unsafe_unretained ReadingListModel* model_; > > > > > > Ideally, try to replace it with: > > > __weak ReadingListModel* model_; > > > and see if it builds. If it does, also replace: > > > __unsafe_unretained id<ReadingListModelBridgeObserver> observer_; > > > with: > > > __weak id<ReadingListModelBridgeObserver> observer_; > > > > It won't build with __weak ReadingListModel* model_; so I put > > __unsafe_unretained ReadingListModel* model_; and that also doesn't build > > successfully. The error for the latter is: > > ../../components/reading_list/ios/reading_list_model_bridge_observer.h:78:3: > > error: 'objc_ownership' only applies to Objective-C object or block pointer > > types; type here is 'ReadingListModel *' [-Werror,-Wignored-attributes] > > > > > > Oh, disregard that then, turns out ReadingListModel is not an objc class, but a > C++ one. No need to change anything. I've added a toto to refactor the weak pointer. PTAL, noyau :)
The CQ bit was checked by lindsayw@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. https://codereview.chromium.org/2888163004/diff/20001/components/reading_list... File components/reading_list/ios/reading_list_model_bridge_observer.h (right): https://codereview.chromium.org/2888163004/diff/20001/components/reading_list... components/reading_list/ios/reading_list_model_bridge_observer.h:79: // TODO(crbug.com/729015): Refactor into strong pointer not into strong pointer. It could be a weak one. I'd say "refactor to remove the naked pointer."
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/06/02 14:25:41, noyau (Ping after 24h) wrote: > lgtm. > > https://codereview.chromium.org/2888163004/diff/20001/components/reading_list... > File components/reading_list/ios/reading_list_model_bridge_observer.h (right): > > https://codereview.chromium.org/2888163004/diff/20001/components/reading_list... > components/reading_list/ios/reading_list_model_bridge_observer.h:79: // > TODO(crbug.com/729015): Refactor into strong pointer > not into strong pointer. It could be a weak one. I'd say "refactor to remove the > naked pointer." Done.
The CQ bit was checked by lindsayw@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from noyau@chromium.org Link to the patchset: https://codereview.chromium.org/2888163004/#ps40001 (title: "Reworded todo.")
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": 1496427111255930, "parent_rev": "e43cd34d7b6f8ce4a48f4e3a704c01e3c6e25dfc", "commit_rev": "f3fd628d5db7f01f9f11bf1ba00130ee03a2f2f8"}
Message was sent while issue was closed.
Description was changed from ========== [ObjC ARC] Converts components/reading_list/ios:ios to ARC. Automatically generated ARCMigrate commit Notable issues:None BUG=624363 TEST=None ========== to ========== [ObjC ARC] Converts components/reading_list/ios:ios to ARC. Automatically generated ARCMigrate commit Notable issues:None BUG=624363 TEST=None Review-Url: https://codereview.chromium.org/2888163004 Cr-Commit-Position: refs/heads/master@{#476763} Committed: https://chromium.googlesource.com/chromium/src/+/f3fd628d5db7f01f9f11bf1ba001... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/f3fd628d5db7f01f9f11bf1ba001... |