Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
12035043:
Implementing app command to query EULA acceptance state for Chrome. (Closed)
|
Created:
7 years, 11 months ago by huangs Modified:
7 years, 10 months ago CC:
chromium-reviews, grt+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionImplementing app command to query EULA acceptance state for Chrome.
There are 2 parts to this change:
1. Implement setup.exe switch --query-eula-acceptance, and return the result via exit code.
- Main use case is system-level, which requires the "--system-level" switch.
- Return code: 0 if "not accepted", 1 if "accepted", and -1 if failed.
2. Add the app command query-eula-acceptance to Chrome binary during installation to execute:
setup.exe --query-eula-acceptance [--system-level]
BUG=151303
TBR=ben
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=180052
Patch Set 1 #Patch Set 2 : Code complete: added app command install and app_host.exe wait/forward. #
Total comments: 27
Patch Set 3 : Now using setup.exe; removing all Browser and App Launcher changes. #
Total comments: 36
Patch Set 4 : Updated ProductState logic on system/user level, Binaries/Chrome. #
Total comments: 13
Patch Set 5 : Comments, naming, and cleanup. #
Total comments: 16
Patch Set 6 : Installation validator; adding user-level vs. system-level param; limit user-level checks for Chrom… #
Total comments: 5
Patch Set 7 : Small cleanup. #Patch Set 8 : Fixing installation validator unit test. #
Total comments: 2
Patch Set 9 : Also try to get ProductState from Chrome Frame for IsEULAAccepted(); refactoring. #
Total comments: 4
Patch Set 10 : Make IsEULAAccepted() fallback to Chrome only in system level. #
Total comments: 4
Patch Set 11 : Style and string fixes. #
Messages
Total messages: 26 (0 generated)
Preliminary code (for part 1), PTAL regarding logic for EULA acceptance.
Since Friday is a write-off, it makes more sense to implement part 2 and part 3 to save time. Notes: - This is feature-complete. I did some basic testing, but we may have some major restructuring / cleanup to do. - I think the "query-eula-accepted" should be added to App Launcher, since the command is anchored to app_host.exe. If Chrome needs it in the future, it should get its own. - app_host.exe: Instead of simple return values of 0/1, I want it to do standard HRESULT thing, i.e., return E_FAIL on failure. - chrome.exe might be too slow; for debug build I see its memory shoot up to 100 MB's! Maybe we can extract the EULA checking code as separate module in installer, and have app_host.exe call it directly? https://codereview.chromium.org/12035043/diff/9/chrome/installer/setup/instal... File chrome/installer/setup/install_worker.cc (left): https://codereview.chromium.org/12035043/diff/9/chrome/installer/setup/instal... chrome/installer/setup/install_worker.cc:215: const FilePath& setup_path, (Unrelated) code cleanup: unused parameter. https://codereview.chromium.org/12035043/diff/9/chrome/installer/setup/instal... chrome/installer/setup/install_worker.cc:256: setup_path, new_version, p, list); (Unrelated) code cleanup: unused parameter. https://codereview.chromium.org/12035043/diff/9/chrome/installer/setup/instal... File chrome/installer/setup/install_worker.cc (right): https://codereview.chromium.org/12035043/diff/9/chrome/installer/setup/instal... chrome/installer/setup/install_worker.cc:1657: // cmd.set_sends_pings(true); // QUESTION(huangs): Do we need this????? Please note question.
https://codereview.chromium.org/12035043/diff/9/chrome/app/chrome_main_delega... File chrome/app/chrome_main_delegate.cc (right): https://codereview.chromium.org/12035043/diff/9/chrome/app/chrome_main_delega... chrome/app/chrome_main_delegate.cc:439: if (HandleInfoQuery(command_line, exit_code)) { nit: no braces https://codereview.chromium.org/12035043/diff/9/chrome/browser/first_run/firs... File chrome/browser/first_run/first_run.h (right): https://codereview.chromium.org/12035043/diff/9/chrome/browser/first_run/firs... chrome/browser/first_run/first_run.h:155: #if defined(OS_WIN) i think it's cleaner to implement this for all platforms (non-Win platforms return true). https://codereview.chromium.org/12035043/diff/9/chrome/browser/first_run/firs... chrome/browser/first_run/first_run.h:157: // Chrome, and returns the result. This result may be obtained via exit code remove second sentence. https://codereview.chromium.org/12035043/diff/9/chrome/browser/first_run/firs... File chrome/browser/first_run/first_run_win.cc (right): https://codereview.chromium.org/12035043/diff/9/chrome/browser/first_run/firs... chrome/browser/first_run/first_run_win.cc:25: #include "base/win/registry.h" remove this https://codereview.chromium.org/12035043/diff/9/chrome/browser/first_run/firs... chrome/browser/first_run/first_run_win.cc:45: #include "chrome/installer/util/install_util.h" #include "chrome/installer/util/installation_state.h" https://codereview.chromium.org/12035043/diff/9/chrome/browser/first_run/firs... chrome/browser/first_run/first_run_win.cc:65: // Taken from google_update_constants.cc. remove these https://codereview.chromium.org/12035043/diff/9/chrome/browser/first_run/firs... chrome/browser/first_run/first_run_win.cc:140: bool GetGoogleUpdateEULAAccepted(DWORD* value) { remove this https://codereview.chromium.org/12035043/diff/9/chrome/browser/first_run/firs... chrome/browser/first_run/first_run_win.cc:149: bool IsEULAUnrequiredOrAccepted(installer::MasterPreferences* install_prefs) { please either justify this change or revert it and call !IsEULANotAccepted in your new code. https://codereview.chromium.org/12035043/diff/9/chrome/browser/first_run/firs... chrome/browser/first_run/first_run_win.cc:525: if (GetGoogleUpdateEULAAccepted(&google_udpate_eula_accepted) why are you checking if google update's EULA has been accepted rather than chrome's? i think you should instead do something like: installer::ProductState binaries; if (!binaries.Initialize(true, BrowserDistribution::CHROME_BINARIES)) { // Error: system-level binaries aren't installed. I'm not sure what you want to do here. } // Is Omaha waiting for Chrome's EULA to be accepted? DWORD eula_accepted = 0; if (binaries.GetEulaAccepted(&eula_accepted) && !eula_accepted) return false; // Has the current user been through first-run? if (!IsChromeFirstRun()) return true; // Will Chrome show the EULA on first-run? ... return !IsEULANotAccepted(install_prefs.get()); https://codereview.chromium.org/12035043/diff/9/chrome/common/chrome_switches.cc File chrome/common/chrome_switches.cc (right): https://codereview.chromium.org/12035043/diff/9/chrome/common/chrome_switches... chrome/common/chrome_switches.cc:1104: // the result (0 or 1) as exit code. please be explicit as to what 0 and 1 mean. in general, a process exit code of 0 means success and non-zero means failure, but it looks like the implementation returns 1 for has-been-accepted and 0 for not-been-accepted (in contrast to this comment's implied 0=accepted, 1=not accepted). https://codereview.chromium.org/12035043/diff/9/chrome/installer/setup/instal... File chrome/installer/setup/install_worker.cc (right): https://codereview.chromium.org/12035043/diff/9/chrome/installer/setup/instal... chrome/installer/setup/install_worker.cc:1644: string16 cmd_key(product.distribution()->GetVersionKey()); this is adding the command to the app host. something here doesn't seem right. should it be invoking the system-level chrome.exe? https://codereview.chromium.org/12035043/diff/9/chrome/installer/setup/instal... chrome/installer/setup/install_worker.cc:1654: installer_state.target_path().Append(installer::kChromeAppHostExe)); kChromeAppHostExe -> kChromeExe?
Everything is moved to setup.exe. Please note that there is some code duplication between eula_util.cc and first_run.cc. PTAL. https://codereview.chromium.org/12035043/diff/9/chrome/app/chrome_main_delega... File chrome/app/chrome_main_delegate.cc (right): https://codereview.chromium.org/12035043/diff/9/chrome/app/chrome_main_delega... chrome/app/chrome_main_delegate.cc:439: if (HandleInfoQuery(command_line, exit_code)) { On 2013/01/23 19:02:29, grt wrote: > nit: no braces Done. https://codereview.chromium.org/12035043/diff/9/chrome/browser/first_run/firs... File chrome/browser/first_run/first_run.h (right): https://codereview.chromium.org/12035043/diff/9/chrome/browser/first_run/firs... chrome/browser/first_run/first_run.h:155: #if defined(OS_WIN) Moot; code is moved to eula_util.h/cc https://codereview.chromium.org/12035043/diff/9/chrome/browser/first_run/firs... chrome/browser/first_run/first_run.h:157: // Chrome, and returns the result. This result may be obtained via exit code On 2013/01/23 19:02:29, grt wrote: > remove second sentence. Done. https://codereview.chromium.org/12035043/diff/9/chrome/browser/first_run/firs... File chrome/browser/first_run/first_run_win.cc (right): https://codereview.chromium.org/12035043/diff/9/chrome/browser/first_run/firs... chrome/browser/first_run/first_run_win.cc:25: #include "base/win/registry.h" Moot; all Browser-specific changes are now gone. https://codereview.chromium.org/12035043/diff/9/chrome/browser/first_run/firs... chrome/browser/first_run/first_run_win.cc:45: #include "chrome/installer/util/install_util.h" Done for eula_util.cc https://codereview.chromium.org/12035043/diff/9/chrome/browser/first_run/firs... chrome/browser/first_run/first_run_win.cc:65: // Taken from google_update_constants.cc. On 2013/01/23 19:02:29, grt wrote: > remove these Done. https://codereview.chromium.org/12035043/diff/9/chrome/browser/first_run/firs... chrome/browser/first_run/first_run_win.cc:140: bool GetGoogleUpdateEULAAccepted(DWORD* value) { On 2013/01/23 19:02:29, grt wrote: > remove this Done. https://codereview.chromium.org/12035043/diff/9/chrome/browser/first_run/firs... chrome/browser/first_run/first_run_win.cc:149: bool IsEULAUnrequiredOrAccepted(installer::MasterPreferences* install_prefs) { Reverted in this file. In eula_util.cc, decided to break this into two routines: IsEULARequiredInMasterPrefs() [return 1 if false], IsEULASentinelPresent() [return 1 iff true]. https://codereview.chromium.org/12035043/diff/9/chrome/browser/first_run/firs... chrome/browser/first_run/first_run_win.cc:525: if (GetGoogleUpdateEULAAccepted(&google_udpate_eula_accepted) Made the change. Also: - Return value is HRESULT, with 0 if EULA not accepted, 1 if EULA accepted, and E_FAIL on error. - binaries.Initialize(): Checking for system level, THEN user level. https://codereview.chromium.org/12035043/diff/9/chrome/common/chrome_switches.cc File chrome/common/chrome_switches.cc (right): https://codereview.chromium.org/12035043/diff/9/chrome/common/chrome_switches... chrome/common/chrome_switches.cc:1104: // the result (0 or 1) as exit code. Done; made comment explicit, and returning E_FAIL on error. https://codereview.chromium.org/12035043/diff/9/chrome/installer/setup/instal... File chrome/installer/setup/install_worker.cc (right): https://codereview.chromium.org/12035043/diff/9/chrome/installer/setup/instal... chrome/installer/setup/install_worker.cc:1644: string16 cmd_key(product.distribution()->GetVersionKey()); It's invoking whatever Chrome that's available. https://codereview.chromium.org/12035043/diff/9/chrome/installer/setup/instal... chrome/installer/setup/install_worker.cc:1654: installer_state.target_path().Append(installer::kChromeAppHostExe)); On 2013/01/23 19:02:29, grt wrote: > kChromeAppHostExe -> kChromeExe? Done.
some preliminary comments. more to come. https://codereview.chromium.org/12035043/diff/10005/chrome/installer/util/eul... File chrome/installer/util/eula_util.cc (right): https://codereview.chromium.org/12035043/diff/10005/chrome/installer/util/eul... chrome/installer/util/eula_util.cc:23: bool IsChromeFirstRun(BrowserDistribution* dist) { IsChromeFirstRun -> IsChromeFirstRunPending or something like that. https://codereview.chromium.org/12035043/diff/10005/chrome/installer/util/eul... chrome/installer/util/eula_util.cc:24: // If there is problem getting sentinel path, assume not first run. If there is a problem getting the sentinel path, assume Chrome has been run. https://codereview.chromium.org/12035043/diff/10005/chrome/installer/util/eul... chrome/installer/util/eula_util.cc:32: bool IsEULASentinelPresent(BrowserDistribution* dist ) { "dist " -> "dist" https://codereview.chromium.org/12035043/diff/10005/chrome/installer/util/eul... chrome/installer/util/eula_util.cc:41: MasterPreferences* GetNewMasterPrefs() { scoped_ptr<MasterPreferences> GetNewMasterPrefs() { https://codereview.chromium.org/12035043/diff/10005/chrome/installer/util/eul... chrome/installer/util/eula_util.cc:43: FilePath chrome_exe(chrome_launcher_support::GetAnyChromePath()); i haven't read what GetAnyChromePath does, but what you want here is specifically the master_preferences file corresponding to the ProductState used in IsEULAAccepted below. it should be in the directory product_state.GetSetupPath().DirName().DirName().DirName(). https://codereview.chromium.org/12035043/diff/10005/chrome/installer/util/eul... chrome/installer/util/eula_util.cc:46: MasterPreferences* install_prefs = new MasterPreferences( scoped_ptr<MasterPreferences> install_prefs(...); https://codereview.chromium.org/12035043/diff/10005/chrome/installer/util/eul... chrome/installer/util/eula_util.cc:48: if (install_prefs != NULL) { if (install_prefs && install_prefs->read_from_file()) return install_prefs.Pass(); and remove line 52 https://codereview.chromium.org/12035043/diff/10005/chrome/installer/util/eul... chrome/installer/util/eula_util.cc:55: return NULL; you may need to make this: return scoped_ptr<MasterPreferences>(); https://codereview.chromium.org/12035043/diff/10005/chrome/installer/util/eul... chrome/installer/util/eula_util.cc:58: bool IsEULARequiredInMasterPrefs(MasterPreferences* install_prefs) { this function takes a MasterPreferences, so it doesn't need "MasterPrefs" in the name. i suggest IsRequireEulaTrue since that accurately describes what the function does. otherwise, i find this so trivial that putting it in its own function obscures the code rather than clarifying it. https://codereview.chromium.org/12035043/diff/10005/chrome/installer/util/eul... chrome/installer/util/eula_util.cc:73: return E_FAIL; // Error: system-level binaries aren't installed. !dist means that the impossible happened rather than that the binaries aren't installed. you don't need to check this. https://codereview.chromium.org/12035043/diff/10005/chrome/installer/util/eul... chrome/installer/util/eula_util.cc:76: if (!binaries.Initialize(true, dist) && !binaries.Initialize(false, dist)) eula only applies to system-level installs. you should not check for user-level. for this to be comprehensive, you should fallback to checking for CHROME_BROWSER if the binaries aren't installed and put a: LOG_IF(DFATAL, whatever.is_multi_install()) << "binaries are not installed yet Chrome thinks it's multi-install."; https://codereview.chromium.org/12035043/diff/10005/chrome/installer/util/eul... chrome/installer/util/eula_util.cc:85: if (!IsChromeFirstRun(dist)) use the BrowserDistribution::CHROME_BROWSER instance here. https://codereview.chromium.org/12035043/diff/10005/chrome/installer/util/eul... chrome/installer/util/eula_util.cc:87: is the result the same if the EULA sentinel check is moved here: if (IsEULASentinelPresent(dist)) return 1; if so, i think this is the teensiest bit more efficient since loading master_prefs can be short-circuited in that case. https://codereview.chromium.org/12035043/diff/10005/chrome/installer/util/eul... chrome/installer/util/eula_util.cc:89: if (!install_prefs.get()) if (!install_prefs) https://codereview.chromium.org/12035043/diff/10005/chrome/installer/util/eul... File chrome/installer/util/eula_util.h (right): https://codereview.chromium.org/12035043/diff/10005/chrome/installer/util/eul... chrome/installer/util/eula_util.h:5: // This file contains utilities to read and manage EULA for Chrome. i think it's more accurate to say something about the file containing helper functions relating to Chrome's EULA. as it is, the comment implies that there's a utility to read the EULA. https://codereview.chromium.org/12035043/diff/10005/chrome/installer/util/eul... chrome/installer/util/eula_util.h:14: // Performs a comprehensive test on EULA acceptance in Chrome. "...in system-level Chrome." https://codereview.chromium.org/12035043/diff/10005/chrome/installer/util/eul... chrome/installer/util/eula_util.h:15: // Returns: 0 if EULA not accepted, 1 if EULA accepted, and E_FAIL on error. you're mixing windows HRESULTS with arbitrary values you've picked. please don't do this. either use HRESULTS consistently (e.g., S_FALSE, S_OK, or E_FAIL), or make up your own type. fwiw, i don't see an obvious benefit to using HRESULT here (except if you're coding with malice :-)). using your own enum will add clarity to the code.
PTAL. https://codereview.chromium.org/12035043/diff/10005/chrome/installer/util/eul... File chrome/installer/util/eula_util.cc (right): https://codereview.chromium.org/12035043/diff/10005/chrome/installer/util/eul... chrome/installer/util/eula_util.cc:23: bool IsChromeFirstRun(BrowserDistribution* dist) { On 2013/01/24 02:51:40, grt wrote: > IsChromeFirstRun -> IsChromeFirstRunPending or something like that. Done. https://codereview.chromium.org/12035043/diff/10005/chrome/installer/util/eul... chrome/installer/util/eula_util.cc:24: // If there is problem getting sentinel path, assume not first run. Done (rephrased to fit line). https://codereview.chromium.org/12035043/diff/10005/chrome/installer/util/eul... chrome/installer/util/eula_util.cc:32: bool IsEULASentinelPresent(BrowserDistribution* dist ) { On 2013/01/24 02:51:40, grt wrote: > "dist " -> "dist" Done. https://codereview.chromium.org/12035043/diff/10005/chrome/installer/util/eul... chrome/installer/util/eula_util.cc:41: MasterPreferences* GetNewMasterPrefs() { On 2013/01/24 02:51:40, grt wrote: > scoped_ptr<MasterPreferences> GetNewMasterPrefs() { Done. https://codereview.chromium.org/12035043/diff/10005/chrome/installer/util/eul... chrome/installer/util/eula_util.cc:43: FilePath chrome_exe(chrome_launcher_support::GetAnyChromePath()); Done (need to pass const ProductState& product_state). https://codereview.chromium.org/12035043/diff/10005/chrome/installer/util/eul... chrome/installer/util/eula_util.cc:46: MasterPreferences* install_prefs = new MasterPreferences( On 2013/01/24 02:51:40, grt wrote: > scoped_ptr<MasterPreferences> install_prefs(...); Done. https://codereview.chromium.org/12035043/diff/10005/chrome/installer/util/eul... chrome/installer/util/eula_util.cc:48: if (install_prefs != NULL) { On 2013/01/24 02:51:40, grt wrote: > if (install_prefs && install_prefs->read_from_file()) > return install_prefs.Pass(); > and remove line 52 Done. https://codereview.chromium.org/12035043/diff/10005/chrome/installer/util/eul... chrome/installer/util/eula_util.cc:55: return NULL; Done. Thanks! https://codereview.chromium.org/12035043/diff/10005/chrome/installer/util/eul... chrome/installer/util/eula_util.cc:58: bool IsEULARequiredInMasterPrefs(MasterPreferences* install_prefs) { The function is added because I was envisioning replacing IsEULANotAccepted() in first_run_win.cc by IsEULARequiredInMasterPrefs() && !IsEULASentinelPresent(). in the future. But we're not touching that code, so this is redundant. Removed and inlined. https://codereview.chromium.org/12035043/diff/10005/chrome/installer/util/eul... chrome/installer/util/eula_util.cc:73: return E_FAIL; // Error: system-level binaries aren't installed. On 2013/01/24 02:51:40, grt wrote: > !dist means that the impossible happened rather than that the binaries aren't > installed. you don't need to check this. Done. https://codereview.chromium.org/12035043/diff/10005/chrome/installer/util/eul... chrome/installer/util/eula_util.cc:76: if (!binaries.Initialize(true, dist) && !binaries.Initialize(false, dist)) Done. Also added logic to return EULA_YES when queries were made at user-level. https://codereview.chromium.org/12035043/diff/10005/chrome/installer/util/eul... chrome/installer/util/eula_util.cc:85: if (!IsChromeFirstRun(dist)) On 2013/01/24 02:51:40, grt wrote: > use the BrowserDistribution::CHROME_BROWSER instance here. Done. https://codereview.chromium.org/12035043/diff/10005/chrome/installer/util/eul... chrome/installer/util/eula_util.cc:87: It's a bit bizarre since we check for "yes" before deciding if the check is even necessary. Making the change anyway, since this is short enough. https://codereview.chromium.org/12035043/diff/10005/chrome/installer/util/eul... chrome/installer/util/eula_util.cc:89: if (!install_prefs.get()) On 2013/01/24 02:51:40, grt wrote: > if (!install_prefs) Done. https://codereview.chromium.org/12035043/diff/10005/chrome/installer/util/eul... File chrome/installer/util/eula_util.h (right): https://codereview.chromium.org/12035043/diff/10005/chrome/installer/util/eul... chrome/installer/util/eula_util.h:5: // This file contains utilities to read and manage EULA for Chrome. On 2013/01/24 02:51:40, grt wrote: > i think it's more accurate to say something about the file containing helper > functions relating to Chrome's EULA. as it is, the comment implies that there's > a utility to read the EULA. Done. https://codereview.chromium.org/12035043/diff/10005/chrome/installer/util/eul... chrome/installer/util/eula_util.h:14: // Performs a comprehensive test on EULA acceptance in Chrome. On 2013/01/24 02:51:40, grt wrote: > "...in system-level Chrome." Done (more elaborate comments). https://codereview.chromium.org/12035043/diff/10005/chrome/installer/util/eul... chrome/installer/util/eula_util.h:15: // Returns: 0 if EULA not accepted, 1 if EULA accepted, and E_FAIL on error. Defined enum EULAAcceptanceResponse. Is it okay that it gets implicitly casted to int, and returned as exit code?
https://codereview.chromium.org/12035043/diff/3004/chrome/installer/setup/ins... File chrome/installer/setup/install_worker.cc (right): https://codereview.chromium.org/12035043/diff/3004/chrome/installer/setup/ins... chrome/installer/setup/install_worker.cc:1663: // cmd.set_sends_pings(true); // QUESTION(huangs): Do we need this????? Probably not. It is used for reporting how often app commands run and whether they succeed. Useful for installation-like commands, but probably not for this. https://codereview.chromium.org/12035043/diff/3004/chrome/installer/util/eula... File chrome/installer/util/eula_util.cc (right): https://codereview.chromium.org/12035043/diff/3004/chrome/installer/util/eula... chrome/installer/util/eula_util.cc:21: bool IsChromeFirstRunPending(BrowserDistribution* dist) { Comment something like: // Chrome creates a sentinel file after first run... https://codereview.chromium.org/12035043/diff/3004/chrome/installer/util/eula... chrome/installer/util/eula_util.cc:30: bool IsEULASentinelPresent(BrowserDistribution* dist) { Something like: // Chrome creates a sentinel after the Eula is accepted. Also, the name is inconsistent with "IsChromeFirstRunPending". How about making the name reflect the meaning of the 'eula_sentinel' presence (whatever that is). https://codereview.chromium.org/12035043/diff/3004/chrome/installer/util/eula... chrome/installer/util/eula_util.cc:63: return prod_state.Initialize(false, BrowserDistribution::CHROME_BINARIES) If we accept the presence of a user-level Chrome or Binaries as proof of EULA acceptance, that should arguably be the first thing we look for (as it would be valid even if a system-level Chrome were installed without EULA acceptance). https://codereview.chromium.org/12035043/diff/3004/chrome/installer/util/eula... chrome/installer/util/eula_util.cc:81: // If we fail to get master preferences, assume EULA is not accepted. I think master preferences can reasonably be absent.. grt? https://codereview.chromium.org/12035043/diff/3004/chrome/installer/util/eula... File chrome/installer/util/eula_util.h (right): https://codereview.chromium.org/12035043/diff/3004/chrome/installer/util/eula... chrome/installer/util/eula_util.h:21: // Returns EULAAcceptanceResponse: EULA_NO if EULA not accepted, I think "Returns..." is unnecessary verbosity.
PTAL. https://codereview.chromium.org/12035043/diff/3004/chrome/installer/setup/ins... File chrome/installer/setup/install_worker.cc (right): https://codereview.chromium.org/12035043/diff/3004/chrome/installer/setup/ins... chrome/installer/setup/install_worker.cc:1663: // cmd.set_sends_pings(true); // QUESTION(huangs): Do we need this????? Removed. https://codereview.chromium.org/12035043/diff/3004/chrome/installer/util/eula... File chrome/installer/util/eula_util.cc (right): https://codereview.chromium.org/12035043/diff/3004/chrome/installer/util/eula... chrome/installer/util/eula_util.cc:21: bool IsChromeFirstRunPending(BrowserDistribution* dist) { Done. Please note the subsequent comment is about the (unexpected case) where GetSentinelFilePath() fails. This behaviour is copied from first_run. https://codereview.chromium.org/12035043/diff/3004/chrome/installer/util/eula... chrome/installer/util/eula_util.cc:30: bool IsEULASentinelPresent(BrowserDistribution* dist) { Renamed to IsEULAAcceptanceFlagged(). https://codereview.chromium.org/12035043/diff/3004/chrome/installer/util/eula... chrome/installer/util/eula_util.cc:63: return prod_state.Initialize(false, BrowserDistribution::CHROME_BINARIES) On 2013/01/24 19:09:54, erikwright wrote: > If we accept the presence of a user-level Chrome or Binaries as proof of EULA > acceptance, that should arguably be the first thing we look for (as it would be > valid even if a system-level Chrome were installed without EULA acceptance). Done. https://codereview.chromium.org/12035043/diff/3004/chrome/installer/util/eula... File chrome/installer/util/eula_util.h (right): https://codereview.chromium.org/12035043/diff/3004/chrome/installer/util/eula... chrome/installer/util/eula_util.h:21: // Returns EULAAcceptanceResponse: EULA_NO if EULA not accepted, Removed.
LGTM minus the remaining question to grt about master preferences. https://codereview.chromium.org/12035043/diff/16001/chrome/installer/util/eul... File chrome/installer/util/eula_util.cc (right): https://codereview.chromium.org/12035043/diff/16001/chrome/installer/util/eul... chrome/installer/util/eula_util.cc:64: // Try to use system-level Chrome binaries product state first. nit 'first' now seems out of place. Probably OK to just remove it and leave the rest.
OWNER review for ben@: Changed chrome_installer_util.gypi to add 2 files: chrome/installer/util/eula_util.h chrome/installer/util/eula_util.cc
https://codereview.chromium.org/12035043/diff/10005/chrome/installer/util/eul... File chrome/installer/util/eula_util.cc (right): https://codereview.chromium.org/12035043/diff/10005/chrome/installer/util/eul... chrome/installer/util/eula_util.cc:87: On 2013/01/24 18:56:03, huangs1 wrote: > It's a bit bizarre since we check for "yes" before deciding if the check is even > necessary. Making the change anyway, since this is short enough. it's an optimization. the cost of parsing master_preferences is significantly higher than the cost of checking for the sentinel. as long as the logic is correct, i think it makes sense. https://codereview.chromium.org/12035043/diff/10005/chrome/installer/util/eul... File chrome/installer/util/eula_util.h (right): https://codereview.chromium.org/12035043/diff/10005/chrome/installer/util/eul... chrome/installer/util/eula_util.h:15: // Returns: 0 if EULA not accepted, 1 if EULA accepted, and E_FAIL on error. On 2013/01/24 18:56:03, huangs1 wrote: > Defined enum EULAAcceptanceResponse. Is it okay that it gets implicitly casted > to int, and returned as exit code? IIRC, a process exit code in Windows is a DWORD, which is unsigned. I don't think you should be returning -1 from your process. For consistency with ordinary process exit, I think you should try to make 0 represent the thumbs-up success case, and other non-zero values represent the other cases. https://codereview.chromium.org/12035043/diff/16001/chrome/installer/setup/in... File chrome/installer/setup/install_worker.cc (right): https://codereview.chromium.org/12035043/diff/16001/chrome/installer/setup/in... chrome/installer/setup/install_worker.cc:1661: cmd_line.AppendSwitch(switches::kQueryEULAAcceptance); does it make sense to add --system-level if it's a system-level install so that the query only checks at the level of the installed binaries? i feel vaguely uncomfortable with the current code which tries to check both. also, consider adding kVerboseLogging if installer_state.verbose_logging(). https://codereview.chromium.org/12035043/diff/16001/chrome/installer/util/eul... File chrome/installer/util/eula_util.cc (right): https://codereview.chromium.org/12035043/diff/16001/chrome/installer/util/eul... chrome/installer/util/eula_util.cc:22: // Chrome creates a sentinel file after first run, and we look for it. // Chrome creates the first run sentinel after the user has gone through the // first-run flow. Assume Chrome has been run if the path to the sentinel // cannot be determined. https://codereview.chromium.org/12035043/diff/16001/chrome/installer/util/eul... chrome/installer/util/eula_util.cc:32: // Chrome creates a sentinel after the Eula is accepted. // Chrome creates the EULA sentinel after the EULA has been accepted when // doing so is required by master_preferences. Assume the EULA has not been // accepted if the path to the sentinel cannot be determined. https://codereview.chromium.org/12035043/diff/16001/chrome/installer/util/eul... chrome/installer/util/eula_util.cc:58: // If user-level products exist, then EULA has been accepted. Are you sure it's is safe to consider the eula to have been accepted if any multi-install product is installed at user-level? This is true for Chrome and Chrome Frame, but I'm not sure it'll always be true for all products. https://codereview.chromium.org/12035043/diff/16001/chrome/installer/util/eul... File chrome/installer/util/eula_util.h (right): https://codereview.chromium.org/12035043/diff/16001/chrome/installer/util/eul... chrome/installer/util/eula_util.h:14: EULA_NO = 0, EULA_NOT_ACCEPTED https://codereview.chromium.org/12035043/diff/16001/chrome/installer/util/eul... chrome/installer/util/eula_util.h:15: EULA_YES = 1 nit: trailing comma https://codereview.chromium.org/12035043/diff/16001/chrome/installer/util/eul... chrome/installer/util/eula_util.h:15: EULA_YES = 1 EULA_ACCEPTED
https://codereview.chromium.org/12035043/diff/3004/chrome/installer/util/eula... File chrome/installer/util/eula_util.cc (right): https://codereview.chromium.org/12035043/diff/3004/chrome/installer/util/eula... chrome/installer/util/eula_util.cc:81: // If we fail to get master preferences, assume EULA is not accepted. On 2013/01/24 19:09:54, erikwright wrote: > I think master preferences can reasonably be absent.. grt? absolutely. no master preferences file should be treated the same way as a master preferences file that doesn't contain require_eula.
I forgot to update the installation validator previously... PTAL. https://codereview.chromium.org/12035043/diff/3004/chrome/installer/util/eula... File chrome/installer/util/eula_util.cc (right): https://codereview.chromium.org/12035043/diff/3004/chrome/installer/util/eula... chrome/installer/util/eula_util.cc:81: // If we fail to get master preferences, assume EULA is not accepted. !!! In this case, we should return EULA_YES, since missing "require_eula" => require_eula == false => Accepted. https://codereview.chromium.org/12035043/diff/16001/chrome/installer/setup/in... File chrome/installer/setup/install_worker.cc (right): https://codereview.chromium.org/12035043/diff/16001/chrome/installer/setup/in... chrome/installer/setup/install_worker.cc:1661: cmd_line.AppendSwitch(switches::kQueryEULAAcceptance); Ah, right. Done. https://codereview.chromium.org/12035043/diff/16001/chrome/installer/util/eul... File chrome/installer/util/eula_util.cc (right): https://codereview.chromium.org/12035043/diff/16001/chrome/installer/util/eul... chrome/installer/util/eula_util.cc:22: // Chrome creates a sentinel file after first run, and we look for it. On 2013/01/24 21:04:55, grt wrote: > // Chrome creates the first run sentinel after the user has gone through the > // first-run flow. Assume Chrome has been run if the path to the sentinel > // cannot be determined. Done. https://codereview.chromium.org/12035043/diff/16001/chrome/installer/util/eul... chrome/installer/util/eula_util.cc:32: // Chrome creates a sentinel after the Eula is accepted. On 2013/01/24 21:04:55, grt wrote: > // Chrome creates the EULA sentinel after the EULA has been accepted when > // doing so is required by master_preferences. Assume the EULA has not been > // accepted if the path to the sentinel cannot be determined. Done. https://codereview.chromium.org/12035043/diff/16001/chrome/installer/util/eul... chrome/installer/util/eula_util.cc:58: // If user-level products exist, then EULA has been accepted. Changing interface to get "bool system_level" parameter. For the user-level case, I'll be conservative, and explicitly look for Chrome and App Launcher only. https://codereview.chromium.org/12035043/diff/16001/chrome/installer/util/eul... chrome/installer/util/eula_util.cc:64: // Try to use system-level Chrome binaries product state first. Removed. https://codereview.chromium.org/12035043/diff/16001/chrome/installer/util/eul... File chrome/installer/util/eula_util.h (right): https://codereview.chromium.org/12035043/diff/16001/chrome/installer/util/eul... chrome/installer/util/eula_util.h:14: EULA_NO = 0, On 2013/01/24 21:04:55, grt wrote: > EULA_NOT_ACCEPTED Done. https://codereview.chromium.org/12035043/diff/16001/chrome/installer/util/eul... chrome/installer/util/eula_util.h:15: EULA_YES = 1 On 2013/01/24 21:04:55, grt wrote: > nit: trailing comma Done. https://codereview.chromium.org/12035043/diff/16001/chrome/installer/util/eul... chrome/installer/util/eula_util.h:15: EULA_YES = 1 On 2013/01/24 21:04:55, grt wrote: > EULA_ACCEPTED Using QUERY_EULA_ prefix, because EULA_ACCEPTED was already defined in util_constants.h!
lg. please update the CL description. https://codereview.chromium.org/12035043/diff/9032/chrome/installer/util/eula... File chrome/installer/util/eula_util.cc (right): https://codereview.chromium.org/12035043/diff/9032/chrome/installer/util/eula... chrome/installer/util/eula_util.cc:57: EULAAcceptanceResponse IsEULAAccepted(bool system_level) { is this expected to be called when !system_level? https://codereview.chromium.org/12035043/diff/9032/chrome/installer/util/eula... chrome/installer/util/eula_util.cc:69: if (!prod_state.Initialize(system_level, why use false up above and system_level here?
Made fix and updated Description. PTAL. https://codereview.chromium.org/12035043/diff/9032/chrome/installer/util/eula... File chrome/installer/util/eula_util.cc (right): https://codereview.chromium.org/12035043/diff/9032/chrome/installer/util/eula... chrome/installer/util/eula_util.cc:57: EULAAcceptanceResponse IsEULAAccepted(bool system_level) { The main use case is for system-level, but since this is an app command, so we want to handle the user-level case. Otherwise we'll have to make the installer *not* add the app command on user-level install, which leads to move complexity. https://codereview.chromium.org/12035043/diff/9032/chrome/installer/util/eula... chrome/installer/util/eula_util.cc:69: if (!prod_state.Initialize(system_level, Using "true" now, and eliminated wrapping.
Needed to fix unit test. PTAL.
https://codereview.chromium.org/12035043/diff/9032/chrome/installer/util/eula... File chrome/installer/util/eula_util.cc (right): https://codereview.chromium.org/12035043/diff/9032/chrome/installer/util/eula... chrome/installer/util/eula_util.cc:57: EULAAcceptanceResponse IsEULAAccepted(bool system_level) { On 2013/01/28 19:54:13, huangs1 wrote: > The main use case is for system-level, but since this is an app command, so we > want to handle the user-level case. Otherwise we'll have to make the installer > *not* add the app command on user-level install, which leads to move complexity. Will the app command ever need to be invoked for a user-level install? If so, why? Chrome's EULA prompt is only for system-level Chrome. I think it's more confusing to have code in here for an irrelevant case than to only register the app command where it makes sense.
On 2013/01/29 15:25:18, grt wrote: > https://codereview.chromium.org/12035043/diff/9032/chrome/installer/util/eula... > File chrome/installer/util/eula_util.cc (right): > > https://codereview.chromium.org/12035043/diff/9032/chrome/installer/util/eula... > chrome/installer/util/eula_util.cc:57: EULAAcceptanceResponse > IsEULAAccepted(bool system_level) { > On 2013/01/28 19:54:13, huangs1 wrote: > > The main use case is for system-level, but since this is an app command, so we > > want to handle the user-level case. Otherwise we'll have to make the > installer > > *not* add the app command on user-level install, which leads to move > complexity. > > Will the app command ever need to be invoked for a user-level install? If so, > why? Chrome's EULA prompt is only for system-level Chrome. I think it's more > confusing to have code in here for an irrelevant case than to only register the > app command where it makes sense. My suggestion was to not make the webstore aware that this was not required for a user-level install. It seems like it breaks encapsulation and will lead to possible problems in the future. So I suggest that the webstore always call the command, even though it's guaranteed to return true for user-level.
https://codereview.chromium.org/12035043/diff/25002/chrome/installer/util/eul... File chrome/installer/util/eula_util.cc (right): https://codereview.chromium.org/12035043/diff/25002/chrome/installer/util/eul... chrome/installer/util/eula_util.cc:64: || prod_state.Initialize(false, BrowserDistribution::CHROME_APP_HOST) CHROME_FRAME?
PTAL. https://codereview.chromium.org/12035043/diff/25002/chrome/installer/util/eul... File chrome/installer/util/eula_util.cc (right): https://codereview.chromium.org/12035043/diff/25002/chrome/installer/util/eul... chrome/installer/util/eula_util.cc:64: || prod_state.Initialize(false, BrowserDistribution::CHROME_APP_HOST) Added and refactored.
apologies for the delay https://codereview.chromium.org/12035043/diff/21002/chrome/installer/util/eul... File chrome/installer/util/eula_util.cc (right): https://codereview.chromium.org/12035043/diff/21002/chrome/installer/util/eul... chrome/installer/util/eula_util.cc:71: // these prodcut at user-level implies that the EULA has been accepted. "product" -> "products" https://codereview.chromium.org/12035043/diff/21002/chrome/installer/util/eul... chrome/installer/util/eula_util.cc:79: if (!GetAnyChromeProductState(true, &prod_state)) i believe the fallback here must only be CHROME_BROWSER. it doesn't make sense to check for pending first-run on any other product. if you believe otherwise, please explain.
PTAL. https://codereview.chromium.org/12035043/diff/21002/chrome/installer/util/eul... File chrome/installer/util/eula_util.cc (right): https://codereview.chromium.org/12035043/diff/21002/chrome/installer/util/eul... chrome/installer/util/eula_util.cc:71: // these prodcut at user-level implies that the EULA has been accepted. On 2013/01/31 15:39:27, grt wrote: > "product" -> "products" Done. https://codereview.chromium.org/12035043/diff/21002/chrome/installer/util/eul... chrome/installer/util/eula_util.cc:79: if (!GetAnyChromeProductState(true, &prod_state)) Done.
lgtm w/ two nits https://codereview.chromium.org/12035043/diff/31001/chrome/installer/util/eul... File chrome/installer/util/eula_util.cc (right): https://codereview.chromium.org/12035043/diff/31001/chrome/installer/util/eul... chrome/installer/util/eula_util.cc:79: if (!prod_state.Initialize(system_level, you can get rid of the braces if you switch back to true rather than system_level (as in a previous patchset). https://codereview.chromium.org/12035043/diff/31001/chrome/installer/util/eul... chrome/installer/util/eula_util.cc:85: << "Binaries are not installed, but found multi-install product."; "found multi-install product" -> "Chrome is multi-install"
OWNER review for ben@ again (TBR'ed): Changed chrome_installer_util.gypi to add 2 files: chrome/installer/util/eula_util.h chrome/installer/util/eula_util.cc https://codereview.chromium.org/12035043/diff/31001/chrome/installer/util/eul... File chrome/installer/util/eula_util.cc (right): https://codereview.chromium.org/12035043/diff/31001/chrome/installer/util/eul... chrome/installer/util/eula_util.cc:79: if (!prod_state.Initialize(system_level, Doh! Done. https://codereview.chromium.org/12035043/diff/31001/chrome/installer/util/eul... chrome/installer/util/eula_util.cc:85: << "Binaries are not installed, but found multi-install product."; On 2013/01/31 21:09:48, grt wrote: > "found multi-install product" -> "Chrome is multi-install" Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/huangs@chromium.org/12035043/26006
Message was sent while issue was closed.
Change committed as 180052 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
