Child coordinators notify their parent upon -start and -stop.

Child coordinators notify their parent upon -start and -stop.

BUG=none
R=rohitrao@chromium.org

Review-Url: https://codereview.chromium.org/2734333003
Cr-Commit-Position: refs/heads/master@{#456072}
Committed: https://chromium.googlesource.com/chromium/src/+/f351604c9dd3faeedb53ac99e71c7594414bd19a

[lpromero, 2017-03-08 10:54:08]

The CQ bit was checked by lpromero@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-08 10:54:23]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-08 11:04:25]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-08 11:04:25]

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...)

[lpromero, 2017-03-08 11:46:44]

The CQ bit was checked by lpromero@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-08 11:46:54]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-08 11:55:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-08 11:55:21]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[lpromero, 2017-03-08 12:30:11]

The CQ bit was checked by lpromero@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-08 12:30:28]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[rohitrao (ping after 24h), 2017-03-08 13:01:03]

https://codereview.chromium.org/2734333003/diff/40001/ios/clean/chrome/browse...
File ios/clean/chrome/browser/browser_coordinator+internal.h (right):

https://codereview.chromium.org/2734333003/diff/40001/ios/clean/chrome/browse...
ios/clean/chrome/browser/browser_coordinator+internal.h:58: // Called when a
child coordinator did start. This is a blank template method.
We should eventually offer guidance on what to do with this method, once we
figure out the pattern =)

https://codereview.chromium.org/2734333003/diff/40001/ios/clean/chrome/browse...
File ios/clean/chrome/browser/browser_coordinator.h (right):

https://codereview.chromium.org/2734333003/diff/40001/ios/clean/chrome/browse...
ios/clean/chrome/browser/browser_coordinator.h:44: // called at the end of the
overriding implementation.
Called at the beginning, not the end?

https://codereview.chromium.org/2734333003/diff/40001/ios/clean/chrome/browse...
File ios/clean/chrome/browser/ui/tab/tab_coordinator.mm (right):

https://codereview.chromium.org/2734333003/diff/40001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/tab/tab_coordinator.mm:75:
self.viewController.contentViewController = webCoordinator.viewController;
These two lines are examples of code that should move into didStart, under the
proposed pattern.  If you make that change, how does it feel to you?

[lpromero, 2017-03-08 13:44:47]

The CQ bit was checked by lpromero@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-08 13:45:01]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[lpromero, 2017-03-08 13:45:41]

https://codereview.chromium.org/2734333003/diff/40001/ios/clean/chrome/browse...
File ios/clean/chrome/browser/browser_coordinator.h (right):

https://codereview.chromium.org/2734333003/diff/40001/ios/clean/chrome/browse...
ios/clean/chrome/browser/browser_coordinator.h:44: // called at the end of the
overriding implementation.
On 2017/03/08 13:01:03, rohitrao wrote:
> Called at the beginning, not the end?

Done.

https://codereview.chromium.org/2734333003/diff/40001/ios/clean/chrome/browse...
File ios/clean/chrome/browser/ui/tab/tab_coordinator.mm (right):

https://codereview.chromium.org/2734333003/diff/40001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/tab/tab_coordinator.mm:75:
self.viewController.contentViewController = webCoordinator.viewController;
On 2017/03/08 13:01:03, rohitrao wrote:
> These two lines are examples of code that should move into didStart, under the
> proposed pattern.  If you make that change, how does it feel to you?

Uploading a patch. But is clearly contrived.

https://codereview.chromium.org/2734333003/diff/40001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/tab/tab_coordinator.mm:75:
self.viewController.contentViewController = webCoordinator.viewController;
On 2017/03/08 13:01:03, rohitrao wrote:
> These two lines are examples of code that should move into didStart, under the
> proposed pattern.  If you make that change, how does it feel to you?

The goal (maybe not in this class, but generally speaking) is to have someone
else than the parent coordinator call -start, is that right?

[rohitrao (ping after 24h), 2017-03-08 13:50:23]

https://codereview.chromium.org/2734333003/diff/40001/ios/clean/chrome/browse...
File ios/clean/chrome/browser/ui/tab/tab_coordinator.mm (right):

https://codereview.chromium.org/2734333003/diff/40001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/tab/tab_coordinator.mm:75:
self.viewController.contentViewController = webCoordinator.viewController;
> The goal (maybe not in this class, but generally speaking) is to have someone
> else than the parent coordinator call -start, is that right?

The goal is to allow it, but that may not always happen. The toolbar is UI that
is always visible, so its parent coordinator will always immediately call
-start.  The find bar is only sometimes visible, so it is useful to have a
mechanism that allows the FindBarCoordinator to start itself, letting us stitch
together the view controllers on demand.

I expect that our UI will have a mix of the two cases.  I'd like to develop one
patterns that handles both.

[commit-bot: I haz the power, 2017-03-08 16:55:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-08 16:55:42]

Dry run: This issue passed the CQ dry run.

[lpromero, 2017-03-08 17:31:02]

What do you think of the implementation of didStart in the tab_coordinator?

[rohitrao (ping after 24h), 2017-03-08 19:01:45]

lgtm

This is what I was picturing, at a high level.  How do you feel about it?

https://codereview.chromium.org/2737563006 has a less-silly use case, in the
interaction between [FIPCoordinator start] and [TabCoordinator childDidStart].

https://codereview.chromium.org/2734333003/diff/60001/ios/clean/chrome/browse...
File ios/clean/chrome/browser/ui/tab/tab_coordinator.mm (right):

https://codereview.chromium.org/2734333003/diff/60001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/tab/tab_coordinator.mm:102: -
(void)childCoordinatorDidStart:(BrowserCoordinator*)coordinator {
Overall this lg.  We can discuss whether to use isKindOfClass or to hold
explicit pointers to the child coordinators, but that decision doesn't affect
the high-level flow.

[edchin, 2017-03-08 21:02:47]

edchin@chromium.org changed reviewers:
+ edchin@chromium.org

[edchin, 2017-03-08 21:02:48]

lgtm.

https://codereview.chromium.org/2734333003/diff/60001/ios/clean/chrome/browse...
File ios/clean/chrome/browser/browser_coordinator+internal.h (right):

https://codereview.chromium.org/2734333003/diff/60001/ios/clean/chrome/browse...
ios/clean/chrome/browser/browser_coordinator+internal.h:59: -
(void)childCoordinatorDidStart:(BrowserCoordinator*)coordinator;
Is |coordinator| the child coordinator? Can you add this to the comment?

https://codereview.chromium.org/2734333003/diff/60001/ios/clean/chrome/browse...
ios/clean/chrome/browser/browser_coordinator+internal.h:61: // Called when a
child coordinator will stop. This is a blank template method.
Can you add guidance as to when it is appropriate to override this in a
subclass? Perhaps clarify that overriding it is optional.

[lpromero, 2017-03-10 13:11:54]

The CQ bit was checked by lpromero@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-10 13:12:03]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[lpromero, 2017-03-10 13:14:55]

The CQ bit was checked by lpromero@chromium.org

[lpromero, 2017-03-10 13:14:56]

The patchset sent to the CQ was uploaded after l-g-t-m from edchin@chromium.org,
rohitrao@chromium.org
Link to the patchset: https://codereview.chromium.org/2734333003/#ps80001
(title: "Feedback")

[lpromero, 2017-03-10 13:15:02]

https://codereview.chromium.org/2734333003/diff/60001/ios/clean/chrome/browse...
File ios/clean/chrome/browser/browser_coordinator+internal.h (right):

https://codereview.chromium.org/2734333003/diff/60001/ios/clean/chrome/browse...
ios/clean/chrome/browser/browser_coordinator+internal.h:59: -
(void)childCoordinatorDidStart:(BrowserCoordinator*)coordinator;
On 2017/03/08 21:02:48, edchin wrote:
> Is |coordinator| the child coordinator? Can you add this to the comment?

Done.

https://codereview.chromium.org/2734333003/diff/60001/ios/clean/chrome/browse...
ios/clean/chrome/browser/browser_coordinator+internal.h:61: // Called when a
child coordinator will stop. This is a blank template method.
On 2017/03/08 21:02:48, edchin wrote:
> Can you add guidance as to when it is appropriate to override this in a
> subclass? Perhaps clarify that overriding it is optional.

Done.

[commit-bot: I haz the power, 2017-03-10 13:15:04]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-10 13:48:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-10 13:48:13]

Failed to apply patch for ios/clean/chrome/browser/browser_coordinator.h:
While running git apply --index -p1;
  error: patch failed: ios/clean/chrome/browser/browser_coordinator.h:32
  error: ios/clean/chrome/browser/browser_coordinator.h: patch does not apply

Patch:       ios/clean/chrome/browser/browser_coordinator.h
Index: ios/clean/chrome/browser/browser_coordinator.h
diff --git a/ios/clean/chrome/browser/browser_coordinator.h
b/ios/clean/chrome/browser/browser_coordinator.h
index
550c4dbdb4bafac3cf8c6f0e26d67e05a8820848..cf4bc5d198fad0fd234efdae2c29177fa28a3c2e
100644
--- a/ios/clean/chrome/browser/browser_coordinator.h
+++ b/ios/clean/chrome/browser/browser_coordinator.h
@@ -32,15 +32,19 @@ class ChromeBrowserState;
 @property(nonatomic, assign) ios::ChromeBrowserState* browserState;
 
 // The basic lifecycle methods for coordinators are -start and -stop. These
-// are blank template methods; child classes are expected to implement them and
-// do not need to invoke the superclass methods.
+// implementations only notify the parent coordinator when this coordinator did
+// start and will stop. Child classes are expected to override and call the
+// superclass method at the end of -start and at the beginning of -stop.
+
 // Starts the user interaction managed by the receiver. Typical implementations
 // will create a view controller and then use |baseViewController| to present
-// it.
-- (void)start;
+// it. This method needs to be called at the end of the overriding
+// implementation.
+- (void)start NS_REQUIRES_SUPER;
 
-// Stops the user interaction managed by the receiver.
-- (void)stop;
+// Stops the user interaction managed by the receiver. This method needs to be
+// called at the beginning of the overriding implementation.
+- (void)stop NS_REQUIRES_SUPER;
 
 @end

[lpromero, 2017-03-10 16:01:13]

The CQ bit was checked by lpromero@chromium.org to run a CQ dry run

[lpromero, 2017-03-10 16:01:23]

The CQ bit was checked by lpromero@chromium.org

[lpromero, 2017-03-10 16:01:24]

The patchset sent to the CQ was uploaded after l-g-t-m from edchin@chromium.org,
rohitrao@chromium.org
Link to the patchset: https://codereview.chromium.org/2734333003/#ps100001
(title: "Feedback")

[commit-bot: I haz the power, 2017-03-10 16:01:25]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-10 16:10:26]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1489161683815290,
"parent_rev": "ad5d52437c9f5cd1e412e0a6a845ad01c592cf88", "commit_rev":
"f351604c9dd3faeedb53ac99e71c7594414bd19a"}

[commit-bot: I haz the power, 2017-03-10 16:11:13]

Description was changed from

==========
Child coordinators notify their parent upon -start and -stop.

BUG=none
R=rohitrao@chromium.org
==========

to

==========
Child coordinators notify their parent upon -start and -stop.

BUG=none
R=rohitrao@chromium.org

Review-Url: https://codereview.chromium.org/2734333003
Cr-Commit-Position: refs/heads/master@{#456072}
Committed:
https://chromium.googlesource.com/chromium/src/+/f351604c9dd3faeedb53ac99e71c...
==========

[commit-bot: I haz the power, 2017-03-10 16:11:14]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/f351604c9dd3faeedb53ac99e71c...