Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2814713002:
Add a bottom toolbar setting. (Closed)
|
Created:
3 years, 8 months ago by lpromero Modified:
3 years, 7 months ago Reviewers:
marq (ping after 24h) CC:
chromium-reviews, marq+scrutinize_chromium.org, ios-reviews+chrome_chromium.org, ios-reviews_chromium.org, pkl (ping after 24h if needed), noyau+watch_chromium.org, arv+watch_chromium.org, ios-reviews+clean_chromium.org, marq+watch_chromium.org, lpromero+watch_chromium.org, sdefresne+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd a bottom toolbar setting.
BUG=none
R=marq@chromium.org
Review-Url: https://codereview.chromium.org/2814713002
Cr-Commit-Position: refs/heads/master@{#468966}
Committed: https://chromium.googlesource.com/chromium/src/+/0b33b3200cfccc6453b9b7c5e29600369dac0f5a
Patch Set 1 #Patch Set 2 : Rebase #Patch Set 3 : Rebased #Patch Set 4 : Reupload with a normal diff #Patch Set 5 : With tests #
Total comments: 5
Patch Set 6 : Rebased #
Messages
Total messages: 31 (20 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: 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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
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: 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-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
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.
lgtm
PTAL. I must have done ~10 CLs just to unblock this one :p https://codereview.chromium.org/2814713002/diff/80001/ios/clean/chrome/browse... File ios/clean/chrome/browser/ui/tab/tab_coordinator.mm (right): https://codereview.chromium.org/2814713002/diff/80001/ios/clean/chrome/browse... ios/clean/chrome/browser/ui/tab/tab_coordinator.mm:97: [self removeChildCoordinator:child]; This is necessary to balance the addChildCoordinator calls from -start. In the tests, not doing this means too much stuff lingers.
lgtm https://codereview.chromium.org/2814713002/diff/80001/ios/clean/chrome/browse... File ios/clean/chrome/browser/ui/tab/tab_coordinator.mm (right): https://codereview.chromium.org/2814713002/diff/80001/ios/clean/chrome/browse... ios/clean/chrome/browser/ui/tab/tab_coordinator.mm:97: [self removeChildCoordinator:child]; On 2017/04/21 08:08:34, lpromero wrote: > This is necessary to balance the addChildCoordinator calls from -start. In the > tests, not doing this means too much stuff lingers. This goes away with crrev.com/2832473003, right?
https://codereview.chromium.org/2814713002/diff/80001/ios/clean/chrome/browse... File ios/clean/chrome/browser/ui/tab/tab_coordinator.mm (right): https://codereview.chromium.org/2814713002/diff/80001/ios/clean/chrome/browse... ios/clean/chrome/browser/ui/tab/tab_coordinator.mm:97: [self removeChildCoordinator:child]; On 2017/04/21 08:26:32, marq wrote: > On 2017/04/21 08:08:34, lpromero wrote: > > This is necessary to balance the addChildCoordinator calls from -start. In the > > tests, not doing this means too much stuff lingers. > > This goes away with crrev.com/2832473003, right? In tests, this coordinator is never removed (because never added). I see wasRemoved as a trap-all, for cases not handled by balanced calls like this one. WDYT? The other solution is to have the TabCoordinatorTest class remove the children in TearDown.
https://codereview.chromium.org/2814713002/diff/80001/ios/clean/chrome/browse... File ios/clean/chrome/browser/ui/tab/tab_coordinator.mm (right): https://codereview.chromium.org/2814713002/diff/80001/ios/clean/chrome/browse... ios/clean/chrome/browser/ui/tab/tab_coordinator.mm:97: [self removeChildCoordinator:child]; On 2017/04/21 09:00:26, lpromero wrote: > On 2017/04/21 08:26:32, marq wrote: > > On 2017/04/21 08:08:34, lpromero wrote: > > > This is necessary to balance the addChildCoordinator calls from -start. In > the > > > tests, not doing this means too much stuff lingers. > > > > This goes away with crrev.com/2832473003, right? > > In tests, this coordinator is never removed (because never added). > I see wasRemoved as a trap-all, for cases not handled by balanced calls like > this one. > WDYT? The other solution is to have the TabCoordinatorTest class remove the > children in TearDown. Kindly ping
https://codereview.chromium.org/2814713002/diff/80001/ios/clean/chrome/browse... File ios/clean/chrome/browser/ui/tab/tab_coordinator.mm (right): https://codereview.chromium.org/2814713002/diff/80001/ios/clean/chrome/browse... ios/clean/chrome/browser/ui/tab/tab_coordinator.mm:97: [self removeChildCoordinator:child]; On 2017/04/26 17:59:19, lpromero wrote: > On 2017/04/21 09:00:26, lpromero wrote: > > On 2017/04/21 08:26:32, marq wrote: > > > On 2017/04/21 08:08:34, lpromero wrote: > > > > This is necessary to balance the addChildCoordinator calls from -start. In > > the > > > > tests, not doing this means too much stuff lingers. > > > > > > This goes away with crrev.com/2832473003, right? > > > > In tests, this coordinator is never removed (because never added). > > I see wasRemoved as a trap-all, for cases not handled by balanced calls like > > this one. > > WDYT? The other solution is to have the TabCoordinatorTest class remove the > > children in TearDown. > > Kindly ping Ping :)
On 2017/05/03 09:15:39, lpromero wrote: > https://codereview.chromium.org/2814713002/diff/80001/ios/clean/chrome/browse... > File ios/clean/chrome/browser/ui/tab/tab_coordinator.mm (right): > > https://codereview.chromium.org/2814713002/diff/80001/ios/clean/chrome/browse... > ios/clean/chrome/browser/ui/tab/tab_coordinator.mm:97: [self > removeChildCoordinator:child]; > On 2017/04/26 17:59:19, lpromero wrote: > > On 2017/04/21 09:00:26, lpromero wrote: > > > On 2017/04/21 08:26:32, marq wrote: > > > > On 2017/04/21 08:08:34, lpromero wrote: > > > > > This is necessary to balance the addChildCoordinator calls from -start. > In > > > the > > > > > tests, not doing this means too much stuff lingers. > > > > > > > > This goes away with crrev.com/2832473003, right? > > > > > > In tests, this coordinator is never removed (because never added). > > > I see wasRemoved as a trap-all, for cases not handled by balanced calls like > > > this one. > > > WDYT? The other solution is to have the TabCoordinatorTest class remove the > > > children in TearDown. > > > > Kindly ping > > Ping :) marq: ping
Sorry, lost this one. LGTM. (feature Idea: if I've LGTM'd and you want me to re-LGTM, you should be able to remove my LGTM).
The CQ bit was checked by lpromero@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from marq@chromium.org Link to the patchset: https://codereview.chromium.org/2814713002/#ps100001 (title: "Rebased")
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": 1493815427377950,
"parent_rev": "e6e2d301f7a892c62098da14c7f47eec0aad40f8", "commit_rev":
"0b33b3200cfccc6453b9b7c5e29600369dac0f5a"}
Message was sent while issue was closed.
Description was changed from ========== Add a bottom toolbar setting. BUG=none R=marq@chromium.org ========== to ========== Add a bottom toolbar setting. BUG=none R=marq@chromium.org Review-Url: https://codereview.chromium.org/2814713002 Cr-Commit-Position: refs/heads/master@{#468966} Committed: https://chromium.googlesource.com/chromium/src/+/0b33b3200cfccc6453b9b7c5e296... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/0b33b3200cfccc6453b9b7c5e296... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
