Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2846233002:
[ios] ARCMigrate ios/chrome/browser/tabs/tab.mm to ARC. (Closed)
|
Created:
3 years, 7 months ago by sdefresne Modified:
3 years, 7 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[ios] ARCMigrate ios/chrome/browser/tabs/tab.mm to ARC.
Semi-automatic conversion of ios/chrome/browser/tabs/tab.mm
to ARC (Automatic Reference Counting).
Manual changes:
- audit raw Objective-C pointers and mark them as __weak if
needed (were all correctly annotated with weak comments);
- convert base::scoped_nsobject<> & base::WeakNSObject<> to
annotations for the compiler (e.g. __weak);
- remove setter/getter for properties that can be synthesized;
- remove [[... retain] autorelease] as the CRWWebController
has been converted to ARC already and thus the object will
be autorelease and the hack is no longer required.
BUG=None
Review-Url: https://codereview.chromium.org/2846233002
Cr-Commit-Position: refs/heads/master@{#468660}
Committed: https://chromium.googlesource.com/chromium/src/+/376186504c93feac078435f1620005d15609974f
Patch Set 1 #
Total comments: 10
Patch Set 2 : Rebase. #Patch Set 3 : Address comments. #
Messages
Total messages: 25 (13 generated)
The CQ bit was checked by sdefresne@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...
sdefresne@chromium.org changed reviewers: + rohitrao@chromium.org, stkhapugin@chromium.org
stkhapugin: please take a look. rohitrao: FYI
Will take a look on Monday morning.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
rohitrao: ping
Description was changed from
==========
[ios] ARCMigrate ios/chrome/borwser/tabs/tab.mm to ARC.
Semi-automatic conversion of ios/chrome/borwser/tabs/tab.mm
to ARC (Automatic Reference Counting).
Manual changes:
- audit raw Objective-C pointers and mark them as __weak if
needed (were all correctly annotated with weak comments);
- convert base::scoped_nsobject<> & base::WeakNSObject<> to
annotations for the compiler (e.g. __weak);
- remove setter/getter for properties that can be synthesized;
- remove [[... retain] autorelease] as the CRWWebController
has been converted to ARC already and thus the object will
be autorelease and the hack is no longer required.
BUG=None
==========
to
==========
[ios] ARCMigrate ios/chrome/browser/tabs/tab.mm to ARC.
Semi-automatic conversion of ios/chrome/browser/tabs/tab.mm
to ARC (Automatic Reference Counting).
Manual changes:
- audit raw Objective-C pointers and mark them as __weak if
needed (were all correctly annotated with weak comments);
- convert base::scoped_nsobject<> & base::WeakNSObject<> to
annotations for the compiler (e.g. __weak);
- remove setter/getter for properties that can be synthesized;
- remove [[... retain] autorelease] as the CRWWebController
has been converted to ARC already and thus the object will
be autorelease and the hack is no longer required.
BUG=None
==========
https://codereview.chromium.org/2846233002/diff/1/ios/chrome/browser/tabs/BUI... File ios/chrome/browser/tabs/BUILD.gn (right): https://codereview.chromium.org/2846233002/diff/1/ios/chrome/browser/tabs/BUI... ios/chrome/browser/tabs/BUILD.gn:40: deps = [ Can any of these deps be removed, now that tab.mm is no longer in this target? https://codereview.chromium.org/2846233002/diff/1/ios/chrome/browser/tabs/tab.mm File ios/chrome/browser/tabs/tab.mm (right): https://codereview.chromium.org/2846233002/diff/1/ios/chrome/browser/tabs/tab... ios/chrome/browser/tabs/tab.mm:500: base::scoped_nsprotocol<id<PasswordsUiDelegate>> passwordsUiDelegate( Intentionally still using scoped_nsprotocol? https://codereview.chromium.org/2846233002/diff/1/ios/chrome/browser/tabs/tab... ios/chrome/browser/tabs/tab.mm:554: if ([parentTabModel_ syncedWindowDelegate]) { This no longer works as a property? https://codereview.chromium.org/2846233002/diff/1/ios/chrome/browser/tabs/tab... ios/chrome/browser/tabs/tab.mm:1100: // [[self.webController retain] autorelease]; What is the plan for replacing this call? Should we hold a strong reference to the webController instead? https://codereview.chromium.org/2846233002/diff/1/ios/chrome/browser/tabs/tab... ios/chrome/browser/tabs/tab.mm:1557: base::scoped_nsprotocol<id<NativeAppMetadata>> metadata( Are we intentionally still using scoped_nsprotocol here? If so, is that worth an explanatory comment?
LGTM with Rohit's comments. Special thanks for MakeUnique. Optional follow-up - rename trailing_underscore_ to _leading_underscore ivars!
The CQ bit was checked by sdefresne@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...
rohitrao: PTAL https://codereview.chromium.org/2846233002/diff/1/ios/chrome/browser/tabs/BUI... File ios/chrome/browser/tabs/BUILD.gn (right): https://codereview.chromium.org/2846233002/diff/1/ios/chrome/browser/tabs/BUI... ios/chrome/browser/tabs/BUILD.gn:40: deps = [ On 2017/05/02 11:42:53, rohitrao (ping after 24h) wrote: > Can any of these deps be removed, now that tab.mm is no longer in this target? "tabs_internal" & "tabs_internal_arc" will be merged in the third CL converting "tabs_internal" to ARC. I've already added all the required dependencies to "tabs_internal_arc", so I'll do the merge by removing "tabs_internal" and renaming "tabs_internal_arc" to "tabs_internal". Cleanup this list of dependencies has low value and I don't think it should be done. https://codereview.chromium.org/2846233002/diff/1/ios/chrome/browser/tabs/tab.mm File ios/chrome/browser/tabs/tab.mm (right): https://codereview.chromium.org/2846233002/diff/1/ios/chrome/browser/tabs/tab... ios/chrome/browser/tabs/tab.mm:500: base::scoped_nsprotocol<id<PasswordsUiDelegate>> passwordsUiDelegate( On 2017/05/02 11:42:54, rohitrao (ping after 24h) wrote: > Intentionally still using scoped_nsprotocol? No, I missed that one. Fixed. Thank you for the catch. https://codereview.chromium.org/2846233002/diff/1/ios/chrome/browser/tabs/tab... ios/chrome/browser/tabs/tab.mm:554: if ([parentTabModel_ syncedWindowDelegate]) { On 2017/05/02 11:42:54, rohitrao (ping after 24h) wrote: > This no longer works as a property? I had an intermediate steps where I changed parentTabModel_ to base::WeakNSObject<> which required changing this to method call syntax. This still works as a property, reverted. https://codereview.chromium.org/2846233002/diff/1/ios/chrome/browser/tabs/tab... ios/chrome/browser/tabs/tab.mm:1100: // [[self.webController retain] autorelease]; On 2017/05/02 11:42:54, rohitrao (ping after 24h) wrote: > What is the plan for replacing this call? Should we hold a strong reference to > the webController instead? This call can be safely removed as the lifetime of the objects is now sane. I just forgot to remove the code after commenting it. I've tested the reproduction steps for the linked bug and the app does not crash. https://codereview.chromium.org/2846233002/diff/1/ios/chrome/browser/tabs/tab... ios/chrome/browser/tabs/tab.mm:1557: base::scoped_nsprotocol<id<NativeAppMetadata>> metadata( On 2017/05/02 11:42:54, rohitrao (ping after 24h) wrote: > Are we intentionally still using scoped_nsprotocol here? If so, is that worth > an explanatory comment? No, it was a mistake (but the code has been removed in another CL and disappeared during the rebase, so nothing to do here).
lgtm
The CQ bit was unchecked by sdefresne@chromium.org
The CQ bit was checked by sdefresne@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from stkhapugin@chromium.org Link to the patchset: https://codereview.chromium.org/2846233002/#ps40001 (title: "Address comments.")
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": 1493740388950140,
"parent_rev": "14f41de96dc519117571e6601e0859648b2e249b", "commit_rev":
"376186504c93feac078435f1620005d15609974f"}
Message was sent while issue was closed.
Description was changed from
==========
[ios] ARCMigrate ios/chrome/browser/tabs/tab.mm to ARC.
Semi-automatic conversion of ios/chrome/browser/tabs/tab.mm
to ARC (Automatic Reference Counting).
Manual changes:
- audit raw Objective-C pointers and mark them as __weak if
needed (were all correctly annotated with weak comments);
- convert base::scoped_nsobject<> & base::WeakNSObject<> to
annotations for the compiler (e.g. __weak);
- remove setter/getter for properties that can be synthesized;
- remove [[... retain] autorelease] as the CRWWebController
has been converted to ARC already and thus the object will
be autorelease and the hack is no longer required.
BUG=None
==========
to
==========
[ios] ARCMigrate ios/chrome/browser/tabs/tab.mm to ARC.
Semi-automatic conversion of ios/chrome/browser/tabs/tab.mm
to ARC (Automatic Reference Counting).
Manual changes:
- audit raw Objective-C pointers and mark them as __weak if
needed (were all correctly annotated with weak comments);
- convert base::scoped_nsobject<> & base::WeakNSObject<> to
annotations for the compiler (e.g. __weak);
- remove setter/getter for properties that can be synthesized;
- remove [[... retain] autorelease] as the CRWWebController
has been converted to ARC already and thus the object will
be autorelease and the hack is no longer required.
BUG=None
Review-Url: https://codereview.chromium.org/2846233002
Cr-Commit-Position: refs/heads/master@{#468660}
Committed:
https://chromium.googlesource.com/chromium/src/+/376186504c93feac078435f16200...
==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/376186504c93feac078435f16200...
Message was sent while issue was closed.
A revert of this CL (patchset #3 id:40001) has been created in https://codereview.chromium.org/2859663002/ by michaeldo@chromium.org. The reason for reverting is: causes failure of ios_chrome_ui_egtests on ipad air 2 ios 10. Ref: https://build.chromium.org/p/chromium.fyi/builders/EarlGreyiOS/builds/27116.
Message was sent while issue was closed.
On 2017/05/02 21:22:49, michaeldo wrote: > A revert of this CL (patchset #3 id:40001) has been created in > https://codereview.chromium.org/2859663002/ by mailto:michaeldo@chromium.org. > > The reason for reverting is: causes failure of ios_chrome_ui_egtests on ipad air > 2 ios 10. > > Ref: > https://build.chromium.org/p/chromium.fyi/builders/EarlGreyiOS/builds/27116. It looks like the crash happens because StaticHtmlViewController is using KVO to observe the WebView and unregister itself in its -dealloc method. Since some objects are now autoreleased instead of being released, the -dealloc is invoked later, and by this time, CRWWebController's NavigationManager pointer is null, thus crash.
Message was sent while issue was closed.
On 2017/05/03 08:25:51, sdefresne wrote: > On 2017/05/02 21:22:49, michaeldo wrote: > > A revert of this CL (patchset #3 id:40001) has been created in > > https://codereview.chromium.org/2859663002/ by mailto:michaeldo@chromium.org. > > > > The reason for reverting is: causes failure of ios_chrome_ui_egtests on ipad > air > > 2 ios 10. > > > > Ref: > > https://build.chromium.org/p/chromium.fyi/builders/EarlGreyiOS/builds/27116. > > It looks like the crash happens because StaticHtmlViewController is using KVO to > observe the WebView and unregister itself in its -dealloc method. Since some > objects are now autoreleased instead of being released, the -dealloc is invoked > later, and by this time, CRWWebController's NavigationManager pointer is null, > thus crash. Could be due to https://bugs.chromium.org/p/chromium/issues/detail?id=503297 (which the migration to ARC exacerbates). |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
