Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2709013002:
[ios clean] Rename removeChildViewController -> detachChildViewController (Closed)
|
Created:
3 years, 10 months ago by justincohen Modified:
3 years, 10 months ago CC:
chromium-reviews, marq+scrutinize_chromium.org, lpromero+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[ios clean] Rename removeChildViewController -> detachChildViewController
-removeChildViewController is used internally by UIKit. Without this change,
calling removeChildViewController's leads to an infinite loop.
BUG=695013
Review-Url: https://codereview.chromium.org/2709013002
Cr-Commit-Position: refs/heads/master@{#452024}
Committed: https://chromium.googlesource.com/chromium/src/+/8c112333b3a0eff529b84d80437c3d27c519142f
Patch Set 1 #
Total comments: 4
Patch Set 2 : Rename method #Patch Set 3 : Rebase #
Messages
Total messages: 28 (12 generated)
justincohen@chromium.org changed reviewers: + lpromero@chromium.org, marq@chromium.org, rohitrao@chromium.org
Can we add some OWNERS to clean?
rohitrao@chromium.org changed reviewers: + edchin@chromium.org
lgtm but please wait for one other reviewer We should document the reason for this somewhere. It's probably overkill to add a comment in every file that contains this method, but is there a better place for it?
The name -removeChildViewController sounds like it would be a UIKit method, but the UIKit method is -removeFromParentViewController. (https://developer.apple.com/reference/uikit/uiviewcontroller/1621425-removefr...) https://codereview.chromium.org/2709013002/diff/1/ios/clean/chrome/browser/ui... File ios/clean/chrome/browser/ui/tab/tab_container_view_controller.mm (right): https://codereview.chromium.org/2709013002/diff/1/ios/clean/chrome/browser/ui... ios/clean/chrome/browser/ui/tab/tab_container_view_controller.mm:37: - (void)removeChildViewControllerFromParent:(UIViewController*)viewController; This looks like the parameter is the parent, not the child VC.
The problem is that UIViewController has a (undocumented) method called removeChildViewController: You can test this by running something like this in a test app: NSLog(@"%d", [self respondsToSelector:@selector(removeChildViewController:)]); --> Prints 1, because that's a real method =( Because of this, we have to avoid giving our methods the same name. We've found evidence of other projects making similar changes on github.
Ah yes, I now understand. This method is undocumented. However, let me play the devil's advocate here. Dynamic binding should ensure that the subclass method is called at runtime. Calling [super removeChildViewController:vc] would cause a compile error.
Description was changed from ========== [ios clean] Rename removeChildViewController -> removeChildViewControllerFromParent -removeChildViewController is used internally by UIKit. BUG= ========== to ========== [ios clean] Rename removeChildViewController -> removeChildViewControllerFromParent -removeChildViewController is used internally by UIKit. Without this change, calling removeChildViewController's leads to an infinite loop. BUG= ==========
Updated description. This change is required due to an infinite loop caused by us using the same method name as a private API. ::-[TabContainerViewController removeChildViewController:](UIViewController *) at chrome/browser/ui/tab/tab_container_view_controller.mm:209 -[UIViewController(UIContainerViewControllerProtectedMethods) removeFromParentViewController] () ::-[TabContainerViewController removeChildViewController:](UIViewController *) at chrome/browser/ui/tab/tab_container_view_controller.mm:209 -[UIViewController(UIContainerViewControllerProtectedMethods) removeFromParentViewController] () ::-[TabContainerViewController removeChildViewController:](UIViewController *) at chrome/browser/ui/tab/tab_container_view_controller.mm:209 ::-[TabContainerViewController setContentViewController:](UIViewController *) at chrome/browser/ui/tab/tab_container_view_controller.mm:76
https://codereview.chromium.org/2709013002/diff/1/ios/clean/chrome/browser/ui... File ios/clean/chrome/browser/ui/tab/tab_container_view_controller.mm (right): https://codereview.chromium.org/2709013002/diff/1/ios/clean/chrome/browser/ui... ios/clean/chrome/browser/ui/tab/tab_container_view_controller.mm:37: - (void)removeChildViewControllerFromParent:(UIViewController*)viewController; On 2017/02/21 23:28:16, edchin wrote: > This looks like the parameter is the parent, not the child VC. I suggest namespacing with a prefix. -(void)cr_removeChildViewController:
Forgot to lgtm in my last comment.
> I suggest namespacing with a prefix. -(void)cr_removeChildViewController: Namespacing will imply that the method is in a category or otherwise bolted on to UIViewController, which isn't the case here. We shouldn't have to prefix methods when we subclass from UIKit, and we don't do this anywhere else. I'd prefer to pick a different name and leave a comment explaining why we needed these shenanigans. We could also file an ExternalDependency bug if we want a paper trail.
LGTM modulo a different method name. https://codereview.chromium.org/2709013002/diff/1/ios/clean/chrome/browser/ui... File ios/clean/chrome/browser/ui/tab/tab_container_view_controller.mm (right): https://codereview.chromium.org/2709013002/diff/1/ios/clean/chrome/browser/ui... ios/clean/chrome/browser/ui/tab/tab_container_view_controller.mm:37: - (void)removeChildViewControllerFromParent:(UIViewController*)viewController; On 2017/02/21 23:28:16, edchin wrote: > This looks like the parameter is the parent, not the child VC. I agree. How about -detachChildViewController: ?, and a comment that -removeChildViewController: is a private superclass method.
On 2017/02/22 09:13:44, marq wrote: > LGTM modulo a different method name. > > https://codereview.chromium.org/2709013002/diff/1/ios/clean/chrome/browser/ui... > File ios/clean/chrome/browser/ui/tab/tab_container_view_controller.mm (right): > > https://codereview.chromium.org/2709013002/diff/1/ios/clean/chrome/browser/ui... > ios/clean/chrome/browser/ui/tab/tab_container_view_controller.mm:37: - > (void)removeChildViewControllerFromParent:(UIViewController*)viewController; > On 2017/02/21 23:28:16, edchin wrote: > > This looks like the parameter is the parent, not the child VC. > > I agree. How about -detachChildViewController: ?, and a comment that > -removeChildViewController: is a private superclass method. Also a PRESUBMIT check wouldn't hurt.
Description was changed from ========== [ios clean] Rename removeChildViewController -> removeChildViewControllerFromParent -removeChildViewController is used internally by UIKit. Without this change, calling removeChildViewController's leads to an infinite loop. BUG= ========== to ========== [ios clean] Rename removeChildViewController -> detachChildViewController -removeChildViewController is used internally by UIKit. Without this change, calling removeChildViewController's leads to an infinite loop. BUG= ==========
I'll create a tracking bug for a PRESUBMIT followup. https://codereview.chromium.org/2709013002/diff/1/ios/clean/chrome/browser/ui... File ios/clean/chrome/browser/ui/tab/tab_container_view_controller.mm (right): https://codereview.chromium.org/2709013002/diff/1/ios/clean/chrome/browser/ui... ios/clean/chrome/browser/ui/tab/tab_container_view_controller.mm:37: - (void)removeChildViewControllerFromParent:(UIViewController*)viewController; On 2017/02/22 09:13:44, marq wrote: > On 2017/02/21 23:28:16, edchin wrote: > > This looks like the parameter is the parent, not the child VC. > > I agree. How about -detachChildViewController: ?, and a comment that > -removeChildViewController: is a private superclass method. Done.
Description was changed from ========== [ios clean] Rename removeChildViewController -> detachChildViewController -removeChildViewController is used internally by UIKit. Without this change, calling removeChildViewController's leads to an infinite loop. BUG= ========== to ========== [ios clean] Rename removeChildViewController -> detachChildViewController -removeChildViewController is used internally by UIKit. Without this change, calling removeChildViewController's leads to an infinite loop. BUG=695013 ==========
The CQ bit was checked by justincohen@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from marq@chromium.org, edchin@chromium.org, rohitrao@chromium.org Link to the patchset: https://codereview.chromium.org/2709013002/#ps20001 (title: "Rename method")
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: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by justincohen@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from marq@chromium.org, edchin@chromium.org, rohitrao@chromium.org Link to the patchset: https://codereview.chromium.org/2709013002/#ps40001 (title: "Rebase")
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": 1487768326921160,
"parent_rev": "befbcdf4f36ecd0f2cbc58687bdc9c854156c49e", "commit_rev":
"8c112333b3a0eff529b84d80437c3d27c519142f"}
Message was sent while issue was closed.
Description was changed from ========== [ios clean] Rename removeChildViewController -> detachChildViewController -removeChildViewController is used internally by UIKit. Without this change, calling removeChildViewController's leads to an infinite loop. BUG=695013 ========== to ========== [ios clean] Rename removeChildViewController -> detachChildViewController -removeChildViewController is used internally by UIKit. Without this change, calling removeChildViewController's leads to an infinite loop. BUG=695013 Review-Url: https://codereview.chromium.org/2709013002 Cr-Commit-Position: refs/heads/master@{#452024} Committed: https://chromium.googlesource.com/chromium/src/+/8c112333b3a0eff529b84d80437c... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/8c112333b3a0eff529b84d80437c... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
