Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(221)

Issue 11636031: [Fixit Dec-2012] Refactor first_run, very few things should depend on whether the First Run senti... (Closed)

Created:
8 years ago by gab
Modified:
7 years, 11 months ago
CC:
chromium-reviews, somast1
Visibility:
Public.

Description

Refactor first_run, very few things should depend on whether the First Run sentinel itself was present... (i.e. most tasks should not be on if flags such as --no-first-run are present). This is a prerequisite for issue 165048 as we want to be able to launch Chrome with --no-first-run and have two guarantees: 1) No first run tasks happen 2) First Run sentinel is set so that first run never happens for this new Chrome. This also removes PreInteractiveFirstRunInit() and PostInteractiveFirstRunInit() from BrowserMainParts as I just realized AutoImport is now silent and it is thus no longer necessary to split this in two :)! Moved everything to first_run::PostImportTasks(). This CL also buffs --first-run to --force-first-run (to be more explicit); --first-run now overrides --no-first-run as far as first run tasks are concerned (i.e. as far as everything first run related is concerned; except code that specifically looks for --no-first-run on the command line) -- kNoFirstRun used to state it would override kFirstRun, but that wasn't true anyways ans it makes more sense/is more useful the other way around. Now a browser test may specify --force-first-run and get the full first run experience even if the underlying test harness specifies --no-first-run for all tests :). --no-first-run is otherwise unaffected (i.e. same behavior as before) if --force-first-run is not present BUG=165048 TEST= 1) Run chrome.exe --force-first-run from build output and see First Run flow although First Run beacon is present. 2) Delete "First Run" beacon and ensure that chrome.exe --no-first-run both skips the first run and creates the beacon. 3) Ensure that chrome.exe --force-first-run --no-first-run acts the same as chrome.exe --force-first-run. Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=174809

Patch Set 1 #

Patch Set 2 : fix compile #

Patch Set 3 : fix posix compile #

Patch Set 4 : fix android and chromeos compile #

Total comments: 6

Patch Set 5 : actually call DoPostImportPlatformSpecificTasks()... #

Patch Set 6 : merge up to r174446 #

Total comments: 8

Patch Set 7 : tweaks #

Patch Set 8 : +comment #

Total comments: 2

Patch Set 9 : refactor some more #

Patch Set 10 : merge up to r174686 #

Patch Set 11 : --first-run => --force-first-run #

Total comments: 6

Patch Set 12 : restructured and clarified comments #

Patch Set 13 : fix typo #

Patch Set 14 : No first run import test on OS_CHROMEOS. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+113 lines, -122 lines) Patch
M chrome/browser/chrome_browser_main.h View 9 10 11 2 chunks +1 line, -4 lines 0 comments Download
M chrome/browser/chrome_browser_main.cc View 1 2 3 4 5 6 7 8 9 10 11 11 chunks +45 lines, -66 lines 0 comments Download
M chrome/browser/chrome_browser_main_extra_parts.h View 1 chunk +0 lines, -2 lines 0 comments Download
M chrome/browser/chrome_browser_main_win.h View 1 chunk +0 lines, -1 line 0 comments Download
M chrome/browser/chrome_browser_main_win.cc View 3 chunks +0 lines, -23 lines 0 comments Download
M chrome/browser/first_run/first_run.h View 1 2 3 4 5 6 7 8 9 2 chunks +4 lines, -4 lines 0 comments Download
M chrome/browser/first_run/first_run.cc View 1 2 3 4 5 6 7 8 9 5 chunks +7 lines, -5 lines 0 comments Download
M chrome/browser/first_run/first_run_browsertest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 3 chunks +3 lines, -2 lines 0 comments Download
M chrome/browser/first_run/first_run_internal.h View 1 chunk +2 lines, -0 lines 0 comments Download
M chrome/browser/first_run/first_run_posix.cc View 1 2 2 chunks +16 lines, -0 lines 0 comments Download
M chrome/browser/first_run/first_run_win.cc View 1 2 3 4 5 3 chunks +22 lines, -0 lines 0 comments Download
M chrome/browser/process_singleton_browsertest.cc View 1 2 3 4 5 6 7 8 9 10 2 chunks +2 lines, -2 lines 0 comments Download
M chrome/common/chrome_switches.h View 1 2 3 4 5 6 7 8 9 10 1 chunk +1 line, -2 lines 0 comments Download
M chrome/common/chrome_switches.cc View 1 2 3 4 5 6 7 8 9 10 11 12 2 chunks +6 lines, -7 lines 0 comments Download
M chrome/common/switch_utils.cc View 1 2 3 4 5 6 7 8 9 10 1 chunk +1 line, -1 line 0 comments Download
M chrome/common/switch_utils_unittest.cc View 1 2 3 4 5 6 7 8 9 10 2 chunks +2 lines, -2 lines 0 comments Download
M chrome/installer/setup/setup_main.cc View 1 2 3 4 5 6 7 8 9 10 1 chunk +1 line, -1 line 0 comments Download

Messages

Total messages: 22 (0 generated)
gab
Nico, please take a look: as an incentive I did one of your TODOs :). ...
8 years ago (2012-12-19 22:33:24 UTC) #1
Nico
On 2012/12/19 22:33:24, gab wrote: > Nico, please take a look: as an incentive I ...
8 years ago (2012-12-19 23:46:15 UTC) #2
gab
@Nico: fixed compile, PTAL. Thanks! Gab
8 years ago (2012-12-20 17:49:16 UTC) #3
gab
@Nico: friendly ping :)! On 2012/12/20 17:49:16, gab wrote: > @Nico: fixed compile, PTAL. > ...
8 years ago (2012-12-21 18:00:36 UTC) #4
gab
Resolving some merge issue with ToT, but the CL is still good to be reviewed! ...
8 years ago (2012-12-21 21:07:22 UTC) #5
gab
@tapted: this removes the kFirstRunForceImport flag you just added. I unified the first run flow ...
8 years ago (2012-12-21 21:30:55 UTC) #6
Nico
Sorry for the delay. I'm not very familiar with the first run code, maybe you ...
8 years ago (2012-12-21 21:44:21 UTC) #7
gab
Comments addressed/replied to. Trying to find someone to take a closer look. Thanks, Gab https://codereview.chromium.org/11636031/diff/9004/chrome/browser/chrome_browser_main.cc ...
8 years ago (2012-12-21 22:10:34 UTC) #8
Nico
Ok, lgtm from me. https://codereview.chromium.org/11636031/diff/18016/chrome/browser/chrome_browser_main.cc File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/11636031/diff/18016/chrome/browser/chrome_browser_main.cc#newcode1192 chrome/browser/chrome_browser_main.cc:1192: first_run::DoPostImportTasks(profile_, master_prefs_->make_chrome_default); On 2012/12/21 22:10:35, ...
8 years ago (2012-12-21 22:13:05 UTC) #9
gab
@cpu: can you please take a look? Thanks! Gab
8 years ago (2012-12-21 22:17:36 UTC) #10
gab
https://codereview.chromium.org/11636031/diff/18016/chrome/browser/chrome_browser_main.cc File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/11636031/diff/18016/chrome/browser/chrome_browser_main.cc#newcode1192 chrome/browser/chrome_browser_main.cc:1192: first_run::DoPostImportTasks(profile_, master_prefs_->make_chrome_default); On 2012/12/21 22:13:05, Nico wrote: > On ...
8 years ago (2012-12-21 22:21:37 UTC) #11
tapted
https://codereview.chromium.org/11636031/diff/9010/chrome/browser/chrome_browser_main.cc File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/11636031/diff/9010/chrome/browser/chrome_browser_main.cc#newcode826 chrome/browser/chrome_browser_main.cc:826: if (parsed_command_line().HasSwitch(switches::kNoFirstRun)) { I think this line will mean ...
8 years ago (2012-12-21 23:23:44 UTC) #12
gab
Thanks for your feedback, updated revised code (see comment below -- also updated the description ...
7 years, 12 months ago (2012-12-27 21:18:16 UTC) #13
cpu_(ooo_6.6-7.5)
in general looks good, some concerns below and one observation: we need chrome to work ...
7 years, 12 months ago (2012-12-27 23:53:05 UTC) #14
tapted
Changes around kFirstRunForceImport lgtm. Definitely nicer than the approach I had to fall back on ...
7 years, 12 months ago (2012-12-28 13:59:42 UTC) #15
gab
Changed some logic around in response to your comments: About your concerns: > 1- installed ...
7 years, 11 months ago (2012-12-28 20:17:45 UTC) #16
gab
On 2012/12/28 20:17:45, gab wrote: > Changed some logic around in response to your comments: ...
7 years, 11 months ago (2012-12-28 20:22:57 UTC) #17
cpu_(ooo_6.6-7.5)
lgtm
7 years, 11 months ago (2012-12-28 20:46:38 UTC) #18
gab
TBR tapted: see patch set 14, turned off your new FirstRunIntegrationBrowserTest.WaitForImport test on Chrome OS; ...
7 years, 11 months ago (2013-01-02 15:44:54 UTC) #19
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gab@chromium.org/11636031/61001
7 years, 11 months ago (2013-01-02 15:45:33 UTC) #20
commit-bot: I haz the power
Change committed as 174809
7 years, 11 months ago (2013-01-02 17:52:40 UTC) #21
tapted
7 years, 11 months ago (2013-01-03 13:48:36 UTC) #22
Message was sent while issue was closed.
On 2013/01/02 15:44:54, gab wrote:
> TBR tapted: see patch set 14, turned off your new
> FirstRunIntegrationBrowserTest.WaitForImport test on Chrome OS; 

lgtm. Rightly so -- ChromeOS should not have to care about import. Thanks.

Powered by Google App Engine
This is Rietveld 408576698