Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2812303003:
BrowserCoordinator stops its children. (Closed)
|
Created:
3 years, 8 months ago by lpromero Modified:
3 years, 8 months ago Reviewers:
marq (ping after 24h) CC:
chromium-reviews, marq+scrutinize_chromium.org, lpromero+watch_chromium.org, ios-reviews+clean_chromium.org, ios-reviews_chromium.org, edchin, justincohen, rohitrao (ping after 24h), sczs Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionBrowserCoordinator stops its children.
BUG=none
R=marq@chromium.org
CC=rohitrao@chromium.org,sczs@chromium.org,edchin@chromium.org,justincohen@chromium.org
Review-Url: https://codereview.chromium.org/2812303003
Cr-Commit-Position: refs/heads/master@{#464446}
Committed: https://chromium.googlesource.com/chromium/src/+/0cb3058fb201e904741ba2bea76f643f02e36b28
Patch Set 1 #
Total comments: 4
Patch Set 2 : Fix tests #Patch Set 3 : Rebased #
Depends on Patchset: Dependent Patchsets: Messages
Total messages: 29 (17 generated)
The CQ bit was checked by lpromero@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...
https://codereview.chromium.org/2812303003/diff/1/ios/shared/chrome/browser/u... File ios/shared/chrome/browser/ui/coordinators/browser_coordinator.mm (right): https://codereview.chromium.org/2812303003/diff/1/ios/shared/chrome/browser/u... ios/shared/chrome/browser/ui/coordinators/browser_coordinator.mm:49: for (BrowserCoordinator* child in self.children) { Actually, it might need to stop only the started children.
https://codereview.chromium.org/2812303003/diff/1/ios/shared/chrome/browser/u... File ios/shared/chrome/browser/ui/coordinators/browser_coordinator.mm (right): https://codereview.chromium.org/2812303003/diff/1/ios/shared/chrome/browser/u... ios/shared/chrome/browser/ui/coordinators/browser_coordinator.mm:49: for (BrowserCoordinator* child in self.children) { On 2017/04/12 17:10:03, lpromero wrote: > Actually, it might need to stop only the started children. Every time I see "if (self.started) { [self stop] }" I want to make -stop a no-op for unstarted coordinators. But we aren't set up for that. Should the first line of this method just be: if (!self.started) return; ? (and similarly for -stop).
The CQ bit was unchecked by commit-bot@chromium.org
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...)
The CQ bit was checked by lpromero@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...
https://codereview.chromium.org/2812303003/diff/1/ios/shared/chrome/browser/u... File ios/shared/chrome/browser/ui/coordinators/browser_coordinator.mm (right): https://codereview.chromium.org/2812303003/diff/1/ios/shared/chrome/browser/u... ios/shared/chrome/browser/ui/coordinators/browser_coordinator.mm:49: for (BrowserCoordinator* child in self.children) { On 2017/04/12 17:13:13, marq wrote: > On 2017/04/12 17:10:03, lpromero wrote: > > Actually, it might need to stop only the started children. > > Every time I see "if (self.started) { [self stop] }" I want to make -stop a > no-op for unstarted coordinators. But we aren't set up for that. > > Should the first line of this method just be: > > if (!self.started) return; > > ? > > (and similarly for -stop). Maybe. I make a followup CL.
https://codereview.chromium.org/2812303003/diff/1/ios/shared/chrome/browser/u... File ios/shared/chrome/browser/ui/coordinators/browser_coordinator.mm (right): https://codereview.chromium.org/2812303003/diff/1/ios/shared/chrome/browser/u... ios/shared/chrome/browser/ui/coordinators/browser_coordinator.mm:49: for (BrowserCoordinator* child in self.children) { On 2017/04/12 17:25:15, lpromero wrote: > On 2017/04/12 17:13:13, marq wrote: > > On 2017/04/12 17:10:03, lpromero wrote: > > > Actually, it might need to stop only the started children. > > > > Every time I see "if (self.started) { [self stop] }" I want to make -stop a > > no-op for unstarted coordinators. But we aren't set up for that. > > > > Should the first line of this method just be: > > > > if (!self.started) return; > > > > ? > > > > (and similarly for -stop). > > Maybe. I make a followup CL. Actually, I am starting (…) to think that we should instead DCHECK. If we want to no-op, we will have to no-op in all subclasses implementations of start/stop.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by lpromero@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/04/12 17:28:24, lpromero wrote: > https://codereview.chromium.org/2812303003/diff/1/ios/shared/chrome/browser/u... > File ios/shared/chrome/browser/ui/coordinators/browser_coordinator.mm (right): > > https://codereview.chromium.org/2812303003/diff/1/ios/shared/chrome/browser/u... > ios/shared/chrome/browser/ui/coordinators/browser_coordinator.mm:49: for > (BrowserCoordinator* child in self.children) { > On 2017/04/12 17:25:15, lpromero wrote: > > On 2017/04/12 17:13:13, marq wrote: > > > On 2017/04/12 17:10:03, lpromero wrote: > > > > Actually, it might need to stop only the started children. > > > > > > Every time I see "if (self.started) { [self stop] }" I want to make -stop a > > > no-op for unstarted coordinators. But we aren't set up for that. > > > > > > Should the first line of this method just be: > > > > > > if (!self.started) return; > > > > > > ? > > > > > > (and similarly for -stop). > > > > Maybe. I make a followup CL. > > Actually, I am starting (…) to think that we should instead DCHECK. If we want > to no-op, we will have to no-op in all subclasses implementations of start/stop. *and* they will still have to call super as well as no-opping.
On 2017/04/13 09:54:48, marq wrote: > On 2017/04/12 17:28:24, lpromero wrote: > > > https://codereview.chromium.org/2812303003/diff/1/ios/shared/chrome/browser/u... > > File ios/shared/chrome/browser/ui/coordinators/browser_coordinator.mm (right): > > > > > https://codereview.chromium.org/2812303003/diff/1/ios/shared/chrome/browser/u... > > ios/shared/chrome/browser/ui/coordinators/browser_coordinator.mm:49: for > > (BrowserCoordinator* child in self.children) { > > On 2017/04/12 17:25:15, lpromero wrote: > > > On 2017/04/12 17:13:13, marq wrote: > > > > On 2017/04/12 17:10:03, lpromero wrote: > > > > > Actually, it might need to stop only the started children. > > > > > > > > Every time I see "if (self.started) { [self stop] }" I want to make -stop > a > > > > no-op for unstarted coordinators. But we aren't set up for that. > > > > > > > > Should the first line of this method just be: > > > > > > > > if (!self.started) return; > > > > > > > > ? > > > > > > > > (and similarly for -stop). > > > > > > Maybe. I make a followup CL. > > > > Actually, I am starting (…) to think that we should instead DCHECK. If we want > > to no-op, we will have to no-op in all subclasses implementations of > start/stop. > > *and* they will still have to call super as well as no-opping. Maybe not as NS_REQUIRES_SUPER doesn't seem really smart: This triggers a warning: "Method possibly missing a [super foo] call" - (void)foo { } While this builds totally fine: - (void)foo { if (NO) return [super foo]; } So the compiler doesn't really guarantee a call to super, just that it needs to be present at some point in the method! :S So what do you advise? DCHECK or string of no-op?
On 2017/04/13 13:10:09, lpromero wrote: > On 2017/04/13 09:54:48, marq wrote: > > On 2017/04/12 17:28:24, lpromero wrote: > > > > > > https://codereview.chromium.org/2812303003/diff/1/ios/shared/chrome/browser/u... > > > File ios/shared/chrome/browser/ui/coordinators/browser_coordinator.mm > (right): > > > > > > > > > https://codereview.chromium.org/2812303003/diff/1/ios/shared/chrome/browser/u... > > > ios/shared/chrome/browser/ui/coordinators/browser_coordinator.mm:49: for > > > (BrowserCoordinator* child in self.children) { > > > On 2017/04/12 17:25:15, lpromero wrote: > > > > On 2017/04/12 17:13:13, marq wrote: > > > > > On 2017/04/12 17:10:03, lpromero wrote: > > > > > > Actually, it might need to stop only the started children. > > > > > > > > > > Every time I see "if (self.started) { [self stop] }" I want to make > -stop > > a > > > > > no-op for unstarted coordinators. But we aren't set up for that. > > > > > > > > > > Should the first line of this method just be: > > > > > > > > > > if (!self.started) return; > > > > > > > > > > ? > > > > > > > > > > (and similarly for -stop). > > > > > > > > Maybe. I make a followup CL. > > > > > > Actually, I am starting (…) to think that we should instead DCHECK. If we > want > > > to no-op, we will have to no-op in all subclasses implementations of > > start/stop. > > > > *and* they will still have to call super as well as no-opping. > > Maybe not as NS_REQUIRES_SUPER doesn't seem really smart: > > This triggers a warning: "Method possibly missing a [super foo] call" > - (void)foo { > } > > While this builds totally fine: > - (void)foo { > if (NO) return [super foo]; > } > > So the compiler doesn't really guarantee a call to super, just that it needs to > be present at some point in the method! :S > > > So what do you advise? DCHECK or string of no-op? Maybe no-ops in the base class and let subclasses do whatever they want, provided they call super -- with the understanding that the super call won't do anything if the coordinator is stopped/started. I think that gets us the most boilerplate reduction with the least amount of change to the API.
lgtm
On 2017/04/13 14:59:57, marq wrote: > On 2017/04/13 13:10:09, lpromero wrote: > > On 2017/04/13 09:54:48, marq wrote: > > > On 2017/04/12 17:28:24, lpromero wrote: > > > > > > > > > > https://codereview.chromium.org/2812303003/diff/1/ios/shared/chrome/browser/u... > > > > File ios/shared/chrome/browser/ui/coordinators/browser_coordinator.mm > > (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2812303003/diff/1/ios/shared/chrome/browser/u... > > > > ios/shared/chrome/browser/ui/coordinators/browser_coordinator.mm:49: for > > > > (BrowserCoordinator* child in self.children) { > > > > On 2017/04/12 17:25:15, lpromero wrote: > > > > > On 2017/04/12 17:13:13, marq wrote: > > > > > > On 2017/04/12 17:10:03, lpromero wrote: > > > > > > > Actually, it might need to stop only the started children. > > > > > > > > > > > > Every time I see "if (self.started) { [self stop] }" I want to make > > -stop > > > a > > > > > > no-op for unstarted coordinators. But we aren't set up for that. > > > > > > > > > > > > Should the first line of this method just be: > > > > > > > > > > > > if (!self.started) return; > > > > > > > > > > > > ? > > > > > > > > > > > > (and similarly for -stop). > > > > > > > > > > Maybe. I make a followup CL. > > > > > > > > Actually, I am starting (…) to think that we should instead DCHECK. If we > > want > > > > to no-op, we will have to no-op in all subclasses implementations of > > > start/stop. > > > > > > *and* they will still have to call super as well as no-opping. > > > > Maybe not as NS_REQUIRES_SUPER doesn't seem really smart: > > > > This triggers a warning: "Method possibly missing a [super foo] call" > > - (void)foo { > > } > > > > While this builds totally fine: > > - (void)foo { > > if (NO) return [super foo]; > > } > > > > So the compiler doesn't really guarantee a call to super, just that it needs > to > > be present at some point in the method! :S > > > > > > So what do you advise? DCHECK or string of no-op? > > Maybe no-ops in the base class and let subclasses do whatever they want, > provided they call super -- with the understanding that the super call won't do > anything if the coordinator is stopped/started. I think that gets us the most > boilerplate reduction with the least amount of change to the API. https://codereview.chromium.org/2811973004/ it is. Thanks!
The CQ bit was checked by lpromero@chromium.org
The CQ bit was unchecked by lpromero@chromium.org
The CQ bit was checked by lpromero@chromium.org
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": 1492102563345690,
"parent_rev": "b20672f0a5080fe4318e049bd408db1a01105904", "commit_rev":
"0cb3058fb201e904741ba2bea76f643f02e36b28"}
Message was sent while issue was closed.
Description was changed from ========== BrowserCoordinator stops its children. BUG=none R=marq@chromium.org CC=rohitrao@chromium.org,sczs@chromium.org,edchin@chromium.org,justincohen@ch... ========== to ========== BrowserCoordinator stops its children. BUG=none R=marq@chromium.org CC=rohitrao@chromium.org,sczs@chromium.org,edchin@chromium.org,justincohen@ch... Review-Url: https://codereview.chromium.org/2812303003 Cr-Commit-Position: refs/heads/master@{#464446} Committed: https://chromium.googlesource.com/chromium/src/+/0cb3058fb201e904741ba2bea76f... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/0cb3058fb201e904741ba2bea76f... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
