Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2434853002:
[ObjC ARC] Converts crw_web_controller to ARC. (Closed)
|
Created:
4 years, 2 months ago by stkhapugin Modified:
4 years, 1 month ago CC:
chromium-reviews, mac-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[ObjC ARC] Converts crw_web_controller to ARC.
Notable changes:
* crw_web_controller.mm used to have a dictionary storing selectors just
for keeping the KVO code clean. ARC cannot call a selector from a string
like this, because it cannot guarantee correct memory management in
this case. So it was replaced with an array of observed key paths to
keep some of the code cleanness, while the KVO callback has a huge
switch.
BUG=624365
TEST=None
Committed: https://crrev.com/9b6f44b09855a803914efd9877fddb471a97cbf1
Cr-Commit-Position: refs/heads/master@{#430304}
Patch Set 1 #Patch Set 2 : Removes weak_nsobjects #Patch Set 3 : little changes #Patch Set 4 : Expose delegate #
Total comments: 38
Patch Set 5 : Comments #Patch Set 6 : rebase #
Total comments: 2
Patch Set 7 : rebase #Patch Set 8 : Comments & rebase #Patch Set 9 : d #
Depends on Patchset: Messages
Total messages: 31 (21 generated)
The CQ bit was checked by stkhapugin@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...
Description was changed from ========== [ObjC ARC] Converts crw_web_controller to ARC. BUG=624365 TEST=None ========== to ========== [ObjC ARC] Converts crw_web_controller to ARC. Notable changes: * crw_web_controller.mm used to have a dictionary storing selectors just for keeping the KVO code clean. ARC cannot call a selector from a string like this, because it cannot guarantee correct memory management in this case. So it was replaced with an array of observed key paths to keep some of the code cleanness, while the KVO callback has a huge switch. BUG=624365 TEST=None ==========
stkhapugin@chromium.org changed reviewers: + eugenebut@google.com, sdefresne@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
The CQ bit was checked by stkhapugin@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...
Description was changed from ========== [ObjC ARC] Converts crw_web_controller to ARC. Notable changes: * crw_web_controller.mm used to have a dictionary storing selectors just for keeping the KVO code clean. ARC cannot call a selector from a string like this, because it cannot guarantee correct memory management in this case. So it was replaced with an array of observed key paths to keep some of the code cleanness, while the KVO callback has a huge switch. BUG=624365 TEST=None ========== to ========== [ObjC ARC] Converts crw_web_controller to ARC. Notable changes: * crw_web_controller.mm used to have a dictionary storing selectors just for keeping the KVO code clean. ARC cannot call a selector from a string like this, because it cannot guarantee correct memory management in this case. So it was replaced with an array of observed key paths to keep some of the code cleanness, while the KVO callback has a huge switch. * Exposed delegate property on crw_web_controller_container_view. It is required as crw_web_controller needs to zero-out its container view reference before self destruction, because of a slight difference in lifetimes of WeakNSObject and __weak objects. BUG=624365 TEST=None ==========
stkhapugin@chromium.org changed reviewers: + eugenebut@chromium.org - eugenebut@google.com
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm but please wait for eugenebut review too https://codereview.chromium.org/2434853002/diff/60001/ios/web/web_state/ui/cr... File ios/web/web_state/ui/crw_web_controller.h (right): https://codereview.chromium.org/2434853002/diff/60001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_web_controller.h:85: @property(weak, nonatomic, readonly) UIView* view; nit: could you use the same order as above (here and below): @property(nonatomic, weak, readonly) UIView* view; https://codereview.chromium.org/2434853002/diff/60001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_web_controller.h:96: @property(nonatomic, readwrite, assign) BOOL usePlaceholderOverlay; nit: remove readwrite as this is the default https://codereview.chromium.org/2434853002/diff/60001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_web_controller.h:99: @property(nonatomic, readonly) web::LoadPhase loadPhase; nit: assign https://codereview.chromium.org/2434853002/diff/60001/ios/web/web_state/ui/cr... File ios/web/web_state/ui/crw_web_controller.mm (right): https://codereview.chromium.org/2434853002/diff/60001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_web_controller.mm:474: @property(weak, nonatomic, readonly) WKWebView* webView; ditto regarding ordering of keywords https://codereview.chromium.org/2434853002/diff/60001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_web_controller.mm:481: @property(weak, nonatomic, readwrite) id<CRWNativeContent> nativeController; ditto regarding readwrite https://codereview.chromium.org/2434853002/diff/60001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_web_controller.mm:5255: DCHECK(false); NOTREACHED();
Thank you for this! https://codereview.chromium.org/2434853002/diff/60001/ios/web/web_state/ui/cr... File ios/web/web_state/ui/crw_web_controller.h (right): https://codereview.chromium.org/2434853002/diff/60001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_web_controller.h:85: @property(weak, nonatomic, readonly) UIView* view; WebController owns a view, so it should probably be strong. https://codereview.chromium.org/2434853002/diff/60001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_web_controller.h:88: @property(weak, nonatomic, readonly) id<CRWWebViewProxy> webViewProxy; Should this be strong? https://codereview.chromium.org/2434853002/diff/60001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_web_controller.h:92: @property(weak, nonatomic, readonly) UIView* viewForPrinting; Should this be strong? https://codereview.chromium.org/2434853002/diff/60001/ios/web/web_state/ui/cr... File ios/web/web_state/ui/crw_web_controller.mm (right): https://codereview.chromium.org/2434853002/diff/60001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_web_controller.mm:474: @property(weak, nonatomic, readonly) WKWebView* webView; Should this actually be strong? https://codereview.chromium.org/2434853002/diff/60001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_web_controller.mm:476: @property(weak, nonatomic, readonly) UIScrollView* webScrollView; Should this actually be strong? https://codereview.chromium.org/2434853002/diff/60001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_web_controller.mm:481: @property(weak, nonatomic, readwrite) id<CRWNativeContent> nativeController; Should this actually be strong? https://codereview.chromium.org/2434853002/diff/60001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_web_controller.mm:483: @property(weak, nonatomic, readonly) CRWSessionController* sessionController; Should this actually be strong? https://codereview.chromium.org/2434853002/diff/60001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_web_controller.mm:485: @property(weak, nonatomic, readonly) NSString* activityIndicatorGroupID; Should this be copy? https://codereview.chromium.org/2434853002/diff/60001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_web_controller.mm:488: @property(weak, nonatomic, readonly) NSArray* observedKeyPaths; ditto https://codereview.chromium.org/2434853002/diff/60001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_web_controller.mm:490: @property(weak, nonatomic, readonly) CRWPassKitDownloader* passKitDownloader; Should this actually be strong? https://codereview.chromium.org/2434853002/diff/60001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_web_controller.mm:1084: [_containerView.get() setDelegate:nil]; You don't need |.get()| here. Also could you please land this separately as it looks like a general bug, unrelated to ARC. https://codereview.chromium.org/2434853002/diff/60001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_web_controller.mm:1700: base::scoped_nsobject<CRWWebController> strongSelf(weakSelf); nit: Do we still need scoped_nsobject here? https://codereview.chromium.org/2434853002/diff/60001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_web_controller.mm:2405: base::scoped_nsobject<CRWWebController> strongSelf(weakSelf); nit: Do we still need scoped_nsobject here? https://codereview.chromium.org/2434853002/diff/60001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_web_controller.mm:3013: base::scoped_nsobject<CRWWebController> strongSelf(weakSelf); nit: Do we still need scoped_nsobject here? https://codereview.chromium.org/2434853002/diff/60001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_web_controller.mm:3073: base::scoped_nsobject<CRWWebController> strongSelf(weakSelf); nit: Do we still need scoped_nsobject here? https://codereview.chromium.org/2434853002/diff/60001/ios/web/web_state/ui/cr... File ios/web/web_state/ui/crw_web_controller_container_view.h (right): https://codereview.chromium.org/2434853002/diff/60001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_web_controller_container_view.h:43: delegate; // weak So you can't use weak property because this header is included in non ARC implementation?
The CQ bit was checked by stkhapugin@chromium.org to run a CQ dry run
Thanks for your comments. I've split into two patches, as you asked. PTAL https://codereview.chromium.org/2434853002/diff/60001/ios/web/web_state/ui/cr... File ios/web/web_state/ui/crw_web_controller.h (right): https://codereview.chromium.org/2434853002/diff/60001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_web_controller.h:85: @property(weak, nonatomic, readonly) UIView* view; On 2016/10/19 18:44:07, sdefresne wrote: > nit: could you use the same order as above (here and below): > > @property(nonatomic, weak, readonly) UIView* view; Done. https://codereview.chromium.org/2434853002/diff/60001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_web_controller.h:88: @property(weak, nonatomic, readonly) id<CRWWebViewProxy> webViewProxy; On 2016/10/25 02:26:05, Eugene But wrote: > Should this be strong? Done. https://codereview.chromium.org/2434853002/diff/60001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_web_controller.h:92: @property(weak, nonatomic, readonly) UIView* viewForPrinting; On 2016/10/25 02:26:06, Eugene But wrote: > Should this be strong? Here and everywhere: a purely readonly property shouldn't be a property at all, IMO. Memory classifiers for them don't affect the behavior, unless they are synthesized. But most such properties in Chrome are actually getters for some other ivar, so they cannot be synthesized at all. Changing to strong, but in the future I'm not even sure we should keep memory classifiers for purely readonly properties. https://codereview.chromium.org/2434853002/diff/60001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_web_controller.h:96: @property(nonatomic, readwrite, assign) BOOL usePlaceholderOverlay; On 2016/10/19 18:44:07, sdefresne wrote: > nit: remove readwrite as this is the default Done. https://codereview.chromium.org/2434853002/diff/60001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_web_controller.h:99: @property(nonatomic, readonly) web::LoadPhase loadPhase; On 2016/10/19 18:44:07, sdefresne wrote: > nit: assign Done. https://codereview.chromium.org/2434853002/diff/60001/ios/web/web_state/ui/cr... File ios/web/web_state/ui/crw_web_controller.mm (right): https://codereview.chromium.org/2434853002/diff/60001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_web_controller.mm:474: @property(weak, nonatomic, readonly) WKWebView* webView; On 2016/10/19 18:44:07, sdefresne wrote: > ditto regarding ordering of keywords Done. https://codereview.chromium.org/2434853002/diff/60001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_web_controller.mm:476: @property(weak, nonatomic, readonly) UIScrollView* webScrollView; On 2016/10/25 02:26:06, Eugene But wrote: > Should this actually be strong? Done. https://codereview.chromium.org/2434853002/diff/60001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_web_controller.mm:483: @property(weak, nonatomic, readonly) CRWSessionController* sessionController; On 2016/10/25 02:26:06, Eugene But wrote: > Should this actually be strong? Done. https://codereview.chromium.org/2434853002/diff/60001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_web_controller.mm:485: @property(weak, nonatomic, readonly) NSString* activityIndicatorGroupID; On 2016/10/25 02:26:06, Eugene But wrote: > Should this be copy? Done. https://codereview.chromium.org/2434853002/diff/60001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_web_controller.mm:488: @property(weak, nonatomic, readonly) NSArray* observedKeyPaths; On 2016/10/25 02:26:06, Eugene But wrote: > ditto Done. https://codereview.chromium.org/2434853002/diff/60001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_web_controller.mm:490: @property(weak, nonatomic, readonly) CRWPassKitDownloader* passKitDownloader; On 2016/10/25 02:26:06, Eugene But wrote: > Should this actually be strong? Done. https://codereview.chromium.org/2434853002/diff/60001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_web_controller.mm:2405: base::scoped_nsobject<CRWWebController> strongSelf(weakSelf); On 2016/10/25 02:26:06, Eugene But wrote: > nit: Do we still need scoped_nsobject here? Here and below: no, we shouldn't need scoped_nsobject in .mm built with ARC at all. There will be a separate cleanup for scoped_nsobject in the future. I'm tempted to do one for this file without waiting for ARC to be complete. https://codereview.chromium.org/2434853002/diff/60001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_web_controller.mm:3013: base::scoped_nsobject<CRWWebController> strongSelf(weakSelf); On 2016/10/25 02:26:06, Eugene But wrote: > nit: Do we still need scoped_nsobject here? we shouldn't need scoped_nsobject in .mm built with ARC at all. There will be a separate cleanup for scoped_nsobject in the future https://codereview.chromium.org/2434853002/diff/60001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_web_controller.mm:3073: base::scoped_nsobject<CRWWebController> strongSelf(weakSelf); On 2016/10/25 02:26:06, Eugene But wrote: > nit: Do we still need scoped_nsobject here? we shouldn't need scoped_nsobject in .mm built with ARC at all. There will be a separate cleanup for scoped_nsobject in the future https://codereview.chromium.org/2434853002/diff/60001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_web_controller.mm:5255: DCHECK(false); On 2016/10/19 18:44:07, sdefresne wrote: > NOTREACHED(); Done. https://codereview.chromium.org/2434853002/diff/60001/ios/web/web_state/ui/cr... File ios/web/web_state/ui/crw_web_controller_container_view.h (right): https://codereview.chromium.org/2434853002/diff/60001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_web_controller_container_view.h:43: delegate; // weak On 2016/10/25 02:26:06, Eugene But wrote: > So you can't use weak property because this header is included in non ARC > implementation? Yes, specifically from crw_web_controller_container_view.mm. It will become weak in the next cl: https://codereview.chromium.org/2432363003/
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.
lgtm. Please update CL description since some of the statements are not valid after CL split. https://codereview.chromium.org/2434853002/diff/100001/ios/web/web_state/ui/c... File ios/web/web_state/ui/crw_web_controller.mm (right): https://codereview.chromium.org/2434853002/diff/100001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_web_controller.mm:481: @property(strong, nonatomic) id<CRWNativeContent> nativeController; nit: could you please swap strong and nonatomic for consistency
Description was changed from ========== [ObjC ARC] Converts crw_web_controller to ARC. Notable changes: * crw_web_controller.mm used to have a dictionary storing selectors just for keeping the KVO code clean. ARC cannot call a selector from a string like this, because it cannot guarantee correct memory management in this case. So it was replaced with an array of observed key paths to keep some of the code cleanness, while the KVO callback has a huge switch. * Exposed delegate property on crw_web_controller_container_view. It is required as crw_web_controller needs to zero-out its container view reference before self destruction, because of a slight difference in lifetimes of WeakNSObject and __weak objects. BUG=624365 TEST=None ========== to ========== [ObjC ARC] Converts crw_web_controller to ARC. Notable changes: * crw_web_controller.mm used to have a dictionary storing selectors just for keeping the KVO code clean. ARC cannot call a selector from a string like this, because it cannot guarantee correct memory management in this case. So it was replaced with an array of observed key paths to keep some of the code cleanness, while the KVO callback has a huge switch. BUG=624365 TEST=None ==========
https://codereview.chromium.org/2434853002/diff/100001/ios/web/web_state/ui/c... File ios/web/web_state/ui/crw_web_controller.mm (right): https://codereview.chromium.org/2434853002/diff/100001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_web_controller.mm:481: @property(strong, nonatomic) id<CRWNativeContent> nativeController; On 2016/10/25 20:04:54, Eugene But wrote: > nit: could you please swap strong and nonatomic for consistency Done.
The CQ bit was checked by stkhapugin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sdefresne@chromium.org, eugenebut@chromium.org Link to the patchset: https://codereview.chromium.org/2434853002/#ps140001 (title: "Comments & rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== [ObjC ARC] Converts crw_web_controller to ARC. Notable changes: * crw_web_controller.mm used to have a dictionary storing selectors just for keeping the KVO code clean. ARC cannot call a selector from a string like this, because it cannot guarantee correct memory management in this case. So it was replaced with an array of observed key paths to keep some of the code cleanness, while the KVO callback has a huge switch. BUG=624365 TEST=None ========== to ========== [ObjC ARC] Converts crw_web_controller to ARC. Notable changes: * crw_web_controller.mm used to have a dictionary storing selectors just for keeping the KVO code clean. ARC cannot call a selector from a string like this, because it cannot guarantee correct memory management in this case. So it was replaced with an array of observed key paths to keep some of the code cleanness, while the KVO callback has a huge switch. BUG=624365 TEST=None ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== [ObjC ARC] Converts crw_web_controller to ARC. Notable changes: * crw_web_controller.mm used to have a dictionary storing selectors just for keeping the KVO code clean. ARC cannot call a selector from a string like this, because it cannot guarantee correct memory management in this case. So it was replaced with an array of observed key paths to keep some of the code cleanness, while the KVO callback has a huge switch. BUG=624365 TEST=None ========== to ========== [ObjC ARC] Converts crw_web_controller to ARC. Notable changes: * crw_web_controller.mm used to have a dictionary storing selectors just for keeping the KVO code clean. ARC cannot call a selector from a string like this, because it cannot guarantee correct memory management in this case. So it was replaced with an array of observed key paths to keep some of the code cleanness, while the KVO callback has a huge switch. BUG=624365 TEST=None Committed: https://crrev.com/9b6f44b09855a803914efd9877fddb471a97cbf1 Cr-Commit-Position: refs/heads/master@{#430304} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/9b6f44b09855a803914efd9877fddb471a97cbf1 Cr-Commit-Position: refs/heads/master@{#430304}
Message was sent while issue was closed.
A revert of this CL (patchset #8 id:140001) has been created in https://codereview.chromium.org/2482803003/ by baxley@chromium.org. The reason for reverting is: This caused multiple Chrome for iOS unit tests to fail when creating tabs. It resulted in tests crashing when checking if currently on the web thread.. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
