Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2820703002:
Remove code in subclasses already performed by the base class. (Closed)
DescriptionRemove code in subclasses already performed by the base class.
This was introduced in https://codereview.chromium.org/2800313002/ while
the parent class added the code in
https://codereview.chromium.org/2812303003/.
BUG=none
R=marq@chromium.org,edchin@chromium.org
Review-Url: https://codereview.chromium.org/2820703002
Cr-Commit-Position: refs/heads/master@{#468977}
Committed: https://chromium.googlesource.com/chromium/src/+/d60ed8e2a67ef19c3cef5596938364dea1342b14
Patch Set 1 #
Total comments: 6
Messages
Total messages: 15 (7 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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2820703002/diff/1/ios/clean/chrome/browser/ui... File ios/clean/chrome/browser/ui/root/root_coordinator.mm (right): https://codereview.chromium.org/2820703002/diff/1/ios/clean/chrome/browser/ui... ios/clean/chrome/browser/ui/root/root_coordinator.mm:42: - (void)stop { can this whole method go away now? https://codereview.chromium.org/2820703002/diff/1/ios/clean/chrome/browser/ui... File ios/clean/chrome/browser/ui/tab_grid/tab_grid_coordinator.mm (right): https://codereview.chromium.org/2820703002/diff/1/ios/clean/chrome/browser/ui... ios/clean/chrome/browser/ui/tab_grid/tab_grid_coordinator.mm:91: for (BrowserCoordinator* child in self.children) { Update this for coordinator changes?
PTAL https://codereview.chromium.org/2820703002/diff/1/ios/clean/chrome/browser/ui... File ios/clean/chrome/browser/ui/root/root_coordinator.mm (right): https://codereview.chromium.org/2820703002/diff/1/ios/clean/chrome/browser/ui... ios/clean/chrome/browser/ui/root/root_coordinator.mm:42: - (void)stop { On 2017/04/18 12:13:26, marq (ping after 24h) wrote: > can this whole method go away now? No, to balance in stop the child coordinators additions from start. The parent class only removes all child coordinators in dealloc. https://codereview.chromium.org/2820703002/diff/1/ios/clean/chrome/browser/ui... File ios/clean/chrome/browser/ui/tab_grid/tab_grid_coordinator.mm (right): https://codereview.chromium.org/2820703002/diff/1/ios/clean/chrome/browser/ui... ios/clean/chrome/browser/ui/tab_grid/tab_grid_coordinator.mm:91: for (BrowserCoordinator* child in self.children) { On 2017/04/18 12:13:26, marq (ping after 24h) wrote: > Update this for coordinator changes? Was this comment the same as the previous one? Otherwise what did you mean by "coordinator changes"?
PTAL https://codereview.chromium.org/2820703002/diff/1/ios/clean/chrome/browser/ui... File ios/clean/chrome/browser/ui/root/root_coordinator.mm (right): https://codereview.chromium.org/2820703002/diff/1/ios/clean/chrome/browser/ui... ios/clean/chrome/browser/ui/root/root_coordinator.mm:42: - (void)stop { On 2017/04/18 12:13:26, marq (ping after 24h) wrote: > can this whole method go away now? No, to balance in stop the child coordinators additions from start. The parent class only removes all child coordinators in dealloc. https://codereview.chromium.org/2820703002/diff/1/ios/clean/chrome/browser/ui... File ios/clean/chrome/browser/ui/tab_grid/tab_grid_coordinator.mm (right): https://codereview.chromium.org/2820703002/diff/1/ios/clean/chrome/browser/ui... ios/clean/chrome/browser/ui/tab_grid/tab_grid_coordinator.mm:91: for (BrowserCoordinator* child in self.children) { On 2017/04/18 12:13:26, marq (ping after 24h) wrote: > Update this for coordinator changes? Was this comment the same as the previous one? Otherwise what did you mean by "coordinator changes"?
lgtm https://codereview.chromium.org/2820703002/diff/1/ios/clean/chrome/browser/ui... File ios/clean/chrome/browser/ui/tab_grid/tab_grid_coordinator.mm (right): https://codereview.chromium.org/2820703002/diff/1/ios/clean/chrome/browser/ui... ios/clean/chrome/browser/ui/tab_grid/tab_grid_coordinator.mm:91: for (BrowserCoordinator* child in self.children) { On 2017/05/03 13:13:23, lpromero wrote: > On 2017/04/18 12:13:26, marq (ping after 24h) wrote: > > Update this for coordinator changes? > > Was this comment the same as the previous one? Otherwise what did you mean by > "coordinator changes"? I thought we were handling this (removing children) in the base class's -stop method. I was probably confused.
Thanks~ https://codereview.chromium.org/2820703002/diff/1/ios/clean/chrome/browser/ui... File ios/clean/chrome/browser/ui/tab_grid/tab_grid_coordinator.mm (right): https://codereview.chromium.org/2820703002/diff/1/ios/clean/chrome/browser/ui... ios/clean/chrome/browser/ui/tab_grid/tab_grid_coordinator.mm:91: for (BrowserCoordinator* child in self.children) { On 2017/05/03 13:17:49, marq (ping after 24h) wrote: > On 2017/05/03 13:13:23, lpromero wrote: > > On 2017/04/18 12:13:26, marq (ping after 24h) wrote: > > > Update this for coordinator changes? > > > > Was this comment the same as the previous one? Otherwise what did you mean by > > "coordinator changes"? > > I thought we were handling this (removing children) in the base class's -stop > method. I was probably confused. We stop childrens in the parent's stop, but we only remove children from the parent's dealloc.
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": 1, "attempt_start_ts": 1493824279029750, "parent_rev":
"5aa19d360acef358288a23b8e9b69d1292bafc0a", "commit_rev":
"d60ed8e2a67ef19c3cef5596938364dea1342b14"}
Message was sent while issue was closed.
Description was changed from ========== Remove code in subclasses already performed by the base class. This was introduced in https://codereview.chromium.org/2800313002/ while the parent class added the code in https://codereview.chromium.org/2812303003/. BUG=none R=marq@chromium.org,edchin@chromium.org ========== to ========== Remove code in subclasses already performed by the base class. This was introduced in https://codereview.chromium.org/2800313002/ while the parent class added the code in https://codereview.chromium.org/2812303003/. BUG=none R=marq@chromium.org,edchin@chromium.org Review-Url: https://codereview.chromium.org/2820703002 Cr-Commit-Position: refs/heads/master@{#468977} Committed: https://chromium.googlesource.com/chromium/src/+/d60ed8e2a67ef19c3cef55969383... ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1) as https://chromium.googlesource.com/chromium/src/+/d60ed8e2a67ef19c3cef55969383... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
