Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1986823002:
Reset user data directory and disk cache directory after downgrade. (Closed)
|
Created:
4 years, 7 months ago by zmin Modified:
4 years, 6 months ago CC:
chromium-reviews, tnagel+watch_chromium.org, chrome-enterprise-changes_google.com, Georges Khalil, sky Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionReset user data directory and disk cache directory after downgrade.
BUG=607592
Committed: https://crrev.com/18df9171296c5e7474adef49cd53d1638dbbb22c
Cr-Commit-Position: refs/heads/master@{#399604}
Patch Set 1 #Patch Set 2 : moving constants #Patch Set 3 : add code in mini_installer #
Total comments: 64
Patch Set 4 : cr #
Total comments: 27
Patch Set 5 : cr #
Total comments: 13
Patch Set 6 : cr+unittest #
Total comments: 12
Patch Set 7 : cr #
Total comments: 1
Patch Set 8 : add new OWNER file #
Total comments: 12
Patch Set 9 : cr #Patch Set 10 : browser_tests #
Total comments: 11
Patch Set 11 : cr #
Total comments: 1
Patch Set 12 : cr #
Total comments: 2
Patch Set 13 : cr #
Total comments: 2
Patch Set 14 : cr #
Total comments: 6
Patch Set 15 : #Patch Set 16 : fix trybot failure #Patch Set 17 : #Patch Set 18 : #Patch Set 19 : #
Total comments: 2
Patch Set 20 : #
Total comments: 21
Patch Set 21 : cr #Patch Set 22 : cr #
Total comments: 11
Patch Set 23 : #
Total comments: 1
Patch Set 24 : cr #Patch Set 25 : msi #
Total comments: 2
Patch Set 26 : #Messages
Total messages: 92 (13 generated)
zmin@chromium.org changed reviewers: + grt@chromium.org
Description was changed from ========== Reset user data directory and disk cache directory after downgrade. BUG=607592 ========== to ========== Reset user data directory and disk cache directory after downgrade. BUG=607592 ==========
a few preliminary comments. more to come. https://codereview.chromium.org/1986823002/diff/40001/chrome/browser/user_pro... File chrome/browser/user_profiles_cleanup.cc (right): https://codereview.chromium.org/1986823002/diff/40001/chrome/browser/user_pro... chrome/browser/user_profiles_cleanup.cc:29: namespace { nit: blank line after this https://codereview.chromium.org/1986823002/diff/40001/chrome/browser/user_pro... chrome/browser/user_profiles_cleanup.cc:30: base::FilePath::StringType g_delete_suffix = L".CHROME_DELETE"; static objects are banned since they introduce exit-time dtors. please use: constexpr base::FilePath::CharType kDeleteSuffix[] = FILE_PATH_LITERAL("..."); https://codereview.chromium.org/1986823002/diff/40001/chrome/browser/user_pro... File chrome/browser/user_profiles_cleanup.h (right): https://codereview.chromium.org/1986823002/diff/40001/chrome/browser/user_pro... chrome/browser/user_profiles_cleanup.h:10: void ResetUserProfilesAfterDowngrade(); (pre-comment: make function/method doc comments descriptive rather than imperative as per https://google.github.io/styleguide/cppguide.html#Function_Comments: "Renames the ...") "profile" is the wrong word since it's too easily confused with multi-profile. the user data directory is what we're talking about in practice. in the abstract, though, what we're doing here is removing all locally stored data because a downgrade has happened and we can't rely on an older version of Chrome being able to read data from a newer version. "AfterDowngrade" relates more to why this would be called rather than what it does. i think it's good to separate these two things: name this function for what it does, and document at the callsite why it is being called. in that light, what do you think about "MoveUserDataForFirstRun" or something like that to convey that this function moves all user data directories out of the way so that a first-run will commence? one thing that this leaves out is that a subsequent call to the Remove function below is needed. i think it's safe to cover this in the doc comment. as such, please explain in the doc comment that, while this function does involve blocking I/O, it does only preliminary work that must be followed up with a call to Remove... at some point after startup. maybe: // Moves aside all directories containing user data to prevent the browser from // reading them. When called early in startup, this will result in the browser // performing first-run for the user. To avoid long-running I/O operations, this // function merely moves directories aside. A subsequent call to // RemoveMovedUserData (below) must be made later in startup to free up disk // space. void MoveUserDataForFirstRun(); https://codereview.chromium.org/1986823002/diff/40001/chrome/browser/user_pro... chrome/browser/user_profiles_cleanup.h:12: // Delete the renamed user data directory and disk cache directory after Chrome suggestion: // Deletes any user data directories left behind after a previous Reset...AfterDowngrade. https://codereview.chromium.org/1986823002/diff/40001/chrome/browser/user_pro... chrome/browser/user_profiles_cleanup.h:14: void RemoveUnusedUserProfilesInBlockingPool(); this poses an interesting design decision: should the caller be responsible for picking the thread, or should the impl take care of it? in my world view, most of the browser lives on the main (UI) thread, except for all of the stuff that doesn't. :-) with that in mind, it's nice for the expectation to be that this function will be called from the main thread and that it will magically do the right thing. the fact that it may use the blocking pool is an implementation detail that doesn't need to bleed into the interface, so i'd leave that out of the function name. here's my suggestion based on the comment above (and inspired by SequencedTaskRunner::DeleteSoon): // Schedules a search for the removal of any directories moved aside by // MoveUserDataForFirstRun. This operation is idempotent, and may be safely // called when no such directories exist. void DeleteMovedUserDataSoon(); https://codereview.chromium.org/1986823002/diff/40001/chrome/installer/setup/... File chrome/installer/setup/install.cc (right): https://codereview.chromium.org/1986823002/diff/40001/chrome/installer/setup/... chrome/installer/setup/install.cc:292: return base::string16(L"Software\\Google\\Update\\ClientState\\") + use BrowserDistribution::GetStateKey() for this rather than computing it https://codereview.chromium.org/1986823002/diff/40001/chrome/installer/setup/... chrome/installer/setup/install.cc:296: void AddDowngradeVersion(HKEY root, const base::Version* existing_version) { is it possible to do this work somewhere in AddInstallWorkItems via WorkItems (maybe Conditional work items) so that rollback will bring them back to their previous values in case of error? https://codereview.chromium.org/1986823002/diff/40001/chrome/installer/setup/... chrome/installer/setup/install.cc:313: } nit: blank line after this
https://codereview.chromium.org/1986823002/diff/40001/chrome/browser/user_pro... File chrome/browser/user_profiles_cleanup.h (right): https://codereview.chromium.org/1986823002/diff/40001/chrome/browser/user_pro... chrome/browser/user_profiles_cleanup.h:10: void ResetUserProfilesAfterDowngrade(); On 2016/05/19 03:24:18, grt (slow) wrote: > (pre-comment: make function/method doc comments descriptive rather than > imperative as per > https://google.github.io/styleguide/cppguide.html#Function_Comments: "Renames > the ...") > > "profile" is the wrong word since it's too easily confused with multi-profile. > the user data directory is what we're talking about in practice. in the > abstract, though, what we're doing here is removing all locally stored data > because a downgrade has happened and we can't rely on an older version of Chrome > being able to read data from a newer version. > > "AfterDowngrade" relates more to why this would be called rather than what it > does. i think it's good to separate these two things: name this function for > what it does, and document at the callsite why it is being called. I wrote that before reading the code and seeing that this function does the downgrade detection. Please ignore this part. I think it is important to put "AfterDowngrade" or "OnDowngrade" or something in the name and document it. > in that light, what do you think about "MoveUserDataForFirstRun" or something > like that to convey that this function moves all user data directories out of > the way so that a first-run will commence? one thing that this leaves out is > that a subsequent call to the Remove function below is needed. i think it's safe > to cover this in the doc comment. as such, please explain in the doc comment > that, while this function does involve blocking I/O, it does only preliminary > work that must be followed up with a call to Remove... at some point after > startup. > > maybe: > > // Moves aside all directories containing user data to prevent the browser from > // reading them. When called early in startup, this will result in the browser > // performing first-run for the user. To avoid long-running I/O operations, this > // function merely moves directories aside. A subsequent call to > // RemoveMovedUserData (below) must be made later in startup to free up disk > // space. > void MoveUserDataForFirstRun();
this is a good start. thanks! https://codereview.chromium.org/1986823002/diff/40001/chrome/browser/user_pro... File chrome/browser/user_profiles_cleanup.cc (right): https://codereview.chromium.org/1986823002/diff/40001/chrome/browser/user_pro... chrome/browser/user_profiles_cleanup.cc:32: // Get disk cache dir override value if exists. Return empty path for default "Returns the..." https://codereview.chromium.org/1986823002/diff/40001/chrome/browser/user_pro... chrome/browser/user_profiles_cleanup.cc:46: base::Version GetDowngradeVersion() { this function belongs in chrome/installer/util somewhere. for the sake of keeping related code together (and unittested), i propose that you put all "interact with the downgrade version in the registry" functions together in install_util.{cc,h} with tests in install_util_unittest.cc. https://codereview.chromium.org/1986823002/diff/40001/chrome/browser/user_pro... chrome/browser/user_profiles_cleanup.cc:49: base::string16 key_path = BrowserDistribution::GetDistribution()->GetStateKey() https://codereview.chromium.org/1986823002/diff/40001/chrome/browser/user_pro... chrome/browser/user_profiles_cleanup.cc:52: if (key.Open(HKEY_LOCAL_MACHINE, key_path.c_str(), KEY_QUERY_VALUE) != Google Update interactions are always in the 32-bit hive: KEY_QUERY_VALUE | KEY_WOW64_32KEY https://codereview.chromium.org/1986823002/diff/40001/chrome/browser/user_pro... chrome/browser/user_profiles_cleanup.cc:52: if (key.Open(HKEY_LOCAL_MACHINE, key_path.c_str(), KEY_QUERY_VALUE) != use InstallUtil::IsPerUserInstall to determine whether to look in HKLM or HKCU rather than looking in both. https://codereview.chromium.org/1986823002/diff/40001/chrome/browser/user_pro... chrome/browser/user_profiles_cleanup.cc:68: return user_data_dir.Append(L"LastChromeVersion"); L"..." -> FILE_PATH_LITERAL("...") https://codereview.chromium.org/1986823002/diff/40001/chrome/browser/user_pro... chrome/browser/user_profiles_cleanup.cc:76: base::CollapseWhitespaceASCII(last_chrome_version_str, false)); why not TrimWhitespaceASCII? https://codereview.chromium.org/1986823002/diff/40001/chrome/browser/user_pro... chrome/browser/user_profiles_cleanup.cc:80: base::FilePath GetTempDirNameForDelete(const base::FilePath& source_path) { doc comment https://codereview.chromium.org/1986823002/diff/40001/chrome/browser/user_pro... chrome/browser/user_profiles_cleanup.cc:84: base::FilePath target_path(source_path.value() + g_delete_suffix); use FilePath methods whenever possible. is this equivalent to the above: base::FilePath target_path(source_path.AddExtension(g_delete_suffix)); ? https://codereview.chromium.org/1986823002/diff/40001/chrome/browser/user_pro... chrome/browser/user_profiles_cleanup.cc:86: if (uniquifier > 0) nit: add braces for multi-line body https://codereview.chromium.org/1986823002/diff/40001/chrome/browser/user_pro... chrome/browser/user_profiles_cleanup.cc:89: what if uniquifier < 0? https://codereview.chromium.org/1986823002/diff/40001/chrome/browser/user_pro... chrome/browser/user_profiles_cleanup.cc:93: void RenameDirectory(const base::FilePath& source, add a doc comment https://codereview.chromium.org/1986823002/diff/40001/chrome/browser/user_pro... chrome/browser/user_profiles_cleanup.cc:100: void DeleteDirectoryWithSuffix(base::FilePath path) { const-ref https://codereview.chromium.org/1986823002/diff/40001/chrome/browser/user_pro... chrome/browser/user_profiles_cleanup.cc:100: void DeleteDirectoryWithSuffix(base::FilePath path) { DeleteAllRenamedUserDirectories? https://codereview.chromium.org/1986823002/diff/40001/chrome/browser/user_pro... chrome/browser/user_profiles_cleanup.cc:112: for (base::FilePath file = enumerator.Next(); !file.empty(); file -> dir https://codereview.chromium.org/1986823002/diff/40001/chrome/browser/user_pro... chrome/browser/user_profiles_cleanup.cc:113: file = enumerator.Next()) nit: braces since the for() spans multiple lines https://codereview.chromium.org/1986823002/diff/40001/chrome/browser/user_pro... chrome/browser/user_profiles_cleanup.cc:129: base::Version current_version(CHROME_VERSION_STRING); CHROME_VERSION_STRING -> chrome::kChromeVersion https://codereview.chromium.org/1986823002/diff/40001/chrome/browser/user_pro... chrome/browser/user_profiles_cleanup.cc:131: if (GetDowngradeVersion() > current_version) { if (GetDowngradeVersion() > current_version && GetLastChromeVersion(user_data_dir) > current_version) { https://codereview.chromium.org/1986823002/diff/40001/chrome/browser/user_pro... chrome/browser/user_profiles_cleanup.cc:132: base::FilePath disk_cache_dir(GetDiskCacheDir()); keep locals in the narrowest scope possible (https://google.github.io/styleguide/cppguide.html#Local_Variables) https://codereview.chromium.org/1986823002/diff/40001/chrome/browser/user_pro... chrome/browser/user_profiles_cleanup.cc:138: // We don't have to recreate |disk_cache_dir| because it'll be initialized while use of personal pronouns isn't banned (it was discussed here: https://groups.google.com/a/chromium.org/d/msg/chromium-dev/NH-S6KCkr2M/-wTxR...), i am of the opinion that comments are usually more clear without "we" and such. for example: // Do not recreate the disk cache dir as it will be initialized later. https://codereview.chromium.org/1986823002/diff/40001/chrome/browser/user_pro... chrome/browser/user_profiles_cleanup.cc:145: current_version.GetString().c_str(), c_str() -> data() since you don't write the string terminator https://codereview.chromium.org/1986823002/diff/40001/chrome/browser/user_pro... chrome/browser/user_profiles_cleanup.cc:145: current_version.GetString().c_str(), stash current_version.GetString() in a local var so it isn't created twice for this call. better yet, use the original string: WriteFile(..., chrome::kChromeVersion, strlen(chrome::kChromeVersion)); https://codereview.chromium.org/1986823002/diff/40001/chrome/chrome_browser.gypi File chrome/chrome_browser.gypi (right): https://codereview.chromium.org/1986823002/diff/40001/chrome/chrome_browser.g... chrome/chrome_browser.gypi:1467: 'browser/user_profiles_cleanup.h', wdyt of user_data_downgrade.{cc,h}? https://codereview.chromium.org/1986823002/diff/40001/chrome/installer/util/g... File chrome/installer/util/google_update_constants.h (right): https://codereview.chromium.org/1986823002/diff/40001/chrome/installer/util/g... chrome/installer/util/google_update_constants.h:48: extern const wchar_t kRegDowngradeVersion[]; since this value is used purely by Chrome and isn't part of any of Google Update's registry-based API, i don't think it necessarily belongs here. it's okay for it to live with the impl somewhere (perhaps as a constant in an unnamed namespace in the .cc). https://codereview.chromium.org/1986823002/diff/40001/components/policy/resou... File components/policy/resources/policy_templates.json (right): https://codereview.chromium.org/1986823002/diff/40001/components/policy/resou... components/policy/resources/policy_templates.json:1420: Setting this policy to root directory or using this directory for any other purposes are not recommanded.''', let's be even more direct: "This policy must not be set to a volume's root directory, and should not be set to a directory used for other purposes."
https://codereview.chromium.org/1986823002/diff/40001/chrome/browser/user_pro... File chrome/browser/user_profiles_cleanup.cc (right): https://codereview.chromium.org/1986823002/diff/40001/chrome/browser/user_pro... chrome/browser/user_profiles_cleanup.cc:29: namespace { On 2016/05/19 03:24:18, grt (slow) wrote: > nit: blank line after this Done. https://codereview.chromium.org/1986823002/diff/40001/chrome/browser/user_pro... chrome/browser/user_profiles_cleanup.cc:30: base::FilePath::StringType g_delete_suffix = L".CHROME_DELETE"; On 2016/05/19 03:24:18, grt (slow) wrote: > static objects are banned since they introduce exit-time dtors. please use: > > constexpr base::FilePath::CharType kDeleteSuffix[] = FILE_PATH_LITERAL("..."); Done. https://codereview.chromium.org/1986823002/diff/40001/chrome/browser/user_pro... chrome/browser/user_profiles_cleanup.cc:32: // Get disk cache dir override value if exists. Return empty path for default On 2016/05/19 15:37:28, grt (slow) wrote: > "Returns the..." Done. https://codereview.chromium.org/1986823002/diff/40001/chrome/browser/user_pro... chrome/browser/user_profiles_cleanup.cc:46: base::Version GetDowngradeVersion() { On 2016/05/19 15:37:27, grt (slow) wrote: > this function belongs in chrome/installer/util somewhere. for the sake of > keeping related code together (and unittested), i propose that you put all > "interact with the downgrade version in the registry" functions together in > install_util.{cc,h} with tests in install_util_unittest.cc. Done. Unit test will be uploaded later as I need to find a way to change the state_path . Right now I get the state path by BrowserDistribution::GetDistribution()->GetStateKey() as you suggested below which is a static function. https://codereview.chromium.org/1986823002/diff/40001/chrome/browser/user_pro... chrome/browser/user_profiles_cleanup.cc:49: base::string16 key_path = On 2016/05/19 15:37:28, grt (slow) wrote: > BrowserDistribution::GetDistribution()->GetStateKey() Done. https://codereview.chromium.org/1986823002/diff/40001/chrome/browser/user_pro... chrome/browser/user_profiles_cleanup.cc:52: if (key.Open(HKEY_LOCAL_MACHINE, key_path.c_str(), KEY_QUERY_VALUE) != On 2016/05/19 15:37:27, grt (slow) wrote: > Google Update interactions are always in the 32-bit hive: > KEY_QUERY_VALUE | KEY_WOW64_32KEY Done. https://codereview.chromium.org/1986823002/diff/40001/chrome/browser/user_pro... chrome/browser/user_profiles_cleanup.cc:52: if (key.Open(HKEY_LOCAL_MACHINE, key_path.c_str(), KEY_QUERY_VALUE) != On 2016/05/19 15:37:28, grt (slow) wrote: > use InstallUtil::IsPerUserInstall to determine whether to look in HKLM or HKCU > rather than looking in both. Done. https://codereview.chromium.org/1986823002/diff/40001/chrome/browser/user_pro... chrome/browser/user_profiles_cleanup.cc:68: return user_data_dir.Append(L"LastChromeVersion"); On 2016/05/19 15:37:28, grt (slow) wrote: > L"..." -> FILE_PATH_LITERAL("...") Done. https://codereview.chromium.org/1986823002/diff/40001/chrome/browser/user_pro... chrome/browser/user_profiles_cleanup.cc:76: base::CollapseWhitespaceASCII(last_chrome_version_str, false)); On 2016/05/19 15:37:28, grt (slow) wrote: > why not TrimWhitespaceASCII? Done. https://codereview.chromium.org/1986823002/diff/40001/chrome/browser/user_pro... chrome/browser/user_profiles_cleanup.cc:80: base::FilePath GetTempDirNameForDelete(const base::FilePath& source_path) { On 2016/05/19 15:37:28, grt (slow) wrote: > doc comment Done. https://codereview.chromium.org/1986823002/diff/40001/chrome/browser/user_pro... chrome/browser/user_profiles_cleanup.cc:84: base::FilePath target_path(source_path.value() + g_delete_suffix); On 2016/05/19 15:37:27, grt (slow) wrote: > use FilePath methods whenever possible. is this equivalent to the above: > base::FilePath target_path(source_path.AddExtension(g_delete_suffix)); > ? Done. https://codereview.chromium.org/1986823002/diff/40001/chrome/browser/user_pro... chrome/browser/user_profiles_cleanup.cc:86: if (uniquifier > 0) On 2016/05/19 15:37:27, grt (slow) wrote: > nit: add braces for multi-line body Done. https://codereview.chromium.org/1986823002/diff/40001/chrome/browser/user_pro... chrome/browser/user_profiles_cleanup.cc:89: On 2016/05/19 15:37:27, grt (slow) wrote: > what if uniquifier < 0? Good question. <0 means the dir have been copied 100 times without deletion. We can return an empty path and do not rename dir if that happened. https://codereview.chromium.org/1986823002/diff/40001/chrome/browser/user_pro... chrome/browser/user_profiles_cleanup.cc:93: void RenameDirectory(const base::FilePath& source, On 2016/05/19 15:37:28, grt (slow) wrote: > add a doc comment Done. https://codereview.chromium.org/1986823002/diff/40001/chrome/browser/user_pro... chrome/browser/user_profiles_cleanup.cc:100: void DeleteDirectoryWithSuffix(base::FilePath path) { On 2016/05/19 15:37:27, grt (slow) wrote: > const-ref Done. https://codereview.chromium.org/1986823002/diff/40001/chrome/browser/user_pro... chrome/browser/user_profiles_cleanup.cc:100: void DeleteDirectoryWithSuffix(base::FilePath path) { On 2016/05/19 15:37:27, grt (slow) wrote: > DeleteAllRenamedUserDirectories? Done. https://codereview.chromium.org/1986823002/diff/40001/chrome/browser/user_pro... chrome/browser/user_profiles_cleanup.cc:112: for (base::FilePath file = enumerator.Next(); !file.empty(); On 2016/05/19 15:37:28, grt (slow) wrote: > file -> dir Done. https://codereview.chromium.org/1986823002/diff/40001/chrome/browser/user_pro... chrome/browser/user_profiles_cleanup.cc:113: file = enumerator.Next()) On 2016/05/19 15:37:27, grt (slow) wrote: > nit: braces since the for() spans multiple lines Done. https://codereview.chromium.org/1986823002/diff/40001/chrome/browser/user_pro... chrome/browser/user_profiles_cleanup.cc:129: base::Version current_version(CHROME_VERSION_STRING); On 2016/05/19 15:37:28, grt (slow) wrote: > CHROME_VERSION_STRING -> chrome::kChromeVersion Done. https://codereview.chromium.org/1986823002/diff/40001/chrome/browser/user_pro... chrome/browser/user_profiles_cleanup.cc:132: base::FilePath disk_cache_dir(GetDiskCacheDir()); On 2016/05/19 15:37:28, grt (slow) wrote: > keep locals in the narrowest scope possible > (https://google.github.io/styleguide/cppguide.html#Local_Variables) More comments added https://codereview.chromium.org/1986823002/diff/40001/chrome/browser/user_pro... chrome/browser/user_profiles_cleanup.cc:138: // We don't have to recreate |disk_cache_dir| because it'll be initialized On 2016/05/19 15:37:27, grt (slow) wrote: > while use of personal pronouns isn't banned (it was discussed here: > https://groups.google.com/a/chromium.org/d/msg/chromium-dev/NH-S6KCkr2M/-wTxR...), > i am of the opinion that comments are usually more clear without "we" and such. > for example: > // Do not recreate the disk cache dir as it will be initialized later. Done. https://codereview.chromium.org/1986823002/diff/40001/chrome/browser/user_pro... chrome/browser/user_profiles_cleanup.cc:145: current_version.GetString().c_str(), On 2016/05/19 15:37:28, grt (slow) wrote: > stash current_version.GetString() in a local var so it isn't created twice for > this call. > > better yet, use the original string: > WriteFile(..., chrome::kChromeVersion, strlen(chrome::kChromeVersion)); Done. https://codereview.chromium.org/1986823002/diff/40001/chrome/browser/user_pro... File chrome/browser/user_profiles_cleanup.h (right): https://codereview.chromium.org/1986823002/diff/40001/chrome/browser/user_pro... chrome/browser/user_profiles_cleanup.h:10: void ResetUserProfilesAfterDowngrade(); On 2016/05/19 14:08:05, grt (slow) wrote: > On 2016/05/19 03:24:18, grt (slow) wrote: > > (pre-comment: make function/method doc comments descriptive rather than > > imperative as per > > https://google.github.io/styleguide/cppguide.html#Function_Comments: "Renames > > the ...") > > > > "profile" is the wrong word since it's too easily confused with multi-profile. > > the user data directory is what we're talking about in practice. in the > > abstract, though, what we're doing here is removing all locally stored data > > because a downgrade has happened and we can't rely on an older version of > Chrome > > being able to read data from a newer version. > > > > "AfterDowngrade" relates more to why this would be called rather than what it > > does. i think it's good to separate these two things: name this function for > > what it does, and document at the callsite why it is being called. > > I wrote that before reading the code and seeing that this function does the > downgrade detection. Please ignore this part. I think it is important to put > "AfterDowngrade" or "OnDowngrade" or something in the name and document it. > > > in that light, what do you think about "MoveUserDataForFirstRun" or something > > like that to convey that this function moves all user data directories out of > > the way so that a first-run will commence? one thing that this leaves out is > > that a subsequent call to the Remove function below is needed. i think it's > safe > > to cover this in the doc comment. as such, please explain in the doc comment > > that, while this function does involve blocking I/O, it does only preliminary > > work that must be followed up with a call to Remove... at some point after > > startup. > > > > maybe: > > > > // Moves aside all directories containing user data to prevent the browser > from > > // reading them. When called early in startup, this will result in the browser > > // performing first-run for the user. To avoid long-running I/O operations, > this > > // function merely moves directories aside. A subsequent call to > > // RemoveMovedUserData (below) must be made later in startup to free up disk > > // space. > > void MoveUserDataForFirstRun(); > Change the name to MoveUserDataForFirstRunAfterDowngrade(). Also mentioned downgrade in the comments. https://codereview.chromium.org/1986823002/diff/40001/chrome/browser/user_pro... chrome/browser/user_profiles_cleanup.h:12: // Delete the renamed user data directory and disk cache directory after Chrome On 2016/05/19 03:24:18, grt (slow) wrote: > suggestion: > // Deletes any user data directories left behind after a previous > Reset...AfterDowngrade. Done. https://codereview.chromium.org/1986823002/diff/40001/chrome/browser/user_pro... chrome/browser/user_profiles_cleanup.h:14: void RemoveUnusedUserProfilesInBlockingPool(); On 2016/05/19 03:24:18, grt (slow) wrote: > this poses an interesting design decision: should the caller be responsible for > picking the thread, or should the impl take care of it? in my world view, most > of the browser lives on the main (UI) thread, except for all of the stuff that > doesn't. :-) with that in mind, it's nice for the expectation to be that this > function will be called from the main thread and that it will magically do the > right thing. the fact that it may use the blocking pool is an implementation > detail that doesn't need to bleed into the interface, so i'd leave that out of > the function name. > > here's my suggestion based on the comment above (and inspired by > SequencedTaskRunner::DeleteSoon): > // Schedules a search for the removal of any directories moved aside by > // MoveUserDataForFirstRun. This operation is idempotent, and may be safely > // called when no such directories exist. > void DeleteMovedUserDataSoon(); Done. https://codereview.chromium.org/1986823002/diff/40001/chrome/chrome_browser.gypi File chrome/chrome_browser.gypi (right): https://codereview.chromium.org/1986823002/diff/40001/chrome/chrome_browser.g... chrome/chrome_browser.gypi:1467: 'browser/user_profiles_cleanup.h', On 2016/05/19 15:37:28, grt (slow) wrote: > wdyt of user_data_downgrade.{cc,h}? i think data is better because we use 'data' in the function name. And 'downgrade' is a better name https://codereview.chromium.org/1986823002/diff/40001/chrome/installer/setup/... File chrome/installer/setup/install.cc (right): https://codereview.chromium.org/1986823002/diff/40001/chrome/installer/setup/... chrome/installer/setup/install.cc:292: return base::string16(L"Software\\Google\\Update\\ClientState\\") + On 2016/05/19 03:24:18, grt (slow) wrote: > use BrowserDistribution::GetStateKey() for this rather than computing it Done. Also changed in user_profiles_cleanup.cc https://codereview.chromium.org/1986823002/diff/40001/chrome/installer/setup/... chrome/installer/setup/install.cc:313: } On 2016/05/19 03:24:18, grt (slow) wrote: > nit: blank line after this Done. https://codereview.chromium.org/1986823002/diff/40001/chrome/installer/util/g... File chrome/installer/util/google_update_constants.h (right): https://codereview.chromium.org/1986823002/diff/40001/chrome/installer/util/g... chrome/installer/util/google_update_constants.h:48: extern const wchar_t kRegDowngradeVersion[]; On 2016/05/19 15:37:28, grt (slow) wrote: > since this value is used purely by Chrome and isn't part of any of Google > Update's registry-based API, i don't think it necessarily belongs here. it's > okay for it to live with the impl somewhere (perhaps as a constant in an unnamed > namespace in the .cc). Move this to install_util.h as all related code are moved to there. https://codereview.chromium.org/1986823002/diff/40001/components/policy/resou... File components/policy/resources/policy_templates.json (right): https://codereview.chromium.org/1986823002/diff/40001/components/policy/resou... components/policy/resources/policy_templates.json:1420: Setting this policy to root directory or using this directory for any other purposes are not recommanded.''', On 2016/05/19 15:37:28, grt (slow) wrote: > let's be even more direct: "This policy must not be set to a volume's root > directory, and should not be set to a directory used for other purposes." Done.
a few comments https://codereview.chromium.org/1986823002/diff/60001/chrome/browser/chrome_b... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/1986823002/diff/60001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main.cc:1789: // Clean up old user data directory and disk cache directory nit: '.' at end of sentence https://codereview.chromium.org/1986823002/diff/60001/chrome/browser/policy/p... File chrome/browser/policy/policy_path_parser.h (right): https://codereview.chromium.org/1986823002/diff/60001/chrome/browser/policy/p... chrome/browser/policy/policy_path_parser.h:67: #if defined(OS_WIN) although it's only used on Win, it's cleaner to declare/define it on all platforms https://codereview.chromium.org/1986823002/diff/60001/chrome/browser/policy/p... File chrome/browser/policy/policy_path_parser_win.cc (right): https://codereview.chromium.org/1986823002/diff/60001/chrome/browser/policy/p... chrome/browser/policy/policy_path_parser_win.cc:153: HKEY_LOCAL_MACHINE, policy::key::kUserDataDir, user_data_dir)) nit: braces for multi-line conditional and body (below, too) https://codereview.chromium.org/1986823002/diff/60001/chrome/browser/policy/p... chrome/browser/policy/policy_path_parser_win.cc:153: HKEY_LOCAL_MACHINE, policy::key::kUserDataDir, user_data_dir)) nit: "policy::" isn't needed since this function is within the policy::path_parser namespace https://codereview.chromium.org/1986823002/diff/60001/chrome/browser/user_dat... File chrome/browser/user_data_downgrade.cc (right): https://codereview.chromium.org/1986823002/diff/60001/chrome/browser/user_dat... chrome/browser/user_data_downgrade.cc:57: base::TrimWhitespaceASCII(last_chrome_version_str, base::TRIM_TRAILING) if you're going to trim whitespace to be conservative (it is never written by Chrome), why not TRIM_ALL? https://codereview.chromium.org/1986823002/diff/60001/chrome/browser/user_dat... chrome/browser/user_data_downgrade.cc:86: recreate) nit: braces https://codereview.chromium.org/1986823002/diff/60001/chrome/browser/user_dat... chrome/browser/user_data_downgrade.cc:131: // directory's name will be get before the last chrome version . "will be get before the last chrome version" -> "will be computed before the LastChromeVersion file is read." https://codereview.chromium.org/1986823002/diff/60001/chrome/browser/user_dat... chrome/browser/user_data_downgrade.cc:132: // Because the deletion will be scheduled after singleton, the directory "...after the singleton is acquired," https://codereview.chromium.org/1986823002/diff/60001/chrome/installer/setup/... File chrome/installer/setup/install_worker.cc (right): https://codereview.chromium.org/1986823002/diff/60001/chrome/installer/setup/... chrome/installer/setup/install_worker.cc:253: InstallUtil::AddSetDowngradeVersionItem(installer_state.root_key(), if you collapse these two functions into one AddDowngradeVersionItems that does the version comparison, i think it'll be easier to make a unittest that covers all the bases. https://codereview.chromium.org/1986823002/diff/60001/chrome/installer/util/i... File chrome/installer/util/install_util.cc (right): https://codereview.chromium.org/1986823002/diff/60001/chrome/installer/util/i... chrome/installer/util/install_util.cc:632: base::Version InstallUtil::GetDowngradeVersion(HKEY root) { i think it's cleaner for this to take a bool system_install rather than the HKEY. it should also take the const AppRegistrationData& as mentioned below https://codereview.chromium.org/1986823002/diff/60001/chrome/installer/util/i... chrome/installer/util/install_util.cc:646: void InstallUtil::AddSetDowngradeVersionItem( this should take a const AppRegistrationData& as input rather than call BrowserDistribution::GetDistribution(). same for below.
Patchset #5 (id:80001) has been deleted
https://codereview.chromium.org/1986823002/diff/60001/chrome/browser/chrome_b... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/1986823002/diff/60001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main.cc:1789: // Clean up old user data directory and disk cache directory On 2016/05/19 23:39:27, grt (slow) wrote: > nit: '.' at end of sentence Done. https://codereview.chromium.org/1986823002/diff/60001/chrome/browser/policy/p... File chrome/browser/policy/policy_path_parser.h (right): https://codereview.chromium.org/1986823002/diff/60001/chrome/browser/policy/p... chrome/browser/policy/policy_path_parser.h:67: #if defined(OS_WIN) On 2016/05/19 23:39:27, grt (slow) wrote: > although it's only used on Win, it's cleaner to declare/define it on all > platforms Done. But I'll leave Linux/Mac version as unimplemented because no one will use this function on those platform. https://codereview.chromium.org/1986823002/diff/60001/chrome/browser/policy/p... File chrome/browser/policy/policy_path_parser_win.cc (right): https://codereview.chromium.org/1986823002/diff/60001/chrome/browser/policy/p... chrome/browser/policy/policy_path_parser_win.cc:153: HKEY_LOCAL_MACHINE, policy::key::kUserDataDir, user_data_dir)) On 2016/05/19 23:39:27, grt (slow) wrote: > nit: braces for multi-line conditional and body (below, too) Done. https://codereview.chromium.org/1986823002/diff/60001/chrome/browser/policy/p... chrome/browser/policy/policy_path_parser_win.cc:153: HKEY_LOCAL_MACHINE, policy::key::kUserDataDir, user_data_dir)) On 2016/05/19 23:39:27, grt (slow) wrote: > nit: "policy::" isn't needed since this function is within the > policy::path_parser namespace Done. https://codereview.chromium.org/1986823002/diff/60001/chrome/browser/user_dat... File chrome/browser/user_data_downgrade.cc (right): https://codereview.chromium.org/1986823002/diff/60001/chrome/browser/user_dat... chrome/browser/user_data_downgrade.cc:57: base::TrimWhitespaceASCII(last_chrome_version_str, base::TRIM_TRAILING) On 2016/05/19 23:39:28, grt (slow) wrote: > if you're going to trim whitespace to be conservative (it is never written by > Chrome), why not TRIM_ALL? Done. https://codereview.chromium.org/1986823002/diff/60001/chrome/browser/user_dat... chrome/browser/user_data_downgrade.cc:86: recreate) On 2016/05/19 23:39:27, grt (slow) wrote: > nit: braces Done. https://codereview.chromium.org/1986823002/diff/60001/chrome/browser/user_dat... chrome/browser/user_data_downgrade.cc:131: // directory's name will be get before the last chrome version . On 2016/05/19 23:39:28, grt (slow) wrote: > "will be get before the last chrome version" -> "will be computed before the > LastChromeVersion file is read." Done. https://codereview.chromium.org/1986823002/diff/60001/chrome/browser/user_dat... chrome/browser/user_data_downgrade.cc:132: // Because the deletion will be scheduled after singleton, the directory On 2016/05/19 23:39:27, grt (slow) wrote: > "...after the singleton is acquired," Done. https://codereview.chromium.org/1986823002/diff/60001/chrome/installer/setup/... File chrome/installer/setup/install_worker.cc (right): https://codereview.chromium.org/1986823002/diff/60001/chrome/installer/setup/... chrome/installer/setup/install_worker.cc:253: InstallUtil::AddSetDowngradeVersionItem(installer_state.root_key(), On 2016/05/19 23:39:28, grt (slow) wrote: > if you collapse these two functions into one AddDowngradeVersionItems that does > the version comparison, i think it'll be easier to make a unittest that covers > all the bases. Done. https://codereview.chromium.org/1986823002/diff/60001/chrome/installer/util/i... File chrome/installer/util/install_util.cc (right): https://codereview.chromium.org/1986823002/diff/60001/chrome/installer/util/i... chrome/installer/util/install_util.cc:632: base::Version InstallUtil::GetDowngradeVersion(HKEY root) { On 2016/05/19 23:39:28, grt (slow) wrote: > i think it's cleaner for this to take a bool system_install rather than the > HKEY. it should also take the const AppRegistrationData& as mentioned below I use HKEY instead of bool here because 1) Make it same as the functions below. The AddDowngradeVersionItem function is called in install_worker.cc which the root key is get directly from installer_state.root_key(). 2) This function is also called by AddDowngradeVersionItem(), using HKEY avoid converting between bool and HKEY https://codereview.chromium.org/1986823002/diff/60001/chrome/installer/util/i... chrome/installer/util/install_util.cc:646: void InstallUtil::AddSetDowngradeVersionItem( On 2016/05/19 23:39:28, grt (slow) wrote: > this should take a const AppRegistrationData& as input rather than call > BrowserDistribution::GetDistribution(). same for below. Done. But I'll use BrowserDistribution* which is a wrapper of AppRegistrationData.
https://codereview.chromium.org/1986823002/diff/60001/chrome/browser/policy/p... File chrome/browser/policy/policy_path_parser.h (right): https://codereview.chromium.org/1986823002/diff/60001/chrome/browser/policy/p... chrome/browser/policy/policy_path_parser.h:67: #if defined(OS_WIN) On 2016/05/20 00:52:25, zmin wrote: > On 2016/05/19 23:39:27, grt (slow) wrote: > > although it's only used on Win, it's cleaner to declare/define it on all > > platforms > > Done. But I'll leave Linux/Mac version as unimplemented because no one will use > this function on those platform. Acknowledged. https://codereview.chromium.org/1986823002/diff/60001/chrome/installer/util/i... File chrome/installer/util/install_util.cc (right): https://codereview.chromium.org/1986823002/diff/60001/chrome/installer/util/i... chrome/installer/util/install_util.cc:632: base::Version InstallUtil::GetDowngradeVersion(HKEY root) { On 2016/05/20 00:52:25, zmin wrote: > On 2016/05/19 23:39:28, grt (slow) wrote: > > i think it's cleaner for this to take a bool system_install rather than the > > HKEY. it should also take the const AppRegistrationData& as mentioned below > > I use HKEY instead of bool here because > 1) Make it same as the functions below. The AddDowngradeVersionItem function is > called in install_worker.cc which the root key is get directly from > installer_state.root_key(). > 2) This function is also called by AddDowngradeVersionItem(), using HKEY avoid > converting between bool and HKEY using HKEY leaks an implementation detail into the interface. why should MoveUserDataForFirstRunAfterDowngrade need to know that the value is stored in the registry, and know how to figure out which hive to look in? i think it's fine if this takes a bool and the fn below takes an HKEY. also fine to to the bool->HKEY here and down below. could also have the fn below take a const InstallerState& and use its root_key() method, but that may make unittesting more difficult. https://codereview.chromium.org/1986823002/diff/60001/chrome/installer/util/i... chrome/installer/util/install_util.cc:646: void InstallUtil::AddSetDowngradeVersionItem( On 2016/05/20 00:52:25, zmin wrote: > On 2016/05/19 23:39:28, grt (slow) wrote: > > this should take a const AppRegistrationData& as input rather than call > > BrowserDistribution::GetDistribution(). same for below. > > Done. But I'll use BrowserDistribution* which is a wrapper of > AppRegistrationData. Why? I think it'll be easier to unittest using ARD. https://codereview.chromium.org/1986823002/diff/100001/chrome/browser/user_da... File chrome/browser/user_data_downgrade.cc (right): https://codereview.chromium.org/1986823002/diff/100001/chrome/browser/user_da... chrome/browser/user_data_downgrade.cc:134: // required, the directory can only be moved twice in the worst case. required -> acquired https://codereview.chromium.org/1986823002/diff/100001/chrome/installer/setup... File chrome/installer/setup/install_worker.cc (right): https://codereview.chromium.org/1986823002/diff/100001/chrome/installer/setup... chrome/installer/setup/install_worker.cc:253: *current_version, new_version, |current_version| is null on a new install, so you'll have a crash here. in this case, i think the downgrade version should be removed from the registry; we shouldn't treat an uninstall of N followed by an install of N-1 as a downgrade. https://codereview.chromium.org/1986823002/diff/100001/chrome/installer/util/... File chrome/installer/util/install_util.cc (right): https://codereview.chromium.org/1986823002/diff/100001/chrome/installer/util/... chrome/installer/util/install_util.cc:660: base::ASCIIToUTF16(current_version.GetString()), false); false -> true since you want to overwrite if there happens to be bogus data in the value. https://codereview.chromium.org/1986823002/diff/100001/chrome/installer/util/... chrome/installer/util/install_util.cc:661: } else if (current_version < new_version && downgrade_version.IsValid() && |downgrade_version| will be invalid if either the value is not present in the registry, or if it a bogus value is present. i think it's desirable to delete a bogus value. with that in mind, what do you think about changing this to: } else if (current_version < new_version && (!downgrade_version.IsValid() || downgrade_version <= new_version)) { https://codereview.chromium.org/1986823002/diff/100001/chrome/installer/util/... File chrome/installer/util/install_util.h (right): https://codereview.chromium.org/1986823002/diff/100001/chrome/installer/util/... chrome/installer/util/install_util.h:186: // Return downgrade version if the value exists or empty version. "Returns..." as per https://google.github.io/styleguide/cppguide.html#Function_Comments. also might be nice to explain a bit more about what this value means. how about: // Returns the highest Chrome version that was installed prior to a downgrade, // or an invalid Version if Chrome was not previously downgraded from a newer // version. https://codereview.chromium.org/1986823002/diff/100001/chrome/installer/util/... chrome/installer/util/install_util.h:190: // Add or remove downgrade version registry value. This function should only "Adds or removes..."
https://codereview.chromium.org/1986823002/diff/60001/chrome/installer/util/i... File chrome/installer/util/install_util.cc (right): https://codereview.chromium.org/1986823002/diff/60001/chrome/installer/util/i... chrome/installer/util/install_util.cc:632: base::Version InstallUtil::GetDowngradeVersion(HKEY root) { On 2016/05/20 15:25:05, grt (slow) wrote: > On 2016/05/20 00:52:25, zmin wrote: > > On 2016/05/19 23:39:28, grt (slow) wrote: > > > i think it's cleaner for this to take a bool system_install rather than the > > > HKEY. it should also take the const AppRegistrationData& as mentioned below > > > > I use HKEY instead of bool here because > > 1) Make it same as the functions below. The AddDowngradeVersionItem function > is > > called in install_worker.cc which the root key is get directly from > > installer_state.root_key(). > > 2) This function is also called by AddDowngradeVersionItem(), using HKEY avoid > > converting between bool and HKEY > > using HKEY leaks an implementation detail into the interface. why should > MoveUserDataForFirstRunAfterDowngrade need to know that the value is stored in > the registry, and know how to figure out which hive to look in? i think it's > fine if this takes a bool and the fn below takes an HKEY. also fine to to the > bool->HKEY here and down below. could also have the fn below take a const > InstallerState& and use its root_key() method, but that may make unittesting > more difficult. Sure, I'll do bool for both. https://codereview.chromium.org/1986823002/diff/60001/chrome/installer/util/i... chrome/installer/util/install_util.cc:646: void InstallUtil::AddSetDowngradeVersionItem( On 2016/05/20 15:25:05, grt (slow) wrote: > On 2016/05/20 00:52:25, zmin wrote: > > On 2016/05/19 23:39:28, grt (slow) wrote: > > > this should take a const AppRegistrationData& as input rather than call > > > BrowserDistribution::GetDistribution(). same for below. > > > > Done. But I'll use BrowserDistribution* which is a wrapper of > > AppRegistrationData. > > Why? I think it'll be easier to unittest using ARD. There is no much difference. BrowserDistribution->GetStateKey just simply call ARD->GetStateKey. Unittest works fine with BrowserDistribution. https://codereview.chromium.org/1986823002/diff/100001/chrome/browser/user_da... File chrome/browser/user_data_downgrade.cc (right): https://codereview.chromium.org/1986823002/diff/100001/chrome/browser/user_da... chrome/browser/user_data_downgrade.cc:134: // required, the directory can only be moved twice in the worst case. On 2016/05/20 15:25:05, grt (slow) wrote: > required -> acquired Done. https://codereview.chromium.org/1986823002/diff/100001/chrome/installer/setup... File chrome/installer/setup/install_worker.cc (right): https://codereview.chromium.org/1986823002/diff/100001/chrome/installer/setup... chrome/installer/setup/install_worker.cc:253: *current_version, new_version, On 2016/05/20 15:25:05, grt (slow) wrote: > |current_version| is null on a new install, so you'll have a crash here. in this > case, i think the downgrade version should be removed from the registry; we > shouldn't treat an uninstall of N followed by an install of N-1 as a downgrade. make sense, I'll update it. https://codereview.chromium.org/1986823002/diff/100001/chrome/installer/util/... File chrome/installer/util/install_util.cc (right): https://codereview.chromium.org/1986823002/diff/100001/chrome/installer/util/... chrome/installer/util/install_util.cc:660: base::ASCIIToUTF16(current_version.GetString()), false); On 2016/05/20 15:25:05, grt (slow) wrote: > false -> true since you want to overwrite if there happens to be bogus data in > the value. Done. https://codereview.chromium.org/1986823002/diff/100001/chrome/installer/util/... chrome/installer/util/install_util.cc:661: } else if (current_version < new_version && downgrade_version.IsValid() && On 2016/05/20 15:25:05, grt (slow) wrote: > |downgrade_version| will be invalid if either the value is not present in the > registry, or if it a bogus value is present. i think it's desirable to delete a > bogus value. with that in mind, what do you think about changing this to: > } else if (current_version < new_version && > (!downgrade_version.IsValid() || > downgrade_version <= new_version)) { That means for every upgrade(even for the ppl never downgrade chrome), we'll try to delete a value that never existed. If bogus value is something happened before for other registry value and the cost of deletion is tiny, I'll agree with that. https://codereview.chromium.org/1986823002/diff/100001/chrome/installer/util/... File chrome/installer/util/install_util.h (right): https://codereview.chromium.org/1986823002/diff/100001/chrome/installer/util/... chrome/installer/util/install_util.h:186: // Return downgrade version if the value exists or empty version. On 2016/05/20 15:25:05, grt (slow) wrote: > "Returns..." as per > https://google.github.io/styleguide/cppguide.html#Function_Comments. > > also might be nice to explain a bit more about what this value means. how about: > > // Returns the highest Chrome version that was installed prior to a downgrade, > // or an invalid Version if Chrome was not previously downgraded from a newer > // version. Done. https://codereview.chromium.org/1986823002/diff/100001/chrome/installer/util/... chrome/installer/util/install_util.h:190: // Add or remove downgrade version registry value. This function should only On 2016/05/20 15:25:05, grt (slow) wrote: > "Adds or removes..." Done.
https://codereview.chromium.org/1986823002/diff/100001/chrome/installer/util/... File chrome/installer/util/install_util.cc (right): https://codereview.chromium.org/1986823002/diff/100001/chrome/installer/util/... chrome/installer/util/install_util.cc:661: } else if (current_version < new_version && downgrade_version.IsValid() && On 2016/05/20 16:33:05, zmin wrote: > On 2016/05/20 15:25:05, grt (slow) wrote: > > |downgrade_version| will be invalid if either the value is not present in the > > registry, or if it a bogus value is present. i think it's desirable to delete > a > > bogus value. with that in mind, what do you think about changing this to: > > } else if (current_version < new_version && > > (!downgrade_version.IsValid() || > > downgrade_version <= new_version)) { > > That means for every upgrade(even for the ppl never downgrade chrome), we'll try > to delete a value that never existed. If bogus value is something happened > before for other registry value and the cost of deletion is tiny, I'll agree > with that. the cost of the delete is insignificant. strange things happen in the registry on a regular basis, so it's best to be defensive. https://codereview.chromium.org/1986823002/diff/120001/chrome/installer/util/... File chrome/installer/util/install_util.cc (right): https://codereview.chromium.org/1986823002/diff/120001/chrome/installer/util/... chrome/installer/util/install_util.cc:656: DCHECK(dist); to enforce the "only call this for Chrome", add: DCHECK_EQ(BrowserDistribution::CHROME_BROWSER, dist->GetType()); https://codereview.chromium.org/1986823002/diff/120001/chrome/installer/util/... chrome/installer/util/install_util.cc:659: if (current_version == nullptr || nit: if (!current_version https://codereview.chromium.org/1986823002/diff/120001/chrome/installer/util/... chrome/installer/util/install_util.cc:660: (*current_version < new_version && should this be <= so that the delete is done for same-version-repair as well? is there harm in this? https://codereview.chromium.org/1986823002/diff/120001/chrome/installer/util/... File chrome/installer/util/install_util_unittest.cc (right): https://codereview.chromium.org/1986823002/diff/120001/chrome/installer/util/... chrome/installer/util/install_util_unittest.cc:550: list.reset((WorkItem::CreateWorkItemList())); nit: remove spurious parens https://codereview.chromium.org/1986823002/diff/120001/chrome/installer/util/... chrome/installer/util/install_util_unittest.cc:565: // Multiple downgrads should not change the value. downgrades https://codereview.chromium.org/1986823002/diff/120001/chrome/installer/util/... chrome/installer/util/install_util_unittest.cc:622: // Fresh install should delete the value if exist nit: "it exists."
https://codereview.chromium.org/1986823002/diff/120001/chrome/installer/util/... File chrome/installer/util/install_util.cc (right): https://codereview.chromium.org/1986823002/diff/120001/chrome/installer/util/... chrome/installer/util/install_util.cc:656: DCHECK(dist); On 2016/05/20 18:17:05, grt (slow) wrote: > to enforce the "only call this for Chrome", add: > DCHECK_EQ(BrowserDistribution::CHROME_BROWSER, dist->GetType()); Done. https://codereview.chromium.org/1986823002/diff/120001/chrome/installer/util/... chrome/installer/util/install_util.cc:659: if (current_version == nullptr || On 2016/05/20 18:17:05, grt (slow) wrote: > nit: if (!current_version Done. https://codereview.chromium.org/1986823002/diff/120001/chrome/installer/util/... chrome/installer/util/install_util.cc:660: (*current_version < new_version && On 2016/05/20 18:17:05, grt (slow) wrote: > should this be <= so that the delete is done for same-version-repair as well? is > there harm in this? For same-version-repair, the |downgrade_version| should be taken care of by previous install. However, because "strange things happen in the registry on a regular basis", I think it worth to add '=' so that we can fix the |downgrade_version| during repair if needed. https://codereview.chromium.org/1986823002/diff/120001/chrome/installer/util/... File chrome/installer/util/install_util_unittest.cc (right): https://codereview.chromium.org/1986823002/diff/120001/chrome/installer/util/... chrome/installer/util/install_util_unittest.cc:550: list.reset((WorkItem::CreateWorkItemList())); On 2016/05/20 18:17:05, grt (slow) wrote: > nit: remove spurious parens Done. https://codereview.chromium.org/1986823002/diff/120001/chrome/installer/util/... chrome/installer/util/install_util_unittest.cc:565: // Multiple downgrads should not change the value. On 2016/05/20 18:17:05, grt (slow) wrote: > downgrades Done. https://codereview.chromium.org/1986823002/diff/120001/chrome/installer/util/... chrome/installer/util/install_util_unittest.cc:622: // Fresh install should delete the value if exist On 2016/05/20 18:17:05, grt (slow) wrote: > nit: "it exists." Done.
nice. lgtm w/ a final nit https://codereview.chromium.org/1986823002/diff/140001/chrome/installer/util/... File chrome/installer/util/install_util_unittest.cc (right): https://codereview.chromium.org/1986823002/diff/140001/chrome/installer/util/... chrome/installer/util/install_util_unittest.cc:622: // Fresh install should delete the value if exists. nit: "if it exists."
zmin@chromium.org changed reviewers: + atwilson@chromium.org, sky@chromium.org
sky@chromium.org: Please review changes in chrome/browser/chrome_browser_main.cc chrome/browser/downgrade/* atwilson@chromium.org: Please review changes in chrome/browser/policy/* components/policy/resources/policy_templated.json Here is the doc related to the CL if you want to know more background https://docs.google.com/a/google.com/document/d/1awm7D4O1v0ad1t8N9dHxcvNkrfw-... Owen
https://codereview.chromium.org/1986823002/diff/160001/chrome/browser/downgra... File chrome/browser/downgrade/user_data_downgrade.h (right): https://codereview.chromium.org/1986823002/diff/160001/chrome/browser/downgra... chrome/browser/downgrade/user_data_downgrade.h:19: void DeleteMovedUserDataSoon(); Does the uninstaller have the option of removing profile data? If so, is it possible to use that code?
https://codereview.chromium.org/1986823002/diff/160001/chrome/browser/downgra... File chrome/browser/downgrade/user_data_downgrade.h (right): https://codereview.chromium.org/1986823002/diff/160001/chrome/browser/downgra... chrome/browser/downgrade/user_data_downgrade.h:19: void DeleteMovedUserDataSoon(); On 2016/05/24 16:54:44, sky wrote: > Does the uninstaller have the option of removing profile data? If so, is it > possible to use that code? Yes, the uninstaller has that option. However, the code can not be reused easily because of some reasons: 1) This function will not delete the profile data directly as uninstaller but all directories renamed by previous function. It's also responsible for searching all renamed directories. 2) The uninstaller removing does not support group policy or command line flag now. 3) This function doesn't need MOVEFILE_DELAY_UNTIL_REBOOT as uninstaller. If the profiles can not be deleted, we can simple retry in the next launch.
On Tue, May 24, 2016 at 10:32 AM, <zmin@chromium.org> wrote: > > https://codereview.chromium.org/1986823002/diff/160001/chrome/browser/downgra... > File chrome/browser/downgrade/user_data_downgrade.h (right): > > https://codereview.chromium.org/1986823002/diff/160001/chrome/browser/downgra... > chrome/browser/downgrade/user_data_downgrade.h:19: void > DeleteMovedUserDataSoon(); > On 2016/05/24 16:54:44, sky wrote: >> Does the uninstaller have the option of removing profile data? If so, > is it >> possible to use that code? > > Yes, the uninstaller has that option. However, the code can not be > reused easily because of some reasons: > > 1) This function will not delete the profile data directly as > uninstaller but all directories renamed by previous function. It's also > responsible for searching all renamed directories. What is the difference between the two? Is it just the name to search from? > 2) The uninstaller removing does not support group policy or command > line flag now. > > 3) This function doesn't need MOVEFILE_DELAY_UNTIL_REBOOT as > uninstaller. If the profiles can not be deleted, we can simple retry in > the next launch. My hope is that the finding files part is the same, and that if necessary the actual deletion can be separate. -Scott -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2016/05/24 20:28:42, sky wrote: > On Tue, May 24, 2016 at 10:32 AM, <mailto:zmin@chromium.org> wrote: > > > > > https://codereview.chromium.org/1986823002/diff/160001/chrome/browser/downgra... > > File chrome/browser/downgrade/user_data_downgrade.h (right): > > > > > https://codereview.chromium.org/1986823002/diff/160001/chrome/browser/downgra... > > chrome/browser/downgrade/user_data_downgrade.h:19: void > > DeleteMovedUserDataSoon(); > > On 2016/05/24 16:54:44, sky wrote: > >> Does the uninstaller have the option of removing profile data? If so, > > is it > >> possible to use that code? > > > > Yes, the uninstaller has that option. However, the code can not be > > reused easily because of some reasons: > > > > 1) This function will not delete the profile data directly as > > uninstaller but all directories renamed by previous function. It's also > > responsible for searching all renamed directories. > > What is the difference between the two? Is it just the name to search from? > > > 2) The uninstaller removing does not support group policy or command > > line flag now. > > > > 3) This function doesn't need MOVEFILE_DELAY_UNTIL_REBOOT as > > uninstaller. If the profiles can not be deleted, we can simple retry in > > the next launch. > > My hope is that the finding files part is the same, and that if > necessary the actual deletion can be separate. > > -Scott > > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. > Here is the step of two removing: downgrade removing: Find all dir (for both |user_data_dir| and |disk_cache_dir|) Delete the dir (and ignore any error) uninstall removing: Delete the dir (only for |user_data_dir|) Handle error if wanted(schedule reboot deletion) Delete parent and grandparent dir if necessary. The only logic that is shared by two removing is calling base::DeleteFile(). Even they can potentially share more things in the future, I feel it's not worth refactoring uninstall removing code right now just because one shared API call.
Meta question for this. Can we move the file to trash and let the system deal? https://codereview.chromium.org/1986823002/diff/160001/chrome/browser/downgra... File chrome/browser/downgrade/user_data_downgrade.cc (right): https://codereview.chromium.org/1986823002/diff/160001/chrome/browser/downgra... chrome/browser/downgrade/user_data_downgrade.cc:49: return user_data_dir.Append(FILE_PATH_LITERAL("LastChromeVersion")); nit: move to constant. Also, we generally leave spaces in file names, eg 'Last Chrome Version'. Any why is Chrome in the name? Why not "Last Version"? https://codereview.chromium.org/1986823002/diff/160001/chrome/browser/downgra... chrome/browser/downgrade/user_data_downgrade.cc:91: void DeleteAllRenamedUserDirectories(const base::FilePath& path) { How come you can't deletefile on path with recursive? https://codereview.chromium.org/1986823002/diff/160001/chrome/browser/downgra... chrome/browser/downgrade/user_data_downgrade.cc:144: base::WriteFile(GetLastChromeVersionFile(user_data_dir), Why did you decide to put the version in it's own file, rather than an existing file?
Or ask the system to delete on reboot? If we really need to manually delete, then please add tests.
On 2016/05/24 21:35:57, sky wrote: > Or ask the system to delete on reboot? > If we really need to manually delete, then please add tests. Because user data may be put on network drive which. may not have recycle bin. Yes, there will be test for the whole downgrade thing which is currently blocked by other CL. It'll be added once the blocker is resolved.
https://codereview.chromium.org/1986823002/diff/160001/chrome/browser/downgra... File chrome/browser/downgrade/user_data_downgrade.cc (right): https://codereview.chromium.org/1986823002/diff/160001/chrome/browser/downgra... chrome/browser/downgrade/user_data_downgrade.cc:49: return user_data_dir.Append(FILE_PATH_LITERAL("LastChromeVersion")); On 2016/05/24 21:35:30, sky wrote: > nit: move to constant. Also, we generally leave spaces in file names, eg 'Last > Chrome Version'. Any why is Chrome in the name? Why not "Last Version"? Move to const and add spaces. Use Chrome here because it's stored the version of chrome that is launched recently by this user. https://codereview.chromium.org/1986823002/diff/160001/chrome/browser/downgra... chrome/browser/downgrade/user_data_downgrade.cc:91: void DeleteAllRenamedUserDirectories(const base::FilePath& path) { On 2016/05/24 21:35:30, sky wrote: > How come you can't deletefile on path with recursive? Are you asking why I need to delete |disk_cache_dir| separately? That's because |disk_cache_dir| can be edited by group policy which may not be located under |user_data_dir|. https://codereview.chromium.org/1986823002/diff/160001/chrome/browser/downgra... chrome/browser/downgrade/user_data_downgrade.cc:144: base::WriteFile(GetLastChromeVersionFile(user_data_dir), On 2016/05/24 21:35:30, sky wrote: > Why did you decide to put the version in it's own file, rather than an existing > file? Because this file needs to be read before everything else use user data directory which means it's not safe to reuse any existing file.
Please add tests for the move and delete logic in this patch. https://codereview.chromium.org/1986823002/diff/160001/chrome/browser/downgra... File chrome/browser/downgrade/user_data_downgrade.cc (right): https://codereview.chromium.org/1986823002/diff/160001/chrome/browser/downgra... chrome/browser/downgrade/user_data_downgrade.cc:91: void DeleteAllRenamedUserDirectories(const base::FilePath& path) { On 2016/05/24 22:39:15, zmin wrote: > On 2016/05/24 21:35:30, sky wrote: > > How come you can't deletefile on path with recursive? > > Are you asking why I need to delete |disk_cache_dir| separately? That's because > |disk_cache_dir| can be edited by group policy which may not be located under > |user_data_dir|. Good point. My concern is more that you are doing a wild card search and may unintentionally pick up more than you expect. But I guess that makes sense in this case. https://codereview.chromium.org/1986823002/diff/160001/chrome/browser/downgra... chrome/browser/downgrade/user_data_downgrade.cc:129: // Without the browser process singleton protection, the directory may be Is there a reason we can't do this after singleton protection? https://codereview.chromium.org/1986823002/diff/160001/chrome/browser/downgra... chrome/browser/downgrade/user_data_downgrade.cc:136: base::FilePath temp_disk_cache_dir(GetTempDirNameForDelete(disk_cache_dir)); You don't check for empty returns here. https://codereview.chromium.org/1986823002/diff/160001/chrome/browser/downgra... chrome/browser/downgrade/user_data_downgrade.cc:144: base::WriteFile(GetLastChromeVersionFile(user_data_dir), On 2016/05/24 22:39:15, zmin wrote: > On 2016/05/24 21:35:30, sky wrote: > > Why did you decide to put the version in it's own file, rather than an > existing > > file? > > Because this file needs to be read before everything else use user data > directory which means it's not safe to reuse any existing file. That's not entirely true, you could still read from prefs, and then close and move right? But what you have may be easier.
On 2016/05/25 15:43:34, sky wrote: > Please add tests for the move and delete logic in this patch. Ok, I'll add test for move and delete into this patch. > https://codereview.chromium.org/1986823002/diff/160001/chrome/browser/downgra... > File chrome/browser/downgrade/user_data_downgrade.cc (right): > > https://codereview.chromium.org/1986823002/diff/160001/chrome/browser/downgra... > chrome/browser/downgrade/user_data_downgrade.cc:91: void > DeleteAllRenamedUserDirectories(const base::FilePath& path) { > On 2016/05/24 22:39:15, zmin wrote: > > On 2016/05/24 21:35:30, sky wrote: > > > How come you can't deletefile on path with recursive? > > > > Are you asking why I need to delete |disk_cache_dir| separately? That's > because > > |disk_cache_dir| can be edited by group policy which may not be located under > > |user_data_dir|. > > Good point. > My concern is more that you are doing a wild card search and may unintentionally > pick up more than you expect. But I guess that makes sense in this case. > > https://codereview.chromium.org/1986823002/diff/160001/chrome/browser/downgra... > chrome/browser/downgrade/user_data_downgrade.cc:129: // Without the browser > process singleton protection, the directory may be > Is there a reason we can't do this after singleton protection? The removing happened right after the user data dir is initialized which is the place that guarantees no one else is able to touch the dir. Singleton protection is activated much later during the launch. > > https://codereview.chromium.org/1986823002/diff/160001/chrome/browser/downgra... > chrome/browser/downgrade/user_data_downgrade.cc:136: base::FilePath > temp_disk_cache_dir(GetTempDirNameForDelete(disk_cache_dir)); > You don't check for empty returns here. The empty path is checked later in RenameDirectory() > https://codereview.chromium.org/1986823002/diff/160001/chrome/browser/downgra... > chrome/browser/downgrade/user_data_downgrade.cc:144: > base::WriteFile(GetLastChromeVersionFile(user_data_dir), > On 2016/05/24 22:39:15, zmin wrote: > > On 2016/05/24 21:35:30, sky wrote: > > > Why did you decide to put the version in it's own file, rather than an > > existing > > > file? > > > > Because this file needs to be read before everything else use user data > > directory which means it's not safe to reuse any existing file. > > That's not entirely true, you could still read from prefs, and then close and > move right? But what you have may be easier. Yes, I could. However, that means I have to read the file by myself but not prefs API which may not be a good idea.
I agree with your reasons for wanting to do this early on. On Wed, May 25, 2016 at 10:18 AM, <zmin@chromium.org> wrote: > On 2016/05/25 15:43:34, sky wrote: >> Please add tests for the move and delete logic in this patch. > Ok, I'll add test for move and delete into this patch. > >> > https://codereview.chromium.org/1986823002/diff/160001/chrome/browser/downgra... >> File chrome/browser/downgrade/user_data_downgrade.cc (right): >> >> > https://codereview.chromium.org/1986823002/diff/160001/chrome/browser/downgra... >> chrome/browser/downgrade/user_data_downgrade.cc:91: void >> DeleteAllRenamedUserDirectories(const base::FilePath& path) { >> On 2016/05/24 22:39:15, zmin wrote: >> > On 2016/05/24 21:35:30, sky wrote: >> > > How come you can't deletefile on path with recursive? >> > >> > Are you asking why I need to delete |disk_cache_dir| separately? That's >> because >> > |disk_cache_dir| can be edited by group policy which may not be located > under >> > |user_data_dir|. >> >> Good point. >> My concern is more that you are doing a wild card search and may > unintentionally >> pick up more than you expect. But I guess that makes sense in this case. >> >> > https://codereview.chromium.org/1986823002/diff/160001/chrome/browser/downgra... >> chrome/browser/downgrade/user_data_downgrade.cc:129: // Without the >> browser >> process singleton protection, the directory may be >> Is there a reason we can't do this after singleton protection? > > The removing happened right after the user data dir is initialized which is > the place that guarantees no one else is able to touch the dir. Singleton > protection is activated much later during the launch. > >> >> > https://codereview.chromium.org/1986823002/diff/160001/chrome/browser/downgra... >> chrome/browser/downgrade/user_data_downgrade.cc:136: base::FilePath >> temp_disk_cache_dir(GetTempDirNameForDelete(disk_cache_dir)); >> You don't check for empty returns here. > > The empty path is checked later in RenameDirectory() > >> > https://codereview.chromium.org/1986823002/diff/160001/chrome/browser/downgra... >> chrome/browser/downgrade/user_data_downgrade.cc:144: >> base::WriteFile(GetLastChromeVersionFile(user_data_dir), >> On 2016/05/24 22:39:15, zmin wrote: >> > On 2016/05/24 21:35:30, sky wrote: >> > > Why did you decide to put the version in it's own file, rather than an >> > existing >> > > file? >> > >> > Because this file needs to be read before everything else use user data >> > directory which means it's not safe to reuse any existing file. >> >> That's not entirely true, you could still read from prefs, and then close >> and >> move right? But what you have may be easier. > > Yes, I could. However, that means I have to read the file by myself but not > prefs API which may not be a good idea. > > > > > https://codereview.chromium.org/1986823002/ -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Patchset #10 (id:200001) has been deleted
https://codereview.chromium.org/1986823002/diff/220001/chrome/browser/downgra... File chrome/browser/downgrade/user_data_downgrade.cc (right): https://codereview.chromium.org/1986823002/diff/220001/chrome/browser/downgra... chrome/browser/downgrade/user_data_downgrade.cc:154: if (g_is_browser_test) { is it possible to get the browser test into "after startup" processing through other means so that this isn't needed? https://codereview.chromium.org/1986823002/diff/220001/chrome/browser/downgra... File chrome/browser/downgrade/user_data_downgrade.h (right): https://codereview.chromium.org/1986823002/diff/220001/chrome/browser/downgra... chrome/browser/downgrade/user_data_downgrade.h:21: // Enable browser test mode to make above functions test friendly. please explain what this does https://codereview.chromium.org/1986823002/diff/220001/chrome/browser/downgra... chrome/browser/downgrade/user_data_downgrade.h:22: void EnableBrowserTest(); call this something like SimulateDowngradeForTests to take advantage of the "don't call test functions" presubmit check: https://code.google.com/p/chromium/codesearch#chromium/src/PRESUBMIT.py&q=For... https://codereview.chromium.org/1986823002/diff/220001/chrome/browser/downgra... File chrome/browser/downgrade/user_data_downgrade_browsertest.cc (right): https://codereview.chromium.org/1986823002/diff/220001/chrome/browser/downgra... chrome/browser/downgrade/user_data_downgrade_browsertest.cc:20: const base::FilePath::StringType delete_suffix = nit: constexpr https://codereview.chromium.org/1986823002/diff/220001/chrome/browser/downgra... chrome/browser/downgrade/user_data_downgrade_browsertest.cc:48: base::ReadFileToString(last_chrome_version_path_, &last_chrome_version_str); please add comments to explain what this is testing
https://codereview.chromium.org/1986823002/diff/220001/chrome/browser/downgra... File chrome/browser/downgrade/user_data_downgrade_browsertest.cc (right): https://codereview.chromium.org/1986823002/diff/220001/chrome/browser/downgra... chrome/browser/downgrade/user_data_downgrade_browsertest.cc:20: const base::FilePath::StringType delete_suffix = On 2016/05/27 16:26:28, grt (slow) wrote: > nit: constexpr oops, nevermind. i read it as ::CharType rather than ::StringType
https://codereview.chromium.org/1986823002/diff/220001/chrome/browser/downgra... File chrome/browser/downgrade/user_data_downgrade.cc (right): https://codereview.chromium.org/1986823002/diff/220001/chrome/browser/downgra... chrome/browser/downgrade/user_data_downgrade.cc:154: if (g_is_browser_test) { On 2016/05/27 16:26:28, grt (slow) wrote: > is it possible to get the browser test into "after startup" processing through > other means so that this isn't needed? The after_startup_task will be executed sometimes. However, I have tried couple ways but none of them is able to make sure it always get executed. https://codereview.chromium.org/1986823002/diff/220001/chrome/browser/downgra... File chrome/browser/downgrade/user_data_downgrade.h (right): https://codereview.chromium.org/1986823002/diff/220001/chrome/browser/downgra... chrome/browser/downgrade/user_data_downgrade.h:21: // Enable browser test mode to make above functions test friendly. On 2016/05/27 16:26:28, grt (slow) wrote: > please explain what this does Done. https://codereview.chromium.org/1986823002/diff/220001/chrome/browser/downgra... chrome/browser/downgrade/user_data_downgrade.h:22: void EnableBrowserTest(); On 2016/05/27 16:26:28, grt (slow) wrote: > call this something like SimulateDowngradeForTests to take advantage of the > "don't call test functions" presubmit check: > https://code.google.com/p/chromium/codesearch#chromium/src/PRESUBMIT.py&q=For... Done. https://codereview.chromium.org/1986823002/diff/220001/chrome/browser/downgra... File chrome/browser/downgrade/user_data_downgrade_browsertest.cc (right): https://codereview.chromium.org/1986823002/diff/220001/chrome/browser/downgra... chrome/browser/downgrade/user_data_downgrade_browsertest.cc:48: base::ReadFileToString(last_chrome_version_path_, &last_chrome_version_str); On 2016/05/27 16:26:28, grt (slow) wrote: > please add comments to explain what this is testing Done.
https://codereview.chromium.org/1986823002/diff/220001/chrome/browser/downgra... File chrome/browser/downgrade/user_data_downgrade.cc (right): https://codereview.chromium.org/1986823002/diff/220001/chrome/browser/downgra... chrome/browser/downgrade/user_data_downgrade.cc:154: if (g_is_browser_test) { On 2016/05/27 17:43:00, zmin wrote: > On 2016/05/27 16:26:28, grt (slow) wrote: > > is it possible to get the browser test into "after startup" processing through > > other means so that this isn't needed? > > The after_startup_task will be executed sometimes. However, I have tried couple > ways but none of them is able to make sure it always get executed. What if the test posts its own AfterStartupTask and waits for it to be run before checking postconditions and exiting? https://codereview.chromium.org/1986823002/diff/240001/chrome/browser/downgra... File chrome/browser/downgrade/user_data_downgrade.h (right): https://codereview.chromium.org/1986823002/diff/240001/chrome/browser/downgra... chrome/browser/downgrade/user_data_downgrade.h:21: // Enable browser test mode. MoveUserDataForFirstRunAfterDowngrade will no maybe change the first sentence to: "Changes the behavior of the above functions to simulate a downgrade for use in tests."
On 2016/05/27 18:03:00, grt (slow) wrote: > https://codereview.chromium.org/1986823002/diff/220001/chrome/browser/downgra... > File chrome/browser/downgrade/user_data_downgrade.cc (right): > > https://codereview.chromium.org/1986823002/diff/220001/chrome/browser/downgra... > chrome/browser/downgrade/user_data_downgrade.cc:154: if (g_is_browser_test) { > On 2016/05/27 17:43:00, zmin wrote: > > On 2016/05/27 16:26:28, grt (slow) wrote: > > > is it possible to get the browser test into "after startup" processing > through > > > other means so that this isn't needed? > > > > The after_startup_task will be executed sometimes. However, I have tried > couple > > ways but none of them is able to make sure it always get executed. > > What if the test posts its own AfterStartupTask and waits for it to be run > before checking postconditions and exiting? Even the browser_test's task is executed, it's still possible the removing task hasn't. Because "The order or execution of tasks posted here is unspecified even when posting to a SequencedTaskRunner". > https://codereview.chromium.org/1986823002/diff/240001/chrome/browser/downgra... > File chrome/browser/downgrade/user_data_downgrade.h (right): > > https://codereview.chromium.org/1986823002/diff/240001/chrome/browser/downgra... > chrome/browser/downgrade/user_data_downgrade.h:21: // Enable browser test mode. > MoveUserDataForFirstRunAfterDowngrade will no > maybe change the first sentence to: "Changes the behavior of the above functions > to simulate a downgrade for use in tests." Done.
https://codereview.chromium.org/1986823002/diff/260001/chrome/browser/downgra... File chrome/browser/downgrade/user_data_downgrade.cc (right): https://codereview.chromium.org/1986823002/diff/260001/chrome/browser/downgra... chrome/browser/downgrade/user_data_downgrade.cc:156: content::BrowserThread::PostBlockingPoolTask( there's nothing explicit in the test to guarantee that this tasks runs before the test shuts down. i think this isn't a good idea. could you survey other uses of PostAfterStartupTask to see if any others are covered by browser tests? i think it would be better to have something explicit in here. if there's really no way for the test to force all AfterStartup tasks to be posted, then one alternative is to allow the test to pass a base::Closure into this impl somehow that DeleteMovedUserData() will post to the UI thread upon completion. https://codereview.chromium.org/1986823002/diff/260001/chrome/browser/downgra... chrome/browser/downgrade/user_data_downgrade.cc:160: FROM_HERE, content::BrowserThread::GetBlockingPool(), as-is, this task will block shutdown. i think it's better if the user data dir and cache dir are fetched in this function and bound into the callback so that it's safe for DeleteMovedUserData to be posted as a CONTINUE_ON_SHUTDOWN task.
On 2016/05/29 01:22:06, grt (slow) wrote: > https://codereview.chromium.org/1986823002/diff/260001/chrome/browser/downgra... > File chrome/browser/downgrade/user_data_downgrade.cc (right): > > https://codereview.chromium.org/1986823002/diff/260001/chrome/browser/downgra... > chrome/browser/downgrade/user_data_downgrade.cc:156: > content::BrowserThread::PostBlockingPoolTask( > there's nothing explicit in the test to guarantee that this tasks runs before > the test shuts down. i think this isn't a good idea. could you survey other uses > of PostAfterStartupTask to see if any others are covered by browser tests? i > think it would be better to have something explicit in here. if there's really > no way for the test to force all AfterStartup tasks to be posted, then one > alternative is to allow the test to pass a base::Closure into this impl somehow > that DeleteMovedUserData() will post to the UI thread upon completion. Talk with grt@ offline. There is no good way to force this task be executed in browser test because: 1) We can't use RunLoop().RunUntiIidle() because blocking pool has multiple threads. 2) We can't use SequencedWorkerPool->FlushForTesting() because the task will be posted with delay by after_startup_task_utils.cc A good way may be adding a new ForTesting API in after_startup_task_utils.cc to change the delay sec to 0 so that we can use FlushForTesting. I'll talk with the creator of that file(michaeln@) for that solution. > https://codereview.chromium.org/1986823002/diff/260001/chrome/browser/downgra... > chrome/browser/downgrade/user_data_downgrade.cc:160: FROM_HERE, > content::BrowserThread::GetBlockingPool(), > as-is, this task will block shutdown. i think it's better if the user data dir > and cache dir are fetched in this function and bound into the callback so that > it's safe for DeleteMovedUserData to be posted as a CONTINUE_ON_SHUTDOWN task. Add shutdown behavior and fetch path of both directory before the task post.
FYI: michaeln@ is OOO until Jun. 13 On 2016/05/31 18:39:00, zmin wrote: > On 2016/05/29 01:22:06, grt (slow) wrote: > > > https://codereview.chromium.org/1986823002/diff/260001/chrome/browser/downgra... > > File chrome/browser/downgrade/user_data_downgrade.cc (right): > > > > > https://codereview.chromium.org/1986823002/diff/260001/chrome/browser/downgra... > > chrome/browser/downgrade/user_data_downgrade.cc:156: > > content::BrowserThread::PostBlockingPoolTask( > > there's nothing explicit in the test to guarantee that this tasks runs before > > the test shuts down. i think this isn't a good idea. could you survey other > uses > > of PostAfterStartupTask to see if any others are covered by browser tests? i > > think it would be better to have something explicit in here. if there's really > > no way for the test to force all AfterStartup tasks to be posted, then one > > alternative is to allow the test to pass a base::Closure into this impl > somehow > > that DeleteMovedUserData() will post to the UI thread upon completion. > > Talk with grt@ offline. There is no good way to force this task be executed in > browser test because: > 1) We can't use RunLoop().RunUntiIidle() because blocking pool has multiple > threads. > 2) We can't use SequencedWorkerPool->FlushForTesting() because the task will be > posted with delay by after_startup_task_utils.cc > > A good way may be adding a new ForTesting API in after_startup_task_utils.cc to > change the delay sec to 0 so that we can use FlushForTesting. > I'll talk with the creator of that file(michaeln@) for that solution. > > > > https://codereview.chromium.org/1986823002/diff/260001/chrome/browser/downgra... > > chrome/browser/downgrade/user_data_downgrade.cc:160: FROM_HERE, > > content::BrowserThread::GetBlockingPool(), > > as-is, this task will block shutdown. i think it's better if the user data dir > > and cache dir are fetched in this function and bound into the callback so that > > it's safe for DeleteMovedUserData to be posted as a CONTINUE_ON_SHUTDOWN task. > > Add shutdown behavior and fetch path of both directory before the task post.
lgtm https://codereview.chromium.org/1986823002/diff/280001/chrome/browser/downgra... File chrome/browser/downgrade/user_data_downgrade_browsertest.cc (right): https://codereview.chromium.org/1986823002/diff/280001/chrome/browser/downgra... chrome/browser/downgrade/user_data_downgrade_browsertest.cc:18: SimulateDowngradeForTest(); suggestion: since this sets a process-wide singleton, add a complement to this in TearDown... so that other tests running after this one in the same process don't run the silent downgrade path.
https://codereview.chromium.org/1986823002/diff/280001/chrome/browser/downgra... File chrome/browser/downgrade/user_data_downgrade_browsertest.cc (right): https://codereview.chromium.org/1986823002/diff/280001/chrome/browser/downgra... chrome/browser/downgrade/user_data_downgrade_browsertest.cc:18: SimulateDowngradeForTest(); On 2016/05/31 19:44:48, grt (slow) wrote: > suggestion: since this sets a process-wide singleton, add a complement to this > in TearDown... so that other tests running after this one in the same process > don't run the silent downgrade path. Done.
zmin@chromium.org changed reviewers: + pastarmovj@chromium.org - atwilson@chromium.org
- atwilson@ + pastarmovj@ For chrome/browser/policy/* components/policy/resources/policy_templates.json
One clarification question and a nit before I can approve the change to the policy_templates.json file. https://codereview.chromium.org/1986823002/diff/300001/components/policy/reso... File components/policy/resources/policy_templates.json (right): https://codereview.chromium.org/1986823002/diff/300001/components/policy/reso... components/policy/resources/policy_templates.json:1420: This policy must not be set to a volume's root directory, and should not be set to a directory used for other purposes.''', Can you explain to me why is this the case? I don't think this is really impossible only it is a bad choice I guess. Or do you mean this would cause your profile deletion logic to wipe the hard drive? 8-| Regardless of the discussion above I think this paragraph belongs above the paragraph "If this policy is not set...". https://codereview.chromium.org/1986823002/diff/300001/components/policy/reso... components/policy/resources/policy_templates.json:1444: This policy must not be set to a volume's root directory, and should not be set to a directory used for other purposes.''', Ditto.
https://codereview.chromium.org/1986823002/diff/300001/components/policy/reso... File components/policy/resources/policy_templates.json (right): https://codereview.chromium.org/1986823002/diff/300001/components/policy/reso... components/policy/resources/policy_templates.json:1420: This policy must not be set to a volume's root directory, and should not be set to a directory used for other purposes.''', On 2016/06/01 15:09:40, pastarmovj wrote: > Can you explain to me why is this the case? I don't think this is really > impossible only it is a bad choice I guess. Or do you mean this would cause your > profile deletion logic to wipe the hard drive? 8-| > > Regardless of the discussion above I think this paragraph belongs above the > paragraph "If this policy is not set...". No, it won't wipe the hard drive. However, there is no easy way for us to delete a directory if it's set to root directory. Because we can't move root directory before delete like others and delete without move causes many other issues (too slow, partially delete). As a result, if the |user_data_dir| or |disk_cache_dir| is set to root directory, the removing code will simply be skipped. Because root directory can potentially cause issues after the downgrade and it's hard for us to explain why as we don't want to document downgrade feature publicly. I think it's just good to tell the users to avoid using it even though Admins can still set it at their own risk. I'll combine this paragraph with the above one. https://codereview.chromium.org/1986823002/diff/300001/components/policy/reso... components/policy/resources/policy_templates.json:1444: This policy must not be set to a volume's root directory, and should not be set to a directory used for other purposes.''', On 2016/06/01 15:09:40, pastarmovj wrote: > Ditto. Done.
https://codereview.chromium.org/1986823002/diff/300001/components/policy/reso... File components/policy/resources/policy_templates.json (right): https://codereview.chromium.org/1986823002/diff/300001/components/policy/reso... components/policy/resources/policy_templates.json:1420: This policy must not be set to a volume's root directory, and should not be set to a directory used for other purposes.''', On 2016/06/01 15:34:30, zmin wrote: > On 2016/06/01 15:09:40, pastarmovj wrote: > > Can you explain to me why is this the case? I don't think this is really > > impossible only it is a bad choice I guess. Or do you mean this would cause > your > > profile deletion logic to wipe the hard drive? 8-| > > > > Regardless of the discussion above I think this paragraph belongs above the > > paragraph "If this policy is not set...". > > No, it won't wipe the hard drive. However, there is no easy way for us to delete > a directory if it's set to root directory. Because we can't move root directory > before delete like others and delete without move causes many other issues (too > slow, partially delete). > As a result, if the |user_data_dir| or |disk_cache_dir| is set to root > directory, the removing code will simply be skipped. > > Because root directory can potentially cause issues after the downgrade and it's > hard for us to explain why as we don't want to document downgrade feature > publicly. I think it's just good to tell the users to avoid using it even though > Admins can still set it at their own risk. > > I'll combine this paragraph with the above one. Actually, I'll put it behind the paragraph of 'If you set this policy, .... " as they are both talking about "set".
https://codereview.chromium.org/1986823002/diff/300001/components/policy/reso... File components/policy/resources/policy_templates.json (right): https://codereview.chromium.org/1986823002/diff/300001/components/policy/reso... components/policy/resources/policy_templates.json:1420: This policy must not be set to a volume's root directory, and should not be set to a directory used for other purposes.''', On 2016/06/01 15:45:22, zmin wrote: > On 2016/06/01 15:34:30, zmin wrote: > > On 2016/06/01 15:09:40, pastarmovj wrote: > > > Can you explain to me why is this the case? I don't think this is really > > > impossible only it is a bad choice I guess. Or do you mean this would cause > > your > > > profile deletion logic to wipe the hard drive? 8-| > > > > > > Regardless of the discussion above I think this paragraph belongs above the > > > paragraph "If this policy is not set...". > > > > No, it won't wipe the hard drive. However, there is no easy way for us to > delete > > a directory if it's set to root directory. Because we can't move root > directory > > before delete like others and delete without move causes many other issues > (too > > slow, partially delete). > > As a result, if the |user_data_dir| or |disk_cache_dir| is set to root > > directory, the removing code will simply be skipped. > > > > Because root directory can potentially cause issues after the downgrade and > it's > > hard for us to explain why as we don't want to document downgrade feature > > publicly. I think it's just good to tell the users to avoid using it even > though > > Admins can still set it at their own risk. > > > > I'll combine this paragraph with the above one. > > Actually, I'll put it behind the paragraph of 'If you set this policy, .... " as > they are both talking about "set". I understand that we don't want to have a lengthy and potentially hard to follow conversation here why is this the case. But at least let's have something like "To avoid data loss this policy should not be set to a volume's root directory or to a directory used for other purposes, because <ph name="PRODUCT_NAME">$1<ex>Google Chrome</ex></ph> manages its contents." or some such. It still doesn't go into detail but kind of gives the idea of why is this not a great idea. WDYT?
On 2016/06/02 10:04:35, pastarmovj wrote: > https://codereview.chromium.org/1986823002/diff/300001/components/policy/reso... > File components/policy/resources/policy_templates.json (right): > > https://codereview.chromium.org/1986823002/diff/300001/components/policy/reso... > components/policy/resources/policy_templates.json:1420: This policy must not be > set to a volume's root directory, and should not be set to a directory used for > other purposes.''', > On 2016/06/01 15:45:22, zmin wrote: > > On 2016/06/01 15:34:30, zmin wrote: > > > On 2016/06/01 15:09:40, pastarmovj wrote: > > > > Can you explain to me why is this the case? I don't think this is really > > > > impossible only it is a bad choice I guess. Or do you mean this would > cause > > > your > > > > profile deletion logic to wipe the hard drive? 8-| > > > > > > > > Regardless of the discussion above I think this paragraph belongs above > the > > > > paragraph "If this policy is not set...". > > > > > > No, it won't wipe the hard drive. However, there is no easy way for us to > > delete > > > a directory if it's set to root directory. Because we can't move root > > directory > > > before delete like others and delete without move causes many other issues > > (too > > > slow, partially delete). > > > As a result, if the |user_data_dir| or |disk_cache_dir| is set to root > > > directory, the removing code will simply be skipped. > > > > > > Because root directory can potentially cause issues after the downgrade and > > it's > > > hard for us to explain why as we don't want to document downgrade feature > > > publicly. I think it's just good to tell the users to avoid using it even > > though > > > Admins can still set it at their own risk. > > > > > > I'll combine this paragraph with the above one. > > > > Actually, I'll put it behind the paragraph of 'If you set this policy, .... " > as > > they are both talking about "set". > > I understand that we don't want to have a lengthy and potentially hard to follow > conversation here why is this the case. But at least let's have something like > "To avoid data loss this policy should not be set to a volume's root directory > or to a directory used for other purposes, because <ph > name="PRODUCT_NAME">$1<ex>Google Chrome</ex></ph> manages its contents." or some > such. It still doesn't go into detail but kind of gives the idea of why is this > not a great idea. WDYT? Sure, we can do that. However root directory won't cause data loss but potential issues for downgrade. So I'll say "To avoid data loss or other unexpected errors"
lgtm
https://codereview.chromium.org/1986823002/diff/400001/chrome/browser/downgra... File chrome/browser/downgrade/user_data_downgrade.h (right): https://codereview.chromium.org/1986823002/diff/400001/chrome/browser/downgra... chrome/browser/downgrade/user_data_downgrade.h:25: void SimulateDowngradeForTest(bool is_browser_test); Why do you need this? Can't you make the test harness setup the right environment for you?
https://codereview.chromium.org/1986823002/diff/400001/chrome/browser/downgra... File chrome/browser/downgrade/user_data_downgrade.h (right): https://codereview.chromium.org/1986823002/diff/400001/chrome/browser/downgra... chrome/browser/downgrade/user_data_downgrade.h:25: void SimulateDowngradeForTest(bool is_browser_test); On 2016/06/02 18:00:32, sky wrote: > Why do you need this? Can't you make the test harness setup the right > environment for you? I use this for two things: 1) Avoid registry access. Unfortunately, RegistryOverrideManager is not working for browser tests and editing the registry directly during the test is also not a good idea for me. 2) Make sure the deletion task always got executed in the test. I can't find a way to do it as it posted by PostTaskAfterStartup with BlockingPool. I have an idea to make that API more test friendly but I need to talk with its creator(michaeln@) when he back to the office from vacation.
On Thu, Jun 2, 2016 at 11:21 AM, <zmin@chromium.org> wrote: > > https://codereview.chromium.org/1986823002/diff/400001/chrome/browser/downgra... > File chrome/browser/downgrade/user_data_downgrade.h (right): > > https://codereview.chromium.org/1986823002/diff/400001/chrome/browser/downgra... > chrome/browser/downgrade/user_data_downgrade.h:25: void > SimulateDowngradeForTest(bool is_browser_test); > On 2016/06/02 18:00:32, sky wrote: >> Why do you need this? Can't you make the test harness setup the right >> environment for you? > > I use this for two things: > 1) Avoid registry access. Unfortunately, RegistryOverrideManager is not > working for browser tests and editing the registry directly during the > test is also not a good idea for me. If you place this in interactive_ui_tests does that mitigate the concern? Tests in that harness run serially. > > 2) Make sure the deletion task always got executed in the test. I can't > find a way to do it as it posted by PostTaskAfterStartup with > BlockingPool. I have an idea to make that API more test friendly but I > need to talk with its creator(michaeln@) when he back to the office from > vacation. Again, that's kind of counter to the test. This is important enough and subtle enough that you really need a test that both verifies we actually do what we expect, and that verifies we don't attempt to delete if a policy is set and the version remains the same. > > https://codereview.chromium.org/1986823002/ -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2016/06/02 21:30:04, sky wrote: > On Thu, Jun 2, 2016 at 11:21 AM, <mailto:zmin@chromium.org> wrote: > > > > > https://codereview.chromium.org/1986823002/diff/400001/chrome/browser/downgra... > > File chrome/browser/downgrade/user_data_downgrade.h (right): > > > > > https://codereview.chromium.org/1986823002/diff/400001/chrome/browser/downgra... > > chrome/browser/downgrade/user_data_downgrade.h:25: void > > SimulateDowngradeForTest(bool is_browser_test); > > On 2016/06/02 18:00:32, sky wrote: > >> Why do you need this? Can't you make the test harness setup the right > >> environment for you? > > > > I use this for two things: > > 1) Avoid registry access. Unfortunately, RegistryOverrideManager is not > > working for browser tests and editing the registry directly during the > > test is also not a good idea for me. > > If you place this in interactive_ui_tests does that mitigate the > concern? Tests in that harness run serially. Thanks Scott, interactive_ui_tests works good. so I'll move my test there. For the second reason, I think we can get rid of it after I talk with michaeln@. The challenge I have now is the task posted by PostTaskAfterStartup() has a random delay between 0~10 seconds. However, the SequencedWorkerPool->FlushForTesting() does not support delay task. > > > > 2) Make sure the deletion task always got executed in the test. I can't > > find a way to do it as it posted by PostTaskAfterStartup with > > BlockingPool. I have an idea to make that API more test friendly but I > > need to talk with its creator(michaeln@) when he back to the office from > > vacation. > > Again, that's kind of counter to the test. This is important enough > and subtle enough that you really need a test that both verifies we > actually do what we expect, and that verifies we don't attempt to > delete if a policy is set and the version remains the same. > > > https://codereview.chromium.org/1986823002/ > > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. >
code updated
https://codereview.chromium.org/1986823002/diff/420001/chrome/browser/downgra... File chrome/browser/downgrade/user_data_downgrade.cc (right): https://codereview.chromium.org/1986823002/diff/420001/chrome/browser/downgra... chrome/browser/downgrade/user_data_downgrade.cc:51: FILE_PATH_LITERAL("Last Chrome Version"); nit: Chrome Version? Generally we don't use 'Last' for things that are current. For example, we have Bookmarks, not 'Last Bookmarks'. https://codereview.chromium.org/1986823002/diff/420001/chrome/browser/downgra... chrome/browser/downgrade/user_data_downgrade.cc:88: if (!source.empty() && !target.empty() && base::Move(source, target) && What if Move fails? Doesn't that leave things in a weird state? https://codereview.chromium.org/1986823002/diff/420001/chrome/browser/downgra... chrome/browser/downgrade/user_data_downgrade.cc:122: PathService::Get(chrome::DIR_USER_DATA, &user_data_dir); Check return value. https://codereview.chromium.org/1986823002/diff/420001/chrome/browser/downgra... chrome/browser/downgrade/user_data_downgrade.cc:142: if (last_chrome_version.IsValid() && Move this earlier on. You don't need to figure out temp dirs name is the last_chrome_version is bogus. https://codereview.chromium.org/1986823002/diff/420001/chrome/browser/downgra... chrome/browser/downgrade/user_data_downgrade.cc:149: base::WriteFile(GetLastChromeVersionFile(user_data_dir), I find it confusing that a function named MoveUserDataForFirstRunAfterDowngrade() is also responsible for updating the version string. I think this code would be clearer if there is a specific function for updating the version string. https://codereview.chromium.org/1986823002/diff/420001/chrome/browser/downgra... File chrome/browser/downgrade/user_data_downgrade.h (right): https://codereview.chromium.org/1986823002/diff/420001/chrome/browser/downgra... chrome/browser/downgrade/user_data_downgrade.h:25: void SimulateDowngradeForTest(bool is_browser_test); Again, can you not call this function? Can you instead configure the profile and then rerun the test and verify everything? I PRE_ should do what you want. See: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/test/base/i... https://codereview.chromium.org/1986823002/diff/420001/chrome/browser/downgra... File chrome/browser/downgrade/user_data_downgrade_browsertest.cc (right): https://codereview.chromium.org/1986823002/diff/420001/chrome/browser/downgra... chrome/browser/downgrade/user_data_downgrade_browsertest.cc:35: FILE_PATH_LITERAL(" (1).CHROME_DELETE"); Make a constant for this. In fact you should also verify profile is actually moved to this directory. https://codereview.chromium.org/1986823002/diff/420001/chrome/browser/downgra... chrome/browser/downgrade/user_data_downgrade_browsertest.cc:67: IN_PROC_BROWSER_TEST_F(UserDataDowngradeBrowserTest, CopyAndClean) { Can you also add a test that the directory is not removed if the registry key is setup? https://codereview.chromium.org/1986823002/diff/420001/chrome/browser/downgra... chrome/browser/downgrade/user_data_downgrade_browsertest.cc:69: base::ReadFileToString(last_chrome_version_path_, &last_chrome_version_str); It would be good if user_data_downgrade.h had a specific function for getting the last version rather than having the test have to read the file.
https://codereview.chromium.org/1986823002/diff/420001/chrome/browser/downgra... File chrome/browser/downgrade/user_data_downgrade.cc (right): https://codereview.chromium.org/1986823002/diff/420001/chrome/browser/downgra... chrome/browser/downgrade/user_data_downgrade.cc:51: FILE_PATH_LITERAL("Last Chrome Version"); On 2016/06/03 00:04:24, sky wrote: > nit: Chrome Version? Generally we don't use 'Last' for things that are current. > For example, we have Bookmarks, not 'Last Bookmarks'. I use 'Last' here because this file is used to indicate the version of last launched Chrome so that we're able to know whether the profiles needs to be deleted or not. What's more, I think Chrome use 'last' in 'Local State' file for chrome version too. https://codereview.chromium.org/1986823002/diff/420001/chrome/browser/downgra... chrome/browser/downgrade/user_data_downgrade.cc:88: if (!source.empty() && !target.empty() && base::Move(source, target) && On 2016/06/03 00:04:24, sky wrote: > What if Move fails? Doesn't that leave things in a weird state? If move failed, we just end up with a higher version profile with lower version Chrome and a pop up error window if the format is different. https://codereview.chromium.org/1986823002/diff/420001/chrome/browser/downgra... chrome/browser/downgrade/user_data_downgrade.cc:122: PathService::Get(chrome::DIR_USER_DATA, &user_data_dir); On 2016/06/03 00:04:23, sky wrote: > Check return value. Done. https://codereview.chromium.org/1986823002/diff/420001/chrome/browser/downgra... chrome/browser/downgrade/user_data_downgrade.cc:142: if (last_chrome_version.IsValid() && On 2016/06/03 00:04:24, sky wrote: > Move this earlier on. You don't need to figure out temp dirs name is the > last_chrome_version is bogus. Finding temp dir name is put before this on purpose. It is used to prevent multiple moving without singleton protection. The comments above describes more details. https://codereview.chromium.org/1986823002/diff/420001/chrome/browser/downgra... chrome/browser/downgrade/user_data_downgrade.cc:149: base::WriteFile(GetLastChromeVersionFile(user_data_dir), On 2016/06/03 00:04:23, sky wrote: > I find it confusing that a function named > MoveUserDataForFirstRunAfterDowngrade() is also responsible for updating the > version string. I think this code would be clearer if there is a specific > function for updating the version string. Done. https://codereview.chromium.org/1986823002/diff/420001/chrome/browser/downgra... File chrome/browser/downgrade/user_data_downgrade.h (right): https://codereview.chromium.org/1986823002/diff/420001/chrome/browser/downgra... chrome/browser/downgrade/user_data_downgrade.h:25: void SimulateDowngradeForTest(bool is_browser_test); On 2016/06/03 00:04:24, sky wrote: > Again, can you not call this function? Can you instead configure the profile and > then rerun the test and verify everything? I PRE_ should do what you want. See: > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/test/base/i... > I have tried PRE_. The issue here is the browser test usually shutdown very quickly before the deletion task is executed which will be post with delay up to 10 seconds. This function will be removed once we solved the delay problem for testing in an separate CL. https://codereview.chromium.org/1986823002/diff/420001/chrome/browser/downgra... File chrome/browser/downgrade/user_data_downgrade_browsertest.cc (right): https://codereview.chromium.org/1986823002/diff/420001/chrome/browser/downgra... chrome/browser/downgrade/user_data_downgrade_browsertest.cc:35: FILE_PATH_LITERAL(" (1).CHROME_DELETE"); On 2016/06/03 00:04:24, sky wrote: > Make a constant for this. In fact you should also verify profile is actually > moved to this directory. The moved profile may be deleted very quickly during launch before we are able to check it. And it's constant already. https://codereview.chromium.org/1986823002/diff/420001/chrome/browser/downgra... chrome/browser/downgrade/user_data_downgrade_browsertest.cc:67: IN_PROC_BROWSER_TEST_F(UserDataDowngradeBrowserTest, CopyAndClean) { On 2016/06/03 00:04:24, sky wrote: > Can you also add a test that the directory is not removed if the registry key is > setup? Sure. https://codereview.chromium.org/1986823002/diff/420001/chrome/browser/downgra... chrome/browser/downgrade/user_data_downgrade_browsertest.cc:69: base::ReadFileToString(last_chrome_version_path_, &last_chrome_version_str); On 2016/06/03 00:04:24, sky wrote: > It would be good if user_data_downgrade.h had a specific function for getting > the last version rather than having the test have to read the file. Done.
https://codereview.chromium.org/1986823002/diff/420001/chrome/browser/downgra... File chrome/browser/downgrade/user_data_downgrade.cc (right): https://codereview.chromium.org/1986823002/diff/420001/chrome/browser/downgra... chrome/browser/downgrade/user_data_downgrade.cc:51: FILE_PATH_LITERAL("Last Chrome Version"); On 2016/06/03 15:46:52, zmin wrote: > On 2016/06/03 00:04:24, sky wrote: > > nit: Chrome Version? Generally we don't use 'Last' for things that are > current. > > For example, we have Bookmarks, not 'Last Bookmarks'. > > I use 'Last' here because this file is used to indicate the version of last > launched Chrome so that we're able to know whether the profiles needs to be > deleted or not. > > What's more, I think Chrome use 'last' in 'Local State' file for chrome version > too. I'm still of the opinion 'Last' is unnecessary here. I would also avoid 'Chrome' too as I assume this will be written for chromium builds too. https://codereview.chromium.org/1986823002/diff/420001/chrome/browser/downgra... chrome/browser/downgrade/user_data_downgrade.cc:88: if (!source.empty() && !target.empty() && base::Move(source, target) && On 2016/06/03 15:46:52, zmin wrote: > On 2016/06/03 00:04:24, sky wrote: > > What if Move fails? Doesn't that leave things in a weird state? > > If move failed, we just end up with a higher version profile with lower version > Chrome and a pop up error window if the format is different. > That's assuming every file deals with a version from the future, which I don't believe is the case. If it did, then we wouldn't need to nuke it, right? https://codereview.chromium.org/1986823002/diff/420001/chrome/browser/downgra... File chrome/browser/downgrade/user_data_downgrade_browsertest.cc (right): https://codereview.chromium.org/1986823002/diff/420001/chrome/browser/downgra... chrome/browser/downgrade/user_data_downgrade_browsertest.cc:35: FILE_PATH_LITERAL(" (1).CHROME_DELETE"); On 2016/06/03 15:46:52, zmin wrote: > On 2016/06/03 00:04:24, sky wrote: > > Make a constant for this. In fact you should also verify profile is actually > > moved to this directory. > > The moved profile may be deleted very quickly during launch before we are able > to check it. You need some way to verify all this is working. Add callbacks/observer from the test if you need to. > And it's constant already. By constant I mean the CHROME_DELETE portion. For example, if you change the name of the mv file this test will continue to succeed no matter what.
> https://codereview.chromium.org/1986823002/diff/420001/chrome/browser/downgra... > File chrome/browser/downgrade/user_data_downgrade.cc (right): > > https://codereview.chromium.org/1986823002/diff/420001/chrome/browser/downgra... > chrome/browser/downgrade/user_data_downgrade.cc:51: FILE_PATH_LITERAL("Last > Chrome Version"); > On 2016/06/03 15:46:52, zmin wrote: > > On 2016/06/03 00:04:24, sky wrote: > > > nit: Chrome Version? Generally we don't use 'Last' for things that are > > current. > > > For example, we have Bookmarks, not 'Last Bookmarks'. > > > > I use 'Last' here because this file is used to indicate the version of last > > launched Chrome so that we're able to know whether the profiles needs to be > > deleted or not. > > > > What's more, I think Chrome use 'last' in 'Local State' file for chrome > version > > too. > > I'm still of the opinion 'Last' is unnecessary here. I would also avoid 'Chrome' > too as I assume this will be written for chromium builds too. Change it to "Browser Version" except some internal variable name so they are different from 'current'. > https://codereview.chromium.org/1986823002/diff/420001/chrome/browser/downgra... > chrome/browser/downgrade/user_data_downgrade.cc:88: if (!source.empty() && > !target.empty() && base::Move(source, target) && > On 2016/06/03 15:46:52, zmin wrote: > > On 2016/06/03 00:04:24, sky wrote: > > > What if Move fails? Doesn't that leave things in a weird state? > > > > If move failed, we just end up with a higher version profile with lower > version > > Chrome and a pop up error window if the format is different. > > > > That's assuming every file deals with a version from the future, which I don't > believe is the case. If it did, then we wouldn't need to nuke it, right? Firstly, we shall still nuke it even with the pop-up because it's not a user-friendly pop-up and I want to make sure Chrome is working fine with as less user interactive as possible. There're things that can fail the move: 1) Multiple processes move dir at the same time. One should success, the others should fail by design. 2) Path is set to root directory. I have update the policy web page to tell user avoid using that and IT-admins have to manually delete the profiles in this case. 3) Other Errors. Admins have to manually delete the profiles. In the worst case, admins have to manually delete the profile to make Chrome work again. We have a help page to tell user do that anyway. And downgrade is something only should be done with support engineer. > https://codereview.chromium.org/1986823002/diff/420001/chrome/browser/downgra... > File chrome/browser/downgrade/user_data_downgrade_browsertest.cc (right): > > https://codereview.chromium.org/1986823002/diff/420001/chrome/browser/downgra... > chrome/browser/downgrade/user_data_downgrade_browsertest.cc:35: > FILE_PATH_LITERAL(" (1).CHROME_DELETE"); > On 2016/06/03 15:46:52, zmin wrote: > > On 2016/06/03 00:04:24, sky wrote: > > > Make a constant for this. In fact you should also verify profile is actually > > > moved to this directory. > > > > The moved profile may be deleted very quickly during launch before we are able > > to check it. > > You need some way to verify all this is working. Add callbacks/observer from the > test if you need to. Good news: I found a way to verify this. Bad news: It can only be done after solving the PostTaskAfterStartup issue. The InProcessBrowserTest::RunTestOnMainThreadLoop() will call AfterStartupTaskUtils::SetBrowserStartupIsCompleteForTesting() to trigger all after-startup-task. It means I can verify the moved dir just before the deletion task by overriding RunTestOnMainThreadLoop() function. However, I can't do it now because I can't really use PostTaskAfterStart() function in the test. > > And it's constant already. > > By constant I mean the CHROME_DELETE portion. For example, if you change the > name of the mv file this test will continue to succeed no matter what. Done. Also did to 'Browser Test'.
On Mon, Jun 6, 2016 at 3:29 PM, <zmin@chromium.org> wrote: >> > https://codereview.chromium.org/1986823002/diff/420001/chrome/browser/downgra... >> File chrome/browser/downgrade/user_data_downgrade.cc (right): >> >> > https://codereview.chromium.org/1986823002/diff/420001/chrome/browser/downgra... >> chrome/browser/downgrade/user_data_downgrade.cc:51: >> FILE_PATH_LITERAL("Last >> Chrome Version"); >> On 2016/06/03 15:46:52, zmin wrote: >> > On 2016/06/03 00:04:24, sky wrote: >> > > nit: Chrome Version? Generally we don't use 'Last' for things that are >> > current. >> > > For example, we have Bookmarks, not 'Last Bookmarks'. >> > >> > I use 'Last' here because this file is used to indicate the version of >> > last >> > launched Chrome so that we're able to know whether the profiles needs to >> > be >> > deleted or not. >> > >> > What's more, I think Chrome use 'last' in 'Local State' file for chrome >> version >> > too. >> >> I'm still of the opinion 'Last' is unnecessary here. I would also avoid > 'Chrome' >> too as I assume this will be written for chromium builds too. > > > Change it to "Browser Version" except some internal variable name so they > are > different from 'current'. That names is still confusing too. AFAIK there are no files with the name 'browser'. > https://codereview.chromium.org/1986823002/diff/420001/chrome/browser/downgra... >> chrome/browser/downgrade/user_data_downgrade.cc:88: if (!source.empty() && >> !target.empty() && base::Move(source, target) && >> On 2016/06/03 15:46:52, zmin wrote: >> > On 2016/06/03 00:04:24, sky wrote: >> > > What if Move fails? Doesn't that leave things in a weird state? >> > >> > If move failed, we just end up with a higher version profile with lower >> version >> > Chrome and a pop up error window if the format is different. >> > >> >> That's assuming every file deals with a version from the future, which I >> don't >> believe is the case. If it did, then we wouldn't need to nuke it, right? > Firstly, we shall still nuke it even with the pop-up because it's not a > user-friendly pop-up and I want to make sure Chrome is working fine with as > less > user interactive as possible. > > There're things that can fail the move: > 1) Multiple processes move dir at the same time. One should success, the > others > should fail by design. > 2) Path is set to root directory. I have update the policy web page to tell > user > avoid using that and IT-admins have to manually delete the profiles in this > case. > 3) Other Errors. Admins have to manually delete the profiles. > > In the worst case, admins have to manually delete the profile to make Chrome > work again. We have a help page to tell user do that anyway. And downgrade > is > something only should be done with support engineer. I'm still confused by this. It seems that if the move fails we shouldn't start, no? > https://codereview.chromium.org/1986823002/diff/420001/chrome/browser/downgra... >> File chrome/browser/downgrade/user_data_downgrade_browsertest.cc (right): >> >> > https://codereview.chromium.org/1986823002/diff/420001/chrome/browser/downgra... >> chrome/browser/downgrade/user_data_downgrade_browsertest.cc:35: >> FILE_PATH_LITERAL(" (1).CHROME_DELETE"); >> On 2016/06/03 15:46:52, zmin wrote: >> > On 2016/06/03 00:04:24, sky wrote: >> > > Make a constant for this. In fact you should also verify profile is > actually >> > > moved to this directory. >> > >> > The moved profile may be deleted very quickly during launch before we >> > are > able >> > to check it. >> >> You need some way to verify all this is working. Add callbacks/observer >> from > the >> test if you need to. > Good news: I found a way to verify this. > Bad news: It can only be done after solving the PostTaskAfterStartup issue. > > The InProcessBrowserTest::RunTestOnMainThreadLoop() will call > AfterStartupTaskUtils::SetBrowserStartupIsCompleteForTesting() to trigger > all > after-startup-task. > It means I can verify the moved dir just before the deletion task by > overriding > RunTestOnMainThreadLoop() function. > However, I can't do it now because I can't really use PostTaskAfterStart() > function in the test. > > >> > And it's constant already. >> >> By constant I mean the CHROME_DELETE portion. For example, if you change >> the >> name of the mv file this test will continue to succeed no matter what. > > Done. Also did to 'Browser Test'. > > https://codereview.chromium.org/1986823002/ -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2016/06/06 23:22:06, sky wrote: > On Mon, Jun 6, 2016 at 3:29 PM, <mailto:zmin@chromium.org> wrote: > >> > > > https://codereview.chromium.org/1986823002/diff/420001/chrome/browser/downgra... > >> File chrome/browser/downgrade/user_data_downgrade.cc (right): > >> > >> > > > https://codereview.chromium.org/1986823002/diff/420001/chrome/browser/downgra... > >> chrome/browser/downgrade/user_data_downgrade.cc:51: > >> FILE_PATH_LITERAL("Last > >> Chrome Version"); > >> On 2016/06/03 15:46:52, zmin wrote: > >> > On 2016/06/03 00:04:24, sky wrote: > >> > > nit: Chrome Version? Generally we don't use 'Last' for things that are > >> > current. > >> > > For example, we have Bookmarks, not 'Last Bookmarks'. > >> > > >> > I use 'Last' here because this file is used to indicate the version of > >> > last > >> > launched Chrome so that we're able to know whether the profiles needs to > >> > be > >> > deleted or not. > >> > > >> > What's more, I think Chrome use 'last' in 'Local State' file for chrome > >> version > >> > too. > >> > >> I'm still of the opinion 'Last' is unnecessary here. I would also avoid > > 'Chrome' > >> too as I assume this will be written for chromium builds too. > > > > > > Change it to "Browser Version" except some internal variable name so they > > are > > different from 'current'. > > That names is still confusing too. AFAIK there are no files with the > name 'browser'. How about "Client Version"? It means the version of user(Chrome) of that directory. Or "Profiles Version"/"Data Version"? It presents the version of dir itself. > > > https://codereview.chromium.org/1986823002/diff/420001/chrome/browser/downgra... > >> chrome/browser/downgrade/user_data_downgrade.cc:88: if (!source.empty() && > >> !target.empty() && base::Move(source, target) && > >> On 2016/06/03 15:46:52, zmin wrote: > >> > On 2016/06/03 00:04:24, sky wrote: > >> > > What if Move fails? Doesn't that leave things in a weird state? > >> > > >> > If move failed, we just end up with a higher version profile with lower > >> version > >> > Chrome and a pop up error window if the format is different. > >> > > >> > >> That's assuming every file deals with a version from the future, which I > >> don't > >> believe is the case. If it did, then we wouldn't need to nuke it, right? > > Firstly, we shall still nuke it even with the pop-up because it's not a > > user-friendly pop-up and I want to make sure Chrome is working fine with as > > less > > user interactive as possible. > > > > There're things that can fail the move: > > 1) Multiple processes move dir at the same time. One should success, the > > others > > should fail by design. > > 2) Path is set to root directory. I have update the policy web page to tell > > user > > avoid using that and IT-admins have to manually delete the profiles in this > > case. > > 3) Other Errors. Admins have to manually delete the profiles. > > > > In the worst case, admins have to manually delete the profile to make Chrome > > work again. We have a help page to tell user do that anyway. And downgrade > > is > > something only should be done with support engineer. > > I'm still confused by this. It seems that if the move fails we > shouldn't start, no? No. Firstly, some of the failures are on purpose. They're meant to prevent multiple processes from moving the user data dir for several times because there is no protection of singleton at this stage. Secondly, if there's an error (i.e. IO Error), the dir cannot be used anyway. If this happens, user data dir initialization in the previous step should handle this issue. After all, most of moving failures should not be a problem, and the possibility of the worst case is quite low (error occurs just from moving + profiles format changes + warning dialogue doesn't catch the error). Manual deletion should solve the worst case. And it can be provided by support engineers during the downgrade. I think it isn't worth blocking the launch because of that. > > > https://codereview.chromium.org/1986823002/diff/420001/chrome/browser/downgra... > >> File chrome/browser/downgrade/user_data_downgrade_browsertest.cc (right): > >> > >> > > > https://codereview.chromium.org/1986823002/diff/420001/chrome/browser/downgra... > >> chrome/browser/downgrade/user_data_downgrade_browsertest.cc:35: > >> FILE_PATH_LITERAL(" (1).CHROME_DELETE"); > >> On 2016/06/03 15:46:52, zmin wrote: > >> > On 2016/06/03 00:04:24, sky wrote: > >> > > Make a constant for this. In fact you should also verify profile is > > actually > >> > > moved to this directory. > >> > > >> > The moved profile may be deleted very quickly during launch before we > >> > are > > able > >> > to check it. > >> > >> You need some way to verify all this is working. Add callbacks/observer > >> from > > the > >> test if you need to. > > Good news: I found a way to verify this. > > Bad news: It can only be done after solving the PostTaskAfterStartup issue. > > > > The InProcessBrowserTest::RunTestOnMainThreadLoop() will call > > AfterStartupTaskUtils::SetBrowserStartupIsCompleteForTesting() to trigger > > all > > after-startup-task. > > It means I can verify the moved dir just before the deletion task by > > overriding > > RunTestOnMainThreadLoop() function. > > However, I can't do it now because I can't really use PostTaskAfterStart() > > function in the test. > > > > > >> > And it's constant already. > >> > >> By constant I mean the CHROME_DELETE portion. For example, if you change > >> the > >> name of the mv file this test will continue to succeed no matter what. > > > > Done. Also did to 'Browser Test'. > > > > https://codereview.chromium.org/1986823002/ > > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. >
On 2016/06/07 01:10:30, zmin wrote: > On 2016/06/06 23:22:06, sky wrote: > > On Mon, Jun 6, 2016 at 3:29 PM, <mailto:zmin@chromium.org> wrote: > > >> > > > > > > https://codereview.chromium.org/1986823002/diff/420001/chrome/browser/downgra... > > >> File chrome/browser/downgrade/user_data_downgrade.cc (right): > > >> > > >> > > > > > > https://codereview.chromium.org/1986823002/diff/420001/chrome/browser/downgra... > > >> chrome/browser/downgrade/user_data_downgrade.cc:51: > > >> FILE_PATH_LITERAL("Last > > >> Chrome Version"); > > >> On 2016/06/03 15:46:52, zmin wrote: > > >> > On 2016/06/03 00:04:24, sky wrote: > > >> > > nit: Chrome Version? Generally we don't use 'Last' for things that are > > >> > current. > > >> > > For example, we have Bookmarks, not 'Last Bookmarks'. > > >> > > > >> > I use 'Last' here because this file is used to indicate the version of > > >> > last > > >> > launched Chrome so that we're able to know whether the profiles needs to > > >> > be > > >> > deleted or not. > > >> > > > >> > What's more, I think Chrome use 'last' in 'Local State' file for chrome > > >> version > > >> > too. > > >> > > >> I'm still of the opinion 'Last' is unnecessary here. I would also avoid > > > 'Chrome' > > >> too as I assume this will be written for chromium builds too. > > > > > > > > > Change it to "Browser Version" except some internal variable name so they > > > are > > > different from 'current'. > > > > That names is still confusing too. AFAIK there are no files with the > > name 'browser'. > > How about "Client Version"? It means the version of user(Chrome) of that > directory. > > Or "Profiles Version"/"Data Version"? It presents the version of dir itself. > > > > > > > https://codereview.chromium.org/1986823002/diff/420001/chrome/browser/downgra... > > >> chrome/browser/downgrade/user_data_downgrade.cc:88: if (!source.empty() && > > >> !target.empty() && base::Move(source, target) && > > >> On 2016/06/03 15:46:52, zmin wrote: > > >> > On 2016/06/03 00:04:24, sky wrote: > > >> > > What if Move fails? Doesn't that leave things in a weird state? > > >> > > > >> > If move failed, we just end up with a higher version profile with lower > > >> version > > >> > Chrome and a pop up error window if the format is different. > > >> > > > >> > > >> That's assuming every file deals with a version from the future, which I > > >> don't > > >> believe is the case. If it did, then we wouldn't need to nuke it, right? > > > Firstly, we shall still nuke it even with the pop-up because it's not a > > > user-friendly pop-up and I want to make sure Chrome is working fine with as > > > less > > > user interactive as possible. > > > > > > There're things that can fail the move: > > > 1) Multiple processes move dir at the same time. One should success, the > > > others > > > should fail by design. > > > 2) Path is set to root directory. I have update the policy web page to tell > > > user > > > avoid using that and IT-admins have to manually delete the profiles in this > > > case. > > > 3) Other Errors. Admins have to manually delete the profiles. > > > > > > In the worst case, admins have to manually delete the profile to make Chrome > > > work again. We have a help page to tell user do that anyway. And downgrade > > > is > > > something only should be done with support engineer. > > > > I'm still confused by this. It seems that if the move fails we > > shouldn't start, no? > > No. (I mean "Yes, we should start Chrome if the move failed) > Firstly, some of the failures are on purpose. They're meant to prevent multiple > processes from moving the user data dir for several times because there is no > protection of singleton at this stage. > > Secondly, if there's an error (i.e. IO Error), the dir cannot be used anyway. If > this happens, user data dir initialization in the previous step should handle > this issue. > > After all, most of moving failures should not be a problem, and the possibility > of the worst case is quite low (error occurs just from moving + profiles format > changes + warning dialogue doesn't catch the error). Manual deletion should > solve the worst case. And it can be provided by support engineers during the > downgrade. I think it isn't worth blocking the launch because of that. > > > > > > > > > https://codereview.chromium.org/1986823002/diff/420001/chrome/browser/downgra... > > >> File chrome/browser/downgrade/user_data_downgrade_browsertest.cc (right): > > >> > > >> > > > > > > https://codereview.chromium.org/1986823002/diff/420001/chrome/browser/downgra... > > >> chrome/browser/downgrade/user_data_downgrade_browsertest.cc:35: > > >> FILE_PATH_LITERAL(" (1).CHROME_DELETE"); > > >> On 2016/06/03 15:46:52, zmin wrote: > > >> > On 2016/06/03 00:04:24, sky wrote: > > >> > > Make a constant for this. In fact you should also verify profile is > > > actually > > >> > > moved to this directory. > > >> > > > >> > The moved profile may be deleted very quickly during launch before we > > >> > are > > > able > > >> > to check it. > > >> > > >> You need some way to verify all this is working. Add callbacks/observer > > >> from > > > the > > >> test if you need to. > > > Good news: I found a way to verify this. > > > Bad news: It can only be done after solving the PostTaskAfterStartup issue. > > > > > > The InProcessBrowserTest::RunTestOnMainThreadLoop() will call > > > AfterStartupTaskUtils::SetBrowserStartupIsCompleteForTesting() to trigger > > > all > > > after-startup-task. > > > It means I can verify the moved dir just before the deletion task by > > > overriding > > > RunTestOnMainThreadLoop() function. > > > However, I can't do it now because I can't really use PostTaskAfterStart() > > > function in the test. > > > > > > > > >> > And it's constant already. > > >> > > >> By constant I mean the CHROME_DELETE portion. For example, if you change > > >> the > > >> name of the mv file this test will continue to succeed no matter what. > > > > > > Done. Also did to 'Browser Test'. > > > > > > https://codereview.chromium.org/1986823002/ > > > > -- > > You received this message because you are subscribed to the Google Groups > > "Chromium-reviews" group. > > To unsubscribe from this group and stop receiving emails from it, send an > email > > to mailto:chromium-reviews+unsubscribe@chromium.org. > >
q from the sidelines: how much of the code can be tested in a unittest? does getting coverage there simplify the browser test at all? https://codereview.chromium.org/1986823002/diff/460001/chrome/app/chrome_main... File chrome/app/chrome_main_delegate.cc (right): https://codereview.chromium.org/1986823002/diff/460001/chrome/app/chrome_main... chrome/app/chrome_main_delegate.cc:726: MoveUserDataForFirstRunAfterDowngrade(); since the first thing Move... does is get DIR_USER_DATA, how about taking it as an in-param and moving this statement down within the "if" below? https://codereview.chromium.org/1986823002/diff/460001/chrome/browser/downgra... File chrome/browser/downgrade/user_data_downgrade.h (right): https://codereview.chromium.org/1986823002/diff/460001/chrome/browser/downgra... chrome/browser/downgrade/user_data_downgrade.h:12: extern const base::FilePath::CharType g_downgrade_delete_suffix[]; these are constants, so use kFoo naming https://codereview.chromium.org/1986823002/diff/460001/chrome/browser/downgra... File chrome/browser/downgrade/user_data_downgrade_browsertest.cc (right): https://codereview.chromium.org/1986823002/diff/460001/chrome/browser/downgra... chrome/browser/downgrade/user_data_downgrade_browsertest.cc:21: // Verify the renamed user data directory has been deleted. it's common to document the class that declares overidden methods like so: // content::BrowserTestBase: https://codereview.chromium.org/1986823002/diff/460001/chrome/browser/downgra... chrome/browser/downgrade/user_data_downgrade_browsertest.cc:30: bool SetUpUserDataDirectory() override { // InProcessBrowserTest: also, please move this up above the TearDown method above so that they're listed in the order they're invoked https://codereview.chromium.org/1986823002/diff/460001/chrome/browser/downgra... chrome/browser/downgrade/user_data_downgrade_browsertest.cc:47: base::FilePath other_file_; please document. maybe something like: // The path to an arbitrary file in the user data dir that will be present only when a reset does not take place.
https://codereview.chromium.org/1986823002/diff/460001/chrome/app/chrome_main... File chrome/app/chrome_main_delegate.cc (right): https://codereview.chromium.org/1986823002/diff/460001/chrome/app/chrome_main... chrome/app/chrome_main_delegate.cc:726: MoveUserDataForFirstRunAfterDowngrade(); On 2016/06/07 01:25:28, grt (slow) wrote: > since the first thing Move... does is get DIR_USER_DATA, how about taking it as > an in-param and moving this statement down within the "if" below? MoveUserDataForFirstRunAfterDowngrade() not only moves user data dir but also disk cache dir. There is more consistency if both paths are obtained inside the function. Also it reduces the number parameters of the function. Even though PathService::Get will be called twice I think it is a better choice. https://codereview.chromium.org/1986823002/diff/460001/chrome/browser/downgra... File chrome/browser/downgrade/user_data_downgrade.h (right): https://codereview.chromium.org/1986823002/diff/460001/chrome/browser/downgra... chrome/browser/downgrade/user_data_downgrade.h:12: extern const base::FilePath::CharType g_downgrade_delete_suffix[]; On 2016/06/07 01:25:28, grt (slow) wrote: > these are constants, so use kFoo naming Done. https://codereview.chromium.org/1986823002/diff/460001/chrome/browser/downgra... File chrome/browser/downgrade/user_data_downgrade_browsertest.cc (right): https://codereview.chromium.org/1986823002/diff/460001/chrome/browser/downgra... chrome/browser/downgrade/user_data_downgrade_browsertest.cc:21: // Verify the renamed user data directory has been deleted. On 2016/06/07 01:25:28, grt (slow) wrote: > it's common to document the class that declares overidden methods like so: > // content::BrowserTestBase: Done. Also did to SetUpInProcessBrowserTestFixture() below. https://codereview.chromium.org/1986823002/diff/460001/chrome/browser/downgra... chrome/browser/downgrade/user_data_downgrade_browsertest.cc:30: bool SetUpUserDataDirectory() override { On 2016/06/07 01:25:28, grt (slow) wrote: > // InProcessBrowserTest: > > also, please move this up above the TearDown method above so that they're listed > in the order they're invoked Done. https://codereview.chromium.org/1986823002/diff/460001/chrome/browser/downgra... chrome/browser/downgrade/user_data_downgrade_browsertest.cc:47: base::FilePath other_file_; On 2016/06/07 01:25:28, grt (slow) wrote: > please document. maybe something like: > // The path to an arbitrary file in the user data dir that will be present > only when a reset does not take place. Done.
my other question remains: how much of the code can be tested in a unittest? does getting coverage there simplify the browser test at all? https://codereview.chromium.org/1986823002/diff/460001/chrome/app/chrome_main... File chrome/app/chrome_main_delegate.cc (right): https://codereview.chromium.org/1986823002/diff/460001/chrome/app/chrome_main... chrome/app/chrome_main_delegate.cc:726: MoveUserDataForFirstRunAfterDowngrade(); On 2016/06/07 14:41:49, zmin wrote: > On 2016/06/07 01:25:28, grt (slow) wrote: > > since the first thing Move... does is get DIR_USER_DATA, how about taking it > as > > an in-param and moving this statement down within the "if" below? > > MoveUserDataForFirstRunAfterDowngrade() not only moves user data dir but also > disk cache dir. There is more consistency if both paths are obtained inside the > function. Also it reduces the number parameters of the function. Even though > PathService::Get will be called twice I think it is a better choice. That's a good argument. Why does UpdateBrowserVersion take the user data dir as an in param, then? https://codereview.chromium.org/1986823002/diff/480001/chrome/browser/downgra... File chrome/browser/downgrade/user_data_downgrade_browsertest.cc (right): https://codereview.chromium.org/1986823002/diff/480001/chrome/browser/downgra... chrome/browser/downgrade/user_data_downgrade_browsertest.cc:35: // Verify the renamed user data directory has been deleted. swap these two comments. the first tells the reader "the overridden methods below here originate from "content::BrowserTestBase". the second one documents the specific function. if you search around, you may find that some locations add a blank line between the "originating class" comment and the method doc comment. i'm not sure what the convention is there.
On Mon, Jun 6, 2016 at 6:10 PM, <zmin@chromium.org> wrote: > On 2016/06/06 23:22:06, sky wrote: > >> On Mon, Jun 6, 2016 at 3:29 PM, <mailto:zmin@chromium.org> wrote: >> >> >> > >> > https://codereview.chromium.org/1986823002/diff/420001/chrome/browser/downgra... >> >> File chrome/browser/downgrade/user_data_downgrade.cc (right): >> >> >> >> >> > >> > https://codereview.chromium.org/1986823002/diff/420001/chrome/browser/downgra... >> >> chrome/browser/downgrade/user_data_downgrade.cc:51: >> >> FILE_PATH_LITERAL("Last >> >> Chrome Version"); >> >> On 2016/06/03 15:46:52, zmin wrote: >> >> > On 2016/06/03 00:04:24, sky wrote: >> >> > > nit: Chrome Version? Generally we don't use 'Last' for things that >> >> > > are >> >> > current. >> >> > > For example, we have Bookmarks, not 'Last Bookmarks'. >> >> > >> >> > I use 'Last' here because this file is used to indicate the version >> >> > of >> >> > last >> >> > launched Chrome so that we're able to know whether the profiles needs >> >> > to >> >> > be >> >> > deleted or not. >> >> > >> >> > What's more, I think Chrome use 'last' in 'Local State' file for >> >> > chrome >> >> version >> >> > too. >> >> >> >> I'm still of the opinion 'Last' is unnecessary here. I would also avoid >> > 'Chrome' >> >> too as I assume this will be written for chromium builds too. >> > >> > >> > Change it to "Browser Version" except some internal variable name so >> > they >> > are >> > different from 'current'. >> >> That names is still confusing too. AFAIK there are no files with the >> name 'browser'. > > How about "Client Version"? It means the version of user(Chrome) of that > directory. > > Or "Profiles Version"/"Data Version"? It presents the version of dir itself. I still like 'Chrome Version'/'Chromium Version'. > > >> > >> > https://codereview.chromium.org/1986823002/diff/420001/chrome/browser/downgra... >> >> chrome/browser/downgrade/user_data_downgrade.cc:88: if (!source.empty() >> >> && >> >> !target.empty() && base::Move(source, target) && >> >> On 2016/06/03 15:46:52, zmin wrote: >> >> > On 2016/06/03 00:04:24, sky wrote: >> >> > > What if Move fails? Doesn't that leave things in a weird state? >> >> > >> >> > If move failed, we just end up with a higher version profile with >> >> > lower >> >> version >> >> > Chrome and a pop up error window if the format is different. >> >> > >> >> >> >> That's assuming every file deals with a version from the future, which >> >> I >> >> don't >> >> believe is the case. If it did, then we wouldn't need to nuke it, >> >> right? >> > Firstly, we shall still nuke it even with the pop-up because it's not a >> > user-friendly pop-up and I want to make sure Chrome is working fine with >> > as >> > less >> > user interactive as possible. >> > >> > There're things that can fail the move: >> > 1) Multiple processes move dir at the same time. One should success, the >> > others >> > should fail by design. >> > 2) Path is set to root directory. I have update the policy web page to >> > tell >> > user >> > avoid using that and IT-admins have to manually delete the profiles in >> > this >> > case. >> > 3) Other Errors. Admins have to manually delete the profiles. >> > >> > In the worst case, admins have to manually delete the profile to make >> > Chrome >> > work again. We have a help page to tell user do that anyway. And >> > downgrade >> > is >> > something only should be done with support engineer. >> >> I'm still confused by this. It seems that if the move fails we >> shouldn't start, no? > > No. > > Firstly, some of the failures are on purpose. They're meant to prevent > multiple > processes from moving the user data dir for several times because there is > no > protection of singleton at this stage. > > Secondly, if there's an error (i.e. IO Error), the dir cannot be used > anyway. If > this happens, user data dir initialization in the previous step should > handle > this issue. How do you know that? What if the move fails because another process (say virus scanner) has a file open? If you continue then you're not solving the problem you want to address. That you are willing to continue makes me question the entire feature. -Scott > > After all, most of moving failures should not be a problem, and the > possibility > of the worst case is quite low (error occurs just from moving + profiles > format > changes + warning dialogue doesn't catch the error). Manual deletion should > solve the worst case. And it can be provided by support engineers during the > downgrade. I think it isn't worth blocking the launch because of that. > > > > >> > >> > https://codereview.chromium.org/1986823002/diff/420001/chrome/browser/downgra... >> >> File chrome/browser/downgrade/user_data_downgrade_browsertest.cc >> >> (right): >> >> >> >> >> > >> > https://codereview.chromium.org/1986823002/diff/420001/chrome/browser/downgra... >> >> chrome/browser/downgrade/user_data_downgrade_browsertest.cc:35: >> >> FILE_PATH_LITERAL(" (1).CHROME_DELETE"); >> >> On 2016/06/03 15:46:52, zmin wrote: >> >> > On 2016/06/03 00:04:24, sky wrote: >> >> > > Make a constant for this. In fact you should also verify profile is >> > actually >> >> > > moved to this directory. >> >> > >> >> > The moved profile may be deleted very quickly during launch before we >> >> > are >> > able >> >> > to check it. >> >> >> >> You need some way to verify all this is working. Add callbacks/observer >> >> from >> > the >> >> test if you need to. >> > Good news: I found a way to verify this. >> > Bad news: It can only be done after solving the PostTaskAfterStartup >> > issue. >> > >> > The InProcessBrowserTest::RunTestOnMainThreadLoop() will call >> > AfterStartupTaskUtils::SetBrowserStartupIsCompleteForTesting() to >> > trigger >> > all >> > after-startup-task. >> > It means I can verify the moved dir just before the deletion task by >> > overriding >> > RunTestOnMainThreadLoop() function. >> > However, I can't do it now because I can't really use >> > PostTaskAfterStart() >> > function in the test. >> > >> > >> >> > And it's constant already. >> >> >> >> By constant I mean the CHROME_DELETE portion. For example, if you >> >> change >> >> the >> >> name of the mv file this test will continue to succeed no matter what. >> > >> > Done. Also did to 'Browser Test'. >> > >> > https://codereview.chromium.org/1986823002/ >> >> -- >> You received this message because you are subscribed to the Google Groups >> "Chromium-reviews" group. >> To unsubscribe from this group and stop receiving emails from it, send an > email >> to mailto:chromium-reviews+unsubscribe@chromium.org. >> > > > https://codereview.chromium.org/1986823002/ -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2016/06/07 15:40:44, grt (slow) wrote: > my other question remains: how much of the code can be tested in a unittest? > does getting coverage there simplify the browser test at all? I don't see too many differences between browser test and unit test here. I have to create multiple test classes for browser test because I need different setup functions. But in the meanwhile, I got automatically |user_data_dir| overridden. After all, I don't feel like there is something unit test can't do but browser test can or is difficult for browser test but easy for unit test. > https://codereview.chromium.org/1986823002/diff/460001/chrome/app/chrome_main... > File chrome/app/chrome_main_delegate.cc (right): > > https://codereview.chromium.org/1986823002/diff/460001/chrome/app/chrome_main... > chrome/app/chrome_main_delegate.cc:726: MoveUserDataForFirstRunAfterDowngrade(); > On 2016/06/07 14:41:49, zmin wrote: > > On 2016/06/07 01:25:28, grt (slow) wrote: > > > since the first thing Move... does is get DIR_USER_DATA, how about taking it > > as > > > an in-param and moving this statement down within the "if" below? > > > > MoveUserDataForFirstRunAfterDowngrade() not only moves user data dir but also > > disk cache dir. There is more consistency if both paths are obtained inside > the > > function. Also it reduces the number parameters of the function. Even though > > PathService::Get will be called twice I think it is a better choice. > > That's a good argument. Why does UpdateBrowserVersion take the user data dir as > an in param, then? My understanding is MoveUserDataForFirstRunAfterDowngrade() is a more high level function that hides as much details as possible. While UpdateBrowserVersion() and GetBrowserVersion() is a pair of getter and setter and should only focus on the IO. > https://codereview.chromium.org/1986823002/diff/480001/chrome/browser/downgra... > File chrome/browser/downgrade/user_data_downgrade_browsertest.cc (right): > > https://codereview.chromium.org/1986823002/diff/480001/chrome/browser/downgra... > chrome/browser/downgrade/user_data_downgrade_browsertest.cc:35: // Verify the > renamed user data directory has been deleted. > swap these two comments. the first tells the reader "the overridden methods > below here originate from "content::BrowserTestBase". the second one documents > the specific function. if you search around, you may find that some locations > add a blank line between the "originating class" comment and the method doc > comment. i'm not sure what the convention is there. Done. I think there's no blank line for most of comments I can find.
On 2016/06/07 15:40:44, grt (slow) wrote: > my other question remains: how much of the code can be tested in a unittest? > does getting coverage there simplify the browser test at all? I don't see too many differences between browser test and unit test here. I have to create multiple test classes for browser test because I need different setup functions. But in the meanwhile, I got automatically |user_data_dir| overridden. After all, I don't feel like there is something unit test can't do but browser test can or is difficult for browser test but easy for unit test. > https://codereview.chromium.org/1986823002/diff/460001/chrome/app/chrome_main... > File chrome/app/chrome_main_delegate.cc (right): > > https://codereview.chromium.org/1986823002/diff/460001/chrome/app/chrome_main... > chrome/app/chrome_main_delegate.cc:726: MoveUserDataForFirstRunAfterDowngrade(); > On 2016/06/07 14:41:49, zmin wrote: > > On 2016/06/07 01:25:28, grt (slow) wrote: > > > since the first thing Move... does is get DIR_USER_DATA, how about taking it > > as > > > an in-param and moving this statement down within the "if" below? > > > > MoveUserDataForFirstRunAfterDowngrade() not only moves user data dir but also > > disk cache dir. There is more consistency if both paths are obtained inside > the > > function. Also it reduces the number parameters of the function. Even though > > PathService::Get will be called twice I think it is a better choice. > > That's a good argument. Why does UpdateBrowserVersion take the user data dir as > an in param, then? My understanding is MoveUserDataForFirstRunAfterDowngrade() is a more high level function that hides as much details as possible. While UpdateBrowserVersion() and GetBrowserVersion() is a pair of getter and setter and should only focus on the IO. > https://codereview.chromium.org/1986823002/diff/480001/chrome/browser/downgra... > File chrome/browser/downgrade/user_data_downgrade_browsertest.cc (right): > > https://codereview.chromium.org/1986823002/diff/480001/chrome/browser/downgra... > chrome/browser/downgrade/user_data_downgrade_browsertest.cc:35: // Verify the > renamed user data directory has been deleted. > swap these two comments. the first tells the reader "the overridden methods > below here originate from "content::BrowserTestBase". the second one documents > the specific function. if you search around, you may find that some locations > add a blank line between the "originating class" comment and the method doc > comment. i'm not sure what the convention is there. Done. I think there's no blank line for most of comments I can find.
On 2016/06/07 17:28:40, sky wrote: > On Mon, Jun 6, 2016 at 6:10 PM, <mailto:zmin@chromium.org> wrote: > > On 2016/06/06 23:22:06, sky wrote: > > > >> On Mon, Jun 6, 2016 at 3:29 PM, <mailto:zmin@chromium.org> wrote: > >> >> > >> > > >> > > > https://codereview.chromium.org/1986823002/diff/420001/chrome/browser/downgra... > >> >> File chrome/browser/downgrade/user_data_downgrade.cc (right): > >> >> > >> >> > >> > > >> > > > https://codereview.chromium.org/1986823002/diff/420001/chrome/browser/downgra... > >> >> chrome/browser/downgrade/user_data_downgrade.cc:51: > >> >> FILE_PATH_LITERAL("Last > >> >> Chrome Version"); > >> >> On 2016/06/03 15:46:52, zmin wrote: > >> >> > On 2016/06/03 00:04:24, sky wrote: > >> >> > > nit: Chrome Version? Generally we don't use 'Last' for things that > >> >> > > are > >> >> > current. > >> >> > > For example, we have Bookmarks, not 'Last Bookmarks'. > >> >> > > >> >> > I use 'Last' here because this file is used to indicate the version > >> >> > of > >> >> > last > >> >> > launched Chrome so that we're able to know whether the profiles needs > >> >> > to > >> >> > be > >> >> > deleted or not. > >> >> > > >> >> > What's more, I think Chrome use 'last' in 'Local State' file for > >> >> > chrome > >> >> version > >> >> > too. > >> >> > >> >> I'm still of the opinion 'Last' is unnecessary here. I would also avoid > >> > 'Chrome' > >> >> too as I assume this will be written for chromium builds too. > >> > > >> > > >> > Change it to "Browser Version" except some internal variable name so > >> > they > >> > are > >> > different from 'current'. > >> > >> That names is still confusing too. AFAIK there are no files with the > >> name 'browser'. > > > > How about "Client Version"? It means the version of user(Chrome) of that > > directory. > > > > Or "Profiles Version"/"Data Version"? It presents the version of dir itself. > > I still like 'Chrome Version'/'Chromium Version'. Sure, I'll do that with #if defined(GOOGLE_CHROME_BUILD) > > > > > >> > > >> > > > https://codereview.chromium.org/1986823002/diff/420001/chrome/browser/downgra... > >> >> chrome/browser/downgrade/user_data_downgrade.cc:88: if (!source.empty() > >> >> && > >> >> !target.empty() && base::Move(source, target) && > >> >> On 2016/06/03 15:46:52, zmin wrote: > >> >> > On 2016/06/03 00:04:24, sky wrote: > >> >> > > What if Move fails? Doesn't that leave things in a weird state? > >> >> > > >> >> > If move failed, we just end up with a higher version profile with > >> >> > lower > >> >> version > >> >> > Chrome and a pop up error window if the format is different. > >> >> > > >> >> > >> >> That's assuming every file deals with a version from the future, which > >> >> I > >> >> don't > >> >> believe is the case. If it did, then we wouldn't need to nuke it, > >> >> right? > >> > Firstly, we shall still nuke it even with the pop-up because it's not a > >> > user-friendly pop-up and I want to make sure Chrome is working fine with > >> > as > >> > less > >> > user interactive as possible. > >> > > >> > There're things that can fail the move: > >> > 1) Multiple processes move dir at the same time. One should success, the > >> > others > >> > should fail by design. > >> > 2) Path is set to root directory. I have update the policy web page to > >> > tell > >> > user > >> > avoid using that and IT-admins have to manually delete the profiles in > >> > this > >> > case. > >> > 3) Other Errors. Admins have to manually delete the profiles. > >> > > >> > In the worst case, admins have to manually delete the profile to make > >> > Chrome > >> > work again. We have a help page to tell user do that anyway. And > >> > downgrade > >> > is > >> > something only should be done with support engineer. > >> > >> I'm still confused by this. It seems that if the move fails we > >> shouldn't start, no? > > > > No. > > > > Firstly, some of the failures are on purpose. They're meant to prevent > > multiple > > processes from moving the user data dir for several times because there is > > no > > protection of singleton at this stage. > > > > Secondly, if there's an error (i.e. IO Error), the dir cannot be used > > anyway. If > > this happens, user data dir initialization in the previous step should > > handle > > this issue. > > How do you know that? What if the move fails because another process > (say virus scanner) has a file open? If you continue then you're not > solving the problem you want to address. That you are willing to > continue makes me question the entire feature. > > -Scott The philosophy of this feature is trying to help IT-Admins as much as possible. There're two tradeoff here. 1) Should the launch be blocked or continued if move failed. Block means no Chrome for user and continue won't make the situation worse than that. 2) Should the profile be deleted automatically with possible failure or leave the entire things to IT-Admins. I want to give as less work as possible to IT-Admins because downgrade is used in an emergency situation. Throwing the problem to IT-Admins doesn't solve it. Virus scanner can block IT-Admins as well. There is no perfect way to address these edge cases. The most important reason makes me feel it's OK not to cover these cases is downgrade is a feature designed for IT-Admins and support engineer who should know Chrome better than normal user. > > > > After all, most of moving failures should not be a problem, and the > > possibility > > of the worst case is quite low (error occurs just from moving + profiles > > format > > changes + warning dialogue doesn't catch the error). Manual deletion should > > solve the worst case. And it can be provided by support engineers during the > > downgrade. I think it isn't worth blocking the launch because of that. > > > > > > > > > >> > > >> > > > https://codereview.chromium.org/1986823002/diff/420001/chrome/browser/downgra... > >> >> File chrome/browser/downgrade/user_data_downgrade_browsertest.cc > >> >> (right): > >> >> > >> >> > >> > > >> > > > https://codereview.chromium.org/1986823002/diff/420001/chrome/browser/downgra... > >> >> chrome/browser/downgrade/user_data_downgrade_browsertest.cc:35: > >> >> FILE_PATH_LITERAL(" (1).CHROME_DELETE"); > >> >> On 2016/06/03 15:46:52, zmin wrote: > >> >> > On 2016/06/03 00:04:24, sky wrote: > >> >> > > Make a constant for this. In fact you should also verify profile is > >> > actually > >> >> > > moved to this directory. > >> >> > > >> >> > The moved profile may be deleted very quickly during launch before we > >> >> > are > >> > able > >> >> > to check it. > >> >> > >> >> You need some way to verify all this is working. Add callbacks/observer > >> >> from > >> > the > >> >> test if you need to. > >> > Good news: I found a way to verify this. > >> > Bad news: It can only be done after solving the PostTaskAfterStartup > >> > issue. > >> > > >> > The InProcessBrowserTest::RunTestOnMainThreadLoop() will call > >> > AfterStartupTaskUtils::SetBrowserStartupIsCompleteForTesting() to > >> > trigger > >> > all > >> > after-startup-task. > >> > It means I can verify the moved dir just before the deletion task by > >> > overriding > >> > RunTestOnMainThreadLoop() function. > >> > However, I can't do it now because I can't really use > >> > PostTaskAfterStart() > >> > function in the test. > >> > > >> > > >> >> > And it's constant already. > >> >> > >> >> By constant I mean the CHROME_DELETE portion. For example, if you > >> >> change > >> >> the > >> >> name of the mv file this test will continue to succeed no matter what. > >> > > >> > Done. Also did to 'Browser Test'. > >> > > >> > https://codereview.chromium.org/1986823002/ > >> > >> -- > >> You received this message because you are subscribed to the Google Groups > >> "Chromium-reviews" group. > >> To unsubscribe from this group and stop receiving emails from it, send an > > email > >> to mailto:chromium-reviews+unsubscribe@chromium.org. > >> > > > > > > https://codereview.chromium.org/1986823002/ > > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. >
Patchset #24 (id:500001) has been deleted
As Brett asked in the design review and I'll ask here again, can we only maintain the version file if it's going to be needed? By that I mean this file only matters for managed users, so only updated it then. If you go from managed to not managed we delete the file. On Tue, Jun 7, 2016 at 4:00 PM, <zmin@chromium.org> wrote: > On 2016/06/07 17:28:40, sky wrote: > >> On Mon, Jun 6, 2016 at 6:10 PM, <mailto:zmin@chromium.org> wrote: >> > On 2016/06/06 23:22:06, sky wrote: >> > >> >> On Mon, Jun 6, 2016 at 3:29 PM, <mailto:zmin@chromium.org> wrote: >> >> >> >> >> > >> >> >> > >> > https://codereview.chromium.org/1986823002/diff/420001/chrome/browser/downgra... >> >> >> File chrome/browser/downgrade/user_data_downgrade.cc (right): >> >> >> >> >> >> >> >> > >> >> >> > >> > https://codereview.chromium.org/1986823002/diff/420001/chrome/browser/downgra... >> >> >> chrome/browser/downgrade/user_data_downgrade.cc:51: >> >> >> FILE_PATH_LITERAL("Last >> >> >> Chrome Version"); >> >> >> On 2016/06/03 15:46:52, zmin wrote: >> >> >> > On 2016/06/03 00:04:24, sky wrote: >> >> >> > > nit: Chrome Version? Generally we don't use 'Last' for things >> >> >> > > that >> >> >> > > are >> >> >> > current. >> >> >> > > For example, we have Bookmarks, not 'Last Bookmarks'. >> >> >> > >> >> >> > I use 'Last' here because this file is used to indicate the >> >> >> > version >> >> >> > of >> >> >> > last >> >> >> > launched Chrome so that we're able to know whether the profiles >> >> >> > needs >> >> >> > to >> >> >> > be >> >> >> > deleted or not. >> >> >> > >> >> >> > What's more, I think Chrome use 'last' in 'Local State' file for >> >> >> > chrome >> >> >> version >> >> >> > too. >> >> >> >> >> >> I'm still of the opinion 'Last' is unnecessary here. I would also >> >> >> avoid >> >> > 'Chrome' >> >> >> too as I assume this will be written for chromium builds too. >> >> > >> >> > >> >> > Change it to "Browser Version" except some internal variable name so >> >> > they >> >> > are >> >> > different from 'current'. >> >> >> >> That names is still confusing too. AFAIK there are no files with the >> >> name 'browser'. >> > >> > How about "Client Version"? It means the version of user(Chrome) of that >> > directory. >> > >> > Or "Profiles Version"/"Data Version"? It presents the version of dir >> > itself. >> >> I still like 'Chrome Version'/'Chromium Version'. > > Sure, I'll do that with #if defined(GOOGLE_CHROME_BUILD) > > >> > >> > >> >> > >> >> >> > >> > https://codereview.chromium.org/1986823002/diff/420001/chrome/browser/downgra... >> >> >> chrome/browser/downgrade/user_data_downgrade.cc:88: if >> >> >> (!source.empty() >> >> >> && >> >> >> !target.empty() && base::Move(source, target) && >> >> >> On 2016/06/03 15:46:52, zmin wrote: >> >> >> > On 2016/06/03 00:04:24, sky wrote: >> >> >> > > What if Move fails? Doesn't that leave things in a weird state? >> >> >> > >> >> >> > If move failed, we just end up with a higher version profile with >> >> >> > lower >> >> >> version >> >> >> > Chrome and a pop up error window if the format is different. >> >> >> > >> >> >> >> >> >> That's assuming every file deals with a version from the future, >> >> >> which >> >> >> I >> >> >> don't >> >> >> believe is the case. If it did, then we wouldn't need to nuke it, >> >> >> right? >> >> > Firstly, we shall still nuke it even with the pop-up because it's not >> >> > a >> >> > user-friendly pop-up and I want to make sure Chrome is working fine >> >> > with >> >> > as >> >> > less >> >> > user interactive as possible. >> >> > >> >> > There're things that can fail the move: >> >> > 1) Multiple processes move dir at the same time. One should success, >> >> > the >> >> > others >> >> > should fail by design. >> >> > 2) Path is set to root directory. I have update the policy web page >> >> > to >> >> > tell >> >> > user >> >> > avoid using that and IT-admins have to manually delete the profiles >> >> > in >> >> > this >> >> > case. >> >> > 3) Other Errors. Admins have to manually delete the profiles. >> >> > >> >> > In the worst case, admins have to manually delete the profile to make >> >> > Chrome >> >> > work again. We have a help page to tell user do that anyway. And >> >> > downgrade >> >> > is >> >> > something only should be done with support engineer. >> >> >> >> I'm still confused by this. It seems that if the move fails we >> >> shouldn't start, no? >> > >> > No. >> > >> > Firstly, some of the failures are on purpose. They're meant to prevent >> > multiple >> > processes from moving the user data dir for several times because there >> > is >> > no >> > protection of singleton at this stage. >> > >> > Secondly, if there's an error (i.e. IO Error), the dir cannot be used >> > anyway. If >> > this happens, user data dir initialization in the previous step should >> > handle >> > this issue. >> >> How do you know that? What if the move fails because another process >> (say virus scanner) has a file open? If you continue then you're not >> solving the problem you want to address. That you are willing to >> continue makes me question the entire feature. >> >> -Scott > > The philosophy of this feature is trying to help IT-Admins as much as > possible. > > There're two tradeoff here. > 1) Should the launch be blocked or continued if move failed. > Block means no Chrome for user and continue won't make the situation worse > than > that. > > 2) Should the profile be deleted automatically with possible failure or > leave > the entire things to IT-Admins. > I want to give as less work as possible to IT-Admins because downgrade is > used > in an emergency situation. > Throwing the problem to IT-Admins doesn't solve it. Virus scanner can block > IT-Admins as well. > > There is no perfect way to address these edge cases. The most important > reason > makes me feel it's OK not to cover these cases is downgrade is a feature > designed for IT-Admins and support engineer who should know Chrome better > than > normal user. > > >> > >> > After all, most of moving failures should not be a problem, and the >> > possibility >> > of the worst case is quite low (error occurs just from moving + profiles >> > format >> > changes + warning dialogue doesn't catch the error). Manual deletion >> > should >> > solve the worst case. And it can be provided by support engineers during >> > the >> > downgrade. I think it isn't worth blocking the launch because of that. >> > >> > >> > >> > >> >> > >> >> >> > >> > https://codereview.chromium.org/1986823002/diff/420001/chrome/browser/downgra... >> >> >> File chrome/browser/downgrade/user_data_downgrade_browsertest.cc >> >> >> (right): >> >> >> >> >> >> >> >> > >> >> >> > >> > https://codereview.chromium.org/1986823002/diff/420001/chrome/browser/downgra... >> >> >> chrome/browser/downgrade/user_data_downgrade_browsertest.cc:35: >> >> >> FILE_PATH_LITERAL(" (1).CHROME_DELETE"); >> >> >> On 2016/06/03 15:46:52, zmin wrote: >> >> >> > On 2016/06/03 00:04:24, sky wrote: >> >> >> > > Make a constant for this. In fact you should also verify profile >> >> >> > > is >> >> > actually >> >> >> > > moved to this directory. >> >> >> > >> >> >> > The moved profile may be deleted very quickly during launch before >> >> >> > we >> >> >> > are >> >> > able >> >> >> > to check it. >> >> >> >> >> >> You need some way to verify all this is working. Add >> >> >> callbacks/observer >> >> >> from >> >> > the >> >> >> test if you need to. >> >> > Good news: I found a way to verify this. >> >> > Bad news: It can only be done after solving the PostTaskAfterStartup >> >> > issue. >> >> > >> >> > The InProcessBrowserTest::RunTestOnMainThreadLoop() will call >> >> > AfterStartupTaskUtils::SetBrowserStartupIsCompleteForTesting() to >> >> > trigger >> >> > all >> >> > after-startup-task. >> >> > It means I can verify the moved dir just before the deletion task by >> >> > overriding >> >> > RunTestOnMainThreadLoop() function. >> >> > However, I can't do it now because I can't really use >> >> > PostTaskAfterStart() >> >> > function in the test. >> >> > >> >> > >> >> >> > And it's constant already. >> >> >> >> >> >> By constant I mean the CHROME_DELETE portion. For example, if you >> >> >> change >> >> >> the >> >> >> name of the mv file this test will continue to succeed no matter >> >> >> what. >> >> > >> >> > Done. Also did to 'Browser Test'. >> >> > >> >> > https://codereview.chromium.org/1986823002/ >> >> >> >> -- >> >> You received this message because you are subscribed to the Google >> >> Groups >> >> "Chromium-reviews" group. >> >> To unsubscribe from this group and stop receiving emails from it, send >> >> an >> > email >> >> to mailto:chromium-reviews+unsubscribe@chromium.org. >> >> >> > >> > >> > https://codereview.chromium.org/1986823002/ >> >> -- >> You received this message because you are subscribed to the Google Groups >> "Chromium-reviews" group. >> To unsubscribe from this group and stop receiving emails from it, send an > email >> to mailto:chromium-reviews+unsubscribe@chromium.org. >> > > > https://codereview.chromium.org/1986823002/ -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2016/06/08 19:56:19, sky wrote: > As Brett asked in the design review and I'll ask here again, can we > only maintain the version file if it's going to be needed? By that I > mean this file only matters for managed users, so only updated it > then. If you go from managed to not managed we delete the file. Firstly, how to define managed? Group policy is not a good idea as my reply to Brett. One thing we can do is check the "msi" value in registry because enterprisers are expected to use the MSI file. However, it doesn't really mean the Chrome is managed. The MSI file can be downloaded by anyone. So the question is what is the benefits of doing this? Yes, I agree that we shall hide the downgrade feature from normal users. But can a version file cause any issue?
File io is slow, and this code blocks startup. We should only maintain the file if we know it might be used. -Scott On Wed, Jun 8, 2016 at 2:16 PM, <zmin@chromium.org> wrote: > On 2016/06/08 19:56:19, sky wrote: >> As Brett asked in the design review and I'll ask here again, can we >> only maintain the version file if it's going to be needed? By that I >> mean this file only matters for managed users, so only updated it >> then. If you go from managed to not managed we delete the file. > > Firstly, how to define managed? Group policy is not a good idea as my reply > to > Brett. One thing we can do is check the "msi" value in registry because > enterprisers are expected to use the MSI file. However, it doesn't really > mean > the Chrome is managed. The MSI file can be downloaded by anyone. > > So the question is what is the benefits of doing this? Yes, I agree that we > shall hide the downgrade feature from normal users. But can a version file > cause > any issue? > > > > https://codereview.chromium.org/1986823002/ -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2016/06/08 21:16:53, zmin wrote: > On 2016/06/08 19:56:19, sky wrote: > > As Brett asked in the design review and I'll ask here again, can we > > only maintain the version file if it's going to be needed? By that I > > mean this file only matters for managed users, so only updated it > > then. If you go from managed to not managed we delete the file. > > Firstly, how to define managed? Group policy is not a good idea as my reply to > Brett. One thing we can do is check the "msi" value in registry because > enterprisers are expected to use the MSI file. However, it doesn't really mean > the Chrome is managed. The MSI file can be downloaded by anyone. Since the downgrade feature is only supported for MSI installs, I think it makes sense to gate the in-product support on Chrome believing that it was installed via the MSI.
Is MSI the only way we know the account is managed? Or do we have more information but that information is only available when we open the profile? I talked with others on the name, and I'm ok with 'Last Version'. Sorry for the run around. -Scott On Thu, Jun 9, 2016 at 7:53 AM, <grt@chromium.org> wrote: > On 2016/06/08 21:16:53, zmin wrote: >> On 2016/06/08 19:56:19, sky wrote: >> > As Brett asked in the design review and I'll ask here again, can we >> > only maintain the version file if it's going to be needed? By that I >> > mean this file only matters for managed users, so only updated it >> > then. If you go from managed to not managed we delete the file. >> >> Firstly, how to define managed? Group policy is not a good idea as my >> reply to >> Brett. One thing we can do is check the "msi" value in registry because >> enterprisers are expected to use the MSI file. However, it doesn't really >> mean >> the Chrome is managed. The MSI file can be downloaded by anyone. > > Since the downgrade feature is only supported for MSI installs, I think it > makes > sense to gate the in-product support on Chrome believing that it was > installed > via the MSI. > > https://codereview.chromium.org/1986823002/ -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On Thu, Jun 9, 2016 at 1:27 PM, Scott Violet <sky@chromium.org> wrote: > Is MSI the only way we know the account is managed? Or do we have more > information but that information is only available when we open the > profile? > I don't know if there is a thing can 100% represent 'managed' in Chrome. However, MSI is one of the things that close enough. More important, the MSI command line option is the only supported way to trigger downgrade. In the other words, it's the best way I can find now that kicks the normal customers out without accidentally excludes the paying customers. I talked with others on the name, and I'm ok with 'Last Version'. > Sorry for the run around. OK, so we go for "Last Version". No Chrome, right? > -Scott > > On Thu, Jun 9, 2016 at 7:53 AM, <grt@chromium.org> wrote: > > On 2016/06/08 21:16:53, zmin wrote: > >> On 2016/06/08 19:56:19, sky wrote: > >> > As Brett asked in the design review and I'll ask here again, can we > >> > only maintain the version file if it's going to be needed? By that I > >> > mean this file only matters for managed users, so only updated it > >> > then. If you go from managed to not managed we delete the file. > >> > >> Firstly, how to define managed? Group policy is not a good idea as my > >> reply to > >> Brett. One thing we can do is check the "msi" value in registry because > >> enterprisers are expected to use the MSI file. However, it doesn't > really > >> mean > >> the Chrome is managed. The MSI file can be downloaded by anyone. > > > > Since the downgrade feature is only supported for MSI installs, I think > it > > makes > > sense to gate the in-product support on Chrome believing that it was > > installed > > via the MSI. > > > > https://codereview.chromium.org/1986823002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On Thu, Jun 9, 2016 at 3:07 PM, <zmin@google.com> wrote: > On Thu, Jun 9, 2016 at 1:27 PM, Scott Violet <sky@chromium.org> wrote: > >> Is MSI the only way we know the account is managed? Or do we have more >> information but that information is only available when we open the >> profile? >> > > I don't know if there is a thing can 100% represent 'managed' in Chrome. > However, MSI is one of the things that close enough. More important, the > MSI command line option is the only supported way to trigger downgrade. In > the other words, it's the best way I can find now that kicks the normal > customers out without accidentally excludes the paying customers. > One more thing, I don't want to try to delete the file if it's not a 'managed' Chrome because it needs file IO. A edge case for doing that is one user install a 'managed' Chrome, uninstall and reinstall a 'non-manager' Chrome. Then the file is there forever. Once Chrome is updated in the future, the content of that file will be confused. > > I talked with others on the name, and I'm ok with 'Last Version'. >> Sorry for the run around. > > OK, so we go for "Last Version". No Chrome, right? > > >> -Scott >> >> On Thu, Jun 9, 2016 at 7:53 AM, <grt@chromium.org> wrote: >> > On 2016/06/08 21:16:53, zmin wrote: >> >> On 2016/06/08 19:56:19, sky wrote: >> >> > As Brett asked in the design review and I'll ask here again, can we >> >> > only maintain the version file if it's going to be needed? By that I >> >> > mean this file only matters for managed users, so only updated it >> >> > then. If you go from managed to not managed we delete the file. >> >> >> >> Firstly, how to define managed? Group policy is not a good idea as my >> >> reply to >> >> Brett. One thing we can do is check the "msi" value in registry because >> >> enterprisers are expected to use the MSI file. However, it doesn't >> really >> >> mean >> >> the Chrome is managed. The MSI file can be downloaded by anyone. >> > >> > Since the downgrade feature is only supported for MSI installs, I think >> it >> > makes >> > sense to gate the in-product support on Chrome believing that it was >> > installed >> > via the MSI. >> > >> > https://codereview.chromium.org/1986823002/ >> > > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On Thu, Jun 9, 2016 at 12:07 PM, Owen Min <zmin@google.com> wrote: > On Thu, Jun 9, 2016 at 1:27 PM, Scott Violet <sky@chromium.org> wrote: >> >> Is MSI the only way we know the account is managed? Or do we have more >> information but that information is only available when we open the >> profile? > > > I don't know if there is a thing can 100% represent 'managed' in Chrome. > However, MSI is one of the things that close enough. More important, the MSI > command line option is the only supported way to trigger downgrade. In the > other words, it's the best way I can find now that kicks the normal > customers out without accidentally excludes the paying customers. > >> I talked with others on the name, and I'm ok with 'Last Version'. >> Sorry for the run around. > > OK, so we go for "Last Version". No Chrome, right? Yes. Sorry again for the run around. > >> >> -Scott >> >> On Thu, Jun 9, 2016 at 7:53 AM, <grt@chromium.org> wrote: >> > On 2016/06/08 21:16:53, zmin wrote: >> >> On 2016/06/08 19:56:19, sky wrote: >> >> > As Brett asked in the design review and I'll ask here again, can we >> >> > only maintain the version file if it's going to be needed? By that I >> >> > mean this file only matters for managed users, so only updated it >> >> > then. If you go from managed to not managed we delete the file. >> >> >> >> Firstly, how to define managed? Group policy is not a good idea as my >> >> reply to >> >> Brett. One thing we can do is check the "msi" value in registry because >> >> enterprisers are expected to use the MSI file. However, it doesn't >> >> really >> >> mean >> >> the Chrome is managed. The MSI file can be downloaded by anyone. >> > >> > Since the downgrade feature is only supported for MSI installs, I think >> > it >> > makes >> > sense to gate the in-product support on Chrome believing that it was >> > installed >> > via the MSI. >> > >> > https://codereview.chromium.org/1986823002/ > > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On Thu, Jun 9, 2016 at 12:44 PM, Owen Min <zmin@chromium.org> wrote: > On Thu, Jun 9, 2016 at 3:07 PM, <zmin@google.com> wrote: >> >> On Thu, Jun 9, 2016 at 1:27 PM, Scott Violet <sky@chromium.org> wrote: >>> >>> Is MSI the only way we know the account is managed? Or do we have more >>> information but that information is only available when we open the >>> profile? >> >> >> I don't know if there is a thing can 100% represent 'managed' in Chrome. >> However, MSI is one of the things that close enough. More important, the MSI >> command line option is the only supported way to trigger downgrade. In the >> other words, it's the best way I can find now that kicks the normal >> customers out without accidentally excludes the paying customers. > > > One more thing, I don't want to try to delete the file if it's not a > 'managed' Chrome because it needs file IO. A edge case for doing that is one > user install a 'managed' Chrome, uninstall and reinstall a 'non-manager' > Chrome. Then the file is there forever. Once Chrome is updated in the > future, the content of that file will be confused. Fair enough. > >> >> >>> I talked with others on the name, and I'm ok with 'Last Version'. >>> Sorry for the run around. >> >> OK, so we go for "Last Version". No Chrome, right? >> >>> >>> -Scott >>> >>> On Thu, Jun 9, 2016 at 7:53 AM, <grt@chromium.org> wrote: >>> > On 2016/06/08 21:16:53, zmin wrote: >>> >> On 2016/06/08 19:56:19, sky wrote: >>> >> > As Brett asked in the design review and I'll ask here again, can we >>> >> > only maintain the version file if it's going to be needed? By that I >>> >> > mean this file only matters for managed users, so only updated it >>> >> > then. If you go from managed to not managed we delete the file. >>> >> >>> >> Firstly, how to define managed? Group policy is not a good idea as my >>> >> reply to >>> >> Brett. One thing we can do is check the "msi" value in registry >>> >> because >>> >> enterprisers are expected to use the MSI file. However, it doesn't >>> >> really >>> >> mean >>> >> the Chrome is managed. The MSI file can be downloaded by anyone. >>> > >>> > Since the downgrade feature is only supported for MSI installs, I think >>> > it >>> > makes >>> > sense to gate the in-product support on Chrome believing that it was >>> > installed >>> > via the MSI. >>> > >>> > https://codereview.chromium.org/1986823002/ >> >> > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Also, I will be on vacation until the 16th, so you'll need another reviewer. You could try Brett as he reviewed the proposal and I discussed some of this with him. On Thu, Jun 9, 2016 at 3:37 PM, Scott Violet <sky@chromium.org> wrote: > On Thu, Jun 9, 2016 at 12:44 PM, Owen Min <zmin@chromium.org> wrote: >> On Thu, Jun 9, 2016 at 3:07 PM, <zmin@google.com> wrote: >>> >>> On Thu, Jun 9, 2016 at 1:27 PM, Scott Violet <sky@chromium.org> wrote: >>>> >>>> Is MSI the only way we know the account is managed? Or do we have more >>>> information but that information is only available when we open the >>>> profile? >>> >>> >>> I don't know if there is a thing can 100% represent 'managed' in Chrome. >>> However, MSI is one of the things that close enough. More important, the MSI >>> command line option is the only supported way to trigger downgrade. In the >>> other words, it's the best way I can find now that kicks the normal >>> customers out without accidentally excludes the paying customers. >> >> >> One more thing, I don't want to try to delete the file if it's not a >> 'managed' Chrome because it needs file IO. A edge case for doing that is one >> user install a 'managed' Chrome, uninstall and reinstall a 'non-manager' >> Chrome. Then the file is there forever. Once Chrome is updated in the >> future, the content of that file will be confused. > > Fair enough. > >> >>> >>> >>>> I talked with others on the name, and I'm ok with 'Last Version'. >>>> Sorry for the run around. >>> >>> OK, so we go for "Last Version". No Chrome, right? >>> >>>> >>>> -Scott >>>> >>>> On Thu, Jun 9, 2016 at 7:53 AM, <grt@chromium.org> wrote: >>>> > On 2016/06/08 21:16:53, zmin wrote: >>>> >> On 2016/06/08 19:56:19, sky wrote: >>>> >> > As Brett asked in the design review and I'll ask here again, can we >>>> >> > only maintain the version file if it's going to be needed? By that I >>>> >> > mean this file only matters for managed users, so only updated it >>>> >> > then. If you go from managed to not managed we delete the file. >>>> >> >>>> >> Firstly, how to define managed? Group policy is not a good idea as my >>>> >> reply to >>>> >> Brett. One thing we can do is check the "msi" value in registry >>>> >> because >>>> >> enterprisers are expected to use the MSI file. However, it doesn't >>>> >> really >>>> >> mean >>>> >> the Chrome is managed. The MSI file can be downloaded by anyone. >>>> > >>>> > Since the downgrade feature is only supported for MSI installs, I think >>>> > it >>>> > makes >>>> > sense to gate the in-product support on Chrome believing that it was >>>> > installed >>>> > via the MSI. >>>> > >>>> > https://codereview.chromium.org/1986823002/ >>> >>> >> -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2016/06/09 22:38:33, sky wrote: > Also, I will be on vacation until the 16th, so you'll need another > reviewer. You could try Brett as he reviewed the proposal and I > discussed some of this with him. OK, thanks a lot for the review. > On Thu, Jun 9, 2016 at 3:37 PM, Scott Violet <mailto:sky@chromium.org> wrote: > > On Thu, Jun 9, 2016 at 12:44 PM, Owen Min <mailto:zmin@chromium.org> wrote: > >> On Thu, Jun 9, 2016 at 3:07 PM, <mailto:zmin@google.com> wrote: > >>> > >>> On Thu, Jun 9, 2016 at 1:27 PM, Scott Violet <mailto:sky@chromium.org> wrote: > >>>> > >>>> Is MSI the only way we know the account is managed? Or do we have more > >>>> information but that information is only available when we open the > >>>> profile? > >>> > >>> > >>> I don't know if there is a thing can 100% represent 'managed' in Chrome. > >>> However, MSI is one of the things that close enough. More important, the > MSI > >>> command line option is the only supported way to trigger downgrade. In the > >>> other words, it's the best way I can find now that kicks the normal > >>> customers out without accidentally excludes the paying customers. > >> > >> > >> One more thing, I don't want to try to delete the file if it's not a > >> 'managed' Chrome because it needs file IO. A edge case for doing that is one > >> user install a 'managed' Chrome, uninstall and reinstall a 'non-manager' > >> Chrome. Then the file is there forever. Once Chrome is updated in the > >> future, the content of that file will be confused. > > > > Fair enough. > > > >> > >>> > >>> > >>>> I talked with others on the name, and I'm ok with 'Last Version'. > >>>> Sorry for the run around. > >>> > >>> OK, so we go for "Last Version". No Chrome, right? > >>> > >>>> > >>>> -Scott > >>>> > >>>> On Thu, Jun 9, 2016 at 7:53 AM, <mailto:grt@chromium.org> wrote: > >>>> > On 2016/06/08 21:16:53, zmin wrote: > >>>> >> On 2016/06/08 19:56:19, sky wrote: > >>>> >> > As Brett asked in the design review and I'll ask here again, can we > >>>> >> > only maintain the version file if it's going to be needed? By that I > >>>> >> > mean this file only matters for managed users, so only updated it > >>>> >> > then. If you go from managed to not managed we delete the file. > >>>> >> > >>>> >> Firstly, how to define managed? Group policy is not a good idea as my > >>>> >> reply to > >>>> >> Brett. One thing we can do is check the "msi" value in registry > >>>> >> because > >>>> >> enterprisers are expected to use the MSI file. However, it doesn't > >>>> >> really > >>>> >> mean > >>>> >> the Chrome is managed. The MSI file can be downloaded by anyone. > >>>> > > >>>> > Since the downgrade feature is only supported for MSI installs, I think > >>>> > it > >>>> > makes > >>>> > sense to gate the in-product support on Chrome believing that it was > >>>> > installed > >>>> > via the MSI. > >>>> > > >>>> > https://codereview.chromium.org/1986823002/ > >>> > >>> > >> > > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. >
zmin@chromium.org changed reviewers: - sky@chromium.org
zmin@chromium.org changed reviewers: + brettw@chromium.org
Hi Brett Please review changes in chrome/browser/chrome_browser_main.cc chrome/browser/downgrade/*
Just some stylistic suggestions. https://codereview.chromium.org/1986823002/diff/540001/chrome/browser/downgra... File chrome/browser/downgrade/user_data_downgrade.cc (right): https://codereview.chromium.org/1986823002/diff/540001/chrome/browser/downgra... chrome/browser/downgrade/user_data_downgrade.cc:159: base::Version GetLastVersion(const base::FilePath& user_data_dir) { I really think we need some kind of namespace for this file. Some of these functions, especially this one, are very generic. I would be file with just "namespace downgrade" wrapping all files in this directory. https://codereview.chromium.org/1986823002/diff/540001/chrome/browser/downgra... chrome/browser/downgrade/user_data_downgrade.cc:201: void SimulateDowngradeForTest(bool is_browser_test) { I was confused about this function since from the name I was expecting calling it to turn on test mode, and the arg to do something around browser_tests vs. other tyeps of test. How about this naming: void SetSimulateDowngradeForTest(bool similate_downgrade)
On 2016/06/10 22:42:42, brettw wrote: > Just some stylistic suggestions. > > https://codereview.chromium.org/1986823002/diff/540001/chrome/browser/downgra... > File chrome/browser/downgrade/user_data_downgrade.cc (right): > > https://codereview.chromium.org/1986823002/diff/540001/chrome/browser/downgra... > chrome/browser/downgrade/user_data_downgrade.cc:159: base::Version > GetLastVersion(const base::FilePath& user_data_dir) { > I really think we need some kind of namespace for this file. Some of these > functions, especially this one, are very generic. > > I would be file with just "namespace downgrade" wrapping all files in this > directory. Done. > https://codereview.chromium.org/1986823002/diff/540001/chrome/browser/downgra... > chrome/browser/downgrade/user_data_downgrade.cc:201: void > SimulateDowngradeForTest(bool is_browser_test) { > I was confused about this function since from the name I was expecting calling > it to turn on test mode, and the arg to do something around browser_tests vs. > other tyeps of test. > > How about this naming: > void SetSimulateDowngradeForTest(bool similate_downgrade) Done.
lgtm
The CQ bit was checked by zmin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from grt@chromium.org, pastarmovj@chromium.org Link to the patchset: https://codereview.chromium.org/1986823002/#ps560001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1986823002/560001
Message was sent while issue was closed.
Description was changed from ========== Reset user data directory and disk cache directory after downgrade. BUG=607592 ========== to ========== Reset user data directory and disk cache directory after downgrade. BUG=607592 ==========
Message was sent while issue was closed.
Committed patchset #26 (id:560001)
Message was sent while issue was closed.
CQ bit was unchecked
Message was sent while issue was closed.
Description was changed from ========== Reset user data directory and disk cache directory after downgrade. BUG=607592 ========== to ========== Reset user data directory and disk cache directory after downgrade. BUG=607592 Committed: https://crrev.com/18df9171296c5e7474adef49cd53d1638dbbb22c Cr-Commit-Position: refs/heads/master@{#399604} ==========
Message was sent while issue was closed.
Patchset 26 (id:??) landed as https://crrev.com/18df9171296c5e7474adef49cd53d1638dbbb22c Cr-Commit-Position: refs/heads/master@{#399604} |
