Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2201793002:
Fixed sole profile double deletion. (Closed)
DescriptionFixed sole profile double deletion.
It is still possible to perform double delete operation on sole profile
After sole profile scheduled for deletion a new profile will be created
asynchronously on blocking pool and only then FinishDeletingProfile will
be called to set ProfilesToDelete entry.
BUG=601049
R=anthonyvd@chromium.org, bauerb@chromium.org
Committed: https://crrev.com/c684d2e6774693c6b975e0ee7e7f3a134d5f31ea
Cr-Commit-Position: refs/heads/master@{#409837}
Patch Set 1 #
Total comments: 3
Patch Set 2 : review fixes #Patch Set 3 : fixed typo #Patch Set 4 : rewrite comments #
Total comments: 5
Patch Set 5 : review fixes #
Total comments: 1
Patch Set 6 : ABANDONED -> MARKED #Messages
Total messages: 14 (2 generated)
ping
(FTR, you need to "Publish + Mail Comments" before anything is sent to reviewers. Your Ping was the first email that was sent.) https://codereview.chromium.org/2201793002/diff/1/chrome/browser/profiles/pro... File chrome/browser/profiles/profile_manager.cc (right): https://codereview.chromium.org/2201793002/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_manager.cc:209: bool ScheduleProfileDirectoryForDeletion(const base::FilePath& path) { Document what the return value means and what the difference to QueueProfileDirectoryForDeletion() and ScheduleProfileForDeletion() is? https://codereview.chromium.org/2201793002/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_manager.cc:210: if (ContainsKey(ProfilesToDelete(), path)) { Nit: no braces for single-line bodies. https://codereview.chromium.org/2201793002/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_manager.cc:214: return true; Nit: remove one space after `return`.
On 2016/08/03 16:38:05, Bernhard Bauer wrote: > (FTR, you need to "Publish + Mail Comments" before anything is sent to > reviewers. Your Ping was the first email that was sent.) > > https://codereview.chromium.org/2201793002/diff/1/chrome/browser/profiles/pro... > File chrome/browser/profiles/profile_manager.cc (right): > > https://codereview.chromium.org/2201793002/diff/1/chrome/browser/profiles/pro... > chrome/browser/profiles/profile_manager.cc:209: bool > ScheduleProfileDirectoryForDeletion(const base::FilePath& path) { > Document what the return value means and what the difference to > QueueProfileDirectoryForDeletion() and ScheduleProfileForDeletion() is? > > https://codereview.chromium.org/2201793002/diff/1/chrome/browser/profiles/pro... > chrome/browser/profiles/profile_manager.cc:210: if > (ContainsKey(ProfilesToDelete(), path)) { > Nit: no braces for single-line bodies. > > https://codereview.chromium.org/2201793002/diff/1/chrome/browser/profiles/pro... > chrome/browser/profiles/profile_manager.cc:214: return true; > Nit: remove one space after `return`. done.
Thanks! Generally this looks good, but I think we can use somewhat clearer naming and add more comments (God knows this code is already pretty messy as it is...). https://codereview.chromium.org/2201793002/diff/60001/chrome/browser/profiles... File chrome/browser/profiles/profile_manager.cc (right): https://codereview.chromium.org/2201793002/diff/60001/chrome/browser/profiles... chrome/browser/profiles/profile_manager.cc:137: // Profile deletion can pass thru two stages: scheduled for deletion when where Nit: "through". Also, would it make sense to move this comment to the enum definition, and maybe split it up to have a comment for each of the enum values? https://codereview.chromium.org/2201793002/diff/60001/chrome/browser/profiles... chrome/browser/profiles/profile_manager.cc:138: // performing necessary activity prior to deletion, and abandoned stage when it I think we can also go into detail about what the necessary activity is -- it's creating a new profile as a replacement when the last profile is scheduled to be deleted, right? https://codereview.chromium.org/2201793002/diff/60001/chrome/browser/profiles... chrome/browser/profiles/profile_manager.cc:210: // Return true if profile can be scheduled for deletion. I think we should explain in the comment under which conditions the profile can be scheduled for deletion, and maybe also be more explicit about the action that is performed. Something like: // Schedule a profile for deletion if it isn't already scheduled. // Returns whether the profile has been newly scheduled. bool MaybeScheduleProfileDirectoryForDeletion() https://codereview.chromium.org/2201793002/diff/60001/chrome/browser/profiles... chrome/browser/profiles/profile_manager.cc:218: void QueueProfileDirectoryForDeletion(const base::FilePath& path) { Should we maybe use the term "marked" for the second stage? It's already used in code, and it makes the difference to "scheduled" a bit clearer. Then this method would be MarkProfileDirectoryForDeletion(). https://codereview.chromium.org/2201793002/diff/60001/chrome/browser/profiles... chrome/browser/profiles/profile_manager.cc:225: bool IsProfileMarkedForDeletion(const base::FilePath& profile_path) { For consistency, this method should then probably be renamed to IsProfileDirectoryMarkedForDeletion().
On 2016/08/04 12:24:35, Bernhard Bauer wrote: > Thanks! Generally this looks good, but I think we can use somewhat clearer > naming and add more comments (God knows this code is already pretty messy as it > is...). > > https://codereview.chromium.org/2201793002/diff/60001/chrome/browser/profiles... > File chrome/browser/profiles/profile_manager.cc (right): > > https://codereview.chromium.org/2201793002/diff/60001/chrome/browser/profiles... > chrome/browser/profiles/profile_manager.cc:137: // Profile deletion can pass > thru two stages: scheduled for deletion when where > Nit: "through". Also, would it make sense to move this comment to the enum > definition, and maybe split it up to have a comment for each of the enum values? > > https://codereview.chromium.org/2201793002/diff/60001/chrome/browser/profiles... > chrome/browser/profiles/profile_manager.cc:138: // performing necessary activity > prior to deletion, and abandoned stage when it > I think we can also go into detail about what the necessary activity is -- it's > creating a new profile as a replacement when the last profile is scheduled to be > deleted, right? > > https://codereview.chromium.org/2201793002/diff/60001/chrome/browser/profiles... > chrome/browser/profiles/profile_manager.cc:210: // Return true if profile can be > scheduled for deletion. > I think we should explain in the comment under which conditions the profile can > be scheduled for deletion, and maybe also be more explicit about the action that > is performed. Something like: > > // Schedule a profile for deletion if it isn't already scheduled. > // Returns whether the profile has been newly scheduled. > bool MaybeScheduleProfileDirectoryForDeletion() > > https://codereview.chromium.org/2201793002/diff/60001/chrome/browser/profiles... > chrome/browser/profiles/profile_manager.cc:218: void > QueueProfileDirectoryForDeletion(const base::FilePath& path) { > Should we maybe use the term "marked" for the second stage? It's already used in > code, and it makes the difference to "scheduled" a bit clearer. Then this method > would be MarkProfileDirectoryForDeletion(). > > https://codereview.chromium.org/2201793002/diff/60001/chrome/browser/profiles... > chrome/browser/profiles/profile_manager.cc:225: bool > IsProfileMarkedForDeletion(const base::FilePath& profile_path) { > For consistency, this method should then probably be renamed to > IsProfileDirectoryMarkedForDeletion(). done.
https://codereview.chromium.org/2201793002/diff/80001/chrome/browser/profiles... File chrome/browser/profiles/profile_manager.cc (right): https://codereview.chromium.org/2201793002/diff/80001/chrome/browser/profiles... chrome/browser/profiles/profile_manager.cc:141: ABANDONED MARKED maybe?
On 2016/08/04 16:28:37, Bernhard Bauer wrote: > https://codereview.chromium.org/2201793002/diff/80001/chrome/browser/profiles... > File chrome/browser/profiles/profile_manager.cc (right): > > https://codereview.chromium.org/2201793002/diff/80001/chrome/browser/profiles... > chrome/browser/profiles/profile_manager.cc:141: ABANDONED > MARKED maybe? ABANDONED is better describe profile state, but MARKED is more consistent. done.
LGTM, thanks!
Thanks for iterating with bauerb@. lgtm as well.
The CQ bit was checked by palar@yandex-team.ru
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Fixed sole profile double deletion. It is still possible to perform double delete operation on sole profile After sole profile scheduled for deletion a new profile will be created asynchronously on blocking pool and only then FinishDeletingProfile will be called to set ProfilesToDelete entry. BUG=601049 R=anthonyvd@chromium.org, bauerb@chromium.org ========== to ========== Fixed sole profile double deletion. It is still possible to perform double delete operation on sole profile After sole profile scheduled for deletion a new profile will be created asynchronously on blocking pool and only then FinishDeletingProfile will be called to set ProfilesToDelete entry. BUG=601049 R=anthonyvd@chromium.org, bauerb@chromium.org Committed: https://crrev.com/c684d2e6774693c6b975e0ee7e7f3a134d5f31ea Cr-Commit-Position: refs/heads/master@{#409837} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/c684d2e6774693c6b975e0ee7e7f3a134d5f31ea Cr-Commit-Position: refs/heads/master@{#409837} |
