Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2667873002:
[ios clean] Remove stop on Browser coordinator dealloc (Closed)
Description[ios clean] Remove stop on Browser coordinator dealloc
The previous code caused that dismissing the ToolMenu would dismiss the whole Tab
(Toolbar+content view) and left the App in a corrupted state. (The coordinators were
not stopped but their VC was dismissed).
This is a temporary fix. Marq will submit a more robust solution to this problem later on.
BUG=
Review-Url: https://codereview.chromium.org/2667873002
Cr-Commit-Position: refs/heads/master@{#448040}
Committed: https://chromium.googlesource.com/chromium/src/+/b367bcc4494b1b211268529557198d77d7cc0cb0
Patch Set 1 #Patch Set 2 : Format #
Total comments: 2
Patch Set 3 : Remove stop on Browsercoordinator dealloc #
Total comments: 2
Patch Set 4 : Update header #Patch Set 5 : Disable Test #
Total comments: 4
Patch Set 6 : CL Feedback #
Messages
Total messages: 37 (12 generated)
Description was changed from ========== [ios clean] Revert Tools Coordinator dismiss. BUG= ========== to ========== [ios clean] Revert Tools Coordinator dismiss. The previous code caused that dismissing the ToolMenu would dismiss the whole Tab (Toolbar+content view) and left the App in a corrupted state. (The coordinators were not stopped but their VC was dismissed). We can go over this once Louis is back. But I'd like to revert this on the meantime. We should also think on how to prevent this on the future. Of course tests will help, but maybe something can be done with the coordinators themselves to prevent others from dismissing the view they presented. BUG= ==========
sczs@chromium.org changed reviewers: + edchin@chromium.org, marq@chromium.org
Description was changed from ========== [ios clean] Revert Tools Coordinator dismiss. The previous code caused that dismissing the ToolMenu would dismiss the whole Tab (Toolbar+content view) and left the App in a corrupted state. (The coordinators were not stopped but their VC was dismissed). We can go over this once Louis is back. But I'd like to revert this on the meantime. We should also think on how to prevent this on the future. Of course tests will help, but maybe something can be done with the coordinators themselves to prevent others from dismissing the view they presented. BUG= ========== to ========== [ios clean] Revert Tools Coordinator dismiss. The previous code caused that dismissing the ToolMenu would dismiss the whole Tab (Toolbar+content view) and left the App in a corrupted state. (The coordinators were not stopped but their VC was dismissed). We can go over this once Louis is back. But I'd like to revert this on the meantime. We should also think on how to prevent this on the future. Of course tests will help, but maybe something can be done with the coordinators themselves to prevent others from dismissing the view they presented. BUG= ==========
Description was changed from ========== [ios clean] Revert Tools Coordinator dismiss. The previous code caused that dismissing the ToolMenu would dismiss the whole Tab (Toolbar+content view) and left the App in a corrupted state. (The coordinators were not stopped but their VC was dismissed). We can go over this once Louis is back. But I'd like to revert this on the meantime. We should also think on how to prevent this on the future. Of course tests will help, but maybe something can be done with the coordinators themselves to prevent others from dismissing the view they presented. BUG= ========== to ========== [ios clean] Revert Tools Coordinator dismiss. The previous code caused that dismissing the ToolMenu would dismiss the whole Tab (Toolbar+content view) and left the App in a corrupted state. (The coordinators were not stopped but their VC was dismissed). We can go over this once Louis is back. But I'd like to revert this on the meantime. We should also think on how to prevent this on the future. Of course tests will help, but maybe something can be done with the coordinators themselves to prevent others from dismissing the view they presented. BUG= ==========
PTAL
sczs@chromium.org changed reviewers: + lpromero@chromium.org
Description was changed from ========== [ios clean] Revert Tools Coordinator dismiss. The previous code caused that dismissing the ToolMenu would dismiss the whole Tab (Toolbar+content view) and left the App in a corrupted state. (The coordinators were not stopped but their VC was dismissed). We can go over this once Louis is back. But I'd like to revert this on the meantime. We should also think on how to prevent this on the future. Of course tests will help, but maybe something can be done with the coordinators themselves to prevent others from dismissing the view they presented. BUG= ========== to ========== [ios clean] Revert Tools Coordinator dismiss. The previous code caused that dismissing the ToolMenu would dismiss the whole Tab (Toolbar+content view) and left the App in a corrupted state. (The coordinators were not stopped but their VC was dismissed). We can go over this once Louis is back. But I'd like to revert this on the meantime. We should also think on how to prevent this on the future. Of course tests will help, but maybe something can be done with the coordinators themselves to prevent others from dismissing the view they presented, or to stop themselves if that happens? BUG= ==========
https://codereview.chromium.org/2667873002/diff/20001/ios/clean/chrome/browse... File ios/clean/chrome/browser/ui/tools/tools_coordinator.mm (left): https://codereview.chromium.org/2667873002/diff/20001/ios/clean/chrome/browse... ios/clean/chrome/browser/ui/tools/tools_coordinator.mm:41: [self.menuViewController.presentingViewController At cursory glance, this seems right to me since it has symmetry in the start and stop methods. I wonder if there is something else that is causing the corrupted state? https://codereview.chromium.org/2667873002/diff/20001/ios/clean/chrome/browse... File ios/clean/chrome/browser/ui/tools/tools_coordinator.mm (right): https://codereview.chromium.org/2667873002/diff/20001/ios/clean/chrome/browse... ios/clean/chrome/browser/ui/tools/tools_coordinator.mm:42: completion:nil]; Can you explain how this fixes the corrupted state?
I'd also like to see an explanation of what's broken here (I'm not disputing that anything is, just that I don't understand it based on the CL description). The change to have the presenting view controller do the dismissing is one we made globally, so are there other fixes that need to be made? And: how could we have caught this with a unit test?
On 2017/01/31 12:02:35, marq wrote: > I'd also like to see an explanation of what's broken here (I'm not disputing > that anything is, just that I don't understand it based on the CL description). > > The change to have the presenting view controller do the dismissing is one we > made globally, so are there other fixes that need to be made? > > And: how could we have caught this with a unit test? Thanks for taking a look! I was just suggesting this change since I don't think this was the expected behavior, but I didn't get into a lot of detail since I thought Louis would have a better idea.Not sure of other changes, but this is a video of what's happening: https://drive.google.com/open?id=0Byo6-Nuda2jgd1k4T0txWDVkaU0 This is the CL that made the change: https://codereview.chromium.org/2645973005/diff/20001/ios/clean/chrome/browse... I can start digging into this or we can wait for Louis, I guess its up to our priorities.
On 2017/01/31 15:44:27, sczs wrote: > On 2017/01/31 12:02:35, marq wrote: > > I'd also like to see an explanation of what's broken here (I'm not disputing > > that anything is, just that I don't understand it based on the CL > description). > > > > The change to have the presenting view controller do the dismissing is one we > > made globally, so are there other fixes that need to be made? > > > > And: how could we have caught this with a unit test? > > Thanks for taking a look! > > I was just suggesting this change since I don't think this was the expected > behavior, but I didn't get into a lot of detail since I thought Louis would > have a better idea.Not sure of other changes, but this is a video of what's > happening: > https://drive.google.com/open?id=0Byo6-Nuda2jgd1k4T0txWDVkaU0 > > This is the CL that made the change: > https://codereview.chromium.org/2645973005/diff/20001/ios/clean/chrome/browse... > > I can start digging into this or we can wait for Louis, I guess its up to our > priorities. Louis is on vacation for another nine days, so we can't wait for him. If you could get a better picture of why the bug happens that would be great.
On 2017/01/31 16:05:40, marq wrote: > On 2017/01/31 15:44:27, sczs wrote: > > On 2017/01/31 12:02:35, marq wrote: > > > I'd also like to see an explanation of what's broken here (I'm not disputing > > > that anything is, just that I don't understand it based on the CL > > description). > > > > > > The change to have the presenting view controller do the dismissing is one > we > > > made globally, so are there other fixes that need to be made? > > > > > > And: how could we have caught this with a unit test? > > > > Thanks for taking a look! > > > > I was just suggesting this change since I don't think this was the expected > > behavior, but I didn't get into a lot of detail since I thought Louis would > > have a better idea.Not sure of other changes, but this is a video of what's > > happening: > > https://drive.google.com/open?id=0Byo6-Nuda2jgd1k4T0txWDVkaU0 > > > > This is the CL that made the change: > > > https://codereview.chromium.org/2645973005/diff/20001/ios/clean/chrome/browse... > > > > I can start digging into this or we can wait for Louis, I guess its up to our > > priorities. > > Louis is on vacation for another nine days, so we can't wait for him. If you > could get a better picture of why the bug happens that would be great. Sounds good, will start taking a look at this!
On 2017/01/31 16:09:05, sczs wrote: > On 2017/01/31 16:05:40, marq wrote: > > On 2017/01/31 15:44:27, sczs wrote: > > > On 2017/01/31 12:02:35, marq wrote: > > > > I'd also like to see an explanation of what's broken here (I'm not > disputing > > > > that anything is, just that I don't understand it based on the CL > > > description). > > > > > > > > The change to have the presenting view controller do the dismissing is one > > we > > > > made globally, so are there other fixes that need to be made? > > > > > > > > And: how could we have caught this with a unit test? > > > > > > Thanks for taking a look! > > > > > > I was just suggesting this change since I don't think this was the expected > > > behavior, but I didn't get into a lot of detail since I thought Louis would > > > have a better idea.Not sure of other changes, but this is a video of what's > > > happening: > > > https://drive.google.com/open?id=0Byo6-Nuda2jgd1k4T0txWDVkaU0 > > > > > > This is the CL that made the change: > > > > > > https://codereview.chromium.org/2645973005/diff/20001/ios/clean/chrome/browse... > > > > > > I can start digging into this or we can wait for Louis, I guess its up to > our > > > priorities. > > > > Louis is on vacation for another nine days, so we can't wait for him. If you > > could get a better picture of why the bug happens that would be great. > > Sounds good, will start taking a look at this! I know Louis was trying to dismiss everything that was modally presented by the VC, that way we would dismiss all the Views that were presented by the VC and not just the topmost View. The problem is that presentViewController is not always called on the object handling the presentation. "The object on which you call this method may not always be the one that handles the presentation." https://developer.apple.com/reference/uikit/uiviewcontroller/1621380-presentv... For this reason self.menuViewController.presentingViewController is actually the TabStripContainerViewController and not the ToolbarVC as I think we were expecting. https://developer.apple.com/reference/uikit/uiviewcontroller/1621430-presenti... So as Louis intended we are dismissing all the Views that were being presented, the only problem is that this is taking it one level further than what's needed and dismissing everything including the TabStripContainerVC. The second issue is that the TabStripContainerVC is dismissed but the TabStripCoordinator hasn't been stopped, this causes that tapping on settings will crash the App, since TabGridCoordinator doesn't handle -addOverlayCoordinator (browser_coordinator.mm line 98). Same thing happens with showTabGrid, since there will be more than 1 children coordinator and the check fails.
> I know Louis was trying to dismiss everything that was modally presented by the > VC, > that way we would dismiss all the Views that were presented by the VC and not > just > the topmost View. If we keep a balance of starting/stopping coordinators and presenting/dismissing VC's, stopping a coordinator in the middle of the coordinator stack should properly dismiss all the VC's at that level and beyond. > The problem is that presentViewController is not always called on the object > handling the presentation. > "The object on which you call this method may not always be the one that handles > the presentation." > https://developer.apple.com/reference/uikit/uiviewcontroller/1621380-presentv... One such example is if you call presentVC on a contained VC. The actual presentingVC is the containerVC. In this scenario, I believe we would see the correct behavior and the correct VC would be dismissed. > that this is taking it one level further than what's needed and dismissing I'm not quite understanding what is causing the dismissal to go "one level further". Can you clarify?
On 2017/01/31 23:29:20, edchin wrote: > > I know Louis was trying to dismiss everything that was modally presented by > the > > VC, > > that way we would dismiss all the Views that were presented by the VC and not > > just > > the topmost View. > > If we keep a balance of starting/stopping coordinators and presenting/dismissing > VC's, > stopping a coordinator in the middle of the coordinator stack should properly > dismiss all the VC's at that level and beyond. That's the problem, the ViewController was dismissed but the coordinator was never stopped. > > > > The problem is that presentViewController is not always called on the object > > handling the presentation. > > "The object on which you call this method may not always be the one that > handles > > the presentation." > > > https://developer.apple.com/reference/uikit/uiviewcontroller/1621380-presentv... > > One such example is if you call presentVC on a contained VC. The actual > presentingVC is the containerVC. > In this scenario, I believe we would see the correct behavior and the correct VC > would be dismissed. > > > > that this is taking it one level further than what's needed and dismissing > > I'm not quite understanding what is causing the dismissal to go "one level > further". > Can you clarify? That even though that we are calling presentViewController on the ToolbarVC presentVC doesn't guarantee that toolbarVC will handle the presentation. "If the current view controller is unable to fulfill a request, it forwards the request up the view controller hierarchy to its nearest parent, which can then handle or forward the request." So when you break on self.menuViewController.presentingViewController you can see that the presentingViewController is actually TabStripContainerVC. Which I don't think was the intention, I think that Louis intention was to call dismiss on ToolbarVC, that way the MenuVC and everything else that could've been presented would be dismissed. Since TabStripContainerVC is the presentingViewController, and you call dismissViewController on TabStripContainerVC (which is itself a presentedView) the following happens: "If you call this method on the presented view controller itself, UIKit asks the presenting view controller to handle the dismissal." https://developer.apple.com/reference/uikit/uiviewcontroller/1621505-dismissv... That means that TabGridVC will handle the dismissal but it will never stop its Coordinator. Since TabGridVC itself is doing the dismissal, not the TabGridCoordinator. I think there are a couple of ways to fix this: -Figure out why ToolbarVC is not the presentingView. (It could be since the presented MenuVC wont fit inside ToolbarVC bounds.) - Call dismiss on MenuVC itself (as this CL does). - Try to force TabContainerVC to be the presentingView maybe using setDefinesPresentationContext. Probably we should also discuss how to avoid this from happening in the future. A bad call to dismissVC can leave our coordinators in a "corrupted state".
On 2017/02/01 00:06:58, sczs wrote: > On 2017/01/31 23:29:20, edchin wrote: > > > I know Louis was trying to dismiss everything that was modally presented by > > the > > > VC, > > > that way we would dismiss all the Views that were presented by the VC and > not > > > just > > > the topmost View. > > > > If we keep a balance of starting/stopping coordinators and > presenting/dismissing > > VC's, > > stopping a coordinator in the middle of the coordinator stack should properly > > dismiss all the VC's at that level and beyond. > > That's the problem, the ViewController was dismissed but the coordinator was > never stopped. > > > > > > > > The problem is that presentViewController is not always called on the object > > > handling the presentation. > > > "The object on which you call this method may not always be the one that > > handles > > > the presentation." > > > > > > https://developer.apple.com/reference/uikit/uiviewcontroller/1621380-presentv... > > > > One such example is if you call presentVC on a contained VC. The actual > > presentingVC is the containerVC. > > In this scenario, I believe we would see the correct behavior and the correct > VC > > would be dismissed. > > > > > > > that this is taking it one level further than what's needed and dismissing > > > > I'm not quite understanding what is causing the dismissal to go "one level > > further". > > Can you clarify? > > That even though that we are calling presentViewController on the ToolbarVC > presentVC doesn't guarantee that toolbarVC will handle the presentation. > "If the current view controller is unable to fulfill a request, it forwards the > request up the view controller hierarchy to its nearest parent, which can > then handle or forward the request." > > So when you break on self.menuViewController.presentingViewController > you can see that the presentingViewController is actually > TabStripContainerVC. Which I don't think was the intention, I think that > Louis intention was to call dismiss on ToolbarVC, that way the MenuVC > and everything else that could've been presented would be dismissed. > > Since TabStripContainerVC is the presentingViewController, and you > call dismissViewController on TabStripContainerVC (which is > itself a presentedView) the following happens: > "If you call this method on the presented view controller itself, UIKit asks > the presenting view controller to handle the dismissal." > https://developer.apple.com/reference/uikit/uiviewcontroller/1621505-dismissv... > That means that TabGridVC will handle the dismissal but it will never > stop its Coordinator. Since TabGridVC itself is doing the dismissal, > not the TabGridCoordinator. > > I think there are a couple of ways to fix this: > -Figure out why ToolbarVC is not the presentingView. (It could be > since the presented MenuVC wont fit inside ToolbarVC bounds.) > - Call dismiss on MenuVC itself (as this CL does). > - Try to force TabContainerVC to be the presentingView > maybe using setDefinesPresentationContext. > > Probably we should also discuss how to avoid this from happening > in the future. A bad call to dismissVC can leave our coordinators > in a "corrupted state". When I was taking a look at the second issue of calling stop on the coordinator twice, I realize that the issue was "fixed"... I checked with Ed and he told me that calling dismiss on a presentedVC should only dismiss the presentedVC if its not presenting anything, which I now realize is correct. The reason why this was happening is because the stop method is called twice (which I thought it was a different issue we should discuss) so a double dismissal happens. closeToolsMenu in toolbar_coordinator will call stop on the toolsMenuCoordinator, and removing the childCoordinator will also call stop. So there's a double call to dismiss on TabStripContainerVC, and in the second call TabGrid dismisses TabStripContainerVC without stopping the TabStripCoordinator. I've already added this issue to tomorrow's agenda, so we can discuss then.
On 2017/02/01 00:43:59, sczs wrote: > On 2017/02/01 00:06:58, sczs wrote: > > On 2017/01/31 23:29:20, edchin wrote: > > > > I know Louis was trying to dismiss everything that was modally presented > by > > > the > > > > VC, > > > > that way we would dismiss all the Views that were presented by the VC and > > not > > > > just > > > > the topmost View. > > > > > > If we keep a balance of starting/stopping coordinators and > > presenting/dismissing > > > VC's, > > > stopping a coordinator in the middle of the coordinator stack should > properly > > > dismiss all the VC's at that level and beyond. > > > > That's the problem, the ViewController was dismissed but the coordinator was > > never stopped. > > > > > > > > > > > > The problem is that presentViewController is not always called on the > object > > > > handling the presentation. > > > > "The object on which you call this method may not always be the one that > > > handles > > > > the presentation." > > > > > > > > > > https://developer.apple.com/reference/uikit/uiviewcontroller/1621380-presentv... > > > > > > One such example is if you call presentVC on a contained VC. The actual > > > presentingVC is the containerVC. > > > In this scenario, I believe we would see the correct behavior and the > correct > > VC > > > would be dismissed. > > > > > > > > > > that this is taking it one level further than what's needed and dismissing > > > > > > I'm not quite understanding what is causing the dismissal to go "one level > > > further". > > > Can you clarify? > > > > That even though that we are calling presentViewController on the ToolbarVC > > presentVC doesn't guarantee that toolbarVC will handle the presentation. > > "If the current view controller is unable to fulfill a request, it forwards > the > > request up the view controller hierarchy to its nearest parent, which can > > then handle or forward the request." > > > > So when you break on self.menuViewController.presentingViewController > > you can see that the presentingViewController is actually > > TabStripContainerVC. Which I don't think was the intention, I think that > > Louis intention was to call dismiss on ToolbarVC, that way the MenuVC > > and everything else that could've been presented would be dismissed. > > > > Since TabStripContainerVC is the presentingViewController, and you > > call dismissViewController on TabStripContainerVC (which is > > itself a presentedView) the following happens: > > "If you call this method on the presented view controller itself, UIKit asks > > the presenting view controller to handle the dismissal." > > > https://developer.apple.com/reference/uikit/uiviewcontroller/1621505-dismissv... > > That means that TabGridVC will handle the dismissal but it will never > > stop its Coordinator. Since TabGridVC itself is doing the dismissal, > > not the TabGridCoordinator. > > > > I think there are a couple of ways to fix this: > > -Figure out why ToolbarVC is not the presentingView. (It could be > > since the presented MenuVC wont fit inside ToolbarVC bounds.) > > - Call dismiss on MenuVC itself (as this CL does). > > - Try to force TabContainerVC to be the presentingView > > maybe using setDefinesPresentationContext. > > > > Probably we should also discuss how to avoid this from happening > > in the future. A bad call to dismissVC can leave our coordinators > > in a "corrupted state". > > When I was taking a look at the second issue of calling stop on the > coordinator twice, I realize that the issue was "fixed"... I checked with > Ed and he told me that calling dismiss on a presentedVC should only > dismiss the presentedVC if its not presenting anything, which I now > realize is correct. The reason why this was happening is because the stop > method is called twice (which I thought it was a different issue > we should discuss) so a double dismissal happens. > > closeToolsMenu in toolbar_coordinator will call stop on the > toolsMenuCoordinator, and removing the childCoordinator > will also call stop. So there's a double call to dismiss on > TabStripContainerVC, and in the second call TabGrid dismisses > TabStripContainerVC without stopping the TabStripCoordinator. > > I've already added this issue to tomorrow's agenda, so we can discuss > then. Great, thanks for digging into this.
Description was changed from ========== [ios clean] Revert Tools Coordinator dismiss. The previous code caused that dismissing the ToolMenu would dismiss the whole Tab (Toolbar+content view) and left the App in a corrupted state. (The coordinators were not stopped but their VC was dismissed). We can go over this once Louis is back. But I'd like to revert this on the meantime. We should also think on how to prevent this on the future. Of course tests will help, but maybe something can be done with the coordinators themselves to prevent others from dismissing the view they presented, or to stop themselves if that happens? BUG= ========== to ========== [ios clean] Revert Tools Coordinator dismiss. The previous code caused that dismissing the ToolMenu would dismiss the whole Tab (Toolbar+content view) and left the App in a corrupted state. (The coordinators were not stopped but their VC was dismissed). BUG= ==========
PTAL
Description was changed from ========== [ios clean] Revert Tools Coordinator dismiss. The previous code caused that dismissing the ToolMenu would dismiss the whole Tab (Toolbar+content view) and left the App in a corrupted state. (The coordinators were not stopped but their VC was dismissed). BUG= ========== to ========== [ios clean] Remove stop on Browser coordinator dealloc The previous code caused that dismissing the ToolMenu would dismiss the whole Tab (Toolbar+content view) and left the App in a corrupted state. (The coordinators were not stopped but their VC was dismissed). This is a temporary fix. Marq will submit a more robust solution to this problem later on. BUG= ==========
https://codereview.chromium.org/2667873002/diff/40001/ios/clean/chrome/browse... File ios/clean/chrome/browser/browser_coordinator.mm (left): https://codereview.chromium.org/2667873002/diff/40001/ios/clean/chrome/browse... ios/clean/chrome/browser/browser_coordinator.mm:43: - (void)dealloc { I'm pretty sure you will need to update the comments in the header as well.
Completely right! https://codereview.chromium.org/2667873002/diff/40001/ios/clean/chrome/browse... File ios/clean/chrome/browser/browser_coordinator.mm (left): https://codereview.chromium.org/2667873002/diff/40001/ios/clean/chrome/browse... ios/clean/chrome/browser/browser_coordinator.mm:43: - (void)dealloc { On 2017/02/01 17:55:14, marq wrote: > I'm pretty sure you will need to update the comments in the header as well. Done.
lgtm
On 2017/02/02 08:31:12, marq wrote: > lgtm Sorry, not lgtm. This will fail unit tests! We have unit tests for this class. We should all get in the habit, when making changes to to classes like this with good test coverage, to first *change the test* to test for the new behavior, then run the tests, then change the implementation so that the tests pass.
On 2017/02/02 11:21:18, marq wrote: > On 2017/02/02 08:31:12, marq wrote: > > lgtm > > Sorry, not lgtm. > > This will fail unit tests! We have unit tests for this class. > > We should all get in the habit, when making changes to to classes like this with > good test coverage, to first *change the test* to test for the new behavior, > then run the tests, then change the implementation so that the tests pass. I've disabled the test now. Sorry about that, you're right about the habit. From now on I'll work on clean as if I were working on ios/chrome
LGTM. with minor comment below. https://codereview.chromium.org/2667873002/diff/80001/ios/clean/chrome/browse... File ios/clean/chrome/browser/browser_coordinator_unittest.mm (right): https://codereview.chromium.org/2667873002/diff/80001/ios/clean/chrome/browse... ios/clean/chrome/browser/browser_coordinator_unittest.mm:54: // BrowserCoordinator behaves on dealloc. I think it is best to remove this code entirely. You completely removed the actual dealloc code without any comments. So this code is confusing to someone who has no context.
https://codereview.chromium.org/2667873002/diff/80001/ios/clean/chrome/browse... File ios/clean/chrome/browser/browser_coordinator_unittest.mm (right): https://codereview.chromium.org/2667873002/diff/80001/ios/clean/chrome/browse... ios/clean/chrome/browser/browser_coordinator_unittest.mm:54: // BrowserCoordinator behaves on dealloc. On 2017/02/02 22:43:20, edchin wrote: > I think it is best to remove this code entirely. You completely removed the > actual dealloc code without any comments. So this code is confusing to someone > who has no context. I was thinking the exact same thing. But decide to disable it since we're not sure of the final behavior yet, Mark mentioned that we might still call stop on dealloc. If not this can become a Test to check that stop is NOT called on dealloc. The HACK: tag will hopefully give some context to people. In any case, I wouldn't mind removing it entirely and re-introducing it later if you feel strongly about it.
LGTM with the test change as described. https://codereview.chromium.org/2667873002/diff/80001/ios/clean/chrome/browse... File ios/clean/chrome/browser/browser_coordinator_unittest.mm (right): https://codereview.chromium.org/2667873002/diff/80001/ios/clean/chrome/browse... ios/clean/chrome/browser/browser_coordinator_unittest.mm:54: // BrowserCoordinator behaves on dealloc. On 2017/02/02 23:25:51, sczs wrote: > On 2017/02/02 22:43:20, edchin wrote: > > I think it is best to remove this code entirely. You completely removed the > > actual dealloc code without any comments. So this code is confusing to someone > > who has no context. > > I was thinking the exact same thing. But decide to disable it since we're not > sure of > the final behavior yet, Mark mentioned that we might still call stop on dealloc. > If not this can become a Test to check that stop is NOT called on dealloc. > > The HACK: tag will hopefully give some context to people. In any case, I > wouldn't > mind removing it entirely and re-introducing it later if you feel strongly about > it. I want to get out of the mindset of disabling tests, especially in a case like this where the behavioral change is intentional. For this change, it's probably correct just to change line 65 to EXPECT_FALSE. A comment that stop-on-dealloc used to be the behavior but isn't at the moment is fine, but I don't think it needs to be marked as a hack or placeholder.
https://codereview.chromium.org/2667873002/diff/80001/ios/clean/chrome/browse... File ios/clean/chrome/browser/browser_coordinator_unittest.mm (right): https://codereview.chromium.org/2667873002/diff/80001/ios/clean/chrome/browse... ios/clean/chrome/browser/browser_coordinator_unittest.mm:54: // BrowserCoordinator behaves on dealloc. On 2017/02/03 08:36:15, marq wrote: > On 2017/02/02 23:25:51, sczs wrote: > > On 2017/02/02 22:43:20, edchin wrote: > > > I think it is best to remove this code entirely. You completely removed the > > > actual dealloc code without any comments. So this code is confusing to > someone > > > who has no context. > > > > I was thinking the exact same thing. But decide to disable it since we're not > > sure of > > the final behavior yet, Mark mentioned that we might still call stop on > dealloc. > > If not this can become a Test to check that stop is NOT called on dealloc. > > > > The HACK: tag will hopefully give some context to people. In any case, I > > wouldn't > > mind removing it entirely and re-introducing it later if you feel strongly > about > > it. > > I want to get out of the mindset of disabling tests, especially in a case like > this where the behavioral change is intentional. > > For this change, it's probably correct just to change line 65 to EXPECT_FALSE. A > comment that stop-on-dealloc used to be the behavior but isn't at the moment is > fine, but I don't think it needs to be marked as a hack or placeholder. Done.
The CQ bit was checked by sczs@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from marq@chromium.org, edchin@chromium.org Link to the patchset: https://codereview.chromium.org/2667873002/#ps100001 (title: "CL Feedback")
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": 100001, "attempt_start_ts": 1486147904690010,
"parent_rev": "8abb3475d41c011a9f573ea65200e7f16a9a4496", "commit_rev":
"b367bcc4494b1b211268529557198d77d7cc0cb0"}
Message was sent while issue was closed.
Description was changed from ========== [ios clean] Remove stop on Browser coordinator dealloc The previous code caused that dismissing the ToolMenu would dismiss the whole Tab (Toolbar+content view) and left the App in a corrupted state. (The coordinators were not stopped but their VC was dismissed). This is a temporary fix. Marq will submit a more robust solution to this problem later on. BUG= ========== to ========== [ios clean] Remove stop on Browser coordinator dealloc The previous code caused that dismissing the ToolMenu would dismiss the whole Tab (Toolbar+content view) and left the App in a corrupted state. (The coordinators were not stopped but their VC was dismissed). This is a temporary fix. Marq will submit a more robust solution to this problem later on. BUG= Review-Url: https://codereview.chromium.org/2667873002 Cr-Commit-Position: refs/heads/master@{#448040} Committed: https://chromium.googlesource.com/chromium/src/+/b367bcc4494b1b21126852955719... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/b367bcc4494b1b21126852955719...
Message was sent while issue was closed.
Mark, we got the same issue with ChromeCoordinator in the current architecture: https://codereview.chromium.org/2701923003/diff/40001/ios/chrome/browser/paym... What was your idea for a more robust fix?
Message was sent while issue was closed.
On 2017/02/21 16:37:54, lpromero wrote: > Mark, we got the same issue with ChromeCoordinator in the current architecture: > https://codereview.chromium.org/2701923003/diff/40001/ios/chrome/browser/paym... > > What was your idea for a more robust fix? Use a presentation controller which will (presumably) get reliably informed about VC dismissals. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
