Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2475913003:
Enabling kUseConsolidatedStartupFlow by default on trunk. (Closed)
DescriptionEnabling kUseConsolidatedStartupFlow by default on trunk.
Code changes:
- Disabling onboarding content for ChromeOS. Promotions for CrOS are out-of-scope for this launch.
- Ensuring that preference-specified tabs do not appear in new windows when a tabbed browser already exists for the profile.
Test changes:
- Removed 12 outdated tests
- Fixed or updated 9 tests
Details of affected tests, and rationale, can be found in this doc: https://docs.google.com/a/google.com/document/d/1PjAemEEFrl0gIDVQfnr1oAnHkb6DcV47HlOyyo7ICZI/edit?usp=sharing
BUG=608875, 314819, 313856
Review-Url: https://codereview.chromium.org/2475913003
Cr-Original-Commit-Position: refs/heads/master@{#442487}
Committed: https://chromium.googlesource.com/chromium/src/+/a52322134679a349366417e04e502bc8ce1f68e4
Review-Url: https://codereview.chromium.org/2475913003
Cr-Commit-Position: refs/heads/master@{#442712}
Committed: https://chromium.googlesource.com/chromium/src/+/82e5578d15c0661c3306eebdd12bd7287f35cdf6
Patch Set 1 #Patch Set 2 : Second try at browser tests #Patch Set 3 : All tests hopefully fixed #Patch Set 4 : Stragglers #Patch Set 5 : Mac test, build issues #Patch Set 6 : Trying to fix triggered reset #Patch Set 7 : Temporarily adding logging for remote debugging #Patch Set 8 : Removing CrOS onboarding; adding additional logging to debug Win and Mac issues #Patch Set 9 : Using proper SetUp methods #Patch Set 10 : Adding override tag #Patch Set 11 : Pointer typo #Patch Set 12 : Removing LOGs and DCHECKs #Patch Set 13 : Fixing overrides #Patch Set 14 : Expect Welcome on Mac tests #Patch Set 15 : Alternate Tab Closure #Patch Set 16 : Using navigation instead of closing tab on Mac #Patch Set 17 : Rebase #Patch Set 18 : Deref typo #Patch Set 19 : Controller issue #Patch Set 20 : Adding Navigation Observer #
Total comments: 4
Patch Set 21 : Addressing pkasting comments #Patch Set 22 : Corrected check for tabbed browser #
Total comments: 3
Patch Set 23 : Final nit #Patch Set 24 : [Reland] Enabling kUseConsolidatedStartupFlow by default on trunk #Patch Set 25 : Reintroducing Win10 guard from original code #Patch Set 26 : Rebase-updated #Messages
Total messages: 106 (95 generated)
The CQ bit was checked by tmartino@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: 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 tmartino@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_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by tmartino@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: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by tmartino@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: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by tmartino@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: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by tmartino@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: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by tmartino@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_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by tmartino@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: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by tmartino@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 checked by tmartino@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 checked by tmartino@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...
Description was changed from ========== Enabling kUseConsolidatedStartupFlow by default on trunk. BUG=608875 ========== to ========== Enabling kUseConsolidatedStartupFlow by default on trunk. Code changes: - Disabling onboarding content for ChromeOS. Promotions for CrOS are out-of-scope for this launch. Test changes: - Removed 13 outdated tests - Fixed or updated 9 tests Details of affected tests, and rationale, can be found in this doc: https://docs.google.com/a/google.com/document/d/1PjAemEEFrl0gIDVQfnr1oAnHkb6D... BUG=608875,314819,313856 ==========
The CQ bit was checked by tmartino@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: 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 tmartino@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: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by tmartino@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: 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 tmartino@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: 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 tmartino@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 on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) 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 tmartino@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 checked by tmartino@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: 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 tmartino@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.
tmartino@chromium.org changed reviewers: + pkasting@chromium.org, rsesek@chromium.org
+pkasting for OWNERS on c/b/ui/* +rsesek for OWNERS on app_controller_mac_browsertest.mm
https://codereview.chromium.org/2475913003/diff/380001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_browser_creator_browsertest.cc (left): https://codereview.chromium.org/2475913003/diff/380001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_browsertest.cc:311: // new browser. Your doc says this test was removed because it's enforcing a de facto policy rather than actual design. Are you saying that in the consolidated flow, launching Chrome via a shortcut, while Chrome is running, will load the user's normal startup URLs in the new window? If so, this behavior change seems bad. Startup URLs are intended to be "cold start"/"session start", and the meaning of launching from a shortcut/command-line while the session is running is "add to this session". Showing these URLs would be wrong. So if the behavior is changing, then please fix that. If the behavior is not changing, but you're concerned that this tests checks something that's not part of our formal contract, then let's specify it as part of our formal contract. https://codereview.chromium.org/2475913003/diff/380001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_browser_creator_browsertest.cc (right): https://codereview.chromium.org/2475913003/diff/380001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_browsertest.cc:638: // Disarm the Welcome page on all four profiles. Nit: See comments on "disarm" in next file (2 places) https://codereview.chromium.org/2475913003/diff/380001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_browser_creator_triggered_reset_browsertest_win.cc (right): https://codereview.chromium.org/2475913003/diff/380001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_triggered_reset_browsertest_win.cc:114: // Disarm the Welcome page. Nit: "Disarm" is a little unclear (is it going to blow up Chrome?). Maybe you just want "Avoid showing" or the like? (2 places) https://codereview.chromium.org/2475913003/diff/380001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_tab_provider.cc (right): https://codereview.chromium.org/2475913003/diff/380001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_tab_provider.cc:29: #if defined(OS_CHROMEOS) Nit: Does this deserve an explanatory comment?
app_controller_mac_browsertest.mm LGTM
The CQ bit was checked by tmartino@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_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by tmartino@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.
Description was changed from ========== Enabling kUseConsolidatedStartupFlow by default on trunk. Code changes: - Disabling onboarding content for ChromeOS. Promotions for CrOS are out-of-scope for this launch. Test changes: - Removed 13 outdated tests - Fixed or updated 9 tests Details of affected tests, and rationale, can be found in this doc: https://docs.google.com/a/google.com/document/d/1PjAemEEFrl0gIDVQfnr1oAnHkb6D... BUG=608875,314819,313856 ========== to ========== Enabling kUseConsolidatedStartupFlow by default on trunk. Code changes: - Disabling onboarding content for ChromeOS. Promotions for CrOS are out-of-scope for this launch. - Ensuring that preference-specified tabs do not appear in new windows when a tabbed browser already exists for the profile. Test changes: - Removed 12 outdated tests - Fixed or updated 9 tests Details of affected tests, and rationale, can be found in this doc: https://docs.google.com/a/google.com/document/d/1PjAemEEFrl0gIDVQfnr1oAnHkb6D... BUG=608875,314819,313856 ==========
Added a small code fix to reverse the behavior change, and addressed the documentation nits. PTAL
LGTM https://codereview.chromium.org/2475913003/diff/420001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_browser_creator_browsertest.cc (right): https://codereview.chromium.org/2475913003/diff/420001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_browsertest.cc:351: if (!IsWindows10OrNewer()) { Nit: Seems like maybe this should be combined with the previous conditional. https://codereview.chromium.org/2475913003/diff/420001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_tab_provider.cc (right): https://codereview.chromium.org/2475913003/diff/420001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_tab_provider.cc:104: } Nit: Might be possible to rewrite this more briefly as find_if() with a lambda (which would also be a little more idiomatic as "I'm looking for a specific item in this list").
The CQ bit was checked by tmartino@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: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by tmartino@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rsesek@chromium.org, pkasting@chromium.org Link to the patchset: https://codereview.chromium.org/2475913003/#ps440001 (title: "Final nit")
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": 440001, "attempt_start_ts": 1484019570789530,
"parent_rev": "4847079a1779a4f643dafca0c5da6425561a378e", "commit_rev":
"a52322134679a349366417e04e502bc8ce1f68e4"}
Message was sent while issue was closed.
Description was changed from ========== Enabling kUseConsolidatedStartupFlow by default on trunk. Code changes: - Disabling onboarding content for ChromeOS. Promotions for CrOS are out-of-scope for this launch. - Ensuring that preference-specified tabs do not appear in new windows when a tabbed browser already exists for the profile. Test changes: - Removed 12 outdated tests - Fixed or updated 9 tests Details of affected tests, and rationale, can be found in this doc: https://docs.google.com/a/google.com/document/d/1PjAemEEFrl0gIDVQfnr1oAnHkb6D... BUG=608875,314819,313856 ========== to ========== Enabling kUseConsolidatedStartupFlow by default on trunk. Code changes: - Disabling onboarding content for ChromeOS. Promotions for CrOS are out-of-scope for this launch. - Ensuring that preference-specified tabs do not appear in new windows when a tabbed browser already exists for the profile. Test changes: - Removed 12 outdated tests - Fixed or updated 9 tests Details of affected tests, and rationale, can be found in this doc: https://docs.google.com/a/google.com/document/d/1PjAemEEFrl0gIDVQfnr1oAnHkb6D... BUG=608875,314819,313856 Review-Url: https://codereview.chromium.org/2475913003 Cr-Commit-Position: refs/heads/master@{#442487} Committed: https://chromium.googlesource.com/chromium/src/+/a52322134679a349366417e04e50... ==========
Message was sent while issue was closed.
Committed patchset #23 (id:440001) as https://chromium.googlesource.com/chromium/src/+/a52322134679a349366417e04e50...
Message was sent while issue was closed.
A revert of this CL (patchset #23 id:440001) has been created in https://codereview.chromium.org/2624443004/ by foolip@chromium.org. The reason for reverting is: StartupBrowserCreatorFirstRunTest.RestoreOnStartupURLsPolicySpecified failing on Win10 Tests x64 BUG=679671.
The CQ bit was checked by tmartino@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.
Preparing to re-land. Revert was addressed by: - Reintroducing a two-line guard on RestoreOnStartupURLsPolicySpecified. This test was not intended to run on Win10, and the check enforcing this was erroneously removed. - Adding Win10 trybot, which is green. https://codereview.chromium.org/2475913003/diff/420001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_browser_creator_browsertest.cc (right): https://codereview.chromium.org/2475913003/diff/420001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_browsertest.cc:351: if (!IsWindows10OrNewer()) { On 2017/01/09 at 23:27:07, Peter Kasting wrote: > Nit: Seems like maybe this should be combined with the previous conditional. Ack. Not really in-scope for this change, but I should be touching this soon when turning W10 FRE on-by-default.
The CQ bit was checked by tmartino@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rsesek@chromium.org, pkasting@chromium.org Link to the patchset: https://codereview.chromium.org/2475913003/#ps500001 (title: "Rebase-updated")
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": 500001, "attempt_start_ts": 1484087026073270,
"parent_rev": "e14c8ed4a52032ed6569a95febe1d2c73af782dc", "commit_rev":
"82e5578d15c0661c3306eebdd12bd7287f35cdf6"}
Message was sent while issue was closed.
Description was changed from ========== Enabling kUseConsolidatedStartupFlow by default on trunk. Code changes: - Disabling onboarding content for ChromeOS. Promotions for CrOS are out-of-scope for this launch. - Ensuring that preference-specified tabs do not appear in new windows when a tabbed browser already exists for the profile. Test changes: - Removed 12 outdated tests - Fixed or updated 9 tests Details of affected tests, and rationale, can be found in this doc: https://docs.google.com/a/google.com/document/d/1PjAemEEFrl0gIDVQfnr1oAnHkb6D... BUG=608875,314819,313856 Review-Url: https://codereview.chromium.org/2475913003 Cr-Commit-Position: refs/heads/master@{#442487} Committed: https://chromium.googlesource.com/chromium/src/+/a52322134679a349366417e04e50... ========== to ========== Enabling kUseConsolidatedStartupFlow by default on trunk. Code changes: - Disabling onboarding content for ChromeOS. Promotions for CrOS are out-of-scope for this launch. - Ensuring that preference-specified tabs do not appear in new windows when a tabbed browser already exists for the profile. Test changes: - Removed 12 outdated tests - Fixed or updated 9 tests Details of affected tests, and rationale, can be found in this doc: https://docs.google.com/a/google.com/document/d/1PjAemEEFrl0gIDVQfnr1oAnHkb6D... BUG=608875,314819,313856 Review-Url: https://codereview.chromium.org/2475913003 Cr-Original-Commit-Position: refs/heads/master@{#442487} Committed: https://chromium.googlesource.com/chromium/src/+/a52322134679a349366417e04e50... Review-Url: https://codereview.chromium.org/2475913003 Cr-Commit-Position: refs/heads/master@{#442712} Committed: https://chromium.googlesource.com/chromium/src/+/82e5578d15c0661c3306eebdd12b... ==========
Message was sent while issue was closed.
Committed patchset #26 (id:500001) as https://chromium.googlesource.com/chromium/src/+/82e5578d15c0661c3306eebdd12b... |
